From d397981e139e628411852c2256d1492b5cb8b0a1 Mon Sep 17 00:00:00 2001 From: Rohan Thakkar Date: Tue, 28 Jul 2026 10:26:48 -0700 Subject: [PATCH] Add watch.labelSelector for label-scoped operator instances Allow several operator instances to run side by side, each managing a disjoint label-defined subset of CHI/CHK resources across the same namespaces: - new watch.labelSelector config field (standard Kubernetes label selector syntax) with WATCH_LABEL_SELECTOR env override; invalid selectors abort startup - new watch.requireLabelSelector guard (WATCH_LABEL_SELECTOR_REQUIRED env override): abort startup on an empty selector so a lost or typo'd selector fails loudly instead of silently watching everything - filter CHI/CHK informer events and reconciles by the selector; guard again inside reconcile since requests from owned objects bypass predicates, and confirm ownership on live CR state before any write - treat a label flip-away as unwatch (stop metrics, drop in-memory state) without running the deletion protocol, leaving the CR and its child objects intact for the operator that now matches - auto-exclude selector label keys from label propagation so shard re-assignment never restarts ClickHouse pods - filter metrics-exporter discovery by the same selector so metrics ownership follows reconcile ownership - count skipped CRs in clickhouse_operator_cr_skipped_by_label_selector - document the feature, extend the chopconf CRD schema, config templates, install manifests and helm chart Signed-off-by: Rohan Thakkar --- cmd/operator/app/thread_keeper_label_test.go | 75 ++++ config/config.yaml | 15 + deploy/builder/templates-config/config.yaml | 15 + ...l-template-01-section-crd-02-chopconf.yaml | 6 + ...onfigurations.clickhouse.altinity.com.yaml | 6 + .../clickhouse-operator/values.schema.json | 6 + deploy/helm/clickhouse-operator/values.yaml | 13 + .../clickhouse-operator-install-ansible.yaml | 21 ++ ...house-operator-install-bundle-v1beta1.yaml | 21 ++ .../clickhouse-operator-install-bundle.yaml | 21 ++ ...use-operator-install-template-v1beta1.yaml | 21 ++ .../clickhouse-operator-install-template.yaml | 21 ++ .../clickhouse-operator-install-tf.yaml | 21 ++ deploy/operator/parts/crd.yaml | 6 + docs/operator_configuration.md | 38 ++ .../v1/type_configuration_chop.go | 117 ++++++ .../v1/type_configuration_chop_watch_test.go | 352 ++++++++++++++++++ pkg/apis/deployment/env_vars.go | 6 + pkg/chop/choptest/choptest.go | 33 ++ pkg/chop/config_manager.go | 2 + pkg/controller/chi/controller-chk-watcher.go | 8 +- .../chi/controller-chk-watcher_label_test.go | 77 ++++ pkg/controller/chi/controller.go | 90 ++++- .../chi/controller_watch_label_test.go | 242 ++++++++++++ pkg/controller/chi/kube/cr.go | 9 + pkg/controller/chi/worker-deleter.go | 10 + pkg/controller/chi/worker-reconciler-chi.go | 13 +- pkg/controller/chi/worker.go | 37 +- pkg/controller/chk/controller.go | 33 ++ .../chk/controller_watch_label_test.go | 234 ++++++++++++ pkg/controller/chk/kube/cr.go | 9 + pkg/controller/chk/kube/cr_test.go | 161 ++++++++ pkg/controller/chk/worker-deleter.go | 10 + pkg/controller/chk/worker-reconciler-chk.go | 10 - pkg/controller/chk/worker.go | 20 +- pkg/metrics/clickhouse/exporter.go | 6 + .../clickhouse/exporter_watch_label_test.go | 71 ++++ pkg/metrics/operator/label_selector_skips.go | 51 +++ pkg/metrics/operator/machinery.go | 23 ++ 39 files changed, 1882 insertions(+), 48 deletions(-) create mode 100644 cmd/operator/app/thread_keeper_label_test.go create mode 100644 pkg/chop/choptest/choptest.go create mode 100644 pkg/controller/chi/controller-chk-watcher_label_test.go create mode 100644 pkg/controller/chi/controller_watch_label_test.go create mode 100644 pkg/controller/chk/controller_watch_label_test.go create mode 100644 pkg/controller/chk/kube/cr_test.go create mode 100644 pkg/metrics/clickhouse/exporter_watch_label_test.go create mode 100644 pkg/metrics/operator/label_selector_skips.go diff --git a/cmd/operator/app/thread_keeper_label_test.go b/cmd/operator/app/thread_keeper_label_test.go new file mode 100644 index 000000000..fa9e11893 --- /dev/null +++ b/cmd/operator/app/thread_keeper_label_test.go @@ -0,0 +1,75 @@ +package app + +import ( + "testing" + + meta "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/event" + + api "github.com/altinity/clickhouse-operator/pkg/apis/clickhouse-keeper.altinity.com/v1" + "github.com/altinity/clickhouse-operator/pkg/chop/choptest" +) + +const testShardKey = choptest.ShardLabelKey + +var setWatchLabelSelector = choptest.SetWatchLabelSelector + +func newLabeledCHK(labels map[string]string) *api.ClickHouseKeeperInstallation { + return &api.ClickHouseKeeperInstallation{ + ObjectMeta: meta.ObjectMeta{ + Namespace: "clickhouse", + Name: "test-chk", + Labels: labels, + }, + } +} + +func Test_keeperPredicateWithLabelSelector(t *testing.T) { + tests := []struct { + name string + selector string + labels map[string]string + want bool + }{ + {"shard operator passes matching CHK", testShardKey + "=stg", map[string]string{testShardKey: "stg"}, true}, + {"shard operator filters other shard's CHK", testShardKey + "=stg", map[string]string{testShardKey: "logs"}, false}, + {"shard operator filters unlabeled CHK", testShardKey + "=stg", nil, false}, + {"legacy operator passes unlabeled CHK", "!" + testShardKey, nil, true}, + {"legacy operator filters shard-labeled CHK", "!" + testShardKey, map[string]string{testShardKey: "stg"}, false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + setWatchLabelSelector(t, tt.selector) + predicate := keeperPredicate() + chk := newLabeledCHK(tt.labels) + + if got := predicate.Create(event.CreateEvent{Object: chk}); got != tt.want { + t.Errorf("keeperPredicate.Create() = %v, want %v", got, tt.want) + } + if got := predicate.Update(event.UpdateEvent{ObjectNew: chk}); got != tt.want { + t.Errorf("keeperPredicate.Update() = %v, want %v", got, tt.want) + } + }) + } +} + +// A label flip arrives at both operators as a plain Update: the losing operator filters it +// (no delete flow), the gaining operator processes it as a normal reconcile. +func Test_keeperLabelFlipIsNotDelete(t *testing.T) { + oldCHK := newLabeledCHK(nil) + newCHK := newLabeledCHK(map[string]string{testShardKey: "stg"}) + + t.Run("losing operator filters the flip update", func(t *testing.T) { + setWatchLabelSelector(t, "!"+testShardKey) + if keeperPredicate().Update(event.UpdateEvent{ObjectOld: oldCHK, ObjectNew: newCHK}) { + t.Error("operator losing a CHK on label flip must filter the update") + } + }) + + t.Run("gaining operator processes the flip update", func(t *testing.T) { + setWatchLabelSelector(t, testShardKey+"=stg") + if !keeperPredicate().Update(event.UpdateEvent{ObjectOld: oldCHK, ObjectNew: newCHK}) { + t.Error("operator gaining a CHK on label flip must process the update") + } + }) +} diff --git a/config/config.yaml b/config/config.yaml index c932c84d1..226aae704 100644 --- a/config/config.yaml +++ b/config/config.yaml @@ -29,6 +29,21 @@ watch: include: [] exclude: [] + # Restricts which CHI/CHK resources this operator manages (label selector syntax, + # e.g. "example.com/clickhouse-shard=logs" or "!example.com/clickhouse-shard"). Empty manages everything. + # Env var override: WATCH_LABEL_SELECTOR. Invalid selector aborts startup. + # Note: an empty env var does NOT clear a file-configured selector (same semantics + # as WATCH_NAMESPACES) — de-sharding requires a file config change. + # Every label key referenced by the selector is automatically appended to + # `label.exclude` below: ownership labels never propagate to child objects, so + # re-assigning a CR to another operator never restarts its pods. + labelSelector: "" + + # When true, an empty labelSelector aborts startup. Set on sharded operator deployments + # so a lost/typo'd selector fails loudly instead of silently watching everything. + # Env var override: WATCH_LABEL_SELECTOR_REQUIRED. + requireLabelSelector: false + # Behavior when ClickHouseOperatorConfiguration changes: none | restart configuration: onChange: restart diff --git a/deploy/builder/templates-config/config.yaml b/deploy/builder/templates-config/config.yaml index de55a41d8..2a62a2cf4 100644 --- a/deploy/builder/templates-config/config.yaml +++ b/deploy/builder/templates-config/config.yaml @@ -23,6 +23,21 @@ watch: include: [${WATCH_NAMESPACES}] exclude: [] + # Restricts which CHI/CHK resources this operator manages (label selector syntax, + # e.g. "example.com/clickhouse-shard=logs" or "!example.com/clickhouse-shard"). Empty manages everything. + # Env var override: WATCH_LABEL_SELECTOR. Invalid selector aborts startup. + # Note: an empty env var does NOT clear a file-configured selector (same semantics + # as WATCH_NAMESPACES) — de-sharding requires a file config change. + # Every label key referenced by the selector is automatically appended to + # `label.exclude` below: ownership labels never propagate to child objects, so + # re-assigning a CR to another operator never restarts its pods. + labelSelector: "" + + # When true, an empty labelSelector aborts startup. Set on sharded operator deployments + # so a lost/typo'd selector fails loudly instead of silently watching everything. + # Env var override: WATCH_LABEL_SELECTOR_REQUIRED. + requireLabelSelector: false + # Behavior when ClickHouseOperatorConfiguration changes: none | restart configuration: onChange: restart diff --git a/deploy/builder/templates-install-bundle/clickhouse-operator-install-yaml-template-01-section-crd-02-chopconf.yaml b/deploy/builder/templates-install-bundle/clickhouse-operator-install-yaml-template-01-section-crd-02-chopconf.yaml index e2e49703c..a37de3052 100644 --- a/deploy/builder/templates-install-bundle/clickhouse-operator-install-yaml-template-01-section-crd-02-chopconf.yaml +++ b/deploy/builder/templates-install-bundle/clickhouse-operator-install-yaml-template-01-section-crd-02-chopconf.yaml @@ -56,6 +56,12 @@ spec: type: object description: "List of namespaces where clickhouse-operator watches for events." x-kubernetes-preserve-unknown-fields: true + labelSelector: + type: string + description: "Label selector (standard Kubernetes label selector syntax) restricting which CHI/CHK resources this operator instance manages. Empty selector manages everything. Env var override: WATCH_LABEL_SELECTOR." + requireLabelSelector: + type: boolean + description: "When true, an empty labelSelector aborts operator startup. Env var override: WATCH_LABEL_SELECTOR_REQUIRED." configuration: type: object description: "Behavior when ClickHouseOperatorConfiguration resources change" diff --git a/deploy/helm/clickhouse-operator/crds/CustomResourceDefinition-clickhouseoperatorconfigurations.clickhouse.altinity.com.yaml b/deploy/helm/clickhouse-operator/crds/CustomResourceDefinition-clickhouseoperatorconfigurations.clickhouse.altinity.com.yaml index 6073e41a8..90afadf81 100644 --- a/deploy/helm/clickhouse-operator/crds/CustomResourceDefinition-clickhouseoperatorconfigurations.clickhouse.altinity.com.yaml +++ b/deploy/helm/clickhouse-operator/crds/CustomResourceDefinition-clickhouseoperatorconfigurations.clickhouse.altinity.com.yaml @@ -56,6 +56,12 @@ spec: type: object description: "List of namespaces where clickhouse-operator watches for events." x-kubernetes-preserve-unknown-fields: true + labelSelector: + type: string + description: "Label selector (standard Kubernetes label selector syntax) restricting which CHI/CHK resources this operator instance manages. Empty selector manages everything. Env var override: WATCH_LABEL_SELECTOR." + requireLabelSelector: + type: boolean + description: "When true, an empty labelSelector aborts operator startup. Env var override: WATCH_LABEL_SELECTOR_REQUIRED." configuration: type: object description: "Behavior when ClickHouseOperatorConfiguration resources change" diff --git a/deploy/helm/clickhouse-operator/values.schema.json b/deploy/helm/clickhouse-operator/values.schema.json index f51eda897..cca78417d 100644 --- a/deploy/helm/clickhouse-operator/values.schema.json +++ b/deploy/helm/clickhouse-operator/values.schema.json @@ -614,6 +614,12 @@ "namespaces": { "type": ["array", "object"] }, + "labelSelector": { + "type": "string" + }, + "requireLabelSelector": { + "type": "boolean" + }, "configuration": { "type": "object", "properties": { diff --git a/deploy/helm/clickhouse-operator/values.yaml b/deploy/helm/clickhouse-operator/values.yaml index a0b60c024..13fbbf9cb 100644 --- a/deploy/helm/clickhouse-operator/values.yaml +++ b/deploy/helm/clickhouse-operator/values.yaml @@ -337,6 +337,19 @@ configs: namespaces: include: [] exclude: [] + # Restricts which CHI/CHK resources this operator manages (label selector syntax, + # e.g. "example.com/clickhouse-shard=logs" or "!example.com/clickhouse-shard"). Empty manages everything. + # Env var override: WATCH_LABEL_SELECTOR. Invalid selector aborts startup. + # Note: an empty env var does NOT clear a file-configured selector (same semantics + # as WATCH_NAMESPACES) — de-sharding requires a file config change. + # Every label key referenced by the selector is automatically appended to + # `label.exclude` below: ownership labels never propagate to child objects, so + # re-assigning a CR to another operator never restarts its pods. + labelSelector: "" + # When true, an empty labelSelector aborts startup. Set on sharded operator deployments + # so a lost/typo'd selector fails loudly instead of silently watching everything. + # Env var override: WATCH_LABEL_SELECTOR_REQUIRED. + requireLabelSelector: false # Behavior when ClickHouseOperatorConfiguration changes: none | restart configuration: onChange: restart diff --git a/deploy/operator/clickhouse-operator-install-ansible.yaml b/deploy/operator/clickhouse-operator-install-ansible.yaml index bcc35d782..7bee56769 100644 --- a/deploy/operator/clickhouse-operator-install-ansible.yaml +++ b/deploy/operator/clickhouse-operator-install-ansible.yaml @@ -3769,6 +3769,12 @@ spec: type: object description: "List of namespaces where clickhouse-operator watches for events." x-kubernetes-preserve-unknown-fields: true + labelSelector: + type: string + description: "Label selector (standard Kubernetes label selector syntax) restricting which CHI/CHK resources this operator instance manages. Empty selector manages everything. Env var override: WATCH_LABEL_SELECTOR." + requireLabelSelector: + type: boolean + description: "When true, an empty labelSelector aborts operator startup. Env var override: WATCH_LABEL_SELECTOR_REQUIRED." configuration: type: object description: "Behavior when ClickHouseOperatorConfiguration resources change" @@ -5720,6 +5726,21 @@ data: include: [{{ namespace }}] exclude: [] + # Restricts which CHI/CHK resources this operator manages (label selector syntax, + # e.g. "example.com/clickhouse-shard=logs" or "!example.com/clickhouse-shard"). Empty manages everything. + # Env var override: WATCH_LABEL_SELECTOR. Invalid selector aborts startup. + # Note: an empty env var does NOT clear a file-configured selector (same semantics + # as WATCH_NAMESPACES) — de-sharding requires a file config change. + # Every label key referenced by the selector is automatically appended to + # `label.exclude` below: ownership labels never propagate to child objects, so + # re-assigning a CR to another operator never restarts its pods. + labelSelector: "" + + # When true, an empty labelSelector aborts startup. Set on sharded operator deployments + # so a lost/typo'd selector fails loudly instead of silently watching everything. + # Env var override: WATCH_LABEL_SELECTOR_REQUIRED. + requireLabelSelector: false + # Behavior when ClickHouseOperatorConfiguration changes: none | restart configuration: onChange: restart diff --git a/deploy/operator/clickhouse-operator-install-bundle-v1beta1.yaml b/deploy/operator/clickhouse-operator-install-bundle-v1beta1.yaml index dd1f3724c..ddba52a96 100644 --- a/deploy/operator/clickhouse-operator-install-bundle-v1beta1.yaml +++ b/deploy/operator/clickhouse-operator-install-bundle-v1beta1.yaml @@ -3736,6 +3736,12 @@ spec: type: object description: "List of namespaces where clickhouse-operator watches for events." x-kubernetes-preserve-unknown-fields: true + labelSelector: + type: string + description: "Label selector (standard Kubernetes label selector syntax) restricting which CHI/CHK resources this operator instance manages. Empty selector manages everything. Env var override: WATCH_LABEL_SELECTOR." + requireLabelSelector: + type: boolean + description: "When true, an empty labelSelector aborts operator startup. Env var override: WATCH_LABEL_SELECTOR_REQUIRED." configuration: type: object description: "Behavior when ClickHouseOperatorConfiguration resources change" @@ -5919,6 +5925,21 @@ data: include: [] exclude: [] + # Restricts which CHI/CHK resources this operator manages (label selector syntax, + # e.g. "example.com/clickhouse-shard=logs" or "!example.com/clickhouse-shard"). Empty manages everything. + # Env var override: WATCH_LABEL_SELECTOR. Invalid selector aborts startup. + # Note: an empty env var does NOT clear a file-configured selector (same semantics + # as WATCH_NAMESPACES) — de-sharding requires a file config change. + # Every label key referenced by the selector is automatically appended to + # `label.exclude` below: ownership labels never propagate to child objects, so + # re-assigning a CR to another operator never restarts its pods. + labelSelector: "" + + # When true, an empty labelSelector aborts startup. Set on sharded operator deployments + # so a lost/typo'd selector fails loudly instead of silently watching everything. + # Env var override: WATCH_LABEL_SELECTOR_REQUIRED. + requireLabelSelector: false + # Behavior when ClickHouseOperatorConfiguration changes: none | restart configuration: onChange: restart diff --git a/deploy/operator/clickhouse-operator-install-bundle.yaml b/deploy/operator/clickhouse-operator-install-bundle.yaml index be12c80dd..25cd338e5 100644 --- a/deploy/operator/clickhouse-operator-install-bundle.yaml +++ b/deploy/operator/clickhouse-operator-install-bundle.yaml @@ -3762,6 +3762,12 @@ spec: type: object description: "List of namespaces where clickhouse-operator watches for events." x-kubernetes-preserve-unknown-fields: true + labelSelector: + type: string + description: "Label selector (standard Kubernetes label selector syntax) restricting which CHI/CHK resources this operator instance manages. Empty selector manages everything. Env var override: WATCH_LABEL_SELECTOR." + requireLabelSelector: + type: boolean + description: "When true, an empty labelSelector aborts operator startup. Env var override: WATCH_LABEL_SELECTOR_REQUIRED." configuration: type: object description: "Behavior when ClickHouseOperatorConfiguration resources change" @@ -5979,6 +5985,21 @@ data: include: [] exclude: [] + # Restricts which CHI/CHK resources this operator manages (label selector syntax, + # e.g. "example.com/clickhouse-shard=logs" or "!example.com/clickhouse-shard"). Empty manages everything. + # Env var override: WATCH_LABEL_SELECTOR. Invalid selector aborts startup. + # Note: an empty env var does NOT clear a file-configured selector (same semantics + # as WATCH_NAMESPACES) — de-sharding requires a file config change. + # Every label key referenced by the selector is automatically appended to + # `label.exclude` below: ownership labels never propagate to child objects, so + # re-assigning a CR to another operator never restarts its pods. + labelSelector: "" + + # When true, an empty labelSelector aborts startup. Set on sharded operator deployments + # so a lost/typo'd selector fails loudly instead of silently watching everything. + # Env var override: WATCH_LABEL_SELECTOR_REQUIRED. + requireLabelSelector: false + # Behavior when ClickHouseOperatorConfiguration changes: none | restart configuration: onChange: restart diff --git a/deploy/operator/clickhouse-operator-install-template-v1beta1.yaml b/deploy/operator/clickhouse-operator-install-template-v1beta1.yaml index 6d7b55de3..2237275ee 100644 --- a/deploy/operator/clickhouse-operator-install-template-v1beta1.yaml +++ b/deploy/operator/clickhouse-operator-install-template-v1beta1.yaml @@ -3736,6 +3736,12 @@ spec: type: object description: "List of namespaces where clickhouse-operator watches for events." x-kubernetes-preserve-unknown-fields: true + labelSelector: + type: string + description: "Label selector (standard Kubernetes label selector syntax) restricting which CHI/CHK resources this operator instance manages. Empty selector manages everything. Env var override: WATCH_LABEL_SELECTOR." + requireLabelSelector: + type: boolean + description: "When true, an empty labelSelector aborts operator startup. Env var override: WATCH_LABEL_SELECTOR_REQUIRED." configuration: type: object description: "Behavior when ClickHouseOperatorConfiguration resources change" @@ -5666,6 +5672,21 @@ data: include: [] exclude: [] + # Restricts which CHI/CHK resources this operator manages (label selector syntax, + # e.g. "example.com/clickhouse-shard=logs" or "!example.com/clickhouse-shard"). Empty manages everything. + # Env var override: WATCH_LABEL_SELECTOR. Invalid selector aborts startup. + # Note: an empty env var does NOT clear a file-configured selector (same semantics + # as WATCH_NAMESPACES) — de-sharding requires a file config change. + # Every label key referenced by the selector is automatically appended to + # `label.exclude` below: ownership labels never propagate to child objects, so + # re-assigning a CR to another operator never restarts its pods. + labelSelector: "" + + # When true, an empty labelSelector aborts startup. Set on sharded operator deployments + # so a lost/typo'd selector fails loudly instead of silently watching everything. + # Env var override: WATCH_LABEL_SELECTOR_REQUIRED. + requireLabelSelector: false + # Behavior when ClickHouseOperatorConfiguration changes: none | restart configuration: onChange: restart diff --git a/deploy/operator/clickhouse-operator-install-template.yaml b/deploy/operator/clickhouse-operator-install-template.yaml index a2240a7d9..64386e2b8 100644 --- a/deploy/operator/clickhouse-operator-install-template.yaml +++ b/deploy/operator/clickhouse-operator-install-template.yaml @@ -3762,6 +3762,12 @@ spec: type: object description: "List of namespaces where clickhouse-operator watches for events." x-kubernetes-preserve-unknown-fields: true + labelSelector: + type: string + description: "Label selector (standard Kubernetes label selector syntax) restricting which CHI/CHK resources this operator instance manages. Empty selector manages everything. Env var override: WATCH_LABEL_SELECTOR." + requireLabelSelector: + type: boolean + description: "When true, an empty labelSelector aborts operator startup. Env var override: WATCH_LABEL_SELECTOR_REQUIRED." configuration: type: object description: "Behavior when ClickHouseOperatorConfiguration resources change" @@ -5713,6 +5719,21 @@ data: include: [] exclude: [] + # Restricts which CHI/CHK resources this operator manages (label selector syntax, + # e.g. "example.com/clickhouse-shard=logs" or "!example.com/clickhouse-shard"). Empty manages everything. + # Env var override: WATCH_LABEL_SELECTOR. Invalid selector aborts startup. + # Note: an empty env var does NOT clear a file-configured selector (same semantics + # as WATCH_NAMESPACES) — de-sharding requires a file config change. + # Every label key referenced by the selector is automatically appended to + # `label.exclude` below: ownership labels never propagate to child objects, so + # re-assigning a CR to another operator never restarts its pods. + labelSelector: "" + + # When true, an empty labelSelector aborts startup. Set on sharded operator deployments + # so a lost/typo'd selector fails loudly instead of silently watching everything. + # Env var override: WATCH_LABEL_SELECTOR_REQUIRED. + requireLabelSelector: false + # Behavior when ClickHouseOperatorConfiguration changes: none | restart configuration: onChange: restart diff --git a/deploy/operator/clickhouse-operator-install-tf.yaml b/deploy/operator/clickhouse-operator-install-tf.yaml index 5c841acea..3baa13279 100644 --- a/deploy/operator/clickhouse-operator-install-tf.yaml +++ b/deploy/operator/clickhouse-operator-install-tf.yaml @@ -3769,6 +3769,12 @@ spec: type: object description: "List of namespaces where clickhouse-operator watches for events." x-kubernetes-preserve-unknown-fields: true + labelSelector: + type: string + description: "Label selector (standard Kubernetes label selector syntax) restricting which CHI/CHK resources this operator instance manages. Empty selector manages everything. Env var override: WATCH_LABEL_SELECTOR." + requireLabelSelector: + type: boolean + description: "When true, an empty labelSelector aborts operator startup. Env var override: WATCH_LABEL_SELECTOR_REQUIRED." configuration: type: object description: "Behavior when ClickHouseOperatorConfiguration resources change" @@ -5720,6 +5726,21 @@ data: include: [${namespace}] exclude: [] + # Restricts which CHI/CHK resources this operator manages (label selector syntax, + # e.g. "example.com/clickhouse-shard=logs" or "!example.com/clickhouse-shard"). Empty manages everything. + # Env var override: WATCH_LABEL_SELECTOR. Invalid selector aborts startup. + # Note: an empty env var does NOT clear a file-configured selector (same semantics + # as WATCH_NAMESPACES) — de-sharding requires a file config change. + # Every label key referenced by the selector is automatically appended to + # `label.exclude` below: ownership labels never propagate to child objects, so + # re-assigning a CR to another operator never restarts its pods. + labelSelector: "" + + # When true, an empty labelSelector aborts startup. Set on sharded operator deployments + # so a lost/typo'd selector fails loudly instead of silently watching everything. + # Env var override: WATCH_LABEL_SELECTOR_REQUIRED. + requireLabelSelector: false + # Behavior when ClickHouseOperatorConfiguration changes: none | restart configuration: onChange: restart diff --git a/deploy/operator/parts/crd.yaml b/deploy/operator/parts/crd.yaml index 397cfd9cc..2f8e3297a 100644 --- a/deploy/operator/parts/crd.yaml +++ b/deploy/operator/parts/crd.yaml @@ -8306,6 +8306,12 @@ spec: type: object description: "List of namespaces where clickhouse-operator watches for events." x-kubernetes-preserve-unknown-fields: true + labelSelector: + type: string + description: "Label selector (standard Kubernetes label selector syntax) restricting which CHI/CHK resources this operator instance manages. Empty selector manages everything. Env var override: WATCH_LABEL_SELECTOR." + requireLabelSelector: + type: boolean + description: "When true, an empty labelSelector aborts operator startup. Env var override: WATCH_LABEL_SELECTOR_REQUIRED." configuration: type: object description: "Behavior when ClickHouseOperatorConfiguration resources change" diff --git a/docs/operator_configuration.md b/docs/operator_configuration.md index 7e4d58af1..e94d2c94a 100644 --- a/docs/operator_configuration.md +++ b/docs/operator_configuration.md @@ -271,5 +271,43 @@ The per-component TLS knobs `clickhouse.tls` and `zookeeper.tls` use 3-level inh See [security_hardening.md](security_hardening.md) for per-knob semantics, the `security.policy: Enforced` master switch, the orthogonal-axes posture table, and the externally-managed-token (Secret-backed) GitOps pattern. FIPS-specific controls (`security.fips.enforced` cryptographic-module gate, `security.images.policy: FIPSRequired` workload supply-chain gate, FIPS coercion details, ACVP responder, FIPS build and release evidence) are documented in [security_hardening_fips.md](security_hardening_fips.md). +## Sharding CHI/CHK resources across operator instances + +By default a single operator instance manages every `ClickHouseInstallation` (CHI) and `ClickHouseKeeperInstallation` (CHK) in its watched namespaces. In large fleets this makes one operator a scaling and blast-radius bottleneck: a slow reconcile of one CR delays all others, and an operator bug or bad rollout affects the whole fleet at once. + +`watch.labelSelector` lets several operator instances run side by side, each managing a disjoint, label-defined subset ("shard") of CRs across the same namespaces: + +```yaml +watch: + # Standard Kubernetes label selector syntax. + # This operator instance only manages CRs whose labels match. + labelSelector: "example.com/clickhouse-shard=shard-a" + + # Abort startup when labelSelector is empty. Recommended on sharded + # deployments so a lost or typo'd selector fails loudly instead of the + # operator silently taking over the whole fleet. + requireLabelSelector: true +``` + +A typical two-shard layout plus a legacy catch-all: + +| Operator instance | `watch.labelSelector` | Manages | +|---|---|---| +| `clickhouse-operator-shard-a` | `example.com/clickhouse-shard=shard-a` | CRs labeled `shard-a` | +| `clickhouse-operator-shard-b` | `example.com/clickhouse-shard=shard-b` | CRs labeled `shard-b` | +| `clickhouse-operator` (legacy) | `!example.com/clickhouse-shard` | unlabeled CRs | + +Behavior details: + +* **Standard selector syntax.** Anything `k8s.io/apimachinery`'s `labels.Parse` accepts: equality (`k=v`), set-based (`k in (a,b)`), presence (`k`), absence (`!k`), and conjunctions (`k=v,other!=x`). An invalid selector aborts startup. +* **Environment overrides.** `WATCH_LABEL_SELECTOR` overrides the file value; `WATCH_LABEL_SELECTOR_REQUIRED` overrides `requireLabelSelector`. An *empty* `WATCH_LABEL_SELECTOR` env var does **not** clear a file-configured selector (same semantics as `WATCH_NAMESPACES`), so removing a selector requires a config file change — an accidentally-unset env var cannot silently widen an operator's scope. +* **Applies to CHI and CHK resources and to the metrics-exporter**: each exporter only scrapes the CHIs its operator instance manages, so metrics ownership follows reconcile ownership. `ClickHouseInstallationTemplate`s are not filtered — templates remain visible to every operator instance. +* **No pod restarts on re-assignment.** Every label key referenced by the selector is automatically appended to `label.exclude`, so shard-assignment labels never propagate from the CR to child objects (StatefulSets, Pods, Services, ConfigMaps). Flipping a CR's shard label hands it to another operator without touching running ClickHouse pods. +* **Flip-away handling.** When a CR's labels stop matching, the losing operator treats it as an unwatch (stops metrics scraping, drops it from in-memory state) without running the deletion protocol — the CR and its child objects are left intact for the operator that now matches. +* **Live-state confirmation before writes.** Reconciles triggered by cached/stale events re-check ownership against the live CR state before any write (finalizer install, child-object reconcile, deletion), so two operators do not fight over a CR during a label flip. +* **Observability.** CRs skipped due to a selector mismatch are counted in the `clickhouse_operator_cr_skipped_by_label_selector` metric and logged at debug verbosity. + +Selectors on different operator instances should be mutually exclusive — a CR matching two operators would be reconciled by both. A disjoint scheme like the table above (one value per shard plus an absence-based catch-all) guarantees every possible label state matches exactly one operator. + [clickhouse-operator-install-bundle.yaml]: ../deploy/operator/clickhouse-operator-install-bundle.yaml [70-chop-config.yaml]: ./chi-examples/70-chop-config.yaml diff --git a/pkg/apis/clickhouse.altinity.com/v1/type_configuration_chop.go b/pkg/apis/clickhouse.altinity.com/v1/type_configuration_chop.go index a11de3b1b..53604466c 100644 --- a/pkg/apis/clickhouse.altinity.com/v1/type_configuration_chop.go +++ b/pkg/apis/clickhouse.altinity.com/v1/type_configuration_chop.go @@ -31,6 +31,7 @@ import ( "gopkg.in/yaml.v3" meta "k8s.io/apimachinery/pkg/apis/meta/v1" + kubeLabels "k8s.io/apimachinery/pkg/labels" "github.com/altinity/clickhouse-operator/pkg/apis/common/types" "github.com/altinity/clickhouse-operator/pkg/apis/deployment" @@ -256,8 +257,21 @@ type OperatorConfigWatch struct { // Namespaces where operator watches for events Namespaces OperatorConfigWatchNamespaces `json:"namespaces" yaml:"namespaces"` + // LabelSelector restricts which CHI/CHK custom resources this operator instance manages + // (standard label selector syntax, e.g. "example.com/clickhouse-shard=logs" or "!example.com/clickhouse-shard"). + // Filtering is applied CLIENT-SIDE only, never as a server-side watch selector. + // Empty (default) manages all CRs in the watched namespaces. + LabelSelector string `json:"labelSelector,omitempty" yaml:"labelSelector,omitempty"` + + // RequireLabelSelector makes an empty LabelSelector a fatal startup error, so a sharded + // operator that loses its selector crashes instead of silently watching everything. + RequireLabelSelector bool `json:"requireLabelSelector,omitempty" yaml:"requireLabelSelector,omitempty"` + // Configuration specifies behavior related to ClickHouseOperatorConfiguration watches Configuration OperatorConfigWatchConfiguration `json:"configuration,omitempty" yaml:"configuration,omitempty"` + + // parsed form of LabelSelector; nil means match everything + labelSelector kubeLabels.Selector } // OperatorConfigWatchConfiguration specifies behavior when operator-wide configuration changes. @@ -1038,6 +1052,12 @@ func (c *OperatorConfig) MergeFrom(from *OperatorConfig) error { } excludeRegexp := from.ClickHouse.Metrics.ExcludeRegexp + + // Selector fields define this instance's identity and must not come from CR-based configs, + // which every operator in the namespace merges. File config and env are the only channels. + labelSelector := c.Watch.LabelSelector + requireLabelSelector := c.Watch.RequireLabelSelector + if err := mergo.Merge(c, *from, mergo.WithAppendSlice, mergo.WithOverride); err != nil { return fmt.Errorf("FAIL merge config Error: %q", err) } @@ -1048,6 +1068,15 @@ func (c *OperatorConfig) MergeFrom(from *OperatorConfig) error { c.ClickHouse.Metrics.ExcludeRegexp = slices.Clone(excludeRegexp) } + if from.Watch.LabelSelector != "" { + log.Warningf("ignoring watch.labelSelector %q from CR-based config: selector may only be set via file config or env", from.Watch.LabelSelector) + } + if from.Watch.RequireLabelSelector { + log.Warningf("ignoring watch.requireLabelSelector from CR-based config: it may only be set via file config or env") + } + c.Watch.LabelSelector = labelSelector + c.Watch.RequireLabelSelector = requireLabelSelector + return nil } @@ -1216,6 +1245,80 @@ func (c *OperatorConfig) Postprocess() { c.readCHITemplates() c.applyEnvVarParams() c.applyDefaultWatchNamespace() + c.applyWatchLabelSelector() +} + +// applyWatchLabelSelector parses and validates watch.labelSelector. Failures abort startup: +// falling back to match-all would cause dual management, match-none would orphan CRs. +func (c *OperatorConfig) applyWatchLabelSelector() { + if err := c.ValidateWatchLabelSelector(); err != nil { + log.Fatalf("%v", err) + } + c.excludeWatchLabelSelectorKeysFromPropagation() +} + +// excludeWatchLabelSelectorKeysFromPropagation appends every label key referenced by +// watch.labelSelector to label.exclude. Ownership labels must never propagate from the CR +// to child objects: if the shard label reached the StatefulSet pod template, re-assigning +// a CR to another operator (a label change) would roll-restart its ClickHouse pods. +// Must run after applyEnvVarParams, which may overwrite Label.Exclude from the deprecated +// ExcludeFromPropagationLabels field. +func (c *OperatorConfig) excludeWatchLabelSelectorKeysFromPropagation() { + if c.Watch.labelSelector == nil { + return + } + requirements, _ := c.Watch.labelSelector.Requirements() + for _, requirement := range requirements { + key := requirement.Key() + if !util.StringSliceContains(c.Label.Exclude, key) { + c.Label.Exclude = append(c.Label.Exclude, key) + } + } +} + +// ValidateWatchLabelSelector parses watch.labelSelector and enforces watch.requireLabelSelector. +func (c *OperatorConfig) ValidateWatchLabelSelector() error { + if err := c.ParseWatchLabelSelector(); err != nil { + return fmt.Errorf("invalid watch.labelSelector %q: %v", c.Watch.LabelSelector, err) + } + if c.Watch.RequireLabelSelector && !c.HasWatchLabelSelector() { + return fmt.Errorf("watch.requireLabelSelector is set but watch.labelSelector is empty") + } + return nil +} + +// ParseWatchLabelSelector parses and caches watch.labelSelector. Empty means match everything. +func (c *OperatorConfig) ParseWatchLabelSelector() error { + if strings.TrimSpace(c.Watch.LabelSelector) == "" { + c.Watch.labelSelector = nil + return nil + } + selector, err := kubeLabels.Parse(c.Watch.LabelSelector) + if err != nil { + return err + } + c.Watch.labelSelector = selector + return nil +} + +// HasWatchLabelSelector returns whether a watch label selector is configured +func (c *OperatorConfig) HasWatchLabelSelector() bool { + return c.Watch.labelSelector != nil +} + +// IsLabelSelectorWatched returns whether the given CR labels match watch.labelSelector. +// Always true when no selector is configured. +func (c *OperatorConfig) IsLabelSelectorWatched(lbls map[string]string) bool { + if c.Watch.labelSelector == nil { + return true + } + return c.Watch.labelSelector.Matches(kubeLabels.Set(lbls)) +} + +// IsCRWatched returns whether a CR in the given namespace with the given labels falls within +// this operator's watch scope (namespace filter AND label selector). +func (c *OperatorConfig) IsCRWatched(namespace string, lbls map[string]string) bool { + return c.IsNamespaceWatched(namespace) && c.IsLabelSelectorWatched(lbls) } func (c *OperatorConfig) normalizeSectionClickHouseConfigurationFile() { @@ -1606,6 +1709,20 @@ func (c *OperatorConfig) applyEnvVarParams() { c.Watch.Namespaces.Exclude = types.NewStrings(namespaces) } } + + if selector := os.Getenv(deployment.WATCH_LABEL_SELECTOR); len(selector) > 0 { + // We have WATCH_LABEL_SELECTOR explicitly specified + c.Watch.LabelSelector = selector + } + + if str := os.Getenv(deployment.WATCH_LABEL_SELECTOR_REQUIRED); len(str) > 0 { + // We have WATCH_LABEL_SELECTOR_REQUIRED explicitly specified + if required, err := strconv.ParseBool(str); err == nil { + c.Watch.RequireLabelSelector = required + } else { + log.Fatalf("invalid %s value %q: %v", deployment.WATCH_LABEL_SELECTOR_REQUIRED, str, err) + } + } } func (c *OperatorConfig) splitNamespaces(combined string) (namespaces []string) { diff --git a/pkg/apis/clickhouse.altinity.com/v1/type_configuration_chop_watch_test.go b/pkg/apis/clickhouse.altinity.com/v1/type_configuration_chop_watch_test.go index 99ded8924..3127d2520 100644 --- a/pkg/apis/clickhouse.altinity.com/v1/type_configuration_chop_watch_test.go +++ b/pkg/apis/clickhouse.altinity.com/v1/type_configuration_chop_watch_test.go @@ -19,6 +19,7 @@ import ( "github.com/stretchr/testify/require" + "github.com/altinity/clickhouse-operator/pkg/apis/common/types" "github.com/altinity/clickhouse-operator/pkg/apis/deployment" ) @@ -53,3 +54,354 @@ func TestApplyEnvVarParamsWatchNamespaces(t *testing.T) { }) } } + +const testShardKey = "example.com/clickhouse-shard" + +// An absent/empty selector must behave exactly like the pre-patch operator: +// every CR matches, regardless of its labels. +func TestWatchLabelSelectorBackwardCompat(t *testing.T) { + labelStates := []map[string]string{ + nil, + {}, + {testShardKey: "stg"}, + {testShardKey: "logs"}, + {"unrelated": "value"}, + } + + for _, selector := range []string{"", " "} { + config := &OperatorConfig{} + config.Watch.LabelSelector = selector + if err := config.ParseWatchLabelSelector(); err != nil { + t.Fatalf("ParseWatchLabelSelector(%q) unexpected error: %v", selector, err) + } + if config.HasWatchLabelSelector() { + t.Errorf("HasWatchLabelSelector() = true for empty selector %q, want false", selector) + } + for _, lbls := range labelStates { + if !config.IsLabelSelectorWatched(lbls) { + t.Errorf("IsLabelSelectorWatched(%v) = false with empty selector, want true (backward compat)", lbls) + } + if !config.IsCRWatched("any-namespace", lbls) { + t.Errorf("IsCRWatched(any-namespace, %v) = false with empty selector, want true", lbls) + } + } + } +} + +func TestParseWatchLabelSelectorValid(t *testing.T) { + tests := []struct { + selector string + match map[string]string + noMatch map[string]string + }{ + { + selector: testShardKey + "=stg", + match: map[string]string{testShardKey: "stg"}, + noMatch: map[string]string{testShardKey: "logs"}, + }, + { + selector: "!" + testShardKey, + match: map[string]string{"unrelated": "value"}, + noMatch: map[string]string{testShardKey: "stg"}, + }, + { + selector: testShardKey + " in (logs,ads)", + match: map[string]string{testShardKey: "ads"}, + noMatch: map[string]string{testShardKey: "stg"}, + }, + { + selector: testShardKey + "=stg,env=prod", + match: map[string]string{testShardKey: "stg", "env": "prod"}, + noMatch: map[string]string{testShardKey: "stg"}, + }, + } + for _, tt := range tests { + t.Run(tt.selector, func(t *testing.T) { + config := &OperatorConfig{} + config.Watch.LabelSelector = tt.selector + if err := config.ParseWatchLabelSelector(); err != nil { + t.Fatalf("ParseWatchLabelSelector(%q) unexpected error: %v", tt.selector, err) + } + if !config.HasWatchLabelSelector() { + t.Fatalf("HasWatchLabelSelector() = false after parsing %q, want true", tt.selector) + } + if !config.IsLabelSelectorWatched(tt.match) { + t.Errorf("IsLabelSelectorWatched(%v) = false under %q, want true", tt.match, tt.selector) + } + if config.IsLabelSelectorWatched(tt.noMatch) { + t.Errorf("IsLabelSelectorWatched(%v) = true under %q, want false", tt.noMatch, tt.selector) + } + // Unlabeled CRs must never match an equality/set selector + if tt.selector != "!"+testShardKey { + if config.IsLabelSelectorWatched(nil) { + t.Errorf("IsLabelSelectorWatched(nil) = true under %q, want false", tt.selector) + } + } + }) + } +} + +// An invalid selector must surface as a parse error (startup turns this into log.Fatalf), +// never silently fall back to match-all or match-none. +func TestParseWatchLabelSelectorInvalid(t *testing.T) { + invalid := []string{ + testShardKey + "===stg", + testShardKey + " in (", + "!!" + testShardKey, + testShardKey + "=val;ue", + "=stg", + } + for _, selector := range invalid { + t.Run(selector, func(t *testing.T) { + config := &OperatorConfig{} + config.Watch.LabelSelector = selector + if err := config.ParseWatchLabelSelector(); err == nil { + t.Errorf("ParseWatchLabelSelector(%q) = nil error, want parse failure (fail-fast)", selector) + } + if config.HasWatchLabelSelector() { + t.Errorf("HasWatchLabelSelector() = true after failed parse of %q, want false", selector) + } + }) + } +} + +func TestWatchLabelSelectorEnvVarOverride(t *testing.T) { + t.Setenv(deployment.WATCH_LABEL_SELECTOR, testShardKey+"=stg") + + config := &OperatorConfig{} + config.Watch.LabelSelector = "!" + testShardKey // file-based value, env must win + config.applyEnvVarParams() + + if config.Watch.LabelSelector != testShardKey+"=stg" { + t.Fatalf("WATCH_LABEL_SELECTOR env var did not override file config: got %q", config.Watch.LabelSelector) + } + if err := config.ParseWatchLabelSelector(); err != nil { + t.Fatalf("ParseWatchLabelSelector() unexpected error: %v", err) + } + if !config.IsLabelSelectorWatched(map[string]string{testShardKey: "stg"}) { + t.Error("env-var-provided selector not effective") + } +} + +func TestWatchLabelSelectorEnvVarAbsentKeepsFileValue(t *testing.T) { + t.Setenv(deployment.WATCH_LABEL_SELECTOR, "") + + config := &OperatorConfig{} + config.Watch.LabelSelector = "!" + testShardKey + config.applyEnvVarParams() + + if config.Watch.LabelSelector != "!"+testShardKey { + t.Fatalf("absent WATCH_LABEL_SELECTOR must keep file config, got %q", config.Watch.LabelSelector) + } +} + +// For the production scheme (shards use `example.com/clickhouse-shard=`, legacy uses +// `!example.com/clickhouse-shard`) every possible label state matches exactly one operator. +func TestWatchLabelSelectorDisjointness(t *testing.T) { + selectors := []string{ + testShardKey + "=stg", + testShardKey + "=logs", + testShardKey + "=ads", + "!" + testShardKey, + } + labelStates := []map[string]string{ + nil, + {}, + {testShardKey: "stg"}, + {testShardKey: "logs"}, + {testShardKey: "ads"}, + {"unrelated": "value"}, + {testShardKey: "stg", "unrelated": "value"}, + } + + countMatches := func(lbls map[string]string) int { + matches := 0 + for _, selector := range selectors { + config := &OperatorConfig{} + config.Watch.LabelSelector = selector + if err := config.ParseWatchLabelSelector(); err != nil { + t.Fatalf("ParseWatchLabelSelector(%q) unexpected error: %v", selector, err) + } + if config.IsLabelSelectorWatched(lbls) { + matches++ + } + } + return matches + } + + for _, lbls := range labelStates { + if matches := countMatches(lbls); matches != 1 { + t.Errorf("label state %v matched %d selectors, want exactly 1 (disjointness violated)", lbls, matches) + } + } + + // Documented gap: an unknown shard value matches ZERO operators (orphaned CHI). + // Prevented upstream by the charts CI allowlist, not by the operator. + if matches := countMatches(map[string]string{testShardKey: "no-such-shard"}); matches != 0 { + t.Errorf("unknown shard value matched %d selectors, want 0", matches) + } +} + +// requireLabelSelector converts a missing selector on a sharded operator into a startup error. +func TestValidateWatchLabelSelectorRequire(t *testing.T) { + tests := []struct { + name string + selector string + require bool + wantErr bool + }{ + {"require + selector set", testShardKey + "=stg", true, false}, + {"require + selector empty", "", true, true}, + {"no require + selector empty", "", false, false}, + {"require + invalid selector", "=stg", true, true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + config := &OperatorConfig{} + config.Watch.LabelSelector = tt.selector + config.Watch.RequireLabelSelector = tt.require + if err := config.ValidateWatchLabelSelector(); (err != nil) != tt.wantErr { + t.Errorf("ValidateWatchLabelSelector() error = %v, wantErr %v", err, tt.wantErr) + } + }) + } +} + +func TestWatchLabelSelectorRequiredEnvVar(t *testing.T) { + for _, val := range []string{"true", "1"} { + t.Setenv(deployment.WATCH_LABEL_SELECTOR_REQUIRED, val) + config := &OperatorConfig{} + config.applyEnvVarParams() + if !config.Watch.RequireLabelSelector { + t.Errorf("WATCH_LABEL_SELECTOR_REQUIRED=%q did not set RequireLabelSelector", val) + } + } + + t.Setenv(deployment.WATCH_LABEL_SELECTOR_REQUIRED, "false") + config := &OperatorConfig{} + config.Watch.RequireLabelSelector = true + config.applyEnvVarParams() + if config.Watch.RequireLabelSelector { + t.Error("WATCH_LABEL_SELECTOR_REQUIRED=false did not clear RequireLabelSelector") + } +} + +// A ClickHouseOperatorConfiguration CR is merged by every operator in the namespace, so it +// must never be able to set or change an instance's selector identity. +func TestMergeFromIgnoresCRLabelSelector(t *testing.T) { + tests := []struct { + name string + base string + fromSelector string + want string + }{ + {"CR cannot override file selector", testShardKey + "=stg", testShardKey + "=logs", testShardKey + "=stg"}, + {"CR cannot set selector on catch-all", "", testShardKey + "=logs", ""}, + {"CR without selector leaves file selector", testShardKey + "=stg", "", testShardKey + "=stg"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + base := &OperatorConfig{} + base.Watch.LabelSelector = tt.base + base.Watch.RequireLabelSelector = true + from := &OperatorConfig{} + from.Watch.LabelSelector = tt.fromSelector + if err := base.MergeFrom(from); err != nil { + t.Fatalf("MergeFrom() unexpected error: %v", err) + } + if base.Watch.LabelSelector != tt.want { + t.Errorf("LabelSelector after merge = %q, want %q", base.Watch.LabelSelector, tt.want) + } + if !base.Watch.RequireLabelSelector { + t.Error("RequireLabelSelector lost across merge") + } + }) + } +} + +// Every key referenced by watch.labelSelector must land in label.exclude so ownership +// labels never propagate to child objects (a propagated shard label would turn any +// re-shard label flip into a pod-template change and a rolling restart). +func TestWatchLabelSelectorKeysExcludedFromPropagation(t *testing.T) { + tests := []struct { + name string + selector string + existingExclude []string + wantExclude []string + }{ + {"equality selector", testShardKey + "=stg", nil, []string{testShardKey}}, + {"negation selector", "!" + testShardKey, nil, []string{testShardKey}}, + {"set selector", testShardKey + " in (logs,ads)", nil, []string{testShardKey}}, + {"multi-key selector", testShardKey + "=stg,env=prod", nil, []string{testShardKey, "env"}}, + {"existing excludes preserved", testShardKey + "=stg", []string{"team"}, []string{"team", testShardKey}}, + {"no duplicate append", testShardKey + "=stg", []string{testShardKey}, []string{testShardKey}}, + {"empty selector leaves excludes alone", "", []string{"team"}, []string{"team"}}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + config := &OperatorConfig{} + config.Watch.LabelSelector = tt.selector + config.Label.Exclude = tt.existingExclude + if err := config.ParseWatchLabelSelector(); err != nil { + t.Fatalf("ParseWatchLabelSelector(%q) unexpected error: %v", tt.selector, err) + } + config.excludeWatchLabelSelectorKeysFromPropagation() + + if len(config.Label.Exclude) != len(tt.wantExclude) { + t.Fatalf("Label.Exclude = %v, want %v", config.Label.Exclude, tt.wantExclude) + } + for _, key := range tt.wantExclude { + found := false + for _, have := range config.Label.Exclude { + if have == key { + found = true + break + } + } + if !found { + t.Errorf("Label.Exclude = %v, missing key %q", config.Label.Exclude, key) + } + } + }) + } +} + +// The exclusion must survive repeated normalization without growing the list. +func TestWatchLabelSelectorKeysExclusionIdempotent(t *testing.T) { + config := &OperatorConfig{} + config.Watch.LabelSelector = testShardKey + "=stg" + if err := config.ParseWatchLabelSelector(); err != nil { + t.Fatalf("ParseWatchLabelSelector() unexpected error: %v", err) + } + config.excludeWatchLabelSelectorKeysFromPropagation() + config.excludeWatchLabelSelectorKeysFromPropagation() + if len(config.Label.Exclude) != 1 || config.Label.Exclude[0] != testShardKey { + t.Errorf("Label.Exclude after double apply = %v, want [%s]", config.Label.Exclude, testShardKey) + } +} + +// IsCRWatched must be the conjunction of the namespace filter and the label selector filter. +func TestIsCRWatched(t *testing.T) { + config := &OperatorConfig{} + config.Watch.LabelSelector = testShardKey + "=stg" + if err := config.ParseWatchLabelSelector(); err != nil { + t.Fatalf("ParseWatchLabelSelector() unexpected error: %v", err) + } + config.Watch.Namespaces.Exclude = types.NewStrings([]string{"denied-ns"}) + + tests := []struct { + namespace string + lbls map[string]string + want bool + }{ + {"clickhouse", map[string]string{testShardKey: "stg"}, true}, + {"clickhouse", map[string]string{testShardKey: "logs"}, false}, + {"clickhouse", nil, false}, + {"denied-ns", map[string]string{testShardKey: "stg"}, false}, + } + for _, tt := range tests { + if got := config.IsCRWatched(tt.namespace, tt.lbls); got != tt.want { + t.Errorf("IsCRWatched(%q, %v) = %v, want %v", tt.namespace, tt.lbls, got, tt.want) + } + } +} diff --git a/pkg/apis/deployment/env_vars.go b/pkg/apis/deployment/env_vars.go index b85878d81..47b55ee75 100644 --- a/pkg/apis/deployment/env_vars.go +++ b/pkg/apis/deployment/env_vars.go @@ -50,6 +50,12 @@ const ( WATCH_NAMESPACES = "WATCH_NAMESPACES" // WATCH_NAMESPACES_EXCLUDE specifies namespaces that should be excluded from reconciliation WATCH_NAMESPACES_EXCLUDE = "WATCH_NAMESPACES_EXCLUDE" + // WATCH_LABEL_SELECTOR specifies a label selector restricting which CHI/CHK custom resources + // this operator instance manages (see watch.labelSelector in operator config) + WATCH_LABEL_SELECTOR = "WATCH_LABEL_SELECTOR" + // WATCH_LABEL_SELECTOR_REQUIRED makes an empty watch.labelSelector a fatal startup error + // (see watch.requireLabelSelector in operator config) + WATCH_LABEL_SELECTOR_REQUIRED = "WATCH_LABEL_SELECTOR_REQUIRED" // CHOP_CONFIG path to clickhouse operator configuration file CHOP_CONFIG = "CHOP_CONFIG" diff --git a/pkg/chop/choptest/choptest.go b/pkg/chop/choptest/choptest.go new file mode 100644 index 000000000..aa4c251c9 --- /dev/null +++ b/pkg/chop/choptest/choptest.go @@ -0,0 +1,33 @@ +// Package choptest provides shared helpers for tests exercising the global chop config. +package choptest + +import ( + "testing" + + "github.com/altinity/clickhouse-operator/pkg/chop" +) + +// ShardLabelKey is the shard label key used across watch label selector tests. +const ShardLabelKey = "example.com/clickhouse-shard" + +// EnsureInit initializes the global chop singleton if no test package has done so yet. +func EnsureInit() { + if chop.Get() == nil { + chop.New(nil, nil, "") + } +} + +// SetWatchLabelSelector sets watch.labelSelector on the global chop config for one test, +// restoring the selector-less default afterwards. +func SetWatchLabelSelector(t *testing.T, selector string) { + t.Helper() + EnsureInit() + chop.Config().Watch.LabelSelector = selector + if err := chop.Config().ParseWatchLabelSelector(); err != nil { + t.Fatalf("ParseWatchLabelSelector(%q) unexpected error: %v", selector, err) + } + t.Cleanup(func() { + chop.Config().Watch.LabelSelector = "" + _ = chop.Config().ParseWatchLabelSelector() + }) +} diff --git a/pkg/chop/config_manager.go b/pkg/chop/config_manager.go index 25952d682..d1745003b 100644 --- a/pkg/chop/config_manager.go +++ b/pkg/chop/config_manager.go @@ -361,6 +361,8 @@ func (cm *ConfigManager) listSupportedEnvVarNames() []string { deployment.WATCH_NAMESPACE, deployment.WATCH_NAMESPACES, deployment.WATCH_NAMESPACES_EXCLUDE, + deployment.WATCH_LABEL_SELECTOR, + deployment.WATCH_LABEL_SELECTOR_REQUIRED, } } diff --git a/pkg/controller/chi/controller-chk-watcher.go b/pkg/controller/chi/controller-chk-watcher.go index 5c9100dfe..3e854b0f3 100644 --- a/pkg/controller/chi/controller-chk-watcher.go +++ b/pkg/controller/chi/controller-chk-watcher.go @@ -158,11 +158,9 @@ func (c *Controller) enqueueDependentCHIs(chkNamespace, chkName string) { for i := range chiList.Items { chi := &chiList.Items[i] - // Filter out CHIs in namespaces the operator doesn't actually watch. - // The List above may surface namespaces beyond the watch scope when - // the watch is regexp-driven; the normal informer path applies - // IsNamespaceWatched at ShouldEnqueue time, and we must mirror that. - if !chop.Config().IsNamespaceWatched(chi.Namespace) { + // Mirror ShouldEnqueue: namespace AND label selector. The triggering CHK is + // deliberately not filtered — our CHI may reference another shard's CHK. + if !chop.Config().IsCRWatched(chi.Namespace, chi.GetLabels()) { continue } diff --git a/pkg/controller/chi/controller-chk-watcher_label_test.go b/pkg/controller/chi/controller-chk-watcher_label_test.go new file mode 100644 index 000000000..cbf538915 --- /dev/null +++ b/pkg/controller/chi/controller-chk-watcher_label_test.go @@ -0,0 +1,77 @@ +package chi + +import ( + "testing" + + meta "k8s.io/apimachinery/pkg/apis/meta/v1" + + api "github.com/altinity/clickhouse-operator/pkg/apis/clickhouse.altinity.com/v1" + chopFake "github.com/altinity/clickhouse-operator/pkg/client/clientset/versioned/fake" +) + +func newKeeperReferencingCHI(name string, labels map[string]string, chkName string) *api.ClickHouseInstallation { + return &api.ClickHouseInstallation{ + ObjectMeta: meta.ObjectMeta{ + Namespace: "clickhouse", + Name: name, + Labels: labels, + }, + Spec: api.ChiSpec{ + Configuration: &api.Configuration{ + Zookeeper: &api.ZookeeperConfig{ + Keeper: &api.KeeperRef{Name: chkName}, + }, + }, + }, + } +} + +func queuedItems(c *Controller) int { + total := 0 + for i := range c.queues { + total += c.queues[i].Len() + } + return total +} + +// enqueueDependentCHIs must only enqueue CHIs within this operator's watch scope. The +// triggering CHK is not label-filtered: a CHI of this shard may reference another shard's CHK. +func Test_enqueueDependentCHIsWithLabelSelector(t *testing.T) { + stgCHI := newKeeperReferencingCHI("chi-stg", map[string]string{testShardKey: "stg"}, "keeper1") + logsCHI := newKeeperReferencingCHI("chi-logs", map[string]string{testShardKey: "logs"}, "keeper1") + unrelatedCHI := newKeeperReferencingCHI("chi-other-keeper", map[string]string{testShardKey: "stg"}, "keeper2") + + newController := func() *Controller { + c := &Controller{ + chopClient: chopFake.NewSimpleClientset(stgCHI, logsCHI, unrelatedCHI), + } + c.initQueues() + return c + } + + t.Run("no selector enqueues all CHIs referencing the CHK (backward compat)", func(t *testing.T) { + c := newController() + c.enqueueDependentCHIs("clickhouse", "keeper1") + if got := queuedItems(c); got != 2 { + t.Errorf("queued %d CHIs, want 2 (both keeper1 referents, any labels)", got) + } + }) + + t.Run("shard selector enqueues only matching CHIs", func(t *testing.T) { + setWatchLabelSelector(t, testShardKey+"=stg") + c := newController() + c.enqueueDependentCHIs("clickhouse", "keeper1") + if got := queuedItems(c); got != 1 { + t.Errorf("queued %d CHIs, want 1 (only the stg-labeled keeper1 referent)", got) + } + }) + + t.Run("legacy selector enqueues nothing when all referents are shard-labeled", func(t *testing.T) { + setWatchLabelSelector(t, "!"+testShardKey) + c := newController() + c.enqueueDependentCHIs("clickhouse", "keeper1") + if got := queuedItems(c); got != 0 { + t.Errorf("queued %d CHIs, want 0", got) + } + }) +} diff --git a/pkg/controller/chi/controller.go b/pkg/controller/chi/controller.go index 2aa9efd07..d629109d7 100644 --- a/pkg/controller/chi/controller.go +++ b/pkg/controller/chi/controller.go @@ -48,12 +48,15 @@ import ( chopClientSet "github.com/altinity/clickhouse-operator/pkg/client/clientset/versioned" chopClientSetScheme "github.com/altinity/clickhouse-operator/pkg/client/clientset/versioned/scheme" chopInformers "github.com/altinity/clickhouse-operator/pkg/client/informers/externalversions" + chopListers "github.com/altinity/clickhouse-operator/pkg/client/listers/clickhouse.altinity.com/v1" "github.com/altinity/clickhouse-operator/pkg/controller" "github.com/altinity/clickhouse-operator/pkg/controller/chi/cmd_queue" chiKube "github.com/altinity/clickhouse-operator/pkg/controller/chi/kube" ctrlLabeler "github.com/altinity/clickhouse-operator/pkg/controller/chi/labeler" + chiMetrics "github.com/altinity/clickhouse-operator/pkg/controller/chi/metrics" "github.com/altinity/clickhouse-operator/pkg/interfaces" "github.com/altinity/clickhouse-operator/pkg/metrics/clickhouse" + operatorMetrics "github.com/altinity/clickhouse-operator/pkg/metrics/operator" chiLabeler "github.com/altinity/clickhouse-operator/pkg/model/chi/tags/labeler" "github.com/altinity/clickhouse-operator/pkg/model/common/volume" "github.com/altinity/clickhouse-operator/pkg/model/managers" @@ -74,6 +77,12 @@ type Controller struct { chopClient chopClientSet.Interface dynamicClient dynamic.Interface + // chiLister resolves the owning CHI of child objects from the informer cache + chiLister chopListers.ClickHouseInstallationLister + // chiListerSynced reports whether the CHI informer cache has completed its initial sync; + // before that, a cache miss means "not synced yet", not "CHI does not exist" + chiListerSynced cache.InformerSynced + // queues used to organize events queue processed by the operator queues []queue.PriorityQueue // not used explicitly @@ -118,15 +127,17 @@ func NewController( // Create Controller instance controller := &Controller{ - kubeClient: kubeClient, - extClient: extClient, - chopClient: chopClient, - dynamicClient: dynamicClient, - recorder: recorder, - namer: namer, - kube: kube, - ctrlLabeler: ctrlLabeler.New(kube), - pvcDeleter: volume.NewPVCDeleter(managers.NewNameManager(managers.NameManagerTypeClickHouse)), + kubeClient: kubeClient, + extClient: extClient, + chopClient: chopClient, + dynamicClient: dynamicClient, + chiLister: chopInformerFactory.Clickhouse().V1().ClickHouseInstallations().Lister(), + chiListerSynced: chopInformerFactory.Clickhouse().V1().ClickHouseInstallations().Informer().HasSynced, + recorder: recorder, + namer: namer, + kube: kube, + ctrlLabeler: ctrlLabeler.New(kube), + pvcDeleter: volume.NewPVCDeleter(managers.NewNameManager(managers.NameManagerTypeClickHouse)), } controller.initQueues() controller.addEventHandlers(chopConfigInformerFactory, chopInformerFactory, kubeInformerFactory) @@ -196,13 +207,21 @@ func (c *Controller) addEventHandlersCHI( oldChi := old.(*api.ClickHouseInstallation) newChi := new.(*api.ClickHouseInstallation) if !ShouldEnqueue(newChi) { + if isFlipAway(oldChi, newChi) { + // Drop local watch/exporter state and per-CR metrics only; cluster resources + // belong to the gaining operator and a flip must never be treated as a delete. + log.V(1).M(newChi).Info("CHI flipped out of this operator's shard, dropping local watch state") + c.deleteWatch(oldChi) + chiMetrics.CRUnregister(context.Background(), oldChi) + } return } log.V(3).M(newChi).Info("chiInformer.UpdateFunc") c.enqueueObject(cmd_queue.NewReconcileCHI(cmd_queue.ReconcileUpdate, oldChi, newChi)) }, DeleteFunc: deleteHandler("chiInformer.DeleteFunc", func(chi *api.ClickHouseInstallation) { - if !chop.Config().IsNamespaceWatched(chi.Namespace) { + // Same guard as Add/Update: another shard's CHI is not ours to tear down + if !ShouldEnqueue(chi) { return } log.V(3).M(chi).Info("chiInformer.DeleteFunc") @@ -561,7 +580,40 @@ func (c *Controller) addEventHandlers( // isTrackedObject checks whether operator is interested in changes of this object func (c *Controller) isTrackedObject(meta meta.Object) bool { - return chop.Config().IsNamespaceWatched(meta.GetNamespace()) && chiLabeler.New(nil).IsCHOPGeneratedObject(meta) + return chop.Config().IsNamespaceWatched(meta.GetNamespace()) && + chiLabeler.New(nil).IsCHOPGeneratedObject(meta) && + c.isOwningCRWatched(meta) +} + +// isOwningCRWatched checks watch.labelSelector against a child object's owning CHI (child +// objects stay unlabeled — labeling them would restart pods on every shard flip). +// Unresolvable owner => not tracked. +func (c *Controller) isOwningCRWatched(obj meta.Object) bool { + if !chop.Config().HasWatchLabelSelector() { + return true + } + chiName, err := chiLabeler.New(nil).GetCRNameFromObjectMeta(obj) + if err != nil { + log.V(2).Info("skip child object %s/%s: unable to resolve owning CHI name: %v", obj.GetNamespace(), obj.GetName(), err) + return false + } + if c.chiLister == nil { + log.V(2).Info("skip child object %s/%s: no CHI lister available to check watch.labelSelector", obj.GetNamespace(), obj.GetName()) + return false + } + // Until the CHI cache completes its initial sync, a miss means "not synced yet", not + // "does not exist". Dropping edge-triggered events here (e.g. EndpointSlice IP assignment) + // would lose them permanently — resync re-fires with old==new and produces no diff. Track + // the object instead; every downstream write path re-checks ownership at dequeue time. + if (c.chiListerSynced != nil) && !c.chiListerSynced() { + return true + } + chi, err := c.chiLister.ClickHouseInstallations(obj.GetNamespace()).Get(chiName) + if (err != nil) || (chi == nil) { + log.V(2).Info("skip child object %s/%s: owning CHI '%s' not found in cache: %v", obj.GetNamespace(), obj.GetName(), chiName, err) + return false + } + return chop.Config().IsLabelSelectorWatched(chi.GetLabels()) } // Run syncs caches, starts workers @@ -925,6 +977,16 @@ func (c *Controller) uninstallFinalizer(ctx context.Context, chi *api.ClickHouse return c.patchCHIFinalizers(ctx, cur) } +// ownsCR re-checks watch scope against fresh CR state (labels may flip after enqueue). +func ownsCR(cr meta.Object) bool { + return chop.Config().IsCRWatched(cr.GetNamespace(), cr.GetLabels()) +} + +// isFlipAway reports whether an update moves a CR out of this operator's shard. +func isFlipAway(old, new *api.ClickHouseInstallation) bool { + return !ShouldEnqueue(new) && ShouldEnqueue(old) +} + func ShouldEnqueue(cr *api.ClickHouseInstallation) bool { ns := cr.GetNamespace() if !chop.Config().IsNamespaceWatched(ns) { @@ -932,5 +994,11 @@ func ShouldEnqueue(cr *api.ClickHouseInstallation) bool { return false } + if !chop.Config().IsLabelSelectorWatched(cr.GetLabels()) { + log.V(2).M(cr).Info("skip enqueue, CHI labels do not match watch.labelSelector '%s'", chop.Config().Watch.LabelSelector) + operatorMetrics.CRSkippedByLabelSelector("chi", cr.GetNamespace(), cr.GetName()) + return false + } + return true } diff --git a/pkg/controller/chi/controller_watch_label_test.go b/pkg/controller/chi/controller_watch_label_test.go new file mode 100644 index 000000000..4cc1743ff --- /dev/null +++ b/pkg/controller/chi/controller_watch_label_test.go @@ -0,0 +1,242 @@ +package chi + +import ( + "testing" + + core "k8s.io/api/core/v1" + meta "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/tools/cache" + + api "github.com/altinity/clickhouse-operator/pkg/apis/clickhouse.altinity.com/v1" + "github.com/altinity/clickhouse-operator/pkg/chop/choptest" + chopListers "github.com/altinity/clickhouse-operator/pkg/client/listers/clickhouse.altinity.com/v1" +) + +const ( + testShardKey = choptest.ShardLabelKey + + // CHOP-generated child object labels (see pkg/model/chi/tags/labeler/list.go) + labelApp = "clickhouse.altinity.com/app" + labelAppVal = "chop" + labelCRName = "clickhouse.altinity.com/chi" +) + +var setWatchLabelSelector = choptest.SetWatchLabelSelector + +func newCHI(name string, labels map[string]string) *api.ClickHouseInstallation { + return &api.ClickHouseInstallation{ + ObjectMeta: meta.ObjectMeta{ + Namespace: "clickhouse", + Name: name, + Labels: labels, + }, + } +} + +func Test_shouldEnqueueWithLabelSelector(t *testing.T) { + tests := []struct { + name string + selector string + labels map[string]string + want bool + }{ + {"shard operator enqueues matching CHI", testShardKey + "=stg", map[string]string{testShardKey: "stg"}, true}, + {"shard operator skips other shard's CHI", testShardKey + "=stg", map[string]string{testShardKey: "logs"}, false}, + {"shard operator skips unlabeled CHI", testShardKey + "=stg", nil, false}, + {"shard operator skips CHI with unrelated labels", testShardKey + "=stg", map[string]string{"unrelated": "value"}, false}, + {"legacy operator enqueues unlabeled CHI", "!" + testShardKey, nil, true}, + {"legacy operator enqueues CHI with unrelated labels", "!" + testShardKey, map[string]string{"unrelated": "value"}, true}, + {"legacy operator skips shard-labeled CHI", "!" + testShardKey, map[string]string{testShardKey: "stg"}, false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + setWatchLabelSelector(t, tt.selector) + if got := ShouldEnqueue(newCHI("test-chi", tt.labels)); got != tt.want { + t.Errorf("ShouldEnqueue() = %v, want %v", got, tt.want) + } + }) + } +} + +// A label flip arrives at both operators as a plain Update: the losing operator ignores it +// (no delete flow), the gaining operator reconciles it. +func Test_labelFlipIsNotDelete(t *testing.T) { + flipped := newCHI("flipping-chi", map[string]string{testShardKey: "stg"}) + + t.Run("losing operator ignores the update and would ignore a delete", func(t *testing.T) { + setWatchLabelSelector(t, "!"+testShardKey) + if ShouldEnqueue(flipped) { + t.Error("operator losing a CHI on label flip must not enqueue any work for it") + } + }) + + t.Run("gaining operator sees a normal reconcile", func(t *testing.T) { + setWatchLabelSelector(t, testShardKey+"=stg") + if !ShouldEnqueue(flipped) { + t.Error("operator gaining a CHI on label flip must enqueue a normal reconcile") + } + }) +} + +// A shard flip while a command sits in the queue must be caught at dequeue time: ownsCR +// re-checks live labels so the losing operator drops the stale work instead of reconciling +// (and purging) a CHI the gaining operator now owns. +func Test_ownsCRAfterLabelFlip(t *testing.T) { + setWatchLabelSelector(t, testShardKey+"=stg") + + if !ownsCR(newCHI("chi", map[string]string{testShardKey: "stg"})) { + t.Error("ownsCR() = false for in-shard CHI, want true") + } + // Live labels flipped to another shard after enqueue + if ownsCR(newCHI("chi", map[string]string{testShardKey: "logs"})) { + t.Error("ownsCR() = true for flipped CHI, want false (stale queued work must be dropped)") + } + if ownsCR(newCHI("chi", nil)) { + t.Error("ownsCR() = true for unlabeled CHI under shard selector, want false") + } +} + +func Test_isFlipAway(t *testing.T) { + setWatchLabelSelector(t, testShardKey+"=stg") + stg := newCHI("chi", map[string]string{testShardKey: "stg"}) + logs := newCHI("chi", map[string]string{testShardKey: "logs"}) + + tests := []struct { + name string + old, new *api.ClickHouseInstallation + want bool + }{ + {"flip away triggers local cleanup", stg, logs, true}, + {"still owned", stg, stg, false}, + {"never owned", logs, logs, false}, + {"flip toward is a normal enqueue", logs, stg, false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := isFlipAway(tt.old, tt.new); got != tt.want { + t.Errorf("isFlipAway() = %v, want %v", got, tt.want) + } + }) + } +} + +// newTestControllerWithCHIs builds a Controller whose CHI lister is backed by an in-memory +// cache containing the given CHIs. +func newTestControllerWithCHIs(t *testing.T, chis ...*api.ClickHouseInstallation) *Controller { + t.Helper() + indexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{cache.NamespaceIndex: cache.MetaNamespaceIndexFunc}) + for _, chi := range chis { + if err := indexer.Add(chi); err != nil { + t.Fatalf("failed to add CHI to test indexer: %v", err) + } + } + return &Controller{ + chiLister: chopListers.NewClickHouseInstallationLister(indexer), + } +} + +// newChildObject builds a CHOP-generated child object owned by the named CHI. Child objects +// carry no shard label — the selector is resolved against the owning CHI. +func newChildObject(owningCHI string) *core.ConfigMap { + return &core.ConfigMap{ + ObjectMeta: meta.ObjectMeta{ + Namespace: "clickhouse", + Name: "child-of-" + owningCHI, + Labels: map[string]string{ + labelApp: labelAppVal, + labelCRName: owningCHI, + }, + }, + } +} + +func Test_isTrackedObjectWithLabelSelector(t *testing.T) { + stgCHI := newCHI("chi-stg", map[string]string{testShardKey: "stg"}) + logsCHI := newCHI("chi-logs", map[string]string{testShardKey: "logs"}) + unlabeledCHI := newCHI("chi-unlabeled", nil) + c := newTestControllerWithCHIs(t, stgCHI, logsCHI, unlabeledCHI) + + t.Run("no selector: all CHOP-generated objects tracked (backward compat)", func(t *testing.T) { + for _, owner := range []string{"chi-stg", "chi-logs", "chi-unlabeled", "chi-not-in-cache"} { + if !c.isTrackedObject(&newChildObject(owner).ObjectMeta) { + t.Errorf("child of %q not tracked without selector, want tracked", owner) + } + } + }) + + t.Run("no selector: non-CHOP object still untracked", func(t *testing.T) { + plain := &core.ConfigMap{ObjectMeta: meta.ObjectMeta{Namespace: "clickhouse", Name: "plain"}} + if c.isTrackedObject(&plain.ObjectMeta) { + t.Error("non-CHOP-generated object tracked, want untracked") + } + }) + + t.Run("shard selector: tracked iff owning CHI matches", func(t *testing.T) { + setWatchLabelSelector(t, testShardKey+"=stg") + if !c.isTrackedObject(&newChildObject("chi-stg").ObjectMeta) { + t.Error("child of matching CHI untracked, want tracked") + } + if c.isTrackedObject(&newChildObject("chi-logs").ObjectMeta) { + t.Error("child of other shard's CHI tracked, want untracked") + } + if c.isTrackedObject(&newChildObject("chi-unlabeled").ObjectMeta) { + t.Error("child of unlabeled CHI tracked under shard selector, want untracked") + } + }) + + t.Run("legacy selector: tracked iff owning CHI is unlabeled", func(t *testing.T) { + setWatchLabelSelector(t, "!"+testShardKey) + if !c.isTrackedObject(&newChildObject("chi-unlabeled").ObjectMeta) { + t.Error("child of unlabeled CHI untracked under legacy selector, want tracked") + } + if c.isTrackedObject(&newChildObject("chi-stg").ObjectMeta) { + t.Error("child of shard-labeled CHI tracked under legacy selector, want untracked") + } + }) + + t.Run("selector set: owning CHI missing from cache => untracked, no panic", func(t *testing.T) { + setWatchLabelSelector(t, testShardKey+"=stg") + if c.isTrackedObject(&newChildObject("chi-not-in-cache").ObjectMeta) { + t.Error("child with unresolvable owning CHI tracked, want untracked (cannot attribute to a shard)") + } + }) + + t.Run("selector set: CHOP object without CR name label => untracked, no panic", func(t *testing.T) { + setWatchLabelSelector(t, testShardKey+"=stg") + orphan := &core.ConfigMap{ + ObjectMeta: meta.ObjectMeta{ + Namespace: "clickhouse", + Name: "orphan", + Labels: map[string]string{labelApp: labelAppVal}, + }, + } + if c.isTrackedObject(&orphan.ObjectMeta) { + t.Error("CHOP object without CR name label tracked under selector, want untracked") + } + }) + + t.Run("selector set: nil lister => untracked, no panic", func(t *testing.T) { + setWatchLabelSelector(t, testShardKey+"=stg") + noLister := &Controller{} + if noLister.isTrackedObject(&newChildObject("chi-stg").ObjectMeta) { + t.Error("tracked with nil lister under selector, want untracked") + } + }) + + t.Run("selector set: CHI cache not yet synced => tracked (startup fallback)", func(t *testing.T) { + // Before the initial CHI list completes, a cache miss means "not synced yet", not + // "does not exist" — dropping edge-triggered child events (e.g. EndpointSlice IP + // assignment) here would lose them permanently, since resync produces no diff. + setWatchLabelSelector(t, testShardKey+"=stg") + unsynced := newTestControllerWithCHIs(t) // empty cache + unsynced.chiListerSynced = func() bool { return false } + if !unsynced.isTrackedObject(&newChildObject("chi-not-listed-yet").ObjectMeta) { + t.Error("child untracked while CHI cache not synced, want tracked (downstream guards re-check ownership)") + } + // Once synced, an actual cache miss means untracked again + unsynced.chiListerSynced = func() bool { return true } + if unsynced.isTrackedObject(&newChildObject("chi-not-listed-yet").ObjectMeta) { + t.Error("child of missing CHI tracked after cache sync, want untracked") + } + }) +} diff --git a/pkg/controller/chi/kube/cr.go b/pkg/controller/chi/kube/cr.go index 325bcc636..7a1bb525b 100644 --- a/pkg/controller/chi/kube/cr.go +++ b/pkg/controller/chi/kube/cr.go @@ -28,6 +28,7 @@ import ( log "github.com/altinity/clickhouse-operator/pkg/announcer" api "github.com/altinity/clickhouse-operator/pkg/apis/clickhouse.altinity.com/v1" commonTypes "github.com/altinity/clickhouse-operator/pkg/apis/common/types" + "github.com/altinity/clickhouse-operator/pkg/chop" chopClientSet "github.com/altinity/clickhouse-operator/pkg/client/clientset/versioned" "github.com/altinity/clickhouse-operator/pkg/controller" commonKube "github.com/altinity/clickhouse-operator/pkg/controller/common/kube" @@ -184,6 +185,14 @@ func (c *CR) statusUpdateProcess(ctx context.Context, icr api.ICustomResource, o return fmt.Errorf("ERROR GetCR (%s/%s): NULL returned", namespace, name) } + // Sharding: validate ownership on the same object snapshot whose resourceVersion + // fences the update below. If the shard label flips after this read, the flip bumps + // the resourceVersion, the update conflicts, and the retry re-runs this check. + if chop.Config().HasWatchLabelSelector() && !chop.Config().IsCRWatched(cur.GetNamespace(), cur.GetLabels()) { + log.V(1).M(cr).F().Info("CR no longer matches watch label selector, skip status update: %s/%s", namespace, name) + return nil + } + // Update status of a real (current) object. cur.EnsureStatus().CopyFrom(cr.Status, opts.CopyStatusOptions) diff --git a/pkg/controller/chi/worker-deleter.go b/pkg/controller/chi/worker-deleter.go index 6a91eff90..97ae5ca32 100644 --- a/pkg/controller/chi/worker-deleter.go +++ b/pkg/controller/chi/worker-deleter.go @@ -25,6 +25,7 @@ import ( log "github.com/altinity/clickhouse-operator/pkg/announcer" api "github.com/altinity/clickhouse-operator/pkg/apis/clickhouse.altinity.com/v1" "github.com/altinity/clickhouse-operator/pkg/apis/common/types" + "github.com/altinity/clickhouse-operator/pkg/chop" "github.com/altinity/clickhouse-operator/pkg/controller" "github.com/altinity/clickhouse-operator/pkg/controller/chi/metrics" a "github.com/altinity/clickhouse-operator/pkg/controller/common/announcer" @@ -47,6 +48,15 @@ func (w *worker) clean(ctx context.Context, cr api.ICustomResource) { M(cr).F(). Info("remove items scheduled for deletion") + // Re-check ownership on live labels before the destructive purge; skipping is recoverable, + // deleting another shard's objects is not. No-op unless a selector is configured. + if chop.Config().HasWatchLabelSelector() { + if live, err := w.c.kube.CR().Get(ctx, cr.GetNamespace(), cr.GetName()); (err != nil) || !ownsCR(live) { + w.a.V(1).M(cr).F().Warning("skip purge: CR ownership not confirmed (flipped to another shard or lookup failed): %s/%s err: %v", cr.GetNamespace(), cr.GetName(), err) + return + } + } + // Remove deleted items w.a.V(1).M(cr).F().Info("List of objects which have failed to reconcile:\n%s", w.task.RegistryFailed()) w.a.V(1).M(cr).F().Info("List of successfully reconciled objects:\n%s", w.task.RegistryReconciled()) diff --git a/pkg/controller/chi/worker-reconciler-chi.go b/pkg/controller/chi/worker-reconciler-chi.go index debfdf6ee..05c860fb4 100644 --- a/pkg/controller/chi/worker-reconciler-chi.go +++ b/pkg/controller/chi/worker-reconciler-chi.go @@ -20,9 +20,6 @@ import ( "fmt" "time" - apiequality "k8s.io/apimachinery/pkg/api/equality" - meta "k8s.io/apimachinery/pkg/apis/meta/v1" - log "github.com/altinity/clickhouse-operator/pkg/announcer" api "github.com/altinity/clickhouse-operator/pkg/apis/clickhouse.altinity.com/v1" "github.com/altinity/clickhouse-operator/pkg/apis/common/types" @@ -38,6 +35,7 @@ import ( "github.com/altinity/clickhouse-operator/pkg/model/chi/config" commonNormalizer "github.com/altinity/clickhouse-operator/pkg/model/common/normalizer" "github.com/altinity/clickhouse-operator/pkg/util" + apiequality "k8s.io/apimachinery/pkg/api/equality" ) // reconcileCR runs reconcile cycle for a Custom Resource @@ -208,15 +206,6 @@ func (w *worker) buildCR(ctx context.Context, _cr *api.ClickHouseInstallation) * return cr } -func (w *worker) buildCRFromObj(ctx context.Context, obj meta.Object) (*api.ClickHouseInstallation, error) { - _cr, err := w.c.GetCR(obj) - if err != nil { - w.a.M(obj).F().Error("UNABLE-1 to find obj by labels: %v err: %v", obj.GetLabels(), err) - return nil, err - } - return w.buildCR(ctx, _cr), nil -} - func (w *worker) buildTemplates(chi *api.ClickHouseInstallation) (templates []*api.ClickHouseInstallation) { for _, spec := range model.GetConfigMatchSpecs(chi) { templates = append(templates, &api.ClickHouseInstallation{ diff --git a/pkg/controller/chi/worker.go b/pkg/controller/chi/worker.go index 77e1eed29..08f82f72b 100644 --- a/pkg/controller/chi/worker.go +++ b/pkg/controller/chi/worker.go @@ -282,12 +282,23 @@ func (w *worker) finalizeCR( updateStatusOpts types.UpdateStatusOptions, f func(*api.ClickHouseInstallation), ) error { - chi, err := w.buildCRFromObj(ctx, obj) + raw, err := w.c.GetCR(obj) if err != nil { log.V(1).Error("Unable to finalize CR: %s err: %v", util.NamespacedName(obj), err) return err } + // Re-check ownership on the RAW live CR before the status/configmap writes — the shard + // label may have flipped since this work was enqueued. Must be checked pre-normalization: + // templates (CHIT) merge their ObjectMeta into the CR and could inject/override the shard + // label, making this guard disagree with the enqueue-time guards that see raw labels. + if !ownsCR(raw) { + log.V(1).Info("CR no longer matches this operator's watch scope, skip finalize: %s", util.NamespacedName(obj)) + return nil + } + + chi := w.buildCR(ctx, raw) + if f != nil { f(chi) } @@ -319,6 +330,22 @@ func (w *worker) updateCHI(ctx context.Context, old, new *api.ClickHouseInstalla w.a.V(1).M(new).S().P() defer w.a.V(1).M(new).E().P() + if new != nil { + n, err := w.c.kube.CR().Get(ctx, new.GetNamespace(), new.GetName()) + if err != nil { + return err + } + new = n.(*api.ClickHouseInstallation) + } + + // Shard label may have flipped while this command sat in the queue; a stale reconcile + // would purge the gaining operator's objects. Checked before any write, including the + // finalizer install below. + if !ownsCR(new) { + w.a.V(1).M(new).F().Info("CHI no longer matches this operator's watch scope (shard label flipped?), skip reconcile: %s/%s", new.Namespace, new.Name) + return nil + } + if w.ensureFinalizer(context.Background(), new) { w.a.M(new).F().Info("finalizer installed, let's restart reconcile cycle. CHI: %s/%s", new.Namespace, new.Name) w.a.M(new).F().Info("---------------------------------------------------------------------") @@ -332,14 +359,6 @@ func (w *worker) updateCHI(ctx context.Context, old, new *api.ClickHouseInstalla return nil } - if new != nil { - n, err := w.c.kube.CR().Get(ctx, new.GetNamespace(), new.GetName()) - if err != nil { - return err - } - new = n.(*api.ClickHouseInstallation) - } - metrics.CRRegister(ctx, new) if w.deleteCHI(ctx, old, new) { diff --git a/pkg/controller/chk/controller.go b/pkg/controller/chk/controller.go index bef2cd13f..5fe36d8b0 100644 --- a/pkg/controller/chk/controller.go +++ b/pkg/controller/chk/controller.go @@ -31,6 +31,7 @@ import ( "github.com/altinity/clickhouse-operator/pkg/chop" "github.com/altinity/clickhouse-operator/pkg/controller/chk/kube" "github.com/altinity/clickhouse-operator/pkg/interfaces" + operatorMetrics "github.com/altinity/clickhouse-operator/pkg/metrics/operator" "github.com/altinity/clickhouse-operator/pkg/model/managers" "github.com/altinity/clickhouse-operator/pkg/util" ) @@ -95,6 +96,20 @@ func (c *Controller) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu return ctrl.Result{}, err } + // Label guard here, not just in the predicate: reconcile requests from owned + // StatefulSet changes bypass predicates. + if !chop.Config().IsLabelSelectorWatched(new.GetLabels()) { + log.V(2).M(new).Info("skip reconcile, CHK labels do not match watch.labelSelector '%s'", chop.Config().Watch.LabelSelector) + return ctrl.Result{}, nil + } + + // The cached read above can lag a shard-label flip; confirm ownership on live state + // before any write (finalizer install, child-object reconcile, deletion protocol). + if chop.Config().HasWatchLabelSelector() && !c.ownsLiveCR(ctx, req.Namespace, req.Name) { + log.V(1).M(new).Info("skip reconcile, live CHK ownership not confirmed (shard label flipped?): %s/%s", req.Namespace, req.Name) + return ctrl.Result{}, nil + } + w := c.newWorker() if w.ensureFinalizer(ctx, new) { @@ -191,6 +206,18 @@ func (c *Controller) poll(ctx context.Context, cr api.ICustomResource, f func(c } } +// ownsLiveCR re-checks watch scope against live (non-cached) CR state — the entry guard in +// Reconcile reads the informer cache, which can miss a shard label flip mid-reconcile. +// Lookup failure counts as not-owned: skipping is recoverable, purging another shard's objects is not. +func (c *Controller) ownsLiveCR(ctx context.Context, namespace, name string) bool { + live := &apiChk.ClickHouseKeeperInstallation{} + if err := c.APIReader.Get(ctx, kubeTypes.NamespacedName{Namespace: namespace, Name: name}, live); err != nil { + log.V(1).Warning("unable to confirm live CHK ownership %s/%s: %v", namespace, name, err) + return false + } + return chop.Config().IsCRWatched(live.GetNamespace(), live.GetLabels()) +} + func ShouldEnqueue(cr *apiChk.ClickHouseKeeperInstallation) bool { ns := cr.GetNamespace() if !chop.Config().IsNamespaceWatched(ns) { @@ -198,5 +225,11 @@ func ShouldEnqueue(cr *apiChk.ClickHouseKeeperInstallation) bool { return false } + if !chop.Config().IsLabelSelectorWatched(cr.GetLabels()) { + log.V(2).M(cr).Info("skip enqueue, CHK labels do not match watch.labelSelector '%s'", chop.Config().Watch.LabelSelector) + operatorMetrics.CRSkippedByLabelSelector("chk", cr.GetNamespace(), cr.GetName()) + return false + } + return true } diff --git a/pkg/controller/chk/controller_watch_label_test.go b/pkg/controller/chk/controller_watch_label_test.go new file mode 100644 index 000000000..5d5c873d9 --- /dev/null +++ b/pkg/controller/chk/controller_watch_label_test.go @@ -0,0 +1,234 @@ +package chk + +import ( + "context" + "testing" + + meta "k8s.io/apimachinery/pkg/apis/meta/v1" + apiMachineryRuntime "k8s.io/apimachinery/pkg/runtime" + kubeTypes "k8s.io/apimachinery/pkg/types" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + apiChk "github.com/altinity/clickhouse-operator/pkg/apis/clickhouse-keeper.altinity.com/v1" + commonTypes "github.com/altinity/clickhouse-operator/pkg/apis/common/types" + "github.com/altinity/clickhouse-operator/pkg/chop/choptest" +) + +const testShardKey = choptest.ShardLabelKey + +func init() { + // ShouldEnqueue()/Reconcile() → chop.Config(), so the global chop singleton + // must be initialized before tests run. + choptest.EnsureInit() +} + +var setWatchLabelSelector = choptest.SetWatchLabelSelector + +func newCHK(name string, labels map[string]string) *apiChk.ClickHouseKeeperInstallation { + return &apiChk.ClickHouseKeeperInstallation{ + ObjectMeta: meta.ObjectMeta{ + Namespace: "clickhouse", + Name: name, + Labels: labels, + }, + } +} + +func Test_chkShouldEnqueueWithLabelSelector(t *testing.T) { + tests := []struct { + name string + selector string + labels map[string]string + want bool + }{ + {"no selector enqueues everything (backward compat)", "", map[string]string{testShardKey: "logs"}, true}, + {"no selector enqueues unlabeled", "", nil, true}, + {"shard operator enqueues matching CHK", testShardKey + "=stg", map[string]string{testShardKey: "stg"}, true}, + {"shard operator skips other shard's CHK", testShardKey + "=stg", map[string]string{testShardKey: "logs"}, false}, + {"shard operator skips unlabeled CHK", testShardKey + "=stg", nil, false}, + {"legacy operator enqueues unlabeled CHK", "!" + testShardKey, nil, true}, + {"legacy operator skips shard-labeled CHK", "!" + testShardKey, map[string]string{testShardKey: "stg"}, false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if tt.selector != "" { + setWatchLabelSelector(t, tt.selector) + } + if got := ShouldEnqueue(newCHK("test-chk", tt.labels)); got != tt.want { + t.Errorf("ShouldEnqueue() = %v, want %v", got, tt.want) + } + }) + } +} + +// Reconcile requests from owned StatefulSet changes bypass keeperPredicate(), so the +// post-Get label guard in Reconcile must return cleanly, without mutating the CR. +func Test_reconcilePostGetLabelGuard(t *testing.T) { + scheme := apiMachineryRuntime.NewScheme() + if err := apiChk.AddToScheme(scheme); err != nil { + t.Fatalf("AddToScheme failed: %v", err) + } + + otherShardCHK := newCHK("other-shard-chk", map[string]string{testShardKey: "logs"}) + fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(otherShardCHK).Build() + c := &Controller{Client: fakeClient} + + setWatchLabelSelector(t, testShardKey+"=stg") + + result, err := c.Reconcile(context.Background(), ctrl.Request{ + NamespacedName: kubeTypes.NamespacedName{Namespace: "clickhouse", Name: "other-shard-chk"}, + }) + if err != nil { + t.Fatalf("Reconcile() of non-matching CHK returned error: %v, want clean skip", err) + } + if result.Requeue || result.RequeueAfter != 0 { + t.Errorf("Reconcile() of non-matching CHK requested requeue %+v, want none", result) + } + + after := &apiChk.ClickHouseKeeperInstallation{} + if err := fakeClient.Get(context.Background(), kubeTypes.NamespacedName{Namespace: "clickhouse", Name: "other-shard-chk"}, after); err != nil { + t.Fatalf("Get after Reconcile failed: %v", err) + } + if len(after.GetFinalizers()) != 0 { + t.Errorf("non-matching CHK was mutated (finalizers %v), want untouched", after.GetFinalizers()) + } +} + +// The informer cache can lag a shard-label flip: cached labels still match this operator +// while the live object already belongs to another shard. Reconcile must skip cleanly on +// the live re-check without mutating the CR. +func Test_reconcileLiveOwnershipGuard(t *testing.T) { + scheme := apiMachineryRuntime.NewScheme() + if err := apiChk.AddToScheme(scheme); err != nil { + t.Fatalf("AddToScheme failed: %v", err) + } + + cachedCHK := newCHK("flipping-chk", map[string]string{testShardKey: "stg"}) + liveCHK := newCHK("flipping-chk", map[string]string{testShardKey: "logs"}) + cachedClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(cachedCHK).Build() + liveClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(liveCHK).Build() + c := &Controller{Client: cachedClient, APIReader: liveClient} + + setWatchLabelSelector(t, testShardKey+"=stg") + + result, err := c.Reconcile(context.Background(), ctrl.Request{ + NamespacedName: kubeTypes.NamespacedName{Namespace: "clickhouse", Name: "flipping-chk"}, + }) + if err != nil { + t.Fatalf("Reconcile() with flipped live labels returned error: %v, want clean skip", err) + } + if result.Requeue || result.RequeueAfter != 0 { + t.Errorf("Reconcile() with flipped live labels requested requeue %+v, want none", result) + } + + after := &apiChk.ClickHouseKeeperInstallation{} + if err := cachedClient.Get(context.Background(), kubeTypes.NamespacedName{Namespace: "clickhouse", Name: "flipping-chk"}, after); err != nil { + t.Fatalf("Get after Reconcile failed: %v", err) + } + if len(after.GetFinalizers()) != 0 { + t.Errorf("flipped CHK was mutated (finalizers %v), want untouched", after.GetFinalizers()) + } +} + +// ownsLiveCR guards the purge phase: it must confirm ownership on live labels and treat +// lookup failure (deleted CR) as not-owned. +func Test_ownsLiveCR(t *testing.T) { + scheme := apiMachineryRuntime.NewScheme() + if err := apiChk.AddToScheme(scheme); err != nil { + t.Fatalf("AddToScheme failed: %v", err) + } + fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects( + newCHK("stg-chk", map[string]string{testShardKey: "stg"}), + newCHK("logs-chk", map[string]string{testShardKey: "logs"}), + ).Build() + c := &Controller{APIReader: fakeClient} + + setWatchLabelSelector(t, testShardKey+"=stg") + + tests := []struct { + name string + chk string + want bool + }{ + {"owns matching CHK", "stg-chk", true}, + {"does not own other shard's CHK (flipped away)", "logs-chk", false}, + {"does not own missing CHK (deleted)", "gone-chk", false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := c.ownsLiveCR(context.Background(), "clickhouse", tt.chk); got != tt.want { + t.Errorf("ownsLiveCR(%s) = %v, want %v", tt.chk, got, tt.want) + } + }) + } +} + +// finalizeCR guards status writes with a CR fetched through the cached client, which can +// lag a shard-label flip: cached labels still match this operator while the live object +// already belongs to another shard. finalizeCR must confirm ownership on live state and +// skip the status mutation entirely — a stale operator writing status would stomp on the +// operator that now owns the CR. +func Test_finalizeCRLiveOwnershipGuard(t *testing.T) { + scheme := apiMachineryRuntime.NewScheme() + if err := apiChk.AddToScheme(scheme); err != nil { + t.Fatalf("AddToScheme failed: %v", err) + } + + setWatchLabelSelector(t, testShardKey+"=stg") + + tests := []struct { + name string + liveLabels map[string]string + wantWrite bool + }{ + {"live labels flipped away: skip status write", map[string]string{testShardKey: "logs"}, false}, + {"live labels still match: status write proceeds", map[string]string{testShardKey: "stg"}, true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Cached view always shows this operator's shard; live view varies per case. + cachedCHK := newCHK("flipping-chk", map[string]string{testShardKey: "stg"}) + liveCHK := newCHK("flipping-chk", tt.liveLabels) + cachedClient := fake.NewClientBuilder().WithScheme(scheme). + WithObjects(cachedCHK).WithStatusSubresource(cachedCHK).Build() + liveClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(liveCHK).Build() + + c := NewController(cachedClient, liveClient, scheme, nil) + w := c.newWorker() + + mutated := false + err := w.finalizeCR(context.Background(), cachedCHK, commonTypes.UpdateStatusOptions{}, func(chk *apiChk.ClickHouseKeeperInstallation) { + mutated = true + }) + if err != nil { + t.Fatalf("finalizeCR() returned error: %v, want nil", err) + } + if mutated != tt.wantWrite { + t.Errorf("status mutation callback invoked = %v, want %v", mutated, tt.wantWrite) + } + }) + } +} + +// A reconcile request for a CHK that no longer exists must return cleanly regardless of selector. +func Test_reconcileNotFoundWithSelector(t *testing.T) { + scheme := apiMachineryRuntime.NewScheme() + if err := apiChk.AddToScheme(scheme); err != nil { + t.Fatalf("AddToScheme failed: %v", err) + } + fakeClient := fake.NewClientBuilder().WithScheme(scheme).Build() + c := &Controller{Client: fakeClient} + + setWatchLabelSelector(t, testShardKey+"=stg") + + result, err := c.Reconcile(context.Background(), ctrl.Request{ + NamespacedName: kubeTypes.NamespacedName{Namespace: "clickhouse", Name: "gone-chk"}, + }) + if err != nil { + t.Fatalf("Reconcile() of missing CHK returned error: %v, want clean return", err) + } + if result.Requeue || result.RequeueAfter != 0 { + t.Errorf("Reconcile() of missing CHK requested requeue %+v, want none", result) + } +} diff --git a/pkg/controller/chk/kube/cr.go b/pkg/controller/chk/kube/cr.go index c19121c1b..bf470aea7 100644 --- a/pkg/controller/chk/kube/cr.go +++ b/pkg/controller/chk/kube/cr.go @@ -29,6 +29,7 @@ import ( apiChk "github.com/altinity/clickhouse-operator/pkg/apis/clickhouse-keeper.altinity.com/v1" api "github.com/altinity/clickhouse-operator/pkg/apis/clickhouse.altinity.com/v1" commonTypes "github.com/altinity/clickhouse-operator/pkg/apis/common/types" + "github.com/altinity/clickhouse-operator/pkg/chop" "github.com/altinity/clickhouse-operator/pkg/util" ) @@ -130,6 +131,14 @@ func (c *CR) statusUpdateProcess(ctx context.Context, icr api.ICustomResource, o return fmt.Errorf("ERROR GetCR (%s/%s): NULL returned", namespace, name) } + // Sharding: validate ownership on the same object snapshot whose resourceVersion + // fences the update below. If the shard label flips after this read, the flip bumps + // the resourceVersion, the update conflicts, and the retry re-runs this check. + if chop.Config().HasWatchLabelSelector() && !chop.Config().IsCRWatched(cur.GetNamespace(), cur.GetLabels()) { + log.V(1).M(cr).F().Info("CR no longer matches watch label selector, skip status update: %s/%s", namespace, name) + return nil + } + // Update status of a real (current) object. cur.EnsureStatus().CopyFrom(cr.Status, opts.CopyStatusOptions) diff --git a/pkg/controller/chk/kube/cr_test.go b/pkg/controller/chk/kube/cr_test.go new file mode 100644 index 000000000..8a4640f01 --- /dev/null +++ b/pkg/controller/chk/kube/cr_test.go @@ -0,0 +1,161 @@ +package kube + +import ( + "context" + "testing" + + meta "k8s.io/apimachinery/pkg/apis/meta/v1" + apiMachineryRuntime "k8s.io/apimachinery/pkg/runtime" + kubeTypes "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" + + apiChk "github.com/altinity/clickhouse-operator/pkg/apis/clickhouse-keeper.altinity.com/v1" + commonTypes "github.com/altinity/clickhouse-operator/pkg/apis/common/types" + "github.com/altinity/clickhouse-operator/pkg/chop/choptest" +) + +const testShardKey = choptest.ShardLabelKey + +func init() { + // statusUpdateProcess() → chop.Config(), so the global chop singleton + // must be initialized before tests run. + choptest.EnsureInit() +} + +func newCHK(labels map[string]string) *apiChk.ClickHouseKeeperInstallation { + return &apiChk.ClickHouseKeeperInstallation{ + ObjectMeta: meta.ObjectMeta{ + Namespace: "clickhouse", + Name: "test-chk", + Labels: labels, + }, + } +} + +// statusUpdateProcess re-reads the CR and publishes status onto whatever it finds, +// adopting the new resourceVersion — so a shard-label flip landing after upstream +// ownership guards (e.g. finalizeCR) would still get status stomped by the stale +// operator. The ownership guard must be evaluated on the same object snapshot whose +// resourceVersion fences the write: a flip visible in the read is skipped here, and a +// flip landing after the read bumps the resourceVersion, so the update conflicts and +// the retry re-runs this check. +func TestStatusUpdateOwnershipGuard(t *testing.T) { + scheme := apiMachineryRuntime.NewScheme() + if err := apiChk.AddToScheme(scheme); err != nil { + t.Fatalf("AddToScheme failed: %v", err) + } + + tests := []struct { + name string + selector string + storedLabels map[string]string + wantWrite bool + }{ + {"stored labels flipped away: skip status write", testShardKey + "=stg", map[string]string{testShardKey: "logs"}, false}, + {"stored labels match: status write proceeds", testShardKey + "=stg", map[string]string{testShardKey: "stg"}, true}, + {"unsharded mode: status write proceeds regardless of labels", "", map[string]string{testShardKey: "logs"}, true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if tt.selector != "" { + choptest.SetWatchLabelSelector(t, tt.selector) + } + + stored := newCHK(tt.storedLabels) + fakeClient := fake.NewClientBuilder().WithScheme(scheme). + WithObjects(stored).WithStatusSubresource(stored).Build() + c := NewCR(fakeClient) + + // The in-flight reconcile's view of the CR, carrying the status to publish. + desired := newCHK(map[string]string{testShardKey: "stg"}) + desired.EnsureStatus().CHOpVersion = "test-version" + + err := c.StatusUpdate(context.Background(), desired, commonTypes.UpdateStatusOptions{ + CopyStatusOptions: commonTypes.CopyStatusOptions{ + CopyStatusFieldGroup: commonTypes.CopyStatusFieldGroup{FieldGroupWholeStatus: true}, + }, + }) + if err != nil { + t.Fatalf("StatusUpdate() returned error: %v, want nil", err) + } + + after := &apiChk.ClickHouseKeeperInstallation{} + if err := fakeClient.Get(context.Background(), kubeTypes.NamespacedName{Namespace: "clickhouse", Name: "test-chk"}, after); err != nil { + t.Fatalf("Get after StatusUpdate failed: %v", err) + } + gotWrite := after.Status != nil && after.Status.CHOpVersion == "test-version" + if gotWrite != tt.wantWrite { + t.Errorf("status written = %v, want %v", gotWrite, tt.wantWrite) + } + }) + } +} + +// The key concurrency guarantee of the snapshot-fenced guard: a shard-label flip landing +// AFTER the ownership check's snapshot read must not slip through. The write carries the +// pre-flip resourceVersion, so it conflicts; the retry re-reads the flipped object and +// the guard skips. Without the guard the retry would re-read, copy status onto the +// flipped object and publish it with the fresh resourceVersion — a stale status write. +func TestStatusUpdateConflictRetrySkips(t *testing.T) { + scheme := apiMachineryRuntime.NewScheme() + if err := apiChk.AddToScheme(scheme); err != nil { + t.Fatalf("AddToScheme failed: %v", err) + } + choptest.SetWatchLabelSelector(t, testShardKey+"=stg") + + nn := kubeTypes.NamespacedName{Namespace: "clickhouse", Name: "test-chk"} + stored := newCHK(map[string]string{testShardKey: "stg"}) + + attempts := 0 + fakeClient := fake.NewClientBuilder().WithScheme(scheme). + WithObjects(stored).WithStatusSubresource(stored). + WithInterceptorFuncs(interceptor.Funcs{ + SubResourceUpdate: func(ctx context.Context, c client.Client, subResourceName string, obj client.Object, opts ...client.SubResourceUpdateOption) error { + if subResourceName == "status" { + attempts++ + if attempts == 1 { + // Simulate the race: the shard label flips away between the + // ownership snapshot read and this status write. The flip bumps + // the stored resourceVersion, so the delegated update below + // (carrying the pre-flip resourceVersion) must conflict. + live := &apiChk.ClickHouseKeeperInstallation{} + if err := c.Get(ctx, nn, live); err != nil { + t.Fatalf("interceptor Get failed: %v", err) + } + live.Labels = map[string]string{testShardKey: "logs"} + if err := c.Update(ctx, live); err != nil { + t.Fatalf("interceptor label flip failed: %v", err) + } + } + } + return c.SubResource(subResourceName).Update(ctx, obj, opts...) + }, + }).Build() + c := NewCR(fakeClient) + + desired := newCHK(map[string]string{testShardKey: "stg"}) + desired.EnsureStatus().CHOpVersion = "test-version" + + err := c.StatusUpdate(context.Background(), desired, commonTypes.UpdateStatusOptions{ + CopyStatusOptions: commonTypes.CopyStatusOptions{ + CopyStatusFieldGroup: commonTypes.CopyStatusFieldGroup{FieldGroupWholeStatus: true}, + }, + }) + if err != nil { + t.Fatalf("StatusUpdate() returned error: %v, want nil (retry must skip cleanly)", err) + } + + if attempts != 1 { + t.Errorf("status update attempts = %d, want exactly 1 (first attempt conflicts, retry must skip without a second write)", attempts) + } + + after := &apiChk.ClickHouseKeeperInstallation{} + if err := fakeClient.Get(context.Background(), nn, after); err != nil { + t.Fatalf("Get after StatusUpdate failed: %v", err) + } + if after.Status != nil && after.Status.CHOpVersion == "test-version" { + t.Errorf("stale status was published after the conflict; retry must skip the flipped CR") + } +} diff --git a/pkg/controller/chk/worker-deleter.go b/pkg/controller/chk/worker-deleter.go index eef72d355..25f8e58a1 100644 --- a/pkg/controller/chk/worker-deleter.go +++ b/pkg/controller/chk/worker-deleter.go @@ -24,6 +24,7 @@ import ( log "github.com/altinity/clickhouse-operator/pkg/announcer" api "github.com/altinity/clickhouse-operator/pkg/apis/clickhouse.altinity.com/v1" + "github.com/altinity/clickhouse-operator/pkg/chop" a "github.com/altinity/clickhouse-operator/pkg/controller/common/announcer" "github.com/altinity/clickhouse-operator/pkg/model" chkLabeler "github.com/altinity/clickhouse-operator/pkg/model/chk/tags/labeler" @@ -42,6 +43,15 @@ func (w *worker) clean(ctx context.Context, cr api.ICustomResource) { M(cr).F(). Info("remove items scheduled for deletion") + // Re-check ownership on live labels before the destructive purge; skipping is recoverable, + // deleting another shard's objects is not. No-op unless a selector is configured. + if chop.Config().HasWatchLabelSelector() { + if !w.c.ownsLiveCR(ctx, cr.GetNamespace(), cr.GetName()) { + w.a.V(1).M(cr).F().Warning("skip purge: CR ownership not confirmed (flipped to another shard or lookup failed): %s/%s", cr.GetNamespace(), cr.GetName()) + return + } + } + // Remove deleted items w.a.V(1).M(cr).F().Info("List of objects which have failed to reconcile:\n%s", w.task.RegistryFailed()) w.a.V(1).M(cr).F().Info("List of successfully reconciled objects:\n%s", w.task.RegistryReconciled()) diff --git a/pkg/controller/chk/worker-reconciler-chk.go b/pkg/controller/chk/worker-reconciler-chk.go index 1bd95a4e7..49c9b9231 100644 --- a/pkg/controller/chk/worker-reconciler-chk.go +++ b/pkg/controller/chk/worker-reconciler-chk.go @@ -21,7 +21,6 @@ import ( "time" core "k8s.io/api/core/v1" - meta "k8s.io/apimachinery/pkg/apis/meta/v1" log "github.com/altinity/clickhouse-operator/pkg/announcer" apiChk "github.com/altinity/clickhouse-operator/pkg/apis/clickhouse-keeper.altinity.com/v1" @@ -185,15 +184,6 @@ func (w *worker) buildCR(ctx context.Context, _cr *apiChk.ClickHouseKeeperInstal return cr } -func (w *worker) buildCRFromObj(ctx context.Context, obj meta.Object) (*apiChk.ClickHouseKeeperInstallation, error) { - _cr, err := w.c.GetCR(obj) - if err != nil { - w.a.M(obj).F().Error("UNABLE-1 to find obj by labels: %v err: %v", obj.GetLabels(), err) - return nil, err - } - return w.buildCR(ctx, _cr), nil -} - func (w *worker) buildTemplates(chi *apiChk.ClickHouseKeeperInstallation) (templates []*apiChk.ClickHouseKeeperInstallation) { return templates } diff --git a/pkg/controller/chk/worker.go b/pkg/controller/chk/worker.go index 61fa1b613..7ebd6cddf 100644 --- a/pkg/controller/chk/worker.go +++ b/pkg/controller/chk/worker.go @@ -25,6 +25,7 @@ import ( apiChk "github.com/altinity/clickhouse-operator/pkg/apis/clickhouse-keeper.altinity.com/v1" api "github.com/altinity/clickhouse-operator/pkg/apis/clickhouse.altinity.com/v1" "github.com/altinity/clickhouse-operator/pkg/apis/common/types" + "github.com/altinity/clickhouse-operator/pkg/chop" "github.com/altinity/clickhouse-operator/pkg/controller/common" a "github.com/altinity/clickhouse-operator/pkg/controller/common/announcer" "github.com/altinity/clickhouse-operator/pkg/controller/common/poller/domain" @@ -203,12 +204,29 @@ func (w *worker) finalizeCR( updateStatusOpts types.UpdateStatusOptions, f func(*apiChk.ClickHouseKeeperInstallation), ) error { - chi, err := w.buildCRFromObj(ctx, obj) + raw, err := w.c.GetCR(obj) if err != nil { log.V(1).Error("Unable to finalize CR: %s err: %v", util.NamespacedName(obj), err) return err } + // Re-check ownership on the RAW CR before the status write — checked pre-normalization + // because template merging could alter labels (mirrors the CHI-side finalizeCR guard). + if !chop.Config().IsCRWatched(raw.GetNamespace(), raw.GetLabels()) { + log.V(1).Info("CR no longer matches this operator's watch scope, skip finalize: %s", util.NamespacedName(obj)) + return nil + } + + // The read above goes through the cached client and can lag a shard-label flip; + // confirm ownership on live state before the status write (mirrors the Reconcile + // entry guard). + if chop.Config().HasWatchLabelSelector() && !w.c.ownsLiveCR(ctx, raw.GetNamespace(), raw.GetName()) { + log.V(1).Info("live CR ownership not confirmed (shard label flipped?), skip finalize: %s", util.NamespacedName(obj)) + return nil + } + + chi := w.buildCR(ctx, raw) + if f != nil { f(chi) } diff --git a/pkg/metrics/clickhouse/exporter.go b/pkg/metrics/clickhouse/exporter.go index 7d0fd3e23..6094c7ba6 100644 --- a/pkg/metrics/clickhouse/exporter.go +++ b/pkg/metrics/clickhouse/exporter.go @@ -181,5 +181,11 @@ func (e *Exporter) shouldWatchCR(chi *api.ClickHouseInstallation) bool { return false } + // Respect watch.labelSelector so shard exporters don't double-scrape other shards' CHIs + if !chop.Config().IsLabelSelectorWatched(chi.GetLabels()) { + log.V(1).Infof("CHI %s/%s labels do not match watch.labelSelector, unable to watch it", chi.Namespace, chi.Name) + return false + } + return true } diff --git a/pkg/metrics/clickhouse/exporter_watch_label_test.go b/pkg/metrics/clickhouse/exporter_watch_label_test.go new file mode 100644 index 000000000..45e70e590 --- /dev/null +++ b/pkg/metrics/clickhouse/exporter_watch_label_test.go @@ -0,0 +1,71 @@ +package clickhouse + +import ( + "testing" + + meta "k8s.io/apimachinery/pkg/apis/meta/v1" + + api "github.com/altinity/clickhouse-operator/pkg/apis/clickhouse.altinity.com/v1" + "github.com/altinity/clickhouse-operator/pkg/apis/common/types" + "github.com/altinity/clickhouse-operator/pkg/chop/choptest" +) + +const testShardKey = choptest.ShardLabelKey + +func init() { + // shouldWatchCR() → chop.Config(), so the global chop singleton must be initialized. + choptest.EnsureInit() +} + +var setWatchLabelSelector = choptest.SetWatchLabelSelector + +func newDiscoveredCHI(labels map[string]string) *api.ClickHouseInstallation { + return &api.ClickHouseInstallation{ + ObjectMeta: meta.ObjectMeta{ + Namespace: "clickhouse", + Name: "test-chi", + Labels: labels, + }, + } +} + +func Test_shouldWatchCRWithLabelSelector(t *testing.T) { + e := &Exporter{} + + tests := []struct { + name string + selector string + labels map[string]string + want bool + }{ + {"no selector watches everything (backward compat)", "", map[string]string{testShardKey: "logs"}, true}, + {"no selector watches unlabeled", "", nil, true}, + {"shard exporter watches matching CHI", testShardKey + "=stg", map[string]string{testShardKey: "stg"}, true}, + {"shard exporter skips other shard's CHI", testShardKey + "=stg", map[string]string{testShardKey: "logs"}, false}, + {"shard exporter skips unlabeled CHI", testShardKey + "=stg", nil, false}, + {"legacy exporter watches unlabeled CHI", "!" + testShardKey, nil, true}, + {"legacy exporter skips shard-labeled CHI", "!" + testShardKey, map[string]string{testShardKey: "stg"}, false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if tt.selector != "" { + setWatchLabelSelector(t, tt.selector) + } + if got := e.shouldWatchCR(newDiscoveredCHI(tt.labels)); got != tt.want { + t.Errorf("shouldWatchCR() = %v, want %v", got, tt.want) + } + }) + } +} + +// A stopped CHI is never watched, selector or not. +func Test_shouldWatchCRStoppedStillSkipped(t *testing.T) { + e := &Exporter{} + setWatchLabelSelector(t, testShardKey+"=stg") + + chi := newDiscoveredCHI(map[string]string{testShardKey: "stg"}) + chi.Spec.Stop = types.NewStringBool(true) + if e.shouldWatchCR(chi) { + t.Error("stopped CHI watched, want skipped") + } +} diff --git a/pkg/metrics/operator/label_selector_skips.go b/pkg/metrics/operator/label_selector_skips.go new file mode 100644 index 000000000..219c77c90 --- /dev/null +++ b/pkg/metrics/operator/label_selector_skips.go @@ -0,0 +1,51 @@ +// Copyright 2019 Altinity Ltd and/or its affiliates. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package operator + +import ( + "context" + "sync" + + "go.opentelemetry.io/otel/attribute" + otelApi "go.opentelemetry.io/otel/metric" +) + +var ( + crSkippedByLabelSelector otelApi.Int64Counter + crSkippedByLabelSelectorOnce sync.Once +) + +// CRSkippedByLabelSelector counts CR events skipped because the CR labels do not match this +// operator's watch.labelSelector. Informer resync re-fires events periodically, so a CR whose +// label matches NO operator (orphaned shard value) shows up as a steady skip rate on every +// operator while appearing in no operator's watched-CR metrics — alert on that combination. +func CRSkippedByLabelSelector(kind string, namespace string, name string) { + if Meter() == nil { + // Metrics machinery not started (unit tests, exporter binary) + return + } + crSkippedByLabelSelectorOnce.Do(func() { + crSkippedByLabelSelector, _ = Meter().Int64Counter( + "clickhouse_operator_cr_skipped_by_label_selector", + otelApi.WithDescription("number of CR events skipped because CR labels do not match watch.labelSelector"), + otelApi.WithUnit("items"), + ) + }) + crSkippedByLabelSelector.Add(context.Background(), 1, otelApi.WithAttributes( + attribute.String("kind", kind), + attribute.String("namespace", namespace), + attribute.String("name", name), + )) +} diff --git a/pkg/metrics/operator/machinery.go b/pkg/metrics/operator/machinery.go index b0d6637cc..3853c9fb9 100644 --- a/pkg/metrics/operator/machinery.go +++ b/pkg/metrics/operator/machinery.go @@ -15,11 +15,13 @@ package operator import ( + "context" "fmt" "net/http" prom "github.com/prometheus/client_golang/prometheus" "github.com/prometheus/client_golang/prometheus/promhttp" + "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/exporters/prometheus" otelApi "go.opentelemetry.io/otel/metric" "go.opentelemetry.io/otel/sdk/metric" @@ -86,6 +88,8 @@ func StartMetricsExporter(endpoint, path string) { meter = meterProvider.Meter("clickhouse-operator-meter", otelApi.WithInstrumentationVersion(version.Version)) + recordOperatorInfo() + // Start the prometheus HTTP server and pass the exporter Collector to it serveMetrics(endpoint, path) } @@ -96,6 +100,25 @@ func Meter() otelApi.Meter { return meter } +// recordOperatorInfo publishes an info-style metric (constant 1) carrying this operator +// instance's shard identity. Joinable against clickhouse_operator_cr_skipped_by_label_selector +// to identify CRs whose shard label matches no running operator. +func recordOperatorInfo() { + info, err := meter.Int64Gauge( + "clickhouse_operator_info", + otelApi.WithDescription("operator instance configuration info; value is always 1"), + ) + if err != nil { + log.V(1).Warning("failed to create clickhouse_operator_info metric: %v", err) + return + } + info.Record(context.Background(), 1, otelApi.WithAttributes( + attribute.String("watch_label_selector", chop.Config().Watch.LabelSelector), + attribute.Bool("require_label_selector", chop.Config().Watch.RequireLabelSelector), + attribute.String("version", version.Version), + )) +} + func serveMetrics(addr, path string) { fmt.Printf("start serving metrics at: %s%s\n", addr, path) // Use ContinueOnError so that a single untranslatable OTel metric (e.g. a metric