Skip to content

TRT-1989: model changes to prep for partitioning #3532

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
neisw wants to merge 1 commit into
base: main
Choose a base branch
from
Open

TRT-1989: model changes to prep for partitioning #3532

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 14 additions & 10 deletions cmd/sippy/seed_data.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -574,10 +574,11 @@ func seedRunsForJob(dbc *db.DB, suite *models.Suite, prowJob models.ProwJob, jrK
for i := range totalRuns {
timestamp := start.Add(time.Duration(i) * interval)
run := models.ProwJobRun{
ProwJobID: prowJob.ID,
Cluster: "build01",
Timestamp: timestamp,
Duration: 3 * time.Hour,
ProwJobID: prowJob.ID,
ProwJobRelease: prowJob.Release,
Cluster: "build01",
Timestamp: timestamp,
Duration: 3 * time.Hour,
}
if err := dbc.DB.Create(&run).Error; err != nil {
return 0, 0, fmt.Errorf("failed to create ProwJobRun: %w", err)
Expand Down Expand Up @@ -616,12 +617,15 @@ func seedRunsForJob(dbc *db.DB, suite *models.Suite, prowJob models.ProwJob, jrK
}

result := models.ProwJobRunTest{
ProwJobRunID: runIDs[i],
TestID: testID,
SuiteID: &suite.ID,
Status: status,
Duration: 5.0,
CreatedAt: start.Add(time.Duration(i) * interval),
ProwJobRunID: runIDs[i],
ProwJobID: prowJob.ID,
ProwJobRunRelease: prowJob.Release,
ProwJobRunTimestamp: start.Add(time.Duration(i) * interval),
TestID: testID,
SuiteID: &suite.ID,
Status: status,
Duration: 5.0,
CreatedAt: start.Add(time.Duration(i) * interval),
}
if err := dbc.DB.Create(&result).Error; err != nil {
return 0, 0, fmt.Errorf("failed to create ProwJobRunTest: %w", err)
Expand Down
9 changes: 6 additions & 3 deletions pkg/api/job_runs_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,9 +220,11 @@ func TestRunJobAnalysis(t *testing.T) {

for _, t := range tests {
fakeProwJobRun.Tests = append(fakeProwJobRun.Tests, models.ProwJobRunTest{
Test: models.Test{Name: t.Name},
Suite: models.Suite{Name: t.SuiteName},
Status: 12,
ProwJobID: fakeProwJobRun.ProwJobID,
ProwJobRunRelease: fakeProwJobRun.ProwJobRelease,
Test: models.Test{Name: t.Name},
Suite: models.Suite{Name: t.SuiteName},
Status: 12,
})
}

Expand Down Expand Up @@ -289,6 +291,7 @@ func buildFakeProwJobRun() *models.ProwJobRun {
},
},
ProwJobID: 1000000000,
ProwJobRelease: "4.12",
URL: "https://example.com/run/1000000000",
Tests: []models.ProwJobRunTest{}, // will be populated in the test cases
TestCount: 5,
Expand Down
46 changes: 26 additions & 20 deletions pkg/dataloader/prowloader/prow.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -930,7 +930,7 @@ func (pl *ProwLoader) createOrUpdateProwJob(ctx context.Context, pj *prow.ProwJo
}

func (pl *ProwLoader) processGCSBucketJobRun(ctx context.Context, pj *prow.ProwJob, id uint64, path string, junitMatches []string, dbProwJob *models.ProwJob) error {
tests, failures, overallResult, err := pl.prowJobRunTestsFromGCS(ctx, pj, uint(id), path, junitMatches)
tests, failures, overallResult, err := pl.prowJobRunTestsFromGCS(ctx, pj, uint(id), dbProwJob.ID, dbProwJob.Release, path, junitMatches)
if err != nil {
return err
}
Expand Down Expand Up @@ -959,19 +959,20 @@ func (pl *ProwLoader) processGCSBucketJobRun(ctx context.Context, pj *prow.ProwJ
Model: gorm.Model{
ID: uint(id),
},
Cluster: pj.Spec.Cluster,
Duration: duration,
ProwJob: *dbProwJob,
ProwJobID: dbProwJob.ID,
URL: pj.Status.URL,
GCSBucket: pj.Spec.DecorationConfig.GCSConfiguration.Bucket,
Timestamp: pj.Status.StartTime,
OverallResult: overallResult,
PullRequests: pulls,
TestFailures: failures,
Succeeded: overallResult == sippyprocessingv1.JobSucceeded,
Labels: labels,
Annotations: annotations,
Cluster: pj.Spec.Cluster,
Duration: duration,
ProwJob: *dbProwJob,
ProwJobID: dbProwJob.ID,
ProwJobRelease: dbProwJob.Release,
URL: pj.Status.URL,
GCSBucket: pj.Spec.DecorationConfig.GCSConfiguration.Bucket,
Timestamp: pj.Status.StartTime,
OverallResult: overallResult,
PullRequests: pulls,
TestFailures: failures,
Succeeded: overallResult == sippyprocessingv1.JobSucceeded,
Labels: labels,
Annotations: annotations,
}).Error
if err != nil {
return err
Expand Down Expand Up @@ -1187,7 +1188,7 @@ func (pl *ProwLoader) findSuite(name string) *uint {
return id
}

func (pl *ProwLoader) prowJobRunTestsFromGCS(ctx context.Context, pj *prow.ProwJob, id uint, path string, junitPaths []string) ([]*models.ProwJobRunTest, int, sippyprocessingv1.JobOverallResult, error) {
func (pl *ProwLoader) prowJobRunTestsFromGCS(ctx context.Context, pj *prow.ProwJob, id, prowJobID uint, prowJobRelease, path string, junitPaths []string) ([]*models.ProwJobRunTest, int, sippyprocessingv1.JobOverallResult, error) {
failures := 0

bkt := pl.gcsClient.Bucket(pj.Spec.DecorationConfig.GCSConfiguration.Bucket)
Expand All @@ -1206,7 +1207,7 @@ func (pl *ProwLoader) prowJobRunTestsFromGCS(ctx context.Context, pj *prow.ProwJ
continue
}

pl.extractTestCases(suite, suiteID, testCases)
pl.extractTestCases(suite, suiteID, testCases, prowJobRelease, pj.Status.StartTime)
}

syntheticSuite, jobResult := testconversion.ConvertProwJobRunToSyntheticTests(*pj, testCases, pl.syntheticTestManager)
Expand All @@ -1216,7 +1217,7 @@ func (pl *ProwLoader) prowJobRunTestsFromGCS(ctx context.Context, pj *prow.ProwJ
// this shouldn't happen but if it does we want to know
panic("synthetic suite is missing from the database")
}
pl.extractTestCases(syntheticSuite, suiteID, testCases)
pl.extractTestCases(syntheticSuite, suiteID, testCases, prowJobRelease, pj.Status.StartTime)
log.Infof("synthetic suite had %d tests", syntheticSuite.NumTests)

results := make([]*models.ProwJobRunTest, 0)
Expand All @@ -1226,6 +1227,9 @@ func (pl *ProwLoader) prowJobRunTestsFromGCS(ctx context.Context, pj *prow.ProwJ
}

testCases[k].ProwJobRunID = id
testCases[k].ProwJobID = prowJobID
testCases[k].ProwJobRunRelease = prowJobRelease
testCases[k].ProwJobRunTimestamp = pj.Status.StartTime
results = append(results, testCases[k])
if testCases[k].Status == 12 {
failures++
Expand All @@ -1235,7 +1239,7 @@ func (pl *ProwLoader) prowJobRunTestsFromGCS(ctx context.Context, pj *prow.ProwJ
return results, failures, jobResult, nil
}

func (pl *ProwLoader) extractTestCases(suite *junit.TestSuite, suiteID *uint, testCases map[string]*models.ProwJobRunTest) {
func (pl *ProwLoader) extractTestCases(suite *junit.TestSuite, suiteID *uint, testCases map[string]*models.ProwJobRunTest, prowJobRelease string, prowJobStartTime time.Time) {

for _, tc := range suite.TestCases {
if testidentification.IsIgnoredTest(tc.Name) {
Expand All @@ -1250,7 +1254,9 @@ func (pl *ProwLoader) extractTestCases(suite *junit.TestSuite, suiteID *uint, te
status = sippyprocessingv1.TestStatusSuccess
default:
failureOutput = &models.ProwJobRunTestOutput{
Output: tc.FailureOutput.Output,
Output: tc.FailureOutput.Output,
ProwJobRunTestTimestamp: prowJobStartTime,
ProwJobRunTestRelease: prowJobRelease,
}
}

Expand Down Expand Up @@ -1283,6 +1289,6 @@ func (pl *ProwLoader) extractTestCases(suite *junit.TestSuite, suiteID *uint, te
}

for _, c := range suite.Children {
pl.extractTestCases(c, suiteID, testCases)
pl.extractTestCases(c, suiteID, testCases, prowJobRelease, prowJobStartTime)
}
}
17 changes: 15 additions & 2 deletions pkg/db/models/prow.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@ type ProwJobRun struct {
// ProwJob is a link to the prow job this run belongs to.
ProwJob ProwJob
ProwJobID uint `gorm:"index"`
// Used for partitioning
ProwJobRelease string `gorm:"varchar(10)"`

// Cluster is the cluster where the prow job was run.
Cluster string
Expand Down Expand Up @@ -87,8 +89,15 @@ type ProwJobRunTest struct {
gorm.Model
ProwJobRunID uint `gorm:"index"`
ProwJobRun ProwJobRun
TestID uint `gorm:"index;index:idx_prow_job_run_tests_test_id_status"`
Test Test
// used for variants
// skips joining on ProwJobRunID just to get ProwJobID
ProwJobID uint
// used for partitioning
ProwJobRunTimestamp time.Time `gorm:"expression:DATE(timestamp AT TIME ZONE 'UTC')"`
// used for partitioning
ProwJobRunRelease string `gorm:"varchar(10)"`
TestID uint `gorm:"index;index:idx_prow_job_run_tests_test_id_status"`
Comment on lines +92 to +99
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot May 15, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Index the new denormalized lookup key.

ProwJobID is added to avoid a join, but without an index this can still force full scans on prow_job_run_tests under real data volume.

💡 Suggested fix 
-	ProwJobID uint
+	ProwJobID uint `gorm:"index"`
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// used for variants
// skips joining on ProwJobRunID just to get ProwJobID
ProwJobID uint
// used for partitioning
ProwJobRunTimestamp time.Time `gorm:"expression:DATE(timestamp AT TIME ZONE 'UTC')"`
// used for partitioning
ProwJobRunRelease string `gorm:"varchar(10)"`
TestID uint `gorm:"index;index:idx_prow_job_run_tests_test_id_status"`
// used for variants
// skips joining on ProwJobRunID just to get ProwJobID
ProwJobID uint `gorm:"index"`
// used for partitioning
ProwJobRunTimestamp time.Time `gorm:"expression:DATE(timestamp AT TIME ZONE 'UTC')"`
// used for partitioning
ProwJobRunRelease string `gorm:"varchar(10)"`
TestID uint `gorm:"index;index:idx_prow_job_run_tests_test_id_status"`
🤖 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/models/prow.go` around lines 92 - 99, ProwJobID was added as a
denormalized lookup key but lacks an index; modify the struct field declaration
for ProwJobID (the ProwJobID field in the prow job run tests model) to add a
GORM index tag (e.g., include `gorm:"index"` or a named index) so queries
filtering by ProwJobID use an index rather than causing full table scans.

Copy link
Copy Markdown
Contributor Author

@neisw neisw May 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai, we are only staging the new columns for now and beginning data collection. We will be migrating the tables and backfilling the new columns at a later time. Indexing will be updated then.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot May 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@neisw, understood — thanks for the context! A phased approach (stage columns → collect data → migrate/backfill → add indexes) makes sense here. Would you like me to open a follow-up GitHub issue to track adding the index on ProwJobID (and any other partitioning-related indexes) once the migration and backfill are complete?

✏️ Learnings added 
Learnt from: neisw
Repo: openshift/sippy PR: 3532
File: pkg/db/models/prow.go:92-99
Timestamp: 2026-05-15T13:46:44.315Z
Learning: In `pkg/db/models/prow.go`, the `ProwJobID`, `ProwJobRunTimestamp`, and `ProwJobRunRelease` fields added to `ProwJobRunTest`, as well as `ProwJobRelease` on `ProwJobRun` and the timestamp/release fields on `ProwJobRunTestOutput`, are being staged in a phased approach: columns are added first for data collection, with table migration, backfill, and index creation deferred to a later PR. Do not flag missing indexes on these new columns as blocking issues.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

Test Test
// SuiteID may be nil if no suite name could be parsed from the testgrid test name.
SuiteID *uint `gorm:"index"`
Suite Suite
Expand All @@ -107,6 +116,10 @@ type ProwJobRunTestOutput struct {
ProwJobRunTestID uint `gorm:"index"`
// Output stores the output of a ProwJobRunTest.
Output string
// used for partitioning
ProwJobRunTestTimestamp time.Time `gorm:"expression:DATE(timestamp AT TIME ZONE 'UTC')"`
// used for partitioning
ProwJobRunTestRelease string `gorm:"varchar(10)"`
}

// Suite defines a junit testsuite. Used to differentiate the same test being run in different suites in ProwJobRunTest.
Expand Down
10 changes: 6 additions & 4 deletions pkg/sippyserver/pr_new_tests_worker_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,10 +233,11 @@ func TestUnit_getNewTestsForJobRun(t *testing.T) {
name: "successful fetch",
fetchJobRun: func(dbc *db.DB, jobRunID int64, unknownTests bool, preloads []string, logger *logrus.Entry) (*models.ProwJobRun, error) {
pjr := models.ProwJobRun{
ProwJobRelease: "4.12",
Tests: []models.ProwJobRunTest{
{Test: models.Test{Name: "test1"}, Status: int(v1.TestStatusSuccess)},
{Test: models.Test{Name: "test2"}, Status: int(v1.TestStatusFailure)},
{Test: models.Test{Name: "test3"}, Status: int(v1.TestStatusFlake)},
{ProwJobID: 1, ProwJobRunRelease: "4.12", Test: models.Test{Name: "test1"}, Status: int(v1.TestStatusSuccess)},
{ProwJobID: 1, ProwJobRunRelease: "4.12", Test: models.Test{Name: "test2"}, Status: int(v1.TestStatusFailure)},
{ProwJobID: 1, ProwJobRunRelease: "4.12", Test: models.Test{Name: "test3"}, Status: int(v1.TestStatusFlake)},
},
}
pjr.ID = 12345 // Gorm model ID for some reason can't be put in the struct literal
Expand All @@ -252,8 +253,9 @@ func TestUnit_getNewTestsForJobRun(t *testing.T) {
name: "error on filtering",
fetchJobRun: func(dbc *db.DB, jobRunID int64, unknownTests bool, preloads []string, logger *logrus.Entry) (*models.ProwJobRun, error) {
pjr := models.ProwJobRun{
ProwJobRelease: "4.12",
Tests: []models.ProwJobRunTest{
{Test: models.Test{Name: "test1"}, Status: int(v1.TestStatusSuccess)},
{ProwJobID: 1, ProwJobRunRelease: "4.12", Test: models.Test{Name: "test1"}, Status: int(v1.TestStatusSuccess)},
},
}
pjr.ID = 12345 // Gorm model ID for some reason can't be put in the struct literal
Expand Down