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
kube-scheduler: add taints filtering logic consistent with TaintToleration plugin for PodTopologySpread plugin #112357
Conversation
@SataQiu: This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The 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. |
…ation plugin for PodTopologySpread plugin Signed-off-by: SataQiu <shidaqiu2018@gmail.com>
9b4d5de
to
bc9c494
Compare
/cc @kerthcet |
@@ -52,7 +52,7 @@ func (tsc *topologySpreadConstraint) matchNodeInclusionPolicies(pod *v1.Pod, nod | |||
} | |||
|
|||
if tsc.NodeTaintsPolicy == v1.NodeInclusionPolicyHonor { | |||
if _, untolerated := v1helper.FindMatchingUntoleratedTaint(node.Spec.Taints, pod.Spec.Tolerations, nil); untolerated { | |||
if _, untolerated := v1helper.FindMatchingUntoleratedTaint(node.Spec.Taints, pod.Spec.Tolerations, helper.DoNotScheduleTaintsFilterFunc()); untolerated { |
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, it's reasonable, we should filter out PreferNoSchedule
ones. But we need to add more tests to cover this, maybe in TestPreFilterStateAddPod
, TestPreFilterState
and TestPodTopologySpreadScore
, not too much, but we should cover them.
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.
make sense
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 change makes sense, but I am looking at the usage of the function matchNodeInclusionPolicies
; in
!constraint.matchNodeInclusionPolicies(updatedPod, node, requiredSchedulingTerm) { |
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.
It's a big mistake made by me before, I'll fix it ASAP.
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.
It's a big mistake made by me before, I'll fix it ASAP.
Hi @kerthcet
I'd like to cherry-pick it to 1.25, then you can further fix the related problems based on this.
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.
Already fixed, see #112507, I should broadcast it. Sorry for that.
@ahg-g for more review about this. |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahg-g, SataQiu 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 |
please send a patch for 1.25 |
The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass. This bot retests PRs for certain kubernetes repos according to the following rules:
You can:
/retest |
…357-upstream-release-1.25 Automated cherry pick of #112357: kube-scheduler: add taints filtering logic consistent with
What type of PR is this?
/kind bug
/kind cleanup
What this PR does / why we need it:
kube-scheduler: add taints filtering logic consistent with TaintToleration plugin for PodTopologySpread plugin
helper
packageWhich issue(s) this PR fixes:
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: