Hi Philipp,
On Wed, Dec 22, 2021 at 05:28:19PM +0100, Philipp Rudo wrote:
Hi Coiby,
the patch looks really good. Only a few small nits :)
Thanks for reviewing the patch!
On Thu, 16 Dec 2021 14:36:50 +0800
Coiby Xu <coxu(a)redhat.com> wrote:
> Rewrite kdumpctl reset-crashkernel KERNEL_PATH as
> kdumpctl reset-crashkernel [--fadump=[on|off|nocma]] [--kernel=path_to_kernel]
[--reboot]
>
> This interface would reset a specific kernel to the default crasherknel
typo s/crasherknel/crashkernel/
> value given the kernel path. And it also supports grubby's syntax so
> there are the following special cases,
> - if --kernel not specified, current running kernel, i.e. `uname -r` is chosen
What happens when the user has set KDUMP_KERNELVER in
/etc/sysconfig/kdump? My expectation is that it has precedence over
uname -r. I.e. if no --kernel is given choose KDUMP_KERNELVER, if that
is empty choose uname -r.
Nice catch for these two issues. I will fix it in v4.
> - if --kernel=DEFAULT, the default boot kernel is chosen
> - if --kernel=ALL, all kernels would have its crashkernel reset to the
> default value and the /etc/default/grub is updated as well
>
> --fadump=[on|off|nocma] toggles fadump on/off for the kernel provided
> in KERNEL_PATH. If --fadump is omitted the dump mode is determined by
> parsing the kernel command line for the kernel to update.
>
> CoreOS/Atomic/Silverblue needs to be treated as a special case because,
> - "rpm-ostree kargs" is used to manage kernel command line parameters
> so --kernel doesn't make sense and there is no need to find current
> running kernel
> - "rpm-ostree kargs" itself would prompt the user to reboot the system
> after modify the kernel command line parameter
> - POWER is not supported so we can assume the dump mode is always kdump
>
> This interface will also be called by kexec-tools RPM scriptlets [1]
> to reset crashkernel.
>
> Note the support of crashkenrel.default is dropped.
>
> [1]
https://docs.fedoraproject.org/en-US/packaging-guidelines/Scriptlets/
>
> Signed-off-by: Coiby Xu <coxu(a)redhat.com>
> ---
> kdumpctl | 165 +++++++++++++++++++++++++++++++++++++++++++++--------
> kdumpctl.8 | 16 +++---
> 2 files changed, 151 insertions(+), 30 deletions(-)
>
> diff --git a/kdumpctl b/kdumpctl
> index 27971a1..412420d 100755
> --- a/kdumpctl
> +++ b/kdumpctl
> @@ -1381,39 +1381,157 @@ _get_current_running_kernel_path()
> fi
> }
>
> -reset_crashkernel()
> +_update_grub()
> {
> - local kernel=$1 entry crashkernel_default
> - local grub_etc_default="/etc/default/grub"
> -
> - [[ -z $kernel ]] && kernel=$(uname -r)
> - crashkernel_default=$(cat "/usr/lib/modules/$kernel/crashkernel.default"
2> /dev/null)
> -
> - if [[ -z $crashkernel_default ]]; then
> - derror "$kernel doesn't have a crashkernel.default"
> - exit 1
> - fi
> + local _kernel_path=$1 _crashkernel=$2 _dump_mode=$3 _fadump_val=$4
>
> if is_atomic; then
> if rpm-ostree kargs | grep -q "crashkernel="; then
> - rpm-ostree kargs --replace="crashkernel=$crashkernel_default"
> + rpm-ostree kargs --replace="crashkernel=$_crashkernel"
> else
> - rpm-ostree kargs --append="crashkernel=$crashkernel_default"
> + rpm-ostree kargs --append="crashkernel=$_crashkernel"
> fi
> else
> - entry=$(grubby --info ALL | grep "^kernel=.*$kernel")
> - entry=${entry#kernel=}
> - entry=${entry#\"}
> - entry=${entry%\"}
> + [[ -f /etc/zipl.conf ]] && zipl_arg="--zipl"
> + grubby --args "crashkernel=$_crashkernel" --update-kernel
"$_kernel_path" $zipl_arg
> + if [[ $_dump_mode == kdump ]]; then
> + grubby --remove-args="fadump" --update-kernel
"$_kernel_path"
> + else
> + grubby --args="fadump=$_fadump_val" --update-kernel
"$_kernel_path"
> + fi
> + fi
> + [[ $zipl_arg ]] && zipl > /dev/null
> +}
> +
> +_valid_grubby_kernel_path()
> +{
> + [[ -n "$1" ]] && grubby --info="$1" > /dev/null
2>&1
> +}
>
> - if [[ -f $grub_etc_default ]]; then
> - sed -i -e "s/^\(GRUB_CMDLINE_LINUX=.*\)crashkernel=[^\ \"]*\([\
\"].*\)$/\1$crashkernel_default\2/" "$grub_etc_default"
> +_get_all_kernels_from_grubby()
> +{
> + local _kernels _line _kernel_path _grubby_kernel_path=$1
> +
> + for _line in $(grubby --info "$_grubby_kernel_path" | grep
"^kernel="); do
> + _kernel_path=$(_filter_grubby_kernel_str "$_line")
> + _kernels="$_kernels $_kernel_path"
> + done
> + echo -n "$_kernels"
> +}
> +
> +# modify the kernel command line parameter in default grub conf
> +#
> +# $1: the name of the kernel command line parameter
> +# $2: new value. If empty, the parameter would be removed
> +_update_kernel_cmdline_in_grub_etc_default()
> +{
> + local grub_etc_default="/etc/default/grub" _para=$2 _val=$3 _para_val
> +
> + [[ -n $val ]] && _para_val="$_para=$_val"
> +
> + sed -i -E
"s/^(GRUB_CMDLINE_LINUX=.*)([[:space:]\"])crashkernel=[^[:space:]\"]*(.*)$/\1\2$_para_val\3/"
"$grub_etc_default"
> +}
> +
> +reset_crashkernel()
> +{
> + local _opt _val _dump_mode _fadump_val _reboot _grubby_kernel_path _kernel
_kernels
> + local _old_crashkernel _new_crashkernel _new_dump_mode _crashkernel_changed
_new_fadump_val
> +
> + for _opt in "$@"; do
> + case "$_opt" in
> + --fadump=*)
> + _val=${_opt#*=}
> + if _dump_mode=$(get_dump_mode_by_fadump_val $_val); then
> + _fadump_val=$_val
> + else
> + derror "failed to determine dump mode"
> + exit
> + fi
> + ;;
> + --kernel=*)
> + _val=${_opt#*=}
> + if ! _valid_grubby_kernel_path $_val; then
> + derror "Invalid $_opt, please specify a valid kernel path, ALL or
DEFAULT"
> + exit
> + fi
> + _grubby_kernel_path=$_val
> + ;;
> + --reboot)
> + _reboot=yes
> + ;;
> + *)
> + derror "$_opt not recognized"
> + exit 1
> + ;;
> + esac
> + done
> +
> + # 1. CoreOS uses "rpm-ostree kargs" instead of grubby to manage kernel
command
> + # line. --kernel=ALL doesn't make sense for CoreOS.
> + # 2. CoreOS doesn't support POWER so the dump mode is always kdump.
> + # 3. "rpm-ostree kargs" would prompt the user to reboot the system after
> + # modifying the kernel command line so there is no need for kexec-tools
> + # to repeat it.
> + if is_atomic; then
> + _old_crashkernel=$(rpm-ostree kargs | sed -n -E
's/.*(^|\s)crashkernel=(\S*).*/\2/p')
> + _new_dump_mode=kdump
> + _new_crashkernel=$(kdump_get_arch_recommend_crashkernel
"$_new_dump_mode")
> + if [[ $_old_crashkernel != "$_new_crashkernel" ]]; then
> + _update_grub "" "$_new_crashkernel"
"$_new_dump_mode" ""
> + [[ $_reboot == yes ]] && systemctl reboot
> fi
> + return
> + fi
>
> - [[ -f /etc/zipl.conf ]] && zipl_arg="--zipl"
> - grubby --args "$crashkernel_default" --update-kernel "$entry"
$zipl_arg
> - [[ $zipl_arg ]] && zipl > /dev/null
> + # Only ppc64le supports fadump. If not specified for ppc64le, the dump
> + # mode would be determined by parsing the kernel command line of the
> + # kernel(s) to be updated
> + if [[ -z $_dump_mode && $(uname -m) != ppc64le ]]; then
> + _dump_mode=kdump
> + fi
I don't think this will work. The way I see it $_dump_mode will always
be set after this if-block. But that will make all tests for
-n/-z $_dump_mode predetermined. Which is not what you want. E.g. ...
_dump_mode is set to kdump only for non-ppc systems. So this is
something I want.
> + if [[ -n $_dump_mode ]]; then
> + _crashkernel=$(kdump_get_arch_recommend_crashkernel "$_dump_mode")
> + _fadump_val=off
> + fi
... this will always turn off fadump independent what the user gave on
the command line.
> + if [[ $_grubby_kernel_path == ALL && -n $_dump_mode ]]; then
> + _update_kernel_cmdline_in_grub_etc_default crashkernel "$_crashkernel"
> + # remove the fadump if fadump is disabled
> + [[ $_fadump_val == off ]] && _fadump_val=""
> + _update_kernel_cmdline_in_grub_etc_default fadump "$_fadump_val"
> + fi
> +
> + if [[ -z $_grubby_kernel_path ]]; then
> + if ! _kernel_path=$(_get_current_running_kernel_path); then
> + derror "no running kernel found"
> + exit 1
> + fi
> + _kernels=$_kernel_path
> + else
> + _kernels=$(_get_all_kernels_from_grubby "$_grubby_kernel_path")
> fi
> +
> + for _kernel in $_kernels; do
> + if [[ -z $_dump_mode ]]; then
> + _new_dump_mode=$(get_dump_mode_by_kernel "$_kernel")
> + _new_crashkernel=$(kdump_get_arch_recommend_crashkernel
"$_new_dump_mode")
> + _new_fadump_val=$(get_grub_kernel_boot_parameter "$_kernel" fadump)
> + else
> + _new_dump_mode=$_dump_mode
> + _new_crashkernel=$_crashkernel
> + _new_fadump_val=$_fadump_val
> + fi
> +
> + _old_crashkernel=$(get_grub_kernel_boot_parameter "$_kernel"
crashkernel)
> + if [[ $_old_crashkernel != "$_new_crashkernel" ]]; then
> + _update_grub "$_kernel" "$_new_crashkernel"
"$_new_dump_mode" "$_new_fadump_val"
> + [[ $_reboot != yes ]] && dwarn "For kernel=$_kernel,
crashkernel=$_new_crashkernel now. Please reboot the system to take effect."
The error message is a little bit clumsy. I would use
Updated crashkernel=$_new_crashkernel parameter for kernel=$_kernel.
Please reboot the system for the change to take effect.
> + _crashkernel_changed=yes
> + fi
> + done
> +
> + [[ $_reboot == yes && $_crashkernel_changed == yes ]] && reboot
> }
>
> if [[ ! -f $KDUMP_CONFIG_FILE ]]; then
> @@ -1478,7 +1596,8 @@ main()
> get_default_crashkernel "$2"
> ;;
> reset-crashkernel)
> - reset_crashkernel "$2"
> + shift
> + reset_crashkernel "$@"
> ;;
> *)
> dinfo $"Usage: $0
{estimate|start|stop|status|restart|reload|rebuild|reset-crashkernel|propagate|showmem}"
> diff --git a/kdumpctl.8 b/kdumpctl.8
> index 74be062..fd44bea 100644
> --- a/kdumpctl.8
> +++ b/kdumpctl.8
> @@ -50,13 +50,15 @@ Estimate a suitable crashkernel value for current machine. This
is a
> best-effort estimate. It will print a recommanded crashkernel value
> based on current kdump setup, and list some details of memory usage.
> .TP
> -.I reset-crashkernel [KERNEL]
> -Reset crashkernel value to default value. kdumpctl will try to read
> -from /usr/lib/modules/<KERNEL>/crashkernel.default and reset specified
> -kernel's crashkernel cmdline value. If no kernel is
> -specified, will reset current running kernel's crashkernel value.
> -If /usr/lib/modules/<KERNEL>/crashkernel.default doesn't exist, will
> -simply exit return 1.
> +.I reset-crashkernel [--kernel=path_to_kernel] [--reboot]
> +Reset crashkernel to default value recommended by kexec-tools. If no kernel
> +is specified, will reset current running kernel's crashkernel value. You can
> +also specify --kernel=ALL and --kernel=DEFAULT which have the same meaning as
> +grubby's kernel-path=ALL and kernel-path=DEFAULT. ppc64le supports FADump and
> +supports an additonal [--fadump=[on|off|nocma]] paramerter to toggle FADump
typo s/paramerter/parameter/
> +on/off. Note that there are many factors would affect the memory requirement,
> +there is no guarantee the value recommended by kexec-tools would work for your
> +case.
I would rephrase the last sentence. How about
Note: The memory requirements for kdump varies heavily depending on the
used hardware and system configuration. Thus the recommended
crashkernel might not work for your specific setup. Please test if
kdump works after resetting the crashkernel value.
Thanks for the above three improvements!
Thanks
Philipp
>
> .SH "SEE ALSO"
--
Best regards,
Coiby