diff --git a/pkg/util/provider/machinecontroller/machine.go b/pkg/util/provider/machinecontroller/machine.go index 844e2f7f53..1132e29dc2 100644 --- a/pkg/util/provider/machinecontroller/machine.go +++ b/pkg/util/provider/machinecontroller/machine.go @@ -76,7 +76,13 @@ func (c *controller) updateMachine(oldObj, newObj any) { } if oldMachine.Generation == newMachine.Generation { - klog.V(3).Infof("Skipping other non-spec updates for machine %q", oldMachine.Name) + // Finalizer changes increment resourceVersion but not Generation, so a machine that just + // had the MCM finalizer added would be silently dropped here. Re-enqueue it so + // reconcileClusterMachine is reached and the machine advances past the empty phase. + if !sets.NewString(oldMachine.Finalizers...).HasAll(newMachine.Finalizers...) { + klog.V(4).Infof("updateMachine: machine %q gained finalizer — re-enqueuing despite unchanged Generation", newMachine.Name) + c.enqueueMachine(newObj, "handling machine finalizer UPDATE event") + } return } diff --git a/pkg/util/provider/machinecontroller/machine_test.go b/pkg/util/provider/machinecontroller/machine_test.go index 0704f5e34d..d8a76bf9d4 100644 --- a/pkg/util/provider/machinecontroller/machine_test.go +++ b/pkg/util/provider/machinecontroller/machine_test.go @@ -6,18 +6,22 @@ package controller import ( "context" + "errors" "fmt" "math" "time" taintutils "github.com/gardener/machine-controller-manager/pkg/util/taints" k8stesting "k8s.io/client-go/testing" + "k8s.io/client-go/tools/cache" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/util/validation/field" machineapi "github.com/gardener/machine-controller-manager/pkg/apis/machine" @@ -29,6 +33,7 @@ import ( "github.com/gardener/machine-controller-manager/pkg/util/provider/machinecodes/codes" "github.com/gardener/machine-controller-manager/pkg/util/provider/machinecodes/status" "github.com/gardener/machine-controller-manager/pkg/util/provider/machineutils" + "github.com/gardener/machine-controller-manager/pkg/util/worker" ) const testNamespace = "test" @@ -5049,4 +5054,70 @@ var _ = Describe("machine", func() { }), ) }) + + // Regression test for https://github.com/gardener/machine-controller-manager/issues/1141 + // A new Machine can get permanently stuck with empty phase when the worker retry budget is + // exhausted by 409 Conflict errors from addMachineFinalizers. + Describe("#reconcileClusterMachineKey", func() { + It("machine should have finalizer even after MaxRetries 409 conflicts on finalizer addition", func() { + stop := make(chan struct{}) + defer close(stop) + + // Machine with no finalizer and empty phase — as created by MachineSet + machine := &v1alpha1.Machine{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-machine", + Namespace: testNamespace, + }, + } + + c, trackers := createController(stop, testNamespace, []runtime.Object{machine}, nil, nil, nil, false) + defer trackers.Stop() + waitForCacheSync(stop, c) + + // Inject 409 Conflict on the first DefaultMaxRetries finalizer Update calls, then allow + // subsequent calls to succeed. + conflictErr := apierrors.NewConflict( + schema.GroupResource{Group: "machine.sapcloud.io", Resource: "machines"}, + machine.Name, + errors.New("the object has been modified; please apply your changes to the latest version and try again"), + ) + conflictCallCount := 0 + fakeClient := c.controlMachineClient.(*fakemachineapi.FakeMachineV1alpha1) + fakeClient.Fake.PrependReactor("update", "machines", func(action k8stesting.Action) (bool, runtime.Object, error) { + // Allow UpdateStatus (subresource "status") through unconditionally. + if action.GetSubresource() == "status" { + return false, nil, nil + } + // Return a 409 for the first DefaultMaxRetries finalizer updates, then pass through. + if conflictCallCount < worker.DefaultMaxRetries { + conflictCallCount++ + return true, nil, conflictErr + } + return false, nil, nil + }) + + key := cache.MetaObjectToName(machine).String() + + // Run DefaultMaxRetries+5 iterations — without the fix this would exhaust the worker's + // retry budget on the first DefaultMaxRetries calls and permanently strand the machine. + for i := 0; i < worker.DefaultMaxRetries+5; i++ { + _ = c.reconcileClusterMachineKey(key) + } + + // Sync the lister so it reflects what the fake API server now holds. + waitForCacheSync(stop, c) + + // The machine must have the finalizer; without the fix it would not (the key would have + // been dropped from the queue before the Update could succeed). + updatedMachine, err := c.controlMachineClient.Machines(testNamespace).Get( + context.TODO(), machine.Name, metav1.GetOptions{}, + ) + Expect(err).NotTo(HaveOccurred()) + Expect(updatedMachine.Finalizers).To( + ContainElement(MCMFinalizerName), + "machine must have the MCM finalizer — 409 conflict errors must not prevent finalizer addition", + ) + }) + }) }) diff --git a/pkg/util/provider/machinecontroller/machine_util.go b/pkg/util/provider/machinecontroller/machine_util.go index d34092e33e..fba1429214 100644 --- a/pkg/util/provider/machinecontroller/machine_util.go +++ b/pkg/util/provider/machinecontroller/machine_util.go @@ -1197,6 +1197,12 @@ func (c *controller) addMachineFinalizers(ctx context.Context, machine *v1alpha1 clone.Finalizers = finalizers.List() _, err := c.controlMachineClient.Machines(clone.Namespace).Update(ctx, clone, metav1.UpdateOptions{}) if err != nil { + if apierrors.IsConflict(err) { + // Informer cache is stale; the watch event will re-enqueue the machine once the + // cache catches up, so this is not a retriable error from the worker's perspective. + // Returning nil avoids burning a retry slot from the fixed budget (DefaultMaxRetries). + return machineutils.ShortRetry, nil + } // Keep retrying until update goes through klog.Errorf("Failed to add finalizers for machine %q: %s", machine.Name, err) } else { diff --git a/pkg/util/worker/worker.go b/pkg/util/worker/worker.go index 87b52ff95a..ed797c4116 100644 --- a/pkg/util/worker/worker.go +++ b/pkg/util/worker/worker.go @@ -59,7 +59,7 @@ func worker(queue workqueue.TypedRateLimitingInterface[string], resourceType str return false } - klog.V(4).Infof("Dropping %s %q out of the queue: %v", resourceType, key, err) + klog.V(3).Infof("Dropping %s %q out of the queue after %d retries: %v", resourceType, key, maxRetries, err) queue.Forget(key) return false }()