From b99bbe3832b7d36fa666792afb7bbed76b869e74 Mon Sep 17 00:00:00 2001 From: yummybomb <19238148+yummybomb@users.noreply.github.com> Date: Thu, 20 Aug 2026 15:39:50 +0000 Subject: [PATCH 01/17] Quarantine wedged vGPU VFs convicted by the guest sentinel --- cmd/api/main.go | 6 + cmd/api/wire.go | 42 ++-- cmd/api/wire_gen.go | 86 ++++---- lib/devices/GPU.md | 32 ++- lib/devices/manager.go | 4 + lib/devices/vendor_vfio_linux.go | 40 +++- lib/devices/vendor_vfio_linux_test.go | 72 +++++++ lib/devices/vf_health.go | 166 ++++++++++++++++ lib/devices/vf_health_darwin.go | 7 + lib/devices/vf_health_test.go | 67 +++++++ lib/instances/vgpu_sentinel.go | 274 ++++++++++++++++++++++++++ lib/instances/vgpu_sentinel_test.go | 184 +++++++++++++++++ lib/paths/paths.go | 5 + lib/providers/vgpu_sentinel.go | 13 ++ 14 files changed, 929 insertions(+), 69 deletions(-) create mode 100644 lib/devices/vf_health.go create mode 100644 lib/devices/vf_health_darwin.go create mode 100644 lib/devices/vf_health_test.go create mode 100644 lib/instances/vgpu_sentinel.go create mode 100644 lib/instances/vgpu_sentinel_test.go create mode 100644 lib/providers/vgpu_sentinel.go diff --git a/cmd/api/main.go b/cmd/api/main.go index 71e147ecf..2cd41d238 100644 --- a/cmd/api/main.go +++ b/cmd/api/main.go @@ -709,6 +709,12 @@ func run() error { return app.HealthCheckController.Run(gctx) }) } + if app.VGPUSentinelController != nil { + grp.Go(func() error { + logger.Info("starting vGPU sentinel controller") + return app.VGPUSentinelController.Run(gctx) + }) + } if restartController, ok := app.InstanceManager.(interface { StartRestartPolicyController(context.Context) error }); ok { diff --git a/cmd/api/wire.go b/cmd/api/wire.go index 133bf41f6..123d59bf5 100644 --- a/cmd/api/wire.go +++ b/cmd/api/wire.go @@ -29,26 +29,27 @@ import ( // application struct to hold initialized components type application struct { - Ctx context.Context - Logger *slog.Logger - Config *config.Config - ImageManager images.Manager - SystemManager system.Manager - NetworkManager network.Manager - DeviceManager devices.Manager - InstanceManager instances.Manager - VolumeManager volumes.Manager - BuilderManager builders.Manager - IngressManager ingress.Manager - BuildManager builds.Manager - PushManager imagepush.Manager - ResourceManager *resources.Manager - GuestMemoryController guestmemory.Controller - AutoStandbyController *autostandby.Controller - HealthCheckController *instances.HealthCheckController - VMMetricsManager *vm_metrics.Manager - Registry *registry.Registry - ApiService *api.ApiService + Ctx context.Context + Logger *slog.Logger + Config *config.Config + ImageManager images.Manager + SystemManager system.Manager + NetworkManager network.Manager + DeviceManager devices.Manager + InstanceManager instances.Manager + VolumeManager volumes.Manager + BuilderManager builders.Manager + IngressManager ingress.Manager + BuildManager builds.Manager + PushManager imagepush.Manager + ResourceManager *resources.Manager + GuestMemoryController guestmemory.Controller + AutoStandbyController *autostandby.Controller + HealthCheckController *instances.HealthCheckController + VGPUSentinelController *instances.VGPUSentinelController + VMMetricsManager *vm_metrics.Manager + Registry *registry.Registry + ApiService *api.ApiService } // initializeApp is the injector function @@ -72,6 +73,7 @@ func initializeApp() (*application, func(), error) { providers.ProvideGuestMemoryController, providers.ProvideAutoStandbyController, providers.ProvideHealthCheckController, + providers.ProvideVGPUSentinelController, providers.ProvideVMMetricsManager, providers.ProvideRegistry, api.New, diff --git a/cmd/api/wire_gen.go b/cmd/api/wire_gen.go index 9eb13c6ea..57034551a 100644 --- a/cmd/api/wire_gen.go +++ b/cmd/api/wire_gen.go @@ -82,6 +82,10 @@ func initializeApp() (*application, func(), error) { } autostandbyController := providers.ProvideAutoStandbyController(instancesManager, config, logger) healthCheckController := providers.ProvideHealthCheckController(instancesManager, logger) + vgpuSentinelController, err := providers.ProvideVGPUSentinelController(instancesManager, logger) + if err != nil { + return nil, nil, err + } vm_metricsManager, err := providers.ProvideVMMetricsManager(instancesManager, config, logger) if err != nil { return nil, nil, err @@ -92,26 +96,27 @@ func initializeApp() (*application, func(), error) { } apiService := api.New(config, manager, instancesManager, volumesManager, buildersManager, networkManager, devicesManager, ingressManager, buildsManager, imagepushManager, resourcesManager, controller, autostandbyController, vm_metricsManager) mainApplication := &application{ - Ctx: context, - Logger: logger, - Config: config, - ImageManager: manager, - SystemManager: systemManager, - NetworkManager: networkManager, - DeviceManager: devicesManager, - InstanceManager: instancesManager, - VolumeManager: volumesManager, - BuilderManager: buildersManager, - IngressManager: ingressManager, - BuildManager: buildsManager, - PushManager: imagepushManager, - ResourceManager: resourcesManager, - GuestMemoryController: controller, - AutoStandbyController: autostandbyController, - HealthCheckController: healthCheckController, - VMMetricsManager: vm_metricsManager, - Registry: registry, - ApiService: apiService, + Ctx: context, + Logger: logger, + Config: config, + ImageManager: manager, + SystemManager: systemManager, + NetworkManager: networkManager, + DeviceManager: devicesManager, + InstanceManager: instancesManager, + VolumeManager: volumesManager, + BuilderManager: buildersManager, + IngressManager: ingressManager, + BuildManager: buildsManager, + PushManager: imagepushManager, + ResourceManager: resourcesManager, + GuestMemoryController: controller, + AutoStandbyController: autostandbyController, + HealthCheckController: healthCheckController, + VGPUSentinelController: vgpuSentinelController, + VMMetricsManager: vm_metricsManager, + Registry: registry, + ApiService: apiService, } return mainApplication, func() { }, nil @@ -121,24 +126,25 @@ func initializeApp() (*application, func(), error) { // application struct to hold initialized components type application struct { - Ctx context.Context - Logger *slog.Logger - Config *config.Config - ImageManager images.Manager - SystemManager system.Manager - NetworkManager network.Manager - DeviceManager devices.Manager - InstanceManager instances.Manager - VolumeManager volumes.Manager - BuilderManager builders.Manager - IngressManager ingress.Manager - BuildManager builds.Manager - PushManager imagepush.Manager - ResourceManager *resources.Manager - GuestMemoryController guestmemory.Controller - AutoStandbyController *autostandby.Controller - HealthCheckController *instances.HealthCheckController - VMMetricsManager *vm_metrics.Manager - Registry *registry.Registry - ApiService *api.ApiService + Ctx context.Context + Logger *slog.Logger + Config *config.Config + ImageManager images.Manager + SystemManager system.Manager + NetworkManager network.Manager + DeviceManager devices.Manager + InstanceManager instances.Manager + VolumeManager volumes.Manager + BuilderManager builders.Manager + IngressManager ingress.Manager + BuildManager builds.Manager + PushManager imagepush.Manager + ResourceManager *resources.Manager + GuestMemoryController guestmemory.Controller + AutoStandbyController *autostandby.Controller + HealthCheckController *instances.HealthCheckController + VGPUSentinelController *instances.VGPUSentinelController + VMMetricsManager *vm_metrics.Manager + Registry *registry.Registry + ApiService *api.ApiService } diff --git a/lib/devices/GPU.md b/lib/devices/GPU.md index ff11c5cde..c6e264b2f 100644 --- a/lib/devices/GPU.md +++ b/lib/devices/GPU.md @@ -282,13 +282,26 @@ NVRM: GPU 0000:00:03.0: RmInitAdapter failed! (0x22:0x65:884) ``` (0x65 = timeout; the guest's init requests are never answered, and -`/proc/interrupts` shows the GPU's MSI-X vectors allocated but idle). Because -placement is deterministic least-loaded, an idle host re-picks the same VF for -every request, so one wedged VF presents as all vGPU instances failing while -`/resources` reports full capacity. - -The wedge itself leaves no host-side log: no kernel error, no XID, no plugin -crash. The trigger is a SIGKILL delivered to QEMU while the vGPU plugin is +`/proc/interrupts` shows the GPU's MSI-X vectors allocated but idle). + +Hypeman detects this automatically: the guest kernel writes that line to the +serial console, which lands in the instance's `logs/app.log`, and the vGPU +sentinel controller scans that file for every vendor VFIO instance. A match +quarantines the VF in `/gpu/vf-health.json` (it survives restarts): +the VF is excluded from placement and from advertised profile availability, +and its parent GPU becomes overflow-only so it drains toward the SR-IOV +cycle. The conviction is logged at error level (`quarantined wedged vGPU VF`) +and counted in `hypeman_instances_vgpu_sentinel_convictions_total`; +`hypeman_instances_vgpu_quarantined_vfs` gauges the current quarantine count. +A burst of convictions (more than 3 in 15 minutes) pauses auto-conviction, so +a systemic non-wedge init failure — e.g. a guest/host driver mismatch rolling +out — cannot quarantine the fleet. + +The wedge-creating kill itself leaves no host-side log: no kernel error, no +XID, no plugin crash. Detection therefore happens on the next boot that lands +on the VF, whose guest emits the sentinel ~27s after spawn. + +The trigger is a SIGKILL delivered to QEMU while the vGPU plugin is still initializing the VF (roughly the first seconds after process start): a single hard kill in that window wedges the VF near-deterministically, while QEMU processes that exit voluntarily — error exits, QMP quit, SIGTERM — @@ -310,6 +323,11 @@ requires no vGPU assignments on that GPU): /usr/lib/nvidia/sriov-manage -e ``` +After the cycle, boot a verification instance on the recovered VF and confirm +its guest reaches the driver (no sentinel in its app log), then clear the +quarantine by removing the VF's entry from `/gpu/vf-health.json` +and restarting hypeman. + Do not unbind/rebind the VF from the nvidia driver — it breaks the nvidia-vgpu-vfio core-device registration (`vfio_pci_core_device not found`) and the VF stops accepting assignments entirely until the SR-IOV cycle. diff --git a/lib/devices/manager.go b/lib/devices/manager.go index 30763c04d..50e615e76 100644 --- a/lib/devices/manager.go +++ b/lib/devices/manager.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "fmt" + "log/slog" "os" "runtime" "strings" @@ -85,6 +86,9 @@ type manager struct { // NewManager creates a new device manager. // Use SetLivenessChecker after construction to enable accurate orphan detection. func NewManager(p *paths.Paths) Manager { + if err := initVFHealthStore(p.VFHealthState()); err != nil { + slog.Default().Error("failed to load VF health state; persisted quarantines are not in effect", "error", err) + } return &manager{ paths: p, vfioBinder: NewVFIOBinder(), diff --git a/lib/devices/vendor_vfio_linux.go b/lib/devices/vendor_vfio_linux.go index a6d37e80d..2201ecaf5 100644 --- a/lib/devices/vendor_vfio_linux.go +++ b/lib/devices/vendor_vfio_linux.go @@ -7,6 +7,7 @@ import ( "errors" "fmt" "log/slog" + "math/rand/v2" "os" "path/filepath" "sort" @@ -29,6 +30,15 @@ type vendorVFIOSysfs struct { vfioDevicesPath string owners map[string]string framebufferByType map[string]int + pickVFIndex func(n int) int // overridden in tests; nil means random +} + +// withVGPUPlacementLock runs f under the lock that serializes vendor VFIO +// vGPU placement, so quarantine updates and VF selection cannot interleave. +func withVGPUPlacementLock(f func()) { + vendorVFIOMu.Lock() + defer vendorVFIOMu.Unlock() + f() } var ( @@ -102,6 +112,7 @@ func (s vendorVFIOSysfs) discoverVFs() ([]VirtualFunction, error) { // available_instances. This is a best-effort snapshot because creating on one // VF may revoke the type from siblings that share its GPU framebuffer. func (s vendorVFIOSysfs) listProfiles(vfs []VirtualFunction) ([]GPUProfile, error) { + quarantined := vfHealth.snapshotAddresses() profilesByType := make(map[string]profileMetadata) creatableVFs := make(map[string]int) for _, vf := range vfs { @@ -113,9 +124,10 @@ func (s vendorVFIOSysfs) listProfiles(vfs []VirtualFunction) ([]GPUProfile, erro slog.Default().Warn("skipping unreadable creatable vGPU types", "vf", vf.PCIAddress, "error", err) continue } + _, bad := quarantined[vf.PCIAddress] for _, profile := range creatable { profilesByType[profile.TypeName] = profile - if !vf.Allocated { + if !vf.Allocated && !bad { creatableVFs[profile.TypeName]++ } } @@ -304,10 +316,22 @@ func (s vendorVFIOSysfs) reconcile(ctx context.Context, protectedDevicePaths map } func (s vendorVFIOSysfs) selectLeastLoadedVF(vfs []VirtualFunction, profileType string) (string, error) { + quarantined := vfHealth.snapshotAddresses() usageByGPU := make(map[string]int) unknownUsageByGPU := make(map[string]bool) + quarantinedByGPU := make(map[string]int) freeByGPU := make(map[string][]VirtualFunction) for _, vf := range vfs { + // A quarantined VF is never a placement candidate, but its parent GPU + // stays usable: the count only deprioritizes the card so it drains + // toward the SR-IOV cycle instead of staying warm. + _, bad := quarantined[vf.PCIAddress] + if bad { + quarantinedByGPU[vf.ParentGPU]++ + if !vf.Allocated { + continue + } + } if vf.Allocated { // framebufferByType only covers currently creatable profiles, so // after a restart an allocated type can be missing when its @@ -341,6 +365,9 @@ func (s vendorVFIOSysfs) selectLeastLoadedVF(vfs []VirtualFunction, profileType gpus = append(gpus, gpu) } sort.Slice(gpus, func(i, j int) bool { + if quarantinedByGPU[gpus[i]] != quarantinedByGPU[gpus[j]] { + return quarantinedByGPU[gpus[i]] < quarantinedByGPU[gpus[j]] + } if unknownUsageByGPU[gpus[i]] != unknownUsageByGPU[gpus[j]] { return !unknownUsageByGPU[gpus[i]] } @@ -352,7 +379,16 @@ func (s vendorVFIOSysfs) selectLeastLoadedVF(vfs []VirtualFunction, profileType if len(gpus) == 0 { return "", nil } - return freeByGPU[gpus[0]][0].PCIAddress, nil + // Randomize among the chosen GPU's free VFs. A deterministic + // lowest-address pick would route every first create on an idle host to + // the same VF, so a single undetected wedged VF presents as every GPU + // create failing. + candidates := freeByGPU[gpus[0]] + pick := s.pickVFIndex + if pick == nil { + pick = rand.IntN + } + return candidates[pick(len(candidates))].PCIAddress, nil } func (s vendorVFIOSysfs) profileMetadata(vfs []VirtualFunction) ([]profileMetadata, error) { diff --git a/lib/devices/vendor_vfio_linux_test.go b/lib/devices/vendor_vfio_linux_test.go index 4f2e40844..490348502 100644 --- a/lib/devices/vendor_vfio_linux_test.go +++ b/lib/devices/vendor_vfio_linux_test.go @@ -571,3 +571,75 @@ func assertFileValue(t *testing.T, path, expected string) { require.NoError(t, err) assert.Equal(t, expected, string(value)) } + +func TestVendorVFIOSkipsQuarantinedVF(t *testing.T) { + resetVFHealthStore(t) + _, err := QuarantineVF(VFQuarantine{VFAddress: "0000:82:00.4"}) + require.NoError(t, err) + + sysfs := newTestVendorVFIOSysfs(t) + sysfs.pickVFIndex = func(int) int { return 0 } + sysfs.addVF(t, "0000:82:00.0", "0000:82:00.4", "42", "0", testCreatableTypes) + sysfs.addVF(t, "0000:82:00.0", "0000:82:00.5", "43", "0", testCreatableTypes) + + device, err := sysfs.create(context.Background(), "NVIDIA L40S-1Q", "instance-1") + require.NoError(t, err) + assert.Equal(t, "0000:82:00.5", device.VFAddress) +} + +func TestVendorVFIONoVFWhenAllQuarantined(t *testing.T) { + resetVFHealthStore(t) + _, err := QuarantineVF(VFQuarantine{VFAddress: "0000:82:00.4"}) + require.NoError(t, err) + + sysfs := newTestVendorVFIOSysfs(t) + sysfs.addVF(t, "0000:82:00.0", "0000:82:00.4", "42", "0", testCreatableTypes) + + _, err = sysfs.create(context.Background(), "NVIDIA L40S-1Q", "instance-1") + require.ErrorContains(t, err, "no available VF") +} + +func TestVendorVFIOCardBiasAvoidsGPUWithQuarantinedVF(t *testing.T) { + resetVFHealthStore(t) + _, err := QuarantineVF(VFQuarantine{VFAddress: "0000:82:00.4"}) + require.NoError(t, err) + + // GPU 82 sorts first by address and has a free healthy VF, but its + // quarantined sibling must demote the whole card to overflow-only. + sysfs := newTestVendorVFIOSysfs(t) + sysfs.pickVFIndex = func(int) int { return 0 } + sysfs.addVF(t, "0000:82:00.0", "0000:82:00.4", "42", "0", testCreatableTypes) + sysfs.addVF(t, "0000:82:00.0", "0000:82:00.5", "43", "0", testCreatableTypes) + sysfs.addVF(t, "0000:e3:00.0", "0000:e3:00.4", "44", "0", testCreatableTypes) + + device, err := sysfs.create(context.Background(), "NVIDIA L40S-1Q", "instance-1") + require.NoError(t, err) + assert.Equal(t, "0000:e3:00.4", device.VFAddress) +} + +func TestVendorVFIOSelectUsesTiebreakAmongFreeVFs(t *testing.T) { + sysfs := newTestVendorVFIOSysfs(t) + sysfs.pickVFIndex = func(n int) int { return n - 1 } + sysfs.addVF(t, "0000:82:00.0", "0000:82:00.4", "42", "0", testCreatableTypes) + sysfs.addVF(t, "0000:82:00.0", "0000:82:00.5", "43", "0", testCreatableTypes) + + device, err := sysfs.create(context.Background(), "NVIDIA L40S-1Q", "instance-1") + require.NoError(t, err) + assert.Equal(t, "0000:82:00.5", device.VFAddress) +} + +func TestVendorVFIOListProfilesExcludesQuarantinedFromAvailability(t *testing.T) { + resetVFHealthStore(t) + _, err := QuarantineVF(VFQuarantine{VFAddress: "0000:82:00.4"}) + require.NoError(t, err) + + sysfs := newTestVendorVFIOSysfs(t) + sysfs.addVF(t, "0000:82:00.0", "0000:82:00.4", "42", "0", testCreatableTypes) + sysfs.addVF(t, "0000:82:00.0", "0000:82:00.5", "43", "0", testCreatableTypes) + + vfs, err := sysfs.discoverVFs() + require.NoError(t, err) + profiles, err := sysfs.listProfiles(vfs) + require.NoError(t, err) + assert.Equal(t, 1, profileAvailability(profiles, "NVIDIA L40S-1Q")) +} diff --git a/lib/devices/vf_health.go b/lib/devices/vf_health.go new file mode 100644 index 000000000..1560ab0eb --- /dev/null +++ b/lib/devices/vf_health.go @@ -0,0 +1,166 @@ +package devices + +import ( + "encoding/json" + "fmt" + "os" + "path/filepath" + "sort" + "sync" + "time" +) + +// VFHealthRecord tracks a virtual function quarantined after a wedge +// conviction. A quarantined VF is excluded from vGPU placement until the +// parent GPU is SR-IOV cycled and the record cleared. +type VFHealthRecord struct { + VFAddress string `json:"vf_address"` + InstanceID string `json:"instance_id,omitempty"` + SentinelLine string `json:"sentinel_line,omitempty"` + WedgeCount int `json:"wedge_count"` + QuarantinedAt time.Time `json:"quarantined_at"` +} + +// VFQuarantine describes a wedge conviction to record. +type VFQuarantine struct { + VFAddress string + InstanceID string + SentinelLine string +} + +type vfHealthStore struct { + mu sync.Mutex + path string + records map[string]VFHealthRecord +} + +var vfHealth = &vfHealthStore{records: make(map[string]VFHealthRecord)} + +// initVFHealthStore points the store at its state file and loads any +// persisted quarantines. Called from NewManager during startup wiring. +func initVFHealthStore(path string) error { + vfHealth.mu.Lock() + defer vfHealth.mu.Unlock() + vfHealth.path = path + vfHealth.records = make(map[string]VFHealthRecord) + + data, err := os.ReadFile(path) + if err != nil { + if os.IsNotExist(err) { + return nil + } + return fmt.Errorf("read VF health state: %w", err) + } + var records []VFHealthRecord + if err := json.Unmarshal(data, &records); err != nil { + return fmt.Errorf("unmarshal VF health state: %w", err) + } + for _, record := range records { + vfHealth.records[record.VFAddress] = record + } + return nil +} + +// QuarantineVF records a wedge conviction for a VF and persists it. It takes +// the vGPU placement lock so a convicted VF is never concurrently selected +// for a new assignment. Repeat convictions on the same VF increment its +// wedge count and keep the original quarantine time. +func QuarantineVF(q VFQuarantine) (VFHealthRecord, error) { + var record VFHealthRecord + var err error + withVGPUPlacementLock(func() { + record, err = vfHealth.quarantine(q) + }) + return record, err +} + +// ClearVFQuarantine removes a VF's quarantine record, returning whether one +// existed. Callers clear a VF only after the parent GPU has been SR-IOV +// cycled and a verification boot came back clean. +func ClearVFQuarantine(vfAddress string) (bool, error) { + var cleared bool + var err error + withVGPUPlacementLock(func() { + cleared, err = vfHealth.clear(vfAddress) + }) + return cleared, err +} + +// QuarantinedVFs returns all quarantine records, ordered by VF address. +func QuarantinedVFs() []VFHealthRecord { + vfHealth.mu.Lock() + defer vfHealth.mu.Unlock() + records := make([]VFHealthRecord, 0, len(vfHealth.records)) + for _, record := range vfHealth.records { + records = append(records, record) + } + sort.Slice(records, func(i, j int) bool { return records[i].VFAddress < records[j].VFAddress }) + return records +} + +func (s *vfHealthStore) quarantine(q VFQuarantine) (VFHealthRecord, error) { + s.mu.Lock() + defer s.mu.Unlock() + record, ok := s.records[q.VFAddress] + if !ok { + record = VFHealthRecord{ + VFAddress: q.VFAddress, + QuarantinedAt: time.Now().UTC(), + } + } + record.WedgeCount++ + record.InstanceID = q.InstanceID + record.SentinelLine = q.SentinelLine + s.records[q.VFAddress] = record + if err := s.persistLocked(); err != nil { + return record, err + } + return record, nil +} + +func (s *vfHealthStore) clear(vfAddress string) (bool, error) { + s.mu.Lock() + defer s.mu.Unlock() + if _, ok := s.records[vfAddress]; !ok { + return false, nil + } + delete(s.records, vfAddress) + return true, s.persistLocked() +} + +// snapshotAddresses returns the set of quarantined VF addresses. +func (s *vfHealthStore) snapshotAddresses() map[string]struct{} { + s.mu.Lock() + defer s.mu.Unlock() + addresses := make(map[string]struct{}, len(s.records)) + for address := range s.records { + addresses[address] = struct{}{} + } + return addresses +} + +func (s *vfHealthStore) persistLocked() error { + if s.path == "" { + return nil + } + records := make([]VFHealthRecord, 0, len(s.records)) + for _, record := range s.records { + records = append(records, record) + } + sort.Slice(records, func(i, j int) bool { return records[i].VFAddress < records[j].VFAddress }) + data, err := json.MarshalIndent(records, "", " ") + if err != nil { + return fmt.Errorf("marshal VF health state: %w", err) + } + if err := os.MkdirAll(filepath.Dir(s.path), 0755); err != nil { + return fmt.Errorf("create VF health state dir: %w", err) + } + tmp := s.path + ".tmp" + if err := os.WriteFile(tmp, data, 0644); err != nil { + return fmt.Errorf("write VF health state: %w", err) + } + if err := os.Rename(tmp, s.path); err != nil { + return fmt.Errorf("rename VF health state: %w", err) + } + return nil +} diff --git a/lib/devices/vf_health_darwin.go b/lib/devices/vf_health_darwin.go new file mode 100644 index 000000000..3d5db72a1 --- /dev/null +++ b/lib/devices/vf_health_darwin.go @@ -0,0 +1,7 @@ +package devices + +// withVGPUPlacementLock runs f. Vendor VFIO placement does not exist on +// darwin, so there is no placement lock to hold. +func withVGPUPlacementLock(f func()) { + f() +} diff --git a/lib/devices/vf_health_test.go b/lib/devices/vf_health_test.go new file mode 100644 index 000000000..c5a11b17a --- /dev/null +++ b/lib/devices/vf_health_test.go @@ -0,0 +1,67 @@ +package devices + +import ( + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// resetVFHealthStore points the package-level store at a fresh temp file and +// restores an empty, unpersisted store when the test finishes. +func resetVFHealthStore(t *testing.T) string { + t.Helper() + path := filepath.Join(t.TempDir(), "vf-health.json") + require.NoError(t, initVFHealthStore(path)) + t.Cleanup(func() { + vfHealth.mu.Lock() + defer vfHealth.mu.Unlock() + vfHealth.path = "" + vfHealth.records = make(map[string]VFHealthRecord) + }) + return path +} + +func TestQuarantineVFPersistsAcrossReload(t *testing.T) { + path := resetVFHealthStore(t) + + record, err := QuarantineVF(VFQuarantine{ + VFAddress: "0000:e3:00.4", + InstanceID: "instance-1", + SentinelLine: "NVRM: GPU 0000:e3:00.4: RmInitAdapter failed! (0x22:0x65:884)", + }) + require.NoError(t, err) + assert.Equal(t, 1, record.WedgeCount) + assert.False(t, record.QuarantinedAt.IsZero()) + + // A repeat conviction increments the count and keeps the original time. + again, err := QuarantineVF(VFQuarantine{VFAddress: "0000:e3:00.4", InstanceID: "instance-2"}) + require.NoError(t, err) + assert.Equal(t, 2, again.WedgeCount) + assert.Equal(t, record.QuarantinedAt, again.QuarantinedAt) + + // Reload from disk, as a hypeman restart would. + require.NoError(t, initVFHealthStore(path)) + records := QuarantinedVFs() + require.Len(t, records, 1) + assert.Equal(t, "0000:e3:00.4", records[0].VFAddress) + assert.Equal(t, 2, records[0].WedgeCount) + assert.Equal(t, "instance-2", records[0].InstanceID) +} + +func TestClearVFQuarantine(t *testing.T) { + resetVFHealthStore(t) + + _, err := QuarantineVF(VFQuarantine{VFAddress: "0000:e3:00.4"}) + require.NoError(t, err) + + cleared, err := ClearVFQuarantine("0000:e3:00.4") + require.NoError(t, err) + assert.True(t, cleared) + assert.Empty(t, QuarantinedVFs()) + + cleared, err = ClearVFQuarantine("0000:e3:00.4") + require.NoError(t, err) + assert.False(t, cleared) +} diff --git a/lib/instances/vgpu_sentinel.go b/lib/instances/vgpu_sentinel.go new file mode 100644 index 000000000..1340d582f --- /dev/null +++ b/lib/instances/vgpu_sentinel.go @@ -0,0 +1,274 @@ +package instances + +import ( + "bufio" + "context" + "errors" + "io" + "log/slog" + "os" + "path/filepath" + "regexp" + "strings" + "time" + + "github.com/kernel/hypeman/lib/devices" + "go.opentelemetry.io/otel/attribute" + "go.opentelemetry.io/otel/metric" +) + +const ( + vgpuSentinelScanInterval = 5 * time.Second + + // A burst of convictions is more likely a systemic, non-wedge init + // failure (e.g. a guest/host driver mismatch rolling out fleet-wide) + // than several independent wedges; quarantining every VF would degrade + // the host harder than the failure itself, so auto-conviction pauses. + vgpuSentinelBrakeWindow = 15 * time.Minute + vgpuSentinelBrakeLimit = 3 +) + +// vgpuSentinelPattern matches the guest NVIDIA driver's init-failure line as +// it arrives on the serial console. The full "NVRM: ... RmInitAdapter +// failed!" shape is required: the bare token also appears in echoed exec +// command lines, and the trailing (stage:status:line) tuple is +// driver-build-specific, so neither is safe to match. +var vgpuSentinelPattern = regexp.MustCompile(`NVRM: .*RmInitAdapter failed!`) + +type vgpuSentinelTarget struct { + instanceID string + vfAddress string + appLogPath string +} + +type vgpuSentinelStore interface { + listVGPUSentinelTargets(ctx context.Context) ([]vgpuSentinelTarget, error) +} + +type vgpuSentinelTail struct { + offset int64 + done bool +} + +// VGPUSentinelController scans the serial console log of vendor VFIO vGPU +// instances for the guest driver's RmInitAdapter failure line. That line is a +// positive fingerprint of a wedged VF — the driver is present, trying, and +// failing — so a match quarantines the VF, removing it from placement until +// an operator cycles the parent GPU. +type VGPUSentinelController struct { + store vgpuSentinelStore + log *slog.Logger + interval time.Duration + now func() time.Time + quarantine func(devices.VFQuarantine) (devices.VFHealthRecord, error) + convictions metric.Int64Counter + tails map[string]*vgpuSentinelTail + recent []time.Time +} + +func NewVGPUSentinelController(manager Manager, meter metric.Meter, log *slog.Logger) (*VGPUSentinelController, error) { + if manager == nil || log == nil { + return nil, nil + } + store, ok := manager.(vgpuSentinelStore) + if !ok { + return nil, nil + } + + convictions, err := meter.Int64Counter( + "hypeman_instances_vgpu_sentinel_convictions_total", + metric.WithDescription("Total wedged-VF sentinel matches by result"), + ) + if err != nil { + return nil, err + } + _, err = meter.Int64ObservableGauge( + "hypeman_instances_vgpu_quarantined_vfs", + metric.WithDescription("Number of vGPU virtual functions currently quarantined"), + metric.WithInt64Callback(func(_ context.Context, o metric.Int64Observer) error { + o.Observe(int64(len(devices.QuarantinedVFs()))) + return nil + }), + ) + if err != nil { + return nil, err + } + + return &VGPUSentinelController{ + store: store, + log: log.With("controller", "vgpu_sentinel"), + interval: vgpuSentinelScanInterval, + now: time.Now, + quarantine: devices.QuarantineVF, + convictions: convictions, + tails: make(map[string]*vgpuSentinelTail), + }, nil +} + +func (c *VGPUSentinelController) Run(ctx context.Context) error { + c.log.Info("vGPU sentinel controller started") + ticker := time.NewTicker(c.interval) + defer ticker.Stop() + for { + select { + case <-ctx.Done(): + return nil + case <-ticker.C: + c.scanOnce(ctx) + } + } +} + +func (c *VGPUSentinelController) scanOnce(ctx context.Context) { + targets, err := c.store.listVGPUSentinelTargets(ctx) + if err != nil { + c.log.WarnContext(ctx, "vGPU sentinel scan failed to list instances", "error", err) + return + } + alive := make(map[string]struct{}, len(targets)) + for _, target := range targets { + alive[target.instanceID] = struct{}{} + c.scanTarget(ctx, target) + } + for id := range c.tails { + if _, ok := alive[id]; !ok { + delete(c.tails, id) + } + } +} + +func (c *VGPUSentinelController) scanTarget(ctx context.Context, target vgpuSentinelTarget) { + tail := c.tails[target.instanceID] + if tail == nil { + tail = &vgpuSentinelTail{} + c.tails[target.instanceID] = tail + } + if tail.done { + return + } + line, found, err := scanForSentinel(target.appLogPath, &tail.offset) + if err != nil { + c.log.WarnContext(ctx, "vGPU sentinel scan failed to read app log", + "instance_id", target.instanceID, "error", err) + return + } + if !found { + return + } + tail.done = c.convict(ctx, target, line) +} + +// convict quarantines the target's VF, subject to the conviction brake. +// It reports whether scanning for this instance is finished; a failed +// quarantine leaves the tail open so the recurring sentinel retries it. +func (c *VGPUSentinelController) convict(ctx context.Context, target vgpuSentinelTarget, line string) bool { + now := c.now() + recent := c.recent[:0] + for _, t := range c.recent { + if now.Sub(t) < vgpuSentinelBrakeWindow { + recent = append(recent, t) + } + } + c.recent = recent + + if len(c.recent) >= vgpuSentinelBrakeLimit { + c.log.ErrorContext(ctx, "vGPU sentinel conviction brake engaged; VF not quarantined", + "vf", target.vfAddress, + "instance_id", target.instanceID, + "sentinel_line", line, + "convictions_in_window", len(c.recent), + "window", vgpuSentinelBrakeWindow.String(), + ) + c.convictions.Add(ctx, 1, metric.WithAttributes(attribute.String("result", "suppressed"))) + return true + } + + record, err := c.quarantine(devices.VFQuarantine{ + VFAddress: target.vfAddress, + InstanceID: target.instanceID, + SentinelLine: line, + }) + if err != nil { + c.log.ErrorContext(ctx, "failed to quarantine wedged vGPU VF", + "vf", target.vfAddress, "instance_id", target.instanceID, "error", err) + return false + } + c.recent = append(c.recent, now) + c.log.ErrorContext(ctx, "quarantined wedged vGPU VF", + "vf", target.vfAddress, + "instance_id", target.instanceID, + "sentinel_line", line, + "wedge_count", record.WedgeCount, + ) + c.convictions.Add(ctx, 1, metric.WithAttributes(attribute.String("result", "convicted"))) + return true +} + +// scanForSentinel reads complete lines from offset onward, advancing offset +// and returning the first sentinel match. A partial trailing line is left +// unconsumed for the next scan. An offset past the file size means the log +// was archived for a new boot, so the scan restarts from the top. +func scanForSentinel(path string, offset *int64) (string, bool, error) { + f, err := os.Open(path) + if err != nil { + if os.IsNotExist(err) { + return "", false, nil + } + return "", false, err + } + defer f.Close() + + info, err := f.Stat() + if err != nil { + return "", false, err + } + if info.Size() < *offset { + *offset = 0 + } + if _, err := f.Seek(*offset, io.SeekStart); err != nil { + return "", false, err + } + + reader := bufio.NewReader(f) + for { + line, err := reader.ReadString('\n') + if err != nil { + if errors.Is(err, io.EOF) { + return "", false, nil + } + return "", false, err + } + *offset += int64(len(line)) + if vgpuSentinelPattern.MatchString(line) { + return strings.TrimSpace(line), true, nil + } + } +} + +// listVGPUSentinelTargets returns instances holding a vendor VFIO vGPU +// assignment. It reads raw metadata rather than hydrating full instances: +// the scan runs continuously and deriving state would query every +// hypervisor on the host. +func (m *manager) listVGPUSentinelTargets(ctx context.Context) ([]vgpuSentinelTarget, error) { + files, err := m.listMetadataFiles() + if err != nil { + return nil, err + } + targets := make([]vgpuSentinelTarget, 0, len(files)) + for _, file := range files { + id := filepath.Base(filepath.Dir(file)) + meta, err := m.loadMetadata(id) + if err != nil { + continue + } + if meta.GPUFramework != devices.VGPUFrameworkVendorVFIO || meta.GPUDevicePath == "" { + continue + } + targets = append(targets, vgpuSentinelTarget{ + instanceID: id, + vfAddress: filepath.Base(meta.GPUDevicePath), + appLogPath: m.paths.InstanceAppLog(id), + }) + } + return targets, nil +} diff --git a/lib/instances/vgpu_sentinel_test.go b/lib/instances/vgpu_sentinel_test.go new file mode 100644 index 000000000..b1b2f7140 --- /dev/null +++ b/lib/instances/vgpu_sentinel_test.go @@ -0,0 +1,184 @@ +package instances + +import ( + "context" + "errors" + "log/slog" + "os" + "path/filepath" + "testing" + "time" + + "github.com/kernel/hypeman/lib/devices" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.opentelemetry.io/otel/metric/noop" +) + +const testSentinelLine = "[ 27.031415] NVRM: GPU 0000:e3:00.4: RmInitAdapter failed! (0x22:0x65:884)\n" + +func TestVGPUSentinelPattern(t *testing.T) { + t.Parallel() + + assert.True(t, vgpuSentinelPattern.MatchString(testSentinelLine)) + // Tuple values are driver-build-specific; the match must not depend on them. + assert.True(t, vgpuSentinelPattern.MatchString("[ 47.2] NVRM: GPU 0000:e3:00.4: RmInitAdapter failed! (0x26:0xffff:1482)")) + + // Echoed exec command lines carry the bare token on the same console. + assert.False(t, vgpuSentinelPattern.MatchString("$ dmesg | grep -c RmInitAdapter")) + assert.False(t, vgpuSentinelPattern.MatchString("[ 7.1] NVRM: loading NVIDIA UNIX Open Kernel Module for x86_64")) +} + +type fakeSentinelStore struct { + targets []vgpuSentinelTarget +} + +func (s *fakeSentinelStore) listVGPUSentinelTargets(context.Context) ([]vgpuSentinelTarget, error) { + return s.targets, nil +} + +func newTestSentinelController(t *testing.T, store vgpuSentinelStore) (*VGPUSentinelController, *[]devices.VFQuarantine) { + t.Helper() + counter, err := noop.NewMeterProvider().Meter("test").Int64Counter("test") + require.NoError(t, err) + var quarantined []devices.VFQuarantine + c := &VGPUSentinelController{ + store: store, + log: slog.New(slog.DiscardHandler), + interval: time.Hour, + now: time.Now, + quarantine: func(q devices.VFQuarantine) (devices.VFHealthRecord, error) { + quarantined = append(quarantined, q) + return devices.VFHealthRecord{VFAddress: q.VFAddress, WedgeCount: 1}, nil + }, + convictions: counter, + tails: make(map[string]*vgpuSentinelTail), + } + return c, &quarantined +} + +func TestVGPUSentinelControllerConvictsOnce(t *testing.T) { + t.Parallel() + + logPath := filepath.Join(t.TempDir(), "app.log") + store := &fakeSentinelStore{targets: []vgpuSentinelTarget{{ + instanceID: "instance-1", + vfAddress: "0000:e3:00.4", + appLogPath: logPath, + }}} + c, quarantined := newTestSentinelController(t, store) + ctx := context.Background() + + // Log missing (boot not started) and then healthy output: no conviction. + c.scanOnce(ctx) + require.NoError(t, os.WriteFile(logPath, []byte("booting\nnvidia driver loaded\n"), 0644)) + c.scanOnce(ctx) + assert.Empty(t, *quarantined) + + f, err := os.OpenFile(logPath, os.O_APPEND|os.O_WRONLY, 0644) + require.NoError(t, err) + // A partial line without its newline must not convict yet. + _, err = f.WriteString("[ 27.03] NVRM: GPU 0000:e3:00.4: RmInitAdapter failed") + require.NoError(t, err) + c.scanOnce(ctx) + assert.Empty(t, *quarantined) + + _, err = f.WriteString("! (0x22:0x65:884)\n") + require.NoError(t, err) + require.NoError(t, f.Close()) + c.scanOnce(ctx) + require.Len(t, *quarantined, 1) + assert.Equal(t, "0000:e3:00.4", (*quarantined)[0].VFAddress) + assert.Equal(t, "instance-1", (*quarantined)[0].InstanceID) + assert.Contains(t, (*quarantined)[0].SentinelLine, "RmInitAdapter failed!") + + // The sentinel recurs every ~20s; a convicted instance is not re-convicted. + appendSentinelLine(t, logPath) + c.scanOnce(ctx) + assert.Len(t, *quarantined, 1) +} + +func TestVGPUSentinelControllerRetriesFailedQuarantine(t *testing.T) { + t.Parallel() + + logPath := filepath.Join(t.TempDir(), "app.log") + require.NoError(t, os.WriteFile(logPath, []byte(testSentinelLine), 0644)) + store := &fakeSentinelStore{targets: []vgpuSentinelTarget{{ + instanceID: "instance-1", + vfAddress: "0000:e3:00.4", + appLogPath: logPath, + }}} + c, quarantined := newTestSentinelController(t, store) + quarantineErr := errors.New("persist failed") + realQuarantine := c.quarantine + c.quarantine = func(devices.VFQuarantine) (devices.VFHealthRecord, error) { + return devices.VFHealthRecord{}, quarantineErr + } + ctx := context.Background() + + c.scanOnce(ctx) + assert.Empty(t, *quarantined) + assert.False(t, c.tails["instance-1"].done) + + // The next recurrence of the sentinel retries the quarantine. + c.quarantine = realQuarantine + appendSentinelLine(t, logPath) + c.scanOnce(ctx) + assert.Len(t, *quarantined, 1) + assert.True(t, c.tails["instance-1"].done) +} + +func TestVGPUSentinelControllerBrakeSuppressesConvictionBursts(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + targets := make([]vgpuSentinelTarget, 0, vgpuSentinelBrakeLimit+1) + for i := 0; i < vgpuSentinelBrakeLimit+1; i++ { + logPath := filepath.Join(dir, string(rune('a'+i))+".log") + require.NoError(t, os.WriteFile(logPath, []byte(testSentinelLine), 0644)) + targets = append(targets, vgpuSentinelTarget{ + instanceID: "instance-" + string(rune('a'+i)), + vfAddress: "0000:e3:00." + string(rune('4'+i)), + appLogPath: logPath, + }) + } + c, quarantined := newTestSentinelController(t, &fakeSentinelStore{targets: targets}) + + c.scanOnce(context.Background()) + // The burst converts to convictions up to the limit; the rest are + // suppressed rather than quarantining the whole host. + assert.Len(t, *quarantined, vgpuSentinelBrakeLimit) + for _, target := range targets { + assert.True(t, c.tails[target.instanceID].done) + } +} + +func TestVGPUSentinelControllerDropsStaleTails(t *testing.T) { + t.Parallel() + + logPath := filepath.Join(t.TempDir(), "app.log") + require.NoError(t, os.WriteFile(logPath, []byte("booting\n"), 0644)) + store := &fakeSentinelStore{targets: []vgpuSentinelTarget{{ + instanceID: "instance-1", + vfAddress: "0000:e3:00.4", + appLogPath: logPath, + }}} + c, _ := newTestSentinelController(t, store) + ctx := context.Background() + + c.scanOnce(ctx) + require.Contains(t, c.tails, "instance-1") + + store.targets = nil + c.scanOnce(ctx) + assert.NotContains(t, c.tails, "instance-1") +} + +func appendSentinelLine(t *testing.T, path string) { + t.Helper() + f, err := os.OpenFile(path, os.O_APPEND|os.O_WRONLY, 0644) + require.NoError(t, err) + _, err = f.WriteString(testSentinelLine) + require.NoError(t, err) + require.NoError(t, f.Close()) +} diff --git a/lib/paths/paths.go b/lib/paths/paths.go index add23ff52..aff107dca 100644 --- a/lib/paths/paths.go +++ b/lib/paths/paths.go @@ -314,6 +314,11 @@ func (p *Paths) DeviceMetadata(id string) string { return filepath.Join(p.DeviceDir(id), "metadata.json") } +// VFHealthState returns the path to the persisted vGPU VF health file. +func (p *Paths) VFHealthState() string { + return filepath.Join(p.dataDir, "gpu", "vf-health.json") +} + // Volume path methods // VolumesDir returns the root volumes directory. diff --git a/lib/providers/vgpu_sentinel.go b/lib/providers/vgpu_sentinel.go new file mode 100644 index 000000000..0931f1fe6 --- /dev/null +++ b/lib/providers/vgpu_sentinel.go @@ -0,0 +1,13 @@ +package providers + +import ( + "log/slog" + + "github.com/kernel/hypeman/lib/instances" + "go.opentelemetry.io/otel" +) + +func ProvideVGPUSentinelController(instanceManager instances.Manager, log *slog.Logger) (*instances.VGPUSentinelController, error) { + meter := otel.GetMeterProvider().Meter("hypeman") + return instances.NewVGPUSentinelController(instanceManager, meter, log) +} From 1872ad127101ace41cadaacdd16ed104fd5d1104 Mon Sep 17 00:00:00 2001 From: yummybomb <19238148+yummybomb@users.noreply.github.com> Date: Thu, 20 Aug 2026 19:10:07 +0000 Subject: [PATCH 02/17] Report GPU init failure from the guest agent and harden quarantine Detection moves from host-side matching of the raw NVRM kernel line to an explicit guest-to-host report: the guest agent watches /dev/kmsg for the driver's RmInitAdapter failure and emits a HYPEMAN-GPU-INIT-FAILED marker over the established sentinel-marker channel, which the controller convicts on. The raw kernel line is no longer matched, so detection now requires the guest agent; images without it do not report. Quarantine hardening in the same pass: - The conviction brake pauses instead of drops: a suppressed conviction leaves the tail open and the agent's re-emission retries it once the window clears. - A match on an already-quarantined VF is not a new conviction: no brake accounting, no metric, no wedge-count inflation on controller restarts. - Tails reset when the instance acquires a new assignment, so a finished tail from a previous boot or VF cannot suppress scanning the next one. - A vf-health state file that fails to load refuses mutations (and retries the load) instead of letting the next conviction clobber every previously persisted quarantine. - Oversized unterminated log lines are skipped instead of re-buffered on every scan. - The controller idles on hosts without the vendor VFIO framework. GPU.md: DCGM quiesce is now an ordered step of the recovery sequence, and clearing vf-health.json documents the immediate-restart requirement. --- lib/devices/GPU.md | 55 +++++++---- lib/devices/vendor_vfio_linux_test.go | 8 +- lib/devices/vf_health.go | 81 +++++++++++----- lib/devices/vf_health_test.go | 53 +++++++++-- lib/instances/logs.go | 1 + lib/instances/vgpu_sentinel.go | 115 ++++++++++++++++------ lib/instances/vgpu_sentinel_test.go | 116 +++++++++++++++++++---- lib/system/guest_agent/gpu_watch.go | 106 +++++++++++++++++++++ lib/system/guest_agent/gpu_watch_test.go | 41 ++++++++ lib/system/guest_agent/main.go | 4 + 10 files changed, 481 insertions(+), 99 deletions(-) create mode 100644 lib/system/guest_agent/gpu_watch.go create mode 100644 lib/system/guest_agent/gpu_watch_test.go diff --git a/lib/devices/GPU.md b/lib/devices/GPU.md index c6e264b2f..1e4d38c47 100644 --- a/lib/devices/GPU.md +++ b/lib/devices/GPU.md @@ -284,22 +284,32 @@ NVRM: GPU 0000:00:03.0: RmInitAdapter failed! (0x22:0x65:884) (0x65 = timeout; the guest's init requests are never answered, and `/proc/interrupts` shows the GPU's MSI-X vectors allocated but idle). -Hypeman detects this automatically: the guest kernel writes that line to the -serial console, which lands in the instance's `logs/app.log`, and the vGPU -sentinel controller scans that file for every vendor VFIO instance. A match -quarantines the VF in `/gpu/vf-health.json` (it survives restarts): -the VF is excluded from placement and from advertised profile availability, -and its parent GPU becomes overflow-only so it drains toward the SR-IOV -cycle. The conviction is logged at error level (`quarantined wedged vGPU VF`) -and counted in `hypeman_instances_vgpu_sentinel_convictions_total`; +Hypeman detects this automatically: the guest agent watches the guest kernel +log (`/dev/kmsg`) for that line and reports it as a `HYPEMAN-GPU-INIT-FAILED` +marker — the same guest-to-host channel as the other `HYPEMAN-*` markers, +landing in the instance's `logs/app.log` — and the vGPU sentinel controller +scans that file for every vendor VFIO instance. A match quarantines the VF in +`/gpu/vf-health.json` (it survives restarts): the VF is excluded +from placement and from advertised profile availability, and its parent GPU +becomes overflow-only so it drains toward the SR-IOV cycle. The conviction is +logged at error level (`quarantined wedged vGPU VF`) and counted in +`hypeman_instances_vgpu_sentinel_convictions_total`; `hypeman_instances_vgpu_quarantined_vfs` gauges the current quarantine count. -A burst of convictions (more than 3 in 15 minutes) pauses auto-conviction, so -a systemic non-wedge init failure — e.g. a guest/host driver mismatch rolling -out — cannot quarantine the fleet. +A burst of convictions (more than 3 in 15 minutes) pauses auto-conviction — +the guest agent keeps re-emitting the marker while the failure persists, so a +paused conviction lands once the window clears — so a systemic non-wedge init +failure (e.g. a guest/host driver mismatch rolling out) cannot quarantine the +fleet before an operator sees it. + +Detection requires the hypeman guest agent: an image that skips the agent +never reports, so a wedge hit exclusively by such images stays undetected in +v1. The marker also rides a guest-writable channel — a root guest could forge +it and quarantine its own VF; the conviction brake bounds the blast radius, +and the quarantine only ever removes capacity, never touches the instance. The wedge-creating kill itself leaves no host-side log: no kernel error, no XID, no plugin crash. Detection therefore happens on the next boot that lands -on the VF, whose guest emits the sentinel ~27s after spawn. +on the VF, whose guest driver starts failing ~27s after spawn. The trigger is a SIGKILL delivered to QEMU while the vGPU plugin is still initializing the VF (roughly the first seconds after process start): @@ -316,23 +326,32 @@ External SIGKILLs (OOM killer, manual `kill -9`) can still trigger it. Confirm by assigning the same profile on a different VF: if that guest initializes, the VF is wedged, not the driver stack. Remediate by cycling SR-IOV on the parent GPU (this destroys and recreates all of its VFs, so it -requires no vGPU assignments on that GPU): +requires no vGPU assignments on that GPU). The DCGM quiesce is not optional: +with `nv-hostengine`/`dcgm-exporter` holding the GPUs open, `sriov-manage -d` +fails with `Cannot obtain unbindLock` on first contact. ```bash +# 1. Quiesce the services holding the GPU (required for the unbind lock). +systemctl stop nvidia-dcgm-exporter nvidia-dcgm + +# 2. Cycle SR-IOV on the parent GPU. /usr/lib/nvidia/sriov-manage -d /usr/lib/nvidia/sriov-manage -e + +# 3. Restart the quiesced services. +systemctl start nvidia-dcgm nvidia-dcgm-exporter ``` After the cycle, boot a verification instance on the recovered VF and confirm -its guest reaches the driver (no sentinel in its app log), then clear the -quarantine by removing the VF's entry from `/gpu/vf-health.json` -and restarting hypeman. +its guest reaches the driver (no `HYPEMAN-GPU-INIT-FAILED` in its app log), +then clear the quarantine by removing the VF's entry from +`/gpu/vf-health.json` and restarting hypeman immediately — the +running process keeps the quarantine in memory, and a conviction landing +before the restart re-persists it over your edit. Do not unbind/rebind the VF from the nvidia driver — it breaks the nvidia-vgpu-vfio core-device registration (`vfio_pci_core_device not found`) and the VF stops accepting assignments entirely until the SR-IOV cycle. -Services holding the GPU (DCGM, persistenced) must be stopped for the cycle -to obtain the unbind lock. ### vGPU assignment fails diff --git a/lib/devices/vendor_vfio_linux_test.go b/lib/devices/vendor_vfio_linux_test.go index 490348502..dae633ed7 100644 --- a/lib/devices/vendor_vfio_linux_test.go +++ b/lib/devices/vendor_vfio_linux_test.go @@ -574,7 +574,7 @@ func assertFileValue(t *testing.T, path, expected string) { func TestVendorVFIOSkipsQuarantinedVF(t *testing.T) { resetVFHealthStore(t) - _, err := QuarantineVF(VFQuarantine{VFAddress: "0000:82:00.4"}) + _, _, err := QuarantineVF(VFQuarantine{VFAddress: "0000:82:00.4"}) require.NoError(t, err) sysfs := newTestVendorVFIOSysfs(t) @@ -589,7 +589,7 @@ func TestVendorVFIOSkipsQuarantinedVF(t *testing.T) { func TestVendorVFIONoVFWhenAllQuarantined(t *testing.T) { resetVFHealthStore(t) - _, err := QuarantineVF(VFQuarantine{VFAddress: "0000:82:00.4"}) + _, _, err := QuarantineVF(VFQuarantine{VFAddress: "0000:82:00.4"}) require.NoError(t, err) sysfs := newTestVendorVFIOSysfs(t) @@ -601,7 +601,7 @@ func TestVendorVFIONoVFWhenAllQuarantined(t *testing.T) { func TestVendorVFIOCardBiasAvoidsGPUWithQuarantinedVF(t *testing.T) { resetVFHealthStore(t) - _, err := QuarantineVF(VFQuarantine{VFAddress: "0000:82:00.4"}) + _, _, err := QuarantineVF(VFQuarantine{VFAddress: "0000:82:00.4"}) require.NoError(t, err) // GPU 82 sorts first by address and has a free healthy VF, but its @@ -630,7 +630,7 @@ func TestVendorVFIOSelectUsesTiebreakAmongFreeVFs(t *testing.T) { func TestVendorVFIOListProfilesExcludesQuarantinedFromAvailability(t *testing.T) { resetVFHealthStore(t) - _, err := QuarantineVF(VFQuarantine{VFAddress: "0000:82:00.4"}) + _, _, err := QuarantineVF(VFQuarantine{VFAddress: "0000:82:00.4"}) require.NoError(t, err) sysfs := newTestVendorVFIOSysfs(t) diff --git a/lib/devices/vf_health.go b/lib/devices/vf_health.go index 1560ab0eb..e6e8e7fcb 100644 --- a/lib/devices/vf_health.go +++ b/lib/devices/vf_health.go @@ -32,6 +32,11 @@ type vfHealthStore struct { mu sync.Mutex path string records map[string]VFHealthRecord + // loadErr remembers a failed load of an existing state file. While set, + // mutations are refused: persisting the empty in-memory store would + // permanently clobber every previously persisted quarantine. Mutating + // calls retry the load first so a transient read error self-heals. + loadErr error } var vfHealth = &vfHealthStore{records: make(map[string]VFHealthRecord)} @@ -42,36 +47,62 @@ func initVFHealthStore(path string) error { vfHealth.mu.Lock() defer vfHealth.mu.Unlock() vfHealth.path = path - vfHealth.records = make(map[string]VFHealthRecord) + return vfHealth.loadLocked() +} + +func (s *vfHealthStore) loadLocked() error { + s.records = make(map[string]VFHealthRecord) + s.loadErr = nil - data, err := os.ReadFile(path) + data, err := os.ReadFile(s.path) if err != nil { if os.IsNotExist(err) { return nil } - return fmt.Errorf("read VF health state: %w", err) + s.loadErr = fmt.Errorf("read VF health state: %w", err) + return s.loadErr } var records []VFHealthRecord if err := json.Unmarshal(data, &records); err != nil { - return fmt.Errorf("unmarshal VF health state: %w", err) + s.loadErr = fmt.Errorf("unmarshal VF health state: %w", err) + return s.loadErr } for _, record := range records { - vfHealth.records[record.VFAddress] = record + s.records[record.VFAddress] = record } return nil } +// ensureLoadedLocked retries a previously failed load. Mutations must not +// proceed on a store that failed to load its existing state file. +func (s *vfHealthStore) ensureLoadedLocked() error { + if s.loadErr == nil { + return nil + } + return s.loadLocked() +} + // QuarantineVF records a wedge conviction for a VF and persists it. It takes // the vGPU placement lock so a convicted VF is never concurrently selected -// for a new assignment. Repeat convictions on the same VF increment its -// wedge count and keep the original quarantine time. -func QuarantineVF(q VFQuarantine) (VFHealthRecord, error) { +// for a new assignment. A conviction of an already-quarantined VF returns +// the existing record unchanged with existed=true — one wedge produces one +// record no matter how many victim boots report it. +func QuarantineVF(q VFQuarantine) (VFHealthRecord, bool, error) { var record VFHealthRecord + var existed bool var err error withVGPUPlacementLock(func() { - record, err = vfHealth.quarantine(q) + record, existed, err = vfHealth.quarantine(q) }) - return record, err + return record, existed, err +} + +// IsVFQuarantined reports whether a quarantine record exists for the VF. +func IsVFQuarantined(vfAddress string) bool { + vfHealth.mu.Lock() + defer vfHealth.mu.Unlock() + _, ok := vfHealth.records[vfAddress] + return ok } // ClearVFQuarantine removes a VF's quarantine record, returning whether one @@ -98,29 +129,35 @@ func QuarantinedVFs() []VFHealthRecord { return records } -func (s *vfHealthStore) quarantine(q VFQuarantine) (VFHealthRecord, error) { +func (s *vfHealthStore) quarantine(q VFQuarantine) (VFHealthRecord, bool, error) { s.mu.Lock() defer s.mu.Unlock() - record, ok := s.records[q.VFAddress] - if !ok { - record = VFHealthRecord{ - VFAddress: q.VFAddress, - QuarantinedAt: time.Now().UTC(), - } + if err := s.ensureLoadedLocked(); err != nil { + return VFHealthRecord{}, false, err + } + if record, ok := s.records[q.VFAddress]; ok { + return record, true, nil + } + record := VFHealthRecord{ + VFAddress: q.VFAddress, + InstanceID: q.InstanceID, + SentinelLine: q.SentinelLine, + WedgeCount: 1, + QuarantinedAt: time.Now().UTC(), } - record.WedgeCount++ - record.InstanceID = q.InstanceID - record.SentinelLine = q.SentinelLine s.records[q.VFAddress] = record if err := s.persistLocked(); err != nil { - return record, err + return record, false, err } - return record, nil + return record, false, nil } func (s *vfHealthStore) clear(vfAddress string) (bool, error) { s.mu.Lock() defer s.mu.Unlock() + if err := s.ensureLoadedLocked(); err != nil { + return false, err + } if _, ok := s.records[vfAddress]; !ok { return false, nil } diff --git a/lib/devices/vf_health_test.go b/lib/devices/vf_health_test.go index c5a11b17a..9e759906a 100644 --- a/lib/devices/vf_health_test.go +++ b/lib/devices/vf_health_test.go @@ -1,6 +1,7 @@ package devices import ( + "os" "path/filepath" "testing" @@ -19,6 +20,7 @@ func resetVFHealthStore(t *testing.T) string { defer vfHealth.mu.Unlock() vfHealth.path = "" vfHealth.records = make(map[string]VFHealthRecord) + vfHealth.loadErr = nil }) return path } @@ -26,42 +28,75 @@ func resetVFHealthStore(t *testing.T) string { func TestQuarantineVFPersistsAcrossReload(t *testing.T) { path := resetVFHealthStore(t) - record, err := QuarantineVF(VFQuarantine{ + record, existed, err := QuarantineVF(VFQuarantine{ VFAddress: "0000:e3:00.4", InstanceID: "instance-1", - SentinelLine: "NVRM: GPU 0000:e3:00.4: RmInitAdapter failed! (0x22:0x65:884)", + SentinelLine: "HYPEMAN-GPU-INIT-FAILED ts=2026-08-20T15:04:05Z nvrm=\"NVRM: GPU 0000:e3:00.4: RmInitAdapter failed! (0x22:0x65:884)\"", }) require.NoError(t, err) + assert.False(t, existed) assert.Equal(t, 1, record.WedgeCount) assert.False(t, record.QuarantinedAt.IsZero()) + assert.True(t, IsVFQuarantined("0000:e3:00.4")) - // A repeat conviction increments the count and keeps the original time. - again, err := QuarantineVF(VFQuarantine{VFAddress: "0000:e3:00.4", InstanceID: "instance-2"}) + // A repeat conviction (another victim boot, or a rescan after restart) + // returns the original record unchanged: one wedge, one record. + again, existed, err := QuarantineVF(VFQuarantine{VFAddress: "0000:e3:00.4", InstanceID: "instance-2"}) require.NoError(t, err) - assert.Equal(t, 2, again.WedgeCount) - assert.Equal(t, record.QuarantinedAt, again.QuarantinedAt) + assert.True(t, existed) + assert.Equal(t, record, again) // Reload from disk, as a hypeman restart would. require.NoError(t, initVFHealthStore(path)) records := QuarantinedVFs() require.Len(t, records, 1) assert.Equal(t, "0000:e3:00.4", records[0].VFAddress) - assert.Equal(t, 2, records[0].WedgeCount) - assert.Equal(t, "instance-2", records[0].InstanceID) + assert.Equal(t, 1, records[0].WedgeCount) + assert.Equal(t, "instance-1", records[0].InstanceID) } func TestClearVFQuarantine(t *testing.T) { resetVFHealthStore(t) - _, err := QuarantineVF(VFQuarantine{VFAddress: "0000:e3:00.4"}) + _, _, err := QuarantineVF(VFQuarantine{VFAddress: "0000:e3:00.4"}) require.NoError(t, err) cleared, err := ClearVFQuarantine("0000:e3:00.4") require.NoError(t, err) assert.True(t, cleared) assert.Empty(t, QuarantinedVFs()) + assert.False(t, IsVFQuarantined("0000:e3:00.4")) cleared, err = ClearVFQuarantine("0000:e3:00.4") require.NoError(t, err) assert.False(t, cleared) } + +func TestQuarantineVFRefusesToClobberUnloadedState(t *testing.T) { + path := resetVFHealthStore(t) + + _, _, err := QuarantineVF(VFQuarantine{VFAddress: "0000:e3:00.4"}) + require.NoError(t, err) + + // Corrupt the state file and reload, as a hypeman restart over a bad + // file would. The load fails and mutations must not persist the empty + // in-memory store over the previous quarantines. + require.NoError(t, os.WriteFile(path, []byte("not json"), 0644)) + require.Error(t, initVFHealthStore(path)) + + _, _, err = QuarantineVF(VFQuarantine{VFAddress: "0000:e3:00.5"}) + require.Error(t, err) + data, err := os.ReadFile(path) + require.NoError(t, err) + assert.Equal(t, "not json", string(data), "a failed load must not be overwritten by later convictions") + + // Once the file is readable again the next conviction self-heals: it + // reloads the persisted records and appends to them. + restored := `[{"vf_address":"0000:e3:00.4","wedge_count":1,"quarantined_at":"2026-08-20T00:00:00Z"}]` + require.NoError(t, os.WriteFile(path, []byte(restored), 0644)) + _, existed, err := QuarantineVF(VFQuarantine{VFAddress: "0000:e3:00.5"}) + require.NoError(t, err) + assert.False(t, existed) + assert.Len(t, QuarantinedVFs(), 2) + assert.True(t, IsVFQuarantined("0000:e3:00.4"), "reload must recover the previously persisted quarantine") +} diff --git a/lib/instances/logs.go b/lib/instances/logs.go index f10e8537d..208f21cc3 100644 --- a/lib/instances/logs.go +++ b/lib/instances/logs.go @@ -35,6 +35,7 @@ var ErrLogNotFound = fmt.Errorf("log file not found") var appLogNoiseMarkers = []string{ "HYPEMAN-PROGRAM-START", "HYPEMAN-AGENT-READY", + "HYPEMAN-GPU-INIT-FAILED", "HYPEMAN-HEADERS-START", "HYPEMAN-HEADERS-READY", "HYPEMAN-HEADERS-FAILED", diff --git a/lib/instances/vgpu_sentinel.go b/lib/instances/vgpu_sentinel.go index 1340d582f..3b45997c2 100644 --- a/lib/instances/vgpu_sentinel.go +++ b/lib/instances/vgpu_sentinel.go @@ -26,19 +26,30 @@ const ( // the host harder than the failure itself, so auto-conviction pauses. vgpuSentinelBrakeWindow = 15 * time.Minute vgpuSentinelBrakeLimit = 3 + + // vgpuSentinelMaxLineBytes bounds how much of an unterminated trailing + // line a scan will buffer and re-read. Marker lines are short; anything + // this long is console spam and is skipped once complete reading it + // becomes unreasonable. + vgpuSentinelMaxLineBytes = 64 * 1024 ) -// vgpuSentinelPattern matches the guest NVIDIA driver's init-failure line as -// it arrives on the serial console. The full "NVRM: ... RmInitAdapter -// failed!" shape is required: the bare token also appears in echoed exec -// command lines, and the trailing (stage:status:line) tuple is -// driver-build-specific, so neither is safe to match. -var vgpuSentinelPattern = regexp.MustCompile(`NVRM: .*RmInitAdapter failed!`) +// vgpuSentinelPattern matches the guest agent's HYPEMAN-GPU-INIT-FAILED +// report as it arrives on the serial console (the agent observes the NVIDIA +// driver's RmInitAdapter failure in the guest kernel log and emits this +// marker — the same guest-to-host channel as the other HYPEMAN-* markers). +// The full marker shape including the nvrm field is required: a bare token +// could appear in echoed exec command lines. +var vgpuSentinelPattern = regexp.MustCompile(`HYPEMAN-GPU-INIT-FAILED ts=\S+ nvrm="`) type vgpuSentinelTarget struct { instanceID string vfAddress string appLogPath string + // assignedAt identifies one assignment epoch: it changes whenever the + // instance acquires a vGPU, so a tail from a previous boot or a previous + // VF is never carried into the next one. + assignedAt string } type vgpuSentinelStore interface { @@ -46,8 +57,10 @@ type vgpuSentinelStore interface { } type vgpuSentinelTail struct { - offset int64 - done bool + vfAddress string + assignedAt string + offset int64 + done bool } // VGPUSentinelController scans the serial console log of vendor VFIO vGPU @@ -56,14 +69,16 @@ type vgpuSentinelTail struct { // failing — so a match quarantines the VF, removing it from placement until // an operator cycles the parent GPU. type VGPUSentinelController struct { - store vgpuSentinelStore - log *slog.Logger - interval time.Duration - now func() time.Time - quarantine func(devices.VFQuarantine) (devices.VFHealthRecord, error) - convictions metric.Int64Counter - tails map[string]*vgpuSentinelTail - recent []time.Time + store vgpuSentinelStore + log *slog.Logger + interval time.Duration + now func() time.Time + quarantine func(devices.VFQuarantine) (devices.VFHealthRecord, bool, error) + isQuarantined func(string) bool + hostFramework func() devices.VGPUFramework + convictions metric.Int64Counter + tails map[string]*vgpuSentinelTail + recent []time.Time } func NewVGPUSentinelController(manager Manager, meter metric.Meter, log *slog.Logger) (*VGPUSentinelController, error) { @@ -95,17 +110,35 @@ func NewVGPUSentinelController(manager Manager, meter metric.Meter, log *slog.Lo } return &VGPUSentinelController{ - store: store, - log: log.With("controller", "vgpu_sentinel"), - interval: vgpuSentinelScanInterval, - now: time.Now, - quarantine: devices.QuarantineVF, + store: store, + log: log.With("controller", "vgpu_sentinel"), + interval: vgpuSentinelScanInterval, + now: time.Now, + quarantine: devices.QuarantineVF, + isQuarantined: devices.IsVFQuarantined, + hostFramework: func() devices.VGPUFramework { + framework, _, err := devices.DiscoverVGPU() + if err != nil { + // Fail open: a transient discovery error must not disable + // detection on a vendor VFIO host. + return devices.VGPUFrameworkVendorVFIO + } + return framework + }, convictions: convictions, tails: make(map[string]*vgpuSentinelTail), }, nil } func (c *VGPUSentinelController) Run(ctx context.Context) error { + // Wedges only exist on vendor VFIO hosts; scanning instance metadata + // every interval on mdev and CPU-only hosts would be pure overhead. The + // framework is fixed for the lifetime of the process (SR-IOV provisioning + // precedes hypeman startup), so this is a one-time gate. + if framework := c.hostFramework(); framework != devices.VGPUFrameworkVendorVFIO { + c.log.Info("vGPU sentinel controller idle: host has no vendor VFIO framework", "framework", string(framework)) + return nil + } c.log.Info("vGPU sentinel controller started") ticker := time.NewTicker(c.interval) defer ticker.Stop() @@ -139,8 +172,11 @@ func (c *VGPUSentinelController) scanOnce(ctx context.Context) { func (c *VGPUSentinelController) scanTarget(ctx context.Context, target vgpuSentinelTarget) { tail := c.tails[target.instanceID] - if tail == nil { - tail = &vgpuSentinelTail{} + if tail == nil || tail.vfAddress != target.vfAddress || tail.assignedAt != target.assignedAt { + // A new assignment (stop/start, possibly on a different VF) writes a + // fresh log; a tail finished for the previous assignment must not + // suppress scanning the new one. + tail = &vgpuSentinelTail{vfAddress: target.vfAddress, assignedAt: target.assignedAt} c.tails[target.instanceID] = tail } if tail.done { @@ -159,9 +195,20 @@ func (c *VGPUSentinelController) scanTarget(ctx context.Context, target vgpuSent } // convict quarantines the target's VF, subject to the conviction brake. -// It reports whether scanning for this instance is finished; a failed -// quarantine leaves the tail open so the recurring sentinel retries it. +// It reports whether scanning for this instance is finished; a failed or +// brake-suppressed quarantine leaves the tail open so the recurring report +// retries it — the brake pauses auto-conviction, it must not permanently +// drop a conviction. func (c *VGPUSentinelController) convict(ctx context.Context, target vgpuSentinelTarget, line string) bool { + if c.isQuarantined(target.vfAddress) { + // Already out of placement — typically a rescan of a standing + // victim's log after a controller restart. Not a new wedge: no + // metric, no brake accounting. + c.log.InfoContext(ctx, "vGPU sentinel matched an already-quarantined VF", + "vf", target.vfAddress, "instance_id", target.instanceID) + return true + } + now := c.now() recent := c.recent[:0] for _, t := range c.recent { @@ -180,10 +227,10 @@ func (c *VGPUSentinelController) convict(ctx context.Context, target vgpuSentine "window", vgpuSentinelBrakeWindow.String(), ) c.convictions.Add(ctx, 1, metric.WithAttributes(attribute.String("result", "suppressed"))) - return true + return false } - record, err := c.quarantine(devices.VFQuarantine{ + record, existed, err := c.quarantine(devices.VFQuarantine{ VFAddress: target.vfAddress, InstanceID: target.instanceID, SentinelLine: line, @@ -193,6 +240,10 @@ func (c *VGPUSentinelController) convict(ctx context.Context, target vgpuSentine "vf", target.vfAddress, "instance_id", target.instanceID, "error", err) return false } + if existed { + // Lost a conviction race; the VF is already quarantined. + return true + } c.recent = append(c.recent, now) c.log.ErrorContext(ctx, "quarantined wedged vGPU VF", "vf", target.vfAddress, @@ -234,6 +285,11 @@ func scanForSentinel(path string, offset *int64) (string, bool, error) { line, err := reader.ReadString('\n') if err != nil { if errors.Is(err, io.EOF) { + // An oversized unterminated line would otherwise be buffered + // again on every scan; skip it once it exceeds the bound. + if len(line) > vgpuSentinelMaxLineBytes { + *offset += int64(len(line)) + } return "", false, nil } return "", false, err @@ -264,10 +320,15 @@ func (m *manager) listVGPUSentinelTargets(ctx context.Context) ([]vgpuSentinelTa if meta.GPUFramework != devices.VGPUFrameworkVendorVFIO || meta.GPUDevicePath == "" { continue } + assignedAt := "" + if meta.GPUAssignedAt != nil { + assignedAt = meta.GPUAssignedAt.UTC().Format(time.RFC3339Nano) + } targets = append(targets, vgpuSentinelTarget{ instanceID: id, vfAddress: filepath.Base(meta.GPUDevicePath), appLogPath: m.paths.InstanceAppLog(id), + assignedAt: assignedAt, }) } return targets, nil diff --git a/lib/instances/vgpu_sentinel_test.go b/lib/instances/vgpu_sentinel_test.go index b1b2f7140..d552345c9 100644 --- a/lib/instances/vgpu_sentinel_test.go +++ b/lib/instances/vgpu_sentinel_test.go @@ -15,18 +15,20 @@ import ( "go.opentelemetry.io/otel/metric/noop" ) -const testSentinelLine = "[ 27.031415] NVRM: GPU 0000:e3:00.4: RmInitAdapter failed! (0x22:0x65:884)\n" +const testSentinelLine = "2026/08/20 15:04:05 [guest-agent] HYPEMAN-GPU-INIT-FAILED ts=2026-08-20T15:04:05.123456789Z nvrm=\"NVRM: GPU 0000:e3:00.4: RmInitAdapter failed! (0x22:0x65:884)\"\n" func TestVGPUSentinelPattern(t *testing.T) { t.Parallel() assert.True(t, vgpuSentinelPattern.MatchString(testSentinelLine)) - // Tuple values are driver-build-specific; the match must not depend on them. - assert.True(t, vgpuSentinelPattern.MatchString("[ 47.2] NVRM: GPU 0000:e3:00.4: RmInitAdapter failed! (0x26:0xffff:1482)")) - // Echoed exec command lines carry the bare token on the same console. - assert.False(t, vgpuSentinelPattern.MatchString("$ dmesg | grep -c RmInitAdapter")) - assert.False(t, vgpuSentinelPattern.MatchString("[ 7.1] NVRM: loading NVIDIA UNIX Open Kernel Module for x86_64")) + // The raw guest kernel line is not the conviction signal: the guest + // agent observes it in the guest and reports the marker instead. + assert.False(t, vgpuSentinelPattern.MatchString("[ 27.031415] NVRM: GPU 0000:e3:00.4: RmInitAdapter failed! (0x22:0x65:884)")) + + // Echoed exec command lines can carry the bare token on the same console. + assert.False(t, vgpuSentinelPattern.MatchString("$ dmesg | grep -c HYPEMAN-GPU-INIT-FAILED")) + assert.False(t, vgpuSentinelPattern.MatchString("2026/08/20 15:04:05 [guest-agent] HYPEMAN-AGENT-READY ts=2026-08-20T15:04:05Z")) } type fakeSentinelStore struct { @@ -47,12 +49,13 @@ func newTestSentinelController(t *testing.T, store vgpuSentinelStore) (*VGPUSent log: slog.New(slog.DiscardHandler), interval: time.Hour, now: time.Now, - quarantine: func(q devices.VFQuarantine) (devices.VFHealthRecord, error) { + quarantine: func(q devices.VFQuarantine) (devices.VFHealthRecord, bool, error) { quarantined = append(quarantined, q) - return devices.VFHealthRecord{VFAddress: q.VFAddress, WedgeCount: 1}, nil + return devices.VFHealthRecord{VFAddress: q.VFAddress, WedgeCount: 1}, false, nil }, - convictions: counter, - tails: make(map[string]*vgpuSentinelTail), + isQuarantined: func(string) bool { return false }, + convictions: counter, + tails: make(map[string]*vgpuSentinelTail), } return c, &quarantined } @@ -78,12 +81,12 @@ func TestVGPUSentinelControllerConvictsOnce(t *testing.T) { f, err := os.OpenFile(logPath, os.O_APPEND|os.O_WRONLY, 0644) require.NoError(t, err) // A partial line without its newline must not convict yet. - _, err = f.WriteString("[ 27.03] NVRM: GPU 0000:e3:00.4: RmInitAdapter failed") + _, err = f.WriteString("2026/08/20 15:04:05 [guest-agent] HYPEMAN-GPU-INIT-FAILED ts=2026-08-20T15:04:05Z nvrm=\"NVRM: GPU 0000:e3:00.4: RmInitAdapter fail") require.NoError(t, err) c.scanOnce(ctx) assert.Empty(t, *quarantined) - _, err = f.WriteString("! (0x22:0x65:884)\n") + _, err = f.WriteString("ed! (0x22:0x65:884)\"\n") require.NoError(t, err) require.NoError(t, f.Close()) c.scanOnce(ctx) @@ -92,7 +95,8 @@ func TestVGPUSentinelControllerConvictsOnce(t *testing.T) { assert.Equal(t, "instance-1", (*quarantined)[0].InstanceID) assert.Contains(t, (*quarantined)[0].SentinelLine, "RmInitAdapter failed!") - // The sentinel recurs every ~20s; a convicted instance is not re-convicted. + // The guest agent re-emits the marker while the failure persists; a + // convicted instance is not re-convicted. appendSentinelLine(t, logPath) c.scanOnce(ctx) assert.Len(t, *quarantined, 1) @@ -111,8 +115,8 @@ func TestVGPUSentinelControllerRetriesFailedQuarantine(t *testing.T) { c, quarantined := newTestSentinelController(t, store) quarantineErr := errors.New("persist failed") realQuarantine := c.quarantine - c.quarantine = func(devices.VFQuarantine) (devices.VFHealthRecord, error) { - return devices.VFHealthRecord{}, quarantineErr + c.quarantine = func(devices.VFQuarantine) (devices.VFHealthRecord, bool, error) { + return devices.VFHealthRecord{}, false, quarantineErr } ctx := context.Background() @@ -120,7 +124,7 @@ func TestVGPUSentinelControllerRetriesFailedQuarantine(t *testing.T) { assert.Empty(t, *quarantined) assert.False(t, c.tails["instance-1"].done) - // The next recurrence of the sentinel retries the quarantine. + // The next recurrence of the marker retries the quarantine. c.quarantine = realQuarantine appendSentinelLine(t, logPath) c.scanOnce(ctx) @@ -128,7 +132,7 @@ func TestVGPUSentinelControllerRetriesFailedQuarantine(t *testing.T) { assert.True(t, c.tails["instance-1"].done) } -func TestVGPUSentinelControllerBrakeSuppressesConvictionBursts(t *testing.T) { +func TestVGPUSentinelControllerBrakePausesConvictionBursts(t *testing.T) { t.Parallel() dir := t.TempDir() @@ -143,14 +147,88 @@ func TestVGPUSentinelControllerBrakeSuppressesConvictionBursts(t *testing.T) { }) } c, quarantined := newTestSentinelController(t, &fakeSentinelStore{targets: targets}) + base := time.Now() + c.now = func() time.Time { return base } c.scanOnce(context.Background()) // The burst converts to convictions up to the limit; the rest are - // suppressed rather than quarantining the whole host. + // suppressed rather than quarantining the whole host, and their tails + // stay open — the brake pauses conviction, it does not drop it. assert.Len(t, *quarantined, vgpuSentinelBrakeLimit) + suppressed := 0 for _, target := range targets { - assert.True(t, c.tails[target.instanceID].done) + if !c.tails[target.instanceID].done { + suppressed++ + // The guest agent re-emits the marker while the failure persists. + appendSentinelLine(t, target.appLogPath) + } } + assert.Equal(t, 1, suppressed) + + // Within the window the suppression holds. + c.scanOnce(context.Background()) + assert.Len(t, *quarantined, vgpuSentinelBrakeLimit) + + // Once the window clears, the next re-emission convicts. + c.now = func() time.Time { return base.Add(vgpuSentinelBrakeWindow + time.Second) } + for _, target := range targets { + if !c.tails[target.instanceID].done { + appendSentinelLine(t, target.appLogPath) + } + } + c.scanOnce(context.Background()) + assert.Len(t, *quarantined, vgpuSentinelBrakeLimit+1) +} + +func TestVGPUSentinelControllerSkipsQuarantinedVFs(t *testing.T) { + t.Parallel() + + logPath := filepath.Join(t.TempDir(), "app.log") + require.NoError(t, os.WriteFile(logPath, []byte(testSentinelLine), 0644)) + store := &fakeSentinelStore{targets: []vgpuSentinelTarget{{ + instanceID: "instance-1", + vfAddress: "0000:e3:00.4", + appLogPath: logPath, + }}} + c, quarantined := newTestSentinelController(t, store) + c.isQuarantined = func(vf string) bool { return vf == "0000:e3:00.4" } + + // A rescan of a standing victim's log after a controller restart must + // not re-convict a persisted quarantine: no brake accounting, no metric, + // and the tail closes. + c.scanOnce(context.Background()) + assert.Empty(t, *quarantined) + assert.Empty(t, c.recent) + assert.True(t, c.tails["instance-1"].done) +} + +func TestVGPUSentinelControllerRescansNewAssignment(t *testing.T) { + t.Parallel() + + logPath := filepath.Join(t.TempDir(), "app.log") + require.NoError(t, os.WriteFile(logPath, []byte(testSentinelLine), 0644)) + store := &fakeSentinelStore{targets: []vgpuSentinelTarget{{ + instanceID: "instance-1", + vfAddress: "0000:e3:00.4", + appLogPath: logPath, + assignedAt: "2026-08-20T15:00:00Z", + }}} + c, quarantined := newTestSentinelController(t, store) + ctx := context.Background() + + c.scanOnce(ctx) + require.Len(t, *quarantined, 1) + require.True(t, c.tails["instance-1"].done) + + // A stop/start acquires a new assignment (possibly a different VF) and + // archives the log; the finished tail from the previous assignment must + // not suppress scanning the new boot. + require.NoError(t, os.WriteFile(logPath, []byte(testSentinelLine), 0644)) + store.targets[0].vfAddress = "0000:e3:00.5" + store.targets[0].assignedAt = "2026-08-20T16:00:00Z" + c.scanOnce(ctx) + require.Len(t, *quarantined, 2) + assert.Equal(t, "0000:e3:00.5", (*quarantined)[1].VFAddress) } func TestVGPUSentinelControllerDropsStaleTails(t *testing.T) { diff --git a/lib/system/guest_agent/gpu_watch.go b/lib/system/guest_agent/gpu_watch.go new file mode 100644 index 000000000..2e08df7af --- /dev/null +++ b/lib/system/guest_agent/gpu_watch.go @@ -0,0 +1,106 @@ +package main + +import ( + "bufio" + "io" + "log" + "os" + "path/filepath" + "strings" + "time" +) + +const ( + // gpuInitFailedSentinelPrefix is the guest-to-host report of a failed + // NVIDIA driver init. It rides the same channel as the other HYPEMAN-* + // markers: agent log output reaches the serial console, which the host + // persists per instance and scans. The host quarantines the vGPU VF on + // this marker, so it is only ever emitted for a kernel-log line the + // driver itself produced. + gpuInitFailedSentinelPrefix = "HYPEMAN-GPU-INIT-FAILED" + + kmsgPath = "/dev/kmsg" + nvidiaPCIVendorID = "0x10de" + + // gpuReportThrottle bounds marker emission. The driver retries init + // every ~20s on a wedged VF, and reopening /dev/kmsg replays the ring + // buffer, so without a floor a long-lived broken guest would spam the + // console. + gpuReportThrottle = 30 * time.Second + + kmsgReopenDelay = 5 * time.Second +) + +// hasNVIDIADevice reports whether any PCI function belongs to NVIDIA. A vGPU +// guest always enumerates its VF, even on a wedged slot, so a guest with no +// NVIDIA function never needs the watcher. +func hasNVIDIADevice() bool { + vendors, _ := filepath.Glob("/sys/bus/pci/devices/*/vendor") + for _, path := range vendors { + data, err := os.ReadFile(path) + if err != nil { + continue + } + if strings.TrimSpace(string(data)) == nvidiaPCIVendorID { + return true + } + } + return false +} + +// watchGPUInitFailure tails the guest kernel log for the NVIDIA driver's +// RmInitAdapter failure and reports each occurrence with a +// HYPEMAN-GPU-INIT-FAILED marker. Opening /dev/kmsg replays the ring buffer +// from the start, so failures that predate the agent are reported too. Reads +// error with EPIPE when the ring overwrites the read position; reopen and +// resume. +func watchGPUInitFailure() { + var lastReport time.Time + for { + f, err := os.Open(kmsgPath) + if err != nil { + log.Printf("[guest-agent] cannot open %s for GPU init watch: %v", kmsgPath, err) + return + } + scanKmsg(f, func(msg string) { + if time.Since(lastReport) < gpuReportThrottle { + return + } + lastReport = time.Now() + log.Printf("[guest-agent] %s ts=%s nvrm=%q", gpuInitFailedSentinelPrefix, time.Now().UTC().Format(time.RFC3339Nano), msg) + }) + _ = f.Close() + time.Sleep(kmsgReopenDelay) + } +} + +// scanKmsg reads /dev/kmsg records from r and calls report for each GPU +// init-failure message. +func scanKmsg(r io.Reader, report func(msg string)) { + reader := bufio.NewReader(r) + for { + record, err := reader.ReadString('\n') + if msg, ok := gpuInitFailureMessage(record); ok { + report(msg) + } + if err != nil { + return + } + } +} + +// gpuInitFailureMessage extracts the message from a /dev/kmsg record +// (";") and reports whether it is the NVIDIA driver's +// init-failure line. The full line shape is required — the trailing +// (stage:status:line) tuple is driver-build-specific and unsafe to match. +func gpuInitFailureMessage(record string) (string, bool) { + _, msg, found := strings.Cut(record, ";") + if !found { + return "", false + } + msg = strings.TrimSpace(msg) + if !strings.HasPrefix(msg, "NVRM:") || !strings.Contains(msg, "RmInitAdapter failed!") { + return "", false + } + return msg, true +} diff --git a/lib/system/guest_agent/gpu_watch_test.go b/lib/system/guest_agent/gpu_watch_test.go new file mode 100644 index 000000000..1481f995e --- /dev/null +++ b/lib/system/guest_agent/gpu_watch_test.go @@ -0,0 +1,41 @@ +package main + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestGPUInitFailureMessage(t *testing.T) { + msg, ok := gpuInitFailureMessage("3,1042,8462102,-;NVRM: GPU 0000:00:03.0: RmInitAdapter failed! (0x22:0x65:884)\n") + assert.True(t, ok) + assert.Equal(t, "NVRM: GPU 0000:00:03.0: RmInitAdapter failed! (0x22:0x65:884)", msg) + + // Tuple values are driver-build-specific; the match must not depend on them. + _, ok = gpuInitFailureMessage("3,1042,8462102,-;NVRM: GPU 0000:00:03.0: RmInitAdapter failed! (0x26:0xffff:1482)\n") + assert.True(t, ok) + + for _, record := range []string{ + "6,1041,8462100,-;NVRM: loading NVIDIA UNIX Open Kernel Module for x86_64\n", + "6,1043,8462110,-;nvidia-gridd: RmInitAdapter failed mentioned in userspace\n", + "no separator RmInitAdapter failed!\n", + " continuation line of a multi-line record\n", + } { + _, ok := gpuInitFailureMessage(record) + assert.False(t, ok, "record %q must not match", record) + } +} + +func TestScanKmsgReportsEachFailureRecord(t *testing.T) { + records := strings.Join([]string{ + "6,1,100,-;booting", + "3,2,200,-;NVRM: GPU 0000:00:03.0: RmInitAdapter failed! (0x22:0x65:884)", + "6,3,300,-;unrelated", + "3,4,400,-;NVRM: GPU 0000:00:03.0: RmInitAdapter failed! (0x22:0x65:884)", + }, "\n") + "\n" + + var got []string + scanKmsg(strings.NewReader(records), func(msg string) { got = append(got, msg) }) + assert.Len(t, got, 2) +} diff --git a/lib/system/guest_agent/main.go b/lib/system/guest_agent/main.go index 84fd2a5da..f726dd286 100644 --- a/lib/system/guest_agent/main.go +++ b/lib/system/guest_agent/main.go @@ -54,6 +54,10 @@ func main() { startClockKeeper() + if hasNVIDIADevice() { + go watchGPUInitFailure() + } + // Create gRPC server grpcServer := grpc.NewServer() pb.RegisterGuestServiceServer(grpcServer, &guestServer{}) From 88c4da9bbd2ee5b1b6cd36b63a1b681087eac6af Mon Sep 17 00:00:00 2001 From: yummybomb <19238148+yummybomb@users.noreply.github.com> Date: Thu, 20 Aug 2026 20:15:38 +0000 Subject: [PATCH 03/17] Close sentinel replay, fail-open, and persist-retry holes Review fixes on the quarantine layer: - start now archives the previous boot's serial log before persisting the new vGPU assignment. The sentinel keys its tail on the assignment epoch, so the old ordering let a scan replay the previous boot's wedge report against the freshly assigned VF. A failed archive is fatal for GPU instances instead of a warning. - placement and profile availability refuse to run when the VF health state file exists but cannot be loaded, instead of treating the empty in-memory set as healthy and returning every quarantined VF to rotation. The load is retried on each attempt, so a repaired file self-heals. - a conviction whose persist fails is rolled back from memory: keeping it made the next report look like a repeat conviction, ending retries with nothing on disk. - the marker pattern requires the full shape through the quoted NVRM payload; a truncated or payload-less marker echoed by an exec command no longer matches. - unreadable instance metadata in the sentinel target listing logs a warning instead of silently shrinking detection coverage. - GPU.md recovery runbook unwound a circularity: placement excludes quarantined VFs and there is no VF-pin API, so the entry is cleared before the verification boot; the sentinel re-quarantines automatically if the cycle did not cure the VF. --- lib/devices/GPU.md | 10 +++++--- lib/devices/manager.go | 2 +- lib/devices/vendor_vfio_linux.go | 10 ++++++-- lib/devices/vf_health.go | 34 +++++++++++++++--------- lib/devices/vf_health_test.go | 40 +++++++++++++++++++++++++++++ lib/instances/start.go | 17 +++++++++--- lib/instances/vgpu_sentinel.go | 12 ++++++--- lib/instances/vgpu_sentinel_test.go | 5 +++- 8 files changed, 103 insertions(+), 27 deletions(-) diff --git a/lib/devices/GPU.md b/lib/devices/GPU.md index 1e4d38c47..8d78b7057 100644 --- a/lib/devices/GPU.md +++ b/lib/devices/GPU.md @@ -342,12 +342,14 @@ systemctl stop nvidia-dcgm-exporter nvidia-dcgm systemctl start nvidia-dcgm nvidia-dcgm-exporter ``` -After the cycle, boot a verification instance on the recovered VF and confirm -its guest reaches the driver (no `HYPEMAN-GPU-INIT-FAILED` in its app log), -then clear the quarantine by removing the VF's entry from +After the cycle, clear the quarantine by removing the VF's entry from `/gpu/vf-health.json` and restarting hypeman immediately — the running process keeps the quarantine in memory, and a conviction landing -before the restart re-persists it over your edit. +before the restart re-persists it over your edit. Then boot a GPU instance as +verification: placement excludes quarantined VFs, so the recovered VF cannot +be targeted while its entry exists, and there is no VF-pin API — clearing +first is safe because the sentinel automatically re-quarantines the VF if the +cycle did not cure it (every cycle in hardware validation did). Do not unbind/rebind the VF from the nvidia driver — it breaks the nvidia-vgpu-vfio core-device registration (`vfio_pci_core_device not found`) diff --git a/lib/devices/manager.go b/lib/devices/manager.go index 50e615e76..7d44b4e92 100644 --- a/lib/devices/manager.go +++ b/lib/devices/manager.go @@ -87,7 +87,7 @@ type manager struct { // Use SetLivenessChecker after construction to enable accurate orphan detection. func NewManager(p *paths.Paths) Manager { if err := initVFHealthStore(p.VFHealthState()); err != nil { - slog.Default().Error("failed to load VF health state; persisted quarantines are not in effect", "error", err) + slog.Default().Error("failed to load VF health state; vGPU placement is disabled until the state file is repaired or removed", "error", err) } return &manager{ paths: p, diff --git a/lib/devices/vendor_vfio_linux.go b/lib/devices/vendor_vfio_linux.go index 2201ecaf5..a263dd59f 100644 --- a/lib/devices/vendor_vfio_linux.go +++ b/lib/devices/vendor_vfio_linux.go @@ -112,7 +112,10 @@ func (s vendorVFIOSysfs) discoverVFs() ([]VirtualFunction, error) { // available_instances. This is a best-effort snapshot because creating on one // VF may revoke the type from siblings that share its GPU framebuffer. func (s vendorVFIOSysfs) listProfiles(vfs []VirtualFunction) ([]GPUProfile, error) { - quarantined := vfHealth.snapshotAddresses() + quarantined, err := vfHealth.checkedAddresses() + if err != nil { + return nil, err + } profilesByType := make(map[string]profileMetadata) creatableVFs := make(map[string]int) for _, vf := range vfs { @@ -316,7 +319,10 @@ func (s vendorVFIOSysfs) reconcile(ctx context.Context, protectedDevicePaths map } func (s vendorVFIOSysfs) selectLeastLoadedVF(vfs []VirtualFunction, profileType string) (string, error) { - quarantined := vfHealth.snapshotAddresses() + quarantined, err := vfHealth.checkedAddresses() + if err != nil { + return "", err + } usageByGPU := make(map[string]int) unknownUsageByGPU := make(map[string]bool) quarantinedByGPU := make(map[string]int) diff --git a/lib/devices/vf_health.go b/lib/devices/vf_health.go index e6e8e7fcb..b3e88d736 100644 --- a/lib/devices/vf_health.go +++ b/lib/devices/vf_health.go @@ -82,6 +82,23 @@ func (s *vfHealthStore) ensureLoadedLocked() error { return s.loadLocked() } +// checkedAddresses returns the quarantined VF addresses, retrying a failed +// load first. Placement must not run against an empty record set that only +// exists because the state file was unreadable: that would put every +// previously quarantined VF back into rotation. +func (s *vfHealthStore) checkedAddresses() (map[string]struct{}, error) { + s.mu.Lock() + defer s.mu.Unlock() + if err := s.ensureLoadedLocked(); err != nil { + return nil, fmt.Errorf("VF health state unavailable: %w", err) + } + addresses := make(map[string]struct{}, len(s.records)) + for address := range s.records { + addresses[address] = struct{}{} + } + return addresses, nil +} + // QuarantineVF records a wedge conviction for a VF and persists it. It takes // the vGPU placement lock so a convicted VF is never concurrently selected // for a new assignment. A conviction of an already-quarantined VF returns @@ -147,7 +164,11 @@ func (s *vfHealthStore) quarantine(q VFQuarantine) (VFHealthRecord, bool, error) } s.records[q.VFAddress] = record if err := s.persistLocked(); err != nil { - return record, false, err + // A quarantine is only real once it is on disk. Keeping the record in + // memory would make the next report look like a repeat conviction and + // end retries, leaving nothing persisted for the next restart. + delete(s.records, q.VFAddress) + return VFHealthRecord{}, false, err } return record, false, nil } @@ -165,17 +186,6 @@ func (s *vfHealthStore) clear(vfAddress string) (bool, error) { return true, s.persistLocked() } -// snapshotAddresses returns the set of quarantined VF addresses. -func (s *vfHealthStore) snapshotAddresses() map[string]struct{} { - s.mu.Lock() - defer s.mu.Unlock() - addresses := make(map[string]struct{}, len(s.records)) - for address := range s.records { - addresses[address] = struct{}{} - } - return addresses -} - func (s *vfHealthStore) persistLocked() error { if s.path == "" { return nil diff --git a/lib/devices/vf_health_test.go b/lib/devices/vf_health_test.go index 9e759906a..e7a06e736 100644 --- a/lib/devices/vf_health_test.go +++ b/lib/devices/vf_health_test.go @@ -72,6 +72,46 @@ func TestClearVFQuarantine(t *testing.T) { assert.False(t, cleared) } +func TestQuarantineVFRollsBackOnPersistFailure(t *testing.T) { + resetVFHealthStore(t) + // Point the store below a path component that is a file, so persisting + // fails at MkdirAll. + blocker := filepath.Join(t.TempDir(), "blocker") + require.NoError(t, os.WriteFile(blocker, nil, 0644)) + vfHealth.path = filepath.Join(blocker, "vf-health.json") + + _, _, err := QuarantineVF(VFQuarantine{VFAddress: "0000:e3:00.4"}) + require.Error(t, err) + + // A quarantine is only real once persisted: the failed record must not + // linger in memory, or the retried conviction would be treated as a + // repeat and never reach disk. + assert.False(t, IsVFQuarantined("0000:e3:00.4")) + assert.Empty(t, QuarantinedVFs()) +} + +func TestCheckedAddressesFailsClosedOnUnloadedState(t *testing.T) { + path := resetVFHealthStore(t) + + _, _, err := QuarantineVF(VFQuarantine{VFAddress: "0000:e3:00.4"}) + require.NoError(t, err) + + require.NoError(t, os.WriteFile(path, []byte("not json"), 0644)) + require.Error(t, initVFHealthStore(path)) + + // Placement must not run against a record set that is only empty because + // the state file was unreadable. + _, err = vfHealth.checkedAddresses() + require.Error(t, err) + + // A repaired file self-heals on the next placement attempt. + restored := `[{"vf_address":"0000:e3:00.4","wedge_count":1,"quarantined_at":"2026-08-20T00:00:00Z"}]` + require.NoError(t, os.WriteFile(path, []byte(restored), 0644)) + addresses, err := vfHealth.checkedAddresses() + require.NoError(t, err) + assert.Contains(t, addresses, "0000:e3:00.4") +} + func TestQuarantineVFRefusesToClobberUnloadedState(t *testing.T) { path := resetVFHealthStore(t) diff --git a/lib/instances/start.go b/lib/instances/start.go index c7eb85493..ed4e43fef 100644 --- a/lib/instances/start.go +++ b/lib/instances/start.go @@ -179,6 +179,19 @@ func (m *manager) startInstance( } } + // Archive the previous boot's serial log before any new vGPU assignment + // is persisted: the sentinel controller keys its tail on the assignment + // epoch, so an old log left in place would be rescanned from the top as + // the new boot's output and could replay a wedge report against the + // fresh VF. For GPU instances a failed archive is therefore fatal. + if err := m.archiveAppLogForBoot(id); err != nil { + if stored.GPUProfile != "" { + log.ErrorContext(ctx, "failed to archive app log before start", "instance_id", id, "error", err) + return nil, fmt.Errorf("archive app log before start: %w", err) + } + log.WarnContext(ctx, "failed to archive app log before start", "instance_id", id, "error", err) + } + // 4b. Recreate the vGPU if this instance had a GPU profile // Note: GPU availability was already validated in step 2b if stored.GPUProfile != "" { @@ -227,10 +240,6 @@ func (m *manager) startInstance( } configDiskSpanEnd(nil) - if err := m.archiveAppLogForBoot(id); err != nil { - log.WarnContext(ctx, "failed to archive app log before start", "instance_id", id, "error", err) - } - // 6. Start hypervisor and boot VM (reuses logic from create) bootStart := time.Now().UTC() stored.StartedAt = &bootStart diff --git a/lib/instances/vgpu_sentinel.go b/lib/instances/vgpu_sentinel.go index 3b45997c2..c3bb62f9f 100644 --- a/lib/instances/vgpu_sentinel.go +++ b/lib/instances/vgpu_sentinel.go @@ -13,6 +13,7 @@ import ( "time" "github.com/kernel/hypeman/lib/devices" + "github.com/kernel/hypeman/lib/logger" "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/metric" ) @@ -38,9 +39,9 @@ const ( // report as it arrives on the serial console (the agent observes the NVIDIA // driver's RmInitAdapter failure in the guest kernel log and emits this // marker — the same guest-to-host channel as the other HYPEMAN-* markers). -// The full marker shape including the nvrm field is required: a bare token -// could appear in echoed exec command lines. -var vgpuSentinelPattern = regexp.MustCompile(`HYPEMAN-GPU-INIT-FAILED ts=\S+ nvrm="`) +// The full marker shape through the quoted NVRM payload is required: a bare +// token or a truncated marker could appear in echoed exec command lines. +var vgpuSentinelPattern = regexp.MustCompile(`HYPEMAN-GPU-INIT-FAILED ts=\S+ nvrm="NVRM: [^"]*RmInitAdapter failed![^"]*"`) type vgpuSentinelTarget struct { instanceID string @@ -315,6 +316,11 @@ func (m *manager) listVGPUSentinelTargets(ctx context.Context) ([]vgpuSentinelTa id := filepath.Base(filepath.Dir(file)) meta, err := m.loadMetadata(id) if err != nil { + if !errors.Is(err, ErrNotFound) { + // An unreadable record removes its VF from detection; say so + // rather than silently shrinking coverage. + logger.FromContext(ctx).WarnContext(ctx, "vGPU sentinel skipping unreadable instance metadata", "instance_id", id, "error", err) + } continue } if meta.GPUFramework != devices.VGPUFrameworkVendorVFIO || meta.GPUDevicePath == "" { diff --git a/lib/instances/vgpu_sentinel_test.go b/lib/instances/vgpu_sentinel_test.go index d552345c9..8e5ba7fb5 100644 --- a/lib/instances/vgpu_sentinel_test.go +++ b/lib/instances/vgpu_sentinel_test.go @@ -26,8 +26,11 @@ func TestVGPUSentinelPattern(t *testing.T) { // agent observes it in the guest and reports the marker instead. assert.False(t, vgpuSentinelPattern.MatchString("[ 27.031415] NVRM: GPU 0000:e3:00.4: RmInitAdapter failed! (0x22:0x65:884)")) - // Echoed exec command lines can carry the bare token on the same console. + // Echoed exec command lines can carry the bare token, a truncated marker, + // or a marker-shaped line without the NVRM payload on the same console. assert.False(t, vgpuSentinelPattern.MatchString("$ dmesg | grep -c HYPEMAN-GPU-INIT-FAILED")) + assert.False(t, vgpuSentinelPattern.MatchString("$ echo 'HYPEMAN-GPU-INIT-FAILED ts=x nvrm=\"'")) + assert.False(t, vgpuSentinelPattern.MatchString("HYPEMAN-GPU-INIT-FAILED ts=2026-08-20T15:04:05Z nvrm=\"something else\"")) assert.False(t, vgpuSentinelPattern.MatchString("2026/08/20 15:04:05 [guest-agent] HYPEMAN-AGENT-READY ts=2026-08-20T15:04:05Z")) } From e5d8616fc0e40eb3442170f6b0f8d62e55480f3a Mon Sep 17 00:00:00 2001 From: yummybomb <19238148+yummybomb@users.noreply.github.com> Date: Fri, 21 Aug 2026 15:11:35 +0000 Subject: [PATCH 04/17] Match only kernel-facility kmsg records in the GPU init watch The /dev/kmsg record priority encodes facility*8+level. Kernel printk is always facility 0 and the kernel assigns userspace writers LOG_USER or higher (a facility-0 prefix is coerced to LOG_USER, verified on a live 6.12 kernel), so requiring facility 0 makes in-guest forgery of the report impossible, matching the intended kernel-records-only semantics. Also retry a failed /dev/kmsg open instead of permanently disabling the watcher for the guest's lifetime. --- lib/system/guest_agent/gpu_watch.go | 32 +++++++++++++++++++----- lib/system/guest_agent/gpu_watch_test.go | 13 ++++++++++ 2 files changed, 39 insertions(+), 6 deletions(-) diff --git a/lib/system/guest_agent/gpu_watch.go b/lib/system/guest_agent/gpu_watch.go index 2e08df7af..7a4376526 100644 --- a/lib/system/guest_agent/gpu_watch.go +++ b/lib/system/guest_agent/gpu_watch.go @@ -6,6 +6,7 @@ import ( "log" "os" "path/filepath" + "strconv" "strings" "time" ) @@ -29,6 +30,11 @@ const ( gpuReportThrottle = 30 * time.Second kmsgReopenDelay = 5 * time.Second + + // kmsgOpenRetryDelay paces reopen attempts after a failed /dev/kmsg + // open, so a guest where the open fails does not silently lose wedge + // detection for its whole lifetime. + kmsgOpenRetryDelay = time.Minute ) // hasNVIDIADevice reports whether any PCI function belongs to NVIDIA. A vGPU @@ -59,8 +65,9 @@ func watchGPUInitFailure() { for { f, err := os.Open(kmsgPath) if err != nil { - log.Printf("[guest-agent] cannot open %s for GPU init watch: %v", kmsgPath, err) - return + log.Printf("[guest-agent] cannot open %s for GPU init watch (retrying): %v", kmsgPath, err) + time.Sleep(kmsgOpenRetryDelay) + continue } scanKmsg(f, func(msg string) { if time.Since(lastReport) < gpuReportThrottle { @@ -90,14 +97,27 @@ func scanKmsg(r io.Reader, report func(msg string)) { } // gpuInitFailureMessage extracts the message from a /dev/kmsg record -// (";") and reports whether it is the NVIDIA driver's -// init-failure line. The full line shape is required — the trailing -// (stage:status:line) tuple is driver-build-specific and unsafe to match. +// (",,,;") and reports whether it is the +// NVIDIA driver's init-failure line. Only kernel records match: the priority +// field encodes facility*8+level, kernel printk is always facility 0, and +// the kernel assigns userspace /dev/kmsg writers LOG_USER or higher (a +// facility-0 prefix from userspace is coerced to LOG_USER), so a process +// inside the guest cannot forge a matching record. The full line shape is +// required — the trailing (stage:status:line) tuple is driver-build-specific +// and unsafe to match. func gpuInitFailureMessage(record string) (string, bool) { - _, msg, found := strings.Cut(record, ";") + prefix, msg, found := strings.Cut(record, ";") + if !found { + return "", false + } + priority, _, found := strings.Cut(prefix, ",") if !found { return "", false } + value, err := strconv.ParseUint(priority, 10, 32) + if err != nil || value>>3 != 0 { + return "", false + } msg = strings.TrimSpace(msg) if !strings.HasPrefix(msg, "NVRM:") || !strings.Contains(msg, "RmInitAdapter failed!") { return "", false diff --git a/lib/system/guest_agent/gpu_watch_test.go b/lib/system/guest_agent/gpu_watch_test.go index 1481f995e..3c46149d2 100644 --- a/lib/system/guest_agent/gpu_watch_test.go +++ b/lib/system/guest_agent/gpu_watch_test.go @@ -16,11 +16,24 @@ func TestGPUInitFailureMessage(t *testing.T) { _, ok = gpuInitFailureMessage("3,1042,8462102,-;NVRM: GPU 0000:00:03.0: RmInitAdapter failed! (0x26:0xffff:1482)\n") assert.True(t, ok) + // Any kernel log level matches; only the facility is load-bearing. + _, ok = gpuInitFailureMessage("4,1044,8462120,-;NVRM: GPU 0000:00:03.0: RmInitAdapter failed! (0x22:0x65:884)\n") + assert.True(t, ok) + for _, record := range []string{ "6,1041,8462100,-;NVRM: loading NVIDIA UNIX Open Kernel Module for x86_64\n", "6,1043,8462110,-;nvidia-gridd: RmInitAdapter failed mentioned in userspace\n", "no separator RmInitAdapter failed!\n", " continuation line of a multi-line record\n", + // Userspace /dev/kmsg writes carry facility LOG_USER or higher — the + // kernel coerces a facility-0 prefix to LOG_USER — so these records, + // captured from a live 6.12 kernel by writing the driver's line into + // /dev/kmsg from a root shell, must never convict: + "12,307,4250363151,-;NVRM: GPU 0000:00:03.0: RmInitAdapter failed! (0x22:0x65:884)\n", // plain write + "8,308,4250380620,-;NVRM: GPU 0000:00:03.0: RmInitAdapter failed! (0x22:0x65:884)\n", // "<0>" prefix + "9,310,5898419120,-;NVRM: GPU 0000:00:03.0: RmInitAdapter failed! (0x22:0x65:884)\n", // "<1>" prefix + "24,309,5898400480,-;NVRM: GPU 0000:00:03.0: RmInitAdapter failed! (0x22:0x65:884)\n", // "<24>" prefix (facility 3) + "x,1,100,-;NVRM: GPU 0000:00:03.0: RmInitAdapter failed! (0x22:0x65:884)\n", // malformed priority } { _, ok := gpuInitFailureMessage(record) assert.False(t, ok, "record %q must not match", record) From 4ac60df7a12bc9ee9f8cf200d29cc9888cd0aa98 Mon Sep 17 00:00:00 2001 From: yummybomb <19238148+yummybomb@users.noreply.github.com> Date: Fri, 21 Aug 2026 15:11:57 +0000 Subject: [PATCH 05/17] Bound vGPU sentinel scans to the line cap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The scan buffered each complete line whole, so guest console output could make the controller allocate line-sized buffers every pass; the 64KB cap only kept an unterminated tail from being re-read, and once such a line was skipped its late-arriving tail was parsed as a fresh line. Read through a fixed-size buffer instead: a line that overflows it cannot be a marker, so it is discarded — across scans if its newline has not arrived — without ever being held in memory, and its tail can no longer replay a marker. Rotation resets the skip state with the offset. --- lib/instances/vgpu_sentinel.go | 73 ++++++++++++++++----------- lib/instances/vgpu_sentinel_test.go | 77 +++++++++++++++++++++++++++++ 2 files changed, 122 insertions(+), 28 deletions(-) diff --git a/lib/instances/vgpu_sentinel.go b/lib/instances/vgpu_sentinel.go index c3bb62f9f..b8483a6e1 100644 --- a/lib/instances/vgpu_sentinel.go +++ b/lib/instances/vgpu_sentinel.go @@ -28,10 +28,11 @@ const ( vgpuSentinelBrakeWindow = 15 * time.Minute vgpuSentinelBrakeLimit = 3 - // vgpuSentinelMaxLineBytes bounds how much of an unterminated trailing - // line a scan will buffer and re-read. Marker lines are short; anything - // this long is console spam and is skipped once complete reading it - // becomes unreasonable. + // vgpuSentinelMaxLineBytes bounds how much of any single log line a scan + // holds in memory. The log is guest-controlled console output; marker + // lines are a few hundred bytes, so anything longer is console spam and + // is discarded — including its later-arriving tail — without ever being + // buffered whole. vgpuSentinelMaxLineBytes = 64 * 1024 ) @@ -61,7 +62,11 @@ type vgpuSentinelTail struct { vfAddress string assignedAt string offset int64 - done bool + // skippingLongLine marks that offset sits inside a line that exceeded + // vgpuSentinelMaxLineBytes; content is discarded until its newline so an + // oversized line's tail is never parsed as a fresh line. + skippingLongLine bool + done bool } // VGPUSentinelController scans the serial console log of vendor VFIO vGPU @@ -183,7 +188,7 @@ func (c *VGPUSentinelController) scanTarget(ctx context.Context, target vgpuSent if tail.done { return } - line, found, err := scanForSentinel(target.appLogPath, &tail.offset) + line, found, err := scanForSentinel(target.appLogPath, tail) if err != nil { c.log.WarnContext(ctx, "vGPU sentinel scan failed to read app log", "instance_id", target.instanceID, "error", err) @@ -256,11 +261,14 @@ func (c *VGPUSentinelController) convict(ctx context.Context, target vgpuSentine return true } -// scanForSentinel reads complete lines from offset onward, advancing offset -// and returning the first sentinel match. A partial trailing line is left -// unconsumed for the next scan. An offset past the file size means the log -// was archived for a new boot, so the scan restarts from the top. -func scanForSentinel(path string, offset *int64) (string, bool, error) { +// scanForSentinel reads complete lines from the tail's offset onward, +// advancing the offset and returning the first sentinel match. A partial +// trailing line is left unconsumed for the next scan. An offset past the +// file size means the log was archived for a new boot, so the scan restarts +// from the top. Memory is bounded by the line cap: a line that overflows the +// read buffer cannot be a marker, so it is consumed — across scans if its +// newline has not arrived yet — without ever being held whole. +func scanForSentinel(path string, tail *vgpuSentinelTail) (string, bool, error) { f, err := os.Open(path) if err != nil { if os.IsNotExist(err) { @@ -274,31 +282,40 @@ func scanForSentinel(path string, offset *int64) (string, bool, error) { if err != nil { return "", false, err } - if info.Size() < *offset { - *offset = 0 + if info.Size() < tail.offset { + tail.offset = 0 + tail.skippingLongLine = false } - if _, err := f.Seek(*offset, io.SeekStart); err != nil { + if _, err := f.Seek(tail.offset, io.SeekStart); err != nil { return "", false, err } - reader := bufio.NewReader(f) + reader := bufio.NewReaderSize(f, vgpuSentinelMaxLineBytes) for { - line, err := reader.ReadString('\n') - if err != nil { - if errors.Is(err, io.EOF) { - // An oversized unterminated line would otherwise be buffered - // again on every scan; skip it once it exceeds the bound. - if len(line) > vgpuSentinelMaxLineBytes { - *offset += int64(len(line)) - } - return "", false, nil + line, err := reader.ReadSlice('\n') + switch { + case err == nil: + tail.offset += int64(len(line)) + if tail.skippingLongLine { + tail.skippingLongLine = false + continue + } + if vgpuSentinelPattern.Match(line) { + return strings.TrimSpace(string(line)), true, nil + } + case errors.Is(err, bufio.ErrBufferFull): + tail.offset += int64(len(line)) + tail.skippingLongLine = true + case errors.Is(err, io.EOF): + // A partial trailing line stays unconsumed for the next scan, + // unless it is the tail of an oversized line being discarded. + if tail.skippingLongLine { + tail.offset += int64(len(line)) } + return "", false, nil + default: return "", false, err } - *offset += int64(len(line)) - if vgpuSentinelPattern.MatchString(line) { - return strings.TrimSpace(line), true, nil - } } } diff --git a/lib/instances/vgpu_sentinel_test.go b/lib/instances/vgpu_sentinel_test.go index 8e5ba7fb5..c9a7b37f5 100644 --- a/lib/instances/vgpu_sentinel_test.go +++ b/lib/instances/vgpu_sentinel_test.go @@ -6,6 +6,7 @@ import ( "log/slog" "os" "path/filepath" + "strings" "testing" "time" @@ -255,6 +256,82 @@ func TestVGPUSentinelControllerDropsStaleTails(t *testing.T) { assert.NotContains(t, c.tails, "instance-1") } +// The marker is a few hundred bytes, so a line that overflows the read +// buffer is guest console spam by definition: it must not convict even when +// it embeds a marker, must never be buffered whole, and its tail — arriving +// on a later scan — must not be parsed as a fresh line. +func TestScanForSentinelDiscardsOversizedLines(t *testing.T) { + t.Parallel() + + logPath := filepath.Join(t.TempDir(), "app.log") + tail := &vgpuSentinelTail{} + + // An oversized terminated line with an embedded marker: skipped whole. + huge := strings.Repeat("x", vgpuSentinelMaxLineBytes) + testSentinelLine + require.NoError(t, os.WriteFile(logPath, []byte(huge), 0644)) + line, found, err := scanForSentinel(logPath, tail) + require.NoError(t, err) + assert.False(t, found, "a marker inside an oversized line must not convict") + assert.Empty(t, line) + + // A short marker line after the oversized one still convicts. + appendSentinelLine(t, logPath) + line, found, err = scanForSentinel(logPath, tail) + require.NoError(t, err) + require.True(t, found) + assert.Contains(t, line, "RmInitAdapter failed!") +} + +func TestScanForSentinelDiscardsOversizedLineTailAcrossScans(t *testing.T) { + t.Parallel() + + logPath := filepath.Join(t.TempDir(), "app.log") + tail := &vgpuSentinelTail{} + + // An oversized line still missing its newline: the scan enters skip mode. + require.NoError(t, os.WriteFile(logPath, []byte(strings.Repeat("x", vgpuSentinelMaxLineBytes+10)), 0644)) + _, found, err := scanForSentinel(logPath, tail) + require.NoError(t, err) + assert.False(t, found) + assert.True(t, tail.skippingLongLine) + + // The line's tail arrives later carrying a marker shape; it is still the + // same oversized line, so it must be discarded, not parsed as fresh. + appendSentinelLine(t, logPath) + _, found, err = scanForSentinel(logPath, tail) + require.NoError(t, err) + assert.False(t, found, "the tail of an oversized line must not convict") + assert.False(t, tail.skippingLongLine) + + // The next genuine marker line convicts. + appendSentinelLine(t, logPath) + line, found, err := scanForSentinel(logPath, tail) + require.NoError(t, err) + require.True(t, found) + assert.Contains(t, line, "RmInitAdapter failed!") +} + +func TestScanForSentinelResetsSkipStateOnTruncatedLog(t *testing.T) { + t.Parallel() + + logPath := filepath.Join(t.TempDir(), "app.log") + tail := &vgpuSentinelTail{} + + require.NoError(t, os.WriteFile(logPath, []byte(strings.Repeat("x", vgpuSentinelMaxLineBytes+10)), 0644)) + _, _, err := scanForSentinel(logPath, tail) + require.NoError(t, err) + require.True(t, tail.skippingLongLine) + + // Rotation truncates the file under the tail; the restart from the top + // must clear the skip state or a marker in the fresh log would be lost. + require.NoError(t, os.WriteFile(logPath, []byte(testSentinelLine), 0644)) + line, found, err := scanForSentinel(logPath, tail) + require.NoError(t, err) + require.True(t, found) + assert.Contains(t, line, "RmInitAdapter failed!") + assert.False(t, tail.skippingLongLine) +} + func appendSentinelLine(t *testing.T, path string) { t.Helper() f, err := os.OpenFile(path, os.O_APPEND|os.O_WRONLY, 0644) From 1649d4998be18c69073bf4e9f7f435e2ac04ad21 Mon Sep 17 00:00:00 2001 From: yummybomb <19238148+yummybomb@users.noreply.github.com> Date: Fri, 21 Aug 2026 15:12:09 +0000 Subject: [PATCH 06/17] Signal an unavailable VF health store MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When the persisted state file fails to load, quarantine mutations are refused and vGPU placement fails closed, but the quarantined-VFs gauge reads zero from the empty in-memory set — exactly when quarantines exist and are unreadable. Export the load-failure state as its own gauge so the condition is alertable. --- lib/devices/vf_health.go | 11 +++++++++++ lib/instances/vgpu_sentinel.go | 18 ++++++++++++++++++ 2 files changed, 29 insertions(+) diff --git a/lib/devices/vf_health.go b/lib/devices/vf_health.go index b3e88d736..8fe36e049 100644 --- a/lib/devices/vf_health.go +++ b/lib/devices/vf_health.go @@ -134,6 +134,17 @@ func ClearVFQuarantine(vfAddress string) (bool, error) { return cleared, err } +// VFHealthStoreUnavailable reports whether the persisted VF health state +// failed to load. While true, quarantine mutations are refused and vGPU +// placement fails closed — and the in-memory record set is empty, so the +// quarantine gauge would otherwise read zero exactly when quarantines exist +// but are unreadable. Surface this state instead of hiding it. +func VFHealthStoreUnavailable() bool { + vfHealth.mu.Lock() + defer vfHealth.mu.Unlock() + return vfHealth.loadErr != nil +} + // QuarantinedVFs returns all quarantine records, ordered by VF address. func QuarantinedVFs() []VFHealthRecord { vfHealth.mu.Lock() diff --git a/lib/instances/vgpu_sentinel.go b/lib/instances/vgpu_sentinel.go index b8483a6e1..c8d325ff3 100644 --- a/lib/instances/vgpu_sentinel.go +++ b/lib/instances/vgpu_sentinel.go @@ -114,6 +114,24 @@ func NewVGPUSentinelController(manager Manager, meter metric.Meter, log *slog.Lo if err != nil { return nil, err } + // The quarantined-VFs gauge reads zero when the state file is unreadable + // — exactly the state where quarantines exist but are unloadable and vGPU + // placement is failing closed — so that condition gets its own signal. + _, err = meter.Int64ObservableGauge( + "hypeman_instances_vgpu_vf_health_store_unavailable", + metric.WithDescription("1 when the persisted VF health state failed to load; quarantine mutations are refused and vGPU placement is disabled until it is repaired"), + metric.WithInt64Callback(func(_ context.Context, o metric.Int64Observer) error { + if devices.VFHealthStoreUnavailable() { + o.Observe(1) + } else { + o.Observe(0) + } + return nil + }), + ) + if err != nil { + return nil, err + } return &VGPUSentinelController{ store: store, From 297dda2dde255aee7668344550d37ee5b1116846 Mon Sep 17 00:00:00 2001 From: yummybomb <19238148+yummybomb@users.noreply.github.com> Date: Fri, 21 Aug 2026 15:41:25 +0000 Subject: [PATCH 07/17] Repeat the GPU init-failure marker so printk splits cannot lose a report The serial console is a shared byte stream: kernel printk bypasses the tty buffer and can land mid-marker, and on a wedged VF the kernel is emitting NVRM errors exactly when the agent reports. A corrupted copy does not match the host's full-shape scan (deliberately, so echoed commands cannot convict), which delayed the report to the next 30s re-emission. Emit each report as three identical lines sharing one ts; the host convicts on the first intact copy and ignores the rest. --- lib/instances/vgpu_sentinel_test.go | 26 ++++++++++++++++++++++ lib/system/guest_agent/gpu_watch.go | 20 ++++++++++++++++- lib/system/guest_agent/gpu_watch_test.go | 28 ++++++++++++++++++++++++ 3 files changed, 73 insertions(+), 1 deletion(-) diff --git a/lib/instances/vgpu_sentinel_test.go b/lib/instances/vgpu_sentinel_test.go index c9a7b37f5..4a2175906 100644 --- a/lib/instances/vgpu_sentinel_test.go +++ b/lib/instances/vgpu_sentinel_test.go @@ -256,6 +256,32 @@ func TestVGPUSentinelControllerDropsStaleTails(t *testing.T) { assert.NotContains(t, c.tails, "instance-1") } +// Kernel printk shares the serial console with the agent and can split a +// marker mid-write — which is why the agent emits each report as several +// identical lines. A corrupted copy must not convict (the strict shape is +// what keeps echoed commands from convicting), and the intact repeat on the +// next line must. +func TestScanForSentinelConvictsOnIntactRepeatAfterSplitMarker(t *testing.T) { + t.Parallel() + + logPath := filepath.Join(t.TempDir(), "app.log") + split := "2026/08/20 15:04:05 [guest-agent] HYPEMAN-GPU-INIT-FAILED ts=2026-08-20T15:04:05Z nvrm=\"NVRM: GPU 0000:e3:0\n" + + "[ 27.031415] NVRM: GPU 0000:00:03.0: RmInitAdapter failed! (0x22:0x65:884)\n" + + "0.4: RmInitAdapter failed! (0x22:0x65:884)\"\n" + require.NoError(t, os.WriteFile(logPath, []byte(split), 0644)) + + tail := &vgpuSentinelTail{} + _, found, err := scanForSentinel(logPath, tail) + require.NoError(t, err) + assert.False(t, found, "neither a split marker nor the raw kernel line may convict") + + appendSentinelLine(t, logPath) + line, found, err := scanForSentinel(logPath, tail) + require.NoError(t, err) + require.True(t, found, "the intact repeat must convict") + assert.Contains(t, line, "HYPEMAN-GPU-INIT-FAILED") +} + // The marker is a few hundred bytes, so a line that overflows the read // buffer is guest console spam by definition: it must not convict even when // it embeds a marker, must never be buffered whole, and its tail — arriving diff --git a/lib/system/guest_agent/gpu_watch.go b/lib/system/guest_agent/gpu_watch.go index 7a4376526..297b0c095 100644 --- a/lib/system/guest_agent/gpu_watch.go +++ b/lib/system/guest_agent/gpu_watch.go @@ -29,6 +29,15 @@ const ( // console. gpuReportThrottle = 30 * time.Second + // gpuReportRepeats is how many identical marker lines one report emits. + // The serial console is a shared byte stream: kernel printk bypasses the + // tty buffer and can split a userspace write mid-marker, and on a wedged + // VF the kernel is emitting NVRM errors exactly when reports are sent. A + // corrupted copy does not match the host's full-shape scan, so each + // report is several identical lines — one write each — and the host + // convicts on the first intact copy, ignoring the rest. + gpuReportRepeats = 3 + kmsgReopenDelay = 5 * time.Second // kmsgOpenRetryDelay paces reopen attempts after a failed /dev/kmsg @@ -74,13 +83,22 @@ func watchGPUInitFailure() { return } lastReport = time.Now() - log.Printf("[guest-agent] %s ts=%s nvrm=%q", gpuInitFailedSentinelPrefix, time.Now().UTC().Format(time.RFC3339Nano), msg) + emitGPUInitFailureReport(msg) }) _ = f.Close() time.Sleep(kmsgReopenDelay) } } +// emitGPUInitFailureReport writes one report as gpuReportRepeats identical +// marker lines, all carrying the same ts so the host sees one report. +func emitGPUInitFailureReport(msg string) { + ts := time.Now().UTC().Format(time.RFC3339Nano) + for range gpuReportRepeats { + log.Printf("[guest-agent] %s ts=%s nvrm=%q", gpuInitFailedSentinelPrefix, ts, msg) + } +} + // scanKmsg reads /dev/kmsg records from r and calls report for each GPU // init-failure message. func scanKmsg(r io.Reader, report func(msg string)) { diff --git a/lib/system/guest_agent/gpu_watch_test.go b/lib/system/guest_agent/gpu_watch_test.go index 3c46149d2..cca106278 100644 --- a/lib/system/guest_agent/gpu_watch_test.go +++ b/lib/system/guest_agent/gpu_watch_test.go @@ -1,10 +1,13 @@ package main import ( + "bytes" + "log" "strings" "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestGPUInitFailureMessage(t *testing.T) { @@ -40,6 +43,31 @@ func TestGPUInitFailureMessage(t *testing.T) { } } +// One report is emitted as several identical marker lines because kernel +// printk shares the serial console and can split a single write mid-marker +// — and a wedged VF guarantees printk traffic at report time. Any one +// intact copy convicts; the copies share one ts so they read as one report. +func TestEmitGPUInitFailureReportRepeatsMarkerLines(t *testing.T) { + var buf bytes.Buffer + prev := log.Writer() + log.SetOutput(&buf) + defer log.SetOutput(prev) + + emitGPUInitFailureReport("NVRM: GPU 0000:00:03.0: RmInitAdapter failed! (0x22:0x65:884)") + + lines := strings.Split(strings.TrimSpace(buf.String()), "\n") + require.Len(t, lines, gpuReportRepeats) + for i, line := range lines { + assert.Contains(t, line, "HYPEMAN-GPU-INIT-FAILED ts=") + assert.Contains(t, line, `nvrm="NVRM: GPU 0000:00:03.0: RmInitAdapter failed! (0x22:0x65:884)"`) + // Identical from the marker onward: same ts, one report. + assert.Equal(t, + lines[0][strings.Index(lines[0], "HYPEMAN"):], + line[strings.Index(line, "HYPEMAN"):], + "copy %d must be identical to the first", i) + } +} + func TestScanKmsgReportsEachFailureRecord(t *testing.T) { records := strings.Join([]string{ "6,1,100,-;booting", From 09b94a55236cc141cafdf5c686ffc753f6416e00 Mon Sep 17 00:00:00 2001 From: yummybomb <19238148+yummybomb@users.noreply.github.com> Date: Fri, 21 Aug 2026 15:52:31 +0000 Subject: [PATCH 08/17] Remove the conviction brake Auto-conviction no longer pauses on a burst. Systemic non-wedge init failures (e.g. a driver-mismatch image rollout) are expected to be caught on a test host before reaching production, and the convictions counter remains the alerting signal if one gets through; the brake was extra state and logic guarding against a case the rollout process already covers. Quarantine still only removes capacity and never touches instances, and the store's fail-closed load handling is unchanged. --- lib/devices/GPU.md | 14 ++++----- lib/instances/vgpu_sentinel.go | 47 ++++------------------------- lib/instances/vgpu_sentinel_test.go | 44 ++++++--------------------- 3 files changed, 23 insertions(+), 82 deletions(-) diff --git a/lib/devices/GPU.md b/lib/devices/GPU.md index 8d78b7057..800c4b5a0 100644 --- a/lib/devices/GPU.md +++ b/lib/devices/GPU.md @@ -295,17 +295,17 @@ becomes overflow-only so it drains toward the SR-IOV cycle. The conviction is logged at error level (`quarantined wedged vGPU VF`) and counted in `hypeman_instances_vgpu_sentinel_convictions_total`; `hypeman_instances_vgpu_quarantined_vfs` gauges the current quarantine count. -A burst of convictions (more than 3 in 15 minutes) pauses auto-conviction — -the guest agent keeps re-emitting the marker while the failure persists, so a -paused conviction lands once the window clears — so a systemic non-wedge init -failure (e.g. a guest/host driver mismatch rolling out) cannot quarantine the -fleet before an operator sees it. +There is no rate limit on convictions: a systemic non-wedge init failure +(e.g. a guest/host driver mismatch) emits the same line on every VF and +would quarantine the whole host, so such changes must be validated on a +test host first, and the convictions counter is the signal to alert on if +one gets through. Detection requires the hypeman guest agent: an image that skips the agent never reports, so a wedge hit exclusively by such images stays undetected in v1. The marker also rides a guest-writable channel — a root guest could forge -it and quarantine its own VF; the conviction brake bounds the blast radius, -and the quarantine only ever removes capacity, never touches the instance. +it and quarantine the VF its own instance holds; the quarantine only ever +removes capacity, never touches the instance. The wedge-creating kill itself leaves no host-side log: no kernel error, no XID, no plugin crash. Detection therefore happens on the next boot that lands diff --git a/lib/instances/vgpu_sentinel.go b/lib/instances/vgpu_sentinel.go index c8d325ff3..e97a611fa 100644 --- a/lib/instances/vgpu_sentinel.go +++ b/lib/instances/vgpu_sentinel.go @@ -14,20 +14,12 @@ import ( "github.com/kernel/hypeman/lib/devices" "github.com/kernel/hypeman/lib/logger" - "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/metric" ) const ( vgpuSentinelScanInterval = 5 * time.Second - // A burst of convictions is more likely a systemic, non-wedge init - // failure (e.g. a guest/host driver mismatch rolling out fleet-wide) - // than several independent wedges; quarantining every VF would degrade - // the host harder than the failure itself, so auto-conviction pauses. - vgpuSentinelBrakeWindow = 15 * time.Minute - vgpuSentinelBrakeLimit = 3 - // vgpuSentinelMaxLineBytes bounds how much of any single log line a scan // holds in memory. The log is guest-controlled console output; marker // lines are a few hundred bytes, so anything longer is console spam and @@ -78,13 +70,11 @@ type VGPUSentinelController struct { store vgpuSentinelStore log *slog.Logger interval time.Duration - now func() time.Time quarantine func(devices.VFQuarantine) (devices.VFHealthRecord, bool, error) isQuarantined func(string) bool hostFramework func() devices.VGPUFramework convictions metric.Int64Counter tails map[string]*vgpuSentinelTail - recent []time.Time } func NewVGPUSentinelController(manager Manager, meter metric.Meter, log *slog.Logger) (*VGPUSentinelController, error) { @@ -98,7 +88,7 @@ func NewVGPUSentinelController(manager Manager, meter metric.Meter, log *slog.Lo convictions, err := meter.Int64Counter( "hypeman_instances_vgpu_sentinel_convictions_total", - metric.WithDescription("Total wedged-VF sentinel matches by result"), + metric.WithDescription("Total wedged-VF sentinel convictions"), ) if err != nil { return nil, err @@ -137,7 +127,6 @@ func NewVGPUSentinelController(manager Manager, meter metric.Meter, log *slog.Lo store: store, log: log.With("controller", "vgpu_sentinel"), interval: vgpuSentinelScanInterval, - now: time.Now, quarantine: devices.QuarantineVF, isQuarantined: devices.IsVFQuarantined, hostFramework: func() devices.VGPUFramework { @@ -218,42 +207,19 @@ func (c *VGPUSentinelController) scanTarget(ctx context.Context, target vgpuSent tail.done = c.convict(ctx, target, line) } -// convict quarantines the target's VF, subject to the conviction brake. -// It reports whether scanning for this instance is finished; a failed or -// brake-suppressed quarantine leaves the tail open so the recurring report -// retries it — the brake pauses auto-conviction, it must not permanently -// drop a conviction. +// convict quarantines the target's VF. It reports whether scanning for this +// instance is finished; a failed quarantine leaves the tail open so the +// recurring report retries it. func (c *VGPUSentinelController) convict(ctx context.Context, target vgpuSentinelTarget, line string) bool { if c.isQuarantined(target.vfAddress) { // Already out of placement — typically a rescan of a standing // victim's log after a controller restart. Not a new wedge: no - // metric, no brake accounting. + // metric. c.log.InfoContext(ctx, "vGPU sentinel matched an already-quarantined VF", "vf", target.vfAddress, "instance_id", target.instanceID) return true } - now := c.now() - recent := c.recent[:0] - for _, t := range c.recent { - if now.Sub(t) < vgpuSentinelBrakeWindow { - recent = append(recent, t) - } - } - c.recent = recent - - if len(c.recent) >= vgpuSentinelBrakeLimit { - c.log.ErrorContext(ctx, "vGPU sentinel conviction brake engaged; VF not quarantined", - "vf", target.vfAddress, - "instance_id", target.instanceID, - "sentinel_line", line, - "convictions_in_window", len(c.recent), - "window", vgpuSentinelBrakeWindow.String(), - ) - c.convictions.Add(ctx, 1, metric.WithAttributes(attribute.String("result", "suppressed"))) - return false - } - record, existed, err := c.quarantine(devices.VFQuarantine{ VFAddress: target.vfAddress, InstanceID: target.instanceID, @@ -268,14 +234,13 @@ func (c *VGPUSentinelController) convict(ctx context.Context, target vgpuSentine // Lost a conviction race; the VF is already quarantined. return true } - c.recent = append(c.recent, now) c.log.ErrorContext(ctx, "quarantined wedged vGPU VF", "vf", target.vfAddress, "instance_id", target.instanceID, "sentinel_line", line, "wedge_count", record.WedgeCount, ) - c.convictions.Add(ctx, 1, metric.WithAttributes(attribute.String("result", "convicted"))) + c.convictions.Add(ctx, 1) return true } diff --git a/lib/instances/vgpu_sentinel_test.go b/lib/instances/vgpu_sentinel_test.go index 4a2175906..449e441d9 100644 --- a/lib/instances/vgpu_sentinel_test.go +++ b/lib/instances/vgpu_sentinel_test.go @@ -52,7 +52,6 @@ func newTestSentinelController(t *testing.T, store vgpuSentinelStore) (*VGPUSent store: store, log: slog.New(slog.DiscardHandler), interval: time.Hour, - now: time.Now, quarantine: func(q devices.VFQuarantine) (devices.VFHealthRecord, bool, error) { quarantined = append(quarantined, q) return devices.VFHealthRecord{VFAddress: q.VFAddress, WedgeCount: 1}, false, nil @@ -136,12 +135,16 @@ func TestVGPUSentinelControllerRetriesFailedQuarantine(t *testing.T) { assert.True(t, c.tails["instance-1"].done) } -func TestVGPUSentinelControllerBrakePausesConvictionBursts(t *testing.T) { +// A burst of convictions across many instances is quarantined without any +// rate limit: systemic non-wedge failures (e.g. a driver-mismatch rollout) +// are expected to be caught on a test host before reaching production, and +// the convictions counter is the alerting signal if one gets through. +func TestVGPUSentinelControllerConvictsBursts(t *testing.T) { t.Parallel() dir := t.TempDir() - targets := make([]vgpuSentinelTarget, 0, vgpuSentinelBrakeLimit+1) - for i := 0; i < vgpuSentinelBrakeLimit+1; i++ { + targets := make([]vgpuSentinelTarget, 0, 5) + for i := 0; i < 5; i++ { logPath := filepath.Join(dir, string(rune('a'+i))+".log") require.NoError(t, os.WriteFile(logPath, []byte(testSentinelLine), 0644)) targets = append(targets, vgpuSentinelTarget{ @@ -151,37 +154,12 @@ func TestVGPUSentinelControllerBrakePausesConvictionBursts(t *testing.T) { }) } c, quarantined := newTestSentinelController(t, &fakeSentinelStore{targets: targets}) - base := time.Now() - c.now = func() time.Time { return base } c.scanOnce(context.Background()) - // The burst converts to convictions up to the limit; the rest are - // suppressed rather than quarantining the whole host, and their tails - // stay open — the brake pauses conviction, it does not drop it. - assert.Len(t, *quarantined, vgpuSentinelBrakeLimit) - suppressed := 0 + assert.Len(t, *quarantined, len(targets)) for _, target := range targets { - if !c.tails[target.instanceID].done { - suppressed++ - // The guest agent re-emits the marker while the failure persists. - appendSentinelLine(t, target.appLogPath) - } + assert.True(t, c.tails[target.instanceID].done) } - assert.Equal(t, 1, suppressed) - - // Within the window the suppression holds. - c.scanOnce(context.Background()) - assert.Len(t, *quarantined, vgpuSentinelBrakeLimit) - - // Once the window clears, the next re-emission convicts. - c.now = func() time.Time { return base.Add(vgpuSentinelBrakeWindow + time.Second) } - for _, target := range targets { - if !c.tails[target.instanceID].done { - appendSentinelLine(t, target.appLogPath) - } - } - c.scanOnce(context.Background()) - assert.Len(t, *quarantined, vgpuSentinelBrakeLimit+1) } func TestVGPUSentinelControllerSkipsQuarantinedVFs(t *testing.T) { @@ -198,11 +176,9 @@ func TestVGPUSentinelControllerSkipsQuarantinedVFs(t *testing.T) { c.isQuarantined = func(vf string) bool { return vf == "0000:e3:00.4" } // A rescan of a standing victim's log after a controller restart must - // not re-convict a persisted quarantine: no brake accounting, no metric, - // and the tail closes. + // not re-convict a persisted quarantine: no metric, and the tail closes. c.scanOnce(context.Background()) assert.Empty(t, *quarantined) - assert.Empty(t, c.recent) assert.True(t, c.tails["instance-1"].done) } From 36a064f524934f730c7a569e577d227008ec4af3 Mon Sep 17 00:00:00 2001 From: yummybomb <19238148+yummybomb@users.noreply.github.com> Date: Fri, 21 Aug 2026 16:32:03 +0000 Subject: [PATCH 09/17] Read /dev/kmsg with a record-sized buffer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Each /dev/kmsg read returns exactly one record and fails with EINVAL — without consuming the record — when the buffer is smaller. Records run up to CONSOLE_EXT_LOG_MAX (8 KiB), so bufio's default 4 KiB buffer wedged the watcher on the first oversized record: every reopen replayed the ring into the same record, silently losing all detection behind it. Size the buffer to the kernel's record bound and log non-EPIPE scan errors so a wedge is visible instead of silent. --- lib/system/guest_agent/gpu_watch.go | 27 ++++++++++++--- lib/system/guest_agent/gpu_watch_test.go | 44 +++++++++++++++++++++++- 2 files changed, 65 insertions(+), 6 deletions(-) diff --git a/lib/system/guest_agent/gpu_watch.go b/lib/system/guest_agent/gpu_watch.go index 297b0c095..daa2ae666 100644 --- a/lib/system/guest_agent/gpu_watch.go +++ b/lib/system/guest_agent/gpu_watch.go @@ -2,12 +2,14 @@ package main import ( "bufio" + "errors" "io" "log" "os" "path/filepath" "strconv" "strings" + "syscall" "time" ) @@ -40,6 +42,14 @@ const ( kmsgReopenDelay = 5 * time.Second + // kmsgRecordBufferBytes must be at least the kernel's maximum /dev/kmsg + // record size (CONSOLE_EXT_LOG_MAX, 8 KiB): each read(2) returns exactly + // one record and fails with EINVAL — without consuming the record — when + // the buffer is smaller. bufio's default 4 KiB buffer would wedge the + // watcher on the first oversized record, with every reopen replaying the + // ring back into it, silently losing all detection behind it. + kmsgRecordBufferBytes = 8192 + // kmsgOpenRetryDelay paces reopen attempts after a failed /dev/kmsg // open, so a guest where the open fails does not silently lose wedge // detection for its whole lifetime. @@ -78,7 +88,7 @@ func watchGPUInitFailure() { time.Sleep(kmsgOpenRetryDelay) continue } - scanKmsg(f, func(msg string) { + err = scanKmsg(f, func(msg string) { if time.Since(lastReport) < gpuReportThrottle { return } @@ -86,6 +96,9 @@ func watchGPUInitFailure() { emitGPUInitFailureReport(msg) }) _ = f.Close() + if err != nil && !errors.Is(err, syscall.EPIPE) { + log.Printf("[guest-agent] GPU init watch read %s failed (reopening): %v", kmsgPath, err) + } time.Sleep(kmsgReopenDelay) } } @@ -100,16 +113,20 @@ func emitGPUInitFailureReport(msg string) { } // scanKmsg reads /dev/kmsg records from r and calls report for each GPU -// init-failure message. -func scanKmsg(r io.Reader, report func(msg string)) { - reader := bufio.NewReader(r) +// init-failure message. It returns the error that ended the scan, nil on +// EOF. +func scanKmsg(r io.Reader, report func(msg string)) error { + reader := bufio.NewReaderSize(r, kmsgRecordBufferBytes) for { record, err := reader.ReadString('\n') if msg, ok := gpuInitFailureMessage(record); ok { report(msg) } if err != nil { - return + if errors.Is(err, io.EOF) { + return nil + } + return err } } } diff --git a/lib/system/guest_agent/gpu_watch_test.go b/lib/system/guest_agent/gpu_watch_test.go index cca106278..f93682a1c 100644 --- a/lib/system/guest_agent/gpu_watch_test.go +++ b/lib/system/guest_agent/gpu_watch_test.go @@ -2,8 +2,10 @@ package main import ( "bytes" + "io" "log" "strings" + "syscall" "testing" "github.com/stretchr/testify/assert" @@ -77,6 +79,46 @@ func TestScanKmsgReportsEachFailureRecord(t *testing.T) { }, "\n") + "\n" var got []string - scanKmsg(strings.NewReader(records), func(msg string) { got = append(got, msg) }) + require.NoError(t, scanKmsg(strings.NewReader(records), func(msg string) { got = append(got, msg) })) assert.Len(t, got, 2) } + +// kmsgConn mimics /dev/kmsg read(2) semantics: each read returns exactly one +// record, and a buffer smaller than the record fails with EINVAL without +// consuming it. +type kmsgConn struct { + records []string + pos int +} + +func (k *kmsgConn) Read(p []byte) (int, error) { + if k.pos >= len(k.records) { + return 0, io.EOF + } + rec := k.records[k.pos] + if len(p) < len(rec) { + return 0, syscall.EINVAL + } + k.pos++ + return copy(p, rec), nil +} + +// A record larger than bufio's default 4 KiB buffer must not wedge the scan: +// /dev/kmsg rejects a short read with EINVAL without consuming the record, +// so an undersized buffer would replay into the same record on every reopen +// and never reach a failure line behind it. +func TestScanKmsgReadsOversizedRecords(t *testing.T) { + oversized := "6,1,100,-;" + strings.Repeat("x", 5000) + "\n" + failure := "3,2,200,-;NVRM: GPU 0000:00:03.0: RmInitAdapter failed! (0x22:0x65:884)\n" + + var got []string + require.NoError(t, scanKmsg(&kmsgConn{records: []string{oversized, failure}}, + func(msg string) { got = append(got, msg) })) + assert.Len(t, got, 1) + + // A record beyond even the sized buffer surfaces the EINVAL instead of + // ending the scan silently, so the watcher logs the wedge. + huge := "6,3,300,-;" + strings.Repeat("x", kmsgRecordBufferBytes) + "\n" + err := scanKmsg(&kmsgConn{records: []string{huge}}, func(string) {}) + assert.ErrorIs(t, err, syscall.EINVAL) +} From b2ca1796cd1489b4e42cb5e182356579d1dc879b Mon Sep 17 00:00:00 2001 From: yummybomb <19238148+yummybomb@users.noreply.github.com> Date: Fri, 21 Aug 2026 16:32:03 +0000 Subject: [PATCH 10/17] Fsync VF health state writes A quarantine is only real once it is on disk, but the persist renamed without syncing the file or directory, so a host crash right after a conviction could silently drop it. Sync the temp file before the rename and the directory after, and deduplicate the sorted record listing. --- lib/devices/vf_health.go | 44 ++++++++++++++++++++++++++++++++-------- 1 file changed, 35 insertions(+), 9 deletions(-) diff --git a/lib/devices/vf_health.go b/lib/devices/vf_health.go index 8fe36e049..d19bf48a3 100644 --- a/lib/devices/vf_health.go +++ b/lib/devices/vf_health.go @@ -149,8 +149,14 @@ func VFHealthStoreUnavailable() bool { func QuarantinedVFs() []VFHealthRecord { vfHealth.mu.Lock() defer vfHealth.mu.Unlock() - records := make([]VFHealthRecord, 0, len(vfHealth.records)) - for _, record := range vfHealth.records { + return vfHealth.sortedRecordsLocked() +} + +// sortedRecordsLocked returns every quarantine record ordered by VF address. +// The caller must hold s.mu. +func (s *vfHealthStore) sortedRecordsLocked() []VFHealthRecord { + records := make([]VFHealthRecord, 0, len(s.records)) + for _, record := range s.records { records = append(records, record) } sort.Slice(records, func(i, j int) bool { return records[i].VFAddress < records[j].VFAddress }) @@ -201,12 +207,7 @@ func (s *vfHealthStore) persistLocked() error { if s.path == "" { return nil } - records := make([]VFHealthRecord, 0, len(s.records)) - for _, record := range s.records { - records = append(records, record) - } - sort.Slice(records, func(i, j int) bool { return records[i].VFAddress < records[j].VFAddress }) - data, err := json.MarshalIndent(records, "", " ") + data, err := json.MarshalIndent(s.sortedRecordsLocked(), "", " ") if err != nil { return fmt.Errorf("marshal VF health state: %w", err) } @@ -214,11 +215,36 @@ func (s *vfHealthStore) persistLocked() error { return fmt.Errorf("create VF health state dir: %w", err) } tmp := s.path + ".tmp" - if err := os.WriteFile(tmp, data, 0644); err != nil { + f, err := os.OpenFile(tmp, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0644) + if err != nil { + return fmt.Errorf("create VF health state: %w", err) + } + if _, err := f.Write(data); err != nil { + f.Close() + os.Remove(tmp) return fmt.Errorf("write VF health state: %w", err) } + // A quarantine is only real once it is on disk: sync before rename so a + // host crash cannot leave an empty or partial file where a durable + // conviction should be. + if err := f.Sync(); err != nil { + f.Close() + os.Remove(tmp) + return fmt.Errorf("sync VF health state: %w", err) + } + if err := f.Close(); err != nil { + os.Remove(tmp) + return fmt.Errorf("close VF health state: %w", err) + } if err := os.Rename(tmp, s.path); err != nil { + os.Remove(tmp) return fmt.Errorf("rename VF health state: %w", err) } + // Sync the directory so the rename itself survives a crash. Best-effort + // — a directory sync failure is not worth failing the conviction over. + if dir, err := os.Open(filepath.Dir(s.path)); err == nil { + _ = dir.Sync() + _ = dir.Close() + } return nil } From 3cdf748d8c1e17b340ea830df63d4d60b2fd6537 Mon Sep 17 00:00:00 2001 From: yummybomb <19238148+yummybomb@users.noreply.github.com> Date: Fri, 21 Aug 2026 16:32:03 +0000 Subject: [PATCH 11/17] Persist only the matched sentinel marker The matched line is guest-controlled console bytes up to the 64 KiB line cap; logging and persisting it verbatim put up to that much guest output in error logs and vf-health.json. Keep just the marker match. --- lib/instances/vgpu_sentinel.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/lib/instances/vgpu_sentinel.go b/lib/instances/vgpu_sentinel.go index e97a611fa..fe9a39bdd 100644 --- a/lib/instances/vgpu_sentinel.go +++ b/lib/instances/vgpu_sentinel.go @@ -9,7 +9,6 @@ import ( "os" "path/filepath" "regexp" - "strings" "time" "github.com/kernel/hypeman/lib/devices" @@ -245,7 +244,10 @@ func (c *VGPUSentinelController) convict(ctx context.Context, target vgpuSentine } // scanForSentinel reads complete lines from the tail's offset onward, -// advancing the offset and returning the first sentinel match. A partial +// advancing the offset and returning the first sentinel match — only the +// matched marker, not the whole line: the surrounding bytes are +// guest-controlled console output up to the line cap and do not belong in +// error logs or the persisted health state. A partial // trailing line is left unconsumed for the next scan. An offset past the // file size means the log was archived for a new boot, so the scan restarts // from the top. Memory is bounded by the line cap: a line that overflows the @@ -283,8 +285,8 @@ func scanForSentinel(path string, tail *vgpuSentinelTail) (string, bool, error) tail.skippingLongLine = false continue } - if vgpuSentinelPattern.Match(line) { - return strings.TrimSpace(string(line)), true, nil + if match := vgpuSentinelPattern.Find(line); match != nil { + return string(match), true, nil } case errors.Is(err, bufio.ErrBufferFull): tail.offset += int64(len(line)) From 9e25c907327c2265569803e27ef9f3c39cc64c7d Mon Sep 17 00:00:00 2001 From: yummybomb <19238148+yummybomb@users.noreply.github.com> Date: Fri, 21 Aug 2026 17:52:25 +0000 Subject: [PATCH 12/17] Trim unused VF health and sentinel surface MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ClearVFQuarantine had no callers — the runbook documents editing vf-health.json directly — and WedgeCount was always 1. QuarantineVF's record return and IsVFQuarantined only served tests, and the sentinel's isQuarantined pre-check duplicated what QuarantineVF already reports via existed. The hostFramework indirection wrapped a single DiscoverVGPU call nothing injected. --- lib/devices/vendor_vfio_linux_test.go | 8 ++-- lib/devices/vf_health.go | 63 +++++---------------------- lib/devices/vf_health_test.go | 52 ++++++++-------------- lib/instances/vgpu_sentinel.go | 58 ++++++++++-------------- lib/instances/vgpu_sentinel_test.go | 24 +++++----- 5 files changed, 68 insertions(+), 137 deletions(-) diff --git a/lib/devices/vendor_vfio_linux_test.go b/lib/devices/vendor_vfio_linux_test.go index dae633ed7..490348502 100644 --- a/lib/devices/vendor_vfio_linux_test.go +++ b/lib/devices/vendor_vfio_linux_test.go @@ -574,7 +574,7 @@ func assertFileValue(t *testing.T, path, expected string) { func TestVendorVFIOSkipsQuarantinedVF(t *testing.T) { resetVFHealthStore(t) - _, _, err := QuarantineVF(VFQuarantine{VFAddress: "0000:82:00.4"}) + _, err := QuarantineVF(VFQuarantine{VFAddress: "0000:82:00.4"}) require.NoError(t, err) sysfs := newTestVendorVFIOSysfs(t) @@ -589,7 +589,7 @@ func TestVendorVFIOSkipsQuarantinedVF(t *testing.T) { func TestVendorVFIONoVFWhenAllQuarantined(t *testing.T) { resetVFHealthStore(t) - _, _, err := QuarantineVF(VFQuarantine{VFAddress: "0000:82:00.4"}) + _, err := QuarantineVF(VFQuarantine{VFAddress: "0000:82:00.4"}) require.NoError(t, err) sysfs := newTestVendorVFIOSysfs(t) @@ -601,7 +601,7 @@ func TestVendorVFIONoVFWhenAllQuarantined(t *testing.T) { func TestVendorVFIOCardBiasAvoidsGPUWithQuarantinedVF(t *testing.T) { resetVFHealthStore(t) - _, _, err := QuarantineVF(VFQuarantine{VFAddress: "0000:82:00.4"}) + _, err := QuarantineVF(VFQuarantine{VFAddress: "0000:82:00.4"}) require.NoError(t, err) // GPU 82 sorts first by address and has a free healthy VF, but its @@ -630,7 +630,7 @@ func TestVendorVFIOSelectUsesTiebreakAmongFreeVFs(t *testing.T) { func TestVendorVFIOListProfilesExcludesQuarantinedFromAvailability(t *testing.T) { resetVFHealthStore(t) - _, _, err := QuarantineVF(VFQuarantine{VFAddress: "0000:82:00.4"}) + _, err := QuarantineVF(VFQuarantine{VFAddress: "0000:82:00.4"}) require.NoError(t, err) sysfs := newTestVendorVFIOSysfs(t) diff --git a/lib/devices/vf_health.go b/lib/devices/vf_health.go index d19bf48a3..891067dc3 100644 --- a/lib/devices/vf_health.go +++ b/lib/devices/vf_health.go @@ -17,7 +17,6 @@ type VFHealthRecord struct { VFAddress string `json:"vf_address"` InstanceID string `json:"instance_id,omitempty"` SentinelLine string `json:"sentinel_line,omitempty"` - WedgeCount int `json:"wedge_count"` QuarantinedAt time.Time `json:"quarantined_at"` } @@ -101,37 +100,14 @@ func (s *vfHealthStore) checkedAddresses() (map[string]struct{}, error) { // QuarantineVF records a wedge conviction for a VF and persists it. It takes // the vGPU placement lock so a convicted VF is never concurrently selected -// for a new assignment. A conviction of an already-quarantined VF returns -// the existing record unchanged with existed=true — one wedge produces one -// record no matter how many victim boots report it. -func QuarantineVF(q VFQuarantine) (VFHealthRecord, bool, error) { - var record VFHealthRecord - var existed bool - var err error +// for a new assignment. A conviction of an already-quarantined VF leaves the +// existing record unchanged and reports existed=true — one wedge produces +// one record no matter how many victim boots report it. +func QuarantineVF(q VFQuarantine) (existed bool, err error) { withVGPUPlacementLock(func() { - record, existed, err = vfHealth.quarantine(q) + existed, err = vfHealth.quarantine(q) }) - return record, existed, err -} - -// IsVFQuarantined reports whether a quarantine record exists for the VF. -func IsVFQuarantined(vfAddress string) bool { - vfHealth.mu.Lock() - defer vfHealth.mu.Unlock() - _, ok := vfHealth.records[vfAddress] - return ok -} - -// ClearVFQuarantine removes a VF's quarantine record, returning whether one -// existed. Callers clear a VF only after the parent GPU has been SR-IOV -// cycled and a verification boot came back clean. -func ClearVFQuarantine(vfAddress string) (bool, error) { - var cleared bool - var err error - withVGPUPlacementLock(func() { - cleared, err = vfHealth.clear(vfAddress) - }) - return cleared, err + return existed, err } // VFHealthStoreUnavailable reports whether the persisted VF health state @@ -163,44 +139,29 @@ func (s *vfHealthStore) sortedRecordsLocked() []VFHealthRecord { return records } -func (s *vfHealthStore) quarantine(q VFQuarantine) (VFHealthRecord, bool, error) { +func (s *vfHealthStore) quarantine(q VFQuarantine) (bool, error) { s.mu.Lock() defer s.mu.Unlock() if err := s.ensureLoadedLocked(); err != nil { - return VFHealthRecord{}, false, err + return false, err } - if record, ok := s.records[q.VFAddress]; ok { - return record, true, nil + if _, ok := s.records[q.VFAddress]; ok { + return true, nil } - record := VFHealthRecord{ + s.records[q.VFAddress] = VFHealthRecord{ VFAddress: q.VFAddress, InstanceID: q.InstanceID, SentinelLine: q.SentinelLine, - WedgeCount: 1, QuarantinedAt: time.Now().UTC(), } - s.records[q.VFAddress] = record if err := s.persistLocked(); err != nil { // A quarantine is only real once it is on disk. Keeping the record in // memory would make the next report look like a repeat conviction and // end retries, leaving nothing persisted for the next restart. delete(s.records, q.VFAddress) - return VFHealthRecord{}, false, err - } - return record, false, nil -} - -func (s *vfHealthStore) clear(vfAddress string) (bool, error) { - s.mu.Lock() - defer s.mu.Unlock() - if err := s.ensureLoadedLocked(); err != nil { return false, err } - if _, ok := s.records[vfAddress]; !ok { - return false, nil - } - delete(s.records, vfAddress) - return true, s.persistLocked() + return false, nil } func (s *vfHealthStore) persistLocked() error { diff --git a/lib/devices/vf_health_test.go b/lib/devices/vf_health_test.go index e7a06e736..b36a5b296 100644 --- a/lib/devices/vf_health_test.go +++ b/lib/devices/vf_health_test.go @@ -28,50 +28,32 @@ func resetVFHealthStore(t *testing.T) string { func TestQuarantineVFPersistsAcrossReload(t *testing.T) { path := resetVFHealthStore(t) - record, existed, err := QuarantineVF(VFQuarantine{ + existed, err := QuarantineVF(VFQuarantine{ VFAddress: "0000:e3:00.4", InstanceID: "instance-1", SentinelLine: "HYPEMAN-GPU-INIT-FAILED ts=2026-08-20T15:04:05Z nvrm=\"NVRM: GPU 0000:e3:00.4: RmInitAdapter failed! (0x22:0x65:884)\"", }) require.NoError(t, err) assert.False(t, existed) - assert.Equal(t, 1, record.WedgeCount) - assert.False(t, record.QuarantinedAt.IsZero()) - assert.True(t, IsVFQuarantined("0000:e3:00.4")) + initial := QuarantinedVFs() + require.Len(t, initial, 1) + assert.False(t, initial[0].QuarantinedAt.IsZero()) // A repeat conviction (another victim boot, or a rescan after restart) - // returns the original record unchanged: one wedge, one record. - again, existed, err := QuarantineVF(VFQuarantine{VFAddress: "0000:e3:00.4", InstanceID: "instance-2"}) + // leaves the original record unchanged: one wedge, one record. + existed, err = QuarantineVF(VFQuarantine{VFAddress: "0000:e3:00.4", InstanceID: "instance-2"}) require.NoError(t, err) assert.True(t, existed) - assert.Equal(t, record, again) + assert.Equal(t, initial, QuarantinedVFs()) // Reload from disk, as a hypeman restart would. require.NoError(t, initVFHealthStore(path)) records := QuarantinedVFs() require.Len(t, records, 1) assert.Equal(t, "0000:e3:00.4", records[0].VFAddress) - assert.Equal(t, 1, records[0].WedgeCount) assert.Equal(t, "instance-1", records[0].InstanceID) } -func TestClearVFQuarantine(t *testing.T) { - resetVFHealthStore(t) - - _, _, err := QuarantineVF(VFQuarantine{VFAddress: "0000:e3:00.4"}) - require.NoError(t, err) - - cleared, err := ClearVFQuarantine("0000:e3:00.4") - require.NoError(t, err) - assert.True(t, cleared) - assert.Empty(t, QuarantinedVFs()) - assert.False(t, IsVFQuarantined("0000:e3:00.4")) - - cleared, err = ClearVFQuarantine("0000:e3:00.4") - require.NoError(t, err) - assert.False(t, cleared) -} - func TestQuarantineVFRollsBackOnPersistFailure(t *testing.T) { resetVFHealthStore(t) // Point the store below a path component that is a file, so persisting @@ -80,20 +62,19 @@ func TestQuarantineVFRollsBackOnPersistFailure(t *testing.T) { require.NoError(t, os.WriteFile(blocker, nil, 0644)) vfHealth.path = filepath.Join(blocker, "vf-health.json") - _, _, err := QuarantineVF(VFQuarantine{VFAddress: "0000:e3:00.4"}) + _, err := QuarantineVF(VFQuarantine{VFAddress: "0000:e3:00.4"}) require.Error(t, err) // A quarantine is only real once persisted: the failed record must not // linger in memory, or the retried conviction would be treated as a // repeat and never reach disk. - assert.False(t, IsVFQuarantined("0000:e3:00.4")) assert.Empty(t, QuarantinedVFs()) } func TestCheckedAddressesFailsClosedOnUnloadedState(t *testing.T) { path := resetVFHealthStore(t) - _, _, err := QuarantineVF(VFQuarantine{VFAddress: "0000:e3:00.4"}) + _, err := QuarantineVF(VFQuarantine{VFAddress: "0000:e3:00.4"}) require.NoError(t, err) require.NoError(t, os.WriteFile(path, []byte("not json"), 0644)) @@ -105,7 +86,7 @@ func TestCheckedAddressesFailsClosedOnUnloadedState(t *testing.T) { require.Error(t, err) // A repaired file self-heals on the next placement attempt. - restored := `[{"vf_address":"0000:e3:00.4","wedge_count":1,"quarantined_at":"2026-08-20T00:00:00Z"}]` + restored := `[{"vf_address":"0000:e3:00.4","quarantined_at":"2026-08-20T00:00:00Z"}]` require.NoError(t, os.WriteFile(path, []byte(restored), 0644)) addresses, err := vfHealth.checkedAddresses() require.NoError(t, err) @@ -115,7 +96,7 @@ func TestCheckedAddressesFailsClosedOnUnloadedState(t *testing.T) { func TestQuarantineVFRefusesToClobberUnloadedState(t *testing.T) { path := resetVFHealthStore(t) - _, _, err := QuarantineVF(VFQuarantine{VFAddress: "0000:e3:00.4"}) + _, err := QuarantineVF(VFQuarantine{VFAddress: "0000:e3:00.4"}) require.NoError(t, err) // Corrupt the state file and reload, as a hypeman restart over a bad @@ -124,7 +105,7 @@ func TestQuarantineVFRefusesToClobberUnloadedState(t *testing.T) { require.NoError(t, os.WriteFile(path, []byte("not json"), 0644)) require.Error(t, initVFHealthStore(path)) - _, _, err = QuarantineVF(VFQuarantine{VFAddress: "0000:e3:00.5"}) + _, err = QuarantineVF(VFQuarantine{VFAddress: "0000:e3:00.5"}) require.Error(t, err) data, err := os.ReadFile(path) require.NoError(t, err) @@ -132,11 +113,12 @@ func TestQuarantineVFRefusesToClobberUnloadedState(t *testing.T) { // Once the file is readable again the next conviction self-heals: it // reloads the persisted records and appends to them. - restored := `[{"vf_address":"0000:e3:00.4","wedge_count":1,"quarantined_at":"2026-08-20T00:00:00Z"}]` + restored := `[{"vf_address":"0000:e3:00.4","quarantined_at":"2026-08-20T00:00:00Z"}]` require.NoError(t, os.WriteFile(path, []byte(restored), 0644)) - _, existed, err := QuarantineVF(VFQuarantine{VFAddress: "0000:e3:00.5"}) + existed, err := QuarantineVF(VFQuarantine{VFAddress: "0000:e3:00.5"}) require.NoError(t, err) assert.False(t, existed) - assert.Len(t, QuarantinedVFs(), 2) - assert.True(t, IsVFQuarantined("0000:e3:00.4"), "reload must recover the previously persisted quarantine") + records := QuarantinedVFs() + require.Len(t, records, 2, "reload must recover the previously persisted quarantine") + assert.Equal(t, "0000:e3:00.4", records[0].VFAddress) } diff --git a/lib/instances/vgpu_sentinel.go b/lib/instances/vgpu_sentinel.go index fe9a39bdd..7d6c2faac 100644 --- a/lib/instances/vgpu_sentinel.go +++ b/lib/instances/vgpu_sentinel.go @@ -66,14 +66,12 @@ type vgpuSentinelTail struct { // failing — so a match quarantines the VF, removing it from placement until // an operator cycles the parent GPU. type VGPUSentinelController struct { - store vgpuSentinelStore - log *slog.Logger - interval time.Duration - quarantine func(devices.VFQuarantine) (devices.VFHealthRecord, bool, error) - isQuarantined func(string) bool - hostFramework func() devices.VGPUFramework - convictions metric.Int64Counter - tails map[string]*vgpuSentinelTail + store vgpuSentinelStore + log *slog.Logger + interval time.Duration + quarantine func(devices.VFQuarantine) (bool, error) + convictions metric.Int64Counter + tails map[string]*vgpuSentinelTail } func NewVGPUSentinelController(manager Manager, meter metric.Meter, log *slog.Logger) (*VGPUSentinelController, error) { @@ -123,20 +121,10 @@ func NewVGPUSentinelController(manager Manager, meter metric.Meter, log *slog.Lo } return &VGPUSentinelController{ - store: store, - log: log.With("controller", "vgpu_sentinel"), - interval: vgpuSentinelScanInterval, - quarantine: devices.QuarantineVF, - isQuarantined: devices.IsVFQuarantined, - hostFramework: func() devices.VGPUFramework { - framework, _, err := devices.DiscoverVGPU() - if err != nil { - // Fail open: a transient discovery error must not disable - // detection on a vendor VFIO host. - return devices.VGPUFrameworkVendorVFIO - } - return framework - }, + store: store, + log: log.With("controller", "vgpu_sentinel"), + interval: vgpuSentinelScanInterval, + quarantine: devices.QuarantineVF, convictions: convictions, tails: make(map[string]*vgpuSentinelTail), }, nil @@ -147,7 +135,13 @@ func (c *VGPUSentinelController) Run(ctx context.Context) error { // every interval on mdev and CPU-only hosts would be pure overhead. The // framework is fixed for the lifetime of the process (SR-IOV provisioning // precedes hypeman startup), so this is a one-time gate. - if framework := c.hostFramework(); framework != devices.VGPUFrameworkVendorVFIO { + framework, _, err := devices.DiscoverVGPU() + if err != nil { + // Fail open: a transient discovery error must not disable detection + // on a vendor VFIO host. + framework = devices.VGPUFrameworkVendorVFIO + } + if framework != devices.VGPUFrameworkVendorVFIO { c.log.Info("vGPU sentinel controller idle: host has no vendor VFIO framework", "framework", string(framework)) return nil } @@ -210,16 +204,7 @@ func (c *VGPUSentinelController) scanTarget(ctx context.Context, target vgpuSent // instance is finished; a failed quarantine leaves the tail open so the // recurring report retries it. func (c *VGPUSentinelController) convict(ctx context.Context, target vgpuSentinelTarget, line string) bool { - if c.isQuarantined(target.vfAddress) { - // Already out of placement — typically a rescan of a standing - // victim's log after a controller restart. Not a new wedge: no - // metric. - c.log.InfoContext(ctx, "vGPU sentinel matched an already-quarantined VF", - "vf", target.vfAddress, "instance_id", target.instanceID) - return true - } - - record, existed, err := c.quarantine(devices.VFQuarantine{ + existed, err := c.quarantine(devices.VFQuarantine{ VFAddress: target.vfAddress, InstanceID: target.instanceID, SentinelLine: line, @@ -230,14 +215,17 @@ func (c *VGPUSentinelController) convict(ctx context.Context, target vgpuSentine return false } if existed { - // Lost a conviction race; the VF is already quarantined. + // Already out of placement — typically a rescan of a standing + // victim's log after a controller restart. Not a new wedge: no + // metric. + c.log.InfoContext(ctx, "vGPU sentinel matched an already-quarantined VF", + "vf", target.vfAddress, "instance_id", target.instanceID) return true } c.log.ErrorContext(ctx, "quarantined wedged vGPU VF", "vf", target.vfAddress, "instance_id", target.instanceID, "sentinel_line", line, - "wedge_count", record.WedgeCount, ) c.convictions.Add(ctx, 1) return true diff --git a/lib/instances/vgpu_sentinel_test.go b/lib/instances/vgpu_sentinel_test.go index 449e441d9..e0c84e556 100644 --- a/lib/instances/vgpu_sentinel_test.go +++ b/lib/instances/vgpu_sentinel_test.go @@ -52,13 +52,12 @@ func newTestSentinelController(t *testing.T, store vgpuSentinelStore) (*VGPUSent store: store, log: slog.New(slog.DiscardHandler), interval: time.Hour, - quarantine: func(q devices.VFQuarantine) (devices.VFHealthRecord, bool, error) { + quarantine: func(q devices.VFQuarantine) (bool, error) { quarantined = append(quarantined, q) - return devices.VFHealthRecord{VFAddress: q.VFAddress, WedgeCount: 1}, false, nil + return false, nil }, - isQuarantined: func(string) bool { return false }, - convictions: counter, - tails: make(map[string]*vgpuSentinelTail), + convictions: counter, + tails: make(map[string]*vgpuSentinelTail), } return c, &quarantined } @@ -118,8 +117,8 @@ func TestVGPUSentinelControllerRetriesFailedQuarantine(t *testing.T) { c, quarantined := newTestSentinelController(t, store) quarantineErr := errors.New("persist failed") realQuarantine := c.quarantine - c.quarantine = func(devices.VFQuarantine) (devices.VFHealthRecord, bool, error) { - return devices.VFHealthRecord{}, false, quarantineErr + c.quarantine = func(devices.VFQuarantine) (bool, error) { + return false, quarantineErr } ctx := context.Background() @@ -172,13 +171,14 @@ func TestVGPUSentinelControllerSkipsQuarantinedVFs(t *testing.T) { vfAddress: "0000:e3:00.4", appLogPath: logPath, }}} - c, quarantined := newTestSentinelController(t, store) - c.isQuarantined = func(vf string) bool { return vf == "0000:e3:00.4" } + c, _ := newTestSentinelController(t, store) + c.quarantine = func(devices.VFQuarantine) (bool, error) { + return true, nil + } - // A rescan of a standing victim's log after a controller restart must - // not re-convict a persisted quarantine: no metric, and the tail closes. + // A rescan of a standing victim's log after a controller restart reports + // an existing quarantine: not a new wedge, and the tail closes. c.scanOnce(context.Background()) - assert.Empty(t, *quarantined) assert.True(t, c.tails["instance-1"].done) } From a5d66bd08d5f0089e131c2c02d34b95dd8b5d377 Mon Sep 17 00:00:00 2001 From: yummybomb <19238148+yummybomb@users.noreply.github.com> Date: Fri, 21 Aug 2026 18:42:12 +0000 Subject: [PATCH 13/17] Re-probe framework discovery while the sentinel gate is unknown A transient DiscoverVGPU error at startup failed open permanently, leaving a host that is not vendor VFIO scanning instance metadata every tick for the process lifetime. Keep failing open while discovery errors, but retry the probe each tick so the controller exits once the host is known not to be vendor VFIO. --- lib/instances/vgpu_sentinel.go | 41 ++++++++++++++++++++++------- lib/instances/vgpu_sentinel_test.go | 28 ++++++++++++++++++++ 2 files changed, 60 insertions(+), 9 deletions(-) diff --git a/lib/instances/vgpu_sentinel.go b/lib/instances/vgpu_sentinel.go index 7d6c2faac..32d008831 100644 --- a/lib/instances/vgpu_sentinel.go +++ b/lib/instances/vgpu_sentinel.go @@ -72,6 +72,8 @@ type VGPUSentinelController struct { quarantine func(devices.VFQuarantine) (bool, error) convictions metric.Int64Counter tails map[string]*vgpuSentinelTail + // discoverFramework is overridden in tests; nil means devices.DiscoverVGPU. + discoverFramework func() (devices.VGPUFramework, []devices.VirtualFunction, error) } func NewVGPUSentinelController(manager Manager, meter metric.Meter, log *slog.Logger) (*VGPUSentinelController, error) { @@ -134,15 +136,12 @@ func (c *VGPUSentinelController) Run(ctx context.Context) error { // Wedges only exist on vendor VFIO hosts; scanning instance metadata // every interval on mdev and CPU-only hosts would be pure overhead. The // framework is fixed for the lifetime of the process (SR-IOV provisioning - // precedes hypeman startup), so this is a one-time gate. - framework, _, err := devices.DiscoverVGPU() - if err != nil { - // Fail open: a transient discovery error must not disable detection - // on a vendor VFIO host. - framework = devices.VGPUFrameworkVendorVFIO - } - if framework != devices.VGPUFrameworkVendorVFIO { - c.log.Info("vGPU sentinel controller idle: host has no vendor VFIO framework", "framework", string(framework)) + // precedes hypeman startup), so one successful probe settles the gate. A + // failed probe fails open — a transient discovery error must not disable + // detection on a vendor VFIO host — and is retried each tick so one bad + // probe cannot leave a non-GPU host scanning for the process lifetime. + vendor, known := c.probeVendorVFIO() + if known && !vendor { return nil } c.log.Info("vGPU sentinel controller started") @@ -153,11 +152,35 @@ func (c *VGPUSentinelController) Run(ctx context.Context) error { case <-ctx.Done(): return nil case <-ticker.C: + if !known { + if vendor, known = c.probeVendorVFIO(); known && !vendor { + return nil + } + } c.scanOnce(ctx) } } } +// probeVendorVFIO reports whether the host's vGPU framework could be +// determined and, when known, whether it is vendor VFIO. It logs the idle +// verdict so every exit path announces why the controller stopped. +func (c *VGPUSentinelController) probeVendorVFIO() (vendor, known bool) { + discover := c.discoverFramework + if discover == nil { + discover = devices.DiscoverVGPU + } + framework, _, err := discover() + if err != nil { + return false, false + } + if framework != devices.VGPUFrameworkVendorVFIO { + c.log.Info("vGPU sentinel controller idle: host has no vendor VFIO framework", "framework", string(framework)) + return false, true + } + return true, true +} + func (c *VGPUSentinelController) scanOnce(ctx context.Context) { targets, err := c.store.listVGPUSentinelTargets(ctx) if err != nil { diff --git a/lib/instances/vgpu_sentinel_test.go b/lib/instances/vgpu_sentinel_test.go index e0c84e556..a9f4d39e8 100644 --- a/lib/instances/vgpu_sentinel_test.go +++ b/lib/instances/vgpu_sentinel_test.go @@ -232,6 +232,34 @@ func TestVGPUSentinelControllerDropsStaleTails(t *testing.T) { assert.NotContains(t, c.tails, "instance-1") } +// A transient discovery error fails open — the controller scans as if the +// host were vendor VFIO — but must keep re-probing so a host that is not +// vendor VFIO stops scanning once discovery recovers. +func TestVGPUSentinelControllerRunReprobesFailedDiscovery(t *testing.T) { + t.Parallel() + + c, _ := newTestSentinelController(t, &fakeSentinelStore{}) + c.interval = time.Millisecond + var probes int + c.discoverFramework = func() (devices.VGPUFramework, []devices.VirtualFunction, error) { + probes++ + if probes == 1 { + return devices.VGPUFrameworkNone, nil, errors.New("transient sysfs error") + } + return devices.VGPUFrameworkNone, nil, nil + } + + done := make(chan error, 1) + go func() { done <- c.Run(context.Background()) }() + select { + case err := <-done: + require.NoError(t, err) + case <-time.After(10 * time.Second): + t.Fatal("Run did not exit after discovery resolved to a non vendor VFIO host") + } + assert.Equal(t, 2, probes) +} + // Kernel printk shares the serial console with the agent and can split a // marker mid-write — which is why the agent emits each report as several // identical lines. A corrupted copy must not convict (the strict shape is From a0bbbca69df2d575ad13ef2c495366f12fb0e299 Mon Sep 17 00:00:00 2001 From: yummybomb <19238148+yummybomb@users.noreply.github.com> Date: Fri, 21 Aug 2026 19:48:40 +0000 Subject: [PATCH 14/17] Revalidate the assignment before conviction and list targets strictly A conviction used the metadata snapshot taken before the log read, so a stop/start racing the scan could quarantine the VF the instance no longer holds. Reload the assignment and convict only when the VF and assignment epoch are unchanged; start's archive-before-persist ordering guarantees a marker read under an unchanged epoch belongs to that epoch's VF. On any other outcome the tail stays open and the next scan picks up the current assignment. Target listing used the lenient metadata walk, which drops entries whose stat fails without ever reaching the unreadable-metadata warning. Walk strictly first so a stat failure names the instance losing detection coverage, then fall back to the lenient walk so the readable rest keeps being scanned. --- lib/instances/vgpu_sentinel.go | 81 ++++++++++++++++++++++------- lib/instances/vgpu_sentinel_test.go | 69 ++++++++++++++++++++++++ 2 files changed, 130 insertions(+), 20 deletions(-) diff --git a/lib/instances/vgpu_sentinel.go b/lib/instances/vgpu_sentinel.go index 32d008831..9669cc3dc 100644 --- a/lib/instances/vgpu_sentinel.go +++ b/lib/instances/vgpu_sentinel.go @@ -47,6 +47,7 @@ type vgpuSentinelTarget struct { type vgpuSentinelStore interface { listVGPUSentinelTargets(ctx context.Context) ([]vgpuSentinelTarget, error) + getVGPUSentinelTarget(ctx context.Context, instanceID string) (vgpuSentinelTarget, bool, error) } type vgpuSentinelTail struct { @@ -227,6 +228,26 @@ func (c *VGPUSentinelController) scanTarget(ctx context.Context, target vgpuSent // instance is finished; a failed quarantine leaves the tail open so the // recurring report retries it. func (c *VGPUSentinelController) convict(ctx context.Context, target vgpuSentinelTarget, line string) bool { + // The target came from a metadata snapshot taken before the log read: a + // stop/start racing the scan could have assigned a different VF by now, + // and convicting the snapshot's VF would quarantine a healthy device. + // Reload the assignment and convict only if it is unchanged. Start + // archives the previous boot's log before persisting a new assignment, + // so if the metadata still shows this epoch after the marker was read, + // the marker provably belongs to this epoch's VF. On any other outcome + // the tail stays open; the next scan sees the current assignment and + // rescans its log from the top. + current, ok, err := c.store.getVGPUSentinelTarget(ctx, target.instanceID) + if err != nil { + c.log.WarnContext(ctx, "vGPU sentinel could not confirm assignment before conviction", + "vf", target.vfAddress, "instance_id", target.instanceID, "error", err) + return false + } + if !ok || current.vfAddress != target.vfAddress || current.assignedAt != target.assignedAt { + c.log.InfoContext(ctx, "vGPU sentinel skipping conviction: assignment changed during scan", + "vf", target.vfAddress, "instance_id", target.instanceID) + return false + } existed, err := c.quarantine(devices.VFQuarantine{ VFAddress: target.vfAddress, InstanceID: target.instanceID, @@ -320,35 +341,55 @@ func scanForSentinel(path string, tail *vgpuSentinelTail) (string, bool, error) // the scan runs continuously and deriving state would query every // hypervisor on the host. func (m *manager) listVGPUSentinelTargets(ctx context.Context) ([]vgpuSentinelTarget, error) { - files, err := m.listMetadataFiles() + // The strict walk names an instance whose metadata cannot be statted; the + // lenient retry keeps detection running on the readable rest, so one bad + // instance degrades coverage loudly instead of losing it host-wide. + files, err := m.listMetadataFilesStrict() if err != nil { - return nil, err + logger.FromContext(ctx).WarnContext(ctx, "vGPU sentinel skipping unreadable instance metadata", "error", err) + if files, err = m.listMetadataFiles(); err != nil { + return nil, err + } } targets := make([]vgpuSentinelTarget, 0, len(files)) for _, file := range files { id := filepath.Base(filepath.Dir(file)) - meta, err := m.loadMetadata(id) + target, ok, err := m.getVGPUSentinelTarget(ctx, id) if err != nil { - if !errors.Is(err, ErrNotFound) { - // An unreadable record removes its VF from detection; say so - // rather than silently shrinking coverage. - logger.FromContext(ctx).WarnContext(ctx, "vGPU sentinel skipping unreadable instance metadata", "instance_id", id, "error", err) - } + // An unreadable record removes its VF from detection; say so + // rather than silently shrinking coverage. + logger.FromContext(ctx).WarnContext(ctx, "vGPU sentinel skipping unreadable instance metadata", "instance_id", id, "error", err) continue } - if meta.GPUFramework != devices.VGPUFrameworkVendorVFIO || meta.GPUDevicePath == "" { - continue - } - assignedAt := "" - if meta.GPUAssignedAt != nil { - assignedAt = meta.GPUAssignedAt.UTC().Format(time.RFC3339Nano) + if ok { + targets = append(targets, target) } - targets = append(targets, vgpuSentinelTarget{ - instanceID: id, - vfAddress: filepath.Base(meta.GPUDevicePath), - appLogPath: m.paths.InstanceAppLog(id), - assignedAt: assignedAt, - }) } return targets, nil } + +// getVGPUSentinelTarget returns the instance's current vendor VFIO sentinel +// target, reporting ok=false when the instance is gone or holds no such +// assignment. +func (m *manager) getVGPUSentinelTarget(_ context.Context, instanceID string) (vgpuSentinelTarget, bool, error) { + meta, err := m.loadMetadata(instanceID) + if err != nil { + if errors.Is(err, ErrNotFound) { + return vgpuSentinelTarget{}, false, nil + } + return vgpuSentinelTarget{}, false, err + } + if meta.GPUFramework != devices.VGPUFrameworkVendorVFIO || meta.GPUDevicePath == "" { + return vgpuSentinelTarget{}, false, nil + } + assignedAt := "" + if meta.GPUAssignedAt != nil { + assignedAt = meta.GPUAssignedAt.UTC().Format(time.RFC3339Nano) + } + return vgpuSentinelTarget{ + instanceID: instanceID, + vfAddress: filepath.Base(meta.GPUDevicePath), + appLogPath: m.paths.InstanceAppLog(instanceID), + assignedAt: assignedAt, + }, true, nil +} diff --git a/lib/instances/vgpu_sentinel_test.go b/lib/instances/vgpu_sentinel_test.go index a9f4d39e8..ca063c913 100644 --- a/lib/instances/vgpu_sentinel_test.go +++ b/lib/instances/vgpu_sentinel_test.go @@ -11,6 +11,7 @@ import ( "time" "github.com/kernel/hypeman/lib/devices" + "github.com/kernel/hypeman/lib/paths" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "go.opentelemetry.io/otel/metric/noop" @@ -43,6 +44,15 @@ func (s *fakeSentinelStore) listVGPUSentinelTargets(context.Context) ([]vgpuSent return s.targets, nil } +func (s *fakeSentinelStore) getVGPUSentinelTarget(_ context.Context, instanceID string) (vgpuSentinelTarget, bool, error) { + for _, target := range s.targets { + if target.instanceID == instanceID { + return target, true, nil + } + } + return vgpuSentinelTarget{}, false, nil +} + func newTestSentinelController(t *testing.T, store vgpuSentinelStore) (*VGPUSentinelController, *[]devices.VFQuarantine) { t.Helper() counter, err := noop.NewMeterProvider().Meter("test").Int64Counter("test") @@ -232,6 +242,65 @@ func TestVGPUSentinelControllerDropsStaleTails(t *testing.T) { assert.NotContains(t, c.tails, "instance-1") } +// A marker read against a metadata snapshot must not convict once the +// instance's assignment has changed under the scan: the marker belonged to +// the old epoch's VF, and the current VF may be healthy. +func TestVGPUSentinelControllerSkipsConvictionOnChangedAssignment(t *testing.T) { + t.Parallel() + + logPath := filepath.Join(t.TempDir(), "app.log") + require.NoError(t, os.WriteFile(logPath, []byte(testSentinelLine), 0644)) + store := &fakeSentinelStore{targets: []vgpuSentinelTarget{{ + instanceID: "instance-1", + vfAddress: "0000:e3:00.5", + appLogPath: logPath, + assignedAt: "2026-08-21T00:00:10Z", + }}} + c, quarantined := newTestSentinelController(t, store) + + stale := vgpuSentinelTarget{ + instanceID: "instance-1", + vfAddress: "0000:e3:00.4", + appLogPath: logPath, + assignedAt: "2026-08-21T00:00:00Z", + } + c.scanTarget(context.Background(), stale) + assert.Empty(t, *quarantined) + assert.False(t, c.tails["instance-1"].done) + + // A deleted instance can no longer vouch for the marker either. + store.targets = nil + c.tails["instance-1"] = &vgpuSentinelTail{vfAddress: stale.vfAddress, assignedAt: stale.assignedAt} + c.scanTarget(context.Background(), stale) + assert.Empty(t, *quarantined) +} + +func TestListVGPUSentinelTargetsSkipsUnstattableMetadata(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("root bypasses directory permissions") + } + + m := &manager{paths: paths.New(t.TempDir())} + assigned := time.Now().UTC() + for _, id := range []string{"readable", "unreadable"} { + require.NoError(t, m.ensureDirectories(id)) + require.NoError(t, m.saveMetadata(&metadata{StoredMetadata: StoredMetadata{ + Id: id, + GPUFramework: devices.VGPUFrameworkVendorVFIO, + GPUDevicePath: "/sys/bus/pci/devices/0000:e3:00.4", + GPUAssignedAt: &assigned, + }})) + } + instanceDir := filepath.Dir(m.paths.InstanceMetadata("unreadable")) + require.NoError(t, os.Chmod(instanceDir, 0o000)) + t.Cleanup(func() { _ = os.Chmod(instanceDir, 0o755) }) + + targets, err := m.listVGPUSentinelTargets(context.Background()) + require.NoError(t, err) + require.Len(t, targets, 1) + assert.Equal(t, "readable", targets[0].instanceID) +} + // A transient discovery error fails open — the controller scans as if the // host were vendor VFIO — but must keep re-probing so a host that is not // vendor VFIO stops scanning once discovery recovers. From af322ba6deff9da8ff9abe0fbf935c679ce42c91 Mon Sep 17 00:00:00 2001 From: yummybomb <19238148+yummybomb@users.noreply.github.com> Date: Fri, 21 Aug 2026 19:59:25 +0000 Subject: [PATCH 15/17] Document draining the parent GPU before an SR-IOV cycle The remediation section required an empty card but never said how to get one: the quarantine only deprioritizes the card, so under capacity pressure new placements keep refilling its healthy VFs. Document the manual cordon (pre-quarantining the card's VFs in vf-health.json plus a hypeman restart), why running instances are untouched and drain through their normal lifecycle, and how to monitor and uncordon. --- lib/devices/GPU.md | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/lib/devices/GPU.md b/lib/devices/GPU.md index 800c4b5a0..8a8a92d22 100644 --- a/lib/devices/GPU.md +++ b/lib/devices/GPU.md @@ -330,6 +330,27 @@ requires no vGPU assignments on that GPU). The DCGM quiesce is not optional: with `nv-hostengine`/`dcgm-exporter` holding the GPUs open, `sriov-manage -d` fails with `Cannot obtain unbindLock` on first contact. +**Draining the parent GPU.** The quarantine only deprioritizes the card +(overflow-only), so under capacity pressure new placements can still land on +its healthy VFs and refill it. To actually drain the card, cordon it by +adding every one of its VFs to `/gpu/vf-health.json` (copy the +record shape of a real conviction: `vf_address` plus `quarantined_at`) and +restarting hypeman immediately — the store loads only at startup, and a +conviction landing before the restart re-persists the in-memory set over +your edit. The restart does not disturb running VMs: startup reconciliation +protects live VFs. Quarantined VFs are fully excluded from placement and +from advertised profile availability, so the cordon holds even when every +other card is full, and `hypeman_instances_vgpu_quarantined_vfs` reads high +for its duration — expected, not an incident. + +Running instances are untouched by the cordon and drain through their normal +lifecycle: standby is blocked for vGPU instances, so only a running VM pins +a VF, every stop or delete releases it (never to be re-selected while +cordoned), and the next start picks a fresh VF on another card. Monitor the +drain by listing instances whose `gpu.device_path` sits under the parent +GPU; once none remain, run the cycle below, then remove the card's entries +from `vf-health.json` and restart hypeman again to uncordon. + ```bash # 1. Quiesce the services holding the GPU (required for the unbind lock). systemctl stop nvidia-dcgm-exporter nvidia-dcgm From ef934fba8d90bae146578c3ef8fc4e67a7d84c5f Mon Sep 17 00:00:00 2001 From: yummybomb <19238148+yummybomb@users.noreply.github.com> Date: Fri, 21 Aug 2026 20:08:02 +0000 Subject: [PATCH 16/17] Deduplicate the wedge remediation docs The drain section restated the quarantine's placement-exclusion semantics and the vf-health.json edit-plus-restart rule, and gave the uncordon instruction twice. Hoist the edit rule into one paragraph both the cordon and clear steps reference, and let the detection paragraph own the exclusion semantics. --- lib/devices/GPU.md | 47 +++++++++++++++++++--------------------------- 1 file changed, 19 insertions(+), 28 deletions(-) diff --git a/lib/devices/GPU.md b/lib/devices/GPU.md index 8a8a92d22..75df2de55 100644 --- a/lib/devices/GPU.md +++ b/lib/devices/GPU.md @@ -286,12 +286,11 @@ NVRM: GPU 0000:00:03.0: RmInitAdapter failed! (0x22:0x65:884) Hypeman detects this automatically: the guest agent watches the guest kernel log (`/dev/kmsg`) for that line and reports it as a `HYPEMAN-GPU-INIT-FAILED` -marker — the same guest-to-host channel as the other `HYPEMAN-*` markers, -landing in the instance's `logs/app.log` — and the vGPU sentinel controller -scans that file for every vendor VFIO instance. A match quarantines the VF in +marker in the instance's `logs/app.log`, which the vGPU sentinel controller +scans for every vendor VFIO instance. A match quarantines the VF in `/gpu/vf-health.json` (it survives restarts): the VF is excluded from placement and from advertised profile availability, and its parent GPU -becomes overflow-only so it drains toward the SR-IOV cycle. The conviction is +becomes overflow-only — deprioritized for new placements. The conviction is logged at error level (`quarantined wedged vGPU VF`) and counted in `hypeman_instances_vgpu_sentinel_convictions_total`; `hypeman_instances_vgpu_quarantined_vfs` gauges the current quarantine count. @@ -330,26 +329,20 @@ requires no vGPU assignments on that GPU). The DCGM quiesce is not optional: with `nv-hostengine`/`dcgm-exporter` holding the GPUs open, `sriov-manage -d` fails with `Cannot obtain unbindLock` on first contact. -**Draining the parent GPU.** The quarantine only deprioritizes the card -(overflow-only), so under capacity pressure new placements can still land on -its healthy VFs and refill it. To actually drain the card, cordon it by -adding every one of its VFs to `/gpu/vf-health.json` (copy the -record shape of a real conviction: `vf_address` plus `quarantined_at`) and -restarting hypeman immediately — the store loads only at startup, and a -conviction landing before the restart re-persists the in-memory set over -your edit. The restart does not disturb running VMs: startup reconciliation -protects live VFs. Quarantined VFs are fully excluded from placement and -from advertised profile availability, so the cordon holds even when every -other card is full, and `hypeman_instances_vgpu_quarantined_vfs` reads high -for its duration — expected, not an incident. - -Running instances are untouched by the cordon and drain through their normal -lifecycle: standby is blocked for vGPU instances, so only a running VM pins -a VF, every stop or delete releases it (never to be re-selected while -cordoned), and the next start picks a fresh VF on another card. Monitor the -drain by listing instances whose `gpu.device_path` sits under the parent -GPU; once none remain, run the cycle below, then remove the card's entries -from `vf-health.json` and restart hypeman again to uncordon. +Any manual edit to `vf-health.json` needs an immediate hypeman restart: the +store loads only at startup, and a conviction landing first re-persists the +in-memory set over your edit. The restart does not disturb running VMs — +startup reconciliation protects live VFs. + +**Draining the parent GPU.** Overflow-only is a preference, not a cordon: +under capacity pressure new placements still land on the card's healthy VFs +and refill it. To drain the card, quarantine all of its VFs by hand — add +records to `vf-health.json` (the shape of a real conviction: `vf_address` +plus `quarantined_at`) and restart. Running instances are untouched and +drain through their normal lifecycle: standby is blocked for vGPU instances, +so only a running VM pins a VF, and each stop or delete frees one for good. +Monitor by listing instances whose `gpu.device_path` sits under the parent +GPU; once none remain, run the cycle below. ```bash # 1. Quiesce the services holding the GPU (required for the unbind lock). @@ -363,10 +356,8 @@ systemctl stop nvidia-dcgm-exporter nvidia-dcgm systemctl start nvidia-dcgm nvidia-dcgm-exporter ``` -After the cycle, clear the quarantine by removing the VF's entry from -`/gpu/vf-health.json` and restarting hypeman immediately — the -running process keeps the quarantine in memory, and a conviction landing -before the restart re-persists it over your edit. Then boot a GPU instance as +After the cycle, clear the quarantine by removing the card's entries from +`vf-health.json` (restart rule above), then boot a GPU instance as verification: placement excludes quarantined VFs, so the recovered VF cannot be targeted while its entry exists, and there is no VF-pin API — clearing first is safe because the sentinel automatically re-quarantines the VF if the From ee1b16076489f0201deeb4af11cef6408f6b2d7f Mon Sep 17 00:00:00 2001 From: yummybomb <19238148+yummybomb@users.noreply.github.com> Date: Fri, 21 Aug 2026 20:25:43 +0000 Subject: [PATCH 17/17] Convict on a released assignment instead of skipping The pre-conviction recheck treated a missing assignment like a changed one and skipped the quarantine. A stop or delete landing between the metadata snapshot and the recheck releases the assignment but cannot un-wedge the VF, and the archive-before-persist ordering already ties a marker read under this epoch's snapshot to this epoch's VF, so the conviction is valid. Skip only when a different assignment exists. --- lib/instances/vgpu_sentinel.go | 13 +++++++------ lib/instances/vgpu_sentinel_test.go | 6 ++++-- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/lib/instances/vgpu_sentinel.go b/lib/instances/vgpu_sentinel.go index 9669cc3dc..5e90a9dff 100644 --- a/lib/instances/vgpu_sentinel.go +++ b/lib/instances/vgpu_sentinel.go @@ -231,19 +231,20 @@ func (c *VGPUSentinelController) convict(ctx context.Context, target vgpuSentine // The target came from a metadata snapshot taken before the log read: a // stop/start racing the scan could have assigned a different VF by now, // and convicting the snapshot's VF would quarantine a healthy device. - // Reload the assignment and convict only if it is unchanged. Start + // Reload the assignment and skip only if a different one exists. Start // archives the previous boot's log before persisting a new assignment, - // so if the metadata still shows this epoch after the marker was read, - // the marker provably belongs to this epoch's VF. On any other outcome - // the tail stays open; the next scan sees the current assignment and - // rescans its log from the top. + // so a marker read under this epoch's snapshot provably belongs to this + // epoch's VF — including when the instance was stopped or deleted after + // the read, which releases the assignment but cannot un-wedge the VF. + // On a skip or error the tail stays open; the next scan sees the current + // assignment and rescans its log from the top. current, ok, err := c.store.getVGPUSentinelTarget(ctx, target.instanceID) if err != nil { c.log.WarnContext(ctx, "vGPU sentinel could not confirm assignment before conviction", "vf", target.vfAddress, "instance_id", target.instanceID, "error", err) return false } - if !ok || current.vfAddress != target.vfAddress || current.assignedAt != target.assignedAt { + if ok && (current.vfAddress != target.vfAddress || current.assignedAt != target.assignedAt) { c.log.InfoContext(ctx, "vGPU sentinel skipping conviction: assignment changed during scan", "vf", target.vfAddress, "instance_id", target.instanceID) return false diff --git a/lib/instances/vgpu_sentinel_test.go b/lib/instances/vgpu_sentinel_test.go index ca063c913..395539ea1 100644 --- a/lib/instances/vgpu_sentinel_test.go +++ b/lib/instances/vgpu_sentinel_test.go @@ -268,11 +268,13 @@ func TestVGPUSentinelControllerSkipsConvictionOnChangedAssignment(t *testing.T) assert.Empty(t, *quarantined) assert.False(t, c.tails["instance-1"].done) - // A deleted instance can no longer vouch for the marker either. + // A stop or delete after the marker was read releases the assignment but + // cannot un-wedge the VF: the marker still convicts the scanned epoch's VF. store.targets = nil c.tails["instance-1"] = &vgpuSentinelTail{vfAddress: stale.vfAddress, assignedAt: stale.assignedAt} c.scanTarget(context.Background(), stale) - assert.Empty(t, *quarantined) + require.Len(t, *quarantined, 1) + assert.Equal(t, "0000:e3:00.4", (*quarantined)[0].VFAddress) } func TestListVGPUSentinelTargetsSkipsUnstattableMetadata(t *testing.T) {