From 8305c0a3a1fad1ce847d8f302773c4080e887380 Mon Sep 17 00:00:00 2001 From: Adithya Kolla Date: Wed, 9 Aug 2023 17:34:00 -0700 Subject: [PATCH 01/53] change exit codes of svc/env/job deploy --- internal/pkg/cli/svc_deploy.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/internal/pkg/cli/svc_deploy.go b/internal/pkg/cli/svc_deploy.go index d5685348073..9731734cd44 100644 --- a/internal/pkg/cli/svc_deploy.go +++ b/internal/pkg/cli/svc_deploy.go @@ -685,6 +685,18 @@ func (e *errHasDiff) ExitCode() int { return 1 } +type errNoInfrastructureChanges struct { + parentErr error +} + +func (e *errNoInfrastructureChanges) Error() string { + return e.parentErr.Error() +} + +func (e *errNoInfrastructureChanges) ExitCode() int { + return 0 +} + func diff(differ templateDiffer, tmpl string, writer io.Writer) error { if out, err := differ.DeployDiff(tmpl); err != nil { return err From ee2ee49fc05b256ee607779649803152e2b76bda Mon Sep 17 00:00:00 2001 From: Adithya Kolla Date: Thu, 10 Aug 2023 14:23:15 -0700 Subject: [PATCH 02/53] move errstruct to errors.go --- internal/pkg/cli/svc_deploy.go | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/internal/pkg/cli/svc_deploy.go b/internal/pkg/cli/svc_deploy.go index 9731734cd44..d5685348073 100644 --- a/internal/pkg/cli/svc_deploy.go +++ b/internal/pkg/cli/svc_deploy.go @@ -685,18 +685,6 @@ func (e *errHasDiff) ExitCode() int { return 1 } -type errNoInfrastructureChanges struct { - parentErr error -} - -func (e *errNoInfrastructureChanges) Error() string { - return e.parentErr.Error() -} - -func (e *errNoInfrastructureChanges) ExitCode() int { - return 0 -} - func diff(differ templateDiffer, tmpl string, writer io.Writer) error { if out, err := differ.DeployDiff(tmpl); err != nil { return err From f00e15c0d21f829d43647d53f43512e0bc80738c Mon Sep 17 00:00:00 2001 From: Adithya Kolla Date: Tue, 14 Nov 2023 01:29:57 -0800 Subject: [PATCH 03/53] parse container deps from manifest --- internal/pkg/cli/run_local.go | 22 +++++++ .../pkg/docker/orchestrator/orchestrator.go | 10 +-- internal/pkg/manifest/backend_svc.go | 6 ++ internal/pkg/manifest/job.go | 6 ++ internal/pkg/manifest/lb_web_svc.go | 6 ++ internal/pkg/manifest/lb_web_svc_test.go | 65 +++++++++++++++++++ internal/pkg/manifest/validate.go | 32 ++------- internal/pkg/manifest/worker_svc.go | 6 ++ internal/pkg/manifest/workload_ecs.go | 24 +++++++ 9 files changed, 148 insertions(+), 29 deletions(-) diff --git a/internal/pkg/cli/run_local.go b/internal/pkg/cli/run_local.go index 712e86c9710..2b1f8ed7ff4 100644 --- a/internal/pkg/cli/run_local.go +++ b/internal/pkg/cli/run_local.go @@ -515,9 +515,31 @@ func (o *runLocalOpts) prepareTask(ctx context.Context) (orchestrator.Task, erro task.Containers[name] = ctr } + // TODO (Adi): Use this dependency order in orchestrator to start and stop containers. + containerDeps := o.containerDependencies(mft.Manifest()) + for name, dep := range containerDeps { + ctr, ok := task.Containers[name] + if !ok { + return orchestrator.Task{}, fmt.Errorf("missing container: %q is listed as a dependency, which doesn't exist in the task", name) + } + ctr.IsEssential = dep.IsEssential + ctr.DependsOn = dep.DependsOn + } + return task, nil } +func (o *runLocalOpts) containerDependencies(unmarshaledManifest interface{}) map[string]manifest.ContainerDependency { + type containerDependency interface { + ContainerDependencies() map[string]manifest.ContainerDependency + } + mf, ok := unmarshaledManifest.(containerDependency) + if ok { + return mf.ContainerDependencies() + } + return nil +} + func (o *runLocalOpts) watchLocalFiles(stopCh <-chan struct{}) (<-chan interface{}, <-chan error, error) { workspacePath := o.ws.Path() diff --git a/internal/pkg/docker/orchestrator/orchestrator.go b/internal/pkg/docker/orchestrator/orchestrator.go index f1e9e97c52e..c5ef7f9dd7f 100644 --- a/internal/pkg/docker/orchestrator/orchestrator.go +++ b/internal/pkg/docker/orchestrator/orchestrator.go @@ -449,10 +449,12 @@ type Task struct { // ContainerDefinition defines information necessary to run a container. type ContainerDefinition struct { - ImageURI string - EnvVars map[string]string - Secrets map[string]string - Ports map[string]string // host port -> container port + ImageURI string + EnvVars map[string]string + Secrets map[string]string + Ports map[string]string // host port -> container port + IsEssential bool + DependsOn map[string]string } // pauseRunOptions returns RunOptions for the pause container for t. diff --git a/internal/pkg/manifest/backend_svc.go b/internal/pkg/manifest/backend_svc.go index 5a9be19b9db..35a7abd96b6 100644 --- a/internal/pkg/manifest/backend_svc.go +++ b/internal/pkg/manifest/backend_svc.go @@ -139,6 +139,12 @@ func (s *BackendService) EnvFiles() map[string]string { return envFiles(s.Name, s.TaskConfig, s.Logging, s.Sidecars) } +// ContainerDependencies constructs a map of ContainerDependency objects for the BackendService +// including dependencies for its main container, any logging sidecar, and additional sidecars. +func (s *BackendService) ContainerDependencies() map[string]ContainerDependency { + return containerDependencies(aws.StringValue(s.Name), s.ImageConfig.Image, s.Logging, s.Sidecars) +} + func (s *BackendService) subnets() *SubnetListOrArgs { return &s.Network.VPC.Placement.Subnets } diff --git a/internal/pkg/manifest/job.go b/internal/pkg/manifest/job.go index 5ef0f67a32d..a4502fe3a8f 100644 --- a/internal/pkg/manifest/job.go +++ b/internal/pkg/manifest/job.go @@ -159,6 +159,12 @@ func (j *ScheduledJob) EnvFiles() map[string]string { return envFiles(j.Name, j.TaskConfig, j.Logging, j.Sidecars) } +// ContainerDependencies constructs a map of ContainerDependency objects for ScheduledJob +// including dependencies for its main container, any logging sidecar, and additional sidecars. +func (s *ScheduledJob) ContainerDependencies() map[string]ContainerDependency { + return containerDependencies(aws.StringValue(s.Name), s.ImageConfig.Image, s.Logging, s.Sidecars) +} + // newDefaultScheduledJob returns an empty ScheduledJob with only the default values set. func newDefaultScheduledJob() *ScheduledJob { return &ScheduledJob{ diff --git a/internal/pkg/manifest/lb_web_svc.go b/internal/pkg/manifest/lb_web_svc.go index 904e2997109..9e0a436cbf3 100644 --- a/internal/pkg/manifest/lb_web_svc.go +++ b/internal/pkg/manifest/lb_web_svc.go @@ -214,6 +214,12 @@ func (s *LoadBalancedWebService) EnvFiles() map[string]string { return envFiles(s.Name, s.TaskConfig, s.Logging, s.Sidecars) } +// ContainerDependencies constructs a map of ContainerDependency objects for the LoadBalancedWebService +// including dependencies for its main container, any logging sidecar, and additional sidecars. +func (s *LoadBalancedWebService) ContainerDependencies() map[string]ContainerDependency { + return containerDependencies(aws.StringValue(s.Name), s.ImageConfig.Image, s.Logging, s.Sidecars) +} + func (s *LoadBalancedWebService) subnets() *SubnetListOrArgs { return &s.Network.VPC.Placement.Subnets } diff --git a/internal/pkg/manifest/lb_web_svc_test.go b/internal/pkg/manifest/lb_web_svc_test.go index b04dd9c8280..1f3b6dc8d41 100644 --- a/internal/pkg/manifest/lb_web_svc_test.go +++ b/internal/pkg/manifest/lb_web_svc_test.go @@ -2838,6 +2838,71 @@ func TestLoadBalancedWebService_BuildArgs(t *testing.T) { } } +func TestLoadBalancedWebService_ContainerDependencies(t *testing.T) { + testCases := map[string]struct { + in *LoadBalancedWebService + wantedDependencies map[string]ContainerDependency + wantedErr error + }{ + "return main container and sidecar container build args": { + in: &LoadBalancedWebService{ + Workload: Workload{ + Name: aws.String("mock-svc"), + Type: aws.String(manifestinfo.LoadBalancedWebServiceType), + }, + LoadBalancedWebServiceConfig: LoadBalancedWebServiceConfig{ + ImageConfig: ImageWithPortAndHealthcheck{ + ImageWithPort: ImageWithPort{ + Image: Image{ + DependsOn: DependsOn{ + "nginx": "start", + }, + }, + }, + }, + Sidecars: map[string]*SidecarConfig{ + "nginx": { + Essential: aws.Bool(true), + }, + "nginx1": { + DependsOn: DependsOn{ + "nginx": "healthy", + "mock-svc": "start", + }, + }, + }, + }, + }, + wantedDependencies: map[string]ContainerDependency{ + "mock-svc": { + IsEssential: true, + DependsOn: DependsOn{ + "nginx": "start", + }, + }, + "nginx": { + IsEssential: true, + }, + "nginx1": { + IsEssential: true, + DependsOn: DependsOn{ + "nginx": "healthy", + "mock-svc": "start", + }, + }, + }, + }, + } + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + // WHEN + got := tc.in.ContainerDependencies() + // THEN + require.Equal(t, tc.wantedDependencies, got) + }) + } +} + func TestNetworkLoadBalancerConfiguration_NLBListeners(t *testing.T) { testCases := map[string]struct { in NetworkLoadBalancerConfiguration diff --git a/internal/pkg/manifest/validate.go b/internal/pkg/manifest/validate.go index a6598b3ae01..c44fa02ca22 100644 --- a/internal/pkg/manifest/validate.go +++ b/internal/pkg/manifest/validate.go @@ -2014,11 +2014,6 @@ type validateDependenciesOpts struct { logging Logging } -type containerDependency struct { - dependsOn DependsOn - isEssential bool -} - type validateTargetContainerOpts struct { mainContainerName string mainContainerPort *uint16 @@ -2097,30 +2092,17 @@ func validateTargetContainer(opts validateTargetContainerOpts) error { } func validateContainerDeps(opts validateDependenciesOpts) error { - containerDependencies := make(map[string]containerDependency) - containerDependencies[opts.mainContainerName] = containerDependency{ - dependsOn: opts.imageConfig.DependsOn, - isEssential: true, - } - if !opts.logging.IsEmpty() { - containerDependencies[FirelensContainerName] = containerDependency{} - } - for name, config := range opts.sidecarConfig { - containerDependencies[name] = containerDependency{ - dependsOn: config.DependsOn, - isEssential: config.Essential == nil || aws.BoolValue(config.Essential), - } - } + containerDependencies := containerDependencies(opts.mainContainerName, opts.imageConfig, opts.logging, opts.sidecarConfig) if err := validateDepsForEssentialContainers(containerDependencies); err != nil { return err } return validateNoCircularDependencies(containerDependencies) } -func validateDepsForEssentialContainers(deps map[string]containerDependency) error { +func validateDepsForEssentialContainers(deps map[string]ContainerDependency) error { for name, containerDep := range deps { - for dep, status := range containerDep.dependsOn { - if !deps[dep].isEssential { + for dep, status := range containerDep.DependsOn { + if !deps[dep].IsEssential { continue } if err := validateEssentialContainerDependency(dep, strings.ToUpper(status)); err != nil { @@ -2313,7 +2295,7 @@ func validateEssentialContainerDependency(name, status string) error { return fmt.Errorf("essential container %s can only have status %s", name, english.WordSeries([]string{dependsOnStart, dependsOnHealthy}, "or")) } -func validateNoCircularDependencies(deps map[string]containerDependency) error { +func validateNoCircularDependencies(deps map[string]ContainerDependency) error { dependencies, err := buildDependencyGraph(deps) if err != nil { return err @@ -2330,10 +2312,10 @@ func validateNoCircularDependencies(deps map[string]containerDependency) error { return fmt.Errorf("circular container dependency chain includes the following containers: %s", cycle) } -func buildDependencyGraph(deps map[string]containerDependency) (*graph.Graph[string], error) { +func buildDependencyGraph(deps map[string]ContainerDependency) (*graph.Graph[string], error) { dependencyGraph := graph.New[string]() for name, containerDep := range deps { - for dep := range containerDep.dependsOn { + for dep := range containerDep.DependsOn { if _, ok := deps[dep]; !ok { return nil, fmt.Errorf("container %s does not exist", dep) } diff --git a/internal/pkg/manifest/worker_svc.go b/internal/pkg/manifest/worker_svc.go index 35b6dcb92ad..db8aa7625a9 100644 --- a/internal/pkg/manifest/worker_svc.go +++ b/internal/pkg/manifest/worker_svc.go @@ -293,6 +293,12 @@ func (s *WorkerService) EnvFiles() map[string]string { return envFiles(s.Name, s.TaskConfig, s.Logging, s.Sidecars) } +// ContainerDependencies constructs a map of ContainerDependency objects for the WorkerService +// including dependencies for its main container, any logging sidecar, and additional sidecars. +func (s *WorkerService) ContainerDependencies() map[string]ContainerDependency { + return containerDependencies(aws.StringValue(s.Name), s.ImageConfig.Image, s.Logging, s.Sidecars) +} + // Subscriptions returns a list of TopicSubscriotion objects which represent the SNS topics the service // receives messages from. This method also appends ".fifo" to the topics and returns a new set of subs. func (s *WorkerService) Subscriptions() []TopicSubscription { diff --git a/internal/pkg/manifest/workload_ecs.go b/internal/pkg/manifest/workload_ecs.go index 291e749db2c..672192d813e 100644 --- a/internal/pkg/manifest/workload_ecs.go +++ b/internal/pkg/manifest/workload_ecs.go @@ -438,3 +438,27 @@ func buildArgs(contextDir string, buildArgs map[string]*DockerBuildArgs, sc map[ } return buildArgs, nil } + +// Dependencies defined for container startup and shutdown. +type ContainerDependency struct { + IsEssential bool + DependsOn DependsOn +} + +func containerDependencies(name string, img Image, lc Logging, sc map[string]*SidecarConfig) map[string]ContainerDependency { + containerDependencies := make(map[string]ContainerDependency) + containerDependencies[name] = ContainerDependency{ + DependsOn: img.DependsOn, + IsEssential: true, + } + if !lc.IsEmpty() { + containerDependencies[FirelensContainerName] = ContainerDependency{} + } + for name, config := range sc { + containerDependencies[name] = ContainerDependency{ + DependsOn: config.DependsOn, + IsEssential: config.Essential == nil || aws.BoolValue(config.Essential), + } + } + return containerDependencies +} From a3a62af7d04aee57ccde533f9e10afbc714f6891 Mon Sep 17 00:00:00 2001 From: Adithya Kolla Date: Tue, 14 Nov 2023 01:34:56 -0800 Subject: [PATCH 04/53] change doc comment --- internal/pkg/manifest/backend_svc.go | 2 +- internal/pkg/manifest/job.go | 2 +- internal/pkg/manifest/lb_web_svc.go | 2 +- internal/pkg/manifest/worker_svc.go | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/internal/pkg/manifest/backend_svc.go b/internal/pkg/manifest/backend_svc.go index 35a7abd96b6..d35bc1bf104 100644 --- a/internal/pkg/manifest/backend_svc.go +++ b/internal/pkg/manifest/backend_svc.go @@ -139,7 +139,7 @@ func (s *BackendService) EnvFiles() map[string]string { return envFiles(s.Name, s.TaskConfig, s.Logging, s.Sidecars) } -// ContainerDependencies constructs a map of ContainerDependency objects for the BackendService +// ContainerDependencies returns a map of ContainerDependency objects for the BackendService // including dependencies for its main container, any logging sidecar, and additional sidecars. func (s *BackendService) ContainerDependencies() map[string]ContainerDependency { return containerDependencies(aws.StringValue(s.Name), s.ImageConfig.Image, s.Logging, s.Sidecars) diff --git a/internal/pkg/manifest/job.go b/internal/pkg/manifest/job.go index a4502fe3a8f..fdd1b523dd9 100644 --- a/internal/pkg/manifest/job.go +++ b/internal/pkg/manifest/job.go @@ -159,7 +159,7 @@ func (j *ScheduledJob) EnvFiles() map[string]string { return envFiles(j.Name, j.TaskConfig, j.Logging, j.Sidecars) } -// ContainerDependencies constructs a map of ContainerDependency objects for ScheduledJob +// ContainerDependencies returns a map of ContainerDependency objects for ScheduledJob // including dependencies for its main container, any logging sidecar, and additional sidecars. func (s *ScheduledJob) ContainerDependencies() map[string]ContainerDependency { return containerDependencies(aws.StringValue(s.Name), s.ImageConfig.Image, s.Logging, s.Sidecars) diff --git a/internal/pkg/manifest/lb_web_svc.go b/internal/pkg/manifest/lb_web_svc.go index 9e0a436cbf3..1786c1c155d 100644 --- a/internal/pkg/manifest/lb_web_svc.go +++ b/internal/pkg/manifest/lb_web_svc.go @@ -214,7 +214,7 @@ func (s *LoadBalancedWebService) EnvFiles() map[string]string { return envFiles(s.Name, s.TaskConfig, s.Logging, s.Sidecars) } -// ContainerDependencies constructs a map of ContainerDependency objects for the LoadBalancedWebService +// ContainerDependencies returns a map of ContainerDependency objects for the LoadBalancedWebService // including dependencies for its main container, any logging sidecar, and additional sidecars. func (s *LoadBalancedWebService) ContainerDependencies() map[string]ContainerDependency { return containerDependencies(aws.StringValue(s.Name), s.ImageConfig.Image, s.Logging, s.Sidecars) diff --git a/internal/pkg/manifest/worker_svc.go b/internal/pkg/manifest/worker_svc.go index db8aa7625a9..49ebfa37e8d 100644 --- a/internal/pkg/manifest/worker_svc.go +++ b/internal/pkg/manifest/worker_svc.go @@ -293,7 +293,7 @@ func (s *WorkerService) EnvFiles() map[string]string { return envFiles(s.Name, s.TaskConfig, s.Logging, s.Sidecars) } -// ContainerDependencies constructs a map of ContainerDependency objects for the WorkerService +// ContainerDependencies returns a map of ContainerDependency objects for the WorkerService // including dependencies for its main container, any logging sidecar, and additional sidecars. func (s *WorkerService) ContainerDependencies() map[string]ContainerDependency { return containerDependencies(aws.StringValue(s.Name), s.ImageConfig.Image, s.Logging, s.Sidecars) From 965b13312a5282542d1f88bd325a94bba7896988 Mon Sep 17 00:00:00 2001 From: Adithya Kolla Date: Tue, 14 Nov 2023 09:07:56 -0800 Subject: [PATCH 05/53] change doc comment --- internal/pkg/manifest/workload_ecs.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/internal/pkg/manifest/workload_ecs.go b/internal/pkg/manifest/workload_ecs.go index 672192d813e..7abb107fbb7 100644 --- a/internal/pkg/manifest/workload_ecs.go +++ b/internal/pkg/manifest/workload_ecs.go @@ -439,7 +439,8 @@ func buildArgs(contextDir string, buildArgs map[string]*DockerBuildArgs, sc map[ return buildArgs, nil } -// Dependencies defined for container startup and shutdown. +// ContainerDependency represents order of container startup and shutdown. +// Also indicates if a container is marked as essential or not. type ContainerDependency struct { IsEssential bool DependsOn DependsOn From c4379c3d3839d7990f3994af637d4d08d1f0f535 Mon Sep 17 00:00:00 2001 From: Adithya Kolla Date: Tue, 14 Nov 2023 11:46:35 -0800 Subject: [PATCH 06/53] change test case name --- internal/pkg/manifest/lb_web_svc_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/pkg/manifest/lb_web_svc_test.go b/internal/pkg/manifest/lb_web_svc_test.go index 1f3b6dc8d41..eb8561f3150 100644 --- a/internal/pkg/manifest/lb_web_svc_test.go +++ b/internal/pkg/manifest/lb_web_svc_test.go @@ -2844,7 +2844,7 @@ func TestLoadBalancedWebService_ContainerDependencies(t *testing.T) { wantedDependencies map[string]ContainerDependency wantedErr error }{ - "return main container and sidecar container build args": { + "return container dependencies of all containers": { in: &LoadBalancedWebService{ Workload: Workload{ Name: aws.String("mock-svc"), From feb441294708b535273f6e41bd5ecbe51aa606aa Mon Sep 17 00:00:00 2001 From: Adithya Kolla Date: Tue, 14 Nov 2023 13:55:10 -0800 Subject: [PATCH 07/53] Address fb from @ CaptainCarpensir --- internal/pkg/cli/deploy/workload.go | 11 +++++++++++ internal/pkg/cli/run_local.go | 13 +------------ internal/pkg/manifest/lb_web_svc_test.go | 4 ++++ 3 files changed, 16 insertions(+), 12 deletions(-) diff --git a/internal/pkg/cli/deploy/workload.go b/internal/pkg/cli/deploy/workload.go index 37b93b6f533..a6b6a8a7c4b 100644 --- a/internal/pkg/cli/deploy/workload.go +++ b/internal/pkg/cli/deploy/workload.go @@ -738,6 +738,17 @@ func envFiles(unmarshaledManifest interface{}) map[string]string { return nil } +func ContainerDependencies(unmarshaledManifest interface{}) map[string]manifest.ContainerDependency { + type containerDependency interface { + ContainerDependencies() map[string]manifest.ContainerDependency + } + mf, ok := unmarshaledManifest.(containerDependency) + if ok { + return mf.ContainerDependencies() + } + return nil +} + func (d *workloadDeployer) pushAddonsTemplateToS3Bucket() (string, error) { if d.addons == nil { return "", nil diff --git a/internal/pkg/cli/run_local.go b/internal/pkg/cli/run_local.go index 2b1f8ed7ff4..788345df564 100644 --- a/internal/pkg/cli/run_local.go +++ b/internal/pkg/cli/run_local.go @@ -516,7 +516,7 @@ func (o *runLocalOpts) prepareTask(ctx context.Context) (orchestrator.Task, erro } // TODO (Adi): Use this dependency order in orchestrator to start and stop containers. - containerDeps := o.containerDependencies(mft.Manifest()) + containerDeps := clideploy.ContainerDependencies(mft.Manifest()) for name, dep := range containerDeps { ctr, ok := task.Containers[name] if !ok { @@ -529,17 +529,6 @@ func (o *runLocalOpts) prepareTask(ctx context.Context) (orchestrator.Task, erro return task, nil } -func (o *runLocalOpts) containerDependencies(unmarshaledManifest interface{}) map[string]manifest.ContainerDependency { - type containerDependency interface { - ContainerDependencies() map[string]manifest.ContainerDependency - } - mf, ok := unmarshaledManifest.(containerDependency) - if ok { - return mf.ContainerDependencies() - } - return nil -} - func (o *runLocalOpts) watchLocalFiles(stopCh <-chan struct{}) (<-chan interface{}, <-chan error, error) { workspacePath := o.ws.Path() diff --git a/internal/pkg/manifest/lb_web_svc_test.go b/internal/pkg/manifest/lb_web_svc_test.go index eb8561f3150..a7d6c2e849d 100644 --- a/internal/pkg/manifest/lb_web_svc_test.go +++ b/internal/pkg/manifest/lb_web_svc_test.go @@ -2871,6 +2871,9 @@ func TestLoadBalancedWebService_ContainerDependencies(t *testing.T) { }, }, }, + Logging: Logging{ + ConfigFile: aws.String("mockConfigFile"), + }, }, }, wantedDependencies: map[string]ContainerDependency{ @@ -2890,6 +2893,7 @@ func TestLoadBalancedWebService_ContainerDependencies(t *testing.T) { "mock-svc": "start", }, }, + "firelens_log_router": {}, }, }, } From 4ac754de48f6a114799199c71fe04c794b9e5d30 Mon Sep 17 00:00:00 2001 From: Adithya Kolla Date: Tue, 14 Nov 2023 13:58:38 -0800 Subject: [PATCH 08/53] fix static check --- internal/pkg/cli/deploy/workload.go | 1 + 1 file changed, 1 insertion(+) diff --git a/internal/pkg/cli/deploy/workload.go b/internal/pkg/cli/deploy/workload.go index a6b6a8a7c4b..1be4d0d255c 100644 --- a/internal/pkg/cli/deploy/workload.go +++ b/internal/pkg/cli/deploy/workload.go @@ -738,6 +738,7 @@ func envFiles(unmarshaledManifest interface{}) map[string]string { return nil } +// ContainerDependencies returns a map of ContainerDependency objects from workload manifest. func ContainerDependencies(unmarshaledManifest interface{}) map[string]manifest.ContainerDependency { type containerDependency interface { ContainerDependencies() map[string]manifest.ContainerDependency From 18b2db1bcc96bbcc3b37ca2b5da72ab1c3296046 Mon Sep 17 00:00:00 2001 From: AdithyaKolla Date: Thu, 16 Nov 2023 11:26:44 -0800 Subject: [PATCH 09/53] add docker container methods --- .../pkg/docker/dockerengine/dockerengine.go | 121 ++++++- .../docker/dockerengine/dockerengine_test.go | 295 +++++++++++++++++- 2 files changed, 399 insertions(+), 17 deletions(-) diff --git a/internal/pkg/docker/dockerengine/dockerengine.go b/internal/pkg/docker/dockerengine/dockerengine.go index 4e5f0268e91..c18067a78a9 100644 --- a/internal/pkg/docker/dockerengine/dockerengine.go +++ b/internal/pkg/docker/dockerengine/dockerengine.go @@ -44,6 +44,20 @@ const ( credStoreECRLogin = "ecr-login" // set on `credStore` attribute in docker configuration file ) +// Health states of a Container. +const ( + noHealthcheck = "none" // Indicates there is no healthcheck + starting = "starting" // Starting indicates that the container is not yet ready + healthy = "healthy" // Healthy indicates that the container is running correctly + unhealthy = "unhealthy" // Unhealthy indicates that the container has a problem +) + +// State of a docker container. +const ( + containerStatusRunning = "running" + containerStatusExited = "exited" +) + // DockerCmdClient represents the docker client to interact with the server via external commands. type DockerCmdClient struct { runner Cmd @@ -339,13 +353,112 @@ func (c DockerCmdClient) Run(ctx context.Context, options *RunOptions) error { // 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) + if err != nil { + return false, err + } + switch state.Status { + case containerStatusRunning: + return true, nil + case containerStatusExited: + return false, &ErrContainerExited{name: name, exitcode: state.ExitCode} + } + return false, nil +} + +// IsContainerComplete returns true if a docker container exits with an exitcode. +func (c DockerCmdClient) IsContainerComplete(ctx context.Context, containerName string) (bool, error) { + state, err := c.containerState(ctx, containerName) + if err != nil { + return false, err + } + return state.Status == containerStatusExited, nil +} + +// IsContainerSuccess returns true if a docker container exits with exitcode 0. +func (c DockerCmdClient) IsContainerSuccess(ctx context.Context, containerName string) (bool, error) { + state, err := c.containerState(ctx, containerName) + if err != nil { + return false, err + } + return state.Status == containerStatusExited && state.ExitCode == 0, nil +} + +func (c DockerCmdClient) IsContainerHealthy(ctx context.Context, containerName string) (bool, error) { + state, err := c.containerState(ctx, containerName) + if err != nil { + return false, err + } + if state.Status == containerStatusExited { + return false, &ErrContainerExited{name: containerName, exitcode: state.ExitCode} + } + if state.Health == nil { + return false, fmt.Errorf("healthcheck is not configured for container %s", containerName) + } + switch state.Health.Status { + case healthy: + return true, nil + case starting: + return false, nil + case unhealthy: + return false, fmt.Errorf("container %q is %q", containerName, unhealthy) + case noHealthcheck: + return false, fmt.Errorf("healthcheck configuration is set to %q for container %s", unhealthy, containerName) + default: + return false, fmt.Errorf("container %s had unexpected health status %q", containerName, state.Health.Status) + } +} + +// ContainerState holds the status, exit code, and health information of a Docker container. +type ContainerState struct { + Status string `json:"Status"` + ExitCode int `json:"ExitCode"` + Health *struct { + Status string `json:"Status"` + } +} + +// containerState retrieves the current state of a specified Docker container. +// It returns a ContainerState object and any error encountered during retrieval. +func (d *DockerCmdClient) containerState(ctx context.Context, containerName string) (ContainerState, error) { + containerID, err := d.containerID(ctx, containerName) + if err != nil { + return ContainerState{}, err + } + if containerID == "" { + return ContainerState{}, nil + } + // return ContainerState{}, nil buf := &bytes.Buffer{} - if err := c.runner.RunWithContext(ctx, "docker", []string{"ps", "-q", "--filter", "name=" + name}, exec.Stdout(buf)); err != nil { - return false, fmt.Errorf("run docker ps: %w", err) + if err := d.runner.RunWithContext(ctx, "docker", []string{"inspect", "--format", "{{json .State}}", containerID}, exec.Stdout(buf)); err != nil { + return ContainerState{}, fmt.Errorf("run docker inspect: %w", err) + } + var containerState ContainerState + if err := json.Unmarshal([]byte(strings.TrimSpace(buf.String())), &containerState); err != nil { + return ContainerState{}, fmt.Errorf("unmarshal state of container %q:%w", containerName, err) } + return containerState, nil +} + +// containerID gets the ID of a Docker container by its name. +func (d *DockerCmdClient) containerID(ctx context.Context, containerName string) (string, error) { + buf := &bytes.Buffer{} + if err := d.runner.RunWithContext(ctx, "docker", []string{"ps", "-a", "-q", "--filter", "name=" + containerName}, exec.Stdout(buf)); err != nil { + return "", fmt.Errorf("run docker ps: %w", err) + } + return strings.TrimSpace(buf.String()), nil +} + +// ErrContainerExited represents an error when a Docker container has exited. +// It includes the container name and exit code in the error message. +type ErrContainerExited struct { + name string + exitcode int +} - output := strings.TrimSpace(buf.String()) - return output != "", nil +// ErrContainerExited represents docker container exited with an exitcode. +func (e *ErrContainerExited) Error() string { + return fmt.Sprintf("container %q exited with code %d", e.name, e.exitcode) } // Stop calls `docker stop` to stop a running container. diff --git a/internal/pkg/docker/dockerengine/dockerengine_test.go b/internal/pkg/docker/dockerengine/dockerengine_test.go index ec01f8f6751..9bfdc680060 100644 --- a/internal/pkg/docker/dockerengine/dockerengine_test.go +++ b/internal/pkg/docker/dockerengine/dockerengine_test.go @@ -805,37 +805,81 @@ func TestDockerCommand_IsContainerRunning(t *testing.T) { mockError := errors.New("some error") mockContainerName := "mockContainer" mockUnknownContainerName := "mockUnknownContainer" - var mockCmd *MockCmd tests := map[string]struct { - setupMocks func(controller *gomock.Controller) + setupMocks func(controller *gomock.Controller) *MockCmd inContainerName string - - wantedErr error + wantRunning bool + wantedErr error }{ "error running docker info": { inContainerName: mockUnknownContainerName, - setupMocks: func(controller *gomock.Controller) { - mockCmd = NewMockCmd(controller) - mockCmd.EXPECT().RunWithContext(gomock.Any(), "docker", []string{"ps", "-q", "--filter", "name=mockUnknownContainer"}, gomock.Any()).Return(mockError) + setupMocks: func(controller *gomock.Controller) *MockCmd { + mockCmd := NewMockCmd(controller) + mockCmd.EXPECT().RunWithContext(gomock.Any(), "docker", []string{"ps", "-a", "-q", "--filter", "name=mockUnknownContainer"}, gomock.Any()).Return(mockError) + return mockCmd }, - wantedErr: fmt.Errorf("run docker ps: some error"), }, "successfully check if the container is running": { inContainerName: mockContainerName, - setupMocks: func(controller *gomock.Controller) { - mockCmd = NewMockCmd(controller) - mockCmd.EXPECT().RunWithContext(gomock.Any(), "docker", []string{"ps", "-q", "--filter", "name=mockContainer"}, gomock.Any()).Return(nil) + setupMocks: func(controller *gomock.Controller) *MockCmd { + mockCmd := NewMockCmd(controller) + mockCmd.EXPECT().RunWithContext(gomock.Any(), "docker", []string{"ps", "-a", "-q", "--filter", "name=mockContainer"}, gomock.Any()).DoAndReturn(func(ctx context.Context, name string, args []string, opts ...exec.CmdOption) error { + cmd := &osexec.Cmd{} + for _, opt := range opts { + opt(cmd) + } + cmd.Stdout.Write([]byte("53d6417769ed")) + return nil + }) + mockCmd.EXPECT().RunWithContext(gomock.Any(), "docker", []string{"inspect", "--format", "{{json .State}}", "53d6417769ed"}, gomock.Any()).DoAndReturn(func(ctx context.Context, name string, args []string, opts ...exec.CmdOption) error { + cmd := &osexec.Cmd{} + for _, opt := range opts { + opt(cmd) + } + cmd.Stdout.Write([]byte(` +{ + "Status": "running" +}`)) + return nil + }) + return mockCmd + }, + }, + "return that container is exited": { + inContainerName: mockContainerName, + setupMocks: func(controller *gomock.Controller) *MockCmd { + mockCmd := NewMockCmd(controller) + mockCmd.EXPECT().RunWithContext(gomock.Any(), "docker", []string{"ps", "-a", "-q", "--filter", "name=mockContainer"}, gomock.Any()).DoAndReturn(func(ctx context.Context, name string, args []string, opts ...exec.CmdOption) error { + cmd := &osexec.Cmd{} + for _, opt := range opts { + opt(cmd) + } + cmd.Stdout.Write([]byte("53d6417769ed")) + return nil + }) + mockCmd.EXPECT().RunWithContext(gomock.Any(), "docker", []string{"inspect", "--format", "{{json .State}}", "53d6417769ed"}, gomock.Any()).DoAndReturn(func(ctx context.Context, name string, args []string, opts ...exec.CmdOption) error { + cmd := &osexec.Cmd{} + for _, opt := range opts { + opt(cmd) + } + cmd.Stdout.Write([]byte(` +{ + "Status": "exited" +}`)) + return nil + }) + return mockCmd }, + wantedErr: fmt.Errorf(`container "mockContainer" exited with code 0`), }, } for name, tc := range tests { t.Run(name, func(t *testing.T) { controller := gomock.NewController(t) - tc.setupMocks(controller) s := DockerCmdClient{ - runner: mockCmd, + runner: tc.setupMocks(controller), } _, err := s.IsContainerRunning(context.Background(), tc.inContainerName) if tc.wantedErr != nil { @@ -890,3 +934,228 @@ func TestDockerCommand_Exec(t *testing.T) { }) } } + +func TestDockerCommand_IsContainerHealthy(t *testing.T) { + tests := map[string]struct { + mockContainerName string + mockHealthStatus string + setupMocks func(*gomock.Controller) *MockCmd + wantHealthy bool + wantErr error + }{ + "unhealthy container": { + mockContainerName: "mockContainer", + mockHealthStatus: "unhealthy", + setupMocks: func(controller *gomock.Controller) *MockCmd { + mockCmd := NewMockCmd(controller) + mockCmd.EXPECT().RunWithContext(gomock.Any(), "docker", []string{"ps", "-a", "-q", "--filter", "name=mockContainer"}, gomock.Any()).DoAndReturn(func(ctx context.Context, name string, args []string, opts ...exec.CmdOption) error { + cmd := &osexec.Cmd{} + for _, opt := range opts { + opt(cmd) + } + cmd.Stdout.Write([]byte("53d6417769ed")) + return nil + }) + mockCmd.EXPECT().RunWithContext(gomock.Any(), "docker", []string{"inspect", "--format", "{{json .State}}", "53d6417769ed"}, gomock.Any()).DoAndReturn(func(ctx context.Context, name string, args []string, opts ...exec.CmdOption) error { + cmd := &osexec.Cmd{} + for _, opt := range opts { + opt(cmd) + } + cmd.Stdout.Write([]byte(` +{ + "Status": "running", + "Running": true, + "Health": { + "Status": "unhealthy" + } +}`)) + return nil + }) + return mockCmd + }, + wantHealthy: false, + wantErr: fmt.Errorf(`container "mockContainer" is "unhealthy"`), + }, + + "healthy container": { + mockContainerName: "mockContainer", + mockHealthStatus: "unhealthy", + setupMocks: func(controller *gomock.Controller) *MockCmd { + mockCmd := NewMockCmd(controller) + mockCmd.EXPECT().RunWithContext(gomock.Any(), "docker", []string{"ps", "-a", "-q", "--filter", "name=mockContainer"}, gomock.Any()).DoAndReturn(func(ctx context.Context, name string, args []string, opts ...exec.CmdOption) error { + cmd := &osexec.Cmd{} + for _, opt := range opts { + opt(cmd) + } + cmd.Stdout.Write([]byte("53d6417769ed")) + return nil + }) + mockCmd.EXPECT().RunWithContext(gomock.Any(), "docker", []string{"inspect", "--format", "{{json .State}}", "53d6417769ed"}, gomock.Any()).DoAndReturn(func(ctx context.Context, name string, args []string, opts ...exec.CmdOption) error { + cmd := &osexec.Cmd{} + for _, opt := range opts { + opt(cmd) + } + cmd.Stdout.Write([]byte(` +{ + "Status": "running", + "Running": true, + "Health": { + "Status": "healthy" + } +}`)) + return nil + }) + return mockCmd + }, + wantHealthy: true, + wantErr: nil, + }, + } + + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + s := DockerCmdClient{ + runner: tc.setupMocks(ctrl), // Correctly invoke the setupMocks function + } + + healthy, err := s.IsContainerHealthy(context.Background(), tc.mockContainerName) + require.Equal(t, tc.wantHealthy, healthy) + if tc.wantErr != nil { + require.EqualError(t, err, tc.wantErr.Error()) + } else { + require.NoError(t, err) + } + }) + } +} + +func TestDockerCommand_IsContainerComplete(t *testing.T) { + tests := map[string]struct { + mockContainerName string + mockHealthStatus string + setupMocks func(*gomock.Controller) *MockCmd + wantComplete bool + wantErr error + }{ + "container successfully complete": { + mockContainerName: "mockContainer", + mockHealthStatus: "unhealthy", + setupMocks: func(controller *gomock.Controller) *MockCmd { + mockCmd := NewMockCmd(controller) + mockCmd.EXPECT().RunWithContext(gomock.Any(), "docker", []string{"ps", "-a", "-q", "--filter", "name=mockContainer"}, gomock.Any()).DoAndReturn(func(ctx context.Context, name string, args []string, opts ...exec.CmdOption) error { + cmd := &osexec.Cmd{} + for _, opt := range opts { + opt(cmd) + } + cmd.Stdout.Write([]byte("53d6417769ed")) + return nil + }) + mockCmd.EXPECT().RunWithContext(gomock.Any(), "docker", []string{"inspect", "--format", "{{json .State}}", "53d6417769ed"}, gomock.Any()).DoAndReturn(func(ctx context.Context, name string, args []string, opts ...exec.CmdOption) error { + cmd := &osexec.Cmd{} + for _, opt := range opts { + opt(cmd) + } + cmd.Stdout.Write([]byte(` +{ + "Status": "exited", + "ExitCode": 143 +}`)) + return nil + }) + return mockCmd + }, + wantComplete: true, + }, + } + + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + s := DockerCmdClient{ + runner: tc.setupMocks(ctrl), // Correctly invoke the setupMocks function + } + + healthy, err := s.IsContainerComplete(context.Background(), tc.mockContainerName) + require.Equal(t, tc.wantComplete, healthy) + if tc.wantErr != nil { + require.EqualError(t, err, tc.wantErr.Error()) + } else { + require.NoError(t, err) + } + }) + } +} + +func TestDockerCommand_IsContainerSuccess(t *testing.T) { + tests := map[string]struct { + mockContainerName string + mockHealthStatus string + setupMocks func(*gomock.Controller) *MockCmd + wantSuccess bool + wantErr error + }{ + "container successfully complete": { + mockContainerName: "mockContainer", + mockHealthStatus: "unhealthy", + setupMocks: func(controller *gomock.Controller) *MockCmd { + mockCmd := NewMockCmd(controller) + mockCmd.EXPECT().RunWithContext(gomock.Any(), "docker", []string{"ps", "-a", "-q", "--filter", "name=mockContainer"}, gomock.Any()).DoAndReturn(func(ctx context.Context, name string, args []string, opts ...exec.CmdOption) error { + cmd := &osexec.Cmd{} + for _, opt := range opts { + opt(cmd) + } + cmd.Stdout.Write([]byte("53d6417769ed")) + return nil + }) + mockCmd.EXPECT().RunWithContext(gomock.Any(), "docker", []string{"inspect", "--format", "{{json .State}}", "53d6417769ed"}, gomock.Any()).DoAndReturn(func(ctx context.Context, name string, args []string, opts ...exec.CmdOption) error { + cmd := &osexec.Cmd{} + for _, opt := range opts { + opt(cmd) + } + cmd.Stdout.Write([]byte(` +{ + "Status": "exited", + "ExitCode": 0 +}`)) + return nil + }) + return mockCmd + }, + wantSuccess: true, + }, + "error when fetching container state": { + mockContainerName: "mockContainer", + mockHealthStatus: "unhealthy", + setupMocks: func(controller *gomock.Controller) *MockCmd { + mockCmd := NewMockCmd(controller) + mockCmd.EXPECT().RunWithContext(gomock.Any(), "docker", []string{"ps", "-a", "-q", "--filter", "name=mockContainer"}, gomock.Any()).Return(fmt.Errorf("some error")) + return mockCmd + }, + wantErr: fmt.Errorf("run docker ps: some error"), + }, + } + + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + s := DockerCmdClient{ + runner: tc.setupMocks(ctrl), // Correctly invoke the setupMocks function + } + + healthy, err := s.IsContainerSuccess(context.Background(), tc.mockContainerName) + require.Equal(t, tc.wantSuccess, healthy) + if tc.wantErr != nil { + require.EqualError(t, err, tc.wantErr.Error()) + } else { + require.NoError(t, err) + } + }) + } +} From 87fecebe08b11753c2d9dcfebbe8f269fadd3ad7 Mon Sep 17 00:00:00 2001 From: AdithyaKolla Date: Sat, 18 Nov 2023 17:57:28 -0800 Subject: [PATCH 10/53] modify graph package --- internal/pkg/graph/graph.go | 217 ++++++++++++++++++++++++++++++- internal/pkg/graph/graph_test.go | 77 ++++++++++- 2 files changed, 284 insertions(+), 10 deletions(-) diff --git a/internal/pkg/graph/graph.go b/internal/pkg/graph/graph.go index 9f94c3ebcdd..f8463dcd2ed 100644 --- a/internal/pkg/graph/graph.go +++ b/internal/pkg/graph/graph.go @@ -4,6 +4,13 @@ // Package graph provides functionality for directed graphs. package graph +import ( + "context" + "sync" + + "golang.org/x/sync/errgroup" +) + // vertexStatus denotes the visiting status of a vertex when running DFS in a graph. type vertexStatus int @@ -17,6 +24,8 @@ const ( type Graph[V comparable] struct { vertices map[V]neighbors[V] // Adjacency list for each vertex. inDegrees map[V]int // Number of incoming edges for each vertex. + status map[V]string // status of each vertex. + lock sync.Mutex // lock used to mutate graph data. } // Edge represents one edge of a directed graph. @@ -41,6 +50,15 @@ func New[V comparable](vertices ...V) *Graph[V] { } } +func NewWithStatus[V comparable](status string, vertices ...V) *Graph[V] { + g := New[V](vertices...) + g.status = make(map[V]string) + for vertex := range g.vertices { + g.status[vertex] = status + } + return g +} + // Neighbors returns the list of connected vertices from vtx. func (g *Graph[V]) Neighbors(vtx V) []V { neighbors, ok := g.vertices[vtx] @@ -135,6 +153,77 @@ func (g *Graph[V]) Roots() []V { return roots } +// updateStatus updates the status of a vertex. +func (g *Graph[V]) updateStatus(vertex V, status string) { + g.lock.Lock() + defer g.lock.Unlock() + g.status[vertex] = status +} + +// getStatus gets the status of a vertex. +func (g *Graph[V]) getStatus(vertex V) string { + g.lock.Lock() + defer g.lock.Unlock() + return g.status[vertex] +} + +// getLeaves returns the leaves of a given vertex. +func (g *Graph[V]) leaves() []V { + g.lock.Lock() + defer g.lock.Unlock() + var leaves []V + for vtx := range g.vertices { + if len(g.vertices[vtx]) == 0 { + leaves = append(leaves, vtx) + } + } + return leaves +} + +// getParents returns the parent vertices (incoming edges) of vertex. +func (g *Graph[V]) parents(vtx V) []V { + g.lock.Lock() + defer g.lock.Unlock() + var parents []V + for v, neighbors := range g.vertices { + if neighbors[vtx] { + parents = append(parents, v) + } + } + return parents +} + +// getChildren returns the child vertices (outgoing edges) of vertex. +func (g *Graph[V]) children(vtx V) []V { + g.lock.Lock() + defer g.lock.Unlock() + return g.Neighbors(vtx) +} + +// filterParents filters parents based on the vertex status. +func (g *Graph[V]) filterParents(vtx V, status string) []V { + parents := g.parents(vtx) + var filtered []V + for _, parent := range parents { + if g.getStatus(parent) == status { + filtered = append(filtered, parent) + } + } + return filtered +} + +// filterChildren filters children based on the vertex status. +func (g *Graph[V]) filterChildren(vtx V, status string) []V { + children := g.children(vtx) + var filtered []V + for _, child := range children { + if g.getStatus(child) == status { + filtered = append(filtered, child) + } + } + return filtered +} + func (g *Graph[V]) hasCycles(temp *findCycleTempVars[V], currVertex V) bool { temp.status[currVertex] = visiting for vertex := range g.vertices[currVertex] { @@ -196,12 +285,13 @@ func (alg *TopologicalSorter[V]) traverse(g *Graph[V]) { // // An example graph and their ranks is shown below to illustrate: // . -//├── a rank: 0 -//│ ├── c rank: 1 -//│ │ └── f rank: 2 -//│ └── d rank: 1 -//└── b rank: 0 -// └── e rank: 1 +// ├── a rank: 0 +// │ ├── c rank: 1 +// │ │ └── f rank: 2 +// │ └── d rank: 1 +// └── b rank: 0 +// +// └── e rank: 1 func TopologicalOrder[V comparable](digraph *Graph[V]) (*TopologicalSorter[V], error) { if vertices, isAcyclic := digraph.IsAcyclic(); !isAcyclic { return nil, &errCycle[V]{ @@ -215,3 +305,118 @@ func TopologicalOrder[V comparable](digraph *Graph[V]) (*TopologicalSorter[V], e topo.traverse(digraph) return topo, nil } + +/* +UpwardTraversal performs an upward traversal on the graph starting from leaves (nodes with no children) +and moving towards root nodes (nodes with children). +It applies the specified process function to each vertex in the graph, skipping vertices with the +"adjacentVertexSkipStatus" status, and continuing traversal until reaching vertices with the "requiredVertexStatus" status. +The traversal is concurrent and may process vertices in parallel. +Returns an error if the traversal encounters any issues, or nil if successful. +*/ +func (g *Graph[V]) UpwardTraversal(ctx context.Context, processVertexFunc func(context.Context, V) error, adjacentVertexSkipStatus, requiredVertexStatus string) error { + traversal := &graphTraversal[V]{ + mu: sync.Mutex{}, + seen: make(map[V]struct{}), + findBoundaryVertices: func(g *Graph[V]) []V { return g.leaves() }, + findAdjacentVertices: func(g *Graph[V], v V) []V { return g.parents(v) }, + filterVerticesByStatus: func(g *Graph[V], v V, status string) []V { return g.filterChildren(v, status) }, + requiredVertexStatus: requiredVertexStatus, + adjacentVertexSkipStatus: adjacentVertexSkipStatus, + processVertex: processVertexFunc, + } + return traversal.execute(ctx, g) +} + +/* +DownwardTraversal performs a downward traversal on the graph starting from root nodes (nodes with no parents) +and moving towards leaf nodes (nodes with parents). It applies the specified process function to each +vertex in the graph, skipping vertices with the "adjacentVertexSkipStatus" status, and continuing traversal +until reaching vertices with the "requiredVertexStatus" status. +The traversal is concurrent and may process vertices in parallel. +Returns an error if the traversal encounters any issues. +*/ +func (g *Graph[V]) DownwardTraversal(ctx context.Context, processVertexFunc func(context.Context, V) error, adjacentVertexSkipStatus, requiredVertexStatus string) error { + traversal := &graphTraversal[V]{ + mu: sync.Mutex{}, + seen: make(map[V]struct{}), + findBoundaryVertices: func(g *Graph[V]) []V { return g.Roots() }, + findAdjacentVertices: func(g *Graph[V], v V) []V { return g.children(v) }, + filterVerticesByStatus: func(g *Graph[V], v V, status string) []V { return g.filterParents(v, status) }, + requiredVertexStatus: requiredVertexStatus, + adjacentVertexSkipStatus: adjacentVertexSkipStatus, + processVertex: processVertexFunc, + } + return traversal.execute(ctx, g) +} + +type graphTraversal[V comparable] struct { + mu sync.Mutex + seen map[V]struct{} + findBoundaryVertices func(*Graph[V]) []V + findAdjacentVertices func(*Graph[V], V) []V + filterVerticesByStatus func(*Graph[V], V, string) []V + requiredVertexStatus string + adjacentVertexSkipStatus string + processVertex func(context.Context, V) error +} + +func (t *graphTraversal[V]) execute(ctx context.Context, graph *Graph[V]) error { + vertexCount := len(graph.vertices) + if vertexCount == 0 { + return nil + } + eg, ctx := errgroup.WithContext(ctx) + vertexCh := make(chan V, vertexCount) + defer close(vertexCh) + + processVertices := func(ctx context.Context, graph *Graph[V], eg *errgroup.Group, vertices []V, vertexCh chan V) { + for _, vertex := range vertices { + vertex := vertex + // Delay processing this vertex if any of its dependent vertices are yet to be processed. + if len(t.filterVerticesByStatus(graph, vertex, t.adjacentVertexSkipStatus)) != 0 { + continue + } + if !t.markAsSeen(vertex) { + // Skip this vertex if it's already been processed by another routine. + continue + } + eg.Go(func() error { + if err := t.processVertex(ctx, vertex); err != nil { + return err + } + // Assign new status to the vertex upon successful processing. + graph.updateStatus(vertex, t.requiredVertexStatus) + vertexCh <- vertex + return nil + }) + } + } + + eg.Go(func() error { + for { + select { + case <-ctx.Done(): + return nil + case vertex := <-vertexCh: + vertexCount-- + if vertexCount == 0 { + return nil + } + processVertices(ctx, graph, eg, t.findAdjacentVertices(graph, vertex), vertexCh) + } + } + }) + processVertices(ctx, graph, eg, t.findBoundaryVertices(graph), vertexCh) + return eg.Wait() +} + +func (t *graphTraversal[V]) markAsSeen(vertex V) bool { + t.mu.Lock() + defer t.mu.Unlock() + if _, seen := t.seen[vertex]; seen { + return false + } + t.seen[vertex] = struct{}{} + return true +} diff --git a/internal/pkg/graph/graph_test.go b/internal/pkg/graph/graph_test.go index 71121718314..736a73c5d52 100644 --- a/internal/pkg/graph/graph_test.go +++ b/internal/pkg/graph/graph_test.go @@ -4,6 +4,7 @@ package graph import ( + "context" "strings" "testing" @@ -123,13 +124,13 @@ func TestGraph_Remove(t *testing.T) { func TestGraph_IsAcyclic(t *testing.T) { testCases := map[string]struct { - graph Graph[string] + graph *Graph[string] isAcyclic bool cycle []string }{ "small non acyclic graph": { - graph: Graph[string]{ + graph: &Graph[string]{ vertices: map[string]neighbors[string]{ "A": {"B": true, "C": true}, "B": {"A": true}, @@ -140,7 +141,7 @@ func TestGraph_IsAcyclic(t *testing.T) { cycle: []string{"A", "B"}, }, "non acyclic": { - graph: Graph[string]{ + graph: &Graph[string]{ vertices: map[string]neighbors[string]{ "K": {"F": true}, "A": {"B": true, "C": true}, @@ -155,7 +156,7 @@ func TestGraph_IsAcyclic(t *testing.T) { cycle: []string{"A", "G", "E", "B"}, }, "acyclic": { - graph: Graph[string]{ + graph: &Graph[string]{ vertices: map[string]neighbors[string]{ "A": {"B": true, "C": true}, "B": {"D": true}, @@ -369,3 +370,71 @@ func TestTopologicalOrder(t *testing.T) { }) } } + +func buildGraphWithSingleParent() *Graph[string] { + vertices := []string{"A", "B", "C", "D"} + graph := NewWithStatus[string]("started", vertices...) + graph.Add(Edge[string]{From: "D", To: "C"}) // D -> C + graph.Add(Edge[string]{From: "C", To: "B"}) // C -> B + graph.Add(Edge[string]{From: "B", To: "A"}) // B -> A + return graph +} + +func TestTraverseInDependencyOrder(t *testing.T) { + t.Run("graph with single root vertex", func(t *testing.T) { + graph := buildGraphWithSingleParent() + var visited []string + processFn := func(ctx context.Context, v string) error { + visited = append(visited, v) + return nil + } + err := graph.UpwardTraversal(context.Background(), processFn, "started", "stopped") + require.NoError(t, err) + expected := []string{"A", "B", "C", "D"} + require.Equal(t, expected, visited) + }) + t.Run("graph with multiple parents and boundary nodes", func(t *testing.T) { + vertices := []string{"A", "B", "C", "D"} + graph := NewWithStatus[string]("started", vertices...) + graph.Add(Edge[string]{From: "A", To: "C"}) + graph.Add(Edge[string]{From: "A", To: "D"}) + graph.Add(Edge[string]{From: "B", To: "D"}) + vtxChan := make(chan string, 4) + seen := make(map[string]int) + done := make(chan struct{}) + go func() { + for _, vtx := range vertices { + seen[vtx]++ + } + done <- struct{}{} + }() + + err := graph.DownwardTraversal(context.Background(), func(ctx context.Context, vertice string) error { + vtxChan <- vertice + return nil + }, "started", "stopped") + require.NoError(t, err, "Error during iteration") + close(vtxChan) + <-done + + require.Len(t, seen, 4) + for vtx, count := range seen { + require.Equal(t, 1, count, "%s", vtx) + } + }) +} + +func TestTraverseInReverseDependencyOrder(t *testing.T) { + t.Run("Graph with single root vertex", func(t *testing.T) { + graph := buildGraphWithSingleParent() + var visited []string + processFn := func(ctx context.Context, v string) error { + visited = append(visited, v) + return nil + } + err := graph.DownwardTraversal(context.Background(), processFn, "started", "stopped") + require.NoError(t, err) + expected := []string{"D", "C", "B", "A"} + require.Equal(t, expected, visited) + }) +} From 8176525ab7f2dbbfce70025a6187893a38e55e74 Mon Sep 17 00:00:00 2001 From: AdithyaKolla Date: Sat, 18 Nov 2023 20:50:48 -0800 Subject: [PATCH 11/53] implement dockerengine chnages and mocks --- internal/pkg/cli/interfaces.go | 2 + .../pkg/docker/dockerengine/dockerengine.go | 36 +++---- .../docker/dockerengine/dockerengine_test.go | 93 +++++++------------ .../dockerenginetest/dockerenginetest.go | 22 ++++- 4 files changed, 63 insertions(+), 90 deletions(-) diff --git a/internal/pkg/cli/interfaces.go b/internal/pkg/cli/interfaces.go index 2d72598b6ea..797704f9719 100644 --- a/internal/pkg/cli/interfaces.go +++ b/internal/pkg/cli/interfaces.go @@ -712,6 +712,8 @@ type dockerEngineRunner interface { Rm(string) error Build(context.Context, *dockerengine.BuildArguments, io.Writer) error Exec(ctx context.Context, container string, out io.Writer, cmd string, args ...string) error + IsContainerCompleteOrSuccess(ctx context.Context, containerName string) (bool, int, error) + IsContainerHealthy(ctx context.Context, containerName string) (bool, error) } type workloadStackGenerator interface { diff --git a/internal/pkg/docker/dockerengine/dockerengine.go b/internal/pkg/docker/dockerengine/dockerengine.go index c18067a78a9..1fcf72e78bc 100644 --- a/internal/pkg/docker/dockerengine/dockerengine.go +++ b/internal/pkg/docker/dockerengine/dockerengine.go @@ -263,7 +263,6 @@ func (c DockerCmdClient) Push(ctx context.Context, uri string, w io.Writer, tags func (in *RunOptions) generateRunArguments() []string { args := []string{"run"} - args = append(args, "--rm") if in.ContainerName != "" { args = append(args, "--name", in.ContainerName) @@ -366,24 +365,16 @@ func (c DockerCmdClient) IsContainerRunning(ctx context.Context, name string) (b return false, nil } -// IsContainerComplete returns true if a docker container exits with an exitcode. -func (c DockerCmdClient) IsContainerComplete(ctx context.Context, containerName string) (bool, error) { +// IsContainerCompleteOrSuccess returns true if a docker container exits with an exitcode. +func (c DockerCmdClient) IsContainerCompleteOrSuccess(ctx context.Context, containerName string) (bool, int, error) { state, err := c.containerState(ctx, containerName) if err != nil { - return false, err - } - return state.Status == containerStatusExited, nil -} - -// IsContainerSuccess returns true if a docker container exits with exitcode 0. -func (c DockerCmdClient) IsContainerSuccess(ctx context.Context, containerName string) (bool, error) { - state, err := c.containerState(ctx, containerName) - if err != nil { - return false, err + return false, 0, err } - return state.Status == containerStatusExited && state.ExitCode == 0, nil + return state.Status == containerStatusExited, state.ExitCode, nil } +// IsContainerHealthy returns true if a container health state is healthy. func (c DockerCmdClient) IsContainerHealthy(ctx context.Context, containerName string) (bool, error) { state, err := c.containerState(ctx, containerName) if err != nil { @@ -403,7 +394,7 @@ func (c DockerCmdClient) IsContainerHealthy(ctx context.Context, containerName s case unhealthy: return false, fmt.Errorf("container %q is %q", containerName, unhealthy) case noHealthcheck: - return false, fmt.Errorf("healthcheck configuration is set to %q for container %s", unhealthy, containerName) + return false, fmt.Errorf("healthcheck configuration is set to %q for container %s", noHealthcheck, containerName) default: return false, fmt.Errorf("container %s had unexpected health status %q", containerName, state.Health.Status) } @@ -428,7 +419,6 @@ func (d *DockerCmdClient) containerState(ctx context.Context, containerName stri if containerID == "" { return ContainerState{}, nil } - // return ContainerState{}, nil buf := &bytes.Buffer{} if err := d.runner.RunWithContext(ctx, "docker", []string{"inspect", "--format", "{{json .State}}", containerID}, exec.Stdout(buf)); err != nil { return ContainerState{}, fmt.Errorf("run docker inspect: %w", err) @@ -572,13 +562,13 @@ func PlatformString(os, arch string) string { func parseCredFromDockerConfig(config []byte) (*dockerConfig, error) { /* - Sample docker config file - { - "credsStore" : "ecr-login", - "credHelpers": { - "dummyaccountId.dkr.ecr.region.amazonaws.com": "ecr-login" - } - } + Sample docker config file + { + "credsStore" : "ecr-login", + "credHelpers": { + "dummyaccountId.dkr.ecr.region.amazonaws.com": "ecr-login" + } + } */ cred := dockerConfig{} err := json.Unmarshal(config, &cred) diff --git a/internal/pkg/docker/dockerengine/dockerengine_test.go b/internal/pkg/docker/dockerengine/dockerengine_test.go index 9bfdc680060..0cf852edfd3 100644 --- a/internal/pkg/docker/dockerengine/dockerengine_test.go +++ b/internal/pkg/docker/dockerengine/dockerengine_test.go @@ -680,7 +680,6 @@ func TestDockerCommand_Run(t *testing.T) { setupMocks: func(controller *gomock.Controller) { mockCmd = NewMockCmd(controller) mockCmd.EXPECT().RunWithContext(gomock.Any(), "docker", []string{"run", - "--rm", "--name", mockPauseContainer, "mockImageUri", "sleep", "infinity"}, gomock.Any(), gomock.Any(), gomock.Any()).Return(mockError) @@ -695,7 +694,6 @@ func TestDockerCommand_Run(t *testing.T) { setupMocks: func(controller *gomock.Controller) { mockCmd = NewMockCmd(controller) mockCmd.EXPECT().RunWithContext(gomock.Any(), "docker", gomock.InAnyOrder([]string{"run", - "--rm", "--name", mockPauseContainer, "--publish", "8080:8080", "--publish", "8081:8081", @@ -712,7 +710,6 @@ func TestDockerCommand_Run(t *testing.T) { setupMocks: func(controller *gomock.Controller) { mockCmd = NewMockCmd(controller) mockCmd.EXPECT().RunWithContext(gomock.Any(), "docker", gomock.InAnyOrder([]string{"run", - "--rm", "--name", mockContainerName, "--network", "container:pauseContainer", "--env", "DB_PASSWORD=mysecretPassword", @@ -733,7 +730,6 @@ func TestDockerCommand_Run(t *testing.T) { setupMocks: func(controller *gomock.Controller) { mockCmd = NewMockCmd(controller) mockCmd.EXPECT().RunWithContext(gomock.Any(), "docker", gomock.InAnyOrder([]string{"run", - "--rm", "--name", mockContainerName, "--network", "container:pauseContainer", "--env", "DB_PASSWORD=mysecretPassword", @@ -866,7 +862,7 @@ func TestDockerCommand_IsContainerRunning(t *testing.T) { } cmd.Stdout.Write([]byte(` { - "Status": "exited" + "Status": "exited" }`)) return nil }) @@ -963,11 +959,11 @@ func TestDockerCommand_IsContainerHealthy(t *testing.T) { } cmd.Stdout.Write([]byte(` { - "Status": "running", - "Running": true, - "Health": { - "Status": "unhealthy" - } + "Status": "running", + "Running": true, + "Health": { + "Status": "unhealthy" + } }`)) return nil }) @@ -997,11 +993,11 @@ func TestDockerCommand_IsContainerHealthy(t *testing.T) { } cmd.Stdout.Write([]byte(` { - "Status": "running", - "Running": true, - "Health": { - "Status": "healthy" - } + "Status": "running", + "Running": true, + "Health": { + "Status": "healthy" + } }`)) return nil }) @@ -1021,8 +1017,8 @@ func TestDockerCommand_IsContainerHealthy(t *testing.T) { runner: tc.setupMocks(ctrl), // Correctly invoke the setupMocks function } - healthy, err := s.IsContainerHealthy(context.Background(), tc.mockContainerName) - require.Equal(t, tc.wantHealthy, healthy) + expected, err := s.IsContainerHealthy(context.Background(), tc.mockContainerName) + require.Equal(t, tc.wantHealthy, expected) if tc.wantErr != nil { require.EqualError(t, err, tc.wantErr.Error()) } else { @@ -1032,13 +1028,14 @@ func TestDockerCommand_IsContainerHealthy(t *testing.T) { } } -func TestDockerCommand_IsContainerComplete(t *testing.T) { +func TestDockerCommand_IsContainerCompleteOrSuccess(t *testing.T) { tests := map[string]struct { - mockContainerName string - mockHealthStatus string - setupMocks func(*gomock.Controller) *MockCmd - wantComplete bool - wantErr error + mockContainerName string + mockHealthStatus string + setupMocks func(*gomock.Controller) *MockCmd + wantCompleteOrSuccess bool + wantExitCode int + wantErr error }{ "container successfully complete": { mockContainerName: "mockContainer", @@ -1060,46 +1057,17 @@ func TestDockerCommand_IsContainerComplete(t *testing.T) { } cmd.Stdout.Write([]byte(` { - "Status": "exited", - "ExitCode": 143 + "Status": "exited", + "ExitCode": 143 }`)) return nil }) return mockCmd }, - wantComplete: true, + wantCompleteOrSuccess: true, + wantExitCode: 143, }, - } - - for name, tc := range tests { - t.Run(name, func(t *testing.T) { - ctrl := gomock.NewController(t) - defer ctrl.Finish() - - s := DockerCmdClient{ - runner: tc.setupMocks(ctrl), // Correctly invoke the setupMocks function - } - - healthy, err := s.IsContainerComplete(context.Background(), tc.mockContainerName) - require.Equal(t, tc.wantComplete, healthy) - if tc.wantErr != nil { - require.EqualError(t, err, tc.wantErr.Error()) - } else { - require.NoError(t, err) - } - }) - } -} - -func TestDockerCommand_IsContainerSuccess(t *testing.T) { - tests := map[string]struct { - mockContainerName string - mockHealthStatus string - setupMocks func(*gomock.Controller) *MockCmd - wantSuccess bool - wantErr error - }{ - "container successfully complete": { + "container successfully success": { mockContainerName: "mockContainer", mockHealthStatus: "unhealthy", setupMocks: func(controller *gomock.Controller) *MockCmd { @@ -1119,14 +1087,14 @@ func TestDockerCommand_IsContainerSuccess(t *testing.T) { } cmd.Stdout.Write([]byte(` { - "Status": "exited", - "ExitCode": 0 + "Status": "exited", + "ExitCode": 0 }`)) return nil }) return mockCmd }, - wantSuccess: true, + wantCompleteOrSuccess: true, }, "error when fetching container state": { mockContainerName: "mockContainer", @@ -1149,8 +1117,9 @@ func TestDockerCommand_IsContainerSuccess(t *testing.T) { runner: tc.setupMocks(ctrl), // Correctly invoke the setupMocks function } - healthy, err := s.IsContainerSuccess(context.Background(), tc.mockContainerName) - require.Equal(t, tc.wantSuccess, healthy) + expected, expectedCode, err := s.IsContainerCompleteOrSuccess(context.Background(), tc.mockContainerName) + require.Equal(t, tc.wantCompleteOrSuccess, expected) + require.Equal(t, tc.wantExitCode, expectedCode) if tc.wantErr != nil { require.EqualError(t, err, tc.wantErr.Error()) } else { diff --git a/internal/pkg/docker/dockerengine/dockerenginetest/dockerenginetest.go b/internal/pkg/docker/dockerengine/dockerenginetest/dockerenginetest.go index 39da68ee741..29eb5dcd118 100644 --- a/internal/pkg/docker/dockerengine/dockerenginetest/dockerenginetest.go +++ b/internal/pkg/docker/dockerengine/dockerenginetest/dockerenginetest.go @@ -12,11 +12,23 @@ import ( // Double is a test double for dockerengine.DockerCmdClient type Double struct { - StopFn func(context.Context, string) error - IsContainerRunningFn func(context.Context, string) (bool, error) - RunFn func(context.Context, *dockerengine.RunOptions) error - BuildFn func(context.Context, *dockerengine.BuildArguments, io.Writer) error - ExecFn func(context.Context, string, io.Writer, string, ...string) error + StopFn func(context.Context, string) error + IsContainerRunningFn func(context.Context, string) (bool, error) + RunFn func(context.Context, *dockerengine.RunOptions) error + BuildFn func(context.Context, *dockerengine.BuildArguments, io.Writer) error + ExecFn func(context.Context, string, io.Writer, string, ...string) error + IsContainerHealthyFn func(ctx context.Context, containerName string) (bool, error) + IsContainerRunningOrHealthyFn func(ctx context.Context, containerName string) (bool, int, error) +} + +// IsContainerCompleteOrSuccess implements orchestrator.DockerEngine. +func (*Double) IsContainerCompleteOrSuccess(ctx context.Context, containerName string) (bool, int, error) { + panic("unimplemented") +} + +// IsContainerHealthy implements orchestrator.DockerEngine. +func (*Double) IsContainerHealthy(ctx context.Context, containerName string) (bool, error) { + panic("unimplemented") } // Stop calls the stubbed function. From e923d352cdf6af5931bda5850bb6bd275a416a8c Mon Sep 17 00:00:00 2001 From: AdithyaKolla Date: Sat, 18 Nov 2023 21:04:15 -0800 Subject: [PATCH 12/53] add run local changes --- internal/pkg/cli/mocks/mock_interfaces.go | 31 +++++++++++++++++++++ internal/pkg/cli/run_local.go | 33 ++++++++++++++++++++--- internal/pkg/cli/run_local_test.go | 16 ++++++++++- 3 files changed, 75 insertions(+), 5 deletions(-) diff --git a/internal/pkg/cli/mocks/mock_interfaces.go b/internal/pkg/cli/mocks/mock_interfaces.go index 1e0ce2d6fc6..1448ce78fce 100644 --- a/internal/pkg/cli/mocks/mock_interfaces.go +++ b/internal/pkg/cli/mocks/mock_interfaces.go @@ -7727,6 +7727,37 @@ func (mr *MockdockerEngineRunnerMockRecorder) Exec(ctx, container, out, cmd inte return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Exec", reflect.TypeOf((*MockdockerEngineRunner)(nil).Exec), varargs...) } +// IsContainerCompleteOrSuccess mocks base method. +func (m *MockdockerEngineRunner) IsContainerCompleteOrSuccess(ctx context.Context, containerName string) (bool, int, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "IsContainerCompleteOrSuccess", ctx, containerName) + ret0, _ := ret[0].(bool) + ret1, _ := ret[1].(int) + ret2, _ := ret[2].(error) + return ret0, ret1, ret2 +} + +// IsContainerCompleteOrSuccess indicates an expected call of IsContainerCompleteOrSuccess. +func (mr *MockdockerEngineRunnerMockRecorder) IsContainerCompleteOrSuccess(ctx, containerName interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "IsContainerCompleteOrSuccess", reflect.TypeOf((*MockdockerEngineRunner)(nil).IsContainerCompleteOrSuccess), ctx, containerName) +} + +// IsContainerHealthy mocks base method. +func (m *MockdockerEngineRunner) IsContainerHealthy(ctx context.Context, containerName string) (bool, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "IsContainerHealthy", ctx, containerName) + ret0, _ := ret[0].(bool) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// IsContainerHealthy indicates an expected call of IsContainerHealthy. +func (mr *MockdockerEngineRunnerMockRecorder) IsContainerHealthy(ctx, containerName interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "IsContainerHealthy", reflect.TypeOf((*MockdockerEngineRunner)(nil).IsContainerHealthy), ctx, containerName) +} + // IsContainerRunning mocks base method. func (m *MockdockerEngineRunner) IsContainerRunning(arg0 context.Context, arg1 string) (bool, error) { m.ctrl.T.Helper() diff --git a/internal/pkg/cli/run_local.go b/internal/pkg/cli/run_local.go index 788345df564..a41b7e60994 100644 --- a/internal/pkg/cli/run_local.go +++ b/internal/pkg/cli/run_local.go @@ -436,6 +436,8 @@ func (o *runLocalOpts) getTask(ctx context.Context) (orchestrator.Task, error) { return orchestrator.Task{}, fmt.Errorf("get env vars: %w", err) } + containerDeps := o.getContainerDependencies(td) + task := orchestrator.Task{ Containers: make(map[string]orchestrator.ContainerDefinition, len(td.ContainerDefinitions)), } @@ -451,10 +453,12 @@ func (o *runLocalOpts) getTask(ctx context.Context) (orchestrator.Task, error) { for _, ctr := range td.ContainerDefinitions { name := aws.StringValue(ctr.Name) def := orchestrator.ContainerDefinition{ - ImageURI: aws.StringValue(ctr.Image), - EnvVars: envVars[name].EnvVars(), - Secrets: envVars[name].Secrets(), - Ports: make(map[string]string, len(ctr.PortMappings)), + ImageURI: aws.StringValue(ctr.Image), + EnvVars: envVars[name].EnvVars(), + Secrets: envVars[name].Secrets(), + Ports: make(map[string]string, len(ctr.PortMappings)), + IsEssential: containerDeps[name].isEssential, + DependsOn: containerDeps[name].dependsOn, } for _, port := range ctr.PortMappings { @@ -516,6 +520,7 @@ func (o *runLocalOpts) prepareTask(ctx context.Context) (orchestrator.Task, erro } // TODO (Adi): Use this dependency order in orchestrator to start and stop containers. + // replace container dependencies with the local dependencies from manifest. containerDeps := clideploy.ContainerDependencies(mft.Manifest()) for name, dep := range containerDeps { ctr, ok := task.Containers[name] @@ -813,6 +818,26 @@ func (o *runLocalOpts) getSecret(ctx context.Context, valueFrom string) (string, return getter.GetSecretValue(ctx, valueFrom) } +type containerDependency struct { + isEssential bool + dependsOn map[string]string +} + +func (o *runLocalOpts) getContainerDependencies(taskDef *awsecs.TaskDefinition) map[string]containerDependency { + dependencies := make(map[string]containerDependency, len(taskDef.ContainerDefinitions)) + for _, ctr := range taskDef.ContainerDefinitions { + dep := containerDependency{ + isEssential: aws.BoolValue(ctr.Essential), + dependsOn: make(map[string]string), + } + for _, containerDep := range ctr.DependsOn { + dep.dependsOn[aws.StringValue(containerDep.ContainerName)] = strings.ToLower(aws.StringValue(containerDep.Condition)) + } + dependencies[aws.StringValue(ctr.Name)] = dep + } + return dependencies +} + type hostDiscoverer struct { ecs ecsClient app string diff --git a/internal/pkg/cli/run_local_test.go b/internal/pkg/cli/run_local_test.go index b300a50f22f..467fda1b241 100644 --- a/internal/pkg/cli/run_local_test.go +++ b/internal/pkg/cli/run_local_test.go @@ -292,9 +292,17 @@ func TestRunLocalOpts_Execute(t *testing.T) { HostPort: aws.Int64(9999), }, }, + Essential: aws.Bool(true), + DependsOn: []*sdkecs.ContainerDependency{ + { + Condition: aws.String("START"), + ContainerName: aws.String("bar"), + }, + }, }, { - Name: aws.String("bar"), + Name: aws.String("bar"), + Essential: aws.Bool(true), Environment: []*sdkecs.KeyValuePair{ { Name: aws.String("BAR_VAR"), @@ -388,6 +396,10 @@ func TestRunLocalOpts_Execute(t *testing.T) { "80": "8080", "999": "9999", }, + IsEssential: true, + DependsOn: map[string]string{ + "bar": "start", + }, }, "bar": { ImageURI: "image2", @@ -404,6 +416,8 @@ func TestRunLocalOpts_Execute(t *testing.T) { "777": "7777", "10000": "10000", }, + IsEssential: true, + DependsOn: map[string]string{}, }, }, } From c0ff69dba56ceff2733ac32f8cc777e5a4628b69 Mon Sep 17 00:00:00 2001 From: AdithyaKolla Date: Sat, 18 Nov 2023 22:36:03 -0800 Subject: [PATCH 13/53] add WithStatus to graph package --- internal/pkg/deploy/pipeline.go | 5 +++-- internal/pkg/graph/graph.go | 24 ++++++++++++++++-------- internal/pkg/graph/graph_test.go | 28 ++++++++++++++-------------- internal/pkg/manifest/validate.go | 2 +- 4 files changed, 34 insertions(+), 25 deletions(-) diff --git a/internal/pkg/deploy/pipeline.go b/internal/pkg/deploy/pipeline.go index bee9a60c5e1..355951a69a0 100644 --- a/internal/pkg/deploy/pipeline.go +++ b/internal/pkg/deploy/pipeline.go @@ -8,13 +8,14 @@ package deploy import ( "errors" "fmt" - "gopkg.in/yaml.v3" "path" "path/filepath" "regexp" "sort" "strings" + "gopkg.in/yaml.v3" + "github.com/aws/aws-sdk-go/aws/arn" "github.com/aws/copilot-cli/internal/pkg/config" "github.com/aws/copilot-cli/internal/pkg/graph" @@ -780,7 +781,7 @@ func (stg *PipelineStage) buildActionsGraph(rankables []actionGraphNode) *graph. for _, r := range rankables { names = append(names, r.name) } - digraph := graph.New(names...) + digraph := graph.New(names) for _, r := range rankables { if r.depends_on == nil { diff --git a/internal/pkg/graph/graph.go b/internal/pkg/graph/graph.go index f8463dcd2ed..3b6dcdcac9e 100644 --- a/internal/pkg/graph/graph.go +++ b/internal/pkg/graph/graph.go @@ -37,26 +37,34 @@ type Edge[V comparable] struct { type neighbors[V comparable] map[V]bool // New initiates a new Graph. -func New[V comparable](vertices ...V) *Graph[V] { +func New[V comparable](vertices []V, opts ...GraphOption[V]) *Graph[V] { adj := make(map[V]neighbors[V]) inDegrees := make(map[V]int) for _, vertex := range vertices { adj[vertex] = make(neighbors[V]) inDegrees[vertex] = 0 } - return &Graph[V]{ + g := &Graph[V]{ vertices: adj, inDegrees: inDegrees, } + for _, opt := range opts { + opt(g) + } + return g } -func NewWithStatus[V comparable](status string, vertices ...V) *Graph[V] { - g := New[V](vertices...) - g.status = make(map[V]string) - for vertex := range g.vertices { - g.status[vertex] = status +// GraphOption allows you to initialize Graph with additional properties. +type GraphOption[V comparable] func(g *Graph[V]) + +// WithStatus sets the status of each vertex in the Graph. +func WithStatus[V comparable](status string) func(g *Graph[V]) { + return func(g *Graph[V]) { + g.status = make(map[V]string) + for vertex := range g.vertices { + g.status[vertex] = status + } } - return g } // Neighbors returns the list of connected vertices from vtx. diff --git a/internal/pkg/graph/graph_test.go b/internal/pkg/graph/graph_test.go index 736a73c5d52..4c857af9a02 100644 --- a/internal/pkg/graph/graph_test.go +++ b/internal/pkg/graph/graph_test.go @@ -14,7 +14,7 @@ import ( func TestGraph_Add(t *testing.T) { t.Run("success", func(t *testing.T) { // GIVEN - graph := New[string]() + graph := New[string]([]string{}) // WHEN // A <-> B @@ -45,7 +45,7 @@ func TestGraph_InDegree(t *testing.T) { wanted map[rune]int }{ "should return 0 for nodes that don't exist in the graph": { - graph: New[rune](), + graph: New[rune]([]rune{}), wanted: map[rune]int{ 'a': 0, @@ -53,7 +53,7 @@ func TestGraph_InDegree(t *testing.T) { }, "should return number of incoming edges for complex graph": { graph: func() *Graph[rune] { - g := New[rune]() + g := New[rune]([]rune{}) g.Add(Edge[rune]{'a', 'b'}) g.Add(Edge[rune]{'b', 'a'}) g.Add(Edge[rune]{'a', 'c'}) @@ -89,7 +89,7 @@ func TestGraph_Remove(t *testing.T) { }{ "edge deletion should be idempotent": { graph: func() *Graph[rune] { - g := New[rune]() + g := New[rune]([]rune{}) g.Add(Edge[rune]{'a', 'b'}) g.Add(Edge[rune]{'z', 'b'}) g.Remove(Edge[rune]{'a', 'b'}) @@ -187,15 +187,15 @@ func TestGraph_Roots(t *testing.T) { wantedRoots []int }{ "should return nil if the graph is empty": { - graph: New[int](), + graph: New[int]([]int{}), }, "should return all the vertices if there are no edges in the graph": { - graph: New[int](1, 2, 3, 4, 5), + graph: New[int]([]int{1, 2, 3, 4, 5}), wantedRoots: []int{1, 2, 3, 4, 5}, }, "should return only vertices with no in degrees": { graph: func() *Graph[int] { - g := New[int]() + g := New[int]([]int{}) g.Add(Edge[int]{ From: 1, To: 3, @@ -232,7 +232,7 @@ func TestTopologicalOrder(t *testing.T) { "should return an error when a cycle is detected": { // frontend <-> backend graph: func() *Graph[string] { - g := New("frontend", "backend") + g := New([]string{"frontend", "backend"}) g.Add(Edge[string]{ From: "frontend", To: "backend", @@ -248,7 +248,7 @@ func TestTopologicalOrder(t *testing.T) { "should return the ranks for a graph that looks like a bus": { // vpc -> lb -> api graph: func() *Graph[string] { - g := New[string]() + g := New[string]([]string{}) g.Add(Edge[string]{ From: "vpc", To: "lb", @@ -271,7 +271,7 @@ func TestTopologicalOrder(t *testing.T) { // vpc -> rds -> backend // -> s3 -> api // -> frontend - g := New[string]() + g := New[string]([]string{}) g.Add(Edge[string]{ From: "vpc", To: "rds", @@ -308,7 +308,7 @@ func TestTopologicalOrder(t *testing.T) { graph: func() *Graph[string] { // warehouse -> orders -> frontend // payments -> - g := New[string]() + g := New[string]([]string{}) g.Add(Edge[string]{ From: "payments", To: "frontend", @@ -335,7 +335,7 @@ func TestTopologicalOrder(t *testing.T) { graph: func() *Graph[string] { // a -> b -> c -> d -> f // a -> e -> f - g := New[string]() + g := New[string]([]string{}) for _, edge := range []Edge[string]{{"a", "b"}, {"b", "c"}, {"c", "d"}, {"d", "f"}, {"a", "e"}, {"e", "f"}} { g.Add(edge) } @@ -373,7 +373,7 @@ func TestTopologicalOrder(t *testing.T) { func buildGraphWithSingleParent() *Graph[string] { vertices := []string{"A", "B", "C", "D"} - graph := NewWithStatus[string]("started", vertices...) + graph := New[string](vertices, WithStatus[string]("started")) graph.Add(Edge[string]{From: "D", To: "C"}) // D -> C graph.Add(Edge[string]{From: "C", To: "B"}) // C -> B graph.Add(Edge[string]{From: "B", To: "A"}) // B -> A @@ -395,7 +395,7 @@ func TestTraverseInDependencyOrder(t *testing.T) { }) t.Run("graph with multiple parents and boundary nodes", func(t *testing.T) { vertices := []string{"A", "B", "C", "D"} - graph := NewWithStatus[string]("started", vertices...) + graph := New[string](vertices, WithStatus[string]("started")) graph.Add(Edge[string]{From: "A", To: "C"}) graph.Add(Edge[string]{From: "A", To: "D"}) graph.Add(Edge[string]{From: "B", To: "D"}) diff --git a/internal/pkg/manifest/validate.go b/internal/pkg/manifest/validate.go index c44fa02ca22..92aec976dc5 100644 --- a/internal/pkg/manifest/validate.go +++ b/internal/pkg/manifest/validate.go @@ -2313,7 +2313,7 @@ func validateNoCircularDependencies(deps map[string]ContainerDependency) error { } func buildDependencyGraph(deps map[string]ContainerDependency) (*graph.Graph[string], error) { - dependencyGraph := graph.New[string]() + dependencyGraph := graph.New[string]([]string{}) for name, containerDep := range deps { for dep := range containerDep.DependsOn { if _, ok := deps[dep]; !ok { From 3b212ee954b7836fd386a42a87e8ea673ca99045 Mon Sep 17 00:00:00 2001 From: AdithyaKolla Date: Sat, 18 Nov 2023 23:46:42 -0800 Subject: [PATCH 14/53] override container with dependsOn --- internal/pkg/cli/run_local.go | 1 + 1 file changed, 1 insertion(+) diff --git a/internal/pkg/cli/run_local.go b/internal/pkg/cli/run_local.go index a41b7e60994..4991dfdfc9d 100644 --- a/internal/pkg/cli/run_local.go +++ b/internal/pkg/cli/run_local.go @@ -529,6 +529,7 @@ func (o *runLocalOpts) prepareTask(ctx context.Context) (orchestrator.Task, erro } ctr.IsEssential = dep.IsEssential ctr.DependsOn = dep.DependsOn + task.Containers[name] = ctr } return task, nil From 636586862ab75b683ea49ad1f8f27713c352abb8 Mon Sep 17 00:00:00 2001 From: AdithyaKolla Date: Wed, 29 Nov 2023 16:52:39 -0800 Subject: [PATCH 15/53] most of the chnages --- .../pkg/docker/dockerengine/dockerengine.go | 17 +++ .../docker/dockerengine/dockerengine_test.go | 24 +++ .../pkg/docker/orchestrator/orchestrator.go | 140 ++++++++++++++++-- internal/pkg/graph/graph.go | 51 ++++--- 4 files changed, 201 insertions(+), 31 deletions(-) diff --git a/internal/pkg/docker/dockerengine/dockerengine.go b/internal/pkg/docker/dockerengine/dockerengine.go index 1fcf72e78bc..821d1bc974b 100644 --- a/internal/pkg/docker/dockerengine/dockerengine.go +++ b/internal/pkg/docker/dockerengine/dockerengine.go @@ -9,6 +9,7 @@ import ( "bytes" "context" "encoding/json" + "errors" "fmt" "io" "os" @@ -29,6 +30,10 @@ type Cmd interface { RunWithContext(ctx context.Context, name string, args []string, opts ...exec.CmdOption) error } +type exitCodeError interface { + ExitCode() int +} + // Operating systems and architectures supported by docker. const ( OSLinux = "linux" @@ -342,6 +347,13 @@ func (c DockerCmdClient) Run(ctx context.Context, options *RunOptions) error { exec.Stdout(stdout), exec.Stderr(stderr), exec.NewProcessGroup()); err != nil { + var ec exitCodeError + if errors.As(err, &ec) { + return &ErrContainerExited{ + name: options.ContainerName, + exitcode: ec.ExitCode(), + } + } return fmt.Errorf("running container: %w", err) } return nil @@ -446,6 +458,11 @@ type ErrContainerExited struct { exitcode int } +// ExitCode returns the OS exit code configured for this error. +func (e *ErrContainerExited) ExitCode() int { + return e.exitcode +} + // ErrContainerExited represents docker container exited with an exitcode. func (e *ErrContainerExited) Error() string { return fmt.Sprintf("container %q exited with code %d", e.name, e.exitcode) diff --git a/internal/pkg/docker/dockerengine/dockerengine_test.go b/internal/pkg/docker/dockerengine/dockerengine_test.go index 0cf852edfd3..b815f0b29f0 100644 --- a/internal/pkg/docker/dockerengine/dockerengine_test.go +++ b/internal/pkg/docker/dockerengine/dockerengine_test.go @@ -9,6 +9,7 @@ import ( "errors" "fmt" "io" + "os" osexec "os/exec" "path/filepath" "strings" @@ -686,6 +687,29 @@ func TestDockerCommand_Run(t *testing.T) { }, wantedError: fmt.Errorf("running container: %w", mockError), }, + + "should return error when container exits": { + containerName: mockPauseContainer, + command: mockCommand, + uri: mockImageURI, + setupMocks: func(controller *gomock.Controller) { + mockCmd = NewMockCmd(controller) + + mockCmd.EXPECT().RunWithContext(gomock.Any(), "docker", []string{"run", + "--name", mockPauseContainer, + "mockImageUri", + "sleep", "infinity"}, gomock.Any(), gomock.Any(), gomock.Any()). + DoAndReturn(func(ctx context.Context, name string, args []string, opts ...exec.CmdOption) error { + // Simulate an zero exit code. + return &osexec.ExitError{ProcessState: &os.ProcessState{}} + }) + }, + wantedError: &ErrContainerExited{ + name: mockPauseContainer, + exitcode: 0, + }, + }, + "success with run options for pause container": { containerName: mockPauseContainer, ports: mockContainerPorts, diff --git a/internal/pkg/docker/orchestrator/orchestrator.go b/internal/pkg/docker/orchestrator/orchestrator.go index c5ef7f9dd7f..acd0165dcdf 100644 --- a/internal/pkg/docker/orchestrator/orchestrator.go +++ b/internal/pkg/docker/orchestrator/orchestrator.go @@ -20,6 +20,9 @@ import ( "time" "github.com/aws/copilot-cli/internal/pkg/docker/dockerengine" + "github.com/aws/copilot-cli/internal/pkg/graph" + "github.com/aws/copilot-cli/internal/pkg/term/log" + "golang.org/x/sync/errgroup" ) // Orchestrator manages running a Task. Only a single Task @@ -49,6 +52,8 @@ type logOptionsFunc func(name string, ctr ContainerDefinition) dockerengine.RunL type DockerEngine interface { Run(context.Context, *dockerengine.RunOptions) error IsContainerRunning(context.Context, string) (bool, error) + IsContainerCompleteOrSuccess(ctx context.Context, containerName string) (bool, int, error) + IsContainerHealthy(ctx context.Context, containerName string) (bool, error) Stop(context.Context, string) error Build(ctx context.Context, args *dockerengine.BuildArguments, w io.Writer) error Exec(ctx context.Context, container string, out io.Writer, cmd string, args ...string) error @@ -68,6 +73,18 @@ const ( proxyPortStart = uint16(50000) ) +const ( + ctrStateUnknown = "unknown" + ctrStateRunningOrExited = "RunningOrExited" +) + +const ( + ctrStateHealthy = "healthy" + ctrStateComplete = "complete" + ctrStateSuccess = "success" + ctrStateStart = "start" +) + //go:embed Pause-Dockerfile var pauseDockerfile string @@ -194,8 +211,8 @@ func (a *runTaskAction) Do(o *Orchestrator) error { // start the pause container opts := o.pauseRunOptions(a.task) - o.run(pauseCtrTaskID, opts) - if err := o.waitForContainerToStart(ctx, opts.ContainerName); err != nil { + o.run(pauseCtrTaskID, opts, true, cancel) + if err := o.waitForContainerToStart(ctx, opts.ContainerName, true); err != nil { return fmt.Errorf("wait for pause container to start: %w", err) } @@ -222,16 +239,43 @@ func (a *runTaskAction) Do(o *Orchestrator) error { // TODO(Aiden): Implement a container ID system or use `docker ps` to ensure containers are stopped time.Sleep(1 * time.Second) } - - for name, ctr := range a.task.Containers { - name, ctr := name, ctr - o.run(taskID, o.containerRunOptions(name, ctr)) - } - o.curTask = a.task + depGraph := buildDependencyGraph(a.task.Containers) + err := depGraph.UpwardTraversal(ctx, func(ctx context.Context, containerName string) error { + if len(a.task.Containers[containerName].DependsOn) > 0 { + if err := o.waitForContainerDependencies(ctx, containerName, a); err != nil { + return fmt.Errorf("wait for container %s dependencies: %w", containerName, err) + } + } + o.run(taskID, o.containerRunOptions(containerName, a.task.Containers[containerName]), a.task.Containers[containerName].IsEssential, cancel) + return o.waitForContainerToStart(ctx, o.containerID(containerName), a.task.Containers[containerName].IsEssential) + }, ctrStateUnknown, ctrStateRunningOrExited) + if err != nil { + if errors.Is(err, context.Canceled) { + return nil + } + return fmt.Errorf("upward traversal: %w", err) + } return nil } +func buildDependencyGraph(containers map[string]ContainerDefinition) *graph.Graph[string] { + var vertices []string + for vertex := range containers { + vertices = append(vertices, vertex) + } + dependencyGraph := graph.New(vertices, graph.WithStatus[string](ctrStateUnknown)) + for containerName, container := range containers { + for depCtr := range container.DependsOn { + dependencyGraph.Add(graph.Edge[string]{ + From: containerName, + To: depCtr, + }) + } + } + return dependencyGraph +} + // setupProxyConnections creates proxy connections to a.hosts in pauseContainer. // It assumes that pauseContainer is already running. A unique proxy connection // is created for each host (in parallel) using AWS SSM Port Forwarding through @@ -416,16 +460,19 @@ func (o *Orchestrator) stopTask(ctx context.Context, task Task) error { } // waitForContainerToStart blocks until the container specified by id starts. -func (o *Orchestrator) waitForContainerToStart(ctx context.Context, id string) error { +func (o *Orchestrator) waitForContainerToStart(ctx context.Context, id string, isEssential bool) error { for { isRunning, err := o.docker.IsContainerRunning(ctx, id) switch { case err != nil: + var errContainerExited *dockerengine.ErrContainerExited + if errors.As(err, &errContainerExited) && !isEssential { + return nil + } return fmt.Errorf("check if %q is running: %w", id, err) case isRunning: return nil } - select { case <-time.After(1 * time.Second): case <-ctx.Done(): @@ -434,6 +481,66 @@ func (o *Orchestrator) waitForContainerToStart(ctx context.Context, id string) e } } +func (o *Orchestrator) waitForContainerDependencies(ctx context.Context, containerName string, a *runTaskAction) error { + fmt.Printf("Waiting for container %q dependencies...\n", containerName) + eg, ctx := errgroup.WithContext(ctx) + for name, state := range a.task.Containers[containerName].DependsOn { + name, state := name, state + eg.Go(func() error { + ctrId := o.containerID(name) + isEssential := a.task.Containers[name].IsEssential + ticker := time.NewTicker(700 * time.Millisecond) + defer ticker.Stop() + for { + select { + case <-ticker.C: + switch state { + case ctrStateStart: + err := o.waitForContainerToStart(ctx, ctrId, isEssential) + if err != nil { + return err + } + if !isEssential { + fmt.Printf("non-essential container %q started successfully\n", ctrId) + } + log.Successf("Successfully container %s is running\n", ctrId) + return nil + case ctrStateHealthy: + healthy, err := o.docker.IsContainerHealthy(ctx, ctrId) + if err != nil { + if !isEssential { + fmt.Printf("non-essential container %q failed to be %q: %v\n", ctrId, state, err) + return nil + } + return fmt.Errorf("essential container %q failed to be %q: %w", ctrId, state, err) + } + if healthy { + log.Successf("Successfully dependency container %q reached %q\n", ctrId, state) + return nil + } + case ctrStateComplete, ctrStateSuccess: + exited, exitCode, err := o.docker.IsContainerCompleteOrSuccess(ctx, ctrId) + if err != nil { + if !isEssential { + fmt.Printf("non-essential container %q failed to be %q: %v\n", ctrId, state, err) + return nil + } + return fmt.Errorf("dependency container %q failed to be %q: %w", ctrId, state, err) + } + if exited && ((state == "complete" && exitCode != 0) || (state == "success" && exitCode == 0)) { + log.Successf("Successfully dependency container %q exited with code: %d\n", name, exitCode) + return nil + } + } + case <-ctx.Done(): + return ctx.Err() + } + } + }) + } + return eg.Wait() +} + // containerID returns the full ID for a container with name run by s. func (o *Orchestrator) containerID(name string) string { return o.idPrefix + name @@ -495,7 +602,10 @@ func (o *Orchestrator) containerRunOptions(name string, ctr ContainerDefinition) // run calls `docker run` using opts. Errors are only returned // to the main Orchestrator routine if the taskID the container was run with // matches the current taskID the Orchestrator is running. -func (o *Orchestrator) run(taskID int32, opts dockerengine.RunOptions) { +// run calls `docker run` using opts. Errors are only returned +// to the main Orchestrator routine if the taskID the container was run with +// matches the current taskID the Orchestrator is running. +func (o *Orchestrator) run(taskID int32, opts dockerengine.RunOptions, isEssential bool, cancelFn context.CancelFunc) { o.wg.Add(1) go func() { defer o.wg.Done() @@ -514,7 +624,13 @@ func (o *Orchestrator) run(taskID int32, opts dockerengine.RunOptions) { if err == nil { err = errors.New("container stopped unexpectedly") } - o.runErrs <- fmt.Errorf("run %q: %w", opts.ContainerName, err) + var errContainerExited *dockerengine.ErrContainerExited + if errors.As(err, &errContainerExited) && !isEssential { + return + } + // cancel context to indicate all the other go routines spawned by `graph.UpwardTarversal`. + cancelFn() + o.runErrs <- fmt.Errorf("run essential %q: %w", opts.ContainerName, err) } }() } diff --git a/internal/pkg/graph/graph.go b/internal/pkg/graph/graph.go index 3b6dcdcac9e..1e02a05b6df 100644 --- a/internal/pkg/graph/graph.go +++ b/internal/pkg/graph/graph.go @@ -7,8 +7,6 @@ package graph import ( "context" "sync" - - "golang.org/x/sync/errgroup" ) // vertexStatus denotes the visiting status of a vertex when running DFS in a graph. @@ -370,53 +368,68 @@ type graphTraversal[V comparable] struct { } func (t *graphTraversal[V]) execute(ctx context.Context, graph *Graph[V]) error { + + ctx, cancel := context.WithCancel(ctx) + defer cancel() + vertexCount := len(graph.vertices) if vertexCount == 0 { return nil } - eg, ctx := errgroup.WithContext(ctx) + + var wg sync.WaitGroup vertexCh := make(chan V, vertexCount) - defer close(vertexCh) + errCh := make(chan error, vertexCount) // Channel for errors - processVertices := func(ctx context.Context, graph *Graph[V], eg *errgroup.Group, vertices []V, vertexCh chan V) { + processVertices := func(ctx context.Context, graph *Graph[V], wg *sync.WaitGroup, vertices []V, vertexCh chan V) { for _, vertex := range vertices { vertex := vertex - // Delay processing this vertex if any of its dependent vertices are yet to be processed. if len(t.filterVerticesByStatus(graph, vertex, t.adjacentVertexSkipStatus)) != 0 { continue } if !t.markAsSeen(vertex) { - // Skip this vertex if it's already been processed by another routine. continue } - eg.Go(func() error { + wg.Add(1) + go func() { + defer wg.Done() if err := t.processVertex(ctx, vertex); err != nil { - return err + errCh <- err + return } - // Assign new status to the vertex upon successful processing. graph.updateStatus(vertex, t.requiredVertexStatus) vertexCh <- vertex - return nil - }) + }() } } - eg.Go(func() error { + wg.Add(1) + go func() { + defer wg.Done() for { select { case <-ctx.Done(): - return nil + return case vertex := <-vertexCh: vertexCount-- if vertexCount == 0 { - return nil + return } - processVertices(ctx, graph, eg, t.findAdjacentVertices(graph, vertex), vertexCh) + processVertices(ctx, graph, &wg, t.findAdjacentVertices(graph, vertex), vertexCh) } } - }) - processVertices(ctx, graph, eg, t.findBoundaryVertices(graph), vertexCh) - return eg.Wait() + }() + processVertices(ctx, graph, &wg, t.findBoundaryVertices(graph), vertexCh) + wg.Wait() // Wait for all goroutines to finish + close(errCh) // Close error channel + + // Check if there were any errors + for err := range errCh { + if err != nil { + return err + } + } + return nil } func (t *graphTraversal[V]) markAsSeen(vertex V) bool { From 3cace5410258c1bdc2b375f90c222952c9322019 Mon Sep 17 00:00:00 2001 From: AdithyaKolla Date: Wed, 29 Nov 2023 19:15:35 -0800 Subject: [PATCH 16/53] use errorgroup --- internal/pkg/graph/graph.go | 47 +++++++++++++++---------------------- 1 file changed, 19 insertions(+), 28 deletions(-) diff --git a/internal/pkg/graph/graph.go b/internal/pkg/graph/graph.go index 1e02a05b6df..842e8f3b96a 100644 --- a/internal/pkg/graph/graph.go +++ b/internal/pkg/graph/graph.go @@ -7,6 +7,8 @@ package graph import ( "context" "sync" + + "golang.org/x/sync/errgroup" ) // vertexStatus denotes the visiting status of a vertex when running DFS in a graph. @@ -376,60 +378,49 @@ func (t *graphTraversal[V]) execute(ctx context.Context, graph *Graph[V]) error if vertexCount == 0 { return nil } - - var wg sync.WaitGroup + eg, ctx := errgroup.WithContext(ctx) vertexCh := make(chan V, vertexCount) - errCh := make(chan error, vertexCount) // Channel for errors + defer close(vertexCh) - processVertices := func(ctx context.Context, graph *Graph[V], wg *sync.WaitGroup, vertices []V, vertexCh chan V) { + processVertices := func(ctx context.Context, graph *Graph[V], eg *errgroup.Group, vertices []V, vertexCh chan V) { for _, vertex := range vertices { vertex := vertex + // Delay processing this vertex if any of its dependent vertices are yet to be processed. if len(t.filterVerticesByStatus(graph, vertex, t.adjacentVertexSkipStatus)) != 0 { continue } if !t.markAsSeen(vertex) { + // Skip this vertex if it's already been processed by another routine. continue } - wg.Add(1) - go func() { - defer wg.Done() + eg.Go(func() error { if err := t.processVertex(ctx, vertex); err != nil { - errCh <- err - return + return err } + // Assign new status to the vertex upon successful processing. graph.updateStatus(vertex, t.requiredVertexStatus) vertexCh <- vertex - }() + return nil + }) } } - wg.Add(1) - go func() { - defer wg.Done() + eg.Go(func() error { for { select { case <-ctx.Done(): - return + return nil case vertex := <-vertexCh: vertexCount-- if vertexCount == 0 { - return + return nil } - processVertices(ctx, graph, &wg, t.findAdjacentVertices(graph, vertex), vertexCh) + processVertices(ctx, graph, eg, t.findAdjacentVertices(graph, vertex), vertexCh) } } - }() - processVertices(ctx, graph, &wg, t.findBoundaryVertices(graph), vertexCh) - wg.Wait() // Wait for all goroutines to finish - close(errCh) // Close error channel - - // Check if there were any errors - for err := range errCh { - if err != nil { - return err - } - } - return nil + }) + processVertices(ctx, graph, eg, t.findBoundaryVertices(graph), vertexCh) + return eg.Wait() } func (t *graphTraversal[V]) markAsSeen(vertex V) bool { From 1da538f556c4835c4c4290a599facb841a40ce40 Mon Sep 17 00:00:00 2001 From: AdithyaKolla Date: Wed, 29 Nov 2023 19:15:46 -0800 Subject: [PATCH 17/53] update mocks --- .../docker/dockerengine/mock_dockerengine.go | 37 +++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/internal/pkg/docker/dockerengine/mock_dockerengine.go b/internal/pkg/docker/dockerengine/mock_dockerengine.go index a0b0d3b10b6..5a80c6f5806 100644 --- a/internal/pkg/docker/dockerengine/mock_dockerengine.go +++ b/internal/pkg/docker/dockerengine/mock_dockerengine.go @@ -72,3 +72,40 @@ func (mr *MockCmdMockRecorder) RunWithContext(ctx, name, args interface{}, opts varargs := append([]interface{}{ctx, name, args}, opts...) return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RunWithContext", reflect.TypeOf((*MockCmd)(nil).RunWithContext), varargs...) } + +// MockexitCodeError is a mock of exitCodeError interface. +type MockexitCodeError struct { + ctrl *gomock.Controller + recorder *MockexitCodeErrorMockRecorder +} + +// MockexitCodeErrorMockRecorder is the mock recorder for MockexitCodeError. +type MockexitCodeErrorMockRecorder struct { + mock *MockexitCodeError +} + +// NewMockexitCodeError creates a new mock instance. +func NewMockexitCodeError(ctrl *gomock.Controller) *MockexitCodeError { + mock := &MockexitCodeError{ctrl: ctrl} + mock.recorder = &MockexitCodeErrorMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockexitCodeError) EXPECT() *MockexitCodeErrorMockRecorder { + return m.recorder +} + +// ExitCode mocks base method. +func (m *MockexitCodeError) ExitCode() int { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "ExitCode") + ret0, _ := ret[0].(int) + return ret0 +} + +// ExitCode indicates an expected call of ExitCode. +func (mr *MockexitCodeErrorMockRecorder) ExitCode() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ExitCode", reflect.TypeOf((*MockexitCodeError)(nil).ExitCode)) +} From 33fecbeedae1725f137ab2eb77cda5b58171b9e7 Mon Sep 17 00:00:00 2001 From: AdithyaKolla Date: Thu, 30 Nov 2023 10:18:39 -0800 Subject: [PATCH 18/53] add remove of containers in stop --- internal/pkg/cli/interfaces.go | 2 +- internal/pkg/cli/mocks/mock_interfaces.go | 8 ++++---- internal/pkg/docker/dockerengine/dockerengine.go | 4 ++-- .../dockerenginetest/dockerenginetest.go | 8 ++++++++ internal/pkg/docker/orchestrator/orchestrator.go | 13 ++++++++++++- 5 files changed, 27 insertions(+), 8 deletions(-) diff --git a/internal/pkg/cli/interfaces.go b/internal/pkg/cli/interfaces.go index 797704f9719..f399712c878 100644 --- a/internal/pkg/cli/interfaces.go +++ b/internal/pkg/cli/interfaces.go @@ -709,11 +709,11 @@ type dockerEngineRunner interface { Run(context.Context, *dockerengine.RunOptions) error IsContainerRunning(context.Context, string) (bool, error) Stop(context.Context, string) error - Rm(string) error Build(context.Context, *dockerengine.BuildArguments, io.Writer) error Exec(ctx context.Context, container string, out io.Writer, cmd string, args ...string) error IsContainerCompleteOrSuccess(ctx context.Context, containerName string) (bool, int, error) IsContainerHealthy(ctx context.Context, containerName string) (bool, error) + Rm(context.Context, string) error } type workloadStackGenerator interface { diff --git a/internal/pkg/cli/mocks/mock_interfaces.go b/internal/pkg/cli/mocks/mock_interfaces.go index 1448ce78fce..a1388d74a30 100644 --- a/internal/pkg/cli/mocks/mock_interfaces.go +++ b/internal/pkg/cli/mocks/mock_interfaces.go @@ -7774,17 +7774,17 @@ func (mr *MockdockerEngineRunnerMockRecorder) IsContainerRunning(arg0, arg1 inte } // Rm mocks base method. -func (m *MockdockerEngineRunner) Rm(arg0 string) error { +func (m *MockdockerEngineRunner) Rm(arg0 context.Context, arg1 string) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Rm", arg0) + ret := m.ctrl.Call(m, "Rm", arg0, arg1) ret0, _ := ret[0].(error) return ret0 } // Rm indicates an expected call of Rm. -func (mr *MockdockerEngineRunnerMockRecorder) Rm(arg0 interface{}) *gomock.Call { +func (mr *MockdockerEngineRunnerMockRecorder) Rm(arg0, arg1 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Rm", reflect.TypeOf((*MockdockerEngineRunner)(nil).Rm), arg0) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Rm", reflect.TypeOf((*MockdockerEngineRunner)(nil).Rm), arg0, arg1) } // Run mocks base method. diff --git a/internal/pkg/docker/dockerengine/dockerengine.go b/internal/pkg/docker/dockerengine/dockerengine.go index 821d1bc974b..3952b610860 100644 --- a/internal/pkg/docker/dockerengine/dockerengine.go +++ b/internal/pkg/docker/dockerengine/dockerengine.go @@ -478,9 +478,9 @@ func (c DockerCmdClient) Stop(ctx context.Context, containerID string) error { } // Rm calls `docker rm` to remove a stopped container. -func (c DockerCmdClient) Rm(containerID string) error { +func (c DockerCmdClient) Rm(ctx context.Context, containerID string) error { buf := &bytes.Buffer{} - if err := c.runner.Run("docker", []string{"rm", containerID}, exec.Stdout(buf), exec.Stderr(buf)); err != nil { + if err := c.runner.RunWithContext(ctx, "docker", []string{"rm", containerID}, exec.Stdout(buf), exec.Stderr(buf)); err != nil { return fmt.Errorf("%s: %w", strings.TrimSpace(buf.String()), err) } return nil diff --git a/internal/pkg/docker/dockerengine/dockerenginetest/dockerenginetest.go b/internal/pkg/docker/dockerengine/dockerenginetest/dockerenginetest.go index 29eb5dcd118..54692c22b21 100644 --- a/internal/pkg/docker/dockerengine/dockerenginetest/dockerenginetest.go +++ b/internal/pkg/docker/dockerengine/dockerenginetest/dockerenginetest.go @@ -19,6 +19,7 @@ type Double struct { ExecFn func(context.Context, string, io.Writer, string, ...string) error IsContainerHealthyFn func(ctx context.Context, containerName string) (bool, error) IsContainerRunningOrHealthyFn func(ctx context.Context, containerName string) (bool, int, error) + RmFn func(context.Context, string) error } // IsContainerCompleteOrSuccess implements orchestrator.DockerEngine. @@ -70,3 +71,10 @@ func (d *Double) Exec(ctx context.Context, container string, out io.Writer, cmd } return d.ExecFn(ctx, container, out, cmd, args...) } + +func (d *Double) Rm(ctx context.Context, name string) error { + if d.RmFn == nil { + return nil + } + return d.RmFn(ctx, name) +} diff --git a/internal/pkg/docker/orchestrator/orchestrator.go b/internal/pkg/docker/orchestrator/orchestrator.go index acd0165dcdf..97df578367a 100644 --- a/internal/pkg/docker/orchestrator/orchestrator.go +++ b/internal/pkg/docker/orchestrator/orchestrator.go @@ -57,6 +57,7 @@ type DockerEngine interface { Stop(context.Context, string) error Build(ctx context.Context, args *dockerengine.BuildArguments, w io.Writer) error Exec(ctx context.Context, container string, out io.Writer, cmd string, args ...string) error + Rm(context.Context, string) error } const ( @@ -422,6 +423,9 @@ func (a *stopAction) Do(o *Orchestrator) error { if err := o.docker.Stop(context.Background(), o.containerID("pause")); err != nil { errs = append(errs, fmt.Errorf("stop %q: %w", "pause", err)) } + if err := o.docker.Rm(context.Background(), o.containerID("pause")); err != nil { + errs = append(errs, fmt.Errorf("remove %q: %w", "pause", err)) + } fmt.Printf("Stopped %q\n", "pause") return errors.Join(errs...) @@ -444,6 +448,13 @@ func (o *Orchestrator) stopTask(ctx context.Context, task Task) error { return } fmt.Printf("Stopped %q\n", name) + // Remove the container + fmt.Printf("Removing %q\n", name) + if err := o.docker.Rm(ctx, o.containerID(name)); err != nil { + errCh <- fmt.Errorf("remove %q: %w", name, err) + return + } + fmt.Printf("Removed %q\n", name) errCh <- nil }() } @@ -630,7 +641,7 @@ func (o *Orchestrator) run(taskID int32, opts dockerengine.RunOptions, isEssenti } // cancel context to indicate all the other go routines spawned by `graph.UpwardTarversal`. cancelFn() - o.runErrs <- fmt.Errorf("run essential %q: %w", opts.ContainerName, err) + o.runErrs <- fmt.Errorf("run %q: %w", opts.ContainerName, err) } }() } From 4d5287d55ed42e4e8a5eeef5bf68271a4b6e4d4c Mon Sep 17 00:00:00 2001 From: AdithyaKolla Date: Tue, 5 Dec 2023 21:25:20 -0800 Subject: [PATCH 19/53] resolve all merge conflicts --- internal/pkg/cli/interfaces.go | 2 +- internal/pkg/cli/mocks/mock_interfaces.go | 9 +- internal/pkg/deploy/pipeline.go | 7 +- .../pkg/docker/dockerengine/dockerengine.go | 37 +-- .../docker/dockerengine/dockerengine_test.go | 79 +++--- .../dockerenginetest/dockerenginetest.go | 30 ++- .../docker/dockerengine/mock_dockerengine.go | 37 --- .../pkg/docker/orchestrator/orchestrator.go | 52 ++-- .../docker/orchestrator/orchestrator_test.go | 4 +- internal/pkg/graph/graph.go | 251 ++++++++++-------- internal/pkg/graph/graph_test.go | 38 +-- internal/pkg/manifest/validate.go | 2 +- 12 files changed, 263 insertions(+), 285 deletions(-) diff --git a/internal/pkg/cli/interfaces.go b/internal/pkg/cli/interfaces.go index f399712c878..ddd4a2bb925 100644 --- a/internal/pkg/cli/interfaces.go +++ b/internal/pkg/cli/interfaces.go @@ -711,7 +711,7 @@ type dockerEngineRunner interface { Stop(context.Context, string) error Build(context.Context, *dockerengine.BuildArguments, io.Writer) error Exec(ctx context.Context, container string, out io.Writer, cmd string, args ...string) error - IsContainerCompleteOrSuccess(ctx context.Context, containerName string) (bool, int, error) + IsContainerCompleteOrSuccess(ctx context.Context, containerName string) (int, error) IsContainerHealthy(ctx context.Context, containerName string) (bool, error) Rm(context.Context, string) error } diff --git a/internal/pkg/cli/mocks/mock_interfaces.go b/internal/pkg/cli/mocks/mock_interfaces.go index a1388d74a30..7763afb81dd 100644 --- a/internal/pkg/cli/mocks/mock_interfaces.go +++ b/internal/pkg/cli/mocks/mock_interfaces.go @@ -7728,13 +7728,12 @@ func (mr *MockdockerEngineRunnerMockRecorder) Exec(ctx, container, out, cmd inte } // IsContainerCompleteOrSuccess mocks base method. -func (m *MockdockerEngineRunner) IsContainerCompleteOrSuccess(ctx context.Context, containerName string) (bool, int, error) { +func (m *MockdockerEngineRunner) IsContainerCompleteOrSuccess(ctx context.Context, containerName string) (int, error) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "IsContainerCompleteOrSuccess", ctx, containerName) - ret0, _ := ret[0].(bool) - ret1, _ := ret[1].(int) - ret2, _ := ret[2].(error) - return ret0, ret1, ret2 + ret0, _ := ret[0].(int) + ret1, _ := ret[1].(error) + return ret0, ret1 } // IsContainerCompleteOrSuccess indicates an expected call of IsContainerCompleteOrSuccess. diff --git a/internal/pkg/deploy/pipeline.go b/internal/pkg/deploy/pipeline.go index 355951a69a0..9ffb39442e8 100644 --- a/internal/pkg/deploy/pipeline.go +++ b/internal/pkg/deploy/pipeline.go @@ -8,14 +8,13 @@ package deploy import ( "errors" "fmt" + "gopkg.in/yaml.v3" "path" "path/filepath" "regexp" "sort" "strings" - "gopkg.in/yaml.v3" - "github.com/aws/aws-sdk-go/aws/arn" "github.com/aws/copilot-cli/internal/pkg/config" "github.com/aws/copilot-cli/internal/pkg/graph" @@ -781,7 +780,7 @@ func (stg *PipelineStage) buildActionsGraph(rankables []actionGraphNode) *graph. for _, r := range rankables { names = append(names, r.name) } - digraph := graph.New(names) + digraph := graph.New(names...) for _, r := range rankables { if r.depends_on == nil { @@ -917,4 +916,4 @@ func (p *PrePostDeployAction) Name() string { func (p *PrePostDeployAction) RunOrder() int { rank, _ := p.ranker.Rank(p.name) // The deployment is guaranteed to be in the ranker. return p.action.RunOrder() /* baseline */ + rank -} +} \ No newline at end of file diff --git a/internal/pkg/docker/dockerengine/dockerengine.go b/internal/pkg/docker/dockerengine/dockerengine.go index 3952b610860..f71c30c937c 100644 --- a/internal/pkg/docker/dockerengine/dockerengine.go +++ b/internal/pkg/docker/dockerengine/dockerengine.go @@ -9,7 +9,6 @@ import ( "bytes" "context" "encoding/json" - "errors" "fmt" "io" "os" @@ -30,10 +29,6 @@ type Cmd interface { RunWithContext(ctx context.Context, name string, args []string, opts ...exec.CmdOption) error } -type exitCodeError interface { - ExitCode() int -} - // Operating systems and architectures supported by docker. const ( OSLinux = "linux" @@ -268,6 +263,7 @@ func (c DockerCmdClient) Push(ctx context.Context, uri string, w io.Writer, tags func (in *RunOptions) generateRunArguments() []string { args := []string{"run"} + args = append(args, "--rm") if in.ContainerName != "" { args = append(args, "--name", in.ContainerName) @@ -347,13 +343,6 @@ func (c DockerCmdClient) Run(ctx context.Context, options *RunOptions) error { exec.Stdout(stdout), exec.Stderr(stderr), exec.NewProcessGroup()); err != nil { - var ec exitCodeError - if errors.As(err, &ec) { - return &ErrContainerExited{ - name: options.ContainerName, - exitcode: ec.ExitCode(), - } - } return fmt.Errorf("running container: %w", err) } return nil @@ -378,12 +367,15 @@ func (c DockerCmdClient) IsContainerRunning(ctx context.Context, name string) (b } // IsContainerCompleteOrSuccess returns true if a docker container exits with an exitcode. -func (c DockerCmdClient) IsContainerCompleteOrSuccess(ctx context.Context, containerName string) (bool, int, error) { +func (c DockerCmdClient) IsContainerCompleteOrSuccess(ctx context.Context, containerName string) (int, error) { state, err := c.containerState(ctx, containerName) if err != nil { - return false, 0, err + return 0, err } - return state.Status == containerStatusExited, state.ExitCode, nil + if state.Status == containerStatusRunning { + return -1, nil + } + return state.ExitCode, nil } // IsContainerHealthy returns true if a container health state is healthy. @@ -392,11 +384,11 @@ func (c DockerCmdClient) IsContainerHealthy(ctx context.Context, containerName s if err != nil { return false, err } - if state.Status == containerStatusExited { - return false, &ErrContainerExited{name: containerName, exitcode: state.ExitCode} + if state.Status != containerStatusRunning { + return false, fmt.Errorf("container %q is not in %q state", containerName, containerStatusRunning) } if state.Health == nil { - return false, fmt.Errorf("healthcheck is not configured for container %s", containerName) + return false, fmt.Errorf("healthcheck is not configured for container %q", containerName) } switch state.Health.Status { case healthy: @@ -406,9 +398,9 @@ func (c DockerCmdClient) IsContainerHealthy(ctx context.Context, containerName s case unhealthy: return false, fmt.Errorf("container %q is %q", containerName, unhealthy) case noHealthcheck: - return false, fmt.Errorf("healthcheck configuration is set to %q for container %s", noHealthcheck, containerName) + return false, fmt.Errorf("healthcheck is not configured for container %q", containerName) default: - return false, fmt.Errorf("container %s had unexpected health status %q", containerName, state.Health.Status) + return false, fmt.Errorf("container %q had unexpected health status %q", containerName, state.Health.Status) } } @@ -458,11 +450,6 @@ type ErrContainerExited struct { exitcode int } -// ExitCode returns the OS exit code configured for this error. -func (e *ErrContainerExited) ExitCode() int { - return e.exitcode -} - // ErrContainerExited represents docker container exited with an exitcode. func (e *ErrContainerExited) Error() string { return fmt.Sprintf("container %q exited with code %d", e.name, e.exitcode) diff --git a/internal/pkg/docker/dockerengine/dockerengine_test.go b/internal/pkg/docker/dockerengine/dockerengine_test.go index b815f0b29f0..d8cd130f254 100644 --- a/internal/pkg/docker/dockerengine/dockerengine_test.go +++ b/internal/pkg/docker/dockerengine/dockerengine_test.go @@ -9,7 +9,6 @@ import ( "errors" "fmt" "io" - "os" osexec "os/exec" "path/filepath" "strings" @@ -681,35 +680,13 @@ func TestDockerCommand_Run(t *testing.T) { setupMocks: func(controller *gomock.Controller) { mockCmd = NewMockCmd(controller) mockCmd.EXPECT().RunWithContext(gomock.Any(), "docker", []string{"run", + "--rm", "--name", mockPauseContainer, "mockImageUri", "sleep", "infinity"}, gomock.Any(), gomock.Any(), gomock.Any()).Return(mockError) }, wantedError: fmt.Errorf("running container: %w", mockError), }, - - "should return error when container exits": { - containerName: mockPauseContainer, - command: mockCommand, - uri: mockImageURI, - setupMocks: func(controller *gomock.Controller) { - mockCmd = NewMockCmd(controller) - - mockCmd.EXPECT().RunWithContext(gomock.Any(), "docker", []string{"run", - "--name", mockPauseContainer, - "mockImageUri", - "sleep", "infinity"}, gomock.Any(), gomock.Any(), gomock.Any()). - DoAndReturn(func(ctx context.Context, name string, args []string, opts ...exec.CmdOption) error { - // Simulate an zero exit code. - return &osexec.ExitError{ProcessState: &os.ProcessState{}} - }) - }, - wantedError: &ErrContainerExited{ - name: mockPauseContainer, - exitcode: 0, - }, - }, - "success with run options for pause container": { containerName: mockPauseContainer, ports: mockContainerPorts, @@ -718,6 +695,7 @@ func TestDockerCommand_Run(t *testing.T) { setupMocks: func(controller *gomock.Controller) { mockCmd = NewMockCmd(controller) mockCmd.EXPECT().RunWithContext(gomock.Any(), "docker", gomock.InAnyOrder([]string{"run", + "--rm", "--name", mockPauseContainer, "--publish", "8080:8080", "--publish", "8081:8081", @@ -734,6 +712,7 @@ func TestDockerCommand_Run(t *testing.T) { setupMocks: func(controller *gomock.Controller) { mockCmd = NewMockCmd(controller) mockCmd.EXPECT().RunWithContext(gomock.Any(), "docker", gomock.InAnyOrder([]string{"run", + "--rm", "--name", mockContainerName, "--network", "container:pauseContainer", "--env", "DB_PASSWORD=mysecretPassword", @@ -754,6 +733,7 @@ func TestDockerCommand_Run(t *testing.T) { setupMocks: func(controller *gomock.Controller) { mockCmd = NewMockCmd(controller) mockCmd.EXPECT().RunWithContext(gomock.Any(), "docker", gomock.InAnyOrder([]string{"run", + "--rm", "--name", mockContainerName, "--network", "container:pauseContainer", "--env", "DB_PASSWORD=mysecretPassword", @@ -1054,12 +1034,11 @@ func TestDockerCommand_IsContainerHealthy(t *testing.T) { func TestDockerCommand_IsContainerCompleteOrSuccess(t *testing.T) { tests := map[string]struct { - mockContainerName string - mockHealthStatus string - setupMocks func(*gomock.Controller) *MockCmd - wantCompleteOrSuccess bool - wantExitCode int - wantErr error + mockContainerName string + mockHealthStatus string + setupMocks func(*gomock.Controller) *MockCmd + wantExitCode int + wantErr error }{ "container successfully complete": { mockContainerName: "mockContainer", @@ -1088,10 +1067,9 @@ func TestDockerCommand_IsContainerCompleteOrSuccess(t *testing.T) { }) return mockCmd }, - wantCompleteOrSuccess: true, - wantExitCode: 143, + wantExitCode: 143, }, - "container successfully success": { + "container success": { mockContainerName: "mockContainer", mockHealthStatus: "unhealthy", setupMocks: func(controller *gomock.Controller) *MockCmd { @@ -1118,7 +1096,6 @@ func TestDockerCommand_IsContainerCompleteOrSuccess(t *testing.T) { }) return mockCmd }, - wantCompleteOrSuccess: true, }, "error when fetching container state": { mockContainerName: "mockContainer", @@ -1130,6 +1107,35 @@ func TestDockerCommand_IsContainerCompleteOrSuccess(t *testing.T) { }, wantErr: fmt.Errorf("run docker ps: some error"), }, + "return negative exitcode if container is running": { + mockContainerName: "mockContainer", + mockHealthStatus: "unhealthy", + setupMocks: func(controller *gomock.Controller) *MockCmd { + mockCmd := NewMockCmd(controller) + mockCmd.EXPECT().RunWithContext(gomock.Any(), "docker", []string{"ps", "-a", "-q", "--filter", "name=mockContainer"}, gomock.Any()).DoAndReturn(func(ctx context.Context, name string, args []string, opts ...exec.CmdOption) error { + cmd := &osexec.Cmd{} + for _, opt := range opts { + opt(cmd) + } + cmd.Stdout.Write([]byte("53d6417769ed")) + return nil + }) + mockCmd.EXPECT().RunWithContext(gomock.Any(), "docker", []string{"inspect", "--format", "{{json .State}}", "53d6417769ed"}, gomock.Any()).DoAndReturn(func(ctx context.Context, name string, args []string, opts ...exec.CmdOption) error { + cmd := &osexec.Cmd{} + for _, opt := range opts { + opt(cmd) + } + cmd.Stdout.Write([]byte(` +{ + "Status": "running", + "ExitCode": 0 +}`)) + return nil + }) + return mockCmd + }, + wantExitCode: -1, + }, } for name, tc := range tests { @@ -1138,11 +1144,10 @@ func TestDockerCommand_IsContainerCompleteOrSuccess(t *testing.T) { defer ctrl.Finish() s := DockerCmdClient{ - runner: tc.setupMocks(ctrl), // Correctly invoke the setupMocks function + runner: tc.setupMocks(ctrl), } - expected, expectedCode, err := s.IsContainerCompleteOrSuccess(context.Background(), tc.mockContainerName) - require.Equal(t, tc.wantCompleteOrSuccess, expected) + expectedCode, err := s.IsContainerCompleteOrSuccess(context.Background(), tc.mockContainerName) require.Equal(t, tc.wantExitCode, expectedCode) if tc.wantErr != nil { require.EqualError(t, err, tc.wantErr.Error()) diff --git a/internal/pkg/docker/dockerengine/dockerenginetest/dockerenginetest.go b/internal/pkg/docker/dockerengine/dockerenginetest/dockerenginetest.go index 54692c22b21..545b8a13377 100644 --- a/internal/pkg/docker/dockerengine/dockerenginetest/dockerenginetest.go +++ b/internal/pkg/docker/dockerengine/dockerenginetest/dockerenginetest.go @@ -12,24 +12,30 @@ import ( // Double is a test double for dockerengine.DockerCmdClient type Double struct { - StopFn func(context.Context, string) error - IsContainerRunningFn func(context.Context, string) (bool, error) - RunFn func(context.Context, *dockerengine.RunOptions) error - BuildFn func(context.Context, *dockerengine.BuildArguments, io.Writer) error - ExecFn func(context.Context, string, io.Writer, string, ...string) error - IsContainerHealthyFn func(ctx context.Context, containerName string) (bool, error) - IsContainerRunningOrHealthyFn func(ctx context.Context, containerName string) (bool, int, error) - RmFn func(context.Context, string) error + StopFn func(context.Context, string) error + IsContainerRunningFn func(context.Context, string) (bool, error) + RunFn func(context.Context, *dockerengine.RunOptions) error + BuildFn func(context.Context, *dockerengine.BuildArguments, io.Writer) error + ExecFn func(context.Context, string, io.Writer, string, ...string) error + IsContainerHealthyFn func(ctx context.Context, containerName string) (bool, error) + IsContainerCompleteOrSuccessFn func(ctx context.Context, containerName string) (int, error) + RmFn func(context.Context, string) error } // IsContainerCompleteOrSuccess implements orchestrator.DockerEngine. -func (*Double) IsContainerCompleteOrSuccess(ctx context.Context, containerName string) (bool, int, error) { - panic("unimplemented") +func (d *Double) IsContainerCompleteOrSuccess(ctx context.Context, containerName string) (int, error) { + if d.IsContainerCompleteOrSuccessFn == nil { + return -1, nil + } + return d.IsContainerCompleteOrSuccessFn(ctx, containerName) } // IsContainerHealthy implements orchestrator.DockerEngine. -func (*Double) IsContainerHealthy(ctx context.Context, containerName string) (bool, error) { - panic("unimplemented") +func (d *Double) IsContainerHealthy(ctx context.Context, containerName string) (bool, error) { + if d.IsContainerHealthyFn == nil { + return false, nil + } + return d.IsContainerHealthyFn(ctx, containerName) } // Stop calls the stubbed function. diff --git a/internal/pkg/docker/dockerengine/mock_dockerengine.go b/internal/pkg/docker/dockerengine/mock_dockerengine.go index 5a80c6f5806..a0b0d3b10b6 100644 --- a/internal/pkg/docker/dockerengine/mock_dockerengine.go +++ b/internal/pkg/docker/dockerengine/mock_dockerengine.go @@ -72,40 +72,3 @@ func (mr *MockCmdMockRecorder) RunWithContext(ctx, name, args interface{}, opts varargs := append([]interface{}{ctx, name, args}, opts...) return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RunWithContext", reflect.TypeOf((*MockCmd)(nil).RunWithContext), varargs...) } - -// MockexitCodeError is a mock of exitCodeError interface. -type MockexitCodeError struct { - ctrl *gomock.Controller - recorder *MockexitCodeErrorMockRecorder -} - -// MockexitCodeErrorMockRecorder is the mock recorder for MockexitCodeError. -type MockexitCodeErrorMockRecorder struct { - mock *MockexitCodeError -} - -// NewMockexitCodeError creates a new mock instance. -func NewMockexitCodeError(ctrl *gomock.Controller) *MockexitCodeError { - mock := &MockexitCodeError{ctrl: ctrl} - mock.recorder = &MockexitCodeErrorMockRecorder{mock} - return mock -} - -// EXPECT returns an object that allows the caller to indicate expected use. -func (m *MockexitCodeError) EXPECT() *MockexitCodeErrorMockRecorder { - return m.recorder -} - -// ExitCode mocks base method. -func (m *MockexitCodeError) ExitCode() int { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "ExitCode") - ret0, _ := ret[0].(int) - return ret0 -} - -// ExitCode indicates an expected call of ExitCode. -func (mr *MockexitCodeErrorMockRecorder) ExitCode() *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ExitCode", reflect.TypeOf((*MockexitCodeError)(nil).ExitCode)) -} diff --git a/internal/pkg/docker/orchestrator/orchestrator.go b/internal/pkg/docker/orchestrator/orchestrator.go index 97df578367a..eb0f7624ec7 100644 --- a/internal/pkg/docker/orchestrator/orchestrator.go +++ b/internal/pkg/docker/orchestrator/orchestrator.go @@ -52,7 +52,7 @@ type logOptionsFunc func(name string, ctr ContainerDefinition) dockerengine.RunL type DockerEngine interface { Run(context.Context, *dockerengine.RunOptions) error IsContainerRunning(context.Context, string) (bool, error) - IsContainerCompleteOrSuccess(ctx context.Context, containerName string) (bool, int, error) + IsContainerCompleteOrSuccess(ctx context.Context, containerName string) (int, error) IsContainerHealthy(ctx context.Context, containerName string) (bool, error) Stop(context.Context, string) error Build(ctx context.Context, args *dockerengine.BuildArguments, w io.Writer) error @@ -260,12 +260,12 @@ func (a *runTaskAction) Do(o *Orchestrator) error { return nil } -func buildDependencyGraph(containers map[string]ContainerDefinition) *graph.Graph[string] { +func buildDependencyGraph(containers map[string]ContainerDefinition) *graph.LabeledGraph[string] { var vertices []string for vertex := range containers { vertices = append(vertices, vertex) } - dependencyGraph := graph.New(vertices, graph.WithStatus[string](ctrStateUnknown)) + dependencyGraph := graph.NewLabeledGraph(vertices, graph.WithStatus[string](ctrStateUnknown)) for containerName, container := range containers { for depCtr := range container.DependsOn { dependencyGraph.Add(graph.Edge[string]{ @@ -419,14 +419,14 @@ func (a *stopAction) Do(o *Orchestrator) error { } // stop pause container - fmt.Printf("Stopping %q\n", "pause") + fmt.Printf("Stopping and Removing %q\n", "pause") if err := o.docker.Stop(context.Background(), o.containerID("pause")); err != nil { errs = append(errs, fmt.Errorf("stop %q: %w", "pause", err)) } if err := o.docker.Rm(context.Background(), o.containerID("pause")); err != nil { errs = append(errs, fmt.Errorf("remove %q: %w", "pause", err)) } - fmt.Printf("Stopped %q\n", "pause") + fmt.Printf("Stopped and Removed %q\n", "pause") return errors.Join(errs...) } @@ -442,19 +442,16 @@ func (o *Orchestrator) stopTask(ctx context.Context, task Task) error { for name := range task.Containers { name := name go func() { - fmt.Printf("Stopping %q\n", name) + fmt.Printf("Stopping and Removing %q\n", name) if err := o.docker.Stop(ctx, o.containerID(name)); err != nil { errCh <- fmt.Errorf("stop %q: %w", name, err) return } - fmt.Printf("Stopped %q\n", name) - // Remove the container - fmt.Printf("Removing %q\n", name) if err := o.docker.Rm(ctx, o.containerID(name)); err != nil { errCh <- fmt.Errorf("remove %q: %w", name, err) return } - fmt.Printf("Removed %q\n", name) + fmt.Printf("Stopped and Removed %q\n", name) errCh <- nil }() } @@ -529,19 +526,27 @@ func (o *Orchestrator) waitForContainerDependencies(ctx context.Context, contain log.Successf("Successfully dependency container %q reached %q\n", ctrId, state) return nil } - case ctrStateComplete, ctrStateSuccess: - exited, exitCode, err := o.docker.IsContainerCompleteOrSuccess(ctx, ctrId) + case ctrStateComplete: + exitCode, err := o.docker.IsContainerCompleteOrSuccess(ctx, ctrId) if err != nil { - if !isEssential { - fmt.Printf("non-essential container %q failed to be %q: %v\n", ctrId, state, err) - return nil - } return fmt.Errorf("dependency container %q failed to be %q: %w", ctrId, state, err) } - if exited && ((state == "complete" && exitCode != 0) || (state == "success" && exitCode == 0)) { + if exitCode != -1 { log.Successf("Successfully dependency container %q exited with code: %d\n", name, exitCode) return nil } + case ctrStateSuccess: + exitCode, err := o.docker.IsContainerCompleteOrSuccess(ctx, ctrId) + if err != nil { + return fmt.Errorf("dependency container %q failed to be %q: %w", ctrId, state, err) + } + if exitCode == 0 { + log.Successf("Successfully dependency container %q reached %q\n", ctrId, state) + return nil + } + if exitCode > 0 { + return fmt.Errorf("dependency container %q failed to be %q: %w", ctrId, state, err) + } } case <-ctx.Done(): return ctx.Err() @@ -613,10 +618,7 @@ func (o *Orchestrator) containerRunOptions(name string, ctr ContainerDefinition) // run calls `docker run` using opts. Errors are only returned // to the main Orchestrator routine if the taskID the container was run with // matches the current taskID the Orchestrator is running. -// run calls `docker run` using opts. Errors are only returned -// to the main Orchestrator routine if the taskID the container was run with -// matches the current taskID the Orchestrator is running. -func (o *Orchestrator) run(taskID int32, opts dockerengine.RunOptions, isEssential bool, cancelFn context.CancelFunc) { +func (o *Orchestrator) run(taskID int32, opts dockerengine.RunOptions, isEssential bool, cancel context.CancelFunc) { o.wg.Add(1) go func() { defer o.wg.Done() @@ -632,15 +634,15 @@ func (o *Orchestrator) run(taskID int32, opts dockerengine.RunOptions, isEssenti // the error is from the pause container // or from the currently running task if taskID == pauseCtrTaskID || taskID == curTaskID { - if err == nil { - err = errors.New("container stopped unexpectedly") - } var errContainerExited *dockerengine.ErrContainerExited if errors.As(err, &errContainerExited) && !isEssential { return } + if err == nil && isEssential { + err = errors.New("container stopped unexpectedly") + } // cancel context to indicate all the other go routines spawned by `graph.UpwardTarversal`. - cancelFn() + cancel() o.runErrs <- fmt.Errorf("run %q: %w", opts.ContainerName, err) } }() diff --git a/internal/pkg/docker/orchestrator/orchestrator_test.go b/internal/pkg/docker/orchestrator/orchestrator_test.go index 6e14133089b..a655ae66611 100644 --- a/internal/pkg/docker/orchestrator/orchestrator_test.go +++ b/internal/pkg/docker/orchestrator/orchestrator_test.go @@ -253,7 +253,9 @@ func TestOrchestrator(t *testing.T) { return func(t *testing.T, o *Orchestrator) { o.RunTask(Task{ Containers: map[string]ContainerDefinition{ - "foo": {}, + "foo": { + IsEssential: true, + }, }, }) }, de diff --git a/internal/pkg/graph/graph.go b/internal/pkg/graph/graph.go index 842e8f3b96a..85e13537902 100644 --- a/internal/pkg/graph/graph.go +++ b/internal/pkg/graph/graph.go @@ -24,8 +24,6 @@ const ( type Graph[V comparable] struct { vertices map[V]neighbors[V] // Adjacency list for each vertex. inDegrees map[V]int // Number of incoming edges for each vertex. - status map[V]string // status of each vertex. - lock sync.Mutex // lock used to mutate graph data. } // Edge represents one edge of a directed graph. @@ -37,34 +35,17 @@ type Edge[V comparable] struct { type neighbors[V comparable] map[V]bool // New initiates a new Graph. -func New[V comparable](vertices []V, opts ...GraphOption[V]) *Graph[V] { +func New[V comparable](vertices ...V) *Graph[V] { adj := make(map[V]neighbors[V]) inDegrees := make(map[V]int) for _, vertex := range vertices { adj[vertex] = make(neighbors[V]) inDegrees[vertex] = 0 } - g := &Graph[V]{ + return &Graph[V]{ vertices: adj, inDegrees: inDegrees, } - for _, opt := range opts { - opt(g) - } - return g -} - -// GraphOption allows you to initialize Graph with additional properties. -type GraphOption[V comparable] func(g *Graph[V]) - -// WithStatus sets the status of each vertex in the Graph. -func WithStatus[V comparable](status string) func(g *Graph[V]) { - return func(g *Graph[V]) { - g.status = make(map[V]string) - for vertex := range g.vertices { - g.status[vertex] = status - } - } } // Neighbors returns the list of connected vertices from vtx. @@ -161,77 +142,6 @@ func (g *Graph[V]) Roots() []V { return roots } -// updateStatus updates the status of a vertex. -func (g *Graph[V]) updateStatus(vertex V, status string) { - g.lock.Lock() - defer g.lock.Unlock() - g.status[vertex] = status -} - -// getStatus gets the status of a vertex. -func (g *Graph[V]) getStatus(vertex V) string { - g.lock.Lock() - defer g.lock.Unlock() - return g.status[vertex] -} - -// getLeaves returns the leaves of a given vertex. -func (g *Graph[V]) leaves() []V { - g.lock.Lock() - defer g.lock.Unlock() - var leaves []V - for vtx := range g.vertices { - if len(g.vertices[vtx]) == 0 { - leaves = append(leaves, vtx) - } - } - return leaves -} - -// getParents returns the parent vertices (incoming edges) of vertex. -func (g *Graph[V]) parents(vtx V) []V { - g.lock.Lock() - defer g.lock.Unlock() - var parents []V - for v, neighbors := range g.vertices { - if neighbors[vtx] { - parents = append(parents, v) - } - } - return parents -} - -// getChildren returns the child vertices (outgoing edges) of vertex. -func (g *Graph[V]) children(vtx V) []V { - g.lock.Lock() - defer g.lock.Unlock() - return g.Neighbors(vtx) -} - -// filterParents filters parents based on the vertex status. -func (g *Graph[V]) filterParents(vtx V, status string) []V { - parents := g.parents(vtx) - var filtered []V - for _, parent := range parents { - if g.getStatus(parent) == status { - filtered = append(filtered, parent) - } - } - return filtered -} - -// filterChildren filters children based on the vertex status. -func (g *Graph[V]) filterChildren(vtx V, status string) []V { - children := g.children(vtx) - var filtered []V - for _, child := range children { - if g.getStatus(child) == status { - filtered = append(filtered, child) - } - } - return filtered -} - func (g *Graph[V]) hasCycles(temp *findCycleTempVars[V], currVertex V) bool { temp.status[currVertex] = visiting for vertex := range g.vertices[currVertex] { @@ -293,13 +203,12 @@ func (alg *TopologicalSorter[V]) traverse(g *Graph[V]) { // // An example graph and their ranks is shown below to illustrate: // . -// ├── a rank: 0 -// │ ├── c rank: 1 -// │ │ └── f rank: 2 -// │ └── d rank: 1 -// └── b rank: 0 -// -// └── e rank: 1 +//├── a rank: 0 +//│ ├── c rank: 1 +//│ │ └── f rank: 2 +//│ └── d rank: 1 +//└── b rank: 0 +// └── e rank: 1 func TopologicalOrder[V comparable](digraph *Graph[V]) (*TopologicalSorter[V], error) { if vertices, isAcyclic := digraph.IsAcyclic(); !isAcyclic { return nil, &errCycle[V]{ @@ -314,6 +223,112 @@ func TopologicalOrder[V comparable](digraph *Graph[V]) (*TopologicalSorter[V], e return topo, nil } +// LabeledGraph extends a generic Graph by associating a label (or status) with each vertex. +// It is concurrency-safe, utilizing a mutex lock for synchronized access. +type LabeledGraph[V comparable] struct { + *Graph[V] + status map[V]string + lock sync.Mutex +} + +// NewLabeledGraph initializes a LabeledGraph with specified vertices and optional configurations. +// It creates a base Graph with the vertices and applies any LabeledGraphOption to configure additional properties. +func NewLabeledGraph[V comparable](vertices []V, opts ...LabeledGraphOption[V]) *LabeledGraph[V] { + g := New(vertices...) + lg := &LabeledGraph[V]{ + Graph: g, + status: make(map[V]string), + } + for _, opt := range opts { + opt(lg) + } + return lg +} + +// LabeledGraphOption allows you to initialize Graph with additional properties. +type LabeledGraphOption[V comparable] func(g *LabeledGraph[V]) + +// WithStatus sets the status of each vertex in the Graph. +func WithStatus[V comparable](status string) func(g *LabeledGraph[V]) { + return func(g *LabeledGraph[V]) { + g.status = make(map[V]string) + for vertex := range g.vertices { + g.status[vertex] = status + } + } +} + +// updateStatus updates the status of a vertex. +func (lg *LabeledGraph[V]) updateStatus(vertex V, status string) { + lg.lock.Lock() + defer lg.lock.Unlock() + lg.status[vertex] = status +} + +// getStatus gets the status of a vertex. +func (lg *LabeledGraph[V]) getStatus(vertex V) string { + lg.lock.Lock() + defer lg.lock.Unlock() + return lg.status[vertex] +} + +// getLeaves returns the leaves of a given vertex. +func (lg *LabeledGraph[V]) leaves() []V { + lg.lock.Lock() + defer lg.lock.Unlock() + var leaves []V + for vtx := range lg.vertices { + if len(lg.vertices[vtx]) == 0 { + leaves = append(leaves, vtx) + } + } + return leaves +} + +// getParents returns the parent vertices (incoming edges) of vertex. +func (lg *LabeledGraph[V]) parents(vtx V) []V { + lg.lock.Lock() + defer lg.lock.Unlock() + var parents []V + for v, neighbors := range lg.vertices { + if neighbors[vtx] { + parents = append(parents, v) + } + } + return parents +} + +// getChildren returns the child vertices (outgoing edges) of vertex. +func (lg *LabeledGraph[V]) children(vtx V) []V { + lg.lock.Lock() + defer lg.lock.Unlock() + return lg.Neighbors(vtx) +} + +// filterParents filters parents based on the vertex status. +func (lg *LabeledGraph[V]) filterParents(vtx V, status string) []V { + parents := lg.parents(vtx) + var filtered []V + for _, parent := range parents { + if lg.getStatus(parent) == status { + filtered = append(filtered, parent) + } + } + return filtered +} + +// filterChildren filters children based on the vertex status. +func (lg *LabeledGraph[V]) filterChildren(vtx V, status string) []V { + children := lg.children(vtx) + var filtered []V + for _, child := range children { + if lg.getStatus(child) == status { + filtered = append(filtered, child) + } + } + return filtered +} + /* UpwardTraversal performs an upward traversal on the graph starting from leaves (nodes with no children) and moving towards root nodes (nodes with children). @@ -322,18 +337,18 @@ It applies the specified process function to each vertex in the graph, skipping The traversal is concurrent and may process vertices in parallel. Returns an error if the traversal encounters any issues, or nil if successful. */ -func (g *Graph[V]) UpwardTraversal(ctx context.Context, processVertexFunc func(context.Context, V) error, adjacentVertexSkipStatus, requiredVertexStatus string) error { +func (lg *LabeledGraph[V]) UpwardTraversal(ctx context.Context, processVertexFunc func(context.Context, V) error, adjacentVertexSkipStatus, requiredVertexStatus string) error { traversal := &graphTraversal[V]{ mu: sync.Mutex{}, seen: make(map[V]struct{}), - findBoundaryVertices: func(g *Graph[V]) []V { return g.leaves() }, - findAdjacentVertices: func(g *Graph[V], v V) []V { return g.parents(v) }, - filterVerticesByStatus: func(g *Graph[V], v V, status string) []V { return g.filterChildren(v, status) }, + findBoundaryVertices: func(lg *LabeledGraph[V]) []V { return lg.leaves() }, + findAdjacentVertices: func(lg *LabeledGraph[V], v V) []V { return lg.parents(v) }, + filterVerticesByStatus: func(g *LabeledGraph[V], v V, status string) []V { return g.filterChildren(v, status) }, requiredVertexStatus: requiredVertexStatus, adjacentVertexSkipStatus: adjacentVertexSkipStatus, processVertex: processVertexFunc, } - return traversal.execute(ctx, g) + return traversal.execute(ctx, lg) } /* @@ -344,37 +359,37 @@ until reaching vertices with the "requiredVertexStatus" status. The traversal is concurrent and may process vertices in parallel. Returns an error if the traversal encounters any issues. */ -func (g *Graph[V]) DownwardTraversal(ctx context.Context, processVertexFunc func(context.Context, V) error, adjacentVertexSkipStatus, requiredVertexStatus string) error { +func (lg *LabeledGraph[V]) DownwardTraversal(ctx context.Context, processVertexFunc func(context.Context, V) error, adjacentVertexSkipStatus, requiredVertexStatus string) error { traversal := &graphTraversal[V]{ mu: sync.Mutex{}, seen: make(map[V]struct{}), - findBoundaryVertices: func(g *Graph[V]) []V { return g.Roots() }, - findAdjacentVertices: func(g *Graph[V], v V) []V { return g.children(v) }, - filterVerticesByStatus: func(g *Graph[V], v V, status string) []V { return g.filterParents(v, status) }, + findBoundaryVertices: func(lg *LabeledGraph[V]) []V { return lg.Roots() }, + findAdjacentVertices: func(lg *LabeledGraph[V], v V) []V { return lg.children(v) }, + filterVerticesByStatus: func(lg *LabeledGraph[V], v V, status string) []V { return lg.filterParents(v, status) }, requiredVertexStatus: requiredVertexStatus, adjacentVertexSkipStatus: adjacentVertexSkipStatus, processVertex: processVertexFunc, } - return traversal.execute(ctx, g) + return traversal.execute(ctx, lg) } type graphTraversal[V comparable] struct { mu sync.Mutex seen map[V]struct{} - findBoundaryVertices func(*Graph[V]) []V - findAdjacentVertices func(*Graph[V], V) []V - filterVerticesByStatus func(*Graph[V], V, string) []V + findBoundaryVertices func(*LabeledGraph[V]) []V + findAdjacentVertices func(*LabeledGraph[V], V) []V + filterVerticesByStatus func(*LabeledGraph[V], V, string) []V requiredVertexStatus string adjacentVertexSkipStatus string processVertex func(context.Context, V) error } -func (t *graphTraversal[V]) execute(ctx context.Context, graph *Graph[V]) error { +func (t *graphTraversal[V]) execute(ctx context.Context, lg *LabeledGraph[V]) error { ctx, cancel := context.WithCancel(ctx) defer cancel() - vertexCount := len(graph.vertices) + vertexCount := len(lg.vertices) if vertexCount == 0 { return nil } @@ -382,7 +397,7 @@ func (t *graphTraversal[V]) execute(ctx context.Context, graph *Graph[V]) error vertexCh := make(chan V, vertexCount) defer close(vertexCh) - processVertices := func(ctx context.Context, graph *Graph[V], eg *errgroup.Group, vertices []V, vertexCh chan V) { + processVertices := func(ctx context.Context, graph *LabeledGraph[V], eg *errgroup.Group, vertices []V, vertexCh chan V) { for _, vertex := range vertices { vertex := vertex // Delay processing this vertex if any of its dependent vertices are yet to be processed. @@ -409,17 +424,17 @@ func (t *graphTraversal[V]) execute(ctx context.Context, graph *Graph[V]) error for { select { case <-ctx.Done(): - return nil + return ctx.Err() case vertex := <-vertexCh: vertexCount-- if vertexCount == 0 { return nil } - processVertices(ctx, graph, eg, t.findAdjacentVertices(graph, vertex), vertexCh) + processVertices(ctx, lg, eg, t.findAdjacentVertices(lg, vertex), vertexCh) } } }) - processVertices(ctx, graph, eg, t.findBoundaryVertices(graph), vertexCh) + processVertices(ctx, lg, eg, t.findBoundaryVertices(lg), vertexCh) return eg.Wait() } diff --git a/internal/pkg/graph/graph_test.go b/internal/pkg/graph/graph_test.go index 4c857af9a02..8cbee84c37c 100644 --- a/internal/pkg/graph/graph_test.go +++ b/internal/pkg/graph/graph_test.go @@ -14,7 +14,7 @@ import ( func TestGraph_Add(t *testing.T) { t.Run("success", func(t *testing.T) { // GIVEN - graph := New[string]([]string{}) + graph := New[string]() // WHEN // A <-> B @@ -45,7 +45,7 @@ func TestGraph_InDegree(t *testing.T) { wanted map[rune]int }{ "should return 0 for nodes that don't exist in the graph": { - graph: New[rune]([]rune{}), + graph: New[rune](), wanted: map[rune]int{ 'a': 0, @@ -53,7 +53,7 @@ func TestGraph_InDegree(t *testing.T) { }, "should return number of incoming edges for complex graph": { graph: func() *Graph[rune] { - g := New[rune]([]rune{}) + g := New[rune]() g.Add(Edge[rune]{'a', 'b'}) g.Add(Edge[rune]{'b', 'a'}) g.Add(Edge[rune]{'a', 'c'}) @@ -89,7 +89,7 @@ func TestGraph_Remove(t *testing.T) { }{ "edge deletion should be idempotent": { graph: func() *Graph[rune] { - g := New[rune]([]rune{}) + g := New[rune]() g.Add(Edge[rune]{'a', 'b'}) g.Add(Edge[rune]{'z', 'b'}) g.Remove(Edge[rune]{'a', 'b'}) @@ -124,13 +124,13 @@ func TestGraph_Remove(t *testing.T) { func TestGraph_IsAcyclic(t *testing.T) { testCases := map[string]struct { - graph *Graph[string] + graph Graph[string] isAcyclic bool cycle []string }{ "small non acyclic graph": { - graph: &Graph[string]{ + graph: Graph[string]{ vertices: map[string]neighbors[string]{ "A": {"B": true, "C": true}, "B": {"A": true}, @@ -141,7 +141,7 @@ func TestGraph_IsAcyclic(t *testing.T) { cycle: []string{"A", "B"}, }, "non acyclic": { - graph: &Graph[string]{ + graph: Graph[string]{ vertices: map[string]neighbors[string]{ "K": {"F": true}, "A": {"B": true, "C": true}, @@ -156,7 +156,7 @@ func TestGraph_IsAcyclic(t *testing.T) { cycle: []string{"A", "G", "E", "B"}, }, "acyclic": { - graph: &Graph[string]{ + graph: Graph[string]{ vertices: map[string]neighbors[string]{ "A": {"B": true, "C": true}, "B": {"D": true}, @@ -187,15 +187,15 @@ func TestGraph_Roots(t *testing.T) { wantedRoots []int }{ "should return nil if the graph is empty": { - graph: New[int]([]int{}), + graph: New[int](), }, "should return all the vertices if there are no edges in the graph": { - graph: New[int]([]int{1, 2, 3, 4, 5}), + graph: New[int](1, 2, 3, 4, 5), wantedRoots: []int{1, 2, 3, 4, 5}, }, "should return only vertices with no in degrees": { graph: func() *Graph[int] { - g := New[int]([]int{}) + g := New[int]() g.Add(Edge[int]{ From: 1, To: 3, @@ -232,7 +232,7 @@ func TestTopologicalOrder(t *testing.T) { "should return an error when a cycle is detected": { // frontend <-> backend graph: func() *Graph[string] { - g := New([]string{"frontend", "backend"}) + g := New("frontend", "backend") g.Add(Edge[string]{ From: "frontend", To: "backend", @@ -248,7 +248,7 @@ func TestTopologicalOrder(t *testing.T) { "should return the ranks for a graph that looks like a bus": { // vpc -> lb -> api graph: func() *Graph[string] { - g := New[string]([]string{}) + g := New[string]() g.Add(Edge[string]{ From: "vpc", To: "lb", @@ -271,7 +271,7 @@ func TestTopologicalOrder(t *testing.T) { // vpc -> rds -> backend // -> s3 -> api // -> frontend - g := New[string]([]string{}) + g := New[string]() g.Add(Edge[string]{ From: "vpc", To: "rds", @@ -308,7 +308,7 @@ func TestTopologicalOrder(t *testing.T) { graph: func() *Graph[string] { // warehouse -> orders -> frontend // payments -> - g := New[string]([]string{}) + g := New[string]() g.Add(Edge[string]{ From: "payments", To: "frontend", @@ -335,7 +335,7 @@ func TestTopologicalOrder(t *testing.T) { graph: func() *Graph[string] { // a -> b -> c -> d -> f // a -> e -> f - g := New[string]([]string{}) + g := New[string]() for _, edge := range []Edge[string]{{"a", "b"}, {"b", "c"}, {"c", "d"}, {"d", "f"}, {"a", "e"}, {"e", "f"}} { g.Add(edge) } @@ -371,9 +371,9 @@ func TestTopologicalOrder(t *testing.T) { } } -func buildGraphWithSingleParent() *Graph[string] { +func buildGraphWithSingleParent() *LabeledGraph[string] { vertices := []string{"A", "B", "C", "D"} - graph := New[string](vertices, WithStatus[string]("started")) + graph := NewLabeledGraph[string](vertices, WithStatus[string]("started")) graph.Add(Edge[string]{From: "D", To: "C"}) // D -> C graph.Add(Edge[string]{From: "C", To: "B"}) // C -> B graph.Add(Edge[string]{From: "B", To: "A"}) // B -> A @@ -395,7 +395,7 @@ func TestTraverseInDependencyOrder(t *testing.T) { }) t.Run("graph with multiple parents and boundary nodes", func(t *testing.T) { vertices := []string{"A", "B", "C", "D"} - graph := New[string](vertices, WithStatus[string]("started")) + graph := NewLabeledGraph[string](vertices, WithStatus[string]("started")) graph.Add(Edge[string]{From: "A", To: "C"}) graph.Add(Edge[string]{From: "A", To: "D"}) graph.Add(Edge[string]{From: "B", To: "D"}) diff --git a/internal/pkg/manifest/validate.go b/internal/pkg/manifest/validate.go index 92aec976dc5..c44fa02ca22 100644 --- a/internal/pkg/manifest/validate.go +++ b/internal/pkg/manifest/validate.go @@ -2313,7 +2313,7 @@ func validateNoCircularDependencies(deps map[string]ContainerDependency) error { } func buildDependencyGraph(deps map[string]ContainerDependency) (*graph.Graph[string], error) { - dependencyGraph := graph.New[string]([]string{}) + dependencyGraph := graph.New[string]() for name, containerDep := range deps { for dep := range containerDep.DependsOn { if _, ok := deps[dep]; !ok { From 8efb61aba2f3abc16151888d26219247f4dc9503 Mon Sep 17 00:00:00 2001 From: AdithyaKolla Date: Tue, 5 Dec 2023 21:39:05 -0800 Subject: [PATCH 20/53] add exitcode interface --- .../pkg/docker/dockerengine/dockerengine.go | 17 ++++++++++- .../docker/dockerengine/dockerengine_test.go | 28 ++++++++++++++++--- 2 files changed, 40 insertions(+), 5 deletions(-) diff --git a/internal/pkg/docker/dockerengine/dockerengine.go b/internal/pkg/docker/dockerengine/dockerengine.go index f71c30c937c..123e9ab0383 100644 --- a/internal/pkg/docker/dockerengine/dockerengine.go +++ b/internal/pkg/docker/dockerengine/dockerengine.go @@ -9,6 +9,7 @@ import ( "bytes" "context" "encoding/json" + "errors" "fmt" "io" "os" @@ -263,7 +264,6 @@ func (c DockerCmdClient) Push(ctx context.Context, uri string, w io.Writer, tags func (in *RunOptions) generateRunArguments() []string { args := []string{"run"} - args = append(args, "--rm") if in.ContainerName != "" { args = append(args, "--name", in.ContainerName) @@ -303,6 +303,9 @@ func (in *RunOptions) generateRunArguments() []string { // Run runs a Docker container with the sepcified options. func (c DockerCmdClient) Run(ctx context.Context, options *RunOptions) error { + type exitCodeError interface { + ExitCode() int + } // set default options if options.LogOptions.Color == nil { options.LogOptions.Color = color.New() @@ -343,6 +346,13 @@ func (c DockerCmdClient) Run(ctx context.Context, options *RunOptions) error { exec.Stdout(stdout), exec.Stderr(stderr), exec.NewProcessGroup()); err != nil { + var ec exitCodeError + if errors.As(err, &ec) { + return &ErrContainerExited{ + name: options.ContainerName, + exitcode: ec.ExitCode(), + } + } return fmt.Errorf("running container: %w", err) } return nil @@ -450,6 +460,11 @@ type ErrContainerExited struct { exitcode int } +// ExitCode returns the OS exit code configured for this error. +func (e *ErrContainerExited) ExitCode() int { + return e.exitcode +} + // ErrContainerExited represents docker container exited with an exitcode. func (e *ErrContainerExited) Error() string { return fmt.Sprintf("container %q exited with code %d", e.name, e.exitcode) diff --git a/internal/pkg/docker/dockerengine/dockerengine_test.go b/internal/pkg/docker/dockerengine/dockerengine_test.go index d8cd130f254..7f291be8d77 100644 --- a/internal/pkg/docker/dockerengine/dockerengine_test.go +++ b/internal/pkg/docker/dockerengine/dockerengine_test.go @@ -9,6 +9,7 @@ import ( "errors" "fmt" "io" + "os" osexec "os/exec" "path/filepath" "strings" @@ -680,13 +681,35 @@ func TestDockerCommand_Run(t *testing.T) { setupMocks: func(controller *gomock.Controller) { mockCmd = NewMockCmd(controller) mockCmd.EXPECT().RunWithContext(gomock.Any(), "docker", []string{"run", - "--rm", "--name", mockPauseContainer, "mockImageUri", "sleep", "infinity"}, gomock.Any(), gomock.Any(), gomock.Any()).Return(mockError) }, wantedError: fmt.Errorf("running container: %w", mockError), }, + + "should return error when container exits": { + containerName: mockPauseContainer, + command: mockCommand, + uri: mockImageURI, + setupMocks: func(controller *gomock.Controller) { + mockCmd = NewMockCmd(controller) + + mockCmd.EXPECT().RunWithContext(gomock.Any(), "docker", []string{"run", + "--name", mockPauseContainer, + "mockImageUri", + "sleep", "infinity"}, gomock.Any(), gomock.Any(), gomock.Any()). + DoAndReturn(func(ctx context.Context, name string, args []string, opts ...exec.CmdOption) error { + // Simulate an zero exit code. + return &osexec.ExitError{ProcessState: &os.ProcessState{}} + }) + }, + wantedError: &ErrContainerExited{ + name: mockPauseContainer, + exitcode: 0, + }, + }, + "success with run options for pause container": { containerName: mockPauseContainer, ports: mockContainerPorts, @@ -695,7 +718,6 @@ func TestDockerCommand_Run(t *testing.T) { setupMocks: func(controller *gomock.Controller) { mockCmd = NewMockCmd(controller) mockCmd.EXPECT().RunWithContext(gomock.Any(), "docker", gomock.InAnyOrder([]string{"run", - "--rm", "--name", mockPauseContainer, "--publish", "8080:8080", "--publish", "8081:8081", @@ -712,7 +734,6 @@ func TestDockerCommand_Run(t *testing.T) { setupMocks: func(controller *gomock.Controller) { mockCmd = NewMockCmd(controller) mockCmd.EXPECT().RunWithContext(gomock.Any(), "docker", gomock.InAnyOrder([]string{"run", - "--rm", "--name", mockContainerName, "--network", "container:pauseContainer", "--env", "DB_PASSWORD=mysecretPassword", @@ -733,7 +754,6 @@ func TestDockerCommand_Run(t *testing.T) { setupMocks: func(controller *gomock.Controller) { mockCmd = NewMockCmd(controller) mockCmd.EXPECT().RunWithContext(gomock.Any(), "docker", gomock.InAnyOrder([]string{"run", - "--rm", "--name", mockContainerName, "--network", "container:pauseContainer", "--env", "DB_PASSWORD=mysecretPassword", From 2376957022b00ab93fb9763209c8254a547e5fb1 Mon Sep 17 00:00:00 2001 From: AdithyaKolla Date: Tue, 5 Dec 2023 22:05:30 -0800 Subject: [PATCH 21/53] add status to buildGraph --- internal/pkg/docker/orchestrator/orchestrator.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/pkg/docker/orchestrator/orchestrator.go b/internal/pkg/docker/orchestrator/orchestrator.go index eb0f7624ec7..27ee7af269f 100644 --- a/internal/pkg/docker/orchestrator/orchestrator.go +++ b/internal/pkg/docker/orchestrator/orchestrator.go @@ -241,7 +241,7 @@ func (a *runTaskAction) Do(o *Orchestrator) error { time.Sleep(1 * time.Second) } o.curTask = a.task - depGraph := buildDependencyGraph(a.task.Containers) + depGraph := buildDependencyGraph(a.task.Containers, ctrStateUnknown) err := depGraph.UpwardTraversal(ctx, func(ctx context.Context, containerName string) error { if len(a.task.Containers[containerName].DependsOn) > 0 { if err := o.waitForContainerDependencies(ctx, containerName, a); err != nil { @@ -260,12 +260,12 @@ func (a *runTaskAction) Do(o *Orchestrator) error { return nil } -func buildDependencyGraph(containers map[string]ContainerDefinition) *graph.LabeledGraph[string] { +func buildDependencyGraph(containers map[string]ContainerDefinition, status string) *graph.LabeledGraph[string] { var vertices []string for vertex := range containers { vertices = append(vertices, vertex) } - dependencyGraph := graph.NewLabeledGraph(vertices, graph.WithStatus[string](ctrStateUnknown)) + dependencyGraph := graph.NewLabeledGraph(vertices, graph.WithStatus[string](status)) for containerName, container := range containers { for depCtr := range container.DependsOn { dependencyGraph.Add(graph.Edge[string]{ From ad1afa80697d3c3a1162f6e9a49e5d35d042a08e Mon Sep 17 00:00:00 2001 From: AdithyaKolla Date: Tue, 5 Dec 2023 22:46:20 -0800 Subject: [PATCH 22/53] add buffered channel and curTask --- internal/pkg/docker/orchestrator/orchestrator.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/internal/pkg/docker/orchestrator/orchestrator.go b/internal/pkg/docker/orchestrator/orchestrator.go index 27ee7af269f..670deec5359 100644 --- a/internal/pkg/docker/orchestrator/orchestrator.go +++ b/internal/pkg/docker/orchestrator/orchestrator.go @@ -112,7 +112,8 @@ func New(docker DockerEngine, idPrefix string, logOptions logOptionsFunc) *Orche func (o *Orchestrator) Start() <-chan error { // close done when all goroutines created by Orchestrator have finished done := make(chan struct{}) - errs := make(chan error) + // buffered channel so that the orchestrator routine can always send the errors from runErrs and action.Do + errs := make(chan error, 1) // orchestrator routine o.wg.Add(1) // decremented by stopAction @@ -206,6 +207,7 @@ func (a *runTaskAction) Do(o *Orchestrator) error { }() if taskID == 1 { + o.curTask = a.task if err := o.buildPauseContainer(ctx); err != nil { return fmt.Errorf("build pause container: %w", err) } From 06ac0b9dab936554099d6bd1bb7df5e58f230349 Mon Sep 17 00:00:00 2001 From: AdithyaKolla Date: Wed, 6 Dec 2023 01:31:29 -0800 Subject: [PATCH 23/53] remove containers in reverse dependency order --- .../pkg/docker/orchestrator/orchestrator.go | 70 +++++++++++-------- 1 file changed, 42 insertions(+), 28 deletions(-) diff --git a/internal/pkg/docker/orchestrator/orchestrator.go b/internal/pkg/docker/orchestrator/orchestrator.go index 670deec5359..131c8b0fbf7 100644 --- a/internal/pkg/docker/orchestrator/orchestrator.go +++ b/internal/pkg/docker/orchestrator/orchestrator.go @@ -21,6 +21,7 @@ import ( "github.com/aws/copilot-cli/internal/pkg/docker/dockerengine" "github.com/aws/copilot-cli/internal/pkg/graph" + "github.com/aws/copilot-cli/internal/pkg/term/color" "github.com/aws/copilot-cli/internal/pkg/term/log" "golang.org/x/sync/errgroup" ) @@ -76,7 +77,8 @@ const ( const ( ctrStateUnknown = "unknown" - ctrStateRunningOrExited = "RunningOrExited" + ctrStateRunningOrExited = "runningOrExited" + ctrStateRemoved = "removed" ) const ( @@ -112,7 +114,7 @@ func New(docker DockerEngine, idPrefix string, logOptions logOptionsFunc) *Orche func (o *Orchestrator) Start() <-chan error { // close done when all goroutines created by Orchestrator have finished done := make(chan struct{}) - // buffered channel so that the orchestrator routine can always send the errors from runErrs and action.Do + // buffered channel so that the orchestrator routine does not block and can always send the errors from runErrs and action.Do errs := make(chan error, 1) // orchestrator routine @@ -403,7 +405,6 @@ func (o *Orchestrator) buildPauseContainer(ctx context.Context) error { func (o *Orchestrator) Stop() { o.stopOnce.Do(func() { close(o.stopped) - fmt.Printf("\nStopping task...\n") o.actions <- &stopAction{} }) } @@ -414,6 +415,7 @@ func (a *stopAction) Do(o *Orchestrator) error { defer o.wg.Done() // for the Orchestrator o.curTaskID.Store(orchestratorStoppedTaskID) // ignore runtime errors + fmt.Printf("\nStopping task...\n") // collect errors since we want to try to clean up everything we can var errs []error if err := o.stopTask(context.Background(), o.curTask); err != nil { @@ -440,22 +442,25 @@ func (o *Orchestrator) stopTask(ctx context.Context, task Task) error { } // errCh gets one error per container - errCh := make(chan error) - for name := range task.Containers { - name := name - go func() { - fmt.Printf("Stopping and Removing %q\n", name) - if err := o.docker.Stop(ctx, o.containerID(name)); err != nil { - errCh <- fmt.Errorf("stop %q: %w", name, err) - return - } - if err := o.docker.Rm(ctx, o.containerID(name)); err != nil { - errCh <- fmt.Errorf("remove %q: %w", name, err) - return - } - fmt.Printf("Stopped and Removed %q\n", name) - errCh <- nil - }() + errCh := make(chan error, len(task.Containers)) + depGraph := buildDependencyGraph(task.Containers, ctrStateRunningOrExited) + err := depGraph.DownwardTraversal(ctx, func(ctx context.Context, name string) error { + fmt.Printf("Stopping and Removing %q\n", name) + if err := o.docker.Stop(ctx, o.containerID(name)); err != nil { + errCh <- fmt.Errorf("stop %q: %w", name, err) + return nil + } + if err := o.docker.Rm(ctx, o.containerID(name)); err != nil { + errCh <- fmt.Errorf("remove %q: %w", name, err) + return nil + } + fmt.Printf("Stopped and Removed %q\n", name) + errCh <- nil + return nil + }, ctrStateRunningOrExited, ctrStateRemoved) + + if err != nil { + return fmt.Errorf("downward traversal: %w", err) } var errs []error @@ -492,7 +497,12 @@ func (o *Orchestrator) waitForContainerToStart(ctx context.Context, id string, i } func (o *Orchestrator) waitForContainerDependencies(ctx context.Context, containerName string, a *runTaskAction) error { - fmt.Printf("Waiting for container %q dependencies...\n", containerName) + var deps []string + for depName, state := range a.task.Containers[containerName].DependsOn { + deps = append(deps, fmt.Sprintf("%s->%s", depName, state)) + } + logMsg := strings.Join(deps, ", ") + fmt.Printf("Waiting for container %q dependencies: [%s]\n", containerName, color.Emphasize(logMsg)) eg, ctx := errgroup.WithContext(ctx) for name, state := range a.task.Containers[containerName].DependsOn { name, state := name, state @@ -510,10 +520,11 @@ func (o *Orchestrator) waitForContainerDependencies(ctx context.Context, contain if err != nil { return err } - if !isEssential { - fmt.Printf("non-essential container %q started successfully\n", ctrId) + if isEssential { + log.Successf("Successfully container %s is running\n", ctrId) + return nil } - log.Successf("Successfully container %s is running\n", ctrId) + fmt.Printf("non-essential container %q started successfully\n", ctrId) return nil case ctrStateHealthy: healthy, err := o.docker.IsContainerHealthy(ctx, ctrId) @@ -534,7 +545,7 @@ func (o *Orchestrator) waitForContainerDependencies(ctx context.Context, contain return fmt.Errorf("dependency container %q failed to be %q: %w", ctrId, state, err) } if exitCode != -1 { - log.Successf("Successfully dependency container %q exited with code: %d\n", name, exitCode) + log.Successf("Successfully dependency container %q exited with code: %d\n", ctrId, exitCode) return nil } case ctrStateSuccess: @@ -542,13 +553,13 @@ func (o *Orchestrator) waitForContainerDependencies(ctx context.Context, contain if err != nil { return fmt.Errorf("dependency container %q failed to be %q: %w", ctrId, state, err) } + if exitCode > 0 { + return fmt.Errorf("dependency container %q exited with code %d", ctrId, exitCode) + } if exitCode == 0 { log.Successf("Successfully dependency container %q reached %q\n", ctrId, state) return nil } - if exitCode > 0 { - return fmt.Errorf("dependency container %q failed to be %q: %w", ctrId, state, err) - } } case <-ctx.Done(): return ctx.Err() @@ -640,7 +651,10 @@ func (o *Orchestrator) run(taskID int32, opts dockerengine.RunOptions, isEssenti if errors.As(err, &errContainerExited) && !isEssential { return } - if err == nil && isEssential { + if err == nil { + if !isEssential { + return + } err = errors.New("container stopped unexpectedly") } // cancel context to indicate all the other go routines spawned by `graph.UpwardTarversal`. From 226448b5a15a1e30d836cb97d08cff1fe3608130 Mon Sep 17 00:00:00 2001 From: AdithyaKolla Date: Wed, 6 Dec 2023 02:47:38 -0800 Subject: [PATCH 24/53] reolve conflicts --- internal/pkg/cli/deploy/workload.go | 12 ------------ internal/pkg/cli/run_local.go | 4 +--- internal/pkg/manifest/workload.go | 12 ++++++++++++ 3 files changed, 13 insertions(+), 15 deletions(-) diff --git a/internal/pkg/cli/deploy/workload.go b/internal/pkg/cli/deploy/workload.go index 1be4d0d255c..37b93b6f533 100644 --- a/internal/pkg/cli/deploy/workload.go +++ b/internal/pkg/cli/deploy/workload.go @@ -738,18 +738,6 @@ func envFiles(unmarshaledManifest interface{}) map[string]string { return nil } -// ContainerDependencies returns a map of ContainerDependency objects from workload manifest. -func ContainerDependencies(unmarshaledManifest interface{}) map[string]manifest.ContainerDependency { - type containerDependency interface { - ContainerDependencies() map[string]manifest.ContainerDependency - } - mf, ok := unmarshaledManifest.(containerDependency) - if ok { - return mf.ContainerDependencies() - } - return nil -} - func (d *workloadDeployer) pushAddonsTemplateToS3Bucket() (string, error) { if d.addons == nil { return "", nil diff --git a/internal/pkg/cli/run_local.go b/internal/pkg/cli/run_local.go index 4991dfdfc9d..56cc6154f52 100644 --- a/internal/pkg/cli/run_local.go +++ b/internal/pkg/cli/run_local.go @@ -519,9 +519,7 @@ func (o *runLocalOpts) prepareTask(ctx context.Context) (orchestrator.Task, erro task.Containers[name] = ctr } - // TODO (Adi): Use this dependency order in orchestrator to start and stop containers. - // replace container dependencies with the local dependencies from manifest. - containerDeps := clideploy.ContainerDependencies(mft.Manifest()) + containerDeps := manifest.ContainerDependencies(mft.Manifest()) for name, dep := range containerDeps { ctr, ok := task.Containers[name] if !ok { diff --git a/internal/pkg/manifest/workload.go b/internal/pkg/manifest/workload.go index 23a17c2cd26..a05f51e1168 100644 --- a/internal/pkg/manifest/workload.go +++ b/internal/pkg/manifest/workload.go @@ -864,3 +864,15 @@ func (cfg PublishConfig) publishedTopics() []Topic { } return pubs } + +// ContainerDependencies returns a map of ContainerDependency objects from workload manifest. +func ContainerDependencies(unmarshaledManifest interface{}) map[string]ContainerDependency { + type containerDependency interface { + ContainerDependencies() map[string]ContainerDependency + } + mf, ok := unmarshaledManifest.(containerDependency) + if ok { + return mf.ContainerDependencies() + } + return nil +} From 70be81a8d5a658e103896a15def02835650a584b Mon Sep 17 00:00:00 2001 From: AdithyaKolla Date: Wed, 6 Dec 2023 04:03:28 -0800 Subject: [PATCH 25/53] merge changes of doesContainerExist --- internal/pkg/cli/interfaces.go | 1 + internal/pkg/cli/mocks/mock_interfaces.go | 15 +++++ .../pkg/docker/dockerengine/dockerengine.go | 9 +++ .../dockerenginetest/dockerenginetest.go | 42 ++++++++------ .../pkg/docker/orchestrator/orchestrator.go | 26 ++++++++- .../docker/orchestrator/orchestrator_test.go | 56 +++++++++---------- 6 files changed, 102 insertions(+), 47 deletions(-) diff --git a/internal/pkg/cli/interfaces.go b/internal/pkg/cli/interfaces.go index cf95375297e..7e1f11759c6 100644 --- a/internal/pkg/cli/interfaces.go +++ b/internal/pkg/cli/interfaces.go @@ -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 Build(context.Context, *dockerengine.BuildArguments, io.Writer) error diff --git a/internal/pkg/cli/mocks/mock_interfaces.go b/internal/pkg/cli/mocks/mock_interfaces.go index 718d83fb9ae..60d46fbc3ee 100644 --- a/internal/pkg/cli/mocks/mock_interfaces.go +++ b/internal/pkg/cli/mocks/mock_interfaces.go @@ -7693,6 +7693,21 @@ func (mr *MockdockerEngineRunnerMockRecorder) CheckDockerEngineRunning() *gomock return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CheckDockerEngineRunning", reflect.TypeOf((*MockdockerEngineRunner)(nil).CheckDockerEngineRunning)) } +// DoesContainerExist mocks base method. +func (m *MockdockerEngineRunner) DoesContainerExist(arg0 context.Context, arg1 string) (bool, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "DoesContainerExist", arg0, arg1) + ret0, _ := ret[0].(bool) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// DoesContainerExist indicates an expected call of DoesContainerExist. +func (mr *MockdockerEngineRunnerMockRecorder) DoesContainerExist(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DoesContainerExist", reflect.TypeOf((*MockdockerEngineRunner)(nil).DoesContainerExist), arg0, arg1) +} + // Exec mocks base method. func (m *MockdockerEngineRunner) Exec(ctx context.Context, container string, out io.Writer, cmd string, args ...string) error { m.ctrl.T.Helper() diff --git a/internal/pkg/docker/dockerengine/dockerengine.go b/internal/pkg/docker/dockerengine/dockerengine.go index 123e9ab0383..cd288130800 100644 --- a/internal/pkg/docker/dockerengine/dockerengine.go +++ b/internal/pkg/docker/dockerengine/dockerengine.go @@ -361,6 +361,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) diff --git a/internal/pkg/docker/dockerengine/dockerenginetest/dockerenginetest.go b/internal/pkg/docker/dockerengine/dockerenginetest/dockerenginetest.go index 545b8a13377..a8147a0b1ca 100644 --- a/internal/pkg/docker/dockerengine/dockerenginetest/dockerenginetest.go +++ b/internal/pkg/docker/dockerengine/dockerenginetest/dockerenginetest.go @@ -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 @@ -22,22 +23,6 @@ type Double struct { RmFn func(context.Context, string) error } -// IsContainerCompleteOrSuccess implements orchestrator.DockerEngine. -func (d *Double) IsContainerCompleteOrSuccess(ctx context.Context, containerName string) (int, error) { - if d.IsContainerCompleteOrSuccessFn == nil { - return -1, nil - } - return d.IsContainerCompleteOrSuccessFn(ctx, containerName) -} - -// IsContainerHealthy implements orchestrator.DockerEngine. -func (d *Double) IsContainerHealthy(ctx context.Context, containerName string) (bool, error) { - if d.IsContainerHealthyFn == nil { - return false, nil - } - return d.IsContainerHealthyFn(ctx, containerName) -} - // Stop calls the stubbed function. func (d *Double) Stop(ctx context.Context, name string) error { if d.StopFn == nil { @@ -46,6 +31,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 { @@ -78,9 +71,26 @@ func (d *Double) Exec(ctx context.Context, container string, out io.Writer, cmd return d.ExecFn(ctx, container, out, cmd, args...) } +// Rm calls the stubbed function. func (d *Double) Rm(ctx context.Context, name string) error { if d.RmFn == nil { return nil } return d.RmFn(ctx, name) } + +// IsContainerCompleteOrSuccess implements orchestrator.DockerEngine. +func (d *Double) IsContainerCompleteOrSuccess(ctx context.Context, containerName string) (int, error) { + if d.IsContainerCompleteOrSuccessFn == nil { + return -1, nil + } + return d.IsContainerCompleteOrSuccessFn(ctx, containerName) +} + +// IsContainerHealthy implements orchestrator.DockerEngine. +func (d *Double) IsContainerHealthy(ctx context.Context, containerName string) (bool, error) { + if d.IsContainerHealthyFn == nil { + return false, nil + } + return d.IsContainerHealthyFn(ctx, containerName) +} diff --git a/internal/pkg/docker/orchestrator/orchestrator.go b/internal/pkg/docker/orchestrator/orchestrator.go index fb6b1980083..7150032ecfe 100644 --- a/internal/pkg/docker/orchestrator/orchestrator.go +++ b/internal/pkg/docker/orchestrator/orchestrator.go @@ -52,6 +52,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) IsContainerCompleteOrSuccess(ctx context.Context, containerName string) (int, error) IsContainerHealthy(ctx context.Context, containerName string) (bool, error) @@ -450,9 +451,28 @@ func (o *Orchestrator) stopTask(ctx context.Context, task Task) error { errCh <- fmt.Errorf("remove %q: %w", name, err) return nil } - fmt.Printf("Stopped and Removed %q\n", name) - errCh <- nil - return nil + // ensure that container is fully stopped before stopTask finishes blocking + for { + running, err := o.docker.DoesContainerExist(ctx, o.containerID(name)) + if err != nil { + errCh <- fmt.Errorf("polling container %q for removal: %w", name, err) + return nil + } + + if running { + select { + case <-time.After(1 * time.Second): + continue + case <-ctx.Done(): + errCh <- fmt.Errorf("check container %q stopped: %w", name, ctx.Err()) + return nil + } + } + + fmt.Printf("Stopped and Removed %q\n", name) + errCh <- nil + return nil + } }, ctrStateRunningOrExited, ctrStateRemoved) if err != nil { diff --git a/internal/pkg/docker/orchestrator/orchestrator_test.go b/internal/pkg/docker/orchestrator/orchestrator_test.go index 5b89b691073..331ac807956 100644 --- a/internal/pkg/docker/orchestrator/orchestrator_test.go +++ b/internal/pkg/docker/orchestrator/orchestrator_test.go @@ -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 @@ -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 }, @@ -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{ @@ -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" { @@ -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") @@ -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 @@ -422,12 +422,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") From 1d90997c16a2797a5d0bf54a28d6accb6d040995 Mon Sep 17 00:00:00 2001 From: AdithyaKolla Date: Wed, 6 Dec 2023 09:38:24 -0800 Subject: [PATCH 26/53] add tests --- .../docker/orchestrator/orchestrator_test.go | 110 +++++++++++++++++- 1 file changed, 109 insertions(+), 1 deletion(-) diff --git a/internal/pkg/docker/orchestrator/orchestrator_test.go b/internal/pkg/docker/orchestrator/orchestrator_test.go index 331ac807956..478141e874e 100644 --- a/internal/pkg/docker/orchestrator/orchestrator_test.go +++ b/internal/pkg/docker/orchestrator/orchestrator_test.go @@ -229,6 +229,103 @@ func TestOrchestrator(t *testing.T) { }, de }, }, + + "return nil error if non essential container exits": { + logOptions: noLogs, + runUntilStopped: true, + test: func(t *testing.T) (test, *dockerenginetest.Double) { + de := &dockerenginetest.Double{ + DoesContainerExistFn: func(ctx context.Context, s string) (bool, error) { + return false, nil + }, + IsContainerRunningFn: func(ctx context.Context, name string) (bool, error) { + if name == "prefix-foo" { + return false, fmt.Errorf("container %q exited with code %d", name, 143) + } + return true, nil + }, + RunFn: func(ctx context.Context, opts *dockerengine.RunOptions) error { + return nil + }, + StopFn: func(ctx context.Context, name string) error { + return nil + }, + IsContainerCompleteOrSuccessFn: func(ctx context.Context, containerName string) (int, error) { + if containerName == "prefix-foo" { + return 143, nil + } + return -1, nil + }, + } + return func(t *testing.T, o *Orchestrator) { + o.RunTask(Task{ + Containers: map[string]ContainerDefinition{ + "foo": { + IsEssential: false, + }, + "bar": { + IsEssential: true, + }, + }, + }) + }, de + }, + }, + + "success with dependsOn order": { + logOptions: noLogs, + test: func(t *testing.T) (test, *dockerenginetest.Double) { + stopPause := make(chan struct{}) + stopFoo := make(chan struct{}) + de := &dockerenginetest.Double{ + IsContainerRunningFn: func(ctx context.Context, name string) (bool, error) { + return true, nil + }, + DoesContainerExistFn: func(ctx context.Context, s string) (bool, error) { + return false, nil + }, + RunFn: func(ctx context.Context, opts *dockerengine.RunOptions) error { + if opts.ContainerName == "prefix-bar" { + return errors.New("container `prefix-bar` exited with code 143") + } + if opts.ContainerName == "prefix-pause" { + // block pause container until Stop(pause) + <-stopPause + } + if opts.ContainerName == "prefix-foo" { + // block bar container until Stop(foo) + <-stopFoo + } + return nil + }, + StopFn: func(ctx context.Context, s string) error { + if s == "prefix-pause" { + stopPause <- struct{}{} + } + if s == "prefix-foo" { + stopFoo <- struct{}{} + } + return nil + }, + } + return func(t *testing.T, o *Orchestrator) { + o.RunTask(Task{ + Containers: map[string]ContainerDefinition{ + "foo": { + IsEssential: true, + }, + "bar": { + IsEssential: false, + DependsOn: map[string]string{ + "foo": ctrStateStart, + }, + }, + }, + }) + }, de + }, + }, + "container run stops early with error": { logOptions: noLogs, test: func(t *testing.T) (test, *dockerenginetest.Double) { @@ -240,6 +337,9 @@ func TestOrchestrator(t *testing.T) { IsContainerRunningFn: func(ctx context.Context, name string) (bool, error) { return true, nil }, + IsContainerCompleteOrSuccessFn: func(ctx context.Context, containerName string) (int, error) { + return -1, nil + }, RunFn: func(ctx context.Context, opts *dockerengine.RunOptions) error { if opts.ContainerName == "prefix-foo" { return errors.New("some error") @@ -259,7 +359,15 @@ func TestOrchestrator(t *testing.T) { return func(t *testing.T, o *Orchestrator) { o.RunTask(Task{ Containers: map[string]ContainerDefinition{ - "foo": {}, + "foo": { + IsEssential: true, + }, + "bar": { + IsEssential: true, + DependsOn: map[string]string{ + "foo": "start", + }, + }, }, }) }, de From 25e074cc0bc3cc5e7abb9a618f80083d803eec20 Mon Sep 17 00:00:00 2001 From: AdithyaKolla Date: Wed, 6 Dec 2023 09:48:56 -0800 Subject: [PATCH 27/53] add test for remove --- .../docker/orchestrator/orchestrator_test.go | 37 +++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/internal/pkg/docker/orchestrator/orchestrator_test.go b/internal/pkg/docker/orchestrator/orchestrator_test.go index 478141e874e..88c08b13aeb 100644 --- a/internal/pkg/docker/orchestrator/orchestrator_test.go +++ b/internal/pkg/docker/orchestrator/orchestrator_test.go @@ -114,6 +114,43 @@ func TestOrchestrator(t *testing.T) { `stop "bar": some error`, }, }, + "error removing task": { + logOptions: noLogs, + runUntilStopped: true, + test: func(t *testing.T) (test, *dockerenginetest.Double) { + de := &dockerenginetest.Double{ + 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 { + return nil + }, + RmFn: func(ctx context.Context, name string) error { + if name == "prefix-success" { + return nil + } + return errors.New("some error") + }, + } + return func(t *testing.T, o *Orchestrator) { + o.RunTask(Task{ + Containers: map[string]ContainerDefinition{ + "foo": {}, + "bar": {}, + "success": {}, + }, + }) + }, de + }, + errs: []string{ + `remove "pause": some error`, + `remove "foo": some error`, + `remove "bar": some error`, + }, + }, "error polling tasks removed": { logOptions: noLogs, runUntilStopped: true, From 5739e602e427e81f8e3b24f05c650b76fb53e783 Mon Sep 17 00:00:00 2001 From: AdithyaKolla Date: Wed, 6 Dec 2023 10:26:56 -0800 Subject: [PATCH 28/53] add testcase for ess container exits --- .../docker/orchestrator/orchestrator_test.go | 45 ++++++++++++++----- 1 file changed, 35 insertions(+), 10 deletions(-) diff --git a/internal/pkg/docker/orchestrator/orchestrator_test.go b/internal/pkg/docker/orchestrator/orchestrator_test.go index 88c08b13aeb..ab1d13f3dac 100644 --- a/internal/pkg/docker/orchestrator/orchestrator_test.go +++ b/internal/pkg/docker/orchestrator/orchestrator_test.go @@ -267,7 +267,7 @@ func TestOrchestrator(t *testing.T) { }, }, - "return nil error if non essential container exits": { + "return nil if non essential container exits": { logOptions: noLogs, runUntilStopped: true, test: func(t *testing.T) (test, *dockerenginetest.Double) { @@ -277,7 +277,7 @@ func TestOrchestrator(t *testing.T) { }, IsContainerRunningFn: func(ctx context.Context, name string) (bool, error) { if name == "prefix-foo" { - return false, fmt.Errorf("container %q exited with code %d", name, 143) + return false, &dockerengine.ErrContainerExited{} } return true, nil }, @@ -287,12 +287,6 @@ func TestOrchestrator(t *testing.T) { StopFn: func(ctx context.Context, name string) error { return nil }, - IsContainerCompleteOrSuccessFn: func(ctx context.Context, containerName string) (int, error) { - if containerName == "prefix-foo" { - return 143, nil - } - return -1, nil - }, } return func(t *testing.T, o *Orchestrator) { o.RunTask(Task{ @@ -300,15 +294,46 @@ func TestOrchestrator(t *testing.T) { "foo": { IsEssential: false, }, - "bar": { + }, + }) + }, de + }, + }, + + "return error if essential container exits": { + logOptions: noLogs, + runUntilStopped: true, + stopAfterNErrs: 1, + test: func(t *testing.T) (test, *dockerenginetest.Double) { + de := &dockerenginetest.Double{ + DoesContainerExistFn: func(ctx context.Context, s string) (bool, error) { + return false, nil + }, + IsContainerRunningFn: func(ctx context.Context, name string) (bool, error) { + if name == "prefix-foo" { + return false, &dockerengine.ErrContainerExited{} + } + return true, nil + }, + RunFn: func(ctx context.Context, opts *dockerengine.RunOptions) error { + return nil + }, + StopFn: func(ctx context.Context, name string) error { + return nil + }, + } + return func(t *testing.T, o *Orchestrator) { + o.RunTask(Task{ + Containers: map[string]ContainerDefinition{ + "foo": { IsEssential: true, }, }, }) }, de }, + errs: []string{"upward traversal: check if \"prefix-foo\" is running: container \"\" exited with code 0"}, }, - "success with dependsOn order": { logOptions: noLogs, test: func(t *testing.T) (test, *dockerenginetest.Double) { From 41bd18f9135c6b952fda14f657ed9cb551c604fb Mon Sep 17 00:00:00 2001 From: AdithyaKolla Date: Wed, 6 Dec 2023 14:46:27 -0800 Subject: [PATCH 29/53] Fix newline in pipeline.go --- internal/pkg/deploy/pipeline.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/pkg/deploy/pipeline.go b/internal/pkg/deploy/pipeline.go index 9ffb39442e8..bee9a60c5e1 100644 --- a/internal/pkg/deploy/pipeline.go +++ b/internal/pkg/deploy/pipeline.go @@ -916,4 +916,4 @@ func (p *PrePostDeployAction) Name() string { func (p *PrePostDeployAction) RunOrder() int { rank, _ := p.ranker.Rank(p.name) // The deployment is guaranteed to be in the ranker. return p.action.RunOrder() /* baseline */ + rank -} \ No newline at end of file +} From 5990309c0adb90a69e9b5d0a89cb27b06b3cd520 Mon Sep 17 00:00:00 2001 From: AdithyaKolla Date: Wed, 6 Dec 2023 15:07:49 -0800 Subject: [PATCH 30/53] increase test coverage --- .../docker/orchestrator/orchestrator_test.go | 62 +++++++++++++++++++ 1 file changed, 62 insertions(+) diff --git a/internal/pkg/docker/orchestrator/orchestrator_test.go b/internal/pkg/docker/orchestrator/orchestrator_test.go index ab1d13f3dac..6c3066d3e36 100644 --- a/internal/pkg/docker/orchestrator/orchestrator_test.go +++ b/internal/pkg/docker/orchestrator/orchestrator_test.go @@ -388,6 +388,68 @@ func TestOrchestrator(t *testing.T) { }, }, + "return error when dependency container is unhealthy": { + logOptions: noLogs, + stopAfterNErrs: 1, + test: func(t *testing.T) (test, *dockerenginetest.Double) { + stopPause := make(chan struct{}) + stopFoo := make(chan struct{}) + de := &dockerenginetest.Double{ + IsContainerRunningFn: func(ctx context.Context, name string) (bool, error) { + return true, nil + }, + DoesContainerExistFn: func(ctx context.Context, s string) (bool, error) { + return false, nil + }, + RunFn: func(ctx context.Context, opts *dockerengine.RunOptions) error { + if opts.ContainerName == "prefix-bar" { + return errors.New("container `prefix-bar` exited with code 143") + } + if opts.ContainerName == "prefix-pause" { + // block pause container until Stop(pause) + <-stopPause + } + if opts.ContainerName == "prefix-foo" { + // block bar container until Stop(foo) + <-stopFoo + } + return nil + }, + StopFn: func(ctx context.Context, s string) error { + if s == "prefix-pause" { + stopPause <- struct{}{} + } + if s == "prefix-foo" { + stopFoo <- struct{}{} + } + return nil + }, + IsContainerHealthyFn: func(ctx context.Context, containerName string) (bool, error) { + if containerName == "prefix-foo" { + return false, fmt.Errorf("container `prefix-foo` is unhealthy") + } + return true, nil + }, + } + return func(t *testing.T, o *Orchestrator) { + o.RunTask(Task{ + Containers: map[string]ContainerDefinition{ + "foo": { + IsEssential: true, + }, + "bar": { + IsEssential: false, + DependsOn: map[string]string{ + "foo": ctrStateHealthy, + }, + }, + }, + }) + }, de + }, + errs: []string{"upward traversal: wait for container bar dependencies: essential container \"prefix-foo\" failed to be \"healthy\": container `prefix-foo` is unhealthy"}, + }, + "container run stops early with error": { logOptions: noLogs, test: func(t *testing.T) (test, *dockerenginetest.Double) { From 5a4952fcd741bce40b220ce2ce28ff52df3ea7ed Mon Sep 17 00:00:00 2001 From: AdithyaKolla Date: Thu, 7 Dec 2023 21:38:54 -0800 Subject: [PATCH 31/53] address few nits and add tests --- .../pkg/docker/orchestrator/orchestrator.go | 10 +- .../docker/orchestrator/orchestrator_test.go | 123 +++++++++++++++++- 2 files changed, 123 insertions(+), 10 deletions(-) diff --git a/internal/pkg/docker/orchestrator/orchestrator.go b/internal/pkg/docker/orchestrator/orchestrator.go index a7a312e5dd0..6303fff0d94 100644 --- a/internal/pkg/docker/orchestrator/orchestrator.go +++ b/internal/pkg/docker/orchestrator/orchestrator.go @@ -115,7 +115,8 @@ func New(docker DockerEngine, idPrefix string, logOptions logOptionsFunc) *Orche func (o *Orchestrator) Start() <-chan error { // close done when all goroutines created by Orchestrator have finished done := make(chan struct{}) - // buffered channel so that the orchestrator routine does not block and can always send the errors from runErrs and action.Do + // buffered channel so that the orchestrator routine does not block and + // can always send the error from both runErrs and action.Do to errs. errs := make(chan error, 1) // orchestrator routine @@ -570,7 +571,7 @@ func (o *Orchestrator) waitForContainerDependencies(ctx context.Context, contain return fmt.Errorf("dependency container %q failed to be %q: %w", ctrId, state, err) } if exitCode > 0 { - return fmt.Errorf("dependency container %q exited with code %d", ctrId, exitCode) + return fmt.Errorf("dependency container %q exited with code %d instead of %d exit code", ctrId, exitCode, 0) } if exitCode == 0 { log.Successf("Successfully dependency container %q reached %q\n", ctrId, state) @@ -664,13 +665,10 @@ func (o *Orchestrator) run(taskID int32, opts dockerengine.RunOptions, isEssenti // or from the currently running task if taskID == pauseCtrTaskID || taskID == curTaskID { var errContainerExited *dockerengine.ErrContainerExited - if errors.As(err, &errContainerExited) && !isEssential { + if !isEssential && (errors.As(err, &errContainerExited) || err == nil) { return } if err == nil { - if !isEssential { - return - } err = errors.New("container stopped unexpectedly") } // cancel context to indicate all the other go routines spawned by `graph.UpwardTarversal`. diff --git a/internal/pkg/docker/orchestrator/orchestrator_test.go b/internal/pkg/docker/orchestrator/orchestrator_test.go index 6c3066d3e36..d1ff37d69c9 100644 --- a/internal/pkg/docker/orchestrator/orchestrator_test.go +++ b/internal/pkg/docker/orchestrator/orchestrator_test.go @@ -347,9 +347,6 @@ func TestOrchestrator(t *testing.T) { return false, nil }, RunFn: func(ctx context.Context, opts *dockerengine.RunOptions) error { - if opts.ContainerName == "prefix-bar" { - return errors.New("container `prefix-bar` exited with code 143") - } if opts.ContainerName == "prefix-pause" { // block pause container until Stop(pause) <-stopPause @@ -377,7 +374,7 @@ func TestOrchestrator(t *testing.T) { IsEssential: true, }, "bar": { - IsEssential: false, + IsEssential: true, DependsOn: map[string]string{ "foo": ctrStateStart, }, @@ -450,6 +447,124 @@ func TestOrchestrator(t *testing.T) { errs: []string{"upward traversal: wait for container bar dependencies: essential container \"prefix-foo\" failed to be \"healthy\": container `prefix-foo` is unhealthy"}, }, + "return error when dependency container complete failed": { + logOptions: noLogs, + stopAfterNErrs: 1, + test: func(t *testing.T) (test, *dockerenginetest.Double) { + stopPause := make(chan struct{}) + stopFoo := make(chan struct{}) + de := &dockerenginetest.Double{ + IsContainerRunningFn: func(ctx context.Context, name string) (bool, error) { + return true, nil + }, + DoesContainerExistFn: func(ctx context.Context, s string) (bool, error) { + return false, nil + }, + RunFn: func(ctx context.Context, opts *dockerengine.RunOptions) error { + if opts.ContainerName == "prefix-pause" { + // block pause container until Stop(pause) + <-stopPause + } + if opts.ContainerName == "prefix-foo" { + // block bar container until Stop(foo) + <-stopFoo + } + return nil + }, + StopFn: func(ctx context.Context, s string) error { + if s == "prefix-pause" { + stopPause <- struct{}{} + } + if s == "prefix-foo" { + stopFoo <- struct{}{} + } + return nil + }, + IsContainerCompleteOrSuccessFn: func(ctx context.Context, containerName string) (int, error) { + if containerName == "prefix-foo" { + return 143, fmt.Errorf("some error") + } + return -1, nil + }, + } + return func(t *testing.T, o *Orchestrator) { + o.RunTask(Task{ + Containers: map[string]ContainerDefinition{ + "foo": { + IsEssential: false, + }, + "bar": { + IsEssential: true, + DependsOn: map[string]string{ + "foo": ctrStateComplete, + }, + }, + }, + }) + }, de + }, + errs: []string{"upward traversal: wait for container bar dependencies: dependency container \"prefix-foo\" failed to be \"complete\": some error"}, + }, + + "return error when container with exit code other than zero if condition is success": { + logOptions: noLogs, + stopAfterNErrs: 1, + test: func(t *testing.T) (test, *dockerenginetest.Double) { + stopPause := make(chan struct{}) + stopFoo := make(chan struct{}) + de := &dockerenginetest.Double{ + IsContainerRunningFn: func(ctx context.Context, name string) (bool, error) { + return true, nil + }, + DoesContainerExistFn: func(ctx context.Context, s string) (bool, error) { + return false, nil + }, + RunFn: func(ctx context.Context, opts *dockerengine.RunOptions) error { + if opts.ContainerName == "prefix-pause" { + // block pause container until Stop(pause) + <-stopPause + } + if opts.ContainerName == "prefix-foo" { + // block bar container until Stop(foo) + <-stopFoo + } + return nil + }, + StopFn: func(ctx context.Context, s string) error { + if s == "prefix-pause" { + stopPause <- struct{}{} + } + if s == "prefix-foo" { + stopFoo <- struct{}{} + } + return nil + }, + IsContainerCompleteOrSuccessFn: func(ctx context.Context, containerName string) (int, error) { + if containerName == "prefix-foo" { + return 143, nil + } + return -1, nil + }, + } + return func(t *testing.T, o *Orchestrator) { + o.RunTask(Task{ + Containers: map[string]ContainerDefinition{ + "foo": { + IsEssential: false, + }, + "bar": { + IsEssential: true, + DependsOn: map[string]string{ + "foo": ctrStateSuccess, + }, + }, + }, + }) + }, de + }, + errs: []string{"upward traversal: wait for container bar dependencies: dependency container \"prefix-foo\" exited with code 143 instead of 0 exit code"}, + }, + "container run stops early with error": { logOptions: noLogs, test: func(t *testing.T) (test, *dockerenginetest.Double) { From fafc521e414d3ee58e16309b206215693ac27000 Mon Sep 17 00:00:00 2001 From: AdithyaKolla Date: Fri, 8 Dec 2023 09:31:35 -0800 Subject: [PATCH 32/53] address one more nit --- .../pkg/docker/orchestrator/orchestrator.go | 90 +++++++++---------- 1 file changed, 45 insertions(+), 45 deletions(-) diff --git a/internal/pkg/docker/orchestrator/orchestrator.go b/internal/pkg/docker/orchestrator/orchestrator.go index 6303fff0d94..e095e0c4cca 100644 --- a/internal/pkg/docker/orchestrator/orchestrator.go +++ b/internal/pkg/docker/orchestrator/orchestrator.go @@ -531,55 +531,55 @@ func (o *Orchestrator) waitForContainerDependencies(ctx context.Context, contain for { select { case <-ticker.C: - switch state { - case ctrStateStart: - err := o.waitForContainerToStart(ctx, ctrId, isEssential) - if err != nil { - return err - } - if isEssential { - log.Successf("Successfully container %s is running\n", ctrId) - return nil - } - fmt.Printf("non-essential container %q started successfully\n", ctrId) + case <-ctx.Done(): + return ctx.Err() + } + switch state { + case ctrStateStart: + err := o.waitForContainerToStart(ctx, ctrId, isEssential) + if err != nil { + return err + } + if isEssential { + log.Successf("Successfully container %s is running\n", ctrId) return nil - case ctrStateHealthy: - healthy, err := o.docker.IsContainerHealthy(ctx, ctrId) - if err != nil { - if !isEssential { - fmt.Printf("non-essential container %q failed to be %q: %v\n", ctrId, state, err) - return nil - } - return fmt.Errorf("essential container %q failed to be %q: %w", ctrId, state, err) - } - if healthy { - log.Successf("Successfully dependency container %q reached %q\n", ctrId, state) - return nil - } - case ctrStateComplete: - exitCode, err := o.docker.IsContainerCompleteOrSuccess(ctx, ctrId) - if err != nil { - return fmt.Errorf("dependency container %q failed to be %q: %w", ctrId, state, err) - } - if exitCode != -1 { - log.Successf("Successfully dependency container %q exited with code: %d\n", ctrId, exitCode) - return nil - } - case ctrStateSuccess: - exitCode, err := o.docker.IsContainerCompleteOrSuccess(ctx, ctrId) - if err != nil { - return fmt.Errorf("dependency container %q failed to be %q: %w", ctrId, state, err) - } - if exitCode > 0 { - return fmt.Errorf("dependency container %q exited with code %d instead of %d exit code", ctrId, exitCode, 0) - } - if exitCode == 0 { - log.Successf("Successfully dependency container %q reached %q\n", ctrId, state) + } + fmt.Printf("non-essential container %q started successfully\n", ctrId) + return nil + case ctrStateHealthy: + healthy, err := o.docker.IsContainerHealthy(ctx, ctrId) + if err != nil { + if !isEssential { + fmt.Printf("non-essential container %q failed to be %q: %v\n", ctrId, state, err) return nil } + return fmt.Errorf("essential container %q failed to be %q: %w", ctrId, state, err) + } + if healthy { + log.Successf("Successfully dependency container %q reached %q\n", ctrId, state) + return nil + } + case ctrStateComplete: + exitCode, err := o.docker.IsContainerCompleteOrSuccess(ctx, ctrId) + if err != nil { + return fmt.Errorf("dependency container %q failed to be %q: %w", ctrId, state, err) + } + if exitCode != -1 { + log.Successf("Successfully dependency container %q exited with code: %d\n", ctrId, exitCode) + return nil + } + case ctrStateSuccess: + exitCode, err := o.docker.IsContainerCompleteOrSuccess(ctx, ctrId) + if err != nil { + return fmt.Errorf("dependency container %q failed to be %q: %w", ctrId, state, err) + } + if exitCode > 0 { + return fmt.Errorf("dependency container %q exited with code %d instead of %d exit code", ctrId, exitCode, 0) + } + if exitCode == 0 { + log.Successf("Successfully dependency container %q reached %q\n", ctrId, state) + return nil } - case <-ctx.Done(): - return ctx.Err() } } }) From c903b6dd164a175d4389b66c5b6a78c6c5feaad2 Mon Sep 17 00:00:00 2001 From: AdithyaKolla Date: Wed, 13 Dec 2023 09:27:03 -0800 Subject: [PATCH 33/53] remove DoesContainerExist logic : --- internal/pkg/cli/interfaces.go | 1 - internal/pkg/cli/mocks/mock_interfaces.go | 15 ---- .../pkg/docker/dockerengine/dockerengine.go | 9 --- .../dockerenginetest/dockerenginetest.go | 9 --- .../pkg/docker/orchestrator/orchestrator.go | 26 +------ .../docker/orchestrator/orchestrator_test.go | 68 ------------------- 6 files changed, 3 insertions(+), 125 deletions(-) diff --git a/internal/pkg/cli/interfaces.go b/internal/pkg/cli/interfaces.go index 7e1f11759c6..cf95375297e 100644 --- a/internal/pkg/cli/interfaces.go +++ b/internal/pkg/cli/interfaces.go @@ -706,7 +706,6 @@ 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 Build(context.Context, *dockerengine.BuildArguments, io.Writer) error diff --git a/internal/pkg/cli/mocks/mock_interfaces.go b/internal/pkg/cli/mocks/mock_interfaces.go index 60d46fbc3ee..718d83fb9ae 100644 --- a/internal/pkg/cli/mocks/mock_interfaces.go +++ b/internal/pkg/cli/mocks/mock_interfaces.go @@ -7693,21 +7693,6 @@ func (mr *MockdockerEngineRunnerMockRecorder) CheckDockerEngineRunning() *gomock return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CheckDockerEngineRunning", reflect.TypeOf((*MockdockerEngineRunner)(nil).CheckDockerEngineRunning)) } -// DoesContainerExist mocks base method. -func (m *MockdockerEngineRunner) DoesContainerExist(arg0 context.Context, arg1 string) (bool, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "DoesContainerExist", arg0, arg1) - ret0, _ := ret[0].(bool) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// DoesContainerExist indicates an expected call of DoesContainerExist. -func (mr *MockdockerEngineRunnerMockRecorder) DoesContainerExist(arg0, arg1 interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DoesContainerExist", reflect.TypeOf((*MockdockerEngineRunner)(nil).DoesContainerExist), arg0, arg1) -} - // Exec mocks base method. func (m *MockdockerEngineRunner) Exec(ctx context.Context, container string, out io.Writer, cmd string, args ...string) error { m.ctrl.T.Helper() diff --git a/internal/pkg/docker/dockerengine/dockerengine.go b/internal/pkg/docker/dockerengine/dockerengine.go index cd288130800..123e9ab0383 100644 --- a/internal/pkg/docker/dockerengine/dockerengine.go +++ b/internal/pkg/docker/dockerengine/dockerengine.go @@ -361,15 +361,6 @@ 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) diff --git a/internal/pkg/docker/dockerengine/dockerenginetest/dockerenginetest.go b/internal/pkg/docker/dockerengine/dockerenginetest/dockerenginetest.go index a8147a0b1ca..232962c5130 100644 --- a/internal/pkg/docker/dockerengine/dockerenginetest/dockerenginetest.go +++ b/internal/pkg/docker/dockerengine/dockerenginetest/dockerenginetest.go @@ -13,7 +13,6 @@ 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 @@ -31,14 +30,6 @@ 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 { diff --git a/internal/pkg/docker/orchestrator/orchestrator.go b/internal/pkg/docker/orchestrator/orchestrator.go index e095e0c4cca..3eea3019bd3 100644 --- a/internal/pkg/docker/orchestrator/orchestrator.go +++ b/internal/pkg/docker/orchestrator/orchestrator.go @@ -52,7 +52,6 @@ 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) IsContainerCompleteOrSuccess(ctx context.Context, containerName string) (int, error) IsContainerHealthy(ctx context.Context, containerName string) (bool, error) @@ -452,28 +451,9 @@ func (o *Orchestrator) stopTask(ctx context.Context, task Task) error { errCh <- fmt.Errorf("remove %q: %w", name, err) return nil } - // ensure that container is fully stopped before stopTask finishes blocking - for { - exists, err := o.docker.DoesContainerExist(ctx, o.containerID(name)) - if err != nil { - errCh <- fmt.Errorf("polling container %q for removal: %w", name, err) - return nil - } - - if exists { - select { - case <-time.After(1 * time.Second): - continue - case <-ctx.Done(): - errCh <- fmt.Errorf("check container %q stopped: %w", name, ctx.Err()) - return nil - } - } - - fmt.Printf("Stopped and Removed %q\n", name) - errCh <- nil - return nil - } + fmt.Printf("Stopped and Removed %q\n", name) + errCh <- nil + return nil }, ctrStateRunningOrExited, ctrStateRemoved) if err != nil { diff --git a/internal/pkg/docker/orchestrator/orchestrator_test.go b/internal/pkg/docker/orchestrator/orchestrator_test.go index d1ff37d69c9..d1701fdcafd 100644 --- a/internal/pkg/docker/orchestrator/orchestrator_test.go +++ b/internal/pkg/docker/orchestrator/orchestrator_test.go @@ -85,9 +85,6 @@ func TestOrchestrator(t *testing.T) { runUntilStopped: true, test: func(t *testing.T) (test, *dockerenginetest.Double) { de := &dockerenginetest.Double{ - DoesContainerExistFn: func(ctx context.Context, s string) (bool, error) { - return false, nil - }, IsContainerRunningFn: func(ctx context.Context, name string) (bool, error) { return true, nil }, @@ -119,9 +116,6 @@ func TestOrchestrator(t *testing.T) { runUntilStopped: true, test: func(t *testing.T) (test, *dockerenginetest.Double) { de := &dockerenginetest.Double{ - DoesContainerExistFn: func(ctx context.Context, s string) (bool, error) { - return false, nil - }, IsContainerRunningFn: func(ctx context.Context, name string) (bool, error) { return true, nil }, @@ -151,43 +145,11 @@ func TestOrchestrator(t *testing.T) { `remove "bar": some error`, }, }, - "error polling tasks removed": { - logOptions: noLogs, - runUntilStopped: true, - test: func(t *testing.T) (test, *dockerenginetest.Double) { - de := &dockerenginetest.Double{ - 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 - }, - } - return func(t *testing.T, o *Orchestrator) { - o.RunTask(Task{ - Containers: map[string]ContainerDefinition{ - "foo": {}, - "bar": {}, - }, - }) - }, de - }, - errs: []string{ - `polling container "foo" for removal: some error`, - `polling container "bar" for removal: some error`, - }, - }, "error restarting new task due to pause changes": { logOptions: noLogs, runUntilStopped: true, test: func(t *testing.T) (test, *dockerenginetest.Double) { de := &dockerenginetest.Double{ - DoesContainerExistFn: func(ctx context.Context, s string) (bool, error) { - return false, nil - }, IsContainerRunningFn: func(ctx context.Context, name string) (bool, error) { return true, nil }, @@ -222,9 +184,6 @@ func TestOrchestrator(t *testing.T) { runUntilStopped: true, test: func(t *testing.T) (test, *dockerenginetest.Double) { de := &dockerenginetest.Double{ - DoesContainerExistFn: func(ctx context.Context, s string) (bool, error) { - return false, nil - }, IsContainerRunningFn: func(ctx context.Context, name string) (bool, error) { return true, nil }, @@ -272,9 +231,6 @@ func TestOrchestrator(t *testing.T) { runUntilStopped: true, test: func(t *testing.T) (test, *dockerenginetest.Double) { de := &dockerenginetest.Double{ - DoesContainerExistFn: func(ctx context.Context, s string) (bool, error) { - return false, nil - }, IsContainerRunningFn: func(ctx context.Context, name string) (bool, error) { if name == "prefix-foo" { return false, &dockerengine.ErrContainerExited{} @@ -306,9 +262,6 @@ func TestOrchestrator(t *testing.T) { stopAfterNErrs: 1, test: func(t *testing.T) (test, *dockerenginetest.Double) { de := &dockerenginetest.Double{ - DoesContainerExistFn: func(ctx context.Context, s string) (bool, error) { - return false, nil - }, IsContainerRunningFn: func(ctx context.Context, name string) (bool, error) { if name == "prefix-foo" { return false, &dockerengine.ErrContainerExited{} @@ -343,9 +296,6 @@ func TestOrchestrator(t *testing.T) { IsContainerRunningFn: func(ctx context.Context, name string) (bool, error) { return true, nil }, - DoesContainerExistFn: func(ctx context.Context, s string) (bool, error) { - return false, nil - }, RunFn: func(ctx context.Context, opts *dockerengine.RunOptions) error { if opts.ContainerName == "prefix-pause" { // block pause container until Stop(pause) @@ -395,9 +345,6 @@ func TestOrchestrator(t *testing.T) { IsContainerRunningFn: func(ctx context.Context, name string) (bool, error) { return true, nil }, - DoesContainerExistFn: func(ctx context.Context, s string) (bool, error) { - return false, nil - }, RunFn: func(ctx context.Context, opts *dockerengine.RunOptions) error { if opts.ContainerName == "prefix-bar" { return errors.New("container `prefix-bar` exited with code 143") @@ -457,9 +404,6 @@ func TestOrchestrator(t *testing.T) { IsContainerRunningFn: func(ctx context.Context, name string) (bool, error) { return true, nil }, - DoesContainerExistFn: func(ctx context.Context, s string) (bool, error) { - return false, nil - }, RunFn: func(ctx context.Context, opts *dockerengine.RunOptions) error { if opts.ContainerName == "prefix-pause" { // block pause container until Stop(pause) @@ -516,9 +460,6 @@ func TestOrchestrator(t *testing.T) { IsContainerRunningFn: func(ctx context.Context, name string) (bool, error) { return true, nil }, - DoesContainerExistFn: func(ctx context.Context, s string) (bool, error) { - return false, nil - }, RunFn: func(ctx context.Context, opts *dockerengine.RunOptions) error { if opts.ContainerName == "prefix-pause" { // block pause container until Stop(pause) @@ -570,9 +511,6 @@ func TestOrchestrator(t *testing.T) { test: func(t *testing.T) (test, *dockerenginetest.Double) { stopPause := make(chan struct{}) de := &dockerenginetest.Double{ - DoesContainerExistFn: func(ctx context.Context, s string) (bool, error) { - return false, nil - }, IsContainerRunningFn: func(ctx context.Context, name string) (bool, error) { return true, nil }, @@ -619,9 +557,6 @@ func TestOrchestrator(t *testing.T) { test: func(t *testing.T) (test, *dockerenginetest.Double) { stopPause := make(chan struct{}) de := &dockerenginetest.Double{ - DoesContainerExistFn: func(ctx context.Context, s string) (bool, error) { - return false, nil - }, IsContainerRunningFn: func(ctx context.Context, name string) (bool, error) { return true, nil }, @@ -769,9 +704,6 @@ func TestOrchestrator(t *testing.T) { runUntilStopped: true, test: func(t *testing.T) (test, *dockerenginetest.Double) { de := &dockerenginetest.Double{ - DoesContainerExistFn: func(ctx context.Context, s string) (bool, error) { - return false, nil - }, IsContainerRunningFn: func(ctx context.Context, name string) (bool, error) { return true, nil }, From d1b159d202e26b6e0385a6f099ecdb7892508709 Mon Sep 17 00:00:00 2001 From: AdithyaKolla Date: Fri, 15 Dec 2023 00:54:55 -0800 Subject: [PATCH 34/53] address fb: modify graph pkg API --- .../pkg/docker/orchestrator/orchestrator.go | 31 +++---- internal/pkg/graph/graph.go | 91 ++++++------------- internal/pkg/graph/graph_test.go | 10 +- 3 files changed, 43 insertions(+), 89 deletions(-) diff --git a/internal/pkg/docker/orchestrator/orchestrator.go b/internal/pkg/docker/orchestrator/orchestrator.go index 3eea3019bd3..1c2a41d5e74 100644 --- a/internal/pkg/docker/orchestrator/orchestrator.go +++ b/internal/pkg/docker/orchestrator/orchestrator.go @@ -75,12 +75,6 @@ const ( proxyPortStart = uint16(50000) ) -const ( - ctrStateUnknown = "unknown" - ctrStateRunningOrExited = "runningOrExited" - ctrStateRemoved = "removed" -) - const ( ctrStateHealthy = "healthy" ctrStateComplete = "complete" @@ -208,9 +202,8 @@ func (a *runTaskAction) Do(o *Orchestrator) error { cancel() } }() - + o.curTask = a.task if taskID == 1 { - o.curTask = a.task if err := o.buildPauseContainer(ctx); err != nil { return fmt.Errorf("build pause container: %w", err) } @@ -241,8 +234,7 @@ func (a *runTaskAction) Do(o *Orchestrator) error { return fmt.Errorf("stop existing task: %w", err) } } - o.curTask = a.task - depGraph := buildDependencyGraph(a.task.Containers, ctrStateUnknown) + depGraph := buildDependencyGraph(a.task.Containers) err := depGraph.UpwardTraversal(ctx, func(ctx context.Context, containerName string) error { if len(a.task.Containers[containerName].DependsOn) > 0 { if err := o.waitForContainerDependencies(ctx, containerName, a); err != nil { @@ -251,7 +243,7 @@ func (a *runTaskAction) Do(o *Orchestrator) error { } o.run(taskID, o.containerRunOptions(containerName, a.task.Containers[containerName]), a.task.Containers[containerName].IsEssential, cancel) return o.waitForContainerToStart(ctx, o.containerID(containerName), a.task.Containers[containerName].IsEssential) - }, ctrStateUnknown, ctrStateRunningOrExited) + }) if err != nil { if errors.Is(err, context.Canceled) { return nil @@ -261,12 +253,12 @@ func (a *runTaskAction) Do(o *Orchestrator) error { return nil } -func buildDependencyGraph(containers map[string]ContainerDefinition, status string) *graph.LabeledGraph[string] { +func buildDependencyGraph(containers map[string]ContainerDefinition) *graph.LabeledGraph[string] { var vertices []string for vertex := range containers { vertices = append(vertices, vertex) } - dependencyGraph := graph.NewLabeledGraph(vertices, graph.WithStatus[string](status)) + dependencyGraph := graph.NewLabeledGraph(vertices) for containerName, container := range containers { for depCtr := range container.DependsOn { dependencyGraph.Add(graph.Edge[string]{ @@ -420,15 +412,14 @@ func (a *stopAction) Do(o *Orchestrator) error { } // stop pause container - fmt.Printf("Stopping and Removing %q\n", "pause") + fmt.Printf("Stopping and removing %q\n", "pause") if err := o.docker.Stop(context.Background(), o.containerID("pause")); err != nil { errs = append(errs, fmt.Errorf("stop %q: %w", "pause", err)) } if err := o.docker.Rm(context.Background(), o.containerID("pause")); err != nil { errs = append(errs, fmt.Errorf("remove %q: %w", "pause", err)) } - fmt.Printf("Stopped and Removed %q\n", "pause") - + fmt.Printf("Stopped and removed %q\n", "pause") return errors.Join(errs...) } @@ -440,9 +431,9 @@ func (o *Orchestrator) stopTask(ctx context.Context, task Task) error { // errCh gets one error per container errCh := make(chan error, len(task.Containers)) - depGraph := buildDependencyGraph(task.Containers, ctrStateRunningOrExited) + depGraph := buildDependencyGraph(task.Containers) err := depGraph.DownwardTraversal(ctx, func(ctx context.Context, name string) error { - fmt.Printf("Stopping and Removing %q\n", name) + fmt.Printf("Stopping and removing %q\n", name) if err := o.docker.Stop(ctx, o.containerID(name)); err != nil { errCh <- fmt.Errorf("stop %q: %w", name, err) return nil @@ -451,10 +442,10 @@ func (o *Orchestrator) stopTask(ctx context.Context, task Task) error { errCh <- fmt.Errorf("remove %q: %w", name, err) return nil } - fmt.Printf("Stopped and Removed %q\n", name) + fmt.Printf("Stopped and removed %q\n", name) errCh <- nil return nil - }, ctrStateRunningOrExited, ctrStateRemoved) + }) if err != nil { return fmt.Errorf("downward traversal: %w", err) diff --git a/internal/pkg/graph/graph.go b/internal/pkg/graph/graph.go index 0eb69f059dc..bd52661ff2a 100644 --- a/internal/pkg/graph/graph.go +++ b/internal/pkg/graph/graph.go @@ -227,46 +227,32 @@ func TopologicalOrder[V comparable](digraph *Graph[V]) (*TopologicalSorter[V], e // It is concurrency-safe, utilizing a mutex lock for synchronized access. type LabeledGraph[V comparable] struct { *Graph[V] - status map[V]string + status map[V]vertexStatus lock sync.Mutex } // NewLabeledGraph initializes a LabeledGraph with specified vertices and optional configurations. // It creates a base Graph with the vertices and applies any LabeledGraphOption to configure additional properties. -func NewLabeledGraph[V comparable](vertices []V, opts ...LabeledGraphOption[V]) *LabeledGraph[V] { - g := New(vertices...) +func NewLabeledGraph[V comparable](vertices []V) *LabeledGraph[V] { lg := &LabeledGraph[V]{ - Graph: g, - status: make(map[V]string), + Graph: New(vertices...), + status: make(map[V]vertexStatus), } - for _, opt := range opts { - opt(lg) + for _, vertex := range vertices { + lg.status[vertex] = unvisited } return lg } -// LabeledGraphOption allows you to initialize Graph with additional properties. -type LabeledGraphOption[V comparable] func(g *LabeledGraph[V]) - -// WithStatus sets the status of each vertex in the Graph. -func WithStatus[V comparable](status string) func(g *LabeledGraph[V]) { - return func(g *LabeledGraph[V]) { - g.status = make(map[V]string) - for vertex := range g.vertices { - g.status[vertex] = status - } - } -} - // updateStatus updates the status of a vertex. -func (lg *LabeledGraph[V]) updateStatus(vertex V, status string) { +func (lg *LabeledGraph[V]) updateStatus(vertex V, status vertexStatus) { lg.lock.Lock() defer lg.lock.Unlock() lg.status[vertex] = status } // getStatus gets the status of a vertex. -func (lg *LabeledGraph[V]) getStatus(vertex V) string { +func (lg *LabeledGraph[V]) getStatus(vertex V) vertexStatus { lg.lock.Lock() defer lg.lock.Unlock() return lg.status[vertex] @@ -306,7 +292,7 @@ func (lg *LabeledGraph[V]) children(vtx V) []V { } // filterParents filters parents based on the vertex status. -func (lg *LabeledGraph[V]) filterParents(vtx V, status string) []V { +func (lg *LabeledGraph[V]) filterParents(vtx V, status vertexStatus) []V { parents := lg.parents(vtx) var filtered []V for _, parent := range parents { @@ -318,7 +304,7 @@ func (lg *LabeledGraph[V]) filterParents(vtx V, status string) []V { } // filterChildren filters children based on the vertex status. -func (lg *LabeledGraph[V]) filterChildren(vtx V, status string) []V { +func (lg *LabeledGraph[V]) filterChildren(vtx V, status vertexStatus) []V { children := lg.children(vtx) var filtered []V for _, child := range children { @@ -330,44 +316,32 @@ func (lg *LabeledGraph[V]) filterChildren(vtx V, status string) []V { } /* -UpwardTraversal performs an upward traversal on the graph starting from leaves (nodes with no children) -and moving towards root nodes (nodes with children). -It applies the specified process function to each vertex in the graph, skipping vertices with the -"adjacentVertexSkipStatus" status, and continuing traversal until reaching vertices with the "requiredVertexStatus" status. -The traversal is concurrent and may process vertices in parallel. -Returns an error if the traversal encounters any issues, or nil if successful. +UpwardTraversal performs a traversal from leaf nodes (with no children) to root nodes (with children). +It processes each vertex using processVertexFunc, and skips processing for vertices with specific statuses. +The traversal is concurrent, handling vertices in parallel, and returns an error if any issue occurs. */ -func (lg *LabeledGraph[V]) UpwardTraversal(ctx context.Context, processVertexFunc func(context.Context, V) error, nextVertexSkipStatus, requiredVertexStatus string) error { +func (lg *LabeledGraph[V]) UpwardTraversal(ctx context.Context, processVertexFunc func(context.Context, V) error) error { traversal := &graphTraversal[V]{ mu: sync.Mutex{}, - seen: make(map[V]struct{}), findStartVertices: func(lg *LabeledGraph[V]) []V { return lg.leaves() }, findNextVertices: func(lg *LabeledGraph[V], v V) []V { return lg.parents(v) }, - filterPreviousVerticesByStatus: func(g *LabeledGraph[V], v V, status string) []V { return g.filterChildren(v, status) }, - requiredVertexStatus: requiredVertexStatus, - nextVertexSkipStatus: nextVertexSkipStatus, + filterPreviousVerticesByStatus: func(g *LabeledGraph[V], v V, status vertexStatus) []V { return g.filterChildren(v, status) }, processVertex: processVertexFunc, } return traversal.execute(ctx, lg) } /* -DownwardTraversal performs a downward traversal on the graph starting from root nodes (nodes with no parents) -and moving towards leaf nodes (nodes with parents). It applies the specified process function to each -vertex in the graph, skipping vertices with the "adjacentVertexSkipStatus" status, and continuing traversal -until reaching vertices with the "requiredVertexStatus" status. -The traversal is concurrent and may process vertices in parallel. -Returns an error if the traversal encounters any issues. +DownwardTraversal performs a traversal from root nodes (with no parents) to leaf nodes (with parents). +It applies processVertexFunc to each vertex, skipping those with specified statuses. +It conducts concurrent processing of vertices and returns an error for any encountered issues. */ -func (lg *LabeledGraph[V]) DownwardTraversal(ctx context.Context, processVertexFunc func(context.Context, V) error, adjacentVertexSkipStatus, requiredVertexStatus string) error { +func (lg *LabeledGraph[V]) DownwardTraversal(ctx context.Context, processVertexFunc func(context.Context, V) error) error { traversal := &graphTraversal[V]{ mu: sync.Mutex{}, - seen: make(map[V]struct{}), findStartVertices: func(lg *LabeledGraph[V]) []V { return lg.Roots() }, findNextVertices: func(lg *LabeledGraph[V], v V) []V { return lg.children(v) }, - filterPreviousVerticesByStatus: func(lg *LabeledGraph[V], v V, status string) []V { return lg.filterParents(v, status) }, - requiredVertexStatus: requiredVertexStatus, - nextVertexSkipStatus: adjacentVertexSkipStatus, + filterPreviousVerticesByStatus: func(lg *LabeledGraph[V], v V, status vertexStatus) []V { return lg.filterParents(v, status) }, processVertex: processVertexFunc, } return traversal.execute(ctx, lg) @@ -375,12 +349,9 @@ func (lg *LabeledGraph[V]) DownwardTraversal(ctx context.Context, processVertexF type graphTraversal[V comparable] struct { mu sync.Mutex - seen map[V]struct{} findStartVertices func(*LabeledGraph[V]) []V findNextVertices func(*LabeledGraph[V], V) []V - filterPreviousVerticesByStatus func(*LabeledGraph[V], V, string) []V - requiredVertexStatus string - nextVertexSkipStatus string + filterPreviousVerticesByStatus func(*LabeledGraph[V], V, vertexStatus) []V processVertex func(context.Context, V) error } @@ -401,19 +372,21 @@ func (t *graphTraversal[V]) execute(ctx context.Context, lg *LabeledGraph[V]) er for _, vertex := range vertices { vertex := vertex // Delay processing this vertex if any of its dependent vertices are yet to be processed. - if len(t.filterPreviousVerticesByStatus(graph, vertex, t.nextVertexSkipStatus)) != 0 { + if len(t.filterPreviousVerticesByStatus(graph, vertex, unvisited)) != 0 { continue } - if !t.markAsSeen(vertex) { - // Skip this vertex if it's already been processed by another routine. + // Check if the vertex is already visited or being visited + if graph.getStatus(vertex) != unvisited { continue } + // Mark the vertex as visiting + graph.updateStatus(vertex, visiting) eg.Go(func() error { if err := t.processVertex(ctx, vertex); err != nil { return err } // Assign new status to the vertex upon successful processing. - graph.updateStatus(vertex, t.requiredVertexStatus) + graph.updateStatus(vertex, visited) vertexCh <- vertex return nil }) @@ -437,13 +410,3 @@ func (t *graphTraversal[V]) execute(ctx context.Context, lg *LabeledGraph[V]) er processVertices(ctx, lg, eg, t.findStartVertices(lg), vertexCh) return eg.Wait() } - -func (t *graphTraversal[V]) markAsSeen(vertex V) bool { - t.mu.Lock() - defer t.mu.Unlock() - if _, seen := t.seen[vertex]; seen { - return false - } - t.seen[vertex] = struct{}{} - return true -} diff --git a/internal/pkg/graph/graph_test.go b/internal/pkg/graph/graph_test.go index 1f4a818e1a1..cf770ccf441 100644 --- a/internal/pkg/graph/graph_test.go +++ b/internal/pkg/graph/graph_test.go @@ -373,7 +373,7 @@ func TestTopologicalOrder(t *testing.T) { func buildGraphWithSingleParent() *LabeledGraph[string] { vertices := []string{"A", "B", "C", "D"} - graph := NewLabeledGraph[string](vertices, WithStatus[string]("started")) + graph := NewLabeledGraph[string](vertices) graph.Add(Edge[string]{From: "D", To: "C"}) // D -> C graph.Add(Edge[string]{From: "C", To: "B"}) // C -> B graph.Add(Edge[string]{From: "B", To: "A"}) // B -> A @@ -388,14 +388,14 @@ func TestTraverseInDependencyOrder(t *testing.T) { visited = append(visited, v) return nil } - err := graph.UpwardTraversal(context.Background(), processFn, "started", "stopped") + err := graph.UpwardTraversal(context.Background(), processFn) require.NoError(t, err) expected := []string{"A", "B", "C", "D"} require.Equal(t, expected, visited) }) t.Run("graph with multiple parents and boundary nodes", func(t *testing.T) { vertices := []string{"A", "B", "C", "D"} - graph := NewLabeledGraph[string](vertices, WithStatus[string]("started")) + graph := NewLabeledGraph[string](vertices) graph.Add(Edge[string]{From: "A", To: "C"}) graph.Add(Edge[string]{From: "A", To: "D"}) graph.Add(Edge[string]{From: "B", To: "D"}) @@ -412,7 +412,7 @@ func TestTraverseInDependencyOrder(t *testing.T) { err := graph.DownwardTraversal(context.Background(), func(ctx context.Context, vtx string) error { vtxChan <- vtx return nil - }, "started", "stopped") + }) require.NoError(t, err, "Error during iteration") close(vtxChan) <-done @@ -432,7 +432,7 @@ func TestTraverseInReverseDependencyOrder(t *testing.T) { visited = append(visited, v) return nil } - err := graph.DownwardTraversal(context.Background(), processFn, "started", "stopped") + err := graph.DownwardTraversal(context.Background(), processFn) require.NoError(t, err) expected := []string{"D", "C", "B", "A"} require.Equal(t, expected, visited) From 77c4e1a1d478f0ec8c589e278525a6bf1c919827 Mon Sep 17 00:00:00 2001 From: AdithyaKolla Date: Fri, 15 Dec 2023 08:53:52 -0800 Subject: [PATCH 35/53] use o.curTask at two places --- internal/pkg/docker/orchestrator/orchestrator.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/internal/pkg/docker/orchestrator/orchestrator.go b/internal/pkg/docker/orchestrator/orchestrator.go index 1c2a41d5e74..d75baa58632 100644 --- a/internal/pkg/docker/orchestrator/orchestrator.go +++ b/internal/pkg/docker/orchestrator/orchestrator.go @@ -202,8 +202,8 @@ func (a *runTaskAction) Do(o *Orchestrator) error { cancel() } }() - o.curTask = a.task if taskID == 1 { + o.curTask = a.task if err := o.buildPauseContainer(ctx); err != nil { return fmt.Errorf("build pause container: %w", err) } @@ -234,6 +234,7 @@ func (a *runTaskAction) Do(o *Orchestrator) error { return fmt.Errorf("stop existing task: %w", err) } } + o.curTask = a.task depGraph := buildDependencyGraph(a.task.Containers) err := depGraph.UpwardTraversal(ctx, func(ctx context.Context, containerName string) error { if len(a.task.Containers[containerName].DependsOn) > 0 { From 3bb15b691b8a646cc4ecca9d6ff0f2fdec27d4d1 Mon Sep 17 00:00:00 2001 From: AdithyaKolla Date: Fri, 15 Dec 2023 08:56:22 -0800 Subject: [PATCH 36/53] fix doc comment --- internal/pkg/graph/graph.go | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/internal/pkg/graph/graph.go b/internal/pkg/graph/graph.go index bd52661ff2a..6b583301714 100644 --- a/internal/pkg/graph/graph.go +++ b/internal/pkg/graph/graph.go @@ -203,12 +203,13 @@ func (alg *TopologicalSorter[V]) traverse(g *Graph[V]) { // // An example graph and their ranks is shown below to illustrate: // . -//├── a rank: 0 -//│ ├── c rank: 1 -//│ │ └── f rank: 2 -//│ └── d rank: 1 -//└── b rank: 0 -// └── e rank: 1 +// ├── a rank: 0 +// │ ├── c rank: 1 +// │ │ └── f rank: 2 +// │ └── d rank: 1 +// └── b rank: 0 +// +// └── e rank: 1 func TopologicalOrder[V comparable](digraph *Graph[V]) (*TopologicalSorter[V], error) { if vertices, isAcyclic := digraph.IsAcyclic(); !isAcyclic { return nil, &errCycle[V]{ @@ -231,12 +232,12 @@ type LabeledGraph[V comparable] struct { lock sync.Mutex } -// NewLabeledGraph initializes a LabeledGraph with specified vertices and optional configurations. -// It creates a base Graph with the vertices and applies any LabeledGraphOption to configure additional properties. +// NewLabeledGraph initializes a LabeledGraph with specified vertices and set the status of each vertex to unvisited. func NewLabeledGraph[V comparable](vertices []V) *LabeledGraph[V] { lg := &LabeledGraph[V]{ Graph: New(vertices...), status: make(map[V]vertexStatus), + lock: sync.Mutex{}, } for _, vertex := range vertices { lg.status[vertex] = unvisited From 831ca737699a59b14e1bc94a812b85348a853e36 Mon Sep 17 00:00:00 2001 From: AdithyaKolla Date: Fri, 15 Dec 2023 08:57:59 -0800 Subject: [PATCH 37/53] fix more doc comments --- internal/pkg/graph/graph.go | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/internal/pkg/graph/graph.go b/internal/pkg/graph/graph.go index 6b583301714..8e7e712f554 100644 --- a/internal/pkg/graph/graph.go +++ b/internal/pkg/graph/graph.go @@ -203,13 +203,12 @@ func (alg *TopologicalSorter[V]) traverse(g *Graph[V]) { // // An example graph and their ranks is shown below to illustrate: // . -// ├── a rank: 0 -// │ ├── c rank: 1 -// │ │ └── f rank: 2 -// │ └── d rank: 1 -// └── b rank: 0 -// -// └── e rank: 1 +//├── a rank: 0 +//│ ├── c rank: 1 +//│ │ └── f rank: 2 +//│ └── d rank: 1 +//└── b rank: 0 +// └── e rank: 1 func TopologicalOrder[V comparable](digraph *Graph[V]) (*TopologicalSorter[V], error) { if vertices, isAcyclic := digraph.IsAcyclic(); !isAcyclic { return nil, &errCycle[V]{ From 6bc79683927c38c40750b7559a008fc88ced6aa7 Mon Sep 17 00:00:00 2001 From: AdithyaKolla Date: Fri, 15 Dec 2023 09:26:12 -0800 Subject: [PATCH 38/53] remove mutex in graph traversal --- internal/pkg/graph/graph.go | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/internal/pkg/graph/graph.go b/internal/pkg/graph/graph.go index 8e7e712f554..36389288bf8 100644 --- a/internal/pkg/graph/graph.go +++ b/internal/pkg/graph/graph.go @@ -203,12 +203,13 @@ func (alg *TopologicalSorter[V]) traverse(g *Graph[V]) { // // An example graph and their ranks is shown below to illustrate: // . -//├── a rank: 0 -//│ ├── c rank: 1 -//│ │ └── f rank: 2 -//│ └── d rank: 1 -//└── b rank: 0 -// └── e rank: 1 +// ├── a rank: 0 +// │ ├── c rank: 1 +// │ │ └── f rank: 2 +// │ └── d rank: 1 +// └── b rank: 0 +// +// └── e rank: 1 func TopologicalOrder[V comparable](digraph *Graph[V]) (*TopologicalSorter[V], error) { if vertices, isAcyclic := digraph.IsAcyclic(); !isAcyclic { return nil, &errCycle[V]{ @@ -322,7 +323,6 @@ The traversal is concurrent, handling vertices in parallel, and returns an error */ func (lg *LabeledGraph[V]) UpwardTraversal(ctx context.Context, processVertexFunc func(context.Context, V) error) error { traversal := &graphTraversal[V]{ - mu: sync.Mutex{}, findStartVertices: func(lg *LabeledGraph[V]) []V { return lg.leaves() }, findNextVertices: func(lg *LabeledGraph[V], v V) []V { return lg.parents(v) }, filterPreviousVerticesByStatus: func(g *LabeledGraph[V], v V, status vertexStatus) []V { return g.filterChildren(v, status) }, @@ -338,7 +338,6 @@ It conducts concurrent processing of vertices and returns an error for any encou */ func (lg *LabeledGraph[V]) DownwardTraversal(ctx context.Context, processVertexFunc func(context.Context, V) error) error { traversal := &graphTraversal[V]{ - mu: sync.Mutex{}, findStartVertices: func(lg *LabeledGraph[V]) []V { return lg.Roots() }, findNextVertices: func(lg *LabeledGraph[V], v V) []V { return lg.children(v) }, filterPreviousVerticesByStatus: func(lg *LabeledGraph[V], v V, status vertexStatus) []V { return lg.filterParents(v, status) }, @@ -348,7 +347,6 @@ func (lg *LabeledGraph[V]) DownwardTraversal(ctx context.Context, processVertexF } type graphTraversal[V comparable] struct { - mu sync.Mutex findStartVertices func(*LabeledGraph[V]) []V findNextVertices func(*LabeledGraph[V], V) []V filterPreviousVerticesByStatus func(*LabeledGraph[V], V, vertexStatus) []V From 1a690f2edfa02472e2dcf75e30d791ccd339a598 Mon Sep 17 00:00:00 2001 From: AdithyaKolla Date: Fri, 15 Dec 2023 14:40:32 -0800 Subject: [PATCH 39/53] address @Lou1415926 fb --- .../pkg/docker/orchestrator/orchestrator.go | 22 +++++++++---------- internal/pkg/graph/graph.go | 13 +++++------ 2 files changed, 17 insertions(+), 18 deletions(-) diff --git a/internal/pkg/docker/orchestrator/orchestrator.go b/internal/pkg/docker/orchestrator/orchestrator.go index d75baa58632..a581a63ba24 100644 --- a/internal/pkg/docker/orchestrator/orchestrator.go +++ b/internal/pkg/docker/orchestrator/orchestrator.go @@ -485,15 +485,15 @@ func (o *Orchestrator) waitForContainerToStart(ctx context.Context, id string, i } } -func (o *Orchestrator) waitForContainerDependencies(ctx context.Context, containerName string, a *runTaskAction) error { +func (o *Orchestrator) waitForContainerDependencies(ctx context.Context, name string, a *runTaskAction) error { var deps []string - for depName, state := range a.task.Containers[containerName].DependsOn { + for depName, state := range a.task.Containers[name].DependsOn { deps = append(deps, fmt.Sprintf("%s->%s", depName, state)) } logMsg := strings.Join(deps, ", ") - fmt.Printf("Waiting for container %q dependencies: [%s]\n", containerName, color.Emphasize(logMsg)) + fmt.Printf("Waiting for container %q dependencies: [%s]\n", name, color.Emphasize(logMsg)) eg, ctx := errgroup.WithContext(ctx) - for name, state := range a.task.Containers[containerName].DependsOn { + for name, state := range a.task.Containers[name].DependsOn { name, state := name, state eg.Go(func() error { ctrId := o.containerID(name) @@ -513,7 +513,7 @@ func (o *Orchestrator) waitForContainerDependencies(ctx context.Context, contain return err } if isEssential { - log.Successf("Successfully container %s is running\n", ctrId) + log.Successf("Successfully started container %s\n", ctrId) return nil } fmt.Printf("non-essential container %q started successfully\n", ctrId) @@ -522,10 +522,10 @@ func (o *Orchestrator) waitForContainerDependencies(ctx context.Context, contain healthy, err := o.docker.IsContainerHealthy(ctx, ctrId) if err != nil { if !isEssential { - fmt.Printf("non-essential container %q failed to be %q: %v\n", ctrId, state, err) + fmt.Printf("non-essential container %q is not healthy: %v\n", ctrId, err) return nil } - return fmt.Errorf("essential container %q failed to be %q: %w", ctrId, state, err) + return fmt.Errorf("essential container %q is not healthy: %w", ctrId, err) } if healthy { log.Successf("Successfully dependency container %q reached %q\n", ctrId, state) @@ -534,10 +534,10 @@ func (o *Orchestrator) waitForContainerDependencies(ctx context.Context, contain case ctrStateComplete: exitCode, err := o.docker.IsContainerCompleteOrSuccess(ctx, ctrId) if err != nil { - return fmt.Errorf("dependency container %q failed to be %q: %w", ctrId, state, err) + return fmt.Errorf("wait for container %q to complete: %w", ctrId, err) } if exitCode != -1 { - log.Successf("Successfully dependency container %q exited with code: %d\n", ctrId, exitCode) + log.Successf("%q's dependency container %q exited with code: %d\n", name, ctrId, exitCode) return nil } case ctrStateSuccess: @@ -546,10 +546,10 @@ func (o *Orchestrator) waitForContainerDependencies(ctx context.Context, contain return fmt.Errorf("dependency container %q failed to be %q: %w", ctrId, state, err) } if exitCode > 0 { - return fmt.Errorf("dependency container %q exited with code %d instead of %d exit code", ctrId, exitCode, 0) + return fmt.Errorf("dependency container %q exited with non-zero exit code %d", ctrId, exitCode) } if exitCode == 0 { - log.Successf("Successfully dependency container %q reached %q\n", ctrId, state) + log.Successf("%q's dependency container %q exited with code: %d\n", name, ctrId, exitCode) return nil } } diff --git a/internal/pkg/graph/graph.go b/internal/pkg/graph/graph.go index 36389288bf8..d329fdaccde 100644 --- a/internal/pkg/graph/graph.go +++ b/internal/pkg/graph/graph.go @@ -203,13 +203,12 @@ func (alg *TopologicalSorter[V]) traverse(g *Graph[V]) { // // An example graph and their ranks is shown below to illustrate: // . -// ├── a rank: 0 -// │ ├── c rank: 1 -// │ │ └── f rank: 2 -// │ └── d rank: 1 -// └── b rank: 0 -// -// └── e rank: 1 +//├── a rank: 0 +//│ ├── c rank: 1 +//│ │ └── f rank: 2 +//│ └── d rank: 1 +//└── b rank: 0 +// └── e rank: 1 func TopologicalOrder[V comparable](digraph *Graph[V]) (*TopologicalSorter[V], error) { if vertices, isAcyclic := digraph.IsAcyclic(); !isAcyclic { return nil, &errCycle[V]{ From 075023c9b572af9d46c48708d1afa851b9a6294b Mon Sep 17 00:00:00 2001 From: AdithyaKolla Date: Fri, 15 Dec 2023 15:47:31 -0800 Subject: [PATCH 40/53] address fb @ Lou1415926 --- internal/pkg/cli/interfaces.go | 2 +- internal/pkg/cli/mocks/mock_interfaces.go | 30 +++++++++---------- .../pkg/docker/dockerengine/dockerengine.go | 8 ++--- .../docker/dockerengine/dockerengine_test.go | 8 ++--- .../dockerenginetest/dockerenginetest.go | 26 ++++++++-------- .../docker/orchestrator/orchestrator_test.go | 20 ++++++------- 6 files changed, 47 insertions(+), 47 deletions(-) diff --git a/internal/pkg/cli/interfaces.go b/internal/pkg/cli/interfaces.go index cf95375297e..1bada558c00 100644 --- a/internal/pkg/cli/interfaces.go +++ b/internal/pkg/cli/interfaces.go @@ -710,7 +710,7 @@ type dockerEngineRunner interface { Stop(context.Context, string) error Build(context.Context, *dockerengine.BuildArguments, io.Writer) error Exec(ctx context.Context, container string, out io.Writer, cmd string, args ...string) error - IsContainerCompleteOrSuccess(ctx context.Context, containerName string) (int, error) + ContainerExitCode(ctx context.Context, containerName string) (int, error) IsContainerHealthy(ctx context.Context, containerName string) (bool, error) Rm(context.Context, string) error } diff --git a/internal/pkg/cli/mocks/mock_interfaces.go b/internal/pkg/cli/mocks/mock_interfaces.go index 718d83fb9ae..decab61a443 100644 --- a/internal/pkg/cli/mocks/mock_interfaces.go +++ b/internal/pkg/cli/mocks/mock_interfaces.go @@ -7693,6 +7693,21 @@ func (mr *MockdockerEngineRunnerMockRecorder) CheckDockerEngineRunning() *gomock return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CheckDockerEngineRunning", reflect.TypeOf((*MockdockerEngineRunner)(nil).CheckDockerEngineRunning)) } +// ContainerExitCode mocks base method. +func (m *MockdockerEngineRunner) ContainerExitCode(ctx context.Context, containerName string) (int, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "ContainerExitCode", ctx, containerName) + ret0, _ := ret[0].(int) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// ContainerExitCode indicates an expected call of ContainerExitCode. +func (mr *MockdockerEngineRunnerMockRecorder) ContainerExitCode(ctx, containerName interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ContainerExitCode", reflect.TypeOf((*MockdockerEngineRunner)(nil).ContainerExitCode), ctx, containerName) +} + // Exec mocks base method. func (m *MockdockerEngineRunner) Exec(ctx context.Context, container string, out io.Writer, cmd string, args ...string) error { m.ctrl.T.Helper() @@ -7712,21 +7727,6 @@ func (mr *MockdockerEngineRunnerMockRecorder) Exec(ctx, container, out, cmd inte return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Exec", reflect.TypeOf((*MockdockerEngineRunner)(nil).Exec), varargs...) } -// IsContainerCompleteOrSuccess mocks base method. -func (m *MockdockerEngineRunner) IsContainerCompleteOrSuccess(ctx context.Context, containerName string) (int, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "IsContainerCompleteOrSuccess", ctx, containerName) - ret0, _ := ret[0].(int) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// IsContainerCompleteOrSuccess indicates an expected call of IsContainerCompleteOrSuccess. -func (mr *MockdockerEngineRunnerMockRecorder) IsContainerCompleteOrSuccess(ctx, containerName interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "IsContainerCompleteOrSuccess", reflect.TypeOf((*MockdockerEngineRunner)(nil).IsContainerCompleteOrSuccess), ctx, containerName) -} - // IsContainerHealthy mocks base method. func (m *MockdockerEngineRunner) IsContainerHealthy(ctx context.Context, containerName string) (bool, error) { m.ctrl.T.Helper() diff --git a/internal/pkg/docker/dockerengine/dockerengine.go b/internal/pkg/docker/dockerengine/dockerengine.go index 123e9ab0383..898e76b9ed9 100644 --- a/internal/pkg/docker/dockerengine/dockerengine.go +++ b/internal/pkg/docker/dockerengine/dockerengine.go @@ -376,14 +376,14 @@ func (c DockerCmdClient) IsContainerRunning(ctx context.Context, name string) (b return false, nil } -// IsContainerCompleteOrSuccess returns true if a docker container exits with an exitcode. -func (c DockerCmdClient) IsContainerCompleteOrSuccess(ctx context.Context, containerName string) (int, error) { - state, err := c.containerState(ctx, containerName) +// ContainerExitCode returns the exit code of a container. +func (c DockerCmdClient) ContainerExitCode(ctx context.Context, name string) (int, error) { + state, err := c.containerState(ctx, name) if err != nil { return 0, err } if state.Status == containerStatusRunning { - return -1, nil + return 0, fmt.Errorf("container %q has not exited", name) } return state.ExitCode, nil } diff --git a/internal/pkg/docker/dockerengine/dockerengine_test.go b/internal/pkg/docker/dockerengine/dockerengine_test.go index 7f291be8d77..a2c6d5609de 100644 --- a/internal/pkg/docker/dockerengine/dockerengine_test.go +++ b/internal/pkg/docker/dockerengine/dockerengine_test.go @@ -1052,7 +1052,7 @@ func TestDockerCommand_IsContainerHealthy(t *testing.T) { } } -func TestDockerCommand_IsContainerCompleteOrSuccess(t *testing.T) { +func TestDockerCommand_ContainerExitCode(t *testing.T) { tests := map[string]struct { mockContainerName string mockHealthStatus string @@ -1127,7 +1127,7 @@ func TestDockerCommand_IsContainerCompleteOrSuccess(t *testing.T) { }, wantErr: fmt.Errorf("run docker ps: some error"), }, - "return negative exitcode if container is running": { + "return err if container is running": { mockContainerName: "mockContainer", mockHealthStatus: "unhealthy", setupMocks: func(controller *gomock.Controller) *MockCmd { @@ -1154,7 +1154,7 @@ func TestDockerCommand_IsContainerCompleteOrSuccess(t *testing.T) { }) return mockCmd }, - wantExitCode: -1, + wantErr: fmt.Errorf(`container "mockContainer" has not exited`), }, } @@ -1167,7 +1167,7 @@ func TestDockerCommand_IsContainerCompleteOrSuccess(t *testing.T) { runner: tc.setupMocks(ctrl), } - expectedCode, err := s.IsContainerCompleteOrSuccess(context.Background(), tc.mockContainerName) + expectedCode, err := s.ContainerExitCode(context.Background(), tc.mockContainerName) require.Equal(t, tc.wantExitCode, expectedCode) if tc.wantErr != nil { require.EqualError(t, err, tc.wantErr.Error()) diff --git a/internal/pkg/docker/dockerengine/dockerenginetest/dockerenginetest.go b/internal/pkg/docker/dockerengine/dockerenginetest/dockerenginetest.go index 232962c5130..ac3416bd571 100644 --- a/internal/pkg/docker/dockerengine/dockerenginetest/dockerenginetest.go +++ b/internal/pkg/docker/dockerengine/dockerenginetest/dockerenginetest.go @@ -12,14 +12,14 @@ import ( // Double is a test double for dockerengine.DockerCmdClient type Double struct { - StopFn func(context.Context, string) error - IsContainerRunningFn func(context.Context, string) (bool, error) - RunFn func(context.Context, *dockerengine.RunOptions) error - BuildFn func(context.Context, *dockerengine.BuildArguments, io.Writer) error - ExecFn func(context.Context, string, io.Writer, string, ...string) error - IsContainerHealthyFn func(ctx context.Context, containerName string) (bool, error) - IsContainerCompleteOrSuccessFn func(ctx context.Context, containerName string) (int, error) - RmFn func(context.Context, string) error + StopFn func(context.Context, string) error + IsContainerRunningFn func(context.Context, string) (bool, error) + RunFn func(context.Context, *dockerengine.RunOptions) error + BuildFn func(context.Context, *dockerengine.BuildArguments, io.Writer) error + ExecFn func(context.Context, string, io.Writer, string, ...string) error + IsContainerHealthyFn func(ctx context.Context, containerName string) (bool, error) + ContainerExitCodeFn func(ctx context.Context, containerName string) (int, error) + RmFn func(context.Context, string) error } // Stop calls the stubbed function. @@ -70,12 +70,12 @@ func (d *Double) Rm(ctx context.Context, name string) error { return d.RmFn(ctx, name) } -// IsContainerCompleteOrSuccess implements orchestrator.DockerEngine. -func (d *Double) IsContainerCompleteOrSuccess(ctx context.Context, containerName string) (int, error) { - if d.IsContainerCompleteOrSuccessFn == nil { - return -1, nil +// ContainerExitCode implements orchestrator.DockerEngine. +func (d *Double) ContainerExitCode(ctx context.Context, containerName string) (int, error) { + if d.ContainerExitCodeFn == nil { + return 0, nil } - return d.IsContainerCompleteOrSuccessFn(ctx, containerName) + return d.ContainerExitCodeFn(ctx, containerName) } // IsContainerHealthy implements orchestrator.DockerEngine. diff --git a/internal/pkg/docker/orchestrator/orchestrator_test.go b/internal/pkg/docker/orchestrator/orchestrator_test.go index d1701fdcafd..6f1275c91c6 100644 --- a/internal/pkg/docker/orchestrator/orchestrator_test.go +++ b/internal/pkg/docker/orchestrator/orchestrator_test.go @@ -391,7 +391,7 @@ func TestOrchestrator(t *testing.T) { }) }, de }, - errs: []string{"upward traversal: wait for container bar dependencies: essential container \"prefix-foo\" failed to be \"healthy\": container `prefix-foo` is unhealthy"}, + errs: []string{"upward traversal: wait for container bar dependencies: essential container \"prefix-foo\" is not healthy: container `prefix-foo` is unhealthy"}, }, "return error when dependency container complete failed": { @@ -424,11 +424,11 @@ func TestOrchestrator(t *testing.T) { } return nil }, - IsContainerCompleteOrSuccessFn: func(ctx context.Context, containerName string) (int, error) { - if containerName == "prefix-foo" { + ContainerExitCodeFn: func(ctx context.Context, name string) (int, error) { + if name == "prefix-foo" { return 143, fmt.Errorf("some error") } - return -1, nil + return 0, fmt.Errorf("container %q has not exited", name) }, } return func(t *testing.T, o *Orchestrator) { @@ -447,10 +447,10 @@ func TestOrchestrator(t *testing.T) { }) }, de }, - errs: []string{"upward traversal: wait for container bar dependencies: dependency container \"prefix-foo\" failed to be \"complete\": some error"}, + errs: []string{"upward traversal: wait for container bar dependencies: wait for container \"prefix-foo\" to complete: some error"}, }, - "return error when container with exit code other than zero if condition is success": { + "return error when container with non zero exitcode if condition is success": { logOptions: noLogs, stopAfterNErrs: 1, test: func(t *testing.T) (test, *dockerenginetest.Double) { @@ -480,11 +480,11 @@ func TestOrchestrator(t *testing.T) { } return nil }, - IsContainerCompleteOrSuccessFn: func(ctx context.Context, containerName string) (int, error) { + ContainerExitCodeFn: func(ctx context.Context, containerName string) (int, error) { if containerName == "prefix-foo" { return 143, nil } - return -1, nil + return 0, fmt.Errorf("container %q has not exited", containerName) }, } return func(t *testing.T, o *Orchestrator) { @@ -503,7 +503,7 @@ func TestOrchestrator(t *testing.T) { }) }, de }, - errs: []string{"upward traversal: wait for container bar dependencies: dependency container \"prefix-foo\" exited with code 143 instead of 0 exit code"}, + errs: []string{"upward traversal: wait for container bar dependencies: dependency container \"prefix-foo\" exited with non-zero exit code 143"}, }, "container run stops early with error": { @@ -514,7 +514,7 @@ func TestOrchestrator(t *testing.T) { IsContainerRunningFn: func(ctx context.Context, name string) (bool, error) { return true, nil }, - IsContainerCompleteOrSuccessFn: func(ctx context.Context, containerName string) (int, error) { + ContainerExitCodeFn: func(ctx context.Context, containerName string) (int, error) { return -1, nil }, RunFn: func(ctx context.Context, opts *dockerengine.RunOptions) error { From cd497f9e886b893bc9ced4ae0033ec6bc418366b Mon Sep 17 00:00:00 2001 From: AdithyaKolla Date: Fri, 15 Dec 2023 15:47:59 -0800 Subject: [PATCH 41/53] fix more nits --- .../pkg/docker/orchestrator/orchestrator.go | 27 ++++++++++++------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/internal/pkg/docker/orchestrator/orchestrator.go b/internal/pkg/docker/orchestrator/orchestrator.go index a581a63ba24..d77dafcd62d 100644 --- a/internal/pkg/docker/orchestrator/orchestrator.go +++ b/internal/pkg/docker/orchestrator/orchestrator.go @@ -53,7 +53,7 @@ type logOptionsFunc func(name string, ctr ContainerDefinition) dockerengine.RunL type DockerEngine interface { Run(context.Context, *dockerengine.RunOptions) error IsContainerRunning(context.Context, string) (bool, error) - IsContainerCompleteOrSuccess(ctx context.Context, containerName string) (int, error) + ContainerExitCode(ctx context.Context, containerName string) (int, error) IsContainerHealthy(ctx context.Context, containerName string) (bool, error) Stop(context.Context, string) error Build(ctx context.Context, args *dockerengine.BuildArguments, w io.Writer) error @@ -238,7 +238,7 @@ func (a *runTaskAction) Do(o *Orchestrator) error { depGraph := buildDependencyGraph(a.task.Containers) err := depGraph.UpwardTraversal(ctx, func(ctx context.Context, containerName string) error { if len(a.task.Containers[containerName].DependsOn) > 0 { - if err := o.waitForContainerDependencies(ctx, containerName, a); err != nil { + if err := o.waitForContainerDependencies(ctx, containerName, a.task.Containers); err != nil { return fmt.Errorf("wait for container %s dependencies: %w", containerName, err) } } @@ -464,6 +464,7 @@ func (o *Orchestrator) stopTask(ctx context.Context, task Task) error { } // waitForContainerToStart blocks until the container specified by id starts. +// If the container is not essential, then waitForContainerToStart returns nil. func (o *Orchestrator) waitForContainerToStart(ctx context.Context, id string, isEssential bool) error { for { isRunning, err := o.docker.IsContainerRunning(ctx, id) @@ -485,19 +486,19 @@ func (o *Orchestrator) waitForContainerToStart(ctx context.Context, id string, i } } -func (o *Orchestrator) waitForContainerDependencies(ctx context.Context, name string, a *runTaskAction) error { +func (o *Orchestrator) waitForContainerDependencies(ctx context.Context, name string, definitions map[string]ContainerDefinition) error { var deps []string - for depName, state := range a.task.Containers[name].DependsOn { + for depName, state := range definitions[name].DependsOn { deps = append(deps, fmt.Sprintf("%s->%s", depName, state)) } logMsg := strings.Join(deps, ", ") fmt.Printf("Waiting for container %q dependencies: [%s]\n", name, color.Emphasize(logMsg)) eg, ctx := errgroup.WithContext(ctx) - for name, state := range a.task.Containers[name].DependsOn { + for name, state := range definitions[name].DependsOn { name, state := name, state eg.Go(func() error { ctrId := o.containerID(name) - isEssential := a.task.Containers[name].IsEssential + isEssential := definitions[name].IsEssential ticker := time.NewTicker(700 * time.Millisecond) defer ticker.Stop() for { @@ -532,20 +533,26 @@ func (o *Orchestrator) waitForContainerDependencies(ctx context.Context, name st return nil } case ctrStateComplete: - exitCode, err := o.docker.IsContainerCompleteOrSuccess(ctx, ctrId) + exitCode, err := o.docker.ContainerExitCode(ctx, ctrId) + if errors.Is(err, fmt.Errorf("container %q has not exited", name)) { + continue + } if err != nil { return fmt.Errorf("wait for container %q to complete: %w", ctrId, err) } - if exitCode != -1 { + if exitCode != 0 { log.Successf("%q's dependency container %q exited with code: %d\n", name, ctrId, exitCode) return nil } case ctrStateSuccess: - exitCode, err := o.docker.IsContainerCompleteOrSuccess(ctx, ctrId) + exitCode, err := o.docker.ContainerExitCode(ctx, ctrId) + if errors.Is(err, fmt.Errorf("container %q has not exited", name)) { + continue + } if err != nil { return fmt.Errorf("dependency container %q failed to be %q: %w", ctrId, state, err) } - if exitCode > 0 { + if exitCode != 0 { return fmt.Errorf("dependency container %q exited with non-zero exit code %d", ctrId, exitCode) } if exitCode == 0 { From 08542d3c0bcf62dc0b894e732039c41fccf92051 Mon Sep 17 00:00:00 2001 From: AdithyaKolla Date: Fri, 15 Dec 2023 16:30:20 -0800 Subject: [PATCH 42/53] use custom error --- internal/pkg/docker/dockerengine/dockerengine.go | 12 +++++++++++- internal/pkg/docker/orchestrator/orchestrator.go | 6 ++++-- .../pkg/docker/orchestrator/orchestrator_test.go | 6 +++--- 3 files changed, 18 insertions(+), 6 deletions(-) diff --git a/internal/pkg/docker/dockerengine/dockerengine.go b/internal/pkg/docker/dockerengine/dockerengine.go index 898e76b9ed9..9ab55a3d398 100644 --- a/internal/pkg/docker/dockerengine/dockerengine.go +++ b/internal/pkg/docker/dockerengine/dockerengine.go @@ -383,7 +383,7 @@ func (c DockerCmdClient) ContainerExitCode(ctx context.Context, name string) (in return 0, err } if state.Status == containerStatusRunning { - return 0, fmt.Errorf("container %q has not exited", name) + return 0, &ErrContainerNotExited{name: name} } return state.ExitCode, nil } @@ -614,3 +614,13 @@ type errEmptyImageTags struct { func (e *errEmptyImageTags) Error() string { return fmt.Sprintf("tags to reference an image should not be empty for building and pushing into the ECR repository %s", e.uri) } + +// ErrContainerNotExited represents an error when a Docker container has not exited. +type ErrContainerNotExited struct { + name string +} + +// Error returns the error message. +func (e *ErrContainerNotExited) Error() string { + return fmt.Sprintf("container %q has not exited", e.name) +} diff --git a/internal/pkg/docker/orchestrator/orchestrator.go b/internal/pkg/docker/orchestrator/orchestrator.go index d77dafcd62d..822914c00b3 100644 --- a/internal/pkg/docker/orchestrator/orchestrator.go +++ b/internal/pkg/docker/orchestrator/orchestrator.go @@ -534,7 +534,8 @@ func (o *Orchestrator) waitForContainerDependencies(ctx context.Context, name st } case ctrStateComplete: exitCode, err := o.docker.ContainerExitCode(ctx, ctrId) - if errors.Is(err, fmt.Errorf("container %q has not exited", name)) { + var errContainerNotExited *dockerengine.ErrContainerNotExited + if errors.As(err, &errContainerNotExited) { continue } if err != nil { @@ -546,7 +547,8 @@ func (o *Orchestrator) waitForContainerDependencies(ctx context.Context, name st } case ctrStateSuccess: exitCode, err := o.docker.ContainerExitCode(ctx, ctrId) - if errors.Is(err, fmt.Errorf("container %q has not exited", name)) { + var errContainerNotExited *dockerengine.ErrContainerNotExited + if errors.As(err, &errContainerNotExited) { continue } if err != nil { diff --git a/internal/pkg/docker/orchestrator/orchestrator_test.go b/internal/pkg/docker/orchestrator/orchestrator_test.go index 6f1275c91c6..62f33077638 100644 --- a/internal/pkg/docker/orchestrator/orchestrator_test.go +++ b/internal/pkg/docker/orchestrator/orchestrator_test.go @@ -428,7 +428,7 @@ func TestOrchestrator(t *testing.T) { if name == "prefix-foo" { return 143, fmt.Errorf("some error") } - return 0, fmt.Errorf("container %q has not exited", name) + return 0, &dockerengine.ErrContainerNotExited{} }, } return func(t *testing.T, o *Orchestrator) { @@ -484,7 +484,7 @@ func TestOrchestrator(t *testing.T) { if containerName == "prefix-foo" { return 143, nil } - return 0, fmt.Errorf("container %q has not exited", containerName) + return 0, &dockerengine.ErrContainerNotExited{} }, } return func(t *testing.T, o *Orchestrator) { @@ -515,7 +515,7 @@ func TestOrchestrator(t *testing.T) { return true, nil }, ContainerExitCodeFn: func(ctx context.Context, containerName string) (int, error) { - return -1, nil + return 0, nil }, RunFn: func(ctx context.Context, opts *dockerengine.RunOptions) error { if opts.ContainerName == "prefix-foo" { From eeb156059d431ab217d70d83b2ac34800313681b Mon Sep 17 00:00:00 2001 From: AdithyaKolla Date: Fri, 15 Dec 2023 19:11:14 -0800 Subject: [PATCH 43/53] use prevTask --- internal/pkg/docker/orchestrator/orchestrator.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/internal/pkg/docker/orchestrator/orchestrator.go b/internal/pkg/docker/orchestrator/orchestrator.go index 822914c00b3..fe065d8bc7a 100644 --- a/internal/pkg/docker/orchestrator/orchestrator.go +++ b/internal/pkg/docker/orchestrator/orchestrator.go @@ -202,8 +202,9 @@ func (a *runTaskAction) Do(o *Orchestrator) error { cancel() } }() + prevTask := o.curTask + o.curTask = a.task if taskID == 1 { - o.curTask = a.task if err := o.buildPauseContainer(ctx); err != nil { return fmt.Errorf("build pause container: %w", err) } @@ -222,19 +223,18 @@ func (a *runTaskAction) Do(o *Orchestrator) error { } } else { // ensure no pause container changes - curOpts := o.pauseRunOptions(o.curTask) + prevOpts := o.pauseRunOptions(prevTask) newOpts := o.pauseRunOptions(a.task) - if !maps.Equal(curOpts.EnvVars, newOpts.EnvVars) || - !maps.Equal(curOpts.Secrets, newOpts.Secrets) || - !maps.Equal(curOpts.ContainerPorts, newOpts.ContainerPorts) { + if !maps.Equal(prevOpts.EnvVars, newOpts.EnvVars) || + !maps.Equal(prevOpts.Secrets, newOpts.Secrets) || + !maps.Equal(prevOpts.ContainerPorts, newOpts.ContainerPorts) { return errors.New("new task requires recreating pause container") } - if err := o.stopTask(ctx, o.curTask); err != nil { + if err := o.stopTask(ctx, prevTask); err != nil { return fmt.Errorf("stop existing task: %w", err) } } - o.curTask = a.task depGraph := buildDependencyGraph(a.task.Containers) err := depGraph.UpwardTraversal(ctx, func(ctx context.Context, containerName string) error { if len(a.task.Containers[containerName].DependsOn) > 0 { From a30c0976777af245e0241d16186757405fb3b0bb Mon Sep 17 00:00:00 2001 From: AdithyaKolla Date: Fri, 15 Dec 2023 20:34:55 -0800 Subject: [PATCH 44/53] remove mix up of essential and dependsOn --- .../pkg/docker/orchestrator/orchestrator.go | 48 ++++++++----------- .../docker/orchestrator/orchestrator_test.go | 35 +------------- 2 files changed, 21 insertions(+), 62 deletions(-) diff --git a/internal/pkg/docker/orchestrator/orchestrator.go b/internal/pkg/docker/orchestrator/orchestrator.go index fe065d8bc7a..926a67e6715 100644 --- a/internal/pkg/docker/orchestrator/orchestrator.go +++ b/internal/pkg/docker/orchestrator/orchestrator.go @@ -212,7 +212,7 @@ func (a *runTaskAction) Do(o *Orchestrator) error { // start the pause container opts := o.pauseRunOptions(a.task) o.run(pauseCtrTaskID, opts, true, cancel) - if err := o.waitForContainerToStart(ctx, opts.ContainerName, true); err != nil { + if err := o.waitForContainerToStart(ctx, opts.ContainerName); err != nil { return fmt.Errorf("wait for pause container to start: %w", err) } @@ -243,7 +243,11 @@ func (a *runTaskAction) Do(o *Orchestrator) error { } } o.run(taskID, o.containerRunOptions(containerName, a.task.Containers[containerName]), a.task.Containers[containerName].IsEssential, cancel) - return o.waitForContainerToStart(ctx, o.containerID(containerName), a.task.Containers[containerName].IsEssential) + var errContainerExited *dockerengine.ErrContainerExited + if err := o.waitForContainerToStart(ctx, o.containerID(containerName)); err != nil && !errors.As(err, &errContainerExited) { + return fmt.Errorf("wait for container %s to start: %w", containerName, err) + } + return nil }) if err != nil { if errors.Is(err, context.Canceled) { @@ -464,20 +468,16 @@ func (o *Orchestrator) stopTask(ctx context.Context, task Task) error { } // waitForContainerToStart blocks until the container specified by id starts. -// If the container is not essential, then waitForContainerToStart returns nil. -func (o *Orchestrator) waitForContainerToStart(ctx context.Context, id string, isEssential bool) error { +func (o *Orchestrator) waitForContainerToStart(ctx context.Context, id string) error { for { isRunning, err := o.docker.IsContainerRunning(ctx, id) switch { case err != nil: - var errContainerExited *dockerengine.ErrContainerExited - if errors.As(err, &errContainerExited) && !isEssential { - return nil - } return fmt.Errorf("check if %q is running: %w", id, err) case isRunning: return nil } + select { case <-time.After(1 * time.Second): case <-ctx.Done(): @@ -498,7 +498,6 @@ func (o *Orchestrator) waitForContainerDependencies(ctx context.Context, name st name, state := name, state eg.Go(func() error { ctrId := o.containerID(name) - isEssential := definitions[name].IsEssential ticker := time.NewTicker(700 * time.Millisecond) defer ticker.Stop() for { @@ -509,27 +508,20 @@ func (o *Orchestrator) waitForContainerDependencies(ctx context.Context, name st } switch state { case ctrStateStart: - err := o.waitForContainerToStart(ctx, ctrId, isEssential) - if err != nil { - return err + var errContainerExited *dockerengine.ErrContainerExited + err := o.waitForContainerToStart(ctx, ctrId) + if err != nil && !errors.As(err, &errContainerExited) { + return fmt.Errorf("wait for container %q to start: %w", ctrId, err) } - if isEssential { - log.Successf("Successfully started container %s\n", ctrId) - return nil - } - fmt.Printf("non-essential container %q started successfully\n", ctrId) + log.Successf("Successfully started container %s\n", ctrId) return nil case ctrStateHealthy: healthy, err := o.docker.IsContainerHealthy(ctx, ctrId) if err != nil { - if !isEssential { - fmt.Printf("non-essential container %q is not healthy: %v\n", ctrId, err) - return nil - } - return fmt.Errorf("essential container %q is not healthy: %w", ctrId, err) + return fmt.Errorf("container %q is not healthy: %w", ctrId, err) } if healthy { - log.Successf("Successfully dependency container %q reached %q\n", ctrId, state) + log.Successf("Successfully dependency container %q reached healthy\n", ctrId) return nil } case ctrStateComplete: @@ -552,15 +544,13 @@ func (o *Orchestrator) waitForContainerDependencies(ctx context.Context, name st continue } if err != nil { - return fmt.Errorf("dependency container %q failed to be %q: %w", ctrId, state, err) + return fmt.Errorf("wait for container %q to success: %w", ctrId, err) } if exitCode != 0 { return fmt.Errorf("dependency container %q exited with non-zero exit code %d", ctrId, exitCode) } - if exitCode == 0 { - log.Successf("%q's dependency container %q exited with code: %d\n", name, ctrId, exitCode) - return nil - } + log.Successf("%q's dependency container %q exited with code: %d\n", name, ctrId, exitCode) + return nil } } }) @@ -650,7 +640,7 @@ func (o *Orchestrator) run(taskID int32, opts dockerengine.RunOptions, isEssenti return } if err == nil { - err = errors.New("container stopped unexpectedly") + err = errors.New("essential container stopped unexpectedly") } // cancel context to indicate all the other go routines spawned by `graph.UpwardTarversal`. cancel() diff --git a/internal/pkg/docker/orchestrator/orchestrator_test.go b/internal/pkg/docker/orchestrator/orchestrator_test.go index 62f33077638..f7d69bd6c6f 100644 --- a/internal/pkg/docker/orchestrator/orchestrator_test.go +++ b/internal/pkg/docker/orchestrator/orchestrator_test.go @@ -256,37 +256,6 @@ func TestOrchestrator(t *testing.T) { }, }, - "return error if essential container exits": { - logOptions: noLogs, - runUntilStopped: true, - stopAfterNErrs: 1, - test: func(t *testing.T) (test, *dockerenginetest.Double) { - de := &dockerenginetest.Double{ - IsContainerRunningFn: func(ctx context.Context, name string) (bool, error) { - if name == "prefix-foo" { - return false, &dockerengine.ErrContainerExited{} - } - return true, nil - }, - RunFn: func(ctx context.Context, opts *dockerengine.RunOptions) error { - return nil - }, - StopFn: func(ctx context.Context, name string) error { - return nil - }, - } - return func(t *testing.T, o *Orchestrator) { - o.RunTask(Task{ - Containers: map[string]ContainerDefinition{ - "foo": { - IsEssential: true, - }, - }, - }) - }, de - }, - errs: []string{"upward traversal: check if \"prefix-foo\" is running: container \"\" exited with code 0"}, - }, "success with dependsOn order": { logOptions: noLogs, test: func(t *testing.T) (test, *dockerenginetest.Double) { @@ -391,7 +360,7 @@ func TestOrchestrator(t *testing.T) { }) }, de }, - errs: []string{"upward traversal: wait for container bar dependencies: essential container \"prefix-foo\" is not healthy: container `prefix-foo` is unhealthy"}, + errs: []string{"upward traversal: wait for container bar dependencies: container \"prefix-foo\" is not healthy: container `prefix-foo` is unhealthy"}, }, "return error when dependency container complete failed": { @@ -587,7 +556,7 @@ func TestOrchestrator(t *testing.T) { }, de }, stopAfterNErrs: 1, - errs: []string{`run "prefix-foo": container stopped unexpectedly`}, + errs: []string{`run "prefix-foo": essential container stopped unexpectedly`}, }, "proxy setup, connection returns error": { logOptions: noLogs, From 456c6b899b06d21467c8eb3b90cdc25d13654c66 Mon Sep 17 00:00:00 2001 From: AdithyaKolla Date: Fri, 15 Dec 2023 20:37:36 -0800 Subject: [PATCH 45/53] remove essential from err --- internal/pkg/docker/orchestrator/orchestrator.go | 2 +- internal/pkg/docker/orchestrator/orchestrator_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/pkg/docker/orchestrator/orchestrator.go b/internal/pkg/docker/orchestrator/orchestrator.go index 926a67e6715..cd639365aab 100644 --- a/internal/pkg/docker/orchestrator/orchestrator.go +++ b/internal/pkg/docker/orchestrator/orchestrator.go @@ -640,7 +640,7 @@ func (o *Orchestrator) run(taskID int32, opts dockerengine.RunOptions, isEssenti return } if err == nil { - err = errors.New("essential container stopped unexpectedly") + err = errors.New("container stopped unexpectedly") } // cancel context to indicate all the other go routines spawned by `graph.UpwardTarversal`. cancel() diff --git a/internal/pkg/docker/orchestrator/orchestrator_test.go b/internal/pkg/docker/orchestrator/orchestrator_test.go index f7d69bd6c6f..634e343899c 100644 --- a/internal/pkg/docker/orchestrator/orchestrator_test.go +++ b/internal/pkg/docker/orchestrator/orchestrator_test.go @@ -556,7 +556,7 @@ func TestOrchestrator(t *testing.T) { }, de }, stopAfterNErrs: 1, - errs: []string{`run "prefix-foo": essential container stopped unexpectedly`}, + errs: []string{`run "prefix-foo": container stopped unexpectedly`}, }, "proxy setup, connection returns error": { logOptions: noLogs, From ac862a7016e4692e250bdf70ea24fc1f5bf39a79 Mon Sep 17 00:00:00 2001 From: AdithyaKolla Date: Fri, 15 Dec 2023 21:01:01 -0800 Subject: [PATCH 46/53] fix error msg --- internal/pkg/docker/orchestrator/orchestrator.go | 2 +- internal/pkg/docker/orchestrator/orchestrator_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/pkg/docker/orchestrator/orchestrator.go b/internal/pkg/docker/orchestrator/orchestrator.go index cd639365aab..44c067a693a 100644 --- a/internal/pkg/docker/orchestrator/orchestrator.go +++ b/internal/pkg/docker/orchestrator/orchestrator.go @@ -518,7 +518,7 @@ func (o *Orchestrator) waitForContainerDependencies(ctx context.Context, name st case ctrStateHealthy: healthy, err := o.docker.IsContainerHealthy(ctx, ctrId) if err != nil { - return fmt.Errorf("container %q is not healthy: %w", ctrId, err) + return fmt.Errorf("wait for container %q to be healthy: %w", ctrId, err) } if healthy { log.Successf("Successfully dependency container %q reached healthy\n", ctrId) diff --git a/internal/pkg/docker/orchestrator/orchestrator_test.go b/internal/pkg/docker/orchestrator/orchestrator_test.go index 634e343899c..88d58594afc 100644 --- a/internal/pkg/docker/orchestrator/orchestrator_test.go +++ b/internal/pkg/docker/orchestrator/orchestrator_test.go @@ -360,7 +360,7 @@ func TestOrchestrator(t *testing.T) { }) }, de }, - errs: []string{"upward traversal: wait for container bar dependencies: container \"prefix-foo\" is not healthy: container `prefix-foo` is unhealthy"}, + errs: []string{"upward traversal: wait for container bar dependencies: wait for container \"prefix-foo\" to be healthy: container `prefix-foo` is unhealthy"}, }, "return error when dependency container complete failed": { From f54965841b099cc22657b8204bad6df87b2defa2 Mon Sep 17 00:00:00 2001 From: AdithyaKolla Date: Sat, 16 Dec 2023 13:13:43 -0800 Subject: [PATCH 47/53] add test case --- .../docker/orchestrator/orchestrator_test.go | 53 +++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/internal/pkg/docker/orchestrator/orchestrator_test.go b/internal/pkg/docker/orchestrator/orchestrator_test.go index 88d58594afc..756d953feb6 100644 --- a/internal/pkg/docker/orchestrator/orchestrator_test.go +++ b/internal/pkg/docker/orchestrator/orchestrator_test.go @@ -363,6 +363,59 @@ func TestOrchestrator(t *testing.T) { errs: []string{"upward traversal: wait for container bar dependencies: wait for container \"prefix-foo\" to be healthy: container `prefix-foo` is unhealthy"}, }, + "return error when dependency container is not started": { + logOptions: noLogs, + stopAfterNErrs: 1, + test: func(t *testing.T) (test, *dockerenginetest.Double) { + stopPause := make(chan struct{}) + stopFoo := make(chan struct{}) + de := &dockerenginetest.Double{ + IsContainerRunningFn: func(ctx context.Context, name string) (bool, error) { + if name == "prefix-foo" { + return false, fmt.Errorf("some error") + } + return true, nil + }, + RunFn: func(ctx context.Context, opts *dockerengine.RunOptions) error { + if opts.ContainerName == "prefix-pause" { + // block pause container until Stop(pause) + <-stopPause + } + if opts.ContainerName == "prefix-foo" { + // block bar container until Stop(foo) + <-stopFoo + } + return nil + }, + StopFn: func(ctx context.Context, s string) error { + if s == "prefix-pause" { + stopPause <- struct{}{} + } + if s == "prefix-foo" { + stopFoo <- struct{}{} + } + return nil + }, + } + return func(t *testing.T, o *Orchestrator) { + o.RunTask(Task{ + Containers: map[string]ContainerDefinition{ + "foo": { + IsEssential: false, + }, + "bar": { + IsEssential: true, + DependsOn: map[string]string{ + "foo": ctrStateStart, + }, + }, + }, + }) + }, de + }, + errs: []string{"upward traversal: wait for container foo to start: check if \"prefix-foo\" is running: some error"}, + }, + "return error when dependency container complete failed": { logOptions: noLogs, stopAfterNErrs: 1, From 9aa5bb45e4f36adbe5594dbe2118576edc728803 Mon Sep 17 00:00:00 2001 From: AdithyaKolla Date: Mon, 18 Dec 2023 09:13:45 -0800 Subject: [PATCH 48/53] add non essential info --- internal/pkg/docker/orchestrator/orchestrator.go | 1 + 1 file changed, 1 insertion(+) diff --git a/internal/pkg/docker/orchestrator/orchestrator.go b/internal/pkg/docker/orchestrator/orchestrator.go index 44c067a693a..9cf3a2d2ec3 100644 --- a/internal/pkg/docker/orchestrator/orchestrator.go +++ b/internal/pkg/docker/orchestrator/orchestrator.go @@ -637,6 +637,7 @@ func (o *Orchestrator) run(taskID int32, opts dockerengine.RunOptions, isEssenti if taskID == pauseCtrTaskID || taskID == curTaskID { var errContainerExited *dockerengine.ErrContainerExited if !isEssential && (errors.As(err, &errContainerExited) || err == nil) { + fmt.Printf("non-essential container %q stopped\n", opts.ContainerName) return } if err == nil { From a164eac97ebc188e1b82983a8e0e00172544ba03 Mon Sep 17 00:00:00 2001 From: AdithyaKolla Date: Mon, 18 Dec 2023 21:37:33 -0800 Subject: [PATCH 49/53] address ph fb --- .../pkg/docker/dockerengine/dockerengine.go | 44 ++----------------- internal/pkg/docker/dockerengine/errors.go | 35 +++++++++++++++ 2 files changed, 38 insertions(+), 41 deletions(-) diff --git a/internal/pkg/docker/dockerengine/dockerengine.go b/internal/pkg/docker/dockerengine/dockerengine.go index 9ab55a3d398..ca97affff65 100644 --- a/internal/pkg/docker/dockerengine/dockerengine.go +++ b/internal/pkg/docker/dockerengine/dockerengine.go @@ -303,9 +303,6 @@ func (in *RunOptions) generateRunArguments() []string { // Run runs a Docker container with the sepcified options. func (c DockerCmdClient) Run(ctx context.Context, options *RunOptions) error { - type exitCodeError interface { - ExitCode() int - } // set default options if options.LogOptions.Color == nil { options.LogOptions.Color = color.New() @@ -346,11 +343,11 @@ func (c DockerCmdClient) Run(ctx context.Context, options *RunOptions) error { exec.Stdout(stdout), exec.Stderr(stderr), exec.NewProcessGroup()); err != nil { - var ec exitCodeError - if errors.As(err, &ec) { + var errorContainerExited *ErrContainerExited + if errors.As(err, &errorContainerExited) { return &ErrContainerExited{ name: options.ContainerName, - exitcode: ec.ExitCode(), + exitcode: errorContainerExited.ExitCode(), } } return fmt.Errorf("running container: %w", err) @@ -453,23 +450,6 @@ func (d *DockerCmdClient) containerID(ctx context.Context, containerName string) return strings.TrimSpace(buf.String()), nil } -// ErrContainerExited represents an error when a Docker container has exited. -// It includes the container name and exit code in the error message. -type ErrContainerExited struct { - name string - exitcode int -} - -// ExitCode returns the OS exit code configured for this error. -func (e *ErrContainerExited) ExitCode() int { - return e.exitcode -} - -// ErrContainerExited represents docker container exited with an exitcode. -func (e *ErrContainerExited) Error() string { - return fmt.Sprintf("container %q exited with code %d", e.name, e.exitcode) -} - // Stop calls `docker stop` to stop a running container. func (c DockerCmdClient) Stop(ctx context.Context, containerID string) error { buf := &bytes.Buffer{} @@ -606,21 +586,3 @@ func userHomeDirectory() string { return home } - -type errEmptyImageTags struct { - uri string -} - -func (e *errEmptyImageTags) Error() string { - return fmt.Sprintf("tags to reference an image should not be empty for building and pushing into the ECR repository %s", e.uri) -} - -// ErrContainerNotExited represents an error when a Docker container has not exited. -type ErrContainerNotExited struct { - name string -} - -// Error returns the error message. -func (e *ErrContainerNotExited) Error() string { - return fmt.Sprintf("container %q has not exited", e.name) -} diff --git a/internal/pkg/docker/dockerengine/errors.go b/internal/pkg/docker/dockerengine/errors.go index 9d0f469dcee..d43fbb512ed 100644 --- a/internal/pkg/docker/dockerengine/errors.go +++ b/internal/pkg/docker/dockerengine/errors.go @@ -19,3 +19,38 @@ type ErrDockerDaemonNotResponsive struct { func (e ErrDockerDaemonNotResponsive) Error() string { return fmt.Sprintf("docker daemon is not responsive: %s", e.msg) } + +type errEmptyImageTags struct { + uri string +} + +func (e *errEmptyImageTags) Error() string { + return fmt.Sprintf("tags to reference an image should not be empty for building and pushing into the ECR repository %s", e.uri) +} + +// ErrContainerNotExited represents an error when a Docker container has not exited. +type ErrContainerNotExited struct { + name string +} + +// Error returns the error message. +func (e *ErrContainerNotExited) Error() string { + return fmt.Sprintf("container %q has not exited", e.name) +} + +// ErrContainerExited represents an error when a Docker container has exited. +// It includes the container name and exit code in the error message. +type ErrContainerExited struct { + name string + exitcode int +} + +// ExitCode returns the OS exit code configured for this error. +func (e *ErrContainerExited) ExitCode() int { + return e.exitcode +} + +// ErrContainerExited represents docker container exited with an exitcode. +func (e *ErrContainerExited) Error() string { + return fmt.Sprintf("container %q exited with code %d", e.name, e.exitcode) +} From c0b515bc9821ffb6e88ce57383c6bddd65b03212 Mon Sep 17 00:00:00 2001 From: AdithyaKolla Date: Mon, 18 Dec 2023 23:43:06 -0800 Subject: [PATCH 50/53] use interface --- internal/pkg/docker/dockerengine/dockerengine.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/internal/pkg/docker/dockerengine/dockerengine.go b/internal/pkg/docker/dockerengine/dockerengine.go index ca97affff65..5f216a584b6 100644 --- a/internal/pkg/docker/dockerengine/dockerengine.go +++ b/internal/pkg/docker/dockerengine/dockerengine.go @@ -303,6 +303,9 @@ func (in *RunOptions) generateRunArguments() []string { // Run runs a Docker container with the sepcified options. func (c DockerCmdClient) Run(ctx context.Context, options *RunOptions) error { + type exitCodeError interface { + ExitCode() int + } // set default options if options.LogOptions.Color == nil { options.LogOptions.Color = color.New() @@ -343,11 +346,11 @@ func (c DockerCmdClient) Run(ctx context.Context, options *RunOptions) error { exec.Stdout(stdout), exec.Stderr(stderr), exec.NewProcessGroup()); err != nil { - var errorContainerExited *ErrContainerExited - if errors.As(err, &errorContainerExited) { + var ec exitCodeError + if errors.As(err, &ec) { return &ErrContainerExited{ name: options.ContainerName, - exitcode: errorContainerExited.ExitCode(), + exitcode: ec.ExitCode(), } } return fmt.Errorf("running container: %w", err) From 98377c3e4b2bc8e86827f22e8817d7327d58dcf5 Mon Sep 17 00:00:00 2001 From: Adithya Kolla <71282729+KollaAdithya@users.noreply.github.com> Date: Tue, 19 Dec 2023 11:03:52 -0800 Subject: [PATCH 51/53] Update internal/pkg/graph/graph.go Co-authored-by: Wanxian Yang <79273084+Lou1415926@users.noreply.github.com> --- internal/pkg/graph/graph.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/pkg/graph/graph.go b/internal/pkg/graph/graph.go index d329fdaccde..098db5150d6 100644 --- a/internal/pkg/graph/graph.go +++ b/internal/pkg/graph/graph.go @@ -368,7 +368,7 @@ func (t *graphTraversal[V]) execute(ctx context.Context, lg *LabeledGraph[V]) er processVertices := func(ctx context.Context, graph *LabeledGraph[V], eg *errgroup.Group, vertices []V, vertexCh chan V) { for _, vertex := range vertices { vertex := vertex - // Delay processing this vertex if any of its dependent vertices are yet to be processed. + // If any of the vertices that should be visited before this vertex are yet to be processed, we delay processing it. if len(t.filterPreviousVerticesByStatus(graph, vertex, unvisited)) != 0 { continue } From 369c30d060e21a078496b300e0b1e6fc303d908f Mon Sep 17 00:00:00 2001 From: AdithyaKolla Date: Tue, 19 Dec 2023 11:08:52 -0800 Subject: [PATCH 52/53] add check for visiting --- internal/pkg/graph/graph.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/internal/pkg/graph/graph.go b/internal/pkg/graph/graph.go index 098db5150d6..9769b7423c9 100644 --- a/internal/pkg/graph/graph.go +++ b/internal/pkg/graph/graph.go @@ -369,7 +369,8 @@ func (t *graphTraversal[V]) execute(ctx context.Context, lg *LabeledGraph[V]) er for _, vertex := range vertices { vertex := vertex // If any of the vertices that should be visited before this vertex are yet to be processed, we delay processing it. - if len(t.filterPreviousVerticesByStatus(graph, vertex, unvisited)) != 0 { + if len(t.filterPreviousVerticesByStatus(graph, vertex, unvisited)) != 0 || + len(t.filterPreviousVerticesByStatus(graph, vertex, visiting)) != 0 { continue } // Check if the vertex is already visited or being visited From 045db69bd579a1935b56e8cb8464e996b94e1465 Mon Sep 17 00:00:00 2001 From: AdithyaKolla Date: Tue, 19 Dec 2023 12:42:09 -0800 Subject: [PATCH 53/53] remove unnecessary waitForContainerToStart --- internal/pkg/docker/orchestrator/orchestrator.go | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/internal/pkg/docker/orchestrator/orchestrator.go b/internal/pkg/docker/orchestrator/orchestrator.go index 9cf3a2d2ec3..4ed28be08d0 100644 --- a/internal/pkg/docker/orchestrator/orchestrator.go +++ b/internal/pkg/docker/orchestrator/orchestrator.go @@ -475,6 +475,7 @@ func (o *Orchestrator) waitForContainerToStart(ctx context.Context, id string) e case err != nil: return fmt.Errorf("check if %q is running: %w", id, err) case isRunning: + log.Successf("Successfully started container %s\n", id) return nil } @@ -508,12 +509,6 @@ func (o *Orchestrator) waitForContainerDependencies(ctx context.Context, name st } switch state { case ctrStateStart: - var errContainerExited *dockerengine.ErrContainerExited - err := o.waitForContainerToStart(ctx, ctrId) - if err != nil && !errors.As(err, &errContainerExited) { - return fmt.Errorf("wait for container %q to start: %w", ctrId, err) - } - log.Successf("Successfully started container %s\n", ctrId) return nil case ctrStateHealthy: healthy, err := o.docker.IsContainerHealthy(ctx, ctrId) @@ -533,10 +528,8 @@ func (o *Orchestrator) waitForContainerDependencies(ctx context.Context, name st if err != nil { return fmt.Errorf("wait for container %q to complete: %w", ctrId, err) } - if exitCode != 0 { - log.Successf("%q's dependency container %q exited with code: %d\n", name, ctrId, exitCode) - return nil - } + log.Successf("%q's dependency container %q exited with code: %d\n", name, ctrId, exitCode) + return nil case ctrStateSuccess: exitCode, err := o.docker.ContainerExitCode(ctx, ctrId) var errContainerNotExited *dockerengine.ErrContainerNotExited