KEP-3762: graduate PersistentVolumeLastPhaseTransitionTime to beta in v1.29

[RomanBednar]

• One-line PR description: update KEP for PersistentVolumeLastPhaseTransitionTime feature to match actual implementation and graduate to beta

• Issue link: PersistentVolume last phase transition time #3762

• Other comments:
  This is a followup for discussed changes:
  • PersistentVolume last phase transition time kubernetes#116469 (comment)
  • PersistentVolume last phase transition time kubernetes#116469 (comment)

[wojtek-t]

/assign @jsafrane

/sig storage

[RomanBednar]

Test rollout-rollback-rollout

Perform pre-upgrade tests (1.27.5)

Create a PVC to provision a volume:

$ cat /tmp/pvc.yaml
---
apiVersion: v1
kind: PersistentVolumeClaim
metadata:
  name: pvc-1
spec:
  accessModes:
    - ReadWriteOnce
  volumeMode: Filesystem
  resources:
    requests:
      storage: 1Gi
  storageClassName: csi-hostpath-sc

kc create -f /tmp/pvc.yaml

Verify the PV does not have lastPhaseTransitionTime set:

$ kc get pv/$(kc get pvc/pvc-1 -o json | jq '.spec.volumeName' | tr -d "\"")  -o json | jq  '.status.lastPhaseTransitionTime'
null

Upgrade cluster (1.27.5 -> 1.28.1)

Check available versions:

$ dnf search kubeadm --showduplicates --quiet | grep 1.28
kubeadm-1.28.0-0.x86_64 : Command-line utility for administering a Kubernetes cluster.
kubeadm-1.28.1-0.x86_64 : Command-line utility for administering a Kubernetes cluster.

Upgrade kubeadm:

$ sudo dnf install -y kubeadm-1.28.1-0

Prepare config file that enables FeatureGate:

$ cat /tmp/config.yaml
---
apiVersion: kubeadm.k8s.io/v1beta3
kind: ClusterConfiguration
apiServer:
  extraArgs:
    feature-gates: PersistentVolumeLastPhaseTransitionTime=true
controllerManager:
  extraArgs:
    cluster-cidr: 10.244.0.0/16
    feature-gates: PersistentVolumeLastPhaseTransitionTime=true

Perform kubeadm upgrade:

$ sudo kubeadm upgrade plan --config /tmp/config.yaml
$ sudo kubeadm upgrade apply --config /tmp/config.yaml v1.28.1

Perform kubelet upgrade:

$ sudo dnf install -y kubelet-1.28.1-0
$ sudo systemctl daemon-reload 
$ sudo systemctl restart kubelet

Perform post-upgrade tests

Create a second PVC to provision a volume:

$ cat /tmp/pvc2.yaml
---
apiVersion: v1
kind: PersistentVolumeClaim
metadata:
  name: pvc-2
spec:
  accessModes:
    - ReadWriteOnce
  volumeMode: Filesystem
  resources:
    requests:
      storage: 1Gi
  storageClassName: csi-hostpath-sc

kc create -f /tmp/pvc2.yaml

Verify it has lastPhaseTransitionTime set:

$ kc get pv/$(kc get pvc/pvc-2 -o json | jq '.spec.volumeName' | tr -d "\"")  -o json | jq  '.status.lastPhaseTransitionTime'
"2023-09-12T08:53:09Z"

Change retain policy on the first PV to Retain:

$ kc get pv/pvc-0c9ea251-b156-4786-ac82-8713b76bb312
NAME                                       CAPACITY   ACCESS MODES   RECLAIM POLICY   STATUS   CLAIM           STORAGECLASS      REASON   AGE
pvc-0c9ea251-b156-4786-ac82-8713b76bb312   1Gi        RWO            Retain           Bound    default/pvc-1   csi-hostpath-sc            52m

Delete PVC for the first volume to release the PV:

kc delete pvc/pvc-1

Verify the first (pre-upgrade) PVC transitioned phase and transition timestamp is now set:

$ kc get pv/pvc-f2eee26c-bca3-448b-9198-d4948f54dce3 -o json | jq '.status.phase'
"Released"

$ kc get pv/pvc-f2eee26c-bca3-448b-9198-d4948f54dce3 -o json | jq '.status.lastPhaseTransitionTime'
"2023-09-12T08:58:01Z"

Downgrade cluster (1.28.1 -> 1.27.5)

$ kc version -o json | jq '.serverVersion.gitVersion'
"v1.27.5"

Perform post-rollback tests

$ cat /tmp/pvc3.yaml
---
apiVersion: v1
kind: PersistentVolumeClaim
metadata:
  name: pvc-3
spec:
  accessModes:
    - ReadWriteOnce
  volumeMode: Filesystem
  resources:
    requests:
      storage: 1Gi
  storageClassName: csi-hostpath-sc

kc create -f /tmp/pvc3.yaml

Verify new PV does not have lastPhaseTransitionTime set:

$ kc get pv/$(kc get pvc/pvc-3 -o json | jq '.spec.volumeName' | tr -d "\"")  -o json | jq  '.status.lastPhaseTransitionTime'
null

Verify lastPhaseTransitionTime of previous PVs can not be accessed anymore:

$ kc get pv/$(kc get pvc/pvc-2 -o json | jq '.spec.volumeName' | tr -d "\"")  -o json | jq  '.status.lastPhaseTransitionTime'
null

