Improve Kdump test suite #4148
Improve Kdump test suite #4148
Conversation
adityagesh
commented
Dec 9, 2025
adityagesh
commented
- Kdump currently has a lot of failures when sufficient disk space is not present, add disk using platform when additional space is required.
- Add fstab entry when partition used to store crashdump is not root partition - observed some cases where kdump service fails to start after rebooting due to mount issues.
- Refactoring
There was a problem hiding this comment.
Pull request overview
This pull request enhances the kdump test suite to handle insufficient disk space scenarios and improve reliability across reboots. The changes address kdump failures when systems lack sufficient disk space by dynamically adding data disks when needed, and ensure mount points persist across reboots by adding fstab entries.
Key Changes:
- Introduced a new
Fstabtool for managing/etc/fstabentries programmatically - Enhanced kdump to calculate required disk space based on system memory and dynamically provision disks when needed
- Added mount persistence logic to prevent kdump service failures after reboot
- Refactored code to use tool abstractions (Mkdir, Ls) instead of raw execute commands
- Fixed Oracle Linux grubby command to remove existing crashkernel parameters before adding new ones
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| lisa/tools/fstab.py | New tool for managing /etc/fstab entries with methods to read, add, and ensure entries with UUID support |
| lisa/tools/kdump.py | Enhanced disk space management with dynamic provisioning, added mount persistence, refactored to use tool abstractions, and improved Oracle Linux crashkernel configuration |
| lisa/tools/init.py | Exported new Fstab and FstabEntry classes |
Test Suggestions:
According to the LISA Testing Guidelines, all code reviews must include test suggestions. Here are the recommendations for this PR:
Key Test Cases:
verify_kdumpcrash_single_core|verify_kdumpcrash_smp|verify_kdumpcrash_auto_size|verify_kdumpcrash_large_memory_auto_size
Impacted LISA Features:
Disk, StartStop, SerialConsole
Tested Azure Marketplace Images:
canonical 0001-com-ubuntu-server-jammy 22_04-lts-gen2 latestredhat rhel 9_5 latestoracle oracle-linux ol94-lvm-gen2 latestdebian debian-12 12-gen2 latestmicrosoftcblmariner azure-linux-3 azure-linux-3-gen2 latest
Rationale:
- The test cases cover single-core, multi-core, auto-size crashkernel, and large memory scenarios which exercise the new disk provisioning and mount persistence logic
- Oracle Linux images are critical to test the grubby command changes for crashkernel configuration
- A variety of distributions ensures the fstab and disk management changes work across different OS families
- Gen2 images are prioritized as they represent newer VM generations commonly used in production
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
adityagesh
force-pushed
the
aditya/kdump_fixes
branch
from
da16486 to
90762b0
Compare
adityagesh
force-pushed
the
aditya/kdump_fixes
branch
from
90762b0 to
0766da2
Compare
| @property | ||
| def command(self) -> str: | ||
| # fstab is a file, not a command | ||
| return "cat" |
There was a problem hiding this comment.
Since fstab actually is not a tool, can we have this placed else where? or are we doing this solely because we want to import it in kdump tool?
Also why does it have to be cat? below we are importing cat anyway to read from the FSTAB_PATH
There was a problem hiding this comment.
It is implemented as a tool to provide an abstraction for standard linux fstab, it's not due to import in kdump tool
I don't think command can be empty, hence used cat. I'll recheck if it can be empty
| options: str = "defaults", | ||
| dump: int = 0, | ||
| pass_num: int = 2, | ||
| use_uuid: bool = True, |
There was a problem hiding this comment.
sicne this is being implemented, do you want to also add a simple option to override/force flag this is to update instead of returning from the method if an entry exists.
There was a problem hiding this comment.
I don't have that use case right now. I would prefer it to updated in that way when a use case arises
| else: | ||
| self._log.info( | ||
| f"Could not get UUID for {device}, using device name" | ||
| ) | ||
| except LisaException as e: | ||
| self._log.info( | ||
| f"Could not get UUID for {device}: {e}. Using device name" | ||
| ) |
There was a problem hiding this comment.
You have an else condition and an except for the same reasoning.
There was a problem hiding this comment.
Good catch, I'll remove the else block
There was a problem hiding this comment.
On second thoughts, this is correct.
We need the if else to handle scenario where an entry exists, but UUID is empty.
Meaning,
partition_info = blkid.get_partition_info_by_name(
device, force_run=True
)
returns without exception, but the UUID field is empty
|
|
||
| # Try to add a data disk if platform supports it | ||
| try: | ||
| return self._try_add_data_disk_for_dump(required_space_gb) |
There was a problem hiding this comment.
You had previously mentioned about exploring ways to provision the VM itself with enough disk space based on memory, is that option ruled out?
There was a problem hiding this comment.
Yeah, since kdump does not have to be configured in root disk, I feel this is a better approach.
However, the os disk size requirement would be implemented for Hibernation
