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

Merged
openshift-merge-bot merged 1 commit into from
Aug 27, 2026
Merged

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
158 changes: 86 additions & 72 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 @@ -88,6 +88,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 @@ -382,8 +383,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 @@ -394,9 +395,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 @@ -414,9 +413,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 @@ -601,7 +598,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 @@ -1168,32 +1165,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(sets.List(missingImages), ", "),
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.
Resolved

Expand Down Expand Up @@ -2103,6 +2095,12 @@ func (r *HostedControlPlaneReconciler) cleanupOldRedHatMarketplaceCatalogResourc
func (r *HostedControlPlaneReconciler) reconcileValidIDPConfigurationCondition(ctx context.Context, hcp *hyperv1.HostedControlPlane, releaseImageProvider imageprovider.ReleaseImageProvider, oauthHost string, oauthPort int32) error {
p := oauth.NewOAuthServerParams(hcp, releaseImageProvider, oauthHost, oauthPort, r.SetDefaultSecurityContext)

// The condition below is evaluated from the spec at this generation. If the spec
// changes before we patch, the evaluation is stale and must not be applied blindly.
// Note: p.OauthConfigOverrides is derived from IDP-override annotations, which don't
// bump Generation — an annotation-only change in this window isn't caught by this guard.
evaluatedGeneration := hcp.Generation

// Report any IDP configuration errors as a condition on the HCP
new := metav1.Condition{
Type: string(hyperv1.ValidIDPConfiguration),
Expand All @@ -2120,12 +2118,14 @@ 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.PatchStatus(ctx, r.Client, hcp, func() error {
if hcp.Generation != evaluatedGeneration {
return fmt.Errorf("hcp generation changed from %d to %d while evaluating IDP configuration, requeueing to re-evaluate", evaluatedGeneration, hcp.Generation)
}
meta.SetStatusCondition(&hcp.Status.Conditions, new)
return nil
}); err != nil {
return fmt.Errorf("failed to patch valid IDP configuration condition: %w", err)
}
return nil
}
Expand Down Expand Up @@ -2636,18 +2636,25 @@ func (r *HostedControlPlaneReconciler) removeCloudResources(ctx context.Context,

if timeElapsed > resourceDeletionTimeout {
log.Info("Giving up on resource deletion after timeout", "timeElapsed", duration.ShortHumanDuration(timeElapsed))
message := fmt.Sprintf("Giving up on cloud resource deletion after %s", duration.ShortHumanDuration(timeElapsed))
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{
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 {
if err := statuspatching.PatchStatus(ctx, r.Client, hcp, func() error {
// HCCO writes this same condition concurrently when it actually finishes
// destroying resources. Re-check the freshly fetched object so we don't
// clobber a concurrent True with a stale timeout.
if fresh := meta.FindStatusCondition(hcp.Status.Conditions, string(hyperv1.CloudResourcesDestroyed)); fresh != nil && fresh.Status == metav1.ConditionTrue {
return nil
}
message := fmt.Sprintf("Giving up on cloud resource deletion after %s", duration.ShortHumanDuration(timeElapsed))
if resourcesDestroyedCond != nil && resourcesDestroyedCond.Message != "" {
message = fmt.Sprintf("%s (last status: %s)", message, resourcesDestroyedCond.Message)
}
meta.SetStatusCondition(&hcp.Status.Conditions, metav1.Condition{
Type: string(hyperv1.CloudResourcesDestroyed),
Status: metav1.ConditionFalse,
Reason: string(hyperv1.CloudResourcesDeletionTimedOutReason),
Message: message,
})
return nil
}); err != nil {
return false, fmt.Errorf("failed to patch cloud resources destroyed condition: %w", err)
}
return true, nil
Expand All @@ -2673,15 +2680,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 @@ -2749,11 +2752,14 @@ func (r *HostedControlPlaneReconciler) reconcileDefaultSecurityGroup(ctx context
return nil
}

originalHCP := hcp.DeepCopy()
var condition *metav1.Condition
// The security group result below is derived from the spec at this generation
// (VPC, machine CIDR). If the spec changes before we patch, that result is stale.
evaluatedGeneration := hcp.Generation

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 @@ -2770,23 +2776,31 @@ 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 {

@cblecker cblecker Aug 7, 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.

This is the most complex migration site in the PR — it combines a condition update with a Platform status field update in a single PatchStatus callback, including the init-if-nil pattern for Platform/AWS. The other migration sites (reconcileDeletion, reencryption) have dedicated unit tests covering the PatchStatus flow. Worth adding a TestReconcileDefaultSecurityGroup to cover the success/failure paths and verify existing Platform data is preserved through the nil-safe init.

@vsolanki12 vsolanki12 Aug 8, 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. Added TestReconcileDefaultSecurityGroup covering the failure path (DescribeVpcs error) and the identity-provider-not-ready skip path. Both verify PatchStatus correctly persists the condition and leaves platform status nil when creation fails.

@bryan-cox bryan-cox Aug 24, 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.

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.

@vsolanki12 vsolanki12 Aug 24, 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. 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 hcp.Generation != evaluatedGeneration {
return fmt.Errorf("hcp generation changed from %d to %d while creating default security group, requeueing to re-evaluate", evaluatedGeneration, hcp.Generation)
}
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
Loading
Loading