/moby

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

Data volumes #3

Closed
shykes opened this Issue Jan 22, 2013 · 2 comments

Comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@shykes @progrium
@shykes
Collaborator

shykes commented Jan 22, 2013

This is a placeholder for @jpetazzo's adamant request for data volumes :)

Jerome, I will let you make your case in your own words. I agree with a lot of it, although we'll have to be careful how we distill it into the simplest possible API, and I'm not sure how early we want to do it.
@progrium

This comment has been minimized.

Show comment
Hide comment
@progrium

progrium Jan 24, 2013

Contributor

I'd love to learn more about this because I think state/data is often one of the biggest challenges of any system and should really be treated special. If stateful (ie databases/datastores) support in containers is builtin too directly, it may create a false affordance.
Contributor

progrium commented Jan 24, 2013

I'd love to learn more about this because I think state/data is often one of the biggest challenges of any system and should really be treated special. If stateful (ie databases/datastores) support in containers is builtin too directly, it may create a false affordance.

@shykes shykes referenced this issue Mar 21, 2013

Closed

Persistent data volumes #111

@shykes

This comment has been minimized.

Show comment
Hide comment
@shykes

shykes Mar 26, 2013

Collaborator

More detailed proposal here: #111
Collaborator

shykes commented Mar 26, 2013

More detailed proposal here: #111

@shykes shykes closed this Mar 26, 2013

@brynary brynary referenced this issue Jul 30, 2013

Closed

Production-ready process monitoring #1311

@kstaken kstaken referenced this issue Aug 8, 2013

Closed

Move the MASQUERADE from Bridge creation to network init + prevent inter-container networking #1454

@ohardy ohardy referenced this issue Dec 31, 2013

Closed

EXT4-fs (dm-1): warning: maximal mount count reached, running e2fsck is recommended #3404

@chrisruffalo chrisruffalo referenced this issue Feb 12, 2014

Closed

running systemd inside docker arch container hangs or segfaults #3629

jakedt pushed a commit to DevTable/docker that referenced this issue Jun 5, 2014

@fermayo
Merge pull request #3 from tutumcloud/0.9.1-tutum2 
0.9.1 tutum2
d418230

@rvs rvs referenced this issue Jul 2, 2014

Closed

flatten images - merge multiple layers into a single one #332

@k0a1a k0a1a referenced this issue Jul 9, 2014

Closed

Error: Cannot start container: fork/exec /var/lib/docker/init/dockerinit-1.0.0: invalid argument #6944

goldmann pushed a commit to goldmann/docker that referenced this issue Jul 30, 2014

@lsm5
Merge pull request #3 from maxamillion/maxamillion/remove_sysctl_epel 
Remove unneeded sysctl entries, docker 0.8.0 handles this
21b3b55

@46labs 46labs referenced this issue Aug 19, 2014

Closed

Pipework Re-assigning IP to existing container kills host / Segfault #7627

jlhawn referenced this issue in jlhawn/docker Aug 29, 2014

@vbatts
Merge pull request #3 from jlhawn/vbatts-v2_hacking 
Retrofit of registry v2 client
7e1354d

@stevef1uk stevef1uk referenced this issue Feb 10, 2015

Closed

wifi dropping when docker starts on RPi #10678

@rhatdan rhatdan referenced this issue Mar 18, 2015

Closed

Add some SELinux info I've pieced together #11396

@diogomonica diogomonica referenced this issue Mar 30, 2015

Closed

Proposal: User namespace support for re-mapped root per daemon setting #11253

@thockin thockin referenced this issue Apr 3, 2015

Open

Proposal: pinning namespaces aka --net=container:name, --net=ns:name without container running #12035

@blackfader blackfader referenced this issue May 7, 2015

Closed

docker in docker consulting #13043

@Khazrak Khazrak referenced this issue Jun 17, 2015

Closed

Kernel Panic when mounting volumes on RHEL 6.5 using Docker 1.4.0, Docker 1.4.1, with latest device mapper #9856

