From 88a744dab443c7eeac3d58489033b8a6d9a68b35 Mon Sep 17 00:00:00 2001 From: Matthew Booth Date: Wed, 13 May 2026 11:04:23 +0100 Subject: [PATCH] Enable actuators to set terminal failure on Exists and Update An actuator can already mutate its Machine argument to specificy an explicit Failed phase. However, because we don't also pass the error, updateStatus() cannot correctly set ErrorMessage and ErrorReason. --- pkg/controller/machine/controller.go | 4 +- pkg/controller/machine/controller_test.go | 59 +++++++++++++++++++++-- pkg/controller/machine/testactuator.go | 38 +++++++++------ 3 files changed, 82 insertions(+), 19 deletions(-) diff --git a/pkg/controller/machine/controller.go b/pkg/controller/machine/controller.go index ab2cd5c633..ed8a0379a2 100644 --- a/pkg/controller/machine/controller.go +++ b/pkg/controller/machine/controller.go @@ -346,7 +346,7 @@ func (r *ReconcileMachine) Reconcile(ctx context.Context, request reconcile.Requ "Failed to check if machine exists: %v", err, )) - if patchErr := r.updateStatus(ctx, m, ptr.Deref(m.Status.Phase, ""), nil, originalConditions); patchErr != nil { + if patchErr := r.updateStatus(ctx, m, ptr.Deref(m.Status.Phase, ""), err, originalConditions); patchErr != nil { klog.Errorf("%v: error patching status: %v", machineName, patchErr) } @@ -358,7 +358,7 @@ func (r *ReconcileMachine) Reconcile(ctx context.Context, request reconcile.Requ if err := r.actuator.Update(ctx, m); err != nil { klog.Errorf("%v: error updating machine: %v, retrying in %v seconds", machineName, err, requeueAfter) - if patchErr := r.updateStatus(ctx, m, ptr.Deref(m.Status.Phase, ""), nil, originalConditions); patchErr != nil { + if patchErr := r.updateStatus(ctx, m, ptr.Deref(m.Status.Phase, ""), err, originalConditions); patchErr != nil { klog.Errorf("%v: error patching status: %v", machineName, patchErr) } diff --git a/pkg/controller/machine/controller_test.go b/pkg/controller/machine/controller_test.go index 0b29b69b05..b6ad6993d9 100644 --- a/pkg/controller/machine/controller_test.go +++ b/pkg/controller/machine/controller_test.go @@ -370,11 +370,16 @@ func TestReconcileRequest(t *testing.T) { result reconcile.Result error bool phase string + errorMessage *string } testCases := []struct { - request reconcile.Request - existsValue bool - expected expected + request reconcile.Request + existsValue bool + existsErr error + updateErr error + existsMachineHook func(*machinev1.Machine) + updateMachineHook func(*machinev1.Machine) + expected expected }{ { request: reconcile.Request{NamespacedName: types.NamespacedName{Name: machineNoPhase.Name, Namespace: machineNoPhase.Namespace}}, @@ -532,6 +537,42 @@ func TestReconcileRequest(t *testing.T) { phase: machinev1.PhaseRunning, }, }, + { + request: reconcile.Request{NamespacedName: types.NamespacedName{Name: machineProvisioning.Name, Namespace: machineProvisioning.Namespace}}, + existsValue: false, + existsErr: errors.New("actuator exists error"), + existsMachineHook: func(m *machinev1.Machine) { + m.Status.Phase = ptr.To(machinev1.PhaseFailed) + }, + expected: expected{ + createCallCount: 0, + existCallCount: 1, + updateCallCount: 0, + deleteCallCount: 0, + result: reconcile.Result{}, + error: true, + phase: machinev1.PhaseFailed, + errorMessage: ptr.To("actuator exists error"), + }, + }, + { + request: reconcile.Request{NamespacedName: types.NamespacedName{Name: machineProvisioned.Name, Namespace: machineProvisioned.Namespace}}, + existsValue: true, + updateErr: errors.New("actuator update error"), + updateMachineHook: func(m *machinev1.Machine) { + m.Status.Phase = ptr.To(machinev1.PhaseFailed) + }, + expected: expected{ + createCallCount: 0, + existCallCount: 1, + updateCallCount: 1, + deleteCallCount: 0, + result: reconcile.Result{RequeueAfter: requeueAfter}, + error: false, + phase: machinev1.PhaseFailed, + errorMessage: ptr.To("actuator update error"), + }, + }, } for _, tc := range testCases { @@ -542,6 +583,10 @@ func TestReconcileRequest(t *testing.T) { } act := newTestActuator() act.ExistsValue = tc.existsValue + act.ExistsErr = tc.existsErr + act.UpdateErr = tc.updateErr + act.ExistsMachineHook = tc.existsMachineHook + act.UpdateMachineHook = tc.updateMachineHook r := &ReconcileMachine{ Client: fake.NewClientBuilder().WithScheme(scheme.Scheme).WithRuntimeObjects( &machineNoPhase, @@ -598,6 +643,14 @@ func TestReconcileRequest(t *testing.T) { if tc.expected.phase != ptr.Deref(machine.Status.Phase, "") { t.Errorf("Case %s. Got: %v, expected: %v", tc.request.Name, ptr.Deref(machine.Status.Phase, ""), tc.expected.phase) } + + if tc.expected.errorMessage != nil { + if machine.Status.ErrorMessage == nil { + t.Errorf("Case %s. Expected errorMessage %q, got nil", tc.request.Name, *tc.expected.errorMessage) + } else if *machine.Status.ErrorMessage != *tc.expected.errorMessage { + t.Errorf("Case %s. Got errorMessage: %q, expected: %q", tc.request.Name, *machine.Status.ErrorMessage, *tc.expected.errorMessage) + } + } }) } } diff --git a/pkg/controller/machine/testactuator.go b/pkg/controller/machine/testactuator.go index 94917efdaa..b358416a9f 100644 --- a/pkg/controller/machine/testactuator.go +++ b/pkg/controller/machine/testactuator.go @@ -26,17 +26,21 @@ import ( var _ Actuator = &TestActuator{} type TestActuator struct { - unblock chan string - BlockOnCreate bool - BlockOnDelete bool - BlockOnUpdate bool - BlockOnExists bool - CreateCallCount int64 - DeleteCallCount int64 - UpdateCallCount int64 - ExistsCallCount int64 - ExistsValue bool - Lock sync.Mutex + unblock chan string + BlockOnCreate bool + BlockOnDelete bool + BlockOnUpdate bool + BlockOnExists bool + CreateCallCount int64 + DeleteCallCount int64 + UpdateCallCount int64 + ExistsCallCount int64 + ExistsValue bool + ExistsErr error + UpdateErr error + ExistsMachineHook func(*machinev1.Machine) + UpdateMachineHook func(*machinev1.Machine) + Lock sync.Mutex } func (a *TestActuator) Create(context.Context, *machinev1.Machine) error { @@ -74,10 +78,13 @@ func (a *TestActuator) Update(ctx context.Context, machine *machinev1.Machine) e a.Lock.Lock() defer a.Lock.Unlock() a.UpdateCallCount++ - return nil + if a.UpdateMachineHook != nil { + a.UpdateMachineHook(machine) + } + return a.UpdateErr } -func (a *TestActuator) Exists(context.Context, *machinev1.Machine) (bool, error) { +func (a *TestActuator) Exists(_ context.Context, machine *machinev1.Machine) (bool, error) { defer func() { if a.BlockOnExists { <-a.unblock @@ -87,7 +94,10 @@ func (a *TestActuator) Exists(context.Context, *machinev1.Machine) (bool, error) a.Lock.Lock() defer a.Lock.Unlock() a.ExistsCallCount++ - return a.ExistsValue, nil + if a.ExistsMachineHook != nil { + a.ExistsMachineHook(machine) + } + return a.ExistsValue, a.ExistsErr } func newTestActuator() *TestActuator {