Skip to content

Commit

Permalink
Return ErrInvalidArgument in cloud upon EC2 ModifyVolume
Browse files Browse the repository at this point in the history
  • Loading branch information
AndrewSirenko committed Mar 8, 2024
1 parent f335f99 commit cfef212
Show file tree
Hide file tree
Showing 3 changed files with 80 additions and 3 deletions.
25 changes: 24 additions & 1 deletion pkg/cloud/cloud.go
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,9 @@ var (

// VolumeNotBeingModified is returned if volume being described is not being modified
VolumeNotBeingModified = fmt.Errorf("volume is not being modified")

// ErrInvalidArgument is returned if parameters were rejected by cloud provider
ErrInvalidArgument = errors.New("invalid argument")
)

// Set during build time via -ldflags
Expand Down Expand Up @@ -653,7 +656,11 @@ func (c *cloud) ResizeOrModifyDisk(ctx context.Context, volumeID string, newSize

response, err := c.ec2.ModifyVolumeWithContext(ctx, req)
if err != nil {
return 0, fmt.Errorf("unable to modify AWS volume %q: %w", volumeID, err)
if isAWSErrorModifyVolumeInvalidParameter(err) {
// Wrap the many possible EC2 errors with cloud error, but pass on actual EC2 error to operator
return 0, fmt.Errorf("%w: %w", ErrInvalidArgument, err)
}
return 0, err
}

// If the volume modification isn't immediately completed, wait for it to finish
Expand Down Expand Up @@ -1407,6 +1414,22 @@ func isAWSErrorBlockDeviceInUse(err error) bool {
return false
}

// isAWSErrorModifyVolumeInvalidParameter returns a boolean indicating whether the
// given error is caused by invalid parameters in a ModifyVolume request
func isAWSErrorModifyVolumeInvalidParameter(err error) bool {
if isAWSError(err, "InvalidParameterValue") {
return true
}
if isAWSError(err, "InvalidParameterCombination") {
return true
}
if isAWSError(err, "UnknownVolumeType") {
return true
}

return false
}

// Checks for desired size on volume by also verifying volume size by describing volume.
// This is to get around potential eventual consistency problems with describing volume modifications
// objects and ensuring that we read two different objects to verify volume state.
Expand Down
52 changes: 51 additions & 1 deletion pkg/cloud/cloud_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1926,8 +1926,52 @@ func TestResizeOrModifyDisk(t *testing.T) {
AvailabilityZone: aws.String(defaultZone),
VolumeType: aws.String("gp2"),
},
modifiedVolumeError: awserr.New("GenericErr", "Generic error that does not involve invalid parameters", nil),
expErr: awserr.New("GenericErr", "Generic error that does not involve invalid parameters", nil),
},
{
name: "failure: returned ErrInvalidArgument when ModifyVolume returned InvalidParameterCombination",
volumeID: "vol-test",
modifyDiskOptions: &ModifyDiskOptions{
VolumeType: "GP2",
IOPS: 3000,
},
existingVolume: &ec2.Volume{
VolumeId: aws.String("vol-test"),
AvailabilityZone: aws.String(defaultZone),
VolumeType: aws.String("gp2"),
},
modifiedVolumeError: awserr.New("InvalidParameterCombination", "The parameter iops is not supported for gp2 volumes", nil),
expErr: awserr.New("InvalidParameterCombination", "The parameter iops is not supported for gp2 volumes", nil),
expErr: ErrInvalidArgument,
},
{
name: "failure: returned returned ErrInvalidArgument when ModifyVolume returned UnknownVolumeType",
volumeID: "vol-test",
modifyDiskOptions: &ModifyDiskOptions{
VolumeType: "GPFake",
},
existingVolume: &ec2.Volume{
VolumeId: aws.String("vol-test"),
AvailabilityZone: aws.String(defaultZone),
VolumeType: aws.String("gp2"),
},
modifiedVolumeError: awserr.New("UnknownVolumeType", "Unsupported volume type 'GPFake' for volume creation.", nil),
expErr: ErrInvalidArgument,
},
{
name: "failure: returned ErrInvalidArgument when ModifyVolume returned InvalidParameterValue",
volumeID: "vol-test",
modifyDiskOptions: &ModifyDiskOptions{
VolumeType: "GP3",
IOPS: 9999999,
},
existingVolume: &ec2.Volume{
VolumeId: aws.String("vol-test"),
AvailabilityZone: aws.String(defaultZone),
VolumeType: aws.String("gp2"),
},
modifiedVolumeError: awserr.New("InvalidParameterValue", "Volume iops of 9999999 is too high; maximum is x", nil),
expErr: ErrInvalidArgument,
},
{
name: "success: does not call ModifyVolume when no modification required",
Expand Down Expand Up @@ -2016,6 +2060,12 @@ func TestResizeOrModifyDisk(t *testing.T) {
if err != nil {
if tc.expErr == nil {
t.Fatalf("ResizeOrModifyDisk() failed: expected no error, got: %v", err)
} else {
if tc.expErr == ErrInvalidArgument {
if !errors.Is(err, ErrInvalidArgument) {
t.Fatalf("ResizeOrModifyDisk() failed: expected ErrInvalidArgument, got: %v", err)
}
}
}
} else {
if tc.expErr != nil {
Expand Down
6 changes: 5 additions & 1 deletion pkg/driver/controller_modify_volume.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package driver

import (
"context"
"errors"
"fmt"
"strconv"
"sync"
Expand Down Expand Up @@ -183,7 +184,7 @@ func (d *controllerService) executeModifyVolumeRequest(volumeID string, req *mod
defer cancel()
actualSizeGiB, err := d.cloud.ResizeOrModifyDisk(ctx, volumeID, req.newSize, &req.modifyDiskOptions)
if err != nil {
return 0, status.Errorf(codes.Internal, "Could not modify volume %q: %v", volumeID, err)
return 0, err
} else {
return actualSizeGiB, nil
}
Expand Down Expand Up @@ -272,6 +273,9 @@ func (d *controllerService) modifyVolumeWithCoalescing(ctx context.Context, volu
select {
case response := <-responseChan:
if response.err != nil {
if errors.Is(response.err, cloud.ErrInvalidArgument) {
return status.Errorf(codes.InvalidArgument, "Could not modify volume %q: %v", volume, response.err)
}
return status.Errorf(codes.Internal, "Could not modify volume %q: %v", volume, response.err)
}
case <-ctx.Done():
Expand Down

0 comments on commit cfef212

Please sign in to comment.