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