graduate PersistentVolumeLastPhaseTransitionTime to beta in v1.29 #120627

RomanBednar · 2023-09-13T09:54:50Z

What type of PR is this?

/kind feature
/kind api-change

What this PR does / why we need it:

PersistentVolumeLastPhaseTransitionTime is planned to graduate to beta in 1.29.

PR for updating KEP: kubernetes/enhancements#4125

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Here are the criteria for beta graduation according to this KEP:

Manually test upgrade->downgrade->upgrade path: KEP-3762: graduate PersistentVolumeLastPhaseTransitionTime to beta in v1.29 enhancements#4125 (comment)

Does this PR introduce a user-facing change?

PersistentVolumeLastPhaseTransitionTime is now beta, enabled by default.

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

- [KEP]: https://kep.k8s.io/3762

k8s-ci-robot · 2023-09-13T09:54:59Z

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.

RomanBednar · 2023-09-13T10:16:38Z

/assign @jsafrane

/sig storage

carlory · 2023-09-13T14:40:15Z

pkg/registry/core/persistentvolume/strategy.go

@@ -143,7 +143,7 @@ func (persistentvolumeStatusStrategy) GetResetFields() map[fieldpath.APIVersion]
 	return fields
 }

-var nowFunc = metav1.Now
+var NowFunc = metav1.Now


How about adding a comment explaining the reason why the function is defined, e.g. for a test purpose.

This function is not entirely for testing, I just exported it to allow override in tests. It is used to set actual timestamp in PV strategy: https://github.com/kubernetes/kubernetes/blob/0fcfea649cda8915b1ceaa886eeea23d903afb55/pkg/registry/core/persistentvolume/strategy.go#L74

jsafrane · 2023-10-09T18:07:10Z

/lgtm

k8s-ci-robot · 2023-10-09T18:07:17Z

LGTM label has been added.

Git tree hash: 5e743e73814a428680689c9017bdcba1ed99242b

jsafrane · 2023-10-09T18:08:35Z

/label api-review

msau42 · 2023-10-24T02:09:46Z

pkg/registry/core/persistentvolume/storage/storage_test.go

