podman-generate-systemd --new for pods #6415

vrothberg · 2020-05-28T11:36:21Z

This PR turned out to be really big, sorry folks. Supporting the --new flag for pods required to first do the ground work in container and pod configs and to wire in flags to store and load pod ID in podman pod and podman container subcommands.

After the ground work, I suffered from the sins of my past: the system-generation code was scattered across multiple packages and really complex. Both made it impossible for me to extend the code, so I started restructuring and refactoring the code into something usable and maintainable.

Finally, the --new flag is applied to pods.

vrothberg · 2020-05-28T11:43:29Z

@baude @giuseppe @mheon @rhatdan PTAL

vrothberg · 2020-05-28T12:15:46Z

quay.io is down...let's wait a bit

test/e2e/pod_stop_test.go

cmd/podman/parse/common.go

vrothberg · 2020-05-29T07:24:34Z

Moving to WIP as I'll start working on the generator part now.

vrothberg · 2020-05-29T11:26:04Z

... and more plumbing:

container-{create,run}: add --pod-id-file
pod create: add --infra-conmon-pidfile

vrothberg · 2020-06-05T17:13:57Z

@baude FYI, the refactoring is done. Now I can merge the --new pod logic.

libpod/pod.go

jdoss · 2020-06-09T04:12:36Z

@vrothberg I am very much looking forward to this getting merged! Thanks for your hard work! Once things are ready and some RPMs are made, I would be glad to test and give it Bodhi karma.

vrothberg · 2020-06-09T08:34:36Z

@vrothberg I am very much looking forward to this getting merged! Thanks for your hard work! Once things are ready and some RPMs are made, I would be glad to test and give it Bodhi karma.

Thanks for the kind words, @jdoss, a really nice motivation!

vrothberg · 2020-06-09T11:31:47Z

Here's the first working state for --new $POD. I'll now work on tests and may look into further refactorings.

vrothberg · 2020-06-09T12:29:26Z

@edsantiago @giuseppe @jwhonce @mheon @rhatdan @TomSweeneyRedHat PTAL

It's big, so I suggest going through the commits one-by-one.

vrothberg · 2020-06-09T13:27:59Z

Curious:

Uploading 3 artifacts for /var/tmp/go/src/github.com/containers/libpod/*.log.html
Uploaded /var/tmp/go/src/github.com/containers/libpod/apiv2_test.log.html
Failed to upload artifact file /var/tmp/go/src/github.com/containers/libpod/integration_test.log.html: EOF

cmd/podman/parse/common.go

rhatdan · 2020-06-10T20:59:22Z

@mheon @baude @giuseppe @TomSweeneyRedHat @QiWang19 @ashley-cui @jwhonce PTAL

edsantiago · 2020-06-10T21:00:51Z

test/e2e/pod_start_test.go

+		// Create a pod with --infra-conmon-pid.
+		session := podmanTest.Podman([]string{"pod", "create", "--name", podName, "--infra-conmon-pidfile", tmpFile})
+		session.WaitWithDefaultTimeout()
+		Expect(session.ExitCode()).To(Equal(0))


Could you test that tmpFile gets created, read it, confirm that it's a number, and check /proc/RESULT/exe or /proc/RESULT/cmdline to confirm that it's conmon? Or perhaps just write this as a BATS test instead? I despair at the number of ginkgo tests that do this: merely check exit status without actually testing anything.

Thanks a lot for catching that! I updated the e2e test as suggested 👍

Add a `CreateCommand` field to the pod config which includes the entire `os.Args` at pod-creation. Similar to the already existing field in a container config, we need this information to properly generate generic systemd unit files for pods. It's a prerequisite to support the `--new` flag for pods. Also add the `CreateCommand` to the pod-inspect data, which can come in handy for debugging, general inspection and certainly for the tests that are added along with the other changes. Signed-off-by: Valentin Rothberg <rothberg@redhat.com>

Signed-off-by: Valentin Rothberg <rothberg@redhat.com>

Support the `--pod-id-file` flag in the rm, start and stop pod commands. This completes the already support flag in pod-create and is another prerequisite for generating generic systemd unit files for pods. Also add completions, docs and tests. Signed-off-by: Valentin Rothberg <rothberg@redhat.com>

Allow containers to join an existing pod via the `--pod-id-file` which is already supported by a number of `podman-pod` subcommands. Also add tests to make sure it's working and to prevent future regressions. Signed-off-by: Valentin Rothberg <rothberg@redhat.com>

Rename the container ID file from "cid" to "ctr-id" to make the generated unit files a) easier to read and to b) pro-actively avoid any confusion when pod ID files are being added in the future. Signed-off-by: Valentin Rothberg <rothberg@redhat.com>

Add an `--infra-conmon-pidfile` flag to `podman-pod-create` to write the infra container's conmon process ID to a specified path. Several container sub-commands already support `--conmon-pidfile` which is especially helpful to allow for systemd to access and track the conmon processes. This allows for easily tracking the conmon process of a pod's infra container. Signed-off-by: Valentin Rothberg <rothberg@redhat.com>

Rephrase the lookup error when the specified name or ID does not refer to a container or pod. Until, only the pod-lookup error has been returned which can be confusing when actually looking for a container; a user might have just mistyped the ID or name. Signed-off-by: Valentin Rothberg <rothberg@redhat.com>

Rename to `containers{_test}.go` to make some place for the upcoming pod changes. Signed-off-by: Valentin Rothberg <rothberg@redhat.com>

Add a method to Pod to easily access its .config.CreateCommand. Signed-off-by: Valentin Rothberg <rothberg@redhat.com>

Refactor the systemd-unit generation code and move all the logic into `pkg/systemd/generate`. The code was already hard to maintain but I found it impossible to wire the `--new` logic for pods in all the chaos. The code refactoring in this commit will make maintaining the code easier and should make it easier to extend as well. Further changes and refactorings may still be needed but they will easier. Signed-off-by: Valentin Rothberg <rothberg@redhat.com>

Create a new template for generating a pod unit file. Eventually, this allows for treating and extending pod and container generation seprately. The `--new` flag now also works on pods. Signed-off-by: Valentin Rothberg <rothberg@redhat.com>

Signed-off-by: Valentin Rothberg <rothberg@redhat.com>

giuseppe

LGTM

openshift-ci-robot · 2020-06-11T12:16:50Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe, rhatdan, vrothberg

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~~ [giuseppe,rhatdan,vrothberg]

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

rhatdan · 2020-06-11T12:49:50Z

/lgtm
/hold

vrothberg · 2020-06-11T14:52:53Z

/hold cancel

openshift-ci-robot requested review from mheon and umohnani8 May 28, 2020 11:36

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 28, 2020

mheon reviewed May 28, 2020

View reviewed changes

test/e2e/pod_stop_test.go Outdated Show resolved Hide resolved

TomSweeneyRedHat reviewed May 28, 2020

View reviewed changes

cmd/podman/parse/common.go Outdated Show resolved Hide resolved

vrothberg force-pushed the systemd-new-pod branch 2 times, most recently from 1f07bb4 to 3bf4b51 Compare May 29, 2020 06:38

vrothberg changed the title ~~pod commands preparation for generic systemd unit files~~ WIP - pod commands preparation for generic systemd unit files May 29, 2020

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 May 29, 2020

vrothberg force-pushed the systemd-new-pod branch from 198af9f to 76e39de Compare May 29, 2020 11:40

vrothberg force-pushed the systemd-new-pod branch from 76e39de to 83642cb Compare June 5, 2020 17:09

vrothberg commented Jun 5, 2020

View reviewed changes

libpod/pod.go Show resolved Hide resolved

mheon reviewed Jun 5, 2020

View reviewed changes

libpod/pod.go Outdated Show resolved Hide resolved

vrothberg force-pushed the systemd-new-pod branch from 83642cb to 98f160d Compare June 9, 2020 11:30

vrothberg force-pushed the systemd-new-pod branch 2 times, most recently from ba39780 to 91a74eb Compare June 9, 2020 12:21

vrothberg changed the title ~~WIP - pod commands preparation for generic systemd unit files~~ podman-generate-systemd --new for pods Jun 9, 2020

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 Jun 9, 2020

vrothberg force-pushed the systemd-new-pod branch from 91a74eb to 0862783 Compare June 9, 2020 13:30

edsantiago reviewed Jun 9, 2020

View reviewed changes

cmd/podman/parse/common.go Outdated Show resolved Hide resolved

edsantiago reviewed Jun 10, 2020

View reviewed changes

vrothberg added 14 commits June 11, 2020 11:01

systemd/generate: remove unused infra container field

7f5aabb

Signed-off-by: Valentin Rothberg <rothberg@redhat.com>

generate systemd: rename source files

557a9c1

Rename to `containers{_test}.go` to make some place for the upcoming pod changes. Signed-off-by: Valentin Rothberg <rothberg@redhat.com>

add (*Pod).CreateCommand()

b4a4102

Add a method to Pod to easily access its .config.CreateCommand. Signed-off-by: Valentin Rothberg <rothberg@redhat.com>

generate systemd: create pod template

8d8746a

Create a new template for generating a pod unit file. Eventually, this allows for treating and extending pod and container generation seprately. The `--new` flag now also works on pods. Signed-off-by: Valentin Rothberg <rothberg@redhat.com>

docs: create/run fix --pod-id-file description

139f829

Signed-off-by: Valentin Rothberg <rothberg@redhat.com>

generate systemd: wrap pod/ctr lookup errors

05713fb

Signed-off-by: Valentin Rothberg <rothberg@redhat.com>

e2e: sanity check --infra-conmon-pidfile

c7c81a8

Signed-off-by: Valentin Rothberg <rothberg@redhat.com>

vrothberg force-pushed the systemd-new-pod branch from 33ddf8e to c7c81a8 Compare June 11, 2020 09:02

giuseppe approved these changes Jun 11, 2020

View reviewed changes

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 11, 2020

openshift-ci-robot assigned rhatdan Jun 11, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 11, 2020

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 11, 2020

openshift-merge-robot merged commit 39ad038 into containers:master Jun 11, 2020

jdoss mentioned this pull request Jun 11, 2020

Rootless podman via systemd with User directive does not work #6582

Closed

edsantiago mentioned this pull request Aug 19, 2020

CI flake: healthcheck command exceeded timeout of 3s #7378

Closed

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 24, 2023

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

podman-generate-systemd --new for pods #6415

podman-generate-systemd --new for pods #6415

vrothberg commented May 28, 2020 •

edited

Loading

vrothberg commented May 28, 2020

vrothberg commented May 28, 2020

vrothberg commented May 29, 2020

vrothberg commented May 29, 2020

vrothberg commented Jun 5, 2020

jdoss commented Jun 9, 2020

vrothberg commented Jun 9, 2020

vrothberg commented Jun 9, 2020

vrothberg commented Jun 9, 2020

vrothberg commented Jun 9, 2020

rhatdan commented Jun 10, 2020

edsantiago Jun 10, 2020

vrothberg Jun 11, 2020

giuseppe left a comment

openshift-ci-robot commented Jun 11, 2020

rhatdan commented Jun 11, 2020

vrothberg commented Jun 11, 2020

podman-generate-systemd --new for pods #6415

podman-generate-systemd --new for pods #6415

Conversation

vrothberg commented May 28, 2020 • edited Loading

vrothberg commented May 28, 2020

vrothberg commented May 28, 2020

vrothberg commented May 29, 2020

vrothberg commented May 29, 2020

vrothberg commented Jun 5, 2020

jdoss commented Jun 9, 2020

vrothberg commented Jun 9, 2020

vrothberg commented Jun 9, 2020

vrothberg commented Jun 9, 2020

vrothberg commented Jun 9, 2020

rhatdan commented Jun 10, 2020

edsantiago Jun 10, 2020

Choose a reason for hiding this comment

vrothberg Jun 11, 2020

Choose a reason for hiding this comment

giuseppe left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Jun 11, 2020

rhatdan commented Jun 11, 2020

vrothberg commented Jun 11, 2020

vrothberg commented May 28, 2020 •

edited

Loading