[EricHripko]

Overview

This PR updates docker-compose run to wait for condition: service_healthy dependencies to become healthy. This effectively reflects the behaviour that docker-compose up has.
Resolves #7453.

Implementation Summary

• Project.up now takes care of creating the one-off container:
  • This is powered by new one_off ConvergencePlan
  • One-off containers also accept option overrides
• Project.up doesn't start the one-off container, this is still responsibility of run_one_off_container

Pre-commit hooks

According to script/test/default pre-commit hooks pass:

Check for added large files..............................................Passed
Check docstring is first.................................................Passed
Check for merge conflicts................................................Passed
Check Yaml...............................................................Passed
Check JSON...............................................................Passed
Debug Statements (Python)................................................Passed
Fix End of Files.........................................................Passed
Flake8...................................................................Passed
Tests should end in _test.py.............................................Passed
Fix requirements.txt.....................................................Passed
Trim Trailing Whitespace.................................................Passed
Reorder python imports...................................................Passed

Other Changes

• service.py and main.py have 2 additional changes I had to apply in order to get pre-commit hooks (namely, flake8) to pass (rebased to pick this up from main branch)
• Gotten rid of force parameter in remove_container since it doesn't actually get used

User Experience

An extra Creating <project>_<service>_run ... done will be printed to the terminal in addition to dependency information that's already printed. Examples of happy paths:

$ docker-compose run web
Starting docker-compose-example_redis_1 ... done
Creating docker-compose-example_web_run ... done
 * Debugger is active!
 * Debugger PIN: 302-617-874

$ docker-compose run test bash
Starting docker-compose-example_db_1 ... done
Creating docker-compose-example_test_run ... done
root@008cf9d35de0:/# ls
bin   dev                  docker-entrypoint.sh  home  lib64  mnt  proc  run   srv  tmp  var
boot  docker-entrypoint.d  etc                   lib   media  opt  root  sbin  sys  usr

We're also now getting up checks for free. For example, if a dependency is unhealthy and condition: service_healthy was specified, the following will happen:

$ docker-compose run web bash
Starting docker-compose-example_redis_1 ... done

ERROR: for web  Container "830730e0db0c" is unhealthy.
ERROR: Encountered errors while bringing up the project.

Testing

• An acceptance test was added; it passes
• Tested it manually to verify that docker-compose run indeed works as expected

[EricHripko]

From user perspective, this is how this addition feels:

$ docker-compose run --rm mydfesvc
Starting myproj_mydb_1    ... done
ERROR: for mydb  Container "dfd548facba4" is unhealthy.
$

[EricHripko]

I am not a huge fan of doing a sleep - so would appreciate any suggestions you have here 🙂

[EricHripko]

Rebased PR on latest master to pick up the flake8 fixes.

[gitpushdashf]

Could someone review this? Thank you!

[EricHripko]

Hey @rumpl @ndeloof @ulyssessouza - do you have any feedback on this?

[EricHripko]

Hey @rumpl @ndeloof @ulyssessouza - could you take a look at this PR when you got a moment please?

[EricHripko]

Now that Compose Spec has been landed, it make sense to merge this PR since it ensures the docker-compose is compliant with the spec.

Hey @rumpl @ndeloof @ulyssessouza, could you take a look?

[aiordache]

Hi @EricHripko I can have a look at this but can you rebase it first please?

[EricHripko]

@aiordache thanks for working with me on this 👍 I've rebased the PR and fixed the tests - looks all green and ready to review 🙂

[aiordache]

Thank you @EricHripko! I had a look and tested the workflow and it works pretty well.

I was thinking though why don't we let the project.up do the complete check(including the one_off container) as it is anyway being called before the new method you added. That would require to change the way we create the one_off container which for now doesn't seem easy to follow.
What do you think about it?

[EricHripko]

Thank you for taking a look @aiordache 👍

I've re-implemented this using project.up as suggested. In order to make this possible, I've added one_off convergence strategy and override_options to customise the one-off container. This is what it looks like from user perspective:

# If dependency is unhealthy: now the same as up
$ docker-compose run web bash
Starting docker-compose-example_redis_1 ... done

ERROR: for web  Container "830730e0db0c" is unhealthy.
ERROR: Encountered errors while bringing up the project.

# If dependency is healthy: now follows new workflow more similar to up
$ docker-compose run web
Starting docker-compose-example_redis_1 ... done
Creating docker-compose-example_web_run ... done
 * Debugger is active!
 * Debugger PIN: 302-617-874

I still need to fix up a few tests, but would love to hear if I'm heading in the right direction 🙂

[aiordache]

I tested the new changes and a small thing to fix is, on a successful run command, we need to press enter to get the shell prompt of the container.
Apart from this, it works great. Thank you @EricHripko! I like having the same workflow as up.

[EricHripko]

Thanks for your feedback @aiordache 👍 That's interesting - I've not noticed this issue despite testing natively on Windows and inside WSL2. Will try on Mac or a Linux VM to see if I can reproduce.

As an aside, I've just updated CLITestCase::test_run_handles_sig* to swap top for sleep infinity. I assume that's fine since it looks like top is just used as a long-running process. The reason for this was that top failed the tests for some reason after my changes (and I couldn't reproduce the failures). This might be related to the issue you mentioned - so will investigate more and revert the change if that's the culprit.

[EricHripko]

@aiordache, I've had a go at Mac as well and I am unable to reproduce this. docker-compose run with these changes drops me straight into the interactive shell - similar to what previous version (1.25.5 on my Mac) does. What platform are you on? Is there a specific project I could try to reproduce this with?

[aiordache]

@EricHripko I tested it on two different machines running Arch linux (bash shell).
The compose file I used:

services:
  db:
    image: redis
    healthcheck:
      test: "exit 0"
  test:
    image: nginx
    depends_on:
      db:
        condition: service_healthy

[EricHripko]

Thank you for sharing this @aiordache 👍 I've tried this on Ubuntu (in bash) and had the following results:

$ docker-compose run test
Creating docker-compose-example_db_1 ... done
Creating docker-compose-example_test_run ... done

^C
$ docker-compose run test bash
Starting docker-compose-example_db_1 ... done
Creating docker-compose-example_test_run ... done
root@008cf9d35de0:/# ls
bin   dev                  docker-entrypoint.sh  home  lib64  mnt  proc  run   srv  tmp  var
boot  docker-entrypoint.d  etc                   lib   media  opt  root  sbin  sys  usr
root@008cf9d35de0:/# exit

Will install Arch to further eliminate the affecting factors. Could you please share the command that exhibits the bug too?

[aiordache]

@EricHripko I tested this command:

$ docker-compose run --rm test sh

I tried now the bash cmd and it works fine, but with sh I need to press enter. Sorry for the confusion.

[EricHripko]

Ah - great! I can indeed confirm that I see the same issue with sh (even on Ubuntu). Will dig in to see why this happens 👍

[thaJeztah]

Curious; how does this interact with --no-deps ? Does it produce an error in that case, or just skips the checks?

[EricHripko]

@thaJeztah, it'll skip the checks if --no-deps is specified 🙂

[EricHripko]

@aiordache, found the root cause for the issue. It appears that my changes were trying to "start" the container twice. This caused weirdness in some shells. I've simplified the code now and will push once tests look good.

[EricHripko]

@aiordache this should be all green and ready for review 🙌 I've also updated the PR Overview to include the latest information about how it was implemented and what UX is like 🙂

[aiordache]

Thank you @EricHripko for the effort to get this done. I have tested the latest changes and it works great! If you check my last comment and stash the commits, I could merge this tomorrow unless anybody else sees an issue with it.

[aiordache]

LGTM!

[EricHripko]

Awesome 🎉 Thank you for your review and mentoring throughout this PR - really appreciate it 🙂 I've addressed your feedback and squashed the PR down to a single commit.

[rumpl]

LGTM! 👍