Refine VMG member update to avoid race condition

Signed-off-by: Gong Zhang <gong.zhang@broadcom.com>

Author: zhanggbj

controllers/vmware/virtualmachinegroup_reconciler.go

@@ -199,6 +199,27 @@ func (r *VirtualMachineGroupReconciler) createOrUpdateVirtualMachineGroup(ctx co
 		}
 	}
 
+	// The core purpose of isCreateOrPatchAllowed 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 added under the group.
+	//s
+	// The CreateOrPatch is allowed if:
+	// 1. The VirtualMachineGroup is being initially created.
+	// 2. The update is a scale-down operation.
+	// 3. The VirtualMachineGroup is placement Ready.
+	// 4. The new member's underlying CAPI Machine has a FailureDomain set (will skip placement process).
+	// 5. 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.
+	err = isCreateOrPatchAllowed(ctx, r.Client, members, vmg)
+	if err != nil {
+		return reconcile.Result{}, err
+	}
+
 	// Use CreateOrPatch to create or update the VirtualMachineGroup.
 	_, err = controllerutil.CreateOrPatch(ctx, r.Client, vmg, func() error {
 		return r.reconcileVirtualMachineGroup(ctx, vmg, cluster, members, mdNames)
@@ -227,31 +248,10 @@ 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:
-	//
-	// 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.
-	//
-	// 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)
-	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) {
+// isCreateOrPatchAllowed checks if a VirtualMachineGroup is allowd to create or patch by check if BootOrder.Members update is allowed.
+func isCreateOrPatchAllowed(ctx context.Context, kubeClient client.Client, targetMember []vmoprv1.GroupMember, vmg *vmoprv1.VirtualMachineGroup) error {
 	logger := log.FromContext(ctx)
 	key := client.ObjectKey{
 		Namespace: vmg.Namespace,
@@ -275,38 +274,48 @@ 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 CreateOrPatch as it should be in initial creation phase.
+			logger.V(6).Info("VirtualMachineGroup not created yet, allowing create")
+			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 patch", 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
+		logger.V(6).Info("Scaling down detected (fewer target members), allowing patch.")
+		return nil
 	}
 
-	// 2. If adding members, continue following checks.
 	var newMembers []vmoprv1.GroupMember
 	for _, m := range targetMember {
 		if _, exists := currentMemberNames[m.Name]; !exists {
 			newMembers = append(newMembers, m)
 		}
 	}
 
-	// 3. Check newly added members for Machine.Spec.FailureDomain via VSphereMachine.If a member belongs to a Machine
+	// If no new member added, allow patch.
+	if len(newMembers) == 0 {
+		logger.V(6).Info("No new member detected, allowing patch.")
+		return nil
+	}
+
+	// 3. If initial placement is still in progress, block adding new member.
+	if !conditions.IsTrue(vmg, vmoprv1.ReadyConditionType) {
+		return fmt.Errorf("waiting for VirtualMachineGroup %s to get condition %s to true, temporarily blocking patch", klog.KObj(vmg), vmoprv1.ReadyConditionType)
+	}
+
+	// 4. Check newly added members for Machine.Spec.FailureDomain via VSphereMachine.If a member belongs to a Machine
 	// which has failureDomain specified, allow it since it will skip the placement
 	// process. If not, continue to check if the belonging MachineDeployment has got placement annotation.
 	for _, newMember := range newMembers {
@@ -317,10 +326,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 patch", 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 +341,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 fmt.Errorf("VSphereMachine %s found but owner Machine reference is missing, temporarily blocking patch", newMember.Name)
 		}
 
 		machineKey := types.NamespacedName{
@@ -345,46 +352,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 patch", 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(6).Info("New member's Machine has FailureDomain specified. Allowing patch", "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.
+		// 5. 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 fmt.Errorf("waiting for placement annotation to add VMG member %s, temporarily blocking patch", 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 fmt.Errorf("VSphereMachine doesn't have MachineDeployment name label %s, blocking patch", 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 fmt.Errorf("waiting for placement annotation %s to add VMG member %s, temporarily blocking patch", 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(6).Info("All newly added members either existed or have satisfied placement requirements, allowing patch")
+	return nil
 }
 
 // getExpectedVSphereMachineCount get expected total count of Machines belonging to the Cluster.