Initialize Supervisor Core state in constructor #5686
Conversation
agners
force-pushed
the
initialize-state-early
branch
from
b501c79 to
af25003
Compare
|
Warning Rate limit exceeded@agners has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 52 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe pull request refactors the state management in the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Core
participant StateFile
Client ->> Core: Initialize Core
Core ->> StateFile: _write_run_state(INITIALIZE)
StateFile -->> Core: Confirmation
Core -->> Client: Initialization Complete
sequenceDiagram
participant Client
participant Core
participant StateFile
Client ->> Core: set state(new_state)
Core ->> StateFile: _write_run_state(new_state)
StateFile -->> Core: Confirmation
Core -->> Client: State Updated
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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
supervisor/core.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
`*/**(html|markdown|md)`: - For instructional content in doc...
*/**(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'
supervisor/core.py
`*/**(html|markdown|md)`: - Use bold to mark UI strings. - I...
*/**(html|markdown|md): - Use bold to mark UI strings.
- If "" are used to mark UI strings, replace them by bold.
supervisor/core.py
`*/**(html|markdown|md)`: - Be brief in your replies and don...
*/**(html|markdown|md): - Be brief in your replies and don't add fluff like "thank you for..." and "Please let me know if"
supervisor/core.py
`*/**(html|markdown|md)`: - Use sentence-style capitalizatio...
*/**(html|markdown|md): - Use sentence-style capitalization also in headings.
supervisor/core.py
`*/**(html|markdown|md)`: do not comment on HTML used for ic...
*/**(html|markdown|md): do not comment on HTML used for icons
supervisor/core.py
`*/**(html|markdown|md)`: Avoid flagging inline HTML for emb...
*/**(html|markdown|md): Avoid flagging inline HTML for embedding videos in future reviews for this repository.
supervisor/core.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
🔇 Additional comments (2)
supervisor/core.py (2)
41-42: The state initialization looks good.Initializing the state to a valid
CoreStatevalue (CoreState.INITIALIZE) instead ofNoneand writing it to the state file during initialization addresses the PR objective. This ensures that the state is always a valid enumeration value, avoiding potential errors with filters like Sentry during early startup.
69-86: State management improvement looks good.The refactored state setter with early return for unchanged states and explicit state writing before updates is a good improvement. This ensures that external systems always see a consistent state and prevents unnecessary file operations.
agners
force-pushed
the
initialize-state-early
branch
3 times, most recently
from
a508b15 to
1265d1b
Compare
Make sure the Supervisor Core state is set to a value early on. This makes sure that the state is always of type CoreState, and makes sure that any use of the state can rely on it being an actual value from the CoreState enum. This fixes Sentry filter during early startup, where the state previously was None. Because of that, the Sentry filter tried to collect more Context, which lead to an exception and not reporting errors.
It seems that with initializing the state early, the pytest actually runs a system evaluation with: Starting system evaluation with state initialize Before it did that with: Starting system evaluation with state None It detects that the container runs as privileged, and declares the system as unhealthy. It is unclear to me why coresys.core.healthy was checked in this context, it doesn't seem useful. Just remove the check, and validate the state through the getter instead.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
With the Supervisor Core state being valid now, some evaluations now actually run when loading the resolution center. This leads to Supervisor getting declared unhealthy due to not running in a privileged container under pytest. Fake the host container to be privileged to make evaluations not causing the system to be declared unhealthy under pytest.
agners
force-pushed
the
initialize-state-early
branch
from
cd278e4 to
bbfa436
Compare
With the Supervisor Core state being valid from the very start, we end up writing a state everytime. Instead of actually writing a state file, simply validate the the necessary calls are being made. This is more conform to typical unit tests and avoids writing a file for every test.
Extend the ha_ws_client WebSocket client fixture to set Supervisor Core into run state and clear all pending messages. Currently only some tests use the ha_ws_client WebSocket client fixture. Use it consistently for all tests.
agners
force-pushed
the
initialize-state-early
branch
from
bbfa436 to
0389e71
Compare
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
Make sure the Supervisor Core state is set to a value early on. This makes sure that the state is always of type CoreState, and makes sure that any use of the state can rely on it being an actual value from the CoreState enum.
This fixes Sentry filter during early startup, where the state previously was None. Because of that, the Sentry filter tried to collect more Context, which lead to an exception and not reporting errors.
Type of change
Additional information
Checklist
ruff format supervisor tests)If API endpoints or add-on configuration are added/changed:
Summary by CodeRabbit
Refactor
Tests
RUNNINGduring write failures.