diff --git a/pkg/scheduler/framework/plugins/podtopologyspread/filtering_test.go b/pkg/scheduler/framework/plugins/podtopologyspread/filtering_test.go index b9abaaefc1d..30f50806a5a 100644 --- a/pkg/scheduler/framework/plugins/podtopologyspread/filtering_test.go +++ b/pkg/scheduler/framework/plugins/podtopologyspread/filtering_test.go @@ -505,7 +505,7 @@ func TestPreFilterState(t *testing.T) { { MaxSkew: 3, TopologyKey: "node", - Selector: labels.SelectorFromValidatedSet(labels.Set{"foo": "bar"}), + Selector: mustConvertLabelSelectorAsSelector(t, st.MakeLabelSelector().Label("foo", "bar").Obj()), MinDomains: 1, NodeAffinityPolicy: v1.NodeInclusionPolicyHonor, NodeTaintsPolicy: v1.NodeInclusionPolicyIgnore, @@ -513,7 +513,7 @@ func TestPreFilterState(t *testing.T) { { MaxSkew: 5, TopologyKey: "rack", - Selector: labels.SelectorFromValidatedSet(labels.Set{"foo": "bar"}), + Selector: mustConvertLabelSelectorAsSelector(t, st.MakeLabelSelector().Label("foo", "bar").Obj()), MinDomains: 1, NodeAffinityPolicy: v1.NodeInclusionPolicyHonor, NodeTaintsPolicy: v1.NodeInclusionPolicyIgnore, @@ -1283,7 +1283,7 @@ func TestPreFilterState(t *testing.T) { { MaxSkew: 1, TopologyKey: "zone", - Selector: labels.SelectorFromValidatedSet(labels.Set{"foo": "a"}), + Selector: mustConvertLabelSelectorAsSelector(t, st.MakeLabelSelector().Label("foo", "a").Obj()), MinDomains: 1, NodeAffinityPolicy: v1.NodeInclusionPolicyHonor, NodeTaintsPolicy: v1.NodeInclusionPolicyIgnore, diff --git a/staging/src/k8s.io/apimachinery/pkg/labels/labels.go b/staging/src/k8s.io/apimachinery/pkg/labels/labels.go index 8360d842b6c..a7e275a449e 100644 --- a/staging/src/k8s.io/apimachinery/pkg/labels/labels.go +++ b/staging/src/k8s.io/apimachinery/pkg/labels/labels.go @@ -77,6 +77,8 @@ func (ls Set) AsValidatedSelector() (Selector, error) { // perform any validation. // According to our measurements this is significantly faster // in codepaths that matter at high scale. +// Note: this method copies the Set; if the Set is immutable, consider wrapping it with SetSelector +// instead, which does not copy. func (ls Set) AsSelectorPreValidated() Selector { return SelectorFromValidatedSet(ls) } diff --git a/staging/src/k8s.io/apimachinery/pkg/labels/selector.go b/staging/src/k8s.io/apimachinery/pkg/labels/selector.go index 5155a3ca941..5e1c980f41a 100644 --- a/staging/src/k8s.io/apimachinery/pkg/labels/selector.go +++ b/staging/src/k8s.io/apimachinery/pkg/labels/selector.go @@ -919,14 +919,6 @@ func SelectorFromSet(ls Set) Selector { return SelectorFromValidatedSet(ls) } -var ( - // Create constants for hot-path selectors - value0Path, keyPath = func() (*field.Path, *field.Path) { - path := field.ToPath() - return path.Child("values").Index(0), path.Child("key") - }() -) - // ValidatedSelectorFromSet returns a Selector which will match exactly the given Set. A // nil and empty Sets are considered equivalent to Everything(). // The Set is validated client-side, which allows to catch errors early. @@ -934,29 +926,35 @@ func ValidatedSelectorFromSet(ls Set) (Selector, error) { if ls == nil || len(ls) == 0 { return internalSelector{}, nil } - - var allErrs field.ErrorList - for k, v := range ls { - if err := validateLabelKey(k, keyPath); err != nil { - allErrs = append(allErrs, err) - } - if err := validateLabelValue(k, v, value0Path); err != nil { - allErrs = append(allErrs, err) + requirements := make([]Requirement, 0, len(ls)) + for label, value := range ls { + r, err := NewRequirement(label, selection.Equals, []string{value}) + if err != nil { + return nil, err } + requirements = append(requirements, *r) } - - // TODO: validate labels are valid label values - return setSelector(ls), allErrs.ToAggregate() + // sort to have deterministic string representation + sort.Sort(ByKey(requirements)) + return internalSelector(requirements), nil } // SelectorFromValidatedSet returns a Selector which will match exactly the given Set. // A nil and empty Sets are considered equivalent to Everything(). // It assumes that Set is already validated and doesn't do any validation. +// Note: this method copies the Set; if the Set is immutable, consider wrapping it with SetSelector +// instead, which does not copy. func SelectorFromValidatedSet(ls Set) Selector { if ls == nil || len(ls) == 0 { return internalSelector{} } - return setSelector(ls) + requirements := make([]Requirement, 0, len(ls)) + for label, value := range ls { + requirements = append(requirements, Requirement{key: label, operator: selection.Equals, strValues: []string{value}}) + } + // sort to have deterministic string representation + sort.Sort(ByKey(requirements)) + return internalSelector(requirements) } // ParseToRequirements takes a string representing a selector and returns a list of @@ -968,9 +966,13 @@ func ParseToRequirements(selector string, opts ...field.PathOption) ([]Requireme return parse(selector, field.ToPath(opts...)) } -type setSelector Set +// SetSelector wraps a Set, allowing it to implement the Selector interface. Unlike +// Set.AsSelectorPreValidated (which copies the input Set), this type simply wraps the underlying +// Set. As a result, it is substantially more efficient, but requires the caller to not mutate the +// Set. +type SetSelector Set -func (s setSelector) Matches(labels Labels) bool { +func (s SetSelector) Matches(labels Labels) bool { for k, v := range s { if !labels.Has(k) || v != labels.Get(k) { return false @@ -979,11 +981,11 @@ func (s setSelector) Matches(labels Labels) bool { return true } -func (s setSelector) Empty() bool { +func (s SetSelector) Empty() bool { return len(s) == 0 } -func (s setSelector) String() string { +func (s SetSelector) String() string { keys := make([]string, 0, len(s)) for k := range s { keys = append(keys, k) @@ -1004,28 +1006,28 @@ func (s setSelector) String() string { return b.String() } -func (s setSelector) Add(r ...Requirement) Selector { +func (s SetSelector) Add(r ...Requirement) Selector { return s.toFullSelector().Add(r...) } -func (s setSelector) Requirements() (requirements Requirements, selectable bool) { +func (s SetSelector) Requirements() (requirements Requirements, selectable bool) { return s.toFullSelector().Requirements() } -func (s setSelector) DeepCopySelector() Selector { - res := make(setSelector, len(s)) +func (s SetSelector) DeepCopySelector() Selector { + res := make(SetSelector, len(s)) for k, v := range s { res[k] = v } return res } -func (s setSelector) RequiresExactMatch(label string) (value string, found bool) { +func (s SetSelector) RequiresExactMatch(label string) (value string, found bool) { v, f := s[label] return v, f } -func (s setSelector) toFullSelector() Selector { +func (s SetSelector) toFullSelector() Selector { if s == nil || len(s) == 0 { return internalSelector{} } @@ -1038,4 +1040,4 @@ func (s setSelector) toFullSelector() Selector { return internalSelector(requirements) } -var _ Selector = setSelector{} +var _ Selector = SetSelector{} diff --git a/staging/src/k8s.io/apimachinery/pkg/labels/selector_test.go b/staging/src/k8s.io/apimachinery/pkg/labels/selector_test.go index 76fc82beca6..8557c12c772 100644 --- a/staging/src/k8s.io/apimachinery/pkg/labels/selector_test.go +++ b/staging/src/k8s.io/apimachinery/pkg/labels/selector_test.go @@ -826,6 +826,28 @@ func BenchmarkSelectorFromValidatedSet(b *testing.B) { } } +func BenchmarkSetSelector(b *testing.B) { + set := map[string]string{ + "foo": "foo", + "bar": "bar", + } + matchee := Set(map[string]string{ + "foo": "foo", + "bar": "bar", + "extra": "label", + }) + + for i := 0; i < b.N; i++ { + s := SetSelector(set) + if s.Empty() { + b.Errorf("Unexpected selector") + } + if !s.Matches(matchee) { + b.Errorf("Unexpected match") + } + } +} + func TestSetSelectorString(t *testing.T) { cases := []struct { set Set @@ -847,7 +869,7 @@ func TestSetSelectorString(t *testing.T) { for _, tt := range cases { t.Run(tt.out, func(t *testing.T) { - if got := setSelector(tt.set).String(); tt.out != got { + if got := SetSelector(tt.set).String(); tt.out != got { t.Fatalf("expected %v, got %v", tt.out, got) } }) @@ -936,13 +958,19 @@ func TestValidatedSelectorFromSet(t *testing.T) { tests := []struct { name string input Set - expectedSelector Selector + expectedSelector internalSelector expectedError field.ErrorList }{ { - name: "Simple Set, no error", - input: Set{"key": "val"}, - expectedSelector: setSelector{"key": "val"}, + name: "Simple Set, no error", + input: Set{"key": "val"}, + expectedSelector: internalSelector{ + Requirement{ + key: "key", + operator: selection.Equals, + strValues: []string{"val"}, + }, + }, }, { name: "Invalid Set, value too long",