chore(migration): generic store for migrations by dashrews78 · Pull Request #7382 · stackrox/stackrox

dashrews78 · 2023-08-10T14:59:17Z

Description

This is a take on a generic store for migrations. The tooling from the make bootstrap_migration target was updated to take in a list of storage protos. For each storage proto it will create a gen.go that will create a store gogen -run pg-table-bindings is executed. The store being generic will allow for the previous data to be retrieved as those functions simply return the protos (or IDs) generically. The generated store will contain the insert logic for the latest proto and as such will insert data in the form the current release understands. See the testing section below to look at an example.

I made a copy for the migration to provide a level of isolation. If we decide we can simply use the generic store in pkg/search/postgres I would recommend we do that under additional tech debt tickets. Since currently we cannot use a copy of a store for a migration easily due to the move to the generic store. Similarly if we decide we want to handle sac in migrations that would be another issue to handle as tech debt. I simply removed all the SAC checks in the copy of the store for the migrator.

This also includes any necessary changes for ROX-17423, ROX-18778.

Checklist

Investigated and inspected CI test results
Unit test and regression tests added
~~- [ ] Evaluated and added CHANGELOG entry if required~~
~~- [ ] Determined and documented upgrade steps~~
~~- [ ] Documented user facing changes (create PR based on [openshift/openshift-docs]~~(https://github.com/openshift/openshift-docs) and merge into rhacs-docs)

If any of these don't apply, please comment below.

Testing Performed

Created #7387 where I created m_188_to_m_189_test_generic_example to demonstrate the use of the generic store and executed the migration test. (Ignore the m_187_to_m_188_add_nftables_to_policy as that was my first example attempt)

ghost · 2023-08-11T16:27:24Z

Images are ready for the commit at afd4fd9.

To use with deploy scripts, first export MAIN_IMAGE_TAG=4.1.x-840-gafd4fd9291.

rhybrillou

At first glance, the changes seem to introduce a copy of the generic store with the following changes:

calls to metrics monitoring are removed
Scoped access control checks are removed

Technically, the generated metrics functions could be replaced by an empty pass-through function.
On the Scoped access control side, I'd rather be in favour of using sac.WithAllAccess(ctx) in the migrations. That would leave room for some foreseeable changes to the way stores do work (the SAC checks could be moved from the generic stores further down to the SQL query generator).

dashrews78 · 2023-08-14T10:47:14Z

At first glance, the changes seem to introduce a copy of the generic store with the following changes:

calls to metrics monitoring are removed

Scoped access control checks are removed

Technically, the generated metrics functions could be replaced by an empty pass-through function. On the Scoped access control side, I'd rather be in favour of using sac.WithAllAccess(ctx) in the migrations. That would leave room for some foreseeable changes to the way stores do work (the SAC checks could be moved from the generic stores further down to the SQL query generator).

The idea with the copy was to strip out stuff the migrations didn't need while also isolating the migrations from changes to the generic store so that we would be more intentional about what was included. Regarding SAC, the migrations do run with sac.WithAllAccess(ctx) passed through the caller to the functions, so that should be fine. I'm going to spend some time cleaning this up next couple days. Any additional thoughts you may have would be appreciated.

rhybrillou

Thank you for this great piece of work.

I really like the idea of auto-generating the store files for migrations. This will definitely improve the developer experience when writing data migrations.
I think the migration management experience improvement could be achieved with less code duplication (details as to why and how follow).

The recent changes in pkg/search/postgres/store.go move the access control checks of the read and delete functions down to pkg/search/postgres/common.go and pkg/search/postgres/common.go.

After this, the remaining changes between the migrator/migrations/postgreshelper/store.go file and pkg/search/postgres/store.go are:

removal of the metrics collection logic (in store struct, constructor and internal functions)
removal of the operation type from the acquireConn function and function calls
removal of the access-control related elements and logic (in store struct, constructor as well as helper and Upsert* functions)
small differences to the query passed down to the functions from pkg/search/postgres/common.go (search.ConjunctionQuery not removed where only one argument remains or nil vs search.EmptyQuery())

The migration_store.go.tpl file in tools/generate-helpers/pg-table-bindings also has few changes compared to the store.go.tpl file:

removal of the metrics collection functions
removal of the access-control related elements

The time metrics collection functions could be replaced with empty pass-through functions in the generated code in the case of migration.

The access control checks will be there anyway for Walk, Count and Get* functions, and will therefore require the migrations to use contexts with elevated permissions. The elevated-permission context could be crafted in a way that the access control checks in the Upsert* functions would pass anyway.

I think using the standard store.go.tpl template with minor adjustments covering the migration-related cases would significantly reduce the need for code duplication here.

rhybrillou · 2023-08-24T14:36:03Z

migrator/README.md

-   to the `CurrentDBVersionSeqNum` of the release immediately following the release that cannot tolerate the change. 
-   For example, in 4.2 a column `column_v2` is added to replace the `column_v1` column in 4.1.  All the code from 4.2
+2. If using the optional `STORE_OBJECT` parameter, generate the migration stores with the following command:
+    `gogen -run pg-table-bindings`


Should the command be run in the migration directory, in the directory where the gen.go file was generated, or in the directory where the code was cloned ? In case it is the clone directory, are there command line options to specify the directory in which the gen.go file is ?

rhybrillou · 2023-09-06T11:44:56Z

migrator/migrations/postgreshelper/store.go

+
+// Exists tells whether the ID exists in the store.
+func (s *GenericStore[T, PT]) Exists(ctx context.Context, id string) (bool, error) {
+	q := search.ConjunctionQuery(


I think the search.ConjunctionQuery wrapper here is not needed anymore.

rhybrillou · 2023-09-06T11:46:36Z

migrator/migrations/postgreshelper/store.go

+func (s *GenericStore[T, PT]) Count(ctx context.Context) (int, error) {
+	var query *v1.Query
+
+	return searcher.RunCountRequestForSchema(ctx, s.schema, query, s.db)


nit: query here could be replaced by search.EmptyQuery().

rhybrillou · 2023-09-06T12:11:07Z

migrator/migrations/postgreshelper/store.go

+
+// Get returns the object, if it exists from the store.
+func (s *GenericStore[T, PT]) Get(ctx context.Context, id string) (PT, bool, error) {
+	q := search.ConjunctionQuery(


nit: no need to wrap with search.ConjunctionQuery

rhybrillou · 2023-09-06T12:16:35Z

migrator/migrations/postgreshelper/store.go

+	q := search.ConjunctionQuery(
+		query,
+	)
+	q.Pagination = pagination


nit: no need for the overhead of using ConjunctionQuery and propagating pagination information when any.

rhybrillou · 2023-09-06T15:49:17Z

tools/generate-helpers/bootstrap-migration/migration_impl.go.tpl

-//  - remove any unused function from the copied store files (the minimum for the public API should contain Walk, UpsertMany, DeleteMany)
-//  - remove the scoped access control code from the copied store files
-//  - remove the metrics collection code from the copied store files
+//  - remove the gen.go file generated in ../{OBJECT}/store


(repeat for all types impacted by the migration).

rhybrillou · 2023-09-07T08:36:18Z

tools/generate-helpers/pg-table-bindings/main.go

-				SingletonStore:  props.SingletonStore,
-				BatchSize:       props.MigrationBatchSize,
-			}
+			if props.MigrateSeq != 0 {


Although not directly related to the change at hand, I think it the generation of the stores for the initial migration to postgres could be removed from this generator.

rhybrillou · 2023-09-07T09:00:38Z

tools/generate-helpers/pg-table-bindings/migration_store.go.tpl

+
+// New returns a new Store instance using the provided sql instance.
+func New(db postgres.DB) Store {
+    return postgreshelper.NewGenericStore{{ if .PermissionChecker }}WithPermissionChecker{{ end }}[storeType, *storeType](


The postgreshelper package has no WithPermissionChecker variant of the constructor, which would be the same as the base constructor once the access-control related elements are removed.

dashrews78 · 2023-09-07T11:12:32Z

That's great info. Thanks. I'm HOPING I can get back to this and finish it in the near future. Especially since the SAC changes were moved. Should simplify things.

openshift-ci · 2023-12-12T00:31:47Z

@dashrews78: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/ocp-4-13-ebpf-qa-e2e-tests	`afd4fd9`	link	false	`/test ocp-4-13-ebpf-qa-e2e-tests`
ci/prow/shell-unit-tests	`afd4fd9`	link	true	`/test shell-unit-tests`

Full PR test history. Your PR dashboard.

Details

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/test-infra repository. I understand the commands that are listed here.

dashrews78 requested a review from a team as a code owner August 10, 2023 14:59

github-actions bot added the area/postgres label Aug 10, 2023

dashrews78 mentioned this pull request Aug 10, 2023

chore(migration): migration generic store sample implementation [WIP DO_NOT_MERGE] #7387

Closed

5 tasks

rhybrillou reviewed Aug 14, 2023

View reviewed changes

dashrews78 changed the title ~~chore(migration): generic store for migrations [DO NOT MERGE--WIP]~~ chore(migration): generic store for migrations Aug 15, 2023

dashrews78 requested review from c-du, connorgorman and theencee August 15, 2023 19:32

dashrews78 added 10 commits August 17, 2023 13:14

X-Smart-Branch-Parent: master

9ddc059

generic migration store

c3ef5b0

gen a migration store

0ee4f25

clean up ops

d0442ae

some more bootstrap work

8bdbe75

style

be7cb77

style

8f8e22c

adjustment

02c3c78

some adjustments

d504f5e

style

afd4fd9

dashrews78 force-pushed the dashrews/migration-generic-store branch from 93a3093 to afd4fd9 Compare August 17, 2023 17:14

dashrews78 added do-not-merge/work-in-progress and removed do-not-merge/work-in-progress labels Aug 22, 2023

dashrews78 marked this pull request as draft August 22, 2023 09:49

openshift-ci bot added the do-not-merge/work-in-progress label Aug 22, 2023

rhybrillou reviewed Sep 7, 2023

View reviewed changes

dashrews78 closed this Dec 20, 2023

dashrews78 deleted the dashrews/migration-generic-store branch September 10, 2025 15:46

Conversation

dashrews78 commented Aug 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Testing Performed

Uh oh!

ghost commented Aug 11, 2023 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rhybrillou left a comment

Choose a reason for hiding this comment

Uh oh!

dashrews78 commented Aug 14, 2023

Uh oh!

rhybrillou left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dashrews78 commented Sep 7, 2023

Uh oh!

openshift-ci bot commented Dec 12, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dashrews78 commented Aug 10, 2023 •

edited

Loading

ghost commented Aug 11, 2023 •

edited by ghost

Loading