Skip to content

OCPBUGS-58230: Revert: fix: controllers: guard on empty .status.authoritativeAPI #1383

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

OCPBUGS-58230: Revert: fix: controllers: guard on empty .status.authoritativeAPI #1383

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: 31 additions & 65 deletions pkg/controller/machine/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,60 +177,43 @@ func (r *ReconcileMachine) Reconcile(ctx context.Context, request reconcile.Requ
originalConditions := conditions.DeepCopyConditions(m.Status.Conditions)

if r.gate.Enabled(featuregate.Feature(openshiftfeatures.FeatureGateMachineAPIMigration)) {
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

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

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

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

Expand Down Expand Up @@ -450,23 +433,6 @@ 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: 12 additions & 27 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,20 +135,19 @@ var _ = Describe("Machine Reconciler", func() {

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

By("Creating the Machine (empty status.authoritativeAPI)")
By("Creating the Machine")
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 appropriately set to true")
By("Verifying the paused condition is approproately 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 @@ -190,50 +189,36 @@ var _ = Describe("Machine Reconciler", func() {
})
}()

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

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")
By("Updating the AuthoritativeAPI from Migrating to MachineAPI")
Eventually(k.UpdateStatus(instance, func() {
instance.Status.AuthoritativeAPI = machinev1.MachineAuthorityMachineAPI
})).Should(Succeed())

By("Verifying the paused condition is appropriately set to false")
By("Verifying the paused condition is approproately 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("Changing status.authoritativeAPI from MachineAPI to empty")
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 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("")),
))
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")),
))))
})
})
})
108 changes: 45 additions & 63 deletions pkg/controller/machineset/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,60 +174,59 @@ func (r *ReconcileMachineSet) Reconcile(ctx context.Context, request reconcile.R
}

if r.gate.Enabled(featuregate.Feature(openshiftfeatures.FeatureGateMachineAPIMigration)) {
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)
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
}
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
}

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

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

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

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

klog.Infof("%v: setting paused to false and continuing reconcile", machineSetName)
}

result, err := r.reconcile(ctx, machineSet)
Expand Down Expand Up @@ -330,23 +329,6 @@ 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