Hi Coiby,

On Wed, 8 Dec 2021 10:25:29 +0800 Coiby Xu coxu@redhat.com wrote:

Factor out kdump_get_arch_recommend_crashkernel to prepare for kdump-anaconda-plugin for example to retrieve the default crashkernel value.

Note the support of crashkenrel.default is dropped.

Signed-off-by: Coiby Xu coxu@redhat.com

kdump-lib.sh | 60 +++++++++++++++++++++++++++++++--------------------- 1 file changed, 36 insertions(+), 24 deletions(-)

diff --git a/kdump-lib.sh b/kdump-lib.sh index b8f6c96..d5edaf4 100755 --- a/kdump-lib.sh +++ b/kdump-lib.sh @@ -822,40 +822,52 @@ get_recommend_size() IFS="$OLDIFS" }

+# get default crashkernel +# $1 dump mode, if not specified, dump_mode will be judged by is_fadump_capable +kdump_get_arch_recommend_crashkernel() +{

• local _arch _ck_cmdline _dump_mode
• if [[ -z "$1" ]]; then

• if is_fadump_capable; then
  

• 	_dump_mode=fadump
  

• else
  

• 	_dump_mode=kdump
  

• fi
  

• else

• _dump_mode=$1
  

• fi
• _arch=$(uname -m)
• if [[ $_arch == "x86_64" ]] || [[ $_arch == "s390x" ]]; then

• _ck_cmdline="1G-4G:192M,4G-64G:256M,64G-:512M"
  

• elif [[ $_arch == "aarch64" ]]; then

• _ck_cmdline="2G-:448M"
  

• elif [[ $_arch == "ppc64le" ]]; then

• if [[ $_dump_mode == "fadump" ]]; then
  

• 	_ck_cmdline="4G-16G:768M,16G-64G:1G,64G-128G:2G,128G-1T:4G,1T-2T:6G,2T-4T:12G,4T-8T:20G,8T-16T:36G,16T-32T:64G,32T-64T:128G,64T-:180G"
  

• else
  

• 	_ck_cmdline="2G-4G:384M,4G-16G:512M,16G-64G:1G,64G-128G:2G,128G-:4G"
  

• fi
  

• fi
• _ck_cmdline=${_ck_cmdline//-:/-102400T:}
• echo -n "$_ck_cmdline"

+}

# return recommended size based on current system RAM size # $1: kernel version, if not set, will defaults to $(uname -r) kdump_get_arch_recommend_size() {

• local kernel=$1 arch

• local _ck_cmdline

  if ! [[ -r "/proc/iomem" ]]; then echo "Error, can not access /proc/iomem." return 1 fi

• [[ -z $kernel ]] && kernel=$(uname -r)
• ck_cmdline=$(cat "/usr/lib/modules/$kernel/crashkernel.default" 2> /dev/null)
• if [[ -n $ck_cmdline ]]; then

• ck_cmdline=${ck_cmdline#crashkernel=}
  

• else

• arch=$(lscpu | grep Architecture | awk -F ":" '{ print $2 }' | tr '[:lower:]' '[:upper:]')
  

• if [[ $arch == "X86_64" ]] || [[ $arch == "S390X" ]]; then
  

• 	ck_cmdline="1G-4G:192M,4G-64G:256M,64G-:512M"
  

• elif [[ $arch == "AARCH64" ]]; then
  

• 	ck_cmdline="2G-:448M"
  

• elif [[ $arch == "PPC64LE" ]]; then
  

• 	if is_fadump_capable; then
  

• 		ck_cmdline="4G-16G:768M,16G-64G:1G,64G-128G:2G,128G-1T:4G,1T-2T:6G,2T-4T:12G,4T-8T:20G,8T-16T:36G,16T-32T:64G,32T-64T:128G,64T-:180G"
  

• 	else
  

• 		ck_cmdline="2G-4G:384M,4G-16G:512M,16G-64G:1G,64G-128G:2G,128G-:4G"
  

• 	fi
  

• fi
  

• fi
• ck_cmdline=${ck_cmdline//-:/-102400T:} sys_mem=$(get_system_size)

• _ck_cmdline=$(kdump_get_arch_recommend_crashkernel is_fadump_capable)

^^^^^^^^^^^^^^^^^ passing is_fadump_capable looks false to me. Is this a leftover from a previous version?

get_recommend_size "$sys_mem" "$ck_cmdline"

s/$ck_cmdline/$_ck_cmdline/ ?

Thanks Philipp

}

On Thu, Dec 09, 2021 at 06:56:06PM +0100, Philipp Rudo wrote:

On Wed, 8 Dec 2021 10:25:30 +0800 Coiby Xu coxu@redhat.com wrote:

...

Provide "kdumpctl get_default_crashkernel" for kdump_anaconda_addon

s/get_default_crashkernel/get-default-crashkernel/

I'll fix in v3, thanks!

Thanks Philipp

...

so crashkernel.default isn't needed.

When fadump is on, kdump_anaconda_addon would need to specify the dump mode, i.e. "kdumpctl get-default-crashkernel fadump".

This interface would also be used by RPM scriptlet [1] to fetch default crashkernel value.

[1] https://docs.fedoraproject.org/en-US/packaging-guidelines/Scriptlets/

Signed-off-by: Coiby Xu coxu@redhat.com

kdumpctl | 10 ++++++++++ 1 file changed, 10 insertions(+)

diff --git a/kdumpctl b/kdumpctl index 59ec068..1938968 100755 --- a/kdumpctl +++ b/kdumpctl @@ -1299,6 +1299,13 @@ do_estimate() fi }

+get_default_crashkernel() +{

• local _dump_mode=$1
• echo -n "$(kdump_get_arch_recommend_crashkernel "$_dump_mode")"

+}

