On Tue, May 09, 2023 at 05:03:04PM +0200, Philipp Rudo wrote:
Hi Coiby,
a few nits blow.
On Tue, 9 May 2023 15:01:40 +0800
Coiby Xu <coxu(a)redhat.com> wrote:
> Signed-off-by: Coiby Xu <coxu(a)redhat.com>
> ---
> spec/kdumpctl_manage_crashkernel_spec.sh | 100 +++++++++++++++++++++++
> 1 file changed, 100 insertions(+)
> create mode 100644 spec/kdumpctl_manage_crashkernel_spec.sh
>
> diff --git a/spec/kdumpctl_manage_crashkernel_spec.sh
b/spec/kdumpctl_manage_crashkernel_spec.sh
> new file mode 100644
> index 00000000..71a9e42d
> --- /dev/null
> +++ b/spec/kdumpctl_manage_crashkernel_spec.sh
> @@ -0,0 +1,100 @@
> +#!/bin/bash
> +
> +Describe 'Management of the kernel crashkernel parameter.'
> + Include ./kdumpctl
> + kernel1=/boot/vmlinuz-5.15.6-100.fc34.x86_64
> + kernel2=/boot/vmlinuz-5.14.14-200.fc34.x86_64
> + old_ck=1G-4G:162M,4G-64G:256M,64G-:512M
> + GRUB_CFG=/boot/grub2/grub.cfg
> + KDUMP_SPEC_TEST_RUN_DIR=$(mktemp -d /tmp/spec_test.XXXXXXXXXX)
> +
> + uname() {
> + if [[ $1 == '-m' ]]; then
> + echo -n x86_64
> + elif [[ $1 == '-r' ]]; then
> + echo -n $current_kernel
> + fi
> + }
> +
> + kdump_crashkernel=$(get_default_crashkernel kdump)
There's a tiny chance this test case will fail when the default get's
changed such that kdump_crashkernel==old_ck. The proper solution would
be to choose an old and a new value and then mock
kdump_get_arch_recommend_crashkernel so it returns the new one.
Good suggestion! I'll address this case as suggested.
> + setup() {
> + mkdir -p "$KDUMP_SPEC_TEST_RUN_DIR"
> + cp -r spec/support/boot_load_entries "$KDUMP_SPEC_TEST_RUN_DIR"
> + cp spec/support/grub_env "$KDUMP_SPEC_TEST_RUN_DIR"/env_temp
> +
> + grubby --args crashkernel=$old_ck --update-kernel=$kernel1
> + grubby --args crashkernel=$kdump_crashkernel --update-kernel=$kernel2
> + grubby --remove-args fadump --update-kernel=ALL
> +
> + if [[ ! -e $GRUB_CFG ]]; then
> + touch $GRUB_CFG
> + fi
This touches the GRUB_CFG of the system you are running the test on,
doesn't it? Especially, ...
> + }
> +
> + cleanup() {
> + rm -rf "$KDUMP_SPEC_TEST_RUN_DIR"
> + rm -f $GRUB_CFG
... this removes it. I don't think that is what you want. Why don't you
create a temporary config file and add --config-file pointing to it to
the mocked grubby below instead?
Good idea!
> + }
> +
> + grubby() {
> + # - --no-etc-grub-update, not update /etc/default/grub
> + # - --bad-image-okay, don't check the validity of the image
> + # - --env, specify custom grub2 environment block file to avoid modifying
> + # the default /boot/grub2/grubenv
> + # - --bls-directory, specify custom BootLoaderSpec config files to avoid
> + # modifying the default /boot/loader/entries
> + @grubby --no-etc-grub-update --grub2 --bad-image-okay
--env="$KDUMP_SPEC_TEST_RUN_DIR"/env_temp -b
"$KDUMP_SPEC_TEST_RUN_DIR"/boot_load_entries "$@"
> + }
> +
> + Describe "When kexec-tools have its default crashkernel updated, "
> +
> + BeforeAll 'setup'
> + AfterAll 'cleanup'
> +
> + Context "if kexec-tools is updated alone, "
> + Specify 'reset_crashkernel_after_update should report updated kernels and
note that auto_reset_crashkernel=yes'
> + When call reset_crashkernel_after_update
> + The output should include "For kernel=$kernel1,
crashkernel=$kdump_crashkernel now."
> + The output should not include "For kernel=$kernel2,
crashkernel=$kdump_crashkernel now."
> + The output should include "auto_reset_crashkernel=no"
I find the last check quite confusing. First I thought that you return
the value auto_reset_crashkernel is set to. But it is only a hint on
how to turn off the auto update. Personally I would simply remove the
line.
Sorry it confuses you. Personally, I think we should let users know this
hint otherwise users may complain somehow there custom crasherkernel
doesn't persist. How about adding a comment to explain it?
Thanks
Philipp
--
Best regards,
Coiby