Fix #556 #557

liangchenye · 2018-01-22T03:32:51Z

should rebase after validation/util/container: Use --bundle (and stop requiring BundleDir) #551 -- done
should add more test case to cover 'create'
should add test case to cover 'kill' -- will do that in other PR

Signed-off-by: Liang Chenye [email protected]

liangchenye · 2018-01-22T14:17:36Z

This PR does two things:

add LifecycleFuncs

type LifecycleFuncs struct {
 	Setup     func(runtime *Runtime) error
 	PreStart  func(runtime *Runtime) error
 	PostStart func(runtime *Runtime) error
 	PostStop  func(runtime *Runtime) error
 }

It is similar to RuntimeOutsideValidate(g *generate.Generator, f AfterFunc) .
But I think we don't need to define PreFunc or AfterFunc, we can use func(runtime *Runtime) and get any information from the Runtime structure.

So if this approach is useful, we can merge 'RuntimeInsideValidate'， ‘RuntimeOutsideValidate' and 'RuntimeLifecycleValidate' into one.

add PIDFile test
I take this as an example of using 'RuntimeLifecycleValidate'.

wking · 2018-01-22T17:26:51Z

validation/pidfile.go

+	t.Header(0)
+
+	var lifecycle util.LifecycleFuncs
+	tempFile, err := ioutil.TempFile("", "oci-pidfile")


This approach (using an empty but already existing PID file) seems strange to me. We may want to update the CLI spec to specifically address this case and keep this test, but I think a more traditional test would be to use a temp directory with a --pid-file path that is a direct child of that temp directory (so the target path does not exist when create is called).

It is not an existing PID file, it is also a new temp file, similar to using TempDir + 'pid'.

It is not an existing PID file, it is also a new temp file, similar to using TempDir + 'pid'.

The TempFile docs say it creates the file. TempDir + "pid" would not.

wking · 2018-01-22T17:29:10Z

validation/pidfile.go

