Repository: Azure/ARO-RP
Pull request #1850 (2021-11-22 13:31:51)

Feat: Add Restart Counter Metric for System Components
### Which issue this PR addresses:


Feat: Add Restart Counter Metric for System Components

https://msazure.visualstudio.com/AzureRedHatOpenShift/_workitems/edit/9534801

### What this PR does / why we need it:

This PR adds a logic to report pods/containers that saw more than 10 restarts to Geneva. A Geneva alert will be added to create IcMs when the restart rate exceeds a certain limit.

The reporting threashold of 10 is currently hardcoded.

### Test plan for issue:

Manual Unit Test, testing the the code needs a 'broken' cluster that shows the restart behaviour.

### Is there any documentation that needs to be updated for this PR?

not that I'm aware of



Patch:
diff --git a/pkg/monitor/cluster/podconditions.go b/pkg/monitor/cluster/podconditions.go
index 681219e0b..119db6454 100644
--- a/pkg/monitor/cluster/podconditions.go
+++ b/pkg/monitor/cluster/podconditions.go
@@ -20,6 +20,8 @@ var podConditionsExpected = map[corev1.PodConditionType]corev1.ConditionStatus{
 	corev1.PodReady:        corev1.ConditionTrue,
 }
 
+var restartCounterThreshold int32 = 10
+
 func (mon *Monitor) emitPodConditions(ctx context.Context) error {
 	// to list pods once
 	var cont string
@@ -34,6 +36,7 @@ func (mon *Monitor) emitPodConditions(ctx context.Context) error {
 
 		mon._emitPodConditions(ps)
 		mon._emitPodContainerStatuses(ps)
+		mon._emitPodContainerRestartCounter(ps)
 
 		cont = ps.Continue
 		if cont == "" {
@@ -120,3 +123,34 @@ func (mon *Monitor) _emitPodContainerStatuses(ps *corev1.PodList) {
 		}
 	}
 }
+
+func (mon *Monitor) _emitPodContainerRestartCounter(ps *corev1.PodList) {
+	for _, p := range ps.Items {
+		if !namespace.IsOpenShiftSystemNamespace(p.Namespace) {
+			continue
+		}
+
+		//Sum up the total number of restarts in the pod to match the number of restarts shown in the 'oc get pods' display
+		t := int32(0)
+		for _, cs := range p.Status.ContainerStatuses {
+			t += cs.RestartCount
+		}
+
+		if t < restartCounterThreshold {
+			continue
+		}
+
+		mon.emitGauge("pod.restartcounter", int64(t), map[string]string{
+			"name":      p.Name,
+			"namespace": p.Namespace,
+		})
+
+		if mon.hourlyRun {
+			mon.log.WithFields(logrus.Fields{
+				"metric":    "pod.restartcounter",
+				"name":      p.Name,
+				"namespace": p.Namespace,
+			}).Print()
+		}
+	}
+}
diff --git a/pkg/util/namespace/namespace.go b/pkg/util/namespace/namespace.go
index 6094b38e6..fd7fd785c 100644
--- a/pkg/util/namespace/namespace.go
+++ b/pkg/util/namespace/namespace.go
@@ -7,7 +7,7 @@ import (
 	"strings"
 )
 
-// IsOpenShift returns true if ns is an openshift managed namespace.
+// IsOpenShift returns true if ns is an openshift managed namespace, including the default namespace
 func IsOpenShift(ns string) bool {
 	return ns == "" ||
 		ns == "default" ||
@@ -15,3 +15,11 @@ func IsOpenShift(ns string) bool {
 		strings.HasPrefix(ns, "kube-") ||
 		strings.HasPrefix(ns, "openshift-")
 }
+
+// IsOpenShiftSystemNamespace returns true if ns is an openshift managed namespace, without the default namespace
+func IsOpenShiftSystemNamespace(ns string) bool {
+	return ns == "" ||
+		ns == "openshift" ||
+		strings.HasPrefix(ns, "kube-") ||
+		strings.HasPrefix(ns, "openshift-")
+}


Test patch:
diff --git a/pkg/monitor/cluster/podconditions_test.go b/pkg/monitor/cluster/podconditions_test.go
index 22a0f6122..ceb069968 100644
--- a/pkg/monitor/cluster/podconditions_test.go
+++ b/pkg/monitor/cluster/podconditions_test.go
@@ -8,6 +8,9 @@ import (
 	"testing"
 
 	"github.com/golang/mock/gomock"
+	"github.com/sirupsen/logrus"
+	"github.com/sirupsen/logrus/hooks/test"
+	"github.com/stretchr/testify/assert"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/client-go/kubernetes/fake"
@@ -141,3 +144,162 @@ func TestEmitPodContainerStatuses(t *testing.T) {
 	ps, _ := cli.CoreV1().Pods("").List(context.Background(), metav1.ListOptions{})
 	mon._emitPodContainerStatuses(ps)
 }
+
+func TestEmitPodContainerRestartCounter(t *testing.T) {
+
+	cli := fake.NewSimpleClientset(
+		&corev1.Pod{ // #1 metrics and log entry expected
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "podname1",
+				Namespace: "openshift",
+			},
+			Status: corev1.PodStatus{
+				ContainerStatuses: []corev1.ContainerStatus{
+					{
+						Name:         "containername",
+						RestartCount: 42,
+					},
+				},
+			},
+			Spec: corev1.PodSpec{
+				NodeName: "fake-node-name",
+			},
+		},
+		&corev1.Pod{ // #2 no metrics expected
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "podname2",
+				Namespace: "openshift",
+			},
+			Status: corev1.PodStatus{
+				ContainerStatuses: []corev1.ContainerStatus{
+					{
+						Name:         "containername",
+						RestartCount: restartCounterThreshold - 1,
+					},
+				},
+			},
+			Spec: corev1.PodSpec{
+				NodeName: "fake-node-name",
+			},
+		},
+		&corev1.Pod{ // #3 metrics and log entry expected
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "podname3",
+				Namespace: "openshift",
+			},
+			Status: corev1.PodStatus{
+				ContainerStatuses: []corev1.ContainerStatus{
+					{
+						Name:         "containername",
+						RestartCount: restartCounterThreshold,
+					},
+				},
+			},
+			Spec: corev1.PodSpec{
+				NodeName: "fake-node-name",
+			},
+		},
+		&corev1.Pod{ // #4 no metrics expected
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "podname4",
+				Namespace: "openshift",
+			},
+			Status: corev1.PodStatus{
+				ContainerStatuses: []corev1.ContainerStatus{
+					{
+						Name:         "containername",
+						RestartCount: 0,
+					},
+				},
+			},
+			Spec: corev1.PodSpec{
+				NodeName: "fake-node-name",
+			},
+		},
+		&corev1.Pod{ // #5 no metrics expected
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "not-system-namespace",
+				Namespace: "default",
+			},
+			Status: corev1.PodStatus{
+				ContainerStatuses: []corev1.ContainerStatus{
+					{
+						Name:         "containername",
+						RestartCount: 42,
+					},
+				},
+			},
+			Spec: corev1.PodSpec{
+				NodeName: "fake-node-name",
+			},
+		},
+		&corev1.Pod{ // #6 Multi-container pod
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "multi-container-pod",
+				Namespace: "openshift-test",
+			},
+			Status: corev1.PodStatus{
+				ContainerStatuses: []corev1.ContainerStatus{
+					{
+						Name:         "firstcontainer",
+						RestartCount: restartCounterThreshold,
+					},
+					{
+						Name:         "secondcontainer",
+						RestartCount: restartCounterThreshold,
+					},
+				},
+			},
+			Spec: corev1.PodSpec{
+				NodeName: "fake-node-name",
+			},
+		},
+	)
+
+	controller := gomock.NewController(t)
+	defer controller.Finish()
+
+	m := mock_metrics.NewMockInterface(controller)
+
+	mon := &Monitor{
+		cli:       cli,
+		m:         m,
+		hourlyRun: true,
+	}
+	logger, hook := test.NewNullLogger()
+	log := logrus.NewEntry(logger)
+	mon.log = log
+
+	m.EXPECT().EmitGauge("pod.restartcounter", int64(42), map[string]string{
+		"name":      "podname1",
+		"namespace": "openshift",
+	})
+
+	// Expecting data for 'podname2' to be dropped
+
+	m.EXPECT().EmitGauge("pod.restartcounter", int64(restartCounterThreshold), map[string]string{
+		"name":      "podname3",
+		"namespace": "openshift",
+	})
+
+	// Expecting data for 'podname4' to be dropped
+
+	m.EXPECT().EmitGauge("pod.restartcounter", int64(restartCounterThreshold*2), map[string]string{
+		"name":      "multi-container-pod",
+		"namespace": "openshift-test",
+	})
+
+	ps, _ := cli.CoreV1().Pods("").List(context.Background(), metav1.ListOptions{})
+	mon._emitPodContainerRestartCounter(ps)
+
+	// Matches the number of emitted messages
+	assert.Equal(t, 3, len(hook.Entries))
+
+	// the order of the log entries does not seem to be stable, so testing one entry only
+	// and no test for specific values, except for the metric
+
+	x := hook.LastEntry()
+	assert.NotEmpty(t, x.Data["name"])
+	assert.NotEmpty(t, x.Data["namespace"])
+	assert.Equal(t, "pod.restartcounter", x.Data["metric"])
+}
diff --git a/pkg/util/namespace/namespace_test.go b/pkg/util/namespace/namespace_test.go
index ba8193170..ae2cd2729 100644
--- a/pkg/util/namespace/namespace_test.go
+++ b/pkg/util/namespace/namespace_test.go
@@ -43,3 +43,40 @@ func TestIsOpenShift(t *testing.T) {
 		})
 	}
 }
+
+func TestIsOpenShiftSystemNamespace(t *testing.T) {
+	for _, tt := range []struct {
+		namespace string
+		want      bool
+	}{
+		{
+			want: true,
+		},
+		{
+			namespace: "openshift-ns",
+			want:      true,
+		},
+		{
+			namespace: "openshift",
+			want:      true,
+		},
+		{
+			namespace: "kube-ns",
+			want:      true,
+		},
+		{
+			namespace: "default",
+			want:      false,
+		},
+		{
+			namespace: "customer",
+		},
+	} {
+		t.Run(tt.namespace, func(t *testing.T) {
+			got := IsOpenShiftSystemNamespace(tt.namespace)
+			if tt.want != got {
+				t.Error(got)
+			}
+		})
+	}
+}