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

Replace chp with traefik-proxy #266

Merged
merged 14 commits into from Feb 22, 2019

Conversation

GeorgianaElena
Copy link
Member

@GeorgianaElena GeorgianaElena commented Jan 30, 2019

  • Add / update documentation
  • Add tests

Hi!

I replaced configurable-http-proxy with a proxy that uses traefik and a toml file to store the routes (rules.toml), regarding jupyterhub/traefik-proxy#32.
The proxy is available here.
Also, I managed to find a way to wait for the hub to be fully operational after a restart and got rid of the sleeps in the tests (the 1 sec sleep wasn't enough for traefik proxy)

assert 0 == await (await asyncio.create_subprocess_exec(*TLJH_CONFIG_PATH, 'reload')).wait()
# FIXME: wait for reload to finish & hub to come up
# Should be part of tljh-config reload
await asyncio.sleep(1)

In order to make sure the hub is ready after the restart, I first waited for jupyterhub service to be active and then I waited for the string "JupyterHub is now running at" to be available in jupyterhub's logs(since last restart time). @minrk, @yuvipanda, I'm not sure I chose the right approach for this, so a feedback would be highly appreciated.

Also, I don't understand why the test test_hub.py/test_user_admin_remove is failing. I noticed this test was also failing before this PR.
In the logs I see this:

[SingleUserNotebookApp auth:866] oauth state 'eyJ1dWlkIjogImZhYTAxZmRhODc4YTQ2ZjFhMGE0ZDMzMjE5Njc3YWExIiwgIm5leHRfdXJsIjogIi91c2VyLzhjYjk0ZTBlMzZkOTVjZjYvdHJlZSJ9' != None`
[SingleUserNotebookApp web:1667] 403 GET /user/8cb94e0e36d95cf6/oauth_callback?code=9ec9fca1-0b50-4b23-9692-ad1e222866d0&state=eyJ1dWlkIjogImZhYTAxZmRhODc4YTQ2ZjFhMGE0ZDMzMjE5Njc3YWExIiwgIm5leHRfdXJsIjogIi91c2VyLzhjYjk0ZTBlMzZkOTVjZjYvdHJlZSJ9 (127.0.0.1): oauth state does not match. Try logging in again.

Copy link
Member

@minrk minrk left a comment

Choose a reason for hiding this comment

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

This is great progress! It's super exciting to see this coming together.

tljh/systemd.py Outdated Show resolved Hide resolved
tljh/traefik.py Show resolved Hide resolved
tljh/traefik.toml.tpl Outdated Show resolved Hide resolved
tljh/traefik.toml.tpl Outdated Show resolved Hide resolved
tljh/traefik.toml.tpl Outdated Show resolved Hide resolved
tljh/config.py Outdated Show resolved Hide resolved
@minrk minrk changed the title WIP: Replace chp with traefik-proxy Replace chp with traefik-proxy Feb 11, 2019
Copy link
Member

@minrk minrk left a comment

Choose a reason for hiding this comment

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

This is great! I've tested this locally and it seems to work very well. I'm super pleased with the traefik-proxy so far.

@yuvipanda what do you think about using this unconditionally in tljh? A more complicated option could be to preserve the CHP implementation and allow users to pick the implementation, in case there are issues.

The only downside to traefik-proxy that I'm aware of is the lack of activity tracking. I'm working on an alternate version of that that would work for all proxies in jupyterhub/jupyterhub#2346

bootstrap/bootstrap.py Outdated Show resolved Hide resolved
tljh/traefik.toml.tpl Outdated Show resolved Hide resolved
@yuvipanda
Copy link
Collaborator

yuvipanda commented Feb 11, 2019

@GeorgianaElena this is awesome work! I'm very happy to have it here :)

@minrk I think it's ok to switch to it unconditionally. Here's what I think we should do before that:

How does that sound?

@minrk
Copy link
Member

minrk commented Feb 12, 2019

Sounds good. I'll run a couple of those tests.

Copy link
Member

@minrk minrk left a comment

Choose a reason for hiding this comment

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

I did some review with minor things to change, but this seems to work great!

I was able to run an upgrade using:

export TLJH_BOOTSTRAP_PIP_SPEC=git+https://github.com/GeorgianaElena/the-littlest-jupyterhub@replace_chp_traefik
curl https://raw.githubusercontent.com/GeorgianaElena/the-littlest-jupyterhub/replace_chp_traefik/bootstrap/bootstrap.py   | sudo -E python3

which left the following tasks:

  1. make sure that the old configurable-http-proxy.service is stopped and deleted, if it exists
  2. add passlib and jupyterhub-traefik-proxy to install_requires in setup.py

I opened #277 as a side issue, since bootstrap doesn't work if https is enabled, but disabling https during the upgrade and then re-enabling after worked fine. I had to do /opt/tljh/bin/hub/pip install passlib to get past the missing dependency.

I ran hubtraf with 4 CPUs and 8GB of RAM on GCP. At 60 users, I started to see failures which prompted jupyterhub/traefik-proxy#41 which I think was the culprit (a race between the hub updating the routing table and traefik trying to read it). After installing that branch, 60 users ran without a single failure.

I'll look into parsing the results shortly.

tljh/config.py Outdated Show resolved Hide resolved
tljh/installer.py Show resolved Hide resolved
dev-requirements.txt Outdated Show resolved Hide resolved
tljh/configurer.py Outdated Show resolved Hide resolved
tljh/configurer.py Outdated Show resolved Hide resolved
bootstrap/bootstrap.py Outdated Show resolved Hide resolved
tljh/config.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
tljh/installer.py Outdated Show resolved Hide resolved
@minrk minrk closed this Feb 21, 2019
@minrk minrk reopened this Feb 21, 2019
compute this when we write the template, not when we load config
during load_config

rather than applying directly to defaults, which should be left static
@minrk
Copy link
Member

minrk commented Feb 22, 2019

@GeorgianaElena this is great! I think it's ready to go. I added a small change that fixed the issue, which was that the jupyterhub_config.py wasn't loading the generated password. I also moved the generation of the auth_basic bit to traefik.py where we generate that config file, so that secrets get loaded as part of the 'user' configuration instead of the 'default' configuration, which removed the need for mocking out the generate_traefik_api_credentials

@GeorgianaElena
Copy link
Member Author

Thanks a lot for your help, @minrk!

@minrk
Copy link
Member

minrk commented Feb 22, 2019

Running hubtraf with jupyterhub-traefik-proxy 0.1.2 (needs jupyterhub/traefik-proxy#51) resulted in 0 failures for 20, 40, and 60 users, so I think we are set here.

@minrk minrk merged commit 909dd70 into jupyterhub:master Feb 22, 2019
@minrk
Copy link
Member

minrk commented Feb 22, 2019

Great job, @GeorgianaElena!

@yuvipanda
Copy link
Collaborator

This is awesome, @GeorgianaElena / @minrk! How would you feel about writing a blog post about this work?

@GeorgianaElena
Copy link
Member Author

Thank you @minrk, @yuvipanda!
@yuvipanda , I would really like to give it a try.

@yuvipanda
Copy link
Collaborator

@GeorgianaElena awesome. If you can make an account on medium.com and tell us, we can give you access to send stories to blog.jupyter.org. Then you can write it, a bunch of us can help edit it and publish it. How does that sound?

/cc @choldgraf who is amazing and helps with these things.

@choldgraf
Copy link
Member

definitely more than happy to help writing/editing/whatever's useful!

@minrk
Copy link
Member

minrk commented Mar 1, 2019

@GeorgianaElena's draft is submitted to the Jupyter blog if folks want to give it a review. I think we can post it early next week.

@GeorgianaElena GeorgianaElena deleted the replace_chp_traefik branch June 19, 2020 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants