From 300f531389384384f831f00e24dc65067e9f66e3 Mon Sep 17 00:00:00 2001 From: FengyunPan Date: Wed, 31 May 2017 07:52:15 +0800 Subject: [PATCH] Wait for detach operation to complete When volume's status is 'detaching', controllermanager will detach it again and return err. It is necessary to wait for detach operation to complete within the alloted time. --- pkg/volume/cinder/BUILD | 1 + pkg/volume/cinder/attacher.go | 78 ++++++++++++++++++++++++++++-- pkg/volume/cinder/attacher_test.go | 61 +++++++++++++++++------ 3 files changed, 122 insertions(+), 18 deletions(-) diff --git a/pkg/volume/cinder/BUILD b/pkg/volume/cinder/BUILD index a09e56a4dc1..eaa1a4b508d 100644 --- a/pkg/volume/cinder/BUILD +++ b/pkg/volume/cinder/BUILD @@ -35,6 +35,7 @@ go_library( "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", "//vendor/k8s.io/apimachinery/pkg/types:go_default_library", "//vendor/k8s.io/apimachinery/pkg/util/sets:go_default_library", + "//vendor/k8s.io/apimachinery/pkg/util/wait:go_default_library", ], ) diff --git a/pkg/volume/cinder/attacher.go b/pkg/volume/cinder/attacher.go index ace2100306b..8d976ca4a1d 100644 --- a/pkg/volume/cinder/attacher.go +++ b/pkg/volume/cinder/attacher.go @@ -25,6 +25,7 @@ import ( "github.com/golang/glog" "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/wait" "k8s.io/kubernetes/pkg/util/exec" "k8s.io/kubernetes/pkg/util/mount" "k8s.io/kubernetes/pkg/volume" @@ -41,7 +42,13 @@ var _ volume.Attacher = &cinderDiskAttacher{} var _ volume.AttachableVolumePlugin = &cinderPlugin{} const ( - checkSleepDuration = time.Second + checkSleepDuration = 1 * time.Second + operationFinishInitDealy = 1 * time.Second + operationFinishFactor = 1.1 + operationFinishSteps = 10 + diskDetachInitDealy = 1 * time.Second + diskDetachFactor = 1.2 + diskDetachSteps = 13 ) func (plugin *cinderPlugin) NewAttacher() (volume.Attacher, error) { @@ -267,6 +274,63 @@ func (plugin *cinderPlugin) NewDetacher() (volume.Detacher, error) { }, nil } +func (detacher *cinderDiskDetacher) waitOperationFinished(volumeID string) error { + backoff := wait.Backoff{ + Duration: operationFinishInitDealy, + Factor: operationFinishFactor, + Steps: operationFinishSteps, + } + + var volumeStatus string + err := wait.ExponentialBackoff(backoff, func() (bool, error) { + var pending bool + var err error + pending, volumeStatus, err = detacher.cinderProvider.OperationPending(volumeID) + if err == nil { + if pending == false { + return true, nil + } else { + return false, nil + } + } else { + return false, err + } + }) + + if err == wait.ErrWaitTimeout { + err = fmt.Errorf("Volume %q is %s, can't finish within the alloted time", volumeID, volumeStatus) + } + + return err +} + +func (detacher *cinderDiskDetacher) waitDiskDetached(instanceID, volumeID string) error { + backoff := wait.Backoff{ + Duration: diskDetachInitDealy, + Factor: diskDetachFactor, + Steps: diskDetachSteps, + } + + err := wait.ExponentialBackoff(backoff, func() (bool, error) { + attached, err := detacher.cinderProvider.DiskIsAttached(instanceID, volumeID) + if err == nil { + if attached == false { + return true, nil + } else { + return false, nil + } + } else { + return false, err + } + }) + + if err == wait.ErrWaitTimeout { + err = fmt.Errorf("Volume %q failed to detach within the alloted time", volumeID) + } + + return err +} + func (detacher *cinderDiskDetacher) Detach(deviceMountPath string, nodeName types.NodeName) error { volumeID := path.Base(deviceMountPath) instances, res := detacher.cinderProvider.Instances() @@ -278,6 +342,10 @@ func (detacher *cinderDiskDetacher) Detach(deviceMountPath string, nodeName type instanceID = instanceID[(ind + 1):] } + if err := detacher.waitOperationFinished(volumeID); err != nil { + return err + } + attached, err := detacher.cinderProvider.DiskIsAttached(instanceID, volumeID) if err != nil { // Log error and continue with detach @@ -293,10 +361,14 @@ func (detacher *cinderDiskDetacher) Detach(deviceMountPath string, nodeName type } if err = detacher.cinderProvider.DetachDisk(instanceID, volumeID); err != nil { - glog.Errorf("Error detaching volume %q: %v", volumeID, err) + glog.Errorf("Error detaching volume %q from node %q: %v", volumeID, nodeName, err) return err } - glog.Infof("detached volume %q from instance %q", volumeID, instanceID) + if err = detacher.waitDiskDetached(instanceID, volumeID); err != nil { + glog.Errorf("Error waiting for volume %q to detach from node %q: %v", volumeID, nodeName, err) + return err + } + glog.Infof("detached volume %q from node %q", volumeID, nodeName) return nil } diff --git a/pkg/volume/cinder/attacher_test.go b/pkg/volume/cinder/attacher_test.go index 6f13bad32cf..534ee57c864 100644 --- a/pkg/volume/cinder/attacher_test.go +++ b/pkg/volume/cinder/attacher_test.go @@ -36,6 +36,8 @@ import ( const VolumeStatusPending = "pending" const VolumeStatusDone = "done" +var VolumeIsNotAttached = true + func TestGetDeviceName_Volume(t *testing.T) { plugin := newPlugin() name := "my-cinder-volume" @@ -96,6 +98,7 @@ type testcase struct { disksAreAttached disksAreAttachedCall diskPath diskPathCall t *testing.T + notAttached *bool instanceID string // Actual test to run @@ -118,6 +121,7 @@ func TestAttachDetach(t *testing.T) { diskCheckError := errors.New("Fake DiskIsAttached error") diskPathError := errors.New("Fake GetAttachmentDiskPath error") disksCheckError := errors.New("Fake DisksAreAttached error") + operationFinishTimeout := errors.New("Fake waitOperationFinished error") tests := []testcase{ // Successful Attach call { @@ -247,10 +251,11 @@ func TestAttachDetach(t *testing.T) { // Detach succeeds { - name: "Detach_Positive", - instanceID: instanceID, - diskIsAttached: diskIsAttachedCall{instanceID, volumeID, true, nil}, - detach: detachCall{instanceID, volumeID, nil}, + name: "Detach_Positive", + instanceID: instanceID, + operationPending: operationPendingCall{volumeID, false, done, nil}, + diskIsAttached: diskIsAttachedCall{instanceID, volumeID, true, nil}, + detach: detachCall{instanceID, volumeID, nil}, test: func(testcase *testcase) (string, error) { detacher := newDetacher(testcase) return "", detacher.Detach(volumeID, nodeName) @@ -259,9 +264,10 @@ func TestAttachDetach(t *testing.T) { // Disk is already detached { - name: "Detach_Positive_AlreadyDetached", - instanceID: instanceID, - diskIsAttached: diskIsAttachedCall{instanceID, volumeID, false, nil}, + name: "Detach_Positive_AlreadyDetached", + instanceID: instanceID, + operationPending: operationPendingCall{volumeID, false, done, nil}, + diskIsAttached: diskIsAttachedCall{instanceID, volumeID, false, nil}, test: func(testcase *testcase) (string, error) { detacher := newDetacher(testcase) return "", detacher.Detach(volumeID, nodeName) @@ -270,10 +276,11 @@ func TestAttachDetach(t *testing.T) { // Detach succeeds when DiskIsAttached fails { - name: "Detach_Positive_CheckFails", - instanceID: instanceID, - diskIsAttached: diskIsAttachedCall{instanceID, volumeID, false, diskCheckError}, - detach: detachCall{instanceID, volumeID, nil}, + name: "Detach_Positive_CheckFails", + instanceID: instanceID, + operationPending: operationPendingCall{volumeID, false, done, nil}, + diskIsAttached: diskIsAttachedCall{instanceID, volumeID, false, diskCheckError}, + detach: detachCall{instanceID, volumeID, nil}, test: func(testcase *testcase) (string, error) { detacher := newDetacher(testcase) return "", detacher.Detach(volumeID, nodeName) @@ -282,20 +289,36 @@ func TestAttachDetach(t *testing.T) { // Detach fails { - name: "Detach_Negative", - instanceID: instanceID, - diskIsAttached: diskIsAttachedCall{instanceID, volumeID, false, diskCheckError}, - detach: detachCall{instanceID, volumeID, detachError}, + name: "Detach_Negative", + instanceID: instanceID, + operationPending: operationPendingCall{volumeID, false, done, nil}, + diskIsAttached: diskIsAttachedCall{instanceID, volumeID, false, diskCheckError}, + detach: detachCall{instanceID, volumeID, detachError}, test: func(testcase *testcase) (string, error) { detacher := newDetacher(testcase) return "", detacher.Detach(volumeID, nodeName) }, expectedError: detachError, }, + + // // Disk is detaching + { + name: "Detach_Is_Detaching", + instanceID: instanceID, + operationPending: operationPendingCall{volumeID, true, pending, operationFinishTimeout}, + test: func(testcase *testcase) (string, error) { + detacher := newDetacher(testcase) + return "", detacher.Detach(volumeID, nodeName) + }, + expectedError: operationFinishTimeout, + }, } for _, testcase := range tests { testcase.t = t + // set VolumeIsNotAttached to test detach case, attach case ignore it + VolumeIsNotAttached = false + testcase.notAttached = &VolumeIsNotAttached result, err := testcase.test(&testcase) if err != testcase.expectedError { t.Errorf("%s failed: expected err=%q, got %q", testcase.name, testcase.expectedError, err) @@ -457,6 +480,8 @@ func (testcase *testcase) AttachDisk(instanceID, volumeID string) (string, error glog.V(4).Infof("AttachDisk call: %s, %s, returning %q, %v", volumeID, instanceID, expected.retDeviceName, expected.ret) + VolumeIsNotAttached = false + testcase.notAttached = &VolumeIsNotAttached return expected.retDeviceName, expected.ret } @@ -482,6 +507,8 @@ func (testcase *testcase) DetachDisk(instanceID, volumeID string) error { glog.V(4).Infof("DetachDisk call: %s, %s, returning %v", volumeID, instanceID, expected.ret) + VolumeIsNotAttached = true + testcase.notAttached = &VolumeIsNotAttached return expected.ret } @@ -500,6 +527,10 @@ func (testcase *testcase) OperationPending(diskName string) (bool, string, error func (testcase *testcase) DiskIsAttached(instanceID, volumeID string) (bool, error) { expected := &testcase.diskIsAttached + // If testcase call DetachDisk*, return false + if *testcase.notAttached == true { + return false, nil + } if expected.volumeID == "" && expected.instanceID == "" { // testcase.diskIsAttached looks uninitialized, test did not expect to