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

Added --follow-retry/-F for docker logs #7663

Closed
wants to merge 2 commits into from
Closed

Added --follow-retry/-F for docker logs #7663

wants to merge 2 commits into from

Conversation

LK4D4
Copy link
Contributor

@LK4D4 LK4D4 commented Aug 21, 2014

If container stops this commands retry to join it logs until it start again. When container is destroyed this command just exit.

@SvenDowideit
Copy link
Contributor

Docs LGTM - though the api change will need moving to 1.15 :)

@LK4D4
Copy link
Contributor Author

LK4D4 commented Aug 25, 2014

I'll wait until someone bump docs :)

@tiborvass
Copy link
Contributor

@LK4D4 I am not sure I like the name --follow-retry. Also, do you think it would be possible to have logs subscribe to the "start" event of the container instead of retrying?

@LK4D4
Copy link
Contributor Author

LK4D4 commented Aug 26, 2014

@tiborvass I get this name from combination of options in tail utility, if you have ideas - let me know.
I'm not sure what do you mean about "subscribe". Now code is simply recreate pipes for stdin/stdout, when container stops and begin wait info from this pipes. I think this is much simpler then wait for events.

@LK4D4
Copy link
Contributor Author

LK4D4 commented Aug 26, 2014

rebased

@tiborvass
Copy link
Contributor

@LK4D4 My bad, it's fine like this, since the reader is blocking anyways. I'm still trying to find a better name than --follow-retry. What about --follow-persist ?

@3onyc
Copy link

3onyc commented Aug 26, 2014

--follow-retry stems from tail having --follow and --retry so it makes sense from that aspect (and -F is the shortcut for --follow --retry).

@erikh
Copy link
Contributor

erikh commented Aug 26, 2014

LGTM

@tiborvass
Copy link
Contributor

@3onyc thanks!

@LK4D4
Copy link
Contributor Author

LK4D4 commented Aug 27, 2014

Moved docs to 1.15. @tiborvass If you okay with name now - feel free to merge.

@crosbymichael
Copy link
Contributor

Do we really need a flag for this specific functionality? Please be conservative when adding flags because they are almost impossible to take a way. We should only do something like this if it is an extreme inconvenience to a user or if it would be impossible for them to implement with tooling.

@LK4D4
Copy link
Contributor Author

LK4D4 commented Aug 27, 2014

@crosbymichael Actualy I don't know, but moving this functionality to just -f will be API breakage.
ping @fsouza @michaelshobbs

@michaelshobbs
Copy link

see the discussion from #7020

I think the point was to not have -f hang out until the container is removed but some folks still wanted that functionality...

@jessfraz
Copy link
Contributor

jessfraz commented Sep 2, 2014

what is the functionality on attach currently, because if we do this for logs we might want to mirror it for attach

@tiborvass
Copy link
Contributor

@jfrazelle attach has the same behavior as docker logs -f currently: when the container stops, the attach stops.

@tianon
Copy link
Member

tianon commented Sep 2, 2014

So maybe as a mirror, we could add "--retry" to attach? (mirroring
"--follow-retry" here)

@tiborvass
Copy link
Contributor

This will be blocked until @shykes approves introducing yet another flag.

I personally think this is useful especially with the introduction of restart policies: it makes total sense to me to retrieve the logs of a redis container that has a restart policy of always or on-failure.

@crosbymichael @LK4D4 @vieux maybe an alternative solution would be to change -f behavior without introducing a new flag, as following: logs -f yields on a manual stop, or when the container stops because of a restart based on a restart policy. For anything more complicated, outside tools (like init systems) can be used. That's the best compromise I can think of.

@LK4D4
Copy link
Contributor Author

LK4D4 commented Sep 3, 2014

@tiborvass I'm pretty sure that if we change -f we'll have issue about -f not exiting on container stops in 48 hours.
@tianon We can add --retry for attach but this will be totally different code. So I think we should do this in separate PR.

If container stops this commands retry to join it logs until it
start again. When container is destroyed this command just exit.

Fixes #7643

Signed-off-by: Alexandr Morozov <lk4d4math@gmail.com>
Signed-off-by: Alexandr Morozov <lk4d4math@gmail.com>
@shykes
Copy link
Contributor

shykes commented Nov 1, 2014

When you docker logs -f to a container, the natural behavior is to send all output of the container. That should include all processes in the container. In other words, we should just incorporate the change of behavior you suggest, but make it the default instead of adding a new flag. I believe it's what every user expects out of the box anyway.

Note: doing this will break users who rely on the specific (IMO incorrect) behavior of ending the stream when the process dies (instead of continuing the stream across any number of processes in the container). For example, maybe you are using docker logs -f to wait for an individual process to terminate. That would no longer work. I think that's OK since docker wait exists for that purpose.

@shykes shykes closed this Nov 1, 2014
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.

None yet