diff --git a/internal/curator/draft.go b/internal/curator/draft.go index 2769b13c..0468b0fd 100644 --- a/internal/curator/draft.go +++ b/internal/curator/draft.go @@ -168,7 +168,7 @@ func alertResourceIfDistinct(inv providers.Investigation) string { // exact source; only the value the model happened to produce differed, so fixing // one path and not the other would leave the same defect armed here. func normalizeResource(w providers.Workload) string { - w.Name = normalizeResourceName(w.Name) + w.Name = normalizeResourceName(w) ref, _ := kbvalidate.DraftResource(w.Ref()) return ref } diff --git a/internal/curator/fingerprint.go b/internal/curator/fingerprint.go index 8136daa6..44f54e5e 100644 --- a/internal/curator/fingerprint.go +++ b/internal/curator/fingerprint.go @@ -38,21 +38,24 @@ func normalizeText(s string) string { return strings.Join(strings.Fields(s), " ") } -// normalizeResourceName reduces a resource name to the identity the curator groups +// normalizeResourceName reduces a workload's name to the identity the curator groups // by: an AWS ARN collapses to the resource identifier its CloudWatch dimension -// carries, and a trailing pod-name hash is stripped so a per-pod name reduces to its -// controller family. It is a thin alias for providers.NormalizeResourceName, the -// single source of truth shared with the instant-recall path (CORE-681) so the two -// can never drift. +// carries, and a trailing per-instance suffix is stripped so a per-pod name reduces +// to its controller family — a pod hash, a CronJob run stamp and, when the Kind says +// the name is a Pod's, a StatefulSet replica ordinal (#513). It is a thin alias for +// providers.Workload.IdentityName, the single source of truth shared with the +// instant-recall path (CORE-681) so the two can never drift. // -// It is deliberately NOT named normalizeWorkloadName any more: providers also -// exports a NormalizeWorkloadName that does only the per-instance-suffix half — pod -// hashes and CronJob run stamps — and a local alias wearing that exact name would -// read as a pass-through to it while meaning something wider. Read providers.NormalizeResourceName for the accepted consequence of the ARN -// collapse — the account and region are dropped, and the surrounding key fields do -// not reliably put them back. -func normalizeResourceName(name string) string { - return providers.NormalizeResourceName(name) +// It takes the Workload and not the bare name because the Kind is part of the rule: +// on a kind-less name a trailing ordinal is the name, and no string-level normalizer +// may fold it. It is deliberately NOT named normalizeWorkloadName: providers also +// exports a NormalizeWorkloadName that does only the per-instance-suffix half, and a +// local alias wearing that exact name would read as a pass-through to it while +// meaning something wider. Read providers.NormalizeResourceName for the accepted +// consequence of the ARN collapse — the account and region are dropped, and the +// surrounding key fields do not reliably put them back. +func normalizeResourceName(w providers.Workload) string { + return w.IdentityName() } // IncidentKey builds a host-invariant, per-class dedup key for an alert: the alert @@ -88,12 +91,15 @@ func normalizeResourceName(name string) string { // The same one-time reset now also reaches CronJob-generated workloads: // NormalizeWorkloadName folds the run suffix into the CronJob family, // so github-teams-sync-29787720 and -29790030 key alike where they used to key apart. +// StatefulSet replicas reach it the same way (#513): with Kind "Pod", +// vmagent-vmagent-0 and -1 key alike where they used to key apart. func IncidentKey(alertname string, w providers.Workload, cluster string) string { + w.Name = strings.TrimSpace(w.Name) parts := []string{ strings.TrimSpace(alertname), strings.TrimSpace(w.Namespace), strings.TrimSpace(w.Kind), - normalizeResourceName(strings.TrimSpace(w.Name)), + normalizeResourceName(w), strings.TrimSpace(cluster), } if account := strings.TrimSpace(w.Account); account != "" { @@ -123,7 +129,7 @@ func Fingerprint(inv providers.Investigation) string { } if len(inv.Changes) > 0 { w := inv.Changes[0].Workload - b.WriteString(" " + w.Namespace + " " + normalizeResourceName(w.Name)) + b.WriteString(" " + w.Namespace + " " + normalizeResourceName(w)) } return strings.TrimSpace(b.String()) } @@ -174,7 +180,7 @@ func DupFingerprint(inv providers.Investigation) string { // incident on a different pod/node keys alike (CORE-681). The TriggerKey is // already a host-invariant per-class key for alert sources (see IncidentKey). res := inv.Resource - res.Name = normalizeResourceName(res.Name) + res.Name = normalizeResourceName(res) ref := strings.ToLower(res.Ref()) // The AWS account qualifies the ref, so one instance name in two accounts does not // curate as one incident. Appended only when present, so a Kubernetes ref — and diff --git a/internal/curator/fingerprint_test.go b/internal/curator/fingerprint_test.go index 797a7b3e..5885bd0c 100644 --- a/internal/curator/fingerprint_test.go +++ b/internal/curator/fingerprint_test.go @@ -198,7 +198,7 @@ func TestNormalizeResourceNameAlias(t *testing.T) { "": "", } for in, want := range cases { - if got := normalizeResourceName(in); got != want { + if got := normalizeResourceName(providers.Workload{Name: in}); got != want { t.Errorf("normalizeResourceName(%q) = %q, want %q", in, got, want) } } diff --git a/internal/curator/replica_identity_test.go b/internal/curator/replica_identity_test.go new file mode 100644 index 00000000..5b048700 --- /dev/null +++ b/internal/curator/replica_identity_test.go @@ -0,0 +1,83 @@ +// SPDX-License-Identifier: Apache-2.0 + +package curator + +import ( + "strings" + "testing" + + "github.com/Smana/runlore/internal/providers" +) + +// TestIncidentKeyFoldsStatefulSetReplicas is #513 on the WRITE side: the same fault +// on two replicas of one StatefulSet is one incident identity, exactly as it already +// is on two pods of one Deployment — so the recurrence chain, suppression and the +// dedup key stop restarting per replica. The Kind is what licenses the fold: a +// kind-less numbered name is a name, and two databases must not key alike. +func TestIncidentKeyFoldsStatefulSetReplicas(t *testing.T) { + pod := func(name string) providers.Workload { + return providers.Workload{Kind: "Pod", Namespace: "observability", Name: name} + } + a := IncidentKey("VMAgentHighMemory", pod("vmagent-vmagent-0"), "prod") + b := IncidentKey("VMAgentHighMemory", pod("vmagent-vmagent-1"), "prod") + if a == "" || a != b { + t.Fatalf("two replicas of one StatefulSet must key alike: %q vs %q", a, b) + } + if !strings.Contains(a, "|vmagent-vmagent|") { + t.Errorf("key must carry the StatefulSet family, got %q", a) + } + db := func(name string) providers.Workload { + return providers.Workload{Namespace: "observability", Name: name, Account: "111111111111"} + } + if IncidentKey("RDSCPUHigh", db("aurora-serverless-postgres-old-1"), "prod") == + IncidentKey("RDSCPUHigh", db("aurora-serverless-postgres-old-2"), "prod") { + t.Fatal("two databases that differ by a trailing digit must not key alike") + } +} + +// TestDupFingerprintFoldsStatefulSetReplicas: the curator's dedup fingerprint reads +// the same identity, so a finding on replica 1 coalesces with the entry filed from +// replica 0 instead of filing a second KB entry saying the same thing. +func TestDupFingerprintFoldsStatefulSetReplicas(t *testing.T) { + base := providers.Investigation{ + Resource: providers.Workload{Kind: "Pod", Namespace: "observability", Name: "vmagent-vmagent-0"}, + RootCauses: []providers.Hypothesis{{Summary: "scrape cardinality grew past the memory limit"}}, + } + other := base + other.Resource = providers.Workload{Kind: "Pod", Namespace: "observability", Name: "vmagent-vmagent-1"} + if fa, fb := DupFingerprint(base), DupFingerprint(other); fa == "" || fa != fb { + t.Fatalf("same cause on two replicas must fingerprint alike: %q vs %q", fa, fb) + } +} + +// TestDraftKBEntryFoldsStatefulSetReplica is #513 at the stored resource: an entry +// written from a StatefulSet pod is filed under the StatefulSet, not the replica, so +// the human-facing ref stops naming one of N identical pods and the READ side has +// one family to match. A kind-less numbered resource is written as named. +func TestDraftKBEntryFoldsStatefulSetReplica(t *testing.T) { + cases := []struct { + name string + resource providers.Workload + want string + }{ + {"a StatefulSet pod is filed under its StatefulSet", + providers.Workload{Kind: "Pod", Namespace: "observability", Name: "vmagent-vmagent-1"}, + "observability/vmagent-vmagent"}, + {"a kind-less numbered resource is its own name", + providers.Workload{Namespace: "observability", Name: "aurora-serverless-postgres-old-1"}, + "observability/aurora-serverless-postgres-old-1"}, + } + for _, tt := range cases { + t.Run(tt.name, func(t *testing.T) { + inv := providers.Investigation{ + Title: "VMAgentHighMemory", + Confidence: 0.9, + Resource: tt.resource, + RootCauses: []providers.Hypothesis{{Summary: "scrape cardinality grew", Evidence: []string{"e"}}}, + } + if got := draftKBEntry(inv).Resource; got != tt.want { + t.Fatalf("KBEntry.Resource = %q, want %q", got, tt.want) + } + }) + } +} diff --git a/internal/investigate/recall.go b/internal/investigate/recall.go index 9df8d50a..320b3b38 100644 --- a/internal/investigate/recall.go +++ b/internal/investigate/recall.go @@ -706,8 +706,10 @@ func refsAgree(reqW providers.Workload, b string) bool { return true } // The request's name half is not re-cut out of the ref: reqW.ResourceID already - // holds it, resolved and qualified, and a namespace never contains a "/". - return reqW.ResourceID().Agrees(providers.ParseResourceID(bname)) + // holds it, resolved and qualified, and a namespace never contains a "/". The + // entry's name is read the way the request's Kind licenses (#513: a pod-scoped + // request also agrees with an entry filed under a sibling StatefulSet replica). + return reqW.AgreesWithEntryName(bname) } func clampF(v, lo, hi float64) float64 { diff --git a/internal/investigate/replica_identity_test.go b/internal/investigate/replica_identity_test.go new file mode 100644 index 00000000..b826f8e5 --- /dev/null +++ b/internal/investigate/replica_identity_test.go @@ -0,0 +1,57 @@ +// SPDX-License-Identifier: Apache-2.0 + +package investigate + +import ( + "testing" + + "github.com/Smana/runlore/internal/providers" +) + +// TestResourceAgreesAcrossStatefulSetReplicas is #513 on the READ side. Live: 56 +// recall evaluations over 7 days, 0 accepted, 38 of them no_resource_match — a KB +// holding observability/vmagent-vmagent-0 and -1 as two entries that an incident on +// -2 could match neither of. A pod-scoped request folds its replica ordinal, and an +// entry is read both as written and folded once, so an entry filed under a sibling +// replica (legacy) and one filed under the StatefulSet (new) both agree. +// +// The Kind is the evidence: a kind-less request keeps its ordinal, so two databases +// that differ only by a trailing digit stay two resources. +func TestResourceAgreesAcrossStatefulSetReplicas(t *testing.T) { + pod := func(name string) providers.Workload { + return providers.Workload{Kind: "Pod", Namespace: "observability", Name: name} + } + cases := []struct { + name string + reqW providers.Workload + entry string + requireWL bool + want matchStrength + }{ + {"a replica matches an entry filed under a sibling replica", + pod("vmagent-vmagent-2"), "observability/vmagent-vmagent-0", false, matchExact}, + {"a replica matches an entry filed under the StatefulSet", + pod("vmagent-vmagent-2"), "observability/vmagent-vmagent", false, matchExact}, + {"strict mode agrees too — it is one workload", + pod("vmagent-vmagent-2"), "observability/vmagent-vmagent-0", true, matchExact}, + {"a StatefulSet whose own name ends in a digit is not folded past itself", + pod("shard-1-0"), "observability/shard-1", false, matchExact}, + {"the same replica in another namespace is another workload", + pod("vmagent-vmagent-2"), "other/vmagent-vmagent-0", false, matchNone}, + {"a replica does not agree with a different StatefulSet", + pod("vmagent-vmagent-2"), "observability/vmalert-vmalert-0", false, matchNone}, + {"without a Kind the ordinal is the name: two databases stay two", + providers.Workload{Namespace: "observability", Name: "aurora-serverless-postgres-old-1"}, + "observability/aurora-serverless-postgres-old-2", false, matchNone}, + {"without a Kind a numbered name matches only itself", + providers.Workload{Namespace: "observability", Name: "vmagent-vmagent-2"}, + "observability/vmagent-vmagent-0", false, matchNone}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + if got := resourceAgrees(c.reqW, c.entry, c.requireWL); got != c.want { + t.Errorf("resourceAgrees(%+v, %q, %v) = %v, want %v", c.reqW, c.entry, c.requireWL, got, c.want) + } + }) + } +} diff --git a/internal/providers/pod_name_test.go b/internal/providers/pod_name_test.go new file mode 100644 index 00000000..b57f3c8d --- /dev/null +++ b/internal/providers/pod_name_test.go @@ -0,0 +1,45 @@ +// SPDX-License-Identifier: Apache-2.0 + +package providers_test + +import ( + "testing" + + "github.com/Smana/runlore/internal/providers" +) + +// TestNormalizePodNameFoldsStatefulSetOrdinal pins the rule NormalizeWorkloadName +// deliberately does NOT apply (#513): a StatefulSet pod is -, and for +// a name KNOWN to be a Pod's that ordinal is a per-replica suffix like a pod hash. +// Only the Kind can say so — the same shape on a kind-less name is the name +// (aurora-serverless-postgres-old-1 and -2 are two databases), which is why the +// kind-less TestNormalizeWorkloadName keeps "vmagent-vmagent-0" and this does not. +// +// Exactly ONE ordinal is folded, not a fixed point: a StatefulSet named sts-0 has +// pods sts-0-N, and folding to a fixed point would erase the StatefulSet's own name. +// The function is therefore NOT idempotent on such a family, and the recall gate +// compares an entry both as written and folded once rather than folding blindly. +func TestNormalizePodNameFoldsStatefulSetOrdinal(t *testing.T) { + cases := map[string]string{ + "vmagent-vmagent-0": "vmagent-vmagent", + "vmagent-vmagent-1": "vmagent-vmagent", + "vmagent-vmagent-12": "vmagent-vmagent", + "vmagent-vmagent": "vmagent-vmagent", // the StatefulSet itself + "sts-0-3": "sts-0", // one ordinal: the family keeps its own digit + + // Everything NormalizeWorkloadName already folds still folds, first. + "web-7d9c8b6f5-abcde": "web", + "harbor-registry-59598dbd57-ltkzw": "harbor-registry", + "github-teams-sync-aqemia-29787720-3-mdft8": "github-teams-sync-aqemia", // Indexed CronJob pod + + // A strip must never leave debris — the same guard as every other rule. + "x-0": "x-0", + "-0": "-0", + "": "", + } + for in, want := range cases { + if got := providers.NormalizePodName(in); got != want { + t.Errorf("NormalizePodName(%q) = %q, want %q", in, got, want) + } + } +} diff --git a/internal/providers/providers.go b/internal/providers/providers.go index 0e1c38c9..0057c4d9 100644 --- a/internal/providers/providers.go +++ b/internal/providers/providers.go @@ -246,7 +246,9 @@ const minFamilyName = 2 // pod of an Indexed one) all collapse to . The rules run to a FIXED POINT, so // a CronJob's pod sheds both its hash and its run stamp. Names without // such a suffix are returned unchanged, so real trailing words (e.g. "redis-cache") -// and short numeric tails (e.g. "vmagent-vmagent-0") are preserved. It is idempotent. +// and short numeric tails (e.g. "vmagent-vmagent-0") are preserved — a StatefulSet +// replica ordinal is folded only where the Kind is known to be a Pod's (see +// NormalizePodName and Workload.IdentityName). It is idempotent. // // The CronJob case was the expensive omission. A CronJob that fails once emits a // Job named for THAT run, and the run number reaches four separate consumers — the @@ -277,6 +279,56 @@ func NormalizeWorkloadName(name string) string { } } +// NormalizePodName is NormalizeWorkloadName for a name KNOWN to be a Pod's: after +// the per-instance rules it also folds ONE StatefulSet replica ordinal +// (-) into the StatefulSet family, so vmagent-vmagent-0, -1 +// and -2 are one workload the way three pods of a Deployment already are (#513). +// +// It is a separate function, not another rule in NormalizeWorkloadName, because +// only the Kind licenses the fold: on a name of unknown kind a short numeric tail is +// ordinary naming — aurora-serverless-postgres-old-1 and -2 are two databases, and +// ip-10-20-0-144 is a node — and folding it would fuse distinct resources in the +// dedup key and the suppression chain. Callers that hold a Workload should read +// Workload.IdentityName, which consults the Kind; this is the string-level rule. +// +// ONE ordinal, not a fixed point: a StatefulSet named sts-0 has pods sts-0-N, and +// folding to a fixed point would erase the StatefulSet's own name. So this is NOT +// idempotent on such a family — Workload.AgreesWithEntryName reads a stored entry +// both as written and folded once for exactly that reason. +func NormalizePodName(name string) string { + return stripReplicaOrdinal(NormalizeWorkloadName(name)) +} + +// stripReplicaOrdinal removes ONE trailing - segment, or returns its input +// unchanged. It runs after the per-instance rules, so the tail it sees on a real pod +// name is a StatefulSet ordinal: a hash or run stamp has already been folded. +func stripReplicaOrdinal(name string) string { + i := strings.LastIndexByte(name, '-') + if i < 0 || !isOrdinal(name[i+1:]) { + return name + } + // The same debris guard as stripInstanceSuffix: a strip that leaves too little to + // be a name is not a family. + family := strings.TrimRight(name[:i], "-") + if len(family) < minFamilyName { + return name + } + return family +} + +// isOrdinal reports whether s is a non-empty run of ASCII digits. +func isOrdinal(s string) bool { + if s == "" { + return false + } + for i := range len(s) { + if s[i] < '0' || s[i] > '9' { + return false + } + } + return true +} + // stripInstanceSuffix removes at most ONE trailing per-instance suffix, or returns // its input unchanged. NormalizeWorkloadName drives it to a fixed point. func stripInstanceSuffix(name string) string { diff --git a/internal/providers/replica_identity_test.go b/internal/providers/replica_identity_test.go new file mode 100644 index 00000000..cd65f9bc --- /dev/null +++ b/internal/providers/replica_identity_test.go @@ -0,0 +1,39 @@ +// SPDX-License-Identifier: Apache-2.0 + +package providers + +import "testing" + +// TestWorkloadIdentityFoldsReplicaOrdinalForPodsOnly pins #513 at the identity seam +// every key and comparison reads through: a StatefulSet replica ordinal is a +// per-instance suffix, but ONLY the Kind can say so, because the same shape on a +// cloud resource or a controller is the name. Ingestion still keeps the raw pod name +// (normalization is a comparison-time concern); this is where the Kind is consulted. +func TestWorkloadIdentityFoldsReplicaOrdinalForPodsOnly(t *testing.T) { + cases := []struct { + name string + w Workload + want string + }{ + {"a StatefulSet pod folds to its StatefulSet", + Workload{Kind: "Pod", Namespace: "observability", Name: "vmagent-vmagent-2"}, "vmagent-vmagent"}, + {"a Deployment pod still folds to its Deployment", + Workload{Kind: "Pod", Namespace: "tooling", Name: "harbor-registry-59598dbd57-ltkzw"}, "harbor-registry"}, + {"a kind-less name keeps its numeric tail: two databases, not two replicas", + Workload{Namespace: "observability", Name: "aurora-serverless-postgres-old-1"}, "aurora-serverless-postgres-old-1"}, + {"a controller whose own name ends in a digit is its own name", + Workload{Kind: "StatefulSet", Namespace: "apps", Name: "shard-1"}, "shard-1"}, + {"an ARN-spelled cloud resource is unaffected", + Workload{Name: "arn:aws:rds:us-east-1:111111111111:db:datagrok-1"}, "datagrok-1"}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + if got := c.w.IdentityName(); got != c.want { + t.Errorf("IdentityName() = %q, want %q", got, c.want) + } + if got := c.w.ResourceID().Name; got != c.want { + t.Errorf("ResourceID().Name = %q, want %q", got, c.want) + } + }) + } +} diff --git a/internal/providers/resourceid.go b/internal/providers/resourceid.go index b7abeffa..5c80bd40 100644 --- a/internal/providers/resourceid.go +++ b/internal/providers/resourceid.go @@ -224,6 +224,9 @@ func labelValue(labels map[string]string, keys []string) string { // filed under a full ARN in another account. func (w Workload) ResourceID() ResourceID { id := ParseResourceID(w.Name) + if w.foldsReplicaOrdinal() { + id.Name = stripReplicaOrdinal(id.Name) + } return ResourceID{ Name: id.Name, Region: cmp.Or(id.Region, w.Region), @@ -231,6 +234,48 @@ func (w Workload) ResourceID() ResourceID { } } +// IdentityName is the name half of the identity w is keyed and compared by: the +// bare resource identifier (an ARN loses its scaffolding) with its per-instance +// suffix folded — pod hash, CronJob run stamp and, when the Kind says the name is a +// Pod's, ONE StatefulSet replica ordinal (#513). It is ResourceID().Name, spelled +// as its own method so the key builders (curator.IncidentKey, DupFingerprint, the +// drafted entry's resource) and the comparison (ResourceID.Agrees) read one rule. +// +// This is the seam where the Kind is consulted. NormalizeResourceName and +// NormalizeWorkloadName take a bare string and cannot be: on a kind-less name a +// trailing ordinal is the name, and they keep it. +func (w Workload) IdentityName() string { + return w.ResourceID().Name +} + +// foldsReplicaOrdinal reports whether w's Name is a Pod's, so that a trailing +// - is a StatefulSet replica ordinal rather than part of the name. Only the +// Kind can say so: a Kubernetes controller, a node or a cloud resource whose name +// ends in a digit is named that way. +func (w Workload) foldsReplicaOrdinal() bool { + return w.Kind == "Pod" +} + +// AgreesWithEntryName reports whether w names the same resource as a catalog +// entry's stored resource name. It is ResourceID.Agrees with one tolerance: a +// pod-scoped workload has folded its StatefulSet replica ordinal, but an entry +// filed before #513 still names the sibling replica it was written from, so the +// entry is read both as written and folded once. Once, not to a fixed point, so an +// entry filed under a StatefulSet whose own name ends in a digit still matches; and +// only for a workload whose Kind licenses the fold — a kind-less request's numbered +// name is the name, and it matches only itself. +func (w Workload) AgreesWithEntryName(name string) bool { + req, entry := w.ResourceID(), ParseResourceID(name) + if req.Agrees(entry) { + return true + } + if !w.foldsReplicaOrdinal() { + return false + } + entry.Name = stripReplicaOrdinal(entry.Name) + return req.Agrees(entry) +} + // ARNResourceName reduces an AWS ARN to the resource identifier that the matching // CloudWatch dimension carries, and returns any other value byte-for-byte. It is the // name half of the ingestion-side canonicalisation, so only one spelling of a