Merge pull request #39998 from DukeXar/cinder_instance_id

Automatic merge from submit-queue (batch tested with PRs 41246, 39998)

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

**What this PR does / why we need it**: Cinder volume attacher incorrectly uses NodeID instead of openstack instance id, so that reconciliation fails.

**Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes #39978 

**Special notes for your reviewer**:

**Release note**:

```release-note
```

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()
+	instanceid, err := attacher.nodeInstanceID(nodeName)
-	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):]
-	}
 	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
+}

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
+	attach           attachCall
-	detach         detachCall
+	detach           detachCall
-	diskIsAttached diskIsAttachedCall
+	diskIsAttached   diskIsAttachedCall
-	diskPath       diskPathCall
+	disksAreAttached disksAreAttachedCall
-	t              *testing.T
+	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 {
+		if result != testcase.expectedResult {
-			t.Errorf("%s failed: expected device=%q, got %q", testcase.name, testcase.expectedDevice, device)
+			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) {
+func (testcase *testcase) DisksAreAttached(diskNames []string, instanceID string) (map[string]bool, error) {
-	return nil, errors.New("Not implemented")
+	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