From a884a00d30bf2a4d7aa3f33965c72987eef5f127 Mon Sep 17 00:00:00 2001 From: Jan Safranek Date: Fri, 4 May 2018 12:22:15 +0200 Subject: [PATCH] Fix CSI volume detach when the volume is already detached. "NotFound" error should be treated as successful detach. --- pkg/volume/csi/BUILD | 1 + pkg/volume/csi/csi_attacher.go | 5 ++++ pkg/volume/csi/csi_attacher_test.go | 36 +++++++++++++++++++++-------- 3 files changed, 33 insertions(+), 9 deletions(-) diff --git a/pkg/volume/csi/BUILD b/pkg/volume/csi/BUILD index 3e0b9672e6f..2067ebfeb9d 100644 --- a/pkg/volume/csi/BUILD +++ b/pkg/volume/csi/BUILD @@ -48,6 +48,7 @@ go_test( "//vendor/k8s.io/apimachinery/pkg/api/errors:go_default_library", "//vendor/k8s.io/apimachinery/pkg/api/resource:go_default_library", "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", + "//vendor/k8s.io/apimachinery/pkg/runtime:go_default_library", "//vendor/k8s.io/apimachinery/pkg/types:go_default_library", "//vendor/k8s.io/apimachinery/pkg/watch:go_default_library", "//vendor/k8s.io/client-go/kubernetes/fake:go_default_library", diff --git a/pkg/volume/csi/csi_attacher.go b/pkg/volume/csi/csi_attacher.go index ae6a267c001..8a171b129f9 100644 --- a/pkg/volume/csi/csi_attacher.go +++ b/pkg/volume/csi/csi_attacher.go @@ -384,6 +384,11 @@ func (c *csiAttacher) Detach(volumeName string, nodeName types.NodeName) error { volID := parts[1] attachID := getAttachmentName(volID, driverName, string(nodeName)) if err := c.k8s.StorageV1beta1().VolumeAttachments().Delete(attachID, nil); err != nil { + if apierrs.IsNotFound(err) { + // object deleted or never existed, done + glog.V(4).Info(log("VolumeAttachment object [%v] for volume [%v] not found, object deleted", attachID, volID)) + return nil + } glog.Error(log("detacher.Detach failed to delete VolumeAttachment [%s]: %v", attachID, err)) return err } diff --git a/pkg/volume/csi/csi_attacher_test.go b/pkg/volume/csi/csi_attacher_test.go index e8c227e306b..03c68ba4974 100644 --- a/pkg/volume/csi/csi_attacher_test.go +++ b/pkg/volume/csi/csi_attacher_test.go @@ -26,6 +26,7 @@ import ( storage "k8s.io/api/storage/v1beta1" apierrs "k8s.io/apimachinery/pkg/api/errors" meta "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/watch" fakeclient "k8s.io/client-go/kubernetes/fake" @@ -110,7 +111,7 @@ func TestAttacherAttach(t *testing.T) { for i, tc := range testCases { t.Logf("test case: %s", tc.name) - plug, fakeWatcher, tmpDir := newTestWatchPlugin(t) + plug, fakeWatcher, tmpDir, _ := newTestWatchPlugin(t) defer os.RemoveAll(tmpDir) attacher, err := plug.NewAttacher() @@ -216,7 +217,7 @@ func TestAttacherWaitForVolumeAttachment(t *testing.T) { } for i, tc := range testCases { - plug, fakeWatcher, tmpDir := newTestWatchPlugin(t) + plug, fakeWatcher, tmpDir, _ := newTestWatchPlugin(t) defer os.RemoveAll(tmpDir) attacher, err := plug.NewAttacher() @@ -331,16 +332,33 @@ func TestAttacherDetach(t *testing.T) { volID string attachID string shouldFail bool + reactor func(action core.Action) (handled bool, ret runtime.Object, err error) }{ {name: "normal test", volID: "vol-001", attachID: getAttachmentName("vol-001", testDriver, nodeName)}, {name: "normal test 2", volID: "vol-002", attachID: getAttachmentName("vol-002", testDriver, nodeName)}, - {name: "object not found", volID: "vol-001", attachID: getAttachmentName("vol-002", testDriver, nodeName), shouldFail: true}, + {name: "object not found", volID: "vol-non-existing", attachID: getAttachmentName("vol-003", testDriver, nodeName)}, + { + name: "API error", + volID: "vol-004", + attachID: getAttachmentName("vol-004", testDriver, nodeName), + shouldFail: true, // All other API errors should be propagated to caller + reactor: func(action core.Action) (handled bool, ret runtime.Object, err error) { + // return Forbidden to all DELETE requests + if action.Matches("delete", "volumeattachments") { + return true, nil, apierrs.NewForbidden(action.GetResource().GroupResource(), action.GetNamespace(), fmt.Errorf("mock error")) + } + return false, nil, nil + }, + }, } for _, tc := range testCases { t.Logf("running test: %v", tc.name) - plug, fakeWatcher, tmpDir := newTestWatchPlugin(t) + plug, fakeWatcher, tmpDir, client := newTestWatchPlugin(t) defer os.RemoveAll(tmpDir) + if tc.reactor != nil { + client.PrependReactor("*", "*", tc.reactor) + } attacher, err0 := plug.NewAttacher() if err0 != nil { @@ -385,7 +403,7 @@ func TestAttacherDetach(t *testing.T) { func TestAttacherGetDeviceMountPath(t *testing.T) { // Setup // Create a new attacher - plug, _, tmpDir := newTestWatchPlugin(t) + plug, _, tmpDir, _ := newTestWatchPlugin(t) defer os.RemoveAll(tmpDir) attacher, err0 := plug.NewAttacher() if err0 != nil { @@ -498,7 +516,7 @@ func TestAttacherMountDevice(t *testing.T) { // Setup // Create a new attacher - plug, fakeWatcher, tmpDir := newTestWatchPlugin(t) + plug, fakeWatcher, tmpDir, _ := newTestWatchPlugin(t) defer os.RemoveAll(tmpDir) attacher, err0 := plug.NewAttacher() if err0 != nil { @@ -620,7 +638,7 @@ func TestAttacherUnmountDevice(t *testing.T) { t.Logf("Running test case: %s", tc.testName) // Setup // Create a new attacher - plug, _, tmpDir := newTestWatchPlugin(t) + plug, _, tmpDir, _ := newTestWatchPlugin(t) defer os.RemoveAll(tmpDir) attacher, err0 := plug.NewAttacher() if err0 != nil { @@ -678,7 +696,7 @@ func TestAttacherUnmountDevice(t *testing.T) { } // create a plugin mgr to load plugins and setup a fake client -func newTestWatchPlugin(t *testing.T) (*csiPlugin, *watch.RaceFreeFakeWatcher, string) { +func newTestWatchPlugin(t *testing.T) (*csiPlugin, *watch.RaceFreeFakeWatcher, string, *fakeclient.Clientset) { tmpDir, err := utiltesting.MkTmpdir("csi-test") if err != nil { t.Fatalf("can't create temp dir: %v", err) @@ -706,5 +724,5 @@ func newTestWatchPlugin(t *testing.T) (*csiPlugin, *watch.RaceFreeFakeWatcher, s t.Fatalf("cannot assert plugin to be type csiPlugin") } - return csiPlug, fakeWatcher, tmpDir + return csiPlug, fakeWatcher, tmpDir, fakeClient }