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

Correct microcluster schema migration order #676

Merged
merged 1 commit into from
Sep 18, 2024

Conversation

louiseschmidtgen
Copy link
Contributor

@louiseschmidtgen louiseschmidtgen commented Sep 17, 2024

Description

Migrations should be applied in the correct order with the latest migration being applied last.
Microcluster counts the migrations we have done.

Changing the order can lead to mistakes:
We previously apply schemaApplyMigration("feature-status", "000-feature-status.sql") at schema count 5, then we add in schemaApplyMigration("worker-tokens", "001-add-expiry.sql") at schema count 5, leading us to re-apply feature-status in count 6 when migrating the schema.

For instance, this issue occurred migrating moonray: https://github.com/canonical/k8s-snap/blob/archive/release-1.30-moonray/src/k8s/pkg/k8sd/database/schema.go

schemaApplyMigration("feature-status", "000-feature-status.sql"),
schemaApplyMigration("worker-tokens", "001-add-expiry.sql"),
Copy link
Contributor

@HomayoonAlimohammadi HomayoonAlimohammadi Sep 17, 2024

Choose a reason for hiding this comment

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

I think the ordering matters on the table scope right? e.g. the 001-add-expiry.sql for worker-tokens should be after the 000-create.sql for the same table which it is.
the 001 here only refers to the worker-token table counter. the 000-feature-status.sql is not related to worker-token table so the ordering of feature-status related migrations should not matter with regards to the worker-token migrations. wdyt?

I might be totally wrong, but if this is the case, I would personally prefer to have the pseudo "grouping" that we already have with the tables being separated by new lines.

Copy link
Contributor Author

@louiseschmidtgen louiseschmidtgen Sep 17, 2024

Choose a reason for hiding this comment

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

I think there is an issue with how microcluster applies these migrations here: https://github.com/canonical/microcluster/blob/v3/internal/db/update/schema.go#L266. MC seems to iterate over the latest migrations so if we change the order of the migrations we run into trouble.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes,looking at the code, the order seems important. @HomayoonAlimohammadi the grouping aspect was the reason why I put the 001-add-expiry.sql in that location. Seems like this was not a good idea :)

Copy link
Contributor

@HomayoonAlimohammadi HomayoonAlimohammadi left a comment

Choose a reason for hiding this comment

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

Thanks a lot @louiseschmidtgen! left a minor comment of out curiosity

@louiseschmidtgen louiseschmidtgen changed the title Correct migration order Correct microcluster schema migration order Sep 17, 2024
Copy link
Contributor

@bschimke95 bschimke95 left a comment

Choose a reason for hiding this comment

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

LGTM, nice catch

@louiseschmidtgen louiseschmidtgen merged commit 87eb341 into main Sep 18, 2024
20 checks passed
@louiseschmidtgen louiseschmidtgen deleted the KU-1462/migrations-fix branch September 18, 2024 14:05
bschimke95 pushed a commit that referenced this pull request Sep 26, 2024
)

* Correct microcluster schema migration order (#676)
* Restoring the Microcluster Schema Migration History (#689)
evilnick pushed a commit to evilnick/k8s-snap that referenced this pull request Nov 14, 2024
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.

3 participants