From 90f546f9aabb92852f8b15557ae05412e9c5bde9 Mon Sep 17 00:00:00 2001 From: Matty Evans Date: Thu, 16 Jan 2025 08:50:47 +1000 Subject: [PATCH 1/3] fix(docker): ensure orphaned contributoor containers can be managed --- internal/sidecar/docker.go | 37 ++++++++++++++++++------- internal/sidecar/docker_test.go | 48 ++++++++++++++++++++++++++++++++- 2 files changed, 74 insertions(+), 11 deletions(-) diff --git a/internal/sidecar/docker.go b/internal/sidecar/docker.go index 5f911eb..e3673ea 100644 --- a/internal/sidecar/docker.go +++ b/internal/sidecar/docker.go @@ -83,7 +83,8 @@ func (s *dockerSidecar) Start() error { // Stop stops and removes the docker container using docker-compose. func (s *dockerSidecar) Stop() error { - // Stop and remove containers, volumes, and networks + // First try to stop via compose. If there has been any sort of configuration change + // between versions, then this will not stop the container. //nolint:gosec // validateComposePath() and filepath.Clean() in-use. cmd := exec.Command("docker", "compose", "-f", s.composePath, "down", "--remove-orphans", @@ -93,7 +94,15 @@ func (s *dockerSidecar) Stop() error { cmd.Env = s.getComposeEnv() if output, err := cmd.CombinedOutput(); err != nil { - return fmt.Errorf("failed to stop containers: %w\nOutput: %s", err, string(output)) + // Don't return error here, try our fallback. + s.logger.Warnf("failed to stop via compose: %v\noutput: %s", err, string(output)) + } + + // Fallback in the case of a configuration change between versions, attempt to remove + // the container by name. + cmd = exec.Command("docker", "rm", "-f", "contributoor") + if output, err := cmd.CombinedOutput(); err != nil { + return fmt.Errorf("failed to stop container: %w\nOutput: %s", err, string(output)) } fmt.Printf("%sContributoor stopped successfully%s\n", tui.TerminalColorGreen, tui.TerminalColorReset) @@ -103,23 +112,31 @@ func (s *dockerSidecar) Stop() error { // IsRunning checks if the docker container is running. func (s *dockerSidecar) IsRunning() (bool, error) { + // Check via compose first. If there has been any sort of configuration change between + // versions, then this will return a non running state. //nolint:gosec // validateComposePath() and filepath.Clean() in-use. cmd := exec.Command("docker", "compose", "-f", s.composePath, "ps", "--format", "{{.State}}") cmd.Env = s.getComposeEnv() output, err := cmd.Output() - if err != nil { - return false, fmt.Errorf("failed to check container status: %w", err) + if err == nil { + states := strings.Split(strings.TrimSpace(string(output)), "\n") + for _, state := range states { + if strings.Contains(strings.ToLower(state), "running") { + return true, nil + } + } } - states := strings.Split(strings.TrimSpace(string(output)), "\n") - for _, state := range states { - if strings.Contains(strings.ToLower(state), "running") { - return true, nil - } + // In that case, we will fallback to checking for any container with the name 'contributoor'. + cmd = exec.Command("docker", "ps", "-q", "-f", "name=contributoor", "-f", "status=running") + + output, err = cmd.Output() + if err != nil { + return false, fmt.Errorf("failed to check container status: %w", err) } - return false, nil + return len(strings.TrimSpace(string(output))) > 0, nil } // Update pulls the latest image and restarts the container. diff --git a/internal/sidecar/docker_test.go b/internal/sidecar/docker_test.go index c832aaf..81e5c8c 100644 --- a/internal/sidecar/docker_test.go +++ b/internal/sidecar/docker_test.go @@ -3,8 +3,11 @@ package sidecar_test import ( "context" "fmt" + "io" "os" + "os/exec" "path/filepath" + "sync" "testing" "time" @@ -123,10 +126,18 @@ func TestDockerService_Integration(t *testing.T) { for { select { case <-ctx.Done(): + // Fix data race by using a mutex for logs access + var logsMutex sync.Mutex + logsMutex.Lock() logs, err := container.Logs(context.Background()) if err == nil { - t.Logf("docker-in-docker container logs:\n%s", logs) + // Convert logs to string before logging + logsBytes, err := io.ReadAll(logs) + if err == nil { + t.Logf("docker-in-docker container logs:\n%s", string(logsBytes)) + } } + logsMutex.Unlock() t.Fatal("timeout waiting for docker-in-docker container to become healthy") default: running, err := ds.IsRunning() @@ -174,4 +185,39 @@ func TestDockerService_Integration(t *testing.T) { require.NoError(t, err) require.False(t, running) }) + + t.Run("lifecycle_with_external_container", func(t *testing.T) { + // Write out compose file. + require.NoError(t, os.WriteFile(filepath.Join(tmpDir, "docker-compose.yml"), []byte(composeFile), 0644)) + + // Start a container directly with docker (not via compose) of the same name. This mimics + // a container the installer isn't aware of. + cmd := exec.Command("docker", "run", "-d", "--name", "contributoor", "busybox", + "sh", "-c", "while true; do echo 'Container is running'; sleep 1; done") + output, err := cmd.CombinedOutput() + require.NoError(t, err, "failed to start container: %s", string(output)) + + // IsRunning should detect the external container. + running, err := ds.IsRunning() + require.NoError(t, err) + require.True(t, running, "IsRunning should detect externally started container") + + // Stop should be able to handle the external container. + require.NoError(t, ds.Stop()) + + // Verify container is stopped. + running, err = ds.IsRunning() + require.NoError(t, err) + require.False(t, running, "Container should be stopped") + + // Finally, test normal compose lifecycle works after cleaning up external container. + require.NoError(t, ds.Start()) + checkContainerHealth(t) + + require.NoError(t, ds.Stop()) + + running, err = ds.IsRunning() + require.NoError(t, err) + require.False(t, running) + }) } From 58316a5396fdef2c1807b6b8642cc391d4d7d087 Mon Sep 17 00:00:00 2001 From: Matty Evans Date: Thu, 16 Jan 2025 08:56:57 +1000 Subject: [PATCH 2/3] refactor: Remove unnecessary code and simplify logging in test --- internal/sidecar/docker_test.go | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/internal/sidecar/docker_test.go b/internal/sidecar/docker_test.go index 81e5c8c..e0f5b43 100644 --- a/internal/sidecar/docker_test.go +++ b/internal/sidecar/docker_test.go @@ -3,11 +3,9 @@ package sidecar_test import ( "context" "fmt" - "io" "os" "os/exec" "path/filepath" - "sync" "testing" "time" @@ -126,18 +124,10 @@ func TestDockerService_Integration(t *testing.T) { for { select { case <-ctx.Done(): - // Fix data race by using a mutex for logs access - var logsMutex sync.Mutex - logsMutex.Lock() logs, err := container.Logs(context.Background()) if err == nil { - // Convert logs to string before logging - logsBytes, err := io.ReadAll(logs) - if err == nil { - t.Logf("docker-in-docker container logs:\n%s", string(logsBytes)) - } + t.Logf("docker-in-docker container logs:\n%s", logs) } - logsMutex.Unlock() t.Fatal("timeout waiting for docker-in-docker container to become healthy") default: running, err := ds.IsRunning() From 02c72bd0373c8797276a7cb7463aa488b33db3fd Mon Sep 17 00:00:00 2001 From: Matty Evans Date: Thu, 16 Jan 2025 09:00:52 +1000 Subject: [PATCH 3/3] style: Use debug log level for docker stop error message --- internal/sidecar/docker.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/sidecar/docker.go b/internal/sidecar/docker.go index e3673ea..5d3299c 100644 --- a/internal/sidecar/docker.go +++ b/internal/sidecar/docker.go @@ -95,7 +95,7 @@ func (s *dockerSidecar) Stop() error { if output, err := cmd.CombinedOutput(); err != nil { // Don't return error here, try our fallback. - s.logger.Warnf("failed to stop via compose: %v\noutput: %s", err, string(output)) + s.logger.Debugf("failed to stop via compose: %v\noutput: %s", err, string(output)) } // Fallback in the case of a configuration change between versions, attempt to remove