-
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
Add cross-region replication support to AlloyDB #9012
Add cross-region replication support to AlloyDB #9012
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Hello! I am a robot. It looks like you are a: @ScottSuarez, 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. Terraform GA: Diff ( 4 files changed, 487 insertions(+)) |
Tests analyticsTotal tests: Action takenFound 37 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccAlloydbCluster_secondaryClusterDefinedSecondaryConfigButClusterTypeIsPrimary|TestAccAlloydbCluster_secondaryClusterDefinedSecondaryConfigButMissingClusterTypeSecondary|TestAccAlloydbCluster_secondaryClusterMissingSecondaryConfig|TestAccAlloydbCluster_secondaryClusterMandatoryFields|TestAccAlloydbCluster_alloydbSecondaryClusterBasicExample|TestAccAlloydbCluster_AutomatedBackupPolicyHandlesMidnight|TestAccAlloydbCluster_continuousBackup_noChangeIfRemoved|TestAccAlloydbCluster_alloydbClusterFullExample|TestAccAlloydbCluster_maximumFields|TestAccAlloydbCluster_withNetworkConfig|TestAccAlloydbInstance_alloydbInstanceBasicExample|TestAccAlloydbCluster_continuousBackup_update_noChangeIfDefaultsSet|TestAccAlloydbCluster_continuousBackup_enabledByDefault|TestAccAlloydbCluster_deleteAutomatedBackupPolicyAndInitialUser|TestAccAlloydbCluster_addAutomatedBackupPolicyAndInitialUser|TestAccAlloydbCluster_withNetworkConfigAndAllocatedIPRange|TestAccAlloydbCluster_continuousBackup_update|TestAccAlloydbCluster_mandatoryFields|TestAccAlloydbCluster_alloydbClusterBasicExample|TestAccAlloydbCluster_update|TestAccAlloydbBackup_alloydbBackupFullExample|TestAccAlloydbBackup_alloydbBackupBasicExample|TestAccAlloydbInstance_createInstanceWithMandatoryFields|TestAccAlloydbCluster_deleteTimeBasedRetentionPolicy|TestAccAlloydbInstance_createPrimaryAndReadPoolInstance|TestAccAlloydbInstance_createInstanceWithMaximumFields|TestAccAlloydbCluster_restore|TestAccAlloydbInstance_update|TestAccAlloydbInstance_createInstanceWithNetworkConfigAndAllocatedIPRange|TestAccAlloydbInstance_updateDatabaseFlagInPrimaryInstance|TestAccAlloydbCluster_CMEKInAutomatedBackupIsUpdatable|TestAccAlloydbBackup_update|TestAccAlloydbCluster_usingCMEK|TestAccAlloydbBackup_usingCMEK|TestAccAlloydbCluster_continuousBackup_CMEKIsUpdatable|TestAccAlloydbCluster_missingWeeklySchedule|TestAccAlloydbBackup_createBackupWithMandatoryFields |
Rerun these tests in REPLAYING mode to catch issues
|
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.
nit for test files: indentation.
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.
What does delete mean in this instance? How is the secondary cluster getting cleaned up ?
Delete for secondary cluster is the same as the primary cluster because the same API is called with the same url and parameter. So no modification is needed. |
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.
This looks good to me if the other reviewer signs off as well
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.
Apologies, I just noticed the VCR failures. We'll need to fix those before moving forward. This change makes sense to me though in philosophy
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. Terraform GA: Diff ( 4 files changed, 489 insertions(+)) |
Tests analyticsTotal tests: Action takenFound 6 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccAlloydbCluster_alloydbSecondaryClusterBasicExample|TestAccAlloydbInstance_createPrimaryAndReadPoolInstance|TestAccAlloydbInstance_createInstanceWithMaximumFields|TestAccAlloydbInstance_updateDatabaseFlagInPrimaryInstance|TestAccAlloydbInstance_createInstanceWithMandatoryFields|TestAccDataSourceGoogleServiceAccountAccessToken_basic |
Rerun these tests in REPLAYING mode to catch issues
|
All of these tests are failing due to service networking issue that is being tracked here: #8904 Error: https://screenshot.googleplex.com/8vfrTwfM6sWqYG8 @199201shubhamsahu Please comment these tests and add their names in the bug attached above so that I will enable them afterwards. Before doing that, please check if these tests are passing on you machine or not. The tests which are already attached in the bug were all passing on the local/cloudtop machine but were failing in the nightly run due to service networking issue attached above. We shouldn't be blocked due an issue in an unrelated service to release our feature to Terraform. |
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. Terraform GA: Diff ( 5 files changed, 497 insertions(+), 9 deletions(-)) |
Tests analyticsTotal tests: Action takenFound 2 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccWorkstationsWorkstationConfig_update|TestAccWorkstationsWorkstationConfig_updateHostDetails |
Rerun these tests in REPLAYING mode to catch issues
|
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. Terraform GA: Diff ( 5 files changed, 497 insertions(+), 9 deletions(-)) |
Tests analyticsTotal tests: Action takenFound 2 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccDataprocJobIamPolicy|TestAccDataprocClusterIamPolicy |
Rerun these tests in REPLAYING mode to catch issues
|
@199201shubhamsahu could you pull changes into this PR so it can be merged? There are merge conflicts. |
3578c66
to
aef0da0
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. Terraform GA: Diff ( 4 files changed, 1233 insertions(+), 6 deletions(-)) |
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. Terraform GA: Diff ( 4 files changed, 1156 insertions(+), 6 deletions(-)) |
Tests analyticsTotal tests: Action takenFound 28 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccAlloydbCluster_secondaryClusterWithNetworkConfigAndAllocatedIPRange|TestAccAlloydbCluster_secondaryClusterWithNetworkConfig|TestAccAlloydbCluster_secondaryClusterAddAutomatedBackupPolicy|TestAccAlloydbCluster_secondaryClusterUsingCMEK|TestAccAlloydbCluster_secondaryClusterMandatoryFields|TestAccAlloydbCluster_alloydbSecondaryClusterBasicExample|TestAccAlloydbCluster_secondaryClusterUpdate|TestAccAlloydbBackup_alloydbBackupBasicExample|TestAccAlloydbCluster_secondaryClusterDefinedSecondaryConfigButClusterTypeIsPrimary|TestAccAlloydbCluster_secondaryClusterMissingSecondaryConfig|TestAccAlloydbUser_updateRoles_BuiltIn|TestAccAlloydbUser_alloydbUserBuiltinExample|TestAccAlloydbBackup_createBackupWithMandatoryFields|TestAccAlloydbUser_updateRoles_IAM|TestAccAlloydbUser_alloydbUserIamExample|TestAccAlloydbBackup_update|TestAccAlloydbUser_updatePassword_BuiltIn|TestAccAlloydbCluster_restore|TestAccAlloydbCluster_secondaryClusterDefinedSecondaryConfigButMissingClusterTypeSecondary|TestAccAlloydbBackup_usingCMEK|TestAccAlloydbInstance_createInstanceWithMandatoryFields|TestAccAlloydbInstance_createPrimaryAndReadPoolInstance|TestAccAlloydbInstance_alloydbInstanceBasicExample|TestAccAlloydbInstance_createInstanceWithNetworkConfigAndAllocatedIPRange|TestAccAlloydbInstance_update|TestAccAlloydbBackup_alloydbBackupFullExample|TestAccBigQueryDataTable_bigtable|TestAccDataprocJobIamPolicy |
mmv1/third_party/terraform/services/alloydb/resource_alloydb_secondary_cluster_test.go
Show resolved
Hide resolved
mmv1/third_party/terraform/services/alloydb/resource_alloydb_secondary_cluster_test.go
Show resolved
Hide resolved
`, context) | ||
} | ||
|
||
func TestAccAlloydbCluster_secondaryClusterUsingCMEK(t *testing.T) { |
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.
optional: We could consolidate the other secondary settings into a _full
test if you wanted! Setting the fields at least once is the target.
Other than TestAccAlloydbCluster_secondaryClusterAddAutomatedBackupPolicy
since that's an error. When we get to promotion we'll want to test promoting + setting that at the same time.
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.
Other fields like initial_user
, continuous_backup_config
, automated_backup_policy
can not be enabled or set, so keeping only basic secondary cluster.
Please let me know if you meant something else by _full
test
Yes, will add all possible tests that exists in primary cluster to the promoted cluster tests as well
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.
non-blocking: We should document those fields not being available for secondary clusters somewhere.
Rerun these tests in REPLAYING mode to catch issues
|
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. Terraform GA: Diff ( 4 files changed, 1163 insertions(+), 6 deletions(-)) |
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. Terraform GA: Diff ( 4 files changed, 1135 insertions(+), 6 deletions(-)) |
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.
One missing immutable
, otherwise LGTM.
Co-authored-by: Riley Karson <[email protected]>
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. Terraform GA: Diff ( 4 files changed, 1136 insertions(+), 6 deletions(-)) |
Tests analyticsTotal tests: Action takenFound 10 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccAlloydbCluster_update|TestAccAlloydbCluster_addAutomatedBackupPolicyAndInitialUser|TestAccAlloydbCluster_continuousBackup_update|TestAccAlloydbCluster_secondaryClusterUpdate|TestAccAlloydbCluster_CMEKInAutomatedBackupIsUpdatable|TestAccAlloydbCluster_secondaryClusterAddAutomatedBackupPolicy|TestAccAlloydbCluster_continuousBackup_CMEKIsUpdatable|TestAccAlloydbCluster_deleteAutomatedBackupPolicyAndInitialUser|TestAccAlloydbCluster_AutomatedBackupPolicyHandlesMidnight|TestAccAlloydbCluster_restore |
Rerun these tests in REPLAYING mode to catch issues
|
Backport hashicorp/terraform-provider-google-beta#6474 To Create a Secondary Cluster, `cluster_type` and `secondary_config` are the additional fields to be mentioned in the cluster. The secondary cluster is dependent on the primary instance and the same needs to be added in the config using the `depends_on` field. The field continuous_backup_config.enabled needs to be set false as continuous backup is not supported for secondary clusters. The deletion_policy is set to FORCE as the secondary instance that will be created in the secondary cluster can not be deleted independently, but instead the entire secondary cluster needs to be deleted forcefully along with its secondary instance. ``` cluster_type = "SECONDARY" deletion_policy = "FORCE" secondary_config { primary_cluster_name = <fully qualified primary cluster name> // Eg: primary_cluster_name = google_alloydb_cluster.<primary_cluster_name>.name } continuous_backup_config { enabled = false } depends_on = [google_alloydb_instance.<primary_instance_name>] ``` Part of [hashicorp/terraform-provider-google#13251](hashicorp/terraform-provider-google#13251) **Release Note Template for Downstream PRs (will be copied)** ``` alloydb: added `cluster_type` and `secondary_config` fields to support secondary clusters in `google_alloydb_cluster` resource. ``` Derived from [GoogleCloudPlatform/magic-modules#9012](GoogleCloudPlatform/magic-modules#9012)
Backport hashicorp/terraform-provider-google-beta#6474 To Create a Secondary Cluster, `cluster_type` and `secondary_config` are the additional fields to be mentioned in the cluster. The secondary cluster is dependent on the primary instance and the same needs to be added in the config using the `depends_on` field. The field continuous_backup_config.enabled needs to be set false as continuous backup is not supported for secondary clusters. The deletion_policy is set to FORCE as the secondary instance that will be created in the secondary cluster can not be deleted independently, but instead the entire secondary cluster needs to be deleted forcefully along with its secondary instance. ``` cluster_type = "SECONDARY" deletion_policy = "FORCE" secondary_config { primary_cluster_name = <fully qualified primary cluster name> // Eg: primary_cluster_name = google_alloydb_cluster.<primary_cluster_name>.name } continuous_backup_config { enabled = false } depends_on = [google_alloydb_instance.<primary_instance_name>] ``` Part of [hashicorp/terraform-provider-google#13251](hashicorp/terraform-provider-google#13251) **Release Note Template for Downstream PRs (will be copied)** ``` alloydb: added `cluster_type` and `secondary_config` fields to support secondary clusters in `google_alloydb_cluster` resource. ``` Derived from [GoogleCloudPlatform/magic-modules#9012](GoogleCloudPlatform/magic-modules#9012)
To Create a Secondary Cluster,
cluster_type
andsecondary_config
are the additional fields to be mentioned in the cluster.The secondary cluster is dependent on the primary instance and the same needs to be added in the config using the
depends_on
field.The field continuous_backup_config.enabled needs to be set false as continuous backup is not supported for secondary clusters.
The deletion_policy is set to FORCE as the secondary instance that will be created in the secondary cluster can not be deleted independently, but instead the entire secondary cluster needs to be deleted forcefully along with its secondary instance.
Part of hashicorp/terraform-provider-google#13251
Release Note Template for Downstream PRs (will be copied)