Support for moving snapshots to s3 #5

RiyaJohn · 2020-08-03T18:17:19Z

Description

Thanks for contributing this Pull Request. Make sure that you submit this Pull Request against the master branch of this repository, add a brief description, and tag the relevant issue(s) and PR(s) below.
Fixes #4

Relevant Issues : Add support for moving the snapshot to a S3 bucket #4
Relevant PRs : (optional)
Type of change :
- New feature
- Bug fix for existing feature
- Code quality improvement
- Addition or Improvement of tests
- Addition or Improvement of documentation

namitad

Minor comments, please address.
Also, do add the unit test file for the newly added lambda

src/export/export_snapshot_s3_function.py

namitad · 2020-08-13T17:27:39Z

src/export/export_snapshot_s3_function.py

+    snap_arn = snap['DBSnapshotArn']
+    snap_status = snapshots[0]['Status']
+    if snap_status == 'available':
+        return snap_arn


can we directly not return snap['DBSnapshotArn'] if snapshot status is available?

ARN will be available even in creating state, couldn't place it inside if case with cluster support changes now. Let me know if its not ok

the above comment will address this issue as well i believe. do check and verify

RiyaJohn · 2020-09-07T12:34:33Z

@namitad resolved conflicts that came up. Do take a look the PR that has review changes and support for Cluster snapshots also added.

namitad · 2020-09-10T10:16:57Z

src/export/export_snapshot_s3_function.py

+    region = os.environ['Region']
+    rds = boto3.client('rds', region)
+    if is_cluster:
+        snapshots_response = rds.describe_db_cluster_snapshots(DBClusterSnapshotIdentifier=snapshot_name)


the pattern followed is to have separate py files for cluster and instance as they have completely independent branches and the isCluster check is added in the beginning itself.
Please check the pattern followed and move the cluster specific logic to its own file. Any common processing can be added in the common dir

refactored to have separate py files for cluster and instance, with common function in util file

namitad · 2020-09-10T10:18:04Z

src/export/export_snapshot_s3_function.py

+    snap_arn = snap['DBSnapshotArn']
+    snap_status = snapshots[0]['Status']
+    if snap_status == 'available':
+        return snap_arn


the above comment will address this issue as well i believe. do check and verify

despot · 2020-09-15T10:48:33Z

src/export/export_snapshot_s3_function.py

+        response = rds.start_export_task(
+            ExportTaskIdentifier=export_id,
+            SourceArn=snapshot_arn,
+            S3BucketName=bucket_name,


Why don't we use the same bucket name that was provided by the user in https://github.com/intuit/Trapheus/blob/master/README.md -> Instructions -> Setup -> point 2. ?

We can use that too, but that bucket is meant for Trapheus Cloudformation deployment files right. So I did not want to store the snapshots there. What do you guys think @namitad and @stationeros

Also not sure how we can get that bucket name in our lambdas, unless the user passes it in params, which I don't think should be done

s3 bucket name needs to be a input as its not part of the state machine while executing sam deploy

ghost · 2020-09-16T21:55:09Z

src/common/python/constants.py


 DELETE = "Delete"
 RENAME = "Rename"
 SNAPSHOT = "SnapshotCreation"
 DB_RESTORE = "Restore"
 CLUSTER_RESTORE = "ClusterRestore"
+EXPORT_SNAPSHOT = "SnapshotExportTask"


Can we remove the Task from SnapShotExportTask as the name doesn't follow the convention.

yes, will do

done,
4d22883

ghost · 2020-09-16T21:56:46Z

src/export/export_cluster_snapshot_s3_function.py

@@ -0,0 +1,52 @@
+import os
+import time
+


Remove whitespaces between imports

@stationeros So the blanks between imports are added by PyCharm as the import layout when you click optimize imports cmd

ghost · 2020-09-16T21:59:25Z

src/export/export_cluster_snapshot_s3_function.py

+    rds = boto3.client('rds', region)
+    snapshots_response = rds.describe_db_cluster_snapshots(DBClusterSnapshotIdentifier=snapshot_name)
+    assert snapshots_response['ResponseMetadata'][
+               'HTTPStatusCode'] == 200, f"Error fetching cluster snapshots: {snapshots_response}"


There is an illegal character "f" present in the line 44. Can you please check that.

RiyaJohn · 2020-09-18T11:36:31Z

src/export/export_cluster_snapshot_s3_function.py

+    assert snapshots_response['ResponseMetadata'][
+               'HTTPStatusCode'] == 200, f"Error fetching cluster snapshots: {snapshots_response}"
+    snapshots = snapshots_response['DBClusterSnapshots']
+    assert len(snapshots) == 1, f"More than one snapshot matches name {snapshot_name}"


I think @namitad already added info around it in the above comment. Just to add to it, F-strings have a shorter, more readable syntax, and is also much faster. Con is, it requires at least Python 3.6, since I saw Trapheus doc already had python 3.7 as a pre-req. I thought it wasn't a problem.

ghost · 2020-09-16T22:01:38Z

src/export/export_cluster_snapshot_s3_function.py

+    if snap_status == 'available':
+        return snap['DBClusterSnapshotArn']
+    else:
+        raise Exception(f"Snapshot is not available yet, status is {snap_status}")


Illegal character . Please check.

namitad · 2020-09-21T08:08:36Z

src/export/export_snapshot_s3_function.py

+    snapshot_id = instance_id + constants.SNAPSHOT_POSTFIX
+    snapshot_arn = get_instance_snapshot_arn(snapshot_id)
+    account_id = util.get_aws_account_id()
+    bucket_name = constants.RDS_SNAPSHOTS_BUCKET_NAME_PREFIX + account_id


@RiyaJohn is it assumed that the s3 bucket is already existing? and if it doesnt, the export task throws an exception?
can we add a testcase on this scenario?

Bucket will exist as the bucket is created as part of our deployment here

Trapheus/template.yaml

Line 370 in 4d22883

SnapshotsBucket:

. Don't remember if it will throw error or will create one, will test and update here. Also yes will add a test case for it

Doesn't throw an exception, when the bucket doesn't exist, as the deployment always creates one (which imo is correct (better)). Imo there is no need handling a case that will not appear (having no export bucket), as that would assume the user tempering manually. Usually system doesn't handle these scenarios. Anyway, just my 2 cents.

@despot agreed. If its being created as part of the deployment, i think we are ok. But considering a scenario that the s3 bucket can be deleted, do you think its good to have that handled?

@namitad thanks for considering my opinion. I was trying to find some articles about what i already practiced within the team in the past, though I would need to dig deeper. The idea is, that usually the application refers to doing one thing, and as soon as the team starts handling cases (including only exception handling) what a user can do outside of the prescribed way, the team goes into the direction of solving the problem of the universe. More pragmatically, the team wouldn't wanna handle someone deleting a lambda function or the kmsKeyId manually e.t.c. as this is not what they should do. So I don't think a test should be written for the user deleting the S3 bucket. It all comes down to what you want the application to do. Does a user deleting a S3 bucket a normal behavior that you expect to handle. I don't imagine this app like that. But if you do, then either handle the case (by, for instance, creating a new S3 bucket) or have a custom exception so you remember to do this in future. As I think this shouldn't be handled, imo @RiyaJohn should not do anything more regarding the S3 bucket. But again, you are leading the project :), so let us know.

Totally agree with @despot. But already pushed code for this test case, so @namitad you can take a look at it b4701c8

src/export/export_snapshot_s3_function.py

src/export/export_cluster_snapshot_s3_function.py

wip: export task initial

7d84a0d

RiyaJohn requested a review from namitad as a code owner August 3, 2020 18:17

RiyaJohn marked this pull request as draft August 3, 2020 18:17

RiyaJohn changed the title ~~WIP: export task to s3~~ WIP: Snapshot to s3 Aug 3, 2020

RiyaJohn added 3 commits August 9, 2020 21:09

feat: start export task

276939b

ci: add resources

ef34c77

fix: cluster to db case

2c86b7d

RiyaJohn changed the title ~~WIP: Snapshot to s3~~ Support for moving snapshots to s3 Aug 9, 2020

RiyaJohn added 2 commits August 10, 2020 19:30

fix: template and common.zip

980734c

docs: CFT update

7e86b7d

RiyaJohn marked this pull request as ready for review August 10, 2020 14:25

namitad requested changes Aug 13, 2020

View reviewed changes

RiyaJohn added 6 commits August 16, 2020 23:40

test: export ut

66e3908

review fixes

36f9e63

feat: add cluster snapshot export

7e0ab30

pass isCluster param to export sf

7796925

fix: use get()

92da5ae

Merge remote-tracking branch 'upstream/master'

c520370

namitad requested changes Sep 10, 2020

View reviewed changes

RiyaJohn added 4 commits September 11, 2020 21:33

refactor: separate for cluster export

57c6f10

refactor: rename arn

3004f47

fix: missing comma

8da0751

fix: use epoch over random str

464a618

despot suggested changes Sep 15, 2020

View reviewed changes

ghost reviewed Sep 16, 2020

View reviewed changes

refactor: remove Task suffix

4d22883

namitad reviewed Sep 21, 2020

View reviewed changes

test: add case when bucket doesnt exist

b4701c8

namitad requested changes Sep 29, 2020

View reviewed changes

src/export/export_snapshot_s3_function.py Outdated Show resolved Hide resolved

src/export/export_cluster_snapshot_s3_function.py Outdated Show resolved Hide resolved

rename msg, add func in mock_util, update readme

ea82f4c

namitad approved these changes Sep 29, 2020

View reviewed changes

namitad merged commit f2ac1c4 into intuit:master Sep 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for moving snapshots to s3 #5

Support for moving snapshots to s3 #5

RiyaJohn commented Aug 3, 2020 •

edited

Loading

namitad left a comment •

edited

Loading

namitad Aug 13, 2020

RiyaJohn Aug 23, 2020

namitad Sep 10, 2020

RiyaJohn commented Sep 7, 2020

namitad Sep 10, 2020

RiyaJohn Sep 14, 2020

namitad Sep 10, 2020

despot Sep 15, 2020 •

edited

Loading

RiyaJohn Sep 18, 2020

RiyaJohn Sep 18, 2020

namitad Sep 21, 2020

ghost Sep 16, 2020

RiyaJohn Sep 18, 2020

RiyaJohn Sep 18, 2020

ghost Sep 16, 2020

RiyaJohn Sep 18, 2020

ghost Sep 16, 2020

This comment was marked as outdated.

This comment was marked as outdated.

RiyaJohn Sep 18, 2020

ghost Sep 16, 2020 •

edited by ghost

Loading

namitad Sep 21, 2020 •

edited

Loading

RiyaJohn Sep 22, 2020

despot Sep 23, 2020 •

edited

Loading

namitad Sep 24, 2020

despot Sep 24, 2020

RiyaJohn Sep 25, 2020 •

edited

Loading

Support for moving snapshots to s3 #5

Support for moving snapshots to s3 #5

Conversation

RiyaJohn commented Aug 3, 2020 • edited Loading

Description

namitad left a comment • 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

RiyaJohn commented Sep 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

despot Sep 15, 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

This comment was marked as outdated.

This comment was marked as outdated.

Choose a reason for hiding this comment

ghost Sep 16, 2020 • edited by ghost Loading

Choose a reason for hiding this comment

namitad Sep 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

despot Sep 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RiyaJohn Sep 25, 2020 • edited Loading

Choose a reason for hiding this comment

RiyaJohn commented Aug 3, 2020 •

edited

Loading

namitad left a comment •

edited

Loading

despot Sep 15, 2020 •

edited

Loading

ghost Sep 16, 2020 •

edited by ghost

Loading

namitad Sep 21, 2020 •

edited

Loading

despot Sep 23, 2020 •

edited

Loading

RiyaJohn Sep 25, 2020 •

edited

Loading