From 2141b0fb805dc30137dda37f1b37b46976b6379e Mon Sep 17 00:00:00 2001 From: Cheng Xing Date: Fri, 21 Apr 2017 14:36:07 -0700 Subject: [PATCH] Created unit tests for GCE cloud provider storage interface. - Currently covers CreateDisk and DeleteDisk, GetAutoLabelsForPD - Created ServiceManager interface in gce.go to facilitate mocking in tests. --- pkg/cloudprovider/providers/gce/BUILD | 11 +- pkg/cloudprovider/providers/gce/gce.go | 54 +- pkg/cloudprovider/providers/gce/gce_disks.go | 10 +- .../providers/gce/gce_disks_test.go | 502 ++++++++++++++++++ 4 files changed, 569 insertions(+), 8 deletions(-) create mode 100644 pkg/cloudprovider/providers/gce/gce_disks_test.go diff --git a/pkg/cloudprovider/providers/gce/BUILD b/pkg/cloudprovider/providers/gce/BUILD index ea1367033e0..ee79c5d6681 100644 --- a/pkg/cloudprovider/providers/gce/BUILD +++ b/pkg/cloudprovider/providers/gce/BUILD @@ -68,9 +68,18 @@ go_library( go_test( name = "go_default_test", - srcs = ["gce_test.go"], + srcs = [ + "gce_disks_test.go", + "gce_test.go", + ], library = ":go_default_library", tags = ["automanaged"], + deps = [ + "//pkg/cloudprovider:go_default_library", + "//vendor/google.golang.org/api/compute/v1:go_default_library", + "//vendor/google.golang.org/api/googleapi:go_default_library", + "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", + ], ) filegroup( diff --git a/pkg/cloudprovider/providers/gce/gce.go b/pkg/cloudprovider/providers/gce/gce.go index 1200fdc8d60..0dbda893b5b 100644 --- a/pkg/cloudprovider/providers/gce/gce.go +++ b/pkg/cloudprovider/providers/gce/gce.go @@ -90,6 +90,25 @@ type GCECloud struct { nodeInstancePrefix string // If non-"", an advisory prefix for all nodes in the cluster useMetadataServer bool operationPollRateLimiter flowcontrol.RateLimiter + manager ServiceManager +} + +type ServiceManager interface { + // Creates a new persistent disk on GCE with the given disk spec. + CreateDisk(project string, zone string, disk *compute.Disk) (*compute.Operation, error) + + // Gets the persistent disk from GCE with the given diskName. + GetDisk(project string, zone string, diskName string) (*compute.Disk, error) + + // Deletes the persistent disk from GCE with the given diskName. + DeleteDisk(project string, zone string, disk string) (*compute.Operation, error) + + // Waits until GCE reports the given operation in the given zone as done. + WaitForZoneOp(op *compute.Operation, zone string, mc *metricContext) error +} + +type GCEServiceManager struct { + gce *GCECloud } type Config struct { @@ -220,7 +239,7 @@ func CreateGCECloud(projectID, region, zone string, managedZones []string, netwo operationPollRateLimiter := flowcontrol.NewTokenBucketRateLimiter(10, 100) // 10 qps, 100 bucket size. - return &GCECloud{ + gce := &GCECloud{ service: service, serviceBeta: serviceBeta, containerService: containerService, @@ -233,7 +252,10 @@ func CreateGCECloud(projectID, region, zone string, managedZones []string, netwo nodeInstancePrefix: nodeInstancePrefix, useMetadataServer: useMetadataServer, operationPollRateLimiter: operationPollRateLimiter, - }, nil + } + + gce.manager = &GCEServiceManager{gce} + return gce, nil } // Initialize takes in a clientBuilder and spawns a goroutine for watching the clusterid configmap. @@ -368,3 +390,31 @@ func newOauthClient(tokenSource oauth2.TokenSource) (*http.Client, error) { return oauth2.NewClient(oauth2.NoContext, tokenSource), nil } + +func (manager *GCEServiceManager) CreateDisk( + project string, + zone string, + disk *compute.Disk) (*compute.Operation, error) { + + return manager.gce.service.Disks.Insert(project, zone, disk).Do() +} + +func (manager *GCEServiceManager) GetDisk( + project string, + zone string, + diskName string) (*compute.Disk, error) { + + return manager.gce.service.Disks.Get(project, zone, diskName).Do() +} + +func (manager *GCEServiceManager) DeleteDisk( + project string, + zone string, + diskName string) (*compute.Operation, error) { + + return manager.gce.service.Disks.Delete(project, zone, diskName).Do() +} + +func (manager *GCEServiceManager) WaitForZoneOp(op *compute.Operation, zone string, mc *metricContext) error { + return manager.gce.waitForZoneOp(op, zone, mc) +} diff --git a/pkg/cloudprovider/providers/gce/gce_disks.go b/pkg/cloudprovider/providers/gce/gce_disks.go index fab96f9081e..9f0ea55205e 100644 --- a/pkg/cloudprovider/providers/gce/gce_disks.go +++ b/pkg/cloudprovider/providers/gce/gce_disks.go @@ -244,7 +244,7 @@ func (gce *GCECloud) CreateDisk( } mc := newDiskMetricContext("create", zone) - createOp, err := gce.service.Disks.Insert(gce.projectID, zone, diskToCreate).Do() + createOp, err := gce.manager.CreateDisk(gce.projectID, zone, diskToCreate) if isGCEError(err, "alreadyExists") { glog.Warningf("GCE PD %q already exists, reusing", name) return nil @@ -252,7 +252,7 @@ func (gce *GCECloud) CreateDisk( return mc.Observe(err) } - err = gce.waitForZoneOp(createOp, zone, mc) + err = gce.manager.WaitForZoneOp(createOp, zone, mc) if isGCEError(err, "alreadyExists") { glog.Warningf("GCE PD %q already exists, reusing", name) return nil @@ -323,7 +323,7 @@ func (gce *GCECloud) GetAutoLabelsForPD(name string, zone string) (map[string]st // If not found, returns (nil, nil) func (gce *GCECloud) findDiskByName(diskName string, zone string) (*GCEDisk, error) { mc := newDiskMetricContext("get", zone) - disk, err := gce.service.Disks.Get(gce.projectID, zone, diskName).Do() + disk, err := gce.manager.GetDisk(gce.projectID, zone, diskName) if err == nil { d := &GCEDisk{ Zone: lastComponent(disk.Zone), @@ -410,12 +410,12 @@ func (gce *GCECloud) doDeleteDisk(diskToDelete string) error { mc := newDiskMetricContext("delete", disk.Zone) - deleteOp, err := gce.service.Disks.Delete(gce.projectID, disk.Zone, disk.Name).Do() + deleteOp, err := gce.manager.DeleteDisk(gce.projectID, disk.Zone, disk.Name) if err != nil { return mc.Observe(err) } - return gce.waitForZoneOp(deleteOp, disk.Zone, mc) + return gce.manager.WaitForZoneOp(deleteOp, disk.Zone, mc) } // Converts a Disk resource to an AttachedDisk resource. diff --git a/pkg/cloudprovider/providers/gce/gce_disks_test.go b/pkg/cloudprovider/providers/gce/gce_disks_test.go new file mode 100644 index 00000000000..0c27977138a --- /dev/null +++ b/pkg/cloudprovider/providers/gce/gce_disks_test.go @@ -0,0 +1,502 @@ +/* +Copyright 2017 The Kubernetes Authors. + +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 + +import ( + "testing" + + "fmt" + compute "google.golang.org/api/compute/v1" + "google.golang.org/api/googleapi" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/kubernetes/pkg/cloudprovider" +) + +func TestCreateDisk_Basic(t *testing.T) { + /* Arrange */ + fakeManager := newFakeManager() + projectId := "test-project" + gce := GCECloud{ + manager: fakeManager, + managedZones: []string{"zone1"}, + projectID: projectId, + } + + diskName := "disk" + diskType := DiskTypeSSD + zone := "zone1" + const sizeGb int64 = 128 + tags := make(map[string]string) + tags["test-tag"] = "test-value" + + diskTypeUri := fmt.Sprintf(diskTypeUriTemplate, projectId, zone, diskType) + expectedDescription := "{\"test-tag\":\"test-value\"}" + + /* Act */ + err := gce.CreateDisk(diskName, diskType, zone, sizeGb, tags) + + /* Assert */ + if err != nil { + t.Error(err) + } + if !fakeManager.createDiskCalled { + t.Error("Never called GCE disk create.") + } + if !fakeManager.doesOpMatch { + t.Error("Ops used in WaitForZoneOp does not match what's returned by CreateDisk.") + } + + // Partial check of equality between disk description sent to GCE and parameters of method. + diskToCreate := fakeManager.diskToCreate + if diskToCreate.Name != diskName { + t.Errorf("Expected disk name: %s; Actual: %s", diskName, diskToCreate.Name) + } + + if diskToCreate.Type != diskTypeUri { + t.Errorf("Expected disk type: %s; Actual: %s", diskTypeUri, diskToCreate.Type) + } + if diskToCreate.SizeGb != sizeGb { + t.Errorf("Expected disk size: %d; Actual: %d", sizeGb, diskToCreate.SizeGb) + } + if diskToCreate.Description != expectedDescription { + t.Errorf("Expected tag string: %s; Actual: %s", expectedDescription, diskToCreate.Description) + } +} + +func TestCreateDisk_DiskAlreadyExists(t *testing.T) { + /* Arrange */ + fakeManager := newFakeManager() + gce := GCECloud{manager: fakeManager, managedZones: []string{"zone1"}} + + // Inject disk AlreadyExists error. + alreadyExistsError := googleapi.ErrorItem{Reason: "alreadyExists"} + fakeManager.waitForZoneOpError = &googleapi.Error{ + Errors: []googleapi.ErrorItem{alreadyExistsError}, + } + + /* Act */ + err := gce.CreateDisk("disk", DiskTypeSSD, "zone1", 128, nil) + + /* Assert */ + if err != nil { + t.Error( + "Expected success when a disk with the given name already exists, but an error is returned.") + } +} + +func TestCreateDisk_WrongZone(t *testing.T) { + /* Arrange */ + fakeManager := newFakeManager() + gce := GCECloud{manager: fakeManager, managedZones: []string{"zone1"}} + + diskName := "disk" + diskType := DiskTypeSSD + const sizeGb int64 = 128 + + /* Act */ + err := gce.CreateDisk(diskName, diskType, "zone2", sizeGb, nil) + + /* Assert */ + if err == nil { + t.Error("Expected error when zone is not managed, but none returned.") + } +} + +func TestCreateDisk_NoManagedZone(t *testing.T) { + /* Arrange */ + fakeManager := newFakeManager() + gce := GCECloud{manager: fakeManager, managedZones: []string{}} + + diskName := "disk" + diskType := DiskTypeSSD + const sizeGb int64 = 128 + + /* Act */ + err := gce.CreateDisk(diskName, diskType, "zone1", sizeGb, nil) + + /* Assert */ + if err == nil { + t.Error("Expected error when managedZones is empty, but none returned.") + } +} + +func TestCreateDisk_BadDiskType(t *testing.T) { + /* Arrange */ + fakeManager := newFakeManager() + gce := GCECloud{manager: fakeManager, managedZones: []string{"zone1"}} + + diskName := "disk" + diskType := "arbitrary-disk" + zone := "zone1" + const sizeGb int64 = 128 + + /* Act */ + err := gce.CreateDisk(diskName, diskType, zone, sizeGb, nil) + + /* Assert */ + if err == nil { + t.Error("Expected error when disk type is not supported, but none returned.") + } +} + +func TestCreateDisk_MultiZone(t *testing.T) { + /* Arrange */ + fakeManager := newFakeManager() + gce := GCECloud{manager: fakeManager, managedZones: []string{"zone1", "zone2", "zone3"}} + + diskName := "disk" + diskType := DiskTypeStandard + const sizeGb int64 = 128 + + /* Act & Assert */ + for _, zone := range gce.managedZones { + diskName = zone + "disk" + err := gce.CreateDisk(diskName, diskType, zone, sizeGb, nil) + if err != nil { + t.Errorf("Error creating disk in zone '%v'; error: \"%v\"", zone, err) + } + } +} + +func TestDeleteDisk_Basic(t *testing.T) { + /* Arrange */ + fakeManager := newFakeManager() + gce := GCECloud{manager: fakeManager, managedZones: []string{"zone1"}} + diskName := "disk" + diskType := DiskTypeSSD + zone := "zone1" + const sizeGb int64 = 128 + + gce.CreateDisk(diskName, diskType, zone, sizeGb, nil) + + /* Act */ + err := gce.DeleteDisk(diskName) + + /* Assert */ + if err != nil { + t.Error(err) + } + if !fakeManager.deleteDiskCalled { + t.Error("Never called GCE disk delete.") + } + if !fakeManager.doesOpMatch { + t.Error("Ops used in WaitForZoneOp does not match what's returned by DeleteDisk.") + } + +} + +func TestDeleteDisk_NotFound(t *testing.T) { + /* Arrange */ + fakeManager := newFakeManager() + gce := GCECloud{manager: fakeManager, managedZones: []string{"zone1"}} + diskName := "disk" + + /* Act */ + err := gce.DeleteDisk(diskName) + + /* Assert */ + if err != nil { + t.Error("Expected successful operation when disk is not found, but an error is returned.") + } +} + +func TestDeleteDisk_ResourceBeingUsed(t *testing.T) { + /* Arrange */ + fakeManager := newFakeManager() + gce := GCECloud{manager: fakeManager, managedZones: []string{"zone1"}} + diskName := "disk" + diskType := DiskTypeSSD + zone := "zone1" + const sizeGb int64 = 128 + + gce.CreateDisk(diskName, diskType, zone, sizeGb, nil) + fakeManager.resourceInUse = true + + /* Act */ + err := gce.DeleteDisk(diskName) + + /* Assert */ + if err == nil { + t.Error("Expected error when disk is in use, but none returned.") + } +} + +func TestDeleteDisk_SameDiskMultiZone(t *testing.T) { + /* Assert */ + fakeManager := newFakeManager() + gce := GCECloud{manager: fakeManager, managedZones: []string{"zone1", "zone2", "zone3"}} + diskName := "disk" + diskType := DiskTypeSSD + const sizeGb int64 = 128 + + for _, zone := range gce.managedZones { + gce.CreateDisk(diskName, diskType, zone, sizeGb, nil) + } + + /* Act */ + // DeleteDisk will call FakeServiceManager.GetDisk() with all zones, + // and FakeServiceManager.GetDisk() always returns a disk, + // so DeleteDisk thinks a disk with diskName exists in all zones. + err := gce.DeleteDisk(diskName) + + /* Assert */ + if err == nil { + t.Error("Expected error when disk is found in multiple zones, but none returned.") + } +} + +func TestDeleteDisk_DiffDiskMultiZone(t *testing.T) { + /* Arrange */ + fakeManager := newFakeManager() + gce := GCECloud{manager: fakeManager, managedZones: []string{"zone1"}} + diskName := "disk" + diskType := DiskTypeSSD + const sizeGb int64 = 128 + + for _, zone := range gce.managedZones { + diskName = zone + "disk" + gce.CreateDisk(diskName, diskType, zone, sizeGb, nil) + } + + /* Act & Assert */ + var err error + for _, zone := range gce.managedZones { + diskName = zone + "disk" + err = gce.DeleteDisk(diskName) + if err != nil { + t.Errorf("Error deleting disk in zone '%v'; error: \"%v\"", zone, err) + } + } +} + +func TestGetAutoLabelsForPD_Basic(t *testing.T) { + /* Arrange */ + fakeManager := newFakeManager() + diskName := "disk" + diskType := DiskTypeSSD + zone := "us-central1-c" + const sizeGb int64 = 128 + gce := GCECloud{manager: fakeManager, managedZones: []string{zone}} + + gce.CreateDisk(diskName, diskType, zone, sizeGb, nil) + + /* Act */ + labels, err := gce.GetAutoLabelsForPD(diskName, zone) + + /* Assert */ + if err != nil { + t.Error(err) + } + if labels[metav1.LabelZoneFailureDomain] != zone { + t.Errorf("Failure domain is '%v', but zone is '%v'", + labels[metav1.LabelZoneFailureDomain], zone) + } + if labels[metav1.LabelZoneRegion] != "us-central1" { + t.Errorf("Region is '%v', but zone is 'us-central1'", labels[metav1.LabelZoneRegion]) + } +} + +func TestGetAutoLabelsForPD_NoZone(t *testing.T) { + /* Arrange */ + fakeManager := newFakeManager() + diskName := "disk" + diskType := DiskTypeStandard + zone := "europe-west1-d" + const sizeGb int64 = 128 + gce := GCECloud{manager: fakeManager, managedZones: []string{zone}} + gce.CreateDisk(diskName, diskType, zone, sizeGb, nil) + + /* Act */ + labels, err := gce.GetAutoLabelsForPD(diskName, "") + + /* Assert */ + if err != nil { + t.Error(err) + } + if labels[metav1.LabelZoneFailureDomain] != zone { + t.Errorf("Failure domain is '%v', but zone is '%v'", + labels[metav1.LabelZoneFailureDomain], zone) + } + if labels[metav1.LabelZoneRegion] != "europe-west1" { + t.Errorf("Region is '%v', but zone is 'europe-west1'", labels[metav1.LabelZoneRegion]) + } +} + +func TestGetAutoLabelsForPD_DiskNotFound(t *testing.T) { + /* Arrange */ + fakeManager := newFakeManager() + diskName := "disk" + zone := "asia-northeast1-a" + gce := GCECloud{manager: fakeManager, managedZones: []string{zone}} + + /* Act */ + _, err := gce.GetAutoLabelsForPD(diskName, zone) + + /* Assert */ + if err == nil { + t.Error("Expected error when the specified disk does not exist, but none returned.") + } +} + +func TestGetAutoLabelsForPD_DiskNotFoundAndNoZone(t *testing.T) { + /* Arrange */ + fakeManager := newFakeManager() + diskName := "disk" + gce := GCECloud{manager: fakeManager, managedZones: []string{}} + + /* Act */ + _, err := gce.GetAutoLabelsForPD(diskName, "") + + /* Assert */ + if err == nil { + t.Error("Expected error when the specified disk does not exist, but none returned.") + } +} + +func TestGetAutoLabelsForPD_DupDisk(t *testing.T) { + /* Arrange */ + fakeManager := newFakeManager() + diskName := "disk" + diskType := DiskTypeStandard + zone := "us-west1-b" + const sizeGb int64 = 128 + + gce := GCECloud{manager: fakeManager, managedZones: []string{"us-west1-b", "asia-southeast1-a"}} + for _, zone := range gce.managedZones { + gce.CreateDisk(diskName, diskType, zone, sizeGb, nil) + } + + /* Act */ + labels, err := gce.GetAutoLabelsForPD(diskName, zone) + + /* Assert */ + if err != nil { + t.Error("Disk name and zone uniquely identifies a disk, yet an error is returned.") + } + if labels[metav1.LabelZoneFailureDomain] != zone { + t.Errorf("Failure domain is '%v', but zone is '%v'", + labels[metav1.LabelZoneFailureDomain], zone) + } + if labels[metav1.LabelZoneRegion] != "us-west1" { + t.Errorf("Region is '%v', but zone is 'us-west1'", labels[metav1.LabelZoneRegion]) + } +} + +func TestGetAutoLabelsForPD_DupDiskNoZone(t *testing.T) { + /* Arrange */ + fakeManager := newFakeManager() + diskName := "disk" + diskType := DiskTypeStandard + const sizeGb int64 = 128 + + gce := GCECloud{manager: fakeManager, managedZones: []string{"us-west1-b", "asia-southeast1-a"}} + for _, zone := range gce.managedZones { + gce.CreateDisk(diskName, diskType, zone, sizeGb, nil) + } + + /* Act */ + _, err := gce.GetAutoLabelsForPD(diskName, "") + + /* Assert */ + if err == nil { + t.Error("Expected error when the disk is duplicated and zone is not specified, but none returned.") + } +} + +type FakeServiceManager struct { + // Common fields shared among tests + op *compute.Operation // Mocks an operation returned by GCE API calls + doesOpMatch bool + disks map[string]string // zone: diskName + waitForZoneOpError error // Error to be returned by WaitForZoneOp + + // Fields for TestCreateDisk + createDiskCalled bool + diskToCreate *compute.Disk + + // Fields for TestDeleteDisk + deleteDiskCalled bool + resourceInUse bool // Marks the disk as in-use +} + +func newFakeManager() *FakeServiceManager { + return &FakeServiceManager{disks: make(map[string]string)} +} + +/** + * Upon disk creation, disk info is stored in FakeServiceManager + * to be used by other tested methods. + */ +func (manager *FakeServiceManager) CreateDisk( + project string, + zone string, + disk *compute.Disk) (*compute.Operation, error) { + + manager.createDiskCalled = true + op := &compute.Operation{} + manager.op = op + manager.diskToCreate = disk + manager.disks[zone] = disk.Name + return op, nil +} + +/** + * Gets disk info stored in the FakeServiceManager. + */ +func (manager *FakeServiceManager) GetDisk( + project string, + zone string, + diskName string) (*compute.Disk, error) { + + if manager.disks[zone] == "" { + return nil, cloudprovider.DiskNotFound + } + + if manager.resourceInUse { + errorItem := googleapi.ErrorItem{Reason: "resourceInUseByAnotherResource"} + err := &googleapi.Error{Errors: []googleapi.ErrorItem{errorItem}} + return nil, err + } + + disk := &compute.Disk{Name: diskName, Zone: zone, Kind: "compute#disk"} + return disk, nil +} + +/** + * Disk info is removed from the FakeServiceManager. + */ +func (manager *FakeServiceManager) DeleteDisk( + project string, + zone string, + disk string) (*compute.Operation, error) { + + manager.deleteDiskCalled = true + op := &compute.Operation{} + manager.op = op + manager.disks[zone] = "" + return op, nil +} + +func (manager *FakeServiceManager) WaitForZoneOp( + op *compute.Operation, + zone string, + mc *metricContext) error { + if op == manager.op { + manager.doesOpMatch = true + } + return manager.waitForZoneOpError +}