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 #559

Merged
merged 4 commits into from May 4, 2020

Conversation

yuvipanda
Copy link
Collaborator

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

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.

  • Add / update documentation
  • Add tests

@yuvipanda
Copy link
Collaborator Author

If all tests pass, I will make a release of the idle culler and reference that instead of the git url

@yuvipanda
Copy link
Collaborator Author

Similar PR in z2jh: jupyterhub/zero-to-jupyterhub-k8s#1648

@yuvipanda
Copy link
Collaborator Author

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

@jtpio jtpio mentioned this pull request Apr 27, 2020
2 tasks
Copy link
Member

@GeorgianaElena GeorgianaElena left a comment

Choose a reason for hiding this comment

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

Yay ❤️
This is great @yuvipanda, thanks for working on it.

P.S: It would be great to have it in the JupyterHub org.

@yuvipanda yuvipanda requested a review from minrk April 27, 2020 13:00
@yuvipanda
Copy link
Collaborator Author

Upgrade tests are failing with JupyterLab builds - I suspect this is another memory issue. I have bumped up the memory limit to 2G to test that, but I hope this doesn't mean we have to bump our minimum requirement to 2G, just for JupyterLab builds :'(

@GeorgianaElena
Copy link
Member

@yuvipanda, unfortunately the upgrade test is failing often when building jupyterlab :( (jupyterlab build command is ran 2 times in this tests). I think this is what happened with the integration test before increasing the min memory. However, this comment says that the 1GB memory might sometimes not be enough even for installation, so I'm confused 😕

Also, part of the solution in the past was this PR.

@yuvipanda
Copy link
Collaborator Author

yeah, 2G fixes it :( I'm trying 1.5G.

See jupyterlab/jupyterlab#4824 for prior work

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
jupyterhub@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.
A v1.0 release has been made!
@yuvipanda
Copy link
Collaborator Author

Looks like I just didn't restart the tests enough times at 1G! I'm going to remove all the commits I made moving the memory limit around.

This is ready to merge! \o/

@yuvipanda
Copy link
Collaborator Author

hmmm, so it fails now but succeeds if I make a few commits that eventually bring the memory requirement to 1G?

I couldn't figure out why JupyterLab fails to build
with 1G on initial commit, but succeeds if you do a bunch
of other commits after. Previously, I binary searched
down from 2G. Here, I just add a new, no-op commit
to see if that helps
@GeorgianaElena
Copy link
Member

@yuvipanda, I've restarted the tests and now they pass again 😕

@GeorgianaElena
Copy link
Member

I think we should merge this PR as it's a great addition ❤️ and open an issue about the tests failing intermittently again (😞)
WDYT?

@yuvipanda
Copy link
Collaborator Author

@GeorgianaElena ah, damn - re: intermittent. At least that makes more sense and I'm not going bananas...

Would love to get this merged and open another issue for the flaky tests!

@consideRatio
Copy link
Member

Things seem to work as intented in the Z2JH project from what I can tell after trying it, and the changes look good to me! Merging given Georgianas approval!

@consideRatio consideRatio merged commit a97adcc into jupyterhub:master May 4, 2020
@yuvipanda yuvipanda deleted the feat/upstream-culler branch May 5, 2020 02:08
@yuvipanda
Copy link
Collaborator Author

Thanks, @GeorgianaElena and @consideRatio!

yuvipanda added a commit to yuvipanda/jupyterhub that referenced this pull request Jul 12, 2020
Has been moved to its own repo.

See jupyterhub/the-littlest-jupyterhub#559
for more info
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

3 participants