Added MatchFields to NodeSelectorTerm by k82cn · Pull Request #62002 · kubernetes/kubernetes

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

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

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

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

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

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

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. ```

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

Please add // +optional to both fields.

and omitempty

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

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

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

Same as above. Shouldn't we return false?

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

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

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

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

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

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

+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

Details

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

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

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

Copy link

Member

bsalamat Apr 23, 2018

Choose a reason for hiding this comment

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

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

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

scheduler-tester mentioned this pull request Aug 3, 2024

Inconsistency between the code and the doc on how matchFields works #126531

Closed

odeinjul mentioned this pull request Sep 16, 2024

Missing information #127379

Closed

scheduler-tester mentioned this pull request Sep 16, 2024

Missing details in nodeAffinity's API specification #127401

Closed

Conversation

k82cn commented Apr 2, 2018

Uh oh!

k82cn commented Apr 2, 2018

Uh oh!

k82cn commented Apr 2, 2018

Uh oh!

k82cn commented Apr 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k82cn commented Apr 9, 2018

Uh oh!

k82cn commented Apr 10, 2018

Uh oh!

k82cn commented Apr 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k82cn Apr 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liggitt Apr 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liggitt Apr 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bsalamat commented Apr 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k82cn Apr 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

k82cn Apr 12, 2018 •

edited

Loading

liggitt Apr 13, 2018 •

edited

Loading

liggitt Apr 13, 2018 •

edited

Loading

k82cn Apr 21, 2018 •

edited

Loading

liggitt commented Apr 23, 2018 •

edited

Loading