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

Python/Flask - Replaces gunicorn with uWSGI #463

Merged
merged 14 commits into from
Mar 17, 2021
Merged

Python/Flask - Replaces gunicorn with uWSGI #463

merged 14 commits into from
Mar 17, 2021

Conversation

askmeegs
Copy link
Collaborator

@askmeegs askmeegs commented Mar 4, 2021

Note - I created an uptime check for my own long-standing deployment of this PR. Before merging, I want to make sure the readiness probe issues don't recur. Given no probe failures, this PR will be safe to merge on Thursday, 3/18

Fixes the ongoing readiness + liveness probe issues for the Python services (#447), which seem to be caused by this gunicorn sockets bug. While the gunicorn bug has been fixed, they haven't put out a release yet, and haven't since 2019. After conferring w/ team, seems best to switch the Flask WSGI server to uWSGI - this doesn't affect the Flask app itself or the Python APIs. Just the server fronting the Flask app.

Changelog

(for each Python service - contacts, userservice, frontend)

  • replaces gunicorn server startup CMD in Dockerfile with uwsgi server startup
  • removes logging.conf - flask log date formatting now inline in python
  • adds uwsgi.ini which is the uwsgi config file. hardcodes # threads (same # as we used for gunicorn), date format, etc. -- port configurable from dockerfile.

@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Mar 4, 2021
@askmeegs askmeegs changed the title Python/Flask - Replaces gunicorn WSGI server with uwsgi Python/Flask - Replaces gunicorn with uWSGI Mar 4, 2021
module = contacts:create_app()
die-on-term = true

disable-logging=True
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note - this logging only refers to the uwsgi access logs, which happen on every request and were noisy. And I couldn't find a way to match them to the user-provided log level, as we do with the flask app logs. So I turned them off for now.

But the Flask logs are intact / unchanged, still respond to log level.

@askmeegs askmeegs changed the title Python/Flask - Replaces gunicorn with uWSGI [WIP] Python/Flask - Replaces gunicorn with uWSGI Mar 4, 2021
@github-actions
Copy link

github-actions bot commented Mar 4, 2021

🚀 PR staged at http://35.226.168.122

@askmeegs askmeegs changed the title [WIP] Python/Flask - Replaces gunicorn with uWSGI [do not merge] Python/Flask - Replaces gunicorn with uWSGI Mar 4, 2021
Copy link
Member

@Shabirmean Shabirmean left a comment

Choose a reason for hiding this comment

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

LGTM! 👌🏼

Does the PR deployments also have the loadgenerator running?

Copy link
Member

@bourgeoisor bourgeoisor left a comment

Choose a reason for hiding this comment

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

LGTM! Just a general question as well as a comment on the linter rules.

@@ -5,4 +5,5 @@ disable=F0401, # import loading
E1101, # function members
R0801, # duplicate code
R0903, # public methods
R0915 # too many statements
R0915, # too many statements
R0914 # too many local variables
Copy link
Member

Choose a reason for hiding this comment

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

Are we using an open source Google style guide for the linter?
If so, we should strongly consider disabling as few rules as possible, else it sorts of defeats the purpose (of the linter).
I'm not too sure about pylint specifically but there should also be a way to disable a certain rule locally, for a tiny chunk of code where the rule just doesn't make sense, while keeping it enabled elsewhere.
Thoughts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Filed a follow-up issue for this! #465

@@ -1,4 +1,4 @@
# Copyright 2020 Google LLC
# Copyright 2021 Google LLC
Copy link
Member

Choose a reason for hiding this comment

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

General question: do we typically modify the copyright year per-file on-change, or do we try to keep all of them up-to-date as much as possible?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I try to manually update to the current year anytime I change the file. The dev-kubernetes-manifests have to get manually updated for the new year, before generating the next release

@askmeegs
Copy link
Collaborator Author

askmeegs commented Mar 8, 2021

LGTM! 👌🏼

Does the PR deployments also have the loadgenerator running?

Yes!

@askmeegs askmeegs changed the title [do not merge] Python/Flask - Replaces gunicorn with uWSGI Python/Flask - Replaces gunicorn with uWSGI Mar 17, 2021
@github-actions
Copy link

🚀 PR staged at http://35.226.168.122

@askmeegs askmeegs merged commit b5b0024 into master Mar 17, 2021
@askmeegs askmeegs deleted the uwsgi branch March 17, 2021 17:06
j-windsor added a commit to j-windsor/bank-of-anthos that referenced this pull request Jun 9, 2021
askmeegs added a commit that referenced this pull request Jun 22, 2021
…501)

* Revert "Python/Flask - Replaces gunicorn with uWSGI  (#463)"

This reverts commit b5b0024.

* bump gunicorn and adjust dockerfiles

* call locust directly in loadgenerator

* remove escapes from entrypoint

* update copyright year in touched files

Co-authored-by: Megan O'Keefe <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants