Skip to content
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.

Sign up for GitHub

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

WIP: Tidy up container teardown code #358

Closed
wants to merge 1 commit into from
Closed

WIP: Tidy up container teardown code #358

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions core/src/main/java/org/testcontainers/containers/DockerComposeContainer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,12 +243,12 @@ public void finished(Description description) {
getDockerCompose("down -v")
.start();

// remove the networks before removing the containers
ResourceReaper.instance().removeNetworks(identifier);

// kill the spawned service containers
spawnedContainerIds.forEach(id -> ResourceReaper.instance().stopAndRemoveContainer(id));
spawnedContainerIds.clear();

// remove the networks after removing the containers
ResourceReaper.instance().removeNetworks(identifier);
}
}

Expand Down
40 changes: 18 additions & 22 deletions core/src/main/java/org/testcontainers/utility/ResourceReaper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package org.testcontainers.utility;

import com.github.dockerjava.api.DockerClient;
import com.github.dockerjava.api.command.InspectContainerResponse;
import com.github.dockerjava.api.exception.DockerException;
import com.github.dockerjava.api.exception.InternalServerErrorException;
import com.github.dockerjava.api.exception.NotFoundException;
Expand All @@ -14,8 +13,11 @@
import java.util.ArrayList;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.concurrent.ConcurrentHashMap;

import static java.util.stream.Collectors.toList;

/**
* Component that responsible for container removal and automatic cleanup of dead containers at JVM shutdown.
*/
Expand Down Expand Up @@ -44,7 +46,6 @@ public synchronized static ResourceReaper instance() {

/**
* Perform a cleanup.
*
*/
public synchronized void performCleanup() {
registeredContainers.forEach(this::stopContainer);
Expand Down Expand Up @@ -85,10 +86,13 @@ public void stopAndRemoveContainer(String containerId, String imageName) {
}

private void stopContainer(String containerId, String imageName) {
boolean running;
boolean present;
try {
InspectContainerResponse containerInfo = dockerClient.inspectContainerCmd(containerId).exec();
running = containerInfo.getState().getRunning();
Optional<Container> container = dockerClient.listContainersCmd().exec()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should avoid getting the data without filters. It has proven to fail in some environments with many containers

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually...... do we even have to check here? Force removal should do the job anyway

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thing I'm trying to avoid is docker-java dumping loud error logs if it tries to remove a container that doesn't exist. The name filter also seems to be responsible for this other error message which we see a lot:

20:35:52.348 ERROR com.github.dockerjava.core.async.ResultCallbackTemplate - Error during callback
com.github.dockerjava.api.exception.InternalServerErrorException: {"message":"invalid character '%' looking for beginning of value"}

I can't remember the exact condition that leads to it (even though it was only a week ago!) but found that the name filters can't be relied upon to work cleanly.

.stream()
.filter(it -> it.getId().equals(containerId))
.findFirst();
present = container.isPresent();
} catch (NotFoundException e) {
LOGGER.trace("Was going to stop container but it apparently no longer exists: {}");
return;
Expand All @@ -97,20 +101,8 @@ private void stopContainer(String containerId, String imageName) {
return;
}

if (running) {
try {
LOGGER.trace("Stopping container: {}", containerId);
dockerClient.killContainerCmd(containerId).exec();
LOGGER.trace("Stopped container: {}", imageName);
} catch (DockerException e) {
LOGGER.trace("Error encountered shutting down container (ID: {}) - it may not have been stopped, or may already be stopped: {}", containerId, e.getMessage());
}
}

try {
dockerClient.inspectContainerCmd(containerId).exec();
} catch (NotFoundException e) {
LOGGER.trace("Was going to remove container but it apparently no longer exists: {}");
if (!present) {
LOGGER.trace("Was going to stop container but it apparently no longer exists: {}");
return;
}

Expand All @@ -130,24 +122,28 @@ 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 networkName the image name of the network
*/
public void registerNetworkForCleanup(String networkName) {
registeredNetworks.add(networkName);
}

/**
* Removes any networks that contain the identifier.
*
* @param identifier
*/
public void removeNetworks(String identifier) {
removeNetwork(identifier);
removeNetwork(identifier);
}

private void removeNetwork(String networkName) {
List<Network> networks;
try {
networks = dockerClient.listNetworksCmd().withNameFilter(networkName).exec();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't it work? Maybe we can play with filter's format?

networks = dockerClient.listNetworksCmd().exec()
.stream()
.filter(network -> network.getName().startsWith(networkName))
.collect(toList());
} catch (DockerException e) {
LOGGER.trace("Error encountered when looking up network for removal (name: {}) - it may not have been removed", networkName);
return;
Expand Down