-
Couldn't load subscription status.
- Fork 9
[#418] Implement support for physical quotas (main) #457
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
base: main
Are you sure you want to change the base?
Conversation
53a6fd9 to
dd4fe0f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
noting here that there are more open quota issues in the server than i remembered...
|
Need to rebase and rerun tests. |
|
Just noticed failures with connection pooling disabled. Putting this back in draft to avoid accidental merging. |
| } | ||
| ``` | ||
|
|
||
| ## Quota Operations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this all under an "admin only" heading? if not, should we mention that this is all admin only?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are all quota operations admin only? iquota?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of the operations in this PR except stat require admin privileges.
I haven't looked at what iquota does too closely. Will take a peek.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this all under an "admin only" heading? if not, should we mention that this is all admin only?
Forgot about the admin-ness of these ops. Will add a note to each operation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, not sure about groupadmins...
| input.arg2 = group_iter->second.c_str(); | ||
| input.arg4 = quota_iter->second.c_str(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may or may not be an issue, but is there any potential to provide a string that is too long to get (correctly) packed? Or is there some kind of safety mechanism in place to prevent that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the only limit for rxGeneralAdmin is memory. We need to apply a reasonable limit to guard against malicious input like that.
I'm thinking the limit for each of these should be the max length of a group name and signed 64bit integer (i.e. try to convert it to int64_t).
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me
| # Recalculate the quotas. | ||
| r = requests.post(self.url_endpoint, headers=rodsadmin_headers, data={ | ||
| 'op': 'recalculate' | ||
| }) | ||
| self.logger.debug(r.content) | ||
|
|
||
| # Show the quotas. | ||
| r = requests.get(self.url_endpoint, headers=rodsadmin_headers, params={ | ||
| 'op': 'stat', | ||
| 'group': 'public' | ||
| }) | ||
| self.logger.debug(r.content) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this here for debugging, or is it required in order to properly clean up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's for debugging purposes only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would like to see some tests with incomplete / invalid inputs to make sure the things we care about in this client are being enforced.
This commit introduces a new endpoint, /quotas, which supports the
following operations:
- stat
- set_group_quota
- recalculate
|
This was put in draft due to there being a problem with recalculating totals for quotas. See irods/irods#8633 for details. |
|
I've confirmed that the PR at irods/irods#8657 resolves the inconsistency in quota calculations. This PR can now move forward. |
|
Just learned that irods/irods#8633 is still reproducible. All that's required is to run the test enough times. |
|
The problem might be due to the test. I don't have any evidence supporting that thought yet, but investigating. |
I'm not able to reproduce this anymore. I think my test zone may have been dirty. |
|
Okay, good - because I was very confused. |
This PR adds support for system/resource quotas (not logical quotas).
Through this work, I've noticed that the recalculation of quotas doesn't always produce the expected results. This might be caused by misuse of transactions and/or bad math in the iRODS quota code. I'll open an issue for that soon.
As of right now, support behaves as expected when the connection pool is disabled or carefully placed calls to
time.sleep()are inserted into the tests.Putting this in draft until we can decide on whether this will be part of the 0.6.0 release. The design of the operations have not been settled on yet either.
Feedback is welcome.