Skip to content

Stop hang when use mongo SSL #5240

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.

Sign up for GitHub

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

Merged
amanda11 merged 10 commits into from
Apr 22, 2021
Merged

Stop hang when use mongo SSL #5240

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
d974873
Stop hang when use mongo SSL
amanda11 Apr 19, 2021
c1a0296
Fix black
amanda11 Apr 19, 2021
ceefe7a
Update CHANGELOG.rst
amanda11 Apr 21, 2021
b287836
Merge branch 'master' into monkey_patch_st2stream
amanda11 Apr 21, 2021
ae326c9
Update wsgi.py
Kami Apr 22, 2021
b4027f3
Merge branch 'master' into monkey_patch_st2stream
Kami Apr 22, 2021
58e543f
Merge branch 'master' of github.com:StackStorm/st2 into monkey_patch_…
Kami Apr 22, 2021
2a71ff0
Remove testing change, also run micro benchmarks under 3.8.
Kami Apr 22, 2021
67b0051
Merge branch 'master' into monkey_patch_st2stream
Kami Apr 22, 2021
3df9f4d
Merge branch 'monkey_patch_st2stream' of github.com:StackStorm/st2 in…
Kami Apr 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 5 additions & 6 deletions .github/workflows/microbenchmarks.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,6 @@
name: Micro Benchmarks

on:
pull_request:
type: [opened, reopened, edited]
branches:
# Only for PRs targeting those branches
- master
- v[0-9]+.[0-9]+
schedule:
- cron: '30 3 * * *'

Expand Down Expand Up @@ -43,6 +37,11 @@ jobs:
nosetests_node_total: 1
nosetests_node_index: 0
python-version: '3.6'
- name: 'Microbenchmarks'
task: 'micro-benchmarks'
nosetests_node_total: 1
nosetests_node_index: 0
python-version: '3.8'
services:
mongo:
image: mongo:4.4
Expand Down
4 changes: 4 additions & 0 deletions CHANGELOG.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,10 @@ Changed

Contributed by @chadpatt .

* Monkey patch on st2stream earlier in flow #5240

Contributed by Amanda McGuinness (@amanda11 Ammeon Solutions)

Improvements
~~~~~~~~~~~~

Expand Down
5 changes: 0 additions & 5 deletions st2stream/st2stream/app.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@
from st2common.middleware.instrumentation import RequestInstrumentationMiddleware
from st2common.middleware.instrumentation import ResponseInstrumentationMiddleware
from st2common.router import Router
from st2common.util.monkey_patch import monkey_patch
from st2common.constants.system import VERSION_STRING
from st2common.service_setup import setup as common_setup
from st2common.util import spec_loader
Expand All @@ -47,10 +46,6 @@ def setup_app(config={}):

is_gunicorn = config.get("is_gunicorn", False)
if is_gunicorn:
# Note: We need to perform monkey patching in the worker. If we do it in
# the master process (gunicorn_config.py), it breaks tons of things
# including shutdown
monkey_patch()

st2stream_config.register_opts(ignore_errors=True)
capabilities = {
Expand Down
4 changes: 4 additions & 0 deletions st2stream/st2stream/wsgi.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,10 @@
# See the License for the specific language governing permissions and
# limitations under the License.

from st2common.util.monkey_patch import monkey_patch

monkey_patch()
Copy link
Member

@Kami Kami Apr 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@Kami Kami Apr 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

@amanda11 amanda11 Apr 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@amanda11 amanda11 Apr 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Member

@Kami Kami Apr 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@Kami Kami Apr 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@amanda11 amanda11 Apr 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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...)

Copy link
Contributor Author

@amanda11 amanda11 Apr 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Member

@Kami Kami Apr 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).


import os

from st2stream import app
Expand Down