-
Notifications
You must be signed in to change notification settings - Fork 39.2k
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
Supported nodeSelector.matchFields in scheduler. #62453
Conversation
b112be2
to
b95db49
Compare
/retest |
@kubernetes/sig-scheduling-pr-reviews |
} | ||
return false | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Remove blank line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM from scheduling side. I will wait for @bsalamat to comment as he has more context on this PR.
@@ -1387,11 +1388,66 @@ func TestPodFitsSelector(t *testing.T) { | |||
fits: false, | |||
test: "Pod with an invalid value in Affinity term won't be scheduled onto the node", | |||
}, | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please also add a test case with multiple selector terms? For example a case where there are multiple MatchExpressions
and MatchFields
terms?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done :)
pkg/scheduler/algorithm/types.go
Outdated
@@ -25,6 +25,12 @@ import ( | |||
"k8s.io/kubernetes/pkg/scheduler/schedulercache" | |||
) | |||
|
|||
// NodeFieldSelectorKeys is a map that: the key are node field selector keys; the value are |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/the value/the values/
}, | ||
nodeName: "node_2", | ||
fits: false, | ||
test: "Pod with matchFields using In operator that not matches the existing node", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/not matches/does not match/
} | ||
|
||
// The pod can only schedule onto nodes that satisfy requirements in both NodeAffinity and nodeSelector. | ||
func podMatchesNodeLabels(pod *v1.Pod, node *v1.Node) bool { | ||
func podMatchesNodeSelector(pod *v1.Pod, node *v1.Node) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rename it to podMatchesNodeSelectorAndAffinityTerms
to further distinguish it from the predicate function. Please update the comment and start it with the function name.
} | ||
|
||
// The pod can only schedule onto nodes that satisfy requirements in both NodeAffinity and nodeSelector. | ||
func podMatchesNodeLabels(pod *v1.Pod, node *v1.Node) bool { | ||
func podMatchesNodeSelector(pod *v1.Pod, node *v1.Node) bool { | ||
// Check if node.Labels match pod.Spec.NodeSelector. | ||
if len(pod.Spec.NodeSelector) > 0 { | ||
selector := labels.SelectorFromSet(pod.Spec.NodeSelector) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we call nodeMatchesNodeSelectorTerms
and pass the NodeSelector
terms of the pod? Am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nodeMatchesNodeSelectorTerms
only accept NodeSelectorTerm
, but we can only get Selector
here :(.
Signed-off-by: Da K. Ma <klaus1982.cn@gmail.com>
@ravisantoshgudimetla , PTAL :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @k82cn
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: k82cn, ravisantoshgudimetla 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:
Approvers can indicate their approval by writing |
/retest |
/test all [submit-queue is verifying that this PR is safe to merge] |
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here. |
|
||
// NodeFieldSelectorKeyNodeName ('metadata.name') uses this as node field selector key | ||
// when selecting node by node's name. | ||
NodeFieldSelectorKeyNodeName = api.ObjectNameField |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a label, so should not be placed in well_known_labels.go
.
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:
Release note: