fix(support-bundle): agent doesn't respect node-selector and taint-toleration

controller/support_bundle_controller.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"net/http"
 	"reflect"
+	"strings"
 	"time"
 
 	"github.com/pkg/errors"
@@ -586,11 +587,6 @@ func (c *SupportBundleController) createSupportBundleManagerDeployment(supportBu
 		return nil, err
 	}
 
-	tolerations, err := c.ds.GetSettingTaintToleration()
-	if err != nil {
-		return nil, errors.Wrapf(err, "failed to get taint toleration setting before creating support bundle manager deployment")
-	}
-
 	imagePullPolicy, err := c.ds.GetSettingImagePullPolicy()
 	if err != nil {
 		return nil, errors.Wrapf(err, "failed to get system pods image pull policy before creating support bundle manager deployment")
@@ -608,13 +604,25 @@ func (c *SupportBundleController) createSupportBundleManagerDeployment(supportBu
 	}
 	registrySecret := registrySecretSetting.Value
 
-	newSupportBundleManager := c.newSupportBundleManager(supportBundle, nodeSelector, tolerations, imagePullPolicy, priorityClass, registrySecret)
+	newSupportBundleManager, err := c.newSupportBundleManager(supportBundle, nodeSelector, imagePullPolicy, priorityClass, registrySecret)
+	if err != nil {
+		return nil, errors.Wrapf(err, "failed to create new support bundle manager")
+	}
 	return c.ds.CreateDeployment(newSupportBundleManager)
 }
 
