Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Waiting attach operation to be finished rather than returning nil #47018

Merged
merged 1 commit into from Jun 7, 2017
Merged

Waiting attach operation to be finished rather than returning nil #47018

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
92 changes: 61 additions & 31 deletions pkg/volume/cinder/attacher.go
View file
Open in desktop
Expand Up @@ -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
Expand All @@ -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 {
Expand All @@ -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 {
Expand All @@ -103,27 +148,22 @@ 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)
return "", err
}
}

// When volume's status is 'attaching', DiskIsAttached() return <false, nil>, 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
}
Expand Down Expand Up @@ -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 {
Expand All @@ -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 {
Expand Down
51 changes: 20 additions & 31 deletions pkg/volume/cinder/attacher_test.go
View file
Open in desktop
Expand Up @@ -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()
Expand Down Expand Up @@ -98,7 +101,7 @@ type testcase struct {
disksAreAttached disksAreAttachedCall
diskPath diskPathCall
t *testing.T
notAttached *bool
attachOrDetach *string

instanceID string
// Actual test to run
Expand Down Expand Up @@ -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},
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},
operationPending: operationPendingCall{volumeID, true, pending, operationFinishTimeout},
test: func(testcase *testcase) (string, error) {
attacher := newAttacher(testcase)
return attacher.Attach(spec, nodeName)
},
expectedResult: "/dev/sda",
expectedError: operationFinishTimeout,
},

// Attach call fails
Expand All @@ -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) {
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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
}

Expand All @@ -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
}

Expand All @@ -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
Expand Down
3 changes: 0 additions & 3 deletions pkg/volume/util/operationexecutor/operation_generator.go
View file
Open in desktop
Expand Up @@ -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", ""))
Expand Down