fix: skip ThanosQuerier reconciliation during deletion to prevent foregroundDeletion deadlock - #1202
fix: skip ThanosQuerier reconciliation during deletion to prevent foregroundDeletion deadlock#1202jeffdyoung wants to merge 1 commit into
Conversation
The ThanosQuerier controller does not check DeletionTimestamp before reconciling, so it continues to update owned resources (Deployment, Service, ServiceAccount, ConfigMap) while the CR is being deleted. This races with the Kubernetes garbage collector and prevents the foregroundDeletion finalizer from clearing. The MonitoringStack controller already guards against this at monitoring-stack/controller.go:132. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jeffdyoung The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @jeffdyoung. Thanks for your PR. I'm waiting for a rhobs member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthrough
Estimated code review effort: 1 (Trivial) | ~3 minutes Merge Risk: ⚪ Minimal · up to The controller now stops reconciling resources while a ThanosQuerier is being deleted, allowing dependent resources to finish cleanup and preventing the reported termination deadlock. No actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…ing test cleanup The ThanosQuerier CR can get stuck in terminating state with a foregroundDeletion finalizer when deleted between monitoring test groups. This happens because: 1. The deploy action sets blockOwnerDeletion=true on the ThanosQuerier's ownerReference to the Monitoring CR. 2. When the operator's GC action deletes the ThanosQuerier (e.g. when metrics config is removed), Kubernetes uses foreground cascading deletion and adds a foregroundDeletion finalizer. 3. The Cluster Observability Operator's ThanosQuerier controller does not check DeletionTimestamp, so it continues to update owned resources (Deployment, Service, etc.) while the GC is trying to delete them. 4. The resulting resourceVersion conflicts prevent the GC from clearing the finalizer, leaving the CR stuck in terminating state for 600+ seconds until the test times out. Add WithRemoveFinalizersOnDelete(true) for ThanosQuerier in cleanupGroup and ValidateMonitoringServiceDisabled, matching the existing pattern for MonitoringStack, TempoStack, and TempoMonolithic which have the same external-operator finalizer issue. The upstream root cause fix has been submitted as rhobs/observability-operator#1202. This workaround is safe to keep after the upstream fix ships — it's a no-op when there are no stuck finalizers. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ing test cleanup The ThanosQuerier CR can get stuck in terminating state with a foregroundDeletion finalizer when deleted between monitoring test groups. This happens because: 1. The deploy action sets blockOwnerDeletion=true on the ThanosQuerier's ownerReference to the Monitoring CR. 2. When the operator's GC action deletes the ThanosQuerier (e.g. when metrics config is removed), Kubernetes uses foreground cascading deletion and adds a foregroundDeletion finalizer. 3. The Cluster Observability Operator's ThanosQuerier controller does not check DeletionTimestamp, so it continues to update owned resources (Deployment, Service, etc.) while the GC is trying to delete them. 4. The resulting resourceVersion conflicts prevent the GC from clearing the finalizer, leaving the CR stuck in terminating state for 600+ seconds until the test times out. Add WithRemoveFinalizersOnDelete(true) for ThanosQuerier in cleanupGroup and ValidateMonitoringServiceDisabled, matching the existing pattern for MonitoringStack, TempoStack, and TempoMonolithic which have the same external-operator finalizer issue. The upstream root cause fix has been submitted as rhobs/observability-operator#1202. This workaround is safe to keep after the upstream fix ships — it's a no-op when there are no stuck finalizers. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ing test cleanup (#4007) The ThanosQuerier CR can get stuck in terminating state with a foregroundDeletion finalizer when deleted between monitoring test groups. This happens because: 1. The deploy action sets blockOwnerDeletion=true on the ThanosQuerier's ownerReference to the Monitoring CR. 2. When the operator's GC action deletes the ThanosQuerier (e.g. when metrics config is removed), Kubernetes uses foreground cascading deletion and adds a foregroundDeletion finalizer. 3. The Cluster Observability Operator's ThanosQuerier controller does not check DeletionTimestamp, so it continues to update owned resources (Deployment, Service, etc.) while the GC is trying to delete them. 4. The resulting resourceVersion conflicts prevent the GC from clearing the finalizer, leaving the CR stuck in terminating state for 600+ seconds until the test times out. Add WithRemoveFinalizersOnDelete(true) for ThanosQuerier in cleanupGroup and ValidateMonitoringServiceDisabled, matching the existing pattern for MonitoringStack, TempoStack, and TempoMonolithic which have the same external-operator finalizer issue. The upstream root cause fix has been submitted as rhobs/observability-operator#1202. This workaround is safe to keep after the upstream fix ships — it's a no-op when there are no stuck finalizers. Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Problem
The ThanosQuerier controller enters a race condition with the Kubernetes garbage collector during CR deletion, causing ThanosQuerier resources to get stuck in terminating state for 600+ seconds.
How the deadlock occurs
A consumer (e.g. the ODH/RHOAI operator) creates ThanosQuerier CRs with an ownerReference to a parent resource (e.g. a Monitoring CR) with
blockOwnerDeletion: trueandcontroller: true.When the parent is deleted or the consumer's GC action removes the ThanosQuerier, Kubernetes initiates foreground cascading deletion: it adds a
foregroundDeletionfinalizer to the ThanosQuerier and begins deleting its dependents (the Deployment, Service, ServiceAccount, ConfigMap that the ThanosQuerier controller created viaOwns()).The ThanosQuerier controller's
Reconcilemethod has noDeletionTimestampcheck. It continues to run while the CR is being deleted — callingthanosComponentReconcilers()which updates the same Deployment, Service, etc. that the garbage collector is trying to delete.The controller's updates bump
resourceVersionon the owned resources. The GC, which was working with the previousresourceVersion, encounters a conflict and retries. On the next attempt, the controller updates again. This cycle prevents the GC from deleting all dependents, so theforegroundDeletionfinalizer is never removed.The ThanosQuerier CR is stuck in terminating state indefinitely.
Observed impact
In the opendatahub-operator E2E suite, this causes
Test_ThanosQuerier_not_deployed_without_metricsto time out after 600s waiting for the ThanosQuerier to be deleted. The test disables metrics on the Monitoring CR, the operator's GC action deletes the ThanosQuerier, and the foreground deletion deadlock prevents it from completing. This failure is deterministic and reproduces on every CI run.Failing build: pull-ci-opendatahub-io-opendatahub-operator-main-opendatahub-operator-rhoai-e2e / 2090374980800876544
Fix
Add a
DeletionTimestampcheck at the top ofReconcileto skip reconciliation when the ThanosQuerier is being deleted. This allows the garbage collector to delete owned resources without interference and clear theforegroundDeletionfinalizer.This is consistent with the MonitoringStack controller, which already has the same guard at
monitoring-stack/controller.go:132:The ThanosQuerier controller doesn't need active cleanup (unlike MonitoringStack, which has cluster-scoped resources) — it relies entirely on Kubernetes GC via
Owns()for owned Deployments, Services, ServiceAccounts, and ConfigMaps. Skipping reconciliation is sufficient.🤖 Generated with Claude Code