Merge pull request #2467 from rikatz/improve-storage-policy-query

🐛 Use a cheaper method for storage policy

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

go.sum

@@ -621,8 +621,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=

pkg/services/govmomi/service.go

@@ -394,35 +394,48 @@ 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)
 	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)
-		found := false
 		// entities associated with storage policy has key in the form <vm-ID>:<disk>
 		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{

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

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