Modifying Tags On Existing Volume #2082

mdzraf · 2024-07-08T13:53:40Z

Is this a bug fix or adding new feature?

New Feature

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

This feature allows for EBS CSI Driver customers to Add, Modify, and Delete tags of existing volumes via the official CSI ControllerModifyVolume RPC through a VolumeAttributesClass.

What testing is done?

Besides running existing unit and e2e tests, this feature was tested manually by doing the following:

Make test cluster by running make cluster/create
Creating and uploading image of driver with changes to ECR Repo
Deploying driver to cluster via helm, with the necessary flags.
Creating various testing VolumeAttributesClass documents
Applying VACs and verifying that the correct tag operations were performed on the volume by describing the volume and looking at it's tags as well as ensuring that there is proper error handling

Notes:

This PR has a few extra VolumeAttributeClass documents that will be removed before merging, they are currently there because the unit tests and e2e tests for this feature are still being worked on (this is why there is some commented out code in the testing files) as well as to ensure that I can test any changes made after initial reviews.

linux-foundation-easycla · 2024-07-08T13:53:46Z

The committers listed above are authorized under a signed CLA.

✅ login: mdzraf (6572609)

k8s-ci-robot · 2024-07-08T13:53:49Z

Welcome @mdzraf!

It looks like this is your first PR to kubernetes-sigs/aws-ebs-csi-driver 🎉. 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/aws-ebs-csi-driver 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 · 2024-07-08T13:53:50Z

Hi @mdzraf. 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-sigs/prow repository.

ConnorJC3 · 2024-07-08T14:03:53Z

/ok-to-test

github-actions · 2024-07-08T15:24:38Z

Code Coverage Diff

File	Old Coverage	New Coverage	Delta
github.com/kubernetes-sigs/aws-ebs-csi-driver/pkg/cloud/cloud.go	85.4%	85.7%	0.3
github.com/kubernetes-sigs/aws-ebs-csi-driver/pkg/cloud/devicemanager/allocator.go	93.3%	100.0%	6.7
github.com/kubernetes-sigs/aws-ebs-csi-driver/pkg/driver/controller_modify_volume.go	77.2%	72.0%	-5.2

ConnorJC3

Rough initial review, did not review the tests yet and is not super thorough. Will do a more thorough and comprehensive review later.

hack/e2e/kops/patch-cluster.yaml

pkg/cloud/cloud.go

pkg/driver/controller_modify_volume.go

pkg/cloud/cloud.go

pkg/driver/controller_modify_volume.go

ConnorJC3

Mostly lgtm, a few comments and a bunch of nitpicks

pkg/driver/controller_modify_volume.go

pkg/cloud/cloud.go

pkg/driver/controller_modify_volume.go

pkg/cloud/cloud_test.go

pkg/driver/controller_modify_volume_test.go

pkg/driver/controller_modify_volume.go

docs/example-iam-policy.json

pkg/cloud/cloud.go

pkg/driver/controller_modify_volume.go

torredil · 2024-07-11T13:35:23Z

/retest

AndrewSirenko

Great work on this! Left a few non-blocking comments. Thanks!

examples/kubernetes/modify-volume/manifests/volumeattributesclass.yaml

pkg/cloud/cloud.go

pkg/driver/controller_modify_volume.go

tests/e2e/modify_volume.go

ConnorJC3

lgtm, other than

