Remove PodBackoffMap #87948

notpad · 2020-02-08T09:15:00Z

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespace from that line:

/kind cleanup

What this PR does / why we need it:

The PodBackoffMap don't need to store pod attempts information because PodInfo already contains this data

Which issue(s) this PR fixes:

Fixes #87832

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

None

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

/sig scheduling

notpad · 2020-02-08T09:18:45Z

/assign @alculquicondor

notpad · 2020-02-08T11:42:08Z

/retest

notpad · 2020-02-08T12:34:00Z

/retest

notpad · 2020-02-08T15:07:59Z

/retest

notpad · 2020-02-09T00:54:37Z

/retest

notpad · 2020-02-09T02:20:22Z

/retest

pkg/scheduler/internal/queue/scheduling_queue.go

pkg/scheduler/internal/queue/pod_backoff.go

pkg/scheduler/internal/queue/pod_backoff_test.go

pkg/scheduler/internal/queue/scheduling_queue.go

alculquicondor · 2020-02-10T16:15:20Z

pkg/scheduler/internal/queue/scheduling_queue_test.go

+
+	// Override clock to exceed the DefaultPodInitialBackoffDuration so that unschedulable pods
+	// will be moved to activeQ
+	c.SetTime(timestamp.Add(DefaultPodInitialBackoffDuration + 1))


Use c.Step instead.

But we shouldn't be doing this. Is there a clearBackoff related to this that we removed? If so, we should probably also reset Attempts here? @Huang-Wei

pkg/scheduler/internal/queue/scheduling_queue_test.go

pkg/scheduler/internal/queue/pod_backoff.go

ahg-g · 2020-02-11T15:18:45Z

pkg/scheduler/internal/queue/pod_backoff.go

-		maxDuration:       maxDuration,
-		podAttempts:       make(map[ktypes.NamespacedName]int),
-		podLastUpdateTime: make(map[ktypes.NamespacedName]time.Time),
+// NewPodBackoff creates a PodBackoff with initial duration and max duration.


I am not sure we need this struct at all, those couple of variables can be part of PriorityQueue, the couple of functions below are pretty short, so they can be methods under PriorityQueue as well

pkg/scheduler/internal/queue/scheduling_queue.go

notpad · 2020-02-12T13:53:50Z

/retest

alculquicondor · 2020-02-12T14:58:52Z

Please avoid amending until we have finished reviewing. When you amend, we lose context of the ongoing discussions and we might have to review the whole PR again 😞

notpad · 2020-02-12T15:10:58Z

oh, sorry. I thought the code changed a lot, so I used a force push. Will remember next time.

pkg/scheduler/internal/queue/scheduling_queue_test.go

pkg/scheduler/internal/queue/scheduling_queue.go

alculquicondor · 2020-02-12T15:21:10Z

pkg/scheduler/internal/queue/scheduling_queue_test.go

@@ -1201,6 +1206,22 @@ func TestPendingPodsMetric(t *testing.T) {
 		}
 		pInfos = append(pInfos, p)
 	}
+	totalWithDelay := 20
+	var pInfosWithDelay = make([]*framework.PodInfo, 0, totalWithDelay)


instead, you could make the creation of podInfos a function that accepts the current timestamp.

Then, you would have the same steps that you are setting (add, step, add), and some operands like:
pInfos(0, 20), nil, pInfos(20, 30)

but the pInfos will be calculated before the execution of steps (add, step, add),, so it can't get the current timestamp dynamically

I've added a new func to avoid duplicate code

well operands could be functions instead of actual values, but this is good enough.

alculquicondor · 2020-02-12T15:26:12Z

pkg/scheduler/internal/queue/scheduling_queue_test.go

@@ -1589,11 +1607,67 @@ func TestBackOffFlow(t *testing.T) {
 			}
 		})
 	}
-	// After some time, backoff information is cleared.
-	cl.Step(time.Hour)


Great to see this going away.

pkg/scheduler/internal/queue/scheduling_queue_test.go

alculquicondor · 2020-02-12T15:27:57Z

oh, sorry. I thought the code changed a lot, so I used a force push. Will remember next time.

That's actually fair. No big deal.

pkg/scheduler/internal/queue/scheduling_queue.go

pkg/scheduler/internal/queue/scheduling_queue_test.go

alculquicondor · 2020-02-12T17:14:46Z

There a few other comments that you haven't addressed. Maybe you missed them because they were added to "resolved" threads?

alculquicondor

Resurfacing comments

pkg/scheduler/internal/queue/scheduling_queue.go

alculquicondor · 2020-02-12T17:16:47Z

pkg/scheduler/internal/queue/scheduling_queue_test.go

