KEP-3333: Graduate RetroactiveDefaultStorageClass to beta in v1.26 #3544

RomanBednar · 2022-09-23T11:55:51Z

One-line PR description: Graduate RetroactiveDefaultStorageClass feature to beta

Issue link: Retroactive default StorageClass assignement #3333

Other comments:

keps/sig-storage/3333-reconcile-default-storage-class/kep.yaml

keps/sig-storage/3333-reconcile-default-storage-class/README.md

keps/sig-storage/3333-reconcile-default-storage-class/kep.yaml

keps/sig-storage/3333-reconcile-default-storage-class/README.md

wojtek-t · 2022-09-27T11:57:22Z

keps/sig-storage/3333-reconcile-default-storage-class/README.md

-Longer term, we may want to require automated upgrade/rollback tests, but we
-are missing a bunch of machinery and tooling and can't do that now.
-->
+Upgrade and rollback will be tested when the feature gate will change to beta.


Please send a dedicated KEP PR once you do this, before graduating the feature to beta in k/k (and assign both to SIG leads and myself).

Ok, will do.

wojtek-t · 2022-09-27T11:59:12Z

keps/sig-storage/3333-reconcile-default-storage-class/README.md

+A metric which would include PersistentVolumeClaim name or StorageClass name 
+as a label to help users debug possible issues. A metric with such labels should
+not be added because PVCs and SCs are in control of users and if they would
+create too many of those resources it could overload the metric system.


Let's maybe rephrase that "such metric would have potentially unbounded cardinality, which is a hard blocker for adding it".

keps/sig-storage/3333-reconcile-default-storage-class/README.md

keps/sig-storage/3333-reconcile-default-storage-class/kep.yaml

jsafrane · 2022-09-27T12:54:24Z

keps/sig-storage/3333-reconcile-default-storage-class/README.md

 ones.

+Test:
+https://github.com/kubernetes/kubernetes/blob/91a9ce28ac2486c50222aeeec1f76e664155d769/test/e2e/storage/pvc_storageclass.go#L62


Please link testgrid, there were few green runs in https://testgrid.k8s.io/sig-network-gce#gci-gce-alpha-features.

RomanBednar · 2022-09-27T15:12:22Z

Removed stage: section from kep.yaml but the CI test is complaining now:

time="2022-09-27T15:12:25Z" level=info msg="the following PRR validation errors occurred in /home/prow/go/src/github.com/kubernetes/enhancements/keps/sig-storage/3333-reconcile-default-storage-class/kep.yaml:"
time="2022-09-27T15:12:25Z" level=info msg="getting PRR approver for  stage: invalid stage: , should be one of [alpha beta stable]"

@wojtek-t maybe the field removal is not yet reflected in CI?

wojtek-t · 2022-09-28T07:00:36Z

keps/sig-storage/3333-reconcile-default-storage-class/kep.yaml

-stage: alpha
+prr-approvers:
+  - "@deads2k"
+  - "@wojtek-t"


Please remove this section.

And readd back stage: beta - lack of it is failing validation.

wojtek-t · 2022-09-29T07:36:41Z

keps/sig-storage/3333-reconcile-default-storage-class/README.md

@@ -352,68 +365,53 @@ enabled for the first time.

 ###### Are there any tests for feature enablement/disablement?

-Unit tests will cover feature enablement/disablement.
+Unit tests cover feature enablement/disablement.


Those aren't really testing feature enablement/disablement.
Those are testing feature itself (when the feature gate is on).

Feature enablement/disablement is the operation of enabling/disabling the feature (so changing the value of feature gate).

As mentioned in the comment in the KEP template, here is an example of such test:
https://github.com/kubernetes/kubernetes/pull/97058/files#diff-7826f7adbc1996a05ab52e3f5f02429e94b68ce6bce0dc534d1be636154fded3R246-R282

I see, something like this would be sufficient? kubernetes/kubernetes#112786

I am not sure such a test is applicable here - there is no new field that needs to be cleared / handled specially when the feature is disabled/enabled.
The feature gate just enables / disables a code path in PV controller and IMO that's covered by existing unit test,

Would it be enough just to mention that in this section?

Thinking more about it - I agree with @jsafrane - there is no new field here and there is no "migration phase" per-se.

So yes - I don't think we need additional code changes - just please explain it better in here in the text (roughly what @jsafrane wrote above).

Ok, I've added an explanation and closed the PR linked above.

wojtek-t · 2022-09-29T07:41:01Z

keps/sig-storage/3333-reconcile-default-storage-class/README.md

- [ ] Other (treat as last resort)
-    - Details:
+    - Other field: `pvc.spec.storageClassName` changing from nil to current default StorageClass name after the default is set
+- [X] Other (treat as last resort)


That's not really true - the metric is an aggregate, I can check if the feature is used, but not if my particular instance of PVC is using it.
Please unmark this one.

sftim · 2022-09-29T15:44:29Z

keps/sig-storage/3333-reconcile-default-storage-class/README.md

@@ -352,68 +365,48 @@ enabled for the first time.

 ###### Are there any tests for feature enablement/disablement?

