ci: analyze-disruption: root-cause attribution rules, onset/suspect-PR step, existing-bug discovery - #655
ci: analyze-disruption: root-cause attribution rules, onset/suspect-PR step, existing-bug discovery#655mkowalski wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mkowalski The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
WalkthroughThe disruption analysis skill adds root-cause attribution rules, disruption-onset analysis, known-issue checks, and expanded single-run and multi-run report requirements. ChangesDisruption analysis guidance
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 9 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (9 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/ci/skills/analyze-disruption/SKILL.md`:
- Around line 325-333: Revise the guidance in the provider-corroboration rule so
provider health is evidence rather than an ownership verdict. Require a
time-correlated provider failure and trace dependencies to the first independent
fault before assigning ownership; likewise, do not conclude the victim is
responsible solely because the provider appears healthy.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0a773c56-0b45-4439-9e8d-e23376a17249
📒 Files selected for processing (2)
plugins/ci/.claude-plugin/plugin.jsonplugins/ci/skills/analyze-disruption/SKILL.md
d92d4a9 to
f497b94
Compare
f497b94 to
06a94d3
Compare
…R step, existing-bug discovery Fold the lessons from recent Component Readiness triage duty into the disruption analysis skill: - Step 3.5 (new): attribution rules — victim-vs-owner with upstream corroboration (dependency-error crash-loopers are usually victims, but check the provider's health before reassigning; RBAC/config faults masquerade as dependency errors), clusteroperators.json as the end-state oracle (pod existence proves nothing about etcd/apiserver serving), extract ClusterVersion/ClusterOperator condition *messages* (they name the culprit controller), read *_previous.log for any container restarted in the window, and count condition transitions (sync-loop flap vs slow rollout). - Step 5.3 (new): onset determination for new/worsening disruption — clean-vs-disrupted payload delta (incl. initial/target skew on upgrade jobs), fetch-new-prs-in-payload vetting, and the revert-PR shortcut (fresh reverts citing TRT/OCPBUGS keys yield trigger + ticket in one query); bans unanchored 'unidentified payload change' claims. - Step 5.4 (new): existing-bug discovery — fetch-related-triages with verify-before-adopt, component-scoped JIRA listing (developer vocabulary defeats keyword search), and the known recurring disruption families to rule out first. - Report templates: Root Cause Hypothesis sections now require the full causal chain, the Step 3.5 corroboration, and existing-ticket references. Assisted-By: Claude Fable 5
06a94d3 to
1e976b1
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
plugins/ci/skills/analyze-disruption/SKILL.md (4)
732-742: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAlign the OVS threshold with the signal definition.
The diagnostic reference uses
>1000ms, but the Jira query triggers at>500ms. Use one threshold or label>500msas an early-warning threshold. Otherwise the lookup covers a different signal class.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/ci/skills/analyze-disruption/SKILL.md` around lines 732 - 742, Align the OVS Jira lookup in the “If OVS stalls were detected” section with the diagnostic signal definition by changing max_poll_interval_ms > 500 to the defined >1000 threshold, or explicitly label >500ms as an early-warning threshold if that behavior is intentional.
537-551: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winTreat clean-run comparisons as evidence, not proof.
A signal present in both runs can still contribute if its rate, duration, magnitude, or timing differs. A signal unique to disrupted runs can be incidental. Compare quantitative values before excluding or prioritizing a signal.
Proposed wording
- are not the cause of disruption + are not sufficient discriminators; compare timing, rate, duration, and magnitude - are the strongest root cause candidates + are candidate signals that require causal corroboration🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/ci/skills/analyze-disruption/SKILL.md` around lines 537 - 551, Revise the “Signals present in both” and “Signals unique to disrupted runs” guidance to treat clean-run comparisons as evidence rather than definitive proof. Require comparing signal rate, duration, magnitude, and timing before excluding shared signals or prioritizing disrupted-only signals, and update the E2E noise statement to preserve this qualification.
50-62: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winValidate the URL list before type detection.
Reject empty input before reading the first URL. Reject unknown hosts and mixed Grafana/Prow input before parsing. The current
any other URLbranch is broader than the two supported Prow formats.Proposed wording
-2. **Detect input type** based on the first URL provided: +2. **Validate input** before type detection: + - Require at least one URL. + - Accept one Grafana URL or one or more supported Prow URLs. + - Reject unknown hosts and mixed input types. +3. **Detect input type** after validation:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/ci/skills/analyze-disruption/SKILL.md` around lines 50 - 62, Update the input-validation flow before the “Detect input type” step: require at least one URL, classify each URL as either the supported Grafana host/path or an explicitly supported Prow format, reject unknown hosts, and reject inputs mixing Grafana and Prow URLs. Only proceed to Grafana or Prow processing after the entire URL list passes validation.
100-112: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winEmit
recommended: falsefor unselected rows. When--auto-selectreturns no indices,print_jsonomitsrecommended, although the documented schema requires this boolean. Keeproleoptional as documented.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/ci/skills/analyze-disruption/SKILL.md` around lines 100 - 112, Update print_json in find_disruption_runs.py so every unselected row explicitly emits recommended: false, including when --auto-select produces no indices; preserve the existing recommended: true behavior for selected rows and keep role optional.
♻️ Duplicate comments (1)
plugins/ci/skills/analyze-disruption/SKILL.md (1)
377-385: 🎯 Functional Correctness | 🟠 MajorDo not use provider health as an ownership verdict.
An unhealthy provider supports an upstream hypothesis. It does not establish ownership. A healthy provider also does not prove that the victim owns the fault. Require time correlation and trace the first independent fault.
Proposed wording
- provider unhealthy → the disruption belongs upstream + provider unhealthy → supports an upstream hypothesis; trace to the first independent fault - provider healthy → the fault is inside the "victim" after all + provider healthy → does not establish ownership; check RBAC, client configuration, and network reachability🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/ci/skills/analyze-disruption/SKILL.md` around lines 377 - 385, Revise the guidance in the first numbered principle so provider health is treated only as evidence, not an ownership verdict. Require temporal correlation and tracing the first independent fault before assigning responsibility; preserve the victim/upstream examples while avoiding claims that an unhealthy provider proves upstream ownership or a healthy provider proves victim ownership.
🧹 Nitpick comments (1)
plugins/ci/skills/analyze-disruption/SKILL.md (1)
462-500: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate the duplicated disruption-triage workflow.
- HIGH overlap:
bulk-triage-regressions/SKILL.mdalready documents the onset, revert-PR, related-triage, component-scoped JIRA, and recurring-family checks. Keep one canonical workflow and reference it here.- LOW overlap: Open PR
#617is below the 60% threshold, and PR#686changes only helper scripts.fetch-related-triagesrequires aregression_id, but this skill accepts only Prow or Grafana URLs. Add a step to obtain that ID or remove this invocation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/ci/skills/analyze-disruption/SKILL.md` around lines 462 - 500, Consolidate the onset, revert-PR, related-triage, component-scoped JIRA, and recurring-family guidance in sections 5.3–5.4 by removing the duplicated workflow and referencing the canonical workflow in bulk-triage-regressions/SKILL.md. Preserve any disruption-specific context not covered there. Also resolve the fetch-related-triages call by adding a supported step to obtain regression_id from the available Prow or Grafana URL, or remove that invocation if no reliable conversion exists.Sources: Coding guidelines, Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/ci/skills/analyze-disruption/SKILL.md`:
- Around line 400-402: Update the “Condition churn ≠ slow rollout” guidance to
avoid treating rapid transitions as definitive proof of a sync-loop or
permafail. Require confirmation from the condition message, transition count,
and controller logs before assigning either classification, while preserving the
distinction from transient slow rollout behavior.
- Around line 467-470: Update the job-history guidance to compare the last clean
run immediately preceding the first disrupted run, rather than the newest clean
and oldest disrupted runs overall. When intervening runs exist, inspect them to
identify the onset boundary, then compare the relevant payloads for that
adjacent clean/disrupted pair.
- Around line 471-490: Expand the instructions for fetch-new-prs-in-payload and
fetch-related-triages to document each skill’s invocation, required inputs,
output fields, and failure handling. For upgrade jobs, explicitly identify
whether release-images-latest or release-images-initial provides the PR diff,
and define how payload skew is detected and handled.
---
Outside diff comments:
In `@plugins/ci/skills/analyze-disruption/SKILL.md`:
- Around line 732-742: Align the OVS Jira lookup in the “If OVS stalls were
detected” section with the diagnostic signal definition by changing
max_poll_interval_ms > 500 to the defined >1000 threshold, or explicitly label
>500ms as an early-warning threshold if that behavior is intentional.
- Around line 537-551: Revise the “Signals present in both” and “Signals unique
to disrupted runs” guidance to treat clean-run comparisons as evidence rather
than definitive proof. Require comparing signal rate, duration, magnitude, and
timing before excluding shared signals or prioritizing disrupted-only signals,
and update the E2E noise statement to preserve this qualification.
- Around line 50-62: Update the input-validation flow before the “Detect input
type” step: require at least one URL, classify each URL as either the supported
Grafana host/path or an explicitly supported Prow format, reject unknown hosts,
and reject inputs mixing Grafana and Prow URLs. Only proceed to Grafana or Prow
processing after the entire URL list passes validation.
- Around line 100-112: Update print_json in find_disruption_runs.py so every
unselected row explicitly emits recommended: false, including when --auto-select
produces no indices; preserve the existing recommended: true behavior for
selected rows and keep role optional.
---
Duplicate comments:
In `@plugins/ci/skills/analyze-disruption/SKILL.md`:
- Around line 377-385: Revise the guidance in the first numbered principle so
provider health is treated only as evidence, not an ownership verdict. Require
temporal correlation and tracing the first independent fault before assigning
responsibility; preserve the victim/upstream examples while avoiding claims that
an unhealthy provider proves upstream ownership or a healthy provider proves
victim ownership.
---
Nitpick comments:
In `@plugins/ci/skills/analyze-disruption/SKILL.md`:
- Around line 462-500: Consolidate the onset, revert-PR, related-triage,
component-scoped JIRA, and recurring-family guidance in sections 5.3–5.4 by
removing the duplicated workflow and referencing the canonical workflow in
bulk-triage-regressions/SKILL.md. Preserve any disruption-specific context not
covered there. Also resolve the fetch-related-triages call by adding a supported
step to obtain regression_id from the available Prow or Grafana URL, or remove
that invocation if no reliable conversion exists.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1ee835de-04ea-40c4-b76d-32081f303287
📒 Files selected for processing (1)
plugins/ci/skills/analyze-disruption/SKILL.md
| 5. **Condition churn ≠ slow rollout.** If an operator's condition flaps continuously (many | ||
| transitions with `DurationSinceTransition` of ~1s), that is a sync-loop bug (permafail | ||
| class), not a transient — count transitions before classifying severity. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Require evidence before classifying a sync-loop.
Rapid condition churn is evidence of possible sync-loop behavior. It is not proof of a sync-loop bug. Confirm the condition message, transition count, and controller logs before assigning sync-loop or permafail.
Proposed wording
- that is a sync-loop bug (permafail class), not a transient
+ treat that as possible sync-loop behavior; confirm it with condition messages and controller logs before classification📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 5. **Condition churn ≠ slow rollout.** If an operator's condition flaps continuously (many | |
| transitions with `DurationSinceTransition` of ~1s), that is a sync-loop bug (permafail | |
| class), not a transient — count transitions before classifying severity. | |
| 5. **Condition churn ≠ slow rollout.** If an operator's condition flaps continuously (many | |
| transitions with `DurationSinceTransition` of ~1s), treat that as possible sync-loop behavior; confirm it with condition messages and controller logs before classification — count transitions before classifying severity. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plugins/ci/skills/analyze-disruption/SKILL.md` around lines 400 - 402, Update
the “Condition churn ≠ slow rollout” guidance to avoid treating rapid
transitions as definitive proof of a sync-loop or permafail. Require
confirmation from the condition message, transition count, and controller logs
before assigning either classification, while preserving the distinction from
transient slow rollout behavior.
| 1. Find the newest **clean** run and the oldest **disrupted** run of the same job (job-history) | ||
| and compare their payloads (`artifacts/release/artifacts/release-images-latest`, and | ||
| `release-images-initial` for upgrade jobs — upgrade jobs can regress via either endpoint, | ||
| and a *skew* between initial and target payloads is itself a known failure mode). |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Compare adjacent runs at the onset boundary.
The newest clean run and oldest disrupted run can span unrelated payload changes. Compare the last clean run immediately before the first disrupted run. Inspect intervening runs when the boundary is not adjacent.
Proposed wording
- Find the newest clean run and the oldest disrupted run of the same job
+ Find the last clean run immediately before the first disrupted run of the same job📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 1. Find the newest **clean** run and the oldest **disrupted** run of the same job (job-history) | |
| and compare their payloads (`artifacts/release/artifacts/release-images-latest`, and | |
| `release-images-initial` for upgrade jobs — upgrade jobs can regress via either endpoint, | |
| and a *skew* between initial and target payloads is itself a known failure mode). | |
| 1. Find the last clean run immediately before the first disrupted run of the same job (job-history) | |
| and compare their payloads (`artifacts/release/artifacts/release-images-latest`, and | |
| `release-images-initial` for upgrade jobs — upgrade jobs can regress via either endpoint, | |
| and a *skew* between initial and target payloads is itself a known failure mode). |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plugins/ci/skills/analyze-disruption/SKILL.md` around lines 467 - 470, Update
the job-history guidance to compare the last clean run immediately preceding the
first disrupted run, rather than the newest clean and oldest disrupted runs
overall. When intervening runs exist, inspect them to identify the onset
boundary, then compare the relevant payloads for that adjacent clean/disrupted
pair.
| 2. Feed the failing payload to the `fetch-new-prs-in-payload` skill and vet candidates with | ||
| `gh` — a LIKELY PR names the owning component. | ||
| 3. **Scan the owning repo's newest merges for revert PRs**: a fresh `Revert "..."` title citing | ||
| a TRT/OCPBUGS key hands you the trigger PR, the tracking ticket, and the expected recovery | ||
| in one query — for currently-live breakage this is frequently faster than any artifact | ||
| analysis. | ||
| 4. Never write "resolved/appeared due to an unidentified payload change" without having run | ||
| steps 1–3; a real duplicate bug was filed because a cessation was written off as | ||
| unidentified while the owning repo had merged an `OCPBUGS-*`-titled fix at exactly that | ||
| boundary. | ||
|
|
||
| #### 5.4: Existing Bugs and Known Disruption Families | ||
|
|
||
| Before presenting a root-cause hypothesis as novel, check whether it is already tracked | ||
| (complementary to the known-disruption-card lookup in Step 8, which searches TRT's own | ||
| disruption Jira cards): | ||
|
|
||
| 1. Query `fetch-related-triages` for any Component Readiness regression associated with the | ||
| disrupted backends' tests, and treat confidence scores as clustering hints — verify against | ||
| this run's evidence before adopting a match. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 8 \
'fetch-new-prs-in-payload|fetch-related-triages|LIKELY PR|confidence' \
pluginsRepository: openshift-eng/ai-helpers
Length of output: 50383
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- target skill references ---'
rg -n -C 12 'fetch-new-prs-in-payload|fetch-related-triages|upgrade|payload|Section 5|5\.4' \
plugins/ci/skills/analyze-disruption/SKILL.md
printf '%s\n' '--- fetch-new-prs-in-payload interface ---'
sed -n '1,260p' plugins/ci/skills/fetch-new-prs-in-payload/SKILL.md
sed -n '1,280p' plugins/ci/skills/fetch-new-prs-in-payload/fetch_new_prs_in_payload.py
printf '%s\n' '--- fetch-related-triages interface ---'
sed -n '1,260p' plugins/ci/skills/fetch-related-triages/SKILL.md
sed -n '1,310p' plugins/ci/skills/fetch-related-triages/fetch_related_triages.pyRepository: openshift-eng/ai-helpers
Length of output: 50382
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
target = Path("plugins/ci/skills/analyze-disruption/SKILL.md").read_text()
target_lines = target.splitlines()
print("target_contract_references:")
for i, line in enumerate(target_lines, 1):
if "fetch-new-prs-in-payload" in line or "fetch-related-triages" in line:
print(f"{i}: {line}")
print("\ntarget_payload_selection_lines:")
for i, line in enumerate(target_lines, 1):
if re.search(r"payload|release-images-(latest|initial)|upgrade", line, re.I) and 450 <= i <= 500:
print(f"{i}: {line}")
print("\ntarget_regression_id_references:")
for i, line in enumerate(target_lines, 1):
if re.search(r"regression[_ -]?id|Component Readiness|regression", line, re.I):
print(f"{i}: {line}")
for name in (
"plugins/ci/skills/fetch-new-prs-in-payload/SKILL.md",
"plugins/ci/skills/fetch-related-triages/SKILL.md",
):
text = Path(name).read_text()
print(f"\n{name}:")
for i, line in enumerate(text.splitlines(), 1):
if re.search(r"python3 .*\.py|--format|--min-confidence|Exit Codes|Error Handling|payload_tag|regression_id|triaged_matches|untriaged_regressions", line):
print(f"{i}: {line}")
PYRepository: openshift-eng/ai-helpers
Length of output: 3416
Document the cross-skill lookup contracts.
Define the invocation, required inputs, output fields, and failure handling for both skills. For upgrade jobs, specify whether release-images-latest or release-images-initial supplies the PR diff and how payload skew is handled.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plugins/ci/skills/analyze-disruption/SKILL.md` around lines 471 - 490, Expand
the instructions for fetch-new-prs-in-payload and fetch-related-triages to
document each skill’s invocation, required inputs, output fields, and failure
handling. For upgrade jobs, explicitly identify whether release-images-latest or
release-images-initial provides the PR diff, and define how payload skew is
detected and handled.
|
Stale PRs are closed after 21d of inactivity. If this PR is still relevant, comment to refresh it or remove the stale label. If this PR is safe to close now please do so with /lifecycle stale |
Summary
Recent AI-assisted Component Readiness triage duty for
5.0-mainsurfaced several root-cause-attribution failure modes that cost real time (regressions attached to the wrong bug, a router change blamed for an etcd defect, a duplicate bug filed for an already-fixed issue). Most of them apply directly to disruption analysis, which is symptom-chain work by nature. This PR folds those lessons intoanalyze-disruption.Related: #651 (same lessons applied to the prow-job-analysis / triage-adjacent skills); #635 (the bulk-triage skill where they were learned).
Changes
Step 5.6 (new) — Root-Cause Attribution Rules
configmaps is forbiddenRBAC bug).clusteroperators.jsonas the end-state oracle: pods Running proves nothing about whether cluster etcd/apiservers were serving.*_previous.logis mandatory reading for any container that restarted in the disruption window.Step 7.3 (new) — Onset Determination and Suspect PRs for new/worsening disruption: clean-vs-disrupted payload comparison (including initial/target skew on upgrade jobs — a real regression was pure skew between a pre-GA base and post-GA target),
fetch-new-prs-in-payloadvetting, and the revert-PR shortcut (a freshRevert "..."citing a TRT/OCPBUGS key yields trigger PR + tracking ticket + recovery ETA in one query). Unanchored "unidentified payload change" claims are banned.Step 7.4 (new) — Existing Bugs and Known Disruption Families:
fetch-related-triageswith verify-before-adopt (confidence is clustering, not root cause), component-scoped JIRA listing (owning teams write summaries in developer vocabulary that keyword search misses — the exact mechanism behind a real duplicate filing), and the recurring disruption families to rule out first (etcd/Azure disk pressure, OVN upgrade CNI gaps, single-node OVS stalls, cloud LB churn).Report templates: both Root Cause Hypothesis sections now require the full causal chain, the 5.6 corroboration evidence, and existing-ticket references.
Also bumps the
ciplugin version (0.0.74 → 0.0.75).AI-generated content: This PR was created by AI based on lessons from AI-assisted triage duty. Please verify before acting on it.
Summary by CodeRabbit