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

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!

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.

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

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?

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

…n failed

This should fix #342

@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?

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

👍

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.

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") ?

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

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, 👍

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

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

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

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

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

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

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

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

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

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

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

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

Uh oh!

Uh oh!

Improve cleanup for docker-compose. - #394

Improve cleanup for docker-compose.#394bsideup merged 5 commits intomastertestcontainers/testcontainers-java:masterfrom improve-compose-cleanuptestcontainers/testcontainers-java:improve-compose-cleanupCopy head branch name to clipboard

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

Improve cleanup for docker-compose.#394
bsideup merged 5 commits into
masterfrom
improve-compose-cleanup

rnorth commented Jul 8, 2017 •
edited
Loading