Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions crates/openshell-cli/src/commands/common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -576,9 +576,17 @@ pub fn print_sandbox_header(
pub fn ready_false_condition_message(
status: Option<&openshell_core::proto::SandboxStatus>,
) -> Option<String> {
let condition = status?.conditions.iter().find(|condition| {
condition.r#type == "Ready" && condition.status.eq_ignore_ascii_case("false")
})?;
let conditions = &status?.conditions;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

let condition = conditions
.iter()
.find(|condition| {
condition.r#type == "PodScheduled" && condition.status.eq_ignore_ascii_case("false")
})
Comment on lines +582 to +584

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

if condition.message.is_empty() {
if condition.reason.is_empty() {
Expand Down
67 changes: 67 additions & 0 deletions crates/openshell-cli/src/run.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8241,6 +8241,73 @@ mod tests {
);
}

#[test]
fn provisioning_timeout_prefers_failed_scheduling_over_ready_placeholder() {
let ready = SandboxCondition {
r#type: "Ready".into(),
status: "False".into(),
reason: "ConfigurationPending".into(),
message: "Waiting for effective configuration validation before workload activation"
.into(),
transition_time: None,
};
let scheduled = SandboxCondition {
r#type: "PodScheduled".into(),
status: "false".into(),
reason: "Unschedulable".into(),
message: "0/5 nodes are available: 1 Too many pods".into(),
transition_time: None,
};
for conditions in [
vec![ready.clone(), scheduled.clone()],
vec![scheduled.clone(), ready],
vec![scheduled],
] {
let status = SandboxStatus {
conditions,
..Default::default()
};
let detail = ready_false_condition_message(Some(&status));
assert_eq!(
detail.as_deref(),
Some("Unschedulable: 0/5 nodes are available: 1 Too many pods")
);
assert_eq!(
provisioning_timeout_message(300, None, detail.as_deref()),
"sandbox provisioning timed out after 300s. Last reported status: Unschedulable: 0/5 nodes are available: 1 Too many pods"
);
}
}

#[test]
fn ready_false_condition_message_falls_back_when_scheduling_has_not_failed() {
for scheduled_status in ["True", "Unknown", ""] {
let status = SandboxStatus {
conditions: vec![
SandboxCondition {
r#type: "PodScheduled".into(),
status: scheduled_status.into(),
reason: "SchedulingStatus".into(),
message: "Scheduling observation".into(),
transition_time: None,
},
SandboxCondition {
r#type: "Ready".into(),
status: "False".into(),
reason: "ConfigurationPending".into(),
message: "Waiting for configuration".into(),
transition_time: None,
},
],
..Default::default()
};
assert_eq!(
ready_false_condition_message(Some(&status)).as_deref(),
Some("ConfigurationPending: Waiting for configuration")
);
}
}

#[test]
fn ready_false_condition_message_ignores_non_ready_conditions() {
let status = SandboxStatus {
Expand Down
Loading