NO-JIRA: check for dynamic suites#3478
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@neisw: This pull request explicitly references no jira issue. 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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughDelegate suite name resolution to a new exported GetSuiteID that applies static list and regex pattern checks and centralize DB upsert in getOrCreateSuite; prow loader now caches and returns GetSuiteID results; add unit tests validating dynamic regex suite matching. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ProwLoader as Prow Loader
participant DB as Database
participant Pattern as Pattern Matcher
Client->>ProwLoader: request suite by name
ProwLoader->>ProwLoader: check cache
alt cache hit
ProwLoader-->>Client: return cached suite ID
else cache miss
ProwLoader->>DB: GetSuiteID(name)
DB->>Pattern: evaluate name against static list + regex patterns
alt pattern matches
Pattern-->>DB: match found
DB->>DB: getOrCreateSuite (FirstOrCreate / First fallback)
DB-->>ProwLoader: suite ID (uint)
ProwLoader->>ProwLoader: cache suite ID
ProwLoader-->>Client: return suite ID
else no match
Pattern-->>DB: no match
DB-->>ProwLoader: nil
ProwLoader->>ProwLoader: cache nil result
ProwLoader-->>Client: no suite found
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 16 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (16 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/dataloader/prowloader/prow.go`:
- Around line 1187-1191: The code is caching the result of
db.CheckForDynamicSuite into pl.suiteCache even when it returns nil, making
transient DB/create failures become permanent misses; update the loader to only
assign pl.suiteCache[name] = id when id != nil (i.e., after calling
CheckForDynamicSuite) so nil results aren't cached, or alternatively change
CheckForDynamicSuite to return a distinguishable outcome (e.g., (id, error) or a
sentinel) and only cache explicit "no-match" negatives while not caching on
creation/DB errors; adjust callers (the code around CheckForDynamicSuite and
pl.suiteCache usage) accordingly.
🪄 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: 86f2f674-058f-4540-87d0-075ce06b27d8
📒 Files selected for processing (3)
pkg/dataloader/prowloader/prow.gopkg/db/suites.gopkg/db/suites_test.go
|
@coderabbitai[bot]: changing LGTM is restricted to collaborators 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 kubernetes-sigs/prow repository. |
|
Scheduling required tests: |
etirta
left a comment
etirta
left a comment
There was a problem hiding this comment.
Hi @neisw, I support moving forward with this PR, as I agree with exposing the API in pkg/db/suites.go to handle the search algorithm.
However, I have a few suggestions to reduce the footprint and ensure the entire search algorithm is fully encapsulated within the pkg/db/suites.go module.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/db/suites.go`:
- Around line 122-129: getOrCreateSuite uses db.Where(...).FirstOrCreate(&suite)
which is race-prone and currently returns nil on any error; update the error
handling to detect unique-constraint/race errors, and on FirstOrCreate failure
perform a fallback SELECT (e.g., db.Where("name = ?", name).First(&suite)) to
retrieve the row created by the concurrent caller before giving up; ensure the
subsequent validation/logging that examines suite.ID (the logic after the
upsert) uses the suite loaded by the fallback SELECT and logs the actual error
only if the fallback also fails. Refer to getOrCreateSuite, models.Suite,
db.Where(...).FirstOrCreate, result.Error and the validation/logging block that
follows the upsert.
- Around line 85-100: GetSuiteID currently returns nil for both "not importable"
and DB errors causing callers like findSuite to cache false negatives; change
GetSuiteID signature to return (*uint, error), have it return (nil, nil) when
!isSuiteImportable(name) and propagate any DB error from getOrCreateSuite as
(nil, err), update getOrCreateSuite to return (*uint, error) if it doesn't
already, and adjust callers (e.g., findSuite and the loader caching logic) to
only cache a nil suite ID when err == nil and to retry/propagate on non-nil
errors so transient DB failures are not treated as deliberate filtering.
🪄 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: ad722fed-d3d1-4c25-bd88-1d641ac961ca
📒 Files selected for processing (3)
pkg/dataloader/prowloader/prow.gopkg/db/suites.gopkg/db/suites_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/db/suites_test.go
- pkg/dataloader/prowloader/prow.go
|
@coderabbitai[bot]: changing LGTM is restricted to collaborators 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 kubernetes-sigs/prow repository. |
|
Scheduling required tests: |
There was a problem hiding this comment.
♻️ Duplicate comments (2)
pkg/db/suites.go (2)
122-141:⚠️ Potential issue | 🟠 Major
FirstOrCreatehandling is not concurrency-safe as written, and Line 139 log text is misleading.Line 123 claims “thread-safe upsert behavior,” but
FirstOrCreatecan still hit concurrent insert races. Current error handling at Line 127-130 returnsnilimmediately instead of recovering via a fallback read. Also, at Line 139,RowsAffected > 0corresponds to creation, not retrieval.Suggested direction
-// getOrCreateSuite finds or creates a suite by name. Returns the suite ID on success, nil on error. -// Uses FirstOrCreate for thread-safe upsert behavior. -func getOrCreateSuite(db *gorm.DB, name string) *uint { +// getOrCreateSuite finds or creates a suite by name. +// Returns suite ID on success; returns an error on DB failures. +func getOrCreateSuite(db *gorm.DB, name string) (*uint, error) { suite := models.Suite{Name: name} result := db.Where("name = ?", name).FirstOrCreate(&suite) if result.Error != nil { - log.WithError(result.Error).Errorf("failed to get or create suite %q", name) - return nil + // Recover from concurrent creator race by re-reading. + if err := db.Where("name = ?", name).First(&suite).Error; err != nil { + log.WithError(result.Error).Errorf("failed to get or create suite %q", name) + return nil, errors.Wrapf(result.Error, "failed to get or create suite %q", name) + } } if suite.ID == 0 { log.Errorf("suite %q has invalid ID 0", name) - return nil + return nil, errors.Errorf("suite %q has invalid ID 0", name) } - if result.RowsAffected > 0 { - log.WithField("suite", name).Info("retrieved test suite") + if result.RowsAffected > 0 { + log.WithField("suite", name).Info("created test suite") + } else { + log.WithField("suite", name).Debug("retrieved test suite") } id := suite.ID - return &id + return &id, nil }For gorm.io/gorm v1.22.x, what are the documented semantics of FirstOrCreate and RowsAffected, and what is the recommended pattern to handle concurrent unique-key insert races?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/db/suites.go` around lines 122 - 141, getOrCreateSuite currently returns nil on any FirstOrCreate error and misinterprets RowsAffected; change it to, on result.Error, detect a unique-key/duplicate-insert race (e.g., inspect result.Error for duplicate key or attempt a fallback query) and if so do a fallback db.Where("name = ?", name).First(&suite) to load the existing row before returning; only return nil if both FirstOrCreate and the fallback read fail. Also fix the RowsAffected logging in getOrCreateSuite: treat RowsAffected > 0 as "created test suite" and otherwise log "retrieved test suite", validate suite.ID > 0 and return &suite.ID (not nil) on success. Ensure you reference the getOrCreateSuite function, the db.Where(...).FirstOrCreate(&suite) call, and the fallback db.Where(...).First(&suite) read when implementing this change.
85-100:⚠️ Potential issue | 🟠 MajorSeparate “not importable” from “DB failure” in
GetSuiteID.At Line 87 and Line 94-99,
nilcurrently means both “filtered out” and “lookup/create failed.” WithfindSuitecachingnilresults inpkg/dataloader/prowloader/prow.go(Line 1170-1187 in provided context), transient DB failures get cached as permanent skips for that loader instance.Suggested direction
-func GetSuiteID(db *gorm.DB, name string) *uint { +func GetSuiteID(db *gorm.DB, name string) (*uint, error) { if name == "" { - return nil + return nil, nil } if !isSuiteImportable(name) { - return nil + return nil, nil } - return getOrCreateSuite(db, name) + return getOrCreateSuite(db, name) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/db/suites.go` around lines 85 - 100, GetSuiteID currently returns nil for both "suite not importable" and "DB failure", causing transient DB errors to be treated as permanent skips; change GetSuiteID(name string) to return a distinct result and error (e.g., (*uint, error) or (uint, bool, error)) so "not importable" is signaled without error and DB failures return a non-nil error; keep the isSuiteImportable check first, have getOrCreateSuite propagate DB errors instead of swallowing them, and update callers (notably findSuite / pkg/dataloader/prowloader/prow.go) to treat a non-nil error as transient and avoid caching the skip.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/db/suites.go`:
- Around line 122-141: getOrCreateSuite currently returns nil on any
FirstOrCreate error and misinterprets RowsAffected; change it to, on
result.Error, detect a unique-key/duplicate-insert race (e.g., inspect
result.Error for duplicate key or attempt a fallback query) and if so do a
fallback db.Where("name = ?", name).First(&suite) to load the existing row
before returning; only return nil if both FirstOrCreate and the fallback read
fail. Also fix the RowsAffected logging in getOrCreateSuite: treat RowsAffected
> 0 as "created test suite" and otherwise log "retrieved test suite", validate
suite.ID > 0 and return &suite.ID (not nil) on success. Ensure you reference the
getOrCreateSuite function, the db.Where(...).FirstOrCreate(&suite) call, and the
fallback db.Where(...).First(&suite) read when implementing this change.
- Around line 85-100: GetSuiteID currently returns nil for both "suite not
importable" and "DB failure", causing transient DB errors to be treated as
permanent skips; change GetSuiteID(name string) to return a distinct result and
error (e.g., (*uint, error) or (uint, bool, error)) so "not importable" is
signaled without error and DB failures return a non-nil error; keep the
isSuiteImportable check first, have getOrCreateSuite propagate DB errors instead
of swallowing them, and update callers (notably findSuite /
pkg/dataloader/prowloader/prow.go) to treat a non-nil error as transient and
avoid caching the skip.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: c66e491d-a537-46c9-8338-b21c2828707b
📒 Files selected for processing (1)
pkg/db/suites.go
|
Scheduling required tests: |
etirta
left a comment
etirta
left a comment
There was a problem hiding this comment.
Rather than having another PR, let's define what we plan for all of our teams. Thx.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/db/suites.go (1)
106-122: Consider using a set for O(1) lookup on testSuites.The current linear scan through
testSuitesis fine for ~50 items, but if this list grows orisSuiteImportableis called frequently in hot paths, amap[string]struct{}initialized once would provide O(1) lookups.♻️ Optional optimization
var testSuitesSet = func() map[string]struct{} { set := make(map[string]struct{}, len(testSuites)) for _, s := range testSuites { set[s] = struct{}{} } return set }() func isSuiteImportable(name string) bool { if _, ok := testSuitesSet[name]; ok { return true } for _, re := range testSuitePatterns { if re.MatchString(name) { return true } } return false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/db/suites.go` around lines 106 - 122, Replace the linear scan over testSuites in isSuiteImportable with an O(1) lookup by creating a package-level set (map[string]struct{}) initialized once from testSuites (e.g., testSuitesSet) and then check membership via an existence check; keep the subsequent loop over testSuitePatterns and return true on either match, otherwise false.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/db/suites.go`:
- Around line 129-139: The fallback read branch after a failed FirstOrCreate
currently always logs the original error and returns nil even when the fallback
read succeeds; update the logic in pkg/db/suites.go (the block handling
result.Error from FirstOrCreate) so that if the fallback read (read :=
db.Where("name = ?", name).First(&suite)) has no error you validate/return the
found suite (or its ID) instead of falling through to the error return; only
call log.WithError(result.Error).Errorf(...) and return nil when read.Error !=
nil, otherwise proceed to return the successful suite (use the existing suite
variable and its ID/result as the normal success path).
---
Nitpick comments:
In `@pkg/db/suites.go`:
- Around line 106-122: Replace the linear scan over testSuites in
isSuiteImportable with an O(1) lookup by creating a package-level set
(map[string]struct{}) initialized once from testSuites (e.g., testSuitesSet) and
then check membership via an existence check; keep the subsequent loop over
testSuitePatterns and return true on either match, otherwise false.
🪄 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: 41385948-6309-4e3e-9669-d7ef47541501
📒 Files selected for processing (1)
pkg/db/suites.go
|
@coderabbitai[bot]: changing LGTM is restricted to collaborators 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 kubernetes-sigs/prow repository. |
|
Scheduling required tests: |
|
Scheduling required tests: |
|
Scheduling required tests: |
|
@neisw: 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. |
openshift-merge-bot
Bot
added
the
ready-for-human-review
|
@coderabbitai[bot]: changing LGTM is restricted to collaborators 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 kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: coderabbitai[bot], 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 |
4 participants
Derivative work of #3456
Summary by CodeRabbit
New Features
Bug Fixes
Tests