Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[backport 23.0] TestDaemonRestartKillContainers: Fix races #45196

Merged
merged 2 commits into from Mar 23, 2023

Conversation

vvoland
Copy link
Contributor

@vvoland vvoland commented Mar 22, 2023

TestDaemonRestartKillContainers: Fix loop capture

TestDaemonRestartKillContainers test was always executing the last case (container created should not be restarted) because the iterated variables were not copied correctly.
Capture iterated values by value correctly.

StartWithLogFile: Fix d.cmd race

Use exec.Command created by this function instead of obtaining it from daemon struct. This prevents a race condition where daemon.Kill is called before the goroutine has the chance to call cmd.Wait.

Detected by -race:

WARNING: DATA RACE
Write at 0x00c00071c048 by goroutine 40:
  github.com/docker/docker/testutil/daemon.(*Daemon).Kill.func1()
      /go/src/github.com/docker/docker/testutil/daemon/daemon.go:497 +0x88
  runtime.deferreturn()
      /usr/local/go/src/runtime/panic.go:476 +0x32
  github.com/docker/docker/integration/container.TestDaemonRestartKillContainers.func1()
      /go/src/github.com/docker/docker/integration/container/restart_test.go:72 +0x31
  github.com/docker/docker/integration/container.TestDaemonRestartKillContainers.func3()
      /go/src/github.com/docker/docker/integration/container/restart_test.go:111 +0x6b8
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:1576 +0x216
  testing.(*T).Run.func1()
      /usr/local/go/src/testing/testing.go:1629 +0x47

Previous read at 0x00c00071c048 by goroutine 50:
  github.com/docker/docker/testutil/daemon.(*Daemon).StartWithLogFile.func1()
      /go/src/github.com/docker/docker/testutil/daemon/daemon.go:413 +0x46

- How to verify it

make  BUILDFLAGS='-race'  TEST_FILTER='TestDaemonRestartKillContainers'  test-integration

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)
image

TestDaemonRestartKillContainers test was always executing the last case
(`container created should not be restarted`) because the iterated
variables were not copied correctly.
Capture iterated values by value correctly and rename c to tc.

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
(cherry picked from commit fed1c96)
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Use `exec.Command` created by this function instead of obtaining it from
daemon struct. This prevents a race condition where `daemon.Kill` is
called before the goroutine has the chance to call `cmd.Wait`.

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
(cherry picked from commit 88992de)
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
@vvoland vvoland added area/testing kind/bugfix PR's that fix bugs labels Mar 22, 2023
@vvoland vvoland added this to the 23.0.3 milestone Mar 22, 2023
@vvoland vvoland changed the title TestDaemonRestartKillContainers: Fix races [backport 23.0] TestDaemonRestartKillContainers: Fix races Mar 22, 2023
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@vvoland
Copy link
Contributor Author

vvoland commented Mar 22, 2023

Failures unrelated:

=== FAIL: libnetwork/networkdb TestNetworkDBCRUDMediumCluster (22.04s)
    networkdb_test.go:426: timeout hit after 20s: node2:Waiting for cluster peers to be established
=== Failed
=== FAIL: github.com/docker/docker/integration/container TestLogs/driver_json-file/without_tty/only_stderr (25.03s)
    logs_test.go:139: timeout hit after 10s: waiting for container to be stopped
        --- FAIL: TestLogs/driver_json-file/without_tty/only_stderr (25.03s)

=== FAIL: github.com/docker/docker/integration/container TestLogs/driver_json-file (0.04s)
    --- FAIL: TestLogs/driver_json-file (0.04s)

=== FAIL: github.com/docker/docker/integration/container TestLogs (43.74s)

@thaJeztah thaJeztah modified the milestones: 23.0.3, 23.0.2 Mar 23, 2023
@neersighted neersighted merged commit 219f21b into moby:23.0 Mar 23, 2023
86 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants