Skip to content

Commit

Permalink
chore: prevent orchestrator stopTask from returning false errors (#5531)
Browse files Browse the repository at this point in the history 
Made a mistake in #5489 that causes an error message to falsely appear when stopping `run local` because it would call `docker inspect` when it only needed to call `docker ps`. This introduces another function that only runs the latter function, rather than running both, fixing the issue.



By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the Apache 2.0 License.
  • Loading branch information
@CaptainCarpensir
CaptainCarpensir authored Dec 6, 2023
1 parent 688b639 commit 90b23b0
Show file tree
Hide file tree
Showing 6 changed files with 65 additions and 30 deletions.
1 change: 1 addition & 0 deletions internal/pkg/cli/interfaces.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -706,6 +706,7 @@ type templateDiffer interface {
type dockerEngineRunner interface {
CheckDockerEngineRunning() error
Run(context.Context, *dockerengine.RunOptions) error
DoesContainerExist(context.Context, string) (bool, error)
IsContainerRunning(context.Context, string) (bool, error)
Stop(context.Context, string) error
Rm(string) error
Expand Down
15 changes: 15 additions & 0 deletions internal/pkg/cli/mocks/mock_interfaces.go
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 9 additions & 0 deletions internal/pkg/docker/dockerengine/dockerengine.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -351,6 +351,15 @@ func (c DockerCmdClient) Run(ctx context.Context, options *RunOptions) error {
return g.Wait()
}

// DoesContainerExist checks if a specific Docker container exists.
func (c DockerCmdClient) DoesContainerExist(ctx context.Context, name string) (bool, error) {
output, err := c.containerID(ctx, name)
if err != nil {
return false, err
}
return output != "", nil
}

// IsContainerRunning checks if a specific Docker container is running.
func (c DockerCmdClient) IsContainerRunning(ctx context.Context, name string) (bool, error) {
state, err := c.containerState(ctx, name)
Expand Down
9 changes: 9 additions & 0 deletions internal/pkg/docker/dockerengine/dockerenginetest/dockerenginetest.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
// Double is a test double for dockerengine.DockerCmdClient
type Double struct {
StopFn func(context.Context, string) error
DoesContainerExistFn func(context.Context, string) (bool, error)
IsContainerRunningFn func(context.Context, string) (bool, error)
RunFn func(context.Context, *dockerengine.RunOptions) error
BuildFn func(context.Context, *dockerengine.BuildArguments, io.Writer) error
Expand All @@ -27,6 +28,14 @@ func (d *Double) Stop(ctx context.Context, name string) error {
return d.StopFn(ctx, name)
}

// DoesContainerExist calls the stubbed function.
func (d *Double) DoesContainerExist(ctx context.Context, name string) (bool, error) {
if d.IsContainerRunningFn == nil {
return false, nil
}
return d.DoesContainerExistFn(ctx, name)
}

// IsContainerRunning calls the stubbed function.
func (d *Double) IsContainerRunning(ctx context.Context, name string) (bool, error) {
if d.IsContainerRunningFn == nil {
Expand Down
5 changes: 3 additions & 2 deletions internal/pkg/docker/orchestrator/orchestrator.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ type logOptionsFunc func(name string, ctr ContainerDefinition) dockerengine.RunL
// DockerEngine is used by Orchestrator to manage containers.
type DockerEngine interface {
Run(context.Context, *dockerengine.RunOptions) error
DoesContainerExist(context.Context, string) (bool, error)
IsContainerRunning(context.Context, string) (bool, error)
Stop(context.Context, string) error
Build(ctx context.Context, args *dockerengine.BuildArguments, w io.Writer) error
Expand Down Expand Up @@ -398,13 +399,13 @@ func (o *Orchestrator) stopTask(ctx context.Context, task Task) error {

// ensure that container is fully stopped before stopTask finishes blocking
for {
running, err := o.docker.IsContainerRunning(ctx, o.containerID(name))
exists, err := o.docker.DoesContainerExist(ctx, o.containerID(name))
if err != nil {
errCh <- fmt.Errorf("polling container %q for removal: %w", name, err)
return
}

if running {
if exists {
select {
case <-time.After(1 * time.Second):
continue
Expand Down
56 changes: 28 additions & 28 deletions internal/pkg/docker/orchestrator/orchestrator_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,12 +85,12 @@ func TestOrchestrator(t *testing.T) {
runUntilStopped: true,
test: func(t *testing.T) (test, *dockerenginetest.Double) {
de := &dockerenginetest.Double{
IsContainerRunningFn: func(ctx context.Context, name string) (bool, error) {
if name == "prefix-pause" {
return true, nil
}
DoesContainerExistFn: func(ctx context.Context, s string) (bool, error) {
return false, nil
},
IsContainerRunningFn: func(ctx context.Context, name string) (bool, error) {
return true, nil
},
StopFn: func(ctx context.Context, name string) error {
if name == "prefix-success" {
return nil
Expand Down Expand Up @@ -119,12 +119,12 @@ func TestOrchestrator(t *testing.T) {
runUntilStopped: true,
test: func(t *testing.T) (test, *dockerenginetest.Double) {
de := &dockerenginetest.Double{
IsContainerRunningFn: func(ctx context.Context, name string) (bool, error) {
if name == "prefix-pause" {
return true, nil
}
DoesContainerExistFn: func(ctx context.Context, s string) (bool, error) {
return false, errors.New("some error")
},
IsContainerRunningFn: func(ctx context.Context, name string) (bool, error) {
return true, nil
},
StopFn: func(ctx context.Context, name string) error {
return nil
},
Expand All @@ -148,12 +148,12 @@ func TestOrchestrator(t *testing.T) {
runUntilStopped: true,
test: func(t *testing.T) (test, *dockerenginetest.Double) {
de := &dockerenginetest.Double{
IsContainerRunningFn: func(ctx context.Context, name string) (bool, error) {
if name == "prefix-pause" {
return true, nil
}
DoesContainerExistFn: func(ctx context.Context, s string) (bool, error) {
return false, nil
},
IsContainerRunningFn: func(ctx context.Context, name string) (bool, error) {
return true, nil
},
}
return func(t *testing.T, o *Orchestrator) {
o.RunTask(Task{
Expand Down Expand Up @@ -185,12 +185,12 @@ func TestOrchestrator(t *testing.T) {
runUntilStopped: true,
test: func(t *testing.T) (test, *dockerenginetest.Double) {
de := &dockerenginetest.Double{
IsContainerRunningFn: func(ctx context.Context, name string) (bool, error) {
if name == "prefix-pause" {
return true, nil
}
DoesContainerExistFn: func(ctx context.Context, s string) (bool, error) {
return false, nil
},
IsContainerRunningFn: func(ctx context.Context, name string) (bool, error) {
return true, nil
},
RunFn: func(ctx context.Context, opts *dockerengine.RunOptions) error {
// validate pause container has correct ports and secrets
if opts.ContainerName == "prefix-pause" {
Expand Down Expand Up @@ -234,12 +234,12 @@ func TestOrchestrator(t *testing.T) {
test: func(t *testing.T) (test, *dockerenginetest.Double) {
stopPause := make(chan struct{})
de := &dockerenginetest.Double{
IsContainerRunningFn: func(ctx context.Context, name string) (bool, error) {
if name == "prefix-pause" {
return true, nil
}
DoesContainerExistFn: func(ctx context.Context, s string) (bool, error) {
return false, nil
},
IsContainerRunningFn: func(ctx context.Context, name string) (bool, error) {
return true, nil
},
RunFn: func(ctx context.Context, opts *dockerengine.RunOptions) error {
if opts.ContainerName == "prefix-foo" {
return errors.New("some error")
Expand Down Expand Up @@ -272,12 +272,12 @@ func TestOrchestrator(t *testing.T) {
test: func(t *testing.T) (test, *dockerenginetest.Double) {
stopPause := make(chan struct{})
de := &dockerenginetest.Double{
IsContainerRunningFn: func(ctx context.Context, name string) (bool, error) {
if name == "prefix-pause" {
return true, nil
}
DoesContainerExistFn: func(ctx context.Context, s string) (bool, error) {
return false, nil
},
IsContainerRunningFn: func(ctx context.Context, name string) (bool, error) {
return true, nil
},
RunFn: func(ctx context.Context, opts *dockerengine.RunOptions) error {
if opts.ContainerName == "prefix-foo" {
return nil
Expand Down Expand Up @@ -420,12 +420,12 @@ func TestOrchestrator(t *testing.T) {
runUntilStopped: true,
test: func(t *testing.T) (test, *dockerenginetest.Double) {
de := &dockerenginetest.Double{
IsContainerRunningFn: func(ctx context.Context, name string) (bool, error) {
if name == "prefix-pause" {
return true, nil
}
DoesContainerExistFn: func(ctx context.Context, s string) (bool, error) {
return false, nil
},
IsContainerRunningFn: func(ctx context.Context, name string) (bool, error) {
return true, nil
},
ExecFn: func(ctx context.Context, ctr string, w io.Writer, cmd string, args ...string) error {
if cmd == "aws" {
fmt.Fprintf(w, "Port 61972 opened for sessionId mySessionId\n")
Expand Down

0 comments on commit 90b23b0

Please sign in to comment.