ROX-17183: Migrate persistent data #1 - scanner definition by c-du · Pull Request #6068 · stackrox/stackrox

c-du · 2023-05-17T17:51:18Z

Description

Add:

Large objects utilities in addition to the Gorm
Add migration to move scanner definition to blobstore with test.

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 and merge into rhacs-docs)~~

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

Testing Performed

Add unit test
Run a migration test to add a scanner bundle and then upgrade and then verify.
Run a migration without scanner bundle and upgrade and then verify.

In addition to reviewing your code, reviewers must also review your testing
instructions and make sure they are sufficient.

dashrews78 · 2023-05-18T09:55:14Z

migrator/migrations/m_180_to_m_181_move_to_blobstore/migration.go

@@ -0,0 +1,124 @@
+package m179tom180


package name

yes, will fix.

dashrews78 · 2023-05-18T10:43:35Z

migrator/migrations/m_180_to_m_181_move_to_blobstore/migration_test.go

@@ -0,0 +1,98 @@
+//go:build sql_integration
+
+package m179tom180


dashrews78

Need to update the sequence number. Should update the minimum sequence number too.

Can you add some thoughts to the description on what the advantages of using gorm in this case are? It feels duplicative from what you already did with the blob store.

c-du · 2023-05-18T16:45:45Z

The discussions around why use Gorm was documented here. https://docs.google.com/document/d/1OLMylIIoPvqD0yqOdF6fJGF4kSCPznPjTse1pBjdkJE/edit#heading=h.bupciudrwmna

The scenarios are the same. Migrator needs to have backward compatibility to earlier versions while the schema and utilities evolving. The target is to have the schema stable while we still have the freedom to innovate and optimize the central without keep looking back.

Due to lack of facilities and experience, we are still copying codes of stores in addition to the schema in migrations which is cumbersome which IMHO are all feasible with Gorm. What is missing here is to have enough examples and utilities to show people how to create gorm-style migration. We only need to have a frozen schema instead of the whole store.

In this migration, large object is not supported by Gorm but it is easy to add it. This can be used in the future if any other migration may need it.

RTann

I'm not familiar with Gorm nor the blob stuff, so I just focused on the main migration file and the test

RTann · 2023-05-18T16:44:11Z

migrator/migrations/m_180_to_m_181_move_to_blobstore/migration.go

