Merge pull request kubernetes#121902 from carlory/kep-3751-pv-controller

[kep-3751] pvc bind pv with vac

Author: k8s-ci-robot

pkg/apis/core/validation/validation.go

@@ -2399,7 +2399,9 @@ func ValidatePersistentVolumeClaimUpdate(newPvc, oldPvc *core.PersistentVolumeCl
 		newPvcClone.Spec.Resources.Requests["storage"] = oldPvc.Spec.Resources.Requests["storage"] // +k8s:verify-mutation:reason=clone
 	}
 	// lets make sure volume attributes class name is same.
-	newPvcClone.Spec.VolumeAttributesClassName = oldPvcClone.Spec.VolumeAttributesClassName // +k8s:verify-mutation:reason=clone
+	if newPvc.Status.Phase == core.ClaimBound && newPvcClone.Spec.VolumeAttributesClassName != nil {
+		newPvcClone.Spec.VolumeAttributesClassName = oldPvcClone.Spec.VolumeAttributesClassName // +k8s:verify-mutation:reason=clone
+	}
 
 	oldSize := oldPvc.Spec.Resources.Requests["storage"]
 	newSize := newPvc.Spec.Resources.Requests["storage"]

pkg/apis/core/validation/validation_test.go

@@ -3007,6 +3007,20 @@ func TestValidatePersistentVolumeClaimUpdate(t *testing.T) {
 			enableVolumeAttributesClass: true,
 			isExpectedFailure:           true,
 		},
+		"invalid-update-volume-attributes-class-when-claim-not-bound": {
+			oldClaim: func() *core.PersistentVolumeClaim {
+				clone := validClaimVolumeAttributesClass1.DeepCopy()
+				clone.Status.Phase = core.ClaimPending
+				return clone
+			}(),
+			newClaim: func() *core.PersistentVolumeClaim {
+				clone := validClaimVolumeAttributesClass2.DeepCopy()
+				clone.Status.Phase = core.ClaimPending
+				return clone
+			}(),
+			enableVolumeAttributesClass: true,
+			isExpectedFailure:           true,
+		},
 		"invalid-update-volume-attributes-class-to-nil-without-featuregate-enabled": {
 			oldClaim:                    validClaimVolumeAttributesClass1,
 			newClaim:                    validClaimNilVolumeAttributesClass,

pkg/controller/volume/persistentvolume/binder_test.go

@@ -17,13 +17,17 @@ limitations under the License.
 package persistentvolume
 
 import (
+	"fmt"
 	"testing"
 
 	v1 "k8s.io/api/core/v1"
 	storage "k8s.io/api/storage/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	utilfeature "k8s.io/apiserver/pkg/util/feature"
+	featuregatetesting "k8s.io/component-base/featuregate/testing"
 	"k8s.io/component-helpers/storage/volume"
 	"k8s.io/klog/v2/ktesting"
+	"k8s.io/kubernetes/pkg/features"
 )
 
 // Test single call to syncClaim and syncVolume methods.
@@ -750,13 +754,73 @@ func TestSync(t *testing.T) {
 			test:            testSyncClaim,
 		},
 	}
-	_, ctx := ktesting.NewTestContext(t)
-	runSyncTests(t, ctx, tests, []*storage.StorageClass{
+
+	// Once the feature-gate VolumeAttributesClass is promoted to GA, merge it with the above tests
+	whenFeatureGateEnabled := []controllerTest{
+		{
+			// syncClaim with claim pre-bound to a PV that exists and is
+			// unbound, but its volume attributes class does not match. Check that the claim status is reset to Pending
+			name:            "2-11 - claim prebound to unbound volume that volume attributes class is different",
+			initialVolumes:  volumesWithVAC(classGold, newVolumeArray("volume2-11", "1Gi", "", "", v1.VolumeAvailable, v1.PersistentVolumeReclaimRetain, classEmpty)),
+			expectedVolumes: volumesWithVAC(classGold, newVolumeArray("volume2-11", "1Gi", "", "", v1.VolumeAvailable, v1.PersistentVolumeReclaimRetain, classEmpty)),
+			initialClaims:   newClaimArray("claim2-11", "uid2-11", "1Gi", "volume2-11", v1.ClaimBound, nil),
+			expectedClaims:  newClaimArray("claim2-11", "uid2-11", "1Gi", "volume2-11", v1.ClaimPending, nil),
+			expectedEvents:  []string{"Warning VolumeMismatch"},
+			errors:          noerrors,
+			test:            testSyncClaim,
+		},
 		{
-			ObjectMeta:        metav1.ObjectMeta{Name: classWait},
-			VolumeBindingMode: &modeWait,
+			// syncClaim with claim pre-bound to a PV that exists and is
+			// unbound, they have the same volume attributes class. Check it gets bound and no volume.AnnBoundByController is set.
+			name:            "2-12 - claim prebound to unbound volume that volume attributes class is same",
+			initialVolumes:  volumesWithVAC(classGold, newVolumeArray("volume2-12", "1Gi", "", "", v1.VolumeAvailable, v1.PersistentVolumeReclaimRetain, classEmpty)),
+			expectedVolumes: volumesWithVAC(classGold, newVolumeArray("volume2-12", "1Gi", "uid2-12", "claim2-12", v1.VolumeBound, v1.PersistentVolumeReclaimRetain, classEmpty, volume.AnnBoundByController)),
+			initialClaims:   claimWithVAC(&classGold, newClaimArray("claim2-12", "uid2-12", "1Gi", "volume2-12", v1.ClaimPending, nil)),
+			expectedClaims:  withExpectedVAC(&classGold, claimWithVAC(&classGold, newClaimArray("claim2-12", "uid2-12", "1Gi", "volume2-12", v1.ClaimBound, nil, volume.AnnBindCompleted))),
+			expectedEvents:  noevents,
+			errors:          noerrors,
+			test:            testSyncClaim,
 		},
-	}, []*v1.Pod{})
+	}
+
+	// Once the feature-gate VolumeAttributesClass is promoted to GA, remove it
+	whenFeatureGateDisabled := []controllerTest{
+		{
+			// syncClaim with claim pre-bound to a PV that exists and is
+			// unbound, they have the same volume attributes class but the feature-gate is disabled. Check that the claim status is reset to Pending
+			name:            "2-13 - claim prebound to unbound volume that volume attributes class is same",
+			initialVolumes:  volumesWithVAC(classGold, newVolumeArray("volume2-13", "1Gi", "", "", v1.VolumeAvailable, v1.PersistentVolumeReclaimRetain, classEmpty)),
+			expectedVolumes: volumesWithVAC(classGold, newVolumeArray("volume2-13", "1Gi", "", "", v1.VolumeAvailable, v1.PersistentVolumeReclaimRetain, classEmpty)),
+			initialClaims:   withExpectedVAC(&classGold, claimWithVAC(&classGold, newClaimArray("claim2-13", "uid2-13", "1Gi", "volume2-13", v1.ClaimBound, nil))),
+			expectedClaims:  claimWithVAC(&classGold, newClaimArray("claim2-13", "uid2-13", "1Gi", "volume2-13", v1.ClaimPending, nil)),
+			expectedEvents:  []string{"Warning VolumeMismatch"},
+			errors:          noerrors,
+			test:            testSyncClaim,
+		},
+	}
+
+	for _, isEnabled := range []bool{true, false} {
+		featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.VolumeAttributesClass, isEnabled)
+
+		allTests := tests
+		if isEnabled {
+			allTests = append(allTests, whenFeatureGateEnabled...)
+		} else {
+			allTests = append(allTests, whenFeatureGateDisabled...)
+		}
+
+		for i := range allTests {
+			allTests[i].name = fmt.Sprintf("features.VolumeAttributesClass=%v %s", isEnabled, allTests[i].name)
+		}
+
+		_, ctx := ktesting.NewTestContext(t)
+		runSyncTests(t, ctx, allTests, []*storage.StorageClass{
+			{
+				ObjectMeta:        metav1.ObjectMeta{Name: classWait},
+				VolumeBindingMode: &modeWait,
+			},
+		}, []*v1.Pod{})
+	}
 }
 
 func TestSyncBlockVolume(t *testing.T) {

pkg/controller/volume/persistentvolume/framework_test.go

@@ -409,6 +409,14 @@ func withExpectedCapacity(capacity string, claims []*v1.PersistentVolumeClaim) [
 	return claims
 }
 
+// withExpectedVAC sets the claim.Status.CurrentVolumeAttributesClassName of the first claim in the
+// array to given value and returns the array.  Meant to be used to compose
+// claims specified inline in a test.
+func withExpectedVAC(vacName *string, claims []*v1.PersistentVolumeClaim) []*v1.PersistentVolumeClaim {
+	claims[0].Status.CurrentVolumeAttributesClassName = vacName
+	return claims
+}
+
 // withMessage saves given message into volume.Status.Message of the first
 // volume in the array and returns the array.  Meant to be used to compose
 // volumes specified inline in a test.
@@ -419,6 +427,7 @@ func withMessage(message string, volumes []*v1.PersistentVolume) []*v1.Persisten
 
 // newVolumeArray returns array with a single volume that would be returned by
 // newVolume() with the same parameters.
+// TODO: make the newVolumeArray function accept volume attributes class name as an input parameter
 func newVolumeArray(name, capacity, boundToClaimUID, boundToClaimName string, phase v1.PersistentVolumePhase, reclaimPolicy v1.PersistentVolumeReclaimPolicy, class string, annotations ...string) []*v1.PersistentVolume {
 	return []*v1.PersistentVolume{
 		newVolume(name, capacity, boundToClaimUID, boundToClaimName, phase, reclaimPolicy, class, annotations...),
@@ -489,19 +498,28 @@ func newClaim(name, claimUID, capacity, boundToVolume string, phase v1.Persisten
 		// For most of the tests it's enough to copy claim's requested capacity,
 		// individual tests can adjust it using withExpectedCapacity()
 		claim.Status.Capacity = claim.Spec.Resources.Requests
+		// For most of the tests it's enough to copy claim's requested vac,
+		// individual tests can adjust it using withExpectedVAC()
+		claim.Status.CurrentVolumeAttributesClassName = claim.Spec.VolumeAttributesClassName
 	}
 
 	return &claim
 }
 
 // newClaimArray returns array with a single claim that would be returned by
 // newClaim() with the same parameters.
+// TODO: make the newClaimArray function accept volume attributes class name as an input parameter
 func newClaimArray(name, claimUID, capacity, boundToVolume string, phase v1.PersistentVolumeClaimPhase, class *string, annotations ...string) []*v1.PersistentVolumeClaim {
 	return []*v1.PersistentVolumeClaim{
 		newClaim(name, claimUID, capacity, boundToVolume, phase, class, annotations...),
 	}
 }
 
+func claimWithVAC(vacName *string, claims []*v1.PersistentVolumeClaim) []*v1.PersistentVolumeClaim {
+	claims[0].Spec.VolumeAttributesClassName = vacName
+	return claims
+}
+
 // claimWithAnnotation saves given annotation into given claims. Meant to be
 // used to compose claims specified inline in a test.
 // TODO(refactor): This helper function (and other helpers related to claim
@@ -536,6 +554,22 @@ func annotateClaim(claim *v1.PersistentVolumeClaim, ann map[string]string) *v1.P
 	return claim
 }
 
+// volumeWithVAC saves given vac into given volume.
+// Meant to be used to compose volume specified inline in a test.
+func volumeWithVAC(vacName string, volume *v1.PersistentVolume) *v1.PersistentVolume {
+	volume.Spec.VolumeAttributesClassName = &vacName
+	return volume
+}
+
+// volumesWithVAC saves given vac into given volumes.
+// Meant to be used to compose volumes specified inline in a test.
+func volumesWithVAC(vacName string, volumes []*v1.PersistentVolume) []*v1.PersistentVolume {
+	for _, volume := range volumes {
+		volumeWithVAC(vacName, volume)
+	}
+	return volumes
+}
+
 // volumeWithAnnotation saves given annotation into given volume.
 // Meant to be used to compose volume specified inline in a test.
 func volumeWithAnnotation(name, value string, volume *v1.PersistentVolume) *v1.PersistentVolume {

pkg/controller/volume/persistentvolume/index.go

@@ -21,9 +21,11 @@ import (
 	"sort"
 
 	v1 "k8s.io/api/core/v1"
+	utilfeature "k8s.io/apiserver/pkg/util/feature"
 	"k8s.io/client-go/tools/cache"
 	"k8s.io/component-helpers/storage/volume"
 	v1helper "k8s.io/kubernetes/pkg/apis/core/v1/helper"
+	"k8s.io/kubernetes/pkg/features"
 	"k8s.io/kubernetes/pkg/volume/util"
 )
 
@@ -92,7 +94,7 @@ func (pvIndex *persistentVolumeOrderedIndex) findByClaim(claim *v1.PersistentVol
 			return nil, err
 		}
 
-		bestVol, err := volume.FindMatchingVolume(claim, volumes, nil /* node for topology binding*/, nil /* exclusion map */, delayBinding)
+		bestVol, err := volume.FindMatchingVolume(claim, volumes, nil /* node for topology binding*/, nil /* exclusion map */, delayBinding, utilfeature.DefaultFeatureGate.Enabled(features.VolumeAttributesClass))
 		if err != nil {
 			return nil, err
 		}

pkg/controller/volume/persistentvolume/index_test.go

@@ -23,9 +23,12 @@ import (
 	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	utilfeature "k8s.io/apiserver/pkg/util/feature"
 	"k8s.io/client-go/kubernetes/scheme"
 	ref "k8s.io/client-go/tools/reference"
+	featuregatetesting "k8s.io/component-base/featuregate/testing"
 	"k8s.io/component-helpers/storage/volume"
+	"k8s.io/kubernetes/pkg/features"
 	"k8s.io/kubernetes/pkg/volume/util"
 )
 
@@ -75,6 +78,9 @@ func makeVolumeModePVC(size string, mode *v1.PersistentVolumeMode, modfn func(*v
 }
 
 func TestMatchVolume(t *testing.T) {
+	// Default enable the VolumeAttributesClass feature gate.
+	featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.VolumeAttributesClass, true)
+
 	volList := newPersistentVolumeOrderedIndex()
 	for _, pv := range createTestVolumes() {
 		volList.store.Add(pv)
@@ -134,6 +140,24 @@ func TestMatchVolume(t *testing.T) {
 				pvc.Spec.AccessModes = []v1.PersistentVolumeAccessMode{v1.ReadWriteOnce}
 			}),
 		},
+		"successful-match-with-empty-vac": {
+			expectedMatch: "gce-pd-10",
+			claim: makePVC("8G", func(pvc *v1.PersistentVolumeClaim) {
+				pvc.Spec.VolumeAttributesClassName = &classEmpty
+			}),
+		},
+		"successful-match-with-vac": {
+			expectedMatch: "gce-pd-vac-silver1",
+			claim: makePVC("1G", func(pvc *v1.PersistentVolumeClaim) {
+				pvc.Spec.VolumeAttributesClassName = &classSilver
+			}),
+		},
+		"successful-no-match-vac-nonexisting": {
+			expectedMatch: "",
+			claim: makePVC("1G", func(pvc *v1.PersistentVolumeClaim) {
+				pvc.Spec.VolumeAttributesClassName = &classNonExisting
+			}),
+		},
 		"successful-match-with-class": {
 			expectedMatch: "gce-pd-silver1",
 			claim: makePVC("1G", func(pvc *v1.PersistentVolumeClaim) {
@@ -962,6 +986,29 @@ func createTestVolumes() []*v1.PersistentVolume {
 				VolumeMode:       &fs,
 			},
 		},
+		{
+			ObjectMeta: metav1.ObjectMeta{
+				UID:  "gce-pd-vac-silver1",
+				Name: "gce-pd-vac-silver1",
+			},
+			Spec: v1.PersistentVolumeSpec{
+				Capacity: v1.ResourceList{
+					v1.ResourceName(v1.ResourceStorage): resource.MustParse("100G"),
+				},
+				PersistentVolumeSource: v1.PersistentVolumeSource{
+					GCEPersistentDisk: &v1.GCEPersistentDiskVolumeSource{},
+				},
+				AccessModes: []v1.PersistentVolumeAccessMode{
+					v1.ReadWriteOnce,
+					v1.ReadOnlyMany,
+				},
+				VolumeAttributesClassName: &classSilver,
+				VolumeMode:                &fs,
+			},
+			Status: v1.PersistentVolumeStatus{
+				Phase: v1.VolumeAvailable,
+			},
+		},
 	}
 }
 

pkg/controller/volume/persistentvolume/pv_controller.go

@@ -26,6 +26,7 @@ import (
 	utilfeature "k8s.io/apiserver/pkg/util/feature"
 	"k8s.io/kubernetes/pkg/features"
 	"k8s.io/kubernetes/pkg/util/slice"
+	"k8s.io/utils/ptr"
 
 	v1 "k8s.io/api/core/v1"
 	storage "k8s.io/api/storage/v1"
@@ -274,6 +275,20 @@ func checkVolumeSatisfyClaim(volume *v1.PersistentVolume, claim *v1.PersistentVo
 		return fmt.Errorf("storageClassName does not match")
 	}
 
+	if utilfeature.DefaultFeatureGate.Enabled(features.VolumeAttributesClass) {
+		requestedVAC := ptr.Deref(claim.Spec.VolumeAttributesClassName, "")
+		volumeVAC := ptr.Deref(volume.Spec.VolumeAttributesClassName, "")
+		if requestedVAC != volumeVAC {
+			return fmt.Errorf("volumeAttributesClassName does not match")
+		}
+	} else {
+		requestedVAC := ptr.Deref(claim.Spec.VolumeAttributesClassName, "")
+		volumeVAC := ptr.Deref(volume.Spec.VolumeAttributesClassName, "")
+		if requestedVAC != "" || volumeVAC != "" {
+			return fmt.Errorf("volumeAttributesClassName is not supported when the feature-gate VolumeAttributesClass is disabled")
+		}
+	}
+
 	if storagehelpers.CheckVolumeModeMismatches(&claim.Spec, &volume.Spec) {
 		return fmt.Errorf("incompatible volumeMode")
 	}
@@ -764,7 +779,7 @@ func (ctrl *PersistentVolumeController) syncVolume(ctx context.Context, volume *
 //
 //	claim - claim to update
 //	phase - phase to set
-//	volume - volume which Capacity is set into claim.Status.Capacity
+//	volume - volume which Capacity is set into claim.Status.Capacity and VolumeAttributesClassName is set into claim.Status.CurrentVolumeAttributesClassName
 func (ctrl *PersistentVolumeController) updateClaimStatus(ctx context.Context, claim *v1.PersistentVolumeClaim, phase v1.PersistentVolumeClaimPhase, volume *v1.PersistentVolume) (*v1.PersistentVolumeClaim, error) {
 	logger := klog.FromContext(ctx)
 	logger.V(4).Info("Updating PersistentVolumeClaim status", "PVC", klog.KObj(claim), "setPhase", phase)
@@ -778,7 +793,7 @@ func (ctrl *PersistentVolumeController) updateClaimStatus(ctx context.Context, c
 	}
 
 	if volume == nil {
-		// Need to reset AccessModes and Capacity
+		// Need to reset AccessModes, Capacity and CurrentVolumeAttributesClassName
 		if claim.Status.AccessModes != nil {
 			claimClone.Status.AccessModes = nil
 			dirty = true
@@ -787,8 +802,12 @@ func (ctrl *PersistentVolumeController) updateClaimStatus(ctx context.Context, c
 			claimClone.Status.Capacity = nil
 			dirty = true
 		}
+		if claim.Status.CurrentVolumeAttributesClassName != nil {
+			claimClone.Status.CurrentVolumeAttributesClassName = nil
+			dirty = true
+		}
 	} else {
-		// Need to update AccessModes and Capacity
+		// Need to update AccessModes, Capacity and CurrentVolumeAttributesClassName
 		if !reflect.DeepEqual(claim.Status.AccessModes, volume.Spec.AccessModes) {
 			claimClone.Status.AccessModes = volume.Spec.AccessModes
 			dirty = true
@@ -821,6 +840,20 @@ func (ctrl *PersistentVolumeController) updateClaimStatus(ctx context.Context, c
 				dirty = true
 			}
 		}
+
+		if utilfeature.DefaultFeatureGate.Enabled(features.VolumeAttributesClass) {
+			// There are two components to updating the current vac name, this controller and external-resizer.
+			// The controller ensures that the field is set properly when the volume is statically provisioned.
+			// It is safer for the controller to only set this field during binding, but not after. Without this
+			// constraint, there is a potential race condition where the resizer sets the field after the controller
+			// has set it, or vice versa. Afterwards, it should be handled only by the resizer, or if an admin wants
+			// to override.
+			if claim.Status.Phase == v1.ClaimPending && phase == v1.ClaimBound &&
+				!reflect.DeepEqual(claim.Status.CurrentVolumeAttributesClassName, volume.Spec.VolumeAttributesClassName) {
+				claimClone.Status.CurrentVolumeAttributesClassName = volume.Spec.VolumeAttributesClassName
+				dirty = true
+			}
+		}
 	}
 
 	if !dirty {
@@ -850,7 +883,7 @@ func (ctrl *PersistentVolumeController) updateClaimStatus(ctx context.Context, c
 //
 //	claim - claim to update
 //	phase - phase to set
-//	volume - volume which Capacity is set into claim.Status.Capacity
+//	volume - volume which Capacity is set into claim.Status.Capacity and VolumeAttributesClassName is set into claim.Status.CurrentVolumeAttributesClassName
 //	eventtype, reason, message - event to send, see EventRecorder.Event()
 func (ctrl *PersistentVolumeController) updateClaimStatusWithEvent(ctx context.Context, claim *v1.PersistentVolumeClaim, phase v1.PersistentVolumeClaimPhase, volume *v1.PersistentVolume, eventtype, reason, message string) (*v1.PersistentVolumeClaim, error) {
 	logger := klog.FromContext(ctx)