Add API for swap configuration #5770
Conversation
📝 WalkthroughWalkthroughThe changes add swap management functionality to the system. New API routes are introduced under Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant R as RestAPI
participant A as APIOS
participant O as OSAgent
participant S as Swap (DBus)
C->>R: GET /os/config/swap
R->>A: config_swap_info(request)
A->>O: Retrieve swap settings
O->>S: Query swap_size & swappiness
S-->>O: Return current swap values
O-->>A: Provide swap info
A-->>R: Return response data
R-->>C: 200 OK (swap info)
sequenceDiagram
participant C as Client
participant R as RestAPI
participant A as APIOS
participant O as OSAgent
participant S as Swap (DBus)
C->>R: POST /os/config/swap (payload)
R->>A: config_swap_options(request)
A->>O: Validate and apply new swap settings
O->>S: Update swap_size & swappiness
S-->>O: Acknowledge update
O-->>A: Check for reboot requirement
A-->>R: Return success/issue response
R-->>C: 200 OK
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (11)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (6)
🧰 Additional context used🧬 Code Definitions (2)supervisor/api/__init__.py (1)
supervisor/dbus/agent/__init__.py (2)
⏰ Context from checks skipped due to timeout of 90000ms (4)
🔇 Additional comments (18)
✨ Finishing Touches
🪧 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: 3
🔭 Outside diff range comments (1)
supervisor/dbus/agent/__init__.py (1)
97-99:⚠️ Potential issueMissing swap in the 'all' property.
The
allproperty returns a list of all managed DBUS interfaces, but it doesn't include the newly addedself._swap. This could cause issues with methods that rely on this list being complete, such as theconnectandshutdownmethods.@property def all(self) -> list[DBusInterface]: """Return all managed dbus interfaces.""" - return [self.apparmor, self.board, self.cgroup, self.datadisk, self.system] + return [self.apparmor, self.board, self.cgroup, self.datadisk, self.system, self.swap]
🧹 Nitpick comments (1)
supervisor/api/os.py (1)
243-256: Consider adding validation for swap parametersWhile the schema ensures the proper types, there's no validation for the range of swappiness (typically 0-100) or the format of swap size (e.g., "1M", "2G").
Consider adding additional validation to ensure the provided values are within acceptable ranges:
if ATTR_SWAPPINESS in body: swappiness = body[ATTR_SWAPPINESS] if not 0 <= swappiness <= 100: raise web.HTTPBadRequest(text="Swappiness must be between 0 and 100") if ATTR_SWAP_SIZE in body: # Add validation for swap size format if needed # For example, ensure it follows the pattern like "1M", "2G", etc.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
supervisor/api/__init__.py(1 hunks)supervisor/api/os.py(4 hunks)supervisor/const.py(1 hunks)supervisor/dbus/agent/__init__.py(3 hunks)supervisor/dbus/agent/swap.py(1 hunks)supervisor/dbus/const.py(3 hunks)tests/api/test_os.py(2 hunks)tests/conftest.py(1 hunks)tests/dbus/agent/test_agent.py(1 hunks)tests/dbus/agent/test_swap.py(1 hunks)tests/dbus_service_mocks/agent_swap.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
`*/**(html|markdown|md)`: - For instructional content in documentation, use a direct and authoritative tone. Avoid expressions of politeness such as 'may' or 'please', and ensure t...
*/**(html|markdown|md): - For instructional content in documentation, use a direct and authoritative tone. Avoid expressions of politeness such as 'may' or 'please', and ensure the goal of the instruction is fronted.
- Apply the Microsoft Style Guide to ensure documentation maintains clarity and conciseness.
- In step-by-step instructions, front the location phrase in the instructional sentence.
- In step-by-step instructions, front the 'goal' in the instructional sentence.
- In step-by-step instructions, if in doubt what to front, front the 'goal' before the location phrase in the instructional sentence.
- do not hyphenate terms like 'top-right' or 'bottom-left' with 'corner'
tests/conftest.pysupervisor/const.py
`*/**(html|markdown|md)`: - Use bold to mark UI strings. - If "" are used to mark UI strings, replace them by bold.
*/**(html|markdown|md): - Use bold to mark UI strings.
- If "" are used to mark UI strings, replace them by bold.
tests/conftest.pysupervisor/const.py
`*/**(html|markdown|md)`: - Be brief in your replies and don't add fluff like "thank you for..." and "Please let me know if"
*/**(html|markdown|md): - Be brief in your replies and don't add fluff like "thank you for..." and "Please let me know if"
tests/conftest.pysupervisor/const.py
`*/**(html|markdown|md)`: - Use sentence-style capitalization also in headings.
*/**(html|markdown|md): - Use sentence-style capitalization also in headings.
tests/conftest.pysupervisor/const.py
`*/**(html|markdown|md)`: do not comment on HTML used for icons
*/**(html|markdown|md): do not comment on HTML used for icons
tests/conftest.pysupervisor/const.py
`*/**(html|markdown|md)`: Avoid flagging inline HTML for embedding videos in future reviews for this repository.
*/**(html|markdown|md): Avoid flagging inline HTML for embedding videos in future reviews for this repository.
tests/conftest.pysupervisor/const.py
🧬 Code Definitions (5)
supervisor/api/__init__.py (1)
supervisor/api/os.py (2) (2)
config_swap_info(227-235)config_swap_options(238-262)
supervisor/dbus/agent/__init__.py (2)
supervisor/dbus/agent/swap.py (1) (1)
Swap(15-40)tests/dbus_service_mocks/agent_swap.py (1) (1)
Swap(15-42)
supervisor/dbus/agent/swap.py (1)
tests/dbus_service_mocks/agent_swap.py (1) (1)
Swap(15-42)
tests/dbus_service_mocks/agent_swap.py (1)
supervisor/dbus/agent/swap.py (1) (1)
Swap(15-40)
tests/dbus/agent/test_swap.py (4)
supervisor/dbus/agent/__init__.py (3) (3)
OSAgent(31-148)swap(61-63)connect(101-124)supervisor/dbus/agent/swap.py (3) (3)
Swap(15-40)swap_size(24-26)swappiness(34-36)tests/dbus_service_mocks/agent_swap.py (1) (1)
Swap(15-42)tests/conftest.py (1) (1)
dbus_session_bus(148-152)
🔇 Additional comments (30)
supervisor/const.py (1)
316-317: LGTM! New attribute constants for swap configurationThe new constants
ATTR_SWAP_SIZEandATTR_SWAPPINESSfollow the consistent naming pattern used throughout this file. These will support the new swap configuration API.supervisor/dbus/const.py (3)
28-28: LGTM! New DBus interface constant for swap configurationThis constant correctly defines the D-Bus interface identifier following the established pattern used for other interfaces.
57-57: LGTM! New DBus object path for swap configurationThis constant correctly defines the D-Bus object path following the same pattern as other object paths in the file.
174-175: LGTM! New DBus attribute constants for swap propertiesThese constants correctly define the D-Bus attribute names for swap size and swappiness, maintaining the camelCase convention used for D-Bus attributes.
tests/conftest.py (1)
282-282: LGTM! Added mock for agent_swap serviceProperly added the mock for the new agent_swap service in the fixture_os_agent_services function, which is necessary for testing the new swap configuration functionality.
tests/dbus/agent/test_agent.py (1)
72-72: LGTM! Added agent_swap mock for connection error testingCorrectly added the agent_swap service to the os_agent_services dictionary in the test_dbus_osagent_connect_error function, ensuring consistent test coverage for the new swap configuration service.
supervisor/api/__init__.py (1)
261-267: New routes added for swap configuration management.These routes implement the HTTP API for managing swap size and swappiness settings via the
/os/config/swapendpoint, which aligns with the PR objectives. The implementation uses the appropriate HTTP methods (GET for retrieving settings, POST for updating them).supervisor/dbus/agent/__init__.py (3)
25-25: The import of the new Swap class is properly added.This import statement correctly brings in the newly implemented
Swapclass from the swap module.
47-47: The Swap instance is initialized properly.Following the established pattern in this class, a
Swapinstance is created during initialization.
60-64: The property accessor for Swap is correctly implemented.The property accessor follows the same pattern as other DBUS objects in this class, providing a clean interface for accessing the swap functionality.
supervisor/dbus/agent/swap.py (3)
1-21: The Swap class implementation follows good practices.The class is well-structured, with clear docstrings and proper organization of constants and methods. It inherits from
DBusInterfaceProxyand properly configures the necessary DBus connection details.
28-31: The set_swap_size method is properly implemented.This method provides the necessary functionality to set the swap size via DBus, returning an Awaitable for asynchronous operation.
38-40: The set_swappiness method is properly implemented.This method provides the necessary functionality to set the swappiness value via DBus, returning an Awaitable for asynchronous operation.
tests/dbus_service_mocks/agent_swap.py (4)
1-13: The setup function for the swap mock is implemented correctly.This function follows the pattern of other mock setup functions, creating and returning an instance of the Swap mock class.
15-23: The Swap mock class is properly configured.The class properly inherits from
DBusServiceMockand sets up the correct object path and interface for mocking the swap configuration DBus service.
24-33: The SwapSize property is properly implemented.The getter returns a default value of "1M" and the setter correctly emits properties changed signals, which is essential for testing property change notifications.
34-42: The Swappiness property is properly implemented.The getter returns a default value of 1 and the setter correctly emits properties changed signals when the value is updated.
tests/dbus/agent/test_swap.py (4)
1-11: LGTM: Clear imports and docstringThe module docstring and import organization follow the project's standard structure.
12-17: LGTM: Well-structured fixtureThe
fixture_swap_serviceis properly set up to provide the mock swap service from theos_agent_servicesdictionary, following the project's standard testing pattern.
20-33: LGTM: Thorough swap size test implementationThe test correctly verifies both initial state and property changes for the swap size. The pattern of connecting to the bus, checking initial values, emitting property changes, and verifying the updated values is well-implemented.
36-50: LGTM: Consistent swappiness test implementationThe swappiness test follows the same pattern as the swap size test, ensuring consistent testing methodology across related properties.
supervisor/api/os.py (3)
25-26: LGTM: Added new attributes for swap configurationThe new attributes
ATTR_SWAP_SIZEandATTR_SWAPPINESSare properly imported from the constants module.
72-77: LGTM: Well-defined schema for swap optionsThe schema properly validates and coerces the swap configuration parameters to their expected types (string for swap size and integer for swappiness).
226-235: LGTM: Implemented swap configuration info endpointThe implementation correctly checks for OS availability and version compatibility before proceeding. The response includes both swap_size and swappiness values from the DBus agent.
tests/api/test_os.py (6)
21-21: LGTM: Added import for SwapServiceThe import for the mock SwapService follows the project's import pattern for test mocks.
343-356: LGTM: Well-implemented swap info testThe test properly checks that the API returns the expected swap configuration values, using the parametrized fixture to ensure the test runs with OS version 15.0.
358-422: LGTM: Comprehensive swap options testThis test thoroughly verifies the API's capability to set both swap parameters together and individually. It also checks that reboot issues are correctly created when settings are changed.
424-465: LGTM: Edge case testing for no reboot scenariosGood coverage of edge cases where no reboot should be required, such as empty settings or unchanged values. This ensures the reboot flag logic works correctly.
468-486: LGTM: Error case testing for non-OS environmentProper verification that a 404 response is returned when the system is not running on Home Assistant OS.
489-509: LGTM: Version compatibility testGood test coverage ensuring that the API correctly returns 404 for systems running OS versions older than 15.0, as specified in the requirements.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
supervisor/api/os.py (1)
229-230: Consider extracting the version check into a helper method.Since the version check appears in both methods with identical logic, consider extracting it to a private helper method to avoid code duplication.
@@ -226,6 +226,11 @@ @api_process + def _check_swap_support(self) -> None: + """Check if swap management is supported.""" + if not self.coresys.os.available or self.coresys.os.version < "15.0": + raise HTTPNotFound() + + @api_process async def config_swap_info(self, request: web.Request) -> dict[str, Any]: """Get swap settings.""" - if not self.coresys.os.available or self.coresys.os.version < "15.0": - raise HTTPNotFound() + self._check_swap_support() @@ -237,8 +242,7 @@ async def config_swap_options(self, request: web.Request) -> None: """Update swap settings.""" - if not self.coresys.os.available or self.coresys.os.version < "15.0": - raise HTTPNotFound() + self._check_swap_support()Also applies to: 240-241
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
supervisor/api/os.py(4 hunks)supervisor/dbus/agent/swap.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- supervisor/dbus/agent/swap.py
🧰 Additional context used
🧬 Code Definitions (1)
supervisor/api/os.py (2)
supervisor/dbus/agent/__init__.py (2) (2)
version(82-84)swap(61-63)supervisor/dbus/agent/swap.py (4) (4)
swap_size(24-26)swappiness(33-35)set_swap_size(27-29)set_swappiness(36-38)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build armv7 supervisor
- GitHub Check: Build armhf supervisor
- GitHub Check: Build aarch64 supervisor
- GitHub Check: Run tests Python 3.13.2
🔇 Additional comments (6)
supervisor/api/os.py (6)
9-9: HTTPNotFound import is appropriate for the new endpoints.The import is correctly added to handle cases where OS is not available or doesn't meet the minimum version requirements in the new swap configuration endpoints.
25-26: Attribute constants are properly imported.These constants are correctly added to the imports to support the new swap configuration functionality.
72-77: Schema definition is well-implemented.The SCHEMA_SWAP_OPTIONS is correctly defined with proper type coercion for both parameters, making it consistent with other validation schemas in the file.
226-235: Good implementation of the swap info endpoint.The method correctly:
- Checks for OS availability and version compatibility
- Returns appropriate HTTP status when requirements aren't met
- Returns the current swap configuration values from the DBus agent
237-262: Swap options update logic is well-implemented with proper reboot handling.The implementation correctly:
- Validates OS availability and version requirements
- Validates input using the schema
- Updates swap settings as needed
- Correctly determines if a reboot is required using accumulative logic
- Creates a system issue when reboot is required
The reboot logic has been properly fixed as suggested in previous review comments.
229-230:✅ Verification successful
Verify OS version check logic is consistent with other endpoints.
Both methods use the same check before accessing OS agent functionality. Let's verify this is consistent with how version requirements are handled in other endpoints.
Also applies to: 240-241
🏁 Script executed:
#!/bin/bash # Check how version requirements are handled in other API methods in the codebase rg -B3 -A1 "os\.version\s*<" --type pyLength of output: 727
OS Version Check Logic Consistent Across Endpoints
The verification confirms that both endpoints insupervisor/api/os.py(lines 229-230 and 240-241) are using the same check (if not self.coresys.os.available or self.coresys.os.version < "15.0":) before accessing OS agent functionality. No further changes are needed.
supervisor/api/__init__.py
Outdated
| web.get("/os/config/swap", api_os.config_swap_info), | ||
| web.post("/os/config/swap", api_os.config_swap_options), |
There was a problem hiding this comment.
Obviously more of a nitpick but we have /os/config/sync up above, think we should just add these two to the list up here:
supervisor/supervisor/api/__init__.py
Line 242 in c2455f8
supervisor/api/os.py
Outdated
| async def config_swap_info(self, request: web.Request) -> dict[str, Any]: | ||
| """Get swap settings.""" | ||
| if not self.coresys.os.available or self.coresys.os.version < "15.0": | ||
| raise HTTPNotFound() |
There was a problem hiding this comment.
Looking at the status codes, this is the only one that makes sense. That being said, we can provide a better explanation then this so I think we should. Something like telling them this API requires the system be HAOS with version >= 15.0 . And perhaps log this as well.
There was a problem hiding this comment.
Yeah, good point. I also kind of forgot about our APINotFound here 🙈
supervisor/api/os.py
Outdated
| vol.Optional(ATTR_SWAP_SIZE): vol.Coerce(str), | ||
| vol.Optional(ATTR_SWAPPINESS): vol.Coerce(int), |
There was a problem hiding this comment.
This seems very open-ended, is it actually this open-ended? Can we provide any further validation on the string? Maybe vol.Match([1-9][0-9]M)? I'm guessing based on the test cases. Although actually if M is the only allowed unit we should probably just require a positive integer instead and add the M for them.
Similar question on swappiness. Is there a valid range of values? If its just a positive number then we can do vol.All(int, vol.Range(min=0)) or min=1 if 0 is invalid.
Additionally we don't want coerce for str and int. Those data types can be expressed in JSON so we should require that the JSON has a str or an int, not something that can be coerced into a str or int
There was a problem hiding this comment.
Thanks, I'll improve the validations. Swappiness can be only 0-200, that's what Linux accepts. For swap size it's actually quite permissive: https://github.com/home-assistant/operating-system/blob/15.0/buildroot-external/rootfs-overlay/usr/libexec/haos-swapfile#L4-L14
I'll add a regexp that matches what's defined there.
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
mdegat01
added
missing-documentation
|
@sairon btw this outside diff range comment by coderabbit here is right:
supervisor/supervisor/dbus/agent/__init__.py Lines 112 to 114 in c2455f8 |
Add HTTP API for swap size and swappiness to /os/config/swap. Individual options can be set in JSON and are calling the DBus API added in OS Agent 1.7.x, available since OS 15.0. Check for presence of OS of the required version and return 404 if the criteria are not met.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
- Improve swap options validation - Add swap to the 'all' property of dbus agent - Use APINotFound with reason instead of HTTPNotFound - Reorder API routes
|
Thanks for the comments, I've addressed them and rebased the PR. |
Proposed change
Add HTTP API for swap size and swappiness to /os/config/swap. Individual options can be set in JSON and are calling the DBus API added in OS Agent 1.7.x, available since OS 15.0. Check for presence of OS of the required version and return 404 if the criteria are not met.
Type of change
Additional information
Checklist
ruff format supervisor tests)If API endpoints or add-on configuration are added/changed:
Summary by CodeRabbit
New Features
Tests