Add stop command #21
Add stop command #21
Conversation
gtsiolis
requested review from
carole-lavillonniere and
silv-io
as code owners
internal/container/stop.go
Outdated
| msg := e.Err.Error() | ||
|
|
||
| if strings.Contains(msg, "No such container") || strings.Contains(msg, "not found") { | ||
| return fmt.Sprintf("%s is not running", e.Name) |
There was a problem hiding this comment.
suggestion: It seems like the docker sdk is defining functions that can be used to check the error instead of doing string comparison: https://pkg.go.dev/github.com/containerd/errdefs#IsNotFound
There was a problem hiding this comment.
With this change I don't think there is a need to define a custom error StopError anymore
📝 WalkthroughWalkthroughThis PR introduces a new "stop" subcommand for the CLI that stops and removes LocalStack containers. The implementation spans infrastructure layers: a new Stop and Remove interface contract in the runtime abstraction, corresponding Docker implementations, container orchestration logic, and integration tests that validate the stop behavior. Changes
Sequence DiagramsequenceDiagram
actor User
participant CLI as CLI Command<br/>(stop.go)
participant Orchestration as Container<br/>Orchestration<br/>(container/stop.go)
participant Runtime as Docker<br/>Runtime<br/>(runtime/docker.go)
participant Docker as Docker<br/>Daemon
User->>CLI: Execute stop command
CLI->>CLI: Create DockerRuntime
CLI->>Orchestration: Call Stop(ctx, rt, onProgress)
Orchestration->>Orchestration: Load configuration
Orchestration->>Orchestration: Iterate containers
loop For each container
Orchestration->>Orchestration: Emit onProgress (starting)
Orchestration->>Runtime: Stop(ctx, containerName)
Runtime->>Docker: ContainerStop
Docker->>Docker: Stop container
Docker-->>Runtime: OK
Runtime->>Docker: ContainerRemove
Docker->>Docker: Remove container
Docker-->>Runtime: OK
Orchestration->>Orchestration: Emit onProgress (stopped)
end
Orchestration-->>CLI: Return result
CLI-->>User: Output progress/status
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~30 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
Some of the checks also fail in @carole-lavillonniere, could you take another look when you get a chance? 😁 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/integration/stop_test.go`:
- Around line 14-22: The test fails when the image is missing because
startTestContainer calls dockerClient.ContainerCreate without pulling the image;
update startTestContainer to explicitly pull "alpine:latest" before calling
dockerClient.ContainerCreate by invoking dockerClient.ImagePull(ctx,
"alpine:latest", types.ImagePullOptions{}), reading and closing the returned
io.ReadCloser to completion, checking for errors, then proceed with
dockerClient.ContainerCreate and dockerClient.ContainerStart (use the same
containerName/resp.ID as before).
| func startTestContainer(t *testing.T, ctx context.Context) { | ||
| t.Helper() | ||
| resp, err := dockerClient.ContainerCreate(ctx, &container.Config{ | ||
| Image: "alpine:latest", | ||
| Cmd: []string{"sleep", "infinity"}, | ||
| }, nil, nil, nil, containerName) | ||
| require.NoError(t, err, "failed to create test container") | ||
| err = dockerClient.ContainerStart(ctx, resp.ID, container.StartOptions{}) | ||
| require.NoError(t, err, "failed to start test container") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the test file
find . -name "stop_test.go" -path "*/test/integration/*" | head -5Repository: localstack/lstk
Length of output: 90
🏁 Script executed:
# Read the test file to see the current implementation
cat -n test/integration/stop_test.goRepository: localstack/lstk
Length of output: 3076
🏁 Script executed:
# Check the imports already present in the file
head -20 test/integration/stop_test.goRepository: localstack/lstk
Length of output: 563
🏁 Script executed:
# Search for any CI configuration or workflow files that might show failures
find . -name "*.yml" -o -name "*.yaml" | grep -E "(workflow|ci|github)" | head -10Repository: localstack/lstk
Length of output: 85
🏁 Script executed:
# Check the CI workflow for test execution and any indicators of failures
cat -n ./.github/workflows/ci.yml | head -100Repository: localstack/lstk
Length of output: 3019
🏁 Script executed:
# Search for any other references to alpine or docker image pulling in test files
rg "ImagePull|PullOptions" test/Repository: localstack/lstk
Length of output: 41
🏁 Script executed:
# Check if there's a setup function or init in the test file that might handle image pulling
rg -B5 -A5 "func.*dockerClient" test/integration/stop_test.goRepository: localstack/lstk
Length of output: 41
🏁 Script executed:
# Look at the entire stop_test.go file to understand the test setup
tail -n +1 test/integration/stop_test.go | head -100Repository: localstack/lstk
Length of output: 2502
🏁 Script executed:
# Find where dockerClient is defined
rg "var dockerClient|dockerClient\s*=" test/integration/Repository: localstack/lstk
Length of output: 120
🏁 Script executed:
# Search for any test setup or init functions
rg "func init|func setup|func Setup" test/integration/Repository: localstack/lstk
Length of output: 41
🏁 Script executed:
# Check if there are other files in the integration test directory
ls -la test/integration/Repository: localstack/lstk
Length of output: 759
🏁 Script executed:
# Look for the dockerClient initialization
rg -B10 "dockerClient" test/integration/stop_test.go | head -30Repository: localstack/lstk
Length of output: 1013
🏁 Script executed:
# Check the main_test.go file for dockerClient initialization and setup
cat -n test/integration/main_test.goRepository: localstack/lstk
Length of output: 3051
🏁 Script executed:
# Search for docker image pulling in the entire test directory
rg "ImagePull|PullOptions" test/Repository: localstack/lstk
Length of output: 41
🏁 Script executed:
# Check if alpine image is pre-pulled or if there's a makefile target that sets it up
cat -n Makefile | grep -A10 "test-integration"Repository: localstack/lstk
Length of output: 1018
🏁 Script executed:
# Check if there's any docker image pre-pulling or setup in CI or any other helper scripts
rg "pull.*alpine|docker.*pull" .github/ Makefile scripts/ 2>/dev/null || echo "No docker pull found in CI/Makefile"Repository: localstack/lstk
Length of output: 94
🏁 Script executed:
# Verify the docker client API - check if ImagePull is the correct method
rg "ImagePull" test/integration/ || echo "ImagePull not used yet"Repository: localstack/lstk
Length of output: 81
🏁 Script executed:
# Check start_test.go to see if it has a similar pattern
cat -n test/integration/start_test.go | grep -A20 "startTestContainer"Repository: localstack/lstk
Length of output: 41
🏁 Script executed:
# Check git history to see if there were recent test failures or commits related to this
git log --oneline test/integration/stop_test.go 2>/dev/null | head -10Repository: localstack/lstk
Length of output: 82
🏁 Script executed:
# Look for any docker-compose files that might pre-pull images
find . -name "docker-compose*" -o -name ".dockerignore" | head -10Repository: localstack/lstk
Length of output: 41
🏁 Script executed:
# Verify the docker client library version and check if ImagePull behavior is as described
rg "github.com/docker/docker" test/integration/go.modRepository: localstack/lstk
Length of output: 105
🏁 Script executed:
# Verify the Docker Go client ImagePull API signature
rg -A5 "func.*ImagePull" test/integration/ || echo "Checking Docker client docs..."Repository: localstack/lstk
Length of output: 89
🏁 Script executed:
# Check if there are any recent test runs or logs that show failures
find . -name "*test*results*" -o -name "*junit*" 2>/dev/null | head -5Repository: localstack/lstk
Length of output: 41
🏁 Script executed:
# Verify the proposed fix compiles by checking if the imports are correct for this version
rg "github.com/docker/docker/api/types/image" test/integration/ || echo "image package not yet imported"Repository: localstack/lstk
Length of output: 89
Pull the test image before creating the container.
ContainerCreate doesn't auto-pull missing images. Without pulling alpine:latest, the test container creation will fail on systems where the image isn't pre-cached (such as CI runners). Add the image pull before container creation.
Proposed fix
@@
import (
"context"
+ "io"
"os/exec"
"testing"
"time"
"github.com/docker/docker/api/types/container"
+ "github.com/docker/docker/api/types/image"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
@@
func startTestContainer(t *testing.T, ctx context.Context) {
t.Helper()
+ reader, err := dockerClient.ImagePull(ctx, "alpine:latest", image.PullOptions{})
+ require.NoError(t, err, "failed to pull test image")
+ defer func() {
+ _ = reader.Close()
+ }()
+ _, err = io.Copy(io.Discard, reader)
+ require.NoError(t, err, "failed to read image pull output")
+
resp, err := dockerClient.ContainerCreate(ctx, &container.Config{
Image: "alpine:latest",
Cmd: []string{"sleep", "infinity"},
}, nil, nil, nil, containerName)
require.NoError(t, err, "failed to create test container")
err = dockerClient.ContainerStart(ctx, resp.ID, container.StartOptions{})
require.NoError(t, err, "failed to start test container")
}🤖 Prompt for AI Agents
In `@test/integration/stop_test.go` around lines 14 - 22, The test fails when the
image is missing because startTestContainer calls dockerClient.ContainerCreate
without pulling the image; update startTestContainer to explicitly pull
"alpine:latest" before calling dockerClient.ContainerCreate by invoking
dockerClient.ImagePull(ctx, "alpine:latest", types.ImagePullOptions{}), reading
and closing the returned io.ReadCloser to completion, checking for errors, then
proceed with dockerClient.ContainerCreate and dockerClient.ContainerStart (use
the same containerName/resp.ID as before).
We are aware of the integration tests failing on windows/main, just created an issue: DRG-505 However there are more tests failing in this PR, namely: |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@internal/container/start.go`:
- Around line 53-57: The error check in the container removal loop uses
containerd's errdefs, causing Docker-specific errors to be misclassified; update
the import and check to use Docker's errdefs: replace usages of
containerd/errdefs.IsNotFound(err) with
github.com/docker/docker/errdefs.IsNotFound(err) (the removal loop calling
rt.Remove and the DockerRuntime.Remove implementation will then correctly allow
not-found errors to be ignored).
In `@internal/runtime/docker.go`:
- Around line 99-107: The Stop implementation currently calls ContainerRemove
with empty RemoveOptions which leaves volumes behind; update DockerRuntime.Stop
to call ContainerRemove(ctx, containerName,
container.RemoveOptions{RemoveVolumes: true}) so volumes are removed when
stopping a container (and consider making the same change for
DockerRuntime.Remove if removal should always include volumes), or alternatively
add a comment/docstring on DockerRuntime.Stop and DockerRuntime.Remove
clarifying that volumes are intentionally retained.
🧹 Nitpick comments (1)
cmd/stop.go (1)
12-31: UseRunEto return errors instead of callingos.Exitdirectly.This is the recommended Cobra pattern for commands that can fail. It improves testability and lets Cobra handle error output and exit codes consistently. Errors returned from
RunEare processed at the top-levelExecute()call, which handles the exit code once instead of scattered throughout the command logic.♻️ Proposed refactor
var stopCmd = &cobra.Command{ Use: "stop", Short: "Stop LocalStack", Long: "Stop the LocalStack emulator.", - Run: func(cmd *cobra.Command, args []string) { + RunE: func(cmd *cobra.Command, args []string) error { rt, err := runtime.NewDockerRuntime() if err != nil { - fmt.Fprintf(os.Stderr, "Error: %v\n", err) - os.Exit(1) + return err } onProgress := func(msg string) { - fmt.Println(msg) + fmt.Fprintln(cmd.OutOrStdout(), msg) } if err := container.Stop(cmd.Context(), rt, onProgress); err != nil { - fmt.Fprintln(os.Stderr, err) - os.Exit(1) + return err } + return nil }, }
| for _, config := range containers { | ||
| // Remove any existing stopped container with the same name | ||
| if err := rt.Remove(ctx, config.Name); err != nil && !errdefs.IsNotFound(err) { | ||
| return fmt.Errorf("failed to remove existing container %s: %w", config.Name, err) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's understand the file structure and locate the relevant code
head -60 internal/container/start.go | cat -nRepository: localstack/lstk
Length of output: 1912
🏁 Script executed:
# Check imports in the start.go file to understand what errdefs is being used
head -20 internal/container/start.go | cat -nRepository: localstack/lstk
Length of output: 630
🏁 Script executed:
# Search for the rt.Remove definition and understand what rt is
rg -B 10 "rt.Remove" internal/container/start.goRepository: localstack/lstk
Length of output: 364
🏁 Script executed:
# Look for the runtime type definition in the codebase
rg "type.*Runtime" internal/container/ --max-count 5Repository: localstack/lstk
Length of output: 41
🏁 Script executed:
# Check for Docker/containerd imports across the codebase
rg "containerd|docker" internal/container/start.goRepository: localstack/lstk
Length of output: 91
🏁 Script executed:
# Find the runtime package and interface definition
find internal/runtime -type f -name "*.go" | head -10Repository: localstack/lstk
Length of output: 113
🏁 Script executed:
# Look for the Runtime interface definition
rg "type Runtime interface" internal/runtime/Repository: localstack/lstk
Length of output: 111
🏁 Script executed:
# Search for Remove method implementation
rg -A 10 "func.*Remove" internal/runtime/ --max-count 5Repository: localstack/lstk
Length of output: 749
🏁 Script executed:
# Check what Docker client is being used in the runtime implementation
rg "docker|Docker" internal/runtime/ | head -20Repository: localstack/lstk
Length of output: 1448
🏁 Script executed:
# Let's check the complete Remove method and error handling in docker.go
cat -n internal/runtime/docker.go | sed -n '/func (d \*DockerRuntime) Remove/,/^[0-9]*\tFunc/p' | head -20Repository: localstack/lstk
Length of output: 840
🏁 Script executed:
# Check if there's any error handling or conversion in the docker.go file
cat internal/runtime/docker.go | head -50Repository: localstack/lstk
Length of output: 1236
🏁 Script executed:
# Look for any other error handling in docker.go to see patterns
rg "errdefs\|error" internal/runtime/docker.goRepository: localstack/lstk
Length of output: 41
🏁 Script executed:
# Check the complete docker.go file size and content
wc -l internal/runtime/docker.go && echo "---" && cat internal/runtime/docker.goRepository: localstack/lstk
Length of output: 3568
Replace containerd/errdefs.IsNotFound() with docker/errdefs.IsNotFound() for Docker error compatibility.
The Remove() method in DockerRuntime returns errors directly from the Docker client's ContainerRemove(), which returns Docker-specific error types from the docker/errdefs package. However, the error check at line 55 uses containerd/errdefs.IsNotFound(), which cannot recognize Docker errors. This causes the code to fail when a container doesn't exist instead of gracefully skipping the removal, breaking fresh starts.
Import github.com/docker/docker/errdefs and use errdefs.IsNotFound(err) from the Docker package instead of the containerd version.
🤖 Prompt for AI Agents
In `@internal/container/start.go` around lines 53 - 57, The error check in the
container removal loop uses containerd's errdefs, causing Docker-specific errors
to be misclassified; update the import and check to use Docker's errdefs:
replace usages of containerd/errdefs.IsNotFound(err) with
github.com/docker/docker/errdefs.IsNotFound(err) (the removal loop calling
rt.Remove and the DockerRuntime.Remove implementation will then correctly allow
not-found errors to be ignored).
| func (d *DockerRuntime) Stop(ctx context.Context, containerName string) error { | ||
| if err := d.client.ContainerStop(ctx, containerName, container.StopOptions{}); err != nil { | ||
| return err | ||
| } | ||
| return d.client.ContainerRemove(ctx, containerName, container.RemoveOptions{}) | ||
| } | ||
|
|
||
| func (d *DockerRuntime) Remove(ctx context.Context, containerName string) error { | ||
| return d.client.ContainerRemove(ctx, containerName, container.RemoveOptions{}) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
github.com/docker/docker v28 ContainerStopOptions defaults timeout
💡 Result:
In github.com/docker/docker v28, ContainerStopOptions (API type container.StopOptions) has:
Timeout *int(seconds). If you don’t set it, it staysnil(so it’s omitted). [1]
What happens when Timeout == nil:
- The client won’t send the
t/timeout query parameter at all. [2] - The daemon then uses the container’s configured
StopTimeout(if set); otherwise it falls back to the engine default. [3]
Engine default stop timeout (when neither API timeout nor per-container StopTimeout is set):
- non-Windows:
10seconds (defaultStopTimeout = 10) [4] - Windows:
30seconds (defaultStopTimeout = 30) [5]
Sources
[1] (pkg.go.dev)
[2] (fuchsia.googlesource.com)
[3] (pkg.go.dev)
[4] (fossies.org)
[5] (fossies.org)
Citations:
- 1: https://pkg.go.dev/github.com/docker/docker/api/types/container?utm_source=openai
- 2: https://fuchsia.googlesource.com/third_party/github.com/moby/moby/%2B/refs/tags/v28.3.0/client/container_stop.go?utm_source=openai
- 3: https://pkg.go.dev/github.com/docker/docker/daemon?utm_source=openai
- 4: https://fossies.org/linux/moby/container/container_unix.go?utm_source=openai
- 5: https://fossies.org/linux/moby/container/container_windows.go?utm_source=openai
🌐 Web query:
github.com/docker/docker v28 ContainerRemoveOptions defaults volume removal
💡 Result:
In github.com/docker/docker v28, ContainerRemoveOptions.RemoveVolumes is a bool with no explicit default set in code, so its zero-value default is false (i.e., volumes are not removed unless you set it). [1]
This matches the Docker Engine API: DELETE /containers/{id} has query param v (“remove volumes”) with default false. [2]
So, to remove volumes you must opt in, e.g.:
- Go:
ContainerRemoveOptions{RemoveVolumes: true} - API:
DELETE .../containers/{id}?v=1 - CLI:
docker rm -v .../--volumes[3]
Sources: [1] [2] [3]
Clarify whether volumes should be removed in the Stop operation.
The Stop() method uses empty StopOptions{}, which defers to the Docker daemon's default timeout (10 seconds on non-Windows, 30 seconds on Windows)—a reasonable default. However, ContainerRemove() with empty RemoveOptions{} leaves volumes behind by default (equivalent to docker rm without -v). If lstk stop is meant to fully clean up the container and its associated resources, set RemoveVolumes: true in the RemoveOptions:
container.RemoveOptions{RemoveVolumes: true}Otherwise, document the volume retention behavior if it is intentional.
🤖 Prompt for AI Agents
In `@internal/runtime/docker.go` around lines 99 - 107, The Stop implementation
currently calls ContainerRemove with empty RemoveOptions which leaves volumes
behind; update DockerRuntime.Stop to call ContainerRemove(ctx, containerName,
container.RemoveOptions{RemoveVolumes: true}) so volumes are removed when
stopping a container (and consider making the same change for
DockerRuntime.Remove if removal should always include volumes), or alternatively
add a comment/docstring on DockerRuntime.Stop and DockerRuntime.Remove
clarifying that volumes are intentionally retained.
2 participants
Fix DES-108
Summary by CodeRabbit
Release Notes