Skip to content

Commit

Permalink
Improve clustermodule existence check
Browse files Browse the repository at this point in the history
  • Loading branch information
rikatz committed Dec 13, 2023
1 parent c97bf21 commit 4016141
Show file tree
Hide file tree
Showing 7 changed files with 23 additions and 63 deletions.
2 changes: 1 addition & 1 deletion go.mod
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
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
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
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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

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
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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) {

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

This file was deleted.

9 changes: 2 additions & 7 deletions test/e2e/anti_affinity_test.go
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 @@ -224,11 +222,8 @@ func FetchWorkerVMsForCluster(ctx context.Context, bootstrapClusterProxy framewo
func verifyModuleInfo(ctx context.Context, finder *find.Finder, modules []infrav1.ClusterModule, toExist bool) {

Check warning on line 222 in test/e2e/anti_affinity_test.go

View workflow job for this annotation

GitHub Actions / lint

unused-parameter: parameter 'finder' seems to be unused, consider removing or renaming it as _ (revive)
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

0 comments on commit 4016141

Please sign in to comment.