Refine godoc

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

Author: zhanggbj

controllers/vmware/virtualmachinegroup_reconciler.go

@@ -40,7 +40,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
 	vmwarev1 "sigs.k8s.io/cluster-api-provider-vsphere/apis/vmware/v1beta1"
-	infrautilv1 "sigs.k8s.io/cluster-api-provider-vsphere/pkg/util"
+	"sigs.k8s.io/cluster-api-provider-vsphere/pkg/services/vmoperator"
 )
 
 const (
@@ -167,7 +167,7 @@ func (r *VirtualMachineGroupReconciler) createOrUpdateVirtualMachineGroup(ctx co
 	// Generate VM names according to the naming strategy set on the VSphereMachine.
 	vmNames := make([]string, 0, len(currentVSphereMachines))
 	for _, machine := range currentVSphereMachines {
-		name, err := GenerateVirtualMachineName(machine.Name, machine.Spec.NamingStrategy)
+		name, err := vmoperator.GenerateVirtualMachineName(machine.Name, machine.Spec.NamingStrategy)
 		if err != nil {
 			return reconcile.Result{}, err
 		}
@@ -536,20 +536,3 @@ func generateVirtualMachineGroupAnnotations(ctx context.Context, kubeClient clie
 
 	return nil
 }
-
-// GenerateVirtualMachineName generates the name of a VirtualMachine based on the naming strategy.
-// Duplicated this logic from pkg/services/vmoperator/vmopmachine.go.
-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 }}")
-	if namingStrategy == nil || namingStrategy.Template == nil {
-		// Note: No need to trim to max length in this case as valid Machine names will also be valid VirtualMachine names.
-		return machineName, nil
-	}
-
-	name, err := infrautilv1.GenerateMachineNameFromTemplate(machineName, namingStrategy.Template)
-	if err != nil {
-		return "", errors.Wrapf(err, "failed to generate name for VirtualMachine %s", machineName)
-	}
-
-	return name, nil
-}

controllers/vmware/virtualmachinegroup_reconciler_test.go

