diff --git a/cmd/config/apply.go b/cmd/config/apply.go index 5db02fc0e..43b3fa8e0 100644 --- a/cmd/config/apply.go +++ b/cmd/config/apply.go @@ -159,7 +159,7 @@ func (cmd *ApplyCmd) inspectRunningContainer( } containerDetails := &details[0] - if !strings.EqualFold(containerDetails.State.Status, pkgworkspace.ContainerStatusRunning) { + if containerDetails.State.Status != devcconfig.ContainerStatusRunning { return nil, fmt.Errorf( "container %s is not running (status: %s)", cmd.Container, diff --git a/cmd/internal/agentworkspace/status.go b/cmd/internal/agentworkspace/status.go index 48e6450a6..99584b458 100644 --- a/cmd/internal/agentworkspace/status.go +++ b/cmd/internal/agentworkspace/status.go @@ -3,11 +3,11 @@ package agentworkspace import ( "context" "fmt" - "strings" "github.com/devsy-org/devsy/cmd/flags" "github.com/devsy-org/devsy/pkg/agent" "github.com/devsy-org/devsy/pkg/client" + config2 "github.com/devsy-org/devsy/pkg/devcontainer/config" cliflags "github.com/devsy-org/devsy/pkg/flags" "github.com/devsy-org/devsy/pkg/flags/names" "github.com/spf13/cobra" @@ -66,10 +66,11 @@ func (cmd *StatusCmd) Run(ctx context.Context) error { } // is running? - if strings.ToLower(containerDetails.State.Status) == "running" { + switch containerDetails.State.Status { + case config2.ContainerStatusRunning: fmt.Print(client.StatusRunning) return nil - } else if strings.ToLower(containerDetails.State.Status) == "exited" { + case config2.ContainerStatusExited: fmt.Print(client.StatusStopped) return nil } diff --git a/cmd/internal/container_tunnel.go b/cmd/internal/container_tunnel.go index 075d6d618..6e63d8dce 100644 --- a/cmd/internal/container_tunnel.go +++ b/cmd/internal/container_tunnel.go @@ -15,6 +15,7 @@ import ( "github.com/devsy-org/devsy/pkg/agent" pkgconfig "github.com/devsy-org/devsy/pkg/config" "github.com/devsy-org/devsy/pkg/devcontainer" + "github.com/devsy-org/devsy/pkg/devcontainer/config" "github.com/devsy-org/devsy/pkg/encoding" cliflags "github.com/devsy-org/devsy/pkg/flags" "github.com/devsy-org/devsy/pkg/flags/names" @@ -24,8 +25,6 @@ import ( "github.com/spf13/cobra" ) -const containerStatusRunning = "running" - // containerRootUser is the user to use when running commands inside the container that // require root privileges. const containerRootUser = "root" @@ -166,7 +165,7 @@ func startDevContainer( return fmt.Errorf("find devcontainer: %w", err) } - if containerDetails == nil || containerDetails.State.Status != containerStatusRunning { + if containerDetails == nil || containerDetails.State.Status != config.ContainerStatusRunning { if err := startContainer(ctx, runner, workspaceConfig); err != nil { return fmt.Errorf("start container: %w", err) } diff --git a/cmd/internal/container_tunnel_test.go b/cmd/internal/container_tunnel_test.go index 3fa9259ae..60b5641f9 100644 --- a/cmd/internal/container_tunnel_test.go +++ b/cmd/internal/container_tunnel_test.go @@ -124,7 +124,7 @@ func TestStartDevContainer_RunningContainerWithoutResultRestarts(t *testing.T) { upErr := errors.New("boom") runner := stubRunner{ findResult: &config.ContainerDetails{ - State: config.ContainerDetailsState{Status: containerStatusRunning}, + State: config.ContainerDetailsState{Status: config.ContainerStatusRunning}, }, commandErr: errors.New("cat: no such file"), // hasDevContainerResult -> false upErr: upErr, @@ -147,7 +147,7 @@ func TestStartDevContainer_RunningContainerWithoutResultRestarts(t *testing.T) { func TestStartDevContainer_RunningContainerWithResultIsNoOp(t *testing.T) { runner := stubRunner{ findResult: &config.ContainerDetails{ - State: config.ContainerDetailsState{Status: containerStatusRunning}, + State: config.ContainerDetailsState{Status: config.ContainerStatusRunning}, }, commandErr: nil, // hasDevContainerResult -> true upErr: errors.New("Up must not be called on the happy path"), diff --git a/cmd/internal/runusercommands.go b/cmd/internal/runusercommands.go index 5113a206e..373208140 100644 --- a/cmd/internal/runusercommands.go +++ b/cmd/internal/runusercommands.go @@ -299,7 +299,7 @@ func (cmd *RunUserCommandsCmd) inspectRunningContainer( } containerDetails := &details[0] - if !strings.EqualFold(containerDetails.State.Status, workspace2.ContainerStatusRunning) { + if containerDetails.State.Status != devcconfig.ContainerStatusRunning { errMsg := fmt.Sprintf( "container %s is not running (status: %s)", cmd.ContainerID, diff --git a/cmd/pro/start.go b/cmd/pro/start.go index c874b2d1b..c794fbbd8 100644 --- a/cmd/pro/start.go +++ b/cmd/pro/start.go @@ -24,6 +24,7 @@ import ( loftclientset "github.com/devsy-org/api/pkg/clientset/versioned" proflags "github.com/devsy-org/devsy/cmd/pro/flags" "github.com/devsy-org/devsy/pkg/config" + devcconfig "github.com/devsy-org/devsy/pkg/devcontainer/config" "github.com/devsy-org/devsy/pkg/hash" "github.com/devsy-org/devsy/pkg/log" "github.com/devsy-org/devsy/pkg/machineid" @@ -665,8 +666,8 @@ func (cmd *StartCmd) successDocker(ctx context.Context, containerID string) erro containerDetails, err := cmd.inspectContainer(ctx, containerID) if err != nil { return false, fmt.Errorf("inspect loft container: %w", err) - } else if strings.ToLower(containerDetails.State.Status) == "exited" || - strings.ToLower(containerDetails.State.Status) == "dead" { + } else if containerDetails.State.Status == devcconfig.ContainerStatusExited || + containerDetails.State.Status == devcconfig.ContainerStatusDead { logs, _ := cmd.logsContainer(ctx, containerID) return false, fmt.Errorf( "container failed (status: %s):\n %s", @@ -919,7 +920,7 @@ func (cmd *StartCmd) resolveRunningContainer( switch { case err != nil: return "", err - case onlyRunning && strings.ToLower(containerState.State.Status) != "running": + case onlyRunning && containerState.State.Status != devcconfig.ContainerStatusRunning: err = cmd.removeContainer(ctx, containerID) if err != nil { return "", err @@ -2144,8 +2145,8 @@ type ContainerDetailsConfig struct { } type ContainerDetailsState struct { - Status string `json:"Status,omitempty"` - StartedAt string `json:"StartedAt,omitempty"` + Status devcconfig.ContainerStatus `json:"Status,omitempty"` + StartedAt string `json:"StartedAt,omitempty"` } func WrapCommandError(stdout []byte, err error) error { diff --git a/cmd/workspace/exec.go b/cmd/workspace/exec.go index a26084eb3..383b61d8e 100644 --- a/cmd/workspace/exec.go +++ b/cmd/workspace/exec.go @@ -245,7 +245,7 @@ func (cmd *ExecCmd) runWithContainerID(ctx context.Context, args []string) error } containerDetails := &details[0] - if !strings.EqualFold(containerDetails.State.Status, workspace2.ContainerStatusRunning) { + if containerDetails.State.Status != devcconfig.ContainerStatusRunning { return fmt.Errorf( "container %s is not running (status: %s)", cmd.ContainerID, diff --git a/pkg/agent/delivery/local_docker.go b/pkg/agent/delivery/local_docker.go index fd29a1388..8b23e403c 100644 --- a/pkg/agent/delivery/local_docker.go +++ b/pkg/agent/delivery/local_docker.go @@ -66,7 +66,7 @@ func (d *LocalDockerDelivery) DeliverPreStart(ctx context.Context, opts PreStart if opts.RunOptions.Env == nil { opts.RunOptions.Env = make(map[string]string) } - opts.RunOptions.Env["DEVSY_AGENT_PATH"] = volumeMountPath + "/" + binaryName() + opts.RunOptions.Env[pkgconfig.EnvAgentPath] = volumeMountPath + "/" + binaryName() return nil } diff --git a/pkg/apple/helper.go b/pkg/apple/helper.go index 55cebd0f6..01e434a4e 100644 --- a/pkg/apple/helper.go +++ b/pkg/apple/helper.go @@ -167,10 +167,10 @@ func (h *AppleHelper) WaitContainerRunning(ctx context.Context, id string) error if len(details) == 0 { return false, nil } - switch strings.ToLower(details[0].State.Status) { - case stateRunning: + switch details[0].State.Status { + case config.ContainerStatusRunning: return true, nil - case stateExited: + case config.ContainerStatusExited: return false, fmt.Errorf("container %s exited before reaching running state", id) default: return false, nil @@ -249,7 +249,7 @@ func (h *AppleHelper) FindContainerByID( return nil, err } for i := range details { - if strings.ToLower(details[i].State.Status) != "removing" { + if details[i].State.Status != config.ContainerStatusRemoving { return &details[i], nil } } @@ -357,7 +357,7 @@ func (h *AppleHelper) SystemRunning(ctx context.Context) bool { if err != nil { return false } - return strings.Contains(strings.ToLower(string(out)), stateRunning) + return strings.Contains(strings.ToLower(string(out)), string(config.ContainerStatusRunning)) } func (h *AppleHelper) buildCmd(ctx context.Context, args ...string) *exec.Cmd { diff --git a/pkg/apple/integration_test.go b/pkg/apple/integration_test.go index c8c6ba77b..e8795f075 100644 --- a/pkg/apple/integration_test.go +++ b/pkg/apple/integration_test.go @@ -83,8 +83,12 @@ func (p *probe) findRunning(label string) *config.ContainerDetails { if found == nil { p.t.Fatal("FindDevContainer returned nil for a running labelled container") } - if found.State.Status != stateRunning { - p.t.Errorf("found.State.Status = %q, want %q", found.State.Status, stateRunning) + if found.State.Status != config.ContainerStatusRunning { + p.t.Errorf( + "found.State.Status = %q, want %q", + found.State.Status, + config.ContainerStatusRunning, + ) } if found.Config.Labels["devsy.e2e"] != "1" { p.t.Errorf("label devsy.e2e = %q, want 1", found.Config.Labels["devsy.e2e"]) diff --git a/pkg/apple/types.go b/pkg/apple/types.go index 367eeff5f..3d0e0d6d2 100644 --- a/pkg/apple/types.go +++ b/pkg/apple/types.go @@ -9,8 +9,6 @@ import ( ) const ( - stateRunning = "running" - stateExited = "exited" mountTypeBind = "bind" archUnknown = "unknown" // placeholder arch in Apple's multi-arch image index ) @@ -130,12 +128,13 @@ func (c containerInspect) toContainerDetails() config.ContainerDetails { } } -func normalizeState(state string) string { - s := strings.ToLower(strings.TrimSpace(state)) +// normalizeState maps the runner's status vocabulary onto the shared Docker +// vocabulary: "stopped" means the container ran and exited, matching what +// terminal-state checks expect. +func normalizeState(state string) config.ContainerStatus { + s := config.ToContainerStatus(strings.TrimSpace(state)) if s == "stopped" { - // Docker uses "exited" for a container that ran and stopped; the - // runner's terminal-state checks key off that vocabulary. - return stateExited + return config.ContainerStatusExited } return s } diff --git a/pkg/apple/types_test.go b/pkg/apple/types_test.go index 57ac6ed99..8f273d9b2 100644 --- a/pkg/apple/types_test.go +++ b/pkg/apple/types_test.go @@ -3,6 +3,8 @@ package apple import ( "encoding/json" "testing" + + "github.com/devsy-org/devsy/pkg/devcontainer/config" ) // containerInspectFixture is verbatim output from `container inspect` on @@ -46,7 +48,7 @@ func TestContainerInspectMapping(t *testing.T) { name, got, want string }{ {"ID", got.ID, "devsy-probe"}, - {"State.Status", got.State.Status, stateRunning}, + {"State.Status", string(got.State.Status), string(config.ContainerStatusRunning)}, {"StartedAt", got.State.StartedAt, "2026-07-25T01:57:25Z"}, {"WorkingDir", got.Config.WorkingDir, "/"}, {"User", got.Config.User, "0"}, @@ -69,11 +71,14 @@ func TestContainerInspectMapping(t *testing.T) { } func TestNormalizeState(t *testing.T) { - cases := []struct{ in, want string }{ - {"running", stateRunning}, - {"Running", stateRunning}, - {"stopped", stateExited}, - {"Stopped", stateExited}, + cases := []struct { + in string + want config.ContainerStatus + }{ + {"running", config.ContainerStatusRunning}, + {"Running", config.ContainerStatusRunning}, + {"stopped", config.ContainerStatusExited}, + {"Stopped", config.ContainerStatusExited}, } for _, c := range cases { if got := normalizeState(c.in); got != c.want { diff --git a/pkg/compose/helper.go b/pkg/compose/helper.go index aed737561..ff97305ec 100644 --- a/pkg/compose/helper.go +++ b/pkg/compose/helper.go @@ -217,9 +217,9 @@ func (h *ComposeHelper) FindDevContainer( return nil, err } - for _, details := range containerDetails { - if details.State.Status != "removing" { - return &details, nil + for i := range containerDetails { + if containerDetails[i].State.Status != config.ContainerStatusRemoving { + return &containerDetails[i], nil } } diff --git a/pkg/config/env.go b/pkg/config/env.go index 48ab7ef75..b42a69d3d 100644 --- a/pkg/config/env.go +++ b/pkg/config/env.go @@ -60,10 +60,15 @@ const ( // EnvAgentPreferDownload forces agent binary download even if a local copy exists. EnvAgentPreferDownload = "DEVSY_AGENT_PREFER_DOWNLOAD" - // EnvOS is set to the host operating system (runtime.GOOS). + // EnvAgentPath is the path to the agent binary inside the workspace + // container, set by agent delivery so the container entrypoint can locate + // it (defaults to /usr/local/bin/devsy). + EnvAgentPath = "DEVSY_AGENT_PATH" + + // EnvOS is set to the host operating system. EnvOS = "DEVSY_OS" - // EnvArch is set to the host architecture (runtime.GOARCH). + // EnvArch is set to the host architecture. EnvArch = "DEVSY_ARCH" // EnvLogLevel is set to the current log level. @@ -121,8 +126,6 @@ const ( // EnvProviderPrefix is the prefix for provider-specific option env vars (append provider name + "_"). EnvProviderPrefix = EnvPrefix + "PROVIDER_" - // --- Provider-scoped env vars (set when running provider commands) ---. - // EnvProviderWorkspaceID is the workspace identifier passed to providers. EnvProviderWorkspaceID = "WORKSPACE_ID" diff --git a/pkg/devcontainer/compose.go b/pkg/devcontainer/compose.go index f19179803..700f49e0a 100644 --- a/pkg/devcontainer/compose.go +++ b/pkg/devcontainer/compose.go @@ -24,7 +24,6 @@ const ( FeaturesBuildOverrideFilePrefix = "docker-compose.devcontainer.build" FeaturesStartOverrideFilePrefix = "docker-compose.devcontainer.containerFeatures" - containerStatusRunning = "running" composeProjectNameFlag = "--project-name" ) @@ -328,7 +327,7 @@ func (r *runner) ensureComposeContainer( } // container already exists and is running, nothing to do - if containerDetails != nil && containerDetails.State.Status == containerStatusRunning && + if containerDetails != nil && containerDetails.State.Status == config.ContainerStatusRunning && !options.Recreate { return containerDetails, nil } diff --git a/pkg/devcontainer/compose_test.go b/pkg/devcontainer/compose_test.go index 516096cad..41009c0d6 100644 --- a/pkg/devcontainer/compose_test.go +++ b/pkg/devcontainer/compose_test.go @@ -781,7 +781,7 @@ func TestBuildOverrideEntrypointAppendsUserEntrypoint(t *testing.T) { func TestBuildOverrideEntrypointKeepsDefaultEntrypointReachable(t *testing.T) { script := buildOverrideEntrypoint(&config.MergedDevContainerConfig{}, nil) body := script[2] - if !strings.Contains(body, "devsy internal agent container daemon") { + if !strings.Contains(body, "internal agent container daemon") { t.Errorf("expected default entrypoint invocation in script, got %q", body) } } diff --git a/pkg/devcontainer/config/container_details.go b/pkg/devcontainer/config/container_details.go index 8f018a8d1..a032236ad 100644 --- a/pkg/devcontainer/config/container_details.go +++ b/pkg/devcontainer/config/container_details.go @@ -1,5 +1,10 @@ package config +import ( + "encoding/json" + "strings" +) + type ImageDetails struct { ID string Config ImageDetailsConfig @@ -43,8 +48,41 @@ type ContainerDetailsConfig struct { } type ContainerDetailsState struct { - Status string `json:"Status,omitempty"` - StartedAt string `json:"StartedAt,omitempty"` - ExitCode int `json:"ExitCode,omitempty"` - Error string `json:"Error,omitempty"` + Status ContainerStatus `json:"Status,omitempty"` + StartedAt string `json:"StartedAt,omitempty"` + ExitCode int `json:"ExitCode,omitempty"` + Error string `json:"Error,omitempty"` +} + +// UnmarshalJSON decodes inspect output and normalizes Status, so the field is +// always canonical regardless of the runtime's casing. +func (s *ContainerDetailsState) UnmarshalJSON(data []byte) error { + type alias ContainerDetailsState + var raw alias + if err := json.Unmarshal(data, &raw); err != nil { + return err + } + raw.Status = ToContainerStatus(string(raw.Status)) + *s = ContainerDetailsState(raw) + return nil +} + +// ContainerStatus is a normalized container state string (`State.Status` from +// `docker inspect` and friends), comparable against the ContainerStatus* +// constants. +type ContainerStatus string + +const ( + ContainerStatusRunning ContainerStatus = "running" + ContainerStatusExited ContainerStatus = "exited" + ContainerStatusCreated ContainerStatus = "created" + ContainerStatusPaused ContainerStatus = "paused" + ContainerStatusRestarting ContainerStatus = "restarting" + ContainerStatusDead ContainerStatus = "dead" + ContainerStatusRemoving ContainerStatus = "removing" +) + +// ToContainerStatus normalizes a raw status string for comparison. +func ToContainerStatus(s string) ContainerStatus { + return ContainerStatus(strings.ToLower(s)) } diff --git a/pkg/devcontainer/delete.go b/pkg/devcontainer/delete.go index 8c5ab9993..a5954579e 100644 --- a/pkg/devcontainer/delete.go +++ b/pkg/devcontainer/delete.go @@ -3,7 +3,6 @@ package devcontainer import ( "context" "fmt" - "strings" "github.com/devsy-org/devsy/pkg/devcontainer/config" "github.com/devsy-org/devsy/pkg/log" @@ -32,7 +31,7 @@ func (r *runner) Delete(ctx context.Context, options DeleteOptions) error { func (r *runner) stopAndDeleteContainer( ctx context.Context, containerDetails *config.ContainerDetails, ) error { - if strings.ToLower(containerDetails.State.Status) == "running" { + if containerDetails.State.Status == config.ContainerStatusRunning { if err := r.driver.StopDevContainer(ctx, r.id); err != nil { return err } @@ -65,7 +64,7 @@ func (r *runner) Stop(ctx context.Context) error { return nil } - if strings.ToLower(containerDetails.State.Status) != "running" { + if containerDetails.State.Status != config.ContainerStatusRunning { return nil } diff --git a/pkg/devcontainer/single.go b/pkg/devcontainer/single.go index 26936b825..b4a79e50f 100644 --- a/pkg/devcontainer/single.go +++ b/pkg/devcontainer/single.go @@ -44,10 +44,17 @@ func joinShellStatements(statements ...string) string { // DefaultEntrypoint waits for the devsy agent binary to become available // before handing off to the container daemon. var DefaultEntrypoint = joinShellStatements( - `while ! command -v /usr/local/bin/devsy >/dev/null 2>&1; do echo "waiting for devsy agent to be available"`, + fmt.Sprintf( + `while ! command -v "${%s:-/usr/local/bin/devsy}" >/dev/null 2>&1`, + pkgconfig.EnvAgentPath, + ), + `do echo "waiting for devsy agent to be available"`, "sleep 1", "done", - "exec /usr/local/bin/devsy internal agent container daemon", + fmt.Sprintf( + `exec "${%s:-/usr/local/bin/devsy}" internal agent container daemon`, + pkgconfig.EnvAgentPath, + ), ) // resolvedContainer holds the outputs that every code path through @@ -209,7 +216,7 @@ func (r *runner) ensureRunning( ctx context.Context, containerDetails *config.ContainerDetails, ) (*config.ContainerDetails, error) { - if strings.ToLower(containerDetails.State.Status) == containerStatusRunning { + if containerDetails.State.Status == config.ContainerStatusRunning { return containerDetails, nil } diff --git a/pkg/devcontainer/single_test.go b/pkg/devcontainer/single_test.go index ec425b617..079fe67fe 100644 --- a/pkg/devcontainer/single_test.go +++ b/pkg/devcontainer/single_test.go @@ -209,9 +209,12 @@ func TestDefaultEntrypointSingleLine(t *testing.T) { if strings.Contains(DefaultEntrypoint, "\n") { t.Fatalf("DefaultEntrypoint must be single-line, got %q", DefaultEntrypoint) } - if !strings.Contains(DefaultEntrypoint, "devsy internal agent container daemon") { + if !strings.Contains(DefaultEntrypoint, "internal agent container daemon") { t.Errorf("DefaultEntrypoint must invoke the agent daemon, got %q", DefaultEntrypoint) } + if !strings.Contains(DefaultEntrypoint, `"${DEVSY_AGENT_PATH:-/usr/local/bin/devsy}"`) { + t.Errorf("DefaultEntrypoint must honor DEVSY_AGENT_PATH, got %q", DefaultEntrypoint) + } } func TestGetStartScriptSingleLine(t *testing.T) { @@ -227,7 +230,7 @@ func TestGetStartScriptSingleLine(t *testing.T) { if !strings.Contains(got, `exec "$@"`) { t.Fatalf("GetStartScript() must keep the shell exec passthrough, got %q", got) } - if !strings.Contains(got, "devsy internal agent container daemon") { + if !strings.Contains(got, "internal agent container daemon") { t.Fatalf("GetStartScript() must invoke the agent, got %q", got) } } @@ -245,7 +248,7 @@ func TestGetStartScriptPreservesStatementOrder(t *testing.T) { `exec "$@"`, "first-entrypoint", "second-entrypoint", - "devsy internal agent container daemon", + "internal agent container daemon", } lastIdx := -1 for _, want := range wantOrder { diff --git a/pkg/docker/boot_state_test.go b/pkg/docker/boot_state_test.go index 587f6377d..cc965e81f 100644 --- a/pkg/docker/boot_state_test.go +++ b/pkg/docker/boot_state_test.go @@ -3,48 +3,70 @@ package docker import ( "testing" + "github.com/devsy-org/devsy/pkg/devcontainer/config" "github.com/stretchr/testify/assert" ) -const ( - statusDead = "dead" - statusRemoving = "removing" - statusExited = "exited" - statusCreated = "created" - statusPaused = "paused" - statusRestarting = "restarting" -) - func TestFailedBootSentinel(t *testing.T) { tests := []struct { name string - status string + status config.ContainerStatus graceElapsed bool wantSentinel error wantNil bool }{ - {"dead is terminal regardless of grace", statusDead, false, ErrContainerTerminal, false}, - {"dead is terminal even after grace", statusDead, true, ErrContainerTerminal, false}, + { + "dead is terminal regardless of grace", + config.ContainerStatusDead, + false, + ErrContainerTerminal, + false, + }, + { + "dead is terminal even after grace", + config.ContainerStatusDead, + true, + ErrContainerTerminal, + false, + }, { "removing is terminal regardless of grace", - statusRemoving, + config.ContainerStatusRemoving, false, ErrContainerTerminal, false, }, { "removing is terminal even after grace", - statusRemoving, + config.ContainerStatusRemoving, true, ErrContainerTerminal, false, }, - {"exited before grace is still booting", statusExited, false, nil, true}, - {"exited after grace failed", statusExited, true, ErrContainerExited, false}, - {"created before grace is still booting", statusCreated, false, nil, true}, - {"created after grace failed", statusCreated, true, ErrContainerExited, false}, - {"paused is not a terminal boot state", statusPaused, true, nil, true}, - {"restarting is not a terminal boot state", statusRestarting, false, nil, true}, + {"exited before grace is still booting", config.ContainerStatusExited, false, nil, true}, + { + "exited after grace failed", + config.ContainerStatusExited, + true, + ErrContainerExited, + false, + }, + {"created before grace is still booting", config.ContainerStatusCreated, false, nil, true}, + { + "created after grace failed", + config.ContainerStatusCreated, + true, + ErrContainerExited, + false, + }, + {"paused is not a terminal boot state", config.ContainerStatusPaused, true, nil, true}, + { + "restarting is not a terminal boot state", + config.ContainerStatusRestarting, + false, + nil, + true, + }, {"empty status is not a terminal boot state", "", true, nil, true}, } @@ -69,8 +91,8 @@ func TestFailedBootSentinel(t *testing.T) { } func TestFailedBootSentinel_TerminalNotConfusedWithExited(t *testing.T) { - terminal := failedBootSentinel(statusRemoving, false) - exited := failedBootSentinel(statusExited, true) + terminal := failedBootSentinel(config.ContainerStatusRemoving, false) + exited := failedBootSentinel(config.ContainerStatusExited, true) assert.ErrorIs(t, terminal, ErrContainerTerminal) assert.NotErrorIs(t, terminal, ErrContainerExited, diff --git a/pkg/docker/helper.go b/pkg/docker/helper.go index f9f190c19..6d0cfe7ae 100644 --- a/pkg/docker/helper.go +++ b/pkg/docker/helper.go @@ -32,12 +32,19 @@ const ( DockerBuilderBuildKit ) -const ( +var ( containerRunningPollInterval = 500 * time.Millisecond containerRunningTimeout = 30 * time.Second containerExitGrace = 2 * time.Second + + // podmanMachineStartTimeout is the maximum time to wait for a Podman machine to start. + podmanMachineStartTimeout = 90 * time.Second + + // pingTimeout is the maximum time to wait for a ping to the runtime daemon. + pingTimeout = 30 * time.Second ) +// Sentinels for container lifecycle failures; match with errors.Is. var ( ErrContainerTerminal = errors.New("container in terminal state") ErrContainerExited = errors.New("container exited after start") @@ -158,11 +165,6 @@ func (r *DockerHelper) ClientVersion(ctx context.Context) string { return strings.TrimSpace(string(out)) } -// podmanMachineStartTimeout bounds a Podman machine boot, which spins up a VM. -var podmanMachineStartTimeout = 90 * time.Second - -var pingTimeout = 30 * time.Second - func runCmdCombined(ctx context.Context, cmd *exec.Cmd) error { var out bytes.Buffer cmd.Stdout = &out @@ -279,11 +281,10 @@ func (r *DockerHelper) FindContainerByID( return nil, err } - // find matching container - for _, details := range containerDetails { - if strings.ToLower(details.State.Status) != "removing" { - details.State.Status = strings.ToLower(details.State.Status) - return &details, nil + // find matching container, skipping containers already being removed + for i := range containerDetails { + if containerDetails[i].State.Status != config.ContainerStatusRemoving { + return &containerDetails[i], nil } } @@ -439,6 +440,16 @@ func (r *DockerHelper) StartContainer(ctx context.Context, containerId string) e return nil } +// UnpauseContainer unpauses a paused container. +func (r *DockerHelper) UnpauseContainer(ctx context.Context, containerId string) error { + out, err := r.buildCmd(ctx, "unpause", containerId).CombinedOutput() + if err != nil { + return fmt.Errorf("failed to unpause container: %s: %w", string(out), err) + } + + return nil +} + // WaitContainerRunning waits for the given container to be running, returning an error if // it is in a terminal state or does not become running within a timeout. func (r *DockerHelper) WaitContainerRunning(ctx context.Context, containerID string) error { @@ -450,7 +461,7 @@ func (r *DockerHelper) WaitContainerRunning(ctx context.Context, containerID str details, err := r.InspectContainers(ctx, []string{containerID}) if err != nil { lastErr = err - log.Debugf("WaitContainerRunning: inspect error (will retry): %v", err) + log.Debugf("inspecting container %s: %v", containerID, err) return false, nil } lastErr = nil @@ -458,7 +469,12 @@ func (r *DockerHelper) WaitContainerRunning(ctx context.Context, containerID str }, ) if pollErr != nil && lastErr != nil { - return fmt.Errorf("%w (last inspect error: %v)", pollErr, lastErr) + return fmt.Errorf( + "waiting for container %s to be running: %w (last inspect error: %v)", + containerID, + pollErr, + lastErr, + ) } return pollErr } @@ -671,7 +687,7 @@ func (r *DockerHelper) containerStateError( "%w: container %s is %q (%s)", sentinel, containerID, - strings.ToLower(state.Status), + state.Status, detail, ) } @@ -706,8 +722,8 @@ func (r *DockerHelper) evaluateContainerState( ) } state := details[0].State - status := strings.ToLower(state.Status) - if status == "running" { + status := state.Status + if status == config.ContainerStatusRunning { return true, nil } if sentinel := failedBootSentinel(status, elapsed > containerExitGrace); sentinel != nil { @@ -719,11 +735,11 @@ func (r *DockerHelper) evaluateContainerState( // failedBootSentinel returns an error if the container is in a terminal state or has exited // after the grace period, or nil if it is still booting. -func failedBootSentinel(status string, graceElapsed bool) error { +func failedBootSentinel(status config.ContainerStatus, graceElapsed bool) error { switch status { - case "dead", "removing": + case config.ContainerStatusDead, config.ContainerStatusRemoving: return ErrContainerTerminal - case "exited", "created": + case config.ContainerStatusExited, config.ContainerStatusCreated: if graceElapsed { return ErrContainerExited } diff --git a/pkg/docker/helper_test.go b/pkg/docker/helper_test.go index 940622b33..f4a41c16f 100644 --- a/pkg/docker/helper_test.go +++ b/pkg/docker/helper_test.go @@ -516,3 +516,23 @@ sleep 5 require.Error(t, err) assert.ErrorIs(t, err, context.DeadlineExceeded) } + +func TestWaitContainerRunning_PreservesDeadlineExceededWithInspectError(t *testing.T) { + original := containerRunningTimeout + containerRunningTimeout = 50 * time.Millisecond + t.Cleanup(func() { containerRunningTimeout = original }) + + bin := writeScript(t, t.TempDir(), "docker-fake", `#!/bin/sh +case "$1" in + inspect) echo "Cannot connect to the Docker daemon" >&2; exit 1 ;; +esac +`) + h := &DockerHelper{DockerCommand: bin} + err := h.WaitContainerRunning(context.Background(), "c1") + + require.Error(t, err) + assert.ErrorIs(t, err, context.DeadlineExceeded, + "poll timeout must remain in the error chain even when inspect also fails") + assert.Contains(t, err.Error(), "Cannot connect to the Docker daemon", + "inspect error should be included for diagnostics") +} diff --git a/pkg/driver/apple/driver.go b/pkg/driver/apple/driver.go index 426b7e1a2..caa07a7f4 100644 --- a/pkg/driver/apple/driver.go +++ b/pkg/driver/apple/driver.go @@ -22,7 +22,6 @@ import ( const ( appleExec = "exec" defaultCommand = "container" - statusRunning = "running" ) type appleDriver struct { diff --git a/pkg/driver/apple/lifecycle.go b/pkg/driver/apple/lifecycle.go index 216f764da..459bcd590 100644 --- a/pkg/driver/apple/lifecycle.go +++ b/pkg/driver/apple/lifecycle.go @@ -4,7 +4,6 @@ import ( "context" "fmt" "io" - "strings" "github.com/devsy-org/devsy/pkg/apple" "github.com/devsy-org/devsy/pkg/devcontainer/config" @@ -44,7 +43,7 @@ func (d *appleDriver) ensureContainerRunning( ctx context.Context, container *config.ContainerDetails, ) error { - if strings.ToLower(container.State.Status) == statusRunning { + if container.State.Status == config.ContainerStatusRunning { return nil } if err := d.Apple.StartContainer(ctx, container.ID); err != nil { @@ -83,7 +82,7 @@ func (d *appleDriver) DeleteDevContainer(ctx context.Context, workspaceID string // Apple's `delete` requires a stopped container. `container stop` is synchronous; // a stop failure is logged but delete is still attempted (it fails loudly if the // container is genuinely still running). - if strings.ToLower(container.State.Status) == statusRunning { + if container.State.Status == config.ContainerStatusRunning { if err := d.Apple.Stop(ctx, container.ID); err != nil { log.Warnf("stop before delete failed for %s: %v", container.ID, err) } diff --git a/pkg/driver/apple/lifecycle_test.go b/pkg/driver/apple/lifecycle_test.go index d70e6614f..27d25ac15 100644 --- a/pkg/driver/apple/lifecycle_test.go +++ b/pkg/driver/apple/lifecycle_test.go @@ -103,7 +103,7 @@ func (m *mockClient) GetContainerLogs(context.Context, string, io.Writer, io.Wri func running(id string) *config.ContainerDetails { return &config.ContainerDetails{ ID: id, - State: config.ContainerDetailsState{Status: statusRunning}, + State: config.ContainerDetailsState{Status: config.ContainerStatusRunning}, } } diff --git a/pkg/driver/docker/build.go b/pkg/driver/docker/build.go index f68703bd4..34366a2b6 100644 --- a/pkg/driver/docker/build.go +++ b/pkg/driver/docker/build.go @@ -415,13 +415,14 @@ func (d *dockerDriver) executeBuild( return nil } +// createBuildInfo constructs the BuildInfo after a successful build. When pushing, +// the image may not be available locally, so ImageDetails may be nil. func (d *dockerDriver) createBuildInfo( ctx context.Context, imageName string, req driver.BuildRequest, buildOptions *build.BuildOptions, ) (*config.BuildInfo, error) { - // When pushing, image may not be available locally var imageDetails *config.ImageDetails if !buildOptions.Push { var err error diff --git a/pkg/driver/docker/lifecycle.go b/pkg/driver/docker/lifecycle.go index 0713733b1..069c945de 100644 --- a/pkg/driver/docker/lifecycle.go +++ b/pkg/driver/docker/lifecycle.go @@ -15,17 +15,13 @@ import ( "k8s.io/apimachinery/pkg/util/wait" ) -const containerRestartAttempts = 3 +const ( + containerRestartAttempts = 3 -const containerStatusRunning = "running" - -// snapshotImageLabel marks a committed image as a devsy workspace snapshot, -// so it's identifiable via `docker inspect`/`docker images --filter` by -// anyone who pulls or lists it outside `devsy snapshot` tooling — the -// snapshot manifest (pkg/snapshot) already carries richer sh.devsy.snapshot.* -// metadata, but that lives in a separate OCI artifact a raw image pull won't -// see. -const snapshotImageLabel = "sh.devsy.snapshot=true" + // snapshotImageLabel marks a committed image as a devsy workspace snapshot, + // so it's identifiable via `docker inspect`/`docker images --filter`. + snapshotImageLabel = "sh.devsy.snapshot=true" +) func (d *dockerDriver) CommandDevContainer( ctx context.Context, @@ -58,75 +54,123 @@ func (d *dockerDriver) CommandDevContainer( return nil } -// ensureContainerRunning checks that the given container is running, and if -// not, attempts to start it and wait for it to be running. If the container is -// in a terminal state (dead or removing), it returns an error. +// ensureContainerRunning checks the container's state and starts it if necessary. func (d *dockerDriver) ensureContainerRunning( ctx context.Context, container *config.ContainerDetails, ) error { - status := strings.ToLower(container.State.Status) - if status == "dead" || status == "removing" { + status := container.State.Status + switch status { + case config.ContainerStatusRunning: + return nil + case config.ContainerStatusDead, config.ContainerStatusRemoving: return fmt.Errorf( "%w: container %s is %q", docker.ErrContainerTerminal, container.ID, status, ) + case config.ContainerStatusPaused: + return d.unpauseAndWait(ctx, container) + case config.ContainerStatusRestarting: + return d.waitForRestart(ctx, container) + case config.ContainerStatusExited, config.ContainerStatusCreated: + return d.restartAndWait(ctx, container, status) + default: + return fmt.Errorf( + "%w: container %s is in unknown state %q", + docker.ErrContainerTerminal, + container.ID, + status, + ) } - if status == containerStatusRunning { - return nil - } +} +// restartAndWait starts the container and waits for it to be running, +// retrying up to containerRestartAttempts times. It aborts immediately when +// the container enters a terminal state. +func (d *dockerDriver) restartAndWait( + ctx context.Context, + container *config.ContainerDetails, + status config.ContainerStatus, +) error { var lastErr error for attempt := 1; attempt <= containerRestartAttempts; attempt++ { if err := ctx.Err(); err != nil { return err } log.Infof( - "container %s is not running (status=%s), restarting (attempt %d/%d)", + "restarting container %s (status=%s, attempt=%d/%d)", container.ID, status, attempt, containerRestartAttempts, ) - err := d.restartAndWait(ctx, container.ID) - if err == nil { - log.Infof("container %s is now running", container.ID) + if err := d.Docker.StartContainer(ctx, container.ID); err != nil { + lastErr = fmt.Errorf("start container: %w", err) + } else if err := d.Docker.WaitContainerRunning(ctx, container.ID); err != nil { + lastErr = fmt.Errorf("wait for container to be running: %w", err) + } else { + log.Infof("container %s is running", container.ID) return nil } - if errors.Is(err, docker.ErrContainerTerminal) { - return err + if errors.Is(lastErr, docker.ErrContainerTerminal) || + errors.Is(lastErr, context.Canceled) || + errors.Is(lastErr, context.DeadlineExceeded) { + return lastErr } - lastErr = err - log.Debugf("container %s restart attempt %d failed: %v", container.ID, attempt, err) + log.Debugf("container %s restart attempt %d failed: %v", container.ID, attempt, lastErr) } return fmt.Errorf( - "%w: container %s did not stay running after %d restart attempts: %v", + "%w: container %s did not stay running after %d attempts: %w", docker.ErrContainerTerminal, container.ID, containerRestartAttempts, lastErr, ) } -func (d *dockerDriver) restartAndWait(ctx context.Context, containerID string) error { - if err := d.Docker.StartContainer(ctx, containerID); err != nil { - return fmt.Errorf("restart container: %w", err) +// unpauseAndWait unpauses a paused container and waits for it to be running. +func (d *dockerDriver) unpauseAndWait( + ctx context.Context, + container *config.ContainerDetails, +) error { + log.Infof("unpausing container %s", container.ID) + if err := d.Docker.UnpauseContainer(ctx, container.ID); err != nil { + return fmt.Errorf("unpause container: %w", err) } - if err := d.Docker.WaitContainerRunning(ctx, containerID); err != nil { + if err := d.Docker.WaitContainerRunning(ctx, container.ID); err != nil { return fmt.Errorf("wait for container to be running: %w", err) } + log.Infof("container %s is running", container.ID) return nil } +// waitForRestart lets the daemon finish an in-flight restart before acting: +// DockerHelper.StartContainer rejects containers still in the "restarting" +// state. Once the container settles, either it is already running or it has +// stopped again (ErrContainerExited) and needs an explicit start. +func (d *dockerDriver) waitForRestart( + ctx context.Context, + container *config.ContainerDetails, +) error { + log.Infof("container %s is restarting, waiting for a stable state", container.ID) + err := d.Docker.WaitContainerRunning(ctx, container.ID) + switch { + case err == nil: + log.Infof("container %s is running", container.ID) + return nil + case errors.Is(err, docker.ErrContainerExited): + return d.restartAndWait(ctx, container, config.ContainerStatusExited) + default: + return err + } +} + func (d *dockerDriver) PushDevContainer(ctx context.Context, image string) error { - // push image writer := log.Writer(log.LevelInfo) defer func() { _ = writer.Close() }() - // build args args := []string{ "push", image, } - // run command log.Debugf( "running docker push command: command=%s, args=%s", d.Docker.DockerCommand, @@ -141,18 +185,15 @@ func (d *dockerDriver) PushDevContainer(ctx context.Context, image string) error } func (d *dockerDriver) TagDevContainer(ctx context.Context, image, tag string) error { - // Tag image writer := log.Writer(log.LevelInfo) defer func() { _ = writer.Close() }() - // build args args := []string{ "tag", image, tag, } - // run command log.Debugf( "running docker tag command: command=%s, args=%s", d.Docker.DockerCommand, @@ -201,7 +242,7 @@ func (d *dockerDriver) DeleteDevContainer(ctx context.Context, workspaceId strin return nil } - if strings.ToLower(container.State.Status) == containerStatusRunning { + if container.State.Status == config.ContainerStatusRunning { if err := d.Docker.Stop(ctx, container.ID); err != nil { log.Warnf("stop before delete failed for %s: %v", container.ID, err) } diff --git a/pkg/driver/docker/lifecycle_test.go b/pkg/driver/docker/lifecycle_test.go index ecacf8599..c8aa4ed60 100644 --- a/pkg/driver/docker/lifecycle_test.go +++ b/pkg/driver/docker/lifecycle_test.go @@ -203,7 +203,7 @@ func TestEnsureContainerRunning_AlreadyRunning(t *testing.T) { d := &dockerDriver{Docker: &docker.DockerHelper{DockerCommand: testDockerCmd}} container := &config.ContainerDetails{ ID: "c1", - State: config.ContainerDetailsState{Status: containerStatusRunning}, + State: config.ContainerDetailsState{Status: config.ContainerStatusRunning}, } require.NoError(t, d.ensureContainerRunning(context.Background(), container)) @@ -358,3 +358,98 @@ esac assert.Contains(t, string(logged), "stop\n") assert.Contains(t, string(logged), "rm\n") } + +func TestEnsureContainerRunning_PausedUnpausesNotRestarts(t *testing.T) { + dir := t.TempDir() + calls := filepath.Join(dir, "calls") + script := `#!/bin/sh +echo "$1" >> "` + calls + `" +case "$1" in + inspect) + echo '[{"ID":"c1","State":{"Status":"running"}}]' + ;; + unpause) + ;; + start) + echo "start should not be called on paused container" >&2 + exit 1 + ;; +esac +` + bin := filepath.Join(dir, "docker-fake") + require.NoError(t, os.WriteFile(bin, []byte(script), 0o755)) //nolint:gosec + + d := &dockerDriver{Docker: &docker.DockerHelper{DockerCommand: bin}} + container := &config.ContainerDetails{ + ID: "c1", + State: config.ContainerDetailsState{Status: "paused"}, + } + + require.NoError(t, d.ensureContainerRunning(context.Background(), container)) + + logged, readErr := os.ReadFile(calls) //nolint:gosec // test-controlled path + require.NoError(t, readErr) + assert.Contains(t, string(logged), "unpause\n") + assert.NotContains( + t, + string(logged), + "start\n", + "paused containers should be unpaused, not started", + ) +} + +func TestEnsureContainerRunning_RestartingWaitsNotStarts(t *testing.T) { + dir := t.TempDir() + calls := filepath.Join(dir, "calls") + inspects := filepath.Join(dir, "inspects") + script := `#!/bin/sh +echo "$1" >> "` + calls + `" +case "$1" in + inspect) + n=$(cat "` + inspects + `" 2>/dev/null || echo 0) + n=$((n + 1)) + echo "$n" > "` + inspects + `" + if [ "$n" -ge 2 ]; then + echo '[{"ID":"c1","State":{"Status":"running"}}]' + else + echo '[{"ID":"c1","State":{"Status":"restarting"}}]' + fi + ;; + start) + echo "start should not be called on a restarting container" >&2 + exit 1 + ;; +esac +` + bin := filepath.Join(dir, "docker-fake") + require.NoError(t, os.WriteFile(bin, []byte(script), 0o755)) //nolint:gosec + + d := &dockerDriver{Docker: &docker.DockerHelper{DockerCommand: bin}} + container := &config.ContainerDetails{ + ID: "c1", + State: config.ContainerDetailsState{Status: config.ContainerStatusRestarting}, + } + + require.NoError(t, d.ensureContainerRunning(context.Background(), container)) + + logged, readErr := os.ReadFile(calls) //nolint:gosec // test-controlled path + require.NoError(t, readErr) + assert.NotContains( + t, + string(logged), + "start\n", + "restarting containers should be waited out, not started", + ) +} + +func TestEnsureContainerRunning_UnknownStateIsTerminal(t *testing.T) { + d := &dockerDriver{Docker: &docker.DockerHelper{DockerCommand: testDockerCmd}} + container := &config.ContainerDetails{ + ID: "c1", + State: config.ContainerDetailsState{Status: "unknown"}, + } + + err := d.ensureContainerRunning(context.Background(), container) + require.Error(t, err) + assert.ErrorIs(t, err, docker.ErrContainerTerminal) +} diff --git a/pkg/driver/kubernetes/find.go b/pkg/driver/kubernetes/find.go index 23aeea2e2..b91b36789 100644 --- a/pkg/driver/kubernetes/find.go +++ b/pkg/driver/kubernetes/find.go @@ -36,9 +36,9 @@ func (k *KubernetesDriver) FindDevContainer( } // determine status - status := "exited" + status := config.ContainerStatusExited if pod != nil && isPodRunning(pod) { - status = "running" + status = config.ContainerStatusRunning } // check started diff --git a/pkg/driver/microsandbox/microsandbox.go b/pkg/driver/microsandbox/microsandbox.go index 5c5d47b04..100cef53d 100644 --- a/pkg/driver/microsandbox/microsandbox.go +++ b/pkg/driver/microsandbox/microsandbox.go @@ -465,9 +465,9 @@ func hasUserNSMapping(options *driver.RunOptions) bool { } func toContainerDetails(info *sandboxInfo) *config.ContainerDetails { - status := "exited" + status := config.ContainerStatusExited if info.Running { - status = "running" + status = config.ContainerStatusRunning } return &config.ContainerDetails{ ID: info.Name, diff --git a/pkg/workspace/exec.go b/pkg/workspace/exec.go index 2c07ef6ab..18fb5e0ad 100644 --- a/pkg/workspace/exec.go +++ b/pkg/workspace/exec.go @@ -23,8 +23,7 @@ import ( ) const ( - DefaultDockerCommand = "docker" - ContainerStatusRunning = "running" + DefaultDockerCommand = "docker" ) // defaultExecTimeoutSeconds bounds an exec when no caller or configured default @@ -283,7 +282,7 @@ func (r *DockerRuntime) FindRunning( ) } - if !strings.EqualFold(container.State.Status, ContainerStatusRunning) { + if container.State.Status != devcconfig.ContainerStatusRunning { return nil, fmt.Errorf( "container %s is not running (status: %s)", container.ID, diff --git a/pkg/workspace/exec_apple.go b/pkg/workspace/exec_apple.go index 676c4e9bc..8f6619e37 100644 --- a/pkg/workspace/exec_apple.go +++ b/pkg/workspace/exec_apple.go @@ -4,7 +4,6 @@ import ( "context" "fmt" "io" - "strings" "github.com/devsy-org/devsy/pkg/apple" devcconfig "github.com/devsy-org/devsy/pkg/devcontainer/config" @@ -116,7 +115,7 @@ func (r *AppleRuntime) FindRunning( if container == nil { return nil, fmt.Errorf("no running container found for workspace %q", workspaceID) } - if !strings.EqualFold(container.State.Status, ContainerStatusRunning) { + if container.State.Status != devcconfig.ContainerStatusRunning { return nil, fmt.Errorf( "container %s is not running (status: %s)", container.ID, container.State.Status,