feat: create alert rules for constantly syncing GitOps applications

controllers/argocd_metrics_controller.go

@@ -44,9 +44,11 @@ import (
 )
 
 const (
-	readRoleNameFormat         = "%s-read"
-	readRoleBindingNameFormat  = "%s-prometheus-k8s-read-binding"
-	alertRuleName              = "gitops-operator-argocd-alerts"
+	readRoleNameFormat        = "%s-read"
+	readRoleBindingNameFormat = "%s-prometheus-k8s-read-binding"
+	alertRuleName             = "gitops-operator-argocd-alerts"
+	// Use a separate rule so upgrades install the sync-loop alerts.
+	syncLoopAlertRuleName      = "gitops-operator-argocd-sync-loop-alerts"
 	dashboardNamespace         = "openshift-config-managed"
 	dashboardFolder            = "dashboards"
 	operatorMetricsServiceName = "openshift-gitops-operator-metrics-service"
@@ -173,7 +175,7 @@ func (r *ArgoCDMetricsReconciler) Reconcile(ctx context.Context, request reconci
 		}
 
 		// Create alert rule
-		err = r.createPrometheusRuleIfAbsent(request.Namespace, argocd, reqLogger)
+		err = r.createPrometheusRulesIfAbsent(request.Namespace, argocd, reqLogger)
 		if err != nil {
 			return reconcile.Result{}, err
 		}
@@ -238,13 +240,15 @@ func (r *ArgoCDMetricsReconciler) Reconcile(ctx context.Context, request reconci
 				return reconcile.Result{}, err
 			}
 
-			// Delete alert rule
-			err = r.Client.Delete(context.TODO(), &monitoringv1.PrometheusRule{ObjectMeta: metav1.ObjectMeta{Namespace: request.Namespace, Name: alertRuleName}})
-			if err != nil {
-				if !errors.IsNotFound(err) {
-					reqLogger.Error(err, "Error deleting prometheus in ",
-						"Namespace", request.Namespace)
-					return reconcile.Result{}, err
+			// Delete alert rules
+			for _, name := range []string{alertRuleName, syncLoopAlertRuleName} {
+				err = r.Client.Delete(context.TODO(), &monitoringv1.PrometheusRule{ObjectMeta: metav1.ObjectMeta{Namespace: request.Namespace, Name: name}})
+				if err != nil {
+					if !errors.IsNotFound(err) {
+						reqLogger.Error(err, "Error deleting prometheus rule",
+							"Namespace", request.Namespace, "Name", name)
+						return reconcile.Result{}, err
+					}
 				}
 			}
 
@@ -406,38 +410,48 @@ func (r *ArgoCDMetricsReconciler) reconcileOperatorMetricsServiceMonitor(reqLogg
 	return nil
 }
 
-func (r *ArgoCDMetricsReconciler) createPrometheusRuleIfAbsent(namespace string, argocd *argoapp.ArgoCD, reqLogger logr.Logger) error {
-	alertRule := newPrometheusRule(namespace)
+func (r *ArgoCDMetricsReconciler) createPrometheusRulesIfAbsent(namespace string, argocd *argoapp.ArgoCD, reqLogger logr.Logger) error {
+	for _, alertRule := range []*monitoringv1.PrometheusRule{
+		newPrometheusRule(namespace),
+		newSyncLoopPrometheusRule(namespace),
+	} {
+		if err := r.createPrometheusRuleIfAbsent(alertRule, argocd, reqLogger); err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
+func (r *ArgoCDMetricsReconciler) createPrometheusRuleIfAbsent(alertRule *monitoringv1.PrometheusRule, argocd *argoapp.ArgoCD, reqLogger logr.Logger) error {
 	existingAlertRule := &monitoringv1.PrometheusRule{}
 	err := r.Client.Get(context.TODO(), types.NamespacedName{Name: alertRule.Name, Namespace: alertRule.Namespace}, existingAlertRule)
 	if err == nil {
 		reqLogger.Info("An alert rule instance already exists",
 			"Namespace", existingAlertRule.Namespace, "Name", existingAlertRule.Name)
 		return nil
 	}
-	if errors.IsNotFound(err) {
-		reqLogger.Info("Creating new alert rule",
+	if !errors.IsNotFound(err) {
+		reqLogger.Error(err, "Error querying for existing alert rule",
 			"Namespace", alertRule.Namespace, "Name", alertRule.Name)
+		return err
+	}
 
-		// Set the ArgoCD instance as the owner and controller
-		if err := controllerutil.SetControllerReference(argocd, alertRule, r.Scheme); err != nil {
-			reqLogger.Error(err, "Error setting read role owner ref",
-				"Namespace", alertRule.Namespace, "Name", alertRule.Name, "ArgoCD Name", argocd.Name)
-			return err
-		}
+	reqLogger.Info("Creating new alert rule",
+		"Namespace", alertRule.Namespace, "Name", alertRule.Name)
 
-		err := r.Client.Create(context.TODO(), alertRule)
-		if err != nil {
-			reqLogger.Error(err, "Error creating a new alert rule",
-				"Namespace", alertRule.Namespace, "Name", alertRule.Name)
-			return err
-		}
+	if err := controllerutil.SetControllerReference(argocd, alertRule, r.Scheme); err != nil {
+		reqLogger.Error(err, "Error setting alert rule owner ref",
+			"Namespace", alertRule.Namespace, "Name", alertRule.Name, "ArgoCD Name", argocd.Name)
+		return err
+	}
 
-		return nil
+	if err := r.Client.Create(context.TODO(), alertRule); err != nil {
+		reqLogger.Error(err, "Error creating a new alert rule",
+			"Namespace", alertRule.Namespace, "Name", alertRule.Name)
+		return err
 	}
-	reqLogger.Error(err, "Error querying for existing alert rule",
-		"Namespace", namespace, "Name", alertRuleName)
-	return err
+
+	return nil
 }
 
 func (r *ArgoCDMetricsReconciler) reconcileDashboards(reqLogger logr.Logger) error {
@@ -635,3 +649,56 @@ func newPrometheusRule(namespace string) *monitoringv1.PrometheusRule {
 		Spec:       spec,
 	}
 }
+
+// newSyncLoopPrometheusRule defines alerts for sustained application sync rates.
+func newSyncLoopPrometheusRule(namespace string) *monitoringv1.PrometheusRule {
+	return &monitoringv1.PrometheusRule{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      syncLoopAlertRuleName,
+			Namespace: namespace,
+		},
+		Spec: monitoringv1.PrometheusRuleSpec{
+			Groups: []monitoringv1.RuleGroup{
+				{
+					Name: "GitOpsOperatorArgoCDSyncLoop",
+					Rules: []monitoringv1.Rule{
+						newRecordingRule("gitops:argocd_app_sync:rate10m",
+							fmt.Sprintf(`sum by (name, namespace) (rate(argocd_app_sync_total{namespace="%s"}[10m]))`, namespace)),
+						newRecordingRule("gitops:argocd_app_sync_failed:rate10m",
+							fmt.Sprintf(`sum by (name, namespace) (rate(argocd_app_sync_total{namespace="%s",phase=~"Error|Failed"}[10m]))`, namespace)),
+						newAlertRule("ArgoCDAppSyncLoop", "warning", "20m",
+							fmt.Sprintf(`gitops:argocd_app_sync:rate10m{namespace="%s"} > 0.01`, namespace),
+							"Argo CD application is syncing continuously",
+							"Argo CD application {{ $labels.name }} in namespace {{ $labels.namespace }} has a sustained sync rate above 0.01/s (about one sync every ~100s) for 20m. This often indicates a selfHeal conflict (for example HPA fighting declared replicas). Check application sync history, diff, and conflicting controllers."),
+						newAlertRule("ArgoCDAppSyncFailureLoop", "warning", "15m",
+							fmt.Sprintf(`gitops:argocd_app_sync_failed:rate10m{namespace="%s"} > 0.005`, namespace),
+							"Argo CD application syncs are failing repeatedly",
+							"Argo CD application {{ $labels.name }} in namespace {{ $labels.namespace }} has a sustained failed sync rate above 0.005/s for 15m. Investigate the application operation status and sync errors."),
+					},
+				},
+			},
+		},
+	}
+}
+
+func newRecordingRule(name, expr string) monitoringv1.Rule {
+	return monitoringv1.Rule{
+		Record: name,
+		Expr:   intstr.FromString(expr),
+	}
+}
+
+func newAlertRule(name, severity, duration, expr, summary, description string) monitoringv1.Rule {
+	return monitoringv1.Rule{
+		Alert: name,
+		Annotations: map[string]string{
+			"summary":     summary,
+			"description": description,
+		},
+		Expr: intstr.FromString(expr),
+		For:  ptr.To(monitoringv1.Duration(duration)),
+		Labels: map[string]string{
+			"severity": severity,
+		},
+	}
+}

controllers/argocd_metrics_controller_test.go

@@ -29,6 +29,7 @@ import (
 	is "gotest.tools/assert/cmp"
 	corev1 "k8s.io/api/core/v1"
 	rbacv1 "k8s.io/api/rbac/v1"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
@@ -340,6 +341,107 @@ func TestReconciler_add_prometheus_rule(t *testing.T) {
 	}
 }
 
+func TestReconciler_add_sync_loop_prometheus_rule(t *testing.T) {
+	testCases := []struct {
+		instanceName string
+		namespace    string
+	}{
+		{
+			instanceName: argoCDInstanceName,
+			namespace:    "openshift-gitops",
+		},
+		{
+			instanceName: "instance-two",
+			namespace:    "namespace-two",
+		},
+	}
+	flagPtr := false
+	for _, tc := range testCases {
+		r := newMetricsReconciler(t, tc.namespace, tc.instanceName, &flagPtr)
+		_, err := r.Reconcile(context.TODO(), newRequest(tc.namespace, tc.instanceName))
+		assert.NilError(t, err)
+
+		rule := monitoringv1.PrometheusRule{}
+		err = r.Client.Get(context.TODO(), types.NamespacedName{Name: syncLoopAlertRuleName, Namespace: tc.namespace}, &rule)
+		assert.NilError(t, err)
+
+		assert.Assert(t, is.Len(rule.OwnerReferences, 1))
+		assert.Equal(t, rule.OwnerReferences[0].Kind, argocdKind)
+		assert.Equal(t, rule.OwnerReferences[0].Name, tc.instanceName)
+
+		assert.Equal(t, rule.Spec.Groups[0].Name, "GitOpsOperatorArgoCDSyncLoop")
+		assert.Assert(t, is.Len(rule.Spec.Groups[0].Rules, 4))
+
+		recordRule := rule.Spec.Groups[0].Rules[0]
+		assert.Equal(t, recordRule.Record, "gitops:argocd_app_sync:rate10m")
+		assert.Equal(t, recordRule.Expr.StrVal,
+			fmt.Sprintf(`sum by (name, namespace) (rate(argocd_app_sync_total{namespace="%s"}[10m]))`, tc.namespace))
+
+		failureRecordRule := rule.Spec.Groups[0].Rules[1]
+		assert.Equal(t, failureRecordRule.Record, "gitops:argocd_app_sync_failed:rate10m")
+		assert.Equal(t, failureRecordRule.Expr.StrVal,
+			fmt.Sprintf(`sum by (name, namespace) (rate(argocd_app_sync_total{namespace="%s",phase=~"Error|Failed"}[10m]))`, tc.namespace))
+
+		loop := rule.Spec.Groups[0].Rules[2]
+		assert.Equal(t, loop.Alert, "ArgoCDAppSyncLoop")
+		assert.Equal(t, string(*loop.For), "20m")
+		assert.Equal(t, loop.Labels["severity"], "warning")
+		assert.Equal(t, loop.Expr.StrVal,
+			fmt.Sprintf(`gitops:argocd_app_sync:rate10m{namespace="%s"} > 0.01`, tc.namespace))
+		assert.Assert(t, loop.Annotations["summary"] != "")
+		assert.Assert(t, loop.Annotations["description"] != "")
+
+		failureLoop := rule.Spec.Groups[0].Rules[3]
+		assert.Equal(t, failureLoop.Alert, "ArgoCDAppSyncFailureLoop")
+		assert.Equal(t, string(*failureLoop.For), "15m")
+		assert.Equal(t, failureLoop.Labels["severity"], "warning")
+		assert.Equal(t, failureLoop.Expr.StrVal,
+			fmt.Sprintf(`gitops:argocd_app_sync_failed:rate10m{namespace="%s"} > 0.005`, tc.namespace))
+		assert.Assert(t, failureLoop.Annotations["summary"] != "")
+		assert.Assert(t, failureLoop.Annotations["description"] != "")
+	}
+}
+
+func TestReconcile_remove_prometheus_rules(t *testing.T) {
+	testCases := []struct {
+		instanceName string
+		namespace    string
+	}{
+		{
+			instanceName: argoCDInstanceName,
+			namespace:    "openshift-gitops",
+		},
+		{
+			instanceName: "instance-two",
+			namespace:    "namespace-two",
+		},
+	}
+
+	for _, tc := range testCases {
+		r := newMetricsReconciler(t, tc.namespace, tc.instanceName, new(false))
+		request := newRequest(tc.namespace, tc.instanceName)
+
+		_, err := r.Reconcile(context.TODO(), request)
+		assert.NilError(t, err)
+
+		argocd := &argoapp.ArgoCD{}
+		err = r.Client.Get(context.TODO(), request.NamespacedName, argocd)
+		assert.NilError(t, err)
+		argocd.Spec.Monitoring.DisableMetrics = new(true)
+		err = r.Client.Update(context.TODO(), argocd)
+		assert.NilError(t, err)
+
+		_, err = r.Reconcile(context.TODO(), request)
+		assert.NilError(t, err)
+
+		for _, name := range []string{alertRuleName, syncLoopAlertRuleName} {
+			rule := &monitoringv1.PrometheusRule{}
+			err = r.Client.Get(context.TODO(), types.NamespacedName{Name: name, Namespace: tc.namespace}, rule)
+			assert.Assert(t, apierrors.IsNotFound(err))
+		}
+	}
+}
+
 func TestReconciler_add_dashboard(t *testing.T) {
 
 	// Need to create openshift-config-managed namespace for dashboards