driver: make sure to handle concurrent calls to the droplet #61
Conversation
The DigitalOcean API doesn't process actions for a given resource in async manner. Calls needs to be done sequentially. This means, if there is an ongoing process, say an "attach" process, another "attach" or "detach" will fail. We return a `ABORTED` error, which makes sure the csi-attacher to retry again in the next backoff tick (see: https://github.com/kubernetes-csi/external-attacher/blob/5a4cc3b1af15966cdb9b8ca1af6ab3adade877b0/pkg/controller/csi_handler.go#L307 and https://github.com/kubernetes-csi/external-attacher/blob/5a4cc3b1af15966cdb9b8ca1af6ab3adade877b0/pkg/connection/connection.go#L223-L227 )
fatih
force-pushed
the
multiple-volumes
branch
from
4ef90b9 to
4438af4
Compare
|
lgtm |
| "resp": resp, | ||
| }).Warn("assuming volume is attached already") | ||
| return &csi.ControllerPublishVolumeResponse{}, nil | ||
| if resp != nil && resp.StatusCode == http.StatusUnprocessableEntity { |
There was a problem hiding this comment.
Looks like there's some code duplication in regards to the checks here ?
There was a problem hiding this comment.
Peter how exactly? I don't see any duplication. Can you give a more concrete example?
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
Oh I see, a little copy is ok I guess :) I'll refactor if needed though.
| return &csi.ControllerPublishVolumeResponse{}, nil | ||
| } | ||
|
|
||
| if strings.Contains(err.Error(), "Droplet already has a pending event") { |
There was a problem hiding this comment.
Doesn't digitalocean return a specific error code for this? What if the string itself changes
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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 :)
|
I'll merge this as I want to continue on debugging and also release a new version so we can make sure people are up to date. Follow up fixes will be opened. |
The DigitalOcean API doesn't process actions for a given resource in
async manner. Calls needs to be done sequentially. This means, if there
is an ongoing process, say an "attach" process, another "attach" or
"detach" will fail.
We return a
ABORTEDerror, which makes sure the csi-attacher to retryagain in the next backoff tick (see:
https://github.com/kubernetes-csi/external-attacher/blob/5a4cc3b1af15966cdb9b8ca1af6ab3adade877b0/pkg/controller/csi_handler.go#L307
and
https://github.com/kubernetes-csi/external-attacher/blob/5a4cc3b1af15966cdb9b8ca1af6ab3adade877b0/pkg/connection/connection.go#L223-L227
)
fixes #32