-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Always try to reinitialize Crypto.Random when forking minion process #55635
Conversation
6da4251
to
746cbed
Compare
Added tests + rebased to current master (@Ch3LL) |
Seems like the tests fail on Windows. From the look of it, it might actually be bug in Python (https://bugs.python.org/issue17560), but I have to test it manually on Windows first. |
7e7b268
to
5a48834
Compare
I've made one fix for Win test, but they still fail (probably because when the fork happens the current process it so big it triggers either the mentioned bug or something else). But I'm not able to reproduce it on clean Win machine and I don't have access to these testing golden images. Any ideas how to tackle this? Otherwise I will just add |
@s0undt3ch or @twangboy would probably have the most knowledge about this. Could either of you weigh in? |
re-run all full |
re-run full all |
@lukasraska Please address @s0undt3ch's comments as well as the windows test failures. |
33a2a75
to
cc00b94
Compare
Rebased, implementation changed as |
re-run pr-amazon1-py2 |
What does this PR do?
Always try to reinitialize Crypto.Random when processing minion events to avoid issues when minion tries to sign something during/after job execution.
What issues does this PR fix or reference?
Fix #55116
Previous Behavior
When having minion configured with option
minion_sign_messages: True
, any job result event would fail withPID check failed. RNG must be re-initialized after fork()
when using RHEL 6 or TCP transportNew Behavior
Minion successfully responds with job result
Tests written?
No, AFAIK the only way how to really test this is to enableminion_sign_messages
for integration tests. As this is currently global for all tests, it might affect other tests that inspect raw events (cc @waynew )Yes
Commits signed with GPG?
Yes