[ARVADOS] created: 2.1.0-1281-g7aa1380f8
Git user
git at public.arvados.org
Thu Sep 2 19:03:53 UTC 2021
at 7aa1380f8dbca657b347df513baca513c10650ba (commit)
commit 7aa1380f8dbca657b347df513baca513c10650ba
Author: Lucas Di Pentima <lucas.dipentima at curii.com>
Date: Thu Sep 2 15:54:23 2021 -0300
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 at curii.com>
diff --git a/lib/controller/integration_test.go b/lib/controller/integration_test.go
index def8fd485..d2d852cfe 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 f4d42edf6..82594c1eb 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
commit 43e6f6ecae2eb26b58cfc0afb44b2b6476408741
Author: Lucas Di Pentima <lucas.dipentima at curii.com>
Date: Wed Sep 1 17:41:01 2021 -0300
18076: Adds test exposing the issue.
Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <lucas.dipentima at curii.com>
diff --git a/lib/controller/integration_test.go b/lib/controller/integration_test.go
index 70c2e9e43..def8fd485 100644
--- a/lib/controller/integration_test.go
+++ b/lib/controller/integration_test.go
@@ -662,6 +662,73 @@ func (s *IntegrationSuite) TestIntermediateCluster(c *check.C) {
}
}
+// Test for bug #18076
+func (s *IntegrationSuite) TestStaleCachedUserRecord(c *check.C) {
+ rootctx1, _, _ := s.testClusters["z1111"].RootClients()
+ conn1 := s.testClusters["z1111"].Conn()
+ conn3 := s.testClusters["z3333"].Conn()
+
+ // Make sure LoginCluster is properly configured
+ for cls := range s.testClusters {
+ c.Check(
+ s.testClusters[cls].Config.Clusters[cls].Login.LoginCluster,
+ check.Equals, "z1111",
+ check.Commentf("incorrect LoginCluster config on cluster %q", cls))
+ }
+
+ // Create some users, request them on the federated cluster so they're cached.
+ var users []arvados.User
+ for userNr := range []int{1, 2} {
+ _, _, _, user := s.testClusters["z1111"].UserClients(
+ rootctx1,
+ c,
+ conn1,
+ fmt.Sprintf("user%d at example.com", userNr),
+ true)
+ c.Assert(user.Username, check.Not(check.Equals), "")
+ users = append(users, user)
+
+ lst, err := conn3.UserList(rootctx1, arvados.ListOptions{Limit: math.MaxInt64})
+ c.Assert(err, check.Equals, nil)
+ userFound := false
+ for _, fedUser := range lst.Items {
+ if fedUser.UUID == user.UUID {
+ c.Assert(fedUser.Username, check.Equals, user.Username)
+ userFound = true
+ break
+ }
+ }
+ c.Assert(userFound, check.Equals, true)
+ }
+
+ // Swap the usernames
+ _, err := conn1.UserUpdate(rootctx1, arvados.UpdateOptions{
+ UUID: users[0].UUID,
+ Attrs: map[string]interface{}{
+ "username": "",
+ },
+ })
+ c.Assert(err, check.Equals, nil)
+ _, err = conn1.UserUpdate(rootctx1, arvados.UpdateOptions{
+ UUID: users[1].UUID,
+ Attrs: map[string]interface{}{
+ "username": users[0].Username,
+ },
+ })
+ c.Assert(err, check.Equals, nil)
+ _, err = conn1.UserUpdate(rootctx1, arvados.UpdateOptions{
+ UUID: users[0].UUID,
+ Attrs: map[string]interface{}{
+ "username": users[1].Username,
+ },
+ })
+ c.Assert(err, check.Equals, nil)
+
+ // Re-request the list on the federated cluster
+ _, err = conn3.UserList(rootctx1, arvados.ListOptions{Limit: math.MaxInt64})
+ c.Assert(err, check.Equals, nil)
+}
+
// Test for bug #16263
func (s *IntegrationSuite) TestListUsers(c *check.C) {
rootctx1, _, _ := s.testClusters["z1111"].RootClients()
-----------------------------------------------------------------------
hooks/post-receive
--
More information about the arvados-commits
mailing list