TRT-2887: Significantly improve analyze-disruption skill - #679
openshift-merge-bot[bot] merged 8 commits into
Conversation
…e, and blast radius for disruption analysis Add find_disruption_runs.py for resolving Grafana disruption dashboard URLs into Prow job runs via Sippy API, with auto-selection of diverse representative samples (dedup, tier-based selection, job diversity, clean comparison from same job). Add download_timelines.py for parallel artifact downloading across multiple runs. Enhance parse_disruption.py with --format summary (one-line triage), --blast-radius (other disrupted backends), E2E test name extraction for cross-run correlation, and format_blast_radius for compact multi-backend output. Restructure SKILL.md: Grafana URL input flow, summary-first workflow to avoid context overflow, clean comparison A/B analysis guidance, simplified Jira search, collapsed redundant steps. Includes 55 tests across all three scripts. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
analyze-disruption skillanalyze-disruption skill
|
@smg247: This pull request references TRT-2887 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe CI disruption-analysis workflow now accepts Grafana dashboards, discovers representative Prow runs, downloads timelines concurrently, and produces summary and blast-radius reports. Documentation and plugin metadata now reflect version ChangesDisruption analysis workflow
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 9 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (9 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/ci/skills/analyze-disruption/SKILL.md (1)
366-482: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the stale step reference. Change “Step 1 step 3” to “Step 1, item 3” at line 200.
🤖 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 366 - 482, Update the stale reference in the Step 1 guidance near the referenced location, changing “Step 1 step 3” to “Step 1, item 3” without altering the surrounding instructions.
🧹 Nitpick comments (6)
plugins/ci/skills/analyze-disruption/find_disruption_runs.py (2)
493-493: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the unused unpacked variable.
Ruff reports
RUF059forconn_type, which is never used. Prefix it with an underscore to satisfy the lint rule.♻️ Proposed fix
- base_backend, conn_type = parse_backend(backend) + base_backend, _conn_type = parse_backend(backend)🤖 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/find_disruption_runs.py` at line 493, Rename the unused conn_type unpacked by parse_backend in the surrounding function to _conn_type, preserving base_backend and the existing parsing behavior while satisfying Ruff RUF059.Source: Linters/SAST tools
236-260: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unreachable zero-disruption branch and the redundant
Nonechecks.Line 222 removes every candidate with
disruption_seconds is None, and line 228 keeps only candidates withdisruption_seconds > 0. Thereforenon_zeroat line 237 always equals the full candidate list and can never be empty, so the branch at lines 240-242 is dead. For the same reason thec["disruption_seconds"] is not Noneguards at lines 246, 258, and 259 always evaluate true.♻️ Proposed simplification
# Phase 4: Categorize by disruption level - non_zero = sorted([c["disruption_seconds"] for c in candidates - if c["disruption_seconds"] > 0]) - - if not non_zero: - # All runs have 0 disruption — select by job diversity only - return _select_by_job_diversity(candidates, n) + non_zero = sorted(c["disruption_seconds"] for c in candidates) if len(non_zero) < 3: p50 = non_zero[len(non_zero) // 2] - high = [c for c in candidates if c["disruption_seconds"] is not None and c["disruption_seconds"] >= p50] + high = [c for c in candidates if c["disruption_seconds"] >= p50] low = [c for c in candidates if c not in high] @@ - high = [c for c in candidates if c["disruption_seconds"] is not None and c["disruption_seconds"] >= high_thresh] - moderate = [c for c in candidates if c["disruption_seconds"] is not None and low_thresh <= c["disruption_seconds"] < high_thresh] + high = [c for c in candidates if c["disruption_seconds"] >= high_thresh] + moderate = [c for c in candidates if low_thresh <= c["disruption_seconds"] < high_thresh]🤖 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/find_disruption_runs.py` around lines 236 - 260, Remove the unreachable `if not non_zero` branch and its `_select_by_job_diversity` return. In the disruption-tier construction, remove redundant `c["disruption_seconds"] is not None` guards from the `high` and `moderate` comprehensions, relying on the earlier candidate filtering while preserving the existing tier behavior.plugins/ci/skills/analyze-disruption/test_download_timelines.py (1)
129-132: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the downloaded timeline files.
test_process_run_full_successcheckstargetanderrorbut nottimeline_files, which is the primary output ofprocess_run. The mock already returns one timeline path, so the assertion is cheap and it locks in the basename-to-local-path mapping.💚 Proposed addition
assert r["target"] == "e2e-gcp-ovn-upgrade" assert r["error"] is None + assert len(r["timeline_files"]) == 1 + assert r["timeline_files"][0].endswith( + os.path.join("999", "logs", "e2e-timelines_spyglass_20260804-000654.json"))🤖 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/test_download_timelines.py` around lines 129 - 132, Update test_process_run_full_success to assert the returned timeline_files value, using the mock timeline path and expected basename-to-local-path mapping. Keep the existing build_id, job, target, and error assertions unchanged.plugins/ci/skills/analyze-disruption/parse_disruption.py (1)
386-393: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winClarify that
passed_testsholds every non-Error test.
passed_testscollects any test whose level is notError, soWarning-level tests are reported as passed.SKILL.mdlines 363-364 tells the analyst that tests which pass during disruption windows are likely causes, so this grouping can misdirect the conclusion. Rename the key or state the semantics in the docstring.♻️ Proposed clarification
- failed = sorted(n for n, t in tests.items() if t["level"] == "Error") - passed = sorted(n for n, t in tests.items() if t["level"] != "Error") + # "passed" means "not Error"; Warning-level tests are included here. + failed = sorted(n for n, t in tests.items() if t["level"] == "Error") + non_error = sorted(n for n, t in tests.items() if t["level"] != "Error") return { "count": len(events), "failed_tests": failed, - "passed_tests": passed, + "passed_tests": non_error, "tests": tests, }🤖 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/parse_disruption.py` around lines 386 - 393, Clarify the semantics of the passed_tests field in the return value of the disruption parsing function: it includes every test whose level is not "Error", including "Warning" tests, rather than only tests that passed. Update the relevant docstring or documentation to state this explicitly, preserving the existing grouping behavior.plugins/ci/skills/analyze-disruption/SKILL.md (1)
196-200: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winState that Step 1.5.3 requires the JSON invocation.
Step 1.5.1 runs the script with
--format table. The table output does not contain theurlfield. Step 1.5.3 then tells the reader to use theurlfield. Add an explicit instruction to re-run the script with--format jsonafter the user selects runs, so the Prow URLs are available.♻️ Proposed clarification
#### 1.5.3: Convert Selections to Prow URLs -Use the `url` field from the JSON output to get Prow URLs for the selected runs. Set: +Re-run the script with `--format json` (same flags as Step 1.5.1) and use the `url` field of +the selected rows to get Prow URLs. Set:🤖 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 196 - 200, Update Step 1.5.3 in the Analyze Disruption guide to explicitly require re-running the script with `--format json` after the user selects runs, so the `url` field is available for converting selections to Prow URLs. Keep the existing `--backends` default behavior and the downstream flow into Step 1 step 3 and Step 2 unchanged, and anchor the clarification in the Step 1.5.3 section rather than Step 1.5.1.plugins/ci/skills/analyze-disruption/test_find_disruption_runs.py (1)
361-369: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the assertion unconditional.
The assertion at line 369 runs only when
job-eis absent fromselected_jobs. If the selection changes and includesjob-e, this test passes without checking anything. The algorithm is deterministic, so assert the expected precondition explicitly.💚 Proposed fix
- # If job-e wasn't selected, its 0s run should not be the clean comparison - if "job-e" not in selected_jobs: - assert 0 not in selected_secs + # job-e has the lowest disruption and is not selected, so its 0s run must not + # be used as the clean comparison. + assert "job-e" not in selected_jobs + assert 0 not in selected_secs🤖 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/test_find_disruption_runs.py` around lines 361 - 369, Update the test in select_representative_runs to make the 0s-run check unconditional instead of guarding it with the job-e presence check. Use the existing selected_jobs and selected_secs setup in test_find_disruption_runs to explicitly assert the expected precondition for the deterministic selection, and keep the current max_disruption_for_backend-based comparison path unchanged.
🤖 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/download_timelines.py`:
- Around line 51-84: Update download_file and list_timeline_files to catch
subprocess.TimeoutExpired and OSError from subprocess.run, returning their
existing failure values so one failed run does not abort processing. Also guard
future.result() in the as_completed loop with exception handling and append the
standard failed-run result containing job, build_id, empty timeline_files, and
an error message, preserving successful results.
- Around line 113-126: Update process_run’s timeline download loop to count
failed download_file calls and set result["error"] when any download fails, not
only when result["timeline_files"] is empty. Preserve the existing no-files
error and successful-file collection, and make the error information include the
failed-download count so callers can detect an incomplete timeline set.
In `@plugins/ci/skills/analyze-disruption/find_disruption_runs.py`:
- Around line 111-112: Move JSON decoding in the response-parsing helper into
its existing error-handling path, catching ValueError/json.JSONDecodeError and
emitting the documented warning before returning an empty rows result. Apply the
same guard in fetch_disruption_data so invalid endpoint responses warn and
return {} without aborting the run.
In `@plugins/ci/skills/analyze-disruption/parse_disruption.py`:
- Around line 869-873: Update the blast-radius handling around args.blast_radius
and backend_filter to emit a warning when --blast-radius is provided without
--backends, while preserving the existing computation when both options are
present. Use the module’s established warning or logging mechanism and ensure
the warning clearly explains that --backends is required.
- Around line 652-662: Update format_summary in
plugins/ci/skills/analyze-disruption/parse_disruption.py lines 652-662 to
aggregate backend counts by stripped short name before constructing
backend_parts, then sort by summed count. Update
test_format_summary_strips_connection_suffix in
plugins/ci/skills/analyze-disruption/test_parse_disruption.py lines 165-177 to
expect one merged label such as kube-api:5.
In `@plugins/ci/skills/analyze-disruption/SKILL.md`:
- Around line 622-632: Update the two JQL examples in the disruption analysis
section so the boolean OR is applied outside each text search instead of inside
the quoted value. In both query blocks, replace the current `text ~
"{backend_name_1} OR {backend_name_2}"` pattern with a grouped `(text ~
"{backend_name_1}" OR text ~ "{backend_name_2}")` form, and apply the same
structure consistently for any additional backend names shown nearby.
---
Outside diff comments:
In `@plugins/ci/skills/analyze-disruption/SKILL.md`:
- Around line 366-482: Update the stale reference in the Step 1 guidance near
the referenced location, changing “Step 1 step 3” to “Step 1, item 3” without
altering the surrounding instructions.
---
Nitpick comments:
In `@plugins/ci/skills/analyze-disruption/find_disruption_runs.py`:
- Line 493: Rename the unused conn_type unpacked by parse_backend in the
surrounding function to _conn_type, preserving base_backend and the existing
parsing behavior while satisfying Ruff RUF059.
- Around line 236-260: Remove the unreachable `if not non_zero` branch and its
`_select_by_job_diversity` return. In the disruption-tier construction, remove
redundant `c["disruption_seconds"] is not None` guards from the `high` and
`moderate` comprehensions, relying on the earlier candidate filtering while
preserving the existing tier behavior.
In `@plugins/ci/skills/analyze-disruption/parse_disruption.py`:
- Around line 386-393: Clarify the semantics of the passed_tests field in the
return value of the disruption parsing function: it includes every test whose
level is not "Error", including "Warning" tests, rather than only tests that
passed. Update the relevant docstring or documentation to state this explicitly,
preserving the existing grouping behavior.
In `@plugins/ci/skills/analyze-disruption/SKILL.md`:
- Around line 196-200: Update Step 1.5.3 in the Analyze Disruption guide to
explicitly require re-running the script with `--format json` after the user
selects runs, so the `url` field is available for converting selections to Prow
URLs. Keep the existing `--backends` default behavior and the downstream flow
into Step 1 step 3 and Step 2 unchanged, and anchor the clarification in the
Step 1.5.3 section rather than Step 1.5.1.
In `@plugins/ci/skills/analyze-disruption/test_download_timelines.py`:
- Around line 129-132: Update test_process_run_full_success to assert the
returned timeline_files value, using the mock timeline path and expected
basename-to-local-path mapping. Keep the existing build_id, job, target, and
error assertions unchanged.
In `@plugins/ci/skills/analyze-disruption/test_find_disruption_runs.py`:
- Around line 361-369: Update the test in select_representative_runs to make the
0s-run check unconditional instead of guarding it with the job-e presence check.
Use the existing selected_jobs and selected_secs setup in
test_find_disruption_runs to explicitly assert the expected precondition for the
deterministic selection, and keep the current max_disruption_for_backend-based
comparison path unchanged.
🪄 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: 4ae00eb4-9a7f-4181-8d5e-f8ad344845e4
📒 Files selected for processing (11)
.claude-plugin/marketplace.jsondocs/index.htmlplugins/ci/.claude-plugin/plugin.jsonplugins/ci/commands/analyze-disruption.mdplugins/ci/skills/analyze-disruption/SKILL.mdplugins/ci/skills/analyze-disruption/download_timelines.pyplugins/ci/skills/analyze-disruption/find_disruption_runs.pyplugins/ci/skills/analyze-disruption/parse_disruption.pyplugins/ci/skills/analyze-disruption/test_download_timelines.pyplugins/ci/skills/analyze-disruption/test_find_disruption_runs.pyplugins/ci/skills/analyze-disruption/test_parse_disruption.py
💤 Files with no reviewable changes (1)
- plugins/ci/commands/analyze-disruption.md
Guard subprocess calls against TimeoutExpired/OSError, handle invalid JSON from Sippy APIs, count partial download failures, warn on --blast-radius without --backends, merge backend counts by short name in summary output, fix JQL OR syntax in SKILL.md, and clean up unreachable code paths and unused variables. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/download_timelines.py`:
- Around line 124-127: Update the `process_run`/`parse_runs` flow in
`download_timelines.py` to validate `build_id` before `logs_dir` is derived, so
a crafted value cannot escape `output_dir` when `gcloud storage cp` writes
files. Reject absolute paths and any path separators in `build_id`, or normalize
the computed `logs_dir`/`local_path` and enforce that the resolved target stays
contained under `output_dir` before copying.
In `@plugins/ci/skills/analyze-disruption/find_disruption_runs.py`:
- Around line 111-115: Validate the decoded Sippy response type in fetch_runs
and fetch_disruption_data before any .get access: after json.loads(body), reject
any non-dict value as an invalid response. In fetch_runs, keep the existing
stderr error path and exit(1); in fetch_disruption_data, emit the warning and
return {}. Update the checks around the json.loads result in both code paths so
arrays, scalars, and null cannot reach the rows or disruption lookup logic.
🪄 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: ac965f37-e81d-4a0d-95ec-ca8d7f68035a
📒 Files selected for processing (7)
plugins/ci/skills/analyze-disruption/SKILL.mdplugins/ci/skills/analyze-disruption/download_timelines.pyplugins/ci/skills/analyze-disruption/find_disruption_runs.pyplugins/ci/skills/analyze-disruption/parse_disruption.pyplugins/ci/skills/analyze-disruption/test_download_timelines.pyplugins/ci/skills/analyze-disruption/test_find_disruption_runs.pyplugins/ci/skills/analyze-disruption/test_parse_disruption.py
🚧 Files skipped from review as they are similar to previous changes (3)
- plugins/ci/skills/analyze-disruption/test_find_disruption_runs.py
- plugins/ci/skills/analyze-disruption/parse_disruption.py
- plugins/ci/skills/analyze-disruption/SKILL.md
The command file was removed earlier but is required for the /ci:analyze-disruption slash command to be discoverable. Restores it as a minimal wrapper that delegates entirely to the skill, avoiding the prior issue of duplicated context between command and SKILL.md. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/ci/commands/analyze-disruption.md (1)
3-3: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winKeep the command metadata and generated documentation aligned with the skill interface.
The command supports Grafana input and
--skip-jira, but the command hint, synopsis, and generated synopsis do not expose the complete contract. Update the command source, then regenerate the generated documentation.
plugins/ci/commands/analyze-disruption.md#L3-L3: add Grafana input and--skip-jiratoargument-hint.plugins/ci/commands/analyze-disruption.md#L12-L18: add--skip-jirato both synopsis forms.docs/index.html#L560-L560: regenerate the published synopsis from the updated command source.🤖 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/commands/analyze-disruption.md` at line 3, Update the analyze-disruption command metadata and generated documentation to expose the complete skill interface: in plugins/ci/commands/analyze-disruption.md lines 3-3, add Grafana input and --skip-jira to argument-hint; in lines 12-18, add --skip-jira to both synopsis forms; then regenerate docs/index.html lines 560-560 from the updated command source.
🤖 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.
Outside diff comments:
In `@plugins/ci/commands/analyze-disruption.md`:
- Line 3: Update the analyze-disruption command metadata and generated
documentation to expose the complete skill interface: in
plugins/ci/commands/analyze-disruption.md lines 3-3, add Grafana input and
--skip-jira to argument-hint; in lines 12-18, add --skip-jira to both synopsis
forms; then regenerate docs/index.html lines 560-560 from the updated command
source.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 81167f5e-5e35-4911-83b0-cd32ed6a2402
📒 Files selected for processing (2)
docs/index.htmlplugins/ci/commands/analyze-disruption.md
Reject build_id values with path separators or leading dots in parse_runs to prevent directory traversal. Validate json.loads results are dicts before .get access in fetch_runs and fetch_disruption_data. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
/label tide/merge-method-squash |
Update argument-hint and synopsis to reflect the full skill interface. Regenerate docs/index.html. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…kill Step 8.1.1 builds additional JQL queries from OVS stalls, CPU pressure, and etcd events already identified by the parser, so bugs filed about the mechanism (not the disrupted backend) are found. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…docs Fix substring conflation in max_disruption_for_backend that matched cache variants (e.g. cache-kube-api) when querying non-cache backends. Add E2ETest branch to format_text so failed/passed test names appear in --format text output. Document --limit/--since-hours as cost levers. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Previously cache backends were excluded unconditionally, breaking disruption lookups when the Grafana target itself was a cache backend (e.g. cache-kube-api). Now matching respects whether both sides are cache or non-cache. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mstaeble, smg247 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Add
find_disruption_runs.pyfor resolving Grafana disruption dashboard URLs into Prow job runs via Sippy API, with auto-selection of diverse representative samples (dedup, tier-based selection, job diversity, clean comparison from same job).Add
download_timelines.pyfor parallel artifact downloading across multiple runs.Enhance
parse_disruption.pywith --format summary (one-line triage), --blast-radius (other disrupted backends), E2E test name extraction for cross-run correlation, and format_blast_radius for compact multi-backend output.Restructure SKILL.md: Grafana URL input flow, summary-first workflow to avoid context overflow, clean comparison A/B analysis guidance, simplified Jira search, collapsed redundant steps.
Includes 55 tests across all three scripts.
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes
Special notes for your reviewer:
Checklist:
Summary by CodeRabbit
New Features
--skip-jiraoption.Changes
Tests