OCPEDGE-2813: OCPEDGE-2812: Added timing cache for ocp-ci-monitor dashboard

[vimauro]

Summary by CodeRabbit

• New Features
  • Improves dashboard timing performance by reusing timing cache from previous runs when available.
  • Adds retention-window filtering to avoid using outdated timing run data.
  • Enhances the dashboard with a “Timing insights unavailable” banner and surfaces timing collection errors when present.
• Bug Fixes
  • Validates cached timing data structure and handles warm-start/backfill failures gracefully.
• Documentation
  • Updates --with-timing instructions with clearer timeout guidance and cold-start vs warm behavior.
• Chores
  • Bumped the Edge OCP CI plugin version to 1.2.0.

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vimauro

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [vimauro]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[vimauro]

/label tide/merge-method-squash

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: 01c308f0-90ea-4919-90e7-61514dfe2f66

📥 Commits

Reviewing files that changed from the base of the PR and between 9921e99 and a43a083.

📒 Files selected for processing (2)

• payload-monitor/payload_monitor/collectors/timing.py
• payload-monitor/tests/test_collectors_timing.py

🚧 Files skipped from review as they are similar to previous changes (1)

• payload-monitor/payload_monitor/collectors/timing.py

---

Walkthrough

Changes

Timing collection and dashboard reporting

Layer / File(s)	Summary
Warm-start cache and retention filtering payload-monitor/payload_monitor/collectors/timing.py, payload-monitor/tests/test_collectors_timing.py	Timing collection seeds missing local caches from previous runs, validates downloaded payloads, filters runs outside the retention window, and tests success and failure paths.
Unavailable timing dashboard state payload-monitor/payload_monitor/report/generator.py, payload-monitor/payload_monitor/report/templates/dashboard.html, payload-monitor/tests/test_report_generator.py	Report context exposes timing availability and errors, and the dashboard renders an unavailable-timing banner with optional error details.
Plugin release and execution guidance .claude-plugin/marketplace.json, plugins/edge-ocp-ci/.claude-plugin/plugin.json, plugins/edge-ocp-ci/skills/generate-dashboard/SKILL.md	Plugin versions are updated to 1.2.0, with timeout and warm-start guidance added to the dashboard skill documentation.

Estimated code review effort: 3 (Moderate) | ~25 minutes

🚥 Pre-merge checks | ✅ 9 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Ai-Attribution	⚠️ Warning	PR commits use Co-Authored-By for Claude (e.g. 807fc52...), not the required Assisted-by/Generated-by trailer.	Replace AI co-author trailers with Red Hat Approved attribution, e.g. Assisted-by: Claude ... or Generated-by: ... in the affected commits.

