From ad1d3f8fc71da01eab4353a74bdbcff6dddf329d Mon Sep 17 00:00:00 2001 From: jbtrystram Date: Fri, 15 Mar 2024 15:28:50 +0100 Subject: [PATCH] quadlet: Add a network requirement on .image units If a container unit starts on boot with a dependency on `default.target` the image unit may start too soon, before network is ready. This cause the unit to fail to pull the image. - Add a dependency on `network-online.target` to make sure image pulls don't fail. See https://github.com/containers/podman/issues/21873 - Document the hardcoded dependency on `network-online.target` for images unit and explain how it can be overriden if necessary. - tests/e2e/quadlet: Add `assert-last-key-regex` Required to test the `After=` override in [Unit] section See https://github.com/containers/podman/pull/22057#issuecomment-2008959993 - quadlet/unitfile: add a prepenUnitLine method Requirements on networks should be inserted at the top of the section so the user can override them. Signed-off-by: jbtrystram --- docs/source/markdown/podman-systemd.unit.5.md | 5 +++++ pkg/systemd/parser/unitfile.go | 16 ++++++++++++++ pkg/systemd/quadlet/quadlet.go | 16 ++++++++++++++ test/e2e/quadlet/basic.container | 2 ++ test/e2e/quadlet/basic.image | 2 ++ test/e2e/quadlet/mount.container | 2 +- test/e2e/quadlet/network.quadlet.container | 2 +- .../e2e/quadlet/unit-after-override.container | 7 ++++++ test/e2e/quadlet/unit-after-override.image | 7 ++++++ test/e2e/quadlet_test.go | 22 +++++++++++++++++++ 10 files changed, 79 insertions(+), 2 deletions(-) create mode 100644 test/e2e/quadlet/unit-after-override.container create mode 100644 test/e2e/quadlet/unit-after-override.image diff --git a/docs/source/markdown/podman-systemd.unit.5.md b/docs/source/markdown/podman-systemd.unit.5.md index af5dabd196..3015ee326b 100644 --- a/docs/source/markdown/podman-systemd.unit.5.md +++ b/docs/source/markdown/podman-systemd.unit.5.md @@ -1333,6 +1333,11 @@ exists on the host, pulling it if needed. Using image units allows containers and volumes to depend on images being automatically pulled. This is particularly interesting when using special options to control image pulls. +Note: The generated service have a dependency on `network-online.target` assuring the network is reachable if +an image needs to be pulled. +If the image service needs to run without available network (e.g. early in boot), the requirement can be +overriden simply by adding an empty `After=` in the unit file. This will unset all previously set After's. + Valid options for `[Image]` are listed below: | **[Image] options** | **podman image pull equivalent** | diff --git a/pkg/systemd/parser/unitfile.go b/pkg/systemd/parser/unitfile.go index e9b0186611..9ecbf4a158 100644 --- a/pkg/systemd/parser/unitfile.go +++ b/pkg/systemd/parser/unitfile.go @@ -90,6 +90,11 @@ func (g *unitGroup) addLine(line *unitLine) { g.lines = append(g.lines, line) } +func (g *unitGroup) prependLine(line *unitLine) { + n := []*unitLine{line} + g.lines = append(n, g.lines...) +} + func (g *unitGroup) addComment(line *unitLine) { g.comments = append(g.comments, line) } @@ -923,6 +928,17 @@ func (f *UnitFile) PrependComment(groupName string, comments ...string) { } } +func (f *UnitFile) PrependUnitLine(groupName string, key string, value string) { + var group *unitGroup + if groupName == "" && len(f.groups) > 0 { + group = f.groups[0] + } else { + // Uses magic "" for first comment-only group if no other groups + group = f.ensureGroup(groupName) + } + group.prependLine(newUnitLine(key, value, false)) +} + func (f *UnitFile) GetTemplateParts() (string, string) { ext := filepath.Ext(f.Filename) basename := strings.TrimSuffix(f.Filename, ext) diff --git a/pkg/systemd/quadlet/quadlet.go b/pkg/systemd/quadlet/quadlet.go index b32cee8671..9c13d88dcb 100644 --- a/pkg/systemd/quadlet/quadlet.go +++ b/pkg/systemd/quadlet/quadlet.go @@ -411,6 +411,14 @@ func ConvertContainer(container *parser.UnitFile, names map[string]string, isUse service := container.Dup() service.Filename = replaceExtension(container.Filename, ".service", "", "") + // Add a dependency on network-online.target so the image pull does not happen + // before network is ready + // https://github.com/containers/podman/issues/21873 + // Prepend the lines, so the user-provided values + // override the default ones. + service.PrependUnitLine(UnitGroup, "After", "network-online.target") + service.PrependUnitLine(UnitGroup, "Wants", "network-online.target") + if container.Path != "" { service.Add(UnitGroup, "SourcePath", container.Path) } @@ -1182,6 +1190,14 @@ func ConvertImage(image *parser.UnitFile) (*parser.UnitFile, string, error) { service := image.Dup() service.Filename = replaceExtension(image.Filename, ".service", "", "-image") + // Add a dependency on network-online.target so the image pull does not happen + // before network is ready + // https://github.com/containers/podman/issues/21873 + // Prepend the lines, so the user-provided values + // override the default ones. + service.PrependUnitLine(UnitGroup, "After", "network-online.target") + service.PrependUnitLine(UnitGroup, "Wants", "network-online.target") + if image.Path != "" { service.Add(UnitGroup, "SourcePath", image.Path) } diff --git a/test/e2e/quadlet/basic.container b/test/e2e/quadlet/basic.container index 3a3a98f695..3d507c884a 100644 --- a/test/e2e/quadlet/basic.container +++ b/test/e2e/quadlet/basic.container @@ -15,6 +15,8 @@ ## assert-key-is-regex "Service" "ExecStopPost" "-[/S].*/podman rm -v -f -i --cidfile=%t/%N.cid" ## assert-key-is-regex "Service" "ExecStop" ".*/podman rm -v -f -i --cidfile=%t/%N.cid" ## assert-key-is "Service" "Environment" "PODMAN_SYSTEMD_UNIT=%n" +## assert-key-is "Unit" "After" "network-online.target" +## assert-key-is "Unit" "Wants" "network-online.target" [Container] Image=localhost/imagename diff --git a/test/e2e/quadlet/basic.image b/test/e2e/quadlet/basic.image index b0502a7a57..9a30765e52 100644 --- a/test/e2e/quadlet/basic.image +++ b/test/e2e/quadlet/basic.image @@ -1,4 +1,6 @@ ## assert-podman-final-args localhost/imagename +## assert-key-is "Unit" "After" "network-online.target" +## assert-key-is "Unit" "Wants" "network-online.target" ## assert-key-is "Unit" "RequiresMountsFor" "%t/containers" ## assert-key-is "Service" "Type" "oneshot" ## assert-key-is "Service" "RemainAfterExit" "yes" diff --git a/test/e2e/quadlet/mount.container b/test/e2e/quadlet/mount.container index b982b73837..9969571bb1 100644 --- a/test/e2e/quadlet/mount.container +++ b/test/e2e/quadlet/mount.container @@ -10,7 +10,7 @@ Mount=type=bind,src=/path/on/host,dst=/path/in/container,relabel=shared,U=true Mount=type=volume,source=vol1,destination=/path/in/container,ro=true ## assert-podman-args-key-val "--mount" "," "type=volume,source=systemd-vol2,destination=/path/in/container,ro=true" ## assert-key-is "Unit" "Requires" "vol2-volume.service" -## assert-key-is "Unit" "After" "vol2-volume.service" +## assert-key-is "Unit" "After" "network-online.target" "vol2-volume.service" Mount=type=volume,source=vol2.volume,destination=/path/in/container,ro=true ## assert-podman-args-key-val "--mount" "," "type=tmpfs,tmpfs-size=512M,destination=/path/in/container" Mount=type=tmpfs,tmpfs-size=512M,destination=/path/in/container diff --git a/test/e2e/quadlet/network.quadlet.container b/test/e2e/quadlet/network.quadlet.container index 27193617f2..a37e56cdf0 100644 --- a/test/e2e/quadlet/network.quadlet.container +++ b/test/e2e/quadlet/network.quadlet.container @@ -1,6 +1,6 @@ ## assert-podman-args "--network=systemd-basic" ## assert-key-is "Unit" "Requires" "basic-network.service" -## assert-key-is "Unit" "After" "basic-network.service" +## assert-key-is "Unit" "After" "network-online.target" "basic-network.service" [Container] Image=localhost/imagename diff --git a/test/e2e/quadlet/unit-after-override.container b/test/e2e/quadlet/unit-after-override.container new file mode 100644 index 0000000000..f2ebb4ffc3 --- /dev/null +++ b/test/e2e/quadlet/unit-after-override.container @@ -0,0 +1,7 @@ +## assert-last-key-is-regex "Unit" "After" "^$" + +[Unit] +After= + +[Container] +Image=localhost/imagename diff --git a/test/e2e/quadlet/unit-after-override.image b/test/e2e/quadlet/unit-after-override.image new file mode 100644 index 0000000000..87549d3c06 --- /dev/null +++ b/test/e2e/quadlet/unit-after-override.image @@ -0,0 +1,7 @@ +## assert-last-key-is-regex "Unit" "After" "^$" + +[Unit] +After= + +[Image] +Image=localhost/imagename diff --git a/test/e2e/quadlet_test.go b/test/e2e/quadlet_test.go index 14b79e5130..db63c4f1e7 100644 --- a/test/e2e/quadlet_test.go +++ b/test/e2e/quadlet_test.go @@ -172,6 +172,24 @@ func (t *quadletTestcase) assertKeyIsRegex(args []string, unit *parser.UnitFile) return true } +func (t *quadletTestcase) assertLastKeyIsRegex(args []string, unit *parser.UnitFile) bool { + Expect(len(args)).To(BeNumerically(">=", 3)) + group := args[0] + key := args[1] + regex := args[2] + + value, ok := unit.LookupLast(group, key) + if !ok { + return false + } + + matched, err := regexp.MatchString(regex, value) + if err != nil || !matched { + return false + } + return true +} + func (t *quadletTestcase) assertKeyContains(args []string, unit *parser.UnitFile) bool { Expect(args).To(HaveLen(3)) group := args[0] @@ -469,6 +487,8 @@ func (t *quadletTestcase) doAssert(check []string, unit *parser.UnitFile, sessio ok = t.assertKeyIsRegex(args, unit) case "assert-key-contains": ok = t.assertKeyContains(args, unit) + case "assert-last-key-is-regex": + ok = t.assertLastKeyIsRegex(args, unit) case "assert-podman-args": ok = t.assertStartPodmanArgs(args, unit) case "assert-podman-args-regex": @@ -847,6 +867,7 @@ BOGUS=foo Entry("merged-override.container", "merged-override.container", 0, ""), Entry("template@.container", "template@.container", 0, ""), Entry("template@instance.container", "template@instance.container", 0, ""), + Entry("Unit After Override", "unit-after-override.container", 0, ""), Entry("basic.volume", "basic.volume", 0, ""), Entry("device-copy.volume", "device-copy.volume", 0, ""), @@ -921,6 +942,7 @@ BOGUS=foo Entry("Image - Arch and OS", "arch-os.image", 0, ""), Entry("Image - global args", "globalargs.image", 0, ""), Entry("Image - Containers Conf Modules", "containersconfmodule.image", 0, ""), + Entry("Image - Unit After Override", "unit-after-override.image", 0, ""), Entry("basic.pod", "basic.pod", 0, ""), Entry("name.pod", "name.pod", 0, ""),