Bläddra i källkod

pkg/plugins: Guard storage and unparallel racy tests

These tests were made parallel to speed up the execution, but this
turned out to be flaky, because they mutate some shared state.

The tests use shared `storage` variable without any synchronization.
However, adding synchronization is not enough in all cases, some tests
register the same plugin, so they can't be run in parallel to each
other.

This commit adds the synchronization around `storage` variable
modification and removes parallel from the tests where it's not enough.

Before:
```
$ go test -race -v . -count 1
...
--- FAIL: TestGet (15.02s)
    --- FAIL: TestGet/not_implemented (0.00s)
        testing.go:1446: race detected during execution of test
    testing.go:1446: race detected during execution of test
FAIL
FAIL    github.com/docker/docker/pkg/plugins    17.655s
FAIL
```

After:
```
$ go test -race -v . -count 1
ok      github.com/docker/docker/pkg/plugins    32.702s
```

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Paweł Gronowski 1 år sedan
förälder
incheckning
0034a98eb1
1 ändrade filer med 14 tillägg och 3 borttagningar
  1. 14 3
      pkg/plugins/plugin_test.go

+ 14 - 3
pkg/plugins/plugin_test.go

@@ -29,7 +29,9 @@ func TestPluginAddHandler(t *testing.T) {
 	// make a plugin which is pre-activated
 	// make a plugin which is pre-activated
 	p := &Plugin{activateWait: sync.NewCond(&sync.Mutex{})}
 	p := &Plugin{activateWait: sync.NewCond(&sync.Mutex{})}
 	p.Manifest = &Manifest{Implements: []string{"bananas"}}
 	p.Manifest = &Manifest{Implements: []string{"bananas"}}
+	storage.Lock()
 	storage.plugins["qwerty"] = p
 	storage.plugins["qwerty"] = p
+	storage.Unlock()
 
 
 	testActive(t, p)
 	testActive(t, p)
 	Handle("bananas", func(_ string, _ *Client) {})
 	Handle("bananas", func(_ string, _ *Client) {})
@@ -37,7 +39,6 @@ func TestPluginAddHandler(t *testing.T) {
 }
 }
 
 
 func TestPluginWaitBadPlugin(t *testing.T) {
 func TestPluginWaitBadPlugin(t *testing.T) {
-	t.Parallel()
 	p := &Plugin{activateWait: sync.NewCond(&sync.Mutex{})}
 	p := &Plugin{activateWait: sync.NewCond(&sync.Mutex{})}
 	p.activateErr = errors.New("some junk happened")
 	p.activateErr = errors.New("some junk happened")
 	testActive(t, p)
 	testActive(t, p)
@@ -59,10 +60,14 @@ func testActive(t *testing.T, p *Plugin) {
 }
 }
 
 
 func TestGet(t *testing.T) {
 func TestGet(t *testing.T) {
-	t.Parallel()
+	// TODO: t.Parallel()
+	// TestPluginWithNoManifest also registers fruitPlugin
+
 	p := &Plugin{name: fruitPlugin, activateWait: sync.NewCond(&sync.Mutex{})}
 	p := &Plugin{name: fruitPlugin, activateWait: sync.NewCond(&sync.Mutex{})}
 	p.Manifest = &Manifest{Implements: []string{fruitImplements}}
 	p.Manifest = &Manifest{Implements: []string{fruitImplements}}
+	storage.Lock()
 	storage.plugins[fruitPlugin] = p
 	storage.plugins[fruitPlugin] = p
+	storage.Unlock()
 
 
 	t.Run("success", func(t *testing.T) {
 	t.Run("success", func(t *testing.T) {
 		plugin, err := Get(fruitPlugin, fruitImplements)
 		plugin, err := Get(fruitPlugin, fruitImplements)
@@ -94,7 +99,8 @@ func TestGet(t *testing.T) {
 }
 }
 
 
 func TestPluginWithNoManifest(t *testing.T) {
 func TestPluginWithNoManifest(t *testing.T) {
-	t.Parallel()
+	// TODO: t.Parallel()
+	// TestGet also registers fruitPlugin
 	mux, addr := setupRemotePluginServer(t)
 	mux, addr := setupRemotePluginServer(t)
 
 
 	m := Manifest{[]string{fruitImplements}}
 	m := Manifest{[]string{fruitImplements}}
@@ -120,7 +126,9 @@ func TestPluginWithNoManifest(t *testing.T) {
 		Addr:         addr,
 		Addr:         addr,
 		TLSConfig:    &tlsconfig.Options{InsecureSkipVerify: true},
 		TLSConfig:    &tlsconfig.Options{InsecureSkipVerify: true},
 	}
 	}
+	storage.Lock()
 	storage.plugins[fruitPlugin] = p
 	storage.plugins[fruitPlugin] = p
+	storage.Unlock()
 
 
 	plugin, err := Get(fruitPlugin, fruitImplements)
 	plugin, err := Get(fruitPlugin, fruitImplements)
 	if err != nil {
 	if err != nil {
@@ -133,6 +141,7 @@ func TestPluginWithNoManifest(t *testing.T) {
 
 
 func TestGetAll(t *testing.T) {
 func TestGetAll(t *testing.T) {
 	t.Parallel()
 	t.Parallel()
+
 	tmpdir := t.TempDir()
 	tmpdir := t.TempDir()
 	r := LocalRegistry{
 	r := LocalRegistry{
 		socketsPath: tmpdir,
 		socketsPath: tmpdir,
@@ -154,7 +163,9 @@ func TestGetAll(t *testing.T) {
 		t.Fatal(err)
 		t.Fatal(err)
 	}
 	}
 	plugin.Manifest = &Manifest{Implements: []string{"apple"}}
 	plugin.Manifest = &Manifest{Implements: []string{"apple"}}
+	storage.Lock()
 	storage.plugins["example"] = plugin
 	storage.plugins["example"] = plugin
+	storage.Unlock()
 
 
 	fetchedPlugins, err := r.GetAll("apple")
 	fetchedPlugins, err := r.GetAll("apple")
 	if err != nil {
 	if err != nil {