From 65493b44cfcef4e7a61aafe2d93adb68f11808b0 Mon Sep 17 00:00:00 2001 From: GatewayJ <18332154+GatewayJ@users.noreply.github.com> Date: Sat, 11 Jul 2026 20:48:03 +0800 Subject: [PATCH] fix: reconcile pod deletion policy reliably --- deploy/rustfs-operator/crds/tenant-crd.yaml | 2 + deploy/rustfs-operator/crds/tenant.yaml | 2 + docs/operator-user-guide.md | 4 +- docs/operator-user-guide.zh-CN.md | 4 +- src/lib.rs | 39 ++++++- src/reconcile.rs | 79 +++++++++++--- src/reconcile/phases.rs | 108 ++++++++++++++++++-- src/types/v1alpha1/k8s.rs | 9 +- src/types/v1alpha1/tenant.rs | 2 + 9 files changed, 217 insertions(+), 32 deletions(-) diff --git a/deploy/rustfs-operator/crds/tenant-crd.yaml b/deploy/rustfs-operator/crds/tenant-crd.yaml index 34303f5..fceeaa0 100644 --- a/deploy/rustfs-operator/crds/tenant-crd.yaml +++ b/deploy/rustfs-operator/crds/tenant-crd.yaml @@ -469,6 +469,8 @@ spec: StatefulSet controller can recreate it elsewhere. Force deletion requires the Node object to be deleted or marked with an effective `node.kubernetes.io/out-of-service` taint that the target Pod does not tolerate. + Before using force policies, operators must confirm the node is powered off or + otherwise isolated; deleting the Node object is treated as that operational assertion. Values: DoNothing | Delete | ForceDelete | DeleteStatefulSetPod | DeleteDeploymentPod | DeleteBothStatefulSetAndDeploymentPod enum: diff --git a/deploy/rustfs-operator/crds/tenant.yaml b/deploy/rustfs-operator/crds/tenant.yaml index 34303f5..fceeaa0 100755 --- a/deploy/rustfs-operator/crds/tenant.yaml +++ b/deploy/rustfs-operator/crds/tenant.yaml @@ -469,6 +469,8 @@ spec: StatefulSet controller can recreate it elsewhere. Force deletion requires the Node object to be deleted or marked with an effective `node.kubernetes.io/out-of-service` taint that the target Pod does not tolerate. + Before using force policies, operators must confirm the node is powered off or + otherwise isolated; deleting the Node object is treated as that operational assertion. Values: DoNothing | Delete | ForceDelete | DeleteStatefulSetPod | DeleteDeploymentPod | DeleteBothStatefulSetAndDeploymentPod enum: diff --git a/docs/operator-user-guide.md b/docs/operator-user-guide.md index bbf1356..556464a 100644 --- a/docs/operator-user-guide.md +++ b/docs/operator-user-guide.md @@ -353,13 +353,13 @@ For a single-pool single-node single-disk Tenant, `RUSTFS_VOLUMES` is rendered a `podDeletionPolicyWhenNodeIsDown` accepts: - `DoNothing`: do not delete pods automatically. -- `Delete`: request a normal pod delete. +- `Delete`: request a best-effort normal pod delete; this does not force-release a StatefulSet identity when the kubelet is unreachable. - `ForceDelete`: force delete the pod with `gracePeriodSeconds=0`. - `DeleteStatefulSetPod`: Longhorn-compatible force delete for StatefulSet pods stuck on down nodes. - `DeleteDeploymentPod`: Longhorn-compatible force delete for Deployment pods stuck on down nodes. - `DeleteBothStatefulSetAndDeploymentPod`: Longhorn-compatible force delete for both StatefulSet and Deployment pods. -Force deletion can have data consistency implications. Use it only when the storage backend and operational procedure are designed for that failure mode. Force deletion requires the Node object to be deleted or marked with an effective `node.kubernetes.io/out-of-service` taint that the target Pod does not tolerate, so volume detach fencing is explicit. +Force deletion can have data consistency implications. Use it only when the storage backend and operational procedure are designed for that failure mode. Force deletion requires the Node object to be deleted or marked with an effective `node.kubernetes.io/out-of-service` taint that the target Pod does not tolerate, so volume detach fencing is explicit. Before using force policies, confirm the node is powered off or otherwise isolated; deleting the Node object is treated as that operational assertion. ### 7.5 TLS diff --git a/docs/operator-user-guide.zh-CN.md b/docs/operator-user-guide.zh-CN.md index 279d36e..3328d9f 100644 --- a/docs/operator-user-guide.zh-CN.md +++ b/docs/operator-user-guide.zh-CN.md @@ -355,13 +355,13 @@ Operator 会自动管理以下环境变量： `podDeletionPolicyWhenNodeIsDown` 支持以下值： - `DoNothing`：不自动删除 Pod。 -- `Delete`：发起普通 Pod 删除。 +- `Delete`：发起 best-effort 普通 Pod 删除；当 kubelet 不可达时，它不会强制释放 StatefulSet identity。 - `ForceDelete`：使用 `gracePeriodSeconds=0` 强制删除 Pod。 - `DeleteStatefulSetPod`：Longhorn 兼容模式，强制删除 down node 上卡住的 StatefulSet Pod。 - `DeleteDeploymentPod`：Longhorn 兼容模式，强制删除 down node 上卡住的 Deployment Pod。 - `DeleteBothStatefulSetAndDeploymentPod`：Longhorn 兼容模式，同时处理 StatefulSet 和 Deployment Pod。 -强制删除可能影响数据一致性。只有当存储后端和运维流程明确支持该故障处理方式时才应启用。强制删除要求 Node 对象已删除，或 Node 带有对目标 Pod 生效且未被该 Pod tolerate 的 `node.kubernetes.io/out-of-service` taint，确保 volume detach fencing 是显式的。 +强制删除可能影响数据一致性。只有当存储后端和运维流程明确支持该故障处理方式时才应启用。强制删除要求 Node 对象已删除，或 Node 带有对目标 Pod 生效且未被该 Pod tolerate 的 `node.kubernetes.io/out-of-service` taint，确保 volume detach fencing 是显式的。启用 force 类策略前，必须先确认节点已经关机或被隔离；删除 Node 对象会被视为这一运维断言。 ### 7.5 TLS diff --git a/src/lib.rs b/src/lib.rs index fa402de..1b4bde1 100755 --- a/src/lib.rs +++ b/src/lib.rs @@ -200,9 +200,10 @@ async fn run_controller(client: Client, cancel: CancellationToken) { Api::::all(client.clone()), watcher::Config::default(), ) - .owns( + .watches( Api::::all(client.clone()), watcher::Config::default(), + tenant_refs_for_pod, ) .owns( Api::::all(client.clone()), @@ -573,6 +574,14 @@ fn tenant_refs_for_config_map(config_map: corev1::ConfigMap) -> Vec Vec> { + tenant_refs_from_metadata( + pod.metadata.namespace.as_deref(), + pod.metadata.owner_references.as_deref(), + pod.metadata.labels.as_ref(), + ) +} + fn tenant_refs_for_cert_manager_certificate(certificate: DynamicObject) -> Vec> { tenant_refs_from_metadata( certificate.metadata.namespace.as_deref(), @@ -753,6 +762,34 @@ mod controller_watch_tests { assert_single_ref(&refs, "tenant-policy-label", "storage"); } + #[test] + fn pod_mapper_uses_rustfs_tenant_label_for_statefulset_pods() { + let pod = corev1::Pod { + metadata: metav1::ObjectMeta { + name: Some("tenant-a-pool-0-0".to_string()), + namespace: Some("storage".to_string()), + owner_references: Some(vec![metav1::OwnerReference { + api_version: "apps/v1".to_string(), + kind: "StatefulSet".to_string(), + name: "tenant-a-pool-0".to_string(), + uid: "statefulset-uid".to_string(), + controller: Some(true), + ..Default::default() + }]), + labels: Some(BTreeMap::from([( + "rustfs.tenant".to_string(), + "tenant-a".to_string(), + )])), + ..Default::default() + }, + ..Default::default() + }; + + let refs = tenant_refs_for_pod(pod); + + assert_single_ref(&refs, "tenant-a", "storage"); + } + #[test] fn cert_manager_certificate_mapper_uses_owner_reference_or_label() { let resource = cert_manager_certificate_api_resource(); diff --git a/src/reconcile.rs b/src/reconcile.rs index 9463774..106e133 100755 --- a/src/reconcile.rs +++ b/src/reconcile.rs @@ -380,6 +380,7 @@ fn replicaset_matches_pod_controller_and_tenant( replicaset: &appsv1::ReplicaSet, owning_deployment: Option<&appsv1::Deployment>, tenant: &Tenant, + require_deployment_owner: bool, ) -> bool { let Some((_, replicaset_uid)) = pod_controller_owner_name_and_uid(pod, "ReplicaSet") else { return false; @@ -387,7 +388,7 @@ fn replicaset_matches_pod_controller_and_tenant( if replicaset.metadata.uid.as_deref() != Some(replicaset_uid.as_str()) { return false; } - if object_owned_by_tenant(&replicaset.metadata, tenant) { + if !require_deployment_owner && object_owned_by_tenant(&replicaset.metadata, tenant) { return true; } @@ -404,6 +405,17 @@ fn replicaset_matches_pod_controller_and_tenant( && deployment.metadata.uid.as_deref() == Some(deployment_uid.as_str()) } +fn policy_requires_deployment_owner_for_replicaset( + policy: &crate::types::v1alpha1::k8s::PodDeletionPolicyWhenNodeIsDown, +) -> bool { + use crate::types::v1alpha1::k8s::PodDeletionPolicyWhenNodeIsDown as P; + + matches!( + policy, + P::DeleteDeploymentPod | P::DeleteBothStatefulSetAndDeploymentPod + ) +} + #[derive(Clone, Copy, Debug, Eq, PartialEq)] enum NodePodDeletionSafety { Ready, @@ -518,14 +530,11 @@ async fn cleanup_stuck_terminating_pods_on_down_nodes( { let owned_by_tenant = match replicasets_api.get(&replicaset_name).await { Ok(replicaset) => { - let deployment = if object_owned_by_tenant(&replicaset.metadata, tenant) { - None - } else if let Some((deployment_name, _deployment_uid)) = + let deployment = if let Some((deployment_name, _deployment_uid)) = controller_owner_name_and_uid( replicaset.metadata.owner_references.as_ref(), "Deployment", - ) - { + ) { match deployments_api.get(&deployment_name).await { Ok(deployment) => Some(deployment), Err(kube::Error::Api(ae)) if ae.code == 404 => None, @@ -544,6 +553,7 @@ async fn cleanup_stuck_terminating_pods_on_down_nodes( &replicaset, deployment.as_ref(), tenant, + policy_requires_deployment_owner_for_replicaset(&policy), ) } Err(kube::Error::Api(ae)) if ae.code == 404 => false, @@ -598,14 +608,26 @@ async fn cleanup_stuck_terminating_pods_on_down_nodes( let deletion_plan = match cleanup_decision_for_pod(&pod, &policy, node_deletion_safety) { PodCleanupDecision::Skip => continue, PodCleanupDecision::SkipForceDeleteNeedsFencing => { + let pod_name = pod.name_any(); warn!( tenant = %tenant.name(), namespace = %namespace, node = %node_name, - pod = %pod.name_any(), + pod = %pod_name, policy = ?policy, "skipping pod force deletion because the node is not fenced" ); + let _ = ctx + .record( + tenant, + EventType::Warning, + "PodForceDeleteSkippedNodeNotFenced", + &format!( + "Pod '{}' is terminating on down node '{}', but policy {:?} requires the Node to be deleted or marked with an effective node.kubernetes.io/out-of-service taint before force deletion", + pod_name, node_name, policy + ), + ) + .await; continue; } PodCleanupDecision::Delete(plan) => plan, @@ -735,7 +757,7 @@ fn cleanup_decision_for_pod( P::Delete => PodCleanupDecision::Delete(PodDeletionPlan { force: false, precondition_uid, - event_reason: "DeletedPodOnDownNode", + event_reason: "RequestedPodDeleteOnDownNode", }), P::ForceDelete => PodCleanupDecision::Delete(PodDeletionPlan { force: true, @@ -1400,31 +1422,60 @@ mod tests { &pod, &matching_replicaset, None, - &tenant + &tenant, + false )); assert!(!replicaset_matches_pod_controller_and_tenant( &pod, &wrong_replicaset_uid, None, - &tenant + &tenant, + false )); assert!(replicaset_matches_pod_controller_and_tenant( &pod, &deployment_owned_replicaset, Some(&matching_deployment), - &tenant + &tenant, + false )); assert!(!replicaset_matches_pod_controller_and_tenant( &pod, &deployment_owned_replicaset, Some(&wrong_deployment_tenant), - &tenant + &tenant, + false )); assert!(!replicaset_matches_pod_controller_and_tenant( &pod, &deployment_owned_replicaset, None, - &tenant + &tenant, + false + )); + } + + #[test] + fn test_deployment_policy_requires_deployment_owned_replicaset() { + let tenant = crate::tests::create_test_tenant(None, None); + let pod = pod_with_owner("ReplicaSet"); + let direct_tenant_replicaset = replicaset_owned_by_test_tenant("uid", "test-uid-123"); + let deployment_owned_replicaset = replicaset_owned_by_deployment("uid", "deployment-uid"); + let matching_deployment = deployment_owned_by_test_tenant("deployment-uid", "test-uid-123"); + + assert!(!replicaset_matches_pod_controller_and_tenant( + &pod, + &direct_tenant_replicaset, + None, + &tenant, + true + )); + assert!(replicaset_matches_pod_controller_and_tenant( + &pod, + &deployment_owned_replicaset, + Some(&matching_deployment), + &tenant, + true )); } @@ -1481,7 +1532,7 @@ mod tests { }; assert!(!plan.force); assert_eq!(plan.precondition_uid.as_deref(), Some("pod-uid")); - assert_eq!(plan.event_reason, "DeletedPodOnDownNode"); + assert_eq!(plan.event_reason, "RequestedPodDeleteOnDownNode"); let delete_params = plan.delete_params(); assert_eq!(delete_params.grace_period_seconds, None); diff --git a/src/reconcile/phases.rs b/src/reconcile/phases.rs index 7c475dd..3475506 100644 --- a/src/reconcile/phases.rs +++ b/src/reconcile/phases.rs @@ -49,6 +49,7 @@ pub(super) struct PoolReconcileSummary { } const REMOVED_POOL_CLEANUP_REQUEUE_INTERVAL: Duration = Duration::from_secs(10); +const POD_DELETION_POLICY_REQUEUE_INTERVAL: Duration = Duration::from_secs(30); #[derive(Default)] pub(super) struct RemovedDecommissionedPoolCleanup { @@ -514,6 +515,41 @@ fn earliest_requeue_after(left: Option, right: Option) -> Op } } +fn pod_deletion_policy_is_enabled(tenant: &Tenant) -> bool { + tenant + .spec + .pod_deletion_policy_when_node_is_down + .as_ref() + .is_some_and(|policy| { + policy != &crate::types::v1alpha1::k8s::PodDeletionPolicyWhenNodeIsDown::DoNothing + }) +} + +fn pod_deletion_policy_requeue_after( + tenant: &Tenant, + summary: &PoolReconcileSummary, +) -> Option { + if pod_deletion_policy_is_enabled(tenant) + && summary.total_replicas > 0 + && summary.ready_replicas < summary.total_replicas + { + Some(POD_DELETION_POLICY_REQUEUE_INTERVAL) + } else { + None + } +} + +fn reconcile_requeue_after(tenant: &Tenant, summary: &PoolReconcileSummary) -> Option { + let lifecycle_requeue = summary.lifecycle_requeue_after; + let updating_requeue = summary.any_updating.then_some(Duration::from_secs(10)); + let pod_cleanup_requeue = pod_deletion_policy_requeue_after(tenant, summary); + + earliest_requeue_after( + earliest_requeue_after(lifecycle_requeue, updating_requeue), + pod_cleanup_requeue, + ) +} + fn is_not_found_context_error(error: &context::Error) -> bool { matches!( error, @@ -810,6 +846,7 @@ pub(super) async fn finalize_tenant_status( ) -> Result { let mut builder = StatusBuilder::from_tenant(tenant); let pool_count = summary.pool_statuses.len(); + let requeue_after = reconcile_requeue_after(tenant, &summary); builder.set_pool_statuses(summary.pool_statuses); if let Some(tls_status) = tls_plan.status { builder.set_tls_status(tls_status); @@ -976,22 +1013,14 @@ pub(super) async fn finalize_tenant_status( ) .await?; - if let Some(requeue_after) = summary.lifecycle_requeue_after { + if let Some(requeue_after) = requeue_after { debug!( tenant = %tenant.name(), namespace = ?tenant.namespace(), seconds = requeue_after.as_secs(), - "Pool lifecycle is active, requeuing" + "tenant reconcile has active follow-up work, requeuing" ); Ok(Action::requeue(requeue_after)) - } else if summary.any_updating { - debug!( - tenant = %tenant.name(), - namespace = ?tenant.namespace(), - seconds = 10, - "Pools are updating, requeuing" - ); - Ok(Action::requeue(Duration::from_secs(10))) } else { Ok(Action::await_change()) } @@ -1025,4 +1054,63 @@ mod tests { Some(Duration::from_secs(10)) ); } + + #[test] + fn pod_deletion_policy_requeues_not_ready_tenant() { + let mut tenant = crate::tests::create_test_tenant(None, None); + tenant.spec.pod_deletion_policy_when_node_is_down = + Some(crate::types::v1alpha1::k8s::PodDeletionPolicyWhenNodeIsDown::ForceDelete); + let summary = PoolReconcileSummary { + total_replicas: 4, + ready_replicas: 3, + ..Default::default() + }; + + assert_eq!( + pod_deletion_policy_requeue_after(&tenant, &summary), + Some(POD_DELETION_POLICY_REQUEUE_INTERVAL) + ); + } + + #[test] + fn pod_deletion_policy_requeue_skips_ready_or_disabled_tenant() { + let mut enabled = crate::tests::create_test_tenant(None, None); + enabled.spec.pod_deletion_policy_when_node_is_down = + Some(crate::types::v1alpha1::k8s::PodDeletionPolicyWhenNodeIsDown::ForceDelete); + let ready = PoolReconcileSummary { + total_replicas: 4, + ready_replicas: 4, + ..Default::default() + }; + assert_eq!(pod_deletion_policy_requeue_after(&enabled, &ready), None); + + let disabled = crate::tests::create_test_tenant(None, None); + let not_ready = PoolReconcileSummary { + total_replicas: 4, + ready_replicas: 3, + ..Default::default() + }; + assert_eq!( + pod_deletion_policy_requeue_after(&disabled, ¬_ready), + None + ); + } + + #[test] + fn reconcile_requeue_after_prefers_existing_shorter_work() { + let mut tenant = crate::tests::create_test_tenant(None, None); + tenant.spec.pod_deletion_policy_when_node_is_down = + Some(crate::types::v1alpha1::k8s::PodDeletionPolicyWhenNodeIsDown::ForceDelete); + let summary = PoolReconcileSummary { + any_updating: true, + total_replicas: 4, + ready_replicas: 3, + ..Default::default() + }; + + assert_eq!( + reconcile_requeue_after(&tenant, &summary), + Some(Duration::from_secs(10)) + ); + } } diff --git a/src/types/v1alpha1/k8s.rs b/src/types/v1alpha1/k8s.rs index 5411beb..c832a83 100755 --- a/src/types/v1alpha1/k8s.rs +++ b/src/types/v1alpha1/k8s.rs @@ -67,7 +67,9 @@ pub enum ImagePullPolicy { /// WARNING: Force-deleting pods can have data consistency implications depending on /// your storage backend and workload semantics. Force deletion requires the Node object /// to be deleted or marked with an effective `node.kubernetes.io/out-of-service` taint -/// that the target Pod does not tolerate. +/// that the target Pod does not tolerate. Before using force policies, operators must +/// confirm the node is powered off or otherwise isolated; deleting the Node object is +/// treated as that operational assertion. #[derive(Default, Deserialize, Serialize, Clone, Debug, Display, PartialEq, Eq)] #[serde(rename_all = "PascalCase")] pub enum PodDeletionPolicyWhenNodeIsDown { @@ -76,7 +78,8 @@ pub enum PodDeletionPolicyWhenNodeIsDown { #[default] DoNothing, - /// Request a normal delete for the pod. + /// Request a best-effort normal delete for the pod. This does not force-release a + /// StatefulSet identity when the kubelet is unreachable. #[strum(to_string = "Delete")] Delete, @@ -123,7 +126,7 @@ impl JsonSchema for PodDeletionPolicyWhenNodeIsDown { "DeleteDeploymentPod", "DeleteBothStatefulSetAndDeploymentPod" ], - "description": "Pod deletion policy when the node hosting the Pod is down (NotReady/Unknown). Force deletion requires the Node object to be deleted or marked with an effective node.kubernetes.io/out-of-service taint that the target Pod does not tolerate. Values: DoNothing | Delete | ForceDelete | DeleteStatefulSetPod | DeleteDeploymentPod | DeleteBothStatefulSetAndDeploymentPod" + "description": "Pod deletion policy when the node hosting the Pod is down (NotReady/Unknown). Force deletion requires the Node object to be deleted or marked with an effective node.kubernetes.io/out-of-service taint that the target Pod does not tolerate. Before using force policies, operators must confirm the node is powered off or otherwise isolated; deleting the Node object is treated as that operational assertion. Values: DoNothing | Delete | ForceDelete | DeleteStatefulSetPod | DeleteDeploymentPod | DeleteBothStatefulSetAndDeploymentPod" } } } diff --git a/src/types/v1alpha1/tenant.rs b/src/types/v1alpha1/tenant.rs index 5c79562..dd6f1e3 100755 --- a/src/types/v1alpha1/tenant.rs +++ b/src/types/v1alpha1/tenant.rs @@ -93,6 +93,8 @@ pub struct TenantSpec { /// StatefulSet controller can recreate it elsewhere. /// Force deletion requires the Node object to be deleted or marked with an effective /// `node.kubernetes.io/out-of-service` taint that the target Pod does not tolerate. + /// Before using force policies, operators must confirm the node is powered off or + /// otherwise isolated; deleting the Node object is treated as that operational assertion. /// /// Values: DoNothing | Delete | ForceDelete | DeleteStatefulSetPod | DeleteDeploymentPod | DeleteBothStatefulSetAndDeploymentPod #[serde(default, skip_serializing_if = "Option::is_none")]