Hi Philipp,
On Wed, Dec 22, 2021 at 05:29:22PM +0100, Philipp Rudo wrote:
Hi Coiby,
On Thu, 16 Dec 2021 14:36:52 +0800
Coiby Xu <coxu(a)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
>
> There are several problems when running kdumpctl in the RPM scripts
> for CoreOS/Atomic/Silverblue, for example, the lock can't be acquired by
> kdumpctl, "rpm-ostree kargs" can't be run and etc.. So don't enable
this
> feature for CoreOS/Atomic/Silverblue.
>
> 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(a)redhat.com>
> ---
> kdumpctl | 35 +++++++++++++++++++++++++++++++++++
> kexec-tools.spec | 15 +++++++++++++++
> 2 files changed, 50 insertions(+)
>
> diff --git a/kdumpctl b/kdumpctl
> index 412420d..a6582f8 100755
> --- a/kdumpctl
> +++ b/kdumpctl
> @@ -1534,6 +1534,36 @@ reset_crashkernel()
> [[ $_reboot == yes && $_crashkernel_changed == yes ]] && reboot
> }
>
> +# 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)
> +
> + 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
> @@ -1599,6 +1629,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..d72044a 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 ! grep -q "ostree" /proc/cmdline && [ $1 == 2 ] && grep
"get-default-crashkernel" /usr/bin/kdumpctl &>/dev/null; then
do you really need to check if get-default-crashkernel exists here?
What would happen if kdumpctl returns with an error?
I'm just asking myself as there is no corresponding check in the %post
scriplet. Furthermore I think we can assume that the spec file and
kdumpctl are updated together. Especially that using a new spec file
with an old kdumpctl that doesn't support get-default-crashkernel is
not a supported use case. Or am I missing something?
Using using a new spec file with an old kdumpctl does happen for the %pre
RPM scriplet since this scriptlet is called before the installation thus
the old kdumpctl is called by the %pre scriptlet. But there is indeed no
need to check if get-default-crashkernel exists since the case of an empty
/tmp/default_crashkernel file could be correctly taken care of. I'll
drop this check in v4.
Thanks
Philipp
> + kdumpctl get-default-crashkernel kdump > /tmp/old_default_crashkernel
2>/dev/null
> +%ifarch ppc64 ppc64le
> + kdumpctl get-default-crashkernel fadump > /tmp/old_default_crashkernel_fadump
2>/dev/null
> +%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 ! grep -q "ostree" /proc/cmdline && [ $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
--
Best regards,
Coiby