From 2ccf1ac96b05cbfd9e05921e478356de45c2d5ca Mon Sep 17 00:00:00 2001 From: David Zhu Date: Tue, 10 Jul 2018 15:39:36 -0700 Subject: [PATCH] Add controller unit tests and move some helper functions around --- pkg/gce-pd-csi-driver/controller.go | 117 ++++--- pkg/gce-pd-csi-driver/controller_test.go | 354 +++++++++++++++----- pkg/gce-pd-csi-driver/gce-pd-driver_test.go | 20 ++ pkg/gce-pd-csi-driver/identity_test.go | 7 +- test/e2e/gce_pd_e2e_test.go | 4 + 5 files changed, 362 insertions(+), 140 deletions(-) diff --git a/pkg/gce-pd-csi-driver/controller.go b/pkg/gce-pd-csi-driver/controller.go index 249fe3644..0f3879d96 100644 --- a/pkg/gce-pd-csi-driver/controller.go +++ b/pkg/gce-pd-csi-driver/controller.go @@ -52,40 +52,6 @@ const ( attachableDiskTypePersistent = "PERSISTENT" ) -func getRequestCapacity(capRange *csi.CapacityRange) (int64, error) { - // TODO: Take another look at these casts/caps. Make sure this func is correct - var capBytes int64 - // Default case where nothing is set - if capRange == nil { - capBytes = MinimumVolumeSizeInBytes - return capBytes, nil - } - - rBytes := capRange.GetRequiredBytes() - rSet := rBytes > 0 - lBytes := capRange.GetLimitBytes() - lSet := lBytes > 0 - - if lSet && rSet && lBytes < rBytes { - return 0, fmt.Errorf("Limit bytes %v is less than required bytes %v", lBytes, rBytes) - } - if lSet && lBytes < MinimumVolumeSizeInBytes { - return 0, fmt.Errorf("Limit bytes %v is less than minimum volume size: %v", lBytes, MinimumVolumeSizeInBytes) - } - - // If Required set just set capacity to that which is Required - if rSet { - capBytes = rBytes - } - - // Limit is more than Required, but larger than Minimum. So we just set capcity to Minimum - // Too small, default - if capBytes < MinimumVolumeSizeInBytes { - capBytes = MinimumVolumeSizeInBytes - } - return capBytes, nil -} - func (gceCS *GCEControllerServer) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest) (*csi.CreateVolumeResponse, error) { // TODO: Check create zone against Driver zone. They must MATCH glog.Infof("CreateVolume called with request %v", *req) @@ -371,31 +337,6 @@ func (gceCS *GCEControllerServer) ControllerUnpublishVolume(ctx context.Context, return &csi.ControllerUnpublishVolumeResponse{}, nil } -// TODO: This abstraction isn't great. We shouldn't need diskIsAttached AND diskIsAttachedAndCompatible to duplicate code -func diskIsAttached(volume *compute.Disk, instance *compute.Instance) bool { - for _, disk := range instance.Disks { - if disk.DeviceName == volume.Name { - // Disk is attached to node - return true - } - } - return false -} - -func diskIsAttachedAndCompatible(volume *compute.Disk, instance *compute.Instance, volumeCapability *csi.VolumeCapability, readWrite string) (bool, error) { - for _, disk := range instance.Disks { - if disk.DeviceName == volume.Name { - // Disk is attached to node - if disk.Mode != readWrite { - return true, fmt.Errorf("disk mode does not match. Got %v. Want %v", disk.Mode, readWrite) - } - // TODO: Check volume_capability. - return true, nil - } - } - return false, nil -} - func (gceCS *GCEControllerServer) ValidateVolumeCapabilities(ctx context.Context, req *csi.ValidateVolumeCapabilitiesRequest) (*csi.ValidateVolumeCapabilitiesResponse, error) { // TODO: Factor out the volume capability functionality and use as validation in all other functions as well glog.V(5).Infof("Using default ValidateVolumeCapabilities") @@ -459,3 +400,61 @@ func (gceCS *GCEControllerServer) DeleteSnapshot(ctx context.Context, req *csi.D func (gceCS *GCEControllerServer) ListSnapshots(ctx context.Context, req *csi.ListSnapshotsRequest) (*csi.ListSnapshotsResponse, error) { return nil, status.Error(codes.Unimplemented, "") } + +func getRequestCapacity(capRange *csi.CapacityRange) (int64, error) { + // TODO: Take another look at these casts/caps. Make sure this func is correct + var capBytes int64 + // Default case where nothing is set + if capRange == nil { + capBytes = MinimumVolumeSizeInBytes + return capBytes, nil + } + + rBytes := capRange.GetRequiredBytes() + rSet := rBytes > 0 + lBytes := capRange.GetLimitBytes() + lSet := lBytes > 0 + + if lSet && rSet && lBytes < rBytes { + return 0, fmt.Errorf("Limit bytes %v is less than required bytes %v", lBytes, rBytes) + } + if lSet && lBytes < MinimumVolumeSizeInBytes { + return 0, fmt.Errorf("Limit bytes %v is less than minimum volume size: %v", lBytes, MinimumVolumeSizeInBytes) + } + + // If Required set just set capacity to that which is Required + if rSet { + capBytes = rBytes + } + + // Limit is more than Required, but larger than Minimum. So we just set capcity to Minimum + // Too small, default + if capBytes < MinimumVolumeSizeInBytes { + capBytes = MinimumVolumeSizeInBytes + } + return capBytes, nil +} + +func diskIsAttached(volume *compute.Disk, instance *compute.Instance) bool { + for _, disk := range instance.Disks { + if disk.DeviceName == volume.Name { + // Disk is attached to node + return true + } + } + return false +} + +func diskIsAttachedAndCompatible(volume *compute.Disk, instance *compute.Instance, volumeCapability *csi.VolumeCapability, readWrite string) (bool, error) { + for _, disk := range instance.Disks { + if disk.DeviceName == volume.Name { + // Disk is attached to node + if disk.Mode != readWrite { + return true, fmt.Errorf("disk mode does not match. Got %v. Want %v", disk.Mode, readWrite) + } + // TODO: Check volume_capability. + return true, nil + } + } + return false, nil +} diff --git a/pkg/gce-pd-csi-driver/controller_test.go b/pkg/gce-pd-csi-driver/controller_test.go index fb4fe0a9e..3d91c02c6 100644 --- a/pkg/gce-pd-csi-driver/controller_test.go +++ b/pkg/gce-pd-csi-driver/controller_test.go @@ -18,6 +18,7 @@ import ( "testing" "golang.org/x/net/context" + compute "google.golang.org/api/compute/v1" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" @@ -27,13 +28,13 @@ import ( ) const ( - project = "test-project" - zone = "test-zone" - node = "test-node" - driver = "test-driver" + project = "test-project" + zone = "test-zone" + node = "test-node" + driver = "test-driver" + testVolumeId = zone + "/" + "test-vol" ) -// Create Volume Tests func TestCreateVolumeArguments(t *testing.T) { // Define "normal" parameters stdVolCap := []*csi.VolumeCapability{ @@ -71,7 +72,7 @@ func TestCreateVolumeArguments(t *testing.T) { }, expVol: &csi.Volume{ CapacityBytes: utils.GbToBytes(20), - Id: zone + "/" + "test-vol", + Id: testVolumeId, Attributes: nil, }, }, @@ -94,7 +95,7 @@ func TestCreateVolumeArguments(t *testing.T) { }, expVol: &csi.Volume{ CapacityBytes: MinimumVolumeSizeInBytes, - Id: zone + "/" + "test-vol", + Id: testVolumeId, Attributes: nil, }, }, @@ -108,7 +109,6 @@ func TestCreateVolumeArguments(t *testing.T) { expErrCode: codes.InvalidArgument, }, { - // TODO(dyzz) maybe if no params, we should have defaults. name: "success no params", req: &csi.CreateVolumeRequest{ Name: "test-vol", @@ -117,7 +117,7 @@ func TestCreateVolumeArguments(t *testing.T) { }, expVol: &csi.Volume{ CapacityBytes: utils.GbToBytes(20), - Id: zone + "/" + "test-vol", + Id: testVolumeId, Attributes: nil, }, }, @@ -132,68 +132,10 @@ func TestCreateVolumeArguments(t *testing.T) { }, expVol: &csi.Volume{ CapacityBytes: utils.GbToBytes(20), - Id: zone + "/" + "test-vol", + Id: testVolumeId, Attributes: nil, }, }, - { - name: "success capacity range: full specified", - req: &csi.CreateVolumeRequest{ - Name: "test-vol", - CapacityRange: &csi.CapacityRange{ - RequiredBytes: utils.GbToBytes(20), - LimitBytes: utils.GbToBytes(50), - }, - VolumeCapabilities: stdVolCap, - Parameters: stdParams, - }, - expVol: &csi.Volume{ - CapacityBytes: utils.GbToBytes(20), - Id: zone + "/" + "test-vol", - Attributes: nil, - }, - }, - { - name: "fail capacity range: limit less than required", - req: &csi.CreateVolumeRequest{ - Name: "test-vol", - CapacityRange: &csi.CapacityRange{ - RequiredBytes: utils.GbToBytes(50), - LimitBytes: utils.GbToBytes(20), - }, - VolumeCapabilities: stdVolCap, - Parameters: stdParams, - }, - expErrCode: codes.InvalidArgument, - }, - { - name: "success capacity range: only limit specified", - req: &csi.CreateVolumeRequest{ - Name: "test-vol", - CapacityRange: &csi.CapacityRange{ - LimitBytes: utils.GbToBytes(50), - }, - VolumeCapabilities: stdVolCap, - Parameters: stdParams, - }, - expVol: &csi.Volume{ - CapacityBytes: MinimumVolumeSizeInBytes, - Id: zone + "/" + "test-vol", - Attributes: nil, - }, - }, - { - name: "fail capacity range: only limit specified too low", - req: &csi.CreateVolumeRequest{ - Name: "test-vol", - CapacityRange: &csi.CapacityRange{ - LimitBytes: utils.GbToBytes(2), - }, - VolumeCapabilities: stdVolCap, - Parameters: stdParams, - }, - expErrCode: codes.InvalidArgument, - }, } // Run test cases @@ -205,7 +147,7 @@ func TestCreateVolumeArguments(t *testing.T) { if err != nil { t.Fatalf("Failed to create fake cloud provider: %v", err) } - err = gceDriver.SetupGCEDriver(fakeCloudProvider, nil, driver, node) + err = gceDriver.SetupGCEDriver(fakeCloudProvider, nil, driver, node, "vendor-version") if err != nil { t.Fatalf("Failed to setup GCE Driver: %v", err) } @@ -254,20 +196,276 @@ func TestCreateVolumeArguments(t *testing.T) { } } -// Test volume already exists +func TestDeleteVolume(t *testing.T) { + testCases := []struct { + name string + req *csi.DeleteVolumeRequest + expErr bool + }{ + { + name: "valid", + req: &csi.DeleteVolumeRequest{ + VolumeId: testVolumeId, + }, + }, + { + name: "invalid id", + req: &csi.DeleteVolumeRequest{ + VolumeId: testVolumeId + "/foo", + }, + expErr: true, + }, + } + for _, tc := range testCases { + t.Logf("test case: %s", tc.name) + // Setup new driver each time so no interference + gceDriver := initGCEDriver(t) -// Test volume with op pending + _, err := gceDriver.cs.DeleteVolume(context.TODO(), tc.req) + if err == nil && tc.expErr { + t.Fatalf("Expected error but got none") + } + if err != nil && !tc.expErr { + t.Fatalf("Did not expect error but got: %v", err) + } -// Test DeleteVolume + if err != nil { + continue + } -// Test ControllerPublishVolume + } +} -// Test ControllerUnpublishVolume +func TestGetRequestCapacity(t *testing.T) { + testCases := []struct { + name string + capRange *csi.CapacityRange + expCap int64 + expErr bool + }{ + { + name: "nil cap range", + capRange: nil, + expCap: MinimumVolumeSizeInBytes, + }, + { + name: "success: required below min", + capRange: &csi.CapacityRange{ + RequiredBytes: MinimumVolumeSizeInBytes - 1, + }, + expCap: MinimumVolumeSizeInBytes, + }, + { + name: "success: required equals min", + capRange: &csi.CapacityRange{ + RequiredBytes: MinimumVolumeSizeInBytes, + }, + expCap: MinimumVolumeSizeInBytes, + }, + { + name: "success: required above min", + capRange: &csi.CapacityRange{ + RequiredBytes: MinimumVolumeSizeInBytes + 1, + }, + expCap: MinimumVolumeSizeInBytes + 1, + }, + { + name: "fail: limit below min", + capRange: &csi.CapacityRange{ + LimitBytes: MinimumVolumeSizeInBytes - 1, + }, + expErr: true, + }, + { + name: "success: limit equal min", + capRange: &csi.CapacityRange{ + LimitBytes: MinimumVolumeSizeInBytes, + }, + expCap: MinimumVolumeSizeInBytes, + }, + { + name: "success: limit above min", + capRange: &csi.CapacityRange{ + LimitBytes: MinimumVolumeSizeInBytes + 1, + }, + expCap: MinimumVolumeSizeInBytes, + }, + { + name: "success: fully specified both above min", + capRange: &csi.CapacityRange{ + RequiredBytes: utils.GbToBytes(20), + LimitBytes: utils.GbToBytes(50), + }, + expCap: utils.GbToBytes(20), + }, + { + name: "success: fully specified required below min", + capRange: &csi.CapacityRange{ + RequiredBytes: MinimumVolumeSizeInBytes - 1, + LimitBytes: utils.GbToBytes(50), + }, + expCap: MinimumVolumeSizeInBytes, + }, + { + name: "success: fully specified both below min", + capRange: &csi.CapacityRange{ + RequiredBytes: MinimumVolumeSizeInBytes - 2, + LimitBytes: MinimumVolumeSizeInBytes - 1, + }, + expErr: true, + }, + { + name: "fail: limit less than required", + capRange: &csi.CapacityRange{ + RequiredBytes: utils.GbToBytes(50), + LimitBytes: utils.GbToBytes(20), + }, + expErr: true, + }, + { + name: "fail: limit less than required below min", + capRange: &csi.CapacityRange{ + RequiredBytes: MinimumVolumeSizeInBytes - 2, + LimitBytes: MinimumVolumeSizeInBytes - 3, + }, + expErr: true, + }, + } + for _, tc := range testCases { + t.Logf("test case: %s", tc.name) -// Test ValidateVolumeCapabilities + gotCap, err := getRequestCapacity(tc.capRange) + if err == nil && tc.expErr { + t.Fatalf("Expected error but got none") + } + if err != nil && !tc.expErr { + t.Fatalf("Did not expect error but got: %v", err) + } + + if err != nil { + continue + } -// Test ListVolumes + if gotCap != tc.expCap { + t.Fatalf("Got capacity: %v, expected: %v", gotCap, tc.expCap) + } + } +} -// Test GetCapacity +func TestDiskIsAttached(t *testing.T) { + testCases := []struct { + name string + disk *compute.Disk + instance *compute.Instance + expAttached bool + }{ + { + name: "normal-attached", + disk: &compute.Disk{ + Name: "test-disk", + }, + instance: &compute.Instance{ + Disks: []*compute.AttachedDisk{ + { + DeviceName: "test-disk", + }, + }, + }, + expAttached: true, + }, + { + name: "normal-not-attached", + disk: &compute.Disk{ + Name: "test-disk", + }, + instance: &compute.Instance{ + Disks: []*compute.AttachedDisk{ + { + DeviceName: "not-the-test-disk", + }, + }, + }, + expAttached: false, + }, + } + for _, tc := range testCases { + t.Logf("test case: %s", tc.name) + if attached := diskIsAttached(tc.disk, tc.instance); attached != tc.expAttached { + t.Errorf("Expected disk attached to be %v, but got %v", tc.expAttached, attached) + } + } +} -// Test ControllerGetCapabilities +func TestDiskIsAttachedAndCompatible(t *testing.T) { + testCases := []struct { + name string + disk *compute.Disk + instance *compute.Instance + mode string + expAttached bool + expErr bool + }{ + { + name: "normal-attached", + disk: &compute.Disk{ + Name: "test-disk", + }, + instance: &compute.Instance{ + Disks: []*compute.AttachedDisk{ + { + DeviceName: "test-disk", + Mode: "test-mode", + }, + }, + }, + mode: "test-mode", + expAttached: true, + }, + { + name: "normal-not-attached", + disk: &compute.Disk{ + Name: "test-disk", + }, + instance: &compute.Instance{ + Disks: []*compute.AttachedDisk{ + { + DeviceName: "not-the-test-disk", + Mode: "test-mode", + }, + }, + }, + mode: "test-mode", + expAttached: false, + }, + { + name: "incompatible mode", + disk: &compute.Disk{ + Name: "test-disk", + }, + instance: &compute.Instance{ + Disks: []*compute.AttachedDisk{ + { + DeviceName: "test-disk", + Mode: "test-mode", + }, + }, + }, + mode: "random-mode", + expAttached: true, + expErr: true, + }, + } + for _, tc := range testCases { + t.Logf("test case: %s", tc.name) + attached, err := diskIsAttachedAndCompatible(tc.disk, tc.instance, nil, tc.mode) + if err != nil && !tc.expErr { + t.Errorf("Did not expect error but got: %v", err) + } + if err == nil && tc.expErr { + t.Errorf("Expected error but got none") + } + if attached != tc.expAttached { + t.Errorf("Expected disk attached to be %v, but got %v", tc.expAttached, attached) + } + } +} diff --git a/pkg/gce-pd-csi-driver/gce-pd-driver_test.go b/pkg/gce-pd-csi-driver/gce-pd-driver_test.go index dd66aeb40..e0dbb6fae 100644 --- a/pkg/gce-pd-csi-driver/gce-pd-driver_test.go +++ b/pkg/gce-pd-csi-driver/gce-pd-driver_test.go @@ -13,3 +13,23 @@ limitations under the License. */ package gceGCEDriver + +import ( + "testing" + + gce "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/gce-cloud-provider" +) + +func initGCEDriver(t *testing.T) *GCEDriver { + vendorVersion := "test-vendor" + gceDriver := GetGCEDriver() + fakeCloudProvider, err := gce.FakeCreateCloudProvider(project, zone) + if err != nil { + t.Fatalf("Failed to create fake cloud provider: %v", err) + } + err = gceDriver.SetupGCEDriver(fakeCloudProvider, nil, driver, node, vendorVersion) + if err != nil { + t.Fatalf("Failed to setup GCE Driver: %v", err) + } + return gceDriver +} diff --git a/pkg/gce-pd-csi-driver/identity_test.go b/pkg/gce-pd-csi-driver/identity_test.go index 96b04c48c..7e600e5a9 100644 --- a/pkg/gce-pd-csi-driver/identity_test.go +++ b/pkg/gce-pd-csi-driver/identity_test.go @@ -22,8 +22,9 @@ import ( ) func TestGetPluginInfo(t *testing.T) { + vendorVersion := "test-vendor" gceDriver := GetGCEDriver() - err := gceDriver.SetupGCEDriver(nil, nil, driver, node) + err := gceDriver.SetupGCEDriver(nil, nil, driver, node, vendorVersion) if err != nil { t.Fatalf("Failed to setup GCE Driver: %v", err) } @@ -45,7 +46,7 @@ func TestGetPluginInfo(t *testing.T) { func TestGetPluginCapabilities(t *testing.T) { gceDriver := GetGCEDriver() - err := gceDriver.SetupGCEDriver(nil, nil, driver, node) + err := gceDriver.SetupGCEDriver(nil, nil, driver, node, "test-vendor") if err != nil { t.Fatalf("Failed to setup GCE Driver: %v", err) } @@ -66,7 +67,7 @@ func TestGetPluginCapabilities(t *testing.T) { func TestProbe(t *testing.T) { gceDriver := GetGCEDriver() - err := gceDriver.SetupGCEDriver(nil, nil, driver, node) + err := gceDriver.SetupGCEDriver(nil, nil, driver, node, "test-vendor") if err != nil { t.Fatalf("Failed to setup GCE Driver: %v", err) } diff --git a/test/e2e/gce_pd_e2e_test.go b/test/e2e/gce_pd_e2e_test.go index bc0df8b6a..0e9727657 100644 --- a/test/e2e/gce_pd_e2e_test.go +++ b/test/e2e/gce_pd_e2e_test.go @@ -205,6 +205,10 @@ var _ = Describe("GCE PD CSI Driver", func() { Expect(err).To(BeNil(), "NodeUnpublishVolume failed with error") }) + + // Test volume already exists + + // Test volume with op pending }) func Logf(format string, args ...interface{}) {