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

skip xfs quota tests when running in user namespace #35526

Merged
merged 1 commit into from Nov 17, 2017

Conversation

Projects
None yet
7 participants
@brauner
Contributor

brauner commented Nov 16, 2017

Commit 7a1618c regresses running Docker
in user namespaces. The new test for quota support 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. This
just restores the status quo.

Signed-off-by: Christian Brauner christian.brauner@ubuntu.com

- What I did
Skip newly introduced check for quota support when running in user namespaces.

- How I did it
Added a check whether we are running in user namespace.

- How to verify it
Tests have been added for quota support as part of 7a1618c.

- Description for the changelog
skip xfs quota tests when running in user namespace

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Nov 16, 2017

Member

linking the pull request that introduced this: #35231

ping @sargun @kolyshkin PTAL

Member

thaJeztah commented Nov 16, 2017

linking the pull request that introduced this: #35231

ping @sargun @kolyshkin PTAL

@estesp

This comment has been minimized.

Show comment
Hide comment
@estesp

estesp Nov 16, 2017

Contributor

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.

Contributor

estesp commented Nov 16, 2017

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

estesp approved these changes Nov 16, 2017

@brauner

This comment has been minimized.

Show comment
Hide comment
@brauner

brauner Nov 16, 2017

Contributor

@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.

Contributor

brauner commented Nov 16, 2017

@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.

@yongtang

LGTM, thanks!

@stgraber

This comment has been minimized.

Show comment
Hide comment
@stgraber

stgraber Nov 16, 2017

@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.

stgraber commented Nov 16, 2017

@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.

@kolyshkin

This comment has been minimized.

Show comment
Hide comment
@kolyshkin

kolyshkin Nov 17, 2017

Contributor

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?

Contributor

kolyshkin commented Nov 17, 2017

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?

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Nov 17, 2017

Member

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

Member

thaJeztah commented Nov 17, 2017

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

@brauner

This comment has been minimized.

Show comment
Hide comment
@brauner

brauner Nov 17, 2017

Contributor

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.

Contributor

brauner commented Nov 17, 2017

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.

@kolyshkin

This comment has been minimized.

Show comment
Hide comment
@kolyshkin

kolyshkin Nov 17, 2017

Contributor

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

Contributor

kolyshkin commented Nov 17, 2017

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

Skip further checks for quota in user namespaces
Commit 7a1618c 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 <christian.brauner@ubuntu.com>
@brauner

This comment has been minimized.

Show comment
Hide comment
@brauner

brauner Nov 17, 2017

Contributor

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 <christian.brauner@ubuntu.com>
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 <christian.brauner@ubuntu.com>
---
Changelog 2017-11-17:
* non-functional changes: rewrite commit message
---
 daemon/graphdriver/quota/projectquota.go | 9 +++++++++
 1 file changed, 9 insertions(+)
Contributor

brauner commented Nov 17, 2017

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 <christian.brauner@ubuntu.com>
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 <christian.brauner@ubuntu.com>
---
Changelog 2017-11-17:
* non-functional changes: rewrite commit message
---
 daemon/graphdriver/quota/projectquota.go | 9 +++++++++
 1 file changed, 9 insertions(+)
@thaJeztah

LGTM, thanks!

@estesp estesp merged commit 8124d21 into moby:master Nov 17, 2017

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 37907 has succeeded
Details
janky Jenkins build Docker-PRs 46620 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 7031 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 18177 has succeeded
Details
z Jenkins build Docker-PRs-s390x 6836 has succeeded
Details

@estesp estesp removed the status/4-merge label Nov 17, 2017

@brauner

This comment has been minimized.

Show comment
Hide comment
@brauner

brauner Nov 17, 2017

Contributor

Thanks everyone!

Contributor

brauner commented Nov 17, 2017

Thanks everyone!

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