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

Support interpolated snapshot tags #1558

Merged

Conversation

hanyuel
Copy link
Contributor

@hanyuel hanyuel commented Apr 4, 2023

Is this a bug fix or adding new feature?
New feature.

What is this PR about? / Why do we need it?
For more details, see issue #1553

  1. Added support for interpolated snapshot tags that are dynamically computed at runtime using VolumeSnapshot name, namespace and VolumeSnapshotContent name.
  2. Modified the helm chart template controller.yaml to allow setting --extra-create-metadata for csi-snapshotter sidecar. It enables external-snapshotter to pass the snapshot parameters to CSI Driver.
  3. Added the documentation for Snapshot Tagging.

What testing is done?

  1. Added unit tests of string interpolation on the VolumeSnapshot template.
  2. E2E tests. Deployed the Driver with the changes. Provisioned a snapshot using this VolumeSnapshotClass
apiVersion: snapshot.storage.k8s.io/v1
kind: VolumeSnapshotClass
metadata:
  name: csi-aws-vsc
driver: ebs.csi.aws.com
deletionPolicy: Delete
parameters:
  tagSpecification_1: "key1={{ .VolumeSnapshotNamespace }}"
  tagSpecification_2: "key2={{ .VolumeSnapshotName }}"
  tagSpecification_3: "key3={{ .VolumeSnapshotContentName }}"
  tagSpecification_4: 'key4={{ .VolumeSnapshotName |  contains "ebs"}}'

and this VolumeSnapshot

apiVersion: snapshot.storage.k8s.io/v1
kind: VolumeSnapshot
metadata:
  name: ebs-volume-snapshot
spec:
  volumeSnapshotClassName: csi-aws-vsc
  source:
    persistentVolumeClaimName: ebs-claim

The tags for the snapshot in the AWS EC2 console were created as expected:

key2 | ebs-volume-snapshot
key1 | default
key3 | snapcontent-97516503-aa32-4fc5-96ae-3e694d2519d7
key4 | true

@k8s-ci-robot k8s-ci-robot requested a review from rdpsin April 4, 2023 21:20
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 4, 2023
@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 Apr 4, 2023
@k8s-ci-robot
Copy link
Contributor

Hi @hanyuel. 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 size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 4, 2023
@ConnorJC3
Copy link
Contributor

/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 Apr 4, 2023
docs/tagging.md Show resolved Hide resolved
@torredil
Copy link
Member

torredil commented Apr 5, 2023

/retest

@torredil
Copy link
Member

torredil commented Apr 6, 2023

Waiting for #1560 to land, fixes pull-aws-ebs-csi-driver-test-helm-chart.

@torredil
Copy link
Member

torredil commented Apr 6, 2023

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 6, 2023
docs/tagging.md Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 6, 2023
@torredil
Copy link
Member

torredil commented Apr 6, 2023

/hold will let the PR author unhold when everything is good to go.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 6, 2023
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 6, 2023
@rdpsin
Copy link
Contributor

rdpsin commented Apr 6, 2023

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 6, 2023
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 6, 2023
@hanyuel
Copy link
Contributor Author

hanyuel commented Apr 6, 2023

/unhold

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 6, 2023
@gtxu gtxu removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 6, 2023
@gtxu
Copy link
Contributor

gtxu commented Apr 6, 2023

Waiting for #1560 before we can merge this.

@torredil
Copy link
Member

torredil commented Apr 7, 2023

/retest

1 similar comment
@ConnorJC3
Copy link
Contributor

/retest

@ConnorJC3
Copy link
Contributor

/retest
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 7, 2023
@rdpsin
Copy link
Contributor

rdpsin commented Apr 10, 2023

/lgtm

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.

/lgtm

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gtxu, torredil

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 Apr 10, 2023
@k8s-ci-robot k8s-ci-robot merged commit 89b1594 into kubernetes-sigs:master Apr 10, 2023
4 checks passed
@hanyuel hanyuel deleted the snapshot-tag-interpolate branch June 30, 2023 13:58
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.

None yet

6 participants