TRT-2741: Add GA release test data loader sourced from BigQuery#3780
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@mstaeble: This pull request references TRT-2741 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. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
Tip For best results, initiate chat on the files or code changes.
|
0a7d821 to
a6ccc80
Compare
mstaeble
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
mstaeble
changed the title
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
a6ccc80 to
a0f401f
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 56 minutes. |
WalkthroughAdds a ChangesGA test status pipeline
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant LoadCommand
participant GATestStatusLoader
participant BigQuery
participant Postgres
LoadCommand->>GATestStatusLoader: Start GA test status load
GATestStatusLoader->>Postgres: Select GA releases
GATestStatusLoader->>BigQuery: Query GA window aggregates
BigQuery-->>GATestStatusLoader: Return deduplicated counts
GATestStatusLoader->>Postgres: Persist raw counts and loaded date
Possibly related PRs
Suggested reviewers: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 3 warnings)
✅ Passed checks (17 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@cmd/sippy/seed_data.go`:
- Around line 1128-1149: Wrap the `CreateInBatches` call and every
`ga_data_loaded_date` update in a single database transaction. Use the
transaction handle for both insertion and updates, propagate any error so the
transaction rolls back, and commit only after all releases in the `gaReleases`
loop complete successfully.
- Around line 1059-1156: The GA data pipeline needs cross-layer test coverage.
In cmd/sippy/seed_data.go:1059-1156, add tests for seedGARawTestData covering
window scaling, missing test/job dimension lookups, generated row values,
ga_data_loaded_date updates, and transaction rollback. In
pkg/db/views.go:496-509, add tests covering grouping, job filtering, and
total/success/flake aggregation behavior.
- Around line 1074-1106: Update the suite lookup and the test/job queries in the
GA raw test data generation flow to distinguish confirmed record-not-found
errors from operational database failures. Continue the existing fallback or
skip behavior only for not-found results; wrap and return any other errors
instead of continuing, ensuring callers receive failures and releases are not
marked loaded with incomplete rows.
In `@pkg/dataloader/gateststatus/loader.go`:
- Around line 207-215: Update GATestStatusLoader.gaReleases to compare the
date-only ga_date column against CURRENT_DATE instead of NOW(), ensuring
releases are loaded only after the GA day is complete. Add a regression test
covering a release on the current date and verifying it is excluded, while a
prior-date release remains eligible.
- Around line 231-274: Validate l.bqClient.Dataset at client construction using
an anchored allow-list or the project’s shared BigQuery identifier helper, and
reject malformed values before this query’s fmt.Sprintf interpolation. Ensure
only valid dataset identifiers reach the query while preserving normal
configured dataset behavior.
In `@pkg/db/models/prow.go`:
- Around line 228-232: The model fields in the diff lack a database-level
uniqueness constraint for each persisted aggregate. Update the relevant GORM
tags on Release, WindowDays, TestID, ProwJobID, and SuiteID to share a composite
unique index covering (release, window_days, test_id, prow_job_id, suite_id),
preserving the existing index behavior where needed.
🪄 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: 651e401b-c985-4afc-a935-b9a9f4139631
📒 Files selected for processing (9)
cmd/sippy/load.gocmd/sippy/seed_data.gopkg/api/componentreadiness/utils/utils.gopkg/bigquery/bqlabel/labels.gopkg/dataloader/gateststatus/loader.gopkg/db/db.gopkg/db/models/prow.gopkg/db/models/releases.gopkg/db/views.go
Add a new `ga-test-status` loader that fetches test results from BigQuery for GA releases and stores them in `prow_ga_raw_test_data`. The loader runs as a separate pod (not included in default loaders) and supports three lookback windows (1-day, 30-day, 90-day) matching Component Readiness view definitions. Key design choices: - Per-release BQ query batching all 3 windows via UNNEST with struct array parameters, with static scalar date bounds for partition pruning - COPY-to-temp-table + INSERT...SELECT with JOINs to resolve string names (test_name, job_name, suite) to integer FK IDs server-side, reducing row size from ~265 bytes to ~44 bytes (6x reduction) - Matview (prow_ga_test_statuses_matview) only needs a single JOIN on prow_jobs for variant_combination_id; tests and suites JOINs are eliminated since IDs are stored directly - Load state tracked via ga_data_loaded_date on release_definitions, updated atomically within the per-release transaction Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
a0f401f to
90bc050
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
mstaeble
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
@coderabbitai resolve |
openshift-ci
Bot
added
the
ready-for-human-review
✅ Action performedComments resolved and changes approved. |
|
Scheduling required tests: |
| &models.ReleasePullRequest{}, | ||
| &models.ReleaseRepository{}, | ||
| &models.ReleaseJobRun{}, | ||
| &models.ProwGARawTestDatum{}, |
There was a problem hiding this comment.
Are you thinking making this a partitioned table (possibly only by release) is overkill? Would this table go away once we fully backfill for older job runs?
There was a problem hiding this comment.
My initial stance is that partitioning this table is overkill. I will explore the benefit of that later if there is a need for improvement.
If we were to fully backfill, then yes this table would not be necessary. We may still want the matviews that this table feeds, though. The same data static data is used across many CR reports. It does not change from one day to the next, unlike the sample data.
| ReplaceStrings: map[string]string{}, | ||
| }, | ||
| { | ||
| Name: "prow_ga_test_statuses_matview", |
There was a problem hiding this comment.
You just got rid of matviews 😁
There was a problem hiding this comment.
Remove some, add some more.
We did not get rid of all matviews. My goal is not to eliminate matviews.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mstaeble, neisw 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 |
openshift-ci
Bot
added
the
approved
|
@mstaeble: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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 kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
Summary
prow_ga_raw_test_datatable andGATestStatusLoaderthat fetches test results from BigQuery for each GA release's lookback windowsprow_ga_test_statuses_matviewthat joins raw data with prow_jobs for variant_combination_id (tests/suites JOINs eliminated since IDs stored directly)--loader ga-test-status), not included in default loadersStaging test results
Loaded release 4.17 against staging (510K BQ rows):
Matview refresh (460K raw rows, 1 release):
~10% of BQ rows (50K/510K) were dropped by INNER JOINs to tests/prow_jobs, which is expected: these are tests/jobs from older releases that the prow loader doesn't load into Postgres dimension tables.
Review feedback
ga_date < NOW()toga_date < CURRENT_DATEto avoid loading partial GA-day datagorm.ErrRecordNotFoundfrom real DB errors in dimension lookupsTest plan
go buildcompilesgo vetpassesmake lintpassesmake testpasses (13,938 Go tests, sippy-ng Jest tests)--force-ga-refreshcorrectly re-fetches and replaces data🤖 Generated with Claude Code