Skip to content

feat: add host PID broker server and device plugin integration - #2417

Open
iemAnshuman wants to merge 21 commits into
Project-HAMi:masterfrom
iemAnshuman:feat-hostpid-broker-server
Open

feat: add host PID broker server and device plugin integration#2417
iemAnshuman wants to merge 21 commits into
Project-HAMi:masterfrom
iemAnshuman:feat-hostpid-broker-server

Conversation

@iemAnshuman

@iemAnshuman iemAnshuman commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Related issue: #1662

Design discussion: #2244

Companion client PR: Project-HAMi/HAMi-core#251

Current companion client head: 4421b0970c172e343c7fb59491952b78ea9d86eb

What this changes

This adds an optional host PID broker to the NVIDIA device plugin. The broker obtains each connecting process's host PID from Linux SO_PEERCRED and returns it to the HAMi-core client.

The client never supplies a PID. The design does not expose or mount host procfs.

The feature is disabled by default. Enable it with:

devicePlugin:
  hostPID: true
  hostPIDBroker:
    enabled: true

The chart rejects configurations that enable the broker while devicePlugin.hostPID is false.

When enabled:

  1. The device plugin serves /var/run/hami/hostpid/broker.sock.

  2. Compatible workload allocations receive LIBVGPU_HOSTPID_BROKER=1.

  3. The broker directory is mounted read only at /tmp/vgpulock/hostpid.

  4. HAMi-core uses its NVML discovery path if the broker request fails and the trusted fallback mount is available.

Disabled configurations retain the current allocation response.

Before returning a non-MIG allocation, the device plugin prepares /tmp/vgpulock as a directory owned by root with mode 01777. Allocation fails if the parent cannot be prepared safely. This check rejects a symlink, the wrong object type, an unsafe owner or mode, and replacement during preparation.

The allocation response contains one writable parent mount at /tmp/vgpulock. When the broker is enabled, it also contains one read only mount at /tmp/vgpulock/hostpid. The plugin removes duplicate or path equivalent entries and places the parent before the nested broker mount. When the gate is disabled, it clears the reserved broker environment value and removes stale broker mounts.

Protocol and failure behavior

Protocol version 1 uses an 8 byte request and a 12 byte response. The request contains the magic, version, and command. The response adds the host PID obtained from the kernel.

The server requires root for its default path, protects its directory with a root owned lock file, rejects unsafe or active socket paths, limits active handlers to 512, and applies one 500 millisecond transaction deadline. Excess or incomplete connections are closed so the client can use its fallback.

A broker startup failure prevents the device plugin from serving allocations. An unexpected broker exit stops the device plugin so Kubernetes can restart it.

Validation

  1. Current server head 146ee027b8b0ac44c8f8b3d48581f2cf69a485e9 passes the GitHub Go analysis, CodeQL, compile, unit, chart lint, end to end, lint, packaging, and DCO checks. Focused tests cover disabled and enabled startup, listener and serve failures, shutdown, plugin cleanup, restart after a plugin start failure, parent ownership and mode checks, canonical mount ordering, stale configuration cleanup, and allocation failure handling.

  2. The production C client to Go server contract passed with 300 concurrent clients and no failures.

  3. Exact public heads 8d59bf9 and 935a6de completed all 16 balanced eight GPU cuInit() cells in job 181726. Mean API wall time changed from 56.373 to 22.501 seconds at N=128 and from 130.662 to 50.288 seconds at N=300. Every worker completed.

  4. Helm 3.8.1 lint and render checks pass for disabled and enabled configurations. The invalid hostPID: false configuration is rejected with the named prerequisite.

  5. Allocation response tests verify the exact environment value and the read only broker mount.

  6. The Linux root path hardening run completed 28 recorded checks with zero failures. Its archive SHA256 is 941d0a7c3de6e4602a8c57f3e71b8c21ad3acc60cef127d07599ef8ed6f7783a. The archive covers the server experiment at private revision 261567a97d2ad157094f07b8f0a007587d4481bd and the companion client through 607d7e83c282f8c494b39a0c2d92e16a83bdf965. It does not bind either current public head byte for byte.

  7. The package verifier reports 46 verified artifacts, 0 pending artifacts, and 0 failures. The package ledger SHA256 is 294637ae0abbe69115c9f900442029fa0ccb335ec08ad3e8982d17f8dea1448a.

The interim validation report records the source boundaries, A100 results, fallback result, Linux archive, and remaining Kubernetes checks.

