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

azure file migration go beta #96293

Merged
merged 1 commit into from
Feb 5, 2021
Merged

azure file migration go beta #96293

merged 1 commit into from
Feb 5, 2021

Conversation

andyzhangx
Copy link
Member

@andyzhangx andyzhangx commented Nov 6, 2020 •
edited
Loading

What type of PR is this?
/kind documentation

What this PR does / why we need it:
azure file migration go beta in 1.21

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

feat: azure file migration go beta in 1.21. Feature gates CSIMigration to Beta (on by default) and CSIMigrationAzureFile to Beta (off by default since it requires installation of the AzureFile CSI Driver)
The in-tree AzureFile plugin "kubernetes.io/azure-file" is now deprecated and will be removed in 1.23. Users should enable CSIMigration + CSIMigrationAzureFile features and install the AzureFile CSI Driver (https://github.com/kubernetes-sigs/azurefile-csi-driver) to avoid disruption to existing Pod and PVC objects at that time.
Users should start using the AzureFile CSI Driver directly for any new volumes.

Azure File CSI driver does not support using same persistent volume with different fsgroups, when CSI migration is enabled for azurefile driver, such case is not supported. (there is a case we support where volume is mounted with 0777 and then it readable/writable by everyone)

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

feat: azure file migration go beta in 1.21. Feature gates CSIMigration to Beta (on by default) and CSIMigrationAzureFile to Beta (off by default since it requires installation of the AzureFile CSI Driver)
The in-tree AzureFile plugin "kubernetes.io/azure-file" is now deprecated and will be removed in 1.23. Users should enable CSIMigration + CSIMigrationAzureFile features and install the AzureFile CSI Driver (https://github.com/kubernetes-sigs/azurefile-csi-driver) to avoid disruption to existing Pod and PVC objects at that time.
Users should start using the AzureFile CSI Driver directly for any new volumes.

Azure File CSI driver does not support using same persistent volume with different fsgroups, when CSI migration is enabled for azurefile driver, such case is not supported. (there is a case we support where volume is mounted with 0777 and then it readable/writable by everyone)

/priority important-soon
/sig cloud-provider
/area provider/azure
/triage accepted
/assign @msau42

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/documentation Categorizes issue or PR as related to documentation. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. area/provider/azure Issues or PRs related to azure provider cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. triage/accepted Indicates an issue or PR is ready to be actively worked on. labels Nov 6, 2020
@andyzhangx andyzhangx mentioned this pull request Nov 6, 2020
Closed
@msau42
Copy link
Member

msau42 commented Nov 6, 2020

/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 Nov 6, 2020
@andyzhangx andyzhangx mentioned this pull request Nov 7, 2020
Closed
@irvifa
Copy link
Member

irvifa commented Nov 8, 2020

/assign @pwittrock

@pwittrock
Copy link
Member

I don't seem like the right person to review this.

@kikisdeliveryservice
Copy link
Member

This is for kubernetes/enhancements#1885 correct?

@msau42
Copy link
Member

msau42 commented Nov 9, 2020

Correct, I can be the tech reviewer for this

@feiskyer
Copy link
Member

/lgtm
/milestone v1.20

@k8s-ci-robot k8s-ci-robot added this to the v1.20 milestone Nov 11, 2020
@andyzhangx
Copy link
Member Author

/hold
since there is blocking issue here: #96508, let's hold for a few days, to check whether it could be fixed soon or postponed to 1.21

@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 Nov 12, 2020
@kikisdeliveryservice
Copy link
Member

/hold
since there is blocking issue here: #96508, let's hold for a few days, to check whether it could be fixed soon or postponed to 1.21

Just so you're aware: code freeze is EOD PST today. Please keep us updated about your plans for 1.20.

Thanks!
Kirsten

@kikisdeliveryservice
Copy link
Member

Hi @andyzhangx

Code Freeze is now in effect. Your PR is still unmerged, and has a hold. If you believe this should be in the 1.20 Release, please file an Exception.

Best,
Kirsten

/milestone clear

@k8s-ci-robot k8s-ci-robot removed this from the v1.20 milestone Nov 13, 2020
@ialidzhikov ialidzhikov mentioned this pull request Dec 2, 2020
Closed
@pwittrock pwittrock removed their assignment Dec 9, 2020
@andyzhangx andyzhangx changed the title azure file migration go beta in 1.20 azure file migration go beta Feb 4, 2021
@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Feb 4, 2021
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 4, 2021
@andyzhangx
Copy link
Member Author

/hold cancel

@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 Feb 4, 2021
@andyzhangx
Copy link
Member Author

/test pull-kubernetes-e2e-azure-file-windows-containerd

@andyzhangx
Copy link
Member Author

@msau42 it's ready to go beta since block issue was fixed by #97877

@andyzhangx
Copy link
Member Author

/retest

feiskyer
feiskyer approved these changes Feb 5, 2021
View reviewed changes
Copy link
Member

@feiskyer feiskyer 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 Feb 5, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andyzhangx, feiskyer, msau42

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 merged commit d3fce91 into kubernetes:master Feb 5, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.21 milestone Feb 5, 2021
@gnufied
Copy link
Member

gnufied commented Mar 1, 2021 •
edited
Loading

We should cover restrictions and known issues in azurefile CSI driver if they enable the migration. @andyzhangx @msau42 - should we open a new PR to address that?

Namely we need to capture:

  1. If CSI migration is enabled for azurefile driver then users can't use fsgroup for pods currently.

The other thing we need to do is deprecate the support for using same volume multiple times on a node with different fsgroup. That is unsupported as well (well there is a case we support where volume could be mounted with 777 and then it readable/writable by everyone).

@andyzhangx
Copy link
Member Author

We should cover restrictions and known issues in azurefile CSI driver if they enable the migration. @andyzhangx @msau42 - should we open a new PR to address that?

Namely we need to capture:

  1. If CSI migration is enabled for azurefile driver then users can't use fsgroup for pods currently.

The other thing we need to do is deprecate the support for using same volume multiple times on a node with different fsgroup. That is unsupported as well (well there is a case we support where volume could be mounted with 777 and then it readable/writable by everyone).

@gnufied thanks, I have appended following note in the PR:

Azure File CSI driver does not support using same persistent volume with different fsgroups, when CSI migration is enabled for azurefile driver, such case is not supported. (there is a case we support where volume is mounted with 0777 and then it readable/writable by everyone)

@gnufied
Copy link
Member

gnufied commented Mar 2, 2021

I think there are two separate issues:

  1. If Azurefile CSI migration is enabled, using fsgroup of pods won't be supported at all. This is what we are trying to fix here - Add spec change for volume mount group container-storage-interface/spec#468 but in current beta status we should note that mounting volumes via pod's fsgroup is not supported.
  2. The second issue is more like longer term thing. Assuming a version of Add spec change for volume mount group container-storage-interface/spec#468 gets merged and we start supporting fsgroup for mounting azurefile volumes, we still can't support using same volume with different fsgroup on same node because driver has node-stage. So this behaviour needs to be deprecated.

@Jiawei0227 Jiawei0227 mentioned this pull request Mar 3, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@feiskyer feiskyer feiskyer approved these changes

@brendandburns brendandburns Awaiting requested review from brendandburns

@dims dims Awaiting requested review from dims

Assignees

@feiskyer feiskyer

@msau42 msau42

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/provider/azure Issues or PRs related to azure provider cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/documentation Categorizes issue or PR as related to documentation. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Milestone
v1.21
Development

Successfully merging this pull request may close these issues.
8 participants
@andyzhangx @msau42 @irvifa @pwittrock @kikisdeliveryservice @feiskyer @k8s-ci-robot @gnufied