emit warning on deprecated annotation volume.beta.kubernetes.io/storage-class #117036

haoruan · 2023-03-31T10:23:07Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

The annotation volume.beta.kubernetes.io/storage-class is long deprecated #53580, but with no warning message.

Before we stop honoring it, a warning message should be added

Which issue(s) this PR fixes:

Fixes part of #116336

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Show a warning when `volume.beta.kubernetes.io/storage-class` annotation is used in pv or pvc

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

NONE

k8s-ci-robot · 2023-03-31T10:23:12Z

Please note that we're already in Test Freeze for the release-1.27 branch. This means every merged PR will be automatically fast-forwarded via the periodic ci-fast-forward job to the release branch of the upcoming v1.27.0 release.

Fast forwards are scheduled to happen every 6 hours, whereas the most recent run was: Fri Mar 31 04:32:14 UTC 2023.

k8s-ci-robot · 2023-03-31T10:23:16Z

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 · 2023-03-31T10:23:17Z

Hi @haoruan. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

haoruan · 2023-03-31T10:24:13Z

/sig storage

jingxu97 · 2023-05-13T21:37:28Z

/lgtm
/assign @liggitt

I think this is quite needed because we have seen users still use this long deprecated APIs.

k8s-ci-robot · 2023-05-13T21:37:35Z

LGTM label has been added.

Git tree hash: 2c00dfbb6c9931111127461290602ea31f7e01e4

pkg/api/persistentvolume/util.go

liggitt · 2023-05-15T13:40:39Z

pkg/api/persistentvolume/util.go

+func GetWarningsForStorageClassAnnotations(fieldPath *field.Path, meta *metav1.ObjectMeta) []string {
+	var warnings []string
+
+	if meta.Annotations != nil {


no need for the nil check, a nil map returns false for lookups

liggitt · 2023-05-15T13:42:57Z

pkg/api/persistentvolume/util.go

+	return warningsForPersistentVolumeSpecAndMeta(nil, &pv.Spec, &pv.ObjectMeta)
+}
+
+func GetWarningsForStorageClassAnnotations(fieldPath *field.Path, meta *metav1.ObjectMeta) []string {


exporting a method from the persistentvolume package and reusing it from the persistentvolumeclaim package is a little weird... I would probably just replicate/inline the annotation check and warning in warningsForPersistentVolumeSpecAndMeta and GetWarningsForPersistentVolumeClaim

The reason I add the exported function is because maybe we need it somewhere else, but I didn't find a suitable pkg to add it.
Yes it's a little weird, so let's remove exported method and add to both functions

liggitt · 2023-05-15T13:45:02Z

pkg/api/persistentvolume/util.go

+				fmt.Sprintf(
+					"%s: %s is %s",
+					fieldPath.Child("metadata", "annotations"),
+					api.BetaStorageClassAnnotation,
+					deprecatedStorageClassAnnotationsMsg,
+				),


Suggested change

fmt.Sprintf(

"%s: %s is %s",

fieldPath.Child("metadata", "annotations"),

api.BetaStorageClassAnnotation,

deprecatedStorageClassAnnotationsMsg,

),

fmt.Sprintf(

"%s: %s",

fieldPath.Child("metadata", "annotations").Key(api.BetaStorageClassAnnotation),

deprecatedStorageClassAnnotationsMsg,

),

…ge-class

liggitt · 2023-05-16T13:52:33Z

/lgtm
/approve

liggitt · 2023-05-16T13:52:39Z

/ok-to-test

k8s-ci-robot · 2023-05-16T13:52:41Z

LGTM label has been added.

Git tree hash: ebc9401f71088e58a8913873911bc234d5eb1e34

k8s-ci-robot · 2023-05-16T13:52:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: haoruan, liggitt

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/api/OWNERS~~ [liggitt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-triage-robot · 2023-05-16T18:00:13Z

The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass.

This bot retests PRs for certain kubernetes repos according to the following rules:

The PR does have any do-not-merge/* labels
The PR does not have the needs-ok-to-test label
The PR is mergeable (does not have a needs-rebase label)
The PR is approved (has cncf-cla: yes, lgtm, approved labels)
The PR is failing tests required for merge

You can:

Review the full test history for this PR
Prevent this bot from retesting with /lgtm cancel or /hold
Help make our tests less flaky by following our Flaky Tests Guide

/retest

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 31, 2023

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Mar 31, 2023

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Mar 31, 2023

k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Mar 31, 2023

k8s-ci-robot requested review from andrewsykim and smarterclayton March 31, 2023 10:24

k8s-ci-robot added sig/storage Categorizes an issue or PR as relevant to SIG Storage. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Mar 31, 2023

k8s-ci-robot assigned jingxu97 May 13, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 13, 2023

liggitt reviewed May 14, 2023

View reviewed changes

pkg/api/persistentvolume/util.go Show resolved Hide resolved

liggitt reviewed May 15, 2023

View reviewed changes

emit warning on deprecated annotation volume.beta.kubernetes.io/stora…

19ae103

…ge-class

haoruan force-pushed the bugfix/warning-on-deprecated-annotations branch from a95b714 to 19ae103 Compare May 16, 2023 02:58

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 16, 2023

k8s-ci-robot requested a review from jingxu97 May 16, 2023 02:58

k8s-ci-robot assigned liggitt May 16, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 16, 2023

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 16, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 16, 2023

k8s-ci-robot merged commit 2f0c2e5 into kubernetes:master May 16, 2023

k8s-ci-robot added this to the v1.28 milestone May 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

emit warning on deprecated annotation volume.beta.kubernetes.io/storage-class #117036

emit warning on deprecated annotation volume.beta.kubernetes.io/storage-class #117036

haoruan commented Mar 31, 2023

k8s-ci-robot commented Mar 31, 2023

k8s-ci-robot commented Mar 31, 2023

k8s-ci-robot commented Mar 31, 2023

haoruan commented Mar 31, 2023

jingxu97 commented May 13, 2023

k8s-ci-robot commented May 13, 2023

liggitt May 15, 2023

haoruan May 16, 2023

liggitt May 15, 2023

haoruan May 16, 2023

liggitt May 15, 2023

haoruan May 16, 2023

liggitt commented May 16, 2023

liggitt commented May 16, 2023

k8s-ci-robot commented May 16, 2023

k8s-ci-robot commented May 16, 2023

k8s-triage-robot commented May 16, 2023

emit warning on deprecated annotation volume.beta.kubernetes.io/storage-class #117036

emit warning on deprecated annotation volume.beta.kubernetes.io/storage-class #117036

Conversation

haoruan commented Mar 31, 2023

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

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

k8s-ci-robot commented Mar 31, 2023

k8s-ci-robot commented Mar 31, 2023

k8s-ci-robot commented Mar 31, 2023

haoruan commented Mar 31, 2023

jingxu97 commented May 13, 2023

k8s-ci-robot commented May 13, 2023

liggitt May 15, 2023

Choose a reason for hiding this comment

haoruan May 16, 2023

Choose a reason for hiding this comment

liggitt May 15, 2023

Choose a reason for hiding this comment

haoruan May 16, 2023

Choose a reason for hiding this comment

liggitt May 15, 2023

Choose a reason for hiding this comment

haoruan May 16, 2023

Choose a reason for hiding this comment

liggitt commented May 16, 2023

liggitt commented May 16, 2023

k8s-ci-robot commented May 16, 2023

k8s-ci-robot commented May 16, 2023

k8s-triage-robot commented May 16, 2023