@shafreeck shafreeck referenced this issue Jun 20, 2015

Closed

docker is blocked when starts to attach a container #14063

@jimonreal jimonreal referenced this issue Jul 9, 2015

Closed

Execution driver lxc problems #14499

@asridharan asridharan referenced this issue Jul 13, 2015

Closed

docker daemon fails silently due to incorrect iptables NAT kernel modules #14592

@alex-aizman alex-aizman referenced this issue Jul 24, 2015

Closed

Proposal: unified driver for union filesystems #14968

kolyshkin added a commit to kolyshkin/moby that referenced this issue Jul 30, 2015

@kolyshkin
Simplify and fix os.MkdirAll() usage 
TL;DR: check for IsExist(err) after a failed MkdirAll() is both
redundant and wrong -- so two reasons to remove it.

Quoting MkdirAll documentation:

> MkdirAll creates a directory named path, along with any necessary
> parents, and returns nil, or else returns an error. If path
> is already a directory, MkdirAll does nothing and returns nil.

This means two things:

1. If a directory to be created already exists, no error is returned.

2. If the error returned is IsExist (EEXIST), it means there exists
a non-directory with the same name as MkdirAll need to use for
directory. Example: we want to MkdirAll("a/b"), but file "a"
(or "a/b") already exists, so MkdirAll fails.

The above is a theory, based on quoted documentation and my UNIX
knowledge.

3. In practice, though, current MkdirAll implementation [1] returns
ENOTDIR in most of cases described in #2, with the exception when
there is a race between MkdirAll and someone else creating the
last component of MkdirAll argument as a file. In this very case
MkdirAll() will indeed return EEXIST.

Because of #1, IsExist check after MkdirAll is not needed.

Because of #2 and #3, ignoring IsExist error is just plain wrong,
as directory we require is not created. It's cleaner to report
the error now.

Note this error is all over the tree, I guess due to copy-paste,
or trying to follow the same usage pattern as for Mkdir(),
or some not quite correct examples on the Internet.

[1] https://github.com/golang/go/blob/f9ed2f75/src/os/path.go

Signed-off-by: Kir Kolyshkin <kir@openvz.org>
8d4f275

kolyshkin added a commit to kolyshkin/moby that referenced this issue Jul 30, 2015

@kolyshkin
Simplify and fix os.MkdirAll() usage 
TL;DR: check for IsExist(err) after a failed MkdirAll() is both
redundant and wrong -- so two reasons to remove it.

Quoting MkdirAll documentation:

> MkdirAll creates a directory named path, along with any necessary
> parents, and returns nil, or else returns an error. If path
> is already a directory, MkdirAll does nothing and returns nil.

This means two things:

1. If a directory to be created already exists, no error is returned.

2. If the error returned is IsExist (EEXIST), it means there exists
a non-directory with the same name as MkdirAll need to use for
directory. Example: we want to MkdirAll("a/b"), but file "a"
(or "a/b") already exists, so MkdirAll fails.

The above is a theory, based on quoted documentation and my UNIX
knowledge.

3. In practice, though, current MkdirAll implementation [1] returns
ENOTDIR in most of cases described in #2, with the exception when
there is a race between MkdirAll and someone else creating the
last component of MkdirAll argument as a file. In this very case
MkdirAll() will indeed return EEXIST.

Because of #1, IsExist check after MkdirAll is not needed.

Because of #2 and #3, ignoring IsExist error is just plain wrong,
as directory we require is not created. It's cleaner to report
the error now.

Note this error is all over the tree, I guess due to copy-paste,
or trying to follow the same usage pattern as for Mkdir(),
or some not quite correct examples on the Internet.

[v2: a separate aufs commit is merged into this one]

[1] https://github.com/golang/go/blob/f9ed2f75/src/os/path.go

Signed-off-by: Kir Kolyshkin <kir@openvz.org>
a83a769

