Exposure the race condition on the pod preemption #94358

chendave · 2020-08-31T09:45:16Z

Fix a potential test flake.
Add new testcase to exposure the race condition on pod preemption.
Take the chance to fix a typo.

Signed-off-by: Dave Chen dave.chen@arm.com

What type of PR is this?

Add one of the following kinds:
/kind bug

What this PR does / why we need it:

Which issue(s) this PR fixes:

Ref #93505

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

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

chendave · 2020-08-31T09:45:36Z

/release-note-none

k8s-ci-robot · 2020-08-31T09:45:37Z

@chendave: you can not set the release note label to "release-note-none" because the PR has the label "kind/deprecation".

In response to this:

/release-note-none

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 · 2020-08-31T09:45:53Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: chendave
To complete the pull request process, please assign huang-wei
You can assign the PR to them by writing /assign @huang-wei in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

pkg/scheduler/OWNERS

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

chendave · 2020-08-31T09:50:32Z

/remove-kind api-change
/remove-kind deprecation
/remove-kind regression

chendave · 2020-08-31T09:51:00Z

/release-note-none

chendave · 2020-08-31T09:54:49Z

/cc @Huang-Wei @ahg-g @alculquicondor @soulxu

I will propose a fix later, the preemption could eventually got convergent if we skip the move rquest when the unscheduleQ is empty.

k8s-ci-robot · 2020-08-31T09:54:52Z

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

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

In response to this:

/cc @Huang-Wei @ahg-g @alculquicondor @soulxu

I will propose a fix later, the preemption could eventually got convergent if we skip the move rquest when the unscheduleQ is empty.

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.

pkg/scheduler/internal/queue/scheduling_queue_test.go

alculquicondor · 2020-08-31T13:56:05Z

pkg/scheduler/internal/queue/scheduling_queue_test.go

+	if q.podBackoffQ.Len() != 1 {
+		t.Error("Expected 1 items to be in podBackoffQ")
+	}
+	// the lowPriority pod is popped and got scheduled while the highPriorityPod is stuck in the


This is by design to avoid starvation from high priority pods. Let's discuss in the original issue. However, we intend to reduce the problems of this with #94009

problem here is that the highPriorityPod should not be in backoffQ in the first place, it is moved to be backoffQ just because there was a move request happened first.
lowPriority pod popped here should be fine, but there should a way for the preemption to get convergent, pls see the code following,

// another pod is added to activeQ. q.Add(&unschedulablePod) if q.activeQ.Len() != 1 { t.Error("Expected 1 item to be in activeQ") }

it's possible a new pod is added to Queue while the highPriorityPod is still backoff-ing, after the backoff time is up, the highPriorityPod needs to preempt the lowPriority pod again, if the move request is detected the highPriorityPod is backoff again.

I prefer this to be split in a different PR so we can already merge the tests using the deterministic clock.

- Fix a potential test flake. - Add new testcase to exposure the race condition on pod preemption. - Take the chance to fix a typo. Signed-off-by: Dave Chen <dave.chen@arm.com>

chendave · 2020-09-01T09:42:11Z

/retest

k8s-ci-robot · 2020-09-01T10:07:10Z

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

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-kind	`12de87f`	link	`/test pull-kubernetes-e2e-kind`

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.

pkg/scheduler/internal/queue/scheduling_queue_test.go

alculquicondor · 2020-09-01T14:27:58Z

pkg/scheduler/internal/queue/scheduling_queue_test.go

+	if q.podBackoffQ.Len() != 1 {
+		t.Error("Expected 1 items to be in podBackoffQ")
+	}
+	// the lowPriority pod is popped and got scheduled while the highPriorityPod is stuck in the


I prefer this to be split in a different PR so we can already merge the tests using the deterministic clock.

soulxu · 2020-09-02T03:42:30Z

pkg/scheduler/internal/queue/scheduling_queue_test.go

+	}
+	// the lowPriority pod is popped and got scheduled while the highPriorityPod is stuck in the
+	// backoffQ.
+	q.Pop()


I still stuck at the problem of the low priority pod shouldn't be gotten a chance to schedule successful, since when the low priority pod getting scheduling, the resources already token by high priority pod.

