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

Cinder volume attacher: use instanceID instead of NodeID when verifying attachment #39998

Merged
merged 2 commits into from
Feb 10, 2017
Merged

Cinder volume attacher: use instanceID instead of NodeID when verifying attachment #39998

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2267588
Cinder volume attacher: use instanceID not NodeID
DukeXar Jan 14, 2017
084d801
Add unit-tests for DisksAreAttached
DukeXar Jan 18, 2017
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
33 changes: 24 additions & 9 deletions pkg/volume/cinder/attacher.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,17 +68,11 @@ func (attacher *cinderDiskAttacher) Attach(spec *volume.Spec, nodeName types.Nod

volumeID := volumeSource.VolumeID

instances, res := attacher.cinderProvider.Instances()
if !res {
return "", fmt.Errorf("failed to list openstack instances")
}
instanceid, err := instances.InstanceID(nodeName)
instanceid, err := attacher.nodeInstanceID(nodeName)
if err != nil {
return "", err
}
if ind := strings.LastIndex(instanceid, "/"); ind >= 0 {
instanceid = instanceid[(ind + 1):]
}

attached, err := attacher.cinderProvider.DiskIsAttached(volumeID, instanceid)
if err != nil {
// Log error and continue with attach
Expand Down Expand Up @@ -124,7 +118,13 @@ func (attacher *cinderDiskAttacher) VolumesAreAttached(specs []*volume.Spec, nod
volumesAttachedCheck[spec] = true
volumeSpecMap[volumeSource.VolumeID] = spec
}
attachedResult, err := attacher.cinderProvider.DisksAreAttached(volumeIDList, string(nodeName))

instanceid, err := attacher.nodeInstanceID(nodeName)
if err != nil {
return volumesAttachedCheck, err
}

attachedResult, err := attacher.cinderProvider.DisksAreAttached(volumeIDList, instanceid)
Copy link
Contributor

@resouer resouer Jan 17, 2017 •
edited

Choose a reason for hiding this comment

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

It would be better to have a unit test to verify this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

if err != nil {
// Log error and continue with attach
glog.Errorf(
Expand Down Expand Up @@ -284,3 +284,18 @@ func (detacher *cinderDiskDetacher) Detach(deviceMountPath string, nodeName type
func (detacher *cinderDiskDetacher) UnmountDevice(deviceMountPath string) error {
return volumeutil.UnmountPath(deviceMountPath, detacher.mounter)
}

func (attacher *cinderDiskAttacher) nodeInstanceID(nodeName types.NodeName) (string, error) {
instances, res := attacher.cinderProvider.Instances()
if !res {
return "", fmt.Errorf("failed to list openstack instances")
}
instanceid, err := instances.InstanceID(nodeName)
if err != nil {
return "", err
}
if ind := strings.LastIndex(instanceid, "/"); ind >= 0 {
instanceid = instanceid[(ind + 1):]
}
return instanceid, nil
}
142 changes: 126 additions & 16 deletions pkg/volume/cinder/attacher_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,17 @@ package cinder

import (
"errors"
"reflect"
"testing"

"k8s.io/kubernetes/pkg/api/v1"
"k8s.io/kubernetes/pkg/cloudprovider"
"k8s.io/kubernetes/pkg/volume"
volumetest "k8s.io/kubernetes/pkg/volume/testing"

"fmt"
"sort"

"github.com/golang/glog"
"k8s.io/apimachinery/pkg/types"
)
Expand Down Expand Up @@ -82,30 +86,32 @@ func TestGetDeviceMountPath(t *testing.T) {
type testcase struct {
name string
// For fake GCE:
attach attachCall
detach detachCall
diskIsAttached diskIsAttachedCall
diskPath diskPathCall
t *testing.T
attach attachCall
detach detachCall
diskIsAttached diskIsAttachedCall
disksAreAttached disksAreAttachedCall
diskPath diskPathCall
t *testing.T

instanceID string
// Actual test to run
test func(test *testcase) (string, error)
// Expected return of the test
expectedDevice string
expectedResult string
expectedError error
}

func TestAttachDetach(t *testing.T) {
diskName := "disk"
instanceID := "instance"
nodeName := types.NodeName(instanceID)
nodeName := types.NodeName("nodeName")
readOnly := false
spec := createVolSpec(diskName, readOnly)
attachError := errors.New("Fake attach error")
detachError := errors.New("Fake detach error")
diskCheckError := errors.New("Fake DiskIsAttached error")
diskPathError := errors.New("Fake GetAttachmentDiskPath error")
disksCheckError := errors.New("Fake DisksAreAttached error")
tests := []testcase{
// Successful Attach call
{
Expand All @@ -118,7 +124,7 @@ func TestAttachDetach(t *testing.T) {
attacher := newAttacher(testcase)
return attacher.Attach(spec, nodeName)
},
expectedDevice: "/dev/sda",
expectedResult: "/dev/sda",
},

// Disk is already attached
Expand All @@ -131,7 +137,7 @@ func TestAttachDetach(t *testing.T) {
attacher := newAttacher(testcase)
return attacher.Attach(spec, nodeName)
},
expectedDevice: "/dev/sda",
expectedResult: "/dev/sda",
},

// DiskIsAttached fails and Attach succeeds
Expand All @@ -145,7 +151,7 @@ func TestAttachDetach(t *testing.T) {
attacher := newAttacher(testcase)
return attacher.Attach(spec, nodeName)
},
expectedDevice: "/dev/sda",
expectedResult: "/dev/sda",
},

// Attach call fails
Expand Down Expand Up @@ -175,6 +181,46 @@ func TestAttachDetach(t *testing.T) {
expectedError: diskPathError,
},

// Successful VolumesAreAttached call, attached
{
name: "VolumesAreAttached_Positive",
instanceID: instanceID,
disksAreAttached: disksAreAttachedCall{[]string{diskName}, instanceID, map[string]bool{diskName: true}, nil},
test: func(testcase *testcase) (string, error) {
attacher := newAttacher(testcase)
attachments, err := attacher.VolumesAreAttached([]*volume.Spec{spec}, nodeName)
return serializeAttachments(attachments), err
Copy link
Contributor Author

Choose a reason for hiding this comment

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

serializeAttachments

Does not look like the best thing to do, open for suggestions on what is idiomatic in Go

},
expectedResult: serializeAttachments(map[*volume.Spec]bool{spec: true}),
},

// Successful VolumesAreAttached call, not attached
{
name: "VolumesAreAttached_Negative",
instanceID: instanceID,
disksAreAttached: disksAreAttachedCall{[]string{diskName}, instanceID, map[string]bool{diskName: false}, nil},
test: func(testcase *testcase) (string, error) {
attacher := newAttacher(testcase)
attachments, err := attacher.VolumesAreAttached([]*volume.Spec{spec}, nodeName)
return serializeAttachments(attachments), err
},
expectedResult: serializeAttachments(map[*volume.Spec]bool{spec: false}),
},

// Treat as attached when DisksAreAttached call fails
{
name: "VolumesAreAttached_CinderFailed",
instanceID: instanceID,
disksAreAttached: disksAreAttachedCall{[]string{diskName}, instanceID, nil, disksCheckError},
test: func(testcase *testcase) (string, error) {
attacher := newAttacher(testcase)
attachments, err := attacher.VolumesAreAttached([]*volume.Spec{spec}, nodeName)
return serializeAttachments(attachments), err
},
expectedResult: serializeAttachments(map[*volume.Spec]bool{spec: true}),
expectedError: disksCheckError,
},

// Detach succeeds
{
name: "Detach_Positive",
Expand Down Expand Up @@ -226,17 +272,51 @@ func TestAttachDetach(t *testing.T) {

for _, testcase := range tests {
testcase.t = t
device, err := testcase.test(&testcase)
result, err := testcase.test(&testcase)
if err != testcase.expectedError {
t.Errorf("%s failed: expected err=%q, got %q", testcase.name, testcase.expectedError.Error(), err.Error())
t.Errorf("%s failed: expected err=%q, got %q", testcase.name, testcase.expectedError, err)
}
if device != testcase.expectedDevice {
t.Errorf("%s failed: expected device=%q, got %q", testcase.name, testcase.expectedDevice, device)
if result != testcase.expectedResult {
t.Errorf("%s failed: expected result=%q, got %q", testcase.name, testcase.expectedResult, result)
}
t.Logf("Test %q succeeded", testcase.name)
}
}

type volumeAttachmentFlag struct {
diskName string
attached bool
}

type volumeAttachmentFlags []volumeAttachmentFlag

func (va volumeAttachmentFlags) Len() int {
return len(va)
}

func (va volumeAttachmentFlags) Swap(i, j int) {
va[i], va[j] = va[j], va[i]
}

func (va volumeAttachmentFlags) Less(i, j int) bool {
if va[i].diskName < va[j].diskName {
return true
}
if va[i].diskName > va[j].diskName {
return false
}
return va[j].attached
}

func serializeAttachments(attachments map[*volume.Spec]bool) string {
var attachmentFlags volumeAttachmentFlags
for spec, attached := range attachments {
attachmentFlags = append(attachmentFlags, volumeAttachmentFlag{spec.Name(), attached})
}
sort.Sort(attachmentFlags)
return fmt.Sprint(attachmentFlags)
}

// newPlugin creates a new gcePersistentDiskPlugin with fake cloud, NewAttacher
// and NewDetacher won't work.
func newPlugin() *cinderPlugin {
Expand All @@ -263,6 +343,7 @@ func newDetacher(testcase *testcase) *cinderDiskDetacher {
func createVolSpec(name string, readOnly bool) *volume.Spec {
return &volume.Spec{
Volume: &v1.Volume{
Name: name,
VolumeSource: v1.VolumeSource{
Cinder: &v1.CinderVolumeSource{
VolumeID: name,
Expand Down Expand Up @@ -315,6 +396,13 @@ type diskPathCall struct {
ret error
}

type disksAreAttachedCall struct {
diskNames []string
instanceID string
areAttached map[string]bool
ret error
}

func (testcase *testcase) AttachDisk(instanceID string, diskName string) (string, error) {
expected := &testcase.attach

Expand Down Expand Up @@ -442,8 +530,30 @@ func (testcase *testcase) Instances() (cloudprovider.Instances, bool) {
return &instances{testcase.instanceID}, true
}

func (testcase *testcase) DisksAreAttached(diskNames []string, nodeName string) (map[string]bool, error) {
return nil, errors.New("Not implemented")
func (testcase *testcase) DisksAreAttached(diskNames []string, instanceID string) (map[string]bool, error) {
expected := &testcase.disksAreAttached

areAttached := make(map[string]bool)

if len(expected.diskNames) == 0 && expected.instanceID == "" {
// testcase.diskNames looks uninitialized, test did not expect to call DisksAreAttached
testcase.t.Errorf("Unexpected DisksAreAttached call!")
return areAttached, errors.New("Unexpected DisksAreAttached call")
}

if !reflect.DeepEqual(expected.diskNames, diskNames) {
testcase.t.Errorf("Unexpected DisksAreAttached call: expected diskNames %v, got %v", expected.diskNames, diskNames)
return areAttached, errors.New("Unexpected DisksAreAttached call: wrong diskName")
}

if expected.instanceID != instanceID {
testcase.t.Errorf("Unexpected DisksAreAttached call: expected instanceID %s, got %s", expected.instanceID, instanceID)
return areAttached, errors.New("Unexpected DisksAreAttached call: wrong instanceID")
}

glog.V(4).Infof("DisksAreAttached call: %v, %s, returning %v, %v", diskNames, instanceID, expected.areAttached, expected.ret)

return expected.areAttached, expected.ret
}

// Implementation of fake cloudprovider.Instances
Expand Down