CNTRLPLANE-3375: bugfix: make switched controller clear status conditions on delegate controller shutdown#955
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
/payload-aggregate periodic-ci-openshift-cluster-authentication-operator-release-5.0-periodics-e2e-azure-external-oidc-upstream-parity 10 |
|
@everettraven: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/9c54e180-86bf-11f1-9d5b-1744dbf73ad8-0 |
WalkthroughThe switched controller now optionally clears delegate-owned degraded conditions through Server-Side Apply when switching off. The OAuth clients controller enables this behavior, and tests verify the field manager and empty conditions configuration. ChangesDelegate condition cleanup
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant SwitchedController
participant DelegateController
participant OperatorClient
SwitchedController->>DelegateController: cancel delegate context
SwitchedController->>OperatorClient: ApplyOperatorStatus with empty OperatorStatus
OperatorClient-->>SwitchedController: return apply result
Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
pkg/controllers/switchedcontroller/switched_controller.go (1)
100-119: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftClearing delegate conditions can race with an in-flight delegate sync, and a failed clear is never retried.
Two related gaps in this shutdown path:
c.switchContextCancel()only requests cancellation; the delegate goroutine started withgo ...Run(c.switchContext, 1)(line 94) is not waited on. If the delegate is mid-sync (or picks up one more queued item before observingctx.Done()), it can call its ownWithSyncDegradedOnErrorand re-set the degraded condition after this clear runs, silently reintroducing the stale condition this PR is meant to fix.- Once this case executes,
c.switchContextis set tonilregardless of whetherApplyOperatorStatusbelow succeeds. On the next sync,noContextistrue, which routes to the no-op branch (Line 100) — so a failed apply (e.g., transient API error) is never retried; the stale degraded condition can persist indefinitely until the delegate is switched back on.Consider tracking delegate-goroutine completion (e.g. a done channel/
sync.WaitGroup) and only issuing the clear once the delegate has actually stopped, plus tracking a "conditions cleared" flag so a failed apply can be retried on a subsequent sync (rather than being a one-shot transition).🤖 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 `@pkg/controllers/switchedcontroller/switched_controller.go` around lines 100 - 119, Update the switched-controller shutdown flow around switchContextCancel and the delegate goroutine running Run to track delegate completion, waiting for it to stop before clearing conditions. Track whether delegate conditions were successfully cleared, retain the shutdown state when ApplyOperatorStatus fails, and retry the clear on subsequent syncs until it succeeds; only transition to the no-op state after confirmed cleanup.
🤖 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 `@pkg/controllers/switchedcontroller/switched_controller_test.go`:
- Around line 493-497: Update the applyStatusCalls assertion in the switched
controller test to require exactly one call, rejecting both zero and multiple
calls before indexing the slice. Preserve the existing failure message context
and only access applyStatusCalls[0] after this validation.
---
Outside diff comments:
In `@pkg/controllers/switchedcontroller/switched_controller.go`:
- Around line 100-119: Update the switched-controller shutdown flow around
switchContextCancel and the delegate goroutine running Run to track delegate
completion, waiting for it to stop before clearing conditions. Track whether
delegate conditions were successfully cleared, retain the shutdown state when
ApplyOperatorStatus fails, and retry the clear on subsequent syncs until it
succeeds; only transition to the no-op state after confirmed cleanup.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: dccd4add-b168-42ae-a054-1a953eea1f11
📒 Files selected for processing (2)
pkg/controllers/switchedcontroller/switched_controller.gopkg/controllers/switchedcontroller/switched_controller_test.go
|
/lgtm |
|
Scheduling required tests: |
…controller shutdown Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
13bf582 to
03fc95d
Compare
|
New changes are detected. LGTM label has been removed. |
|
@everettraven: The following test failed, say
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. |
everettraven
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
openshift-ci-robot
added
the
jira/valid-reference
|
@everettraven: This pull request references CNTRLPLANE-3375 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. |
|
/payload-aggregate periodic-ci-openshift-cluster-authentication-operator-release-5.0-periodics-e2e-azure-external-oidc-upstream-parity 10 |
|
@everettraven: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/e0b25f80-876c-11f1-99da-85f49dc1b7d9-0 |
3 participants
Summary by CodeRabbit