From 2267588d95cac3e0e9406b31c5accb1b2f6ffadc Mon Sep 17 00:00:00 2001 From: Anton Klautsan Date: Sat, 14 Jan 2017 20:07:22 +0000 Subject: [PATCH 1/2] Cinder volume attacher: use instanceID not NodeID --- pkg/volume/cinder/attacher.go | 33 ++++++++++++++++++++++++--------- 1 file changed, 24 insertions(+), 9 deletions(-) diff --git a/pkg/volume/cinder/attacher.go b/pkg/volume/cinder/attacher.go index 594895013c7..dccb1584c04 100644 --- a/pkg/volume/cinder/attacher.go +++ b/pkg/volume/cinder/attacher.go @@ -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 @@ -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) if err != nil { // Log error and continue with attach glog.Errorf( @@ -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 +} From 084d801e0a4d6cdf6eec20941cba7b26b65325b1 Mon Sep 17 00:00:00 2001 From: Anton Klautsan Date: Wed, 18 Jan 2017 01:50:49 +0000 Subject: [PATCH 2/2] Add unit-tests for DisksAreAttached --- pkg/volume/cinder/attacher_test.go | 142 +++++++++++++++++++++++++---- 1 file changed, 126 insertions(+), 16 deletions(-) diff --git a/pkg/volume/cinder/attacher_test.go b/pkg/volume/cinder/attacher_test.go index 17d6ec24266..d45b99784d2 100644 --- a/pkg/volume/cinder/attacher_test.go +++ b/pkg/volume/cinder/attacher_test.go @@ -18,6 +18,7 @@ package cinder import ( "errors" + "reflect" "testing" "k8s.io/kubernetes/pkg/api/v1" @@ -25,6 +26,9 @@ import ( "k8s.io/kubernetes/pkg/volume" volumetest "k8s.io/kubernetes/pkg/volume/testing" + "fmt" + "sort" + "github.com/golang/glog" "k8s.io/apimachinery/pkg/types" ) @@ -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 { @@ -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 @@ -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 @@ -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 @@ -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 + }, + 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", @@ -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 { @@ -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, @@ -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 @@ -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