Merge pull request #743 from bryanv/bryanv/bound-pvc-placement-constr…

…aints

🌱 Handle unbound PVCs with Immediate SC during placement

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"))
 				})
 			})
 

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
 			}
 

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
 	}

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

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