github/kubernetes
1
0
Fork 0
mirror of https://github.com/k3s-io/kubernetes.git synced 2025-07-23 11:50:44 +00:00
Code Issues Projects Releases Wiki Activity

PV controller changes to support PV Deletion protection finalizer

Browse Source 
Signed-off-by: Deepak Kinni <dkinni@vmware.com>
This commit is contained in:
Deepak Kinni 2021-10-19 14:26:28 -07:00
parent bafa87c553
commit bfd5f23a0b
6 changed files with 222 additions and 18 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

9
pkg/controller/volume/persistentvolume/framework_test.go
View File

@ -370,6 +370,15 @@ func withVolumeDeletionTimestamp(pvs []*v1.PersistentVolume) []*v1.PersistentVol
return result
}
func volumesWithFinalizers(pvs []*v1.PersistentVolume, finalizers []string) []*v1.PersistentVolume {
result := []*v1.PersistentVolume{}
for _, pv := range pvs {
pv.SetFinalizers(finalizers)
result = append(result, pv)
}
return result
}
// newClaim returns a new claim with given attributes
func newClaim(name, claimUID, capacity, boundToVolume string, phase v1.PersistentVolumeClaimPhase, class *string, annotations ...string) *v1.PersistentVolumeClaim {
fs := v1.PersistentVolumeFilesystem

5
pkg/controller/volume/persistentvolume/pv_controller.go
View File

@ -537,10 +537,9 @@ func (ctrl *PersistentVolumeController) syncBoundClaim(claim *v1.PersistentVolum
// these events.
func (ctrl *PersistentVolumeController) syncVolume(ctx context.Context, volume *v1.PersistentVolume) error {
klog.V(4).Infof("synchronizing PersistentVolume[%s]: %s", volume.Name, getVolumeStatusForLogging(volume))
// Set correct "migrated-to" annotations on PV and update in API server if
// Set correct "migrated-to" annotations and modify finalizers on PV and update in API server if
// necessary
newVolume, err := ctrl.updateVolumeMigrationAnnotations(ctx, volume)
newVolume, err := ctrl.updateVolumeMigrationAnnotationsAndFinalizers(ctx, volume)
if err != nil {
// Nothing was saved; we will fall back into the same
// condition in the next call to this method

38
pkg/controller/volume/persistentvolume/pv_controller_base.go
View File

@ -19,6 +19,8 @@ package persistentvolume
import (
"context"
"fmt"
"k8s.io/kubernetes/pkg/features"
"k8s.io/kubernetes/pkg/util/slice"
"strconv"
"time"
@ -323,7 +325,8 @@ func (ctrl *PersistentVolumeController) Run(ctx context.Context) {
<-ctx.Done()
}
func (ctrl *PersistentVolumeController) updateClaimMigrationAnnotations(ctx context.Context, claim *v1.PersistentVolumeClaim) (*v1.PersistentVolumeClaim, error) {
func (ctrl *PersistentVolumeController) updateClaimMigrationAnnotations(ctx context.Context,
claim *v1.PersistentVolumeClaim) (*v1.PersistentVolumeClaim, error) {
// TODO: update[Claim|Volume]MigrationAnnotations can be optimized to not
// copy the claim/volume if no modifications are required. Though this
// requires some refactoring as well as an interesting change in the
@ -331,7 +334,7 @@ func (ctrl *PersistentVolumeController) updateClaimMigrationAnnotations(ctx cont
// when no modifications are required this function could sometimes return a
// copy of the volume and sometimes return a ref to the original
claimClone := claim.DeepCopy()
modified := updateMigrationAnnotations(ctrl.csiMigratedPluginManager, ctrl.translator, claimClone.Annotations, true)
modified := updateMigrationAnnotationsAndFinalizers(ctrl.csiMigratedPluginManager, ctrl.translator, claimClone.Annotations, nil, true)
if !modified {
return claimClone, nil
}
@ -346,33 +349,36 @@ func (ctrl *PersistentVolumeController) updateClaimMigrationAnnotations(ctx cont
return newClaim, nil
}
func (ctrl *PersistentVolumeController) updateVolumeMigrationAnnotations(ctx context.Context, volume *v1.PersistentVolume) (*v1.PersistentVolume, error) {
func (ctrl *PersistentVolumeController) updateVolumeMigrationAnnotationsAndFinalizers(ctx context.Context,
volume *v1.PersistentVolume) (*v1.PersistentVolume, error) {
volumeClone := volume.DeepCopy()
modified := updateMigrationAnnotations(ctrl.csiMigratedPluginManager, ctrl.translator, volumeClone.Annotations, false)
modified := updateMigrationAnnotationsAndFinalizers(ctrl.csiMigratedPluginManager, ctrl.translator,
volumeClone.Annotations, &volumeClone.Finalizers, false)
if !modified {
return volumeClone, nil
}
newVol, err := ctrl.kubeClient.CoreV1().PersistentVolumes().Update(ctx, volumeClone, metav1.UpdateOptions{})
if err != nil {
return nil, fmt.Errorf("persistent Volume Controller can't anneal migration annotations: %v", err)
return nil, fmt.Errorf("persistent Volume Controller can't anneal migration annotations or finalizer: %v", err)
}
_, err = ctrl.storeVolumeUpdate(newVol)
if err != nil {
return nil, fmt.Errorf("persistent Volume Controller can't anneal migration annotations: %v", err)
return nil, fmt.Errorf("persistent Volume Controller can't anneal migration annotations or finalizer: %v", err)
}
return newVol, nil
}
// updateMigrationAnnotations takes an Annotations map and checks for a
// updateMigrationAnnotationsAndFinalizers takes an Annotations map and finalizers slice and checks for a
// provisioner name depending if the annotation came from a PVC or not.
// It will then add a "pv.kubernetes.io/migrated-to" annotation if migration with
// the CSI driver name for that provisioner is "on" based on feature flags, it will also
// remove the annotation if migration is "off" for that provisioner in rollback
// scenarios. Returns true if the annotations map was modified and false otherwise.
// remove the PV deletion protection finalizer and "pv.kubernetes.io/migrated-to" annotation
// if migration is "off" for that provisioner in rollback scenarios. Returns true if the annotations
// map or finalizers were modified and false otherwise.
// Parameters:
// - claim: true means the ann came from a PVC, false means the ann came from a PV
func updateMigrationAnnotations(cmpm CSIMigratedPluginManager, translator CSINameTranslator, ann map[string]string, claim bool) bool {
func updateMigrationAnnotationsAndFinalizers(cmpm CSIMigratedPluginManager, translator CSINameTranslator,
ann map[string]string, finalizers *[]string, claim bool) bool {
var csiDriverName string
var err error
@ -416,6 +422,12 @@ func updateMigrationAnnotations(cmpm CSIMigratedPluginManager, translator CSINam
if migratedToDriver != "" {
// Migration annotation exists but the driver isn't migrated currently
delete(ann, pvutil.AnnMigratedTo)
// Remove the PV deletion protection finalizer on volumes.
if !claim && finalizers != nil && utilfeature.DefaultFeatureGate.Enabled(features.HonorPVReclaimPolicy) {
if slice.ContainsString(*finalizers, pvutil.PVDeletionProtectionFinalizer, nil) {
*finalizers = slice.RemoveString(*finalizers, pvutil.PVDeletionProtectionFinalizer, nil)
}
}
return true
}
}
@ -576,8 +588,8 @@ func (ctrl *PersistentVolumeController) setClaimProvisioner(ctx context.Context,
// TODO: remove the beta storage provisioner anno after the deprecation period
metav1.SetMetaDataAnnotation(&claimClone.ObjectMeta, pvutil.AnnBetaStorageProvisioner, provisionerName)
metav1.SetMetaDataAnnotation(&claimClone.ObjectMeta, pvutil.AnnStorageProvisioner, provisionerName)
updateMigrationAnnotations(ctrl.csiMigratedPluginManager, ctrl.translator, claimClone.Annotations, true)
newClaim, err := ctrl.kubeClient.CoreV1().PersistentVolumeClaims(claim.Namespace).Update(ctx, claimClone, metav1.UpdateOptions{})
updateMigrationAnnotationsAndFinalizers(ctrl.csiMigratedPluginManager, ctrl.translator, claimClone.Annotations, nil, true)
newClaim, err := ctrl.kubeClient.CoreV1().PersistentVolumeClaims(claim.Namespace).Update(context.TODO(), claimClone, metav1.UpdateOptions{})
if err != nil {
return newClaim, err
}

176
pkg/controller/volume/persistentvolume/pv_controller_test.go
View File

@ -59,6 +59,7 @@ var (
// can't reliably simulate periodic sync of volumes/claims - it would be
// either very timing-sensitive or slow to wait for real periodic sync.
func TestControllerSync(t *testing.T) {
defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.HonorPVReclaimPolicy, true)()
tests := []controllerTest{
// [Unit test set 5] - controller tests.
// We test the controller as if
@ -295,6 +296,23 @@ func TestControllerSync(t *testing.T) {
return nil
},
},
{
// Test that the finalizer gets removed if CSI migration is disabled.
"5-9 - volume has its PV deletion protection finalizer removed as CSI migration is disabled",
volumesWithFinalizers(
volumesWithAnnotation(pvutil.AnnMigratedTo, "pd.csi.storage.gke.io",
newVolumeArray("volume-5-9", "1Gi", "", "", v1.VolumeAvailable, v1.PersistentVolumeReclaimDelete, classEmpty, pvutil.AnnDynamicallyProvisioned)),
[]string{pvutil.PVDeletionProtectionFinalizer},
),
newVolumeArray("volume-5-9", "1Gi", "", "", v1.VolumeAvailable, v1.PersistentVolumeReclaimDelete, classEmpty, pvutil.AnnDynamicallyProvisioned),
noclaims,
noclaims,
noevents,
noerrors,
func(ctrl *PersistentVolumeController, reactor *pvtesting.VolumeReactor, test controllerTest) error {
return nil
},
},
}
doit := func(test controllerTest) {
@ -642,14 +660,14 @@ func TestAnnealMigrationAnnotations(t *testing.T) {
}
if tc.volumeAnnotations != nil {
ann := tc.volumeAnnotations
updateMigrationAnnotations(cmpm, translator, ann, false)
updateMigrationAnnotationsAndFinalizers(cmpm, translator, ann, nil, false)
if !reflect.DeepEqual(tc.expVolumeAnnotations, ann) {
t.Errorf("got volume annoations: %v, but expected: %v", ann, tc.expVolumeAnnotations)
}
}
if tc.claimAnnotations != nil {
ann := tc.claimAnnotations
updateMigrationAnnotations(cmpm, translator, ann, true)
updateMigrationAnnotationsAndFinalizers(cmpm, translator, ann, nil, true)
if !reflect.DeepEqual(tc.expClaimAnnotations, ann) {
t.Errorf("got volume annoations: %v, but expected: %v", ann, tc.expVolumeAnnotations)
}
@ -658,3 +676,157 @@ func TestAnnealMigrationAnnotations(t *testing.T) {
})
}
}
func TestUpdateFinalizer(t *testing.T) {
defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.HonorPVReclaimPolicy, true)()
const gcePlugin = "kubernetes.io/gce-pd"
const gceDriver = "pd.csi.storage.gke.io"
const customFinalizer = "test.volume.kubernetes.io/finalizer"
tests := []struct {
name string
volumeAnnotations map[string]string
volumeFinalizers []string
expVolumeFinalizers []string
expModified bool
migratedDriverGates []featuregate.Feature
}{
{
// Represents a volume provisioned through external-provisioner
name: "13-1 migration was never enabled, volume has the finalizer",
volumeAnnotations: map[string]string{pvutil.AnnDynamicallyProvisioned: gceDriver},
volumeFinalizers: []string{pvutil.PVDeletionProtectionFinalizer},
expVolumeFinalizers: []string{pvutil.PVDeletionProtectionFinalizer},
expModified: false,
migratedDriverGates: []featuregate.Feature{},
},
{
// Represents a volume provisioned through external-provisioner but the external-provisioner has
// yet to sync the volume to add the new finalizer
name: "13-2 migration was never enabled, volume does not have the finalizer",
volumeAnnotations: map[string]string{pvutil.AnnDynamicallyProvisioned: gceDriver},
volumeFinalizers: nil,
expVolumeFinalizers: nil,
expModified: false,
migratedDriverGates: []featuregate.Feature{},
},
{
// Represents an in-tree volume that has the migrated-to annotation but the external-provisioner is
// yet to sync the volume and add the pv deletion protection finalizer. The custom finalizer is some
// pre-existing finalizer, for example the pv-protection finalizer. The csi-migration is disabled,
// the migrated-to annotation will be removed shortly when updateVolumeMigrationAnnotationsAndFinalizers is called.
name: "13-3 migration was disabled but still has migrated-to annotation, volume does not have pv deletion protection finalizer",
volumeAnnotations: map[string]string{pvutil.AnnDynamicallyProvisioned: gcePlugin, pvutil.AnnMigratedTo: gceDriver},
volumeFinalizers: []string{customFinalizer},
expVolumeFinalizers: []string{customFinalizer},
expModified: true,
migratedDriverGates: []featuregate.Feature{},
},
{
name: "13-4 migration was disabled but still has migrated-to annotation, volume has no finalizers",
volumeAnnotations: map[string]string{pvutil.AnnDynamicallyProvisioned: gcePlugin, pvutil.AnnMigratedTo: gceDriver},
volumeFinalizers: nil,
expVolumeFinalizers: nil,
expModified: true,
migratedDriverGates: []featuregate.Feature{},
},
{
// Represents roll back scenario where the external-provisioner has added the pv deletion protection
// finalizer and later the csi migration was disabled. The pv deletion protection finalizer added through
// external-provisioner will be removed.
name: "13-5 migration was disabled as it has the migrated-to annotation, volume has the finalizer",
volumeAnnotations: map[string]string{pvutil.AnnDynamicallyProvisioned: gcePlugin, pvutil.AnnMigratedTo: gceDriver},
volumeFinalizers: []string{pvutil.PVDeletionProtectionFinalizer},
expVolumeFinalizers: nil,
expModified: true,
migratedDriverGates: []featuregate.Feature{},
},
{
// Represents roll-back of csi-migration as 13-5, here there are multiple finalizers, only the pv deletion
// protection finalizer added by external-provisioner needs to be removed.
name: "13-6 migration was disabled as it has the migrated-to annotation, volume has multiple finalizers",
volumeAnnotations: map[string]string{pvutil.AnnDynamicallyProvisioned: gcePlugin, pvutil.AnnMigratedTo: gceDriver},
volumeFinalizers: []string{pvutil.PVDeletionProtectionFinalizer, customFinalizer},
expVolumeFinalizers: []string{customFinalizer},
expModified: true,
migratedDriverGates: []featuregate.Feature{},
},
{
// csi migration is enabled, the pv controller should not delete the finalizer added by the
// external-provisioner.
name: "13-7 migration is enabled, has the migrated-to annotation, volume has the finalizer",
volumeAnnotations: map[string]string{pvutil.AnnDynamicallyProvisioned: gcePlugin, pvutil.AnnMigratedTo: gceDriver},
volumeFinalizers: []string{pvutil.PVDeletionProtectionFinalizer},
expVolumeFinalizers: []string{pvutil.PVDeletionProtectionFinalizer},
expModified: false,
migratedDriverGates: []featuregate.Feature{features.CSIMigration, features.CSIMigrationGCE},
},
{
// csi-migration is not completely enabled as the specific plugin feature is not present. This is equivalent
// of disabled csi-migration.
name: "13-8 migration is enabled but plugin migration feature is disabled, has the migrated-to annotation, volume has the finalizer",
volumeAnnotations: map[string]string{pvutil.AnnDynamicallyProvisioned: gcePlugin, pvutil.AnnMigratedTo: gceDriver},
volumeFinalizers: []string{pvutil.PVDeletionProtectionFinalizer},
expVolumeFinalizers: nil,
expModified: true,
migratedDriverGates: []featuregate.Feature{features.CSIMigration},
},
{
// same as 13-8 but multiple finalizers exists, only the pv deletion protection finalizer needs to be removed.
name: "13-9 migration is enabled but plugin migration feature is disabled, has the migrated-to annotation, volume has multiple finalizers",
volumeAnnotations: map[string]string{pvutil.AnnDynamicallyProvisioned: gcePlugin, pvutil.AnnMigratedTo: gceDriver},
volumeFinalizers: []string{pvutil.PVDeletionProtectionFinalizer, customFinalizer},
expVolumeFinalizers: []string{customFinalizer},
expModified: true,
migratedDriverGates: []featuregate.Feature{features.CSIMigration},
},
{
// corner error case.
name: "13-10 missing annotations but finalizers exist",
volumeAnnotations: nil,
volumeFinalizers: []string{pvutil.PVDeletionProtectionFinalizer},
expVolumeFinalizers: []string{pvutil.PVDeletionProtectionFinalizer},
expModified: false,
migratedDriverGates: []featuregate.Feature{},
},
{
name: "13-11 missing annotations and finalizers",
volumeAnnotations: nil,
volumeFinalizers: nil,
expVolumeFinalizers: nil,
expModified: false,
migratedDriverGates: []featuregate.Feature{},
},
{
// corner error case
name: "13-12 missing provisioned-by annotation, existing finalizers",
volumeAnnotations: map[string]string{"fake": gcePlugin},
volumeFinalizers: []string{pvutil.PVDeletionProtectionFinalizer},
expVolumeFinalizers: []string{pvutil.PVDeletionProtectionFinalizer},
expModified: false,
migratedDriverGates: []featuregate.Feature{},
},
}
translator := csitrans.New()
cmpm := csimigration.NewPluginManager(translator, utilfeature.DefaultFeatureGate)
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
for _, f := range tc.migratedDriverGates {
defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, f, true)()
}
if tc.volumeAnnotations != nil {
ann := tc.volumeAnnotations
finalizers := tc.volumeFinalizers
modified := updateMigrationAnnotationsAndFinalizers(cmpm, translator, ann, &finalizers, false)
if modified != tc.expModified {
t.Errorf("got modified: %v, but expected: %v", modified, tc.expModified)
}
if !reflect.DeepEqual(tc.expVolumeFinalizers, finalizers) {
t.Errorf("got volume finaliers: %v, but expected: %v", finalizers, tc.expVolumeFinalizers)
}
}
})
}
}

3
pkg/controller/volume/persistentvolume/util/util.go
View File

@ -75,6 +75,9 @@ const (
// TODO: remove beta anno once deprecation period ends
AnnStorageProvisioner = "volume.kubernetes.io/storage-provisioner"
AnnBetaStorageProvisioner = "volume.beta.kubernetes.io/storage-provisioner"
//PVDeletionProtectionFinalizer is the finalizer added by the external-provisioner on the PV
PVDeletionProtectionFinalizer = "external-provisioner.volume.kubernetes.io/finalizer"
)
// IsDelayBindingProvisioning checks if claim provisioning with selected-node annotation

9
pkg/features/kube_features.go
View File

@ -792,6 +792,14 @@ const (
// Configures the Kubelet to use the CRI to populate pod and container stats, instead of supplimenting with stats from cAdvisor.
// Requires the CRI implementation supports supplying the required stats.
PodAndContainerStatsFromCRI featuregate.Feature = "PodAndContainerStatsFromCRI"
// owner: @deepakkinni @xing-yang
// kep: http://kep.k8s.io/2680
// alpha: v1.23
//
// Honor Persistent Volume Reclaim Policy when it is "Delete" irrespective of PV-PVC
// deletion ordering.
HonorPVReclaimPolicy featuregate.Feature = "HonorPVReclaimPolicy"
)
func init() {
@ -908,6 +916,7 @@ var defaultKubernetesFeatureGates = map[featuregate.Feature]featuregate.FeatureS
JobMutableNodeSchedulingDirectives: {Default: true, PreRelease: featuregate.Beta},
IdentifyPodOS: {Default: false, PreRelease: featuregate.Alpha},
PodAndContainerStatsFromCRI: {Default: false, PreRelease: featuregate.Alpha},
HonorPVReclaimPolicy: {Default: false, PreRelease: featuregate.Alpha},
// inherited features from generic apiserver, relisted here to get a conflict if it is changed
// unintentionally on either side: