Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

modify error message when request volume is in use with other node #698

Merged
merged 1 commit into from
Feb 6, 2021
Merged

modify error message when request volume is in use with other node #698

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 22 additions & 2 deletions pkg/cloud/cloud.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"errors"
"fmt"
"os"
"strings"
"time"

"github.com/aws/aws-sdk-go/aws"
Expand Down Expand Up @@ -115,6 +116,9 @@ var (
// ErrAlreadyExists is returned when a resource is already existent.
ErrAlreadyExists = errors.New("Resource already exists")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we not use this anymore?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I see the TODO below.


// ErrVolumeInUse is returned when a volume is already attached to an instance.
ErrVolumeInUse = errors.New("Request volume is already attached to an instance")

// ErrMultiSnapshots is returned when multiple snapshots are found
// with the same ID
ErrMultiSnapshots = errors.New("Multiple snapshots with the same name found")
Expand All @@ -133,6 +137,7 @@ type Disk struct {
AvailabilityZone string
SnapshotID string
OutpostArn string
Attachments []string
}

// DiskOptions represents parameters to create an EBS volume
Expand Down Expand Up @@ -378,7 +383,7 @@ func (c *cloud) AttachDisk(ctx context.Context, volumeID, nodeID string) (string
if err != nil {
if awsErr, ok := err.(awserr.Error); ok {
if awsErr.Code() == "VolumeInUse" {
return "", ErrAlreadyExists
return "", ErrVolumeInUse
}
}
return "", fmt.Errorf("could not attach volume %q to node %q: %v", volumeID, nodeID, err)
Expand All @@ -396,7 +401,9 @@ func (c *cloud) AttachDisk(ctx context.Context, volumeID, nodeID string) (string
// TODO: Double check the attachment to be 100% sure we attached the correct volume at the correct mountpoint
// It could happen otherwise that we see the volume attached from a previous/separate AttachVolume call,
// which could theoretically be against a different device (or even instance).

// TODO: Check volume capability matches for ALREADY_EXISTS
// This could happen when request volume already attached to request node,
// but is incompatible with the specified volume_capability or readonly flag
return device.Path, nil
}

Expand Down Expand Up @@ -521,6 +528,7 @@ func (c *cloud) GetDiskByID(ctx context.Context, volumeID string) (*Disk, error)
}

volume, err := c.getVolume(ctx, request)

if err != nil {
return nil, err
}
Expand All @@ -530,6 +538,7 @@ func (c *cloud) GetDiskByID(ctx context.Context, volumeID string) (*Disk, error)
CapacityGiB: aws.Int64Value(volume.Size),
AvailabilityZone: aws.StringValue(volume.AvailabilityZone),
OutpostArn: aws.StringValue(volume.OutpostArn),
Attachments: getVolumeAttachmentsList(volume),
}, nil
}

Expand Down Expand Up @@ -1044,3 +1053,14 @@ func volumeModificationDone(state string) bool {
}
return false
}

func getVolumeAttachmentsList(volume *ec2.Volume) []string {
var volumeAttachmentList []string
for _, attachment := range volume.Attachments {
if attachment.State != nil && strings.ToLower(aws.StringValue(attachment.State)) == "attached" {
volumeAttachmentList = append(volumeAttachmentList, aws.StringValue(attachment.InstanceId))
}
}

return volumeAttachmentList
}
17 changes: 17 additions & 0 deletions pkg/cloud/cloud_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -525,21 +525,34 @@ func TestGetDiskByID(t *testing.T) {
volumeID string
availabilityZone string
outpostArn string
attachments *ec2.VolumeAttachment
expErr error
}{
{
name: "success: normal",
volumeID: "vol-test-1234",
availabilityZone: expZone,
attachments: &ec2.VolumeAttachment{},
expErr: nil,
},
{
name: "success: outpost volume",
volumeID: "vol-test-1234",
availabilityZone: expZone,
outpostArn: "arn:aws:outposts:us-west-2:111111111111:outpost/op-0aaa000a0aaaa00a0",
attachments: &ec2.VolumeAttachment{},
expErr: nil,
},
{
name: "success: attached instance list",
volumeID: "vol-test-1234",
availabilityZone: expZone,
outpostArn: "arn:aws:outposts:us-west-2:111111111111:outpost/op-0aaa000a0aaaa00a0",
attachments: &ec2.VolumeAttachment{
InstanceId: aws.String("test-instance"),
State: aws.String("attached")},
expErr: nil,
},
{
name: "fail: DescribeVolumes returned generic error",
volumeID: "vol-test-1234",
Expand All @@ -561,6 +574,7 @@ func TestGetDiskByID(t *testing.T) {
VolumeId: aws.String(tc.volumeID),
AvailabilityZone: aws.String(tc.availabilityZone),
OutpostArn: aws.String(tc.outpostArn),
Attachments: []*ec2.VolumeAttachment{tc.attachments},
},
},
},
Expand All @@ -585,6 +599,9 @@ func TestGetDiskByID(t *testing.T) {
if disk.OutpostArn != tc.outpostArn {
t.Fatalf("GetDisk() failed: expected outpostArn %q, got %q", tc.outpostArn, disk.OutpostArn)
}
if len(disk.Attachments) > 0 && disk.Attachments[0] != aws.StringValue(tc.attachments.InstanceId) {
t.Fatalf("GetDisk() failed: expected attachment instance %q, got %q", aws.StringValue(tc.attachments.InstanceId), disk.Attachments[0])
}
}

