Try to find nodes that support the volume type#3013
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
PR SummaryMedium Risk Overview For volumes, orchestrator retries now prefer nodes whose labels include For sandboxes, required scheduling labels are built in one place: team scheduling labels (defaulting to New feature flags gate volume-type sandbox scheduling and unmatched-node volume fallback; cluster context for flags is aligned to use a resolved cluster UUID consistently. Reviewed by Cursor Bugbot for commit 7eaaa4f. Bugbot is set up for automated code reviews on this repo. Configure here. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The node pointer retrieved from the nodes slice in findVolumeNodes can potentially be nil, which would cause a nil pointer dereference panic when calling node.Labels(). A nil check should be added before accessing its methods to ensure safety.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| for _, node := range nodes { | ||
| labels := node.Labels() | ||
| if _, ok := labels[expectedLabel]; ok { | ||
| matchingNodes = append(matchingNodes, node) | ||
| } | ||
| } |
There was a problem hiding this comment.
The node pointer retrieved from the nodes slice can potentially be nil, which would cause a nil pointer dereference panic when calling node.Labels(). Adding a nil check before accessing its methods ensures safety.
for _, node := range nodes {
if node == nil {
continue
}
labels := node.Labels()
if _, ok := labels[expectedLabel]; ok {
matchingNodes = append(matchingNodes, node)
}
}
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 4 total unresolved issues (including 3 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit c721a5e. Configure here.
|
For the volumes, wondering if the specific handling for volumes in scheduling is necessary. In theory just the labels should be enough and then pipe through the existing pipeline |
2 participants
Old API behavior:
New API behavior:
flowchart TB subgraph OLD["🕐 OLD — sandbox create"] direction TB OB["Node boot:<br/>node gets custom labels only<br/>(no default, no volume labels)"] OS1["Get team's required<br/>sandbox-creation labels"] OS2["If a node has no labels,<br/>add 'default' to the node"] OS3["If request has no labels,<br/>add 'default' to the request"] OS4["Find nodes whose labels match"] OS5["Schedule on a matching node;<br/>if no room, try the next match"] OB --> OS1 --> OS2 --> OS3 --> OS4 --> OS5 end subgraph NEW["✨ NEW — sandbox create"] direction TB NB["Node boot:<br/>node gets (custom OR default label)<br/>AND persistent-volume-type labels"] F1{"sandbox-label-based-scheduling<br/>flag on?"} ANY["No label filtering:<br/>schedule on ANY node"] NS1["Get team's required<br/>sandbox-creation labels<br/>('default' if team has none)"] F2{"sandbox-volume-label-based-scheduling<br/>flag on?"} NS3["Add persistent<br/>volume-type labels"] NS4["Find ALL nodes matching<br/>(sandbox + volume labels)"] NS4b["Find ALL nodes matching<br/>sandbox labels only"] NS5["Schedule on those nodes"] NB --> F1 F1 -- no --> ANY F1 -- yes --> NS1 --> F2 F2 -- yes --> NS3 --> NS4 --> NS5 F2 -- no --> NS4b --> NS5 end OS1 -.migration.-> NS1flowchart TB subgraph OLD["🕐 OLD — volume create / delete"] direction TB OB["Node boot:<br/>node gets custom labels only<br/>(no volume labels)"] OV1["Randomize list of<br/>ALL orchestrators"] OV2["Try each one in order<br/>until one succeeds"] OB --> OV1 --> OV2 end subgraph NEW["✨ NEW — volume create / delete"] direction TB NB["Node boot:<br/>node gets persistent-volume-type labels"] NV1["Randomize list of orchestrators"] NV2["Try nodes WITH matching<br/>volume label first"] NV3{"Succeeded?"} NV4["Fall back to the rest,<br/>try until success or<br/>options exhausted"] NV5(["Done"]) NB --> NV1 --> NV2 --> NV3 NV3 -- yes --> NV5 NV3 -- "no (retryable)" --> NV4 --> NV5 end OV1 -.migration.-> NV1```