TRT-1989: add go_partman#3534
Open
neisw wants to merge 5 commits into
Open
Conversation
Contributor
|
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 references TRT-1989 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. |
Contributor
WalkthroughAdds a PartitionManager wrapper around go_partman, integrates it into ProwLoader (conditional Maintain and EnsurePartition), initializes/starts maintenance in load/serve commands, updates module dependencies, and adds an end-to-end partition manager test. ChangesPartition Manager Integration
Sequence DiagramsequenceDiagram
participant LoadCmd as cmd/sippy/load
participant ServeCmd as cmd/sippy/serve
participant ProwLoader as pkg/dataloader/prowloader.ProwLoader
participant PartMgr as pkg/db/partitionmanager.PartitionManager
participant Postgres as Postgres (gorm.DB)
LoadCmd->>PartMgr: NewWithDefaults(gorm.DB)
LoadCmd->>ProwLoader: New(..., partMgr)
ServeCmd->>PartMgr: NewWithDefaults(gorm.DB)
ServeCmd->>PartMgr: Start(ctx)
ProwLoader->>PartMgr: Maintain(ctx) (optional)
ProwLoader->>PartMgr: EnsurePartition(table, date, nextDay)
PartMgr->>Postgres: Execute DDL / Maintain actions
Postgres-->>PartMgr: result
PartMgr-->>ServeCmd: logs/errors (non-fatal)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 13 | ❌ 4❌ Failed checks (4 warnings)
✅ Passed checks (13 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/db/partitionmanager/manager.go`:
- Around line 44-46: The New constructor can panic by dereferencing a nil
gormDB; update New(gormDB *gorm.DB, sampleRate time.Duration, tables
[]TableConfig) to first check if gormDB == nil and return a descriptive error
before calling gormDB.DB(), then proceed to call gormDB.DB() and handle its
error as before (references: New, gormDB, sqlDB, TableConfig, PartitionManager)
so no nil pointer is dereferenced.
- Around line 99-111: EnsurePartition currently interpolates identifiers and
values into SQL; validate the table and partition identifiers and use
parameterized placeholders for dates instead. In EnsurePartition, validate that
table and partitionName (derived from dateParsed) match a safe identifier
pattern (e.g., /^[A-Za-z0-9_]+$/) before using fmt.Sprintf to insert them into
the SQL string, and then call db.Exec with parameter placeholders for the date
range values (date and nextDay) instead of embedding them directly; reference
EnsurePartition, partitionName, table, dateParsed, date and nextDay to locate
the code.
🪄 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: 9fa0a3b5-9768-493c-9f1b-fc8414d7d4e5
⛔ Files ignored due to path filters (42)
go.sumis excluded by!**/*.sum,!go.sumvendor/github.com/jackc/pgservicefile/.travis.ymlis excluded by!vendor/**,!**/vendor/**vendor/github.com/jackc/pgservicefile/README.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/jackc/pgservicefile/pgservicefile.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jirevwe/go_partman/.gitignoreis excluded by!vendor/**,!**/vendor/**vendor/github.com/jirevwe/go_partman/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/github.com/jirevwe/go_partman/README.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/jirevwe/go_partman/clock.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jirevwe/go_partman/logger.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jirevwe/go_partman/manager.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jirevwe/go_partman/queries.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jirevwe/go_partman/type.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jirevwe/go_partman/ui.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jirevwe/go_partman/web/dist/index.htmlis excluded by!**/dist/**,!vendor/**,!**/vendor/**vendor/github.com/jmoiron/sqlx/.gitignoreis excluded by!vendor/**,!**/vendor/**vendor/github.com/jmoiron/sqlx/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/github.com/jmoiron/sqlx/Makefileis excluded by!vendor/**,!**/vendor/**vendor/github.com/jmoiron/sqlx/README.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/jmoiron/sqlx/bind.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jmoiron/sqlx/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jmoiron/sqlx/named.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jmoiron/sqlx/named_context.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jmoiron/sqlx/reflectx/README.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/jmoiron/sqlx/reflectx/reflect.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jmoiron/sqlx/sqlx.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/jmoiron/sqlx/sqlx_context.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/oklog/ulid/v2/.gitignoreis excluded by!vendor/**,!**/vendor/**vendor/github.com/oklog/ulid/v2/AUTHORS.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/oklog/ulid/v2/CHANGELOG.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/oklog/ulid/v2/CONTRIBUTING.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/oklog/ulid/v2/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/github.com/oklog/ulid/v2/README.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/oklog/ulid/v2/ulid.gois excluded by!vendor/**,!**/vendor/**vendor/gopkg.in/guregu/null.v4/.gitignoreis excluded by!vendor/**,!**/vendor/**vendor/gopkg.in/guregu/null.v4/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/gopkg.in/guregu/null.v4/README.mdis excluded by!vendor/**,!**/vendor/**vendor/gopkg.in/guregu/null.v4/bool.gois excluded by!vendor/**,!**/vendor/**vendor/gopkg.in/guregu/null.v4/float.gois excluded by!vendor/**,!**/vendor/**vendor/gopkg.in/guregu/null.v4/int.gois excluded by!vendor/**,!**/vendor/**vendor/gopkg.in/guregu/null.v4/string.gois excluded by!vendor/**,!**/vendor/**vendor/gopkg.in/guregu/null.v4/time.gois excluded by!vendor/**,!**/vendor/**vendor/modules.txtis excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (5)
cmd/sippy/load.gocmd/sippy/serve.gogo.modpkg/dataloader/prowloader/prow.gopkg/db/partitionmanager/manager.go
openshift-ci
Bot
added
the
ready-for-human-review
Contributor
|
Scheduling required tests: |
Contributor
Author
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/e2e/db/migrate/migrate_test.go`:
- Around line 190-196: The cleanup currently ignores errors from dbc.DB.Exec and
drops partition child tables using quoted schema-qualified names (e.g., dropping
"%s" where input is "public.table") which fails; update all cleanup/reset blocks
that call dbc.DB.Exec (including the t.Cleanup anonymous func and the other
reset blocks) to check the returned .Error and handle or fail the test
accordingly, and change the child-table drop logic: instead of quoting the full
"schema.table" string, query pg_class/pg_namespace (or split the regclass into
schema and relname) to build DROP TABLE statements with separate schema and
table identifiers or call dbc.DB.Migrator().DropTable() with properly qualified
identifiers so partitions are actually removed and errors are surfaced.
🪄 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: e0a0168d-d44e-42d6-972a-37ced88bcfff
⛔ Files ignored due to path filters (2)
vendor/github.com/jirevwe/go_partman/SIPPY_VENDOR_PATCHES.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/jirevwe/go_partman/manager.gois excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (2)
pkg/db/partitionmanager/manager.gotest/e2e/db/migrate/migrate_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/db/partitionmanager/manager.go
Contributor
|
Scheduling required tests: |
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/e2e/db/migrate/migrate_test.go (1)
190-215:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake cleanup failures fail the test instead of just logging.
On Line 192, Line 199, Line 204, and Line 212, cleanup errors are logged but the test still passes. This can hide state leakage and cause downstream flakes.
Suggested fix
dropPartmanState := func(t *testing.T) { t.Helper() - if err := dbc.DB.Exec("DROP SCHEMA IF EXISTS partman CASCADE").Error; err != nil { - t.Logf("cleanup: drop partman schema: %v", err) - } + assert.NoError(t, dbc.DB.Exec("DROP SCHEMA IF EXISTS partman CASCADE").Error, "cleanup: drop partman schema") var childTables []string if err := dbc.DB.Raw( "SELECT c.relname FROM pg_inherits JOIN pg_class c ON c.oid = pg_inherits.inhrelid WHERE inhparent = ?::regclass", testTable, ).Scan(&childTables).Error; err != nil { - t.Logf("cleanup: query child tables: %v", err) + assert.NoError(t, err, "cleanup: query child tables") return } for _, child := range childTables { - if err := dbc.DB.Exec(fmt.Sprintf(`DROP TABLE IF EXISTS "%s"`, child)).Error; err != nil { - t.Logf("cleanup: drop child table %s: %v", child, err) - } + assert.NoError(t, dbc.DB.Exec(fmt.Sprintf(`DROP TABLE IF EXISTS "%s"`, child)).Error, "cleanup: drop child table %s", child) } } t.Cleanup(func() { dropPartmanState(t) - if err := dbc.DB.Exec(fmt.Sprintf(`DROP TABLE IF EXISTS "%s" CASCADE`, testTable)).Error; err != nil { - t.Logf("cleanup: drop test table: %v", err) - } + assert.NoError(t, dbc.DB.Exec(fmt.Sprintf(`DROP TABLE IF EXISTS "%s" CASCADE`, testTable)).Error, "cleanup: drop test table") })As per coding guidelines, "Never ignore returned Go errors as
_without clear justification. Errors should be wrapped with context..." and for tests, "Setup and cleanup—... flag tests creating resources without cleanup, especially cluster-scoped."🤖 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 `@test/e2e/db/migrate/migrate_test.go` around lines 190 - 215, The cleanup currently logs errors in dropPartmanState and the t.Cleanup block (calls to dbc.DB.Exec and dbc.DB.Raw related to dropPartmanState, the child table loop and the final DROP TABLE) which hides failures; change these t.Logf calls to fail the test (use t.Fatalf or t.Fatalf-style test failure) and include wrapped context for each error (e.g. "cleanup: drop partman schema", "cleanup: query child tables", "cleanup: drop child table <name>", "cleanup: drop test table") so any cleanup failure aborts the test; update dropPartmanState, the dbc.DB.Raw error handling, each child drop error, and the final DROP TABLE error in the t.Cleanup closure to call the failing test helper and include the original error message.
🤖 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.
Duplicate comments:
In `@test/e2e/db/migrate/migrate_test.go`:
- Around line 190-215: The cleanup currently logs errors in dropPartmanState and
the t.Cleanup block (calls to dbc.DB.Exec and dbc.DB.Raw related to
dropPartmanState, the child table loop and the final DROP TABLE) which hides
failures; change these t.Logf calls to fail the test (use t.Fatalf or
t.Fatalf-style test failure) and include wrapped context for each error (e.g.
"cleanup: drop partman schema", "cleanup: query child tables", "cleanup: drop
child table <name>", "cleanup: drop test table") so any cleanup failure aborts
the test; update dropPartmanState, the dbc.DB.Raw error handling, each child
drop error, and the final DROP TABLE error in the t.Cleanup closure to call the
failing test helper and include the original error message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: af89d561-82eb-40da-b11f-6385178ada9b
📒 Files selected for processing (1)
test/e2e/db/migrate/migrate_test.go
Contributor
|
Scheduling required tests: |
Contributor
|
@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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds go_partman for partition managment
Summary by CodeRabbit
New Features
Tests
Chores