diff --git a/crates/openshell-server/src/grpc/validation.rs b/crates/openshell-server/src/grpc/validation.rs index cb0d01b9d..cc3834954 100644 --- a/crates/openshell-server/src/grpc/validation.rs +++ b/crates/openshell-server/src/grpc/validation.rs @@ -197,8 +197,8 @@ pub(super) fn validate_sandbox_spec(name: &str, spec: &SandboxSpec) -> Result<() validate_env_entries(&tmpl.environment, "spec.template.environment")?; } - // --- spec.resource_requirements.gpu --- - validate_gpu_request_fields(spec)?; + // --- spec.resource_requirements --- + validate_resource_requirement_fields(spec)?; if !spec.command.is_empty() { validate_main_process_command(&spec.command)?; @@ -296,7 +296,7 @@ fn validate_main_process_command(command: &[String]) -> Result<(), Status> { Ok(()) } -fn validate_gpu_request_fields(spec: &SandboxSpec) -> Result<(), Status> { +fn validate_resource_requirement_fields(spec: &SandboxSpec) -> Result<(), Status> { if openshell_core::gpu::sandbox_gpu_count(spec.resource_requirements.as_ref()) == Some(0) { return Err(invalid_argument( "spec.resource_requirements.gpu.count", @@ -304,9 +304,47 @@ fn validate_gpu_request_fields(spec: &SandboxSpec) -> Result<(), Status> { )); } + if let Some(cpu) = spec + .resource_requirements + .as_ref() + .and_then(|requirements| requirements.cpu.as_ref()) + { + if let Some(quantity) = cpu.quantity.as_deref() { + validate_cpu_quantity(quantity, "spec.resource_requirements.cpu.quantity")?; + } else { + return Err(Status::invalid_argument( + "spec.resource_requirements.cpu must set quantity", + )); + } + } + + if let Some(memory) = spec + .resource_requirements + .as_ref() + .and_then(|requirements| requirements.memory.as_ref()) + { + if let Some(quantity) = memory.quantity.as_deref() { + validate_memory_quantity(quantity, "spec.resource_requirements.memory.quantity")?; + } else { + return Err(Status::invalid_argument( + "spec.resource_requirements.memory must set quantity", + )); + } + } + Ok(()) } +fn validate_cpu_quantity(value: &str, field_name: &str) -> Result<(), Status> { + openshell_core::quantity::validate_cpu_quantity(value, field_name) + .map_err(Status::invalid_argument) +} + +fn validate_memory_quantity(value: &str, field_name: &str) -> Result<(), Status> { + openshell_core::quantity::validate_memory_quantity(value, field_name) + .map_err(Status::invalid_argument) +} + /// Validate template-level field sizes. fn validate_sandbox_template(tmpl: &SandboxTemplate) -> Result<(), Status> { // String fields. @@ -360,6 +398,7 @@ fn validate_sandbox_template(tmpl: &SandboxTemplate) -> Result<(), Status> { ), )); } + reject_legacy_template_cpu_memory_resources(s)?; } if let Some(ref s) = tmpl.driver_config { let size = s.encoded_len(); @@ -401,6 +440,31 @@ fn reject_gateway_owned_driver_config_keys(config: &prost_types::Struct) -> Resu } } } + + Ok(()) +} + +fn reject_legacy_template_cpu_memory_resources( + resources: &prost_types::Struct, +) -> Result<(), Status> { + for section_name in ["limits", "requests"] { + let Some(value) = resources.fields.get(section_name) else { + continue; + }; + let Some(prost_types::value::Kind::StructValue(section)) = value.kind.as_ref() else { + return Err(Status::invalid_argument(format!( + "template.resources.{section_name} must be an object" + ))); + }; + + for resource_name in ["cpu", "memory"] { + if section.fields.contains_key(resource_name) { + return Err(Status::invalid_argument(format!( + "template.resources.{section_name}.{resource_name} is no longer supported; use spec.resource_requirements.{resource_name}.quantity" + ))); + } + } + } Ok(()) } @@ -1188,6 +1252,8 @@ mod tests { let spec = SandboxSpec { resource_requirements: Some(openshell_core::proto::ResourceRequirements { gpu: Some(openshell_core::proto::GpuResourceRequirements { count: None }), + cpu: None, + memory: None, }), ..Default::default() }; @@ -1199,6 +1265,8 @@ mod tests { let spec = SandboxSpec { resource_requirements: Some(openshell_core::proto::ResourceRequirements { gpu: Some(openshell_core::proto::GpuResourceRequirements { count: Some(2) }), + cpu: None, + memory: None, }), ..Default::default() }; @@ -1210,6 +1278,8 @@ mod tests { let spec = SandboxSpec { resource_requirements: Some(openshell_core::proto::ResourceRequirements { gpu: Some(openshell_core::proto::GpuResourceRequirements { count: Some(0) }), + cpu: None, + memory: None, }), ..Default::default() }; @@ -1218,6 +1288,160 @@ mod tests { assert!(err.message().contains("gpu count must be greater than 0")); } + #[test] + fn validate_sandbox_spec_accepts_cpu_and_memory_requirements() { + let spec = SandboxSpec { + resource_requirements: Some(openshell_core::proto::ResourceRequirements { + gpu: None, + cpu: Some(openshell_core::proto::CpuResourceRequirements { + quantity: Some("500m".to_string()), + }), + memory: Some(openshell_core::proto::MemoryResourceRequirements { + quantity: Some("2Gi".to_string()), + }), + }), + ..Default::default() + }; + + assert!(validate_sandbox_spec("compute-sandbox", &spec).is_ok()); + } + + #[test] + fn validate_sandbox_spec_rejects_invalid_cpu_requirements() { + let spec = SandboxSpec { + resource_requirements: Some(openshell_core::proto::ResourceRequirements { + gpu: None, + cpu: Some(openshell_core::proto::CpuResourceRequirements { + quantity: Some("0".to_string()), + }), + memory: Some(openshell_core::proto::MemoryResourceRequirements { + quantity: Some("2Gi".to_string()), + }), + }), + ..Default::default() + }; + + let err = validate_sandbox_spec("compute-sandbox", &spec).unwrap_err(); + assert_eq!(err.code(), Code::InvalidArgument); + assert!( + err.message() + .contains("spec.resource_requirements.cpu.quantity") + ); + } + + #[test] + fn validate_sandbox_spec_accepts_cpu_and_memory_quantities() { + let spec = SandboxSpec { + resource_requirements: Some(openshell_core::proto::ResourceRequirements { + gpu: None, + cpu: Some(openshell_core::proto::CpuResourceRequirements { + quantity: Some("250m".to_string()), + }), + memory: Some(openshell_core::proto::MemoryResourceRequirements { + quantity: Some("512Mi".to_string()), + }), + }), + ..Default::default() + }; + + assert!(validate_sandbox_spec("compute-sandbox", &spec).is_ok()); + } + + #[test] + fn validate_sandbox_spec_rejects_invalid_memory_quantity() { + let spec = SandboxSpec { + resource_requirements: Some(openshell_core::proto::ResourceRequirements { + gpu: None, + cpu: None, + memory: Some(openshell_core::proto::MemoryResourceRequirements { + quantity: Some("0".to_string()), + }), + }), + ..Default::default() + }; + + let err = validate_sandbox_spec("compute-sandbox", &spec).unwrap_err(); + assert_eq!(err.code(), Code::InvalidArgument); + assert!( + err.message() + .contains("spec.resource_requirements.memory.quantity") + ); + } + + #[test] + fn validate_sandbox_spec_rejects_legacy_template_cpu_memory_resources() { + use prost_types::{Struct, Value, value::Kind}; + + let mut limits = std::collections::BTreeMap::new(); + limits.insert( + "cpu".to_string(), + Value { + kind: Some(Kind::StringValue("500m".to_string())), + }, + ); + let mut fields = std::collections::BTreeMap::new(); + fields.insert( + "limits".to_string(), + Value { + kind: Some(Kind::StructValue(Struct { fields: limits })), + }, + ); + let spec = SandboxSpec { + template: Some(SandboxTemplate { + resources: Some(Struct { fields }), + ..Default::default() + }), + ..Default::default() + }; + + let err = validate_sandbox_spec("legacy-resources", &spec).unwrap_err(); + assert_eq!(err.code(), Code::InvalidArgument); + assert!(err.message().contains("template.resources.limits.cpu")); + assert!( + err.message() + .contains("spec.resource_requirements.cpu.quantity") + ); + } + + #[test] + fn validate_sandbox_spec_rejects_non_object_legacy_resource_sections() { + use prost_types::{ListValue, Struct, Value, value::Kind}; + + let invalid_sections = [ + ( + "limits", + "string", + Some(Kind::StringValue("500m".to_string())), + ), + ( + "requests", + "list", + Some(Kind::ListValue(ListValue { values: Vec::new() })), + ), + ("limits", "null", None), + ]; + + for (section_name, name, kind) in invalid_sections { + let mut fields = std::collections::BTreeMap::new(); + fields.insert(section_name.to_string(), Value { kind }); + let spec = SandboxSpec { + template: Some(SandboxTemplate { + resources: Some(Struct { fields }), + ..Default::default() + }), + ..Default::default() + }; + + let message = format!("{name} {section_name} section should be rejected"); + let err = validate_sandbox_spec("legacy-resources", &spec).expect_err(&message); + assert_eq!(err.code(), Code::InvalidArgument, "{name}"); + assert_eq!( + err.message(), + format!("template.resources.{section_name} must be an object") + ); + } + } + #[test] fn validate_sandbox_spec_accepts_empty_defaults() { assert!(validate_sandbox_spec("", &default_spec()).is_ok());