From 4a6e1f2a1d7bd7d3e0981fad1fe2f43931745f0a Mon Sep 17 00:00:00 2001 From: FengyunPan Date: Fri, 12 May 2017 17:03:34 +0800 Subject: [PATCH 1/2] Don't return err when volume's status is 'attaching' When volume's status is 'attaching', its attachments will be None, controllermanager can't get device path and make some failed event. But it is normal, let's fix it. --- .../providers/openstack/openstack_volumes.go | 42 ++++++++- .../providers/rackspace/rackspace.go | 48 ++++++++-- pkg/volume/cinder/attacher.go | 26 +++++- pkg/volume/cinder/attacher_test.go | 90 ++++++++++++++----- pkg/volume/cinder/cinder.go | 1 + .../operationexecutor/operation_generator.go | 3 + 6 files changed, 175 insertions(+), 35 deletions(-) diff --git a/pkg/cloudprovider/providers/openstack/openstack_volumes.go b/pkg/cloudprovider/providers/openstack/openstack_volumes.go index cdd1c8d2f44..91284650980 100644 --- a/pkg/cloudprovider/providers/openstack/openstack_volumes.go +++ b/pkg/cloudprovider/providers/openstack/openstack_volumes.go @@ -74,8 +74,14 @@ type VolumeCreateOpts struct { Metadata map[string]string } -func (volumes *VolumesV1) createVolume(opts VolumeCreateOpts) (string, string, error) { +const ( + VolumeAvailableStatus = "available" + VolumeInUseStatus = "in-use" + VolumeDeletedStatus = "deleted" + VolumeErrorStatus = "error" +) +func (volumes *VolumesV1) createVolume(opts VolumeCreateOpts) (string, string, error) { create_opts := volumes_v1.CreateOpts{ Name: opts.Name, Size: opts.Size, @@ -92,7 +98,6 @@ func (volumes *VolumesV1) createVolume(opts VolumeCreateOpts) (string, string, e } func (volumes *VolumesV2) createVolume(opts VolumeCreateOpts) (string, string, error) { - create_opts := volumes_v2.CreateOpts{ Name: opts.Name, Size: opts.Size, @@ -201,12 +206,33 @@ func (volumes *VolumesV2) deleteVolume(volumeName string) error { return err } +func (os *OpenStack) OperationPending(diskName string) (bool, string, error) { + volume, err := os.getVolume(diskName) + if err != nil { + return false, "", err + } + volumeStatus := volume.Status + if volumeStatus == VolumeErrorStatus { + glog.Errorf("status of volume %s is %s", diskName, volumeStatus) + return false, volumeStatus, nil + } + if volumeStatus == VolumeAvailableStatus || volumeStatus == VolumeInUseStatus || volumeStatus == VolumeDeletedStatus { + return false, volume.Status, nil + } + return true, volumeStatus, nil +} + // Attaches given cinder volume to the compute running kubelet func (os *OpenStack) AttachDisk(instanceID string, diskName string) (string, error) { volume, err := os.getVolume(diskName) if err != nil { return "", err } + if volume.Status != VolumeAvailableStatus { + errmsg := fmt.Sprintf("volume %s status is %s, not %s, can not be attached to instance %s.", volume.Name, volume.Status, VolumeAvailableStatus, instanceID) + glog.Errorf(errmsg) + return "", errors.New(errmsg) + } cClient, err := openstack.NewComputeV2(os.provider, gophercloud.EndpointOpts{ Region: os.region, }) @@ -245,6 +271,11 @@ func (os *OpenStack) DetachDisk(instanceID string, partialDiskId string) error { if err != nil { return err } + if volume.Status != VolumeInUseStatus { + errmsg := fmt.Sprintf("can not detach volume %s, its status is %s.", volume.Name, volume.Status) + glog.Errorf(errmsg) + return errors.New(errmsg) + } cClient, err := openstack.NewComputeV2(os.provider, gophercloud.EndpointOpts{ Region: os.region, }) @@ -369,6 +400,11 @@ func (os *OpenStack) GetAttachmentDiskPath(instanceID string, diskName string) ( if err != nil { return "", err } + if volume.Status != VolumeInUseStatus { + errmsg := fmt.Sprintf("can not get device path of volume %s, its status is %s.", volume.Name, volume.Status) + glog.Errorf(errmsg) + return "", errors.New(errmsg) + } if volume.AttachedServerId != "" { if instanceID == volume.AttachedServerId { // Attachment[0]["device"] points to the device path @@ -380,7 +416,7 @@ func (os *OpenStack) GetAttachmentDiskPath(instanceID string, diskName string) ( return "", errors.New(errMsg) } } - return "", fmt.Errorf("volume %s is not attached to %s", diskName, instanceID) + return "", fmt.Errorf("volume %s has not ServerId.", diskName) } // query if a volume is attached to a compute instance diff --git a/pkg/cloudprovider/providers/rackspace/rackspace.go b/pkg/cloudprovider/providers/rackspace/rackspace.go index 1c561bc29ff..253b0508870 100644 --- a/pkg/cloudprovider/providers/rackspace/rackspace.go +++ b/pkg/cloudprovider/providers/rackspace/rackspace.go @@ -45,8 +45,13 @@ import ( "k8s.io/kubernetes/pkg/cloudprovider" ) -const ProviderName = "rackspace" -const metaDataPath = "/media/configdrive/openstack/latest/meta_data.json" +const ( + ProviderName = "rackspace" + MetaDataPath = "/media/configdrive/openstack/latest/meta_data.json" + VolumeAvailableStatus = "available" + VolumeInUseStatus = "in-use" + VolumeErrorStatus = "error" +) var ErrNotFound = errors.New("Failed to find object") var ErrMultipleResults = errors.New("Multiple results where only one expected") @@ -146,16 +151,16 @@ func parseMetaData(file io.Reader) (string, error) { metaData := MetaData{} err = json.Unmarshal(metaDataBytes, &metaData) if err != nil { - return "", fmt.Errorf("Cannot parse %s: %v", metaDataPath, err) + return "", fmt.Errorf("Cannot parse %s: %v", MetaDataPath, err) } return metaData.UUID, nil } func readInstanceID() (string, error) { - file, err := os.Open(metaDataPath) + file, err := os.Open(MetaDataPath) if err != nil { - return "", fmt.Errorf("Cannot open %s: %v", metaDataPath, err) + return "", fmt.Errorf("Cannot open %s: %v", MetaDataPath, err) } defer file.Close() @@ -487,6 +492,22 @@ func (rs *Rackspace) DeleteVolume(volumeName string) error { return errors.New("unimplemented") } +func (rs *Rackspace) OperationPending(diskName string) (bool, string, error) { + disk, err := rs.getVolume(diskName) + if err != nil { + return false, "", err + } + volumeStatus := disk.Status + if volumeStatus == VolumeErrorStatus { + glog.Errorf("status of volume %s is %s", diskName, volumeStatus) + return false, volumeStatus, nil + } + if volumeStatus == VolumeAvailableStatus || volumeStatus == VolumeInUseStatus { + return false, disk.Status, nil + } + return true, volumeStatus, nil +} + // Attaches given cinder volume to the compute running kubelet func (rs *Rackspace) AttachDisk(instanceID string, diskName string) (string, error) { disk, err := rs.getVolume(diskName) @@ -494,6 +515,12 @@ func (rs *Rackspace) AttachDisk(instanceID string, diskName string) (string, err return "", err } + if disk.Status != VolumeAvailableStatus { + errmsg := fmt.Sprintf("volume %s status is %s, not %s, can not be attached to instance %s.", disk.Name, disk.Status, VolumeAvailableStatus, instanceID) + glog.Errorf(errmsg) + return "", errors.New(errmsg) + } + compute, err := rs.getComputeClient() if err != nil { return "", err @@ -589,6 +616,12 @@ func (rs *Rackspace) DetachDisk(instanceID string, partialDiskId string) error { return err } + if disk.Status != VolumeInUseStatus { + errmsg := fmt.Sprintf("can not detach volume %s, its status is %s.", disk.Name, disk.Status) + glog.Errorf(errmsg) + return errors.New(errmsg) + } + compute, err := rs.getComputeClient() if err != nil { return err @@ -626,6 +659,11 @@ func (rs *Rackspace) GetAttachmentDiskPath(instanceID string, diskName string) ( if err != nil { return "", err } + if disk.Status != VolumeInUseStatus { + errmsg := fmt.Sprintf("can not get device path of volume %s, its status is %s.", disk.Name, disk.Status) + glog.Errorf(errmsg) + return "", errors.New(errmsg) + } if len(disk.Attachments) > 0 && disk.Attachments[0]["server_id"] != nil { if instanceID == disk.Attachments[0]["server_id"] { // Attachment[0]["device"] points to the device path diff --git a/pkg/volume/cinder/attacher.go b/pkg/volume/cinder/attacher.go index ad9aa50becd..4abc668c543 100644 --- a/pkg/volume/cinder/attacher.go +++ b/pkg/volume/cinder/attacher.go @@ -73,6 +73,15 @@ func (attacher *cinderDiskAttacher) Attach(spec *volume.Spec, nodeName types.Nod return "", err } + pending, volumeStatus, err := attacher.cinderProvider.OperationPending(volumeID) + if err != nil { + return "", err + } + if pending { + glog.Infof("status of volume %q is %s, wait it to be finished", volumeID, volumeStatus) + return "", nil + } + attached, err := attacher.cinderProvider.DiskIsAttached(volumeID, instanceid) if err != nil { // Log error and continue with attach @@ -89,18 +98,27 @@ func (attacher *cinderDiskAttacher) Attach(spec *volume.Spec, nodeName types.Nod if err == nil { glog.Infof("Attach operation successful: volume %q attached to instance %q.", volumeID, instanceid) } else { - glog.Infof("Attach volume %q to instance %q failed with %v", volumeID, instanceid, err) + glog.Infof("Attach volume %q to instance %q failed with: %v", volumeID, instanceid, err) return "", err } } - devicePath, err := attacher.cinderProvider.GetAttachmentDiskPath(instanceid, volumeID) + // When volume's status is 'attaching', DiskIsAttached() return , and can't get device path. + // We should get device path next time and do not return err. + devicePath := "" + pending, _, err = attacher.cinderProvider.OperationPending(volumeID) if err != nil { - glog.Infof("Attach volume %q to instance %q failed with %v", volumeID, instanceid, err) return "", err } + if !pending { + devicePath, err = attacher.cinderProvider.GetAttachmentDiskPath(instanceid, volumeID) + if err != nil { + glog.Infof("Can not get device path of volume %q which be attached to instance %q, failed with: %v", volumeID, instanceid, err) + return "", err + } + } - return devicePath, err + return devicePath, nil } func (attacher *cinderDiskAttacher) VolumesAreAttached(specs []*volume.Spec, nodeName types.NodeName) (map[*volume.Spec]bool, error) { diff --git a/pkg/volume/cinder/attacher_test.go b/pkg/volume/cinder/attacher_test.go index 86031bb8090..726b77462d6 100644 --- a/pkg/volume/cinder/attacher_test.go +++ b/pkg/volume/cinder/attacher_test.go @@ -33,6 +33,9 @@ import ( "k8s.io/apimachinery/pkg/types" ) +const VolumeStatusPending = "pending" +const VolumeStatusDone = "done" + func TestGetDeviceName_Volume(t *testing.T) { plugin := newPlugin() name := "my-cinder-volume" @@ -88,6 +91,7 @@ type testcase struct { // For fake GCE: attach attachCall detach detachCall + operationPending operationPendingCall diskIsAttached diskIsAttachedCall disksAreAttached disksAreAttachedCall diskPath diskPathCall @@ -104,6 +108,8 @@ type testcase struct { func TestAttachDetach(t *testing.T) { diskName := "disk" instanceID := "instance" + pending := VolumeStatusPending + done := VolumeStatusDone nodeName := types.NodeName("nodeName") readOnly := false spec := createVolSpec(diskName, readOnly) @@ -115,11 +121,12 @@ func TestAttachDetach(t *testing.T) { tests := []testcase{ // Successful Attach call { - name: "Attach_Positive", - instanceID: instanceID, - diskIsAttached: diskIsAttachedCall{diskName, instanceID, false, nil}, - attach: attachCall{diskName, instanceID, "", nil}, - diskPath: diskPathCall{diskName, instanceID, "/dev/sda", nil}, + name: "Attach_Positive", + instanceID: instanceID, + operationPending: operationPendingCall{diskName, false, done, nil}, + diskIsAttached: diskIsAttachedCall{diskName, instanceID, false, nil}, + attach: attachCall{diskName, instanceID, "", nil}, + diskPath: diskPathCall{diskName, instanceID, "/dev/sda", nil}, test: func(testcase *testcase) (string, error) { attacher := newAttacher(testcase) return attacher.Attach(spec, nodeName) @@ -129,10 +136,11 @@ func TestAttachDetach(t *testing.T) { // Disk is already attached { - name: "Attach_Positive_AlreadyAttached", - instanceID: instanceID, - diskIsAttached: diskIsAttachedCall{diskName, instanceID, true, nil}, - diskPath: diskPathCall{diskName, instanceID, "/dev/sda", nil}, + name: "Attach_Positive_AlreadyAttached", + instanceID: instanceID, + operationPending: operationPendingCall{diskName, false, done, nil}, + diskIsAttached: diskIsAttachedCall{diskName, instanceID, true, nil}, + diskPath: diskPathCall{diskName, instanceID, "/dev/sda", nil}, test: func(testcase *testcase) (string, error) { attacher := newAttacher(testcase) return attacher.Attach(spec, nodeName) @@ -140,13 +148,27 @@ func TestAttachDetach(t *testing.T) { expectedResult: "/dev/sda", }, + // Disk is attaching + { + name: "Attach_is_attaching", + instanceID: instanceID, + operationPending: operationPendingCall{diskName, true, pending, nil}, + diskIsAttached: diskIsAttachedCall{diskName, instanceID, false, diskCheckError}, + test: func(testcase *testcase) (string, error) { + attacher := newAttacher(testcase) + return attacher.Attach(spec, nodeName) + }, + expectedResult: "", + }, + // DiskIsAttached fails and Attach succeeds { - name: "Attach_Positive_CheckFails", - instanceID: instanceID, - diskIsAttached: diskIsAttachedCall{diskName, instanceID, false, diskCheckError}, - attach: attachCall{diskName, instanceID, "", nil}, - diskPath: diskPathCall{diskName, instanceID, "/dev/sda", nil}, + name: "Attach_Positive_CheckFails", + instanceID: instanceID, + operationPending: operationPendingCall{diskName, false, done, nil}, + diskIsAttached: diskIsAttachedCall{diskName, instanceID, false, diskCheckError}, + attach: attachCall{diskName, instanceID, "", nil}, + diskPath: diskPathCall{diskName, instanceID, "/dev/sda", nil}, test: func(testcase *testcase) (string, error) { attacher := newAttacher(testcase) return attacher.Attach(spec, nodeName) @@ -156,10 +178,11 @@ func TestAttachDetach(t *testing.T) { // Attach call fails { - name: "Attach_Negative", - instanceID: instanceID, - diskIsAttached: diskIsAttachedCall{diskName, instanceID, false, diskCheckError}, - attach: attachCall{diskName, instanceID, "/dev/sda", attachError}, + name: "Attach_Negative", + instanceID: instanceID, + operationPending: operationPendingCall{diskName, false, done, nil}, + diskIsAttached: diskIsAttachedCall{diskName, instanceID, false, diskCheckError}, + attach: attachCall{diskName, instanceID, "/dev/sda", attachError}, test: func(testcase *testcase) (string, error) { attacher := newAttacher(testcase) return attacher.Attach(spec, nodeName) @@ -169,11 +192,12 @@ func TestAttachDetach(t *testing.T) { // GetAttachmentDiskPath call fails { - name: "Attach_Negative_DiskPatchFails", - instanceID: instanceID, - diskIsAttached: diskIsAttachedCall{diskName, instanceID, false, diskCheckError}, - attach: attachCall{diskName, instanceID, "", nil}, - diskPath: diskPathCall{diskName, instanceID, "", diskPathError}, + name: "Attach_Negative_DiskPatchFails", + instanceID: instanceID, + operationPending: operationPendingCall{diskName, false, done, nil}, + diskIsAttached: diskIsAttachedCall{diskName, instanceID, false, diskCheckError}, + attach: attachCall{diskName, instanceID, "", nil}, + diskPath: diskPathCall{diskName, instanceID, "", diskPathError}, test: func(testcase *testcase) (string, error) { attacher := newAttacher(testcase) return attacher.Attach(spec, nodeName) @@ -384,6 +408,13 @@ type detachCall struct { ret error } +type operationPendingCall struct { + diskName string + pending bool + volumeStatus string + ret error +} + type diskIsAttachedCall struct { diskName, instanceID string isAttached bool @@ -453,6 +484,19 @@ func (testcase *testcase) DetachDisk(instanceID string, partialDiskId string) er return expected.ret } +func (testcase *testcase) OperationPending(diskName string) (bool, string, error) { + expected := &testcase.operationPending + + if expected.volumeStatus == VolumeStatusPending { + glog.V(4).Infof("OperationPending call: %s, returning %v, %v, %v", diskName, expected.pending, expected.volumeStatus, expected.ret) + return true, expected.volumeStatus, expected.ret + } + + glog.V(4).Infof("OperationPending call: %s, returning %v, %v, %v", diskName, expected.pending, expected.volumeStatus, expected.ret) + + return false, expected.volumeStatus, expected.ret +} + func (testcase *testcase) DiskIsAttached(diskName, instanceID string) (bool, error) { expected := &testcase.diskIsAttached diff --git a/pkg/volume/cinder/cinder.go b/pkg/volume/cinder/cinder.go index 7aab25929dd..704fd06a516 100644 --- a/pkg/volume/cinder/cinder.go +++ b/pkg/volume/cinder/cinder.go @@ -51,6 +51,7 @@ type CinderProvider interface { GetDevicePath(diskId string) string InstanceID() (string, error) GetAttachmentDiskPath(instanceID string, diskName string) (string, error) + OperationPending(diskName string) (bool, string, error) DiskIsAttached(diskName, instanceID string) (bool, error) DisksAreAttached(diskNames []string, instanceID string) (map[string]bool, error) ShouldTrustDevicePath() bool diff --git a/pkg/volume/util/operationexecutor/operation_generator.go b/pkg/volume/util/operationexecutor/operation_generator.go index 7557acd31c9..f6ced832005 100644 --- a/pkg/volume/util/operationexecutor/operation_generator.go +++ b/pkg/volume/util/operationexecutor/operation_generator.go @@ -263,6 +263,9 @@ func (og *operationGenerator) GenerateAttachVolumeFunc( og.recorder.Eventf(pod, v1.EventTypeWarning, kevents.FailedMountVolume, eventErr.Error()) } return detailedErr + } else if devicePath == "" { + // do nothing and get device path next time. + return nil } glog.Infof(volumeToAttach.GenerateMsgDetailed("AttachVolume.Attach succeeded", "")) From d86bf8a0b8546dae8705c33cbc0c4df43608e0d8 Mon Sep 17 00:00:00 2001 From: FengyunPan Date: Sat, 13 May 2017 15:19:34 +0800 Subject: [PATCH 2/2] Fix reconciler test of attaching volume The Attach() of FakeVolume should return device path --- pkg/volume/testing/testing.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/volume/testing/testing.go b/pkg/volume/testing/testing.go index 9c859c13383..8eccc091bf4 100644 --- a/pkg/volume/testing/testing.go +++ b/pkg/volume/testing/testing.go @@ -387,7 +387,7 @@ func (fv *FakeVolume) Attach(spec *Spec, nodeName types.NodeName) (string, error fv.Lock() defer fv.Unlock() fv.AttachCallCount++ - return "", nil + return "/dev/vdb-test", nil } func (fv *FakeVolume) GetAttachCallCount() int {