Make swap size configurable #3882
Conversation
Allow configuration of the swap size via /etc/default/haos-swapfile file. By setting the SWAPSIZE variable in this file, swapfile get recreated on the next reboot to the defined size. Size can be either in bytes or with optional units (B/K/M/G, accepting some variations but always interpreted as power of 10). The size is then rounded to 4k block size. If no override is defined or the value can't be parsed, it falls back to previously used 33% of system RAM. Fixes #968
📝 WalkthroughWalkthroughThis pull request introduces a new systemd mount unit to bind Changes
Sequence Diagram(s)sequenceDiagram
participant O as mnt-overlay.mount
participant ED as etc-default.mount
participant DD as mnt-data.mount
participant HSS as haos-swapfile.service
participant GFS as [email protected]
O->>ED: Activate dependency
ED->>HSS: Required for service startup
DD->>HSS: Must be active
HSS->>GFS: Executes after mounts are active
sequenceDiagram
participant HS as haos-swapfile (script)
participant Config as /etc/default/haos-swapfile
participant FS as /mnt/data/swapfile
HS->>Config: Source swap configuration
HS->>HS: Compute swap size using size2kilobytes()
HS->>FS: Check if swap file exists and its size
alt Swap file missing or size below minimum
HS->>FS: Create new swap file or remove existing one
else
HS->>FS: Use existing swap file
end
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (5)
buildroot-external/rootfs-overlay/usr/libexec/haos-swapfile (2)
22-29: Consider adding upper and lower bounds for swap size.While the script handles small sizes (<40kB) by disabling swap, there's no upper limit check. Consider adding a maximum size limit to prevent excessive swap allocation.
if [ -z "${SWAPSIZE}" ] || [ "${SWAPSIZE}" = "-1" ]; then # Default to 33% of total memory SWAPSIZE="$(awk '/MemTotal/{ print int($2 * 0.33) }' /proc/meminfo)" echo "[INFO] Using default swapsize of 33% RAM (${SWAPSIZE} kB)" +else + # Maximum swap size: 32GB (in kB) + MAX_SWAP=$((32 * 1024 * 1024)) + if [ "${SWAPSIZE}" -gt "${MAX_SWAP}" ]; then + echo "[WARN] Requested swap size exceeds 32GB, capping at maximum" + SWAPSIZE="${MAX_SWAP}" + fi fi
45-55: Enhance error handling for swap file creation.The swap file creation process could benefit from additional error handling and logging:
- The
ddcommand should usestatus=progressfor better feedback- Consider adding error handling for the
ddcommandecho "[INFO] Creating swapfile of size ${SWAPSIZE} kB (rounded to ${SWAPSIZE_BLOCKS} blocks)" umask 0077 -dd if=/dev/zero of="${SWAPFILE}" bs=4k count="${SWAPSIZE_BLOCKS}" +if ! dd if=/dev/zero of="${SWAPFILE}" bs=4k count="${SWAPSIZE_BLOCKS}" status=progress; then + echo "[ERROR] Failed to create swapfile" + rm -f "${SWAPFILE}" + exit 1 +fibuildroot-external/rootfs-overlay/usr/lib/systemd/system/mnt-data-swapfile.swap (1)
1-4: Consider adding additional conditions for swap activation.The current condition checks if the swap file is not empty, but we might want to add more conditions to ensure system stability:
- Check if there's enough memory available
- Verify the filesystem is not read-only
[Unit] Description=HAOS swap file ConditionFileNotEmpty=/mnt/data/swapfile +ConditionPathIsReadWrite=/mnt/data +AssertPathExists=/proc/meminfotests/smoke_test/test_basic.py (2)
81-92: LGTM! Consider adding additional verifications for robustness.The test correctly verifies that the swap size can be customized. However, consider adding these checks for improved robustness:
- Verify that the swap file exists before testing
- Verify that the swap is active using
swapon --show- Add a timeout for the reboot to prevent test flakiness
@pytest.mark.dependency(depends=["test_init"]) def test_custom_swap_size(shell, target): + # Verify swap file exists and is active + assert shell.run_check("test -f /mnt/data/swapfile") + assert shell.run_check("swapon --show") + output = shell.run_check("stat -c '%s' /mnt/data/swapfile") # set new swap size to half of the previous size - round to 4k blocks new_swap_size = (int(output[0]) // 2 // 4096) * 4096 shell.run_check(f"echo 'SWAPSIZE={new_swap_size/1024/1024}M' > /etc/default/haos-swapfile; reboot") + + # Wait for reboot with timeout + sleep(30) # Adjust timeout as needed + # reactivate ShellDriver to handle login again target.deactivate(shell) target.activate(shell) + + # Verify swap is active with new size output = shell.run_check("stat -c '%s' /mnt/data/swapfile") assert int(output[0]) == new_swap_size, f"Incorrect swap size {new_swap_size}B: {output}" + assert shell.run_check("swapon --show"), "Swap is not active"
94-104: LGTM! Consider adding additional verifications for robustness.The test correctly verifies that the swap can be disabled. However, consider adding these checks for improved robustness:
- Verify that the swap file is actually removed
- Add a timeout for the reboot to prevent test flakiness
@pytest.mark.dependency(depends=["test_custom_swap_size"]) def test_no_swap(shell, target): output = shell.run_check("echo 'SWAPSIZE=0' > /etc/default/haos-swapfile; reboot") + + # Wait for reboot with timeout + sleep(30) # Adjust timeout as needed + # reactivate ShellDriver to handle login again target.deactivate(shell) target.activate(shell) + output = shell.run_check("systemctl --no-pager -l list-units --state=failed") assert "0 loaded units listed." in output, f"Some units failed:\n{"\n".join(output)}" + swapon = shell.run_check("swapon --show") assert swapon == [], f"Swapfile still exists: {swapon}" + assert not shell.run_check("test -f /mnt/data/swapfile"), "Swap file not removed"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
buildroot-external/rootfs-overlay/usr/lib/systemd/system/etc-default.mount(1 hunks)buildroot-external/rootfs-overlay/usr/lib/systemd/system/haos-swapfile.service(1 hunks)buildroot-external/rootfs-overlay/usr/lib/systemd/system/mnt-data-swapfile.swap(1 hunks)buildroot-external/rootfs-overlay/usr/libexec/haos-swapfile(1 hunks)tests/smoke_test/test_basic.py(1 hunks)
🔇 Additional comments (4)
buildroot-external/rootfs-overlay/usr/libexec/haos-swapfile (2)
4-14: LGTM: Well-implemented size conversion function.The
size2kilobytesfunction effectively handles various size formats (B, K, M, G) with case-insensitive matching and proper power-of-10 conversions. The implementation using awk is robust and handles edge cases well.
16-19:❓ Verification inconclusive
Verify configuration file permissions.
The script sources
/etc/default/haos-swapfilebut doesn't verify its permissions. Since this file affects system-wide swap settings, we should ensure it has appropriate permissions.
🏁 Script executed:
#!/bin/bash # Check permissions of the config file in the rootfs overlay find buildroot-external/rootfs-overlay/etc/default -type f -name "haos-swapfile" -ls 2>/dev/null # Check if there's a default config file with preset permissions find buildroot-external/package -type f -name "haos-swapfile" -ls 2>/dev/nullLength of output: 150
Action Required: Manually Verify Configuration File Permissions
The script sources
/etc/default/haos-swapfilewithout checking its file permissions, and our automated search did not return any instance of this file. This leaves ambiguity about whether a default configuration file is present and, if so, whether it has appropriate permissions.
- Manual Check: Please verify on your system if
/etc/default/haos-swapfileexists.- Permission Verification: If it exists, ensure that its permissions are properly restricted (e.g., owner-readable and non-writable by unauthorized users) to avoid unintentional system-wide swap configuration changes.
buildroot-external/rootfs-overlay/usr/lib/systemd/system/etc-default.mount (1)
12-14: LGTM: Appropriate target dependency.The mount unit is correctly configured to be wanted by
hassos-bind.target, ensuring proper integration with the system's mount hierarchy.buildroot-external/rootfs-overlay/usr/lib/systemd/system/haos-swapfile.service (1)
4-5: LGTM! Dependencies are correctly ordered.The service dependencies have been properly updated to ensure that
/etc/defaultis mounted before the swap file service starts, which is necessary for reading the swap size configuration.
buildroot-external/rootfs-overlay/usr/lib/systemd/system/etc-default.mount
Show resolved
Hide resolved
Add /io/hass/os/Config/Swap object that exposes interface with properties controlling the swap size and swappiness configuration added in [1] and [2]. There are no checks whether this supported on the target system (while swappiness would probably have effect also on Supervised installs, swap size can't be controlled there). These checks should be implemented on upper layer (i.e. Supervisor). [1] home-assistant/operating-system#3882 [2] home-assistant/operating-system#3884
Add /io/hass/os/Config/Swap object that exposes interface with properties controlling the swap size and swappiness configuration added in [1] and [2]. There are no checks whether this supported on the target system (while swappiness would probably have effect also on Supervised installs, swap size can't be controlled there). These checks should be implemented on upper layer (i.e. Supervisor). [1] home-assistant/operating-system#3882 [2] home-assistant/operating-system#3884
* Add DBus node for swap size and swappiness config Add /io/hass/os/Config/Swap object that exposes interface with properties controlling the swap size and swappiness configuration added in [1] and [2]. There are no checks whether this supported on the target system (while swappiness would probably have effect also on Supervised installs, swap size can't be controlled there). These checks should be implemented on upper layer (i.e. Supervisor). [1] home-assistant/operating-system#3882 [2] home-assistant/operating-system#3884 * Read default/current swappiness from procfs
Allow configuration of the swap size via /etc/default/haos-swapfile file. By setting the SWAPSIZE variable in this file, swapfile get recreated on the next reboot to the defined size. Size can be either in bytes or with optional units (B/K/M/G, accepting some variations but always interpreted as power of 10). The size is then rounded to 4k block size. If no override is defined or the value can't be parsed, it falls back to previously used 33% of system RAM.
Fixes #968
Summary by CodeRabbit
New Features
Tests