[WIP] Optimize prow loader with COPY protocol, bulk operations, and GCS improvements#3745
[WIP] Optimize prow loader with COPY protocol, bulk operations, and GCS improvements#3745mstaeble wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR replaces regex-based GCS discovery with glob matching and updates consumers for flat match lists. Prow loading now preprocesses jobs, filters completed work, deduplicates job runs, and bulk-inserts extracted test data through temporary tables. ChangesGCS discovery and consumers
Prow import pipeline
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ProwLoader
participant Database
participant GCSJobRun
participant GCS
ProwLoader->>Database: preprocess jobs and find new job-run IDs
Database-->>ProwLoader: eligible jobs
ProwLoader->>GCSJobRun: FindAllMatches(ctx, junit glob)
GCSJobRun->>GCS: query matching objects
GCS-->>GCSJobRun: matched JUnit paths
GCSJobRun-->>ProwLoader: JUnit suites
ProwLoader->>Database: bulk-load extracted test rows
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 18 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (18 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 |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
needs-rebase
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mstaeble 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 |
mstaeble
changed the title
mstaeble
force-pushed
the
worktree-prow-job-run-tests-copy
branch
from
d17a12c to
88fc542
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai full review |
✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 16 minutes. |
|
@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: 2
🧹 Nitpick comments (3)
pkg/dataloader/prowloader/gcs/gcs_jobrun.go (1)
155-180: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAccept
context.ContextinFindAllMatchesfor cancellation and timeout support.
FindAllMatchesusescontext.Background()at line 167, making GCS listing operations uncancellable by the caller.GetContentin the same struct already acceptsctx context.Context, so this creates an inconsistency. Theocp.gocaller has a context available, and the API handler callers already usecontext.TODO()forGetContent. For buckets with many objects, listing can be long-running and should respect cancellation.As per path instructions, Go code should use
context.Contextfor cancellation and timeouts. As per coding guidelines, follow idiomatic Go practices.♻️ Proposed refactor
-func (j *GCSJobRun) FindAllMatches(glob string) ([]string, error) { +func (j *GCSJobRun) FindAllMatches(ctx context.Context, glob string) ([]string, error) { q := &storage.Query{ Prefix: j.gcsProwJobPath, MatchGlob: glob, } if err := q.SetAttrSelection([]string{"Name"}); err != nil { return nil, errors.Wrap(err, "error setting attribute selection") } var matches []string - it := j.bkt.Objects(context.Background(), q) + it := j.bkt.Objects(ctx, q)Callers would then pass their available context:
// gcs_jobrun.go:52 -matches, err := j.FindAllMatches(j.gcsProwJobPath + "/**junit*xml") +matches, err := j.FindAllMatches(ctx, j.gcsProwJobPath + "/**junit*xml")// job_run_events.go:75 -matches, err := gcsJobRun.FindAllMatches("**/gather-extra/artifacts/events.json") +matches, err := gcsJobRun.FindAllMatches(context.TODO(), "**/gather-extra/artifacts/events.json")// job_run_intervals.go:47,51 -intervalFiles, err := gcsJobRun.FindAllMatches("**e2e-events*json") +intervalFiles, err := gcsJobRun.FindAllMatches(context.TODO(), "**e2e-events*json") -timelineFiles, err := gcsJobRun.FindAllMatches("**e2e-timelines*json") +timelineFiles, err := gcsJobRun.FindAllMatches(context.TODO(), "**e2e-timelines*json")// ocp.go:218 -clusterMatches, err := gcsJobRun.FindAllMatches("**/cluster-data_*json") +clusterMatches, err := gcsJobRun.FindAllMatches(ctx, "**/cluster-data_*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 `@pkg/dataloader/prowloader/gcs/gcs_jobrun.go` around lines 155 - 180, FindAllMatches currently hardcodes context.Background(), so the GCS listing cannot be canceled or timed out by callers. Update the GCSJobRun.FindAllMatches method to accept a context.Context parameter, pass that context into j.bkt.Objects, and adjust callers such as the ocp.go path and any API handler usage to forward their existing context instead of relying on an uncancellable background context.Sources: Coding guidelines, Path instructions
pkg/dataloader/prowloader/prow.go (2)
1516-1554: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider unit tests for the reworked suite-attribution and flake logic.
extractTestCasesis pure logic (child-suite name attribution, failure/success→flake promotion, output retention) and this PR changes its behavior; a small table-driven test over a syntheticjunit.TestSuitetree would lock in the new attribution and flake rules without needing any storage clients.🤖 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 `@pkg/dataloader/prowloader/prow.go` around lines 1516 - 1554, Add table-driven unit tests for extractTestCases to lock in the new pure logic around suite attribution, recursive child-suite traversal, and failure/success-to-flake promotion. Cover synthetic junit.TestSuite trees with parent/child suites, ignored and skipped cases, and mixed duplicate results so you verify the testCaseEntry status/output behavior stays correct without depending on any storage clients.Source: Coding guidelines
1120-1137: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConnection-pool sizing with 50 concurrent workers.
Each worker acquires a dedicated
pgxconnection viastdlib.AcquireConnfor the temp-table + COPY path, on top of the GORM transaction inprocessGCSBucketJobRun. WithmaxConcurrencyraised to 50, confirm the underlyingsql.DBpool (SetMaxOpenConns) can supply at least that many, otherwise workers will serialize on connection acquisition and erode the bulk-insert speedup.🤖 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 `@pkg/dataloader/prowloader/prow.go` around lines 1120 - 1137, The bulk insert path in ProwLoader’s bulkInsertJobRunTests currently acquires a dedicated pgx connection per worker on top of the GORM transaction, so the sql.DB pool must be sized to support the raised maxConcurrency of 50. Update the connection-pool configuration used by processGCSBucketJobRun and/or the DB initialization so SetMaxOpenConns is at least 50 (and consistent with any related idle limits), ensuring stdlib.AcquireConn in bulkInsertJobRunTests does not serialize workers and negate the COPY/temp-table throughput gain.
🤖 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 `@pkg/dataloader/prowloader/prow.go`:
- Around line 1485-1488: The synthetic suite importability check in
prowJobRunTestsFromGCS currently panics, which can crash the worker pool instead
of failing just that job run. Replace the panic after
db.IsSuiteImportable(syntheticSuite.Name) with an error return from
prowJobRunTestsFromGCS, and make the caller handle that error path cleanly
before extractTestCases runs. Use the existing prowJobRunTestsFromGCS and
db.IsSuiteImportable symbols to locate the guard and propagate a descriptive
error rather than terminating the goroutine.
- Around line 736-753: The row processing in the query path for new job run IDs
is ignoring scan/iteration failures, so fix the logic in the function that
builds newIDs from rows to handle errors from rows.Scan and to check rows.Err()
after the loop. If either Scan or iteration fails, return a wrapped error with
context using fmt.Errorf and %w instead of silently continuing, and only build
the set from fully validated IDs.
---
Nitpick comments:
In `@pkg/dataloader/prowloader/gcs/gcs_jobrun.go`:
- Around line 155-180: FindAllMatches currently hardcodes context.Background(),
so the GCS listing cannot be canceled or timed out by callers. Update the
GCSJobRun.FindAllMatches method to accept a context.Context parameter, pass that
context into j.bkt.Objects, and adjust callers such as the ocp.go path and any
API handler usage to forward their existing context instead of relying on an
uncancellable background context.
In `@pkg/dataloader/prowloader/prow.go`:
- Around line 1516-1554: Add table-driven unit tests for extractTestCases to
lock in the new pure logic around suite attribution, recursive child-suite
traversal, and failure/success-to-flake promotion. Cover synthetic
junit.TestSuite trees with parent/child suites, ignored and skipped cases, and
mixed duplicate results so you verify the testCaseEntry status/output behavior
stays correct without depending on any storage clients.
- Around line 1120-1137: The bulk insert path in ProwLoader’s
bulkInsertJobRunTests currently acquires a dedicated pgx connection per worker
on top of the GORM transaction, so the sql.DB pool must be sized to support the
raised maxConcurrency of 50. Update the connection-pool configuration used by
processGCSBucketJobRun and/or the DB initialization so SetMaxOpenConns is at
least 50 (and consistent with any related idle limits), ensuring
stdlib.AcquireConn in bulkInsertJobRunTests does not serialize workers and
negate the COPY/temp-table throughput gain.
🪄 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: 80558e1e-95c0-49f7-baa3-4a682b5326e6
📒 Files selected for processing (7)
pkg/api/jobrunevents/job_run_events.gopkg/api/jobrunintervals/job_run_intervals.gopkg/dataloader/prowloader/bigqueryjobs.gopkg/dataloader/prowloader/gcs/gcs_jobrun.gopkg/dataloader/prowloader/prow.gopkg/db/suites.gopkg/variantregistry/ocp.go
mstaeble
force-pushed
the
worktree-prow-job-run-tests-copy
branch
from
88fc542 to
d5a8bf1
Compare
openshift-ci
Bot
added
the
ready-for-human-review
mstaeble
force-pushed
the
worktree-prow-job-run-tests-copy
branch
from
d5a8bf1 to
33c0771
Compare
mstaeble
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
Scheduling required tests: |
…nd GCS improvements Replace per-row database operations and in-memory caches with bulk PostgreSQL operations, and optimize GCS artifact fetching. Database changes: - Replace per-row findOrAddTest (SELECT+INSERT per test name) and CreateInBatches with COPY into a temp table + a writable CTE that resolves test/suite IDs via JOINs and inserts test outputs in one statement - Replace 800k+ in-memory job run ID cache with a temp table COPY + LEFT JOIN anti-pattern to find new runs - Bulk-upsert ProwJob definitions in batches of 100 instead of per-row inserts - Remove all shared mutable caches and their locks (prowJobRunTestCache, suiteCache, prowJobCacheLock, prowJobRunCacheLock) - Pre-filter candidates by release matching and PG existence check before the goroutine loop, so workers receive only new runs GCS changes: - Use MatchGlob for server-side filtering in FindAllMatches, replacing client-side regex matching - Use SetAttrSelection(["Name"]) to reduce GCS response payload - Remove unnecessary Attrs/Generation pinning in GetContent (CI artifacts are immutable) - Increase worker concurrency from 10 to 50 - Pre-compile release config regexes at construction instead of per-job Behavioral change: extractTestCases now attributes tests to their actual child suite name rather than inheriting the parent suite's ID. The old behavior assigned all nested JUnit tests to the top-level suite, which was incorrect for tests in child suites. Benchmarked against staging-2 (all releases, 4h lookback): - Baseline: 5m11s for ~1,100 jobs - Optimized: 38s for ~830 jobs (8x speedup) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
mstaeble
force-pushed
the
worktree-prow-job-run-tests-copy
branch
from
33c0771 to
71baed9
Compare
mstaeble
changed the title
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
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)
pkg/dataloader/prowloader/prow.go (1)
1063-1101: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake the run and test inserts atomic.
bulkInsertJobRunTestshappens after theprow_jobs_runtransaction on a separate connection, so a failure here can leave a committed run with noprow_job_run_tests.findNewJobRunIDsonly checksprow_job_runs, so that run will be skipped on the next load and never backfilled. Move the test insert into the same transaction or add a repair path.🤖 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 `@pkg/dataloader/prowloader/prow.go` around lines 1063 - 1101, Make creation of the Prow job run, pull-request associations, and test rows atomic by moving the bulk test insertion performed by bulkInsertJobRunTests into the existing transaction in the surrounding loader method. Update the helper as needed to accept and use the transaction’s *gorm.DB connection, and return any test-insert error from the transaction before committing; remove the separate post-transaction call so a failure rolls back all related records.
🧹 Nitpick comments (2)
pkg/db/pgx.go (1)
37-52: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueStatic-analysis SQL-injection hint here is a false positive, but consider validating the identifier.
tempTableis interpolated intoCREATE TEMP TABLE/DROP TABLEbecause SQL identifiers cannot use bind placeholders. All current callers pass hard-coded constants, so there is no user-controlled input and the ast-grep finding is a false positive. Since this is a reusablepkg/dbhelper, a cheap defense-in-depth guard (reject names not matching^[a-zA-Z_][a-zA-Z0-9_]*$) would keep it safe against future callers.🤖 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 `@pkg/db/pgx.go` around lines 37 - 52, Validate tempTable before interpolating it into SQL in the helper containing the drop closure and createSQL construction, allowing only identifiers matching ^[a-zA-Z_][a-zA-Z0-9_]*$; return a descriptive error for invalid names so both CREATE and DROP statements remain protected.Source: Linters/SAST tools
pkg/dataloader/prowloader/prow.go (1)
269-303: 🚀 Performance & Scalability | 🔵 TrivialConnection-pool sizing with 50 concurrent workers.
maxConcurrencyis now 50, and each worker'sbulkInsertJobRunTestshijacks a dedicated connection viastdlib.AcquireConn, holding it for the temp-table COPY plus multipleExecstatements. If the underlyingdatabase/sqlpoolMaxOpenConnsis below ~50 (plus headroom for the gorm transaction that also runs per job), workers will serialize or stall on connection acquisition, negating the concurrency increase. Worth verifying the pool is sized accordingly.🤖 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 `@pkg/dataloader/prowloader/prow.go` around lines 269 - 303, Verify the database connection pool configuration used by the workers launched in the `prowJobToJobRun` processing loop and ensure `MaxOpenConns` provides at least 50 connections plus headroom for each job’s GORM transaction and other database operations. Update the pool sizing configuration or reduce `maxConcurrency` as appropriate, and confirm `bulkInsertJobRunTests` and related transaction paths do not stall on `stdlib.AcquireConn`.
🤖 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 `@pkg/dataloader/prowloader/prow.go`:
- Around line 1063-1101: Make creation of the Prow job run, pull-request
associations, and test rows atomic by moving the bulk test insertion performed
by bulkInsertJobRunTests into the existing transaction in the surrounding loader
method. Update the helper as needed to accept and use the transaction’s *gorm.DB
connection, and return any test-insert error from the transaction before
committing; remove the separate post-transaction call so a failure rolls back
all related records.
---
Nitpick comments:
In `@pkg/dataloader/prowloader/prow.go`:
- Around line 269-303: Verify the database connection pool configuration used by
the workers launched in the `prowJobToJobRun` processing loop and ensure
`MaxOpenConns` provides at least 50 connections plus headroom for each job’s
GORM transaction and other database operations. Update the pool sizing
configuration or reduce `maxConcurrency` as appropriate, and confirm
`bulkInsertJobRunTests` and related transaction paths do not stall on
`stdlib.AcquireConn`.
In `@pkg/db/pgx.go`:
- Around line 37-52: Validate tempTable before interpolating it into SQL in the
helper containing the drop closure and createSQL construction, allowing only
identifiers matching ^[a-zA-Z_][a-zA-Z0-9_]*$; return a descriptive error for
invalid names so both CREATE and DROP statements remain protected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 3d561a5d-8280-4ab9-800d-de3f60d11e06
📒 Files selected for processing (9)
pkg/api/jobrunevents/job_run_events.gopkg/api/jobrunintervals/job_run_intervals.gopkg/dataloader/prowloader/bigqueryjobs.gopkg/dataloader/prowloader/extract_test_cases_test.gopkg/dataloader/prowloader/gcs/gcs_jobrun.gopkg/dataloader/prowloader/prow.gopkg/db/pgx.gopkg/db/suites.gopkg/variantregistry/ocp.go
|
@coderabbitai pause |
✅ Action performedReviews paused. |
1 participant
Summary
Behavioral change
extractTestCasesnow attributes tests to their actual child suite name rather than inheriting the parent suite's ID. The old behavior assigned all nested JUnit tests to the top-level suite, which was incorrect for tests in child suites with different names.Benchmarks (staging-2, all releases, 4h lookback)
Job count varies by run (different 4-hour windows). Per-job throughput is consistently much higher.
Test plan
gofmt,go vet,golangci-lintpassmake testpasses🤖 Generated with Claude Code
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation