Gunicorn update #414

fmigneault · 2021-04-07T20:33:44Z

Update to most recent gunicorn version such that security fixes and performance improvements are integrated (resolve [Feature] Update gunicorn version #410)
General docs updates.

important
needs to be properly updated in daccs/pavics, as the necessary binding changes using pserve
config INI with host = localhost does not work
relates to bird-house/birdhouse-deploy#143

codecov-io · 2021-04-07T23:26:20Z

Codecov Report

Merging #414 (5eefef0) into master (8f53482) will decrease coverage by 0.04%.
The diff coverage is 78.57%.

@@            Coverage Diff             @@
##           master     #414      +/-   ##
==========================================
- Coverage   76.01%   75.97%   -0.05%     
==========================================
  Files          66       66              
  Lines        8056     8221     +165     
  Branches     1133     1154      +21     
==========================================
+ Hits         6124     6246     +122     
- Misses       1644     1683      +39     
- Partials      288      292       +4

Impacted Files	Coverage Δ
magpie/app.py	`90.74% <ø> (ø)`
magpie/cli/run_db_migration.py	`52.63% <0.00%> (ø)`
magpie/cli/sync_resources.py	`29.29% <0.00%> (-0.46%)`	⬇️
magpie/cli/sync_services.py	`37.89% <ø> (ø)`
magpie/security.py	`90.00% <0.00%> (ø)`
magpie/cli/register_defaults.py	`62.60% <41.37%> (-6.96%)`	⬇️
magpie/models.py	`87.45% <43.33%> (-5.98%)`	⬇️
magpie/api/webhooks.py	`72.16% <78.00%> (+9.82%)`	⬆️
magpie/db.py	`70.19% <80.00%> (ø)`
magpie/utils.py	`57.66% <81.48%> (+1.89%)`	⬆️
... and 15 more

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 918e4eb...5eefef0. Read the comment docs.

dbyrns

Some minor quirks to be answered, but good overall.

dbyrns · 2021-04-08T16:03:13Z

Dockerfile

 COPY ./config/magpie.ini $MAGPIE_CONFIG_DIR/magpie.ini
 COPY ./env/*.env.example $MAGPIE_ENV_DIR/
 COPY ./magpie $MAGPIE_DIR/magpie/
 # equivalent of `make install` without conda env and pre-installed packages
 RUN pip install --no-dependencies -e $MAGPIE_DIR

 # equivalent of `make cron start` without conda env
-CMD crond -c $CRON_DIR && gunicorn -b 0.0.0.0:2001 --paste $MAGPIE_CONFIG_DIR/magpie.ini --workers 10 --preload
+# bind to '0.0.0.0' such that any IP employed by the server will retrieve the application endpoint by default
+CMD crond -c $CRON_DIR && pserve "$MAGPIE_CONFIG_DIR/magpie.ini"


Number of workers was override to 10 here while set to 3 in the ini file.

It should now be done in the INI. It didn't make much sense to be in the command in the first place.
It is very hard to track that this value was overridden here, and the first impression is usally that whatever was in the INI is the 'truth'.

Yeah, I'm fine with the fact that the INI is the 'truth'. I'm more concerned about the fact that we go from 10 to 3 workers. I would not have said a word if the INI set workers to 10 too.

I can set it by default to 10, so that it will be transparent for anyone that used the image without override of the command.

Yes that's the kind of backward compatibility that we must keep in mind and avoid unnecessary changes when possible.

dbyrns · 2021-04-08T16:04:00Z

config/magpie.ini

@@ -84,7 +84,7 @@ prefix = /magpie

 [server:main]
 use = egg:gunicorn#main
-host = localhost
+host = 0.0.0.0
 port = 2001


We have the now required environment variable MAGPIE_PORT and it is not used.

Yes. I have yet to find a way to simplify this. I have tried defining a "shared" section where magpie.port, and [server:main]'s port would use the same reference, but it was not working.

Ok, then I suggest that you add a small comment telling users that port must be defined at both place.

I still wonder though why MAGPIE_PORT even needs to be defined in your case ?
It is important to remember that MAGPIE_PORT, MAGPIE_URL, etc. are only variable to set the "exposed" URL.
So an application running on http://my-pavics-server.ca should define exactly that value in MAGPIE_URL only.
The port of [server:main] still remains 2001 because it is where the local app is accessible from the docker.

The MAGPIE_PORT should only be 2001 when running locally.

From my perspective I wonder why you did not saw this error on your side 😜, I guess that MAGPIE_URL is not set properly somewhere. But with #418 I'm not worried anymore.
I also suggest that you remove MAGPIE_PORT and fix MAGPIE_URL if required in bird-house/birdhouse-deploy#143 before running tests.

dbyrns · 2021-04-08T16:06:19Z

docs/usage.rst

-For example, these default values should work with ``docker-compose``::
+For example, these default values should work with ``docker-compose``:
+
+.. code-block:: console

    export MAGPIE_TEST_REMOTE_SERVER_URL=http://localhost:2001


Is it still valid since we also need the MAGPIE_PORT variable?

This one is only for tests.
MAGPIE_PORT is needed only if MAGPIE_URL or magpie.url in the INI was not defined.

dbyrns

Everything is set. Thank you.

fmigneault added 3 commits April 7, 2021 13:23

ignore 'dontcare' packages versions

6a34312

update latest gunicorn + change gunicorn->pserve CMD + update docs

e4f63bd

update changelog resolve gunicorn (fixes #410)

dcc1baf

fmigneault requested a review from dbyrns April 7, 2021 20:33

Merge branch 'master' into gunicorn-update

0a48bc0

github-actions bot added ci Something related to code tests, deployment and packaging doc Documentation improvements or building problem tests Test execution or additional use cases labels Apr 7, 2021

fmigneault added 2 commits April 7, 2021 16:35

ignore pyup security warning for bwcompat gunicorn

680f79b

remove non-existing bin/gunicorn in favor of installed pserve

8529b5d

fmigneault added 2 commits April 7, 2021 20:16

fix bind host for docker execution

2757f4d

use important note in changelog

5eefef0

fmigneault added a commit to bird-house/birdhouse-deploy that referenced this pull request Apr 8, 2021

update magpie.ini bind host ip to match Ouranosinc/Magpie#414

991f48b

fmigneault mentioned this pull request Apr 8, 2021

magpie 3.x + gunicorn bind bird-house/birdhouse-deploy#143

Merged

fmigneault added a commit to bird-house/birdhouse-deploy that referenced this pull request Apr 8, 2021

update magpie.ini bind host ip to match Ouranosinc/Magpie#414

12835bd

dbyrns previously approved these changes Apr 8, 2021

View reviewed changes

doc updates related to ports

11fe09b

fmigneault dismissed dbyrns’s stale review via 11fe09b April 9, 2021 00:58

dbyrns approved these changes Apr 9, 2021

View reviewed changes

fmigneault merged commit 486726d into master Apr 9, 2021

fmigneault deleted the gunicorn-update branch April 9, 2021 19:12

fmigneault added a commit to bird-house/birdhouse-deploy that referenced this pull request Apr 21, 2021

update magpie.ini bind host ip to match Ouranosinc/Magpie#414

cb5e11a

fmigneault added a commit to bird-house/birdhouse-deploy that referenced this pull request May 12, 2021

update magpie.ini bind host ip to match Ouranosinc/Magpie#414

161ee06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gunicorn update #414

Gunicorn update #414

fmigneault commented Apr 7, 2021 •

edited

Loading

codecov-io commented Apr 7, 2021 •

edited

Loading

dbyrns left a comment

dbyrns Apr 8, 2021

fmigneault Apr 8, 2021

dbyrns Apr 8, 2021

fmigneault Apr 8, 2021

dbyrns Apr 9, 2021

dbyrns Apr 8, 2021

fmigneault Apr 8, 2021

dbyrns Apr 8, 2021

fmigneault Apr 9, 2021

dbyrns Apr 9, 2021

dbyrns Apr 8, 2021

fmigneault Apr 8, 2021

dbyrns left a comment

Gunicorn update #414

Gunicorn update #414

Conversation

fmigneault commented Apr 7, 2021 • edited Loading

codecov-io commented Apr 7, 2021 • edited Loading

Codecov Report

dbyrns left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbyrns left a comment

Choose a reason for hiding this comment

fmigneault commented Apr 7, 2021 •

edited

Loading

codecov-io commented Apr 7, 2021 •

edited

Loading