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

feat: support pv/pvc metadata for subDir parameter #503

Merged
merged 2 commits into from
Jun 23, 2022

Conversation

andyzhangx
Copy link
Member

@andyzhangx andyzhangx commented Jun 17, 2022

What type of PR is this?
/kind feature

What this PR does / why we need it:
feat: support pv/pvc metadata in subDir parameter

  • subDir parameter supports following pv/pvc metadata transform

if subDir value contains following string, it would transforms into corresponding pv/pvc name or namespace

  • ${pvc.metadata.name}
  • ${pvc.metadata.namespace}
  • ${pv.metadata.name}

Which issue(s) this PR fixes:

Fixes #502

Requirements:

Special notes for your reviewer:

Release note:

feat: support pv/pvc metadata in subDir parameter

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 17, 2022
@coveralls
Copy link

coveralls commented Jun 17, 2022

Pull Request Test Coverage Report for Build 2522565494

  • 22 of 25 (88.0%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.06%) to 85.131%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/smb/nodeserver.go 7 10 70.0%
Totals Coverage Status
Change from base Build 2516152007: 0.06%
Covered Lines: 813
Relevant Lines: 955

💛 - Coveralls

@andyzhangx andyzhangx changed the title [WIP]feat: support pv/pvc metadata in subDir parameter feat: support pv/pvc metadata in subDir parameter Jun 18, 2022
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 18, 2022
@andyzhangx andyzhangx changed the title feat: support pv/pvc metadata in subDir parameter feat: support pv/pvc metadata for subDir parameter Jun 19, 2022
@jingxu97
Copy link

@andyzhangx does it require any csi provisioner side of changes to support this feature? Is this a feature that is currently supported by any driver?

@andyzhangx
Copy link
Member Author

andyzhangx commented Jun 21, 2022

@andyzhangx does it require any csi provisioner side of changes to support this feature? Is this a feature that is currently supported by any driver?

@jingxu97 csi-provisioner already supports this by setting - "--extra-create-metadata=true", this is a feature specific to CSI driver , so answer is No, it depends on specific CSI driver.

I received such requirements from #502

@ayuzzz
Copy link

ayuzzz commented Jun 22, 2022

Hi there, we are eagerly waiting for the fix to be available. Do you have a timeline for this to be released?

We tried to install the master version of csi-smb driver and got the following sub-directory name (placeholders not getting replaced by actual parameters from PVC.yml).

image

@andyzhangx
Copy link
Member Author

Hi there, we are eagerly waiting for the fix to be available. Do you have a timeline for this to be released?

We tried to install the master version of csi-smb driver and got the following sub-directory name (placeholders not getting replaced by actual parameters from PVC.yml).

image

@ayuzzz it's not merged, @jingxu97 could you approve? I think this is quite a useful feature.

@jingxu97
Copy link

is there e2e tests to validate the behavior including creation/deletion?

Could you also have an end to end example to show how this feature can be used?

@andyzhangx
Copy link
Member Author

andyzhangx commented Jun 23, 2022

is there e2e tests to validate the behavior including creation/deletion?

Could you also have an end to end example to show how this feature can be used?

@jingxu97 Yes, such metadata is already added in e2e test (test/e2e/...):
"subDir": "subDirectory-${pvc.metadata.name}",

And example are already added to driver-parameters.md file, I think that's ok since it's special usage.

@jingxu97
Copy link

/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 Jun 23, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andyzhangx, jingxu97

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 [andyzhangx,jingxu97]

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 merged commit 7647407 into kubernetes-csi:master Jun 23, 2022
@andyzhangx
Copy link
Member Author

@jingxu97 there is quite similar implementation on nfs driver, could you also approve? kubernetes-csi/csi-driver-nfs#353 thanks.

@andyzhangx
Copy link
Member Author

@ayuzzz pls try master branch with gcr.io/k8s-staging-sig-storage/smbplugin:canary image, let me know whether that works in your side, and then I would cut a new release.

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. release-note Denotes a PR that will be considered when it comes time to generate release notes. 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.

Templated Share Subdirectory
5 participants