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

Check volume's status before detaching volume #46182

Merged
merged 1 commit into from
May 31, 2017
Merged

Check volume's status before detaching volume #46182

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
1 change: 1 addition & 0 deletions pkg/volume/cinder/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
],
)

Expand Down
78 changes: 75 additions & 3 deletions pkg/volume/cinder/attacher.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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) {
Expand Down Expand Up @@ -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()
Expand All @@ -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
Expand All @@ -293,10 +361,14 @@ func (detacher *cinderDiskDetacher) Detach(deviceMountPath string, nodeName type
}

if err = detacher.cinderProvider.DetachDisk(instanceID, volumeID); err != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if we return timeout error, then the detach controller will retry again later. Do we still need to check if operation is pending? Or can the cinder provider handle duplicate detach calls?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the cinder provider can't handle duplicate detach, we need to check if operation is pending. I update it soon.

glog.Errorf("Error detaching volume %q: %v", volumeID, err)
glog.Errorf("Error detaching volume %q from node %q: %v", volumeID, nodeName, err)
return err
}
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 instance %q", volumeID, instanceID)
glog.Infof("detached volume %q from node %q", volumeID, nodeName)
return nil
}

Expand Down
61 changes: 46 additions & 15 deletions pkg/volume/cinder/attacher_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -96,6 +98,7 @@ type testcase struct {
disksAreAttached disksAreAttachedCall
diskPath diskPathCall
t *testing.T
notAttached *bool

instanceID string
// Actual test to run
Expand All @@ -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
{
Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -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
}

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

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