diff --git a/pkg/volume/cinder/attacher.go b/pkg/volume/cinder/attacher.go index 8d976ca4a1d..7c6cf470bc4 100644 --- a/pkg/volume/cinder/attacher.go +++ b/pkg/volume/cinder/attacher.go @@ -46,6 +46,9 @@ const ( operationFinishInitDealy = 1 * time.Second operationFinishFactor = 1.1 operationFinishSteps = 10 + diskAttachInitDealy = 1 * time.Second + diskAttachFactor = 1.2 + diskAttachSteps = 15 diskDetachInitDealy = 1 * time.Second diskDetachFactor = 1.2 diskDetachSteps = 13 @@ -67,6 +70,53 @@ func (plugin *cinderPlugin) GetDeviceMountRefs(deviceMountPath string) ([]string return mount.GetMountRefs(mounter, deviceMountPath) } +func (attacher *cinderDiskAttacher) 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 = attacher.cinderProvider.OperationPending(volumeID) + if err != nil { + return false, err + } + return !pending, nil + }) + + if err == wait.ErrWaitTimeout { + err = fmt.Errorf("Volume %q is %s, can't finish within the alloted time", volumeID, volumeStatus) + } + + return err +} + +func (attacher *cinderDiskAttacher) waitDiskAttached(instanceID, volumeID string) error { + backoff := wait.Backoff{ + Duration: diskAttachInitDealy, + Factor: diskAttachFactor, + Steps: diskAttachSteps, + } + + err := wait.ExponentialBackoff(backoff, func() (bool, error) { + attached, err := attacher.cinderProvider.DiskIsAttached(instanceID, volumeID) + if err != nil { + return false, err + } + return attached, nil + }) + + if err == wait.ErrWaitTimeout { + err = fmt.Errorf("Volume %q failed to be attached within the alloted time", volumeID) + } + + return err +} + func (attacher *cinderDiskAttacher) Attach(spec *volume.Spec, nodeName types.NodeName) (string, error) { volumeSource, _, err := getVolumeSource(spec) if err != nil { @@ -80,14 +130,9 @@ func (attacher *cinderDiskAttacher) Attach(spec *volume.Spec, nodeName types.Nod return "", err } - pending, volumeStatus, err := attacher.cinderProvider.OperationPending(volumeID) - if err != nil { + if err := attacher.waitOperationFinished(volumeID); err != nil { return "", err } - if pending { - glog.Infof("status of volume %q is %s, wait it to be finished", volumeID, volumeStatus) - return "", nil - } attached, err := attacher.cinderProvider.DiskIsAttached(instanceID, volumeID) if err != nil { @@ -103,6 +148,10 @@ func (attacher *cinderDiskAttacher) Attach(spec *volume.Spec, nodeName types.Nod } else { _, err = attacher.cinderProvider.AttachDisk(instanceID, volumeID) if err == nil { + if err = attacher.waitDiskAttached(instanceID, volumeID); err != nil { + glog.Errorf("Error waiting for volume %q to be attached from node %q: %v", volumeID, nodeName, err) + return "", err + } glog.Infof("Attach operation successful: volume %q attached to instance %q.", volumeID, instanceID) } else { glog.Infof("Attach volume %q to instance %q failed with: %v", volumeID, instanceID, err) @@ -110,20 +159,11 @@ func (attacher *cinderDiskAttacher) Attach(spec *volume.Spec, nodeName types.Nod } } - // When volume's status is 'attaching', DiskIsAttached() return , and can't get device path. - // We should get device path next time and do not return err. - devicePath := "" - pending, _, err = attacher.cinderProvider.OperationPending(volumeID) + devicePath, err := attacher.cinderProvider.GetAttachmentDiskPath(instanceID, volumeID) if err != nil { + glog.Infof("Can not get device path of volume %q which be attached to instance %q, failed with: %v", volumeID, instanceID, err) return "", err } - if !pending { - devicePath, err = attacher.cinderProvider.GetAttachmentDiskPath(instanceID, volumeID) - if err != nil { - glog.Infof("Can not get device path of volume %q which be attached to instance %q, failed with: %v", volumeID, instanceID, err) - return "", err - } - } return devicePath, nil } @@ -286,15 +326,10 @@ func (detacher *cinderDiskDetacher) waitOperationFinished(volumeID string) 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 { + if err != nil { return false, err } + return !pending, nil }) if err == wait.ErrWaitTimeout { @@ -313,15 +348,10 @@ func (detacher *cinderDiskDetacher) waitDiskDetached(instanceID, volumeID string 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 { + if err != nil { return false, err } + return !attached, nil }) if err == wait.ErrWaitTimeout { diff --git a/pkg/volume/cinder/attacher_test.go b/pkg/volume/cinder/attacher_test.go index 534ee57c864..ee4373ebc8b 100644 --- a/pkg/volume/cinder/attacher_test.go +++ b/pkg/volume/cinder/attacher_test.go @@ -33,10 +33,13 @@ import ( "k8s.io/apimachinery/pkg/types" ) -const VolumeStatusPending = "pending" -const VolumeStatusDone = "done" +const ( + VolumeStatusPending = "pending" + VolumeStatusDone = "done" +) -var VolumeIsNotAttached = true +var attachStatus = "Attach" +var detachStatus = "Detach" func TestGetDeviceName_Volume(t *testing.T) { plugin := newPlugin() @@ -98,7 +101,7 @@ type testcase struct { disksAreAttached disksAreAttachedCall diskPath diskPathCall t *testing.T - notAttached *bool + attachOrDetach *string instanceID string // Actual test to run @@ -156,28 +159,12 @@ func TestAttachDetach(t *testing.T) { { name: "Attach_is_attaching", instanceID: instanceID, - operationPending: operationPendingCall{volumeID, true, pending, nil}, - diskIsAttached: diskIsAttachedCall{instanceID, volumeID, false, diskCheckError}, + operationPending: operationPendingCall{volumeID, true, pending, operationFinishTimeout}, test: func(testcase *testcase) (string, error) { attacher := newAttacher(testcase) return attacher.Attach(spec, nodeName) }, - expectedResult: "", - }, - - // DiskIsAttached fails and Attach succeeds - { - name: "Attach_Positive_CheckFails", - instanceID: instanceID, - operationPending: operationPendingCall{volumeID, false, done, nil}, - diskIsAttached: diskIsAttachedCall{instanceID, volumeID, false, diskCheckError}, - attach: attachCall{instanceID, volumeID, "", nil}, - diskPath: diskPathCall{instanceID, volumeID, "/dev/sda", nil}, - test: func(testcase *testcase) (string, error) { - attacher := newAttacher(testcase) - return attacher.Attach(spec, nodeName) - }, - expectedResult: "/dev/sda", + expectedError: operationFinishTimeout, }, // Attach call fails @@ -199,7 +186,7 @@ func TestAttachDetach(t *testing.T) { name: "Attach_Negative_DiskPatchFails", instanceID: instanceID, operationPending: operationPendingCall{volumeID, false, done, nil}, - diskIsAttached: diskIsAttachedCall{instanceID, volumeID, false, diskCheckError}, + diskIsAttached: diskIsAttachedCall{instanceID, volumeID, false, nil}, attach: attachCall{instanceID, volumeID, "", nil}, diskPath: diskPathCall{instanceID, volumeID, "", diskPathError}, test: func(testcase *testcase) (string, error) { @@ -316,9 +303,8 @@ func TestAttachDetach(t *testing.T) { for _, testcase := range tests { testcase.t = t - // set VolumeIsNotAttached to test detach case, attach case ignore it - VolumeIsNotAttached = false - testcase.notAttached = &VolumeIsNotAttached + attachOrDetach := "" + testcase.attachOrDetach = &attachOrDetach result, err := testcase.test(&testcase) if err != testcase.expectedError { t.Errorf("%s failed: expected err=%q, got %q", testcase.name, testcase.expectedError, err) @@ -480,8 +466,7 @@ 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 + testcase.attachOrDetach = &attachStatus return expected.retDeviceName, expected.ret } @@ -507,8 +492,7 @@ 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 + testcase.attachOrDetach = &detachStatus return expected.ret } @@ -528,10 +512,15 @@ 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 { + if *testcase.attachOrDetach == detachStatus { return false, nil } + // If testcase call AttachDisk*, return true + if *testcase.attachOrDetach == attachStatus { + return true, nil + } + if expected.volumeID == "" && expected.instanceID == "" { // testcase.diskIsAttached looks uninitialized, test did not expect to // call DiskIsAttached diff --git a/pkg/volume/util/operationexecutor/operation_generator.go b/pkg/volume/util/operationexecutor/operation_generator.go index a558713af4e..419bf1e6adc 100644 --- a/pkg/volume/util/operationexecutor/operation_generator.go +++ b/pkg/volume/util/operationexecutor/operation_generator.go @@ -266,9 +266,6 @@ func (og *operationGenerator) GenerateAttachVolumeFunc( og.recorder.Eventf(pod, v1.EventTypeWarning, kevents.FailedMountVolume, eventErr.Error()) } return detailedErr - } else if devicePath == "" { - // do nothing and get device path next time. - return nil } glog.Infof(volumeToAttach.GenerateMsgDetailed("AttachVolume.Attach succeeded", ""))