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

Refactor GIE Launching to use -P #790

Merged
merged 14 commits into from
Sep 25, 2015

Conversation

hexylena
Copy link
Member

TL;DR

Why

This is a large refactor to use the -P option instead of -p. This change lets docker automatically and safely bind the port, rather than our hopeful hack which just found an empty port and bound to it. In this way we fix the remaining issue with launching docker containers on a remote host (inability to check used ports) and clean up our codebase.

What's new

This PR removes the DOCKER_PORT variable which was originally used to allow multiple containers to be shared by a single upstream proxy. Originally we had been using apache/nginx as our upstream proxy, however @jmchilton wrote a simple nodejs proxy which replaced that. That proxy had the lovely feature of being able to distinguish users based on their Galaxy session cookie, and route appropriately from there. This meant that we could have multiple things mapped to the same route, e.g. 100 users could all access $Proxy_Url/ipython/tree and see different results as the nodejs proxy handled fanning them out to the right backends. Thus, the DOCKER_PORT that was used in our URLs became pointless.

This PR introduces the PROXY_PREFIX variable which now allows the nodejs proxy to bind and listen under a prefix such as /galaxy/gie_proxy. This is extremely important for enterprise deployments where we have galaxy under a cookie path. Trying to direct users to /ipython will consistently fail because the browser refuses to send the galaxy session cookie to that path. Now our URLs are constructed as $ProxyUrl/$GalaxyCookiePath/$CustomProxyPrefix/ipython/tree. The nodejs proxy continues to sort out traffic, and we get URLs that can access the correct cookies.

This PR fixes docker-on-another-host which had previously been partially broken. We used a very naïve method to determine whether or not a port was in use, and this method returned factually incorrect results when the containers were launched on a remote host through docker. This PR uses -P which randomly selects a port, and then uses docker inspect to figure out which port was actually assigned.

Interested parties

Y'all work on GIEs in some capacity, so you've been mentioned here in case you're wishing/willing to help us test your specific deployment environments. The images have been pinned to :15.10 following new procedures, however you'll need to change that to :dev if you actually wish to test.

@bgruening @jmchilton @scholtalbers @remimarenco @pvanheus

This is a major refactoring to use the -P option of docker
which should simplify some of our deployment like running docker containers
on another host. In support of galaxyproject#372
Normally the proxy binds to :8800 and runs containers under
:8800/ipython/... which is great... until you have an upstream proxy
like apache wrapping that at the url FQDN/ipython and suddenly your
cookies aren't available because they're specific to /galaxy.

Thus, when an upstream proxy is in use, we correct the proxy's path to:
with :8800/{cookie_path}/gie_proxy/ipython/... which behaves much more
nicely with upstraem proxies.
@jmchilton
Copy link
Member

This is fantastic - this really cleans up a lot of the rough edges with IEs I think and clears out some technical debt that will hopefully allow for even further enhancements down the road.

👍 once the unit tests pass - Galaxy supports Python 2.6 so you cannon use check_output (https://docs.python.org/2/library/subprocess.html#subprocess.check_output).

bgruening and others added 3 commits September 24, 2015 16:00
Use a backport for check_output that is not part of python 2.6
Sorry bjoern, ripped out what you'd added and just used the code because
the doctest was failing for some unknown reason and we didn't ever need
to reuse the function, so ... easier this way
@hexylena
Copy link
Member Author

Okay, passing CI now.

@hexylena
Copy link
Member Author

Also, lest I follow in your footsteps too much @jmchilton ;) I'll make a followup PR sometime soon with more detailed "How GIEs work" to the Galaxy docs folder, since it's release specific information that needs to stay up-to-date.

@jmchilton
Copy link
Member

😡 ... wait are you suggesting that I... yeah... fair enough 😿.

@hexylena
Copy link
Member Author

/teasing

@jmchilton
Copy link
Member

This has a 👍 from me now, I'll wait a day to give @bgruening and others to comment though before merging.

@hexylena
Copy link
Member Author

Alright, sounds good! Thanks!
24. sep. 2015 16.43 skrev "John Chilton" [email protected]:

This has a [image: 👍] from me now, I'll wait a day to give @bgruening
https://github.com/bgruening and others to comment though before
merging.


Reply to this email directly or view it on GitHub
#790 (comment).

@bgruening
Copy link
Member

👍 from my side already talked with Eric about this yesterday.

jmchilton added a commit that referenced this pull request Sep 25, 2015
Refactor GIE Launching to use -P
@jmchilton jmchilton merged commit 6d3ea7e into galaxyproject:dev Sep 25, 2015
@hexylena
Copy link
Member Author

@bgruening did you have a patch for the shared temporary directory stuff? I'm moving the documentation into galaxy/docs (since it's heavily version specific for Galaxy) and saw your bit about sshfs. Just wondering how that's used/remembering you had a patch regarding it. :)

@bgruening
Copy link
Member

@erasche documentation is here: https://wiki.galaxyproject.org/Admin/GIEs and the patch was integrated with your last PR: hexylena@e8db8a1

@hexylena
Copy link
Member Author

@bgruening okay, I saw your docs (thank you!) but forgot about that changeset. Great!

@hexylena hexylena deleted the gie-docker-launch branch December 19, 2016 21:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants