Skip to content

OCPCLOUD-2986,OCPBUGS-56849: fix: controllers: guard on empty .status.authoritativeAPI #1380

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
Jun 27, 2025
Merged

OCPCLOUD-2986,OCPBUGS-56849: fix: controllers: guard on empty .status.authoritativeAPI #1380

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
96 changes: 65 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
// by the migration controller, which has the responsibility for propagating .spec.authoritativeAPI to the status.
// Pause the resource and take no further action but return until that field is populated.
desiredCondition := conditions.TrueConditionWithReason(
PausedCondition, PausedConditionReason,
"The AuthoritativeAPI status is not yet set",
)

if _, err := r.ensureUpdatedPausedCondition(ctx, m, desiredCondition,
fmt.Sprintf("%v: machine .status.authoritativeAPI is not yet set, ensuring machine is paused", machineName)); 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.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)
}

// 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)
klog.Infof("%v: machine is paused, taking no further action", machineName)

return reconcile.Result{}, nil

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)
}

// 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
39 changes: 27 additions & 12 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 @@ -135,19 +135,20 @@ var _ = Describe("Machine Reconciler", func() {

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

By("Creating the Machine")
By("Creating the Machine (empty status.authoritativeAPI)")
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 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 +190,50 @@ 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")
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")
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")
By("Changing status.authoritativeAPI from MachineAPI to empty")
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")),
))))
By("Verifying the paused condition is still appropriately set to true when empty status.authoritativeAPI")
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 not yet set")),
))),
HaveField("Status.AuthoritativeAPI", BeEquivalentTo("")),
))
})
})
})
108 changes: 63 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,60 @@ 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
// by the migration controller, which has the responsibility for propagating .spec.authoritativeAPI to the status.
// Pause the resource and take no further action but return until that field is populated.
desiredCondition := conditions.TrueConditionWithReason(
machine.PausedCondition, machine.PausedConditionReason,
"The AuthoritativeAPI status is not yet set",
)

if _, err := r.ensureUpdatedPausedCondition(machineSet, desiredCondition,
fmt.Sprintf("%v: machine set .status.authoritativeAPI is not yet set, ensuring machine set is paused", machineSet.Name)); err != nil {
return reconcile.Result{}, fmt.Errorf("failed to ensure paused condition: %w", err)
}
klog.Infof("%v: machine set is paused, taking no further action", machineSet.Name)

klog.Infof("%v: machine set is paused, taking no further action", machineSetName)
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 +330,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
Loading