Hi Coiby,
the patch looks really good. Only a few small nits :)
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.
- 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. ...
+ 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
Philipp
.SH "SEE ALSO"