Skip to content
This repository has been archived by the owner. It is now read-only.

shutdown idle containers #10

Merged
merged 3 commits into from Sep 20, 2014
Merged

shutdown idle containers #10

merged 3 commits into from Sep 20, 2014

Conversation

@minrk
Copy link
Member

minrk commented Sep 18, 2014

like it says on the tin.

every hour (configurable), it checks for idle containers and brings them down, removing the proxy route.

rando.py Outdated
@@ -89,9 +135,11 @@ def create_notebook_server(self, base_path):
env = {"RAND_BASE": base_path}
container_id = docker_client.create_container(image="jupyter/tmpnb",
environment=env)
docker_client.start(container_id, port_bindings={8888: ('127.0.0.1',)})
docker_client.start(container_id, port_bindings={8888: ('0.0.0.0',)})

This comment has been minimized.

Copy link
@rgbkrk

rgbkrk Sep 18, 2014

Member

Did you mean to change this to 0.0.0.0? I'd rather not have these notebooks be public by default.

This comment has been minimized.

Copy link
@minrk

minrk Sep 18, 2014

Author Member

I guess I didn't. Without this, I can't run the server under boot2docker. The containers never become accessible.

This comment has been minimized.

Copy link
@rgbkrk

rgbkrk Sep 18, 2014

Member

Hmmm, maybe we make this configurable, leave the default as 127.0.0.1.

@ellisonbg
Copy link

ellisonbg commented Sep 19, 2014

LOL - upon seeing this Olivier said "great, I am glad you implemented it this way, that was I can write some javascipt that keeps it alive..."

rando.py Outdated
for base_path in data:
container_id = containers.pop(base_path.lstrip('/'), None)
if container_id:
app_log.info("shutting down container %s at %s", container_id, base_path)

This comment has been minimized.

Copy link
@rgbkrk

rgbkrk Sep 19, 2014

Member

Something we could do in the future here is also dump the logs of the container before we obliterate it.

if container_id:
app_log.info("shutting down container %s at %s", container_id, base_path)
docker_client.stop(container_id)
docker_client.remove_container(container_id)

This comment has been minimized.

Copy link
@rgbkrk

rgbkrk Sep 19, 2014

Member

Sweet, thanks for removing the container too. That will save storage space as well.

rando.py Outdated
)

# check for idle containers to cull every five minutes
cull_timeout = 300

This comment has been minimized.

Copy link
@rgbkrk

rgbkrk Sep 19, 2014

Member

Personally, I'm of the opinion we should be more generous at the beginning here (for our own sanity especially), maybe do 1200 for 20 minutes instead. @ellisonbg was even aiming for like 12 hours.

rando.py Outdated
delta = datetime.timedelta(seconds=cull_timeout)
cull_ms = cull_timeout * 1e3

culler = tornado.ioloop.PeriodicCallback(

This comment has been minimized.

Copy link
@rgbkrk

rgbkrk Sep 19, 2014

Member

This is cool, I didn't know about this.

This comment has been minimized.

Copy link
@ellisonbg

ellisonbg Sep 19, 2014

Thinking more about the delay - I do think we can start out more generous
than 10 minutes - but 12 hours is probably overboard. I think something in
the 1 hour range would be good.

On Thu, Sep 18, 2014 at 7:20 PM, Kyle Kelley notifications@github.com
wrote:

In rando.py:

 )
  • check for idle containers to cull every five minutes

  • cull_timeout = 300
  • delta = datetime.timedelta(seconds=cull_timeout)
  • cull_ms = cull_timeout * 1e3
  • culler = tornado.ioloop.PeriodicCallback(

This is cool, I didn't know about this.


Reply to this email directly or view it on GitHub
https://github.com/jupyter/tmpnb/pull/10/files#r17766549.

Brian E. Granger
Cal Poly State University, San Luis Obispo
@ellisonbg on Twitter and GitHub
bgranger@calpoly.edu and ellisonbg@gmail.com

This comment has been minimized.

Copy link
@rgbkrk

rgbkrk Sep 19, 2014

Member

Let's do two hours.

@@ -1,4 +1,4 @@
#!/usr/bin/env bash
export CONFIGPROXY_AUTH_TOKEN=`head -c 30 /dev/urandom | xxd -p`
configurable-http-proxy --default-target=http://localhost:9999 &
python rando.py
python rando.py $@

This comment has been minimized.

Copy link
@rgbkrk

rgbkrk Sep 19, 2014

Member

In the version that's running right now, I'm actually managing configurable-http-proxy and rando as two separate supervisor setups.

This comment has been minimized.

Copy link
@rgbkrk

rgbkrk Sep 19, 2014

Member

Are you passing arguments to tornado I should be using?

This comment has been minimized.

Copy link
@minrk

minrk Sep 19, 2014

Author Member

Just --logging=debug. I will also pass the --container-ip=0.0.0.0 when that exists.

This comment has been minimized.

Copy link
@rgbkrk

rgbkrk Sep 20, 2014

Member

If you want to set the dev settings here too, I'm OK with that. The deployed version is using supervisor.

@rgbkrk rgbkrk force-pushed the jupyter:master branch from c9084b7 to c2c0ce9 Sep 19, 2014
@minrk minrk changed the title shutdown containers that have been idle for at least five minutes shutdown idle containers Sep 20, 2014
minrk added 3 commits Sep 18, 2014
use the proxy's inactive_since REST API
and kill the proxy if the server exits
also changes default cull timeout to 1 hr
@minrk minrk force-pushed the minrk:cull branch from 2d9082c to 96a8fbc Sep 20, 2014
@minrk
Copy link
Member Author

minrk commented Sep 20, 2014

now configurable, with a default of one hour.

port = docker_client.port(container_id, 8888)[0]['HostPort']

self.settings['containers'][base_path] = container_id

This comment has been minimized.

Copy link
@rgbkrk

rgbkrk Sep 20, 2014

Member

Ah, you're tracking which paths map to which containers!

Maybe I should just close #13 then and make use of the container IDs elsewhere.

@rgbkrk
Copy link
Member

rgbkrk commented Sep 20, 2014

This PR closes all containers created by this process. Upon a hot reload of tornado (change of the python script), does it keep the old values?

If we restart the process through other means, I take it we'll have to do maintenance to clean out old containers.

rgbkrk added a commit that referenced this pull request Sep 20, 2014
shutdown idle containers
@rgbkrk rgbkrk merged commit db53b0d into jupyter:master Sep 20, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.