✨ Add ebs volume throughput field to v1alpha4 Volume #2468

cnmcavoy · 2021-06-03T20:51:49Z

What type of PR is this?
/kind feature

What this PR does / why we need it:

Adds configurable EBS volume throughput in AWS Machine Templates.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #2254

Special notes for your reviewer:
N/A

Adds the ability to configure ebs volume throughput in supported ebs types

k8s-ci-robot · 2021-06-03T20:51:56Z

Welcome @cnmcavoy!

It looks like this is your first PR to kubernetes-sigs/cluster-api-provider-aws 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/cluster-api-provider-aws has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2021-06-03T20:51:57Z

Hi @cnmcavoy. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

richardcase · 2021-06-03T20:57:33Z

/ok-to-test

richardcase

Thanks for this PR 👍

A couple of small comments.

api/v1alpha3/types.go

pkg/cloud/services/ec2/instances.go

sedefsavas · 2021-06-11T01:06:04Z

api/v1alpha4/awsmachine_webhook.go

@@ -149,6 +149,14 @@ func (r *AWSMachine) validateRootVolume() field.ErrorList {
 		allErrs = append(allErrs, field.Required(field.NewPath("spec.rootVolumeOptions.iops"), "iops required if type is 'io1' or 'io2'"))
 	}

+	if r.Spec.RootVolume.Type == "gp2" && r.Spec.RootVolume.Throughput != 0 {


We don't only support gp2 AFAIK.
Please take a look at the webhook validation check in this PR for gp3: https://github.com/kubernetes-sigs/cluster-api-provider-aws/pull/2496/files

thanks! you are correct, I forgot that gp2 support is provided by the legacy aws provider in kubernetes.

sedefsavas · 2021-06-29T18:09:48Z

@cnmcavoy will you have time to rebase this PR and do some changes that are in https://github.com/kubernetes-sigs/cluster-api-provider-aws/pull/2496/files?

cnmcavoy · 2021-06-29T18:43:38Z

@cnmcavoy will you have time to rebase this PR and do some changes that are in https://github.com/kubernetes-sigs/cluster-api-provider-aws/pull/2496/files?

I have time - just to confirm, is the intent to combine the changes in your PR with those here?

sedefsavas · 2021-07-01T03:17:25Z

Yes and resolving the comments.
Not much is different I think, mostly just the comment I made above.

richardcase · 2021-07-01T09:56:52Z

@cnmcavoy - sorry for not following up on this. The change i made in relation to this merged so i can take a look at the fuzzing tests for you.

randomvariable · 2021-07-23T12:54:13Z

/retest
/test pull-cluster-api-provider-aws-e2e-blocking

randomvariable · 2021-07-23T12:54:38Z

Hmm, maybe there's still a flake in the EKS reconciler test, though am unable to reproduce at 200 interations :(

randomvariable · 2021-07-23T12:54:54Z

e2e failed because of test infra issues from the looks of things.

randomvariable · 2021-07-23T12:57:24Z

Hmm, maybe there's still a flake in the EKS reconciler test, though am unable to reproduce at 200 interations :(

Found the problem. PR'ing a fix.

randomvariable · 2021-07-23T13:01:48Z

#2599 should fix the test

randomvariable · 2021-07-23T13:26:39Z

This is looking good, let's squash these commits @cnmcavoy

cnmcavoy · 2021-07-23T16:00:44Z

@randomvariable rebased & squashed.

richardcase · 2021-07-30T08:38:00Z

/lgtm

richardcase · 2021-07-30T08:40:04Z

For final approval:

/assign sedefsavas
/assign randomvariable

sedefsavas · 2021-07-30T10:55:29Z

/test pull-cluster-api-provider-aws-e2e-blocking

cnmcavoy · 2021-08-09T15:25:20Z

@richardcase @sedefsavas @randomvariable if you have time, can this be approved and merged?

randomvariable · 2021-08-09T15:37:07Z

/approve

k8s-ci-robot · 2021-08-09T15:37:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: randomvariable

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [randomvariable]

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

The removed code, introduced in kubernetes-sigs#2556, became unnecessary when equivalent code was added to the restoreInstance() function in kubernetes-sigs#2468. This code handles a subset of the cases that restoreInstances() does; namely, when both the restored and dst pointers are non-nil. The conversion tests fail if the restoreInstance() call is removed, but this code is left. They pass if this code is removed. Signed-off-by: Daniel Lipovetsky <[email protected]>

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 3, 2021

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 3, 2021

k8s-ci-robot requested review from randomvariable and richardcase June 3, 2021 20:52

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 3, 2021

cnmcavoy force-pushed the cnmcavoy/volume-throughput branch from b81c154 to 4ef24f8 Compare June 3, 2021 21:15

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 3, 2021

cnmcavoy force-pushed the cnmcavoy/volume-throughput branch from 4ef24f8 to 0f357f2 Compare June 3, 2021 21:31

richardcase requested changes Jun 3, 2021

View reviewed changes

api/v1alpha3/types.go Outdated Show resolved Hide resolved

pkg/cloud/services/ec2/instances.go Outdated Show resolved Hide resolved

cnmcavoy force-pushed the cnmcavoy/volume-throughput branch 2 times, most recently from 84414bb to 7fc29c8 Compare June 3, 2021 22:21

sedefsavas reviewed Jun 11, 2021

View reviewed changes

cnmcavoy force-pushed the cnmcavoy/volume-throughput branch from 7a26768 to 094df80 Compare June 17, 2021 17:47

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 23, 2021

cnmcavoy force-pushed the cnmcavoy/volume-throughput branch from 094df80 to e78e5df Compare June 23, 2021 15:22

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 23, 2021

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 22, 2021

cnmcavoy force-pushed the cnmcavoy/volume-throughput branch 2 times, most recently from 5f478aa to 920ca3c Compare July 22, 2021 17:02

Add ebs throughput field to v1alpha4 Volume

eb9f275

cnmcavoy force-pushed the cnmcavoy/volume-throughput branch from 920ca3c to eb9f275 Compare July 23, 2021 15:59

k8s-ci-robot assigned richardcase Jul 30, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 30, 2021

k8s-ci-robot assigned randomvariable and sedefsavas Jul 30, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 9, 2021

k8s-ci-robot merged commit 528de47 into kubernetes-sigs:main Aug 9, 2021

k8s-ci-robot added this to the v0.7.0 milestone Aug 9, 2021

dlipovetsky mentioned this pull request Sep 24, 2021

chore: Remove unnecessary conversion code #2789

Merged

4 tasks

cnmcavoy mentioned this pull request Sep 12, 2022

Update AWSMachine webhook validate logic on update to be consistent #3728

Merged

4 tasks

cnmcavoy deleted the cnmcavoy/volume-throughput branch September 26, 2022 17:30

cnmcavoy mentioned this pull request Sep 21, 2023

REQUEST: New membership for @cnmcavoy kubernetes/org#4474

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Add ebs volume throughput field to v1alpha4 Volume #2468

✨ Add ebs volume throughput field to v1alpha4 Volume #2468

cnmcavoy commented Jun 3, 2021 •

edited

Loading

k8s-ci-robot commented Jun 3, 2021

k8s-ci-robot commented Jun 3, 2021

richardcase commented Jun 3, 2021

richardcase left a comment

sedefsavas Jun 11, 2021

cnmcavoy Jun 17, 2021

sedefsavas commented Jun 29, 2021

cnmcavoy commented Jun 29, 2021

sedefsavas commented Jul 1, 2021

richardcase commented Jul 1, 2021

randomvariable commented Jul 23, 2021

randomvariable commented Jul 23, 2021

randomvariable commented Jul 23, 2021

randomvariable commented Jul 23, 2021

randomvariable commented Jul 23, 2021

randomvariable commented Jul 23, 2021

cnmcavoy commented Jul 23, 2021

richardcase commented Jul 30, 2021

richardcase commented Jul 30, 2021

sedefsavas commented Jul 30, 2021

cnmcavoy commented Aug 9, 2021

randomvariable commented Aug 9, 2021

k8s-ci-robot commented Aug 9, 2021

✨ Add ebs volume throughput field to v1alpha4 Volume #2468

✨ Add ebs volume throughput field to v1alpha4 Volume #2468

Conversation

cnmcavoy commented Jun 3, 2021 • edited Loading

k8s-ci-robot commented Jun 3, 2021

k8s-ci-robot commented Jun 3, 2021

richardcase commented Jun 3, 2021

richardcase left a comment

Choose a reason for hiding this comment

sedefsavas Jun 11, 2021

Choose a reason for hiding this comment

cnmcavoy Jun 17, 2021

Choose a reason for hiding this comment

sedefsavas commented Jun 29, 2021

cnmcavoy commented Jun 29, 2021

sedefsavas commented Jul 1, 2021

richardcase commented Jul 1, 2021

randomvariable commented Jul 23, 2021

randomvariable commented Jul 23, 2021

randomvariable commented Jul 23, 2021

randomvariable commented Jul 23, 2021

randomvariable commented Jul 23, 2021

randomvariable commented Jul 23, 2021

cnmcavoy commented Jul 23, 2021

richardcase commented Jul 30, 2021

richardcase commented Jul 30, 2021

sedefsavas commented Jul 30, 2021

cnmcavoy commented Aug 9, 2021

randomvariable commented Aug 9, 2021

k8s-ci-robot commented Aug 9, 2021

cnmcavoy commented Jun 3, 2021 •

edited

Loading