Skip to content

fix(device-plugin): return error on failed Send in ListAndWatch - #2353

Merged
hami-robot[bot] merged 2 commits into
Project-HAMi:masterfrom
AyushSrivastava1818:fix/listandwatch-send-error
Aug 7, 2026
Merged

fix(device-plugin): return error on failed Send in ListAndWatch#2353
hami-robot[bot] merged 2 commits into
Project-HAMi:masterfrom
AyushSrivastava1818:fix/listandwatch-send-error

Conversation

@AyushSrivastava1818

@AyushSrivastava1818 AyushSrivastava1818 commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

What type of PR is this?

/kind bug

What this PR does / why we need it

ListAndWatch() (pkg/device-plugin/nvidiadevice/nvinternal/plugin/server.go) discarded the error returned by s.Send() at both call sites.

If the gRPC stream to kubelet becomes unavailable (for example, due to a client disconnect or stream cancellation), Send() returns an error. The current implementation ignored that error and continued running, causing subsequent device health updates to continue attempting writes on a failed stream.

This PR logs and returns the Send() error at both call sites, allowing the current ListAndWatch() RPC to terminate instead of silently ignoring stream failures. This follows the standard gRPC server-stream error handling pattern already used elsewhere in the codebase.

Which issue(s) this PR fixes

Fixes #2352

Special notes for your reviewer

  • No behavioral changes to the successful (Send() succeeds) path.
  • Added TestListAndWatch_SendError covering both Send() call sites:
    • Initial Send() failure.
    • Health-update Send() failure after an initial successful send.
  • The tests verify that ListAndWatch() returns the propagated error and stops sending additional updates after the failure.
  • Verified locally with:
    • go test ./pkg/device-plugin/nvidiadevice/nvinternal/plugin/... -v
    • make verify

AI assistance disclosure

I consulted ChatGPT and Claude to identify the issue and discuss the implementation approach. The implementation, tests, validation, commit message, and final code changes were completed and verified by me. I reviewed all generated suggestions, ran the relevant tests and make verify locally, and take full responsibility for the final contribution.

Does this PR introduce a user-facing change?

NONE

Summary by CodeRabbit

  • Bug Fixes

    • Improved reliability of device status reporting by detecting communication failures with the cluster.
    • Prevented failed initial or health-update messages from being silently ignored.
    • Ensured device monitoring responds appropriately when status updates cannot be delivered.
  • Tests

    • Added coverage for communication failures during initial and subsequent device status updates.

Signed-off-by: AyushSrivastava1818 <ayush.sri0705@gmail.com>
@hami-robot hami-robot Bot added kind/bug Something isn't working dco-signoff: yes labels Aug 4, 2026
@hami-robot
hami-robot Bot requested review from lengrongfu and wawa0210 August 4, 2026 14:20
@hami-robot hami-robot Bot added the size/M label Aug 4, 2026
@coderabbitai

coderabbitai Bot commented Aug 4, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 64c30e71-2236-44a5-ad41-e4ff95fc4562

📥 Commits

Reviewing files that changed from the base of the PR and between 06e379d and 7ee96a8.

📒 Files selected for processing (2)
  • 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 (2)
  • pkg/device-plugin/nvidiadevice/nvinternal/plugin/server.go
  • pkg/device-plugin/nvidiadevice/nvinternal/plugin/server_test.go

📝 Walkthrough

Walkthrough

ListAndWatch now handles errors from initial and health-update gRPC response sends. Tests cover both send-failure paths with a mock server stream.

Changes

ListAndWatch stream error handling

Layer / File(s) Summary
Propagate response send errors
pkg/device-plugin/nvidiadevice/nvinternal/plugin/server.go, pkg/device-plugin/nvidiadevice/nvinternal/plugin/server_test.go
ListAndWatch logs failures from initial and health-update sends. The method returns after each failure path. Mock stream tests verify the initial-send error and the update-send error after two responses.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

Suggested reviewers: wawa0210

Poem

A rabbit watched the stream send,
Then found an error at the end.
Logs marked the break,
Tests checked each wake,
And updates stopped as planned.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning Initial Send errors propagate, but health-update Send errors return nil instead of the Send error required by issue #2352. Return the health-update Send error from ListAndWatch instead of returning nil after logging it.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the primary change to handle failed Send calls in ListAndWatch.
Out of Scope Changes check ✅ Passed The changes are limited to ListAndWatch Send-error handling and focused tests for issue #2352.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ 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.

@codecov

codecov Bot commented Aug 4, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

Flag Coverage Δ
unittests 62.44% <100.00%> (+1.42%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...ce-plugin/nvidiadevice/nvinternal/plugin/server.go 42.15% <100.00%> (+2.82%) ⬆️

... and 21 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread pkg/device-plugin/nvidiadevice/nvinternal/plugin/server.go Outdated
…ream NVIDIA device plugin

Signed-off-by: AyushSrivastava1818 <ayush.sri0705@gmail.com>
@archlitchi

Copy link
Copy Markdown
Member

/assign @Shouren

@Shouren Shouren left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@archlitchi archlitchi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@hami-robot

hami-robot Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: archlitchi, AyushSrivastava1818, Shouren

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

The pull request process is described 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

@hami-robot
hami-robot Bot merged commit 30cee9e into Project-HAMi:master Aug 7, 2026
17 checks passed
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.

device-plugin: ListAndWatch ignores gRPC stream Send() errors

3 participants