Merge pull request #32565 from jsafrane/deleter-plugin

Automatic merge from submit-queue

Do not report warning event when an unknown deleter is requested

When Kubernetes does not have a plugin to delete a PV it should wait for
either external deleter or storage admin to delete the volume instead of
throwing an error.

This is the same approach as in #32077

@kubernetes/sig-storage

pkg/controller/volume/persistentvolume/controller.go

@@ -1072,7 +1072,8 @@ func (ctrl *PersistentVolumeController) deleteVolumeOperation(arg interface{}) {
 		return
 	}
 
-	if err = ctrl.doDeleteVolume(volume); err != nil {
+	deleted, err := ctrl.doDeleteVolume(volume)
+	if 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 {
@@ -1084,6 +1085,10 @@ func (ctrl *PersistentVolumeController) deleteVolumeOperation(arg interface{}) {
 		// the volume in every syncVolume() call.
 		return
 	}
+	if !deleted {
+		// The volume waits for deletion by an external plugin. Do nothing.
+		return
+	}
 
 	glog.V(4).Infof("deleteVolumeOperation [%s]: success", volume.Name)
 	// Delete the volume
@@ -1140,51 +1145,40 @@ 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 {
+// doDeleteVolume finds appropriate delete plugin and deletes given volume. It
+// returns 'true', when the volume was deleted and 'false' when the volume
+// cannot be deleted because of the deleter is external. No error should be
+// reported in this case.
+func (ctrl *PersistentVolumeController) doDeleteVolume(volume *api.PersistentVolume) (bool, error) {
 	glog.V(4).Infof("doDeleteVolume [%s]", volume.Name)
 	var err error
 
-	// Find a plugin. Try to find the same plugin that provisioned the volume
-	var plugin vol.DeletableVolumePlugin
-	if hasAnnotation(volume.ObjectMeta, annDynamicallyProvisioned) {
-		provisionPluginName := volume.Annotations[annDynamicallyProvisioned]
-		if provisionPluginName != "" {
-			plugin, err = ctrl.volumePluginMgr.FindDeletablePluginByName(provisionPluginName)
-			if err != nil {
-				glog.V(3).Infof("did not find a deleter plugin %q for volume %q: %v, will try to find a generic one",
-					provisionPluginName, volume.Name, err)
-			}
-		}
+	plugin, err := ctrl.findDeletablePlugin(volume)
+	if err != nil {
+		return false, err
 	}
-
-	spec := vol.NewSpecFromPersistentVolume(volume, false)
 	if plugin == nil {
-		// The plugin that provisioned the volume was not found or the volume
-		// was not dynamically provisioned. Try to find a plugin by spec.
-		plugin, err = ctrl.volumePluginMgr.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)
-		}
+		// External deleter is requested, do nothing
+		glog.V(3).Infof("external deleter for volume %q requested, ignoring", volume.Name)
+		return false, nil
 	}
-	glog.V(5).Infof("found a deleter plugin %q for volume %q", plugin.GetPluginName(), volume.Name)
 
 	// Plugin found
+	glog.V(5).Infof("found a deleter plugin %q for volume %q", plugin.GetPluginName(), volume.Name)
+	spec := vol.NewSpecFromPersistentVolume(volume, false)
 	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)
+		return false, 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)
+		return false, fmt.Errorf("Delete of volume %q failed: %v", volume.Name, err)
 	}
 
 	glog.V(2).Infof("volume %q deleted", volume.Name)
-	return nil
+	return true, nil
 }
 
 // provisionClaim starts new asynchronous operation to provision a claim if
@@ -1339,11 +1333,19 @@ func (ctrl *PersistentVolumeController) provisionClaimOperation(claimObj interfa
 		ctrl.eventRecorder.Event(claim, api.EventTypeWarning, "ProvisioningFailed", strerr)
 
 		for i := 0; i < ctrl.createProvisionedPVRetryCount; i++ {
-			if err = ctrl.doDeleteVolume(volume); err == nil {
+			deleted, err := ctrl.doDeleteVolume(volume)
+			if err == nil && deleted {
 				// Delete succeeded
 				glog.V(4).Infof("provisionClaimOperation [%s]: cleaning volume %s succeeded", claimToClaimKey(claim), volume.Name)
 				break
 			}
+			if !deleted {
+				// This is unreachable code, the volume was provisioned by an
+				// internal plugin and therefore there MUST be an internal
+				// plugin that deletes it.
+				glog.Errorf("Error finding internal deleter for volume plugin %q", plugin.GetPluginName())
+				break
+			}
 			// Delete failed, try again after a while.
 			glog.V(3).Infof("failed to delete volume %q: %v", volume.Name, err)
 			time.Sleep(ctrl.createProvisionedPVInterval)
@@ -1453,3 +1455,34 @@ func (ctrl *PersistentVolumeController) findAlphaProvisionablePlugin() (vol.Prov
 	glog.V(4).Infof("using alpha provisioner %s", ctrl.alphaProvisioner.GetPluginName())
 	return ctrl.alphaProvisioner, storageClass, nil
 }
+
+// findDeletablePlugin finds a deleter plugin for a given volume. It returns
+// either the deleter plugin or nil when an external deleter is requested.
+func (ctrl *PersistentVolumeController) findDeletablePlugin(volume *api.PersistentVolume) (vol.DeletableVolumePlugin, error) {
+	// Find a plugin. Try to find the same plugin that provisioned the volume
+	var plugin vol.DeletableVolumePlugin
+	if hasAnnotation(volume.ObjectMeta, annDynamicallyProvisioned) {
+		provisionPluginName := volume.Annotations[annDynamicallyProvisioned]
+		if provisionPluginName != "" {
+			plugin, err := ctrl.volumePluginMgr.FindDeletablePluginByName(provisionPluginName)
+			if err != nil {
+				if !strings.HasPrefix(provisionPluginName, "kubernetes.io/") {
+					// External provisioner is requested, do not report error
+					return nil, nil
+				}
+				return nil, err
+			}
+			return plugin, nil
+		}
+	}
+
+	// The plugin that provisioned the volume was not found or the volume
+	// was not dynamically provisioned. Try to find a plugin by spec.
+	spec := vol.NewSpecFromPersistentVolume(volume, false)
+	plugin, err := ctrl.volumePluginMgr.FindDeletablePluginBySpec(spec)
+	if err != nil {
+		// No deleter found. Emit an event and mark the volume Failed.
+		return nil, fmt.Errorf("Error getting deleter volume plugin for volume %q: %v", volume.Name, err)
+	}
+	return plugin, nil
+}

pkg/controller/volume/persistentvolume/delete_test.go

@@ -133,6 +133,21 @@ func TestDeleteSync(t *testing.T) {
 			// deleter simulates one delete() call that succeeds.
 			wrapTestWithReclaimCalls(operationDelete, []error{nil}, testSyncVolume),
 		},
+		{
+			// PV requires external deleter
+			"8-10 - external deleter",
+			newVolumeArray("volume8-10", "1Gi", "uid10-1", "claim10-1", api.VolumeBound, api.PersistentVolumeReclaimDelete, annBoundByController),
+			newVolumeArray("volume8-10", "1Gi", "uid10-1", "claim10-1", api.VolumeReleased, api.PersistentVolumeReclaimDelete, annBoundByController),
+			noclaims,
+			noclaims,
+			noevents, noerrors,
+			func(ctrl *PersistentVolumeController, reactor *volumeReactor, test controllerTest) error {
+				// Inject external deleter annotation
+				test.initialVolumes[0].Annotations[annDynamicallyProvisioned] = "external.io/test"
+				test.expectedVolumes[0].Annotations[annDynamicallyProvisioned] = "external.io/test"
+				return testSyncVolume(ctrl, reactor, test)
+			},
+		},
 	}
 	runSyncTests(t, tests, []*storage.StorageClass{})
 }