-Unit tests will cover feature enablement/disablement.
+Unit test covers feature enablement/disablement:
+<TODO: add a link once merged>


When do we add this?

As discussed above - we won't be adding any new tests for this section.

wojtek-t · 2022-10-03T06:13:45Z

/label tide/merge-method-squash

/approve PRR

Requires SIG-level approval still.

/assign @jsafrane
@jsafrane - can you please take this?

jsafrane · 2022-10-03T12:17:05Z

/lgtm
/approve

k8s-ci-robot · 2022-10-03T12:17:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jsafrane, RomanBednar, wojtek-t

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:

~~keps/prod-readiness/OWNERS~~ [wojtek-t]
~~keps/sig-storage/OWNERS~~ [jsafrane]

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

…ubernetes#3544) * update RetroactiveDefaultStorageClass for beta graduation * address review comments 1 * address review comments 2 * address review comments 3 * address review comments 4

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 23, 2022

k8s-ci-robot requested review from saad-ali and xing-yang September 23, 2022 11:55

k8s-ci-robot added kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/storage Categorizes an issue or PR as relevant to SIG Storage. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 23, 2022

wojtek-t self-assigned this Sep 23, 2022

wojtek-t reviewed Sep 23, 2022

View reviewed changes

keps/sig-storage/3333-reconcile-default-storage-class/kep.yaml Show resolved Hide resolved

RomanBednar force-pushed the sc-kep-beta branch from afe23c0 to 0bcb840 Compare September 23, 2022 12:32

jsafrane reviewed Sep 26, 2022

View reviewed changes

keps/sig-storage/3333-reconcile-default-storage-class/README.md Outdated Show resolved Hide resolved

keps/sig-storage/3333-reconcile-default-storage-class/kep.yaml Outdated Show resolved Hide resolved

RomanBednar force-pushed the sc-kep-beta branch from 0bcb840 to 277023f Compare September 27, 2022 08:44

wojtek-t reviewed Sep 27, 2022

View reviewed changes

jsafrane reviewed Sep 27, 2022

View reviewed changes

RomanBednar force-pushed the sc-kep-beta branch from 277023f to c373d10 Compare September 27, 2022 15:11

wojtek-t reviewed Sep 28, 2022

View reviewed changes

RomanBednar force-pushed the sc-kep-beta branch from c373d10 to 848276a Compare September 29, 2022 07:26

wojtek-t reviewed Sep 29, 2022

View reviewed changes

RomanBednar force-pushed the sc-kep-beta branch from 848276a to 72d60cc Compare September 29, 2022 12:58

sftim reviewed Sep 29, 2022

View reviewed changes

RomanBednar added 5 commits September 30, 2022 14:24

update RetroactiveDefaultStorageClass for beta graduation

72b7b1d

address review comments 1

f526ad0

address review comments 2

0093f68

address review comments 3

6ea06f4

address review comments 4

40d2ebe

RomanBednar force-pushed the sc-kep-beta branch from 72d60cc to 40d2ebe Compare September 30, 2022 12:24

k8s-ci-robot assigned jsafrane Oct 3, 2022

k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Oct 3, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 3, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 3, 2022

k8s-ci-robot merged commit 32bccf4 into kubernetes:master Oct 3, 2022

k8s-ci-robot added this to the v1.26 milestone Oct 3, 2022

rhockenbury mentioned this pull request Oct 7, 2022

Retroactive default StorageClass assignement #3333

Closed

12 tasks

RomanBednar mentioned this pull request Feb 1, 2023

KEP-3762: PersistentVolume last phase transition time #3796

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KEP-3333: Graduate RetroactiveDefaultStorageClass to beta in v1.26 #3544

KEP-3333: Graduate RetroactiveDefaultStorageClass to beta in v1.26 #3544

RomanBednar commented Sep 23, 2022

wojtek-t Sep 27, 2022

RomanBednar Sep 27, 2022

wojtek-t Sep 27, 2022

RomanBednar Sep 27, 2022

jsafrane Sep 27, 2022

RomanBednar Sep 27, 2022

RomanBednar commented Sep 27, 2022 •

edited

Loading

wojtek-t Sep 28, 2022

RomanBednar Sep 29, 2022

wojtek-t Sep 29, 2022

RomanBednar Sep 29, 2022

jsafrane Sep 29, 2022

wojtek-t Sep 30, 2022

RomanBednar Sep 30, 2022

wojtek-t Sep 29, 2022

RomanBednar Sep 29, 2022

sftim Sep 29, 2022

RomanBednar Sep 30, 2022

wojtek-t commented Oct 3, 2022

jsafrane commented Oct 3, 2022

k8s-ci-robot commented Oct 3, 2022

KEP-3333: Graduate RetroactiveDefaultStorageClass to beta in v1.26 #3544

KEP-3333: Graduate RetroactiveDefaultStorageClass to beta in v1.26 #3544

Conversation

RomanBednar commented Sep 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RomanBednar commented Sep 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wojtek-t commented Oct 3, 2022

jsafrane commented Oct 3, 2022

k8s-ci-robot commented Oct 3, 2022

RomanBednar commented Sep 27, 2022 •

edited

Loading