Call kdumpctl setup-crypttab so kdump will work on boot for encrypted dump target#41
Conversation
There was a problem hiding this comment.
Summary of Changes
Hello @coiby, 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 enhances kdump functionality for systems with encrypted targets by integrating the "kdumpctl setup_crypttab" command into the Anaconda installation workflow. This ensures that the necessary "/etc/crypttab" configuration is in place and included in the initramfs, enabling kdump to work correctly on boot. Additionally, it improves the robustness of the kdump installation process by conditionally skipping the task in environments lacking "systemd" and refines the display of encryption-related warnings in the GUI based on the system's architecture.
Highlights
- Kdump Crypttab Setup: The pull request introduces a call to "kdumpctl setup_crypttab" during the Anaconda installation process. This ensures that "/etc/crypttab" is correctly configured, allowing volume keys to be passed to the crash kernel for encrypted targets.
- Installation Phase Timing: The "setup_crypttab" call is strategically placed within the "Anaconda addon configuration" phase, which occurs before initramfs generation. This guarantees that the updated "crypttab" is included in the generated initramfs.
- Systemd Check for Kdump Task: A new check has been added to the "KdumpInstallationTask" to determine if "systemd" (specifically "systemctl") is installed. If "systemd" is not found (e.g., in minimal container images), the kdump installation task is skipped to prevent errors.
- Architecture-Specific Encryption Warning: The GUI's encryption warning for kdump has been updated to only display if the system's architecture is not "x86_64". This implies that the warning is less relevant or handled differently on "x86_64" systems.
Using Gemini Code Assist
The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in pull request comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `com_redhat_kdump/service/installation.py:146-148` </location>
<code_context>
+ # Anaconda may be used to create minimal container image which doesn't
+ # have systemd installed
+ # https://issues.redhat.com/browse/RHEL-41082?focusedId=26969576&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-26969576
+ if not shutil.which(self._sysroot / "usr/bin/systemctl"):
+ log.debug("systemd not installed, skip KdumpInstallationTask")
+ return
</code_context>
<issue_to_address>
**suggestion:** Consider using Path objects consistently for systemctl path check.
shutil.which expects a string executable name, not a Path object or full path. Convert the Path to a string or use os.path.exists for direct path existence checks.
```suggestion
systemctl_path = self._sysroot / "usr/bin/systemctl"
if not os.path.exists(systemctl_path):
log.debug("systemd not installed, skip KdumpInstallationTask")
return
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Code Review
This pull request correctly adds a call to kdumpctl setup_crypttab to improve kdump support for encrypted targets, and includes a necessary check for systemd's presence. My main feedback concerns an inconsistency in how a related warning is handled for different architectures in the GUI. Additionally, it is critical to update the unit tests to cover the new logic in KdumpInstallationTask, as the current tests in test/unit_tests/test_installation.py appear to be outdated and do not cover the new code paths. Please add tests for the kdumpctl call and the systemd existence check.
278eaa9 to
bdde0a2
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new task, KdumpCrypttabSetupTask, to configure /etc/crypttab for kdump, allowing it to work with encrypted targets. The new task is correctly integrated into the installation process and is accompanied by a comprehensive set of unit tests. However, I've identified a significant issue in a related change within KdumpInstallationTask where the path to systemctl is constructed incorrectly, which could lead to the task being skipped. Please see my detailed comment.
The early patch checks sysroot/systemctl by mistake. Fix it. Fixes: 25c549c ("Handle the case where systemd isn't installed") Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
… dump target Resolves: https://issues.redhat.com/browse/RHEL-29039 Call "kdumpctl setup-crypttab" to set up /etc/crypttab so the volume keys can be passed to the crash kernel. Note Anaconda writes to /etc/crypttab at "Early storage configuration" phase. So we set up /etc/crypttab at "Anaconda addon configuration" phase which happens before Anaconda generates initramfs. So the updated crypttab will be built into the initramfs. 01:52:19,877 INF installation: Queue started: Early storage configuration (3/18) ... ... 01:56:17,041 INF installation: Queue started: Anaconda addon configuration (15/18) 01:56:17,044 INF installation: Queue started: Initramfs generation (16/18) One benefit of setting up crypttab via kdump is "kdumpctl setup-crypttab" will continue only the dumping target is truly encrypted and the logic to detect encrypted dumping target is already there. After all architectures supports encrypted dump target, the current implementing of detecting if any volume has been encrypted which may not necessary be a dump target can be dropped. Assisted-by: Claude Code Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
Resolves: https://issues.redhat.com/browse/RHEL-29039 x86_64 now supports encrypted dump target. There is no need for this warning. Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
Use latest Fedora container image and install dependent packages on the fly. Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
Resolves: https://issues.redhat.com/browse/RHEL-29039
Call "kdumpctl setup-crypttab" to set up /etc/crypttab so the volume
keys can be passed to the crash kernel.
Note Anaconda writes to /etc/crypttab at "Early storage configuration"
phase. So we set up /etc/crypttab at "Anaconda addon configuration"
phase which happens before Anaconda generates initramfs. So the updated
crypttab will be built into the initramfs.