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.

Sign up for GitHub

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

Jump to bottom

Fix tiering with complex permissions and rodsusers running tiering rules (4-3-stable) #295

Merged

alanking merged 6 commits into irods:4-3-stable from alanking:perms.43s

Jan 15, 2025

Contributor

alanking commented Jan 13, 2025 •

edited

Loading

Addresses #164
Addresses #189
Addresses #273
Addresses #293

A few notes with this change:

Leaving in draft for now since I need to run the full test suite and make sure things are working when built against true-blue 4.3.3 packages. The build hook currently requires you to provide iRODS packages so I want to make extra sure I haven't been testing against tip of 4-3-stable or something that is not bit-for-bit identical to 4.3.3 iRODS packages.
I didn't change any of the internal interfaces (e.g. removing the user and zone parameters from function signatures), but we can update those if desired. Felt like a bit of a clean-up maneuver, distracting from the meat of the change.
"Run all storage tiering ops as admin" is not quite accurate as updating access_time metadata is not being done by rodsadmins in this change. That will come in a later change to address Downloading a read-only file via python client gives irods.exception.CAT_NO_ACCESS_PERMISSION: failed to set access time for [file] #175 and CAT_NO_ACCESS_PERMISSION and USER_PACKSTRUCT_INPUT_ERR on open/read/close activity #203. The reason it is not included in this change is that this one is already quite big and testing for the updating of access_time for the read-only case is going to require yet more changes, so it felt better to leave those off of this PR. Can update that commit message if desired.
The tests provided here are very basic and may not be comprehensive. I made sure to hit the use cases in the linked issues, at least.

Contributor Author

alanking commented Jan 14, 2025

Re-built with 4.3.3 packages downloaded from packages.irods.org and saw all the tests pass. Marking ready for review.

alanking marked this pull request as ready for review

January 14, 2025 16:16

korydraughn reviewed

View reviewed changes

Collaborator

korydraughn left a comment

Looking real good.

packaging/test_plugin_unified_storage_tiering.py Show resolved Hide resolved

packaging/test_plugin_unified_storage_tiering.py Outdated Show resolved Hide resolved

src/main.cpp Outdated Show resolved Hide resolved

packaging/test_plugin_unified_storage_tiering.py Outdated Show resolved Hide resolved

packaging/test_plugin_unified_storage_tiering.py Outdated Show resolved Hide resolved

include/irods/private/storage_tiering/proxy_connection.hpp Show resolved Hide resolved

src/storage_tiering.cpp Outdated Show resolved Hide resolved

src/storage_tiering.cpp Outdated Show resolved Hide resolved

src/storage_tiering.cpp Outdated Show resolved Hide resolved

src/storage_tiering.cpp Outdated Show resolved Hide resolved

trel reviewed

View reviewed changes

packaging/test_plugin_unified_storage_tiering.py Outdated Show resolved Hide resolved

korydraughn requested review from MartinFlores751 and FifthPotato

January 14, 2025 18:27

Contributor Author

alanking commented Jan 14, 2025

All tests passed

korydraughn reviewed

View reviewed changes

src/main.cpp Show resolved Hide resolved

src/main.cpp Show resolved Hide resolved

packaging/test_plugin_unified_storage_tiering.py Outdated Show resolved Hide resolved

FifthPotato reviewed

View reviewed changes

FifthPotato left a comment

Looks good. Just a few things from me!

packaging/test_plugin_unified_storage_tiering.py Outdated Show resolved Hide resolved

src/storage_tiering.cpp Show resolved Hide resolved

src/storage_tiering.cpp Outdated Show resolved Hide resolved

alanking mentioned this pull request

Investigate changing object_is_processed to std::set or std::unordered_set #298

Open

trel reviewed

View reviewed changes

src/storage_tiering.cpp Outdated Show resolved Hide resolved

MartinFlores751 reviewed

View reviewed changes

MartinFlores751 left a comment

Few comments on my part. Some repeat questions about use of make(), mostly for understanding.

include/irods/private/storage_tiering/proxy_connection.hpp Outdated Show resolved Hide resolved

include/irods/private/storage_tiering/proxy_connection.hpp Outdated Show resolved Hide resolved

src/main.cpp Show resolved Hide resolved

src/main.cpp Outdated Show resolved Hide resolved

src/main.cpp Outdated Show resolved Hide resolved

src/storage_tiering.cpp Outdated Show resolved Hide resolved