reset_crashkernel() { local kernel=$1 entry crashkernel_default @@ -1392,6 +1399,9 @@ main() estimate) do_estimate ;;

• get-default-crashkernel)

• get_default_crashkernel "$2"
  

• ;;
  

  reset-crashkernel) reset_crashkernel "$2" ;;

On Thu, Dec 09, 2021 at 06:57:03PM +0100, Philipp Rudo wrote:

Hi Coiby,

On Wed, 8 Dec 2021 10:25:32 +0800 Coiby Xu coxu@redhat.com wrote:

...

Add a helper function to get dump mode. The dump mode would be

• fadump if fadump=on or fadump=nocma
• kdump if fadump=off or empty fadump

Otherwise return 1.

Also add another helper function to return a kernel's dump mode.

Signed-off-by: Coiby Xu coxu@redhat.com

kdumpctl | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+)

diff --git a/kdumpctl b/kdumpctl index 2f05c76..f42a8c6 100755 --- a/kdumpctl +++ b/kdumpctl @@ -1317,6 +1317,40 @@ get_grub_kernel_boot_parameter() grubby --info="$_kernel_path" | sed -En -e "/^args=.*$/{s/^.*(\s|")${_para}=(\S*).*"$/\2/p;q}" }

+# get dump mode by fadump value +# return +# - fadump, if fadump=on or fadump=nocma +# - kdump, if fadump=off or empty fadump, return kdump +# - error if otherwise +get_dump_mode_by_fadump_val() +{

• local _fadump_val=$1
• if [[ -z $_fadump_val ]] || [[ $_fadump_val == off ]]; then

• echo -n kdump
  

• elif [[ $_fadump_val == on ]] || [[ $_fadump_val == nocma ]]; then

• echo -n fadump
  

• else

• derror "invalid fadump=$_fadump_val"
  

• retunr 1
  

s/retunr/return/

Fixed in v3, thanks!

Thanks Philipp

...

• fi

+}

+# get dump mode of a specific kernel +# based on its fadump kernel cmdline parameter +get_dump_mode_by_kernel() +{

• local _kernel_path=$1 _fadump_val _dump_mode
• _fadump_val=$(get_grub_kernel_boot_parameter "$_kernel_path" fadump)
• if _dump_mode=$(get_dump_mode_by_fadump_val "$_fadump_val"); then

• echo -n "$_dump_mode"
  

• else

• derror "failed to get dump mode for kernel $_kernel_path"
  

• exit
  

• fi

+}

reset_crashkernel() { local kernel=$1 entry crashkernel_default

On Thu, Dec 16, 2021 at 01:00:05PM +0800, Pingfan Liu wrote:

On Wed, Dec 08, 2021 at 10:25:32AM +0800, Coiby Xu wrote:

...

Add a helper function to get dump mode. The dump mode would be

• fadump if fadump=on or fadump=nocma
• kdump if fadump=off or empty fadump

Otherwise return 1.

Also add another helper function to return a kernel's dump mode.

Signed-off-by: Coiby Xu coxu@redhat.com

kdumpctl | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+)

diff --git a/kdumpctl b/kdumpctl index 2f05c76..f42a8c6 100755 --- a/kdumpctl +++ b/kdumpctl @@ -1317,6 +1317,40 @@ get_grub_kernel_boot_parameter() grubby --info="$_kernel_path" | sed -En -e "/^args=.*$/{s/^.*(\s|")${_para}=(\S*).*"$/\2/p;q}" }

+# get dump mode by fadump value +# return +# - fadump, if fadump=on or fadump=nocma +# - kdump, if fadump=off or empty fadump, return kdump +# - error if otherwise +get_dump_mode_by_fadump_val() +{

• local _fadump_val=$1
• if [[ -z $_fadump_val ]] || [[ $_fadump_val == off ]]; then

• echo -n kdump
  

• elif [[ $_fadump_val == on ]] || [[ $_fadump_val == nocma ]]; then

• echo -n fadump
  

• else

• derror "invalid fadump=$_fadump_val"
  

• retunr 1
  

• fi

+}

+# get dump mode of a specific kernel +# based on its fadump kernel cmdline parameter +get_dump_mode_by_kernel()

Is it better named as get_dump_mode_by_kernel_cmdline()?

Thanks for the suggestion! But renaming it to get_dump_mode_by_kernel_cmdline would suggest the reader to pass the kernel command line to this function.

Or it will be confused with something in live system by polling sysfs, say, FADUMP_REGISTER_SYS_NODE="/sys/kernel/fadump_registered"

For live system, I notice there is "current" in the function name, e.g. check_current_fadump_status. So maybe this is not a issue at all.

Thanks,

Pingfan

Hi Coiby,

On Wed, 8 Dec 2021 10:25:33 +0800 Coiby Xu coxu@redhat.com wrote:

grubby --info=kernel-path or --add-kernel=kernel-path accepts a kernel path (e.g. /boot/vmlinuz-5.14.14-200.fc34.x86_64) instead of kernel release (e.g 5.14.14-200.fc34.x86_64). So we need to know the kernel path given a kernel release. Although for Fedora/RHEL, the kernel path is "/boot/vmlinuz-<KERNEL_RELEASE>", a path kernel could also be /boot/<machine-id>/<KERNEL_RELEASE>/vmlinuz. So the most reliable way to find the kernel path given a kernel release is to use "grubby --info".

Note these helper functions doesn't support CoreOS/Atomic/Silverblue since grubby isn't used by them.

Signed-off-by: Coiby Xu coxu@redhat.com

kdumpctl | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+)

diff --git a/kdumpctl b/kdumpctl index f42a8c6..0b8dc0f 100755 --- a/kdumpctl +++ b/kdumpctl @@ -1351,6 +1351,36 @@ get_dump_mode_by_kernel() fi }

