Add restoreBackup support for sql db instance #4336

c2thorn · 2020-12-21T17:12:41Z

Fixes hashicorp/terraform-provider-google#2446

Add support for the very imperative restoreBackup action. This change essentially triggers the restoreBackup action:

After the resource is created and restore_backup_context is set
At the end of update if restore_backup_context has a change

If this PR is for Terraform, I acknowledge that I have:

Searched through the issue tracker for an open issue that this either resolves or contributes to, commented on it to claim it, and written "fixes {url}" or "part of {url}" in this PR description. If there were no relevant open issues, I opened one and commented that I would like to work on it (not necessary for very small changes).
Generated Terraform, and ran make test and make lint to ensure it passes unit and linter tests.
Ensured that all new fields I added that can be set by a user appear in at least one example (for generated resources) or third_party test (for handwritten resources or update tests).
Ran relevant acceptance tests (If the acceptance tests do not yet pass or you are unable to run them, please let your reviewer know).
Read the Release Notes Guide before writing my release note below.

Release Note Template for Downstream PRs (will be copied)

sql: added restore from backup support to `google_sql_database_instance`

c2thorn · 2020-12-21T17:15:33Z

third_party/terraform/resources/resource_sql_database_instance.go.erb

@@ -1025,6 +1056,8 @@ func resourceSqlDatabaseInstanceUpdate(d *schema.ResourceData, meta interface{})
 		return err
 	}

+	// TODO if d.HasChange("settings") doesn't work, so we just send an empty request if only restoring from backup


Thanks to hashicorp/terraform-plugin-sdk#98 and settings.authorized_networks, d.HasChange("settings") always returns true.

As it stands the resource will send empty updates which causes an extra harmless request.

modular-magician · 2020-12-21T17:20:36Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 1 file changed, 79 insertions(+))
Terraform Beta: Diff ( 1 file changed, 79 insertions(+))

modular-magician · 2020-12-21T17:20:45Z

I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=164108"

c2thorn · 2020-12-21T17:23:16Z

third_party/terraform/resources/resource_sql_database_instance.go.erb

@@ -1051,6 +1084,15 @@ func resourceSqlDatabaseInstanceUpdate(d *schema.ResourceData, meta interface{})
 		return err
 	}

+	if r, ok := d.GetOk("restore_backup_context"); ok {


To support restoring from a backup during Update, we'll have to trigger when changes are made to restore_backup_context other than the field just being removed

Can you comment that inline?

c2thorn · 2020-12-21T17:24:17Z

The draft is still pending:

Tests showcasing create + update restoreBackup triggers
Documentation

modular-magician · 2020-12-21T17:28:36Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 1 file changed, 80 insertions(+))
Terraform Beta: Diff ( 1 file changed, 80 insertions(+))

modular-magician · 2020-12-21T17:28:46Z

I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=164108"

rileykarson

This seems like a reasonable take. We'll want to be explicit in the documentation that this is a pretty dangerous field to set & document how it may behave differently than other fields.

c2thorn · 2020-12-29T21:08:03Z

Test capability is pending #4352 for datasource use and backup bootstrapping

modular-magician · 2021-01-05T16:05:13Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 3 files changed, 206 insertions(+))
Terraform Beta: Diff ( 3 files changed, 206 insertions(+))

modular-magician · 2021-01-05T16:05:22Z

I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=165413"

modular-magician · 2021-01-05T17:30:31Z

I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccDataSourceGoogleSubnetwork|TestAccDataSourceStorageBucketObjectContent_Basic|TestAccActiveDirectoryDomainTrust_activeDirectoryDomainTrustBasicExample|TestAccContainerCluster_withConfidentialNodes|TestAccContainerCluster_withPrivateClusterConfigMissingCidrBlock|TestAccDataprocJob_Spark You can view the result here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=165417"

rileykarson · 2021-01-06T18:31:52Z

third_party/terraform/resources/resource_sql_database_instance.go.erb

@@ -1051,6 +1084,15 @@ func resourceSqlDatabaseInstanceUpdate(d *schema.ResourceData, meta interface{})
 		return err
 	}

