Failover controller to support multicluster watches for reservations#625
Merged
Failover controller to support multicluster watches for reservations#625
Conversation
SoWieMarkus
requested review from
PhilippMatthes,
auhlig,
juliusclausnitzer,
mblos and
umswmayj
as code owners
umswmayj
approved these changes
Mar 24, 2026
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
PhilippMatthes
approved these changes
Mar 24, 2026
Contributor
Test Coverage ReportTest Coverage 📊: 68.5% |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/scheduling/reservations/failover/controller.go (1)
777-779: Add context to setup error return.Line 778 currently returns the raw error. Wrapping it with setup context will make controller startup failures easier to triage in logs.
Proposed change
if err != nil { - return err + return fmt.Errorf("failed to register multicluster reservation watch: %w", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/scheduling/reservations/failover/controller.go` around lines 777 - 779, The current error return in the controller startup path just returns err; update the error handling around the if err != nil { return err } in the controller setup/startup function to wrap the error with contextual text (e.g., "controller setup failed" or "failed to initialize controller") using fmt.Errorf or the project's error wrapping helper, so logs show the setup context when startup fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/scheduling/reservations/failover/controller.go`:
- Around line 777-779: The current error return in the controller startup path
just returns err; update the error handling around the if err != nil { return
err } in the controller setup/startup function to wrap the error with contextual
text (e.g., "controller setup failed" or "failed to initialize controller")
using fmt.Errorf or the project's error wrapping helper, so logs show the setup
context when startup fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 109147f9-af90-465f-b161-9b6f17181e93
📒 Files selected for processing (1)
internal/scheduling/reservations/failover/controller.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.