diff --git a/pkg/util/tolerations/tolerations.go b/pkg/util/tolerations/tolerations.go index ad28b5e1e70..316b6437244 100644 --- a/pkg/util/tolerations/tolerations.go +++ b/pkg/util/tolerations/tolerations.go @@ -46,7 +46,7 @@ next: // another, only the superset is kept. func MergeTolerations(first, second []api.Toleration) []api.Toleration { all := append(first, second...) - merged := []api.Toleration{} + var merged []api.Toleration next: for i, t := range all { @@ -58,7 +58,7 @@ next: if i+1 < len(all) { for _, t2 := range all[i+1:] { // If the tolerations are equal, prefer the first. - if isSuperset(t2, t) && !apiequality.Semantic.DeepEqual(&t, &t2) { + if !apiequality.Semantic.DeepEqual(&t, &t2) && isSuperset(t2, t) { continue next // t is redundant; ignore it } } diff --git a/plugin/pkg/admission/podtolerationrestriction/BUILD b/plugin/pkg/admission/podtolerationrestriction/BUILD index 3e8d84eb05b..98aa60a1b35 100644 --- a/plugin/pkg/admission/podtolerationrestriction/BUILD +++ b/plugin/pkg/admission/podtolerationrestriction/BUILD @@ -14,7 +14,6 @@ go_test( "//pkg/apis/core:go_default_library", "//pkg/features:go_default_library", "//pkg/scheduler/api:go_default_library", - "//pkg/util/tolerations:go_default_library", "//plugin/pkg/admission/podtolerationrestriction/apis/podtolerationrestriction:go_default_library", "//staging/src/k8s.io/api/core/v1:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/api/resource:go_default_library", @@ -27,6 +26,7 @@ go_test( "//staging/src/k8s.io/client-go/kubernetes:go_default_library", "//staging/src/k8s.io/client-go/kubernetes/fake:go_default_library", "//staging/src/k8s.io/component-base/featuregate/testing:go_default_library", + "//vendor/github.com/stretchr/testify/assert:go_default_library", ], ) diff --git a/plugin/pkg/admission/podtolerationrestriction/admission.go b/plugin/pkg/admission/podtolerationrestriction/admission.go index dfc20260472..7747251d8e8 100644 --- a/plugin/pkg/admission/podtolerationrestriction/admission.go +++ b/plugin/pkg/admission/podtolerationrestriction/admission.go @@ -84,7 +84,7 @@ func (p *Plugin) Admit(ctx context.Context, a admission.Attributes, o admission. } pod := a.GetObject().(*api.Pod) - var finalTolerations []api.Toleration + var extraTolerations []api.Toleration if a.GetOperation() == admission.Create { ts, err := p.getNamespaceDefaultTolerations(a.GetNamespace()) if err != nil { @@ -97,37 +97,20 @@ func (p *Plugin) Admit(ctx context.Context, a admission.Attributes, o admission. ts = p.pluginConfig.Default } - if len(ts) > 0 { - if len(pod.Spec.Tolerations) > 0 { - if tolerations.IsConflict(ts, pod.Spec.Tolerations) { - return fmt.Errorf("namespace tolerations and pod tolerations conflict") - } - - // modified pod tolerations = namespace tolerations + current pod tolerations - finalTolerations = tolerations.MergeTolerations(ts, pod.Spec.Tolerations) - } else { - finalTolerations = ts - - } - } else { - finalTolerations = pod.Spec.Tolerations - } - } else { - finalTolerations = pod.Spec.Tolerations + extraTolerations = ts } if qoshelper.GetPodQOS(pod) != api.PodQOSBestEffort { - finalTolerations = tolerations.MergeTolerations(finalTolerations, []api.Toleration{ - { - Key: schedulerapi.TaintNodeMemoryPressure, - Operator: api.TolerationOpExists, - Effect: api.TaintEffectNoSchedule, - }, + extraTolerations = append(extraTolerations, api.Toleration{ + Key: schedulerapi.TaintNodeMemoryPressure, + Operator: api.TolerationOpExists, + Effect: api.TaintEffectNoSchedule, }) } - // Final merge of tolerations irrespective of pod type, if the user while creating pods gives - // conflicting tolerations(with same key+effect), the existing ones should be overwritten by latest one - pod.Spec.Tolerations = tolerations.MergeTolerations(finalTolerations, []api.Toleration{}) + // Final merge of tolerations irrespective of pod type. + if len(extraTolerations) > 0 { + pod.Spec.Tolerations = tolerations.MergeTolerations(pod.Spec.Tolerations, extraTolerations) + } return p.Validate(ctx, a, o) } diff --git a/plugin/pkg/admission/podtolerationrestriction/admission_test.go b/plugin/pkg/admission/podtolerationrestriction/admission_test.go index 2fde37fa5c5..fcf9c545eba 100644 --- a/plugin/pkg/admission/podtolerationrestriction/admission_test.go +++ b/plugin/pkg/admission/podtolerationrestriction/admission_test.go @@ -22,6 +22,7 @@ import ( "testing" "time" + "github.com/stretchr/testify/assert" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -36,7 +37,6 @@ import ( api "k8s.io/kubernetes/pkg/apis/core" "k8s.io/kubernetes/pkg/features" schedulerapi "k8s.io/kubernetes/pkg/scheduler/api" - "k8s.io/kubernetes/pkg/util/tolerations" pluginapi "k8s.io/kubernetes/plugin/pkg/admission/podtolerationrestriction/apis/podtolerationrestriction" ) @@ -139,10 +139,11 @@ func TestPodAdmission(t *testing.T) { { pod: bestEffortPod, defaultClusterTolerations: []api.Toleration{}, - namespaceTolerations: []api.Toleration{{Key: "testKey", Operator: "Equal", Value: "testValue", Effect: "NoSchedule", TolerationSeconds: nil}}, - podTolerations: []api.Toleration{{Key: "testKey", Operator: "Equal", Value: "testValue1", Effect: "NoSchedule", TolerationSeconds: nil}}, - admit: false, - testName: "conflicting pod and namespace tolerations", + namespaceTolerations: []api.Toleration{{Key: "testKey", Operator: "Equal", Value: "testValue", Effect: "NoSchedule"}}, + podTolerations: []api.Toleration{{Key: "testKey", Operator: "Equal", Value: "testValue1", Effect: "NoSchedule"}}, + mergedTolerations: []api.Toleration{{Key: "testKey", Operator: "Equal", Value: "testValue", Effect: "NoSchedule"}, {Key: "testKey", Operator: "Equal", Value: "testValue1", Effect: "NoSchedule"}}, + admit: true, + testName: "duplicate key pod and namespace tolerations", }, { pod: bestEffortPod, @@ -210,10 +211,11 @@ func TestPodAdmission(t *testing.T) { whitelist: []api.Toleration{}, podTolerations: []api.Toleration{{Key: "testKey", Operator: "Equal", Value: "testValue", Effect: "NoSchedule", TolerationSeconds: nil}, {Key: "testKey", Operator: "Equal", Value: "testValue1", Effect: "NoSchedule", TolerationSeconds: nil}}, mergedTolerations: []api.Toleration{ + {Key: "testKey", Operator: "Equal", Value: "testValue", Effect: "NoSchedule", TolerationSeconds: nil}, {Key: "testKey", Operator: "Equal", Value: "testValue1", Effect: "NoSchedule", TolerationSeconds: nil}, }, admit: true, - testName: "Besteffort pod should overwrite for conflicting tolerations", + testName: "Pod with duplicate key tolerations should not be modified", }, { pod: guaranteedPod, @@ -275,8 +277,8 @@ func TestPodAdmission(t *testing.T) { } updatedPodTolerations := pod.Spec.Tolerations - if test.admit && !tolerations.EqualTolerations(updatedPodTolerations, test.mergedTolerations) { - t.Errorf("Test: %s, expected: %#v but got: %#v", test.testName, test.mergedTolerations, updatedPodTolerations) + if test.admit { + assert.ElementsMatch(t, updatedPodTolerations, test.mergedTolerations) } }) }