Stop hang when use mongo SSL #5240
Conversation
pull-request-size
bot
added
the
size/XS
| import os | ||
| from st2common.util.monkey_patch import monkey_patch | ||
|
|
||
| monkey_patch() |
There was a problem hiding this comment.
I'm somewhat worried if that is the correct / right change - I know that in the past we intentionally only did monkey patching in the worker and not the parent process.
I guess technically if all the tests pass and we observe no weird behavior locally with CTRL+C and stopping gunicorn process, it's probably.
But having said that, I believe we do exactly the same thing for st2api and st2auth - and if we do, we should likely update all the affected places.
There was a problem hiding this comment.
Btw, which version of st2 + mongoengine + pymongo are you using?
That hanging looks exactly the same as the hanging PR which adds support for Python 3.8 + Mongo DB 4.4 uncovered which I believe I tracked to us not performing monkey patching early enough so pymongo obtains reference to non patched version of threading module which will cause all kinds of issues.
And from the symptoms you mentioned it indeed looks like it could be related to the same root cause (although in that case it's related to threading module and here it's likely related to ssl one).
There was a problem hiding this comment.
This was StackStorm 3.4.1 on EL8. Mongo 4.0 and python 3.6.
st2api, st2auth were connecting successfully but st2stream wasn't. (So applied same change that was made to st2api to st2stream).
st2stream seems to be the only one that monkey patches differently to the other gunicorn processes. Was there a reason for that?
So st2api and st2auth both monkey patch in wsgi.py and have removed the line to monkey patch in app.py. But st2stream was the odd one out.
There was a problem hiding this comment.
Problem might be fixed already in 3.5dev. Putting back to draft whilst investigate further.
Upgraded a 3.4.1 on CentOS 8 that was having problem to 3.5 dev, and couldn't reproduce - so although this change fixed 3.4.1, might not be required on 3.5dev.
Will look to investigate monkey patch changes in 3.5.dev...
There was a problem hiding this comment.
Yeah, st2stream is a bit special since it keeps a long running connections open and IIRC there were issues related to gracefully shutting down the server when performing monkey patching in the main gunicorn process as well.
Having said that - I do think your change is generally correct and right one (we do want to perform monkey patching as early as possible, before importing anything else), but if possible we should verify it doesn't negatively affect the server shutdown logic and / or only do that if we are running in the worker and not the main gunicorn process.
There was a problem hiding this comment.
Also, with this logic we will call monkey_patch() twice in case stream service is started using regular API endpoint and not using gunicorn (st2stream/bin/st2stream), but this likely should not be an issue.
There was a problem hiding this comment.
@Kami It looks like this is fixed by 3.5dev-31 (4922329). This is the minimalist 3.5dev RPM that I can upgrade my 3.4.1 system to, to resolve the problem.
3.5dev-27 (0f8af40) from unstable repo has the problem. So fixed somewhere between those commits (and it's not just the upgrade of eventlet)
So we shouldn't need to patch 3.5. Am trying to track down what the change is that has fixed this and see if can find same fix for 3.4.1. So we don't have to move the monkey patch on 3.5 as the current master branch is ok - to resolve this problem.
So I'm going to suggest closing this PR and not merging... (Though I'd like to find out what fix could be applied to 3.4.1 to fix it...)
There was a problem hiding this comment.
3.5dev-27 + gunicorn==20.1.0 works.
Will try a 3.4.1 install with gunicorn upgraded to see if that fixes it.
Yeah - 3.4.1 + gunicorn==20.1.0 works.
I might translate this to an issue - so that others know the workaround...
There was a problem hiding this comment.
Per my comment above, I still think it would be better if we can try to move monkey patching to as early as possible (before any other imports) - that's the right way to do it.
Doing it later on has no guarantee it will always work and won't break in the feature. Technically, even if some module obtain a reference to a patched module which is patched later on, patching logic will try to replace that module in sys.module, but there is no guarantee that this will always work correctly (depends on how reference to the module is obtained if some hacks are involved and also what's done with the module before it's monkey patched - technically if it's just imported and not used until post monkey patch it should work, but that's not always the case and sometimes imports have side affects).
|
@amanda11 I tested this change locally using gunicorn and non-gunicorn entry point with active connections and it seems to be working correctly, including the shut down phase. So as per my comment above, I think it's probably still a good idea to merge this (to avoid surprises in the future). |
|
Thanks so much for that, I was going to do some further testing after last comment - but was on leave. But I will merge them if all good, and it's great to have had someone test who knew about previous problems - as didn't want to break it! |
|
@Kami - un-drafted - so just need an approval review and then I can merge this. |
|
Just realised I missed the CHANGELOG! - will add that and then merge... |
pull-request-size
bot
added
size/S
Perform monkey patch as the very first thing - even before importing os module.
…to monkey_patch_st2stream
When using SSL to connect to mongo using certificates, was getting recursion error in st2stream, and therefore commands like
st2 pack installwould hang.
Example of error:
Applying same fix as made to st2api under #4834 to st2stream resolved the issue.