Merge pull request #96825 from roycaihw/storage-version/conditions

storage-version: update conditions

pkg/controller/storageversiongc/BUILD

@@ -14,6 +14,7 @@ go_library(
         "//staging/src/k8s.io/apimachinery/pkg/util/errors:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/util/runtime:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/util/wait:go_default_library",
+        "//staging/src/k8s.io/apiserver/pkg/storageversion:go_default_library",
         "//staging/src/k8s.io/client-go/informers/apiserverinternal/v1alpha1:go_default_library",
         "//staging/src/k8s.io/client-go/informers/coordination/v1:go_default_library",
         "//staging/src/k8s.io/client-go/kubernetes:go_default_library",

pkg/controller/storageversiongc/gc_controller.go

@@ -28,6 +28,7 @@ import (
 	utilerrors "k8s.io/apimachinery/pkg/util/errors"
 	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
 	"k8s.io/apimachinery/pkg/util/wait"
+	"k8s.io/apiserver/pkg/storageversion"
 	apiserverinternalinformers "k8s.io/client-go/informers/apiserverinternal/v1alpha1"
 	coordinformers "k8s.io/client-go/informers/coordination/v1"
 	"k8s.io/client-go/kubernetes"
@@ -265,32 +266,13 @@ func (c *Controller) enqueueLease(obj *coordinationv1.Lease) {
 	c.leaseQueue.Add(obj.Name)
 }
 
-func setCommonEncodingVersion(sv *apiserverinternalv1alpha1.StorageVersion) {
-	if len(sv.Status.StorageVersions) == 0 {
-		return
-	}
-	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
-	} else {
-		sv.Status.CommonEncodingVersion = nil
-	}
-}
-
 func (c *Controller) updateOrDeleteStorageVersion(sv *apiserverinternalv1alpha1.StorageVersion, serverStorageVersions []apiserverinternalv1alpha1.ServerStorageVersion) error {
 	if len(serverStorageVersions) == 0 {
 		return c.kubeclientset.InternalV1alpha1().StorageVersions().Delete(
 			context.TODO(), sv.Name, metav1.DeleteOptions{})
 	}
 	sv.Status.StorageVersions = serverStorageVersions
-	setCommonEncodingVersion(sv)
+	storageversion.SetCommonEncodingVersion(sv)
 	_, err := c.kubeclientset.InternalV1alpha1().StorageVersions().UpdateStatus(
 		context.TODO(), sv, metav1.UpdateOptions{})
 	return err

staging/src/k8s.io/apiserver/pkg/storageversion/BUILD

@@ -32,6 +32,7 @@ go_test(
     embed = [":go_default_library"],
     deps = [
         "//staging/src/k8s.io/api/apiserverinternal/v1alpha1:go_default_library",
+        "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/runtime/schema:go_default_library",
         "//vendor/github.com/google/go-cmp/cmp:go_default_library",
     ],

staging/src/k8s.io/apiserver/pkg/storageversion/updater.go

@@ -35,10 +35,16 @@ type Client interface {
 	Get(context.Context, string, metav1.GetOptions) (*v1alpha1.StorageVersion, error)
 }
 
-func setCommonEncodingVersion(sv *v1alpha1.StorageVersion) {
+// SetCommonEncodingVersion updates the CommonEncodingVersion and the AllEncodingVersionsEqual
-	if len(sv.Status.StorageVersions) == 0 {
+// condition based on the StorageVersions.
-		return
+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 {
@@ -49,9 +55,70 @@ func setCommonEncodingVersion(sv *v1alpha1.StorageVersion) {
 		}
 		if agreed {
 			sv.Status.CommonEncodingVersion = &firstVersion
-	} else {
-		sv.Status.CommonEncodingVersion = nil
 		}
+	}
+
+	condition := v1alpha1.StorageVersionCondition{
+		Type:               v1alpha1.AllEncodingVersionsEqual,
+		Status:             v1alpha1.ConditionFalse,
+		ObservedGeneration: sv.Generation,
+		LastTransitionTime: metav1.NewTime(time.Now()),
+		Reason:             "CommonEncodingVersionUnset",
+		Message:            "Common encoding version unset",
+	}
+	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) {
+	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)
+	SetCommonEncodingVersion(sv)
 	return sv
 }

staging/src/k8s.io/apiserver/pkg/storageversion/updater_test.go

@@ -22,35 +22,46 @@ import (
 
 	"github.com/google/go-cmp/cmp"
 	"k8s.io/api/apiserverinternal/v1alpha1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
-func TestLocalUpdateStorageVersion(t *testing.T) {
+var (
-	v1 := "v1"
+	v1   = "v1"
-	ssv1 := v1alpha1.ServerStorageVersion{
+	v2   = "v2"
+	ssv1 = v1alpha1.ServerStorageVersion{
 		APIServerID:       "1",
 		EncodingVersion:   "v1",
 		DecodableVersions: []string{"v1", "v2"},
 	}
-	ssv2 := v1alpha1.ServerStorageVersion{
+	ssv2 = v1alpha1.ServerStorageVersion{
 		APIServerID:       "2",
 		EncodingVersion:   "v1",
 		DecodableVersions: []string{"v1", "v2"},
 	}
 	// ssv3 has a different encoding version
-	ssv3 := v1alpha1.ServerStorageVersion{
+	ssv3 = v1alpha1.ServerStorageVersion{
 		APIServerID:       "3",
 		EncodingVersion:   "v2",
 		DecodableVersions: []string{"v1", "v2"},
 	}
-	ssv4 := v1alpha1.ServerStorageVersion{
+	ssv4 = v1alpha1.ServerStorageVersion{
 		APIServerID:       "4",
 		EncodingVersion:   "v1",
 		DecodableVersions: []string{"v1", "v2", "v4"},
 	}
+	ssv5 = v1alpha1.ServerStorageVersion{
+		APIServerID:       "5",
+		EncodingVersion:   "v2",
+		DecodableVersions: []string{"v1", "v2", "v4"},
+	}
+)
+
+func TestLocalUpdateStorageVersion(t *testing.T) {
 	tests := []struct {
 		old                            v1alpha1.StorageVersionStatus
 		newSSV                         v1alpha1.ServerStorageVersion
 		expected                       v1alpha1.StorageVersionStatus
+		expectLastTransitionTimeUpdate bool
 	}{
 		{
 			old:    v1alpha1.StorageVersionStatus{},
@@ -58,36 +69,45 @@ func TestLocalUpdateStorageVersion(t *testing.T) {
 			expected: v1alpha1.StorageVersionStatus{
 				StorageVersions:       []v1alpha1.ServerStorageVersion{ssv1},
 				CommonEncodingVersion: &v1,
+				Conditions:            commonVersionTrueCondition(),
 			},
+			expectLastTransitionTimeUpdate: true,
 		},
 		{
 			old: v1alpha1.StorageVersionStatus{
 				StorageVersions:       []v1alpha1.ServerStorageVersion{ssv1, ssv2},
 				CommonEncodingVersion: &v1,
+				Conditions:            commonVersionTrueCondition(),
 			},
 			newSSV: ssv3,
 			expected: v1alpha1.StorageVersionStatus{
 				StorageVersions: []v1alpha1.ServerStorageVersion{ssv1, ssv2, ssv3},
+				Conditions:      commonVersionFalseCondition(),
 			},
+			expectLastTransitionTimeUpdate: true,
 		},
 		{
 			old: v1alpha1.StorageVersionStatus{
 				StorageVersions:       []v1alpha1.ServerStorageVersion{ssv1, ssv2},
 				CommonEncodingVersion: &v1,
+				Conditions:            commonVersionTrueCondition(),
 			},
 			newSSV: ssv4,
 			expected: v1alpha1.StorageVersionStatus{
 				StorageVersions:       []v1alpha1.ServerStorageVersion{ssv1, ssv2, ssv4},
 				CommonEncodingVersion: &v1,
+				Conditions:            commonVersionTrueCondition(),
 			},
 		},
 		{
 			old: v1alpha1.StorageVersionStatus{
 				StorageVersions: []v1alpha1.ServerStorageVersion{ssv1, ssv2, ssv3},
+				Conditions:      commonVersionFalseCondition(),
 			},
 			newSSV: ssv4,
 			expected: v1alpha1.StorageVersionStatus{
 				StorageVersions: []v1alpha1.ServerStorageVersion{ssv1, ssv2, ssv3, ssv4},
+				Conditions:      commonVersionFalseCondition(),
 			},
 		},
 	}
@@ -95,8 +115,143 @@ func TestLocalUpdateStorageVersion(t *testing.T) {
 	for _, tc := range tests {
 		sv := &v1alpha1.StorageVersion{Status: tc.old}
 		updated := localUpdateStorageVersion(sv, tc.newSSV.APIServerID, tc.newSSV.EncodingVersion, tc.newSSV.DecodableVersions)
+		if tc.expectLastTransitionTimeUpdate == updated.Status.Conditions[0].LastTransitionTime.IsZero() {
+			t.Errorf("unexpected LastTransitionTime, expected update: %v, got: %v",
+				tc.expectLastTransitionTimeUpdate, updated.Status.Conditions[0].LastTransitionTime)
+		}
+		updated.Status.Conditions[0].LastTransitionTime = metav1.Time{}
 		if e, a := tc.expected, updated.Status; !reflect.DeepEqual(e, a) {
 			t.Errorf("unexpected: %v", cmp.Diff(e, a))
 		}
 	}
 }
+
+func commonVersionTrueCondition() []v1alpha1.StorageVersionCondition {
+	return []v1alpha1.StorageVersionCondition{{
+		Type:    v1alpha1.AllEncodingVersionsEqual,
+		Status:  v1alpha1.ConditionTrue,
+		Reason:  "CommonEncodingVersionSet",
+		Message: "Common encoding version set",
+	}}
+}
+func commonVersionFalseCondition() []v1alpha1.StorageVersionCondition {
+	return []v1alpha1.StorageVersionCondition{{
+		Type:    v1alpha1.AllEncodingVersionsEqual,
+		Status:  v1alpha1.ConditionFalse,
+		Reason:  "CommonEncodingVersionUnset",
+		Message: "Common encoding version unset",
+	}}
+}
+
+func TestSetCommonEncodingVersion(t *testing.T) {
+	tests := []struct {
+		name                           string
+		old                            v1alpha1.StorageVersionStatus
+		expected                       v1alpha1.StorageVersionStatus
+		expectLastTransitionTimeUpdate bool
+	}{
+		{
+			name: "no-common_init",
+			old: v1alpha1.StorageVersionStatus{
+				StorageVersions: []v1alpha1.ServerStorageVersion{ssv1, ssv2, ssv3},
+			},
+			expected: v1alpha1.StorageVersionStatus{
+				StorageVersions: []v1alpha1.ServerStorageVersion{ssv1, ssv2, ssv3},
+				Conditions:      commonVersionFalseCondition(),
+			},
+			expectLastTransitionTimeUpdate: true,
+		},
+		{
+			name: "no-common_transition",
+			old: v1alpha1.StorageVersionStatus{
+				StorageVersions:       []v1alpha1.ServerStorageVersion{ssv1, ssv2, ssv3},
+				CommonEncodingVersion: &v1,
+				Conditions:            commonVersionTrueCondition(),
+			},
+			expected: v1alpha1.StorageVersionStatus{
+				StorageVersions: []v1alpha1.ServerStorageVersion{ssv1, ssv2, ssv3},
+				Conditions:      commonVersionFalseCondition(),
+			},
+			expectLastTransitionTimeUpdate: true,
+		},
+		{
+			name: "no-common_no-transition",
+			old: v1alpha1.StorageVersionStatus{
+				StorageVersions: []v1alpha1.ServerStorageVersion{ssv1, ssv2, ssv3},
+				Conditions:      commonVersionFalseCondition(),
+			},
+			expected: v1alpha1.StorageVersionStatus{
+				StorageVersions: []v1alpha1.ServerStorageVersion{ssv1, ssv2, ssv3},
+				Conditions:      commonVersionFalseCondition(),
+			},
+		},
+		{
+			name: "common_init",
+			old: v1alpha1.StorageVersionStatus{
+				StorageVersions: []v1alpha1.ServerStorageVersion{ssv1, ssv2},
+			},
+			expected: v1alpha1.StorageVersionStatus{
+				StorageVersions:       []v1alpha1.ServerStorageVersion{ssv1, ssv2},
+				CommonEncodingVersion: &v1,
+				Conditions:            commonVersionTrueCondition(),
+			},
+			expectLastTransitionTimeUpdate: true,
+		},
+		{
+			name: "common_no-transition",
+			old: v1alpha1.StorageVersionStatus{
+				StorageVersions:       []v1alpha1.ServerStorageVersion{ssv1, ssv2},
+				CommonEncodingVersion: &v1,
+				Conditions:            commonVersionTrueCondition(),
+			},
+			expected: v1alpha1.StorageVersionStatus{
+				StorageVersions:       []v1alpha1.ServerStorageVersion{ssv1, ssv2},
+				CommonEncodingVersion: &v1,
+				Conditions:            commonVersionTrueCondition(),
+			},
+		},
+		{
+			name: "common_transition",
+			old: v1alpha1.StorageVersionStatus{
+				StorageVersions: []v1alpha1.ServerStorageVersion{ssv1, ssv2},
+				Conditions:      commonVersionFalseCondition(),
+			},
+			expected: v1alpha1.StorageVersionStatus{
+				StorageVersions:       []v1alpha1.ServerStorageVersion{ssv1, ssv2},
+				CommonEncodingVersion: &v1,
+				Conditions:            commonVersionTrueCondition(),
+			},
+			expectLastTransitionTimeUpdate: true,
+		},
+		{
+			name: "common_version-changed",
+			old: v1alpha1.StorageVersionStatus{
+				StorageVersions:       []v1alpha1.ServerStorageVersion{ssv3, ssv5},
+				CommonEncodingVersion: &v1,
+				Conditions:            commonVersionTrueCondition(),
+			},
+			expected: v1alpha1.StorageVersionStatus{
+				StorageVersions:       []v1alpha1.ServerStorageVersion{ssv3, ssv5},
+				CommonEncodingVersion: &v2,
+				Conditions:            commonVersionTrueCondition(),
+			},
+			expectLastTransitionTimeUpdate: true,
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			sv := &v1alpha1.StorageVersion{Status: tc.old}
+			SetCommonEncodingVersion(sv)
+			if (tc.expectLastTransitionTimeUpdate && sv.Status.Conditions[0].LastTransitionTime.IsZero()) ||
+				(!tc.expectLastTransitionTimeUpdate && !sv.Status.Conditions[0].LastTransitionTime.IsZero()) {
+				t.Errorf("unexpected LastTransitionTime, expected update: %v, got: %v",
+					tc.expectLastTransitionTimeUpdate, sv.Status.Conditions[0].LastTransitionTime)
+			}
+			sv.Status.Conditions[0].LastTransitionTime = metav1.Time{}
+			if e, a := tc.expected, sv.Status; !reflect.DeepEqual(e, a) {
+				t.Errorf("unexpected: %v", cmp.Diff(e, a))
+			}
+		})
+	}
+}