add tests when prestart/poststart/poststop hooks fail by liangchenye · Pull Request #569 · opencontainers/runtime-tools

liangchenye · 2018-02-06T03:12:49Z

Signed-off-by: Liang Chenye liangchenye@huawei.com

liangchenye · 2018-02-06T03:13:49Z

It should after #567.
PTAL @q384566678 @wking

wking

Some suggestions for poststart_fail.go. Some of these may apply to the other files you're adding too, but I haven't had time to look those over yet.

wking · 2018-02-06T05:46:25Z

validation/poststart_fail.go

+			r.SetID(uuid.NewV4().String())
+			g := util.GetDefaultGenerator()
+			poststart := rspec.Hook{
+				Path: fmt.Sprintf("%s/%s/bin/false", r.BundleDir, g.Spec().Root.Path),


Can we use filepath.Join here?

OK, I'll update it.

wking · 2018-02-06T05:51:45Z

validation/poststart_fail.go

+				Path: fmt.Sprintf("%s/%s/bin/false", r.BundleDir, g.Spec().Root.Path),
+				Args: []string{"false"},
+			}
+			g.AddPostStartHook(poststart)


I think you also want a second hook which writes to a file, and a post-RuntimeLifecycleValidate check to ensure that file was never written. Similarly with SetProcessArgs, you'll want to set something there so we can confirm that the runtime didn't actually execute the user-specified program.

+1 for this, I'll update these.

wking · 2018-02-06T05:54:50Z

validation/poststart_fail.go

+	t.Header(0)
+
+	config := util.LifecycleConfig{
+		Actions: util.LifecycleActionCreate | util.LifecycleActionStart | util.LifecycleActionDelete,


No need to set util.LifecycleActionDelete or the PreDelete code below, because the start call should exit nonzero after the hook failure. That means RuntimeLifecycleValidate will exit here, before any of its delete-related code fires. You will need to explicitly delete the container in this file, and not rely on RuntimeLifecycleValidate for deletion.

https://github.com/opencontainers/runtime-spec/blob/master/runtime.md#lifecycle
In poststart fail test, it will continue to work. 'start' should not exit.

Also with 'ActionDelete' set, even there is no chance to get 'r.Delete', the container will be deleted in the 'defer' function.

runtime-tools/validation/util/test.go

Line 294 in 3eba113

defer func() {

wking · 2018-02-06T05:58:21Z

validation/poststart_fail.go

+
+	err := util.RuntimeLifecycleValidate(nil, config)
+	if err != nil {
+		err := specerror.NewError(specerror.PoststartHookFailGenWarn, fmt.Errorf("if any poststart hook fails, the runtime MUST log a warning, but the remaining hooks and lifecycle continue as if the hook had succeeded"), rspec.Version)


Instead of assuming any error is the error we're trying to trigger, we should add a PostCreate action that flips a boolean:

successfulCreate := false config := util.LifecycleConfig{ … PostCreate: func(r *util.Runtime) error { successfulCreate = true return nil }, } … if !successfulCreate { …fail the test… } else if err != nil { …what you have here… }

The sequence test is covered by 'prestart.go/poststart.go/poststop.go'. Here we just need to verify if the hook and lifecycle after 'poststart' will still work even if 'poststart' fails.
So we just need to verify if 'poststop' hook works.

liangchenye · 2018-02-06T12:07:50Z

change the RuntimeLifecycleValidation by
1.1 add a 'BundleDir' to config
it will be easier to test output out of the lifecycleValidation function
1.2 add a 'generator' to config
we don't need to have an extra parameter
updates testing code by checking the outputs

liangchenye · 2018-02-06T12:11:46Z

One issue found in 'runc': once a hook fail in 'poststart' or 'poststop', the remain hooks will not be excused.

One issue found in our generator: we cannot have hooks with a same Path. https://github.com/opencontainers/runtime-tools/blob/master/generate/generate.go#L918
The spec does not forbid it.

zhouhao3 · 2018-02-07T06:46:05Z

validation/poststart_fail.go

+		Args: []string{
+			"sh", "-c", fmt.Sprintf("echo 'post-stop called' >> %s", output),
+		},
+	}


I don't think this poststop is needed. If the final output is not post-start called\npost-stop called\n, it's not immediately clear that it's poststart's error, and further judgment is needed. So I think this poststop should be removed to remove unnecessary effects.

you are right, updated.

zhouhao3 · 2018-02-07T06:46:59Z

validation/poststop_fail.go

+	}
+	g.AddPostStopHook(poststop)
+	poststopOK := rspec.Hook{
+		Path: filepath.Join(bundleDir, g.Spec().Root.Path, "/bin/echo"),


/bin/echo -> /bin/sh

liangchenye · 2018-02-07T07:52:28Z

updated PTAL @q384566678

Remove the unnecessary 'poststop' hook check in poststart_fail.
Correct the wrong path

zhouhao3 · 2018-02-07T08:55:00Z

validation/poststop_fail.go

+	// if runErr is not nil, it means the runtime generates an error
+	// if outputData is not equal to the expected content, it means there is something wrong with the remaining hooks and lifecycle
+	//	if runErr != nil || string(outputData) != "post-stop called\n" {
+	if runErr != nil || string(outputData) != "" {


I think the line you commented out is correct.

// if runErr != nil || string(outputData) != "post-stop called\n" {

ops, big mistake.
Updated

Signed-off-by: Liang Chenye <liangchenye@huawei.com>

zhouhao3 · 2018-02-07T09:12:15Z

LGTM

liangchenye mentioned this pull request Feb 6, 2018

'prestart/poststart' hooks are done in the 'create' operation opencontainers/runc#1710

Closed

liangchenye force-pushed the master branch from 8d3db60 to e061a10 Compare February 6, 2018 03:20

wking reviewed Feb 6, 2018

View reviewed changes

liangchenye force-pushed the master branch from e061a10 to 66e4232 Compare February 6, 2018 12:00

zhouhao3 reviewed Feb 7, 2018

View reviewed changes

liangchenye force-pushed the master branch from 66e4232 to ca34a9c Compare February 7, 2018 07:25

liangchenye mentioned this pull request Feb 7, 2018

don't overwrite hook which has a same path #571

Merged

zhouhao3 reviewed Feb 7, 2018

View reviewed changes

add tests when prestart/poststart/poststop hooks fail

9177741

Signed-off-by: Liang Chenye <liangchenye@huawei.com>

liangchenye force-pushed the master branch 2 times, most recently from 4b08dde to 9177741 Compare February 7, 2018 09:05

zhouhao3 merged commit dc61225 into opencontainers:master Feb 8, 2018

Conversation

liangchenye commented Feb 6, 2018

Uh oh!

liangchenye commented Feb 6, 2018

Uh oh!

wking left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wking Feb 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liangchenye Feb 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liangchenye commented Feb 6, 2018

Uh oh!

liangchenye commented Feb 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liangchenye commented Feb 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhouhao3 commented Feb 7, 2018 • edited by caniszczyk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

wking Feb 6, 2018 •

edited

Loading

liangchenye Feb 6, 2018 •

edited

Loading

zhouhao3 commented Feb 7, 2018 •

edited by caniszczyk

Loading