Repository: kinvolk/lokomotive
Pull request #927 (2020-09-09 11:07:39)

MetalLB: Fix regressions of tolerations and nodeSelectors
- Remove the tolerations.
- Remove the nodeSelector from yaml and move the golang templated block down.

johananl: I've tested the following manually:

- Speaker node selector is set to match a label which exists only on one node. A speaker is scheduled only on that node as expected.
- A node is tainted. No tolerations are configured. A speaker isn't scheduled on the tainted node as expected.
- A speaker toleration is added. A speaker is now scheduled on the tainted node as expected.
niels-s: I'm not familiar with the testing strategy for the lokomotive repo, but will you also provide a regression test to check the custom tolerations/node selectors are added, just to prevent this from happening in the future?

Patch:
diff --git a/pkg/components/metallb/manifests.go b/pkg/components/metallb/manifests.go
index 103cc442..941eb19e 100644
--- a/pkg/components/metallb/manifests.go
+++ b/pkg/components/metallb/manifests.go
@@ -248,12 +248,6 @@ spec:
         app: metallb
         component: controller
     spec:
-      {{- if .ControllerNodeSelectors }}
-      nodeSelector:
-        {{- range $key, $value := .ControllerNodeSelectors }}
-        {{ $key }}: "{{ $value }}"
-        {{- end }}
-      {{- end }}
       containers:
       - args:
         - --port=7472
@@ -274,13 +268,19 @@ spec:
             drop:
             - all
           readOnlyRootFilesystem: true
+      # XXX: Lokomotive specific change.
+      {{- if .ControllerNodeSelectors }}
       nodeSelector:
-        beta.kubernetes.io/os: linux
+        {{- range $key, $value := .ControllerNodeSelectors }}
+        {{ $key }}: "{{ $value }}"
+        {{- end }}
+      {{- end }}
       securityContext:
         runAsNonRoot: true
         runAsUser: 65534
       serviceAccountName: controller
       terminationGracePeriodSeconds: 0
+      # XXX: Lokomotive specific change.
       {{- if .ControllerTolerationsJSON }}
       tolerations: {{ .ControllerTolerationsJSON }}
       {{- end }}
@@ -312,12 +312,6 @@ spec:
         app: metallb
         component: speaker
     spec:
-      {{- if .SpeakerNodeSelectors }}
-      nodeSelector:
-        {{- range $key, $value := .SpeakerNodeSelectors }}
-        {{ $key }}: "{{ $value }}"
-        {{- end }}
-      {{- end }}
       containers:
       - args:
         - --metrics-port=7472
@@ -371,13 +365,19 @@ spec:
             - ALL
           readOnlyRootFilesystem: true
       hostNetwork: true
+      # XXX: Lokomotive specific change.
+      {{- if .SpeakerNodeSelectors }}
       nodeSelector:
-        beta.kubernetes.io/os: linux
+        {{- range $key, $value := .SpeakerNodeSelectors }}
+        {{ $key }}: "{{ $value }}"
+        {{- end }}
+      {{- end }}
       serviceAccountName: speaker
       terminationGracePeriodSeconds: 2
-      tolerations:
-      - effect: NoSchedule
-        key: node-role.kubernetes.io/master
+      # XXX: Lokomotive specific change.
+      {{- if .SpeakerTolerationsJSON }}
+      tolerations: {{ .SpeakerTolerationsJSON }}
+      {{- end }}
 `
 
 const pspMetallbController = `


Test patch:
diff --git a/pkg/components/metallb/component_test.go b/pkg/components/metallb/component_test.go
index a13738fb..c2175398 100644
--- a/pkg/components/metallb/component_test.go
+++ b/pkg/components/metallb/component_test.go
@@ -15,9 +15,13 @@
 package metallb
 
 import (
+	"reflect"
 	"testing"
 
 	"github.com/hashicorp/hcl/v2"
+	appsv1 "k8s.io/api/apps/v1"
+	corev1 "k8s.io/api/core/v1"
+	"sigs.k8s.io/yaml"
 
 	"github.com/kinvolk/lokomotive/pkg/components/util"
 )
