Arvados

Updates at 7aa1380 - branch 18076-stale-cached-users-handling
Test run: developer-run-tests: #2670

If there's a stale cached user record creating the username field collision, set it to nil before retrying the update. There're two scenarios this is covering:

1. The stale user record belongs to an existing user on LoginCluster. Another user took its username, so the new username is coming in the /arvados/v1/users/batch_update operation -- it's ok to have it set to nil temporarily.
2. The stale user record doesn't exist on LoginCluster anymore, so resetting it to nil isn't harmful and avoids future collisions.

The fix looks great.

• Limit: math.MaxInt64 can also be spelled Limit: -1 (probably easier to read in logs)
• The test first checks whether all clusters use z1111 as their login cluster, but all it really needs to be effective is for z1111 and z3333 to use z1111 as login cluster. (I figure eventually we'll add another cluster to our integration test suite with a different config, and at that point it will be nice to have fewer test failures to fix.)
• for userNr := range []int{0, 1} is sort of misleading in that userNr is the array index, not the array element -- []int{9,3} would have the same result. Idiomatic Go is to say for userNr := 0; userNr < 2; userNr++ if you want a sequence of numbers, or for _, userNr := range ... if you want specific int values. This is a very nitpicky nitpick though!

LGTM, thanks!

Thanks for the suggestions! Updated at c534e74c5 and merged.

Once Jenkins deployed this on ce8i5, I started to see the following:

$ arv user list
Error: error updating local user records: //railsapi.internal/arvados/v1/users/batch_update: 422 Unprocessable Entity: #<ActiveRecord::RecordNotSaved: Failed to save the record> (req-vr2et8tnawgndjcxocwb)

...and in ce8i5's logs:

[req-vr2et8tnawgndjcxocwb] cached username 'ward2' collision with user 'ce8i5-tpzed-249xu4osx4us5lg' - resetting
#<ActiveRecord::RecordNotSaved: Failed to save the record>
Error 1630934673+e96dc935: 422
{"method":"PATCH","path":"/arvados/v1/users/batch_update","format":"html","controller":"Arvados::V1::UsersController","action":"batch_update","status":422,"duration":74.54,"view":0.32,"db":22.33,"request_id":null,"client_ipaddr":"127.0.0.1","client_auth":"tordo-gj3su-000000000000000","exception":"#\u003cActiveRecord::RecordNotSaved: Failed to save the record\u003e","exception_backtrace":"/var/www/arvados-api/shared/vendor_bundle/ruby/2.5.0/gems/activerecord-5.2.6/lib/active_record/persistence.rb:308:in `save!'\n/var/www/arvados-api/shared/vendor_bundle/ruby/2.5.0/gems/activerecord-5.2.6/lib/active_record/validations.rb:52:in `save!'\n/var/www/arvados-api/shared/vendor_bundle/ruby/2.5.0/gems/activereco[...]

From reading the Rails docs, the ActiveRecord::RecordNotSaved exception is raised when a before_* callback fails when calling save!(), so I thought there was some issue with resetting the user's username... and when trying to do so manually, I got:

$ arv user update -b --uuid ce8i5-tpzed-249xu4osx4us5lg --user='{"username": null}'
Error: //railsapi.internal/arvados/v1/users/ce8i5-tpzed-249xu4osx4us5lg: 422 Unprocessable Entity: Username can't be unset when the user owns repositories (req-wt8kut96pkj91l187pjn)

So I guess this is what's triggering the problem on the users model:

  before_update :verify_repositories_empty, :if => Proc.new {
    username.nil? and username_changed?
  }

My approach would be to set the conflicting username to something random (maybe a random suffix?) instead of nil, as it seems that this wouldn't cause any issues with preexisting repositories.

Ugh! Yeah, a random string/suffix sounds like a decent workaround.

Fix at 7edaf0605 - branch 18076-user-cache-with-repository
Test run: developer-run-tests: #2676
WB1 integration test re-run: developer-run-tests-apps-workbench-integration: #2837

• Sets a "conflictNNNNNNN" random suffix to the conflicting user's username instead of nil.
• Expands the previous test.

7edaf0605 LGTM, thanks!