+_filter_grubby_kernel_str() +{

• local _grubby_kernel_str=$1 _kernel_path
• _kernel_path=${_grubby_kernel_str#kernel=}
• _kernel_path=${_kernel_path#"}
• _kernel_path=${_kernel_path%"}
• echo -n "$_kernel_path"

+}

+_find_kernel_path_by_release() +{

• local _release="$1" _grubby_kernel_str _kernel_path
• _grubby_kernel_str=$(grubby --info ALL | grep "^kernel=.*$_release")
• _kernel_path=$(_filter_grubby_kernel_str "$_grubby_kernel_str")
• [[ -e "$_kernel_path" ]] || derror "kernel $_release doesn't exist" && return 1
• echo -n "$_kernel_path"

+}

+_get_current_running_kernel_path() +{

• local _release _path
• _release=$(uname -r)
• if _path=$(_find_kernel_path_by_release "$_release"); then

•    echo -n "$_path"
  

• else

•    return 1
  

• fi

+}

I don't see a benefit in having _get_current_kernel_path.

_find_kernel_path_by_release $(uname -r)

does exactly the same but is in my opinion much easier to understand.

Thanks Philipp

reset_crashkernel() { local kernel=$1 entry crashkernel_default

On Fri, 10 Dec 2021 11:31:32 +0800 Coiby Xu coxu@redhat.com wrote:

Hi Philipp,

On Thu, Dec 09, 2021 at 06:57:28PM +0100, Philipp Rudo wrote:

...

Hi Coiby,

On Wed, 8 Dec 2021 10:25:33 +0800 Coiby Xu coxu@redhat.com wrote:

...

grubby --info=kernel-path or --add-kernel=kernel-path accepts a kernel path (e.g. /boot/vmlinuz-5.14.14-200.fc34.x86_64) instead of kernel release (e.g 5.14.14-200.fc34.x86_64). So we need to know the kernel path given a kernel release. Although for Fedora/RHEL, the kernel path is "/boot/vmlinuz-<KERNEL_RELEASE>", a path kernel could also be /boot/<machine-id>/<KERNEL_RELEASE>/vmlinuz. So the most reliable way to find the kernel path given a kernel release is to use "grubby --info".

Note these helper functions doesn't support CoreOS/Atomic/Silverblue since grubby isn't used by them.

Signed-off-by: Coiby Xu coxu@redhat.com

kdumpctl | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+)

diff --git a/kdumpctl b/kdumpctl index f42a8c6..0b8dc0f 100755 --- a/kdumpctl +++ b/kdumpctl @@ -1351,6 +1351,36 @@ get_dump_mode_by_kernel() fi }

+_filter_grubby_kernel_str() +{

• local _grubby_kernel_str=$1 _kernel_path
• _kernel_path=${_grubby_kernel_str#kernel=}
• _kernel_path=${_kernel_path#"}
• _kernel_path=${_kernel_path%"}
• echo -n "$_kernel_path"

+}

+_find_kernel_path_by_release() +{

• local _release="$1" _grubby_kernel_str _kernel_path
• _grubby_kernel_str=$(grubby --info ALL | grep "^kernel=.*$_release")
• _kernel_path=$(_filter_grubby_kernel_str "$_grubby_kernel_str")
• [[ -e "$_kernel_path" ]] || derror "kernel $_release doesn't exist" && return 1
• echo -n "$_kernel_path"

+}

+_get_current_running_kernel_path() +{

• local _release _path
• _release=$(uname -r)
• if _path=$(_find_kernel_path_by_release "$_release"); then

•    echo -n "$_path"
  

• else

•    return 1
  

• fi

+}

I don't see a benefit in having _get_current_kernel_path.

_find_kernel_path_by_release $(uname -r)

does exactly the same but is in my opinion much easier to understand.

Currently, there are two places that this function is needed. And there is a special case that's waiting to be dealt with. That's setting the kernel crashkernel when osbuild is building an system image e.g. .qcow2 and the package is installed inside a sandbox. In this case, current running kernel doesn't exist. So that's why I abstract it to a function here.

Sure, when this function helpts with the implementation of the CoreOS support it totally makes sense to keep it.

Thanks for the explanation Philipp

[1] https://bugzilla.redhat.com/show_bug.cgi?id=2024976

...

Thanks Philipp

...

reset_crashkernel() { local kernel=$1 entry crashkernel_default

Hi Coiby,

On Wed, 8 Dec 2021 10:25:34 +0800 Coiby Xu coxu@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 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
• if --kernel=DEFAULT, the default boot kernel is chosen
• if --kernel=ALL, all kernels would have its crashkernel reset to the default value

--fadump=[on|off|nocma] would be specified optionally to toggle fadump on/off. If it's not specified, the dump mode would be determined based on the fadump kernel cmdline parameter read from grubby and for --kernel=ALL, all installed kernels would be iterated to tell its dump mode kernel by kernel.

Just for my understanding. You cannot combine option --fadump together with --kernel=ALL. Meaning you can only turn fadump on/off for specific kernels but not for all. Is that correct? Is that intended?

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@redhat.com

kdumpctl | 113 +++++++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 89 insertions(+), 24 deletions(-)

diff --git a/kdumpctl b/kdumpctl index 0b8dc0f..26358d2 100755 --- a/kdumpctl +++ b/kdumpctl @@ -1381,41 +1381,105 @@ _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 _grubby_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 --replace="crashkernel=$crashkernel_default"
  

• 	rpm-ostree --replace="crashkernel=$_crashkernel"
  

  else

• 	rpm-ostree --append="crashkernel=$crashkernel_default"
  

• 	rpm-ostree --append="crashkernel=$_crashkernel"
  

  fi else

• entry=$(grubby --info ALL | grep "^kernel=.*$kernel")
  

• entry=${entry#kernel=}
  

• entry=${entry#\"}
  

• entry=${entry%\"}
  

• if [[ -f $grub_etc_default ]]; then
  

• 	sed -i -e "s/^\(GRUB_CMDLINE_LINUX=.*\)crashkernel=[^\ \"]*\([\ \"].*\)$/\1$crashkernel_default\2/" "$grub_etc_default"
  

• fi
  

• [[ -f /etc/zipl.conf ]] && zipl_arg="--zipl"

• grubby --args "$crashkernel_default" --update-kernel "$entry" $zipl_arg
  

• grubby --args "crashkernel=$_crashkernel" --update-kernel "$_grubby_kernel_path" $zipl_arg
  

  [[ $zipl_arg ]] && zipl > /dev/null

you need to be careful here. On s390 any change made by grubby won't take effect until zipl gets called. So ...

• if [[ $_dump_mode == kdump ]]; then
  

• 	grubby --remove-args="fadump" --update-kernel "$_grubby_kernel_path"
  

• else
  

• 	grubby --args="fadump=$_fadump_val" --update-kernel "$_grubby_kernel_path"
  

• fi
  

... these changes would be ignored on the next boot (unless the user calls zipl manually). This doesn't make a difference here as s390 doesn't support fadump. Nevertheless I think the call of zipl should be moved to to the end.

fi }

+_valid_grubby_kernel_path() +{

• grubby --info="$1" > /dev/null 2>&1

Just noticed that this also returns true for $1 being empty. So this should be

[[ -n "$1" ]] && grubby ...

+}

+_get_all_kernels_from_grubby() +{

• local _kernels _line _kernel_path
• for _line in $(grubby --info ALL | grep "^kernel="); do

• _kernel_path=$(_filter_grubby_kernel_str "$_line")
  

• _kernels="$_kernels $_kernel_path"
  

• done
• echo -n "$_kernels"

+}

+reset_crashkernel() +{

• local _opt _val _dump_mode _fadump_val _reboot _grubby_kernel_path _kernel_crashkernel _kernel
• 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
• if [[ -z $_grubby_kernel_path ]] &&

• ! _grubby_kernel_path=$(_get_current_running_kernel_path); then
  

• 	derror "no running kernel found"
  

• 	exit 1
  

• fi

I see multiple problems with the rest of the function.

• if [[ -z $_dump_mode ]]; then

• if [[ $_grubby_kernel_path == ALL ]]; then
  

• 	for _kernel in $(_get_all_kernels_from_grubby); do
  

• 	_dump_mode=$(get_dump_mode_by_kernel "$_kernel")
  

• 	_fadump_val=$(get_grub_kernel_boot_parameter "$_kernel" fadump)
  

• 	_update_grub "$_grubby_kernel_path" "$_kernel_crashkernel" "$_dump_mode" "$_fadump_val"
  

* missing indentation

* $_kernel_crashkernel is used uninitialized.

• 	done
  

• 	[[ $_reboot == yes ]] && reboot
  

• 	dwarn "For kernel=$_kernel, crashkernel=$_kernel_crashkernel now. Please reboot the system to take effect."
  

* the warning is only printed for the last kernel

* the warning is printed even when nothing changed e.g. if kernel already had the recommended crashkernel

• 	return
  

• else
  

• 	_dump_mode=$(get_dump_mode_by_kernel "$_kernel")
  

• 	_fadump_val=$(get_grub_kernel_boot_parameter "$_kernel" fadump)
  

• fi
  

• fi
• _kernel_crashkernel=$(kdump_get_arch_recommend_crashkernel "$_dump_mode")
• _update_grub "$_grubby_kernel_path" "$_kernel_crashkernel" "$_dump_mode" "$_fadump_val"
• [[ $_reboot == yes ]] && reboot
• dwarn "For kernel=$_grubby_kernel_path, crashkernel=$_kernel_crashkernel now. Please reboot the system to take effect."

* same like above

* all in all I don't understand why you don't simply use a single loop

In my opinion the function should look like this (not tested)

_grubby_get_kernel() { # use sed to modify all lines in case $1=ALL grubby --info="$1" | sed -n -e 's/^kernel="(.*)"/\1/p' }

reset_crashkernel() { local _opt _reboot _grubby_kernel_path _kernel local _dump_mode __dump_mode _fadump_val __fadump_val local _new_crashkernel _old_crashkernel

_grubby_kernel_path=$(_find_kernel_path_by_release "$(uname -r)")

for _opt in "$@"; do case "$_opt" in --fadump=*) _fadump_val=${_opt#*=} if ! _dump_mode=$(get_dump_mode_by_fadump_val $_fadump_val); then derror "failed to determine dump mode" exit fi ;; --kernel=*) _grubby_kernel_path=${_opt#*=} ;; --reboot) _reboot=yes ;; *) derror "$_opt not recognized" exit 1 ;; esac done

if ! _valid_grubby_kernel_path $_grubby_kernel_path; then derror "Invalid --kernel=$_grubby_kernel_path provided." derror "Please specify a valid kernel path, ALL or DEFAULT" exit fi

# in case the two options cannot be combined. # Error handling is just an example. if [[ $_grubby_kernel_path == ALL ]] && [[ -n $_dump_mode ]]; then dwarn "Cannot combine --kernel=ALL with option --fadump" _dump_mode="" fi

while read _kernel; do if [[ -z $_dump_mode ]]; then __dump_mode=$(get_dump_mode_by_kernel "$_kernel") __fadump_val=$(get_grub_kernel_boot_parameter "$_kernel" fadump) else __dump_mode=$_dump_mode __fadump_val=$_fadump_val fi

_new_crashkernel=$(kdump_get_arch_recommend_crashkernel "$__dump_mode") _old_crashkernel=$(get_grub_kernel_boot_parameter "$_kernel" "crashkernel") [[ $_new_crashkernel == $_old_crashkernel ]] && continue

ret=$(_update_grub "$_grubby_kernel_path" "$_kernel_crashkernel" "$__dump_mode" "$__fadump_val")

if [[ $ret ]]; then dwarn "Updated crashkernel=$_kernel_crashkernel for kernel=$_grubby_kernel_path." _kernel_updated=1 fi done < <(_grubby_get_kernel $_grubby_kernel_path)

[[ $_reboot == yes ]] && reboot [[ $_kernel_updated -eq 1 ]] && dwarn " Please reboot the system for the updates to take effect." }

Thanks Philipp

+}

if [[ ! -f $KDUMP_CONFIG_FILE ]]; then derror "Error: No kdump config file found!" exit 1 @@ -1478,7 +1542,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}"

On Fri, 10 Dec 2021 11:16:32 +0800 Coiby Xu coxu@redhat.com wrote:

Hi Philipp,

Thanks for reviewing the patch!

On Thu, Dec 09, 2021 at 06:58:23PM +0100, Philipp Rudo wrote:

...

Hi Coiby,

On Wed, 8 Dec 2021 10:25:34 +0800 Coiby Xu coxu@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 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
• if --kernel=DEFAULT, the default boot kernel is chosen
• if --kernel=ALL, all kernels would have its crashkernel reset to the default value

--fadump=[on|off|nocma] would be specified optionally to toggle fadump on/off. If it's not specified, the dump mode would be determined based on the fadump kernel cmdline parameter read from grubby and for --kernel=ALL, all installed kernels would be iterated to tell its dump mode kernel by kernel.

Just for my understanding. You cannot combine option --fadump together with --kernel=ALL. Meaning you can only turn fadump on/off for specific kernels but not for all. Is that correct? Is that intended?

Sorry for not clear enough. We can combine option --fadump with --kernel=ALL and this feature comes at no cost thanks to "grubby --update-kernel=ALL". But if the user doesn't specify the fadump parameter, we can't make use of "grubby --update-kernel=ALL" because some kernels could have fadump enabled while some not. In this case, we should reset crashkernel kernel by kernel.

Sorry, I totally misinterpreted the code which made me think that the two options cannot be combined. But after looking at it again I noticed I was wrong. Sorry for the confusion.

Nevertheless the wording in the paragraph is a little bit confusing. How about

"--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."

...

...

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@redhat.com

kdumpctl | 113 +++++++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 89 insertions(+), 24 deletions(-)

diff --git a/kdumpctl b/kdumpctl index 0b8dc0f..26358d2 100755 --- a/kdumpctl +++ b/kdumpctl @@ -1381,41 +1381,105 @@ _get_current_running_kernel_path()

[...]

...

• all in all I don't understand why you don't simply use a single loop

As explained in the beginning, I want to make use of "grubby --update-kernel= ALL". But when --fadump is not specified, this needs to be treated as a special case. I'll see if there is a more elegant solution for this special case.

Personally I still think the single loop as sketched below is more elegant as it simplifies the code significantly. It requires a few more calls to grubby though. But as the code isn't performance critical I think that is not a big problem.

Anyway, it's just my personal preference.

Thanks Philipp

...

In my opinion the function should look like this (not tested)

_grubby_get_kernel() { # use sed to modify all lines in case $1=ALL grubby --info="$1" | sed -n -e 's/^kernel="(.*)"/\1/p' }

reset_crashkernel() { local _opt _reboot _grubby_kernel_path _kernel local _dump_mode __dump_mode _fadump_val __fadump_val local _new_crashkernel _old_crashkernel

   _grubby_kernel_path=$(_find_kernel_path_by_release "$(uname -r)")

   for _opt in "$@"; do
           case "$_opt" in
                   --fadump=*)
                           _fadump_val=${_opt#*=}
                           if ! _dump_mode=$(get_dump_mode_by_fadump_val $_fadump_val); then
                                   derror "failed to determine dump mode"
                                   exit
                           fi
                           ;;
                   --kernel=*)
                           _grubby_kernel_path=${_opt#*=}
                           ;;
                   --reboot)
                           _reboot=yes
                           ;;
                   *)
                           derror "$_opt not recognized"
                           exit 1
                           ;;
           esac
   done

   if ! _valid_grubby_kernel_path $_grubby_kernel_path; then
           derror "Invalid --kernel=$_grubby_kernel_path provided."
           derror "Please specify a valid kernel path, ALL or DEFAULT"
           exit
   fi

   # in case the two options cannot be combined.
   # Error handling is just an example.
   if [[ $_grubby_kernel_path == ALL ]] && [[ -n $_dump_mode ]]; then
           dwarn "Cannot combine --kernel=ALL with option --fadump"
           _dump_mode=""
   fi

   while read _kernel; do
           if [[ -z $_dump_mode ]]; then
                   __dump_mode=$(get_dump_mode_by_kernel "$_kernel")
                   __fadump_val=$(get_grub_kernel_boot_parameter "$_kernel" fadump)
           else
                   __dump_mode=$_dump_mode
                   __fadump_val=$_fadump_val
           fi

           _new_crashkernel=$(kdump_get_arch_recommend_crashkernel "$__dump_mode")
           _old_crashkernel=$(get_grub_kernel_boot_parameter "$_kernel" "crashkernel")
           [[ $_new_crashkernel == $_old_crashkernel ]] && continue

           ret=$(_update_grub "$_grubby_kernel_path" "$_kernel_crashkernel" "$__dump_mode" "$__fadump_val")

           if [[ $ret ]]; then
                   dwarn "Updated crashkernel=$_kernel_crashkernel for kernel=$_grubby_kernel_path."
                   _kernel_updated=1
           fi
   done < <(_grubby_get_kernel $_grubby_kernel_path)

   [[ $_reboot == yes ]] && reboot
   [[ $_kernel_updated -eq 1 ]] && dwarn " Please reboot the system for the updates to take effect."

}

Thanks Philipp

...

+}

if [[ ! -f $KDUMP_CONFIG_FILE ]]; then derror "Error: No kdump config file found!" exit 1 @@ -1478,7 +1542,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}"

On Wed, 8 Dec 2021 10:25:36 +0800 Coiby Xu coxu@redhat.com wrote:

kexec-tools could update the default crashkernel value. When auto_reset_crashkernel=yes, reset kernel to new crashkernel value in the following two cases,

• crashkernel=auto is found in the kernel cmdline
• the kernel crashkernel was previously set by kexec-tools i.e. the kernel is using old default crashkernel value

To tell if the user is using a custom value for the kernel crashkernel or not, we assume the user would never use the default crashkernel value as custom value. When kexec-tools gets updated,

1. save the default crashkernel value of the older package to /tmp/crashkernel (for POWER system, /tmp/crashkernel_fadump is saved as well).
2. If auto_reset_crashkernel=yes, iterate all installed kernels. For each kernel, compare its crashkernel value with the old default crashkernel and reset it if yes

The implementation makes use of two RPM scriptlets [2],

• %pre is run before a package is installed so we can use it to save old default crashkernel value
• %post is run after a package installed so we can use it to try to reset kernel crashkernel

Note latest shellcheck (0.8.0) gives false positives about the associative array as of this commit. And Fedora's shellcheck is 0.7.2 and can't even correctly parse the shell code because of the associative array.

[1] https://github.com/koalaman/shellcheck/issues/2399 [2] https://docs.fedoraproject.org/en-US/packaging-guidelines/Scriptlets/

Signed-off-by: Coiby Xu coxu@redhat.com

kdumpctl | 35 +++++++++++++++++++++++++++++++++++ kexec-tools.spec | 15 +++++++++++++++ 2 files changed, 50 insertions(+)

diff --git a/kdumpctl b/kdumpctl index 26358d2..070f777 100755 --- a/kdumpctl +++ b/kdumpctl @@ -1480,6 +1480,36 @@ reset_crashkernel() dwarn "For kernel=$_grubby_kernel_path, crashkernel=$_kernel_crashkernel now. Please reboot the system to take effect." }

+# shellcheck disable=SC2154 # false positive when dereferencing an array +reset_crashkernel_after_update() +{

• local _kernel _crashkernel _dump_mode _fadump_val _old_default_crashkernel _new_default_crashkernel
• declare -A _crashkernel_vals
• _crashkernel_vals[old_kdump]=$(cat /tmp/old_default_crashkernel 2>/dev/null)
• _crashkernel_vals[old_fadump]=$(cat /tmp/old_default_crashkernel_fadump 2>/dev/null)
• _crashkernel_vals[new_kdump]=$(get_default_crashkernel kdump)
• _crashkernel_vals[new_fadump]=$(get_default_crashkernel fadump)

Are you planning to use the array more often for the CoreOS support? Because at the moment I find it a little bit overkill for what it does. Especially as you still need variables for _{old,new}_default_crashkernel. So having this array is somewhat redundant at the moment.

• for _kernel in $(_get_all_kernels_from_grubby); do

• _crashkernel=$(get_grub_kernel_boot_parameter "$_kernel" crashkernel)
  

• if [[ $_crashkernel == auto ]]; then
  

• 	reset_crashkernel "--kernel=$_kernel"
  

• elif [[ -n $_crashkernel ]]; then
  

• 	_dump_mode=$(get_dump_mode_by_kernel "$_kernel")
  

• 	_old_default_crashkernel=${_crashkernel_vals[old_${_dump_mode}]}
  

• 	_new_default_crashkernel=${_crashkernel_vals[new_${_dump_mode}]}
  

• 	if [[ $_crashkernel == "$_old_default_crashkernel" ]] &&
  

• 		 [[ $_new_default_crashkernel != "$_old_default_crashkernel" ]]; then
  

• 		_fadump_val=$(get_grub_kernel_boot_parameter "$_kernel" fadump)
  

• 		if _update_grub "$_kernel" "$_new_default_crashkernel" "$_dump_mode" "$_fadump_val"; then
  

• 			echo "For kernel=$_kernel, crashkernel=$_new_default_crashkernel now."
  

• 		fi
  

• 	fi
  

• fi
  

• done

+}

if [[ ! -f $KDUMP_CONFIG_FILE ]]; then derror "Error: No kdump config file found!" exit 1 @@ -1545,6 +1575,11 @@ main() shift reset_crashkernel "$@" ;;

• reset-crashkernel-after-update)

• if [[ $(kdump_get_conf_val auto_reset_crashkernel) != no ]]; then
  

• 	reset_crashkernel_after_update
  

• fi
  

• ;;
  

  *) dinfo $"Usage: $0 {estimate|start|stop|status|restart|reload|rebuild|reset-crashkernel|propagate|showmem}" exit 1

diff --git a/kexec-tools.spec b/kexec-tools.spec index ab7f41f..e346062 100644 --- a/kexec-tools.spec +++ b/kexec-tools.spec @@ -258,6 +258,14 @@ chmod 755 $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/99zz-fadumpini mkdir -p $RPM_BUILD_ROOT/%{dracutlibdir}/modules.d/ mv $RPM_BUILD_ROOT/etc/kdump-adv-conf/kdump_dracut_modules/* $RPM_BUILD_ROOT/%{dracutlibdir}/modules.d/

+%pre +if [ $1 == 2 ] && grep "get-default-crashkernel" /usr/bin/kdumpctl &>/dev/null; then

• kdumpctl get-default-crashkernel > /tmp/old_default_crashkernel 2>/dev/null

+%ifarch ppc64 ppc64le

• kdumpctl get_default_crashkernel fadump > /tmp/old_default_crashkernel_fadump 2>/dev/null

^^^^^^^^^^^^^^^^^^^^^^^ s/_/-/g

Thanks Philipp

+%endif +fi

%post # Initial installation %systemd_post kdump.service @@ -290,6 +298,13 @@ then /etc/sysconfig/kdump > /etc/sysconfig/kdump.new mv /etc/sysconfig/kdump.new /etc/sysconfig/kdump fi +if [ $1 == 2 ]; then

• kdumpctl reset-crashkernel-after-update
• rm /tmp/old_default_crashkernel 2>/dev/null

+%ifarch ppc64 ppc64le

• rm /tmp/old_default_crashkernel_fadump 2>/dev/null

+%endif +fi

%postun

On Mon, Dec 13, 2021 at 04:13:26PM +0100, Philipp Rudo wrote:

Hi Coiby,

On Wed, 8 Dec 2021 10:25:37 +0800 Coiby Xu coxu@redhat.com wrote:

...

When kexec-tools updates the default crashkernel value, it will try to reset the existing installed kernels including the currently running kernel. So the running kernel could have different kernel cmdline parameters from /proc/cmdline. When installing a kernel after updating kexec-tools, /usr/lib/kernel/install.d/20-grub.install would be called by kernel-install [1] which would use /proc/cmdline to set up new kernel's cmdline. To address this special case, reset the new kernel's crashkernel and fadump value to the value that would be used by running kernel after rebooting by the installation hook. One side effect of this commit is it would reset the installed kernel's crashkernel even currently running kernel don't use the default crashkernel value after rebooting. But I think this side effect is a benefit for the user.

The implementation depends on kernel-install which run the scripts in /usr/lib/kernel/install.d passing the following arguments,

add KERNEL-VERSION $BOOT/MACHINE-ID/KERNEL-VERSION/ KERNEL-IMAGE [INITRD-FILE ...]

these twp lines...

...

An concrete example is given as follows, add 5.11.12-300.fc34.x86_64 /lib/modules/5.11.12-300.fc34.x86_64/vmlinuz /boot/e986846f63134c7295458cf36300ba5b/5.11.12-300.fc34.x86_64

... don't match. At least the image location was moved by one.

These two issues have been fixed in v3, thanks!

...

Since rpm-ostree ignores all kernel hooks, this method doesn't work for CoreOS/Atomic/Silverblue. A collaboration between rpm-ostree and kexec-tools is needed [2].

Note the crashkernel.default support is dropped.

[1] https://www.freedesktop.org/software/systemd/man/kernel-install.html [2] https://github.com/coreos/rpm-ostree/issues/2894

Signed-off-by: Coiby Xu coxu@redhat.com

92-crashkernel.install | 135 +---------------------------------------- kdumpctl | 31 ++++++++++ 2 files changed, 32 insertions(+), 134 deletions(-)

diff --git a/92-crashkernel.install b/92-crashkernel.install index 78365ff..54edc8a 100755 --- a/92-crashkernel.install +++ b/92-crashkernel.install @@ -5,142 +5,9 @@ KERNEL_VERSION="$2" KDUMP_INITRD_DIR_ABS="$3" KERNEL_IMAGE="$4"

-grub_etc_default="/etc/default/grub"

-ver_lt() {

• [[ "$(echo -e "$1\n$2" | sort -V)" == $1$'\n'* ]] && [[ $1 != "$2" ]]

-}

-# Read crashkernel= value in /etc/default/grub -get_grub_etc_ck() {

• [[ -e $grub_etc_default ]] && \
• sed -n -e "s/^GRUB_CMDLINE_LINUX=.*(crashkernel=[^\ "]*)[\ "].*$/\1/p" $grub_etc_default

-}

-# Read crashkernel.default value of specified kernel -get_ck_default() {

• ck_file="/usr/lib/modules/$1/crashkernel.default"
• [[ -f "$ck_file" ]] && cat "$ck_file"

-}

-# Iterate installed kernels, find the kernel with the highest version that has a -# valid crashkernel.default file, exclude current installing/removing kernel -# -# $1: a string representing a crashkernel= cmdline. If given, will also check the -# content of crashkernel.default, only crashkernel.default with the same value will match -get_highest_ck_default_kver() {

• for kernel in $(find /usr/lib/modules -maxdepth 1 -mindepth 1 -printf "%f\n" | sort --version-sort -r); do

• [[ $kernel == "$KERNEL_VERSION" ]] && continue
  

• [[ -s "/usr/lib/modules/$kernel/crashkernel.default" ]] || continue
  

• echo "$kernel"
  

• return 0
  

• done
• return 1

-}

-set_grub_ck() {

• sed -i -e "s/^(GRUB_CMDLINE_LINUX=.*)crashkernel=[^\ "]*([\ "].*)$/\1$1\2/" "$grub_etc_default"

-}

-# Set specified kernel's crashkernel cmdline value -set_kernel_ck() {

• kernel=$1
• ck_cmdline=$2
• entry=$(grubby --info ALL | grep "^kernel=.*$kernel")
• entry=${entry#kernel=}
• entry=${entry#"}
• entry=${entry%"}
• if [[ -z "$entry" ]]; then

• echo "$0: failed to find boot entry for kernel $kernel"
  

• return 1
  

• fi
• [[ -f /etc/zipl.conf ]] && zipl_arg="--zipl"
• grubby --args "$ck_cmdline" --update-kernel "$entry" $zipl_arg
• [[ $zipl_arg ]] && zipl > /dev/null ||:

-}

case "$COMMAND" in add)

• # - If current boot kernel is using default crashkernel value, update
• # installing kernel's crashkernel value to its default value,
• # - If intalling a higher version kernel, and /etc/default/grub's
• # crashkernel value is using default value, update it to installing
• # kernel's default value.
• inst_ck_default=$(get_ck_default "$KERNEL_VERSION")
• # If installing kernel doesn't have crashkernel.default, just exit.
• [[ -z "$inst_ck_default" ]] && exit 0
• boot_kernel=$(uname -r)
• boot_ck_cmdline=$(sed -n -e "s/^.*(crashkernel=\S*).*$/\1/p" /proc/cmdline)
• highest_ck_default_kver=$(get_highest_ck_default_kver)
• highest_ck_default=$(get_ck_default "$highest_ck_default_kver")
• # Try update /etc/default/grub if present, else grub2-mkconfig could
• # override crashkernel value.
• grub_etc_ck=$(get_grub_etc_ck)
• if [[ -n "$grub_etc_ck" ]]; then

• if [[ -z "$highest_ck_default_kver" ]]; then
  

• 	# None of installed kernel have a crashkernel.default,
  

• 	# check for 'crashkernel=auto' in case of legacy kernel
  

• 	[[ "$grub_etc_ck" == "crashkernel=auto" ]] && \
  

• 		set_grub_ck "$inst_ck_default"
  

• else
  

• 	# There is a valid crashkernel.default, check if installing kernel
  

• 	# have a higher version and grub config is using default value
  

• 	ver_lt "$highest_ck_default_kver" "$KERNEL_VERSION" && \
  

• 		[[ "$grub_etc_ck" == "$highest_ck_default" ]] && \
  

• 		[[ "$grub_etc_ck" != "$inst_ck_default" ]] && \
  

• 		set_grub_ck "$inst_ck_default"
  

• fi
  

• fi
• # Exit if crashkernel is not used in current cmdline
• [[ -z $boot_ck_cmdline ]] && exit 0
• # Get current boot kernel's default value
• boot_ck_default=$(get_ck_default "$boot_kernel")
• if [[ $boot_ck_cmdline == "crashkernel=auto" ]]; then

• # Legacy RHEL kernel defaults to "auto"
  

• boot_ck_default="$boot_ck_cmdline"
  

• fi
• # If boot kernel doesn't have a crashkernel.default, check
• # if it's using any installed kernel's crashkernel.default
• if [[ -z $boot_ck_default ]]; then

• [[ $(get_highest_ck_default_kver "$boot_ck_cmdline") ]] && boot_ck_default="$boot_ck_cmdline"
  

• fi
• # If boot kernel is using a default crashkernel, update
• # installing kernel's crashkernel to new default value
• if [[ "$boot_ck_cmdline" != "$inst_ck_default" ]] && [[ "$boot_ck_cmdline" == "$boot_ck_default" ]]; then

• set_kernel_ck "$KERNEL_VERSION" "$inst_ck_default"
  

• fi
• exit 0
• ;;

-remove)

• # If grub default value is upgraded when this kernel was installed, try downgrade it
• grub_etc_ck=$(get_grub_etc_ck)
• [[ $grub_etc_ck ]] || exit 0
• removing_ck_conf=$(get_ck_default "$KERNEL_VERSION")
• [[ $removing_ck_conf ]] || exit 0
• highest_ck_default_kver=$(get_highest_ck_default_kver) || exit 0
• highest_ck_default=$(get_ck_default "$highest_ck_default_kver")
• [[ $highest_ck_default ]] || exit 0
• if ver_lt "$highest_ck_default_kver" "$KERNEL_VERSION"; then

• if [[ $grub_etc_ck == "$removing_ck_conf" ]] && [[ $grub_etc_ck != "$highest_ck_default" ]]; then
  

• 	set_grub_ck "$highest_ck_default"
  

• fi
  

• fi

• kdumpctl reset-crashkernel-for-installed_kernel "$KERNEL_VERSION"

wrong indentation.

Thanks Philipp

...

exit 0 ;; esac diff --git a/kdumpctl b/kdumpctl index 070f777..3af62b0 100755 --- a/kdumpctl +++ b/kdumpctl @@ -1510,6 +1510,32 @@ reset_crashkernel_after_update() done }

+reset_crashkernel_for_installed_kernel() +{

• local _installed_kernel _running_kernel _crashkernel _crashkernel_running
• local _dump_mode_running _fadump_val_running
• if ! _installed_kernel=$(_find_kernel_path_by_release "$1"); then

• exit 1
  

• fi
• if ! _running_kernel=$(_get_current_running_kernel_path); then

• derror "Couldn't find current running kernel"
  

• exit
  

• fi
• _crashkernel=$(get_grub_kernel_boot_parameter "$_installed_kernel" crashkernel)
• _crashkernel_running=$(get_grub_kernel_boot_parameter "$_running_kernel" crashkernel)
• _dump_mode_running=$(get_dump_mode_by_kernel "$_running_kernel")
• _fadump_val_running=$(get_grub_kernel_boot_parameter "$_kernel" fadump)
• if [[ $_crashkernel != "$_crashkernel_running" ]]; then

• 	if _update_grub "$_installed_kernel" "$_crashkernel_running" "$_dump_mode_running" "$_fadump_val_running"; then
  

• 		echo "kexec-tools has reset $_installed_kernel to use the new default crashkernel value $_crashkernel_running"
  

• 	fi
  

• fi

+}

if [[ ! -f $KDUMP_CONFIG_FILE ]]; then derror "Error: No kdump config file found!" exit 1 @@ -1580,6 +1606,11 @@ main() reset_crashkernel_after_update fi ;;

• reset-crashkernel-for-installed_kernel)

• if [[ $(kdump_get_conf_val auto_reset_crashkernel) != no ]]; then
  

• 	reset_crashkernel_for_installed_kernel "$2"
  

• fi
  

• ;;
  

  *) dinfo $"Usage: $0 {estimate|start|stop|status|restart|reload|rebuild|reset-crashkernel|propagate|showmem}" exit 1