Schedule ephemeral volumes in the same zone/region as the machine#1509
Conversation
|
Warning Review limit reached
Next review available in: 18 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughEphemeral volume creation now copies missing zone and region selectors from ChangesEphemeral volume topology
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Machine
participant Controller
participant EphemeralVolume
Machine->>Controller: provide MachinePoolSelector
Controller->>EphemeralVolume: construct volume with template selector
Controller->>EphemeralVolume: copy missing zone and region labels
EphemeralVolume-->>Controller: retain template-defined selector values
🚥 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 |
![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
🧹 Nitpick comments (2)
internal/controllers/compute/machine_ephemeralvolume_controller_test.go (2)
196-242: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winStrengthen the precedence test by also verifying non-conflicting region propagation.
The test verifies that a user-specified zone (
zone-b) takes precedence over the machine's zone (zone-a), but the machine only has a zone label. Adding a region label to the machine'sMachinePoolSelectorand asserting it propagates to the volume (while zone is still overridden) would cover the partial-override scenario where some topology labels are user-specified and others are propagated.♻️ Proposed test enhancement
MachinePoolSelector: map[string]string{ - "topology.ironcore.dev/zone": "zone-a", + "topology.ironcore.dev/zone": "zone-a", + "topology.ironcore.dev/region": "region-1", },Eventually(Object(ephemVolume)).Should(SatisfyAll( HaveField("Spec.VolumePoolSelector", HaveKeyWithValue("topology.ironcore.dev/zone", "zone-b")), + HaveField("Spec.VolumePoolSelector", HaveKeyWithValue("topology.ironcore.dev/region", "region-1")), ))🤖 Prompt for 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. In `@internal/controllers/compute/machine_ephemeralvolume_controller_test.go` around lines 196 - 242, Strengthen the test in “should not overwrite user-specified topology labels in ephemeral volume pool selector” by adding a non-conflicting region label to the machine’s MachinePoolSelector, while retaining the user-specified zone override in the volume template. Update the Eventually assertion on Spec.VolumePoolSelector to verify both zone-b remains unchanged and the machine’s region label is propagated.
149-242: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider using topology label constants instead of hardcoded strings.
The tests use literal strings
"topology.ironcore.dev/zone"and"topology.ironcore.dev/region"instead ofcommonv1alpha1.TopologyLabelZoneandcommonv1alpha1.TopologyLabelRegion. Using the constants would keep tests in sync if the label keys ever change.🤖 Prompt for 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. In `@internal/controllers/compute/machine_ephemeralvolume_controller_test.go` around lines 149 - 242, Replace the hardcoded topology label keys in the tests for ephemeral volume selector propagation with commonv1alpha1.TopologyLabelZone and commonv1alpha1.TopologyLabelRegion, including the machine selectors, volume selectors, and HaveKeyWithValue assertions in both test cases.
🤖 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 `@internal/controllers/compute/machine_ephemeralvolume_controller.go`:
- Around line 68-83: Clone ephemeral.VolumeTemplate.Spec.VolumePoolSelector when
constructing the volume, matching the existing maps.Clone handling for
annotations, before passing it to copyTopologyLabels. Ensure the new volume owns
an independent map so copyTopologyLabels cannot mutate the shared template
selector.
---
Nitpick comments:
In `@internal/controllers/compute/machine_ephemeralvolume_controller_test.go`:
- Around line 196-242: Strengthen the test in “should not overwrite
user-specified topology labels in ephemeral volume pool selector” by adding a
non-conflicting region label to the machine’s MachinePoolSelector, while
retaining the user-specified zone override in the volume template. Update the
Eventually assertion on Spec.VolumePoolSelector to verify both zone-b remains
unchanged and the machine’s region label is propagated.
- Around line 149-242: Replace the hardcoded topology label keys in the tests
for ephemeral volume selector propagation with commonv1alpha1.TopologyLabelZone
and commonv1alpha1.TopologyLabelRegion, including the machine selectors, volume
selectors, and HaveKeyWithValue assertions in both test cases.
🪄 Autofix (Beta)
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
Run ID: f01a1bf9-3899-4570-b0eb-906a705776d9
📒 Files selected for processing (2)
internal/controllers/compute/machine_ephemeralvolume_controller.gointernal/controllers/compute/machine_ephemeralvolume_controller_test.go
Copy zone and region labels from `MachinePoolSelector` to `VolumePoolSelector` when creating ephemeral volumes, without overwriting labels already set on the volume. This ensures ephemeral volumes are scheduled in the same zone/region as the machine they belong to, except if the user has specifically defined different topology labels. Signed-off-by: Daniel Gonzalez Nothnagel <daniel.gonzalez.nothnagel@sap.com>
Signed-off-by: Daniel Gonzalez Nothnagel <daniel.gonzalez.nothnagel@sap.com>
gonzolino
force-pushed
the
fix/eph-vol-zone
branch
from
4b9362b to
88fc206
Compare
maxmoehl
left a comment
maxmoehl
left a comment
There was a problem hiding this comment.
This ensures ephemeral volumes are scheduled in the same zone/region as the machine they belong to, except if the user has specifically defined different topology labels.
An important caveat is that this does not cover the case where the user did not put any labels into the selector. In that case the volume and machine could still end up in separate zones, no?
Otherwise, this is a worthwhile improvement, thank you!
Yes good point, this fix will only apply for situations were topology labels are given. I am not sure how to solve it for cases were we don't have them 🤔 |
3 participants
Proposed Changes
Propagate topology labels from machine to ephemeral volumes
Copy zone and region labels from
MachinePoolSelectortoVolumePoolSelectorwhen creating ephemeral volumes, without overwriting labels already set on the volume.This ensures ephemeral volumes are scheduled in the same zone/region as the machine they belong to, except if the user has specifically defined different topology labels.
Fixes #1497
Summary by CodeRabbit
New Features
Bug Fixes