From 10d2a1adc85ec4cff097014eb4be77a81b94ff20 Mon Sep 17 00:00:00 2001 From: Xing Yang Date: Wed, 17 Oct 2018 10:32:40 -0700 Subject: [PATCH] Add Delete Volume Finalizer This PR adds Finalizer on the snapshot source PVC to prevent it from being deleted when a snapshot is being created from it. --- pkg/controller/framework_test.go | 170 +++++++++++++++++++++- pkg/controller/snapshot_controller.go | 126 +++++++++++++++- pkg/controller/snapshot_finalizer_test.go | 44 ++++++ pkg/controller/util.go | 3 + 4 files changed, 339 insertions(+), 4 deletions(-) create mode 100644 pkg/controller/snapshot_finalizer_test.go diff --git a/pkg/controller/framework_test.go b/pkg/controller/framework_test.go index a9ad4832e..9c2e94c2e 100644 --- a/pkg/controller/framework_test.go +++ b/pkg/controller/framework_test.go @@ -21,6 +21,7 @@ import ( "errors" "fmt" "reflect" + sysruntime "runtime" "strconv" "strings" "sync" @@ -53,6 +54,7 @@ import ( "k8s.io/client-go/tools/cache" "k8s.io/client-go/tools/record" "k8s.io/klog" + "k8s.io/kubernetes/pkg/util/slice" ) // This is a unit test framework for snapshot controller. @@ -177,6 +179,11 @@ func withContentFinalizer(content *crdv1.VolumeSnapshotContent) *crdv1.VolumeSna return content } +func withPVCFinalizer(pvc *v1.PersistentVolumeClaim) *v1.PersistentVolumeClaim { + pvc.ObjectMeta.Finalizers = append(pvc.ObjectMeta.Finalizers, PVCFinalizer) + return pvc +} + // React is a callback called by fake kubeClient from the controller. // In other words, every snapshot/content change performed by the controller ends // here. @@ -331,6 +338,32 @@ func (r *snapshotReactor) React(action core.Action) (handled bool, ret runtime.O klog.V(4).Infof("GetClaim: claim %s not found", name) return true, nil, fmt.Errorf("cannot find claim %s", name) + case action.Matches("update", "persistentvolumeclaims"): + obj := action.(core.UpdateAction).GetObject() + claim := obj.(*v1.PersistentVolumeClaim) + + // Check and bump object version + storedClaim, found := r.claims[claim.Name] + if found { + storedVer, _ := strconv.Atoi(storedClaim.ResourceVersion) + requestedVer, _ := strconv.Atoi(claim.ResourceVersion) + if storedVer != requestedVer { + return true, obj, errVersionConflict + } + // Don't modify the existing object + claim = claim.DeepCopy() + claim.ResourceVersion = strconv.Itoa(storedVer + 1) + } else { + return true, nil, fmt.Errorf("cannot update claim %s: claim not found", claim.Name) + } + + // Store the updated object to appropriate places. + r.claims[claim.Name] = claim + r.changedObjects = append(r.changedObjects, claim) + r.changedSinceLastSync++ + klog.V(4).Infof("saved updated claim %s", claim.Name) + return true, claim, nil + case action.Matches("get", "storageclasses"): name := action.(core.GetAction).GetName() storageClass, found := r.storageClasses[name] @@ -550,6 +583,9 @@ func (r *snapshotReactor) syncAll() { for _, v := range r.contents { r.changedObjects = append(r.changedObjects, v) } + for _, pvc := range r.claims { + r.changedObjects = append(r.changedObjects, pvc) + } r.changedSinceLastSync = 0 } @@ -699,6 +735,7 @@ func newSnapshotReactor(kubeClient *kubefake.Clientset, client *fake.Clientset, client.AddReactor("delete", "volumesnapshotcontents", reactor.React) client.AddReactor("delete", "volumesnapshots", reactor.React) kubeClient.AddReactor("get", "persistentvolumeclaims", reactor.React) + kubeClient.AddReactor("update", "persistentvolumeclaims", reactor.React) kubeClient.AddReactor("get", "persistentvolumes", reactor.React) kubeClient.AddReactor("get", "storageclasses", reactor.React) kubeClient.AddReactor("get", "secrets", reactor.React) @@ -746,6 +783,7 @@ func newTestController(kubeClient kubernetes.Interface, clientset clientset.Inte ctrl.contentListerSynced = alwaysReady ctrl.snapshotListerSynced = alwaysReady ctrl.classListerSynced = alwaysReady + ctrl.pvcListerSynced = alwaysReady return ctrl, nil } @@ -845,7 +883,7 @@ func newSnapshotArray(name, className, boundToContent, snapshotUID, claimName st } // newClaim returns a new claim with given attributes -func newClaim(name, claimUID, capacity, boundToVolume string, phase v1.PersistentVolumeClaimPhase, class *string) *v1.PersistentVolumeClaim { +func newClaim(name, claimUID, capacity, boundToVolume string, phase v1.PersistentVolumeClaimPhase, class *string, bFinalizer bool) *v1.PersistentVolumeClaim { claim := v1.PersistentVolumeClaim{ ObjectMeta: metav1.ObjectMeta{ Name: name, @@ -877,6 +915,9 @@ func newClaim(name, claimUID, capacity, boundToVolume string, phase v1.Persisten claim.Status.Capacity = claim.Spec.Resources.Requests } + if bFinalizer { + return withPVCFinalizer(&claim) + } return &claim } @@ -884,7 +925,15 @@ func newClaim(name, claimUID, capacity, boundToVolume string, phase v1.Persisten // newClaim() with the same parameters. func newClaimArray(name, claimUID, capacity, boundToVolume string, phase v1.PersistentVolumeClaimPhase, class *string) []*v1.PersistentVolumeClaim { return []*v1.PersistentVolumeClaim{ - newClaim(name, claimUID, capacity, boundToVolume, phase, class), + newClaim(name, claimUID, capacity, boundToVolume, phase, class, false), + } +} + +// newClaimArrayFinalizer returns array with a single claim that would be returned by +// newClaim() with the same parameters plus finalizer. +func newClaimArrayFinalizer(name, claimUID, capacity, boundToVolume string, phase v1.PersistentVolumeClaimPhase, class *string) []*v1.PersistentVolumeClaim { + return []*v1.PersistentVolumeClaim{ + newClaim(name, claimUID, capacity, boundToVolume, phase, class, true), } } @@ -961,6 +1010,14 @@ func testSyncContent(ctrl *csiSnapshotController, reactor *snapshotReactor, test return ctrl.syncContent(test.initialContents[0]) } +func testAddPVCFinalizer(ctrl *csiSnapshotController, reactor *snapshotReactor, test controllerTest) error { + return ctrl.ensureSnapshotSourceFinalizer(test.initialSnapshots[0]) +} + +func testRemovePVCFinalizer(ctrl *csiSnapshotController, reactor *snapshotReactor, test controllerTest) error { + return ctrl.checkandRemoveSnapshotSourceFinalizer(test.initialSnapshots[0]) +} + var ( classEmpty string classGold = "gold" @@ -1097,6 +1154,115 @@ func runSyncTests(t *testing.T, tests []controllerTest, snapshotClasses []*crdv1 } } +// This tests ensureSnapshotSourceFinalizer and checkandRemoveSnapshotSourceFinalizer +func runPVCFinalizerTests(t *testing.T, tests []controllerTest, snapshotClasses []*crdv1.VolumeSnapshotClass) { + snapshotscheme.AddToScheme(scheme.Scheme) + for _, test := range tests { + klog.V(4).Infof("starting test %q", test.name) + + // Initialize the controller + kubeClient := &kubefake.Clientset{} + client := &fake.Clientset{} + + ctrl, err := newTestController(kubeClient, client, nil, t, test) + if err != nil { + t.Fatalf("Test %q construct persistent content failed: %v", test.name, err) + } + + reactor := newSnapshotReactor(kubeClient, client, ctrl, nil, nil, test.errors) + for _, snapshot := range test.initialSnapshots { + ctrl.snapshotStore.Add(snapshot) + reactor.snapshots[snapshot.Name] = snapshot + } + for _, content := range test.initialContents { + if ctrl.isDriverMatch(test.initialContents[0]) { + ctrl.contentStore.Add(content) + reactor.contents[content.Name] = content + } + } + + pvcIndexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{}) + for _, claim := range test.initialClaims { + reactor.claims[claim.Name] = claim + pvcIndexer.Add(claim) + } + ctrl.pvcLister = corelisters.NewPersistentVolumeClaimLister(pvcIndexer) + + for _, volume := range test.initialVolumes { + reactor.volumes[volume.Name] = volume + } + for _, storageClass := range test.initialStorageClasses { + reactor.storageClasses[storageClass.Name] = storageClass + } + for _, secret := range test.initialSecrets { + reactor.secrets[secret.Name] = secret + } + + // Inject classes into controller via a custom lister. + indexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{}) + for _, class := range snapshotClasses { + indexer.Add(class) + } + ctrl.classLister = storagelisters.NewVolumeSnapshotClassLister(indexer) + + // Run the tested functions + err = test.test(ctrl, reactor, test) + if err != nil { + t.Errorf("Test %q failed: %v", test.name, err) + } + + // Verify PVCFinalizer tests results + evaluatePVCFinalizerTests(ctrl, reactor, test, t) + } +} + +// Evaluate PVCFinalizer tests results +func evaluatePVCFinalizerTests(ctrl *csiSnapshotController, reactor *snapshotReactor, test controllerTest, t *testing.T) { + // Evaluate results + bHasPVCFinalizer := false + name := sysruntime.FuncForPC(reflect.ValueOf(test.test).Pointer()).Name() + index := strings.LastIndex(name, ".") + if index == -1 { + t.Errorf("Test %q: failed to test finalizer - invalid test call name [%s]", test.name, name) + return + } + names := []rune(name) + funcName := string(names[index+1 : len(name)]) + klog.V(4).Infof("test %q: PVCFinalizer test func name: [%s]", test.name, funcName) + + if funcName == "testAddPVCFinalizer" { + for _, pvc := range reactor.claims { + if test.initialClaims[0].Name == pvc.Name { + if !slice.ContainsString(test.initialClaims[0].ObjectMeta.Finalizers, PVCFinalizer, nil) && slice.ContainsString(pvc.ObjectMeta.Finalizers, PVCFinalizer, nil) { + klog.V(4).Infof("test %q succeeded. PVCFinalizer is added to PVC %s", test.name, pvc.Name) + fmt.Printf("\ntest %q succeeded. PVCFinalizer is added to PVC %s\n", test.name, pvc.Name) + bHasPVCFinalizer = true + } + break + } + } + if !bHasPVCFinalizer { + t.Errorf("Test %q: failed to add finalizer to PVC %s", test.name, test.initialClaims[0].Name) + } + } + bHasPVCFinalizer = true + if funcName == "testRemovePVCFinalizer" { + for _, pvc := range reactor.claims { + if test.initialClaims[0].Name == pvc.Name { + if slice.ContainsString(test.initialClaims[0].ObjectMeta.Finalizers, PVCFinalizer, nil) && !slice.ContainsString(pvc.ObjectMeta.Finalizers, PVCFinalizer, nil) { + klog.V(4).Infof("test %q succeeded. PVCFinalizer is removed from PVC %s", test.name, pvc.Name) + fmt.Printf("\ntest %q succeeded. PVCFinalizer is removed from PVC %s\n", test.name, pvc.Name) + bHasPVCFinalizer = false + } + break + } + } + if bHasPVCFinalizer { + t.Errorf("Test %q: failed to remove finalizer from PVC %s", test.name, test.initialClaims[0].Name) + } + } +} + func getSize(size int64) *resource.Quantity { return resource.NewQuantity(size, resource.BinarySI) } diff --git a/pkg/controller/snapshot_controller.go b/pkg/controller/snapshot_controller.go index 3d30844af..5528623a0 100644 --- a/pkg/controller/snapshot_controller.go +++ b/pkg/controller/snapshot_controller.go @@ -192,11 +192,19 @@ func (ctrl *csiSnapshotController) syncSnapshot(snapshot *crdv1.VolumeSnapshot) return ctrl.addSnapshotFinalizer(snapshot) } + klog.V(5).Infof("syncSnapshot[%s]: check if we should remove finalizer on snapshot source and remove it if we can", snapshotKey(snapshot)) + // Check if we should remove finalizer on snapshot source and remove it if we can. + errFinalizer := ctrl.checkandRemoveSnapshotSourceFinalizer(snapshot) + if errFinalizer != nil { + klog.Errorf("error check and remove snapshot source finalizer for snapshot [%s]: %v", snapshot.Name, errFinalizer) + // Log an event and keep the original error from syncUnready/ReadySnapshot + ctrl.eventRecorder.Event(snapshot, v1.EventTypeWarning, "ErrorSnapshotSourceFinalizer", "Error check and remove PVC Finalizer for VolumeSnapshot") + } + if !snapshot.Status.ReadyToUse { return ctrl.syncUnreadySnapshot(snapshot) } return ctrl.syncReadySnapshot(snapshot) - } // syncReadySnapshot checks the snapshot which has been bound to snapshot content successfully before. @@ -367,7 +375,6 @@ func (ctrl *csiSnapshotController) createSnapshot(snapshot *crdv1.VolumeSnapshot // We will get an "snapshot update" event soon, this is not a big error klog.V(4).Infof("createSnapshot [%s]: cannot update internal cache: %v", snapshotKey(snapshotObj), updateErr) } - return nil }) return nil @@ -612,6 +619,14 @@ func (ctrl *csiSnapshotController) createSnapshotOperation(snapshot *crdv1.Volum return snapshot, nil } + // If PVC is not being deleted and finalizer is not added yet, a finalizer should be added. + klog.V(5).Infof("createSnapshotOperation: Check if PVC is not being deleted and add Finalizer for source of snapshot [%s] if needed", snapshot.Name) + err := ctrl.ensureSnapshotSourceFinalizer(snapshot) + if err != nil { + klog.Errorf("createSnapshotOperation failed to add finalizer for source of snapshot %s", err) + return nil, err + } + class, volume, contentName, snapshotterCredentials, err := ctrl.getCreateSnapshotInput(snapshot) if err != nil { return nil, fmt.Errorf("failed to get input parameters to create snapshot %s: %q", snapshot.Name, err) @@ -1068,3 +1083,110 @@ func (ctrl *csiSnapshotController) removeSnapshotFinalizer(snapshot *crdv1.Volum klog.V(5).Infof("Removed protection finalizer from volume snapshot %s", snapshotKey(snapshot)) return nil } + +// ensureSnapshotSourceFinalizer checks if a Finalizer needs to be added for the snapshot source; +// if true, adds a Finalizer for VolumeSnapshot Source PVC +func (ctrl *csiSnapshotController) ensureSnapshotSourceFinalizer(snapshot *crdv1.VolumeSnapshot) error { + // Get snapshot source which is a PVC + pvc, err := ctrl.getClaimFromVolumeSnapshot(snapshot) + if err != nil { + return nil + } + + // If PVC is not being deleted and PVCFinalizer is not added yet, the PVCFinalizer should be added. + if pvc.ObjectMeta.DeletionTimestamp == nil && !slice.ContainsString(pvc.ObjectMeta.Finalizers, PVCFinalizer, nil) { + // Add the finalizer + pvcClone := pvc.DeepCopy() + pvcClone.ObjectMeta.Finalizers = append(pvcClone.ObjectMeta.Finalizers, PVCFinalizer) + _, err = ctrl.client.CoreV1().PersistentVolumeClaims(pvcClone.Namespace).Update(pvcClone) + if err != nil { + return newControllerUpdateError(pvcClone.Name, err.Error()) + } + klog.V(5).Infof("Added protection finalizer to persistent volume claim %s", pvc.Name) + } + + return nil +} + +// removeSnapshotSourceFinalizer removes a Finalizer for VolumeSnapshot Source PVC. +func (ctrl *csiSnapshotController) removeSnapshotSourceFinalizer(snapshot *crdv1.VolumeSnapshot) error { + // Get snapshot source which is a PVC + pvc, err := ctrl.getClaimFromVolumeSnapshot(snapshot) + if err != nil { + return err + } + + pvcClone := pvc.DeepCopy() + pvcClone.ObjectMeta.Finalizers = slice.RemoveString(pvcClone.ObjectMeta.Finalizers, PVCFinalizer, nil) + + _, err = ctrl.client.CoreV1().PersistentVolumeClaims(pvcClone.Namespace).Update(pvcClone) + if err != nil { + return newControllerUpdateError(pvcClone.Name, err.Error()) + } + + klog.V(5).Infof("Removed protection finalizer from persistent volume claim %s", pvc.Name) + return nil +} + +// isSnapshotSourceBeingUsed checks if a PVC is being used as a source to create a snapshot +func (ctrl *csiSnapshotController) isSnapshotSourceBeingUsed(snapshot *crdv1.VolumeSnapshot) bool { + klog.V(5).Infof("isSnapshotSourceBeingUsed[%s]: started", snapshotKey(snapshot)) + // Get snapshot source which is a PVC + pvc, err := ctrl.getClaimFromVolumeSnapshot(snapshot) + if err != nil { + klog.Errorf("isSnapshotSourceBeingUsed: failed to get claim from snapshot: %v", err) + return false + } + + // Going through snapshots in the cache (snapshotLister). If a snapshot's PVC source + // is the same as the input snapshot's PVC source and snapshot's ReadyToUse status + // is false, the snapshot is still being created from the PVC and the PVC is in-use. + snapshots, err := ctrl.snapshotLister.VolumeSnapshots(snapshot.Namespace).List(labels.Everything()) + if err != nil { + return false + } + for _, snap := range snapshots { + // Skip static bound snapshot without a PVC source + if snap.Spec.Source == nil { + klog.V(4).Infof("Skipping static bound snapshot %s when checking PVC %s/%s", snap.Name, pvc.Namespace, pvc.Name) + continue + } + if pvc.Name == snap.Spec.Source.Name && snap.Status.ReadyToUse == false { + klog.V(2).Infof("Keeping PVC %s/%s, it is used by snapshot %s/%s", pvc.Namespace, pvc.Name, snap.Namespace, snap.Name) + return true + } + } + + klog.V(5).Infof("isSnapshotSourceBeingUsed: no snapshot is being created from PVC %s/%s", pvc.Namespace, pvc.Name) + return false +} + +// checkandRemoveSnapshotSourceFinalizer checks if the snapshot source finalizer should be removed +// and removed it if needed. +func (ctrl *csiSnapshotController) checkandRemoveSnapshotSourceFinalizer(snapshot *crdv1.VolumeSnapshot) error { + // Get snapshot source which is a PVC + pvc, err := ctrl.getClaimFromVolumeSnapshot(snapshot) + if err != nil { + klog.Infof("cannot get claim from snapshot [%s]: [%v] Claim may be deleted already. No need to remove finalizer on the claim.", snapshot.Name, err) + return nil + } + + klog.V(5).Infof("checkandRemoveSnapshotSourceFinalizer for snapshot [%s]: snapshot status [%#v]", snapshot.Name, snapshot.Status) + + // Check if there is a Finalizer on PVC to be removed + if slice.ContainsString(pvc.ObjectMeta.Finalizers, PVCFinalizer, nil) { + // There is a Finalizer on PVC. Check if PVC is used + // and remove finalizer if it's not used. + isUsed := ctrl.isSnapshotSourceBeingUsed(snapshot) + if !isUsed { + klog.Infof("checkandRemoveSnapshotSourceFinalizer[%s]: Remove Finalizer for PVC %s as it is not used by snapshots in creation", snapshot.Name, pvc.Name) + err = ctrl.removeSnapshotSourceFinalizer(snapshot) + if err != nil { + klog.Errorf("checkandRemoveSnapshotSourceFinalizer [%s]: removeSnapshotSourceFinalizer failed to remove finalizer %v", snapshot.Name, err) + return err + } + } + } + + return nil +} diff --git a/pkg/controller/snapshot_finalizer_test.go b/pkg/controller/snapshot_finalizer_test.go new file mode 100644 index 000000000..b675bd88e --- /dev/null +++ b/pkg/controller/snapshot_finalizer_test.go @@ -0,0 +1,44 @@ +/* +Copyright 2019 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package controller + +import ( + "testing" + + "k8s.io/api/core/v1" +) + +// Test single call to ensureSnapshotSourceFinalizer and checkandRemoveSnapshotSourceFinalizer, +// expecting PVCFinalizer to be added or removed +func TestPVCFinalizer(t *testing.T) { + + tests := []controllerTest{ + { + name: "1-1 - successful add PVC finalizer", + initialSnapshots: newSnapshotArray("snap6-2", classSilver, "", "snapuid6-2", "claim6-2", false, nil, nil, nil), + initialClaims: newClaimArray("claim6-2", "pvc-uid6-2", "1Gi", "volume6-2", v1.ClaimBound, &classEmpty), + test: testAddPVCFinalizer, + }, + { + name: "1-2 - successful remove PVC finalizer", + initialSnapshots: newSnapshotArray("snap6-2", classSilver, "", "snapuid6-2", "claim6-2", false, nil, nil, nil), + initialClaims: newClaimArrayFinalizer("claim6-2", "pvc-uid6-2", "1Gi", "volume6-2", v1.ClaimBound, &classEmpty), + test: testRemovePVCFinalizer, + }, + } + runPVCFinalizerTests(t, tests, snapshotClasses) +} diff --git a/pkg/controller/util.go b/pkg/controller/util.go index 75593d499..b91f32591 100644 --- a/pkg/controller/util.go +++ b/pkg/controller/util.go @@ -75,6 +75,9 @@ var snapshotterSecretParams = deprecatedSecretParamsMap{ secretNamespaceKey: prefixedSnapshotterSecretNamespaceKey, } +// Name of finalizer on PVCs that have been used as a source to create VolumeSnapshots +const PVCFinalizer = "snapshot.storage.kubernetes.io/pvc-protection" + func snapshotKey(vs *crdv1.VolumeSnapshot) string { return fmt.Sprintf("%s/%s", vs.Namespace, vs.Name) }