Merge pull request #1219 from srm09/bug/fd-host-placement

Failure Domain: Fixes VM Placement on Hosts

Author: k8s-ci-robot

controllers/vspheredeploymentzone_controller_domain.go

@@ -176,7 +176,7 @@ func (r vsphereDeploymentZoneReconciler) createAndAttachMetadata(ctx *context.VS
 		logger.V(4).Info("attaching tag to object")
 		err := obj.AttachTag(ctx, failureDomain.Name)
 		if err != nil {
-			logger.V(4).Error(err, "failed to find object", obj)
+			logger.V(4).Error(err, "failed to find object")
 			errList = append(errList, errors.Wrapf(err, "failed to attach tag"))
 		}
 	}

controllers/vspheremachine_controller.go

@@ -335,11 +335,6 @@ func (r machineReconciler) reconcileNormal(ctx *context.MachineContext) (reconci
 		return reconcile.Result{}, nil
 	}
 
-	// Propagating the failure domain name to the VSphereMachine object
-	if failureDomain := ctx.Machine.Spec.FailureDomain; failureDomain != nil {
-		ctx.VSphereMachine.Spec.FailureDomain = failureDomain
-	}
-
 	// TODO(akutz) Determine the version of vSphere.
 	vm, err := r.reconcileNormalPre7(ctx, vsphereVM)
 	if err != nil {
@@ -517,7 +512,7 @@ func (r machineReconciler) generateOverrideFunc(ctx *context.MachineContext) (fu
 
 		for index := range vsphereDeploymentZoneList.Items {
 			zone := vsphereDeploymentZoneList.Items[index]
-			if zone.Spec.FailureDomain == *ctx.Machine.Spec.FailureDomain {
+			if zone.Spec.FailureDomain == *failureDomainName {
 				overrideWithFailureDomainFunc = func(vm *infrav1.VSphereVM) {
 					vm.Spec.Server = zone.Spec.Server
 					vm.Spec.Datacenter = vsphereFailureDomain.Spec.Topology.Datacenter

controllers/vspherevm_controller_test.go

@@ -1,3 +1,19 @@
+/*
+Copyright 2021 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
 package controllers
 
 import (

pkg/services/govmomi/cluster/cluster_suite_test.go

@@ -23,7 +23,6 @@ import (
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
 
-	"github.com/go-logr/logr"
 	"github.com/vmware/govmomi/find"
 	"sigs.k8s.io/cluster-api-provider-vsphere/pkg/session"
 )
@@ -41,7 +40,3 @@ type testComputeClusterCtx struct {
 func (t testComputeClusterCtx) GetSession() *session.Session {
 	return &session.Session{Finder: t.finder}
 }
-
-func (t testComputeClusterCtx) GetLogger() logr.Logger {
-	return logr.DiscardLogger{}
-}

pkg/services/govmomi/cluster/rule.go

@@ -50,9 +50,6 @@ func negate(input bool) bool {
 }
 
 func VerifyAffinityRule(ctx computeClusterContext, clusterName, hostGroupName, vmGroupName string) (Rule, error) {
-	logger := ctx.GetLogger().WithValues("compute cluster", clusterName, "VM Group", vmGroupName, "Host Group", hostGroupName)
-
-	logger.V(4).Info("listing affinity rules")
 	rules, err := listRules(ctx, clusterName)
 	if err != nil {
 		return nil, errors.Wrapf(err, "unable to list rules for compute cluster %s", clusterName)
@@ -62,7 +59,6 @@ func VerifyAffinityRule(ctx computeClusterContext, clusterName, hostGroupName, v
 		if vmHostRuleInfo, ok := rule.(*types.ClusterVmHostRuleInfo); ok {
 			if vmHostRuleInfo.AffineHostGroupName == hostGroupName &&
 				vmHostRuleInfo.VmGroupName == vmGroupName {
-				logger.V(4).Info("found matching VM Host affinity rule")
 				return vmHostAffinityRule{vmHostRuleInfo}, nil
 			}
 		}

pkg/services/govmomi/cluster/service.go

@@ -19,7 +19,6 @@ package cluster
 import (
 	"context"
 
-	"github.com/go-logr/logr"
 	"github.com/vmware/govmomi/object"
 	"github.com/vmware/govmomi/vim25/types"
 
@@ -30,8 +29,6 @@ type computeClusterContext interface {
 	context.Context
 
 	GetSession() *session.Session
-
-	GetLogger() logr.Logger
 }
 
 func ListHostsFromGroup(ctx context.Context, ccr *object.ClusterComputeResource, hostGroup string) ([]object.Reference, error) {
@@ -53,17 +50,3 @@ func ListHostsFromGroup(ctx context.Context, ccr *object.ClusterComputeResource,
 	}
 	return refs, nil
 }
-
-// TODO(srm09): Consider deferring the reconciliation of the task completion to a separate loop.
-func reconfigure(ctx context.Context, ccr *object.ClusterComputeResource, spec types.BaseComputeResourceConfigSpec) error {
-	reconfigureTask, err := ccr.Reconfigure(ctx, spec, true)
-	if err != nil {
-		return err
-	}
-
-	err = reconfigureTask.Wait(ctx)
-	if err != nil {
-		return err
-	}
-	return nil
-}

pkg/services/govmomi/cluster/vmgroup.go

@@ -19,59 +19,67 @@ package cluster
 import (
 	"context"
 
+	"github.com/pkg/errors"
 	"github.com/vmware/govmomi/object"
 	"github.com/vmware/govmomi/vim25/types"
 )
 
-func AddVMToGroup(ctx computeClusterContext, clusterName, vmGroupName, vm string) error {
+func FindVMGroup(ctx computeClusterContext, clusterName, vmGroupName string) (*VMGroup, error) {
 	ccr, err := ctx.GetSession().Finder.ClusterComputeResource(ctx, clusterName)
 	if err != nil {
-		return err
+		return nil, err
 	}
 
-	vms, err := listVMs(ctx, ccr, vmGroupName)
+	clusterConfigInfoEx, err := ccr.Configuration(ctx)
 	if err != nil {
-		return err
+		return nil, err
 	}
-
-	vmObj, err := ctx.GetSession().Finder.VirtualMachine(ctx, vm)
-	if err != nil {
-		return err
+	for _, group := range clusterConfigInfoEx.Group {
+		if clusterVMGroup, ok := group.(*types.ClusterVmGroup); ok {
+			if clusterVMGroup.Name == vmGroupName {
+				return &VMGroup{ccr, clusterVMGroup}, nil
+			}
+		}
 	}
-	vms = append(vms, vmObj.Reference())
+	return nil, errors.Errorf("cannot find VM group %s", vmGroupName)
+}
+
+// VMGroup represents a VSphere VM Group object
+type VMGroup struct {
+	*object.ClusterComputeResource
+	*types.ClusterVmGroup
+}
+
+// Add a VSphere VM object to the VM Group
+func (vg VMGroup) Add(ctx context.Context, vmObj types.ManagedObjectReference) (*object.Task, error) {
+	vms := vg.listVMs()
+	vg.ClusterVmGroup.Vm = append(vms, vmObj) //nolint:gocritic
 
-	info := &types.ClusterVmGroup{
-		ClusterGroupInfo: types.ClusterGroupInfo{
-			Name: vmGroupName,
-		},
-		Vm: vms,
-	}
 	spec := &types.ClusterConfigSpecEx{
 		GroupSpec: []types.ClusterGroupSpec{
 			{
 				ArrayUpdateSpec: types.ArrayUpdateSpec{
 					Operation: types.ArrayUpdateOperationEdit,
 				},
-				Info: info,
+				Info: vg.ClusterVmGroup,
 			},
 		},
 	}
-	return reconfigure(ctx, ccr, spec)
+	return vg.ClusterComputeResource.Reconfigure(ctx, spec, true)
 }
 
-func listVMs(ctx context.Context, ccr *object.ClusterComputeResource, vmGroupName string) ([]types.ManagedObjectReference, error) {
-	clusterConfigInfoEx, err := ccr.Configuration(ctx)
-	if err != nil {
-		return nil, err
-	}
+// HasVM returns whether a VSphere VM object is a member of the VM Group
+func (vg VMGroup) HasVM(vmObj types.ManagedObjectReference) (bool, error) {
+	vms := vg.listVMs()
 
-	var refs []types.ManagedObjectReference
-	for _, group := range clusterConfigInfoEx.Group {
-		if clusterVMGroup, ok := group.(*types.ClusterVmGroup); ok {
-			if clusterVMGroup.Name == vmGroupName {
-				return clusterVMGroup.Vm, nil
-			}
+	for _, vm := range vms {
+		if vm == vmObj {
+			return true, nil
 		}
 	}
-	return refs, nil
+	return false, nil
+}
+
+func (vg VMGroup) listVMs() []types.ManagedObjectReference {
+	return vg.ClusterVmGroup.Vm
 }

pkg/services/govmomi/cluster/vmgroup_test.go

@@ -28,14 +28,12 @@ import (
 	"sigs.k8s.io/cluster-api-provider-vsphere/test/helpers"
 )
 
-func TestAddVMToGroup(t *testing.T) {
+func Test_VMGroup(t *testing.T) {
 	g := NewWithT(t)
 	sim, err := helpers.VCSimBuilder().
-		WithOperations("cluster.group.create -cluster DC0_C0 -name blah-vm-group -vm").
+		WithOperations("cluster.group.create -cluster DC0_C0 -name blah-vm-group -vm DC0_C0_RP0_VM0 DC0_C0_RP0_VM1").
 		Build()
-	if err != nil {
-		t.Fatalf("failed to create a VC simulator object %s", err)
-	}
+	g.Expect(err).NotTo(HaveOccurred())
 	defer sim.Destroy()
 
 	ctx := context.Background()
@@ -50,14 +48,31 @@ func TestAddVMToGroup(t *testing.T) {
 		finder:  finder,
 	}
 
+	computeClusterName := "DC0_C0"
 	vmGroupName := "blah-vm-group"
-	g.Expect(AddVMToGroup(computeClusterCtx, "DC0_C0", vmGroupName, "DC0_H0_VM0")).To(Succeed())
-	g.Expect(AddVMToGroup(computeClusterCtx, "DC0_C0", vmGroupName, "DC0_H0_VM1")).To(Succeed())
 
-	ccr, err := finder.ClusterComputeResource(ctx, "DC0_C0")
+	vmGrp, err := FindVMGroup(computeClusterCtx, computeClusterName, vmGroupName)
 	g.Expect(err).NotTo(HaveOccurred())
+	g.Expect(vmGrp.listVMs()).To(HaveLen(2))
 
-	refs, err := listVMs(ctx, ccr, "blah-vm-group")
+	vmObjOne, err := finder.VirtualMachine(ctx, "DC0_H0_VM0")
 	g.Expect(err).NotTo(HaveOccurred())
-	g.Expect(refs).To(HaveLen(2))
+	vmRef := vmObjOne.Reference()
+
+	hasVM, err := vmGrp.HasVM(vmRef)
+	g.Expect(err).NotTo(HaveOccurred())
+	g.Expect(hasVM).To(BeFalse())
+
+	task, err := vmGrp.Add(computeClusterCtx, vmRef)
+	g.Expect(err).NotTo(HaveOccurred())
+	g.Expect(task.Wait(ctx)).To(Succeed())
+	g.Expect(vmGrp.listVMs()).To(HaveLen(3))
+
+	hasVM, err = vmGrp.HasVM(vmRef)
+	g.Expect(err).NotTo(HaveOccurred())
+	g.Expect(hasVM).To(BeTrue())
+
+	vmGroupName = "incorrect-vm-group"
+	_, err = FindVMGroup(computeClusterCtx, computeClusterName, vmGroupName)
+	g.Expect(err).To(HaveOccurred())
 }

pkg/services/govmomi/metadata/metadata.go

@@ -47,26 +47,36 @@ func getCategoryAssociableType(domainType infrav1.FailureDomainType) string {
 	}
 }
 
+// CreateCategory either creates a new vSphere category or updates the associable type for an existing category.
 func CreateCategory(ctx metadataContext, name string, failureDomainType infrav1.FailureDomainType) (string, error) {
 	logger := ctx.GetLogger().WithValues("category", name)
 	manager := ctx.GetSession().TagManager
 	category, err := manager.GetCategory(ctx, name)
 	if err != nil {
 		logger.V(4).Info("failed to find existing category, creating a new category")
-		id, err := manager.CreateCategory(ctx, &tags.Category{
-			Name:            name,
-			Description:     "CAPV generated category for Failure Domain support",
-			AssociableTypes: []string{getCategoryAssociableType(failureDomainType)},
-			Cardinality:     "MULTIPLE",
-		})
+		id, err := manager.CreateCategory(ctx, getCategoryObject(name, failureDomainType))
 		if err != nil {
 			return "", err
 		}
 		return id, nil
 	}
+	category.Patch(getCategoryObject(name, failureDomainType))
+	if err := manager.UpdateCategory(ctx, category); err != nil {
+		logger.V(4).Error(err, "failed to update existing category")
+		return "", err
+	}
 	return category.ID, nil
 }
 
+func getCategoryObject(name string, failureDomainType infrav1.FailureDomainType) *tags.Category {
+	return &tags.Category{
+		Name:            name,
+		Description:     "CAPV generated category for Failure Domain support",
+		AssociableTypes: []string{getCategoryAssociableType(failureDomainType)},
+		Cardinality:     "MULTIPLE",
+	}
+}
+
 func CreateTag(ctx metadataContext, name, categoryID string) error {
 	logger := ctx.GetLogger().WithValues("tag", name, "category", categoryID)
 	manager := ctx.GetSession().TagManager

pkg/services/govmomi/service.go

@@ -132,11 +132,11 @@ func (vms *VMService) ReconcileVM(ctx *context.VMContext) (vm infrav1.VirtualMac
 		return vm, err
 	}
 
-	if ok, err := vms.reconcilePowerState(vmCtx); err != nil || !ok {
+	if ok, err := vms.reconcileVMGroupInfo(vmCtx); err != nil || !ok {
 		return vm, err
 	}
 
-	if err := vms.reconcileVMGroupInfo(ctx); err != nil {
+	if ok, err := vms.reconcilePowerState(vmCtx); err != nil || !ok {
 		return vm, err
 	}
 
@@ -490,15 +490,31 @@ func (vms *VMService) getBootstrapData(ctx *context.VMContext) ([]byte, error) {
 	return value, nil
 }
 
-func (vms *VMService) reconcileVMGroupInfo(ctx *context.VMContext) error {
-	if ctx.VSphereFailureDomain != nil {
-		topology := ctx.VSphereFailureDomain.Spec.Topology
-		if topology.Hosts != nil {
-			return cluster.AddVMToGroup(ctx,
-				*topology.ComputeCluster,
-				topology.Hosts.VMGroupName,
-				ctx.VSphereVM.Name)
+func (vms *VMService) reconcileVMGroupInfo(ctx *virtualMachineContext) (bool, error) {
+	if ctx.VSphereFailureDomain == nil || ctx.VSphereFailureDomain.Spec.Topology.Hosts == nil {
+		ctx.Logger.Info("hosts topology in failure domain not defined. skipping reconcile VM group")
+		return true, nil
+	}
+
+	topology := ctx.VSphereFailureDomain.Spec.Topology
+	vmGroup, err := cluster.FindVMGroup(ctx, *topology.ComputeCluster, topology.Hosts.VMGroupName)
+	if err != nil {
+		return false, errors.Wrapf(err, "unable to find VM Group %s", topology.Hosts.VMGroupName)
+	}
+
+	hasVM, err := vmGroup.HasVM(ctx.Ref)
+	if err != nil {
+		return false, errors.Wrapf(err, "unable to find VM Group %s membership", topology.Hosts.VMGroupName)
+	}
+
+	if !hasVM {
+		task, err := vmGroup.Add(ctx, ctx.Ref)
+		if err != nil {
+			return false, errors.Wrapf(err, "failed to add VM %s to VM group", ctx.VSphereVM.Name)
 		}
+		ctx.VSphereVM.Status.TaskRef = task.Reference().Value
+		ctx.Logger.Info("wait for VM to be added to group")
+		return false, nil
 	}
-	return nil
+	return true, nil
 }