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

- run 'stopContainer' in a Future (resolves #518) #528

Merged
merged 1 commit into from
Aug 2, 2016
Merged

- run 'stopContainer' in a Future (resolves #518) #528

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
3 changes: 3 additions & 0 deletions doc/changelog.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
# ChangeLog

* **0.15.14**
- Run 'stopContainer' in a Future to short circuit extra waiting ([#518](https://github.com/fabric8io/docker-maven-plugin/issues/518))

* **0.15.13** (2016-07-29)
- Add <securityOpts> for running containers in special security contexts (#524)
- Add support for multiples network aliases (#466)
Expand Down
104 changes: 69 additions & 35 deletions src/main/java/io/fabric8/maven/docker/service/RunService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,16 @@
*/

import java.util.*;
import java.util.concurrent.Callable;
import java.util.concurrent.ExecutionException;

import io.fabric8.maven.docker.model.Container;
import io.fabric8.maven.docker.log.LogOutputSpecFactory;
import io.fabric8.maven.docker.access.*;
import io.fabric8.maven.docker.config.*;
import io.fabric8.maven.docker.model.Network;
import io.fabric8.maven.docker.util.*;
import io.fabric8.maven.docker.util.WaitUtil.WaitTimeoutException;


/**
Expand Down Expand Up @@ -116,13 +119,13 @@ public String createAndStartContainer(ImageConfiguration imageConfig,
* @param removeVolumes whether to remove volumes after stopping
*/
public void stopContainer(String containerId,
ImageConfiguration imageConfig,
boolean keepContainer,
boolean removeVolumes)
throws DockerAccessException {
ImageConfiguration imageConfig,
boolean keepContainer,
boolean removeVolumes)
throws DockerAccessException {
ContainerTracker.ContainerShutdownDescriptor descriptor =
new ContainerTracker.ContainerShutdownDescriptor(imageConfig,containerId);
shutdown(docker, descriptor, log, keepContainer, removeVolumes);
new ContainerTracker.ContainerShutdownDescriptor(imageConfig, containerId);
shutdown(descriptor, keepContainer, removeVolumes);
}

/**
Expand All @@ -140,7 +143,7 @@ public void stopPreviouslyStartedContainer(String containerId,
throws DockerAccessException {
ContainerTracker.ContainerShutdownDescriptor descriptor = tracker.removeContainer(containerId);
if (descriptor != null) {
shutdown(docker, descriptor, log, keepContainer, removeVolumes);
shutdown(descriptor, keepContainer, removeVolumes);
}
}

Expand All @@ -159,7 +162,7 @@ public void stopStartedContainers(boolean keepContainer,
Set<Network> networksToRemove = new HashSet<>();
for (ContainerTracker.ContainerShutdownDescriptor descriptor : tracker.removeShutdownDescriptors(pomLabel)) {
collectCustomNetworks(networksToRemove, descriptor, removeCustomNetworks);
shutdown(docker, descriptor, log, keepContainer, removeVolumes);
shutdown(descriptor, keepContainer, removeVolumes);
}
removeCustomNetworks(networksToRemove);
}
Expand Down Expand Up @@ -399,10 +402,8 @@ private void updateMappedPortsAndAddresses(String containerId, PortMapping mappe
}
}

private void shutdown(DockerAccess access,
ContainerTracker.ContainerShutdownDescriptor descriptor,
Logger log, boolean keepContainer, boolean removeVolumes)
throws DockerAccessException {
private void shutdown(ContainerTracker.ContainerShutdownDescriptor descriptor, boolean keepContainer, boolean removeVolumes)
throws DockerAccessException {

String containerId = descriptor.getContainerId();
if (descriptor.getPreStop() != null) {
Expand All @@ -412,32 +413,20 @@ private void shutdown(DockerAccess access,
log.error("%s", e.getMessage());
}
}
// Stop the container
int killGracePeriod = descriptor.getKillGracePeriod();
int killGracePeriodInSeconds = (killGracePeriod + 500) / 1000;
if (killGracePeriod != 0 && killGracePeriodInSeconds == 0) {
log.warn("A kill grace period of %d ms leads to no wait at all since its rounded to seconds. " +
"Please use at least 500 as value for wait.kill",killGracePeriod);
}
access.stopContainer(containerId, killGracePeriodInSeconds);
if (killGracePeriod > 0) {
log.debug("Shutdown: Wait %d s after stopping and before killing container", killGracePeriodInSeconds);
WaitUtil.sleep(killGracePeriodInSeconds * 1000);
}

int killGracePeriod = adjustGracePeriod(descriptor.getKillGracePeriod());
log.debug("shutdown will wait max of %d seconds before removing container", killGracePeriod);

long waited = shutdownAndWait(containerId, killGracePeriod);

if (!keepContainer) {
int shutdownGracePeriod = descriptor.getShutdownGracePeriod();
if (shutdownGracePeriod != 0) {
log.debug("Shutdown: Wait %d ms before removing container", shutdownGracePeriod);
WaitUtil.sleep(shutdownGracePeriod);
}
// Remove the container
access.removeContainer(containerId, removeVolumes);
removeContainer(descriptor, removeVolumes, containerId);
Copy link
Collaborator

Choose a reason for hiding this comment

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

There will be only a wait if killGracePeriod is > 0. I think we should do the same with the new logic, which means when no wait specified we shouldn't wait at all.

}

log.info("%s: Stop%s container %s",
descriptor.getDescription(),
(keepContainer ? "" : " and remove"),
containerId.substring(0, 12));
log.info("%s: Stop%s container %s after %s ms",
descriptor.getDescription(),
(keepContainer ? "" : " and removed"),
containerId.substring(0, 12), waited);
}

public void createCustomNetworkIfNotExistant(String customNetwork) throws DockerAccessException {
Expand All @@ -453,4 +442,49 @@ public void removeCustomNetworks(Collection<Network> networks) throws DockerAcce
docker.removeNetwork(network.getId());
}
}

private int adjustGracePeriod(int gracePeriod) {
int killGracePeriodInSeconds = (gracePeriod + 500) / 1000;
if (gracePeriod != 0 && killGracePeriodInSeconds == 0) {
log.warn("A kill grace period of %d ms leads to no wait at all since its rounded to seconds. " +
"Please use at least 500 as value for wait.kill", gracePeriod);
}

return killGracePeriodInSeconds;
}

private void removeContainer(ContainerTracker.ContainerShutdownDescriptor descriptor, boolean removeVolumes, String containerId)
throws DockerAccessException {
int shutdownGracePeriod = descriptor.getShutdownGracePeriod();
if (shutdownGracePeriod != 0) {
log.debug("Shutdown: Wait %d ms before removing container", shutdownGracePeriod);
WaitUtil.sleep(shutdownGracePeriod);
}
// Remove the container
docker.removeContainer(containerId, removeVolumes);
}

private long shutdownAndWait(final String containerId, final int killGracePeriodInSeconds) throws DockerAccessException {
long waited = 0;
try {
waited = WaitUtil.wait(killGracePeriodInSeconds, new Callable<Void>() {
@Override
public Void call() throws Exception {
docker.stopContainer(containerId, killGracePeriodInSeconds);
return null;
}
});
} catch (ExecutionException e) {
if (e.getCause() instanceof DockerAccessException) {
throw (DockerAccessException) e.getCause();
} else {
throw new DockerAccessException(e, "failed to stop container id [%s]", containerId);
}
} catch (WaitTimeoutException e) {
waited = e.getWaited();
log.warn("Stop container id [%s] timed out after %s ms", containerId, waited);
}

return waited;
}
}
23 changes: 22 additions & 1 deletion src/main/java/io/fabric8/maven/docker/util/WaitUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
import java.util.Arrays;
import java.util.Iterator;
import java.util.List;
import java.util.concurrent.TimeoutException;
import java.util.concurrent.*;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

Expand All @@ -25,6 +25,8 @@
*/
public class WaitUtil {

private static final int DEFAULT_DOCKER_TIMEOUT = 10;
Copy link
Collaborator

@rhuss rhuss Aug 2, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

10s maybe a bit high as default, which is even used when no killGracePeriod is specified. In fact I wouldn't wait at all if no killGracePeriod is specified so simply skipping any wait when int wait is null.

Any particular reason why you always want a wait when a shutdown happens ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i don't always want to wait - the docker container takes up to 10 seconds which is why i used that default. if it exited before then, the future would complete, so i don't think we were waiting any extra time w/ this.

but whatever you think is best here.

Copy link
Collaborator Author

@jgangemi jgangemi Aug 2, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

yeah - looking at your change, i'm not 100% sure that's correct either in terms of returning wait = 0 b/c you are always waiting some time for the daemon to complete, so i actually think the original implementation was correct.

get(long timeout, TimeUnit unit)
Waits if necessary for at most the given time for the computation to complete, and then retrieves its result, if available.

if anything maybe it should be if (wait == 0) { wait = DEFAULT } (i can't remember what i originally had and i'm about to walk out the door for something) b/c then we are matched against the daemon itself.

and now that i think about it, maybe that value should be padded by a second or two so we're sure we get the output back from the docker daemon. i think the only time there would be 'extra' waiting is if the docker daemon dies, and at that point you've got bigger problems.


// how long to wait at max when doing a http ping
private static final long DEFAULT_MAX_WAIT = 10 * 1000;

Expand All @@ -48,6 +50,24 @@ public class WaitUtil {

private WaitUtil() {}

public static long wait(int wait, Callable<Void> callable) throws ExecutionException, WaitTimeoutException {
long now = System.currentTimeMillis();
wait = (wait == 0) ? DEFAULT_DOCKER_TIMEOUT : wait;

try {
FutureTask<Void> task = new FutureTask<>(callable);
task.run();

task.get(wait, TimeUnit.SECONDS);
} catch (@SuppressWarnings("unused") InterruptedException e) {
Thread.currentThread().interrupt();
} catch (@SuppressWarnings("unused") TimeoutException e) {
throw new WaitTimeoutException("timed out waiting for execution to complete", delta(now));
}

return delta(now);
}

public static long wait(int maxWait, WaitChecker ... checkers) throws WaitTimeoutException {
return wait(maxWait, Arrays.asList(checkers));
}
Expand Down Expand Up @@ -89,6 +109,7 @@ public static void sleep(long millis) {
Thread.sleep(millis);
} catch (InterruptedException e) {
// ...
Thread.currentThread().interrupt();
}
}

Expand Down
28 changes: 20 additions & 8 deletions src/test/java/io/fabric8/maven/docker/service/RunServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

import io.fabric8.maven.docker.log.LogOutputSpec;
import io.fabric8.maven.docker.util.Logger;
import io.fabric8.maven.docker.util.WaitUtil;
import io.fabric8.maven.docker.access.*;
import io.fabric8.maven.docker.config.*;
import io.fabric8.maven.docker.log.LogOutputSpecFactory;
Expand Down Expand Up @@ -106,36 +107,45 @@ public void shutdownWithoutKeepingContainers() throws Exception {
}
@Test
public void killafterAndShutdownWithoutKeepingContainers() throws Exception {
long start = System.currentTimeMillis();
setupForKillWait();

long start = System.currentTimeMillis();
runService.stopContainer(container, createImageConfig(SHUTDOWN_WAIT, KILL_AFTER), false, false);
assertTrue("Waited for at least " + (SHUTDOWN_WAIT + KILL_AFTER) + " ms",
System.currentTimeMillis() - start >= SHUTDOWN_WAIT + KILL_AFTER);
}

@Test
public void killafterWithoutKeepingContainers() throws Exception {
long start = System.currentTimeMillis();
setupForKillWait();

long start = System.currentTimeMillis();
runService.stopContainer(container, createImageConfig(0, KILL_AFTER), false, false);
assertTrue("Waited for at least " + (KILL_AFTER) + " ms",
System.currentTimeMillis() - start >= KILL_AFTER);
}

private void setupForKillWait() throws DockerAccessException {
// use this to simulate something happened - timers need to be started before this method gets invoked
docker = new MockUp<DockerAccess>() {
@Mock
public void stopContainer(String contaierId, int wait) {
WaitUtil.sleep(KILL_AFTER);
}
}.getMockInstance();

new Expectations() {{
docker.stopContainer(container, (KILL_AFTER + 500) / 1000);
log.debug(anyString, (Object[]) any); minTimes = 1;
docker.removeContainer(container, false);
new LogInfoMatchingExpectations(container, true);
docker.stopContainer(container, (KILL_AFTER + 500) / 1000);
log.debug(anyString, (Object[]) any); minTimes = 1;
docker.removeContainer(container, false);
new LogInfoMatchingExpectations(container, true);
}};
}

@Test
public void shutdownWithoutKeepingContainersAndRemovingVolumes() throws Exception {
new Expectations() {{

docker.stopContainer(container, 0);
log.debug(anyString, (Object[]) any); minTimes = 1;
docker.removeContainer(container, true);
Expand Down Expand Up @@ -362,8 +372,10 @@ final class LogInfoMatchingExpectations extends Expectations {
LogInfoMatchingExpectations(String container, boolean withRemove) {
log.info(withSubstring("Stop"),
anyString,
withRemove ? withSubstring("remove") : withNotEqual(" and remiver"),
withSubstring(container.substring(0,12)));
withRemove ? withSubstring("removed") : withNotEqual(" and removed"),
withSubstring(container.substring(0,12)),
anyLong);
}
}

}
Loading