fix v2.8.1 monitor for new version of cache file

[peachest]

What type of PR is this?

/kind bug

What this PR does / why we need it:

v2.8.1 tag HAMi bump HAMi-core version which has added new fields for struct type shared_region_t. This make vgpu-monitor error on newly created workload with new libvgpu.so

提交 1988c5dadd3e0dd49dc7c6f97c8014401b9f0f55

diff --git a/src/multiprocess/multiprocess_memory_limit.h b/src/multiprocess/multiprocess_memory_limit.h
index 870e4da..4a66bb0 100755
--- a/src/multiprocess/multiprocess_memory_limit.h
+++ b/src/multiprocess/multiprocess_memory_limit.h
@@ -78,11 +78,12 @@ typedef struct {
 typedef struct {
     _Atomic int32_t pid;           // Atomic to detect slot allocation
     _Atomic int32_t hostpid;
+    _Atomic uint64_t seqlock;      // Sequence lock for consistent snapshots
     device_memory_t used[CUDA_DEVICE_MAX_COUNT];
     _Atomic uint64_t monitorused[CUDA_DEVICE_MAX_COUNT];
     device_util_t device_util[CUDA_DEVICE_MAX_COUNT];
     _Atomic int32_t status;
-    uint64_t unused[3];
+    uint64_t unused[2];
 } shrreg_proc_slot_t;

提交 e200da2c2ace17207accf106030715c787ce8e54

diff --git a/src/multiprocess/multiprocess_memory_limit.h b/src/multiprocess/multiprocess_memory_limit.h
index 10794a2..600375e 100755
--- a/src/multiprocess/multiprocess_memory_limit.h
+++ b/src/multiprocess/multiprocess_memory_limit.h
@@ -95,6 +95,7 @@ typedef struct {
     _Atomic int32_t sm_init_flag;
     _Atomic size_t owner_pid;
     sem_t sem;  // Only for process slot add/remove
+    sem_t sem_postinit;  // For serializing postInit() host PID detection
     uint64_t device_num;
     uuid uuids[CUDA_DEVICE_MAX_COUNT];
     uint64_t limit[CUDA_DEVICE_MAX_COUNT];

These newly added fields do not use the space reserved by unused but inserted before other fields. This cause the vgpu-monitor failed to parse new format cache file and read out wrong data. But if I change the v1 spec directly, the old cache file will failed parsing too.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Also modify the field ownerPid from type uint32 to uint64 to match the declaration size_t owner_pid of struct shared_region_t in HAMi-core src/multiprocess/multiprocess_memory_limit.h.

Since we generally build HAMi and hami-core on platform amd64/arm64, the size_t should be 64-bit width. Previously, HAMi vgpu-monitor used uint32 as the type for the ownerPid field, but it was still able to parse container cache files correctly to retrieve metrics because the num uint64 field has an alignment guarantee of 8. (Refer to document: https://golang.google.cn/ref/spec#Size_and_alignment_guarantees), This compensates for the misalignment in the memory layout caused by using uint32 rather than uint64 for ownerPid. In fact, the values read from the fields between ownerPid and num are also incorrect, but this has no impact on the metric. All the fields required to parse the metric are located after num, so the metric appears to be perfectly normal.

The memory layout for SharedRegionT in package HAMi/pkg/monitor/nvidia/v1 is shown in the table below. Notice that the OffsetOf(sem)+SizeOf(sem)=52, but OffsetOf(num) is 56. Because num has type uint64, which has an alignment of 8.

=== sharedRegionT ===
sizeof: 2008952

Field                 OffsetOf    Size        AlignOf   
----------------------------------------------------------------
initializedFlag       0           4           4         
majorVersion          4           4           4         
minorVersion          8           4           4         
smInitFlag            12          4           4         
ownerPid              16          4           4         
sem                   20          32          1         
num                   56          8           8         
uuids                 64          1536        1         
limit                 1600        128         8         
smLimit               1728        128         8         
procs                 1856        2007040     8         
procnum               2008896     4           4         
utilizationSwitch     2008900     4           4         
recentKernel          2008904     4           4         
priority              2008908     4           4         
lastKernelTime        2008912     8           8         
unused                2008920     32          8

Does this PR introduce a user-facing change?: No

[hami-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: peachest
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:

• OWNERS

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

[gemini-code-assist]

Code Review

This pull request refactors the NVIDIA monitor to use a factory pattern for handling different versions of shared region cache files, decoupling the main monitor logic from specific version implementations. However, several issues were identified: changing ownerPid to uint64 in the base sharedRegionT struct breaks backward compatibility for v1.0 and v1.1 files; multiple methods in v1/spec.go lack bounds checking when slicing the procs array or iterating over device limits, which could lead to runtime panics; and the version matching logic for v1-sem should be updated to support future minor versions.

[codecov]

Codecov Report

❌ Patch coverage is 8.33333% with 99 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
pkg/monitor/nvidia/v1/spec.go	2.70%	72 Missing ⚠️
pkg/monitor/nvidia/api/api.go	0.00%	11 Missing ⚠️
pkg/monitor/nvidia/cudevshr.go	0.00%	11 Missing ⚠️
pkg/monitor/nvidia/v0/spec.go	0.00%	5 Missing ⚠️

Flag	Coverage Δ
unittests	57.53% <8.33%> (-0.52%)	⬇️

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

Files with missing lines	Coverage Δ
pkg/monitor/nvidia/factory.go	100.00% <100.00%> (ø)
pkg/monitor/nvidia/v0/spec.go	88.57% <0.00%> (-6.82%)	⬇️
pkg/monitor/nvidia/api/api.go	0.00% <0.00%> (ø)
pkg/monitor/nvidia/cudevshr.go	0.62% <0.00%> (+0.62%)	⬆️
pkg/monitor/nvidia/v1/spec.go	43.60% <2.70%> (-51.78%)	⬇️

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[spencercjh]

I found an initialization issue in this PR: the v0/v1 cache factories are not being registered correctly at startup, so findFactory(...) can return nil for valid cache files.

Because of that, the vgpu-monitor fails to parse the shared cache and crashes in the loadCache path.

Could you update the initialization flow to ensure v0 and v1 are always registered before cache loading begins, and add a test that verifies factory registration/routing on startup?

When I patch it in the test env. The vgpu-monitor finally works.

Here is my test branch: https://github.com/spencercjh/HAMi/tree/fix/multiple-version-monitor

[peachest]

I found an initialization issue in this PR: the v0/v1 cache factories are not being registered correctly at startup, so findFactory(...) can return nil for valid cache files.我在这个 PR 中发现了一个初始化问题：v0/v1 缓存工厂在启动时没有被正确注册，因此 findFactory(...)可能会对有效的缓存文件返回 nil。

Because of that, the vgpu-monitor fails to parse the shared cache and crashes in the loadCache path.由于这个原因，vgpu-monitor 无法解析共享缓存，并在加载缓存路径中崩溃。

Could you update the initialization flow to ensure v0 and v1 are always registered before cache loading begins, and add a test that verifies factory registration/routing on startup?你能更新初始化流程，确保在缓存加载开始前 v0 和 v1 总是被注册，并添加一个测试来验证启动时的工厂注册/路由吗？

When I patch it in the test env. The vgpu-monitor finally works.在我测试环境中打上补丁后，vgpu-monitor 终于可以正常工作了。

Here is my test branch: https://github.com/spencercjh/HAMi/tree/fix/multiple-version-monitor这是我的测试分支：https://github.com/spencercjh/HAMi/tree/fix/multiple-version-monitor

The reason v0 and v1 were not registered correctly is likely because the relevant packages were originally imported, so the init function was used for registration at the outset. However, after switching to the factory pattern, the v0 and v1 packages were no longer imported, meaning that the initialisation via the init function did not take effect.

I greatly appreciate your code implementation and hope it will be adopted; I am currently working on merging your latest commit.

I found that you have added a new file values-minikube.yaml in your last 2 commits. Could you provide a clean commit without this file, so that I can directly cherry-pick your commit and preserve your author info

[spencercjh]

@peachest You can cherry-pick commits from https://github.com/spencercjh/HAMi/tree/fix/multiple-version-monitor.

Thanks for you time.

[mesutoezdil]

@peachest

nr1. seqlock is in the wrong position in shrregProcSlotTWithSeqlock.
The C struct places seqlock as the 3rd field right after hostpid (offset 8). The Go struct places it near the end after status. This means used[], monitorused[], and deviceUtil[] will all be read from wrong offsets for v1.2 cache files.

C layout: pid | hostpid | seqlock | used[] | ... | status | unused[2]
Go layout: pid | hostpid | used[] | ... | status | seqlock | unused[2] (wrong)

nr2. procnum is not bounds-checked before slicing.
s.sr.procs[:int(s.sr.procnum)] will panic if procnum is negative or greater than 1024. A corrupted or mid-write cache file can trigger this.