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

Use idle culler from jupyterhub-idle-culler package #1648

Merged
merged 2 commits into from
Apr 29, 2020

Conversation

yuvipanda
Copy link
Collaborator

@yuvipanda yuvipanda commented Apr 27, 2020

The idle culler lives as a script in at least 3 different
places:

The idle culler is a core integral part of every JupyterHub deployment
these days. It would be great if it was maintained separately on
its own, without being split across multiple repos.

The latest changes had been to the version in the JupyterHub repo, so I
copied it (while preserving commit history, because credit is important)
to a new repository: https://github.com/yuvipanda/jupyterhub-idle-culler

I looked through z2jh and tljh copies, and cherry-picked the following
changes manually

ae80fb5
836f19a
a0787c6
b230ef8
20374db#diff-f00cd100e9f673285208aaa6fc0c3212

There were a few from https://github.com/jupyterhub/zero-to-jupyterhub-k8s/commits/9c15a42b1227f3b54826f273f1689e4dc8c8e12e/images/hub/cull_idle_servers.py
I could not apply, but mostly because those features had been
re-implemented already.

Right now, the package is a direct port of the code we had. Once
this settles in, I am hopefull we can iterate faster and make cool
new changes.

The idle culler lives as a script in at least 3 different
places:

    In the JupyterHub repo, as an 'example'
    https://github.com/jupyterhub/jupyterhub/tree/d126baa443ad7d893be2ff4a70afe9ef5b8a4a1a/examples/cull-idle
    In the TLJH repo, as a core part of the service
    https://github.com/jupyterhub/the-littlest-jupyterhub/blob/01ba34857dd4e316d839034ae2b3cc400b929964/tljh/cull_idle_servers.py.
    This is an import from a specific version of the JupyterHub repo,
    and has had a couple of changes made to it since.
    In the z2jh repo, as a core part of the service
    https://github.com/jupyterhub/zero-to-jupyterhub-k8s/blob/c3f3be25f8ae6c72d02f385f41983b70ee1d416e/jupyterhub/files/hub/cull_idle_servers.py
    This is also an import from a specific version of the JupyterHub
    repo, but has had a lot more work done on it. Most had been sync'd
    back the JupyterHub repo, but some had not been. See
    https://github.com/jupyterhub/zero-to-jupyterhub-k8s/commits/9c15a42b1227f3b54826f273f1689e4dc8c8e12e/images/hub/cull_idle_servers.py
    and https://github.com/jupyterhub/zero-to-jupyterhub-k8s/commits/master/jupyterhub/files/hub/cull_idle_servers.py

The idle culler is a core integral part of every JupyterHub deployment
these days. It would be great if it was maintained separately on
its own, without being split across multiple repos.

The latest changes had been to the version in the JupyterHub repo, so I
copied it (while preserving commit history, because credit is important)
to a new repository: https://github.com/yuvipanda/jupyterhub-idle-culler

I looked through z2jh and tljh copies, and cherry-picked the following
changes manually

jupyterhub/zero-to-jupyterhub-k8s@ae80fb5
jupyterhub/zero-to-jupyterhub-k8s@836f19a
jupyterhub/zero-to-jupyterhub-k8s@a0787c6
jupyterhub/zero-to-jupyterhub-k8s@b230ef8
20374db#diff-f00cd100e9f673285208aaa6fc0c3212

There were a few from https://github.com/jupyterhub/zero-to-jupyterhub-k8s/commits/9c15a42b1227f3b54826f273f1689e4dc8c8e12e/images/hub/cull_idle_servers.py
I could not apply, but mostly because those features had been
re-implemented already.

Right now, the package is a direct port of the code we had. Once
this settles in, I am hopefull we can iterate faster and make cool
new changes.
@yuvipanda
Copy link
Collaborator Author

similar PR in TLJH: jupyterhub/the-littlest-jupyterhub#559

@yuvipanda
Copy link
Collaborator Author

I'll probably move the repo to the JupyterHub org once a few people agree.

@minrk
Copy link
Member

minrk commented Apr 27, 2020

Yay! Please do move it to the jupyterhub org and start using it from there. Then we can finally close jupyterhub/jupyterhub#1791 and update the example in the main repo to point to the new package.

Can we wait for a versioned release before merging it into the image here? (I say move it to the jupyterhub org and release 1.0 immediately with what you have today)

@consideRatio
Copy link
Member

Wieee nice work extracting and merging the culler logic into a package Yuvi!! 🎉

@consideRatio
Copy link
Member

Hi @minrk ❤️! I've missed you!

yuvipanda added a commit to jupyterhub/jupyterhub-idle-culler that referenced this pull request Apr 27, 2020
yuvipanda added a commit to jupyterhub/jupyterhub-idle-culler that referenced this pull request Apr 28, 2020
@yuvipanda
Copy link
Collaborator Author

@minrk I've moved it to the jupyterhub org, released to PyPI and bumped that here!

@consideRatio
Copy link
Member

Let's try it out, we lack testing for this at the moment so eyes open for if this works as intended!

Thanks for this excellent refactor @yuvipanda!

@consideRatio consideRatio merged commit fcf9c2e into jupyterhub:master Apr 29, 2020
@yuvipanda
Copy link
Collaborator Author

yw, @consideRatio. Thanks for the merge

@mriedem
Copy link
Contributor

mriedem commented Jul 9, 2020

Just a note but we upgraded to https://jupyterhub.github.io/helm-chart/jupyterhub-0.9.0-n131.hd080b1d.tgz today to pick up an unrelated change and our hub-managed cull-idle service is failing with this:

Jul 9 15:33:01 hub-5db954b774-v7qsv hub [I 2020-07-09 20:33:01.013 JupyterHub service:335] Starting service 'cull-idle': ['python3', '-m', 'jupyterhub_idle_culler', '--url=http://localhost:8081/hub/api', '--timeout=432000', '--cull-every=3600', '--concurrency=1', '--cull-users']
Jul 9 15:33:01 hub-5db954b774-v7qsv hub [I 2020-07-09 20:33:01.018 JupyterHub service:121] Spawning python3 -m jupyterhub_idle_culler --url=http://localhost:8081/hub/api --timeout=432000 --cull-every=3600 --concurrency=1 --cull-users
Jul 9 15:33:01 hub-5db954b774-v7qsv hub [D 2020-07-09 20:33:01.026 JupyterHub spawner:1113] Polling subprocess every 30s
Jul 9 15:33:01 hub-5db954b774-v7qsv hub /usr/bin/python3: No module named jupyterhub_idle_culler 

This is unsurprising since we build our own hub image because we have a custom authenticator and we just need to reflect the requirements.txt change in our hub image build, but if there is a way to flag this for upgrade in the release notes it'd probably be worth it.

Having said that, I'm happy to see the cull idle scripts get cat-herded into a separate repo so 👍 for your work.

@consideRatio
Copy link
Member

This is unsurprising since we build our own hub image because we have a custom authenticator and we just need to reflect the requirements.txt change in our hub image build, but if there is a way to flag this for upgrade in the release notes it'd probably be worth it.

Good point to make a note of this in the changelog! @minrk has done excellent work making it possible for us now to compare the versions of various packages built to the docker image, so I plan that we refer to that.

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.

4 participants