From 5dc644c650e23584022ef46b9e84a653148fef45 Mon Sep 17 00:00:00 2001 From: rcourtman Date: Wed, 1 Apr 2026 13:02:14 +0100 Subject: [PATCH] Preserve docker metadata across container recreation --- .../v6/internal/subsystems/monitoring.md | 4 + .../monitoring/canonical_guardrails_test.go | 29 +++++ .../monitoring/docker_metadata_migration.go | 65 +++++++++++ internal/monitoring/monitor_agents.go | 4 + internal/monitoring/monitor_docker_test.go | 101 ++++++++++++++++++ 5 files changed, 203 insertions(+) diff --git a/docs/release-control/v6/internal/subsystems/monitoring.md b/docs/release-control/v6/internal/subsystems/monitoring.md index ddc8f9f16..42fcae093 100644 --- a/docs/release-control/v6/internal/subsystems/monitoring.md +++ b/docs/release-control/v6/internal/subsystems/monitoring.md @@ -296,6 +296,10 @@ copy custom URL, description, tags, and notes metadata onto the new container ID instead of dropping that operator state on ordinary container replacement. If multiple prior containers normalize to the same name, the migration must fail closed and skip the copy rather than guessing between ambiguous sources. +Name normalization for that contract must treat Docker's leading `/` prefix as +presentation noise rather than identity, so routine recreate flows keep +metadata continuity when one report spells the same container as `/app` and a +later report spells it as `app`. The same applies to proxmox topology coordinates exposed through typed views: node, cluster, and instance accessors must return canonical trimmed values so monitoring consumers do not fork topology grouping or labeling on `" pve-a "` diff --git a/internal/monitoring/canonical_guardrails_test.go b/internal/monitoring/canonical_guardrails_test.go index e80e394c9..fc9be5302 100644 --- a/internal/monitoring/canonical_guardrails_test.go +++ b/internal/monitoring/canonical_guardrails_test.go @@ -259,6 +259,35 @@ func TestProxmoxNodeDiskUsesCanonicalResolver(t *testing.T) { } } +func TestDockerReportPreservesMetadataAcrossObservedContainerRecreation(t *testing.T) { + migrationData, err := os.ReadFile("docker_metadata_migration.go") + if err != nil { + t.Fatalf("failed to read docker_metadata_migration.go: %v", err) + } + migrationSource := string(migrationData) + requiredMigrationSnippets := []string{ + "func (m *Monitor) migrateDockerContainerMetadataForRecreatedContainers(", + "normalizeDockerContainerMetadataIdentity(container.Name)", + `strings.TrimSpace(strings.TrimPrefix(name, "/"))`, + "m.CopyDockerContainerMetadata(hostID, previousContainer.ID, container.ID)", + } + for _, snippet := range requiredMigrationSnippets { + if !strings.Contains(migrationSource, snippet) { + t.Fatalf("docker_metadata_migration.go must contain %q", snippet) + } + } + + agentsData, err := os.ReadFile("monitor_agents.go") + if err != nil { + t.Fatalf("failed to read monitor_agents.go: %v", err) + } + agentsSource := string(agentsData) + requiredAgentsSnippet := "m.migrateDockerContainerMetadataForRecreatedContainers(identifier, previous.Containers(), host.Containers)" + if !strings.Contains(agentsSource, requiredAgentsSnippet) { + t.Fatalf("monitor_agents.go must contain %q", requiredAgentsSnippet) + } +} + func TestProxmoxNodeDiskFallbackPrefersCanonicalSystemStorage(t *testing.T) { requiredSnippets := map[string][]string{ "node_disk_sources.go": { diff --git a/internal/monitoring/docker_metadata_migration.go b/internal/monitoring/docker_metadata_migration.go index 3cbf1dbbd..8c0f81509 100644 --- a/internal/monitoring/docker_metadata_migration.go +++ b/internal/monitoring/docker_metadata_migration.go @@ -6,6 +6,8 @@ import ( "strings" "github.com/rcourtman/pulse-go-rewrite/internal/config" + "github.com/rcourtman/pulse-go-rewrite/internal/models" + "github.com/rs/zerolog/log" ) // CopyDockerContainerMetadata copies persisted container metadata from an old container runtime ID to a new one. @@ -70,3 +72,66 @@ func (m *Monitor) CopyDockerContainerMetadata(hostID, oldContainerID, newContain return m.dockerMetadataStore.Set(newKey, &merged) } + +func (m *Monitor) migrateDockerContainerMetadataForRecreatedContainers( + hostID string, + previousContainers []models.DockerContainer, + currentContainers []models.DockerContainer, +) { + if m == nil || m.dockerMetadataStore == nil { + return + } + + hostID = strings.TrimSpace(hostID) + if hostID == "" || len(previousContainers) == 0 || len(currentContainers) == 0 { + return + } + + previousByName := make(map[string]models.DockerContainer) + ambiguous := make(map[string]struct{}) + for _, container := range previousContainers { + name := normalizeDockerContainerMetadataIdentity(container.Name) + if name == "" || strings.TrimSpace(container.ID) == "" { + continue + } + if _, exists := previousByName[name]; exists { + ambiguous[name] = struct{}{} + delete(previousByName, name) + continue + } + if _, dup := ambiguous[name]; dup { + continue + } + previousByName[name] = container + } + + for _, container := range currentContainers { + name := normalizeDockerContainerMetadataIdentity(container.Name) + if name == "" || strings.TrimSpace(container.ID) == "" { + continue + } + if _, dup := ambiguous[name]; dup { + continue + } + previousContainer, ok := previousByName[name] + if !ok { + continue + } + if strings.TrimSpace(previousContainer.ID) == strings.TrimSpace(container.ID) { + continue + } + if err := m.CopyDockerContainerMetadata(hostID, previousContainer.ID, container.ID); err != nil { + log.Warn(). + Err(err). + Str("dockerHostID", hostID). + Str("containerName", container.Name). + Str("oldContainerID", previousContainer.ID). + Str("newContainerID", container.ID). + Msg("Failed to migrate docker container metadata after observed recreation") + } + } +} + +func normalizeDockerContainerMetadataIdentity(name string) string { + return strings.TrimSpace(strings.TrimPrefix(name, "/")) +} diff --git a/internal/monitoring/monitor_agents.go b/internal/monitoring/monitor_agents.go index f55c12956..5a058935a 100644 --- a/internal/monitoring/monitor_agents.go +++ b/internal/monitoring/monitor_agents.go @@ -1219,6 +1219,10 @@ func (m *Monitor) ApplyDockerReport(report agentsdocker.Report, tokenRecord *con IsLegacy: isLegacyAgent(report.Agent.Type), } + if hasPrevious { + m.migrateDockerContainerMetadataForRecreatedContainers(identifier, previous.Containers(), host.Containers) + } + if tokenRecord != nil { host.TokenID = tokenRecord.ID host.TokenName = tokenRecord.Name diff --git a/internal/monitoring/monitor_docker_test.go b/internal/monitoring/monitor_docker_test.go index a02978563..3cf18c330 100644 --- a/internal/monitoring/monitor_docker_test.go +++ b/internal/monitoring/monitor_docker_test.go @@ -267,6 +267,107 @@ func TestApplyDockerReportIncludesContainerDiskDetails(t *testing.T) { } } +func TestApplyDockerReportMigratesMetadataWhenContainerRuntimeIDChanges(t *testing.T) { + monitor := newTestMonitor(t) + + baseTimestamp := time.Now().UTC() + firstReport := agentsdocker.Report{ + Agent: agentsdocker.AgentInfo{ + ID: "agent-1", + Version: "1.0.0", + IntervalSeconds: 30, + }, + Host: agentsdocker.HostInfo{ + Hostname: "docker-host-1", + MachineID: "machine-1", + }, + Containers: []agentsdocker.Container{ + {ID: "container-old", Name: "app"}, + }, + Timestamp: baseTimestamp, + } + + host, err := monitor.ApplyDockerReport(firstReport, nil) + if err != nil { + t.Fatalf("first ApplyDockerReport failed: %v", err) + } + if host.ID == "" { + t.Fatal("expected docker host ID") + } + if err := monitor.dockerMetadataStore.Set(host.ID+":container:container-old", &config.DockerMetadata{ + CustomURL: "https://app.internal", + }); err != nil { + t.Fatalf("seed docker metadata: %v", err) + } + + secondReport := firstReport + secondReport.Timestamp = baseTimestamp.Add(30 * time.Second) + secondReport.Containers = []agentsdocker.Container{ + {ID: "container-new", Name: "app"}, + } + + host, err = monitor.ApplyDockerReport(secondReport, nil) + if err != nil { + t.Fatalf("second ApplyDockerReport failed: %v", err) + } + + meta := monitor.dockerMetadataStore.Get(host.ID + ":container:container-new") + if meta == nil { + t.Fatalf("expected migrated metadata for recreated container") + } + if meta.CustomURL != "https://app.internal" { + t.Fatalf("expected migrated custom URL, got %#v", meta) + } +} + +func TestApplyDockerReportSkipsMetadataMigrationForAmbiguousContainerNames(t *testing.T) { + monitor := newTestMonitor(t) + + baseTimestamp := time.Now().UTC() + firstReport := agentsdocker.Report{ + Agent: agentsdocker.AgentInfo{ + ID: "agent-ambiguous", + Version: "1.0.0", + IntervalSeconds: 30, + }, + Host: agentsdocker.HostInfo{ + Hostname: "docker-host-ambiguous", + MachineID: "machine-ambiguous", + }, + Containers: []agentsdocker.Container{ + {ID: "container-old-a", Name: "/app"}, + {ID: "container-old-b", Name: "app"}, + }, + Timestamp: baseTimestamp, + } + + host, err := monitor.ApplyDockerReport(firstReport, nil) + if err != nil { + t.Fatalf("first ApplyDockerReport failed: %v", err) + } + if err := monitor.dockerMetadataStore.Set(host.ID+":container:container-old-a", &config.DockerMetadata{ + CustomURL: "https://ambiguous.internal", + }); err != nil { + t.Fatalf("seed docker metadata: %v", err) + } + + secondReport := firstReport + secondReport.Timestamp = baseTimestamp.Add(30 * time.Second) + secondReport.Containers = []agentsdocker.Container{ + {ID: "container-new", Name: "app"}, + } + + host, err = monitor.ApplyDockerReport(secondReport, nil) + if err != nil { + t.Fatalf("second ApplyDockerReport failed: %v", err) + } + + meta := monitor.dockerMetadataStore.Get(host.ID + ":container:container-new") + if meta != nil { + t.Fatalf("expected ambiguous metadata migration to be skipped, got %#v", meta) + } +} + func TestApplyDockerReportComputesContainerNetworkAndDiskRates(t *testing.T) { monitor := newTestMonitor(t) baseTime := time.Now().UTC()