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

Wait for all containers to exit #1754

Merged
merged 4 commits into from
Jul 30, 2015

Conversation

aanand
Copy link

@aanand aanand commented Jul 22, 2015

Behold: the rainbow.

stop

I've also made it so that if an exception gets raised when attaching to a container, it gets raised in the main thread and docker-compose up exits immediately. This should fix the problem @icy was trying to solve in #1723.

This is a hard one to integration test without resorting to threads or multiprocessing, but I've written some unit tests for Multiplexer that assert the basic logic.

Would like to get some feedback on the approach and the change in functionality (which may surprise some users and qualify as breaking backwards-compatibility) before this is merged.

@icy
Copy link

icy commented Jul 22, 2015

Perfect. I love this!

@iafilatov
Copy link

Cool! Now I can get rid of the ugly up -d, ps | grep Up -> logs -> ps | grep Up -> ... loop. Thanks!

@dnephin
Copy link

dnephin commented Jul 27, 2015

LGTM

@aanand aanand force-pushed the wait-for-all-containers-to-exit branch 2 times, most recently from 549e707 to 76e9de7 Compare July 28, 2015 12:00
@aanand aanand changed the title WIP: Wait for all containers to exit Wait for all containers to exit Jul 28, 2015
@aanand aanand added this to the 1.5.0 milestone Jul 28, 2015
Signed-off-by: Aanand Prasad <aanand.prasad@gmail.com>
Signed-off-by: Aanand Prasad <aanand.prasad@gmail.com>
Signed-off-by: Aanand Prasad <aanand.prasad@gmail.com>
Signed-off-by: Aanand Prasad <aanand.prasad@gmail.com>

Conflicts:
	compose/cli/multiplexer.py
@vad
Copy link

vad commented Nov 5, 2015

I'm currently using docker-compose for tests, with a main container that runs the application (with the tests) and a few other containers with databases/elasticsearch/...

In compose 1.4 I could just do:

docker-compose rm -f
docker-compose build
docker-compose up --timeout 1 --no-build

When the application tests end, compose would exit and the tests finish. Are there best practices to change this to docker-compose 1.5?

Thank you

@dnephin
Copy link

dnephin commented Nov 5, 2015

Option 1

I would recommend changing your test container to have a default no-op command (something like echo, or true), and doing:

docker-compose rm -f
docker-compose build
docker-compose up --timeout 1 --no-build -d
docker-compose run tests test_command...
docker-compose stop

Using run allows you to get the exit status from the test run, and you only see the output of the tests (not all the dependencies).

Option 2

If you wanted to continue to use up, you could do something with multiple docker-compose files.

  • remove the test runner service from the default docker-compose.yml
  • create a new file with the test runner service called docker-compose.test.yml
    Then you can run with
docker-compose -f docker-compose.yml -f docker-compose.test.yml rm -f
docker-compose -f docker-compose.yml -f docker-compose.test.yml build
docker-compose up --timeout 1 --no-build -d  # Only uses the default file without the test runner service
docker-compose up -f docker-compose.yml -f docker-compose.test.yml up

@hamzakc
Copy link

hamzakc commented Nov 9, 2015

Thanks @dnephin option 1 worked like a charm :)

@cgeoffroy
Copy link

What was the logic for merging this as the default behaviour into the main branch without adding a flag to bypass it ?.

The "stop when any container exits" behaviour was the default until 1.5.0 . Which means it is a major change without any proper way to opt out.

Now, I got weird deployment issues were a system continue to run with halted containers. The only solutions left are to:

@dnephin
Copy link

dnephin commented Nov 10, 2015

@cgeoffroy can you tell us more about your use case?

Could the issue be addressed by setting a restart policy? Or by either of the examples I included above?

@AndersKOlsson
Copy link

I agree with @cgeoffroy. It's unfortunate not to have the possibility to have the pre 1.5 behavior.

Our use case is that we use this in testing, and when a container goes down, it's probably because of a bug. So, automatic restarts are not wanted. Rather, a clear indication that there has been a problem is of high value. The compose session ending is much clearer than that the other services will have to time out trying to reach the service that has gone down.

It's not the same case as @VAS had, since the tests are executing externally to the compose environment. We don't know which service could go down, they're all equally likely.

#2130 seems like it would be what we need.

@simonvanderveldt
Copy link

@dnephin I'd like to add our use case.
This basically broke our pre-push hooks as well as a part of our CI.
We used to simply run docker-compose up and if the main service failed the whole compose process exited with exit code 1 and no push would happen.
Now when we run compose up and the main service fails it keeps hanging indefinitely.

The workarounds mentioned introduce redundancy: I have to do docker-compose up -d <service1> etc for all services because I don't want to start the main application.
And the docker-compose stop command which has to be run after docker-compose run is problematic. For example a lot of tools immediately stop when a exit code 1 is encountered so the docker-compose stop never gets executed.

More generally speaking I think the use case of treating the combination of services defined with compose as 1 unit and thus exiting when one of the units fails is just as valid as the use case treating the services as loosely coupled and not exiting when one of the units fails.

TL;DR I'd like something like #2130 as well :)

@pandeiro
Copy link

pandeiro commented Dec 1, 2015

I've also filed #2474 because our integration testing workflow is no longer compatible with Compose as of 1.5.x.

This is the second consecutive "breakage" to our app introduced by Compose in subsequent minor versions (1.4.x stopped recreating containers by default (but provided a flag to re-enable the previous default behavior)).

Is there, or could there be, a process for gathering user feedback/requirements that could be used to evaluate the cost/benefits of these breaking changes (and better prepare users for them)?

@dnephin
Copy link

dnephin commented Dec 1, 2015

User feedback is really import to us. Both of these changes you mentioned were a result of lots of user feedback, but everyone uses Compose in a slightly different way, so it's hard to get it just right for everyone.

