plugins: add spdk_kv interface headers for NVMe-KV backends#1885
Open
gaowayne wants to merge 9 commits into
Open
plugins: add spdk_kv interface headers for NVMe-KV backends#1885gaowayne wants to merge 9 commits into
gaowayne wants to merge 9 commits into
Conversation
|
👋 Hi gaowayne! Thank you for contributing to ai-dynamo/nixl. Your PR reviewers will review your contribution then trigger the CI to test your changes. 🚀 |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds SPDK KV interface headers for device and engine contracts, a compile-time contract test with build wiring, and README documentation describing the interface-only plugin structure and usage expectations. ChangesSPDK KV Interface Scaffolding
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@docs/superpowers/plans/2026-07-04-spdk-kv-high-findings.md`:
- Around line 13-129: The Markdown heading levels in this plan skip a level,
causing lint failures and hurting readability. Update the task headings under
this document to use consistent `##`-level headings for each task section, and
keep the existing substeps beneath them unchanged so the hierarchy is lint-clean
and easy to scan.
In `@src/plugins/spdk_kv/ispdk_kv_device.h`:
- Around line 129-137: The maxKeyLen() contract is wider than the key_len type
used by store, retrieve, and exist, so the interface can advertise values the
API cannot represent. Update ISPDKKVDevice::maxKeyLen() in ispdk_kv_device.h to
use a self-consistent type such as uint8_t, or otherwise document and enforce
caller-side clamping within the same interface contract; make sure any
implementations and callers that reference maxKeyLen(), key_len, or the SPDK KV
device metadata follow the same range.
In `@src/plugins/spdk_kv/spdk_kv_engine.h`:
- Around line 113-115: The queryMem declaration in spdk_kv_engine.h is formatted
in a way that violates clang-format, so update the signature to match the
project’s formatting style. Run clang-format on the file and commit the
resulting change, making sure the queryMem method declaration in the SPDK KV
engine header is wrapped exactly as clang-format expects.
- Around line 34-49: The rationale comment for nixlSpdkKvEngine is incomplete
because it only lists a subset of the overridden methods. Update the doc block
in spdk_kv_engine.h to include the additional declared overrides in
nixlSpdkKvEngine, especially supportsRemote, supportsLocal, supportsNotif,
connect, disconnect, unloadMD, and loadLocalMD, so the base-class design
description stays aligned with the actual interface.
🪄 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: Enterprise
Run ID: c3301522-4845-4e9b-af18-e5bcf3083711
📒 Files selected for processing (8)
docs/superpowers/plans/2026-07-04-spdk-kv-high-findings.mddocs/superpowers/specs/2026-07-04-spdk-kv-high-findings-design.mdsrc/plugins/spdk_kv/README.mdsrc/plugins/spdk_kv/ispdk_kv_device.hsrc/plugins/spdk_kv/spdk_kv_engine.htest/gtest/unit/meson.buildtest/gtest/unit/spdk_kv/interface_contract.cpptest/gtest/unit/spdk_kv/meson.build
Comment on lines
+13
to
+129
| ### Task 1: Add a compile-time interface contract test | ||
|
|
||
| **Files:** | ||
| - Create: `test/gtest/unit/spdk_kv/interface_contract.cpp` | ||
| - Create: `test/gtest/unit/spdk_kv/meson.build` | ||
| - Modify: `test/gtest/unit/meson.build` | ||
|
|
||
| - [ ] **Step 1: Write the failing contract test** | ||
|
|
||
| Create a concrete inspection subclass that publicly inherits the protected constructors, implements the two customization hooks, and defines a `const` method that locks `deviceMutex_`. Add static assertions requiring both constructor signatures and `size_t` value-operation signatures. | ||
|
|
||
| ```cpp | ||
| using MaxValueLenSignature = size_t (iSpdkKvDevice::*)() const; | ||
| using StoreSignature = | ||
| SpdkKvStatus (iSpdkKvDevice::*)(const void *, uint8_t, const void *, size_t); | ||
| using RetrieveSignature = SpdkKvStatus (iSpdkKvDevice::*)( | ||
| const void *, uint8_t, void *, size_t, size_t *); | ||
|
|
||
| static_assert(std::is_same_v<decltype(&iSpdkKvDevice::maxValueLen), MaxValueLenSignature>); | ||
| static_assert(std::is_same_v<decltype(&iSpdkKvDevice::store), StoreSignature>); | ||
| static_assert(std::is_same_v<decltype(&iSpdkKvDevice::retrieve), RetrieveSignature>); | ||
| static_assert(std::is_constructible_v<InspectableSpdkKvEngine, | ||
| const nixlBackendInitParams *>); | ||
| static_assert(std::is_constructible_v<InspectableSpdkKvEngine, | ||
| const nixlBackendInitParams *, | ||
| std::unique_ptr<iSpdkKvDevice>>); | ||
| ``` | ||
|
|
||
| - [ ] **Step 2: Run the direct compile and verify RED** | ||
|
|
||
| Run: | ||
|
|
||
| ```bash | ||
| c++ -std=c++20 -fsyntax-only -Isrc/api/cpp -Isrc/core -Isrc/core/telemetry \ | ||
| -Isrc/infra -Isrc/utils -Isrc -Isubprojects/abseil-cpp-20250814.1 \ | ||
| -Isrc/plugins/spdk_kv test/gtest/unit/spdk_kv/interface_contract.cpp | ||
| ``` | ||
|
|
||
| Expected: compilation fails because value signatures still use `uint32_t`, the injecting constructor is absent, and `deviceMutex_` is absent. | ||
|
|
||
| - [ ] **Step 3: Wire the source into the unit-test aggregate** | ||
|
|
||
| Create `spdk_kv_interface_unit_test_dep` with `interface_contract.cpp` as its source and add it to `unit_test_deps` unconditionally from `test/gtest/unit/meson.build`. | ||
|
|
||
| ### Task 2: Harden the core headers | ||
|
|
||
| **Files:** | ||
| - Modify: `src/plugins/spdk_kv/ispdk_kv_device.h` | ||
| - Modify: `src/plugins/spdk_kv/spdk_kv_engine.h` | ||
| - Test: `test/gtest/unit/spdk_kv/interface_contract.cpp` | ||
|
|
||
| - [ ] **Step 1: Make abstract value lengths size-safe** | ||
|
|
||
| Change `maxValueLen()` to return `size_t`; change Store and Retrieve value lengths and `value_len_out` to `size_t`. Document that adapters for narrower native APIs must range-check before narrowing. | ||
|
|
||
| - [ ] **Step 2: Add the injecting constructor and shared initialization declaration** | ||
|
|
||
| Keep the existing constructor and add: | ||
|
|
||
| ```cpp | ||
| nixlSpdkKvEngine(const nixlBackendInitParams *init_params, | ||
| std::unique_ptr<iSpdkKvDevice> device); | ||
| ``` | ||
|
|
||
| Add a private `setDevice(std::unique_ptr<iSpdkKvDevice>)` declaration used by both constructor initialization and `initDevice()` implementations. | ||
|
|
||
| - [ ] **Step 3: Add the serialization primitive** | ||
|
|
||
| Add `<mutex>` and: | ||
|
|
||
| ```cpp | ||
| mutable std::mutex deviceMutex_; | ||
| ``` | ||
|
|
||
| Document that the shared implementation holds it across a complete allocation/staging/device-operation/free transaction. | ||
|
|
||
| - [ ] **Step 4: Run the direct compile and verify GREEN** | ||
|
|
||
| Run the command from Task 1 Step 2. Expected: exit 0. | ||
|
|
||
| ### Task 3: Update user-facing documentation | ||
|
|
||
| **Files:** | ||
| - Modify: `src/plugins/spdk_kv/README.md` | ||
| - Modify: `src/plugins/spdk_kv/ispdk_kv_device.h` | ||
| - Modify: `src/plugins/spdk_kv/spdk_kv_engine.h` | ||
|
|
||
| - [ ] **Step 1: Update API examples and construction guidance** | ||
|
|
||
| Show constructor injection as the preferred path, and show the existing constructor plus `initDevice()` as the compatibility path. Update example value signatures to `size_t`. | ||
|
|
||
| - [ ] **Step 2: Document size and concurrency guarantees** | ||
|
|
||
| State that native adapters validate before narrowing and that one engine serializes complete operations against its device. | ||
|
|
||
| - [ ] **Step 3: Remove contradictory DMA wording** | ||
|
|
||
| Describe the engine as independent of SPDK allocation details while still aware of an abstract transfer-capable buffer contract. | ||
|
|
||
| ### Task 4: Verify and commit | ||
|
|
||
| **Files:** | ||
| - Verify all modified files. | ||
|
|
||
| - [ ] **Step 1: Run contract and header compilation checks** | ||
|
|
||
| Run the Task 1 direct compile command and standalone compilation for both headers. Expected: exit 0. | ||
|
|
||
| - [ ] **Step 2: Run repository checks available in the workspace** | ||
|
|
||
| Run `git diff --check`. If the configured Meson unit build is usable, build its unit target; otherwise report the concrete dependency/configuration blocker. | ||
|
|
||
| - [ ] **Step 3: Inspect scope** | ||
|
|
||
| Run `git diff --stat`, `git diff`, and `git status --short`; confirm unrelated untracked dependency files were not changed. | ||
|
|
||
| - [ ] **Step 4: Commit the implementation** |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the heading hierarchy.
markdownlint already flags the skipped level here; use ## for each task heading so the plan stays lint-clean and easier to scan.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 13-13: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
🤖 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 `@docs/superpowers/plans/2026-07-04-spdk-kv-high-findings.md` around lines 13 -
129, The Markdown heading levels in this plan skip a level, causing lint
failures and hurting readability. Update the task headings under this document
to use consistent `##`-level headings for each task section, and keep the
existing substeps beneath them unchanged so the hierarchy is lint-clean and easy
to scan.
Source: Linters/SAST tools
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/gtest/unit/spdk_kv/interface_contract.cpp (1)
29-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer const-qualified
lock_guard.✏️ Proposed fix
void lockDeviceFromConstMethod() const { - std::lock_guard<std::mutex> lock(deviceMutex_); + const std::lock_guard<std::mutex> lock(deviceMutex_); }Based on learnings, "Prefer using const qualifiers on local variables in C++ code where possible to improve const-correctness... use const std::lock_guardstd::mutex lock(mutex_)."
🤖 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/spdk_kv/interface_contract.cpp` around lines 29 - 32, The local mutex guard in lockDeviceFromConstMethod should be const-qualified to match the requested const-correctness style. Update the lock_guard declaration inside lockDeviceFromConstMethod to use a const std::lock_guard<std::mutex>, keeping the same deviceMutex_ usage and preserving the existing locking behavior.Source: Learnings
src/plugins/spdk_kv/spdk_kv_engine.h (1)
200-200: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConstant naming deviates from CodeStyle.md.
kMaxKeyLenuses a 'k'-prefixed camelCase style, but the header path instructions require snake_case for constants (e.g.,max_key_lenorkMaxKeyLen→MAX_KEY_LEN/max_key_len, per repo convention). Since this is a new symbol (not an existing established member), it's worth aligning before it propagates to consumers.✏️ Proposed fix
- static constexpr uint8_t kMaxKeyLen = 16; + static constexpr uint8_t max_key_len = 16;(and update all references accordingly)
As per path instructions, "use lower-camel for types/funcs and snake_case for variables/locals/constants."
🤖 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 `@src/plugins/spdk_kv/spdk_kv_engine.h` at line 200, The new constant name in spdk_kv_engine.h does not match the repo’s snake_case style for constants. Rename kMaxKeyLen to a snake_case constant name such as max_key_len, and update every reference to it in the spdk_kv engine code so the symbol remains consistent across consumers.Source: Path instructions
🤖 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/spdk_kv/interface_contract.cpp`:
- Around line 41-44: The function-pointer type aliases in the interface contract
test use PascalCase and may violate the repo’s `TypeAlias` naming convention.
Review the `MaxValueLenSignature`, `StoreSignature`, and `RetrieveSignature`
aliases in `interface_contract` and rename them to the project’s expected
`lower_case` form with a `_t` suffix if this test target is covered by the same
clang-tidy rule. Verify the aliases still match the `iSpdkKvDevice` member
function signatures after renaming.
---
Outside diff comments:
In `@src/plugins/spdk_kv/spdk_kv_engine.h`:
- Line 200: The new constant name in spdk_kv_engine.h does not match the repo’s
snake_case style for constants. Rename kMaxKeyLen to a snake_case constant name
such as max_key_len, and update every reference to it in the spdk_kv engine code
so the symbol remains consistent across consumers.
In `@test/gtest/unit/spdk_kv/interface_contract.cpp`:
- Around line 29-32: The local mutex guard in lockDeviceFromConstMethod should
be const-qualified to match the requested const-correctness style. Update the
lock_guard declaration inside lockDeviceFromConstMethod to use a const
std::lock_guard<std::mutex>, keeping the same deviceMutex_ usage and preserving
the existing locking behavior.
🪄 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: Enterprise
Run ID: c0d8687b-e6a0-4f07-8f99-d994a93aeeef
📒 Files selected for processing (2)
src/plugins/spdk_kv/spdk_kv_engine.htest/gtest/unit/spdk_kv/interface_contract.cpp
hxieustc
force-pushed
the
pr_nixl_spdk_kv_plugin_interface
branch
from
a7d68d2 to
d110c18
Compare
Contributor
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/spdk_kv/ispdk_kv_device.h`:
- Around line 100-113: The enum type name SpdkKvStatus appears to violate the
repository’s enum-case naming convention, so verify whether this header is
intentionally exempt; if not, rename the enum to match the repo standard and
update all usages accordingly. Also revise the nearby comments in
ispdk_kv_device.h and any references to the type in the SPDK KV device code so
the docs and identifiers stay consistent after the rename.
🪄 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: Enterprise
Run ID: fd587081-82ba-4be7-9c8a-a78111671762
📒 Files selected for processing (8)
docs/superpowers/plans/2026-07-04-spdk-kv-high-findings.mddocs/superpowers/specs/2026-07-04-spdk-kv-high-findings-design.mdsrc/plugins/spdk_kv/README.mdsrc/plugins/spdk_kv/ispdk_kv_device.hsrc/plugins/spdk_kv/spdk_kv_engine.htest/gtest/unit/meson.buildtest/gtest/unit/spdk_kv/interface_contract.cpptest/gtest/unit/spdk_kv/meson.build
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/plugins/spdk_kv/ispdk_kv_device.h (1)
129-130: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
maxKeyLen()return type still wider thankey_lenused elsewhere.Still returns
uint32_twhilestore/retrieve/existtakeuint8_t key_len, so a device that legitimately advertises >255 max key length can't have that value represented by the actual operation parameters. This was previously flagged and doesn't appear addressed in this revision.🛡️ Proposed fix
- virtual uint32_t + virtual uint8_t maxKeyLen() const = 0;🤖 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 `@src/plugins/spdk_kv/ispdk_kv_device.h` around lines 129 - 130, The maxKeyLen() contract in ISPDKKVDevice is still inconsistent with the key_len parameters used by store, retrieve, and exist, which are limited to uint8_t. Update the interface and any implementations/call sites so the advertised maximum key length is representable by the operation signatures, or widen the operation key_len type to match maxKeyLen() and keep all related methods aligned. Make sure the symbols maxKeyLen, store, retrieve, and exist all use the same key-length range end to end.
🤖 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/spdk_kv/interface_contract.cpp`:
- Around line 41-51: The compile-time contract in interface_contract.cpp only
validates maxValueLen, store, and retrieve, so signature drift in other
iSpdkKvDevice methods can slip through. Extend the existing static_assert
coverage by adding signature aliases and assertions for exist(), dmaAlloc(),
dmaFree(), and maxKeyLen() alongside the current checks, using the same pattern
as the existing max_value_len_signature_t/store_signature_t/retrieve_signature_t
validations.
---
Duplicate comments:
In `@src/plugins/spdk_kv/ispdk_kv_device.h`:
- Around line 129-130: The maxKeyLen() contract in ISPDKKVDevice is still
inconsistent with the key_len parameters used by store, retrieve, and exist,
which are limited to uint8_t. Update the interface and any implementations/call
sites so the advertised maximum key length is representable by the operation
signatures, or widen the operation key_len type to match maxKeyLen() and keep
all related methods aligned. Make sure the symbols maxKeyLen, store, retrieve,
and exist all use the same key-length range end to end.
🪄 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: Enterprise
Run ID: 2879597b-483c-43c7-8d80-9e6d94015b62
📒 Files selected for processing (4)
src/plugins/spdk_kv/README.mdsrc/plugins/spdk_kv/ispdk_kv_device.hsrc/plugins/spdk_kv/spdk_kv_engine.htest/gtest/unit/spdk_kv/interface_contract.cpp
Comment on lines
+41
to
+51
| using max_value_len_signature_t = size_t (iSpdkKvDevice::*)() const; | ||
| using store_signature_t = spdk_kv_status_t (iSpdkKvDevice::*)(const void *, | ||
| uint8_t, | ||
| const void *, | ||
| size_t); | ||
| using retrieve_signature_t = | ||
| spdk_kv_status_t (iSpdkKvDevice::*)(const void *, uint8_t, void *, size_t, size_t *); | ||
|
|
||
| static_assert(std::is_same_v<decltype(&iSpdkKvDevice::maxValueLen), max_value_len_signature_t>); | ||
| static_assert(std::is_same_v<decltype(&iSpdkKvDevice::store), store_signature_t>); | ||
| static_assert(std::is_same_v<decltype(&iSpdkKvDevice::retrieve), retrieve_signature_t>); |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Contract test doesn't cover all iSpdkKvDevice methods.
Only maxValueLen, store, and retrieve signatures are statically asserted. exist(), dmaAlloc()/dmaFree(), and maxKeyLen() aren't checked, so drift in those signatures wouldn't be caught by this compile-time contract.
♻️ Suggested addition
+using exist_signature_t = spdk_kv_status_t (iSpdkKvDevice::*)(const void *, uint8_t);
+using max_key_len_signature_t = uint32_t (iSpdkKvDevice::*)() const;
+
+static_assert(std::is_same_v<decltype(&iSpdkKvDevice::exist), exist_signature_t>);
+static_assert(std::is_same_v<decltype(&iSpdkKvDevice::maxKeyLen), max_key_len_signature_t>);📝 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.
Suggested change
| using max_value_len_signature_t = size_t (iSpdkKvDevice::*)() const; | |
| using store_signature_t = spdk_kv_status_t (iSpdkKvDevice::*)(const void *, | |
| uint8_t, | |
| const void *, | |
| size_t); | |
| using retrieve_signature_t = | |
| spdk_kv_status_t (iSpdkKvDevice::*)(const void *, uint8_t, void *, size_t, size_t *); | |
| static_assert(std::is_same_v<decltype(&iSpdkKvDevice::maxValueLen), max_value_len_signature_t>); | |
| static_assert(std::is_same_v<decltype(&iSpdkKvDevice::store), store_signature_t>); | |
| static_assert(std::is_same_v<decltype(&iSpdkKvDevice::retrieve), retrieve_signature_t>); | |
| using max_value_len_signature_t = size_t (iSpdkKvDevice::*)() const; | |
| using store_signature_t = spdk_kv_status_t (iSpdkKvDevice::*)(const void *, | |
| uint8_t, | |
| const void *, | |
| size_t); | |
| using retrieve_signature_t = | |
| spdk_kv_status_t (iSpdkKvDevice::*)(const void *, uint8_t, void *, size_t, size_t *); | |
| using exist_signature_t = spdk_kv_status_t (iSpdkKvDevice::*)(const void *, uint8_t); | |
| using max_key_len_signature_t = uint32_t (iSpdkKvDevice::*)() const; | |
| static_assert(std::is_same_v<decltype(&iSpdkKvDevice::maxValueLen), max_value_len_signature_t>); | |
| static_assert(std::is_same_v<decltype(&iSpdkKvDevice::store), store_signature_t>); | |
| static_assert(std::is_same_v<decltype(&iSpdkKvDevice::retrieve), retrieve_signature_t>); | |
| static_assert(std::is_same_v<decltype(&iSpdkKvDevice::exist), exist_signature_t>); | |
| static_assert(std::is_same_v<decltype(&iSpdkKvDevice::maxKeyLen), max_key_len_signature_t>); |
🤖 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/spdk_kv/interface_contract.cpp` around lines 41 - 51, The
compile-time contract in interface_contract.cpp only validates maxValueLen,
store, and retrieve, so signature drift in other iSpdkKvDevice methods can slip
through. Extend the existing static_assert coverage by adding signature aliases
and assertions for exist(), dmaAlloc(), dmaFree(), and maxKeyLen() alongside the
current checks, using the same pattern as the existing
max_value_len_signature_t/store_signature_t/retrieve_signature_t validations.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/gtest/unit/spdk_kv/interface_contract.cpp (1)
41-57: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winContract test still missing
exist(),dmaAlloc(),dmaFree()coverage.
maxKeyLen()was added per the prior review, butexist(),dmaAlloc(), anddmaFree()remain unchecked, so signature drift on those methods still wouldn't be caught at compile time.♻️ Suggested addition
using max_key_len_signature_t = uint8_t (iSpdkKvDevice::*)() const; using max_value_len_signature_t = size_t (iSpdkKvDevice::*)() const; +using dma_alloc_signature_t = void *(iSpdkKvDevice::*)(size_t); +using dma_free_signature_t = void (iSpdkKvDevice::*)(void *); +using exist_signature_t = spdk_kv_status_t (iSpdkKvDevice::*)(const void *, uint8_t); using store_signature_t = spdk_kv_status_t (iSpdkKvDevice::*)(const void *, uint8_t, const void *, size_t); using retrieve_signature_t = spdk_kv_status_t (iSpdkKvDevice::*)(const void *, uint8_t, void *, size_t, size_t *); static_assert(std::is_same_v<decltype(&iSpdkKvDevice::maxKeyLen), max_key_len_signature_t>); static_assert(std::is_same_v<decltype(&iSpdkKvDevice::maxValueLen), max_value_len_signature_t>); +static_assert(std::is_same_v<decltype(&iSpdkKvDevice::dmaAlloc), dma_alloc_signature_t>); +static_assert(std::is_same_v<decltype(&iSpdkKvDevice::dmaFree), dma_free_signature_t>); +static_assert(std::is_same_v<decltype(&iSpdkKvDevice::exist), exist_signature_t>); static_assert(std::is_same_v<decltype(&iSpdkKvDevice::store), store_signature_t>); static_assert(std::is_same_v<decltype(&iSpdkKvDevice::retrieve), retrieve_signature_t>);🤖 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/spdk_kv/interface_contract.cpp` around lines 41 - 57, The interface contract test in interface_contract.cpp is still missing compile-time checks for iSpdkKvDevice::exist, iSpdkKvDevice::dmaAlloc, and iSpdkKvDevice::dmaFree. Add static_asserts alongside the existing maxKeyLen/maxValueLen/store/retrieve signature checks, using distinct pointer-to-member signature aliases for those three methods so any future signature drift is caught at compile time.
🤖 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.
Duplicate comments:
In `@test/gtest/unit/spdk_kv/interface_contract.cpp`:
- Around line 41-57: The interface contract test in interface_contract.cpp is
still missing compile-time checks for iSpdkKvDevice::exist,
iSpdkKvDevice::dmaAlloc, and iSpdkKvDevice::dmaFree. Add static_asserts
alongside the existing maxKeyLen/maxValueLen/store/retrieve signature checks,
using distinct pointer-to-member signature aliases for those three methods so
any future signature drift is caught at compile time.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 91224bba-f711-4e7a-b31f-3e381bcb71f1
📒 Files selected for processing (3)
src/plugins/spdk_kv/README.mdsrc/plugins/spdk_kv/ispdk_kv_device.htest/gtest/unit/spdk_kv/interface_contract.cpp
Introduce the abstract device and engine contracts that RADOS_NKV and CSAL_NKV will inherit, so the shared NVMe-KV data plane is defined once without pulling in kv_host_shim or backend-specific code. Signed-off-by: Wayne Gao <wayne.gao1@solidigm.com>
hxieustc
force-pushed
the
pr_nixl_spdk_kv_plugin_interface
branch
from
e5a64eb to
449e8e9
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What?
This PR adds the shared, backend-agnostic interface layer for NIXL plugins that speak the generic NVMe Key-Value protocol over SPDK.
New files under
src/plugins/spdk_kv/:ispdk_kv_device.hiSpdkKvDevice— abstract device interface with semanticSpdkKvStatus(OK/NOT_FOUND/BUFFER_TOO_SMALL/ERROR)spdk_kv_engine.hnixlSpdkKvEngine— abstract NIXL backend base; declares the shared South-Bound API and two customization hooks (openDevice(),deriveKey())README.mdnixlKvHostShimDevice,nixlFakeKvDevice)kv_host_shim/ SPDK implementation codenixlRadosNkvEngine/nixlCsalNkvEngineconcrete backendsThose will land in follow-up backend PRs that inherit these interfaces.
Why?
RADOS_NKV and CSAL_NKV both use the same generic
kv_host_shimNVMe-KV protocol and the same data-plane algorithm:validate → derive key → DMA-stage → store / retrieve / exist
A shared abstract base is the simplest way to implement that algorithm once, while letting each backend customize only:
openDevice())deriveKey())Splitting the device into
iSpdkKvDevicefurther isolates transport/SPDK details from the engine, so:nixlFakeKvDevicecan unit-test the data plane without hardwarenixlRedisKVEngineremains standalone (inheritsnixlBackendEnginedirectly), consistent with the review feedback that Redis should not share a KV abstraction layer ahead of real reuse.How?
Class hierarchy
nixlBackendEngine
├── nixlSpdkKvEngine (abstract; shared NVMe-KV data plane)
│ ├── nixlRadosNkvEngine (openDevice: RADOS target) ← backend PR
│ └── nixlCsalNkvEngine (openDevice: CSAL target) ← backend PR
└── nixlRedisKVEngine (standalone; implemented directly)
iSpdkKvDevice (interface; used by nixlSpdkKvEngine)
├── nixlKvHostShimDevice (real: wraps kv_host_shim C API) ← backend PR
└── nixlFakeKvDevice (test double) ← backend PR
Responsibility split
iSpdkKvDevicecontractnixlKvHostShimDevice,nixlFakeKvDevicenixlSpdkKvEngineSB-API declarationsspdk_kv_engine.cpp(shared data-plane bodies)openDevice()/deriveKey()hooksFollow-up PRs (out of scope here)
nixlSpdkKvEngine, providesnixlKvHostShimDevice+nixlRadosNkvEngineopenDevice()/deriveKey()nixlFakeKvDevice+ data-plane unit tests (no SPDK)Test plan
pre-commit run --files src/plugins/spdk_kv/ispdk_kv_device.h src/plugins/spdk_kv/spdk_kv_engine.h src/plugins/spdk_kv/README.mdkv_host_shimdependency is introduced by this PRnixlSpdkKvEnginehook signatures and `iSSummary by CodeRabbit
Documentation
New Features
iSpdkKvDevice) with DMA buffer and key-value operations.nixlSpdkKvEngine) supporting deferred initialization and device injection.Tests