✅ Passed checks (9 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main change: adding timing cache support for the ocp-ci-monitor dashboard.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
No-Weak-Crypto	✅ Passed	No MD5/SHA1/DES/RC4/3DES/Blowfish/ECB, custom crypto, or secret/token comparisons were added; changes use HTTPS, JSON parsing, and benign BUILD_ID equality.
Container-Privileges	✅ Passed	PR only changes timing.py and tests; no container/K8s manifests or privilege settings (privileged, hostPID/Network/IPC, SYS_ADMIN, allowPrivilegeEscalation) are present.
No-Sensitive-Data-In-Logs	✅ Passed	New logs only include job/build IDs, file paths, and exception text; no passwords, tokens, PII, session IDs, or customer data are logged.
No-Hardcoded-Secrets	✅ Passed	Changed files add timing-cache logic/tests only; scans found no hardcoded secrets, embedded credentials, private keys, or long base64 strings.
No-Injection-Vectors	✅ Passed	No new shell/eval/pickle/yaml/SQL sinks were added; new timing errors are rendered via Jinja autoescape, and the existing safe HTML use pre-existed.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 3

🤖 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 `@payload-monitor/payload_monitor/collectors/timing.py`:
- Around line 88-95: Update the cache validation flow in the timing collector
before cache_path.write_text, using a schema-aware, allow-list validator that
requires a dictionary root, a dictionary runs value, and every field required by
load_cache with the expected types. Log and discard any structurally invalid
response so the documented cold-start fallback remains intact, while preserving
persistence for valid caches.
- Around line 102-114: Update _within_retention_window to validate timestamp_ms
as an accepted numeric value at the trust boundary, while preserving the True
fallback for missing or invalid timestamps. Ensure truthy strings, lists,
mappings, and other unsupported types cannot raise during timestamp conversion,
and add a regression test covering a nonnumeric timestamp.

In `@payload-monitor/tests/test_collectors_timing.py`:
- Line 477: Replace the hardcoded /tmp paths in the affected tests with paths
built from pytest’s tmp_path fixture. Add tmp_path to each relevant test
function signature and construct each intentionally nonexistent cache path
beneath it, preserving the existing test behavior while avoiding shared
filenames and Ruff S108 violations.

🪄 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: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: b5a34b92-c55e-40e4-9dc8-b68625dfcc82

📥 Commits

Reviewing files that changed from the base of the PR and between 2561ad6 and d089e11.

📒 Files selected for processing (8)

• .claude-plugin/marketplace.json
• payload-monitor/payload_monitor/collectors/timing.py
• payload-monitor/payload_monitor/report/generator.py
• payload-monitor/payload_monitor/report/templates/dashboard.html
• payload-monitor/tests/test_collectors_timing.py
• payload-monitor/tests/test_report_generator.py
• plugins/edge-ocp-ci/.claude-plugin/plugin.json
• plugins/edge-ocp-ci/skills/generate-dashboard/SKILL.md

[lucaconsalvi]

PR Review — Timing Cache Seeding & Retention Window

Nice work on this, Vincenzo — the defensive coding throughout is solid. The seeding function correctly treats every failure as non-fatal, Jinja2 auto-escaping prevents XSS on the error banner, and the test coverage is thorough for most paths. A couple of items worth addressing.

---

Critical

1. Unprotected filesystem writes violate the non-fatal contract
timing.py — end of seed_cache_from_previous_run()

The docstring states "this must never be fatal." Every HTTP and JSON failure path is wrapped in try/except, but the final filesystem operations are unprotected:

cache_path.parent.mkdir(parents=True, exist_ok=True)
cache_path.write_text(resp.text)

PermissionError (read-only FS), OSError (disk full), or IsADirectoryError would crash collect(). In CI with a full ephemeral disk, the user gets a traceback instead of the graceful "Timing insights unavailable" banner.

Suggested fix:

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

2. _is_valid_cache_payload doesn't validate step_durations/variant dict values
timing.py — _is_valid_cache_payload()

The validator checks that step_durations and variant are dicts, but not that their values have correct types. A poisoned cache entry like {"step_durations": {"install": "not_a_number"}} passes validation, gets deserialized into a TimingRun, and install_duration_seconds silently returns a string — downstream arithmetic raises TypeError. Similarly, non-string variant values crash _variant_key() during rendering.

Suggested fix — add after the existing optional-field loop:

step_durs = run_data.get("step_durations", {})
for v in step_durs.values():
    if not isinstance(v, (int, float)) or isinstance(v, bool):
        return False
variant = run_data.get("variant", {})
for v in variant.values():
    if not isinstance(v, str):
        return False

---

Important

3. collect() docstring not updated
The pipeline description still reads "1. Load existing cache" but the PR adds seed_cache_from_previous_run(cache_path) as the very first line. A future maintainer won't know collect() can make external HTTP calls to GCS before touching the local cache.

4. JSON parse failure log lacks diagnostic context
The json.JSONDecodeError handler says "not valid JSON" but doesn't include the exception message or build number — makes debugging a failed seed in a Prow run harder than it needs to be:

except (json.JSONDecodeError, ValueError) as e:
    logger.warning(f"Previous cache artifact is not valid JSON (build {latest_build}): {e}")

---

Suggestions

• _is_valid_cache_payload() has no direct unit tests — it's the security boundary between the internet and local disk, currently only exercised indirectly through one seed_cache_from_previous_run test case
• _within_retention_window has an explicit isinstance(timestamp_ms, bool) guard but no test pinning that behavior
• "Skips silently (with a log warning)" in the docstring is self-contradictory — consider "Skips gracefully on any failure (logging a warning)"

---

Verified safe (no action needed)

• XSS in template: {{ err }} is safe — autoescape=True at generator.py:43
• Corrupt partial cache write: load_cache() catches json.JSONDecodeError and returns empty TimingReport(), so a truncated file degrades to cold start
• latest_build URL injection: malformed value produces a 404 caught by RequestException handler; the "<" check is a reasonable extra guard
• Fail-open retention design: returning True for unjudgeable timestamps is correct — prune_cache() later applies stricter eviction on ISO start_time
• Version sync: marketplace.json jumping 1.0.2 → 1.2.0 brings it in line with plugin.json (was already behind)

[lucaconsalvi]

/lgtm