add lifecycle validation by zhouhao3 · Pull Request #558 · opencontainers/runtime-tools

zhouhao3 · 2018-01-22T06:22:34Z

I am going to add a validation of the lifecycle, the specific norms are as follows:

The pre-start hooks MUST be called after the start operation is called but before the user-specified program command is executed.

But I can not confirm the specific call state, so for the moment I'm just verifying that the relevant command has not been called.

This is just a preliminary idea. Lifecycle validation also need more work, any comments welcome. Or you can write a new patch.

Signed-off-by: Zhou Hao zhouhao@cn.fujitsu.com

liangchenye · 2018-01-22T09:03:24Z

The pre-start hooks MUST be called after the start operation is called but before the user-specified program command is executed.

Before 'start', we have 'create' operation, so I think this spec means:

when 'create', pre-start hooks SHOULD not be called
only when 'start', pre-start hooks will be called and be called after process

How about make a testcase like this:

in 'pre-start hook' we can set a command echo "pre-start called" >> $bundle/$rootfs/output
in 'process' we can set a command echo "process called" >> $bundle/$rootfs/output

so when 'create', the $bundle/$rootfs/output should be empty,
when 'start, the $bundle/$rootfs/output should look like this:
pre-start called process called

liangchenye · 2018-01-22T11:41:16Z

I think RuntimeOutsideValidate is not suitable for lifecycle test.
We need to add 'preStart', 'postStart', 'postStop' functions as the arguments.
I try to do it in #557.

zhouhao3 · 2018-01-23T09:06:46Z

I've already seen #557, and the setting of LifecycleFuncs is good. I will wait for #557 to merge and then update this patch.

zhouhao3 · 2018-01-31T09:31:05Z

According to the description of lifecycle, I have determined the order of calls for each part:

create -> start -> prestart -> process -> poststart -> delete -> poststop

The code is updated according to this call order.
@liangchenye @wking @Mashimiao PTAL

zhouhao3 · 2018-01-31T09:34:00Z

validation/lifecycle.go

+				return err
+			}
+			if string(outputData) != "" {
+				return errors.New("Wrong call")


I am not quite sure what to say about this type of error. Any suggestions?

I am not quite sure what to say about this type of error.

The associated specerror entries start around here.

Depending on what is read, the specific error is different. It would be very troublesome to be accurate to every error condition. So instead of making a further distinction, I want to output the error directly.

Do you think it is necessary to distinguish between each error?

Yes, each error should explicitly reflect the relevant 'specerror'.

initlove · 2018-01-31T11:05:47Z

validation/lifecycle.go

+	t.Header(0)
+
+	prestart := rspec.Hook{
+		Path: "/usr/bin/prestart",


Where does /usr/bin/prestart come from?

About this path, I thought it was created automatically when added.
Maybe this idea is wrong.
Do you have any suggestions for the path?

Do you have any suggestions for the path?

The echo on our BusyBox tarball?

I'm writing a demo to show my idea, I'll link to you soom.

liangchenye · 2018-02-01T03:39:27Z

Following your idea, I made some changes: https://github.com/liangchenye/ocitools/blob/gotest/validation/lifecycle.go

using 'sh' and 'echo' in the bundle
compose hooks in 'preCreate'

About the 'Wrong Error", I suggest we may split the lifecycle.go to 'prestart.go', 'poststart.go' and the etc. Each hook has multiply spec errors, it will make it easier to maintain and cover all the errors. How do you think ? @q384566678

wking · 2018-02-01T04:27:24Z

On Wed, Jan 31, 2018 at 07:39:28PM -0800, 梁辰晔 (Liang Chenye) wrote: 1. using 'sh' and 'echo' in the bundle

This general approach looks good to me, but you'll want to use Go's filepath.Join to compose the paths for Windows compat. And we'll need future work to ensure we have a usable tarball for the host platform that includes ‘sh’, ‘echo’, and anything else we need.

About the 'Wrong Error", I suggest we may split the lifecycle.go to 'prestart.go', 'poststart.go' and the etc. Each hook has multiply spec errors, it will make it easier to maintain and cover all the errors.

This sounds like a good idea to me. Common code can always go into a helper, although I'm not sure how much overlap there will be.

zhouhao3 · 2018-02-01T05:09:04Z

@liangchenye That sounds good. I'll update patch in this way. Thank you for your advice.

zhouhao3 · 2018-02-01T07:38:11Z

updated, @liangchenye @wking PTAL.

liangchenye · 2018-02-01T08:04:24Z

validation/poststart.go

+		},
+		PostCreate: func(r *util.Runtime) error {
+			outputData, err := ioutil.ReadFile(output)
+			if err != nil {


It is expected to get an 'err', since the 'output' will not exist.

liangchenye · 2018-02-01T08:05:28Z

validation/poststart.go

+				return err
+			}
+			switch string(outputData) {
+			case "":


It should not happen, we can move it to 'default' part.

liangchenye · 2018-02-01T08:11:48Z

validation/poststart.go

+			switch string(outputData) {
+			case "":
+				return nil
+			case "post-start called\n":


We can make it simpler:

if strings.Contains(string(outputData), "post-start") { return specerror.NewError(specerror.PoststartTimin } else if strings.Contains(string(outputData), "process") { return specerror.NewError(specerror.ProcNotRunAtResRequest) }

liangchenye · 2018-02-01T08:12:54Z

validation/poststart.go

+		PreDelete: func(r *util.Runtime) error {
+			outputData, err := ioutil.ReadFile(output)
+			if err != nil {
+				return err


Also

return fmt.Errorf("%v\n%v", specerror.NewError(specerror.PoststartHooksInvoke,

liangchenye · 2018-02-01T08:14:50Z

I commented in prestart.go and it misses one case:

 If any prestart hook fails, the runtime MUST generate an error, stop the container, and continue the lifecycle at step 9.

liangchenye · 2018-02-01T08:16:27Z

validation/poststop.go

+		},
+		PostCreate: func(r *util.Runtime) error {
+			outputData, err := ioutil.ReadFile(output)
+			if err != nil {


Similar to 'poststart.go', we should get an error: the output file is not exist.

liangchenye · 2018-02-01T08:19:24Z

validation/prestart.go

+		},
+		PostCreate: func(r *util.Runtime) error {
+			outputData, err := ioutil.ReadFile(output)
+			if err != nil {


same, should get an error.

liangchenye · 2018-02-01T08:20:15Z

validation/prestart.go

+				return err
+			}
+			switch string(outputData) {
+			case "":


"" could merge with default

liangchenye · 2018-02-01T08:20:30Z

validation/poststop.go

+				return err
+			}
+			switch string(outputData) {
+			case "":


a part of 'default'

liangchenye · 2018-02-01T08:20:44Z

validation/poststart.go

+				return err
+			}
+			switch string(outputData) {
+			case "":


a part of 'default'

liangchenye · 2018-02-01T08:21:08Z

validation/prestart.go

+				return err
+			}
+			switch string(outputData) {
+			case "":


part of 'default'

liangchenye · 2018-02-01T08:22:22Z

validation/prestart.go

+				return specerror.NewError(specerror.PrestartTiming, fmt.Errorf("Pre-start hooks MUST be called after the `start` operation is called"), rspec.Version)
+			case "process called\n":
+				return specerror.NewError(specerror.ProcNotRunAtResRequest, fmt.Errorf("The user-specified program (from process) MUST NOT be run at this time"), rspec.Version)
+			case "pre-start called\nprocess called\n", "process called\npre-start called\n":


remove this, change

case "pre-start called\n"

to

strings.Contain

liangchenye · 2018-02-01T08:23:11Z

validation/poststop.go

+				return err
+			}
+			switch string(outputData) {
+			case "":


a part of default

liangchenye · 2018-02-01T08:25:32Z

Same to other hooks.

 If any poststart hook fails, the runtime MUST log a warning, but the remaining hooks and lifecycle continue as if the hook had succeeded.

if any poststop hook fails, the runtime MUST log a warning, but the remaining hooks and lifecycle continue as if the hook had succeeded.

zhouhao3 · 2018-02-02T06:17:26Z

@liangchenye @wking updated, PTAL.

liangchenye · 2018-02-02T09:55:29Z

~~It looks to me. (but I'm waiting for another day to merge it.)~~
In my test, I found runc has three issues:

prestart/poststart hook is called in 'create'
~~2. when the poststop hook was set, the main process never work~~

~~I'll doubt check the runc code.~~

liangchenye · 2018-02-02T10:08:09Z

I saw another problem in the test code, the process 'output' filepath is wrong, it is already running inside the container.

g.SetProcessArgs([]string{"sh", "-c", fmt.Sprintf("echo 'process called' >> %s", output)})

liangchenye · 2018-02-02T10:17:05Z

validation/poststart.go

+			}
+
+			g.AddPostStartHook(poststart)
+			g.SetProcessArgs([]string{"sh", "-c", fmt.Sprintf("echo 'process called' >> %s", output)})


Change to:
g.SetProcessArgs([]string{"sh", "-c", fmt.Sprintf("echo 'process called' >> %s", "/output")})

liangchenye · 2018-02-02T10:20:44Z

validation/poststart.go

+			}
+			return nil
+		},
+		PreDelete: func(r *util.Runtime) error {


I suggest we can add

util.WaitingForStatus(*r, util.LifecycleStatusStopped, time.Second * 10, time.Second)

'start' will return immediately, although 'echo' runs fast, but we are not sure how many time it costs. (for example, in runc/cc/Kata project)

liangchenye · 2018-02-02T10:22:48Z

validation/poststart.go

+		PostCreate: func(r *util.Runtime) error {
+			outputData, err := ioutil.ReadFile(output)
+			if err == nil {
+				if strings.Contains(string(outputData), "post-start called") {


I always got it in 'runc'!

liangchenye · 2018-02-02T10:23:13Z

validation/prestart.go

+			}
+
+			g.AddPreStartHook(prestart)
+			g.SetProcessArgs([]string{"sh", "-c", fmt.Sprintf("echo 'process called' >> %s", output)})


Same, the output is wrong.

liangchenye · 2018-02-02T10:23:39Z

validation/prestart.go

+			return nil
+		},
+		PreDelete: func(r *util.Runtime) error {
+			outputData, err := ioutil.ReadFile(output)


Same, I suggest to add 'Wait.."

liangchenye · 2018-02-02T10:26:08Z

validation/prestart.go

+		PostCreate: func(r *util.Runtime) error {
+			outputData, err := ioutil.ReadFile(output)
+			if err == nil {
+				if strings.Contains(string(outputData), "pre-start called") {


I always get this in 'runc'.

liangchenye · 2018-02-02T10:27:00Z

validation/poststart.go

+			case "process called\n":
+				fmt.Fprintln(os.Stderr, "WARNING: The poststart hook invoke fails")
+				return nil
+			case "post-start called\nprocess called\n":


since 'post-start' runs in 'create', the sequence here is wrong.

liangchenye · 2018-02-02T10:27:45Z

validation/poststop.go

+			}
+
+			g.AddPostStopHook(poststop)
+			g.SetProcessArgs([]string{"sh", "-c", fmt.Sprintf("echo 'process called' >> %s", output)})


same, wrong output

liangchenye · 2018-02-02T11:29:14Z

Report an issue in 'runc' project: opencontainers/runc#1710.

Signed-off-by: Zhou Hao <zhouhao@cn.fujitsu.com>

liangchenye · 2018-02-05T03:48:40Z

LGTM

zhouhao3 added this to the v0.5.0 milestone Jan 24, 2018

zhouhao3 force-pushed the lifecycle-validation branch from 2f516d4 to 7c18974 Compare January 31, 2018 09:25

zhouhao3 commented Jan 31, 2018

View reviewed changes

initlove reviewed Jan 31, 2018

View reviewed changes

zhouhao3 force-pushed the lifecycle-validation branch 2 times, most recently from 1c6387c to 4ad25ac Compare February 1, 2018 07:34