Hi Philipp,
On Fri, May 26, 2023 at 05:05:04PM +0200, Philipp Rudo wrote:
Hi Coiby,
All in all looks good. One tiny nit and one comment below.
Anyway
Reviewed-by: Philipp Rudo <prudo(a)redhat.com>
Thanks for the code review! I've merged the patch set with a few small
changes to this last patch.
There are two groups of examples i.e. when kexec-tools and kernel are
updated respectively. I want two groups to run setup and cleanup before
the first example and after the last example respectively. So for the
merged patch, I made the following changes to align with my intension,
- use "mktmp -u" to avoid creating a temp folder
- Put BeforeAll in both group examples. But it doesn't work as
expected, so the workaround of manually calling setup is still
needed. I will file a ticket to confirm if it's a bug.
Btw, it seems BeforeEach/AfterEach will run the setup before each
example so it's not I want. You'll see the following error and the test
failures if BeforeEach/AfterEach is used,
.Fgrep: /tmp/spec_test.Oim1lAoVrF/env_temp: No such file or directory
Unexpected output to stderr occurred at line 80-84 in
'spec/kdumpctl_manage_crashkernel_spec.sh'
..F.
6 examples, 2 failures
But at least it seems to be consistent with the documentation,
You can specify commands to be executed before / after each example
by BeforeEach (Before), AfterEach (After).
On Wed, 24 May 2023 18:18:16 +0800
Coiby Xu <coxu(a)redhat.com> wrote:
> Signed-off-by: Coiby Xu <coxu(a)redhat.com>
> ---
> - mock GRUB_CFG and use --config-file=$GRUB_CFG to avoid polluting the
> test envrioment [Philipp]
> - Add a note for why including a test for catching the crashkernel=no
> hint [Philipp]
> ---
> kdumpctl | 3 +-
> spec/kdumpctl_manage_crashkernel_spec.sh | 106 +++++++++++++++++++++++
> 2 files changed, 108 insertions(+), 1 deletion(-)
> create mode 100644 spec/kdumpctl_manage_crashkernel_spec.sh
>
> diff --git a/kdumpctl b/kdumpctl
> index 57da1afd..5043a7b2 100755
> --- a/kdumpctl
> +++ b/kdumpctl
> @@ -1666,12 +1666,13 @@ reset_crashkernel()
> fi
> }
>
> +GRUB_CFG=/boot/grub2/grub.cfg
> _is_bootloader_installed()
> {
> if [[ $(uname -m) == s390x ]]; then
> test -f /etc/zipl.conf
> else
> - test -f /boot/grub2/grub.cfg
> + test -f "$GRUB_CFG"
> fi
> }
>
> diff --git a/spec/kdumpctl_manage_crashkernel_spec.sh
b/spec/kdumpctl_manage_crashkernel_spec.sh
> new file mode 100644
> index 00000000..ae9561b8
> --- /dev/null
> +++ b/spec/kdumpctl_manage_crashkernel_spec.sh
> @@ -0,0 +1,106 @@
> +#!/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
> + new_ck=1G-4G:196M,4G-64G:256M,64G-:512M
> + KDUMP_SPEC_TEST_RUN_DIR=$(mktemp -d /tmp/spec_test.XXXXXXXXXX)
> + GRUB_CFG="$KDUMP_SPEC_TEST_RUN_DIR/grub.cfg"
> +
> + uname() {
> + if [[ $1 == '-m' ]]; then
> + echo -n x86_64
> + elif [[ $1 == '-r' ]]; then
> + echo -n $current_kernel
> + fi
> + }
> +
> + # dinfo is a bit complex for unit tets, simply mock it
> + dinfo() {
> + echo "$1"
> + }
> +
> + kdump_get_arch_recommend_crashkernel() {
> + echo -n "$new_ck"
> + }
> +
> + setup() {
> + mkdir -p "$KDUMP_SPEC_TEST_RUN_DIR"
Didn't mktemp already create the directory? I believe you can drop this
line.
> + cp -r spec/support/boot_load_entries "$KDUMP_SPEC_TEST_RUN_DIR"
> + cp spec/support/grub_env "$KDUMP_SPEC_TEST_RUN_DIR"/env_temp
> + touch "$GRUB_CFG"
> +
> + grubby --args crashkernel=$old_ck --update-kernel=$kernel1
> + grubby --args crashkernel=$new_ck --update-kernel=$kernel2
> + grubby --remove-args fadump --update-kernel=ALL
> +
> + }
> +
> + cleanup() {
> + rm -rf "$KDUMP_SPEC_TEST_RUN_DIR"
> + }
> +
> + 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 --config-file="$GRUB_CFG"
--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=$new_ck
now."
> + The output should not include "For kernel=$kernel2, crashkernel=$new_ck
now."
> + # A hint on how to turn off auto update of crashkernel
> + The output should include "auto_reset_crashkernel=no"
> + End
> +
> + Specify 'kernel1 should have crashkernel updated'
> + When call grubby --info $kernel1
> + The line 3 of output should include crashkernel="$new_ck"
> + End
> +
> + Specify 'kernel2 should also have crashkernel updated'
> + When call grubby --info $kernel2
> + The line 3 of output should include crashkernel="$new_ck"
> + End
> +
> + End
> +
> + Context "If kernel package is installed alone, "
> + # BeforeAll somehow doesn't work as expected, manually call setup to bypass
this issue.
The way I understand it BeforeAll/AfterAll is only called once
before/after the first/last Context is entered. When you want it to be
called every time before a new Context is entered/left you need
BeforeEach/AfterEach (in older versions Before/After) [1]
Thanks
Philipp
[1]
https://github.com/shellspec/shellspec#beforeeach-before-aftereach-after-...
> + setup
> + new_kernel_ver=new_kernel
> + new_kernel=/boot/vmlinuz-$new_kernel_ver
> + grubby --add-kernel=$new_kernel --initrd=/boot/initramfs-$new_kernel_ver.img
--title=$new_kernel_ver
> +
> + Specify 'reset_crashkernel_for_installed_kernel should report the new kernel
has its crashkernel updated'
> + When call reset_crashkernel_for_installed_kernel $new_kernel_ver
> + The output should include "crashkernel=$new_ck"
> + End
> +
> + Specify 'the new kernel should have crashkernel updated'
> + When call grubby --info $new_kernel
> + The output should include crashkernel="$new_ck"
> + End
> +
> + Specify 'kernel1 keeps its crashkernel value'
> + When call grubby --info $kernel1
> + The output should include crashkernel="$old_ck"
> + End
> +
> + End
> +
> + End
> +End
--
Best regards,
Coiby