irods / python-irodsclient

A Python API for iRODS
Other
63 stars 73 forks source link

for each replica acls method seems to return available permissions #557

Closed mstfdkmn closed 3 months ago

mstfdkmn commented 4 months ago

I am not sure if this is normal but acls in the PRC looks like to list permissions differently than how iCommands shows. Please note that, we dont have the "replication in our resource anymore, and so this is reported only for documenting. (I encounter this in our pilot zone while working with objects that have more than one replica.)

python-irodsclient 2.0.0 irods 4.3.1

with iRODSSession(**zone_environment, password=password) as session:
        obj = session.data_objects.get(object_path)
        for acl in session.acls.get(obj):
            print(acl)

# the object which has two replicas
<iRODSAccess own /kuleuven_tier1_pilot/home/vsc33586/cloud_test.txt vsc33586(rodsuser) kuleuven_tier1_pilot>
<iRODSAccess own /kuleuven_tier1_pilot/home/vsc33586/cloud_test.txt vsc33586(rodsuser) kuleuven_tier1_pilot>

# the object which has a single replica
<iRODSAccess own /kuleuven_tier1_pilot/home/vsc33586/test.txt vsc33586(rodsuser) kuleuven_tier1_pilot>

objects' status and how iCommands shows acls:

✔ [May/19 13:07] vsc33586@tier2-p-login-2 ~ $ ils -L test.txt
  vsc33586          0 default;gpfs            5 2024-05-19.11:50 & test.txt
        generic    /data/home/vsc33586/test.txt
✔ [May/19 13:08] vsc33586@tier2-p-login-2 ~ $ ils -A test.txt
  /kuleuven_tier1_pilot/home/vsc33586/test.txt
        ACL - vsc33586#kuleuven_tier1_pilot:own
✔ [May/19 13:08] vsc33586@tier2-p-login-2 ~ $ ils -L cloud_test.txt
  vsc33586          0 tier1-p-irods-2020-pilot;tier1-p-irods-posix;tier1-p-irods-posix-1-4;tier1-p-irods-posix-1-a-2-a;tier1-p-irods-posix-1-a-weight;tier1-p-irods-posix-1-a           33 2021-10-05.21:53 & cloud_test.txt
        generic    /data/migrated/p1a/home/vsc33586/cloud_test.txt
  vsc33586          1 tier1-p-irods-2020-pilot;tier1-p-irods-posix;tier1-p-irods-posix-1-4;tier1-p-irods-posix-1-a-2-a;tier1-p-irods-posix-2-a-weight;tier1-p-irods-posix-2-a           33 2021-10-05.21:53 & cloud_test.txt
        generic    /data/migrated/p2a/home/vsc33586/cloud_test.txt
✔ [May/19 13:08] vsc33586@tier2-p-login-2 ~ $ ils -A cloud_test.txt
  /kuleuven_tier1_pilot/home/vsc33586/cloud_test.txt
        ACL - vsc33586#kuleuven_tier1_pilot:own
trel commented 4 months ago

Permissions are per object - not per replica - so on first glance, I agree this double listing looks like a bug.

d-w-moore commented 4 months ago

Indeed, seems like we perhaps need to make the ACLs query more "distinct", or hit the database for the replica with good status only, by default.

trel commented 4 months ago

yes, i would think unique by data_id - has nothing to do with status.

korydraughn commented 4 months ago

Seems the PRC should follow the same pattern as the filesystem library.

Notice the fetching of ACLs for a collection is handled via a specific query on L158 - ShowCollAcls.

I'm pretty sure the filesystem logic was derived partly from the implementation of ils.

trel commented 4 months ago

the filesystem logic was derived partly from the implementation of ils.

i remember this as well.

d-w-moore commented 4 months ago

Seems the PRC should follow the same pattern as the filesystem library.

Notice the fetching of ACLs for a collection is handled via a specific query on L158 - ShowCollAcls.

I'm pretty sure the filesystem logic was derived partly from the implementation of ils.

Agreed, both should use the same logic. Switching over to use of a specific query is an extensive change however, so maybe a milestone of v3.0.0 for that, and make a different issue for it ? I've got a workable solution here that should be fine for v2.1.0, for the meantime.

trel commented 4 months ago

agreed - should be fine - but could be less efficient / heavyweight when there are lots of objects to investigate/report.

workaround for 2.1.0 fixes the bug.

d-w-moore commented 4 months ago

agreed - should be fine - but could be less efficient / heavyweight when there are lots of objects to investigate/report.

workaround for 2.1.0 fixes the bug.

That's true. Another of many ways in which PRC (in current form) isn't optimal for efficiency. But at least the O(n) complexity is linear!

d-w-moore commented 4 months ago

agreed - should be fine - but could be less efficient / heavyweight when there are lots of objects to investigate/report.

workaround for 2.1.0 fixes the bug.

There's another approach that might work better for efficiency, and that's to order_by replica number and only process ACLs reported within the first set of rows, ie sharing the common replica number. Thoughts @trel @alanking @korydraughn ?

korydraughn commented 4 months ago

That sounds reasonable for now.

Switching over to use of a specific query is an extensive change however, so maybe a milestone of v3.0.0 for that, and make a different issue for it ?

What makes the switch an extensive change? Is the retrieval of ACLs complicated?

trel commented 4 months ago

i think the set -> list thing is fine. we'll make it better later - let's just fix the bug now and make a new issue for optimization.

d-w-moore commented 4 months ago

That sounds reasonable for now.

Switching over to use of a specific query is an extensive change however, so maybe a milestone of v3.0.0 for that, and make a different issue for it ?

What makes the switch an extensive change? Is the retrieval of ACLs complicated?

Just that so far the bulk of the machinery is GenQuery based. A lot of code would change, was my meaning. In principle, no , the retrieval of the string fields from the DB rows is all that's necessary under the hood.

korydraughn commented 4 months ago

Got it.

alanking commented 3 months ago

@d-w-moore - Please close if complete. Thanks!