+		util.Fatal(err)
+	}
+	tempPidFile := tempFile.Name()
+	lifecycle.Setup = func(r *util.Runtime) error {


Instead of initializing lifecycle and then filling in these properties, it might be more idiomatic to use:

lifecycle := util.LifecycleFuncs{ Setup: func(…) error { … }, PreStart: …, PostStart: …, }

wking · 2018-01-22T17:31:39Z

validation/pidfile.go

+	defer os.Remove(tempPidFile)
+
+	lifecycle.PreStart = func(r *util.Runtime) error {
+		pidData, err := ioutil.ReadFile(tempPidFile)


I think we want a PostCreate slot (maybe instead of the PreStart slot?). We can run the --pid-file test with create / kill / delete without involving start.

+1, 'PostCreat'e is better, 'Prestart' might also be confusing,

wking · 2018-01-22T17:31:55Z

validation/pidfile.go

+	}
+
+	lifecycle.PostStart = func(r *util.Runtime) error {
+		time.Sleep(1 * time.Second)


What's this about?

Waiting for the container to stop, or we cannot delete it.
There should be a better function to waiting for the status change.

wking · 2018-01-22T17:32:40Z

validation/pidfile.go

+		return nil
+	}
+
+	lifecycle.PostStop = nil


Isn't this the default already? I don't think we need this line.

wking · 2018-01-22T17:33:20Z

validation/pidfile.go

+	lifecycle.PostStop = nil
+
+	g := util.GetDefaultGenerator()
+	g.SetProcessArgs([]string{"ls"})


I'd rather use:

g.Spec().Process = nil

and skip start.

'runc' has a bug: if the 'Process' is nil, runc crashes.

Besides this, 'runc' does not follow the CLI spec, it applies the 'process args' when create a container.
I add a test to change the 'process args' in PostCreate, in theory, runc should execute the new args, but it does not.

'runc' has a bug: if the 'Process' is nil, runc crashes

create crashes? That would be a bug.

I add a test to change the 'process args' in PostCreate, in theory, runc should execute the new args, but it does not.

No, the runtime must ignore any post-create config changes, so runc is fine on that point.

wking · 2018-01-22T17:33:56Z

validation/util/container.go

@@ -56,14 +57,21 @@ func (r *Runtime) SetID(id string) {
 	r.ID = id
 }

+// SetPidFile sets the pid file
+func (r *Runtime) SetPidFile(pidFile string) {


PidFile is already public. I don't think we need a setter.

wking · 2018-01-22T17:34:54Z

validation/util/container.go

@@ -146,23 +154,32 @@ func (r *Runtime) State() (rspecs.State, error) {
 }

 // Delete a container
-func (r *Runtime) Delete() error {
+func (r *Runtime) Delete() ([]byte, error) {


Why are we returning stderr? You don't seem to consume this new value anywhere.

I use this to debug why 'Delete' fails.
The 'time.Sleep' line is coming from this.

wking · 2018-01-22T17:35:59Z

validation/util/test.go

+		}
+	}
+
+	stderr, err = r.Start()


I think we'll want a flag to toggle whether we call Start or not.

So something like this

func RuntimeLifecycleValidate(g *generate.Generator, lifecycle *LifecycleFuncs, actions LifecycleAction) { ... if actions & LifeCycleCreate { stderr, err := r.Create() } ... if actions & LifeCycleStart { stderr, err = r.Start() } ... }

liangchenye · 2018-01-23T11:45:03Z

add LifecycleFuncs 'Setup/PostCreate/PreStop/PostStop'
'PostCreate' and 'PreStop' are slight different with 'PreStart' and 'PostStart', so these new names are used.
add WaitingForStatus
It is flexible to add this to the testing code.
add pidfile.go
It is used to test if '--pid-file' works as expected
I added a WaitingForStatus(Created) function in 'PreStop', just in case the 'creating' takes a long time.

Two issues were found, but I think they could be solved in other PRs.

ProcessArgs issue of 'runc'
'runc' validates ProcessArgs in the 'create' phase which is not the spec expected.
Now I use 'true' command in pidfile test.
Delete a container with the 'Created' status
I think we can delete a container with 'created' status, we should update the spec.

PTAL @wking @q384566678 @Mashimiao

zhouhao3 · 2018-01-24T06:14:49Z

LGTM

wking · 2018-01-24T06:23:42Z

validation/pidfile.go

+		util.Fatal(err)
+	}
+	tempPidFile := filepath.Join(tempDir, "pidfile")
+	defer os.RemoveAll(tempDir)


nit: I'd rather swap the order of these two lines to get the cleanup defer right after the TempDir error check. filepath.Join cannot error, but having the defer set as early as possible would give me warm fuzzy feelings ;).

OK, I'll update it.

wking · 2018-01-24T06:26:45Z

validation/util/test.go

+)
+
+var lifecycleStatusMap = map[string]LifecycleStatus{
+	"creating": LifecycleStatusCreating,


Or these could just be strings:

type LivecycleStatus string const ( LifecycleStatusCreating = "creating" … )

and then you cast with LifecycleStatus(state.Status). I don't see a need for indirection through an integer.

An 'integer' is useful when we expect to have multiple statuses. For example, if we want to delete a container, we expected to have 'created' or 'stopped' status. So we can 'WaitingForStatus(Created|Stopped)'.

wking · 2018-01-24T06:28:12Z

validation/util/test.go

@@ -184,3 +248,74 @@ func RuntimeOutsideValidate(g *generate.Generator, f AfterFunc) error {
 	}
 	return nil
 }
+
+// RuntimeLifecycleValidate validates runtime lifecycle.
+func RuntimeLifecycleValidate(g *generate.Generator, lifecycle *LifecycleFuncs, actions LifecycleAction) error {


I don't see a need to split funcs from actions. Can we rename LifecycleFuncs to LifecycleConfig (or just Lifecycle?) and add an Actions property?

+1 for this

wking · 2018-01-24T06:30:32Z

validation/util/test.go

+		}
+	}
+
+	if lifecycle != nil && lifecycle.PostStop != nil {


I think anything running after Delete should be PostDelete. This PR currently seems to handle stop and delete in one step (or maybe it is never intended to call stop and instead waits for the container to stop on its own?), but calling this setting PostDelete makes sense regardless of an explicit stop or not.

So all the embedded functions should have different new, I'll update it too.

I prefer calling the 'stop' in the related test cases, for example, if we want to test 'kill'.
We can add test codes in the 'PreDelete'. (PreDelete is better than PreStop)

Fixes opencontainers#556 Signed-off-by: Liang Chenye <[email protected]>

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

Updated by using lifecycleConfig (merge funcs and actions), now the funcs are symmetrical: PreCreate/PostCreate, PreDelete/PostDelete

Mashimiao · 2018-01-24T13:39:40Z

LGTM

zhouhao3 · 2018-01-25T03:23:04Z

LGTM

wking · 2018-01-25T03:38:26Z

validation/pidfile.go

+			return nil
+		},
+		PreDelete: func(r *util.Runtime) error {
+			return util.WaitingForStatus(*r, util.LifecycleStatusCreated, time.Second*10, time.Second*1)


We shouldn't need this, because a container must be created before create returns. Maybe a runc bug?

runc does not implement 'creating' lifecycle ,
it is always 'created' or 'fails'.
From the spec, it should be like this:

user run runX create x

user run runX status x
it should be 'creating' (assuming the creating takes a long time).

runc does not implement 'creating' lifecycle...

That's definitely a bug. And there's also some CLI spec wording about create's exit timing. I think we can drop this, and I'll file a runc patch.

The runtime-spec defines a 'creating' status [1] and requires the 'create' operation to finish creating the container [2,3]. Our command line API also requires the 'create' command to block until creation completes: Callers MAY block on this command's successful exit to trigger post-create activity. runc does not support 'creating' yet [4], and it seems to return from 'create' before having quite finished (or we wouldn't have needed the code I'm removing in this commit). However, both of those are runc problems. These tests are about validating spec compliance, not about working around runc's issues, so remove the crutches. [1]: https://github.com/opencontainers/runtime-spec/blame/v1.0.1/runtime.md#L19 [2]: https://github.com/opencontainers/runtime-spec/blame/v1.0.1/runtime.md#L54 [3]: https://github.com/opencontainers/runtime-spec/blame/v1.0.1/runtime.md#L101 [4]: opencontainers#557 (comment) Signed-off-by: W. Trevor King <[email protected]>

liangchenye mentioned this pull request Jan 22, 2018

Complete the CLI args #556

Closed

4 tasks

liangchenye force-pushed the cli-complete branch from ae63341 to 56857d8 Compare January 22, 2018 10:05

liangchenye mentioned this pull request Jan 22, 2018

add lifecycle validation #558

Merged

liangchenye force-pushed the cli-complete branch from 56857d8 to b483dc1 Compare January 22, 2018 14:09

liangchenye force-pushed the cli-complete branch from b483dc1 to 78f41b5 Compare January 22, 2018 14:21

wking reviewed Jan 22, 2018

View reviewed changes

liangchenye force-pushed the cli-complete branch from 78f41b5 to 9df90d8 Compare January 23, 2018 11:28

liangchenye force-pushed the cli-complete branch from 9df90d8 to 08d0bd1 Compare January 23, 2018 11:52

liangchenye changed the title ~~WIP: Fix #556~~ Fix #556 Jan 23, 2018

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

wking reviewed Jan 24, 2018

View reviewed changes

liangchenye force-pushed the cli-complete branch from 08d0bd1 to a7f123b Compare January 24, 2018 07:36

Add lifecycle testing function; Add pidfile test.

5e8b51e

Fixes opencontainers#556 Signed-off-by: Liang Chenye <[email protected]>

liangchenye force-pushed the cli-complete branch from a7f123b to 5e8b51e Compare January 24, 2018 07:44

zhouhao3 merged commit 7ac117a into opencontainers:master Jan 25, 2018

wking reviewed Jan 25, 2018

View reviewed changes

wking mentioned this pull request Jan 25, 2018

validation: Remove runc 'create' exit timing crutches #563

Merged

wking mentioned this pull request Feb 8, 2018

runc does not implement runtime-spec's 'creating' status opencontainers/runc#1715

Open

Fix #556 #557

Fix #556 #557

Conversation

liangchenye commented Jan 22, 2018 • edited Loading

liangchenye commented Jan 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liangchenye commented Jan 23, 2018 • edited Loading

zhouhao3 commented Jan 24, 2018 • edited by caniszczyk Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liangchenye commented Jan 24, 2018

Mashimiao commented Jan 24, 2018 • edited by caniszczyk Loading

zhouhao3 commented Jan 25, 2018 • edited by caniszczyk Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liangchenye commented Jan 22, 2018 •

edited

Loading

liangchenye commented Jan 23, 2018 •

edited

Loading

zhouhao3 commented Jan 24, 2018 •

edited by caniszczyk

Loading

Mashimiao commented Jan 24, 2018 •

edited by caniszczyk

Loading

zhouhao3 commented Jan 25, 2018 •

edited by caniszczyk

Loading