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 Restart of Containers #606

Closed
corbs9-zz opened this issue Mar 13, 2018 · 23 comments
Closed

Allow Restart of Containers #606

corbs9-zz opened this issue Mar 13, 2018 · 23 comments

Comments

@corbs9-zz
Copy link

Currently, test containers does not allow you to restart a running container without stopping it and removing it completely, followed by a start.

Allowing the functionality to restart a running container which issues a docker restart ensures the testability of a container in a multitude of other ways.

Such an example could relate to custom initialisation scripts which create data, and issuing a restart to the container ensures that this data isn't created twice.

@rnorth
Copy link
Member

rnorth commented Mar 13, 2018

Hi @corbs9
Thanks for the suggestion - this seems like a good idea. I can imagine there being two possible approaches to implementation inside Testcontainers:

  • Just do a stop/start via the docker API
  • A more thorough approach that does the above and also uses configured wait strategies etc to make sure the container is ready

I think I'd prefer the latter, but it would be much more work.

Keen to hear others' thoughts here (also contributions would be most welcome 😄 )
Rich

@corbs9-zz
Copy link
Author

corbs9-zz commented Mar 13, 2018

Hi @rnorth,

I thought initially about just restarting the containers, but it seems that port mapping info, for example, is created once at container creation and wouldn't be updated when a docker restart is issued.

I have forked the project and plan to do the following:

  • In ResourceReaper, split out the stopContainer() command into stop and remove
  • In GenericContainer move the functionality from tryStart() into two methods, tryCreate() which will create the container, and tryStart() which will start the created container; there will be another method called tryRun() which is inline with Docker terminology which will create and then start the container.
  • Once this refactor is done, create a new restart() method in GenericContainer which will utilise start and stop but also update any attributes that have changed (for example the ports).

What are your thoughts on this?

Oli

@kiview
Copy link
Member

kiview commented Mar 13, 2018

Hi @corbs9,

Sounds like a good idea in general.
Regarding the ResourceReaper: We are currently killing the container, so depending on the use case, a stop would need to be more graceful (thinking about databases here).

The changes in GenericContainer also sound good and should be okay, since these methods are private.

In addition to the restart() method, we could then also add a pause() method. I'm a bit worried about having different semantics then the docker stop and docker pause commands, but we already kind of have this problem.

@corbs9-zz
Copy link
Author

HI @kiview,

WRT differing docker semantics, renaming the start() command to the more aptly named run() would be a breaking change as GenericContainer is the main entry point into the application. I'd therefore accept that start would be a synonym for run, and in test containers, start and run perform the same job. I would try and limit the disparity between docker semantics and test containers to this only, and try to match 1-2-1 where possible.

Good mention on the ResourceReaper. I will perhaps add a method which signifies that this is a graceful operation as opposed to the default kill command.

WRT the pause() method: what is the intention of this?

Oli

@bsideup
Copy link
Member

bsideup commented Mar 14, 2018

Hi @corbs9,

FYI there is an upcoming huge (from a code respective) change in GenericContainer #600, you might want to wait for it before changing the GenericContainer class :)

@corbs9-zz
Copy link
Author

Hi @bsideup,

Thanks for the heads up. I made a bit of a start last night, but it was just refactoring GenericContainer to separate out start and stop etc.

Is there an ETA on #600?

Oli

@kiview
Copy link
Member

kiview commented Mar 14, 2018

@corbs9
So assuming we won't change our public API, stop() would still stop and remove a container. I of course see the use case for your restart() method. A public pause() method would however allow to stop the container (meaning docker stop), maybe check something on the SUT and then start it again.

Seems nice for some resilience test scenarios, especially for system tests.

@behrangsa
Copy link

Another use case is simulating an out of service condition in integration service:

Start containers
Ensure subject under test is working
Stop one or more containers
Ensure subject under test gracefully tolerates the fault
Restart the stopped containers
Ensure subject under test resumes normal operation
Verify state of containers (e.g. database rows) is as expected

@kiview
Copy link
Member

kiview commented Mar 28, 2018

Hey @behrangsa, you are right, that's also the use case I was explaining above.

@behrangsa
Copy link

@kiview docker has a built in pause command too. So the pause method added to testcontainers api can call that instead of issuing docker stop.

