CNTRLPLANE-3532: migrate CPO status patches to statuspatching helpers - #8966
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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe controllers now use Suggested reviewers: 🚥 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.99% 46.08% +0.08%
==========================================
Files 781 784 +3
Lines 98072 98746 +674
==========================================
+ Hits 45110 45508 +398
- Misses 49892 50161 +269
- Partials 3070 3077 +7
... and 20 files 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:
|
![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
|
@vismishr: 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. |
|
/uncc @devguyio |
bryan-cox
left a comment
bryan-cox
left a comment
There was a problem hiding this comment.
Thanks for splitting this migration into smaller pieces. I see that #8782 and #8968 are related and that this PR explicitly defers two call sites.
I’m requesting changes because several migrated call sites compute their desired status before PatchStatus performs its internal fetch/retry. The optimistic lock therefore protects only the freshly fetched object, not the state used to calculate the update, allowing newer status or spec changes to be overwritten. The inline comments identify the concrete cases.
The PR also describes itself as fixing CNTRLPLANE-3532, but several Jira acceptance criteria remain unimplemented or untracked: the shared JSON Patch variant and nullable-field test, the status-patching linter, migration of remaining direct HCP patches, and AGENTS.md guidance. Please link concrete follow-up work and adjust the completion wording, or complete those criteria here.
Finally, TESTING.md requires direct tests for modified functions; TestUpdate, TestReconcileValidIDPConfigurationCondition, and a primary status-patch error case for re-encryption are missing. The commit subject also includes the Jira ID, contrary to DEVELOPMENT.md.
| if err := r.Status().Patch(ctx, hcp, client.MergeFromWithOptions(originalHCP, client.MergeFromWithOptimisticLock{})); err != nil { | ||
| return fmt.Errorf("failed to patch valid IDP configuration condition: %w", err) | ||
| } | ||
| if err := statuspatching.PatchStatusCondition(ctx, r.Client, hcp, &hcp.Status.Conditions, new); err != nil { |
There was a problem hiding this comment.
Blocking: new is calculated before PatchStatusCondition, but the helper re-fetches the HCP before applying it and on every retry. If the IDP configuration changes meanwhile, this applies a stale validation result to the latest object without producing a conflict. Please detect that the evaluated generation changed and rerun the full reconciliation. Also add TestReconcileValidIDPConfigurationCondition as required by TESTING.md.
There was a problem hiding this comment.
Good catch. Guarded against this: captured hcp.Generation before computing the condition, and inside PatchStatus's closure (using the freshly re-fetched hcp) return an error if generation changed instead of patching a stale evaluation — forces the whole reconcile to requeue and re-evaluate. Note: this catches spec changes but not IDP-override-annotation-only changes (annotations don't bump Generation); flagged that gap in a code comment since it's narrow (IBM Cloud-specific) and low severity. Added TestReconcileValidIDPConfigurationCondition covering valid/invalid/generation-changed cases.
| } | ||
| originalHCP := hcp.DeepCopy() | ||
| meta.SetStatusCondition(&hcp.Status.Conditions, metav1.Condition{ | ||
| if err := statuspatching.PatchStatusCondition(ctx, r.Client, hcp, &hcp.Status.Conditions, metav1.Condition{ |
There was a problem hiding this comment.
Blocking: The timeout decision uses the earlier resourcesDestroyedCond, but PatchStatusCondition then fetches current status and unconditionally writes False. If HCCO concurrently sets CloudResourcesDestroyed=True, this overwrites that successful result with CloudResourcesDeletionTimedOut. The previous optimistic-lock patch would conflict and requeue. Please re-evaluate the condition and timeout against the refreshed object inside the mutation, and add a concurrent-update test.
There was a problem hiding this comment.
Done. Re-evaluate against the refreshed object inside the mutation: the closure now re-checks CloudResourcesDestroyed on the freshly re-fetched hcp and skips patching entirely if HCCO already set it True, instead of blindly overwriting with the timeout condition. Added a concurrent-update test case using a Get interceptor that injects and persists the concurrent True write on the first fetch only, so the test's own verification read reflects real stored state.
| meta.SetStatusCondition(&hcp.Status.Conditions, *condition) | ||
|
|
||
| if err := r.Client.Status().Patch(ctx, hcp, client.MergeFromWithOptions(originalHCP, client.MergeFromWithOptimisticLock{})); err != nil { | ||
| if err := statuspatching.PatchStatus(ctx, r.Client, hcp, func() error { |
There was a problem hiding this comment.
Blocking: sgID, condition, and creationErr were computed using the earlier HCP, but PatchStatus applies them to a freshly fetched object and retries until successful. If the relevant AWS configuration changes during the cloud calls, the old security-group result can be attached to the new HCP state. Please guard the evaluated generation/spec and rerun the reconciliation when it changes, with a conflict test.
There was a problem hiding this comment.
Done. Captured hcp.Generation right before the AWS calls, and guarded the PatchStatus closure the same way as reconcileValidIDPConfigurationCondition: if generation changed while creating the SG, return an error instead of attaching the stale result to the new spec state — forces a full re-reconcile, which is safe since createAWSDefaultSecurityGroup searches by infraID tag before creating (idempotent). Added TestReconcileDefaultSecurityGroup_GenerationConflict.
| if currentState == hyperv1.EncryptionMigrationStateCompleted && previousState != currentState { | ||
| recordMigrationDuration(r.hcpNamespace, r.hcpName, hcp.Status.SecretEncryption) | ||
| } | ||
| if patchErr := statuspatching.PatchStatus(ctx, r.cpClient, hcp, func() error { |
There was a problem hiding this comment.
Blocking: workingCopy is computed once from the initially fetched HCP, then copied into whatever object PatchStatus fetches during each retry. A concurrent update to SecretEncryption or EtcdDataEncryptionUpToDate can therefore be silently overwritten—the exact failure this migration intends to prevent. Please rerun the status calculation from refreshed state or let the full reconcile retry instead of replaying this snapshot. Add a conflict test that verifies concurrent state is preserved, plus the required patch-error path.
There was a problem hiding this comment.
Traced this carefully before deciding how to fix it. hcp.Status.SecretEncryption and the EtcdDataEncryptionUpToDate condition are written exclusively by this one reconciler for this HCP — repo-wide grep confirms no other controller writes either field (the only other references, in hostedcluster_controller.go/reconcile_legacy.go, are read-only FindStatusCondition calls bubbling it up to HostedCluster status). Combined with controller-runtime's per-key reconcile serialization (workqueue defers Add() on a key already in processing until Done()), there's no concurrent writer that could actually be clobbered here — replaying workingCopy on a PatchStatus retry only ever re-applies our own already-correct value onto our own object. Rather than adding an unneeded redesign, documented this single-owner invariant in a comment so a future second writer would prompt someone to revisit it, and added TestReconcile_PatchStatusError for the patch-error path you flagged as missing. Happy to discuss further if you see a writer I missed.
…ching 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 Additionally guards reconcileValidIDPConfigurationCondition and reconcileDefaultSecurityGroup against a stale hcp.Generation, since PatchStatus's internal retry replays a precomputed value onto whatever fresh state it fetches; a spec change mid-flight must not be silently applied. removeCloudResources' timeout branch re-checks CloudResourcesDestroyed against the freshly fetched object before overwriting it, since HCCO can concurrently set it to True. Signed-off-by: Vimal Solanki <vsolanki@redhat.com>
4151250 to
48712e0
Compare
openshift-ci-robot
removed
the
verified
|
@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.1.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. |
|
@bryan-cox Addressed all 4 inline findings individually (generation guards on reconcileValidIDPConfigurationCondition/reconcileDefaultSecurityGroup, fresh-state re-check on removeCloudResources, traced+declined the reencryption.go one with evidence — see inline reply). On the broader points:
|
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cblecker, vsolanki12 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 |
openshift-ci
Bot
added
the
approved
|
/test e2e-aws |
|
/test e2e-aws |
|
/test e2e-v2-gke |
|
/retest-required |
|
/test e2e-v2-azure-self-managed/ The test that failed was run before I increased the node size of the root ci cluster. There were too many pods for the kubelet errors. |
|
/test e2e-v2-azure-self-managed The test that failed was run before I increased the node size of the root ci cluster. There were too many pods for the kubelet errors. |
|
/retest |
|
/verified by @vismishr all results in https://vismishr.github.io/pr-8966-test-verification.html , tested on live cluster |
openshift-ci-robot
added
the
verified
|
@vismishr: 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. |
|
@vsolanki12: 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. |
8 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).
This PR alone does not close CNTRLPLANE-3532. Remaining work tracked as follow-up PRs:
resources.go:destroyCloudResources'sCloudResourcesDestroyedcondition patch is still rawMergeFromWithOptimisticLock, not yet on the shared helper (the other two HCCO sites were already migrated via OCPBUGS-93462: Fix stale resourceVersion in HCCO patchHCPStatusCondition #8902).removeHCPIngressFromRoutes(hostedcontrolplane_controller.go) — currently bareclient.MergeFromwith no optimistic lock at all.support/statuspatchingJSON Patch (RFC 6902) variant + nullable-field test (Jira AC, not yet implemented).make lintintegration.AGENTS.mdguidance update.Which issue(s) this PR fixes:
Part of CNTRLPLANE-3532 — does not fully close it, see remaining work above.
Special notes for your reviewer:
originalHostedControlPlaneparameter was removed fromreconcileDeletionsince the migrated helpers handle re-fetching internally.reconcileValidIDPConfigurationConditionandreconcileDefaultSecurityGroupnow guard against a stalehcp.Generation(spec changed mid-flight) before patching, rather than blindly re-applying a value computed from stale state onPatchStatus's internal retry.removeCloudResources's timeout branch re-checksCloudResourcesDestroyedagainst the freshly re-fetched object before overwriting it, since HCCO can concurrently set it toTrue.reencryption.go'sworkingCopypattern was reviewed for the same class of bug — confirmed no live race (single-owner fields, controller-runtime serializes reconciles per key) and documented the invariant in a code comment; added the requested patch-error-path test.Checklist:
Summary by CodeRabbit
Bug Fixes
Tests