Deflake scheduler PostBind integration test#93661
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Huang-Wei The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/assign @ahg-g |
|
/retest |
| case <-time.After(time.Second * 15): | ||
| t.Errorf("pluginInvokeEventChan timed out") | ||
| } | ||
| if postBindPlugin.numPostBindCalled == 0 { |
There was a problem hiding this comment.
Emm...this check should be useless anymore? since if postBindPlugin is called, the pluginInvokeEventChan will return, if the postBindPlugin isn't called, the timeout will return.
There was a problem hiding this comment.
if the postBindPlugin isn't called, the timeout will return.
t.Errorf won't return - the logic proceeds to L1344.
There was a problem hiding this comment.
if the postBindPlugin isn't called, the timeout will return.
t.Errorfwon't return - the logic proceeds to L1344.
Yea, I mean only the t.Errorf at line 1342 is enough, line 1342 already ensure the plugin isn't called. so the line 1344 to 1346 can be removed.
There was a problem hiding this comment.
I see your point. Although the root cause of L1342 and L1344 ~ L1346 is the same, but their symptoms are different, so IMO keeping L1344 ~ L1346 as is can make the semantics more explicit.
|
/milestone v1.19 I think we should still include it in 1.19. |
Deflake scheduler PostBind integration test
What type of PR is this?
/kind flake
/sig scheduling
What this PR does / why we need it:
For a successful scheduling,
PostBind()is for sure called afterBind(). However, when the test detects the pod is assigned ".spec.nodeName" right afterBind()(beforePostBind()), and process to check the postFilter counter, the test may flake.This PR tries to fix this flake by waiting for
PostBind()to complete. Additionally, it also wraps the scheduler initialization logic into the sub-test, to make each sub-test independent.Which issue(s) this PR fixes:
Fixes #93659
Special notes for your reviewer:
The flake doesn't seem to block 1.19 release, so I think it can be merged to 1.20.
Does this PR introduce a user-facing change?: