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.

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

feat: migration to single table SQL schema #707

Merged
merged 25 commits into from
Sep 24, 2021

Conversation

zepatrik
Copy link
Member

@zepatrik zepatrik commented Sep 13, 2021

Related issue(s)

closes #628

Further Comments

There is a new GH action now that runs an "e2e" kind of script. It first downloads Keto v0.6.0-alpha.3, applies it's migrations, and then adds some data. After that we migrate the data over with the current checked out branch and then use the check-API to see if the data are available and work properly. The edge cases are covered in the unit tests.


Commit Message (WIP)

This change adds a migration path from Keto version v0.6.x to the new persistence structure introduced by #638. Every namespace has to be migrated separately, or you can use the CLI to detect and migrate all namespaces at once. Have a look at keto help namespace migrate legacy for all details.
Please make sure that you backup the database before running the migration command. For live-migrations, it is possible to run Keto v0.6 along v0.7 while migrating. Please note that this migration might be a bit slower than usual, as we have to pull the data from the database, transcode it in Keto, and then write it to the new table structure.
Future versions of Keto will not include this migration script, so you will have to migrate to v0.7 to move on from there.

@zepatrik zepatrik requested a review from aeneasr September 14, 2021 11:43
@zepatrik zepatrik marked this pull request as ready for review September 14, 2021 11:43
@@ -0,0 +1 @@
DROP TABLE "keto_namespace";
Copy link
Member Author

Choose a reason for hiding this comment

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

This table was actually never used, not even in the old versions of Keto. Therefore just dropping it.

Comment on lines 90 to 93
if err := migrator.MigrateDown(cmd.Context(), n); err != nil {
_, _ = fmt.Fprintf(cmd.ErrOrStderr(), "Could not migrate down: %s\n", err.Error())
return cmdx.FailSilently(cmd)
}
Copy link
Member Author

Choose a reason for hiding this comment

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

I was first unsure whether we should migrate down directly without user consent, but I think it is fine because we do it after we have successfully transferred the data. Otherwise we would need a separate command just to migrate the namespace down, which is a bit of an overkill IMO.

Copy link
Member

Choose a reason for hiding this comment

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

Hm, the problem here would be that if something goes wrong, the old data is gone. We should definitely respect this when executing this on our DB!

Copy link
Member Author

Choose a reason for hiding this comment

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

But if something goes wrong before, we would have already returned.

Copy link
Member

Choose a reason for hiding this comment

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

I assume the comment is no longer relevant as per https://github.com/ory/keto/pull/707/files#r712027278

Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

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

Looking great! :)

cmd/migrate/up.go Outdated Show resolved Hide resolved
cmd/namespace/migrate_legacy.go Outdated Show resolved Hide resolved
cmd/namespace/migrate_legacy.go Outdated Show resolved Hide resolved
Comment on lines 90 to 93
if err := migrator.MigrateDown(cmd.Context(), n); err != nil {
_, _ = fmt.Fprintf(cmd.ErrOrStderr(), "Could not migrate down: %s\n", err.Error())
return cmdx.FailSilently(cmd)
}
Copy link
Member

Choose a reason for hiding this comment

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

Hm, the problem here would be that if something goes wrong, the old data is gone. We should definitely respect this when executing this on our DB!

internal/driver/pop_connection.go Show resolved Hide resolved
scripts/single-table-migration-e2e.sh Outdated Show resolved Hide resolved
for _, r := range rs {
ri, err := r.toInternal()
if err != nil {
m.d.Logger().WithField("relation_tuple", r).WithField("hint", "https://github.com/ory/keto/issues/661").WithError(err).Warn("Skipping relation tuple, it seems to be in a broken state. Please recreate it manually.")
Copy link
Member

Choose a reason for hiding this comment

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

Can this actually happen? How would one recover from this, given that we delete the data after the migration is done?

Copy link
Member Author

Choose a reason for hiding this comment

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

As described in the issue, you can theoretically create a tuple that is serialized for the legacy table into a format that is not de-serializable anymore. The only way to recover is to recreate the tuple once the rest is migrated. But you are right, we should maybe not migrate down in this case.

@zepatrik zepatrik requested a review from aeneasr September 21, 2021 13:44
@zepatrik zepatrik merged commit 00713bc into master Sep 24, 2021
@zepatrik zepatrik deleted the feat/persistence-migration-path branch September 24, 2021 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migration path for v0.7
2 participants