Added MatchFields to NodeSelectorTerm #62002

k82cn · 2018-04-02T03:03:08Z

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
part of #61410

Special notes for your reviewer:
According to the discussion at #61410 , we'd like to introduce a new selector term for node's field.

Release note:

Added `MatchFields` to `NodeSelectorTerm`; in 1.11, it only support `metadata.name`.

k82cn · 2018-04-02T12:07:41Z

/retest

k82cn · 2018-04-02T12:32:06Z

/cc @liggitt @smarterclayton

k82cn · 2018-04-03T12:36:33Z

/retest

liggitt · 2018-04-04T02:31:40Z

pkg/apis/core/types.go

@@ -2274,6 +2274,8 @@ type NodeSelector struct {
 type NodeSelectorTerm struct {
 	//Required. A list of node selector requirements. The requirements are ANDed.
 	MatchExpressions []NodeSelectorRequirement
+	//Optional. A list of node selector requirements by node's field. The requirements are ANDed.
+	MatchFields []NodeSelectorRequirement


does this change MatchExpressions to be optional (at least one of MatchExpressions and MatchFields must be specified?), or would you expect this to be paired with a MatchExpression that selected everything (is that even possible)?

at least one of MatchExpressions and MatchFields must be specified

This one :) And the requirements are ANDed; if both are not empty, they also are ANDed.

Done; updated comments and added validation. Does not check the key of MatchFields, will handle it when adding related logic in scheduler.

k82cn · 2018-04-09T11:57:51Z

ping for comments :)

/retest

k82cn · 2018-04-10T00:44:56Z

/sig scheduling

k82cn · 2018-04-11T04:10:56Z

/retest

liggitt · 2018-04-11T04:45:25Z

pkg/apis/core/validation/validation.go

