move detach out of os volumes attach

add test

add test

fix bazel

fix tests

change loglevel, remove else statement

pkg/cloudprovider/providers/openstack/openstack_volumes.go

@@ -26,6 +26,7 @@ import (
 	"time"
 
 	"k8s.io/apimachinery/pkg/api/resource"
+	"k8s.io/apimachinery/pkg/types"
 	k8s_volume "k8s.io/kubernetes/pkg/volume"
 	volumeutil "k8s.io/kubernetes/pkg/volume/util"
 
@@ -319,33 +320,18 @@ func (os *OpenStack) AttachDisk(instanceID, volumeID string) (string, error) {
 		if instanceID == volume.AttachedServerId {
 			glog.V(4).Infof("Disk %s is already attached to instance %s", volumeID, instanceID)
 			return volume.ID, nil
-		} else {
-			nodeName, err := os.GetNodeNameByID(volume.AttachedServerId)
-			attachErr := fmt.Sprintf("disk %s path %s is attached to a different instance (%s)", volumeID, volume.AttachedDevice, volume.AttachedServerId)
-			if err != nil {
-				glog.Error(attachErr)
-				return "", errors.New(attachErr)
-			}
-			// using volume.AttachedDevice may cause problems because cinder does not report device path correctly see issue #33128
-			devicePath := volume.AttachedDevice
-			danglingErr := volumeutil.NewDanglingError(attachErr, nodeName, devicePath)
-			glog.V(4).Infof("volume %s is already attached to node %s path %s", volumeID, nodeName, devicePath)
-			// check special case, if node is deleted from cluster but exist still in openstack
-			// we need to check can we detach the cinder, node is deleted from cluster if state is not ACTIVE
-			srv, err := getServerByName(cClient, nodeName, false)
-			if err != nil {
-				return "", err
-			}
-			if srv.Status != "ACTIVE" {
-				err = os.DetachDisk(volume.AttachedServerId, volumeID)
-				if err != nil {
-					glog.Error(err)
-					return "", err
-				}
-				glog.V(4).Infof("detached volume %s node state was %s", volumeID, srv.Status)
-			}
-			return "", danglingErr
 		}
+		nodeName, err := os.GetNodeNameByID(volume.AttachedServerId)
+		attachErr := fmt.Sprintf("disk %s path %s is attached to a different instance (%s)", volumeID, volume.AttachedDevice, volume.AttachedServerId)
+		if err != nil {
+			glog.Error(attachErr)
+			return "", errors.New(attachErr)
+		}
+		// using volume.AttachedDevice may cause problems because cinder does not report device path correctly see issue #33128
+		devicePath := volume.AttachedDevice
+		danglingErr := volumeutil.NewDanglingError(attachErr, nodeName, devicePath)
+		glog.V(2).Infof("Found dangling volume %s attached to node %s", volumeID, nodeName)
+		return "", danglingErr
 	}
 
 	startTime := time.Now()
@@ -605,6 +591,9 @@ func (os *OpenStack) GetAttachmentDiskPath(instanceID, volumeID string) (string,
 
 // DiskIsAttached queries if a volume is attached to a compute instance
 func (os *OpenStack) DiskIsAttached(instanceID, volumeID string) (bool, error) {
+	if instanceID == "" {
+		glog.Warningf("calling DiskIsAttached with empty instanceid: %s %s", instanceID, volumeID)
+	}
 	volume, err := os.getVolume(volumeID)
 	if err != nil {
 		return false, err
@@ -613,6 +602,29 @@ func (os *OpenStack) DiskIsAttached(instanceID, volumeID string) (bool, error) {
 	return instanceID == volume.AttachedServerId, nil
 }
 
+// DiskIsAttachedByName queries if a volume is attached to a compute instance by name
+func (os *OpenStack) DiskIsAttachedByName(nodeName types.NodeName, volumeID string) (bool, string, error) {
+	cClient, err := os.NewComputeV2()
+	if err != nil {
+		return false, "", err
+	}
+	srv, err := getServerByName(cClient, nodeName, false)
+	if err != nil {
+		if err == ErrNotFound {
+			// instance not found anymore in cloudprovider, assume that cinder is detached
+			return false, "", nil
+		} else {
+			return false, "", err
+		}
+	}
+	instanceID := "/" + srv.ID
+	if ind := strings.LastIndex(instanceID, "/"); ind >= 0 {
+		instanceID = instanceID[(ind + 1):]
+	}
+	attached, err := os.DiskIsAttached(instanceID, volumeID)
+	return attached, instanceID, err
+}
+
 // DisksAreAttached queries if a list of volumes are attached to a compute instance
 func (os *OpenStack) DisksAreAttached(instanceID string, volumeIDs []string) (map[string]bool, error) {
 	attached := make(map[string]bool)
@@ -627,6 +639,32 @@ func (os *OpenStack) DisksAreAttached(instanceID string, volumeIDs []string) (ma
 	return attached, nil
 }
 
+// DisksAreAttachedByName queries if a list of volumes are attached to a compute instance by name
+func (os *OpenStack) DisksAreAttachedByName(nodeName types.NodeName, volumeIDs []string) (map[string]bool, error) {
+	attached := make(map[string]bool)
+	cClient, err := os.NewComputeV2()
+	if err != nil {
+		return attached, err
+	}
+	srv, err := getServerByName(cClient, nodeName, false)
+	if err != nil {
+		if err == ErrNotFound {
+			// instance not found anymore, mark all volumes as detached
+			for _, volumeID := range volumeIDs {
+				attached[volumeID] = false
+			}
+			return attached, nil
+		} else {
+			return attached, err
+		}
+	}
+	instanceID := "/" + srv.ID
+	if ind := strings.LastIndex(instanceID, "/"); ind >= 0 {
+		instanceID = instanceID[(ind + 1):]
+	}
+	return os.DisksAreAttached(instanceID, volumeIDs)
+}
+
 // diskIsUsed returns true a disk is attached to any node.
 func (os *OpenStack) diskIsUsed(volumeID string) (bool, error) {
 	volume, err := os.getVolume(volumeID)

pkg/volume/cinder/attacher.go

@@ -27,7 +27,6 @@ import (
 	"k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/wait"
-	"k8s.io/kubernetes/pkg/cloudprovider"
 	"k8s.io/kubernetes/pkg/util/mount"
 	"k8s.io/kubernetes/pkg/volume"
 	volumeutil "k8s.io/kubernetes/pkg/volume/util"
@@ -187,23 +186,7 @@ func (attacher *cinderDiskAttacher) VolumesAreAttached(specs []*volume.Spec, nod
 		volumeSpecMap[volumeSource.VolumeID] = spec
 	}
 
-	instanceID, err := attacher.nodeInstanceID(nodeName)
+	attachedResult, err := attacher.cinderProvider.DisksAreAttachedByName(nodeName, volumeIDList)
-	if err != nil {
-		if err == cloudprovider.InstanceNotFound {
-			// If node doesn't exist, OpenStack Nova will assume the volumes are not attached to it.
-			// Mark the volumes as detached and return false without error.
-			glog.Warningf("VolumesAreAttached: node %q does not exist.", nodeName)
-			for spec := range volumesAttachedCheck {
-				volumesAttachedCheck[spec] = false
-			}
-
-			return volumesAttachedCheck, nil
-		}
-
-		return volumesAttachedCheck, err
-	}
-
-	attachedResult, err := attacher.cinderProvider.DisksAreAttached(instanceID, volumeIDList)
 	if err != nil {
 		// Log error and continue with attach
 		glog.Errorf(
@@ -381,20 +364,10 @@ func (detacher *cinderDiskDetacher) waitDiskDetached(instanceID, volumeID string
 
 func (detacher *cinderDiskDetacher) Detach(volumeName string, nodeName types.NodeName) error {
 	volumeID := path.Base(volumeName)
-	instances, res := detacher.cinderProvider.Instances()
-	if !res {
-		return fmt.Errorf("failed to list openstack instances")
-	}
-	instanceID, err := instances.InstanceID(nodeName)
-	if ind := strings.LastIndex(instanceID, "/"); ind >= 0 {
-		instanceID = instanceID[(ind + 1):]
-	}
-
 	if err := detacher.waitOperationFinished(volumeID); err != nil {
 		return err
 	}
-
+	attached, instanceID, err := detacher.cinderProvider.DiskIsAttachedByName(nodeName, volumeID)
-	attached, err := detacher.cinderProvider.DiskIsAttached(instanceID, volumeID)
 	if err != nil {
 		// Log error and continue with detach
 		glog.Errorf(

pkg/volume/cinder/attacher_test.go

@@ -132,7 +132,7 @@ func TestAttachDetach(t *testing.T) {
 			name:             "Attach_Positive",
 			instanceID:       instanceID,
 			operationPending: operationPendingCall{volumeID, false, done, nil},
-			diskIsAttached:   diskIsAttachedCall{instanceID, volumeID, false, nil},
+			diskIsAttached:   diskIsAttachedCall{instanceID, nodeName, volumeID, false, nil},
 			attach:           attachCall{instanceID, volumeID, "", nil},
 			diskPath:         diskPathCall{instanceID, volumeID, "/dev/sda", nil},
 			test: func(testcase *testcase) (string, error) {
@@ -147,7 +147,7 @@ func TestAttachDetach(t *testing.T) {
 			name:             "Attach_Positive_AlreadyAttached",
 			instanceID:       instanceID,
 			operationPending: operationPendingCall{volumeID, false, done, nil},
-			diskIsAttached:   diskIsAttachedCall{instanceID, volumeID, true, nil},
+			diskIsAttached:   diskIsAttachedCall{instanceID, nodeName, volumeID, true, nil},
 			diskPath:         diskPathCall{instanceID, volumeID, "/dev/sda", nil},
 			test: func(testcase *testcase) (string, error) {
 				attacher := newAttacher(testcase)
@@ -173,7 +173,7 @@ func TestAttachDetach(t *testing.T) {
 			name:             "Attach_Negative",
 			instanceID:       instanceID,
 			operationPending: operationPendingCall{volumeID, false, done, nil},
-			diskIsAttached:   diskIsAttachedCall{instanceID, volumeID, false, diskCheckError},
+			diskIsAttached:   diskIsAttachedCall{instanceID, nodeName, volumeID, false, diskCheckError},
 			attach:           attachCall{instanceID, volumeID, "/dev/sda", attachError},
 			test: func(testcase *testcase) (string, error) {
 				attacher := newAttacher(testcase)
@@ -187,7 +187,7 @@ func TestAttachDetach(t *testing.T) {
 			name:             "Attach_Negative_DiskPatchFails",
 			instanceID:       instanceID,
 			operationPending: operationPendingCall{volumeID, false, done, nil},
-			diskIsAttached:   diskIsAttachedCall{instanceID, volumeID, false, nil},
+			diskIsAttached:   diskIsAttachedCall{instanceID, nodeName, volumeID, false, nil},
 			attach:           attachCall{instanceID, volumeID, "", nil},
 			diskPath:         diskPathCall{instanceID, volumeID, "", diskPathError},
 			test: func(testcase *testcase) (string, error) {
@@ -201,7 +201,7 @@ func TestAttachDetach(t *testing.T) {
 		{
 			name:             "VolumesAreAttached_Positive",
 			instanceID:       instanceID,
-			disksAreAttached: disksAreAttachedCall{instanceID, []string{volumeID}, map[string]bool{volumeID: true}, nil},
+			disksAreAttached: disksAreAttachedCall{instanceID, nodeName, []string{volumeID}, map[string]bool{volumeID: true}, nil},
 			test: func(testcase *testcase) (string, error) {
 				attacher := newAttacher(testcase)
 				attachments, err := attacher.VolumesAreAttached([]*volume.Spec{spec}, nodeName)
@@ -214,7 +214,7 @@ func TestAttachDetach(t *testing.T) {
 		{
 			name:             "VolumesAreAttached_Negative",
 			instanceID:       instanceID,
-			disksAreAttached: disksAreAttachedCall{instanceID, []string{volumeID}, map[string]bool{volumeID: false}, nil},
+			disksAreAttached: disksAreAttachedCall{instanceID, nodeName, []string{volumeID}, map[string]bool{volumeID: false}, nil},
 			test: func(testcase *testcase) (string, error) {
 				attacher := newAttacher(testcase)
 				attachments, err := attacher.VolumesAreAttached([]*volume.Spec{spec}, nodeName)
@@ -227,7 +227,7 @@ func TestAttachDetach(t *testing.T) {
 		{
 			name:             "VolumesAreAttached_CinderFailed",
 			instanceID:       instanceID,
-			disksAreAttached: disksAreAttachedCall{instanceID, []string{volumeID}, nil, disksCheckError},
+			disksAreAttached: disksAreAttachedCall{instanceID, nodeName, []string{volumeID}, nil, disksCheckError},
 			test: func(testcase *testcase) (string, error) {
 				attacher := newAttacher(testcase)
 				attachments, err := attacher.VolumesAreAttached([]*volume.Spec{spec}, nodeName)
@@ -242,7 +242,7 @@ func TestAttachDetach(t *testing.T) {
 			name:             "Detach_Positive",
 			instanceID:       instanceID,
 			operationPending: operationPendingCall{volumeID, false, done, nil},
-			diskIsAttached:   diskIsAttachedCall{instanceID, volumeID, true, nil},
+			diskIsAttached:   diskIsAttachedCall{instanceID, nodeName, volumeID, true, nil},
 			detach:           detachCall{instanceID, volumeID, nil},
 			test: func(testcase *testcase) (string, error) {
 				detacher := newDetacher(testcase)
@@ -255,7 +255,7 @@ func TestAttachDetach(t *testing.T) {
 			name:             "Detach_Positive_AlreadyDetached",
 			instanceID:       instanceID,
 			operationPending: operationPendingCall{volumeID, false, done, nil},
-			diskIsAttached:   diskIsAttachedCall{instanceID, volumeID, false, nil},
+			diskIsAttached:   diskIsAttachedCall{instanceID, nodeName, volumeID, false, nil},
 			test: func(testcase *testcase) (string, error) {
 				detacher := newDetacher(testcase)
 				return "", detacher.Detach(volumeID, nodeName)
@@ -267,7 +267,7 @@ func TestAttachDetach(t *testing.T) {
 			name:             "Detach_Positive_CheckFails",
 			instanceID:       instanceID,
 			operationPending: operationPendingCall{volumeID, false, done, nil},
-			diskIsAttached:   diskIsAttachedCall{instanceID, volumeID, false, diskCheckError},
+			diskIsAttached:   diskIsAttachedCall{instanceID, nodeName, volumeID, false, diskCheckError},
 			detach:           detachCall{instanceID, volumeID, nil},
 			test: func(testcase *testcase) (string, error) {
 				detacher := newDetacher(testcase)
@@ -280,7 +280,7 @@ func TestAttachDetach(t *testing.T) {
 			name:             "Detach_Negative",
 			instanceID:       instanceID,
 			operationPending: operationPendingCall{volumeID, false, done, nil},
-			diskIsAttached:   diskIsAttachedCall{instanceID, volumeID, false, diskCheckError},
+			diskIsAttached:   diskIsAttachedCall{instanceID, nodeName, volumeID, false, diskCheckError},
 			detach:           detachCall{instanceID, volumeID, detachError},
 			test: func(testcase *testcase) (string, error) {
 				detacher := newDetacher(testcase)
@@ -426,6 +426,7 @@ type operationPendingCall struct {
 
 type diskIsAttachedCall struct {
 	instanceID string
+	nodeName   types.NodeName
 	volumeID   string
 	isAttached bool
 	ret        error
@@ -440,6 +441,7 @@ type diskPathCall struct {
 
 type disksAreAttachedCall struct {
 	instanceID  string
+	nodeName    types.NodeName
 	volumeIDs   []string
 	areAttached map[string]bool
 	ret         error
@@ -572,6 +574,46 @@ func (testcase *testcase) ShouldTrustDevicePath() bool {
 	return true
 }
 
+func (testcase *testcase) DiskIsAttachedByName(nodeName types.NodeName, volumeID string) (bool, string, error) {
+	expected := &testcase.diskIsAttached
+	instanceID := expected.instanceID
+	// If testcase call DetachDisk*, return false
+	if *testcase.attachOrDetach == detachStatus {
+		return false, instanceID, nil
+	}
+
+	// If testcase call AttachDisk*, return true
+	if *testcase.attachOrDetach == attachStatus {
+		return true, instanceID, nil
+	}
+
+	if expected.nodeName != nodeName {
+		testcase.t.Errorf("Unexpected DiskIsAttachedByName call: expected nodename %s, got %s", expected.nodeName, nodeName)
+		return false, instanceID, errors.New("Unexpected DiskIsAttachedByName call: wrong nodename")
+	}
+
+	if expected.volumeID == "" && expected.instanceID == "" {
+		// testcase.diskIsAttached looks uninitialized, test did not expect to
+		// call DiskIsAttached
+		testcase.t.Errorf("Unexpected DiskIsAttachedByName call!")
+		return false, instanceID, errors.New("Unexpected DiskIsAttachedByName call!")
+	}
+
+	if expected.volumeID != volumeID {
+		testcase.t.Errorf("Unexpected DiskIsAttachedByName call: expected volumeID %s, got %s", expected.volumeID, volumeID)
+		return false, instanceID, errors.New("Unexpected DiskIsAttachedByName call: wrong volumeID")
+	}
+
+	if expected.instanceID != instanceID {
+		testcase.t.Errorf("Unexpected DiskIsAttachedByName call: expected instanceID %s, got %s", expected.instanceID, instanceID)
+		return false, instanceID, errors.New("Unexpected DiskIsAttachedByName call: wrong instanceID")
+	}
+
+	glog.V(4).Infof("DiskIsAttachedByName call: %s, %s, returning %v, %v", volumeID, nodeName, expected.isAttached, expected.instanceID, expected.ret)
+
+	return expected.isAttached, expected.instanceID, expected.ret
+}
+
 func (testcase *testcase) CreateVolume(name string, size int, vtype, availability string, tags *map[string]string) (string, string, bool, error) {
 	return "", "", false, errors.New("Not implemented")
 }
@@ -626,6 +668,36 @@ func (testcase *testcase) DisksAreAttached(instanceID string, volumeIDs []string
 	return expected.areAttached, expected.ret
 }
 
+func (testcase *testcase) DisksAreAttachedByName(nodeName types.NodeName, volumeIDs []string) (map[string]bool, error) {
+	expected := &testcase.disksAreAttached
+	areAttached := make(map[string]bool)
+
+	instanceID := expected.instanceID
+	if expected.nodeName != nodeName {
+		testcase.t.Errorf("Unexpected DisksAreAttachedByName call: expected nodeName %s, got %s", expected.nodeName, nodeName)
+		return areAttached, errors.New("Unexpected DisksAreAttachedByName call: wrong nodename")
+	}
+	if len(expected.volumeIDs) == 0 && expected.instanceID == "" {
+		// testcase.volumeIDs looks uninitialized, test did not expect to call DisksAreAttached
+		testcase.t.Errorf("Unexpected DisksAreAttachedByName call!")
+		return areAttached, errors.New("Unexpected DisksAreAttachedByName call")
+	}
+
+	if !reflect.DeepEqual(expected.volumeIDs, volumeIDs) {
+		testcase.t.Errorf("Unexpected DisksAreAttachedByName call: expected volumeIDs %v, got %v", expected.volumeIDs, volumeIDs)
+		return areAttached, errors.New("Unexpected DisksAreAttachedByName call: wrong volumeID")
+	}
+
+	if expected.instanceID != instanceID {
+		testcase.t.Errorf("Unexpected DisksAreAttachedByName call: expected instanceID %s, got %s", expected.instanceID, instanceID)
+		return areAttached, errors.New("Unexpected DisksAreAttachedByName call: wrong instanceID")
+	}
+
+	glog.V(4).Infof("DisksAreAttachedByName call: %v, %s, returning %v, %v", volumeIDs, nodeName, expected.areAttached, expected.ret)
+
+	return expected.areAttached, expected.ret
+}
+
 // Implementation of fake cloudprovider.Instances
 type instances struct {
 	instanceID string

pkg/volume/cinder/cinder.go

@@ -52,7 +52,8 @@ type CinderProvider interface {
 	GetAttachmentDiskPath(instanceID, volumeID string) (string, error)
 	OperationPending(diskName string) (bool, string, error)
 	DiskIsAttached(instanceID, volumeID string) (bool, error)
-	DisksAreAttached(instanceID string, volumeIDs []string) (map[string]bool, error)
+	DiskIsAttachedByName(nodeName types.NodeName, volumeID string) (bool, string, error)
+	DisksAreAttachedByName(nodeName types.NodeName, volumeIDs []string) (map[string]bool, error)
 	ShouldTrustDevicePath() bool
 	Instances() (cloudprovider.Instances, bool)
 	ExpandVolume(volumeID string, oldSize resource.Quantity, newSize resource.Quantity) (resource.Quantity, error)