@@ -19,6 +19,7 @@ package storage
 import (


Should we explicit enable the feature gate in these tests?

I've added it for the modified test (TestUpdateStatus). Explicit is better, but is there any other advantage if it's enabled by default now?

@jsafrane PTAL again, lgtm was removed. Here's the diff, the followup push is just a rebase.

NowFunc() has to be exported so it is available for testing in other packages.

…s update test

After enabling PersistentVolumeLastPhaseTransitionTime feature, any test that compares PV objects that transitioned phase needs to handle timestamp values correctly. Either the tests should avoid phase transitions if not needed or the test needs to set the same timestamp on new PV object so it's not changed and can be checked for equality later, the latter is used in this commit.

carlory · 2023-10-26T10:14:49Z

pkg/registry/core/persistentvolume/storage/storage_test.go

@@ -166,6 +173,7 @@ func TestWatch(t *testing.T) {
 }

 func TestUpdateStatus(t *testing.T) {
+	defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.PersistentVolumeLastPhaseTransitionTime, true)()


Should we add more testcases? i.e. one's featute-gate is on and another's featute-gate is off.

Tests were added mostly in alpha to strategy_test.go and are quite extensive, pretty much covering all possible scenarios in combination with feature gate enablement/disablement. I just fixed this test because timestamp addition broke it (as expected). Is there some specific test case that you have in mind that we might have missed?

RomanBednar · 2023-10-26T11:49:30Z

/retest-required

msau42 · 2023-10-27T03:00:17Z

/lgtm
/approve

k8s-ci-robot · 2023-10-27T03:00:23Z

LGTM label has been added.

Git tree hash: b20e590d2243cd740f8be31c07dbfa1a3a5e0aee

msau42 · 2023-10-27T03:01:01Z

/assign @deads2k

dims · 2023-10-27T20:29:44Z

/approve

k8s-ci-robot · 2023-10-27T20:30:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dims, msau42, RomanBednar

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/features/OWNERS~~ [dims,msau42]
~~pkg/registry/OWNERS~~ [dims]

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

BBBmau · 2024-01-12T04:08:20Z

Looks like the docs didn't get updated that it is now in beta. Making lastPhaseTransitionTime not an alpha field anymore.

RomanBednar · 2024-01-12T09:01:52Z

@BBBmau You're right, I missed that when moving the feature to beta. Thank you for raising the issue, I've opened a PR to remove the comment here: #122728

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

k8s-ci-robot requested review from justinsb and thockin September 13, 2023 09:55

k8s-ci-robot assigned jsafrane Sep 13, 2023

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 Sep 13, 2023

RomanBednar mentioned this pull request Sep 13, 2023

promote PersistentVolumeLastPhaseTransitionTime to beta #120418

Closed

carlory reviewed Sep 13, 2023

View reviewed changes

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

k8s-ci-robot added the api-review Categorizes an issue or PR as actively needing an API review. label Oct 9, 2023

liggitt assigned msau42 Oct 12, 2023

liggitt added this to Assigned in API Reviews Oct 12, 2023

This was referenced Oct 17, 2023

graduate PersistentVolumeLastPhaseTransitionTime to beta in v1.29 kubernetes/website#43532

Merged

PersistentVolume last phase transition time kubernetes/enhancements#3762

Open

msau42 reviewed Oct 24, 2023

View reviewed changes

RomanBednar added 3 commits October 26, 2023 11:47

graduate PersistentVolumeLastPhaseTransitionTime to beta

9cfd96d

export time function

071a67d

NowFunc() has to be exported so it is available for testing in other packages.

enable PersistentVolumeLastPhaseTransitionTime feature gate for statu…

5333989

…s update test

RomanBednar force-pushed the pv-phase-transition-time-beta branch from 0fcfea6 to e42e659 Compare October 26, 2023 09:47

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

k8s-ci-robot requested review from jsafrane and msau42 October 26, 2023 09:47

RomanBednar force-pushed the pv-phase-transition-time-beta branch from e42e659 to fb872e8 Compare October 26, 2023 09:47

carlory reviewed Oct 26, 2023

View reviewed changes

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

k8s-ci-robot assigned deads2k Oct 27, 2023

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

k8s-ci-robot merged commit 6a4f08e into kubernetes:master Oct 27, 2023
15 checks passed

k8s-ci-robot added this to the v1.29 milestone Oct 27, 2023

BBBmau mentioned this pull request Jan 12, 2024

Remove alpha reference from PersistentVolumeStatus. lastPhaseTransitionTime docs kubernetes/website#44711

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

graduate PersistentVolumeLastPhaseTransitionTime to beta in v1.29 #120627

graduate PersistentVolumeLastPhaseTransitionTime to beta in v1.29 #120627

RomanBednar commented Sep 13, 2023

k8s-ci-robot commented Sep 13, 2023

RomanBednar commented Sep 13, 2023

carlory Sep 13, 2023

RomanBednar Sep 14, 2023

jsafrane commented Oct 9, 2023

k8s-ci-robot commented Oct 9, 2023

jsafrane commented Oct 9, 2023

msau42 Oct 24, 2023

RomanBednar Oct 26, 2023

RomanBednar Oct 26, 2023

carlory Oct 26, 2023

RomanBednar Oct 26, 2023

RomanBednar commented Oct 26, 2023

msau42 commented Oct 27, 2023

k8s-ci-robot commented Oct 27, 2023

msau42 commented Oct 27, 2023

dims commented Oct 27, 2023

k8s-ci-robot commented Oct 27, 2023

BBBmau commented Jan 12, 2024

RomanBednar commented Jan 12, 2024

graduate PersistentVolumeLastPhaseTransitionTime to beta in v1.29 #120627

graduate PersistentVolumeLastPhaseTransitionTime to beta in v1.29 #120627

Conversation

RomanBednar commented Sep 13, 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 Sep 13, 2023

RomanBednar commented Sep 13, 2023

carlory Sep 13, 2023

Choose a reason for hiding this comment

RomanBednar Sep 14, 2023

Choose a reason for hiding this comment

jsafrane commented Oct 9, 2023

k8s-ci-robot commented Oct 9, 2023

jsafrane commented Oct 9, 2023

msau42 Oct 24, 2023

Choose a reason for hiding this comment

RomanBednar Oct 26, 2023

Choose a reason for hiding this comment

RomanBednar Oct 26, 2023

Choose a reason for hiding this comment

carlory Oct 26, 2023

Choose a reason for hiding this comment

RomanBednar Oct 26, 2023

Choose a reason for hiding this comment

RomanBednar commented Oct 26, 2023

msau42 commented Oct 27, 2023

k8s-ci-robot commented Oct 27, 2023

msau42 commented Oct 27, 2023

dims commented Oct 27, 2023

k8s-ci-robot commented Oct 27, 2023

BBBmau commented Jan 12, 2024

RomanBednar commented Jan 12, 2024