diff --git a/docs/release-control/v6/internal/subsystems/ai-runtime.md b/docs/release-control/v6/internal/subsystems/ai-runtime.md index c6402b3694..5f78aaac71 100644 --- a/docs/release-control/v6/internal/subsystems/ai-runtime.md +++ b/docs/release-control/v6/internal/subsystems/ai-runtime.md @@ -6543,6 +6543,20 @@ Patrol scope payloads. API-backed TrueNAS systems may still keep `truenas` platform metadata and separate run-history coverage counts, but AI resource type fields must normalize to canonical `agent` once they cross the governed runtime boundary. +Patrol scope resolution registers the alert subsystem's Docker references as +aliases, built by the same `internal/alerts` builders rather than formatted +locally: `DockerHostResourceID` on the Docker host record, and on each +container record `dockerAlertScopeAlias`, which passes the container's name so +a container reported without an ID answers to its marked name reference +(`docker:/name:`) and never to its host's reference. +`dockerServiceAlertScopeAliases` registers service references, including +`docker:/service/name:` for a service without an ID, on the +owning host. The legacy snapshot record of a container without an ID has no ID +to scope by, so only the unified read state can scope its alerts. Proof: +`TestResolvePatrolScopeResolvesDockerAlertResourceIDs` in +`internal/ai/patrol_run_test.go` and +`TestDockerServiceAlertScopeAliasesDeriveIDFromName` in +`internal/ai/patrol_state_scope_alias_test.go`. The same governed-context rule also applies to the main unified AI resource overview: infrastructure, workload, alert-label, and top-consumer summaries must not leak raw resource names, cluster labels, IP addresses, or unresolved diff --git a/docs/release-control/v6/internal/subsystems/alerts.md b/docs/release-control/v6/internal/subsystems/alerts.md index 24d5382258..ccbe657d5f 100644 --- a/docs/release-control/v6/internal/subsystems/alerts.md +++ b/docs/release-control/v6/internal/subsystems/alerts.md @@ -1477,6 +1477,45 @@ short-ID, unified-hash, and slash-tail forms as trailing lookup candidates). Container override work must not reintroduce a runtime-container-ID persistence key. +Docker alert references come only from the builders in +`internal/alerts/docker.go`: a host's own alert `docker:` +(`DockerHostResourceID`, used only by its connectivity alert), a container +`docker:/` (`DockerContainerResourceID`), and a Swarm +service `docker:/service/` (`DockerServiceResourceID`). +A container or service reported without an ID alerts under its name, marked in +the ID position (`docker:/name:`, +`docker:/service/name:`). Docker and Swarm IDs never +contain a colon, so a name reference never takes its host's reference or +another service's ID reference, and one with neither an ID nor a name raises +no alert. Report cleanup, host removal and the host's offline alert firing +clear children by the `docker:/` prefix; the container and service +policy switches classify by the first path segment after the host +(`dockerAlertResourcePath`), so a container name containing `/service/` stays +a container; and Patrol's scope aliases and resource history parse the same +forms. Service overrides key on the same reference, so an override written for +a service without an ID under its old unmarked name no longer applies. A +reference can contain the canonical state separator (a Docker host +disambiguated as `::`), so `splitCanonicalStateID` splits a +state ID before the alert's recorded spec ID, or else after its resource ID, +and only without either at the first `::`. Splitting at the first `::` gave +such a host's alerts the reference of the host named ``; an alert saved +that way recovers its reference on restore because its spec ID is intact. +Before name references existed, a container without an ID alerted under its +host's reference. Only the connectivity alert belongs there, so the host's +next report, or its offline alert firing, clears any such alert +left active whose recorded host matches and whose container ID is empty. A +pending image update recorded there keeps its age under the container's new +reference when that first report still shows the update pending; a first +report with missing or failed update evidence clears it, and the delay starts +again on the next positive report. Proof: +`TestDockerAlertsWithoutIDsUseTheirOwnReferences`, +`TestDockerHostIDWithSeparatorKeepsItsAlertReferences`, +`TestDockerAlertsRestoredWithCutShortReferencesRecover`, +`TestDockerContainerWithoutIDKeepsPendingUpdateAgeAcrossUpgrade` and +`TestDockerContainerResourceID` in `internal/alerts/alerts_test.go`, and +`TestOwnerAlertTimelinesUseCanonicalHistoryIdentity` in +`internal/monitoring/monitor_alert_handling_test.go`. + Backup orphan evaluation is also inventory-scoped. The alerts runtime may evaluate recovery rollups for backup age, but unresolved Proxmox PVE backup subjects must not be treated as orphaned until monitoring has supplied the diff --git a/docs/release-control/v6/internal/subsystems/monitoring.md b/docs/release-control/v6/internal/subsystems/monitoring.md index c233ab1ee2..612334dea6 100644 --- a/docs/release-control/v6/internal/subsystems/monitoring.md +++ b/docs/release-control/v6/internal/subsystems/monitoring.md @@ -1065,14 +1065,15 @@ events (ZFS pools and devices, host filesystems, disks, RAID arrays, sensors and the Unraid array), pass through the same writer, which binds them to the Docker host, Swarm service, storage or host named in the unified-resources history identity clause. Container names and shortened container IDs still -bind nothing. `CheckDockerHost` gives a container without an ID its host's -reference, so that container's events join the host's history. A producer +bind nothing, and neither does the marked name a container or Swarm service +reported without an ID alerts under, so its events join neither its host's +history nor that of a service whose ID it is named like. A producer that changes one of these reference shapes must change `dockerHostHistoryReference` or `historySubResourceOwner` with it. `TestOwnerAlertTimelinesUseCanonicalHistoryIdentity` in `internal/monitoring/monitor_alert_handling_test.go` pins the Docker host, -Swarm service, ZFS pool and device, and host filesystem shapes against the -real alert manager. +Swarm service, ID-less container and service, ZFS pool and device, and host +filesystem shapes against the real alert manager. TrueNAS native alert projection preserves the trimmed, uppercase provider level in ResourceIncident.NativeSeverity. INFO and NOTICE retain the same canonical monitor risk; consumers must not lose their distinct actionability when projecting provider evidence. Native CRITICAL, ALERT, and EMERGENCY all project to canonical critical severity; EMERGENCY must not be discarded as unknown or make a still-active condition appear recovered. WARNING remains warning, and INFO and NOTICE remain informational at this projection boundary. diff --git a/docs/release-control/v6/internal/subsystems/unified-resources.md b/docs/release-control/v6/internal/subsystems/unified-resources.md index c9fb0f7d27..2b7381eab2 100644 --- a/docs/release-control/v6/internal/subsystems/unified-resources.md +++ b/docs/release-control/v6/internal/subsystems/unified-resources.md @@ -5842,17 +5842,20 @@ Docker host alerts (`docker:`) and Swarm service alerts Docker source identities: the host ID is the Docker host's source ID, and a service reference names the service with that exact ID in the host's Swarm cluster, whichever manager reported it. A hostname in the host position is -only ever looked up as a Docker host source ID. A service without an ID is -referenced by its normalized name in the same place, so while its cluster has -such a service every service reference there is a conflict. Alert references -do not record whether they came from an ID, so a row journaled by an ID-less -service can still join another service's history if a later generation or -event binds the same reference; Swarm always reports service IDs, so that needs a malformed -report naming a service exactly like another service's ID. Hostless -`docker-service:` references, container names and shortened container -IDs never bind and are never retried against later inventory. A container -without an ID alerts under its host's reference (`alerts.DockerResourceID`), so -its events join that host's history, never another container's. +only ever looked up as a Docker host source ID. The registry keys a service +without an ID by its raw name, so the service entry must also carry exactly +the referenced ID. A container or service reported without an ID alerts under +its name, marked in the ID position (`docker:/name:` and +`docker:/service/name:`, from `alerts.DockerContainerResourceID` +and `alerts.DockerServiceResourceID`). Docker and Swarm IDs never contain a +colon, so such a reference never equals its host's reference or another +service's ID reference, and `dockerHostHistoryReference` treats a service slot +with a colon as a name. These name references, hostless `docker-service:` +references, container names and shortened container IDs never bind and are +never retried against later inventory. Rows journaled before name references +existed keep the reference they were written under: an ID-less container's +rows stay in its host's history, and an ID-less service's rows bind only to a +service in that cluster whose ID equals its normalized name. Sub-resource alert references name no resource of their own and bind to the owner whose durable ID they carry (`historySubResourceOwner`): ZFS pool and device alerts (`/zfs-pool:[/device:]`) to the diff --git a/internal/ai/patrol_run_test.go b/internal/ai/patrol_run_test.go index e45bd0c944..de708b56b0 100644 --- a/internal/ai/patrol_run_test.go +++ b/internal/ai/patrol_run_test.go @@ -149,12 +149,18 @@ func TestResolvePatrolScopeCanonicalIDOutranksAliasCollision(t *testing.T) { } func TestResolvePatrolScopeResolvesDockerAlertResourceIDs(t *testing.T) { + // A container reported without an ID alerts under its own name reference, + // so the host's reference stays unambiguous. snapshot := models.StateSnapshot{DockerHosts: []models.DockerHost{{ ID: "nas", Hostname: "nas", - Containers: []models.DockerContainer{{ID: "abc123def456", Name: "dockhand-hawser-updater-nas", State: "exited"}}, + Containers: []models.DockerContainer{ + {ID: "abc123def456", Name: "dockhand-hawser-updater-nas", State: "exited"}, + {Name: "legacy-sidecar", State: "exited"}, + }, }}} - containerAlertID := alerts.DockerResourceID("nas", "abc123def456") - hostAlertID := alerts.DockerResourceID("nas", "") + containerAlertID := alerts.DockerContainerResourceID("nas", "abc123def456", "dockhand-hawser-updater-nas") + namedContainerAlertID := alerts.DockerContainerResourceID("nas", "", "legacy-sidecar") + hostAlertID := alerts.DockerHostResourceID("nas") snapshotState := newPatrolRuntimeState(snapshot) registry := unifiedresources.NewRegistry(nil) @@ -165,7 +171,12 @@ func TestResolvePatrolScopeResolvesDockerAlertResourceIDs(t *testing.T) { for _, tc := range []struct { requested string resourceType string - }{{containerAlertID, "app-container"}, {hostAlertID, "docker-host"}} { + }{{containerAlertID, "app-container"}, {namedContainerAlertID, "app-container"}, {hostAlertID, "docker-host"}} { + if tc.requested == namedContainerAlertID && state.readState == nil { + // The legacy snapshot record of a container without an ID has + // no ID to scope by; only the unified read state can. + continue + } resolved, resolution := resolvePatrolScopeState(state, PatrolScope{ResourceIDs: []string{tc.requested}}) if len(resolution.UnmatchedResourceIDs) != 0 || len(resolution.AmbiguousResourceIDs) != 0 { t.Fatalf("alert resource ID %q did not resolve exactly: %+v", tc.requested, resolution) diff --git a/internal/ai/patrol_state.go b/internal/ai/patrol_state.go index 1d0d7e8493..b1cf74e2e0 100644 --- a/internal/ai/patrol_state.go +++ b/internal/ai/patrol_state.go @@ -9,32 +9,37 @@ import ( "github.com/rcourtman/pulse-go-rewrite/internal/unifiedresources" ) -// dockerAlertScopeAlias mirrors the alert subsystem's docker resource IDs -// ("docker:" and "docker:/") so an alert's resource ID -// resolves to the patrol runtime record for that host or container (#1699). -// Returns "" without a host part rather than the shared "docker:unknown" -// fallback, which would alias unrelated records together. -func dockerAlertScopeAlias(hostID, containerID string) string { +// dockerAlertScopeAlias mirrors the alert subsystem's Docker container +// resource IDs ("docker:/", or "docker:/name:" +// for a container without an ID) so an alert's resource ID resolves to the +// patrol runtime record for that container (#1699). A host's own records use +// alerts.DockerHostResourceID. Returns "" without a host part rather than a +// hostless form, which would alias unrelated records together. +func dockerAlertScopeAlias(hostID, containerID, containerName string) string { if strings.TrimSpace(hostID) == "" { return "" } - return alerts.DockerResourceID(hostID, containerID) + return alerts.DockerContainerResourceID(hostID, containerID, containerName) } // dockerServiceAlertScopeAliases mirrors the alert subsystem's Swarm service -// resource IDs ("docker:/service/") onto the owning Docker +// resource IDs ("docker:/service/", or +// "docker:/service/name:" without an ID) onto the owning Docker // host record. Patrol has no service-level analysis, and a service's tasks run // as containers on the host, so the host is the smallest unit Patrol can // investigate for a service alert (#1699). Hosts without an ID are skipped for // the same anti-aliasing reason as dockerAlertScopeAlias: the host-less -// "docker-service:" fallback could collide across hosts. +// "docker-service:" fallback could collide across hosts. A service with +// neither an ID nor a name raises no alert, so it has no alias. func dockerServiceAlertScopeAliases(hostID string, services []models.DockerService) []string { if strings.TrimSpace(hostID) == "" || len(services) == 0 { return nil } aliases := make([]string, 0, len(services)) for _, service := range services { - aliases = append(aliases, alerts.DockerServiceResourceID(hostID, service.ID, service.Name)) + if alias := alerts.DockerServiceResourceID(hostID, service.ID, service.Name); alias != "" { + aliases = append(aliases, alias) + } } return aliases } @@ -263,14 +268,14 @@ func patrolVisitRuntimeResources(s patrolRuntimeState, visit func(patrolRuntimeR } } for _, dh := range rs.DockerHosts() { - aliases := append([]string{dh.Name(), dh.Hostname(), dockerAlertScopeAlias(dh.HostSourceID(), "")}, + aliases := append([]string{dh.Name(), dh.Hostname(), alerts.DockerHostResourceID(dh.HostSourceID())}, dockerServiceAlertScopeAliases(dh.HostSourceID(), dh.Services())...) if !emit(patrolRuntimeResourceDockerHost, []string{dh.ID(), dh.HostSourceID()}, aliases...) { return } } for _, dc := range rs.DockerContainers() { - if !emit(patrolRuntimeResourceDockerItem, []string{dc.ID(), dc.ContainerID()}, dc.Name(), dockerAlertScopeAlias(dc.HostSourceID(), dc.ContainerID())) { + if !emit(patrolRuntimeResourceDockerItem, []string{dc.ID(), dc.ContainerID()}, dc.Name(), dockerAlertScopeAlias(dc.HostSourceID(), dc.ContainerID(), dc.Name())) { return } } @@ -316,13 +321,13 @@ func patrolVisitRuntimeResources(s patrolRuntimeState, visit func(patrolRuntimeR } } for _, dh := range s.DockerHosts { - aliases := append([]string{dh.DisplayName, dh.CustomDisplayName, dh.Hostname, dockerAlertScopeAlias(dh.ID, "")}, + aliases := append([]string{dh.DisplayName, dh.CustomDisplayName, dh.Hostname, alerts.DockerHostResourceID(dh.ID)}, dockerServiceAlertScopeAliases(dh.ID, dh.Services)...) if !emit(patrolRuntimeResourceDockerHost, []string{dh.ID}, aliases...) { return } for _, dc := range dh.Containers { - if !emit(patrolRuntimeResourceDockerItem, []string{dc.ID}, dc.Name, dockerAlertScopeAlias(dh.ID, dc.ID)) { + if !emit(patrolRuntimeResourceDockerItem, []string{dc.ID}, dc.Name, dockerAlertScopeAlias(dh.ID, dc.ID, dc.Name)) { return } } diff --git a/internal/ai/patrol_state_scope_alias_test.go b/internal/ai/patrol_state_scope_alias_test.go index 0889178589..c0d9e88b09 100644 --- a/internal/ai/patrol_state_scope_alias_test.go +++ b/internal/ai/patrol_state_scope_alias_test.go @@ -78,16 +78,17 @@ func TestDockerServiceAlertScopeAliasesWithoutServices(t *testing.T) { } // TestDockerServiceAlertScopeAliasesDeriveIDFromName covers services that carry -// no ID, where the canonical builder derives one from the service name. The -// alias has to follow that derivation rather than formatting its own string. +// no ID, where the canonical builder references the marked service name. The +// alias has to follow that derivation rather than formatting its own string, +// and a service with neither an ID nor a name raises no alert, so no alias. func TestDockerServiceAlertScopeAliasesDeriveIDFromName(t *testing.T) { - got := dockerServiceAlertScopeAliases("host-1", []models.DockerService{{Name: "Web API"}}) + got := dockerServiceAlertScopeAliases("host-1", []models.DockerService{{Name: "Web API"}, {Name: " "}}) want := alerts.DockerServiceResourceID("host-1", "", "Web API") if len(got) != 1 || got[0] != want { t.Fatalf("aliases = %v, want [%q]", got, want) } - if got[0] != "docker:host-1/service/web-api" { - t.Errorf("alias = %q, want %q", got[0], "docker:host-1/service/web-api") + if got[0] != "docker:host-1/service/name:web-api" { + t.Errorf("alias = %q, want %q", got[0], "docker:host-1/service/name:web-api") } } diff --git a/internal/alerts/alerts_test.go b/internal/alerts/alerts_test.go index f95e034cbf..677d046d37 100644 --- a/internal/alerts/alerts_test.go +++ b/internal/alerts/alerts_test.go @@ -2720,7 +2720,7 @@ func TestCheckDockerHostIgnoresContainersByPrefix(t *testing.T) { Containers: []models.DockerContainer{container}, } - resourceID := DockerResourceID(host.ID, container.ID) + resourceID := DockerContainerResourceID(host.ID, container.ID, "") alertID := fmt.Sprintf("docker-container-state-%s", resourceID) // Run twice to satisfy the confirmation threshold when not ignored @@ -2759,7 +2759,7 @@ func TestCheckDockerHostContainerCPUUsesCapacityNormalizedPercent(t *testing.T) CPUPercent: 240, }}, } - alertID := canonicalMetricStateID(DockerResourceID(host.ID, "container-1"), "cpu") + alertID := canonicalMetricStateID(DockerContainerResourceID(host.ID, "container-1", ""), "cpu") m.CheckDockerHost(host) m.mu.RLock() @@ -3058,7 +3058,7 @@ func TestDockerContainerStateUsesDockerDefaults(t *testing.T) { m.CheckDockerHost(host) m.CheckDockerHost(host) - resourceID := DockerResourceID(host.ID, container.ID) + resourceID := DockerContainerResourceID(host.ID, container.ID, "") alertID := fmt.Sprintf("docker-container-state-%s", resourceID) alert, exists := testLookupActiveAlert(t, m, alertID) if !exists { @@ -3091,7 +3091,7 @@ func TestDockerContainerStateRespectsDisableDefault(t *testing.T) { m.CheckDockerHost(host) m.CheckDockerHost(host) - resourceID := DockerResourceID(host.ID, container.ID) + resourceID := DockerContainerResourceID(host.ID, container.ID, "") alertID := fmt.Sprintf("docker-container-state-%s", resourceID) if testHasActiveAlert(t, m, alertID) { t.Fatalf("did not expect docker container state alert when defaults disable connectivity") @@ -3121,7 +3121,7 @@ func TestDockerContainerMemoryLimitHysteresis(t *testing.T) { m.CheckDockerHost(hostHigh) - resourceID := DockerResourceID(hostID, containerID) + resourceID := DockerContainerResourceID(hostID, containerID, "") alertID := fmt.Sprintf("docker-container-memory-limit-%s", resourceID) alert, exists := testLookupActiveAlert(t, m, alertID) if !exists { @@ -3190,7 +3190,7 @@ func TestDockerContainerDiskUsageAlert(t *testing.T) { m.CheckDockerHost(host) - resourceID := DockerResourceID(host.ID, host.Containers[0].ID) + resourceID := DockerContainerResourceID(host.ID, host.Containers[0].ID, "") alertID := canonicalMetricStateID(resourceID, "disk") alert, exists := testLookupActiveAlert(t, m, alertID) if !exists { @@ -3782,7 +3782,7 @@ func TestCheckDockerHostIgnoredPrefixClearsExistingAlerts(t *testing.T) { Hostname: "docker-host.local", Containers: []models.DockerContainer{container}, } - resourceID := DockerResourceID(host.ID, container.ID) + resourceID := DockerContainerResourceID(host.ID, container.ID, "") stateAlertID := fmt.Sprintf("docker-container-state-%s", resourceID) healthAlertID := fmt.Sprintf("docker-container-health-%s", resourceID) restartAlertID := fmt.Sprintf("docker-container-restart-loop-%s", resourceID) @@ -3954,20 +3954,25 @@ func TestDockerContainerDisplayName(t *testing.T) { } } -func TestDockerResourceID(t *testing.T) { +func TestDockerContainerResourceID(t *testing.T) { // t.Parallel() tests := []struct { - name string - hostID string - containerID string - want string + name string + hostID string + containerID string + containerName string + want string }{ - {name: "both ids present", hostID: "host1", containerID: "abc", want: "docker:host1/abc"}, + {name: "both ids present", hostID: "host1", containerID: "abc", containerName: "web", want: "docker:host1/abc"}, {name: "trims ids", hostID: " host1 ", containerID: " abc ", want: "docker:host1/abc"}, {name: "missing host id", hostID: "", containerID: "abc", want: "docker:container/abc"}, - {name: "missing container id", hostID: "host1", containerID: "", want: "docker:host1"}, - {name: "both missing", hostID: "", containerID: "", want: "docker:unknown"}, + // A container without an ID never takes its host's reference. + {name: "missing container id uses the marked name", hostID: "host1", containerID: "", containerName: " /web ", want: "docker:host1/name:web"}, + {name: "a name never equals an ID reference", hostID: "host1", containerID: "", containerName: "abc", want: "docker:host1/name:abc"}, + {name: "no id and no name", hostID: "host1", containerID: "", containerName: " / ", want: ""}, + {name: "a name without a host identifies nothing", hostID: "", containerID: "", containerName: "web", want: ""}, + {name: "all missing", want: ""}, } for _, tc := range tests { @@ -3975,11 +3980,255 @@ func TestDockerResourceID(t *testing.T) { t.Run(tc.name, func(t *testing.T) { // t.Parallel() - if got := DockerResourceID(tc.hostID, tc.containerID); got != tc.want { - t.Fatalf("DockerResourceID(%q, %q) = %q, want %q", tc.hostID, tc.containerID, got, tc.want) + if got := DockerContainerResourceID(tc.hostID, tc.containerID, tc.containerName); got != tc.want { + t.Fatalf("DockerContainerResourceID(%q, %q, %q) = %q, want %q", tc.hostID, tc.containerID, tc.containerName, got, tc.want) } }) } + if got := DockerHostResourceID(" host1 "); got != "docker:host1" { + t.Fatalf("DockerHostResourceID = %q, want docker:host1", got) + } + if got := DockerHostResourceID(" "); got != "" { + t.Fatalf("DockerHostResourceID without a host = %q, want empty", got) + } +} + +// Containers and Swarm services reported without an ID alert under their own +// marked name, never under the host's reference or another service's ID +// reference, so cleanup, removal and the policy switches reach them like any +// other container or service. +func TestDockerAlertsWithoutIDsUseTheirOwnReferences(t *testing.T) { + m := newTestManager(t) + config := m.GetConfig() + config.Enabled = true + config.TimeThresholds = map[string]int{"all": 0} + config.SuppressionWindow = 0 + m.UpdateConfig(config) + + containerID := strings.Repeat("c", 64) + serviceID := "x7k2m9q4w1e8r5t3y6u0i2o4p" + host := models.DockerHost{ID: "h1", Hostname: "tower", DisplayName: "tower", Status: "online", LastSeen: time.Now(), + Containers: []models.DockerContainer{ + {ID: containerID, Name: "web", State: "running", Health: "unhealthy"}, + {Name: "/sidecar", State: "running", Health: "unhealthy"}, + // Docker forbids slashes in names, but a report can carry anything. + {Name: "jobs/service/runner", State: "running", Health: "unhealthy"}, + {State: "running", Health: "unhealthy"}, // no identity at all + }, + Services: []models.DockerService{ + {ID: serviceID, Name: "api", DesiredTasks: 2}, + {Name: strings.ToUpper(serviceID), DesiredTasks: 2}, + {DesiredTasks: 2}, + }} + refs := func() map[string]bool { + got := map[string]bool{} + for _, alert := range m.GetActiveAlerts() { + got[alert.ResourceID] = true + } + return got + } + var firedMu sync.Mutex + fired := map[string]bool{} + m.SubscribeLifecycleCallback(func(event LifecycleEvent) { + if event.Type == eventlog.TypeFired && event.Alert != nil { + firedMu.Lock() + fired[event.Alert.ResourceID] = true + firedMu.Unlock() + } + }) + m.CheckDockerHost(host) + want := map[string]bool{ + "docker:h1/" + containerID: true, + "docker:h1/name:sidecar": true, + "docker:h1/name:jobs/service/runner": true, + "docker:h1/service/" + serviceID: true, + "docker:h1/service/name:" + serviceID: true, + } + if got := refs(); !reflect.DeepEqual(got, want) { + t.Fatalf("alert references = %v, want %v", got, want) + } + firedMu.Lock() + if !reflect.DeepEqual(fired, want) { + t.Fatalf("fired alert references = %v, want %v", fired, want) + } + firedMu.Unlock() + + // A container that leaves the report clears under its own reference. + reported := host + reported.Containers = host.Containers[:1] + m.CheckDockerHost(reported) + if got := refs(); got["docker:h1/name:sidecar"] || got["docker:h1/name:jobs/service/runner"] { + t.Fatalf("alerts of removed containers without an ID survived the report: %v", got) + } + + // Before this reference existed such a container alerted under its host's. + // No producer refreshes those, so the next report clears them. + legacy := func() { + m.mu.Lock() + m.activeAlerts["legacy"] = &Alert{ID: "legacy", Type: "docker-container-health", ResourceID: "docker:h1", StartTime: time.Now(), LastSeen: time.Now(), + Metadata: map[string]interface{}{"hostId": "h1", "containerId": "", "containerName": "sidecar"}} + m.mu.Unlock() + } + legacy() + m.CheckDockerHost(host) + if refs()["docker:h1"] { + t.Fatal("a pre-upgrade container alert on the host's reference survived the host's report") + } + // Going offline clears it too, and keeps the host's own alert. + legacy() + for range 3 { + m.HandleDockerHostOffline(host) + } + offline := 0 + for _, alert := range m.GetActiveAlerts() { + if alert.ResourceID == "docker:h1" { + if alert.Type != "docker-host-offline" { + t.Fatalf("a pre-upgrade container alert on the host's reference survived going offline: %+v", alert) + } + offline++ + } + } + if offline != 1 { + t.Fatalf("host offline alerts = %d, want 1", offline) + } + + // The policy switches classify the name references by kind. + m.CheckDockerHost(host) + config = m.GetConfig() + config.DisableAllDockerContainers = true + m.UpdateConfig(config) + if got := refs(); len(got) != 2 || !got["docker:h1/service/"+serviceID] || !got["docker:h1/service/name:"+serviceID] { + t.Fatalf("after disabling containers, alert references = %v", got) + } + config.DisableAllDockerServices = true + m.UpdateConfig(config) + if got := refs(); len(got) != 0 { + t.Fatalf("after disabling services, alert references = %v", got) + } +} + +// A Docker host disambiguated as "::" carries the canonical +// state separator in its ID. Its alerts keep their full reference, so they +// never take the reference of the host named "", and a restored alert +// whose reference was cut short there is not that host's to clear. +func TestDockerHostIDWithSeparatorKeepsItsAlertReferences(t *testing.T) { + m := newTestManager(t) + config := m.GetConfig() + config.Enabled = true + config.TimeThresholds = map[string]int{"all": 0} + m.UpdateConfig(config) + containerID := strings.Repeat("d", 64) + report := func(hostID string) models.DockerHost { + return models.DockerHost{ID: hostID, Hostname: "clone", LastSeen: time.Now(), + Containers: []models.DockerContainer{{ID: containerID, Name: "web", State: "running", Health: "unhealthy"}}} + } + m.CheckDockerHost(report("base::suffix")) + m.CheckDockerHost(report("base")) + got := map[string]string{} + for _, alert := range m.GetActiveAlerts() { + got[alert.Metadata["hostId"].(string)] = alert.ResourceID + } + want := map[string]string{"base": "docker:base/" + containerID, "base::suffix": "docker:base::suffix/" + containerID} + if !reflect.DeepEqual(got, want) { + t.Fatalf("alert references by host = %v, want %v", got, want) + } + + restored := &Alert{ID: "docker:base::suffix/x::docker:base::suffix/x-health", Type: "docker-container-health", ResourceID: "docker:base", + StartTime: time.Now(), LastSeen: time.Now(), Metadata: map[string]interface{}{"hostId": "base::suffix", "containerId": "x"}} + m.mu.Lock() + m.activeAlerts[restored.ID] = restored + m.mu.Unlock() + m.CheckDockerHost(report("base")) + m.mu.RLock() + _, kept := m.activeAlerts[restored.ID] + m.mu.RUnlock() + if !kept { + t.Fatal("host base cleared another host's restored alert as its own pre-upgrade container alert") + } +} + +// Alerts saved before state IDs split after their own spec carry a resource +// ID cut short at the first "::" of a host ID like "::". They +// recover their reference on restore, so the owning host's report clears them +// and carries the age of a pending update. +func TestDockerAlertsRestoredWithCutShortReferencesRecover(t *testing.T) { + m := newTestManager(t) + config := m.GetConfig() + config.Enabled = true + config.TimeThresholds = map[string]int{"all": 0} + config.DockerDefaults.UpdateAlertDelayHours = 24 + m.UpdateConfig(config) + hostRef := "docker:base::suffix" + containerRef := hostRef + "/" + strings.Repeat("e", 64) + startedAt := time.Now().Add(-72 * time.Hour).Truncate(time.Second) + saved := []*Alert{ + {ID: buildCanonicalStateID(hostRef, hostRef+"-image-update"), CanonicalSpecID: hostRef + "-image-update", Type: "docker-container-update", + ResourceID: "docker:base", Level: AlertLevelWarning, StartTime: startedAt, LastSeen: time.Now(), + Metadata: map[string]interface{}{"hostId": "base::suffix", "containerId": "", "containerName": "sidecar"}}, + {ID: buildCanonicalStateID(containerRef, containerRef+"-health"), CanonicalSpecID: containerRef + "-health", Type: "docker-container-health", + ResourceID: "docker:base", Level: AlertLevelCritical, StartTime: startedAt, LastSeen: time.Now(), + Metadata: map[string]interface{}{"hostId": "base::suffix", "containerId": strings.Repeat("e", 64), "containerName": "gone"}}, + } + data, err := json.Marshal(saved) + if err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(m.getAlertsDir(), alertsDirPerm); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(m.getAlertsDir(), "active-alerts.json"), data, alertsFilePerm); err != nil { + t.Fatal(err) + } + if err := m.LoadActiveAlerts(); err != nil { + t.Fatalf("LoadActiveAlerts() error = %v", err) + } + restored := map[string]bool{} + for _, alert := range m.GetActiveAlerts() { + restored[alert.ResourceID] = true + } + if want := map[string]bool{hostRef: true, containerRef: true}; !reflect.DeepEqual(restored, want) { + t.Fatalf("restored references = %v, want %v", restored, want) + } + + m.CheckDockerHost(models.DockerHost{ID: "base::suffix", Hostname: "clone", LastSeen: time.Now(), Containers: []models.DockerContainer{{ + Name: "sidecar", State: "running", + UpdateStatus: &models.DockerContainerUpdateStatus{UpdateAvailable: true, CurrentDigest: "sha256:a", LatestDigest: "sha256:b", LastChecked: time.Now()}, + }}}) + active := m.GetActiveAlerts() + if len(active) != 1 || active[0].ResourceID != hostRef+"/name:sidecar" || !active[0].StartTime.Equal(startedAt) { + t.Fatalf("active alerts = %+v, want only the update on %s/name:sidecar started %s", active, hostRef, startedAt) + } +} + +// Before a container without an ID had a reference of its own, its pending +// image update was recorded under its host's reference. The first report +// after the upgrade keeps that age instead of restarting the delay. +func TestDockerContainerWithoutIDKeepsPendingUpdateAgeAcrossUpgrade(t *testing.T) { + m := newTestManager(t) + config := m.GetConfig() + config.Enabled = true + config.DockerDefaults.UpdateAlertDelayHours = 24 + m.UpdateConfig(config) + startedAt := time.Now().Add(-72 * time.Hour).Truncate(time.Second) + legacyID := buildCanonicalStateID("docker:h1", "docker:h1-image-update") + m.mu.Lock() + m.activeAlerts[legacyID] = &Alert{ID: legacyID, Type: "docker-container-update", ResourceID: "docker:h1", StartTime: startedAt, LastSeen: time.Now(), + Metadata: map[string]interface{}{"hostId": "h1", "containerId": "", "containerName": "sidecar"}} + m.mu.Unlock() + + m.CheckDockerHost(models.DockerHost{ID: "h1", Hostname: "tower", LastSeen: time.Now(), Containers: []models.DockerContainer{{ + Name: "sidecar", State: "running", Image: "nginx:latest", + UpdateStatus: &models.DockerContainerUpdateStatus{UpdateAvailable: true, CurrentDigest: "sha256:a", LatestDigest: "sha256:b", LastChecked: time.Now()}, + }}}) + var updates []Alert + for _, alert := range m.GetActiveAlerts() { + if alert.Type == "docker-container-update" { + updates = append(updates, alert) + } + } + if len(updates) != 1 || updates[0].ResourceID != "docker:h1/name:sidecar" || !updates[0].StartTime.Equal(startedAt) { + t.Fatalf("update alerts = %+v, want one on docker:h1/name:sidecar started %s", updates, startedAt) + } } func TestHasKnownFirmwareBug(t *testing.T) { @@ -6127,81 +6376,89 @@ func TestDockerServiceResourceID(t *testing.T) { expected: "docker-service:svc-123", }, { - name: "derives ID from service name when ID empty", + name: "marks the derived name when ID empty", hostID: "host-1", serviceID: "", serviceName: "My Service", - expected: "docker:host-1/service/my-service", + expected: "docker:host-1/service/name:my-service", }, { name: "special chars in name replaced with dash", hostID: "host-1", serviceID: "", serviceName: "my/service:v1.0", - expected: "docker:host-1/service/my-service-v1-0", + expected: "docker:host-1/service/name:my-service-v1-0", }, { name: "backslash and colon replaced", hostID: "host-1", serviceID: "", serviceName: "path\\to:service", - expected: "docker:host-1/service/path-to-service", + expected: "docker:host-1/service/name:path-to-service", }, { name: "preserves alphanumeric and underscore", hostID: "host-1", serviceID: "", serviceName: "my_service_123", - expected: "docker:host-1/service/my_service_123", + expected: "docker:host-1/service/name:my_service_123", }, { name: "preserves hyphens", hostID: "host-1", serviceID: "", serviceName: "my-service-name", - expected: "docker:host-1/service/my-service-name", + expected: "docker:host-1/service/name:my-service-name", }, { name: "trims leading/trailing dashes and underscores", hostID: "host-1", serviceID: "", serviceName: "---my-service___", - expected: "docker:host-1/service/my-service", + expected: "docker:host-1/service/name:my-service", }, { name: "truncates long derived ID to 32 chars", hostID: "host-1", serviceID: "", serviceName: "this-is-a-very-long-service-name-that-exceeds-the-limit", - expected: "docker:host-1/service/this-is-a-very-long-service-name", + expected: "docker:host-1/service/name:this-is-a-very-long-service-name", }, { - name: "uses 'service' when name is all special chars", + name: "no identity when name is all special chars", hostID: "host-1", serviceID: "", serviceName: "!!!@@@###", - expected: "docker:host-1/service/service", + expected: "", }, { - name: "uses 'service' when both ID and name empty", + name: "no identity when both ID and name empty", hostID: "host-1", serviceID: "", serviceName: "", - expected: "docker:host-1/service/service", + expected: "", }, { - name: "uses 'service' when both ID and name whitespace", + name: "no identity when both ID and name whitespace", hostID: "host-1", serviceID: " ", serviceName: " ", - expected: "docker:host-1/service/service", + expected: "", }, { name: "no host and derived name", hostID: "", serviceID: "", serviceName: "webserver", - expected: "docker-service:webserver", + expected: "docker-service:name:webserver", + }, + { + // A service named like another service's Swarm ID keeps its own reference. + name: "a name never equals an ID reference", + hostID: "host-1", + serviceID: "", + serviceName: "X7K2M9Q4W1E8R5T3Y6U0I2O4P", + expected: "docker:host-1/service/name:x7k2m9q4w1e8r5t3y6u0i2o4p", }, } @@ -13154,7 +13411,7 @@ func TestDockerContainerHealthAlert(t *testing.T) { m.CheckDockerHost(host) - resourceID := DockerResourceID(host.ID, host.Containers[0].ID) + resourceID := DockerContainerResourceID(host.ID, host.Containers[0].ID, "") alertID := fmt.Sprintf("docker-container-health-%s", resourceID) if testHasActiveAlert(t, m, alertID) { t.Fatal("expected no health alert for healthy container") @@ -13181,7 +13438,7 @@ func TestDockerContainerHealthAlert(t *testing.T) { m.CheckDockerHost(host) - resourceID := DockerResourceID(host.ID, host.Containers[0].ID) + resourceID := DockerContainerResourceID(host.ID, host.Containers[0].ID, "") alertID := fmt.Sprintf("docker-container-health-%s", resourceID) if testHasActiveAlert(t, m, alertID) { t.Fatal("expected no health alert for container with empty health") @@ -13208,7 +13465,7 @@ func TestDockerContainerHealthAlert(t *testing.T) { m.CheckDockerHost(host) - resourceID := DockerResourceID(host.ID, host.Containers[0].ID) + resourceID := DockerContainerResourceID(host.ID, host.Containers[0].ID, "") alertID := fmt.Sprintf("docker-container-health-%s", resourceID) if testHasActiveAlert(t, m, alertID) { t.Fatal("expected no health alert for container with none health") @@ -13235,7 +13492,7 @@ func TestDockerContainerHealthAlert(t *testing.T) { m.CheckDockerHost(host) - resourceID := DockerResourceID(host.ID, host.Containers[0].ID) + resourceID := DockerContainerResourceID(host.ID, host.Containers[0].ID, "") alertID := fmt.Sprintf("docker-container-health-%s", resourceID) if testHasActiveAlert(t, m, alertID) { t.Fatal("expected no health alert for starting container") @@ -13262,7 +13519,7 @@ func TestDockerContainerHealthAlert(t *testing.T) { m.CheckDockerHost(host) - resourceID := DockerResourceID(host.ID, host.Containers[0].ID) + resourceID := DockerContainerResourceID(host.ID, host.Containers[0].ID, "") alertID := fmt.Sprintf("docker-container-health-%s", resourceID) alert, exists := testLookupActiveAlert(t, m, alertID) if !exists { @@ -13302,7 +13559,7 @@ func TestDockerContainerHealthAlert(t *testing.T) { m.CheckDockerHost(host) - resourceID := DockerResourceID(host.ID, host.Containers[0].ID) + resourceID := DockerContainerResourceID(host.ID, host.Containers[0].ID, "") alertID := fmt.Sprintf("docker-container-health-%s", resourceID) alert, exists := testLookupActiveAlert(t, m, alertID) if !exists { @@ -13333,7 +13590,7 @@ func TestDockerContainerHealthAlert(t *testing.T) { m.CheckDockerHost(host) - resourceID := DockerResourceID(host.ID, host.Containers[0].ID) + resourceID := DockerContainerResourceID(host.ID, host.Containers[0].ID, "") alertID := fmt.Sprintf("docker-container-health-%s", resourceID) if !testHasActiveAlert(t, m, alertID) { t.Fatal("expected running unhealthy container to raise a health alert") @@ -13376,7 +13633,7 @@ func TestDockerContainerHealthAlert(t *testing.T) { m.CheckDockerHost(hostUnhealthy) - resourceID := DockerResourceID(hostID, containerID) + resourceID := DockerContainerResourceID(hostID, containerID, "") alertID := fmt.Sprintf("docker-container-health-%s", resourceID) if !testHasActiveAlert(t, m, alertID) { t.Fatal("expected health alert to be raised") @@ -13427,7 +13684,7 @@ func TestDockerContainerOOMKillAlert(t *testing.T) { m.CheckDockerHost(host) - resourceID := DockerResourceID(host.ID, host.Containers[0].ID) + resourceID := DockerContainerResourceID(host.ID, host.Containers[0].ID, "") alertID := fmt.Sprintf("docker-container-oom-%s", resourceID) if testHasActiveAlert(t, m, alertID) { t.Fatal("expected no OOM alert for running container") @@ -13454,7 +13711,7 @@ func TestDockerContainerOOMKillAlert(t *testing.T) { m.CheckDockerHost(host) - resourceID := DockerResourceID(host.ID, host.Containers[0].ID) + resourceID := DockerContainerResourceID(host.ID, host.Containers[0].ID, "") alertID := fmt.Sprintf("docker-container-oom-%s", resourceID) if testHasActiveAlert(t, m, alertID) { t.Fatal("expected no OOM alert for container with exit code 1") @@ -13478,7 +13735,7 @@ func TestDockerContainerOOMKillAlert(t *testing.T) { m.CheckDockerHost(host) - resourceID := DockerResourceID(host.ID, host.Containers[0].ID) + resourceID := DockerContainerResourceID(host.ID, host.Containers[0].ID, "") if testHasActiveAlert(t, m, fmt.Sprintf("docker-container-oom-%s", resourceID)) { t.Fatal("exit code 137 alone must not create an OOM alert") } @@ -13508,7 +13765,7 @@ func TestDockerContainerOOMKillAlert(t *testing.T) { m.CheckDockerHost(host) - resourceID := DockerResourceID(host.ID, host.Containers[0].ID) + resourceID := DockerContainerResourceID(host.ID, host.Containers[0].ID, "") alertID := fmt.Sprintf("docker-container-oom-%s", resourceID) alert, exists := testLookupActiveAlert(t, m, alertID) if !exists { @@ -13552,7 +13809,7 @@ func TestDockerContainerOOMKillAlert(t *testing.T) { m.CheckDockerHost(host) - resourceID := DockerResourceID(host.ID, host.Containers[0].ID) + resourceID := DockerContainerResourceID(host.ID, host.Containers[0].ID, "") alertID := fmt.Sprintf("docker-container-oom-%s", resourceID) alert, exists := testLookupActiveAlert(t, m, alertID) if !exists { @@ -13588,7 +13845,7 @@ func TestDockerContainerOOMKillAlert(t *testing.T) { // First check - should create alert m.CheckDockerHost(host) - resourceID := DockerResourceID(hostID, containerID) + resourceID := DockerContainerResourceID(hostID, containerID, "") alertID := fmt.Sprintf("docker-container-oom-%s", resourceID) alert1, exists := testLookupActiveAlert(t, m, alertID) if !exists { @@ -13633,7 +13890,7 @@ func TestDockerContainerOOMKillAlert(t *testing.T) { m.CheckDockerHost(hostOOM) - resourceID := DockerResourceID(hostID, containerID) + resourceID := DockerContainerResourceID(hostID, containerID, "") alertID := fmt.Sprintf("docker-container-oom-%s", resourceID) if !testHasActiveAlert(t, m, alertID) { t.Fatal("expected OOM alert to be raised") @@ -13687,7 +13944,7 @@ func TestDockerContainerOOMKillAlert(t *testing.T) { m.CheckDockerHost(hostOOM) - resourceID := DockerResourceID(hostID, containerID) + resourceID := DockerContainerResourceID(hostID, containerID, "") alertID := fmt.Sprintf("docker-container-oom-%s", resourceID) if !testHasActiveAlert(t, m, alertID) { t.Fatal("expected OOM alert to be raised") @@ -13738,7 +13995,7 @@ func TestDockerContainerRestartLoopAlert(t *testing.T) { m.CheckDockerHost(host) - resourceID := DockerResourceID(host.ID, host.Containers[0].ID) + resourceID := DockerContainerResourceID(host.ID, host.Containers[0].ID, "") alertID := fmt.Sprintf("docker-container-restart-loop-%s", resourceID) if testHasActiveAlert(t, m, alertID) { t.Fatal("expected no restart loop alert on first check (just initializes tracking)") @@ -13786,7 +14043,7 @@ func TestDockerContainerRestartLoopAlert(t *testing.T) { // Third check - still same restart count m.CheckDockerHost(host) - resourceID := DockerResourceID(hostID, containerID) + resourceID := DockerContainerResourceID(hostID, containerID, "") alertID := fmt.Sprintf("docker-container-restart-loop-%s", resourceID) if testHasActiveAlert(t, m, alertID) { t.Fatal("expected no restart loop alert for stable container") @@ -13827,7 +14084,7 @@ func TestDockerContainerRestartLoopAlert(t *testing.T) { host.Containers[0].RestartCount = 3 m.CheckDockerHost(host) - resourceID := DockerResourceID(hostID, containerID) + resourceID := DockerContainerResourceID(hostID, containerID, "") alertID := fmt.Sprintf("docker-container-restart-loop-%s", resourceID) if testHasActiveAlert(t, m, alertID) { t.Fatal("expected no restart loop alert when restarts <= threshold") @@ -13873,7 +14130,7 @@ func TestDockerContainerRestartLoopAlert(t *testing.T) { host.Containers[0].RestartCount = 4 m.CheckDockerHost(host) - resourceID := DockerResourceID(hostID, containerID) + resourceID := DockerContainerResourceID(hostID, containerID, "") alertID := fmt.Sprintf("docker-container-restart-loop-%s", resourceID) alert, exists := testLookupActiveAlert(t, m, alertID) if !exists { @@ -13909,7 +14166,7 @@ func TestDockerContainerRestartLoopAlert(t *testing.T) { hostID := "host-restart-5" containerID := "container-5" - resourceID := DockerResourceID(hostID, containerID) + resourceID := DockerContainerResourceID(hostID, containerID, "") // Manually set up a restart loop state m.mu.Lock() @@ -13989,7 +14246,7 @@ func TestDockerContainerRestartLoopAlert(t *testing.T) { // First check - initializes m.CheckDockerHost(host) - resourceID := DockerResourceID(hostID, containerID) + resourceID := DockerContainerResourceID(hostID, containerID, "") alertID := fmt.Sprintf("docker-container-restart-loop-%s", resourceID) // Restart 1 @@ -14042,7 +14299,7 @@ func TestDockerContainerRestartLoopAlert(t *testing.T) { host.Containers[0].RestartCount = 5 m.CheckDockerHost(host) - resourceID := DockerResourceID(hostID, containerID) + resourceID := DockerContainerResourceID(hostID, containerID, "") alertID := fmt.Sprintf("docker-container-restart-loop-%s", resourceID) alert1, exists := testLookupActiveAlert(t, m, alertID) if !exists { @@ -20293,7 +20550,7 @@ func TestDockerContainerOverrideSurvivesContainerRecreate(t *testing.T) { } } stateAlertID := func(hostID, containerID string) string { - return fmt.Sprintf("docker-container-state-%s", DockerResourceID(hostID, containerID)) + return fmt.Sprintf("docker-container-state-%s", DockerContainerResourceID(hostID, containerID, "")) } // Subtests keep the two managers in separate lifetimes: newTestManager @@ -20345,13 +20602,13 @@ func TestDockerContainerOverrideLegacyIDKeyStillHonoured(t *testing.T) { } m.mu.Lock() - m.config.Overrides[DockerResourceID(host.ID, container.ID)] = ThresholdConfig{Disabled: true} + m.config.Overrides[DockerContainerResourceID(host.ID, container.ID, "")] = ThresholdConfig{Disabled: true} m.mu.Unlock() m.CheckDockerHost(host) m.CheckDockerHost(host) - alertID := fmt.Sprintf("docker-container-state-%s", DockerResourceID(host.ID, container.ID)) + alertID := fmt.Sprintf("docker-container-state-%s", DockerContainerResourceID(host.ID, container.ID, "")) if testHasActiveAlert(t, m, alertID) { t.Fatalf("expected legacy ID-keyed override to still disable container alerts") } diff --git a/internal/alerts/canonical_identity.go b/internal/alerts/canonical_identity.go index e02961a42c..e6365029d8 100644 --- a/internal/alerts/canonical_identity.go +++ b/internal/alerts/canonical_identity.go @@ -16,6 +16,27 @@ func buildCanonicalStateID(resourceID, specID string) string { return resourceID + canonicalStateSeparator + specID } +// splitCanonicalStateID splits a canonical state ID into its resource and +// spec parts. A resource ID can itself contain the separator (a Docker host +// disambiguated as "::"), so an ID that ends with the alert's +// spec ID splits before it, and one that starts with the alert's resource ID +// splits after it. Only without either does it split at the first separator. +// The spec comes first: an alert restored from before this rule can carry a +// resource ID that was cut short at the first separator. +func splitCanonicalStateID(id, resourceID, specID string) (resource, spec string, ok bool) { + if specID = strings.TrimSpace(specID); specID != "" { + if prefix, found := strings.CutSuffix(id, canonicalStateSeparator+specID); found && prefix != "" { + return prefix, specID, true + } + } + if resourceID = strings.TrimSpace(resourceID); resourceID != "" { + if rest, found := strings.CutPrefix(id, resourceID+canonicalStateSeparator); found && rest != "" { + return resourceID, rest, true + } + } + return strings.Cut(id, canonicalStateSeparator) +} + func canonicalMetricSpecID(resourceID, metric string) string { if resourceID == "" || metric == "" { return "" @@ -149,17 +170,24 @@ func deriveCanonicalIdentity(alert *Alert) canonicalIdentity { } } +// canonicalSpecIDHint is the spec ID an alert already records, on the alert +// or in its metadata. +func canonicalSpecIDHint(alert *Alert) string { + if alert.CanonicalSpecID != "" { + return alert.CanonicalSpecID + } + specID, _ := alert.Metadata["canonicalSpecID"].(string) + return specID +} + func inferCanonicalResourceIDFromLegacyAlert(alert *Alert) string { if alert == nil { return "" } id := alert.ID - if strings.Contains(id, canonicalStateSeparator) { - parts := strings.SplitN(id, canonicalStateSeparator, 2) - if len(parts) == 2 { - return parts[0] - } + if resource, _, ok := splitCanonicalStateID(id, alert.ResourceID, canonicalSpecIDHint(alert)); ok { + return resource } switch { case strings.HasPrefix(id, "host-offline-"): @@ -230,11 +258,8 @@ func inferCanonicalSpecIDFromLegacyAlert(alert *Alert) string { return "" } id := alert.ID - if strings.Contains(id, canonicalStateSeparator) { - parts := strings.SplitN(id, canonicalStateSeparator, 2) - if len(parts) == 2 { - return parts[1] - } + if _, spec, ok := splitCanonicalStateID(id, alert.ResourceID, canonicalSpecIDHint(alert)); ok { + return spec } resourceID := inferCanonicalResourceIDFromLegacyAlert(alert) switch { diff --git a/internal/alerts/cleanup_observation_retention_test.go b/internal/alerts/cleanup_observation_retention_test.go index 9670acc63a..5a4081e1ed 100644 --- a/internal/alerts/cleanup_observation_retention_test.go +++ b/internal/alerts/cleanup_observation_retention_test.go @@ -48,7 +48,7 @@ func TestCleanupContinuingDockerUpdateKeepsOccurrence(t *testing.T) { LastChecked: time.Now().Add(-6 * time.Hour), } container := models.DockerContainer{ID: "retention-container", Name: "web", Image: "mongo:7", UpdateStatus: pending} - resourceID := DockerResourceID(host.ID, container.ID) + resourceID := DockerContainerResourceID(host.ID, container.ID, "") trackingKey := dockerUpdateTrackingKey(host, container) first := time.Now().Add(-25 * time.Hour) m.mu.Lock() diff --git a/internal/alerts/config_runtime.go b/internal/alerts/config_runtime.go index 308d134046..3b6259e71e 100644 --- a/internal/alerts/config_runtime.go +++ b/internal/alerts/config_runtime.go @@ -284,41 +284,39 @@ func (m *Manager) applyGlobalOfflineSettingsLocked() { } } -// dockerAlertResourcePath returns the portion of the alert's resource ID after -// the "docker:" scheme, matching the IDs built by DockerResourceID and -// DockerServiceResourceID. Canonical alerts are stored under +// dockerAlertResourcePath splits the alert's resource ID after the "docker:" +// scheme into the host and the child path, matching the IDs built by +// DockerHostResourceID (""), DockerContainerResourceID +// ("/" or "/name:") and +// DockerServiceResourceID ("/service/" or +// "/service/name:"). child is empty for a host's own alert. A +// name can contain anything, so callers classify by the first segment of +// child, never by searching the whole path. Canonical alerts are stored under // "::" state IDs, so the legacy "docker-container-" / // "docker-service-" alert-ID prefixes never match them. -func dockerAlertResourcePath(alert *Alert) (string, bool) { +func dockerAlertResourcePath(alert *Alert) (host, child string, ok bool) { if alert == nil { - return "", false + return "", "", false } - resourceID := strings.TrimSpace(alert.ResourceID) - if !strings.HasPrefix(resourceID, "docker:") { - return "", false + path, found := strings.CutPrefix(strings.TrimSpace(alert.ResourceID), "docker:") + if !found { + return "", "", false } - return strings.TrimPrefix(resourceID, "docker:"), true + host, child, _ = strings.Cut(path, "/") + return host, child, true } func isDockerContainerAlert(alert *Alert) bool { - path, ok := dockerAlertResourcePath(alert) - if !ok { - return false - } - // "hostID/containerID" has a path separator; host-level IDs ("hostID") do - // not, and services use "hostID/service/serviceID". - return strings.Contains(path, "/") && !strings.Contains(path, "/service/") + _, child, ok := dockerAlertResourcePath(alert) + return ok && child != "" && !strings.HasPrefix(child, "service/") } func isDockerServiceAlert(alert *Alert) bool { if alert != nil && strings.HasPrefix(strings.TrimSpace(alert.ResourceID), "docker-service:") { return true } - path, ok := dockerAlertResourcePath(alert) - if !ok { - return false - } - return strings.Contains(path, "/service/") + _, child, ok := dockerAlertResourcePath(alert) + return ok && strings.HasPrefix(child, "service/") } func alertPrimaryResourceType(alert *Alert) string { diff --git a/internal/alerts/docker.go b/internal/alerts/docker.go index 773d3c645e..3eaa0d370a 100644 --- a/internal/alerts/docker.go +++ b/internal/alerts/docker.go @@ -80,22 +80,43 @@ func (m *Manager) lookupDockerContainerOverrideNoLock(hostID, containerName, leg return ThresholdConfig{}, false } -// DockerResourceID builds a stable identifier for Docker container alerts. -// Patrol scope resolution registers these forms as known aliases so an -// alert's resource ID resolves to the collected container or host. -func DockerResourceID(hostID, containerID string) string { +// dockerNameReferencePrefix marks the name of a Docker container or Swarm +// service that has no ID in the slot of an alert reference that otherwise +// holds the ID. Docker and Swarm IDs never contain a colon, so a name +// reference can never equal an ID reference or the host's own reference. +const dockerNameReferencePrefix = "name:" + +// DockerHostResourceID builds the reference of a Docker host's own alerts +// ("docker:"). Only the host's connectivity alert uses it. Returns "" +// without a host ID. +func DockerHostResourceID(hostID string) string { hostID = strings.TrimSpace(hostID) - containerID = strings.TrimSpace(containerID) - if containerID == "" { - if hostID == "" { - return "docker:unknown" + if hostID == "" { + return "" + } + return "docker:" + hostID +} + +// DockerContainerResourceID builds the reference of a Docker container's +// alerts: "docker:/". A container reported without an +// ID is referenced by its name, "docker:/name:", so it never +// shares its host's reference or another container's. Returns "" for a +// container with neither, which has no identity to alert under. Patrol scope +// resolution registers the same forms as aliases of the collected container. +func DockerContainerResourceID(hostID, containerID, containerName string) string { + hostID = strings.TrimSpace(hostID) + ref := strings.TrimSpace(containerID) + if ref == "" { + name := strings.TrimLeft(strings.TrimSpace(containerName), "/") + if hostID == "" || name == "" { + return "" } - return fmt.Sprintf("docker:%s", hostID) + ref = dockerNameReferencePrefix + name } if hostID == "" { - return fmt.Sprintf("docker:container/%s", containerID) + return fmt.Sprintf("docker:container/%s", ref) } - return fmt.Sprintf("docker:%s/%s", hostID, containerID) + return fmt.Sprintf("docker:%s/%s", hostID, ref) } func normalizeDockerUpdateTrackingPart(part string) string { @@ -164,17 +185,18 @@ func dockerServiceDisplayName(service models.DockerService) string { return serviceID } -// DockerServiceResourceID builds a stable identifier for Docker Swarm service -// alerts. Patrol scope resolution registers this form as a known alias on the -// owning Docker host record so a service alert's resource ID resolves there. +// DockerServiceResourceID builds the reference of a Docker Swarm service's +// alerts: "docker:/service/". A service reported without +// an ID is referenced by its normalized name, "docker:/service/name:", +// so it never shares another service's ID reference. Returns "" for a service +// with neither, which has no identity to alert under. Patrol scope resolution +// registers the same forms as aliases on the owning Docker host record so a +// service alert's resource ID resolves there. func DockerServiceResourceID(hostID, serviceID, serviceName string) string { hostID = strings.TrimSpace(hostID) normalizedServiceID := strings.TrimSpace(serviceID) if normalizedServiceID == "" { name := strings.TrimSpace(serviceName) - if name == "" { - name = "service" - } builder := strings.Builder{} for _, r := range strings.ToLower(name) { switch { @@ -190,11 +212,12 @@ func DockerServiceResourceID(hostID, serviceID, serviceName string) string { } normalizedServiceID = strings.Trim(builder.String(), "-_") if normalizedServiceID == "" { - normalizedServiceID = "service" + return "" } if len(normalizedServiceID) > 32 { normalizedServiceID = normalizedServiceID[:32] } + normalizedServiceID = dockerNameReferencePrefix + normalizedServiceID } if hostID == "" { return fmt.Sprintf("docker-service:%s", normalizedServiceID) @@ -272,8 +295,12 @@ func (m *Manager) CheckDockerHost(host models.DockerHost) { seen := make(map[string]struct{}, len(host.Containers)+len(host.Services)) seenUpdateTracking := make(map[string]struct{}, len(host.Containers)) for _, container := range host.Containers { + resourceID := DockerContainerResourceID(host.ID, container.ID, container.Name) + if resourceID == "" { + // Neither an ID nor a name: nothing identifies its alerts. + continue + } containerName := dockerContainerDisplayName(container) - resourceID := DockerResourceID(host.ID, container.ID) updateTrackingKey := dockerUpdateTrackingKey(host, container) if matchesDockerIgnoredPrefix(containerName, container.ID, ignoredPrefixes) { @@ -301,6 +328,9 @@ func (m *Manager) CheckDockerHost(host models.DockerHost) { for _, service := range host.Services { resourceID := DockerServiceResourceID(host.ID, service.ID, service.Name) + if resourceID == "" { + continue + } seen[resourceID] = struct{}{} m.evaluateDockerService(host, service, resourceID) } @@ -656,7 +686,7 @@ func (m *Manager) HandleDockerHostOnline(host models.DockerHost) { return } - resourceID := fmt.Sprintf("docker:%s", strings.TrimSpace(host.ID)) + resourceID := DockerHostResourceID(host.ID) alertID := canonicalConnectivityStateID(resourceID) m.mu.Lock() @@ -697,7 +727,7 @@ func (m *Manager) HandleDockerHostOffline(host models.DockerHost) { _, disableDockerHostsOffline := m.alertPolicyTypeSwitchesNoLock("docker-host") m.mu.RUnlock() - resourceID := fmt.Sprintf("docker:%s", strings.TrimSpace(host.ID)) + resourceID := DockerHostResourceID(host.ID) alertID := canonicalConnectivityStateID(resourceID) instanceName := dockerInstanceName(host) nodeName := strings.TrimSpace(host.Hostname) @@ -1400,6 +1430,16 @@ func (m *Manager) checkDockerContainerImageUpdate(host models.DockerHost, contai active.Type == "docker-container-update" && active.ResourceID == resourceID && !active.StartTime.IsZero() { firstSeen, exists = active.StartTime, true + } else if strings.TrimSpace(container.ID) == "" { + // A container without an ID used to record its pending update under + // its host's reference. Keep that age; the host's report cleanup then + // clears the old occurrence. + legacyRef := DockerHostResourceID(host.ID) + if legacy, ok := m.getActiveAlertNoLock(buildCanonicalStateID(legacyRef, legacyRef+"-image-update")); ok && + isLegacyDockerHostContainerAlert(legacy, host) && legacy.Type == "docker-container-update" && + alertMetadataString(legacy, "containerName") == containerName && !legacy.StartTime.IsZero() { + firstSeen, exists = legacy.StartTime, true + } } if !exists { firstSeen = time.Now() @@ -1480,6 +1520,19 @@ func (m *Manager) cleanupDockerContainerAlerts(host models.DockerHost, seen map[ m.cleanupDockerContainerAlertsWithTracking(host, seen, nil) } +// isLegacyDockerHostContainerAlert reports an alert that a container without +// an ID on this host raised under the host's own reference, before such +// containers had a reference of their own. Only the host's connectivity alert +// belongs there, so no producer refreshes these. The recorded host and empty +// container ID must match too: an alert restored with a resource ID cut short +// at a "::" in another host's ID ("::") can carry this host's +// reference. +func isLegacyDockerHostContainerAlert(alert *Alert, host models.DockerHost) bool { + hostRef := DockerHostResourceID(host.ID) + return alert != nil && hostRef != "" && alert.ResourceID == hostRef && alert.Type != "docker-host-offline" && + alertMetadataString(alert, "hostId") == strings.TrimSpace(host.ID) && alertMetadataString(alert, "containerId") == "" +} + func (m *Manager) cleanupDockerContainerAlertsWithTracking(host models.DockerHost, seen map[string]struct{}, seenUpdateTracking map[string]struct{}) { prefix := fmt.Sprintf("docker:%s/", strings.TrimSpace(host.ID)) updateTrackingPrefix := dockerUpdateTrackingHostPrefix(host) @@ -1488,6 +1541,10 @@ func (m *Manager) cleanupDockerContainerAlertsWithTracking(host models.DockerHos toClear := make([]string, 0) for storageKey, alert := range m.activeAlerts { alertID := effectiveAlertID(alert, storageKey) + if isLegacyDockerHostContainerAlert(alert, host) { + toClear = append(toClear, alertID) + continue + } if !strings.HasPrefix(alert.ResourceID, prefix) { continue } @@ -1540,7 +1597,7 @@ func (m *Manager) clearDockerHostContainerAlerts(host models.DockerHost) { toClear := make([]string, 0) for storageKey, alert := range m.activeAlerts { alertID := effectiveAlertID(alert, storageKey) - if strings.HasPrefix(alert.ResourceID, prefix) { + if strings.HasPrefix(alert.ResourceID, prefix) || isLegacyDockerHostContainerAlert(alert, host) { toClear = append(toClear, alertID) } } diff --git a/internal/alerts/docker_override_migration.go b/internal/alerts/docker_override_migration.go index e9b32b838d..9599c87d1e 100644 --- a/internal/alerts/docker_override_migration.go +++ b/internal/alerts/docker_override_migration.go @@ -75,7 +75,7 @@ func MigrateDockerContainerOverrideKeys(config *AlertConfig, resources []unified legacyKey := "" if containerID := strings.TrimSpace(resource.Docker.ContainerID); containerID != "" { - legacyKey = DockerResourceID(hostID, containerID) + legacyKey = DockerContainerResourceID(hostID, containerID, "") live[legacyKey] = struct{}{} } if legacyKey != "" && legacyKey != stableKey { diff --git a/internal/alerts/unified_eval_test.go b/internal/alerts/unified_eval_test.go index e5c732cee1..bdb0b7080a 100644 --- a/internal/alerts/unified_eval_test.go +++ b/internal/alerts/unified_eval_test.go @@ -1287,7 +1287,7 @@ func TestCheckDockerContainerStateAnnotatesCanonicalSpecMetadata(t *testing.T) { m.CheckDockerHost(host) m.CheckDockerHost(host) - resourceID := DockerResourceID(host.ID, "container-1") + resourceID := DockerContainerResourceID(host.ID, "container-1", "") alert := activeAlert(t, m, "docker-container-state-"+resourceID) if got := alert.Metadata["canonicalAlertKind"]; got != "discrete-state" { t.Fatalf("canonicalAlertKind = %v, want discrete-state", got) diff --git a/internal/alerts/update_alerts_test.go b/internal/alerts/update_alerts_test.go index 83093943cd..644bb3133a 100644 --- a/internal/alerts/update_alerts_test.go +++ b/internal/alerts/update_alerts_test.go @@ -469,7 +469,7 @@ func TestCheckDockerContainerImageUpdatePreservesDelayAcrossHostIDChange(t *test newHost.ID = "docker-host-new" newHost.DisplayName = "New Docker Host" - oldResourceID := DockerResourceID(oldHost.ID, container.ID) + oldResourceID := DockerContainerResourceID(oldHost.ID, container.ID, "") m.checkDockerContainerImageUpdate(oldHost, container, oldResourceID, "web", "docker-instance", "docker.local") firstSeen := time.Now().Add(-25 * time.Hour) @@ -479,7 +479,7 @@ func TestCheckDockerContainerImageUpdatePreservesDelayAcrossHostIDChange(t *test m.dockerUpdateFirstSeenByIdentity[trackingKey] = firstSeen m.mu.Unlock() - newResourceID := DockerResourceID(newHost.ID, container.ID) + newResourceID := DockerContainerResourceID(newHost.ID, container.ID, "") m.checkDockerContainerImageUpdate(newHost, container, newResourceID, "web", "docker-instance", "docker.local") alertID := "docker-container-update-" + newResourceID @@ -639,7 +639,7 @@ func TestUpdateConfigClearsDockerContainerUpdateAlertsWhenDisabled(t *testing.T) Name: "/frontend", Image: "nginx:latest", } - resourceID := DockerResourceID(host.ID, container.ID) + resourceID := DockerContainerResourceID(host.ID, container.ID, "") firstSeen := time.Now().Add(-48 * time.Hour) _, canonicalAlertID, trackingKey := seedDockerUpdateAlert(t, m, host, container, resourceID, firstSeen) @@ -677,7 +677,7 @@ func TestUpdateConfigKeepsDockerContainerUpdateAlertsWhenStillEnabled(t *testing Name: "/api", Image: "ghcr.io/example/api:latest", } - resourceID := DockerResourceID(host.ID, container.ID) + resourceID := DockerContainerResourceID(host.ID, container.ID, "") firstSeen := time.Now().Add(-30 * time.Hour) _, canonicalAlertID, trackingKey := seedDockerUpdateAlert(t, m, host, container, resourceID, firstSeen) @@ -723,7 +723,7 @@ func TestEvaluateDockerContainerClearsUpdateAlertWhenOverrideDisabled(t *testing Name: "/worker", Image: "ghcr.io/example/worker:latest", } - resourceID := DockerResourceID(host.ID, container.ID) + resourceID := DockerContainerResourceID(host.ID, container.ID, "") firstSeen := time.Now().Add(-36 * time.Hour) alertID, canonicalAlertID, trackingKey := seedDockerUpdateAlert(t, m, host, container, resourceID, firstSeen) @@ -884,7 +884,7 @@ func TestDockerUpdateRestartRestoresActivePendingAge(t *testing.T) { } container := models.DockerContainer{ID: "container", Name: "web", Image: "mongo:7", State: "running", UpdateStatus: pending} host := models.DockerHost{ID: "host", AgentID: "agent", Hostname: "host", Containers: []models.DockerContainer{container}} - resourceID := DockerResourceID(host.ID, container.ID) + resourceID := DockerContainerResourceID(host.ID, container.ID, "") canonicalID := buildCanonicalStateID(resourceID, resourceID+"-image-update") trackingKey := dockerUpdateTrackingKey(host, container) first := time.Now().Add(-time.Duration(delay+2) * time.Hour) diff --git a/internal/monitoring/monitor_alert_handling_test.go b/internal/monitoring/monitor_alert_handling_test.go index a8b7454b6c..3a5b27f43f 100644 --- a/internal/monitoring/monitor_alert_handling_test.go +++ b/internal/monitoring/monitor_alert_handling_test.go @@ -153,7 +153,10 @@ func TestProxmoxAlertTimelineUsesCanonicalHistoryIdentity(t *testing.T) { // Docker host, Swarm service and sub-resource alerts (a ZFS pool and device, // a host filesystem) carry references the registry has no resource for. Their // lifecycle must reach the owner's canonical history, so the producers' ref -// shapes are pinned here against the real alert manager. +// shapes are pinned here against the real alert manager. A container or +// service reported without an ID alerts under its own name reference, so it +// joins neither its host's history nor that of the service whose ID it is +// named like. func TestOwnerAlertTimelinesUseCanonicalHistoryIdentity(t *testing.T) { store, err := unifiedresources.NewSQLiteResourceStore(t.TempDir(), "default") require.NoError(t, err) @@ -169,8 +172,10 @@ func TestOwnerAlertTimelinesUseCanonicalHistoryIdentity(t *testing.T) { manager.UpdateConfig(config) now := time.Now() dockerHost := models.DockerHost{ID: "dh-7f3a", Hostname: "tower", DisplayName: "tower", Status: "online", LastSeen: now, - Swarm: &models.DockerSwarmInfo{NodeID: "node-a", NodeRole: "manager", LocalState: "active", ControlAvailable: true, ClusterID: "swarm-1"}, - Services: []models.DockerService{{ID: "x7k2m9q4w1e8r5t3y6u0i2o4p", Name: "web", DesiredTasks: 2, RunningTasks: 0}}} + Swarm: &models.DockerSwarmInfo{NodeID: "node-a", NodeRole: "manager", LocalState: "active", ControlAvailable: true, ClusterID: "swarm-1"}, + Containers: []models.DockerContainer{{Name: "sidecar", State: "running", Health: "unhealthy"}}, + Services: []models.DockerService{{ID: "x7k2m9q4w1e8r5t3y6u0i2o4p", Name: "web", DesiredTasks: 2, RunningTasks: 0}, + {Name: "X7K2M9Q4W1E8R5T3Y6U0I2O4P", DesiredTasks: 2, RunningTasks: 0}}} storage := models.Storage{ID: "lab-pve1-local-zfs", Name: "local-zfs", Node: "pve1", Instance: "lab", Type: "zfspool", Status: "available", Total: 100, Used: 10, Usage: 10, Enabled: true, Active: true, LastSeen: now, ZFSPool: &models.ZFSPool{Name: "rpool", State: "DEGRADED", Status: "Degraded", Devices: []models.ZFSDevice{{Name: "sda2", Type: "disk", State: "FAULTED"}}}} agentHost := models.Host{ID: "host-nas", Hostname: "nas", DisplayName: "nas", MachineID: "fedcba9876543210", Status: "online", LastSeen: now, @@ -209,6 +214,23 @@ func TestOwnerAlertTimelinesUseCanonicalHistoryIdentity(t *testing.T) { } require.True(t, found, "%s alert missing from %s history: %+v", alertType, canonicalID, changes) } + alertTypes := func(ref string) []any { + changes, err := store.GetRecentChangesFiltered(ref, time.Time{}, 10, filters) + require.NoError(t, err) + types := make([]any, 0, len(changes)) + for _, change := range changes { + types = append(types, change.Metadata["alert_type"]) + } + return types + } + require.ElementsMatch(t, []any{"docker-host-offline"}, alertTypes(owner["agent/tower"])) + require.ElementsMatch(t, []any{"docker-service-health"}, alertTypes(owner["docker-service/web"])) + require.ElementsMatch(t, []any{"docker-container-health"}, alertTypes("docker:dh-7f3a/name:sidecar")) + require.ElementsMatch(t, []any{"docker-service-health"}, alertTypes("docker:dh-7f3a/service/name:x7k2m9q4w1e8r5t3y6u0i2o4p")) + for _, name := range []string{"app-container/sidecar", "docker-service/X7K2M9Q4W1E8R5T3Y6U0I2O4P"} { + require.NotEmpty(t, owner[name], name) + require.Empty(t, alertTypes(owner[name]), "a name binds nothing: %s", name) + } } func TestMonitor_HandleAlertFired_Extra(t *testing.T) { diff --git a/internal/unifiedresources/history_identity.go b/internal/unifiedresources/history_identity.go index 61ddc1ce1b..c6d71b126b 100644 --- a/internal/unifiedresources/history_identity.go +++ b/internal/unifiedresources/history_identity.go @@ -26,7 +26,7 @@ func legacyDockerHistoryIdentity(ref string) (sourceID, canonicalID string, ok b return "", "", false } sourceID = host + "/container/" + container - if host == "container" { // DockerResourceID's explicit hostless form. + if host == "container" { // DockerContainerResourceID's explicit hostless form. sourceID = container } return sourceID, SourceSpecificID(ResourceTypeAppContainer, SourceDocker, sourceID), true @@ -43,19 +43,23 @@ func isDockerHistoryReference(ref string) bool { } // isDockerNameHistoryReference marks Docker alert references that are not host -// or service references: container references, which only a complete -// container ID binds, and hostless service names. A name or shortened ID must -// never join history, not even through a retained binding. +// or service ID references: container references, which only a complete +// container ID binds, the names of containers and services reported without an +// ID, and hostless service names. A name or shortened ID must never join +// history, not even through a retained binding. func isDockerNameHistoryReference(ref string) bool { _, _, ok := dockerHostHistoryReference(ref) return isDockerHistoryReference(ref) && !ok } // dockerHostHistoryReference parses the Docker host alert reference -// ("docker:", alerts.DockerResourceID without a container) and the -// Swarm service alert reference ("docker:/service/", +// ("docker:", alerts.DockerHostResourceID) and the Swarm service +// alert reference ("docker:/service/", // alerts.DockerServiceResourceID). serviceID is empty for a host reference. -// "docker:unknown" is the shared fallback for alerts without a host. +// A service reported without an ID alerts under "name:" in the ID +// position, and Swarm IDs never contain a colon, so that is a name, not a +// service reference. "docker:unknown" was the old builder's fallback for +// alerts without a host. func dockerHostHistoryReference(ref string) (hostID, serviceID string, ok bool) { rest, found := strings.CutPrefix(strings.TrimSpace(ref), "docker:") if !found { @@ -65,7 +69,7 @@ func dockerHostHistoryReference(ref string) (hostID, serviceID string, ok bool) if hostID == "" || hostID == "unknown" || strings.TrimSpace(hostID) != hostID || strings.Contains(hostID, "/") { return "", "", false } - if isService && (serviceID == "" || strings.TrimSpace(serviceID) != serviceID || strings.Contains(serviceID, "/")) { + if isService && (serviceID == "" || strings.TrimSpace(serviceID) != serviceID || strings.ContainsAny(serviceID, "/:")) { return "", "", false } return hostID, serviceID, true @@ -74,43 +78,37 @@ func dockerHostHistoryReference(ref string) (hostID, serviceID string, ok bool) // dockerHistoryOwnerLocked resolves a Docker host or Swarm service alert // reference through the registry's own Docker source identities. The host ID // is the Docker host's source ID. A service reference names the service with -// that ID in the host's Swarm cluster, which every manager of the cluster -// reports under its own host ID. A service without an ID is referenced by its -// normalized name in the same place, so while the cluster has such a service -// every service reference there is a conflict. -func (rr *ResourceRegistry) dockerHistoryOwnerLocked(ref string) (resourceID string, conflict bool) { +// that exact ID in the host's Swarm cluster, which every manager of the +// cluster reports under its own host ID. +func (rr *ResourceRegistry) dockerHistoryOwnerLocked(ref string) string { hostID, serviceID, ok := dockerHostHistoryReference(ref) if !ok { - return "", false + return "" } hostResourceID := rr.bySource[SourceDocker][hostID] host := rr.resources[hostResourceID] if host == nil || CanonicalResourceType(host.Type) != ResourceTypeAgent { - return "", false + return "" } if serviceID == "" { - return hostResourceID, false + return hostResourceID } if host.Docker == nil { - return "", false + return "" } cluster := dockerSwarmClusterKeyFromMeta(host.Docker.Swarm) if cluster == "" { - return "", false - } - for _, other := range rr.resources { - if CanonicalResourceType(other.Type) == ResourceTypeDockerService && other.Docker != nil && - strings.TrimSpace(other.Docker.ServiceID) == "" && dockerSwarmClusterKeyFromMeta(other.Docker.Swarm) == cluster { - return "", true - } + return "" } + // The registry keys a service without an ID by its raw name, so the entry + // must carry exactly this ID. serviceResourceID := rr.bySource[SourceDocker][normalizeSourceID(cluster+":service:"+serviceID)] service := rr.resources[serviceResourceID] if service == nil || CanonicalResourceType(service.Type) != ResourceTypeDockerService || service.Docker == nil || strings.TrimSpace(service.Docker.ServiceID) != serviceID { - return "", false + return "" } - return serviceResourceID, false + return serviceResourceID } // historySubResourceOwner names the owner of a sub-resource alert reference, @@ -169,8 +167,8 @@ func (rr *ResourceRegistry) resolveHistoryReference(ref string) (resourceID stri rr.mu.RLock() defer rr.mu.RUnlock() if isDockerHistoryReference(ref) { - resourceID, conflict := rr.dockerHistoryOwnerLocked(ref) - return resourceID, resourceID != "" || conflict + resourceID = rr.dockerHistoryOwnerLocked(ref) + return resourceID, resourceID != "" } matches := rr.durableHistoryMatchesLocked(ref) if ownerRef, ownerType, ok := historySubResourceOwner(ref); ok && len(matches) == 0 { diff --git a/internal/unifiedresources/history_identity_test.go b/internal/unifiedresources/history_identity_test.go index 4744e08503..c6e183f117 100644 --- a/internal/unifiedresources/history_identity_test.go +++ b/internal/unifiedresources/history_identity_test.go @@ -391,6 +391,8 @@ func TestHistoryIdentityMonitorAdapterResolvesDockerHostAndServiceReferences(t * hostA.AgentID, hostA.MachineID = "host-tower", "0123456789abcdef" hostA.Containers = []models.DockerContainer{{ID: container, Name: "worker", State: "running"}} hostB := manager("dh-91c0", "rack", "swarm-1", web) + // A container without an ID, named like its host's ID. + hostB.Containers = []models.DockerContainer{{Name: "dh-91c0", State: "running"}} // In swarm-2 a service without an ID is named like another service's ID. hostC := manager("dh-c4d2", "edge", "swarm-2", models.DockerService{ID: otherServiceID, Name: "api"}, models.DockerService{Name: strings.ToUpper(otherServiceID)}) // Rows journaled before this process started are bound by a later generation. @@ -407,6 +409,7 @@ func TestHistoryIdentityMonitorAdapterResolvesDockerHostAndServiceReferences(t * hostAID := registry.sourceResourceID(SourceDocker, "dh-7f3a") hostBID := registry.sourceResourceID(SourceDocker, "dh-91c0") serviceResourceID := historyIdentityResourceID(t, registry, ResourceTypeDockerService, "web") + apiID := historyIdentityResourceID(t, registry, ResourceTypeDockerService, "api") containerID := registry.sourceResourceID(SourceDocker, "dh-7f3a/container/"+container) require.Equal(t, registry.sourceResourceID(SourceAgent, "host-tower"), hostAID, "the Docker host merges into its agent's machine") require.NotEqual(t, hostAID, hostBID) @@ -417,17 +420,22 @@ func TestHistoryIdentityMonitorAdapterResolvesDockerHostAndServiceReferences(t * "docker:dh-91c0/service/" + serviceID: serviceResourceID, "docker:dh-7f3a": hostAID, "docker:dh-7f3a/service/" + serviceID: serviceResourceID, + // A service without an ID in the same cluster no longer shares this + // reference, so it binds. + "docker:dh-c4d2/service/" + otherServiceID: apiID, } unbound := []string{ - "docker:tower", // hostname - "docker:dh-7f3a/service/web", // service name - "docker:dh-c4d2/service/" + otherServiceID, // its cluster has a service without an ID - "docker-service:web", // hostless service name - "docker:dh-7f3a/worker", // container name - "docker:dh-7f3a/" + container[:12], // shortened container ID + "docker:tower", // hostname + "docker:dh-7f3a/service/web", // service name + "docker:dh-c4d2/service/name:" + otherServiceID, // service without an ID + "docker-service:name:web", // hostless service name + "docker:dh-7f3a/worker", // container name + "docker:dh-7f3a/" + container[:12], // shortened container ID + "docker:dh-91c0/name:dh-91c0", // container without an ID "docker:unknown", } - for i, ref := range append([]string{"docker:dh-7f3a", "docker:dh-7f3a/service/" + serviceID}, unbound...) { + ids := []string{"docker:dh-7f3a", "docker:dh-7f3a/service/" + serviceID, "docker:dh-c4d2/service/" + otherServiceID} + for i, ref := range append(ids, unbound...) { if ref == "docker:dh-7f3a/worker" { continue // journaled above } @@ -436,29 +444,29 @@ func TestHistoryIdentityMonitorAdapterResolvesDockerHostAndServiceReferences(t * requireHistoryReferenceBindings(t, store, bound, unbound) // Names and shortened IDs are never retried against later inventory. require.Contains(t, adapter.legacyHistory.pending, "docker:tower") - for _, ref := range []string{"docker:dh-7f3a/worker", "docker:dh-7f3a/" + container[:12], "docker-service:web"} { + for _, ref := range []string{"docker:dh-7f3a/worker", "docker:dh-7f3a/" + container[:12], "docker-service:name:web", + "docker:dh-91c0/name:dh-91c0", "docker:dh-c4d2/service/name:" + otherServiceID} { require.NotContains(t, adapter.legacyHistory.pending, ref) } got, err := store.GetRecentChangesFiltered(containerID, time.Time{}, 10, ResourceChangeFilters{Kinds: []ChangeKind{ChangeAlertFired}}) require.NoError(t, err) require.Empty(t, got, "a container name or short ID never joins the container's history") - // While its cluster has a service without an ID, a service reference is a - // conflict and must not follow a binding recorded earlier. - collision := "docker:dh-c4d2/service/" + otherServiceID - apiID := historyIdentityResourceID(t, registry, ResourceTypeDockerService, "api") - require.NoError(t, store.RecordChangeWithSourceIdentity(ResourceChange{ID: "earlier-api", ResourceID: apiID, Kind: ChangeAlertFired, SourceType: SourceHeuristic, ObservedAt: now}, collision)) - require.NoError(t, adapter.RecordChange(ResourceChange{ID: "conflict-api", ResourceID: collision, Kind: ChangeAlertFired, SourceType: SourceHeuristic, ObservedAt: now.Add(2 * time.Minute)})) - // The same holds once the service with that ID has left inventory. + // The service's ID reference follows its binding once the service has left + // inventory, while the service without an ID is still there; the name + // reference stays its own. hostC.Services = hostC.Services[1:] adapter.PopulateFromSnapshot(models.StateSnapshot{ Hosts: []models.Host{{ID: "host-tower", Hostname: "tower", MachineID: "0123456789abcdef", Status: "online", LastSeen: now}}, DockerHosts: []models.DockerHost{hostA, hostB, hostC}, }) - require.NoError(t, adapter.RecordChange(ResourceChange{ID: "conflict-api-removed", ResourceID: collision, Kind: ChangeAlertFired, SourceType: SourceHeuristic, ObservedAt: now.Add(3 * time.Minute)})) - for _, id := range []string{"conflict-api", "conflict-api-removed"} { + nameRef := "docker:dh-c4d2/service/name:" + otherServiceID + for id, ref := range map[string]string{"api-removed": "docker:dh-c4d2/service/" + otherServiceID, "name-removed": nameRef} { + require.NoError(t, adapter.RecordChange(ResourceChange{ID: id, ResourceID: ref, Kind: ChangeAlertResolved, SourceType: SourceHeuristic, ObservedAt: now.Add(3 * time.Minute)})) + } + for id, want := range map[string]string{"api-removed": apiID, "name-removed": nameRef} { var recordedAs string require.NoError(t, store.db.QueryRow(`SELECT canonical_id FROM resource_changes WHERE id = ?`, id).Scan(&recordedAs)) - require.Equal(t, collision, recordedAs, id) + require.Equal(t, want, recordedAs, id) } // After the service leaves inventory its alerts follow the retained binding;