Quadlet - add support for global arguments #20253

ygalblum · 2023-10-04T13:58:36Z

Add support for adding podman level arguments before subcommand Add specific key for Containers Conf Modules
Adjust testing environment
Add tests
Add to man page

Does this PR introduce a user-facing change?

Yes

Quadlet - add support for podman level arguments

Resolves: #20246

vrothberg · 2023-10-04T14:08:36Z

pkg/systemd/quadlet/quadlet.go

@@ -72,6 +73,7 @@ const (
 	KeyExec                  = "Exec"
 	KeyExitCodePropagation   = "ExitCodePropagation"
 	KeyExposeHostPort        = "ExposeHostPort"
+	KeyGlobalOptions         = "GlobalOptions"


We use PodmanArgs for flags to the command. To have a symmetric name, could name this GlobalArgs or RootArgs?

Since both you and @Luap99 suggested GlobalArgs, I went with that.

docs/source/markdown/podman-systemd.unit.5.md

vrothberg

That was super quick, thanks!

I think that both, --module and the other root flags need to be added to all invocations of Podman. That includes the ExecStop{Post} as well. Podman may be pointed to a completely different graph root, so the podman rms won't work without sharing the flags.

packit-as-a-service · 2023-10-04T14:12:03Z

Ephemeral COPR build failed. @containers/packit-build please check.

packit-as-a-service · 2023-10-04T14:12:04Z

Ephemeral COPR build failed. @containers/packit-build please check.

packit-as-a-service · 2023-10-04T14:36:06Z

Cockpit tests failed for commit c33bf3f2e51cf296dde886ea16322f3a91410623. @martinpitt, @jelly, @mvollmer please check.

TomSweeneyRedHat · 2023-10-04T18:50:40Z

All kinds of test unhappiness

lsm5 · 2023-10-04T19:35:14Z

FATAL: docs/source/markdown/podman-systemd.unit.5.md has a too-long table column; use 'man -l docs/build/man/podman-systemd.unit.5' and look for empty table cells.
make: *** [Makefile:498: docs/source/markdown/podman-systemd.unit.5] Error 1

ygalblum · 2023-10-05T07:11:46Z

That was super quick, thanks!

I think that both, --module and the other root flags need to be added to all invocations of Podman. That includes the ExecStop{Post} as well. Podman may be pointed to a completely different graph root, so the podman rms won't work without sharing the flags.

You're right, thanks. I've updated the code and tests accordingly

ygalblum · 2023-10-05T07:15:11Z

@TomSweeneyRedHat @lsm5 yes I've seen this error, though, I don't know how to fix it. It's not the first time this happens but I don't remember how I fixed it before as it was the result of trial-and-error cycles.

The issue is caused by the table line added for ContainersConfModule:

| ContainersConfModule=/etc/1.conf | --module=/etc/1.conf                                 |

So, I was wondering if you know what needs to change in order to fix this error.

packit-as-a-service · 2023-10-05T07:19:50Z

Ephemeral COPR build failed. @containers/packit-build please check.

vrothberg

To pass the docs build, the 1.conf needs to be escaped to 1\.conf. The error is quite misleading

vrothberg · 2023-10-05T09:44:43Z

To pass the docs build, the 1.conf needs to be escaped to 1\.conf. The error is quite misleading

Maybe replace it to nvidia\.conf?

ygalblum · 2023-10-05T09:59:09Z

@vrothberg Thanks for your help (and I've changed to nvidia\.conf as you suggested

docs/source/markdown/podman-systemd.unit.5.md

vrothberg

LGTM

vrothberg · 2023-10-05T12:34:25Z

Linter isn't happy yet

mheon · 2023-10-05T21:14:14Z

Code LGTM

vrothberg

/lgtm
/hold
Restarted the flaked jobs; transient errors on Quay.

Luap99 · 2023-10-06T09:15:05Z

pkg/systemd/quadlet/quadlet.go

-	service.Add(ServiceGroup, "ExecStopPost", "-"+podmanBinary()+" rm -v -f -i --cidfile=%t/%N.cid")
+	serviceStopCmd.Args = append([]string{"-"}, serviceStopCmd.Args...)


This change does not look right, it will result in - binary not the old (correct) -binary, notice the extra space.

The fact that none of the test catches that worries me, testing locally I see

Empty path in command line, ignoring: '- /usr/bin/podman rm -v -f -i --cidfile=%t/%N.cid'

as warning in the journal so it is not accepted by systemd.

Thanks for finding this.
The issue was in the regex being checked as it was not forcing non white space after the - sign. I've fixed it and the code.

Add support for adding podman level arguments before subcommand Add specific key for Containers Conf Modules Global arguments are added for both start and stop commands Adjust testing environment Add tests Add to man page Signed-off-by: Ygal Blum <[email protected]>

Luap99

LGTM

openshift-ci · 2023-10-09T08:58:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Luap99, vrothberg, ygalblum

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~~ [Luap99,vrothberg,ygalblum]

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

Luap99 · 2023-10-09T08:58:48Z

/lgtm
/hold cancel

openshift-ci bot added release-note approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 4, 2023

vrothberg reviewed Oct 4, 2023

View reviewed changes

Luap99 reviewed Oct 4, 2023

View reviewed changes

docs/source/markdown/podman-systemd.unit.5.md Show resolved Hide resolved

vrothberg reviewed Oct 4, 2023

View reviewed changes

ygalblum force-pushed the quadlet-command-args branch from c33bf3f to addefec Compare October 5, 2023 07:10

vrothberg reviewed Oct 5, 2023

View reviewed changes

ygalblum force-pushed the quadlet-command-args branch 2 times, most recently from e07ce99 to 752c9c6 Compare October 5, 2023 09:58

vrothberg reviewed Oct 5, 2023

View reviewed changes

docs/source/markdown/podman-systemd.unit.5.md Outdated Show resolved Hide resolved

ygalblum force-pushed the quadlet-command-args branch from 752c9c6 to 515277b Compare October 5, 2023 11:36

vrothberg approved these changes Oct 5, 2023

View reviewed changes

vrothberg mentioned this pull request Oct 5, 2023

Please support --module feature in quadlet #20246

Closed

ygalblum force-pushed the quadlet-command-args branch from 515277b to 941337e Compare October 5, 2023 14:56

vrothberg approved these changes Oct 6, 2023

View reviewed changes

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 6, 2023

openshift-ci bot assigned vrothberg Oct 6, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 6, 2023

Luap99 requested changes Oct 6, 2023

View reviewed changes

Luap99 removed the lgtm Indicates that a PR is ready to be merged. label Oct 6, 2023

ygalblum force-pushed the quadlet-command-args branch from 941337e to d321d42 Compare October 8, 2023 07:14

Luap99 approved these changes Oct 9, 2023

View reviewed changes

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 9, 2023

openshift-ci bot assigned Luap99 Oct 9, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 9, 2023

openshift-ci bot merged commit 6e7e875 into containers:main Oct 9, 2023
95 of 97 checks passed

ygalblum deleted the quadlet-command-args branch October 9, 2023 11:56

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 Jan 8, 2024

github-actions bot locked as resolved and limited conversation to collaborators Jan 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Quadlet - add support for global arguments #20253

Quadlet - add support for global arguments #20253

ygalblum commented Oct 4, 2023

vrothberg Oct 4, 2023

ygalblum Oct 5, 2023

vrothberg left a comment

packit-as-a-service bot commented Oct 4, 2023

packit-as-a-service bot commented Oct 4, 2023

packit-as-a-service bot commented Oct 4, 2023

TomSweeneyRedHat commented Oct 4, 2023

lsm5 commented Oct 4, 2023

ygalblum commented Oct 5, 2023

ygalblum commented Oct 5, 2023 •

edited

Loading

packit-as-a-service bot commented Oct 5, 2023

vrothberg left a comment

vrothberg commented Oct 5, 2023

ygalblum commented Oct 5, 2023

vrothberg left a comment

vrothberg commented Oct 5, 2023

mheon commented Oct 5, 2023

vrothberg left a comment

Luap99 Oct 6, 2023

ygalblum Oct 8, 2023

Luap99 left a comment

openshift-ci bot commented Oct 9, 2023

Luap99 commented Oct 9, 2023

		service.Add(ServiceGroup, "ExecStopPost", "-"+podmanBinary()+" rm -v -f -i --cidfile=%t/%N.cid")
		serviceStopCmd.Args = append([]string{"-"}, serviceStopCmd.Args...)

Quadlet - add support for global arguments #20253

Quadlet - add support for global arguments #20253

Conversation

ygalblum commented Oct 4, 2023

Does this PR introduce a user-facing change?

vrothberg Oct 4, 2023

Choose a reason for hiding this comment

ygalblum Oct 5, 2023

Choose a reason for hiding this comment

vrothberg left a comment

Choose a reason for hiding this comment

packit-as-a-service bot commented Oct 4, 2023

packit-as-a-service bot commented Oct 4, 2023

packit-as-a-service bot commented Oct 4, 2023

TomSweeneyRedHat commented Oct 4, 2023

lsm5 commented Oct 4, 2023

ygalblum commented Oct 5, 2023

ygalblum commented Oct 5, 2023 • edited Loading

packit-as-a-service bot commented Oct 5, 2023

vrothberg left a comment

Choose a reason for hiding this comment

vrothberg commented Oct 5, 2023

ygalblum commented Oct 5, 2023

vrothberg left a comment

Choose a reason for hiding this comment

vrothberg commented Oct 5, 2023

mheon commented Oct 5, 2023

vrothberg left a comment

Choose a reason for hiding this comment

Luap99 Oct 6, 2023

Choose a reason for hiding this comment

ygalblum Oct 8, 2023

Choose a reason for hiding this comment

Luap99 left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Oct 9, 2023

Luap99 commented Oct 9, 2023

ygalblum commented Oct 5, 2023 •

edited

Loading