duglin added a commit to duglin/docker that referenced this issue Jul 31, 2015

@kolyshkin @duglin
Simplify and fix os.MkdirAll() usage 
TL;DR: check for IsExist(err) after a failed MkdirAll() is both
redundant and wrong -- so two reasons to remove it.

Quoting MkdirAll documentation:

> MkdirAll creates a directory named path, along with any necessary
> parents, and returns nil, or else returns an error. If path
> is already a directory, MkdirAll does nothing and returns nil.

This means two things:

1. If a directory to be created already exists, no error is returned.

2. If the error returned is IsExist (EEXIST), it means there exists
a non-directory with the same name as MkdirAll need to use for
directory. Example: we want to MkdirAll("a/b"), but file "a"
(or "a/b") already exists, so MkdirAll fails.

The above is a theory, based on quoted documentation and my UNIX
knowledge.

3. In practice, though, current MkdirAll implementation [1] returns
ENOTDIR in most of cases described in #2, with the exception when
there is a race between MkdirAll and someone else creating the
last component of MkdirAll argument as a file. In this very case
MkdirAll() will indeed return EEXIST.

Because of #1, IsExist check after MkdirAll is not needed.

Because of #2 and #3, ignoring IsExist error is just plain wrong,
as directory we require is not created. It's cleaner to report
the error now.

Note this error is all over the tree, I guess due to copy-paste,
or trying to follow the same usage pattern as for Mkdir(),
or some not quite correct examples on the Internet.

[v2: a separate aufs commit is merged into this one]

[1] https://github.com/golang/go/blob/f9ed2f75/src/os/path.go

Signed-off-by: Kir Kolyshkin <kir@openvz.org>
6a3f4b9

kolyshkin added a commit to kolyshkin/moby that referenced this issue Aug 6, 2015

@kolyshkin
make binary: do not ignore unresolved symbols 
TL;DR: stop building static binary that may fail

Linker flag --unresolved-symbols=ignore-in-shared-libs was added
in commit 06d0843 two years ago for the static build case, presumably
to avoid dealing with problem of missing libraries.

For the record, this is what ld(1) man page says:

> --unresolved-symbols=method
>    Determine how to handle unresolved symbols.  There are four
>    possible values for method:
> .........
>    ignore-in-shared-libs
>        Report unresolved symbols that come from regular object files,
>        but ignore them if they come from shared libraries.  This can
>        be useful when creating a dynamic binary and it is known that
>        all the shared libraries that it should be referencing are
>        included on the linker's command line.

Here, the flag is not used for its purpose ("creating a dynamic binary")
and does more harm than good. Instead of complaining about missing symbols
as it should do if some libraries are missing from LIBS/LDFLAGS, it lets
ld create a binary with unresolved symbols, ike this:

 $ readelf -s bundles/1.7.1/binary/docker-1.7.1 | grep -w UND
 ........
 21029: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT  UND dlopen
 .........

Such binary is working just fine -- until code calls one of those
functions, then it crashes (for apparently no reason, i.e. it is
impossible to tell why from the diagnistics printed).

In other words, adding this flag allows to build a static binary
with missing libraries, hiding the problem from both a developer
(who forgot to add a library to #cgo: LDFLAGS -- I was one such
developer a few days ago when I was working on ploop graphdriver)
and from a user (who expects the binary to work without crashing,
and it does that until the code calls a function in one of those
libraries).

Removing the flag immediately unveils the problem (as it should):

	/usr/lib/gcc/x86_64-linux-gnu/4.8/../../../x86_64-linux-gnu/libsqlite3.a(sqlite3.o):
	In function `unixDlError':
	(.text+0x20971): undefined reference to `dlerror'
	/usr/lib/gcc/x86_64-linux-gnu/4.8/../../../x86_64-linux-gnu/libsqlite3.a(sqlite3.o):
	In function `unixDlClose':
	(.text+0x8814): undefined reference to `dlclose'

