From d72f88bf3a4fea43779db71873d43e5d35825074 Mon Sep 17 00:00:00 2001 From: saadali Date: Thu, 16 Jun 2016 20:11:09 -0700 Subject: [PATCH] Modify Attach method to return device path --- pkg/volume/aws_ebs/attacher.go | 28 ++++++------------- pkg/volume/cinder/attacher.go | 27 ++++++++++-------- pkg/volume/gce_pd/attacher.go | 17 ++++++----- pkg/volume/gce_pd/attacher_test.go | 25 ++++++++++++++--- pkg/volume/testing/testing.go | 4 +-- .../operationexecutor/operation_executor.go | 2 +- pkg/volume/volume.go | 6 ++-- 7 files changed, 61 insertions(+), 48 deletions(-) diff --git a/pkg/volume/aws_ebs/attacher.go b/pkg/volume/aws_ebs/attacher.go index 8e09025e567..751ecbf3988 100644 --- a/pkg/volume/aws_ebs/attacher.go +++ b/pkg/volume/aws_ebs/attacher.go @@ -41,38 +41,28 @@ func (plugin *awsElasticBlockStorePlugin) NewAttacher() (volume.Attacher, error) return &awsElasticBlockStoreAttacher{host: plugin.host}, nil } -func (attacher *awsElasticBlockStoreAttacher) Attach(spec *volume.Spec, hostName string) error { +func (attacher *awsElasticBlockStoreAttacher) Attach(spec *volume.Spec, hostName string) (string, error) { volumeSource, readOnly, err := getVolumeSource(spec) if err != nil { - return err + return "", err } volumeID := volumeSource.VolumeID awsCloud, err := getCloudProvider(attacher.host.GetCloudProvider()) if err != nil { - return err + return "", err } - attached, err := awsCloud.DiskIsAttached(volumeID, hostName) + // awsCloud.AttachDisk checks if disk is already attached to node and + // succeeds in that case, so no need to do that separately. + devicePath, err := awsCloud.AttachDisk(volumeID, hostName, readOnly) if err != nil { - // Log error and continue with attach - glog.Errorf( - "Error checking if volume (%q) is already attached to current node (%q). Will continue and try attach anyway. err=%v", - volumeID, hostName, err) - } - - if err == nil && attached { - // Volume is already attached to node. - glog.Infof("Attach operation is successful. volume %q is already attached to node %q.", volumeID, hostName) - return nil - } - - if _, err = awsCloud.AttachDisk(volumeID, hostName, readOnly); err != nil { glog.Errorf("Error attaching volume %q: %+v", volumeID, err) - return err + return "", err } - return nil + + return devicePath, nil } func (attacher *awsElasticBlockStoreAttacher) WaitForAttach(spec *volume.Spec, timeout time.Duration) (string, error) { diff --git a/pkg/volume/cinder/attacher.go b/pkg/volume/cinder/attacher.go index 2bf880a7232..c5fd3bf3ad0 100644 --- a/pkg/volume/cinder/attacher.go +++ b/pkg/volume/cinder/attacher.go @@ -45,25 +45,25 @@ func (plugin *cinderPlugin) NewAttacher() (volume.Attacher, error) { return &cinderDiskAttacher{host: plugin.host}, nil } -func (attacher *cinderDiskAttacher) Attach(spec *volume.Spec, hostName string) error { +func (attacher *cinderDiskAttacher) Attach(spec *volume.Spec, hostName string) (string, error) { volumeSource, _, err := getVolumeSource(spec) if err != nil { - return err + return "", err } volumeID := volumeSource.VolumeID cloud, err := getCloudProvider(attacher.host.GetCloudProvider()) if err != nil { - return err + return "", err } instances, res := cloud.Instances() if !res { - return fmt.Errorf("failed to list openstack instances") + return "", fmt.Errorf("failed to list openstack instances") } instanceid, err := instances.InstanceID(hostName) if err != nil { - return err + return "", err } if ind := strings.LastIndex(instanceid, "/"); ind >= 0 { instanceid = instanceid[(ind + 1):] @@ -79,15 +79,20 @@ func (attacher *cinderDiskAttacher) Attach(spec *volume.Spec, hostName string) e if err == nil && attached { // Volume is already attached to node. glog.Infof("Attach operation is successful. volume %q is already attached to node %q.", volumeID, instanceid) - return nil + } else { + _, err = cloud.AttachDisk(instanceid, volumeID) + if err != nil { + glog.Infof("attach volume %q to instance %q gets %v", volumeID, instanceid, err) + } } - _, err = cloud.AttachDisk(instanceid, volumeID) - if err != nil { - glog.Infof("attach volume %q to instance %q gets %v", volumeID, instanceid, err) - } glog.Infof("attached volume %q to instance %q", volumeID, instanceid) - return err + devicePath, err := cloud.GetAttachmentDiskPath(instanceid, volumeID) + if err != nil { + return "", err + } + + return devicePath, err } func (attacher *cinderDiskAttacher) WaitForAttach(spec *volume.Spec, timeout time.Duration) (string, error) { diff --git a/pkg/volume/gce_pd/attacher.go b/pkg/volume/gce_pd/attacher.go index 82c42ef41e0..413d498acb4 100644 --- a/pkg/volume/gce_pd/attacher.go +++ b/pkg/volume/gce_pd/attacher.go @@ -60,10 +60,10 @@ func (plugin *gcePersistentDiskPlugin) NewAttacher() (volume.Attacher, error) { // Callers are responsible for retryinging on failure. // Callers are responsible for thread safety between concurrent attach and // detach operations. -func (attacher *gcePersistentDiskAttacher) Attach(spec *volume.Spec, hostName string) error { +func (attacher *gcePersistentDiskAttacher) Attach(spec *volume.Spec, hostName string) (string, error) { volumeSource, readOnly, err := getVolumeSource(spec) if err != nil { - return err + return "", err } pdName := volumeSource.PDName @@ -79,15 +79,14 @@ func (attacher *gcePersistentDiskAttacher) Attach(spec *volume.Spec, hostName st if err == nil && attached { // Volume is already attached to node. glog.Infof("Attach operation is successful. PD %q is already attached to node %q.", pdName, hostName) - return nil + } else { + if err := attacher.gceDisks.AttachDisk(pdName, hostName, readOnly); err != nil { + glog.Errorf("Error attaching PD %q to node %q: %+v", pdName, hostName, err) + return "", err + } } - if err = attacher.gceDisks.AttachDisk(pdName, hostName, readOnly); err != nil { - glog.Errorf("Error attaching PD %q to node %q: %+v", pdName, hostName, err) - return err - } - - return nil + return path.Join(diskByIdPath, diskGooglePrefix+pdName), nil } func (attacher *gcePersistentDiskAttacher) WaitForAttach(spec *volume.Spec, timeout time.Duration) (string, error) { diff --git a/pkg/volume/gce_pd/attacher_test.go b/pkg/volume/gce_pd/attacher_test.go index 7a690fb80b4..24b9b6045f5 100644 --- a/pkg/volume/gce_pd/attacher_test.go +++ b/pkg/volume/gce_pd/attacher_test.go @@ -18,6 +18,7 @@ package gce_pd import ( "errors" + "fmt" "testing" "k8s.io/kubernetes/pkg/api" @@ -86,7 +87,11 @@ func TestAttachDetach(t *testing.T) { attach: attachCall{diskName, instanceID, readOnly, nil}, test: func(testcase *testcase) error { attacher := newAttacher(testcase) - return attacher.Attach(spec, instanceID) + devicePath, err := attacher.Attach(spec, instanceID) + if devicePath != "/dev/disk/by-id/google-disk" { + return fmt.Errorf("devicePath incorrect. Expected<\"/dev/disk/by-id/google-disk\"> Actual: <%q>", devicePath) + } + return err }, }, @@ -96,7 +101,11 @@ func TestAttachDetach(t *testing.T) { diskIsAttached: diskIsAttachedCall{diskName, instanceID, true, nil}, test: func(testcase *testcase) error { attacher := newAttacher(testcase) - return attacher.Attach(spec, instanceID) + devicePath, err := attacher.Attach(spec, instanceID) + if devicePath != "/dev/disk/by-id/google-disk" { + return fmt.Errorf("devicePath incorrect. Expected<\"/dev/disk/by-id/google-disk\"> Actual: <%q>", devicePath) + } + return err }, }, @@ -107,7 +116,11 @@ func TestAttachDetach(t *testing.T) { attach: attachCall{diskName, instanceID, readOnly, nil}, test: func(testcase *testcase) error { attacher := newAttacher(testcase) - return attacher.Attach(spec, instanceID) + devicePath, err := attacher.Attach(spec, instanceID) + if devicePath != "/dev/disk/by-id/google-disk" { + return fmt.Errorf("devicePath incorrect. Expected<\"/dev/disk/by-id/google-disk\"> Actual: <%q>", devicePath) + } + return err }, }, @@ -118,7 +131,11 @@ func TestAttachDetach(t *testing.T) { attach: attachCall{diskName, instanceID, readOnly, attachError}, test: func(testcase *testcase) error { attacher := newAttacher(testcase) - return attacher.Attach(spec, instanceID) + devicePath, err := attacher.Attach(spec, instanceID) + if devicePath != "" { + return fmt.Errorf("devicePath incorrect. Expected<\"\"> Actual: <%q>", devicePath) + } + return err }, expectedReturn: attachError, }, diff --git a/pkg/volume/testing/testing.go b/pkg/volume/testing/testing.go index bc5b5088741..e711631f470 100644 --- a/pkg/volume/testing/testing.go +++ b/pkg/volume/testing/testing.go @@ -358,11 +358,11 @@ func (fv *FakeVolume) TearDownAt(dir string) error { return os.RemoveAll(dir) } -func (fv *FakeVolume) Attach(spec *Spec, hostName string) error { +func (fv *FakeVolume) Attach(spec *Spec, hostName string) (string, error) { fv.Lock() defer fv.Unlock() fv.AttachCallCount++ - return nil + return "", nil } func (fv *FakeVolume) GetAttachCallCount() int { diff --git a/pkg/volume/util/operationexecutor/operation_executor.go b/pkg/volume/util/operationexecutor/operation_executor.go index 520cb24744f..fd2acccb9ee 100644 --- a/pkg/volume/util/operationexecutor/operation_executor.go +++ b/pkg/volume/util/operationexecutor/operation_executor.go @@ -385,7 +385,7 @@ func (oe *operationExecutor) generateAttachVolumeFunc( return func() error { // Execute attach - attachErr := volumeAttacher.Attach( + _, attachErr := volumeAttacher.Attach( volumeToAttach.VolumeSpec, volumeToAttach.NodeName) if attachErr != nil { diff --git a/pkg/volume/volume.go b/pkg/volume/volume.go index 1b61ab5a0d4..6a16f786b68 100644 --- a/pkg/volume/volume.go +++ b/pkg/volume/volume.go @@ -134,8 +134,10 @@ type Deleter interface { // Attacher can attach a volume to a node. type Attacher interface { - // Attach the volume specified by the given spec to the given host - Attach(spec *Spec, hostName string) error + // Attaches the volume specified by the given spec to the given host. + // On success, returns the device path where the device was attache don the + // node. + Attach(spec *Spec, hostName string) (string, error) // WaitForAttach blocks until the device is attached to this // node. If it successfully attaches, the path to the device