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 latent buildslave for OpenStack #666

Merged
merged 11 commits into from Apr 5, 2013

Conversation

Projects
None yet
4 participants
@ghost
Copy link

commented Mar 18, 2013

The buildslave has limited functionality for interacting with OpenStack's Nova component. Booting and terminating instances works correctly, but no interfacing with the network part.

Add latent buildslave for OpenStack
The buildslave has limited functionality for interacting with
OpenStack's Nova component. Booting and terminating instances
works correctly, but no interfacing with the network part.
@@ -0,0 +1,181 @@
# This file is part of Buildbot. Buildbot is free software: you can

This comment has been minimized.

Copy link
@djmitche

djmitche Mar 18, 2013

Member

I know this is different from the other latent buildslaves, but I'd like to start moving things to the buildbot.buildslave package. Could you rename to master/buildbot/buildslave/openstack.py, and make the similar rename for the tests?

This comment has been minimized.

Copy link
@ghost

ghost Mar 18, 2013

Author

very quick shipping, item as described
Chanel bags http://www.assureur-ideal.com/Chanel-Handbags/

boot_kwargs = {}
if self.meta is not None:
boot_kwargs['meta'] = self.meta
self.instance = os_client.servers.create(*boot_args, **boot_kwargs)

This comment has been minimized.

Copy link
@djmitche

djmitche Mar 18, 2013

Member

Is this (or anything else here) a blocking operation? If so, does it make sense to defer it to a thread?


def setUp(self):
self.patch(openstackbuildslave, "nce", openstack)
self.patch(openstackbuildslave, "client", openstack)

This comment has been minimized.

Copy link
@djmitche

djmitche Mar 18, 2013

Member

This is awesome :)

@djmitche

This comment has been minimized.

Copy link
Member

commented Mar 18, 2013

Otherwise, please add docs and an entry to the release notes. Thanks!

@djmitche

This comment has been minimized.

Copy link
Member

commented Mar 18, 2013

I'm happy. @tomprince is going to have a look shortly.

def _get_image(self, os_client):
# If self.image is a callable, then pass it the list of images. The
# function should return the image's UUID to use.
if hasattr(self.image, '__call__'):

This comment has been minimized.

Copy link
@tomprince

tomprince Mar 18, 2013

Member

This should be if callable(...):. And it should document the fact that it is calls the function in a thread (or maybe be run in the main thread).

I'm not sure what the use case is, so I don't know if this will likely call into the synchronous client library.

@tomprince

This comment has been minimized.

Copy link
Member

commented Mar 18, 2013

We use camelCase rather than underscore (following twisted). (We do use underscores occasionaly, in naming test methods, as a phrase seperator.

self.patch(openstack, "nce", None)
self.patch(openstack, "client", None)
self.assertRaises(config.ConfigErrors, self.ConcreteBuildSlave,
'bot', 'pass', 'flavor', 'image', 'user', 'pass', 'tenant', 'auth')

This comment has been minimized.

Copy link
@tomprince

tomprince Mar 18, 2013

Member

Id be happier if these passed arguments as keyword arguments. Given the number of arguments, users should be passing them that way.

def test_constructor_minimal(self):
bs = self.ConcreteBuildSlave('bot', 'pass', 'flavor', 'image', 'user',
'pass', 'tenant', 'auth')
yield bs._find_existing_deferred

This comment has been minimized.

Copy link
@tomprince

tomprince Mar 18, 2013

Member

I don't see anything name this anywhere.
In any case, yielding in the test, without decorating the test means that the test doesn't actually run. In this case, nothing asynchronous is going on, so you don't need to yield.


class TestOpenStackBuildSlave(unittest.TestCase):

class ConcreteBuildSlave(openstack.OpenStackLatentBuildSlave):

This comment has been minimized.

Copy link
@tomprince

tomprince Mar 18, 2013

Member

You shouldn't need this, since your class is not abstract. That style is only useful for AbstractBuildSlave and AbstractLatentBuildSlave.

passed as options to an OpenStack client.

When creating an OpenStackLatentBuildSlave, the first eight arguments are
required. The first two arguments, the name and password, work the same as with

This comment has been minimized.

Copy link
@tomprince

tomprince Mar 18, 2013

Member

As mention above, arguments should be passed as keyword arguments, sos we shouldn't talk about order of arguments here, or use positional arguments in the examples below.

@tomprince

This comment has been minimized.

Copy link
Member

commented Mar 19, 2013

It would be nice if the fakes tracked the calls with side-effects, and then made assertions about them.

seankelly added some commits Mar 19, 2013

Drop creating and using ConcreteBuildSlave
Not using an abstract slave, so do not need a concrete version. Some of
the lines are a bit long; this will be fixed later.
Use keyword args in test
Due to the number of required arguments, use keywords for them to be
clearer about what's being specified. Also changed the flavor to be an
integer because the docs say to pass the flavor ID, not name.
Update docs
Use keyword arguments in the examples and list the keyword arguments
with a brief description on each.
@djmitche

This comment has been minimized.

Copy link
Member

commented Mar 21, 2013

@tomprince, please have a look at the updates here when you're back on dry land.

Include the image UUID in the log
The instance name was included in the log message, but this is identical
to the slave name. Instead, include the image UUID as this could
actually help in debugging.
@djmitche

This comment has been minimized.

Copy link
Member

commented Apr 1, 2013

@tomprince - ping?

@djmitche

This comment has been minimized.

Copy link
Member

commented Apr 5, 2013

I'm going to get this merged. @tomprince, please have a look when you get a chance and we can fix any problems post-merge.

@djmitche djmitche merged commit 6cdebac into buildbot:master Apr 5, 2013

# started.
return defer.succeed(None)
instance = self.instance
self.instance = None

This comment has been minimized.

Copy link
@jaredgrubb

jaredgrubb Apr 5, 2013

Member

stop_instance can be called from the reactor right? If so, I dont think unseating self.instance is right, as that could make the worker thread crash, which references self.instance in a few places.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.