Docs updates
Cleanup extra files in examples/kubernetes/modify-volume/manifests/ and fix the main example
Squash commits
Rebase to fix govulncheck issue (I'll cut a PR to fix later today)

torredil · 2024-07-11T20:28:31Z

/retest

ConnorJC3

Review: 2 final comments, otherwise lgtm

docs/tagging.md

pkg/cloud/cloud_test.go

ConnorJC3

Changes lgtm, needs squash

torredil

/label tide/merge-method-squash
/lgtm

…butesClass

ConnorJC3

/lgtm

AndrewSirenko · 2024-07-16T14:41:34Z

/approve

k8s-ci-robot · 2024-07-16T14:41:41Z

[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:

~~OWNERS~~ [AndrewSirenko]

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 requested review from ConnorJC3 and torredil July 8, 2024 13:53

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Jul 8, 2024

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 Jul 8, 2024

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

ConnorJC3 mentioned this pull request Jul 8, 2024

Change coalescer InputType from comparable to any #2083

Merged

mdzraf force-pushed the existingVolumeTagModification branch from 93827b4 to e10693c Compare July 8, 2024 18:52

ConnorJC3 reviewed Jul 8, 2024

View reviewed changes

ConnorJC3 reviewed Jul 10, 2024

View reviewed changes

torredil reviewed Jul 11, 2024

View reviewed changes

AndrewSirenko reviewed Jul 11, 2024

View reviewed changes

ConnorJC3 reviewed Jul 11, 2024

View reviewed changes

AndrewSirenko approved these changes Jul 11, 2024

View reviewed changes

k8s-ci-robot assigned AndrewSirenko Jul 11, 2024

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

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 12, 2024

mdzraf force-pushed the existingVolumeTagModification branch from 7a5b1f2 to 957d509 Compare July 12, 2024 13:25

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 12, 2024

mdzraf force-pushed the existingVolumeTagModification branch from 957d509 to 4e7abf9 Compare July 12, 2024 13:53

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 12, 2024

AndrewSirenko approved these changes Jul 12, 2024

View reviewed changes

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

ConnorJC3 reviewed Jul 15, 2024

View reviewed changes

docs/tagging.md Outdated Show resolved Hide resolved

ConnorJC3 reviewed Jul 15, 2024

View reviewed changes

pkg/cloud/cloud_test.go Outdated Show resolved Hide resolved

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 15, 2024

ConnorJC3 reviewed Jul 15, 2024

View reviewed changes

torredil approved these changes Jul 16, 2024

View reviewed changes

k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Jul 16, 2024

k8s-ci-robot assigned torredil Jul 16, 2024

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

Adding Feature To Modify Tags on Existing Volumes Through VolumeAttri…

6572609

…butesClass

mdzraf force-pushed the existingVolumeTagModification branch from aded793 to 6572609 Compare July 16, 2024 14:34

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 16, 2024

ConnorJC3 reviewed Jul 16, 2024

View reviewed changes

k8s-ci-robot assigned ConnorJC3 Jul 16, 2024

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

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 16, 2024

k8s-ci-robot merged commit 2c4937a into kubernetes-sigs:master Jul 16, 2024
19 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modifying Tags On Existing Volume #2082

Modifying Tags On Existing Volume #2082

mdzraf commented Jul 8, 2024

linux-foundation-easycla bot commented Jul 8, 2024 •

edited

Loading

k8s-ci-robot commented Jul 8, 2024

k8s-ci-robot commented Jul 8, 2024

ConnorJC3 commented Jul 8, 2024

github-actions bot commented Jul 8, 2024 •

edited

Loading

ConnorJC3 left a comment

ConnorJC3 left a comment

torredil commented Jul 11, 2024

AndrewSirenko left a comment •

edited

Loading

ConnorJC3 left a comment •

edited

Loading

torredil commented Jul 11, 2024

ConnorJC3 left a comment

ConnorJC3 left a comment

torredil left a comment

ConnorJC3 left a comment

AndrewSirenko commented Jul 16, 2024

k8s-ci-robot commented Jul 16, 2024

Modifying Tags On Existing Volume #2082

Modifying Tags On Existing Volume #2082

Conversation

mdzraf commented Jul 8, 2024

Notes:

linux-foundation-easycla bot commented Jul 8, 2024 • edited Loading

k8s-ci-robot commented Jul 8, 2024

k8s-ci-robot commented Jul 8, 2024

ConnorJC3 commented Jul 8, 2024

github-actions bot commented Jul 8, 2024 • edited Loading

Code Coverage Diff

ConnorJC3 left a comment

Choose a reason for hiding this comment

ConnorJC3 left a comment

Choose a reason for hiding this comment

torredil commented Jul 11, 2024

AndrewSirenko left a comment • edited Loading

Choose a reason for hiding this comment

ConnorJC3 left a comment • edited Loading

Choose a reason for hiding this comment

torredil commented Jul 11, 2024

ConnorJC3 left a comment

Choose a reason for hiding this comment

ConnorJC3 left a comment

Choose a reason for hiding this comment

torredil left a comment

Choose a reason for hiding this comment

ConnorJC3 left a comment

Choose a reason for hiding this comment

AndrewSirenko commented Jul 16, 2024

k8s-ci-robot commented Jul 16, 2024

linux-foundation-easycla bot commented Jul 8, 2024 •

edited

Loading

github-actions bot commented Jul 8, 2024 •

edited

Loading

AndrewSirenko left a comment •

edited

Loading

ConnorJC3 left a comment •

edited

Loading