linking the pull request that introduced this: #35231

ping @sargun @kolyshkin PTAL

Nice to see you over here @brauner :)

This seems totally reasonable to me given the limitations when running in a userns. We have talked about trying to add some form of "run Docker in a userns" integration test to CI to keep from these breakages when we added the initial capability with @hallyn but it's a bit tricky since it adds host requirements (e.g. a properly configured lxc for example). Obviously that would help for situations where new features are added which break this use case.

@estesp, very happy to see you man! :) I hope post-conference season things have been a little more quiet for you. :)

We have talked about trying to add some form of "run Docker in a userns" integration test to CI to keep from these breakages when we added the initial capability

We've set something like this up for ourselves though it's rather hacky atm ( https://github.com/lxc/lxc-ci/blob/master/bin/test-lxd-docker ) but it at least allows us to see breakages.

@estesp https://jenkins.linuxcontainers.org/job/lxd-test-docker/

That's run daily on our Jenkins and helped quite a bit to pinpoint this issue within a 24h commit window.

Indeed, CAP_MKNOD is not allowed for userns.

LGTM except for the commit message which is a bit misleading as it tells about tests, while the code it modifies is not in tests. I think it should say what it does (returns "quota not supported" for project quota when running in userns). Surely it needs to be mentioned that this was found due to failed tests, and the tests are no longer failing after this commit.

Perhaps @sargun can come up with a better decription?

@kolyshkin would "feature detection" instead of "tests" better fit your expectation?

LGTM except for the commit message which is a bit misleading as it tells about tests, while the code it modifies is not in tests. I think it should say what it does (returns "quota not supported" for project quota when running in userns).

With "test" I'm simply trying to refer to
7a1618c#diff-79adb8998bcaf170baf4318e7c63ac44R39
which tries to determine (If that is better parlance.) whether quota are supported. But I'm happy to change it to whatever you prefer.

Surely it needs to be mentioned that this was found due to failed tests, and the tests are no longer failing after this commit.

It wasn't found due to failed test in moby's test suite so that would be inaccurate I think.

Oh, sorry about it, I got it now. I was fooled by "xfs quota tests" and "Skip quota tests", which sounds like we talk about some unit tests. Perhaps it can be phrased as "Skip further checks for quota if we are running in a user namespace, returning ErrQuotaNotSupported" or smth.

Anyway, LGTM

Perhaps it can be phrased as "Skip further checks for quota if we are running in a user namespace, returning ErrQuotaNotSupported" or smth.

Oh sure, np. I updated the commit message nothing else was changed:

From 7e35df0e0484118740dbf01e7db9b482a1827ef1 Mon Sep 17 00:00:00 2001
From: Christian Brauner <[email protected]>
Date: Thu, 16 Nov 2017 12:54:31 +0100
Subject: [PATCH] Skip further checks for quota in user namespaces

Commit 7a1618ced359a3ac921d8a05903d62f544ff17d0 regresses running Docker
in user namespaces. The new check for whether quota are supported calls
NewControl() which in turn calls makeBackingFsDev() which tries to
mknod(). Skip quota tests when we detect that we are running in a user
namespace and return ErrQuotaNotSupported to the caller. This just
restores the status quo.

Signed-off-by: Christian Brauner <[email protected]>
---
Changelog 2017-11-17:
* non-functional changes: rewrite commit message
---
 daemon/graphdriver/quota/projectquota.go | 9 +++++++++
 1 file changed, 9 insertions(+)

Thanks everyone!