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

[Libcloud-589] ProfitBricks Compute Driver Support #352

Conversation

@baldwinSPC
Copy link
Contributor

baldwinSPC commented Aug 26, 2014

I have completed support for the ProfitBricks compute driver. This adds support for the core compute driver functions against ProfitBricks. The JIRA ticket can be found here:

https://issues.apache.org/jira/browse/LIBCLOUD-589

Let me know if there are any issues or if it can be accepted or not.

Thanks.
-matt

@Kami
Copy link
Member

Kami commented Aug 26, 2014

Sweet, thanks!

I will try to review it asap.

@Kami
Copy link
Member

Kami commented Aug 26, 2014

Travis build appears to be failing - https://travis-ci.org/apache/libcloud/builds/33635636

@baldwinSPC
Copy link
Contributor Author

baldwinSPC commented Aug 26, 2014

Thanks. Going through and correcting them now.

On Tue, Aug 26, 2014 at 2:28 PM, Tomaz Muraus notifications@github.com
wrote:

Travis build appears to be failing -
https://travis-ci.org/apache/libcloud/builds/33635636


Reply to this email directly or view it on GitHub
#352 (comment).

baldwinmathew added 20 commits Aug 26, 2014
datacenter) for datacenter in object.findall('.//return')]

def _to_datacenter(self, datacenter):
elements = list(datacenter.iter())

This comment has been minimized.

@Kami

Kami Aug 29, 2014 Member

Element.iter is not available in Python 2.6 (https://docs.python.org/2/library/xml.etree.elementtree.html#xml.etree.ElementTree.Element.iter), so please use findall instead (datacenter.findall('.//*')).

This comment has been minimized.

@baldwinSPC

baldwinSPC Aug 30, 2014 Author Contributor

@Kami OK, this is corrected now: all spots where I use iter are now using findall.

from libcloud.compute.types import NodeState
from libcloud.common.types import LibcloudError, MalformedResponseError

__version__ = '1.0.0'

This comment has been minimized.

@Kami

Kami Aug 29, 2014 Member

We don't __version__ tag / attribute in driver modules, so please remove it.

This comment has been minimized.

@baldwinSPC

baldwinSPC Aug 30, 2014 Author Contributor

@Kami I've removed the version line.

import copy
import time

# from xml.etree import ElementTree as ET

This comment has been minimized.

@Kami

Kami Aug 29, 2014 Member

Seems like this line can be removed all together?

This comment has been minimized.

@baldwinSPC

baldwinSPC Aug 30, 2014 Author Contributor

@Kami Done. The line has been removed.

try:
body = ET.XML(self.body)
except:
raise MalformedResponseError("Failed to parse XML",

This comment has been minimized.

@Kami

Kami Aug 29, 2014 Member

Minor style thing - for consistency, please use single quotes around strings.

This comment has been minimized.

@baldwinSPC

baldwinSPC Aug 30, 2014 Author Contributor

@Kami Strings are now single quoted.

This comment has been minimized.

@baldwinSPC

baldwinSPC Aug 30, 2014 Author Contributor

@Kami The only issue I have at the moment is with Travis CI. It seems that I'm failing the py3x tests, but all others pass. I'm not sure how to diagnose the issue as it refers to the test_profitbricks module.

AttributeError: 'module' object has no attribute 'test_profitbricks'
ERROR: InvocationError: '/home/travis/build/apache/libcloud/.tox/py32/bin/python setup.py test'

data=body,
method='POST').object)

def destroy_node(self, node, remove_attached_disks=None):

This comment has been minimized.

@Kami

Kami Sep 2, 2014 Member

Should default to False and not None.

This comment has been minimized.

@Kami

Kami Sep 2, 2014 Member

Also, it's an extension argument so it should be prefixed with ex_.

data=body,
method='POST').object)

def attach_volume(self, node, volume, device=None, bus_type=None):

This comment has been minimized.

@Kami

Kami Sep 2, 2014 Member

bus_type is an extension argument so it should be prefixed with ex_.

This comment has been minimized.

@Kami

Kami Sep 2, 2014 Member

