Repository navigation
[Issue-1464] replace callback with token management API - #1515
realAaronWu wants to merge 20 commits into
Conversation
…attern.
What changed:
- New CuObjTokenManager: RAII wrapper. Pool-aware memory
registration with refcounting de-duplicates per-page registerMem calls
into a single cuMemObjGetDescriptor per contiguous pool. Supports
rdma_pool_size backend parameter hint for O(1) registration.
- Rewritten awsS3DellObsClient: overrides standard iS3Client methods
(putObjectAsync/getObjectAsync) to inject RDMA tokens as x-rdma-info
headers. No separate iDellS3RdmaClient interface, no dynamic_cast.
Per-request fallback to parent HTTP body transfer for DRAM; hard fail
for VRAM (not CPU-addressable).
- Simplified S3DellObsObjEngineImpl: overrides only registerMem,
deregisterMem, getSupportedMems, getClient. Inherits prepXfer,
postXfer, checkXfer, releaseReqH unchanged from the parent.
- Deleted rdma_interface.h (iDellS3RdmaClient no longer needed).
Shared code (minimal, outside s3_accel/dell/):
- s3/engine_impl.cpp: widen isValidPrepXferParams to accept VRAM_SEG
in addition to DRAM_SEG for local memory type (4-line change).
- meson.build: replace rdma_interface.h with cuobj_token_manager.{h,cpp}
(2-line change).
Signed-off-by: Aaron Wu <aaron.wu@dell.com>
Signed-off-by: Aaron Wu <aaron.wu@dell.com>
…attern.
What changed:
- New CuObjTokenManager: RAII wrapper. Pool-aware memory
registration with refcounting de-duplicates per-page registerMem calls
into a single cuMemObjGetDescriptor per contiguous pool. Supports
rdma_pool_size backend parameter hint for O(1) registration.
- Rewritten awsS3DellObsClient: overrides standard iS3Client methods
(putObjectAsync/getObjectAsync) to inject RDMA tokens as x-rdma-info
headers. No separate iDellS3RdmaClient interface, no dynamic_cast.
Per-request fallback to parent HTTP body transfer for DRAM; hard fail
for VRAM (not CPU-addressable).
- Simplified S3DellObsObjEngineImpl: overrides only registerMem,
deregisterMem, getSupportedMems, getClient. Inherits prepXfer,
postXfer, checkXfer, releaseReqH unchanged from the parent.
- Deleted rdma_interface.h (iDellS3RdmaClient no longer needed).
Shared code (minimal, outside s3_accel/dell/):
- s3/engine_impl.cpp: widen isValidPrepXferParams to accept VRAM_SEG
in addition to DRAM_SEG for local memory type (4-line change).
- meson.build: replace rdma_interface.h with cuobj_token_manager.{h,cpp}
(2-line change).
Signed-off-by: Aaron Wu <aaron.wu@dell.com>
…com/realAaronWu/nixl into feature-issue-1464-replace-callback
|
👋 Hi realAaronWu! Thank you for contributing to ai-dynamo/nixl. Your PR reviewers will review your contribution then trigger the CI to test your changes. 🚀 |
Signed-off-by: Aaron Wu <aaron.wu@dell.com>
Signed-off-by: Aaron Wu <aaron.wu@dell.com>
Signed-off-by: Aaron Wu <aaron.wu@dell.com>
Remove the pool_hint optimization and associated complexity (region tracking, refcounting, findContainingRegion) from CuObjTokenManager. The token manager now does a simple 1:1 mapping: each registerMemory() call triggers one cuMemObjGetDescriptor, each deregisterMemory() maps to one cuMemObjPutDescriptor, and generateToken() calls cuMemObjGetRDMAToken with buffer_offset=0. Signed-off-by: Aaron Wu <aaron.wu@dell.com>
Signed-off-by: Aaron Wu <aaron.wu@dell.com>
… fixes Refactor OBJ unit tests to support both standard S3 and Dell/Accel engine configurations, integrating tests from PR ai-dynamo#1630 while fixing compilation and correctness issues for the Pattern B refactored codebase. Key changes: - Extract shared test infrastructure into obj_test_base.h: - ObjTestConfig with Standard, CRT, Accel, and Dell configurations - Enhanced mockS3Client with per-key outcomes, delays, and error injection - mockDellS3Client using iS3Client (not deleted iDellS3RdmaClient) - objTestBase, objParamTestFixture, and objTestFixture base classes - Add obj_cuobj.cpp for Dell/Accel-specific tests (conditionally compiled): - 18 objDellTestFixture tests covering RDMA read/write, memory registration, transfer failures, descriptor validation, and object segment management - 14 CuObjTokenManagerTest tests for token generation edge cases - Parameterized ObjAccelClientTests suite (Accel + Dell configs) - Simplify obj.cpp by removing duplicated infrastructure and all HAVE_CUOBJ_CLIENT guarded code, now handled in obj_cuobj.cpp - Update meson.build to conditionally compile obj_cuobj.cpp when cuobj_dep is found Fixes from PR ai-dynamo#1630 original tests: - Remove rdma_interface.h include (deleted in Pattern B) - Fix mock client inheritance (iS3Client, not iDellS3RdmaClient) - Fix registerMem expectations: mock path (tokenMgr_=null) returns NIXL_SUCCESS with nullptr metadata, not backend errors - Fix prepXfer vs postXfer validation: devId-to-key mapping is checked in postXfer, not prepXfer - Remove 3 tests with untestable expectations (descriptor count mismatch, unregistered cuObject address, large memory offset)
…place-callback-only
…place-callback-only # Conflicts: # src/plugins/obj/s3_accel/dell/engine_impl.cpp # test/gtest/unit/obj/obj.cpp # test/unit/plugins/object/nixl_object_test.cpp
Signed-off-by: Aaron Wu <aaron.wu@dell.com>
…place-callback-only Resolve conflicts in test/gtest/unit/obj/obj_cuobj.cpp and test/gtest/unit/obj/obj_test_base.h caused by PR ai-dynamo#1630 landing on main. Resolution strategy: - Keep HEAD's CuObjTokenManager architecture (no iDellS3RdmaClient, no mockDellS3Client, no rdma_interface.h) - Keep HEAD's mock behavior: single mockS3Client for all engine types, DRAM/VRAM registerMem returns nullptr metadata when tokenMgr_ is null - Keep HEAD's validation semantics: devId registration checked in postXfer, not prepXfer - Keep HEAD's exceptionThrowingMockS3Client and CuObjTokenManager integration tests - Adopt origin/main's value-initialization (= {}) for nixlBlobDesc - Adopt origin/main's polling loop and buffer verification in testTransferFailure - Adopt origin/main's DellEngineMismatchedDescriptorCounts test (parent-class prepXfer validation, independent of mock design)
Signed-off-by: Aaron Wu <aaron.wu@dell.com>
📝 WalkthroughWalkthroughThis PR refactors the Dell ObjectScale S3 engine from callback-based cuObject I/O to manual RDMA token management. A new ChangesDell ObjectScale RDMA token management refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Infer (1.2.0)test/gtest/unit/obj/obj_cuobj.cppIn file included from test/gtest/unit/obj/obj_cuobj.cpp:33: ... [truncated 1143 characters] ... l/lib/clang/18/include" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/plugins/obj/s3_accel/dell/cuobj_token_manager.h`:
- Around line 6-7: Replace the current header guard macro
NIXL_OBJ_PLUGIN_S3_DELL_CUOBJ_TOKEN_MANAGER_H in cuobj_token_manager.h with the
full repo-relative form NIXL_SRC_PLUGINS_OBJ_S3_ACCEL_DELL_CUOBJ_TOKEN_MANAGER_H
(uppercase, slashes->underscores, prefixed by NIXL_ and suffixed _H) so the
`#ifndef/`#define/#endif use the repository-wide naming convention and pass style
checks.
In `@src/plugins/obj/s3_accel/dell/DESIGN.md`:
- Around line 31-42: Update the two fenced code blocks under "Before (Pattern
A)" and "After (Pattern B)" to include a language identifier (use "text") and
ensure each fenced block is surrounded by blank lines; specifically modify the
blocks labeled "Before (Pattern A)" and "After (Pattern B)" so they become
```text ... ``` with an empty line above and below each fenced block and
preserve the existing content/indentation.
In `@src/plugins/obj/s3_accel/dell/engine_impl.cpp`:
- Around line 64-68: The constructor currently returns early when
tokenMgr_->isConnected() is false, leaving a half-initialized engine; change
these early returns (the block creating tokenMgr_ using
CuObjTokenManager(CUOBJ_PROTO_RDMA_DC_V1) and the similar block at the later
spot) to throw an exception (e.g., std::runtime_error) with a clear message that
includes the token manager connection failure so the engine creation fails fast
and callers never observe a constructed object with tokenMgr_ or getClient() ==
nullptr.
In `@src/plugins/obj/s3_accel/dell/engine_impl.h`:
- Around line 23-26: Add a Dell-specific preflight in the upload path to reject
non-zero PUT offsets before any async PUT is launched: in the Dell engine
implementation (code around DefaultObjEngineImpl::postXfer / prepXfer usage),
inspect the request's remote_desc.addr.offset (or equivalent write offset field)
whenever the code is about to call getClient()->putObjectAsync() (or
awsS3DellObsClient::putObjectAsync()), and return a synchronous error/abort the
request if offset != 0 instead of forwarding the descriptor; apply the same
check to the other symmetric upload paths that would call putObjectAsync so
invalid non-zero offsets are never enqueued into the async path.
In `@test/gtest/unit/obj/obj_test_base.h`:
- Around line 413-415: In testTransferWithSize when operation == NIXL_READ the
test only asserts test_buffer[0], which is too weak; update the read-data
validation to verify multiple positions: if buffer_size is small (e.g., <= 64)
assert the entire buffer equals the expected pattern (compare every byte in
test_buffer to expected value), otherwise sample a few positions (e.g., index 0,
buffer_size/2, buffer_size-1) and use EXPECT_EQ on those positions; locate the
check using symbols operation, NIXL_READ, test_buffer, buffer_size and replace
the single EXPECT_EQ(test_buffer[0], 'A') with the described full-or-sampled
assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: da17eb9c-8df6-4f4e-a099-fb8f671e134b
📒 Files selected for processing (15)
src/plugins/obj/meson.buildsrc/plugins/obj/s3/engine_impl.cppsrc/plugins/obj/s3_accel/dell/DESIGN.mdsrc/plugins/obj/s3_accel/dell/client.cppsrc/plugins/obj/s3_accel/dell/client.hsrc/plugins/obj/s3_accel/dell/cuobj_token_manager.cppsrc/plugins/obj/s3_accel/dell/cuobj_token_manager.hsrc/plugins/obj/s3_accel/dell/engine_impl.cppsrc/plugins/obj/s3_accel/dell/engine_impl.hsrc/plugins/obj/s3_accel/dell/rdma_interface.htest/gtest/unit/obj/meson.buildtest/gtest/unit/obj/obj.cpptest/gtest/unit/obj/obj_cuobj.cpptest/gtest/unit/obj/obj_test_base.htest/unit/plugins/object/nixl_object_test.cpp
💤 Files with no reviewable changes (1)
- src/plugins/obj/s3_accel/dell/rdma_interface.h
1. Header guard: use full repo-relative naming convention (NIXL_SRC_PLUGINS_OBJ_S3_ACCEL_DELL_CUOBJ_TOKEN_MANAGER_H) 2. DESIGN.md: fix markdown lint (MD031/MD040) — add language identifier and blank lines around fenced code blocks 3. engine_impl.cpp: fail fast on CuObjTokenManager connection failure by throwing std::runtime_error instead of silently returning from the constructor with a half-initialized engine 4. engine_impl: add postXfer override to reject non-zero PUT offsets before dispatching async operations. Dell ObjectScale RDMA PUT does not support partial writes; failing early prevents partially-enqueued multi-descriptor requests. 5. obj_test_base.h: strengthen testTransferWithSize read validation from single-byte check to multi-position (front, middle, back) to catch partial or offset-corrupted reads Signed-off-by: Aaron Wu <aaron.wu@dell.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/plugins/obj/s3_accel/dell/engine_impl.h`:
- Around line 87-103: Update the public API docs for the newly overridden method
postXfer: replace the brief class overview above the class to remove the
statement that transfer lifecycle is inherited unchanged and note that Dell
overrides postXfer with RDMA PUT semantics, and add a Doxygen block for postXfer
including `@brief` describing behavior (reject non-zero remote offsets for WRITE,
delegate for READ), `@param` entries for operation, local, remote, remote_agent,
handle, opt_args, and `@return` describing nixl_status_t outcomes; ensure the
comment precedes the postXfer declaration and follows the project's /** ... */
Doxygen style and coding guidelines.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2a703eb4-6cb2-4de4-b655-2229dbf1b75f
📒 Files selected for processing (5)
src/plugins/obj/s3_accel/dell/DESIGN.mdsrc/plugins/obj/s3_accel/dell/cuobj_token_manager.hsrc/plugins/obj/s3_accel/dell/engine_impl.cppsrc/plugins/obj/s3_accel/dell/engine_impl.htest/gtest/unit/obj/obj_test_base.h
jgoldsch12
left a comment
jgoldsch12
left a comment
There was a problem hiding this comment.
Approving this PR with some minor comments.
There was a problem hiding this comment.
I have nothing wrong with these changes - they seem purely style-istic and are probably unnecessary
Signed-off-by: Aaron Wu <aaron.wu@dell.com>
|
👀 Investigating |
|
👀 Investigating |
|
🤖 CI Triage Agent — I have everything needed for a full diagnosis. Here is the complete triage report: Summary: PR #1515 failed the "PR Size Check" because it modifies 871 lines in existing files, exceeding the enforced limit of 500 lines. Root cause: The CI workflow runs This is a policy enforcement failure, not a code defect. The job completed cleanly with no hangs or infrastructure issues (the entire run finished in under 1 second of wall time). Implicated commit: File: Suggested fix: The PR author has two options:
Additionally, a secondary (non-blocking) notice is present in the logs: Related: #1464 (referenced in the branch name as the parent issue), none found for the size-check policy itself.
|
|
🤖 CI Triage Agent — I have a clear and complete picture. Here is the diagnosis: Summary: PR #1515 fails the PR Size Check — it modifies 871 lines (above the 500-line hard limit enforced by CI). Root cause: The CI workflow runs a The decisive log lines are: Implicated commit: File: The CI workflow file that enforces the limit ( Suggested fix: The PR author has two options:
Option 1 (splitting the PR) is the standard and recommended path. Related: #1515 (the failing PR), Issue #1464 (the parent feature issue)
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/gtest/unit/obj/obj_cuobj.cpp (1)
938-946:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't turn token-generation failures into passing tests.
After
mgr.isConnected()and a successfulregisterMemory(), swallowingstd::runtime_errormakes these tests pass for both "no RDMA token support here" and real regressions inCuObjTokenManager::generatePutToken/generateGetToken. Please skip explicitly when the environment cannot generate tokens, or assert success otherwise.💡 One way to preserve hardware gating without reporting false greens
try { - std::string token = mgr.generatePutToken(buffer_, kBufferSize); + const std::string token = mgr.generatePutToken(buffer_, kBufferSize); // Token should be non-empty if generation succeeded. EXPECT_FALSE(token.empty()); } - catch (const std::runtime_error &) { - // cuMemObjGetRDMAToken failed — expected if no RDMA NIC is present. - // This is not a test failure; token generation requires real hardware. + catch (const std::runtime_error &ex) { + GTEST_SKIP() << "RDMA token generation unavailable on this system: " << ex.what(); }try { - std::string token = mgr.generateGetToken(buffer_, kBufferSize); + const std::string token = mgr.generateGetToken(buffer_, kBufferSize); EXPECT_FALSE(token.empty()); } - catch (const std::runtime_error &) { - // Expected if no RDMA NIC. + catch (const std::runtime_error &ex) { + GTEST_SKIP() << "RDMA token generation unavailable on this system: " << ex.what(); }Also applies to: 962-968
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/gtest/unit/obj/obj_cuobj.cpp`:
- Around line 727-818: The DiagnoseConnection test block in
CuObjTokenManagerTest fails clang-format; reformat the entire
TEST_F(CuObjTokenManagerTest, DiagnoseConnection) function to comply with the
project's clang-format rules (or run the repo's clang-format script/IDE
integration) so indentation, spacing, and line-wrapping match the style; ensure
changes touch the TEST_F block and related variables (buffer_, kBufferSize, mgr,
tests, and all device/memory test code) so the Clang Format check passes.
- Around line 727-818: The test DiagnoseConnection in the TEST_F
CuObjTokenManagerTest should be made opt-in for local debugging; rename or move
it so it won't run by default (for example prefix the test name with DISABLED_
to become DISABLED_DiagnoseConnection) or wrap its body behind a debug-only
helper so normal CI won't execute it; update any references to the test name
accordingly and ensure the fixture symbols (CuObjTokenManagerTest,
DiagnoseConnection, mgr.isConnected()) remain unchanged except for the test name
change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c2496cbd-d274-430e-9fb6-99188f4a6de6
📒 Files selected for processing (1)
test/gtest/unit/obj/obj_cuobj.cpp
| TEST_F(CuObjTokenManagerTest, DiagnoseConnection) { | ||
| std::cout << "=== CuObjTokenManager connection diagnostics ===" << std::endl; | ||
|
|
||
| #ifdef HAVE_CUDA | ||
| std::cout << " HAVE_CUDA: yes" << std::endl; | ||
|
|
||
| // GPU info | ||
| int dev_count = 0; | ||
| cudaGetDeviceCount(&dev_count); | ||
| std::cout << " CUDA devices: " << dev_count << std::endl; | ||
| for (int i = 0; i < dev_count; ++i) { | ||
| cudaDeviceProp prop; | ||
| cudaGetDeviceProperties(&prop, i); | ||
| std::cout << " [" << i << "] " << prop.name | ||
| << " (compute " << prop.major << "." << prop.minor << ")" << std::endl; | ||
| } | ||
| #else | ||
| std::cout << " HAVE_CUDA: no" << std::endl; | ||
| #endif | ||
| std::cout << " useCuda_: " << (useCuda_ ? "true" : "false") << std::endl; | ||
| std::cout << " buffer_: " << buffer_ << std::endl; | ||
|
|
||
| CuObjTokenManager mgr; | ||
| std::cout << " isConnected(): " << (mgr.isConnected() ? "true" : "false") << std::endl; | ||
|
|
||
| if (!mgr.isConnected()) { | ||
| std::cout << "=== end diagnostics (not connected) ===" << std::endl; | ||
| return; | ||
| } | ||
|
|
||
| // Try different memory types and sizes | ||
| struct MemTest { | ||
| const char *label; | ||
| void *ptr; | ||
| size_t size; | ||
| std::function<void()> cleanup; | ||
| }; | ||
| std::vector<MemTest> tests; | ||
|
|
||
| // Test 1: fixture buffer_ (cudaMalloc or posix_memalign) with full size | ||
| tests.push_back({"buffer_ (full 64K)", buffer_, kBufferSize, nullptr}); | ||
|
|
||
| // Test 2: fixture buffer_ with 4K | ||
| tests.push_back({"buffer_ (4K)", buffer_, 4096, nullptr}); | ||
|
|
||
| #ifdef HAVE_CUDA | ||
| // Test 3: fresh cudaMalloc 64K | ||
| void *gpu_buf = nullptr; | ||
| if (cudaMalloc(&gpu_buf, kBufferSize) == cudaSuccess) { | ||
| tests.push_back({"cudaMalloc 64K", gpu_buf, kBufferSize, | ||
| [gpu_buf]() { cudaFree(gpu_buf); }}); | ||
| } | ||
|
|
||
| // Test 4: cudaMallocHost (pinned host) 64K | ||
| void *pinned_buf = nullptr; | ||
| if (cudaMallocHost(&pinned_buf, kBufferSize) == cudaSuccess) { | ||
| tests.push_back({"cudaMallocHost 64K", pinned_buf, kBufferSize, | ||
| [pinned_buf]() { cudaFreeHost(pinned_buf); }}); | ||
| } | ||
|
|
||
| // Test 5: cudaMallocManaged (unified) 64K | ||
| void *managed_buf = nullptr; | ||
| if (cudaMallocManaged(&managed_buf, kBufferSize) == cudaSuccess) { | ||
| tests.push_back({"cudaMallocManaged 64K", managed_buf, kBufferSize, | ||
| [managed_buf]() { cudaFree(managed_buf); }}); | ||
| } | ||
| #endif | ||
|
|
||
| // Test 6: posix_memalign 64K | ||
| void *sys_buf = nullptr; | ||
| posix_memalign(&sys_buf, 4096, kBufferSize); | ||
| if (sys_buf) { | ||
| tests.push_back({"posix_memalign 64K", sys_buf, kBufferSize, | ||
| [sys_buf]() { free(sys_buf); }}); | ||
| } | ||
|
|
||
| for (auto &t : tests) { | ||
| cuObjErr_t rc = mgr.registerMemory(t.ptr, t.size); | ||
| std::cout << " registerMemory(" << t.label << ", ptr=" << t.ptr | ||
| << ", size=" << t.size << ") = " << rc << std::endl; | ||
| if (rc == CU_OBJ_SUCCESS) { | ||
| mgr.deregisterMemory(t.ptr); | ||
| } | ||
| } | ||
|
|
||
| // Cleanup | ||
| for (auto &t : tests) { | ||
| if (t.cleanup) t.cleanup(); | ||
| } | ||
|
|
||
| std::cout << "=== end diagnostics ===" << std::endl; | ||
| } |
There was a problem hiding this comment.
Run clang-format on this diagnostics block before merge.
The current Clang Format Check is failing on this region, so the PR is blocked until this block is reformatted.
🧰 Tools
🪛 GitHub Actions: Clang Format Check / clang-format
[error] 737-737: clang-format check failed (diff found). The std::cout line formatting and line breaks do not match clang-format style.
[error] 761-811: clang-format check failed (diff found). Multiple formatting discrepancies in initializer/list formatting, lambda expressions, and stream line breaks.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/gtest/unit/obj/obj_cuobj.cpp` around lines 727 - 818, The
DiagnoseConnection test block in CuObjTokenManagerTest fails clang-format;
reformat the entire TEST_F(CuObjTokenManagerTest, DiagnoseConnection) function
to comply with the project's clang-format rules (or run the repo's clang-format
script/IDE integration) so indentation, spacing, and line-wrapping match the
style; ensure changes touch the TEST_F block and related variables (buffer_,
kBufferSize, mgr, tests, and all device/memory test code) so the Clang Format
check passes.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Make this an opt-in diagnostic instead of a normal unit test.
This body only logs probes and returns early on !mgr.isConnected(), so it stays green even when every registration probe fails. Please move it behind DISABLED_ or a helper that is only run during local debugging.
♻️ Minimal change if you want to keep it in-tree
-TEST_F(CuObjTokenManagerTest, DiagnoseConnection) {
+TEST_F(CuObjTokenManagerTest, DISABLED_DiagnoseConnection) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| TEST_F(CuObjTokenManagerTest, DiagnoseConnection) { | |
| std::cout << "=== CuObjTokenManager connection diagnostics ===" << std::endl; | |
| #ifdef HAVE_CUDA | |
| std::cout << " HAVE_CUDA: yes" << std::endl; | |
| // GPU info | |
| int dev_count = 0; | |
| cudaGetDeviceCount(&dev_count); | |
| std::cout << " CUDA devices: " << dev_count << std::endl; | |
| for (int i = 0; i < dev_count; ++i) { | |
| cudaDeviceProp prop; | |
| cudaGetDeviceProperties(&prop, i); | |
| std::cout << " [" << i << "] " << prop.name | |
| << " (compute " << prop.major << "." << prop.minor << ")" << std::endl; | |
| } | |
| #else | |
| std::cout << " HAVE_CUDA: no" << std::endl; | |
| #endif | |
| std::cout << " useCuda_: " << (useCuda_ ? "true" : "false") << std::endl; | |
| std::cout << " buffer_: " << buffer_ << std::endl; | |
| CuObjTokenManager mgr; | |
| std::cout << " isConnected(): " << (mgr.isConnected() ? "true" : "false") << std::endl; | |
| if (!mgr.isConnected()) { | |
| std::cout << "=== end diagnostics (not connected) ===" << std::endl; | |
| return; | |
| } | |
| // Try different memory types and sizes | |
| struct MemTest { | |
| const char *label; | |
| void *ptr; | |
| size_t size; | |
| std::function<void()> cleanup; | |
| }; | |
| std::vector<MemTest> tests; | |
| // Test 1: fixture buffer_ (cudaMalloc or posix_memalign) with full size | |
| tests.push_back({"buffer_ (full 64K)", buffer_, kBufferSize, nullptr}); | |
| // Test 2: fixture buffer_ with 4K | |
| tests.push_back({"buffer_ (4K)", buffer_, 4096, nullptr}); | |
| #ifdef HAVE_CUDA | |
| // Test 3: fresh cudaMalloc 64K | |
| void *gpu_buf = nullptr; | |
| if (cudaMalloc(&gpu_buf, kBufferSize) == cudaSuccess) { | |
| tests.push_back({"cudaMalloc 64K", gpu_buf, kBufferSize, | |
| [gpu_buf]() { cudaFree(gpu_buf); }}); | |
| } | |
| // Test 4: cudaMallocHost (pinned host) 64K | |
| void *pinned_buf = nullptr; | |
| if (cudaMallocHost(&pinned_buf, kBufferSize) == cudaSuccess) { | |
| tests.push_back({"cudaMallocHost 64K", pinned_buf, kBufferSize, | |
| [pinned_buf]() { cudaFreeHost(pinned_buf); }}); | |
| } | |
| // Test 5: cudaMallocManaged (unified) 64K | |
| void *managed_buf = nullptr; | |
| if (cudaMallocManaged(&managed_buf, kBufferSize) == cudaSuccess) { | |
| tests.push_back({"cudaMallocManaged 64K", managed_buf, kBufferSize, | |
| [managed_buf]() { cudaFree(managed_buf); }}); | |
| } | |
| #endif | |
| // Test 6: posix_memalign 64K | |
| void *sys_buf = nullptr; | |
| posix_memalign(&sys_buf, 4096, kBufferSize); | |
| if (sys_buf) { | |
| tests.push_back({"posix_memalign 64K", sys_buf, kBufferSize, | |
| [sys_buf]() { free(sys_buf); }}); | |
| } | |
| for (auto &t : tests) { | |
| cuObjErr_t rc = mgr.registerMemory(t.ptr, t.size); | |
| std::cout << " registerMemory(" << t.label << ", ptr=" << t.ptr | |
| << ", size=" << t.size << ") = " << rc << std::endl; | |
| if (rc == CU_OBJ_SUCCESS) { | |
| mgr.deregisterMemory(t.ptr); | |
| } | |
| } | |
| // Cleanup | |
| for (auto &t : tests) { | |
| if (t.cleanup) t.cleanup(); | |
| } | |
| std::cout << "=== end diagnostics ===" << std::endl; | |
| } | |
| TEST_F(CuObjTokenManagerTest, DISABLED_DiagnoseConnection) { | |
| std::cout << "=== CuObjTokenManager connection diagnostics ===" << std::endl; | |
| `#ifdef` HAVE_CUDA | |
| std::cout << " HAVE_CUDA: yes" << std::endl; | |
| // GPU info | |
| int dev_count = 0; | |
| cudaGetDeviceCount(&dev_count); | |
| std::cout << " CUDA devices: " << dev_count << std::endl; | |
| for (int i = 0; i < dev_count; ++i) { | |
| cudaDeviceProp prop; | |
| cudaGetDeviceProperties(&prop, i); | |
| std::cout << " [" << i << "] " << prop.name | |
| << " (compute " << prop.major << "." << prop.minor << ")" << std::endl; | |
| } | |
| `#else` | |
| std::cout << " HAVE_CUDA: no" << std::endl; | |
| `#endif` | |
| std::cout << " useCuda_: " << (useCuda_ ? "true" : "false") << std::endl; | |
| std::cout << " buffer_: " << buffer_ << std::endl; | |
| CuObjTokenManager mgr; | |
| std::cout << " isConnected(): " << (mgr.isConnected() ? "true" : "false") << std::endl; | |
| if (!mgr.isConnected()) { | |
| std::cout << "=== end diagnostics (not connected) ===" << std::endl; | |
| return; | |
| } | |
| // Try different memory types and sizes | |
| struct MemTest { | |
| const char *label; | |
| void *ptr; | |
| size_t size; | |
| std::function<void()> cleanup; | |
| }; | |
| std::vector<MemTest> tests; | |
| // Test 1: fixture buffer_ (cudaMalloc or posix_memalign) with full size | |
| tests.push_back({"buffer_ (full 64K)", buffer_, kBufferSize, nullptr}); | |
| // Test 2: fixture buffer_ with 4K | |
| tests.push_back({"buffer_ (4K)", buffer_, 4096, nullptr}); | |
| `#ifdef` HAVE_CUDA | |
| // Test 3: fresh cudaMalloc 64K | |
| void *gpu_buf = nullptr; | |
| if (cudaMalloc(&gpu_buf, kBufferSize) == cudaSuccess) { | |
| tests.push_back({"cudaMalloc 64K", gpu_buf, kBufferSize, | |
| [gpu_buf]() { cudaFree(gpu_buf); }}); | |
| } | |
| // Test 4: cudaMallocHost (pinned host) 64K | |
| void *pinned_buf = nullptr; | |
| if (cudaMallocHost(&pinned_buf, kBufferSize) == cudaSuccess) { | |
| tests.push_back({"cudaMallocHost 64K", pinned_buf, kBufferSize, | |
| [pinned_buf]() { cudaFreeHost(pinned_buf); }}); | |
| } | |
| // Test 5: cudaMallocManaged (unified) 64K | |
| void *managed_buf = nullptr; | |
| if (cudaMallocManaged(&managed_buf, kBufferSize) == cudaSuccess) { | |
| tests.push_back({"cudaMallocManaged 64K", managed_buf, kBufferSize, | |
| [managed_buf]() { cudaFree(managed_buf); }}); | |
| } | |
| `#endif` | |
| // Test 6: posix_memalign 64K | |
| void *sys_buf = nullptr; | |
| posix_memalign(&sys_buf, 4096, kBufferSize); | |
| if (sys_buf) { | |
| tests.push_back({"posix_memalign 64K", sys_buf, kBufferSize, | |
| [sys_buf]() { free(sys_buf); }}); | |
| } | |
| for (auto &t : tests) { | |
| cuObjErr_t rc = mgr.registerMemory(t.ptr, t.size); | |
| std::cout << " registerMemory(" << t.label << ", ptr=" << t.ptr | |
| << ", size=" << t.size << ") = " << rc << std::endl; | |
| if (rc == CU_OBJ_SUCCESS) { | |
| mgr.deregisterMemory(t.ptr); | |
| } | |
| } | |
| // Cleanup | |
| for (auto &t : tests) { | |
| if (t.cleanup) t.cleanup(); | |
| } | |
| std::cout << "=== end diagnostics ===" << std::endl; | |
| } |
🧰 Tools
🪛 GitHub Actions: Clang Format Check / clang-format
[error] 737-737: clang-format check failed (diff found). The std::cout line formatting and line breaks do not match clang-format style.
[error] 761-811: clang-format check failed (diff found). Multiple formatting discrepancies in initializer/list formatting, lambda expressions, and stream line breaks.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/gtest/unit/obj/obj_cuobj.cpp` around lines 727 - 818, The test
DiagnoseConnection in the TEST_F CuObjTokenManagerTest should be made opt-in for
local debugging; rename or move it so it won't run by default (for example
prefix the test name with DISABLED_ to become DISABLED_DiagnoseConnection) or
wrap its body behind a debug-only helper so normal CI won't execute it; update
any references to the test name accordingly and ensure the fixture symbols
(CuObjTokenManagerTest, DiagnoseConnection, mgr.isConnected()) remain unchanged
except for the test name change.
What?
Replace the cuObject callback-based I/O interface (iDellS3RdmaClient, Pattern A) with manual RDMA token management (CuObjTokenManager, Pattern B) in the Dell ObjectScale engine.
Why?
The current implementation abuses cuObjPut()/cuObjGet() callbacks: callbacks are expected to perform the entire server round-trip synchronously, but ours copy rdma_desc into a context and return 0, with the actual S3 request happening later in postXfer. cuMemObjGetRDMAToken() is designed for exactly this use case -- generate a token, inject it into
your own HTTP request, free it when done. No callbacks, no synchronous I/O pretense.
How?
• New CuObjTokenManager: RAII wrapper around cuObjClient. 1:1 memory registration mapping (each registerMemory = one cuMemObjGetDescriptor). Token generation via cuMemObjGetRDMAToken with buffer_offset=0.
• Rewritten awsS3DellObsClient: Overrides standard putObjectAsync/getObjectAsync to inject RDMA tokens as x-rdma-info headers. Implements standard iS3Client -- no separate RDMA interface, no dynamic_cast.
• Simplified S3DellObsObjEngineImpl: Overrides only registerMem, deregisterMem, getSupportedMems, getClient, and postXfer (offset validation). Inherits prepXfer, checkXfer, releaseReqH from parent.
• Deleted rdma_interface.h (iDellS3RdmaClient no longer needed).
• Extended isValidPrepXferParams to accept VRAM_SEG in addition to DRAM_SEG.
• Consolidated test infrastructure: single mockS3Client for all engines, parameterized Accel/Dell test suites, comprehensive CuObjTokenManager integration tests.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor