Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🐛 Improve clustermodule existence check #2535

Merged
merged 1 commit into from
Dec 13, 2023
+26 −66
Merged

🐛 Improve clustermodule existence check #2535

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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=
Expand Down
9 changes: 1 addition & 8 deletions pkg/clustermodule/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
31 changes: 14 additions & 17 deletions pkg/services/govmomi/clustermodules/provider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,21 +19,20 @@

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
Expand Down Expand Up @@ -71,37 +70,35 @@
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

Check warning on line 98 in pkg/services/govmomi/clustermodules/provider.go

View check run for this annotation

Codecov / codecov/patch

pkg/services/govmomi/clustermodules/provider.go#L96-L98 

Added lines #L96 - L98 were not covered by tests
}

log.V(4).Info("Cluster module doesn't exist", "computeClusterRef", clusterRef)
return false, nil
return false, err

Check warning on line 101 in pkg/services/govmomi/clustermodules/provider.go

View check run for this annotation

Codecov / codecov/patch

pkg/services/govmomi/clustermodules/provider.go#L101 

Added line #L101 was not covered by tests
}

// IsMoRefModuleMember checks whether the passed managed object reference is in the ClusterModule.
Expand Down
4 changes: 3 additions & 1 deletion pkg/services/govmomi/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,15 @@
"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"
Expand Down Expand Up @@ -268,7 +270,7 @@

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) {

Check warning on line 273 in pkg/services/govmomi/service.go

View check run for this annotation

Codecov / codecov/patch

pkg/services/govmomi/service.go#L273 

Added line #L273 was not covered by tests
return reconcile.Result{}, vm, err
}
vmCtx.VSphereVM.Status.ModuleUUID = nil
Expand Down
27 changes: 0 additions & 27 deletions pkg/util/errors.go
View file
Open in desktop

This file was deleted.

15 changes: 5 additions & 10 deletions test/e2e/anti_affinity_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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))
}
Expand Down
Loading