From 7aa1380f8dbca657b347df513baca513c10650ba Mon Sep 17 00:00:00 2001 From: Lucas Di Pentima Date: Thu, 2 Sep 2021 15:54:23 -0300 Subject: [PATCH] 18076: Fixes the bug, expands the test with additional checks. If there's a stale cached user record that's creating the username 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. Its username was taken by other user so the new username is coming in the batch_update operation -- it's ok to temporarily have it set to nil. 2. The stale user record doesn't exist on LoginCluster anymore, so having it being reset to nil isn't harmful and avoids future collisions. Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima --- lib/controller/integration_test.go | 18 +++++++++++++++--- .../controllers/arvados/v1/users_controller.rb | 17 ++++++++++++++++- 2 files changed, 31 insertions(+), 4 deletions(-) diff --git a/lib/controller/integration_test.go b/lib/controller/integration_test.go index def8fd4858..d2d852cfe2 100644 --- a/lib/controller/integration_test.go +++ b/lib/controller/integration_test.go @@ -678,7 +678,7 @@ func (s *IntegrationSuite) TestStaleCachedUserRecord(c *check.C) { // Create some users, request them on the federated cluster so they're cached. var users []arvados.User - for userNr := range []int{1, 2} { + for userNr := range []int{0, 1} { _, _, _, user := s.testClusters["z1111"].UserClients( rootctx1, c, @@ -724,9 +724,21 @@ func (s *IntegrationSuite) TestStaleCachedUserRecord(c *check.C) { }) c.Assert(err, check.Equals, nil) - // Re-request the list on the federated cluster - _, err = conn3.UserList(rootctx1, arvados.ListOptions{Limit: math.MaxInt64}) + // Re-request the list on the federated cluster & check for updates + lst, err := conn3.UserList(rootctx1, arvados.ListOptions{Limit: math.MaxInt64}) c.Assert(err, check.Equals, nil) + var user0Found, user1Found bool + for _, user := range lst.Items { + if user.UUID == users[0].UUID { + user0Found = true + c.Assert(user.Username, check.Equals, users[1].Username) + } else if user.UUID == users[1].UUID { + user1Found = true + c.Assert(user.Username, check.Equals, users[0].Username) + } + } + c.Assert(user0Found, check.Equals, true) + c.Assert(user1Found, check.Equals, true) } // Test for bug #16263 diff --git a/services/api/app/controllers/arvados/v1/users_controller.rb b/services/api/app/controllers/arvados/v1/users_controller.rb index f4d42edf6c..82594c1eb3 100644 --- a/services/api/app/controllers/arvados/v1/users_controller.rb +++ b/services/api/app/controllers/arvados/v1/users_controller.rb @@ -29,7 +29,22 @@ class Arvados::V1::UsersController < ApplicationController end end if needupdate.length > 0 - u.update_attributes!(needupdate) + begin + u.update_attributes!(needupdate) + rescue ActiveRecord::RecordInvalid + loginCluster = Rails.configuration.Login.LoginCluster + if u.uuid[0..4] == loginCluster && !needupdate[:username].nil? + local_user = User.find_by_username(needupdate[:username]) + # A cached user record from the LoginCluster is stale, reset its username + # and retry the update operation. + if local_user.andand.uuid[0..4] == loginCluster && local_user.uuid != u.uuid + Rails.logger.warn("cached username '#{needupdate[:username]}' collision with user '#{local_user.uuid}' - resetting") + local_user.update_attributes!({username: nil}) + retry + end + end + raise # Not the issue we're handling above + end end @objects << u end -- 2.30.2