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

Roll back problematic mysql db migrations (issue #813) #814

Conversation

peterhaochen47
Copy link
Member

@peterhaochen47 peterhaochen47 commented Nov 11, 2024

This PR, as a whole, rolls back all the DB migrations that caused* #813 but let the postgres migrations (which do not have problems) stay so as to not create unnecessary divergence in users's postgres DB states.

*root cause analysis:

This reverts commit 23315c7.

reverting for now due to: #813
This reverts commit 274d5ce.

reverting for now due to: #813
This reverts commit 56a4b42.

reverting for now due to: #813
- due to issue #813 (with mysql) we have rolled back all DB related changes:
030302a
873bc6d
34afece
which we previously released.

- since theere is no issue with postgres, it safer to NOT remove
the already-released DB migrations so that various consumers's
postgres DB states do not diverge. So this commit brings back
all those already-release postgres DB migration.
@peterhaochen47 peterhaochen47 changed the title Pr/roll back mysql db migrations 57 plus for issue 813 Roll back problematic mysql db migrations (issue #813) Nov 11, 2024
@hsinn0
Copy link
Contributor

hsinn0 commented Nov 12, 2024

forgetting / not having taken that context into account

I see. So for the change in 2.12.69, which I think that we deleted the migration sql files, it would have been better if we just changed the content of the sql file instead, i.e. to no-op. Then we could see the last-used migration script verison number in the source.

Copy link
Contributor

@hsinn0 hsinn0 left a comment

Choose a reason for hiding this comment

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

It seems that this deleting migration sql files is going to work. However, can you actually test the upgrade scenario if you have not done so?

@peterhaochen47
Copy link
Member Author

peterhaochen47 commented Nov 12, 2024

Upgrade scenario testing results:

  • 10c7295 (contains mysql/V57_2__delete_related_encrypted_value_when_credential_is_deleted.sql) => 0f61935 (aka right before this PR) => this PR
  • b0dd9ef (contains mysql/V58__remove_triggers.sql) => 0f61935 (aka right before this PR) => this PR
    • result: this reproduces Database migration in credhub-release 2.12.94 fails. #813 at step 2, resulting in none of the new migrations being applied because conflict with v58, the flyway history looks like below:
    • and step 3 resolves the issue, the flyway history still looks like below too:
|             72 | 56      | change expiry date in certificate credential                                  | SQL  | V56__change_expiry_date_in_certificate_credential.sql                                  |  -969791477 | root         | 2024-11-12 02:25:49 |             96 |       1 |
|             73 | 58      | index lowercase credential name                                               | SQL  | V58__remove_triggers.sql                                                               |  1764759980 | root         | 2024-11-12 02:25:49 |             29 |       1 |
+----------------+---------+-------------------------------------------------------------------------------+------+----------------------------------------------------------------------------------------+-------------+--------------+---------------------+----------------+---------+

notes

from the mysql migrations directory history, we can see that there was never a v57, only v57_2, which flyway recognizes as different.

If we add back v57, it is still fine after this path: b0dd9ef (contains mysql/V58__remove_triggers.sql) => 0f61935 (aka right before this PR) => this PR, with final flyway history:

             72 | 56      | change expiry date in certificate credential                                  | SQL    | V56__change_expiry_date_in_certificate_credential.sql                                  |  -969791477 | root         | 2024-11-12 02:53:57 |             63 |       1 |
|             73 | 58      | remove triggers                                                               | SQL    | V58__remove_triggers.sql                                                               | -1214956128 | root         | 2024-11-12 02:53:57 |             13 |       1 |
|             74 | 58      | remove triggers                                                               | DELETE | V58__remove_triggers.sql                                                               | -1214956128 | root         | 2024-11-12 02:57:38 |              0 |       1 |
|             75 | 57      | add lowercase credential name column                                          | SQL    | V57__add_lowercase_credential_name_column.sql                                          | -1606535744 | root         | 2024-11-12 02:57:38 |             74 |       1 |
+----------------+---------+-------------------------------------------------------------------------------+--------+----------------------------------------------------------------------------------------+-------------+--------------+---------------------+----------------+---------+

So I think I would opt to add back v57, so one less problem to solve in the future.

@peterhaochen47
Copy link
Member Author

Likely closing this in favor of #815

@peterhaochen47
Copy link
Member Author

closing this in favor of #815

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

2 participants