@@ -380,8 +380,11 @@ func TestPriorityQueue_MoveAllToActiveQueue(t *testing.T) {
 	addOrUpdateUnschedulablePod(q, q.newPodInfo(&unschedulablePod))
 	addOrUpdateUnschedulablePod(q, q.newPodInfo(&highPriorityPod))


can we remove addOrUpdateUnschedulablePod?

alculquicondor · 2020-02-13T15:20:15Z

/approve

@ahg-g anything to add? If not, we can squash.

k8s-ci-robot · 2020-02-13T15:21:06Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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~~ [alculquicondor]

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

ahg-g · 2020-02-13T17:50:59Z

/lgtm
/retest

alculquicondor · 2020-02-13T17:53:48Z

/hold for squash

notpad · 2020-02-14T02:47:18Z

/retest

notpad · 2020-02-14T14:46:26Z

/retest

alculquicondor · 2020-02-14T15:28:42Z

/hold cancel

great work @notpad, thanks

alculquicondor · 2020-02-14T15:28:53Z

/lgtm

fejta-bot · 2020-02-14T17:40:27Z

/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.

alculquicondor · 2020-02-14T18:45:21Z

/priority important-longterm

k8s-ci-robot requested review from draveness and liu-cong February 8, 2020 09:16

k8s-ci-robot assigned alculquicondor Feb 8, 2020

Huang-Wei reviewed Feb 10, 2020

View reviewed changes

pkg/scheduler/internal/queue/scheduling_queue.go Show resolved Hide resolved

alculquicondor reviewed Feb 10, 2020

View reviewed changes

ahg-g reviewed Feb 11, 2020

View reviewed changes

notpad force-pushed the feature/pod_backoff branch from 045c407 to 63d7d8a Compare February 12, 2020 12:34

alculquicondor reviewed Feb 12, 2020

View reviewed changes

ahg-g reviewed Feb 12, 2020

View reviewed changes

pkg/scheduler/internal/queue/scheduling_queue.go Outdated Show resolved Hide resolved

pkg/scheduler/internal/queue/scheduling_queue.go Outdated Show resolved Hide resolved

pkg/scheduler/internal/queue/scheduling_queue_test.go Show resolved Hide resolved

alculquicondor reviewed Feb 12, 2020

View reviewed changes

alculquicondor mentioned this pull request Feb 12, 2020

Fix pod inconsistency between podBackoffQ/unschedulableQ and podBackoffMap for sched… #78849

Closed

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

k8s-ci-robot assigned ahg-g Feb 13, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 13, 2020

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

Remove PodBackoffMap

a4e4a99

notpad force-pushed the feature/pod_backoff branch from c79521a to a4e4a99 Compare February 14, 2020 00:31

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 14, 2020

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

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

k8s-ci-robot added priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Feb 14, 2020

k8s-ci-robot merged commit bde718d into kubernetes:master Feb 14, 2020

k8s-ci-robot added this to the v1.18 milestone Feb 14, 2020

alculquicondor mentioned this pull request Feb 14, 2020

Totally avoid Pod starvation (HOL blocking) or clarify the user expectation on the wiki #86373

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove PodBackoffMap #87948

Remove PodBackoffMap #87948

notpad commented Feb 8, 2020

notpad commented Feb 8, 2020

notpad commented Feb 8, 2020

notpad commented Feb 8, 2020

notpad commented Feb 8, 2020

notpad commented Feb 9, 2020

notpad commented Feb 9, 2020

alculquicondor Feb 10, 2020

alculquicondor Feb 10, 2020

ahg-g Feb 11, 2020

notpad commented Feb 12, 2020

alculquicondor commented Feb 12, 2020

notpad commented Feb 12, 2020

alculquicondor Feb 12, 2020

notpad Feb 12, 2020

notpad Feb 12, 2020

alculquicondor Feb 12, 2020 •

edited

Loading

alculquicondor Feb 12, 2020

alculquicondor commented Feb 12, 2020

alculquicondor commented Feb 12, 2020

alculquicondor left a comment

alculquicondor Feb 12, 2020

alculquicondor commented Feb 13, 2020

k8s-ci-robot commented Feb 13, 2020

ahg-g commented Feb 13, 2020

alculquicondor commented Feb 13, 2020

notpad commented Feb 14, 2020

notpad commented Feb 14, 2020

alculquicondor commented Feb 14, 2020

alculquicondor commented Feb 14, 2020

fejta-bot commented Feb 14, 2020

alculquicondor commented Feb 14, 2020

		@@ -380,8 +380,11 @@ func TestPriorityQueue_MoveAllToActiveQueue(t *testing.T) {
		addOrUpdateUnschedulablePod(q, q.newPodInfo(&unschedulablePod))
		addOrUpdateUnschedulablePod(q, q.newPodInfo(&highPriorityPod))

Remove PodBackoffMap #87948

Remove PodBackoffMap #87948

Conversation

notpad commented Feb 8, 2020

notpad commented Feb 8, 2020

notpad commented Feb 8, 2020

notpad commented Feb 8, 2020

notpad commented Feb 8, 2020

notpad commented Feb 9, 2020

notpad commented Feb 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

notpad commented Feb 12, 2020

alculquicondor commented Feb 12, 2020

notpad commented Feb 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alculquicondor Feb 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alculquicondor commented Feb 12, 2020

alculquicondor commented Feb 12, 2020

alculquicondor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alculquicondor commented Feb 13, 2020

k8s-ci-robot commented Feb 13, 2020

ahg-g commented Feb 13, 2020

alculquicondor commented Feb 13, 2020

notpad commented Feb 14, 2020

notpad commented Feb 14, 2020

alculquicondor commented Feb 14, 2020

alculquicondor commented Feb 14, 2020

fejta-bot commented Feb 14, 2020

alculquicondor commented Feb 14, 2020

alculquicondor Feb 12, 2020 •

edited

Loading