Skip to content

Commit d02ebe2

Browse files
FrostGodFrostGod
authored
fix(kubernetes): prevent false sandbox suspension (#3567)
Signed-off-by: divesh <dgude@nvidia.com>
1 parent c8b20bf commit d02ebe2

1 file changed

Lines changed: 131 additions & 17 deletions

File tree

  • crates/openshell-driver-kubernetes/src

‎crates/openshell-driver-kubernetes/src/driver.rs‎

Lines changed: 131 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -102,8 +102,8 @@ const ANNOTATION_SANDBOX_RUNTIME_MAIN_PROCESS_SPEC: &str =
102102
const ANNOTATION_SANDBOX_RUNTIME_LOG_LEVEL: &str = "openshell.ai/sandbox-runtime-log-level";
103103
const ANNOTATION_SANDBOX_RUNTIME_NETWORK_POLICY_UID: &str =
104104
"openshell.ai/sandbox-runtime-network-policy-uid";
105-
const ANNOTATION_SANDBOX_RUNTIME_NETWORK_POLICY_VERSION: &str =
106-
"openshell.ai/sandbox-runtime-network-policy-version";
105+
const ANNOTATION_SANDBOX_RUNTIME_NETWORK_POLICY_GENERATION: &str =
106+
"openshell.ai/sandbox-runtime-network-policy-generation";
107107

108108
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
109109
enum SandboxRuntimeBootstrapPhase {
@@ -2480,6 +2480,9 @@ impl KubernetesComputeDriver {
24802480
let fence_uid = fence.metadata.uid.ok_or_else(|| {
24812481
KubernetesDriverError::Message("workload NetworkPolicy has no UID".to_string())
24822482
})?;
2483+
let fence_generation = fence.metadata.generation.ok_or_else(|| {
2484+
KubernetesDriverError::Message("workload NetworkPolicy has no generation".to_string())
2485+
})?;
24832486
let fence_resource_version = fence.metadata.resource_version.ok_or_else(|| {
24842487
KubernetesDriverError::Message(
24852488
"workload NetworkPolicy has no resourceVersion".to_string(),
@@ -2529,7 +2532,7 @@ impl KubernetesComputeDriver {
25292532
ANNOTATION_SANDBOX_RUNTIME_WORKLOAD_UID: workload_pod_uid.clone(),
25302533
ANNOTATION_SANDBOX_RUNTIME_SUPERVISOR_UID: supervisor_uid.clone(),
25312534
ANNOTATION_SANDBOX_RUNTIME_NETWORK_POLICY_UID: fence_uid.clone(),
2532-
ANNOTATION_SANDBOX_RUNTIME_NETWORK_POLICY_VERSION: fence_resource_version.clone(),
2535+
ANNOTATION_SANDBOX_RUNTIME_NETWORK_POLICY_GENERATION: fence_generation.to_string(),
25332536
}
25342537
}
25352538
})
@@ -2767,6 +2770,9 @@ impl KubernetesComputeDriver {
27672770
let fence_uid = fence.metadata.uid.ok_or_else(|| {
27682771
KubernetesDriverError::Message("workload NetworkPolicy has no UID".to_string())
27692772
})?;
2773+
let fence_generation = fence.metadata.generation.ok_or_else(|| {
2774+
KubernetesDriverError::Message("workload NetworkPolicy has no generation".to_string())
2775+
})?;
27702776
let fence_resource_version = fence.metadata.resource_version.ok_or_else(|| {
27712777
KubernetesDriverError::Message(
27722778
"workload NetworkPolicy has no resourceVersion".to_string(),
@@ -2808,7 +2814,7 @@ impl KubernetesComputeDriver {
28082814
ANNOTATION_SANDBOX_RUNTIME_WORKLOAD_UID: workload_pod_uid.clone(),
28092815
ANNOTATION_SANDBOX_RUNTIME_SUPERVISOR_UID: supervisor_uid,
28102816
ANNOTATION_SANDBOX_RUNTIME_NETWORK_POLICY_UID: fence_uid.clone(),
2811-
ANNOTATION_SANDBOX_RUNTIME_NETWORK_POLICY_VERSION: fence_resource_version.clone(),
2817+
ANNOTATION_SANDBOX_RUNTIME_NETWORK_POLICY_GENERATION: fence_generation.to_string(),
28122818
}
28132819
}
28142820
})
@@ -5489,9 +5495,12 @@ async fn sandbox_from_object_with_sandbox_runtime_readiness(
54895495
));
54905496
let (dependencies, workload_generation, supervisor_generation) =
54915497
tokio::join!(dependencies, workload_generation, supervisor_generation);
5492-
if dependencies != SandboxRuntimeControlAvailability::Available
5493-
|| workload_generation != SandboxRuntimeControlAvailability::Available
5494-
|| supervisor_generation != SandboxRuntimeControlAvailability::Available
5498+
// A transient API read failure is not evidence that the running
5499+
// boundary disappeared. Preserve the CR's published readiness for
5500+
// Unknown and let periodic reconciliation retry.
5501+
if dependencies == SandboxRuntimeControlAvailability::Unavailable
5502+
|| workload_generation == SandboxRuntimeControlAvailability::Unavailable
5503+
|| supervisor_generation == SandboxRuntimeControlAvailability::Unavailable
54955504
{
54965505
mark_sandbox_runtime_control_unavailable(&mut sandbox);
54975506
}
@@ -6730,6 +6739,22 @@ fn required_sandbox_annotation(
67306739
})
67316740
}
67326741

6742+
fn condition_observes_current_generation(
6743+
object: &DynamicObject,
6744+
condition: &serde_json::Value,
6745+
) -> bool {
6746+
match (
6747+
object.metadata.generation,
6748+
condition
6749+
.get("observedGeneration")
6750+
.and_then(serde_json::Value::as_i64),
6751+
) {
6752+
(Some(generation), Some(observed_generation)) => generation == observed_generation,
6753+
// Older Agent Sandbox API versions can omit observedGeneration.
6754+
_ => true,
6755+
}
6756+
}
6757+
67336758
fn status_from_object(obj: &DynamicObject) -> Option<SandboxStatus> {
67346759
let status = obj.data.get("status")?;
67356760
let status_obj = status.as_object()?;
@@ -6740,6 +6765,7 @@ fn status_from_object(obj: &DynamicObject) -> Option<SandboxStatus> {
67406765
.map(|items| {
67416766
items
67426767
.iter()
6768+
.filter(|condition| condition_observes_current_generation(obj, condition))
67436769
.filter_map(condition_from_value)
67446770
.collect::<Vec<_>>()
67456771
})
@@ -6877,10 +6903,10 @@ fn sandbox_runtime_namespace_fence_generation_matches(
68776903
== annotations
68786904
.get(ANNOTATION_SANDBOX_RUNTIME_NETWORK_POLICY_UID)
68796905
.map(String::as_str)
6880-
&& policy.metadata.resource_version.as_deref()
6881-
== annotations
6882-
.get(ANNOTATION_SANDBOX_RUNTIME_NETWORK_POLICY_VERSION)
6883-
.map(String::as_str)
6906+
&& annotations
6907+
.get(ANNOTATION_SANDBOX_RUNTIME_NETWORK_POLICY_GENERATION)
6908+
.and_then(|generation| generation.parse::<i64>().ok())
6909+
== policy.metadata.generation
68846910
}
68856911

68866912
fn kubernetes_sandbox_has_stopped_condition(obj: &DynamicObject) -> bool {
@@ -6890,8 +6916,9 @@ fn kubernetes_sandbox_has_stopped_condition(obj: &DynamicObject) -> bool {
68906916
.and_then(serde_json::Value::as_array)
68916917
.is_some_and(|conditions| {
68926918
conditions.iter().any(|condition| {
6893-
condition.get("type").and_then(serde_json::Value::as_str)
6894-
== Some(SANDBOX_SUSPENDED_CONDITION)
6919+
condition_observes_current_generation(obj, condition)
6920+
&& condition.get("type").and_then(serde_json::Value::as_str)
6921+
== Some(SANDBOX_SUSPENDED_CONDITION)
68956922
&& condition
68966923
.get("status")
68976924
.and_then(serde_json::Value::as_str)
@@ -6920,8 +6947,9 @@ fn kubernetes_sandbox_stop_failure(obj: &DynamicObject) -> Option<String> {
69206947
.as_array()?
69216948
.iter()
69226949
.find_map(|condition| {
6923-
let is_terminal = condition.get("type").and_then(serde_json::Value::as_str)
6924-
== Some(SANDBOX_SUSPENDED_CONDITION)
6950+
let is_terminal = condition_observes_current_generation(obj, condition)
6951+
&& condition.get("type").and_then(serde_json::Value::as_str)
6952+
== Some(SANDBOX_SUSPENDED_CONDITION)
69256953
&& condition
69266954
.get("status")
69276955
.and_then(serde_json::Value::as_str)
@@ -7074,7 +7102,7 @@ fn sandbox_runtime_suspension_completion_patch(resource_version: &str) -> serde_
70747102
ANNOTATION_SANDBOX_RUNTIME_WORKLOAD_UID: serde_json::Value::Null,
70757103
ANNOTATION_SANDBOX_RUNTIME_SUPERVISOR_UID: serde_json::Value::Null,
70767104
ANNOTATION_SANDBOX_RUNTIME_NETWORK_POLICY_UID: serde_json::Value::Null,
7077-
ANNOTATION_SANDBOX_RUNTIME_NETWORK_POLICY_VERSION: serde_json::Value::Null,
7105+
ANNOTATION_SANDBOX_RUNTIME_NETWORK_POLICY_GENERATION: serde_json::Value::Null,
70787106
ANNOTATION_SANDBOX_RUNTIME_READINESS: "unavailable",
70797107
},
70807108
}
@@ -8240,6 +8268,58 @@ mod tests {
82408268
));
82418269
}
82428270

8271+
fn sandbox_runtime_fence_pair_for_test() -> (NetworkPolicy, DynamicObject) {
8272+
let policy = NetworkPolicy {
8273+
metadata: ObjectMeta {
8274+
uid: Some("policy-uid".to_string()),
8275+
generation: Some(7),
8276+
resource_version: Some("200".to_string()),
8277+
..Default::default()
8278+
},
8279+
..Default::default()
8280+
};
8281+
let resource = ApiResource::from_gvk(&GroupVersionKind::gvk(
8282+
SANDBOX_GROUP,
8283+
SANDBOX_VERSION_V1BETA1,
8284+
SANDBOX_KIND,
8285+
));
8286+
let mut sandbox = DynamicObject::new("sandbox", &resource);
8287+
sandbox.metadata.annotations = Some(BTreeMap::from([
8288+
(
8289+
ANNOTATION_SANDBOX_RUNTIME_NETWORK_POLICY_UID.to_string(),
8290+
"policy-uid".to_string(),
8291+
),
8292+
(
8293+
ANNOTATION_SANDBOX_RUNTIME_NETWORK_POLICY_GENERATION.to_string(),
8294+
"7".to_string(),
8295+
),
8296+
]));
8297+
(policy, sandbox)
8298+
}
8299+
8300+
#[test]
8301+
fn sandbox_runtime_fence_reconciliation_uses_generation_not_resource_version() {
8302+
let (mut policy, sandbox) = sandbox_runtime_fence_pair_for_test();
8303+
8304+
assert!(
8305+
sandbox_runtime_namespace_fence_generation_matches(&policy, &sandbox),
8306+
"metadata-only writes must not invalidate an unchanged fence"
8307+
);
8308+
8309+
policy.metadata.generation = Some(8);
8310+
assert!(
8311+
!sandbox_runtime_namespace_fence_generation_matches(&policy, &sandbox),
8312+
"a policy spec change must invalidate the recorded fence generation"
8313+
);
8314+
8315+
policy.metadata.generation = Some(7);
8316+
policy.metadata.uid = Some("replacement-policy-uid".to_string());
8317+
assert!(
8318+
!sandbox_runtime_namespace_fence_generation_matches(&policy, &sandbox),
8319+
"a replacement policy must invalidate the recorded fence identity"
8320+
);
8321+
}
8322+
82438323
#[test]
82448324
fn sandbox_runtime_bootstrap_marker_and_age_gate_rollback() {
82458325
let started = Duration::from_hours(490_896);
@@ -8660,6 +8740,40 @@ mod tests {
86608740
assert!(kubernetes_sandbox_has_stopped_condition(&sandbox));
86618741
}
86628742

8743+
#[test]
8744+
fn stale_generation_conditions_do_not_change_current_status() {
8745+
let resource = ApiResource::from_gvk(&GroupVersionKind::gvk(
8746+
SANDBOX_GROUP,
8747+
SANDBOX_VERSION_V1BETA1,
8748+
SANDBOX_KIND,
8749+
));
8750+
let mut sandbox = DynamicObject::new("sandbox", &resource);
8751+
sandbox.metadata.generation = Some(2);
8752+
sandbox.data = serde_json::json!({
8753+
"status": {
8754+
"conditions": [
8755+
{
8756+
"type": "Suspended",
8757+
"status": "True",
8758+
"reason": "PodTerminated",
8759+
"observedGeneration": 1
8760+
},
8761+
{
8762+
"type": "Ready",
8763+
"status": "True",
8764+
"reason": "DependenciesReady",
8765+
"observedGeneration": 2
8766+
}
8767+
]
8768+
}
8769+
});
8770+
8771+
assert!(!kubernetes_sandbox_has_stopped_condition(&sandbox));
8772+
let status = status_from_object(&sandbox).expect("sandbox status");
8773+
assert_eq!(status.conditions.len(), 1);
8774+
assert_eq!(status.conditions[0].r#type, "Ready");
8775+
}
8776+
86638777
#[test]
86648778
fn beta_stop_requires_suspended_condition_and_deleted_pod() {
86658779
let resource = ApiResource::from_gvk(&GroupVersionKind::gvk(
@@ -11314,7 +11428,7 @@ mod tests {
1131411428
ANNOTATION_SANDBOX_RUNTIME_WORKLOAD_UID,
1131511429
ANNOTATION_SANDBOX_RUNTIME_SUPERVISOR_UID,
1131611430
ANNOTATION_SANDBOX_RUNTIME_NETWORK_POLICY_UID,
11317-
ANNOTATION_SANDBOX_RUNTIME_NETWORK_POLICY_VERSION,
11431+
ANNOTATION_SANDBOX_RUNTIME_NETWORK_POLICY_GENERATION,
1131811432
] {
1131911433
assert_eq!(
1132011434
complete["metadata"]["annotations"][annotation],

0 commit comments

Comments
 (0)