From 19bf23605e6b7f901a3800d5fdf222325d6eda10 Mon Sep 17 00:00:00 2001 From: Mat Kowalski Date: Thu, 20 Aug 2026 12:32:21 +0200 Subject: [PATCH 1/3] OCPBUGS-100316: vsphere: recover machines when the clone TaskRef is lost The vSphere actuator permanently wedged a machine when the status patch that persists the clone TaskRef failed (for example, a transient admission-webhook denial during install): - Actuator.Create cached the TaskRef before PatchMachine. When the patch was denied, the in-memory TaskIDCache kept the ref while the Machine object never received it, so the staleness guard in Create requeued forever ("machine object missing expected provider task ID"). The cache is only cleared by Update()/Delete(), which never run because exists() keeps returning false, so the machine was stuck in Provisioning. - reconciler.create() only recovered a lost TaskRef when InstanceState was already PoweredOff; otherwise it re-cloned, risking duplicate VMs. Fix both sides: - Cache the TaskRef only after PatchMachine succeeds, so a denied patch cannot leave a phantom cache entry that wedges every future reconcile. - In create(), look the VM up in vCenter before cloning. If it already exists we adopt it and power it on to recover; otherwise we clone the template. This makes create() idempotent and prevents duplicate VMs. Any transient patch failure during creation now self-heals on the next reconcile instead of leaving workers permanently stuck in Provisioning. Assisted-By: Claude Opus 4.6 --- pkg/controller/vsphere/actuator.go | 15 ++- pkg/controller/vsphere/actuator_test.go | 133 ++++++++++++++++++++++ pkg/controller/vsphere/reconciler.go | 42 ++++--- pkg/controller/vsphere/reconciler_test.go | 79 +++++++++++++ 4 files changed, 251 insertions(+), 18 deletions(-) diff --git a/pkg/controller/vsphere/actuator.go b/pkg/controller/vsphere/actuator.go index 6b9aad746..14b0aec0a 100644 --- a/pkg/controller/vsphere/actuator.go +++ b/pkg/controller/vsphere/actuator.go @@ -97,10 +97,6 @@ func (a *Actuator) Create(ctx context.Context, machine *machinev1.Machine) error var retErr error err = newReconciler(scope).create() - // save the taskRef in our cache in case of any error with patch. - if scope.providerStatus.TaskRef != "" { - a.TaskIDCache[machine.Name] = scope.providerStatus.TaskRef - } if err != nil { fmtErr := fmt.Errorf(reconcilerFailFmt, machine.GetName(), createEventAction, err) retErr = a.handleMachineError(machine, fmtErr, createEventAction) @@ -112,6 +108,17 @@ func (a *Actuator) Create(ctx context.Context, machine *machinev1.Machine) error return err } + // Only cache the taskRef once it has been durably persisted on the Machine + // object. Caching it before a successful patch can permanently wedge + // creation: if the patch fails (for example, a transient admission-webhook + // denial) the object never receives the taskRef and the staleness guard + // above would requeue forever. create() is idempotent (it looks the VM up + // in vCenter before cloning), so a lost taskRef is recovered on the next + // reconcile rather than re-cloning. + if scope.providerStatus.TaskRef != "" { + a.TaskIDCache[machine.Name] = scope.providerStatus.TaskRef + } + return retErr } diff --git a/pkg/controller/vsphere/actuator_test.go b/pkg/controller/vsphere/actuator_test.go index 38118eed5..dc871ff50 100644 --- a/pkg/controller/vsphere/actuator_test.go +++ b/pkg/controller/vsphere/actuator_test.go @@ -13,14 +13,18 @@ import ( . "github.com/onsi/gomega" configv1 "github.com/openshift/api/config/v1" machinev1 "github.com/openshift/api/machine/v1beta1" + "github.com/vmware/govmomi/object" "github.com/vmware/govmomi/simulator" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/kubernetes/scheme" + "k8s.io/client-go/tools/events" ipamv1beta1 "sigs.k8s.io/cluster-api/api/ipam/v1beta1" //nolint:staticcheck "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" "sigs.k8s.io/controller-runtime/pkg/envtest" "sigs.k8s.io/controller-runtime/pkg/manager" metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server" @@ -422,3 +426,132 @@ func TestMachineEvents(t *testing.T) { }) } } + +// TestActuatorCreateCachesTaskRefOnlyAfterSuccessfulPatch verifies the actuator +// records a machine's clone TaskRef in its in-memory cache only after that ref +// has been durably persisted. A denied status patch must not leave a phantom +// cache entry, which previously wedged every subsequent reconcile via the +// staleness guard in Create (OCPBUGS-100316). +func TestActuatorCreateCachesTaskRefOnlyAfterSuccessfulPatch(t *testing.T) { + model, session, server := initSimulator(t) + defer model.Remove() + defer server.Close() + + host, port, err := net.SplitHostPort(server.URL.Host) + if err != nil { + t.Fatal(err) + } + + credentialsSecretUsername := fmt.Sprintf("%s.username", host) + credentialsSecretPassword := fmt.Sprintf("%s.password", host) + password, _ := server.URL.User.Password() + namespace := "test" + + vm := model.Map().Any("VirtualMachine").(*simulator.VirtualMachine) + vm.Config.Version = minimumHWVersionString + + credentialsSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: namespace}, + Data: map[string][]byte{ + credentialsSecretUsername: []byte(server.URL.User.Username()), + credentialsSecretPassword: []byte(password), + }, + } + configMap := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Name: OpenshiftConfigManagedConfigMap, Namespace: openshiftConfigNamespaceForTest}, + Data: map[string]string{OpenshiftConfigManagedCloudConfigKey: fmt.Sprintf(testConfigFmt, port, "test", namespace)}, + } + userDataSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: "vsphere-ignition", Namespace: namespace}, + Data: map[string][]byte{userDataSecretKey: []byte("{}")}, + } + + newMachine := func(name string) *machinev1.Machine { + providerSpec, err := RawExtensionFromProviderSpec(&machinev1.VSphereMachineProviderSpec{ + Template: vm.Name, + Workspace: &machinev1.Workspace{Server: host}, + CredentialsSecret: &corev1.LocalObjectReference{Name: "test"}, + UserDataSecret: &corev1.LocalObjectReference{Name: "vsphere-ignition"}, + DiskGiB: 10, + }) + if err != nil { + t.Fatal(err) + } + return &machinev1.Machine{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: namespace, + Labels: map[string]string{machinev1.MachineClusterIDLabel: "CLUSTERID"}, + }, + Spec: machinev1.MachineSpec{ProviderSpec: machinev1.ProviderSpec{Value: providerSpec}}, + Status: machinev1.MachineStatus{}, + } + } + + gates, err := testutils.NewDefaultMutableFeatureGate() + if err != nil { + t.Fatalf("unexpected error setting up feature gates: %v", err) + } + + t.Run("denied status patch does not cache the taskRef", func(t *testing.T) { + g := NewWithT(t) + machine := newMachine("patch-denied") + + base := fake.NewClientBuilder().WithScheme(scheme.Scheme). + WithStatusSubresource(machine). + WithRuntimeObjects(credentialsSecret, configMap, userDataSecret, machine). + Build() + denyStatusPatch := interceptor.NewClient(base, interceptor.Funcs{ + SubResourcePatch: func(_ context.Context, _ client.Client, _ string, _ client.Object, _ client.Patch, _ ...client.SubResourcePatchOption) error { + return fmt.Errorf("admission webhook denied the request") + }, + }) + + taskIDCache := map[string]string{} + actuator := NewActuator(ActuatorParams{ + Client: denyStatusPatch, + APIReader: denyStatusPatch, + EventRecorder: events.NewFakeRecorder(10), + TaskIDCache: taskIDCache, + OpenshiftConfigNamespace: openshiftConfigNamespaceForTest, + FeatureGates: gates, + }) + + err := actuator.Create(context.Background(), machine) + g.Expect(err).To(HaveOccurred()) + // The lost taskRef must not be cached; otherwise the staleness guard in + // Create would requeue forever once the object never receives it. + g.Expect(taskIDCache).ToNot(HaveKey(machine.Name)) + }) + + t.Run("successful patch caches the taskRef", func(t *testing.T) { + g := NewWithT(t) + machine := newMachine("patch-ok") + + c := fake.NewClientBuilder().WithScheme(scheme.Scheme). + WithStatusSubresource(machine). + WithRuntimeObjects(credentialsSecret, configMap, userDataSecret, machine). + Build() + + taskIDCache := map[string]string{} + actuator := NewActuator(ActuatorParams{ + Client: c, + APIReader: c, + EventRecorder: events.NewFakeRecorder(10), + TaskIDCache: taskIDCache, + OpenshiftConfigNamespace: openshiftConfigNamespaceForTest, + FeatureGates: gates, + }) + + g.Expect(actuator.Create(context.Background(), machine)).To(Succeed()) + g.Expect(taskIDCache).To(HaveKey(machine.Name)) + g.Expect(taskIDCache[machine.Name]).ToNot(BeEmpty()) + + // Wait on the async clone task so the simulator is not torn down early. + moTask, err := session.GetTask(context.TODO(), taskIDCache[machine.Name]) + g.Expect(err).ToNot(HaveOccurred()) + if moTask != nil { + g.Expect(object.NewTask(session.Client.Client, moTask.Reference()).Wait(context.TODO())).To(Succeed()) + } + }) +} diff --git a/pkg/controller/vsphere/reconciler.go b/pkg/controller/vsphere/reconciler.go index 7af805f43..3a50fdb5e 100644 --- a/pkg/controller/vsphere/reconciler.go +++ b/pkg/controller/vsphere/reconciler.go @@ -130,39 +130,53 @@ func (r *Reconciler) create() error { return fmt.Errorf("%v: not connected to a vCenter", r.machine.GetName()) } - // Attempt to power on instance in situation where we alredy cloned the instance and lost taskRef. - klog.V(4).Infof("%v: InstanceState is: %q", r.machine.GetName(), ptr.Deref(r.machineScope.providerStatus.InstanceState, "")) - if types.VirtualMachinePowerState(ptr.Deref(r.machineScope.providerStatus.InstanceState, "")) == types.VirtualMachinePowerStatePoweredOff { - klog.Infof("Powering on cloned machine without taskID: %v", r.machine.Name) + // A missing TaskRef usually means the VM has not been cloned yet. It can + // also mean we cloned the VM successfully but lost the TaskRef because + // the status patch that would have persisted it failed (for example, a + // transient admission-webhook denial during install). Look the VM up + // directly in vCenter before cloning so that a lost TaskRef never + // results in a duplicate VM: if the VM already exists we adopt it and + // power it on, otherwise we clone the template. + if _, err := findVM(r.machineScope); err != nil { + if !isNotFound(err) { + metrics.RegisterFailedInstanceCreate(&metrics.MachineLabels{ + Name: r.machine.Name, + Namespace: r.machine.Namespace, + Reason: "FindVM finished with error", + }) + return err + } - task, err := powerOn(r.machineScope) + klog.Infof("%v: cloning", r.machine.GetName()) + task, err := clone(r.machineScope) if err != nil { metrics.RegisterFailedInstanceCreate(&metrics.MachineLabels{ Name: r.machine.Name, Namespace: r.machine.Namespace, - Reason: "PowerOn task finished with error", + Reason: "Clone task finished with error", }) - conditionFailed := conditionFailed() conditionFailed.Message = err.Error() statusError := setProviderStatus(task, conditionFailed, r.machineScope, nil) if statusError != nil { return fmt.Errorf("failed to set provider status: %w", err) } - - return fmt.Errorf("%v: failed to power on machine: %w", r.machine.GetName(), err) + return err } - return setProviderStatus(task, conditionSuccess(), r.machineScope, nil) } - klog.Infof("%v: cloning", r.machine.GetName()) - task, err := clone(r.machineScope) + // The VM already exists but we have no TaskRef for it: we cloned it + // previously and lost the TaskRef. Recover by powering it on and + // recording the power-on task so subsequent reconciles can track it, + // instead of requeueing forever. + klog.Infof("%v: VM already exists without a persisted taskRef, powering on to recover", r.machine.GetName()) + task, err := powerOn(r.machineScope) if err != nil { metrics.RegisterFailedInstanceCreate(&metrics.MachineLabels{ Name: r.machine.Name, Namespace: r.machine.Namespace, - Reason: "Clone task finished with error", + Reason: "PowerOn task finished with error", }) conditionFailed := conditionFailed() conditionFailed.Message = err.Error() @@ -170,7 +184,7 @@ func (r *Reconciler) create() error { if statusError != nil { return fmt.Errorf("failed to set provider status: %w", err) } - return err + return fmt.Errorf("%v: failed to power on machine: %w", r.machine.GetName(), err) } return setProviderStatus(task, conditionSuccess(), r.machineScope, nil) } diff --git a/pkg/controller/vsphere/reconciler_test.go b/pkg/controller/vsphere/reconciler_test.go index 3fc67d993..645e5feed 100644 --- a/pkg/controller/vsphere/reconciler_test.go +++ b/pkg/controller/vsphere/reconciler_test.go @@ -2945,6 +2945,85 @@ func waitForTaskToComplete(session *session.Session, reconciler *Reconciler) err return nil } +// TestCreateRecoversLostTaskRef verifies that create() recovers a VM that was +// cloned but whose TaskRef was never persisted (for example, because the status +// patch was denied by an admission webhook during install). Instead of cloning +// a second VM, create() must find the existing VM and power it on. This is the +// provider-side defense for OCPBUGS-100316. +func TestCreateRecoversLostTaskRef(t *testing.T) { + g := NewWithT(t) + + // Autostart=false leaves the simulator VMs powered off, mimicking a VM that + // was cloned but never powered on. + poweredOff := func(m *simulator.Model) { m.Autostart = false } + model, server := initSimulatorCustom(t, poweredOff) + session := getSimulatorSession(t, server) + defer model.Remove() + defer server.Close() + + host, _, err := net.SplitHostPort(server.URL.Host) + g.Expect(err).ToNot(HaveOccurred()) + + vms := model.Map().All("VirtualMachine") + g.Expect(vms).ToNot(BeEmpty()) + existingVM := vms[0].(*simulator.VirtualMachine) + vmCountBefore := len(vms) + + provisioning := string(machinev1.PhaseProvisioning) + machineObj := &machinev1.Machine{ + ObjectMeta: metav1.ObjectMeta{ + Name: existingVM.Name, + Namespace: "test", + Labels: map[string]string{machinev1.MachineClusterIDLabel: "CLUSTERID"}, + // The machine UID matches the VM instance UUID so findVM adopts the + // already-cloned VM instead of cloning a new one. + UID: apimachinerytypes.UID(existingVM.Config.InstanceUuid), + }, + Status: machinev1.MachineStatus{Phase: &provisioning}, + } + + machineScope := &machineScope{ + Context: context.TODO(), + machine: machineObj, + machineToBePatched: runtimeclient.MergeFrom(machineObj.DeepCopy()), + providerSpec: &machinev1.VSphereMachineProviderSpec{ + Template: existingVM.Name, + Workspace: &machinev1.Workspace{Server: host}, + }, + session: session, + // No TaskRef and no InstanceState: the reference to the clone task was lost. + providerStatus: &machinev1.VSphereMachineProviderStatus{}, + client: fake.NewClientBuilder().WithScheme(scheme.Scheme).WithRuntimeObjects(machineObj).WithStatusSubresource(machineObj).Build(), + } + + reconciler := newReconciler(machineScope) + + g.Expect(reconciler.create()).To(Succeed()) + + // A recovery task must have been recorded rather than requeueing forever. + g.Expect(reconciler.providerStatus.TaskRef).ToNot(BeEmpty(), "expected a recovery power-on task to be recorded") + + // No new VM must have been cloned. + g.Expect(model.Map().All("VirtualMachine")).To(HaveLen(vmCountBefore), "create() must not clone a duplicate VM when one already exists") + + // The recovery task must be a power-on (not a clone) and must succeed. + g.Expect(waitForTaskToComplete(session, reconciler)).To(Succeed()) + moTask, err := session.GetTask(context.TODO(), reconciler.providerStatus.TaskRef) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(moTask).ToNot(BeNil()) + g.Expect(moTask.Info.DescriptionId).ToNot(ContainSubstring(cloneVmTaskDescriptionId)) + + // The existing VM must now be powered on. + vmObj := &virtualMachine{ + Context: context.TODO(), + Obj: object.NewVirtualMachine(session.Client.Client, existingVM.Reference()), + Ref: existingVM.Reference(), + } + powerState, err := vmObj.getPowerState() + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(powerState).To(Equal(types.VirtualMachinePowerStatePoweredOn)) +} + func TestUpdate(t *testing.T) { model, session, server := initSimulator(t) defer model.Remove() From 302951f7eac5c7166b12fa71fcf2b1de9220225b Mon Sep 17 00:00:00 2001 From: Mat Kowalski Date: Thu, 20 Aug 2026 13:07:11 +0200 Subject: [PATCH 2/3] OCPBUGS-100316: vsphere: preserve clone task identity and VM group on recovery Addresses review feedback on the lost-TaskRef recovery. Preserve the clone task identity across a failed status patch. The actuator now caches the TaskRef even when PatchMachine fails, and the staleness guard seeds the cached ref and reconciles the in-flight task instead of requeueing forever. This fixes both the permanent wedge and a duplicate clone being submitted while the first clone is still in-flight (before the cloned VM is discoverable by findVM). Restore VM group membership during recovery. The existing-VM recovery path now runs modifyVMGroup(..., false) before power-on, mirroring the normal completed-clone path, so a recovered VM is not left outside its configured DRS host-affinity group. Regression tests: identity retention across a denied patch, no duplicate clone on retry while the clone is in-flight, and VM group membership restored on recovery. Assisted-By: Claude Opus 4.6 --- pkg/controller/vsphere/actuator.go | 48 ++++----- pkg/controller/vsphere/actuator_test.go | 115 +++++++++++++++++----- pkg/controller/vsphere/reconciler.go | 24 ++++- pkg/controller/vsphere/reconciler_test.go | 99 +++++++++++++++++++ 4 files changed, 231 insertions(+), 55 deletions(-) diff --git a/pkg/controller/vsphere/actuator.go b/pkg/controller/vsphere/actuator.go index 14b0aec0a..6e27b7fce 100644 --- a/pkg/controller/vsphere/actuator.go +++ b/pkg/controller/vsphere/actuator.go @@ -5,12 +5,10 @@ package vsphere import ( "context" "fmt" - "time" "k8s.io/component-base/featuregate" machinev1 "github.com/openshift/api/machine/v1beta1" - machinecontroller "github.com/openshift/machine-api-operator/pkg/controller/machine" corev1 "k8s.io/api/core/v1" "k8s.io/client-go/tools/events" "k8s.io/klog/v2" @@ -18,13 +16,12 @@ import ( ) const ( - scopeFailFmt = "%s: failed to create scope for machine: %v" - reconcilerFailFmt = "%s: reconciler failed to %s machine: %w" - createEventAction = "Create" - updateEventAction = "Update" - deleteEventAction = "Delete" - noEventAction = "" - requeueAfterSeconds = 20 + scopeFailFmt = "%s: failed to create scope for machine: %v" + reconcilerFailFmt = "%s: reconciler failed to %s machine: %w" + createEventAction = "Create" + updateEventAction = "Update" + deleteEventAction = "Delete" + noEventAction = "" ) // Actuator is responsible for performing machine reconciliation. @@ -86,17 +83,25 @@ func (a *Actuator) Create(ctx context.Context, machine *machinev1.Machine) error return a.handleMachineError(machine, fmtErr, createEventAction) } - // Ensure we're not reconciling a stale machine by checking our task-id. - // This is a workaround for a cache race condition. - if val, ok := a.TaskIDCache[machine.Name]; ok { - if val != scope.providerStatus.TaskRef { - klog.Errorf("%s: machine object missing expected provider task ID, requeue", machine.GetName()) - return &machinecontroller.RequeueAfterError{RequeueAfter: requeueAfterSeconds * time.Second} - } + // If we already submitted a task for this machine (tracked in the in-memory + // cache) but the Machine object does not yet reflect it, the status patch + // that would have persisted the TaskRef may have failed, or the client cache + // may be stale. Recover the TaskRef from the cache so we reconcile the task + // we already submitted instead of requeueing forever (which permanently + // wedged creation) or dropping the task identity (which could submit a + // duplicate clone). + if cachedTaskRef, ok := a.TaskIDCache[machine.Name]; ok && scope.providerStatus.TaskRef == "" { + klog.Infof("%s: recovering task reference %q from cache; Machine status does not reflect it yet", machine.GetName(), cachedTaskRef) + scope.providerStatus.TaskRef = cachedTaskRef } var retErr error err = newReconciler(scope).create() + // Remember the submitted task reference even if the patch below fails, so a + // retry reconciles the in-flight task instead of submitting a second clone. + if scope.providerStatus.TaskRef != "" { + a.TaskIDCache[machine.Name] = scope.providerStatus.TaskRef + } if err != nil { fmtErr := fmt.Errorf(reconcilerFailFmt, machine.GetName(), createEventAction, err) retErr = a.handleMachineError(machine, fmtErr, createEventAction) @@ -108,17 +113,6 @@ func (a *Actuator) Create(ctx context.Context, machine *machinev1.Machine) error return err } - // Only cache the taskRef once it has been durably persisted on the Machine - // object. Caching it before a successful patch can permanently wedge - // creation: if the patch fails (for example, a transient admission-webhook - // denial) the object never receives the taskRef and the staleness guard - // above would requeue forever. create() is idempotent (it looks the VM up - // in vCenter before cloning), so a lost taskRef is recovered on the next - // reconcile rather than re-cloning. - if scope.providerStatus.TaskRef != "" { - a.TaskIDCache[machine.Name] = scope.providerStatus.TaskRef - } - return retErr } diff --git a/pkg/controller/vsphere/actuator_test.go b/pkg/controller/vsphere/actuator_test.go index dc871ff50..a1b397495 100644 --- a/pkg/controller/vsphere/actuator_test.go +++ b/pkg/controller/vsphere/actuator_test.go @@ -5,6 +5,7 @@ import ( "fmt" "net" "path/filepath" + "strings" "testing" "time" @@ -427,12 +428,11 @@ func TestMachineEvents(t *testing.T) { } } -// TestActuatorCreateCachesTaskRefOnlyAfterSuccessfulPatch verifies the actuator -// records a machine's clone TaskRef in its in-memory cache only after that ref -// has been durably persisted. A denied status patch must not leave a phantom -// cache entry, which previously wedged every subsequent reconcile via the -// staleness guard in Create (OCPBUGS-100316). -func TestActuatorCreateCachesTaskRefOnlyAfterSuccessfulPatch(t *testing.T) { +// TestActuatorCreateTaskRefLifecycle verifies the actuator's clone task +// reference bookkeeping: the reference is remembered even when the status patch +// that would persist it is denied, so a retry reconciles the same task instead +// of requeueing forever or submitting a duplicate clone (OCPBUGS-100316). +func TestActuatorCreateTaskRefLifecycle(t *testing.T) { model, session, server := initSimulator(t) defer model.Remove() defer server.Close() @@ -488,12 +488,28 @@ func TestActuatorCreateCachesTaskRefOnlyAfterSuccessfulPatch(t *testing.T) { } } + denyStatusPatch := func(base client.WithWatch) client.WithWatch { + return interceptor.NewClient(base, interceptor.Funcs{ + SubResourcePatch: func(_ context.Context, _ client.Client, _ string, _ client.Object, _ client.Patch, _ ...client.SubResourcePatchOption) error { + return fmt.Errorf("admission webhook denied the request") + }, + }) + } + gates, err := testutils.NewDefaultMutableFeatureGate() if err != nil { t.Fatalf("unexpected error setting up feature gates: %v", err) } - t.Run("denied status patch does not cache the taskRef", func(t *testing.T) { + waitForCloneTask := func(g *WithT, taskRef string) { + moTask, err := session.GetTask(context.TODO(), taskRef) + g.Expect(err).ToNot(HaveOccurred()) + if moTask != nil { + g.Expect(object.NewTask(session.Client.Client, moTask.Reference()).Wait(context.TODO())).To(Succeed()) + } + } + + t.Run("a denied status patch retains the clone task reference", func(t *testing.T) { g := NewWithT(t) machine := newMachine("patch-denied") @@ -501,16 +517,11 @@ func TestActuatorCreateCachesTaskRefOnlyAfterSuccessfulPatch(t *testing.T) { WithStatusSubresource(machine). WithRuntimeObjects(credentialsSecret, configMap, userDataSecret, machine). Build() - denyStatusPatch := interceptor.NewClient(base, interceptor.Funcs{ - SubResourcePatch: func(_ context.Context, _ client.Client, _ string, _ client.Object, _ client.Patch, _ ...client.SubResourcePatchOption) error { - return fmt.Errorf("admission webhook denied the request") - }, - }) taskIDCache := map[string]string{} actuator := NewActuator(ActuatorParams{ - Client: denyStatusPatch, - APIReader: denyStatusPatch, + Client: denyStatusPatch(base), + APIReader: base, EventRecorder: events.NewFakeRecorder(10), TaskIDCache: taskIDCache, OpenshiftConfigNamespace: openshiftConfigNamespaceForTest, @@ -519,12 +530,15 @@ func TestActuatorCreateCachesTaskRefOnlyAfterSuccessfulPatch(t *testing.T) { err := actuator.Create(context.Background(), machine) g.Expect(err).To(HaveOccurred()) - // The lost taskRef must not be cached; otherwise the staleness guard in - // Create would requeue forever once the object never receives it. - g.Expect(taskIDCache).ToNot(HaveKey(machine.Name)) + // The clone identity must survive the failed patch so the next reconcile + // reconciles the same task rather than submitting a duplicate clone. + g.Expect(taskIDCache).To(HaveKey(machine.Name)) + g.Expect(taskIDCache[machine.Name]).ToNot(BeEmpty()) + + waitForCloneTask(g, taskIDCache[machine.Name]) }) - t.Run("successful patch caches the taskRef", func(t *testing.T) { + t.Run("a successful patch caches the clone task reference", func(t *testing.T) { g := NewWithT(t) machine := newMachine("patch-ok") @@ -547,11 +561,64 @@ func TestActuatorCreateCachesTaskRefOnlyAfterSuccessfulPatch(t *testing.T) { g.Expect(taskIDCache).To(HaveKey(machine.Name)) g.Expect(taskIDCache[machine.Name]).ToNot(BeEmpty()) - // Wait on the async clone task so the simulator is not torn down early. - moTask, err := session.GetTask(context.TODO(), taskIDCache[machine.Name]) - g.Expect(err).ToNot(HaveOccurred()) - if moTask != nil { - g.Expect(object.NewTask(session.Client.Client, moTask.Reference()).Wait(context.TODO())).To(Succeed()) - } + waitForCloneTask(g, taskIDCache[machine.Name]) }) + + t.Run("a lost task reference is reconciled on retry without a second clone", func(t *testing.T) { + g := NewWithT(t) + + // Hold the clone task in-flight so the cloned VM is not yet discoverable + // in vCenter - the exact window in which a lost TaskRef previously caused + // a duplicate clone submission. + simulator.TaskDelay.MethodDelay = map[string]int{"CloneVm": 2000, "LockHandoff": 0} + defer func() { simulator.TaskDelay = simulator.DelayConfig{} }() + + machine := newMachine("inflight") + base := fake.NewClientBuilder().WithScheme(scheme.Scheme). + WithStatusSubresource(machine). + WithRuntimeObjects(credentialsSecret, configMap, userDataSecret, machine). + Build() + + taskIDCache := map[string]string{} + actuator := NewActuator(ActuatorParams{ + Client: denyStatusPatch(base), + APIReader: base, + EventRecorder: events.NewFakeRecorder(10), + TaskIDCache: taskIDCache, + OpenshiftConfigNamespace: openshiftConfigNamespaceForTest, + FeatureGates: gates, + }) + + vmCountBefore := len(model.Map().All("VirtualMachine")) + cloneTasksBefore := countCloneTasks(model) + + // First reconcile submits the clone; the status patch is denied so the + // TaskRef lives only in the cache. + g.Expect(actuator.Create(context.Background(), machine)).To(HaveOccurred()) + g.Expect(taskIDCache).To(HaveKey(machine.Name)) + g.Expect(countCloneTasks(model)).To(Equal(cloneTasksBefore+1), "first reconcile must submit exactly one clone") + // The clone is still running, so the VM is not yet discoverable. This + // confirms the in-flight window is actually reproduced. + g.Expect(model.Map().All("VirtualMachine")).To(HaveLen(vmCountBefore), "clone should still be in-flight (VM not yet created)") + + // Retry while the clone is in-flight: the actuator must reconcile the + // cached task, not submit a second clone. + g.Expect(actuator.Create(context.Background(), machine)).To(HaveOccurred()) + g.Expect(countCloneTasks(model)).To(Equal(cloneTasksBefore+1), "retry must not submit a second clone") + + // Let the clone finish before teardown. + waitForCloneTask(g, taskIDCache[machine.Name]) + }) +} + +// countCloneTasks returns the number of VM clone tasks recorded in the +// simulator's inventory. +func countCloneTasks(model *simulator.Model) int { + count := 0 + for _, ref := range model.Map().AllReference("") { + if task, ok := ref.(*simulator.Task); ok && strings.Contains(task.Info.DescriptionId, cloneVmTaskDescriptionId) { + count++ + } + } + return count } diff --git a/pkg/controller/vsphere/reconciler.go b/pkg/controller/vsphere/reconciler.go index 3a50fdb5e..45bf71849 100644 --- a/pkg/controller/vsphere/reconciler.go +++ b/pkg/controller/vsphere/reconciler.go @@ -167,10 +167,26 @@ func (r *Reconciler) create() error { } // The VM already exists but we have no TaskRef for it: we cloned it - // previously and lost the TaskRef. Recover by powering it on and - // recording the power-on task so subsequent reconciles can track it, - // instead of requeueing forever. - klog.Infof("%v: VM already exists without a persisted taskRef, powering on to recover", r.machine.GetName()) + // previously and lost the TaskRef. Complete the post-clone sequence to + // recover — restore VM group membership (if configured) and power the VM + // on, recording the power-on task so subsequent reconciles can track it, + // instead of requeueing forever. This mirrors the completed-clone path + // below so a recovered VM is not left outside its configured VM group. + klog.Infof("%v: VM already exists without a persisted taskRef, recovering", r.machine.GetName()) + if r.machineScope.providerSpec.Workspace.VMGroup != "" { + klog.Infof("Adding recovered machine: %s to vm group: %s", r.machine.Name, r.machineScope.providerSpec.Workspace.VMGroup) + + if err := modifyVMGroup(r.machineScope, false); err != nil { + var taskError task.Error + if errors.As(err, &taskError) { + return fmt.Errorf("could not update VM Group membership: %w", taskError) + } + + return fmt.Errorf("could not update VM Group membership: %w", err) + } + } + + klog.Infof("%v: powering on recovered machine", r.machine.GetName()) task, err := powerOn(r.machineScope) if err != nil { metrics.RegisterFailedInstanceCreate(&metrics.MachineLabels{ diff --git a/pkg/controller/vsphere/reconciler_test.go b/pkg/controller/vsphere/reconciler_test.go index 645e5feed..91523d63f 100644 --- a/pkg/controller/vsphere/reconciler_test.go +++ b/pkg/controller/vsphere/reconciler_test.go @@ -3024,6 +3024,105 @@ func TestCreateRecoversLostTaskRef(t *testing.T) { g.Expect(powerState).To(Equal(types.VirtualMachinePowerStatePoweredOn)) } +// TestCreateRecoveryRestoresVMGroup verifies that when create() recovers a VM +// whose TaskRef was lost, it restores the configured VM-group membership before +// powering the VM on, matching the normal completed-clone path. Otherwise a +// recovered VM would be left outside its DRS host-affinity group +// (OCPBUGS-100316). +func TestCreateRecoveryRestoresVMGroup(t *testing.T) { + g := NewWithT(t) + + poweredOff := func(m *simulator.Model) { m.Autostart = false } + model, server := initSimulatorCustom(t, poweredOff) + session := getSimulatorSession(t, server) + defer model.Remove() + defer server.Close() + + host, _, err := net.SplitHostPort(server.URL.Host) + g.Expect(err).ToNot(HaveOccurred()) + + ctx := context.Background() + ccr, err := session.Finder.ClusterComputeResourceOrDefault(ctx, "/...") + g.Expect(err).ToNot(HaveOccurred()) + resourcePool := path.Join(ccr.InventoryPath, "Resources") + + vmGroup := "recovery-vm-group" + g.Expect(createVMGroup(ctx, session, ccr.Name(), vmGroup)).To(Succeed()) + + // Pick a powered-off VM that belongs to the cluster, standing in for a VM we + // cloned but whose TaskRef we lost. + var existingVM *simulator.VirtualMachine + for _, obj := range model.Map().All("VirtualMachine") { + candidate := obj.(*simulator.VirtualMachine) + if candidate.Runtime.PowerState == types.VirtualMachinePowerStatePoweredOff && candidate.ResourcePool != nil { + existingVM = candidate + break + } + } + g.Expect(existingVM).ToNot(BeNil()) + vmCountBefore := len(model.Map().All("VirtualMachine")) + + gates, err := testutils.NewDefaultMutableFeatureGate() + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(gates.SetFromMap(map[string]bool{string(features.FeatureGateVSphereHostVMGroupZonal): true})).To(Succeed()) + + provisioning := string(machinev1.PhaseProvisioning) + machineObj := &machinev1.Machine{ + ObjectMeta: metav1.ObjectMeta{ + Name: existingVM.Name, + Namespace: "test", + Labels: map[string]string{machinev1.MachineClusterIDLabel: "CLUSTERID"}, + UID: apimachinerytypes.UID(existingVM.Config.InstanceUuid), + }, + Status: machinev1.MachineStatus{Phase: &provisioning}, + } + + machineScope := &machineScope{ + Context: ctx, + machine: machineObj, + machineToBePatched: runtimeclient.MergeFrom(machineObj.DeepCopy()), + providerSpec: &machinev1.VSphereMachineProviderSpec{ + Template: existingVM.Name, + Workspace: &machinev1.Workspace{ + Server: host, + VMGroup: vmGroup, + ResourcePool: resourcePool, + }, + }, + session: session, + providerStatus: &machinev1.VSphereMachineProviderStatus{}, + featureGates: gates, + client: fake.NewClientBuilder().WithScheme(scheme.Scheme).WithRuntimeObjects(machineObj).WithStatusSubresource(machineObj).Build(), + } + + reconciler := newReconciler(machineScope) + + g.Expect(reconciler.create()).To(Succeed()) + + // No duplicate VM was cloned. + g.Expect(model.Map().All("VirtualMachine")).To(HaveLen(vmCountBefore)) + + // The recovered VM must have been added to the configured VM group before + // power-on. + clusterConfig, err := ccr.Configuration(ctx) + g.Expect(err).ToNot(HaveOccurred()) + memberFound := false + for _, grp := range clusterConfig.Group { + if vmg, ok := grp.(*types.ClusterVmGroup); ok && vmg.Name == vmGroup { + for _, ref := range vmg.Vm { + if ref.Value == existingVM.Reference().Value { + memberFound = true + } + } + } + } + g.Expect(memberFound).To(BeTrue(), "recovered VM must be a member of its configured VM group") + + // A power-on task must have been recorded for the recovered VM. + g.Expect(reconciler.providerStatus.TaskRef).ToNot(BeEmpty()) + g.Expect(waitForTaskToComplete(session, reconciler)).To(Succeed()) +} + func TestUpdate(t *testing.T) { model, session, server := initSimulator(t) defer model.Remove() From a1263f82fdd91f30d8b63261b41ffc7e0f5bf5e4 Mon Sep 17 00:00:00 2001 From: Mat Kowalski Date: Thu, 20 Aug 2026 13:25:30 +0200 Subject: [PATCH 3/3] OCPBUGS-100316: vsphere: recover cached task ref on any Machine mismatch Follow-up to review of 302951f. The staleness guard recovered the cached task reference only when the Machine object had an empty TaskRef. A denied status patch (or informer lag) can instead leave the Machine on the previous, still-nonempty task while the cache has already advanced to the next one. In that case the guard did nothing and create() reprocessed the stale reference, e.g. reconciling a finished clone again and submitting a duplicate power-on. Recover the cached reference on any mismatch (cachedTaskRef != scope.providerStatus.TaskRef). The cache always holds the most recently submitted task (updated before the patch), so it is the correct thing to reconcile. Also make the delayed-clone test faithful: it now retries with a freshly read Machine instead of the in-memory pointer, since PatchMachine mutates that pointer's status before the failed patch would have hidden the bug. Adds a regression test for the stale-nonempty-TaskRef case. Assisted-By: Claude Opus 4.6 --- pkg/controller/vsphere/actuator.go | 19 ++--- pkg/controller/vsphere/actuator_test.go | 97 ++++++++++++++++++++++--- 2 files changed, 97 insertions(+), 19 deletions(-) diff --git a/pkg/controller/vsphere/actuator.go b/pkg/controller/vsphere/actuator.go index 6e27b7fce..350a1c7a8 100644 --- a/pkg/controller/vsphere/actuator.go +++ b/pkg/controller/vsphere/actuator.go @@ -83,15 +83,16 @@ func (a *Actuator) Create(ctx context.Context, machine *machinev1.Machine) error return a.handleMachineError(machine, fmtErr, createEventAction) } - // If we already submitted a task for this machine (tracked in the in-memory - // cache) but the Machine object does not yet reflect it, the status patch - // that would have persisted the TaskRef may have failed, or the client cache - // may be stale. Recover the TaskRef from the cache so we reconcile the task - // we already submitted instead of requeueing forever (which permanently - // wedged creation) or dropping the task identity (which could submit a - // duplicate clone). - if cachedTaskRef, ok := a.TaskIDCache[machine.Name]; ok && scope.providerStatus.TaskRef == "" { - klog.Infof("%s: recovering task reference %q from cache; Machine status does not reflect it yet", machine.GetName(), cachedTaskRef) + // If the task we last submitted for this machine (tracked in the in-memory + // cache) differs from what the Machine object reflects, the status patch + // that would have persisted it may have failed, or the client cache may be + // stale. The cache always holds the most recently submitted task (it is + // updated on every reconcile, before the patch), so recover it and reconcile + // that task instead of requeueing forever (which permanently wedged + // creation) or reprocessing a stale reference (which could submit a + // duplicate clone or power-on). + if cachedTaskRef, ok := a.TaskIDCache[machine.Name]; ok && cachedTaskRef != scope.providerStatus.TaskRef { + klog.Infof("%s: recovering task reference %q from cache; Machine status reflects %q", machine.GetName(), cachedTaskRef, scope.providerStatus.TaskRef) scope.providerStatus.TaskRef = cachedTaskRef } diff --git a/pkg/controller/vsphere/actuator_test.go b/pkg/controller/vsphere/actuator_test.go index a1b397495..580a2fd79 100644 --- a/pkg/controller/vsphere/actuator_test.go +++ b/pkg/controller/vsphere/actuator_test.go @@ -590,33 +590,110 @@ func TestActuatorCreateTaskRefLifecycle(t *testing.T) { }) vmCountBefore := len(model.Map().All("VirtualMachine")) - cloneTasksBefore := countCloneTasks(model) + cloneTasksBefore := countTasksMatching(model, cloneVmTaskDescriptionId) // First reconcile submits the clone; the status patch is denied so the // TaskRef lives only in the cache. g.Expect(actuator.Create(context.Background(), machine)).To(HaveOccurred()) g.Expect(taskIDCache).To(HaveKey(machine.Name)) - g.Expect(countCloneTasks(model)).To(Equal(cloneTasksBefore+1), "first reconcile must submit exactly one clone") + g.Expect(countTasksMatching(model, cloneVmTaskDescriptionId)).To(Equal(cloneTasksBefore+1), "first reconcile must submit exactly one clone") // The clone is still running, so the VM is not yet discoverable. This // confirms the in-flight window is actually reproduced. g.Expect(model.Map().All("VirtualMachine")).To(HaveLen(vmCountBefore), "clone should still be in-flight (VM not yet created)") - // Retry while the clone is in-flight: the actuator must reconcile the - // cached task, not submit a second clone. - g.Expect(actuator.Create(context.Background(), machine)).To(HaveOccurred()) - g.Expect(countCloneTasks(model)).To(Equal(cloneTasksBefore+1), "retry must not submit a second clone") + // Retry the way the machine controller would: with a freshly read + // Machine. Because the status patch was denied, the persisted object has + // no TaskRef, so the actuator must recover it from the cache rather than + // submit a second clone. (Reusing the in-memory pointer would hide the + // bug, since PatchMachine mutates it before the failed patch.) + fresh := &machinev1.Machine{} + g.Expect(base.Get(context.Background(), client.ObjectKeyFromObject(machine), fresh)).To(Succeed()) + g.Expect(fresh.Status.ProviderStatus).To(BeNil(), "denied status patch must not have persisted a TaskRef") + g.Expect(actuator.Create(context.Background(), fresh)).To(HaveOccurred()) + g.Expect(countTasksMatching(model, cloneVmTaskDescriptionId)).To(Equal(cloneTasksBefore+1), "retry must not submit a second clone") // Let the clone finish before teardown. waitForCloneTask(g, taskIDCache[machine.Name]) }) + + t.Run("a stale nonempty task reference is reconciled from the cache on retry", func(t *testing.T) { + g := NewWithT(t) + + machine := newMachine("stale-nonempty") + base := fake.NewClientBuilder().WithScheme(scheme.Scheme). + WithStatusSubresource(machine). + WithRuntimeObjects(credentialsSecret, configMap, userDataSecret, machine). + Build() + + taskIDCache := map[string]string{} + + // Clone successfully first, so the Machine object and the cache both + // track the clone task and the (powered-off) VM exists. + allowActuator := NewActuator(ActuatorParams{ + Client: base, + APIReader: base, + EventRecorder: events.NewFakeRecorder(10), + TaskIDCache: taskIDCache, + OpenshiftConfigNamespace: openshiftConfigNamespaceForTest, + FeatureGates: gates, + }) + g.Expect(allowActuator.Create(context.Background(), machine)).To(Succeed()) + cloneTaskRef := taskIDCache[machine.Name] + g.Expect(cloneTaskRef).ToNot(BeEmpty()) + waitForCloneTask(g, cloneTaskRef) + + // Hold power-on in-flight and deny status patches, so the power-on task + // is submitted but never persisted onto the Machine object. + simulator.TaskDelay.MethodDelay = map[string]int{"PowerOnMultiVM": 2000, "LockHandoff": 0} + defer func() { simulator.TaskDelay = simulator.DelayConfig{} }() + + denyActuator := NewActuator(ActuatorParams{ + Client: denyStatusPatch(base), + APIReader: base, + EventRecorder: events.NewFakeRecorder(10), + TaskIDCache: taskIDCache, + OpenshiftConfigNamespace: openshiftConfigNamespaceForTest, + FeatureGates: gates, + }) + + powerOnBefore := countTasksMatching(model, powerOnTaskDescriptionID) + + // Reconcile the finished clone: submits a power-on task and advances the + // cache, but the denied patch leaves the Machine object on the clone task. + fresh1 := &machinev1.Machine{} + g.Expect(base.Get(context.Background(), client.ObjectKeyFromObject(machine), fresh1)).To(Succeed()) + g.Expect(denyActuator.Create(context.Background(), fresh1)).To(HaveOccurred()) + g.Expect(countTasksMatching(model, powerOnTaskDescriptionID)).To(Equal(powerOnBefore+1), "reconciling the finished clone must submit exactly one power-on") + g.Expect(taskIDCache[machine.Name]).ToNot(Equal(cloneTaskRef), "cache should have advanced to the power-on task") + + // Retry with a freshly read Machine, which still carries the stale clone + // task because the power-on patch was denied. The actuator must recover + // the newer power-on task from the cache instead of reprocessing the + // clone and submitting a second power-on. + fresh2 := &machinev1.Machine{} + g.Expect(base.Get(context.Background(), client.ObjectKeyFromObject(machine), fresh2)).To(Succeed()) + g.Expect(denyActuator.Create(context.Background(), fresh2)).To(HaveOccurred()) + g.Expect(countTasksMatching(model, powerOnTaskDescriptionID)).To(Equal(powerOnBefore+1), "retry must not submit a second power-on") + + // Let the power-on finish before teardown. + moTask, err := session.GetTask(context.TODO(), taskIDCache[machine.Name]) + g.Expect(err).ToNot(HaveOccurred()) + if moTask != nil { + g.Expect(object.NewTask(session.Client.Client, moTask.Reference()).Wait(context.TODO())).To(Succeed()) + } + }) } -// countCloneTasks returns the number of VM clone tasks recorded in the -// simulator's inventory. -func countCloneTasks(model *simulator.Model) int { +// powerOnTaskDescriptionID is the DescriptionId of the task issued when powering +// on a VM via Datacenter.PowerOnVM in the simulator. +const powerOnTaskDescriptionID = "powerOnMultiVM" + +// countTasksMatching returns the number of tasks in the simulator inventory +// whose DescriptionId contains the given substring. +func countTasksMatching(model *simulator.Model, descriptionSubstring string) int { count := 0 for _, ref := range model.Map().AllReference("") { - if task, ok := ref.(*simulator.Task); ok && strings.Contains(task.Info.DescriptionId, cloneVmTaskDescriptionId) { + if task, ok := ref.(*simulator.Task); ok && strings.Contains(task.Info.DescriptionId, descriptionSubstring) { count++ } }