From 8a240f8312bfe76de450453a2582c64d277de08a Mon Sep 17 00:00:00 2001 From: rcourtman Date: Sat, 28 Mar 2026 00:04:12 +0000 Subject: [PATCH] fix(recovery): prefer canonical item names over raw ids --- .../internal/subsystems/storage-recovery.md | 6 ++ internal/recovery/index.go | 64 ++++++++++++++ internal/recovery/mapper/proxmox/mapper.go | 28 ++++++- .../recovery/mapper/proxmox/mapper_test.go | 30 +++++++ internal/recovery/recovery_test.go | 29 +++++++ .../recovery/store/store_index_backfill.go | 7 +- internal/recovery/store/store_test.go | 83 +++++++++++++++++++ 7 files changed, 245 insertions(+), 2 deletions(-) diff --git a/docs/release-control/v6/internal/subsystems/storage-recovery.md b/docs/release-control/v6/internal/subsystems/storage-recovery.md index ee7197a95e..7132aaa0db 100644 --- a/docs/release-control/v6/internal/subsystems/storage-recovery.md +++ b/docs/release-control/v6/internal/subsystems/storage-recovery.md @@ -245,6 +245,12 @@ and `display.itemType` to recovery consumers, while legacy transport fields such as `subjectLabel` and `subjectType` remain decode-only compatibility aliases in the shared normalization layer instead of leaking into runtime presenters. +That same canonical item-label boundary must prefer recognizable protected-item +names over raw entity IDs. When unresolved Proxmox-backed recovery points only +have a VMID/CTID in the subject ref but still carry a richer backup comment or +notes label, the canonical recovery index and store backfill must promote that +human-readable label into the persisted subject/item label instead of leaving +protected inventory rows to lead with bare numeric IDs. That same shared presentation layer also owns the distinction between aggregate recovery-method language and single-record recovery-method language. Timeline legends and daily breakdowns must use aggregate labels such as diff --git a/internal/recovery/index.go b/internal/recovery/index.go index d2274f967e..18c7598e59 100644 --- a/internal/recovery/index.go +++ b/internal/recovery/index.go @@ -56,6 +56,61 @@ func detailsString(p RecoveryPoint, key string) string { return trimString(p.Details[key]) } +func isNumericOnlyLabel(value string) bool { + value = strings.TrimSpace(value) + if value == "" { + return false + } + for _, r := range value { + if r < '0' || r > '9' { + return false + } + } + return true +} + +func preferredProxmoxBackupCommentLabel(comment, entityID string) string { + comment = strings.TrimSpace(comment) + entityID = strings.TrimSpace(entityID) + if comment == "" { + return "" + } + if entityID != "" { + if comment == entityID { + return "" + } + parts := strings.Split(comment, ",") + if len(parts) >= 2 { + first := strings.TrimSpace(parts[0]) + last := strings.TrimSpace(parts[len(parts)-1]) + if last == entityID && first != "" && first != entityID { + return first + } + } + } + return comment +} + +func preferredSubjectLabelFromDetails(p RecoveryPoint, currentLabel string) string { + currentLabel = strings.TrimSpace(currentLabel) + if !strings.HasPrefix(string(p.Provider), "proxmox-") { + return "" + } + + entityID := entityIDLabel(p) + if currentLabel != "" && currentLabel != entityID && !isNumericOnlyLabel(currentLabel) { + return "" + } + + for _, key := range []string{"comment", "notes"} { + if candidate := preferredProxmoxBackupCommentLabel(detailsString(p, key), entityID); candidate != "" { + return candidate + } + } + + return "" +} + func NormalizeRecoveryItemType(value string) string { t := strings.ToLower(strings.TrimSpace(value)) switch t { @@ -121,9 +176,15 @@ func subjectLabel(p RecoveryPoint) string { } } + if candidate := preferredSubjectLabelFromDetails(p, name); candidate != "" { + return candidate + } if name != "" { return name } + if candidate := preferredSubjectLabelFromDetails(p, id); candidate != "" { + return candidate + } if ns != "" { return ns } @@ -137,6 +198,9 @@ func subjectLabel(p RecoveryPoint) string { if rid != "" { return rid } + if candidate := preferredSubjectLabelFromDetails(p, ""); candidate != "" { + return candidate + } return strings.TrimSpace(p.ID) } diff --git a/internal/recovery/mapper/proxmox/mapper.go b/internal/recovery/mapper/proxmox/mapper.go index 6ff79e38e5..9d2e74f27b 100644 --- a/internal/recovery/mapper/proxmox/mapper.go +++ b/internal/recovery/mapper/proxmox/mapper.go @@ -36,6 +36,28 @@ func guestLookupKey(instanceName, nodeName string, vmid int) string { return fmt.Sprintf("%s|%s|%d", strings.TrimSpace(instanceName), strings.TrimSpace(nodeName), vmid) } +func preferredPBSBackupSubjectName(comment, vmid string) string { + comment = strings.TrimSpace(comment) + vmid = strings.TrimSpace(vmid) + if comment == "" { + return "" + } + if vmid != "" { + if comment == vmid { + return "" + } + parts := strings.Split(comment, ",") + if len(parts) >= 2 { + last := strings.TrimSpace(parts[len(parts)-1]) + first := strings.TrimSpace(parts[0]) + if last == vmid && first != "" && first != vmid { + return first + } + } + } + return comment +} + func proxmoxSubjectRef(resourceType unifiedresources.ResourceType, info GuestInfo, instanceName, nodeName string, vmid int, sourceID string) *recovery.ExternalRef { name := strings.TrimSpace(info.Name) if name == "" && vmid > 0 { @@ -344,10 +366,14 @@ func FromPBSBackups(backups []models.PBSBackup, candidatesByKey map[string][]Gue } else if guestType == "ct" { refType = "proxmox-lxc" } + subjectName := preferredPBSBackupSubjectName(b.Comment, vmidStr) + if subjectName == "" { + subjectName = vmidStr + } subjectRef = &recovery.ExternalRef{ Type: refType, Namespace: strings.TrimSpace(b.Instance), - Name: vmidStr, + Name: subjectName, ID: vmidStr, Class: "", } diff --git a/internal/recovery/mapper/proxmox/mapper_test.go b/internal/recovery/mapper/proxmox/mapper_test.go index 81db438d18..7cb7b3f07f 100644 --- a/internal/recovery/mapper/proxmox/mapper_test.go +++ b/internal/recovery/mapper/proxmox/mapper_test.go @@ -249,6 +249,36 @@ func TestFromPBSBackups_Single(t *testing.T) { } } +func TestFromPBSBackups_PrefersCommentNameWhenGuestIsUnresolved(t *testing.T) { + backups := []models.PBSBackup{ + { + ID: "pbs-backup-legacy", + VMID: "140", + Instance: "pbs-docker", + Namespace: "pimox", + Datastore: "main", + BackupType: "ct", + BackupTime: time.Date(2024, 1, 15, 10, 0, 0, 0, time.UTC), + Comment: "pulse-v4-prod, pi, 140", + }, + } + + result := FromPBSBackups(backups, nil) + + if len(result) != 1 { + t.Fatalf("expected 1 point, got %d", len(result)) + } + if result[0].SubjectRef == nil { + t.Fatal("expected SubjectRef to be set") + } + if got := result[0].SubjectRef.Name; got != "pulse-v4-prod" { + t.Fatalf("SubjectRef.Name = %q, want %q", got, "pulse-v4-prod") + } + if got := result[0].SubjectRef.ID; got != "140" { + t.Fatalf("SubjectRef.ID = %q, want %q", got, "140") + } +} + func TestFromPBSBackups_WithCandidates(t *testing.T) { backups := []models.PBSBackup{ { diff --git a/internal/recovery/recovery_test.go b/internal/recovery/recovery_test.go index e832a0153e..5ffa297ac9 100644 --- a/internal/recovery/recovery_test.go +++ b/internal/recovery/recovery_test.go @@ -140,6 +140,35 @@ func TestDeriveIndex(t *testing.T) { DetailsSummary: "local:backup/vm-100/2024-01-15_00:00:00", }, }, + { + name: "PBS falls back to backup comment when ref name is only vmid", + point: RecoveryPoint{ + ID: "pbs-backup-legacy-140", + Provider: ProviderProxmoxPBS, + SubjectRef: &ExternalRef{ + Type: "proxmox-lxc", + Namespace: "pbs-docker", + Name: "140", + ID: "140", + }, + Details: map[string]any{ + "comment": "pulse-v4-prod, pi, 140", + "vmid": "140", + }, + }, + expected: PointIndex{ + SubjectLabel: "pulse-v4-prod", + SubjectType: "proxmox-lxc", + ItemType: "system-container", + IsWorkload: true, + ClusterLabel: "pbs-docker", + NodeHostLabel: "", + NamespaceLabel: "root", + EntityIDLabel: "140", + RepositoryLabel: "", + DetailsSummary: "pulse-v4-prod, pi, 140", + }, + }, { name: "TrueNAS with hostname", point: RecoveryPoint{ diff --git a/internal/recovery/store/store_index_backfill.go b/internal/recovery/store/store_index_backfill.go index 844303978f..08665b25a1 100644 --- a/internal/recovery/store/store_index_backfill.go +++ b/internal/recovery/store/store_index_backfill.go @@ -37,7 +37,12 @@ func (s *Store) BackfillIndex(ctx context.Context) error { namespace_label IS NULL OR entity_id_label IS NULL OR repository_label IS NULL OR - details_summary IS NULL) + details_summary IS NULL OR + (provider IN ('proxmox-pve', 'proxmox-pbs') AND + subject_label IS NOT NULL AND TRIM(subject_label) <> '' AND + entity_id_label IS NOT NULL AND TRIM(entity_id_label) <> '' AND + TRIM(subject_label) = TRIM(entity_id_label) AND + details_json IS NOT NULL AND TRIM(details_json) <> '')) LIMIT `+fmt.Sprint(maxBackfillRows)+` `) if err != nil { diff --git a/internal/recovery/store/store_test.go b/internal/recovery/store/store_test.go index 20b4658b31..a5a74206b2 100644 --- a/internal/recovery/store/store_test.go +++ b/internal/recovery/store/store_test.go @@ -204,6 +204,89 @@ func TestStore_OpenMigratesLegacySchemaMissingItemType(t *testing.T) { } } +func TestStore_OpenBackfillsLegacyNumericPBSSubjectLabels(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + dbPath := filepath.Join(dir, "recovery.db") + + store, err := Open(dbPath) + if err != nil { + t.Fatalf("Open() error = %v", err) + } + + now := time.Date(2026, 3, 27, 12, 0, 0, 0, time.UTC) + point := recovery.RecoveryPoint{ + ID: "pbs-backup-legacy-140", + Provider: recovery.ProviderProxmoxPBS, + Kind: recovery.KindBackup, + Mode: recovery.ModeRemote, + Outcome: recovery.OutcomeSuccess, + StartedAt: &now, + CompletedAt: &now, + SubjectRef: &recovery.ExternalRef{ + Type: "proxmox-lxc", + Namespace: "pbs-docker", + Name: "pulse-v4-prod", + ID: "140", + }, + Details: map[string]any{ + "comment": "pulse-v4-prod, pi, 140", + "vmid": "140", + "backupType": "ct", + }, + } + + if err := store.UpsertPoints(context.Background(), []recovery.RecoveryPoint{point}); err != nil { + t.Fatalf("UpsertPoints() error = %v", err) + } + + if _, err := store.db.ExecContext( + context.Background(), + `UPDATE recovery_points + SET subject_label = ?, entity_id_label = ?, details_summary = ? + WHERE id = ?`, + "140", + "140", + "pulse-v4-prod, pi, 140", + point.ID, + ); err != nil { + t.Fatalf("degrade legacy numeric pbs label: %v", err) + } + + if err := store.Close(); err != nil { + t.Fatalf("Close() error = %v", err) + } + + reopened, err := Open(dbPath) + if err != nil { + t.Fatalf("reopen Open() error = %v", err) + } + t.Cleanup(func() { _ = reopened.Close() }) + + points, total, err := reopened.ListPoints(context.Background(), recovery.ListPointsOptions{Page: 1, Limit: 50}) + if err != nil { + t.Fatalf("ListPoints() error = %v", err) + } + if total != 1 || len(points) != 1 { + t.Fatalf("ListPoints() total=%d len=%d, want 1/1", total, len(points)) + } + if points[0].Display == nil || points[0].Display.SubjectLabel != "pulse-v4-prod" { + t.Fatalf("ListPoints() display = %#v, want backfilled subject label pulse-v4-prod", points[0].Display) + } + + rollups, rollupTotal, err := reopened.ListRollups(context.Background(), recovery.ListPointsOptions{Page: 1, Limit: 50}) + if err != nil { + t.Fatalf("ListRollups() error = %v", err) + } + if rollupTotal != 1 || len(rollups) != 1 { + t.Fatalf("ListRollups() total=%d len=%d, want 1/1", rollupTotal, len(rollups)) + } + if rollups[0].Display == nil || rollups[0].Display.SubjectLabel != "pulse-v4-prod" { + t.Fatalf("ListRollups() display = %#v, want backfilled subject label pulse-v4-prod", rollups[0].Display) + } +} + func createLegacyRecoveryDBWithoutItemType(t *testing.T, dbPath string, point recovery.RecoveryPoint) { t.Helper()