-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve cleanup for docker-compose. #394
Changes from 1 commit
a7dbb82
2cd6161
cddb22a
8e37969
d142cd2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,7 @@ | |
import com.google.common.base.Splitter; | ||
import com.google.common.collect.Maps; | ||
import com.google.common.util.concurrent.Uninterruptibles; | ||
import org.apache.commons.lang.StringUtils; | ||
import org.junit.runner.Description; | ||
import org.rnorth.ducttape.ratelimits.RateLimiter; | ||
import org.rnorth.ducttape.ratelimits.RateLimiterBuilder; | ||
|
@@ -46,8 +47,9 @@ public class DockerComposeContainer<SELF extends DockerComposeContainer<SELF>> e | |
private final String identifier; | ||
private final Map<String, AmbassadorContainer> ambassadorContainers = new HashMap<>(); | ||
private final List<File> composeFiles; | ||
private Set<String> spawnedContainerIds = Collections.emptySet(); | ||
private Map<String, Integer> scalingPreferences = new HashMap<>(); | ||
private final Set<String> spawnedContainerIds = new HashSet<>(); | ||
private final Set<String> spawnedNetworkIds = new HashSet<>(); | ||
private final Map<String, Integer> scalingPreferences = new HashMap<>(); | ||
private DockerClient dockerClient; | ||
private boolean localCompose; | ||
private boolean pull = true; | ||
|
@@ -117,14 +119,14 @@ public void starting(Description description) { | |
|
||
private void pullImages() { | ||
getDockerCompose("pull") | ||
.start(); | ||
.invoke(); | ||
} | ||
|
||
|
||
private void createServices() { | ||
// Start the docker-compose container, which starts up the services | ||
getDockerCompose("up -d") | ||
.start(); | ||
.invoke(); | ||
} | ||
|
||
private void tailChildContainerLogs() { | ||
|
@@ -158,7 +160,7 @@ private void applyScaling() { | |
} | ||
|
||
getDockerCompose(sb.toString()) | ||
.start(); | ||
.invoke(); | ||
} | ||
} | ||
|
||
|
@@ -173,17 +175,20 @@ private void registerContainersForShutdown() { | |
|
||
// Ensure that the default network for this compose environment, if any, is also cleaned up | ||
ResourceReaper.instance().registerNetworkForCleanup(identifier + "_default"); | ||
spawnedNetworkIds.add(identifier + "_default"); | ||
|
||
// Compose can define their own networks as well; ensure these are cleaned up | ||
dockerClient.listNetworksCmd().exec().forEach(network -> { | ||
if (network.getName().contains(identifier)) { | ||
spawnedNetworkIds.add(network.getId()); | ||
ResourceReaper.instance().registerNetworkForCleanup(network.getId()); | ||
} | ||
}); | ||
|
||
// remember the IDs to allow containers to be killed as soon as we reach stop() | ||
spawnedContainerIds = containers.stream() | ||
spawnedContainerIds.addAll(containers.stream() | ||
.map(Container::getId) | ||
.collect(Collectors.toSet()); | ||
.collect(Collectors.toSet())); | ||
|
||
} catch (DockerException e) { | ||
logger().debug("Failed to stop a service container with exception", e); | ||
|
@@ -240,15 +245,25 @@ public void finished(Description description) { | |
ambassadorContainers.forEach((String address, AmbassadorContainer container) -> container.stop()); | ||
|
||
// Kill the services using docker-compose | ||
getDockerCompose("down -v") | ||
.start(); | ||
try { | ||
getDockerCompose("down -v").invoke(); | ||
|
||
// remove the networks before removing the containers | ||
ResourceReaper.instance().removeNetworks(identifier); | ||
// 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)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the core of the change - we unregister containers and networks if |
||
} catch (ContainerLaunchException e) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe catching just There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
// docker-compose down failed; use ResourceReaper to ensure cleanup | ||
|
||
// remove the networks before removing the containers | ||
spawnedNetworkIds.forEach(id -> ResourceReaper.instance().removeNetworks(identifier)); | ||
|
||
// kill the spawned service containers | ||
spawnedContainerIds.forEach(id -> ResourceReaper.instance().stopAndRemoveContainer(id)); | ||
} | ||
|
||
// kill the spawned service containers | ||
spawnedContainerIds.forEach(id -> ResourceReaper.instance().stopAndRemoveContainer(id)); | ||
spawnedContainerIds.clear(); | ||
spawnedNetworkIds.clear(); | ||
} | ||
} | ||
|
||
|
@@ -372,7 +387,7 @@ interface DockerCompose { | |
|
||
DockerCompose withEnv(Map<String, String> env); | ||
|
||
void start(); | ||
void invoke(); | ||
|
||
default void validateFileList(List<File> composeFiles) { | ||
checkNotNull(composeFiles); | ||
|
@@ -417,7 +432,7 @@ public ContainerisedDockerCompose(List<File> composeFiles, String identifier) { | |
} | ||
|
||
@Override | ||
public void start() { | ||
public void invoke() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changed this (internally used) method name, as it's always a blocking operation and There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that's a good change, I was confused by There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good call, will do! |
||
super.start(); | ||
|
||
this.followOutput(new Slf4jLogConsumer(logger())); | ||
|
@@ -431,6 +446,19 @@ public void start() { | |
logger().info("Docker Compose has finished running"); | ||
|
||
AuditLogger.doComposeLog(this.getCommandParts(), this.getEnv()); | ||
|
||
final Integer exitCode = this.dockerClient.inspectContainerCmd(containerId) | ||
.exec() | ||
.getState() | ||
.getExitCode(); | ||
|
||
if (exitCode == null || exitCode != 0) { | ||
throw new ContainerLaunchException( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As with |
||
"Containerised Docker Compose exited abnormally with code " + | ||
exitCode + | ||
" whilst running command: " + | ||
StringUtils.join(this.getCommandParts(), ' ')); | ||
} | ||
} | ||
} | ||
|
||
|
@@ -468,7 +496,7 @@ public DockerCompose withEnv(Map<String, String> env) { | |
} | ||
|
||
@Override | ||
public void start() { | ||
public void invoke() { | ||
// bail out early | ||
if (!CommandLine.executableExists(COMPOSE_EXECUTABLE)) { | ||
throw new ContainerLaunchException("Local Docker Compose not found. Is " + COMPOSE_EXECUTABLE + " on the PATH?"); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,7 +10,10 @@ | |
import org.slf4j.LoggerFactory; | ||
import org.testcontainers.DockerClientFactory; | ||
|
||
import java.util.*; | ||
import java.util.Collections; | ||
import java.util.List; | ||
import java.util.Map; | ||
import java.util.Set; | ||
import java.util.concurrent.ConcurrentHashMap; | ||
|
||
/** | ||
|
@@ -143,13 +146,6 @@ public void removeNetworks(String identifier) { | |
|
||
private void removeNetwork(String networkName) { | ||
try { | ||
try { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this block should do the job when parameter is id, and "list with filter" is not required anymore There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It could be clearer - I'll do that. |
||
// First try to remove by name | ||
dockerClient.removeNetworkCmd(networkName).exec(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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? |
||
} catch (Exception e) { | ||
LOGGER.trace("Error encountered removing network by name ({}) - it may not have been removed", networkName); | ||
} | ||
|
||
List<Network> networks; | ||
try { | ||
// Then try to list all networks with the same name | ||
|
@@ -172,4 +168,12 @@ private void removeNetwork(String networkName) { | |
registeredNetworks.remove(networkName); | ||
} | ||
} | ||
|
||
public void unregisterNetwork(String identifier) { | ||
registeredNetworks.remove(identifier); | ||
} | ||
|
||
public void unregisterContainer(String identifier) { | ||
registeredContainers.remove(identifier); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not related to this PR, but AFAIK we shouldn't use
contains
here, butstartsWith
or whatever the rule for Docker Compose, otherwise if identifier if "a", then it will delete all networks with "a" character in name :D