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

Split runtime validate #391

Closed

Conversation

Mashimiao
Copy link

If we want to totally validate a runtime, we do not only need to check container inside environment but also outside settings like cgroup, label, lifecycle. So I split runtimevalidate into to parts: inside and outside.
The following is an example output:
`# make localvalidation
RUNTIME=runc go test -tags "" -v github.com/opencontainers/runtime-tools/validation
=== RUN TestValidateRuntimeInside
TAP version 13
ok 1 - root filesystem
ok 2 - hostname
not ok 3 - mounts
not ok 4 - capabilities
ok 5 - default symlinks
ok 6 - default devices
ok 7 - linux devices
ok 8 - linux process
ok 9 - masked paths
ok 10 - oom score adj
ok 11 - read only paths
ok 12 - rlimits
ok 13 - sysctls
ok 14 - uid mappings
ok 15 - gid mappings
1..15
2 errors occurred:

  • Expected mount {/volume/testing bind /tmp [rw bind]} does not exist
  • Expected ambient capability chown not set for process
    --- FAIL: TestValidateRuntimeInside (0.27s)
    validation_test.go:42: runc failed validation: exit status 1
    === RUN TestValidateRuntimeOutside
    TAP version 13
    ok 1 - labels
    1..1
    --- PASS: TestValidateRuntimeOutside (0.08s)
    FAIL
    exit status 1
    FAIL github.com/opencontainers/runtime-tools/validation 0.350s
    Makefile:40: recipe for target 'localvalidation' failed
    make: *** [localvalidation] Error 1
    `

@Mashimiao Mashimiao force-pushed the split-runtime-validate branch from 2bf25f7 to ae33d7b Compare June 9, 2017 08:04
@wking
Copy link
Contributor

wking commented Jun 14, 2017 via email

if err != nil {
t.Errorf("%s failed validation: %v", runtime, err)
}
g.SetProcessArgs([]string{"sleep", "50"})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we setting this at all, since sleep may not exist inside the container? process is optional (since opencontainers/runtime-spec#701), so I'd rather just unset process completely and call create (but never start) during these outer tests.

@Mashimiao Mashimiao force-pushed the split-runtime-validate branch from ae33d7b to a259dda Compare June 15, 2017 05:47
@Mashimiao
Copy link
Author

Mashimiao commented Jun 15, 2017 via email

@Mashimiao
Copy link
Author

ping @opencontainers/runtime-tools-maintainers

if err != nil {
return err
}
// defer os.RemoveAll(bundleDir)

// TODO: Use a library to split run into create/start
// Launch the OCI runtime
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're using create below, so this comment is stale here. You will want to run kill and delete to clean up though.

@Mashimiao Mashimiao force-pushed the split-runtime-validate branch from a259dda to 9ee5be2 Compare June 22, 2017 02:02
@Mashimiao Mashimiao force-pushed the split-runtime-validate branch from 9ee5be2 to 0d93e18 Compare June 29, 2017 02:39
@Mashimiao
Copy link
Author

Rebased, PTAL

@liangchenye
Copy link
Member

I like this change. It makes lifecycle testing easier.
It is the first step, we need to check the output result when 'create/start/stop'.

if err != nil {
return err
}
defer os.RemoveAll(tmpDir)
// defer os.RemoveAll(bundleDir)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing this change was for local testing, and that we want to restore the deferred removal before this PR lands?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah... yes, this is a mistake

bundleDir := tmpDir + "/busybox"
if err := os.MkdirAll(bundleDir, 0755); err != nil {
// Copy the runtimetest binary to the rootfs
err = fileutils.CopyFile("../runtimetest", filepath.Join(rootfsDir, "runtimetest"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this actually work? My local build of runtimetest is dynamically linked:

$ lddtree runtimetest 
runtimetest (interpreter => /lib64/ld-linux-x86-64.so.2)
    libpthread.so.0 => /lib64/libpthread.so.0
    libc.so.6 => /lib64/libc.so.6

and neither that linker nor those libraries aren't part of our Busybox rootfs. For example, here is the statically-linked Busybox working fine in a chroot based on that rootfs:

$ unshare -rUmfp --mount-proc sh -c 'mount --rbind /proc proc && chroot . sh -c "pwd && ls"'
/
bin          etc          proc         runtimetest

And here is runtimetest failing to launch in the same situation:

$ unshare -rUmfp --mount-proc sh -c 'mount --rbind /proc proc && chroot . /runtimetest'
chroot: failed to run command ‘/runtimetest’: No such file or directory

I expect we're going to need to either compile runtimetest as a static binary (#442), copy the linker and required libraries into rootfs, or use busybox for our internal tests.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need #442 merged first

@Mashimiao Mashimiao force-pushed the split-runtime-validate branch from ddef754 to 18b08d4 Compare August 14, 2017 03:10
Signed-off-by: Ma Shimiao <mashimiao.fnst@cn.fujitsu.com>
@Mashimiao Mashimiao force-pushed the split-runtime-validate branch from 18b08d4 to 2010c81 Compare August 14, 2017 03:38
@liangchenye
Copy link
Member

Hi @Mashimiao,
One thing we miss is, we don't have test cases.
Inspired by this PR, I think we can add more Test** functions.

For example, we can add TestCreate function. In this function, we provides testing cases to cover
different scenarios, for example:

  1. success in create
  2. fail to create due to args missing
  3. fail to create since the id is duplicated
    ...

I used to add test like this:
liangchenye@b55cfcd

@wking
Copy link
Contributor

wking commented Aug 30, 2017

I think this PR is obsolete, or at least in need of a fairly involved rebase, now that #447 has landed.

@Mashimiao Mashimiao closed this Oct 18, 2017
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