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.

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

Allow for graceful container stop #1000

Open
iNikem opened this issue Nov 29, 2018 · 22 comments
Open

Allow for graceful container stop #1000

iNikem opened this issue Nov 29, 2018 · 22 comments

Comments

@iNikem
Copy link
Contributor

iNikem commented Nov 29, 2018

Currently I see only 1 method to stop a running testcontainer: org.testcontainers.containers.GenericContainer#stop. But to my great surprise and dismay that method ends up calling dockerClient.killContainerCmd. So there is no way for graceful shutdown of running container which would allow for the application running inside it to perform any expected cleanup.

Please, provide a graceful stop.

@kiview
Copy link
Member

kiview commented Dec 4, 2018

Yes, you are right in this regard.
Can you provide your use case, so we can have a bit more context why this is a desired feature?

@iNikem
Copy link
Contributor Author

iNikem commented Dec 4, 2018

Well, I think it is not very common one... I use testcontainers as a very convenient library to start/stop docker containers from my Java application. It is kinda "orchestrator", which starts and stops several containers in a sequence to achieve some result. So on one of the steps I have to start a container, wait for some externally observable effect and then stop this container. But I need to ensure that all shutdown hooks of that application (Spring's @PreDestroy annotations) complete successfully. Currently it is not possible, container is killed too soon.

@kiview
Copy link
Member

kiview commented Dec 4, 2018

Okay I see.
We will definitely consider this when working on Testcontainers 2.0, since this will probably also include a general purpose object oriented container API.

We can probably also add this to the current version as a manually callable method on GenericContainer.

@iNikem
Copy link
Contributor Author

iNikem commented Dec 4, 2018

Yes, that was what I thinking about. IMO ideally we should introduce new method GenericContainer.kill, which will result in killContainerCmd and change current stop method to do stopContainerCmd. But this is backward incompatible change. So may be it is easier to make new method GenericContainer.stopGracefully, which clients could call if needed.

@kiview
Copy link
Member

kiview commented Dec 4, 2018

Yes, while I'd also like to align our API more closely to the domain language used by Docker (start/stop/pause/kill), I don't think the breaking change is worth it at the moment.

So adding a more explicit method makes sense IMO.

@iNikem
Copy link
Contributor Author

iNikem commented Jun 26, 2019

Hei, guys. What do you think about marking this for 2.0 milestone? :)

@iNikem
Copy link
Contributor Author

iNikem commented Aug 14, 2019

I have taken a look how easy it would be for me to propose a PR. I was unable to find a suitable place for my new proposed methods. Currently container.stop delegates directly to ResourceReaper. It would be strange if the latter was able to just stop containers, without removing them. So it seems to me that my original wish is not so easy implemented.

On the other hand, in our particular case we have found a satisfying workaround (shutting applications down via Spring Boot's actuator), so this issue does not bother us any more at all.

As a result, I suggest just closing this issue as not relevant.

@jesusha123
Copy link

This proposal is still useful for test cases that involve microservice disconnections/reconnections due to docker containers stopping.

@vsuharnikov
Copy link

Also it is strange, that stop stops and removes a container from the point of a Docker user (sooner or later you have to deal with the Docker).

@marcphilipp
Copy link
Contributor

I think a good first step would be to keep everything as-is but use a stop command instead of a kill one. The default timeout of 10s should be fine in most cases. If necessary, it could be made configurable or an overloaded stop(Duration) method could be added. WDYT?

@bsideup
Copy link
Member

bsideup commented May 12, 2021

There is containerIsStopping that one can use to execute stopContainerCmd. It is also possible to stop the container externally:

container.getDockerClient().stopContainerCmd(container.getContainerId()).exec();

The default timeout of 10s should be fine in most cases.
Most of containers don't need a graceful shutdown, and it would only slow down the tests.

Also note that stopping and then starting a container won't work properly (one of the use cases for not killing the container from stop), as Docker will assign new random ports and other state adjustments. For those who want to test what happens when the process is restarted, it is recommended to restart the process inside the container and not the container itself.

Given this, I suggest we close this issue. @rnorth @kiview WDYT?

@marcphilipp
Copy link
Contributor

Thanks, that's slightly better than overriding stop(). However, in order to make it as fault tolerant as stop(), an additional inspect command is still necessary before sending the stop command. Moreover, it feels a little awkward to send a stop command in a method called containerIsStopping. While this is a viable workaround, a way to configure that a container wants to be stopped gracefully or alternatively setting a timeout which defaults to 0 would be more user-friendly in my opinion.

@bsideup
Copy link
Member

bsideup commented May 13, 2021

an additional inspect command is still necessary before sending the stop command

uh? Why?

Moreover, it feels a little awkward to send a stop command in a method called containerIsStopping

Yes, there is an overall confusion of stop() in Testcontainers (meaning "terminate") and in Docker ("stop but keep the instance"). We will definitely improve it in the future versions.

a way to configure that a container wants to be stopped gracefully or alternatively setting a timeout which defaults to 0 would be more user-friendly in my opinion.

We're very usecase-driven, plus we don't want to send a wrong message that, in general, containers should be gracefully terminated, given the domain of testing.
Graceful termination is a very rare thing, plus it would only work if the JVM terminates them (Ryuk would forcibly kill them without waiting for anything).

If it is really needed, I suggest doing it manually and explicitly is the best approach. Not to mention that as soon as we introduce any concept of graceful termination, people will start asking for configurable timeouts, retries, custom termination signals, etc etc. And, even if we add it to the JVM termination process, it would be hard to add it to Ryuk, and this would break the expectations.

@marcphilipp
Copy link
Contributor

uh? Why?

TBH I was just cargo-culting what ResourceReaper does.

Graceful termination is a very rare thing

In my case, I want to test that a container does handle SIGTERM properly which is arguably not the main use case of Testcontainers.

Not to mention that as soon as we introduce any concept of graceful termination, people will start asking for configurable timeouts, retries, custom termination signals, etc etc.

I can certainly relate to the trade-offs of adding such a feature being an open-source maintainer myself. 😉

@bsideup
Copy link
Member

bsideup commented May 13, 2021

In my case, I want to test that a container does handle SIGTERM properly which is arguably not the main use case of Testcontainers.

In this case, I would highly recommend to do it explicitly, as part of the test's scenario 👍
FYI you may want to use killContainerCmd and not stopContainerCmd and specify the signal explicitly.

I can certainly relate to the trade-offs of adding such a feature being an open-source maintainer myself. 😉

:D I knew you would understand it :)

@marcphilipp
Copy link
Contributor

FYI you may want to use killContainerCmd and not stopContainerCmd and specify the signal explicitly.

Thanks for the tip!

@big-andy-coates
Copy link
Contributor

big-andy-coates commented Dec 13, 2022

Hey people,

Another use-case for a graceful stop if when capturing code coverage of the process running in the container with the Jacoco agent. The most reliable way to capture coverage is to have it dump coverage on exit, but that requires a graceful shutdown...

@koscejev
Copy link

koscejev commented Jan 2, 2023

Hi everyone, here's a use-case we have for this issue and it also involves a JVM agent.

We're building our production system as a docker container and then running tests against it, so that system-under-test itself and all the dependencies are running as docker containers using testcontainers with a common network. Currently we're checking out possible transition to spring-native with GraalVM. It provides a special agent that needs to run in JVM while tests are running. So that would be pretty easy for us to do we thought originally, but... the agent only writes out the results when JVM is terminated gracefully. So graceful stop of the container would be very helpful to avoid ugly workarounds.

@shaburov
Copy link

shaburov commented Mar 30, 2023

I'll add my pain from ticket #2608
I have a java service running in testcontainers, and due to the fact that the container is killed, and not stopped via SIGTERM, the application does not correctly close the pool of connections to the database and then they hang idle on the database side.
This is a bug, not a type/feature.
The problem is 5 years old. Maybe you can take the time to fix it?

@big-andy-coates
Copy link
Contributor

big-andy-coates commented Mar 30, 2023

Now @shaburov, I'm all up for encouraging them to fix this by highlighting more use cases where this is an issue.

But, personally, I'd say comments like Maybe you can take the time to fix it aren't exactly inspiring those helpful souls who maintain this open-source project. Many or all of those same souls likely doing the work for free.

Maybe a more productive approach would be to raise a PR or to offer up some sponsorship money to prioritising this bug, if it's causing you or your company enough pain to be worth it. Of course, the same can be said to me too!

@shaburov
Copy link

@big-andy-coates Thank you for taking on the task.
English is not my native language. Apparently you misunderstood me because of some dryness of the presentation of the problem (professional deformation of QA). My phrase should be taken as a question and a request, and not as a sarcasm or something like that. I know well what support for an open source project is and I definitely didn’t want to upset anyone.
The labor of an open source developer is noble!

@YoavZinger
Copy link

Hey everyone!
I got into this thread by searching a way to run a command on test container stop.
My use case is combining mongo-atlas features.
When the test containers start, I'm running a command that creates a db, a collection, several users and indexes.
The db need to be remote and not on a container as I'm using several Atlas features.
I want to delete all the created resources after all the tests are ending (the containers are being created once for all the tests, before the tests run, using a parent class with static block).

I thought about executing a command in an @AfterAll function, but Several different test classes need to use this db, so I need to keep a state of the running classes. Furthermore, when writing and debugging the tests I might not run all of them, so I need a more general solution.

After reading this thread, if I understand correctly, the container is being "killed" after all the tests are running, so there isn't a possibility to execute a command on stop.

Is there a workaround for this issue that I missed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants