Skip to content

feat(cuda): add helper daemon protocol codec - #312

Open
hhzhang16 wants to merge 1 commit into
mainfrom
hannahz/customstorage-daemon-wire-codec
Open

hhzhang16 wants to merge 1 commit into
mainfrom
hannahz/customstorage-daemon-wire-codec

Conversation

@hhzhang16

@hhzhang16 hhzhang16 commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

What type of PR is this?

  • enhancement

What this PR does / why we need it:

Adds a codec between PageBroker and its CUDA helper process. It's used as a safe binary message reader and writer for communication between PageBroker and its CUDA helper. It’s needed because PageBroker and the CUDA helper run as separate processes, so they can’t directly pass C++ objects or call each other’s functions. They communicate over a Unix socket and therefore need an agreed byte format.

Which issue(s) this PR fixes:

Fixes # N/A

How was this tested?

  • Native C++20 build with -O2 -Wall -Wextra -Werror
  • ASan and UBSan build and execution
  • Request and response golden-byte tests
  • Round-trip and malformed-frame tests
  • Decoder-side cgroup and job-file limit tests with otherwise valid aggregate lengths
  • Empty-payload, reserved-field, identity, enum, flag, truncation, overflow, and partial-output tests
  • Linux cuda-transfer-contracts-builder Docker target, including the complete CUDA helper contract suite
  • git diff --check

Special notes for your reviewer:

This MR defines the single canonical PageBroker↔CUDA-helper codec but does not yet start a helper daemon or route PageBroker requests through it -- the following PR will add the helper/server consumer and semantic admission checks.

Does this PR introduce an API change?

N/A

Additional documentation, e.g. enhancement proposals, usage docs:

The CUDA checkpoint helper README now documents the private co-versioned daemon protocol and its validation target.

Checklist

  • Commits are signed off (git commit -s), per CONTRIBUTING.md
  • make check test passes locally
  • Documentation is updated where behavior changed

Summary by CodeRabbit

  • New Features

    • Added a versioned protocol for communication between the CUDA checkpoint daemon and its clients.
    • Added support for encoding and decoding checkpoint requests and responses, including health, checkpoint, restore, lock, and unlock operations.
    • Added validation for malformed messages, invalid values, oversized payloads, and inconsistent metadata.
  • Tests

    • Added comprehensive protocol tests covering valid messages, round trips, size limits, malformed data, and validation failures.
    • Added standalone protocol test execution to the CUDA helper test workflow.
  • Documentation

    • Documented the private daemon protocol and its validation tests.

Signed-off-by: Hannah Zhang <hannahz@nvidia.com>
@coderabbitai

coderabbitai Bot commented Sep 15, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: 3aab8376-dd3a-4ed8-b2e9-21a684c0ff5c

📥 Commits

Reviewing files that changed from the base of the PR and between f4da8a8 and d17a522.

📒 Files selected for processing (6)
  • agent/Dockerfile
  • agent/Makefile
  • agent/cmd/cuda-checkpoint-helper/README.md
  • agent/cmd/cuda-checkpoint-helper/daemon_protocol.cpp
  • agent/cmd/cuda-checkpoint-helper/daemon_protocol.hpp
  • agent/cmd/cuda-checkpoint-helper/daemon_protocol_test.cpp

Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.


Walkthrough

The CUDA helper now defines a versioned binary daemon protocol. It encodes and decodes bounded request and response frames, rejects malformed data, adds focused codec tests, and runs those tests through the Docker and Make build paths.

Changes

CUDA daemon protocol

Layer / File(s) Summary
Protocol contract and codec
agent/cmd/cuda-checkpoint-helper/daemon_protocol.hpp, agent/cmd/cuda-checkpoint-helper/daemon_protocol.cpp
Defines request and response structures, fixed little-endian headers, limits, enums, flags, and bounded encode/decode functions. Validation rejects malformed headers, invalid values, inconsistent lengths, embedded NULs, oversized fields, and unsupported reserved payloads.
Protocol validation tests
agent/cmd/cuda-checkpoint-helper/daemon_protocol_test.cpp
Adds golden-wire, round-trip, boundary, malformed-input, enum, flag, payload, and job-file identity tests for requests and responses.
Build and documentation integration
agent/Dockerfile, agent/Makefile, agent/cmd/cuda-checkpoint-helper/README.md
Builds the protocol sources and test executable. The test target runs the executable, and the README documents the private protocol and validation coverage.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~30 minutes

Merge Risk: ⚪ Minimal · up to d17a5

No production path currently consumes this codec, and the added protocol validation is covered by integrated tests. The change is ready for normal merge checks.

🚥 Pre-merge checks | ✅ 6 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 6.06% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 33 functions across 3 files. (3 skipped: 3… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (6 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title uses the required conventional commit prefix, feat, includes a relevant scope, clearly describes the daemon protocol codec change, and is 44 characters long.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Breaking Api Changes ✅ Passed PASS: The authoritative pull-request diff changes only agent Dockerfile, Makefile, README, and new CUDA helper C++ protocol files. It changes no paths under api/ and no Go files, so it cannot remove, …
Rbac Least Privilege ✅ Passed PASS. The reviewed range changes only CUDA helper protocol/build files and README; it adds no kubebuilder RBAC marker and changes no Helm RBAC manifest. Existing kubebuilder markers use explicit verbs…
Full details: Docstring Coverage

Explanation

Docstring coverage is 6.06% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 33 functions across 3 files. (3 skipped: 3 unsupported.)

  • Fix all pre-merge checks with AI

Comment @coderabbitai help to get the list of available commands.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant