From 127ccacb9d79ce3c4b0aa5c4550d7f604706b835 Mon Sep 17 00:00:00 2001 From: Vincenzo Mauro Date: Thu, 16 Jul 2026 15:48:05 +0200 Subject: [PATCH 1/4] Added timing cache for ocp-ci-monitor dashboard --- .../payload_monitor/collectors/timing.py | 91 ++++++++++- .../payload_monitor/report/generator.py | 4 + .../report/templates/dashboard.html | 17 ++ .../tests/test_collectors_timing.py | 145 ++++++++++++++++++ .../tests/test_report_generator.py | 68 ++++++++ .../edge-ocp-ci/.claude-plugin/plugin.json | 2 +- .../skills/generate-dashboard/SKILL.md | 2 + 7 files changed, 326 insertions(+), 3 deletions(-) diff --git a/payload-monitor/payload_monitor/collectors/timing.py b/payload-monitor/payload_monitor/collectors/timing.py index 8fcd3fa9..78f1160c 100644 --- a/payload-monitor/payload_monitor/collectors/timing.py +++ b/payload-monitor/payload_monitor/collectors/timing.py @@ -5,6 +5,7 @@ from typing import Optional import json import logging +import os import statistics as stats_mod import xml.etree.ElementTree as ET from concurrent.futures import ThreadPoolExecutor, as_completed @@ -28,9 +29,91 @@ GCS_BASE = "https://storage.googleapis.com/test-platform-results/logs" +CACHE_ARTIFACT_RELPATH = ( + "artifacts/ocp-ci-monitor/" + "openshift-edge-tooling-ci-monitor/artifacts/timing_cache.json" +) + _session = create_session() +# --------------------------------------------------------------------------- +# GCS cache seeding (cross-run persistence) +# --------------------------------------------------------------------------- + +def seed_cache_from_previous_run(cache_path: Path) -> None: + """Download timing_cache.json from the previous Prow run's GCS artifacts. + + Uses the ``latest-build.txt`` convention to find the most recent completed + build, then fetches its ``timing_cache.json`` artifact over public HTTPS. + Skips silently (with a log warning) on any failure — this must never be + fatal, because a cold start is the natural fallback. + """ + if cache_path.exists(): + return + + job_name = os.environ.get("JOB_NAME", "") + if not job_name: + return + + current_build = os.environ.get("BUILD_ID", "") + + try: + resp = _session.get( + f"{GCS_BASE}/{job_name}/latest-build.txt", timeout=10, + ) + resp.raise_for_status() + latest_build = resp.text.strip() + except requests_lib.RequestException as e: + logger.warning(f"Could not fetch latest-build.txt for {job_name}: {e}") + return + + if not latest_build or "<" in latest_build: + logger.warning(f"Invalid latest-build.txt content for {job_name}") + return + + # Don't download our own (in-progress) artifacts. + if current_build and latest_build == current_build: + logger.info("latest-build.txt points to current run, skipping seed") + return + + cache_url = f"{GCS_BASE}/{job_name}/{latest_build}/{CACHE_ARTIFACT_RELPATH}" + try: + resp = _session.get(cache_url, timeout=30) + resp.raise_for_status() + except requests_lib.RequestException as e: + logger.warning(f"Could not fetch previous cache artifact: {e}") + return + + try: + json.loads(resp.text) + except (json.JSONDecodeError, ValueError): + logger.warning("Previous cache artifact is not valid JSON, ignoring") + return + + cache_path.parent.mkdir(parents=True, exist_ok=True) + cache_path.write_text(resp.text) + logger.info( + f"Seeded timing cache from build {latest_build} " + f"({len(json.loads(resp.text).get('runs', {}))} runs)" + ) + + +def _within_retention_window(timestamp_ms, days: int) -> bool: + """Return True if *timestamp_ms* falls within the last *days* days. + + Returns True for missing/zero timestamps (can't judge age). + """ + if not timestamp_ms: + return True + try: + run_time = datetime.fromtimestamp(timestamp_ms / 1000, tz=timezone.utc) + except (OSError, ValueError, OverflowError): + return True + cutoff = datetime.now(timezone.utc) - timedelta(days=days) + return run_time >= cutoff + + # --------------------------------------------------------------------------- # Variant extraction & job classification # --------------------------------------------------------------------------- @@ -355,6 +438,7 @@ def collect( 5. Fetch per-phase durations in parallel 6. Prune old data, save cache """ + seed_cache_from_previous_run(cache_path) report = load_cache(cache_path) cached_ids = set(report.runs.keys()) logger.info(f"Timing: loaded {len(cached_ids)} cached runs") @@ -407,8 +491,11 @@ def collect( continue for r in runs: rid = str(r.get("prow_id", "")) - if rid and rid not in cached_ids: - new_run_tasks.append((rid, job_name, r, version, topology, run_type, variant)) + if not rid or rid in cached_ids: + continue + if not _within_retention_window(r.get("timestamp", 0), days): + continue + new_run_tasks.append((rid, job_name, r, version, topology, run_type, variant)) logger.info(f"Timing: {len(new_run_tasks)} new runs to fetch details for") diff --git a/payload-monitor/payload_monitor/report/generator.py b/payload-monitor/payload_monitor/report/generator.py index 467f5870..539014a5 100644 --- a/payload-monitor/payload_monitor/report/generator.py +++ b/payload-monitor/payload_monitor/report/generator.py @@ -137,6 +137,8 @@ def _fail_sort_key(x): timing_html = "" if report.timing_report and not report.skip_timing: timing_html = render_timing_section(report.timing_report) + timing_unavailable = not report.skip_timing and not timing_html + timing_errors = [e for e in report.data_errors if e.startswith("Timing:")] if timing_unavailable else [] # Map blocking job name -> first index in all_failing for stable detail links blocking_job_first_idx = {} @@ -177,6 +179,8 @@ def _fail_sort_key(x): r.version for r in all_regressions if r.version )), "timing_html": timing_html, + "timing_unavailable": timing_unavailable, + "timing_errors": timing_errors, "failure_counts": report.failure_counts, "persistent_count": sum(1 for c in report.failure_counts.values() if c >= report.persistent_threshold), "recurring_threshold": report.recurring_threshold, diff --git a/payload-monitor/payload_monitor/report/templates/dashboard.html b/payload-monitor/payload_monitor/report/templates/dashboard.html index 85c54cf3..f4329be3 100644 --- a/payload-monitor/payload_monitor/report/templates/dashboard.html +++ b/payload-monitor/payload_monitor/report/templates/dashboard.html @@ -98,6 +98,23 @@

Edge OCP Payload Monitor

{% endif %} +{% if timing_unavailable %} +
+ Timing insights unavailable: --with-timing was enabled but timing + data could not be collected. The Timing Insights tab is not available for this report. + {% if timing_errors %} +
+ Show details +
    + {% for err in timing_errors %} +
  • {{ err }}
    • + {% endfor %} +
+
+ {% endif %} +
+{% endif %} +

Edge OCP Jobs Health Overview

diff --git a/payload-monitor/tests/test_collectors_timing.py b/payload-monitor/tests/test_collectors_timing.py index 775b73ce..61135d54 100644 --- a/payload-monitor/tests/test_collectors_timing.py +++ b/payload-monitor/tests/test_collectors_timing.py @@ -1,5 +1,7 @@ """Tests for payload_monitor.collectors.timing.""" +import json +import os import tempfile from pathlib import Path from unittest.mock import MagicMock, patch @@ -421,3 +423,146 @@ def test_zero_duration_skipped(self, mock_session): steps = timing.fetch_step_durations("test-job", "12345") assert "install" not in steps assert steps["pre phase"] == 100.0 + + +# --------------------------------------------------------------------------- +# Retention window helper +# --------------------------------------------------------------------------- + +class TestWithinRetentionWindow: + def test_recent_timestamp_is_within(self): + # 1 hour ago in milliseconds + import time + ts_ms = int((time.time() - 3600) * 1000) + assert timing._within_retention_window(ts_ms, days=7) is True + + def test_old_timestamp_is_outside(self): + # 30 days ago in milliseconds + import time + ts_ms = int((time.time() - 30 * 86400) * 1000) + assert timing._within_retention_window(ts_ms, days=7) is False + + def test_zero_timestamp_returns_true(self): + assert timing._within_retention_window(0, days=7) is True + + def test_none_timestamp_returns_true(self): + assert timing._within_retention_window(None, days=7) is True + + def test_boundary_exactly_at_cutoff(self): + import time + ts_ms = int((time.time() - 7 * 86400) * 1000) + # At the boundary (within a second of the cutoff) — may be just inside or + # just outside depending on execution speed, so just verify it doesn't crash. + result = timing._within_retention_window(ts_ms, days=7) + assert isinstance(result, bool) + + +# --------------------------------------------------------------------------- +# GCS cache seeding +# --------------------------------------------------------------------------- + +class TestSeedCacheFromPreviousRun: + def test_skips_when_cache_exists(self): + with tempfile.NamedTemporaryFile(suffix=".json", delete=False) as f: + f.write(b"{}") + cache_path = Path(f.name) + try: + timing.seed_cache_from_previous_run(cache_path) + # Should not have made any HTTP calls + finally: + cache_path.unlink(missing_ok=True) + + @patch.dict(os.environ, {}, clear=True) + def test_skips_when_job_name_unset(self): + cache_path = Path("/tmp/nonexistent_test_cache.json") + timing.seed_cache_from_previous_run(cache_path) + assert not cache_path.exists() + + @patch.object(timing, "_session") + @patch.dict(os.environ, {"JOB_NAME": "periodic-ci-test-job", "BUILD_ID": "200"}) + def test_success_writes_cache(self, mock_session): + cache_data = json.dumps({ + "last_updated": "2026-07-15T07:00:00Z", + "runs": {"111": { + "job_name": "j1", "topology": "TNA", "release": "4.22", + "start_time": "2026-07-15T06:00:00Z", "duration_seconds": 3600, + "result": "S", "run_type": "install", "variant": {}, + "step_durations": {}, + }}, + "phase_durations": {}, + }) + + latest_resp = MagicMock() + latest_resp.text = "199\n" + latest_resp.raise_for_status = MagicMock() + + cache_resp = MagicMock() + cache_resp.text = cache_data + cache_resp.raise_for_status = MagicMock() + + mock_session.get.side_effect = [latest_resp, cache_resp] + + with tempfile.TemporaryDirectory() as tmpdir: + cache_path = Path(tmpdir) / "timing_cache.json" + timing.seed_cache_from_previous_run(cache_path) + + assert cache_path.exists() + loaded = json.loads(cache_path.read_text()) + assert "111" in loaded["runs"] + + @patch.object(timing, "_session") + @patch.dict(os.environ, {"JOB_NAME": "periodic-ci-test-job", "BUILD_ID": "200"}) + def test_latest_build_failure_no_crash(self, mock_session): + mock_session.get.side_effect = requests.RequestException("network error") + + cache_path = Path("/tmp/nonexistent_seed_test.json") + timing.seed_cache_from_previous_run(cache_path) + assert not cache_path.exists() + + @patch.object(timing, "_session") + @patch.dict(os.environ, {"JOB_NAME": "periodic-ci-test-job", "BUILD_ID": "200"}) + def test_cache_artifact_404_no_crash(self, mock_session): + latest_resp = MagicMock() + latest_resp.text = "199\n" + latest_resp.raise_for_status = MagicMock() + + cache_resp = MagicMock() + cache_resp.raise_for_status.side_effect = requests.RequestException("404") + + mock_session.get.side_effect = [latest_resp, cache_resp] + + cache_path = Path("/tmp/nonexistent_seed_test.json") + timing.seed_cache_from_previous_run(cache_path) + assert not cache_path.exists() + + @patch.object(timing, "_session") + @patch.dict(os.environ, {"JOB_NAME": "periodic-ci-test-job", "BUILD_ID": "200"}) + def test_skips_when_latest_is_current_build(self, mock_session): + latest_resp = MagicMock() + latest_resp.text = "200\n" + latest_resp.raise_for_status = MagicMock() + + mock_session.get.return_value = latest_resp + + cache_path = Path("/tmp/nonexistent_seed_test.json") + timing.seed_cache_from_previous_run(cache_path) + assert not cache_path.exists() + # Should only have called GET once (for latest-build.txt), not for the cache artifact + assert mock_session.get.call_count == 1 + + @patch.object(timing, "_session") + @patch.dict(os.environ, {"JOB_NAME": "periodic-ci-test-job", "BUILD_ID": "200"}) + def test_invalid_json_from_artifact_no_crash(self, mock_session): + latest_resp = MagicMock() + latest_resp.text = "199\n" + latest_resp.raise_for_status = MagicMock() + + cache_resp = MagicMock() + cache_resp.text = "not valid json {" + cache_resp.raise_for_status = MagicMock() + + mock_session.get.side_effect = [latest_resp, cache_resp] + + cache_path = Path("/tmp/nonexistent_seed_test.json") + timing.seed_cache_from_previous_run(cache_path) + assert not cache_path.exists() diff --git a/payload-monitor/tests/test_report_generator.py b/payload-monitor/tests/test_report_generator.py index 58225cf8..48bf56f6 100644 --- a/payload-monitor/tests/test_report_generator.py +++ b/payload-monitor/tests/test_report_generator.py @@ -24,6 +24,8 @@ Regression, StreamReport, SuggestedBug, + TimingReport, + TimingRun, ) from payload_monitor.report.generator import ( _build_template_context, @@ -942,3 +944,69 @@ def test_header_status_updated_via_data_attribute(self, tmp_path): result = html_path.read_text() assert "AI-enriched via Claude" in result assert "data-only" not in result + + +class TestTimingUnavailableContext: + def test_false_when_timing_not_requested(self): + report = MonitorReport(generated_at="now", skip_timing=True) + ctx = _build_template_context(report) + assert ctx["timing_unavailable"] is False + + def test_false_when_timing_succeeded(self): + report = MonitorReport( + generated_at="now", + skip_timing=False, + timing_report=TimingReport( + last_updated="2026-07-15T07:00:00Z", + runs={"1": TimingRun( + "job1", "TNA", "4.22", "2026-07-15T06:00:00Z", + 3600, "S", "install", + )}, + ), + ) + ctx = _build_template_context(report) + assert ctx["timing_unavailable"] is False + + def test_true_when_timing_report_is_none(self): + report = MonitorReport( + generated_at="now", + skip_timing=False, + timing_report=None, + ) + ctx = _build_template_context(report) + assert ctx["timing_unavailable"] is True + + def test_true_when_timing_report_has_no_runs(self): + report = MonitorReport( + generated_at="now", + skip_timing=False, + timing_report=TimingReport(runs={}), + ) + ctx = _build_template_context(report) + assert ctx["timing_unavailable"] is True + + def test_banner_in_html_when_unavailable(self): + report = MonitorReport( + generated_at="now", + skip_timing=False, + timing_report=None, + data_errors=["Timing: Connection timed out"], + ) + html = generate_html(report) + assert "Timing insights unavailable" in html + assert "Connection timed out" in html + + def test_no_banner_in_html_when_timing_succeeded(self): + report = MonitorReport( + generated_at="now", + skip_timing=False, + timing_report=TimingReport( + last_updated="2026-07-15T07:00:00Z", + runs={"1": TimingRun( + "job1", "TNA", "4.22", "2026-07-15T06:00:00Z", + 3600, "S", "install", + )}, + ), + ) + html = generate_html(report) + assert "Timing insights unavailable" not in html diff --git a/plugins/edge-ocp-ci/.claude-plugin/plugin.json b/plugins/edge-ocp-ci/.claude-plugin/plugin.json index 54099c66..6fe5bab3 100644 --- a/plugins/edge-ocp-ci/.claude-plugin/plugin.json +++ b/plugins/edge-ocp-ci/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "edge-ocp-ci", "description": "Edge OCP Payload Monitor — monitor OpenShift nightly payloads for edge topology (SNO/TNA/TNF) failures with AI-enriched analysis", - "version": "1.1.0", + "version": "1.2.0", "author": { "name": "vimauro" }, diff --git a/plugins/edge-ocp-ci/skills/generate-dashboard/SKILL.md b/plugins/edge-ocp-ci/skills/generate-dashboard/SKILL.md index 0e5a73ec..9c82cdb3 100644 --- a/plugins/edge-ocp-ci/skills/generate-dashboard/SKILL.md +++ b/plugins/edge-ocp-ci/skills/generate-dashboard/SKILL.md @@ -85,6 +85,8 @@ cd "$TOOL_DIR" && .venv/bin/python -m payload_monitor --output reports/report-$( Pass through any relevant flags (`--versions`, `--payloads`, `--skip-prow`, `--skip-sippy`, `--with-timing`). +**Timeout guidance:** When using `--with-timing`, set the Bash tool timeout to at least **10 minutes** (600000 ms). On a cold start (first run after deployment, or if the previous run's cache artifact is unavailable), the timing collector may need to backfill up to 7 days of run data. Subsequent warm runs with a seeded cache typically complete in under 2 minutes. + **Important:** If a report with the same filename already exists, the tool automatically appends a timestamp (e.g., `report-2026-03-25-143027.html`). Capture the actual output path from the tool's log line: - `Report: /path/to/report-{name}.html` From d089e11451157870654e7d3c5fd84225d780cf86 Mon Sep 17 00:00:00 2001 From: Vincenzo Mauro Date: Thu, 16 Jul 2026 15:54:04 +0200 Subject: [PATCH 2/4] fixed plugin version --- .claude-plugin/marketplace.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index 0b63d4cf..9b03f279 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -22,7 +22,7 @@ "name": "edge-ocp-ci", "source": "./plugins/edge-ocp-ci", "description": "Edge OCP Payload Monitor — monitor OpenShift nightly payloads for edge topology (SNO/TNA/TNF) failures with AI-enriched analysis", - "version": "1.0.2" + "version": "1.2.0" }, { "name": "edge-scrum", From 9921e99c1f6b7fa40e7fed61ab07a4724471aeff Mon Sep 17 00:00:00 2001 From: Vincenzo Mauro Date: Thu, 16 Jul 2026 18:10:51 +0200 Subject: [PATCH 3/4] addresed CR comments --- .../payload_monitor/collectors/timing.py | 47 +++++++++++++++++-- .../tests/test_collectors_timing.py | 45 ++++++++++++++---- 2 files changed, 79 insertions(+), 13 deletions(-) diff --git a/payload-monitor/payload_monitor/collectors/timing.py b/payload-monitor/payload_monitor/collectors/timing.py index 78f1160c..709fdb9b 100644 --- a/payload-monitor/payload_monitor/collectors/timing.py +++ b/payload-monitor/payload_monitor/collectors/timing.py @@ -86,26 +86,67 @@ def seed_cache_from_previous_run(cache_path: Path) -> None: return try: - json.loads(resp.text) + payload = json.loads(resp.text) except (json.JSONDecodeError, ValueError): logger.warning("Previous cache artifact is not valid JSON, ignoring") return + if not _is_valid_cache_payload(payload): + logger.warning("Previous cache artifact has unexpected structure, ignoring") + return + cache_path.parent.mkdir(parents=True, exist_ok=True) cache_path.write_text(resp.text) logger.info( f"Seeded timing cache from build {latest_build} " - f"({len(json.loads(resp.text).get('runs', {}))} runs)" + f"({len(payload.get('runs', {}))} runs)" ) +# Fields load_cache() indexes directly (run_data["..."]) with no default — +# a missing or wrong-typed value there raises KeyError/TypeError. +_REQUIRED_RUN_STRING_FIELDS = ( + "job_name", "topology", "release", "start_time", "result", "run_type", +) + + +def _is_valid_cache_payload(data) -> bool: + """Validate a downloaded cache payload matches the shape load_cache() expects. + + This is an untrusted, externally-fetched artifact (GCS), so we allow-list + the exact structure rather than trusting arbitrary valid JSON. + """ + if not isinstance(data, dict): + return False + runs = data.get("runs") + if not isinstance(runs, dict): + return False + for run_data in runs.values(): + if not isinstance(run_data, dict): + return False + if not all( + isinstance(run_data.get(field), str) + for field in _REQUIRED_RUN_STRING_FIELDS + ): + return False + duration = run_data.get("duration_seconds") + if not isinstance(duration, (int, float)) or isinstance(duration, bool): + return False + for optional_field in ("variant", "step_durations"): + if optional_field in run_data and not isinstance(run_data[optional_field], dict): + return False + return True + + def _within_retention_window(timestamp_ms, days: int) -> bool: """Return True if *timestamp_ms* falls within the last *days* days. - Returns True for missing/zero timestamps (can't judge age). + Returns True for missing/zero/non-numeric timestamps (can't judge age). """ if not timestamp_ms: return True + if not isinstance(timestamp_ms, (int, float)) or isinstance(timestamp_ms, bool): + return True try: run_time = datetime.fromtimestamp(timestamp_ms / 1000, tz=timezone.utc) except (OSError, ValueError, OverflowError): diff --git a/payload-monitor/tests/test_collectors_timing.py b/payload-monitor/tests/test_collectors_timing.py index 61135d54..2909c154 100644 --- a/payload-monitor/tests/test_collectors_timing.py +++ b/payload-monitor/tests/test_collectors_timing.py @@ -456,6 +456,10 @@ def test_boundary_exactly_at_cutoff(self): result = timing._within_retention_window(ts_ms, days=7) assert isinstance(result, bool) + def test_nonnumeric_timestamp_returns_true(self): + for bad_value in ("2026-07-16", ["not", "a", "number"], {"ts": 1}): + assert timing._within_retention_window(bad_value, days=7) is True + # --------------------------------------------------------------------------- # GCS cache seeding @@ -473,8 +477,8 @@ def test_skips_when_cache_exists(self): cache_path.unlink(missing_ok=True) @patch.dict(os.environ, {}, clear=True) - def test_skips_when_job_name_unset(self): - cache_path = Path("/tmp/nonexistent_test_cache.json") + def test_skips_when_job_name_unset(self, tmp_path): + cache_path = tmp_path / "nonexistent_test_cache.json" timing.seed_cache_from_previous_run(cache_path) assert not cache_path.exists() @@ -512,16 +516,16 @@ def test_success_writes_cache(self, mock_session): @patch.object(timing, "_session") @patch.dict(os.environ, {"JOB_NAME": "periodic-ci-test-job", "BUILD_ID": "200"}) - def test_latest_build_failure_no_crash(self, mock_session): + def test_latest_build_failure_no_crash(self, mock_session, tmp_path): mock_session.get.side_effect = requests.RequestException("network error") - cache_path = Path("/tmp/nonexistent_seed_test.json") + cache_path = tmp_path / "nonexistent_seed_test.json" timing.seed_cache_from_previous_run(cache_path) assert not cache_path.exists() @patch.object(timing, "_session") @patch.dict(os.environ, {"JOB_NAME": "periodic-ci-test-job", "BUILD_ID": "200"}) - def test_cache_artifact_404_no_crash(self, mock_session): + def test_cache_artifact_404_no_crash(self, mock_session, tmp_path): latest_resp = MagicMock() latest_resp.text = "199\n" latest_resp.raise_for_status = MagicMock() @@ -531,20 +535,20 @@ def test_cache_artifact_404_no_crash(self, mock_session): mock_session.get.side_effect = [latest_resp, cache_resp] - cache_path = Path("/tmp/nonexistent_seed_test.json") + cache_path = tmp_path / "nonexistent_seed_test.json" timing.seed_cache_from_previous_run(cache_path) assert not cache_path.exists() @patch.object(timing, "_session") @patch.dict(os.environ, {"JOB_NAME": "periodic-ci-test-job", "BUILD_ID": "200"}) - def test_skips_when_latest_is_current_build(self, mock_session): + def test_skips_when_latest_is_current_build(self, mock_session, tmp_path): latest_resp = MagicMock() latest_resp.text = "200\n" latest_resp.raise_for_status = MagicMock() mock_session.get.return_value = latest_resp - cache_path = Path("/tmp/nonexistent_seed_test.json") + cache_path = tmp_path / "nonexistent_seed_test.json" timing.seed_cache_from_previous_run(cache_path) assert not cache_path.exists() # Should only have called GET once (for latest-build.txt), not for the cache artifact @@ -552,7 +556,7 @@ def test_skips_when_latest_is_current_build(self, mock_session): @patch.object(timing, "_session") @patch.dict(os.environ, {"JOB_NAME": "periodic-ci-test-job", "BUILD_ID": "200"}) - def test_invalid_json_from_artifact_no_crash(self, mock_session): + def test_invalid_json_from_artifact_no_crash(self, mock_session, tmp_path): latest_resp = MagicMock() latest_resp.text = "199\n" latest_resp.raise_for_status = MagicMock() @@ -563,6 +567,27 @@ def test_invalid_json_from_artifact_no_crash(self, mock_session): mock_session.get.side_effect = [latest_resp, cache_resp] - cache_path = Path("/tmp/nonexistent_seed_test.json") + cache_path = tmp_path / "nonexistent_seed_test.json" + timing.seed_cache_from_previous_run(cache_path) + assert not cache_path.exists() + + @patch.object(timing, "_session") + @patch.dict(os.environ, {"JOB_NAME": "periodic-ci-test-job", "BUILD_ID": "200"}) + def test_structurally_invalid_json_no_crash(self, mock_session, tmp_path): + # Valid JSON, but not the shape load_cache() expects (runs entries + # missing required string fields). + cache_data = json.dumps({"runs": {"111": {"job_name": "j1"}}}) + + latest_resp = MagicMock() + latest_resp.text = "199\n" + latest_resp.raise_for_status = MagicMock() + + cache_resp = MagicMock() + cache_resp.text = cache_data + cache_resp.raise_for_status = MagicMock() + + mock_session.get.side_effect = [latest_resp, cache_resp] + + cache_path = tmp_path / "nonexistent_seed_test.json" timing.seed_cache_from_previous_run(cache_path) assert not cache_path.exists() From a43a083ec3a8b8bb37962670c9dc01ab76aab8a5 Mon Sep 17 00:00:00 2001 From: Vincenzo Mauro Date: Tue, 21 Jul 2026 13:47:01 +0200 Subject: [PATCH 4/4] addressed PR comments --- .../payload_monitor/collectors/timing.py | 25 +++++-- .../tests/test_collectors_timing.py | 66 +++++++++++++++++++ 2 files changed, 85 insertions(+), 6 deletions(-) diff --git a/payload-monitor/payload_monitor/collectors/timing.py b/payload-monitor/payload_monitor/collectors/timing.py index 709fdb9b..4bd0b936 100644 --- a/payload-monitor/payload_monitor/collectors/timing.py +++ b/payload-monitor/payload_monitor/collectors/timing.py @@ -46,7 +46,7 @@ def seed_cache_from_previous_run(cache_path: Path) -> None: Uses the ``latest-build.txt`` convention to find the most recent completed build, then fetches its ``timing_cache.json`` artifact over public HTTPS. - Skips silently (with a log warning) on any failure — this must never be + Skips gracefully on any failure (logging a warning) — this must never be fatal, because a cold start is the natural fallback. """ if cache_path.exists(): @@ -87,16 +87,22 @@ def seed_cache_from_previous_run(cache_path: Path) -> None: try: payload = json.loads(resp.text) - except (json.JSONDecodeError, ValueError): - logger.warning("Previous cache artifact is not valid JSON, ignoring") + except (json.JSONDecodeError, ValueError) as e: + logger.warning( + f"Previous cache artifact is not valid JSON (build {latest_build}): {e}" + ) return if not _is_valid_cache_payload(payload): logger.warning("Previous cache artifact has unexpected structure, ignoring") return - cache_path.parent.mkdir(parents=True, exist_ok=True) - cache_path.write_text(resp.text) + try: + cache_path.parent.mkdir(parents=True, exist_ok=True) + cache_path.write_text(resp.text) + except OSError as e: + logger.warning(f"Could not write seeded cache to {cache_path}: {e}") + return logger.info( f"Seeded timing cache from build {latest_build} " f"({len(payload.get('runs', {}))} runs)" @@ -135,6 +141,12 @@ def _is_valid_cache_payload(data) -> bool: for optional_field in ("variant", "step_durations"): if optional_field in run_data and not isinstance(run_data[optional_field], dict): return False + for v in run_data.get("step_durations", {}).values(): + if not isinstance(v, (int, float)) or isinstance(v, bool): + return False + for v in run_data.get("variant", {}).values(): + if not isinstance(v, str): + return False return True @@ -472,7 +484,8 @@ def collect( """Collect timing data for SNO/TNA/TNF jobs across versions. Pipeline (parallelized at each stage): - 1. Load existing cache + 1. Seed the local cache from the previous Prow run's GCS artifacts, then + load the (possibly just-seeded) existing cache 2. Fetch SNO/TNA/TNF jobs for all versions in parallel 3. Fetch job runs for all jobs in parallel 4. Fetch summaries + step durations for all new runs in parallel diff --git a/payload-monitor/tests/test_collectors_timing.py b/payload-monitor/tests/test_collectors_timing.py index 2909c154..f7ed9340 100644 --- a/payload-monitor/tests/test_collectors_timing.py +++ b/payload-monitor/tests/test_collectors_timing.py @@ -460,6 +460,52 @@ def test_nonnumeric_timestamp_returns_true(self): for bad_value in ("2026-07-16", ["not", "a", "number"], {"ts": 1}): assert timing._within_retention_window(bad_value, days=7) is True + def test_bool_timestamp_returns_true(self): + assert timing._within_retention_window(True, days=7) is True + + +VALID_RUN = { + "job_name": "j1", "topology": "TNA", "release": "4.22", + "start_time": "2026-07-15T06:00:00Z", "result": "S", "run_type": "install", + "duration_seconds": 3600, "variant": {"network": "ipv4"}, + "step_durations": {"install": 120.0}, +} + + +class TestIsValidCachePayload: + def test_valid_payload_passes(self): + assert timing._is_valid_cache_payload({"runs": {"111": VALID_RUN}}) is True + + def test_non_dict_payload_rejected(self): + assert timing._is_valid_cache_payload(["not", "a", "dict"]) is False + + def test_non_dict_runs_rejected(self): + assert timing._is_valid_cache_payload({"runs": "not_a_dict"}) is False + + def test_missing_required_field_rejected(self): + bad_run = {k: v for k, v in VALID_RUN.items() if k != "job_name"} + assert timing._is_valid_cache_payload({"runs": {"111": bad_run}}) is False + + def test_non_numeric_duration_rejected(self): + bad_run = {**VALID_RUN, "duration_seconds": "not_a_number"} + assert timing._is_valid_cache_payload({"runs": {"111": bad_run}}) is False + + def test_bool_duration_rejected(self): + bad_run = {**VALID_RUN, "duration_seconds": True} + assert timing._is_valid_cache_payload({"runs": {"111": bad_run}}) is False + + def test_non_numeric_step_duration_value_rejected(self): + bad_run = {**VALID_RUN, "step_durations": {"install": "not_a_number"}} + assert timing._is_valid_cache_payload({"runs": {"111": bad_run}}) is False + + def test_bool_step_duration_value_rejected(self): + bad_run = {**VALID_RUN, "step_durations": {"install": True}} + assert timing._is_valid_cache_payload({"runs": {"111": bad_run}}) is False + + def test_non_string_variant_value_rejected(self): + bad_run = {**VALID_RUN, "variant": {"network": 123}} + assert timing._is_valid_cache_payload({"runs": {"111": bad_run}}) is False + # --------------------------------------------------------------------------- # GCS cache seeding @@ -591,3 +637,23 @@ def test_structurally_invalid_json_no_crash(self, mock_session, tmp_path): cache_path = tmp_path / "nonexistent_seed_test.json" timing.seed_cache_from_previous_run(cache_path) assert not cache_path.exists() + + @patch.object(timing, "_session") + @patch.dict(os.environ, {"JOB_NAME": "periodic-ci-test-job", "BUILD_ID": "200"}) + def test_write_failure_no_crash(self, mock_session, tmp_path): + cache_data = json.dumps({"runs": {"111": VALID_RUN}}) + + latest_resp = MagicMock() + latest_resp.text = "199\n" + latest_resp.raise_for_status = MagicMock() + + cache_resp = MagicMock() + cache_resp.text = cache_data + cache_resp.raise_for_status = MagicMock() + + mock_session.get.side_effect = [latest_resp, cache_resp] + + cache_path = tmp_path / "nonexistent_seed_test.json" + with patch.object(Path, "write_text", side_effect=OSError("disk full")): + timing.seed_cache_from_previous_run(cache_path) + assert not cache_path.exists()