overlay: warn if overlay backing fs doesn't support d_type by AkihiroSuda · Pull Request #27433 · moby/moby

AkihiroSuda · 2016-10-17T05:15:26Z

- What I did

Print a warning if the backing fs for overlay/overlay2 doesn't support d_type.
Close #27358

Considering the discussion at #22937, the warning is not treated as an error.
However, I still think it is useful to treat it as an error in some future version (1.14? 1.15?).

Note that recent kernel prints a similar message: torvalds/linux@e7c0b59
However I think there are advantages to print the message as well in Docker:

Can support older kernels
Can provide much more user-friendly information (e.g. "You need to reformat with f_type=1" for XFS)
Can be switched to an error in future Docker versions

It also shows information in docker info.

- How I did it
Please refer to the source of pkg/fsutils.SupportsDType.

- How to verify it
For example, XFS with ftype=0 prints this warning.

$ qemu-img create /xfs-ftype0.img 10G
$ mkfs.xfs -m crc=0 -n ftype=0 /xfs-ftype0.img
$ mount -o loop /xfs-ftype0.img /mnt
$ dockerd -s overlay2 -g /mnt
WARN[0001] overlay2 won't work properly because the backing filesystem xfs is not configured to support d_type. (The backing filesystem seems formatted with f_type=0. Please r
eformat the filesytem with f_type=1.) Note that this warning is planned to be treated as a fatal error in v1.16

The warning should not be shown if ftype=1.

docker info shows f_type Supported information:

$ docker info
...
Storage Driver: overlay2
 Backing Filesystem: xfs
 Supports d_type: false
...

- Description for the changelog

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

TODO

PR for doc (after this PR gets merged)

Signed-off-by: Akihiro Suda suda.akihiro@lab.ntt.co.jp

AkihiroSuda · 2016-10-17T05:15:50Z

CC @rhvgoyal

thaJeztah · 2016-10-17T05:23:04Z

/cc @dmcgowan @justincormack

rhvgoyal · 2016-10-17T13:47:47Z

If this is just a warning, then kernel is already emitting a warning. How does it help doing another one.

dmcgowan · 2016-10-17T17:56:55Z

How does it help doing another one.

This was prompted by the issue here #27358. The warning should have been shown in the kernel but not is not somewhere users will look before filing an issue. However I am not convinced that adding a warning to the Docker logs will make this situation much better.

@AkihiroSuda what if instead we added a key mentioning d_type status to the the status output so when issues are filed it is easier to see the problem in the docker info output without making users dump their logs. My feeling here is we have to add something to make diagnosis easier, mentioning in the documentation and having warning in the logs can only go so far. I also don't see it as reasonable to someday flip this to an error, the kernel update to make this a warning was done because it had an impact on users, how would that impact change in the future?

thaJeztah · 2016-10-17T20:26:35Z

@dmcgowan I guess we should decide how serious the situation is if someone runs docker without this. If it's a serious issue, then the overlay driver should probably be skipped (as "unsupported"), otherwise, we could output it as warning in docker info (where we already output various warnings if things are not supported). wdyt?

AkihiroSuda · 2016-10-18T02:41:05Z

Updated PR

Now warning emits user-friendly guidance for xfs ("The backing filesystem seems formatted with f_type=0. Please reformat with f_type=1.")
Now docker info contains f_type Supported information

@rhvgoyal

This PR can support older kernel that doesn't print warning
This PR can provide user-friendly guidance ("The backing filesystem seems formatted with f_type=0. Please reformat with f_type=1.")

@dmcgowan
Makes sense, I added the information to docker info

@thaJeztah
IMO overlay is almost unuseful without d_type, and it should be an error after some "grace period" (1.14 or 1.15)

thaJeztah · 2016-10-18T04:16:48Z

IMO overlay is almost unuseful without d_type, and it should be an error after some "grace period" (1.14 or 1.15)

If we are ok with doing that, we should add that in the warning ("running without t_type=0 will no longer be supported in 1.15" for example (needs a better sentence))

AkihiroSuda · 2016-10-19T07:20:26Z

If this should be considered as a "feature deprecation", the warning should remain in v1.13, v1.14. and v.1.15. So the warning will be "running without t_type=0 will no longer be supported in 1.16" in such a case.
https://docs.docker.com/engine/#feature-deprecation-policy

Before an existing feature is removed it will be labeled as “deprecated” within the documentation and will remain in Docker for, usually, at least 3 releases. After that time it may be removed.

IMO this is not a "feature deprecation", because "supporting overlay without d_type" is a bug rather than a feature.
However, I'm ok to set the grace period (when the warning is changed to an error) to v1.16 so that it won't surprise users.

thaJeztah · 2016-10-19T22:18:55Z

We discussed this, and suggest to:

make this show up in docker info
show a warning if it's not supported (as suggested above)
make this a "fatal" error in 1.16

This allows users that currently run without this being supported to migrate/address the issue

tonistiigi · 2016-10-19T22:19:25Z

pkg/fsutils/fsutils.h

It should be possible to do this without c. https://golang.org/pkg/syscall/#Dirent is exposed in go.

AkihiroSuda · 2016-10-20T07:44:50Z

@thaJeztah thank you for making the discussion.

Updated PR and now the warning message is like this:

WARN[0001] overlay2 won't work properly because the backing filesystem xfs is not configured to support d_type. (The backing filesystem seems formatted with f_type=0. Please r
eformat the filesytem with f_type=1.) Note that this warning is planned to be treated as a fatal error in v1.16 .

thaJeztah · 2016-10-20T16:17:34Z

Trying to see if we can make this more consistent with the devmapper dev-sync message;

devmapper: Udev sync is not supported. This will lead to data loss and unexpected
behavior. Install a dynamic binary to use devicemapper or select a different storage
driver. For more information, see https://docs.docker.com/engine/reference/commandline/daemon/#daemon-storage-driver-option

Perhaps (wrapped for readability):

WARN[0001] overlay2: the backing xfs filesystem is configured without d_type support,
which leads to incorrect behavior. Reformat the filesytem with f_type=1 to fix this
issue. Running without d_type support will no longer be supported in Docker 1.16.

@mstanleyjones perhaps you have suggestions?

mdlinville · 2016-10-21T18:14:29Z

How about:

WARN[0001] overlay2: the backing XFS filesystem is formatted without d_type support,
which leads to incorrect behavior. Reformat the filesystem with ftype=1 to enable d_type
support. Running without d_type support will no longer be supported in Docker 1.16.

According to the man page for mkfs.xfs, it is ftype, not f_type. It's odd that ftype=1 is d_type support. I wonder if we should really not use the terminology d_type but say that filesystem information will be stored in the directory structure.

AkihiroSuda · 2016-10-25T03:11:25Z

Updated PR according to the suggestion from @mstanleyjones

mdlinville

Docs LGTM

thaJeztah · 2016-10-25T17:37:38Z

ping @justincormack @tonistiigi PTAL

dmcgowan · 2016-10-25T18:51:05Z

pkg/fsutils/fsutils_linux.go

What is the reasoning behind checking multiple files? Is there a case where there can be a mix of unknown type and known types?

I agree that checking a single file is enough in practice.

But I wonder theoretically there can be a mix of unknown type and known types in some filesystem, so I wrote the code to check multiple files here. ( it should be very lightweight operation)

Please let me know if I should fix this.

AkihiroSuda · 2016-10-28T03:50:24Z

pkg/fsutils/fsutils_linux.go

self-nit: walk maybe misleading because it is not recursive.
I'll try to change the terminology

AkihiroSuda · 2016-11-03T07:13:15Z

rebased to master
@justincormack @tonistiigi PTAL

vieux · 2016-11-09T02:18:46Z

ping @justincormack @tonistiigi

tonistiigi · 2016-11-09T06:39:18Z

pkg/fsutils/fsutils_linux_test.go

I'd remove these helper functions as there is only one caller and they don't seem to have a repeatable use-case. Just makes the flow harder to follow.

Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>

tonistiigi · 2016-11-09T17:40:50Z

LGTM

dmcgowan · 2016-11-09T21:36:33Z

LGTM

cherry-pick from moby#27433 Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp> Signed-off-by: Lei Jitang <leijitang@huawei.com>

AkihiroSuda force-pushed the fix-ovl-xfs-ftype0 branch from 20b0064 to 616033d Compare October 17, 2016 05:17

thaJeztah added the status/2-code-review label Oct 17, 2016

