Skip to content

fix(compute): preserve protobuf configuration integers - #5117

Open
shiju-nv wants to merge 1 commit into
NVIDIA:mainfrom
shiju-nv:fix/5106-protobuf-config-integers/shiju-nv
Open

shiju-nv wants to merge 1 commit into
NVIDIA:mainfrom
shiju-nv:fix/5106-protobuf-config-integers/shiju-nv

Conversation

@shiju-nv

@shiju-nv shiju-nv commented Nov 2, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

A valid integer driver setting, such as Kubernetes tolerationSeconds, can arrive as a floating-point JSON number after protobuf decoding and fail typed parsing. This preserves exactly represented whole numbers through the shared converter and the Kubernetes rendering path.

Related Issue

Closes #5106. Part of #5114.

Changes

  • Use one fallible protobuf-to-JSON converter in all five first-party compute driver factories.
  • Preserve fractional values without rounding and reject non-finite protobuf numbers.
  • Add an integer toleration to the existing Kubernetes scheduling regression.

Release Notes

Per-sandbox integer driver settings retain their JSON number type. The separate gateway TOML PID-limit path is unchanged.

Testing

  • Checks appropriate to the affected code and behavior pass
  • Unit tests added/updated (if applicable)
  • E2E tests added/updated (if applicable)

Checklist

  • Follows Conventional Commits
  • Commits are signed off (DCO)
  • Architecture docs updated (if applicable)

@shiju-nv shiju-nv added the test:e2e Requires end-to-end coverage label Nov 2, 2026
@github-actions

github-actions Bot commented Nov 2, 2026

Copy link
Copy Markdown

Label test:e2e applied for 50d3a86. Open the existing run and click Re-run all jobs to execute with the label set. The run will execute the standard E2E suite after building the required gateway, sandbox, and supervisor images once. The matching required CI gate status on this PR will flip green automatically once the run finishes.

@shiju-nv
shiju-nv force-pushed the fix/5106-protobuf-config-integers/shiju-nv branch from 50d3a86 to c80a053 Compare November 2, 2026 14:46
An integer driver setting such as Kubernetes `tolerationSeconds` can become a fractional JSON number during protobuf decoding and fail typed configuration parsing. This preserves whole-number values through the shared converter and the actual Kubernetes rendering path.

Signed-off-by: Shiju <shiju@nvidia.com>

@Karthik-Chowdary Karthik-Chowdary left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Additional wire-level evidence for de0645c23: I exercised the unmodified shared converter from base cc0642fa and this head in an isolated Rust harness, using real prost::Message Struct encode/decode before typed serde parsing.

  • A nested PodSpec with tolerationSeconds of 0, 512, and 9007199254740994 (2^53+2) decoded into the actual k8s_openapi::api::core::v1::PodSpec and matched direct JSON on head. Base failed with invalid type: floating point ... expected i64.
  • Nested lists of six negative i64 values (including −(2^53+2) and i64::MIN) and all 256 i8 values also matched direct typed JSON on head; both fail on base. Float/null/bool/string controls remained equal.
  • Control checks still rejected i8 overflow/fractions and nonexact producer integers; those are existing serde/producer behavior, not new guarantees of this PR.

The five added checks give base 2 passed / 3 failed, head 5 passed. The head helper’s existing ten unit tests also passed. Direct dependencies were pinned to the PR lockfile versions (prost/prost-types 0.14.3, serde 1.0.229, serde_json 1.0.149, k8s-openapi 0.24.0). This verifies binary transport plus shared typed conversion, not driver factories, Kubernetes admission, or live sandbox behavior.

Reproducer: import the unchanged crates/openshell-core/src/proto_struct.rs; form JSON {"spec":{"containers":[],"tolerations":[{"operator":"Exists","effect":"NoExecute","tolerationSeconds":9007199254740994}]}}; call json_object_to_struct, then Struct::decode(proto.encode_to_vec().as_slice()), convert using the production API, and deserialize the resulting spec as PodSpec.

///
/// Reject non-finite numbers anywhere in the configuration instead of turning
/// them into null, which could silently clear an optional constraint.
pub fn struct_to_typed_json_value(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This now sits next to struct_to_json_value / struct_to_json_object / value_to_json, and all four are documented as "for typed serde decoding". Both struct_to_json_value and struct_to_typed_json_value return the same untyped serde_json::Value; "typed" describes what the caller does next, not what the function does. The actual difference (whole numbers stay integers, non-finite numbers are rejected instead of becoming null) is not visible from the names, so it is hard to tell which one a new caller should use. The shorter, infallible one is the easy pick, and that reintroduces #5106.

Every call site in this PR immediately does serde_json::from_value::<T>(json) and never uses the intermediate JSON, so what about folding that in and keeping the conversion private?

pub fn from_struct<T: DeserializeOwned>(
    config: &prost_types::Struct,
) -> Result<T, StructDecodeError> {
    Ok(serde_json::from_value(struct_to_exact_json(config)?)?)
}

with StructDecodeError having NonFiniteNumber and Deserialize(#[from] serde_json::Error) variants. Benefits:

  • The name says what you get, a typed value, so there is one obvious choice for decoding config.
  • Each driver call site goes from two map_errs to one, e.g. proto_struct::from_struct(config).map_err(|err| format!("invalid docker driver_config: {err}")).
  • The exact-JSON conversion does not need to be public; a caller that really wants the JSON can use from_struct::<serde_json::Value>.

The existing functions still have general-purpose callers (CLI display, OPA input, interceptor patches), so I would keep them but reword their docs to say they are lossy: whole numbers become floats and non-finite numbers become null.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:platform test:e2e Requires end-to-end coverage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Preserve integer driver settings when converting protobuf configuration

4 participants