Arvados

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

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

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.

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.

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.

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

wb1 retry developer-run-tests-apps-workbench-integration: #3708

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.