From 13c3fd5742dad25a1faf064ccd26020b72b11796 Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Tue, 25 Aug 2026 13:53:05 +0200 Subject: [PATCH 1/3] add reconcile early-exit, misses watches and tests --- .../templates/clusterrole-operator.yaml | 25 ++--- rust/operator-binary/src/main.rs | 30 ++++-- rust/operator-binary/src/zk_controller.rs | 63 ++++++++++++- .../kuttl/cluster-operation/50-assert.yaml | 94 +++++++++++++++++++ .../50-delete-owned-resources.yaml | 63 +++++++++++++ 5 files changed, 249 insertions(+), 26 deletions(-) create mode 100644 tests/templates/kuttl/cluster-operation/50-assert.yaml create mode 100644 tests/templates/kuttl/cluster-operation/50-delete-owned-resources.yaml diff --git a/deploy/helm/zookeeper-operator/templates/clusterrole-operator.yaml b/deploy/helm/zookeeper-operator/templates/clusterrole-operator.yaml index 8ed7ed5b..26f73b1f 100644 --- a/deploy/helm/zookeeper-operator/templates/clusterrole-operator.yaml +++ b/deploy/helm/zookeeper-operator/templates/clusterrole-operator.yaml @@ -13,13 +13,14 @@ rules: - nodes/proxy verbs: - get - # Manage core workload resources created per ZookeeperCluster. - # All resources are applied via SSA (create + patch) and tracked for - # orphan cleanup (list + delete). + # Manage core workload resources created per ZookeeperCluster (the ServiceAccount + # provides workload pod identity). All resources are applied via SSA (create + + # patch), tracked for orphan cleanup (list + delete) and watched by the controller. - apiGroups: - "" resources: - configmaps + - serviceaccounts - services verbs: - create @@ -28,20 +29,9 @@ rules: - list - patch - watch - # ServiceAccount created per ZookeeperCluster for workload pod identity. - # Applied via SSA and tracked for orphan cleanup. - - apiGroups: - - "" - resources: - - serviceaccounts - verbs: - - create - - delete - - get - - list - - patch # RoleBinding created per ZookeeperCluster to bind the product ClusterRole to the workload - # ServiceAccount. Applied via SSA and tracked for orphan cleanup. + # ServiceAccount. Applied via SSA and tracked for orphan cleanup and watched by the + # controller. - apiGroups: - rbac.authorization.k8s.io resources: @@ -52,6 +42,7 @@ rules: - get - list - patch + - watch # Required to bind the product ClusterRole to the per-cluster ServiceAccount. - apiGroups: - rbac.authorization.k8s.io @@ -75,6 +66,7 @@ rules: - patch - watch # PodDisruptionBudget created per role. Applied via SSA and tracked for orphan cleanup. + # Watched by the controller. - apiGroups: - policy resources: @@ -85,6 +77,7 @@ rules: - get - list - patch + - watch # Required for maintaining the CRDs (including the conversion webhook certificate). # Also required for the startup condition check. - apiGroups: diff --git a/rust/operator-binary/src/main.rs b/rust/operator-binary/src/main.rs index c7d5d4d0..165036fb 100644 --- a/rust/operator-binary/src/main.rs +++ b/rust/operator-binary/src/main.rs @@ -17,7 +17,9 @@ use stackable_operator::{ eos::EndOfSupportChecker, k8s_openapi::api::{ apps::v1::StatefulSet, - core::v1::{ConfigMap, Service}, + core::v1::{ConfigMap, Service, ServiceAccount}, + policy::v1::PodDisruptionBudget, + rbac::v1::RoleBinding, }, kube::{ CustomResourceExt as _, Resource, @@ -131,22 +133,34 @@ async fn main() -> anyhow::Result<()> { )); let zk_controller = zk_controller .owns( - watch_namespace.get_api::>(&client), + watch_namespace.get_api::>(&client), watcher::Config::default(), ) + // The discovery ConfigMap advertises the addresses that the listener operator + // publishes on the role Listener, so a reconciliation must run once they appear + // or change. .owns( - watch_namespace.get_api::>(&client), + watch_namespace.get_api::>(&client), watcher::Config::default(), ) .owns( - watch_namespace.get_api::>(&client), + watch_namespace.get_api::>(&client), watcher::Config::default(), ) - // The discovery ConfigMap advertises the addresses that the listener operator - // publishes on the role Listener, so a reconciliation must run once they appear - // or change. .owns( - watch_namespace.get_api::>(&client), + watch_namespace.get_api::>(&client), + watcher::Config::default(), + ) + .owns( + watch_namespace.get_api::>(&client), + watcher::Config::default(), + ) + .owns( + watch_namespace.get_api::>(&client), + watcher::Config::default(), + ) + .owns( + watch_namespace.get_api::>(&client), watcher::Config::default(), ) .graceful_shutdown_on(sigterm_watcher.handle()) diff --git a/rust/operator-binary/src/zk_controller.rs b/rust/operator-binary/src/zk_controller.rs index 9f80646b..4719536b 100644 --- a/rust/operator-binary/src/zk_controller.rs +++ b/rust/operator-binary/src/zk_controller.rs @@ -19,6 +19,7 @@ use stackable_operator::{ rbac::v1::RoleBinding, }, kube::{ + Resource, api::DynamicObject, core::{DeserializeGuard, error_boundary}, runtime::controller, @@ -127,6 +128,11 @@ pub async fn reconcile_zk( ctx: Arc, ) -> Result { tracing::info!("Starting reconcile"); + + if zk.meta().deletion_timestamp.is_some() { + return Ok(controller::Action::await_change()); + } + let zk = zk.0.as_ref() .map_err(error_boundary::InvalidObject::clone) @@ -271,14 +277,20 @@ pub(crate) mod test_support { mod tests { use std::str::FromStr; + use std::sync::Arc; + use stackable_operator::{ - k8s_openapi::api::core::v1::ConfigMap, v2::types::operator::RoleGroupName, + cli::OperatorEnvironmentOptions, + client::Client, + k8s_openapi::api::core::v1::ConfigMap, + kube::{Client as KubeClient, Config, runtime::controller::Action}, + v2::types::operator::RoleGroupName, }; use crate::{ crd::ZookeeperRole, zk_controller::{ - CONTROLLER_NAME, + CONTROLLER_NAME, Ctx, reconcile_zk, build::resource::config_map, test_support::{cluster_info, minimal_zk, validated_cluster}, }, @@ -572,4 +584,51 @@ mod tests { ) .unwrap() } + + /// The client points at a closed port, so any API call would fail the reconciliation: an `Ok` + /// proves that a cluster being deleted returns before the reconciler touches the Kubernetes + /// API, and because the spec is invalid, before the `DeserializeGuard` is unwrapped. + #[test] + fn reconcile_exits_early_for_deleted_cluster() { + let zk = serde_yaml::from_str( + r#" +apiVersion: zookeeper.stackable.tech/v1alpha1 +kind: ZookeeperCluster +metadata: + name: zookeeper + namespace: default + deletionTimestamp: "2026-08-14T12:00:00Z" +spec: {} +"#, + ) + .expect("YAML parses; the invalid spec is captured inside the DeserializeGuard"); + + let action = tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .expect("current-thread tokio runtime") + .block_on(async { + let ctx = Arc::new(Ctx { + client: Client::new( + KubeClient::try_from(Config::new( + "http://127.0.0.1:1".parse().expect("valid static URI"), + )) + .expect("client from static config"), + None, + "default".to_owned(), + cluster_info(), + ), + operator_environment: OperatorEnvironmentOptions { + operator_namespace: "stackable-operators".to_owned(), + operator_service_name: "zookeeper-operator".to_owned(), + image_repository: "oci.stackable.tech/sdp".to_owned(), + }, + }); + + reconcile_zk(Arc::new(zk), ctx).await + }) + .expect("a deleted cluster reconciles without any API call"); + + assert_eq!(action, Action::await_change()); + } } diff --git a/tests/templates/kuttl/cluster-operation/50-assert.yaml b/tests/templates/kuttl/cluster-operation/50-assert.yaml new file mode 100644 index 00000000..079945c5 --- /dev/null +++ b/tests/templates/kuttl/cluster-operation/50-assert.yaml @@ -0,0 +1,94 @@ +--- +# The recreated StatefulSet must bring the cluster back to ready, and the recreated +# objects must carry an owner reference back to the ZookeeperCluster so that garbage +# collection still works for them. +apiVersion: kuttl.dev/v1beta1 +kind: TestAssert +metadata: + name: recreate-owned-resources +timeout: 600 +commands: + - script: kubectl -n $NAMESPACE wait --for=condition=available zookeeperclusters.zookeeper.stackable.tech/test-zk --timeout 601s +--- +apiVersion: apps/v1 +kind: StatefulSet +metadata: + name: test-zk-server-default + ownerReferences: + - apiVersion: zookeeper.stackable.tech/v1alpha1 + controller: true + kind: ZookeeperCluster + name: test-zk +status: + readyReplicas: 1 + replicas: 1 +--- +apiVersion: v1 +kind: ServiceAccount +metadata: + name: test-zk-serviceaccount + ownerReferences: + - apiVersion: zookeeper.stackable.tech/v1alpha1 + controller: true + kind: ZookeeperCluster + name: test-zk +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: RoleBinding +metadata: + name: test-zk-rolebinding + ownerReferences: + - apiVersion: zookeeper.stackable.tech/v1alpha1 + controller: true + kind: ZookeeperCluster + name: test-zk +--- +apiVersion: policy/v1 +kind: PodDisruptionBudget +metadata: + name: test-zk-server + ownerReferences: + - apiVersion: zookeeper.stackable.tech/v1alpha1 + controller: true + kind: ZookeeperCluster + name: test-zk +--- +apiVersion: v1 +kind: ConfigMap +metadata: + name: test-zk + ownerReferences: + - apiVersion: zookeeper.stackable.tech/v1alpha1 + controller: true + kind: ZookeeperCluster + name: test-zk +--- +apiVersion: listeners.stackable.tech/v1alpha1 +kind: Listener +metadata: + name: test-zk-server + ownerReferences: + - apiVersion: zookeeper.stackable.tech/v1alpha1 + controller: true + kind: ZookeeperCluster + name: test-zk +--- +apiVersion: v1 +kind: Service +metadata: + name: test-zk-server-default-headless + ownerReferences: + - apiVersion: zookeeper.stackable.tech/v1alpha1 + controller: true + kind: ZookeeperCluster + name: test-zk +--- +apiVersion: v1 +kind: Service +metadata: + name: test-zk-server-default-metrics + ownerReferences: + - apiVersion: zookeeper.stackable.tech/v1alpha1 + controller: true + kind: ZookeeperCluster + name: test-zk diff --git a/tests/templates/kuttl/cluster-operation/50-delete-owned-resources.yaml b/tests/templates/kuttl/cluster-operation/50-delete-owned-resources.yaml new file mode 100644 index 00000000..15fe0ded --- /dev/null +++ b/tests/templates/kuttl/cluster-operation/50-delete-owned-resources.yaml @@ -0,0 +1,63 @@ +--- +# Every resource the operator applies carries an ownerReference and a `.owns()` watch +# (main.rs): deleting it must trigger a reconcile of the ZookeeperCluster that +# re-applies it, proving the `.owns()` routing and the ClusterRole `watch` verbs end +# to end. `.watches()` registrations can't be tested this way: the operator never +# recreates what it didn't apply. +# +# Resources are discovered by label (ClusterResources::add enforces the labels on +# everything the operator applies), so new resources and kinds are covered +# automatically. Labels over-match on derived objects (e.g. the role Listener's +# backing Service, created by the listener-operator), so each match must also carry +# a controller ownerReference pointing at the ZookeeperCluster; kinds that can never +# pass that gate are excluded up front. Recreation is proven by UID change, and a +# floor guard catches a selector that silently matches nothing. ZookeeperZnode +# resources are untouched: their objects carry the znode controller's `managed-by`. +apiVersion: kuttl.dev/v1beta1 +kind: TestStep +metadata: + name: delete-owned-resources +timeout: 300 +commands: + - script: | + set -eu + + delete_and_await_recreation() { + resource=$1 + old_uid=$(kubectl get -n "$NAMESPACE" "$resource" -o jsonpath='{.metadata.uid}') + kubectl delete -n "$NAMESPACE" "$resource" --wait=false + # Recreation is a single reconcile away, so this normally succeeds on the + # first iteration; 30s is a generous upper bound well below the step timeout. + for _ in $(seq 1 30); do + new_uid=$(kubectl get -n "$NAMESPACE" "$resource" -o jsonpath='{.metadata.uid}' 2>/dev/null || true) + if [ -n "$new_uid" ] && [ "$new_uid" != "$old_uid" ]; then + return 0 + fi + sleep 1 + done + echo "$resource was not recreated (old uid: $old_uid, current: '${new_uid:-}')" >&2 + return 1 + } + + selector="app.kubernetes.io/instance=test-zk,app.kubernetes.io/managed-by=zookeeper.stackable.tech_zookeepercluster" + excluded="^(pods|persistentvolumeclaims|endpoints|events|secrets)$|^endpointslices\.|^controllerrevisions\.|^events\." + + deleted=0 + for kind in $(kubectl api-resources --verbs=list --namespaced -o name | grep -Ev "$excluded" | sort); do + for resource in $(kubectl get -n "$NAMESPACE" "$kind" -l "$selector" -o name 2>/dev/null); do + owner=$(kubectl get -n "$NAMESPACE" "$resource" -o jsonpath='{.metadata.ownerReferences[?(@.controller==true)].kind}/{.metadata.ownerReferences[?(@.controller==true)].name}' 2>/dev/null || true) + if [ "$owner" != "ZookeeperCluster/test-zk" ]; then + echo "skipping $resource: controller owner is '${owner:-none}', not the ZookeeperCluster" + continue + fi + delete_and_await_recreation "$resource" + deleted=$((deleted + 1)) + done + done + + # Guard against the sweep silently matching nothing (wrong selector, renamed + # labels): the fixture is known to produce well over this many owned resources. + if [ "$deleted" -lt 6 ]; then + echo "only $deleted labelled resources were swept - the label selector is broken" >&2 + exit 1 + fi From 6c275d6e07b9931413f14043b26f35304484bf7e Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Tue, 25 Aug 2026 13:54:29 +0200 Subject: [PATCH 2/3] changelog --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3f043d1e..c7794a53 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,6 +37,8 @@ All notable changes to this project will be documented in this file. - Fix a longstanding problem of including empty `categories`, `shortNames` and `additionalPrinterColumns` in the CRDs, which could cause problems with GitOps tools (e.g. ArgoCD) reporting a diff in the custom resources. See [our internal issue](https://github.com/stackabletech/hdfs-operator/issues/626) and [the fix](https://github.com/kube-rs/kube/pull/2042) for details ([#1070]). +- The operator now watches all resources that it creates and early-exits the reconcile action when the + cluster is marked for deletion ([#1079]). [#1053]: https://github.com/stackabletech/zookeeper-operator/pull/1053 [#1060]: https://github.com/stackabletech/zookeeper-operator/pull/1060 @@ -45,6 +47,7 @@ All notable changes to this project will be documented in this file. [#1069]: https://github.com/stackabletech/zookeeper-operator/pull/1069 [#1070]: https://github.com/stackabletech/zookeeper-operator/pull/1070 [#1077]: https://github.com/stackabletech/zookeeper-operator/pull/1077 +[#1079]: https://github.com/stackabletech/zookeeper-operator/pull/1079 ## [26.7.0] - 2026-07-21 From 704066e891bb9765a26693eded0ce5f640b390bb Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Tue, 25 Aug 2026 14:15:33 +0200 Subject: [PATCH 3/3] linting --- rust/operator-binary/src/zk_controller.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/rust/operator-binary/src/zk_controller.rs b/rust/operator-binary/src/zk_controller.rs index 4719536b..67871a50 100644 --- a/rust/operator-binary/src/zk_controller.rs +++ b/rust/operator-binary/src/zk_controller.rs @@ -275,9 +275,7 @@ pub(crate) mod test_support { #[cfg(test)] mod tests { - use std::str::FromStr; - - use std::sync::Arc; + use std::{str::FromStr, sync::Arc}; use stackable_operator::{ cli::OperatorEnvironmentOptions, @@ -290,8 +288,9 @@ mod tests { use crate::{ crd::ZookeeperRole, zk_controller::{ - CONTROLLER_NAME, Ctx, reconcile_zk, + CONTROLLER_NAME, Ctx, build::resource::config_map, + reconcile_zk, test_support::{cluster_info, minimal_zk, validated_cluster}, }, };