From 90009b47bd4f7de9276431a3827bf7c0cbc142a7 Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Tue, 25 Aug 2026 11:49:00 +0200 Subject: [PATCH 1/3] add reconcile early-exit, missing watches and tests --- .../templates/clusterrole-operator.yaml | 33 ++++---- rust/operator-binary/src/controller.rs | 65 +++++++++++++- rust/operator-binary/src/main.rs | 33 ++++++-- .../kuttl/cluster-operation/60-assert.yaml | 84 +++++++++++++++++++ .../60-delete-owned-resources.yaml | 65 ++++++++++++++ 5 files changed, 255 insertions(+), 25 deletions(-) create mode 100644 tests/templates/kuttl/cluster-operation/60-assert.yaml create mode 100644 tests/templates/kuttl/cluster-operation/60-delete-owned-resources.yaml diff --git a/deploy/helm/superset-operator/templates/clusterrole-operator.yaml b/deploy/helm/superset-operator/templates/clusterrole-operator.yaml index 185fdc3b..42a1ab6e 100644 --- a/deploy/helm/superset-operator/templates/clusterrole-operator.yaml +++ b/deploy/helm/superset-operator/templates/clusterrole-operator.yaml @@ -13,15 +13,17 @@ rules: - nodes/proxy verbs: - get - # Manage core namespaced resources created per SupersetCluster. - # All resources are applied via Server-Side Apply (create + patch) and tracked for - # orphan cleanup (list + delete). ReconciliationPaused uses get. - # Secrets are needed for auto-generated SECRET_KEY (random_secret_creation). + # Manage core namespaced resources created per SupersetCluster (the ServiceAccount is + # also created per DruidConnection and provides workload pod identity). + # All resources are applied via Server-Side Apply (create + patch), tracked for + # orphan cleanup (list + delete) and watched by the controller. ReconciliationPaused + # uses get. Secrets are needed for auto-generated SECRET_KEY (random_secret_creation). - apiGroups: - "" resources: - configmaps - secrets + - serviceaccounts - services verbs: - create @@ -30,20 +32,9 @@ rules: - list - patch - watch - # ServiceAccount created per SupersetCluster and per DruidConnection. - # Applied via SSA and tracked for orphan cleanup. - - apiGroups: - - "" - resources: - - serviceaccounts - verbs: - - create - - delete - - get - - list - - patch # RoleBinding created per SupersetCluster 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: @@ -54,6 +45,7 @@ rules: - get - list - patch + - watch # Required to bind the product ClusterRole to the per-cluster ServiceAccount. - apiGroups: - rbac.authorization.k8s.io @@ -98,7 +90,8 @@ rules: - list - patch - watch - # PodDisruptionBudget created per role. Applied via SSA and tracked for orphan cleanup. + # PodDisruptionBudget created per role. Applied via SSA and tracked for orphan cleanup + # and watched by the controller. - apiGroups: - policy resources: @@ -109,6 +102,7 @@ rules: - get - list - patch + - watch # Required for maintaining the CRDs within the operator (including the conversion webhook info). # Also for the startup condition check before the controller can run. - apiGroups: @@ -167,7 +161,7 @@ rules: - list - watch # Listener created per role group for external access. Applied via SSA and tracked for orphan - # cleanup. + # cleanup and watched by the controller. - apiGroups: - listeners.stackable.tech resources: @@ -178,3 +172,4 @@ rules: - get - list - patch + - watch diff --git a/rust/operator-binary/src/controller.rs b/rust/operator-binary/src/controller.rs index 8f9ad287..18f283c1 100644 --- a/rust/operator-binary/src/controller.rs +++ b/rust/operator-binary/src/controller.rs @@ -363,6 +363,10 @@ pub async fn reconcile_superset( ) -> Result { tracing::info!("Starting reconcile"); + if superset.meta().deletion_timestamp.is_some() { + return Ok(Action::await_change()); + } + let superset = superset .0 .as_ref() @@ -454,7 +458,16 @@ pub(crate) mod test_support { #[cfg(test)] mod controller_tests { - use super::{CONTROLLER_NAME, OPERATOR_NAME, PRODUCT_NAME}; + use std::str::FromStr; + + use stackable_operator::{ + client::Client, + commons::networking::DomainName, + kube::{Client as KubeClient, Config}, + utils::cluster_info::KubernetesClusterInfo, + }; + + use super::{CONTROLLER_NAME, OPERATOR_NAME, PRODUCT_NAME, *}; #[test] fn test_constants() { @@ -463,4 +476,54 @@ mod controller_tests { let _ = *OPERATOR_NAME; let _ = *CONTROLLER_NAME; } + + /// 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 superset = serde_yaml::from_str( + r#" +apiVersion: superset.stackable.tech/v1alpha1 +kind: SupersetCluster +metadata: + name: superset + 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(), + KubernetesClusterInfo { + cluster_domain: DomainName::from_str("cluster.local") + .expect("valid cluster domain"), + }, + ), + operator_environment: OperatorEnvironmentOptions { + operator_namespace: "stackable-operators".to_owned(), + operator_service_name: "superset-operator".to_owned(), + image_repository: "oci.stackable.tech/sdp".to_owned(), + }, + }); + + reconcile_superset(Arc::new(superset), ctx).await + }) + .expect("a deleted cluster reconciles without any API call"); + + assert_eq!(action, Action::await_change()); + } } diff --git a/rust/operator-binary/src/main.rs b/rust/operator-binary/src/main.rs index 950a5087..957d53b1 100644 --- a/rust/operator-binary/src/main.rs +++ b/rust/operator-binary/src/main.rs @@ -9,12 +9,14 @@ use futures::{FutureExt, StreamExt, TryFutureExt}; use stackable_operator::{ YamlSchema, cli::{Command, RunArguments}, - crd::authentication::core, + crd::{authentication::core, listener}, eos::EndOfSupportChecker, k8s_openapi::api::{ apps::v1::{Deployment, StatefulSet}, batch::v1::Job, - core::v1::{ConfigMap, Service}, + core::v1::{ConfigMap, Service, ServiceAccount}, + policy::v1::PodDisruptionBudget, + rbac::v1::RoleBinding, }, kube::{ CustomResourceExt as _, ResourceExt, @@ -134,17 +136,38 @@ async fn main() -> anyhow::Result<()> { let authentication_class_store = superset_controller.store(); let config_map_store = superset_controller.store(); let superset_controller = superset_controller + .owns( + watch_namespace.get_api::>(&client), + watcher::Config::default(), + ) + // Required for workers and beat. + .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(), + ) + .owns( + watch_namespace.get_api::>(&client), + watcher::Config::default(), + ) .owns( watch_namespace.get_api::>(&client), watcher::Config::default(), ) .owns( - watch_namespace.get_api::>(&client), + watch_namespace.get_api::>(&client), watcher::Config::default(), ) - // Required for workers and beat. .owns( - watch_namespace.get_api::>(&client), + watch_namespace.get_api::>(&client), watcher::Config::default(), ) .watches( diff --git a/tests/templates/kuttl/cluster-operation/60-assert.yaml b/tests/templates/kuttl/cluster-operation/60-assert.yaml new file mode 100644 index 00000000..ef02e8ea --- /dev/null +++ b/tests/templates/kuttl/cluster-operation/60-assert.yaml @@ -0,0 +1,84 @@ +--- +# The recreated StatefulSet must bring the cluster back to ready, and the recreated +# objects must carry an owner reference back to the SupersetCluster 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 supersetclusters.superset.stackable.tech/superset --timeout 601s +--- +apiVersion: apps/v1 +kind: StatefulSet +metadata: + name: superset-node-default + ownerReferences: + - apiVersion: superset.stackable.tech/v1alpha1 + controller: true + kind: SupersetCluster + name: superset +status: + readyReplicas: 2 + replicas: 2 +--- +apiVersion: v1 +kind: ServiceAccount +metadata: + name: superset-serviceaccount + ownerReferences: + - apiVersion: superset.stackable.tech/v1alpha1 + controller: true + kind: SupersetCluster + name: superset +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: RoleBinding +metadata: + name: superset-rolebinding + ownerReferences: + - apiVersion: superset.stackable.tech/v1alpha1 + controller: true + kind: SupersetCluster + name: superset +--- +apiVersion: policy/v1 +kind: PodDisruptionBudget +metadata: + name: superset-node + ownerReferences: + - apiVersion: superset.stackable.tech/v1alpha1 + controller: true + kind: SupersetCluster + name: superset +--- +apiVersion: listeners.stackable.tech/v1alpha1 +kind: Listener +metadata: + name: superset-node + ownerReferences: + - apiVersion: superset.stackable.tech/v1alpha1 + controller: true + kind: SupersetCluster + name: superset +--- +apiVersion: v1 +kind: Service +metadata: + name: superset-node-default-headless + ownerReferences: + - apiVersion: superset.stackable.tech/v1alpha1 + controller: true + kind: SupersetCluster + name: superset +--- +apiVersion: v1 +kind: Service +metadata: + name: superset-node-default-metrics + ownerReferences: + - apiVersion: superset.stackable.tech/v1alpha1 + controller: true + kind: SupersetCluster + name: superset diff --git a/tests/templates/kuttl/cluster-operation/60-delete-owned-resources.yaml b/tests/templates/kuttl/cluster-operation/60-delete-owned-resources.yaml new file mode 100644 index 00000000..4783f0a9 --- /dev/null +++ b/tests/templates/kuttl/cluster-operation/60-delete-owned-resources.yaml @@ -0,0 +1,65 @@ +--- +# Every resource the operator applies carries an ownerReference and a `.owns()` watch +# (main.rs): deleting it must trigger a reconcile of the SupersetCluster 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, so each match must also carry +# a controller ownerReference pointing at the SupersetCluster; 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. +# +# Secrets are excluded: the auto-generated SECRET_KEY Secret is a generate-once value +# that is deliberately not `.owns()`-registered, so deleting it would not trigger a +# recreation. +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=superset,app.kubernetes.io/managed-by=superset.stackable.tech_supersetcluster" + 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" != "SupersetCluster/superset" ]; then + echo "skipping $resource: controller owner is '${owner:-none}', not the SupersetCluster" + 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 7b229a05e076b08ddd4c08301307990401d514d7 Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Tue, 25 Aug 2026 11:58:22 +0200 Subject: [PATCH 2/3] changelog --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index d5bf78ba..1e8d6e84 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,6 +37,8 @@ - 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 ([#773]). +- The operator now watches all resources that it creates and early-exits the reconcile action when the + cluster is marked for deletion ([#781]). [#756]: https://github.com/stackabletech/superset-operator/pull/756 [#761]: https://github.com/stackabletech/superset-operator/pull/761 @@ -44,6 +46,7 @@ [#772]: https://github.com/stackabletech/superset-operator/pull/772 [#773]: https://github.com/stackabletech/superset-operator/pull/773 [#779]: https://github.com/stackabletech/superset-operator/pull/779 +[#781]: https://github.com/stackabletech/superset-operator/pull/781 ## [26.7.0] - 2026-07-21 From 5e0779539a7f3b8dba6c95fbb1270e9f1a40f378 Mon Sep 17 00:00:00 2001 From: Andrew Kenworthy Date: Wed, 26 Aug 2026 11:48:12 +0200 Subject: [PATCH 3/3] add reconcile early-exit for the druid-controller --- .../src/druid_connection_controller/mod.rs | 62 ++++++++++++++++++- 1 file changed, 60 insertions(+), 2 deletions(-) diff --git a/rust/operator-binary/src/druid_connection_controller/mod.rs b/rust/operator-binary/src/druid_connection_controller/mod.rs index e6f8e4ea..0519deac 100644 --- a/rust/operator-binary/src/druid_connection_controller/mod.rs +++ b/rust/operator-binary/src/druid_connection_controller/mod.rs @@ -17,7 +17,7 @@ use stackable_operator::{ core::v1::{ConfigMap, EnvVar, EnvVarSource, PodSpec, PodTemplateSpec, SecretKeySelector}, }, kube::{ - ResourceExt, + Resource, ResourceExt, core::{DeserializeGuard, DynamicObject, error_boundary}, runtime::{controller::Action, reflector::ObjectRef}, }, @@ -154,6 +154,10 @@ pub async fn reconcile_druid_connection( ) -> Result { tracing::info!("Starting reconciling DruidConnections"); + if druid_connection.meta().deletion_timestamp.is_some() { + return Ok(Action::await_change()); + } + let druid_connection = druid_connection .0 .as_ref() @@ -457,10 +461,64 @@ pub fn error_policy( #[cfg(test)] mod tests { - use stackable_operator::utils::yaml_from_str_singleton_map; + use stackable_operator::{ + commons::networking::DomainName, + kube::{Client as KubeClient, Config}, + utils::{cluster_info::KubernetesClusterInfo, yaml_from_str_singleton_map}, + }; use super::*; + /// The client points at a closed port, so any API call would fail the reconciliation: an `Ok` + /// proves that a connection 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_connection() { + let druid_connection = serde_yaml::from_str( + r#" +apiVersion: superset.stackable.tech/v1alpha1 +kind: DruidConnection +metadata: + name: simple-connection + 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(), + KubernetesClusterInfo { + cluster_domain: DomainName::from_str("cluster.local") + .expect("valid cluster domain"), + }, + ), + operator_environment: OperatorEnvironmentOptions { + operator_namespace: "stackable-operators".to_owned(), + operator_service_name: "superset-operator".to_owned(), + image_repository: "oci.stackable.tech/sdp".to_owned(), + }, + }); + + reconcile_druid_connection(Arc::new(druid_connection), ctx).await + }) + .expect("a deleted connection reconciles without any API call"); + + assert_eq!(action, Action::await_change()); + } + #[test] fn test_constants() { // Test that dereferencing the constants does not panic.