Skip to content
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

Production WSGI server #299

Merged
merged 3 commits into from
Nov 5, 2021
Merged

Production WSGI server #299

merged 3 commits into from
Nov 5, 2021

Conversation

Starkteetje
Copy link
Member

@Starkteetje Starkteetje commented Aug 31, 2021

Use a production WSGI server to host the WSGI Connaisseur application. Load and stress testing led us to choose cheroot by a slim margin.

PR also contains some loadtests for the pipeline integration tests

@codecov-commenter
Copy link

codecov-commenter commented Aug 31, 2021

Codecov Report

Merging #299 (a45aeac) into develop (b91d745) will decrease coverage by 2.74%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #299      +/-   ##
===========================================
- Coverage    96.99%   94.24%   -2.75%     
===========================================
  Files           21       22       +1     
  Lines         1064     1095      +31     
===========================================
  Hits          1032     1032              
- Misses          32       63      +31     
Impacted Files Coverage Δ
connaisseur/__main__.py 0.00% <0.00%> (ø)
connaisseur/flask_application.py 91.46% <ø> (ø)
connaisseur/logging_wrapper.py 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b91d745...a45aeac. Read the comment docs.

@xopham xopham linked an issue Sep 1, 2021 that may be closed by this pull request
@xopham xopham mentioned this pull request Sep 2, 2021
13 tasks
@phbelitz phbelitz added this to the v2.2.0 milestone Sep 2, 2021
@Starkteetje Starkteetje force-pushed the prod-wsgi branch 3 times, most recently from 55583bd to c68fcec Compare September 17, 2021 16:06
Copy link
Collaborator

@xopham xopham left a comment

Choose a reason for hiding this comment

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

wow...awesome review!

This seems to give a tendency towards cheroot which seems a good choice though less known.

However, when checking these out I stumbled across the following blog post: https://fgimian.github.io/blog/2018/05/17/choosing-a-fast-python-api-framework/

In there, it is mentioned that CherryPy (Cheroot) is ruled out bc it was slow, but more importantly that flask was ruled out as a WSGI framework since it was slow and that falcon is apparently blazingly fast and "The no-nonsense REST API and microservices framework for Python developers," which sounds pretty good for our use-case. Now, this will cause some additional effort to test whether the framework makes a significant difference, as that would indeed require some rewriting, but I am wondering whether it makes sense to take a look.
Also, Bjoern is claimed to work for python 2 and 3: https://github.com/jonashaag/bjoern . Apparently, it is a strong choice for containerized environments:

Beyond that I noticed that uwsgi is supposedly very complex and overloaded.

In summary, it seems cheroot is a decent choice, Bjoern may be better and apparently using Falcon instead of Flask could strongly affect speed, but would require a bit of rewriting...

docs/adr/ADR-7_wsgi-server.md Outdated Show resolved Hide resolved
@Starkteetje
Copy link
Member Author

https://github.com/jonashaag/bjoern/blob/9a6409138c645c47f6fd1d161e84a40daebabb04/bjoern/request.c#L354
bjoern doesnt do TLS though which isnt acceptable for the admission controller afaik

@Starkteetje Starkteetje linked an issue Oct 15, 2021 that may be closed by this pull request
@phbelitz phbelitz removed this from the v2.2.0 milestone Oct 15, 2021
@xopham xopham mentioned this pull request Oct 22, 2021
3 tasks
@Starkteetje Starkteetje force-pushed the prod-wsgi branch 5 times, most recently from 4dae7a3 to fe4f811 Compare October 22, 2021 14:46
@Starkteetje Starkteetje force-pushed the prod-wsgi branch 10 times, most recently from 1d8d056 to a8e692f Compare November 5, 2021 12:03
@Starkteetje Starkteetje marked this pull request as ready for review November 5, 2021 12:15
phbelitz
phbelitz previously approved these changes Nov 5, 2021
Copy link
Collaborator

@xopham xopham left a comment

Choose a reason for hiding this comment

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

one small version comment, otherwise lgtm!

helm/values.yaml Outdated Show resolved Hide resolved
Copy link
Collaborator

@annekebr annekebr left a comment

Choose a reason for hiding this comment

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

Looks very good! Can we delete flask from the requirements?

docs/adr/ADR-7_wsgi-server.md Outdated Show resolved Hide resolved
docs/adr/ADR-7_wsgi-server.md Outdated Show resolved Hide resolved
@Starkteetje
Copy link
Member Author

Looks very good! Can we delete flask from the requirements?

No, since we're still using the flask WSGI application. Just no longer the server. But that's a great point, gotta rename the flask_server file

This commit adds the documentation for the WSGI server choice including the tests run and our decision
This commit exchanges the previously used Flask server with the Cheroot WSGI server and ensures compatible logging. The Flask server was not intended as a production server, which is why we did the change.

Fix #11
This commit adds a loadtest to the GitHub actions pipeline to ensure the Connaisseur performance when under stress

Fix #154
@Starkteetje Starkteetje merged commit cf8cc75 into develop Nov 5, 2021
@Starkteetje Starkteetje deleted the prod-wsgi branch November 5, 2021 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement a minimal load testing Deployment on Production Environment
5 participants