Surface error when VM is waiting for VMG

Signed-off-by: Gong Zhang <[email protected]>

Author: zhanggbj

apis/v1beta1/vspheremachine_types.go

@@ -81,6 +81,10 @@ const (
 	// Note: This reason is used only in supervisor mode.
 	VSphereMachineVirtualMachinePoweringOnV1Beta2Reason = "PoweringOn"
 
+	// VSphereMachineVirtualMachineWaitingForVirtualMachineGroupV1Beta2Reason surfaces that the VirtualMachine
+	// is waiting for its corresponding VirtualMachineGroup to be created and to include this VM as a member.
+	VSphereMachineVirtualMachineWaitingForVirtualMachineGroupV1Beta2Reason = "WaitingForVirtualMachineGroup"
+
 	// VSphereMachineVirtualMachineWaitingForNetworkAddressV1Beta2Reason surfaces when the VirtualMachine that is controlled
 	// by the VSphereMachine waiting for the machine network settings to be reported after machine being powered on.
 	VSphereMachineVirtualMachineWaitingForNetworkAddressV1Beta2Reason = "WaitingForNetworkAddress"

controllers/vmware/virtualmachinegroup_controller.go

@@ -89,7 +89,7 @@ func AddVirtualMachineGroupControllerToManager(ctx context.Context, controllerMa
 }
 
 // ClusterToVirtualMachineGroup maps Cluster events to VirtualMachineGroup reconcile requests.
-func (r VirtualMachineGroupReconciler) ClusterToVirtualMachineGroup(ctx context.Context, a ctrlclient.Object) []reconcile.Request {
+func (r *VirtualMachineGroupReconciler) ClusterToVirtualMachineGroup(ctx context.Context, a ctrlclient.Object) []reconcile.Request {
 	cluster, ok := a.(*clusterv1.Cluster)
 	if !ok {
 		return nil
@@ -104,11 +104,10 @@ func (r VirtualMachineGroupReconciler) ClusterToVirtualMachineGroup(ctx context.
 	}}
 }
 
-// VsphereMachineToVirtualMachineGroup maps VSphereMachine events to VirtualMachineGroup reconcile requests.
-// This handler only processes VSphereMachine objects for Day-2 operations, ensuring VSphereMachine state stays
-// in sync with its owning VMG. If no corresponding VMG is found, this is a no-op.
-
-func (r VirtualMachineGroupReconciler) VSphereMachineToVirtualMachineGroup(ctx context.Context, a ctrlclient.Object) []reconcile.Request {
+// VSphereMachineToVirtualMachineGroup maps VSphereMachine events to VirtualMachineGroup reconcile requests.
+// This handler only processes VSphereMachine objects for Day-2 operations when VMG could be found, ensuring
+// VMG member list in sync with VSphereMachines. If no corresponding VMG is found, this is a no-op.
+func (r *VirtualMachineGroupReconciler) VSphereMachineToVirtualMachineGroup(ctx context.Context, a ctrlclient.Object) []reconcile.Request {
 	vSphereMachine, ok := a.(*vmwarev1.VSphereMachine)
 	if !ok {
 		return nil

controllers/vmware/virtualmachinegroup_reconciler.go

@@ -51,6 +51,14 @@ type VirtualMachineGroupReconciler struct {
 	Recorder record.EventRecorder
 }
 
+// +kubebuilder:rbac:groups=cluster.x-k8s.io,resources=clusters,verbs=get;list;watch
+// +kubebuilder:rbac:groups=cluster.x-k8s.io,resources=clusters/status,verbs=get
+// +kubebuilder:rbac:groups=vmoperator.vmware.com,resources=virtualmachinegroups,verbs=get;list;watch;create;update;patch;delete
+// +kubebuilder:rbac:groups=vmoperator.vmware.com,resources=virtualmachinegroups/status,verbs=get
+// +kubebuilder:rbac:groups=vmware.infrastructure.cluster.x-k8s.io,resources=vspheremachines,verbs=get;list;watch
+// +kubebuilder:rbac:groups=cluster.x-k8s.io,resources=machinedeployments,verbs=get;list;watch
+// +kubebuilder:rbac:groups=cluster.x-k8s.io,resources=machines,verbs=get;list;watch
+
 func (r *VirtualMachineGroupReconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Result, reterr error) {
 	log := ctrl.LoggerFrom(ctx)
 
@@ -105,7 +113,12 @@ func (r *VirtualMachineGroupReconciler) createOrUpdateVMG(ctx context.Context, c
 		}
 
 		// Calculate expected Machines of all MachineDeployments.
-		expected := getExpectedVSphereMachines(cluster)
+		expected, err := getExpectedVSphereMachines(ctx, r.Client, cluster)
+		if err != nil {
+			log.Error(err, "failed to get expected Machines of all MachineDeployment")
+			return ctrl.Result{}, err
+		}
+
 		if expected == 0 {
 			log.Info("none of MachineDeployments specifies replica and node auto replacement doesn't support this scenario")
 			return reconcile.Result{}, nil
@@ -215,60 +228,31 @@ func (r *VirtualMachineGroupReconciler) createOrUpdateVMG(ctx context.Context, c
 	return reconcile.Result{}, err
 }
 
-// isMDDefined checks if there are any MachineDeployments for the given cluster
-// by listing objects with the cluster.x-k8s.io/cluster-name label.
-func (r *VirtualMachineGroupReconciler) isMDDefined(ctx context.Context, cluster *clusterv1.Cluster) (bool, error) {
-	mdList := &clusterv1.MachineDeploymentList{}
-	if err := r.Client.List(ctx, mdList, client.InNamespace(cluster.Namespace), client.MatchingLabels{clusterv1.ClusterNameLabel: cluster.Name}); err != nil {
-		return false, errors.Wrapf(err, "failed to list MachineDeployments for cluster %s/%s",
-			cluster.Namespace, cluster.Name)
-	}
-
-	if len(mdList.Items) == 0 {
-		return false, errors.Errorf("no MachineDeployments found for cluster %s/%s",
-			cluster.Namespace, cluster.Name)
-	}
-
-	return true, nil
-}
-
-// isExplicitPlacement checks if any MachineDeployment has an explicit failure domain set.
-func (r *VirtualMachineGroupReconciler) isExplicitPlacement(cluster *clusterv1.Cluster) (bool, error) {
-	// First, ensure MachineDeployments are defined
-	mdDefined, err := r.isMDDefined(context.Background(), cluster)
-	if !mdDefined {
-		return false, err
-	}
-
-	// Iterate through MachineDeployments to find if an explicit failure domain is set.
-	mds := cluster.Spec.Topology.Workers.MachineDeployments
-	for _, md := range mds {
-		// If a failure domain is specified for any MachineDeployment, it indicates
-		// explicit placement is configured, so return true.
-		if md.FailureDomain != "" {
-			return true, nil
-		}
-	}
-
-	return false, nil
-}
-
 // getExpectedVSphereMachines returns the total number of replicas across all
-// MachineDeployments in the Cluster's Topology.Workers.
-func getExpectedVSphereMachines(cluster *clusterv1.Cluster) int32 {
-	if !cluster.Spec.Topology.IsDefined() {
-		return 0
+// MachineDeployments belonging to the Cluster
+func getExpectedVSphereMachines(ctx context.Context, kubeClient client.Client, cluster *clusterv1.Cluster) (int32, error) {
+	var mdList clusterv1.MachineDeploymentList
+	if err := kubeClient.List(
+		ctx,
+		&mdList,
+		client.InNamespace(cluster.Namespace),
+		client.MatchingLabels{clusterv1.ClusterNameLabel: cluster.Name},
+	); err != nil {
+		return 0, errors.Wrap(err, "failed to list MachineDeployments")
 	}
 
 	var total int32
-	for _, md := range cluster.Spec.Topology.Workers.MachineDeployments {
-		if md.Replicas != nil {
-			total += *md.Replicas
+	for _, md := range mdList.Items {
+		if md.Spec.Replicas != nil {
+			total += *md.Spec.Replicas
 		}
 	}
-	return total
+
+	return total, nil
 }
 
+// getCurrentVSphereMachines returns the list of VSphereMachines belonging to the Cluster’s MachineDeployments.
+// VSphereMachines marked for removal are excluded from the result.
 func getCurrentVSphereMachines(ctx context.Context, kubeClient client.Client, clusterNamespace, clusterName string) ([]vmwarev1.VSphereMachine, error) {
 	log := ctrl.LoggerFrom(ctx)
 
@@ -321,7 +305,7 @@ func GenerateVMGPlacementAnnotations(ctx context.Context, vmg *vmoprv1.VirtualMa
 
 			// Check if VM belongs to a Machine Deployment by name (e.g. cluster-1-np-1-vm-xxx contains np-1)
 			// TODO: Establish membership via the machine deployment name label
-			if strings.Contains(member.Name, md) {
+			if strings.Contains(member.Name, "-"+md+"-") {
 				// Get the VM placement information by member status.
 				// VMs that have undergone placement do not have Placement info set, skip.
 				if member.Placement == nil {
@@ -345,7 +329,7 @@ func GenerateVMGPlacementAnnotations(ctx context.Context, vmg *vmoprv1.VirtualMa
 	return annotations, nil
 }
 
-// TODO: de-dup this logic with vmopmachine.go
+// Duplicated this logic from pkg/services/vmoperator/vmopmachine.go
 // GenerateVirtualMachineName generates the name of a VirtualMachine based on the naming strategy.
 func GenerateVirtualMachineName(machineName string, namingStrategy *vmwarev1.VirtualMachineNamingStrategy) (string, error) {
 	// Per default the name of the VirtualMachine should be equal to the Machine name (this is the same as "{{ .machine.name }}")

controllers/vmware/virtualmachinegroup_reconciler_test.go

@@ -36,6 +36,7 @@ const (
 
 func TestGetExpectedVSphereMachines(t *testing.T) {
 	g := NewWithT(t)
+	ctx := context.Background()
 
 	tests := []struct {
 		name     string
@@ -64,8 +65,9 @@ func TestGetExpectedVSphereMachines(t *testing.T) {
 	}
 
 	for _, tt := range tests {
+		fakeClient := fake.NewClientBuilder().WithScheme(s).WithObjects(tt.cluster).Build()
 		t.Run(tt.name, func(t *testing.T) {
-			g.Expect(getExpectedVSphereMachines(tt.cluster)).To(Equal(tt.expected))
+			g.Expect(getExpectedVSphereMachines(ctx, fakeClient, tt.cluster)).To(Equal(tt.expected))
 		})
 	}
 }
@@ -74,12 +76,12 @@ func TestGetCurrentVSphereMachines(t *testing.T) {
 	g := NewWithT(t)
 	ctx := context.Background()
 
-	// VM names are based on CAPI Machine names, not VSphereMachine names, but we use VSM objects here.
+	// VM names are based on CAPI Machine names, not VSphereMachine names, but we use VSphereMachine here.
 	vsm1 := newVSphereMachine("vsm-1", mdName1, false, nil)
 	vsm2 := newVSphereMachine("vsm-2", mdName2, false, nil)
 	vsmDeleting := newVSphereMachine("vsm-3", mdName1, true, nil) // Deleting
 	vsmControlPlane := newVSphereMachine("vsm-cp", "cp-md", false, nil)
-	vsmControlPlane.Labels[clusterv1.MachineControlPlaneLabel] = "true" // Should be filtered by label in production, but here filtered implicitly by only listing MD-labelled objects
+	vsmControlPlane.Labels[clusterv1.MachineControlPlaneLabel] = "true"
 
 	tests := []struct {
 		name    string
@@ -94,7 +96,7 @@ func TestGetCurrentVSphereMachines(t *testing.T) {
 				vsmDeleting,
 				vsmControlPlane,
 			},
-			want: 2, // Should exclude vsm-3 (deleting) and vsm-cp (no MD label used in the actual listing logic)
+			want: 2, // Should exclude vsm-3 (deleting) and vsm-cp (control plane VSphereMachine)
 		},
 		{
 			name:    "No VSphereMachines found",
@@ -110,7 +112,7 @@ func TestGetCurrentVSphereMachines(t *testing.T) {
 			g.Expect(err).NotTo(HaveOccurred())
 			g.Expect(len(got)).To(Equal(tt.want))
 
-			// Check that the correct machines are present (e.g., vsm1 and vsm2)
+			// Check that the correct Machines are present
 			if tt.want > 0 {
 				names := make([]string, len(got))
 				for i, vsm := range got {
@@ -191,7 +193,7 @@ func TestGenerateVMGPlacementAnnotations(t *testing.T) {
 					Members: []vmoprv1.VirtualMachineGroupMemberStatus{
 						// First VM sets the placement
 						newVMGMemberStatus(vmName1, "VirtualMachine", true, zoneA),
-						// Second VM is ignored (logic skips finding placement twice)
+						// Second VM is ignored
 						newVMGMemberStatus(vmNameUnplaced, "VirtualMachine", true, zoneB),
 					},
 				},
@@ -208,7 +210,7 @@ func TestGenerateVMGPlacementAnnotations(t *testing.T) {
 				ObjectMeta: metav1.ObjectMeta{Name: clusterName, Namespace: clusterNamespace},
 				Status: vmoprv1.VirtualMachineGroupStatus{
 					Members: []vmoprv1.VirtualMachineGroupMemberStatus{
-						newVMGMemberStatus(vmNameWrongKind, "Pod", true, zoneA),
+						newVMGMemberStatus(vmNameWrongKind, "VirtualMachineGroup", true, zoneA),
 					},
 				},
 			},
@@ -220,7 +222,6 @@ func TestGenerateVMGPlacementAnnotations(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			// Mock client is needed for the logging in the function, but not used for API calls
 			ctx := ctrl.LoggerInto(context.Background(), ctrl.LoggerFrom(context.Background()))
 
 			got, err := GenerateVMGPlacementAnnotations(ctx, tt.vmg, tt.machineDeployments)
@@ -240,7 +241,7 @@ func TestVirtualMachineGroupReconciler_ReconcileFlow(t *testing.T) {
 	ctx := context.Background()
 
 	// Initial objects for the successful VMG creation path (Expected: 1, Current: 1)
-	cluster := newCluster(clusterName, clusterNamespace, true, 1, 0) // Expect 1 machine
+	cluster := newCluster(clusterName, clusterNamespace, true, 1, 0)
 	vsm1 := newVSphereMachine("vsm-1", mdName1, false, nil)
 	md1 := newMachineDeployment(mdName1)
 
@@ -271,13 +272,13 @@ func TestVirtualMachineGroupReconciler_ReconcileFlow(t *testing.T) {
 		{
 			name: "Requeue: ControlPlane Not Initialized",
 			initialObjects: []client.Object{
-				newCluster(clusterName, clusterNamespace, false, 1, 0), // Not Initialized
+				newCluster(clusterName, clusterNamespace, false, 1, 0),
 			},
 			expectedResult: reconcile.Result{RequeueAfter: reconciliationDelay},
 			checkVMGExists: false,
 		},
 		{
-			name: "Requeue: VMG Not Found, Machines Missing (0/1)",
+			name: "Requeue: VMG Not Found",
 			initialObjects: []client.Object{
 				cluster.DeepCopy(),
 				md1.DeepCopy(),
@@ -286,7 +287,7 @@ func TestVirtualMachineGroupReconciler_ReconcileFlow(t *testing.T) {
 			checkVMGExists: false,
 		},
 		{
-			name: "Success: VMG Created (1/1)",
+			name: "Success: VMG Created",
 			initialObjects: []client.Object{
 				cluster.DeepCopy(),
 				md1.DeepCopy(),
@@ -301,7 +302,7 @@ func TestVirtualMachineGroupReconciler_ReconcileFlow(t *testing.T) {
 				cluster.DeepCopy(),
 				md1.DeepCopy(),
 				vsm1.DeepCopy(),
-				&vmoprv1.VirtualMachineGroup{ // Pre-existing VMG
+				&vmoprv1.VirtualMachineGroup{
 					ObjectMeta: metav1.ObjectMeta{Name: clusterName, Namespace: clusterNamespace},
 				},
 			},
@@ -333,7 +334,9 @@ func TestVirtualMachineGroupReconciler_ReconcileFlow(t *testing.T) {
 				// Check that the core fields were set by the MutateFn
 				g.Expect(vmg.Labels).To(HaveKeyWithValue(clusterv1.ClusterNameLabel, clusterName))
 				g.Expect(vmg.Spec.BootOrder).To(HaveLen(1))
-				g.Expect(vmg.Spec.BootOrder[0].Members).To(HaveLen(int(getExpectedVSphereMachines(cluster))))
+				expected, err := getExpectedVSphereMachines(ctx, fakeClient, tt.initialObjects[0].(*clusterv1.Cluster))
+				g.Expect(err).NotTo(HaveOccurred(), "Should get expected Machines")
+				g.Expect(vmg.Spec.BootOrder[0].Members).To(HaveLen(int(expected)))
 
 				// VMG members should match the VSphereMachine (name: vsm-1)
 				g.Expect(vmg.Spec.BootOrder[0].Members[0].Name).To(ContainElement("vsm-1"))
@@ -344,9 +347,6 @@ func TestVirtualMachineGroupReconciler_ReconcileFlow(t *testing.T) {
 	}
 }
 
-// Helper function to create a *string
-func stringPtr(s string) *string { return &s }
-
 // Helper function to create a basic Cluster object
 func newCluster(name, namespace string, initialized bool, replicasMD1, replicasMD2 int32) *clusterv1.Cluster {
 	cluster := &clusterv1.Cluster{

pkg/services/vmoperator/vmopmachine.go

@@ -207,22 +207,27 @@ func (v *VmopMachineService) ReconcileNormal(ctx context.Context, machineCtx cap
 				return false, err
 			}
 			if apierrors.IsNotFound(err) {
-				log.V(4).Info("VirtualMachineGroup not found, requeueing")
+				log.V(4).Info("VirtualMachineGroup not found, requeueing", "Name", key.Name, "Namespace", key.Namespace)
 				return true, nil
 			}
 		}
 
 		// Proceed only if the machine is a member of the VirtualMachineGroup.
 		if !v.checkVirtualMachineGroupMembership(vmOperatorVMGroup, supervisorMachineCtx) {
-			log.V(4).Info("Waiting for VirtualMachineGroup membership, requeueing")
+			v1beta2conditions.Set(supervisorMachineCtx.VSphereMachine, metav1.Condition{
+				Type:   infrav1.VSphereMachineVirtualMachineProvisionedV1Beta2Condition,
+				Status: metav1.ConditionFalse,
+				Reason: infrav1.VSphereMachineVirtualMachineWaitingForVirtualMachineGroupV1Beta2Reason,
+			})
+			log.V(4).Info("Waiting for VirtualMachineGroup membership, requeueing", "VM Name", supervisorMachineCtx.Machine.Name)
 			return true, nil
 		}
 
 		affInfo = affinityInfo{
 			vmGroupName: vmOperatorVMGroup.Name,
 		}
 
-		// Set the zone label using the annotation of the machine deployment:zone mapping from VMG.
+		// Set the zone label using the annotation of the per-md zone mapping from VMG.
 		// This is for new VMs created during day-2 operations in VC 9.1.
 		nodePool := supervisorMachineCtx.Machine.Labels[clusterv1.MachineDeploymentNameLabel]
 		if zone, ok := vmOperatorVMGroup.Annotations[fmt.Sprintf("zone.cluster.x-k8s.io/%s", nodePool)]; ok && zone != "" {
@@ -842,10 +847,13 @@ func (v *VmopMachineService) addVolumes(ctx context.Context, supervisorMachineCt
 			},
 		}
 
+		// Before VC 9.1:
 		// The CSI zone annotation must be set when using a zonal storage class,
 		// which is required when the cluster has multiple (3) zones.
 		// Single zone clusters (legacy/default) do not support zonal storage and must not
 		// have the zone annotation set.
+		// Since VC 9.1: With Node Auto Placement enabled, failureDomain is optional and CAPV no longer
+		// sets PVC annotations. PVC placement now follows the StorageClass behavior (Immediate or WaitForFirstConsumer).
 		zonal := len(supervisorMachineCtx.VSphereCluster.Status.FailureDomains) > 1
 
 		if zone := supervisorMachineCtx.VSphereMachine.Spec.FailureDomain; zonal && zone != nil {
@@ -904,8 +912,7 @@ func getVMLabels(supervisorMachineCtx *vmware.SupervisorMachineContext, vmLabels
 		vmLabels[k] = v
 	}
 
-	// Get the labels that determine the VM's placement inside of a stretched
-	// cluster.
+	// Get the labels that determine the VM's placement
 	var failureDomain *string
 	if affinityInfo != nil && affinityInfo.failureDomain != nil {
 		failureDomain = affinityInfo.failureDomain
@@ -934,12 +941,13 @@ func getVMLabels(supervisorMachineCtx *vmware.SupervisorMachineContext, vmLabels
 //	and thus the code is optimized as such. However, in the future
 //	this function may return a more diverse topology.
 func getTopologyLabels(supervisorMachineCtx *vmware.SupervisorMachineContext, failureDomain *string) map[string]string {
-	// TODO: Make it so that we always set the zone label, might require enquiring the zones present (when unset)
+	// This is for explicit placement.
 	if fd := supervisorMachineCtx.VSphereMachine.Spec.FailureDomain; fd != nil && *fd != "" {
 		return map[string]string{
 			corev1.LabelTopologyZone: *fd,
 		}
 	}
+	// This is for automatic placement.
 	if failureDomain != nil && *failureDomain != "" {
 		return map[string]string{
 			corev1.LabelTopologyZone: *failureDomain,