From b8cc0fc5720a8ff4de9136e1ddcdea15e5abe609 Mon Sep 17 00:00:00 2001 From: Chin-Ya Huang Date: Tue, 21 Mar 2023 14:53:16 +0800 Subject: [PATCH 1/4] fix(support-bundle): pass node-selector to agent Ref: 5614 Signed-off-by: Chin-Ya Huang --- controller/support_bundle_controller.go | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/controller/support_bundle_controller.go b/controller/support_bundle_controller.go index 7d7b16b2da..576d786b31 100644 --- a/controller/support_bundle_controller.go +++ b/controller/support_bundle_controller.go @@ -5,6 +5,7 @@ import ( "fmt" "net/http" "reflect" + "strings" "time" "github.com/pkg/errors" @@ -615,6 +616,7 @@ func (c *SupportBundleController) createSupportBundleManagerDeployment(supportBu func (c *SupportBundleController) newSupportBundleManager(supportBundle *longhorn.SupportBundle, nodeSelector map[string]string, tolerations []corev1.Toleration, imagePullPolicy corev1.PullPolicy, priorityClass, registrySecret string) *appsv1.Deployment { + supportBundleManagerName := GetSupportBundleManagerName(supportBundle) deployment := &appsv1.Deployment{ ObjectMeta: metav1.ObjectMeta{ @@ -687,6 +689,10 @@ func (c *SupportBundleController) newSupportBundleManager(supportBundle *longhor Name: "SUPPORT_BUNDLE_COLLECTOR", Value: "longhorn", }, + { + Name: "SUPPORT_BUNDLE_NODE_SELECTOR", + Value: c.getNodeSelectorString(nodeSelector), + }, }, Ports: []corev1.ContainerPort{ { @@ -710,3 +716,11 @@ func (c *SupportBundleController) newSupportBundleManager(supportBundle *longhor return deployment } + +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, ",") +} From 7f4ac375b538addd80493860eaea5112e6f8d868 Mon Sep 17 00:00:00 2001 From: Chin-Ya Huang Date: Tue, 21 Mar 2023 16:18:39 +0800 Subject: [PATCH 2/4] fix(support-bundle): pass taint-toleration to agent Ref: 5614 Signed-off-by: Chin-Ya Huang --- controller/support_bundle_controller.go | 37 ++++++++++++++++++------- 1 file changed, 27 insertions(+), 10 deletions(-) diff --git a/controller/support_bundle_controller.go b/controller/support_bundle_controller.go index 576d786b31..6f6b3ccffc 100644 --- a/controller/support_bundle_controller.go +++ b/controller/support_bundle_controller.go @@ -587,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") @@ -609,13 +604,24 @@ 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{ @@ -693,6 +699,10 @@ func (c *SupportBundleController) newSupportBundleManager(supportBundle *longhor Name: "SUPPORT_BUNDLE_NODE_SELECTOR", Value: c.getNodeSelectorString(nodeSelector), }, + { + Name: "SUPPORT_BUNDLE_TAINT_TOLERATION", + Value: c.getTaintTolerationString(tolerationSetting), + }, }, Ports: []corev1.ContainerPort{ { @@ -714,7 +724,7 @@ func (c *SupportBundleController) newSupportBundleManager(supportBundle *longhor } } - return deployment + return deployment, nil } func (c *SupportBundleController) getNodeSelectorString(nodeSelector map[string]string) string { @@ -724,3 +734,10 @@ func (c *SupportBundleController) getNodeSelectorString(nodeSelector map[string] } return strings.Join(list, ",") } + +func (c *SupportBundleController) getTaintTolerationString(setting *longhorn.Setting) string { + if setting == nil { + return "" + } + return strings.ReplaceAll(setting.Value, ";", ",") +} From c701184bd837f817a38b2213196207896219a88c Mon Sep 17 00:00:00 2001 From: Chin-Ya Huang Date: Tue, 21 Mar 2023 16:31:36 +0800 Subject: [PATCH 3/4] refactor(setting): unmarshal tolerations Signed-off-by: Chin-Ya Huang --- types/setting.go | 77 ++++++++++++++++++++++++------------------------ 1 file changed, 39 insertions(+), 38 deletions(-) diff --git a/types/setting.go b/types/setting.go index ea789b0b6e..761cec328e 100644 --- a/types/setting.go +++ b/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) { From 7c9ce2222fe85af7ff14998139e01e75e8500cba Mon Sep 17 00:00:00 2001 From: Chin-Ya Huang Date: Thu, 23 Mar 2023 11:14:29 +0800 Subject: [PATCH 4/4] test(types): UnmarshalTolerations Signed-off-by: Chin-Ya Huang --- types/types_test.go | 102 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 102 insertions(+) create mode 100644 types/types_test.go diff --git a/types/types_test.go b/types/types_test.go new file mode 100644 index 0000000000..82899ffbbe --- /dev/null +++ b/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) + } + } +}