Expand Quadlet drop-in search paths

[benniekiss]

This PR seeks to implement expanded search paths for quadlet drop-in files. It implements the systemd scheme where top-level type drop-ins and truncated unit name drop-ins are supported. More information on the systemd parsing scheme can be found here.

The major change is the addition of a function, GetUnitDropinPaths, to pkg/systemd/parser/unitfile.go. GetTemplateParts has also been refactored to return the base unit name instead of an empty string when len(parts) < 2, and a bool reflecting whether the unit was a template or not, and these changes have been reflected elsewhere in the code base.

This PR closes #23158

This is one of my first attempts at go, so please let me know if any refactoring is needed!

Does this PR introduce a user-facing change?

Added: Expand quadlet drop-in search paths to include top-level type drop-ins (container.d, pod.d) and truncated unit drop-ins (unit-.container.d)

[benniekiss]

currently, it looks like there aren't any tests for drop-ins - test/system/252-quadlet.bats. I'd appreciate some direction in how these should be added. A full suite could test drop-ins loaded from multiple levels (container.d, unit-.container.d, unit-name.container.d), and overriding top-level drop-ins with lower-level drop-ins (unit-name.container.d/10-base.conf overrides container.d/10-base.conf)

[ygalblum]

@benniekiss Thanks for this PR. The code looks good, but indeed requires new tests to make sure that it is not broken by later code changes.

I think I would take two testing approaches here. First, you can add unit tests in pkg/systemd/parser/unitfile_test.go
Second, as you noted in test/system/252-quadlet.bats.

Granted, this is a bit more complicated and I can't say for use that all the required boilerplate functions are implemented. Generally speaking, you will need to create the directory structure on your own, store all the overriding files in their sub directories and then run run_quadlet with the actual unit file and the base temporary directory. I hope this will work as is, but I cannot say for sure.

Please LMK if you have any specific question I can help with

[benniekiss]

I think I would take two testing approaches here. First, you can add unit tests in pkg/systemd/parser/unitfile_test.go

Was there a method behind why the current samples were chosen? I would want to add a sample with a more complicated name to test all parts of GetUnitDropinPaths(), but I wasn't sure about how to choose the sample. Otherwise, I can go searching for a good existing systemd unit.

Actually, I realized the file doesn't need to exist, I can just create a test filename without having any real data

[benniekiss]

I'm almost down with the system tests, but I'm having trouble actually comparing the expected file to the generated file. How do I get the contents of the generated quadlet file in the .bats tests?

EDIT: This test seems to have the structure I want:

podman/test/system/252-quadlet.bats

Line 217 in 42fa78b

@test "quadlet conflict names" {

[benniekiss]

I updated the docs to describe the full drop-in functionality. It's mostly just copying the systemd docs, but I thought it would be helpful to include.

[ygalblum]

LGTM for the code. Please squash all commits into one.

@edsantiago PTAL at the test code

[benniekiss]

Commits are squashed. Thank you for the review!

[edsantiago]

Thank you for your PR! A few suggestions inline, for test code

[edsantiago]

And, actually, I found the long complicated variable names antihelpful. WDYT of a table-driven approach, with less duplication and syntactic sugar?

@test "quadlet - dropin files" {
    local tmp_dir=${PODMAN_TMPDIR}/dropin-$(random_string)

    local quadlet_file=truncated-$(random_string).container

    local -A dirs=(
        [toplevel]=container.d
        [truncated]=truncated-.container.d
        [quadlet]=${quadlet_file}.d
    )

    # List of .conf files, with their contents. Format is:
    #
    #   expect?  dir  filename  [Section]  Content=.....
    local testfiles="
y | toplevel  | 10 | [Unit]      | Description=Test File for Dropin Configuration
n | toplevel  | 99 | [Install]   | WantedBy=default.target
y | truncated | 50 | [Container] | ContainerName=truncated-dropins
n | truncated | 99 | [Service]   | Restart=always
n | truncated | 99 | [Install]   | WantedBy=multiuser.target
y | quadlet   | 99 | [Service]   | RestartSec=60s
"

    # Pass 1: create all the conf directories and files
    while read yn dir conf section content; do
        local d="${tmp_dir}/${dirs[$dir]}"
        mkdir -p $d

        local f="$d/$conf.conf"
        echo "$section" >>$f
        echo "$content" >>$f
    done < <(parse_table "$testfiles")

    quadlet_path_in=${PODMAN_TMPDIR}/${quadlet_file}
    cat > ${quadlet_path_in} <<EOF
[Container]
Image=$IMAGE
EOF

    run_quadlet "${quadlet_path_in}" "${tmp_dir}"

    # Pass 2: confirm that each of the wanted entries is present,
    # and the unwanted ones are not
    while read yn dir conf section content; do
        if [[ "$yn" = "y" ]]; then
            assert "$output" =~ "$content" "Set in $dir/$conf.conf"
        else
            assert "$output" !~ "$content" "Set in $dir/$conf.conf but should have been overridden"
        fi
    done < <(parse_table "$testfiles")

    assert "${output}" !~ "WantedBy=" "quadlet should not include WantedBy="
}

[benniekiss]

And, actually, I found the long complicated variable names antihelpful. WDYT of a table-driven approach, with less duplication and syntactic sugar?

Personally, I'm not that proficient in bash, so this approach would make it more difficult to read and understand what's going on. It also departs from the pattern of the other tests.

I'm not against it entirely, because it does seem like a more streamlined approach, but I think it kind of obfuscates what is being tested.

[edsantiago]

Understood. I feel the opposite way, that your test code is hard to understand, but I accept your reasoning. Should this test ever fail, we will call on you to investigate and fix.

My earlier review comments still stand. Thanks again for contributing!

[benniekiss]

Of course! I'll be happy to investigate any failed tests should they come up

[benniekiss]

@edsantiago After sitting with your suggested version a little longer, I actually like it a lot more and decided to go with it! Rewriting mine to make it clearer required a lot more commenting and made it too long. Thanks for taking the time to review and help me write this test ~~

I'll squash the commits once you think it is good to go

[benniekiss]

Actually, one last thing. I based my first draft test on the one below. With the refactored run_quadlet, the hard-coded command can take advantage of the QUADLET_SERVICE_CONTENT output in the same way. Should I go ahead and adapt this test to clean it up? I think this is the only instance of this pattern in the252-quadlet.bats file.

podman/test/system/252-quadlet.bats

Lines 217 to 241 in 42fa78b

	@test "quadlet conflict names" {
	# If two directories in the search have files with the same name, quadlet should
	# only process the first name
	dir1=$PODMAN_TMPDIR/$(random_string)
	dir2=$PODMAN_TMPDIR/$(random_string)
	local quadlet_file=basic_$(random_string).container
	mkdir -p $dir1 $dir2
	cat > $dir1/$quadlet_file <<EOF
	[Container]
	Image=$IMAGE
	Notify=yes
	EOF
	cat > $dir2/$quadlet_file <<EOF
	[Container]
	Image=$IMAGE
	Notify=no
	EOF
	QUADLET_UNIT_DIRS="$dir1:$dir2" run \
	timeout --foreground -v --kill=10 $PODMAN_TIMEOUT \
	$QUADLET --dryrun
	assert "$output" =~ "Notify=yes" "quadlet should show Notify=yes"
	assert "$output" !~ "Notify=no" "quadlet should not show Notify=no"
	}

[edsantiago]

Thank you for your kind words. Have you considered a career in international diplomacy? ;-)

And, my apologies for taking so long to respond. Been chasing down two very tricksy bugs.

Your changes LGTM. Please rebase and squash and push when convenient. Thank you!

[benniekiss]

Thank you for your kind words. Have you considered a career in international diplomacy? ;-)

You're welcome! You joke, but my background is in law and government, and old habits die hard...

[edsantiago]

LGTM. @ygalblum PTANL, merge if you so desire. Thanks again @benniekiss.

[ygalblum]

@edsantiago thanks for your help. @benniekiss Thanks for the contribution
/lgtm

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: benniekiss, edsantiago, 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 [edsantiago,ygalblum]

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