fix(monitor): avoid uint64 underflow when deriving the legacy device-memory offset label

[Luffy-nani]

What type of PR is this?
/kind bug

What this PR does / why we need it:
This fixes a possible uint64 underflow when collectContainerMetrics calculates the legacy Device_memory_desc_of_container metric's offset label

The values used for the calculation come from separate reads of shared memory, so they can become inconsistent if hami-core updates the memory information between reads. In that case, the subtraction can wrap around to a value close to math.MaxUint64

Instead of calculating the offset from those four values, this change uses the existing DeviceMemoryOffset() method, which returns the recorded offset directly and avoids the underflow

A regression test was also added to cover the torn-read scenario

Which issue(s) this PR fixes:
Fixes #2551

Special notes:

The full test suite could not be run locally because the environment could not access the Go module proxy required by the repository's Go toolchain version. gofmt was run on all touched files

The new test also avoids calling initLegacyDescriptors() because it modifies package-level Prometheus descriptors and could affect other tests

Does this PR introduce a user-facing change?:

No. This does not change the intended metric behavior. It prevents an invalid, wrapped offset value from being exposed when shared-memory values are read inconsistently

Summary by CodeRabbit

• Bug Fixes
  • Corrected legacy device-memory metrics to report accurate per-device memory offsets.
  • Prevented invalid offset values when total memory is smaller than component sizes.
  • Improved metric collection reliability for legacy configurations.

[hami-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Luffy-nani
Once this PR has been reviewed and has the lgtm label, please assign shouren 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 @Luffy-nani! It looks like this is your first PR to Project-HAMi/HAMi 🎉

[coderabbitai]

📝 Walkthrough

Walkthrough

The legacy container memory metric now uses the recorded per-device memory offset. Test fixtures support indexed offsets, and a regression test verifies correct emission when component sizes exceed total memory.

Changes

Legacy device-memory metrics

Layer / File(s)	Summary
Use recorded device-memory offsets cmd/vGPUmonitor/feedback_test.go, cmd/vGPUmonitor/metrics.go, cmd/vGPUmonitor/metrics_container_test.go	stubInfo stores indexed offsets. Legacy metrics read DeviceMemoryOffset(i). The regression test verifies that offset 7 is emitted without uint64 underflow.

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

Possibly related PRs

• Project-HAMi/HAMi#2021: Both changes modify legacy container memory metric offset handling.
• Project-HAMi/HAMi#2350: Both changes update stubInfo and container-metrics tests.
• Project-HAMi/HAMi#2392: Both changes modify legacy device-memory metric offset handling.

Suggested reviewers: ouyangluwei163, fouof

Poem

A rabbit checks the offset line,
The recorded seven stays in time.
No unsigned wrap can hop away,
The metric shows the value each day.
“Verified,” says the rabbit.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.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	The implementation uses DeviceMemoryOffset() and adds a regression test that directly address issue #2551.
Out of Scope Changes check	✅ Passed	All changes support the linked issue by updating the offset source, test fixture, and regression coverage.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main fix for the legacy device-memory offset underflow.

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

• Create stacked PR
• Commit on current branch

🧪 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]

Actionable comments posted: 1

🤖 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/vGPUmonitor/metrics_container_test.go`:
- Around line 158-159: Update the comment immediately above
TestCollectContainerMetricsLegacyOffsetNoUnderflow so it describes that test’s
legacy offset behavior and no-underflow validation, removing the stale
TestCheckBlocking_MultiDevice description.

🪄 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: 88e8e39a-c28b-4ed9-9318-bc62edc9995e

📥 Commits

Reviewing files that changed from the base of the PR and between 91f0248 and 791b29e.

📒 Files selected for processing (3)

• cmd/vGPUmonitor/feedback_test.go
• cmd/vGPUmonitor/metrics.go
• cmd/vGPUmonitor/metrics_container_test.go

[FouoF]

Thanks for investigating the underflow risk, but this implementation is not viable. The current head does not compile: the cleanup closure assigns legacyCtrDeviceMemorydesc = prev after the prev declaration was removed. More importantly, the pinned HAMi-core production writer never assigns a nonzero used[dev].offset; allocation updates total and the context/module/data fields instead. DeviceMemoryOffset() therefore reads zero from real caches, while the test invents an offset=7 producer state that does not exist. This change would silently replace a meaningful legacy residual with zero and does not solve the possible torn-read problem. A coherent snapshot/seqlock-style fix or another producer-backed contract would require a separate design and real reproduction, so we are closing this PR.

[Luffy-nani]

got it! thanks