-
Notifications
You must be signed in to change notification settings - Fork 2.8k
test(controller): race condition in handleSigterm() #5821
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
base: master
Are you sure you want to change the base?
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 @vflaux. 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-sigs/prow repository. |
09a2b7c
to
ad9a3b1
Compare
ad9a3b1
to
b1c4916
Compare
Probably not needed if #5798 is merged. |
Fix a race condition between the signal handler setup and the signal send in the test for handleSigterm()
b1c4916
to
04b6ada
Compare
What does it do ?
Fix a race condition between the signal handler setup and the signal send in the test for
handleSigterm()
.In the test, in rare case the SIGTERM signal could be send before the
handleSigterm()
callssignal.Notify()
.In this case, the cancel function is never called.
The test always pass because it silently ignore this with the select case
sig := <-sigChan
.I think we need to test that the signal received is a SIGTERM and that the cancel func has been called. Not one of the two conditions.
Motivation
This primarily concerns testing and ensuring correctness. There is most likely no issue when the controller run.
I could reproduce this with
go test -timeout 30s -count 1000 -run ^TestHandleSigterm$ sigs.k8s.io/external-dns/controller
after removing thesig := <-sigChan
select case in the current code.Found this when trying to understand this race condition report: https://prow.k8s.io/view/gs/kubernetes-ci-logs/pr-logs/pull/kubernetes-sigs_external-dns/5810/pull-external-dns-unit-test/1963995845443457024
Not sure if this would fix as I'm not able to reproduce it.
More