-
Notifications
You must be signed in to change notification settings - Fork 38.7k
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
Promote some InTreePluginXXXUnregister to GA #124023
base: master
Are you sure you want to change the base?
Conversation
@carlory: GitHub didn't allow me to request PR reviews from the following users: davidz627. Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
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. |
This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The 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. |
8337277
to
abc3f23
Compare
abc3f23
to
11a3dcb
Compare
742892e
to
65a906c
Compare
/test pull-kubernetes-unit |
/cc @jsafrane |
@@ -1076,21 +1076,21 @@ var defaultKubernetesFeatureGates = map[featuregate.Feature]featuregate.FeatureS | |||
|
|||
ImageMaximumGCAge: {Default: true, PreRelease: featuregate.Beta}, | |||
|
|||
InTreePluginAWSUnregister: {Default: false, PreRelease: featuregate.Alpha}, | |||
InTreePluginAWSUnregister: {Default: true, PreRelease: featuregate.GA, LockToDefault: true}, // remove in 1.32 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but the in-tree code was removed, so the beta stage may be meaningless.
/hold Putting this on hold until we understand the implications of moving this to GA. If this means the user cannot use in-tree PVs/PVCs/StorageClasses any more, then we can never enable this by default. cc @jsafrane |
Thanks for the enhancement link! I checked the PR and tested with in-tree AWS on Kubernetes 1.29, CSI migration works fine. We should have declare these feature gates GA when CSI migration went GA and remove them when CSI migration gates were removed. What if we just delete the feature gates? They don't affect anything, you can turn them on and off, but they don't have any impact on any code in Kubernetes. For example, @carlory can you please split it into two PRs, one that updates all the unit tests (most of this PR) and a second one that deletes the feature gates and all their usage? That one should be small and we can argue if it's safe or not to remove them. |
|
||
InTreePluginPortworxUnregister: {Default: false, PreRelease: featuregate.Alpha}, | ||
|
||
InTreePluginRBDUnregister: {Default: false, PreRelease: featuregate.Deprecated}, // deprecated in 1.28, remove in 1.31 | ||
|
||
InTreePluginvSphereUnregister: {Default: false, PreRelease: featuregate.Alpha}, | ||
InTreePluginvSphereUnregister: {Default: true, PreRelease: featuregate.GA, LockToDefault: true}, // remove in 1.32 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll need to test this on vSphere before this change is merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI:
- CSIMigrationvSphere is GA in 1.27. Lock CSIMigrationvSphere feature gate for k8s 1.27 #116610.
- the CSIMigrationvSphere feature-gate was removed in 1.29. Remove GA featuregate about CSIMigrationvSphere in 1.29 #121291
- in-tree code was removed in 1.30. remove the deprecated in-tree vsphere volume's code #122090.
So 1.29 is the last release that can handle version skew between managers and nodes. It's not related to this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the https://kubernetes.io/releases/version-skew-policy/, kube-apiserver is at 1.29 and kubelet is supported at 1.29, 1.28, 1.27, and 1.26.
So if kube-apiserver is at 1.30 and kubelet is supported at 1.30, 1.29, 1.28, and 1.27.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the CSIMigrationvSphere feature-gate was GA at 1.27, so csinode's annotation storage.alpha.kubernetes.io/migrated-plugins
always contains the kubernetes.io/vsphere-volume
item.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In other words, operations for the in-tree vsphereVolume type are redirected to the csi.vsphere.vmware.com CSI driver.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @divyenpatel
I will do it |
483ca9c
to
03881eb
Compare
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: carlory The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/test pull-kubernetes-e2e-gce-cos-alpha-features |
You can change https://github.com/kubernetes/test-infra/blob/ae8f22151440e45c688ca6fdea54fbea50ab9e14/config/jobs/kubernetes/sig-cloud-provider/gcp/gcp-gce.yaml#L458 to remove the FG setting there. |
Changelog suggestion Promoted the following feature gates to general availability:
- `InTreePluginAWSUnregister`
- `InTreePluginAzureDiskUnregister`
- `InTreePluginAzureFileUnregister`
- `InTreePluginGCEUnregister`
- `InTreePluginOpenStackUnregister`
- `InTreePluginvSphereUnregister` |
/test all |
replaced by #124815 |
/reopen |
@carlory: Reopened this PR. In response to this:
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-sigs/prow repository. |
03881eb
to
eb52ea8
Compare
@carlory: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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-sigs/prow repository. I understand the commands that are listed here. |
What type of PR is this?
/kind cleanup
/sig storage
What this PR does / why we need it:
The following plugins in the
Special notes for your reviewer
section already completed the migration task and its in-tree code was removed as part of the migration process. So, the corresponding feature gate should be removed, but it has not been removed yet.Whatever the value of the feature gate is, it won't affect the code's behavior. Even if the feature gate is false, we will still get the right value from csinode's annotations. The PR updates the value to true and promotes them to GA. And those feature gates will be removed in v1.32.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
KEPs:
/cc @leakingtapan @andyzhangx @davidz627 @adisky @dims @divyenpatel @xing-yang
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: