Issue 4 #34

despot · 2020-10-01T23:23:59Z

issue-4 Fix export to s3 of rds snapshot in regions that doesn't support export to s3, by copying snapshot to a region that does support export to s3 and exporting to the export supported region s3 bucket. Non-cluster part.

Type of change :
- New feature
- Bug fix for existing feature
- Code quality improvement
- Addition or Improvement of tests
- Addition or Improvement of documentation

When region supports export (the default/old command), the flow is 1-2-8-9-10-11 (check state numbers in the image below).
This PR is for when the region doesn't support an export (the additional new command). The flow then is 1-2-3-4-5-6-7-8-9-10-11.

…ort export to s3, by copying snapshot to a region that does support export to s3 and exporting to the export supported reagion s3 bucket. Non-cluster part. No tests.

# Conflicts: # README.md # src/common/common.zip # src/common/python/constants.py # src/common/python/utility.py # src/export/export_snapshot_s3_function.py # template.yaml

…ort export to s3, by copying snapshot to a region that does support export to s3 and exporting to the export supported reagion s3 bucket. Non-cluster part. No tests.

# Conflicts: # README.md # template.yaml

despot · 2020-10-01T23:25:00Z

@namitad @stationeros @RiyaJohn kindly review? It refers to the issue-4 comment: #4 (comment)

ghost · 2020-10-11T20:56:00Z

@despot Could you check the failing tests and I would suggest to add a more meaningful description to the PR title before we start reviewing it.

…tributeError: module 'mock_utility' has no attribute 'eval_export_exception'"

… not have the attribute 'supports_snapshot_export_region'" and "has no attribute 'ExportSnapshotSupportedRegionNotProvidedException'"

despot · 2020-10-13T22:32:25Z

@despot Could you check the failing tests and I would suggest to add a more meaningful description to the PR title before we start reviewing it.

@stationeros done.

namitad · 2020-10-14T10:55:02Z

src/checkstatus/get_dbstatus_function.py

@@ -5,10 +5,13 @@

 def lambda_get_dbinstance_status(event, context):
    """Method to obtain status of a RDS instance post actions such as snapshot creation, rename, restore or delete"""
-    region = os.environ['Region']
+    taskname = event['output']['taskname']
+    if taskname == constants.COPY_SNAPSHOT:


can this be moved to a separate method and also simplified using a ternary operator

Hi @namitad

I am including the ternary operator as I agree it might be more elegant that way. Though I see redundancy of adding additional structure for a separate method for a one-liner. I will kindly leave the line without creating a new method. Please let me know if there is some additional benefit that I didn't notice.

@despot presently this method is polluted with the logic of client creation based on region when the responsibility of the method should be to get the status. Moving it to a separate method will promote separation of concerns

@namitad I agree creation logic can be viewed as separation of concern, though it doesn't seem extracting a method for a one liner is a good idea. I will do it anyway since you are insisting.

namitad · 2020-10-14T10:55:51Z

src/checkstatus/get_dbstatus_function.py

@@ -20,7 +23,7 @@ def lambda_get_dbinstance_status(event, context):

 def eval_dbinstance_status(rds_client, context, taskname, identifier):
    max_attempts = util.get_waiter_max_attempts(context)
-    if taskname == constants.SNAPSHOT:
+    if taskname == constants.SNAPSHOT or taskname == constants.COPY_SNAPSHOT:


convert into a list and check using in clause

ok changing it as more elegant, although lists imho are required if there is more than 2 elements. Here seemed a bit unnecessary as you explicitly see what the condition is. But anyway, changing it.

@despot this doesn't seem unnecessary to me. I think what @namitad is suggesting here is a construct as follows.
if taskname in [constants.SNAPSHOT, constants.COPY_SNAPSHOT]
which is good since it allows for future proofing the conditional and reducing if else ladders.

@stationeros yes, I already did this as agreed.

namitad · 2020-10-14T10:56:26Z

src/common/python/constants.py

@@ -15,6 +15,8 @@
 DB_RESTORE = "Restore"
 CLUSTER_RESTORE = "ClusterRestore"
 EXPORT_SNAPSHOT = "SnapshotExport"
+EXPORT_SNAPSHOT_SUPPORT = "SnapshotExportSupportedTask"


please follow the naming convention and remove the Task suffix

namitad · 2020-10-14T11:16:05Z

src/common/python/utility.py

+    try:
+        rds.describe_export_tasks()
+    except Exception as error:
+        return "InvalidParameterCombination) when calling the DescribeExportTasks operation: This operation is not currently supported." not in str(error)


can this be moved to a constant

good catch - will change.

namitad · 2020-10-14T11:24:14Z

src/common/python/utility.py

+
+
+def eval_export_exception(export_snapshot_supported_region):
+    if export_snapshot_supported_region == "" or export_snapshot_supported_region.isspace():


can use if(not (str and not str.isspace())) or similar constructs

ok: will change str == "" to not str

namitad · 2020-10-14T11:31:23Z

src/export/copy_across_regions/check_region_support_snapshot_export_to_s3_function.py

+    return result
+
+
+def snapshot_present_in_snapshot_export_to_s3_supporting_region(instance_id, region):


can this be more concise

@namitad I am all for being as concise as possible to the point where you don't loose information. The result is what you see. I don't believe it can be shorter. Can you give me an example without loosing the essence? Otherwise let's keep it as it is.

@despot I agree with @namitad here the method name is quite superflous. The method returns a boolean so ideally should start with "is" . Also "export_to_s3" is something the method isn't doing. All the method does is just check if the snapshot is available in a region. How about "is_snapshot_available" or you can append region to that although regions are something inherent to a cloud provider.

@stationeros

"The method returns a boolean so ideally should start with "is""
This I agree with. Will apply this part.

"Also "export_to_s3" is something the method isn't doing."
The part export_to_s3 doesn't refer to exporting. You should read the whole part "snapshot_export_to_s3_supporting_region". It refers to a region_that_supports_snapshot_export_to_s3. I can change it to this as well, but thought mine had less words.
If I place os.environ['ExportSnapshotSupportedRegion'] inside the method, so less parameters, than the name would need to be is_snapshot_available_in_region_that_supports_snapshot_export_to_s3 , though I am going to leave os.environ['ExportSnapshotSupportedRegion'] where it is as you didn't mention anything about this part, and change the method name to is_snapshot_available . "2 hard things in computer science caching and naming things." :)

namitad · 2020-10-14T11:43:03Z

src/export/copy_across_regions/copy_snapshot_to_s3_export_supportable_region_function.py

+    snapshot_arn = util.get_instance_snapshot_arn(snapshot_id, region)
+    export_snapshot_supported_region = os.environ['ExportSnapshotSupportedRegion']
+    util.eval_export_exception(export_snapshot_supported_region)
+    export_snapshot_supported_region_rds = boto3.client('rds', export_snapshot_supported_region)


can the region check and rds client creation be moved to the copy_db_snapshot method instead of passing many parameters

@namitad unfortunately not. Actually the rds.copy_db_snapshot should be invoked within the parent method (only the parent method should exist). But If I leave it there there is no way to mock it(only at runtime you know the response type and its parameters of the boto3 call), so I can't test anything that happens after the rds.copy_db_snapshot method is invoked. Hence the additional dedicated method. If I follow your logic, even if I place all the statements (to reduce the parameters), we will end up with the same problem of mocking (there will be no tests). So if you can show me how we can do find out the runtime response type within the tests, I would happily do it. Otherwise lets leave it like this as UTs need beat parameter inconvenience.

@despot i assume by response type and runtime parameters for boto3, you mean the response for the rds.copy_db_snapshot calls. these can be mocked as part of UTs. For example: https://github.com/intuit/Trapheus/blob/master/tests/unit/text_export_snapshot_s3_function.py#L25
wouldnt this solve the issue of having another method and remove the parameter inconvenience?
Let me know if my understanding is correct.

