fix(cli): report scheduling failures in provisioning timeouts by FrostGod · Pull Request #5344 · NVIDIA/OpenShell

FrostGod · 2026-11-06T19:51:15Z

Summary

When sandbox creation times out with PodScheduled=False, report the scheduler's reason and message instead of a generic Ready=False placeholder such as ConfigurationPending.

Related Issue

Closes #5230

Changes

Prefer failed pod scheduling conditions regardless of their position in the status list.
Preserve the existing Ready-condition fallback, message formatting, and GPU hint.
Add regression coverage for scheduling failures, non-failing scheduling states, and the final timeout text.

Release Notes

Testing

Z3_LIBRARY_PATH_OVERRIDE=/opt/homebrew/opt/z3/lib mise exec -- cargo test -p openshell-cli --locked: 731 passed, 6 ignored, 0 failed.
mise exec -- cargo clippy -p openshell-cli --all-targets --locked -- -D warnings
cargo fmt -p openshell-cli -- --check and git diff --check
Unit tests added for the reported scheduling failure and fallback behavior.

Live Kubernetes reproduction was not run. The Z3 path selects the installed Homebrew library required by the CLI integration tests.

The repository-wide pre-commit hook failed during Python environment installation (ty wheel copy: No such file or directory). The commit uses the scoped verification above instead of rerunning that hook.

Checklist

Follows Conventional Commits
Commits are signed off (DCO)

copy-pr-bot · 2026-11-06T19:51:19Z

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

EmilienM

Needs a human read: 2 minor (inline)

Both are small polish: an empty PodScheduled condition can suppress the Ready fallback detail, and the helper's name and scope no longer match its behavior. Nothing blocking or major, but this is a code change, which this review can't approve on its own.

How this was reviewed

Commit 6c99e83b, round 1
Standard review, because crates/openshell-cli/src/commands/common.rs and crates/openshell-cli/src/run.rs
Passes: architecture, performance, security, scalability, rust, then correctness
Independent skeptics checked each candidate finding (1 per minor candidate); 3 of 5 held up
Repo rules read from main: AGENTS.md, CLAUDE.md, CONTRIBUTING.md
Not covered: nothing skipped

EmilienM · 2026-11-06T20:11:12Z

+        .find(|condition| {
+            condition.r#type == "PodScheduled" && condition.status.eq_ignore_ascii_case("false")
+        })


Minor: A PodScheduled=False condition with neither reason nor message is still picked here, and the function then returns None, so the timeout message drops the "Last reported status" detail the Ready fallback used to print. The Kubernetes driver passes conditions through with both fields optional (condition_from_value in crates/openshell-driver-kubernetes/src/driver.rs), so this shape can reach the CLI.

Suggested change

.find(|condition| {

condition.r#type == "PodScheduled" && condition.status.eq_ignore_ascii_case("false")

})

.find(|condition| {

condition.r#type == "PodScheduled"

&& condition.status.eq_ignore_ascii_case("false")

&& !(condition.reason.is_empty() && condition.message.is_empty())

})

A regression test with an empty PodScheduled=False next to a Ready=False with detail would cover it.

Fixed in 7d336d3: empty PodScheduled=False conditions are skipped so the Ready detail is kept, with a regression test.

EmilienM · 2026-11-06T20:11:12Z

-    let condition = status?.conditions.iter().find(|condition| {
-        condition.r#type == "Ready" && condition.status.eq_ignore_ascii_case("false")
-    })?;
+    let conditions = &status?.conditions;


Minor: With the PodScheduled preference in place, the name ready_false_condition_message no longer describes what the function does, and lifecycle_phase_error (run.rs:4245) picks up the new precedence on start/stop/resume error paths, outside the provisioning-timeout scope of #5230 and uncovered by the new tests. A rename (say provisioning_blocker_message) or a short doc comment on the precedence, plus either a test or a deliberate note for the lifecycle path, would keep the next reader from tripping over it.

Renamed to provisioning_blocker_message and added a lifecycle_phase_error test in d0c3e04; the start/stop/resume precedence is intentional.

u0324020 · 2026-11-10T01:47:22Z

Thanks for the fix — this is exactly the symptom I hit on a real cluster (#5230). I compared it with the prototype on our fork (VicOne-OSS-TW/OpenShell@ac35c2c) and have one suggestion and one offer.

Suggestion (same spot as EmilienM's first point): a PodScheduled=False condition with empty reason and message is currently selected and then returns None, so the timeout loses the Ready detail. Filtering by type/status first and taking the first condition that actually yields a summary keeps both the preference and the fallback:

let conditions = &status?.conditions;
let false_condition_message = |condition_type: &str| {
    conditions
        .iter()
        .filter(|c| c.r#type == condition_type && c.status.eq_ignore_ascii_case("false"))
        .find_map(condition_summary)
};
false_condition_message("PodScheduled").or_else(|| false_condition_message("Ready"))

with the existing reason/message formatting moved into fn condition_summary(&SandboxCondition) -> Option<String>. I have a unit test for that case (PodScheduled=False with no detail + Ready=False/ConfigurationPending → "ConfigurationPending") on the fork branch; feel free to lift it, or I can send it over.

Offer: the description notes a live Kubernetes reproduction wasn't run. I reproduced the original failure on EKS (0/11 nodes are available: 11 Insufficient cpu) and can run the CLI from this branch against the same scenario and paste the actual timeout output here, if that would help with maintainer approval.

FrostGod · 2026-11-12T06:52:02Z

@u0324020 are you planning to add on top of this PR ?

u0324020 · 2026-11-12T06:58:43Z

@FrostGod happy to, if useful. I have the find_map variant plus three tests (prefers PodScheduled, ignores PodScheduled=True, falls back when PodScheduled has no reason/message) at VicOne-OSS-TW/OpenShell@ac35c2c, DCO signed. Since the branch lives in this repo I can't push to it directly; I can open a PR against your branch, or you can cherry-pick that commit, whichever you prefer. Either way I can also test the final branch on the real unschedulable case I hit (0/11 nodes are available: 11 Insufficient cpu on EKS) and post the output here.

When the sandbox pod cannot be scheduled, the readiness timeout message only showed Ready=False (ConfigurationPending) although the sandbox status already carried PodScheduled=False with the scheduler's reason. Prefer a failing PodScheduled condition when summarizing why the sandbox is not ready, and fall back to the Ready condition as before. Signed-off-by: Jane Chen <gojane20@gmail.com>

… errors Signed-off-by: divesh <dgude@nvidia.com>

github-actions · 2026-11-13T06:48:16Z

All contributors have signed the DCO ✍️ ✅
_{Posted by the DCO Assistant Lite bot.}

FrostGod · 2026-11-13T06:59:03Z

@u0324020 , can you comment this , I have taken your commit on top
I have read the DCO document and I hereby sign the DCO.

FrostGod · 2026-11-13T07:00:00Z

/ok to test d0c3e04

u0324020 · 2026-11-13T07:06:30Z

I have read the DCO document and I hereby sign the DCO.

FrostGod · 2026-11-13T07:11:10Z

@u0324020 , feel free to take up any issue you like and tag me for review in future, if I miss it please reach out in slack
https://cloud-native.slack.com/archives/C0B6Y8WQHU1

Thanks!

EmilienM reviewed Nov 6, 2026

View reviewed changes

FrostGod mentioned this pull request Nov 9, 2026

openshell sandbox create timeout reports ConfigurationPending when the pod could not be scheduled #5230

Open

rhuss mentioned this pull request Nov 11, 2026

bug: sandbox create leaves stale resource on ImagePullFailed, blocking retry #2675

Open

jhjaggars mentioned this pull request Nov 12, 2026

bug(kubernetes): a ResourceQuota or LimitRange rejecting the workspace PVC leaves the sandbox in Provisioning for 300 s with an unrelated message #5491

Open

2 tasks

u0324020 and others added 2 commits November 12, 2026 22:45

refactor(cli): rename provisioning blocker helper and cover lifecycle…

d0c3e04

… errors Signed-off-by: divesh <dgude@nvidia.com>

FrostGod force-pushed the fix/5230-unschedulable-sandbox-reporting/FrostGod branch from 7726e36 to d0c3e04 Compare November 13, 2026 06:48

FrostGod marked this pull request as ready for review November 13, 2026 06:59

Conversation

FrostGod commented Nov 6, 2026

Summary

Related Issue

Changes

Release Notes

Testing

Checklist

Uh oh!

copy-pr-bot Bot commented Nov 6, 2026

Uh oh!

EmilienM left a comment

Choose a reason for hiding this comment

Uh oh!

EmilienM Nov 6, 2026

Choose a reason for hiding this comment

Uh oh!

FrostGod Nov 13, 2026

Choose a reason for hiding this comment

Uh oh!

EmilienM Nov 6, 2026

Choose a reason for hiding this comment

Uh oh!

FrostGod Nov 13, 2026

Choose a reason for hiding this comment

Uh oh!

u0324020 commented Nov 10, 2026

Uh oh!

FrostGod commented Nov 12, 2026

Uh oh!

u0324020 commented Nov 12, 2026

Uh oh!

github-actions Bot commented Nov 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

FrostGod commented Nov 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

FrostGod commented Nov 13, 2026

Uh oh!

u0324020 commented Nov 13, 2026

Uh oh!

FrostGod commented Nov 13, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions Bot commented Nov 13, 2026 •

edited

Loading

FrostGod commented Nov 13, 2026 •

edited

Loading