Add temporary machines support #134
Conversation
nt1m
force-pushed
the
temporary-containers
branch
2 times, most recently
from
f4c3acb
to
d32ef00
Compare
|
@bnjbvr @jankeromnes Not sure who can get to this first, but I'd like to have some feedback on this PR. So this PR allows generating temporary containers (it seems to work in my local testing). I have some questions as well:
|
|
Will try to have a look today! Thanks a lot for doing this 😄 👍 💯 |
|
Ok, I'll have a look at your code.
Without having looked at the code yet, this sounds unrelated to temporary containers. Maybe it's better to make a separate pull request that implements tasks? (It's ok to land it while it's unused, and start using it in a follow-up pull request).
I don't understand this question. Why would you spawn containers "from" an existing container? Also, in general you can access VNC, C9, and any other web service running inside a container by using the usual https://hostname/containerId/port/ URLs. I'll look at your code to see if your question makes more sense. |
| machines.spawnTemporary(session.id, projectId, (error, machine) => { | ||
| if (error) { | ||
| response.statusCode = 500; // Internal Server Error | ||
| response.json({ error: 'Could not create container' }, null, 2); |
There was a problem hiding this comment.
Might want to return after than, in order not to end a response twice.
| } | ||
|
|
||
| machines.spawnTemporary(session.id, projectId, (error, machine) => { | ||
| if (error) { |
There was a problem hiding this comment.
Maybe log the error to know what's happening? E.g. log('[fail] creating temporary container', error).
There was a problem hiding this comment.
It's already logged inside _spawn in machines.js
lib/boot.js
Outdated
| @@ -345,3 +346,13 @@ exports.registerDockerClient = function (next) { | |||
| next(); | |||
| }); | |||
| }; | |||
|
|
|||
| exports.loadTasks = function () { | |||
There was a problem hiding this comment.
Nit: Boot tasks usually take a next callback function. Might want to accept and call it here, just to stay consistent and avoid surprises.
lib/tasks.js
Outdated
| exports.add = function (date, type, data) { | ||
| taskCounter++; | ||
|
|
||
| const dateHash = date.getTime(); |
There was a problem hiding this comment.
Whoops, the var name is a leftover from some older code I wrote.
| @@ -0,0 +1,53 @@ | |||
| const db = require('./db'); | |||
| const log = require('./log'); | |||
| let taskCounter = 0; // Used to generate unique task IDs | |||
There was a problem hiding this comment.
Nit: I would really prefer tasks to be a separate pull request.
Not sure how to explain this. I spawned some containers using the "local" janitor instance I had, but I can't access it using |
nt1m
force-pushed
the
temporary-containers
branch
from
d32ef00
to
4c37db7
Compare
nt1m
force-pushed
the
temporary-containers
branch
from
4c37db7
to
d049b17
Compare
|
(removing myself from reviewers since Jan seems to be on it; thanks for doing that!) |
There was a problem hiding this comment.
Thanks a lot for working on implementing temporary machines! 👍
I'm not very confident about the current approach though. It feels very hacky to have two "types" of machines (some temporary, tied to a session, and others that live on forever, with separate spawn and delete mechanisms).
Instead, I think it would be cleaner to make all containers temporary. But in order not to cause too much trouble with this change, we can start with a very long lifetime (e.g. 6 months or 1 year), and we can also allow bumping the lifetime, or even disabling the automatic removal of a container.
I volunteer for taking over this pull request, as I know Janitor container internals well, and this could allow you to focus more on the new Containers interface, if you wish.
Thanks again!
No description provided.