-
Notifications
You must be signed in to change notification settings - Fork 328
dbus: allow multiple calls for the same unit to *Unit #496
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
Conversation
c963247 to
cf0dab7
Compare
|
lgtm |
|
@kolyshkin PTAL |
Signed-off-by: Peter Hunt <[email protected]>
kolyshkin
left a comment
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.
Left a few comments.
Also, if you can briefly describe the whole issue in the commit message that'd be awesome (in the ideal world I want to know why we're changing the code just by looking at git log or git show).
cf0dab7 to
be3f615
Compare
|
updated as suggested! |
be3f615 to
8b4f566
Compare
kolyshkin
left a comment
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.
LGTM
@Luap99 PTAL
Luap99
left a comment
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.
LGTM
As well as add a test to prove StopUnit is reentrant There are cases that multiple StopUnits may be called for the same job. For instance, in kubernetes, the Kubelet does so in some cases, when it races against itself to delete a pod. Instead of fixing in the caller, this can be worked around in this library. Instead of losing track of an old job if a new StopUnit is called for the same job, we can just track multiple jobs, and signal to multiple channels when the job finishes. see kubernetes/kubernetes#135826 and kubernetes/kubernetes#135825 for some more context Signed-off-by: Peter Hunt <[email protected]>
8b4f566 to
83b9e9b
Compare
As well as add a test to prove StopUnit is reentrant
Alternative to kubernetes/kubernetes#135826
should fix kubernetes/kubernetes#135825 (need to do some testing to double check)
credit to @rphillips for the idea and original piece of code