@@ -121,11 +121,9 @@ func TestIsMemberUpdateAllowed(t *testing.T) {
 		wantErr         bool
 	}{
 		{
-			name:         "Allow member update if VirtualMachineGroup not existed",
-			targetMember: []vmoprv1.GroupMember{member(memberName1)},
-			vmgInput: &vmoprv1.VirtualMachineGroup{
-				ObjectMeta: metav1.ObjectMeta{Name: vmgName, Namespace: vmgNamespace},
-			},
+			name:            "Allow member update if VirtualMachineGroup not existed",
+			targetMember:    []vmoprv1.GroupMember{member(memberName1)},
+			vmgInput:        baseVMG.DeepCopy(),
 			mdNames:         []string{mdName1},
 			existingObjects: nil,
 			wantAllowed:     true,
@@ -134,17 +132,8 @@ func TestIsMemberUpdateAllowed(t *testing.T) {
 		{
 			name:         "Allow member update if it is removing",
 			targetMember: []vmoprv1.GroupMember{},
-			vmgInput: func() *vmoprv1.VirtualMachineGroup {
-				v := baseVMG.DeepCopy()
-				v.Spec.BootOrder = []vmoprv1.VirtualMachineGroupBootOrderGroup{
-					{Members: []vmoprv1.GroupMember{
-						{
-							Name: memberName1,
-							Kind: memberKind,
-						}}}}
-				return v
-			}(),
-			mdNames: []string{mdName1},
+			vmgInput:     baseVMG.DeepCopy(),
+			mdNames:      []string{mdName1},
 			existingObjects: func() []runtime.Object {
 				v := baseVMG.DeepCopy()
 				v.Spec.BootOrder = []vmoprv1.VirtualMachineGroupBootOrderGroup{
@@ -328,7 +317,7 @@ func TestIsMemberUpdateAllowed(t *testing.T) {
 				g.Expect(err).To(HaveOccurred())
 			} else {
 				g.Expect(err).NotTo(HaveOccurred())
-				g.Expect(gotAllowed).To(Equal(true))
+				g.Expect(gotAllowed).To(Equal(tt.wantAllowed))
 			}
 		})
 	}

pkg/services/vmoperator/vmopmachine.go

@@ -199,15 +199,31 @@ func (v *VmopMachineService) ReconcileNormal(ctx context.Context, machineCtx cap
 	// Set the VM state. Will get reset throughout the reconcile
 	supervisorMachineCtx.VSphereMachine.Status.VMStatus = vmwarev1.VirtualMachineStatePending
 
+	// Get the VirtualMachine object Key
+	vmOperatorVM := &vmoprv1.VirtualMachine{}
+	vmKey, err := virtualMachineObjectKey(supervisorMachineCtx.Machine.Name, supervisorMachineCtx.Machine.Namespace, supervisorMachineCtx.VSphereMachine.Spec.NamingStrategy)
+	if err != nil {
+		return false, err
+	}
+
+	// When creating a new cluster and the user doesn't provide info about placement of VMs in a specific failure domain,
+	// CAPV will define affinity rules to ensure proper placement of the machine.
+	//
+	// - All the machines belonging to the same MachineDeployment should be placed in the same failure domain - required.
+	// - All the machines belonging to the same MachineDeployment should be spread across esxi hosts in the same failure domain - best-efforts.
+	// - Different MachineDeployments and corresponding VMs should be spread across failure domains - best-efforts.
+	//
+	// Note: Control plane VM placement doesn't follow the above rules, and the assumption
+	// is that failureDomain is always set for control plane VMs.
 	var affInfo *affinityInfo
 	if feature.Gates.Enabled(feature.NodeAutoPlacement) &&
 		!infrautilv1.IsControlPlaneMachine(machineCtx.GetVSphereMachine()) {
-		vmOperatorVMGroup := &vmoprv1.VirtualMachineGroup{}
+		vmGroup := &vmoprv1.VirtualMachineGroup{}
 		key := client.ObjectKey{
 			Namespace: supervisorMachineCtx.Cluster.Namespace,
 			Name:      supervisorMachineCtx.Cluster.Name,
 		}
-		err := v.Client.Get(ctx, key, vmOperatorVMGroup)
+		err := v.Client.Get(ctx, key, vmGroup)
 		if err != nil {
 			if !apierrors.IsNotFound(err) {
 				return false, err
@@ -222,11 +238,8 @@ func (v *VmopMachineService) ReconcileNormal(ctx context.Context, machineCtx cap
 			return true, nil
 		}
 
-		// Proceed only if the machine is a member of the VirtualMachineGroup.
-		isMember, err := v.checkVirtualMachineGroupMembership(vmOperatorVMGroup, supervisorMachineCtx)
-		if err != nil {
-			return true, errors.Wrapf(err, "%s", fmt.Sprintf("failed to check if VirtualMachine %s is a member of VirtualMachineGroup %s", supervisorMachineCtx.VSphereMachine.Name, klog.KObj(vmOperatorVMGroup)))
-		}
+		// Proceed only if the VSphereMachine is a member of the VirtualMachineGroup.
+		isMember := v.checkVirtualMachineGroupMembership(vmGroup, vmKey.Name)
 		if !isMember {
 			v1beta2conditions.Set(supervisorMachineCtx.VSphereMachine, metav1.Condition{
 				Type:   infrav1.VSphereMachineVirtualMachineProvisionedV1Beta2Condition,
@@ -238,18 +251,19 @@ func (v *VmopMachineService) ReconcileNormal(ctx context.Context, machineCtx cap
 		}
 
 		affInfo = &affinityInfo{
-			vmGroupName: vmOperatorVMGroup.Name,
+			vmGroupName: vmGroup.Name,
 		}
 
 		// Set the zone label using the annotation of the per-md zone mapping from VirtualMachineGroup.
 		// This is for new VMs created during day-2 operations when Node Auto Placement is enabled.
 		mdName := supervisorMachineCtx.Machine.Labels[clusterv1.MachineDeploymentNameLabel]
-		if fd, ok := vmOperatorVMGroup.Annotations[fmt.Sprintf("%s/%s", ZoneAnnotationPrefix, mdName)]; ok && fd != "" {
+		if fd, ok := vmGroup.Annotations[fmt.Sprintf("%s/%s", ZoneAnnotationPrefix, mdName)]; ok && fd != "" {
 			affInfo.failureDomain = fd
 		}
 
-		// Fetch machine deployments without explicit failureDomain specified
-		// to use when setting the anti-affinity rules.
+		// VM in a MachineDeployment ideally should be placed in a different failure domain than VMs
+		// in other MachineDeployments.
+		// In order to do so, collect names of all the MachineDeployments except the one the VM belongs to.
 		machineDeployments := &clusterv1.MachineDeploymentList{}
 		if err := v.Client.List(ctx, machineDeployments,
 			client.InNamespace(supervisorMachineCtx.Cluster.Namespace),
@@ -266,6 +280,7 @@ func (v *VmopMachineService) ReconcileNormal(ctx context.Context, machineCtx cap
 
 		affInfo.affinitySpec = vmoprv1.AffinitySpec{
 			VMAffinity: &vmoprv1.VMAffinitySpec{
+				// All the machines belonging to the same MachineDeployment should be placed in the same failure domain - required.
 				RequiredDuringSchedulingPreferredDuringExecution: []vmoprv1.VMAffinityTerm{
 					{
 						LabelSelector: &metav1.LabelSelector{
@@ -278,6 +293,7 @@ func (v *VmopMachineService) ReconcileNormal(ctx context.Context, machineCtx cap
 				},
 			},
 			VMAntiAffinity: &vmoprv1.VMAntiAffinitySpec{
+				// All the machines belonging to the same MachineDeployment should be spread across esxi hosts in the same failure domain - best-efforts.
 				PreferredDuringSchedulingPreferredDuringExecution: []vmoprv1.VMAffinityTerm{
 					{
 						LabelSelector: &metav1.LabelSelector{
@@ -291,6 +307,7 @@ func (v *VmopMachineService) ReconcileNormal(ctx context.Context, machineCtx cap
 			},
 		}
 		if len(othermMDNames) > 0 {
+			// Different MachineDeployments and corresponding VMs should be spread across failure domains - best-efforts.
 			affInfo.affinitySpec.VMAntiAffinity.PreferredDuringSchedulingPreferredDuringExecution = append(
 				affInfo.affinitySpec.VMAntiAffinity.PreferredDuringSchedulingPreferredDuringExecution,
 				vmoprv1.VMAffinityTerm{
@@ -309,25 +326,22 @@ func (v *VmopMachineService) ReconcileNormal(ctx context.Context, machineCtx cap
 		}
 	}
 
+	// If the failureDomain is explicitly define for a machine, forward this info to the VM.
+	// Note: for consistency, affinity rules will be set on all the VMs, no matter if they are explicitly assigned to a failureDomain or not.
 	if supervisorMachineCtx.Machine.Spec.FailureDomain != "" {
 		supervisorMachineCtx.VSphereMachine.Spec.FailureDomain = ptr.To(supervisorMachineCtx.Machine.Spec.FailureDomain)
 	}
 
 	// Check for the presence of an existing object
-	vmOperatorVM := &vmoprv1.VirtualMachine{}
-	key, err := virtualMachineObjectKey(supervisorMachineCtx.Machine.Name, supervisorMachineCtx.Machine.Namespace, supervisorMachineCtx.VSphereMachine.Spec.NamingStrategy)
-	if err != nil {
-		return false, err
-	}
-	if err := v.Client.Get(ctx, *key, vmOperatorVM); err != nil {
+	if err := v.Client.Get(ctx, *vmKey, vmOperatorVM); err != nil {
 		if !apierrors.IsNotFound(err) {
 			return false, err
 		}
 		// Define the VM Operator VirtualMachine resource to reconcile.
 		vmOperatorVM = &vmoprv1.VirtualMachine{
 			ObjectMeta: metav1.ObjectMeta{
-				Name:      key.Name,
-				Namespace: key.Namespace,
+				Name:      vmKey.Name,
+				Namespace: vmKey.Namespace,
 			},
 		}
 	}
@@ -988,18 +1002,14 @@ func getMachineDeploymentNameForCluster(cluster *clusterv1.Cluster) string {
 }
 
 // checkVirtualMachineGroupMembership checks if the machine is in the first boot order group
-// and performs logic if a match is found.
-func (v *VmopMachineService) checkVirtualMachineGroupMembership(vmOperatorVMGroup *vmoprv1.VirtualMachineGroup, supervisorMachineCtx *vmware.SupervisorMachineContext) (bool, error) {
+// and performs logic if a match is found, as first boot order contains all the worker VMs.
+func (v *VmopMachineService) checkVirtualMachineGroupMembership(vmOperatorVMGroup *vmoprv1.VirtualMachineGroup, virtualMachineName string) bool {
 	if len(vmOperatorVMGroup.Spec.BootOrder) > 0 {
 		for _, member := range vmOperatorVMGroup.Spec.BootOrder[0].Members {
-			virtualMachineName, err := GenerateVirtualMachineName(supervisorMachineCtx.Machine.Name, supervisorMachineCtx.VSphereMachine.Spec.NamingStrategy)
-			if err != nil {
-				return false, err
-			}
 			if member.Name == virtualMachineName {
-				return true, nil
+				return true
 			}
 		}
 	}
-	return false, nil
+	return false
 }