Merge pull request #2535 from rikatz/check-clustermodules-queries

🐛 Improve clustermodule existence check

go.mod

@@ -19,7 +19,7 @@ require (
 	github.com/vmware-tanzu/vm-operator/api v1.8.3-0.20231114230806-852c1641447a
 	github.com/vmware-tanzu/vm-operator/external/ncp v0.0.0-20211209213435-0f4ab286f64f
 	github.com/vmware-tanzu/vm-operator/external/tanzu-topology v0.0.0-20211209213435-0f4ab286f64f
-	github.com/vmware/govmomi v0.33.1
+	github.com/vmware/govmomi v0.34.0
 	golang.org/x/crypto v0.16.0
 	golang.org/x/exp v0.0.0-20230905200255-921286631fa9
 	golang.org/x/mod v0.14.0

go.sum

@@ -663,8 +663,8 @@ github.com/vmware-tanzu/vm-operator/external/ncp v0.0.0-20211209213435-0f4ab286f
 github.com/vmware-tanzu/vm-operator/external/ncp v0.0.0-20211209213435-0f4ab286f64f/go.mod h1:5rqRJ9zGR+KnKbkGx373WgN8xJpvAj99kHnfoDYRO5I=
 github.com/vmware-tanzu/vm-operator/external/tanzu-topology v0.0.0-20211209213435-0f4ab286f64f h1:wwYUf16/g8bLywQMQJB5VHbDtuf6aOFH24Ar2/yA7+I=
 github.com/vmware-tanzu/vm-operator/external/tanzu-topology v0.0.0-20211209213435-0f4ab286f64f/go.mod h1:dfYrWS8DMRN+XZfhu8M4LVHmeGvYB29Ipd7j4uIq+mU=
-github.com/vmware/govmomi v0.33.1 h1:qS2VpEBd/WLbzLO5McI6h5o5zaKsrezUxRY5r9jkW8A=
-github.com/vmware/govmomi v0.33.1/go.mod h1:QuzWGiEMA/FYlu5JXKjytiORQoxv2hTHdS2lWnIqKMM=
+github.com/vmware/govmomi v0.34.0 h1:Aun71BDf1t8r3jNeUWJ3ZM+7kHbIuuNzIuxRVo5LYYU=
+github.com/vmware/govmomi v0.34.0/go.mod h1:qWWT6n9mdCr/T9vySsoUqcI04sSEj4CqHXxtk/Y+Los=
 github.com/xiang90/probing v0.0.0-20190116061207-43a291ad63a2 h1:eY9dn8+vbi4tKz5Qo6v2eYzo7kUS51QINcR5jNpbZS8=
 github.com/xiang90/probing v0.0.0-20190116061207-43a291ad63a2/go.mod h1:UETIi67q53MR2AWcXfiuqkDkRtnGDLqkBTpCHuJHxtU=
 github.com/xlab/treeprint v1.2.0 h1:HzHnuAF1plUN2zGlAFHbSQP2qJ0ZAD3XF5XD7OesXRQ=

pkg/clustermodule/service.go

@@ -103,15 +103,8 @@ func (s *service) DoesExist(ctx context.Context, clusterCtx *capvcontext.Cluster
 		return false, errors.Wrapf(err, "error fetching session for object %s/%s", wrapper.GetNamespace(), wrapper.GetName())
 	}
 
-	// Fetch the compute cluster resource by tracing the owner of the resource pool in use.
-	// TODO (srm09): How do we support Multi AZ scenarios here
-	computeClusterRef, err := getComputeClusterResource(ctx, vCenterSession, template.Spec.Template.Spec.ResourcePool)
-	if err != nil {
-		return false, errors.Wrapf(err, "error fetching compute cluster resource")
-	}
-
 	provider := clustermodules.NewProvider(vCenterSession.TagManager.Client)
-	return provider.DoesModuleExist(ctx, moduleUUID, computeClusterRef)
+	return provider.DoesModuleExist(ctx, moduleUUID)
 }
 
 func (s *service) Remove(ctx context.Context, clusterCtx *capvcontext.ClusterContext, moduleUUID string) error {

pkg/services/govmomi/clustermodules/provider.go

@@ -19,21 +19,20 @@ package clustermodules
 
 import (
 	"context"
+	"net/http"
 
 	"github.com/vmware/govmomi/vapi/cluster"
 	"github.com/vmware/govmomi/vapi/rest"
 	"github.com/vmware/govmomi/vim25/types"
 	ctrl "sigs.k8s.io/controller-runtime"
-
-	"sigs.k8s.io/cluster-api-provider-vsphere/pkg/util"
 )
 
 // Provider exposes methods to interact with the cluster module vCenter API
 // TODO (srm09): Rethink and merge with ClusterModuleService.
 type Provider interface {
 	CreateModule(ctx context.Context, clusterRef types.ManagedObjectReference) (string, error)
 	DeleteModule(ctx context.Context, moduleID string) error
-	DoesModuleExist(ctx context.Context, moduleID string, cluster types.ManagedObjectReference) (bool, error)
+	DoesModuleExist(ctx context.Context, moduleID string) (bool, error)
 
 	IsMoRefModuleMember(ctx context.Context, moduleID string, moRef types.ManagedObjectReference) (bool, error)
 	AddMoRefToModule(ctx context.Context, moduleID string, moRef types.ManagedObjectReference) error
@@ -71,37 +70,35 @@ func (cm *provider) DeleteModule(ctx context.Context, moduleUUID string) error {
 	log.Info("Deleting cluster module")
 
 	err := cm.manager.DeleteModule(ctx, moduleUUID)
-	if err != nil && !util.IsNotFoundError(err) {
+	if err != nil && !rest.IsStatusError(err, http.StatusNotFound) {
 		return err
 	}
 
 	log.Info("Deleted cluster module")
 	return nil
 }
 
-// DoesModuleExist checks whether a module with a given name exists with the passed clusterRef and moduleUUID.
-func (cm *provider) DoesModuleExist(ctx context.Context, moduleUUID string, clusterRef types.ManagedObjectReference) (bool, error) {
+// DoesModuleExist checks whether a module with a given moduleUUID exists.
+func (cm *provider) DoesModuleExist(ctx context.Context, moduleUUID string) (bool, error) {
 	log := ctrl.LoggerFrom(ctx)
-	log.V(4).Info("Checking if cluster module exists", "computeClusterRef", clusterRef)
+	log.V(4).Info("Checking if cluster module exists")
 
 	if moduleUUID == "" {
 		return false, nil
 	}
 
-	modules, err := cm.manager.ListModules(ctx)
-	if err != nil {
-		return false, err
+	_, err := cm.manager.ListModuleMembers(ctx, moduleUUID)
+	if err == nil {
+		log.V(4).Info("Cluster module exists")
+		return true, nil
 	}
 
-	for _, mod := range modules {
-		if mod.Cluster == clusterRef.Value && mod.Module == moduleUUID {
-			log.V(4).Info("Cluster module does exist", "computeClusterRef", clusterRef)
-			return true, nil
-		}
+	if rest.IsStatusError(err, http.StatusNotFound) {
+		log.V(4).Info("Cluster module doesn't exist")
+		return false, nil
 	}
 
-	log.V(4).Info("Cluster module doesn't exist", "computeClusterRef", clusterRef)
-	return false, nil
+	return false, err
 }
 
 // IsMoRefModuleMember checks whether the passed managed object reference is in the ClusterModule.

pkg/services/govmomi/service.go

@@ -20,13 +20,15 @@ import (
 	"context"
 	"encoding/base64"
 	"fmt"
+	"net/http"
 	"time"
 
 	"github.com/pkg/errors"
 	"github.com/vmware/govmomi/object"
 	"github.com/vmware/govmomi/pbm"
 	pbmTypes "github.com/vmware/govmomi/pbm/types"
 	"github.com/vmware/govmomi/property"
+	"github.com/vmware/govmomi/vapi/rest"
 	"github.com/vmware/govmomi/vim25/mo"
 	"github.com/vmware/govmomi/vim25/types"
 	corev1 "k8s.io/api/core/v1"
@@ -268,7 +270,7 @@ func (vms *VMService) DestroyVM(ctx context.Context, vmCtx *capvcontext.VMContex
 
 		provider := clustermodules.NewProvider(vmCtx.Session.TagManager.Client)
 		err := provider.RemoveMoRefFromModule(ctx, *vmCtx.ClusterModuleInfo, virtualMachineCtx.Ref)
-		if err != nil && !util.IsNotFoundError(err) {
+		if err != nil && !rest.IsStatusError(err, http.StatusNotFound) {
 			return reconcile.Result{}, vm, err
 		}
 		vmCtx.VSphereVM.Status.ModuleUUID = nil

test/e2e/anti_affinity_test.go

@@ -44,9 +44,7 @@ type AntiAffinitySpecInput struct {
 }
 
 var _ = Describe("Cluster creation with anti affined nodes", func() {
-	var (
-		namespace *corev1.Namespace
-	)
+	var namespace *corev1.Namespace
 
 	BeforeEach(func() {
 		Expect(bootstrapClusterProxy).NotTo(BeNil(), "BootstrapClusterProxy can't be nil")
@@ -123,7 +121,7 @@ func VerifyAntiAffinity(ctx context.Context, input AntiAffinitySpecInput) {
 	}
 
 	By("verifying presence of cluster modules")
-	verifyModuleInfo(ctx, input.Finder, modules, true)
+	verifyModuleInfo(ctx, modules, true)
 
 	By("verifying node anti-affinity for worker nodes")
 	workerVMs := FetchWorkerVMsForCluster(ctx, input.Global.BootstrapClusterProxy, clusterName, namespace.Name)
@@ -165,7 +163,7 @@ func VerifyAntiAffinity(ctx context.Context, input AntiAffinitySpecInput) {
 	}, input.Global.E2EConfig.GetIntervals("", "wait-delete-cluster")...)
 
 	By("confirming deletion of cluster module constructs")
-	verifyModuleInfo(ctx, input.Finder, modules, false)
+	verifyModuleInfo(ctx, modules, false)
 }
 
 func verifyAntiAffinityForVMs(ctx context.Context, finder *find.Finder, vms []infrav1.VSphereVM) error {
@@ -221,14 +219,11 @@ func FetchWorkerVMsForCluster(ctx context.Context, bootstrapClusterProxy framewo
 	return workerVMs
 }
 
-func verifyModuleInfo(ctx context.Context, finder *find.Finder, modules []infrav1.ClusterModule, toExist bool) {
+func verifyModuleInfo(ctx context.Context, modules []infrav1.ClusterModule, toExist bool) {
 	provider := clustermodules.NewProvider(restClient)
 
-	cc, err := finder.ClusterComputeResourceOrDefault(ctx, "")
-	Expect(err).ToNot(HaveOccurred())
-
 	for _, mod := range modules {
-		exists, err := provider.DoesModuleExist(ctx, mod.ModuleUUID, cc.Reference())
+		exists, err := provider.DoesModuleExist(ctx, mod.ModuleUUID)
 		Expect(err).ToNot(HaveOccurred())
 		Expect(exists).To(Equal(toExist))
 	}