Add DBus node for swap size and swappiness config #222
Conversation
📝 WalkthroughWalkthroughThe changes introduce a new D-Bus interface for managing swap configuration in the system via the file Changes
Sequence Diagram(s)sequenceDiagram
participant Main
participant DBus
participant SwapService
participant TimesyncdService
Main->>DBus: Create DBus connection
Main->>SwapService: InitializeDBus(conn)
SwapService->>DBus: Export swap interface
Main->>TimesyncdService: InitializeDBus(conn)
TimesyncdService->>DBus: Export timesyncd interface
sequenceDiagram
participant Client
participant SwapService
participant ConfigFile
Client->>SwapService: Invoke get/set swap method
alt Get Request
SwapService->>ConfigFile: Read swap size/swappiness
ConfigFile-->>SwapService: Return current value
SwapService-->>Client: Return value
else Set Request
SwapService->>ConfigFile: Validate and update value
alt Validation Succeeds
ConfigFile-->>SwapService: Confirm update
SwapService-->>Client: Return success
else Validation Fails
SwapService-->>Client: Return D-Bus error
end
end
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lint (1.62.2)Error: can't load config: the Go language version (go1.23) used to build golangci-lint is lower than the targeted Go version (1.24) ✨ 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: 0
🧹 Nitpick comments (4)
main.go (1)
75-76: Consider adding error handling or logging here.
Calls toswap.InitializeDBus(conn)andtimesyncd.InitializeDBus(conn)do not check for errors. If these initialization calls fail, the application will proceed without clear feedback. It’s often best practice to log or handle potential errors here.config/swap/swap.go (3)
29-32: Be mindful of concurrency with shared fields.
Theswapstruct references global variablesoptSwapSizeandoptSwappiness. If multiple DBus calls or threads were to change these properties in quick succession, it might create race conditions. Consider using localized fields or synchronization primitives to ensure thread safety.
64-83: Ensure comprehensive swap size validation.
While the current regex covers typical patterns, users might provide unorthodox formats (e.g., “1024kB”). Ensure the regex and subsequent checks explicitly document which formats are supported to reduce confusion.
105-160: Consider system capability checks.
This DBus node attempts to configure swap size and swappiness. If the environment doesn’t support changing swap size (e.g., some Supervised installs), you might log a message or return an error if you detect unsupported behavior. This aligns with the PR note that such checks could be handled elsewhere, but a warning log might prevent confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
config/swap/swap.go(1 hunks)config/timesyncd/timesyncd.go(1 hunks)main.go(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- config/timesyncd/timesyncd.go
🔇 Additional comments (4)
main.go (1)
15-16: Nice modular reorganization.
By directly importing theswapandtimesyncdpackages, you've made the code more cohesive and easier to maintain. This matches the PR objective of splitting concerns into distinct modules.config/swap/swap.go (3)
34-46: Validate missing configuration more explicitly.
WhenswapFileEditor.Find(...)fails to locate a line or read from the file, the function silently returns an empty string. Adding a more explicit log or handling (e.g., debug log) can help in diagnosing misconfigurations or missing swap settings.
48-62: Gracefully notify on default swappiness usage.
Falling back to the default swappiness value is reasonable, but consider logging a warning or debug message. It provides better visibility into how the system is configured if the file is missing or has invalid data.
85-103: Check that int32 casting aligns with potential usage.
Swappiness is being cast toint32. Though valid for typical usage (0–100), be sure there are no edge cases where a larger integer might be passed over DBus.
config/swap/swap.go
Outdated
| ifaceName = "io.hass.os.Config.Swap" | ||
| swapPath = "/etc/default/haos-swapfile" | ||
| swappinessPath = "/etc/sysctl.d/15-swappiness.conf" | ||
| defaultSwappiness = 1 |
There was a problem hiding this comment.
That seems to be the default in the config file, but not the default default of the kernel 🤔
Should we maybe just read the current value using sysctl instead in case the file doesn't exist? 🤔
There was a problem hiding this comment.
I was aware of this. The kernel default would never apply as HAOS sets it in read-only part of the FS but I haven't taken Supervised in account. I would have preferred to return it empty too as the "default" but as it's int, it's not nullable. The other drawback is that once you get this default, first call to set the value will persist it in the /etc/sysctl.d override.
If you don't have better idea, we could also read /proc/sys/vm/swappiness (as it has less overhead than calling systctl).
There was a problem hiding this comment.
If you don't have better idea, we could also read
/proc/sys/vm/swappiness(as it has less overhead than callingsystctl).
Yeah I think that is quite a good and portable solution. If "our" config file isn't there, obviously that is the how the system defaults to through whatever other means it has been set. 👍
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
sairon
force-pushed
the
add-config-swap
branch
from
1708e8b to
60ebe6d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
config/swap/swap.go (5)
35-51: Consider more robust error handling for kernel swappiness.The function correctly reads swappiness from kernel procfs, but logs errors and returns a default of 60 when either reading or parsing fails. This approach was discussed and approved in previous reviews, but consider adding more context to error messages to help with debugging.
- logging.Error.Printf("Failed to read kernel swappiness: %s", err) + logging.Error.Printf("Failed to read kernel swappiness from /proc/sys/vm/swappiness: %s", err)
53-65: Silent error handling in getSwapSize could make debugging difficult.The function silently returns an empty string on errors, which could make debugging difficult in production environments. Consider at least logging the error when it occurs.
found, err := swapFileEditor.Find(`^SWAPSIZE=`, "", true) if found == nil || err != nil { + if err != nil { + logging.Debug.Printf("Failed to read swap size from %s: %s", swapPath, err) + } return "" }
83-93: SwapSize validation regex could be more specific.The current regex allows for any combination of units, including invalid ones like "KB" or "GB". Consider using a more specific regex that matches only valid size specifications like "512M" or "1G".
- re := regexp.MustCompile(`^\d+([KMG]?(i?B)?)?$`) + re := regexp.MustCompile(`^\d+([KMG]|[KMG]iB|[KMG]B)?$`)
94-102: Consider adding logging for successful swap size changes.For audit purposes, it would be helpful to log when the swap size is successfully changed.
if err := swapFileEditor.Present(params); err != nil { return dbus.MakeFailedError(fmt.Errorf("failed to set swap size: %w", err)) } + logging.Info.Printf("Swap size set to %s", swapSize) return nil
104-122: Consider adding logging for successful swappiness changes.Similar to the swap size function, it would be helpful to log when the swappiness value is successfully changed.
if err := swappinessEditor.Present(params); err != nil { return dbus.MakeFailedError(fmt.Errorf("failed to set swappiness: %w", err)) } + logging.Info.Printf("Swappiness set to %d", swappiness) return nil
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
config/swap/swap.go(1 hunks)config/timesyncd/timesyncd.go(1 hunks)main.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- config/timesyncd/timesyncd.go
🔇 Additional comments (7)
main.go (2)
15-16: Import changes look good.The imports for the new
swappackage and renamedtimesyncdpackage (formerly part ofconfig) are correctly added.
75-76: Initialization of D-Bus connections looks good.The
swap.InitializeDBus(conn)andtimesyncd.InitializeDBus(conn)calls properly initialize the new D-Bus interfaces, replacing the formerconfig.InitializeDBus(conn)approach with more modular service initialization.config/swap/swap.go (5)
3-14: Imports look good and are well organized.The imports include all necessary packages for D-Bus communication, file operations, and regex pattern matching.
16-21: Constants are well-defined.The constants clearly define the D-Bus object path, interface name, and file paths for configuration.
67-81: getSwappiness implementation looks good.The function correctly tries to read swappiness from the configuration file and falls back to kernel swappiness if not found or on error. The approach aligns with the previous review discussions.
124-179: InitializeDBus implementation is well-structured.The function properly sets up the D-Bus interface, exports properties and methods, and sets up introspection. The implementation follows the pattern used elsewhere in the codebase.
1-180:❓ Verification inconclusive
Consider adding system capability checks.
As mentioned in the PR description, the implementation doesn't check whether swap size and swappiness configurations are supported on the target system. While the author suggests these checks should be handled at a higher level, it might be worth considering adding some basic capability detection to provide better error messages when unsupported operations are attempted.
This could be especially relevant for supervised installations where swap size cannot be controlled.
🌐 Web query:
How to detect if a Linux system supports swap file configuration?💡 Result:
To determine if a Linux system supports swap file configuration, follow these steps:
1. Check Filesystem Compatibility
Swap file support depends on the underlying filesystem type:
- Use
df -T /target_directoryto identify the filesystem where you plan to create the swap file.- Supported: ext4, XFS, Btrfs (with Copy-on-Write disabled), F2FS, JFS [4].
- Unsupported: ZFS, Bcachefs [4].
2. Verify Kernel Support
Most modern Linux kernels (≥ 4.x) natively support swap files on compatible filesystems. Confirm with:uname -r3. Test Swap File Creation
Attempt these commands in a safe test environment:sudo fallocate -l 1G /test_swapfile # Create a test file sudo chmod 600 /test_swapfile # Set permissions sudo mkswap /test_swapfile # Format as swapIf no errors occur, your system supports swap file creation [2][5][8].
4. Check for Special Cases
- Btrfs: Requires disabling Copy-on-Write (CoW) for the swap file:
chattr +C /swapfile # Disable CoW before allocating space [5].- Encrypted Swap: Requires additional setup like LUKS or dm-crypt [4].
5. Validate Active Swap
After enabling a test swap file:sudo swapon /test_swapfile # Activate cat /proc/swaps # Verify activation [2][4] free -h # Check total swap space [2][8]If all steps succeed without errors or warnings related to unsupported operations, your system fully supports swap file configuration. For legacy systems or niche filesystems (e.g., ZFS), use a dedicated swap partition instead [4][8].
Citations:
- 1: https://phoenixnap.com/kb/swap-space
- 2: https://phoenixnap.com/kb/linux-swap-file
- 3: https://www.tecmint.com/commands-to-monitor-swap-space-usage-in-linux/
- 4: https://wiki.archlinux.org/title/Swap
- 5: https://www.digitalocean.com/community/tutorials/how-to-add-swap-space-on-ubuntu-22-04
- 6: https://discourse.nixos.org/t/how-can-i-see-if-my-system-is-using-my-swap-partition/47803
- 7: https://docs.redhat.com/en/documentation/red_hat_enterprise_linux/9/html/managing_storage_devices/getting-started-with-swap_managing-storage-devices
- 8: https://linuxize.com/post/create-a-linux-swap-file/
- 9: https://www.linux.org/threads/is-there-any-program-to-check-swap-filesystem.41280/
- 10: https://help.ubuntu.com/community/SwapFaq
Action Required: Enhance swap configuration with basic system capability detection
The current implementation doesn’t verify if the target system supports modifying swap settings. While the design assumes such checks are handled at a higher level, adding lightweight detection here could improve error messaging—especially for supervised installations where swap size control might be unsupported. For example, consider:
- Filesystem verification: Check the filesystem type (using commands like
df -T) to ensure it’s one of the supported types (e.g., ext4, XFS, Btrfs with CoW disabled) since filesystems like ZFS typically don't support swap files.- Kernel version check: Ensure the running kernel (e.g., via
uname -r) meets the minimum version (Linux ≥ 4.x) for swap file support.- Test swap file creation: Optionally, simulate swap file creation (using
mkswapandswapon) in a controlled environment to proactively catch unsupported configurations.Implementing these checks could help provide clearer feedback when swap operations are attempted on unsupported systems.
Adds new DBus APIs for NTP servers and swap: - home-assistant/os-agent#207 - home-assistant/os-agent#222
Adds new DBus APIs for NTP servers and swap: - home-assistant/os-agent#207 - home-assistant/os-agent#222
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
Summary by CodeRabbit