-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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.
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
disttask: fix bug when owner change #46054
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Hi @ywqzzy. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. 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. |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #46054 +/- ##
================================================
- Coverage 74.7608% 72.6404% -2.1204%
================================================
Files 1293 1300 +7
Lines 398103 400167 +2064
================================================
- Hits 297625 290683 -6942
- Misses 81108 90992 +9884
+ Partials 19370 18492 -878
Flags with carried forward coverage won't be shown. Click here to find out more.
|
@ywqzzy: The following tests failed, say
Full PR test history. Your PR dashboard. 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. |
@@ -211,6 +214,9 @@ func (d *dispatcher) handleRunning() error { | |||
} | |||
|
|||
func (d *dispatcher) updateTask(taskState string, newSubTasks []*proto.Subtask, retryTimes int) (err error) { | |||
if !d.ownerManager.IsOwner() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if the owner is dropped just after this check? There is always an interval between time of check to time of use. Can we add a version to transactional storage to detect it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point!
What problem does this PR solve?
Issue Number: close #46014
Problem Summary:
It is possible that 2 dispatcher coexist on different server.
Then these 2 dispatcher dispatched subtasks twice.
What is changed and how it works?
Before dispatch task, check the dispatcher is the owner.
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.