Skip to content

Commit

Permalink
Misc VirtualMachineSetResourcePolicy clean ups
Browse files Browse the repository at this point in the history 
The child Folder and ResourcePool name can be empty but we weren't
checking for this.

When getting both the Resource Pool and Folder MoIDs of a namespace,
don't nessciarly 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 11, 2024
1 parent 86a2d34 commit 8094a0c
Show file tree
Hide file tree
Showing 6 changed files with 77 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
}
20 changes: 19 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,24 @@ 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("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 8094a0c

Please sign in to comment.