chore(orchestrator): startup and remove containers based on dependencies #5509

KollaAdithya · 2023-11-30T01:43:21Z

This is final PR of Essential and DependsOn
UX with the below manifest config

name: wc2011
type: Load Balanced Web Service

image:
  build: ../../demo/web/Dockerfile

sidecars:
  nginx:
    essential: false
    depends_on:
      wc2011: healthy
  db:
    depends_on:
      wc2011: healthy
      nginx: success

[wc2011] WARNING: The requested image's platform (linux/amd64) does not match the detected host platform (linux/arm64/v8) and no specific platform was requested
[wc2011] 2023/12/06 23:33:00 Hello
[wc2011] 2023/12/06 23:33:00 Listening on port 80...
Waiting for container "nginx" dependencies: [wc2011->healthy]
✔ Successfully dependency container "dhoni-rfer-wc2011-wc2011" reached "healthy"
[nginx] WARNING: The requested image's platform (linux/amd64) does not match the detected host platform (linux/arm64/v8) and no specific platform was requested
[nginx] Exiting with code 0
Waiting for container "db" dependencies: [wc2011->healthy, nginx->success]
✔ Successfully dependency container "dhoni-rfer-wc2011-wc2011" reached "healthy"
✔ Successfully dependency container "dhoni-rfer-wc2011-nginx" reached "success"
[db] WARNING: The requested image's platform (linux/amd64) does not match the detected host platform (linux/arm64/v8) and no specific platform was requested
[db] 2023/12/06 23:34:03 Hello
[db] 2023/12/06 23:34:03 Listening on port 80...
[db] 2023/12/06 23:34:03 listen tcp :80: bind: address already in use

github-actions · 2023-11-30T01:47:41Z

🍕 Here are the new binary sizes!

Name	New size (kiB)	size (kiB)	Delta (%)
macOS (amd)	56980	57312	❤️ -0.58
macOS (arm)	58000	58332	❤️ -0.57
linux (amd)	49964	50256	❤️ -0.58
linux (arm)	49280	49540	❤️ -0.52
windows (amd)	47088	47344	❤️ -0.54

Lou1415926 · 2023-12-15T21:08:52Z

internal/pkg/docker/orchestrator/orchestrator.go

 	for {
 		isRunning, err := o.docker.IsContainerRunning(ctx, id)
 		switch {
 		case err != nil:
+			var errContainerExited *dockerengine.ErrContainerExited
+			if errors.As(err, &errContainerExited) && !isEssential {


do you know what ECS does when a non-essential container fails to start?

In case if Essential Container Exits the deployment circuit breaker will be triggered. But in case if Non Essential container exits ECS will not trigger the circuit breaker and continue to deploy the service.

This is what i see in the Console for NonEssential containers.

yeah I know ECS doesn't error out or anything if the non-essential container has started and exited. I am curious what happens if the container was never able to start. I think it is not super important for us though, just in case you knew on top of you mind (if you don't, don't bother testing it 😂 !)

internal/pkg/docker/orchestrator/orchestrator.go

internal/pkg/graph/graph.go

Lou1415926 · 2023-12-15T22:40:11Z

internal/pkg/docker/orchestrator/orchestrator.go

 	for {
 		isRunning, err := o.docker.IsContainerRunning(ctx, id)
 		switch {
 		case err != nil:
+			var errContainerExited *dockerengine.ErrContainerExited
+			if errors.As(err, &errContainerExited) && !isEssential {


yeah I know ECS doesn't error out or anything if the non-essential container has started and exited. I am curious what happens if the container was never able to start. I think it is not super important for us though, just in case you knew on top of you mind (if you don't, don't bother testing it 😂 !)

Lou1415926 · 2023-12-15T23:01:05Z

internal/pkg/docker/orchestrator/orchestrator.go

@@ -434,16 +464,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 {


ah, we take isEssential as a boolean parameters, instead of letting the callers swallow the exit error from non-essential container - I assume - because all the callers of waitForContainerToStart want the same error handling right? Essentially, we are passing a boolean parameter to avoid code duplication. Is this guess close to what you thought?

This makes sense to me, though I am wary of boolean parameters: among all the cons it has 😂 , it makes code harder to read. For example, it is not obvious what o.waitForContainerToStart(ctx, opts.ContainerName, true) means.

yeah your guess is absolutely right 😆
I want to reduce the duplicacy where each caller has to handle the error accordingly.
I have two options in this case

Adjust the doc comment to make it more clear

Let each caller handle the error scenario of non essential container exited.

Which one do you suggest among these two?

After a brief search I think there are 3 calls to waitForContainerToStart, and only 2 of them need to handle the non-essential case, so the duplication is not bad. Therefore, if you ask me - I would probably have opted for 2. But I will leave the final decision to you 👍🏼

internal/pkg/docker/orchestrator/orchestrator.go

Lou1415926 · 2023-12-16T00:30:44Z

internal/pkg/docker/orchestrator/orchestrator.go

+					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)


nit nit: just because the container could be in non-running state or maybe docker inspect has some unexpected error (which does not indicate "unhealthiness" of the container):

Suggested change

fmt.Printf("non-essential container %q is not healthy: %v\n", ctrId, err)

fmt.Printf("check health status for container %q: %v\n", ctrId, err)

The wrapped error returned from IsContainerHealthy is enough to tell the users whether it's because the container is indeed unhealthy, or it doesn't have health check, or other.

also.....on a second thought, if I have a manifest like this:

sidecars: container1: depends_on: container2: healthy container2: # not essential healtchcheck:...

And then container2 turns out to be unhealthy, should I really start container1 🤔 it seems from the implementation that we will just log and start contaienr1. Curious to hear your thoughts!

I tested this scenario. ECS tries to deploy container2 and it becomes unhealthy. ECS does not try to start container1nor Circuit breaker is triggered. CloudFormation just errors our after 3 hours.

Resource timed out waiting for completion

So Essential-lity and dependsOn are two different entities.
Changed the logic to completely stop and remove all the containers even if non-essential container becomes unhealthy.

Lou1415926 · 2023-12-16T01:10:56Z

internal/pkg/docker/orchestrator/orchestrator.go

+		name, state := name, state
+		eg.Go(func() error {
+			ctrId := o.containerID(name)
+			isEssential := definitions[name].IsEssential


To me, it's a bit counter-intuitive that a container would care about whether the containers that it depends on are essential 💭 For example, if the depends_on specify that container A can start only after container B has started, why would container A care whether container B is essential?

It seems like we are using isEssential here majorly to decide:

whether to error out from waitForContainerToStart

whether to log.Successf or to just fmt.Printf

whether to error out from IsContainerHealthy

For 1,

copilot-cli/internal/pkg/docker/orchestrator/orchestrator.go

Line 246 in 08542d3

return o.waitForContainerToStart(ctx, o.containerID(containerName), a.task.Containers[containerName].IsEssential)

would ⬆️ have been a better place to handle whether to error out or not depending on the essential-ity? Then, we will swallow the "ErrContainerExited` error here (regardless of whether it is essential), because if the contaienr has exited, that means it must have been started.

For 2, I think

copilot-cli/internal/pkg/docker/orchestrator/orchestrator.go

Line 246 in 08542d3

return o.waitForContainerToStart(ctx, o.containerID(containerName), a.task.Containers[containerName].IsEssential)

again is the better place to have that logic.

For 3, I have the same question in https://github.com/aws/copilot-cli/pull/5509/files#r1428611649.

Essentially, I feel like we are mixing two routes of logics here:

To complain or just log when an essential/non-essential contaienr is not functioning

To determine whether to start a container based on its dependency
The function that handles 2, I think, typically should not care about the essential-ity of the containers.

Let me know what you think!

Thank you for pointing me about this. yeah, essential-ity and dependsOn are different entities.
I changed the logic to reflect the above.

iamhopaul123

Awesome! LGTM overall

internal/pkg/docker/dockerengine/dockerengine.go

internal/pkg/graph/graph.go

internal/pkg/docker/orchestrator/orchestrator.go

internal/pkg/graph/graph.go

Co-authored-by: Wanxian Yang <[email protected]>

Lou1415926

Two more questions and we are good to go!!!! Amazing work Adi!

internal/pkg/docker/orchestrator/orchestrator.go

Lou1415926

awesome!

Lou1415926 · 2023-12-19T22:37:32Z

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)


Not for this PR, but some time later after this is merged -

I feel like it'd be easier to refactor the code in the future if we add a logger to Orchestrator:

type Orchestrator struct { logger log.Logger } func New() *Orchestrator { return &Orchestrator { logger: log.New() } } func (o *Orchestrator) waitForContainerToStart(ctx context.Context, id string) error { o.logger.Successf("...") }

This would be easier for us to a) mute the logger by creating a no-op loggeer in New() b) create a logger that logs to an open file instead of stdout/err and c) refactor the codes, because we can easily locate all the logging by "Go to reference" of the logger (vs. now we need to do a global search on the keyword "log")

KollaAdithya and others added 16 commits November 12, 2023 13:34

change exit codes of svc/env/job deploy

8305c0a

move errstruct to errors.go

ee2ee49

parse container deps from manifest

f00e15c

change doc comment

a3a62af

change doc comment

965b133

Merge branch 'aws:mainline' into dependency/containers

9f4debc

change test case name

c4379c3

Address fb from @ CaptainCarpensir

feb4412

fix static check

4ac754d

add docker container methods

18b2db1

modify graph package

87feceb

implement dockerengine chnages and mocks

8176525

add run local changes

e923d35

add WithStatus to graph package

c0ff69d

override container with dependsOn

3b212ee

most of the chnages

6365868

KollaAdithya requested a review from a team as a code owner November 30, 2023 01:43

KollaAdithya requested review from CaptainCarpensir and removed request for a team November 30, 2023 01:43

KollaAdithya marked this pull request as draft November 30, 2023 01:43

KollaAdithya changed the title ~~chore: startup containers based on dependencies~~ chore(orchestrator): startup containers based on dependencies Nov 30, 2023

KollaAdithya closed this Nov 30, 2023

KollaAdithya added 3 commits November 29, 2023 19:15

use errorgroup

3cace54

update mocks

1da538f

add remove of containers in stop

33fecbe

KollaAdithya reopened this Nov 30, 2023

KollaAdithya added 3 commits December 5, 2023 21:25

resolve all merge conflicts

4d5287d

add exitcode interface

8efb61a

add status to buildGraph

2376957

remove mutex in graph traversal

6bc7968

Lou1415926 reviewed Dec 15, 2023

View reviewed changes

address @Lou1415926 fb

1a690f2

Lou1415926 reviewed Dec 15, 2023

View reviewed changes

KollaAdithya added 2 commits December 15, 2023 15:47

address fb @ Lou1415926

075023c

fix more nits

cd497f9

Lou1415926 reviewed Dec 16, 2023

View reviewed changes

internal/pkg/docker/orchestrator/orchestrator.go Outdated Show resolved Hide resolved

use custom error

08542d3

Lou1415926 reviewed Dec 16, 2023

View reviewed changes

KollaAdithya added 6 commits December 15, 2023 19:11

use prevTask

eeb1560

remove mix up of essential and dependsOn

a30c097

remove essential from err

456c6b8

fix error msg

ac862a7

add test case

f549658

add non essential info

9aa5bb4

iamhopaul123 reviewed Dec 19, 2023

View reviewed changes

internal/pkg/docker/dockerengine/dockerengine.go Show resolved Hide resolved

internal/pkg/docker/dockerengine/dockerengine.go Outdated Show resolved Hide resolved

KollaAdithya added 2 commits December 18, 2023 21:37

address ph fb

a164eac

use interface

c0b515b

Lou1415926 reviewed Dec 19, 2023

View reviewed changes

internal/pkg/graph/graph.go Outdated Show resolved Hide resolved

internal/pkg/docker/orchestrator/orchestrator.go Show resolved Hide resolved

internal/pkg/graph/graph.go Outdated Show resolved Hide resolved

KollaAdithya and others added 2 commits December 19, 2023 11:03

Update internal/pkg/graph/graph.go

98377c3

Co-authored-by: Wanxian Yang <[email protected]>

add check for visiting

369c30d

Lou1415926 reviewed Dec 19, 2023

View reviewed changes

internal/pkg/docker/orchestrator/orchestrator.go Outdated Show resolved Hide resolved

internal/pkg/docker/orchestrator/orchestrator.go Outdated Show resolved Hide resolved

remove unnecessary waitForContainerToStart

045db69

Lou1415926 approved these changes Dec 19, 2023

View reviewed changes

Merge branch 'mainline' into implement/graph/dependencies

6d47259

mergify bot merged commit be03ed1 into aws:mainline Dec 19, 2023
12 checks passed

KollaAdithya deleted the implement/graph/dependencies branch December 20, 2023 22:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(orchestrator): startup and remove containers based on dependencies #5509

chore(orchestrator): startup and remove containers based on dependencies #5509

KollaAdithya commented Nov 30, 2023 •

edited

Loading

github-actions bot commented Nov 30, 2023 •

edited

Loading

Lou1415926 Dec 15, 2023

KollaAdithya Dec 15, 2023

Lou1415926 Dec 15, 2023

Lou1415926 Dec 15, 2023

Lou1415926 Dec 15, 2023

KollaAdithya Dec 15, 2023

Lou1415926 Dec 15, 2023

Lou1415926 Dec 16, 2023

Lou1415926 Dec 16, 2023 •

edited

Loading

KollaAdithya Dec 18, 2023

Lou1415926 Dec 16, 2023 •

edited

Loading

KollaAdithya Dec 18, 2023

iamhopaul123 left a comment

Lou1415926 left a comment

Lou1415926 left a comment

Lou1415926 Dec 19, 2023

	fmt.Printf("non-essential container %q is not healthy: %v\n", ctrId, err)
	fmt.Printf("check health status for container %q: %v\n", ctrId, err)

chore(orchestrator): startup and remove containers based on dependencies #5509

chore(orchestrator): startup and remove containers based on dependencies #5509

Conversation

KollaAdithya commented Nov 30, 2023 • edited Loading

github-actions bot commented Nov 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lou1415926 Dec 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lou1415926 Dec 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamhopaul123 left a comment

Choose a reason for hiding this comment

Lou1415926 left a comment

Choose a reason for hiding this comment

Lou1415926 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KollaAdithya commented Nov 30, 2023 •

edited

Loading

github-actions bot commented Nov 30, 2023 •

edited

Loading

Lou1415926 Dec 16, 2023 •

edited

Loading

Lou1415926 Dec 16, 2023 •

edited

Loading