Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Snapshot Resource changes for Ack #14

Merged

Conversation

AnilYalavarthi
Copy link

@AnilYalavarthi AnilYalavarthi commented Mar 17, 2022

Description of changes: Adding Snapshot Resource for MemoryDB

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Copy link
Contributor

@RedbackThomson RedbackThomson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anil, this is an amazing job! There has clearly been a lot of work put into this PR and it looks great. You definitely chose to tackle a harder API, given the complexity in the CopySnapshot option.

I've left you some comments inline regarding some ways we can save on custom code, and to keep things more inline with other ACK controllers.

KMSKeyID *string `json:"kmsKeyID,omitempty"`
// A name for the snapshot being created.
// +kubebuilder:validation:Required
SnapshotName *string `json:"snapshotName"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please rename this just Name? Use the S3 controller for reference

- SnapshotAlreadyExistsFault
- SnapshotQuotaExceededFault
- TagQuotaPerResourceExceeded
- ClusterNotFoundFault
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this from terminal. We want the controller to retry until the Cluster is found!

- SnapshotQuotaExceededFault
- TagQuotaPerResourceExceeded
- ClusterNotFoundFault
- InvalidClusterStateFault
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should remove this one as well, if there is a chance the cluster will recover into a good state.

from:
operation: CopySnapshot
path: SourceSnapshotName
ClusterName:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this field not automatically generated? Why do we have to add it to the list of fields?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is part of different API (copy-snapshot) so we would need to add it

Comment on lines 58 to 60
// The name of the snapshot
// +kubebuilder:validation:Optional
Name *string `json:"name,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This field should disappear if we rename the spec field - we don't want it duplicated across spec and status.

Comment on lines 1 to 2
//Not used currently but whenever SDKUpdate() is generated for non existing update() operation on resource this will be added then as override
return latest, nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're probably safe to just delete this for now. If update isn't supported yet, better to return the not implemented error.



@dataclass
class Snapshots(Bootstrappable):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job writing a Bootstrappable type! Really happy to see it being used.

This Bootstrappable is actually bootstrapping a MemoryDB cluster - so should probably be called Cluster

Suggested change
class Snapshots(Bootstrappable):
class Cluster(Bootstrappable):

Comment on lines +34 to +42
while True:
clusters = mdb.describe_clusters(ClusterName=self.snapshotClusterName)
cluster = clusters['Clusters'][0]
if cluster.get("Status").casefold() == available_status.casefold():
return True
if time.time() > timeout:
break
time.sleep(60)
raise ValueError('cluster not created within expected time')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ouch there are no waiters supported in boto3 for memorydb yet

@dataclass
class Snapshots(Bootstrappable):
# Output
snapshotClusterName: str
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Outputs need to be instantiated with an initialised value:

Suggested change
snapshotClusterName: str
cluster_name str = field(init=False)

Comment on lines 26 to 27
Snapshot1: Snapshots
Snapshot2: Snapshots
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason we need two clusters? Are we able to run both tests on a single cluster? It would certainly save us some time when bootstrapping.

Comment on lines 25 to 26
"KMSKEY": get_bootstrap_resources().KMSKey.key
"SNAPSHOT_CLUSTER_NAME1": get_bootstrap_resources().Cluster1.clusterName,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems e2e test failed due to missing comma after the KMSKEY statement.

tests/test_scenarios.py:29: in <module>
    from e2e.replacement_values import REPLACEMENT_VALUES
E     File "/memorydb-controller/tests/e2e/replacement_values.py", line 26
E       "SNAPSHOT_CLUSTER_NAME1": get_bootstrap_resources().Cluster1.clusterName,
E       ^
E   SyntaxError: invalid syntax

Comment on lines 26 to 27
"SNAPSHOT_CLUSTER_NAME1": get_bootstrap_resources().Cluster1.clusterName,
"SNAPSHOT_CLUSTER_NAME2": get_bootstrap_resources().Cluster1.clusterName
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should one of them be Cluster 2? We cannot take two snapshot from same cluster at same time.

@kumargauravsharma
Copy link
Contributor

e2e tests have passed, merging the PR.

@ack-bot
Copy link
Collaborator

ack-bot commented Apr 7, 2022

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: AnilYalavarthi, kumargauravsharma
To complete the pull request process, please assign redbackthomson after the PR has been reviewed.
You can assign the PR to them by writing /assign @redbackthomson in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kumargauravsharma kumargauravsharma merged commit 191109d into aws-controllers-k8s:main Apr 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants