Merge pull request vmware-tanzu#821 from bryanv/bryanv/better-handle-…

…vmsetrp-delete 🌱 Misc VirtualMachineSetResourcePolicy improvements
abaruni · Dec 18, 2024 · 9d40af0 · 9d40af0
2 parents 62935ba + 6ec16d4
commit 9d40af0
Show file tree

Hide file tree

Showing 6 changed files with 84 additions and 52 deletions.
diff --git a/pkg/providers/vsphere/vmprovider_resourcepolicy.go b/pkg/providers/vsphere/vmprovider_resourcepolicy.go
@@ -32,14 +32,20 @@ func (vs *vSphereVMProvider) IsVirtualMachineSetResourcePolicyReady(
 		return false, err
 	}
 
-	folderExists, err := vcenter.DoesChildFolderExist(ctx, client.VimClient(), folderMoID, resourcePolicy.Spec.Folder)
-	if err != nil {
-		return false, err
+	folderExists := true
+	if folderName := resourcePolicy.Spec.Folder; folderName != "" {
+		folderExists, err = vcenter.DoesChildFolderExist(ctx, client.VimClient(), folderMoID, folderName)
+		if err != nil {
+			return false, err
+		}
 	}
 
-	rpExists, err := vcenter.DoesChildResourcePoolExist(ctx, client.VimClient(), rpMoID, resourcePolicy.Spec.ResourcePool.Name)
-	if err != nil {
-		return false, err
+	rpExists := true
+	if rpName := resourcePolicy.Spec.ResourcePool.Name; rpName != "" {
+		rpExists, err = vcenter.DoesChildResourcePoolExist(ctx, client.VimClient(), rpMoID, rpName)
+		if err != nil {
+			return false, err
+		}
 	}
 
 	clusterRef, err := vcenter.GetResourcePoolOwnerMoRef(ctx, client.VimClient(), rpMoID)
@@ -67,7 +73,7 @@ func (vs *vSphereVMProvider) CreateOrUpdateVirtualMachineSetResourcePolicy(
 	ctx context.Context,
 	resourcePolicy *vmopv1.VirtualMachineSetResourcePolicy) error {
 
-	folderMoID, rpMoIDs, err := vs.getNamespaceFolderAndRPMoIDs(ctx, resourcePolicy.Namespace)
+	folderMoID, rpMoIDs, err := topology.GetNamespaceFolderAndRPMoIDs(ctx, vs.k8sClient, resourcePolicy.Namespace)
 	if err != nil {
 		return err
 	}
@@ -80,24 +86,34 @@ func (vs *vSphereVMProvider) CreateOrUpdateVirtualMachineSetResourcePolicy(
 	vimClient := client.VimClient()
 	var errs []error
 
-	_, err = vcenter.CreateFolder(ctx, vimClient, folderMoID, resourcePolicy.Spec.Folder)
-	if err != nil {
-		errs = append(errs, err)
+	if folderName := resourcePolicy.Spec.Folder; folderName != "" {
+		if folderMoID != "" {
+			_, err = vcenter.CreateFolder(ctx, vimClient, folderMoID, folderName)
+			if err != nil {
+				errs = append(errs, err)
+			}
+		} else {
+			errs = append(errs, fmt.Errorf("cannot create child folder because Namespace folder not found"))
+		}
 	}
 
 	for _, rpMoID := range rpMoIDs {
-		_, err := vcenter.CreateOrUpdateChildResourcePool(ctx, vimClient, rpMoID, &resourcePolicy.Spec.ResourcePool)
-		if err != nil {
-			errs = append(errs, err)
+		if rpSpec := &resourcePolicy.Spec.ResourcePool; rpSpec.Name != "" {
+			_, err := vcenter.CreateOrUpdateChildResourcePool(ctx, vimClient, rpMoID, rpSpec)
+			if err != nil {
+				errs = append(errs, err)
+			}
 		}
 
-		clusterRef, err := vcenter.GetResourcePoolOwnerMoRef(ctx, vimClient, rpMoID)
-		if err == nil {
-			clusterModuleProvider := clustermodules.NewProvider(client.RestClient())
-			err = vs.createClusterModules(ctx, clusterModuleProvider, clusterRef.Reference(), resourcePolicy)
-		}
-		if err != nil {
-			errs = append(errs, err)
+		if len(resourcePolicy.Spec.ClusterModuleGroups) > 0 {
+			clusterRef, err := vcenter.GetResourcePoolOwnerMoRef(ctx, vimClient, rpMoID)
+			if err == nil {
+				clusterModuleProvider := clustermodules.NewProvider(client.RestClient())
+				err = vs.createClusterModules(ctx, clusterModuleProvider, clusterRef.Reference(), resourcePolicy)
+			}
+			if err != nil {
+				errs = append(errs, err)
+			}
 		}
 	}
 
@@ -109,7 +125,7 @@ func (vs *vSphereVMProvider) DeleteVirtualMachineSetResourcePolicy(
 	ctx context.Context,
 	resourcePolicy *vmopv1.VirtualMachineSetResourcePolicy) error {
 
-	folderMoID, rpMoIDs, err := vs.getNamespaceFolderAndRPMoIDs(ctx, resourcePolicy.Namespace)
+	folderMoID, rpMoIDs, err := topology.GetNamespaceFolderAndRPMoIDs(ctx, vs.k8sClient, resourcePolicy.Namespace)
 	if err != nil {
 		return err
 	}
@@ -132,8 +148,10 @@ func (vs *vSphereVMProvider) DeleteVirtualMachineSetResourcePolicy(
 	clusterModuleProvider := clustermodules.NewProvider(client.RestClient())
 	errs = append(errs, vs.deleteClusterModules(ctx, clusterModuleProvider, resourcePolicy)...)
 
-	if err := vcenter.DeleteChildFolder(ctx, vimClient, folderMoID, resourcePolicy.Spec.Folder); err != nil {
-		errs = append(errs, err)
+	if folderName := resourcePolicy.Spec.Folder; folderMoID != "" && folderName != "" {
+		if err := vcenter.DeleteChildFolder(ctx, vimClient, folderMoID, folderName); err != nil {
+			errs = append(errs, err)
+		}
 	}
 
 	return apierrorsutil.NewAggregate(errs)
@@ -246,19 +264,3 @@ func (vs *vSphereVMProvider) deleteClusterModules(
 	resourcePolicy.Status.ClusterModules = errModStatus
 	return errs
 }
-
-func (vs *vSphereVMProvider) getNamespaceFolderAndRPMoIDs(
-	ctx context.Context,
-	namespace string) (string, []string, error) {
-
-	folderMoID, rpMoIDs, err := topology.GetNamespaceFolderAndRPMoIDs(ctx, vs.k8sClient, namespace)
-	if err != nil {
-		return "", nil, err
-	}
-
-	if folderMoID == "" {
-		return "", nil, fmt.Errorf("namespace %s not present in any AvailabilityZones", namespace)
-	}
-
-	return folderMoID, rpMoIDs, nil
-}
diff --git a/pkg/providers/vsphere/vmprovider_resourcepolicy_test.go b/pkg/providers/vsphere/vmprovider_resourcepolicy_test.go
@@ -16,7 +16,7 @@ import (
 
 	vmopv1 "github.com/vmware-tanzu/vm-operator/api/v1alpha3"
 	"github.com/vmware-tanzu/vm-operator/pkg/providers"
-	vsphere "github.com/vmware-tanzu/vm-operator/pkg/providers/vsphere"
+	"github.com/vmware-tanzu/vm-operator/pkg/providers/vsphere"
 	"github.com/vmware-tanzu/vm-operator/test/builder"
 )
 
@@ -69,6 +69,31 @@ func resourcePolicyTests() {
 			initObjects = nil
 		})
 
+		Context("Empty VirtualMachineSetResourcePolicy", func() {
+			It("Creates and Deletes successfully", func() {
+				resourcePolicy := &vmopv1.VirtualMachineSetResourcePolicy{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "empty-policy",
+						Namespace: nsInfo.Namespace,
+					},
+				}
+
+				By("Create", func() {
+					Expect(vmProvider.CreateOrUpdateVirtualMachineSetResourcePolicy(ctx, resourcePolicy)).To(Succeed())
+				})
+				By("Implicitly IsReady", func() {
+					for _, zoneName := range ctx.ZoneNames {
+						exists, err := vmProvider.IsVirtualMachineSetResourcePolicyReady(ctx, zoneName, resourcePolicy)
+						Expect(err).NotTo(HaveOccurred())
+						Expect(exists).To(BeTrue())
+					}
+				})
+				By("Delete", func() {
+					Expect(vmProvider.DeleteVirtualMachineSetResourcePolicy(ctx, resourcePolicy)).To(Succeed())
+				})
+			})
+		})
+
 		Context("VirtualMachineSetResourcePolicy", func() {
 			var (
 				resourcePolicy *vmopv1.VirtualMachineSetResourcePolicy

diff --git a/pkg/topology/availability_zones.go b/pkg/topology/availability_zones.go
@@ -109,10 +109,14 @@ func GetNamespaceFolderAndRPMoIDs(
 		if err != nil && !errors.Is(err, ErrNoZones) {
 			return "", nil, err
 		}
+
 		for _, zone := range zones {
-			folderMoID = zone.Spec.ManagedVMs.FolderMoID
+			if folderMoID == "" {
+				folderMoID = zone.Spec.ManagedVMs.FolderMoID
+			}
 			rpMoIDs = append(rpMoIDs, zone.Spec.ManagedVMs.PoolMoIDs...)
 		}
+
 		return folderMoID, rpMoIDs, nil
 	}
 
@@ -123,7 +127,9 @@ func GetNamespaceFolderAndRPMoIDs(
 
 	for _, az := range availabilityZones {
 		if nsInfo, ok := az.Spec.Namespaces[namespace]; ok {
-			folderMoID = nsInfo.FolderMoId
+			if folderMoID == "" {
+				folderMoID = nsInfo.FolderMoId
+			}
 			if len(nsInfo.PoolMoIDs) != 0 {
 				rpMoIDs = append(rpMoIDs, nsInfo.PoolMoIDs...)
 			} else {

diff --git a/...s/virtualmachinesetresourcepolicy/validation/virtualmachinesetresourcepolicy_validator.go b/...s/virtualmachinesetresourcepolicy/validation/virtualmachinesetresourcepolicy_validator.go
@@ -107,9 +107,9 @@ func (v validator) validateSpec(ctx *pkgctx.WebhookRequestContext, vmRP *vmopv1.
 	var fieldErrs field.ErrorList
 	specPath := field.NewPath("spec")
 
-	fieldErrs = append(fieldErrs, v.validateResourcePool(ctx, specPath.Child("resourcepool"), vmRP.Spec.ResourcePool)...)
+	fieldErrs = append(fieldErrs, v.validateResourcePool(ctx, specPath.Child("resourcePool"), vmRP.Spec.ResourcePool)...)
 	fieldErrs = append(fieldErrs, v.validateFolder(ctx, specPath.Child("folder"), vmRP.Spec.Folder)...)
-	fieldErrs = append(fieldErrs, v.validateClusterModules(ctx, specPath.Child("clustermodules"), vmRP.Spec.ClusterModuleGroups)...)
+	fieldErrs = append(fieldErrs, v.validateClusterModules(ctx, specPath.Child("clusterModuleGroups"), vmRP.Spec.ClusterModuleGroups)...)
 
 	return fieldErrs
 }
@@ -152,9 +152,9 @@ func (v validator) validateAllowedChanges(ctx *pkgctx.WebhookRequestContext, vmR
 	specPath := field.NewPath("spec")
 
 	// Validate all fields under spec which are not allowed to change.
-	allErrs = append(allErrs, validation.ValidateImmutableField(vmRP.Spec.ResourcePool, oldVMRP.Spec.ResourcePool, specPath.Child("resourcepool"))...)
+	allErrs = append(allErrs, validation.ValidateImmutableField(vmRP.Spec.ResourcePool, oldVMRP.Spec.ResourcePool, specPath.Child("resourcePool"))...)
 	allErrs = append(allErrs, validation.ValidateImmutableField(vmRP.Spec.Folder, oldVMRP.Spec.Folder, specPath.Child("folder"))...)
-	allErrs = append(allErrs, validation.ValidateImmutableField(vmRP.Spec.ClusterModuleGroups, oldVMRP.Spec.ClusterModuleGroups, specPath.Child("clustermodules"))...)
+	allErrs = append(allErrs, validation.ValidateImmutableField(vmRP.Spec.ClusterModuleGroups, oldVMRP.Spec.ClusterModuleGroups, specPath.Child("clusterModuleGroups"))...)
 
 	return allErrs
 }

diff --git a/...achinesetresourcepolicy/validation/virtualmachinesetresourcepolicy_validator_intg_test.go b/...achinesetresourcepolicy/validation/virtualmachinesetresourcepolicy_validator_intg_test.go
@@ -4,15 +4,14 @@
 package validation_test
 
 import (
-	"k8s.io/apimachinery/pkg/api/resource"
-	"k8s.io/apimachinery/pkg/util/validation/field"
-
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 
+	"k8s.io/apimachinery/pkg/api/resource"
+	"k8s.io/apimachinery/pkg/util/validation/field"
+
 	vmopv1 "github.com/vmware-tanzu/vm-operator/api/v1alpha3"
 	"github.com/vmware-tanzu/vm-operator/pkg/constants/testlabels"
-
 	"github.com/vmware-tanzu/vm-operator/test/builder"
 )
 
@@ -99,7 +98,7 @@ func intgTestsValidateCreate() {
 		ctx = nil
 	})
 
-	fieldPath := field.NewPath("spec", "resourcepool", "reservations", "memory")
+	fieldPath := field.NewPath("spec", "resourcePool", "reservations", "memory")
 	DescribeTable("create table", validateCreate,
 		Entry("should work", true, "", nil),
 		Entry("should not work for invalid", false,

diff --git a/...achinesetresourcepolicy/validation/virtualmachinesetresourcepolicy_validator_unit_test.go b/...achinesetresourcepolicy/validation/virtualmachinesetresourcepolicy_validator_unit_test.go
@@ -128,7 +128,7 @@ func unitTestsValidateCreate() {
 		ctx = nil
 	})
 
-	reservationsPath := field.NewPath("spec", "resourcepool", "reservations")
+	reservationsPath := field.NewPath("spec", "resourcePool", "reservations")
 	detailMsg := "reservation value cannot exceed the limit value"
 	DescribeTable("create table", validateCreate,
 		Entry("should allow valid", createArgs{}, true, nil, nil),