wsgi-args in config

[atarkowska]

This PR allows pre-setting wsgi-args for gunicorn via bin/omero config as requested in https://github.com/openmicroscopy/management_tools/pull/77#issuecomment-158105212

to test:

• bin/omero config set omero.web.wsgi_args -- '--forwarded-allow-ips=127.0.0.1'
• bin/omero web start --wsgi-args="--foo"
• --forwarded-allow-ips should overwrite --wsgi-args, check processes for additional args ps aux | grep gunicorn

ola             50393   0.0  0.2  2515720  15968   ??  S     4:26pm   0:00.07 /usr/local/Cellar/python/2.7.10_2/Frameworks/Python.framework/Versions/2.7/Resources/Python.app/Contents/MacOS/Python /usr/local/bin/gunicorn -D -p /Users/ola/OMERO/ome.git/dist/var/django.pid --bind 127.0.0.1:4080 --workers 5 --worker-connections 1000 --max-requests 0 --forwarded-allow-ips=127.0.0.1 omeroweb.wsgi:application

cc: @manics

Note: change of property name from "omero.web.gunicorn.wsgi_args" to "omero.web.wsgi_args"

[atarkowska]

@sbesson I will push appropriate changes to https://github.com/openmicroscopy/management_tools/pull/77 and remove it from #4341 (comment)

[will-moore]

$ bin/omero web start --wsgi-args="--foo" works OK, but now I need to set other configs up to use gunicorn etc. Will look at that tonight.

[atarkowska]

@will-moore what exactly you are testing? You only should test if wsgi-args passed through omero config are passed correctly to subproces. Grep will show you what was passed, sorry what is not clear here?

[will-moore]

