Skip to content

OCPCLOUD-2986,OCPCLOUD-2985,OCPBUGS-56849: Fix controllers guard on empty status authoritative api + defaulting #1386

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 2 commits into from
Jul 4, 2025
Merged

OCPCLOUD-2986,OCPCLOUD-2985,OCPBUGS-56849: Fix controllers guard on empty status authoritative api + defaulting #1386

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
db257cd
fix: controllers: guard on empty .status.authoritativeAPI
damdo Jul 2, 2025
4421b2c
fix: controllers: guard on empty .status.authoritativeAPI, take 2
damdo Jul 2, 2025
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
107 changes: 76 additions & 31 deletions pkg/controller/machine/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,43 +177,60 @@ func (r *ReconcileMachine) Reconcile(ctx context.Context, request reconcile.Requ
originalConditions := conditions.DeepCopyConditions(m.Status.Conditions)

if r.gate.Enabled(featuregate.Feature(openshiftfeatures.FeatureGateMachineAPIMigration)) {
// Check Status.AuthoritativeAPI
// If not MachineAPI. Set the paused condition true and return early.
//
// Once we have a webhook, we want to remove the check that the AuthoritativeAPI
// field is populated.
if m.Status.AuthoritativeAPI != "" &&
m.Status.AuthoritativeAPI != machinev1.MachineAuthorityMachineAPI {
conditions.Set(m, conditions.TrueConditionWithReason(
PausedCondition,
PausedConditionReason,
"The AuthoritativeAPI is set to %s", string(m.Status.AuthoritativeAPI),
))
if patchErr := r.updateStatus(ctx, m, ptr.Deref(m.Status.Phase, ""), nil, originalConditions); patchErr != nil {
klog.Errorf("%v: error patching status: %v", machineName, patchErr)
switch m.Status.AuthoritativeAPI {
case "":
// An empty .status.authoritativeAPI normally means the resource has not yet been reconciled.
// and that the value in .spec.authoritativeAPI has not been propagated to .status.authoritativeAPI yet.
// This value can be set by two separate controllers, depending which one of them is running at that time,
// or in case they are both running, which one gets to set it first (the operation is idempotent so there is no harm in racing).
// - the cluster-capi-operator machine-api-migration's migration controller
// - this controller

klog.Infof("%v: machine .status.authoritativeAPI is not yet set, setting it to .spec.authoritativeAPI", m.Name)

if err := r.patchStatusAuthoritativeAPI(ctx, m, m.Spec.AuthoritativeAPI); err != nil {
klog.Errorf("%v: error patching status to set .status.authoritativeAPI for machine: %v", m.Name, err)
return reconcile.Result{}, fmt.Errorf("error patching status to set .status.authoritativeAPI for machine %s: %w", m.Name, err)
}
Comment thread
damdo marked this conversation as resolved.

// Return to give a chance to the changes to get propagated.
return reconcile.Result{}, nil

case machinev1.MachineAuthorityClusterAPI, machinev1.MachineAuthorityMigrating:
// In cases when .status.authoritativeAPI is set to machinev1.MachineAuthorityClusterAPI, machinev1.MachineAuthorityMigrating
// the resource should be paused and not reconciled further.
desiredCondition := conditions.TrueConditionWithReason(
PausedCondition, PausedConditionReason,
"The AuthoritativeAPI status is set to '%s'", string(m.Status.AuthoritativeAPI),
)

if _, err := r.ensureUpdatedPausedCondition(ctx, m, desiredCondition,
fmt.Sprintf("%v: machine .status.authoritativeAPI is set to '%s', ensuring machine is paused", machineName, m.Status.AuthoritativeAPI)); err != nil {
return reconcile.Result{}, fmt.Errorf("failed to ensure paused condition: %w", err)
}

klog.Infof("%v: machine is paused, taking no further action", machineName)

return reconcile.Result{}, nil
}

var pausedFalseReason string
if m.Status.AuthoritativeAPI != "" {
pausedFalseReason = fmt.Sprintf("The AuthoritativeAPI is set to %s", string(m.Status.AuthoritativeAPI))
} else {
pausedFalseReason = "The AuthoritativeAPI is not set"
}
case machinev1.MachineAuthorityMachineAPI:
// The authority is MachineAPI and the resource should not be paused.
desiredCondition := conditions.FalseCondition(
PausedCondition, NotPausedConditionReason, machinev1.ConditionSeverityInfo, "%s",
fmt.Sprintf("The AuthoritativeAPI status is set to '%s'", string(m.Status.AuthoritativeAPI)),
)

if updated, err := r.ensureUpdatedPausedCondition(ctx, m, desiredCondition,
fmt.Sprintf("%v: machine .status.authoritativeAPI is set to '%s', unpausing machine", machineName, m.Status.AuthoritativeAPI)); err != nil {
return reconcile.Result{}, fmt.Errorf("failed to ensure paused condition: %w", err)
} else if updated {
klog.Infof("%v: setting machine paused condition to false", machineName)
}

// Set the paused condition to false, continue reconciliation
conditions.Set(m, conditions.FalseCondition(
PausedCondition,
NotPausedConditionReason,
machinev1.ConditionSeverityInfo,
"%s",
pausedFalseReason,
))
if patchErr := r.updateStatus(ctx, m, ptr.Deref(m.Status.Phase, ""), nil, originalConditions); patchErr != nil {
klog.Errorf("%v: error patching status: %v", machineName, patchErr)
// Fallthrough and continue reconcilation.
default:
klog.Errorf("%v: invalid .status.authoritativeAPI '%s'", machineName, m.Status.AuthoritativeAPI)
return reconcile.Result{}, nil // Do not return an error to avoid immediate requeue.
}
}

Expand Down Expand Up @@ -433,6 +450,23 @@ func (r *ReconcileMachine) deleteNode(ctx context.Context, name string) error {
return r.Client.Delete(ctx, &node)
}

// ensureUpdatedPausedCondition updates the paused condition if needed.
func (r *ReconcileMachine) ensureUpdatedPausedCondition(ctx context.Context, m *machinev1.Machine, desiredCondition *machinev1.Condition, logMessage string) (bool, error) {
oldM := m.DeepCopy()
if !conditions.IsEquivalentTo(conditions.Get(m, PausedCondition), desiredCondition) {
klog.Info(logMessage)
conditions.Set(m, desiredCondition)
if err := r.updateStatus(ctx, m, ptr.Deref(m.Status.Phase, ""), nil, oldM.Status.Conditions); err != nil {
klog.Errorf("%v: error updating status: %v", oldM.Name, err)
return false, fmt.Errorf("error updating status for machine %s: %w", oldM.Name, err)
}

return true, nil
}

return false, nil
}

func delayIfRequeueAfterError(err error) (reconcile.Result, error) {
var requeueAfterError *RequeueAfterError
if errors.As(err, &requeueAfterError) {
Expand Down Expand Up @@ -538,6 +572,17 @@ func (r *ReconcileMachine) updateStatus(ctx context.Context, machine *machinev1.
return nil
}

func (r *ReconcileMachine) patchStatusAuthoritativeAPI(ctx context.Context, machine *machinev1.Machine, authoritativeAPI machinev1.MachineAuthority) error {
baseToPatch := client.MergeFrom(machine.DeepCopy())
machine.Status.AuthoritativeAPI = authoritativeAPI

if err := r.Client.Status().Patch(ctx, machine, baseToPatch); err != nil {
return fmt.Errorf("error patching machine status: %w", err)
}

return nil
}

func (r *ReconcileMachine) patchFailedMachineInstanceAnnotation(ctx context.Context, machine *machinev1.Machine) error {
baseToPatch := client.MergeFrom(machine.DeepCopy())
if machine.Annotations == nil {
Expand Down
53 changes: 21 additions & 32 deletions pkg/controller/machine/machine_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,20 +104,15 @@ var _ = Describe("Machine Reconciler", func() {
testutils.CleanupResources(Default, ctx, cfg, k8sClient, namespace.GetName(),
&machinev1.Machine{},
)

})

It("Should reconcile a Machine", func() {
instance := machineBuilder.Build()
instance := machineBuilder.WithAuthoritativeAPI(machinev1.MachineAuthorityMachineAPI).Build()

By("Creating the Machine")
Expect(k8sClient.Create(ctx, instance)).To(Succeed())

By("Setting the AuthoritativeAPI to MachineAPI")
Eventually(k.UpdateStatus(instance, func() {
instance.Status.AuthoritativeAPI = machinev1.MachineAuthorityMachineAPI
})).Should(Succeed())

By("Verying the Machine has been reconciled (status set)")
Eventually(k.Object(instance), timeout).Should(HaveField("Status", Not(Equal(machinev1.MachineStatus{}))))

// // Expect platform status is not empty. This (check) means we've called the
Expand All @@ -127,27 +122,22 @@ var _ = Describe("Machine Reconciler", func() {

})

Describe("Paused condition", func() {
Describe("Paused Condition", func() {
var instance *machinev1.Machine
BeforeEach(func() {
instance = machineBuilder.Build()
instance = machineBuilder.WithAuthoritativeAPI(machinev1.MachineAuthorityClusterAPI).Build()
})

It("Should set the Paused condition appropriately", func() {

By("Creating the Machine")
By("Creating the Machine (spec.authoritativeAPI=ClusterAPI)")
Expect(k8sClient.Create(ctx, instance)).To(Succeed())

By("Setting the AuthoritativeAPI to ClusterAPI")
Eventually(k.UpdateStatus(instance, func() {
instance.Status.AuthoritativeAPI = machinev1.MachineAuthorityClusterAPI
})).Should(Succeed())

By("Verifying the paused condition is approproately set to true")
By("Verifying the status.authoritativeAPI has been set to ClusterAPI and the paused condition is appropriately set to true")
Eventually(k.Object(instance), timeout).Should(SatisfyAll(
HaveField("Status.Conditions", ContainElement(SatisfyAll(
HaveField("Type", Equal(PausedCondition)),
HaveField("Status", Equal(corev1.ConditionTrue)),
HaveField("Message", Equal("The AuthoritativeAPI status is set to 'ClusterAPI'")),
))),
HaveField("Status.AuthoritativeAPI", Equal(machinev1.MachineAuthorityClusterAPI)),
))
Expand Down Expand Up @@ -189,36 +179,35 @@ var _ = Describe("Machine Reconciler", func() {
})
}()

By("Transitioning the AuthoritativeAPI though 'Migrating' to MachineAPI")
By("Changing status.authoritativeAPI from ClusterAPI to Migrating")
Eventually(k.UpdateStatus(instance, func() {
instance.Status.AuthoritativeAPI = machinev1.MachineAuthorityMigrating
})).Should(Succeed())

By("Updating the AuthoritativeAPI from Migrating to MachineAPI")
By("Verifying the paused condition is appropriately set to true while in Migrating")
Eventually(k.Object(instance), timeout).Should(SatisfyAll(
HaveField("Status.Conditions", ContainElement(SatisfyAll(
HaveField("Type", Equal(PausedCondition)),
HaveField("Status", Equal(corev1.ConditionTrue)),
HaveField("Message", Equal("The AuthoritativeAPI status is set to 'Migrating'")),
))),
HaveField("Status.AuthoritativeAPI", Equal(machinev1.MachineAuthorityMigrating)),
))

By("Changing status.authoritativeAPI from Migrating to MachineAPI")
Eventually(k.UpdateStatus(instance, func() {
instance.Status.AuthoritativeAPI = machinev1.MachineAuthorityMachineAPI
})).Should(Succeed())

By("Verifying the paused condition is approproately set to false")
By("Verifying the paused condition is appropriately set to false when in MachineAPI")
Eventually(k.Object(instance), timeout).Should(SatisfyAll(
HaveField("Status.Conditions", ContainElement(SatisfyAll(
HaveField("Type", Equal(PausedCondition)),
HaveField("Status", Equal(corev1.ConditionFalse)),
HaveField("Message", Equal("The AuthoritativeAPI status is set to 'MachineAPI'")),
))),
HaveField("Status.AuthoritativeAPI", Equal(machinev1.MachineAuthorityMachineAPI)),
))

By("Unsetting the AuthoritativeAPI field in the status")
Eventually(k.UpdateStatus(instance, func() {
instance.Status.AuthoritativeAPI = ""
})).Should(Succeed())

By("Verifying the paused condition is still approproately set to false")
Eventually(k.Object(instance), timeout).Should(HaveField("Status.Conditions", ContainElement(SatisfyAll(
HaveField("Type", Equal(PausedCondition)),
HaveField("Status", Equal(corev1.ConditionFalse)),
HaveField("Message", Equal("The AuthoritativeAPI is not set")),
))))
})
})
})
120 changes: 75 additions & 45 deletions pkg/controller/machineset/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,59 +174,61 @@ func (r *ReconcileMachineSet) Reconcile(ctx context.Context, request reconcile.R
}

if r.gate.Enabled(featuregate.Feature(openshiftfeatures.FeatureGateMachineAPIMigration)) {
machineSetName := machineSet.GetName()
machineSetCopy := machineSet.DeepCopy()
// Check Status.AuthoritativeAPI. If it's not set to MachineAPI. Set the
// paused condition true and return early.
//
// Once we have a webhook, we want to remove the check that the AuthoritativeAPI
// field is populated.
if machineSet.Status.AuthoritativeAPI != "" &&
machineSet.Status.AuthoritativeAPI != machinev1.MachineAuthorityMachineAPI {
conditions.Set(machineSetCopy, conditions.TrueConditionWithReason(
machine.PausedCondition,
machine.PausedConditionReason,
"The AuthoritativeAPI is set to %s", string(machineSet.Status.AuthoritativeAPI),
))

_, err := updateMachineSetStatus(r.Client, machineSet, machineSetCopy.Status)
if err != nil && !apierrors.IsNotFound(err) {
klog.Errorf("%v: error updating status: %v", machineSetName, err)
return reconcile.Result{}, fmt.Errorf("error updating status: %w", err)
} else if apierrors.IsNotFound(err) {
return reconcile.Result{}, nil
switch machineSet.Status.AuthoritativeAPI {
case "":
// An empty .status.authoritativeAPI normally means the resource has not yet been reconciled.
// and that the value in .spec.authoritativeAPI has not been propagated to .status.authoritativeAPI yet.
// This value can be set by two separate controllers, depending which one of them is running at that time,
// or in case they are both running, which one gets to set it first (the operation is idempotent so there is no harm in racing).
// - the cluster-capi-operator machine-api-migration's migration controller
// - this controller

klog.Infof("%v: machine set .status.authoritativeAPI is not yet set, setting it to .spec.authoritativeAPI", machineSet.Name)

if err := r.patchStatusAuthoritativeAPI(ctx, machineSet, machineSet.Spec.AuthoritativeAPI); err != nil {
klog.Errorf("%v: error patching status to set .status.authoritativeAPI for machine set: %v", machineSet.Name, err)
return reconcile.Result{}, fmt.Errorf("error patching status to set .status.authoritativeAPI for machine set %s: %w", machineSet.Name, err)
}

klog.Infof("%v: machine set is paused, taking no further action", machineSetName)
// Return to give a chance to the changes to get propagated.
return reconcile.Result{}, nil
}

var pausedFalseReason string
if machineSet.Status.AuthoritativeAPI != "" {
pausedFalseReason = fmt.Sprintf("The AuthoritativeAPI is set to %s", string(machineSet.Status.AuthoritativeAPI))
} else {
pausedFalseReason = "The AuthoritativeAPI is not set"
}
case machinev1.MachineAuthorityClusterAPI, machinev1.MachineAuthorityMigrating:
// In case .status.authoritativeAPI is set to machinev1.MachineAuthorityClusterAPI, machinev1.MachineAuthorityMigrating
// the resource should be paused and not reconciled further.
desiredCondition := conditions.TrueConditionWithReason(
machine.PausedCondition, machine.PausedConditionReason,
"The AuthoritativeAPI status is set to '%s'", string(machineSet.Status.AuthoritativeAPI),
)

if _, err := r.ensureUpdatedPausedCondition(machineSet, desiredCondition,
fmt.Sprintf("%v: machine set .status.authoritativeAPI is set to '%s', ensuring machine set is paused", machineSet.Name, machineSet.Status.AuthoritativeAPI)); err != nil {
return reconcile.Result{}, fmt.Errorf("failed to ensure paused condition: %w", err)
}

// Set the paused condition to false, continue reconciliation
conditions.Set(machineSetCopy, conditions.FalseCondition(
machine.PausedCondition,
machine.NotPausedConditionReason,
machinev1.ConditionSeverityInfo,
"%s",
pausedFalseReason,
))
klog.Infof("%v: machine set is paused, taking no further action", machineSet.Name)

var err error
machineSet, err = updateMachineSetStatus(r.Client, machineSet, machineSetCopy.Status)
if err != nil && !apierrors.IsNotFound(err) {
klog.Errorf("%v: error updating status: %v", machineSetName, err)
return reconcile.Result{}, fmt.Errorf("error updating status: %w", err)
} else if apierrors.IsNotFound(err) {
return reconcile.Result{}, nil
}

klog.Infof("%v: setting paused to false and continuing reconcile", machineSetName)
case machinev1.MachineAuthorityMachineAPI:
// The authority is MachineAPI and the resource should not be paused.
desiredCondition := conditions.FalseCondition(
machine.PausedCondition, machine.NotPausedConditionReason, machinev1.ConditionSeverityInfo, "%s",
fmt.Sprintf("The AuthoritativeAPI status is set to '%s'", string(machineSet.Status.AuthoritativeAPI)),
)

if updated, err := r.ensureUpdatedPausedCondition(machineSet, desiredCondition,
fmt.Sprintf("%v: machine set .status.authoritativeAPI is set to '%s', unpausing machine set", machineSet.Name, machineSet.Status.AuthoritativeAPI)); err != nil {
return reconcile.Result{}, fmt.Errorf("failed to ensure paused condition: %w", err)
} else if updated {
klog.Infof("%v: setting machine set paused condition to false", machineSet.Name)
}

// Fallthrough and continue reconciliation.
default:
klog.Errorf("%v: invalid .status.authoritativeAPI '%s'", machineSet.Name, machineSet.Status.AuthoritativeAPI)
return reconcile.Result{}, nil // Do not return an error to avoid immediate requeue.
}
}

result, err := r.reconcile(ctx, machineSet)
Expand Down Expand Up @@ -329,6 +331,23 @@ func (r *ReconcileMachineSet) reconcile(ctx context.Context, machineSet *machine
return reconcile.Result{}, nil
}

// ensureUpdatedPausedCondition updates the paused condition if needed.
func (r *ReconcileMachineSet) ensureUpdatedPausedCondition(ms *machinev1.MachineSet, desiredCondition *machinev1.Condition, logMessage string) (bool, error) {
newMs := ms.DeepCopy()
if !conditions.IsEquivalentTo(conditions.Get(ms, machine.PausedCondition), desiredCondition) {
klog.Info(logMessage)
conditions.Set(newMs, desiredCondition)
if _, err := updateMachineSetStatus(r.Client, ms, newMs.Status); err != nil {
klog.Errorf("%v: error updating status: %v", newMs.Name, err)
return false, fmt.Errorf("error updating status for machine set %s: %w", newMs.Name, err)
}

return true, nil
}

return false, nil
}

// syncReplicas essentially scales machine resources up and down.
func (r *ReconcileMachineSet) syncReplicas(ms *machinev1.MachineSet, machines []*machinev1.Machine) error {
if ms.Spec.Replicas == nil {
Expand Down Expand Up @@ -500,6 +519,17 @@ func (r *ReconcileMachineSet) waitForMachineDeletion(machineList []*machinev1.Ma
return nil
}

func (r *ReconcileMachineSet) patchStatusAuthoritativeAPI(ctx context.Context, machineSet *machinev1.MachineSet, authoritativeAPI machinev1.MachineAuthority) error {
baseToPatch := client.MergeFrom(machineSet.DeepCopy())
machineSet.Status.AuthoritativeAPI = authoritativeAPI

if err := r.Client.Status().Patch(ctx, machineSet, baseToPatch); err != nil {
return fmt.Errorf("error patching machine set status: %w", err)
}

return nil
}

func validateMachineset(m *machinev1.MachineSet) field.ErrorList {
errors := field.ErrorList{}

Expand Down
Loading