Recursively start a pod if a container is run in it #2295

haircommander · 2019-02-08T21:55:12Z

Prior, a pod would have to be started immediately when created, leading to confusion about what a pod state should be immediately after creation. The problem was podman run --pod ... would error out if the infra container wasn't started (as it is a dependency). Fix this by allowing for recursive start, where each of the container's dependencies are started prior to the new container. Only apply this when a container is run in a "created" pod

Also rework container_api Start, StartAndAttach, and Init functions, as there was some duplicated code, which made addressing the --recursive problem easier to fix.

Signed-off-by: Peter Hunt pehunt@redhat.com

rhatdan · 2019-02-09T12:16:15Z

I don't like the idea that this is an option. If this is something a user would expect then it should just happen. If I start a container which is in a pod, then the pod should start. I would rather we spend our time figuring out what happens when the container exits. It should be one of three things

1: Pod stops
2: Pod restarts ontainer
3: Nothing

haircommander · 2019-02-11T14:51:21Z

@rhatdan We can talk about the stop case. In the interest of resolving the problems prompting #2291 I have updated this PR to default to a recursive start when a new container is run in a pod (podman run --pod ...) All other cases recursive start has been disabled, but there's infrastructure to allow it if that is desired in the future. I still need to add tests, but PTAL

haircommander · 2019-02-11T16:13:23Z

@mheon @rhatdan Okay PR should be ready PTAL

mheon · 2019-02-11T18:35:46Z

bot, retest this please

mheon · 2019-02-11T18:37:48Z

libpod/container_api.go