The process we use is generally github issues and release candidates. We use milestones to indicate which issues are going to be in an upcoming release (we've started to put issues into a milestone at the beginning of the cycle).

Releases candidates are also created many weeks in advance so users can try out the new changes.

We also try to make sure that there is always a way to get the old behaviour. With the first change it was using --force-recreate, and with the second there is the suggested approach of using run, which is designed to exit.

I think it's also important to try and document recommended uses of compose. For example http://docs.docker.com/compose/#automated-testing-environments uses up -d to create the environment, and docker run can be used to run the tests. Does that not work for your setup? I'd be great to learn more about why it doesn't.

@pandeiro
Copy link

pandeiro commented Dec 1, 2015

Thanks for the detailed response @dnephin. I appreciate the thought and effort that's gone into the process.

@Tankanow
Copy link

Tankanow commented Dec 3, 2015

Hi @dnephin,

Using run allows you to get the exit status from the test run, and you only see the output of the tests (not all the dependencies).

Is this documented anywhere? I didn't see it in the CLI help messages or on the run or up reference pages.

Thanks,

Adam

@dnephin
Copy link

dnephin commented Dec 3, 2015

It was recently added here: https://github.com/docker/compose/blob/master/docs/faq.md#whats-the-difference-between-up-run-and-start

Which will be published to the official docs pretty soon

@Tankanow
Copy link

Tankanow commented Dec 3, 2015

Great. Thanks @dnephin!

@j-baker
Copy link

j-baker commented Dec 9, 2015

@dnephin, we had I think the same use case as a number of other people here - a number of interacting apps which communicate with each other and a test runner. The specific thing that was nice before was that all our logs got interleaved, so we could broadly see what was going on from just a jenkins build log - the test failure generally came shortly after a stack trace.

Do you have any recommendations as to how we can reobtain the previous behaviour?

Our script currently looks like:

docker-compose rm -f
docker-compose build
docker-compose up -t 0
exit $(docker inspect -f '{{ .State.ExitCode }}' testrunner_container_name)

@dnephin
Copy link

dnephin commented Dec 9, 2015

I think maybe this:

set -e

function on_exit() {
    docker-compose stop -t 1
}

docker-compose rm -f
docker-compose build
docker-compose up -t 0 --no-build -d 
trap "on_exit" EXIT
docker-compose logs &
docker-compose run --rm testrunner test_command

--no-build just skips a few API calls since you already explicitly build first.

Having to background logs isn't great. I think we'll be able to fix that with #2227 by providing a way for logs to exit instead of always follow.

The other change you'd need to make would be to set a no-op command in the docker-compose.yml for the test runner, so that tests don't run as part of up. An alternative would be to remove the test runner service from docker-compose.yml entirely, and put it into a docker-compose.test.yml, and use this run in place of the line above:

docker-compose run -f docker-compose.yml -f docker-compose.test.yml run --rm testrunner

I personally like that approach because it lets you use your docker-compose.yml for dev environments without the testrunner service being run.

We can also continue the discussion in #2474 about adding a flag to restore the "shutdown on first container exit" behaviour.

@simonvanderveldt
Copy link

@dnephin What's the point of having an abstraction layer like Compose when I have to add another abstraction layer like a shell script to manage Compose to do simple stuff like this? The simplicity of Compose is totally lost this way.
All of the suggestions seem to be workarounds, whereas it worked elegantly for our usecases before 1.5.

If we say we have to move to run instead of up for processes for which we need the exit code we should make sure that run has the necessary features to support those usecases.
That would included dependency resolution (even when links get dropped) and being able to stop all containers started by compose when the started service exits.

@dnephin
Copy link

dnephin commented Dec 11, 2015

@simonvanderveldt I don't really see compose or shell scripts as abstractions, they are just tools to accomplish a specific task. They don't change or hide the underlying concepts or interfaces.

Compose is, as it states in the description, "a tool for defining and running multi-container Docker applications". It is not a scripting language (like bash). Could you accomplish what Compose does in any programming languages? Of course.

All of the suggestions seem to be workarounds, whereas it worked elegantly for our usecases before 1.5

The behavior before 1.5 was effectively a bug. It never worked with data volume containers, and it didn't give you an appropriate exit status code, so it was never intended to work that way.

docker-compose run exists for this purpose.

we should make sure that run has the necessary features to support those usecases.

A single command is never going to encapsulate every use case. Compose gets used in many different ways. I don't see having to write multiple commands as a problem. However trying to reduce the number of commands necessary for common use-cases is a goal.

The example above does require a few extra commands. We'll be reducing this soon with: #12, #2227, #2277. We can also discuss #2474 and #2490 as options.

@arigesher
Copy link

Option 1

I would recommend changing your test container to have a default no-op command (something like echo, or true), and doing:

docker-compose rm -f
docker-compose build
docker-compose up --timeout 1 --no-build -d
docker-compose run tests test_command...
docker-compose stop

Using run allows you to get the exit status from the test run, and you only see the output of the tests (not all the dependencies).

For anyone banging their head against the desk trying to get this to work four years later, the semantics appear to have changed.

Use a compose file like this:

version: "3.7"
services:
  service1:
    image: service
    ports:
      - "10001"
  service2:
    image: service
    ports:
      - "10001"
  service3:
    image: service
    ports:
      - "10001"
  tests:
    image: tests
    container_name: tests
    entrypoint: /path/to/test/script

...

Then invoke the tests using compose like this:

docker-compose up --exit-code tests --abort-on-container-exit

That will:

  1. show you the output from the tests container (but also the other service containers, too)
  2. return the exit code of the tests entrypoint
  3. tear down the other services when the test completes

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

Successfully merging this pull request may close these issues.

None yet