fix ImageLocality plugin score is inconsistent #116938

olderTaoist · 2023-03-27T06:31:20Z

What type of PR is this?

/kind bug

the older manner is that update the NodeInfo.ImageStates field when receive add/update/delete event, but doesn't update

ImageStates of the exist NodeInfo when receive node add event, this manner cause inconsistent of ImageLocality plugin.

the new manner is that update the cache.imageStates field when receive add/update/delete event, update ImageStates

of the exist NodeInfo according to cache.imageStates when beginning to schedule pod(in UpdateSnapshot function)

Which issue(s) this PR fixes:

Fixes #116673

Does this PR introduce a user-facing change?

Fixed inconsistency in the calculation of number of nodes that have an image, which affect the scoring in the ImageLocality plugin

k8s-ci-robot · 2023-03-27T06:31:24Z

Please note that we're already in Test Freeze for the release-1.27 branch. This means every merged PR will be automatically fast-forwarded via the periodic ci-fast-forward job to the release branch of the upcoming v1.27.0 release.

Fast forwards are scheduled to happen every 6 hours, whereas the most recent run was: Mon Mar 27 04:26:09 UTC 2023.

linux-foundation-easycla · 2023-03-27T06:31:24Z

The committers listed above are authorized under a signed CLA.

✅ login: olderTaoist / name: walker (5d5958e)

k8s-ci-robot · 2023-03-27T06:31:29Z

Welcome @olderTaoist!

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 · 2023-03-27T06:31:29Z

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 · 2023-03-27T06:31:29Z

Hi @olderTaoist. 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.

olderTaoist · 2023-03-27T07:55:20Z

/cc PTAL @chendave @denkensk

k8s-ci-robot · 2023-03-27T07:55:22Z

@olderTaoist: GitHub didn't allow me to request PR reviews from the following users: PTAL.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @chendave @denkensk PTAL

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.

chendave · 2023-03-28T02:00:58Z

/assign

pkg/scheduler/internal/cache/cache.go

chendave · 2023-03-28T02:44:42Z

/ok-to-test

chendave

Another round of review, and FYI, the code freezing is approaching, I noticed you have pushed some additional commits but they are not shown up here.

chendave · 2023-07-13T13:53:29Z

pkg/scheduler/framework/types.go

@@ -433,8 +433,21 @@ func getNamespacesFromPodAffinityTerm(pod *v1.Pod, podAffinityTerm *v1.PodAffini
 type ImageStateSummary struct {
 	// Size of the image
 	Size int64
-	// Used to track how many nodes have this image
+	// Used to track how many nodes have this image, It is computed from the Nodes field below
+	// during the execution of Snapshot.


I am thinking this might cause the inconsistency for how it is defined and used, how about remove the NumNodes field and just use len(Nodes) for where it is used? while removing it means more changes will be needed, ping @alculquicondor @ahg-g @kerthcet for thoughts.

pkg/scheduler/internal/cache/cache_test.go

pkg/scheduler/framework/types.go

olderTaoist · 2023-09-03T02:31:26Z

Another round of review, and FYI, the code freezing is approaching, I noticed you have pushed some additional commits but they are not shown up here.

i just squash commit as @ahg-g suggestion. there are no more change.

olderTaoist · 2023-09-03T02:32:56Z

@chendave @ahg-g Does this pr need to be adjusted?

olderTaoist · 2023-09-25T10:42:48Z

@chendave @ahg-g @alculquicondor PATL

chendave

It's quite a long time, but I still see some pending review comments there. I'd suggest to ask for review only in case all those comments are either addressed or replied.

pkg/scheduler/framework/types.go

pkg/scheduler/internal/cache/cache.go

pkg/scheduler/internal/cache/cache_test.go

olderTaoist · 2023-10-07T11:38:42Z

It's quite a long time, but I still see some pending review comments there. I'd suggest to ask for review only in case all those comments are either addressed or replied.

all comment addressed, i just figured out how to mark comment as addressed

chendave

Generally LGTM.

Ideally, the fix in pkg/scheduler/framework/types.go and the refactoring around snapshot renaming along with the removing of imageState struct should be organized in different commits, the change like this is hard to review TBH.

pls double check the logic around this comments:
https://github.com/kubernetes/kubernetes/pull/116938/files#r1249435336

/cc @kubernetes/sig-scheduling-approvers

for a double check and approval.

pkg/scheduler/framework/types.go

pkg/scheduler/internal/cache/cache.go

pkg/scheduler/internal/cache/cache_test.go

k8s-ci-robot · 2023-10-15T08:23:41Z

@chendave: GitHub didn't allow me to request PR reviews from the following users: kubernetes/sig-scheduling-approvers.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

Generally LGTM.

Ideally, the fix in pkg/scheduler/framework/types.go and the refactoring around snapshot renaming along with the removing of imageState struct should be organized in different commits, the change like this is hard to review TBH.

pls double check the logic around this comments:
https://github.com/kubernetes/kubernetes/pull/116938/files#r1249435336

/cc @kubernetes/sig-scheduling-approvers

for a double check and approval.

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.

ahg-g

looks good to me, just change the DeepEqual to diff.cmp pls

pkg/scheduler/internal/cache/cache_test.go

chendave · 2023-10-18T03:28:08Z

/lgtm

thanks!

k8s-ci-robot · 2023-10-18T03:28:15Z

LGTM label has been added.

Git tree hash: e9bf9e42b60ee4765be46dade60b04caee81956a

ahg-g · 2023-10-18T21:00:16Z

/approve

k8s-ci-robot · 2023-10-18T21:00:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahg-g, olderTaoist

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:

~~pkg/scheduler/OWNERS~~ [ahg-g]

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

alculquicondor · 2023-10-18T21:28:10Z

/hold cancel

chendave · 2023-10-19T02:24:05Z

/test pull-kubernetes-e2e-gce

k8s-ci-robot · 2023-10-19T03:51:03Z

@olderTaoist: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubernetes-unit	`5d5958e`	link	unknown	`/test pull-kubernetes-unit`

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.

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. kind/bug Categorizes issue or PR as related to a bug. 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 Mar 27, 2023

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Mar 27, 2023

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Mar 27, 2023

k8s-ci-robot requested review from chendave and denkensk March 27, 2023 06:31

k8s-ci-robot added 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 Mar 27, 2023

olderTaoist force-pushed the fix-image-locality branch from 252e03b to acdaff5 Compare March 27, 2023 07:34

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Mar 27, 2023

k8s-ci-robot assigned chendave Mar 28, 2023

chendave reviewed Mar 28, 2023

View reviewed changes

pkg/scheduler/internal/cache/cache.go Outdated Show resolved Hide resolved

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 Mar 28, 2023

olderTaoist force-pushed the fix-image-locality branch 2 times, most recently from 3056bf8 to 01e110d Compare March 29, 2023 01:27

olderTaoist force-pushed the fix-image-locality branch from 73e2ed7 to 22b6f2a Compare July 11, 2023 00:55

chendave reviewed Jul 14, 2023

View reviewed changes

olderTaoist requested a review from chendave August 11, 2023 02:08

olderTaoist force-pushed the fix-image-locality branch from 5d0e171 to e58a085 Compare September 13, 2023 12:10

chendave reviewed Sep 28, 2023

View reviewed changes

olderTaoist force-pushed the fix-image-locality branch from e58a085 to 9fa1162 Compare October 7, 2023 11:30

olderTaoist requested a review from chendave October 8, 2023 09:23

chendave reviewed Oct 15, 2023

View reviewed changes

ahg-g reviewed Oct 16, 2023

View reviewed changes

pkg/scheduler/internal/cache/cache_test.go Outdated Show resolved Hide resolved

pkg/scheduler/internal/cache/cache_test.go Show resolved Hide resolved

fix ImageLocality plugin score is inconsistent

5d5958e

olderTaoist force-pushed the fix-image-locality branch from 9fa1162 to 5d5958e Compare October 17, 2023 01:39

olderTaoist requested review from chendave and ahg-g October 17, 2023 03:19

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

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

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 18, 2023

k8s-ci-robot merged commit 78b34aa into kubernetes:master Oct 19, 2023
13 of 14 checks passed

k8s-ci-robot added this to the v1.29 milestone Oct 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix ImageLocality plugin score is inconsistent #116938

fix ImageLocality plugin score is inconsistent #116938

olderTaoist commented Mar 27, 2023 •

edited

k8s-ci-robot commented Mar 27, 2023

linux-foundation-easycla bot commented Mar 27, 2023 •

edited

k8s-ci-robot commented Mar 27, 2023

k8s-ci-robot commented Mar 27, 2023

k8s-ci-robot commented Mar 27, 2023

olderTaoist commented Mar 27, 2023 •

edited

k8s-ci-robot commented Mar 27, 2023

chendave commented Mar 28, 2023

chendave commented Mar 28, 2023

chendave left a comment •

edited

chendave Jul 13, 2023

olderTaoist commented Sep 3, 2023

olderTaoist commented Sep 3, 2023

olderTaoist commented Sep 25, 2023

chendave left a comment

olderTaoist commented Oct 7, 2023

chendave left a comment

k8s-ci-robot commented Oct 15, 2023

ahg-g left a comment

chendave commented Oct 18, 2023

k8s-ci-robot commented Oct 18, 2023

ahg-g commented Oct 18, 2023

k8s-ci-robot commented Oct 18, 2023

alculquicondor commented Oct 18, 2023

chendave commented Oct 19, 2023

k8s-ci-robot commented Oct 19, 2023

fix ImageLocality plugin score is inconsistent #116938

fix ImageLocality plugin score is inconsistent #116938

Conversation

olderTaoist commented Mar 27, 2023 • edited

What type of PR is this?

Which issue(s) this PR fixes:

Does this PR introduce a user-facing change?

k8s-ci-robot commented Mar 27, 2023

linux-foundation-easycla bot commented Mar 27, 2023 • edited

k8s-ci-robot commented Mar 27, 2023

k8s-ci-robot commented Mar 27, 2023

k8s-ci-robot commented Mar 27, 2023

olderTaoist commented Mar 27, 2023 • edited

k8s-ci-robot commented Mar 27, 2023

chendave commented Mar 28, 2023

chendave commented Mar 28, 2023

chendave left a comment • edited

Choose a reason for hiding this comment

chendave Jul 13, 2023

Choose a reason for hiding this comment

olderTaoist commented Sep 3, 2023

olderTaoist commented Sep 3, 2023

olderTaoist commented Sep 25, 2023

chendave left a comment

Choose a reason for hiding this comment

olderTaoist commented Oct 7, 2023

chendave left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 15, 2023

ahg-g left a comment

Choose a reason for hiding this comment

chendave commented Oct 18, 2023

k8s-ci-robot commented Oct 18, 2023

ahg-g commented Oct 18, 2023

k8s-ci-robot commented Oct 18, 2023

alculquicondor commented Oct 18, 2023

chendave commented Oct 19, 2023

k8s-ci-robot commented Oct 19, 2023

olderTaoist commented Mar 27, 2023 •

edited

linux-foundation-easycla bot commented Mar 27, 2023 •

edited

olderTaoist commented Mar 27, 2023 •

edited

chendave left a comment •

edited