Skip to content
This repository was archived by the owner on Mar 22, 2018. It is now read-only.

Commit 3f5d4c9

Browse files
zetaabzetaab
committed
move detach out of os volumes attach
add test add test fix bazel fix tests change loglevel, remove else statement
1 parent 0c0f1a1 commit 3f5d4c9

4 files changed

Lines changed: 151 additions & 67 deletions

File tree

‎pkg/cloudprovider/providers/openstack/openstack_volumes.go‎

Lines changed: 64 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ import (
2626
"time"
2727

2828
"k8s.io/apimachinery/pkg/api/resource"
29+
"k8s.io/apimachinery/pkg/types"
2930
k8s_volume "k8s.io/kubernetes/pkg/volume"
3031
volumeutil "k8s.io/kubernetes/pkg/volume/util"
3132

@@ -319,33 +320,18 @@ func (os *OpenStack) AttachDisk(instanceID, volumeID string) (string, error) {
319320
if instanceID == volume.AttachedServerId {
320321
glog.V(4).Infof("Disk %s is already attached to instance %s", volumeID, instanceID)
321322
return volume.ID, nil
322-
} else {
323-
nodeName, err := os.GetNodeNameByID(volume.AttachedServerId)
324-
attachErr := fmt.Sprintf("disk %s path %s is attached to a different instance (%s)", volumeID, volume.AttachedDevice, volume.AttachedServerId)
325-
if err != nil {
326-
glog.Error(attachErr)
327-
return "", errors.New(attachErr)
328-
}
329-
// using volume.AttachedDevice may cause problems because cinder does not report device path correctly see issue #33128
330-
devicePath := volume.AttachedDevice
331-
danglingErr := volumeutil.NewDanglingError(attachErr, nodeName, devicePath)
332-
glog.V(4).Infof("volume %s is already attached to node %s path %s", volumeID, nodeName, devicePath)
333-
// check special case, if node is deleted from cluster but exist still in openstack
334-
// we need to check can we detach the cinder, node is deleted from cluster if state is not ACTIVE
335-
srv, err := getServerByName(cClient, nodeName, false)
336-
if err != nil {
337-
return "", err
338-
}
339-
if srv.Status != "ACTIVE" {
340-
err = os.DetachDisk(volume.AttachedServerId, volumeID)
341-
if err != nil {
342-
glog.Error(err)
343-
return "", err
344-
}
345-
glog.V(4).Infof("detached volume %s node state was %s", volumeID, srv.Status)
346-
}
347-
return "", danglingErr
348323
}
324+
nodeName, err := os.GetNodeNameByID(volume.AttachedServerId)
325+
attachErr := fmt.Sprintf("disk %s path %s is attached to a different instance (%s)", volumeID, volume.AttachedDevice, volume.AttachedServerId)
326+
if err != nil {
327+
glog.Error(attachErr)
328+
return "", errors.New(attachErr)
329+
}
330+
// using volume.AttachedDevice may cause problems because cinder does not report device path correctly see issue #33128
331+
devicePath := volume.AttachedDevice
332+
danglingErr := volumeutil.NewDanglingError(attachErr, nodeName, devicePath)
333+
glog.V(2).Infof("Found dangling volume %s attached to node %s", volumeID, nodeName)
334+
return "", danglingErr
349335
}
350336

351337
startTime := time.Now()
@@ -605,6 +591,9 @@ func (os *OpenStack) GetAttachmentDiskPath(instanceID, volumeID string) (string,
605591

606592
// DiskIsAttached queries if a volume is attached to a compute instance
607593
func (os *OpenStack) DiskIsAttached(instanceID, volumeID string) (bool, error) {
594+
if instanceID == "" {
595+
glog.Warningf("calling DiskIsAttached with empty instanceid: %s %s", instanceID, volumeID)
596+
}
608597
volume, err := os.getVolume(volumeID)
609598
if err != nil {
610599
return false, err
@@ -613,6 +602,29 @@ func (os *OpenStack) DiskIsAttached(instanceID, volumeID string) (bool, error) {
613602
return instanceID == volume.AttachedServerId, nil
614603
}
615604

605+
// DiskIsAttachedByName queries if a volume is attached to a compute instance by name
606+
func (os *OpenStack) DiskIsAttachedByName(nodeName types.NodeName, volumeID string) (bool, string, error) {
607+
cClient, err := os.NewComputeV2()
608+
if err != nil {
609+
return false, "", err
610+
}
611+
srv, err := getServerByName(cClient, nodeName, false)
612+
if err != nil {
613+
if err == ErrNotFound {
614+
// instance not found anymore in cloudprovider, assume that cinder is detached
615+
return false, "", nil
616+
} else {
617+
return false, "", err
618+
}
619+
}
620+
instanceID := "/" + srv.ID
621+
if ind := strings.LastIndex(instanceID, "/"); ind >= 0 {
622+
instanceID = instanceID[(ind + 1):]
623+
}
624+
attached, err := os.DiskIsAttached(instanceID, volumeID)
625+
return attached, instanceID, err
626+
}
627+
616628
// DisksAreAttached queries if a list of volumes are attached to a compute instance
617629
func (os *OpenStack) DisksAreAttached(instanceID string, volumeIDs []string) (map[string]bool, error) {
618630
attached := make(map[string]bool)
@@ -627,6 +639,32 @@ func (os *OpenStack) DisksAreAttached(instanceID string, volumeIDs []string) (ma
627639
return attached, nil
628640
}
629641

642+
// DisksAreAttachedByName queries if a list of volumes are attached to a compute instance by name
643+
func (os *OpenStack) DisksAreAttachedByName(nodeName types.NodeName, volumeIDs []string) (map[string]bool, error) {
644+
attached := make(map[string]bool)
645+
cClient, err := os.NewComputeV2()
646+
if err != nil {
647+
return attached, err
648+
}
649+
srv, err := getServerByName(cClient, nodeName, false)
650+
if err != nil {
651+
if err == ErrNotFound {
652+
// instance not found anymore, mark all volumes as detached
653+
for _, volumeID := range volumeIDs {
654+
attached[volumeID] = false
655+
}
656+
return attached, nil
657+
} else {
658+
return attached, err
659+
}
660+
}
661+
instanceID := "/" + srv.ID
662+
if ind := strings.LastIndex(instanceID, "/"); ind >= 0 {
663+
instanceID = instanceID[(ind + 1):]
664+
}
665+
return os.DisksAreAttached(instanceID, volumeIDs)
666+
}
667+
630668
// diskIsUsed returns true a disk is attached to any node.
631669
func (os *OpenStack) diskIsUsed(volumeID string) (bool, error) {
632670
volume, err := os.getVolume(volumeID)

‎pkg/volume/cinder/attacher.go‎

Lines changed: 2 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,6 @@ import (
2727
"k8s.io/api/core/v1"
2828
"k8s.io/apimachinery/pkg/types"
2929
"k8s.io/apimachinery/pkg/util/wait"
30-
"k8s.io/kubernetes/pkg/cloudprovider"
3130
"k8s.io/kubernetes/pkg/util/mount"
3231
"k8s.io/kubernetes/pkg/volume"
3332
volumeutil "k8s.io/kubernetes/pkg/volume/util"
@@ -187,23 +186,7 @@ func (attacher *cinderDiskAttacher) VolumesAreAttached(specs []*volume.Spec, nod
187186
volumeSpecMap[volumeSource.VolumeID] = spec
188187
}
189188

190-
instanceID, err := attacher.nodeInstanceID(nodeName)
191-
if err != nil {
192-
if err == cloudprovider.InstanceNotFound {
193-
// If node doesn't exist, OpenStack Nova will assume the volumes are not attached to it.
194-
// Mark the volumes as detached and return false without error.
195-
glog.Warningf("VolumesAreAttached: node %q does not exist.", nodeName)
196-
for spec := range volumesAttachedCheck {
197-
volumesAttachedCheck[spec] = false
198-
}
199-
200-
return volumesAttachedCheck, nil
201-
}
202-
203-
return volumesAttachedCheck, err
204-
}
205-
206-
attachedResult, err := attacher.cinderProvider.DisksAreAttached(instanceID, volumeIDList)
189+
attachedResult, err := attacher.cinderProvider.DisksAreAttachedByName(nodeName, volumeIDList)
207190
if err != nil {
208191
// Log error and continue with attach
209192
glog.Errorf(
@@ -381,20 +364,10 @@ func (detacher *cinderDiskDetacher) waitDiskDetached(instanceID, volumeID string
381364

382365
func (detacher *cinderDiskDetacher) Detach(volumeName string, nodeName types.NodeName) error {
383366
volumeID := path.Base(volumeName)
384-
instances, res := detacher.cinderProvider.Instances()
385-
if !res {
386-
return fmt.Errorf("failed to list openstack instances")
387-
}
388-
instanceID, err := instances.InstanceID(nodeName)
389-
if ind := strings.LastIndex(instanceID, "/"); ind >= 0 {
390-
instanceID = instanceID[(ind + 1):]
391-
}
392-
393367
if err := detacher.waitOperationFinished(volumeID); err != nil {
394368
return err
395369
}
396-
397-
attached, err := detacher.cinderProvider.DiskIsAttached(instanceID, volumeID)
370+
attached, instanceID, err := detacher.cinderProvider.DiskIsAttachedByName(nodeName, volumeID)
398371
if err != nil {
399372
// Log error and continue with detach
400373
glog.Errorf(

‎pkg/volume/cinder/attacher_test.go‎

Lines changed: 83 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -132,7 +132,7 @@ func TestAttachDetach(t *testing.T) {
132132
name: "Attach_Positive",
133133
instanceID: instanceID,
134134
operationPending: operationPendingCall{volumeID, false, done, nil},
135-
diskIsAttached: diskIsAttachedCall{instanceID, volumeID, false, nil},
135+
diskIsAttached: diskIsAttachedCall{instanceID, nodeName, volumeID, false, nil},
136136
attach: attachCall{instanceID, volumeID, "", nil},
137137
diskPath: diskPathCall{instanceID, volumeID, "/dev/sda", nil},
138138
test: func(testcase *testcase) (string, error) {
@@ -147,7 +147,7 @@ func TestAttachDetach(t *testing.T) {
147147
name: "Attach_Positive_AlreadyAttached",
148148
instanceID: instanceID,
149149
operationPending: operationPendingCall{volumeID, false, done, nil},
150-
diskIsAttached: diskIsAttachedCall{instanceID, volumeID, true, nil},
150+
diskIsAttached: diskIsAttachedCall{instanceID, nodeName, volumeID, true, nil},
151151
diskPath: diskPathCall{instanceID, volumeID, "/dev/sda", nil},
152152
test: func(testcase *testcase) (string, error) {
153153
attacher := newAttacher(testcase)
@@ -173,7 +173,7 @@ func TestAttachDetach(t *testing.T) {
173173
name: "Attach_Negative",
174174
instanceID: instanceID,
175175
operationPending: operationPendingCall{volumeID, false, done, nil},
176-
diskIsAttached: diskIsAttachedCall{instanceID, volumeID, false, diskCheckError},
176+
diskIsAttached: diskIsAttachedCall{instanceID, nodeName, volumeID, false, diskCheckError},
177177
attach: attachCall{instanceID, volumeID, "/dev/sda", attachError},
178178
test: func(testcase *testcase) (string, error) {
179179
attacher := newAttacher(testcase)
@@ -187,7 +187,7 @@ func TestAttachDetach(t *testing.T) {
187187
name: "Attach_Negative_DiskPatchFails",
188188
instanceID: instanceID,
189189
operationPending: operationPendingCall{volumeID, false, done, nil},
190-
diskIsAttached: diskIsAttachedCall{instanceID, volumeID, false, nil},
190+
diskIsAttached: diskIsAttachedCall{instanceID, nodeName, volumeID, false, nil},
191191
attach: attachCall{instanceID, volumeID, "", nil},
192192
diskPath: diskPathCall{instanceID, volumeID, "", diskPathError},
193193
test: func(testcase *testcase) (string, error) {
@@ -201,7 +201,7 @@ func TestAttachDetach(t *testing.T) {
201201
{
202202
name: "VolumesAreAttached_Positive",
203203
instanceID: instanceID,
204-
disksAreAttached: disksAreAttachedCall{instanceID, []string{volumeID}, map[string]bool{volumeID: true}, nil},
204+
disksAreAttached: disksAreAttachedCall{instanceID, nodeName, []string{volumeID}, map[string]bool{volumeID: true}, nil},
205205
test: func(testcase *testcase) (string, error) {
206206
attacher := newAttacher(testcase)
207207
attachments, err := attacher.VolumesAreAttached([]*volume.Spec{spec}, nodeName)
@@ -214,7 +214,7 @@ func TestAttachDetach(t *testing.T) {
214214
{
215215
name: "VolumesAreAttached_Negative",
216216
instanceID: instanceID,
217-
disksAreAttached: disksAreAttachedCall{instanceID, []string{volumeID}, map[string]bool{volumeID: false}, nil},
217+
disksAreAttached: disksAreAttachedCall{instanceID, nodeName, []string{volumeID}, map[string]bool{volumeID: false}, nil},
218218
test: func(testcase *testcase) (string, error) {
219219
attacher := newAttacher(testcase)
220220
attachments, err := attacher.VolumesAreAttached([]*volume.Spec{spec}, nodeName)
@@ -227,7 +227,7 @@ func TestAttachDetach(t *testing.T) {
227227
{
228228
name: "VolumesAreAttached_CinderFailed",
229229
instanceID: instanceID,
230-
disksAreAttached: disksAreAttachedCall{instanceID, []string{volumeID}, nil, disksCheckError},
230+
disksAreAttached: disksAreAttachedCall{instanceID, nodeName, []string{volumeID}, nil, disksCheckError},
231231
test: func(testcase *testcase) (string, error) {
232232
attacher := newAttacher(testcase)
233233
attachments, err := attacher.VolumesAreAttached([]*volume.Spec{spec}, nodeName)
@@ -242,7 +242,7 @@ func TestAttachDetach(t *testing.T) {
242242
name: "Detach_Positive",
243243
instanceID: instanceID,
244244
operationPending: operationPendingCall{volumeID, false, done, nil},
245-
diskIsAttached: diskIsAttachedCall{instanceID, volumeID, true, nil},
245+
diskIsAttached: diskIsAttachedCall{instanceID, nodeName, volumeID, true, nil},
246246
detach: detachCall{instanceID, volumeID, nil},
247247
test: func(testcase *testcase) (string, error) {
248248
detacher := newDetacher(testcase)
@@ -255,7 +255,7 @@ func TestAttachDetach(t *testing.T) {
255255
name: "Detach_Positive_AlreadyDetached",
256256
instanceID: instanceID,
257257
operationPending: operationPendingCall{volumeID, false, done, nil},
258-
diskIsAttached: diskIsAttachedCall{instanceID, volumeID, false, nil},
258+
diskIsAttached: diskIsAttachedCall{instanceID, nodeName, volumeID, false, nil},
259259
test: func(testcase *testcase) (string, error) {
260260
detacher := newDetacher(testcase)
261261
return "", detacher.Detach(volumeID, nodeName)
@@ -267,7 +267,7 @@ func TestAttachDetach(t *testing.T) {
267267
name: "Detach_Positive_CheckFails",
268268
instanceID: instanceID,
269269
operationPending: operationPendingCall{volumeID, false, done, nil},
270-
diskIsAttached: diskIsAttachedCall{instanceID, volumeID, false, diskCheckError},
270+
diskIsAttached: diskIsAttachedCall{instanceID, nodeName, volumeID, false, diskCheckError},
271271
detach: detachCall{instanceID, volumeID, nil},
272272
test: func(testcase *testcase) (string, error) {
273273
detacher := newDetacher(testcase)
@@ -280,7 +280,7 @@ func TestAttachDetach(t *testing.T) {
280280
name: "Detach_Negative",
281281
instanceID: instanceID,
282282
operationPending: operationPendingCall{volumeID, false, done, nil},
283-
diskIsAttached: diskIsAttachedCall{instanceID, volumeID, false, diskCheckError},
283+
diskIsAttached: diskIsAttachedCall{instanceID, nodeName, volumeID, false, diskCheckError},
284284
detach: detachCall{instanceID, volumeID, detachError},
285285
test: func(testcase *testcase) (string, error) {
286286
detacher := newDetacher(testcase)
@@ -426,6 +426,7 @@ type operationPendingCall struct {
426426

427427
type diskIsAttachedCall struct {
428428
instanceID string
429+
nodeName types.NodeName
429430
volumeID string
430431
isAttached bool
431432
ret error
@@ -440,6 +441,7 @@ type diskPathCall struct {
440441

441442
type disksAreAttachedCall struct {
442443
instanceID string
444+
nodeName types.NodeName
443445
volumeIDs []string
444446
areAttached map[string]bool
445447
ret error
@@ -572,6 +574,46 @@ func (testcase *testcase) ShouldTrustDevicePath() bool {
572574
return true
573575
}
574576

577+
func (testcase *testcase) DiskIsAttachedByName(nodeName types.NodeName, volumeID string) (bool, string, error) {
578+
expected := &testcase.diskIsAttached
579+
instanceID := expected.instanceID
580+
// If testcase call DetachDisk*, return false
581+
if *testcase.attachOrDetach == detachStatus {
582+
return false, instanceID, nil
583+
}
584+
585+
// If testcase call AttachDisk*, return true
586+
if *testcase.attachOrDetach == attachStatus {
587+
return true, instanceID, nil
588+
}
589+
590+
if expected.nodeName != nodeName {
591+
testcase.t.Errorf("Unexpected DiskIsAttachedByName call: expected nodename %s, got %s", expected.nodeName, nodeName)
592+
return false, instanceID, errors.New("Unexpected DiskIsAttachedByName call: wrong nodename")
593+
}
594+
595+
if expected.volumeID == "" && expected.instanceID == "" {
596+
// testcase.diskIsAttached looks uninitialized, test did not expect to
597+
// call DiskIsAttached
598+
testcase.t.Errorf("Unexpected DiskIsAttachedByName call!")
599+
return false, instanceID, errors.New("Unexpected DiskIsAttachedByName call!")
600+
}
601+
602+
if expected.volumeID != volumeID {
603+
testcase.t.Errorf("Unexpected DiskIsAttachedByName call: expected volumeID %s, got %s", expected.volumeID, volumeID)
604+
return false, instanceID, errors.New("Unexpected DiskIsAttachedByName call: wrong volumeID")
605+
}
606+
607+
if expected.instanceID != instanceID {
608+
testcase.t.Errorf("Unexpected DiskIsAttachedByName call: expected instanceID %s, got %s", expected.instanceID, instanceID)
609+
return false, instanceID, errors.New("Unexpected DiskIsAttachedByName call: wrong instanceID")
610+
}
611+
612+
glog.V(4).Infof("DiskIsAttachedByName call: %s, %s, returning %v, %v", volumeID, nodeName, expected.isAttached, expected.instanceID, expected.ret)
613+
614+
return expected.isAttached, expected.instanceID, expected.ret
615+
}
616+
575617
func (testcase *testcase) CreateVolume(name string, size int, vtype, availability string, tags *map[string]string) (string, string, bool, error) {
576618
return "", "", false, errors.New("Not implemented")
577619
}
@@ -626,6 +668,36 @@ func (testcase *testcase) DisksAreAttached(instanceID string, volumeIDs []string
626668
return expected.areAttached, expected.ret
627669
}
628670

671+
func (testcase *testcase) DisksAreAttachedByName(nodeName types.NodeName, volumeIDs []string) (map[string]bool, error) {
672+
expected := &testcase.disksAreAttached
673+
areAttached := make(map[string]bool)
674+
675+
instanceID := expected.instanceID
676+
if expected.nodeName != nodeName {
677+
testcase.t.Errorf("Unexpected DisksAreAttachedByName call: expected nodeName %s, got %s", expected.nodeName, nodeName)
678+
return areAttached, errors.New("Unexpected DisksAreAttachedByName call: wrong nodename")
679+
}
680+
if len(expected.volumeIDs) == 0 && expected.instanceID == "" {
681+
// testcase.volumeIDs looks uninitialized, test did not expect to call DisksAreAttached
682+
testcase.t.Errorf("Unexpected DisksAreAttachedByName call!")
683+
return areAttached, errors.New("Unexpected DisksAreAttachedByName call")
684+
}
685+
686+
if !reflect.DeepEqual(expected.volumeIDs, volumeIDs) {
687+
testcase.t.Errorf("Unexpected DisksAreAttachedByName call: expected volumeIDs %v, got %v", expected.volumeIDs, volumeIDs)
688+
return areAttached, errors.New("Unexpected DisksAreAttachedByName call: wrong volumeID")
689+
}
690+
691+
if expected.instanceID != instanceID {
692+
testcase.t.Errorf("Unexpected DisksAreAttachedByName call: expected instanceID %s, got %s", expected.instanceID, instanceID)
693+
return areAttached, errors.New("Unexpected DisksAreAttachedByName call: wrong instanceID")
694+
}
695+
696+
glog.V(4).Infof("DisksAreAttachedByName call: %v, %s, returning %v, %v", volumeIDs, nodeName, expected.areAttached, expected.ret)
697+
698+
return expected.areAttached, expected.ret
699+
}
700+
629701
// Implementation of fake cloudprovider.Instances
630702
type instances struct {
631703
instanceID string

‎pkg/volume/cinder/cinder.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,8 @@ type CinderProvider interface {
5252
GetAttachmentDiskPath(instanceID, volumeID string) (string, error)
5353
OperationPending(diskName string) (bool, string, error)
5454
DiskIsAttached(instanceID, volumeID string) (bool, error)
55-
DisksAreAttached(instanceID string, volumeIDs []string) (map[string]bool, error)
55+
DiskIsAttachedByName(nodeName types.NodeName, volumeID string) (bool, string, error)
56+
DisksAreAttachedByName(nodeName types.NodeName, volumeIDs []string) (map[string]bool, error)
5657
ShouldTrustDevicePath() bool
5758
Instances() (cloudprovider.Instances, bool)
5859
ExpandVolume(volumeID string, oldSize resource.Quantity, newSize resource.Quantity) (resource.Quantity, error)

0 commit comments

Comments
 (0)