CNTRLPLANE-3381: CPO: Surface cloud resource deletion timeout as a status condition - #8426
Conversation
|
Skipping CI for Draft Pull Request. |
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
@csrwng: This pull request references CNTRLPLANE-3381 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
Bot
added
do-not-merge/needs-area
area/api
When the CPO decides to give up on deleting cloud resources after a timeout, it emits a log but otherwise does not expose this behavior through conditions, making it hard to know that this happened. Set the CloudResourcesDestroyed condition to False with reason CloudResourcesDeletionTimedOut when the deletion timeout is exceeded, including a descriptive message with the elapsed time and last known status. Also handle the new reason in the cleanup-skipped check so the CPO does not re-enter cleanup after a timeout. Signed-off-by: Cesar Wong <cewong@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
b580a3d to
299e6af
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughA new exported reason constant Sequence Diagram(s)sequenceDiagram
actor Reconciler as Reconciler
participant HCP as HostedControlPlane.Status
participant API as API_Server_Status
participant CVO as cluster-version-operator_Deployment
Reconciler->>HCP: Read CloudResourcesDestroyed condition
alt Condition true
Reconciler-->>Reconciler: return done
else Condition false with Reason CleanupSkipped or DeletionTimedOut
Reconciler-->>Reconciler: log terminal reason/message
Reconciler-->>Reconciler: return done
else Not terminal
Reconciler->>CVO: Get Deployment
alt Deployment exists with replicas > 0
Reconciler->>CVO: Scale down Deployment (set replicas=0)
Reconciler-->>Reconciler: return not-done
else CVO absent or scaled down
Reconciler-->>Reconciler: check timeElapsed > resourceDeletionTimeout
alt timed out
Reconciler->>HCP: Set CloudResourcesDestroyed(Status=False, Reason=CloudResourcesDeletionTimedOutReason, message)
Reconciler->>API: Status().Update(HCP)
Reconciler-->>Reconciler: continue cleanup
else not timed out
Reconciler-->>Reconciler: return not-done
end
end
end
🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go (1)
2422-2426: ⚡ Quick winClarify terminal cleanup logging for skipped vs timed-out states.
On Line 2426, the log always says cleanup was skipped and logs
Messageas"reason", which is misleading when the terminal state is a timeout.Proposed diff
- log.Info("Cleanup has been skipped", "reason", resourcesDestroyedCond.Message) + log.Info("Cloud resource cleanup reached terminal state", + "reason", resourcesDestroyedCond.Reason, + "message", resourcesDestroyedCond.Message)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go` around lines 2422 - 2426, The log incorrectly always states "Cleanup has been skipped" for both skipped and timed-out terminal states; update the block that checks resourcesDestroyedCond (and the constants hyperv1.CloudResourcesCleanupSkippedReason / hyperv1.CloudResourcesDeletionTimedOutReason) to branch the log message based on resourcesDestroyedCond.Reason and use both resourcesDestroyedCond.Reason and resourcesDestroyedCond.Message in structured fields passed to log.Info (e.g., log.Info("Cleanup has been skipped", "reason", resourcesDestroyedCond.Reason, "message", resourcesDestroyedCond.Message) vs log.Info("Cleanup has timed out", ...)) so the output clearly distinguishes skipped vs timed-out outcomes while keeping the same conditional.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go`:
- Around line 2422-2426: The log incorrectly always states "Cleanup has been
skipped" for both skipped and timed-out terminal states; update the block that
checks resourcesDestroyedCond (and the constants
hyperv1.CloudResourcesCleanupSkippedReason /
hyperv1.CloudResourcesDeletionTimedOutReason) to branch the log message based on
resourcesDestroyedCond.Reason and use both resourcesDestroyedCond.Reason and
resourcesDestroyedCond.Message in structured fields passed to log.Info (e.g.,
log.Info("Cleanup has been skipped", "reason", resourcesDestroyedCond.Reason,
"message", resourcesDestroyedCond.Message) vs log.Info("Cleanup has timed out",
...)) so the output clearly distinguishes skipped vs timed-out outcomes while
keeping the same conditional.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 0f991213-3892-4857-85c3-9b1b2f12fcce
⛔ Files ignored due to path filters (1)
vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_conditions.gois excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (2)
api/hypershift/v1beta1/hostedcluster_conditions.gocontrol-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8426 +/- ##
==========================================
+ Coverage 37.39% 37.47% +0.07%
==========================================
Files 751 751
Lines 91806 91865 +59
==========================================
+ Hits 34333 34425 +92
+ Misses 54838 54800 -38
- Partials 2635 2640 +5
... and 1 file with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Clarify the log message when cloud resource cleanup reaches a terminal state (skipped or timed out) to distinguish between the two cases by logging the actual reason and message from the condition. Add unit test coverage for the removeCloudResources method covering: - resources already destroyed - cleanup skipped - deletion timed out (subsequent reconcile) - deletion timing out (sets condition) - deletion timing out with prior status (includes last status) - deletion still in progress - CVO deployment scaling down Signed-off-by: Cesar Wong <cewong@redhat.com> Commit-Message-Assisted-by: Claude (via Claude Code)
|
@coderabbitai Good catch on the misleading log message. Applied the suggested change — the log now reads Also added unit test coverage for |
|
Tip For best results, initiate chat on the files or code changes.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go (1)
2437-2459:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't use
LastTransitionTimeas the cleanup heartbeat.
CloudResourcesDestroyedcan remainFalsewhile its reason/message changes during deletion progress (e.g., "Deleting load balancers"). In that case,meta.SetStatusConditionpreservesLastTransitionTime, so this branch measures from the firstFalsetransition, not from the latest progress update. That can trigger the timeout even while cleanup is actively advancing.Suggested direction
- var timeElapsed time.Duration - if resourcesDestroyedCond != nil { - timeElapsed = time.Since(resourcesDestroyedCond.LastTransitionTime.Time) - } else { - timeElapsed = time.Since(cvoScaledDownCond.LastTransitionTime.Time) - } + // LastTransitionTime only changes when Condition.Status flips. + // If timeout should be based on cleanup start, use CVOScaledDown. + // If timeout should be based on progress heartbeats, persist a dedicated timestamp. + timeElapsed := time.Since(cvoScaledDownCond.LastTransitionTime.Time)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go` around lines 2437 - 2459, The timeout check is using LastTransitionTime (resourcesDestroyedCond.LastTransitionTime) which doesn't change when the condition message/reason is updated, so replace that heartbeat check with a timestamp that reflects the most recent condition update (e.g., use the condition's LastUpdateTime/LastHeartbeatTime if available or the most-recent-of LastTransitionTime and LastUpdateTime) when computing timeElapsed for resourcesDestroyedCond (and similarly for cvoScaledDownCond); ensure you read the newer timestamp field instead of LastTransitionTime before calling meta.SetStatusCondition for CloudResourcesDestroyed so progress updates reset the timeout.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.go`:
- Around line 2834-2858: The test is reading the post-call condition instead of
the original one and never verifies scaling; capture the original prevCond from
tc.hcp.Status.Conditions before calling removeCloudResources (so prevCond
reflects the pre-timeout "InProgress" state) and use that saved value when
asserting the "last status:" message; additionally, for the "should scale it
down" case reload the cluster-version-operator Deployment from the test client
after removeCloudResources and assert its spec.replicas is 0 to validate the
scaling behavior; reference symbols: removeCloudResources, tc.hcp, prevCond, and
the cluster-version-operator Deployment in your changes.
---
Outside diff comments:
In
`@control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go`:
- Around line 2437-2459: The timeout check is using LastTransitionTime
(resourcesDestroyedCond.LastTransitionTime) which doesn't change when the
condition message/reason is updated, so replace that heartbeat check with a
timestamp that reflects the most recent condition update (e.g., use the
condition's LastUpdateTime/LastHeartbeatTime if available or the most-recent-of
LastTransitionTime and LastUpdateTime) when computing timeElapsed for
resourcesDestroyedCond (and similarly for cvoScaledDownCond); ensure you read
the newer timestamp field instead of LastTransitionTime before calling
meta.SetStatusCondition for CloudResourcesDestroyed so progress updates reset
the timeout.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 10a1f9f9-d361-4f70-af4b-13e5a295c15f
📒 Files selected for processing (2)
control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.gocontrol-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.go
The test was reading the CloudResourcesDestroyed condition after removeCloudResources mutated it in place, so the "last status:" assertions were never actually reached. Capture the original condition before the call to properly validate the carry-forward message. Also add assertion that CVO deployment replicas are set to 0 after scaling. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Addressed the CodeRabbit feedback in 16f63ce: Test assertion bug (inline comment): Fixed. The test was reading Log message nitpick: Already addressed in db26558 — the log now says
|
jparrill
left a comment
jparrill
left a comment
There was a problem hiding this comment.
LGTM — clean implementation. The new CloudResourcesDeletionTimedOutReason condition surfaces the timeout state properly, and the terminal state check now handles both Skipped and TimedOut correctly. Tests are thorough with good coverage of all paths.
Minor nit: timeout tests use hardcoded -15 * time.Minute which assumes resourceDeletionTimeout = 10m — if that constant changes, the tests would silently pass/fail incorrectly. Not blocking.
|
/lgtm |
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: csrwng, jparrill The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Test Resultse2e-aws
e2e-aks
|
|
/verified by @csrwng
|
openshift-ci-robot
added
the
verified
|
@csrwng: This PR has been marked as verified by DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
@csrwng: all tests passed! Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
5 participants
Summary
CloudResourcesDeletionTimedOutReasonconstant to the APICloudResourcesDestroyedcondition toFalsewith reasonCloudResourcesDeletionTimedOutwhen the CPO deletion timeout is exceeded, including elapsed time and last known status in the messageContext
When the CPO gives up on deleting cloud resources after a timeout, it only emits a log message. This behavior is not exposed through any status condition, making it invisible to users and to the hypershift-operator unless someone is actively tailing CPO logs. This change surfaces the timeout as a first-class condition for automated monitoring and OCM integration.
Identified during a debugging session investigating cluster deletion issues in the managed Azure service (ARO HCP).
Jira
https://issues.redhat.com/browse/CNTRLPLANE-3381
Test plan
CloudResourcesDestroyedcondition is set toFalsewith reasonCloudResourcesDeletionTimedOutwhen deletion times outmake testpassesmake buildpassesSummary by CodeRabbit