-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Add a change for killall to not unmount server and agent directory #10403
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #10403 +/- ##
==========================================
- Coverage 49.68% 43.54% -6.15%
==========================================
Files 179 179
Lines 14955 14955
==========================================
- Hits 7431 6512 -919
- Misses 6166 7250 +1084
+ Partials 1358 1193 -165
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
- Need to account for possibility of arbitrary other mounted paths, not just the agent and server dirs.
- Need to account for data and storage dirs under /var/lib/rancher/k3s
8da71f9
to
28926ea
Compare
28926ea
to
50cb1c5
Compare
install.sh
Outdated
do_unmount_and_remove '/var/lib/kubelet/pods' | ||
do_unmount_and_remove '/var/lib/kubelet/plugins' | ||
do_unmount_and_remove '/run/netns/cni-' | ||
clean_mounted_directory '/var/lib/rancher/k3s' |
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.
wait wait wait, we put this in the killall script... so now whenever you run the killall script it deletes all of K3s off your node. This makes sense to do in the uninstaller but the killall script DEFINITELY shouldn't be deleting everything out from under /var/lib/rancher/k3s otherwise you end up wiping your cluster off the face of the earth when all you wanted to do was kill the pods and service.
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.
@brandond when you said about deleting everything inside the /var/lib/rancher/k3s
means also for the data
and storage
? btw I added the clean_mounted_directory
to the uninstall script.
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.
thanks! Sorry, I realize my review here has not been great, I forgot that we were talking about killall and not just uninstall.
d09a327
to
e51862c
Compare
This looks good to me but I'd like someone from QA to validate it on a couple different distros with different disks mounted under /var/lib/rancher/ just to validate that I haven't missed anything - prior to merging. |
872c6f6
to
9a0c31f
Compare
I think it should be OK? |
9a0c31f
to
19f8ba2
Compare
maybe just simulate a disks mounted in some distros and creating the path( /var/lib/rancher/ ) and one test for a custom dir |
1e79d84
to
00fe226
Compare
Ignore centos 7 failure, I am handling it in a separate PR. |
Signed-off-by: Vitor Savian <[email protected]> Add recursive search and deletion of unmounted/mounted dirs in killall Signed-off-by: Vitor Savian <[email protected]> Only clean the server and agent directory if it is uninstall Signed-off-by: Vitor Savian <[email protected]> Add uninstall test to check mount points Signed-off-by: Vitor Savian <[email protected]> Add uninstall test in CI Signed-off-by: Vitor Savian <[email protected]>
00fe226
to
41917b2
Compare
Proposed Changes
/var/lib/rancher/k3s
directory, the main idea is to remove the unmount and delete.uninstall
scriptTypes of Changes
killall
script to not delete anything inside/var/lib/rancher
uninstall
script to not delete mounted dirs inside/var/lib/rancher
Verification
killall
scriptuninstall
scriptTesting
Linked Issues
/var/lib/rancher/k3s/(server|agent)
#8527User-Facing Change
Further Comments