Skip to content
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

validation: add test for NSProcInPath #628

Merged
merged 7 commits into from
May 29, 2018

Conversation

dongsupark
Copy link
Contributor

This PR validates NSProcInPath, i.e. The runtime MUST place the container process in the namespace associated with that path. It checks that Linux namespaces are created with a given path by making use of util.RuntimeOutsideValidate().

Since the previous version of PR, we had to fix the following things:

  • Fix a bug caused by namespace string mismatches, found by @alban.
  • Kill the unshare process as well as child processes at once, making use of setpgid().
  • Use a ticker loop instead of a pure sleep call for general sync mechanism.
  • Print out correct diagnostics based on specError.

This PR replaces #613.
See also #572.

@dongsupark dongsupark force-pushed the dongsu/NSProcInPath branch 2 times, most recently from ab68d5b to e450168 Compare April 24, 2018 08:28
@dongsupark dongsupark force-pushed the dongsu/NSProcInPath branch from e450168 to 8e46543 Compare May 3, 2018 07:52
@zhouhao3
Copy link

It failed when I tested:

➜  runtime-tools git:(dongsupark/NSProcInPath) ✗ sudo ./linux_ns_path
TAP version 13
failed to create the container
namespace {"cgroup" "/proc/27701/ns/cgroup"} does not exist
not ok 1 - set cgroup namespace by path
  ---
  {
    "actual": "err == exit status 1",
    "expected": "err == nil",
    "level": "MUST",
    "namespace type": "cgroup",
    "reference": "https://github.com/opencontainers/runtime-spec/blob/v1.0.0/config-linux.md#namespaces"
  }
  ...
ok 2 - set ipc namespace by path
ok 3 - set mnt namespace by path
ok 4 - set net namespace by path
ok 5 - set pid namespace by path
failed to create the container
User namespaces enabled, but no uid mappings found.
not ok 6 - set user namespace by path
  ---
  {
    "actual": "err == exit status 1",
    "expected": "err == nil",
    "level": "MUST",
    "namespace type": "user",
    "reference": "https://github.com/opencontainers/runtime-spec/blob/v1.0.0/config-linux.md#namespaces"
  }
  ...
ok 7 - set uts namespace by path
1..7

For user namespace errors, I think that uid should be set, but I don't know why the cgroups namespace are wrong.

@dongsupark
Copy link
Contributor Author

@q384566678 Yeah, I'm aware of both failures.
Userns test fails because UID/GID mappings are currently disabled, due to unclear runtime-specs. According to the discussion in opencontainers/runtime-spec#961, I suppose we can enable UID/GID mappings in this test, right?

Cgroupns test fails because runc has never supported cgroup namespaces. See the pending PR opencontainers/runc#1184.

@zhouhao3
Copy link

According to the discussion in opencontainers/runtime-spec#961, I suppose we can enable UID/GID mappings in this test, right?

I think so.

Cgroupns test fails because runc has never supported cgroup namespaces. See the pending PR opencontainers/runc#1184.

I think we can add some explanations to the cgroups to facilitate understanding.

alban and others added 6 commits May 23, 2018 11:58
I initially tried to add the checks in the container process
'runtimetest' by adding annotations prefixed with "runtimetest/".  But
that proved impractical with TAP outputs because I wanted to have
several tests for each namespace.

This patch now validates the namespaces outside the container with
util.RuntimeOutsideValidate().

Signed-off-by: Alban Crequy <[email protected]>
We need to deal with additional namespace strings, in case of mount
& network namespaces, because `MapStrToNamespace()` does not recognize
input strings like `mnt` or `net`.

Found by @alban.

Signed-off-by: Dongsu Park <[email protected]>
`unshare --fork` spawn child processes, which remains even after the
test program finished. To be able to kill these processes at once, we
should set a process group for the child processes.

Signed-off-by: Dongsu Park <[email protected]>
Since it takes some time until unshare can switch to a new namespace,
we need a sync mechanism for the NSProcInPath.
Let's use a generic sync mechanism by using select & time ticker
instead of pure sleep.

Signed-off-by: Dongsu Park <[email protected]>
Like other tests, `testNamespacePath` should also print out details
about the failed tests, based on specError.
Also make `waitForState(doCheckNamespacePath)` return a correct error
coming from `checkNamespacePath`, not from `waitForState`.

Signed-off-by: Dongsu Park <[email protected]>
Now that a new helper `NewRFCError()` is available, we should make use
of the helper instead of `specerror.NewError()`. This way, we can avoid
doing multiple casts to be able to get rfcError.

Signed-off-by: Dongsu Park <[email protected]>
@dongsupark dongsupark force-pushed the dongsu/NSProcInPath branch from 8e46543 to 8043e83 Compare May 23, 2018 11:48
@dongsupark
Copy link
Contributor Author

@q384566678
Pushed with a new commit for enabling UID/GID mappings, as well as adding comments about cgroup namespaces.
Though userns test still fails. I'm not sure why. Will look into it later again.

@dongsupark dongsupark force-pushed the dongsu/NSProcInPath branch from 8043e83 to 742644a Compare May 25, 2018 11:16
@dongsupark
Copy link
Contributor Author

@q384566678
Ok, I decided to exclude both namespaces from this PR, user and cgroups.
As for cgroup namespaces, it's nothing special, as mentioned above.
Though for user namespaces, I've seen so many different kinds of issues when running unshare + runc. I'll have a deeper look into these issues. When I figure it out, I'll create a separate PR.
I think we should not get this PR blocked forever. For it to be merged, I exclude both user and cgroups from this PR.

@dongsupark dongsupark force-pushed the dongsu/NSProcInPath branch 2 times, most recently from a61d4e2 to ad0e97e Compare May 25, 2018 13:41
Cgroup namespaces test fails because runc does not support it yet.
User namespaces test fails because of many unexpected issues when
running unshare with runc, etc.

We are going to revisit these tests later, to figure out how to deal
with them. Let's exclude these two types of namespaces for now.

Signed-off-by: Dongsu Park <[email protected]>
@zhouhao3
Copy link

zhouhao3 commented May 29, 2018

LGTM

Approved with PullApprove

@zhouhao3 zhouhao3 merged commit 9b65419 into opencontainers:master May 29, 2018
@zhouhao3 zhouhao3 mentioned this pull request May 29, 2018
44 tasks
@dongsupark dongsupark deleted the dongsu/NSProcInPath branch May 29, 2018 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants