Carry 232: fix issue #228 replace start to run

[Mashimiao]

No description provided.

[wking]

This still needs subreaper magic or similar to collect the container exit code.

[Mashimiao]

I think it's not. The test_runtime.sh when executed "start container" only need to judge whether container has been started successfully. runtimetest itself prints out whether all validations succeed or not.

[wking]

"Wait an indeterminate amount of time before scraping stout for success messages" isn't the friendliest UI for automated compliance tests ;). And as this PR stands the caller has to call delete on their own too.

[Mashimiao]

If so, we can add result output for each validation

[wking]

I'm having difficulty parsing your last comment. My point is that runtime validation will be much more usable (e.g. from continuous integration tests) if the validation command (currently test_runtime.sh) has an exit code that reflects success/failure for the runtime 's compliance. And I don't see a better way to achieve that than with subreaper magic.

[Mashimiao]

The point is runtimetest's exit code can't reflects whether validation success or not. I investigated code of urfave/cli, app.Run() will not return error exit code of app.Action And I don't want to change too much, I think by now it's the best way to print out result.

[wking]

On Wed, Nov 30, 2016 at 10:04:05PM -0800, Ma Shimiao wrote: I investigated code of urfave/cli, app.Run() will not return error exit code of app.Action

Really? It looks like runtimetest is exiting with the appropriate error code to me: $ git checkout origin/master $ git describe --always 893d9e3 $ make all $ ./runtimetest config.json not found $ echo $? 1 What I think we should be doing is collecting that exit code from runtimetest and returning it when test_runtime.sh exits.

[Mashimiao]

On 12/01/2016 02:18 PM, W. Trevor King wrote: Really? It looks like runtimetest is exiting with the appropriate error code to me: $ git checkout origin/master $ git describe --always 893d9e3 $ make all $ ./runtimetest config.json not found $ echo $? 1

Well, that's a little different from What I expected. if err := app.Run(os.Args); err != nil { logrus.Fatal(err) } I mean logrus.Fatal(err) will not be executed, if validation error out. Of course, if there are some errors app will exit with 1. Sorry for my poor description.

What I think we should be doing is collecting that exit code from runtimetest and returning it when test_runtime.sh exits.

I also want this. But I don't acutally understand what do you mean about subreaper magic. As the start exit code is not the proceess's exit code, so I made changes to let runtimetest itself print out validation result. I have to admit getting exit code of runtimetest is the best way.

[Mashimiao]

And I think we can't solve collecting exit code from runtimetest by using start command in short time.
And currently test_runtime.test has affected normal use. Changing to printing out result of validations based on current conditions is acceptable, in my opinion.

[wking]

On Wed, Nov 30, 2016 at 10:38:48PM -0800, Ma Shimiao wrote: Well, that's a little different from What I expected. if err := app.Run(os.Args); err != nil { logrus.Fatal(err) } I mean logrus.Fatal(err) will no be executed, if validation err out.

No? With the same build as before: $ ./oci-runtime-tool generate | j . >config.json $ ./runtimetest Hostname expected: mrsdalloway, actual: ullr $ echo $? 1

But I don't acutally understand what do you mean about subreaper magic.

My comment in #232 [1] linked here [2] and here [3]. [2] is Go code from @crosbymichael showing how to set subreaper to collect the container process's exit code. [3] is docs in the in-flight command line API, which has links to relevant man pages. [1]: #232 (comment) [2]: opencontainers/runc#827 (comment) [3]: https://github.com/opencontainers/runtime-spec/pull/513/files#diff-0c1e5a5baa5649945697a530e09acbf5R131 Different line number now. Stable links into in-flight PRs are tricky :p, but it's the “Container process exit” section.

[Mashimiao]

On 12/01/2016 03:01 PM, W. Trevor King wrote: $ ./oci-runtime-tool generate | j . >config.json $ ./runtimetest Hostname expected: mrsdalloway, actual: ullr $ echo $? 1

That's printed out by HandleExitCoder(err) in github.com/urfave/cli/app.go not by logrus.Fatal()

[wking]

On Wed, Nov 30, 2016 at 11:06:34PM -0800, Ma Shimiao wrote: On 12/01/2016 03:01 PM, W. Trevor King wrote: > $ ./oci-runtime-tool generate | j . >config.json > $ ./runtimetest > Hostname expected: mrsdalloway, actual: ullr > $ echo $? > 1 That's print out by HandleExitCoder(err) in github.com/urfave/cli/app.go not by logrus.Fatal()

Does it matter? If logrus.Fatal() is never called, then we should drop that line. But as long as runtimetest is returning zero if and only if validation succeeded, that's enough (from runtimetest) for me. The rest is useful-but-not-critical debug logging. test_runtime.sh needs to catch the runtimetest exit code (e.g. by setting subreaper like @crosbymichael does in his example) and return that though. Besides making the test_runtime.sh exit code meaningful, it would also give you a place to attach the ‘delete’ call to avoid leaking resources.

[Mashimiao]

On 12/01/2016 03:20 PM, W. Trevor King wrote: test_runtime.sh needs to catch the runtimetest exit code (e.g. by setting subreaper like @crosbymichael does in his example)

one question, can we implement this by bash?

[wking]

On Wed, Nov 30, 2016 at 11:29:31PM -0800, Ma Shimiao wrote: On 12/01/2016 03:20 PM, W. Trevor King wrote: > test_runtime.sh needs to catch the runtimetest exit code (e.g. by > setting subreaper like @crosbymichael does in his example) one question, can we implement this in bash?

I'm not 100% sure, but my guess is no. If/when we can land a non-detach version of ‘start’ that can (optionally) stay attached, we can go back to pure shell. I've tried to push for that option [1,2], but haven't had much success. Until then, I recommend we write a wrapper in Go (or Python, or C, or whatever) based on @crosbymichael's example which sets subreaper, calls start, blocks until the container process exits, and returns its exit code. [1]: opencontainers/runtime-spec#513 (comment) [2]: opencontainers/runc#827 (comment)

[Mashimiao]

On 12/01/2016 03:45 PM, W. Trevor King wrote: I'm not 100% sure, but my guess is no. If/when we can land a non-detach version of ‘start’ that can (optionally) stay attached, we can go back to pure shell. I've tried to push for that option [1,2], but haven't had much success. Until then, I recommend we write a wrapper in Go (or Python, or C, or whatever) based on @crosbymichael's example which sets subreaper, calls start, blocks until the container process exits, and returns its exit code.

I also think we can't implement it by bash. And what I'm sure is test_runtime.sh will be replaced finally. We will recreate or rebuild a test tool to support container lifecyle and cgroups validation. And I prefer to implement in GO. If so, we can implement crosbymichal's example. I made the changes just want to optimize output of runtimetest, and let test_runtime.sh can work based on latest runc. I think this is transitional change, what you care about can implement in future test tool.

[wking]

On Wed, Nov 30, 2016 at 11:59:25PM -0800, Ma Shimiao wrote: We will recreate or rebuild a test tool to support container lifecyle and cgroups validation… I made the changes just want to optimize output of runtimetest, and let test_runtime.sh can work based on latest runc. I think this is transitional change, what you care about can implement in future test tool.

It's a transitional change that fundamentally breaks the test_runtime.sh exit code. If I was a maintainer, that would be a blocker ;). But I'm not, so you're free to break it (hopefully temporarily) if you like. That's just the carried commit from #232 though. The new-in-this-PR validateSucceed / validateFailed commit doesn't break anything. My preference for that sort of thing is to use TAP (some notes in opencontainers/image-tools#60), but if you want to roll your own output format in runtimetest I don't have a problem with it. How would you feel about landing that change without carrying the commit from #232 until someone writes the wrapper?

[Mashimiao]

On 12/01/2016 04:07 PM, W. Trevor King wrote: How would you feel about landing that change without carrying the commit from #232 until someone writes the wrapper?

Sorry for my poor English. I'm not really sure what do you mean. Do you mean if I insisted on rolling my own output, I should push a new PR with the new commit in this PR? Or something else?

[wking]

On Thu, Dec 01, 2016 at 12:35:40AM -0800, Ma Shimiao wrote: On 12/01/2016 04:07 PM, W. Trevor King wrote: > How would you feel about landing that change without carrying the > commit from #232 until someone writes the wrapper? … Do you mean if I insisted on rolling my own output, I should push a new PR with the new commit in this PR? Or something else?

This PR currently has two commits: * 6117361, carried from #232, breaks test_runtime.sh's exit code. * d76bbd7, new in this PR, adjusts stdout for runtimetest. I'm agnostic on d76bbd7. It can land or not as you see fit. I'd rather not land 6117361 without *also* landing a wrapper to set subreaper and catch the runtimetest exit code for us. If you want d76bbd7 without 6117361 (which is fine with me), you could do something like: $ git checkout carry-232 $ git rebase --onto origin/master HEAD^ $ git push -f Mashimiao carry-232 although at that point the branch has nothing to do with #232. If the branch-name missmatch bothers you, you could create a new branch with just d76bbd7 and open a new PR. And if you aren't convinced by my push-back against 6117361 (without the subreaper wrapper), you can leave this PR exactly as it stands :p.

[Mashimiao]

6117361 will actually breaks test_runtime.sh's exit code.
But without this change, test_runtime.sh also can't work.
I want to hear other opinions from @opencontainers/runtime-tools-maintainers

[mrunalp]

We need subreaper based wrapper to tie create and start together.

[wking]

On Fri, Dec 02, 2016 at 09:44:12AM -0800, Mrunal Patel wrote: We need subreaper based wrapper to tie create and start together.

I don't think you want to handle ‘start’ in the subreaper; that would make it hard to test the lifecycle around ‘start’. All we should need is a subreaper that calls ‘create’ and waits to collect the container process's exit code. You can call ‘start’, ‘delete’, etc. separately.

[Mashimiao]

Closed as test_runtime.sh is not needed any more and we already have output of test passed