Skip to content
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

Jump to bottom

ROX-12578: Migration for groups table #5142

Merged
merged 8 commits into from
Mar 9, 2023
Merged

ROX-12578: Migration for groups table #5142

merged 8 commits into from
Mar 9, 2023

Conversation

dhaus67
Copy link
Contributor

@dhaus67 dhaus67 commented Mar 7, 2023

Description

This PR is a follow-up of pull/5099, for additional context see the parent PR's changes and description.

The PR adds a migration for the groups table.

Essentially, the following will be done:

  • Go through each group and delete all groups which are non-unique (non-unique meaning the tuple role name, auth provider ID, key, value).
  • Apply the new schema which contains a unqiue constraint index on the columns role name, auth provider ID, key, value, ensuring no non-unique groups may be added.

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

  • see unit tests.

@dhaus67 dhaus67 requested a review from a team as a code owner March 7, 2023 21:40
@dhaus67
Copy link
Contributor Author

dhaus67 commented Mar 7, 2023

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@dhaus67 dhaus67 mentioned this pull request Mar 7, 2023
Merged
5 tasks
@dhaus67 dhaus67 requested review from rhybrillou and a team March 7, 2023 21:40
dashrews78
dashrews78 reviewed Mar 7, 2023
View reviewed changes
pkg/migrations/internal/seq_num.go Outdated
@@ -4,7 +4,7 @@ var (
// CurrentDBVersionSeqNum is the current DB version number.
// This must be incremented every time we write a migration.
// It is a shared constant between central and the migrator binary.
CurrentDBVersionSeqNum = 172
CurrentDBVersionSeqNum = 173
Copy link
Contributor

Choose a reason for hiding this comment

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

first comment is I just pushed a change with migration 173, so sadly you will need to adjust that and your naming.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://www.youtube.com/watch?v=KpXsfimrkFo

Copy link
Contributor

Choose a reason for hiding this comment

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

be warned @rhybrillou has a couple of migrations in flight too.

@dashrews78
Copy link
Contributor

I think we need to re-upsert the records to populate the new columns. right?

@ghost
Copy link

ghost commented Mar 7, 2023 •
edited by ghost
Loading

Images are ready for the commit at 4a38fa9.

To use with deploy scripts, first export MAIN_IMAGE_TAG=3.74.x-315-g4a38fa9456.

@dhaus67 dhaus67 force-pushed the master-dh/unique-groups-index branch from ef27ea5 to 8b4a87e Compare March 7, 2023 22:16
@dhaus67 dhaus67 force-pushed the master-dh/unqiue-groups-migration branch from 45a4ecf to 3de9bac Compare March 7, 2023 22:16
@dhaus67
Copy link
Contributor Author

dhaus67 commented Mar 7, 2023

I think we need to re-upsert the records to populate the new columns. right?

Added this.

@dhaus67 dhaus67 requested a review from dashrews78 March 7, 2023 22:17
@dhaus67
Copy link
Contributor Author

dhaus67 commented Mar 7, 2023

/test gke-postgres-upgrade-tests

@stackrox stackrox deleted a comment from openshift-ci bot Mar 7, 2023
@dhaus67
Copy link
Contributor Author

dhaus67 commented Mar 7, 2023

/retest

dashrews78
dashrews78 approved these changes Mar 7, 2023
View reviewed changes
Copy link
Contributor

@dashrews78 dashrews78 left a comment

Choose a reason for hiding this comment

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

LGTM

@dhaus67 dhaus67 force-pushed the master-dh/unique-groups-index branch from 8b4a87e to 3889a1e Compare March 8, 2023 13:18
@dhaus67 dhaus67 force-pushed the master-dh/unqiue-groups-migration branch from 9264729 to 8842eab Compare March 8, 2023 13:18
@dhaus67
Copy link
Contributor Author

dhaus67 commented Mar 8, 2023

/retest

rhybrillou
rhybrillou reviewed Mar 8, 2023
View reviewed changes
Copy link
Contributor

@rhybrillou rhybrillou left a comment

Choose a reason for hiding this comment

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

Good job. Please fix the frozen schema in the updated store, then things should be good to go.

migrator/migrations/m_173_to_m_174_group_unique_constraint/migration.go Outdated
return err
}
if len(groupsToDelete) > 0 {
return previousStore.DeleteMany(ctx, groupsToDelete)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could the error be wrapped like in the walk function for consistency ?
(also applies to re-upserts).

migrator/migrations/m_173_to_m_174_group_unique_constraint/migration_test.go Outdated
},
{
Props: &storage.GroupProperties{
Id: "group-id-3",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any guaranty which if group 3 and group 4 will survive the pruning ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Strictly speaking there isn't, so I rewrote the tests to account for that.

migrator/migrations/m_173_to_m_174_group_unique_constraint/stores/updated/postgres_plugin.go Outdated
ops "github.com/stackrox/rox/pkg/metrics"
"github.com/stackrox/rox/pkg/postgres"
"github.com/stackrox/rox/pkg/postgres/pgutils"
pkgSchema "github.com/stackrox/rox/pkg/postgres/schema"
Copy link
Contributor

Choose a reason for hiding this comment

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

References to the moving pkg/postgres/schema are forbidden in migrations. You should reference the new frozen schema here.

@dhaus67 dhaus67 requested a review from rhybrillou March 8, 2023 20:01
Base automatically changed from master-dh/unique-groups-index to master March 8, 2023 20:23
@rhybrillou rhybrillou mentioned this pull request Mar 9, 2023
Merged
5 tasks
@dhaus67 dhaus67 merged commit 20201b3 into master Mar 9, 2023
@dhaus67 dhaus67 deleted the master-dh/unqiue-groups-migration branch March 9, 2023 15:13
vikin91 pushed a commit that referenced this pull request Mar 13, 2023
@dhaus67 @vikin91
ROX-12578: Migration for groups table (#5142)
ea70c6a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dashrews78 dashrews78 dashrews78 approved these changes

@rhybrillou rhybrillou rhybrillou approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dhaus67 @dashrews78 @rhybrillou