review 3

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

@@ -266,6 +266,17 @@ Go in to as much detail as necessary here.
 This might be a good place to talk about core concepts and how they relate.
 -->
 
+The caveat of this proposal is that admins might not see the effect immediately after enabling/disabling the feature gate.
+This is due to how and when the new `LastPhaseTransitionTime` field needs to be added/removed.
+
+Adding the field to a PV is reasonable only when the PV actually transitions its phase - only at that point we can
+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
 
 <!--
@@ -300,6 +311,7 @@ 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
@@ -470,12 +482,12 @@ in back-to-back releases.
 
 - Feature implemented behind a feature flag
 - Unit tests completed and enabled
+- Add unit tests covering feature enablement/disablement.
 - Initial e2e tests completed and enabled
 
 #### Beta
 
 - Allowing time for feedback (at least 2 releases between beta and GA).
-- Add unit tests covering feature enablement/disablement.
 - 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.
@@ -500,8 +512,14 @@ 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 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.
+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.
+
+This means that enabling and disabling feature gate might not have an immediate effect. See "Notes/Constraints/Caveats"
+section for more details.
 
 ### Version Skew Strategy
 
@@ -584,7 +602,7 @@ Any change of default behavior may be surprising to users or break existing
 automations, so be extremely careful here.
 -->
 
-No. It only adds a new informative field to PV status.
+Yes. All PVs will start to contain the new `LastPhaseTransitionTime` field.
 
 ###### Can the feature be disabled once it has been enabled (i.e. can we roll back the enablement)?
 
@@ -603,7 +621,18 @@ Yes. This will result in the timestamp value being set to `nil`. Mentioned in "U
 
 ###### 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`.
+No issues expected. There are three 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.
+
+See "Upgrade / Downgrade Strategy" and "Notes/Constraints/Caveats" sections for more details.
 
 ###### Are there any tests for feature enablement/disablement?