From dd7890c362d966ac465a19a4d1cfe2eb1ec06132 Mon Sep 17 00:00:00 2001 From: Jan Safranek Date: Tue, 17 May 2016 14:55:23 +0200 Subject: [PATCH] delete: Implement Deleter --- .../persistentvolume_controller.go | 94 ++++++++-- .../persistentvolume_delete_test.go | 169 ++++++++++++++++++ .../persistentvolume_framework_test.go | 23 +++ 3 files changed, 268 insertions(+), 18 deletions(-) create mode 100644 pkg/controller/persistentvolume/persistentvolume_delete_test.go diff --git a/pkg/controller/persistentvolume/persistentvolume_controller.go b/pkg/controller/persistentvolume/persistentvolume_controller.go index 7d47f149a05..9e3f15ddcff 100644 --- a/pkg/controller/persistentvolume/persistentvolume_controller.go +++ b/pkg/controller/persistentvolume/persistentvolume_controller.go @@ -1120,25 +1120,55 @@ func (ctrl *PersistentVolumeController) recycleVolumeOperation(arg interface{}) // deleteVolumeOperation deletes a volume. This method is running in standalone // goroutine and already has all necessary locks. func (ctrl *PersistentVolumeController) deleteVolumeOperation(arg interface{}) { - volume := arg.(*api.PersistentVolume) - glog.V(4).Infof("deleteVolumeOperation [%s] started", volume.Name) - /*else if pv.Spec.ReclaimPolicy == "Delete" { - plugin := findDeleterPluginForPV(pv) - if plugin != nil { - // maintain a map with the current deleter goroutines that are running - // if the key is already present in the map, return - // - // launch the goroutine that: - // 1. deletes the storage asset - // 2. deletes the PV API object - // 3. deletes itself from the map when it's done - } else { - // make an event calling out that no deleter was configured - // mark the PV as failed - // NB: external provisioners/deleters are currently not - // considered. + volume, ok := arg.(*api.PersistentVolume) + if !ok { + glog.Errorf("Cannot convert deleteVolumeOperation argument to volume, got %+v", arg) + return } - */ + glog.V(4).Infof("deleteVolumeOperation [%s] started", volume.Name) + + // This method may have been waiting for a volume lock for some time. + // Previous deleteVolumeOperation might just have saved an updated version, so + // read current volume state now. + newVolume, err := ctrl.kubeClient.Core().PersistentVolumes().Get(volume.Name) + if err != nil { + glog.V(3).Infof("error reading peristent volume %q: %v", volume.Name, err) + return + } + needsReclaim, err := ctrl.isVolumeReleased(newVolume) + if err != nil { + glog.V(3).Infof("error reading claim for volume %q: %v", volume.Name, err) + return + } + if !needsReclaim { + glog.V(3).Infof("volume %q no longer needs deletion, skipping", volume.Name) + return + } + + if err = ctrl.doDeleteVolume(volume); err != nil { + // Delete failed, update the volume and emit an event. + glog.V(3).Infof("deletion of volume %q failed: %v", volume.Name, err) + if _, err = ctrl.updateVolumePhaseWithEvent(volume, api.VolumeFailed, api.EventTypeWarning, "VolumeFailedDelete", err.Error()); err != nil { + glog.V(4).Infof("deleteVolumeOperation [%s]: failed to mark volume as failed: %v", volume.Name, err) + // Save failed, retry on the next deletion attempt + return + } + // Despite the volume being Failed, the controller will retry deleting + // the volume in every syncVolume() call. + return + } + + glog.V(4).Infof("deleteVolumeOperation [%s]: success", volume.Name) + // Delete the volume + if err = ctrl.kubeClient.Core().PersistentVolumes().Delete(volume.Name, nil); err != nil { + // Oops, could not delete the volume and therefore the controller will + // try to delete the volume again on next update. We _could_ maintain a + // cache of "recently deleted volumes" and avoid unnecessary deletion, + // this is left out as future optimization. + glog.V(3).Infof("failed to delete volume %q from database: %v", volume.Name, err) + return + } + return } // isVolumeReleased returns true if given volume is released and can be recycled @@ -1183,6 +1213,34 @@ func (ctrl *PersistentVolumeController) isVolumeReleased(volume *api.PersistentV return true, nil } +// doDeleteVolume finds appropriate delete plugin and deletes given volume +// (it will be re-used in future provisioner error cases). +func (ctrl *PersistentVolumeController) doDeleteVolume(volume *api.PersistentVolume) error { + glog.V(4).Infof("doDeleteVolume [%s]", volume.Name) + // Find a plugin. + spec := vol.NewSpecFromPersistentVolume(volume, false) + plugin, err := ctrl.recyclePluginMgr.FindDeletablePluginBySpec(spec) + if err != nil { + // No deleter found. Emit an event and mark the volume Failed. + return fmt.Errorf("Error getting deleter volume plugin for volume %q: %v", volume.Name, err) + } + + // Plugin found + deleter, err := plugin.NewDeleter(spec) + if err != nil { + // Cannot create deleter + return fmt.Errorf("Failed to create deleter for volume %q: %v", volume.Name, err) + } + + if err = deleter.Delete(); err != nil { + // Deleter failed + return fmt.Errorf("Delete of volume %q failed: %v", volume.Name, err) + } + + glog.V(2).Infof("volume %q deleted", volume.Name) + return nil +} + // scheduleOperation starts given asynchronous operation on given volume. It // makes sure the operation is already not running. func (ctrl *PersistentVolumeController) scheduleOperation(operationName string, operation func(arg interface{}), arg interface{}) { diff --git a/pkg/controller/persistentvolume/persistentvolume_delete_test.go b/pkg/controller/persistentvolume/persistentvolume_delete_test.go new file mode 100644 index 00000000000..c1a3dee9558 --- /dev/null +++ b/pkg/controller/persistentvolume/persistentvolume_delete_test.go @@ -0,0 +1,169 @@ +/* +Copyright 2016 The Kubernetes Authors All rights reserved. + +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 persistentvolume + +import ( + "errors" + "testing" + + "k8s.io/kubernetes/pkg/api" +) + +// Test single call to syncVolume, expecting recycling to happen. +// 1. Fill in the controller with initial data +// 2. Call the syncVolume *once*. +// 3. Compare resulting volumes with expected volumes. +func TestDeleteSync(t *testing.T) { + tests := []controllerTest{ + { + // delete volume bound by controller + "8-1 - successful delete", + newVolumeArray("volume8-1", "1Gi", "uid8-1", "claim8-1", api.VolumeBound, api.PersistentVolumeReclaimDelete, annBoundByController), + novolumes, + noclaims, + noclaims, + noevents, + // Inject deleter into the controller and call syncVolume. The + // deleter simulates one delete() call that succeeds. + wrapTestWithControllerConfig(operationDelete, []error{nil}, testSyncVolume), + }, + { + // delete volume bound by user + "8-2 - successful delete with prebound volume", + newVolumeArray("volume8-2", "1Gi", "uid8-2", "claim8-2", api.VolumeBound, api.PersistentVolumeReclaimDelete), + novolumes, + noclaims, + noclaims, + noevents, + // Inject deleter into the controller and call syncVolume. The + // deleter simulates one delete() call that succeeds. + wrapTestWithControllerConfig(operationDelete, []error{nil}, testSyncVolume), + }, + { + // delete failure - plugin not found + "8-3 - plugin not found", + newVolumeArray("volume8-3", "1Gi", "uid8-3", "claim8-3", api.VolumeBound, api.PersistentVolumeReclaimDelete), + newVolumeArray("volume8-3", "1Gi", "uid8-3", "claim8-3", api.VolumeFailed, api.PersistentVolumeReclaimDelete), + noclaims, + noclaims, + []string{"Warning VolumeFailedDelete"}, testSyncVolume, + }, + { + // delete failure - newDeleter returns error + "8-4 - newDeleter returns error", + newVolumeArray("volume8-4", "1Gi", "uid8-4", "claim8-4", api.VolumeBound, api.PersistentVolumeReclaimDelete), + newVolumeArray("volume8-4", "1Gi", "uid8-4", "claim8-4", api.VolumeFailed, api.PersistentVolumeReclaimDelete), + noclaims, + noclaims, + []string{"Warning VolumeFailedDelete"}, + wrapTestWithControllerConfig(operationDelete, []error{}, testSyncVolume), + }, + { + // delete failure - delete() returns error + "8-5 - delete returns error", + newVolumeArray("volume8-5", "1Gi", "uid8-5", "claim8-5", api.VolumeBound, api.PersistentVolumeReclaimDelete), + newVolumeArray("volume8-5", "1Gi", "uid8-5", "claim8-5", api.VolumeFailed, api.PersistentVolumeReclaimDelete), + noclaims, + noclaims, + []string{"Warning VolumeFailedDelete"}, + wrapTestWithControllerConfig(operationDelete, []error{errors.New("Mock delete error")}, testSyncVolume), + }, + { + // delete success(?) - volume is deleted before doDelete() starts + "8-6 - volume is deleted before deleting", + newVolumeArray("volume8-6", "1Gi", "uid8-6", "claim8-6", api.VolumeBound, api.PersistentVolumeReclaimDelete), + novolumes, + noclaims, + noclaims, + noevents, + wrapTestWithInjectedOperation(wrapTestWithControllerConfig(operationDelete, []error{}, testSyncVolume), func(ctrl *PersistentVolumeController, reactor *volumeReactor) { + // Delete the volume before delete operation starts + reactor.lock.Lock() + delete(reactor.volumes, "volume8-6") + reactor.lock.Unlock() + }), + }, + { + // delete success(?) - volume is bound just at the time doDelete() + // starts. This simulates "volume no longer needs recycling, + // skipping". + "8-7 - volume is bound before deleting", + newVolumeArray("volume8-7", "1Gi", "uid8-7", "claim8-7", api.VolumeBound, api.PersistentVolumeReclaimDelete, annBoundByController), + newVolumeArray("volume8-7", "1Gi", "uid8-7", "claim8-7", api.VolumeBound, api.PersistentVolumeReclaimDelete, annBoundByController), + noclaims, + newClaimArray("claim8-7", "uid8-7", "10Gi", "volume8-7", api.ClaimBound), + noevents, + wrapTestWithInjectedOperation(wrapTestWithControllerConfig(operationDelete, []error{}, testSyncVolume), func(ctrl *PersistentVolumeController, reactor *volumeReactor) { + reactor.lock.Lock() + defer reactor.lock.Unlock() + // Bind the volume to ressurected claim (this should never + // happen) + claim := newClaim("claim8-7", "uid8-7", "10Gi", "volume8-7", api.ClaimBound) + reactor.claims[claim.Name] = claim + ctrl.claims.Add(claim) + volume := reactor.volumes["volume8-7"] + volume.Status.Phase = api.VolumeBound + }), + }, + { + // delete success - volume bound by user is deleted, while a new + // claim is created with another UID. + "8-9 - prebound volume is deleted while the claim exists", + newVolumeArray("volume8-9", "1Gi", "uid8-9", "claim8-9", api.VolumeBound, api.PersistentVolumeReclaimDelete), + novolumes, + newClaimArray("claim8-9", "uid8-9-x", "10Gi", "", api.ClaimPending), + newClaimArray("claim8-9", "uid8-9-x", "10Gi", "", api.ClaimPending), + noevents, + // Inject deleter into the controller and call syncVolume. The + // deleter simulates one delete() call that succeeds. + wrapTestWithControllerConfig(operationDelete, []error{nil}, testSyncVolume), + }, + } + runSyncTests(t, tests) +} + +// Test multiple calls to syncClaim/syncVolume and periodic sync of all +// volume/claims. The test follows this pattern: +// 0. Load the controller with initial data. +// 1. Call controllerTest.testCall() once as in TestSync() +// 2. For all volumes/claims changed by previous syncVolume/syncClaim calls, +// call appropriate syncVolume/syncClaim (simulating "volume/claim changed" +// events). Go to 2. if these calls change anything. +// 3. When all changes are processed and no new changes were made, call +// syncVolume/syncClaim on all volumes/claims (simulating "periodic sync"). +// 4. If some changes were done by step 3., go to 2. (simulation of +// "volume/claim updated" events, eventually performing step 3. again) +// 5. When 3. does not do any changes, finish the tests and compare final set +// of volumes/claims with expected claims/volumes and report differences. +// Some limit of calls in enforced to prevent endless loops. +func TestDeleteMultiSync(t *testing.T) { + tests := []controllerTest{ + { + // delete failure - delete returns error. The controller should + // try again. + "9-1 - delete returns error", + newVolumeArray("volume9-1", "1Gi", "uid9-1", "claim9-1", api.VolumeBound, api.PersistentVolumeReclaimDelete), + novolumes, + noclaims, + noclaims, + []string{"Warning VolumeFailedDelete"}, + wrapTestWithControllerConfig(operationDelete, []error{errors.New("Mock delete error"), nil}, testSyncVolume), + }, + } + + runMultisyncTests(t, tests) +} diff --git a/pkg/controller/persistentvolume/persistentvolume_framework_test.go b/pkg/controller/persistentvolume/persistentvolume_framework_test.go index c43dac4eab3..a0115ae5750 100644 --- a/pkg/controller/persistentvolume/persistentvolume_framework_test.go +++ b/pkg/controller/persistentvolume/persistentvolume_framework_test.go @@ -198,7 +198,30 @@ func (r *volumeReactor) React(action core.Action) (handled bool, ret runtime.Obj glog.V(4).Infof("GetVolume: volume %s not found", name) return true, nil, fmt.Errorf("Cannot find volume %s", name) } + + case action.Matches("delete", "persistentvolumes"): + name := action.(core.DeleteAction).GetName() + glog.V(4).Infof("deleted volume %s", name) + _, found := r.volumes[name] + if found { + delete(r.volumes, name) + return true, nil, nil + } else { + return true, nil, fmt.Errorf("Cannot delete volume %s: not found", name) + } + + case action.Matches("delete", "persistentvolumeclaims"): + name := action.(core.DeleteAction).GetName() + glog.V(4).Infof("deleted claim %s", name) + _, found := r.volumes[name] + if found { + delete(r.claims, name) + return true, nil, nil + } else { + return true, nil, fmt.Errorf("Cannot delete claim %s: not found", name) + } } + return false, nil, nil }