Repository: metallb/metallb
Pull request #1749 (2022-12-19 17:07:17)

FRR liveness probe: move the execution inside the container
Using an exec probe means not binding the resource consumption of the
execution of the probe to the container, but to the container runtime.

By moving the execution of the vtysh behind the http endpoint, we ensure
that all the cpu quotas are respected because the actual vtysh execution
happens in the context of the container.

fedepaol: > lgtm (side note: we might want to use this `Run` in the collectors as well)

Right, I forgot :-)
I will use the vtysh package on metrics too
liornoy: lgtm

Patch:
diff --git a/charts/metallb/README.md b/charts/metallb/README.md
index 35cbc934..db14b58a 100644
--- a/charts/metallb/README.md
+++ b/charts/metallb/README.md
@@ -143,6 +143,9 @@ Kubernetes: `>= 1.19.0-0`
 | speaker.serviceAccount.annotations | object | `{}` |  |
 | speaker.serviceAccount.create | bool | `true` |  |
 | speaker.serviceAccount.name | string | `""` |  |
+| speaker.startupProbe.enabled | bool | `true` |  |
+| speaker.startupProbe.failureThreshold | int | `30` |  |
+| speaker.startupProbe.periodSeconds | int | `5` |  |
 | speaker.tolerateMaster | bool | `true` |  |
 | speaker.tolerations | list | `[]` |  |
 | speaker.updateStrategy.type | string | `"RollingUpdate"` |  |
diff --git a/charts/metallb/templates/speaker.yaml b/charts/metallb/templates/speaker.yaml
index b671a66f..5da5a41f 100644
--- a/charts/metallb/templates/speaker.yaml
+++ b/charts/metallb/templates/speaker.yaml
@@ -165,8 +165,6 @@ spec:
           emptyDir: {}
         - name: metrics
           emptyDir: {}
-        - name: frr-liveness
-          emptyDir: {}
       {{- if .Values.prometheus.speakerMetricsTLSSecret }}
         - name: metrics-certs
           secret:
@@ -192,13 +190,6 @@ spec:
           volumeMounts:
             - name: reloader
               mountPath: /etc/frr_reloader
-        # Copies the liveness probe script to the shared volume between the speaker and reloader.
-        - name: cp-liveness
-          image: {{ .Values.speaker.image.repository }}:{{ .Values.speaker.image.tag | default .Chart.AppVersion }}
-          command: ["/bin/sh", "-c", "cp -f /liveness.sh /etc/frr_liveness/"]
-          volumeMounts:
-            - name: frr-liveness
-              mountPath: /etc/frr_liveness
         # Copies the metrics exporter
         - name: cp-metrics
           image: {{ .Values.speaker.image.repository }}:{{ .Values.speaker.image.tag | default .Chart.AppVersion }}
@@ -332,8 +323,6 @@ spec:
             mountPath: /var/run/frr
           - name: frr-conf
             mountPath: /etc/frr
-          - name: frr-liveness
-            mountPath: /etc/frr_liveness
         # The command is FRR's default entrypoint & waiting for the log file to appear and tailing it.
         # If the log file isn't created in 60 seconds the tail fails and the container is restarted.
         # This workaround is needed to have the frr logs as part of kubectl logs -c frr < speaker_pod_name >.
@@ -352,16 +341,22 @@ spec:
         resources:
           {{- toYaml . | nindent 12 }}
         {{- end }}
+        {{- if .Values.speaker.livenessProbe.enabled }}
         livenessProbe:
-          exec:
-            command: ["/etc/frr_liveness/liveness.sh"]
-          periodSeconds: 5
-          failureThreshold: 3
+          httpGet:
+            path: /livez
+            port: {{ .Values.speaker.frr.metricsPort }}
+          periodSeconds: {{ .Values.speaker.livenessProbe.periodSeconds }}
+          failureThreshold: {{ .Values.speaker.livenessProbe.failureThreshold }}
+        {{- end }}
+        {{- if .Values.speaker.startupProbe.enabled }}
         startupProbe:
-          exec:
-            command: ["/etc/frr_liveness/liveness.sh"]
-          failureThreshold: 30
-          periodSeconds: 5
+          httpGet:
+            path: /livez
+            port: {{ .Values.speaker.frr.metricsPort }}
+          failureThreshold: {{ .Values.speaker.startupProbe.failureThreshold }}
+          periodSeconds: {{ .Values.speaker.startupProbe.periodSeconds }}
+        {{- end }}
       - name: reloader
         image: {{ .Values.speaker.frr.image.repository }}:{{ .Values.speaker.frr.image.tag | default .Chart.AppVersion }}
         {{- if .Values.speaker.frr.image.pullPolicy }}
