Skip to content

CON-9080 Remove checkLimit validation in favor of forbidden response when creating a volume #481

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

Merged
merged 8 commits into from
Mar 17, 2023
Merged

CON-9080 Remove checkLimit validation in favor of forbidden response when creating a volume #481

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
bb280e9
CON-9080 Removed the checklimit validation to favor the forbidden res…
llDrLove Mar 14, 2023
90f1972
CON-9080 handle limit if we have a ErrorResponse
llDrLove Mar 14, 2023
cb4f9ec
CON-9080 refactor conditions
llDrLove Mar 14, 2023
7612e01
CON-9080 refactor error handling
llDrLove Mar 14, 2023
0dbbc1e
CON-9080 add unit tests
llDrLove Mar 15, 2023
910a432
CON-9080 Add release note and use error.As instead of casting error type
llDrLove Mar 15, 2023
1aa54c8
CON-9080 fix unit tests with new limit if statement implementation
llDrLove Mar 15, 2023
68afc7f
CON-9080 fix typo error and don't check inner response struct for nil
llDrLove Mar 17, 2023
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
23 changes: 11 additions & 12 deletions driver/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package driver

import (
"context"
"encoding/json"
"errors"
"fmt"
"net/http"
Expand Down Expand Up @@ -181,20 +182,18 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest)
volumeReq.SnapshotID = snapshotID
}

log.Info("checking volume limit")
details, err := d.checkLimit(ctx)
if err != nil {
return nil, status.Errorf(codes.Internal, "failed to check volume limit: %s", err)
}
if details != nil {
return nil, status.Errorf(codes.ResourceExhausted,
"volume limit (%d) has been reached. Current number of volumes: %d. Please contact support.",
details.limit, details.numVolumes)
}

log.WithField("volume_req", volumeReq).Info("creating volume")
vol, _, err := d.storage.CreateVolume(ctx, volumeReq)
vol, cvResp, err := d.storage.CreateVolume(ctx, volumeReq)
if err != nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@timoreimann ,

Here we have a payload ressembling this :

{
  "message": "failed to create volume: volume/snapshot capacity limit exceeded",
  "id":"forbidden",
  "request_id": "7c230c90-c7a0-4e79-a40d-b8e3ca003bd4"
}

I'm not looking at the id field of the CreateVolume response for the exact match of forbidden since it is not exported by godo. Instead, I'm relying on the http code of the response which is kind of the same check IMO. What do you think ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, wasn't aware it's not exported. I think your approach makes sense then. 👍

if cvResp != nil && cvResp.StatusCode == http.StatusForbidden {
var cvErrResp godo.ErrorResponse
if err = json.NewDecoder(cvResp.Body).Decode(&cvErrResp); err != nil {
return nil, status.Errorf(codes.Internal, "failed to decode create volume forbidden response: %s", err)
}
if strings.Contains(cvErrResp.Message, "capacity limit exceeded") {
return nil, status.Errorf(codes.ResourceExhausted, "volume limit has been reached. Please contact support.")
}
}
return nil, status.Error(codes.Internal, err.Error())
}

Expand Down