Skip to content

fix(agent/agentcontainers): handle race between docker ps and docker inspect #17447

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
merged 5 commits into from
Apr 17, 2025
Merged

fix(agent/agentcontainers): handle race between docker ps and docker inspect #17447

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8c05144
fix(agent/agentcontainers): handle race between docker ps and docker …
johnstcn Apr 17, 2025
6d921f2
chore(agent/agentcontainers): skip devcontainer CLI test on non-Linux OS
johnstcn Apr 17, 2025
11ce370
unparam
johnstcn Apr 17, 2025
3d63bad
address PR comments
johnstcn Apr 17, 2025
23bcd23
avoid disconnected error handling
johnstcn Apr 17, 2025
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
39 changes: 22 additions & 17 deletions agent/agentcontainers/containers_dockercli.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,19 +24,6 @@ import (
"github.com/coder/coder/v2/codersdk"
)

// DockerCLILister is a ContainerLister that lists containers using the docker CLI
type DockerCLILister struct {
execer agentexec.Execer
}

var _ Lister = &DockerCLILister{}

func NewDocker(execer agentexec.Execer) Lister {
return &DockerCLILister{
execer: agentexec.DefaultExecer,
}
}

// DockerEnvInfoer is an implementation of agentssh.EnvInfoer that returns
// information about a container.
type DockerEnvInfoer struct {
Expand Down Expand Up @@ -241,6 +228,19 @@ func run(ctx context.Context, execer agentexec.Execer, cmd string, args ...strin
return stdout, stderr, err
}

// DockerCLILister is a ContainerLister that lists containers using the docker CLI
type DockerCLILister struct {
execer agentexec.Execer
}

var _ Lister = &DockerCLILister{}

func NewDocker(execer agentexec.Execer) Lister {
return &DockerCLILister{
execer: agentexec.DefaultExecer,
}
}

func (dcl *DockerCLILister) List(ctx context.Context) (codersdk.WorkspaceAgentListContainersResponse, error) {
var stdoutBuf, stderrBuf bytes.Buffer
// List all container IDs, one per line, with no truncation
Expand Down Expand Up @@ -310,19 +310,24 @@ func (dcl *DockerCLILister) List(ctx context.Context) (codersdk.WorkspaceAgentLi
// runDockerInspect is a helper function that runs `docker inspect` on the given
// container IDs and returns the parsed output.
// The stderr output is also returned for logging purposes.
func runDockerInspect(ctx context.Context, execer agentexec.Execer, ids ...string) (stdout, stderr []byte, err error) {
func runDockerInspect(ctx context.Context, execer agentexec.Execer, ids ...string) (stdout, stderr []byte, retErr error) {
var stdoutBuf, stderrBuf bytes.Buffer
cmd := execer.CommandContext(ctx, "docker", append([]string{"inspect"}, ids...)...)
cmd.Stdout = &stdoutBuf
cmd.Stderr = &stderrBuf
err = cmd.Run()
err := cmd.Run()
stdout = bytes.TrimSpace(stdoutBuf.Bytes())
stderr = bytes.TrimSpace(stderrBuf.Bytes())
if err != nil {
return stdout, stderr, err
if bytes.Contains(stderr, []byte("No such object:")) {
// This can happen if a container is deleted between the time we check for its existence and the time we inspect it.
retErr = nil
} else {
retErr = err
}
}

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

Nit: Ideally I'd like to avoid disconnected err handling (code slipping in between err := and return err due to whitespace), but I'll settle for removing the newline.

(Preferable, but not required: return explicitly either nil or err from if err != nil-block.)

return stdout, stderr, nil
return stdout, stderr, retErr
}

// To avoid a direct dependency on the Docker API, we use the docker CLI
Expand Down
7 changes: 7 additions & 0 deletions agent/agentcontainers/devcontainercli_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"os"
"os/exec"
"path/filepath"
"runtime"
"strings"
"testing"

Expand Down Expand Up @@ -229,6 +230,12 @@ func TestDockerDevcontainerCLI(t *testing.T) {
if os.Getenv("CODER_TEST_USE_DOCKER") != "1" {
t.Skip("skipping Docker test; set CODER_TEST_USE_DOCKER=1 to run")
}
if runtime.GOOS != "linux" {
t.Skip("Skipping on non-Linux OS")
}
if _, err := exec.LookPath("devcontainer"); err != nil {
t.Fatal("this test requires the devcontainer CLI: npm install -g @devcontainers/cli")
}
Copy link
Member Author

Choose a reason for hiding this comment

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

review: I didn't look too closely into what was actually going on here, but I suspect it's related to path differences between the host and the Docker VM on MacOS/Windows.

Copy link
Member

Choose a reason for hiding this comment

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

I've run this on macOS as well, so IMO we could allow that, but agree that we need to verify the devcontainer CLI here, good add 👍🏻


// Connect to Docker.
pool, err := dockertest.NewPool("")
Expand Down
Loading