Skip to content

Commit

Permalink
graduate PersistentVolumeLastPhaseTransitionTime to beta
Browse files Browse the repository at this point in the history
  • Loading branch information
@RomanBednar
RomanBednar committed Sep 12, 2023
1 parent 16d5ddf commit 7df2bc2
Show file tree
Hide file tree
Showing 3 changed files with 94 additions and 70 deletions.
2 changes: 2 additions & 0 deletions keps/prod-readiness/sig-storage/3762.yaml
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
kep-number: 3762
alpha:
approver: "@wojtek-t"
beta:
approver: "@wojtek-t"
155 changes: 89 additions & 66 deletions keps/sig-storage/3762-persistent-volume-last-phase-transition-time/README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -230,9 +230,8 @@ The "Design Details" section below is for the real
nitty-gritty.
-->

We need to update API server to support the newly proposed field and update PV controller to set a value of the new
timestamp field when a volume transitions to a different phase. Also, if the feature gate is disabled the value must be
re-set to `nil` when updating or creating a volume.
We need to update API server to support the newly proposed field and set a value of the new timestamp field when a volume
transitions to a different phase. The timestamp field must be set to current time also for newly created volumes.

The value of the field is not intended for use by any other Kubernetes components at this point and should be used only
as a convenience feature for cluster admins. Cluster admins should be able to list and sort PersistentVolumes based on
Expand Down Expand Up @@ -273,10 +272,6 @@ Adding the field to a PV is reasonable only when the PV actually transitions its
capture meaningful timestamp. Trying to do this at any other step than phase transition would capture a timestamp
that would semantically incorrect and misleading.

Removing the value can be done more freely, but tradeoffs need to be considered. One alternative approach discussed was
removing the field values more aggressively, during each PV sync. As this might introduce performance issues and does
not add much value the removal should be done during PV validation instead.

### Risks and Mitigations

