From 2b342c1e76e7a8cd3f74c28fe6943bb258355764 Mon Sep 17 00:00:00 2001 From: Jan Safranek Date: Wed, 8 Jun 2016 12:37:08 +0200 Subject: [PATCH 1/2] Add interface to abstract GCE volume operations. We want to write unit test with fake GCE. --- pkg/cloudprovider/providers/gce/gce.go | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/pkg/cloudprovider/providers/gce/gce.go b/pkg/cloudprovider/providers/gce/gce.go index 9acd412732f..223b56e1e83 100644 --- a/pkg/cloudprovider/providers/gce/gce.go +++ b/pkg/cloudprovider/providers/gce/gce.go @@ -100,6 +100,28 @@ type Config struct { } } +// Disks is interface for manipulation with GCE PDs. +type Disks interface { + // AttachDisk attaches given disk to given instance. Current instance + // is used when instanceID is empty string. + AttachDisk(diskName, instanceID string, readOnly bool) error + + // DetachDisk detaches given disk to given instance. Current instance + // is used when instanceID is empty string. + DetachDisk(devicePath, instanceID string) error + + // CreateDisk creates a new PD with given properties. Tags are serialized + // as JSON into Description field. + CreateDisk(name string, zone string, sizeGb int64, tags map[string]string) error + + // DeleteDisk deletes PD. + DeleteDisk(diskToDelete string) error + + // GetAutoLabelsForPD returns labels to apply to PeristentVolume + // representing this PD, namely failure domain and zone. + GetAutoLabelsForPD(name string) (map[string]string, error) +} + type instRefSlice []*compute.InstanceReference func (p instRefSlice) Len() int { return len(p) } From 5cd5ae8d82a57d883032ee3faf159c57d910cff4 Mon Sep 17 00:00:00 2001 From: Jan Safranek Date: Wed, 8 Jun 2016 12:37:08 +0200 Subject: [PATCH 2/2] Add GCE attacher unit tests. --- pkg/cloudprovider/providers/gce/gce.go | 3 + pkg/volume/gce_pd/attacher.go | 43 ++-- pkg/volume/gce_pd/attacher_test.go | 343 +++++++++++++++++++++++++ pkg/volume/gce_pd/gce_util.go | 7 +- 4 files changed, 377 insertions(+), 19 deletions(-) create mode 100644 pkg/volume/gce_pd/attacher_test.go diff --git a/pkg/cloudprovider/providers/gce/gce.go b/pkg/cloudprovider/providers/gce/gce.go index 223b56e1e83..9c197dfc079 100644 --- a/pkg/cloudprovider/providers/gce/gce.go +++ b/pkg/cloudprovider/providers/gce/gce.go @@ -110,6 +110,9 @@ type Disks interface { // is used when instanceID is empty string. DetachDisk(devicePath, instanceID string) error + // DiskIsAttached checks if a disk is attached to the given node. + DiskIsAttached(diskName, instanceID string) (bool, error) + // CreateDisk creates a new PD with given properties. Tags are serialized // as JSON into Description field. CreateDisk(name string, zone string, sizeGb int64, tags map[string]string) error diff --git a/pkg/volume/gce_pd/attacher.go b/pkg/volume/gce_pd/attacher.go index 287aced41a8..e208f2aa690 100644 --- a/pkg/volume/gce_pd/attacher.go +++ b/pkg/volume/gce_pd/attacher.go @@ -25,6 +25,7 @@ import ( "time" "github.com/golang/glog" + "k8s.io/kubernetes/pkg/cloudprovider/providers/gce" "k8s.io/kubernetes/pkg/util/exec" "k8s.io/kubernetes/pkg/util/mount" "k8s.io/kubernetes/pkg/util/sets" @@ -32,7 +33,8 @@ import ( ) type gcePersistentDiskAttacher struct { - host volume.VolumeHost + host volume.VolumeHost + gceDisks gce.Disks } var _ volume.Attacher = &gcePersistentDiskAttacher{} @@ -40,7 +42,15 @@ var _ volume.Attacher = &gcePersistentDiskAttacher{} var _ volume.AttachableVolumePlugin = &gcePersistentDiskPlugin{} func (plugin *gcePersistentDiskPlugin) NewAttacher() (volume.Attacher, error) { - return &gcePersistentDiskAttacher{host: plugin.host}, nil + gceCloud, err := getCloudProvider(plugin.host.GetCloudProvider()) + if err != nil { + return nil, err + } + + return &gcePersistentDiskAttacher{ + host: plugin.host, + gceDisks: gceCloud, + }, nil } func (plugin *gcePersistentDiskPlugin) GetDeviceName(spec *volume.Spec) (string, error) { @@ -63,12 +73,7 @@ func (attacher *gcePersistentDiskAttacher) Attach(spec *volume.Spec, hostName st volumeSource, readOnly := getVolumeSource(spec) pdName := volumeSource.PDName - gceCloud, err := getCloudProvider(attacher.host.GetCloudProvider()) - if err != nil { - return err - } - - attached, err := gceCloud.DiskIsAttached(pdName, hostName) + attached, err := attacher.gceDisks.DiskIsAttached(pdName, hostName) if err != nil { // Log error and continue with attach glog.Errorf( @@ -82,7 +87,7 @@ func (attacher *gcePersistentDiskAttacher) Attach(spec *volume.Spec, hostName st return nil } - if err = gceCloud.AttachDisk(pdName, hostName, readOnly); err != nil { + 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 } @@ -166,13 +171,20 @@ func (attacher *gcePersistentDiskAttacher) MountDevice(spec *volume.Spec, device } type gcePersistentDiskDetacher struct { - host volume.VolumeHost + gceDisks gce.Disks } var _ volume.Detacher = &gcePersistentDiskDetacher{} func (plugin *gcePersistentDiskPlugin) NewDetacher() (volume.Detacher, error) { - return &gcePersistentDiskDetacher{host: plugin.host}, nil + gceCloud, err := getCloudProvider(plugin.host.GetCloudProvider()) + if err != nil { + return nil, err + } + + return &gcePersistentDiskDetacher{ + gceDisks: gceCloud, + }, nil } // Detach checks with the GCE cloud provider if the specified volume is already @@ -185,12 +197,7 @@ func (plugin *gcePersistentDiskPlugin) NewDetacher() (volume.Detacher, error) { func (detacher *gcePersistentDiskDetacher) Detach(deviceMountPath string, hostName string) error { pdName := path.Base(deviceMountPath) - gceCloud, err := getCloudProvider(detacher.host.GetCloudProvider()) - if err != nil { - return err - } - - attached, err := gceCloud.DiskIsAttached(pdName, hostName) + attached, err := detacher.gceDisks.DiskIsAttached(pdName, hostName) if err != nil { // Log error and continue with detach glog.Errorf( @@ -204,7 +211,7 @@ func (detacher *gcePersistentDiskDetacher) Detach(deviceMountPath string, hostNa return nil } - if err = gceCloud.DetachDisk(pdName, hostName); err != nil { + if err = detacher.gceDisks.DetachDisk(pdName, hostName); err != nil { glog.Errorf("Error detaching PD %q from node %q: %v", pdName, hostName, err) return err } diff --git a/pkg/volume/gce_pd/attacher_test.go b/pkg/volume/gce_pd/attacher_test.go new file mode 100644 index 00000000000..ddc93022660 --- /dev/null +++ b/pkg/volume/gce_pd/attacher_test.go @@ -0,0 +1,343 @@ +/* +Copyright 2016 The Kubernetes Authors All rights reserved. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package gce_pd + +import ( + "errors" + "testing" + + "k8s.io/kubernetes/pkg/api" + "k8s.io/kubernetes/pkg/volume" + volumetest "k8s.io/kubernetes/pkg/volume/testing" + + "github.com/golang/glog" +) + +func TestGetDeviceName_Volume(t *testing.T) { + plugin := newPlugin() + name := "my-pd-volume" + spec := createVSpec(name, false) + + deviceName, err := plugin.GetDeviceName(spec) + if err != nil { + t.Errorf("GetDeviceName error: %v", err) + } + if deviceName != name { + t.Errorf("GetDeviceName error: expected %s, got %s", name, deviceName) + } +} + +func TestGetDeviceName_PersistentVolume(t *testing.T) { + plugin := newPlugin() + name := "my-pd-pv" + spec := createPVSpec(name, true) + + deviceName, err := plugin.GetDeviceName(spec) + if err != nil { + t.Errorf("GetDeviceName error: %v", err) + } + if deviceName != name { + t.Errorf("GetDeviceName error: expected %s, got %s", name, deviceName) + } +} + +// One testcase for TestAttachDetach table test below +type testcase struct { + name string + // For fake GCE: + attach attachCall + detach detachCall + diskIsAttached diskIsAttachedCall + t *testing.T + + // Actual test to run + test func(test *testcase) error + // Expected return of the test + expectedReturn error +} + +func TestAttachDetach(t *testing.T) { + diskName := "disk" + instanceID := "instance" + readOnly := false + spec := createVSpec(diskName, readOnly) + attachError := errors.New("Fake attach error") + detachError := errors.New("Fake detach error") + diskCheckError := errors.New("Fake DiskIsAttached error") + tests := []testcase{ + // Successful Attach call + { + name: "Attach_Positive", + diskIsAttached: diskIsAttachedCall{diskName, instanceID, false, nil}, + attach: attachCall{diskName, instanceID, readOnly, nil}, + test: func(testcase *testcase) error { + attacher := newAttacher(testcase) + return attacher.Attach(spec, instanceID) + }, + }, + + // Disk is already attached + { + name: "Attach_Positive_AlreadyAttached", + diskIsAttached: diskIsAttachedCall{diskName, instanceID, true, nil}, + test: func(testcase *testcase) error { + attacher := newAttacher(testcase) + return attacher.Attach(spec, instanceID) + }, + }, + + // DiskIsAttached fails and Attach succeeds + { + name: "Attach_Positive_CheckFails", + diskIsAttached: diskIsAttachedCall{diskName, instanceID, false, diskCheckError}, + attach: attachCall{diskName, instanceID, readOnly, nil}, + test: func(testcase *testcase) error { + attacher := newAttacher(testcase) + return attacher.Attach(spec, instanceID) + }, + }, + + // Attach call fails + { + name: "Attach_Negative", + diskIsAttached: diskIsAttachedCall{diskName, instanceID, false, diskCheckError}, + attach: attachCall{diskName, instanceID, readOnly, attachError}, + test: func(testcase *testcase) error { + attacher := newAttacher(testcase) + return attacher.Attach(spec, instanceID) + }, + expectedReturn: attachError, + }, + + // Detach succeeds + { + name: "Detach_Positive", + diskIsAttached: diskIsAttachedCall{diskName, instanceID, true, nil}, + detach: detachCall{diskName, instanceID, nil}, + test: func(testcase *testcase) error { + detacher := newDetacher(testcase) + return detacher.Detach(diskName, instanceID) + }, + }, + + // Disk is already detached + { + name: "Detach_Positive_AlreadyDetached", + diskIsAttached: diskIsAttachedCall{diskName, instanceID, false, nil}, + test: func(testcase *testcase) error { + detacher := newDetacher(testcase) + return detacher.Detach(diskName, instanceID) + }, + }, + + // Detach succeeds when DiskIsAttached fails + { + name: "Detach_Positive_CheckFails", + diskIsAttached: diskIsAttachedCall{diskName, instanceID, false, diskCheckError}, + detach: detachCall{diskName, instanceID, nil}, + test: func(testcase *testcase) error { + detacher := newDetacher(testcase) + return detacher.Detach(diskName, instanceID) + }, + }, + + // Detach fails + { + name: "Detach_Negative", + diskIsAttached: diskIsAttachedCall{diskName, instanceID, false, diskCheckError}, + detach: detachCall{diskName, instanceID, detachError}, + test: func(testcase *testcase) error { + detacher := newDetacher(testcase) + return detacher.Detach(diskName, instanceID) + }, + expectedReturn: detachError, + }, + } + + for _, testcase := range tests { + testcase.t = t + err := testcase.test(&testcase) + if err != testcase.expectedReturn { + t.Errorf("%s failed: expected err=%q, got %q", testcase.name, testcase.expectedReturn.Error(), err.Error()) + } + t.Logf("Test %q succeeded", testcase.name) + } +} + +// newPlugin creates a new gcePersistentDiskPlugin with fake cloud, NewAttacher +// and NewDetacher won't work. +func newPlugin() *gcePersistentDiskPlugin { + host := volumetest.NewFakeVolumeHost("/tmp", nil, nil) + plugins := ProbeVolumePlugins() + plugin := plugins[0] + plugin.Init(host) + return plugin.(*gcePersistentDiskPlugin) +} + +func newAttacher(testcase *testcase) *gcePersistentDiskAttacher { + return &gcePersistentDiskAttacher{ + host: nil, + gceDisks: testcase, + } +} + +func newDetacher(testcase *testcase) *gcePersistentDiskDetacher { + return &gcePersistentDiskDetacher{ + gceDisks: testcase, + } +} + +func createVSpec(name string, readOnly bool) *volume.Spec { + return &volume.Spec{ + Volume: &api.Volume{ + VolumeSource: api.VolumeSource{ + GCEPersistentDisk: &api.GCEPersistentDiskVolumeSource{ + PDName: name, + ReadOnly: readOnly, + }, + }, + }, + } +} + +func createPVSpec(name string, readOnly bool) *volume.Spec { + return &volume.Spec{ + PersistentVolume: &api.PersistentVolume{ + Spec: api.PersistentVolumeSpec{ + PersistentVolumeSource: api.PersistentVolumeSource{ + GCEPersistentDisk: &api.GCEPersistentDiskVolumeSource{ + PDName: name, + ReadOnly: readOnly, + }, + }, + }, + }, + } +} + +// Fake GCE implementation + +type attachCall struct { + diskName string + instanceID string + readOnly bool + ret error +} + +type detachCall struct { + devicePath string + instanceID string + ret error +} + +type diskIsAttachedCall struct { + diskName, instanceID string + isAttached bool + ret error +} + +func (testcase *testcase) AttachDisk(diskName, instanceID string, readOnly bool) error { + expected := &testcase.attach + + if expected.diskName == "" && expected.instanceID == "" { + // testcase.attach looks uninitialized, test did not expect to call + // AttachDisk + testcase.t.Errorf("Unexpected AttachDisk call!") + return errors.New("Unexpected AttachDisk call!") + } + + if expected.diskName != diskName { + testcase.t.Errorf("Unexpected AttachDisk call: expected diskName %s, got %s", expected.diskName, diskName) + return errors.New("Unexpected AttachDisk call: wrong diskName") + } + + if expected.instanceID != instanceID { + testcase.t.Errorf("Unexpected AttachDisk call: expected instanceID %s, got %s", expected.instanceID, instanceID) + return errors.New("Unexpected AttachDisk call: wrong instanceID") + } + + if expected.readOnly != readOnly { + testcase.t.Errorf("Unexpected AttachDisk call: expected readOnly %v, got %v", expected.readOnly, readOnly) + return errors.New("Unexpected AttachDisk call: wrong readOnly") + } + + glog.V(4).Infof("AttachDisk call: %s, %s, %v, returning %v", diskName, instanceID, readOnly, expected.ret) + + return expected.ret +} + +func (testcase *testcase) DetachDisk(devicePath, instanceID string) error { + expected := &testcase.detach + + if expected.devicePath == "" && expected.instanceID == "" { + // testcase.detach looks uninitialized, test did not expect to call + // DetachDisk + testcase.t.Errorf("Unexpected DetachDisk call!") + return errors.New("Unexpected DetachDisk call!") + } + + if expected.devicePath != devicePath { + testcase.t.Errorf("Unexpected DetachDisk call: expected devicePath %s, got %s", expected.devicePath, devicePath) + return errors.New("Unexpected DetachDisk call: wrong diskName") + } + + if expected.instanceID != instanceID { + testcase.t.Errorf("Unexpected DetachDisk call: expected instanceID %s, got %s", expected.instanceID, instanceID) + return errors.New("Unexpected DetachDisk call: wrong instanceID") + } + + glog.V(4).Infof("DetachDisk call: %s, %s, returning %v", devicePath, instanceID, expected.ret) + + return expected.ret +} + +func (testcase *testcase) DiskIsAttached(diskName, instanceID string) (bool, error) { + expected := &testcase.diskIsAttached + + if expected.diskName == "" && expected.instanceID == "" { + // testcase.diskIsAttached looks uninitialized, test did not expect to + // call DiskIsAttached + testcase.t.Errorf("Unexpected DiskIsAttached call!") + return false, errors.New("Unexpected DiskIsAttached call!") + } + + if expected.diskName != diskName { + testcase.t.Errorf("Unexpected DiskIsAttached call: expected diskName %s, got %s", expected.diskName, diskName) + return false, errors.New("Unexpected DiskIsAttached call: wrong diskName") + } + + if expected.instanceID != instanceID { + testcase.t.Errorf("Unexpected DiskIsAttached call: expected instanceID %s, got %s", expected.instanceID, instanceID) + return false, errors.New("Unexpected DiskIsAttached call: wrong instanceID") + } + + glog.V(4).Infof("DiskIsAttached call: %s, %s, returning %v, %v", diskName, instanceID, expected.isAttached, expected.ret) + + return expected.isAttached, expected.ret +} + +func (testcase *testcase) CreateDisk(name string, zone string, sizeGb int64, tags map[string]string) error { + return errors.New("Not implemented") +} + +func (testcase *testcase) DeleteDisk(diskToDelete string) error { + return errors.New("Not implemented") +} + +func (testcase *testcase) GetAutoLabelsForPD(name string) (map[string]string, error) { + return map[string]string{}, errors.New("Not implemented") +} diff --git a/pkg/volume/gce_pd/gce_util.go b/pkg/volume/gce_pd/gce_util.go index 523262720aa..bb8b31da4bd 100644 --- a/pkg/volume/gce_pd/gce_util.go +++ b/pkg/volume/gce_pd/gce_util.go @@ -43,7 +43,12 @@ const ( maxChecks = 60 maxRetries = 10 checkSleepDuration = time.Second - errorSleepDuration = 5 * time.Second +) + +// These variables are modified only in unit tests and should be constant +// otherwise. +var ( + errorSleepDuration time.Duration = 5 * time.Second ) type GCEDiskUtil struct{}