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.

Sign up for GitHub

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

Jump to bottom

Implement KEP3751 ("ControllerModifyVolume") #1941

Merged
merged 6 commits into from
Feb 28, 2024
Merged

Implement KEP3751 ("ControllerModifyVolume") #1941

merged 6 commits into from
Feb 28, 2024

Conversation

ConnorJC3
Copy link
Contributor

Is this a bug fix or adding new feature?

/kind feature

What is this PR about? / Why do we need it?

Implements support for modifying volumes via KEP3751 (https://kep.k8s.io/3751) using the EBS CSI Driver.
Change is split into multiple small commits:

  • Refactor modification primitives out of ModifyVolumeProperties to separate functions
  • Implement ControllerModifyVolume and MutableParameters in CreateVolume
  • Update unit tests for ControllerModifyVolume/MutableParameters
  • Add VolumeAttributesClass RBAC permissions to external-resizier and external-provisioner
  • Add ControllerModifyVolume e2e tests

What testing is done?

Manually tested, unit tests and e2e tests added

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 21, 2024
@k8s-ci-robot k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Feb 21, 2024
@github-actions GitHub Actions
Copy link

Code Coverage Diff

File Old Coverage New Coverage Delta
github.com/kubernetes-sigs/aws-ebs-csi-driver/pkg/driver/controller_modify_volume.go 77.8% 86.2% 8.4

@ConnorJC3
Copy link
Contributor Author

/retest

Prow was down, back up now

torredil
torredil reviewed Feb 22, 2024
View reviewed changes
Copy link
Member

@torredil torredil left a comment

Choose a reason for hiding this comment

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

Thanks for the comprehensive code coverage and adding e2e tests. Clean implementation, did not run into any issues during manual testing!

Few suggestions but this PR is looking good.

pkg/driver/controller.go Show resolved Hide resolved
pkg/driver/controller.go Show resolved Hide resolved
pkg/driver/controller_test.go Show resolved Hide resolved
pkg/driver/controller.go Show resolved Hide resolved
torredil
torredil reviewed Feb 22, 2024
View reviewed changes
Copy link
Member

@torredil torredil left a comment

Choose a reason for hiding this comment

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

One last request: can we add an example for users in this section: https://github.com/kubernetes-sigs/aws-ebs-csi-driver/tree/master/examples/kubernetes

AndrewSirenko
AndrewSirenko reviewed Feb 22, 2024
View reviewed changes
Copy link
Contributor

@AndrewSirenko AndrewSirenko left a comment

Choose a reason for hiding this comment

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

Manual testing, then will approve.

tests/e2e/modify_volume.go Show resolved Hide resolved
tests/e2e/testsuites/modify_volume_tester.go Outdated Show resolved Hide resolved
pkg/driver/controller_modify_volume.go Show resolved Hide resolved
pkg/driver/request_coalescing_test.go Outdated Show resolved Hide resolved
@torredil torredil mentioned this pull request Feb 22, 2024
Open
20 tasks
@sunnylovestiramisu
Copy link

/cc @sunnylovestiramisu

@sunnylovestiramisu
Copy link

Does the actual e2e tests pass for AWS's use case?

@ConnorJC3
Copy link
Contributor Author

@torredil @AndrewSirenko comments addressed

@sunnylovestiramisu what do you mean by the "actual" e2e tests? If you mean the AWS private e2e tests added in this PR, those pass. If you mean upstream k/k e2e tests, as far as I know none of those exist yet (https://github.com/search?q=repo%3Akubernetes%2Fkubernetes+path%3Atest%2Fe2e%2F*+VolumeAttributesClass&type=code) - as a matter of fact I was planning to contirbute to those after this gets merged.

AndrewSirenko
AndrewSirenko approved these changes Feb 27, 2024
View reviewed changes
Copy link
Contributor

@AndrewSirenko AndrewSirenko left a comment

Choose a reason for hiding this comment

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

This looks great. Especially all the installation documentation raises the bar. Thank you. One small typo otherwise lgtm.

docs/modify-volume.md

- `ebs.csi.aws.com/volumeType`: to update the volume type
- `ebs.csi.aws.com/iops`: to update the IOPS
- `ebs.csi.aws.com/throughput`: to update the throughput
Copy link
Contributor

Choose a reason for hiding this comment

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

throughput

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 27, 2024
@torredil
Copy link
Member

/lgtm

@sunnylovestiramisu
Copy link

@ConnorJC3 I meant the AWS private e2e tests with a ControllerModifyVolume call.

@AndrewSirenko
Copy link
Contributor

@ConnorJC3 I meant the AWS private e2e tests with a ControllerModifyVolume call.

@sunnylovestiramisu yes the private AWS e2e tests pass with a ControllerModifyVolume call. You can see the logs if you search for via external-resizer in pull-aws-ebs-csi-driver-e2e-single-az

For example see this ginkgo spec and this controller_modify_volume.go log message

@sunnylovestiramisu
Copy link

/lgtm

Just a reminder that some error cases were not tested out for this feature other than some unit tests coverage. If you see any bugs please report to the external-resizer or external-provisioner repo.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 28, 2024
@AndrewSirenko
Copy link
Contributor

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 28, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AndrewSirenko

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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 28, 2024
@ConnorJC3
Copy link
Contributor Author

/retest

@k8s-ci-robot k8s-ci-robot merged commit a1aeff3 into kubernetes-sigs:master Feb 28, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@torredil torredil torredil left review comments

@AndrewSirenko AndrewSirenko AndrewSirenko approved these changes

@sunnylovestiramisu sunnylovestiramisu Awaiting requested review from sunnylovestiramisu

Assignees

@AndrewSirenko AndrewSirenko

@sunnylovestiramisu sunnylovestiramisu

@torredil torredil

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ConnorJC3 @sunnylovestiramisu @torredil @AndrewSirenko @k8s-ci-robot