Project

General

Profile

Actions

Story #18693

closed

Deduplicate permission links

Added by Peter Amstutz about 1 year ago. Updated 10 days ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
API
Target version:
Start date:
01/03/2023
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
2.0
Release relationship:
Auto

Description

It's confusing and potentially error prone to have multiple identical permission links, e.g. three link records all granting "can_read" going from the same user to the same project. For example, there's 50 users with read access to a project, but one user is listed 3 times. Someone goes it to remove that user's access, but only deletes one or two of the links, not all three.

Proposed change:

conflicting: permission link between the same head/tail where both permissions are either (can_read, can_write, can_login) or (can_login)

should take a row lock on the permission link when doing these operations.

  • "create" command
    • if there is a conflicting permission link and the existing link has lower permission, update the existing permission link and return that
    • if there is a conflicting permission link and the existing link has same or higher permission, do nothing and return the existing link
  • "update" command
    • if a link is updated so it conflicts with another permission link, delete the other conflicting link (this shouldn't happen because there shouldn't be more than one link)
  • "delete" command
    • delete doesn't change because there shouldn't be multiple conflicting links, but if there are, they should all get deleted
  • perform a data migration to remove any duplicated links

Subtasks 1 (0 open1 closed)

Task #19810: Review 18693-dedup-permissionsResolvedBrett Smith01/03/2023

Actions

Related issues

Related to Arvados - Bug #19057: [controller] should not allow adding the same user+login to a VM more than one timeResolvedTom Clegg01/18/2023

Actions
Related to Arvados - Story #19954: Update API documentation re permission link deduplicationResolvedTom Clegg02/01/2023

Actions
Actions #1

Updated by Peter Amstutz about 1 year ago

  • Status changed from New to In Progress
Actions #2

Updated by Peter Amstutz about 1 year ago

  • Description updated (diff)
Actions #3

Updated by Peter Amstutz about 1 year ago

  • Status changed from In Progress to New
  • Tracker changed from Bug to Story
Actions #5

Updated by Peter Amstutz 11 months ago

  • Category set to API
Actions #6

Updated by Peter Amstutz 11 months ago

  • Description updated (diff)
Actions #8

Updated by Peter Amstutz 9 months ago

  • Target version set to 2022-06-08 sprint
Actions #9

Updated by Ward Vandewege 9 months ago

  • Related to Bug #19057: [controller] should not allow adding the same user+login to a VM more than one time added
Actions #10

Updated by Peter Amstutz 9 months ago

  • Target version changed from 2022-06-08 sprint to 2022-06-22 Sprint
Actions #11

Updated by Peter Amstutz 8 months ago

  • Description updated (diff)
Actions #12

Updated by Peter Amstutz 8 months ago

  • Description updated (diff)
Actions #13

Updated by Peter Amstutz 8 months ago

  • Description updated (diff)
Actions #14

Updated by Peter Amstutz 8 months ago

  • Target version changed from 2022-06-22 Sprint to 2022-07-06
Actions #15

Updated by Peter Amstutz 8 months ago

  • Target version changed from 2022-07-06 to 2022-07-20
Actions #16

Updated by Peter Amstutz 7 months ago

  • Target version changed from 2022-07-20 to 2022-08-03 Sprint
Actions #17

Updated by Peter Amstutz 7 months ago

  • Target version changed from 2022-08-03 Sprint to 2022-08-17 sprint
Actions #18

Updated by Peter Amstutz 6 months ago

  • Target version changed from 2022-08-17 sprint to 2022-08-31 sprint
Actions #19

Updated by Peter Amstutz 6 months ago

  • Target version changed from 2022-08-31 sprint to 2022-09-14 sprint
Actions #20

Updated by Peter Amstutz 5 months ago

  • Target version changed from 2022-09-14 sprint to 2022-09-28 sprint
Actions #21

Updated by Peter Amstutz 5 months ago

  • Target version changed from 2022-09-28 sprint to 2022-10-12 sprint
Actions #22

Updated by Peter Amstutz 4 months ago

  • Target version changed from 2022-10-12 sprint to 2022-10-26 sprint
Actions #23

Updated by Peter Amstutz 4 months ago

  • Target version changed from 2022-10-26 sprint to 2022-11-09 sprint
Actions #24

Updated by Peter Amstutz 3 months ago

  • Target version changed from 2022-11-09 sprint to 2022-11-23 sprint
Actions #25

Updated by Peter Amstutz 3 months ago

  • Target version changed from 2022-11-23 sprint to 2022-12-07 Sprint
Actions #26

Updated by Peter Amstutz 3 months ago

  • Target version changed from 2022-12-07 Sprint to 2022-12-21 Sprint
Actions #27