please thinking about the case that high priority pod is still in the unscheduleQ or backoffQ, low priority pod is the only one in the activeQ.

chendave · 2020-09-03T07:09:53Z

/hold

k8s-ci-robot · 2020-09-23T17:43:08Z

@chendave: PR needs rebase.

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.

fejta-bot · 2020-12-22T18:27:00Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

fejta-bot · 2021-01-21T19:11:28Z

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

chendave · 2021-02-18T08:07:20Z

/close

k8s-ci-robot · 2021-02-18T08:07:29Z

@chendave: Closed this PR.

In response to this:

/close

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 requested review from ahg-g and wgliang August 31, 2020 09:46

k8s-ci-robot added 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 Aug 31, 2020

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. 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 Aug 31, 2020

k8s-ci-robot requested review from alculquicondor and Huang-Wei August 31, 2020 09:54

chendave force-pushed the exposure_93505 branch from 00ff4e8 to 7325fdf Compare August 31, 2020 10:00

alculquicondor reviewed Aug 31, 2020

View reviewed changes

Exposure the race condition on the pod preemption

12de87f

- Fix a potential test flake. - Add new testcase to exposure the race condition on pod preemption. - Take the chance to fix a typo. Signed-off-by: Dave Chen <dave.chen@arm.com>

chendave force-pushed the exposure_93505 branch from 7325fdf to 12de87f Compare September 1, 2020 08:29

alculquicondor reviewed Sep 1, 2020

View reviewed changes

soulxu reviewed Sep 2, 2020

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 3, 2020

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 23, 2020

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 22, 2020

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jan 21, 2021

k8s-ci-robot closed this Feb 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Exposure the race condition on the pod preemption #94358

Exposure the race condition on the pod preemption #94358

chendave commented Aug 31, 2020 •

edited

Loading

chendave commented Aug 31, 2020

k8s-ci-robot commented Aug 31, 2020

k8s-ci-robot commented Aug 31, 2020

chendave commented Aug 31, 2020

chendave commented Aug 31, 2020

chendave commented Aug 31, 2020

k8s-ci-robot commented Aug 31, 2020

alculquicondor Aug 31, 2020

chendave Sep 1, 2020 •

edited

Loading

alculquicondor Sep 1, 2020

chendave commented Sep 1, 2020

k8s-ci-robot commented Sep 1, 2020 •

edited

Loading

alculquicondor Sep 1, 2020

soulxu Sep 2, 2020

chendave Sep 2, 2020

chendave commented Sep 3, 2020

k8s-ci-robot commented Sep 23, 2020

fejta-bot commented Dec 22, 2020

fejta-bot commented Jan 21, 2021

chendave commented Feb 18, 2021

k8s-ci-robot commented Feb 18, 2021

Exposure the race condition on the pod preemption #94358

Exposure the race condition on the pod preemption #94358

Conversation

chendave commented Aug 31, 2020 • edited Loading

chendave commented Aug 31, 2020

k8s-ci-robot commented Aug 31, 2020

k8s-ci-robot commented Aug 31, 2020

chendave commented Aug 31, 2020

chendave commented Aug 31, 2020

chendave commented Aug 31, 2020

k8s-ci-robot commented Aug 31, 2020

alculquicondor Aug 31, 2020

Choose a reason for hiding this comment

chendave Sep 1, 2020 • edited Loading

Choose a reason for hiding this comment

alculquicondor Sep 1, 2020

Choose a reason for hiding this comment

chendave commented Sep 1, 2020

k8s-ci-robot commented Sep 1, 2020 • edited Loading

alculquicondor Sep 1, 2020

Choose a reason for hiding this comment

soulxu Sep 2, 2020

Choose a reason for hiding this comment

chendave Sep 2, 2020

Choose a reason for hiding this comment

chendave commented Sep 3, 2020

k8s-ci-robot commented Sep 23, 2020

fejta-bot commented Dec 22, 2020

fejta-bot commented Jan 21, 2021

chendave commented Feb 18, 2021

k8s-ci-robot commented Feb 18, 2021

chendave commented Aug 31, 2020 •

edited

Loading

chendave Sep 1, 2020 •

edited

Loading

k8s-ci-robot commented Sep 1, 2020 •

edited

Loading