Skip to content

CNTRLPLANE-3532: migrate CPO status patches to statuspatching helpers #8966

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
vsolanki12 wants to merge 1 commit into
base: main
Choose a base branch
from
+86 −79
Open

CNTRLPLANE-3532: migrate CPO status patches to statuspatching helpers #8966

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
116 changes: 51 additions & 65 deletions control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ import (
"github.com/openshift/hypershift/support/metrics"
"github.com/openshift/hypershift/support/netutil"
"github.com/openshift/hypershift/support/releaseinfo"
"github.com/openshift/hypershift/support/statuspatching"
"github.com/openshift/hypershift/support/upsert"
"github.com/openshift/hypershift/support/util"
"github.com/openshift/hypershift/support/validations"
Expand Down Expand Up @@ -381,8 +382,8 @@ func (r *HostedControlPlaneReconciler) eventHandlers(scheme *runtime.Scheme, res
return handlers
}

func (r *HostedControlPlaneReconciler) reconcileDeletion(ctx context.Context, hostedControlPlane *hyperv1.HostedControlPlane, originalHostedControlPlane *hyperv1.HostedControlPlane) (ctrl.Result, error) {
condition := &metav1.Condition{
func (r *HostedControlPlaneReconciler) reconcileDeletion(ctx context.Context, hostedControlPlane *hyperv1.HostedControlPlane) (ctrl.Result, error) {
condition := metav1.Condition{
Type: string(hyperv1.AWSDefaultSecurityGroupDeleted),
}
if shouldCleanupCloudResources(r.Log, hostedControlPlane) {
Expand All @@ -393,9 +394,7 @@ func (r *HostedControlPlaneReconciler) reconcileDeletion(ctx context.Context, ho
}
condition.Reason = hyperv1.AWSErrorReason
condition.Status = metav1.ConditionFalse
meta.SetStatusCondition(&hostedControlPlane.Status.Conditions, *condition)

if err := r.Client.Status().Patch(ctx, hostedControlPlane, client.MergeFromWithOptions(originalHostedControlPlane, client.MergeFromWithOptimisticLock{})); err != nil {
if err := statuspatching.PatchStatusCondition(ctx, r.Client, hostedControlPlane, &hostedControlPlane.Status.Conditions, condition); err != nil {
return ctrl.Result{}, fmt.Errorf("failed to update status on hcp for security group deletion: %w. Condition error message: %v", err, condition.Message)
}

Expand All @@ -411,9 +410,7 @@ func (r *HostedControlPlaneReconciler) reconcileDeletion(ctx context.Context, ho
condition.Message = hyperv1.AllIsWellMessage
condition.Reason = hyperv1.AsExpectedReason
condition.Status = metav1.ConditionTrue
meta.SetStatusCondition(&hostedControlPlane.Status.Conditions, *condition)

if err := r.Client.Status().Patch(ctx, hostedControlPlane, client.MergeFromWithOptions(originalHostedControlPlane, client.MergeFromWithOptimisticLock{})); err != nil {
if err := statuspatching.PatchStatusCondition(ctx, r.Client, hostedControlPlane, &hostedControlPlane.Status.Conditions, condition); err != nil {
return ctrl.Result{}, fmt.Errorf("failed to update status on hcp for security group deletion: %w. Condition message: %v", err, condition.Message)
}
}
Expand Down Expand Up @@ -598,7 +595,7 @@ func (r *HostedControlPlaneReconciler) Reconcile(ctx context.Context, req ctrl.R
originalHostedControlPlane := hostedControlPlane.DeepCopy()

if !hostedControlPlane.DeletionTimestamp.IsZero() {
return r.reconcileDeletion(ctx, hostedControlPlane, originalHostedControlPlane)
return r.reconcileDeletion(ctx, hostedControlPlane)
}

if !controllerutil.ContainsFinalizer(hostedControlPlane, finalizer) {
Expand Down Expand Up @@ -1163,32 +1160,27 @@ func (r *HostedControlPlaneReconciler) update(ctx context.Context, hostedControl
errs = append(errs, err)
}

// Get the latest HCP in memory before we patch the status
if err = r.Client.Get(ctx, client.ObjectKeyFromObject(hostedControlPlane), hostedControlPlane); err != nil {
return reconcile.Result{}, err
}

originalHostedControlPlane := hostedControlPlane.DeepCopy()
missingImages := sets.New(releaseImageProvider.GetMissingImages()...).Insert(userReleaseImageProvider.GetMissingImages()...)
if missingImages.Len() == 0 {
meta.SetStatusCondition(&hostedControlPlane.Status.Conditions, metav1.Condition{
Type: string(hyperv1.ValidReleaseInfo),
Status: metav1.ConditionTrue,
Reason: hyperv1.AsExpectedReason,
Message: hyperv1.AllIsWellMessage,
ObservedGeneration: hostedControlPlane.Generation,
})
} else {
meta.SetStatusCondition(&hostedControlPlane.Status.Conditions, metav1.Condition{
Type: string(hyperv1.ValidReleaseInfo),
Status: metav1.ConditionFalse,
Reason: hyperv1.MissingReleaseImagesReason,
Message: strings.Join(missingImages.UnsortedList(), ", "),
ObservedGeneration: hostedControlPlane.Generation,
})
}

if err := r.Client.Status().Patch(ctx, hostedControlPlane, client.MergeFromWithOptions(originalHostedControlPlane, client.MergeFromWithOptimisticLock{})); err != nil {
if err := statuspatching.PatchStatus(ctx, r.Client, hostedControlPlane, func() error {
if missingImages.Len() == 0 {
meta.SetStatusCondition(&hostedControlPlane.Status.Conditions, metav1.Condition{
Type: string(hyperv1.ValidReleaseInfo),
Status: metav1.ConditionTrue,
Reason: hyperv1.AsExpectedReason,
Message: hyperv1.AllIsWellMessage,
ObservedGeneration: hostedControlPlane.Generation,

@cblecker cblecker Jul 14, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@vsolanki12 vsolanki12 Jul 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

})
} else {
meta.SetStatusCondition(&hostedControlPlane.Status.Conditions, metav1.Condition{
Type: string(hyperv1.ValidReleaseInfo),
Status: metav1.ConditionFalse,
Reason: hyperv1.MissingReleaseImagesReason,
Message: strings.Join(missingImages.UnsortedList(), ", "),
ObservedGeneration: hostedControlPlane.Generation,
})
}
return nil
}); err != nil {
errs = append(errs, fmt.Errorf("failed to update status: %w", err))
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

Expand Down Expand Up @@ -2081,12 +2073,8 @@ func (r *HostedControlPlaneReconciler) reconcileValidIDPConfigurationCondition(c
Message: fmt.Sprintf("failed to initialize identity providers: %v", err),
}
}
// Patch the condition on the HCP if it has changed
originalHCP := hcp.DeepCopy()
if meta.SetStatusCondition(&hcp.Status.Conditions, new) {
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 {
return fmt.Errorf("failed to patch valid IDP configuration condition: %w", err)
}
return nil
}
Expand Down Expand Up @@ -2578,14 +2566,12 @@ func (r *HostedControlPlaneReconciler) removeCloudResources(ctx context.Context,
if resourcesDestroyedCond != nil && resourcesDestroyedCond.Message != "" {
message = fmt.Sprintf("%s (last status: %s)", message, resourcesDestroyedCond.Message)
}
originalHCP := hcp.DeepCopy()
meta.SetStatusCondition(&hcp.Status.Conditions, metav1.Condition{
if err := statuspatching.PatchStatusCondition(ctx, r.Client, hcp, &hcp.Status.Conditions, metav1.Condition{
Type: string(hyperv1.CloudResourcesDestroyed),
Status: metav1.ConditionFalse,
Reason: string(hyperv1.CloudResourcesDeletionTimedOutReason),
Message: message,
})
if err := r.Status().Patch(ctx, hcp, client.MergeFromWithOptions(originalHCP, client.MergeFromWithOptimisticLock{})); err != nil {
}); err != nil {
return false, fmt.Errorf("failed to patch cloud resources destroyed condition: %w", err)
}
return true, nil
Expand All @@ -2611,15 +2597,11 @@ func (r *HostedControlPlaneReconciler) removeCloudResources(ctx context.Context,
return false, nil
}
if cvoScaledDownCond == nil || cvoScaledDownCond.Status != metav1.ConditionTrue {
originalHCP := hcp.DeepCopy()
cvoScaledDownCond = &metav1.Condition{
Type: string(hyperv1.CVOScaledDown),
Status: metav1.ConditionTrue,
Reason: "CVOScaledDown",
LastTransitionTime: metav1.Now(),
}
meta.SetStatusCondition(&hcp.Status.Conditions, *cvoScaledDownCond)
if err := r.Status().Patch(ctx, hcp, client.MergeFromWithOptions(originalHCP, client.MergeFromWithOptimisticLock{})); err != nil {
if err := statuspatching.PatchStatusCondition(ctx, r.Client, hcp, &hcp.Status.Conditions, metav1.Condition{
Type: string(hyperv1.CVOScaledDown),
Status: metav1.ConditionTrue,
Reason: "CVOScaledDown",
}); err != nil {
return false, fmt.Errorf("failed to patch CVO scaled down condition: %w", err)
}
}
Expand Down Expand Up @@ -2687,11 +2669,10 @@ func (r *HostedControlPlaneReconciler) reconcileDefaultSecurityGroup(ctx context
return nil
}

originalHCP := hcp.DeepCopy()
var condition *metav1.Condition
var condition metav1.Condition
sgID, appliedTags, creationErr := createAWSDefaultSecurityGroup(ctx, r.ec2Client, hcp)
if creationErr != nil {
condition = &metav1.Condition{
condition = metav1.Condition{
Type: string(hyperv1.AWSDefaultSecurityGroupCreated),
Status: metav1.ConditionFalse,
Message: creationErr.Error(),
Expand All @@ -2708,23 +2689,28 @@ func (r *HostedControlPlaneReconciler) reconcileDefaultSecurityGroup(ctx context
}); err != nil {
return fmt.Errorf("failed to update HostedControlPlane object: %w", err)
}
originalHCP = hcp.DeepCopy()

condition = &metav1.Condition{
condition = metav1.Condition{
Type: string(hyperv1.AWSDefaultSecurityGroupCreated),
Status: metav1.ConditionTrue,
Message: hyperv1.AllIsWellMessage,
Reason: hyperv1.AsExpectedReason,
}
hcp.Status.Platform = &hyperv1.PlatformStatus{
AWS: &hyperv1.AWSPlatformStatus{
DefaultWorkerSecurityGroupID: sgID,
},
}
}
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 {
meta.SetStatusCondition(&hcp.Status.Conditions, condition)
if creationErr == nil {
if hcp.Status.Platform == nil {
hcp.Status.Platform = &hyperv1.PlatformStatus{}
}
if hcp.Status.Platform.AWS == nil {
hcp.Status.Platform.AWS = &hyperv1.AWSPlatformStatus{}
}
hcp.Status.Platform.AWS.DefaultWorkerSecurityGroupID = sgID
}
return nil
}); err != nil {
return fmt.Errorf("failed to update status: %w", err)
}

Expand Down
5 changes: 2 additions & 3 deletions control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4432,17 +4432,16 @@ func TestReconcileDeletion(t *testing.T) {

ctx := ctrl.LoggerInto(t.Context(), ctrl.Log.WithName("test"))

// Re-read from fake client so the object has a ResourceVersion for OptimisticLock
// Re-read from fake client so the object has a ResourceVersion for PatchStatusCondition
g.Expect(fakeClient.Get(ctx, client.ObjectKeyFromObject(hcp), hcp)).To(Succeed())
originalHCP := hcp.DeepCopy()

r := &HostedControlPlaneReconciler{
Client: fakeClient,
Log: ctrl.Log.WithName("test"),
ec2Client: mockEC2,
}

_, err := r.reconcileDeletion(ctx, hcp, originalHCP)
_, err := r.reconcileDeletion(ctx, hcp)
if tt.wantErr {
g.Expect(err).To(HaveOccurred())
} else {
Expand Down
44 changes: 33 additions & 11 deletions control-plane-operator/hostedclusterconfigoperator/controllers/reencryption/reencryption.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
"github.com/openshift/hypershift/control-plane-operator/controllers/hostedcontrolplane/v2/kas/kms"
"github.com/openshift/hypershift/support/config"
"github.com/openshift/hypershift/support/secretencryption"
"github.com/openshift/hypershift/support/statuspatching"
"github.com/openshift/hypershift/support/util"

"github.com/openshift/library-go/pkg/operator/encryption/controllers/migrators"
Expand Down Expand Up @@ -62,24 +63,45 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco
return reconcile.Result{}, fmt.Errorf("failed to get HCP: %w", err)
}

originalHCP := hcp.DeepCopy()
// Snapshot pre-reconcile encryption state for metrics comparison.
previousEncryption := *hcp.Status.SecretEncryption.DeepCopy()

result, err := r.reconcile(ctx, log, hcp)
if err != nil {
return result, err
}

if !equality.Semantic.DeepEqual(hcp.Status, originalHCP.Status) {
log.Info("Patching HCP status with secret encryption changes")
patch := crclient.MergeFrom(originalHCP)
if err := r.cpClient.Status().Patch(ctx, hcp, patch); err != nil {
return reconcile.Result{}, fmt.Errorf("failed to patch HCP status: %w", err)
// 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

@bryan-cox bryan-cox Jul 28, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@vsolanki12 vsolanki12 Jul 28, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

hasCondition := false
if c := meta.FindStatusCondition(hcp.Status.Conditions, string(hyperv1.EtcdDataEncryptionUpToDate)); c != nil {
desiredCondition = *c
hasCondition = true
}

if patchErr := statuspatching.PatchStatus(ctx, r.cpClient, hcp, func() error {
hcp.Status.SecretEncryption = desiredEncryption
if hasCondition {
meta.SetStatusCondition(&hcp.Status.Conditions, desiredCondition)
} else {
meta.RemoveStatusCondition(&hcp.Status.Conditions, string(hyperv1.EtcdDataEncryptionUpToDate))
}
log.Info("Successfully patched HCP status")
recordMigrationState(r.hcpNamespace, r.hcpName, hcp.Status.SecretEncryption)
previousState := encryptionHistoryState(originalHCP.Status.SecretEncryption)
currentState := encryptionHistoryState(hcp.Status.SecretEncryption)
return nil
}); patchErr != nil {
return reconcile.Result{}, fmt.Errorf("failed to patch HCP status: %w", patchErr)
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

// Always re-emit the current migration state gauge so it survives pod restarts.
recordMigrationState(r.hcpNamespace, r.hcpName, desiredEncryption)

// Record duration only on actual state transitions.
if !equality.Semantic.DeepEqual(previousEncryption, desiredEncryption) {

@cblecker cblecker Jul 14, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@vsolanki12 vsolanki12 Jul 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

previousState := encryptionHistoryState(previousEncryption)
currentState := encryptionHistoryState(desiredEncryption)
if currentState == hyperv1.EncryptionMigrationStateCompleted && previousState != currentState {
recordMigrationDuration(r.hcpNamespace, r.hcpName, hcp.Status.SecretEncryption)
recordMigrationDuration(r.hcpNamespace, r.hcpName, desiredEncryption)
}
}

Expand Down