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

Apply extra volume tags to EBS snapshots #568

Merged
merged 1 commit into from
Oct 9, 2020

Conversation

chrishenzie
Copy link
Contributor

@chrishenzie chrishenzie commented Sep 24, 2020

Is this a bug fix or adding new feature?

Adding a new feature, see #566

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

This PR takes the --extra-volume-tags that are applied to provisioned EBS volumes in CreateVolume(), and also adds them provisioned snapshots via CreateSnapshot().

What testing is done?

  • Included a unit test that asserts the snapshotOptions received by cloud.CreateSnapshot() includes the supplied
    extraVolumeTags
  • Included a unit test that asserts the snapshotOptions received by cloud.CreateSnapshot() includes the cluster ID if that driver option is enabled (See PR feedback)
  • Included a unit test that asserts the request object sent to the AWS API is correct
    • This test caught a bug where we were passing a pointer to a loop variable which resulted in duplicate tags being passed to the API
  • Performed manual end to end testing in an AWS cluster, created a VolumeSnapshot resource and saw the provisioned snapshot have the additional tags applied to it

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 24, 2020
@k8s-ci-robot
Copy link
Contributor

Welcome @chrishenzie!

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
Copy link
Contributor

Hi @chrishenzie. 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.

@k8s-ci-robot 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 Sep 24, 2020
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 24, 2020
@coveralls
Copy link

coveralls commented Sep 24, 2020

Pull Request Test Coverage Report for Build 1261

  • 36 of 37 (97.3%) changed or added relevant lines in 4 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.1%) to 81.253%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/driver/validation.go 11 12 91.67%
Files with Coverage Reduction New Missed Lines %
pkg/driver/validation.go 1 94.59%
Totals Coverage Status
Change from base Build 1260: 0.1%
Covered Lines: 1595
Relevant Lines: 1963

💛 - Coveralls

@bertinatto
Copy link
Member

/ok-to-test

@k8s-ci-robot 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 Sep 25, 2020
@chrishenzie chrishenzie force-pushed the tag-snapshots branch 2 times, most recently from e8374c1 to e6ad3b1 Compare September 28, 2020 18:06
Copy link
Contributor

@msau42 msau42 left a comment

Choose a reason for hiding this comment

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

generally lgtm
/assign @bertinatto @wongma7
especially regarding reusing the "extra-volume-tags" arg.

