-
Notifications
You must be signed in to change notification settings - Fork 22
kdump-lib: check kdump service state when validating fadump load #142
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?
Conversation
Summary of ChangesHello @sourabhjains, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves an issue where the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request fixes an issue where kdumpctl status could report fadump as operational even when the kdump service was in a failed state. The fix is to check the kdump.service status in is_kernel_loaded. The change is correct and effectively solves the problem. I've added one comment suggesting a potential improvement to make the check more robust by applying it to kdump mode as well, as a similar issue could exist there.
kdump-lib.sh
Outdated
| ;; | ||
| fadump) | ||
| _sysfs="$FADUMP_REGISTER_SYS_NODE" | ||
| systemctl is-active --quiet kdump.service || return 1 |
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.
This check correctly fixes the issue for fadump. However, a similar problem could exist for the kdump mode. If kdumpctl start fails for any reason after the crash kernel is loaded via kexec, the kdump.service will be in a failed state, but /sys/kernel/kexec_crash_loaded will be 1. In this scenario, kdumpctl status would incorrectly report kdump as operational.
It would be more robust to apply this service status check for the kdump mode as well. You could consider moving this check to after the case statement to cover all dump modes.
Currently, kdumpctl status reports kdump as operational even when
the kdump service is in a failed state for fadump.
root> kdumpctl status
kdump: Dump mode is fadump
kdump: Kdump is operational
root> systemctl status kdump
× kdump.service - Crash recovery kernel arming
Loaded: loaded (/usr/lib/systemd/system/kdump.service; enabled; preset: enabled)
Active: failed (Result: exit-code) since Tue 2026-01-20 02:52:59 CST; 12s ago
<snip...>
cp: error writing '/boot/initramfs-6.12.0-180.el10.ppc64le.img.tmp': No space left on device
dracut[F]: Creation of /boot/initramfs-6.12.0-180.el10.ppc64le.img.tmp failed
<snip...>
This happens because the kernel registers fadump during early boot
[1]. Even if the kdump service later fails to start due to a failure
while updating the initrd with the fadump component,
/sys/kernel/fadump/registered still remains set to 1. As a result,
kdump is reported as operational, even though it is not.
Fix this by checking the kdump service state when validating the
fadump load.
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=607451ce0aa9bdff590db4d087173edba6d7a29d
Reported-by: Shirisha G <shirisha@linux.ibm.com>
Signed-off-by: Sourabh Jain <sourabhjain@linux.ibm.com>
a4b2163 to
36e4b14
Compare
|
/gemini review |
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.
Code Review
The pull request effectively addresses the reported issue where kdumpctl status incorrectly reported fadump as operational even when the kdump.service was in a failed state. By introducing a check for the systemctl active status of kdump.service specifically for fadump mode, the kdumpctl command now provides a more accurate status. The changes are concise, well-placed, and directly resolve the problem described in the pull request body.
Currently, kdumpctl status reports kdump as operational even when the kdump service is in a failed state for fadump.
root> kdumpctl status
kdump: Dump mode is fadump
kdump: Kdump is operational
root> systemctl status kdump
× kdump.service - Crash recovery kernel arming
Loaded: loaded (/usr/lib/systemd/system/kdump.service; enabled; preset: enabled)
Active: failed (Result: exit-code) since Tue 2026-01-20 02:52:59 CST; 12s ago
<snip...>
cp: error writing '/boot/initramfs-6.12.0-180.el10.ppc64le.img.tmp': No space left on device dracut[F]: Creation of /boot/initramfs-6.12.0-180.el10.ppc64le.img.tmp failed <snip...>
This happens because the kernel registers fadump during early boot [1]. Even if the kdump service later fails to start due to a failure while updating the initrd with the fadump component, /sys/kernel/fadump/registered still remains set to 1. As a result, kdump is reported as operational, even though it is not.
Fix this by checking the kdump service state when validating the fadump load.
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=607451ce0aa9bdff590db4d087173edba6d7a29d
Reported-by: Shirisha G shirisha@linux.ibm.com