@@ -3008,12 +3008,19 @@ func ValidateNodeSelectorRequirement(rq core.NodeSelectorRequirement, fldPath *f
 func ValidateNodeSelectorTerm(term core.NodeSelectorTerm, fldPath *field.Path) field.ErrorList {
 	allErrs := field.ErrorList{}

-	if len(term.MatchExpressions) == 0 {
-		return append(allErrs, field.Required(fldPath.Child("matchExpressions"), "must have at least one node selector requirement"))
+	if len(term.MatchExpressions) == 0 && len(term.MatchFields) == 0 {


I think this loosening of validation makes sense, but can cause grief for a 1.10-level apiserver (during rolling upgrade or downgrade from running 1.11):

an object with a podspec is submitted with MatchFields set and MatchExpressions unset, 1.11 apiserver validates and persists it

during rolling HA upgrade or after downgrade to 1.10, a 1.10 apiserver will not know about the MatchFields field, and would consider the object invalid and disallow updates to it (and possibly disallow deleting it)

The doc on the NodeSelectorTerm type says // A null or empty node selector term matches no objects. which would be acceptable for the 1.10 scheduler to do (would fail safe with "unscheduleable" error), but the actual API validation code in 1.10 prevents an empty node selector term from being allowed.

The doc on the NodeSelectorTerm type says // A null or empty node selector term matches no objects. which would be acceptable for the 1.10 scheduler to do (would fail safe with "unscheduleable" error), but the actual API validation code in 1.10 prevents an empty node selector term from being allowed.

Good point; the implementation of schedule match this doc/comment. Scheduler will let pod pending with 'unscheduleable' error. I think we should remove this validation in 1.11; for 1.10, not sure, cherry-pick??

I'm a little confused about this; another meaning maybe: the NodeSelectorTerms []NodeSelectorTerm in NodeSelector; for each single term, the MatchExpressions can not be nil or empty.

But both cases (NodeSelectorTerms is nill and MatchExpressions is nill ) are handled by scheduler (nodeMatchesNodeSelectorTerms and NodeSelectorRequirementsAsSelector); so that's safe to remove it in 1.11, for 1.10, not sure, cherry-pick it?

I'm a little confused about this; another meaning maybe: the NodeSelectorTerms []NodeSelectorTerm in NodeSelector; for each single term, the MatchExpressions can not be nil or empty.

the documentation is pretty clear, and the implementation actually behaves as documented if it is given a NodeSelectorTerm that is empty (v1helper.NodeSelectorRequirementsAsSelector(req.MatchExpressions) returns labels.Nothing() if there are no match expressions)

I would pick a fix to validation in 1.10 to tolerate missing MatchExpressions:

func ValidateNodeSelectorTerm(term core.NodeSelectorTerm, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} - if len(term.MatchExpressions) == 0 { - return append(allErrs, field.Required(fldPath.Child("matchExpressions"), "must have at least one node selector requirement")) - } for j, req := range term.MatchExpressions { allErrs = append(allErrs, ValidateNodeSelectorRequirement(req, fldPath.Child("matchExpressions").Index(j))...) }

cc @msau42 since this is called from validateStorageNodeAffinityAnnotation as well

liggitt · 2018-04-11T04:47:48Z

pkg/apis/core/validation/validation.go

-	if len(term.MatchExpressions) == 0 {
-		return append(allErrs, field.Required(fldPath.Child("matchExpressions"), "must have at least one node selector requirement"))
+	if len(term.MatchExpressions) == 0 && len(term.MatchFields) == 0 {
+		return append(allErrs, field.Required(fldPath.Child("matchExpressions/matchFields"),


this isn't a valid field path... what we typically do in situations like this is put the required error on the parent struct with a message. for example:

if numVolumes == 0 { allErrs = append(allErrs, field.Required(fldPath, "must specify a volume type")) }

liggitt · 2018-04-11T04:49:45Z

pkg/apis/core/validation/validation.go

 	for j, req := range term.MatchExpressions {
 		allErrs = append(allErrs, ValidateNodeSelectorRequirement(req, fldPath.Child("matchExpressions").Index(j))...)
 	}
+
+	for j, req := range term.MatchFields {
+		allErrs = append(allErrs, ValidateNodeSelectorRequirement(req, fldPath.Child("matchFields").Index(j))...)


is the plan to allow the API to be expressive, and just result in errors by the scheduler or the kubelet if unsupported keys or operations are expressed?

prefer to let scheduler/kubelet the handle the specific key; as fields are supported by k8s version, e.g. metadata.name in 1.11 :)

Is the validation done in ValidateNodeSelectorRequirement valid for keys/values that are not limited to valid label keys/values (given these MatchField terms are referencing non-label field names and values)

Other field references in the API (like the downward API) validate specific field reference names. Are we sure we want to allow arbitrarily complex expressions here, when the intent is just to support metadata.name=foo? We can't reasonably tighten validation on this field in the future, so just calling it out here.

liggitt · 2018-04-13T17:28:48Z

pkg/apis/core/types.go

@@ -2099,10 +2099,14 @@ type NodeSelector struct {
 	NodeSelectorTerms []NodeSelectorTerm
 }

-// A null or empty node selector term matches no objects.
+// A null or empty node selector term matches no objects. At least one of


remove At least one of 'MatchExpressions' and 'MatchFields' must be specified, since that conflicts with A null or empty node selector term matches no objects., and we want a 1.10 server to tolerate a NodeSelectorTerm containing no MatchExpressions field.

liggitt · 2018-04-13T17:29:46Z

pkg/apis/core/validation/validation.go

@@ -3008,12 +3008,19 @@ func ValidateNodeSelectorRequirement(rq core.NodeSelectorRequirement, fldPath *f
 func ValidateNodeSelectorTerm(term core.NodeSelectorTerm, fldPath *field.Path) field.ErrorList {
 	allErrs := field.ErrorList{}

-	if len(term.MatchExpressions) == 0 {
-		return append(allErrs, field.Required(fldPath.Child("matchExpressions"), "must have at least one node selector requirement"))
+	if len(term.MatchExpressions) == 0 && len(term.MatchFields) == 0 {


I would actually remove this requirement completely... an empty term matches no objects, as documented. Stick to validating MatchExpressions or MatchFields if they provide them

sure, will handle it in #62448

Automatic merge from submit-queue (batch tested with PRs 62448, 59317, 59947, 62418, 62352). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Removed no-empty validation of nodeSelectorTerm.matchExpressions. Signed-off-by: Da K. Ma <klaus1982.cn@gmail.com> **Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*: part of #62002 **Release note**: ```release-note Pod affinity `nodeSelectorTerm.matchExpressions` may now be empty, and works as previously documented: nil or empty `matchExpressions` matches no objects in scheduler. ```

k82cn · 2018-04-19T13:28:29Z

@liggitt , PTAL :)

bsalamat · 2018-04-20T18:10:54Z

@k82cn @liggitt Could you guys please ensure that this is merged before KubeCon? Otherwise, we may not be able to move priority and preemption to Beta in 1.11.

bsalamat · 2018-04-20T18:45:32Z

staging/src/k8s.io/api/core/v1/types.go

 type NodeSelectorTerm struct {
-	//Required. A list of node selector requirements. The requirements are ANDed.
+	// A list of node selector requirements by node's labels.


Please add // +optional to both fields.

and omitempty

bsalamat · 2018-04-20T18:48:08Z

pkg/apis/core/v1/helper/helpers.go

+		labelSelector, err := NodeSelectorRequirementsAsSelector(req.MatchExpressions)
+		if err != nil {
+			glog.V(10).Infof("Failed to parse MatchExpressions: %+v,regarding as not match.", req.MatchExpressions)
+			continue


Why do we continue here? Shouldn't we return false instead?

because the terms are ORed, a failure in one should not short circuit

It should be ORed between terms, and ANDed within a terms (matchExpressions and matchFields).

bsalamat · 2018-04-20T18:49:26Z

pkg/apis/core/v1/helper/helpers.go

+		fieldSelector, err := NodeFieldSelectorRequirementsAsSelector(req.MatchFields)
+		if err != nil {
+			glog.V(10).Infof("Failed to parse MatcheFields: %+v,regarding as not match.", req.MatchFields)
+			continue


Same as above. Shouldn't we return false?

bsalamat · 2018-04-20T18:54:01Z

pkg/apis/core/v1/helper/helpers.go

+	return selector, nil
+}
+
+// MatchNodeSelectorTerms checks whether the node labels and fields match node selector terms in ORed;


In most of our code base (or maybe all of it), we AND terms when there are multiple of them. Why are we ORing here?

that was what was designed and documented previously, so for compatibility.

the reason that design makes sense is that each NodeSelectorTerm can already express everything that ANDing together multiple terms could.

the only reason to have a list of multiple terms is to let you select distinct sets of nodes matching non-overlapping selectors, which requires ORing the terms together.

liggitt · 2018-04-20T19:08:24Z

staging/src/k8s.io/apimachinery/pkg/labels/selector.go

+//     of characters. See validateLabelKey for more details.
+//
+// The empty string is a valid value in the input values set.
+func NewFieldRequirement(key string, op selection.Operator, vals []string) (*Requirement, error) {


pkg/fields/selector.go contains field-selector related things... I didn't really expect this added to the labels package

Moved to fields/selector.go; but for now, we only support equals and not equals for matchFields which is enough for scheduling DaemoSet pods by default scheduler. For the other advance features, e.g. IN/NotIN for a set, will be handled in a separate issue. If any comments/suggestion please let me know.

liggitt · 2018-04-23T18:57:36Z

staging/src/k8s.io/apimachinery/pkg/labels/selector.go

@@ -82,19 +83,19 @@ func NewSelector() Selector {

 type internalSelector []Requirement

-func (s internalSelector) DeepCopy() internalSelector {


revert this bit?

+1. Let's not add unnecessary changes to the PR.

liggitt · 2018-04-23T19:02:17Z

staging/src/k8s.io/apimachinery/pkg/fields/selector.go

+
+func (n nothingSelector) Matches(_ Fields) bool                                      { return false }
+func (n nothingSelector) Empty() bool                                                { return false }
+func (n nothingSelector) String() string                                             { return "" }


is the expectation that ParseSelector(s.String()) would return a selector with equivalent behavior? If so, this doesn't hold

I guess labels.Nothing() has the same issue... worth noting that the Nothing() selector is not appropriate for serializing to string for persistence and later parsing

liggitt · 2018-04-23T19:08:52Z

a couple nits, lgtm otherwise

will leave to @kubernetes/sig-scheduling-api-reviews for final LGTM

/approve

bsalamat

Just one nit and then LGTM

bsalamat · 2018-04-23T23:02:35Z

staging/src/k8s.io/apimachinery/pkg/labels/selector.go

@@ -82,19 +83,19 @@ func NewSelector() Selector {

 type internalSelector []Requirement

-func (s internalSelector) DeepCopy() internalSelector {


+1. Let's not add unnecessary changes to the PR.

Signed-off-by: Da K. Ma <klaus1982.cn@gmail.com>

liggitt · 2018-04-24T01:00:25Z

/lgtm

k8s-ci-robot · 2018-04-24T01:01:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: k82cn, liggitt

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~api/OWNERS~~ [liggitt]
~~docs/api-reference/OWNERS~~ [liggitt]
~~pkg/apis/core/OWNERS~~ [liggitt]
~~staging/src/k8s.io/api/OWNERS~~ [liggitt]
~~staging/src/k8s.io/apimachinery/pkg/OWNERS~~ [liggitt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-github-robot · 2018-04-24T05:45:25Z

Automatic merge from submit-queue (batch tested with PRs 62495, 63003, 62829, 62151, 62002). If you want to cherry-pick this change to another branch, please follow the instructions here.

janetkuo · 2018-05-14T20:00:45Z

This seems different from what's discussed in #61410 (just having a specific term for nodeName). Any reason why matchFields is added instead?

k82cn · 2018-05-15T02:08:19Z

here's suggestion: #61410 (comment) . And current term are label based: 1.) the validation for field maybe different with label's, 2.) the name of 'specific term' maybe conflict with existed label.

rhuss · 2019-02-02T07:08:10Z

pkg/apis/core/v1/helper/helpers_test.go

+						MatchFields: []v1.NodeSelectorRequirement{{
+							Key:      "metadata.name",
+							Operator: v1.NodeSelectorOpIn,
+							Values:   []string{"host_1, host_2"},


that test confused me a bit as I guess its still an array with only 1 element ? (guess the test is supposed to check that only one element is allowed by providing something like []string{"host1", "host2"} instead.

Sorry, to be picky, but I ended up here as I found no other real documentation for the "machtFields" selector (and I think the official documentation at https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.11/#nodeselectorterm-v1-core is a bit misleading as it does not mention that only 'In' and 'NotIn' are allowed, with only one value allowed in the list of terms.

@rhuss This PR was merged in April 2018. It is too late to leave review comments. If you have ideas to improve or fix the code, please go ahead and send a PR.

rhuss · 2019-02-02T07:16:42Z

pkg/apis/core/validation/validation.go

+				"must be only one value when `operator` is 'In' or 'NotIn' for node field selector"))
+		}
+	default:
+		allErrs = append(allErrs, field.Invalid(fldPath.Child("operator"), req.Operator, "not a valid selector operator"))


would change to 'not a valid node field selector operator' to make clear which kind of selector is meant.

rhuss · 2019-02-02T07:17:38Z

pkg/apis/core/validation/validation.go

+	}
+
+	if vf, found := nodeFieldSelectorValidators[req.Key]; !found {
+		allErrs = append(allErrs, field.Invalid(fldPath.Child("key"), req.Key, "not a valid field selector key"))


"node field selector" to harmonize with the other error messages.

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 2, 2018

k8s-ci-robot requested review from davidopp, derekwaynecarr, jsafrane, mwielgus and pmorie April 2, 2018 03:03

k8s-github-robot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Apr 2, 2018

k8s-ci-robot requested review from liggitt and smarterclayton April 2, 2018 12:32

liggitt reviewed Apr 4, 2018

View reviewed changes

k82cn force-pushed the k8s_61410_1 branch from 8e3b9f2 to 1506045 Compare April 4, 2018 08:51

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 4, 2018

k8s-ci-robot added the sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. label Apr 10, 2018

k82cn force-pushed the k8s_61410_1 branch from 1506045 to f18cf7c Compare April 11, 2018 03:22

liggitt reviewed Apr 11, 2018

View reviewed changes

kubernetes deleted a comment from liggitt Apr 12, 2018

k82cn mentioned this pull request Apr 12, 2018

Removed no-empty validation of nodeSelectorTerm.matchExpressions. #62448

Merged

liggitt reviewed Apr 13, 2018

View reviewed changes

k82cn force-pushed the k8s_61410_1 branch from f18cf7c to 6cb4d44 Compare April 17, 2018 02:50

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 19, 2018

k82cn mentioned this pull request Apr 19, 2018

Schedule DaemonSet Pods by default scheduler #59194

Closed

7 tasks

bsalamat reviewed Apr 20, 2018

View reviewed changes

liggitt reviewed Apr 20, 2018

View reviewed changes

k82cn force-pushed the k8s_61410_1 branch 3 times, most recently from 586bb2b to 2cc53dc Compare April 21, 2018 07:48

liggitt reviewed Apr 23, 2018

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 23, 2018

bsalamat reviewed Apr 23, 2018

View reviewed changes

k82cn added 2 commits April 24, 2018 08:54

Added MatchFields to NodeSelectorTerm.

8bfc994

Signed-off-by: Da K. Ma <klaus1982.cn@gmail.com>

generated codes.

b80a01b

Signed-off-by: Da K. Ma <klaus1982.cn@gmail.com>

k82cn force-pushed the k8s_61410_1 branch from 2cc53dc to b80a01b Compare April 24, 2018 00:55

k8s-ci-robot assigned liggitt Apr 24, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 24, 2018

k8s-github-robot merged commit 8f20a81 into kubernetes:master Apr 24, 2018

k82cn deleted the k8s_61410_1 branch April 24, 2018 05:47

Huang-Wei mentioned this pull request Aug 28, 2018

Graduate Schedule daemon set pods by default scheduler to beta and fix tests #67899

Merged

rhuss reviewed Feb 2, 2019

View reviewed changes

		@@ -82,19 +83,19 @@ func NewSelector() Selector {

		type internalSelector []Requirement

		func (s internalSelector) DeepCopy() internalSelector {

Added MatchFields to NodeSelectorTerm #62002

Added MatchFields to NodeSelectorTerm #62002

Conversation

k82cn commented Apr 2, 2018

k82cn commented Apr 2, 2018

k82cn commented Apr 2, 2018

k82cn commented Apr 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k82cn commented Apr 9, 2018

k82cn commented Apr 10, 2018

k82cn commented Apr 11, 2018

Choose a reason for hiding this comment

k82cn Apr 12, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt Apr 13, 2018 • edited

Choose a reason for hiding this comment

liggitt Apr 13, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k82cn commented Apr 19, 2018

bsalamat commented Apr 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k82cn Apr 21, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented Apr 23, 2018 • edited

bsalamat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented Apr 24, 2018

k8s-ci-robot commented Apr 24, 2018

k8s-github-robot commented Apr 24, 2018

janetkuo commented May 14, 2018

k82cn commented May 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k82cn Apr 12, 2018 •

edited

liggitt Apr 13, 2018 •

edited

liggitt Apr 13, 2018 •

edited

k82cn Apr 21, 2018 •

edited

liggitt commented Apr 23, 2018 •

edited