fix the flake in scheduling_queue_test #74611
Conversation
k8s-ci-robot
added
release-note
k8s-ci-robot
added
sig/scheduling
denkensk
force-pushed
the
fix-unable-find-backoff-value
branch
from
8e40352
to
a7866e4
Compare
|
/assign @bsalamat |
| flushBackoffQ := func() operation { | ||
| return func() { | ||
| // wait for pod to complete backoff. | ||
| time.Sleep(time.Second) |
There was a problem hiding this comment.
Can you collaborate why this is needed? And it seems to be able to transfer to a more grace solution like channel or wait?
There was a problem hiding this comment.
Sure. Thanks. I transfer to a more grace solution. Done
| flushBackoffQ := func() operation { | ||
| return func() { | ||
| // wait for pod to complete backoff. | ||
| time.Sleep(time.Second) |
There was a problem hiding this comment.
A better approach is to use NewPriorityQueueWithClock instead of NewPriorityQueue at line 1122 and pass a FakeClock (k8s.io/apimachinery/pkg/util/clock) to it. You can then control the time with the fake clock, for example by calling its Step function.
There was a problem hiding this comment.
Done thank you for your help.
denkensk
force-pushed
the
fix-unable-find-backoff-value
branch
2 times, most recently
from
1fbe499
to
1a92402
Compare
|
Test is ok. PTAL @bsalamat thank you. |
| }, | ||
| expected: []*podInfo{pInfo1, pInfo2}, | ||
| }, | ||
| } | ||
|
|
||
| for _, test := range tests { | ||
| t.Run(test.name, func(t *testing.T) { | ||
| queue = NewPriorityQueue(nil) | ||
| queue = NewPriorityQueueWithClock(nil, clock.NewFakeClock(time.Now())) |
There was a problem hiding this comment.
time.Now() changes every time you read it. In order to have a robust test, please read it once in the beginning of the function and use that time here and for initializing pInfo1 and pInfo2 in lines 1026 and 1030.
There was a problem hiding this comment.
Yes, Thanks for your advice. Done
| flushBackoffQ := func() operation { | ||
| return func() { | ||
| queue.clock.(*clock.FakeClock).Step(queue.podBackoff.MaxDuration()) |
There was a problem hiding this comment.
It is better to Step 2 seconds here instead of MaxDuration. That would be a more precise test.
denkensk
force-pushed
the
fix-unable-find-backoff-value
branch
from
1a92402
to
471679f
Compare
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bsalamat, denkensk 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 |
k8s-ci-robot
added
the
approved
What type of PR is this?
/kind failing-test
/kind flake
/sig scheduling
What this PR does / why we need it:
seeing a flake in scheduling unit tests - https://storage.googleapis.com/k8s-gubernator/triage/index.html?pr=1&text=Unable%20to%20find%20backoff%20value
Which issue(s) this PR fixes:
Fixes #73700 (comment)