Skip to content

fix RDMA char device accumulation across multiple devices - #269

Merged
kubernetes-prow[bot] merged 2 commits into
kubernetes-sigs:mainfrom
gauravkghildiyal:fix/rdma-char-device-scope
Aug 7, 2026
Merged

fix RDMA char device accumulation across multiple devices#269
kubernetes-prow[bot] merged 2 commits into
kubernetes-sigs:mainfrom
gauravkghildiyal:fix/rdma-char-device-scope

Conversation

@gauravkghildiyal

Copy link
Copy Markdown
Member

What type of PR is this?

/kind bug
/kind cleanup

What this PR does / why we need it:

Previously, charDevices was declared outside the device loop in prepareResourceClaim, causing character device paths to accumulate across iterations when a single ResourceClaim allocated multiple RDMA devices. This change ensures each DeviceConfig stored in podConfigStore only contains its own character devices

Which issue(s) this PR is related to:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

@kubernetes-prow kubernetes-prow Bot added release-note-none Denotes a PR that doesn't merit a release note. kind/bug Categorizes issue or PR as related to a bug. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels Aug 1, 2026
@netlify

netlify Bot commented Aug 1, 2026

Copy link
Copy Markdown

Deploy Preview for dranet canceled.

Name Link
🔨 Latest commit aa168c8
🔍 Latest deploy log https://app.netlify.com/projects/dranet/deploys/6a7599f5d128f7000801c945

@kubernetes-prow kubernetes-prow Bot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 1, 2026
@kubernetes-prow kubernetes-prow Bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 1, 2026
@aojea

aojea commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

good catch, reproduced with

--- a/pkg/driver/dra_hooks_test.go
+++ b/pkg/driver/dra_hooks_test.go
@@ -32,6 +32,7 @@
     k8sresource "k8s.io/apimachinery/pkg/api/resource"
     metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
     "k8s.io/apimachinery/pkg/types"
+	"k8s.io/apimachinery/pkg/util/sets"
     "k8s.io/client-go/tools/record"
     "k8s.io/dynamic-resource-allocation/kubeletplugin"
     "k8s.io/utils/ptr"
@@ -954,3 +955,24 @@
         })
     }
 }
+
+func TestBuildRDMAConfigCharDeviceAccumulation(t *testing.T) {
+	// Regression test for the RDMA char device accumulation bug: charDevices is
+	// declared once outside the device loop in prepareResourceClaim and passed by
+	// reference into buildRDMAConfig for every device in the claim, so character
+	// device paths inserted while processing one device leak into the RDMAConfig
+	// computed for the next device.
+	//
+	// "/dev/null" stands in for a char device path belonging to a device
+	// processed earlier in the loop; it's a stock character device present on
+	// any Linux system, so the test needs no real RDMA hardware.
+	charDevices := sets.New[string]("/dev/null")
+
+	cfg := buildRDMAConfig("mlx5_1", charDevices)
+
+	for _, dc := range cfg.DevChars {
+		if dc.Path == "/dev/null" {
+			t.Errorf("RDMAConfig for mlx5_1 unexpectedly contains a char device left over from a different device: %v", cfg.DevChars)
+		}
+	}
+} 

please add an unit test as regression test

@gauravkghildiyal
gauravkghildiyal force-pushed the fix/rdma-char-device-scope branch from d899ee1 to 28a360b Compare August 7, 2026 08:08
@kubernetes-prow kubernetes-prow Bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 7, 2026
@gauravkghildiyal

Copy link
Copy Markdown
Member Author

Yep done! Thanks

@gauravkghildiyal
gauravkghildiyal force-pushed the fix/rdma-char-device-scope branch from 28a360b to b2d278e Compare August 7, 2026 08:19
@gauravkghildiyal
gauravkghildiyal force-pushed the fix/rdma-char-device-scope branch from b2d278e to aa168c8 Compare August 7, 2026 08:40
@aojea

aojea commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

/lgtm
/approve

I think that we can make GetRdmaDeviceForInterface(ifName string) (string, error) mockable in the db and improve the cogerage,

@kubernetes-prow kubernetes-prow Bot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 7, 2026
@kubernetes-prow

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aojea, gauravkghildiyal

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:
  • OWNERS [aojea,gauravkghildiyal]

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

@kubernetes-prow
kubernetes-prow Bot merged commit dfa356e into kubernetes-sigs:main Aug 7, 2026
13 checks passed
@gauravkghildiyal

Copy link
Copy Markdown
Member Author

/lgtm /approve

I think that we can make GetRdmaDeviceForInterface(ifName string) (string, error) mockable in the db and improve the cogerage,

+100. I saw that opportunity but plan on addressing this alongside other test improvements separately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants