Defer annotation update until after watchdog is started - #334
Conversation
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: kwohlfahrt The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @kwohlfahrt. Thanks for your PR. I'm waiting for a medik8s member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe watchdog now signals startup completion through ChangesWatchdog startup and annotation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Agent
participant synchronizedWatchdog
participant NodeAnnotation
Agent->>synchronizedWatchdog: wait for Started()
synchronizedWatchdog-->>Agent: startup channel closes
Agent->>NodeAnnotation: update armed state and timeout
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/main.go`:
- Around line 358-364: Update the runnable setup around wd.Started() to call
UpdateNodeAnnotations with the unavailable state (false and timeout 0) before
registering the runnable with mgr. Preserve the existing context cancellation
behavior, then call UpdateNodeAnnotations again after wd.Started() closes using
wd.Status() == watchdog.Armed and wd.GetTimeout() so the reachable state is
published.
In `@internal/watchdog/watchdog_test.go`:
- Around line 90-94: Move the context.WithCancel(context.Background()) setup out
of the BeforeEach callback into a package-level test helper, then invoke that
helper from BeforeEach while preserving the existing ctx, cancel, and
DeferCleanup behavior. Update the test setup around BeforeEach without changing
the test lifecycle semantics.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3bab509f-bd01-4260-a4f9-aeb4c5618c05
📒 Files selected for processing (4)
cmd/main.gointernal/watchdog/interface.gointernal/watchdog/synchronized.gointernal/watchdog/watchdog_test.go
Report the correct error if software reboot is disabled. This tears down the manager and agent if the watchdog cannot be started.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Why we need this PR
Currently, the watchdog timeout is read upon creating the watchdog, before starting it. This always returns a value of 0, because the timeout is only available after the watchdog's start.
Changes made
Started()method to the watchdog interface, that notifies when the watchdog is startedWhich issue(s) this PR fixes
Fixes #332
Test plan
Added new unit test. I'm open to suggestions for a more thorough e2e test, but am not familiar enough with the test setup to come up with a clean idea.