storage-version: update conditions #96825
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,23 +35,90 @@ type Client interface { | |
| Get(context.Context, string, metav1.GetOptions) (*v1alpha1.StorageVersion, error) | ||
| } | ||
|
|
||
| // SetCommonEncodingVersion updates the CommonEncodingVersion and the AllEncodingVersionsEqual | ||
| // condition based on the StorageVersions. | ||
| func SetCommonEncodingVersion(sv *v1alpha1.StorageVersion) { | ||
| var oldCommonEncodingVersion *string | ||
| if sv.Status.CommonEncodingVersion != nil { | ||
| version := *sv.Status.CommonEncodingVersion | ||
| oldCommonEncodingVersion = &version | ||
| } | ||
| sv.Status.CommonEncodingVersion = nil | ||
| if len(sv.Status.StorageVersions) != 0 { | ||
| firstVersion := sv.Status.StorageVersions[0].EncodingVersion | ||
| agreed := true | ||
| for _, ssv := range sv.Status.StorageVersions { | ||
| if ssv.EncodingVersion != firstVersion { | ||
| agreed = false | ||
| break | ||
| } | ||
| } | ||
| if agreed { | ||
| sv.Status.CommonEncodingVersion = &firstVersion | ||
| } | ||
| } | ||
|
|
||
| condition := v1alpha1.StorageVersionCondition{ | ||
| Type: v1alpha1.AllEncodingVersionsEqual, | ||
| Status: v1alpha1.ConditionFalse, | ||
| ObservedGeneration: sv.Generation, | ||
| LastTransitionTime: metav1.NewTime(time.Now()), | ||
| Reason: "CommonEncodingVersionUnset", | ||
| Message: "Common encoding version unset", | ||
|
Comment on lines
+66
to
+67
There was a problem hiding this comment. for free-form text we are likely to tweak in future versions, make sure we won't eternally duel with a future API server that sets a slightly different version of this condition. xref things like #78133 |
||
| } | ||
| if sv.Status.CommonEncodingVersion != nil { | ||
| condition.Status = v1alpha1.ConditionTrue | ||
| condition.Reason = "CommonEncodingVersionSet" | ||
| condition.Message = "Common encoding version set" | ||
| } | ||
| forceTransition := false | ||
| if oldCommonEncodingVersion != nil && sv.Status.CommonEncodingVersion != nil && | ||
| *oldCommonEncodingVersion != *sv.Status.CommonEncodingVersion { | ||
| forceTransition = true | ||
| } | ||
| setStatusCondition(&sv.Status.Conditions, condition, forceTransition) | ||
| } | ||
|
|
||
| func findStatusCondition(conditions []v1alpha1.StorageVersionCondition, | ||
| conditionType v1alpha1.StorageVersionConditionType) *v1alpha1.StorageVersionCondition { | ||
| for i := range conditions { | ||
| if conditions[i].Type == conditionType { | ||
| return &conditions[i] | ||
| } | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| // setStatusCondition sets the corresponding condition in conditions to newCondition. | ||
| // conditions must be non-nil. | ||
| // 1. if the condition of the specified type already exists: all fields of the existing condition are updated to | ||
| // newCondition, LastTransitionTime is set to now if the new status differs from the old status | ||
| // 2. if a condition of the specified type does not exist: LastTransitionTime is set to now() if unset, | ||
| // and newCondition is appended | ||
| // NOTE: forceTransition allows overwriting LastTransitionTime even when the status doesn't change. | ||
| func setStatusCondition(conditions *[]v1alpha1.StorageVersionCondition, newCondition v1alpha1.StorageVersionCondition, | ||
| forceTransition bool) { | ||
|
There was a problem hiding this comment. I'd like to keep the current name so that the function can be used for other storage version conditions in future |
||
| if conditions == nil { | ||
| return | ||
| } | ||
|
|
||
| if newCondition.LastTransitionTime.IsZero() { | ||
| newCondition.LastTransitionTime = metav1.NewTime(time.Now()) | ||
| } | ||
| existingCondition := findStatusCondition(*conditions, newCondition.Type) | ||
| if existingCondition == nil { | ||
| *conditions = append(*conditions, newCondition) | ||
| return | ||
| } | ||
|
|
||
| statusChanged := existingCondition.Status != newCondition.Status | ||
| if statusChanged || forceTransition { | ||
| existingCondition.LastTransitionTime = newCondition.LastTransitionTime | ||
| } | ||
| existingCondition.Status = newCondition.Status | ||
| existingCondition.Reason = newCondition.Reason | ||
| existingCondition.Message = newCondition.Message | ||
|
Comment on lines
+119
to
+120
There was a problem hiding this comment. this is the bit that seems likely to duel... should we skip if the ObservedGeneration of the existing condition is already set to the current generation? There was a problem hiding this comment. StorageVersion has no spec, so the ObservedGeneration is always 0. IIUC, #78133 happened because one controller's update would always trigger another controller to do calculation and update. This doesn't apply to storage version because:
There was a problem hiding this comment. The two controllers updating the condition are the storageVersionGC and the API server. One storageVersion GC or on API server updating the condition of a storageVersion won't cause other GC or API server update the storageVersion again, so this won't cause a duel. |
||
| existingCondition.ObservedGeneration = newCondition.ObservedGeneration | ||
| } | ||
|
There was a problem hiding this comment. I simplified the function a little, and it still passed the unit test, how about: func setStatusCondition(conditions *[]v1alpha1.StorageVersionCondition, newCondition v1alpha1.StorageVersionCondition,
versionChanged bool) {
if conditions == nil {
return
}
existingCondition := findStatusCondition(*conditions, newCondition.Type)
if existingCondition == nil {
*conditions = append(*conditions, newCondition)
return
}
statusChanged := existingCondition.Status != newCondition.Status
if statusChanged || versionChanged {
existingCondition.LastTransitionTime = newCondition.LastTransitionTime
}
existingCondition.Status = newCondition.Status
existingCondition.Reason = newCondition.Reason
existingCondition.Message = newCondition.Message
existingCondition.ObservedGeneration = newCondition.ObservedGeneration
}There was a problem hiding this comment. Got it. Then how about func setStatusCondition(conditions *[]v1alpha1.StorageVersionCondition, newCondition v1alpha1.StorageVersionCondition,
forceTransition bool) {
if conditions == nil {
return
}
if newCondition.LastTransitionTime.IsZero() {
newCondition.LastTransitionTime = metav1.NewTime(time.Now())
}
existingCondition := findStatusCondition(*conditions, newCondition.Type)
if existingCondition == nil {
*conditions = append(*conditions, newCondition)
return
}
statusChanged := existingCondition.Status != newCondition.Status
if statusChanged || forceTransition {
existingCondition.LastTransitionTime = newCondition.LastTransitionTime
}
existingCondition.Status = newCondition.Status
existingCondition.Reason = newCondition.Reason
existingCondition.Message = newCondition.Message
existingCondition.ObservedGeneration = newCondition.ObservedGeneration
} |
||
|
|
||
| // updateStorageVersionFor updates the storage version object for the resource. | ||
|
|
@@ -123,6 +190,6 @@ func localUpdateStorageVersion(sv *v1alpha1.StorageVersion, apiserverID, encodin | |
| if !foundSSV { | ||
| sv.Status.StorageVersions = append(sv.Status.StorageVersions, newSSV) | ||
| } | ||
| SetCommonEncodingVersion(sv) | ||
| return sv | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is updateOrDeleteStorageVersion operating on an object obtained from an informer or a live lookup version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a live lookup version