-
Notifications
You must be signed in to change notification settings - Fork 12
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
Restoring the Microcluster Schema Migration history #689
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Questions regarding the migration order
There was a problem hiding this 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 for discussion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, minor nit
src/k8s/pkg/k8sd/database/sql/migrations/worker-nodes/000-create.sql
Outdated
Show resolved
Hide resolved
ce17486
to
de12ded
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for getting in depth with this! I feel like we should have some sort of dev documentation around this process especially on how to maintain these migrations.
@berkayoz we recently introduces a comment "don't change the order and only append to the migration" (https://github.com/canonical/k8s-snap/pull/689/files#diff-e146bb0bfd60417d271d4806b46753a797e5d4e2cffc7ac9a334c249311fb816R17) but seems like I didn't stick to my own notes :) |
Restoring the Microcluster Migration History
This PR restores our microcluster schema migration history to the original order.
Issue
We had an issue where the latest migration did not get applied: #634
Cause
The reason for this issue is that a migration was removed without dropping the schema version count. Consequently, when refreshing the snap the microcluster migration thought more migrations were already applied, leaving the latest migration un-applied.
Fix
This PR restores the schema migration order which will need to be back-ported into all 1.31 branches.
It is not good practice to remove a migration, this does not remove the existing table. A second migration should be in place to remove the table maintaining the accurate migration history.
Note on Prefix count
We should have labeled these not at all in the first place. It is not good practice to change the existing migration name (in case microcluster introduces checks around migration names). To stay consistent with the current naming I am continuing the current number prefix.