What I was seeing before was nothing (because I hadn't configured to use gunicorn

$ ps aux | grep gunicorn
wmoore          2930   0.7  0.0  2432768    620 s001  S+   10:34pm   0:00.00 grep gunicorn

Now things are looking a bit better:

$ omero web start
...
$ ps aux | grep gunicorn
...
wmoore          3125   0.0  0.1  2446288   7964   ??  S    10:50pm   0:00.05 /usr/local/Cellar/python/2.7.8_1/Frameworks/Python.framework/Versions/2.7/Resources/Python.app/Contents/MacOS/Python /usr/local/bin/gunicorn -D -p /Users/wmoore/Desktop/OMERO/dist/var/django.pid --bind 127.0.0.1:4080 --workers 5 --worker-connections 1000 --max-requests 1 --forwarded-allow-ips=127.0.0.1 omeroweb.wsgi:application

Then I unset and used the --wsgi argument to pass a different IP address...

$ omero config set omero.web.gunicorn.wsgi_args
$ omero web start --wsgi-args="--forwarded-allow-ips=127.0.0.2"

which also worked

$ ps aux | grep gunicorn
wmoore          3351   2.2  0.8  2503560  66124   ??  S    11:03pm   0:01.00 /usr/local/Cellar/python/2.7.8_1/Frameworks/Python.framework/Versions/2.7/Resources/Python.app/Contents/MacOS/Python /usr/local/bin/gunicorn -D -p /Users/wmoore/Desktop/OMERO/dist/var/django.pid --bind 127.0.0.1:4080 --workers 5 --worker-connections 1000 --max-requests 1 --forwarded-allow-ips=127.0.0.2 omeroweb.wsgi:application

Using "--foo" didn't work, perhaps not surprising.
Good to merge.

[atarkowska]

@will-moore because commands above proof that OMERO config takes priority. You need to copy and paste these two commands one after another. Staring web with random args won;t work.

[atarkowska]

Used in https://github.com/openmicroscopy/management_tools/pull/77 tested daily on https://ci.openmicroscopy.org/view/DEV/job/OMERO-DEV-Python27-merge-deploy/ and https://ci.openmicroscopy.org/view/DEV/job/OMERO-DEV-merge-integration-Python27/

Note: Cowfish no longer redirects from http to https by default. It always redirect to index page

[joshmoore]

@aleksandra-tarkowska : one last question re:

bin/omero config set omero.web.gunicorn.wsgi_args -- '--forwarded-allow-ips=127.0.0.1'
bin/omero web start --wsgi-args="--foo"

Why does one have "gunicorn" and one doesn't? Is the goal to support WSGI servers as a specification (in which case should we drop "gunicorn")? or do we want separate implementations (in which case, perhaps just "gunicorn_args")?

[atarkowska]

ok I see what you mean, sure we can drop gunicorn from the first one

[atarkowska]

@joshmoore updated and redeployed https://ci.openmicroscopy.org/view/DEV/job/OMERO-DEV-Python27-merge-deploy/64/console on cowfish/merge

[joshmoore]

Thanks, @aleksandra-tarkowska. Updated description to match.

[will-moore]

New argument names make sense and working fine. Good to merge.

[hflynn]

Do we need a docs update for this? Maybe a note under https://www.openmicroscopy.org/site/support/omero5.2/sysadmins/unix/install-web/install-nginx.html#cmdoption-omero-web-start--wsgi-args ?

[atarkowska]

@hflynn done

[joshmoore]

@aleksandra-tarkowska : comparing https://www.openmicroscopy.org/site/support/omero5.2/sysadmins/unix/install-web/install-nginx.html#cmdoption-omero-web-start--wsgi-args to your comment https://github.com/openmicroscopy/openmicroscopy/pull/4352/files#diff-7e3491c8f527592430b6decfcdc7c4d3R448 -- what is the intended behavior? Assume:

• a user sets via bin/omero config set
• then passes the argument bin/omero web start --wsgi-args

At the moment, the latter will be silently ignored. Is that what you intend? Other options would be:

• to have a warning if the config is set
• to have an error ...
• to combine the options

Seems like some of these could be user-friendlier.

[manics]

Or deprecate bin/omero web start --wsgi-args ?

[atarkowska]

@manics from developer perspective I think both should remain

[atarkowska]

@joshmoore does it make more sense to do them other way around?

bin/omero web start --wsgi-args should take priority on what is in the config already?

[will-moore]

@aleksandra-tarkowska That option certainly makes more sense to me (sorry, should have thought of that when I was testing the PR). That way, you can set your defaults via config, but if you want to override them when you start web then you can.

[joshmoore]

I can definitely live with overriding the other way (arg > config) as well. Unsure if that should warn if the config property is set. If we think we don't need to warn, that probably means that this is just a sensible usage, in which case we might want to follow the arg > config strategy for allowing overriding other parameters moving forward.

[manics]

from developer perspective I think both should remain

OK, could you add a note to omero web start --help, and/or a warning if --wsgi-args is passed, that the recommended way is to use omero.web.wsgi_args?

[atarkowska]

@will-moore @joshmoore the problem with doing this other way around appear if:

• bin/omero config set
• bin/omero web start --wsgi-args
• bin/omero web restart

in this case --wsgi-args won't take priority, I think it is better if the logic remain and add warning.

[will-moore]

I would expect the previous args in bin/omero web start --wsgi-args to be ignored if I did bin/omero web restart.
But I wouldn't expect them to be ignored in bin/omero web start --wsgi-args, even if they've been previously set.

[joshmoore]

@aleksandra-tarkowska : would it be possible to also add --wsgi-args to restart?

[atarkowska]

We don't have any way to store them, do we? That is the reason of that PR to store them in omero properties. Alternatively you need to bin/omero web restart --wsgi-args

[sbesson]

@will-moore: did you have a chance to review this PR? FYI, we realized today with @pwalczysko that this PR being still open has side effects on the usability of the latest eel Webclient.

[atarkowska]

@pwalczysko because @manics restarted nginx few days ago and all the new changes expected by that pr are in. Anyway let's get that asap please

[will-moore]

Sorry I didn't get around to this earlier.... Just testing now...

[will-moore]

Testing deprecation of --wsgi-args:

$ omero web -h
# and
$ omero web start -h

don't mention --wsgi-args.

If I use ---wsgi-args without setting any config then I don't see any deprecation warning:

$ omero web start --wsgi-args="--forwarded-allow-ips=127.0.0.2"
...
Clearing expired sessions. This may take some time... [OK]
Starting OMERO.web... [OK]

Setting the config first DOES give the deprecation warning:

$ omero config set omero.web.wsgi_args -- '--forwarded-allow-ips=127.0.0.1'
$ omero web start --wsgi-args="--forwarded-allow-ips=127.0.0.2"
...
Starting OMERO.web...  `--wsgi-args` is deprecated and ovewritten by `omero.web.wsgi_args`. [OK]

The documentation change looks fine.
Question: why do we need the extra -- in

$ bin/omero config set omero.web.wsgi_args -- '--forwarded-allow-ips=127.0.0.1'

instead of simply

$ bin/omero config set omero.web.wsgi_args '--forwarded-allow-ips=127.0.0.1'

Will users know to use the extra --?

This staging page https://www.openmicroscopy.org/site/support/omero5.2-staging/sysadmins/unix/install-web/install-nginx.html still has:

Additional settings can be configured from command line arguments:
--wsgi-args WSGI_ARGS
  Additional arguments. For more details check Gunicorn Documentation.

[atarkowska]

@will-moore deprecation warning is fixed now. Doc PR will come separately

[atarkowska]

@joshmoore could you remind us where -- is explained in our doc?

[joshmoore]

There's the warning about whitespace on https://www.openmicroscopy.org/site/support/omero5.2-staging/sysadmins/config.html, but I don't see the mention of --: @sbesson @ximenesuk @hflynn ?

[hflynn]

Autogen build needed to update the docs for this page, I'll check the jenkins configuration.

[hflynn]

Once Travis is green I'll re-run the docs merge build to include the snoopy autogen branch, the changes should then be on staging. Once this is merged, we'll need to open an autogen PR to migrate the changes into the docs.

[sbesson]

@will-moore: the short explanation is that if a configuration has to be set to a value starting with a double dash, i.e. set KEY to --VALUE, bin/omero config set KEY --VALUE will interpret VALUE as an option of the config set subcommand and will fail. To prevent this, you need to prepend the passed value with --.

With this option being exposed, we certainly need to explain this in the doc and likely the top-level of the configuration properties page is the best place to put it?

[pwalczysko]

Please see https://github.com/openmicroscopy/management_tools/pull/77#issuecomment-175845177 - could we "unbreak" eel latest please ?

[joshmoore]

With this option being exposed, we certainly need to explain this in the doc and likely the top-level of the configuration properties page is the best place to put it?

Not that we should include it somewhere, but like the whitespace handling, this is a pretty standard feature of CLI's and could almost be called "by design".

[hflynn]

http://www.openmicroscopy.org/site/support/omero5.2-staging/sysadmins/config.html is now up to date with whatever changes were already on this PR prior to the merge build this morning.

[atarkowska]

Thanks @hflynn. Doc updated as well

[will-moore]

Warning is displayed now, even if no config set

$ omero config set omero.web.wsgi_args
$ omero web start --wsgi-args="--forwarded-allow-ips=127.0.0.2"
...
Starting OMERO.web...  `--wsgi-args` is deprecated and overwritten by `omero.web.wsgi_args`. [OK]

I'm not seeing wsgi-args at http://www.openmicroscopy.org/site/support/omero5.2-staging/sysadmins/config.html (as mentioned on ome/omero-documentation#1390) but docs otherwise looking better.

Good to merge.

[atarkowska]

@will-moore warning is displayed when you put it in cmd omero web start --wsgi-args= no when config is set or unset.

Your test should be:

No warning:

omero web start
Starting OMERO.web...  [OK]

Warning:

omero web start --wsgi-args='...'
Starting OMERO.web...  `--wsgi-args` is deprecated and overwritten by `omero.web.wsgi_args`. [OK]

[atarkowska]

unless that is what you meant, sorry

[will-moore]

Yep ;)

[atarkowska]

sorry lets get that PR in I am tired ;-)

[hflynn]

When this PR is merged, https://ci.openmicroscopy.org/view/Docs/job/OMERO-DEV-latest-docs-autogen/build?delay=0sec needs running with the 'OPEN_PR' box checked.

[joshmoore]

Merging and leaving the docs OPEN_PR step in @sbesson's hands.

[sbesson]

See ome/omero-documentation#1392 for the corresponding autogen documentation PR.