+	if r, ok := d.GetOk("restore_backup_context"); ok {


Can you comment that inline?

rileykarson · 2021-01-06T18:33:40Z

third_party/terraform/resources/resource_sql_database_instance.go.erb

+							Required:    true,
+							Description: `The ID of the backup run to restore from.`,
+						},
+						"instance_id": {


Just curious: what does specifying this field (or not specifying this field) change?

You can optionally restore to the backup of another instance (backup ID's are unique per instance). If left blank, the API just assumes the same instance. I can add a line in the documentation to make this more clear.

modular-magician · 2021-01-07T21:07:11Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 3 files changed, 209 insertions(+))
Terraform Beta: Diff ( 3 files changed, 209 insertions(+))

modular-magician · 2021-01-07T21:07:29Z

I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=165856"

modular-magician · 2021-01-07T22:16:05Z

I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccInstanceTemplateDatasource_name|TestAccInstanceTemplateDatasource_filter|TestAccInstanceTemplateDatasource_filter_mostRecent|TestAccDataSourceGoogleSubnetwork|TestAccDataSourceGoogleServiceAccountIdToken_impersonation|TestAccDataSourceStorageBucketObjectContent_Basic|TestAccActiveDirectoryDomainTrust_activeDirectoryDomainTrustBasicExample|TestAccContainerCluster_withConfidentialNodes|TestAccContainerCluster_withPrivateClusterConfigMissingCidrBlock|TestAccDataprocJob_Spark You can view the result here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=165864"

achew22 · 2021-01-21T05:45:19Z

@rileykarson and @c2thorn, I believe this has caused a regression. I was able to trigger it by terraform importing a different resource (in my case a GCS bucket) using the follwoing:

$ terraform import module.modulename.google_storage_bucket.bucketname bucketname
module.modulename.google_storage_bucket.bucketname: Importing from ID "bucketname"...
module.modulename.google_storage_bucket.bucketname: Import prepared!
  Prepared google_storage_bucket for import
module.modulename.google_storage_bucket.bucketname: Refreshing state... [id=bucketname]

Error: Invalid resource instance data in state

  on ../modules/sql/main.tf line 19:
  19: resource "google_sql_database_instance" "postgres" {

Instance module.sql.google_sql_database_instance.postgres
data could not be decoded from the state: unsupported attribute
"restore_backup_context".

This does not happen when I run terraform apply, only when I try to import another resource.

$ terraform version
Terraform v0.14.5
+ provider registry.terraform.io/hashicorp/aws v3.24.1
+ provider registry.terraform.io/hashicorp/google v3.52.0
+ provider registry.terraform.io/hashicorp/google-beta v3.52.0
+ provider registry.terraform.io/hashicorp/kubernetes v1.13.3
+ provider registry.terraform.io/hashicorp/kubernetes-alpha v0.2.1
+ provider registry.terraform.io/hashicorp/random v3.0.1
+ provider registry.terraform.io/hashicorp/tls v3.0.0

c2thorn · 2021-01-21T17:35:53Z

Hey @achew22, sorry for any troubles that this may have caused you. I tried reproducing this error myself by importing a storage bucket while already having a database instance in state, but was not able to get the error.

Do you mind filing a new issue with the steps you have taken and potentially your terraform configuration that produced this error?

achew22 · 2021-01-21T17:39:22Z

@c2thorn I would be very happy to do this. Do you have any hints on things that you think might be related to this. This is deep in the bowels of our terraform config and it even took me a while to get the stack trace (not sure what else to call it) cleaned up for publication. I can poke at it, but I'm, unfortunately timeboxed on this one.

c2thorn · 2021-01-21T17:53:46Z

@achew22 I just realized that this feature was released in v3.53 of the providers and your log shows v3.52. This seems like a versioning issue where maybe some part of your config is using a different version (google_storage_bucket maybe?)

Maybe you can try constraining 3.53 or newer via https://www.terraform.io/docs/configuration/provider-requirements.html#version-constraints ?

achew22 · 2021-01-28T01:29:29Z

@c2thorn Sorry to raise the false alarm. I think you're right. I was able to force my local terraform to v3.53 and it seems to be working now. Thanks so much!

c2thorn · 2021-01-28T16:57:15Z

@achew22 Great! Glad you were able to get unstuck!

google-cla bot added the cla: yes label Dec 21, 2020

c2thorn commented Dec 21, 2020

View reviewed changes

c2thorn requested a review from rileykarson December 21, 2020 17:24

c2thorn changed the title ~~Restore from backup draft~~ Add restoreBackup support for sql db instance Dec 21, 2020

rileykarson reviewed Dec 21, 2020

View reviewed changes

c2thorn added 4 commits December 30, 2020 13:32

Restore from backup draft

270be2f

fix method sig

c0e4956

check restore_backup_context exists on update

3b798c1

Add test and documentation

1a953a1

c2thorn force-pushed the sql-instance-restore branch from 45fe6bf to 1a953a1 Compare January 5, 2021 15:57

c2thorn marked this pull request as ready for review January 5, 2021 16:25

c2thorn requested a review from rileykarson January 5, 2021 21:51

rileykarson approved these changes Jan 6, 2021

View reviewed changes

Changes from review

68fa3bd

rileykarson approved these changes Jan 7, 2021

View reviewed changes

c2thorn merged commit 75a6a50 into GoogleCloudPlatform:master Jan 7, 2021

c2thorn deleted the sql-instance-restore branch January 7, 2021 23:08

This was referenced Jan 7, 2021

Add restoreBackup support for sql db instance hashicorp/terraform-provider-google-beta#2843

Merged

Add restoreBackup support for sql db instance hashicorp/terraform-provider-google#8138

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add restoreBackup support for sql db instance #4336

Add restoreBackup support for sql db instance #4336

c2thorn commented Dec 21, 2020

c2thorn Dec 21, 2020

modular-magician commented Dec 21, 2020

modular-magician commented Dec 21, 2020

c2thorn Dec 21, 2020

rileykarson Jan 6, 2021

c2thorn commented Dec 21, 2020

modular-magician commented Dec 21, 2020

modular-magician commented Dec 21, 2020

rileykarson left a comment

c2thorn commented Dec 29, 2020

modular-magician commented Jan 5, 2021

modular-magician commented Jan 5, 2021

modular-magician commented Jan 5, 2021

rileykarson Jan 6, 2021

rileykarson Jan 6, 2021

c2thorn Jan 7, 2021

modular-magician commented Jan 7, 2021

modular-magician commented Jan 7, 2021

modular-magician commented Jan 7, 2021

achew22 commented Jan 21, 2021

c2thorn commented Jan 21, 2021

achew22 commented Jan 21, 2021

c2thorn commented Jan 21, 2021

achew22 commented Jan 28, 2021

c2thorn commented Jan 28, 2021

Add restoreBackup support for sql db instance #4336

Add restoreBackup support for sql db instance #4336

Conversation

c2thorn commented Dec 21, 2020

c2thorn Dec 21, 2020

Choose a reason for hiding this comment

modular-magician commented Dec 21, 2020

Diff report:

modular-magician commented Dec 21, 2020

c2thorn Dec 21, 2020

Choose a reason for hiding this comment

rileykarson Jan 6, 2021

Choose a reason for hiding this comment

c2thorn commented Dec 21, 2020

modular-magician commented Dec 21, 2020

Diff report:

modular-magician commented Dec 21, 2020

rileykarson left a comment

Choose a reason for hiding this comment

c2thorn commented Dec 29, 2020

modular-magician commented Jan 5, 2021

Diff report:

modular-magician commented Jan 5, 2021

modular-magician commented Jan 5, 2021

rileykarson Jan 6, 2021

Choose a reason for hiding this comment

rileykarson Jan 6, 2021

Choose a reason for hiding this comment

c2thorn Jan 7, 2021

Choose a reason for hiding this comment

modular-magician commented Jan 7, 2021

Diff report:

modular-magician commented Jan 7, 2021

modular-magician commented Jan 7, 2021

achew22 commented Jan 21, 2021

c2thorn commented Jan 21, 2021

achew22 commented Jan 21, 2021

c2thorn commented Jan 21, 2021

achew22 commented Jan 28, 2021

c2thorn commented Jan 28, 2021