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

fix: issue #813 by removing problematic mysql migration #815

Merged
merged 1 commit into from
Nov 12, 2024

Conversation

peterhaochen47
Copy link
Member

  • mysql migration v58 failed flyway validation because there was a prior migration also numbered v58
    (082fd5e) which had since been removed but nevertheless was recorded in flyway history for those users who have installed credhub versions containing the old v58, hence creating a conflict with the new v58. Hence, this commit removes the new v58 for now to fix the issue for those users.

  • leaving v57 alone because there was never a v57 before, only v57_2, which flyway treats as distinct from v57. See code history here: https://github.com/cloudfoundry/credhub/commits/main/applications/credhub-api/src/main/resources/db/migration/mysql

  • we will try to add back the index (in this removed v58) later in a safe way.

  • also: leaving postgres & H2 migrations intact because there is not a problem there.

Upgrade path testing result

Start server at 10c7295

(where v57.2 was applied)

Flyway history:

|             72 | 56      | change expiry date in certificate credential                                  | SQL  | V56__change_expiry_date_in_certificate_credential.sql                                  |  -969791477 | root         | 2024-11-12 03:32:07 |             80 |       1 |
|             73 | 57.2    | delete related encrypted value when credential is deleted                     | SQL  | V57_2__delete_related_encrypted_value_when_credential_is_deleted.sql                   | -1930144391 | root         | 2024-11-12 03:32:07 |             56 |       1 |

Upgrade to b0dd9ef

(where v57.2 was deleted and the old v58 was applied)

Flyway history:

|             72 | 56      | change expiry date in certificate credential                                  | SQL    | V56__change_expiry_date_in_certificate_credential.sql                                  |  -969791477 | root         | 2024-11-12 03:32:07 |             80 |       1 |
|             73 | 57.2    | delete related encrypted value when credential is deleted                     | SQL    | V57_2__delete_related_encrypted_value_when_credential_is_deleted.sql                   | -1930144391 | root         | 2024-11-12 03:32:07 |             56 |       1 |
|             74 | 57.2    | delete related encrypted value when credential is deleted                     | DELETE | V57_2__delete_related_encrypted_value_when_credential_is_deleted.sql                   | -1930144391 | root         | 2024-11-12 03:36:08 |              0 |       1 |
|             75 | 58      | remove triggers                                                               | SQL    | V58__remove_triggers.sql                                                               | -1214956128 | root         | 2024-11-12 03:36:08 |             49 |       1 |

Upgrade to 0f61935

(aka the commit on top of main branch prior to this PR)

Issue #813 is reproduced by this step. flyway history remains the same as the migration fails.

Upgrade to this PR

Everything works and the flyway history is as expected:

  • the old v58 is deleted.
  • the new v57 is still applied.

Flyway history:

|             72 | 56      | change expiry date in certificate credential                                  | SQL    | V56__change_expiry_date_in_certificate_credential.sql                                  |  -969791477 | root         | 2024-11-12 03:32:07 |             80 |       1 |
|             73 | 57.2    | delete related encrypted value when credential is deleted                     | SQL    | V57_2__delete_related_encrypted_value_when_credential_is_deleted.sql                   | -1930144391 | root         | 2024-11-12 03:32:07 |             56 |       1 |
|             74 | 57.2    | delete related encrypted value when credential is deleted                     | DELETE | V57_2__delete_related_encrypted_value_when_credential_is_deleted.sql                   | -1930144391 | root         | 2024-11-12 03:36:08 |              0 |       1 |
|             75 | 58      | index lowercase credential name                                               | SQL    | V58__remove_triggers.sql                                                               |  1764759980 | root         | 2024-11-12 03:36:08 |             49 |       1 |
|             76 | 58      | index lowercase credential name                                               | DELETE | V58__remove_triggers.sql                                                               |  1764759980 | root         | 2024-11-12 03:42:19 |              0 |       1 |
|             77 | 57      | add lowercase credential name column                                          | SQL    | V57__add_lowercase_credential_name_column.sql                                          | -1606535744 | root         | 2024-11-12 03:42:19 |             73 |       1 |

- mysql migration v58 failed flyway validation because
there was a prior migration also numbered v58
(082fd5e) which had since
been removed but nevertheless was recorded in flyway
history for those users who have installed credhub versions
containing the old v58, hence creating a conflict with the new v58.
Hence, this commit removes the new v58 for now to fix
the issue for those users.

- leaving v57 alone because there was never a v57 before,
only v57_2, which flyway treats as distinct from v57. See
code history here: https://github.com/cloudfoundry/credhub/commits/main/applications/credhub-api/src/main/resources/db/migration/mysql

- we will try to add back the index (in this removed v58)
later in a safe way.

- also: leaving postgres & H2 migrations intact because
there is not a problem there.
@peterhaochen47
Copy link
Member Author

peterhaochen47 commented Nov 12, 2024

I prefer this PR over #814 because this PR contains less change and does not need to roll back as many things, and still fixes the problem.

More importantly, leaving the new v57 in place = one less problem to worry about when we try to re-apply the DB improvements. If we remove the new v57, we will have to think about how to add it back safely, in such a way that works for both those who have not applied it before and those who have (and mysql does not have the IF NOT EXIST syntax like postgres). With this PR, we only need to worry about the new v58.

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.

OK. For this, I will have to trust your test result.

@peterhaochen47 peterhaochen47 merged commit 34815e3 into main Nov 12, 2024
2 checks passed
peterhaochen47 added a commit that referenced this pull request Nov 12, 2024
- context: the performance improvement brought by indexing (#803)
was reverted (#815) due to migration error (issue #813) for some users.
This commit brings back that index, but doing so safely to avoid issues
like #813 (#813 happened to some users due to migration file numbering conflict,
this commit uses a fresh migration number (v59) to avoid that).

- Unfortunately, for the users who happened to have installed
credhub v2.12.94 successfully (aka did not encounter #813),
their mysql DB would already have an index called
"credential_name_lowercase". So in order for the migration in this
commit to work for both those who have installed credhub v2.12.94
and those who have not, this commit's migration instead creates
the index under a different name: "credential_name_lowercase_index"
(mysql does not support the "CREATE INDEX...IF NOT EXIST" syntax).
This would create some DB inefficiency (due to the presence of two
functionally identical indexes) for those who have installed credhub v2.12.94
(which we think is a smaller subset of users), but the system
would still work and likely a net improvement compared to before
due to the presence of indexing. And we would instruct these users
to manually drop the old, duplicate index ("credential_name_lowercase").
peterhaochen47 added a commit that referenced this pull request Nov 13, 2024
- context: the performance improvement brought by indexing (#803)
was reverted (#815) due to migration error (issue #813) for some users.
This commit brings back that index, but doing so safely to avoid issues
like #813 (#813 happened to some users due to migration file numbering conflict,
this commit uses a fresh migration number (v59) to avoid that).

- Unfortunately, for the users who happened to have installed
credhub v2.12.94 successfully (aka did not encounter #813),
their mysql DB would already have an index called
"credential_name_lowercase". So in order for the migration in this
commit to work for both those who have installed credhub v2.12.94
and those who have not, this commit's migration instead creates
the index under a different name: "credential_name_lowercase_index"
(mysql does not support the "CREATE INDEX...IF NOT EXIST" syntax).
This would create some DB inefficiency (due to the presence of two
functionally identical indexes) for those who have installed credhub v2.12.94
(which we think is a smaller subset of users), but the system
would still work and likely a net improvement compared to before
due to the presence of indexing. And we would instruct these users
to manually drop the old, duplicate index ("credential_name_lowercase").
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