@namitad yes that is what I meant. Thanks for the direction. I tried it (created a branch issue-4-parameters-fix-attempt with the new test and main code).
, and I got:
mock_export_snapshot_supported_region_rds.copy_db_snapshot.return_value = self.mocked_copy_db_snapshot_response_good AttributeError: 'str' object has no attribute 'copy_db_snapshot'

Feel free to amend it or let me know what exactly is the problem. Since I follow the other test code by the letter and it seems like the test doesn't treat it as mock but like a string. Otherwise, let's leave it as is?

namitad · 2020-10-14T11:43:55Z

src/common/python/constants.py

@@ -34,6 +36,9 @@
 WAITER_FAILURE = "Waiter encountered a terminal failure state"
 RATE_EXCEEDED = "Rate exceeded"
 WAITER_MAX = "Max attempts exceeded"
+EXPORT_FROM_REGION_THAT_SUPPORTS_SNAPSHOT_EXPORT_TO_S3 = "export_from_region_that_supports_snapshot_export_to_s3_task"


please convert all the namings to be more concise

@namitad I am all for being as concise as possible to the point where you don't loose information. The result is what you see. I don't believe it can be shorter. Can you give me an example without loosing the essence? Otherwise let's keep it as it is.

@despot The naming convention here confuses me slightly. Ideally are we saying we are exporting from a region or exporting to a region. Also your LHS and RHS looks almost the same which is concerning. Can we try something of the sort "EXPORT_SNAPSHOT_TO_SUPPORTED_REGION" which i feel is more comprehensible.

Again :) "2 hard things in computer science caching and naming things." :) but ok, let me try to clarify and we maybe find something more convenient to all.

"Ideally are we saying we are exporting from a region or exporting to a region"?
It is more "from" than "to", but perhaps more appropriate is "in" (both the snapshot and the s3 have to be in the same region). It refers to (the full name would be) EXPORT_SNAPSHOT_TO_S3_IN_REGION_THAT_SUPPORTS_SNAPSHOT_EXPORT_TO_S3 .

"Also your LHS and RHS looks almost the same which is concerning."
Ok, will change the RHS to "Case of exporting snapshot to s3 in a region that supports snapshot export to s3."

"Can we try something of the sort "EXPORT_SNAPSHOT_TO_SUPPORTED_REGION" which i feel is more comprehensible."
No - kindly check the above 2 points.

Does this make sense @namitad @stationeros ? Let me know if still concerns.

namitad · 2020-10-14T11:48:32Z

template.yaml

+      Role: !GetAtt LambdaExecutionRole.Arn
+      Runtime: python3.7
+      CodeUri: src/export/copy_across_regions/
+      Handler: copy_snapshot_to_s3_export_supportable_region_function.lambda_copy_snapshot_to_s3_export_supportable_region


more concise naming

@namitad I am all for being as concise as possible to the point where you don't loose information. The result is what you see. I don't believe it can be shorter. Can you give me an example without loosing the essence? Otherwise let's keep it as it is.

@despot considering that the method basically does copy of a snapshot from one region to another, can the naming be kept generic such as copy_snapshot_to_target_region_function and lambda_copy_snapshot_to_target_region

@namitad
"from one region to another"
The boto3 call is only generic. The method copies to the 'ExportSnapshotSupportedRegion' only. Changing it to your proposal misinterprets the method. I wouldn't like people to place non-supportable regions in 'ExportSnapshotSupportedRegion' so they can use it as generic or place logic in the method that is not about exporting to the supportable region. Once a generic use case appears, the appropriate generic method can be created. For now, I believe, the name you suggested is taking us to a wrong direction.

Do you want me to change
copy_snapshot_to_s3_export_supportable_region_function
to
copy_snapshot_to_region_that_supports_export_to_s3_function
?
Otherwise let's please leave it as is.

ghost · 2020-10-17T12:29:05Z

src/export/copy_across_regions/check_region_support_snapshot_export_to_s3_function.py

+    return result
+
+
+def snapshot_present_in_snapshot_export_to_s3_supporting_region(instance_id, region):


@despot I agree with @namitad here the method name is quite superflous. The method returns a boolean so ideally should start with "is" . Also "export_to_s3" is something the method isn't doing. All the method does is just check if the snapshot is available in a region. How about "is_snapshot_available" or you can append region to that although regions are something inherent to a cloud provider.

ghost · 2020-10-17T12:50:09Z

src/common/python/constants.py

@@ -34,6 +36,9 @@
 WAITER_FAILURE = "Waiter encountered a terminal failure state"
 RATE_EXCEEDED = "Rate exceeded"
 WAITER_MAX = "Max attempts exceeded"
+EXPORT_FROM_REGION_THAT_SUPPORTS_SNAPSHOT_EXPORT_TO_S3 = "export_from_region_that_supports_snapshot_export_to_s3_task"


@despot The naming convention here confuses me slightly. Ideally are we saying we are exporting from a region or exporting to a region. Also your LHS and RHS looks almost the same which is concerning. Can we try something of the sort "EXPORT_SNAPSHOT_TO_SUPPORTED_REGION" which i feel is more comprehensible.

ghost · 2020-10-17T18:52:46Z

src/checkstatus/get_dbstatus_function.py

@@ -20,7 +23,7 @@ def lambda_get_dbinstance_status(event, context):

 def eval_dbinstance_status(rds_client, context, taskname, identifier):
    max_attempts = util.get_waiter_max_attempts(context)
-    if taskname == constants.SNAPSHOT:
+    if taskname == constants.SNAPSHOT or taskname == constants.COPY_SNAPSHOT:


@despot this doesn't seem unnecessary to me. I think what @namitad is suggesting here is a construct as follows.
if taskname in [constants.SNAPSHOT, constants.COPY_SNAPSHOT]
which is good since it allows for future proofing the conditional and reducing if else ladders.

…hot to s3 in a region that supports it in intuit#34

despot · 2020-10-17T20:01:57Z

@stationeros
can you kindly help me figure out why CircleCI fails my changes ? It doesn't seem related to the changes I did. Perhaps it is related to some other commit or infrastructure change? If so, can you or the initiator kindly fix this?

When running tests locally in intellij idea, I get
" File "C:\Users\Despot\IdeaProjects\Trapheus\tests\unit\test_slack_notification.py", line 6, in
from requests.exceptions import HTTPError
ModuleNotFoundError: No module named 'requests'"
which again is not something related to my code.

- fix in template.yaml for requested constants changes from a previous commit on non-clustered exporting of snapshot to s3 in a region that supports it in intuit#34 - additional prerequisites in README.md that were missed to be placed for the "non-clustered exporting of snapshot to s3 in a region that supports it" case. - cluster part for "exporting of snapshot to s3 in a region that supports it" case

despot · 2020-10-19T10:01:58Z

@stationeros @namitad this last commit contains the additional fixes that we agreed for the non-clustered part, and additionally it contains the clustered counterpart as well.
Can you kindly review and accept this PR before there are other commits piled up master?

namitad · 2020-10-19T18:07:25Z

template.yaml

+                  "Variable": "$.taskname",
+                  "StringEquals": "Case of exporting snapshot to s3 in a region that supports snapshot export to s3.",
+                  "Next": "StartClusterSnapshotExportTask"
+                },


@despot both the above choices lead to the StartSnapshotExportTask execution, can we check if the options be combined?
in the case of copy to specific region before export, can we check in the lambda itself regarding the branching/ exact task executed

@namitad
"both the above choices lead to the StartSnapshotExportTask execution, can we check if the options be combined?"
ok will combine the first two "StringEquals" in an "Or" condition.

"in the case of copy to specific region before export, can we check in the lambda itself regarding the branching/ exact task executed"
copy lambda does the copy from one region to another and takes the system to the next state GetDbSnapshotStatus. What do you mean by "branching/exact task executed" in copy lambda?

@despot what i meant is, if we need to specify if it was directly exported from the source region or copied to a export_to_s3 supported region and then exported, this specification can be handled in the lambda if needed and
the above 2 choices can be combined

@namitad unfortunately it can't.

