TRT-2734: switch release consumers from BigQuery to PostgreSQL#3736
TRT-2734: switch release consumers from BigQuery to PostgreSQL#3736mstaeble wants to merge 2 commits into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
[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
force-pushed
the
TRT-2734-switch-release-consumers
branch
10 times, most recently
from
cf40d96 to
fbaaf38
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRelease metadata retrieval migrates from BigQuery-only functions to shared DB-backed helpers (GetReleasesFromDB, new GetReleaseDatesFromDB). A new MixedProvider routes release queries to Postgres and other queries to BigQuery. CLI commands and serve.go gain a --data-provider flag with a newDataProvider selector. Callers and server handlers are updated accordingly, alongside test coverage changes. ChangesRelease data provider migration
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ServeCmd
participant newDataProvider
participant BigQueryProvider
participant PostgresProvider
participant MixedProvider
ServeCmd->>newDataProvider: f.DataProvider, bigQueryClient, dbc, cacheClient
newDataProvider->>MixedProvider: construct if both configured
MixedProvider->>PostgresProvider: QueryReleases/QueryReleaseDates
MixedProvider->>BigQueryProvider: all other queries
newDataProvider-->>ServeCmd: crDataProvider
Suggested reviewers: 🚥 Pre-merge checks | ✅ 17 | ❌ 4❌ Failed checks (4 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/sippyserver/metrics/metrics.go (1)
122-129: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRelease list should not depend on
dbcalone
RefreshMetricsDBstill runs withcrProvider/bqcenabled whendbcis nil in the component-readiness path, soreleasesstays empty and CR/disruption metrics lose release metadata. Keep the old trigger or ensure these callers never pass a nildbc.🤖 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/sippyserver/metrics/metrics.go` around lines 122 - 129, RefreshMetricsDB currently gates release loading only on dbc, which leaves releases empty in the component-readiness path even when crProvider or bqc are enabled. Update the release-fetching logic in RefreshMetricsDB/GetReleasesFromDB flow so release metadata is still populated for those callers, either by restoring the previous trigger condition or by guaranteeing that the readiness callers always supply a non-nil dbc; use the dbc check, crProvider, bqc, and releases symbols to locate the fix.
🧹 Nitpick comments (2)
pkg/api/releases.go (1)
503-503: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMagic string
"30"for days adjustment.
util.AdjustReleaseTime(*release.GADate, true, "30", ...)hardcodes the day-adjustment as a string that gets re-parsed internally viastrconv.ParseInt. Consider a named constant for readability, though this mirrors the existingAdjustReleaseTimesignature elsewhere.🤖 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/api/releases.go` at line 503, The `AdjustReleaseTime` call in `releases.go` uses a hardcoded "30" day-adjustment string, which should be replaced with a named constant for readability and maintainability. Update the `prior := util.AdjustReleaseTime(...)` usage to reference a descriptive constant defined near the release-time logic or shared with other callers, while keeping the existing `AdjustReleaseTime` signature unchanged.cmd/sippy/automatejira.go (1)
155-162: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDB client now created before option validation and other client setup.
dbcis now acquired at Line 155, ahead ofjiraClient(Line 164),GetJobVariants(Line 173),GetVariantJiraMap(Line 177), andf.Validate(Line 181). If any of these later steps fail, a Postgres connection was opened unnecessarily. Low impact since the process exits shortly after viareturn/panic, but worth noting as the DB client acquisition moved earlier in the flow compared to before.🤖 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 `@cmd/sippy/automatejira.go` around lines 155 - 162, The Postgres client in the automateJira flow is being opened too early in the setup sequence, before validation and the other client/map initialization steps. Move the dbc acquisition in automateJira so it happens after jiraClient, GetJobVariants, GetVariantJiraMap, and f.Validate succeed, and keep the existing error handling around GetDBClient and GetReleasesFromDB intact.
🤖 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/api/componentreadiness/component_report.go`:
- Around line 281-283: `NewReleaseFallbackMiddleware` is being registered even
when `c.dbc` is nil, which leaves `QueryTestDetails()` able to hit
`GetReleaseDatesFromDB()` and panic via `GetReleasesFromDB()` on `dbc.DB`.
Update the middleware setup in `component_report.go` so the `releasefallback`
path is only added when `c.dbc` is non-nil, matching the existing
`regressiontracker` guard; alternatively, ensure the multi-release analysis
branch is skipped when `dbc` is unavailable.
In `@pkg/api/componentreadiness/middleware/releasefallback/releasefallback.go`:
- Around line 200-202: Guard the fallback DB lookup in ReleaseFallback when dbc
may be nil, since api.GetReleaseDatesFromDB and its call path into
GetReleasesFromDB dereference dbc.DB directly and can panic. Update the
ReleaseFallback middleware flow to either skip the DB-backed fallback path when
r.dbc is nil or enforce a non-nil DB in the ReleaseFallback constructor, and
make sure the handling around GetReleaseDatesFromDB uses the chosen safeguard
consistently.
In `@pkg/sippyserver/server.go`:
- Around line 903-906: The error handling in the BigQuery-backed handler uses
misleading “database” wording even though this path still calls
api.GetTestRunsAndOutputsFromBigQuery with s.bigQueryClient. Update the log and
failure response in this branch to consistently mention BigQuery instead of
database so the messages match the actual data source and are easier to debug.
---
Outside diff comments:
In `@pkg/sippyserver/metrics/metrics.go`:
- Around line 122-129: RefreshMetricsDB currently gates release loading only on
dbc, which leaves releases empty in the component-readiness path even when
crProvider or bqc are enabled. Update the release-fetching logic in
RefreshMetricsDB/GetReleasesFromDB flow so release metadata is still populated
for those callers, either by restoring the previous trigger condition or by
guaranteeing that the readiness callers always supply a non-nil dbc; use the dbc
check, crProvider, bqc, and releases symbols to locate the fix.
---
Nitpick comments:
In `@cmd/sippy/automatejira.go`:
- Around line 155-162: The Postgres client in the automateJira flow is being
opened too early in the setup sequence, before validation and the other
client/map initialization steps. Move the dbc acquisition in automateJira so it
happens after jiraClient, GetJobVariants, GetVariantJiraMap, and f.Validate
succeed, and keep the existing error handling around GetDBClient and
GetReleasesFromDB intact.
In `@pkg/api/releases.go`:
- Line 503: The `AdjustReleaseTime` call in `releases.go` uses a hardcoded "30"
day-adjustment string, which should be replaced with a named constant for
readability and maintainability. Update the `prior :=
util.AdjustReleaseTime(...)` usage to reference a descriptive constant defined
near the release-time logic or shared with other callers, while keeping the
existing `AdjustReleaseTime` signature unchanged.
🪄 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: 14a3eb5d-fa70-446a-a9a6-3563023e2dc0
📒 Files selected for processing (18)
cmd/sippy/automatejira.gocmd/sippy/load.gocmd/sippy/seed_data.gopkg/api/componentreadiness/component_report.gopkg/api/componentreadiness/dataprovider/bigquery/provider.gopkg/api/componentreadiness/dataprovider/bigquery/releasedates.gopkg/api/componentreadiness/dataprovider/interface.gopkg/api/componentreadiness/dataprovider/postgres/provider.gopkg/api/componentreadiness/middleware/releasefallback/releasefallback.gopkg/api/componentreadiness/middleware/releasefallback/releasefallback_test.gopkg/api/componentreadiness/test_details.gopkg/api/job_runs.gopkg/api/releases.gopkg/api/releases_test.gopkg/api/utils.gopkg/mcp/tools/releases.gopkg/sippyserver/metrics/metrics.gopkg/sippyserver/server.go
💤 Files with no reviewable changes (6)
- pkg/api/componentreadiness/dataprovider/bigquery/releasedates.go
- pkg/api/componentreadiness/dataprovider/interface.go
- pkg/api/releases_test.go
- pkg/api/componentreadiness/dataprovider/bigquery/provider.go
- pkg/api/utils.go
- pkg/api/componentreadiness/dataprovider/postgres/provider.go
mstaeble
force-pushed
the
TRT-2734-switch-release-consumers
branch
2 times, most recently
from
f99e94d to
d260205
Compare
openshift-ci
Bot
added
the
ready-for-human-review
mstaeble
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@mstaeble: This pull request references TRT-2734 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
removed
the
do-not-merge/work-in-progress
|
@coderabbitai pause |
mstaeble
changed the title
✅ Action performedReviews paused. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
mstaeble
force-pushed
the
TRT-2734-switch-release-consumers
branch
12 times, most recently
from
9717f43 to
d6d2515
Compare
Switch release data consumers to prefer PostgreSQL when available, falling back to BigQuery for deployments without a database (e.g., the QE component readiness deployment). Key changes: - getReleases() in server.go prefers PG, falls back to BQ - GetComponentReport and RefreshMetricsDB try GetReleasesFromDB first, fall back to provider.QueryReleases for BQ-only deployments - QueryReleases stays on the DataProvider interface for BQ-only deployments; PG provider delegates to GetReleasesFromDB, BQ provider converts via ReleaseRowToDefinition - Remove hardcoded releaseMetadata map from PG provider - Add GetReleaseDatesFromDB to derive CR time ranges from PG - Update releasefallback and test_details to use GetReleaseDatesFromDB - Switch automatejira, MCP releases tool, and job_runs to use PG - Delete BQ releasedates.go - Remove QueryReleaseDates from the DataProvider interface - Fix stale error messages referencing specific backends Ref: TRT-2734 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
mstaeble
force-pushed
the
TRT-2734-switch-release-consumers
branch
from
d6d2515 to
ac48a8c
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. |
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)
cmd/sippy/automatejira.go (1)
163-187: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard the postgres path when
GetDBClient()fails
--data-provider postgresstill builds aPostgresProviderwith a nildbchere. The nextcomponentreadiness.GetJobVariants(ctx, provider)call hitsQueryJobVariants(), which dereferencesp.dbc.DBand crashes before any fallback can happen. Return the DB error, or reject postgres mode without a connection.🤖 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 `@cmd/sippy/automatejira.go` around lines 163 - 187, The postgres branch in automateJiraRun still proceeds after GetDBClient() fails, leaving dbc nil and causing a crash later in componentreadiness.GetJobVariants via PostgresProvider.QueryJobVariants. Update the flow around GetDBClient() and newDataProvider so postgres mode is rejected or the DB error is returned immediately when the client cannot be created, instead of continuing to build a PostgresProvider with a nil dbc.
🧹 Nitpick comments (2)
pkg/api/job_runs.go (1)
423-446: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider caching the
GetReleasesFromDBresult to avoid a duplicate query.When both
compareRelease == "Presubmits"andtotalJobRuns < 20are true,GetReleasesFromDB(ctx, dbc)is called twice with identical arguments. The first call's result (line 375) is scoped to theifblock and not reused by the second call (line 424). Hoisting the result to a shared variable would avoid a redundant DB round-trip.♻️ Optional refactor to deduplicate the DB call
func JobRunRiskAnalysis( ctx context.Context, logger *log.Entry, dbc *db.DB, bqc *bigquery.Client, cacheClient cache.Cache, jobRun *models.ProwJobRun, compareOtherPRs bool, ) (apitype.ProwJobRunRiskAnalysis, error) { logger = logger.WithField("func", "JobRunRiskAnalysis") compareRelease := jobRun.ProwJob.Release neverStableJob := false + var dbReleases []sippyv1.Release + + getDBReleases := func() []sippyv1.Release { + if dbReleases == nil { + if r, err := GetReleasesFromDB(ctx, dbc); err == nil { + dbReleases = r + } + } + return dbReleases + } + if compareRelease == "Presubmits" { - ar, err := GetReleasesFromDB(ctx, dbc) - if err != nil { - return apitype.ProwJobRunRiskAnalysis{}, err - } + ar := getDBReleases() + if ar == nil { + return apitype.ProwJobRunRiskAnalysis{}, fmt.Errorf("failed to load releases from db") + } if len(ar) == 0 { return apitype.ProwJobRunRiskAnalysis{}, fmt.Errorf("no releases found in db") } compareRelease = ar[0].Release }Then at line 424, use
releases := getDBReleases()instead of callingGetReleasesFromDBagain.🤖 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/api/job_runs.go` around lines 423 - 446, Cache the result of GetReleasesFromDB in job_runs.go so it can be reused instead of issuing the same query twice when compareRelease is "Presubmits" and totalJobRuns < 20. Hoist the releases lookup out of the narrower if block into a shared variable in the surrounding scope, then have the prior-release lookup branch reuse that variable rather than calling GetReleasesFromDB(ctx, dbc) again. Keep the existing logic in findReleaseMatchJobNames and the prior-release handling unchanged, only remove the redundant DB round-trip.cmd/sippy/serve.go (1)
273-277: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
bigquerybranch returns(nil, nil), silently yielding a nil provider.When
bigQueryClientis nil, this returns a nilDataProviderwith no error, so callers assigncrDataProvider = niland start the server with component-readiness silently disabled, unlikedefault/postgreswhich surface a configuration error. Consider returning an explicit error for consistency and easier diagnosis.♻️ Proposed change
case "bigquery": if bigQueryClient != nil { return bqprovider.NewBigQueryProvider(bigQueryClient), nil } - return nil, nil + return nil, fmt.Errorf("bigquery data provider requires google-service-account-credential-file to be configured")🤖 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 `@cmd/sippy/serve.go` around lines 273 - 277, The bigquery branch in the provider selection logic is silently returning a nil DataProvider when bigQueryClient is absent, which leaves the server running with readiness disabled and no diagnostic signal. Update the switch case in the provider factory path around the bigquery handling to return an explicit error instead of nil,nil when bigQueryClient is nil, matching the behavior of the default and postgres branches and making the failure visible to callers.
🤖 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 `@cmd/sippy/automatejira.go`:
- Around line 163-187: The postgres branch in automateJiraRun still proceeds
after GetDBClient() fails, leaving dbc nil and causing a crash later in
componentreadiness.GetJobVariants via PostgresProvider.QueryJobVariants. Update
the flow around GetDBClient() and newDataProvider so postgres mode is rejected
or the DB error is returned immediately when the client cannot be created,
instead of continuing to build a PostgresProvider with a nil dbc.
---
Nitpick comments:
In `@cmd/sippy/serve.go`:
- Around line 273-277: The bigquery branch in the provider selection logic is
silently returning a nil DataProvider when bigQueryClient is absent, which
leaves the server running with readiness disabled and no diagnostic signal.
Update the switch case in the provider factory path around the bigquery handling
to return an explicit error instead of nil,nil when bigQueryClient is nil,
matching the behavior of the default and postgres branches and making the
failure visible to callers.
In `@pkg/api/job_runs.go`:
- Around line 423-446: Cache the result of GetReleasesFromDB in job_runs.go so
it can be reused instead of issuing the same query twice when compareRelease is
"Presubmits" and totalJobRuns < 20. Hoist the releases lookup out of the
narrower if block into a shared variable in the surrounding scope, then have the
prior-release lookup branch reuse that variable rather than calling
GetReleasesFromDB(ctx, dbc) again. Keep the existing logic in
findReleaseMatchJobNames and the prior-release handling unchanged, only remove
the redundant DB round-trip.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 8056bd29-51a5-4e17-84f2-a984f258a8ff
📒 Files selected for processing (13)
cmd/sippy/automatejira.gocmd/sippy/component_readiness.gocmd/sippy/serve.gopkg/api/componentreadiness/dataprovider/mixed/provider.gopkg/api/componentreadiness/dataprovider/postgres/provider.gopkg/api/job_runs.gopkg/api/releases.gopkg/api/releases_test.gopkg/api/utils.gopkg/dataloader/releasedefloader/releasedefloader_test.gopkg/mcp/tools/releases.gopkg/sippyserver/server.gotest/e2e/componentreadiness/componentreadiness_test.go
💤 Files with no reviewable changes (1)
- pkg/api/utils.go
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
mstaeble
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
Scheduling required tests: |
|
@mstaeble: The following test failed, say
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
Phase 2 of TRT-2734. Depends on #3679 (merged). Switches release data consumers from BigQuery to PostgreSQL using a new
MixedProviderthat routes release queries to PG and everything else to BQ.What changed
MixedProviderinpkg/api/componentreadiness/dataprovider/mixed/provider.gothat deterministically routes: release queries go to PG, everything else to BQ--data-providerflag default from"bigquery"to"default"acrossserve,automate-jira, andcomponent-readinesscommandsnewDataProvider()factory incmd/sippy/serve.gothat selects the provider based on available backends:MixedProviderBigQueryProviderPostgresProviderGetReleaseDatesFromDBto derive CR time ranges from PG GA datesgetReleases()in server.go tos.crDataProvider.QueryReleases(ctx)releaseMetadatamap from PG providerautomatejira, MCP releases tool, andjob_runsto useGetReleasesFromDBwith BQ fallbackGetReleases(),releaseGeneratorfromutils.go,TestTransformReleaseforceRefreshquery parameter parsing from/api/releases(was only used for BQ Redis cache)What stays unchanged
releasedates.go,provider.go) unchanged from mainQueryReleasesandQueryReleaseDatesremain on theDataProviderinterface for BQ-only deploymentsload.go) unchanged from main13 files changed, +435/-239
Test plan
go build ./...passesgo vet ./...passesmake lintpassesgo test ./pkg/... ./cmd/...passesmake e2epasses locally (108 tests, 0 failures)release_definitionstable is populated (36 rows in prod since Phase 1 merged)/api/releases,/api/component_readiness, and/api/component_readiness/regressionsendpointsDefinitionToRelease,ReleaseRowToDefinition, nil DB guards, andGetReleaseDatesFromDBRef: TRT-2734
Summary by CodeRabbit
New Features
Bug Fixes
Tests