From bdbb42d6eac35218115cf1a7c26d13edf41b9882 Mon Sep 17 00:00:00 2001 From: srikiz Date: Fri, 24 Jun 2022 21:45:57 +0530 Subject: [PATCH 1/5] Set storage size to minimum 1Gi if volume size specified is less than the minimum --- driver/controller.go | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/driver/controller.go b/driver/controller.go index f003d1a93..52ea6d731 100644 --- a/driver/controller.go +++ b/driver/controller.go @@ -93,7 +93,7 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest) return nil, status.Error(codes.InvalidArgument, fmt.Sprintf("volume capabilities cannot be satisified: %s", strings.Join(violations, "; "))) } - size, err := extractStorage(req.CapacityRange) + size, err := d.extractStorage(req.CapacityRange) if err != nil { return nil, status.Errorf(codes.OutOfRange, "invalid capacity range: %v", err) } @@ -873,7 +873,7 @@ func (d *Driver) ControllerExpandVolume(ctx context.Context, req *csi.Controller return nil, status.Errorf(codes.Internal, "ControllerExpandVolume could not retrieve existing volume: %v", err) } - resizeBytes, err := extractStorage(req.GetCapacityRange()) + resizeBytes, err := d.extractStorage(req.GetCapacityRange()) if err != nil { return nil, status.Errorf(codes.OutOfRange, "ControllerExpandVolume invalid capacity range: %v", err) } @@ -936,7 +936,7 @@ func (d *Driver) ControllerGetVolume(ctx context.Context, req *csi.ControllerGet // range. If the capacity range is not satisfied it returns the default volume // size. If the capacity range is below or above supported sizes, it returns an // error. -func extractStorage(capRange *csi.CapacityRange) (int64, error) { +func (d *Driver) extractStorage(capRange *csi.CapacityRange) (int64, error) { if capRange == nil { return defaultVolumeSizeInBytes, nil } @@ -955,7 +955,11 @@ func extractStorage(capRange *csi.CapacityRange) (int64, error) { } if requiredSet && !limitSet && requiredBytes < minimumVolumeSizeInBytes { - return 0, fmt.Errorf("required (%v) can not be less than minimum supported volume size (%v)", formatBytes(requiredBytes), formatBytes(minimumVolumeSizeInBytes)) + d.log.WithFields(logrus.Fields{ + "requiredBytes": formatBytes(requiredBytes), + "minimumVolumeSizeInBytes": formatBytes(minimumVolumeSizeInBytes), + }).Warn("requiredBytes is less than minimum supported volume size, setting requiredBytes default to minimumVolumeSizeBytes") + return minimumVolumeSizeInBytes, nil } if limitSet && limitBytes < minimumVolumeSizeInBytes { From 075fa325dfdd713d87e509ffa388eff35fff12f7 Mon Sep 17 00:00:00 2001 From: srikiz Date: Thu, 30 Jun 2022 20:00:09 +0530 Subject: [PATCH 2/5] Incorporate review comments --- driver/controller.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/driver/controller.go b/driver/controller.go index 52ea6d731..35997eada 100644 --- a/driver/controller.go +++ b/driver/controller.go @@ -934,8 +934,8 @@ func (d *Driver) ControllerGetVolume(ctx context.Context, req *csi.ControllerGet // extractStorage extracts the storage size in bytes from the given capacity // range. If the capacity range is not satisfied it returns the default volume -// size. If the capacity range is below or above supported sizes, it returns an -// error. +// size. If the capacity range is above supported sizes, it returns an +// error. If the capacity range is below supported size, it returns the minimum supported size func (d *Driver) extractStorage(capRange *csi.CapacityRange) (int64, error) { if capRange == nil { return defaultVolumeSizeInBytes, nil @@ -956,9 +956,9 @@ func (d *Driver) extractStorage(capRange *csi.CapacityRange) (int64, error) { if requiredSet && !limitSet && requiredBytes < minimumVolumeSizeInBytes { d.log.WithFields(logrus.Fields{ - "requiredBytes": formatBytes(requiredBytes), - "minimumVolumeSizeInBytes": formatBytes(minimumVolumeSizeInBytes), - }).Warn("requiredBytes is less than minimum supported volume size, setting requiredBytes default to minimumVolumeSizeBytes") + "required_bytes": formatBytes(requiredBytes), + "minimum_volume_size": formatBytes(minimumVolumeSizeInBytes), + }).Warn("requiredBytes is less than minimum volume size, setting requiredBytes default to minimumVolumeSizeBytes") return minimumVolumeSizeInBytes, nil } From 35029546a08caab49a0efe71b204bf0a57d47e84 Mon Sep 17 00:00:00 2001 From: srikiz Date: Mon, 4 Jul 2022 13:53:36 +0530 Subject: [PATCH 3/5] Fix unit test --- driver/controller_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/driver/controller_test.go b/driver/controller_test.go index 1e2e3b768..2881537a5 100644 --- a/driver/controller_test.go +++ b/driver/controller_test.go @@ -206,8 +206,8 @@ func TestControllerExpandVolume(t *testing.T) { RequiredBytes: 0.5 * giB, }, }, - resp: nil, - err: status.Error(codes.OutOfRange, "ControllerExpandVolume invalid capacity range: required (512Mi) can not be less than minimum supported volume size (1Gi)"), + resp: &csi.ControllerExpandVolumeResponse{CapacityBytes: minimumVolumeSizeInBytes, NodeExpansionRequired: true}, + err: nil, }, { name: "volume for corresponding volume id does not exist", From ea7c3630c44875d843b173fe9880c2666ee01a1b Mon Sep 17 00:00:00 2001 From: srikiz Date: Mon, 4 Jul 2022 15:59:30 +0530 Subject: [PATCH 4/5] More fixes to unit tests --- driver/controller_test.go | 39 ++++++++++++++++++++++++--------------- 1 file changed, 24 insertions(+), 15 deletions(-) diff --git a/driver/controller_test.go b/driver/controller_test.go index 2881537a5..dc1e79644 100644 --- a/driver/controller_test.go +++ b/driver/controller_test.go @@ -181,14 +181,20 @@ func (*fakeTagsDriver) UntagResources(context.Context, string, *godo.UntagResour } func TestControllerExpandVolume(t *testing.T) { + default_volume := &godo.Volume{ + ID: "volume-id", + SizeGigaBytes: (defaultVolumeSizeInBytes / giB), + } tcs := []struct { - name string - req *csi.ControllerExpandVolumeRequest - resp *csi.ControllerExpandVolumeResponse - err error + name string + req *csi.ControllerExpandVolumeRequest + resp *csi.ControllerExpandVolumeResponse + err error + godo_volume *godo.Volume }{ { - name: "request exceeds maximum supported size", + name: "request exceeds maximum supported size", + godo_volume: default_volume, req: &csi.ControllerExpandVolumeRequest{ VolumeId: "volume-id", CapacityRange: &csi.CapacityRange{ @@ -200,6 +206,10 @@ func TestControllerExpandVolume(t *testing.T) { }, { name: "requested size less than minimum supported size", + godo_volume: &godo.Volume{ + ID: "volume-id", + SizeGigaBytes: (minimumVolumeSizeInBytes / giB), + }, req: &csi.ControllerExpandVolumeRequest{ VolumeId: "volume-id", CapacityRange: &csi.CapacityRange{ @@ -210,7 +220,8 @@ func TestControllerExpandVolume(t *testing.T) { err: nil, }, { - name: "volume for corresponding volume id does not exist", + name: "volume for corresponding volume id does not exist", + godo_volume: default_volume, req: &csi.ControllerExpandVolumeRequest{ VolumeId: "non-existent-id", CapacityRange: &csi.CapacityRange{ @@ -221,7 +232,8 @@ func TestControllerExpandVolume(t *testing.T) { err: status.Error(codes.Internal, "ControllerExpandVolume could not retrieve existing volume: volume not found"), }, { - name: "new volume size is less than old volume size", + name: "new volume size is less than old volume size", + godo_volume: default_volume, req: &csi.ControllerExpandVolumeRequest{ VolumeId: "volume-id", CapacityRange: &csi.CapacityRange{ @@ -232,7 +244,8 @@ func TestControllerExpandVolume(t *testing.T) { err: nil, }, { - name: "new volume size is equal to old volume size", + name: "new volume size is equal to old volume size", + godo_volume: default_volume, req: &csi.ControllerExpandVolumeRequest{ VolumeId: "volume-id", CapacityRange: &csi.CapacityRange{ @@ -245,20 +258,16 @@ func TestControllerExpandVolume(t *testing.T) { } for _, tc := range tcs { t.Run(tc.name, func(t *testing.T) { - volume := &godo.Volume{ - ID: "volume-id", - SizeGigaBytes: (defaultVolumeSizeInBytes / giB), - } driver := &Driver{ region: "foo", storage: &fakeStorageDriver{ volumes: map[string]*godo.Volume{ - "volume-id": volume, + "volume-id": tc.godo_volume, }, }, storageActions: &fakeStorageActionsDriver{ volumes: map[string]*godo.Volume{ - "volume-id": volume, + "volume-id": tc.godo_volume, }, }, log: logrus.New().WithField("test_enabed", true), @@ -268,7 +277,7 @@ func TestControllerExpandVolume(t *testing.T) { assert.Equal(t, err, tc.err) } else { assert.Equal(t, tc.resp, resp) - assert.Equal(t, (volume.SizeGigaBytes * giB), resp.CapacityBytes) + assert.Equal(t, (tc.godo_volume.SizeGigaBytes * giB), resp.CapacityBytes) } }) From 98c47145c95ce02a059fff69ceacf9a4af81091e Mon Sep 17 00:00:00 2001 From: srikiz Date: Tue, 5 Jul 2022 14:21:42 +0530 Subject: [PATCH 5/5] incorporate review comments --- driver/controller_test.go | 39 +++++++++++++++++++-------------------- 1 file changed, 19 insertions(+), 20 deletions(-) diff --git a/driver/controller_test.go b/driver/controller_test.go index dc1e79644..b1b464df4 100644 --- a/driver/controller_test.go +++ b/driver/controller_test.go @@ -181,20 +181,19 @@ func (*fakeTagsDriver) UntagResources(context.Context, string, *godo.UntagResour } func TestControllerExpandVolume(t *testing.T) { - default_volume := &godo.Volume{ + defaultVolume := &godo.Volume{ ID: "volume-id", SizeGigaBytes: (defaultVolumeSizeInBytes / giB), } tcs := []struct { - name string - req *csi.ControllerExpandVolumeRequest - resp *csi.ControllerExpandVolumeResponse - err error - godo_volume *godo.Volume + name string + req *csi.ControllerExpandVolumeRequest + resp *csi.ControllerExpandVolumeResponse + err error + volume *godo.Volume }{ { - name: "request exceeds maximum supported size", - godo_volume: default_volume, + name: "request exceeds maximum supported size", req: &csi.ControllerExpandVolumeRequest{ VolumeId: "volume-id", CapacityRange: &csi.CapacityRange{ @@ -205,10 +204,10 @@ func TestControllerExpandVolume(t *testing.T) { err: status.Error(codes.OutOfRange, "ControllerExpandVolume invalid capacity range: required (20Ti) can not exceed maximum supported volume size (16Ti)"), }, { - name: "requested size less than minimum supported size", - godo_volume: &godo.Volume{ + name: "requested size less than minimum supported size returns the default minimum volume size", + volume: &godo.Volume{ ID: "volume-id", - SizeGigaBytes: (minimumVolumeSizeInBytes / giB), + SizeGigaBytes: 1, }, req: &csi.ControllerExpandVolumeRequest{ VolumeId: "volume-id", @@ -220,8 +219,7 @@ func TestControllerExpandVolume(t *testing.T) { err: nil, }, { - name: "volume for corresponding volume id does not exist", - godo_volume: default_volume, + name: "volume for corresponding volume id does not exist", req: &csi.ControllerExpandVolumeRequest{ VolumeId: "non-existent-id", CapacityRange: &csi.CapacityRange{ @@ -232,8 +230,7 @@ func TestControllerExpandVolume(t *testing.T) { err: status.Error(codes.Internal, "ControllerExpandVolume could not retrieve existing volume: volume not found"), }, { - name: "new volume size is less than old volume size", - godo_volume: default_volume, + name: "new volume size is less than old volume size", req: &csi.ControllerExpandVolumeRequest{ VolumeId: "volume-id", CapacityRange: &csi.CapacityRange{ @@ -244,8 +241,7 @@ func TestControllerExpandVolume(t *testing.T) { err: nil, }, { - name: "new volume size is equal to old volume size", - godo_volume: default_volume, + name: "new volume size is equal to old volume size", req: &csi.ControllerExpandVolumeRequest{ VolumeId: "volume-id", CapacityRange: &csi.CapacityRange{ @@ -258,16 +254,19 @@ func TestControllerExpandVolume(t *testing.T) { } for _, tc := range tcs { t.Run(tc.name, func(t *testing.T) { + if tc.volume == nil { + tc.volume = defaultVolume + } driver := &Driver{ region: "foo", storage: &fakeStorageDriver{ volumes: map[string]*godo.Volume{ - "volume-id": tc.godo_volume, + "volume-id": tc.volume, }, }, storageActions: &fakeStorageActionsDriver{ volumes: map[string]*godo.Volume{ - "volume-id": tc.godo_volume, + "volume-id": tc.volume, }, }, log: logrus.New().WithField("test_enabed", true), @@ -277,7 +276,7 @@ func TestControllerExpandVolume(t *testing.T) { assert.Equal(t, err, tc.err) } else { assert.Equal(t, tc.resp, resp) - assert.Equal(t, (tc.godo_volume.SizeGigaBytes * giB), resp.CapacityBytes) + assert.Equal(t, (tc.volume.SizeGigaBytes * giB), resp.CapacityBytes) } })