From 40b2c61fb28585dfdb9c5348a7e3b69f88f3b07f Mon Sep 17 00:00:00 2001 From: Ricardo Katz Date: Thu, 26 Oct 2023 09:08:35 -0300 Subject: [PATCH] Use a less expensive method for storage policy --- go.mod | 2 +- go.sum | 4 +- pkg/services/govmomi/service.go | 42 ++++-- pkg/services/govmomi/service_test.go | 137 +++++++++++++++++++- pkg/services/govmomi/storageprofile_util.go | 32 +++++ 5 files changed, 199 insertions(+), 18 deletions(-) create mode 100644 pkg/services/govmomi/storageprofile_util.go diff --git a/go.mod b/go.mod index 1ff3633a70..de68a6eea9 100644 --- a/go.mod +++ b/go.mod @@ -17,7 +17,7 @@ require ( github.com/vmware-tanzu/vm-operator/api v1.8.2 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.32.0 + github.com/vmware/govmomi v0.33.1 golang.org/x/crypto v0.14.0 golang.org/x/exp v0.0.0-20221002003631-540bb7301a08 golang.org/x/mod v0.13.0 diff --git a/go.sum b/go.sum index 17dada223e..886f89d004 100644 --- a/go.sum +++ b/go.sum @@ -619,8 +619,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.32.0 h1:Rsdi/HAX5Ebf9Byp/FvBir4sfM7yP5DBUeRlbC6vLBo= -github.com/vmware/govmomi v0.32.0/go.mod h1:JA63Pg0SgQcSjk+LuPzjh3rJdcWBo/ZNCIwbb1qf2/0= +github.com/vmware/govmomi v0.33.1 h1:qS2VpEBd/WLbzLO5McI6h5o5zaKsrezUxRY5r9jkW8A= +github.com/vmware/govmomi v0.33.1/go.mod h1:QuzWGiEMA/FYlu5JXKjytiORQoxv2hTHdS2lWnIqKMM= github.com/xeipuuv/gojsonpointer v0.0.0-20180127040702-4e3ac2762d5f/go.mod h1:N2zxlSyiKSe5eX1tZViRH5QA0qijqEDrYZiPEAiq3wU= github.com/xeipuuv/gojsonreference v0.0.0-20180127040603-bd5ef7bd5415/go.mod h1:GwrjFmJcFw6At/Gs6z4yjiIwzuJ1/+UwLxMQDVQXShQ= github.com/xeipuuv/gojsonschema v1.2.0/go.mod h1:anYRn/JVcOK2ZgGU+IjEV4nwlhoK5sQluxsYJ78Id3Y= diff --git a/pkg/services/govmomi/service.go b/pkg/services/govmomi/service.go index a1df27fc09..512d4c5ab2 100644 --- a/pkg/services/govmomi/service.go +++ b/pkg/services/govmomi/service.go @@ -394,10 +394,6 @@ func (vms *VMService) reconcileStoragePolicy(ctx context.Context, virtualMachine if err != nil { return errors.Wrap(err, "unable to retrieve storage profile ID") } - entities, err := pbmClient.QueryAssociatedEntity(ctx, pbmTypes.PbmProfileId{UniqueId: storageProfileID}, "virtualDiskId") - if err != nil { - return err - } var changes []types.BaseVirtualDeviceConfigSpec devices, err := virtualMachineCtx.Obj.Device(ctx) @@ -405,24 +401,41 @@ func (vms *VMService) reconcileStoragePolicy(ctx context.Context, virtualMachine return err } + disksRefs := make([]pbmTypes.PbmServerObjectRef, 0) + // diskMap is just an auxiliar map so we don't need to iterate over and over disks to get their configs + // if we realize they are not on the right storage policy + diskMap := make(map[string]*types.VirtualDisk) + disks := devices.SelectByType((*types.VirtualDisk)(nil)) + + // We iterate over disks and create an array of disks refs, so we just need to make a single call + // against vCenter, instead of one call per disk + // the diskMap is an auxiliar way of, besides the disksRefs, we have a "searchable" disk configuration + // in case we need to reconfigure a disk, to get its config for _, d := range disks { disk := d.(*types.VirtualDisk) - found := false // entities associated with storage policy has key in the form : diskID := fmt.Sprintf("%s:%d", virtualMachineCtx.Obj.Reference().Value, disk.Key) - for _, e := range entities { - if e.Key == diskID { - found = true - break - } - } + diskMap[diskID] = disk + + disksRefs = append(disksRefs, pbmTypes.PbmServerObjectRef{ + ObjectType: string(pbmTypes.PbmObjectTypeVirtualDiskId), + Key: diskID, + }) + } + + diskObjects, err := pbmClient.QueryAssociatedProfiles(ctx, disksRefs) + if err != nil { + return errors.Wrap(err, "unable to query disks associated profiles") + } - if !found { - // disk wasn't associated with storage policy, create a device change to make the association + // Ensure storage policy is set correctly for all disks of the VM + for k := range diskObjects { + if !isStoragePolicyIDPresent(storageProfileID, diskObjects[k]) { + virtualMachineCtx.Logger.V(5).Info("storage policy not found on disk, adding for reconciliation", "disk", diskObjects[k].Object.Key) config := &types.VirtualDeviceConfigSpec{ Operation: types.VirtualDeviceConfigSpecOperationEdit, - Device: disk, + Device: diskMap[diskObjects[k].Object.Key], Profile: []types.BaseVirtualMachineProfileSpec{ &types.VirtualMachineDefinedProfileSpec{ProfileId: storageProfileID}, }, @@ -431,6 +444,7 @@ func (vms *VMService) reconcileStoragePolicy(ctx context.Context, virtualMachine } } + // If there are pending changes for Storage Policies, do it before moving next if len(changes) > 0 { task, err := virtualMachineCtx.Obj.Reconfigure(ctx, types.VirtualMachineConfigSpec{ VmProfile: []types.BaseVirtualMachineProfileSpec{ diff --git a/pkg/services/govmomi/service_test.go b/pkg/services/govmomi/service_test.go index 011e6c3427..a0867e3823 100644 --- a/pkg/services/govmomi/service_test.go +++ b/pkg/services/govmomi/service_test.go @@ -18,11 +18,14 @@ package govmomi import ( "context" + "fmt" "testing" "github.com/go-logr/logr" . "github.com/onsi/gomega" "github.com/vmware/govmomi/find" + "github.com/vmware/govmomi/object" + pbmsimulator "github.com/vmware/govmomi/pbm/simulator" "github.com/vmware/govmomi/simulator" "github.com/vmware/govmomi/vim25" "github.com/vmware/govmomi/vim25/types" @@ -32,6 +35,11 @@ import ( infrav1 "sigs.k8s.io/cluster-api-provider-vsphere/apis/v1beta1" capvcontext "sigs.k8s.io/cluster-api-provider-vsphere/pkg/context" + "sigs.k8s.io/cluster-api-provider-vsphere/pkg/session" +) + +const ( + defaultStoragePolicy = "vSAN Default Storage Policy" ) func emptyVirtualMachineContext() *virtualMachineContext { @@ -93,9 +101,136 @@ func Test_reconcilePCIDevices(t *testing.T) { pciDevices := devices.SelectByBackingInfo(&types.VirtualPCIPassthroughDynamicBackingInfo{ AllowedDevice: []types.VirtualPCIPassthroughAllowedDevice{ {DeviceId: 1234, VendorId: 5678}, - }}) + }, + }) g.Expect(pciDevices).To(HaveLen(2)) return nil }) }) } + +func Test_ReconcileStoragePolicy(t *testing.T) { + var vmCtx *virtualMachineContext + var g *WithT + var vms *VMService + + before := func() { + vmCtx = emptyVirtualMachineContext() + vmCtx.Client = fake.NewClientBuilder().Build() + + vms = &VMService{} + } + t.Run("when VM has no storage policy spec", func(t *testing.T) { + g = NewWithT(t) + before() + vmCtx.VSphereVM = &infrav1.VSphereVM{ + ObjectMeta: metav1.ObjectMeta{ + Name: "vsphereVM1", + Namespace: "my-namespace", + }, + Spec: infrav1.VSphereVMSpec{ + VirtualMachineCloneSpec: infrav1.VirtualMachineCloneSpec{}, + }, + } + g.Expect(vms.reconcileStoragePolicy(context.Background(), vmCtx)).ToNot(HaveOccurred()) + g.Expect(vmCtx.VSphereVM.Status.TaskRef).To(BeEmpty()) + }) + + t.Run("when the requested storage policy does not exists should fail", func(t *testing.T) { + g = NewWithT(t) + before() + model, err := storagePolicyModel() + g.Expect(err).ToNot(HaveOccurred()) + + simulator.Run(func(ctx context.Context, c *vim25.Client) error { + authSession, err := getAuthSession(ctx, model.Service.Listen.Host) + g.Expect(err).ToNot(HaveOccurred()) + vmCtx.Session = authSession + vm, err := getPoweredoffVM(ctx, c) + g.Expect(err).ToNot(HaveOccurred()) + + vmCtx.Obj = vm + vmCtx.VSphereVM = &infrav1.VSphereVM{ + ObjectMeta: metav1.ObjectMeta{ + Name: "vsphereVM1", + Namespace: "my-namespace", + }, + Spec: infrav1.VSphereVMSpec{ + VirtualMachineCloneSpec: infrav1.VirtualMachineCloneSpec{ + StoragePolicyName: "non-existing-storagepolicy", + }, + }, + } + err = vms.reconcileStoragePolicy(context.Background(), vmCtx) + g.Expect(err.Error()).To(ContainSubstring("no pbm profile found with name")) + return nil + }, model) + }) + + t.Run("when the requested storage policy exists should pass", func(t *testing.T) { + // This Method should be implemented on Govmomi vcsim and then we can unskip this test + t.Skip("PbmQueryAssociatedProfiles is not yet implemented on PBM simulator") + g = NewWithT(t) + before() + model, err := storagePolicyModel() + g.Expect(err).ToNot(HaveOccurred()) + + simulator.Run(func(ctx context.Context, c *vim25.Client) error { + authSession, err := getAuthSession(ctx, model.Service.Listen.Host) + g.Expect(err).ToNot(HaveOccurred()) + vmCtx.Session = authSession + vm, err := getPoweredoffVM(ctx, c) + g.Expect(err).ToNot(HaveOccurred()) + + vmCtx.Obj = vm + vmCtx.VSphereVM = &infrav1.VSphereVM{ + ObjectMeta: metav1.ObjectMeta{ + Name: "vsphereVM1", + Namespace: "my-namespace", + }, + Spec: infrav1.VSphereVMSpec{ + VirtualMachineCloneSpec: infrav1.VirtualMachineCloneSpec{ + StoragePolicyName: defaultStoragePolicy, + }, + }, + } + err = vms.reconcileStoragePolicy(context.Background(), vmCtx) + g.Expect(err).ToNot(HaveOccurred()) + return nil + }, model) + }) +} + +func getAuthSession(ctx context.Context, server string) (*session.Session, error) { + password, _ := simulator.DefaultLogin.Password() + return session.GetOrCreate( + ctx, + session.NewParams(). + WithUserInfo(simulator.DefaultLogin.Username(), password). + WithServer(fmt.Sprintf("http://%s", server)). + WithDatacenter("*")) +} + +func getPoweredoffVM(ctx context.Context, c *vim25.Client) (*object.VirtualMachine, error) { + finder := find.NewFinder(c) + vm, err := finder.VirtualMachine(ctx, "DC0_H0_VM0") + if err != nil { + return nil, err + } + + _, err = vm.PowerOff(ctx) + return vm, err +} + +func storagePolicyModel() (*simulator.Model, error) { + model := simulator.VPX() + err := model.Create() + if err != nil { + return nil, err + } + model.Service.RegisterSDK(pbmsimulator.New()) + model.Machine = 1 + model.Datacenter = 1 + model.Host = 1 + return model, nil +} diff --git a/pkg/services/govmomi/storageprofile_util.go b/pkg/services/govmomi/storageprofile_util.go new file mode 100644 index 0000000000..3402d8881b --- /dev/null +++ b/pkg/services/govmomi/storageprofile_util.go @@ -0,0 +1,32 @@ +/* +Copyright 2023 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 govmomi + +import ( + pbmTypes "github.com/vmware/govmomi/pbm/types" +) + +// isStoragePolicyIDPresent checks, given a ProfileResult if the requested storageprofileid +// is associated at least with one entity. +func isStoragePolicyIDPresent(storageProfileID string, result pbmTypes.PbmQueryProfileResult) bool { + for _, id := range result.ProfileId { + if id.UniqueId == storageProfileID { + return true + } + } + return false +}