Also, what happens if None is sent as the value for the busType attribute? Does it use a server side default value or does it error out?

data=body,
method='POST').object)

def detach_volume(self, node, volume):

This comment has been minimized.

@Kami

Kami Sep 2, 2014 Member

Base API method doesn't include node argument.

Is it possible to retrieve the id of the node the volume is attached to by retrieving information for a single volume from the API?

This way we can follow the standard API (def detach_volume(self, volume)) and don't need to change the method signature (def detach_volume(self, volume, ex_node)).

This comment has been minimized.

@Kami

Kami Sep 2, 2014 Member

Edit: User is already expected to pass in an instance of the Volume class so we don't actually need to hit the API.

Just doing node_id = volume.extra['server_id'] and then using that in the request body should work.

"""
Detaches a volume.
:param volume: The volume you're attaching.

This comment has been minimized.

@Kami

Kami Sep 2, 2014 Member

Docstrings need to be updated (volume to be detached).


return True

def ex_describe_volume(self, volume):

This comment has been minimized.

@Kami

Kami Sep 2, 2014 Member

The method should take volume_id and not volume (and instance of the Volume class) argument.

The user should already have all the volume information if it has a reference to the volume object. Unless of course, the Volume class is instantiated manually with just an id, but that's an invalid use of the API in this case.

"""
Describes a volume.
:param volume: The volume you're attaching..

This comment has been minimized.

@Kami

Kami Sep 2, 2014 Member

Docstring needs to be updated.

return True

def ex_set_inet_access(self, datacenter,
network_interface, ex_internet_access=True):

This comment has been minimized.

@Kami

Kami Sep 2, 2014 Member

This is an extension method, so the argument itself doesn't need to have ex_ prefix (just internet_access is fine).

else:
public_ips.append(ip)

if ET.iselement(node.find('cpuHotPlug')):

This comment has been minimized.

@Kami

Kami Sep 2, 2014 Member

It seems like we could reduce a couple of lines of code and make this a bit easier to maintain by using a dict and a for loop.

Something along the lines of:

attribute_to_extra_name_map = {
    'ramHotPlug': 'memory_hot_pluggable',
    ...
}

for attribute_name, extra_name in attribute_to_extra_name_map.items():
    if ET.iselement(node.find(attribute_name)):
        value = node.find(attribute_name).text
    else:
        value = None

    extra[extra_name] = value
