From 5cfe20fc08e5f890e826c955acec33fad41b126a Mon Sep 17 00:00:00 2001 From: elijah quinones Date: Mon, 5 Aug 2024 18:17:43 +0000 Subject: [PATCH] add the remaining unit tests and comments --- pkg/cloud/cloud_test.go | 154 ++++++++++++++++++++++++++++++++++ pkg/driver/controller.go | 4 + pkg/driver/controller_test.go | 14 ++++ pkg/driver/driver_test.go | 6 +- 4 files changed, 175 insertions(+), 3 deletions(-) diff --git a/pkg/cloud/cloud_test.go b/pkg/cloud/cloud_test.go index 7b58c0f759..a530dbf368 100644 --- a/pkg/cloud/cloud_test.go +++ b/pkg/cloud/cloud_test.go @@ -82,7 +82,51 @@ func extractVolumeIdentifiers(volumes []types.Volume) (volumeIDs []string, volum } return volumeIDs, volumeNames } +func TestNewCloud(t *testing.T) { + testCases := []struct { + name string + region string + awsSdkDebugLog bool + userAgentExtra string + batchingEnabled bool + }{ + { + name: "sucess with debug,userAgentExtra and batching", + region: "us-east-1", + awsSdkDebugLog: true, + userAgentExtra: "example_user_agent_extra", + batchingEnabled: true, + }, + { + name: "sucess with only debug and userAgenrExtra", + region: "us-east-1", + awsSdkDebugLog: true, + userAgentExtra: "example_user_agent_extra", + batchingEnabled: false, + }, + { + name: "sucess with only region", + region: "us-east-1", + awsSdkDebugLog: false, + userAgentExtra: "", + batchingEnabled: false, + }, + } + for _, tc := range testCases { + ec2Cloud, err := NewCloud(tc.region, tc.awsSdkDebugLog, tc.userAgentExtra, tc.batchingEnabled) + if err != nil { + t.Fatalf("error %v", err) + } + ec2CloudAscloud := ec2Cloud.(*cloud) + assert.Equal(t, ec2CloudAscloud.region, tc.region) + if tc.batchingEnabled { + assert.NotNil(t, ec2CloudAscloud.bm) + } else { + assert.Nil(t, ec2CloudAscloud.bm) + } + } +} func TestBatchDescribeVolumes(t *testing.T) { testCases := []struct { name string @@ -553,6 +597,101 @@ func executeDescribeSnapshotsTest(t *testing.T, c *cloud, snapshotIDs, snapshotN } } +func TestCheckDesiredState(t *testing.T) { + testCases := []struct { + name string + volumeId string + desiredSizeGiB int32 + options *ModifyDiskOptions + expErr error + }{ + { + name: "sucess: normal path", + volumeId: "vol-001", + desiredSizeGiB: 5, + options: &ModifyDiskOptions{ + VolumeType: VolumeTypeGP2, + IOPS: 3000, + Throughput: 1000, + }, + }, + { + name: "failure: volume is still being expanded", + volumeId: "vol-001", + desiredSizeGiB: 500, + options: &ModifyDiskOptions{ + VolumeType: VolumeTypeGP2, + IOPS: 3000, + Throughput: 1000, + }, + expErr: fmt.Errorf("volume \"vol-001\" is still being expanded to 500 size"), + }, + { + name: "failure: volume is still being modified to iops", + volumeId: "vol-001", + desiredSizeGiB: 50, + options: &ModifyDiskOptions{ + VolumeType: VolumeTypeGP2, + IOPS: 4000, + Throughput: 1000, + }, + expErr: fmt.Errorf("volume \"vol-001\" is still being modified to iops 4000"), + }, + { + name: "failure: volume is still being modifed to type", + volumeId: "vol-001", + desiredSizeGiB: 50, + options: &ModifyDiskOptions{ + VolumeType: VolumeTypeGP3, + IOPS: 3000, + Throughput: 1000, + }, + expErr: fmt.Errorf("volume \"vol-001\" is still being modified to type %q", VolumeTypeGP3), + }, + { + name: "failure: volume is still being modified to throughput", + volumeId: "vol-001", + desiredSizeGiB: 5, + options: &ModifyDiskOptions{ + VolumeType: VolumeTypeGP2, + IOPS: 3000, + Throughput: 2000, + }, + expErr: fmt.Errorf("volume \"vol-001\" is still being modified to throughput 2000"), + }, + } + for _, tc := range testCases { + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + mockEC2 := NewMockEC2API(mockCtrl) + c := newCloud(mockEC2) + cloudInstance := c.(*cloud) + mockEC2.EXPECT().DescribeVolumes(gomock.Any(), gomock.Any()).Return(&ec2.DescribeVolumesOutput{ + Volumes: []types.Volume{ + { + VolumeId: aws.String("vol-001"), + Size: aws.Int32(50), + VolumeType: types.VolumeTypeGp2, + Iops: aws.Int32(3000), + Throughput: aws.Int32(1000), + }, + }, + }, nil) + _, err := cloudInstance.checkDesiredState(context.Background(), tc.volumeId, tc.desiredSizeGiB, tc.options) + if err != nil { + if tc.expErr == nil { + t.Fatalf("Did not expect to get an error but got %q", err) + } else if tc.expErr.Error() != err.Error() { + t.Fatalf("checkDesiredState() failed: expected error %q, got: %q", tc.expErr, err) + } + } else { + if tc.expErr != nil { + t.Fatalf("checkDesiredState() failed: expected error got nothing") + } + } + } +} + func TestBatchDescribeVolumesModifications(t *testing.T) { testCases := []struct { name string @@ -1197,6 +1336,21 @@ func TestCreateDisk(t *testing.T) { }, expErr: fmt.Errorf("CreateDisk: multi-attach is only supported for io2 volumes"), }, + { + name: "failure: invalid VolumeType", + volumeName: "vol-test-name", + diskOptions: &DiskOptions{ + CapacityBytes: util.GiBToBytes(1), + Tags: map[string]string{VolumeNameTagKey: "vol-test", AwsEbsDriverTagKey: "true"}, + VolumeType: "invalidVolumeType", + }, + expDisk: &Disk{ + VolumeID: "vol-test", + CapacityGiB: 1, + AvailabilityZone: defaultZone, + }, + expErr: fmt.Errorf("invalid AWS VolumeType %q", "invalidVolumeType"), + }, } for _, tc := range testCases { tc := tc diff --git a/pkg/driver/controller.go b/pkg/driver/controller.go index 7b460fd6f2..de6bcffe8b 100644 --- a/pkg/driver/controller.go +++ b/pkg/driver/controller.go @@ -130,6 +130,10 @@ func (d *ControllerService) CreateVolume(ctx context.Context, req *csi.CreateVol for key, value := range req.GetParameters() { switch strings.ToLower(key) { + // we tell user here that they should use csi.storage.k8s.io/fstype instead but there is no switch cases to handle that + // Suggestion to add case FSTypeKey that will set the fstype and in the case "fstype" we print the warning and still set + // fstype for them + case "fstype": klog.InfoS("\"fstype\" is deprecated, please use \"csi.storage.k8s.io/fstype\" instead") case VolumeTypeKey: diff --git a/pkg/driver/controller_test.go b/pkg/driver/controller_test.go index 4a14d6bb7a..819c17d503 100644 --- a/pkg/driver/controller_test.go +++ b/pkg/driver/controller_test.go @@ -1745,6 +1745,13 @@ func TestCreateVolumeWithFormattingParameters(t *testing.T) { }, errExpected: false, }, + { + name: "failure with IOPSPerGBKey", + formattingOptionParameters: map[string]string{ + IopsPerGBKey: "wrong_value", + }, + errExpected: true, + }, { name: "failure with block size", formattingOptionParameters: map[string]string{ @@ -1789,6 +1796,13 @@ func TestCreateVolumeWithFormattingParameters(t *testing.T) { }, errExpected: true, }, + { + name: "failure with Block Express on io1 volume", + formattingOptionParameters: map[string]string{ + BlockExpressKey: "true", + }, + errExpected: true, + }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { diff --git a/pkg/driver/driver_test.go b/pkg/driver/driver_test.go index 37925a7797..642f70f737 100644 --- a/pkg/driver/driver_test.go +++ b/pkg/driver/driver_test.go @@ -79,11 +79,11 @@ func TestNewDriver(t *testing.T) { } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - result, err := NewDriver(mockCloud, tc.o, mockMounter, mockMetadataService, mockKubernetesClient) - if tc.hasNode && result.node == nil { + driver, err := NewDriver(mockCloud, tc.o, mockMounter, mockMetadataService, mockKubernetesClient) + if tc.hasNode && driver.node == nil { t.Fatalf("Expected driver to have node but driver does not have node") } - if tc.hasController && result.controller == nil { + if tc.hasController && driver.controller == nil { t.Fatalf("Expected driver to have controller but driver does not have controller") }