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

Fix scheduler issue with nodetree additions #93387

Merged
merged 1 commit into from Aug 5, 2020
Merged

Fix scheduler issue with nodetree additions #93387

merged 1 commit into from Aug 5, 2020

Conversation

maelk
Copy link
Contributor

@maelk maelk commented Jul 23, 2020

What type of PR is this?

/kind bug
/sig scheduling

What this PR does / why we need it:

This is a backport of #93355 .
When adding multiple nodes to the scheduler nodetree, the function getting the next node does not return all the nodes one after an other, but skips some and duplicate others. This commit works around the problem by always starting with reset counters.

Which issue(s) this PR fixes:

Fixes #91601

Does this PR introduce a user-facing change?:

NONE

@k8s-ci-robot k8s-ci-robot added the release-note-none Denotes a PR that doesn't merit a release note. label Jul 23, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.18 milestone Jul 23, 2020
@k8s-ci-robot k8s-ci-robot added do-not-merge/cherry-pick-not-approved Indicates that a PR is not yet approved to merge into a release branch. kind/bug Categorizes issue or PR as related to a bug. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jul 23, 2020
@k8s-ci-robot
Copy link
Contributor

Hi @maelk. 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 the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 23, 2020
@maelk
Copy link
Contributor Author

maelk commented Jul 23, 2020

/assign @ahg-g

@maelk maelk mentioned this pull request Jul 23, 2020
Merged
@alculquicondor
Copy link
Member

/hold

let's discuss on the original PR

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 23, 2020
@ahg-g
Copy link
Member

ahg-g commented Jul 23, 2020

/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 Jul 23, 2020
@maelk
Copy link
Contributor Author

maelk commented Jul 23, 2020

/hold cancel
we'll go with this as a hotfix : #93355 (review)

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 23, 2020
@alculquicondor
Copy link
Member

/approve
/lgtm
/retest

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, maelk

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 Jul 23, 2020
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 24, 2020
@maelk
Copy link
Contributor Author

maelk commented Jul 24, 2020

/retest

@alculquicondor
Copy link
Member

@kubernetes/release-managers

@maelk
Copy link
Contributor Author

maelk commented Jul 25, 2020

/retest

2 similar comments
@ahg-g
Copy link
Member

ahg-g commented Jul 25, 2020

/retest

@maelk
Copy link
Contributor Author

maelk commented Jul 26, 2020

/retest

@maelk
Copy link
Contributor Author

maelk commented Jul 26, 2020

/retest
@ahg-g @alculquicondor this has now failed many times on the pull-kubernetes-e2e-gce-device-plugin-gpu. Should I keep retriggering ? also, lgty ?

@ahg-g
Copy link
Member

ahg-g commented Jul 26, 2020

@hasheddan the test pull-kubernetes-e2e-gce-device-plugin-gpu is failing constantly on this 1.18 patch, will your #93207 fix make to 1.18 and is it going to fix the failing test?

@hasheddan
Copy link
Contributor

@ahg-g interesting, I was monitoring the test on the 1.18 blocking dashboard https://testgrid.k8s.io/sig-release-1.18-blocking#gce-device-plugin-gpu-1.18 and it has been passing.. somehow? Anyway, I opened a backport to 1.17 today and am happy to do so for 1.18 as well. Any idea why this is passing on 1.18 blocking?

@ahg-g
Copy link
Member

ahg-g commented Jul 26, 2020

I honestly have no idea what this test is doing and why it is under sig-scheduling in the first place!

@hasheddan
Copy link
Contributor

@ahg-g I believe it is testing that pods requesting GPUs are scheduled to correct nodes, so I am guessing that is why it lives in SIG-scheduling? Anyways, I have opened a backport to 1.18 :)

@ahg-g
Copy link
Member

ahg-g commented Jul 26, 2020

thanks, do you know if the failure in this PR will be fixed by your PR?

@hasheddan
Copy link
Contributor

@ahg-g I cannot be 100% certain, but the failure is consistent with the one that was fixed when the PR was merged to master (i.e. GPUs not being properly set up on nodes)

@ahg-g
Copy link
Member

ahg-g commented Jul 26, 2020

/retest

hasheddan
hasheddan approved these changes Jul 26, 2020
View reviewed changes
Copy link
Contributor

@hasheddan hasheddan 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 Jul 26, 2020
@ahg-g
Copy link
Member

ahg-g commented Jul 26, 2020

/retest

1 similar comment
@maelk
Copy link
Contributor Author

maelk commented Jul 28, 2020

/retest

@alculquicondor
Copy link
Member

ping @kubernetes/release-managers

@feiskyer feiskyer added cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. and removed do-not-merge/cherry-pick-not-approved Indicates that a PR is not yet approved to merge into a release branch. labels Aug 4, 2020
@furkatgofurov7
Copy link
Member

20/20 tests passed, but pull-kubernetes-verify job is still failing. Retriggering it again.

/retest

@hasheddan hasheddan mentioned this pull request Aug 4, 2020
Closed
@furkatgofurov7
Copy link
Member

pull-kubernetes-integration job is failed due to timeout.

/retest

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@feiskyer
Copy link
Member

feiskyer commented Aug 4, 2020

/retest

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@k8s-ci-robot k8s-ci-robot merged commit c6cb4f0 into kubernetes:release-1.18 Aug 5, 2020
@dennis-benzinger-hybris dennis-benzinger-hybris mentioned this pull request Sep 8, 2020
Closed
@Huang-Wei Huang-Wei mentioned this pull request Dec 8, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hasheddan hasheddan hasheddan approved these changes

@alculquicondor alculquicondor Awaiting requested review from alculquicondor

@hex108 hex108 Awaiting requested review from hex108

Assignees

@alculquicondor alculquicondor

@hasheddan hasheddan

@ahg-g ahg-g

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. release-note-none Denotes a PR that doesn't merit a release note. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.18
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@maelk @k8s-ci-robot @alculquicondor @ahg-g @hasheddan @furkatgofurov7 @fejta-bot @feiskyer