Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Return Skip in InterPodAffinity#PreScore under specific conditions #117794

Merged
merged 1 commit into from
May 10, 2023
Merged

Return Skip in InterPodAffinity#PreScore under specific conditions #117794

merged 1 commit into from
May 10, 2023

Conversation

utam0k
Copy link
Member

@utam0k utam0k commented May 5, 2023 •
edited

What type of PR is this?

/kind feature

What this PR does / why we need it:

This commit updates the InterPodAffinity PreScore to return a Skip status when the following conditions are met:

  1. There are no nodes to score.
  2. The incoming pod has no inter-pod affinities && the IgnorePreferredTermsOfExistingPods option is enabled.

Which issue(s) this PR fixes:

This change is a part of the work for issue #115745

Special notes for your reviewer:

Does this PR introduce a user-facing change?

The scheduler skips the InterPodAffinity Score plugin when nothing to do with the Pod.
It will affect some metrics values related to the InterPodAffinity Score plugin.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

NONE

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels May 5, 2023
@k8s-ci-robot
Copy link
Contributor

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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 k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 5, 2023
@k8s-ci-robot
Copy link
Contributor

Hi @utam0k. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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 k8s-ci-robot added needs-priority Indicates a PR lacks a `priority/foo` label and requires one. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels May 5, 2023
@utam0k
Copy link
Member Author

utam0k commented May 5, 2023

It might be beneficial for @sanposhiho to review the issue since he is the author of this issue.

@kerthcet
Copy link
Member

kerthcet commented May 5, 2023

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 5, 2023
@sanposhiho
Copy link
Member

/assign

@utam0k
Copy link
Member Author

utam0k commented May 5, 2023

/retest

1 similar comment
@utam0k
Copy link
Member Author

utam0k commented May 5, 2023

/retest

kerthcet
kerthcet reviewed May 5, 2023
View reviewed changes
Copy link
Member

@kerthcet kerthcet left a comment

Choose a reason for hiding this comment

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

LGTM, leave for @sanposhiho

@Vivekgaddigi
Copy link

LGTM

sanposhiho
sanposhiho reviewed May 6, 2023
View reviewed changes
Copy link
Member

@sanposhiho sanposhiho left a comment

Choose a reason for hiding this comment

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

only one nit

pkg/scheduler/framework/plugins/interpodaffinity/scoring_test.go Outdated Show resolved Hide resolved
@sanposhiho
Copy link
Member

sanposhiho commented May 6, 2023 •
edited

@utam0k Also, please help to change the release note:

The scheduler skips the InterPodAffinity Score plugin when nothing to do with the Pod.
It will affect some metrics values related to the InterPodAffinity Score plugin.

(simplify the description + add the note about metric values)

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 6, 2023
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 6, 2023
sanposhiho
sanposhiho reviewed May 6, 2023
View reviewed changes
Copy link
Member

@sanposhiho sanposhiho left a comment

Choose a reason for hiding this comment

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

Please help other refactor along with that

pkg/scheduler/framework/plugins/interpodaffinity/scoring_test.go Outdated Show resolved Hide resolved
pkg/scheduler/framework/plugins/interpodaffinity/scoring_test.go Outdated Show resolved Hide resolved
sanposhiho
sanposhiho reviewed May 6, 2023
View reviewed changes
Copy link
Member

@sanposhiho sanposhiho left a comment

Choose a reason for hiding this comment

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

Please help other refactor along with that

@utam0k
Copy link
Member Author

utam0k commented May 6, 2023 •
edited

Please help other refactor along with that

@sanposhiho
Sure. I thought this refactoring you suggested was necessary. PTAL

@utam0k
Copy link
Member Author

utam0k commented May 6, 2023

/retest

sanposhiho
sanposhiho approved these changes May 6, 2023
View reviewed changes
Copy link
Member

@sanposhiho sanposhiho left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 6, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 440ab3a920299a0a754b6332edbf7d27cae58956

@utam0k utam0k requested a review from kerthcet May 7, 2023 06:12
@kerthcet
Copy link
Member

kerthcet commented May 8, 2023

/assign @alculquicondor

@utam0k
Return Skip in InterPodAffinity#PreScore under specific conditions
c0611b6 
This commit updates the InterPodAffinity PreScore to return a Skip status when the following conditions are met:
1. There are no nodes to score.
2. The incoming pod has no inter-pod affinities && the `IgnorePreferredTermsOfExistingPods` option is enabled.

Signed-off-by: utam0k <[email protected]>
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 10, 2023
@utam0k utam0k requested a review from Vivekgaddigi May 10, 2023 13:03
@utam0k
Copy link
Member Author

utam0k commented May 10, 2023

@alculquicondor Thank you for taking the time to review this. Please take another look 🙏

@alculquicondor
Copy link
Member

/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 10, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 4138423bb702d740de1f1ad8fe3af65695e2f03d

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, sanposhiho, utam0k, Vivekgaddigi

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 10, 2023
@k8s-ci-robot k8s-ci-robot merged commit 7aea5a9 into kubernetes:master May 10, 2023
12 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.28 milestone May 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chendave chendave Awaiting requested review from chendave

@kerthcet kerthcet Awaiting requested review from kerthcet

@alculquicondor alculquicondor Awaiting requested review from alculquicondor

@sanposhiho sanposhiho Awaiting requested review from sanposhiho

@Vivekgaddigi Vivekgaddigi Awaiting requested review from Vivekgaddigi

Assignees

@alculquicondor alculquicondor

@sanposhiho sanposhiho

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.28
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@utam0k @k8s-ci-robot @kerthcet @sanposhiho @Vivekgaddigi @alculquicondor