Improve cleanup for docker-compose. by rnorth · Pull Request #394 · testcontainers/testcontainers-java

rnorth · 2017-07-08T18:34:12Z

Reduce unnecessary cleanup attempts which cause errors to be logged.
docker-compose down is now trusted to have cleanup up properly if it
exits with a 0 status code.

I hope we can squeeze this in to 1.4.0!

rnorth · 2017-07-08T18:35:34Z

core/src/main/java/org/testcontainers/containers/DockerComposeContainer.java


    @Override
-    public void start() {
+    public void invoke() {


Changed this (internally used) method name, as it's always a blocking operation and invoke seems more accurate.

that's a good change, I was confused by getDockerCompose(...).start(...) constructions :)
P.S. do we use anything else besides invoke() on getDockerCompose's return value? Is not, maybe we can simplify it to something like runWithCompose("down -v") ?

Good call, will do!

Reduce unnecessary cleanup attempts which cause errors to be logged. docker-compose down is now trusted to have cleanup up properly if it exits with a 0 status code.

rnorth · 2017-07-08T18:37:40Z

core/src/main/java/org/testcontainers/containers/DockerComposeContainer.java

+                .getExitCode();
+
+        if (exitCode == null || exitCode != 0) {
+            throw new ContainerLaunchException(


As with LocalDockerCompose, we now throw an exception if a failure occurred. This allows us to detect a failure of docker-compose down.

rnorth · 2017-07-08T18:38:58Z

core/src/main/java/org/testcontainers/utility/ResourceReaper.java

        try {
-            try {
-                // First try to remove by name
-                dockerClient.removeNetworkCmd(networkName).exec();


This seems to always fail and causes an error log. I'm just running some checks that this doesn't let any excess containers survive cleanup.

@rnorth ouch. It doesn't fail when recently added Docker Networks support is being used (because we know the IDs), so I wouldn't remove it.

Since we list networks in Docker Compose's implementation, maybe we can use the IDs there as well?

rnorth · 2017-07-08T18:39:52Z

core/src/main/java/org/testcontainers/containers/DockerComposeContainer.java

+                // If we reach here then docker-compose down has cleared networks and containers;
+                //  we can unregister from ResourceReaper
+                spawnedNetworkIds.forEach(id -> ResourceReaper.instance().unregisterNetwork(identifier));
+                spawnedContainerIds.forEach(id -> ResourceReaper.instance().unregisterContainer(id));


This is the core of the change - we unregister containers and networks if docker-compose down seems to have worked.

…n failed

rnorth · 2017-07-08T19:12:20Z

This should fix #342

bsideup · 2017-07-09T10:27:52Z

core/src/main/java/org/testcontainers/utility/ResourceReaper.java

        try {
-            try {
-                // First try to remove by name
-                dockerClient.removeNetworkCmd(networkName).exec();


@rnorth ouch. It doesn't fail when recently added Docker Networks support is being used (because we know the IDs), so I wouldn't remove it.

Since we list networks in Docker Compose's implementation, maybe we can use the IDs there as well?

bsideup · 2017-07-09T10:30:27Z

core/src/main/java/org/testcontainers/containers/DockerComposeContainer.java

+                //  we can unregister from ResourceReaper
+                spawnedContainerIds.forEach(id -> ResourceReaper.instance().unregisterContainer(id));
+                spawnedNetworkIds.forEach(id -> ResourceReaper.instance().unregisterNetwork(identifier));
+            } catch (ContainerLaunchException e) {


maybe catching just Exception will be better here? Any Exception in invoke will prevent Reaper from being called

bsideup · 2017-07-09T10:31:34Z

core/src/main/java/org/testcontainers/containers/DockerComposeContainer.java

-            ResourceReaper.instance().removeNetworks(identifier);
+                // If we reach here then docker-compose down has cleared networks and containers;
+                //  we can unregister from ResourceReaper
+                spawnedContainerIds.forEach(id -> ResourceReaper.instance().unregisterContainer(id));


FYI spawnedContainerIds.forEach(ResourceReaper.instance()::unregisterContainer); is also valid here :)

Ah interesting - done :)

Not quite sure I follow this:

Since we list networks in Docker Compose's implementation, maybe we can use the IDs there as well?

But I'd interpret this as 'let's only register network IDs, not names'. Is that right? I'll change and make sure we use IDs throughout, then I think we should be OK.

bsideup · 2017-07-09T10:33:59Z

core/src/main/java/org/testcontainers/containers/DockerComposeContainer.java


    @Override
-    public void start() {
+    public void invoke() {


that's a good change, I was confused by getDockerCompose(...).start(...) constructions :)
P.S. do we use anything else besides invoke() on getDockerCompose's return value? Is not, maybe we can simplify it to something like runWithCompose("down -v") ?

bsideup · 2017-07-09T10:40:41Z

core/src/main/java/org/testcontainers/containers/DockerComposeContainer.java

+
            // Compose can define their own networks as well; ensure these are cleaned up
            dockerClient.listNetworksCmd().exec().forEach(network -> {
                if (network.getName().contains(identifier)) {


not related to this PR, but AFAIK we shouldn't use contains here, but startsWith or whatever the rule for Docker Compose, otherwise if identifier if "a", then it will delete all networks with "a" character in name :D

bsideup · 2017-07-09T14:36:07Z

core/src/main/java/org/testcontainers/utility/ResourceReaper.java

-     * @param networkName   the image name of the network
+     * @param id   the ID of the network
     */
-    public void registerNetworkForCleanup(String networkName) {


Since this method was semi-public, maybe it worth keep the old one and mark as deprecated, at least until 1.5.0?

Yep - will reinstate all public methods as they were and deprecate, 👍

bsideup · 2017-07-09T14:36:48Z

core/src/main/java/org/testcontainers/utility/ResourceReaper.java

-    private void removeNetwork(String networkName) {
+    private void removeNetwork(String id) {
        try {
-            try {


I think this block should do the job when parameter is id, and "list with filter" is not required anymore

It could be clearer - I'll do that.
I think we still need to keep the list-with-filter operation though: if the network has already been removed for any reason, dockerClient.removeNetworkCmd will log out an error. So we need to do something to check for the existence of the network first that won't log an error.

Improve comments to aid clarity in removeNetwork method

rnorth added this to the 1.4.0 milestone Jul 8, 2017

rnorth requested a review from bsideup July 8, 2017 18:34

rnorth commented Jul 8, 2017

View reviewed changes

Improve cleanup for docker-compose.

a7dbb82

Reduce unnecessary cleanup attempts which cause errors to be logged. docker-compose down is now trusted to have cleanup up properly if it exits with a 0 status code.

rnorth force-pushed the improve-compose-cleanup branch from 3944eac to a7dbb82 Compare July 8, 2017 18:36

rnorth commented Jul 8, 2017

View reviewed changes

rnorth added 2 commits July 8, 2017 20:08

Swap order of removal of containers vs networks if docker-compose dow…

2cd6161

…n failed

Update changelog

cddb22a

bsideup requested changes Jul 9, 2017

View reviewed changes

bsideup reviewed Jul 9, 2017

View reviewed changes

Further improvements following code review

8e37969

bsideup reviewed Jul 9, 2017

View reviewed changes

bsideup added area/docker-compose enhancement labels Jul 9, 2017

Reinstate semi-public methods and mark as deprecated

d142cd2

Improve comments to aid clarity in removeNetwork method

bsideup approved these changes Jul 9, 2017

View reviewed changes

bsideup merged commit 552937e into master Jul 9, 2017

bsideup deleted the improve-compose-cleanup branch July 9, 2017 19:46

bsideup mentioned this pull request Mar 15, 2018

Double docker-compose shutdown #342

Closed

Uh oh!

Conversation

rnorth commented Jul 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rnorth commented Jul 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rnorth commented Jul 8, 2017 •

edited

Loading