fix: correctly handle static volumes (aws#2033)

For static volumes, we pull the CSI driver name off of the PV after it's bound instead of from the SC named on the PVC. The SC may not even exist in these cases and is informational. Fixes aws#1998
ellistarn · Dec 5, 2023 · 30c99c0 · 30c99c0
1 parent dc7c452
commit 30c99c0
Show file tree

Hide file tree

Showing 3 changed files with 162 additions and 10 deletions.
diff --git a/pkg/controllers/provisioning/scheduling/suite_test.go b/pkg/controllers/provisioning/scheduling/suite_test.go
@@ -4063,6 +4063,123 @@ var _ = Describe("Volume Limits", func() {
 		// 100 of the same PVC should all be schedulable on the same node
 		Expect(nodeList.Items).To(HaveLen(1))
 	})
+	It("should not fail for non-dynamic PVCs", func() {
+		const csiProvider = "fake.csi.provider"
+		cloudProv.InstanceTypes = []cloudprovider.InstanceType{
+			fake.NewInstanceType(
+				fake.InstanceTypeOptions{
+					Name: "instance-type",
+					Resources: map[v1.ResourceName]resource.Quantity{
+						v1.ResourceCPU:  resource.MustParse("1024"),
+						v1.ResourcePods: resource.MustParse("1024"),
+					},
+				}),
+		}
+
+		provisioner.Spec.Limits = nil
+		ExpectApplied(ctx, env.Client, provisioner)
+		initialPods := ExpectProvisioned(ctx, env.Client, controller, test.UnschedulablePod())
+		node := ExpectScheduled(ctx, env.Client, initialPods[0])
+		csiNode := &storagev1.CSINode{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: node.Name,
+			},
+			Spec: storagev1.CSINodeSpec{
+				Drivers: []storagev1.CSINodeDriver{
+					{
+						Name:   csiProvider,
+						NodeID: "fake-node-id",
+						Allocatable: &storagev1.VolumeNodeResources{
+							Count: aws.Int32(10),
+						},
+					},
+				},
+			},
+		}
+		ExpectApplied(ctx, env.Client, csiNode)
+		ExpectReconcileSucceeded(ctx, nodeStateController, client.ObjectKeyFromObject(node))
+
+		sc := test.StorageClass(test.StorageClassOptions{
+			ObjectMeta:  metav1.ObjectMeta{Name: "my-storage-class"},
+			Provisioner: aws.String(csiProvider),
+			Zones:       []string{"test-zone-1"}})
+		ExpectApplied(ctx, env.Client, sc)
+
+		pv := test.PersistentVolume(test.PersistentVolumeOptions{
+			ObjectMeta: metav1.ObjectMeta{Name: "my-volume"},
+			Driver:     csiProvider,
+			Zones:      []string{"test-zone-1"}})
+
+		pvc := test.PersistentVolumeClaim(test.PersistentVolumeClaimOptions{
+			ObjectMeta:       metav1.ObjectMeta{Name: "my-claim"},
+			VolumeName:       pv.Name,
+			StorageClassName: aws.String(""),
+		})
+		ExpectApplied(ctx, env.Client, pv, pvc)
+
+		var pods []*v1.Pod
+		for i := 0; i < 5; i++ {
+			pods = append(pods, test.UnschedulablePod(test.PodOptions{
+				PersistentVolumeClaims: []string{pvc.Name, pvc.Name},
+			}))
+		}
+		ExpectApplied(ctx, env.Client, provisioner)
+		pods = ExpectProvisioned(ctx, env.Client, controller, pods...)
+
+		var nodeList v1.NodeList
+		Expect(env.Client.List(ctx, &nodeList)).To(Succeed())
+		// 5 of the same PVC should all be schedulable on the same node
+		Expect(nodeList.Items).To(HaveLen(1))
+	})
+	It("should not fail for NFS volumes", func() {
+		cloudProv.InstanceTypes = []cloudprovider.InstanceType{
+			fake.NewInstanceType(
+				fake.InstanceTypeOptions{
+					Name: "instance-type",
+					Resources: map[v1.ResourceName]resource.Quantity{
+						v1.ResourceCPU:  resource.MustParse("1024"),
+						v1.ResourcePods: resource.MustParse("1024"),
+					},
+				}),
+		}
+
+		provisioner.Spec.Limits = nil
+		ExpectApplied(ctx, env.Client, provisioner)
+		initialPods := ExpectProvisioned(ctx, env.Client, controller, test.UnschedulablePod())
+		node := ExpectScheduled(ctx, env.Client, initialPods[0])
+		ExpectReconcileSucceeded(ctx, nodeStateController, client.ObjectKeyFromObject(node))
+
+		pv := test.PersistentVolume(test.PersistentVolumeOptions{
+			ObjectMeta:       metav1.ObjectMeta{Name: "my-volume"},
+			StorageClassName: "nfs",
+			Zones:            []string{"test-zone-1"}})
+		pv.Spec.NFS = &v1.NFSVolumeSource{
+			Server: "fake.server",
+			Path:   "/some/path",
+		}
+		pv.Spec.CSI = nil
+
+		pvc := test.PersistentVolumeClaim(test.PersistentVolumeClaimOptions{
+			ObjectMeta:       metav1.ObjectMeta{Name: "my-claim"},
+			VolumeName:       pv.Name,
+			StorageClassName: aws.String(""),
+		})
+		ExpectApplied(ctx, env.Client, pv, pvc)
+
+		var pods []*v1.Pod
+		for i := 0; i < 5; i++ {
+			pods = append(pods, test.UnschedulablePod(test.PodOptions{
+				PersistentVolumeClaims: []string{pvc.Name, pvc.Name},
+			}))
+		}
+		ExpectApplied(ctx, env.Client, provisioner)
+		pods = ExpectProvisioned(ctx, env.Client, controller, pods...)
+
+		var nodeList v1.NodeList
+		Expect(env.Client.List(ctx, &nodeList)).To(Succeed())
+		// 5 of the same PVC should all be schedulable on the same node
+		Expect(nodeList.Items).To(HaveLen(1))
+	})
 })
 
 func MakePods(count int, options test.PodOptions) (pods []*v1.Pod) {

diff --git a/pkg/scheduling/volumelimits.go b/pkg/scheduling/volumelimits.go
@@ -144,39 +144,70 @@ func (v *VolumeLimits) Validate(ctx context.Context, pod *v1.Pod) (VolumeCount,
 
 func (v *VolumeLimits) validate(ctx context.Context, pod *v1.Pod) (volumeUsage, error) {
 	podPVCs := volumeUsage{}
-
 	for _, volume := range pod.Spec.Volumes {
 		var pvcID string
 		var storageClassName *string
+		var volumeName string
+		var pvc v1.PersistentVolumeClaim
 		if volume.PersistentVolumeClaim != nil {
-			var pvc v1.PersistentVolumeClaim
 			if err := v.kubeClient.Get(ctx, client.ObjectKey{Namespace: pod.Namespace, Name: volume.PersistentVolumeClaim.ClaimName}, &pvc); err != nil {
 				return nil, err
 			}
-
 			pvcID = fmt.Sprintf("%s/%s", pod.Namespace, volume.PersistentVolumeClaim.ClaimName)
 			storageClassName = pvc.Spec.StorageClassName
+			volumeName = pvc.Spec.VolumeName
 		} else if volume.Ephemeral != nil {
 			// generated name per https://kubernetes.io/docs/concepts/storage/ephemeral-volumes/#persistentvolumeclaim-naming
 			pvcID = fmt.Sprintf("%s/%s-%s", pod.Namespace, pod.Name, volume.Name)
 			storageClassName = volume.Ephemeral.VolumeClaimTemplate.Spec.StorageClassName
+			volumeName = volume.Ephemeral.VolumeClaimTemplate.Spec.VolumeName
 		} else {
 			continue
 		}
 
-		provisioner := "unspecified"
-		if storageClassName != nil {
-			var sc storagev1.StorageClass
-			if err := v.kubeClient.Get(ctx, client.ObjectKey{Name: *storageClassName}, &sc); err != nil {
+		var driverName string
+		var err error
+		// We can track the volume usage by the CSI Driver name which is pulled from the storage class for dynamic
+		// volumes, or if it's bound/static we can pull the volume name
+		if volumeName != "" {
+			driverName, err = v.driverFromVolume(ctx, volumeName)
+			if err != nil {
+				return nil, err
+			}
+		} else if storageClassName != nil && *storageClassName != "" {
+			driverName, err = v.driverFromSC(ctx, storageClassName)
+			if err != nil {
 				return nil, err
 			}
-			provisioner = sc.Provisioner
 		}
-		podPVCs.Add(provisioner, pvcID)
+
+		// might be a non-CSI driver, something we don't currently handle
+		if driverName != "" {
+			podPVCs.Add(driverName, pvcID)
+		}
 	}
 	return podPVCs, nil
 }
 
+func (v *VolumeLimits) driverFromSC(ctx context.Context, storageClassName *string) (string, error) {
+	var sc storagev1.StorageClass
+	if err := v.kubeClient.Get(ctx, client.ObjectKey{Name: *storageClassName}, &sc); err != nil {
+		return "", err
+	}
+	return sc.Provisioner, nil
+}
+
+func (v *VolumeLimits) driverFromVolume(ctx context.Context, volumeName string) (string, error) {
+	var pv v1.PersistentVolume
+	if err := v.kubeClient.Get(ctx, client.ObjectKey{Name: volumeName}, &pv); err != nil {
+		return "", err
+	}
+	if pv.Spec.CSI != nil {
+		return pv.Spec.CSI.Driver, nil
+	}
+	return "", nil
+}
+
 func (v *VolumeLimits) DeletePod(key types.NamespacedName) {
 	delete(v.podVolumes, key)
 	// volume names could be duplicated, so we re-create our volumes

diff --git a/pkg/test/storage.go b/pkg/test/storage.go
@@ -30,6 +30,7 @@ type PersistentVolumeOptions struct {
 	metav1.ObjectMeta
 	Zones            []string
 	StorageClassName string
+	Driver           string
 }
 
 func PersistentVolume(overrides ...PersistentVolumeOptions) *v1.PersistentVolume {
@@ -39,10 +40,13 @@ func PersistentVolume(overrides ...PersistentVolumeOptions) *v1.PersistentVolume
 			panic(fmt.Sprintf("Failed to merge options: %s", err))
 		}
 	}
+	if options.Driver == "" {
+		options.Driver = "test.driver"
+	}
 	return &v1.PersistentVolume{
 		ObjectMeta: ObjectMeta(metav1.ObjectMeta{}),
 		Spec: v1.PersistentVolumeSpec{
-			PersistentVolumeSource: v1.PersistentVolumeSource{CSI: &v1.CSIPersistentVolumeSource{Driver: "test-driver", VolumeHandle: "test-handle"}},
+			PersistentVolumeSource: v1.PersistentVolumeSource{CSI: &v1.CSIPersistentVolumeSource{Driver: options.Driver, VolumeHandle: "test-handle"}},
 			StorageClassName:       options.StorageClassName,
 			AccessModes:            []v1.PersistentVolumeAccessMode{v1.ReadWriteOnce},
 			Capacity:               v1.ResourceList{v1.ResourceStorage: resource.MustParse("100Gi")},