-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Support Swithcover for MySQL and PostgreSQL #12646
Support Swithcover for MySQL and PostgreSQL #12646
Conversation
Hello! I am a robot. Tests will require approval from a repository maintainer to run. @zli82016, a repository maintainer, has been assigned to review your changes. If you have not received review feedback within 2 business days, please leave a comment on this PR asking them to take a look. You can help make sure that review is quick by doing a self-review and by running impacted tests locally. |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Tests analyticsTotal tests: 106 Click here to see the affected service packages
Action takenFound 30 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
🟢 Tests passed during RECORDING mode: 🟢 No issues found for passed tests after REPLAYING rerun. 🟢 All tests passed! |
@zli82016 This PR has been waiting for review for 3 weekdays. Please take a look! Use the label |
mmv1/third_party/terraform/services/sql/resource_sql_database_instance.go.tmpl
Show resolved
Hide resolved
mmv1/third_party/terraform/services/sql/resource_sql_database_instance.go.tmpl
Outdated
Show resolved
Hide resolved
mmv1/third_party/terraform/services/sql/resource_sql_database_instance.go.tmpl
Show resolved
Hide resolved
mmv1/third_party/terraform/services/sql/resource_sql_database_instance.go.tmpl
Show resolved
Hide resolved
mmv1/third_party/terraform/services/sql/resource_sql_database_instance_test.go
Outdated
Show resolved
Hide resolved
It looks like the switchover method is not called from Terraform provider. Is this method called in API side to complete the switchover operation? |
I'm not familiar with terraform (still), but the API is called from the provider code magic-modules/mmv1/third_party/terraform/services/sql/resource_sql_database_instance.go.tmpl Lines 1962 to 1964 in 890855b
|
@zli82016 I briefly replied to all of your comment. PTAL |
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.
I added some comments. Let me know if you have questions.
I'm currently fixing test. As side affect of setting replication_cluster as If |
Sorry to hear that. Do you have the logs for the test that the plan is not empty? |
I'm still not familiar with terraform (this is first big change I'm making), the first (and probably the biggest) problem I'm having is following: (All manual tests are done after reflecting your suggestion)
However terrafrom plan shows following "1 to change"
probably related warning message
From my understanding, fixing "original-replica" should be done by modifying main.tf, and running "terraform plan" and check that there's no change needed. (I'm following #12241 which is really similar work as I'm doing now) |
Thank you for providing the information. It looks like update operation of |
(Again my answer is probably wrong about terraform) for reference, I published unfinished commit that I'm working on bd4c2df
cloud SQL's update operation on "original-primary" changes both "original-primary" and "original-replica" "original-primary". before:
after: non-present "original-replica". before:
after: non-present So after step 2, change on
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Tests analyticsTotal tests: 106 Click here to see the affected service packages
Action takenFound 30 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
🟢 Tests passed during RECORDING mode: 🟢 No issues found for passed tests after REPLAYING rerun. 🟢 All tests passed! |
mmv1/third_party/terraform/services/sql/resource_sql_database_instance.go.tmpl
Show resolved
Hide resolved
mmv1/third_party/terraform/services/sql/resource_sql_database_instance_test.go
Outdated
Show resolved
Hide resolved
mmv1/third_party/terraform/services/sql/resource_sql_database_instance_test.go
Outdated
Show resolved
Hide resolved
bd4c2df
to
a0f4d0b
Compare
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Tests analyticsTotal tests: 106 Click here to see the affected service packages
Action takenFound 30 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
🟢 Tests passed during RECORDING mode: 🟢 No issues found for passed tests after REPLAYING rerun. 🟢 All tests passed! |
…dification Fix after review 1
Hello @zli82016 I applied all changes we discussed in offline. PTAL |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Tests analyticsTotal tests: 106 Click here to see the affected service packages
Action takenFound 34 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
🟢 Tests passed during RECORDING mode: 🟢 No issues found for passed tests after REPLAYING rerun. 🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
mmv1/third_party/terraform/services/sql/resource_sql_database_instance.go.tmpl
Outdated
Show resolved
Hide resolved
Fix after review 2: Remove NON_EXISTENT
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Tests analyticsTotal tests: 106 Click here to see the affected service packages
Action takenFound 4 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
Add replication_cluster to instances list
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Tests analyticsTotal tests: 106 Click here to see the affected service packages
🟢 All tests passed! View the build log |
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
References
Implementation
main change: adding replication_cluster: just copied corresponding filed from API. https://cloud.google.com/sql/docs/mysql/admin-api/rest/v1beta4/instances#resource:-databaseinstance . dr_replica is read only, and failover_dr_replica_name is optionally set by customer. To support "deletion" of optional computed resource, we use "NON_EXISTENT" as placeholder for non-existent failover_dr_replica_name in terraform.
isSwitchoverRequested(): just removed restriction on the database type.
isSwitchoverFromOldPrimarySide() also removed restriction on the database type, and checking
isCascadableReplica
only for SQL server (because this concept is SQL server specific).nit: when handling error from PromoteReplica/Swithcover, we use d.Get("name") instead of instance.Name which returns nullptr error.
Tests
failover_dr_replica_name
to validate that "NON_EXISTENT" works correctly.Docs
Release Note Template for Downstream PRs (will be copied)