parse pod's node affinity once in preFilter #99213
Conversation
k8s-ci-robot
added
size/M
|
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
k8s-ci-robot
added
cncf-cla: no
|
Welcome @AliceZhang2016! |
k8s-ci-robot
added
needs-triage
|
Hi @AliceZhang2016. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
k8s-ci-robot
added
sig/scheduling
| } | ||
|
|
||
| // Clone implements the mandatory Clone interface. We don't really copy the data since | ||
| // there is not need for that |
There was a problem hiding this comment.
We don't deep copy the data since values never get overridden
| return false | ||
| } | ||
| return true | ||
| } | ||
|
|
||
| // nodeMatchesNodeSelector checks if a node's labels satisfy a list of node selector terms, | ||
| // terms are ORed, and an empty list of terms will match nothing. | ||
| func nodeMatchesNodeSelector(node *v1.Node, nodeSelector *v1.NodeSelector) bool { | ||
| func NodeMatchesNodeSelector(node *v1.Node, nodeSelector *v1.NodeSelector) bool { |
There was a problem hiding this comment.
rather, we should be able to get rid of this function now. And perhaps the entire file.
| } | ||
| var affinity *v1.NodeAffinity | ||
| if pod.Spec.Affinity != nil { | ||
| affinity = pod.Spec.Affinity.NodeAffinity |
There was a problem hiding this comment.
use nodeaffinity.NewLazyErrorNodeSelector to store the parsed affinity.
and perhaps return a preScoreState object directly
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
| return s | ||
| } | ||
|
|
||
| // PreFilter builds and writes cycle state used by Filter |
There was a problem hiding this comment.
| // PreFilter builds and writes cycle state used by Filter | |
| // PreFilter builds and writes cycle state used by Filter. |
Make comment as a sentence. Referring to Comment sentences
| if s.requiredNodeSelector != nil { | ||
| if !s.requiredNodeSelector.Matches(labels.Set(node.Labels)) { | ||
| flagFilter = false | ||
| } |
There was a problem hiding this comment.
use ./hack/update-gofmt.sh to format the files.
| } | ||
| } | ||
| if s.requiredNodeAffinity != nil { | ||
| if s.requiredNodeAffinity.RequiredDuringSchedulingIgnoredDuringExecution != nil && !pluginhelper.NodeMatchesNodeSelector(node, s.requiredNodeAffinity.RequiredDuringSchedulingIgnoredDuringExecution) { |
| if err != nil { | ||
| // fallback to calculate requiredNodeSelector and requiredNodeAffinity | ||
| // here when PreFilter is disabled | ||
| requiredNodeSelector, requiredNodeAffinity, err := getPodRequiredNodeSelectorAndAffinity(pod) |
There was a problem hiding this comment.
requiredNodeSelector, requiredNodeAffinity, err := getPodRequiredNodeSelectorAndAffinity(pod)
if err != nil {
return framework.AsStatus(err)
}
state := &preFilterState{
requiredNodeSelector: requiredNodeSelector,
requiredNodeAffinity: requiredNodeAffinity,
}
abstract a func to reduce duplicated code?
| @@ -68,9 +97,39 @@ func (pl *NodeAffinity) Filter(ctx context.Context, state *framework.CycleState, | |||
| if pl.addedNodeSelector != nil && !pl.addedNodeSelector.Match(node) { | |||
| return framework.NewStatus(framework.UnschedulableAndUnresolvable, errReasonEnforced) | |||
| } | |||
| /* | |||
| if !pluginhelper.PodMatchesNodeSelectorAndAffinityTerms(pod, node) { | |||
There was a problem hiding this comment.
There are several other components depending on pluginhelper.PodMatchesNodeSelectorAndAffinityTerms. This change duplicates similar logic with the function, is it possible that we refactor helper functions to still share codes with other components?
k8s-ci-robot
added
size/L
| } | ||
| if flagFilter { | ||
| if s.requiredNodeAffinity != nil { | ||
| flagFilter, _ = s.requiredNodeAffinity.Match(node) |
There was a problem hiding this comment.
Add a comment for the ignored error:
// Ignoring parsing errors for backwards compatibility.
| } | ||
|
|
||
| var flagFilter bool = true | ||
| if s != nil { |
There was a problem hiding this comment.
this check is unnecessary.
| pod.Spec.Affinity.NodeAffinity.RequiredDuringSchedulingIgnoredDuringExecution != nil { | ||
| affinity = nodeaffinity.NewLazyErrorNodeSelector(pod.Spec.Affinity.NodeAffinity.RequiredDuringSchedulingIgnoredDuringExecution) | ||
| } | ||
| return &preFilterState{requiredNodeSelector: selector, requiredNodeAffinity: affinity}, nil |
There was a problem hiding this comment.
remove the error returned, as it is always nil
| if pod.Spec.Affinity != nil && | ||
| pod.Spec.Affinity.NodeAffinity != nil && | ||
| pod.Spec.Affinity.NodeAffinity.RequiredDuringSchedulingIgnoredDuringExecution != nil { | ||
| affinity = nodeaffinity.NewLazyErrorNodeSelector(pod.Spec.Affinity.NodeAffinity.RequiredDuringSchedulingIgnoredDuringExecution) |
There was a problem hiding this comment.
add a comment:
Using LazyErrorNodeSelector for backwards compatibility of parsing errors
| if s != nil { | ||
| if s.requiredNodeSelector != nil { | ||
| if !s.requiredNodeSelector.Matches(labels.Set(node.Labels)) { | ||
| flagFilter = false |
There was a problem hiding this comment.
it's simpler if you just return UnschedulableAndUnresolvable here.
| labels: map[string]string{ | ||
| "GPU": "NVIDIA-GRID-K1", | ||
| }, | ||
| name: "Matches node affinity correctly even if PreFilter is not called", |
There was a problem hiding this comment.
Let's move the name of the test to the beginning of the struct. Please fix the existing tests too.
| "baz": "blah", | ||
| }, | ||
| name: "Matches node selector correctly even if PreFilter is not called", | ||
| disablePreFilter: true, |
There was a problem hiding this comment.
it looks like this variable is not being used.
| cycleState.Write(preFilterStateKey, state) | ||
| return nil | ||
| } | ||
|
|
||
| // PreFilterExtensions does not exist for this plugin. |
There was a problem hiding this comment.
// PreFilterExtensions not necessary for this plugin as state doesn't depend on nodes
| name: "no selector", | ||
| pod: &v1.Pod{}, | ||
|
|
| state := framework.NewCycleState() | ||
| var gotStatus *framework.Status | ||
| if !test.disablePreFilter { | ||
| _ = p.(framework.PreFilterPlugin).PreFilter(context.Background(), state, test.pod) |
There was a problem hiding this comment.
don't discard the error. Call t.Errorf
|
I signed it |
k8s-ci-robot
added
cncf-cla: yes
k8s-ci-robot
removed
the
sig/testing
AliceZhang2016
force-pushed
the
affinity
branch
from
8ec023c
to
4fb8e34
Compare
k8s-ci-robot
added
area/test
sig/testing
|
/retest |
|
/test pull-kubernetes-e2e-kind |
|
/test pull-kubernetes-e2e-gce-ubuntu-containerd |
|
/kind feature |
k8s-ci-robot
added
kind/feature
|
/retest |
|
/remove-area test |
k8s-ci-robot
removed
area/test
sig/testing
k8s-ci-robot
added
the
lgtm
|
@AliceZhang2016: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
/hold |
k8s-ci-robot
added
the
do-not-merge/hold
|
/retest |
k8s-ci-robot
added
release-note
|
/hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
What type of PR is this?
/kind feature
What this PR does / why we need it:
scheduler: parse Pod's Node affinity once in PreFilter phase
Which issue(s) this PR fixes:
Fixes #96164
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: