Refine VMG member update to avoid race condition

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

Author: zhanggbj

controllers/vmware/virtualmachinegroup_reconciler.go

@@ -227,31 +227,31 @@ func (r *VirtualMachineGroupReconciler) reconcileVirtualMachineGroup(ctx context
 		return err
 	}
 
-	// Member Update:
-	// The VirtualMachineGroup's BootOrder.Members list, is only allowed to be set or added
-	// during two phases to maintain control over VM placement:
+	// The core purpose of isMemberUpdateAllowed is to prevent the VirtualMachineGroup from being updated with new members
+	// that require placement, unless the VirtualMachineGroup
+	// has successfully completed its initial placement and added the required
+	// placement annotations. This stabilizes placement decisions before allowing new VMs
+	// to be created under the group.
 	//
-	// 1. Initial Creation: When the VirtualMachineGroup object does not yet exist.
-	// 2. Post-Placement: After the VirtualMachineGroup exists AND is marked Ready which means all members are placed successfully,
-	//    and critically, all MachineDeployments have a corresponding zone placement annotation recorded on the VMG.
-	//
-	// For member removal, this is always allowed since it doesn't impact ongoing placement or rely on the placement annotation.
+	// The update is allowed if:
+	// 1. The VirtualMachineGroup is being initially created
+	// 2. The update is a scale-down operation (fewer target members).
+	// 3. The new member's underlying CAPI Machine has a FailureDomain set (will skip placement process).
+	// 4. The new member requires placement annotation AND the VirtualMachineGroup has the corresponding
+	//    placement annotation for the member's MachineDeployment.
 	//
 	// This prevents member updates that could lead to new VMs being created
 	// without necessary zone labels, resulting in undesired placement, such as VM within a MachineDeployment but are
 	// placed to different Zones.
-
-	isMemberUpdateAllowed, err := isMemberUpdateAllowed(ctx, r.Client, members, vmg)
+	err := isMemberUpdateAllowed(ctx, r.Client, members, vmg)
 	if err != nil {
 		return err
 	}
 
-	if isMemberUpdateAllowed {
-		vmg.Spec.BootOrder = []vmoprv1.VirtualMachineGroupBootOrderGroup{
-			{
-				Members: members,
-			},
-		}
+	vmg.Spec.BootOrder = []vmoprv1.VirtualMachineGroupBootOrderGroup{
+		{
+			Members: members,
+		},
 	}
 
 	// Set the owner reference
@@ -262,9 +262,8 @@ func (r *VirtualMachineGroupReconciler) reconcileVirtualMachineGroup(ctx context
 	return nil
 }
 
-// isMemberUpdateAllowed determines if the BootOrder.Members field can be safely updated on the VirtualMachineGroup.
-// It allows updates only during initial creation or after all member placement are completed successfully.
-func isMemberUpdateAllowed(ctx context.Context, kubeClient client.Client, targetMember []vmoprv1.GroupMember, vmg *vmoprv1.VirtualMachineGroup) (bool, error) {
+// isMemberUpdateAllowed checks if a VirtualMachineGroup's BootOrder.Members update is allowed.
+func isMemberUpdateAllowed(ctx context.Context, kubeClient client.Client, targetMember []vmoprv1.GroupMember, vmg *vmoprv1.VirtualMachineGroup) error {
 	logger := log.FromContext(ctx)
 	key := client.ObjectKey{
 		Namespace: vmg.Namespace,
@@ -275,30 +274,30 @@ func isMemberUpdateAllowed(ctx context.Context, kubeClient client.Client, target
 	currentVMG := &vmoprv1.VirtualMachineGroup{}
 	if err := kubeClient.Get(ctx, key, currentVMG); err != nil {
 		if apierrors.IsNotFound(err) {
-			// If VirtualMachineGroup is not found, allow member update as it should be in initial creation phase.
-			logger.V(5).Info("VirtualMachineGroup not found, allowing member update for initial creation.")
-			return true, nil
+			// 1. If VirtualMachineGroup is not found, allow member update as it should be in initial creation phase.
+			logger.V(5).Info("VirtualMachineGroup not created yet, allowing member update for initial creation.")
+			return nil
 		}
-		return false, errors.Wrapf(err, "failed to get VirtualMachineGroup %s/%s", vmg.Namespace, vmg.Name)
+		return errors.Wrapf(err, "failed to get VirtualMachineGroup %s/%s, blocking member update", vmg.Namespace, vmg.Name)
 	}
-	// Copy retrieved data back to the input pointer for consistency
+	// Copy retrieved data back to the input pointer for consistency.
 	*vmg = *currentVMG
 
-	// Get current member names from VirtualMachineGroup Spec.BootOrder
+	// Get current member names from VirtualMachineGroup Spec.BootOrder.
 	currentMemberNames := make(map[string]struct{})
 	if len(vmg.Spec.BootOrder) > 0 {
 		for _, m := range vmg.Spec.BootOrder[0].Members {
 			currentMemberNames[m.Name] = struct{}{}
 		}
 	}
 
-	// 1. If removing members, allow immediately since it doesn't impact placement or placement annotation set.
+	// 2. If removing members, allow immediately since it doesn't impact placement or placement annotation set.
 	if len(targetMember) < len(currentMemberNames) {
 		logger.V(5).Info("Scaling down detected (fewer target members), allowing member update.")
-		return true, nil
+		return nil
 	}
 
-	// 2. If adding members, continue following checks.
+	// If adding members, continue following checks.
 	var newMembers []vmoprv1.GroupMember
 	for _, m := range targetMember {
 		if _, exists := currentMemberNames[m.Name]; !exists {
@@ -317,10 +316,9 @@ func isMemberUpdateAllowed(ctx context.Context, kubeClient client.Client, target
 		vsphereMachine := &vmwarev1.VSphereMachine{}
 		if err := kubeClient.Get(ctx, vsphereMachineKey, vsphereMachine); err != nil {
 			if apierrors.IsNotFound(err) {
-				logger.V(5).Info("VSphereMachine for new member not found, temporarily blocking update.", "VSphereMachineName", newMember.Name)
-				return false, nil
+				return errors.Wrapf(err, "VSphereMachine for new member %s not found, temporarily blocking member update", newMember.Name)
 			}
-			return false, errors.Wrapf(err, "failed to get VSphereMachine %s", klog.KRef(newMember.Name, vmg.Namespace))
+			return errors.Wrapf(err, "failed to get VSphereMachine %s", klog.KRef(newMember.Name, vmg.Namespace))
 		}
 
 		var machineOwnerName string
@@ -333,8 +331,7 @@ func isMemberUpdateAllowed(ctx context.Context, kubeClient client.Client, target
 
 		if machineOwnerName == "" {
 			// VSphereMachine found but owner Machine reference is missing
-			logger.V(5).Info("VSphereMachine found but owner Machine reference is missing, temporarily blocking update.", "VSphereMachineName", newMember.Name)
-			return false, nil
+			return errors.Wrapf(nil, "VSphereMachine %s found but owner Machine reference is missing, temporarily blocking member update", newMember.Name)
 		}
 
 		machineKey := types.NamespacedName{
@@ -345,46 +342,38 @@ func isMemberUpdateAllowed(ctx context.Context, kubeClient client.Client, target
 
 		if err := kubeClient.Get(ctx, machineKey, machine); err != nil {
 			if apierrors.IsNotFound(err) {
-				logger.V(5).Info("CAPI Machine not found via owner reference, temporarily blocking update.", "Machine", klog.KRef(machineOwnerName, vmg.Namespace))
-				return false, nil
+				return errors.Wrapf(err, "Machine %s not found via owner reference, temporarily blocking member update", klog.KRef(machineOwnerName, vmg.Namespace))
 			}
-			return false, errors.Wrapf(err, "failed to get CAPI Machine %s", klog.KRef(machineOwnerName, vmg.Namespace))
+			return errors.Wrapf(err, "failed to get CAPI Machine %s", klog.KRef(machineOwnerName, vmg.Namespace))
 		}
 
-		// If FailureDomain is set on CAPI Machine, placement process will be skipped. Allow update.
+		// If FailureDomain is set on CAPI Machine, placement process will be skipped. Allow update for this member.
 		fd := machine.Spec.FailureDomain
 		if fd != "" {
-			logger.V(5).Info("New member's Machine has FailureDomain specified. Allowing VMG update for this member.")
+			logger.V(5).Info("New member's Machine has FailureDomain specified. Allowing VMG update for this member.", "Member", newMember.Name)
 			continue
 		}
 
-		// If FailureDomain is NOT set. Requires placement or placement Annotation. Fall through to full VMG Annotation check.
-		logger.V(5).Info("New member's CAPI Machine lacks FailureDomain. Falling through to  VMG Annotation check.", "MachineName", machineOwnerName)
-
-		// If no Placement Annotations, skip member update and wait for it.
+		// 4. If FailureDomain is NOT set. Requires placement or placement Annotation. Fall through to Annotation check.
+		// If no Placement Annotations, block member update and wait for it.
 		annotations := vmg.GetAnnotations()
 		if len(annotations) == 0 {
-			return false, nil
+			return errors.Wrapf(nil, "waiting for placement annotation to update VMG member %s, temporarily blocking member update", newMember.Name)
 		}
 
 		mdLabelName := vsphereMachine.Labels[clusterv1.MachineDeploymentNameLabel]
 		if mdLabelName == "" {
-			return false, errors.Wrapf(nil, "VSphereMachine doesn't have MachineDeployment name label %s", klog.KObj(vsphereMachine))
+			return errors.Wrapf(nil, "VSphereMachine doesn't have MachineDeployment name label %s, blocking member update", klog.KObj(vsphereMachine))
 		}
 
 		annotationKey := fmt.Sprintf("%s/%s", ZoneAnnotationPrefix, mdLabelName)
-
 		if _, found := annotations[annotationKey]; !found {
-			logger.V(5).Info("Required placement annotation is missing.",
-				"Member", newMember, "Annotation", annotationKey)
-			return false, nil
+			return errors.Wrapf(nil, "waiting for placement annotation %s to update VMG member %s, temporarily blocking member update", annotationKey, newMember.Name)
 		}
-
-		logger.V(5).Info("New member requires placement annotation and it is present. Allowing this member.", "Member", newMember)
 	}
 
-	logger.V(5).Info("Either no new members, or all newly added members  existed or have satisfied placement requirements, allowing update.")
-	return true, nil
+	logger.V(5).Info("All newly added members either existed or have satisfied placement requirements, allowing member update.")
+	return nil
 }
 
 // getExpectedVSphereMachineCount get expected total count of Machines belonging to the Cluster.

controllers/vmware/virtualmachinegroup_reconciler_test.go

@@ -126,7 +126,6 @@ func TestIsMemberUpdateAllowed(t *testing.T) {
 			vmgInput:        baseVMG.DeepCopy(),
 			mdNames:         []string{mdName1},
 			existingObjects: nil,
-			wantAllowed:     true,
 			wantErr:         false,
 		},
 		{
@@ -144,8 +143,7 @@ func TestIsMemberUpdateAllowed(t *testing.T) {
 						}}}}
 				return []runtime.Object{v}
 			}(),
-			wantAllowed: true,
-			wantErr:     false,
+			wantErr: false,
 		},
 		{
 			name:         "Allow member update when VMG Ready and All Annotations Present",
@@ -168,11 +166,10 @@ func TestIsMemberUpdateAllowed(t *testing.T) {
 
 				return []runtime.Object{v}
 			}(),
-			wantAllowed: true,
-			wantErr:     false,
+			wantErr: false,
 		},
 		{
-			name:         "Skip member update if new member VSphereMachine Not Found",
+			name:         "Block member update if new member VSphereMachine Not Found",
 			targetMember: []vmoprv1.GroupMember{member(memberName1), member(memberName2)}, // vm-02 is new
 			vmgInput:     baseVMG.DeepCopy(),
 			mdNames:      []string{mdName1},
@@ -187,11 +184,10 @@ func TestIsMemberUpdateAllowed(t *testing.T) {
 				// vm-02 VSphereMachine is missing
 				return []runtime.Object{v, makeVSphereMachineOwned(memberName1, vmgNamespace, ownerMachineName1, mdName1), makeCAPIMachine(ownerMachineName1, vmgNamespace, ptr.To(failureDomainA))}
 			}(),
-			wantAllowed: false,
-			wantErr:     false,
+			wantErr: true,
 		},
 		{
-			name:         "Skip member update if VSphereMachine found but CAPI Machine missing",
+			name:         "Block member update if VSphereMachine found but CAPI Machine missing",
 			targetMember: []vmoprv1.GroupMember{member(memberName1), member(memberName2)}, // vm-02 is new
 			vmgInput:     baseVMG.DeepCopy(),
 			mdNames:      []string{mdName1},
@@ -206,8 +202,7 @@ func TestIsMemberUpdateAllowed(t *testing.T) {
 				// vm-02 VSphereMachine exists but has no owner ref
 				return []runtime.Object{v, makeVSphereMachineOwned(memberName1, vmgNamespace, "ownerMachineName1", mdName1), makeCAPIMachine("ownerMachineName1", vmgNamespace, ptr.To(failureDomainA)), makeVSphereMachineNoOwner(memberName2, vmgNamespace)}
 			}(),
-			wantAllowed: false,
-			wantErr:     false,
+			wantErr: true,
 		},
 		{
 			name:         "Allow member update if all new members have Machine FailureDomain specified",
@@ -222,15 +217,14 @@ func TestIsMemberUpdateAllowed(t *testing.T) {
 						Kind: memberKind,
 					},
 				}}}
-				// m-02 (owner of vownerMachineName2) has FailureDomain set
+				// m-02 (owner of ownerMachineName2) has FailureDomain set
 				return []runtime.Object{
 					v,
 					makeVSphereMachineOwned(memberName1, vmgNamespace, "ownerMachineName1", mdName1), makeCAPIMachine("ownerMachineName1", vmgNamespace, nil),
 					makeVSphereMachineOwned(memberName2, vmgNamespace, "ownerMachineName2", mdName2), makeCAPIMachine("ownerMachineName2", vmgNamespace, ptr.To(failureDomainA)),
 				}
 			}(),
-			wantAllowed: true, // Allowed because new members don't require VMO placement
-			wantErr:     false,
+			wantErr: false, // Allowed because new members don't require placement
 		},
 		{
 			name:         "Allow member update if no new member",
@@ -248,11 +242,10 @@ func TestIsMemberUpdateAllowed(t *testing.T) {
 				}}}
 				return []runtime.Object{v}
 			}(),
-			wantAllowed: true,
-			wantErr:     false,
+			wantErr: false,
 		},
 		{
-			name:         "Skip member update if new member Machine requires placement annotation",
+			name:         "Block member update if new member Machine requires placement annotation",
 			targetMember: []vmoprv1.GroupMember{member(memberName1), member(memberName2)}, // vm-02 is new and requires placement
 			vmgInput:     baseVMG.DeepCopy(),
 			mdNames:      []string{mdName1},
@@ -271,8 +264,7 @@ func TestIsMemberUpdateAllowed(t *testing.T) {
 					makeVSphereMachineOwned(memberName2, vmgNamespace, "ownerMachineName2", mdName2), makeUnplacedCAPIMachine("ownerMachineName2", vmgNamespace),
 				}
 			}(),
-			wantAllowed: false,
-			wantErr:     false,
+			wantErr: true,
 		},
 		{
 			name:         "Allow new member Machine since required placement annotation exists",
@@ -297,8 +289,7 @@ func TestIsMemberUpdateAllowed(t *testing.T) {
 					makeVSphereMachineOwned(memberName2, vmgNamespace, "ownerMachineName2", mdName2), makeUnplacedCAPIMachine("ownerMachineName2", vmgNamespace),
 				}
 			}(),
-			wantAllowed: true,
-			wantErr:     false,
+			wantErr: false,
 		},
 	}
 
@@ -311,13 +302,12 @@ func TestIsMemberUpdateAllowed(t *testing.T) {
 
 			vmgInput := tt.vmgInput.DeepCopy()
 
-			gotAllowed, err := isMemberUpdateAllowed(ctx, kubeClient, tt.targetMember, vmgInput)
+			err := isMemberUpdateAllowed(ctx, kubeClient, tt.targetMember, vmgInput)
 
 			if tt.wantErr {
 				g.Expect(err).To(HaveOccurred())
 			} else {
 				g.Expect(err).NotTo(HaveOccurred())
-				g.Expect(gotAllowed).To(Equal(tt.wantAllowed))
 			}
 		})
 	}