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

Fix daemon not cleaned up w/ live restore enabled #24435

Merged

Conversation

cpuguy83
Copy link
Member

@cpuguy83 cpuguy83 commented Jul 8, 2016

This patch makes sure daemon resources are cleaned up on shutdown if
there are no running containers.

Fixes #24350

@cpuguy83 cpuguy83 added this to the 1.12.0 milestone Jul 8, 2016
This patch makes sure daemon resources are cleaned up on shutdown if
there are no running containers.

Signed-off-by: Brian Goff <cpuguy83@gmail.com>
@thaJeztah
Copy link
Member

@thaJeztah
Copy link
Member

LGTM

@@ -652,8 +652,12 @@ func (daemon *Daemon) Shutdown() error {
// Keep mounts and networking running on daemon shutdown if
// we are to keep containers running and restore them.
if daemon.configStore.LiveRestore {
return nil
// check if there are any running containers, if none we should do some cleanup
if ls, err := daemon.Containers(&types.ContainerListOptions{}); len(ls) != 0 || err != nil {
Copy link
Member

@runcom runcom Jul 8, 2016

Choose a reason for hiding this comment

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

isn't less expensive and probably cleaner to call daemon.containers.List() (or daemon.List()) then daemon.Containers(...)? I can see much stuff going on with daemon.Containers(...) - like filters even if you're providing an empty list option

Copy link
Member Author

Choose a reason for hiding this comment

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

daemon.containers.List will have to be manually filtered anyway.

@runcom
Copy link
Member

runcom commented Jul 8, 2016

just a question, LGTM otherwise

@anusha-ragunathan
Copy link
Contributor

If there is a mix of running and stopped containers during daemon shutdown, dont we still want the stopped container mounts to be unmounted and other cleanup performed?

@cpuguy83
Copy link
Member Author

cpuguy83 commented Jul 8, 2016

@anusha-ragunathan The stopped containers will already be unmounted.
The issue you saw was with the root grpahdriver mount.

@anusha-ragunathan
Copy link
Contributor

anusha-ragunathan commented Jul 8, 2016

Yes, the graph driver mount is what I'm referring to. daemon.layerStore.Cleanup and daemon.cleanupMounts to be specific.

@cpuguy83
Copy link
Member Author

cpuguy83 commented Jul 8, 2016

@anusha-ragunathan If there are running containers, we can't unmount the root graph mount.
All the other mounts are already gone.

@anusha-ragunathan
Copy link
Contributor

layerStore.Cleanup calls the graphdriver's Cleanup method, which is:

  • an extra call for any leftover container unmounts. Container unmounts already happen as part of daemon.Cleanup
  • the main call for the root graphdriver mount.
    Thanks for the clarification, @cpuguy83

LGTM.

@tonistiigi
Copy link
Member

LGTM

@tonistiigi tonistiigi merged commit a34534f into moby:master Jul 8, 2016
@cpuguy83 cpuguy83 deleted the 24350_cleanup_on_no_running_containers branch July 8, 2016 17:24
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

7 participants