If you checked the template.yaml, DbSnapshotCopy(step function of type Task) is parallel to StartSnapshotExportTask(step function of type Task). Before these two tasks come CheckRegionSupportSnapshotExportToS3(step function of type Task) and DoesRegionSupportSnapshotExportToS3(step function of type Choice). See the left most part of the attached image.

And also consider that after DbSnapshotCopy the status(GetDbSnapshotStatus) and availability(isDbSnapshotAvailable) of the snapshot should be checked. See the upper part of the attached image.

To answer you specific question, what you want to do is place DbSnapshotCopy between isDbSnapshotAvailable and StartSnapshotExportTask by including CheckRegionSupportSnapshotExportToS3 and DoesRegionSupportSnapshotExportToS3 in DbSnapshotCopy. This can't be done because of 3 reasons:

imho, the state machine should not pass copy of db snapshot (DbSnapshotCopy), if the region is supporting exporting (to reach StartSnapshotExportTask).

It becomes a router on top of a copy task (the states should do one job only - separation of concerns)

you can't combine an aws step function of type="Task" and type="Choice" at least not from what I found online.

If you agree with the above, kindly push the PR forward as it is waiting for 3 weeks.

If you disagree and you have a workable solution for this case, kindly lets meet and peer program (XP) as going back to the context of this PR is taking too much time for me (and I guess for you as well). Additionally I would suggest we wrap this last opened review issue, and not create new ones on the old code, as it takes time to reintegrate every new cosmetic fix and acceptance test it all (this last part takes money as well). For now, I would suggest, if the features work and you find another tech debt issue, to create a tech debt issue and accept / push the PR forward. In future, I would suggest doing a full review in order to avoid the time and money issues above. I am more than glad to help - don't get me wrong, but we have to be a bit pragmatic as well. I hope you understand.
If there is anything else that you want to discuss before pushing the PR forward, kindly create a meeting call calendar invite (my email is despot dot jakimovski at gmail). Kindly, either place a link to the conference call or give me a skype hook or similar in the invite). I am available from UTC: 7am-10am or 5pm-8pm tomorrow(Saturday) and Sunday. Kindly schedule the call a day before preferably (at least half a day before).

@despot i am not asking to combine CheckRegionSupportSnapshotExportToS3 and DoesRegionSupportSnapshotExportToS3 in DbSnapshotCopy
I would definitely want the SnapshotExportSupported and Case of exporting snapshot to s3 in a region that supports snapshot export to s3. choices to be merged.
I am merely wondering in the StartClusterSnapshotExportTask or the instance snapshot export task, there is a logging which specifies which if the snapshot was directly exported or copied and then exported (since you added 2 choices explicitly). But as i understand from the template.yml, the DbSnapshotCopy should give that info. Let me know if that understanding is correct.
Also, do add the updated state machine screenshot in the PR description because the number of additional steps is a lot more.

@namitad
"I would definitely want the SnapshotExportSupported and Case of exporting snapshot to s3 in a region that supports snapshot export to s3. choices to be merged."
This can't be done and I explained in my above comment why. And you are, kindly, not giving any explicit steps on how this can be performed with the existing limits.

"I am merely wondering in the StartClusterSnapshotExportTask or the instance snapshot export task, there is a logging which specifies which if the snapshot was directly exported or copied and then exported (since you added 2 choices explicitly)."
Start(Cluster)SnapshotExportTask tasks export from the appropriate region, which depends on the case constants.EXPORT_SNAPSHOT_TO_S3_IN_REGION_THAT_SUPPORTS_SNAPSHOT_EXPORT_TO_S3 , yes (check export_snapshot_s3_function.py).
"But as i understand from the template.yml, the DbSnapshotCopy should give that info."
No. The case constants.EXPORT_SNAPSHOT_TO_S3_IN_REGION_THAT_SUPPORTS_SNAPSHOT_EXPORT_TO_S3 is setup/given by the CheckRegionSupportSnapshotExportToS3 task (check check_region_support_snapshot_export_to_s3_function.py)
Kindly check the picture again.
Please setup a call if you still believe this can be changed. We kindly can't compress 10min of required conversation in few comments. It is taking too much time and it doesn't seem to be taking us to a feasible solution.

