feat: allow per-node memoryFactor override in device-plugin

[alehhu]

Fixes #2287

What this PR does / why we need it:

Currently, memoryFactor is a global setting in NvidiaConfig. When using Volcano vGPU DP (or in general heterogeneous clusters), different nodes might need different chunk granularities to avoid gRPC size limits on nodes with large GPUs, without forcing small GPUs to use large chunk sizes.

This PR allows configuring memoryfactor on a per-node basis via the nodeconfig configmap array, parsing it securely and falling back to the global default if omitted.

Implementation details:

Added MemoryFactor as an optional pointer (*int32 with omitempty) inside NodeDefaultConfig to preserve backwards compatibility. In server.go, the specific memoryfactor cleanly overrides the global sConfig.MemoryFactor during node config initialization.

AI Assistance Notice:

I consulted an AI assistant to explore the repository structure and reason about the scheduling flow, but the code changes and the solution design were authored and verified manually by myself.

Testing:

• make verify and go test ./... passed locally.
• Note on Hardware Validation: This PR only affects how the config file is parsed into structs and doesn't change the underlying device isolation/allocation logic directly, so it was tested via unit tests without needing a bare-metal multi-GPU validation.

Release note:

Allow setting `memoryfactor` on a per-node basis using `nodeConfiguration.config` in helm values.

Known Limits:

Scheduler vs Device Plugin Consistency: When using the default HAMi scheduler, the MutatingAdmissionWebhook operates at the cluster level before a node is assigned, so it will continue to use the global memoryFactor. The per-node memoryFactor is only applied locally by the DP. Users relying on the HAMi scheduler must ensure their global memoryFactor aligns with their workloads, while Volcano users (who bypass the HAMi webhook entirely) can fully benefit from this per-node chunking.

Summary by CodeRabbit

• New Features
  • Added configurable memory scaling for NVIDIA device nodes.
  • The memory factor defaults to 1 and can be customized per node.
  • Node-specific memory settings override the global configuration when provided.

[hami-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: alehhu
Once this PR has been reviewed and has the lgtm label, please assign dsfans2014 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:

• OWNERS

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

[hami-robot]

Welcome @alehhu! It looks like this is your first PR to Project-HAMi/HAMi 🎉

[coderabbitai]

📝 Walkthrough

Walkthrough

The change adds optional per-node NVIDIA MemoryFactor configuration, sets its Helm default to 1, and copies configured values into the scheduler configuration during config-file loading. Tests cover global retention and node-specific override behavior.

Changes

Per-node NVIDIA memory factor

Layer / File(s)	Summary
Memory factor configuration contract pkg/device/nvidia/device.go, charts/hami/values.yaml	NodeDefaultConfig adds optional YAML and JSON MemoryFactor fields. The Helm node configuration sets memoryfactor to 1.
Server propagation and override validation pkg/device-plugin/nvidiadevice/nvinternal/plugin/server.go, pkg/device-plugin/nvidiadevice/nvinternal/plugin/server_test.go	readFromConfigFile copies a configured node MemoryFactor into sConfig.MemoryFactor. Tests verify that the global value remains when no node override exists and that a node value overrides it.

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

Possibly related PRs

• Project-HAMi/HAMi#2172: Consumes NVIDIA MemoryFactor through NvidiaGPUDevices.MemoryFactor().
• Project-HAMi/HAMi#2191: Modifies NVIDIA memoryFactor configuration handling in the device-plugin server.

Suggested reviewers: fouof, cotishq

Poem

A rabbit checks each node’s setting,
Memory factors start offsetting.
Helm writes one into the chart,
The server reads the matching part.
NVIDIA values now align,
“Hop!” says the rabbit, “Config is fine.”

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The PR adds per-node MemoryFactor configuration and override behavior, directly addressing the requirements in [#2287].
Out of Scope Changes check	✅ Passed	All changes support per-node memoryFactor configuration, override handling, defaults, and related tests.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the per-node memoryFactor override added to the device plugin.

📋 Issue Planner

Built with CodeRabbit's Coding Plans for faster development and fewer bugs.

View plan used: #2287

✨ 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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

🧹 Nitpick comments (1)

pkg/device-plugin/nvidiadevice/nvinternal/plugin/server.go (1)

140-142: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Add regression coverage for MemoryFactor propagation.

The nil check and assignment are correct. The supplied Test_configOverride in pkg/device-plugin/nvidiadevice/nvinternal/plugin/server_test.go does not set or assert MemoryFactor. Add one node with an override and one node without the field. Verify both override propagation and fallback to the global value.

🤖 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.go` around lines 140
- 142, Add regression coverage in Test_configOverride for MemoryFactor
propagation: configure one node with a MemoryFactor override and another without
the field, then assert the first uses its node-specific value while the second
falls back to the global value. Keep the existing nil-check and assignment in
the server configuration flow unchanged.

🤖 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.

Nitpick comments:
In `@pkg/device-plugin/nvidiadevice/nvinternal/plugin/server.go`:
- Around line 140-142: Add regression coverage in Test_configOverride for
MemoryFactor propagation: configure one node with a MemoryFactor override and
another without the field, then assert the first uses its node-specific value
while the second falls back to the global value. Keep the existing nil-check and
assignment in the server configuration flow unchanged.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: e7726a9a-3e9b-4ae6-9061-30b048582191

📥 Commits

Reviewing files that changed from the base of the PR and between a5251af and 95554b8.

📒 Files selected for processing (3)

• charts/hami/values.yaml
• pkg/device-plugin/nvidiadevice/nvinternal/plugin/server.go
• pkg/device/nvidia/device.go

[mesutoezdil]

scheduler still uses the global nvidia.MemoryFactor for FitQuota and the webhook quota check. only device plugin config is per node here. scheduler and device plugin can end up using different factors for the same node. pls cover the scheduler side too or note this as a known limit

[mesutoezdil]

pls dont force push, add new commits instead. also re-check the contribution gates in CONTRIBUTING.md before pushing again

[alehhu]

Hi @mesutoezdil , sorry for the force push! It was an automatic reflex while I was fixing the tests locally. I'm a student and this is one of my first open source contributions (small fixes so far), so I'm still learning the right workflow. I will use normal commits next time.

About the scheduler issue: you're totally right. The webhook runs before the node is assigned, so it can't know the node-specific memory factor. I just updated the PR description to note this as a known limit for HAMi scheduler users. For Volcano users though, this should already solve their issue completely.
Let me know if it looks good now!

[mesutoezdil]

good that you added the known limit note. one question: does volcano vgpu skip the hami mutatingwebhook completely, or does the webhook still run and just get ignored by volcano scheduling. if it still runs, fitResourceQuota still uses the global factor, so quota checks could be wrong for volcano users too, not just chunking.

[alehhu]

Volcano vGPU completely skips the HAMi mutatingwebhook logic, so the global memoryFactor issue in fitResourceQuota does not affect Volcano users.

reasons:
In pkg/scheduler/webhook.go (around lines 65-69) https://github.com/Project-HAMi/HAMi/blob/master/pkg/scheduler/webhook.go#L65-L69, the webhook intercepts the
pod creation and performs a check on the SchedulerName:

if pod.Spec.SchedulerName != "" &&
    (pod.Spec.SchedulerName != corev1.DefaultSchedulerName || !config.ForceOverwriteDefaultScheduler) &&
    (len(config.SchedulerName) == 0 || pod.Spec.SchedulerName != config.SchedulerName) {
    klog.V(3).Infof(template+" - Pod already has different scheduler assigned", req.Namespace, req.Name, req.UID)
    return admission.Allowed("pod already has different scheduler assigned")
}

When a user submits a pod using Volcano, the pod.Spec.SchedulerName is explicitly set to volcano (or their custom volcano scheduler name). This means that:

1. It is not empty.
2. It is not the default-scheduler.
3. It does not match the HAMi scheduler name (config.SchedulwrName).

Since all these conditions evaluate to true, the webhook logs the event and performs an early return with admission.Allowed(...).

Because the execution stops there, the fitResourceQuota(pod) function (which is located further down on line 100) is never reached or executed for Volcano pods. Therefore, the global memoryFactor value is never applied to their quota checks.

[archlitchi]

i don't think a good idea to make this a 'per-node' configuration, because that will make 1 unit of 'device-memory' represent different amounts between nodes, and that makes user unable to predict how much device-memory they can actually get.

[MountainGG]

@archlitchi Hi, I think it’s not really worth giving up flexibility just to make memory units semantically consistent. Right now, the dp plugin doesn’t explicitly show the semantics of the units—it’s still up to the user (or higher-level software) to read them from config files or node annotations, so reading it once or multiple times doesn’t really matter. Plus, for people, it’s easier to handle actual physical memory identified by card type, like A800-80GB. Conversion and rounding of memory units should be done by higher-level software, and then just give people a heads-up.