Merge pull request #233 from smarterclayton/double_failure

Bug 1739988: Stop double reporting "failure" cluster_version

Author: openshift-merge-robot

docs/dev/metrics.md

@@ -8,7 +8,7 @@ The cluster version is reported as seconds since the epoch with labels for `vers
 * `current` - the version the operator is applying right now (the running CVO version) and the age of the payload
 * `initial` - the initial version of the cluster, and value is the creation timestamp of the cluster version (cluster age)
 * `cluster` - the current version of the cluster with from_version set to the initial version, and value is the creation timestamp of the cluster version (cluster age)
-* `failure` - if the failure condition is set, reports the last transition time for both desired and current versions
+* `failure` - if the failure condition is set, reports the last transition time for the condition.
 * `desired` - reported if different from current as the most recent timestamp on the cluster version
 * `completed` - the time the most recent version was completely applied, or absent if not reached
 * `updating` - if the operator is moving to a new version, the time the update started

pkg/cvo/metrics.go

@@ -3,10 +3,10 @@ package cvo
 import (
 	"time"
 
-	apierrors "k8s.io/apimachinery/pkg/api/errors"
-	"k8s.io/apimachinery/pkg/util/sets"
 	"github.com/prometheus/client_golang/prometheus"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/labels"
+	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/client-go/tools/cache"
 
 	configv1 "github.com/openshift/api/config/v1"
@@ -181,23 +181,21 @@ func (m *operatorMetrics) Collect(ch chan<- prometheus.Metric) {
 		g.Set(float64(cv.CreationTimestamp.Unix()))
 		ch <- g
 
-		failing := resourcemerge.FindOperatorStatusCondition(cv.Status.Conditions, ClusterStatusFailing)
+		// answers "is there a desired update we have not yet satisfied"
 		if update := cv.Spec.DesiredUpdate; update != nil && update.Image != current.Image {
-			g := m.version.WithLabelValues("desired", update.Version, update.Image, completed.Version)
+			g = m.version.WithLabelValues("desired", update.Version, update.Image, completed.Version)
 			g.Set(float64(mostRecentTimestamp(cv)))
 			ch <- g
-			if failing != nil && failing.Status == configv1.ConditionTrue {
-				g = m.version.WithLabelValues("failure", update.Version, update.Image, completed.Version)
-				if failing.LastTransitionTime.IsZero() {
-					g.Set(0)
-				} else {
-					g.Set(float64(failing.LastTransitionTime.Unix()))
-				}
-				ch <- g
-			}
 		}
+
+		// answers "if we are failing, are we updating or reconciling"
+		failing := resourcemerge.FindOperatorStatusCondition(cv.Status.Conditions, ClusterStatusFailing)
 		if failing != nil && failing.Status == configv1.ConditionTrue {
-			g := m.version.WithLabelValues("failure", current.Version, current.Image, completed.Version)
+			if update := cv.Spec.DesiredUpdate; update != nil && update.Image != current.Image {
+				g = m.version.WithLabelValues("failure", update.Version, update.Image, completed.Version)
+			} else {
+				g = m.version.WithLabelValues("failure", current.Version, current.Image, completed.Version)
+			}
 			if failing.LastTransitionTime.IsZero() {
 				g.Set(0)
 			} else {

pkg/cvo/metrics_test.go

@@ -344,6 +344,9 @@ func Test_operatorMetrics_Collect(t *testing.T) {
 								DesiredUpdate: &configv1.Update{Version: "1.0.0", Image: "test/image:2"},
 							},
 							Status: configv1.ClusterVersionStatus{
+								History: []configv1.UpdateHistory{
+									{State: configv1.CompletedUpdate, Version: "0.0.2", Image: "test/image:1", CompletionTime: &([]metav1.Time{{Time: time.Unix(2, 0)}}[0])},
+								},
 								Conditions: []configv1.ClusterOperatorStatusCondition{
 									{Type: ClusterStatusFailing, Status: configv1.ConditionTrue, LastTransitionTime: metav1.Time{Time: time.Unix(4, 0)}, Reason: "Because stuff"},
 								},
@@ -356,13 +359,13 @@ func Test_operatorMetrics_Collect(t *testing.T) {
 				if len(metrics) != 8 {
 					t.Fatalf("Unexpected metrics %s", spew.Sdump(metrics))
 				}
-				expectMetric(t, metrics[0], 5, map[string]string{"type": "initial", "version": "", "image": "", "from_version": ""})
-				expectMetric(t, metrics[1], 5, map[string]string{"type": "cluster", "version": "0.0.2", "image": "test/image:1", "from_version": ""})
-				expectMetric(t, metrics[2], 5, map[string]string{"type": "desired", "version": "1.0.0", "image": "test/image:2", "from_version": ""})
-				expectMetric(t, metrics[3], 4, map[string]string{"type": "failure", "version": "1.0.0", "image": "test/image:2", "from_version": ""})
-				expectMetric(t, metrics[4], 4, map[string]string{"type": "failure", "version": "0.0.2", "image": "test/image:1", "from_version": ""})
+				expectMetric(t, metrics[0], 2, map[string]string{"type": "completed", "version": "0.0.2", "image": "test/image:1", "from_version": ""})
+				expectMetric(t, metrics[1], 5, map[string]string{"type": "initial", "version": "0.0.2", "image": "test/image:1", "from_version": ""})
+				expectMetric(t, metrics[2], 5, map[string]string{"type": "cluster", "version": "0.0.2", "image": "test/image:1", "from_version": "0.0.2"})
+				expectMetric(t, metrics[3], 5, map[string]string{"type": "desired", "version": "1.0.0", "image": "test/image:2", "from_version": "0.0.2"})
+				expectMetric(t, metrics[4], 4, map[string]string{"type": "failure", "version": "1.0.0", "image": "test/image:2", "from_version": "0.0.2"})
 				expectMetric(t, metrics[5], 1, map[string]string{"name": "version", "condition": "Failing", "reason": "Because stuff"})
-				expectMetric(t, metrics[6], 6, map[string]string{"type": "current", "version": "0.0.2", "image": "test/image:1", "from_version": ""})
+				expectMetric(t, metrics[6], 6, map[string]string{"type": "current", "version": "0.0.2", "image": "test/image:1", "from_version": "0.0.2"})
 				expectMetric(t, metrics[7], 1, map[string]string{"type": ""})
 			},
 		},
@@ -409,7 +412,7 @@ func Test_operatorMetrics_Collect(t *testing.T) {
 				cmConfigLister: &cmConfigLister{
 					Items: []*corev1.ConfigMap{{
 						ObjectMeta: metav1.ObjectMeta{Name: "openshift-install"},
-						Data: map[string]string {"version": "v0.0.2", "invoker": "jane"},
+						Data:       map[string]string{"version": "v0.0.2", "invoker": "jane"},
 					}},
 				},
 			},