<!--
Expand Down Expand Up @@ -308,15 +303,9 @@ Changes required for this KEP:
...
}
```
* validation should check timestamp format
* validation should allow update from `nil`
* validation should allow timestamp update if the previous timestamp is older
* validation should not allow updating to a point in time before the current timestamp
* kube-controller-manager / PV controller
* update the timestamp whenever PV controller transitions PV to a different phase (`pv.Status.Phase`)
* remove the timestamp in `LastPhaseTransitionTime` field during volume status update when feature gate is disabled
* update the timestamp whenever PV transitions to a different phase (`pv.Status.Phase`)
* allow `LastPhaseTransitionTime` to be updated by users if needed
* reset the timestamp in `LastPhaseTransitionTime` to `nil` only when feature gate is disabled and `LastPhaseTransitionTime` is not initialized (time is zero)
<!--
This section should contain enough information that the specifics of your
Expand Down Expand Up @@ -378,7 +367,6 @@ Changes will be implemented in packages with sufficient unit test coverage.
For any new or changed code we will add new unit tests.
- `pkg/controller/volume/persistentvolume`: `2023-01-25` - `79%`
- `pkg/apis/core/validation/`: `2023-01-25` - `82%`
##### Integration tests
Expand Down Expand Up @@ -408,12 +396,6 @@ We expect no non-infra related flakes in the last month as a GA graduation crite
We plan to add new e2e tests which should not interfere with any other tests, and so they could run in parallel.
While the timestamp of volume phase transition will represent an accurate point in time of when it occurred, the tests
will have to consider the time difference between user action that leads to a PV phase transition and the actual volume
phase transition done by the PV controller.
Based on exploratory testing we will define an appropriate time tolerance which will represent maximum time limit for
the volume to transition phase.
### Graduation Criteria
<!--
Expand Down Expand Up @@ -488,8 +470,6 @@ in back-to-back releases.
#### Beta
- Allowing time for feedback (at least 2 releases between beta and GA).
- Manually test version skew between the API server and KCM. See the expected
behavior below in Version Skew Strategy.
- Manually test upgrade->downgrade->upgrade path.
#### GA
Expand All @@ -512,15 +492,10 @@ enhancement:
No change in cluster upgrade / downgrade process.
When upgrading the new `LastPhaseTransitionTime` field and its value will be added to a PV when it transitions phase.
When upgrading, the new `LastPhaseTransitionTime` field and its value will be added to PVs when transitioning phase -
this means that enabling and disabling feature gate might not have an immediate effect.
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 create and update persistent volume REST strategy to remove
the values. More specifically each time the strategy invokes `PrepareForCreate` or `PrepareForUpdate` method we set the
value to `nil` if feature gate is disabled.
This means that enabling and disabling feature gate might not have an immediate effect. See "Notes/Constraints/Caveats"
section for more details.
See "Notes/Constraints/Caveats" section for more details.
### Version Skew Strategy
Expand All @@ -537,12 +512,14 @@ enhancement:
CRI or CNI may require updating that component before the kubelet.
-->
| API server | KCM | Behavior |
|------------|-----|----------------------------------------------------------------------------------------------------------------------------|
| off | off | Existing Kubernetes behavior. |
| on | off| Existing Kubernetes behavior, only users can set `pvc.Status.LastPhaseTransitionTime`. |
| off | on | PV controller may try to change `pvc.Status.LastPhaseTransitionTime`, which will fail on the API server. |
| on | on | New behavior.
Version skew is not applicable, KCM was not changed in scope of this enhancement.
| API server | KCM | Behavior |
|------------|-----|-----------------------|
| off | N/A | Existing Kubernetes behavior.|
| on | N/A | New behavior. |
| off | N/A | Existing Kubernetes behavior. |
| on | N/A | New behavior. |
## Production Readiness Review Questionnaire
Expand Down Expand Up @@ -588,7 +565,7 @@ well as the [existing list] of feature gates.
- [X] Feature gate (also fill in values in `kep.yaml`)
- Feature gate name: PersistentVolumeLastPhaseTransitionTime
- Components depending on the feature gate: kube-apiserver, kube-controller-manager
- Components depending on the feature gate: kube-apiserver
- [ ] Other
- Describe the mechanism:
- Will enabling / disabling the feature require downtime of the control
Expand Down Expand Up @@ -618,32 +595,30 @@ feature.
NOTE: Also set `disable-supported` to `true` or `false` in `kep.yaml`.
-->
Yes. This will result in the timestamp value being eventually set to `nil`. More details in
"Upgrade / Downgrade Strategy" section.
Yes, for PVs not updated while feature was enabled. However once the `LastPhaseTransitionTime` value is set, disabling
feature gate will not remove the value.
More details in "Upgrade / Downgrade Strategy" section.
###### What happens if we reenable the feature if it was previously rolled back?
No issues expected. There are three cases that can occur for a PV:
No issues expected. There are two cases that can occur for a PV:
1. PV did not transition its phase when feature gate was enabled - the `LastPhaseTransitionTime` field was not added
to the PV object so this is the same case as enabling the feature gate for the first time.
2. PV did transition its phase but the `LastPhaseTransitionTime` *was not* reset to `nil` because the PV was not
validated when the feature was enabled - the timestamp value will be updated on next phase change as if the feature
gate was never disabled.
2. PV did transition its phase but the `LastPhaseTransitionTime` *was* reset to `nil` because the PV was validated at
least once already while feature was enabled - the timestamp value will be updated on next phase change because
updates from `nil` are allowed.
2. PV did transition its phase when feature gate was enabled - the `LastPhaseTransitionTime` field is already set,
and it's timestamp value will be updated on next phase change.
See "Upgrade / Downgrade Strategy" and "Notes/Constraints/Caveats" sections for more details.
###### Are there any tests for feature enablement/disablement?
Unit tests for enabling and disabling feature gate will be added when transitioning to beta. See "Graduation criteria"
section.
Unit tests for enabling and disabling feature gate will be added in alpha - see "Graduation criteria" section.
The tests should focus on verifying correct handling of the new PV field in relation to feature gate state. Correct
handling means the values of the newly added field are added or updated when PV transitions its phase and cleared when
the feature is disabled.
handling means the values of the newly added field are added or updated when PV transitions its phase while feature gate
is enabled, and persisted if already set and feature gate is disabled.
<!--
The e2e framework does not currently support enabling or disabling feature
Expand Down Expand Up @@ -676,13 +651,20 @@ rollout. Similarly, consider large clusters and how enablement/disablement
will rollout across nodes.
-->
Rollout should not fail and there should be no need for rollback as this enhancement only adds a new field.
Also, rollback in terms of removal of this new field is not possible, once a PV is updated with the new field it can not
be removed after disabling the feature.
###### What specific metrics should inform a rollback?
<!--
What signals should users be paying attention to when the feature is young
that might indicate a serious problem?
-->
No metrics are required. Not having the new field set after enabling this feature is a sufficient signal to indicate
that there is a problem.
###### Were upgrade and rollback tested? Was the upgrade->downgrade->upgrade path tested?
<!--
Expand All @@ -691,12 +673,26 @@ 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.
-->
Manual upgrade->downgrade->upgrade test was performed to verify correct behavior of the new field. If a downgrade is performed
there are two scenarios that can occur for each PV:
1) Phase transitioned while feature was enabled - in this case the feature gets disabled and any updates to `LastPhaseTransitionTime` field must not be allowed.
2) Phase did not transition while feature was enabled - in this case the timestamp value must be persisted in `LastPhaseTransitionTime` field.
After upgrading again the behavior has to match behavior as if the feature was turned on for the first time.
The difference between feature enablement/disablement and downgrade/upgrade is that after downgrading to a version that
does not support `LastPhaseTransitionTime` field the data can not be accessed. Whereas only disabling the feature will
still show last the value that was set, if present.
###### Is the rollout accompanied by any deprecations and/or removals of features, APIs, fields of API types, flags, etc.?
<!--
Even if applying deprecation policies, they may still surprise some users.
-->
No.
### Monitoring Requirements
<!--
Expand All @@ -714,6 +710,8 @@ checking if there are objects with field X set) may be a last resort. Avoid
logs or events for this purpose.
-->
PV objects can be inspected for `LastPhaseTransitionTime` field.
###### How can someone using this feature know that it is working for their instance?
<!--
Expand All @@ -725,13 +723,8 @@ and operation of this feature.
Recall that end users cannot usually observe component logs or access metrics.
-->
- [ ] Events
- Event Reason:
- [ ] API .status
- Condition name:
- Other field:
- [ ] Other (treat as last resort)
- Details:
- [X] API .status
- Other field: `pvc.Status.LastPhaseTransitionTime`
###### What are the reasonable SLOs (Service Level Objectives) for the enhancement?
Expand All @@ -756,12 +749,9 @@ question.
Pick one more of these and delete the rest.
-->
- [ ] Metrics
- Metric name:
- [Optional] Aggregation method:
- Components exposing the metric:
- [ ] Other (treat as last resort)
- Details:
- [X] Other (treat as last resort)
- Details: To check correct functionality, inspect `LastPhaseTransitionTime` of a PV after binding it to a PVC.
Or simply create a PVC and check dynamically provisioned PV if it has a `LastPhaseTransitionTime` set to current time.
###### Are there any missing metrics that would be useful to have to improve observability of this feature?
Expand All @@ -770,6 +760,8 @@ Describe the metrics themselves and the reasons why they weren't added (e.g., co
implementation difficulties, etc.).
-->
Due to the simple nature if this feature there's no need to add any metric.
### Dependencies
<!--
Expand All @@ -793,6 +785,8 @@ and creating new ones, as well as about cluster-level services (e.g. DNS):
- Impact of its degraded performance or high-error rates on the feature:
-->
No.
### Scalability
<!--
Expand Down Expand Up @@ -820,6 +814,8 @@ Focusing mostly on:
heartbeats, leader election, etc.)
-->
No, the feature is implemented directly in API strategy for updating PVs.
###### Will enabling / using this feature result in introducing new API types?
<!--
Expand All @@ -829,6 +825,8 @@ Describe them, providing:
- Supported number of objects per namespace (for namespace-scoped objects)
-->
No.
###### Will enabling / using this feature result in any new calls to the cloud provider?
<!--
Expand All @@ -837,6 +835,8 @@ Describe them, providing:
- Estimated increase:
-->
No.
###### Will enabling / using this feature result in increasing size or count of the existing API objects?
<!--
Expand All @@ -846,6 +846,8 @@ Describe them, providing:
- Estimated amount of new objects: (e.g., new Object X for every existing Pod)
-->
Yes, all PV objects will have an entirely new field.
###### Will enabling / using this feature result in increasing time taken by any operations covered by existing SLIs/SLOs?
<!--
Expand All @@ -857,6 +859,8 @@ Think about adding additional work or introducing new steps in between
[existing SLIs/SLOs]: https://git.k8s.io/community/sig-scalability/slos/slos.md#kubernetes-slisslos
-->
No.
###### Will enabling / using this feature result in non-negligible increase of resource usage (CPU, RAM, disk, IO, ...) in any components?
<!--
Expand All @@ -869,6 +873,8 @@ This through this both in small and large cases, again with respect to the
[supported limits]: https://git.k8s.io/community//sig-scalability/configs-and-limits/thresholds.md
-->
No.
###### Can enabling / using this feature result in resource exhaustion of some node resources (PIDs, sockets, inodes, etc.)?
<!--
Expand All @@ -880,6 +886,8 @@ Are there any tests that were run/should be run to understand performance charac
and validate the declared limits?
-->
No.
### Troubleshooting
<!--
Expand All @@ -895,6 +903,9 @@ details). For now, we leave it here.
###### How does this feature react if the API server and/or etcd is unavailable?
If API server or etcd is unavailable objects can not be updated. Since this feature relies on PVs being updated to
set `LastPhaseTransitionTime` field this feature is basically disabled in this case.
###### What are other known failure modes?
<!--
Expand All @@ -910,8 +921,12 @@ For each of them, fill in the following information by copying the below templat
- Testing: Are there any tests for failure mode? If not, describe why.
-->
None, the feature is dependent only on API server and should not be affected by other failures.
###### What steps should be taken if SLOs are not being met to determine the problem?
Users should inspect API server logs for errors in case PV objects are not updated properly.
## Implementation History
<!--
Expand All @@ -931,6 +946,8 @@ Major milestones might include:
Why should this KEP _not_ be implemented?
-->
No drawbacks discovered, enhancement only adds a new informative field.
## Alternatives
<!--
Expand All @@ -939,10 +956,16 @@ not need to be as detailed as the proposal, but should include enough
information to express the idea and why it was not acceptable.
-->
Alternative solution is to update phase transition timestamp in PV controller/KCM. This would increase chances of having
a time skew between API audit logs and the timestamp. Updating phase transition timestamp in API strategy code is
therefore a better solution.
## Infrastructure Needed (Optional)
<!--
Use this section if you need things from the project/SIG. Examples include a
new subproject, repos requested, or GitHub details. Listing these here allows a
SIG to get the process for these resources started right away.
-->
None.
Loading

0 comments on commit 7df2bc2

Please sign in to comment.