Verify lastPhaseTransitionTime can not be set manually:

$ kc patch pvc/pvc-3 -p '{"status":{"lastPhaseTransitionTime":"2023-09-11T13:07:09Z"}}'
Warning: unknown field "status.lastPhaseTransitionTime"
persistentvolumeclaim/pvc-3 patched (no change)

Upgrade cluster again (1.27.5 -> 1.28.1)

Install/update kubeadm:

$ sudo dnf install -y kubeadm-1.28.1-0

Perform kubeadm upgrade:

$ sudo kubeadm upgrade plan --config /tmp/config.yaml
$ sudo kubeadm upgrade apply --config /tmp/config.yaml v1.28.1

Perform post-upgrade tests again

Verify timestamp is available again and unchanged on old PVs:

$ kc get pv/$(kc get pvc/pvc-2 -o json | jq '.spec.volumeName' | tr -d "\"")  -o json | jq  '.status.lastPhaseTransitionTime'
"2023-09-12T08:53:09Z"

$ kc get pv/pvc-f2eee26c-bca3-448b-9198-d4948f54dce3 -o json | jq '.status.lastPhaseTransitionTime'
"2023-09-12T08:58:01Z"

Change reclaim policy on exiting PV, release it and check lastPhaseTransitionTime is set correctly:

$ kc get pv/pvc-2e55f2fd-b0dc-4c95-b8d5-085d16ee6d27 -o json | jq '.spec.persistentVolumeReclaimPolicy'
"Delete"

$ kc patch pv/pvc-2e55f2fd-b0dc-4c95-b8d5-085d16ee6d27 -p '{"spec":{"persistentVolumeReclaimPolicy":"Retain"}}'
persistentvolume/pvc-2e55f2fd-b0dc-4c95-b8d5-085d16ee6d27 patched

$ kc get pv/pvc-2e55f2fd-b0dc-4c95-b8d5-085d16ee6d27 -o json | jq '.spec.persistentVolumeReclaimPolicy'
"Retain"

$ kc get pv/pvc-2e55f2fd-b0dc-4c95-b8d5-085d16ee6d27 -o json | jq '.status.phase'
"Bound"

$ kc delete pvc/pvc-2
persistentvolumeclaim "pvc-2" deleted

$ kc get pv/pvc-2e55f2fd-b0dc-4c95-b8d5-085d16ee6d27 -o json | jq '.status.phase'
"Released"

$ kc get pv/pvc-2e55f2fd-b0dc-4c95-b8d5-085d16ee6d27 -o json | jq '.status.lastPhaseTransitionTime'
"2023-09-12T12:05:07Z"

$ date
Tue Sep 12 12:05:24 PM UTC 2023

[wojtek-t]

The above test is great (I will look at details later), but can you put it into the KEP itself?

[RomanBednar]

The above test is great (I will look at details later), but can you put it into the KEP itself?

@wojtek-t Sure, moved.

[wojtek-t]

Few but very minor comments - overall this looks reasonable.

[wojtek-t]

let's remove the KCM column from the table - it doesn't matter

[RomanBednar]

Removed.

[wojtek-t]

I like the change - thanks!

[wojtek-t]

Were the tests added?

In fact this comment from the KEP template was introduced for exactly the cases like this:

Additionally, for features that are introducing a new API field, unit tests that
are exercising the `switch` of feature gate itself (what happens if I disable a
feature gate after having objects written with the new field) are also critical.
You can take a look at one potential example of such test in:
https://github.com/kubernetes/kubernetes/pull/97058/files#diff-7826f7adbc1996a05ab52e3f5f02429e94b68ce6bce0dc534d1be636154fded3R246-R282

[RomanBednar]

Yes, I've added a link to it.

[wojtek-t]

The field won't be deleted automatically, but we can manually reset it, right?

[RomanBednar]

Exactly, I'm adding a better explanation with examples.

[wojtek-t]

Well - not very likely, but one can imagine a some nil-pointer exception or sth like that (i.e. crashes of kube-apiserver when it tries to set it).

[RomanBednar]

Sure, rewording a bit.

[wojtek-t]

This is great - thanks a lot for thorough testing!

[wojtek-t]

line 944: please add something like:

N/A - no SLI defined here

[RomanBednar]

Added.

[wojtek-t]

nit: please add that it's a timestamp, so together with the field name it will be < 50 bytes or so

[RomanBednar]

Added.

[wojtek-t]

Just two super minor comments - once applied this LGTM.

[wojtek-t]

nit: remove these two lines now - those are duplicates now :)

[RomanBednar]

Missed that, removing.

[wojtek-t]

Sorry for confusion - I meant adding this for the question above (for SLO) - if we don't have SLIs, then SLO doesn't make sense.

[RomanBednar]

Thanks, moving.

[wojtek-t]

/lgtm
/approve PRR

Thanks!

@RomanBednar - you still need SIG approval for it

[jsafrane]

Please update the checklist at the beginning of the KEP.
Very well written KEP otherwise!

[jsafrane]

The link should lead to kubernetes/kubernetes, the commit is already merged there

[RomanBednar]

The link is wrong indeed - fixed. And checklist is updated too. Thank you for the review!

[jsafrane]

/lgtm
/approve

[k8s-ci-robot]

[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