[#418] Implement support for physical quotas (main) #457
Conversation
korydraughn
force-pushed
the
418.m
branch
2 times, most recently
from
53a6fd9 to
dd4fe0f
Compare
|
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.
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.
are all quota operations admin only? iquota?
There was a problem hiding this comment.
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.
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.
also, not sure about groupadmins...
| res.body() = json{ | ||
| {"irods_response", { | ||
| {"status_code", e.code()}, | ||
| {"status_message", e.client_display_what()} | ||
| }} | ||
| }.dump(); |
| input.arg2 = group_iter->second.c_str(); | ||
| input.arg4 = quota_iter->second.c_str(); |
There was a problem hiding this comment.
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.
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?
| # 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.
Is this here for debugging, or is it required in order to properly clean up?
There was a problem hiding this comment.
It's for debugging purposes only.
| rodsadmin_headers = {'Authorization': f'Bearer {self.rodsadmin_bearer_token}'} | ||
| resource = 'demoResc' | ||
|
|
||
| # Set a quota for the public group. This applies to ALL resources. |
There was a problem hiding this comment.
| # Set a quota for the public group. This applies to ALL resources. | |
| # Set a quota for the public group. This only applies to the specified resource. |
There was a problem hiding this comment.
no, it's for the public group across ALL resources... aka NOT for any specific resource
There was a problem hiding this comment.
The test is named test_setting_a_group_quota_for_a_specific_resource and the call specifies a resource on L3960-3965. The test after this one is called test_setting_a_group_quota_for_all_resources and makes a similar call on L4095-4099 but does not specify a resource.
There was a problem hiding this comment.
alan is correct. removing my 'no'.
There was a problem hiding this comment.
plus the embarrassment of not catching my own error before he did. daggum.
There was a problem hiding this comment.
He's highlighting that the comment doesn't align with the code which follows it.
Copy-paste situation.
Missed all of the previous comments.
There was a problem hiding this comment.
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
korydraughn
changed the title
|
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. |
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.