fix: use same package manager for k3s-selinux removal as for installation#13711
fix: use same package manager for k3s-selinux removal as for installation#13711becarusys wants to merge 4 commits intok3s-io:mainfrom
Conversation
d54e596 to
84d9288
Compare
Signed-off-by: Tom Risse <tom@becarusys.de>
84d9288 to
4fb9246
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #13711 +/- ##
==========================================
- Coverage 21.79% 21.76% -0.04%
==========================================
Files 191 191
Lines 15539 15556 +17
==========================================
- Hits 3386 3385 -1
- Misses 11702 11720 +18
Partials 451 451
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
install.sh
Outdated
| rm -rf /var/lib/kubelet | ||
| rm -f ${BIN_DIR}/k3s | ||
| rm -f ${KILLALL_K3S_SH} | ||
| EOF |
There was a problem hiding this comment.
This EOF is short circuiting the entire logic for removing the selinux RPM
Test this by running INSTALL_K3S_SH_INCLUDE_SELINUX_RPM=true ./install.sh and check the uninstall script, there is no additional logic around selinux anymore.
| EOF |
There was a problem hiding this comment.
I am not entirely sure what you mean.
Setting UNINSTALL_K3S_SH_INCLUDE_SELINUX_RPM to any value on a supported distro with selinux has no effect, as the variable gets overwritten anyway after the package install and before the uninstall script creation.
Perhaps you meant INSTALL_K3S_SKIP_SELINUX_RPM="true"? In that case excluding uninstall logic was intentional, but I did not consider that it would potentially break airgap uninstalls like you mentioned in the other comment.
install.sh
Outdated
| rm -f /etc/zypp/repos.d/rancher-k3s-common*.repo | ||
| fi | ||
| # append package removal to script if k3s-selinux has been installed | ||
| if [ "${UNINSTALL_K3S_SH_INCLUDE_SELINUX_RPM}" == "true" ]; then |
There was a problem hiding this comment.
Adding this may breaks airgap uninstalls, where the k3s-selinux rpm may have been installed separately before running this install script.
I think we should retain the original (we always run the uninstall if the package manager is available). Really you just want to be smarter about "which package manager are we using.
There was a problem hiding this comment.
That is a good point. I thought of it as a feature, but for others it might be a breaking change.
If we always want to execute the removal logic I can think of three options:
- check for available package managers how it is currently done, but simply check rpm-ostree before the others. Quick & dirty, but is probably already enough to fix k3s-uninstall.sh: k3s-selinux package removal fails on Fedora CoreOS #13710.
- Use logic similar to the install logic. Might be cleaner, flexible and more predictable, but increases code duplication (compared to my current implementation).
- Combine both: First try to match any configured distro like in option 2, but continue with "guessing" like in option 1 if no match occurs.
Which option would you prefer, or do you have another suggestion?
There was a problem hiding this comment.
I went with option 3 and updated the PR, feedback is appreciated :)
Signed-off-by: Tom Risse <tom@becarusys.de>
Signed-off-by: Tom Risse <tom@becarusys.de>
Signed-off-by: Tom Risse <tom@becarusys.de>
|
Fixed one oversight, |
Proposed Changes
This PR introduces generating the k3s-selinux uninstallation section in k3s-uninstall.sh based on the package manager which was used to install it in the first place.
Currently the k3s-uninstall.sh script just checks if yum, rpm-ostree or zypper is installed (in this order) and uses the first one it finds. This is problematic when you have multiple of them are installed, e.g. on Fedora CoreOS 43 both yum (dnf) and rpm-ostree are available by default, but only rpm-ostree can be used for successful package removal.
Types of Changes
Bugfix: k3s-uninstall.sh: fix removal of k3s-selinux package on Fedora CoreOS (change only affects new installations)
Verification
k3s-uninstall.shand check output for successful package removalI have tested it successfully on Fedora CoreOS 43, openSUSE MicroOS and Ubuntu 24.04.
Testing
I don't know if this is already covered by tests. I assume a failure would be hard to notice anyway because the script does not run with
set -e.Linked Issues
#13710
User-Facing Change
Further Comments
The command
removein rpm-ostree seems to be an alias foruninstall. Usingremovewould further simplify the script but the alias is undocumented, therefore I chose against using it.