Merge pull request #18083 from markturansky/fail_if_no_recycler

pkg/controller/persistentvolume/persistentvolume_recycler_controller.go

@@ -136,23 +136,26 @@ func (recycler *PersistentVolumeRecycler) handleRecycle(pv *api.PersistentVolume
 	spec := volume.NewSpecFromPersistentVolume(pv, false)
 	plugin, err := recycler.pluginMgr.FindRecyclablePluginBySpec(spec)
 	if err != nil {
-		return fmt.Errorf("Could not find recyclable volume plugin for spec: %+v", err)
-	}
-	volRecycler, err := plugin.NewRecycler(spec)
-	if err != nil {
-		return fmt.Errorf("Could not obtain Recycler for spec: %+v", err)
-	}
-	// blocks until completion
-	err = volRecycler.Recycle()
-	if err != nil {
-		glog.Errorf("PersistentVolume[%s] failed recycling: %+v", pv.Name, err)
-		pv.Status.Message = fmt.Sprintf("Recycling error: %s", err)
 		nextPhase = api.VolumeFailed
-	} else {
-		glog.V(5).Infof("PersistentVolume[%s] successfully recycled\n", pv.Name)
-		nextPhase = api.VolumePending
+		pv.Status.Message = fmt.Sprintf("%v", err)
+	}
+
+	// an error above means a suitable plugin for this volume was not found.
+	// we don't need to attempt recycling when plugin is nil, but we do need to persist the next/failed phase
+	// of the volume so that subsequent syncs won't attempt recycling through this handler func.
+	if plugin != nil {
+		volRecycler, err := plugin.NewRecycler(spec)
 		if err != nil {
-			glog.Errorf("Error updating pv.Status: %+v", err)
+			return fmt.Errorf("Could not obtain Recycler for spec: %#v  error: %v", spec, err)
+		}
+		// blocks until completion
+		if err := volRecycler.Recycle(); err != nil {
+			glog.Errorf("PersistentVolume[%s] failed recycling: %+v", pv.Name, err)
+			pv.Status.Message = fmt.Sprintf("Recycling error: %s", err)
+			nextPhase = api.VolumeFailed
+		} else {
+			glog.V(5).Infof("PersistentVolume[%s] successfully recycled\n", pv.Name)
+			nextPhase = api.VolumePending
 		}
 	}
 
@@ -178,24 +181,30 @@ func (recycler *PersistentVolumeRecycler) handleDelete(pv *api.PersistentVolume)
 	spec := volume.NewSpecFromPersistentVolume(pv, false)
 	plugin, err := recycler.pluginMgr.FindDeletablePluginBySpec(spec)
 	if err != nil {
-		return fmt.Errorf("Could not find deletable volume plugin for spec: %+v", err)
-	}
-	deleter, err := plugin.NewDeleter(spec)
-	if err != nil {
-		return fmt.Errorf("could not obtain Deleter for spec: %+v", err)
-	}
-	// blocks until completion
-	err = deleter.Delete()
-	if err != nil {
-		glog.Errorf("PersistentVolume[%s] failed deletion: %+v", pv.Name, err)
-		pv.Status.Message = fmt.Sprintf("Deletion error: %s", err)
 		nextPhase = api.VolumeFailed
-	} else {
-		glog.V(5).Infof("PersistentVolume[%s] successfully deleted through plugin\n", pv.Name)
-		// after successful deletion through the plugin, we can also remove the PV from the cluster
-		err = recycler.client.DeletePersistentVolume(pv)
+		pv.Status.Message = fmt.Sprintf("%v", err)
+	}
+
+	// an error above means a suitable plugin for this volume was not found.
+	// we don't need to attempt deleting when plugin is nil, but we do need to persist the next/failed phase
+	// of the volume so that subsequent syncs won't attempt deletion through this handler func.
+	if plugin != nil {
+		deleter, err := plugin.NewDeleter(spec)
+		if err != nil {
+			return fmt.Errorf("Could not obtain Deleter for spec: %#v  error: %v", spec, err)
+		}
+		// blocks until completion
+		err = deleter.Delete()
 		if err != nil {
-			return fmt.Errorf("error deleting persistent volume: %+v", err)
+			glog.Errorf("PersistentVolume[%s] failed deletion: %+v", pv.Name, err)
+			pv.Status.Message = fmt.Sprintf("Deletion error: %s", err)
+			nextPhase = api.VolumeFailed
+		} else {
+			glog.V(5).Infof("PersistentVolume[%s] successfully deleted through plugin\n", pv.Name)
+			// after successful deletion through the plugin, we can also remove the PV from the cluster
+			if err := recycler.client.DeletePersistentVolume(pv); err != nil {
+				return fmt.Errorf("error deleting persistent volume: %+v", err)
+			}
 		}
 	}
 

pkg/controller/persistentvolume/persistentvolume_recycler_controller_test.go

+/*
+Copyright 2015 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 (
+	"testing"
+
+	"k8s.io/kubernetes/pkg/api"
+	"k8s.io/kubernetes/pkg/api/resource"
+	"k8s.io/kubernetes/pkg/client/unversioned/testclient"
+	"k8s.io/kubernetes/pkg/volume"
+)
+
+func TestFailedRecycling(t *testing.T) {
+	pv := &api.PersistentVolume{
+		Spec: api.PersistentVolumeSpec{
+			AccessModes: []api.PersistentVolumeAccessMode{api.ReadWriteOnce},
+			Capacity: api.ResourceList{
+				api.ResourceName(api.ResourceStorage): resource.MustParse("8Gi"),
+			},
+			PersistentVolumeSource: api.PersistentVolumeSource{
+				HostPath: &api.HostPathVolumeSource{
+					Path: "/tmp/data02",
+				},
+			},
+			PersistentVolumeReclaimPolicy: api.PersistentVolumeReclaimRecycle,
+			ClaimRef: &api.ObjectReference{
+				Name:      "foo",
+				Namespace: "bar",
+			},
+		},
+		Status: api.PersistentVolumeStatus{
+			Phase: api.VolumeReleased,
+		},
+	}
+
+	mockClient := &mockBinderClient{
+		volume: pv,
+	}
+
+	// no Init called for pluginMgr and no plugins are available.  Volume should fail recycling.
+	plugMgr := volume.VolumePluginMgr{}
+
+	recycler := &PersistentVolumeRecycler{
+		kubeClient: &testclient.Fake{},
+		client:     mockClient,
+		pluginMgr:  plugMgr,
+	}
+
+	err := recycler.reclaimVolume(pv)
+	if err != nil {
+		t.Errorf("Unexpected non-nil error: %v", err)
+	}
+
+	if mockClient.volume.Status.Phase != api.VolumeFailed {
+		t.Errorf("Expected %s but got %s", api.VolumeFailed, mockClient.volume.Status.Phase)
+	}
+
+	pv.Spec.PersistentVolumeReclaimPolicy = api.PersistentVolumeReclaimDelete
+	err = recycler.reclaimVolume(pv)
+	if err != nil {
+		t.Errorf("Unexpected non-nil error: %v", err)
+	}
+
+	if mockClient.volume.Status.Phase != api.VolumeFailed {
+		t.Errorf("Expected %s but got %s", api.VolumeFailed, mockClient.volume.Status.Phase)
+	}
+}