Consider an absent condition to be Unknown (#106)

Author: jonathan-innis

go.mod

@@ -17,7 +17,6 @@ require (
 	k8s.io/apimachinery v0.31.2
 	k8s.io/client-go v0.31.2
 	k8s.io/klog/v2 v2.130.1
-	k8s.io/kubernetes v1.31.3
 	k8s.io/utils v0.0.0-20240711033017-18e509b52bc8
 	sigs.k8s.io/controller-runtime v0.19.1
 	sigs.k8s.io/yaml v1.4.0

go.sum

@@ -190,8 +190,6 @@ k8s.io/klog/v2 v2.130.1 h1:n9Xl7H1Xvksem4KFG4PYbdQCQxqc/tTUyrgXaOhHSzk=
 k8s.io/klog/v2 v2.130.1/go.mod h1:3Jpz1GvMt720eyJH1ckRHK1EDfpxISzJ7I9OYgaDtPE=
 k8s.io/kube-openapi v0.0.0-20240228011516-70dd3763d340 h1:BZqlfIlq5YbRMFko6/PM7FjZpUb45WallggurYhKGag=
 k8s.io/kube-openapi v0.0.0-20240228011516-70dd3763d340/go.mod h1:yD4MZYeKMBwQKVht279WycxKyM84kkAx2DPrTXaeb98=
-k8s.io/kubernetes v1.31.3 h1:oqb7HdfnTelrGlZ6ziNugvQ/L/aJWR704114EAhUn9Q=
-k8s.io/kubernetes v1.31.3/go.mod h1:9xmT2buyTYj8TRKwRae7FcuY8k5+xlxv7VivvO0KKfs=
 k8s.io/utils v0.0.0-20240711033017-18e509b52bc8 h1:pUdcCO1Lk/tbT5ztQWOBi5HBgbBP1J8+AsQnQCKsi8A=
 k8s.io/utils v0.0.0-20240711033017-18e509b52bc8/go.mod h1:OLgZIPagt7ERELqWJFomSt595RzquPNLL48iOWgYOg0=
 sigs.k8s.io/controller-runtime v0.19.1 h1:Son+Q40+Be3QWb+niBXAg2vFiYWolDjjRfO8hn/cxOk=

object/object.go

@@ -10,8 +10,8 @@ import (
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/apimachinery/pkg/util/dump"
 	"k8s.io/client-go/kubernetes/scheme"
-	"k8s.io/kubernetes/pkg/util/hash"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/apiutil"
 	"sigs.k8s.io/yaml"
@@ -56,6 +56,7 @@ func Unmarshal[T any](raw []byte) *T {
 
 func Hash(o any) string {
 	h := fnv.New64a()
-	hash.DeepHashObject(h, o)
-	return strconv.FormatUint(uint64(h.Sum64()), 10)
+	h.Reset()
+	fmt.Fprintf(h, "%v", dump.ForHash(o))
+	return strconv.FormatUint(h.Sum64(), 10)
 }

status/condition_set.go

@@ -54,7 +54,8 @@ type ConditionSet struct {
 func (r ConditionTypes) For(object Object) ConditionSet {
 	cs := ConditionSet{object: object, ConditionTypes: r}
 	// Set known conditions Unknown if not set.
-	for _, t := range append(r.dependents, r.root) {
+	// Set the root condition first to get consistent timing for LastTransitionTime
+	for _, t := range append([]string{r.root}, r.dependents...) {
 		if cs.Get(t) == nil {
 			cs.SetUnknown(t)
 		}
@@ -99,25 +100,41 @@ func (c ConditionSet) IsTrue(conditionTypes ...string) bool {
 	return true
 }
 
+func (c ConditionSet) IsDependentCondition(t string) bool {
+	return t == c.root || lo.Contains(c.dependents, t)
+}
+
 // Set sets or updates the Condition on Conditions for Condition.Type.
 // If there is an update, Conditions are stored back sorted.
 func (c ConditionSet) Set(condition Condition) (modified bool) {
-	conditionType := condition.Type
 	var conditions []Condition
-	condition.LastTransitionTime = metav1.Now()
+	var foundCondition bool
+
 	condition.ObservedGeneration = c.object.GetGeneration()
 	for _, cond := range c.object.GetConditions() {
-		if cond.Type != conditionType {
+		if cond.Type != condition.Type {
 			conditions = append(conditions, cond)
 		} else {
-			if condition.Status == cond.Status && !cond.LastTransitionTime.IsZero() {
+			foundCondition = true
+			if condition.Status == cond.Status {
 				condition.LastTransitionTime = cond.LastTransitionTime
+			} else {
+				condition.LastTransitionTime = metav1.Now()
 			}
 			if reflect.DeepEqual(condition, cond) {
 				return false
 			}
 		}
 	}
+	if !foundCondition {
+		// Dependent conditions should always be set, so if it's not found, that means
+		// that we are initializing the condition type, and it's last "transition" was object creation
+		if c.IsDependentCondition(condition.Type) {
+			condition.LastTransitionTime = c.object.GetCreationTimestamp()
+		} else {
+			condition.LastTransitionTime = metav1.Now()
+		}
+	}
 	conditions = append(conditions, condition)
 	// Sorted for convenience of the consumer, i.e. kubectl.
 	sort.SliceStable(conditions, func(i, j int) bool {
@@ -130,21 +147,21 @@ func (c ConditionSet) Set(condition Condition) (modified bool) {
 	c.object.SetConditions(conditions)
 
 	// Recompute the root condition after setting any other condition
-	c.recomputeRootCondition(conditionType)
+	c.recomputeRootCondition(condition.Type)
 	return true
 }
 
-// Clear removes the abnormal condition that matches the ConditionType
-// Not implemented for normal conditions
+// Clear removes the independent condition that matches the ConditionType
+// Not implemented for dependent conditions
 func (c ConditionSet) Clear(t string) error {
 	var conditions []Condition
 
 	if c.object == nil {
 		return nil
 	}
-	// Normal conditions are not handled as they can't be nil
-	if t == c.root || lo.Contains(c.dependents, t) {
-		return fmt.Errorf("clearing normal conditions not implemented")
+	// Dependent conditions are not handled as they can't be nil
+	if c.IsDependentCondition(t) {
+		return fmt.Errorf("clearing dependent conditions not implemented")
 	}
 	cond := c.Get(t)
 	if cond == nil {

status/condition_set_test.go

@@ -13,8 +13,7 @@ import (
 
 var _ = Describe("Conditions", func() {
 	It("should correctly toggle conditions", func() {
-		testObject := test.CustomObject{}
-		testObject.Generation = 1
+		testObject := test.Object(&test.CustomObject{})
 		// Conditions should be initialized
 		conditions := testObject.StatusConditions()
 		Expect(conditions.Get(test.ConditionTypeFoo).GetStatus()).To(Equal(metav1.ConditionUnknown))

status/controller_test.go

@@ -10,7 +10,6 @@ import (
 	"github.com/awslabs/operatorpkg/status"
 	"github.com/awslabs/operatorpkg/test"
 	. "github.com/awslabs/operatorpkg/test/expectations"
-	"github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -44,7 +43,7 @@ var _ = Describe("Controller", func() {
 	BeforeEach(func() {
 		recorder = record.NewFakeRecorder(10)
 		kubeClient = fake.NewClientBuilder().WithScheme(scheme.Scheme).Build()
-		ctx = log.IntoContext(context.Background(), ginkgo.GinkgoLogr)
+		ctx = log.IntoContext(context.Background(), GinkgoLogr)
 		controller = status.NewController[*test.CustomObject](kubeClient, recorder, status.EmitDeprecatedMetrics)
 	})
 	AfterEach(func() {
@@ -517,14 +516,54 @@ var _ = Describe("Controller", func() {
 			}()
 		}
 	})
+	It("should set LastTransitionTime for status conditions on initialization to CreationTimestamp", func() {
+		testObject := test.Object(&test.CustomObject{})
+		testObject.StatusConditions() // initialize conditions after applying and setting CreationTimestamp
+
+		Expect(testObject.StatusConditions().Get(test.ConditionTypeFoo).LastTransitionTime.Time).To(Equal(testObject.GetCreationTimestamp().Time))
+		Expect(testObject.StatusConditions().Get(test.ConditionTypeBar).LastTransitionTime.Time).To(Equal(testObject.GetCreationTimestamp().Time))
+		Expect(testObject.StatusConditions().Get(status.ConditionReady).LastTransitionTime.Time).To(Equal(testObject.GetCreationTimestamp().Time))
+	})
+	It("should consider status conditions that aren't set as unknown", func() {
+		// This mimics an object creation
+		testObject := test.Object(&test.CustomObject{})
+		ExpectApplied(ctx, kubeClient, testObject)
+		ExpectReconciled(ctx, controller, testObject)
+
+		// Then the status conditions gets initialized and a condition is set to True
+		testObject.StatusConditions().SetTrue(test.ConditionTypeFoo)
+		testObject.SetCreationTimestamp(metav1.Time{Time: time.Now().Add(time.Hour)})
+		ExpectApplied(ctx, kubeClient, testObject)
+		ExpectReconciled(ctx, controller, testObject)
+
+		Expect(GetMetric("operator_customobject_status_condition_transition_seconds", conditionLabels(status.ConditionReady, metav1.ConditionTrue))).To(BeNil())
+		Expect(GetMetric("operator_customobject_status_condition_transition_seconds", conditionLabels(status.ConditionReady, metav1.ConditionFalse))).To(BeNil())
+		Expect(GetMetric("operator_customobject_status_condition_transition_seconds", conditionLabels(status.ConditionReady, metav1.ConditionUnknown))).To(BeNil())
+		Expect(GetMetric("operator_customobject_status_condition_transition_seconds", conditionLabels(ConditionTypeFoo, metav1.ConditionTrue))).To(BeNil())
+		Expect(GetMetric("operator_customobject_status_condition_transition_seconds", conditionLabels(ConditionTypeFoo, metav1.ConditionFalse))).To(BeNil())
+		Expect(GetMetric("operator_customobject_status_condition_transition_seconds", conditionLabels(ConditionTypeFoo, metav1.ConditionUnknown)).GetHistogram().GetSampleCount()).To(BeNumerically(">", 0))
+		Expect(GetMetric("operator_customobject_status_condition_transition_seconds", conditionLabels(ConditionTypeBar, metav1.ConditionTrue))).To(BeNil())
+		Expect(GetMetric("operator_customobject_status_condition_transition_seconds", conditionLabels(ConditionTypeBar, metav1.ConditionFalse))).To(BeNil())
+		Expect(GetMetric("operator_customobject_status_condition_transition_seconds", conditionLabels(ConditionTypeBar, metav1.ConditionUnknown))).To(BeNil())
+
+		Expect(GetMetric("operator_customobject_status_condition_transitions_total", conditionLabels(status.ConditionReady, metav1.ConditionTrue))).To(BeNil())
+		Expect(GetMetric("operator_customobject_status_condition_transitions_total", conditionLabels(status.ConditionReady, metav1.ConditionFalse))).To(BeNil())
+		Expect(GetMetric("operator_customobject_status_condition_transitions_total", conditionLabels(status.ConditionReady, metav1.ConditionUnknown))).To(BeNil())
+		Expect(GetMetric("operator_customobject_status_condition_transitions_total", conditionLabels(ConditionTypeFoo, metav1.ConditionTrue)).GetCounter().GetValue()).To(BeEquivalentTo(1))
+		Expect(GetMetric("operator_customobject_status_condition_transitions_total", conditionLabels(ConditionTypeFoo, metav1.ConditionFalse))).To(BeNil())
+		Expect(GetMetric("operator_customobject_status_condition_transitions_total", conditionLabels(ConditionTypeFoo, metav1.ConditionUnknown))).To(BeNil())
+		Expect(GetMetric("operator_customobject_status_condition_transitions_total", conditionLabels(ConditionTypeBar, metav1.ConditionTrue))).To(BeNil())
+		Expect(GetMetric("operator_customobject_status_condition_transitions_total", conditionLabels(ConditionTypeBar, metav1.ConditionFalse))).To(BeNil())
+		Expect(GetMetric("operator_customobject_status_condition_transitions_total", conditionLabels(ConditionTypeBar, metav1.ConditionUnknown))).To(BeNil())
+	})
 })
 
 var _ = Describe("Generic Controller", func() {
 	var genericController *status.GenericObjectController[*TestGenericObject]
 	BeforeEach(func() {
 		recorder = record.NewFakeRecorder(10)
 		kubeClient = fake.NewClientBuilder().WithScheme(scheme.Scheme).Build()
-		ctx = log.IntoContext(context.Background(), ginkgo.GinkgoLogr)
+		ctx = log.IntoContext(context.Background(), GinkgoLogr)
 		genericController = status.NewGenericObjectController[*TestGenericObject](kubeClient, recorder, status.EmitDeprecatedMetrics)
 	})
 	AfterEach(func() {

test/object.go

@@ -29,10 +29,12 @@ func Object[T client.Object](base T, overrides ...T) T {
 	dest := reflect.New(reflect.TypeOf(base).Elem()).Interface().(T)
 	dest.SetName(RandomName())
 	dest.SetNamespace(Namespace.Name)
+	dest.SetGeneration(1)
 	dest.SetLabels(lo.Assign(dest.GetLabels(), map[string]string{DiscoveryLabel: dest.GetName()}))
 	for _, src := range append([]T{base}, overrides...) {
 		lo.Must0(mergo.Merge(dest, src, mergo.WithOverride))
 	}
+	dest.SetCreationTimestamp(metav1.Now())
 	return dest
 }