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

Bundle of fixes (main) #110

Merged
merged 8 commits into from
Apr 25, 2024
Merged

Bundle of fixes (main) #110

merged 8 commits into from
Apr 25, 2024

Conversation

korydraughn
Copy link
Collaborator

All tests passed.

Have to add a test for the commit containing (TEST THIS) in the commit message.

@korydraughn korydraughn force-pushed the bundle.m branch 2 times, most recently from 070f178 to 78c2d42 Compare April 22, 2024 17:47
@korydraughn
Copy link
Collaborator Author

After some discussion, we've decided to drop the commit containing (TEST THIS).

Original work can be found here: https://github.com/korydraughn/irods_rule_engine_plugin_logical_quotas/tree/return_violation_on_matching_total_max_data_size.m

Copy link
Contributor

@alanking alanking left a comment

Choose a reason for hiding this comment

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

Seems good. Had one suggestion

I forgot about the "pre-scan" message being removed. May be an issue for some other repos as well for 4.3.2, but we will cross those bridges when we get to them.

src/handler.cpp Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@korydraughn
Copy link
Collaborator Author

The whole nested shared monitored collections thing was added because the plugin uses the RsComm as-is to update quotas. If the user has permission to write to a collection under someone else's collection, updating the quota of the collection will work. However, when the plugin starts walking up the logical path to update quotas on parent-parent collections, that user may NOT have permission to update those which will lead to an error.

This is entirely avoidable by creating a client-side connection as the service account and updating the quotas using the admin flag. Doing that will slow things down due to TCP connection setup/teardown, but it is guaranteed to avoid the issue described. Using a new connection also means the server can exhaust its resources if there's a lot of activity happening on the server.

If use of a new client connection isn't ideal, then it's on the admin to manage the permissions and decide whether nested shared monitored collections should be allowed.

Thoughts?

@trel
Copy link
Member

trel commented Apr 24, 2024

Can we walk up the tree as-is, and then, if a permission error is hit, switch to adminFlag? So we get speed for normal cases, and then have a silent, successful fallback for the scenario you describe?

@korydraughn
Copy link
Collaborator Author

That is possible. Just need to determine if that's a simple/quick tweak or not.

@trel
Copy link
Member

trel commented Apr 24, 2024

It's a complex / not-obvious scenario. So, it can wait if there are other things, I think.

@korydraughn
Copy link
Collaborator Author

Not a blocker for sure. It was something that crossed my mind. I'll put it in an issue so it's not lost.

You agree with the additional wording in the README?

@trel
Copy link
Member

trel commented Apr 25, 2024

You agree with the additional wording in the README?

Sure. This is good - and helps people think about ramifications of alternate approaches.

Copy link
Contributor

@alanking alanking left a comment

Choose a reason for hiding this comment

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

Still seems good. Just saw one more thing

packaging/test_rule_engine_plugin_logical_quotas.py Outdated Show resolved Hide resolved
Copy link
Contributor

@alanking alanking left a comment

Choose a reason for hiding this comment

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

Squash em

@korydraughn
Copy link
Collaborator Author

All tests passed.

Squashing.

@korydraughn
Copy link
Collaborator Author

Squashed.

Copy link
Contributor

@alanking alanking left a comment

Choose a reason for hiding this comment

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

Looks good. # it.

@korydraughn
Copy link
Collaborator Author

Tweaked a test name. Will pound after the tests come back as passing.

@korydraughn
Copy link
Collaborator Author

Pounded.

@korydraughn
Copy link
Collaborator Author

Merging.

@korydraughn korydraughn merged commit 953aa4e into irods:main Apr 25, 2024
2 checks passed
@korydraughn korydraughn deleted the bundle.m branch April 25, 2024 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants