Further tidying of runtime and meta elements

- Simplification of `*Reason` constants in `meta` API package. They now no longer assume to be tied to a specific condition type, and can be used as "generic" reasons within the context of a type. This includes the removal of the `DependencyNotReady` reason. Instead, API objects should have a dedicated condition type that reflects the an observation of the state of the dependencies combined with one of the generic reasons, or a custom one. - Change of `ObjectWithStatusConditions` interface to `ObjectWithConditions`, and introduction of `ObjectWithConditionsSetter`. This interface is further expanded by the `conditions` package to perform getter and setter operations on API type structures that adhere to the interface. - Alignment of method names in the `Metrics` helper, and introduction of `RecordReconciling` and `RecordStalled` methods to provide shortcuts for the generic `meta` condition types. - Replacement of the private interfaces in the `controller` helpers with well-known interfaces for the context they are used in, or are already expected to adhere to. Signed-off-by: Hidde Beydals <[email protected]>
fluxcd · Jun 23, 2021 · a8405cb · a8405cb
1 parent 2a0f2b6
commit a8405cb
Show file tree

Hide file tree

Showing 7 changed files with 50 additions and 70 deletions.
diff --git a/apis/meta/conditions.go b/apis/meta/conditions.go
@@ -17,14 +17,13 @@ limitations under the License.
 package meta
 
 import (
-	apimeta "k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
 const (
 	// ReadyCondition indicates the resource is ready and fully reconciled.
 	// If the Condition is False, the resource should be considered to be in the process
-	// of reconciling and not an representation of actual state.
+	// of reconciling and not a representation of actual state.
 	ReadyCondition string = "Ready"
 
 	// StalledCondition indicates the reconciliation of the resource has stalled, e.g.
@@ -43,45 +42,33 @@ const (
 )
 
 const (
-	// ReconciliationSucceededReason represents the fact that the reconciliation of
+	// SucceededReason represents the fact that the reconciliation of
 	// a toolkit resource has succeeded.
-	ReconciliationSucceededReason string = "ReconciliationSucceeded"
+	SucceededReason string = "Succeeded"
 
-	// ReconciliationFailedReason represents the fact that the reconciliation of a
+	// FailedReason represents the fact that the reconciliation of a
 	// toolkit resource has failed.
-	ReconciliationFailedReason string = "ReconciliationFailed"
+	FailedReason string = "Failed"
 
 	// ProgressingReason represents the fact that the reconciliation of a toolkit
 	// resource is underway.
 	ProgressingReason string = "Progressing"
 
-	// DependencyNotReadyReason represents the fact that one of the toolkit resource
-	// dependencies is not ready.
-	DependencyNotReadyReason string = "DependencyNotReady"
-
 	// SuspendedReason represents the fact that the reconciliation of a toolkit
 	// resource is suspended.
 	SuspendedReason string = "Suspended"
 )
 
-// ObjectWithStatusConditions is an interface that describes kubernetes resource
-// type structs with Status Conditions
+// ObjectWithConditions is an interface that describes Kubernetes API type
+// structs that have status conditions.
 // +k8s:deepcopy-gen=false
-type ObjectWithStatusConditions interface {
-	GetStatusConditions() *[]metav1.Condition
+type ObjectWithConditions interface {
+	GetConditions() []metav1.Condition
 }
 
-// SetResourceCondition sets the given condition with the given status,
-// reason and message on a resource.
-func SetResourceCondition(obj ObjectWithStatusConditions, condition string, status metav1.ConditionStatus, reason, message string) {
-	conditions := obj.GetStatusConditions()
-
-	newCondition := metav1.Condition{
-		Type:    condition,
-		Status:  status,
-		Reason:  reason,
-		Message: message,
-	}
-
-	apimeta.SetStatusCondition(conditions, newCondition)
+// ObjectWithConditionsSetter is an interface that describes Kubernetes API type
+// structs that have a status conditions setter.
+// +k8s:deepcopy-gen=false
+type ObjectWithConditionsSetter interface {
+	SetConditions([]metav1.Condition)
 }
diff --git a/runtime/conditions/getter.go b/runtime/conditions/getter.go
@@ -28,9 +28,7 @@ import (
 // use the conditions package for getting conditions.
 type Getter interface {
 	client.Object
-
-	// GetConditions returns the list of conditions for a GitOps Toolkit API object.
-	GetConditions() []metav1.Condition
+	meta.ObjectWithConditions
 }
 
 // Get returns the condition with the given type, if the condition does not exists,
@@ -236,7 +234,7 @@ func mirror(from Getter, targetCondition string, options ...MirrorOptions) *meta
 // the scope, no target condition is generated.
 func aggregate(from []Getter, targetCondition string, options ...MergeOption) *metav1.Condition {
 	mergeOpt := &mergeOptions{
-		stepCounter:    len(from),
+		stepCounter: len(from),
 	}
 	for _, o := range options {
 		o(mergeOpt)

diff --git a/runtime/conditions/merge_strategies.go b/runtime/conditions/merge_strategies.go
@@ -25,8 +25,8 @@ import (
 // mergeOptions allows to set strategies for merging a set of conditions into a single condition,
 // and more specifically for computing the target Reason and the target Message.
 type mergeOptions struct {
-	conditionTypes                     []string
-	negativePolarityConditionTypes     []string
+	conditionTypes                 []string
+	negativePolarityConditionTypes []string
 
 	addSourceRef                       bool
 	addSourceRefIfConditionTypes       []string
@@ -35,7 +35,7 @@ type mergeOptions struct {
 	addStepCounter                     bool
 	addStepCounterIfOnlyConditionTypes []string
 
-	stepCounter                        int
+	stepCounter int
 }
 
 // MergeOption defines an option for computing a summary of conditions.
@@ -183,7 +183,7 @@ func getMessage(groups conditionGroups, options *mergeOptions) string {
 func getCounterMessage(groups conditionGroups, to int) string {
 	topGroup := groups.TopGroup()
 	if topGroup == nil {
-		return fmt.Sprintf("%d of %d",0, to)
+		return fmt.Sprintf("%d of %d", 0, to)
 	}
 	ct := len(topGroup.conditions)
 	return fmt.Sprintf("%d of %d %s", ct, to, topGroup.conditions[0].Type)

diff --git a/runtime/conditions/setter.go b/runtime/conditions/setter.go
@@ -30,7 +30,7 @@ import (
 // use the conditions package for setting conditions.
 type Setter interface {
 	Getter
-	SetConditions([]metav1.Condition)
+	meta.ObjectWithConditionsSetter
 }
 
 // Set sets the given condition.

diff --git a/runtime/controller/events.go b/runtime/controller/events.go
@@ -21,11 +21,11 @@ import (
 
 	"github.com/go-logr/logr"
 	corev1 "k8s.io/api/core/v1"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	kuberecorder "k8s.io/client-go/tools/record"
 	"k8s.io/client-go/tools/reference"
 	ctrl "sigs.k8s.io/controller-runtime"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	"github.com/fluxcd/pkg/runtime/events"
 )
@@ -41,9 +41,9 @@ import (
 //     }
 //
 // You initialise a suitable value with MakeEvents; in most cases the
-// value only needs to be initialized once per controller, as the
-// specialised logger and object reference data are gathered from the
-// arguments provided to the Eventf method.
+// value needs to be initialized once per controller, as the specialised
+// logger and object reference data are gathered from the arguments
+// provided to the Eventf method.
 type Events struct {
 	Scheme                *runtime.Scheme
 	EventRecorder         kuberecorder.EventRecorder
@@ -58,20 +58,15 @@ func MakeEvents(mgr ctrl.Manager, controllerName string, ext *events.Recorder) E
 	}
 }
 
-type runtimeAndMetaObject interface {
-	runtime.Object
-	metav1.Object
-}
-
 // Event emits a Kubernetes event, and forwards the event to the
 // notification controller if configured.
-func (e Events) Event(ctx context.Context, obj runtimeAndMetaObject, metadata map[string]string, severity, reason, msg string) {
+func (e Events) Event(ctx context.Context, obj client.Object, metadata map[string]string, severity, reason, msg string) {
 	e.Eventf(ctx, obj, metadata, severity, reason, msg)
 }
 
 // Eventf emits a Kubernetes event, and forwards the event to the
 // notification controller if configured.
-func (e Events) Eventf(ctx context.Context, obj runtimeAndMetaObject, metadata map[string]string, severity, reason, msgFmt string, args ...interface{}) {
+func (e Events) Eventf(ctx context.Context, obj client.Object, metadata map[string]string, severity, reason, msgFmt string, args ...interface{}) {
 	if e.EventRecorder != nil {
 		e.EventRecorder.Eventf(obj, severityToEventType(severity), reason, msgFmt, args...)
 	}
@@ -88,6 +83,8 @@ func (e Events) Eventf(ctx context.Context, obj runtimeAndMetaObject, metadata m
 	}
 }
 
+// severityToEventType maps the given severity string to a corev1 event type.
+// In case of an unrecognised severity, EventTypeNormal is returned.
 func severityToEventType(severity string) string {
 	switch severity {
 	case events.EventSeverityError:

diff --git a/runtime/controller/metrics.go b/runtime/controller/metrics.go
@@ -20,9 +20,8 @@ import (
 	"context"
 	"time"
 
+	"github.com/fluxcd/pkg/runtime/conditions"
 	"github.com/go-logr/logr"
-	apimeta "k8s.io/apimachinery/pkg/api/meta"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/client-go/tools/reference"
 	ctrl "sigs.k8s.io/controller-runtime"
@@ -41,14 +40,16 @@ import (
 //       controller.Metrics
 //     }
 //
-// then you can call either or both of RecordDuration and
-// RecordReadinessMetric. API types used in GOTK will usually
-// already be suitable for passing (as a pointer) as the second
-// argument to `RecordReadinessMetric`.
+// Following the GOTK-standard, API types used in GOTK should implement
+// conditions.Getter to work with status condition types, and required
+// to be able to record metrics.
 //
-// When initialising controllers in main.go, use MustMakeMetrics to
+// When initialising the controllers in main.go, use MustMakeMetrics to
 // create a working Metrics value; you can supply the same value to
 // all reconcilers.
+//
+// Once initialised, metrics can be recorded by calling one of the
+// available `Record*` methods.
 type Metrics struct {
 	Scheme          *runtime.Scheme
 	MetricsRecorder *metrics.Recorder
@@ -64,7 +65,7 @@ func MustMakeMetrics(mgr ctrl.Manager) Metrics {
 	}
 }
 
-func (m Metrics) RecordDuration(ctx context.Context, obj readinessMetricsable, startTime time.Time) {
+func (m Metrics) RecordDuration(ctx context.Context, obj conditions.Getter, startTime time.Time) {
 	if m.MetricsRecorder != nil {
 		ref, err := reference.GetReference(m.Scheme, obj)
 		if err != nil {
@@ -75,7 +76,7 @@ func (m Metrics) RecordDuration(ctx context.Context, obj readinessMetricsable, s
 	}
 }
 
-func (m Metrics) RecordSuspend(ctx context.Context, obj readinessMetricsable, suspend bool) {
+func (m Metrics) RecordSuspend(ctx context.Context, obj conditions.Getter, suspend bool) {
 	if m.MetricsRecorder != nil {
 		ref, err := reference.GetReference(m.Scheme, obj)
 		if err != nil {
@@ -86,17 +87,19 @@ func (m Metrics) RecordSuspend(ctx context.Context, obj readinessMetricsable, su
 	}
 }
 
-type readinessMetricsable interface {
-	runtime.Object
-	metav1.Object
-	meta.ObjectWithStatusConditions
+func (m Metrics) RecordReadiness(ctx context.Context, obj conditions.Getter) {
+	m.RecordCondition(ctx, obj, meta.ReadyCondition)
+}
+
+func (m Metrics) RecordReconciling(ctx context.Context, obj conditions.Getter) {
+	m.RecordCondition(ctx, obj, meta.ReconcilingCondition)
 }
 
-func (m Metrics) RecordReadinessMetric(ctx context.Context, obj readinessMetricsable) {
-	m.RecordConditionMetric(ctx, obj, meta.ReadyCondition)
+func (m Metrics) RecordStalled(ctx context.Context, obj conditions.Getter) {
+	m.RecordCondition(ctx, obj, meta.StalledCondition)
 }
 
-func (m Metrics) RecordConditionMetric(ctx context.Context, obj readinessMetricsable, conditionType string) {
+func (m Metrics) RecordCondition(ctx context.Context, obj conditions.Getter, conditionType string) {
 	if m.MetricsRecorder == nil {
 		return
 	}
@@ -105,12 +108,9 @@ func (m Metrics) RecordConditionMetric(ctx context.Context, obj readinessMetrics
 		logr.FromContextOrDiscard(ctx).Error(err, "unable to get object reference to record condition metric")
 		return
 	}
-	rc := apimeta.FindStatusCondition(*obj.GetStatusConditions(), conditionType)
+	rc := conditions.Get(obj, conditionType)
 	if rc == nil {
-		rc = &metav1.Condition{
-			Type:   conditionType,
-			Status: metav1.ConditionUnknown,
-		}
+		rc = conditions.UnknownCondition(conditionType, "", "")
 	}
 	m.MetricsRecorder.RecordCondition(*ref, *rc, !obj.GetDeletionTimestamp().IsZero())
 }
diff --git a/runtime/metrics/recorder.go b/runtime/metrics/recorder.go
@@ -69,11 +69,9 @@ func (r *Recorder) RecordCondition(ref corev1.ObjectReference, condition metav1.
 
 func (r *Recorder) RecordSuspend(ref corev1.ObjectReference, suspend bool) {
 	var value float64
-
 	if suspend {
 		value = 1
 	}
-
 	r.suspendGauge.WithLabelValues(ref.Kind, ref.Name, ref.Namespace).Set(value)
 }