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

driver: make sure to handle concurrent calls to the droplet #61

Merged
merged 1 commit into from
Aug 23, 2018
+38 −12
Merged

driver: make sure to handle concurrent calls to the droplet #61

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
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
gracefully. We're not very strict anymore in cases we don't need to be, but
we're also returning a better error in cases we need to be.
[[GH-60]](https://github.com/digitalocean/csi-digitalocean/pull/60)
* Fix attaching multiple volumes to a single pod
[[GH-61]](https://github.com/digitalocean/csi-digitalocean/pull/61)

## v0.1.3 - August 3rd 2018

Expand Down
48 changes: 36 additions & 12 deletions driver/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,12 +202,24 @@ func (d *Driver) ControllerPublishVolume(ctx context.Context, req *csi.Controlle
action, resp, err := d.doClient.StorageActions.Attach(ctx, req.VolumeId, dropletID)
if err != nil {
// don't do anything if attached
if (resp != nil && resp.StatusCode == http.StatusUnprocessableEntity) || strings.Contains(err.Error(), "This volume is already attached") {
ll.WithFields(logrus.Fields{
"error": err,
"resp": resp,
}).Warn("assuming volume is attached already")
return &csi.ControllerPublishVolumeResponse{}, nil
if resp != nil && resp.StatusCode == http.StatusUnprocessableEntity {
Copy link

Choose a reason for hiding this comment

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

Looks like there's some code duplication in regards to the checks here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Peter how exactly? I don't see any duplication. Can you give a more concrete example?

Copy link

Choose a reason for hiding this comment

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

It's the way the check is bein done. The conditional checks on line 205 are ( except for the string being checked ) exactly the same as the ones on line 278. Not that that's an issue though :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I see, a little copy is ok I guess :) I'll refactor if needed though.

if strings.Contains(err.Error(), "This volume is already attached") {
ll.WithFields(logrus.Fields{
"error": err,
"resp": resp,
}).Warn("assuming volume is attached already")
return &csi.ControllerPublishVolumeResponse{}, nil
}

if strings.Contains(err.Error(), "Droplet already has a pending event") {
Copy link

Choose a reason for hiding this comment

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

Doesn't digitalocean return a specific error code for this? What if the string itself changes

Copy link

Choose a reason for hiding this comment

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

such as for example the rate limit return message has the following as body :

{
        id: "too_many_requests",
        message: "API Rate limit exceeded."
}

for which the id would be a prime use case here :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As for now, this is the only way we can deal with it. But we'll have some internal todo's to make this better :)

ll.WithFields(logrus.Fields{
"error": err,
"resp": resp,
}).Warn("droplet is not able to detach the volume")
// sending an abort makes sure the csi-attacher retries with the next backoff tick
return nil, status.Errorf(codes.Aborted, "volume %q couldn't be attached. droplet %d is in process of another action",
req.VolumeId, dropletID)
}
}
return nil, err
}
Expand Down Expand Up @@ -263,12 +275,24 @@ func (d *Driver) ControllerUnpublishVolume(ctx context.Context, req *csi.Control

action, resp, err := d.doClient.StorageActions.DetachByDropletID(ctx, req.VolumeId, dropletID)
if err != nil {
if (resp != nil && resp.StatusCode == http.StatusUnprocessableEntity) || strings.Contains(err.Error(), "Attachment not found") {
ll.WithFields(logrus.Fields{
"error": err,
"resp": resp,
}).Warn("assuming volume is detached already")
return &csi.ControllerUnpublishVolumeResponse{}, nil
if resp != nil && resp.StatusCode == http.StatusUnprocessableEntity {
if strings.Contains(err.Error(), "Attachment not found") {
ll.WithFields(logrus.Fields{
"error": err,
"resp": resp,
}).Warn("assuming volume is detached already")
return &csi.ControllerUnpublishVolumeResponse{}, nil
}

if strings.Contains(err.Error(), "Droplet already has a pending event") {
ll.WithFields(logrus.Fields{
"error": err,
"resp": resp,
}).Warn("droplet is not able to detach the volume")
// sending an abort makes sure the csi-attacher retries with the next backoff tick
return nil, status.Errorf(codes.Aborted, "volume %q couldn't be detached. droplet %d is in process of another action",
req.VolumeId, dropletID)
}
}
return nil, err
}
Expand Down