Adding support for specifying storageClass type #906
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nrajb
requested review from
aws-ia-ci,
parkand1,
shapirov103,
youngjeong46,
elamaran11 and
a team
as code owners
|
output of lint, build and test: |
1 task
|
@nrajb I see some GH Action warnings, can you fix those? Also can you update the documentation to emphasize on GP3 support. |
shapirov103
requested changes
Jan 12, 2024
There was a problem hiding this comment.
@nrajb thank you for the contribution, I believe the functionality is valuable.
The way the implementation is done, there a bunch of things that are incorrect atm. I added some minor feedback but let's address the design first.
Core requirements:
- The core ebs-csi addon must be deployed. We don't want the default csi driver to be there at all. Atm, I believe it is just the default csi driver that will be running.
- The return value of the addon must be the addon itself or a construct that depends on the addon.
Explanation:
- You never call super.deploy so the actual ebs csi addon does not get a chance to deploy. I believe your patches are applied to the default storage classes supplied with the cluster creation.
- You return arbitrary things from the deploy method. What we return from this method is used for dependencies. So if I have another dependent on your EBS CSI driver, the expectation is that the CfnAddOn is done, before another addon can install. In your case you return the patch or noop construct.
- The patch and the storage class creation manifest do not depend on anything. they can fire arbitrarily in any order. Expectation: the patch and SC creation happens AFTER the ebs-csi addon is installed. I expect to see this dependency.
These are the major points, happy to answer any questions.
shapirov103
reviewed
Feb 5, 2024
There was a problem hiding this comment.
@nrajb looks way better, I added some tactical feedback mostly minor, one functional issue was identified, please address.
|
@nrajb let's address the remaining feedback, it would be helpful to include this in 1.14 release. |
shapirov103
requested changes
Feb 19, 2024
There was a problem hiding this comment.
@nrajb looks great, however please check my comments, you probably inadvertently removed the existing versionMap functionality that we rely on.
Comment on lines
+37
to
+38
| saName: "ebs-csi-controller-sa", | ||
| storageClass: "gp3", // Set the default StorageClass to gp3 |
There was a problem hiding this comment.
@nrajb you removed versionMap from the default props, it has impact on how this addon will behave during build time.
please restore, it will also take care of the warning above stating the versionMap is not used.
There was a problem hiding this comment.
@nrjab please don't resolve conversation, it should be done by the reviewer, otherwise the comments are hidden.
| super({ | ||
| addOnName: defaultProps.addOnName, | ||
| version: options?.version ?? defaultProps.version, | ||
| saName: defaultProps.saName, |
There was a problem hiding this comment.
same here: versionMap is removed.
@elamaran11 FYI
| updateSc.node.addDependency(patchSc); | ||
|
|
||
| return updateSc; | ||
| } else |
There was a problem hiding this comment.
minor: else clause is redundant, since you are returning updateSc.
There was a problem hiding this comment.
So with the new implementation of async deploy(clusterInfo: ClusterInfo): Promise<Construct> in this proposed changes, if I don't handle return type without else I get following error for async deploy line:
Function lacks ending return statement and return type does not include 'undefined'.ts(2366)
Please suggest @shapirov103
There was a problem hiding this comment.
weird, it thinks that base deployment can be undefined. nm, as i said it is minor.
|
@nrajb Can you make progress with this PR so we can move towards release. Also we see |
|
Check Markdown links / markdown-link-check (pull_request) Failing after 1m Unrelated doc errors: |
shapirov103
reviewed
Feb 28, 2024
| updateSc.node.addDependency(patchSc); | ||
|
|
||
| return updateSc; | ||
| } else |
There was a problem hiding this comment.
weird, it thinks that base deployment can be undefined. nm, as i said it is minor.
|
/do-e2e-tests |
aws-ia-ci
requested changes
Feb 28, 2024
|
/do-e2e-tests |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue #, if available:
#896
Description of changes:
Adds support for specifying storageClass and deploys required resources as part of post cluster steps
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.