Load resolution evaluation, check and fixups early #5696
Conversation
Before #5652, these modules were loaded in the constructor, hence early in `initialize_coresys()`. Moving them late actually exposed an issue where NetworkManager connectivity setter couldn't get the `connectivity_check` evaluation, leading to an exception early in bootstrap. Technically, it might be safe to load the resolution modules only in `Core.connect()`, however then we'd have to load them separately for pytest. Let's go conservative and load them the same place where they got loaded before #5652.
📝 WalkthroughWalkthroughThe pull request refactors the module loading process within the resolution component. Across several files, method names have been updated from Changes
Sequence Diagram(s)sequenceDiagram
participant Bootstrap
participant ResolutionManager
participant CheckLoader
participant EvaluateLoader
participant FixupLoader
Bootstrap->>ResolutionManager: initialize_coresys()
ResolutionManager->>CheckLoader: load_modules()
ResolutionManager->>EvaluateLoader: load_modules()
ResolutionManager->>FixupLoader: load_modules()
ResolutionManager-->>Bootstrap: Initialization complete (post health check)
✨ 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 (3)
supervisor/resolution/evaluate.py (1)
36-47: Consider enforcing executor usage and addressing naming consistency.
The docstring states "Must be run in executor," but there is no runtime check or guard preventing invocation outside of an executor context, which can lead to unintended use if called elsewhere. Also, the internal property is calledself._evalutionsinstead ofself._evaluations, which could cause confusion.Below is a sample diff showing how you might rename the property; you may also add a simple condition or raise an exception if
load_modules()is invoked more than once or at an inappropriate time:- self._evalutions: dict[str, EvaluateBase] = {} + self._evaluations: dict[str, EvaluateBase] = {}supervisor/resolution/fixup.py (1)
25-36: Provide an explicit guard for executor-bound loading.
Similar to the evaluation loader, this method is documented to "run in executor" without actively enforcing it. If there's a concern about multiple invocations or loading race conditions, consider explicitly preventing subsequent calls or logging a warning upon repeated usage.supervisor/resolution/module.py (1)
49-58: Potentially inline or rename the nested function and handle repeated loads.
Defining_load_modulesinsideload_moduleshelps separate sync and async responsibilities, but consider renaming it to_sync_load_modules(or inlining it) for clarity. Additionally, if callingload_modules()repeatedly is unsupported, add a simple check or guard to avoid re-running module loading unintentionally.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
supervisor/resolution/check.py(1 hunks)supervisor/resolution/evaluate.py(1 hunks)supervisor/resolution/fixup.py(1 hunks)supervisor/resolution/module.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- supervisor/resolution/check.py
⏰ 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
Since #5696 we don't need to load the resolution center early. In fact, with #5686 this is even problematic for pytests in devcontainer, since the Supervisor Core state is valid and this causes AppArmor evaluations to run (and fail). Actually, #5696 removed the resolution center. #5686 brought it accidentally back. This was seemingly a merge error.
Since #5696 we don't need to load the resolution center early. In fact, with #5686 this is even problematic for pytests in devcontainer, since the Supervisor Core state is valid and this causes AppArmor evaluations to run (and fail). Actually, #5696 removed the resolution center. #5686 brought it accidentally back. This was seemingly a merge error.
Proposed change
Before #5652, these modules were loaded in the constructor, hence early in
initialize_coresys(). Moving them late actually exposed an issue where NetworkManager connectivity setter couldn't get theconnectivity_checkevaluation, leading to an exception early in bootstrap.Technically, it might be safe to load the resolution modules only in
Core.connect(), however then we'd have to load them separately for pytest. Let's go conservative and load them the same place where they got loaded before #5652.Type of change
Additional information
Checklist
ruff format supervisor tests)If API endpoints or add-on configuration are added/changed:
Summary by CodeRabbit