diff --git a/charts/metallb/values.yaml b/charts/metallb/values.yaml
index 2982085a..4f62c74a 100644
--- a/charts/metallb/values.yaml
+++ b/charts/metallb/values.yaml
@@ -307,6 +307,10 @@ speaker:
     periodSeconds: 10
     successThreshold: 1
     timeoutSeconds: 1
+  startupProbe:
+    enabled: true
+    failureThreshold: 30
+    periodSeconds: 5
   # frr contains configuration specific to the MetalLB FRR container,
   # for speaker running alongside FRR.
   frr:
diff --git a/config/frr/speaker-patch.yaml b/config/frr/speaker-patch.yaml
index 6914122a..bd211df9 100644
--- a/config/frr/speaker-patch.yaml
+++ b/config/frr/speaker-patch.yaml
@@ -23,8 +23,6 @@ spec:
           emptyDir: {}
         - name: metrics
           emptyDir: {}
-        - name: frr-liveness
-          emptyDir: {}
       initContainers:
         # Copies the initial config files with the right permissions to the shared volume.
         - name: cp-frr-files
@@ -45,13 +43,6 @@ spec:
           volumeMounts:
             - name: reloader
               mountPath: /etc/frr_reloader
-        # Copies the liveness probe script to the shared volume between the speaker and reloader.
-        - name: cp-liveness
-          image: quay.io/metallb/speaker:main
-          command: ["/bin/sh", "-c", "cp -f /liveness.sh /etc/frr_liveness/"]
-          volumeMounts:
-            - name: frr-liveness
-              mountPath: /etc/frr_liveness
         # Copies the metrics exporter
         - name: cp-metrics
           image: quay.io/metallb/speaker:main
@@ -73,8 +64,6 @@ spec:
               mountPath: /var/run/frr
             - name: frr-conf
               mountPath: /etc/frr
-            - name: frr-liveness
-              mountPath: /etc/frr_liveness
           # The command is FRR's default entrypoint & waiting for the log file to appear and tailing it.
           # If the log file isn't created in 60 seconds the tail fails and the container is restarted.
           # This workaround is needed to have the frr logs as part of kubectl logs -c frr < speaker_pod_name >.
@@ -90,13 +79,15 @@ spec:
               done
               tail -f /etc/frr/frr.log
           livenessProbe:
-            exec:
-              command: ["/etc/frr_liveness/liveness.sh"]
+            httpGet:
+              path: /livez
+              port: 7473
             periodSeconds: 5
             failureThreshold: 3
           startupProbe:
-            exec:
-              command: ["/etc/frr_liveness/liveness.sh"]
+            httpGet:
+              path: /livez
+              port: 7473
             failureThreshold: 30
             periodSeconds: 5
         - name: reloader
diff --git a/config/manifests/metallb-frr-prometheus.yaml b/config/manifests/metallb-frr-prometheus.yaml
index e2d34d60..098cc1e0 100644
--- a/config/manifests/metallb-frr-prometheus.yaml
+++ b/config/manifests/metallb-frr-prometheus.yaml
@@ -2107,10 +2107,10 @@ spec:
           value: "true"
         image: frrouting/frr:v7.5.1
         livenessProbe:
-          exec:
-            command:
-            - /etc/frr_liveness/liveness.sh
           failureThreshold: 3
+          httpGet:
+            path: /livez
+            port: 7473
           periodSeconds: 5
         name: frr
         securityContext:
@@ -2121,18 +2121,16 @@ spec:
             - SYS_ADMIN
             - NET_BIND_SERVICE
         startupProbe:
-          exec:
-            command:
-            - /etc/frr_liveness/liveness.sh
           failureThreshold: 30
+          httpGet:
+            path: /livez
+            port: 7473
           periodSeconds: 5
         volumeMounts:
         - mountPath: /var/run/frr
           name: frr-sockets
         - mountPath: /etc/frr
           name: frr-conf
-        - mountPath: /etc/frr_liveness
-          name: frr-liveness
       - command:
         - /etc/frr_reloader/frr-reloader.sh
         image: frrouting/frr:v7.5.1
@@ -2253,15 +2251,6 @@ spec:
         volumeMounts:
         - mountPath: /etc/frr_reloader
           name: reloader
-      - command:
-        - /bin/sh
-        - -c
-        - cp -f /liveness.sh /etc/frr_liveness/
-        image: quay.io/metallb/speaker:main
-        name: cp-liveness
-        volumeMounts:
-        - mountPath: /etc/frr_liveness
-          name: frr-liveness
       - command:
         - /bin/sh
         - -c