"""
dc_operation_status = self.ex_describe_datacenter(datacenter[0])

timeout = 60 * 5

This comment has been minimized.

@Kami

Kami Sep 2, 2014 Member

Minor thing - it would be nice if timeout, wait_time and interval variables would be declared in the method signature with the same default values which are currently used.

Same goes for _wait_for_storage_volume_state


waittime += interval
time.sleep(interval)

This comment has been minimized.

@Kami

Kami Sep 2, 2014 Member

This method doesn't actually do anything different if a timeout occurs (None is implicitly returned in both cases).

On success, we should probably return object in question (latest reference to the datacenter object in this case) and on time out, we should throw a timeout exception.

Same goes for _wait_for_storage_volume_state.

This comment has been minimized.

@Kami

Kami Sep 7, 2014 Member

While reviewing the changes, I also just noticed ex_describe_datacenters incorrectly returns a list. Anyway, I'll fix it.

@Kami
Copy link
Member

Kami commented Sep 2, 2014

Added some more comments.

When those are addressed, please squash all the commits.

@Kami
Copy link
Member

Kami commented Sep 6, 2014

@baldwinSPC How are things looking for those last couple of issues?

I will probably be preparing a new release this weekend and it would be great if those things could get addressed so I can also include this driver.

If you can't do it this weekend, let me know and I will do it myself (address those issues + squash the commits) while merging the patch.

@baldwinSPC
Copy link
Contributor Author

baldwinSPC commented Sep 6, 2014

Hey Tomaz

I've corrected about half of these. I'll commit and push those this
afternoon. I don't think I can correct the others, but what I'll do is
comment on the ones I've finished up.
On Sep 6, 2014 6:30 AM, "Tomaz Muraus" notifications@github.com wrote:

@baldwinSPC https://github.com/baldwinSPC How are things looking for
those last couple of issues?

I will probably be preparing a new release this weekend and it would be
great if those things could get addressed so I can also include this driver.

If you can't do it this weekend, let me know and I will do it myself
(address those issues + squash the commits) while merging the patch.


Reply to this email directly or view it on GitHub
#352 (comment).

@baldwinSPC
Copy link
Contributor Author

baldwinSPC commented Sep 7, 2014

@Kami I just pushed some updates that address most of the items you called out -- there are only two remaining ones: waiting for state and re-working the ET code.

@Kami
Copy link
Member

Kami commented Sep 7, 2014

@baldwinmathew Great, thanks.

I'll work on rest of the issues and try to get it merged tonight.

@Kami
Copy link
Member

Kami commented Sep 7, 2014

Another thing which got me confused while reviewing the changes:

PROVISIONING_STATE = {
        'INACTIVE': NodeState.PENDING,
        'INPROCESS': NodeState.PENDING,
        'AVAILABLE': NodeState.RUNNING,
        'DELETED': NodeState.TERMINATED,
    }

....

def _wait_for_datacenter_state(
            self,
            datacenter,
            state=PROVISIONING_STATE.get(NodeState.RUNNING),
            timeout=300,
            interval=5):

NodeState.RUNNING is mapped to int 0 (libcloud.compute.types.NodeState), but PROVISIONING_STATE dictionary contains strings as keys. Unless I am missing something, this means .get will always return None and _wait function will always time out.

Same goes with while condition - self.PROVISIONING_STATE.get(NodeState.PENDING).

I believe the state should default to state=PROVISIONING_STATE['available']. I will push those fixes to my branch and wait for you to review them before merging them into trunk.

1. Fix "ex_describe_datacenter" to return a single object instead of a list
2. Fix "ex_describe_datacenter" method signature to take an id instead of an object
3. Fix default value for "state" argument in the "_wait_for_datacenter_state" method
4. Fix while condition in the "_wait_for_datacenter_state" method
5. Modify "_wait_for_datacenter_state" to throw an Exception on timeout
6. Do the same fixes in the "_wait_for_storage_volume_state" method
7. Fix "ex_create_datacenter" to return single object instead of a list
8. Fix "create_volume" to return a single object instead of a list
9. Fix "ex_describe_node" to return a single object instead of a list
10. Fix "create_node" to retirn a single object instead of a list
11. Fix "attach_volume" to return volume on success
12. Fix "ex_create_network_interface" to return a single object instead of a list
@Kami
Copy link
Member

Kami commented Sep 7, 2014

@baldwinSPC I performed another review. During the review I still spotted many issues (mostly with function claiming it returns a single object but it actually returned a list) which I have fixed in my branch - Kami@28e9774

I would appreciate if you can go over my fixes, double check them and test them with the actual Profitbricks API (I don't have an account).

Thanks!

@baldwinSPC
Copy link
Contributor Author

baldwinSPC commented Sep 9, 2014

@Kami Great. Thanks Tomaz. I will be testing through this tomorrow morning and will update this thread.

Various fixes in the Profitbricks driver:
@baldwinSPC
Copy link
Contributor Author

baldwinSPC commented Sep 9, 2014

@Kami I just merged your branch into mine after testing all the methods live against the API. Everything looks good, however, in my testing I caught two bugs due to empty data coming back from their API. Those are two minor changes you should see on my next commit.

@baldwinSPC
Copy link
Contributor Author

baldwinSPC commented Sep 9, 2014

@Kami BTW, thank you for all your support. :)

@Kami
Copy link
Member

Kami commented Sep 12, 2014

@baldwinmathew Great and you are welcome.

I will look into your changes and and look into merging this pull request into trunk this weekend :)

@asfgit asfgit closed this in 34cff30 Sep 14, 2014
@Kami
Copy link
Member

Kami commented Sep 14, 2014

Looks good.

I've made "a bunch of ifs" to "for loop" change and merged patch into trunk. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.