Re: [kubernetes/kubernetes] Remove storage class beta annotations. (#51440)
Timothy St. Clair
/cc @kubernetes/sig-storage-pr-reviews
—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
Michelle Au
Have we announced deprecation on the beta annotation? The deprecation policy says support can be removed 3 months or 1 release from announcement.
Matthew Wong
No. I'm sure the intention to deprecate it in 1.7 is buried somewhere but that probably doesn't constitute an announcement, somebody with authority please make an announcement soon so we can remove this for 1.9? : ]
Peter (XiangPeng) Zhao
I send a PR #53580 to mark it as deprecated. Do we want to cherry-pick it to 1.8?
Michelle Au
I don't think we can cherry pick deprecation notices. So we can remove this in 1.10.
Kubernetes Submit Queue
@xiangpengzhao PR needs rebase
k8s-ci-robot
@xiangpengzhao: The following test failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|---|---|---|
| pull-kubernetes-e2e-gce-bazel | 34a0f2a | link | /test pull-kubernetes-e2e-gce-bazel |
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/test-infra repository. I understand the commands that are listed here.
k8s-ci-robot
@xiangpengzhao: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|
| pull-kubernetes-e2e-gce-gpu | 34a0f2a | link | /test pull-kubernetes-e2e-gce-gpu |
k8s-ci-robot
@xiangpengzhao: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|
| pull-kubernetes-e2e-gce-bazel | 34a0f2a | link | /test pull-kubernetes-e2e-gce-bazel |
| pull-kubernetes-e2e-gce-gpu | 34a0f2a | link | /test pull-kubernetes-e2e-gce-gpu |
| pull-kubernetes-bazel-test | 34a0f2a | link | /test pull-kubernetes-bazel-test |
| pull-kubernetes-bazel-build | 34a0f2a | link | /test pull-kubernetes-bazel-build |
| pull-kubernetes-node-e2e | 34a0f2a | link | /test pull-kubernetes-node-e2e |
k8s-ci-robot
@xiangpengzhao: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|---|---|---|
| pull-kubernetes-e2e-gce-bazel | 34a0f2a | link | /test pull-kubernetes-e2e-gce-bazel |
| pull-kubernetes-e2e-gce-gpu | 34a0f2a | link | /test pull-kubernetes-e2e-gce-gpu |
| pull-kubernetes-bazel-test | 34a0f2a | link | /test pull-kubernetes-bazel-test |
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.
k8s-ci-robot
@xiangpengzhao: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|
| pull-kubernetes-e2e-gce-bazel | 34a0f2a | link | /test pull-kubernetes-e2e-gce-bazel |
| pull-kubernetes-e2e-gce-gpu | 34a0f2a | link | /test pull-kubernetes-e2e-gce-gpu |
| pull-kubernetes-bazel-test | 34a0f2a | link | /test pull-kubernetes-bazel-test |
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.
k8s-ci-robot
@xiangpengzhao: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|
| pull-kubernetes-e2e-gce-bazel | 34a0f2a | link | /test pull-kubernetes-e2e-gce-bazel |
| pull-kubernetes-e2e-gce-gpu | 34a0f2a | link | /test pull-kubernetes-e2e-gce-gpu |
| pull-kubernetes-bazel-test | 34a0f2a | link | /test pull-kubernetes-bazel-test |
| pull-kubernetes-bazel-build | 34a0f2a | link | /test pull-kubernetes-bazel-build |
| pull-kubernetes-e2e-kops-aws | 34a0f2a | link | /test pull-kubernetes-e2e-kops-aws |
k8s-ci-robot
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.
Jan Šafránek
@jsafrane commented on this pull request.
In test/e2e/storage/volume_provisioning.go:
> @@ -419,9 +418,7 @@ var _ = SIGDescribe("Dynamic Provisioning", func() {
defer deleteStorageClass(c, class.Name)
claim := newClaim(*betaTest, ns, "beta")
- claim.Annotations = map[string]string{
I think you should remove whole if betaTest != nil { branch as there is no need to test the beta.
Jan Šafránek
Should we remove whole storage.k8s.io/v1beta1 API as well as part of this PR?
Peter (XiangPeng) Zhao
@xiangpengzhao pushed 1 commit.
- fc73f42 Clear other beta annotations
—
You are receiving this because you are subscribed to this thread.
View it on GitHub or mute the thread.
Peter (XiangPeng) Zhao
Should we remove whole storage.k8s.io/v1beta1 API as well as part of this PR?
@jsafrane Do we still want to announce the deprecation of beta annotations first (#53580) and then remove the beta annotations as this PR does and then graduate from v1beta1 to v1 ?
—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
Peter (XiangPeng) Zhao
@xiangpengzhao commented on this pull request.
In test/e2e/storage/volume_provisioning.go:
> @@ -419,9 +418,7 @@ var _ = SIGDescribe("Dynamic Provisioning", func() {
defer deleteStorageClass(c, class.Name)
claim := newClaim(*betaTest, ns, "beta")
- claim.Annotations = map[string]string{
I will leave the test until we decide when to graduate v1beta1 to v1 :)
Jan Šafránek
@xiangpengzhao, I think we should deprecate all in one release
Peter (XiangPeng) Zhao
Jan Šafránek
Kubernetes Submit Queue
@xiangpengzhao PR needs rebase
Peter (XiangPeng) Zhao
Ah sorry, I just checked the deprecation policy again. It says that for deprecating an API, it is 6 months/2 releases for beta, not 1 release like I originally thought.
@msau42 so we should remove the beta annotations in 1.11 since it's marked as deprecated in 1.9, right?
Also, regarding the release note, I think we should not deprecate the v1beta API group. We plan to reuse it for CSI.
Agreed. I have modified the PR title and release notes.
Michelle Au
Yup, that's right.
Peter (XiangPeng) Zhao
@msau42 thanks!
/hold
until 1.11 train starts.
Kubernetes Submit Queue
@xiangpengzhao PR needs rebase
k8s-ci-robot
[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by: gyliu513, xiangpengzhao
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: dchen1107
Assign the PR to them by writing /assign @dchen1107 in a comment when ready.
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:- pkg/apis/core/OWNERS
- pkg/controller/volume/persistentvolume/OWNERS
- pkg/printers/OWNERS
- pkg/volume/glusterfs/OWNERS
- pkg/volume/util/OWNERS
- staging/src/k8s.io/api/OWNERS
- test/OWNERS
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
@xiangpengzhao: The following test failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|
| pull-kubernetes-bazel-build | 1041fea | link | /test pull-kubernetes-bazel-build |
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/test-infra repository. I understand the commands that are listed here.
—
k8s-ci-robot
@xiangpengzhao: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|
| pull-kubernetes-e2e-gce-device-plugin-gpu | 1041fea | link | /test pull-kubernetes-e2e-gce-device-plugin-gpu |
k8s-ci-robot
@xiangpengzhao: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|
| pull-kubernetes-bazel-build | 1041fea | link | /test pull-kubernetes-bazel-build |
| pull-kubernetes-e2e-gce-device-plugin-gpu | 1041fea | link | /test pull-kubernetes-e2e-gce-device-plugin-gpu |
| pull-kubernetes-e2e-kops-aws | 1041fea | link | /test pull-kubernetes-e2e-kops-aws |
| pull-kubernetes-e2e-gce | 1041fea | link | /test pull-kubernetes-e2e-gce |
| pull-kubernetes-node-e2e | 1041fea | link | /test pull-kubernetes-node-e2e |
k8s-ci-robot
@xiangpengzhao: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|---|---|---|
| pull-kubernetes-bazel-build | 1041fea | link | /test pull-kubernetes-bazel-build |
| pull-kubernetes-e2e-gce-device-plugin-gpu | 1041fea | link | /test pull-kubernetes-e2e-gce-device-plugin-gpu |
| pull-kubernetes-e2e-kops-aws | 1041fea | link | /test pull-kubernetes-e2e-kops-aws |
| pull-kubernetes-e2e-gce | 1041fea | link | /test pull-kubernetes-e2e-gce |
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.
k8s-ci-robot
@xiangpengzhao: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|---|---|---|
| pull-kubernetes-bazel-build | 1041fea | link | /test pull-kubernetes-bazel-build |
| pull-kubernetes-e2e-gce-device-plugin-gpu | 1041fea | link | /test pull-kubernetes-e2e-gce-device-plugin-gpu |
| pull-kubernetes-e2e-kops-aws | 1041fea | link | /test pull-kubernetes-e2e-kops-aws |
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.
k8s-ci-robot
@xiangpengzhao: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|
| pull-kubernetes-bazel-build | 1041fea | link | /test pull-kubernetes-bazel-build |
| pull-kubernetes-e2e-gce-device-plugin-gpu | 1041fea | link | /test pull-kubernetes-e2e-gce-device-plugin-gpu |
| pull-kubernetes-e2e-kops-aws | 1041fea | link | /test pull-kubernetes-e2e-kops-aws |
| pull-kubernetes-integration | 1041fea | link | /test pull-kubernetes-integration |
| pull-kubernetes-typecheck | 1041fea | link | /test pull-kubernetes-typecheck |
k8s-ci-robot
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.
k8s-ci-robot
Peter (XiangPeng) Zhao
rebased & updated. @msau42 PTAL. Thanks!
/hold cancel
k8s-ci-robot
| pull-kubernetes-bazel-test | 1041fea | link | /test pull-kubernetes-bazel-test |
| pull-kubernetes-kubemark-e2e-gce | 1041fea | link | /test pull-kubernetes-kubemark-e2e-gce |
| pull-kubernetes-typecheck | 8833a3f | link | /test pull-kubernetes-typecheck |
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/test-infra repository. I understand the commands that are listed here.
—
k8s-ci-robot
k8s-ci-robot
@xiangpengzhao: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|
| pull-kubernetes-e2e-gce-device-plugin-gpu | 1041fea | link | /test pull-kubernetes-e2e-gce-device-plugin-gpu |
| pull-kubernetes-kubemark-e2e-gce | 1041fea | link | /test pull-kubernetes-kubemark-e2e-gce |
| pull-kubernetes-typecheck | 8833a3f | link | /test pull-kubernetes-typecheck |
| pull-kubernetes-bazel-test | 8833a3f | link | /test pull-kubernetes-bazel-test |
| pull-kubernetes-verify | 8833a3f | link | /test pull-kubernetes-verify |
k8s-ci-robot
@xiangpengzhao: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|
| pull-kubernetes-bazel-test | b238668 | link | /test pull-kubernetes-bazel-test |
Michelle Au
@msau42 commented on this pull request.
In pkg/apis/core/validation/validation.go:
> @@ -1844,15 +1844,7 @@ func ValidatePersistentVolumeClaimUpdate(newPvc, oldPvc *core.PersistentVolumeCl oldPvcClone.Spec.VolumeName = newPvcClone.Spec.VolumeName } - if validateStorageClassUpgrade(oldPvcClone.Annotations, newPvcClone.Annotations,
We only provided an upgrade path in 1.10 even though we officially deprecated it in 1.9. Is it acceptable to remove support for this in 1.11?
In test/e2e/autoscaling/cluster_size_autoscaling.go:
> pvcConfig := framework.PersistentVolumeClaimConfig{
- Annotations: map[string]string{
FYI we will need to backport all the e2e test fixes to 1.10 as well. Upgrade tests will run 1.10 versions of e2e tests against a 1.11 cluster.
Peter (XiangPeng) Zhao
@xiangpengzhao commented on this pull request.
In pkg/apis/core/validation/validation.go:
> @@ -1844,15 +1844,7 @@ func ValidatePersistentVolumeClaimUpdate(newPvc, oldPvc *core.PersistentVolumeCl oldPvcClone.Spec.VolumeName = newPvcClone.Spec.VolumeName } - if validateStorageClassUpgrade(oldPvcClone.Annotations, newPvcClone.Annotations,
It'd be good to keep the annotations for another release as per this concern and #51440 (comment).
/hold
Michelle Au
@msau42 commented on this pull request.
In pkg/apis/core/validation/validation.go:
> @@ -1844,15 +1844,7 @@ func ValidatePersistentVolumeClaimUpdate(newPvc, oldPvc *core.PersistentVolumeCl oldPvcClone.Spec.VolumeName = newPvcClone.Spec.VolumeName } - if validateStorageClassUpgrade(oldPvcClone.Annotations, newPvcClone.Annotations,
Can we make the e2e test update now, so that next release we don't need to backport?
Peter (XiangPeng) Zhao
@xiangpengzhao commented on this pull request.
In pkg/apis/core/validation/validation.go:
> @@ -1844,15 +1844,7 @@ func ValidatePersistentVolumeClaimUpdate(newPvc, oldPvc *core.PersistentVolumeCl oldPvcClone.Spec.VolumeName = newPvcClone.Spec.VolumeName } - if validateStorageClassUpgrade(oldPvcClone.Annotations, newPvcClone.Annotations,
I guess I misread your previous comment. Did you mean that we can remove beta annotations but keep the validation (validateStorageClassUpgrade) and then update the e2e test as is done in this PR and backport it to 1.10?
Michelle Au
@msau42 commented on this pull request.
In pkg/apis/core/validation/validation.go:
> @@ -1844,15 +1844,7 @@ func ValidatePersistentVolumeClaimUpdate(newPvc, oldPvc *core.PersistentVolumeCl oldPvcClone.Spec.VolumeName = newPvcClone.Spec.VolumeName } - if validateStorageClassUpgrade(oldPvcClone.Annotations, newPvcClone.Annotations,
I mean, we can change the e2e tests to not use the beta annotations in 1.11.
Then in 1.12, when beta annotations are removed, we don't have to modify 1.11 e2e tests in a patch release.
Peter (XiangPeng) Zhao
@xiangpengzhao commented on this pull request.
In pkg/apis/core/validation/validation.go:
> @@ -1844,15 +1844,7 @@ func ValidatePersistentVolumeClaimUpdate(newPvc, oldPvc *core.PersistentVolumeCl oldPvcClone.Spec.VolumeName = newPvcClone.Spec.VolumeName } - if validateStorageClassUpgrade(oldPvcClone.Annotations, newPvcClone.Annotations,
Got it. I send a separate PR #62317 and a cherry-picking #62319. Please see if they're what you're expecting :) Thanks!
Michelle Au
According to the updated deprecation policy, beta annotations are considered part of the API, so we cannot deprecate/remove this until we have core/v2
Peter (XiangPeng) Zhao
Closed #51440.