Skip to content

Md/s2#1938

Draft
aschwartz12 wants to merge 2 commits into
ai-dynamo:mainfrom
aschwartz12:md/s2
Draft

Md/s2#1938
aschwartz12 wants to merge 2 commits into
ai-dynamo:mainfrom
aschwartz12:md/s2

Conversation

@aschwartz12

Copy link
Copy Markdown

PR2 - refactor(metadata): manager cutover and ETCD backend

Branch md/s2 (8cb65310), stacked on PR1 (md/s1). Second slice of the
4-PR metadata-manager stack. This is the irreducible core: it wires the manager,
adds the ETCD backend, flips nixlAgent to the manager as the single metadata
path, and deletes nixl_listener.cpp.

After this commit, all P2P and ETCD metadata exchange runs through
nixlMDManager and its backends. The public nixlAgent metadata API is
unchanged.

What?

Adds the manager and ETCD backend, rewires the agent, and removes the old path:

  • nixl_md_manager.{cpp,h} — owns the worker thread, routes calls to backends
  • nixl_etcd_metadata_backend.{cpp,h} — self-contained ETCD client and watch loop
  • nixl_agent.cpp — exchange methods become thin manager wrappers; inline
    socket/etcd/comm-thread code removed
  • agent_data.h — implements nixlMetadataContext, drops comm-thread members,
    owns nixlMDManager
  • nixl_listener.cppdeleted (-820 lines)

PR1's P2P backend (from md/s1) is now wired through the manager.

Why?

Manager, ETCD backend, and nixl_listener.cpp deletion are symbol-coupled:
nixl_listener.cpp defines nixlEtcdClient and default_metadata_label, which
the ETCD backend and manager also define. They cannot land in separate PRs without
duplicate-symbol link errors or a risky two-worker hybrid state.

This commit is the behavioral cutover. Reviewers should focus on: does the new
path preserve P2P and ETCD semantics?

How?

Manager - nixlMDManager is owned by nixlAgentData and is the single path for
all metadata exchange. It owns one worker thread and routes each call by
precedence: a peer address selects P2P, otherwise the configured ETCD store
backend.

Backends - P2P (from PR1) and ETCD (new) implement nixlMetadataBackend. Each
is self-contained; serviceEvents() drives watch/accept/read loops on the
worker thread, gated by needsWorker().

Cutover - nixl_listener.cpp is deleted. Its comm-worker loop moves into the
manager's worker; socket helpers already live in the P2P backend (PR1); the
nixlEtcdClient moves into the ETCD backend.

Agent - nixlAgentData implements nixlMetadataContext for the backends.
Pure-cache operations (getLocalMD, loadRemoteMD, etc.) consolidate on
nixlAgentData. Exchange methods delegate to the manager.

Scope - No public API change. P2P and ETCD behavior preserved. TCPStore lands
in PR4. Gtests for the manager land in PR3.

Size

~1179 lines added, ~1056 deleted across 9 files. Slightly over the 1000-line
added target; the deletions (mostly nixl_listener.cpp) review quickly.

Test plan

  • Builds in the no-ETCD config (builddir) and the ETCD config (builddir-etcd).
  • Existing MetadataExchangeTestFixture (pure-cache + socket) still passes.
  • Manager-specific gtests land in PR3; run them after merging PR3 or by
    checking out md/s3.

Stack

  • Base: PR1 (md/s1)
  • Next: PR3 (md/s3) adds manager gtests; PR4 (md/s4) adds TCPStore

Introduce the metadata-exchange backend layer without wiring it in yet:
the nixlMetadataBackend contract, the nixlMetadataContext interface (the
agent-side operations a backend needs), and the self-contained P2P
(socket) backend that implements the contract against the context.

Nothing constructs these yet; the agent still uses its inline path. The
manager that owns and dispatches to backends, and the cutover that
removes the inline path, come in the following commit. Splitting the
introduction out keeps that cutover reviewable on its own.
Wire the backend layer in and remove the inline path. nixlAgentData now
owns a nixlMDManager (built unconditionally) that owns the worker thread
and dispatches each call to a backend by precedence: a peer address
selects P2P, otherwise the configured centralized store (ETCD). Add the
self-contained ETCD backend (owns its nixlEtcdClient).

nixlAgent's exchange methods (sendLocalMD, sendLocalPartialMD,
fetchRemoteMD, invalidateLocalMD) become thin wrappers over the manager;
pure-cache operations are consolidated on nixlAgentData, which implements
nixlMetadataContext for the backends.

nixl_listener.cpp is deleted: its worker loop moves into the manager, its
socket helpers into the P2P backend, and its etcd client into the ETCD
backend. Public API and observable P2P/ETCD behavior are unchanged;
gtest log expectations are updated to the new backend messages.
@copy-pr-bot

copy-pr-bot Bot commented Jul 14, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@github-actions

Copy link
Copy Markdown

👋 Hi aschwartz12! Thank you for contributing to ai-dynamo/nixl.

Your PR reviewers will review your contribution then trigger the CI to test your changes.

🚀

@svc-nixl

Copy link
Copy Markdown
Collaborator

🤖 CI Triage AgentClang Format Check · commit 8cb65310

TL;DR: The Clang Format Check failed because src/core/nixl_agent.cpp has two function signatures that don't match the project's clang-format style; fix by running clang-format-19 -i (or the repo's format script) on that file and committing the result.

Full analysis

Summary: GitHub Actions clang-format job exited with code 1 because clang-format-19 reported formatting diffs in src/core/nixl_agent.cpp.

Root cause: Two changed lines in nixl_agent.cpp violate the project's .clang-format style:

  • Line ~1826: nixlAgent::sendLocalMD (const nixl_opt_args_t* extra_params) const { — has a stray space before ( and uses type* var pointer placement, but the style requires no space before ( and left-aligned pointer (type *var): nixlAgent::sendLocalMD(const nixl_opt_args_t *extra_params) const {
  • Line ~1834: const nixl_opt_args_t* extra_params) const { — same pointer-alignment issue; should be const nixl_opt_args_t *extra_params) const {

This is a genuine lint failure, not a hang or timeout — the whole job ran in ~14 seconds with continuous output.

Implicated commit: [REDACTED:Hex High Entropy String] (PR #1938, branch md/s2) — the commit under test that introduced these lines.

File: src/core/nixl_agent.cpp:1826 and src/core/nixl_agent.cpp:1834

Suggested fix: Run clang-format on the affected file and commit the result:

clang-format-19 -i -style=file src/core/nixl_agent.cpp

Specifically: remove the space before ( in sendLocalMD ( and change both const nixl_opt_args_t* extra_params to const nixl_opt_args_t *extra_params. (Consider a pre-commit hook / make format to catch this locally before pushing.)

Related: PR #1938 (the PR whose merge commit be80855 was tested).

🛡️ This comment had 1 potential secret(s) redacted (Hex High Entropy String). See request_id 7a1bfa8f-acad-496b-840e-7fe45d281b7e in the triage console for the audit trail.

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.

2 participants