Teardowns the local network on "local stop" #775

efekarakus · 2019-05-20T22:39:04Z

Description of changes: Remove the local endpoints container and the local network if the user doesn't need them anymore. Freeing up resources on their machine.

Enter [N/A] in the box, if an item is not applicable to your change.

Testing

Unit tests passed
Integration tests passed
Unit tests added for new functionality
Listed manual checks and their outputs in the comments (example)
Link to issue or PR for the integration tests: Integration tests for ecs-cli local #772

Documentation

[N/A] Contacted our doc writer
[N/A] Updated our README

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

efekarakus · 2019-05-20T22:58:26Z

Manual Tests

1. `local stop` when only the endpoints container is running

# Given
$ docker ps               
CONTAINER ID        IMAGE                                         COMMAND                  CREATED             STATUS              PORTS               NAMES
697ab62e760c        amazon/amazon-ecs-local-container-endpoints   "/local-container-en…"   55 seconds ago      Up 54 seconds       80/tcp              amazon-ecs-local-container-endpoints
$ docker network ls     
NETWORK ID          NAME                DRIVER              SCOPE
94ac9ec2ce2b        bridge              bridge              local
949837dcb884        ecs-local-network   bridge              local
3803595a49d9        host                host                local
59a17e60cd02        none                null                local

# When
$ dev-ecs-cli local stop                
INFO[0000] The network ecs-local-network has no more running tasks, stopping the endpoints containers... 
INFO[0000] Stopped the amazon-ecs-local-container-endpoints container successfully, removing it... 
INFO[0000] Removed the amazon-ecs-local-container-endpoints container successfully, removing the ecs-local-network network... 
INFO[0000] Removed the ecs-local-network network successfully 

# Then
$ docker ps                  
CONTAINER ID        IMAGE               COMMAND             CREATED             STATUS              PORTS               NAMES
$ docker network ls     
NETWORK ID          NAME                DRIVER              SCOPE
94ac9ec2ce2b        bridge              bridge              local
3803595a49d9        host                host                local
59a17e60cd02        none                null                local

2. `local stop` when a task is running

# Given
$ docker ps                 
CONTAINER ID        IMAGE                                         COMMAND                  CREATED              STATUS              PORTS               NAMES
01bd513e4d4f        ecs-local-other_app                           "/bin/sh -c '/bin/ba…"   10 seconds ago       Up 9 seconds                            ecs-local-other_app_1
03e24e741a3f        amazon/amazon-ecs-local-container-endpoints   "/local-container-en…"   About a minute ago   Up About a minute   80/tcp              amazon-ecs-local-container-endpoints

# When
$ dev-ecs-cli local stop  # does nothing

# Then
$ docker ps                      
CONTAINER ID        IMAGE                                         COMMAND                  CREATED              STATUS              PORTS               NAMES
01bd513e4d4f        ecs-local-other_app                           "/bin/sh -c '/bin/ba…"   About a minute ago   Up About a minute                       ecs-local-other_app_1
03e24e741a3f        amazon/amazon-ecs-local-container-endpoints   "/local-container-en…"   About a minute ago   Up About a minute   80/tcp              amazon-ecs-local-container-endpoints

3. `local stop` when a task is running without local endpoints container

# Given
$ docker ps                  
CONTAINER ID        IMAGE                 COMMAND                  CREATED             STATUS              PORTS               NAMES
01bd513e4d4f        ecs-local-other_app   "/bin/sh -c '/bin/ba…"   2 minutes ago       Up 2 minutes                            ecs-local-other_app_1

# When
$ dev-ecs-cli local stop     
WARN[0000] The ecs-local-other_app_1 container is running in the ecs-local-network network without the amazon-ecs-local-container-endpoints container, please stop it first 

# Then
$ docker ps                      
CONTAINER ID        IMAGE                 COMMAND                  CREATED             STATUS              PORTS               NAMES
01bd513e4d4f        ecs-local-other_app   "/bin/sh -c '/bin/ba…"   3 minutes ago       Up 3 minutes                            ecs-local-other_app_1

4. `local stop` with no running containers in the network

# Given

$ docker network ls
NETWORK ID          NAME                DRIVER              SCOPE
94ac9ec2ce2b        bridge              bridge              local
4cb93d5d5a88        ecs-local-network   bridge              local
3803595a49d9        host                host                local
59a17e60cd02        none                null                local
$ docker container ps --all       
CONTAINER ID        IMAGE               COMMAND             CREATED             STATUS              PORTS               NAMES

# When
$ dev-ecs-cli local stop       
INFO[0000] The network ecs-local-network has no more running tasks, stopping the endpoints containers... 
INFO[0000] Removed the ecs-local-network network successfully 

# Then
$ docker network ls       
NETWORK ID          NAME                DRIVER              SCOPE
94ac9ec2ce2b        bridge              bridge              local
3803595a49d9        host                host                local
59a17e60cd02        none                null                local

nathanpeck · 2019-05-22T16:45:54Z

ecs-cli/modules/cli/local/network/teardown.go

+
+	if len(resp.Containers) > 1 {
+		// Has other containers running in the network
+		return true


I'd suggest adding a log statement here so that users have more visibility in the case where teardown is being skipped because they had launched other containers in the network

Done! Added "%d other task(s) running locally, skipping network removal."

PettitWesley · 2019-05-22T16:59:46Z

ecs-cli/modules/cli/local/local_app.go

+//
+// If the user stops the last running task in the local network then also remove the network.
+func Stop(c *cli.Context) {
+	docker, err := client.NewEnvClient() // Temporary client created to test network.Teardown()


You might want to consider doing something like I did here: https://github.com/awslabs/amazon-ecs-local-container-endpoints/blob/master/local-container-endpoints/clients/docker/client.go#L49

I once or twice ran into weird problems where the client API version was too new for my local docker. If you lock it to the min version that is needed, then customers are safe.

👍 sorry it wasn't clear, the comment meant to denote that this Docker client initialization is temporary. I'll implement the suggestion in a later PR once we actually start working on the Stop and Up commands.

I updated the comment with a TODO and with the link you shared to make this more obvious :)

PettitWesley · 2019-05-22T17:18:51Z

ecs-cli/modules/cli/local/network/teardown.go

+
+	err := d.ContainerStop(ctx, localEndpointsContainerName, nil)
+	if err != nil {
+		if strings.Contains(strings.ToLower(err.Error()), "no such container") {


Why is this needed?

Great question, looks like the comment below wasn't precise enough :P

This error happens if the user stopped all their task containers with docker stop instead of ecs-cli local stop. In that situation, hasRunningTasksInNetwork() will return false and we will try to stop the endpoints container again. But since it was already stopped this error is returned.

We don't want to log.Fatal while trying to stop an already stopped container, it should be a no-op hence this if-statement.

I updated the comment to // The containers in the network were already stopped by the user using "docker stop", do nothing.

An alternative is to not invoke stopEndpointsContainer() or removeEndpointsContainer() if there are 0 containers in the local network.

However, I think that approach pushes complexity upwards instead of downwards (into the function) so I don't prefer it as much. Also if you run ecs-cli local stop on the same task concurrently (two terminals for example) then one of those could trigger this error and fail with fatal instead of continuing cleanly by not doing any work.

Got it... my only concern is that this is yet another place where we are using string matching on errors returned by Docker :/

Yeah, I agree but I think Docker locked themselves and can't change their error messages otherwise they'll break everyone's code :P

Here is a similar example from the agent: https://github.com/aws/amazon-ecs-agent/blob/24c71162368b3a9090689532e5c035fe2bccf133/agent/dockerclient/dockerapi/docker_client.go#L693

This is my least favorite thing in Go...

PettitWesley · 2019-05-24T00:25:47Z

ecs-cli/modules/cli/local/network/setup.go

@@ -77,7 +77,7 @@ const (
 )

 // Setup creates a user-defined bridge network with a running Local Container Endpoints container. If the network
-// already exists or the container is already running then the operation is a no-op.
+// already exists or the container is already running then this function does nothing.


Very minor nit: this change and the other one in teardown.go probably shouldn't be in the "generate mocks" commit.

If you plan to squash all changes to one commit prior to merge, then don't worry about it

PettitWesley · 2019-05-24T00:32:16Z

                                  # #  ( )
                               ___#_#___|__
                           _  |____________|  _
                    _=====| | |            | | |==== _
              =====| |.---------------------------. | |====
<--------------------'   .  .  .  .  .  .  .  .   '--------------/
  \                                                             /
   \___________________________________________________________/
wwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwww
wwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwww
wwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwww

efekarakus added 4 commits May 20, 2019 10:20

Generate LocalEndpointsStopper mocks

7051cee

Rename Configurer to mockStarterCalls and add license

7f0a49b

Update MockLocalEndpointsStopper with ContainerRemove

211d03a

Add "local down" command that teardowns the network

fc87d82

efekarakus requested review from PettitWesley, SoManyHs, allisaurus and piradeepk May 20, 2019 22:39

efekarakus changed the title ~~Teardowns the local network on "local down"~~ Teardowns the local network on "local stop" May 20, 2019

nathanpeck reviewed May 22, 2019

View reviewed changes

PettitWesley reviewed May 22, 2019

View reviewed changes

Improve logging when there are other tasks running locally

e5bba16

PettitWesley reviewed May 24, 2019

View reviewed changes

PettitWesley approved these changes May 24, 2019

View reviewed changes

efekarakus merged commit e5bba16 into aws:ecs-local May 24, 2019

efekarakus deleted the local/teardown branch May 24, 2019 16:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Teardowns the local network on "local stop" #775

Teardowns the local network on "local stop" #775

efekarakus commented May 20, 2019 •

edited

Loading

efekarakus commented May 20, 2019

nathanpeck May 22, 2019

efekarakus May 22, 2019

PettitWesley May 22, 2019

efekarakus May 22, 2019

PettitWesley May 22, 2019

efekarakus May 22, 2019

efekarakus May 22, 2019

PettitWesley May 22, 2019

efekarakus May 23, 2019

PettitWesley May 24, 2019

PettitWesley May 24, 2019

PettitWesley commented May 24, 2019

Teardowns the local network on "local stop" #775

Teardowns the local network on "local stop" #775

Conversation

efekarakus commented May 20, 2019 • edited Loading

efekarakus commented May 20, 2019

Manual Tests

1. local stop when only the endpoints container is running

2. local stop when a task is running

3. local stop when a task is running without local endpoints container

4. local stop with no running containers in the network

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PettitWesley commented May 24, 2019

efekarakus commented May 20, 2019 •

edited

Loading

1. `local stop` when only the endpoints container is running

2. `local stop` when a task is running

3. `local stop` when a task is running without local endpoints container

4. `local stop` with no running containers in the network