Option to ignore existing pods' preferred inter-pod affinities if the incoming pod has no preferred inter-pod affinities #114393

danielvegamyhre · 2022-12-09T20:09:34Z

What type of PR is this?
/kind api-change
/sig scheduling
/sig api-machinery
/priority backlog

What this PR does / why we need it:
The purpose of these changes are to add an option for the scheduler to ignore existing pods` preferred inter-pod affinities if the incoming pod has no preferred inter-pod affinities.

Preferred inter-pod affinity/anti-affinity rules tend to be made in both directions (i.e. if pod type A has a preferred affinity for pod type B, then pod type B will usually have a preferred affinity for pod type A). If an incoming pod that needs to be scheduled has no preferred inter-pod affinity rules, it is most likely the case that no existing pods have no preferred inter-pod affinity rules relating to it.

Therefore, the linear scan through existing pods checking their preferred inter-pod affinities is usually a waste of time that the user can choose to optimize away by enabling this option in the InterPodAffinity plugin config args (at the cost of an occasional pod being scheduled non-optimally/violating existing pods affinity rules).

Specifically, this option will cause PreScore to exit early if this option is enabled and the incoming pod has no existing inter-pod affinity rules.

Which issue(s) this PR fixes:
Fixes #114267

Does this PR introduce a user-facing change?

Added new option to the InterPodAffinity scheduler plugin to ignore existing pods` preferred inter-pod affinities if the incoming pod has no preferred inter-pod affinities. This option can be used as an optimization for higher scheduling throughput (at the cost of an occasional pod being scheduled non-optimally/violating existing pods' preferred inter-pod affinities). To enable this scheduler option, set the InterPodAffinity scheduler plugin arg "ignorePreferredTermsOfExistingPods: true".

k8s-ci-robot · 2022-12-09T20:09:42Z

Welcome @danielvegamyhre!

It looks like this is your first PR to kubernetes/kubernetes 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kubernetes has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2022-12-09T20:09:43Z

Hi @danielvegamyhre. 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-triage-robot · 2022-12-09T20:52:21Z

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

ahg-g · 2022-12-10T00:35:53Z

/assign
/ok-to-test

danielvegamyhre · 2022-12-28T18:26:32Z

@ahg-g any chance you could take a quick look at this when you have a moment? I would really appreciate it.

pkg/scheduler/apis/config/types.go

pkg/scheduler/apis/config/types_pluginargs.go

pkg/scheduler/framework/plugins/interpodaffinity/scoring_test.go

danielvegamyhre · 2022-12-31T00:17:52Z

Thanks for the review @ahg-g I have addressed all your comments, this is ready for another look. The only outstanding issue is in the test case I added to options_test.go. For some reason, despite the test case YAML settingignorePreferredTermsOfExistingPods: true, the resulting scheduler config has it set as false. Any idea how this might be possible?

I have tried both bool and *bool data types (based on other tests I saw using `*bool), as well as read through the YAML -> KubeSchedulerConfiguration code looking for where this might be occurring, but no luck so far.

ahg-g · 2023-01-03T20:55:23Z

You need to update the auto-generated code that does the translation between the external and internal types. Run ./hack/update-codegen.sh to do that.

ahg-g · 2023-01-03T21:52:56Z

pkg/scheduler/apis/config/scheme/scheme_test.go

@@ -1186,6 +1186,7 @@ profiles:
  - args:
      apiVersion: kubescheduler.config.k8s.io/v1beta2
      hardPodAffinityWeight: 5
+      ignorePreferredTermsOfExistingPods: false


worth adding a test to the scheme tests that sets it to true.

Ok I added test cases for both scheme encoding/decoding tests when ignorePreferredTermsOfExistingPods is true.

ahg-g · 2023-01-03T21:54:22Z

pkg/scheduler/framework/plugins/interpodaffinity/scoring.go

+		cycleState.Write(preScoreStateKey, &preScoreState{
+			topologyScore: make(map[string]map[string]int64),
+		})
+		return nil


@alculquicondor @Huang-Wei are we going to allow returning Skip for PreScore extensions to indicate that Score shouldn't run for the same plugin?

that sounds like a better contract, compared to plumbing the placeholder and checking if it's empty in Score.

yes, we should have support for Skip in Score as well, but the work didn't start yet.

ahg-g · 2023-01-03T21:54:57Z

/retitle "Option to ignore existing pods' inter-pod affinities if the incoming pod has no inter-pod affinities"

cici37 · 2023-01-11T22:18:06Z

/cc
Curious about the community requirement on adding field directly into stable API :)
Is that allowed because scheduler API is internal?

ahg-g · 2023-01-11T22:20:27Z

Curious about the community requirement on adding field directly into stable API :) Is that allowed because scheduler API is internal?

Adding fields is allowed, removing them requires a new version.

alculquicondor · 2023-01-12T13:31:42Z

Maybe the question is why no KEP? Since the component config API is not stored, we don't need feature gates or other things that a KEP imposes.

btw

/remove-sig api-machinery

this has nothing to do.

liggitt · 2023-01-12T15:12:17Z

pkg/scheduler/apis/config/types_pluginargs.go

+
+	// IgnorePreferredTermsOfExistingPods configures the scheduler to ignore existing pods' preferred affinity
+	// rules when scoring candidate nodes, unless the incoming pod has inter-pod affinities.
+	IgnorePreferredTermsOfExistingPods bool


the "unless incoming pod..." aspect is not captured in the name... would we eventually want to also allow unconditionally ignoring preferred terms of existing pods? if so, it would be worth thinking about how we would name this field and that eventual field in ways that wouldn't be confusing

Based on my understanding, unconditionally ignoring preferred terms of existing pods is not something we'll want to do in the future. However, @ahg-g has more knowledge about future plans in this area so I will defer to his recommendation here.

Right, I don't think we will want to do that

liggitt · 2023-01-12T15:13:12Z

one question on the field name and future plans around this incoming/existing preferred term behavior

liggitt · 2023-01-13T13:39:29Z

/approve
for API bit

scheduling reviewer has lgtm

ahg-g · 2023-01-13T22:07:29Z

Please squash the commits to one

has inter-pod affinities

k8s-ci-robot · 2023-01-13T23:22:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danielvegamyhre, liggitt

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:

~~cmd/kube-scheduler/OWNERS~~ [liggitt]
~~pkg/generated/openapi/OWNERS~~ [liggitt]
~~pkg/scheduler/OWNERS~~ [liggitt]
~~staging/src/k8s.io/kube-scheduler/config/OWNERS~~ [liggitt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ahg-g · 2023-01-14T00:51:35Z

/lgtm

Thanks @danielvegamyhre

k8s-ci-robot · 2023-01-14T00:51:41Z

LGTM label has been added.

Git tree hash: 307acffe5710111b780cc246c25bce08a81b89b4

k8s-ci-robot added do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Dec 9, 2022

k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Dec 9, 2022

k8s-ci-robot requested review from chendave and denkensk December 9, 2022 20:10

k8s-ci-robot assigned ahg-g Dec 10, 2022

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 Dec 10, 2022

ahg-g reviewed Dec 30, 2022

View reviewed changes

pkg/scheduler/apis/config/types.go Outdated Show resolved Hide resolved

pkg/scheduler/apis/config/types_pluginargs.go Outdated Show resolved Hide resolved

pkg/scheduler/framework/plugins/interpodaffinity/scoring_test.go Outdated Show resolved Hide resolved

k8s-ci-robot added area/code-generation sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. labels Jan 3, 2023

ahg-g reviewed Jan 3, 2023

View reviewed changes

k8s-ci-robot added priority/backlog Higher priority than priority/awaiting-more-evidence. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jan 5, 2023

liggitt added this to Backlog in API Reviews Jan 6, 2023

k8s-ci-robot requested a review from cici37 January 11, 2023 22:18

k8s-ci-robot removed the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Jan 12, 2023

liggitt self-assigned this Jan 12, 2023

liggitt moved this from Backlog to Assigned in API Reviews Jan 12, 2023

liggitt reviewed Jan 12, 2023

View reviewed changes

liggitt moved this from Assigned to API review completed, 1.27 in API Reviews Jan 13, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 13, 2023

danielvegamyhre force-pushed the myfeature branch from fa1fee8 to 6c23b46 Compare January 13, 2023 22:18

k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Jan 13, 2023

optionally ignore preferred terms of existing pods unless incoming pod

41817b1

has inter-pod affinities

danielvegamyhre force-pushed the myfeature branch from 6c23b46 to 41817b1 Compare January 13, 2023 23:16

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 14, 2023

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jan 14, 2023

k8s-ci-robot merged commit 3a8e2e3 into kubernetes:master Jan 14, 2023

k8s-ci-robot added this to the v1.27 milestone Jan 14, 2023

danielvegamyhre mentioned this pull request Feb 2, 2023

REQUEST: New membership for @danielvegamyhre kubernetes/org#4000

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Option to ignore existing pods' preferred inter-pod affinities if the incoming pod has no preferred inter-pod affinities #114393

Option to ignore existing pods' preferred inter-pod affinities if the incoming pod has no preferred inter-pod affinities #114393

danielvegamyhre commented Dec 9, 2022 •

edited

k8s-ci-robot commented Dec 9, 2022

k8s-ci-robot commented Dec 9, 2022

k8s-triage-robot commented Dec 9, 2022

ahg-g commented Dec 10, 2022

danielvegamyhre commented Dec 28, 2022 •

edited

danielvegamyhre commented Dec 31, 2022

ahg-g commented Jan 3, 2023 •

edited

ahg-g Jan 3, 2023

danielvegamyhre Jan 4, 2023

ahg-g Jan 3, 2023

Huang-Wei Jan 4, 2023

alculquicondor Jan 4, 2023

ahg-g commented Jan 3, 2023

cici37 commented Jan 11, 2023

ahg-g commented Jan 11, 2023 •

edited

alculquicondor commented Jan 12, 2023

liggitt Jan 12, 2023

danielvegamyhre Jan 12, 2023

ahg-g Jan 13, 2023

liggitt commented Jan 12, 2023

liggitt commented Jan 13, 2023

ahg-g commented Jan 13, 2023

k8s-ci-robot commented Jan 13, 2023

ahg-g commented Jan 14, 2023

k8s-ci-robot commented Jan 14, 2023

Option to ignore existing pods' preferred inter-pod affinities if the incoming pod has no preferred inter-pod affinities #114393

Option to ignore existing pods' preferred inter-pod affinities if the incoming pod has no preferred inter-pod affinities #114393

Conversation

danielvegamyhre commented Dec 9, 2022 • edited

k8s-ci-robot commented Dec 9, 2022

k8s-ci-robot commented Dec 9, 2022

k8s-triage-robot commented Dec 9, 2022

ahg-g commented Dec 10, 2022

danielvegamyhre commented Dec 28, 2022 • edited

danielvegamyhre commented Dec 31, 2022

ahg-g commented Jan 3, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahg-g commented Jan 3, 2023

cici37 commented Jan 11, 2023

ahg-g commented Jan 11, 2023 • edited

alculquicondor commented Jan 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented Jan 12, 2023

liggitt commented Jan 13, 2023

ahg-g commented Jan 13, 2023

k8s-ci-robot commented Jan 13, 2023

ahg-g commented Jan 14, 2023

k8s-ci-robot commented Jan 14, 2023

danielvegamyhre commented Dec 9, 2022 •

edited

danielvegamyhre commented Dec 28, 2022 •

edited

ahg-g commented Jan 3, 2023 •

edited

ahg-g commented Jan 11, 2023 •

edited