diff --git a/go.mod b/go.mod index 26e9410b6f..aab9e4edcf 100644 --- a/go.mod +++ b/go.mod @@ -6,18 +6,18 @@ require ( github.com/antihax/optional v1.0.0 github.com/go-logr/logr v1.2.4 github.com/google/gofuzz v1.2.0 - github.com/google/uuid v1.3.0 + github.com/google/uuid v1.3.1 github.com/hashicorp/go-version v1.3.0 github.com/onsi/ginkgo/v2 v2.9.2 github.com/onsi/gomega v1.27.6 github.com/pkg/errors v0.9.1 github.com/spf13/cobra v1.7.0 - github.com/stretchr/testify v1.8.2 + github.com/stretchr/testify v1.8.4 github.com/vmware-tanzu/net-operator-api v0.0.0-20210401185409-b0dc6c297707 github.com/vmware-tanzu/vm-operator/api v0.0.0-20221204041501-ec03d8bb733c 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.30.7 + 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.10.0 @@ -94,7 +94,7 @@ require ( github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da // indirect github.com/golang/protobuf v1.5.3 // indirect github.com/google/cel-go v0.12.6 // indirect - github.com/google/go-cmp v0.5.9 // indirect + github.com/google/go-cmp v0.6.0 // indirect github.com/google/go-querystring v1.1.0 // indirect github.com/hashicorp/hcl v1.0.0 // indirect github.com/huandu/xstrings v1.3.3 // indirect diff --git a/go.sum b/go.sum index 4ad561b61c..707f5a34d2 100644 --- a/go.sum +++ b/go.sum @@ -291,8 +291,8 @@ github.com/google/go-cmp v0.5.1/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/ github.com/google/go-cmp v0.5.2/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE= github.com/google/go-cmp v0.5.4/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE= github.com/google/go-cmp v0.5.5/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE= -github.com/google/go-cmp v0.5.9 h1:O2Tfq5qg4qc4AmwVlvv0oLiVAGB7enBSJ2x2DqQFi38= -github.com/google/go-cmp v0.5.9/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY= +github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI= +github.com/google/go-cmp v0.6.0/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY= github.com/google/go-github/v48 v48.2.0 h1:68puzySE6WqUY9KWmpOsDEQfDZsso98rT6pZcz9HqcE= github.com/google/go-github/v48 v48.2.0/go.mod h1:dDlehKBDo850ZPvCTK0sEqTCVWcrGl2LcDiajkYi89Y= github.com/google/go-querystring v1.1.0 h1:AnCroh3fv4ZBgVIf1Iwtovgjaw/GiKJo8M8yD/fhyJ8= @@ -323,8 +323,8 @@ github.com/google/safetext v0.0.0-20220905092116-b49f7bc46da2/go.mod h1:Tv1PlzqC github.com/google/uuid v1.0.0/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo= github.com/google/uuid v1.1.1/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo= github.com/google/uuid v1.1.2/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo= -github.com/google/uuid v1.3.0 h1:t6JiXgmwXMjEs8VusXIJk2BXHsn+wx8BZdTaoZ5fu7I= -github.com/google/uuid v1.3.0/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo= +github.com/google/uuid v1.3.1 h1:KjJaJ9iWZ3jOFZIf1Lqf4laDRCasjl0BCmnEGxkdLb4= +github.com/google/uuid v1.3.1/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo= github.com/googleapis/gax-go/v2 v2.0.4/go.mod h1:0Wqv26UfaUD9n4G6kQubkQ+KchISgw+vpHVxEJEs9eg= github.com/googleapis/gax-go/v2 v2.0.5/go.mod h1:DWXyrwAJ9X0FpwwEdw+IPEYBICEFu5mhpdKc/us6bOk= github.com/googleapis/gnostic v0.0.0-20170729233727-0c5108395e2d/go.mod h1:sJBsCZ4ayReDTBIg8b9dl28c5xFWyhBTVRp3pOg5EKY= @@ -607,8 +607,8 @@ github.com/stretchr/testify v1.7.0/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/ github.com/stretchr/testify v1.7.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= github.com/stretchr/testify v1.8.0/go.mod h1:yNjHg4UonilssWZ8iaSj1OCr/vHnekPRkoO+kdMU+MU= github.com/stretchr/testify v1.8.1/go.mod h1:w2LPCIKwWwSfY2zedu0+kehJoqGctiVI29o6fzry7u4= -github.com/stretchr/testify v1.8.2 h1:+h33VjcLVPDHtOdpUCuF+7gSuG3yGIftsP1YvFihtJ8= -github.com/stretchr/testify v1.8.2/go.mod h1:w2LPCIKwWwSfY2zedu0+kehJoqGctiVI29o6fzry7u4= +github.com/stretchr/testify v1.8.4 h1:CcVxjf3Q8PM0mHUKJCdn+eZZtm5yQwehR5yeSVQQcUk= +github.com/stretchr/testify v1.8.4/go.mod h1:sz/lmYIOXD/1dqDmKjjqLyZ2RngseejIcXlSw2iwfAo= github.com/subosito/gotenv v1.2.0/go.mod h1:N0PQaV/YGNqwC0u51sEeR/aUtSLEXKX9iv69rRypqCw= github.com/subosito/gotenv v1.4.2 h1:X1TuBLAMDFbaTAChgCBLu3DU3UPyELpnF2jjJ2cz/S8= github.com/subosito/gotenv v1.4.2/go.mod h1:ayKnFf/c6rvx/2iiLrJUk1e6plDbT3edrFNGqEflhK0= @@ -625,8 +625,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.30.7 h1:YO8CcDpLJzmq6PK5/CBQbXyV21iCMh8SbdXt+xNkXp8= -github.com/vmware/govmomi v0.30.7/go.mod h1:epgoslm97rLECMV4D+08ORzUBEU7boFSepKjt7AYVGg= +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 355c139efa..d4e0effe1a 100644 --- a/pkg/services/govmomi/service.go +++ b/pkg/services/govmomi/service.go @@ -371,10 +371,6 @@ func (vms *VMService) reconcileStoragePolicy(ctx *virtualMachineContext) error { 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 := ctx.Obj.Device(ctx) @@ -382,24 +378,41 @@ func (vms *VMService) reconcileStoragePolicy(ctx *virtualMachineContext) error { 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) //nolint:forcetypeassert - found := false // entities associated with storage policy has key in the form : diskID := fmt.Sprintf("%s:%d", ctx.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]) { + ctx.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}, }, @@ -408,6 +421,7 @@ func (vms *VMService) reconcileStoragePolicy(ctx *virtualMachineContext) error { } } + // If there are pending changes for Storage Policies, do it before moving next if len(changes) > 0 { task, err := ctx.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 b6115cb69d..eb48769193 100644 --- a/pkg/services/govmomi/service_test.go +++ b/pkg/services/govmomi/service_test.go @@ -18,11 +18,14 @@ package govmomi import ( goctx "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" "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 { @@ -95,9 +103,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(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 goctx.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(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 goctx.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(vmCtx) + g.Expect(err).ToNot(HaveOccurred()) + return nil + }, model) + }) +} + +func getAuthSession(ctx goctx.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 goctx.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 +}