Allow removing implicit quadlet systemd dependencies #24194

lambinoo · 2024-10-07T18:54:21Z

Allow removing implicit quadlet systemd dependencies

Quadlet inserts network-online.target Wants/After dependencies to ensure pulling works.
Those systemd statements cannot be subsequently reset.

In the cases where those dependencies are not wanted, we add a new
configuration item called DefaultDependencies= in a new section called
[Quadlet]. This section is shared between different unit types.

fixes #24193

Adds a directive in a new [Quadlet] section to prevent quadlet from adding implicit dependencies on the network-online target.

packit-as-a-service · 2024-10-07T19:00:31Z

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

ygalblum · 2024-10-07T19:29:48Z

The code looks good. But, you will need to add some tests

lambinoo · 2024-10-07T19:34:10Z

The code looks good. But, you will need to add some tests

Would an e2e test be enough for this?

mheon · 2024-10-07T20:25:23Z

Sure. We generally want an E2E or system test, adding both is usually unnecessary.

lambinoo · 2024-10-07T20:28:19Z

Should be good now I think

vrothberg

LGTM, nice work.

Luap99 · 2024-10-08T09:17:35Z

pkg/systemd/quadlet/quadlet.go

-	// override the default ones.
-	service.PrependUnitLine(UnitGroup, "After", "network-online.target")
-	service.PrependUnitLine(UnitGroup, "Wants", "network-online.target")
+	if service.LookupBooleanWithDefault(ContainerGroup, KeyQuadletDefaultDependencies, true) {


Do we really want to add these generic keys to each group? Overall this just adds a ton of duplication in the docs.
And every time we add such a generic option we have to do that over and over again which does not look sane to me.

So maybe is it time to define a [Quadlet] section and then just name the key DefaultDependencies under there? This would only need to be documented once and then should work for all units. @ygalblum WDYT?

I had a similar thought but didn't consider the long-term impact on new features. +1 from my side.

I agree and I was actually thinking about it for a while now. For example, I would move ContainersConfModule to there as well (I know, for backward compatibility we'll need to support both)
I'm just not sure we want to force @lambinoo to make this change.
Let's hold this PR for a sec and I'll try to find some time to add this section

I don't mind. Just pushed (what I think are) the required changes for this!
Left the ContainersConfModule bit out of this

ygalblum

Additional question:
The new Quadlet section is currently only relevant for .container, .build and .image files. What should be the behaviour if it appears on a different types? I see three options:

Rename the group to X-Quadlet and noop
Error at the Quadlet level
Noop which will result in a systemd error because of an unrecognized section

I think option 1 is preferable as it will also be future proof.

ygalblum · 2024-10-08T15:29:36Z

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

+
+| **[Quadlet] options**      | **Description**                                   |
+|----------------------------|---------------------------------------------------|
+| DefaultDependencies=false  | Include implicit network dependencies to the unit |


The example sets the value to false but the description refers to the case of true. This might be a bit confusing

Ooops, updated!

Something got reverted here and the original issue is back. I'll take advantage and say that since setting it to true is redundant, it might be better to leave the false value and change the description to something like "Disable implicit network..."

pkg/systemd/quadlet/quadlet.go

lambinoo · 2024-10-08T15:49:50Z

Additional question: The new Quadlet section is currently only relevant for .container, .build and .image files. What should be the behaviour if it appears on a different types? I see three options:
1. Rename the group to `X-Quadlet` and noop

2. Error at the Quadlet level

3. Noop which will result in a systemd error because of an unrecognized section
I think option 1 is preferable as it will also be future proof.

+1 on renaming. The quadlet section will be used by .pod too for example when ContainersConfModule= is added to it, as you mentioned before.

ygalblum

LGTM, Thanks!

lambinoo · 2024-10-08T16:59:47Z

Is there a way to retry those podman db test failures? They seem pretty unrelated to quadlet. And I'm not too familiar with Github actions

Luap99 · 2024-10-08T17:04:44Z

Is there a way to retry those podman db test failures? They seem pretty unrelated to quadlet. And I'm not too familiar with Github actions

We can restart individual ones yes, but if basically all of them are red the problem is in the new changes (expect in very rare cases where some external factor is breaking things)
You can click on details and then open the link at the bottom which should show you the error
https://api.cirrus-ci.com/v1/artifact/task/4942923031117824/html/int-podman-debian-13-root-host-sqlite.log.html#t--quadlet-system-generator-Running-quadlet-dryrun-tests-Should-fail-on-bad-quadlet--1

So there is definitely something broken in quadlet that makes this this fail

lambinoo · 2024-10-08T17:38:51Z

Is there a way to retry those podman db test failures? They seem pretty unrelated to quadlet. And I'm not too familiar with Github actions

We can restart individual ones yes, but if basically all of them are red the problem is in the new changes (expect in very rare cases where some external factor is breaking things) You can click on details and then open the link at the bottom which should show you the error https://api.cirrus-ci.com/v1/artifact/task/4942923031117824/html/int-podman-debian-13-root-host-sqlite.log.html#t--quadlet-system-generator-Running-quadlet-dryrun-tests-Should-fail-on-bad-quadlet--1

So there is definitely something broken in quadlet that makes this this fail

Oh, good catch yeah. I did see a lot of those weird "unlikeat" errors before and stopped looking after seeing one. Seems like I forgot a return err! Hopefully it goes through this time.

lambinoo · 2024-10-08T19:22:42Z

Looks a bit better, but I can see a pasta related error and a WSL one.

ygalblum

Your latest forced push reverted all the changes made following the comments. I added a comment about the doc

ygalblum · 2024-10-09T12:20:44Z

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

+
+| **[Quadlet] options**      | **Description**                                   |
+|----------------------------|---------------------------------------------------|
+| DefaultDependencies=false  | Include implicit network dependencies to the unit |


Something got reverted here and the original issue is back. I'll take advantage and say that since setting it to true is redundant, it might be better to leave the false value and change the description to something like "Disable implicit network..."

ygalblum · 2024-10-09T12:21:32Z

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

+
+Add Quadlet's default dependencies to the unit.
+
+If set to true (the default), Quadlet will add a dependency (After=, Wants=) to `network-online.target` to the generated unit.


Also here. Since true is the default, I think it's better to describe the outcome of setting it to false

Updated for both!

lambinoo · 2024-10-09T12:45:38Z

Your latest forced push reverted all the changes made following the comments. I added a comment about the doc

Added back everything. The cons oF working on two different workstations and badly keeping track..

Quadlet inserts network-online.target Wants/After dependencies to ensure pulling works. Those systemd statements cannot be subsequently reset. In the cases where those dependencies are not wanted, we add a new configuration item called `DefaultDependencies=` in a new section called [Quadlet]. This section is shared between different unit types. fixes containers#24193 Signed-off-by: Farya L. Maerten <me@ltow.me>

ygalblum

LGTM

Luap99

/lgtm
thanks

openshift-ci · 2024-10-09T16:19:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lambinoo, 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

openshift-ci bot added the release-note label Oct 7, 2024

lambinoo force-pushed the quadlet-disable-default-dependencies branch 3 times, most recently from 8182de7 to 8b4d085 Compare October 7, 2024 19:26

lambinoo force-pushed the quadlet-disable-default-dependencies branch from 8b4d085 to 4d71241 Compare October 7, 2024 20:27

lambinoo force-pushed the quadlet-disable-default-dependencies branch 4 times, most recently from ccde48a to 29463da Compare October 7, 2024 21:23

vrothberg approved these changes Oct 8, 2024

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 8, 2024

Luap99 reviewed Oct 8, 2024

View reviewed changes

lambinoo force-pushed the quadlet-disable-default-dependencies branch 7 times, most recently from 0da746b to 90fd1dc Compare October 8, 2024 15:16

ygalblum reviewed Oct 8, 2024

View reviewed changes

lambinoo force-pushed the quadlet-disable-default-dependencies branch from 90fd1dc to 6ba0e79 Compare October 8, 2024 15:47

ygalblum approved these changes Oct 8, 2024

View reviewed changes

lambinoo force-pushed the quadlet-disable-default-dependencies branch 2 times, most recently from 2994057 to 426e730 Compare October 8, 2024 17:46

lambinoo force-pushed the quadlet-disable-default-dependencies branch from 426e730 to d01c484 Compare October 9, 2024 07:16

ygalblum requested changes Oct 9, 2024

View reviewed changes

lambinoo force-pushed the quadlet-disable-default-dependencies branch from d01c484 to 426e730 Compare October 9, 2024 12:44

lambinoo force-pushed the quadlet-disable-default-dependencies branch from 426e730 to bac655a Compare October 9, 2024 12:48

ygalblum approved these changes Oct 9, 2024

View reviewed changes

Luap99 approved these changes Oct 9, 2024

View reviewed changes

openshift-ci bot assigned Luap99 Oct 9, 2024

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

openshift-merge-bot bot merged commit 5890190 into containers:main Oct 9, 2024
81 of 82 checks passed

stale-locking-app 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, 2025

stale-locking-app bot locked as resolved and limited conversation to collaborators Jan 8, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow removing implicit quadlet systemd dependencies #24194

Allow removing implicit quadlet systemd dependencies #24194

lambinoo commented Oct 7, 2024 •

edited

Loading

packit-as-a-service bot commented Oct 7, 2024

ygalblum commented Oct 7, 2024

lambinoo commented Oct 7, 2024

mheon commented Oct 7, 2024

lambinoo commented Oct 7, 2024

vrothberg left a comment

Luap99 Oct 8, 2024

vrothberg Oct 8, 2024

ygalblum Oct 8, 2024

lambinoo Oct 8, 2024 •

edited

Loading

ygalblum left a comment

ygalblum Oct 8, 2024

lambinoo Oct 8, 2024

ygalblum Oct 9, 2024

lambinoo commented Oct 8, 2024

ygalblum left a comment

lambinoo commented Oct 8, 2024 •

edited

Loading

Luap99 commented Oct 8, 2024

lambinoo commented Oct 8, 2024 •

edited

Loading

lambinoo commented Oct 8, 2024

ygalblum left a comment

ygalblum Oct 9, 2024

ygalblum Oct 9, 2024

lambinoo Oct 9, 2024

lambinoo commented Oct 9, 2024 •

edited

Loading

ygalblum left a comment

Luap99 left a comment

openshift-ci bot commented Oct 9, 2024


		Add Quadlet's default dependencies to the unit.

		If set to true (the default), Quadlet will add a dependency (After=, Wants=) to `network-online.target` to the generated unit.

Allow removing implicit quadlet systemd dependencies #24194

Allow removing implicit quadlet systemd dependencies #24194

Conversation

lambinoo commented Oct 7, 2024 • edited Loading

packit-as-a-service bot commented Oct 7, 2024

ygalblum commented Oct 7, 2024

lambinoo commented Oct 7, 2024

mheon commented Oct 7, 2024

lambinoo commented Oct 7, 2024

vrothberg 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

lambinoo Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

ygalblum 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

lambinoo commented Oct 8, 2024

ygalblum left a comment

Choose a reason for hiding this comment

lambinoo commented Oct 8, 2024 • edited Loading

Luap99 commented Oct 8, 2024

lambinoo commented Oct 8, 2024 • edited Loading

lambinoo commented Oct 8, 2024

ygalblum 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

lambinoo commented Oct 9, 2024 • edited Loading

ygalblum left a comment

Choose a reason for hiding this comment

Luap99 left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Oct 9, 2024

lambinoo commented Oct 7, 2024 •

edited

Loading

lambinoo Oct 8, 2024 •

edited

Loading

lambinoo commented Oct 8, 2024 •

edited

Loading

lambinoo commented Oct 8, 2024 •

edited

Loading

lambinoo commented Oct 9, 2024 •

edited

Loading