mockCtrl.Finish()
Expand Down
10 changes: 6 additions & 4 deletions pkg/driver/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,19 +286,21 @@ func (d *controllerService) ControllerPublishVolume(ctx context.Context, req *cs
if !d.cloud.IsExistInstance(ctx, nodeID) {
return nil, status.Errorf(codes.NotFound, "Instance %q not found", nodeID)
}

if _, err := d.cloud.GetDiskByID(ctx, volumeID); err != nil {
disk, err := d.cloud.GetDiskByID(ctx, volumeID)
if err != nil {
if err == cloud.ErrNotFound {
return nil, status.Error(codes.NotFound, "Volume not found")
}
return nil, status.Errorf(codes.Internal, "Could not get volume with ID %q: %v", volumeID, err)
}

// If given volumeId already assigned to given node, will directly return current device path
devicePath, err := d.cloud.AttachDisk(ctx, volumeID, nodeID)
if err != nil {
if err == cloud.ErrAlreadyExists {
return nil, status.Error(codes.AlreadyExists, err.Error())
if err == cloud.ErrVolumeInUse {
return nil, status.Error(codes.FailedPrecondition, strings.Join(disk.Attachments, ","))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is what we want. Looking at the API documentation[1][2], this list will have information about previous attachments as well, but the status will be "detached". So I think what we want to do is filter for "attached".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is a good point, will update

}
// TODO: Check volume capability matches for ALREADY_EXISTS
return nil, status.Errorf(codes.Internal, "Could not attach volume %q to node %q: %v", volumeID, nodeID, err)
}
klog.V(5).Infof("ControllerPublishVolume: volume %s attached to node %s through device %s", volumeID, nodeID, devicePath)
Expand Down
17 changes: 12 additions & 5 deletions pkg/driver/controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2669,8 +2669,12 @@ func TestControllerPublishVolume(t *testing.T) {
},
},
{
name: "fail attach disk with already exists error",
name: "fail attach disk with volume already in use error",
testFunc: func(t *testing.T) {
attachedInstancId := "test-instance-id-attached"
disk := &cloud.Disk{
Attachments: []string{attachedInstancId},
}
req := &csi.ControllerPublishVolumeRequest{
VolumeId: "does-not-exist",
NodeId: expInstanceID,
Expand All @@ -2684,8 +2688,8 @@ func TestControllerPublishVolume(t *testing.T) {

mockCloud := mocks.NewMockCloud(mockCtl)
mockCloud.EXPECT().IsExistInstance(gomock.Eq(ctx), gomock.Eq(req.NodeId)).Return(true)
mockCloud.EXPECT().GetDiskByID(gomock.Eq(ctx), gomock.Any()).Return(&cloud.Disk{}, nil)
mockCloud.EXPECT().AttachDisk(gomock.Eq(ctx), gomock.Any(), gomock.Eq(req.NodeId)).Return("", cloud.ErrAlreadyExists)
mockCloud.EXPECT().GetDiskByID(gomock.Eq(ctx), gomock.Any()).Return(disk, nil)
mockCloud.EXPECT().AttachDisk(gomock.Eq(ctx), gomock.Any(), gomock.Eq(req.NodeId)).Return("", cloud.ErrVolumeInUse)

awsDriver := controllerService{
cloud: mockCloud,
Expand All @@ -2697,8 +2701,11 @@ func TestControllerPublishVolume(t *testing.T) {
if !ok {
t.Fatalf("Could not get error status code from error: %v", srvErr)
}
if srvErr.Code() != codes.AlreadyExists {
t.Fatalf("Expected error code %d, got %d message %s", codes.AlreadyExists, srvErr.Code(), srvErr.Message())
if srvErr.Code() != codes.FailedPrecondition {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we also check if we're returning the correct instance id in the error message? It's a requirement from the spec.

t.Fatalf("Expected error code %d, got %d message %s", codes.FailedPrecondition, srvErr.Code(), srvErr.Message())
}
if srvErr.Message() != attachedInstancId {
t.Fatalf("Expected error message to contain previous attached instanceId %s, but get error message %s", attachedInstancId, srvErr.Message())
}
} else {
t.Fatalf("Expected error %v, got no error", codes.AlreadyExists)
Expand Down