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

Skip updating scheduler cache on pod update if the node was deleted #89908

Merged
merged 1 commit into from
Apr 7, 2020
Merged

Skip updating scheduler cache on pod update if the node was deleted #89908

merged 1 commit into from
Apr 7, 2020

Conversation

alculquicondor
Copy link
Member

@alculquicondor alculquicondor commented Apr 6, 2020 •
edited
Loading

What type of PR is this?

/kind bug

What this PR does / why we need it:

/sig scheduling

Skip updating scheduler cache on pod update if the node was deleted. This can happen when events arrive out of order.
The fix doesn't skip updates on pods that might arrive before its node is created. There will always be a pod creation event first that would cause the cache to add a placeholder node.

Add unit test for out-of-order pod/node creation/removal.
This should help spot bugs when:

  • Pod creation event arrives before node creation
  • Node removal event arrives before corresponding pod removal.

Which issue(s) this PR fixes:

Fixes #89552

This only reproduced with at least 2 pods in the node being removed.

Does this PR introduce a user-facing change?:

Fix scheduler crash when removing node before its pods

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. 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 needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Apr 6, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor

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 Apr 6, 2020
@alculquicondor
Copy link
Member Author

/assign @ahg-g

@alculquicondor alculquicondor mentioned this pull request Apr 6, 2020
Closed
@alculquicondor
Copy link
Member Author

/retest

@alculquicondor alculquicondor changed the title Add unit tests for out-of-order pod/node events Skip updating scheduler cache on pod update if the node was deleted Apr 7, 2020
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Apr 7, 2020
@alculquicondor
Copy link
Member Author

/priority critical-urgent

@k8s-ci-robot k8s-ci-robot added priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Apr 7, 2020
@alculquicondor alculquicondor mentioned this pull request Apr 7, 2020
Closed
ahg-g
ahg-g reviewed Apr 7, 2020
View reviewed changes
Copy link
Member

@ahg-g ahg-g left a comment

Choose a reason for hiding this comment

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

tests are failing

pkg/scheduler/internal/cache/cache.go Outdated Show resolved Hide resolved
alculquicondor
alculquicondor commented Apr 7, 2020
View reviewed changes
pkg/scheduler/internal/cache/cache.go Outdated
if err := cache.removePod(oldPod); err != nil {
return err
}
cache.addPod(newPod)
Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, this was the mistake

@alculquicondor
Skip updating cache on pod update if the node was deleted
e75b5dc 
Signed-off-by: Aldo Culquicondor <acondor@google.com>
@ahg-g
Copy link
Member

ahg-g commented Apr 7, 2020

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 7, 2020
@alculquicondor
Copy link
Member Author

/retest

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Apr 7, 2020 •
edited
Loading

@alculquicondor: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-kubernetes-node-e2e-containerd acd29331d584e488e92f31e971cd046e3f068bb6 link /test pull-kubernetes-node-e2e-containerd

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.

@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 1edbfe1 into kubernetes:master Apr 7, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.19 milestone Apr 7, 2020
@ahg-g
Copy link
Member

ahg-g commented Apr 8, 2020

we should backport this, right?

@alculquicondor
Copy link
Member Author

Yes, I'm on it.

@alculquicondor alculquicondor mentioned this pull request Apr 8, 2020
Merged
k8s-ci-robot added a commit that referenced this pull request Apr 10, 2020
@k8s-ci-robot
Merge pull request #89959 from alculquicondor/automated-cherry-pick-o…
c48ba09 
…f-#89908-upstream-release-1.18

Automated cherry pick of #89908: Skip updating cache on pod update if the node was deleted
ping035627 added a commit to ping035627/kubernetes that referenced this pull request Jun 16, 2020
@ping035627
Automated cherry pick of kubernetes#89908: Skip updating cache on pod…
4711920 
… update if the node was deleted

Signed-off-by: PingWang <wang.ping5@zte.com.cn>
@ping035627 ping035627 mentioned this pull request Jun 16, 2020
Closed
@alculquicondor alculquicondor mentioned this pull request Aug 13, 2020
Merged
@alculquicondor alculquicondor mentioned this pull request Jan 5, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ahg-g ahg-g ahg-g left review comments

@damemi damemi Awaiting requested review from damemi

@wgliang wgliang Awaiting requested review from wgliang

Assignees

@ahg-g ahg-g

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/bug Categorizes issue or PR as related to a bug. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. 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.19
Development

Successfully merging this pull request may close these issues.

Delete node causing scheduler crash
4 participants
@alculquicondor @k8s-ci-robot @ahg-g @fejta-bot