diff --git a/.claude/CHANGELOG.md b/.claude/CHANGELOG.md index 4a2aeab..8977a2f 100644 --- a/.claude/CHANGELOG.md +++ b/.claude/CHANGELOG.md @@ -9,6 +9,76 @@ The format is based on the regulated environment requirements: --- +## [2026-04-25 18:00] - Phase 2 of security audit: finalizer cleanup force-remove on PDB stall + +**Author:** Erick Bourgeois + +### Changed +- `src/reconcilers/helpers.rs` (`handle_deletion`): refactor the timeout + branch from `tokio::time::timeout(...).await.map_err(...)??` (which + silently propagated the `TimeoutError` and **prevented** the finalizer + from being removed) into an explicit 3-arm match over a new + `CleanupOutcome` enum. The default mode now force-removes the + finalizer on timeout so namespace deletion is unblocked, surfaces a + `FinalizerCleanupTimedOut` Warning event on the SM, and increments + `fivespot_finalizer_cleanup_timeouts_total`. A real cleanup error (as + opposed to a timeout) still propagates and is retried — the finalizer + is kept in place so a transient API failure does not orphan + resources. +- `src/reconcilers/helpers.rs`: new pure helpers `run_cleanup_with_timeout` + (returns `CleanupOutcome::{Completed, Failed(err), TimedOut}`) and + `build_finalizer_timeout_event`. Both are unit-testable without the + kube API; the timeout test runs in microseconds via + `#[tokio::test(start_paused = true)]`. +- `src/reconcilers/scheduled_machine.rs` (`Context`): new + `force_finalizer_on_timeout: bool` field defaulting to `true`, plus a + `with_force_finalizer_on_timeout(bool)` builder for `main.rs` to + override from CLI/env. Strict-cleanup mode (`false`) keeps the + finalizer and propagates `TimeoutError` so reconciliation retries — + only safe when an external sweep garbage-collects stuck SMs. +- `src/main.rs`: new `--force-finalizer-on-timeout` flag (env + `FORCE_FINALIZER_ON_TIMEOUT`, default `true`) wired through `Context`. +- `src/metrics.rs`: new `FINALIZER_CLEANUP_TIMEOUTS_TOTAL` counter + + `record_finalizer_cleanup_timeout()` helper + label-less + `fallback_counter` constructor. Documented as an operator-alert + signal for orphan-resource detection. +- `deploy/deployment/deployment.yaml`: added `FORCE_FINALIZER_ON_TIMEOUT` + env var with `value: "false"` (strict-cleanup mode) per operational + preference; the binary default remains `true`. Block-comment in the + manifest documents the trade-off and references the troubleshooting + runbook. +- `docs/src/operations/troubleshooting.md`: new "Orphan resources after + finalizer timeout" section with the runbook (find orphan Machines via + ownerRef walk, inspect bootstrap/infra refs, cascading delete via + Machine, prevention guidance). +- `src/reconcilers/helpers_tests.rs` / `src/metrics_tests.rs`: 10 new + tests covering `run_cleanup_with_timeout` (Completed/Failed/TimedOut), + `build_finalizer_timeout_event` (severity, reason, action, + note-content invariants), `Context.force_finalizer_on_timeout` + defaulting, and the metric-increments-on-call contract. + +### Why +Finding F-007 from the 2026-04-25 adversarial security audit (filed in +`~/dev/roadmaps/5spot-security-audit-2026-04-25.md`, Phase 2). A +namespace-tenant with `create pods` + `create poddisruptionbudgets` — +common tenant grants — could plant a `minAvailable: 999` PDB on a +workload they own, schedule a machine on the same node, then delete +the SM. Drain blocks indefinitely on the impossible eviction; with the +old code the finalizer was never removed, the SM was stuck with +`deletionTimestamp`, and namespace deletion stalled. The fix unblocks +namespace deletion in the default mode while preserving +strict-cleanup-or-stall semantics for operators with an external sweep. + +### Impact +- [ ] Breaking change +- [ ] Requires cluster rollout +- [x] Config change only (new env var; behaviour change for clusters + that hit the timeout path — they'll now succeed at deletion + where they previously stalled) +- [ ] Documentation only + +--- + ## [2026-04-25 06:30] - `gitleaks-install` now wires the local pre-commit hook **Author:** Erick Bourgeois diff --git a/deploy/deployment/deployment.yaml b/deploy/deployment/deployment.yaml index 355d739..e966669 100644 --- a/deploy/deployment/deployment.yaml +++ b/deploy/deployment/deployment.yaml @@ -69,6 +69,28 @@ spec: value: "10" - name: LEASE_RETRY_PERIOD_SECONDS value: "2" + # Finalizer-cleanup timeout behaviour. + # + # When "true" (binary default): on cleanup timeout the + # controller force-removes the finalizer so namespace + # deletion is unblocked. May leave orphan CAPI Machine / + # bootstrap / infra resources — operators must reconcile + # via the runbook in + # docs/src/operations/troubleshooting.md + # ("Orphan resources after finalizer timeout"). + # + # When "false" (set here): the SM stays stuck with its + # finalizer until cleanup succeeds. Namespace deletion will + # stall on a misconfigured PodDisruptionBudget — only safe + # when an external sweep is in place to garbage-collect + # stalled SMs. + # + # The fivespot_finalizer_cleanup_timeouts_total metric and + # the FinalizerCleanupTimedOut Warning event fire in BOTH + # modes; the only difference is whether the finalizer is + # removed on timeout. + - name: FORCE_FINALIZER_ON_TIMEOUT + value: "false" ports: - name: metrics containerPort: 8080 diff --git a/docs/src/operations/troubleshooting.md b/docs/src/operations/troubleshooting.md index edf18fd..b5c9ef5 100644 --- a/docs/src/operations/troubleshooting.md +++ b/docs/src/operations/troubleshooting.md @@ -202,6 +202,74 @@ kubectl logs -n 5spot-system -l app=5spot-controller -f | \ kubectl annotate scheduledmachine 5spot.finos.org/force-reconcile="$(date -u +%s)" --overwrite ``` +### Orphan resources after finalizer timeout + +**Symptom:** +- `fivespot_finalizer_cleanup_timeouts_total` increments above zero. +- A `kubectl describe scheduledmachine ` shows a Warning event with reason `FinalizerCleanupTimedOut`. +- The ScheduledMachine has been deleted (gone from `kubectl get`) but a CAPI Machine, bootstrap resource, or infrastructure resource may still exist in the namespace. + +**Root cause.** +`handle_deletion` wraps CAPI cleanup in a hard timeout +(`FINALIZER_CLEANUP_TIMEOUT_SECS`, default 600s / 10 minutes) so a hung +eviction cannot stall namespace deletion. By default +(`--force-finalizer-on-timeout=true`, env `FORCE_FINALIZER_ON_TIMEOUT=true`) +the controller force-removes its finalizer when the timeout fires — +unblocking namespace deletion at the cost of potentially leaving CAPI +resources without a managing ScheduledMachine. The most common trigger +is a **misconfigured Pod Disruption Budget** (e.g. `minAvailable: 999`) +on a workload the controller is trying to evict during node drain. + +**Runbook.** + +1. Find the orphaned CAPI Machine: + ```bash + # Machines from this namespace whose owning ScheduledMachine no longer exists. + kubectl get machines.cluster.x-k8s.io -n -o json \ + | jq -r '.items[] | select(.metadata.ownerReferences[]?.kind == "ScheduledMachine") + | .metadata.name' + for m in $(kubectl get machines.cluster.x-k8s.io -n -o name); do + owner=$(kubectl get $m -n -o jsonpath='{.metadata.ownerReferences[?(@.kind=="ScheduledMachine")].name}') + if [ -n "$owner" ] && ! kubectl get scheduledmachine "$owner" -n >/dev/null 2>&1; then + echo "ORPHAN: $m (was owned by $owner)" + fi + done + ``` + +2. Identify the bootstrap and infrastructure resources the orphan Machine references: + ```bash + kubectl get machine.cluster.x-k8s.io -n -o jsonpath='{.spec.bootstrap.configRef}' + kubectl get machine.cluster.x-k8s.io -n -o jsonpath='{.spec.infrastructureRef}' + ``` + +3. Delete the orphan Machine first; CAPI cascades into the bootstrap / infra refs via ownerReferences: + ```bash + kubectl delete machine.cluster.x-k8s.io -n + ``` + If the Machine itself is stuck terminating (drain still blocked), + inspect Pods + PDBs on the underlying Node and remove the offending + PDB before retrying. + +4. Verify nothing is left behind: + ```bash + kubectl get machines.cluster.x-k8s.io,k0sworkerconfigs.k0smotron.io,remotemachines.k0smotron.io -n + ``` + +**Prevention.** + +- Alert on `rate(fivespot_finalizer_cleanup_timeouts_total[5m]) > 0`. +- Validate Pod Disruption Budgets at admission (CEL + `ValidatingAdmissionPolicy`) — reject `minAvailable` values that exceed + the workload's replica count. +- For environments where stalled SMs are preferable to potential + orphans (e.g. a sweep job is in place), set + `--force-finalizer-on-timeout=false` (env + `FORCE_FINALIZER_ON_TIMEOUT=false`). The metric and Warning event + fire in both modes; the only difference is whether the finalizer is + removed on timeout. **Strict mode requires an external sweep** to + garbage-collect SMs whose drain is permanently blocked, otherwise + namespace deletion stalls indefinitely. + ## Emergency Reclaim (Kill Switch) See [Emergency Reclaim](../concepts/emergency-reclaim.md) for the full lifecycle. diff --git a/src/main.rs b/src/main.rs index 934dd7e..f6e537f 100644 --- a/src/main.rs +++ b/src/main.rs @@ -104,6 +104,15 @@ struct Cli { /// Retry period in seconds — documented for ops parity; not a direct `LeaseManager` parameter #[clap(long, env = "LEASE_RETRY_PERIOD_SECONDS", default_value_t = DEFAULT_LEASE_RETRY_PERIOD_SECS)] _lease_retry_period_secs: u64, + + /// On finalizer-cleanup timeout, force-remove the finalizer so namespace + /// deletion is unblocked (default). Set to `false` for strict-cleanup + /// mode where the SM stays stuck until cleanup succeeds — operators + /// then need an external sweep to garbage-collect stalled SMs. The + /// `fivespot_finalizer_cleanup_timeouts_total` metric and the + /// `FinalizerCleanupTimedOut` Warning event fire in both modes. + #[clap(long, env = "FORCE_FINALIZER_ON_TIMEOUT", default_value_t = true)] + force_finalizer_on_timeout: bool, } /// Async entry point. @@ -167,11 +176,10 @@ async fn main() -> Result<()> { health_state.set_k8s_connected(true); // Create shared context - let context = Arc::new(Context::new( - client.clone(), - cli.instance_id, - cli.instance_count, - )); + let context = Arc::new( + Context::new(client.clone(), cli.instance_id, cli.instance_count) + .with_force_finalizer_on_timeout(cli.force_finalizer_on_timeout), + ); // Leader election (Basel III HA — P2-4) // diff --git a/src/metrics.rs b/src/metrics.rs index 16c437e..4ccb28c 100644 --- a/src/metrics.rs +++ b/src/metrics.rs @@ -30,8 +30,8 @@ use std::sync::LazyLock; use prometheus::{ - register_counter_vec, register_gauge, register_gauge_vec, register_histogram_vec, CounterVec, - Gauge, GaugeVec, HistogramVec, Opts, + register_counter, register_counter_vec, register_gauge, register_gauge_vec, + register_histogram_vec, Counter, CounterVec, Gauge, GaugeVec, HistogramVec, Opts, }; // ============================================================================ @@ -69,6 +69,13 @@ fn fallback_gauge(name: &str, help: &str) -> Gauge { .unwrap_or_else(|e| panic!("{FALLBACK_METRIC_BUG_MSG}: name={name:?} err={e}")) } +/// Create an *unregistered* `Counter` (label-less) used as a no-op fallback +/// when `register_counter!` fails. +fn fallback_counter(name: &str, help: &str) -> Counter { + Counter::new(name, help) + .unwrap_or_else(|e| panic!("{FALLBACK_METRIC_BUG_MSG}: name={name:?} err={e}")) +} + /// Create an *unregistered* `GaugeVec` used as a no-op fallback when /// `register_gauge_vec!` fails. fn fallback_gauge_vec(name: &str, help: &str, labels: &[&str]) -> GaugeVec { @@ -280,6 +287,33 @@ pub static POD_EVICTIONS_TOTAL: LazyLock = LazyLock::new(|| { }) }); +/// Finalizer cleanup timeouts during deletion handling. +/// +/// Incremented every time `handle_deletion` exceeds +/// [`crate::constants::FINALIZER_CLEANUP_TIMEOUT_SECS`] while removing a +/// machine from its cluster. A non-zero value typically indicates a +/// misconfigured Pod Disruption Budget on a workload that the controller +/// is trying to evict — the controller force-removes the finalizer to +/// unblock namespace deletion, but this metric tells operators an +/// orphaned CAPI Machine + bootstrap/infrastructure resources may need +/// manual cleanup. +/// +/// Alert when the rate is non-zero. See +/// `docs/src/operations/troubleshooting.md` for the orphan-cleanup runbook. +pub static FINALIZER_CLEANUP_TIMEOUTS_TOTAL: LazyLock = LazyLock::new(|| { + register_counter!( + "fivespot_finalizer_cleanup_timeouts_total", + "Total number of finalizer cleanup timeouts (force-removed; possible orphan resources)" + ) + .unwrap_or_else(|e| { + eprintln!("WARN: Failed to register fivespot_finalizer_cleanup_timeouts_total: {e}"); + fallback_counter( + "fivespot_finalizer_cleanup_timeouts_total", + "Total number of finalizer cleanup timeouts (force-removed; possible orphan resources)", + ) + }) +}); + /// Record a successful reconciliation pub fn record_reconciliation_success(phase: &str, duration_secs: f64) { RECONCILIATIONS_TOTAL @@ -330,6 +364,15 @@ pub fn record_pod_eviction(success: bool) { POD_EVICTIONS_TOTAL.with_label_values(&[result]).inc(); } +/// Record a finalizer-cleanup timeout (force-remove path). +/// +/// Operators should treat any non-zero rate as a signal that orphan CAPI +/// Machine / bootstrap / infrastructure resources may exist and need +/// manual reconciliation. +pub fn record_finalizer_cleanup_timeout() { + FINALIZER_CLEANUP_TIMEOUTS_TOTAL.inc(); +} + /// Initialize controller info metric pub fn init_controller_info(version: &str, instance_id: u32) { CONTROLLER_INFO diff --git a/src/metrics_tests.rs b/src/metrics_tests.rs index ed1260d..b263e62 100644 --- a/src/metrics_tests.rs +++ b/src/metrics_tests.rs @@ -46,6 +46,17 @@ fn test_record_pod_eviction() { record_pod_eviction(false); } +#[test] +fn test_record_finalizer_cleanup_timeout_increments_counter() { + let before = FINALIZER_CLEANUP_TIMEOUTS_TOTAL.get(); + record_finalizer_cleanup_timeout(); + let after = FINALIZER_CLEANUP_TIMEOUTS_TOTAL.get(); + assert!( + after > before, + "FINALIZER_CLEANUP_TIMEOUTS_TOTAL must increment on call: before={before} after={after}" + ); +} + #[test] fn test_init_controller_info() { init_controller_info("0.1.0", 0); diff --git a/src/reconcilers/helpers.rs b/src/reconcilers/helpers.rs index f872471..9b54f47 100644 --- a/src/reconcilers/helpers.rs +++ b/src/reconcilers/helpers.rs @@ -218,23 +218,104 @@ pub async fn add_finalizer( Ok(Action::requeue(Duration::from_secs(0))) } +/// Outcome of a timed cleanup attempt — produced by +/// [`run_cleanup_with_timeout`] and consumed by [`handle_deletion`] to +/// classify what happened to a finalizer-cleanup future. +/// +/// The variants are deliberately *not* an `enum E { Ok, Err(_) }`: the +/// caller must distinguish "cleanup itself returned an error" (retry) +/// from "cleanup did not complete in time" (force-remove the finalizer +/// to unblock namespace deletion). Folding the timeout into a generic +/// error was the source of the original bug. +#[derive(Debug)] +pub enum CleanupOutcome { + /// The cleanup future completed within the timeout with `Ok(())`. + Completed, + /// The cleanup future completed within the timeout with `Err(_)`. + /// Caller should propagate the error and retry on the next reconcile. + Failed(ReconcilerError), + /// The cleanup future did NOT complete within the timeout. Caller + /// should force-remove the finalizer to prevent stalling namespace + /// deletion and surface a Warning event so operators verify orphans. + TimedOut, +} + +/// Wrap a cleanup future in a hard timeout and classify the outcome. +/// +/// Pulled out as a standalone async function so the three branches can be +/// unit-tested in microseconds (`tokio::test(start_paused = true)`) +/// without mocking the kube API. +pub async fn run_cleanup_with_timeout(timeout_duration: Duration, cleanup: F) -> CleanupOutcome +where + F: std::future::Future>, +{ + match tokio::time::timeout(timeout_duration, cleanup).await { + Ok(Ok(())) => CleanupOutcome::Completed, + Ok(Err(e)) => CleanupOutcome::Failed(e), + Err(_) => CleanupOutcome::TimedOut, + } +} + +/// Build the `Warning` Kubernetes Event surfaced on a `ScheduledMachine` +/// whose finalizer was force-removed because cleanup exceeded +/// [`FINALIZER_CLEANUP_TIMEOUT_SECS`]. +/// +/// The note is the only operator-facing signal in `kubectl describe`, so +/// it MUST direct the operator to the orphan-cleanup runbook — the +/// controller has accepted that CAPI Machine + bootstrap/infra resources +/// may now be unowned and require manual verification. +#[must_use] +pub fn build_finalizer_timeout_event(timeout_secs: u64) -> KubeEvent { + KubeEvent { + type_: EventType::Warning, + reason: "FinalizerCleanupTimedOut".to_string(), + note: Some(format!( + "Finalizer cleanup exceeded {timeout_secs}s timeout. The finalizer was \ + force-removed to unblock namespace deletion. Operators MUST manually verify \ + that the CAPI Machine and its bootstrap/infrastructure resources have been \ + cleaned up — orphan resources are possible. See the troubleshooting docs \ + ('Orphan resources after finalizer timeout') for the runbook." + )), + action: "FinalizerCleanupTimedOut".to_string(), + secondary: None, + } +} + /// Run finalizer cleanup when a `ScheduledMachine` is being deleted. /// /// If the resource is currently in the `Active` or `ShuttingDown` phase the /// corresponding CAPI Machine (and its child resources) are removed from the -/// cluster first. The removal is wrapped in a hard +/// cluster first. The removal is wrapped in a hard /// [`FINALIZER_CLEANUP_TIMEOUT_SECS`] timeout so a hung API call cannot /// block namespace deletion or cluster upgrades indefinitely. /// -/// Once cleanup succeeds (or is skipped for non-running phases) the -/// finalizer string is removed from `metadata.finalizers`. After this patch -/// Kubernetes considers the resource fully deleted. +/// On timeout the finalizer is **force-removed** (the reverse of the +/// original behaviour, which propagated the error and stalled deletion +/// indefinitely). The trade-off: a misbehaving Pod Disruption Budget on +/// a tenant workload can no longer block namespace deletion, but the +/// CAPI Machine and bootstrap/infrastructure resources may be left as +/// orphans. A `FinalizerCleanupTimedOut` Warning event is published on +/// the SM and `fivespot_finalizer_cleanup_timeouts_total` is incremented +/// so operators can detect and reconcile orphans. +/// +/// On a non-timeout cleanup error (e.g., real CAPI delete failure) the +/// finalizer is **kept** and the error is propagated so the controller +/// retries on the next reconcile. +/// +/// Once cleanup succeeds (or is skipped for non-running phases, or is +/// force-removed on timeout) the finalizer string is removed from +/// `metadata.finalizers`. After this patch Kubernetes considers the +/// resource fully deleted. /// /// # Errors /// - [`ReconcilerError::InvalidConfig`] — resource has no namespace -/// - [`ReconcilerError::TimeoutError`] — machine removal exceeded the cleanup timeout /// - [`ReconcilerError::CapiError`] — CAPI Machine delete call failed +/// (returned without removing the finalizer; reconciler retries) /// - kube API error — finalizer patch call failed +/// +/// Note: [`ReconcilerError::TimeoutError`] is no longer returned from this +/// function — timeouts now drive the force-remove path rather than +/// propagating up. pub async fn handle_deletion( resource: Arc, ctx: Arc, @@ -264,16 +345,72 @@ pub async fn handle_deletion( "Removing machine from cluster before deletion" ); - tokio::time::timeout( + let outcome = run_cleanup_with_timeout( cleanup_timeout, remove_machine_from_cluster(&resource, &ctx.client, &namespace), ) - .await - .map_err(|_| { - ReconcilerError::TimeoutError(format!( - "Finalizer cleanup timed out after {FINALIZER_CLEANUP_TIMEOUT_SECS}s for {name}" - )) - })??; + .await; + + match outcome { + CleanupOutcome::Completed => { + debug!( + resource = %name, + namespace = %namespace, + "Cleanup completed within timeout" + ); + } + CleanupOutcome::Failed(e) => { + // Real cleanup error (e.g. CAPI delete failed): keep the + // finalizer, propagate the error, and let the reconciler + // retry with exponential back-off. + return Err(e); + } + CleanupOutcome::TimedOut => { + // Always increment the metric + emit Warning event so + // operators see timeouts regardless of which mode is on. + crate::metrics::record_finalizer_cleanup_timeout(); + let object_ref = ObjectReference { + api_version: Some(API_VERSION_FULL.to_string()), + kind: Some(crate::constants::KIND_SCHEDULED_MACHINE.to_string()), + name: Some(name.clone()), + namespace: Some(namespace.clone()), + ..Default::default() + }; + let event = build_finalizer_timeout_event(FINALIZER_CLEANUP_TIMEOUT_SECS); + publish_best_effort(&ctx, &object_ref, event, "finalizer-timeout").await; + + if ctx.force_finalizer_on_timeout { + // Force-remove path (default): log and fall through + // to finalizer removal so namespace deletion can + // proceed. + warn!( + resource = %name, + namespace = %namespace, + timeout_secs = FINALIZER_CLEANUP_TIMEOUT_SECS, + "Finalizer cleanup timed out; force-removing finalizer to unblock \ + namespace deletion. Operators MUST verify CAPI Machine + \ + bootstrap/infrastructure resources have been cleaned up — \ + orphans are possible." + ); + } else { + // Strict-cleanup mode: keep the finalizer and + // propagate the timeout so the reconciler retries. + // Operators must have an external sweep to garbage- + // collect stuck SMs in this mode. + error!( + resource = %name, + namespace = %namespace, + timeout_secs = FINALIZER_CLEANUP_TIMEOUT_SECS, + "Finalizer cleanup timed out; strict-cleanup mode enabled, \ + keeping finalizer and retrying. SM deletion is BLOCKED until \ + cleanup succeeds." + ); + return Err(ReconcilerError::TimeoutError(format!( + "Finalizer cleanup timed out after {FINALIZER_CLEANUP_TIMEOUT_SECS}s for {name}" + ))); + } + } + } } } diff --git a/src/reconcilers/helpers_tests.rs b/src/reconcilers/helpers_tests.rs index 1f65faf..2b39032 100644 --- a/src/reconcilers/helpers_tests.rs +++ b/src/reconcilers/helpers_tests.rs @@ -3578,4 +3578,127 @@ mod tests { let elapsed = check_grace_period_elapsed(&sm).expect("must not error"); assert!(elapsed, "missing grace condition must return true"); } + + // ======================================================================== + // run_cleanup_with_timeout — finalizer-cleanup timeout / force-remove path + // + // A misconfigured PodDisruptionBudget can block pod evictions during + // node drain, causing finalizer cleanup to hang past + // FINALIZER_CLEANUP_TIMEOUT_SECS. Prior code propagated the timeout + // error and never removed the finalizer, stalling namespace deletion + // indefinitely. The fix extracts the timeout-and-classify pattern into + // a pure function so the caller can decide whether to force-remove. + // ======================================================================== + + use crate::reconcilers::helpers::{run_cleanup_with_timeout, CleanupOutcome}; + + #[tokio::test] + async fn test_run_cleanup_with_timeout_completed_on_ok() { + let outcome = run_cleanup_with_timeout(Duration::from_secs(5), async { + Ok::<(), ReconcilerError>(()) + }) + .await; + assert!( + matches!(outcome, CleanupOutcome::Completed), + "Ok future must produce Completed, got {outcome:?}" + ); + } + + #[tokio::test] + async fn test_run_cleanup_with_timeout_failed_on_inner_error() { + let outcome = run_cleanup_with_timeout(Duration::from_secs(5), async { + Err::<(), ReconcilerError>(ReconcilerError::CapiError("boom".to_string())) + }) + .await; + match outcome { + CleanupOutcome::Failed(e) => { + assert!(e.to_string().contains("boom"), "must preserve inner err"); + } + other => panic!("Err future must produce Failed, got {other:?}"), + } + } + + #[tokio::test(start_paused = true)] + async fn test_run_cleanup_with_timeout_timed_out_when_future_hangs() { + // tokio's paused-time runtime advances the clock automatically when + // every task is blocked on a timer, so this test runs in microseconds. + let outcome = run_cleanup_with_timeout(Duration::from_secs(60), async { + tokio::time::sleep(Duration::from_secs(3600)).await; + Ok::<(), ReconcilerError>(()) + }) + .await; + assert!( + matches!(outcome, CleanupOutcome::TimedOut), + "hanging future must produce TimedOut, got {outcome:?}" + ); + } + + // ======================================================================== + // build_finalizer_timeout_event — Warning event surfaced on the SM after + // the controller force-removes the finalizer. The note must steer + // operators at the orphan-cleanup runbook. + // ======================================================================== + + use crate::reconcilers::helpers::build_finalizer_timeout_event; + + #[test] + fn test_finalizer_timeout_event_is_warning() { + let ev = build_finalizer_timeout_event(600); + assert_eq!(ev.type_, EventType::Warning); + } + + #[test] + fn test_finalizer_timeout_event_reason_and_action() { + let ev = build_finalizer_timeout_event(600); + assert_eq!(ev.reason, "FinalizerCleanupTimedOut"); + assert_eq!(ev.action, "FinalizerCleanupTimedOut"); + } + + #[test] + fn test_finalizer_timeout_event_note_contains_timeout_value() { + let ev = build_finalizer_timeout_event(600); + let note = ev.note.expect("note required for operator-facing event"); + assert!( + note.contains("600"), + "note must include the timeout-seconds value, got: {note}" + ); + } + + #[test] + fn test_finalizer_timeout_event_note_directs_to_orphan_cleanup() { + // The note is the only signal an operator gets in `kubectl describe` + // — it MUST mention orphan-resource cleanup so they know what to do. + let ev = build_finalizer_timeout_event(600); + let note = ev.note.expect("note required"); + let lc = note.to_lowercase(); + assert!( + lc.contains("orphan") || lc.contains("manual") || lc.contains("verify"), + "note must direct operator to orphan-cleanup runbook, got: {note}" + ); + } + + // ======================================================================== + // Context.force_finalizer_on_timeout — flag plumbing + // ======================================================================== + + #[tokio::test] + async fn test_context_default_force_finalizer_on_timeout_is_true() { + let (client, _h) = mock_client_pair(); + let ctx = make_test_context(client); + assert!( + ctx.force_finalizer_on_timeout, + "Context::new must default force_finalizer_on_timeout=true so existing \ + single-instance deployments unblock namespace deletion by default" + ); + } + + #[tokio::test] + async fn test_context_with_force_finalizer_on_timeout_false_overrides_default() { + let (client, _h) = mock_client_pair(); + let ctx = make_test_context(client).with_force_finalizer_on_timeout(false); + assert!( + !ctx.force_finalizer_on_timeout, + "with_force_finalizer_on_timeout(false) must opt into strict-cleanup mode" + ); + } } diff --git a/src/reconcilers/scheduled_machine.rs b/src/reconcilers/scheduled_machine.rs index b0e362c..3e22b9b 100644 --- a/src/reconcilers/scheduled_machine.rs +++ b/src/reconcilers/scheduled_machine.rs @@ -97,6 +97,18 @@ pub struct Context { /// after the resource recovers. Uses a `std::sync::Mutex` because /// `error_policy` is synchronous. pub retry_counts: Arc>>, + /// When `true` (default), `handle_deletion` force-removes the finalizer if + /// CAPI cleanup exceeds [`crate::constants::FINALIZER_CLEANUP_TIMEOUT_SECS`], + /// surfacing a `FinalizerCleanupTimedOut` Warning event and incrementing + /// `fivespot_finalizer_cleanup_timeouts_total`. Trade-off: namespace + /// deletion is unblocked but orphan CAPI resources are possible. + /// + /// Operators who require strict-cleanup-or-stall semantics can set + /// `--force-finalizer-on-timeout=false` (env: `FORCE_FINALIZER_ON_TIMEOUT`) + /// to revert to the original behaviour of returning `TimeoutError` and + /// keeping the finalizer in place. Use only when an external sweep is in + /// place to garbage-collect stuck SMs. + pub force_finalizer_on_timeout: bool, } impl Context { @@ -121,8 +133,19 @@ impl Context { // Default to true so existing single-instance deployments without // leader election continue to work without any configuration change. is_leader: Arc::new(AtomicBool::new(true)), + // Default-true: prefer unblocking namespace deletion over + // strict-cleanup. See field rustdoc for the trade-off. + force_finalizer_on_timeout: true, } } + + /// Override the [`Self::force_finalizer_on_timeout`] flag after + /// construction. Used by `main.rs` to wire the CLI / env var. + #[must_use] + pub fn with_force_finalizer_on_timeout(mut self, force: bool) -> Self { + self.force_finalizer_on_timeout = force; + self + } } // ============================================================================