Project

General

Profile

Actions

Idea #18693

closed

Deduplicate permission links

Added by Peter Amstutz about 2 years ago. Updated about 1 year ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
API
Target version:
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/2023Actions

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/2023Actions
Related to Arvados - Idea #19954: Update API documentation re permission link deduplicationResolvedTom Clegg02/01/2023Actions
Actions #1

Updated by Peter Amstutz about 2 years ago

  • Status changed from New to In Progress
Actions #2

Updated by Peter Amstutz about 2 years ago

  • Description updated (diff)
Actions #3

Updated by Peter Amstutz about 2 years ago

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

Updated by Peter Amstutz about 2 years ago

  • Category set to API
Actions #6

Updated by Peter Amstutz about 2 years ago

  • Description updated (diff)
Actions #8

Updated by Peter Amstutz almost 2 years ago

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

Updated by Ward Vandewege almost 2 years 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 almost 2 years ago

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

Updated by Peter Amstutz almost 2 years ago

  • Description updated (diff)
Actions #12

Updated by Peter Amstutz almost 2 years ago

  • Description updated (diff)
Actions #13

Updated by Peter Amstutz almost 2 years ago

  • Description updated (diff)
Actions #14

Updated by Peter Amstutz almost 2 years ago

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

Updated by Peter Amstutz almost 2 years ago

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

Updated by Peter Amstutz over 1 year ago

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

Updated by Peter Amstutz over 1 year ago

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

Updated by Peter Amstutz over 1 year ago

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

Updated by Peter Amstutz over 1 year ago

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

Updated by Peter Amstutz over 1 year ago

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

Updated by Peter Amstutz over 1 year ago

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

Updated by Peter Amstutz over 1 year ago

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

Updated by Peter Amstutz over 1 year ago

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

Updated by Peter Amstutz over 1 year ago

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

Updated by Peter Amstutz over 1 year ago

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

Updated by Peter Amstutz over 1 year ago

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

Updated by Peter Amstutz over 1 year ago

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

Updated by Peter Amstutz over 1 year ago

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

Updated by Peter Amstutz over 1 year ago

  • Assigned To set to Tom Clegg
Actions #30

Updated by Tom Clegg over 1 year ago

  • Story points set to 2.0
Actions #31

Updated by Tom Clegg over 1 year ago

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

Updated by Tom Clegg over 1 year ago

  • Status changed from New to In Progress
Actions #34

Updated by Tom Clegg about 1 year ago

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

Actions #35

Updated by Tom Clegg about 1 year ago

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

Updated by Brett Smith about 1 year 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 year 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 year 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 about 1 year 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 about 1 year ago

  • Status changed from In Progress to Resolved
Actions #42

Updated by Tom Clegg about 1 year ago

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

Updated by Peter Amstutz about 1 year ago

  • Release set to 57
Actions

Also available in: Atom PDF