-func (c *SupportBundleController) newSupportBundleManager(supportBundle *longhorn.SupportBundle,
-	nodeSelector map[string]string, tolerations []corev1.Toleration,
-	imagePullPolicy corev1.PullPolicy, priorityClass, registrySecret string) *appsv1.Deployment {
+func (c *SupportBundleController) newSupportBundleManager(supportBundle *longhorn.SupportBundle, nodeSelector map[string]string,
+	imagePullPolicy corev1.PullPolicy, priorityClass, registrySecret string) (*appsv1.Deployment, error) {
+
+	tolerationSetting, err := c.ds.GetSetting(types.SettingNameTaintToleration)
+	if err != nil {
+		return nil, errors.Wrapf(err, "failed to get %v setting", types.SettingNameTaintToleration)
+	}
+	tolerations, err := types.UnmarshalTolerations(tolerationSetting.Value)
+	if err != nil {
+		return nil, err
+	}
+
 	supportBundleManagerName := GetSupportBundleManagerName(supportBundle)
 	deployment := &appsv1.Deployment{
 		ObjectMeta: metav1.ObjectMeta{
@@ -687,6 +695,14 @@ func (c *SupportBundleController) newSupportBundleManager(supportBundle *longhor
 									Name:  "SUPPORT_BUNDLE_COLLECTOR",
 									Value: "longhorn",
 								},
+								{
+									Name:  "SUPPORT_BUNDLE_NODE_SELECTOR",
+									Value: c.getNodeSelectorString(nodeSelector),
+								},
+								{
+									Name:  "SUPPORT_BUNDLE_TAINT_TOLERATION",
+									Value: c.getTaintTolerationString(tolerationSetting),
+								},
 							},
 							Ports: []corev1.ContainerPort{
 								{
@@ -708,5 +724,20 @@ func (c *SupportBundleController) newSupportBundleManager(supportBundle *longhor
 		}
 	}
 
-	return deployment
+	return deployment, nil
+}
+
+func (c *SupportBundleController) getNodeSelectorString(nodeSelector map[string]string) string {
+	list := make([]string, 0, len(nodeSelector))
+	for k, v := range nodeSelector {
+		list = append(list, k+"="+v)
+	}
+	return strings.Join(list, ",")
+}
+
+func (c *SupportBundleController) getTaintTolerationString(setting *longhorn.Setting) string {
+	if setting == nil {
+		return ""
+	}
+	return strings.ReplaceAll(setting.Value, ";", ",")
 }

types/setting.go

@@ -1437,54 +1437,55 @@ func getDefaultSettingFromYAML(defaultSettingYAMLData []byte) (map[string]string
 	return defaultSettings, nil
 }
 
-func ValidateAndUnmarshalToleration(s string) (*v1.Toleration, error) {
-	toleration := &v1.Toleration{}
-
-	// The schema should be `key=value:effect` or `key:effect`
-	s = strings.Trim(s, " ")
-	parts := strings.Split(s, ":")
-	if len(parts) != 2 {
-		return nil, fmt.Errorf("invalid toleration setting %v: should contain both effect and key/value pair", s)
-	}
+func UnmarshalTolerations(tolerationSetting string) ([]v1.Toleration, error) {
+	tolerations := []v1.Toleration{}
 
-	effect := v1.TaintEffect(strings.Trim(parts[1], " "))
-	if effect != v1.TaintEffectNoExecute && effect != v1.TaintEffectNoSchedule && effect != v1.TaintEffectPreferNoSchedule && effect != v1.TaintEffect("") {
-		return nil, fmt.Errorf("invalid toleration setting %v: invalid effect", parts[1])
+	tolerationSetting = strings.ReplaceAll(tolerationSetting, " ", "")
+	if tolerationSetting == "" {
+		return tolerations, nil
 	}
-	toleration.Effect = effect
 
-	if strings.Contains(parts[0], "=") {
-		pair := strings.Split(parts[0], "=")
-		if len(pair) != 2 {
-			return nil, fmt.Errorf("invalid toleration setting %v: invalid key/value pair", parts[0])
+	tolerationList := strings.Split(tolerationSetting, ";")
+	for _, toleration := range tolerationList {
+		toleration, err := parseToleration(toleration)
+		if err != nil {
+			return nil, errors.Wrapf(err, "failed to parse toleration: %s", toleration)
 		}
-		toleration.Key = strings.Trim(pair[0], " ")
-		toleration.Value = strings.Trim(pair[1], " ")
-		toleration.Operator = v1.TolerationOpEqual
-	} else {
-		toleration.Key = strings.Trim(parts[0], " ")
-		toleration.Operator = v1.TolerationOpExists
+		tolerations = append(tolerations, *toleration)
 	}
-
-	return toleration, nil
+	return tolerations, nil
 }
 
-func UnmarshalTolerations(tolerationSetting string) ([]v1.Toleration, error) {
-	res := []v1.Toleration{}
+func parseToleration(taintToleration string) (*v1.Toleration, error) {
+	// The schema should be `key=value:effect` or `key:effect`
+	parts := strings.Split(taintToleration, ":")
+	if len(parts) != 2 {
+		return nil, fmt.Errorf("missing key/value and effect pair")
+	}
 
-	tolerationSetting = strings.Trim(tolerationSetting, " ")
-	if tolerationSetting != "" {
-		tolerationList := strings.Split(tolerationSetting, ";")
-		for _, t := range tolerationList {
-			toleration, err := ValidateAndUnmarshalToleration(t)
-			if err != nil {
-				return nil, err
-			}
-			res = append(res, *toleration)
-		}
+	// parse `key=value` or `key`
+	key, value, operator := "", "", v1.TolerationOperator("")
+	pair := strings.Split(parts[0], "=")
+	switch len(pair) {
+	case 1:
+		key, value, operator = parts[0], "", v1.TolerationOpExists
+	case 2:
+		key, value, operator = pair[0], pair[1], v1.TolerationOpEqual
+	}
+
+	effect := v1.TaintEffect(parts[1])
+	switch effect {
+	case "", v1.TaintEffectNoExecute, v1.TaintEffectNoSchedule, v1.TaintEffectPreferNoSchedule:
+	default:
+		return nil, fmt.Errorf("invalid effect: %v", parts[1])
 	}
 
-	return res, nil
+	return &v1.Toleration{
+		Key:      key,
+		Value:    value,
+		Operator: operator,
+		Effect:   effect,
+	}, nil
 }
 
 func validateAndUnmarshalLabel(label string) (key, value string, err error) {

types/types_test.go

@@ -0,0 +1,102 @@
+package types
+
+import (
+	"fmt"
+	"reflect"
+	"testing"
+
+	corev1 "k8s.io/api/core/v1"
+)
+
+func TestParseToleration(t *testing.T) {
+	type testCase struct {
+		input string
+
+		expectedToleration []corev1.Toleration
+		expectError        bool
+	}
+	testCases := map[string]testCase{
+		"valid empty setting": {
+			input:              "",
+			expectedToleration: []corev1.Toleration{},
+			expectError:        false,
+		},
+		"valid key:NoSchedule": {
+			input: "key:NoSchedule",
+			expectedToleration: []corev1.Toleration{
+				{
+					Key:      "key",
+					Value:    "",
+					Operator: corev1.TolerationOpExists,
+					Effect:   corev1.TaintEffectNoSchedule,
+				},
+			},
+			expectError: false,
+		},
+		"valid key=value:NoExecute": {
+			input: "key=value:NoExecute",
+			expectedToleration: []corev1.Toleration{
+				{
+					Key:      "key",
+					Value:    "value",
+					Operator: corev1.TolerationOpEqual,
+					Effect:   corev1.TaintEffectNoExecute,
+				},
+			},
+			expectError: false,
+		},
+		"valid key=value:PreferNoSchedule": {
+			input: "key=value:PreferNoSchedule",
+			expectedToleration: []corev1.Toleration{
+				{
+					Key:      "key",
+					Value:    "value",
+					Operator: corev1.TolerationOpEqual,
+					Effect:   corev1.TaintEffectPreferNoSchedule,
+				},
+			},
+			expectError: false,
+		},
+		"valid key0:NoSchedule;key1=value:NoExecute": {
+			input: "key0:NoSchedule;key1=value:NoExecute",
+			expectedToleration: []corev1.Toleration{
+				{
+					Key:      "key0",
+					Value:    "",
+					Operator: corev1.TolerationOpExists,
+					Effect:   corev1.TaintEffectNoSchedule,
+				},
+				{
+					Key:      "key1",
+					Value:    "value",
+					Operator: corev1.TolerationOpEqual,
+					Effect:   corev1.TaintEffectNoExecute,
+				},
+			},
+			expectError: false,
+		},
+		"invalid key:InvalidEffect": {
+			input:              "key:InvalidEffect",
+			expectedToleration: nil,
+			expectError:        true,
+		},
+		"invalid key=value=NoSchedule": {
+			input:              "key=value=NoSchedule",
+			expectedToleration: nil,
+			expectError:        true,
+		},
+	}
+
+	for name, test := range testCases {
+		fmt.Printf("testing %v\n", name)
+
+		toleration, err := UnmarshalTolerations(test.input)
+		if !reflect.DeepEqual(toleration, test.expectedToleration) {
+			t.Errorf("unexpected toleration:\nGot: %v\nWant: %v", toleration, test.expectedToleration)
+		}
+
+		if test.expectError && err == nil {
+			t.Errorf("unexpected error: %v", err)
+		}
+	}
+}