Add ReservationsResourceRouter for routing reservations based on AZ#598
Merged
SoWieMarkus merged 7 commits intomainfrom Mar 19, 2026
Merged
Add ReservationsResourceRouter for routing reservations based on AZ#598SoWieMarkus merged 7 commits intomainfrom
SoWieMarkus merged 7 commits intomainfrom
Conversation
SoWieMarkus
requested review from
PhilippMatthes,
auhlig,
juliusclausnitzer,
mblos and
umswmayj
as code owners
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Reservation GVK and registers a ReservationsResourceRouter; changes HypervisorResourceRouter to error on nil input and use Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/multicluster/routers.go (1)
44-66: LGTM! Implementation follows the established pattern.The
ReservationsResourceRoutercorrectly implements theResourceRouterinterface and mirrors theHypervisorResourceRouterpattern with appropriate type assertions for both pointer and value forms.One minor note: The error message on Line 63 says "reservation does not have availability zone label" but
AvailabilityZoneis aSpecfield, not a label. Consider adjusting for consistency:💡 Optional: Clarify error message
resAZ := res.Spec.AvailabilityZone if resAZ == "" { - return false, errors.New("reservation does not have availability zone label") + return false, errors.New("reservation does not have availability zone set") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/multicluster/routers.go` around lines 44 - 66, Update the error message in ReservationsResourceRouter.Match to accurately describe the missing availability zone as a spec field rather than a label: locate the Match method on ReservationsResourceRouter and change the error returned when res.Spec.AvailabilityZone is empty (currently "reservation does not have availability zone label") to something like "reservation does not have an availability zone" or "reservation.Spec.AvailabilityZone is empty" so it references res.Spec.AvailabilityZone and avoids calling it a label; no behavior changes required.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/multicluster/routers.go`:
- Around line 44-66: Update the error message in
ReservationsResourceRouter.Match to accurately describe the missing availability
zone as a spec field rather than a label: locate the Match method on
ReservationsResourceRouter and change the error returned when
res.Spec.AvailabilityZone is empty (currently "reservation does not have
availability zone label") to something like "reservation does not have an
availability zone" or "reservation.Spec.AvailabilityZone is empty" so it
references res.Spec.AvailabilityZone and avoids calling it a label; no behavior
changes required.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5ac4ff6b-109d-400f-beec-da777579ebd4
📒 Files selected for processing (3)
cmd/main.gopkg/multicluster/routers.gopkg/multicluster/routers_test.go
PhilippMatthes
approved these changes
Mar 19, 2026
umswmayj
approved these changes
Mar 19, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/multicluster/routers_test.go (1)
35-42:⚠️ Potential issue | 🟡 MinorUse
testlib.Ptrfor pointer fixtures in_test.goPointer test objects are constructed with
&...{}in both tables. Please switch these totestlib.Ptr(...)to match repo test conventions.As per coding guidelines,
**/*_test.go: Usetestlib.Ptrfor test cases that require pointer values.Also applies to: 125-132
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/multicluster/routers_test.go` around lines 35 - 42, Replace literal pointer constructions in the test table with the repo helper: instead of using &hv1.Hypervisor{...} (and other &...{} fixtures at the other occurrence around the 125-132 block), wrap the value with testlib.Ptr(...) so the test uses the canonical pointer fixture helper; update the entries for the case named "matching AZ pointer" and the other pointer case to use testlib.Ptr(hv1.Hypervisor{...}) (and any other pointer instances in this file) while keeping the same fields and labels.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/multicluster/routers_test.go`:
- Around line 81-84: Add test cases that pass typed nil pointers (e.g.
(*hv1.Hypervisor)(nil)) instead of untyped nil so the type assertion branches
are exercised; update the table-driven tests in routers_test.go to include cases
like name: "typed nil pointer doesn't dereference" with obj:
(*hv1.Hypervisor)(nil) (and any other relevant types used elsewhere, e.g.
VirtualMachine) and wantErr: true, and run them to reproduce the panic so you
can then add a nil-check in the implementation branch that handles
*hv1.Hypervisor to avoid dereferencing a nil pointer.
---
Outside diff comments:
In `@pkg/multicluster/routers_test.go`:
- Around line 35-42: Replace literal pointer constructions in the test table
with the repo helper: instead of using &hv1.Hypervisor{...} (and other &...{}
fixtures at the other occurrence around the 125-132 block), wrap the value with
testlib.Ptr(...) so the test uses the canonical pointer fixture helper; update
the entries for the case named "matching AZ pointer" and the other pointer case
to use testlib.Ptr(hv1.Hypervisor{...}) (and any other pointer instances in this
file) while keeping the same fields and labels.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ab7e86ff-6a1a-4fae-9bc2-ece2ab6cd1ae
📒 Files selected for processing (2)
pkg/multicluster/routers.gopkg/multicluster/routers_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/multicluster/routers.go
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/multicluster/routers.go (1)
30-33:⚠️ Potential issue | 🔴 CriticalGuard typed-nil pointers before dereference to prevent panics
obj == nildoes not catch typed-nil interfaces. Both pointer branches can panic on*vdereference (HypervisorResourceRouteralready does in CI at Line 32), andReservationsResourceRouterhas the same defect pattern.🔧 Proposed fix
func (h HypervisorResourceRouter) Match(obj any, labels map[string]string) (bool, error) { var hv hv1.Hypervisor @@ switch v := obj.(type) { case *hv1.Hypervisor: + if v == nil { + return false, errors.New("object is nil") + } hv = *v case hv1.Hypervisor: hv = v @@ func (r ReservationsResourceRouter) Match(obj any, labels map[string]string) (bool, error) { var res v1alpha1.Reservation @@ switch v := obj.(type) { case *v1alpha1.Reservation: + if v == nil { + return false, errors.New("object is nil") + } res = *v case v1alpha1.Reservation: res = vAlso applies to: 59-63
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/multicluster/routers.go` around lines 30 - 33, The type-switch in HypervisorResourceRouter (and similarly in ReservationsResourceRouter) dereferences pointer cases without guarding against typed-nil interfaces, which can panic; first check if obj == nil and in the pointer type branch verify the pointer is non-nil (e.g., if v == nil then handle as error/return) before doing hv = *v, and for the non-pointer branch assign directly; update both occurrences (the pointer case around Hypervisor and the analogous code at lines 59-63) to perform these nil checks rather than unconditionally dereferencing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/multicluster/routers.go`:
- Around line 67-70: The error message when checking labels["availability_zone"]
is misleading (it says "in spec"); update the branch that reads
availabilityZone, ok := labels["availability_zone"] to return a clear message
about the missing label instead (e.g. errors.New("cluster is missing
availability_zone label")) so the error reflects the actual check on labels and
references the availability_zone key and availabilityZone variable.
---
Outside diff comments:
In `@pkg/multicluster/routers.go`:
- Around line 30-33: The type-switch in HypervisorResourceRouter (and similarly
in ReservationsResourceRouter) dereferences pointer cases without guarding
against typed-nil interfaces, which can panic; first check if obj == nil and in
the pointer type branch verify the pointer is non-nil (e.g., if v == nil then
handle as error/return) before doing hv = *v, and for the non-pointer branch
assign directly; update both occurrences (the pointer case around Hypervisor and
the analogous code at lines 59-63) to perform these nil checks rather than
unconditionally dereferencing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a1aa414d-1795-43a0-a966-fe375d7ca03c
📒 Files selected for processing (2)
pkg/multicluster/routers.gopkg/multicluster/routers_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/multicluster/routers_test.go
…d ReservationsResourceRouter
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/multicluster/routers.go (1)
65-68:⚠️ Potential issue | 🟡 MinorFix misleading error message for missing cluster label.
Line 67's error message says "in spec" but this code checks
labels["availability_zone"](cluster labels), not the reservation's spec. This is also inconsistent withHypervisorResourceRouterat line 39 which correctly uses "label".✏️ Suggested fix
availabilityZone, ok := labels["availability_zone"] if !ok { - return false, errors.New("cluster does not have availability zone in spec") + return false, errors.New("cluster does not have availability zone label") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/multicluster/routers.go` around lines 65 - 68, The error message for the availability_zone lookup is misleading: the code checks the cluster's labels map (availabilityZone, ok := labels["availability_zone"]) but returns "cluster does not have availability zone in spec"; change the returned error to mention "label" (e.g., "cluster does not have availability_zone label") to match HypervisorResourceRouter's wording and accurately reflect what's being checked (update the error string wherever availability_zone label is required).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/multicluster/routers.go`:
- Around line 65-68: The error message for the availability_zone lookup is
misleading: the code checks the cluster's labels map (availabilityZone, ok :=
labels["availability_zone"]) but returns "cluster does not have availability
zone in spec"; change the returned error to mention "label" (e.g., "cluster does
not have availability_zone label") to match HypervisorResourceRouter's wording
and accurately reflect what's being checked (update the error string wherever
availability_zone label is required).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6e05b65c-4c47-42aa-bae7-fdce96ed732c
📒 Files selected for processing (1)
pkg/multicluster/routers.go
Contributor
Test Coverage ReportTest Coverage 📊: 68.1% |
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.
Changes
.Spec.AvailabilityZoneazlabel toavailability_zone