Remaining work

Real kubelet allocation with the broker enabled and disabled, DaemonSet rollout and process restart, rollback, and degradation when the socket is unavailable remain untested on an isolated NVIDIA Kubernetes node. Additional container runtimes and other GPU and driver versions also remain untested.

Primary context testing shows that removing PID discovery does not remove the CUDA driver contention after cuInit().

Driver admission is not part of this public change. It remains a separate disabled follow up pending a maintainer decision.

User facing change

This adds devicePlugin.hostPIDBroker.enabled. Its default value is false.

AI assistance

I used AI assistance for implementation, tests, and evidence checks. I reviewed the code and results and can explain the design and failure cases.

Signed-off-by: iemAnshuman <asquare567@gmail.com>
Signed-off-by: iemAnshuman <asquare567@gmail.com>
Signed-off-by: iemAnshuman <asquare567@gmail.com>
Signed-off-by: iemAnshuman <asquare567@gmail.com>
Signed-off-by: iemAnshuman <asquare567@gmail.com>
@hami-robot

hami-robot Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: iemAnshuman
Once this PR has been reviewed and has the lgtm label, please assign archlitchi for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

The PR adds an optional host PID broker for the NVIDIA device plugin. It defines a Unix-socket protocol, implements Linux broker lifecycle and safety controls, integrates socket access into allocation responses, and adds Helm configuration, tests, and documentation.

Changes

Host PID broker

Layer / File(s) Summary
Protocol, configuration, and Helm wiring
pkg/device-plugin/nvidiadevice/nvinternal/hostpid/*, charts/hami/...
Defines broker configuration and protocol values. Adds Helm enablement, validation, environment variables, socket mounts, and the host directory.
Linux broker implementation and validation
pkg/device-plugin/nvidiadevice/nvinternal/hostpid/broker_*, go.mod
Adds trusted socket setup, PID responses, concurrency limits, cleanup, platform handling, and Linux tests.
Allocation response integration
pkg/device-plugin/nvidiadevice/nvinternal/plugin/...
Prepares the lock parent and adds or removes broker environment variables and mounts in allocation responses.
Device-plugin lifecycle and documentation
cmd/device-plugin/nvidia/..., docs/develop/hostpid-broker.md
Starts, monitors, and stops the broker. Tests lifecycle and failure handling. Documents configuration, protocol, security checks, rollout, rollback, and compatibility limits.

Estimated code review effort: 4 (Complex) | ~60 minutes

Possibly related issues

Possibly related PRs

Suggested reviewers: mesutoezdil, ouyangluwei163, chaunceyjiang

Poem

A rabbit guards the socket door,
Host PIDs pass through paths kept sure.
Helm mounts the broker, tests align,
Plugins start and stop in time.
Safe locks close when work is done.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
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.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely summarizes the primary change: adding a host PID broker and integrating it with the device plugin.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

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

Signed-off-by: iemAnshuman <asquare567@gmail.com>
Signed-off-by: iemAnshuman <asquare567@gmail.com>
@iemAnshuman
iemAnshuman force-pushed the feat-hostpid-broker-server branch from 3963c56 to a83c20f Compare August 6, 2026 19:31
@iemAnshuman
iemAnshuman marked this pull request as ready for review August 7, 2026 13:45
@hami-robot
hami-robot Bot requested a review from mesutoezdil August 7, 2026 13:45
@github-actions github-actions Bot added the kind/feature new function label Aug 7, 2026

@coderabbitai coderabbitai Bot 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.

Actionable comments posted: 5

🧹 Nitpick comments (2)
pkg/device-plugin/nvidiadevice/nvinternal/hostpid/broker_linux.go (1)

256-280: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider logging dropped transactions.

handle returns without a response when the read fails, when the deadline expires, or when peerPID fails. The broker emits no log for these paths. Field diagnosis of a failing client then depends only on HAMi-core falling back to NVML.

Add a rate-limited debug log for the read error and the peerPID error paths.

🤖 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 `@pkg/device-plugin/nvidiadevice/nvinternal/hostpid/broker_linux.go` around
lines 256 - 280, Add rate-limited debug logging in Broker.handle for errors
returned by io.ReadFull and peerPID, including the relevant error details, while
preserving the existing early-return behavior and response handling. Do not add
logging for unrelated paths.
pkg/device-plugin/nvidiadevice/nvinternal/hostpid/broker_linux_test.go (1)

576-586: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assert the recorded identity, not just the stat.

TestBrokerSocketIdentityUsesDeviceAndInode discards the broker at Line 577 and only checks that the stat cast succeeds and stat.Ino is non-zero. It never compares against broker.socket, so it does not cover readSocketIdentity. Compare the device and inode with the recorded values.

💚 Proposed test
-	_, socketPath := startTestBroker(t)
+	broker, socketPath := startTestBroker(t)
 	info, err := os.Lstat(socketPath)
 	if err != nil {
 		t.Fatal(err)
 	}
 	stat, ok := info.Sys().(*syscall.Stat_t)
 	if !ok || stat.Ino == 0 {
 		t.Fatalf("invalid socket stat: %#v", info.Sys())
 	}
+	if uint64(stat.Dev) != broker.socket.device ||
+		stat.Ino != broker.socket.inode {
+		t.Fatalf("recorded identity dev=%d ino=%d, want dev=%d ino=%d",
+			broker.socket.device, broker.socket.inode, stat.Dev, stat.Ino)
+	}
🤖 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 `@pkg/device-plugin/nvidiadevice/nvinternal/hostpid/broker_linux_test.go`
around lines 576 - 586, Update TestBrokerSocketIdentityUsesDeviceAndInode to
retain the broker returned by startTestBroker and compare broker.socket’s
recorded device and inode values with the device and inode from the Lstat
result, thereby exercising readSocketIdentity.
🤖 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 `@cmd/device-plugin/nvidia/main.go`:
- Around line 275-291: Update the startup flow around startHostPIDBroker and
p.Start so host PID broker health is validated before the device-plugin gRPC
server starts accepting requests. Await broker readiness and surface any broker
failure before invoking p.Start, or ensure a startup failure cancels/stops both
the broker and server; preserve hostPIDBrokerFailureReported and deferred
cleanup behavior for failures after startup.

In `@pkg/device-plugin/nvidiadevice/nvinternal/hostpid/broker_linux_test.go`:
- Around line 199-221: Update TestBrokerTimesOutPartialRequest to set a
client-side read deadline on connection immediately before reading into buffer.
Keep the existing assertion that the broker closes the connection, while
ensuring an open connection causes Read to return promptly and fail the test
rather than hang.

In `@pkg/device-plugin/nvidiadevice/nvinternal/hostpid/broker_linux.go`:
- Around line 225-238: Update Broker.Serve to retry temporary AcceptUnix errors
with a bounded backoff instead of immediately returning, while preserving the
existing clean return when broker.closing is set. Return only non-temporary
permanent errors, and reset the backoff after a successful connection is
accepted.

In `@pkg/device-plugin/nvidiadevice/nvinternal/plugin/hostpid_broker_test.go`:
- Around line 9-15: Reorder the imports in the hostpid broker test: keep the
standard-library testing import first, place testify and kubelet imports in the
external group next, and move the local
github.com/Project-HAMi/HAMi/pkg/device-plugin/.../hostpid import to the final
group.

In `@pkg/device-plugin/nvidiadevice/nvinternal/plugin/hostpid_broker.go`:
- Around line 9-14: Reorder the imports in hostpid_broker.go so standard-library
imports remain first, external k8s.io imports follow, and the local
github.com/Project-HAMi/HAMi/pkg/device-plugin/nvidiadevice/nvinternal/hostpid
import is placed last.

---

Nitpick comments:
In `@pkg/device-plugin/nvidiadevice/nvinternal/hostpid/broker_linux_test.go`:
- Around line 576-586: Update TestBrokerSocketIdentityUsesDeviceAndInode to
retain the broker returned by startTestBroker and compare broker.socket’s
recorded device and inode values with the device and inode from the Lstat
result, thereby exercising readSocketIdentity.

In `@pkg/device-plugin/nvidiadevice/nvinternal/hostpid/broker_linux.go`:
- Around line 256-280: Add rate-limited debug logging in Broker.handle for
errors returned by io.ReadFull and peerPID, including the relevant error
details, while preserving the existing early-return behavior and response
handling. Do not add logging for unrelated paths.
🪄 Autofix

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: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: a5588f6f-7e0b-4306-9854-aaeb57e348fd

📥 Commits

Reviewing files that changed from the base of the PR and between 87d9795 and a83c20f.

📒 Files selected for processing (18)
  • charts/hami/README.md
  • charts/hami/templates/device-plugin/daemonsetnvidia.yaml
  • charts/hami/values.yaml
  • cmd/device-plugin/nvidia/hostpid_broker.go
  • cmd/device-plugin/nvidia/hostpid_broker_test.go
  • cmd/device-plugin/nvidia/main.go
  • docs/develop/hostpid-broker.md
  • go.mod
  • pkg/device-plugin/nvidiadevice/nvinternal/hostpid/broker_linux.go
  • pkg/device-plugin/nvidiadevice/nvinternal/hostpid/broker_linux_test.go
  • pkg/device-plugin/nvidiadevice/nvinternal/hostpid/broker_unsupported.go
  • pkg/device-plugin/nvidiadevice/nvinternal/hostpid/config.go
  • pkg/device-plugin/nvidiadevice/nvinternal/hostpid/protocol.go
  • pkg/device-plugin/nvidiadevice/nvinternal/hostpid/protocol_test.go
  • pkg/device-plugin/nvidiadevice/nvinternal/plugin/hostpid_broker.go
  • pkg/device-plugin/nvidiadevice/nvinternal/plugin/hostpid_broker_test.go
  • pkg/device-plugin/nvidiadevice/nvinternal/plugin/server.go
  • pkg/device-plugin/nvidiadevice/nvinternal/plugin/server_test.go

Comment thread cmd/device-plugin/nvidia/main.go
Comment thread pkg/device-plugin/nvidiadevice/nvinternal/hostpid/broker_linux.go
Comment thread pkg/device-plugin/nvidiadevice/nvinternal/plugin/hostpid_broker.go
Signed-off-by: iemAnshuman <asquare567@gmail.com>
Signed-off-by: iemAnshuman <asquare567@gmail.com>
Signed-off-by: iemAnshuman <asquare567@gmail.com>
Signed-off-by: iemAnshuman <asquare567@gmail.com>
Signed-off-by: iemAnshuman <asquare567@gmail.com>
Signed-off-by: iemAnshuman <asquare567@gmail.com>
Signed-off-by: iemAnshuman <asquare567@gmail.com>
Signed-off-by: iemAnshuman <asquare567@gmail.com>
@mesutoezdil

Copy link
Copy Markdown
Contributor

/lgtm

@FouoF

FouoF commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

cc @maverick123123

@maverick-woo

maverick-woo commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

@iemAnshuman This is really well done — the broker design is clean, the security validations are thorough (root-owned socket, SO_PEERCRED, read-only mount, 500ms deadline), and the feature gate with hostPIDBroker.enabled defaulting to false keeps backward compatibility intact. Great work pulling together the full client-server stack from RFC to implementation.

A couple of requests for the remaining validation:

  1. Complete test report — once the real kubelet allocation and rollout/rollback tests are done, could you share a comprehensive report covering both the enabled and disabled paths, along with any performance numbers?

  2. Both paths must work — with LIBVGPU_HOSTPID_BROKER=1 enabled, and without it (no env var), everything should function normally. The NVML fallback path should have zero regression since this feature defaults to off. The broker path should degrade gracefully when the socket is unreachable.

Looking forward to seeing this land.

Prepare /tmp/vgpulock as a root owned sticky directory before allocation. Canonicalize the parent and broker mounts, keep the parent before the read only child, and remove stale broker settings when the gate is off.

Reject allocation when the parent cannot be prepared safely.

Signed-off-by: iemAnshuman <asquare567@gmail.com>
@hami-robot hami-robot Bot removed the lgtm label Aug 11, 2026
@hami-robot

hami-robot Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

New changes are detected. LGTM label has been removed.

@coderabbitai coderabbitai Bot 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.

Actionable comments posted: 2

🧹 Nitpick comments (3)
pkg/device-plugin/nvidiadevice/nvinternal/plugin/hostpid_broker.go (2)

40-48: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Use errors.Is for the EEXIST check.

createHostPIDLockParentWith accepts an injected mkdirat function. A wrapped EEXIST error then fails the direct comparison and the function reports a failure for an existing directory. errors.Is handles both raw and wrapped errno values.

♻️ Proposed refactor
 	err := mkdirat(parentFD, baseName, hostPIDLockParentCreateMode)
-	if err != nil && err != unix.EEXIST {
+	if err != nil && !errors.Is(err, unix.EEXIST) {
 		return err
 	}
 	return nil

Add "errors" to the standard-library import group.

🤖 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 `@pkg/device-plugin/nvidiadevice/nvinternal/plugin/hostpid_broker.go` around
lines 40 - 48, Update createHostPIDLockParentWith to use errors.Is when checking
whether mkdirat returned unix.EEXIST, and add the standard-library errors
import. Preserve returning other errors while treating raw or wrapped EEXIST as
success.

74-82: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Separate the directory-type check from the ownership check.

The condition combines three distinct failures. A non-directory or a failed Stat_t assertion produces the message "parent directory is not owned by trusted UID". The message then misleads the operator during triage of a security check failure.

♻️ Proposed refactor
 	parentStat, ok := parentInfo.Sys().(*syscall.Stat_t)
 	parentMode := parentInfo.Mode()
-	if !ok || !parentInfo.IsDir() || parentStat.Uid != trustedOwner {
+	if !ok || !parentInfo.IsDir() {
+		return fmt.Errorf("parent path is not a real directory")
+	}
+	if parentStat.Uid != trustedOwner {
 		return fmt.Errorf("parent directory is not owned by trusted UID %d",
 			trustedOwner)
 	}
🤖 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 `@pkg/device-plugin/nvidiadevice/nvinternal/plugin/hostpid_broker.go` around
lines 74 - 82, In the parent-directory validation flow, separate the `Stat_t`
assertion and `IsDir()` checks from the `parentStat.Uid != trustedOwner` check.
Return an error specific to an invalid or non-directory parent before evaluating
ownership, while preserving the existing trusted-UID error for ownership
failures and the subsequent permission checks.
pkg/device-plugin/nvidiadevice/nvinternal/plugin/server_test.go (1)

1095-1100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assert the allocation-failure bookkeeping on the error path.

Allocate calls the pod-allocation-failure hook before it returns the preparation error. The test stubs that hook with an empty function and never checks it. A regression that skips the node-lock release would still pass.

🧪 Proposed addition
+	failureCalls := 0
+	podAllocationFailed = func(string, *corev1.Pod, string) { failureCalls++ }
 	prepareHostPIDLockParentForAllocation = func() error {
 		return errors.New("parent preparation fixture")
 	}
 	failedResponse, err := plugin.Allocate(context.Background(), request)
 	require.Nil(t, failedResponse)
 	require.ErrorContains(t, err, "failed to prepare host PID lock parent")
+	require.Equal(t, 1, failureCalls)
🤖 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 `@pkg/device-plugin/nvidiadevice/nvinternal/plugin/server_test.go` around lines
1095 - 1100, Update the allocation failure test around plugin.Allocate to stub
the pod-allocation-failure hook with an observable signal, then assert that the
signal is triggered when prepareHostPIDLockParentForAllocation returns its
preparation error. Keep the existing failedResponse and error assertions, and
ensure the test specifically verifies the node-lock release bookkeeping before
returning.
🤖 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/develop/hostpid-broker.md`:
- Around line 48-50: Update the documentation’s compound modifiers to use
hyphens: change “descriptor based” to “descriptor-based,” “read only” to
“read-only,” and the corresponding “path equivalent” and “root owned” usages to
“path-equivalent” and “root-owned” in the affected sections.

In `@pkg/device-plugin/nvidiadevice/nvinternal/plugin/hostpid_broker_test.go`:
- Around line 107-113: Update the "owner" subtest in hostpid_broker_test.go to
assert the specific error returned by prepareHostPIDLockParent, or adjust the
fixture so the parent is trusted and only directory has an untrusted owner.
Ensure the test explicitly exercises and validates the intended directory
ownership-check branch rather than merely asserting any error.

---

Nitpick comments:
In `@pkg/device-plugin/nvidiadevice/nvinternal/plugin/hostpid_broker.go`:
- Around line 40-48: Update createHostPIDLockParentWith to use errors.Is when
checking whether mkdirat returned unix.EEXIST, and add the standard-library
errors import. Preserve returning other errors while treating raw or wrapped
EEXIST as success.
- Around line 74-82: In the parent-directory validation flow, separate the
`Stat_t` assertion and `IsDir()` checks from the `parentStat.Uid !=
trustedOwner` check. Return an error specific to an invalid or non-directory
parent before evaluating ownership, while preserving the existing trusted-UID
error for ownership failures and the subsequent permission checks.

In `@pkg/device-plugin/nvidiadevice/nvinternal/plugin/server_test.go`:
- Around line 1095-1100: Update the allocation failure test around
plugin.Allocate to stub the pod-allocation-failure hook with an observable
signal, then assert that the signal is triggered when
prepareHostPIDLockParentForAllocation returns its preparation error. Keep the
existing failedResponse and error assertions, and ensure the test specifically
verifies the node-lock release bookkeeping before returning.
🪄 Autofix

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: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 5190264b-5643-4065-9876-e7e493bfd6e9

📥 Commits

Reviewing files that changed from the base of the PR and between 935a6de and 2a2178e.

📒 Files selected for processing (5)
  • docs/develop/hostpid-broker.md
  • pkg/device-plugin/nvidiadevice/nvinternal/plugin/hostpid_broker.go
  • pkg/device-plugin/nvidiadevice/nvinternal/plugin/hostpid_broker_test.go
  • pkg/device-plugin/nvidiadevice/nvinternal/plugin/server.go
  • pkg/device-plugin/nvidiadevice/nvinternal/plugin/server_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • pkg/device-plugin/nvidiadevice/nvinternal/plugin/server.go

Comment thread docs/develop/hostpid-broker.md Outdated
Comment thread pkg/device-plugin/nvidiadevice/nvinternal/plugin/hostpid_broker_test.go Outdated
@iemAnshuman

Copy link
Copy Markdown
Contributor Author

@iemAnshuman This is really well done — the broker design is clean, the security validations are thorough (root-owned socket, SO_PEERCRED, read-only mount, 500ms deadline), and the feature gate with hostPIDBroker.enabled defaulting to false keeps backward compatibility intact. Great work pulling together the full client-server stack from RFC to implementation.

A couple of requests for the remaining validation:

  1. Complete test report — once the real kubelet allocation and rollout/rollback tests are done, could you share a comprehensive report covering both the enabled and disabled paths, along with any performance numbers?
  2. Both paths must work — with LIBVGPU_HOSTPID_BROKER=1 enabled, and without it (no env var), everything should function normally. The NVML fallback path should have zero regression since this feature defaults to off. The broker path should degrade gracefully when the socket is unreachable.

Looking forward to seeing this land.

thanks. I agree with both requirements.

The current evidence is not the complete report yet. Local and linux root checks cover lower level broker behavior and /tmp/vgpulock path safety. Real kubelet allocation, rollout, rollback, and performance runs are still pending.

I will post one report here after those runs with the source revisions, environment, enabled and disabled results, unreachable socket behavior, rollout and rollback results, and raw performance data. I will keep the feature marked incomplete until both paths pass and the disabled path shows no regression.

Signed-off-by: iemAnshuman <asquare567@gmail.com>
Signed-off-by: iemAnshuman <asquare567@gmail.com>
Signed-off-by: iemAnshuman <asquare567@gmail.com>
@iemAnshuman

Copy link
Copy Markdown
Contributor Author

Interim host PID broker validation report

I am posting the Linux root, A100, fallback, and package results now. The Kubernetes gate is still open, so this is not the completed test report requested above.

Recorded 11 August 2026 for HAMi issue 1662, RFC 2244, HAMi-core PR 251, and HAMi PR 2417.

Source boundaries

Role Revision Evidence boundary
HAMi-core client 4421b0970c172e343c7fb59491952b78ea9d86eb The Linux root archive covers the source through 607d7e83c282f8c494b39a0c2d92e16a83bdf965. Later review and lint commits are outside that archive.
HAMi server 146ee027b8b0ac44c8f8b3d48581f2cf69a485e9 The Linux root archive covers the parent and mount experiment on private base 261567a97d2ad157094f07b8f0a007587d4481bd. It does not bind the current public head byte for byte.

Each result below keeps its recorded source boundary.

Linux root validation

The root run completed on Ubuntu 24.04 arm64 with Linux 6.8.0. It recorded real and effective UID 0. Before creating a private mount namespace, the initial user namespace mapping was 0 0 4294967295.

The source contract binds HAMi-core base b0df70a7670ed9f8ccd2503f58ac40fa0463e97c to patch SHA256:

22ba0a1c4536b634c9fd85b412b15e1501efa365a2e82db17f596488b794ccaa

That patch is the tree at core commit 607d7e83c282f8c494b39a0c2d92e16a83bdf965.

It also binds private HAMi base 261567a97d2ad157094f07b8f0a007587d4481bd to patch SHA256:

6f7b2bb979a62f7360b49ceed90afc236bcec9a35bab7898d34c0adda3aa9352

All 28 recorded checks passed. They cover the normal and sanitizer CTest runs, cache deadlines, owner death, permission and object replacement cases, reverse order recovery, Go race and vet checks, root parent creation, nonroot fallback access, supported read only filesystems, and rejection of unsafe paths. Both CTest runs completed two tests with zero failures.

Linux result archive SHA256:

941d0a7c3de6e4602a8c57f3e71b8c21ad3acc60cef127d07599ef8ed6f7783a

A100 cuInit() result

Job 181726 compared core 8d59bf91a4ee4ef402a5be07167d319148679c8b and server 935a6dedfc23072c6f1b7ecc353dc5cc681b2591 with the gate unset and enabled. It used eight A100 GPUs, 48 caches, one pre API barrier, and four ABBA repetitions at each density.

Processes Gate unset Gate enabled Change
128 56.373 seconds 22.501 seconds 60.085% lower
300 130.662 seconds 50.288 seconds 61.513% lower

All 16 cells completed with zero failed workers. The 107 file result manifest SHA256 is:

f2f2129e239e3c7ef6e9cca94ee3f559f8c17f9057029a52d6267971aa777100

This run measures balanced cuInit(). It does not measure first primary context completion or Kubernetes deployment.

Primary context and fallback results

Job 181845 completed the broker condition at N=128 in 155.151656 seconds and at N=240 in 792.572612 seconds. N=300 reached its 903.11 second deadline.

Supplemental job 182609 recorded all 300 broker queries at N=300 with zero broker failures. The timeout therefore occurred after the broker query. This result does not establish that the broker alone fixes application ready latency.

Job 182607 tested the cross cache fallback correction at revision b0df70a7670ed9f8ccd2503f58ac40fa0463e97c.

Layout Own PID Clear failure Peer PID External PID
N=128, one GPU 57 71 0 0
N=128, eight GPUs 12 116 0 0
N=300 10 290 0 0

The candidate accepted no peer or external PID. Clear failures remained failures.

Fallback archive SHA256:

b0908fa732086f9244cd495a4c53c9529032faebbda89420aae316b480e29e7c

Package verification

The package verifier was rerun on 11 August 2026. It reports:

46 verified, 0 pending, 0 failures

Generated decision table SHA256:

a1fab16d4a7b25ec8b35962ea3d88f270f936ee09088cc735c6fcdb03dc9e314

Package ledger SHA256:

294637ae0abbe69115c9f900442029fa0ccb335ec08ad3e8982d17f8dea1448a

Kubernetes status

The controlled allocation and Helm bundles cover enabled and disabled response construction, lifecycle handling, mount configuration, chart lint, and render checks. They do not use a real kubelet or CUDA workload.

The following checks remain:

  1. Real kubelet allocation with the broker enabled and disabled.
  2. DaemonSet rollout and device plugin process restart.
  3. Degradation when the broker socket is unavailable.
  4. Helm rollback followed by a new CUDA allocation.

The available Rostam A100 environment provides Slurm user access. It does not provide the cgroup delegation, cluster administration, or node root access required by the runbook. Rootless Podman and Apptainer do not cover that boundary.

No Kubernetes result is claimed here. I will add the sealed Kubernetes result when a suitable isolated NVIDIA Kubernetes node is available.

Disclosure: I use AI assistance in my workflow. The code archaeology and measurements above are my own, and I am happy to walk through any part of them.

@mesutoezdil

Copy link
Copy Markdown
Contributor

This is being closed because it does not comply with the contribution guidelines.

@iemAnshuman

Copy link
Copy Markdown
Contributor Author

Hi @mesutoezdil, i read the contribution guidelines again after this pr was closed. The interim test report were generated with ai assistance rather than writthen by me. I take responsibility for posting that material and I now understand that it does not comply with the contribution guidelines.

Could you tell me which requirements caused the closure? Besides the Interim report? would like to know whether the size of the pr or the incomplete kubelet allocation and rollout tests were also factors.

@FouoF FouoF reopened this Aug 26, 2026
@FouoF

FouoF commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

@iemAnshuman Please resolve the conflict.

Signed-off-by: iemAnshuman <asquare567@gmail.com>
@iemAnshuman

Copy link
Copy Markdown
Contributor Author

@iemAnshuman Please resolve the conflict.

resolved, merged master in.

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.

4 participants