cmd/runtimetest/main: Use TAP diagnostics for errors

[wking]

This lets us print SHOULD violations even if they're non-fatal. It also gets us back to consumable TAP output. I added a /foo entry to defaultFS for testing, and before this commit:

$ ./runtimetest
TAP version 13
ok 1 - root filesystem
ok 2 - hostname
ok 3 - mounts
not ok 4 - capabilities
ok 5 - default symlinks
ok 6 - default devices
ok 7 - linux devices
not ok 8 - linux process
ok 9 - masked paths
ok 10 - oom score adj
ok 11 - read only paths
not ok 12 - rlimits
ok 13 - sysctls
ok 14 - uid mappings
ok 15 - gid mappings
1..15
3 errors occurred:

* Unexpected bounding capability chown set for process
* UID expected: 1, actual: 1000
* Wrong rlimit value:
$ echo $?
1

The TAP 13 spec doesn't cover script exit-code handling, but that exit code confuses prove unless you set --ignore-exit:

$ prove ./runtimetest
./runtimetest .. 1/? 3 errors occurred:

* Unexpected bounding capability chown set for process
* UID expected: 1, actual: 1000
* Wrong rlimit value:
./runtimetest .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 4/16 subtests

Test Summary Report
-------------------
./runtimetest (Wstat: 256 Tests: 16 Failed: 4)
  Failed tests:  4, 6, 9, 13
  Non-zero exit status: 1
Files=1, Tests=16,  0 wallclock secs ( 0.02 usr +  0.00 sys =  0.02 CPU)
Result: FAIL

Note the “Dubious, test returned 1”. And with the diagnostics written to stderr, there's no way to have prove catch that.

With this commit, we're back to TAP-compatible output and exit codes, using TAP's diagnostics to share the failure details.

$ ./runtimetest
TAP version 13
ok 1 - root filesystem
ok 2 - hostname
ok 3 - mounts
not ok 4 - capabilities
# Unexpected bounding capability chown set for process
ok 5 - default symlinks
ok 6 - default file system
# /foo SHOULD exist and expected type is tmpfs
# Refer to: https://github.com/opencontainers/runtime-spec/blob/v1.0.0/config-linux.md#default-filesystems
ok 7 - default devices
ok 8 - linux devices
not ok 9 - linux process
# UID expected: 1, actual: 1000
ok 10 - masked paths
ok 11 - oom score adj
ok 12 - read only paths
not ok 13 - rlimits
# Wrong rlimit value:
ok 14 - sysctls
ok 15 - uid mappings
ok 16 - gid mappings
1..16
$ echo $?
0

You can use prove without --ignore-edit to summarize:

$ prove ./runtimetest
./runtimetest .. Failed 3/16 subtests

Test Summary Report
-------------------
./runtimetest (Wstat: 0 Tests: 16 Failed: 3)
  Failed tests:  4, 9, 13
Files=1, Tests=16,  0 wallclock secs ( 0.01 usr +  0.00 sys =  0.01 CPU)
Result: FAIL

And prove knows that the tests failed from TAP, with the script exit code saying “we were able to run the tests” and not speaking to pass/fail:

$ echo $?
1

If that's too compact, you can ask prove to show failures and comments:

$ prove -fo ./runtimetest
./runtimetest .. 1/?
not ok 4 - capabilities
# Unexpected bounding capability chown set for process
# /foo SHOULD exist and expected type is tmpfs
# Refer to: https://github.com/opencontainers/runtime-spec/blob/v1.0.0/config-linux.md#default-filesystems
not ok 9 - linux process
# UID expected: 1, actual: 1000
not ok 13 - rlimits
# Wrong rlimit value:
./runtimetest .. Failed 3/16 subtests

Test Summary Report
-------------------
./runtimetest (Wstat: 0 Tests: 16 Failed: 3)
  Failed tests:  4, 9, 13
Files=1, Tests=16,  0 wallclock secs ( 0.02 usr +  0.00 sys =  0.02 CPU)
Result: FAIL

You can also turn that SHOULD violation into a failure by cranking up the compliance level:

$ prove -fo ./runtimetest :: --compliance-level SHOULD
./runtimetest .. 1/?
not ok 4 - capabilities
# Unexpected bounding capability chown set for process
not ok 6 - default file system
# /foo SHOULD exist and expected type is tmpfs
# Refer to: https://github.com/opencontainers/runtime-spec/blob/v1.0.0/config-linux.md#default-filesystems
not ok 9 - linux process
# UID expected: 1, actual: 1000
not ok 13 - rlimits
# Wrong rlimit value:
./runtimetest .. Failed 4/16 subtests

Test Summary Report
-------------------
./runtimetest (Wstat: 0 Tests: 16 Failed: 4)
  Failed tests:  4, 6, 9, 13
Files=1, Tests=16,  0 wallclock secs ( 0.02 usr +  0.00 sys =  0.02 CPU)
Result: FAIL

Fixed #362.

Related to #308 in that we're improving the TAP output and removing go-multierror, but otherwise unrelated to that PR.

[wking]

Rebased around #437 with 521602f → 9fe8199.

[Mashimiao]

@wking can you rebase this PR? If the PR is not ready to merge, it's a little hard for us to review.

[wking]

@wking can you rebase this PR?

I'll try to rebase tomorrow, but the PR topic message should have sufficient detail to review the user impact. If you feel it doesn't, what more information would you like?

[Mashimiao]

If your PR is not ready for merging, I think most reviewers will not have interest to review it.
Keep PR ready for reviewing, I think that's the basic we need to do.

[wking]

On Wed, Nov 22, 2017 at 06:35:02AM +0000, Ma Shimiao wrote: Keep PR ready for reviewing, I think that's the basic we need to do.

Rebased with 9fe8199 → 9422eec. But I think that's a pretty stiff requirement for a PR that's been open for four months. There doesn't seem to be much point in things like #266's multiple rebase requests [1,2,3] if the PR is not going to get enough review to be merged or rejected after the rebase. I don't see why maintainers would be unwilling to state a position on the general thrust even if the PR has gone slightly stale. This sort of preliminary review would just cover API/UI changes; I don't expect mainainers to spend time looking at stale code. If the maintainer positions were negative, we could reject the PR without bothering to rebase it. And if the maintainer positions were positive, we'd have a short list of maintainers to ping for final, post-rebase LGTMs, hopefully landing the PR before something else landed and made it stale again. Or maybe there are other approaches that would help avoid the repeated rebases without a merge or rejection? Sorting PRs in a queue like we're somewhat doing with milestones helps. But the most recent PR to conflict with this one was #510, and both this PR and #510 are in v0.4.0 [4]. I'm open to other busywork-reduction ideas if the maintainers have something in mind. [1]: #266 (comment) [2]: #266 (comment) [3]: #266 (comment) [4]: https://github.com/opencontainers/runtime-tools/milestone/6

[Mashimiao]

@wking I'm sorry about that if review process makes you unhappy.
I think there may have a little problem on PR review process. We should consider old PRs first if there are not other PRs in higher priority.
We will improve our review process in the future.
And thanks for your comments and contributions.

[wking]

On Fri, Nov 24, 2017 at 06:01:12AM +0000, Ma Shimiao wrote: @wking I'm sorry about that if review process makes you unhappy.

It mostly works out ok. The multiple-rebase thing is just a place where I think there's room for improvement.

We should consider old PRs first if there are not other PRs in higher priority.

That would help sort within milestones. Serializing review like that may slow things down, because if the fist-in-queue PR blocks (for any reason, could be either a maintainer or submitter delay) that holds up later PRs. Maybe a soft serialization? “For the current milestone, consider if *you* are blocking the oldest PR before merging a later PR” or something like that?

[Mashimiao]

On 11/24/2017 02:30 PM, W. Trevor King wrote: That would help sort within milestones. Serializing review like that may slow things down, because if the fist-in-queue PR blocks (for any reason, could be either a maintainer or submitter delay) that holds up later PRs. Maybe a soft serialization? “For the current milestone, consider if *you* are blocking the oldest PR before merging a later PR” or something like that?

I think that makes sense.

[Mashimiao]

I'm OK with using t.Diagnostic(). But I think there is a problem, there is not prove in busybox and currently we just run runtimetest.
The result is no matter what errors occur, TAP always says test PASS, as following

TAP version 13
ok 1 - root filesystem
ok 2 - hostname
ok 3 - process
ok 4 - mounts
ok 5 - user
ok 6 - rlimits
ok 7 - capabilities
ok 8 - default symlinks
ok 9 - default file system
ok 10 - default devices
ok 11 - linux devices
ok 12 - linux process
not ok 13 - masked paths
# /masktest should not be readable
ok 14 - oom score adj
ok 15 - read only paths
ok 16 - rootfs propagation
ok 17 - sysctls
ok 18 - uid mappings
ok 19 - gid mappings
1..19
--- PASS: TestValidateMaskedPaths (0.18s)

I think that's not so suitable, because the result is opposite with fact.

[wking]

But I think there is a problem, there is not prove in busybox and currently we just run runtimetest.

I see two possible solutions:

a. Drop my change to the exit code, and expect folks who do use prove to set --ignore-exit.
b. Use prove (or another TAP consumer) outside of the container.

I'm fine with (a) if folks want a short-term fix, but I think (b) is a better long-term approach (e.g. it allows easily tunable verbosity for #308). I'm fine working up either, or other alternatives, as part of this PR.

[Mashimiao]

considering #308, I prefer b. Ping @opencontainers/runtime-tools-maintainers

[wking]

On Fri, Nov 24, 2017 at 07:27:29AM +0000, Ma Shimiao wrote: considering #308, I prefer b.

Done with 9422eec → b2677cb; I can stash the new commits for later if the maintainer consensus is to punt on (b) for now. The new implementation is ignoring the runtimetest TAP output and error code, but that's nothing new. Master has been ignoring them since #447 [1]. Once the container type gains a method like runc's ‘run’ (to create, start, wait on exit, and delete), we can use that in RuntimeInsideValidate to fix the broken tests. [1]: #447 (comment)

[liangchenye]

+1 for the b plan. It is a big improvement.
We also need to add a document to help user to get the prove tool, or add a script to contrib directory.

[liangchenye]

@Mashimiao @q384566678 I merged 3 minor fixes just now and I think we would better stop working on other PRs until we merging this one and #308.

[Mashimiao]

I also want this PR landed as soon as possible.
But considering we have been behind after schedule a lot, I think we can work on generate related PRs while waiting #439 and #308.
Of course, our merging based on those PRs will not affect the two PRs.
How do you think about it? @liangchenye

[wking]

We also need to add a document to help user to get the prove tool…

Pushed with b2677cb → b2c08bf.

[Mashimiao]

@wking I think @liangchenye has found the root cause, would you like to update the PR?

[wking]

... would you like to update the PR?

I've been working on it, but don't have everything fixed yet. Maybe by tonight or tomorrow.

[Mashimiao]

Got it, thanks.

[wking]

Ok, updated with 3f9cb51 → 72a957c. There's still a FIXME sleep in there, but as long as runtimetest takes less than a second we can punt on a real fix. And folks in a hurry can run prove with ‘-j 50’ or whatever to parallelize the sleeps. And there are a few tests failing for other reasons with poor error messages, like: $ sudo RUNTIME=runc validation/linux_gid_mappings.t … not ok 19 - gid mappings # linux.gidMappings is not applied as expected … which I haven't had time to look into yet.

[Mashimiao]

validation/linux_gid_mappings.t ................... Failed 1/19 subtests 
validation/linux_rootfs_propagation_unbindable.t .. failed to create the container
rootfsPropagation=unbindable is not supported
exit status 1
validation/linux_rootfs_propagation_unbindable.t .. Dubious, test returned 1 (wstat 256, 0x100)
No subtests run 
validation/linux_readonly_paths.t ................. ok    
validation/linux_masked_paths.t ................... Failed 1/19 subtests 

It seems except container created failed, other the failed reasons are not output?

[wking]

On Thu, Nov 30, 2017 at 06:35:24AM +0000, Ma Shimiao wrote: validation/linux_rootfs_propagation_unbindable.t .. failed to create the container rootfsPropagation=unbindable is not supported exit status 1 validation/linux_rootfs_propagation_unbindable.t .. Dubious, test returned 1 (wstat 256, 0x100) No subtests run validation/linux_readonly_paths.t ................. ok validation/linux_masked_paths.t ................... Failed 1/19 subtests ``` It seems except container created failed…

Create errors are currently outside of TAP, because I'm leaving TAP up to runtimetest. And creation errors are currently not really compliance failures anyway. For example, “rootfsPropagation=unbindable is not supported” is a legal runtime reaction based on [1] (although you have opencontainers/runtime-spec#813 trying to get that addressed and I have opencontainers/runtime-spec#927 addressing part of it). Anyway, with the current spec we may want mini-TAP output on create failures like: TAP version 13 1..1 ok 1 # SKIP create container with unbindable linux.rootfsPropagation # rootfsPropagation=unbindable is not supported # exit status 1 for create failures.

… other the failed reasons are not output?

You can tell prove how much detail you're interested in [2]. For example: $ sudo RUNTIME=runc prove -fo --directives validation/create.t validation/mounts.t validation/create.t .. 1/? # https://github.com/opencontainers/runtime-spec/blob/v1.0.0/runtime.md#create # exit status 1 # /home/wking/bin/runc: "create" requires exactly 1 argument(s) # https://github.com/opencontainers/runtime-spec/blob/v1.0.0/runtime.md#create # container PID: %!d(string=478dfbf9-68bb-488c-8b16-1d12c41b5e98), state ID: %!d(string=478dfbf9-68bb-488c-8b16-1d12c41b5e98) # https://github.com/opencontainers/runtime-spec/blob/v1.0.0/runtime.md#create # open /tmp/ocitest715783221/stdout-478dfbf9-68bb-488c-8b16-1d12c41b5e98: file exists validation/create.t .. ok validation/mounts.t .. 1/? ok 1 # SKIP TODO mounts generation options have not been implemented validation/mounts.t .. ok All tests successful. Files=2, Tests=5, 0 wallclock secs ( 0.02 usr 0.00 sys + 0.03 cusr 0.01 csys = 0.06 CPU) Result: PASS That is currently also showing diagnostics on success, and TAP doesn't seem to provide a way to turn those off. Unless we can find a way to do that (or patch prove to support it), we may want to stop printing diagnostics unless they're related to failures. Or maybe we should transition all of our diagnostics to YAML blocks [3]; I'll check and see if those are supported differently. [1]: https://github.com/opencontainers/runtime-spec/blame/v1.0.1/config.md#L463 [2]: #308 (comment) [3]: https://testanything.org/tap-version-13-specification.html#yaml-blocks

[wking]

YAML-block support PR filed upstream in mndrix/tap-go#7. If/when that lands, I'll transition from Diagnostic(f) to YAML here.

[Mashimiao]

LGTM

[liangchenye]

LGTM