utils_test: created function to update default kernel

[rh-jugraham]

Created function to update default kernel

[rh-jugraham]

@smitterl As per discussion. Review would be appreciated!

Used update_boot_option() function right above in the file as inspiration.

[dzhengfy]

Could we rename to update_vm_default_kernel()?

[dzhengfy]

Usually we do not add new functions to init.py any more. This file is already too big.
Could you please move this function into utils_sys.py?

[dzhengfy]

I know there is update_boot_option() in init.py, too. Actually I prefer to move it out of the file, too, but we can do it later in another pr.

[smitterl]

Maybe for reuse of existing sessions in test code we could expect session as parameter instead.

[rh-jugraham]

@smitterl Since rebooting will result in a new session, would this imply also returning the session (either the updated one or, if no reboot, then the passed in one) for more reuse?

Also, I believe the vm needs to be a param because of, most importantly, vm.reboot() unless there is an alternative that doesn't require the vm. So the session would need to be in addition to the vm, not instead of.

[dzhengfy]

@smitterl , I feel if update_boot_option() can be moved to another file, that will be better. So I do not suggest we add new function here. Not sure about your opinion. Welcome your feedback. Thanks

[smitterl]

@smitterl , I feel if update_boot_option() can be moved to another file, that will be better. So I do not suggest we add new function here. Not sure about your opinion. Welcome your feedback. Thanks

I proposed to add it here because it's semantically related to the boot parameter update function but I wouldn't insist on it.

If making __init__.py smaller is the motivation then I'd be in favor of moving the two related functions into utils_sys.py but importing the old one (update boot parameters) in __init__.py. IIUC, this way existing code that uses it doesn't have to be updated but the two semantically related functions woudl live together in utils_sys.py which might also aid code exploration as opposed to keeping them in the filename __init__.py, ie.

# cat __init__.py
...
from utils_sys import update_boot_option

# cat utils_sys.py
...
def update_boot_option(...

def update_default_kernel(...

[rh-jugraham]

@smitterl @dzhengfy Created new PR - #4087 - for moving update_boot_option that depends on this PR (for shared imports/helper functions)

[dzhengfy]

@smitterl @dzhengfy Created new PR - #4087 - for moving update_boot_option that depends on this PR (for shared imports/helper functions)

@rh-jugraham @smitterl I think more about this change. Moving update_boot_option() and this pr to utils_sys.py will make init.py pretty clean, but seems we also need move update_boot_option_ubuntu(), __run_cmd_and_handle_error(), check_kernel_cmdline() . Without moving them, we will have utils_test import utils_sys , and utils_sys import utils_test as well.

And if we move update_boot_option() to utils_sys.py, we have to also update tp-libvirt and tp-qemu for their reference to this function or we need still keep an encapsulating function in init.py for ease like below
in init.py

from virttest import utils_sys

def update_boot_option( ...):
   utils_sys.update_boot_option(...)

And check_kernel_cmdline() has one reference in tp-qemu (tp-libvirt does not use it) as below:
qemu/tests/boot_without_vectors.py:40: req_args = utils_test.check_kernel_cmdline(session,

Based on above analysis, it is high cost to make init.py to be pretty, at least I do not expect this happen in your PR. Maybe we use separate PR to achieve this later. So let's keep your current implementation using __init.py this time.
Please help close #4087 and thank you for submitting it anyway.

[dzhengfy]

@rh-jugraham you may consider to make init.py in another PR later if you would like. Thanks.

[dzhengfy]

LGTM

[rh-jugraham]

@dzhengfy Moved back to utils_test as indeed, currently the two tests are too integrated with utils_test and reliant on other functions there so it makes sense for a different PR to figure out how to best seperate out the functions.

[dzhengfy]

LGTM

[smitterl]

Grubby will use the kernel image path

# grubby --default-kernel
/boot/vmlinuz-6.13.5-200.fc41.x86_64

That makes me wonder which format you use for kernel_version, especially considering that there's some arch-specific info in that image file name.
From line 299 I assume you expect the full kernel path as there's no manipulation.
How would a user retrieve that value before calling this function? Would it make sense for this function to construct part of the full path, e.g. expecting only 6.13.5-200.fc41?

[rh-jugraham]

Below is some experimentation with what --set-kernel can and can't take. So some variation is accepted. However, I wouldn't be sure how to take in "6.13.5-200.fc41" as, for aarch64, there is the 64k option, so wouldn't be sure how that would work, especially if multiple kernels match the same partial string - such as differentiating between "vmlinuz-5.14.0-570.4.1.el9_6.aarch64+64k" and "vmlinuz-5.14.0-570.4.1.el9_6.aarch64" if only "5.14.0-570.4.1.el9_6" is given.

I added "Can use the entire kernel path or just be the full kernel name." to hopefully clarify that it can be either the path or just the name, but I don't know how to use just part of the kernel name without overcomplicating the function and making incorrect assumptions.

[root@ampere-mtsnow-altramax-47 /]# grubby --default-kernel
/boot/vmlinuz-5.14.0-570.4.1.el9_6.aarch64+64k

# using full output from grubby
[root@ampere-mtsnow-altramax-47 /]# grubby --set-default="kernel="/boot/vmlinuz-5.14.0-570.4.1.el9_6.aarch64""
The default is /boot/loader/entries/0eae46187a3c4734ad721259b7d1c2b5-5.14.0-570.4.1.el9_6.aarch64.conf with index 1 and kernel /boot/vmlinuz-5.14.0-570.4.1.el9_6.aarch64
[root@ampere-mtsnow-altramax-47 /]# grubby --default-kernel
/boot/vmlinuz-5.14.0-570.4.1.el9_6.aarch64

# using full path
[root@ampere-mtsnow-altramax-47 /]# grubby --set-default="/boot/vmlinuz-5.14.0-570.4.1.el9_6.aarch64+64k"
The default is /boot/loader/entries/0eae46187a3c4734ad721259b7d1c2b5-5.14.0-570.4.1.el9_6.aarch64+64k.conf with index 0 and kernel /boot/vmlinuz-5.14.0-570.4.1.el9_6.aarch64+64k
[root@ampere-mtsnow-altramax-47 /]# grubby --default-kernel
/boot/vmlinuz-5.14.0-570.4.1.el9_6.aarch64+64k

# using just full name 
[root@ampere-mtsnow-altramax-47 /]# grubby --set-default="vmlinuz-5.14.0-570.4.1.el9_6.aarch64"
The default is /boot/loader/entries/0eae46187a3c4734ad721259b7d1c2b5-5.14.0-570.4.1.el9_6.aarch64.conf with index 1 and kernel /boot/vmlinuz-5.14.0-570.4.1.el9_6.aarch64
[root@ampere-mtsnow-altramax-47 /]# grubby --default-kernel
/boot/vmlinuz-5.14.0-570.4.1.el9_6.aarch64

# using partial name (not including vmlinuz)
[root@ampere-mtsnow-altramax-47 /]# grubby --set-default="5.14.0-570.4.1.el9_6.aarch64+64k"
The param 5.14.0-570.4.1.el9_6.aarch64+64k is incorrect

[smitterl]

To me it looks like /boot is not necessary but the full kernel image file name is.

In any case, I believe you don't have to worry about that, grubby can return all kernel paths and you can implement a function that helps select one and define it here right next to it and maybe also mention in the docstrings that it can be used to select the right kernel path:

def get_available_kernel_path(session, kernel_pattern):
    """
    Looks up all available kernels on the system and returns the first one whose image file name matches a pattern
    ...
    """
    s, output = utils_misc.cmd_status_output("grubby --info ALL", session=session ...)
    ...
    for line in output.split('\n'):
        if line.startswith("kernel=") and re.match(kernel_pattern, line):
            return line.split("=")[1]
    ...

[rh-jugraham]

Sounds good! I did make it so the function returns all of the kernels it found in a list in case there is some reason to find all --> perhaps might make the function more useful.

[rh-jugraham]

@smitterl thanks for the suggestions! I addressed them all, except for 1 I had a question about.

[smitterl]

LGTM thank you very much for this!