As a final note, the case constants.EXPORT_SNAPSHOT_TO_S3_IN_REGION_THAT_SUPPORTS_SNAPSHOT_EXPORT_TO_S3 refers to the case when the default region is not supporting snapshot export to s3 so a copy was done to another region and the constant is propagated to notify the choice task whether to start an export from the appropriate region or copy it first. Again, kindly check the image.

"Also, do add the updated state machine screenshot in the PR description because the number of additional steps is a lot more."
Will do. Lets please first come to an understanding about the rest of this review issue. Kindly setup a call for more.

Hi @namitad
cc: @stationeros

if you don't schedule a call today or tomorrow, I will assume you understand and accept the limits, and I will fix:

the default parameters issue, and

"add the updated state machine screenshot"
and acceptance test all of this in one go. After the push of this PR, I will expect that it will be accepted if the points 1. and 2. are working, as previously explained otherwise it will take a lot more time and money to go through another round of fixes and acceptance testing.

Kind Regards,
Despot

- fix in template.yaml for requested constants changes from a previous commit on non-clustered exporting of snapshot to s3 in a region that supports it in intuit#34 - additional prerequisites in README.md that were missed to be placed for the "non-clustered exporting of snapshot to s3 in a region that supports it" case. - cluster part for "exporting of snapshot to s3 in a region that supports it" case

- combine the 2 StringEquals in one "Or" condition in the template.yaml state since leading to the same next state. - implementing a new get region method for region creation logic to separate concerns

namitad · 2020-10-23T12:49:44Z

@despot on trying to deploy this CFT with default set of parameters, we see the following error which breaks the deployment
Error: Failed to create changeset for the stack: trapheus, ex: Waiter ChangeSetCreateComplete failed: Waiter encountered a terminal failure state Status: FAILED. Reason: Template error: Fn::Not requires a list argument with one function token.
Have you tried the default working of the application without the additional parameters added as part of this PR?
cc: @stationeros

despot · 2020-10-23T13:06:14Z

@despot on trying to deploy this CFT with default set of parameters, we see the following error which breaks the deployment
Error: Failed to create changeset for the stack: trapheus, ex: Waiter ChangeSetCreateComplete failed: Waiter encountered a terminal failure state Status: FAILED. Reason: Template error: Fn::Not requires a list argument with one function token.
Have you tried the default working of the application without the additional parameters added as part of this PR?
cc: @stationeros

Hi @namitad
cc: @stationeros

what are the default set of parameters? Can you kindly send me the full command that you are using?

Yes I tried the common use case of create_snapshot and it was working. Perhaps the last changes opened up some issue. For future, another reason why the 3 E2E test cases ([Testing] issues) should be implemented as soon as possible in order to enable better iterative approach. For now, I will check and fix this tonight if this is the last review issue you see. If you still want me to change some of the other existing review issues you raised, kindly set up a call (instructions at the end of the other comment), so I can fix and test everything in one go after our call.

Kind Regards,
Despot

namitad · 2020-10-23T13:25:43Z

@despot the below is what was tried
sam deploy --template-file deploy.yaml --stack-name trapheus --region us-west-2 --s3-bucket trapheus.s3 --capabilities CAPABILITY_NAMED_IAM --parameter-overrides vpcId=<vpcid> Subnets=<subnet1,subnet2> SenderEmail=abc@gmail.com RecipientEmail=def@gmail.com

- fixing template.yaml for "Error: Failed to create changeset for the stack: trapheus, ex: Waiter ChangeSetCreateComplete failed: Waiter encountered a terminal failure state Status: FAILED. Reason: Template error: Fn::Not requires a list argument with one function token."

despot · 2020-10-25T22:01:18Z

@despot the below is what was tried
sam deploy --template-file deploy.yaml --stack-name trapheus --region us-west-2 --s3-bucket trapheus.s3 --capabilities CAPABILITY_NAMED_IAM --parameter-overrides vpcId=<vpcid> Subnets=<subnet1,subnet2> SenderEmail=abc@gmail.com RecipientEmail=def@gmail.com

Fix is in (the issue arose in the previous commit). I acceptance tested it and it is working now.
@namitad kindly review?

ghost · 2020-11-04T16:13:46Z

@despot Can we meet on this PR coming Saturday at 5PM your time UTC. Have sent you the invite.

despot · 2020-11-05T13:45:14Z

@despot Can we meet on this PR coming Saturday at 5PM your time UTC. Have sent you the invite.

Hi @stationeros
(CC: @namitad )

I would kindly suggest either accepting or rejecting this PR. If the meeting is informative, without the expectation of any new changes, then let us all attend. Otherwise, kindly, either accept the PR and open a new issue for changes or reject it completely.
Respectfully, I already gave a lot of explanations and time (3 weeks), I gave a week notice if a call was needed so we don't type (and I don't spend time and money on acceptance testing changes) before I do the last changes, and for that reason, I believe it is not fair for me to do any more changes on this PR.

If you feel changes are needed and you don't want to accept the PR as it is and create another issue for required changes, then feel free to cancel the call and reject the PR. No hurt feelings.

Kind Regards,
Despot

ghost · 2020-11-05T17:19:33Z

@despot We tested the PR and wanted to check with you on the following. As per the following documentation
https://docs.aws.amazon.com/AmazonRDS/latest/UserGuide/USER_ExportSnapshot.html
Export is supported in all regions except this.
AWS GovCloud (US-East)
AWS GovCloud (US-West)
China

So we could not test the copy part which is a key objective of this PR. We were able to export snapshots to all regions except the ones mentioned above

despot · 2020-11-06T08:52:43Z

@despot We tested the PR and wanted to check with you on the following. As per the following documentation
https://docs.aws.amazon.com/AmazonRDS/latest/UserGuide/USER_ExportSnapshot.html
Export is supported in all regions except this.
AWS GovCloud (US-East)
AWS GovCloud (US-West)
China

So we could not test the copy part which is a key objective of this PR. We were able to export snapshots to all regions except the ones mentioned above

Hi @stationeros
(CC: @namitad )

just saw it. If you remember the unsupported regions were like 80%. Now it is left to 3 unsupported regions. Until we develop this feature and review it, AWS changed the policy. Pity.

In theory,

This feature will work for the 2 GovCloud regions. I am sure about it, as I tested it before, but to test it now would mean creating a GovCloud account which I think is not easy to do.
The China region doesn't appear for me. A person needs to satisfy some specific China account details so they are eligible for creation of a China account. Additionally even if we are able to do that to test it, for China use case, I believe since having 2 accounts, there needs to be an additional PR (additional credentials and KMS keys I would assume) in order to work. So, as it is now, it is not applicable for the China use case.

So, if you want GovCloud users to have the possibility of exporting the snapshot to S3, then you can accept the PR for those users. And if any GovCloud user starts using this and has additional questions, we could then maybe ask for a test account through these users.
Otherwise, you reject the PR. Pity for all the work, but it is, what it is.

Kind Regards,
Despot

despot added 8 commits September 26, 2020 09:53

issue-4 Fix export to s3 of rds snapshot in regions that doesn't supp…

b966e8b

…ort export to s3, by copying snapshot to a region that does support export to s3 and exporting to the export supported reagion s3 bucket. Non-cluster part. No tests.

issue-4 Fix export to s3 of rds snapshot in regions that doesn't supp…

d99a59f

…ort export to s3, by copying snapshot to a region that does support export to s3 and exporting to the export supported reagion s3 bucket. Non-cluster part. No tests.

Merge remote-tracking branch 'origin/issue-4' into issue-4

9e34879

# Conflicts: # README.md # src/common/common.zip # src/common/python/constants.py # src/common/python/utility.py # src/export/export_snapshot_s3_function.py # template.yaml

issue-4 unit tests for non-cluster lambdas

5328187

issue-4 Fix export to s3 of rds snapshot in regions that doesn't supp…

