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 <lucas.dipentima@curii.com>

diff --git a/lib/controller/integration_test.go b/lib/controller/integration_test.go
index def8fd4858ee11a591372ad6311eb17f936a88f0..d2d852cfe28970b8824f554f8e4932d7686c6bcf 100644 (file)
--- 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 f4d42edf6c1891e69e644d4a0d0c86cd952a0aa1..82594c1eb3fb0996c4055b1f7a65120dc42b2524 100644 (file)
--- 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