AkihiroSuda force-pushed the fix-ovl-xfs-ftype0 branch 3 times, most recently from 4d3a043 to 1610de6 Compare October 18, 2016 02:34

AkihiroSuda force-pushed the fix-ovl-xfs-ftype0 branch from 1610de6 to 32f163f Compare October 19, 2016 06:59

tonistiigi reviewed Oct 19, 2016

View reviewed changes

AkihiroSuda force-pushed the fix-ovl-xfs-ftype0 branch from 32f163f to 0b5b59f Compare October 20, 2016 07:43

AkihiroSuda force-pushed the fix-ovl-xfs-ftype0 branch from 0b5b59f to 707ea30 Compare October 20, 2016 07:46

AkihiroSuda force-pushed the fix-ovl-xfs-ftype0 branch from 707ea30 to 59731ee Compare October 25, 2016 03:10

thaJeztah added this to the 1.13.0 milestone Oct 25, 2016

mdlinville approved these changes Oct 25, 2016

View reviewed changes

dmcgowan reviewed Oct 25, 2016

View reviewed changes

AkihiroSuda commented Oct 28, 2016

View reviewed changes

AkihiroSuda force-pushed the fix-ovl-xfs-ftype0 branch 2 times, most recently from 69bf27f to 8a8d73c Compare November 3, 2016 07:12

vieux assigned justincormack Nov 9, 2016

tonistiigi reviewed Nov 9, 2016

View reviewed changes

overlay: warn if overlay backing fs doesn't support d_type

2e20e63

Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>

AkihiroSuda force-pushed the fix-ovl-xfs-ftype0 branch from 8a8d73c to 2e20e63 Compare November 9, 2016 07:18

AkihiroSuda added the rebuild/experimental label Nov 9, 2016

GordonTheTurtle removed the rebuild/experimental label Nov 9, 2016

thaJeztah assigned tonistiigi Nov 9, 2016

vieux merged commit 643ac2f into moby:master Nov 9, 2016

thaJeztah added the area/storage/overlay label Nov 10, 2016

AkihiroSuda mentioned this pull request Nov 11, 2016

add 1.13.0 CHANGELOG.md #28275

Merged

thaJeztah added the impact/changelog label Nov 11, 2016

albamc mentioned this pull request Nov 14, 2016

Xfs quota for overlay #24807

Closed

thaJeztah mentioned this pull request Feb 3, 2017

[1.13.0]Files in containers filesystem were acting odd when volume mapping #30612

Closed

This was referenced Feb 23, 2017

Can not remove directory inside container created during image build #31283

Closed

Unable to delete files in container #31445

Closed

AkihiroSuda mentioned this pull request May 8, 2017

overlay: abort if backing fs lacks support for d_type containerd/containerd#815

Merged

bebehei mentioned this pull request Aug 23, 2017

Can't use pacman archimg/archlinux#18

Closed

liusdu pushed a commit to liusdu/moby that referenced this pull request Oct 30, 2017

overlay: warn if overlay backing fs doesn't support d_type

1df007e

cherry-pick from moby#27433 Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp> Signed-off-by: Lei Jitang <leijitang@huawei.com>

Conversation

AkihiroSuda commented Oct 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AkihiroSuda commented Oct 17, 2016

Uh oh!

thaJeztah commented Oct 17, 2016

Uh oh!

rhvgoyal commented Oct 17, 2016

Uh oh!

dmcgowan commented Oct 17, 2016

Uh oh!

thaJeztah commented Oct 17, 2016

Uh oh!

AkihiroSuda commented Oct 18, 2016

Uh oh!

thaJeztah commented Oct 18, 2016

Uh oh!

AkihiroSuda commented Oct 19, 2016

Uh oh!

thaJeztah commented Oct 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda commented Oct 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah commented Oct 20, 2016

Uh oh!

mdlinville commented Oct 21, 2016

Uh oh!

AkihiroSuda commented Oct 25, 2016

Uh oh!

mdlinville left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Oct 25, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda commented Nov 3, 2016

Uh oh!

vieux commented Nov 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonistiigi commented Nov 9, 2016

Uh oh!

dmcgowan commented Nov 9, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

AkihiroSuda commented Oct 17, 2016 •

edited

Loading

AkihiroSuda commented Oct 20, 2016 •

edited

Loading