Skip to content

Commit 2b676aa

Browse files
nadaverellnadaverell
authored
Improve pod failure root cause issues (#917)
1 parent 616cf8f commit 2b676aa

10 files changed

Lines changed: 711 additions & 49 deletions

File tree

‎internal/issues/grouping.go‎

Lines changed: 21 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -125,11 +125,10 @@ func foldGroup(members []Issue) Issue {
125125
LastSeen: rep.LastSeen,
126126
}
127127
// A parsed diagnosis (cause/action/remediation) describes ONE resource's
128-
// failure. Carry it onto the grouped row only when every member that has
129-
// one agrees: a single-member group (a GitOps Application is its own
130-
// subject) always carries it, but a workload rollup whose members fail for
131-
// different reasons (OOMKilled vs ImagePullBackOff) must not present one
132-
// member's cause as the whole group's.
128+
// failure. Carry it onto the grouped row only when it is true for the
129+
// entire group: a single-member group carries its own diagnosis, but a
130+
// workload rollup omits diagnosis unless every folded member has the same
131+
// tuple.
133132
if dg, ok := agreedDiagnosis(members); ok {
134133
g.Cause = dg.Cause
135134
g.Action = dg.Action
@@ -195,21 +194,21 @@ func foldGroup(members []Issue) Issue {
195194
}
196195

197196
// agreedDiagnosis returns the parsed diagnosis shared by a group's members, or
198-
// ok=false when members carry conflicting diagnoses. Members with no parsed
199-
// diagnosis (the common case for workload problems today) are ignored — they
200-
// don't count as disagreement. The full (cause, action, remediation) tuple
201-
// must match across every member that has one, so a mixed-cause rollup omits
202-
// the diagnosis rather than misattributing one member's fix to the group.
197+
// ok=false when members carry conflicting or incomplete diagnoses. The full
198+
// (cause, action, remediation) tuple must match across every member in a
199+
// multi-resource group, so a mixed rollup omits diagnosis rather than
200+
// misattributing one member's fix to the group.
203201
func agreedDiagnosis(members []Issue) (Issue, bool) {
204202
var picked Issue
205-
have := false
203+
if len(members) == 0 {
204+
return Issue{}, false
205+
}
206206
for _, m := range members {
207-
if m.Cause == "" && m.Action == "" && m.RemediationKind == "" && m.RemediationTarget == "" &&
208-
m.OperationRetryCount == 0 && !m.Stuck {
209-
continue
207+
if !hasDiagnosis(m) {
208+
return Issue{}, false
210209
}
211-
if !have {
212-
picked, have = m, true
210+
if !hasDiagnosis(picked) {
211+
picked = m
213212
continue
214213
}
215214
if m.Cause != picked.Cause || m.Action != picked.Action ||
@@ -218,7 +217,12 @@ func agreedDiagnosis(members []Issue) (Issue, bool) {
218217
return Issue{}, false
219218
}
220219
}
221-
return picked, have
220+
return picked, true
221+
}
222+
223+
func hasDiagnosis(i Issue) bool {
224+
return i.Cause != "" || i.Action != "" || i.RemediationKind != "" || i.RemediationTarget != "" ||
225+
i.OperationRetryCount != 0 || i.Stuck
222226
}
223227

224228
// betterRepresentative reports whether cand should replace cur as a group's

‎internal/issues/grouping_test.go‎

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -85,12 +85,9 @@ func TestGroupIssues_CarriesAgreedDiagnosis(t *testing.T) {
8585
}
8686
}
8787

88-
// TestGroupIssues_CarriesLoneDiagnosis pins the "no opinion ≠ disagreement"
89-
// rule: when one member of a group has a parsed diagnosis and another has none,
90-
// the lone diagnosis still carries (a blank member is not treated as a
91-
// conflict). A naive "omit whenever any member lacks a diagnosis" would strip
92-
// the cause off every mixed workload+GitOps rollup.
93-
func TestGroupIssues_CarriesLoneDiagnosis(t *testing.T) {
88+
// TestGroupIssues_OmitsLoneDiagnosis pins that a workload rollup only carries
89+
// diagnosis when the diagnosis applies to every folded member.
90+
func TestGroupIssues_OmitsLoneDiagnosis(t *testing.T) {
9491
dep := Ref{Group: "apps", Kind: "Deployment", Namespace: "ns", Name: "web"}
9592
t0 := time.Unix(1000, 0)
9693
a := flatPod("web-a", "CrashLoopBackOff", SeverityCritical, dep, t0, t0)
@@ -100,8 +97,8 @@ func TestGroupIssues_CarriesLoneDiagnosis(t *testing.T) {
10097
if len(got) != 1 {
10198
t.Fatalf("want 1 grouped row, got %d", len(got))
10299
}
103-
if got[0].Cause != a.Cause {
104-
t.Errorf("lone diagnosis should carry; got cause %q, want %q", got[0].Cause, a.Cause)
100+
if got[0].Cause != "" {
101+
t.Errorf("lone member diagnosis must not carry to grouped row, got cause %q", got[0].Cause)
105102
}
106103
}
107104

‎internal/issues/issues_test.go‎

Lines changed: 68 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -178,6 +178,66 @@ func TestCompose_GroupedKindFilterMatchesSubject(t *testing.T) {
178178
}
179179
}
180180

181+
func TestCompose_GroupedDiagnosisRequiresEveryMemberToAgree(t *testing.T) {
182+
p := &fakeProvider{
183+
problems: []k8s.Detection{
184+
{
185+
Kind: "Pod", Namespace: "ns", Name: "web-a", Severity: "critical", Reason: "CrashLoopBackOff",
186+
OwnerKind: "Deployment", OwnerName: "web",
187+
Cause: "command not found", Action: "fix the container command",
188+
},
189+
{Kind: "Pod", Namespace: "ns", Name: "web-b", Severity: "critical", Reason: "CrashLoopBackOff", OwnerKind: "Deployment", OwnerName: "web"},
190+
},
191+
}
192+
out := Compose(p, Filters{Grouped: true})
193+
if len(out) != 1 {
194+
t.Fatalf("got %d issues, want 1: %+v", len(out), out)
195+
}
196+
if out[0].Cause != "" || out[0].Action != "" {
197+
t.Fatalf("mixed parsed/unparsed rollup must omit diagnosis, got cause=%q action=%q", out[0].Cause, out[0].Action)
198+
}
199+
}
200+
201+
func TestCompose_GroupedDiagnosisKeepsIdenticalMemberDiagnosis(t *testing.T) {
202+
p := &fakeProvider{
203+
problems: []k8s.Detection{
204+
{
205+
Kind: "Pod", Namespace: "ns", Name: "web-a", Severity: "critical", Reason: "CrashLoopBackOff",
206+
OwnerKind: "Deployment", OwnerName: "web",
207+
Cause: "command not found", Action: "fix the container command",
208+
},
209+
{
210+
Kind: "Pod", Namespace: "ns", Name: "web-b", Severity: "critical", Reason: "CrashLoopBackOff",
211+
OwnerKind: "Deployment", OwnerName: "web",
212+
Cause: "command not found", Action: "fix the container command",
213+
},
214+
},
215+
}
216+
out := Compose(p, Filters{Grouped: true})
217+
if len(out) != 1 {
218+
t.Fatalf("got %d issues, want 1: %+v", len(out), out)
219+
}
220+
if out[0].Cause != "command not found" || out[0].Action != "fix the container command" {
221+
t.Fatalf("identical member diagnosis should be promoted, got cause=%q action=%q", out[0].Cause, out[0].Action)
222+
}
223+
}
224+
225+
func TestCompose_GroupedSingleIssueKeepsDiagnosis(t *testing.T) {
226+
p := &fakeProvider{
227+
problems: []k8s.Detection{{
228+
Kind: "Pod", Namespace: "ns", Name: "standalone", Severity: "critical", Reason: "CrashLoopBackOff",
229+
Cause: "command not found", Action: "fix the container command",
230+
}},
231+
}
232+
out := Compose(p, Filters{Grouped: true})
233+
if len(out) != 1 {
234+
t.Fatalf("got %d issues, want 1: %+v", len(out), out)
235+
}
236+
if out[0].Cause == "" || out[0].Action == "" {
237+
t.Fatalf("single-resource issue should keep diagnosis, got %+v", out[0])
238+
}
239+
}
240+
181241
func TestCompose_DropsInfoSeverityFromQueue(t *testing.T) {
182242
// info-severity problems are inert/posture (deprecated-RBAC residue,
183243
// singleton-StatefulSet headless-DNS trivia) — excluded from the live issue
@@ -342,7 +402,11 @@ func TestCompose_PVCPendingDedupesOverMissingStorageClass(t *testing.T) {
342402
// One incident, one row: the missing-ref row names the cause and wins.
343403
p := &fakeProvider{
344404
problems: []k8s.Detection{
345-
{Kind: "PersistentVolumeClaim", Namespace: "ns", Name: "stuck-pvc", Severity: "high", Reason: "Pending", IssueTiming: "started_at_resource_creation", IssueTimingBasis: "phase"},
405+
{
406+
Kind: "PersistentVolumeClaim", Namespace: "ns", Name: "stuck-pvc", Severity: "high", Reason: "Pending",
407+
Cause: "Storage provisioner failed to create a volume.", Action: "Check the CSI controller logs.",
408+
IssueTiming: "started_at_resource_creation", IssueTimingBasis: "phase",
409+
},
346410
},
347411
missingRefs: []k8s.Detection{
348412
{Kind: "PersistentVolumeClaim", Namespace: "ns", Name: "stuck-pvc", Severity: "critical", Reason: "Missing StorageClass", Fingerprint: "Missing StorageClass|abc", IssueTiming: "started_at_resource_creation", IssueTimingBasis: "phase"},
@@ -361,6 +425,9 @@ func TestCompose_PVCPendingDedupesOverMissingStorageClass(t *testing.T) {
361425
if pvcRows[0].Reason != "Missing StorageClass" {
362426
t.Errorf("the cause-naming missing-ref row must win, got reason %q", pvcRows[0].Reason)
363427
}
428+
if pvcRows[0].Source != SourceMissingRef {
429+
t.Errorf("missing-ref row must win over enriched phase row, got source %q", pvcRows[0].Source)
430+
}
364431
if pvcRows[0].IssueTiming != "started_at_resource_creation" || pvcRows[0].IssueTimingBasis != "phase" {
365432
t.Errorf("surviving row must keep at-creation/phase timing, got (%q, %q)", pvcRows[0].IssueTiming, pvcRows[0].IssueTimingBasis)
366433
}

0 commit comments

Comments
 (0)