Hi Pratyush,
Thanks for the review. Please see my comments inline:
On Wed, Apr 5, 2017 at 9:26 AM, Pratyush Anand <panand(a)redhat.com> wrote:
Hi Bhupesh,
Thanks for working on it.
On Wednesday 05 April 2017 12:44 AM, Bhupesh Sharma wrote:
>
> This patch introduces the 'force_no_rebuild' option
> inside the 'kdump.conf' and its handling inside the 'kdumpctl'
> script.
>
> There might be several use cases, where a system admin
> decides that he doesn't need to rebuild the kdump initrd
> and wants to use an existing version of the same. In such cases,
> he can set the 'force_no_rebuild' option inside 'kdump.conf'
> to 1, to force the 'kdumpctl' script not to rebuild the kdump
> initrd.
>
> This change also handles existing issues with kdump initrd
> rebuild, when the /boot (and/or /root) are set as read-only
> by a system admin. In such cases, this patch proposes the
> following approach to decide as to whether the kdump initrd needs to be
> rebuild:
>
> -------------------------------------------------------------------------
> | Options |Val| /boot is RO | /boot is WRITABLE |
> | | | | |
> -------------------------------------------------------------------------
> | force_rebuild | 0 |WARN that kdump initrd |kdumpctl decides if|
> | | |will not be rebuilt |kdump initrd |
> | | |but proceed with |rebuild in required|
> | | |loading the kdump | |
> | | |kernel | |
> -------------------------------------------------------------------------
> | force_rebuild | 1 |Bail out with |Proceed with kdump |
> | | |an ERROR |initrd rebuild |
> -------------------------------------------------------------------------
> | force_no_rebuild | 0 |Check force_rebuild value and act |
> | | |accordingly |
> -------------------------------------------------------------------------
> | force_no_rebuild | 1 |WARN that kdump initrd |Do not rebuild |
> | | |will not be rebuilt |kdump initrd |
> | | |but proceed with | |
> | | |loading the kdump | |
> | | |kernel | |
> -------------------------------------------------------------------------
I think, when force_no_rebuild = 1, then we do not need to check /boot.
We just not rebuild it and return success (0).
Right, I was proposing to WARN the user/system admin atleast, that
since force_no_rebuild = 1 and /boot is RO, we are not going to
rebuild the kdump initrd.
But on other thoughts, we can make the WARN message generic and echo
the same whenever force_no_rebuild = 1
Will change the same in v2.
>
> Signed-off-by: Bhupesh Sharma <bhsharma(a)redhat.com>
> ---
> kdump.conf | 8 +++++
> kdump.conf.5 | 10 ++++++
> kdumpctl | 100
> ++++++++++++++++++++++++++++++++++++++---------------------
> 3 files changed, 82 insertions(+), 36 deletions(-)
>
> diff --git a/kdump.conf b/kdump.conf
> index ebf9587a6522..18bbd4ace971 100644
> --- a/kdump.conf
> +++ b/kdump.conf
> @@ -115,6 +115,13 @@
> # Specify 1 to force rebuilding kdump initrd every time when
> kdump
> # service starts.
> #
> +# force_no_rebuild <0 | 1>
> +# - By default, kdump initrd will only be rebuilt when
> necessary.
> +# Specify 1 to force no rebuild of kdump initrd.
> +#
> +# force_no_rebuild and force_rebuild options are mutually
> exclusive and
> +# both should not be set to 1 simultaneously.
> +#
> # override_resettable <0 | 1>
> # - Usually an unresettable block device can't be a dump
> target.
> # Specifying 1 when you want to dump even though the block
> @@ -150,6 +157,7 @@ core_collector makedumpfile -l --message-level 1 -d 31
> #extra_modules gfs2
> #default shell
> #force_rebuild 1
> +#force_no_rebuild 1
> #dracut_args --omit-drivers "cfg80211 snd" --add-drivers "ext2
ext3"
> #fence_kdump_args -p 7410 -f auto -c 0 -i 10
> #fence_kdump_nodes node1 node2
> diff --git a/kdump.conf.5 b/kdump.conf.5
> index ca427699be53..265a67f5d575 100644
> --- a/kdump.conf.5
> +++ b/kdump.conf.5
> @@ -166,6 +166,16 @@ By default, kdump initrd will only be rebuilt when
> necessary.
> Specify 1 to force rebuilding kdump initrd every time when kdump service
> starts.
> .RE
>
> +.B force_no_rebuild <0 | 1>
> +.RS
> +By default, kdump initrd will only be rebuilt when necessary.
> +Specify 1 to force no rebuild of kdump initrd.
> +
> +.PP
> +force_no_rebuild and force_rebuild options are mutually exclusive and
> +both should not be set to 1 simultaneously.
> +.RE
> +
> .B override_resettable <0 | 1>
> .RS
> Usually an unresettable block device can't be a dump target. Specifying 1
> means
> diff --git a/kdumpctl b/kdumpctl
> index 4d68be0c292d..01c62d1b6782 100755
> --- a/kdumpctl
> +++ b/kdumpctl
> @@ -354,7 +354,7 @@ check_config()
> case "$config_opt" in
> \#* | "")
> ;;
> -
>
raw|ext2|ext3|ext4|minix|btrfs|xfs|nfs|ssh|sshkey|path|core_collector|kdump_post|kdump_pre|extra_bins|extra_modules|default|force_rebuild|dracut_args|fence_kdump_args|fence_kdump_nodes)
> +
>
raw|ext2|ext3|ext4|minix|btrfs|xfs|nfs|ssh|sshkey|path|core_collector|kdump_post|kdump_pre|extra_bins|extra_modules|default|force_rebuild|force_no_rebuild|dracut_args|fence_kdump_args|fence_kdump_nodes)
> [ -z "$config_val" ] && {
> echo "Invalid kdump config value for
> option $config_opt."
> return 1;
> @@ -610,6 +610,7 @@ check_rebuild()
> local extra_modules
> local _force_rebuild force_rebuild="0"
> local ret system_modified="0"
> + local _force_no_rebuild force_no_rebuild="0"
>
> check_boot_dir
>
> @@ -625,6 +626,15 @@ check_rebuild()
> return 1
> fi
>
> + _force_no_rebuild=`grep ^force_no_rebuild $KDUMP_CONFIG_FILE
> 2>/dev/null`
> + if [ $? -eq 0 ]; then
> + force_no_rebuild=`echo $_force_no_rebuild | cut -d' '
> -f2`
> + if [ "$force_no_rebuild" != "0" ] && [
"$force_no_rebuild"
> != "1" ];then
> + echo "Error: force_no_rebuild value is invalid"
> + return 1
> + fi
> + fi
> +
> _force_rebuild=`grep ^force_rebuild $KDUMP_CONFIG_FILE
> 2>/dev/null`
> if [ $? -eq 0 ]; then
> force_rebuild=`echo $_force_rebuild | cut -d' ' -f2`
I think,we should have following simple modification just after it.
+ if [ "$force_no_rebuild" = "1" ]; then
+ if [ "$force_rebuild" = "1" ]; then
+ echo "Error: both force_rebuild and force_no_rebuild
cannot be set in kdump.conf"
+ return 1;
+ else
+ return 0;
+ fi
+ fi
and then probably we do not need any furher modification. What do you say?
Hmm. This leaves us with one corner case where:
force_rebuild = 0 && force_no_rebuild ==0 && /boot is RO
In this case what should be the preferred approach?
Any pointers?
Regards,
Bhupesh
> ~Pratyush
>
>> @@ -634,45 +644,63 @@ check_rebuild()
>> fi
>> fi
>>
>> - #will rebuild every time if extra_modules are specified
>> - extra_modules=`grep ^extra_modules $KDUMP_CONFIG_FILE`
>> - [ -n "$extra_modules" ] && force_rebuild="1"
>> -
>> - #check to see if dependent files has been modified
>> - #since last build of the image file
>> - if [ -f $TARGET_INITRD ]; then
>> - image_time=`stat -c "%Y" $TARGET_INITRD
2>/dev/null`
>> - fi
>> + if [ "$force_no_rebuild" == "1" ]; then
>> + if [ "$force_rebuild" == "1" ]; then
>> + echo "Error: both force_rebuild and
>> force_no_rebuild cannot be set in kdump.conf"
>> + exit 1
>> + else
>> + if [[ ! -w "$KDUMP_BOOTDIR" ]];then
>> + echo "$KDUMP_BOOTDIR does not have write
>> permission. Will not rebuild $TARGET_INITRD"
>> + fi
>>
>> - check_system_modified
>> - ret=$?
>> - if [ $ret -eq 2 ]; then
>> - return 1
>> - elif [ $ret -eq 1 ];then
>> - system_modified="1"
>> - fi
>> -
>> - if is_mode_switched; then
>> - echo -n "Dump mode changed from last boot."; echo
>> - handle_mode_switch
>> - elif [ $image_time -eq 0 ]; then
>> - echo -n "No kdump initial ramdisk found."; echo
>> - elif [ "$force_rebuild" != "0" ]; then
>> - echo -n "Force rebuild $TARGET_INITRD"; echo
>> - elif [ "$system_modified" != "0" ]; then
>> - :
>> + return 0
>> + fi
>> else
>> - return 0
>> - fi
>> + if [[ ! -w "$KDUMP_BOOTDIR" ]]; then
>> + if [ "$force_rebuild" == "1" ];then
>> + echo "$KDUMP_BOOTDIR does not have write
>> permission. Can not rebuild $TARGET_INITRD"
>> + return 1
>> + else
>> + echo "$KDUMP_BOOTDIR does not have write
>> permission. Will not rebuild $TARGET_INITRD"
>> + return 0
>> + fi
>> + fi
>>
>> - if [[ ! -w "$KDUMP_BOOTDIR" ]];then
>> - echo "$KDUMP_BOOTDIR does not have write permission. Can
>> not rebuild $TARGET_INITRD"
>> - return 1
>> - fi
>> + #will rebuild every time if extra_modules are specified
>> + extra_modules=`grep ^extra_modules $KDUMP_CONFIG_FILE`
>> + [ -n "$extra_modules" ] &&
force_rebuild="1"
>>
>> - echo "Rebuilding $TARGET_INITRD"
>> - rebuild_initrd
>> - return $?
>> + #check to see if dependent files has been modified
>> + #since last build of the image file
>> + if [ -f $TARGET_INITRD ]; then
>> + image_time=`stat -c "%Y" $TARGET_INITRD
>> 2>/dev/null`
>> + fi
>> +
>> + check_system_modified
>> + ret=$?
>> + if [ $ret -eq 2 ]; then
>> + return 1
>> + elif [ $ret -eq 1 ];then
>> + system_modified="1"
>> + fi
>> +
>> + if is_mode_switched; then
>> + echo -n "Dump mode changed from last boot.";
echo
>> + handle_mode_switch
>> + elif [ $image_time -eq 0 ]; then
>> + echo -n "No kdump initial ramdisk found.";
echo
>> + elif [ "$force_rebuild" != "0" ]; then
>> + echo -n "Force rebuild $TARGET_INITRD"; echo
>> + elif [ "$system_modified" != "0" ]; then
>> + :
>> + else
>> + return 0
>> + fi
>> +
>> + echo "Rebuilding $TARGET_INITRD"
>> + rebuild_initrd
>> + return $?
>> + fi
>> }
>>
>> # This function check iomem and determines if we have more than
>>
>