-
Notifications
You must be signed in to change notification settings - Fork 22
kdump: Introduce new kdump_emergency option and /etc/kdump/emergency.d directory interface #143
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
abfe792
88c7b76
f0add69
633cb23
777dc95
5989c82
7032bc3
c1d601b
e71635f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -660,7 +660,7 @@ kdump_install_net() { | |||||||
| fi | ||||||||
| } | ||||||||
|
|
||||||||
| # install etc/kdump/pre.d and /etc/kdump/post.d | ||||||||
| # install etc/kdump/pre.d and /etc/kdump/post.d /etc/kdump/emergency.d | ||||||||
| kdump_install_pre_post_conf() { | ||||||||
| if [[ -d /etc/kdump/pre.d ]]; then | ||||||||
| for file in /etc/kdump/pre.d/*; do | ||||||||
|
|
@@ -681,6 +681,16 @@ kdump_install_pre_post_conf() { | |||||||
| fi | ||||||||
| done | ||||||||
| fi | ||||||||
|
|
||||||||
| if [[ -d /etc/kdump/emergency.d ]]; then | ||||||||
| for file in /etc/kdump/emergency.d/*; do | ||||||||
| if [[ -x $file ]]; then | ||||||||
| dracut_install "$file" | ||||||||
| elif [[ $file != "/etc/kdump/emergency.d/*" ]]; then | ||||||||
| echo "$file is not executable" | ||||||||
|
Comment on lines
+689
to
+690
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The echo statement does not redirect to standard error, so the message will not be visible in logs. This could make it difficult to diagnose issues with the emergency scripts.
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This isn’t an emergency-time message; it can be logged during initramfs generation when kdump.service starts. |
||||||||
| fi | ||||||||
| done | ||||||||
| fi | ||||||||
| } | ||||||||
|
|
||||||||
| default_dump_target_install_conf() { | ||||||||
|
|
@@ -743,7 +753,7 @@ kdump_install_conf() { | |||||||
| kdump_collect_netif_usage "$(get_dracut_args_target "$_val")" | ||||||||
| fi | ||||||||
| ;; | ||||||||
| kdump_pre | kdump_post | extra_bins) | ||||||||
| kdump_pre | kdump_post | kdump_emergency | extra_bins) | ||||||||
| # shellcheck disable=SC2086 | ||||||||
| dracut_install $_val | ||||||||
| ;; | ||||||||
|
|
||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -90,6 +90,14 @@ generate() | |
| # For core_collector format details, you can refer to | ||
| # kexec-kdump-howto.txt or kdump.conf manpage. | ||
| # | ||
| # kdump_emergency <binary | script> | ||
| # - This directive allows you to run a specified executable | ||
| # in emergency mode within the kdump capture kernel, | ||
| # prior to the dump process. | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I find the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As you pointed out, once the kdump capture kernel enters the error handler, the dump process will never be started.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sorry for nit picking, but my understanding is that the emergency mode is entered for any error, including but not exclusively for those that occur before the dump process starts. Personally I would phrase it like this Would that be ok with you? |
||
| # All files under /etc/kdump/emergency.d are collectively sorted | ||
| # and executed in lexical order, before binary or script | ||
| # specified kdump_emergency parameter is executed. | ||
| # | ||
| # kdump_post <binary | script> | ||
| # - This directive allows you to run a executable binary | ||
| # or script after the vmcore dump process terminates. | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -368,7 +368,7 @@ parse_config() | |||||
| dwarn "Please update $KDUMP_CONFIG_FILE to use option 'failure_action' instead." | ||||||
| _set_config failure_action "$config_val" || return 1 | ||||||
| ;; | ||||||
| path | core_collector | kdump_post | kdump_pre | extra_bins | extra_modules | failure_action | final_action | force_rebuild | force_no_rebuild | fence_kdump_args | fence_kdump_nodes | auto_reset_crashkernel) ;; | ||||||
| path | core_collector | kdump_post | kdump_pre | kdump_emergency | extra_bins | extra_modules | failure_action | final_action | force_rebuild | force_no_rebuild | fence_kdump_args | fence_kdump_nodes | auto_reset_crashkernel) ;; | ||||||
|
|
||||||
| net | options | link_delay | disk_timeout | debug_mem_level | blacklist) | ||||||
| derror "Deprecated kdump config option: $config_opt. Refer to kdump.conf manpage for alternatives." | ||||||
|
|
@@ -461,9 +461,6 @@ check_files_modified() | |||||
| #also rebuild when Pacemaker cluster conf is changed and fence kdump is enabled. | ||||||
| modified_files=$(get_pcs_cluster_modified_files "$image_time") | ||||||
|
|
||||||
| EXTRA_BINS=${OPT[kdump_post]} | ||||||
| CHECK_FILES=${OPT[kdump_pre]} | ||||||
| HOOKS="/etc/kdump/post.d/ /etc/kdump/pre.d/" | ||||||
| if [[ -d /etc/kdump/post.d ]]; then | ||||||
| for file in /etc/kdump/post.d/*; do | ||||||
| if [[ -x $file ]]; then | ||||||
|
|
@@ -478,13 +475,18 @@ check_files_modified() | |||||
| fi | ||||||
| done | ||||||
| fi | ||||||
| HOOKS="$HOOKS $POST_FILES $PRE_FILES" | ||||||
| if [[ -d /etc/kdump/emergency.d ]]; then | ||||||
| for file in /etc/kdump/emergency.d/*; do | ||||||
| if [[ -x $file ]]; then | ||||||
| EMERGENCY_FILES="$EMERGENCY_FILES $file" | ||||||
| fi | ||||||
| done | ||||||
| fi | ||||||
| HOOKS="$POST_FILES $PRE_FILES $EMERGENCY_FILES" | ||||||
| CORE_COLLECTOR=$(echo "${OPT[core_collector]}" | awk '{print $1}') | ||||||
| CORE_COLLECTOR=$(type -P "$CORE_COLLECTOR") | ||||||
| # POST_FILES and PRE_FILES are already checked against executable, need not to check again. | ||||||
| EXTRA_BINS="$EXTRA_BINS $CHECK_FILES" | ||||||
| CHECK_FILES=${OPT[extra_bins]} | ||||||
| EXTRA_BINS="$EXTRA_BINS $CHECK_FILES" | ||||||
| EXTRA_BINS="${OPT[kdump_post]} ${OPT[kdump_pre]} ${OPT[kdump_emergency]} ${OPT[extra_bins]}" | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The variables
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The original code did not take into account cases where the values contain spaces or special characters. |
||||||
| files="$KDUMP_CONFIG_FILE $KDUMP_KERNEL $EXTRA_BINS $CORE_COLLECTOR" | ||||||
| [[ -e /etc/fstab ]] && files="$files /etc/fstab" | ||||||
|
|
||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
do_kdump_emergencyfunction does not have any error handling for the directory iteration. If/etc/kdump/emergency.ddoes not exist or is not a directory, the script will continue without any warning. This could lead to unexpected behavior if the administrator expects the emergency scripts to be executed.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is implemented exactly the same way as pre.d and post.d, if we fix this, we also need to apply the same fix to those as well.
I think it would be better to bundle the changes for pre.d and post.d into a separate pull request.