CNTRLPLANE-3532: migrate CPO status patches to statuspatching helpers - #8966
CNTRLPLANE-3532: migrate CPO status patches to statuspatching helpers#8966vsolanki12 wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@vsolanki12: This pull request references CNTRLPLANE-3532 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. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
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: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe controllers now use Possibly related PRs
Suggested reviewers: Mergeability Score: ⚪ Minimal · up to This PR updates status patch handling and adds consistent conflict retries; no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 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 |
openshift-ci
Bot
added
area/control-plane-operator
fb8a23c to
d73cc0d
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8966 +/- ##
==========================================
+ Coverage 45.67% 45.76% +0.08%
==========================================
Files 781 781
Lines 97726 97728 +2
==========================================
+ Hits 44641 44725 +84
+ Misses 50019 49933 -86
- Partials 3066 3070 +4
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.go`:
- Around line 1117-1139: The status patch in hostedcontrolplane_controller.go is
using a stale copy of HostedControlPlane and will overwrite earlier updates made
in update() and reconcileCPOV2. Fix the PatchStatus call to patch the current
in-memory hostedControlPlane state, or explicitly merge the existing status
fields back before setting ValidReleaseInfo. Keep the existing status mutations
such as Ready, KubeConfig, KubeadminPassword, ControlPlaneVersion, Initialized,
and prior conditions intact when applying the patch.
In
`@control-plane-operator/hostedclusterconfigoperator/controllers/reencryption/reencryption.go`:
- Around line 76-89: The `desiredCondition` in `reconcile` is a pointer into
`hcp.Status.Conditions`, so `statuspatching.PatchStatus` may re-fetch and
overwrite the backing slice before the callback uses it. Capture the condition
by value before calling `PatchStatus` (for example, copy the result of
`meta.FindStatusCondition` into a standalone variable) and then use that copied
value inside the patch callback when setting `hcp.Status.Conditions`.
🪄 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: ddb9d783-3baf-45e9-a217-5ed2a8755bb9
📒 Files selected for processing (3)
control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.gocontrol-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/reencryption/reencryption.go
d73cc0d to
7848ce8
Compare
|
Both failures are now fully analyzed. Let me produce the final report. Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryBoth failures are transient infrastructure flakes completely unrelated to the code changes in PR #8966. The Root CauseJob 1 — verify / Verify:
These are server-side errors from the Go module proxy CDN, not local network issues or code problems. Job 2 — Red Hat Konflux / control-plane-operator-main-on-pull-request: Neither failure is related to the PR's code changes (migrating CPO status patches to statuspatching helpers). Recommendations
Evidence
|
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
cblecker
left a comment
cblecker
left a comment
There was a problem hiding this comment.
The migration to statuspatching helpers looks well-executed across all call sites. F1 (metrics regression in reencryption.go) is the main concern — the rest are suggestions.
| } | ||
|
|
||
| // Record metrics when encryption status changed. | ||
| if !equality.Semantic.DeepEqual(previousEncryption, desiredEncryption) { |
There was a problem hiding this comment.
recordMigrationState is now gated behind the DeepEqual check, but the old code called it unconditionally on every reconcile. After an HCCO pod restart in steady state (no encryption change), all hypershift_encryption_migration_state gauges stay at zero indefinitely — the "idle" gauge is never re-set to 1. This could confuse dashboards/alerts until the next key rotation, which may be weeks away.
Consider moving recordMigrationState outside the if !equality.Semantic.DeepEqual(...) block so it runs unconditionally, matching the old behavior. recordMigrationDuration should stay inside the guard since it should only fire on actual transitions.
There was a problem hiding this comment.
Done. Moved recordMigrationState outside the DeepEqual guard so it runs unconditionally on every reconcile, matching the old behavior. recordMigrationDuration stays inside the guard since it should only fire on actual transitions.
AI-assisted response via Claude Code
|
|
||
| func (r *HostedControlPlaneReconciler) reconcileDeletion(ctx context.Context, hostedControlPlane *hyperv1.HostedControlPlane, originalHostedControlPlane *hyperv1.HostedControlPlane) (ctrl.Result, error) { | ||
| func (r *HostedControlPlaneReconciler) reconcileDeletion(ctx context.Context, hostedControlPlane *hyperv1.HostedControlPlane) (ctrl.Result, error) { | ||
| condition := &metav1.Condition{ |
There was a problem hiding this comment.
Nit: condition is declared as a pointer (&metav1.Condition{...}) and then dereferenced (*condition) when passed to PatchStatusCondition. The other migrated sites (reconcileDefaultSecurityGroup, removeCloudResources) use value types or inline literals. Switching to a value type here would be more consistent and avoids the unnecessary indirection.
There was a problem hiding this comment.
Done. Changed condition from *metav1.Condition to metav1.Condition — consistent with the other migrated sites now.
AI-assisted response via Claude Code
| if err := statuspatching.PatchStatus(ctx, r.Client, hcp, func() error { | ||
| meta.SetStatusCondition(&hcp.Status.Conditions, condition) | ||
| if creationErr == nil { | ||
| hcp.Status.Platform = &hyperv1.PlatformStatus{ |
There was a problem hiding this comment.
Pre-existing, but worth noting since the PatchStatus migration touches this: the callback replaces the entire hcp.Status.Platform struct with a new one containing only the security group ID. If PlatformStatus gains additional fields in the future, they'd be silently cleared on every reconcile. With PatchStatus retrying on conflict, the fresh struct also discards whatever the server has at retry time.
Consider initializing hcp.Status.Platform / hcp.Status.Platform.AWS if nil instead of replacing, then setting only DefaultWorkerSecurityGroupID.
There was a problem hiding this comment.
Done. Changed to init-if-nil pattern — hcp.Status.Platform and hcp.Status.Platform.AWS are now initialized only if nil, then only DefaultWorkerSecurityGroupID is set. This preserves any other fields that may be added to PlatformStatus in the future.
AI-assisted response via Claude Code
| Status: metav1.ConditionTrue, | ||
| Reason: hyperv1.AsExpectedReason, | ||
| Message: hyperv1.AllIsWellMessage, | ||
| ObservedGeneration: hostedControlPlane.Generation, |
There was a problem hiding this comment.
Not a regression (old code also re-fetched before referencing Generation), but ObservedGeneration inside the PatchStatus closure will reflect the re-fetched HCP's generation, which may be newer than the generation used to compute missingImages. If the spec changed between the original read and the re-fetch, the condition content won't match what that generation actually means. A follow-up reconcile self-corrects, so this is minor — just flagging in case you want to snapshot the generation before the PatchStatus call.
There was a problem hiding this comment.
Acknowledged. This is pre-existing — the old code also re-fetched before referencing Generation. A follow-up reconcile self-corrects, so leaving as-is for now.
AI-assisted response via Claude Code
7848ce8 to
4237039
Compare
| // Capture desired status changes computed by reconcile(). | ||
| // Copy by value — PatchStatus re-fetches hcp, which replaces the backing slice. | ||
| desiredEncryption := *hcp.Status.SecretEncryption.DeepCopy() | ||
| var desiredCondition metav1.Condition |
There was a problem hiding this comment.
Potential semantic narrowing: The original MergeFrom(originalHCP) patch captured all status mutations made by reconcile(). This new code snapshots only SecretEncryption and the EtcdDataEncryptionUpToDate condition, then replays just those two fields inside the PatchStatus closure.
If reconcile() (or any of its sub-functions like handleInitialBootstrap, startNewRotation, handleMigratingPhase, etc.) sets other status fields or conditions beyond these two, those changes are silently dropped after PatchStatus re-fetches the object.
Is EtcdDataEncryptionUpToDate the only condition reconcile() touches? If so this is fine — but worth a comment saying so. If not, the other conditions need to be captured and replayed too.
There was a problem hiding this comment.
Good catch. Confirmed — reconcile() only mutates SecretEncryption and the EtcdDataEncryptionUpToDate condition. No other status fields or conditions. Added a comment on the snapshot block stating this explicitly.
openshift-ci
Bot
added
needs-rebase
9a6b2ba to
f0f6ec1
Compare
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 4458-4573: Extend TestReconcileDefaultSecurityGroup with a
successful createAWSDefaultSecurityGroup table case, configuring the EC2 mock
for the required VPC lookup and security-group creation calls. Mark the identity
provider ready, reconcile successfully, re-read the HostedControlPlane from
fakeClient, and assert AWSDefaultSecurityGroupCreated is True,
DefaultWorkerSecurityGroupID is non-empty, and no error occurs.
- Around line 4435-4444: The test currently checks the in-memory hcp condition
rather than the persisted status. After r.reconcileDeletion returns, re-fetch
hcp with fakeClient.Get using client.ObjectKeyFromObject(hcp), then assert the
deletion condition on the re-read object while preserving the existing reconcile
error assertion.
🪄 Autofix
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: Pro Plus
Run ID: 470ad102-68d8-4cb7-ad19-2c2ec8ab387e
📒 Files selected for processing (3)
control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.gocontrol-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/reencryption/reencryption.go
🚧 Files skipped from review as they are similar to previous changes (1)
- control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go
|
/lgtm |
|
Scheduling tests matching the |
f0f6ec1 to
756f882
Compare
|
/test security |
756f882 to
7d42488
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
/lgtm |
|
Scheduling tests matching the |
|
/retest |
|
/rebase |
|
🤖 Rebasing PR onto main: workflow run |
…ng helpers Migrate 9 status patch call sites across hostedcontrolplane_controller.go and reencryption.go to use the shared statuspatching package, adding retry-on-conflict and consistent optimistic locking. - hostedcontrolplane_controller.go: 7 sites migrated to PatchStatus / PatchStatusCondition (reconcileDeletion, update, reconcileValidIDP, removeCloudResources, reconcileDefaultSecurityGroup) - reencryption.go: 1 site migrated to PatchStatus - 2 batch-patch sites (lines 686, 869) deferred — they accumulate status changes across the full reconcile loop and need restructuring Signed-off-by: Vimal Solanki <vsolanki@redhat.com>
7d42488 to
af4c08c
Compare
|
/lgtm Putting this back on after rebase |
|
Scheduling tests matching the |
|
/retest |
1 similar comment
|
/retest |
|
@vsolanki12: 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. |
5 participants
What this PR does / why we need it:
Migrates 9 status patch call sites in the CPO to use
statuspatching.PatchStatus/PatchStatusCondition, adding retry-on-conflict and consistent optimistic locking.Part of the broader CNTRLPLANE-3532 migration. Depends on PR #8782 (merged).
Which issue(s) this PR fixes:
Fixes CNTRLPLANE-3532
Special notes for your reviewer:
originalHostedControlPlaneparameter was removed fromreconcileDeletionsince the migrated helpers handle re-fetching internally.Checklist:
Summary by CodeRabbit
Bug Fixes
Tests