Skip to content

Conversation

@korydraughn
Copy link
Contributor

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.

@korydraughn korydraughn force-pushed the 418.m branch 2 times, most recently from 53a6fd9 to dd4fe0f Compare August 18, 2025 19:07
Copy link
Member

@trel trel left a 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...

@korydraughn
Copy link
Contributor Author

Need to rebase and rerun tests.

@korydraughn
Copy link
Contributor Author

Just noticed failures with connection pooling disabled. Putting this back in draft to avoid accidental merging.

@korydraughn korydraughn marked this pull request as draft August 20, 2025 17:05
}
```

## Quota Operations

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?

Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Member

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...

Comment on lines +230 to +231
input.arg2 = group_iter->second.c_str();
input.arg4 = quota_iter->second.c_str();

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?

Copy link
Contributor Author

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?

Choose a reason for hiding this comment

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

Sounds good to me

Comment on lines +4065 to +4282
# 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)

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?

Copy link
Contributor Author

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.

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
@korydraughn korydraughn changed the title [#418] Implement support for system quotas (main) [#418] Implement support for physical quotas (main) Aug 22, 2025
@korydraughn
Copy link
Contributor Author

This was put in draft due to there being a problem with recalculating totals for quotas.

See irods/irods#8633 for details.

@korydraughn
Copy link
Contributor Author

I've confirmed that the PR at irods/irods#8657 resolves the inconsistency in quota calculations.

This PR can now move forward.

@korydraughn
Copy link
Contributor Author

Just learned that irods/irods#8633 is still reproducible. All that's required is to run the test enough times.

@korydraughn
Copy link
Contributor Author

The problem might be due to the test. I don't have any evidence supporting that thought yet, but investigating.

@korydraughn
Copy link
Contributor Author

Just learned that irods/irods#8633 is still reproducible. All that's required is to run the test enough times.

I'm not able to reproduce this anymore. I think my test zone may have been dirty.

@trel
Copy link
Member

trel commented Oct 1, 2025

Okay, good - because I was very confused.

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.

3 participants