Skip to content

Commit

Permalink
users-mgmt-test: fix possible race condition
Browse files Browse the repository at this point in the history
The clustermesh users management test was affected by a possible race
condition due to the fact that the configuration file was overwritten
with 'os.WriteFile'. Since this method first opens the file truncating
it and then writes the desired content, we could be notified and read
the file in that time interval, hence observing an empty file. This
caused all users to be first deleted and then recreated, leading the
test to fail. To avoid this issue, we now first write a different file,
and then rename it. In this way, the update operation is atomic, and we
always observe the correct file content.

Failing test output:
level=info msg=Invoked duration="34.8µs" function="github.com/cilium/cilium/clustermesh-apiserver.registerUsersManager (users_mgmt.go:69)" subsys=hive
level=info msg=Starting subsys=hive
level=info msg="Starting managing etcd users based on configuration" file-path=/tmp/clustermesh-config159493219/users.yaml subsys=clustermesh-apiserver
level=info msg="Start hook executed" duration="115.098µs" function="*main.usersManager.Start" subsys=hive
level=info msg="User successfully configured" subsys=clustermesh-apiserver user=foo
level=info msg="User successfully configured" subsys=clustermesh-apiserver user=bar
level=info msg="User successfully configured" subsys=clustermesh-apiserver user=qux
level=info msg="User successfully removed" subsys=clustermesh-apiserver user=foo
level=info msg="User successfully removed" subsys=clustermesh-apiserver user=bar
level=info msg="User successfully removed" subsys=clustermesh-apiserver user=qux
level=info msg="User successfully configured" subsys=clustermesh-apiserver user=baz
level=info msg="User successfully configured" subsys=clustermesh-apiserver user=foo
level=info msg="User successfully configured" subsys=clustermesh-apiserver user=qux
    users_mgmt_test.go:123:
        	Error Trace:	/home/runner/work/cilium/cilium/clustermesh-apiserver/users_mgmt_test.go:123
        	Error:      	"map[baz:r3 foo:r1 qux:r4]" should have 2 item(s), but has 3
        	Test:       	TestUsersManagement
level=info msg=Stopping subsys=hive
level=info msg="Stopping managing etcd users based on configuration" file-path=/tmp/clustermesh-config159493219/users.yaml subsys=clustermesh-apiserver
level=info msg=Closing subsys=clustermesh-apiserver
level=info msg="Stop hook executed" duration="27.8µs" function="*main.usersManager.Stop" subsys=hive

Fixes: 60f94e4 ("clustermesh-apiserver: implement users management for remote clusters")
Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
  • Loading branch information
giorio94 authored and joestringer committed Mar 31, 2023
1 parent cb950e6 commit dbb13be
Showing 1 changed file with 8 additions and 2 deletions.
10 changes: 8 additions & 2 deletions clustermesh-apiserver/users_mgmt_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -113,9 +113,15 @@ func TestUsersManagement(t *testing.T) {
require.Equal(t, "r2", client.created["bar"])
require.Equal(t, "r3", client.created["qux"])

// Update the users config file, and require that changes are propagated
client.init()
require.NoError(t, os.WriteFile(cfgPath, []byte(users2), 0600))

// Update the users config file, and require that changes are propagated
// We first write to a different file and then rename it, to avoid the possible
// race condition caused by truncate + write if we detect the event sufficiently
// fast (i.e., we first read an empty file, and then the expected one).
cfgPath2 := path.Join(tmpdir, "users.yaml.2")
require.NoError(t, os.WriteFile(cfgPath2, []byte(users2), 0600))
require.NoError(t, os.Rename(cfgPath2, cfgPath))

// Wait for processing to complete
time.Sleep(25 * time.Millisecond)
Expand Down

0 comments on commit dbb13be

Please sign in to comment.