@@ -32,7 +36,7 @@ func TestEmptyConfig(t *testing.T) {
 	}
 }
 
-func testRenderManifest(t *testing.T, configHCL string) {
+func renderManifest(t *testing.T, configHCL string) map[string]string {
 	component := newComponent()
 
 	body, diagnostics := util.GetComponentBody(configHCL, name)
@@ -45,11 +49,17 @@ func testRenderManifest(t *testing.T, configHCL string) {
 		t.Fatalf("Valid config should not return error, got: %s", diagnostics)
 	}
 
-	m, err := component.RenderManifests()
+	ret, err := component.RenderManifests()
 	if err != nil {
 		t.Fatalf("Rendering manifests with valid config should succeed, got: %s", err)
 	}
-	if len(m) <= 0 {
+
+	return ret
+}
+
+func testRenderManifest(t *testing.T, configHCL string) {
+	m := renderManifest(t, configHCL)
+	if len(m) == 0 {
 		t.Fatalf("Rendered manifests shouldn't be empty")
 	}
 }
@@ -97,3 +107,199 @@ component "metallb" {
 `
 	testRenderManifest(t, configHCL)
 }
+
+func getSpeakerDaemonset(t *testing.T, m map[string]string) *appsv1.DaemonSet {
+	dsStr, ok := m["daemonset-speaker.yaml"]
+	if !ok {
+		t.Fatalf("speaker daemonset config not found")
+	}
+
+	ds := &appsv1.DaemonSet{}
+	if err := yaml.Unmarshal([]byte(dsStr), ds); err != nil {
+		t.Fatalf("failed unmarshaling manifest: %v", err)
+	}
+
+	return ds
+}
+
+func getDeployController(t *testing.T, m map[string]string) *appsv1.Deployment {
+	deployStr, ok := m["deployment-controller.yaml"]
+	if !ok {
+		t.Fatalf("controller deployment config not found")
+	}
+
+	deploy := &appsv1.Deployment{}
+	if err := yaml.Unmarshal([]byte(deployStr), deploy); err != nil {
+		t.Fatalf("failed unmarshaling manifest: %v", err)
+	}
+
+	return deploy
+}
+
+// nolint:funlen
+func TestConversion(t *testing.T) {
+	configHCL := `
+component "metallb" {
+  address_pools = {
+	default = ["1.1.1.1/32", "2.2.2.2/32"]
+  }
+
+  speaker_toleration {
+	key      = "speaker_key1"
+	operator = "Equal"
+	value    = "value1"
+  }
+
+  speaker_toleration {
+	key      = "speaker_key2"
+    operator = "Equal"
+	value    = "value2"
+  }
+
+  speaker_node_selectors = {
+    "speaker_node_key1" = "speaker_node_value1"
+    "speaker_node_key2" = "speaker_node_value2"
+  }
+
+  controller_toleration {
+	key 	 = "controller_key1"
+	operator = "Equal"
+	value 	 = "value1"
+  }
+
+  controller_toleration {
+	key 	 = "controller_key2"
+	operator = "Equal"
+	value 	 = "value2"
+  }
+
+  controller_node_selectors = {
+    "controller_node_key1" = "controller_node_value1"
+    "controller_node_key2" = "controller_node_value2"
+  }
+
+  service_monitor = true
+}`
+
+	m := renderManifest(t, configHCL)
+	if len(m) == 0 {
+		t.Fatalf("Rendered manifests shouldn't be empty")
+	}
+
+	tcs := []struct {
+		Name string
+		Test func(*testing.T, map[string]string)
+	}{
+		{
+			"SpeakerConversions", func(t *testing.T, m map[string]string) {
+				ds := getSpeakerDaemonset(t, m)
+				expected := []corev1.Toleration{
+					{Key: "speaker_key1", Operator: "Equal", Value: "value1"},
+					{Key: "speaker_key2", Operator: "Equal", Value: "value2"},
+				}
+				if !reflect.DeepEqual(expected, ds.Spec.Template.Spec.Tolerations) {
+					t.Fatalf("expected: %#v, got: %#v", expected, ds.Spec.Template.Spec.Tolerations)
+				}
+			},
+		},
+		{
+			"SpeakerNodeSelectors", func(t *testing.T, m map[string]string) {
+				ds := getSpeakerDaemonset(t, m)
+				expected := map[string]string{
+					"beta.kubernetes.io/os": "linux",
+					"speaker_node_key1":     "speaker_node_value1",
+					"speaker_node_key2":     "speaker_node_value2",
+				}
+				if !reflect.DeepEqual(expected, ds.Spec.Template.Spec.NodeSelector) {
+					t.Fatalf("expected: %v, got: %v", expected, ds.Spec.Template.Spec.NodeSelector)
+				}
+			},
+		},
+		{
+			"ControllerTolerations", func(t *testing.T, m map[string]string) {
+				deploy := getDeployController(t, m)
+				expected := []corev1.Toleration{
+					{Key: "controller_key1", Operator: "Equal", Value: "value1"},
+					{Key: "controller_key2", Operator: "Equal", Value: "value2"},
+					{Key: "node-role.kubernetes.io/master", Effect: "NoSchedule"},
+				}
+				if !reflect.DeepEqual(expected, deploy.Spec.Template.Spec.Tolerations) {
+					t.Fatalf("expected: %+v\ngot: %+v", expected, deploy.Spec.Template.Spec.Tolerations)
+				}
+			},
+		},
+		{
+			"ControllerNodeSelectors", func(t *testing.T, m map[string]string) {
+				deploy := getDeployController(t, m)
+				expected := map[string]string{
+					"beta.kubernetes.io/os":     "linux",
+					"controller_node_key1":      "controller_node_value1",
+					"controller_node_key2":      "controller_node_value2",
+					"node.kubernetes.io/master": "",
+				}
+				if !reflect.DeepEqual(expected, deploy.Spec.Template.Spec.NodeSelector) {
+					t.Fatalf("expected: %v\ngot: %v", expected, deploy.Spec.Template.Spec.NodeSelector)
+				}
+			},
+		},
+		{
+			"MonitoringConfig", func(t *testing.T, m map[string]string) {
+				expectedConfig := []string{
+					"service.yaml",
+					"service-monitor.yaml",
+					"grafana-dashboard.yaml",
+					"grafana-alertmanager-rule.yaml",
+				}
+
+				for _, ec := range expectedConfig {
+					if _, ok := m[ec]; !ok {
+						t.Fatalf("expected %s to be generated but it is not available", ec)
+					}
+				}
+			},
+		},
+		{
+			"EIPConfig", func(t *testing.T, m map[string]string) {
+				expectedCM := `peer-autodiscovery:
+  from-labels:
+    my-asn: metallb.lokomotive.io/my-asn
+    peer-asn: metallb.lokomotive.io/peer-asn
+    peer-address: metallb.lokomotive.io/peer-address
+address-pools:
+- name: default
+  protocol: bgp
+  addresses:
+  - 1.1.1.1/32
+  - 2.2.2.2/32
+`
+
+				cmStr, ok := m["configmap.yaml"]
+				if !ok {
+					t.Fatalf("metallb configmap not found")
+				}
+
+				cm := &corev1.ConfigMap{}
+				if err := yaml.Unmarshal([]byte(cmStr), cm); err != nil {
+					t.Fatalf("failed unmarshalling manifest: %v", err)
+				}
+
+				gotCM, ok := cm.Data["config"]
+				if !ok {
+					t.Fatalf("metallb configmap is missing 'config' key")
+				}
+
+				if gotCM != expectedCM {
+					t.Fatalf("expected: %s, got: %s", expectedCM, gotCM)
+				}
+			},
+		},
+	}
+
+	for _, tc := range tcs {
+		tc := tc
+		t.Run(tc.Name, func(t *testing.T) {
+			t.Parallel()
+			tc.Test(t, m)
+		})
+	}
+}