Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 41 additions & 10 deletions controller/support_bundle_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"fmt"
"net/http"
"reflect"
"strings"
"time"

"github.com/pkg/errors"
Expand Down Expand Up @@ -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")
Expand All @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean you can use tolerations []corev1.Toleration instead of querying the setting again.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, I was confused because toleration []corev1.Toleration is unmarshalled from the setting, directly using it needs to be put back to a string again.

I've moved the unmarshalling into the same method so it won't be called again. PTAL, thank you.

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{
Expand Down Expand Up @@ -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{
{
Expand All @@ -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, ";", ",")
}
77 changes: 39 additions & 38 deletions types/setting.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
102 changes: 102 additions & 0 deletions types/types_test.go
Original file line number Diff line number Diff line change
@@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: There should be some assert functions able to use?

t.Errorf("unexpected toleration:\nGot: %v\nWant: %v", toleration, test.expectedToleration)
}

if test.expectError && err == nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: The same. Suggest using assert like https://pkg.go.dev/github.com/stretchr/testify/assert.

t.Errorf("unexpected error: %v", err)
}
}
}