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

Promote CSIMigrationvSphere to GA #113336

Merged
merged 1 commit into from Nov 4, 2022
Merged

Promote CSIMigrationvSphere to GA #113336

merged 1 commit into from Nov 4, 2022

Conversation

divyenpatel
Copy link
Member

@divyenpatel divyenpatel commented Oct 25, 2022 •
edited

What type of PR is this?

/kind feature
/sig storage

What this PR does / why we need it:

CSIMigrationvSphere has been beta since 1.19, on by default since 1.25, this takes it to GA for 1.26.
kubernetes/enhancements#1491

Special notes for your reviewer:

Testing Result
Executed storage tests and they all passed.
https://github.com/kubernetes/kubernetes/blob/master/test/e2e/storage/drivers/in_tree.go#L1409

Test Logs
vSphere in tree driver does not support read-only raw block volume so the failure of the following test is expected
[sig-storage] In-tree Volumes [Driver: vsphere] [Testpattern: Dynamic PV (block volmode)] multiVolume [Slow] should concurrently access the single read-only volume from pods on the same node
All other tests have passed.
Logs are here for both topology aware and non topology aware setups - https://gist.github.com/sashrith/6ec82e6d40c566ee0a70f7365d0986b0

cc: @sashrith @xing-yang

Does this PR introduce a user-facing change?

CSIMigrationvSphere upgraded to GA and locked to true. Do not upgrade to K8s 1.26 if you need Windows, or XFS, or raw block support until vSphere CSI Driver adds support for them in a version post v2.7.x.

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

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. kind/feature Categorizes issue or PR as related to a new feature. sig/storage Categorizes an issue or PR as relevant to SIG Storage. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Oct 25, 2022
@k8s-ci-robot
Copy link
Contributor

@divyenpatel: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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-priority Indicates a PR lacks a `priority/foo` label and requires one. label Oct 25, 2022
@xing-yang
Copy link
Contributor

/assign

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 25, 2022
@xing-yang
Copy link
Contributor

/assign @gnufied @jingxu97

@rhockenbury rhockenbury mentioned this pull request Oct 29, 2022
Closed
2 tasks
@gnufied
Copy link
Member

gnufied commented Nov 2, 2022

cc @msau42 who had some concerns about Windows support.

@gnufied
Copy link
Member

gnufied commented Nov 2, 2022

@divyenpatel @xing-yang Can you add a comment here about Windows support status? I know we have talked about it offline but it would be good to get some clarity here, before we decide to make vSphere CSI migration GA.

@xing-yang
Copy link
Contributor

@divyenpatel @xing-yang Can you add a comment here about Windows support status? I know we have talked about it offline but it would be good to get some clarity here, before we decide to make vSphere CSI migration GA.

We are working on moving CSI Windows support to GA in the next vSphere CSI Driver release.

  • We are working on defining the scale limit that can be supported on Windows.
  • Work is also in progress to fix the resize related issue.
    @divyenpatel Please chime in if I missed anything.

@xing-yang
Copy link
Contributor

Chatted with @msau42. She is fine with the plan to move CSI Windows support to GA in vSphere CSI driver next release.

@xing-yang
Copy link
Contributor

Addressed concerns from @jsafrane in this thread:
https://kubernetes.slack.com/archives/CG04EL876/p1667494293993889

@gnufied
Copy link
Member

gnufied commented Nov 3, 2022

/hold

@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 3, 2022
@gnufied
Copy link
Member

gnufied commented Nov 4, 2022

/hold cancel
/lgtm

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Nov 4, 2022
jsafrane
jsafrane reviewed Nov 4, 2022
View reviewed changes
pkg/features/kube_features.go Outdated
@@ -845,7 +845,7 @@ var defaultKubernetesFeatureGates = map[featuregate.Feature]featuregate.FeatureS

CSIMigrationRBD: {Default: false, PreRelease: featuregate.Alpha}, // Off by default (requires RBD CSI driver)

CSIMigrationvSphere: {Default: true, PreRelease: featuregate.Beta},
CSIMigrationvSphere: {Default: true, PreRelease: featuregate.GA, LockToDefault: true}, // GA in 1.26 (requires vSphere CSI Driver)
Copy link
Member

Choose a reason for hiding this comment

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

There should be only comment // remove in 1.28, so we know when to remove the flag.

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 4, 2022
@jsafrane
Copy link
Member

jsafrane commented Nov 4, 2022

/approve

@jingxu97, @msau42 last chance, I will LGTM on Monday my time. (@xing-yang, feel free to lgmt if I forget)

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: divyenpatel, jsafrane

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 Nov 4, 2022
@msau42
Copy link
Member

msau42 commented Nov 4, 2022

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 4, 2022
@k8s-ci-robot k8s-ci-robot merged commit ee640d7 into kubernetes:master Nov 4, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.26 milestone Nov 4, 2022
@gnufied
Copy link
Member

gnufied commented Nov 21, 2022

Another potential concerns that we have heard from some of the users is XFS and Raw block support. I filed - kubernetes-sigs/vsphere-csi-driver#2097 to track those issues.

Do we have a risk of breaking users if we can't sort these out in time? XFS and Raw block seems like something many customers are using right now and aren't esoteric features as such....

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsafrane jsafrane jsafrane left review comments

@thockin thockin Awaiting requested review from thockin

@wojtek-t wojtek-t Awaiting requested review from wojtek-t

Assignees

@gnufied gnufied

@xing-yang xing-yang

@jingxu97 jingxu97

@msau42 msau42

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. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/storage Categorizes an issue or PR as relevant to SIG Storage. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.26
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@divyenpatel @k8s-ci-robot @xing-yang @gnufied @jsafrane @msau42 @jingxu97