Skip to content

Commit

Permalink
Misc VirtualMachineSetResourcePolicy improvements
Browse files Browse the repository at this point in the history 
The child Folder and ResourcePool names are optional fields but we
were not checking for this.

When getting both the Resource Pool and Folder MoIDs of a namespace,
don't always return the last FolderMoID on the Zone or AZ. The Folder
is VC scoped and we should not ever have a Zone or AZ without it,
return the value if at least one of the Zone/AZ has it set.

Use the v1a2+ field names in the validation webhook error messages
  • Loading branch information
Bryan Venteicher committed Dec 17, 2024
1 parent 62935ba commit 6ec16d4
Show file tree
Hide file tree
Showing 6 changed files with 84 additions and 52 deletions.
80 changes: 41 additions & 39 deletions pkg/providers/vsphere/vmprovider_resourcepolicy.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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
}
Expand All @@ -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)
}
}
}

Expand All @@ -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
}
Expand All @@ -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)
Expand Down Expand Up @@ -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
}
27 changes: 26 additions & 1 deletion pkg/providers/vsphere/vmprovider_resourcepolicy_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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
Expand Down
10 changes: 8 additions & 2 deletions pkg/topology/availability_zones.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand All @@ -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 {
Expand Down
8 changes: 4 additions & 4 deletions ...s/virtualmachinesetresourcepolicy/validation/virtualmachinesetresourcepolicy_validator.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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
}
Expand Down
9 changes: 4 additions & 5 deletions ...achinesetresourcepolicy/validation/virtualmachinesetresourcepolicy_validator_intg_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion ...achinesetresourcepolicy/validation/virtualmachinesetresourcepolicy_validator_unit_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down

0 comments on commit 6ec16d4

Please sign in to comment.