-
Notifications
You must be signed in to change notification settings - Fork 639
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
Bugfix: Cronjob ctx cancel #731
Conversation
Hi @jklaw90. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the 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. |
333da28
to
0f9e5c5
Compare
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.
Looks good!!! Left one nit
if err != nil { | ||
klog.ErrorS(err, "descheduler server") | ||
} | ||
|
||
done() |
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.
Would moving done
back under the defer
produce the same behavior?
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.
I changed it this way to notify the metrics server to close which blocks on the next line until all requests are done. If I kept it as a defer it's going to hang forever on the blocking channel for the cronjob case.
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.
Understood.
@@ -69,13 +69,7 @@ func Run(ctx context.Context, rs *options.DeschedulerServer) error { | |||
return err | |||
} | |||
|
|||
// tie in root ctx with our wait stopChannel | |||
stopChannel := make(chan struct{}) |
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.
I like removing this part!!!
/ok-to-test |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ingvagabund, jklaw90 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 |
There's a valid building error in the e2e:
|
@jklaw90 would you mind squashing the commits? Looks good. |
7cf480d
to
a2cbc25
Compare
/retest |
/lgtm |
/retest |
Removed the stop channel and just use ctx.
Fixes #728