+	pgutils.CreateTableFromModel(context.Background(), db, schema.CreateTableBlobsStmt)
+
+	tx := db.Begin(&sql.TxOptions{Isolation: sql.LevelRepeatableRead})
+	if err = moveScannerDefination(tx); err != nil {


nit: moveScannerDefinitions

RTann · 2023-05-18T16:45:57Z

migrator/migrations/m_180_to_m_181_move_to_blobstore/migration.go

+	// Prepare blob
+	blob := &storage.Blob{
+		Name:         scannerDefBlobName,
+		Oid:          0,


what is this ID used for?

This oid is used to reference the large object in the database. It is filled inside the store if this is a new blob.

RTann · 2023-05-18T16:46:48Z

migrator/migrations/m_180_to_m_181_move_to_blobstore/migration.go

+	if err != nil {
+		return errors.Wrapf(err, "invalid timestamp %v", stat.ModTime())
+	}
+	fd, err := os.Open(scannerDefPath)


be sure to close the fd

should we also remove the file, nor no?

No, we do not remove the file. All the persistent data including databases will be left there for rollback.

RTann · 2023-05-18T17:03:02Z

migrator/migrations/m_180_to_m_181_move_to_blobstore/migration_test.go

+	s.Equal(scannerDefBlobName, blob.GetName())
+	s.EqualValues(size, blob.GetLength())
+
+	fileInfo, err := file.Stat()


I'm thinking we should get the mod time before calling modeToBlobs to make sure moveToBlobs does not change the time

How do we need to move the stat out to make sure it does not change time? It is the same stat operation and it does not change modification time.

From the test, we don't want to assume moveToBlobs is just going to call Stat, so that's why I thought we should check the time before we call moveToBlobs to make sure it didn't change the mod time before making the blob

dashrews78 · 2023-05-18T19:17:54Z

The discussions around why use Gorm was documented here. https://docs.google.com/document/d/1OLMylIIoPvqD0yqOdF6fJGF4kSCPznPjTse1pBjdkJE/edit#heading=h.bupciudrwmna

The scenarios are the same. Migrator needs to have backward compatibility to earlier versions while the schema and utilities evolving. The target is to have the schema stable while we still have the freedom to innovate and optimize the central without keep looking back.

Due to lack of facilities and experience, we are still copying codes of stores in addition to the schema in migrations which is cumbersome which IMHO are all feasible with Gorm. What is missing here is to have enough examples and utilities to show people how to create gorm-style migration. We only need to have a frozen schema instead of the whole store.

In this migration, large object is not supported by Gorm but it is easy to add it. This can be used in the future if any other migration may need it.

If you intend to use this as an example of a Gorm migration, it might be worth updating the README and pointing this out as an example.

RTann

Just two minor things (word misspelling and potential nil dereference). Otherwise LGTM from Scanner side

RTann · 2023-05-18T19:14:15Z

migrator/migrations/m_180_to_m_181_move_to_blobstore/migration.go

+	if os.IsNotExist(err) {
+		return nil
+	}
+	defer utils.IgnoreError(fd.Close)


Maybe move this below the error check? It may be possible fd is nil at this point (I'm not 100% sure)

connorgorman · 2023-05-19T02:04:28Z

migrator/migrations/m_180_to_m_181_move_to_blobstore/migration.go

+	if err = moveScannerDefination(tx); err != nil {
+		result := tx.Rollback()
+		if result.Error != nil {
+			return result.Error


This ends up covering up the initial error, so would be nice to wrap?

connorgorman

Mostly nits, some comments about the tx vs gorm.DB

connorgorman · 2023-05-19T02:04:53Z

migrator/migrations/m_180_to_m_181_move_to_blobstore/migration.go

+	return tx.Commit().Error
+}
+
+func moveScannerDefination(tx *gorm.DB) error {


nit: defination -> definition?

connorgorman · 2023-05-19T02:06:39Z

migrator/migrations/m_180_to_m_181_move_to_blobstore/migration.go

+	if err != nil {
+		return errors.Wrapf(err, "invalid timestamp %v", stat.ModTime())
+	}
+	fd, err := os.Open(scannerDefPath)


should we just open once, then call the Stat() function on the file? Then we don't need to check existence twice

connorgorman · 2023-05-19T02:07:06Z

migrator/migrations/m_180_to_m_181_move_to_blobstore/migration.go

+	// Prepare blob
+	blob := &storage.Blob{
+		Name:         scannerDefBlobName,
+		Oid:          0,


No need to initialize, I would just leave it unset

connorgorman · 2023-05-19T02:08:36Z

migrator/migrations/m_180_to_m_181_move_to_blobstore/migration.go

+		return result.Error
+	}
+
+	if len(targets) == 0 {


Do we need to check and distinguish in this case? Migrations only run once and we can be guaranteed that no object will exist in the database because all of this is wrapped in a txn. I think it's a fair assumption that this does not exist and that we will always need to add and create

This could happen when the first migration fails maybe at migration 182 and then we fixed the problem somehow and customer tries again. All migrations should be re-entriable.

Ah good point, do you think it'd be generally worthwhile to write out the version after each sequence going forward? Not in this PR of course

connorgorman · 2023-05-19T02:09:57Z

migrator/migrations/m_180_to_m_181_move_to_blobstore/migration_test.go

+	"github.com/stretchr/testify/suite"
+)
+
+type categoriesMigrationTestSuite struct {


naming? should be relation to the test

connorgorman · 2023-05-19T02:15:28Z

pkg/postgres/gorm/largeobject/large_objects_test.go

+
+	// Start a new transaction
+	tx2 := s.gormDB.Begin()
+	// tx := gormDB.Begin(&sql.TxOptions{Isolation: sql.LevelRepeatableRead})


Remove comment?

connorgorman · 2023-05-19T02:15:48Z

pkg/postgres/gorm/largeobject/large_objects.go

+// returns err
+func (o *LargeObject) wrapClose(err error) error {
+	if closeErr := o.Close(); closeErr != nil {
+		return closeErr


This will cover up the initial error, can we wrap this?

connorgorman · 2023-05-19T02:16:58Z

pkg/postgres/gorm/largeobject/large_objects.go

+		return err
+	}
+	defer func() {
+		err = obj.Close()


This will overwrite the initial error if there is an error on close

Find a better way to join the errors.

connorgorman · 2023-05-19T02:17:21Z

pkg/postgres/gorm/largeobject/large_objects.go

+	defer func() {
+		err = obj.Close()
+	}()
+	_, err = obj.Truncate(0)


nit: inline this into an if _, err =
and also the same for copy

connorgorman · 2023-05-19T02:17:48Z

pkg/postgres/gorm/largeobject/large_objects.go

+// This is originally created with similar API with existing github.com/jackc/pgx
+// For more details see: http://www.postgresql.org/docs/current/static/largeobjects.html
+type LargeObjects struct {
+	*gorm.DB


should this be gorm.Tx?

No. gorm.Tx is only an interface to have access to transaction and context operation. That is not enough for large object operations.

connorgorman

Mostly nits, some comments about the tx vs gorm.DB

c-du · 2023-05-19T21:27:13Z

/test ocp-4-10-qa-e2e-tests
/test ocp-4-13-qa-e2e-tests
/test gke-nongroovy-e2e-tests

openshift-ci · 2023-05-19T23:03:38Z

@c-du: 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/gke-nongroovy-e2e-tests	`e40addf`	link	false	`/test gke-nongroovy-e2e-tests`
ci/prow/ocp-4-10-qa-e2e-tests	`e40addf`	link	false	`/test ocp-4-10-qa-e2e-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.

Connor Gorman and others added 30 commits March 13, 2023 21:41

Add Blob store to Postgres

7edd1aa

Add utilities to pass transactions through contexts

b77920a

dont forget the proto file

7d4d493

Merge branch 'master' into cgorman-tx-context

46c4217

Merge branch 'cgorman-tx-context' into cgorman-blob-store

9ae1ca7

stage

92d1c0e

stage

d40b4ec

Add delete

4384c9a

Merge branch 'master' into cgorman-tx-context

aa2197e

Merge branch 'cong/tx' into cong/blobstore

fe09a82

regen

8d2a24b

Merge branch 'master' into cong/blobstore

8df6ccc

Address review comments

96cfee9

no blob

13c3efd

Merge branch 'cong/blobstore' into cong/vuldef

c962711

stage

f9d3511

stage

e1129f4

Merge branch 'master' into cong/vuldef

ee06bc3

stage

44e45b5

cherry-pick

c892183

Merge branch 'master' into cong/vuldef

c0434f2

Use blobstore for scanner definitions

007feb1

Merge branch 'master' of github.com:stackrox/stackrox

79142a5

Merge branch 'master' into cong/vuldef

864cca8

stage

1e36fc3

first break it

1312343

Merge branch 'master' into cong/vuldef

59bb9eb

security

e57c2f7

revert sneak

f254423

revert sneak

b2f6f6b

dashrews78 reviewed May 18, 2023

View reviewed changes

dashrews78 requested changes May 18, 2023

View reviewed changes

review comments

09b16a5

c-du requested a review from dashrews78 May 18, 2023 16:55

RTann reviewed May 18, 2023

View reviewed changes

close

c0346c0

c-du requested a review from RTann May 18, 2023 18:10

review

60e8a15

RTann approved these changes May 18, 2023

View reviewed changes

c-du and others added 4 commits May 18, 2023 14:17

size and minor change

0799e65

Merge branch 'cong/vuldef' into cong/vul-migrate

9c6812d

Merge branch 'master' into cong/vuldef

8fe29a9

Merge branch 'cong/vuldef' into cong/vul-migrate

89b1199

connorgorman reviewed May 19, 2023

View reviewed changes

connorgorman suggested changes May 19, 2023

View reviewed changes

Base automatically changed from cong/vuldef to master May 19, 2023 17:24

c-du added 2 commits May 19, 2023 10:25

Resolve review comment

2bb5551

Merge branch 'master' into cong/vul-migrate

e40addf

connorgorman approved these changes May 19, 2023

View reviewed changes

c-du requested a review from connorgorman May 19, 2023 17:35

connorgorman approved these changes May 19, 2023

View reviewed changes

dashrews78 approved these changes May 22, 2023

View reviewed changes

c-du merged commit 220bc3a into master May 22, 2023

c-du deleted the cong/vul-migrate branch May 22, 2023 16:33

		@@ -0,0 +1,98 @@
		//go:build sql_integration

		package m179tom180

Conversation

c-du commented May 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Testing Performed

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 left a comment

Choose a reason for hiding this comment

Uh oh!

c-du commented May 18, 2023

Uh oh!

RTann 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!

Choose a reason for hiding this comment

Uh oh!

dashrews78 commented May 18, 2023

Uh oh!

RTann 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!

connorgorman 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!

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!

connorgorman left a comment

c-du commented May 17, 2023 •

edited

Loading

openshift-ci bot commented May 19, 2023 •

edited

Loading