boot: handle swap entries in /etc/fstab#537
Conversation
bmr-cymru
added
Enhancement
WalkthroughParse /etc/fstab to extract swap entries separately, avoid treating swap lines as mounts, pass the swap list into boom.command.create_entry, and adjust internal function signatures to accept an optional swaps parameter. Changes
Sequence Diagram(s)sequenceDiagram
participant Creator as create_snapset_boot_entry()
participant Mounts as _build_snapset_mount_list()
participant Swaps as _build_swap_list()
participant Maker as _create_boom_boot_entry()
participant Boom as boom.command.create_entry()
Creator->>Mounts: Read /etc/fstab (skip swap & malformed)
Mounts-->>Creator: mounts[]
Creator->>Swaps: Read /etc/fstab (collect swap entries)
Swaps-->>Creator: swaps[] ("WHAT:OPTIONS")
Creator->>Maker: Pass version,title,tag_arg,root_device,mounts,swaps
Maker->>Boom: create_entry(mounts=..., swaps=...)
Boom-->>Maker: boot_entry
Maker-->>Creator: boot_entry
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (45)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
bmr-cymru
changed the title
/etc/fstab
|
Congratulations! One of the builds has completed. 🍾 You can install the built RPMs by following these steps:
Please note that the RPMs should be used only in a testing environment. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
snapm/manager/_boot.py (1)
371-387: Document the new swaps parameter.The function signature now includes a
swapsparameter, but the docstring doesn't describe it. Please add documentation for this parameter to maintain API clarity.Apply this diff to update the docstring:
def _create_boom_boot_entry( version, title, tag_arg, root_device, mounts=None, swaps=None ): """ Create a boom boot entry according to the passed arguments. :param version: The UTS release name for the boot entry :param title: The title for the boot entry. :param tag_arg: A tag argument to be added to the kernel command line and used to associate the entry with a snapshot set name or UUID. :param root_device: The root device for the entry. Passed to root=... :param lvm_root_lv: An optional LVM2 root logical volume. :param mounts: An optional list of mount specifications to use for the boot entry. If defined fstab=no will be appended to the generated kernel command line. + :param swaps: An optional list of swap specifications to use for the + boot entry. Each entry should be in the format "WHAT:OPTIONS". """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
snapm/manager/_boot.py(4 hunks)
🧰 Additional context used
🪛 Ruff (0.14.0)
snapm/manager/_boot.py
364-364: Unpacked variable where is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (30)
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm-thin, fedora42)
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm, centos-stream10)
- GitHub Check: virt_tests (bios, lvm, fedora41)
- GitHub Check: virt_tests (uefi, lvm, fedora41)
- GitHub Check: virt_tests (bios, lvm-thin, fedora42)
- GitHub Check: virt_tests (bios, lvm-thin, fedora41)
- GitHub Check: virt_tests (bios, lvm, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm-thin, fedora41)
- GitHub Check: virt_tests (bios, lvm, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm, fedora42)
- GitHub Check: virt_tests (bios, lvm, fedora42)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream9)
🔇 Additional comments (4)
snapm/manager/_boot.py (4)
343-343: LGTM! Correctly excludes swap entries from the mount list.The modification appropriately filters out swap-type entries, preventing them from being treated as filesystem mounts. This directly addresses issue #533.
423-423: LGTM! Correctly passes swap definitions to boom.The swap list is now properly passed to
boom.command.create_entryvia theswapsparameter, addressing issue #535.
452-452: LGTM! Properly retrieves swap definitions.Swap entries are correctly extracted from
/etc/fstabbefore creating the boot entry. Ensure the error handling suggestions for_build_swap_list()are addressed.
459-459: LGTM! Completes the swap handling flow.The swap definitions are correctly passed through to the boot entry creation function, ensuring swap configurations are included in the generated boot entry.
bmr-cymru
force-pushed
the
bmr-swaps-as-swaps
branch
from
4409fb8 to
705a7bb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
snapm/manager/_boot.py (1)
391-391: Remove stale parameter from docstring.The docstring references an
lvm_root_lvparameter that does not exist in the function signature. This appears to be outdated documentation.Apply this diff:
:param root_device: The root device for the entry. Passed to root=... - :param lvm_root_lv: An optional LVM2 root logical volume. :param mounts: An optional list of mount specifications to use for the
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
snapm/manager/_boot.py(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
snapm/manager/_boot.py (1)
snapm/manager/plugins/_plugin.py (1)
_log_warn(92-96)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (29)
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: virt_tests (bios, lvm, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm-thin, fedora42)
- GitHub Check: virt_tests (uefi, lvm, centos-stream10)
- GitHub Check: virt_tests (bios, lvm, fedora41)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (bios, lvm, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm, fedora41)
- GitHub Check: virt_tests (uefi, lvm, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm-thin, fedora41)
- GitHub Check: virt_tests (bios, lvm-thin, fedora42)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream9)
- GitHub Check: virt_tests (bios, lvm-thin, fedora41)
- GitHub Check: virt_tests (bios, lvm, fedora42)
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm, fedora42)
- GitHub Check: python-basic-tests
🔇 Additional comments (3)
snapm/manager/_boot.py (3)
342-347: LGTM! Swap exclusion and validation logic implemented correctly.The added validation for malformed fstab lines and explicit exclusion of swap entries align with the PR objectives and address issues #533 and #535.
462-462: LGTM! Swap list integration implemented correctly.The integration correctly calls
_build_swap_list()to obtain swap entries and passes them to_create_boom_boot_entry()via the newswapsparameter. This properly separates swap handling from mount handling, addressing PR objectives #532, #533, and #535.Also applies to: 469-469
379-381: Verify external boom.command.create_entry API
- Confirm that the external
boom.command.create_entryfunction accepts aswapsparameter.- Remove the stale
lvm_root_lvreference from the function docstring (lines 395–396).
Related: #375 Signed-off-by: Bryn M. Reeves <bmr@redhat.com>
bmr-cymru
dismissed
coderabbitai[bot]’s stale review
It's better to have a hard & immediate failure than to try to continue operating with a missing /etc/fstab. Propagating the error and logging it is the appropriate action for now.
1 participant
Handle swap entries in /etc/fstab explicitly, rather than punting them to
boom.command.create_entry(mounts=...)to DTRT.Resolves: #532
Resolves: #533
Resolves: #534
Resolves: #535
Summary by CodeRabbit
New Features
Bug Fixes