@@ -440,6 +440,9 @@ func (d *controllerService) CreateSnapshot(ctx context.Context, req *csi.CreateS
opts := &cloud.SnapshotOptions{
Tags: map[string]string{cloud.SnapshotNameTagKey: snapshotName},
}
for k, v := range d.driverOptions.extraVolumeTags {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you also want to add cluster id tag, similar to how it's done for CreateVolume?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added this and an additional unit test to cover this case.

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 29, 2020
Copy link
Member

@bertinatto bertinatto left a comment

Choose a reason for hiding this comment

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

Overal LGTM, just 2 nits.

I'd like to propose we mark the extra-volume-tags option as Deprecated and make it an alias to a new option with a more representative name. Later on we can remove extra-volume-tags and keep only the new one.

// Enforce ordering for unit tests.
sort.SliceStable(tags, func(i, j int) bool {
return *tags[i].Key < *tags[j].Key
})
Copy link
Member

Choose a reason for hiding this comment

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

Is this sorting really necessary? If so, can we potentially move it to the test function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed in favor of a custom matcher for this AWS input type. Let me know if this is what you were thinking.

pkg/driver/controller.go Show resolved Hide resolved
@chrishenzie
Copy link
Contributor Author

@bertinatto One concern I have with this general --extra-tags approach is that it's a little less flexible for customers.

Suppose customers want different tags for snapshots than they do volumes. With this solution, they will need to combine their tags for snapshots and volumes into one string, which could potentially exceed the limit of the max # of tags.

Since this driver only provisions either EBS volumes or snapshots, it seems like another alternative would be to introduce --extra-snapshot-tags and use that for just snapshots. If users want the same tags applied to both, they can provide the same value for both flags.

pkg/driver/controller.go Show resolved Hide resolved
// ID of the kubernetes cluster. This is used only to create the same tags on volumes that
// in-tree volume volume plugin does.
KubernetesClusterID string
}

func (s *ControllerOptions) AddFlags(fs *flag.FlagSet) {
fs.Var(cliflag.NewMapStringString(&s.ExtraVolumeTags), "extra-volume-tags", "Extra volume tags to attach to each dynamically provisioned volume. It is a comma separated list of key value pairs like '<key1>=<value1>,<key2>=<value2>'")
fs.Var(cliflag.NewMapStringString(&s.ExtraTags), "extra-tags", "Extra tags to attach to each dynamically provisioned resource. It is a comma separated list of key value pairs like '<key1>=<value1>,<key2>=<value2>'")
fs.Var(cliflag.NewMapStringString(&s.ExtraTags), "extra-volume-tags", "DEPRECATED: Please use --extra-tags instead. Extra volume tags to attach to each dynamically provisioned volume. It is a comma separated list of key value pairs like '<key1>=<value1>,<key2>=<value2>'")
Copy link
Member

Choose a reason for hiding this comment

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

If both flags are set, is it possible that the deprecated one will overwrite the other? Is there a way we can give extra-tags priority?

Perhaps we can:

  1. Keep ExtraVolumeTags above
  2. Keep the driver.WithExtraVolumeTags line in cmd/main.go
  3. Keep the WithExtraVolumeTags in driver.go and change it to only set o.extraTags if it hasn't been set yet.

Does that makes sense?

Copy link
Member

Choose a reason for hiding this comment

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

The other changes LGTM.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that makes sense. Let me know if this captures it.

@bertinatto
Copy link
Member

@bertinatto One concern I have with this general --extra-tags approach is that it's a little less flexible for customers.

Suppose customers want different tags for snapshots than they do volumes. With this solution, they will need to combine their tags for snapshots and volumes into one string, which could potentially exceed the limit of the max # of tags.

Since this driver only provisions either EBS volumes or snapshots, it seems like another alternative would be to introduce --extra-snapshot-tags and use that for just snapshots. If users want the same tags applied to both, they can provide the same value for both flags.

Sorry, I missed this comment. That's a valid point, but I'm not sure if we need this flexibility at the moment (I don't have any use cases in mind). To me it feels like using a single flag is OK at this point, but we can always split it if we come across a strong use case.

Copy link
Member

@bertinatto bertinatto left a comment

Choose a reason for hiding this comment

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

Just 2 nits regarding the deprecated flag. Other than that, I think we can merge this!

I'm not sure how many people out there is using this flag, so IMO it's worth to be very "loud" about it 👍

cmd/options/controller_options.go Show resolved Hide resolved
pkg/driver/driver.go Show resolved Hide resolved
@chrishenzie
Copy link
Contributor Author

@bertinatto I've squashed these into one commit.

See here for the deprecation comment:

// DEPRECATED: Use ExtraTags instead.

And here for the warning:

if o.extraTags == nil && extraVolumeTags != nil {
klog.Warning("DEPRECATION WARNING: --extra-volume-tags is deprecated, please use --extra-tags instead")
o.extraTags = extraVolumeTags
}

I added an extra check for extraVolumeTags != nil so we don't trip this warning when a user doesn't supply --extra-volume-tags.

Copy link
Member

@bertinatto bertinatto left a comment

Choose a reason for hiding this comment

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

Thank you for working on this @chrishenzie!

/lgtm

func WithExtraVolumeTags(extraVolumeTags map[string]string) func(*DriverOptions) {
return func(o *DriverOptions) {
o.extraVolumeTags = extraVolumeTags
if o.extraTags == nil && extraVolumeTags != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Ideally we'd have 2 checks: 1 for printing the warning if extraVolumeTags != nil and the other to overwrite o.extraTags. But I think this is fine for now.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 5, 2020
@bertinatto
Copy link
Member

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bertinatto, chrishenzie

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 Oct 5, 2020
@bertinatto
Copy link
Member

/retest

1 similar comment
@bertinatto
Copy link
Member

/retest

@bertinatto
Copy link
Member

/test pull-aws-ebs-csi-driver-e2e-single-az

@bertinatto
Copy link
Member

/test pull-aws-ebs-csi-driver-e2e-single-az

Failures seem unrelated to this change.

@bertinatto
Copy link
Member

/test pull-aws-ebs-csi-driver-e2e-single-az

5 similar comments
@bertinatto
Copy link
Member

/test pull-aws-ebs-csi-driver-e2e-single-az

@chrishenzie
Copy link
Contributor Author

/test pull-aws-ebs-csi-driver-e2e-single-az

@bertinatto
Copy link
Member

/test pull-aws-ebs-csi-driver-e2e-single-az

@bertinatto
Copy link
Member

/test pull-aws-ebs-csi-driver-e2e-single-az

@bertinatto
Copy link
Member

/test pull-aws-ebs-csi-driver-e2e-single-az

@bertinatto
Copy link
Member

/retest
@wongma7 @leakingtapan the AZ job seems fairly broken [1], do you have info on that?

[1] https://testgrid.k8s.io/sig-aws-ebs-csi-driver#e2e-test-single-az

@bertinatto
Copy link
Member

/retest

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 8, 2020
@chrishenzie
Copy link
Contributor Author

Ran make test locally, did not encounter these vendor version issues that prow is experiencing.

@bertinatto
Copy link
Member

/retest

Copy link
Member

@bertinatto bertinatto left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 9, 2020
@k8s-ci-robot k8s-ci-robot merged commit 6d6e8b7 into kubernetes-sigs:master Oct 9, 2020
@chrishenzie chrishenzie deleted the tag-snapshots branch October 9, 2020 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants