From ed972325982f50ca9344375dacb9ffb507acd391 Mon Sep 17 00:00:00 2001 From: kisfenyo Date: Sat, 25 Jul 2026 08:21:45 +0200 Subject: [PATCH] =?UTF-8?q?v0.95.0:=20SMART=20coverage=20=E2=80=94=20union?= =?UTF-8?q?-path=20drives=20+=20LVM/dm=20root=20+=20device=20model?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implements SPIKE-smart-coverage-2026-07-25 fixes B+A (additive; MinAgent unchanged). Fix B: storage.SmartReader.SMARTForBacking wired into the /disks union path (localapi Smart seam) so registry/USB drives get a real SMART read (watchdog Known stays enrich-free). Fix A: smartDeviceFor resolves dm/LVM to the whole disk via /sys/block//slaves (recursive; skips >1-disk); the builtin local dir on the LVM root gets a SMART-only device from its containing filesystem (never touches backing/durable_id). SmartSummary.ModelName captured from smartctl. Fix C (-d sat) stays rejected. Tests + red-proofs (dm multi-disk skip, enrich smartHint, union routing); Known-path-never-SMARTs asserted. --- CHANGELOG.md | 23 ++++ CONTEXT.md | 8 ++ REUSE.md | 1 + cmd/felhom-agent/main.go | 5 +- internal/hub/report.go | 5 + internal/localapi/disks.go | 9 +- internal/localapi/disks_smart_test.go | 75 ++++++++++++ internal/localapi/server.go | 140 ++++++++++++---------- internal/storage/observe.go | 51 ++++++-- internal/storage/observe_smart_test.go | 71 ++++++++++++ internal/storage/smart.go | 4 + internal/storage/smartdev.go | 154 +++++++++++++++++++++++++ internal/storage/smartdev_test.go | 107 +++++++++++++++++ 13 files changed, 579 insertions(+), 74 deletions(-) create mode 100644 internal/storage/observe_smart_test.go create mode 100644 internal/storage/smartdev.go create mode 100644 internal/storage/smartdev_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 03b8fd1..fcab02f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,26 @@ +## v0.95.0 — SMART coverage: union-path drives + LVM/dm root + device model (2026-07-25) + +Additive; **MinAgent unchanged**; hub untouched (unknown JSON fields ignored). Implements the graded +fixes from `felhom.eu/documentation/audits/SPIKE-smart-coverage-2026-07-25.md`, which proved both demo +disks answer the allowlisted `smartctl -a -j` with PASSED but the agent never asked. + +- **Fix B — union-path SMART:** registry/USB drives ride the `/disks` union path (`driveTargets.Known`), + which skips Observe's `enrich`, so they showed "Nincs adat" despite working SMART. New + `storage.SmartReader` (`SMARTForBacking`, reuses `smartDeviceFor`) is wired into the union path via a + localapi seam — the USB drive now reports its real verdict. The watchdog `Known` path stays + enrich-free (asserted: zero smartctl calls). +- **Fix A — LVM/device-mapper resolution:** `smartDeviceFor` gains a dm branch that resolves + `/dev/dm-N` / `/dev/mapper/X` to the single backing whole disk via `/sys/block//slaves` + (recursive; **skips** rather than guesses when slaves span >1 physical disk). And the builtin `local` + dir on the LVM root — whose `backing_device` is empty by design (removable-safety) — now gets a + **SMART-only** device resolved from its containing filesystem (mount table), never touching + `backing_device`/`durable_id`. The system SSD stops reading "Nincs adat". +- **Device model:** `SmartSummary.ModelName` captured from smartctl's own `model_name` (already parsed), + so the controller card can label a disk "TOSHIBA MQ04ABF100" instead of a raw UUID. +- Fix C (`-d sat`) stays rejected (disproven live; absent from sudoers). No sudoers/manifest change. + Consumed by controller v0.171.0. Tests: dm-resolution table (+multi-disk-skip red-proof), containing-fs + resolution (+red-proof), union-path SMART (+red-proof), model capture, Known-path-never-SMARTs. + ## v0.94.0 — serialize per-disk SMART into the /disks payload (2026-07-24) Additive, backward-compatible; **MinAgent floor unchanged** (the controller feature-detects by payload diff --git a/CONTEXT.md b/CONTEXT.md index 4e7e64e..b9dc31b 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -5,6 +5,14 @@ ## Current +- **2026-07-25 — v0.95.0 (additive): SMART coverage fixes (spike B+A) + device model.** Union-path + drives (USB/registry) now get SMART via `storage.SmartReader.SMARTForBacking` wired into the localapi + `/disks` union (localapi `Smart` seam); `smartDeviceFor` resolves dm/LVM to the whole disk via + `/sys/block//slaves` (recursive, skips >1-disk); the builtin `local` dir on the LVM root gets a + **SMART-only** device from its containing filesystem (never touches backing/durable_id — the + removable-safety guard in build() stays intact); `SmartSummary.ModelName` captured from smartctl. The + watchdog `Known` path stays enrich-free. Consumed by controller v0.171.0. Source of WHERE: + `felhom.eu/documentation/audits/SPIKE-smart-coverage-2026-07-25.md`. - **2026-07-24 — v0.94.0 (additive): SMART serialized into /disks.** `localapi.DiskInfo` gains `Smart *hub.SmartSummary` (omitempty), copied from the target's already-computed Observe-time enrichment when `Health != ""` — no new smartctl load, no endpoint, no sudoers/MinAgent change. The diff --git a/REUSE.md b/REUSE.md index 0af122a..0cf0618 100644 --- a/REUSE.md +++ b/REUSE.md @@ -174,6 +174,7 @@ - Two lsblk `-J` parsers with near-identical structs: `parseLsblkDevice`/`lsblkDevice` (internal/storage/hostops.go) vs `parseLsblkNodes`/`lsblkDev` (internal/storage/claim.go). - Two smartctl `-a -j` paths: `SudoHostOps.SMART` (internal/storage/hostops.go, parsed `hub.SmartSummary`) vs `Privileged.SMART` (internal/proxmox/privileged.go, raw map). +- **SMART device resolution (v0.95.0):** `smartDeviceFor` (internal/storage/observe.go) resolves partition→disk AND dm/LVM→disk (`dmWholeDisk` in internal/storage/smartdev.go, via `/sys/block//slaves`, `sysBlockRoot` test seam). `storage.SmartReader.SMARTForBacking` is the shared read the localapi `/disks` union path uses (Fix B) — do NOT re-implement smartctl parsing. The builtin-`local` SMART device comes from `containingMountDevice` (SMART-only; never feeds backing/durable_id). - Atomic tmp+rename JSON store implemented 3×: `IntentStore.saveLocked` (internal/storage/intent.go), `FormatJobStore.save` (internal/localapi/formatjob.go), `GuestBindStore.saveLocked` (internal/localapi/guestbindstore.go) — comments say "mirrors", no shared helper. - `run(ctx, name, args...) error` stderr-wrapping helper duplicated 4×: `SudoHostOps.run`, `Privileged.run`, `BackHalf.run` (internal/provision/backhalf.go), `GuestBinder.run` (internal/localapi/guestbind.go). - Several independent /proc mount-table readers: `SudoHostOps.mountedSet` (internal/storage/hostops.go), `ProcHostReader.Mounts` (internal/storage/hostread.go), `isHostMountpoint` + `countHostMounts` (internal/localapi/intermediary.go). diff --git a/cmd/felhom-agent/main.go b/cmd/felhom-agent/main.go index 74af922..e083f38 100644 --- a/cmd/felhom-agent/main.go +++ b/cmd/felhom-agent/main.go @@ -1285,8 +1285,9 @@ func buildLocalAPIServer(cfg config.Config, px *proxmox.Client, store *backup.St Backups: runner, Store: store, Storage: observer, - DriveTargets: driveTargets, // Impl-2a: registry+units drives for the /disks view (union w/ Observe storages) - HostReader: storage.NewProcHostReader(), // Impl-2b: durableIDForMount raw-mount fallback + role gate + DriveTargets: driveTargets, // Impl-2a: registry+units drives for the /disks view (union w/ Observe storages) + Smart: storage.NewSmartReader(hostOps), // v0.95.0 Fix B: SMART for the union-path drives + HostReader: storage.NewProcHostReader(), // Impl-2b: durableIDForMount raw-mount fallback + role gate Tokens: tokens, BackupCadence: cfg.Backup.BackupCadence(), // Disk management (slice 8C): the privileged host surface + the data-bearing wipe gate. diff --git a/internal/hub/report.go b/internal/hub/report.go index 6ed135a..baca996 100644 --- a/internal/hub/report.go +++ b/internal/hub/report.go @@ -316,6 +316,11 @@ type ThinPoolFill struct { type SmartSummary struct { Health string `json:"health"` + // ModelName is smartctl's own device model (v0.95.0), captured from the JSON already parsed, so + // the UI can show a human label ("TOSHIBA MQ04ABF100") instead of a raw UUID. omitempty + + // pointer: absent on an old agent or a device that reports no model. + ModelName *string `json:"model_name,omitempty"` + TemperatureC *int `json:"temperature_c"` PowerOnHours *int `json:"power_on_hours"` diff --git a/internal/localapi/disks.go b/internal/localapi/disks.go index 86231a8..dd832d8 100644 --- a/internal/localapi/disks.go +++ b/internal/localapi/disks.go @@ -255,10 +255,17 @@ func (s *Server) handleDisks(w http.ResponseWriter, r *http.Request, vmid int) { if d.UUID != "" { // Resolve to the real /dev node (e.g. /dev/sdd), not the by-uuid symlink path, to match // how Observe-sourced rows display the backing device. - if dev, err := storage.ResolveStorageDevice("uuid:" + d.UUID); err == nil { + if dev, err := s.resolveStorageDevice("uuid:" + d.UUID); err == nil { di.BackingDevice = dev } } + // Fix B (v0.95.0): union-path drives skip Observe's enrich, so read SMART here through the + // same seam the dir targets use. Only set when the read actually ran (Health != ""). + if di.BackingDevice != "" && s.smart != nil { + if sm := s.smart.SMARTForBacking(r.Context(), di.BackingDevice); sm.Health != "" { + di.Smart = &sm + } + } if total, used, okc := statfsCapacity(d.MountPath); okc { di.TotalBytes, di.UsedBytes = total, used di.UsedFraction = float64(used) / float64(total) diff --git a/internal/localapi/disks_smart_test.go b/internal/localapi/disks_smart_test.go index cf56cea..d18aba6 100644 --- a/internal/localapi/disks_smart_test.go +++ b/internal/localapi/disks_smart_test.go @@ -1,6 +1,9 @@ package localapi import ( + "context" + "io" + "log/slog" "net/http" "testing" @@ -62,3 +65,75 @@ func TestDisks_SmartSerialized(t *testing.T) { t.Errorf("nosmart disk: smart must be omitted when Health is empty, got %+v", byName["nosmart"].Smart) } } + +// ---- Fix B (v0.95.0): the /disks union path reads SMART for registry/USB drives ---- + +type fakeKnownTargets struct{ drives []storage.KnownTarget } + +func (f fakeKnownTargets) Known(context.Context) ([]storage.KnownTarget, error) { return f.drives, nil } + +type fakeSmartReader struct { + byDev map[string]hub.SmartSummary + calls []string +} + +func (f *fakeSmartReader) SMARTForBacking(_ context.Context, dev string) hub.SmartSummary { + f.calls = append(f.calls, dev) + if s, ok := f.byDev[dev]; ok { + return s + } + return hub.SmartSummary{} +} + +func sp(s string) *string { return &s } + +// A union-path (registry/USB) drive now gets a real SMART read + model, via the Smart seam — it used +// to ride the enrich-free union path and show "Nincs adat". +// Red-proof: delete the Fix-B block in handleDisks (the s.smart.SMARTForBacking call) → the union +// drive carries no smart and this fails. +func TestDisks_UnionPathReadsSMART(t *testing.T) { + d := &fakeDiskOps{probe: storage.DeviceProbe{Probed: true}} + sm := &fakeSmartReader{byDev: map[string]hub.SmartSummary{ + "/dev/sdb1": {Health: hub.SmartPassed, ModelName: sp("TOSHIBA MQ04ABF100")}, + }} + srv, err := NewServer(Options{ + ListenAddr: "127.0.0.1:0", + Guests: &fakeGuests{}, + Backups: &fakeBackups{}, + Store: &fakeStore{}, + Storage: fakeStorage{}, // no Observe targets → the union drive is not deduped away + DriveTargets: fakeKnownTargets{drives: []storage.KnownTarget{ + {Name: "data-usb", Type: hub.StorageTypeUSB, MountPath: "/mnt/hdd_1", DurableID: "uuid:47a3", UUID: "47a3"}, + }}, + Smart: sm, + Tokens: staticTokens{"A": 8200}, + Disks: d, + HostReader: sysOnSDA(), + Logger: slog.New(slog.NewTextHandler(io.Discard, nil)), + }) + if err != nil { + t.Fatalf("new server: %v", err) + } + srv.baseCtx = context.Background() + srv.resolveStorageDevice = func(string) (string, error) { return "/dev/sdb1", nil } + + disks := decodeDisks(t, do(t, srv.Handler(), "GET", "/disks", "A", "").Body.Bytes()) + var usb *DiskInfo + for i := range disks { + if disks[i].Name == "data-usb" { + usb = &disks[i] + } + } + if usb == nil { + t.Fatalf("union drive not in /disks: %+v", disks) + } + if usb.Smart == nil || usb.Smart.Health != hub.SmartPassed { + t.Fatalf("union drive SMART not read: %+v", usb.Smart) + } + if usb.Smart.ModelName == nil || *usb.Smart.ModelName != "TOSHIBA MQ04ABF100" { + t.Errorf("union drive model not carried: %v", usb.Smart) + } + if len(sm.calls) != 1 || sm.calls[0] != "/dev/sdb1" { + t.Errorf("SMART should be read once on /dev/sdb1, got %v", sm.calls) + } +} diff --git a/internal/localapi/server.go b/internal/localapi/server.go index e660761..101252a 100644 --- a/internal/localapi/server.go +++ b/internal/localapi/server.go @@ -54,6 +54,13 @@ type StorageView interface { Observe(ctx context.Context) ([]hub.StorageTarget, error) } +// SmartReader (v0.95.0, Fix B) reads per-disk SMART for the /disks union path so registry/USB drives +// that ride the union (not Observe's enrich) still get a health verdict. A zero-value summary +// (Health "") means "could not read". Satisfied by *storage.SmartReader. +type SmartReader interface { + SMARTForBacking(ctx context.Context, backingDevice string) hub.SmartSummary +} + // TokenAuthority resolves a presented bearer token to its guest VMID. Satisfied by *TokenStore. type TokenAuthority interface { Lookup(token string) (int, bool) @@ -77,7 +84,11 @@ type Options struct { // DriveTargets (Impl-2a, optional) yields registry+units-sourced drives for the /disks view, so a // drive with NO PVE storage still appears. Unioned with Storage.Observe (deduped by mount path). DriveTargets storage.KnownTargets - Tokens TokenAuthority + // Smart (v0.95.0, Fix B) reads per-disk SMART for the /disks UNION path — registry/USB drives ride + // the union (not Observe's enrich), so without this they carry no health verdict. OPTIONAL; nil → + // union rows have no SMART (pre-v0.95.0 behavior). Satisfied by *storage.SmartReader. + Smart SmartReader + Tokens TokenAuthority // BackupCadence is the per-guest backup interval driving GET /backup/due (slice 8B). A guest // is "due" when no successful backup is recorded OR the newest one is older than this. 0 → a // safe default (24h). The hub-served policy is slice 10; this is the agent-local cadence. @@ -175,33 +186,34 @@ type backupJob struct { // Server is the per-guest local API (doc 03 §6). It serves the agent's pinned self-signed leaf // and authorizes every request against the token's guest only. type Server struct { - addr string - cert tls.Certificate - guests GuestAPI - backups BackupService - store BackupStore + addr string + cert tls.Certificate + guests GuestAPI + backups BackupService + store BackupStore storage StorageView driveTargets storage.KnownTargets // Impl-2a: registry+units drives for /disks (optional) - tokens TokenAuthority - cadence time.Duration - logger *slog.Logger - now func() time.Time + smart SmartReader // v0.95.0 Fix B: SMART for the /disks union path (optional) + tokens TokenAuthority + cadence time.Duration + logger *slog.Logger + now func() time.Time - disks DiskOps // slice 8C (optional) - diskGate StorageGate // slice 8C (optional) - guestList GuestLister // slice 8C (optional) - guestAttach GuestAttacher // slice 10 P2 (optional) - mem MemoryOps // v0.90.0 R-24 guest RAM resize (optional) - memMu sync.Mutex // single-flight around a resize apply (one customer per host) - netStorage NetworkStorageOps // Part A1: NAS network mounts (optional) - netMountRoot string // the user-data namespace root for the network-mount role gate - smbCredsDir string // where SMB creds files are written (out-of-band, 0600) - escrowStagePath string // fork-4: 0600 staging file for the pushed restic repo password - intent IntentRecorder // slice 10 P3 (optional) - guestBinds *GuestBindStore // F9 startup bind re-assert record (optional) - formatJobs *FormatJobStore // F20-BUG3 detached-format job record (optional) - staleLock StaleLockController // F2-b startup stale-lock recovery (optional) - host storage.HostReader // role classification source (optional; defaults to ProcHostReader) + disks DiskOps // slice 8C (optional) + diskGate StorageGate // slice 8C (optional) + guestList GuestLister // slice 8C (optional) + guestAttach GuestAttacher // slice 10 P2 (optional) + mem MemoryOps // v0.90.0 R-24 guest RAM resize (optional) + memMu sync.Mutex // single-flight around a resize apply (one customer per host) + netStorage NetworkStorageOps // Part A1: NAS network mounts (optional) + netMountRoot string // the user-data namespace root for the network-mount role gate + smbCredsDir string // where SMB creds files are written (out-of-band, 0600) + escrowStagePath string // fork-4: 0600 staging file for the pushed restic repo password + intent IntentRecorder // slice 10 P3 (optional) + guestBinds *GuestBindStore // F9 startup bind re-assert record (optional) + formatJobs *FormatJobStore // F20-BUG3 detached-format job record (optional) + staleLock StaleLockController // F2-b startup stale-lock recovery (optional) + host storage.HostReader // role classification source (optional; defaults to ProcHostReader) hostMetrics HostMetricsProvider // slice 9 (optional) hostID string // slice 10B: for the data-bearing-format pending-op hint @@ -212,6 +224,10 @@ type Server struct { // inline customer-confirmed wipe (durable id → current device, re-derive+match, // re-inspect). Defaults to s.reresolveDurableForWipe (real storage funcs); tests // override it to avoid touching real /dev. + // resolveStorageDevice maps a durable id (uuid:) to its /dev node for the /disks union + // path. Defaults to storage.ResolveStorageDevice (hits /dev/disk/by-*); tests override it. + resolveStorageDevice func(durableID string) (string, error) + reresolveWipe func(ctx context.Context, durableID string) (string, error) // reresolveBlank is the BLANK-format sibling (audit D3): same anti-retarget @@ -255,13 +271,13 @@ type Server struct { // holder. R lives ONLY in escrowR (never in the job struct — snapshots must be structurally // incapable of carrying it) and is zeroed on claim, supersede, or TTL expiry. See // escrow_ceremony.go for the custody rules. - escrowCeremony *EscrowCeremonyConfig - escrowMu sync.Mutex - escrowJob *escrowCeremonyJob - escrowR []byte - escrowRClaimed bool - escrowRExpiry time.Time - escrowDone <-chan struct{} // closes when the detached job finishes (tests wait on it) + escrowCeremony *EscrowCeremonyConfig + escrowMu sync.Mutex + escrowJob *escrowCeremonyJob + escrowR []byte + escrowRClaimed bool + escrowRExpiry time.Time + escrowDone <-chan struct{} // closes when the detached job finishes (tests wait on it) // ceremonyRun executes the fixed-argv sudo self-invocation (tests inject canned JSON). ceremonyRun ceremonyRunner // escrowSudoCheck is the preflight's list-mode grant probe (`sudo -n -l -- `). @@ -290,37 +306,38 @@ func NewServer(o Options) (*Server, error) { cadence = defaultBackupCadence } s := &Server{ - addr: o.ListenAddr, - cert: o.Cert, - guests: o.Guests, - backups: o.Backups, - store: o.Store, - storage: o.Storage, - driveTargets: o.DriveTargets, - tokens: o.Tokens, - cadence: cadence, - logger: o.Logger, - now: func() time.Time { return time.Now().UTC() }, - disks: o.Disks, - diskGate: o.DiskGate, - guestList: o.Guests2, - guestAttach: o.GuestAttach, - mem: o.Memory, - netStorage: o.NetStorage, - netMountRoot: storage.NetworkMountRoot, + addr: o.ListenAddr, + cert: o.Cert, + guests: o.Guests, + backups: o.Backups, + store: o.Store, + storage: o.Storage, + driveTargets: o.DriveTargets, + smart: o.Smart, + tokens: o.Tokens, + cadence: cadence, + logger: o.Logger, + now: func() time.Time { return time.Now().UTC() }, + disks: o.Disks, + diskGate: o.DiskGate, + guestList: o.Guests2, + guestAttach: o.GuestAttach, + mem: o.Memory, + netStorage: o.NetStorage, + netMountRoot: storage.NetworkMountRoot, smbCredsDir: o.SmbCredsDir, escrowStagePath: o.EscrowStagePath, - intent: o.Intent, - guestBinds: o.GuestBinds, - formatJobs: o.FormatJobs, - staleLock: o.StaleLock, - host: o.HostReader, - hostMetrics: o.HostMetrics, - hostID: o.HostID, - agentVersion: o.AgentVersion, - logRing: o.LogRing, - jobs: map[int]*backupJob{}, - swapInFlight: map[int]bool{}, + intent: o.Intent, + guestBinds: o.GuestBinds, + formatJobs: o.FormatJobs, + staleLock: o.StaleLock, + host: o.HostReader, + hostMetrics: o.HostMetrics, + hostID: o.HostID, + agentVersion: o.AgentVersion, + logRing: o.LogRing, + jobs: map[int]*backupJob{}, + swapInFlight: map[int]bool{}, } if s.escrowStagePath == "" { s.escrowStagePath = escrow.StagedResticPasswordPath() @@ -328,6 +345,7 @@ func NewServer(o Options) (*Server, error) { s.reresolveWipe = s.reresolveDurableForWipe s.reresolveBlank = s.reresolveDurableForBlankFormat s.deviceDurableID = storage.DeviceDurableID + s.resolveStorageDevice = storage.ResolveStorageDevice s.netTrigger = triggerNetMount s.netMounted = storage.NetworkMountedAt s.netJournal = readUnitJournal diff --git a/internal/storage/observe.go b/internal/storage/observe.go index b6bfb78..e9d2589 100644 --- a/internal/storage/observe.go +++ b/internal/storage/observe.go @@ -58,6 +58,9 @@ type observed struct { known KnownTarget src proxmox.Storage cat storageCategory + // smartHint (v0.95.0) is a SMART-ONLY whole-disk device for a dir-storage whose own backing is + // empty (the builtin `local` on the shared LVM root). Never assigned to BackingDevice/durable_id. + smartHint string } // Observe builds the reported []hub.StorageTarget. A non-nil error means the Proxmox read @@ -84,13 +87,22 @@ func (o *Observer) enrich(ctx context.Context, ob observed) hub.StorageTarget { if o.ops == nil { return t } - // SMART: only for dir-backed targets with a resolvable whole-disk device. - if ob.cat == catDir && t.BackingDevice != "" { - if dev, ok := smartDeviceFor(t.BackingDevice); ok { - if sm, err := o.ops.SMART(ctx, dev); err != nil { - o.logger.Warn("storage: SMART read failed; health UNKNOWN", "device", dev, "err", err) - } else { - t.Smart = sm + // SMART: for dir-backed targets. The device is the target's own backing (a USB/local-dir exact + // mount) OR, for a dir on a shared filesystem whose backing is deliberately empty (the builtin + // `local` on the LVM root — removable-safety guard in build), the SMART-only hint build() resolved + // from the containing filesystem. smartDeviceFor then resolves dm/LVM/partition to the whole disk. + if ob.cat == catDir { + smartDev := t.BackingDevice + if smartDev == "" { + smartDev = ob.smartHint + } + if smartDev != "" { + if dev, ok := smartDeviceFor(smartDev); ok { + if sm, err := o.ops.SMART(ctx, dev); err != nil { + o.logger.Warn("storage: SMART read failed; health UNKNOWN", "device", dev, "err", err) + } else { + t.Smart = sm + } } } } @@ -238,10 +250,23 @@ func (o *Observer) build(s proxmox.Storage, mounts []Mount) observed { } } + // SMART-only device hint (v0.95.0): a dir-storage that lives INSIDE a shared filesystem (the + // builtin `local` on the LVM root) has empty backing by design, yet we can still read the PHYSICAL + // disk's SMART by resolving its containing filesystem. Gated to catDir + no own backing + reachable, + // so an UNPLUGGED removable (disconnected) never reads root's SMART, and a mounted removable uses + // its own backing instead. + smartHint := "" + if category == catDir && backingDevice == "" && reachable { + if dev, ok := containingMountDevice(mounts, s.Path); ok { + smartHint = dev + } + } + return observed{ - target: tgt, - src: s, - cat: category, + target: tgt, + src: s, + cat: category, + smartHint: smartHint, known: KnownTarget{ Name: s.Storage, Type: typ, @@ -260,6 +285,12 @@ func (o *Observer) build(s proxmox.Storage, mounts []Mount) observed { // smartctl (which targets the disk, not the partition). Returns ok=false when the result // isn't a recognized raw disk (e.g. device-mapper / LVM), so SMART is simply skipped. func smartDeviceFor(device string) (string, bool) { + // Device-mapper / LVM (Fix A, v0.95.0): resolve to the single backing whole disk via sysfs + // slaves. This is what finally covers the system SSD under `pve-root`. The sysfs resolution IS + // the existence check, so we do NOT re-run ValidateSMARTDevice on its result. + if strings.HasPrefix(device, "/dev/dm-") || strings.HasPrefix(device, "/dev/mapper/") { + return dmWholeDisk(device) + } dev := device if m := reNVMePart.FindStringSubmatch(device); m != nil { dev = m[1] // /dev/nvme0n1p2 -> /dev/nvme0n1 diff --git a/internal/storage/observe_smart_test.go b/internal/storage/observe_smart_test.go new file mode 100644 index 0000000..0389f60 --- /dev/null +++ b/internal/storage/observe_smart_test.go @@ -0,0 +1,71 @@ +package storage + +import ( + "context" + "testing" + + "gitea.dooplex.hu/admin/felhom-agent/internal/hub" + "gitea.dooplex.hu/admin/felhom-agent/internal/proxmox" +) + +// Fix A (A2): the builtin `local` dir lives on the LVM root, so its backing is empty by design — but +// enrich now resolves the containing filesystem (/ → /dev/mapper/pve-root) and, via the dm sysfs +// slaves, the physical disk (/dev/sda). The system disk stops reading "Nincs adat". +// Red-proof: drop the `smartDev = ob.smartHint` fallback in enrich → local stays UNKNOWN and SMART +// is never called on /dev/sda. +func TestObserve_SystemDirSMARTViaContainingFS(t *testing.T) { + fixtureSysfs(t, map[string][]string{"dm-1": {"sda3"}}, map[string]string{"dm-1": "pve-root"}) + + api := &fakeStorageAPI{ + node: "demo-felhom", + cluster: []proxmox.Storage{{Storage: "local", Type: "dir", Path: "/var/lib/vz"}}, + nodeSt: []proxmox.Storage{{Storage: "local", Type: "dir", Path: "/var/lib/vz", Active: 1}}, + } + host := &fakeHostReader{ + mounts: []Mount{{Device: "/dev/mapper/pve-root", MountPoint: "/", FSType: "ext4"}}, + } + ops := &fakeHostOps{smartByDevice: map[string]hub.SmartSummary{"/dev/sda": {Health: hub.SmartPassed, ModelName: strptr("AirDisk 512GB SSD")}}} + + got, err := NewObserver(api, host, ops, quietLogger()).Observe(context.Background()) + if err != nil { + t.Fatal(err) + } + local := byName(got)["local"] + if local.Smart.Health != hub.SmartPassed { + t.Errorf("system disk SMART not enriched via the containing fs: health=%q", local.Smart.Health) + } + if local.Smart.ModelName == nil || *local.Smart.ModelName != "AirDisk 512GB SSD" { + t.Errorf("model not carried: %v", local.Smart.ModelName) + } + // SMART must have run on the resolved PHYSICAL disk, never the dm/mapper node. + if len(ops.smartDevices) != 1 || ops.smartDevices[0] != "/dev/sda" { + t.Errorf("SMART should target /dev/sda, got %v", ops.smartDevices) + } + // The backing device / durable_id must stay untouched by the SMART-only resolution. + if local.BackingDevice != "" { + t.Errorf("system-dir SMART resolution leaked into BackingDevice: %q", local.BackingDevice) + } +} + +// The watchdog Known() path MUST remain enrich-free (its slow root-shelling reads are the reason it +// exists as a separate fast path). Known must never invoke SMART. +// Red-proof: route Known through enrich → smartDevices is non-empty and this fails. +func TestKnown_NeverInvokesSMART(t *testing.T) { + fixtureSysfs(t, map[string][]string{"dm-1": {"sda3"}}, map[string]string{"dm-1": "pve-root"}) + api := &fakeStorageAPI{ + node: "demo-felhom", + cluster: []proxmox.Storage{{Storage: "local", Type: "dir", Path: "/var/lib/vz"}}, + nodeSt: []proxmox.Storage{{Storage: "local", Type: "dir", Path: "/var/lib/vz", Active: 1}}, + } + host := &fakeHostReader{mounts: []Mount{{Device: "/dev/mapper/pve-root", MountPoint: "/", FSType: "ext4"}}} + ops := &fakeHostOps{smartByDevice: map[string]hub.SmartSummary{"/dev/sda": {Health: hub.SmartPassed}}} + + if _, err := NewObserver(api, host, ops, quietLogger()).Known(context.Background()); err != nil { + t.Fatal(err) + } + if len(ops.smartDevices) != 0 { + t.Errorf("Known() invoked SMART %d time(s) — it must stay enrich-free: %v", len(ops.smartDevices), ops.smartDevices) + } +} + +func strptr(s string) *string { return &s } diff --git a/internal/storage/smart.go b/internal/storage/smart.go index 24c6aa0..3fcbf8f 100644 --- a/internal/storage/smart.go +++ b/internal/storage/smart.go @@ -11,6 +11,7 @@ import ( // presence so an absent section (e.g. NVMe fields on a SATA disk, or no SMART at all on a // USB bridge) decodes cleanly to nil and we degrade to UNKNOWN. type smartctlJSON struct { + ModelName *string `json:"model_name"` SmartStatus *struct { Passed bool `json:"passed"` } `json:"smart_status"` @@ -64,6 +65,9 @@ func parseSMART(raw []byte) hub.SmartSummary { s.Health = hub.SmartFailing } } + if j.ModelName != nil && *j.ModelName != "" { + s.ModelName = j.ModelName + } if j.Temperature != nil && j.Temperature.Current != nil { s.TemperatureC = j.Temperature.Current } diff --git a/internal/storage/smartdev.go b/internal/storage/smartdev.go new file mode 100644 index 0000000..2a36608 --- /dev/null +++ b/internal/storage/smartdev.go @@ -0,0 +1,154 @@ +package storage + +import ( + "context" + "os" + "path/filepath" + "regexp" + "strings" + + "gitea.dooplex.hu/admin/felhom-agent/internal/hub" +) + +// sysBlockRoot is the sysfs block directory. A package var so tests can point it at a fixture tree. +var sysBlockRoot = "/sys/block" + +// dmWholeDisk resolves a device-mapper / LVM device to its SINGLE backing whole disk, recursing +// through stacked dm layers via /sys/block//slaves (Fix A, SPIKE-smart-coverage-2026-07-25). +// Returns ok=false when the device is not dm, sysfs is missing, there are no slaves, or the slaves +// span MORE THAN ONE physical disk — in that last case we deliberately skip rather than guess which +// of two disks to SMART (e.g. a mirrored LV). +func dmWholeDisk(device string) (string, bool) { + name := dmName(device) + if name == "" { + return "", false + } + disks := map[string]bool{} + if !collectSlaveDisks(name, disks, 0) { + return "", false + } + if len(disks) != 1 { + return "", false // no disk, or an ambiguous multi-disk dm — never guess + } + for d := range disks { + return "/dev/" + d, true + } + return "", false +} + +// dmName maps a dm device path to its sysfs name (dm-N). Handles /dev/dm-N (and a bare dm-N) +// directly, and /dev/mapper/ by matching /sys/block/dm-*/dm/name. +func dmName(device string) string { + base := filepath.Base(device) + if strings.HasPrefix(base, "dm-") { + return base + } + if strings.HasPrefix(device, "/dev/mapper/") { + entries, err := os.ReadDir(sysBlockRoot) + if err != nil { + return "" + } + for _, e := range entries { + if !strings.HasPrefix(e.Name(), "dm-") { + continue + } + b, err := os.ReadFile(filepath.Join(sysBlockRoot, e.Name(), "dm", "name")) + if err == nil && strings.TrimSpace(string(b)) == base { + return e.Name() + } + } + } + return "" +} + +// collectSlaveDisks fills `disks` with the whole-disk names backing dm `name`, recursing through +// nested dm. Returns false on missing sysfs, no slaves, or excessive nesting (loop guard). +func collectSlaveDisks(name string, disks map[string]bool, depth int) bool { + if depth > 8 { + return false + } + entries, err := os.ReadDir(filepath.Join(sysBlockRoot, name, "slaves")) + if err != nil { + return false + } + if len(entries) == 0 { + return false + } + for _, e := range entries { + s := e.Name() + if strings.HasPrefix(s, "dm-") { + if !collectSlaveDisks(s, disks, depth+1) { + return false + } + continue + } + disks[wholeDiskName(s)] = true + } + return true +} + +// wholeDiskName strips a partition suffix to the whole-disk name (sda3→sda, nvme0n1p3→nvme0n1). +func wholeDiskName(part string) string { + if m := reNVMePartName.FindStringSubmatch(part); m != nil { + return m[1] + } + if m := reSDPartName.FindStringSubmatch(part); m != nil { + return m[1] + } + return part +} + +var ( + reNVMePartName = regexp.MustCompile(`^(nvme[0-9]+n[0-9]+)p[0-9]+$`) + reSDPartName = regexp.MustCompile(`^((?:sd|hd|vd)[a-z]+)[0-9]+$`) +) + +// containingMountDevice returns the device of the mount whose mountpoint is the LONGEST prefix of +// path — the filesystem that actually holds `path`. Used ONLY to pick a whole-disk device for a +// SMART read of a dir-storage that lives inside a shared filesystem (the builtin `local` on the LVM +// root); it never feeds durable_id / backing_device (which stay empty for such targets by design — +// the removable-safety guard in build()). +func containingMountDevice(mounts []Mount, path string) (string, bool) { + clean := cleanMountPath(path) + best, bestLen := "", -1 + for _, m := range mounts { + if m.Device == "" { + continue + } + mp := cleanMountPath(m.MountPoint) + if mp == clean || mp == "/" || strings.HasPrefix(clean, strings.TrimRight(mp, "/")+"/") { + if len(mp) > bestLen { + best, bestLen = m.Device, len(mp) + } + } + } + return best, best != "" +} + +// SmartReader reads per-disk SMART for a backing device, resolving partition / dm / LVM down to the +// whole disk. It exists so the localapi /disks UNION path (registry/USB drives that skip Observe's +// enrich) gets the SAME SMART read the dir targets get, without duplicating smartDeviceFor (Fix B). +// A zero-value summary (Health "") means "could not read" (nil ops, unresolvable device, or a read +// error) — distinct from a read that returned UNKNOWN — so the caller can omit it exactly like the +// dir path does. +type SmartReader struct{ ops HostOps } + +// NewSmartReader wraps a HostOps for the localapi union path. +func NewSmartReader(ops HostOps) *SmartReader { return &SmartReader{ops: ops} } + +// SMARTForBacking reads SMART for backingDevice (partition/dm/whole-disk). Never returns an error; +// on any failure the summary's Health is "". +func (r *SmartReader) SMARTForBacking(ctx context.Context, backingDevice string) hub.SmartSummary { + if r == nil || r.ops == nil { + return hub.SmartSummary{} + } + dev, ok := smartDeviceFor(backingDevice) + if !ok { + return hub.SmartSummary{} + } + sm, err := r.ops.SMART(ctx, dev) + if err != nil { + return hub.SmartSummary{} + } + return sm +} diff --git a/internal/storage/smartdev_test.go b/internal/storage/smartdev_test.go new file mode 100644 index 0000000..a03f66e --- /dev/null +++ b/internal/storage/smartdev_test.go @@ -0,0 +1,107 @@ +package storage + +import ( + "os" + "path/filepath" + "testing" + + "gitea.dooplex.hu/admin/felhom-agent/internal/hub" +) + +// fixtureSysfs builds a /sys/block-shaped tree and points sysBlockRoot at it. `slaves` maps a dm +// name to its slave entries; `dmNames` maps a dm name to its /dm/name content (for /dev/mapper/*). +func fixtureSysfs(t *testing.T, slaves map[string][]string, dmNames map[string]string) { + t.Helper() + root := t.TempDir() + for dm, sl := range slaves { + for _, s := range sl { + if err := os.MkdirAll(filepath.Join(root, dm, "slaves", s), 0o755); err != nil { + t.Fatal(err) + } + } + } + for dm, name := range dmNames { + dir := filepath.Join(root, dm, "dm") + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, "name"), []byte(name+"\n"), 0o644); err != nil { + t.Fatal(err) + } + } + old := sysBlockRoot + sysBlockRoot = root + t.Cleanup(func() { sysBlockRoot = old }) +} + +// Fix A dm/LVM resolution. Red-proof: remove the `len(disks) != 1` all-same-disk guard in +// dmWholeDisk → the "mirror over two disks" case resolves to one of them instead of skipping. +func TestDMWholeDisk(t *testing.T) { + cases := []struct { + name string + slaves map[string][]string + dmNames map[string]string + in string + want string + ok bool + }{ + {"single SATA slave", map[string][]string{"dm-1": {"sda3"}}, nil, "/dev/dm-1", "/dev/sda", true}, + {"single NVMe slave", map[string][]string{"dm-0": {"nvme0n1p3"}}, nil, "/dev/dm-0", "/dev/nvme0n1", true}, + {"stacked dm → one disk", map[string][]string{"dm-2": {"dm-1"}, "dm-1": {"sda3"}}, nil, "/dev/dm-2", "/dev/sda", true}, + {"mapper name → dm-1", map[string][]string{"dm-1": {"sda3"}}, map[string]string{"dm-1": "pve-root"}, "/dev/mapper/pve-root", "/dev/sda", true}, + {"mirror over two disks → skip", map[string][]string{"dm-1": {"sda3", "sdb3"}}, nil, "/dev/dm-1", "", false}, + {"no slaves → skip", map[string][]string{"dm-1": {}}, nil, "/dev/dm-1", "", false}, + } + for _, c := range cases { + fixtureSysfs(t, c.slaves, c.dmNames) + got, ok := dmWholeDisk(c.in) + if ok != c.ok || got != c.want { + t.Errorf("%s: dmWholeDisk(%q) = (%q,%v), want (%q,%v)", c.name, c.in, got, ok, c.want, c.ok) + } + } +} + +// smartDeviceFor routes dm/mapper devices through the resolver, and whole-disk/partition through the +// regex path unchanged. +func TestSmartDeviceFor_DMBranch(t *testing.T) { + fixtureSysfs(t, map[string][]string{"dm-1": {"sda3"}}, map[string]string{"dm-1": "pve-root"}) + if dev, ok := smartDeviceFor("/dev/mapper/pve-root"); !ok || dev != "/dev/sda" { + t.Errorf("smartDeviceFor(/dev/mapper/pve-root) = (%q,%v), want (/dev/sda,true)", dev, ok) + } + // missing sysfs → skip, not a guess + fixtureSysfs(t, map[string][]string{}, nil) + if _, ok := smartDeviceFor("/dev/dm-9"); ok { + t.Error("smartDeviceFor should skip an unresolvable dm device") + } +} + +func TestContainingMountDevice(t *testing.T) { + mounts := []Mount{ + {Device: "/dev/mapper/pve-root", MountPoint: "/"}, + {Device: "/dev/sda2", MountPoint: "/boot/efi"}, + {Device: "/dev/sdb1", MountPoint: "/mnt/usb"}, + } + // A dir inside root resolves to root's device (longest prefix wins over "/"). + if dev, ok := containingMountDevice(mounts, "/var/lib/vz"); !ok || dev != "/dev/mapper/pve-root" { + t.Errorf("containing(/var/lib/vz) = (%q,%v), want /dev/mapper/pve-root", dev, ok) + } + // A path under a more-specific mount picks that mount, not root. + if dev, ok := containingMountDevice(mounts, "/mnt/usb/data"); !ok || dev != "/dev/sdb1" { + t.Errorf("containing(/mnt/usb/data) = (%q,%v), want /dev/sdb1", dev, ok) + } +} + +// Model capture (v0.95.0) — smartctl's model_name flows into SmartSummary; absent → nil. +func TestParseSMART_ModelName(t *testing.T) { + withModel := parseSMART([]byte(`{"model_name":"TOSHIBA MQ04ABF100","smart_status":{"passed":true}}`)) + if withModel.ModelName == nil || *withModel.ModelName != "TOSHIBA MQ04ABF100" { + t.Errorf("ModelName = %v, want TOSHIBA MQ04ABF100", withModel.ModelName) + } + if withModel.Health != hub.SmartPassed { + t.Errorf("health = %q, want PASSED", withModel.Health) + } + noModel := parseSMART([]byte(`{"smart_status":{"passed":true}}`)) + if noModel.ModelName != nil { + t.Errorf("absent model_name should be nil, got %v", noModel.ModelName) + } +}