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

non-standard yaml in uwsgi config #8558

Closed
nuwang opened this issue Sep 2, 2019 · 5 comments
Closed

non-standard yaml in uwsgi config #8558

nuwang opened this issue Sep 2, 2019 · 5 comments
Labels
area/configuration Galaxy's configuration system kind/bug

Comments

@nuwang
Copy link
Member

nuwang commented Sep 2, 2019

summary
This issue is to track whether it's now possible to switch uwsgi to libyaml so standard parsing tools can work with galaxy.yml without running the risk of losing certain uwsgi entries.

background
uwsgi's behaviour for parsing yaml files changes based on whether it uses its internal parser or the libyaml parser: unbit/uwsgi#863. The internal parser follows a non-standard yaml syntax for arrays and allows keys to be repeated, which are lost when read with a standard parser. Examples of potentially problematic entries are static-map and socket.

In the uwsgi section of galaxy.yml, the internal parser syntax is being used. My understanding was that this was originally because of a packaging issue but hopefully @natefoo could clarify whether this has now been resolved.

issues
One issue is that since this syntax is not correct yaml, it's difficult to integrate with Helm natively, other than by treating the entire galaxy.yml file as a single text string: https://github.com/galaxyproject/galaxy-helm/blob/f55f9a54c3292949d842332db9d275de06a3ba6f/galaxy/values.yaml#L170

This is a problem because when helm tracks value changes, it now tracks the entire galaxy.yml file as a single string, instead of tracking changes to individual keys, which removes some useful features like letting helm set individual keys, or track individual user modified values.

@jmchilton suggested a good workaround which was to pass the repeated static-map keys as uwsgi commandline parameters, thus removing those problematic entries from galaxy.yml and allowing it to be treated as standard yaml again. We are in the process of implementing this workaround, but with the caveat that it could still result in a few surprises for anyone who needs to deal with these problematic keys (hopefully very few).

Still, the main issue would be that doing some processing on galaxy.yml, such as reading it using libyaml and writing it back, would result in data loss.

@natefoo
Copy link
Member

natefoo commented Sep 6, 2019

In the uwsgi section of galaxy.yml, the internal parser syntax is being used. My understanding was that this was originally because of a packaging issue

It was done because if you pip install uwsgi without pointing it at our wheels, you are most likely going to build a non-libyaml uWSGI. I didn't want configs for our wheels to be incompatible with the upstream uWSGI package in PyPI.

For the k8s case, can you install your own (now pyuwsgi) uWSGI that includes libyaml?

@nuwang
Copy link
Member Author

nuwang commented Sep 7, 2019

@natefoo That's a great idea and so obvious in retrospect! :-) Can't see why not since it's a docker container.

WRT to the uwsgi default parser, we also ran across more weird parsing. For example:

mount: '\=galaxy.webapps.galaxy.buildapp:uwsgi_app()'

would result in uwsgi attempting to use a mount of '\ instead of just \. Is there already an upstream uwsgi bug tracking this or should we open a new one?

@afgane
Copy link
Contributor

afgane commented Sep 9, 2019

Was talking to @almahmoud and we realized it's not clear from this conversation if the use of the libyaml parser prevents use of duplicate keys in galaxy.yaml or not?
If duplicates are not allowed, that implies Galaxy configs will not be interchangeable because they depend on the version of dependencies that were installed? Which leads to two versions of the documentation for Galaxy?

@natefoo
Copy link
Member

natefoo commented Sep 10, 2019

@nuwang uWSGI's internal "YAML" parser does not support quoting, or even typing, really. It mostly treats everything in the value as a string, quotes and all.

@afgane This is exactly the problem, a config that works for a libyaml uWSGI won't work with a non-libyaml uWSGI, and vice-versa. The galaxyproject.galaxy Ansible role can write either syntax but it still needs to know which uWSGI you have.

almahmoud pushed a commit to CloudVE/galaxy-helm that referenced this issue Nov 5, 2019
nsoranzo added a commit to jmchilton/galaxy that referenced this issue Jan 11, 2020
xref. galaxyproject#8558

For other YAML files, `ordered_load()` will raise an exception when it
finds a mapping with duplicate keys.
nsoranzo added a commit to jmchilton/galaxy that referenced this issue Jan 11, 2020
xref. galaxyproject#8558

For other YAML files, `ordered_load()` will raise an exception when it
finds a mapping with duplicate keys.
mvdbeek pushed a commit to mvdbeek/galaxy that referenced this issue Jan 13, 2020
xref. galaxyproject#8558

For other YAML files, `ordered_load()` will raise an exception when it
finds a mapping with duplicate keys.
mvdbeek pushed a commit to mvdbeek/galaxy that referenced this issue Jan 13, 2020
xref. galaxyproject#8558

For other YAML files, `ordered_load()` will raise an exception when it
finds a mapping with duplicate keys.
@jmchilton jmchilton added area/configuration Galaxy's configuration system kind/bug labels Oct 21, 2020
@nuwang
Copy link
Member Author

nuwang commented Nov 16, 2021

This can be worked around by separating the uwsgi config from the galaxy config as follows:

uwsgi --yaml /galaxy/server/config/uwsgi.yml --set galaxy_config_file=/galaxy/server/config/galaxy.yml

@nuwang nuwang closed this as completed Nov 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/configuration Galaxy's configuration system kind/bug
Projects
None yet
Development

No branches or pull requests

4 participants