@@ -2295,8 +2284,6 @@ spec:
         name: reloader
       - emptyDir: {}
         name: metrics
-      - emptyDir: {}
-        name: frr-liveness
       - name: memberlist
         secret:
           defaultMode: 420
diff --git a/config/manifests/metallb-frr.yaml b/config/manifests/metallb-frr.yaml
index b59ba683..b7cb7245 100644
--- a/config/manifests/metallb-frr.yaml
+++ b/config/manifests/metallb-frr.yaml
@@ -1930,10 +1930,10 @@ spec:
           value: "true"
         image: frrouting/frr:v7.5.1
         livenessProbe:
-          exec:
-            command:
-            - /etc/frr_liveness/liveness.sh
           failureThreshold: 3
+          httpGet:
+            path: /livez
+            port: 7473
           periodSeconds: 5
         name: frr
         securityContext:
@@ -1944,18 +1944,16 @@ spec:
             - SYS_ADMIN
             - NET_BIND_SERVICE
         startupProbe:
-          exec:
-            command:
-            - /etc/frr_liveness/liveness.sh
           failureThreshold: 30
+          httpGet:
+            path: /livez
+            port: 7473
           periodSeconds: 5
         volumeMounts:
         - mountPath: /var/run/frr
           name: frr-sockets
         - mountPath: /etc/frr
           name: frr-conf
-        - mountPath: /etc/frr_liveness
-          name: frr-liveness
       - command:
         - /etc/frr_reloader/frr-reloader.sh
         image: frrouting/frr:v7.5.1
@@ -2076,15 +2074,6 @@ spec:
         volumeMounts:
         - mountPath: /etc/frr_reloader
           name: reloader
-      - command:
-        - /bin/sh
-        - -c
-        - cp -f /liveness.sh /etc/frr_liveness/
-        image: quay.io/metallb/speaker:main
-        name: cp-liveness
-        volumeMounts:
-        - mountPath: /etc/frr_liveness
-          name: frr-liveness
       - command:
         - /bin/sh
         - -c
@@ -2118,8 +2107,6 @@ spec:
         name: reloader
       - emptyDir: {}
         name: metrics
-      - emptyDir: {}
-        name: frr-liveness
       - name: memberlist
         secret:
           defaultMode: 420
diff --git a/frr-tools/liveness/liveness.sh b/frr-tools/liveness/liveness.sh
deleted file mode 100755
index 235a2ed5..00000000
--- a/frr-tools/liveness/liveness.sh
+++ /dev/null
@@ -1,12 +0,0 @@
-#!/bin/bash
-set -e
-
-EXPECTED_DAEMONS=" bfdd bgpd staticd watchfrr zebra "
-DAEMONS=$(vtysh -c "show daemons" | tr " " "\n" | sort | tr "\n" " ")
-
-if [ "$DAEMONS" != "$EXPECTED_DAEMONS" ]; then
-        echo "Did not find all the expected daemons [$DAEMONS]"
-        exit 1
-fi
-
-
diff --git a/frr-tools/metrics/collector/bfd.go b/frr-tools/metrics/collector/bfd.go
index 891b9dc0..58ed5605 100644
--- a/frr-tools/metrics/collector/bfd.go
+++ b/frr-tools/metrics/collector/bfd.go
@@ -9,6 +9,7 @@ import (
 	"github.com/go-kit/log/level"
 	"github.com/prometheus/client_golang/prometheus"
 
+	"go.universe.tf/metallb/frr-tools/metrics/vtysh"
 	bgpfrr "go.universe.tf/metallb/internal/bgp/frr"
 	bgpstats "go.universe.tf/metallb/internal/bgp/native"
 )
@@ -86,12 +87,12 @@ var (
 
 type bfd struct {
 	Log    log.Logger
-	frrCli func(args string) (string, error)
+	frrCli vtysh.Cli
 }
 
 func NewBFD(l log.Logger) *bfd {
 	log := log.With(l, "collector", subsystem)
-	return &bfd{Log: log, frrCli: runVtysh}
+	return &bfd{Log: log, frrCli: vtysh.Run}
 }
 
 func (c *bfd) Describe(ch chan<- *prometheus.Desc) {
@@ -146,7 +147,7 @@ func updatePeersCountersMetrics(ch chan<- prometheus.Metric, peersCounters []bfd
 	}
 }
 
-func getBFDPeers(frrCli func(args string) (string, error)) (map[string]bgpfrr.BFDPeer, error) {
+func getBFDPeers(frrCli vtysh.Cli) (map[string]bgpfrr.BFDPeer, error) {
 	res, err := frrCli("show bfd peers json")
 	if err != nil {
 		return nil, err
@@ -155,7 +156,7 @@ func getBFDPeers(frrCli func(args string) (string, error)) (map[string]bgpfrr.BF
 	return bgpfrr.ParseBFDPeers(res)
 }
 
-func getBFDPeersCounters(frrCli func(args string) (string, error)) ([]bfdPeerCounters, error) {
+func getBFDPeersCounters(frrCli vtysh.Cli) ([]bfdPeerCounters, error) {
 	res, err := frrCli("show bfd peers counters json")
 	if err != nil {
 		return nil, err
diff --git a/frr-tools/metrics/collector/bgp.go b/frr-tools/metrics/collector/bgp.go
index 53849f89..0fe3b21d 100644
--- a/frr-tools/metrics/collector/bgp.go
+++ b/frr-tools/metrics/collector/bgp.go
@@ -9,6 +9,7 @@ import (
 	"github.com/go-kit/log/level"
 	"github.com/prometheus/client_golang/prometheus"
 
+	"go.universe.tf/metallb/frr-tools/metrics/vtysh"
 	bgpfrr "go.universe.tf/metallb/internal/bgp/frr"
 	bgpstats "go.universe.tf/metallb/internal/bgp/native"
 )
@@ -101,12 +102,12 @@ var (
 
 type bgp struct {
 	Log    log.Logger
-	frrCli func(args string) (string, error)
+	frrCli vtysh.Cli
 }
 
 func NewBGP(l log.Logger) *bgp {
 	log := log.With(l, "collector", bgpstats.Subsystem)
-	return &bgp{Log: log, frrCli: runVtysh}
+	return &bgp{Log: log, frrCli: vtysh.Run}
 }
 
 func (c *bgp) Describe(ch chan<- *prometheus.Desc) {
@@ -157,7 +158,7 @@ func updateNeighborsMetrics(ch chan<- prometheus.Metric, neighbors []*bgpfrr.Nei
 	}
 }
 
-func getBGPNeighbors(frrCli func(args string) (string, error)) ([]*bgpfrr.Neighbor, error) {
+func getBGPNeighbors(frrCli vtysh.Cli) ([]*bgpfrr.Neighbor, error) {
 	res, err := frrCli("show bgp neighbors json")
 	if err != nil {
 		return nil, err
diff --git a/frr-tools/metrics/exporter.go b/frr-tools/metrics/exporter.go
index dc452aab..6668683a 100644
--- a/frr-tools/metrics/exporter.go
+++ b/frr-tools/metrics/exporter.go
@@ -17,6 +17,8 @@ import (
 	"github.com/prometheus/exporter-toolkit/web"
 
 	"go.universe.tf/metallb/frr-tools/metrics/collector"
+	"go.universe.tf/metallb/frr-tools/metrics/liveness"
+	"go.universe.tf/metallb/frr-tools/metrics/vtysh"
 	"go.universe.tf/metallb/internal/logging"
 	"go.universe.tf/metallb/internal/version"
 )
@@ -60,10 +62,13 @@ func main() {
 
 	level.Info(logger).Log("version", version.Version(), "commit", version.CommitHash(), "branch", version.Branch(), "goversion", version.GoString(), "msg", "FRR metrics exporter starting "+version.String())
 
-	http.Handle(*metricsPath, metricsHandler(logger))
+	mux := http.NewServeMux()
+	mux.Handle(*metricsPath, metricsHandler(logger))
+	mux.Handle("/livez", liveness.Handler(vtysh.Run, logger))
 	srv := &http.Server{
 		Addr:        fmt.Sprintf(":%d", *metricsPort),
 		ReadTimeout: 3 * time.Second,
+		Handler:     mux,
 	}
 	level.Info(logger).Log("msg", "Starting exporter", "metricsPath", metricsPath, "port", metricsPort)
 
diff --git a/frr-tools/metrics/liveness/liveness.go b/frr-tools/metrics/liveness/liveness.go
new file mode 100644
index 00000000..7f9169a8
--- /dev/null
+++ b/frr-tools/metrics/liveness/liveness.go
@@ -0,0 +1,39 @@
+// SPDX-License-Identifier:Apache-2.0
+
+package liveness
+
+import (
+	"net/http"
+	"strings"
+
+	"github.com/go-kit/log"
+
+	"go.universe.tf/metallb/frr-tools/metrics/vtysh"
+)
+
+func Handler(frrCli vtysh.Cli, logger log.Logger) http.Handler {
+	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		res, err := frrCli("show daemons")
+		if err != nil {
+			http.Error(w, "failed to call show daemons", http.StatusInternalServerError)
+			logger.Log("failed to call show daemons", err)
+			return
+		}
+		expected := map[string]struct{}{
+			"bfdd":     {},
+			"bgpd":     {},
+			"staticd":  {},
+			"watchfrr": {},
+			"zebra":    {},
+		}
+
+		runningDaemons := strings.Split(strings.TrimSuffix(res, "\n"), " ")
+		for _, d := range runningDaemons {
+			delete(expected, d)
+		}
+		if len(expected) > 0 {
+			logger.Log("daemons not running. got: ", res, "missing: ", res)
+			http.NotFound(w, r)
+		}
+	})
+}
diff --git a/frr-tools/metrics/collector/vtysh.go b/frr-tools/metrics/vtysh/vtysh.go
similarity index 58%
rename from frr-tools/metrics/collector/vtysh.go
rename to frr-tools/metrics/vtysh/vtysh.go
index d4517a9b..46175067 100644
--- a/frr-tools/metrics/collector/vtysh.go
+++ b/frr-tools/metrics/vtysh/vtysh.go
@@ -1,12 +1,16 @@
 // SPDX-License-Identifier:Apache-2.0
 
-package collector
+package vtysh
 
 import (
 	"os/exec"
 )
 
-func runVtysh(args string) (string, error) {
+type Cli func(args string) (string, error)
+
+func Run(args string) (string, error) {
 	out, err := exec.Command("/usr/bin/vtysh", "-c", args).CombinedOutput()
 	return string(out), err
 }
+
+var _ Cli = Run
diff --git a/speaker/Dockerfile b/speaker/Dockerfile
index 3e212d80..6bd02eca 100644
--- a/speaker/Dockerfile
+++ b/speaker/Dockerfile
@@ -50,7 +50,6 @@ FROM docker.io/alpine:latest
 COPY --from=builder /build/speaker /speaker
 COPY --from=builder /build/frr-metrics /frr-metrics
 COPY frr-tools/reloader/frr-reloader.sh /frr-reloader.sh
-COPY frr-tools/liveness/liveness.sh /liveness.sh
 COPY LICENSE /
 
 LABEL org.opencontainers.image.authors="metallb" \


Test patch:
diff --git a/frr-tools/metrics/liveness/liveness_test.go b/frr-tools/metrics/liveness/liveness_test.go
new file mode 100644
index 00000000..8d87b631
--- /dev/null
+++ b/frr-tools/metrics/liveness/liveness_test.go
@@ -0,0 +1,60 @@
+// SPDX-License-Identifier:Apache-2.0
+
+package liveness
+
+import (
+	"fmt"
+	"net/http"
+	"net/http/httptest"
+	"testing"
+
+	"go.universe.tf/metallb/internal/logging"
+)
+
+func TestLiveness(t *testing.T) {
+	tests := []struct {
+		desc               string
+		vtyshRes           string
+		vtyshError         error
+		expectedStatusCode int
+	}{
+		{
+			desc:               "regular",
+			vtyshRes:           " zebra bgpd watchfrr staticd bfdd\n",
+			expectedStatusCode: http.StatusOK,
+		},
+		{
+			desc:               "returns error",
+			vtyshError:         fmt.Errorf("failed to run"),
+			expectedStatusCode: http.StatusInternalServerError,
+		},
+		{
+			desc:               "less daemons",
+			vtyshRes:           " zebra bgpd staticd bfdd\n",
+			expectedStatusCode: http.StatusNotFound,
+		},
+	}
+
+	logger, err := logging.Init("error")
+	if err != nil {
+		t.Fatalf("failed to create logger %v", err)
+	}
+	req := httptest.NewRequest(http.MethodGet, "/livez", nil)
+	for _, test := range tests {
+		t.Run(test.desc, func(t *testing.T) {
+			w := httptest.NewRecorder()
+			vtysh := func(args string) (string, error) {
+				return test.vtyshRes, test.vtyshError
+			}
+			handler := Handler(vtysh, logger)
+			handler.ServeHTTP(w, req)
+			res := w.Result()
+			defer res.Body.Close()
+			if res.StatusCode != test.expectedStatusCode {
+				t.Errorf("status code %d different from expected %d", res.StatusCode, test.expectedStatusCode)
+			}
+
+		})
+	}
+
+}