-
-
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 all 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() { | ||
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,80 @@ 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 registerNetworkIdForCleanup(String id) { | ||
registeredNetworks.add(id); | ||
} | ||
|
||
/** | ||
* @param networkName the name of the network | ||
* @deprecated see {@link ResourceReaper#registerNetworkIdForCleanup(String)} | ||
*/ | ||
@Deprecated | ||
public void registerNetworkForCleanup(String networkName) { | ||
registeredNetworks.add(networkName); | ||
try { | ||
// Try to find the network by name, so that we can register its ID for later deletion | ||
dockerClient.listNetworksCmd() | ||
.withNameFilter(networkName) | ||
.exec() | ||
.forEach(network -> registerNetworkIdForCleanup(network.getId())); | ||
} catch (Exception e) { | ||
LOGGER.trace("Error encountered when looking up network (name: {})", networkName); | ||
} | ||
} | ||
|
||
/** | ||
* Removes a network by ID. | ||
* @param id | ||
*/ | ||
public void removeNetworkById(String id) { | ||
removeNetwork(id); | ||
} | ||
|
||
/** | ||
* Removes any networks that contain the identifier. | ||
* Removes a network by ID. | ||
* @param identifier | ||
* @deprecated see {@link ResourceReaper#removeNetworkById(String)} | ||
*/ | ||
@Deprecated | ||
public void removeNetworks(String identifier) { | ||
removeNetwork(identifier); | ||
removeNetworkById(identifier); | ||
} | ||
|
||
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(); | ||
} 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 | ||
// Listing by ID first prevents docker-java logging an error if we just go blindly into removeNetworkCmd | ||
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; | ||
} | ||
|
||
// at this point networks should contain either 0 or 1 entries, depending on whether the network exists | ||
// using a for loop we essentially treat the network like an optional, only applying the removal if it exists | ||
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.
Changed this (internally used) method name, as it's always a blocking operation and
invoke
seems more accurate.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.
that's a good change, I was confused by
getDockerCompose(...).start(...)
constructions :)P.S. do we use anything else besides
invoke()
ongetDockerCompose
's return value? Is not, maybe we can simplify it to something likerunWithCompose("down -v")
?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.
Good call, will do!