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

use containers we expect to start for wait condition #10209

Merged
merged 2 commits into from
Feb 7, 2023

Conversation

ndeloof
Copy link
Contributor

@ndeloof ndeloof commented Jan 26, 2023

as we check dependency service is running/healthy, use containers we created for the service so that we can detect failure to start, and not wait with no end for a dead service to become healthy.

Related issue
closes #10200

(not mandatory) A picture of a cute animal, if possible in relation to what you did
image

@ndeloof ndeloof force-pushed the wait_containers branch 2 times, most recently from 0834430 to d9c5a2f Compare January 26, 2023 16:57
@ndeloof ndeloof marked this pull request as draft January 26, 2023 16:58
@ndeloof ndeloof force-pushed the wait_containers branch 2 times, most recently from e44520e to 7778b20 Compare January 27, 2023 12:23
@rborn-tx
Copy link

rborn-tx commented Jan 28, 2023

Hello @ndeloof. I've run some tests on your branch and I've found some possible issues that I wanted to let you know. If I should instead report this as part of the original issue ticket, please let me know, okay?

Test 1

version: "2.4"
services:
  # Long-lived container.
  hello1:
    image: "alpine"
    command: ["sh", "-c", "echo 'Long-lived container, no health check'; sleep 1h;"]

  # Short-lived container.
  hello2:
    image: "alpine"
    command: ["echo", "Short-lived container, no health check"]

Execution:

$ compose --file docker-compose-mixed-nohc.yml up --detach --remove-orphans --wait
[+] Running 2/3
 ⠿ Network aktualizr-tests_default     Created                                                    0.0s
 ⠿ Container aktualizr-tests-hello2-1  Waiting                                                   15.8s
 ⠿ Container aktualizr-tests-hello1-1  Started                                                    0.4s
Killed
(compose is an alias to docker-compose)

Hitting ^C did not stop docker-compose which had to be killed.

Test 2

version: "2.4"
services:
  # Short-lived container.
  hello:
    image: "alpine"
    command: ["echo", "Short-lived container, no health check"]

Execution:

$ compose --file docker-compose-shortlife-nohc.yml up --detach --remove-orphans --wait
[+] Running 1/2
 ⠿ Network aktualizr-tests_default    Created                                                    0.0s
 ⠿ Container aktualizr-tests-hello-1  Waiting                                                    0.8s
container aktualizr-tests-hello-1 exited (0)

$ echo $?
1

Notice in this case the --wait didn't cause docker-compose to wait forever, but the exit code of the program was non-zero indicating something went wrong.

Test 3

# Run multiple short-lived containers.
version: "2.4"
services:
  hello1:
    image: "alpine"
    command: ["echo", "Short-lived container (1), no health check"]
  hello2:
    image: "alpine"
    command: ["echo", "Short-lived container (2), no health check"]
  hello3:
    image: "alpine"
    command: ["echo", "Short-lived container (3), no health check"]

Execution:

$ compose --file docker-compose-shortlife-nohc3.yml up --detach --remove-orphans --wait
[+] Running 3/4
 ⠿ Network aktualizr-tests_default     Created                                                          0.0s
 ⠿ Container aktualizr-tests-hello1-1  Waiting                                                         26.9s
 ⠿ Container aktualizr-tests-hello2-1  Started                                                          0.5s
 ⠿ Container aktualizr-tests-hello3-1  Started                                                          0.6s
Killed

Like with Test 1, hitting ^C did not stop docker-compose which had to be killed.

@ndeloof
Copy link
Contributor Author

ndeloof commented Jan 28, 2023

@rborn-tx thanks, nice feedback. I'll try to convert those examples into e2e tests so we have better coverage for this feature

@ndeloof ndeloof marked this pull request as ready for review February 6, 2023 13:49
@ndeloof
Copy link
Contributor Author

ndeloof commented Feb 6, 2023

I fixed reported corner cases.
For second one ("Test 2") service command immediately complete, --wait detect container exited and report failure to become "running". This sounds a reasonable behavior as service is expected to run until interrupted, not to complete this way (otherwise, --wait makes no sense)

Copy link
Contributor

@glours glours left a comment

Choose a reason for hiding this comment

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

Not sure, but we may have a potential side effect in the dependencies check loop

if err != nil {
return err
}
containers = containers.filter(isService(dep))
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we use a dedicated variable to manage filtered containers? I wondering if we won't loose potential dependencies by re-assigning containers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

indeed, nice catch - I've been lazy here

@codecov
Copy link

codecov bot commented Feb 7, 2023

Codecov Report

Base: 73.89% // Head: 73.89% // No change to project coverage 👍

Coverage data is based on head (b8b02d6) compared to base (e908f41).
Patch has no changes to coverable lines.

❗ Current head b8b02d6 differs from pull request most recent head 52478f0. Consider uploading reports for the commit 52478f0 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##               v2   #10209   +/-   ##
=======================================
  Coverage   73.89%   73.89%           
=======================================
  Files           2        2           
  Lines         272      272           
=======================================
  Hits          201      201           
  Misses         60       60           
  Partials       11       11           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

@glours glours left a comment

Choose a reason for hiding this comment

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

LGTM

Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
@kaos14
Copy link

kaos14 commented Feb 13, 2023

I fixed reported corner cases. For second one ("Test 2") service command immediately complete, --wait detect container exited and report failure to become "running". This sounds a reasonable behavior as service is expected to run until interrupted, not to complete this way (otherwise, --wait makes no sense)

I disagree. I have multiple services in my docker-compose.yml, that I do want compose to wait for. At the same time I have a service that does its job and exits with 0 code. This change broke my setup and I had to add artificial sleep at the end of the script. I think --wait should take into account the restart option and not error out in Test 2.

In other words: Test 1 should pass, but it now errors out with container test-hello2-1 exited (0)

@ndeloof ndeloof deleted the wait_containers branch February 13, 2023 18:14
@ndeloof
Copy link
Contributor Author

ndeloof commented Feb 13, 2023

@kaos14 we miss an explicit way to define "jobs" or "tasks", i.e. container that are EXPECTED to complete

@kaos14
Copy link

kaos14 commented Feb 13, 2023

@ndeloof What did you mean "I fixed reported corner cases."? None of them works in 2.16. Is this as expected?

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

Successfully merging this pull request may close these issues.

[BUG] --wait shouldn't have any effect when using a single service
4 participants