KEP-3762: PersistentVolume last phase transition time #3796

RomanBednar · 2023-01-30T11:21:55Z

One-line PR description:
PersistentVolume last phase transition time

Issue link:
PersistentVolume last phase transition time #3762

Other comments:

jsafrane · 2023-01-31T12:13:10Z

From verify CI job: Table of content not up to date. Did you run 'make update-toc' ?

jsafrane · 2023-01-31T12:18:16Z

Looks good enough for alpha otherwise.

keps/sig-storage/3762-persistent-volume-last-phase-transition-time/README.md

xing-yang · 2023-01-31T18:48:06Z

Can you run update-toc.sh to fix the toc syntax error?

xing-yang · 2023-01-31T18:49:30Z

Please add a file in the following directory for PRR approver:
https://github.com/kubernetes/enhancements/tree/master/keps/prod-readiness/sig-storage

msau42 · 2023-02-01T01:43:17Z

keps/sig-storage/3762-persistent-volume-last-phase-transition-time/README.md

+last used, which is when the volume transitioned to `Released` phase.
+
+We can approach the solution in a more generic way and record a timestamp of when the volume transitioned to any phase,
+not just to `Released` phase. This allows anyone, incl. our perf tests, to measure time e.g. between a PV `Pending` and `Bound`


also for providing metrics/SLO

keps/sig-storage/3762-persistent-volume-last-phase-transition-time/README.md

msau42 · 2023-02-01T01:44:17Z

keps/sig-storage/3762-persistent-volume-last-phase-transition-time/README.md

+Changes required for this KEP:
+
+* kube-apiserver
+  * extend PersistentVolumeStatus type with `LastPhaseTransitionTime` field


Can you add the proposed type changes?

keps/sig-storage/3762-persistent-volume-last-phase-transition-time/README.md

msau42 · 2023-02-02T22:58:49Z

keps/sig-storage/3762-persistent-volume-last-phase-transition-time/README.md

+
+1) Introduce a new status field in PersistentVolumes.
+2) Update the new field with a timestamp every time a volume transitions to a different phase (`pv.Status.Phase`).
+3) Improve general observability and allow SLO definitions for Attach/Detach volume operations.


I'm going to take back the use case for a SLO. I think a SLO may be better served by metrics improvements rather than adding timestamps to API objects, especially if we want to consider deletion cases.

Ack, removing this from goals.

keps/sig-storage/3762-persistent-volume-last-phase-transition-time/README.md

wojtek-t · 2023-02-03T14:58:53Z

keps/sig-storage/3762-persistent-volume-last-phase-transition-time/README.md

+#### Beta
+
+- Allowing time for feedback (at least 2 releases between beta and GA).
+- Add unit tests covering feature enablement/disablement.


Can you move it to Alpha? If we want anyone to enable it in alpha (if not then it's not particularly useful), this one seems important.

Indeed, moved.

wojtek-t · 2023-02-03T15:06:21Z

keps/sig-storage/3762-persistent-volume-last-phase-transition-time/README.md

+No change in cluster upgrade / downgrade process.
+
+When downgrading from a version that added the new timestamp field PVs we need to make sure that after downgrade the
+values of the disabled field are removed.


How exactly you're going to achieve it?

I'm assuming you're talking about "eventual removal" here, really, right?
[the controller will unset it on the next processing of that object if it realized FG is disabled and field is set]

Yes, but the term "next processing" is still quite broad, it would help to specify it more. The field presence check and resetting could be done:

While updating volume status (updateVolumePhase()) - the downside is that disabling FG will have no effect util volume status changes phase again. This does not seem optimal.

At any PV update - some PRs (e.g. this one) implement this by resetting value of the new field to nil during validation. This approach seems ok, is this a viable option for this enhancement as well? Also this presents a similar caveat as when the FG is enabled - the effect won't be immediate.

Make PV controller actively (on each sync) search for PVs with the new field and reset it if FG is disabled. This seems like an overkill. If the value is correct and reflects real last phase change there's no risk of persisting it in the PV objects even if FG is disabled.

I agree that option (3) is probably an overkill - this is what I meant above as "eventual removal".

I'm not sure if (1) isn't good enough - but no matter what decision you will make, it should be explicitly mentioned in the KEP.

Also - @msau42 - for her thoughts on it.

When a feature is disabled, the api server strategy implementation should drop disabled fields on write, so approach 2) should cover it.

Thank you @wojtek-t and @msau42 for the feedback - KEP has been updated.

wojtek-t · 2023-02-03T15:07:13Z

keps/sig-storage/3762-persistent-volume-last-phase-transition-time/README.md

+automations, so be extremely careful here.
+-->
+
+No. It only adds a new informative field to PV status.


So technically the answer is yes.

You're not changing anything in your cluster, and your PVs will start to contain a new field set.

Ok, I'll change it.

wojtek-t · 2023-02-03T15:10:44Z

keps/sig-storage/3762-persistent-volume-last-phase-transition-time/README.md

+
+###### What happens if we reenable the feature if it was previously rolled back?
+
+No issues expected, after rollback the field can be `nil` and validation should allow updates from `nil`.


I guess I missed that in the proposal, but when exactly this field will be set?

I'm assuming that we the status computed in the controller will be different and we will be patching the object.
If so, what happens after enabling the feature for the first time?
We have no idea when the last transition happened, so what we're going to do:

will the field remain to be unset?

are we going to (somewhat misleadingly) set it to now

[I guess the former, but then let's also add to Caveats section that the field will remain unset until the first change in PV status even after enabling the feature.]

If so, what happens after enabling the feature for the first time?

If feature is enabled for the first time there are two sub-cases where phase is modified that we need to think of:

While provisioner creates a PV it's phase is set to Bound - at this point I'd rather not set the field because it would be misleading, as you said. This field is meant for "last phase change" so it does not make sense to insert something else (like current time) just for the sake of having some value.

Updating volume status (updateVolumePhase()) - this is when the new LastPhaseTransitionTime field and its value appears in PV object for the first time. So it's good idea to write down the caveeats since enabling FG won't have an immediate effect.

OK - so please put that explicitly into your KEP so it's clear what exactly we can expect from this field.

Updated this section. Also adding one more design detail to explicitly state that we want to allow timestamp updates, not just from nil.

msau42 · 2023-02-06T19:11:01Z

The KEP lgtm once the comments are addressed

wojtek-t

Just one minor nit - other than that LGTM.

wojtek-t · 2023-02-07T12:15:46Z

keps/sig-storage/3762-persistent-volume-last-phase-transition-time/README.md

+NOTE: Also set `disable-supported` to `true` or `false` in `kep.yaml`.
+-->
+
+Yes. This will result in the timestamp value being set to `nil`. Mentioned in "Upgrade / Downgrade Strategy" section.


... timestamp value being eventually set to nil. More details in ...

wojtek-t · 2023-02-07T12:19:37Z

keps/sig-storage/3762-persistent-volume-last-phase-transition-time/README.md

+
+When downgrading from a version that added the new timestamp field PVs we need to make sure that after downgrade the
+values of the disabled field are removed. We intend to use API server strategy implementation, more specifically PV
+validation, to remove the values - each time a PV gets validated we will set the value to `nil` if feature gate is disabled.


Actually one more thing - k8s validation isn't generally supposed to change anything.

I think what you really want is effectively add that to PrepareForCreate/PrepareForUpdate methods:
https://github.com/kubernetes/kubernetes/blob/master/pkg/registry/core/persistentvolume/strategy.go

Right, this is where I was aiming but shouldn't have used the term "validation" this way. I changed this to be specific about the strategy and the concrete methods.

logicalhan · 2023-02-07T12:35:05Z

keps/sig-storage/3762-persistent-volume-last-phase-transition-time/README.md

+Based on exploratory testing we will define an appropriate time tolerance which will represent maximum time limit for
+the volume to transition phase.


Won't this result in a flaky test?

Technically yes but this kind of polling is widely used in tests already. For example here. So this will not flake more than any other test that has timeout for some specific action to happen.

logicalhan · 2023-02-07T12:35:52Z

keps/sig-storage/3762-persistent-volume-last-phase-transition-time/README.md

+- Feature implemented behind a feature flag
+- Unit tests completed and enabled
+- Add unit tests covering feature enablement/disablement.
+- Initial e2e tests completed and enabled


How do you add e2e tests for an alpha feature when the feature is behind a feature gate?

There are dedicated alpha suites for this purpose.

logicalhan · 2023-02-07T12:39:33Z

keps/sig-storage/3762-persistent-volume-last-phase-transition-time/README.md

+- [ ] Metrics
+  - Metric name:
+  - [Optional] Aggregation method:
+  - Components exposing the metric:


Might be worth measuring the time it takes to transition phases.

But that depends on what exactly is happening (e.g. type of volume, etc.)
I wouldn't bundle it together with this KEP, at least at this point.

xing-yang · 2023-02-08T14:00:47Z

Comments are addressed.

/lgtm
/approve

wojtek-t · 2023-02-08T20:27:33Z

/lgtm
/approve PRR

Thanks!

k8s-ci-robot · 2023-02-08T20:27:44Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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~~ [xing-yang]

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 added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 30, 2023

k8s-ci-robot requested review from saad-ali and xing-yang January 30, 2023 11:22

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/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 30, 2023

xing-yang reviewed Jan 31, 2023

View reviewed changes

msau42 reviewed Feb 1, 2023

View reviewed changes

wojtek-t reviewed Feb 1, 2023

View reviewed changes

keps/sig-storage/3762-persistent-volume-last-phase-transition-time/README.md Show resolved Hide resolved

RomanBednar force-pushed the pv-phase-transition-time branch from d56fd5c to 2491dff Compare February 1, 2023 14:04

wojtek-t self-assigned this Feb 1, 2023

msau42 reviewed Feb 2, 2023

View reviewed changes

RomanBednar force-pushed the pv-phase-transition-time branch from 37fd7d7 to adee430 Compare February 3, 2023 08:27

wojtek-t reviewed Feb 3, 2023

View reviewed changes

msau42 mentioned this pull request Feb 3, 2023

Update recovery kep with new APIs #3829

Merged

RomanBednar force-pushed the pv-phase-transition-time branch from adee430 to 2d16991 Compare February 7, 2023 12:02

wojtek-t reviewed Feb 7, 2023

View reviewed changes

logicalhan reviewed Feb 7, 2023

View reviewed changes

Add KEP for PersistentVolume last phase transition time

eb33be8

RomanBednar force-pushed the pv-phase-transition-time branch from 2d16991 to eb33be8 Compare February 8, 2023 13:43

k8s-ci-robot assigned xing-yang Feb 8, 2023

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

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

k8s-ci-robot merged commit 561aef2 into kubernetes:master Feb 8, 2023

k8s-ci-robot added this to the v1.27 milestone Feb 8, 2023

jsafrane mentioned this pull request Feb 9, 2023

PersistentVolume last phase transition time #3762

Closed

12 tasks

RomanBednar mentioned this pull request Mar 10, 2023

PersistentVolume last phase transition time kubernetes/kubernetes#116469

Merged

This was referenced Jul 11, 2023

Add blog for PersistentVolume last phase transition time kubernetes/website#41981

Merged

Add docs for PersistentVolume last phase transition time kubernetes/website#42008

Merged


		###### What happens if we reenable the feature if it was previously rolled back?

		No issues expected, after rollback the field can be `nil` and validation should allow updates from `nil`.

		Based on exploratory testing we will define an appropriate time tolerance which will represent maximum time limit for
		the volume to transition phase.

KEP-3762: PersistentVolume last phase transition time #3796

KEP-3762: PersistentVolume last phase transition time #3796

Conversation

RomanBednar commented Jan 30, 2023

jsafrane commented Jan 31, 2023

jsafrane commented Jan 31, 2023

xing-yang commented Jan 31, 2023

xing-yang commented Jan 31, 2023

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

msau42 Feb 6, 2023 • 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

msau42 commented Feb 6, 2023

wojtek-t left a 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

Choose a reason for hiding this comment

xing-yang commented Feb 8, 2023

wojtek-t commented Feb 8, 2023

k8s-ci-robot commented Feb 8, 2023

msau42 Feb 6, 2023 •

edited

Loading