From 4016141f8a547b1bfb21f9018cf5c1936a21fac6 Mon Sep 17 00:00:00 2001 From: Ricardo Katz Date: Fri, 8 Dec 2023 17:30:37 -0300 Subject: [PATCH] Improve clustermodule existence check --- go.mod | 2 +- go.sum | 4 +-- pkg/clustermodule/service.go | 9 +----- .../govmomi/clustermodules/provider.go | 31 +++++++++---------- pkg/services/govmomi/service.go | 4 ++- pkg/util/errors.go | 27 ---------------- test/e2e/anti_affinity_test.go | 9 ++---- 7 files changed, 23 insertions(+), 63 deletions(-) delete mode 100644 pkg/util/errors.go diff --git a/go.mod b/go.mod index 7894f32fb6..69716c5a22 100644 --- a/go.mod +++ b/go.mod @@ -17,7 +17,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 diff --git a/go.sum b/go.sum index 7e7fd956d2..11dcc9fd9b 100644 --- a/go.sum +++ b/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= diff --git a/pkg/clustermodule/service.go b/pkg/clustermodule/service.go index 1fedda534d..6a7d62fa12 100644 --- a/pkg/clustermodule/service.go +++ b/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 { diff --git a/pkg/services/govmomi/clustermodules/provider.go b/pkg/services/govmomi/clustermodules/provider.go index e3659fc952..8b3cf10064 100644 --- a/pkg/services/govmomi/clustermodules/provider.go +++ b/pkg/services/govmomi/clustermodules/provider.go @@ -19,13 +19,12 @@ 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 @@ -33,7 +32,7 @@ import ( 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,7 +70,7 @@ 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 } @@ -79,29 +78,27 @@ func (cm *provider) DeleteModule(ctx context.Context, moduleUUID string) error { 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. diff --git a/pkg/services/govmomi/service.go b/pkg/services/govmomi/service.go index d98e7de0ab..cafe48e869 100644 --- a/pkg/services/govmomi/service.go +++ b/pkg/services/govmomi/service.go @@ -20,6 +20,7 @@ import ( "context" "encoding/base64" "fmt" + "net/http" "time" "github.com/pkg/errors" @@ -27,6 +28,7 @@ import ( "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 diff --git a/pkg/util/errors.go b/pkg/util/errors.go deleted file mode 100644 index b4adf20641..0000000000 --- a/pkg/util/errors.go +++ /dev/null @@ -1,27 +0,0 @@ -/* -Copyright 2022 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 util - -import ( - "net/http" - "strings" -) - -// IsNotFoundError identifies a not found error from a HTTP 404 status code. -func IsNotFoundError(err error) bool { - return strings.HasSuffix(err.Error(), http.StatusText(http.StatusNotFound)) -} diff --git a/test/e2e/anti_affinity_test.go b/test/e2e/anti_affinity_test.go index 97abea0c51..60aba24efe 100644 --- a/test/e2e/anti_affinity_test.go +++ b/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") @@ -224,11 +222,8 @@ func FetchWorkerVMsForCluster(ctx context.Context, bootstrapClusterProxy framewo func verifyModuleInfo(ctx context.Context, finder *find.Finder, 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)) }