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

Add a Docker Latent BuildSlave implementation #1344

Merged
merged 10 commits into from Nov 14, 2014

Conversation

benallard
Copy link
Contributor

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.

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):
Copy link
Contributor Author

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.

@benallard
Copy link
Contributor Author

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.
Copy link
Contributor

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.

Copy link
Contributor Author

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 !

Copy link
Contributor

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."

@sa2ajj
Copy link
Contributor

sa2ajj commented Nov 10, 2014

LGTM, I will go through the changes in the morning.

@sa2ajj
Copy link
Contributor

sa2ajj commented Nov 10, 2014

And my pending PR (about environment variables) will make more sense after this change lands in master.

@benallard
Copy link
Contributor Author

Then we will need to introduce an env parameter ... ;) Looking forward to it !

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')),
Copy link
Contributor Author

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 ?

Copy link
Contributor Author

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 ...

Copy link
Member

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

Copy link
Contributor Author

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 ?

Copy link
Member

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.

@benallard
Copy link
Contributor Author

I believe all comments have been worked on, please indicate if it's not the case.

@tardyp
Copy link
Member

tardyp commented Nov 12, 2014

👍

@benallard
Copy link
Contributor Author

Oops, I believe I broke the tests ...

@benallard
Copy link
Contributor Author

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.

@djmitche
Copy link
Member

Looks great to me!

@sa2ajj
Copy link
Contributor

sa2ajj commented Nov 14, 2014

The branch can't be merged, so a rebase is due.

@sa2ajj
Copy link
Contributor

sa2ajj commented Nov 14, 2014

Otherwise LGTM.

@delanne
Copy link
Contributor

delanne commented Nov 14, 2014

👍 thanks for this PR ! I was planning to add this feature ;)

@benallard
Copy link
Contributor Author

Rebased, Travis happy !

@benallard
Copy link
Contributor Author

@delanne: Don't hesitate to give further feedback about the usability of those slaves ! (more/less parameters, different kinds, ...)

sa2ajj pushed a commit that referenced this pull request Nov 14, 2014
Add a Docker Latent BuildSlave implementation

Fixes ticket:2956
@sa2ajj sa2ajj merged commit d89a593 into buildbot:master Nov 14, 2014
@benallard benallard deleted the docker_latent branch November 14, 2014 11:00
'Image "%s" not found on docker host.' % self.image
)

volumes = {}
Copy link
Contributor

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.

Copy link
Contributor Author

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 ?

Copy link
Contributor

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).

Copy link
Contributor Author

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 ;)

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.

None yet

5 participants