common/postprocess: compat symlinks for issues.d#215
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jbtrystram The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/hold this needs the FCOS PR to merge first |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a postprocess step to common.yaml to ensure compatibility for agetty sourcing ignition issue files. It conditionally creates symlinks from /run/issue.d/ to /etc/issue.d/ if the util-linux version is below 2.41. Feedback suggests improving the robustness of the version comparison by removing a newline from the rpm query output and enhancing the symlink creation logic to prevent dangling symlinks when no matching files are found, possibly by using find.
| - | | ||
| #!/usr/bin/env bash | ||
| # set -x | ||
| installed_ver=$(rpm -q util-linux --qf "%{VERSION}\n") |
There was a problem hiding this comment.
The \n in the rpm query format will be included in the installed_ver variable. This can lead to unexpected behavior in string comparisons and makes the script more fragile. Please remove it to ensure the version string is clean.
installed_ver=$(rpm -q util-linux --qf "%{VERSION}")| # Create symlinks from /etc pointing to /run/issue.d/ so agetty picks them up. | ||
| # See https://github.com/coreos/fedora-coreos-tracker/issues/2108 | ||
| [Service] | ||
| ExecStartPost=/usr/bin/bash -c "mkdir -p /etc/issue.d && ln -sf /run/issue.d/30_coreos_ignition_*.issue /etc/issue.d/" |
There was a problem hiding this comment.
The glob pattern 30_coreos_ignition_*.issue might not match any files if ignition does not report any issues. In that case, the shell will pass the literal string to ln, creating a dangling symlink named 30_coreos_ignition_*.issue. To handle this case robustly, it's better to use find to locate the files before attempting to create symlinks. This ensures that ln is only called if files are actually found.
ExecStartPost=/usr/bin/bash -c "mkdir -p /etc/issue.d && find /run/issue.d -maxdepth 1 -name '30_coreos_ignition_*.issue' -exec ln -sft /etc/issue.d/ {} +"
jbtrystram
force-pushed
the
symlink-issue-rhel9
branch
4 times, most recently
from
778bcb2 to
802865a
Compare
Symlink ignition issues to /etc/issues.d Since util-linux 2.41 agetty will source issues from /run so we moved FCOS to write the generated issues there in [1]. Until we have util-linux >= 2.41 we need to symlink these back to /etc/issue.d/. [1] coreos/fedora-coreos-config#4046
jbtrystram
force-pushed
the
symlink-issue-rhel9
branch
from
802865a to
27221d0
Compare
| # Create symlinks from /etc pointing to /run/issue.d/ so agetty picks them up. | ||
| # See https://github.com/coreos/fedora-coreos-tracker/issues/2108 | ||
| [Service] | ||
| ExecStartPost=/usr/bin/bash -c "mkdir -p /etc/issue.d && find /run/issue.d -maxdepth 1 -name '30_coreos_ignition_*.issue' -exec ln -sft /etc/issue.d/ {} +" |
There was a problem hiding this comment.
| ExecStartPost=/usr/bin/bash -c "mkdir -p /etc/issue.d && find /run/issue.d -maxdepth 1 -name '30_coreos_ignition_*.issue' -exec ln -sft /etc/issue.d/ {} +" | |
| ExecStartPost=/usr/bin/bash -c "mkdir -p /etc/issue.d && find /run/issue.d -maxdepth 1 -name '30_*ignition_*.issue' -exec ln -sft /etc/issue.d/ {} +" |
There is a proposed slight renaming of these config files in https://github.com/PeaceRebel/ignition/pull/7/changes#r3112171987
There was a problem hiding this comment.
Thanks for the heads-up ! This whole thing is blocked on a fedora-selinux update that I need to look into, but still haven't got around to it
| # Util-linux won't source issue files from /run/issue.d/ until v2.41. | ||
| # Create symlinks from /etc pointing to /run/issue.d/ so agetty picks them up. | ||
| # See https://github.com/coreos/fedora-coreos-tracker/issues/2108 | ||
| [Service] |
There was a problem hiding this comment.
I wonder if there should be a cleanup that happens here too.
i.e. if there are stale /etc/issues.d/30_*ignition_*.issue files should we delete them?
I guess the answer maybe depends on what happens if a broken symlink exists here. If it's non fatal and also not ugly (meaning we don't get a warning about it) then leaving them in place should be fine. Otherwise we should consider cleaning them up.
There was a problem hiding this comment.
So we were including a systemd-tmpfile that was doing just that in CLHM : https://github.com/coreos/console-login-helper-messages/blob/70344cce1758730245cb46ca4d5bcdb56ff2f20e/usr/lib/tmpfiles.d/console-login-helper-messages-issuegen.conf
We could have the same thing here:
r /etc/issue.d/30_*ignition_*.issue - - - - -
|
@jbtrystram: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
Symlink ignition issues to /etc/issues.d
Since util-linux 2.41 agetty will source issues from /run so we moved FCOS to write the generated issues there in [1].
Until we have util-linux >= 2.41 we need to symlink these back to /etc/issue.d/.
[1] coreos/fedora-coreos-config#4046