@@ -74,7 +65,8 @@ func (c *Container) Init(ctx context.Context) (err error) {
 // started
 // Stopped containers will be deleted and re-created in runc, undergoing a fresh
 // Init()
-func (c *Container) Start(ctx context.Context) (err error) {
+// if recursive is set, Start will also start all containers this ctr depends on


Can you add periods at the end of all the sentences in this comment? I think godoc will mangle the newlines, so we need periods to preserve sentence flow.

Also, can you capitalize the if at the beginning here?

mheon · 2019-02-11T18:38:01Z

libpod/container_api.go

+// attach call.
+// The channel will be closed automatically after the result of attach has been
+// sent
+// if recursive is set, StartAndAttach will also start all containers this ctr depends on


Same as above - capitalize the if, add periods

mheon · 2019-02-11T18:39:21Z

libpod/container_api.go

+}
+
+// helper function that handles dependencies and prints a helpful message
+func (c *Container) checkDependenciesAndHandleError(ctx context.Context) error {


Helper methods into container_internal.go please - we try to keep only public API stuff in container_api.go

mheon · 2019-02-11T18:40:08Z

libpod/container_api.go

-		// Or initialize it if necessary
-		if err := c.init(ctx); err != nil {
-			return nil, err
+	depCtrs := make([]*Container, 0) //, len(depCtrIDs))


Why the commented-out bit?

mheon · 2019-02-11T18:42:13Z

libpod/container_api.go

+	}
+
+	if len(ctrErrors) > 0 {
+		return errors.Wrapf(ErrCtrExists, "error stopping some containers")


We should make some sort of attempt to get the errors out here - maybe logrus.Errorf them and then return?

Also, swap ErrCtrExists to... probably ErrInternal is better?

mheon · 2019-02-11T18:43:29Z

test/e2e/e2e.coverprofile

@@ -0,0 +1,11 @@
+mode: atomic


Do we need to .gitignore this file?

apparently, will add

mheon · 2019-02-11T18:45:34Z

libpod/container_api.go

 	}

-	attachChan := make(chan error)
+	// Build a dependency graph of containers
+	graph, err := buildContainerGraph(depCtrs)


I don't know how well this works if the dependencies have dependencies - it was originally written assuming every container it would ever touch would be passed in as part of the []*Container

ah shoot yeah I forgot to mention that. I haven't tested myself but I'm pretty sure things would get funky. Looking into better solution...

haircommander · 2019-02-11T20:14:16Z

@mheon updated for comments. PTAL now (especially https://github.com/containers/libpod/blob/af5a41a3d7b4fd69480214102956ebc399585d12/libpod/container_internal.go#L669 which is the meat of the changes I made)

mheon · 2019-02-11T20:22:27Z

libpod/container_internal.go

+// in looking up dependencies.
+// Note: this function is currently meant as a robust solution to a narrow problem: start an infra-container when
+// a container in the pod is run. It has not been tested for performance past one level, so expansion of recursive start
+// must be tested first.


I'm fundamentally OK with recursive start being really slow when people make absurd dependency chains. Our primary target is infra containers, with one, maybe two levels of dependencies. Someone will have to show a reaaaally convincing usecase for me to think that anything more than 2 levels is desirable. We need to support these configurations, but I don't intend to optimize for them.

Should the note be removed then? It feels worth mentioning

currently recursive start is only allowed in the infra case, and only for a podman run

Eh, leave the note. If anyone uses the API and hits this case, they'll have an explanation for what's going on.

I don't see this as recursive, I don't even understand what recusive means in this case. This is simply means if I start a container in a POD and the pod is not running, then start the pod rather then the container. Then the container will get started as part of the pod. If the pod is joining a container then it will start the pod and join the container to the pod.

This isn't an actual pod start - we're hitting the container's dependencies, and starting the bare minimum this container needs to come up.

haircommander · 2019-02-11T21:41:43Z

tests are happy @rhatdan @baude PTAL :)

rh-atomic-bot · 2019-02-13T21:57:48Z

☔ The latest upstream changes (presumably #2319) made this pull request unmergeable. Please resolve the merge conflicts.

rhatdan · 2019-02-12T13:53:48Z

cmd/podman/run.go

@@ -70,10 +70,17 @@ func runCmd(c *cliconfig.RunValues) error {
 		}
 	}

+	var recursive bool
+	if c.IsSet("pod") {


Why not just pass c.IsSet(pod) to the function,

Or if you want the variable simpley right
recursive=c.IsSet("pod")

rhatdan · 2019-02-12T13:54:44Z

cmd/podman/start.go

@@ -108,7 +108,7 @@ func startCmd(c *cliconfig.StartValues) error {
 			}

 			// attach to the container and also start it not already running
-			err = startAttachCtr(ctr, os.Stdout, os.Stderr, inputStream, c.DetachKeys, sigProxy, !ctrRunning)
+			err = startAttachCtr(ctr, os.Stdout, os.Stderr, inputStream, c.DetachKeys, sigProxy, !ctrRunning, false)


I think you should check if the container is in a pod and then set this to true?

My thinking is podman pod start should be used in this case, so it should error out if the pod isn't started. The advantage of setting all of this up for run is the container is supposed to be created, then started in one command, so there isn't time to run pod start in between. adding this functionality to container start and having the infra container start makes less sense to me, and also requires a query to the state that otherwise isn't needed.

@mheon concurrent commenting makes this confusing. Who are you agreeing with?

@haircommander I think it's sensible to recursively start here if the container is in a pod

@mheon @rhatdan fixed. Also made the change for podman attach, but opted to not change play_kube

Agree with play kube, it should be doing ordered start already.

(And, honestly, if it fails, it's because the dependency containers just don't exist yet, nevermind they aren't started)

rhatdan · 2019-02-12T13:54:55Z

cmd/podman/start.go

@@ -130,7 +130,7 @@ func startCmd(c *cliconfig.StartValues) error {
 			continue
 		}
 		// Handle non-attach start
-		if err := ctr.Start(ctx); err != nil {
+		if err := ctr.Start(ctx, false); err != nil {


I think you should check if the container is in a pod and then set this to true?

see comment above^

rhatdan · 2019-02-12T13:55:48Z

cmd/podman/utils.go

@@ -20,7 +20,7 @@ type RawTtyFormatter struct {
 }

 // Start (if required) and attach to a container
-func startAttachCtr(ctr *libpod.Container, stdout, stderr, stdin *os.File, detachKeys string, sigProxy bool, startContainer bool) error {
+func startAttachCtr(ctr *libpod.Container, stdout, stderr, stdin *os.File, detachKeys string, sigProxy bool, startContainer bool, recursive bool) error {


I am not crazy about the term recursive. Really this is just saying startPod correct?

Negative - this is an actual recursive start. We don't hit the entire pod, just the dependencies of the specific container in question.

I called it so for extendability (if we actually want a recursive start independent of pods). I guess I can change it for now though.

Leave it as-is - we're not actually starting pods

ah yeah that's good reasoning to keep it; I like that @mheon

rhatdan · 2019-02-12T13:57:55Z

docs/podman-run.1.md

@@ -449,6 +449,7 @@ Tune the container's pids limit. Set `-1` to have unlimited pids for the contain

 Run container in an existing pod. If you want podman to make the pod for you, preference the pod name with `new:`.
 To make a pod with more granular options, use the `podman pod create` command before creating a container.
+If a container is run with a pod, and the pod has an infra-container, the infra-container will be started before the container is.


Do any pods not have an infra-container?
Wouldn't this be more understandable for normal users if the sentence said:

If a container is run with a pod, the entire pod will be started if it was not already running.

Podman pod allows you to skip making one

And we don't hit the entire pod, just our dependencies - probably the infra container

yeah pods have an infra container by default, but they don't need to have one.

rhatdan · 2019-02-12T13:59:49Z

libpod/container_api.go

@@ -205,6 +120,24 @@ func (c *Container) StartAndAttach(ctx context.Context, streams *AttachStreams,
 	return attachChan, nil
 }

+// RestartWithTimeout restarts a running container and takes a given timeout in uint


Why did you move this function?

move it closer to the other functions that have to do with starting containers

rhatdan · 2019-02-12T14:00:59Z

libpod/container_graph.go

@@ -169,3 +172,97 @@ func detectCycles(graph *containerGraph) (bool, error) {

 	return false, nil
 }
+
+// Visit a node on a container graph and start the container, or set an error if


Do we need all of this code if we simply checked if a conainer is in a pod and the pod is not running, then we start the pod?

I made it generalized for other cases where starting a containers dependencies with the container makes sense. Otherwise it feels like a hack to me

rhatdan · 2019-02-12T14:02:48Z

libpod/container_internal.go

+// in looking up dependencies.
+// Note: this function is currently meant as a robust solution to a narrow problem: start an infra-container when
+// a container in the pod is run. It has not been tested for performance past one level, so expansion of recursive start
+// must be tested first.


I don't see this as recursive, I don't even understand what recusive means in this case. This is simply means if I start a container in a POD and the pod is not running, then start the pod rather then the container. Then the container will get started as part of the pod. If the pod is joining a container then it will start the pod and join the container to the pod.

haircommander · 2019-02-15T18:23:50Z

/retest

rh-atomic-bot · 2019-02-15T21:07:13Z

☔ The latest upstream changes (presumably #2335) made this pull request unmergeable. Please resolve the merge conflicts.

…arted, or attached. Prior, a pod would have to be started immediately when created, leading to confusion about what a pod state should be immediately after creation. The problem was podman run --pod ... would error out if the infra container wasn't started (as it is a dependency). Fix this by allowing for recursive start, where each of the container's dependencies are started prior to the new container. This is only applied to the case where a new container is attached to a pod. Also rework container_api Start, StartAndAttach, and Init functions, as there was some duplicated code, which made addressing the problem easier to fix. Signed-off-by: Peter Hunt <pehunt@redhat.com>

rhatdan · 2019-02-17T12:19:56Z

LGTM
@baude @mheon @vrothberg @giuseppe PTAL

mheon · 2019-02-17T17:18:26Z

/lgtm

rhatdan · 2019-02-17T18:54:09Z

/approve

openshift-ci-robot · 2019-02-17T18:54:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: haircommander, rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [rhatdan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

rhatdan · 2019-02-17T19:50:16Z

/lgtm

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 8, 2019

openshift-ci-robot requested review from giuseppe and mrunalp February 8, 2019 21:55

openshift-ci-robot added the size/L label Feb 8, 2019

haircommander force-pushed the recursive-start branch from a743223 to 51b976a Compare February 8, 2019 21:55

haircommander force-pushed the recursive-start branch from 51b976a to c195330 Compare February 11, 2019 14:47

haircommander force-pushed the recursive-start branch 2 times, most recently from 2e6062c to 108488a Compare February 11, 2019 16:11

haircommander changed the title ~~WIP Added --recursive option to run and start~~ Recursively start a pod if a container is run in it Feb 11, 2019

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 11, 2019

haircommander force-pushed the recursive-start branch from 108488a to 7441a70 Compare February 11, 2019 16:22

mheon reviewed Feb 11, 2019

View reviewed changes

mheon mentioned this pull request Feb 11, 2019

Update Pod Create help and man pages #2291

Closed

haircommander force-pushed the recursive-start branch from 7441a70 to af5a41a Compare February 11, 2019 20:11

openshift-ci-robot added size/XL and removed size/L labels Feb 11, 2019

haircommander force-pushed the recursive-start branch from af5a41a to eb685d6 Compare February 11, 2019 20:17

mheon reviewed Feb 11, 2019

View reviewed changes

haircommander force-pushed the recursive-start branch from eb685d6 to 6e91631 Compare February 14, 2019 18:19

rhatdan reviewed Feb 15, 2019

View reviewed changes

haircommander force-pushed the recursive-start branch from 6e91631 to ed548e4 Compare February 15, 2019 15:09

haircommander force-pushed the recursive-start branch from ed548e4 to c92e5c9 Compare February 15, 2019 19:36

haircommander force-pushed the recursive-start branch from c92e5c9 to 81804fc Compare February 15, 2019 21:39

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 17, 2019

openshift-ci-robot assigned rhatdan Feb 17, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 17, 2019

openshift-merge-robot merged commit 6aaf8d3 into containers:master Feb 17, 2019

rh-atomic-bot mentioned this pull request Feb 17, 2019

OpenTracing First Impl #1692

Merged

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 27, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 27, 2023

Recursively start a pod if a container is run in it #2295

Recursively start a pod if a container is run in it #2295

Conversation

haircommander commented Feb 8, 2019 • edited Loading

rhatdan commented Feb 9, 2019

haircommander commented Feb 11, 2019

haircommander commented Feb 11, 2019

mheon commented Feb 11, 2019

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

haircommander commented Feb 11, 2019

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

haircommander commented Feb 11, 2019

rh-atomic-bot commented Feb 13, 2019

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

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

haircommander commented Feb 15, 2019

rh-atomic-bot commented Feb 15, 2019

rhatdan commented Feb 17, 2019

mheon commented Feb 17, 2019

rhatdan commented Feb 17, 2019

openshift-ci-robot commented Feb 17, 2019

rhatdan commented Feb 17, 2019

haircommander commented Feb 8, 2019 •

edited

Loading