Contributor Author

alanking commented Jan 15, 2025

Everything builds, just need to run tests again. Will watch for additional comments

Contributor Author

alanking commented Jan 15, 2025

All tests passed.

Collaborator

korydraughn commented Jan 15, 2025

Good. Is there anything else left to do for this PR?

Contributor Author

alanking commented Jan 15, 2025

If there are no additional review comments, I think it's in a good spot as a stake in the ground. There are some things which were raised in the review which have been captured in issues and will (probably) be addressed before release.

korydraughn reviewed

View reviewed changes

Collaborator

korydraughn left a comment

Squash it and format it.

alanking force-pushed the perms.43s branch from be2b2f4 to 7a697cf Compare

January 15, 2025 17:43

Contributor Author

alanking commented Jan 15, 2025

Squashed. Formatting satisfied

alanking added 4 commits

January 15, 2025 12:44


          [irods#292] .clang-format: Do not use tabs nor align trailing comments

9539f3b

Restore use of tabs before running clang-format on all the code.

AlignTrailingComments is false in irods/irods and is generally annoying,
so don't restore that.


          [irods#293] Add tests for tiering rules as rodsuser

683e5c3

Also adds ability to specify a session for running the
tiering rule in tests. Previously, this was done using
a temporary session for an existing admin user.


          [irods#293] Only allow rodsadmins to run tiering rules

fd45e87


          [irods#164,irods#189,irods#273] Add tests for tiering with various pe…

43ade4d

…rmissions

Collaborator

korydraughn commented Jan 15, 2025

Is the commit message for 2dc3f29 still good. I'm mostly looking at the mention of proxy_connection::make having two overloads, but the names being different following @MartinFlores751's comments.

Also, are both proxy_connection::make member functions used in the code or is the new rodsadmin function used exclusively?

Contributor Author

alanking commented Jan 15, 2025

Is the commit message for 2dc3f29 still good. I'm mostly looking at the mention of proxy_connection::make having two overloads, but the names being different following @MartinFlores751's comments.

Hmm... true, that did change. Will update.

Also, are both proxy_connection::make member functions used in the code or is the new rodsadmin function used exclusively?

I think all instances have switched to using the new one.

Collaborator

korydraughn commented Jan 15, 2025

I think all instances have switched to using the new one.

Consider marking it deprecated to guard against future usage.

Contributor Author

alanking commented Jan 15, 2025

I wouldn't say that its usage is discouraged... It still works, after all. It's just that the plugin is no longer using it because we're going in a different direction (admin only everywhere). Considering it's in a private header file specific to this plugin and it may just get removed in the very near future anyway (#296 (comment)), I don't think we need to mark it deprecated. Thoughts?

Member

trel commented Jan 15, 2025

agreed - we'll be torching it before a release most probably.

korydraughn approved these changes

View reviewed changes

Collaborator

korydraughn left a comment

Pound it.


          [irods#164,irods#189,irods#273] Run all storage tiering ops as admin

08e3019

This commit makes a couple of changes:

1. Added proxy_connection::make_rodsadmin_connection.
This function simply establishes a client connection as the
local service account rodsadmin. Importantly, it also sets
the authFlag in the created RcComm in order to appropriately
indicate the privilege level of the client user.

2. All instances of establishing a connection with the local
server to perform actions have been replaced with connections
as the local service account rodsadmin. This allows storage tiering
to do its job properly without needing to worry about user
permissions. Note: Violating queries still select the USER_NAME
and USER_ZONE, but they are no longer used.


          [irods#273] Fix race condition in migration scheduler

4059c1a

The lambda function fed to the irods::query_processor for
scheduling violating data objects for migration had a race
condition wherein objects marked as already having been
processed were not protected by a mutex. This resulted in
data objects with multiple users or groups with permissions
on those data objects being scheduled for migration multiple
times.

alanking force-pushed the perms.43s branch from 7a697cf to 4059c1a Compare

January 15, 2025 19:42

Contributor Author

alanking commented Jan 15, 2025

Removed mention of overloads from commit message and #'d. Please eyeball message before I merge

alanking mentioned this pull request

Fix tiering with complex permissions and rodsusers running tiering rules (main) #299

Merged

Collaborator

korydraughn commented Jan 15, 2025

Merge it!

alanking merged commit 6a2a095 into irods:4-3-stable

1 check passed

alanking deleted the perms.43s branch

January 15, 2025 20:41

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet