Conversation
|
Great feature. But we might need to ensure that we don't try to remove a running kernel. Because just keeping the last kernel could be not enough if you didn't reboot since the last kernel update. I think that Apt handle this but not sure what will happen when called from dpkg. |
alexAubin
left a comment
There was a problem hiding this comment.
General principle looks fine to me but I have a bunch of comments 😅 I hope it' not too overwhelming, I tried to provide full explanation for the changes I'm proposing 😅
src/tools.py
Outdated
| for kernel in kernels: | ||
| if running_kernel[0] not in kernel: | ||
| subprocess.run("apt remove -y --purge " + kernel, shell=True) | ||
| else: | ||
| print("This is the running kernel, it won't be removed") |
There was a problem hiding this comment.
Imho we probably want to have a single call to apt using something like below. (I hope list comprehension is not too magic but that's a nice context to learn about it ;P)
Note that I'm not comparing running_kernel to kernel because running_kernel is only the version (as returne by uname -r) and kernel is the apt/dpkg package, something like linux-image-1.2.3-4-amd64
To run a single apt command, i'm using ' '.join(kernels_to_remove) inside an f-string (also not sure if you're familiar with those, I'm a big f-an of them but maybe that's too much magic 😅 )
| for kernel in kernels: | |
| if running_kernel[0] not in kernel: | |
| subprocess.run("apt remove -y --purge " + kernel, shell=True) | |
| else: | |
| print("This is the running kernel, it won't be removed") | |
| kernels_to_remove = [kernel for kernel if running_kernel not in kernel] | |
| cmd = f"apt remove -y --purge {' '.join(kernels_to_remove)}" | |
| subprocess.run(cmd, shell=True) |
To further improve upon this I also suggest :
- i would ask the admin for confirmation before actually running the apt, because removing kernels feel a bit touchy ... Something along the lines of "The system is currently running kernel "1.2.3.4". This will remove the following kernels : linux-image-0.1.2, linux-image-0.1.3... Proceed ?" You can use
_ask_confirmation(...)for this for which you'll find example usage in the sametools.py - we should check the
returncodeof the subprocess because apt may fail for some reason, in which case we probably want to have somelogger.error()message
But this can be iterated upon in a second phase, I hope this ain't too overwheling 😅
There was a problem hiding this comment.
Oh I get what you did here ! Guess I'm not used to Python trick and oneliner yet haha. I'll look into it
There was a problem hiding this comment.
I added the error message and the confirmation as suggested.
Note that I used directly the strings and not the translation framework you're using. Should I create an entry in the locals files or will it be managed automatically by Weblate ?
Co-authored-by: Alexandre Aubin <[email protected]>
Co-authored-by: Alexandre Aubin <[email protected]>
Co-authored-by: Alexandre Aubin <[email protected]>
Co-authored-by: Alexandre Aubin <[email protected]>
Co-authored-by: Alexandre Aubin <[email protected]>
… into kernel-removal-tool
Co-authored-by: Alexandre Aubin <[email protected]>
Co-authored-by: Alexandre Aubin <[email protected]>
alexAubin
left a comment
There was a problem hiding this comment.
LGTM after the last remaining comments
Co-authored-by: Alexandre Aubin <[email protected]>
Co-authored-by: Alexandre Aubin <[email protected]>
| return | ||
| else: | ||
| from .utils.app_utils import _ask_confirmation | ||
| _ask_confirmation(f"The system is currently running kernel {running_kernel}. This will remove the following kernels : {', '.join(kernels_to_remove)}. Proceed ?") |
There was a problem hiding this comment.
cf discussion irl : _ask_confirmation in fact expects an i18n key. We can also provide params={ ... } with a dict of values for the i18n string
So let's have something like :
| _ask_confirmation(f"The system is currently running kernel {running_kernel}. This will remove the following kernels : {', '.join(kernels_to_remove)}. Proceed ?") | |
| _ask_confirmation(f"tools_clean_old_kernels_confirm", params={"running_kernel": running_kernel, "kernels_to_remove"=', '.join(kernels_to_remove)}) |
and then you need to edit locales/en.json to add a new entry "tools_clean_old_kernels_confirm" with value :
"The system is currently running kernel {running_kernel}. This will remove the following kernels : {kernels_to_remove}. Proceed ?"
| cmd = ["apt", "remove", "-y", "--purge"] + kernels_to_remove | ||
| kernel_removal = subprocess.run(cmd) | ||
| if kernel_removal.returncode != 0: | ||
| logger.error(f"There was an error during the kernels removal. The return code of the 'apt remove' command is : {kernel_removal.returncode}") |
There was a problem hiding this comment.
Similar i18n consideration here though the syntax is a bit different (oneday™ we will make everything consistent)
| logger.error(f"There was an error during the kernels removal. The return code of the 'apt remove' command is : {kernel_removal.returncode}") | |
| logger.error(m18n.n(""tools_clean_old_kernels_error", returncode=kernel_removal.returncode)) |
and then gotta add another key to en.json tools_clean_old_kernels_error with something like
"There was an error during the kernels removal. The return code of the 'apt remove' command is : {returncode}"
|
i dont want to break the fun but basic-space-cleanup is already doing it... |
|
Ah 😅 Hmmm not sure how it works exactly, on my end it I have version 22, 23, and 28. System is running version 28, but Still I'm thinking in some case, the For scenario for servers with a lot of uptime, you may not even be running the latest version already installed because that would involve a reboot, so you may have (version N-1), (version N = the running one), (version N+1 = need to reboot to actually run it) AND apt wants to install "version N+2", so that's 4 versions |
|
Hi, I wanted to add the key for the string inside the _ask_confirmation call inside the en.json file but it can't seem to find it. Should I run a specific command for it to be taken into account ? |
|
Eeeeh nope supposedly it should use the entry from |
The problem
We need a tool for the user to remove old kernels
Solution
Adding the kernel removal process to the basic cleanup tool
PR Status
Couldn't test with LXC; works fine on host system
How to test
Running the added code in a Python console