Add RDS Restore Option #2728

jasonmcintosh · 2017-12-20T23:30:21Z

Adds support for creating a DB Instance from an S3 xtrabackup snapshot
#2410
First attempt to do go, so certain points are not entirely ideal. Also would love to rewrite the restore and create so many of the required options are set in a function vs. duplicated, but that can be refactored later

… restores work

jasonmcintosh · 2018-01-29T17:26:57Z

Been a month... any updates?

bflad · 2018-02-15T13:32:17Z

Hi @jasonmcintosh sorry for the maintainer delay in getting this reviewed for you. I don't have time to give this a fully exhaustive review right now but skimming the acceptance test, is it possible to actually test and have this succeed currently? It looks like it would be trying to pull the S3 backup from a random bucket and not succeed. Does AWS have a sample S3 backup that might work with this? Otherwise, we might need to get real creative.

jasonmcintosh · 2018-02-15T15:39:34Z

XtraBackup database - we can create and share a sample file. When I was testing in the provider, we only did unit tests vs. integration tests. Then, we built the provider and did manual integration tests. If you want an automatic integration test I can look at adding that support. It'll be trickier but doable - just have to create a random bucket, a bucket item from the sample file, and then run the integration tests and tear down. We'd need to store the xtrabackup file in the tests but thankfully it's a tiny binary file.

jasonmcintosh · 2018-02-15T15:44:28Z

Bringing in master changes (which look like really one file) to validate/test again.

jasonmcintosh · 2018-02-15T16:02:44Z

@bflad Note, this COULD be a misunderstanding on our part - we're pretty new to Go (some of us are). Were these acceptance tests and we just missed something on this?

bflad · 2018-02-15T16:43:12Z

Sorry our terminology for "acceptance" testing is equivalent to "integration" testing. It expects to fully test a Terraform configuration against the AWS APIs and have it succeed end-to-end. In order to accomplish that, it looks like we'll need to have the test perform quite a few steps according to the documentation including setting up an IAM role that RDS can assume and having a S3 location ready to go with a valid backup file in an automated fashion.

As for handling a valid backup file in S3, I can think of a few automated options:

Create all the infrastructure each test run to create a valid backup file in S3 such as creating an EC2 instance running MySQL that runs Percona XtraBackup and uploads to S3 each time (very brittle and time consuming)
Find and use a publicly accessible backup file for testing (potentially brittle)
Create a backup file for testing, provide to a maintainer here, we upload it to a known and managed location in S3 that is publicly accessible, and the acceptance test is updated to point there
Create a backup file for testing, add this file to the repository, then update the acceptance test to create a bucket and upload a backup file each time

But similar to what you said previously, we're probably going to want to go with one of the last two options, but I think it really depends on the size of the backup file. We probably don't want to include something in the repository over a few MB.

So short story here is that at the very least, the acceptance test for this feature will need to handle the IAM role creation, then we need to figure out the S3 backup file handling.

If you don't have time or interest in doing this, that's perfectly okay too 👍 . It just means this PR will be delayed as this functionality is not currently a priority (that I know about) for any of the Hashicorp employed maintainers.

jasonmcintosh · 2018-02-15T18:08:51Z

We're happy to add this support :) Looking at options 3 & 4 right now - we're thinking option 4 should be viable (creating a test file now, getting it as small as we can). On role... looks like there's another test that creates a monitoring role. Can see if I can't re-use some of that. Good feedback/information - thanks! It may be early next week before we can get to this though. We can run the provider locally for now to get us moving while we add support.

jasonmcintosh · 2018-03-21T16:44:34Z

@bflad We have a 1.3MB tiny backup with a test DB. Do you want it in git (it's a .tar.gz file) or an email so you can create a public S3 location? Working on the tests shortly :) Note, we'll be looking to add the same functionality to aurora - do you want those changes in the same pull request or separate?

bflad · 2018-03-21T18:04:06Z

@jasonmcintosh for now can you email it to my Github username at hashicorp dot com? I'll discuss how we want to handle this internally.

Separate pull request would be preferred. Thanks!

jasonmcintosh · 2018-04-24T16:52:42Z

@bflad We've got acceptance tests quasi-working. It creates the database from an s3 restore. However, we've got a problem where IAM role creation is required for this to work, and the IAM role doesn't seem to be fully propagated before it's used. Do you have any thoughts on solutions around this? e.g. how to add a sleep into the code to make sure the IAM role is there first? We tried a depends_on block similar to the way the enhanced monitoring RDS role works, but that doesn't seem to work in the case of a restore. As we understand it, Step blocks in acceptance tests are destroyed before the next steps, so we can't do a Step "Pre-setup resources", Step "Sleep", Step "Create S3"?

bflad · 2018-04-24T17:35:00Z

Inside the resource.Retry() that wraps conn.RestoreDBInstanceFromS3(), add a conditional that allows it to retry based on the error coming back. You didn't provide exactly what you're seeing but something like this should work:

		err = resource.Retry(5*time.Minute, func() *resource.RetryError {
			_, err = conn.RestoreDBInstanceFromS3(&opts)
			if err != nil {
				if isAWSErr(err, "InvalidParameterValue", "... IAM role message here ...") {
					return resource.RetryableError(err)
				}
				if isAWSErr(err, "InvalidParameterValue", "ENHANCED_MONITORING") {
					return resource.RetryableError(err)
				}
				return resource.NonRetryableError(err)
			}
			return nil
		})

jasonmcintosh · 2018-04-24T19:00:58Z

Went ahead and put this in a files/ folder - happy to change the config if you don't like this... didn't seem like there were many good options. Hadn't heard if you'd put this in a public S3 bucket someplace for test purposes. We're starting on the aurora piece next

jasonmcintosh · 2018-04-24T19:02:59Z

Turns out terraform creates S3 buckets and IAM roles so fast, had to retry on both bucket and IAM role. But acceptance tests passed locally. We added VPC creation code as we delete default vpcs to make sure that everything needed for the test to pass would work regardless of environment.

bflad

Thanks for working through this @jasonmcintosh -- I left an initial review down below. Can you check the items and let us know if you have any questions or do not have time to implement the feedback?

bflad · 2018-04-24T19:25:22Z