From 2d4d43203f8699e80fde0e252215bf59b74bf963 Mon Sep 17 00:00:00 2001 From: kisfenyo Date: Sun, 14 Jun 2026 17:33:08 +0200 Subject: [PATCH] v0.64.0: additive storage discovery (A1) + internal-SSD label (A2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A1: AutoDiscoverStoragePaths no longer bails on a non-empty registry; registers only deployed-app paths missing from the registry. Never mutates/removes existing entries, never re-adds or reactivates a path present in ANY state (incl. Decommissioned), never flips IsDefault. A2: InferStorageLabel maps base==felhom-data namespace dir to 'Belső SSD (rendszer)' to disambiguate the internal system volume. Table-driven tests incl. a companion that fails without the skip-by-presence guard. Co-Authored-By: Claude Opus 4.8 (1M context) --- CHANGELOG.md | 27 +++ controller/README.md | 11 + controller/internal/settings/settings.go | 112 ++++++--- .../settings/storage_discovery_test.go | 229 ++++++++++++++++++ 4 files changed, 341 insertions(+), 38 deletions(-) create mode 100644 controller/internal/settings/storage_discovery_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 5e7ac04..70b4075 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,32 @@ ## Changelog +### v0.64.0 — storage-lifecycle cleanups (2026-06-14) + +Two settings-layer cleanups from the F9 storage-registration diagnosis +(`felhom.eu/documentation/backlog/DIAGNOSIS-f9-storage-registration-gap-2026-06-14.md`), trunk-based on +`main`, each with table-driven tests that fail on the pre-fix code. + +- **A1 — `AutoDiscoverStoragePaths` is now ADDITIVE** (`internal/settings/settings.go`). It previously bailed + early (`if len(s.StoragePaths) > 0 { return }`), so a drive a deployed app referenced but that was missing + from the registry was never picked up after first run. It now registers only the discovered paths NOT + already present, while honouring strict invariants: never removes/modifies a manually-added path; SKIPS any + path already in the registry IN ANY STATE — including a `Decommissioned` soft-marked entry — so it can't + re-add or reactivate it; never flips `IsDefault` (a newly-discovered path becomes default ONLY if the + registry currently has no default at all, and only the first such new path). NOT auto-register-on-attach — + it only picks up paths deployed apps already reference (that recommendation was rejected; manual enrollment + is by design). New `internal/settings/storage_discovery_test.go` covers it, incl. a companion test that + FAILS if the skip-by-presence guard is removed (verified: removing the guard re-adds the decommissioned + path). +- **A2 — internal-SSD label disambiguation** (`InferStorageLabel`, `internal/settings/settings.go`). A path + whose basename is the `felhom-data` namespace dir (the internal system volume, e.g. + `/mnt/sys_drive/felhom-data`) previously labelled as `Tárhely (felhom-data)`, colliding with the per-drive + felhom-data namespace. It now reads **`Belső SSD (rendszer)`**. Discriminator is `base == + appbackup.FelhomDataDir`; Model-A user drives register their MOUNT ROOT (e.g. `/mnt/felhom-usb`), never + `.../felhom-data`, so this can't mislabel a user drive. Still overridable via `SetStorageLabel`. The demo's + already-seeded `settings.json` label for that path on guest 9201 was updated out-of-band (the seeded value + doesn't auto-change). Separate host-metrics label in `web/agent_host_metrics_handler.go` was intentionally + left untouched. + ### v0.63.0 — reflect agent F9/F20-BUG2 disk fields (2026-06-14) Pass through two new fields the host agent (v0.31.0) now returns on `/disks`, so they reach diff --git a/controller/README.md b/controller/README.md index 7ea96ad..7c4997a 100644 --- a/controller/README.md +++ b/controller/README.md @@ -666,6 +666,17 @@ not just those with HDD data. Non-HDD apps can configure destination, method, an > `FormatResult.{role,needs_confirmation,durable_id}`; `ErrNeedsConfirmation` (user-data) vs > `ErrFormatRefused` (system/backup). `FormatResult.PendingOp.OpsignCommand()` for the operator path. > - The **`StoragePath` registry** (`settings.go`: `AddStoragePath`/default/schedulable/label) is unchanged. +> - **(v0.64.0) `AutoDiscoverStoragePaths` is now ADDITIVE** — it no longer bails when the registry is +> non-empty; instead it registers only deployed-app paths missing from the registry. It never removes +> or mutates an existing entry, never re-adds or reactivates a path already present in ANY state +> (incl. `Decommissioned`), and never flips `IsDefault` (a new path defaults only if the registry has +> no default at all). This is NOT auto-register-on-attach (that recommendation was rejected — manual +> enrollment is by design); it only picks up drives that deployed apps already reference. +> - **(v0.64.0) `InferStorageLabel` disambiguates the internal SSD** — a path whose basename is the +> `felhom-data` namespace dir (the internal system volume, e.g. `/mnt/sys_drive/felhom-data`) now reads +> **"Belső SSD (rendszer)"** instead of the colliding "Tárhely (felhom-data)". Model-A user drives +> register their MOUNT ROOT (e.g. `/mnt/felhom-usb`), never `.../felhom-data`, so this can't mislabel a +> user drive. Still overridable via `SetStorageLabel`. > - **Migration** (drive + per-stack) is **deferred** to its own slice (buttons disabled "Hamarosan"). > > The privileged controller-side disk subsections **below are historical** (the `internal/storage/*` scan/ diff --git a/controller/internal/settings/settings.go b/controller/internal/settings/settings.go index e50e7fb..0ea707f 100644 --- a/controller/internal/settings/settings.go +++ b/controller/internal/settings/settings.go @@ -9,6 +9,8 @@ import ( "strings" "sync" "time" + + "gitea.dooplex.hu/admin/felhom-controller/internal/appbackup" ) // Settings holds customer-modifiable overrides and cached state. @@ -39,8 +41,8 @@ type Settings struct { // Hub verification state HubVerified bool `json:"hub_verified,omitempty"` - HubVerifiedAt string `json:"hub_verified_at,omitempty"` // RFC3339 - HubLastCheck string `json:"hub_last_check,omitempty"` // RFC3339 + HubVerifiedAt string `json:"hub_verified_at,omitempty"` // RFC3339 + HubLastCheck string `json:"hub_last_check,omitempty"` // RFC3339 // Recovery credentials (saved from setup wizard input) RetrievalPassword string `json:"retrieval_password,omitempty"` @@ -59,7 +61,7 @@ type Settings struct { type IntegrationState struct { Enabled bool `json:"enabled"` EnabledAt string `json:"enabled_at,omitempty"` // RFC3339 - Status string `json:"status,omitempty"` // "active", "error", "disabled", "provider_stopped", "target_unavailable" + Status string `json:"status,omitempty"` // "active", "error", "disabled", "provider_stopped", "target_unavailable" LastError string `json:"last_error,omitempty"` } @@ -80,8 +82,8 @@ type CrossDriveBackup struct { Schedule string `json:"schedule"` // "daily", "weekly", "manual" // Runtime state (updated by backup runner, persisted for display) - LastRun string `json:"last_run,omitempty"` // RFC3339 - LastStatus string `json:"last_status,omitempty"` // "ok", "error", "running" + LastRun string `json:"last_run,omitempty"` // RFC3339 + LastStatus string `json:"last_status,omitempty"` // "ok", "error", "running" LastError string `json:"last_error,omitempty"` LastDuration string `json:"last_duration,omitempty"` // "2m34s" LastSizeHuman string `json:"last_size_human,omitempty"` // "1.2 GB" @@ -95,17 +97,17 @@ type CrossDriveBackup struct { // StoragePath represents a registered external storage location. type StoragePath struct { - Path string `json:"path"` // e.g., "/mnt/hdd_1" - Label string `json:"label,omitempty"` // e.g., "Külső HDD 1TB" - IsDefault bool `json:"is_default,omitempty"` // new apps use this by default - Schedulable bool `json:"schedulable"` // whether new apps can be deployed here - AddedAt string `json:"added_at"` // RFC3339 - Disconnected bool `json:"disconnected,omitempty"` // true when drive detected as disconnected - DisconnectedAt string `json:"disconnected_at,omitempty"` // RFC3339 timestamp of disconnect detection - StoppedStacks []string `json:"stopped_stacks,omitempty"` // stacks auto-stopped on disconnect - Decommissioned bool `json:"decommissioned,omitempty"` // true when drive data migrated to another - DecommissionedAt string `json:"decommissioned_at,omitempty"` // RFC3339 timestamp - MigratedTo string `json:"migrated_to,omitempty"` // path of target drive + Path string `json:"path"` // e.g., "/mnt/hdd_1" + Label string `json:"label,omitempty"` // e.g., "Külső HDD 1TB" + IsDefault bool `json:"is_default,omitempty"` // new apps use this by default + Schedulable bool `json:"schedulable"` // whether new apps can be deployed here + AddedAt string `json:"added_at"` // RFC3339 + Disconnected bool `json:"disconnected,omitempty"` // true when drive detected as disconnected + DisconnectedAt string `json:"disconnected_at,omitempty"` // RFC3339 timestamp of disconnect detection + StoppedStacks []string `json:"stopped_stacks,omitempty"` // stacks auto-stopped on disconnect + Decommissioned bool `json:"decommissioned,omitempty"` // true when drive data migrated to another + DecommissionedAt string `json:"decommissioned_at,omitempty"` // RFC3339 timestamp + MigratedTo string `json:"migrated_to,omitempty"` // path of target drive } // NotificationPrefs holds customer notification preferences. @@ -144,10 +146,10 @@ type GeoRestriction struct { AppOverrides map[string]AppGeoOverride `json:"app_overrides,omitempty"` // Sync state (updated by geo sync manager) - LastSync string `json:"last_sync,omitempty"` // RFC3339 + LastSync string `json:"last_sync,omitempty"` // RFC3339 LastSyncError string `json:"last_sync_error,omitempty"` - ZoneID string `json:"zone_id,omitempty"` // cached Cloudflare zone ID - RulesetID string `json:"ruleset_id,omitempty"` // cached Cloudflare ruleset ID + ZoneID string `json:"zone_id,omitempty"` // cached Cloudflare zone ID + RulesetID string `json:"ruleset_id,omitempty"` // cached Cloudflare ruleset ID } // AppGeoOverride holds per-app country override. @@ -157,7 +159,7 @@ type AppGeoOverride struct { // DBValidationCache holds cached DB dump validation results. type DBValidationCache struct { - ValidatedAt string `json:"validated_at"` // RFC3339 + ValidatedAt string `json:"validated_at"` // RFC3339 TableCount int `json:"table_count"` HasHeader bool `json:"has_header"` Error string `json:"error,omitempty"` @@ -578,9 +580,19 @@ func (s *Settings) SetStorageLabel(path, label string) error { return fmt.Errorf("storage path %q not found", path) } -// AutoDiscoverStoragePaths scans for HDD_PATH values and registers them if none exist. -// discoveredPaths are pre-scanned HDD_PATH values from deployed apps' app.yaml. -// fallbackHDDPath is the legacy controller.yaml paths.hdd_path (may be empty). +// AutoDiscoverStoragePaths scans for HDD_PATH values and registers any that are not +// already in the registry. It is ADDITIVE: pre-existing entries are never removed, +// modified, or reactivated. +// - discoveredPaths are pre-scanned HDD_PATH values from deployed apps' app.yaml. +// - fallbackHDDPath is the legacy controller.yaml paths.hdd_path (may be empty). +// +// Invariants: +// - A path already present in the registry IN ANY STATE (including a Decommissioned +// soft-marked entry) is SKIPPED — never re-added and never re-activated. +// - A manually-added path is never removed or modified. +// - IsDefault is never flipped on an existing entry. A newly-discovered path becomes +// default ONLY if the registry currently has no default at all (and then only the +// first such new path). func (s *Settings) AutoDiscoverStoragePaths(discoveredPaths []string, fallbackHDDPath string, logger *log.Logger) { s.mu.Lock() defer s.mu.Unlock() @@ -589,53 +601,77 @@ func (s *Settings) AutoDiscoverStoragePaths(discoveredPaths []string, fallbackHD s.log.Printf("[DEBUG] [settings] AutoDiscoverStoragePaths discovered=%v fallback=%q existing=%d", discoveredPaths, fallbackHDDPath, len(s.StoragePaths)) } - if len(s.StoragePaths) > 0 { - return // already configured + // Index existing paths (in ANY state) and whether a default already exists. + existing := make(map[string]bool, len(s.StoragePaths)) + hasDefault := false + for i := range s.StoragePaths { + existing[filepath.Clean(s.StoragePaths[i].Path)] = true + if s.StoragePaths[i].IsDefault { + hasDefault = true + } } + // Build the de-duplicated, cleaned candidate list (discovered first, then fallback). seen := make(map[string]bool) var ordered []string for _, p := range discoveredPaths { cleaned := filepath.Clean(p) - if cleaned != "" && !seen[cleaned] { + if cleaned != "" && cleaned != "." && !seen[cleaned] { seen[cleaned] = true ordered = append(ordered, cleaned) } } if fallbackHDDPath != "" { cleaned := filepath.Clean(fallbackHDDPath) - if !seen[cleaned] { + if cleaned != "" && cleaned != "." && !seen[cleaned] { seen[cleaned] = true ordered = append(ordered, cleaned) } } - for i, path := range ordered { + added := 0 + for _, path := range ordered { + if existing[path] { + continue // already registered in some state — never re-add or reactivate + } sp := StoragePath{ Path: path, Label: InferStorageLabel(path), - IsDefault: i == 0, + IsDefault: !hasDefault, // first newly-added path defaults only if none exists yet Schedulable: true, AddedAt: time.Now().UTC().Format(time.RFC3339), } + if sp.IsDefault { + hasDefault = true // don't promote a second new path + } s.StoragePaths = append(s.StoragePaths, sp) + existing[path] = true + added++ } - if len(s.StoragePaths) > 0 { - if err := s.save(); err != nil { - logger.Printf("[ERROR] [settings] Failed to save auto-discovered storage paths: %v", err) - return - } - logger.Printf("[INFO] [settings] Auto-discovered %d storage path(s)", len(s.StoragePaths)) - for _, sp := range s.StoragePaths { - logger.Printf("[INFO] [settings] %s (%s) default=%v", sp.Path, sp.Label, sp.IsDefault) - } + if added == 0 { + return // nothing new to register + } + + if err := s.save(); err != nil { + logger.Printf("[ERROR] [settings] Failed to save auto-discovered storage paths: %v", err) + return + } + logger.Printf("[INFO] [settings] Auto-discovered %d new storage path(s)", added) + for _, sp := range s.StoragePaths { + logger.Printf("[INFO] [settings] %s (%s) default=%v decommissioned=%v", sp.Path, sp.Label, sp.IsDefault, sp.Decommissioned) } } // InferStorageLabel generates a human-readable label for a storage path. func InferStorageLabel(path string) string { base := filepath.Base(path) + // The internal system volume's data path ends in the felhom-data namespace dir + // (e.g. /mnt/sys_drive/felhom-data) — Model-A user drives register their MOUNT ROOT + // (e.g. /mnt/felhom-usb), never .../felhom-data, so this can't mislabel a user drive. + if base == appbackup.FelhomDataDir { + return "Belső SSD (rendszer)" + } if strings.HasPrefix(base, "hdd") || strings.HasPrefix(base, "ssd") || strings.HasPrefix(base, "usb") { return fmt.Sprintf("Külső tárhely (%s)", base) } diff --git a/controller/internal/settings/storage_discovery_test.go b/controller/internal/settings/storage_discovery_test.go new file mode 100644 index 0000000..f82b897 --- /dev/null +++ b/controller/internal/settings/storage_discovery_test.go @@ -0,0 +1,229 @@ +package settings + +import ( + "io" + "log" + "path/filepath" + "reflect" + "testing" +) + +// newTestSettings returns a Settings backed by a writable temp file so save() works. +func newTestSettings(t *testing.T, paths []StoragePath) *Settings { + t.Helper() + logger := log.New(io.Discard, "", 0) + s, err := Load(filepath.Join(t.TempDir(), "settings.json"), logger) + if err != nil { + t.Fatalf("Load: %v", err) + } + s.StoragePaths = paths + return s +} + +// TestAutoDiscoverStoragePaths_Additive covers the additive registration behaviour and +// its invariants: existing entries are never mutated, decommissioned entries are never +// reactivated, and IsDefault is never promoted over an existing default. +func TestAutoDiscoverStoragePaths_Additive(t *testing.T) { + logger := log.New(io.Discard, "", 0) + + tests := []struct { + name string + existing []StoragePath + discovered []string + fallback string + wantPaths []string // expected Path set after discovery (order-insensitive) + wantNewPath string // a path expected to be newly added (may be "") + // assertions run against the resulting registry + check func(t *testing.T, s *Settings, before []StoragePath) + }{ + { + name: "non-empty registry registers only the missing deployed path", + existing: []StoragePath{ + {Path: "/mnt/felhom-usb", Label: "Külső tárhely (felhom-usb)", IsDefault: true, Schedulable: true, AddedAt: "2026-01-01T00:00:00Z"}, + }, + discovered: []string{"/mnt/felhom-usb", "/mnt/hdd_2"}, // existing + a new one + wantPaths: []string{"/mnt/felhom-usb", "/mnt/hdd_2"}, + wantNewPath: "/mnt/hdd_2", + check: func(t *testing.T, s *Settings, before []StoragePath) { + // pre-existing entry must be byte-identical (incl. IsDefault unchanged) + got := findPath(s, "/mnt/felhom-usb") + if got == nil { + t.Fatalf("pre-existing path vanished") + } + if !reflect.DeepEqual(*got, before[0]) { + t.Errorf("pre-existing entry mutated:\n before=%+v\n after =%+v", before[0], *got) + } + // the newly-added path must NOT have stolen default + nw := findPath(s, "/mnt/hdd_2") + if nw == nil { + t.Fatalf("new path /mnt/hdd_2 not registered") + } + if nw.IsDefault { + t.Errorf("new path promoted to default over existing default") + } + if !nw.Schedulable { + t.Errorf("new path should be schedulable") + } + }, + }, + { + name: "registry with no default lets first new path become default", + existing: []StoragePath{ + {Path: "/mnt/felhom-usb", Label: "x", IsDefault: false, Schedulable: true, AddedAt: "2026-01-01T00:00:00Z"}, + }, + discovered: []string{"/mnt/hdd_2", "/mnt/hdd_3"}, + wantPaths: []string{"/mnt/felhom-usb", "/mnt/hdd_2", "/mnt/hdd_3"}, + wantNewPath: "/mnt/hdd_2", + check: func(t *testing.T, s *Settings, before []StoragePath) { + d2 := findPath(s, "/mnt/hdd_2") + d3 := findPath(s, "/mnt/hdd_3") + if d2 == nil || d3 == nil { + t.Fatalf("new paths not registered") + } + if !d2.IsDefault { + t.Errorf("first new path should become default when registry has none") + } + if d3.IsDefault { + t.Errorf("only one new path may become default") + } + }, + }, + { + name: "empty registry behaves like the original (first becomes default)", + existing: nil, + discovered: []string{"/mnt/felhom-usb", "/mnt/hdd_2"}, + wantPaths: []string{"/mnt/felhom-usb", "/mnt/hdd_2"}, + wantNewPath: "/mnt/felhom-usb", + check: func(t *testing.T, s *Settings, before []StoragePath) { + first := findPath(s, "/mnt/felhom-usb") + if first == nil || !first.IsDefault { + t.Errorf("first discovered path should be default in an empty registry") + } + }, + }, + { + name: "fallback path registered when missing", + existing: []StoragePath{ + {Path: "/mnt/felhom-usb", Label: "x", IsDefault: true, Schedulable: true, AddedAt: "2026-01-01T00:00:00Z"}, + }, + discovered: nil, + fallback: "/mnt/legacy_hdd", + wantPaths: []string{"/mnt/felhom-usb", "/mnt/legacy_hdd"}, + wantNewPath: "/mnt/legacy_hdd", + check: nil, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + s := newTestSettings(t, cloneStoragePaths(tc.existing)) + before := cloneStoragePaths(tc.existing) + + s.AutoDiscoverStoragePaths(tc.discovered, tc.fallback, logger) + + // Normalize with filepath.Clean so comparisons hold on both Linux (the deploy + // target) and Windows (the dev machine), where Clean uses backslashes. + gotSet := map[string]bool{} + for _, sp := range s.StoragePaths { + gotSet[filepath.Clean(sp.Path)] = true + } + if len(gotSet) != len(tc.wantPaths) { + t.Fatalf("path count = %d, want %d (%v)", len(gotSet), len(tc.wantPaths), pathList(s)) + } + for _, w := range tc.wantPaths { + if !gotSet[filepath.Clean(w)] { + t.Errorf("missing expected path %q (have %v)", w, pathList(s)) + } + } + if tc.wantNewPath != "" && findPath(s, tc.wantNewPath) == nil { + t.Errorf("expected new path %q to be registered", tc.wantNewPath) + } + if tc.check != nil { + tc.check(t, s, before) + } + }) + } +} + +// TestAutoDiscoverStoragePaths_DecommissionedNotReactivated is the companion guard: a +// decommissioned path that is still referenced by a deployed app must NOT be re-added or +// reactivated. This test FAILS if the skip-by-presence guard is removed. +func TestAutoDiscoverStoragePaths_DecommissionedNotReactivated(t *testing.T) { + logger := log.New(io.Discard, "", 0) + existing := []StoragePath{ + {Path: "/mnt/old_hdd", Label: "Külső tárhely (old_hdd)", IsDefault: false, Schedulable: true, AddedAt: "2026-01-01T00:00:00Z", Decommissioned: true, DecommissionedAt: "2026-02-01T00:00:00Z", MigratedTo: "/mnt/felhom-usb"}, + {Path: "/mnt/felhom-usb", Label: "x", IsDefault: true, Schedulable: true, AddedAt: "2026-01-01T00:00:00Z"}, + } + s := newTestSettings(t, cloneStoragePaths(existing)) + before := cloneStoragePaths(existing) + + // A deployed app still points at the decommissioned drive. + s.AutoDiscoverStoragePaths([]string{"/mnt/old_hdd", "/mnt/felhom-usb"}, "", logger) + + if len(s.StoragePaths) != 2 { + t.Fatalf("path count changed: got %d want 2 (%v)", len(s.StoragePaths), pathList(s)) + } + got := findPath(s, "/mnt/old_hdd") + if got == nil { + t.Fatalf("decommissioned path vanished") + } + if !got.Decommissioned { + t.Errorf("decommissioned path was REACTIVATED (skip-by-presence guard missing)") + } + if !reflect.DeepEqual(*got, before[0]) { + t.Errorf("decommissioned entry mutated:\n before=%+v\n after =%+v", before[0], *got) + } +} + +func TestInferStorageLabel(t *testing.T) { + tests := []struct { + path string + want string + }{ + {"/mnt/sys_drive/felhom-data", "Belső SSD (rendszer)"}, + {"/var/lib/felhom/felhom-data", "Belső SSD (rendszer)"}, + {"/mnt/felhom-usb", "Tárhely (felhom-usb)"}, // "felhom-usb" doesn't start with "usb" + {"/mnt/hdd_1", "Külső tárhely (hdd_1)"}, + {"/mnt/ssd_data", "Külső tárhely (ssd_data)"}, + {"/srv/backups", "Tárhely (backups)"}, + } + for _, tc := range tests { + if got := InferStorageLabel(tc.path); got != tc.want { + t.Errorf("InferStorageLabel(%q) = %q, want %q", tc.path, got, tc.want) + } + } +} + +// --- helpers --- + +func cloneStoragePaths(in []StoragePath) []StoragePath { + if in == nil { + return nil + } + out := make([]StoragePath, len(in)) + copy(out, in) + for i := range out { + if in[i].StoppedStacks != nil { + out[i].StoppedStacks = append([]string(nil), in[i].StoppedStacks...) + } + } + return out +} + +func findPath(s *Settings, path string) *StoragePath { + want := filepath.Clean(path) + for i := range s.StoragePaths { + if filepath.Clean(s.StoragePaths[i].Path) == want { + return &s.StoragePaths[i] + } + } + return nil +} + +func pathList(s *Settings) []string { + var out []string + for _, sp := range s.StoragePaths { + out = append(out, sp.Path) + } + return out +}