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

AWS EBS: add default EBS volume types for provisioners #1931

Merged
merged 2 commits into from Oct 10, 2023

Conversation

junaid-ali
Copy link
Contributor

@junaid-ali junaid-ali commented May 18, 2023

What does this PR change?

In some cases, the type parameter might be missing in a StorageClass definition and defaults to a type determined by the CSI controller. For example, if we don't specify the type for EBS CSI controller, the default is gp3. Ref: https://github.com/kubernetes-sigs/aws-ebs-csi-driver/blob/master/docs/parameters.md

Does this PR relate to any other PRs?

How will this PR impact users?

Does this PR address any GitHub or Zendesk issues?

  • Closes ...

How was this PR tested?

unit tests

Does this PR require changes to documentation?

Have you labeled this PR and its corresponding Issue as "next release" if it should be part of the next Opencost release? If not, why not?

@junaid-ali
Copy link
Contributor Author

@michaelmdresser @nikovacevic if I can get a review please

@mattray mattray added the opencost OpenCost issues vs. external/downstream label May 22, 2023
@junaid-ali
Copy link
Contributor Author

I don't see any OWNERS for in pkg/cloud/aws directory so not sure who should I ask for review. @michaelmdresser @mbolt35 @Sean-Holcomb @nik-kc if you can have a look at this change please?

@junaid-ali
Copy link
Contributor Author

Closing this PR, since open source contributions don't seem to be welcomed. A review would have been appreciated if the change isn't worthy enough (apologies for using a bit strong words).

@junaid-ali junaid-ali closed this Jul 31, 2023
@cliffcolvin
Copy link
Member

Hello @junaid-ali,
I apologize for the lack of response on this PR. The team has been very busy with some major undertakings in the last 6 weeks and probably did not see the notifications. I am working on some processes to make sure these types of things don't get missed in the future. This PR is definitely something that we would be interested in taking into open cost. Setting the default storage class for when it isn't specified would probably add value. I can work with the team to get a plan around testing this and working to get it into one of the upcoming Open Cost releases. I will communicate after we finish the planning process with an expected timeline of when that could happen if you'd be interested in re-opening this PR. Another note, to better enable our processes if you would create an issue to be tied with this PR describing your issue it will help us to have an associated ticket to work through this item.

@junaid-ali
Copy link
Contributor Author

Thanks @cliffcolvin, reopened the PR.

@junaid-ali junaid-ali reopened this Aug 8, 2023
@junaid-ali
Copy link
Contributor Author

Re creating an issue, I should have created an issue. It's been a while I haven't looked at kubecost, but this debug message should be fixed for the known AWS k8s storage classes: https://github.com/opencost/opencost/blob/develop/pkg/cloud/aws/provider.go#L730

@cliffcolvin
Copy link
Member

@junaid-ali we are going to work to get this in 1.107

@mattray mattray added P1 Estimated Priority (P0 is highest, P4 is lowest) E2 Estimated level of Effort (1 is easiest, 4 is hardest) kubecost Relevant to Kubecost's downstream project labels Aug 31, 2023
Copy link
Contributor

@nikovacevic nikovacevic left a comment

Choose a reason for hiding this comment

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

I suspect this is fine, but I don't have an environment at-hand where this would be quick and simple to test. As I read it, at least, it should be backwards compatible with old behavior, but adds support where intended.

The unit tests are nice to have. I'd support merging and testing for regressions, but not worrying too much about setting up a specific env to test.

@nikovacevic
Copy link
Contributor

Hi @junaid-ali. We've approved this PR, and I've rebased it so that it's up-to-date atop the develop branch, but the DCO check is failing. You will need to sign your commits to resolve that. Can you do that for us? See "Details" on that check to learn more about signing commits.

In some cases, the `type` parameter might be missing and defaults to
a type determined by the CSI controller. For example, if we don't specify
the type `ebs.csi.aws.com` provisioner, the default is `gp3`.
Ref: https://github.com/kubernetes-sigs/aws-ebs-csi-driver/blob/master/docs/parameters.md

Signed-off-by: Junaid Ali <junaidali.yahya@gmail.com>
Signed-off-by: Junaid Ali <junaidali.yahya@gmail.com>
@junaid-ali
Copy link
Contributor Author

@nikovacevic done, my original commit had wrong author name

@nikovacevic
Copy link
Contributor

Fantastic, thank you @junaid-ali

@nikovacevic nikovacevic merged commit 9008fbd into opencost:develop Oct 10, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E2 Estimated level of Effort (1 is easiest, 4 is hardest) kubecost Relevant to Kubecost's downstream project opencost OpenCost issues vs. external/downstream P1 Estimated Priority (P0 is highest, P4 is lowest)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants