Fix issue where compose logs doesn't exit when all running containers have been stopped

[laurazard]

Signed-off-by: Laura Brehm laurabrehm@hey.com

Currently, the issue exists where, with a compose.yaml such as

services:
  foo:
    image: alpine
    command: sh -c 'while true; do date; sleep 1; done'
  bar:
    image: alpine
    command: sh -c 'while true; do date; sleep 1; done'

if one runs:

• compose create
• compose start foo
• compose logs -f

and in a separate tab run:

• compose stop foo

the logs hang, even though all containers have stopped. This does not align with Compose v1 behaviour, and leads to inconsistent behavior, such as:

If there is only one service defined in a Compose file:

services:
  foo:
    image: alpine
    command: sh -c 'while true; do date; sleep 1; done'

and then run:

• compose up -d
• compose logs -f
  and in a separate tab run:
• compose restart foo

the logs will exit, since only 1 container is expected (len(expected) == 1) to exist and the container exit during the restart.

However, if there are two services defined, even if one is stopped, running compose restart [running service container] does not cause the logs to exit, because even though the other container is stopped, len(expected) == 2, and when the container restarts, logs are still streamed.

(This also has to do with our current logic to detect whether a container will restart – inspecting the container after the die event and checking whether inspected.State.Restarting == true, which seems broken (always returns false, maybe due to race conditions between when we receive the die event and make the container inspect call). However, I have no idea how to check whether a container will restart other than waiting for a bit before closing the stream after a die event to see if we receive a restart event).

What I did

Adjust the expected calculation, to only account for currently running containers, to align with v1 behavior.

I played around with other ways of not exiting logs if a container is restarting instead of stopping, but I'll make another draft PR with that to get some discussion going.

Made the changes in pkg/compose/logs.go to filter containers passed to watch to only account for running containers (when using -f with compose logs), but keep the same logic elsewhere (like in compose start).

Related issue

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

[laurazard]

Ahh this solution has side-effects 😓 I'll look at this a bit more

[codecov]

Codecov Report

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

Coverage data is based on head (220626e) compared to base (e94eb05).
Patch has no changes to coverable lines.

Additional details and impacted files

@@           Coverage Diff           @@
##               v2   #10181   +/-   ##
=======================================
  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.

[ndeloof]

AFAIK the restart API does not set the Restarting status, which is only set by restart policy.

[glours]

LGTM