@kiview
Copy link
Member

kiview commented Mar 30, 2018 via email

@chungngoops
Copy link
Contributor

chungngoops commented Oct 16, 2018

Hi,
this is my approach to support restart container without losing its configuration:

  • In ResourceReaper: separate stopContainer(String containerId, String imageName) into two methods:
    • shutdown(String containerId, String imageName) : stop the container.
    • removeContainer(String containerId): removes container with its assosicated volumes.
  • In GenericContainer: make a turnOn() method that similar to the current tryStart(Profiler profiler) but we don't need the beginning part that creates container for the image.
    So the restart() will be a combination of shutdown and turnOn

Let me know your thoughts?
ChungNguyen

@kiview
Copy link
Member

kiview commented Oct 21, 2018

Hey @chungngoops, thanks for your input.

Regarding ResourceReaper, seperation of stopping and removing makes sense, but it's also already implemented. We have stopContainer() and stopAndRemoveContainer(), which is already a composition. The problem is, GenericContainer's stop() method is calling stopAndRemoveContainer() and stopContainer() is a private method. So we can probably just make the method public and reuse it.

Also we must keep in mind, that we want to submit a stop container command, instead of the kill container command that is currently issued in stopContainer().

Regarding starting in GenericContainer, having seperate createContainer() and startContainer() methods makes a lot of sense. Also keep in mind, that we should probably use the wait strategy when restarting the container.

We should also try to keep close to Docker semantics when introducing changes. We are currently not really aligned (our start() is more similar to Docker's run), but we should try to avoid to introduce even more verbs into our domain.

@stale
Copy link

stale bot commented Jan 19, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you believe this is a mistake, please reply to this comment to keep it open. If there isn't one already, a PR to fix or at least reproduce the problem in a test case will always help us get back on track to tackle this.

@stale stale bot added the stale label Jan 19, 2019
@rnorth
Copy link
Member

rnorth commented Jan 19, 2019

Not stale - PR pending.

@stale stale bot removed the stale label Jan 19, 2019
@stale
Copy link

stale bot commented Apr 19, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you believe this is a mistake, please reply to this comment to keep it open. If there isn't one already, a PR to fix or at least reproduce the problem in a test case will always help us get back on track to tackle this.

@stale stale bot added the stale label Apr 19, 2019
@stale
Copy link

stale bot commented May 3, 2019

This issue has been automatically closed due to inactivity. We apologise if this is still an active problem for you, and would ask you to re-open the issue if this is the case.

@stale stale bot closed this as completed May 3, 2019
@chungngoops
Copy link
Contributor

not stale - I am working on the PR.

@semistone
Copy link

I use

    public void pause() {
        this.dockerClient.pauseContainerCmd(containerId).exec();
    }

    public void unpause() {
        this.dockerClient.unpauseContainerCmd(containerId).exec();
    }

to pause and unpause container

@asafm
Copy link
Contributor

asafm commented Aug 15, 2020

What happened to this issue - did you end up adding restart command to GenericContainer? I can't seem to find one.

@bsideup
Copy link
Member

bsideup commented Aug 15, 2020

@asafm since restarting the container would cause it to re-assign the ports and a few other things, we decided to not have this functionality for now.

What's your use case?

@asafm
Copy link
Contributor

asafm commented Aug 16, 2020

Well, I have a small java-agent I wrote couple of years ago - jmx2graphite - which spits out the MBeans to Graphite. It had a known issue I wanted to fix but couldn't find the time: When ever Graphite server got restarted, writes after a while started failing on Broken pipe. So I wanted to simulate it, by restarting the Graphite container:

        graphiteContainer.getDockerClient()
                         .restartContainerCmd(graphiteContainer.getContainerId())
                         .exec();

Unfortunately, it didn't work, since I couldn't get that broken pipe I wanted.
Stop / start did work, but it changed the ports (restart command changed ports as well), and I had to remain on same port to simulate real life, so I ended binding fixed port outside.

Long story short, restart won't help my use case :)

@Shmayro
Copy link

Shmayro commented Aug 25, 2022

A possible solution to restart a container :
https://stackoverflow.com/a/73489794/6485371

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

9 participants