Merge pull request #2482 from rikatz/v1.7-cherrypick-2467

[release-1.7] 🐛 Use a less expensive method for storage policy

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

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=

pkg/services/govmomi/service.go

@@ -371,35 +371,48 @@ 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)
 	if err != nil {
 		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 <vm-ID>:<disk>
 		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{

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
+}

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
+}