Fix error in creating aws copy snaphot request #2042
Conversation
|
Thanks for submitting this pull request 🎉. The team will review it soon and get back to you. If you haven't already, please take a moment to review our project contributing guideline and Code of Conduct document. |
| si := ec2.CopySnapshotInput{ | ||
| SourceSnapshotId: aws.String(from.ID), | ||
| SourceRegion: aws.String(from.Region), | ||
| DestinationRegion: ec2Cli.Config.Region, | ||
| } | ||
| rq, _ := fromCli.CopySnapshotRequest(&si) | ||
| rq, _ := ec2Cli.CopySnapshotRequest(&si) |
There was a problem hiding this comment.
IIUC, the region used to create the client (in the constructor above) needs to match the region in the request as well, is that correct? (that's what I gather from the comment above (// Copy operation must be initiated from the destination region.), so double checking.
There was a problem hiding this comment.
Yes, part of the aws sdk update fixed the issue of letting you have the destination-region be different from the client region (which they're supposed to be the same). We were doing it wrong before, but aws was allowing it in a way, whereas now it just needs to be done the correct way which is destination-region = client.region
| @@ -144,7 +144,7 @@ func (s *BlockStorageProviderSuite) TestCreateSnapshot(c *C) { | |||
| } | |||
|
|
|||
| func (s *BlockStorageProviderSuite) TestSnapshotCopy(c *C) { | |||
| c.Skip("Sometimes, snapcopy takes over 10 minutes. go test declares failure if tests are that slow.") | |||
| // c.Skip("Sometimes, snapcopy takes over 10 minutes. go test declares failure if tests are that slow.") | |||
There was a problem hiding this comment.
Is the intent to remove the // before merging to prevent checking in commented code?
There was a problem hiding this comment.
Yea, I agree, we should remove the c.Skip completely instead of commenting.
We ran some tests on it yesterday and it only added 5 seconds per run to have it running. This was skipped five years ago, when, I'm assuming there were performance differences, and in the past five years those have been improved to where it doesn't take as long now. This unit test does catch the original error when running so it's important to have it running.
There was a problem hiding this comment.
My intention of leaving this as a comment is to leave some easy breadcrumbs behind on which test took a long time in the past, in case the problem re-surface. Alternatively, a log of test runs should also quickly provide the same information. I'll remove the Skip statement entirely
|
Follow up issue on testing: #2043 |
Change Overview
Upgrading
aws-sdk-gofromv1.44.244tov1.44.2501 exposed failures inawsebs.SanpshotCopy()`. The upgrade was subsequently reverted in order to mitigate errors while waiting for root cause analysis.This PR has the following changes:
And root cause has been found: A recent update in
aws-sdk-gov1.44.247has made it explicit that client's config region is used asDestinationRegionfor AWS Copy Snapshot operation. This means that whenCopySnapshotRequest()is made to create apresignedURL, the call needs to be made with the destination's client. This PR fix that forawsebsclient .As a result,
aws-sdk-gocan now be upgraded tov1.44.250Lastly, the error mentioned above was not caught in unit testing because
blockstorage_test.BlockStorageProviderSuite.TestSnapshotCopy()was skipped due to the following reason. This PR commented out the skip statement. The test took between 10-15s across 3 runs.Note that the entire
BlockStorageProviderSuitetest are currently skipped bymake testdue to lack of env config. Due to the test's nature of calling external provider's API, they should be placed in integration tests instead of unit tests. Issue is created to track: #2043.As a result,
blockstorage_test.BlockStorageProviderSuite.TestSnapshotCopy()is not run at the moment even with the change of this PRPull request type
Please check the type of change your PR introduces:
Test Plan