1ddd636

…ort export to s3, by copying snapshot to a region that does support export to s3 and exporting to the export supported reagion s3 bucket. Non-cluster part. No tests.

issue-4 Fix export to s3 of rds snapshot in regions that doesn't supp…

4130011

…ort export to s3, by copying snapshot to a region that does support export to s3 and exporting to the export supported reagion s3 bucket. Non-cluster part. No tests.

issue-4 unit tests for non-cluster lambdas

bfcf2ab

Merge remote-tracking branch 'origin/issue-4' into issue-4

271ad64

# Conflicts: # README.md # template.yaml

despot requested a review from namitad as a code owner October 1, 2020 23:23

despot mentioned this pull request Oct 1, 2020

Add support for moving the snapshot to a S3 bucket #4

Closed

despot added 4 commits October 13, 2020 22:18

Merge branch 'master' into issue-4

af476fe

issue-4 unit tests for non-cluster lambdas - fix

846f552

issue-4 unit tests for non-cluster lambdas - fix for CircleCI's "E At…

290a0c4

…tributeError: module 'mock_utility' has no attribute 'eval_export_exception'"

issue-4 unit tests for non-cluster lambdas - fix for CircleCI's "does…

4b9c191

… not have the attribute 'supports_snapshot_export_region'" and "has no attribute 'ExportSnapshotSupportedRegionNotProvidedException'"

namitad requested changes Oct 14, 2020

View reviewed changes

Merge branch 'master' into issue-4

cc067dd

ghost suggested changes Oct 17, 2020

View reviewed changes

issue-4 fix for requested changes on non-clustered exporting of snaps…

d22dfcf

…hot to s3 in a region that supports it in intuit#34

ghost closed this Oct 18, 2020

ghost reopened this Oct 18, 2020

stationeros and others added 4 commits October 18, 2020 12:32

Update README.md

b4b4b48

Merge branch 'master' into issue-4

6c4901f

Merge remote-tracking branch 'origin/issue-4' into issue-4

210f369

despot mentioned this pull request Oct 19, 2020

Automate further the feature for exporting the snapshot to s3 in a region that doesn't support it. #47

Closed

namitad reviewed Oct 19, 2020

View reviewed changes

despot added 2 commits October 20, 2020 17:31

issue-4 fixes from review:

e5088aa

- combine the 2 StringEquals in one "Or" condition in the template.yaml state since leading to the same next state. - implementing a new get region method for region creation logic to separate concerns

Merge branch 'master' into issue-4

2a1fb92

despot added 2 commits October 25, 2020 22:43

Merge remote-tracking branch 'origin/issue-4' into issue-4

7bb8e5d

ghost closed this Nov 30, 2020

This pull request was closed.



		def eval_export_exception(export_snapshot_supported_region):
		if export_snapshot_supported_region == "" or export_snapshot_supported_region.isspace():

		return result


		def snapshot_present_in_snapshot_export_to_s3_supporting_region(instance_id, region):

Issue 4 #34

Issue 4 #34

Conversation

despot commented Oct 1, 2020 • edited Loading

despot commented Oct 1, 2020

ghost commented Oct 11, 2020

despot commented Oct 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

despot Oct 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

despot commented Oct 17, 2020

despot commented Oct 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

despot Oct 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

namitad commented Oct 23, 2020 • edited Loading

despot commented Oct 23, 2020 • edited Loading

namitad commented Oct 23, 2020

despot commented Oct 25, 2020 • edited Loading

ghost commented Nov 4, 2020

despot commented Nov 5, 2020

ghost commented Nov 5, 2020 • edited by ghost Loading

despot commented Nov 6, 2020

despot commented Oct 1, 2020 •

edited

Loading

despot Oct 17, 2020 •

edited

Loading

despot Oct 23, 2020 •

edited

Loading

namitad commented Oct 23, 2020 •

edited

Loading

despot commented Oct 23, 2020 •

edited

Loading

despot commented Oct 25, 2020 •

edited

Loading

ghost commented Nov 5, 2020 •

edited by ghost

Loading