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
Add a Docker Latent BuildSlave implementation #1344
Conversation
found = self._image_exists(docker_client) | ||
if (not found) and (self.dockerfile is not None): | ||
log.msg("Image '%s' not found, building it from scratch" % self.image) | ||
for line in docker_client.build(fileobj=BytesIO(self.dockerfile), tag=self.image): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might want to encode self.dockerfile
to 'utf-8' here.
I'm pretty happy with the result. A throw-away slave that start within no time, and takes no CPU when sleeping, ideal ! |
- boot2docker_ | ||
|
||
Beside, it is always possible to install Docker next to the buildmaster. | ||
This offers less isolation between process though. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not that sure that isolation is going to be any less in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A container remains a container, I just mean that docker itself and the master process are more tightly coupled ... I tend to virtualize everything, hence this is not my favorite setup, but if you have a better sentence to describe that, be my guest !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"In this case you should be aware that overall performance would depend on how many builds the computer where you have your buildmaster can handle."
LGTM, I will go through the changes in the morning. |
And my pending PR (about environment variables) will make more sense after this change lands in master. |
Then we will need to introduce an |
if (not found) and (self.dockerfile is not None): | ||
log.msg("Image '%s' not found, building it from scratch" % | ||
self.image) | ||
for line in docker_client.build(fileobj=BytesIO(self.dockerfile.encode('utf-8')), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This (build
) can take quite some time, especially when pulling the base image from internet ... It is a generator that spew the logs as it happens. Any way we could put that in a deferred ? Does it matter at all ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that we are already being deferToThread
in start_instance
. So it might actually be good ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should at least add a comment stating that those 2 methods are to be called from a thread
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't we had a kind of convention of prefixing with "thd_" the functions supposed to happen in another thread ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, please. Especially with Twisted, it's very important what's in a non-main thread and what is not, as reactors and deferreds can only be used in the main thread. Typically "thd" has been the signal for something in a non-main thread.
I believe all comments have been worked on, please indicate if it's not the case. |
👍 |
Oops, I believe I broke the tests ... |
I believe I fixed the test (it passes here), But I'm not sure I correctly handled the case of self.assertRaise inside an inlineCallback ... Feel free to comment on that if not. |
Looks great to me! |
The branch can't be merged, so a rebase is due. |
Otherwise LGTM. |
a463ecf
to
6a1a372
Compare
6a1a372
to
c88adb8
Compare
👍 thanks for this PR ! I was planning to add this feature ;) |
Rebased, Travis happy ! |
@delanne: Don't hesitate to give further feedback about the usability of those slaves ! (more/less parameters, different kinds, ...) |
Add a Docker Latent BuildSlave implementation Fixes ticket:2956
'Image "%s" not found on docker host.' % self.image | ||
) | ||
|
||
volumes = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should have noticed this earlier. I believe validation of volumes (and generation of self.volumes
) should be done in constructor, this way any problems would be reported at the config loading stage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I mentioned at that time, this code is taken verbatim from upstream, I did not checked it neither tested it, hence my original question about leaving it or re-implementing it when needed ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might have this need now that you are working on TRAC:3048: minimising a number of error situations might help to deal w/ the problem as well (and, yes, I understand that the traceback happens later).
Edit: actually, not later (not my day).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm on it. Also adding test and better doc ... (I don't believe that TRAC:3048 is linked to latent slaves ...)
EDIT: I meant TRAC:3051, not my day either ;)
This is inspired from https://github.com/elbaschid/dockbot/blob/make_single_command_executable/dockbot/slaves/dockerslave.py.
Apart from the concept few parts are remaining though.