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.

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

Add admin bucket quota command to manage quota #3179

Merged
merged 2 commits into from May 27, 2020

Conversation

poornas
Copy link
Contributor

@poornas poornas commented Apr 18, 2020

companion to PR minio/minio#9379

cmd/admin-bucket-quota.go Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
Copy link
Collaborator

@ebozduman ebozduman left a comment

Choose a reason for hiding this comment

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

It looks like the documentation changes are missing.

@ebozduman
Copy link
Collaborator

@poornas
Please also check the build test failure.

Copy link
Collaborator

@ebozduman ebozduman left a comment

Choose a reason for hiding this comment

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

The usage information should also explain the behavior when no quota flag, --fifo or --hard is provided, like
mc admin bucket quota <alias>/<bucket>
This command causes the bucket's quota configuration to be listed, if one exists.

Copy link
Collaborator

@ebozduman ebozduman left a comment

Choose a reason for hiding this comment

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

Where the quota value needs to go is not clear in the usage and doc (doesn't exist yet) explanations.

The only place you can see it is the last example.

Don't we need to make it clear in the usage the expectation somehow. I looked at the usage in the help and tried:

mc admin bucket quota --fifo 64m <alias>/<bucket>`
mc: <ERROR> Unable to initialize admin connection. No valid configuration found for '64m' host alias.

I think this is essential usage information and needs to be clear in the help:
USAGE: mc admin bucket COMMAND [COMMAND FLAGS | -h] [ARGUMENTS...] is not clear in that respect.

Another missing information is; if there is a default value for the type information or not.
If there is no default and TYPE field is mandatory, then we need to make sure it is clear in the USAGE.
Existing USAGE shows flags optional, [COMMAND FLAGS | -h].

cmd/admin-bucket-quota.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@ebozduman ebozduman left a comment

Choose a reason for hiding this comment

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

This suggestion can be considered as a future enhancement

IMHO, it would be a good idea to provide how much --hard space left out of the set quota limit. So, hard hitting the limit should not be the only way we communicate with the user that there is not enough space available for the transaction.

Just an idea: we can add the available space information to the output of quota list functionality.

mc admin bucket quota myminio/mybucket
Bucket `mybucket` has hard quota of 5.0 GiB
2.7GiB is available

cmd/admin-bucket-quota.go Outdated Show resolved Hide resolved
cmd/admin-bucket-quota.go Outdated Show resolved Hide resolved
cmd/admin-bucket-quota.go Show resolved Hide resolved
cmd/admin-bucket-quota.go Outdated Show resolved Hide resolved
cmd/admin-bucket-quota.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@ebozduman ebozduman left a comment

Choose a reason for hiding this comment

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

Newly created --clear option always fails:

$ mc admin bucket quota myminio/bucket 1gb --hard
Successfully set bucket quota of 954 MiB with hard type on `bucket`
$ mc admin bucket quota --clear myminio/bucket
mc: <ERROR> Cannot clear bucket quota config. Failed to parse server response.

This looks like a server side problem though, in minio/minio#9379.

ebozduman
ebozduman previously approved these changes Apr 21, 2020
Copy link
Collaborator

@ebozduman ebozduman left a comment

Choose a reason for hiding this comment

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

LGTM

cmd/admin-bucket-quota.go Outdated Show resolved Hide resolved
cmd/admin-bucket-quota.go Show resolved Hide resolved
ebozduman
ebozduman previously approved these changes Apr 25, 2020
Copy link
Collaborator

@ebozduman ebozduman left a comment

Choose a reason for hiding this comment

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

LGTM (please fix the build test faiure)

@ebozduman
Copy link
Collaborator

@poornas,
As soon as you fix the test errors, I'll approve it.

krisis
krisis previously approved these changes Apr 28, 2020
@poornas
Copy link
Contributor Author

poornas commented Apr 28, 2020

@poornas,
As soon as you fix the test errors, I'll approve it.

@ebozduman , travis will pass when the minio/minio PR gets merged.

ebozduman
ebozduman previously approved these changes Apr 28, 2020
Copy link
Collaborator

@ebozduman ebozduman left a comment

Choose a reason for hiding this comment

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

LGTM

@poornas poornas dismissed stale reviews from ebozduman and krisis via aef148a April 30, 2020 23:00
@poornas poornas force-pushed the quota branch 3 times, most recently from 731f98c to 8b8b11d Compare May 6, 2020 21:02
@poornas
Copy link
Contributor Author

poornas commented May 6, 2020

#3182 needs to be merged before this PR

cmd/admin-bucket-quota.go Outdated Show resolved Hide resolved
cmd/admin-bucket-quota.go Outdated Show resolved Hide resolved
cmd/admin-bucket-quota.go Outdated Show resolved Hide resolved
cmd/admin-bucket-quota.go Outdated Show resolved Hide resolved
docs/minio-admin-complete-guide.md Outdated Show resolved Hide resolved
cmd/admin-bucket-quota.go Outdated Show resolved Hide resolved
@poornas poornas force-pushed the quota branch 2 times, most recently from 4e71831 to b2da582 Compare May 26, 2020 22:15
cmd/admin-bucket-quota.go Outdated Show resolved Hide resolved
cmd/admin-bucket-quota.go Outdated Show resolved Hide resolved
cmd/admin-bucket-quota.go Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
@harshavardhana
Copy link
Member

The dependencies are still not updated looks like @poornas CI is failing.

Copy link
Collaborator

@ebozduman ebozduman left a comment

Choose a reason for hiding this comment

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

LGTM

@harshavardhana harshavardhana merged commit f55e9c1 into minio:master May 27, 2020
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.

None yet

5 participants