-
Notifications
You must be signed in to change notification settings - Fork 373
[no-relnote] cleanup /etc/cdi/nvidia.yaml on uninstall #1177
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
b915e99
to
4f566c7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Adds cleanup of the generated /etc/cdi/nvidia.yaml
file when uninstalling the NVIDIA container toolkit packages.
- Marks
/etc/cdi/nvidia.yaml
as a ghost config in the RPM spec - Adjusts Debian postrm hooks to remove the toolkit symlink on
remove
and clean upnvidia.yaml
in the base package - Introduces a new Debian postrm script for the base package to delete the stale
nvidia.yaml
on purge/remove
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
packaging/rpm/SPECS/nvidia-container-toolkit.spec | Declares /etc/cdi/nvidia.yaml as a %config(noreplace) %ghost |
packaging/debian/nvidia-container-toolkit.postrm | Expands purge to `purge |
packaging/debian/nvidia-container-toolkit-base.postrm | New script to delete /etc/cdi/nvidia.yaml on purge/remove |
Comments suppressed due to low confidence (1)
packaging/debian/nvidia-container-toolkit-base.postrm:7
- [nitpick] There’s no automated test verifying that
/etc/cdi/nvidia.yaml
is removed on purge/remove. Adding a simple integration test for the postrm behavior would help catch regressions.
/bin/rm -f /etc/cdi/nvidia.yaml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - @elezar ?
Pull Request Test Coverage Report for Build 16174536002Details
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing to note is that the /etc/cdi/nvidia.yaml
file is ENTIRELY user created. I can see a case being made for removing /var/run/cdi/nvidia.yaml
, but removing user-generated content when uninstalling packages seems incorrect.
I'm not sure |
Signed-off-by: Pat Riehecky <[email protected]>
a1acd4e
to
5596450
Compare
This PR adds a cleanup of
/etc/cdi/nvidia.yaml
upon uninstallation (not upgrade) of thenvidia-container-toolkit-base
package.When things are truly screwed up on my system, uninstall and reinstall of the nvidia components is a quick way to get back to a known state. However, since the current packages don't track the CDI config, a broken nvidia.yaml can persist on the system. If I don't remember I generated this file, I wont remember to remake it. My existing config management notices when the file is missing and builds it automatically.