Change installation path to the system managed path#61
Conversation
|
It's also possible to update For now, I kept it simple and just hit those files with the same If it's desirable, I can fix up those scripts too. |
|
For reference/convenience, here is an example script for making overrides. I'm currently using this to replicate bug behaviors w.r.t. specific tagged versions of our microservices: [rocky@head ~]$ cat override-quadlet-images.bash
#!/usr/bin/env bash
function log-error() {
if ! [ ${#} -eq 1 ]; then
log-error "log-error must receive only one argument, but received $#"
fi
PREFIX="error: "
MESSAGE="${PREFIX} ${1}"
echo "${MESSAGE}"
logger --id=$$ "${MESSAGE}"
exit 1
}
if ! [ "${UID}" -eq 0 ]; then
log-error "superuser privileges are required to execute this script!"
fi
echo "creating container image overrides for OpenCHAMI quadlet files:"
DEST="/etc/containers/systemd"
FNAME_OVERRIDE="10-override.conf"
while IFS= read -r filename; do
TARGET="${DEST}/$(basename ${filename}).d/${FNAME_OVERRIDE}"
mkdir -v -p "$(dirname ${TARGET})"
cp -v "${filename}" "${TARGET}"
sed -i '/\[Container\]\|Image/!d' "${TARGET}"
done < <(rpm -ql openchami | grep -i '\.container')
echo "finished."Warning It's imperative that you use the If you do follow with this advice, then the config is appended to the corresponding sections upon the next Example [rocky@head ~]$ cat /etc/containers/systemd/coresmd-coredns.container.d/10-override.conf
[Container]
Image=ghcr.io/openchami/coresmd:v0.6.3[rocky@head ~]$ sudo systemctl daemon-reload
[rocky@head ~]$ sudo systemctl cat coresmd-coredns.service
# /run/systemd/generator/coresmd-coredns.service
# Automatically generated by /usr/lib/systemd/system-generators/podman-system-generator
#
[Unit]
Wants=network-online.target
After=network-online.target
Description=The CoreSMD CoreDNS container
Wants=haproxy.service
After=haproxy.service
PartOf=openchami.target
SourcePath=/usr/share/containers/systemd/coresmd-coredns.container
RequiresMountsFor=%t/containers
RequiresMountsFor=/etc/pki/ca-trust/extracted/pem/tls-ca-bundle.pem
RequiresMountsFor=/etc/openchami/configs/Corefile
[X-Container]
ContainerName=coresmd-coredns
HostName=coresmd-coredns
Image=ghcr.io/openchami/coresmd:v0.4.3
Exec=/coredns
# Capabilities
AddCapability=NET_ADMIN
AddCapability=NET_RAW
# Volumes
Volume=/etc/pki/ca-trust/extracted/pem/tls-ca-bundle.pem:/root_ca/root_ca.crt:ro,Z
Volume=/etc/openchami/configs/Corefile:/Corefile:ro,Z
# Networks for the Container to use
Network=host
# Unsupported by generator options
# Proxy settings
PodmanArgs=--http-proxy=false
Image=ghcr.io/openchami/coresmd:v0.6.3
[Service]
Restart=always
Environment=PODMAN_SYSTEMD_UNIT=%n
KillMode=mixed
ExecStop=/usr/bin/podman rm -v -f -i coresmd-coredns
ExecStopPost=-/usr/bin/podman rm -v -f -i coresmd-coredns
Delegate=yes
Type=notify
NotifyAccess=all
SyslogIdentifier=%N
ExecStart=/usr/bin/podman run --name coresmd-coredns --replace --rm --cgroups=split --hostname coresmd-coredns --network host --sdnotify=conmon -d --cap-add net_admin --cap-add net_raw -v /etc/pki/ca-trust/extracted/pem/tls-ca-bund>Notice how the new |
This presents a new challenge in that we need to figure out how to apply the changes that the scripts apply in an idiomatic way that doesn't interfere with the user's expectations. Taking into account the following:
I can think of a couple of solutions that we should decide on, or come up with a different solution: OPTION 1:
|
|
This change seems more idiomatic of Systemd anyway, so I support it. If I were to choose, I think I would probably lean to OPTION 2 since it is more DRY. |
I do like option 2, but a few questions first:
AddendumEvidence for lack of use for
|
|
See above message, a potential option four would be to heavily slim that script down such that it only creates the global override.conf file mentioned at the bottom (and placing it in the admin path). This fixes the two potential issues with the idea:
|
We could also keep this automated by having a systemd oneshot unit run a script to populate the values in the override.conf automatically. Then, the moment someone updates the system hostname with This seems like the path of least friction, so I'll start there even though the systemd specifiers seems cooler/lazier in the less code is better code sense. |
It's used here as a postinstall script to do things like set up the Podman secrets. The logic there was a bit complex to include entirely within the spec file, which is why it's a separate script.
I think the
As an aside, I think removing In sum, keeping the |
Ah... that's even in the grep output I posted. Sorry about that, I was reading too fast. |
@synackd In the recent commits, I've tried some things out and made a few assumptions which deviate a bit from your recommendation a bit, but:
|
|
Hold off on testing. Something is broken. |
ba33bc2 to
6c29448
Compare
|
@synackd it's ready again. Ignore the WIP commit for now. Figured I'd save commit cleanup for once this is farther along in the review process (assuming squash doesn't occur). I was able to bypass the issue before with a distro path level containers.d/10-openchami.conf file. Note I still like this solution, but it requires an additional set of changes to be more elegant. The "Moreover, for unit names containing dashes ("-"), the set of directories generated by repeatedly truncating the unit name after all dashes is searched too. Specifically, for a unit name foo-bar-baz.service not only the regular drop-in directory foo-bar-baz.service.d/ is searched but also both foo-bar-.service.d/ and foo-.service.d/. This is useful for defining common drop-ins for a set of related units, whose names begin with a common prefix. This scheme is particularly useful for mount, automount and slice units, whose systematic naming structure is built around dashes as component separators. Note that equally named drop-in files further down the prefix hierarchy override those further up, i.e. foo-bar-.service.d/10-override.conf overrides foo-.service.d/10-override.conf." However, I figured it'd be best to ask before going off and making major naming changes. |
|
|
synackd
left a comment
synackd
left a comment
There was a problem hiding this comment.
Tested this and it worked (after I cleaned up some old configs). Suggesting some changes I think will improve this.
| Exec=--deploy \ | ||
| Entrypoint=/bin/sh | ||
| Exec=-ec '/entry.sh --deploy \ |
There was a problem hiding this comment.
What's the reason we need to do this?
There was a problem hiding this comment.
I'm not exactly fond of the replacement, but I made the changes to ensure $SYSTEM_FQDN isn't expanded until the process actually executes inside the container.
With the old syntax, it was expanding too early and leaving behind a plain %H in its place, which is invalid.
TLDR: New syntax sucks, but this does let us use the system hostname as a sane default WHILE still enabling overrides.
There was a problem hiding this comment.
Can't this be fixed by quoting the variable as we are below?
There was a problem hiding this comment.
I don't believe so. Well, at least not the way we're doing it here. To explain:
- If SystemD sees basically anything matching the pattern
${<varname>}, it will immediately expand it and this happens before it's passed into the container cmdline args / exec.
This happens at the SystemD parser level, which is why you can escape the behavior using $${<varname>}. No shell like interpretation is done inside the container unless a shell is explicitly defined as the entrypoint. So if we revert back to the old entrypoint config, then acme.sh will receive the string literal '${SYSTEMD_FQDN}' as an argument.
Note
AFAIK, you can get behavior on par with what you desire by setting the env var under the [Service] section. Though, since we're also using specifiers... I'm not sure which off the top of my head is expanded first.
I kept the defs under the [Container] section for consistency too, since in my mind it seemed more confusing to have a one off exception just for ACME. Like if I come back to this a year from now, I'm probably not going to remember systemd has weird parsing syntax and I can technically get around it with the [Service] approach.
There was a problem hiding this comment.
I think setting the env in the [Container] directive is probably less hacky than manually calling the entrypoint. Will that work and would we still need the $$?
There was a problem hiding this comment.
The main issue with that is setting an Environment=<var>=<value> inside a [Container] section doesn't translate back to SystemD env vars. Rather it's only passed as a -e <env> argument to podman run.
I'm pushing up a possible alternative, but it's almost a "double definition" IMO. Still, results in the entrypoint behavior you're requesting and a good explanation comment will do well to mitigate future confusion.
There was a problem hiding this comment.
Check out 32457f0 which has the changes. Tested and functional.
There was a problem hiding this comment.
Looks nicer. I am reading your comment here but I don't quite fully understand why we need both the [Container] and [Service] Environment= with different values.
The following is my understanding for the reason for this, please correct/clarify:
[Service] Environment=assigns hostname toSYSTEM_FQDNenvironment variable via Systemd specifier expansion.[Container] Environment=assigns value ofSYSTEM_FQDNfrom step 1 toSYSTEM_FQDNin container via Podman-eflag.
There was a problem hiding this comment.
I ran into this recently as well because the behavior isn't intuitive. The assumption I made was that Environment= under [Container] behaved like Environment= under [Service], with the addition of generating a podman -e argument. Unfortunately, that's not how it works.
The reason for all of this is that we want three things simultaneously:
- Provide a sensible default FQDN using systemd specifiers.
- Allow that value to be easily overridden through
<container-name>.d/<ordinal>-override.conf. - Use that value within the
Exec=/container startup command line. (issue starts here)
Why define Environment= in both [Service] and [Container]?
If the variable is defined only under [Container]:
-
It is passed into the container as an environment variable (good).
-
It is not defined as a systemd environment variable during unit parsing (bad).
- Only Podman ever sees the variable.
- systemd cannot use it for variable expansion in the unit file.
Exec=is executed directly, not through a shell, so no shell expansion occurs.
Why the duplicate definition?
systemd performs its own variable expansion, which resembles shell expansion but is a separate mechanism.
When systemd parses a unit (including any drop-in overrides), it only considers variables defined in the service environment (for example, via Environment= under [Service] or EnvironmentFile=). Variables defined under [Container] are not visible to the systemd parser—they are only translated into Podman -e arguments when the container is launched.
As a result, the variable must be defined twice:
- Under
[Service]so systemd can expand it while parsing the unit. - Under
[Container]so the container receives the final value as an environment variable.
Although this appears redundant, the two definitions serve different consumers: one is for systemd's parser, the other is for Podman/the container.
There was a problem hiding this comment.
Thanks for the clarification. That confirms my understanding.
As a result, the variable must be defined twice:
1. Under `[Service]` so systemd can expand it while parsing the unit. 2. Under `[Container]` so the container receives the final value as an environment variable.
I think having these bullets in your comment might be useful. They helped me understand why the duplication is needed.
I am good with having the implementation like this. I'll give you a chance to comment your thoughts, but feel free to resolve.
done as of 6706b6b |
|
Looks like your latest commits need a DCO. |
Ah yeah, sorry about the laziness. I'm working off a burner VM for these tests and haven't yet set up GPG keys. The original plan was just to rebase / rewrite history with signed commits + signoffs once this was more prod ready. I'll switch the approach today. |
Signed-off-by: Sean Tronsen <sean.tronsen@gmail.com>
Signed-off-by: Sean Tronsen <sean.tronsen@gmail.com>
Signed-off-by: Sean Tronsen <sean.tronsen@gmail.com>
Signed-off-by: Sean Tronsen <sean.tronsen@gmail.com>
Signed-off-by: Sean Tronsen <sean.tronsen@gmail.com>
Signed-off-by: Sean Tronsen <sean.tronsen@gmail.com>
Signed-off-by: Sean Tronsen <sean.tronsen@gmail.com>
Signed-off-by: Sean Tronsen <sean.tronsen@gmail.com>
Signed-off-by: Sean Tronsen <sean.tronsen@gmail.com>
Signed-off-by: Sean Tronsen <sean.tronsen@gmail.com>
Signed-off-by: Sean Tronsen <sean.tronsen@gmail.com>
Signed-off-by: Sean Tronsen <sean.tronsen@gmail.com>
Signed-off-by: Sean Tronsen <sean.tronsen@gmail.com>
Signed-off-by: Sean Tronsen <sean.tronsen@gmail.com>
Signed-off-by: Sean Tronsen <sean.tronsen@gmail.com>
Signed-off-by: Sean Tronsen <sean.tronsen@gmail.com>
…lets Signed-off-by: Sean Tronsen <sean.tronsen@gmail.com>
Signed-off-by: Sean Tronsen <sean.tronsen@gmail.com>
4 participants
Pull Request Template
Thank you for your contribution! Please ensure the following before submitting:
Checklist
make test(or equivalent) locally and all tests passgit commit -s) with my real name and email<filename>.licensesidecarLICENSES/directoryDescription
Moves plain Systemd units and Podman quadlets to the correct system-managed paths:
/etc/containers/systemd->/usr/share/containers/systemd/etc/systemd/system->/usr/lib/systemd/systemEffect
This frees up the administrator-managed paths for drop-ins and local overrides. On the quadlet side, this makes simple changes much easier (e.g., testing a different image tag). More generally, the full set of Systemd override mechanisms is available.
Functionally, this PR is little more than updating the RPM to install files into the proper locations for system-managed artifacts.
Caveats
If this is accepted, we'll need a quick pass over the
openchami.orgtutorials to update any directory listings that still reference the old paths.Type of Change
For more info, see Contributing Guidelines.