-
-
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 4 commits
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; | ||
|
@@ -116,15 +118,13 @@ public void starting(Description description) { | |
} | ||
|
||
private void pullImages() { | ||
getDockerCompose("pull") | ||
.start(); | ||
runWithCompose("pull"); | ||
} | ||
|
||
|
||
private void createServices() { | ||
// Start the docker-compose container, which starts up the services | ||
getDockerCompose("up -d") | ||
.start(); | ||
// Run the docker-compose container, which starts up the services | ||
runWithCompose("up -d"); | ||
} | ||
|
||
private void tailChildContainerLogs() { | ||
|
@@ -137,16 +137,18 @@ private void tailChildContainerLogs() { | |
); | ||
} | ||
|
||
private DockerCompose getDockerCompose(String cmd) { | ||
private void runWithCompose(String cmd) { | ||
final DockerCompose dockerCompose; | ||
if (localCompose) { | ||
dockerCompose = new LocalDockerCompose(composeFiles, identifier); | ||
} else { | ||
dockerCompose = new ContainerisedDockerCompose(composeFiles, identifier); | ||
} | ||
return dockerCompose | ||
|
||
dockerCompose | ||
.withCommand(cmd) | ||
.withEnv(env); | ||
.withEnv(env) | ||
.invoke(); | ||
} | ||
|
||
private void applyScaling() { | ||
|
@@ -157,8 +159,7 @@ private void applyScaling() { | |
sb.append(" ").append(scale.getKey()).append("=").append(scale.getValue()); | ||
} | ||
|
||
getDockerCompose(sb.toString()) | ||
.start(); | ||
runWithCompose(sb.toString()); | ||
} | ||
} | ||
|
||
|
@@ -171,19 +172,18 @@ private void registerContainersForShutdown() { | |
containers.forEach(container -> | ||
ResourceReaper.instance().registerContainerForCleanup(container.getId(), container.getNames()[0])); | ||
|
||
// Ensure that the default network for this compose environment, if any, is also cleaned up | ||
ResourceReaper.instance().registerNetworkForCleanup(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)) { | ||
ResourceReaper.instance().registerNetworkForCleanup(network.getId()); | ||
spawnedNetworkIds.add(network.getId()); | ||
ResourceReaper.instance().registerNetworkIdForCleanup(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 +240,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 { | ||
runWithCompose("down -v"); | ||
|
||
// If we reach here then docker-compose down has cleared networks and containers; | ||
// we can unregister from ResourceReaper | ||
spawnedContainerIds.forEach(ResourceReaper.instance()::unregisterContainer); | ||
spawnedNetworkIds.forEach(ResourceReaper.instance()::unregisterNetwork); | ||
} catch (Exception e) { | ||
// docker-compose down failed; use ResourceReaper to ensure cleanup | ||
|
||
// remove the networks before removing the containers | ||
ResourceReaper.instance().removeNetworks(identifier); | ||
// kill the spawned service containers | ||
spawnedContainerIds.forEach(ResourceReaper.instance()::stopAndRemoveContainer); | ||
|
||
// remove the networks after removing the containers | ||
spawnedNetworkIds.forEach(ResourceReaper.instance()::removeNetworkById); | ||
} | ||
|
||
// kill the spawned service containers | ||
spawnedContainerIds.forEach(id -> ResourceReaper.instance().stopAndRemoveContainer(id)); | ||
spawnedContainerIds.clear(); | ||
spawnedNetworkIds.clear(); | ||
} | ||
} | ||
|
||
|
@@ -372,7 +382,7 @@ interface DockerCompose { | |
|
||
DockerCompose withEnv(Map<String, String> env); | ||
|
||
void start(); | ||
void invoke(); | ||
|
||
default void validateFileList(List<File> composeFiles) { | ||
checkNotNull(composeFiles); | ||
|
@@ -417,7 +427,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 +441,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 +491,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; | ||
|
||
/** | ||
|
@@ -127,49 +130,50 @@ private void stopContainer(String containerId, String imageName) { | |
/** | ||
* Register a network to be cleaned up at JVM shutdown. | ||
* | ||
* @param networkName the image name of the network | ||
* @param id the ID of the network | ||
*/ | ||
public void registerNetworkForCleanup(String networkName) { | ||
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. Since this method was semi-public, maybe it worth keep the old one and mark as deprecated, at least until 1.5.0? 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. Yep - will reinstate all public methods as they were and deprecate, 👍 |
||
registeredNetworks.add(networkName); | ||
public void registerNetworkIdForCleanup(String id) { | ||
registeredNetworks.add(id); | ||
} | ||
|
||
/** | ||
* Removes any networks that contain the identifier. | ||
* @param identifier | ||
* @param id | ||
*/ | ||
public void removeNetworks(String identifier) { | ||
removeNetwork(identifier); | ||
public void removeNetworkById(String id) { | ||
removeNetwork(id); | ||
} | ||
|
||
private void removeNetwork(String networkName) { | ||
private void removeNetwork(String id) { | ||
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 | ||
networks = dockerClient.listNetworksCmd().withNameFilter(networkName).exec(); | ||
// Try to find the network if it still exists | ||
networks = dockerClient.listNetworksCmd().withIdFilter(id).exec(); | ||
} catch (Exception e) { | ||
LOGGER.trace("Error encountered when looking up network for removal (name: {}) - it may not have been removed", networkName); | ||
LOGGER.trace("Error encountered when looking up network for removal (name: {}) - it may not have been removed", id); | ||
return; | ||
} | ||
|
||
for (Network network : networks) { | ||
try { | ||
dockerClient.removeNetworkCmd(network.getId()).exec(); | ||
registeredNetworks.remove(network.getId()); | ||
LOGGER.debug("Removed network: {}", networkName); | ||
LOGGER.debug("Removed network: {}", id); | ||
} catch (Exception e) { | ||
LOGGER.trace("Error encountered removing network (name: {}) - it may not have been removed", network.getName()); | ||
} | ||
} | ||
} finally { | ||
registeredNetworks.remove(networkName); | ||
registeredNetworks.remove(id); | ||
} | ||
} | ||
|
||
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