Skip to content

Conversation

@basit9958
Copy link

cc @fatih

Copy link
Contributor

@timoreimann timoreimann left a comment

Choose a reason for hiding this comment

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

Hey @basit9958 , thanks for your contribution.

I mentioned one relevant point; there are a few others worth calling out, but I'd like to get alignment first on what GetCapacity should do in order to make sure we're on the same page.


// Create a new response object
resp := &csi.GetCapacityResponse{
AvailableCapacity: totalCapacity,
Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIU the spec for GetCapacity, the availability capacity is not how much storage capacity is currently in use but how much capacity is available to the user.

I believe this is essentially the volume limit (which we still reference to some extent in the checkLimit method that is otherwise unused and was missed for removal in #481) minus the currently used capacity.

Copy link
Author

@basit9958 basit9958 Apr 13, 2023

Choose a reason for hiding this comment

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

Yes you are correct, we can use limitdetails.limit and minus it with used capacity. Used capacity can be calculated by checking volumes whose droplet id are not nil and adding their size.

@basit9958
Copy link
Author

basit9958 commented Apr 13, 2023

Hey @basit9958 , thanks for your contribution.

I mentioned one relevant point; there are a few others worth calling out, but I'd like to get alignment first on what GetCapacity should do in order to make sure we're on the same page.

@timoreimann It should return the total amount of storage capacity that can be allocated for volumes, minus the capacity already in use by existing volumes. I'll update the code tomorrow

Signed-off-by: Basit Hasan <[email protected]>
@basit9958 basit9958 requested a review from timoreimann April 14, 2023 09:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants