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
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 @@ -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.

Expand Down Expand Up @@ -2120,12 +2112,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 @@ -2617,14 +2605,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 @@ -2650,15 +2636,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 @@ -2726,11 +2708,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 @@ -2747,23 +2728,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 {

@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.

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
155 changes: 151 additions & 4 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 @@ -4443,30 +4443,177 @@ 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)
Comment thread
coderabbitai[bot] marked this conversation as resolved.
if tt.wantErr {
g.Expect(err).To(HaveOccurred())
} else {
g.Expect(err).ToNot(HaveOccurred())
}

cond := meta.FindStatusCondition(hcp.Status.Conditions, string(hyperv1.AWSDefaultSecurityGroupDeleted))
// Re-read from server to verify persisted status.
updated := &hyperv1.HostedControlPlane{}
g.Expect(fakeClient.Get(ctx, client.ObjectKeyFromObject(hcp), updated)).To(Succeed())
cond := meta.FindStatusCondition(updated.Status.Conditions, string(hyperv1.AWSDefaultSecurityGroupDeleted))
g.Expect(cond).ToNot(BeNil())
g.Expect(cond.Status).To(Equal(tt.wantCondStatus))
})
}
}

func TestReconcileDefaultSecurityGroup(t *testing.T) {
tests := []struct {
name string
setupEC2Mock func(*gomock.Controller) *awsapi.MockEC2API
wantCondStatus metav1.ConditionStatus
wantCondReason string
wantPlatform bool
wantErr bool
}{
{
name: "When creation fails it should set error condition via PatchStatus and not touch platform",
setupEC2Mock: func(mockCtrl *gomock.Controller) *awsapi.MockEC2API {
m := awsapi.NewMockEC2API(mockCtrl)
m.EXPECT().DescribeVpcs(gomock.Any(), gomock.Any()).Return(nil,
&smithy.GenericAPIError{Code: "VpcNotFound", Message: "vpc not found"})
return m
},
wantCondStatus: metav1.ConditionFalse,
wantCondReason: hyperv1.AWSErrorReason,
wantPlatform: false,
wantErr: true,
},
{
name: "When identity provider is not ready it should skip without error",
setupEC2Mock: func(mockCtrl *gomock.Controller) *awsapi.MockEC2API {
return awsapi.NewMockEC2API(mockCtrl)
},
wantErr: false,
},
{
name: "When existing SG is found it should set success condition and platform status",
setupEC2Mock: func(mockCtrl *gomock.Controller) *awsapi.MockEC2API {
m := awsapi.NewMockEC2API(mockCtrl)
m.EXPECT().DescribeVpcs(gomock.Any(), gomock.Any()).Return(&ec2.DescribeVpcsOutput{
Vpcs: []ec2types.Vpc{{VpcId: aws.String("vpc-123")}},
}, nil)
m.EXPECT().DescribeSecurityGroups(gomock.Any(), gomock.Any()).Return(&ec2.DescribeSecurityGroupsOutput{
SecurityGroups: []ec2types.SecurityGroup{{
GroupId: aws.String("sg-existing"),
OwnerId: aws.String("123456789012"),
Tags: []ec2types.Tag{
{Key: aws.String("Name"), Value: aws.String("test-infra-default-sg")},
},
}},
}, nil)
m.EXPECT().AuthorizeSecurityGroupIngress(gomock.Any(), gomock.Any()).Return(
&ec2.AuthorizeSecurityGroupIngressOutput{}, nil)
return m
},
wantCondStatus: metav1.ConditionTrue,
wantCondReason: hyperv1.AsExpectedReason,
wantPlatform: true,
wantErr: false,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
g := NewWithT(t)
mockCtrl := gomock.NewController(t)
mockEC2 := tt.setupEC2Mock(mockCtrl)

conditions := []metav1.Condition{}
if tt.wantCondStatus != "" {
// Only add ValidAWSIdentityProvider=True for tests that should reach creation.
conditions = append(conditions, metav1.Condition{
Type: string(hyperv1.ValidAWSIdentityProvider),
Status: metav1.ConditionTrue,
Reason: hyperv1.AsExpectedReason,
})
}

hcp := &hyperv1.HostedControlPlane{
ObjectMeta: metav1.ObjectMeta{
Name: "test-hcp",
Namespace: "test-ns",
},
Spec: hyperv1.HostedControlPlaneSpec{
InfraID: "test-infra",
Platform: hyperv1.PlatformSpec{
Type: hyperv1.AWSPlatform,
AWS: &hyperv1.AWSPlatformSpec{
CloudProviderConfig: &hyperv1.AWSCloudProviderConfig{
VPC: "vpc-123",
},
},
},
Networking: hyperv1.ClusterNetworking{
MachineNetwork: []hyperv1.MachineNetworkEntry{
{CIDR: *ipnet.MustParseCIDR("10.0.0.0/16")},
},
},
},
Status: hyperv1.HostedControlPlaneStatus{
Conditions: conditions,
},
}
fakeClient := fake.NewClientBuilder().
WithScheme(api.Scheme).
WithObjects(hcp).
WithStatusSubresource(&hyperv1.HostedControlPlane{}).
Build()

ctx := ctrl.LoggerInto(t.Context(), ctrl.Log.WithName("test"))
g.Expect(fakeClient.Get(ctx, client.ObjectKeyFromObject(hcp), hcp)).To(Succeed())

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

err := r.reconcileDefaultSecurityGroup(ctx, hcp)
if tt.wantErr {
g.Expect(err).To(HaveOccurred())
} else {
g.Expect(err).ToNot(HaveOccurred())
}

if tt.wantCondStatus == "" {
return
}

// Re-read from server to verify persisted status.
updated := &hyperv1.HostedControlPlane{}
g.Expect(fakeClient.Get(ctx, client.ObjectKeyFromObject(hcp), updated)).To(Succeed())

cond := meta.FindStatusCondition(updated.Status.Conditions, string(hyperv1.AWSDefaultSecurityGroupCreated))
g.Expect(cond).ToNot(BeNil(), "condition should be set")
g.Expect(cond.Status).To(Equal(tt.wantCondStatus))
g.Expect(cond.Reason).To(Equal(tt.wantCondReason))

if tt.wantPlatform {
g.Expect(updated.Status.Platform).ToNot(BeNil())
g.Expect(updated.Status.Platform.AWS).ToNot(BeNil())
g.Expect(updated.Status.Platform.AWS.DefaultWorkerSecurityGroupID).ToNot(BeEmpty(),
"DefaultWorkerSecurityGroupID should be set on success")
} else {
g.Expect(updated.Status.Platform).To(BeNil(),
"platform status should remain nil when creation fails")
}
})
}
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

func TestHealthCheckKASEndpoint(t *testing.T) {
t.Parallel()
tests := []struct {
Expand Down
Loading