From 4782186bd6a111b1a43e4dcbce4008679a6c689a Mon Sep 17 00:00:00 2001 From: rcourtman Date: Sat, 28 Mar 2026 00:45:17 +0000 Subject: [PATCH] refactor(recovery): improve scan-first recovery identity --- .../internal/subsystems/storage-recovery.md | 9 + .../Recovery/RecoverySummary.test.tsx | 8 +- .../components/Recovery/RecoverySummary.tsx | 280 +++++++++--------- .../Recovery/__tests__/Recovery.test.tsx | 4 +- .../recoveryItemTypePresentation.test.ts | 4 + internal/recovery/index.go | 67 ++++- internal/recovery/recovery_test.go | 29 ++ .../recovery/store/store_index_backfill.go | 4 +- internal/recovery/store/store_test.go | 74 +++++ 9 files changed, 326 insertions(+), 153 deletions(-) diff --git a/docs/release-control/v6/internal/subsystems/storage-recovery.md b/docs/release-control/v6/internal/subsystems/storage-recovery.md index 480281bb5..a47a3f2f5 100644 --- a/docs/release-control/v6/internal/subsystems/storage-recovery.md +++ b/docs/release-control/v6/internal/subsystems/storage-recovery.md @@ -667,6 +667,10 @@ That same summary rule also applies within individual cards: recovery posture, freshness, attention, footprint, and history cards should favor compact rows and metric lists over stacked prose callouts so the summary strip reads like Pulse monitoring telemetry rather than a page-local narrative panel. +That same card-level scan rule should prefer one dominant metric per card with +short supporting readouts, the same quick-scan rhythm operators already get on +infrastructure and workloads, instead of nested sub-cards that turn recovery +summary into a denser bespoke dashboard than the rest of Pulse. That same inventory surface should also follow the established monitoring-table scan pattern in its first column. Protected-item rows should lead with a clear status cue, the primary item name, and compact badge-backed item/platform @@ -676,6 +680,11 @@ That same row contract should avoid duplicating context that already has a dedicated column. When `Item Type` and `Platform` columns are visible, the primary item cell should not restate those same badges on desktop; duplicate context belongs only as a small-screen fallback when those columns collapse. +That same item-identity contract also applies to synthetic Proxmox task +recovery points. When the persisted subject label is just a raw +`pve-task:*`/`UPID:*` identifier or `vmid=0`, the canonical recovery index +should derive a readable task label and `task` item type from point details so +recovery tables scan by operator meaning instead of transport IDs. That same inventory surface should stay on the flat monitoring-table pattern already used elsewhere in Pulse. Protected items should surface posture through row-level status cues, outcome pills, and filters rather than inserting extra diff --git a/frontend-modern/src/components/Recovery/RecoverySummary.test.tsx b/frontend-modern/src/components/Recovery/RecoverySummary.test.tsx index f4c6dd18b..41a709cb3 100644 --- a/frontend-modern/src/components/Recovery/RecoverySummary.test.tsx +++ b/frontend-modern/src/components/Recovery/RecoverySummary.test.tsx @@ -57,16 +57,16 @@ describe('RecoverySummary', () => { expect(screen.getByText('Protected Footprint')).toBeInTheDocument(); expect(screen.getByText('Freshness')).toBeInTheDocument(); expect(screen.getByText('Recent History')).toBeInTheDocument(); - expect(screen.getAllByText('Stale').length).toBeGreaterThan(0); expect(screen.getAllByText('Attention').length).toBeGreaterThan(0); - expect(screen.getByText('Recovery Points')).toBeInTheDocument(); + expect(screen.getByText(/recovery points/i)).toBeInTheDocument(); expect(screen.getAllByText('Item Types').length).toBeGreaterThan(0); expect(screen.getByText('Primary Item')).toBeInTheDocument(); expect(screen.getByText('Primary Platform')).toBeInTheDocument(); - expect(screen.getByText('Platform Mix')).toBeInTheDocument(); expect(screen.getByText('Avg / Day')).toBeInTheDocument(); expect(screen.getByText('2 protected')).toBeInTheDocument(); expect(screen.getByText('1 attention')).toBeInTheDocument(); - expect(screen.getAllByText('Never succeeded').length).toBeGreaterThan(0); + expect(screen.getAllByText(/Never Succeeded/i).length).toBeGreaterThan(0); + expect(screen.getByText('need attention')).toBeInTheDocument(); + expect(screen.getByText('stale items')).toBeInTheDocument(); }); }); diff --git a/frontend-modern/src/components/Recovery/RecoverySummary.tsx b/frontend-modern/src/components/Recovery/RecoverySummary.tsx index 7f6e909f4..7c6fa48d3 100644 --- a/frontend-modern/src/components/Recovery/RecoverySummary.tsx +++ b/frontend-modern/src/components/Recovery/RecoverySummary.tsx @@ -10,7 +10,6 @@ import { buildRecoveryPlatformCoverage, buildRecoveryPostureSegments, buildRecoveryPostureSummary, - getRecoveryAttentionDotClass, RECOVERY_SUMMARY_TIME_RANGES, RECOVERY_SUMMARY_TIME_RANGE_LABELS, type RecoverySummaryTimeRange, @@ -43,35 +42,35 @@ export const RecoverySummary: Component = (props) => { const activity = createMemo(() => buildRecoveryActivitySummary(props.series())); const healthyCount = createMemo(() => postureSummary().healthy); const attentionCount = createMemo(() => postureSummary().attention); + const primaryPostureMetric = createMemo(() => { + if (attentionCount() > 0) { + return { + value: attentionCount(), + label: 'need attention', + valueClass: 'text-amber-600 dark:text-amber-400', + }; + } + if (postureSummary().running > 0) { + return { + value: postureSummary().running, + label: 'currently running', + valueClass: 'text-blue-600 dark:text-blue-400', + }; + } + return { + value: healthyCount(), + label: 'healthy items', + valueClass: 'text-emerald-600 dark:text-emerald-400', + }; + }); + const visiblePostureSegments = createMemo(() => + postureSegments().filter((segment) => segment.count > 0).slice(0, 4), + ); const recentWindowLabel = createMemo(() => { const activitySummary = activity(); if (!activitySummary.startLabel || !activitySummary.endLabel) return null; return `${activitySummary.startLabel} to ${activitySummary.endLabel}`; }); - const attentionItems = createMemo(() => - [ - { - label: 'Stale', - count: summary().stale, - tone: 'amber', - }, - { - label: 'Never succeeded', - count: summary().neverSucceeded, - tone: 'rose', - }, - { - label: 'Attention', - count: attentionCount(), - tone: 'amber', - }, - { - label: 'Running', - count: postureSummary().running, - tone: 'blue', - }, - ].filter((item) => item.count > 0), - ); const handleTimeRangeChange = (range: string) => props.onTimeRangeChange?.(range as RecoverySummaryTimeRange); @@ -108,30 +107,26 @@ export const RecoverySummary: Component = (props) => { class="overflow-hidden" > -
-
-
-
Healthy
-
- {healthyCount()} +
+
+
+
+ {primaryPostureMetric().value}
+
{primaryPostureMetric().label}
-
-
Attention
-
- {attentionCount()} +
+
+ {summary().total}
-
-
-
Protected
-
{summary().total}
+
protected items
- + {(segment) => (
= (props) => {
- + {(segment) => (
@@ -162,121 +157,117 @@ export const RecoverySummary: Component = (props) => { -
-
-
- Stale - +
+
+
+
{summary().stale} - +
+
stale items
-
- Never succeeded - - {summary().neverSucceeded} - -
-
- Running - - {postureSummary().running} - -
-
- Attention - - {attentionCount()} - +
+
+ + {summary().neverSucceeded} + {' '} + never succeeded +
+
+ + {postureSummary().running} + {' '} + running +
-
+
{(bucket) => ( -
- - {bucket.label} - {bucket.count} +
+
+ + {bucket.label} +
+ {bucket.count}
)} - - {(item) => ( - -
- - {item.label} - {item.count} -
- - )} - +
+ +
+
+
Attention
+
{attentionCount()}
+
+
+
Fresh <24h
+
+ {freshnessBuckets() + .filter((bucket) => bucket.key === 'under1h' || bucket.key === 'under24h') + .reduce((total, bucket) => total + bucket.count, 0)} +
+
-
-
-
-
Item Types
-
{itemCoverage().itemTypeCount}
+
+
+
+
Item Types
+
+ {itemCoverage().itemTypeCount} +
-
-
Primary Item
-
+
+
Platforms
+
+ {platformCoverage().platformCount} +
+
+
+ +
+
+
Primary Item
+
{itemCoverage().primaryItemLabel ?? 'n/a'}
-
-
Platforms
-
{platformCoverage().platformCount}
-
-
Primary Platform
-
+
Primary Platform
+
{platformCoverage().primaryPlatformLabel ?? 'n/a'}
- 0}> -
- +
+ 0}> +
+ {(item) => ( -
+
{item.label} {item.count} - {item.percent}%
)} -
- - -
-
- Platform Mix -
- 0}> -
- {platformCoverage().multiPlatformCount} multi-platform item - {platformCoverage().multiPlatformCount === 1 ? '' : 's'}
+
- + {(item) => { const badge = getSourcePlatformBadge(item.key); return ( -
+
{badge?.label || item.label} {item.count} - {item.percent}%
); }} @@ -292,47 +283,48 @@ export const RecoverySummary: Component = (props) => { hasData={activity().hasData} emptyMessage={props.seriesFailed?.() ? 'Trend data unavailable' : 'No recovery activity yet'} > -
-
+
+
+
+
+ {activity().totalEvents} +
+
recovery points
+
-
-
Window
-
{recentWindowLabel()}
+
+ {recentWindowLabel()}
-
-
Recovery Points
-
{activity().totalEvents}
-
-
-
Days Active
-
{activity().activeDays}
-
-
-
Peak Day
-
{activity().busiestLabel ?? 'n/a'}
-
-
-
Latest Activity
-
{activity().latestLabel ?? 'n/a'}
-
-
-
+
+ +
-
Avg / Day
+
Days Active
+
{activity().activeDays}
+
+
+
Avg / Day
{activity().averagePerDay.toFixed(1)}
-
Peak
+
Peak
{activity().busiestCount}
-
-
Latest
-
{activity().latestCount}
-
+ +
+
+
Peak Day
+
{activity().busiestLabel ?? 'n/a'}
+
+
+
Latest Activity
+
{activity().latestLabel ?? 'n/a'}
+
+
diff --git a/frontend-modern/src/components/Recovery/__tests__/Recovery.test.tsx b/frontend-modern/src/components/Recovery/__tests__/Recovery.test.tsx index 3620abe8e..6ed62af74 100644 --- a/frontend-modern/src/components/Recovery/__tests__/Recovery.test.tsx +++ b/frontend-modern/src/components/Recovery/__tests__/Recovery.test.tsx @@ -331,10 +331,10 @@ describe('Recovery', () => { it('surfaces item-first recovery coverage in the unified summary', async () => { render(() => ); - await screen.findByText('Platform Mix'); + await screen.findByText('Protected Footprint'); expect(screen.getByText('Primary Item')).toBeInTheDocument(); expect(screen.getByText('Primary Platform')).toBeInTheDocument(); - expect(screen.getByText('Platform Mix')).toBeInTheDocument(); + expect(screen.getByText('Platforms')).toBeInTheDocument(); }); it('normalizes legacy provider-shaped recovery payloads before rendering', async () => { diff --git a/frontend-modern/src/utils/__tests__/recoveryItemTypePresentation.test.ts b/frontend-modern/src/utils/__tests__/recoveryItemTypePresentation.test.ts index dc9298ab0..a46033873 100644 --- a/frontend-modern/src/utils/__tests__/recoveryItemTypePresentation.test.ts +++ b/frontend-modern/src/utils/__tests__/recoveryItemTypePresentation.test.ts @@ -37,6 +37,10 @@ describe('recoveryItemTypePresentation', () => { key: 'dataset', label: 'Dataset', }); + expect(getRecoveryItemTypePresentation('task')).toMatchObject({ + key: 'task', + label: 'Task', + }); }); it('falls back cleanly for unknown item types', () => { diff --git a/internal/recovery/index.go b/internal/recovery/index.go index 18c7598e5..aaf7f09b7 100644 --- a/internal/recovery/index.go +++ b/internal/recovery/index.go @@ -91,6 +91,51 @@ func preferredProxmoxBackupCommentLabel(comment, entityID string) string { return comment } +func isOpaqueProxmoxTaskLabel(value string) bool { + value = strings.TrimSpace(strings.ToLower(value)) + if value == "" { + return false + } + return strings.HasPrefix(value, "pve-task:") || strings.Contains(value, "upid:") +} + +func preferredProxmoxTaskLabelFromDetails(p RecoveryPoint, currentLabel string) string { + if strings.TrimSpace(string(p.Provider)) != string(ProviderProxmoxPVE) { + return "" + } + currentLabel = strings.TrimSpace(currentLabel) + if currentLabel != "" && !isOpaqueProxmoxTaskLabel(currentLabel) { + return "" + } + + taskType := strings.ToLower(strings.TrimSpace(detailsString(p, "type"))) + baseLabel := "" + switch taskType { + case "vzdump", "backup": + baseLabel = "backup task" + case "": + baseLabel = "task" + default: + baseLabel = strings.ReplaceAll(taskType, "-", " ") + " task" + } + + entityID := entityIDLabel(p) + node := nodeHostLabel(p) + if entityID != "" { + if node != "" { + return fmt.Sprintf("%s guest %s %s", node, entityID, baseLabel) + } + return fmt.Sprintf("guest %s %s", entityID, baseLabel) + } + if node != "" { + return node + " " + baseLabel + } + if cluster := clusterLabel(p); cluster != "" { + return cluster + " " + baseLabel + } + return "proxmox " + baseLabel +} + func preferredSubjectLabelFromDetails(p RecoveryPoint, currentLabel string) string { currentLabel = strings.TrimSpace(currentLabel) if !strings.HasPrefix(string(p.Provider), "proxmox-") { @@ -99,6 +144,9 @@ func preferredSubjectLabelFromDetails(p RecoveryPoint, currentLabel string) stri entityID := entityIDLabel(p) if currentLabel != "" && currentLabel != entityID && !isNumericOnlyLabel(currentLabel) { + if candidate := preferredProxmoxTaskLabelFromDetails(p, currentLabel); candidate != "" { + return candidate + } return "" } @@ -108,6 +156,10 @@ func preferredSubjectLabelFromDetails(p RecoveryPoint, currentLabel string) stri } } + if candidate := preferredProxmoxTaskLabelFromDetails(p, currentLabel); candidate != "" { + return candidate + } + return "" } @@ -274,11 +326,17 @@ func namespaceLabel(p RecoveryPoint) string { func entityIDLabel(p RecoveryPoint) string { // Proxmox VMID (int or string depending on source). if v := detailsString(p, "vmid"); v != "" { + if strings.TrimSpace(string(p.Provider)) == string(ProviderProxmoxPVE) && v == "0" { + return "" + } return v } if p.Details != nil { if raw, ok := p.Details["vmid"]; ok { if v := anyToString(raw); v != "" { + if strings.TrimSpace(string(p.Provider)) == string(ProviderProxmoxPVE) && v == "0" { + return "" + } return v } } @@ -401,6 +459,11 @@ func DeriveIndex(p RecoveryPoint) PointIndex { if p.SubjectRef != nil { subjectType = strings.TrimSpace(p.SubjectRef.Type) } + subjectLabel := subjectLabel(p) + itemType := NormalizeRecoveryItemType(subjectType) + if itemType == "" && preferredProxmoxTaskLabelFromDetails(p, "") != "" { + itemType = "task" + } isWorkload := isWorkloadSubjectType(subjectType) // If the point is linked to a unified resource, treat it as a protected subject (workload) @@ -410,9 +473,9 @@ func DeriveIndex(p RecoveryPoint) PointIndex { } return PointIndex{ - SubjectLabel: subjectLabel(p), + SubjectLabel: subjectLabel, SubjectType: subjectType, - ItemType: NormalizeRecoveryItemType(subjectType), + ItemType: itemType, IsWorkload: isWorkload, ClusterLabel: clusterLabel(p), NodeHostLabel: nodeHostLabel(p), diff --git a/internal/recovery/recovery_test.go b/internal/recovery/recovery_test.go index 5ffa297ac..7eeab0b43 100644 --- a/internal/recovery/recovery_test.go +++ b/internal/recovery/recovery_test.go @@ -169,6 +169,35 @@ func TestDeriveIndex(t *testing.T) { DetailsSummary: "pulse-v4-prod, pi, 140", }, }, + { + name: "PVE task falls back to node backup task label instead of raw task id", + point: RecoveryPoint{ + ID: "pve-task:delly-UPID:minipc:0014B9F1:22DC4693:69C600C1:vzdump::root@pam:", + Provider: ProviderProxmoxPVE, + Kind: KindBackup, + Mode: ModeLocal, + Outcome: OutcomeSuccess, + Details: map[string]any{ + "instance": "delly", + "node": "minipc", + "vmid": 0, + "type": "vzdump", + "taskID": "delly-UPID:minipc:0014B9F1:22DC4693:69C600C1:vzdump::root@pam:", + }, + }, + expected: PointIndex{ + SubjectLabel: "minipc backup task", + SubjectType: "", + ItemType: "task", + IsWorkload: false, + ClusterLabel: "delly", + NodeHostLabel: "minipc", + NamespaceLabel: "", + EntityIDLabel: "", + RepositoryLabel: "", + DetailsSummary: "", + }, + }, { 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 08665b25a..1fa4df234 100644 --- a/internal/recovery/store/store_index_backfill.go +++ b/internal/recovery/store/store_index_backfill.go @@ -42,7 +42,9 @@ func (s *Store) BackfillIndex(ctx context.Context) error { 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) <> '')) + details_json IS NOT NULL AND TRIM(details_json) <> '') OR + (provider = 'proxmox-pve' AND + subject_label IS NOT NULL AND TRIM(subject_label) LIKE 'pve-task:%')) LIMIT `+fmt.Sprint(maxBackfillRows)+` `) if err != nil { diff --git a/internal/recovery/store/store_test.go b/internal/recovery/store/store_test.go index a5a74206b..2538f9968 100644 --- a/internal/recovery/store/store_test.go +++ b/internal/recovery/store/store_test.go @@ -287,6 +287,80 @@ func TestStore_OpenBackfillsLegacyNumericPBSSubjectLabels(t *testing.T) { } } +func TestStore_OpenBackfillsLegacyPVETaskSubjectLabels(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, 4, 7, 9, 0, time.UTC) + point := recovery.RecoveryPoint{ + ID: "pve-task:delly-UPID:minipc:0014B9F1:22DC4693:69C600C1:vzdump::root@pam:", + Provider: recovery.ProviderProxmoxPVE, + Kind: recovery.KindBackup, + Mode: recovery.ModeLocal, + Outcome: recovery.OutcomeSuccess, + StartedAt: &now, + CompletedAt: &now, + Details: map[string]any{ + "instance": "delly", + "node": "minipc", + "status": "OK", + "type": "vzdump", + "taskID": "delly-UPID:minipc:0014B9F1:22DC4693:69C600C1:vzdump::root@pam:", + "vmid": 0, + }, + } + + 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 = ? + WHERE id = ?`, + point.ID, + "0", + point.ID, + ); err != nil { + t.Fatalf("degrade legacy pve task 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 != "minipc backup task" { + t.Fatalf("ListPoints() display = %#v, want backfilled subject label minipc backup task", points[0].Display) + } + if points[0].Display != nil && points[0].Display.EntityIDLabel != "" { + t.Fatalf("ListPoints() display entity id = %#v, want empty for synthetic PVE task label", points[0].Display) + } + if points[0].Display == nil || points[0].Display.ItemType != "task" { + t.Fatalf("ListPoints() display = %#v, want synthetic item type task", points[0].Display) + } +} + func createLegacyRecoveryDBWithoutItemType(t *testing.T, dbPath string, point recovery.RecoveryPoint) { t.Helper()