Updated by Peter Amstutz 2 months ago

  • Target version changed from 2022-12-21 Sprint to 2023-01-18 sprint
Actions #28

Updated by Peter Amstutz 2 months ago

  • Target version changed from 2023-01-18 sprint to 2022-12-07 Sprint
Actions #29

Updated by Peter Amstutz 2 months ago

  • Assigned To set to Tom Clegg
Actions #30

Updated by Tom Clegg 2 months ago

  • Story points set to 2.0
Actions #31

Updated by Tom Clegg 2 months ago

  • Target version changed from 2022-12-07 Sprint to 2022-12-21 Sprint
Actions #32

Updated by Tom Clegg about 2 months ago

  • Status changed from New to In Progress
Actions #34

Updated by Tom Clegg about 1 month ago

note this branch also addresses #19057 (duplicate login links)

Actions #35

Updated by Tom Clegg about 1 month ago

  • Target version changed from 2022-12-21 Sprint to 2023-01-18 sprint
Actions #36

Updated by Brett Smith about 1 month ago

Tom Clegg wrote in #note-33:

18693-dedup-permissions @ 6241cccaeba4413883699c360bde08e0e544a10e -- developer-run-tests: #3431

The code all seems good. I was going to ask for a handful of tests, but then I kept realizing those situations were already covered, so thanks for that.

I feel bad asking this, but I also feel like I have to: introducing row-level locking to every API update request seems like it could have significant performance implications on a busy API server. Do we have any sense at all of what the cost is likely to look like? Does it add noticeable CPU/RAM/disk requirements to the database generally? Are users or administrators likely to see slowdown when the API server handles multiple simultaneous update requests? How much? Like, even if you just have experience adding row-level locking to some other application in the past, I would be interested to hear about that. (I admit part of my worry here is that I have no personal experience with row locking at all, so I'm just facing this giant expanse of things I know I don't know.) I realize these are huge questions, but I feel like it behooves us to have some idea of what we're getting into if we're going to make such a fundamental change.

Why was test "job readable after updating other attributes" removed from nodes_controller_test.rb?

The each block in apply_max_overlapping_permissions doesn't seem indented as I would expect.

Thanks.

Actions #37

Updated by Tom Clegg about 1 month ago

Brett Smith wrote in #note-36:

row-level locking to every API update request seems like it could have significant performance implications on a busy API server

Agree with all this. Maybe we should add a config knob (API.LockBeforeUpdate?) with default false, enable it on our own clusters first, then change the default to true when we're satisfied?

Why was test "job readable after updating other attributes" removed from nodes_controller_test.rb?

It relied on the job_readable attribute set by NodesController.find_objects_for_index, which now disappears when Rails reloads the row in @object.lock. It wouldn't be too hard to fix, but I didn't bother, because it's not actually used any more (nothing uses the nodes controller, and in particular, jobs don't get assigned to nodes because there's no crunch1 dispatcher).

The each block in apply_max_overlapping_permissions doesn't seem indented as I would expect.

I agree it's awkward, but that's the way emacs does it (the N>1th line of the "if" condition is indented by 2, and so is the inner block) ... so I figure it's better to be standard/consistent than make up something better.

Actions #38

Updated by Brett Smith about 1 month ago

Tom Clegg wrote in #note-37:

Brett Smith wrote in #note-36:

row-level locking to every API update request seems like it could have significant performance implications on a busy API server

Agree with all this. Maybe we should add a config knob (API.LockBeforeUpdate?) with default false, enable it on our own clusters first, then change the default to true when we're satisfied?

That does sound like a nice plan to me, although now we're rescoping the story and I'm not sure that's my call. I also think if we should do this, we should make at least some kind of dedicated effort to stress the server before we do a release with the feature turned on. I don't think the traffic patterns we usually get on our own clusters reach the same levels as some of our busier users.

If we go this route, I wonder if we should intentionally leave the configuration option undocumented? Basically, treat it more like a feature flag than a configuration setting we actually expect admins to change, that just happens to be implemented through the configuration system because that's easiest. I don't think we need or want to commit ourselves to a situation where admins can turn this off in the future.

Your other comments make sense to me, no further questions there.

Actions #40

Updated by Brett Smith 18 days ago

Tom Clegg wrote in #note-39:

18693-dedup-permissions @ 91550c635ed37c0a79c17f276823b48433247c8a -- developer-run-tests: #3446

Retry #2 - developer-run-tests: #3448

Looks good to me, thanks.

Actions #41

Updated by Tom Clegg 18 days ago

  • Status changed from In Progress to Resolved
Actions #42

Updated by Tom Clegg 17 days ago

  • Related to Story #19954: Update API documentation re permission link deduplication added
Actions #43

Updated by Peter Amstutz 10 days ago

  • Release set to 57
Actions

Also available in: Atom PDF