The problem is, gosqlite package says:

	#cgo LDFLAGS: -lsqlite3

which is enough for dynamic linking, as indirect dependencies (i.e.
libraries required by libsqlite3.so) are listed in .so file and will be
resolved dynamically by ldd upon executing the binary.

For static linking though, one has to list all the required libraries,
both direct and indirect. For libraries with pkgconfig support the
list of required libraries can be obtained with pkg-config:

	$ pkg-config --libs sqlite3 # dynamic linking case
	-lsqlite3
	$ pkg-config --libs --static sqlite3 # static case
	-lsqlite3 -ldl -lpthread

It seems that all one has to do is to fix gosqlite this way:

	-#cgo LDFLAGS: -lsqlite3
	+#cgo pkg-config: sqlite3

Unfortunately, cmd/go doesn't know that it needs to pass --static
flag to pkg-config in case of static linking
(see golang/go#12058).

So, for one, one has to do one of these things:

1. Patch sqlite.go like this:

	-#cgo LDFLAGS: -lsqlite3
	+#cgo pkg-config: --static sqlite3

(this is exactly what I do in goploop, see
kolyshkin/goploop@e9aa072)

2. Patch sqlite.go like this:
	-#cgo LDFLAGS: -lsqlite3
	+#cgo LDFLAGS: -lsqlite3 -ldl -lpthread

(I would submit this patch to gosqlite but it seems that
https://code.google.com/p/gosqlite/ is deserted and not maintained,
and patching it here is not right as it is "vendored")

3. Explicitly add -ldl for the static link case.
This is what this patch does.

4. Fork sqlite to github and maintain it there. Personally I am not
ready for that, as I'm neither a Go expert nor gosqlite user.

Now, #3 doesn't look like a clear solution, but nevertheless it makes
the build much better than it was before.

Signed-off-by: Kir Kolyshkin <kir@openvz.org>
9ef4705

kolyshkin added a commit to kolyshkin/moby that referenced this issue Aug 7, 2015

@kolyshkin
make binary: do not ignore unresolved symbols 
TL;DR: stop building static binary that may fail

Linker flag --unresolved-symbols=ignore-in-shared-libs was added
in commit 06d0843 two years ago for the static build case, presumably
to avoid dealing with problem of missing libraries.

For the record, this is what ld(1) man page says:

> --unresolved-symbols=method
>    Determine how to handle unresolved symbols.  There are four
>    possible values for method:
> .........
>    ignore-in-shared-libs
>        Report unresolved symbols that come from regular object files,
>        but ignore them if they come from shared libraries.  This can
>        be useful when creating a dynamic binary and it is known that
>        all the shared libraries that it should be referencing are
>        included on the linker's command line.

Here, the flag is not used for its purpose ("creating a dynamic binary")
and does more harm than good. Instead of complaining about missing symbols
as it should do if some libraries are missing from LIBS/LDFLAGS, it lets
ld create a binary with unresolved symbols, ike this:

 $ readelf -s bundles/1.7.1/binary/docker-1.7.1 | grep -w UND
 ........
 21029: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT  UND dlopen
 .........

Such binary is working just fine -- until code calls one of those
functions, then it crashes (for apparently no reason, i.e. it is
impossible to tell why from the diagnistics printed).

In other words, adding this flag allows to build a static binary
with missing libraries, hiding the problem from both a developer
(who forgot to add a library to #cgo: LDFLAGS -- I was one such
developer a few days ago when I was working on ploop graphdriver)
and from a user (who expects the binary to work without crashing,
and it does that until the code calls a function in one of those
libraries).

Removing the flag immediately unveils the problem (as it should):

	/usr/lib/gcc/x86_64-linux-gnu/4.8/../../../x86_64-linux-gnu/libsqlite3.a(sqlite3.o):
	In function `unixDlError':
	(.text+0x20971): undefined reference to `dlerror'
	/usr/lib/gcc/x86_64-linux-gnu/4.8/../../../x86_64-linux-gnu/libsqlite3.a(sqlite3.o):
	In function `unixDlClose':
	(.text+0x8814): undefined reference to `dlclose'

The problem is, gosqlite package says:

	#cgo LDFLAGS: -lsqlite3

which is enough for dynamic linking, as indirect dependencies (i.e.
libraries required by libsqlite3.so) are listed in .so file and will be
resolved dynamically by ldd upon executing the binary.

For static linking though, one has to list all the required libraries,
both direct and indirect. For libraries with pkgconfig support the
list of required libraries can be obtained with pkg-config:

	$ pkg-config --libs sqlite3 # dynamic linking case
	-lsqlite3
	$ pkg-config --libs --static sqlite3 # static case
	-lsqlite3 -ldl -lpthread

It seems that all one has to do is to fix gosqlite this way:

	-#cgo LDFLAGS: -lsqlite3
	+#cgo pkg-config: sqlite3

Unfortunately, cmd/go doesn't know that it needs to pass --static
flag to pkg-config in case of static linking
(see golang/go#12058).

So, for one, one has to do one of these things:

1. Patch sqlite.go like this:

	-#cgo LDFLAGS: -lsqlite3
	+#cgo pkg-config: --static sqlite3

(this is exactly what I do in goploop, see
kolyshkin/goploop@e9aa072)

2. Patch sqlite.go like this:
	-#cgo LDFLAGS: -lsqlite3
	+#cgo LDFLAGS: -lsqlite3 -ldl -lpthread

(I would submit this patch to gosqlite but it seems that
https://code.google.com/p/gosqlite/ is deserted and not maintained,
and patching it here is not right as it is "vendored")

3. Explicitly add -ldl for the static link case.
This is what this patch does.

4. Fork sqlite to github and maintain it there. Personally I am not
ready for that, as I'm neither a Go expert nor gosqlite user.

Now, #3 doesn't look like a clear solution, but nevertheless it makes
the build much better than it was before.

Signed-off-by: Kir Kolyshkin <kir@openvz.org>
931185f

sallyom added a commit to sallyom/docker that referenced this issue Aug 13, 2015

@kolyshkin @sallyom
Simplify and fix os.MkdirAll() usage 
TL;DR: check for IsExist(err) after a failed MkdirAll() is both
redundant and wrong -- so two reasons to remove it.

Quoting MkdirAll documentation:

> MkdirAll creates a directory named path, along with any necessary
> parents, and returns nil, or else returns an error. If path
> is already a directory, MkdirAll does nothing and returns nil.

This means two things:

1. If a directory to be created already exists, no error is returned.

2. If the error returned is IsExist (EEXIST), it means there exists
a non-directory with the same name as MkdirAll need to use for
directory. Example: we want to MkdirAll("a/b"), but file "a"
(or "a/b") already exists, so MkdirAll fails.

The above is a theory, based on quoted documentation and my UNIX
knowledge.

3. In practice, though, current MkdirAll implementation [1] returns
ENOTDIR in most of cases described in #2, with the exception when
there is a race between MkdirAll and someone else creating the
last component of MkdirAll argument as a file. In this very case
MkdirAll() will indeed return EEXIST.

Because of #1, IsExist check after MkdirAll is not needed.

Because of #2 and #3, ignoring IsExist error is just plain wrong,
as directory we require is not created. It's cleaner to report
the error now.

Note this error is all over the tree, I guess due to copy-paste,
or trying to follow the same usage pattern as for Mkdir(),
or some not quite correct examples on the Internet.

[v2: a separate aufs commit is merged into this one]

[1] https://github.com/golang/go/blob/f9ed2f75/src/os/path.go

Signed-off-by: Kir Kolyshkin <kir@openvz.org>
7894d24

@thaJeztah thaJeztah referenced this issue Jul 25, 2016

Closed

[1.12] Killing leader makes all containers end up on a single node #25017

@vasily-kirichenko vasily-kirichenko referenced this issue Aug 14, 2016

Closed

Swarm VIP stops working on a node #25693

@mixja mixja referenced this issue Sep 12, 2016

Open

Secrets: write-up best practices, do's and don'ts, roadmap #13490

@slawekgh slawekgh referenced this issue Sep 29, 2016

Closed

swarm mode on ubuntu 14.04 - no load balancing at all, published service port is not present on all nodes #27016

@Snorch Snorch referenced this issue Oct 21, 2016

Closed

swarm init segmentation fault #27632

@coryleeio coryleeio referenced this issue Oct 24, 2016

Closed

"host not found" for service in same overlay network #26523

aanand pushed a commit to aanand/docker that referenced this issue Oct 25, 2016

@dnephin
Merge pull request #3 from aanand/compose-on-swarm 
Implement 'environment' option
d7e692b

@erikh erikh referenced this issue Dec 10, 2016

Open

When using copy API for multiple commit runs, cannot persist to volume in image #29303

@zhangyongsu zhangyongsu referenced this issue Jan 8, 2017

Closed

Kernel Panic // dirperm1 breaks the protection by the permission bits on the lower branch #21081

@dattatrayakumbhar dattatrayakumbhar referenced this issue Feb 4, 2017

Merged

#28755: [experimental] Proper error handling for checkpoint conflict #29099

@caleblloyd caleblloyd referenced this issue Feb 4, 2017

Open

Docker for Windows Containers - HNS failed with error : The parameter is incorrect. #30280

@hsoftxl hsoftxl referenced this issue Mar 7, 2017

Open

rm a directory in a container failed #31321

@jasonterando jasonterando referenced this issue Mar 16, 2017

Closed

Docker Windows CE, DNS not working from docker-compose up #31658

@minexew minexew referenced this issue Jul 10, 2017

Closed

Add with relative path to parent directory fails with "Forbidden path" #2745

keloyang pushed a commit to keloyang/cli that referenced this issue Jul 12, 2017

Shukui Yang
Vendor, update moby to d58ffa0364c04d03a8f25704d7f0489ee6cd9634 
Fix docker cp dir with hard link, refer to moby/moby#3.

Signed-off-by: Shukui Yang <yangshukui@huawei.com>
a996206

alshabib added a commit to alshabib/cli that referenced this issue Aug 1, 2017

Shukui Yang @alshabib
Vendor, update moby to d58ffa0364c04d03a8f25704d7f0489ee6cd9634 
Fix docker cp dir with hard link, refer to moby/moby#3.

Signed-off-by: Shukui Yang <yangshukui@huawei.com>
8e9d104

riyazdf added a commit to riyazdf/cli that referenced this issue Aug 2, 2017

Shukui Yang @riyazdf
Vendor, update moby to d58ffa0364c04d03a8f25704d7f0489ee6cd9634 
Fix docker cp dir with hard link, refer to moby/moby#3.

Signed-off-by: Shukui Yang <yangshukui@huawei.com>
70638a2

vieux pushed a commit to vieux/docker-ce that referenced this issue Aug 10, 2017

Shukui Yang
Vendor, update moby to d58ffa0 
Fix docker cp dir with hard link, refer to moby/moby#3.

Signed-off-by: Shukui Yang <yangshukui@huawei.com>
Upstream-commit: a996206cedb04d3fa62723ae96905d83984aa9d0
Component: cli
9cc50cd

@adaiguoguo adaiguoguo referenced this issue Sep 1, 2017

Open

Docker causes system freeze on Ubuntu 14.10 #10355

@mavenugo mavenugo referenced this issue Sep 1, 2017

Closed

Allow user to specify default address pools for docker networks #29376

@jgoeres jgoeres referenced this issue Oct 24, 2017

Open

Swarm & Persistence: Could we have an option that allows creating the "src" directory of a (bind) mount for "service create"? #35278

kolyshkin added a commit to kolyshkin/moby that referenced this issue Nov 27, 2017

@kolyshkin
Simplify/fix MkdirAll usage 
This subtle bug keeps lurking in because error checking for `Mkdir()`
and `MkdirAll()` is slightly different wrt to `EEXIST`/`IsExist`:

 - for `Mkdir()`, `IsExist` error should (usually) be ignored
   (unless you want to make sure directory was not there before)
   as it means "the destination directory was already there"

 - for `MkdirAll()`, `IsExist` error should NEVER be ignored.

Mostly, this commit just removes ignoring the IsExist error, as it
should not be ignored. Also, there are a couple of cases then IsExist
is handled as "directory already exist" which is wrong.

NOTE that `idtools.MkdirAndChown()` behaves like `os.MkdirAll()`
rather than `os.Mkdir()` -- so its description is amended accordingly,
and its usage is handled as such (i.e. IsExist error is not ignored).

For more details, a quote from my runc commit 6f82d4b (July 2015):

    TL;DR: check for IsExist(err) after a failed MkdirAll() is both
    redundant and wrong -- so two reasons to remove it.

    Quoting MkdirAll documentation:

    > MkdirAll creates a directory named path, along with any necessary
    > parents, and returns nil, or else returns an error. If path
    > is already a directory, MkdirAll does nothing and returns nil.

    This means two things:

    1. If a directory to be created already exists, no error is
    returned.

    2. If the error returned is IsExist (EEXIST), it means there exists
    a non-directory with the same name as MkdirAll need to use for
    directory. Example: we want to MkdirAll("a/b"), but file "a"
    (or "a/b") already exists, so MkdirAll fails.

    The above is a theory, based on quoted documentation and my UNIX
    knowledge.

    3. In practice, though, current MkdirAll implementation [1] returns
    ENOTDIR in most of cases described in #2, with the exception when
    there is a race between MkdirAll and someone else creating the
    last component of MkdirAll argument as a file. In this very case
    MkdirAll() will indeed return EEXIST.

    Because of #1, IsExist check after MkdirAll is not needed.

    Because of #2 and #3, ignoring IsExist error is just plain wrong,
    as directory we require is not created. It's cleaner to report
    the error now.

    Note this error is all over the tree, I guess due to copy-paste,
    or trying to follow the same usage pattern as for Mkdir(),
    or some not quite correct examples on the Internet.

    [1] https://github.com/golang/go/blob/f9ed2f75/src/os/path.go

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
0c19f0a

kolyshkin added a commit to kolyshkin/moby that referenced this issue Nov 28, 2017

@kolyshkin
Simplify/fix MkdirAll usage 
This subtle bug keeps lurking in because error checking for `Mkdir()`
and `MkdirAll()` is slightly different wrt to `EEXIST`/`IsExist`:

 - for `Mkdir()`, `IsExist` error should (usually) be ignored
   (unless you want to make sure directory was not there before)
   as it means "the destination directory was already there"

 - for `MkdirAll()`, `IsExist` error should NEVER be ignored.

Mostly, this commit just removes ignoring the IsExist error, as it
should not be ignored.

Also, there are a couple of cases then IsExist is handled as
"directory already exist" which is wrong. As a result, some code
that never worked as intended is now removed.

NOTE that `idtools.MkdirAndChown()` behaves like `os.MkdirAll()`
rather than `os.Mkdir()` -- so its description is amended accordingly,
and its usage is handled as such (i.e. IsExist error is not ignored).

For more details, a quote from my runc commit 6f82d4b (July 2015):

    TL;DR: check for IsExist(err) after a failed MkdirAll() is both
    redundant and wrong -- so two reasons to remove it.

    Quoting MkdirAll documentation:

    > MkdirAll creates a directory named path, along with any necessary
    > parents, and returns nil, or else returns an error. If path
    > is already a directory, MkdirAll does nothing and returns nil.

    This means two things:

    1. If a directory to be created already exists, no error is
    returned.

    2. If the error returned is IsExist (EEXIST), it means there exists
    a non-directory with the same name as MkdirAll need to use for
    directory. Example: we want to MkdirAll("a/b"), but file "a"
    (or "a/b") already exists, so MkdirAll fails.

    The above is a theory, based on quoted documentation and my UNIX
    knowledge.

    3. In practice, though, current MkdirAll implementation [1] returns
    ENOTDIR in most of cases described in #2, with the exception when
    there is a race between MkdirAll and someone else creating the
    last component of MkdirAll argument as a file. In this very case
    MkdirAll() will indeed return EEXIST.

    Because of #1, IsExist check after MkdirAll is not needed.

    Because of #2 and #3, ignoring IsExist error is just plain wrong,
    as directory we require is not created. It's cleaner to report
    the error now.

    Note this error is all over the tree, I guess due to copy-paste,
    or trying to follow the same usage pattern as for Mkdir(),
    or some not quite correct examples on the Internet.

    [1] https://github.com/golang/go/blob/f9ed2f75/src/os/path.go

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
516010e

kolyshkin added a commit to kolyshkin/moby that referenced this issue Dec 6, 2017

@kolyshkin
DockerSwarmSuite.TearDownTest debug 
Debug for the following test failure:

----------------------------------------------------------------------
FAIL: check_test.go:366: DockerSwarmSuite.TearDownTest

Error reading daemon d3cf74261e6a5 logs: <nil>
check_test.go:371:
    d.Stop(c)
daemon/daemon.go:401:
    t.Fatalf("Error while stopping the daemon %s : %v", d.id, err)
... Error: Error while stopping the daemon d3cf74261e6a5 : <nil>

----------------------------------------------------------------------
PANIC: docker_api_swarm_service_test.go:201:
DockerSwarmSuite.TestAPISwarmServicesUpdateStartFirst

[d3cf74261e6a5] waiting for daemon to start
[d3cf74261e6a5] daemon started

[d3cf74261e6a5] daemon started
Attempt #2: daemon is still running with pid 10792
Attempt #3: daemon is still running with pid 10792
Attempt #4: daemon is still running with pid 10792
[d3cf74261e6a5] exiting daemon
... Panic: Fixture has panicked (see related PANIC)

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
0a1c4c0

@anusha-ragunathan anusha-ragunathan referenced this issue Dec 11, 2017

Merged

Fix awslogs batch size calculation #35726

salah-khan added a commit to salah-khan/moby that referenced this issue Dec 14, 2017

@salah-khan
# This is a combination of 3 commits. 
# This is the 1st commit message:

Add ADD/COPY --chown flag support to Windows

This fix actually implements chown support on Windows. Built-in accounts
as well as accounts included in the SAM database of the container are
supported.

The following are valid examples:
ADD --chown=Guest . <some directory>
COPY --chown=Administrator . <some directory>
COPY --chown=Guests . <some directory>
COPY --chown=ContainerUser . <some directory>

On Windows an owner is only granted the permission to read the security
descriptor and read/write the discretionary access control list. This
fix also grants read/write and execute permissions to the owner.

Signed-off-by: Salahuddin Khan <salah@docker.com>

# This is the commit message #2:

Support --chown on Windows.

Changes based on feedback and testing.

Signed-off-by: Salahuddin Khan <salah@docker.com>

# This is the commit message #3:

Minor fix.

Signed-off-by: Salahuddin Khan <salah@docker.com>
576c2be

@gvessere gvessere referenced this issue Mar 21, 2018

Open

--userns-remap causes shared mounts to also become slaves #36472

thaJeztah pushed a commit to thaJeztah/docker that referenced this issue Jul 5, 2018

@andrewhsu
Merge pull request #3 from thaJeztah/18.06-backport-update-windows-ma… 
…nifest-sorting

[18.06] LCOW: Prefer Windows over Linux in a manifest list
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
cba8083

This was referenced Jul 26, 2018

Open
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment