pkg/pod: add WaitReady, dry Sandbox methods #3462

s-urbaniak · 2016-12-08T15:44:40Z

This adds a WaitReady method to be able to wait for pod readiness on the
command line.

It also reduces reduntant implementations in stage1 for checking mutable pods
and pod manifest update logic.

Fixes #3461

s-urbaniak · 2016-12-08T16:22:14Z

/cc @euank

lucab · 2016-12-08T16:55:26Z

It looks like there is a deadlock race.

s-urbaniak · 2016-12-08T17:03:59Z

ack :-/

s-urbaniak · 2016-12-08T17:09:31Z

I think I have to do refreshState in IsSupervisorReady and check if the pod is actually running.

s-urbaniak · 2016-12-09T10:45:18Z

instead of refreshing the state in IsSupervisorReady I refresh in WaitReady (as WaitExited does) in order to keep the current semantics that most of the state query methods do not synchronize the actual pod's filesystem state.

However I added some more documentation to the existing methods and the Pod struct itself to make it more explicit which methods (i.e. the state transitioning ones) and some query methods (ContainerPid1, Wait*) refresh the actual pod state.

lucab · 2016-12-09T16:31:17Z

pkg/pod/sandbox.go

+	if err != nil {
+		return err
+	}
+	defer f.Close()


From here on we have the file and we defer-closing on success; instead, on error, should we try to rm it?

lucab · 2016-12-09T16:59:41Z

pkg/pod/sandbox.go

 	if err != nil {
 		return err
 	}
+	defer tmpf.Close()


Same comment related to file removal.

euank · 2016-12-09T19:45:12Z

pkg/pod/wait.go

+// This method blocks indefinitely and refreshes the pod state.
+func (p *Pod) WaitExited() error {
+	// isExited implies isExitedGarbage.
+	for !p.isExited && !p.isAbortedPrepare && !p.isGarbage && !p.isGone {


similar to p.AfterRun, a helper of p.TerminalState could make this clearer..

Also, technically this isn't technically a "wait exited" because isAbortedPrepare is a pre-run, but terminal, state, so this is really WaitFinished or WaitTerminal

euank · 2016-12-09T19:46:48Z

pkg/pod/wait.go

+		}
+	}
+
+	// TODO(vc): return error or let caller detect the !p.isExited possibilities?


I would rather nix this TODO and just document in the function comment that the pod will be in a terminal state if there is no error, but it's up to the caller to determine what that state is.

euank · 2016-12-09T19:51:27Z

rkt/status.go

@@ -46,6 +48,7 @@ const (
 func init() {
 	cmdRkt.AddCommand(cmdStatus)
 	cmdStatus.Flags().BoolVar(&flagWait, "wait", false, "toggle waiting for the pod to exit")


"for the pod to reach a terminal state" is technically more correct; the caller should still ask status afterwards.

I'm not sure if there's a reason not to include a timeout here. I expect to kill the rkt process after a timeout if one isn't provided here regardless.

euank · 2016-12-09T19:53:46Z

rkt/status.go

@@ -46,6 +48,7 @@ const (
 func init() {
 	cmdRkt.AddCommand(cmdStatus)
 	cmdStatus.Flags().BoolVar(&flagWait, "wait", false, "toggle waiting for the pod to exit")
+	cmdStatus.Flags().DurationVar(&flagWaitReady, "wait-ready", time.Duration(0), "time to wait until the pod is ready. If time is less or equal zero it doesn't wait at all.")


I would expect '-1' to mean 'wait forever'.

euank · 2016-12-09T19:55:48Z

I realize I mostly commented on code that was just being moved, not written anew. This looks okay to me on a first pass.

This changes the rkt status --wait to accept a timeout rather than a boolean value. Addresses rkt#3462 (comment)

This changes the rkt status --wait to accept a timeout in addition to a boolean value. Addresses rkt#3462 (comment)

lucab

Only some typos and missing docs. Code and UX looks good.

lucab · 2016-12-12T13:42:09Z

pkg/pod/pods.go

 	return p.isExitedDeleting || p.isDeleting || p.isExited || p.isGarbage
 }

+// IsFinshed returns true if the pod is in a terminal state, else false.


IsFinished (typo: a missing "i")

lucab · 2016-12-12T13:54:49Z

rkt/status.go

-			stderr.PrintE("unable to wait for pod", err)
+	if dReady != 0 {
+		if err := p.WaitReady(newContext(dReady)); err != nil {
+			stderr.PrintE("error waiting for pod readyiness", err)


Spelling should be fixed in the whole PR, it is written "readiness".

lucab · 2016-12-12T13:55:52Z

rkt/status.go

 )

 var (
 	cmdStatus = &cobra.Command{
-		Use:   "status [--wait] UUID",
+		Use:   "status [--wait=bool|timeout] [--wait-ready=bool|timeout] UUID",


status.md should be updated with these changes.

This changes the rkt status --wait to accept a timeout in addition to a boolean value. Addresses rkt#3462 (comment)

s-urbaniak · 2016-12-12T15:53:42Z

@lucab addressed review comments, PTAL

euank · 2016-12-12T20:54:51Z

tests/rkt_app_sandbox_test.go

@@ -75,6 +75,7 @@ func TestAppSandboxSmoke(t *testing.T) {
 	cmd = strings.Fields(fmt.Sprintf("%s app add --debug %s %s --name=%s", ctx.Cmd(), podUUID, imageName, appName))
 	addCmd := exec.Command(cmd[0], cmd[1:]...)
 	addCmd.Env = append(addCmd.Env, "RKT_EXPERIMENT_APP=true")
+	fmt.Printf("Running command: %v\n", cmd)


nit: all these fmt.Printfs should be removed or changed to t.Logf

ah, I left it there, because this output is unbuffered, but I'll change it back to t.Logf.

euank · 2016-12-12T20:55:26Z

tests/rkt_tests.go

+	// wait up to one minute for the pod supervisor to be ready
+	cmd := strings.Fields(fmt.Sprintf("%s status --wait-ready=%s %s", ctx.Cmd(), timeout, podUUID))
+	statusCmd := exec.Command(cmd[0], cmd[1:]...)
+	fmt.Printf("Running command: %v\n", cmd)


Same for this file

euank · 2016-12-12T20:55:58Z

LGTM apart from the presumably leftover printfs

This changes the rkt status --wait to accept a timeout in addition to a boolean value. Addresses rkt#3462 (comment)

lucab

LGTM

This reduces reduntant implementations for checking mutable pod manifests and pod manifest update logic.

Currently it is unclear which methods in pkg/pod refresh the pod state and which don't. This fixes it by documenting it.

This adds a WaitReady method to be able to wait for pod readiness on the command line. Fixes rkt#3461

This is a minor cleanup in pkg/pod.

This changes the rkt status --wait to accept a timeout in addition to a boolean value. Addresses rkt#3462 (comment)

s-urbaniak · 2016-12-14T14:37:36Z

Merging, as the the rawhide failure seems to be a flake.

s-urbaniak force-pushed the waitReady branch 2 times, most recently from 7b8898d to 8e4e579 Compare December 8, 2016 16:22

s-urbaniak added this to the v1.22.0 milestone Dec 8, 2016

s-urbaniak force-pushed the waitReady branch from 8e4e579 to 51e57c0 Compare December 8, 2016 16:30

s-urbaniak force-pushed the waitReady branch from 51e57c0 to 4617b1d Compare December 9, 2016 10:38

s-urbaniak force-pushed the waitReady branch 2 times, most recently from aaff747 to e69031b Compare December 9, 2016 14:15

lucab reviewed Dec 9, 2016

View reviewed changes

euank reviewed Dec 9, 2016

View reviewed changes

s-urbaniak force-pushed the waitReady branch from e69031b to 56ee19a Compare December 12, 2016 11:19

s-urbaniak added a commit to s-urbaniak/rkt that referenced this pull request Dec 12, 2016

rkt/status: add timeout to --wait

56ee19a

This changes the rkt status --wait to accept a timeout rather than a boolean value. Addresses rkt#3462 (comment)

s-urbaniak force-pushed the waitReady branch from 56ee19a to 863db9c Compare December 12, 2016 12:40

s-urbaniak added a commit to s-urbaniak/rkt that referenced this pull request Dec 12, 2016

rkt/status: add timeout to --wait

863db9c

This changes the rkt status --wait to accept a timeout in addition to a boolean value. Addresses rkt#3462 (comment)

s-urbaniak force-pushed the waitReady branch from 863db9c to ac2930f Compare December 12, 2016 12:44

s-urbaniak added a commit to s-urbaniak/rkt that referenced this pull request Dec 12, 2016

rkt/status: add timeout to --wait

ac2930f

This changes the rkt status --wait to accept a timeout in addition to a boolean value. Addresses rkt#3462 (comment)

s-urbaniak force-pushed the waitReady branch from ac2930f to 6f0509a Compare December 12, 2016 12:45

s-urbaniak added a commit to s-urbaniak/rkt that referenced this pull request Dec 12, 2016

rkt/status: add timeout to --wait

6f0509a

This changes the rkt status --wait to accept a timeout in addition to a boolean value. Addresses rkt#3462 (comment)

s-urbaniak force-pushed the waitReady branch from 6f0509a to 2a2383e Compare December 12, 2016 12:49

s-urbaniak added a commit to s-urbaniak/rkt that referenced this pull request Dec 12, 2016

rkt/status: add timeout to --wait

2a2383e

This changes the rkt status --wait to accept a timeout in addition to a boolean value. Addresses rkt#3462 (comment)

lucab suggested changes Dec 12, 2016

View reviewed changes

s-urbaniak force-pushed the waitReady branch from 2a2383e to ad210f2 Compare December 12, 2016 15:22

s-urbaniak added a commit to s-urbaniak/rkt that referenced this pull request Dec 12, 2016

rkt/status: add timeout to --wait

ad210f2

This changes the rkt status --wait to accept a timeout in addition to a boolean value. Addresses rkt#3462 (comment)

euank reviewed Dec 12, 2016

View reviewed changes

s-urbaniak force-pushed the waitReady branch from ad210f2 to d19da4a Compare December 13, 2016 10:20

s-urbaniak added a commit to s-urbaniak/rkt that referenced this pull request Dec 13, 2016

rkt/status: add timeout to --wait

d19da4a

This changes the rkt status --wait to accept a timeout in addition to a boolean value. Addresses rkt#3462 (comment)

s-urbaniak force-pushed the waitReady branch from d19da4a to 2e0f80a Compare December 14, 2016 09:35

s-urbaniak added a commit to s-urbaniak/rkt that referenced this pull request Dec 14, 2016

rkt/status: add timeout to --wait

2e0f80a

This changes the rkt status --wait to accept a timeout in addition to a boolean value. Addresses rkt#3462 (comment)

lucab approved these changes Dec 14, 2016

View reviewed changes

lucab added component/stage0 component/stage1 kind/enhancement reviewed/lgtm reviewed/merge-on-green labels Dec 14, 2016

s-urbaniak added 6 commits December 14, 2016 13:31

pkg/pod: dry Sandbox methods

f0e3a28

This reduces reduntant implementations for checking mutable pod manifests and pod manifest update logic.

pkg/pod document semantics of pod methods

ca54f46

Currently it is unclear which methods in pkg/pod refresh the pod state and which don't. This fixes it by documenting it.

pkg/pod: add WaitReady

41000f3

This adds a WaitReady method to be able to wait for pod readiness on the command line. Fixes rkt#3461

tests/rkt_app_sandbox_test: add more debug output

d45f152

pkg/pod add IsFinished, rename AfterRun -> IsAfterRun

0f3dff5

This is a minor cleanup in pkg/pod.

rkt/status: add timeout to --wait

aaef5e3

This changes the rkt status --wait to accept a timeout in addition to a boolean value. Addresses rkt#3462 (comment)

s-urbaniak force-pushed the waitReady branch from 2e0f80a to aaef5e3 Compare December 14, 2016 12:31

s-urbaniak mentioned this pull request Dec 14, 2016

Flake: TestAppUserGroup #3477

Open

s-urbaniak merged commit 8fd6c41 into rkt:master Dec 14, 2016

lucab mentioned this pull request Jan 5, 2017

rkt 1.22.0 #3510

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pkg/pod: add WaitReady, dry Sandbox methods #3462

pkg/pod: add WaitReady, dry Sandbox methods #3462

s-urbaniak commented Dec 8, 2016

s-urbaniak commented Dec 8, 2016

lucab commented Dec 8, 2016

s-urbaniak commented Dec 8, 2016

s-urbaniak commented Dec 8, 2016

s-urbaniak commented Dec 9, 2016

lucab Dec 9, 2016

lucab Dec 9, 2016

euank Dec 9, 2016

euank Dec 9, 2016

euank Dec 9, 2016

euank Dec 9, 2016

euank commented Dec 9, 2016

lucab left a comment

lucab Dec 12, 2016

s-urbaniak Dec 12, 2016

lucab Dec 12, 2016

s-urbaniak Dec 12, 2016

lucab Dec 12, 2016

s-urbaniak Dec 12, 2016

s-urbaniak commented Dec 12, 2016

euank Dec 12, 2016

s-urbaniak Dec 13, 2016

euank Dec 12, 2016

euank commented Dec 12, 2016

lucab left a comment

s-urbaniak commented Dec 14, 2016

pkg/pod: add WaitReady, dry Sandbox methods #3462

pkg/pod: add WaitReady, dry Sandbox methods #3462

Conversation

s-urbaniak commented Dec 8, 2016

s-urbaniak commented Dec 8, 2016

lucab commented Dec 8, 2016

s-urbaniak commented Dec 8, 2016

s-urbaniak commented Dec 8, 2016

s-urbaniak commented Dec 9, 2016

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

euank commented Dec 9, 2016

lucab left a 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

s-urbaniak commented Dec 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

euank commented Dec 12, 2016

lucab left a comment

Choose a reason for hiding this comment

s-urbaniak commented Dec 14, 2016