Repository navigation
Conversation
Fixes #5230 Signed-off-by: divesh <dgude@nvidia.com>
EmilienM
left a comment
There was a problem hiding this comment.
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
| .find(|condition| { | ||
| condition.r#type == "PodScheduled" && condition.status.eq_ignore_ascii_case("false") | ||
| }) |
There was a problem hiding this comment.
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.
| .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.
| let condition = status?.conditions.iter().find(|condition| { | ||
| condition.r#type == "Ready" && condition.status.eq_ignore_ascii_case("false") | ||
| })?; | ||
| let conditions = &status?.conditions; |
There was a problem hiding this comment.
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.
Summary
When sandbox creation times out with
PodScheduled=False, report the scheduler's reason and message instead of a genericReady=Falseplaceholder such asConfigurationPending.Related Issue
Closes #5230
Changes
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 warningscargo fmt -p openshell-cli -- --checkandgit diff --checkLive 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 (
tywheel copy:No such file or directory). The commit uses the scoped verification above instead of rerunning that hook.Checklist