Don't create source directory while the daemon is being shutdown, fix #30348 #33330

coolljt0725 · 2017-05-22T08:54:41Z

If a container mount the socket the daemon is listening on into
container while the daemon is being shutdown, the socket will
not exist on the host, then daemon will assume it's a directory
and create it on the host, this will cause the daemon can't start
next time.

fix issue #30348

To reproduce this issue, you can add following code

--- a/daemon/oci_linux.go
+++ b/daemon/oci_linux.go
@@ -8,6 +8,7 @@ import (
        "sort"
        "strconv"
        "strings"
+       "time"

        "github.com/Sirupsen/logrus"
        "github.com/docker/docker/container"
@@ -666,7 +667,8 @@ func (daemon *Daemon) createSpec(c *container.Container) (*libcontainerd.Spec, e
        if err := daemon.setupIpcDirs(c); err != nil {
                return nil, err
        }
-
+       fmt.Printf("===please stop the daemon===\n")
+       time.Sleep(time.Second * 2)
        ms, err := daemon.setupMounts(c)
        if err != nil {
                return nil, err

step1 run a container which has --restart always and -v /var/run/docker.sock:/sock

$ docker run -ti --restart always -v /var/run/docker.sock:/sock busybox
/ #

step2 exit the the container

/ # exit

and kill the daemon when you see

===please stop the daemon===

in the daemon log

The daemon can't restart again and fail with can't create unix socket /var/run/docker.sock: is a directory.

Signed-off-by: Lei Jitang leijitang@huawei.com

ping @cpuguy83 @thaJeztah

- What I did

- How I did it

- How to verify it

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

thaJeztah · 2017-05-22T12:39:30Z

@coolljt0725 looks like build is failing;

11:26:03 # github.com/docker/docker/daemon
11:26:03 daemon/volumes_windows.go:27: not enough arguments in call to mount.Setup
11:26:03 	have (string, number, number)
11:26:03 	want (string, int, int, func() error)

coolljt0725 · 2017-05-23T05:44:57Z

@thaJeztah updated

thaJeztah · 2017-05-30T10:07:47Z

ping @cpuguy83 @mlaventure PTAL

mlaventure

A few nits and a requested change to checkFun prototype.

Otherwise, LGTM. Thanks for finding this!

mlaventure · 2017-05-30T14:42:42Z

daemon/volumes_unix.go

@@ -42,8 +43,21 @@ func (daemon *Daemon) setupMounts(c *container.Container) ([]container.Mount, er
 		if err := daemon.lazyInitializeVolume(c.ID, m); err != nil {
 			return nil, err
 		}
+		// if the daemon is being shutdown, we could not let the container


nit:

If the daemon is being shutdown, we should not let a container start if it is trying to mount the socket the daemon is listening on. During daemon shutdown, the socket (/var/run/docker.sock by default) doesn't exist anymore causing the call to m.Setup to create at directory instead. This in turn will prevent the daemon to restart.

mlaventure · 2017-05-30T14:45:55Z

daemon/volumes_unix.go

+		// the socket (`/var/run/docker.sock` by default) does not exist and
+		// the m.Setup will assume it's a directory and create it, this will
+		// cause the daemon can't start next time.
+		checkfunc := func() error {


I think I'd prefer the callback to be of the form func(m *Mountpoint) error

mlaventure · 2017-05-30T14:46:55Z

volume/volume.go

@@ -146,7 +146,9 @@ func (m *MountPoint) Cleanup() error {

 // Setup sets up a mount point by either mounting the volume if it is
 // configured, or creating the source directory if supplied.
-func (m *MountPoint) Setup(mountLabel string, rootUID, rootGID int) (path string, err error) {
+// The checkFun is to do some checking before creating the


nit:

The, optional, checkFun parameter allows doing additional checking before creating the source directory on the host.

mlaventure · 2017-05-30T14:49:26Z

volume/volume.go

@@ -181,6 +183,14 @@ func (m *MountPoint) Setup(mountLabel string, rootUID, rootGID int) (path string

 	// system.MkdirAll() produces an error if m.Source exists and is a file (not a directory),
 	if m.Type == mounttypes.TypeBind {
+		// Before creating source directory on the host do the checking if it's not nil


nit:

Before creating the source directory on the host, invoke checkFun if it's not nil. One of the use case is to forbid creating the daemon socket as a directory if the daemon is in the process of shutting down.

coolljt0725 · 2017-05-31T02:50:51Z

@mlaventure update

…oby#30348 If a container mount the socket the daemon is listening on into container while the daemon is being shutdown, the socket will not exist on the host, then daemon will assume it's a directory and create it on the host, this will cause the daemon can't start next time. fix issue moby#30348 To reproduce this issue, you can add following code ``` --- a/daemon/oci_linux.go +++ b/daemon/oci_linux.go @@ -8,6 +8,7 @@ import ( "sort" "strconv" "strings" + "time" "github.com/Sirupsen/logrus" "github.com/docker/docker/container" @@ -666,7 +667,8 @@ func (daemon *Daemon) createSpec(c *container.Container) (*libcontainerd.Spec, e if err := daemon.setupIpcDirs(c); err != nil { return nil, err } - + fmt.Printf("===please stop the daemon===\n") + time.Sleep(time.Second * 2) ms, err := daemon.setupMounts(c) if err != nil { return nil, err ``` step1 run a container which has `--restart always` and `-v /var/run/docker.sock:/sock` ``` $ docker run -ti --restart always -v /var/run/docker.sock:/sock busybox / # ``` step2 exit the the container ``` / # exit ``` and kill the daemon when you see ``` ===please stop the daemon=== ``` in the daemon log The daemon can't restart again and fail with `can't create unix socket /var/run/docker.sock: is a directory`. Signed-off-by: Lei Jitang <leijitang@huawei.com>

mlaventure

LGTM

Thanks!

AkihiroSuda · 2017-06-07T03:27:20Z

daemon/daemon.go

+// StoreHosts stores the addresses the daemon is listening on
+func (daemon *Daemon) StoreHosts(hosts []string) {
+	if daemon.hosts == nil {
+		daemon.hosts = make(map[string]bool)


Can we initialize daemon.hosts in NewDaemon()?

I have considered that, but it will have some duplicate code to dealing with the hosts config (https://github.com/moby/moby/blob/master/cmd/dockerd/daemon.go#L157).

AkihiroSuda

LGTM

GordonTheTurtle added the status/0-triage label May 22, 2017

coolljt0725 added status/1-design-review and removed status/0-triage labels May 22, 2017

coolljt0725 force-pushed the fix_sock_is_dir branch from 83328f5 to 806c5e8 Compare May 23, 2017 01:13

thaJeztah added the rebuild/z label May 30, 2017

GordonTheTurtle removed the rebuild/z label May 30, 2017

mlaventure suggested changes May 30, 2017

View reviewed changes

coolljt0725 force-pushed the fix_sock_is_dir branch from 806c5e8 to 7318eba Compare May 31, 2017 02:42

dhiltgen mentioned this pull request May 31, 2017

17.06.0 RC2 tracker docker/for-linux#2

Closed

23 tasks

thaJeztah added status/2-code-review and removed status/1-design-review labels May 31, 2017

mlaventure approved these changes May 31, 2017

View reviewed changes

mlaventure added the rebuild/z label May 31, 2017

GordonTheTurtle removed the rebuild/z label May 31, 2017

GordonTheTurtle assigned AkihiroSuda Jun 6, 2017

andrewhsu mentioned this pull request Jun 6, 2017

17.06.0 RC3 tracker docker/for-linux#8

Closed

40 tasks

AkihiroSuda reviewed Jun 7, 2017

View reviewed changes

AkihiroSuda approved these changes Jun 7, 2017

View reviewed changes

AkihiroSuda merged commit cd2255a into moby:master Jun 7, 2017

GordonTheTurtle added this to the 17.06.0 milestone Jun 7, 2017

coolljt0725 deleted the fix_sock_is_dir branch June 7, 2017 03:38

thaJeztah mentioned this pull request Jun 7, 2017

can't create unix socket /var/run/docker.sock: is a directory #30348

Open

seemethere mentioned this pull request Jun 9, 2017

Don't create source directory while the daemon is being shutdown, fix… docker/docker-ce#48

Merged

thaJeztah added the process/cherry-picked label Jun 10, 2017

thaJeztah mentioned this pull request Jul 13, 2017

vndr github.com/docker/docker from components/engine docker/docker-ce#116

Closed

This was referenced Oct 11, 2018

Don't create source directory while the daemon is being shutdown projectatomic/docker#327

Merged

Don't create source directory while the daemon is being shutdown projectatomic/docker#328

Merged

Don't create source directory while the daemon is being shutdown, fix #30348 #33330

Don't create source directory while the daemon is being shutdown, fix #30348 #33330

coolljt0725 commented May 22, 2017

thaJeztah commented May 22, 2017

coolljt0725 commented May 23, 2017

thaJeztah commented May 30, 2017

mlaventure left a comment

mlaventure May 30, 2017

mlaventure May 30, 2017

mlaventure May 30, 2017

mlaventure May 30, 2017

coolljt0725 commented May 31, 2017

mlaventure left a comment

AkihiroSuda Jun 7, 2017

coolljt0725 Jun 7, 2017

AkihiroSuda left a comment

Don't create source directory while the daemon is being shutdown, fix #30348 #33330

Don't create source directory while the daemon is being shutdown, fix #30348 #33330

Conversation

coolljt0725 commented May 22, 2017

thaJeztah commented May 22, 2017

coolljt0725 commented May 23, 2017

thaJeztah commented May 30, 2017

mlaventure left a comment

Choose a reason for hiding this comment

mlaventure May 30, 2017

Choose a reason for hiding this comment

mlaventure May 30, 2017

Choose a reason for hiding this comment

mlaventure May 30, 2017

Choose a reason for hiding this comment

mlaventure May 30, 2017

Choose a reason for hiding this comment

coolljt0725 commented May 31, 2017

mlaventure left a comment

Choose a reason for hiding this comment

AkihiroSuda Jun 7, 2017

Choose a reason for hiding this comment

coolljt0725 Jun 7, 2017

Choose a reason for hiding this comment

AkihiroSuda left a comment

Choose a reason for hiding this comment