Merge pull request #26615 from jsafrane/gce-attach-tests

Automatic merge from submit-queue

GCE attach tests

Add basic tests for GCE attacher.

Looking at the code, it would deserve some refactoring as suggested in #25888, so mounting is not tested at all.

pkg/cloudprovider/providers/gce/gce.go

@@ -100,6 +100,31 @@ 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
+
+	// 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
+
+	// 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) }

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"
@@ -33,6 +34,7 @@ import (
 
 type gcePersistentDiskAttacher struct {
 	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())
+	attached, err := attacher.gceDisks.DiskIsAttached(pdName, hostName)
-	if err != nil {
-		return err
-	}
-
-	attached, err := gceCloud.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())
+	attached, err := detacher.gceDisks.DiskIsAttached(pdName, hostName)
-	if err != nil {
-		return err
-	}
-
-	attached, err := gceCloud.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
 	}

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")
+}

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{}