Skip to content

[shim] Fix flaky Docker tests #4216

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.

Sign up for GitHub

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

Merged
un-def merged 1 commit into from
Aug 26, 2026
+32 −16
Merged

[shim] Fix flaky Docker tests #4216

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 32 additions & 16 deletions runner/internal/shim/docker_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,17 @@ import (
"github.com/stretchr/testify/require"
)

/*
Docker tests are not parallel: they share the Docker daemon, and a DockerRunner adopts
every task container on the host, including the containers of the other tests.
*/

// TestDocker_SSHServer pulls ubuntu image (without sshd), installs openssh-server and exits
// Basically, it indirectly tests a shell script generated by getSSHShellCommands
func TestDocker_SSHServer(t *testing.T) {
if testing.Short() {
t.Skip()
}
t.Parallel()

params := &dockerParametersMock{
commands: []string{"/usr/sbin/sshd -V 2>&1 | grep OpenSSH"},
Expand All @@ -39,7 +43,7 @@ func TestDocker_SSHServer(t *testing.T) {
require.NoError(t, err)

taskConfig := createTaskConfig(t)
defer dockerRunner.Remove(t.Context(), taskConfig.ID)
defer cleanupTask(t, dockerRunner, taskConfig.ID)

assert.NoError(t, dockerRunner.Submit(ctx, taskConfig))
assert.NoError(t, dockerRunner.Start(ctx, taskConfig.ID))
Expand All @@ -50,7 +54,6 @@ func TestDocker_ShmNoexecByDefault(t *testing.T) {
if testing.Short() {
t.Skip()
}
t.Parallel()

params := &dockerParametersMock{
commands: []string{"mount | grep '/dev/shm .*size=65536k' | grep noexec"},
Expand All @@ -65,7 +68,7 @@ func TestDocker_ShmNoexecByDefault(t *testing.T) {
require.NoError(t, err)

taskConfig := createTaskConfig(t)
defer dockerRunner.Remove(t.Context(), taskConfig.ID)
defer cleanupTask(t, dockerRunner, taskConfig.ID)

assert.NoError(t, dockerRunner.Submit(ctx, taskConfig))
assert.NoError(t, dockerRunner.Start(ctx, taskConfig.ID))
Expand All @@ -76,7 +79,6 @@ func TestDocker_ShmExecIfSizeSpecified(t *testing.T) {
if testing.Short() {
t.Skip()
}
t.Parallel()

params := &dockerParametersMock{
commands: []string{"mount | grep '/dev/shm .*size=1024k' | grep -v noexec"},
Expand All @@ -92,7 +94,7 @@ func TestDocker_ShmExecIfSizeSpecified(t *testing.T) {

taskConfig := createTaskConfig(t)
taskConfig.ShmSize = 1024 * 1024
defer dockerRunner.Remove(t.Context(), taskConfig.ID)
defer cleanupTask(t, dockerRunner, taskConfig.ID)

assert.NoError(t, dockerRunner.Submit(ctx, taskConfig))
assert.NoError(t, dockerRunner.Start(ctx, taskConfig.ID))
Expand All @@ -103,7 +105,6 @@ func TestDocker_ContainerExitedWithError(t *testing.T) {
if testing.Short() {
t.Skip()
}
t.Parallel()

params := &dockerParametersMock{
commands: []string{"echo failed for a reason", "exit 3"},
Expand All @@ -118,7 +119,7 @@ func TestDocker_ContainerExitedWithError(t *testing.T) {
require.NoError(t, err)

taskConfig := createTaskConfig(t)
defer dockerRunner.Remove(t.Context(), taskConfig.ID)
defer cleanupTask(t, dockerRunner, taskConfig.ID)

require.NoError(t, dockerRunner.Submit(ctx, taskConfig))
require.NoError(t, dockerRunner.Start(ctx, taskConfig.ID))
Expand All @@ -135,7 +136,6 @@ func TestDocker_RestoredTaskIsTerminated(t *testing.T) {
if testing.Short() {
t.Skip()
}
t.Parallel()

params := &dockerParametersMock{
commands: []string{"sleep 3", "exit 7"},
Expand All @@ -156,7 +156,7 @@ func TestDocker_RestoredTaskIsTerminated(t *testing.T) {
// The restarted shim restores the task from its container
restartedRunner, err := NewDockerRunner(ctx, params)
require.NoError(t, err)
defer restartedRunner.Remove(t.Context(), taskConfig.ID)
defer cleanupTask(t, restartedRunner, taskConfig.ID)
require.Equal(t, taskConfig.ID, restartedRunner.TaskInfo(taskConfig.ID).ID)

taskInfo := waitTaskTerminated(t, restartedRunner, taskConfig.ID)
Expand Down Expand Up @@ -271,16 +271,32 @@ func assertTaskDone(t *testing.T, runner *DockerRunner, taskID string) {
}

// waitTaskTerminated processes tasks until the task is terminated, as Start() only
// starts the container and does not wait for it to exit
// starts the container and does not wait for it to exit.
// The timeout covers the whole container lifetime, which may be long, e.g., if the
// container installs packages
func waitTaskTerminated(t *testing.T, runner *DockerRunner, taskID string) TaskInfo {
t.Helper()
var taskInfo TaskInfo
require.Eventually(t, func() bool {
runner.ProcessTasks(t.Context())
taskInfo = runner.TaskInfo(taskID)
return taskInfo.Status == TaskStatusTerminated
}, 60*time.Second, 200*time.Millisecond)
return taskInfo
return runner.TaskInfo(taskID).Status == TaskStatusTerminated
}, 150*time.Second, 200*time.Millisecond)
// Read outside of the condition, which Eventually runs in a separate goroutine
// that may outlive the call
return runner.TaskInfo(taskID)
}

// cleanupTask terminates and removes the task, so that its container does not outlive
// the test even if the test fails. A container left behind is adopted by the runners of
// the following test runs, breaking them in confusing ways
func cleanupTask(t *testing.T, runner *DockerRunner, taskID string) {
t.Helper()
reason := string(types.TerminationReasonTerminatedByUser)
if err := runner.Terminate(t.Context(), taskID, 0, reason, "test cleanup"); err != nil {
t.Logf("failed to terminate task %s: %s", taskID, err)
}
if err := runner.Remove(t.Context(), taskID); err != nil {
t.Logf("failed to remove task %s: %s", taskID, err)
}
}

func createTaskConfig(t *testing.T) TaskConfig {
Expand Down
Loading