From 79988c10765700961ca9be924651db9ac69ff28d Mon Sep 17 00:00:00 2001 From: Bryan Venteicher Date: Mon, 14 Oct 2024 11:23:11 -0500 Subject: [PATCH] Handle unbound PVCs Immediate SC during placement We must wait for PVCs with an immediate StorageClass to be bound, so we'll know the accessible zones. Similarly, for bound PVCs we should get its accessible zones as a part of the placement constraints. This is first pass intended to address the lowest hanging PVC placement work, with a lot of reworking of placement to be done later. Specifically, we should be smarter about whether we'll actually do placement or not, based on the cluster and VM config. --- .../vsphere/placement/zone_placement_test.go | 10 +- pkg/providers/vsphere/storage/storage.go | 2 + pkg/providers/vsphere/vmprovider_vm.go | 4 +- pkg/providers/vsphere/vmprovider_vm_test.go | 54 ++++ pkg/util/kube/storage.go | 78 ++++- pkg/util/kube/storage_test.go | 281 +++++++++++++----- 6 files changed, 328 insertions(+), 101 deletions(-) diff --git a/pkg/providers/vsphere/placement/zone_placement_test.go b/pkg/providers/vsphere/placement/zone_placement_test.go index 2f86e5cd5..60f44eacf 100644 --- a/pkg/providers/vsphere/placement/zone_placement_test.go +++ b/pkg/providers/vsphere/placement/zone_placement_test.go @@ -243,10 +243,9 @@ func vcSimPlacement() { Context("Allowed Zone Constraints", func() { Context("Only allowed zone does not exist", func() { It("returns error", func() { - constraints.Zones = sets.New("foobar") + constraints.Zones = sets.New("bogus-zone") _, err := placement.Placement(vmCtx, ctx.Client, ctx.VCClient.Client, configSpec, constraints) - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("no placement candidates available after applying zone constraints")) + Expect(err).To(MatchError("no placement candidates available after applying zone constraints: bogus-zone")) }) }) @@ -437,10 +436,9 @@ func vcSimPlacement() { Context("Allowed Zone Constraints", func() { Context("Only allowed zone does not exist", func() { It("returns error", func() { - constraints.Zones = sets.New("foobar") + constraints.Zones = sets.New("bogus-zone") _, err := placement.Placement(vmCtx, ctx.Client, ctx.VCClient.Client, configSpec, constraints) - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("no placement candidates available after applying zone constraints")) + Expect(err).To(MatchError("no placement candidates available after applying zone constraints: bogus-zone")) }) }) diff --git a/pkg/providers/vsphere/storage/storage.go b/pkg/providers/vsphere/storage/storage.go index 8e7750013..658154a4f 100644 --- a/pkg/providers/vsphere/storage/storage.go +++ b/pkg/providers/vsphere/storage/storage.go @@ -90,6 +90,8 @@ func getVMStorageClassNamesAndPVCs( pvc := &corev1.PersistentVolumeClaim{} pvcKey := ctrlclient.ObjectKey{Name: claim.ClaimName, Namespace: vm.Namespace} if err := client.Get(ctx, pvcKey, pvc); err != nil { + // TODO: If later we won't actually do placement, IMO this shouldn't be fatal. Need + // to rework the code a bit so this isn't as clunky. return nil, nil, err } diff --git a/pkg/providers/vsphere/vmprovider_vm.go b/pkg/providers/vsphere/vmprovider_vm.go index c60e4095d..1766dc39f 100644 --- a/pkg/providers/vsphere/vmprovider_vm.go +++ b/pkg/providers/vsphere/vmprovider_vm.go @@ -592,7 +592,9 @@ func (vs *vSphereVMProvider) vmCreateDoPlacement( return err } - pvcZones, err := kubeutil.GetPVCZoneConstraints(createArgs.Storage.PVCs) + pvcZones, err := kubeutil.GetPVCZoneConstraints( + createArgs.Storage.StorageClasses, + createArgs.Storage.PVCs) if err != nil { return err } diff --git a/pkg/providers/vsphere/vmprovider_vm_test.go b/pkg/providers/vsphere/vmprovider_vm_test.go index 38b31d529..6df3822c7 100644 --- a/pkg/providers/vsphere/vmprovider_vm_test.go +++ b/pkg/providers/vsphere/vmprovider_vm_test.go @@ -1915,6 +1915,7 @@ func vmTests() { JustBeforeEach(func() { delete(vm.Labels, topology.KubernetesTopologyZoneLabelKey) }) + It("creates VM in placement selected zone", func() { Expect(vm.Labels).ToNot(HaveKey(topology.KubernetesTopologyZoneLabelKey)) vcVM, err := createOrUpdateAndGetVcVM(ctx, vm) @@ -1935,6 +1936,7 @@ func vmTests() { }) It("creates VM in assigned zone", func() { + Expect(len(ctx.ZoneNames)).To(BeNumerically(">", 1)) azName := ctx.ZoneNames[rand.Intn(len(ctx.ZoneNames))] vm.Labels[topology.KubernetesTopologyZoneLabelKey] = azName @@ -1950,6 +1952,58 @@ func vmTests() { }) }) + When("VM zone is constrained by PVC", func() { + BeforeEach(func() { + // Need to create the PVC before creating the VM. + skipCreateOrUpdateVM = true + + vm.Spec.Volumes = []vmopv1.VirtualMachineVolume{ + { + Name: "dummy-vol", + VirtualMachineVolumeSource: vmopv1.VirtualMachineVolumeSource{ + PersistentVolumeClaim: &vmopv1.PersistentVolumeClaimVolumeSource{ + PersistentVolumeClaimVolumeSource: corev1.PersistentVolumeClaimVolumeSource{ + ClaimName: "pvc-claim-1", + }, + }, + }, + }, + } + + }) + + It("creates VM in allowed zone", func() { + Expect(len(ctx.ZoneNames)).To(BeNumerically(">", 1)) + azName := ctx.ZoneNames[rand.Intn(len(ctx.ZoneNames))] + + // Make sure we do placement. + delete(vm.Labels, topology.KubernetesTopologyZoneLabelKey) + + pvc1 := &corev1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pvc-claim-1", + Namespace: vm.Namespace, + Annotations: map[string]string{ + "csi.vsphere.volume-accessible-topology": fmt.Sprintf(`[{"topology.kubernetes.io/zone":"%s"}]`, azName), + }, + }, + Spec: corev1.PersistentVolumeClaimSpec{ + StorageClassName: ptr.To(ctx.StorageClassName), + }, + Status: corev1.PersistentVolumeClaimStatus{ + Phase: corev1.ClaimBound, + }, + } + Expect(ctx.Client.Create(ctx, pvc1)).To(Succeed()) + Expect(ctx.Client.Status().Update(ctx, pvc1)).To(Succeed()) + + vm.Spec.PowerState = vmopv1.VirtualMachinePowerStateOff + _, err := createOrUpdateAndGetVcVM(ctx, vm) + Expect(err).ToNot(HaveOccurred()) + Expect(vm.Status.Zone).To(Equal(azName)) + }) + }) + Context("When Instance Storage FSS is enabled", func() { BeforeEach(func() { testConfig.WithInstanceStorage = true diff --git a/pkg/util/kube/storage.go b/pkg/util/kube/storage.go index 78f33bc4e..054af0b07 100644 --- a/pkg/util/kube/storage.go +++ b/pkg/util/kube/storage.go @@ -21,11 +21,12 @@ import ( ) const ( - csiTopologyAnnotation = "csi.vsphere.volume-requested-topology" + csiRequestedTopologyAnnotation = "csi.vsphere.volume-requested-topology" + csiAccessibleTopologyAnnotation = "csi.vsphere.volume-accessible-topology" ) -func getPVCRequestedZones(pvc corev1.PersistentVolumeClaim) sets.Set[string] { - v, ok := pvc.Annotations[csiTopologyAnnotation] +func getPVCZones(pvc corev1.PersistentVolumeClaim, key string) sets.Set[string] { + v, ok := pvc.Annotations[key] if !ok { return nil } @@ -44,25 +45,74 @@ func getPVCRequestedZones(pvc corev1.PersistentVolumeClaim) sets.Set[string] { return zones } -// GetPVCZoneConstraints gets the set of the allowed zones, if any, for the PVCs. -func GetPVCZoneConstraints(pvcs []corev1.PersistentVolumeClaim) (sets.Set[string], error) { +func getPVCRequestedZones(pvc corev1.PersistentVolumeClaim) sets.Set[string] { + return getPVCZones(pvc, csiRequestedTopologyAnnotation) +} + +func getPVCAccessibleZones(pvc corev1.PersistentVolumeClaim) sets.Set[string] { + return getPVCZones(pvc, csiAccessibleTopologyAnnotation) +} + +func GetPVCZoneConstraints( + storageClasses map[string]storagev1.StorageClass, + pvcs []corev1.PersistentVolumeClaim) (sets.Set[string], error) { + var zones sets.Set[string] for _, pvc := range pvcs { - reqZones := getPVCRequestedZones(pvc) - if reqZones.Len() == 0 { + var z sets.Set[string] + + // We don't expect anything else but check since we check CSI specific annotations below. + if v, ok := pvc.Annotations["volume.kubernetes.io/storage-provisioner"]; ok && v != "csi.vsphere.vmware.com" { continue } - if zones == nil { - zones = reqZones - } else { - t := zones.Intersection(reqZones) - if t.Len() == 0 { - return nil, fmt.Errorf("no common zones remaining after applying zone contraints for PVC %s", pvc.Name) + switch pvc.Status.Phase { + case corev1.ClaimBound: + // Easy case: if this PVC is already bound then just get its accessible zones, if any + // (the PVC's StorageClass should be Immediate). + z = getPVCAccessibleZones(pvc) + + case corev1.ClaimPending: + var isImmediate bool + + // We need to determine if the PVC's StorageClass is Immediate and we just + // caught this PVC before it was bound, or otherwise it is WFFC and we need + // to limit our placement candidates to its requested zones. + + // While we have the DefaultStorageClass feature gate enabled, we don't mark an + // StorageClass with storageclass.kubernetes.io/is-default-class, so don't bother + // trying to resolve that here now. + if scName := pvc.Spec.StorageClassName; scName != nil && *scName != "" { + if sc, ok := storageClasses[*scName]; ok { + isImmediate = sc.VolumeBindingMode == nil || *sc.VolumeBindingMode == storagev1.VolumeBindingImmediate + } } - zones = t + if isImmediate { + // Must wait for this PVC to get bound so we know its accessible zones. + return nil, fmt.Errorf("PVC %s is not bound", pvc.Name) + } + + z = getPVCRequestedZones(pvc) + + default: // corev1.ClaimLost + // TBD: For now preserve our prior behavior: don't explicitly fail here, and create the + // VM and wait for the volume to be attached prior to powering on the VM. + continue + } + + if z.Len() > 0 { + if zones == nil { + zones = z + } else { + t := zones.Intersection(z) + if t.Len() == 0 { + return nil, fmt.Errorf("no allowed zones remaining after applying PVC zone constraints") + } + + zones = t + } } } diff --git a/pkg/util/kube/storage_test.go b/pkg/util/kube/storage_test.go index 77ac1f5bf..8a3984c40 100644 --- a/pkg/util/kube/storage_test.go +++ b/pkg/util/kube/storage_test.go @@ -28,6 +28,8 @@ import ( pkgcfg "github.com/vmware-tanzu/vm-operator/pkg/config" kubeutil "github.com/vmware-tanzu/vm-operator/pkg/util/kube" "github.com/vmware-tanzu/vm-operator/pkg/util/kube/internal" + "github.com/vmware-tanzu/vm-operator/pkg/util/ptr" + "github.com/vmware-tanzu/vm-operator/test/builder" ) var _ = DescribeTable("GetStoragePolicyID", @@ -62,108 +64,227 @@ var _ = DescribeTable("GetStoragePolicyID", ) var _ = Describe("GetPVCZoneConstraints", func() { + It("Unmarshal JSON", func() { pvcs := make([]corev1.PersistentVolumeClaim, 1) pvcs[0] = corev1.PersistentVolumeClaim{ ObjectMeta: metav1.ObjectMeta{ - Name: "pvc", + Name: "bound-pvc", + Annotations: map[string]string{ + "csi.vsphere.volume-accessible-topology": `[{"topology.kubernetes.io/zone":"zone1"},{"topology.kubernetes.io/zone":"zone2"},{"topology.kubernetes.io/zone":"zone3"}]`, + }, + }, + Status: corev1.PersistentVolumeClaimStatus{ + Phase: corev1.ClaimBound, + }, + } + zones, err := kubeutil.GetPVCZoneConstraints(nil, pvcs) + Expect(err).ToNot(HaveOccurred()) + Expect(zones.UnsortedList()).To(ConsistOf("zone1", "zone2", "zone3")) + }) + + It("Unmarshal JSON", func() { + pvcs := make([]corev1.PersistentVolumeClaim, 1) + pvcs[0] = corev1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pending-pvc", Annotations: map[string]string{ "csi.vsphere.volume-requested-topology": `[{"topology.kubernetes.io/zone":"zone1"},{"topology.kubernetes.io/zone":"zone2"},{"topology.kubernetes.io/zone":"zone3"}]`, }, }, + Status: corev1.PersistentVolumeClaimStatus{ + Phase: corev1.ClaimPending, + }, } - zones, err := kubeutil.GetPVCZoneConstraints(pvcs) + zones, err := kubeutil.GetPVCZoneConstraints(nil, pvcs) Expect(err).ToNot(HaveOccurred()) Expect(zones.UnsortedList()).To(ConsistOf("zone1", "zone2", "zone3")) }) -}) -var _ = DescribeTable("GetPVCZoneConstraints Table", - func(pvcsZones [][]string, expZones []string, expErr string) { - var pvcs []corev1.PersistentVolumeClaim - for i, zones := range pvcsZones { - var annotations map[string]string + Context("Pending PVC with Immediate StorageClass", func() { - if len(zones) > 0 { - var topology []map[string]string - for _, z := range zones { - topology = append(topology, map[string]string{"topology.kubernetes.io/zone": z}) - } + It("Returns error", func() { + sc := *builder.DummyStorageClass() + sc.VolumeBindingMode = ptr.To(storagev1.VolumeBindingImmediate) + storageClasses := map[string]storagev1.StorageClass{ + sc.Name: sc, + } - requestedZones, err := json.Marshal(topology) - Expect(err).NotTo(HaveOccurred()) - annotations = map[string]string{ - "csi.vsphere.volume-requested-topology": string(requestedZones), - } + pvcs := make([]corev1.PersistentVolumeClaim, 1) + pvcs[0] = corev1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pending-pvc", + Annotations: map[string]string{ + "csi.vsphere.volume-requested-topology": `[{"topology.kubernetes.io/zone":"zone1"}]`, + }, + }, + Spec: corev1.PersistentVolumeClaimSpec{ + StorageClassName: &sc.Name, + }, + Status: corev1.PersistentVolumeClaimStatus{ + Phase: corev1.ClaimPending, + }, } - pvc := corev1.PersistentVolumeClaim{ + _, err := kubeutil.GetPVCZoneConstraints(storageClasses, pvcs) + Expect(err).To(MatchError("PVC pending-pvc is not bound")) + }) + }) + + Context("Bound PVC with Immediate StorageClass and Unbound PVC with WFFC StorageClass ", func() { + + It("Returns success with common zones", func() { + immdSC := *builder.DummyStorageClass() + immdSC.VolumeBindingMode = ptr.To(storagev1.VolumeBindingImmediate) + wffcSC := *builder.DummyStorageClass() + wffcSC.Name += "-wffc" + wffcSC.VolumeBindingMode = ptr.To(storagev1.VolumeBindingWaitForFirstConsumer) + + storageClasses := map[string]storagev1.StorageClass{ + immdSC.Name: immdSC, + wffcSC.Name: wffcSC, + } + + pvcs := make([]corev1.PersistentVolumeClaim, 2) + pvcs[0] = corev1.PersistentVolumeClaim{ ObjectMeta: metav1.ObjectMeta{ - Name: fmt.Sprintf("pvc-%d", i), - Annotations: annotations, + Name: "bound-pvc", + Annotations: map[string]string{ + "csi.vsphere.volume-accessible-topology": `[{"topology.kubernetes.io/zone":"zone1"},{"topology.kubernetes.io/zone":"zoneX"}]`, + }, + }, + Spec: corev1.PersistentVolumeClaimSpec{ + StorageClassName: &immdSC.Name, + }, + Status: corev1.PersistentVolumeClaimStatus{ + Phase: corev1.ClaimBound, + }, + } + pvcs[1] = corev1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pending-pvc", + Annotations: map[string]string{ + "csi.vsphere.volume-requested-topology": `[{"topology.kubernetes.io/zone":"zone1"},{"topology.kubernetes.io/zone":"zoneY"}]`, + }, + }, + Spec: corev1.PersistentVolumeClaimSpec{ + StorageClassName: &wffcSC.Name, + }, + Status: corev1.PersistentVolumeClaimStatus{ + Phase: corev1.ClaimPending, }, } - pvcs = append(pvcs, pvc) - } - zones, err := kubeutil.GetPVCZoneConstraints(pvcs) - if expErr != "" { - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring(expErr)) - Expect(zones).To(BeEmpty()) - } else { - Expect(zones.UnsortedList()).To(ConsistOf(expZones)) - } + zones, err := kubeutil.GetPVCZoneConstraints(storageClasses, pvcs) + Expect(err).ToNot(HaveOccurred()) + Expect(zones.UnsortedList()).To(ConsistOf("zone1")) + }) + }) +}) + +var _ = DescribeTableSubtree("GetPVCZoneConstraints Table", + + func(phase corev1.PersistentVolumeClaimPhase) { + + DescribeTable("PVC Phase", + func(pvcsZones [][]string, expZones []string, expErr string) { + var pvcs []corev1.PersistentVolumeClaim + for i, zones := range pvcsZones { + var annotations map[string]string + + if len(zones) > 0 { + var topology []map[string]string + for _, z := range zones { + topology = append(topology, map[string]string{"topology.kubernetes.io/zone": z}) + } + + volTopology, err := json.Marshal(topology) + Expect(err).NotTo(HaveOccurred()) + if phase == corev1.ClaimBound { + annotations = map[string]string{ + "csi.vsphere.volume-accessible-topology": string(volTopology), + } + } else { + annotations = map[string]string{ + "csi.vsphere.volume-requested-topology": string(volTopology), + } + } + } + + pvc := corev1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: fmt.Sprintf("pvc-%d", i), + Annotations: annotations, + }, + Status: corev1.PersistentVolumeClaimStatus{ + Phase: phase, + }, + } + pvcs = append(pvcs, pvc) + } + + zones, err := kubeutil.GetPVCZoneConstraints(nil, pvcs) + if expErr != "" { + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring(expErr)) + Expect(zones).To(BeEmpty()) + } else { + Expect(zones.UnsortedList()).To(ConsistOf(expZones)) + } + }, + Entry( + "One PVC with no zones", + [][]string{ + {}, + }, + []string{}, + ""), + Entry( + "One PVC with one zone", + [][]string{ + {"zone1"}, + }, + []string{"zone1"}, + ""), + Entry( + "One PVC with one zone and PVC without zone", + [][]string{ + {}, + {"zone1"}, + }, + []string{"zone1"}, + ""), + Entry( + "Multiple PVCs with one common zone", + [][]string{ + {"zone1", "zoneX"}, + {"zone1", "zoneY"}, + {"zone1", "zoneZ"}, + }, + []string{"zone1"}, + ""), + Entry( + "Multiple PVCs with multiple common zones", + [][]string{ + {"zone1", "zoneX", "zone2"}, + {"zone1", "zoneY", "zone2"}, + {"zone1", "zoneZ", "zone2"}, + }, + []string{"zone1", "zone2"}, + ""), + Entry( + "Multiple PVCs with no common zones", + [][]string{ + {"zoneX"}, + {"zoneY", "zone1"}, + {"zoneZ", "zone1"}, + }, + nil, + "no allowed zones remaining after applying PVC zone constraints"), + ) }, - Entry( - "One PVC with no zones", - [][]string{ - {}, - }, - []string{}, - ""), - Entry( - "One PVC with one zone", - [][]string{ - {"zone1"}, - }, - []string{"zone1"}, - ""), - Entry( - "One PVC with one zone and PVC without zone", - [][]string{ - {}, - {"zone1"}, - }, - []string{"zone1"}, - ""), - Entry( - "Multiple PVCs with one common zone", - [][]string{ - {"zone1", "zoneX"}, - {"zone1", "zoneY"}, - {"zone1", "zoneZ"}, - }, - []string{"zone1"}, - ""), - Entry( - "Multiple PVCs with multiple common zones", - [][]string{ - {"zone1", "zoneX", "zone2"}, - {"zone1", "zoneY", "zone2"}, - {"zone1", "zoneZ", "zone2"}, - }, - []string{"zone1", "zone2"}, - ""), - Entry( - "Multiple PVCs with no common zones", - [][]string{ - {"zoneX"}, - {"zoneY", "zone1"}, - {"zoneZ", "zone1"}, - }, - nil, - "no common zones remaining"), + Entry("Bound", corev1.ClaimBound), + Entry("Pending", corev1.ClaimPending), ) var _ = Describe("IsEncryptedStorageClass", func() {