Uploaded image for project: 'Libcloud'
  1. Libcloud
  2. LIBCLOUD-121

OpenNebula Driver Improvements and Additional Driver Updates

    Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 0.6.1
    • Fix Version/s: None
    • Component/s: Core
    • Labels:
    • Environment:

      Latest Libcloud trunk with contents of diff operating on Debian 6.0 (Stable).

      Description

      I'm attaching a diff/patch that contains modifications to several compute drivers; OpenNebula in particular.

      With regard to OpenNebula, these changes include the ability to request a list of virtual networks provided by the OpenNebula infrastructure provider. Furthermore, the OpenNebula NodeDriver has been extended to extract the disk and network descriptions from the compute XML description. Disks and networks are instantiated as NodeImage and NodeNetwork objects and then attached to a newly instantiated Node. To support this capability, I added a NodeNetwork class to the base compute class.

      Furthermore, I modified the Opsource and OpenStack drivers to match in consistency with the OpenNebula driver. This also included changes to include httplib. I hope to extend that work later to other drivers, if the work is desired.

      This patch really requires additional, more in-depth testing, during this following week, but I wanted to present that patch for consideration, and comments.

      1. base-update.diff
        0.3 kB
        Hutson Betts
      2. opennebula.diff
        23 kB
        Hutson Betts
      3. opennebula.patch
        76 kB
        Hutson Betts
      4. opennebula.patch
        73 kB
        Hutson Betts
      5. update.diff
        55 kB
        Hutson Betts

        Activity

        Hide
        hut101 Hutson Betts added a comment -

        Patch has been pushed to trunk.

        Show
        hut101 Hutson Betts added a comment - Patch has been pushed to trunk.
        Hide
        kami Tomaz Muraus added a comment -

        Looks OK. If all the tests and pep8 tool passes, I'd say merge it.

        Edit: Just noticed:

        + return Node(id=compute.findtext('ID'),

        We should enforce that node id is always a string:

        + return Node(id=str(compute.findtext('ID')),

        Show
        kami Tomaz Muraus added a comment - Looks OK. If all the tests and pep8 tool passes, I'd say merge it. Edit: Just noticed: + return Node(id=compute.findtext('ID'), We should enforce that node id is always a string: + return Node(id=str(compute.findtext('ID')),
        Hide
        hut101 Hutson Betts added a comment -

        I've improved upon the second issue regarding the api_version check. However, with regard to the first issue involving a circular reference, I've removed the constructor. I simply don't understand the issue well enough to incorporate code related to it.

        I've also rolled in changes I made in a second patch, which was meant to be a patch to the previous patch. These changes, in addition to those mentioned in my previous file attachment, include:

        • I added a test to the base compute TestCaseMixin to check for the
          proper value of NodeSize.price. Also, I fixed a PEP8 compliance issue
          with the same file.
        • Furthermore, I replaced _xml_action with ex_node_action in the
          OpenNebula driver. Since _xml_action what specific to node actions, I
          though a more targeted method was more appropriate. Furthermore, users
          can now use ex_node_action to specify more actions on a node other than
          just reboot_node.
        • Replaced the default price value in the BrightBox compute driver to be
          an int rather than a string to comply with the NodeSize.price test.
        • Updated the OpenNebula 2.0 collection fixtures to include the name
          attribute.
        • Added additional NodeSize tests to OpenNebula.

        This attached file contains all changes made as a results of this issue, 121.

        Show
        hut101 Hutson Betts added a comment - I've improved upon the second issue regarding the api_version check. However, with regard to the first issue involving a circular reference, I've removed the constructor. I simply don't understand the issue well enough to incorporate code related to it. I've also rolled in changes I made in a second patch, which was meant to be a patch to the previous patch. These changes, in addition to those mentioned in my previous file attachment, include: I added a test to the base compute TestCaseMixin to check for the proper value of NodeSize.price. Also, I fixed a PEP8 compliance issue with the same file. Furthermore, I replaced _xml_action with ex_node_action in the OpenNebula driver. Since _xml_action what specific to node actions, I though a more targeted method was more appropriate. Furthermore, users can now use ex_node_action to specify more actions on a node other than just reboot_node. Replaced the default price value in the BrightBox compute driver to be an int rather than a string to comply with the NodeSize.price test. Updated the OpenNebula 2.0 collection fixtures to include the name attribute. Added additional NodeSize tests to OpenNebula. This attached file contains all changes made as a results of this issue, 121.
        Hide
        kami Tomaz Muraus added a comment -

        Really sorry for the delay, I was pretty busy recently.

        Overall the patch looks good, here are a few minor things I have noticed:

        1.

        + def _init_(self, *args, **kwargs):
        + # done because of a circular reference from
        + # NodeDriver -> Connection -> Response
        + self.connection = OpenNebulaConnection
        + super(OpenNebulaResponse, self)._init_(*args, **kwargs)
        +

        Is there a nicer way to get rid of the cycling dependency?

        2.

        + elif api_version == '2.0' or api_version == '2.2' or \
        + api_version == '3.0' or api_version == '3.2':
        +

        Can be written as:

        elif api_version in ['2.0', '2.2', '3.0', '3.2'] ...

        Show
        kami Tomaz Muraus added a comment - Really sorry for the delay, I was pretty busy recently. Overall the patch looks good, here are a few minor things I have noticed: 1. + def _ init _(self, *args, **kwargs): + # done because of a circular reference from + # NodeDriver -> Connection -> Response + self.connection = OpenNebulaConnection + super(OpenNebulaResponse, self)._ init _(*args, **kwargs) + Is there a nicer way to get rid of the cycling dependency? 2. + elif api_version == '2.0' or api_version == '2.2' or \ + api_version == '3.0' or api_version == '3.2': + Can be written as: elif api_version in ['2.0', '2.2', '3.0', '3.2'] ...
        Hide
        hut101 Hutson Betts added a comment -

        This patch is an update to the OpenNebula compute driver and compute tests as compared to the latest Libcloud trunk.

        Changes include:

        Overall:

        • Change the name of the OpenNebula v3.0 Compute Driver to OpenNebula_2_0_NodeDriver to reflect that every API after v2.0 has remained the same. Therefore, 2.0 should remain the defacto standard API version until an API change is made.
        • Added API v3.2 to supported API versions of the OpenNebula v2.0 Compute Driver. OpenNebula 3.2 will be published later this year, but the OCCI API, OpenNebula uses OCCI v1.0, will remain the same.

        OpenNebula v1.4 Compute Driver

        • Changes to the create_node function:
        • Added support for specifying a disk image for disk images available in the storage pool.
        • Added support for specifying a network for networks available in the network pool.
        • Replaced HTTP Response codes, such a integer values that were used for checking HTTP responses, with codes in the httplib library.
        • Updated network retrieval support to get the 'SIZE' attribute for each network in the network pool.
        • Added support to get extract the image information for each compute node.
        • Corrected code associated with adding an image to a compute node or reading a compute node description with an image.
        • Documentation update.

        OpenNebula v2.0 Compute Driver

        • Added context variable for allowing user-defined context values in create_node.
        • Added support to extract context information from a compute node description.
        • Added support to extract NodeSize information from a compute node description.
        • Added support to extract NodeImage information from a compute node description.
        • Documentation updates.

        OpenNebula Tests

        • New test cases for OpenNebula computer driver v2.0+.
        • Updated MockHTTP handlers.
        • Updated XML files to include network files, and corrections to the files relating to OpenNebula v2.0+.

        I hope this will finish out the requirements for this ticket. If not, please let me know.

        Show
        hut101 Hutson Betts added a comment - This patch is an update to the OpenNebula compute driver and compute tests as compared to the latest Libcloud trunk. Changes include: Overall: Change the name of the OpenNebula v3.0 Compute Driver to OpenNebula_2_0_NodeDriver to reflect that every API after v2.0 has remained the same. Therefore, 2.0 should remain the defacto standard API version until an API change is made. Added API v3.2 to supported API versions of the OpenNebula v2.0 Compute Driver. OpenNebula 3.2 will be published later this year, but the OCCI API, OpenNebula uses OCCI v1.0, will remain the same. OpenNebula v1.4 Compute Driver Changes to the create_node function: Added support for specifying a disk image for disk images available in the storage pool. Added support for specifying a network for networks available in the network pool. Replaced HTTP Response codes, such a integer values that were used for checking HTTP responses, with codes in the httplib library. Updated network retrieval support to get the 'SIZE' attribute for each network in the network pool. Added support to get extract the image information for each compute node. Corrected code associated with adding an image to a compute node or reading a compute node description with an image. Documentation update. OpenNebula v2.0 Compute Driver Added context variable for allowing user-defined context values in create_node. Added support to extract context information from a compute node description. Added support to extract NodeSize information from a compute node description. Added support to extract NodeImage information from a compute node description. Documentation updates. OpenNebula Tests New test cases for OpenNebula computer driver v2.0+. Updated MockHTTP handlers. Updated XML files to include network files, and corrections to the files relating to OpenNebula v2.0+. I hope this will finish out the requirements for this ticket. If not, please let me know.
        Hide
        hut101 Hutson Betts added a comment -

        Thank you for the partial incorporation into Libcloud. I'm not sure when I'll get an opportunity to create tests, but I'll try my best to get them together.

        As for the changes, you're correct, I misspelled 'ADDRESS'. The others make since.

        Yes, I do work in a separate branch, and I'll need to work on my patch matching.

        Show
        hut101 Hutson Betts added a comment - Thank you for the partial incorporation into Libcloud. I'm not sure when I'll get an opportunity to create tests, but I'll try my best to get them together. As for the changes, you're correct, I misspelled 'ADDRESS'. The others make since. Yes, I do work in a separate branch, and I'll need to work on my patch matching.
        Hide
        kami Tomaz Muraus added a comment -

        I have partially applied your patch with Network class in r1187946 (http://svn.apache.org/viewvc?view=revision&revision=1187946). I have added the network class to the OpenNebula driver and called it OpenNebulaNetwork. The method for listing the networks is called ex_list_networks.

        This can be changed once we decide about the unified networking API.

        Some other things I have changed:

        • extra={'address': element.findtext('ADRESS'), -> extra={'address': element.findtext('ADDRESS') - I assume "ADRESS" was a typo or does the API really return "ADRESS"?
        • NodeNetwork(id=element.attrib['network'] -> OpenNebulaNetwork(id=element.attrib.get('network', None)
        • networkElement -> network_element, networkList -> network_list - we follow PEP8 and use underscore separated names

        I won't mark this ticket as closed yet, because we are still missing test. If you want to get this patch included in the 0.6.0, release please add tests during this week.

        Also always make sure you generate the patch against Apache trunk version. It looks like this one was generated against one of your branches with local modifications which aren't in the trunk which means I needed to apply it manually.

        Show
        kami Tomaz Muraus added a comment - I have partially applied your patch with Network class in r1187946 ( http://svn.apache.org/viewvc?view=revision&revision=1187946 ). I have added the network class to the OpenNebula driver and called it OpenNebulaNetwork. The method for listing the networks is called ex_list_networks. This can be changed once we decide about the unified networking API. Some other things I have changed: extra={'address': element.findtext('ADRESS'), -> extra={'address': element.findtext('ADDRESS') - I assume "ADRESS" was a typo or does the API really return "ADRESS"? NodeNetwork(id=element.attrib ['network'] -> OpenNebulaNetwork(id=element.attrib.get('network', None) networkElement -> network_element, networkList -> network_list - we follow PEP8 and use underscore separated names I won't mark this ticket as closed yet, because we are still missing test. If you want to get this patch included in the 0.6.0, release please add tests during this week. Also always make sure you generate the patch against Apache trunk version. It looks like this one was generated against one of your branches with local modifications which aren't in the trunk which means I needed to apply it manually.
        Hide
        kami Tomaz Muraus added a comment -

        I just checked the updates patches. For now I will move "NodeNetwork" class into OpenNebula driver file and call it "OpenNebulaNetwork" (similar thing as the OpSource driver does).

        I think it's to early to put a base class into libcloud.compute.base. Just by looking at the file someone might get the idea that this is a "permanent" / final API for virtual networks in Libcloud.

        Show
        kami Tomaz Muraus added a comment - I just checked the updates patches. For now I will move "NodeNetwork" class into OpenNebula driver file and call it "OpenNebulaNetwork" (similar thing as the OpSource driver does). I think it's to early to put a base class into libcloud.compute.base. Just by looking at the file someone might get the idea that this is a "permanent" / final API for virtual networks in Libcloud.
        Hide
        hut101 Hutson Betts added a comment -

        I've uploaded base-update.diff as an update to opennebula.diff to account for a missing export.

        Show
        hut101 Hutson Betts added a comment - I've uploaded base-update.diff as an update to opennebula.diff to account for a missing export.
        Hide
        hut101 Hutson Betts added a comment -

        Here is an updated diff just for OpenNebula driver and the base file.

        Show
        hut101 Hutson Betts added a comment - Here is an updated diff just for OpenNebula driver and the base file.
        Hide
        hut101 Hutson Betts added a comment -

        My next step was to begin looking at (libcloud.networking.*) and see how that capability can be supported.

        • Storing IP as a member of NodeNetwork rather than a member of the dictionary was a temporary decision so that I could avoid a permanent decision at this time. See, in OpenNebula, when requesting the XML description of a network, such as /network/[ID], the ID, and name are returned in addition to the network address and network size (which combined is equivalent to CIDR notation). However, the network description returned as part of the node description gives the ID and name, and the IP assigned to the node. Therefore, only the ID and name are consistent between those two representations of a network. In regard to IPs, I would consider a list more appropriate since I see no reason a node couldn't have more than one IP address on a single network.
        • Chaning _new_ would be a result of my somewhat extreme programming mentality as I saw a difference between OpenStack and the other drivers. I'll revert my change and update the patches. Thank you for catching my mistake.
        • I didn't realize _fixxpath and _fixall where in libcloud.utils. I'll go ahead and modify OpenStack to use utils and include that in my updated patch.
        • Another consequence of my more extreme programming mentality. However, I can at least split the Opsource and OpenStack style diffs into separate files for review. It'll just be difficult to break down the work in a more granular manner simply because I did all my work in unison on the same branch. On another note, I use bazaar for my VCS so I'll see what other features it gives me in this regard.
        • There shouldn't be any, or at least very few, PEP8 violations in the OpenNebula file after inclusion of the patch. I ran PEP8 over the result of that file only, and resolved a few issues but pep8 kept crashing on me. It's possible there are pep8 violations in the Opsource and OpenStack drivers that I didn't notice, and didn't catch since I didn't run pep8 on those, and could correct in another patch.
        • If I were to request all information on each virtual network for a node, it could lead to N requests as you mentioned. That's why I'm relying on the information already provided within the node description for the special case of OpenNebula.

        I'll create a ticket associated with OpenStack/Opsource cleanup, and a ticket for OpenNebula cleanup and improvements.

        • Should I include the NodeNetwork addition as part of the OpenNebula patch, or as part of a greater "Unified Networking" ticket?
        Show
        hut101 Hutson Betts added a comment - My next step was to begin looking at (libcloud.networking.*) and see how that capability can be supported. Storing IP as a member of NodeNetwork rather than a member of the dictionary was a temporary decision so that I could avoid a permanent decision at this time. See, in OpenNebula, when requesting the XML description of a network, such as /network/ [ID] , the ID, and name are returned in addition to the network address and network size (which combined is equivalent to CIDR notation). However, the network description returned as part of the node description gives the ID and name, and the IP assigned to the node. Therefore, only the ID and name are consistent between those two representations of a network. In regard to IPs, I would consider a list more appropriate since I see no reason a node couldn't have more than one IP address on a single network. Chaning _ new _ would be a result of my somewhat extreme programming mentality as I saw a difference between OpenStack and the other drivers. I'll revert my change and update the patches. Thank you for catching my mistake. I didn't realize _fixxpath and _fixall where in libcloud.utils. I'll go ahead and modify OpenStack to use utils and include that in my updated patch. Another consequence of my more extreme programming mentality. However, I can at least split the Opsource and OpenStack style diffs into separate files for review. It'll just be difficult to break down the work in a more granular manner simply because I did all my work in unison on the same branch. On another note, I use bazaar for my VCS so I'll see what other features it gives me in this regard. There shouldn't be any, or at least very few, PEP8 violations in the OpenNebula file after inclusion of the patch. I ran PEP8 over the result of that file only, and resolved a few issues but pep8 kept crashing on me. It's possible there are pep8 violations in the Opsource and OpenStack drivers that I didn't notice, and didn't catch since I didn't run pep8 on those, and could correct in another patch. If I were to request all information on each virtual network for a node, it could lead to N requests as you mentioned. That's why I'm relying on the information already provided within the node description for the special case of OpenNebula. I'll create a ticket associated with OpenStack/Opsource cleanup, and a ticket for OpenNebula cleanup and improvements. Should I include the NodeNetwork addition as part of the OpenNebula patch, or as part of a greater "Unified Networking" ticket?
        Hide
        kami Tomaz Muraus added a comment -

        Yeah, there already was some debate about virtual networks on the mailing list, but we never came to a concrete conclusion what to do with it.

        One of the ideas was also to include a special API for networking (libcloud.networking.*) which would be pretty coupled with the compute one and have a bunch of methods which take Node object as an argument.

        Some things I have noticed in your patch:

        • shouldn't IP address be stored on the NodeNetwork object, not in the extra dictionary? Maybe this attribute should also be a list? (requires more exploration about the existing provider APIs)
        • is there any special reason why you changed the _new_ signature in OpenStack driver? Before the change it was "backward" compatible and it could be used with args, not it can only be used with kwargs.
        • _fixxpath, _findall - those two methods are already in libcloud.utils, lets re-use them
        • there are some style changes in the same patch which makes it harder to review. Next time it would be nice if the style changes would be in a separate patch (yes, I know that's easier said that done with SVN )
        • I see some pep8 violations just by looking at the patch (mostly line lengths)

        Other things to keep in mind:

        • In a lot of cases, virtual networks won't be returned in the same request as the Node objects which means we might need to preform an extra HTTP request.

        I would be ok with one extra HTTP request in list_nodes, but if it would result in N extra requests in some drivers we might need to rethink the concept or include some kind of retry mechanism in the core.

        The problem is that most of the APIs I have worked with so far aren't really reliable which means if list_nodes() would result in (1 + N) HTTP requests and we would throw on the first error it would rarely succeed (N = number of nodes).

        IIRC some other ppl (including some ppl from Rackspace) were already looking at the possibility to add support for virtual networks to the Libcloud. I will appoint them to this ticket so you can all collaborate on a solution.

        It would also make it easier if we split this ticket into multiple ones:

        • Unified support for virtual networks
        • OpenStack cleanup
        • OpenNebula cleanup and improvements

        In any case, like you have said this patch would require exploring "virtual network" concept for more providers and of course a lot of unit tests

        Show
        kami Tomaz Muraus added a comment - Yeah, there already was some debate about virtual networks on the mailing list, but we never came to a concrete conclusion what to do with it. One of the ideas was also to include a special API for networking (libcloud.networking.*) which would be pretty coupled with the compute one and have a bunch of methods which take Node object as an argument. Some things I have noticed in your patch: shouldn't IP address be stored on the NodeNetwork object, not in the extra dictionary? Maybe this attribute should also be a list? (requires more exploration about the existing provider APIs) is there any special reason why you changed the _ new _ signature in OpenStack driver? Before the change it was "backward" compatible and it could be used with args, not it can only be used with kwargs. _fixxpath, _findall - those two methods are already in libcloud.utils, lets re-use them there are some style changes in the same patch which makes it harder to review. Next time it would be nice if the style changes would be in a separate patch (yes, I know that's easier said that done with SVN ) I see some pep8 violations just by looking at the patch (mostly line lengths) Other things to keep in mind: In a lot of cases, virtual networks won't be returned in the same request as the Node objects which means we might need to preform an extra HTTP request. I would be ok with one extra HTTP request in list_nodes, but if it would result in N extra requests in some drivers we might need to rethink the concept or include some kind of retry mechanism in the core. The problem is that most of the APIs I have worked with so far aren't really reliable which means if list_nodes() would result in (1 + N) HTTP requests and we would throw on the first error it would rarely succeed (N = number of nodes). IIRC some other ppl (including some ppl from Rackspace) were already looking at the possibility to add support for virtual networks to the Libcloud. I will appoint them to this ticket so you can all collaborate on a solution. It would also make it easier if we split this ticket into multiple ones: Unified support for virtual networks OpenStack cleanup OpenNebula cleanup and improvements In any case, like you have said this patch would require exploring "virtual network" concept for more providers and of course a lot of unit tests
        Hide
        hut101 Hutson Betts added a comment -

        Diff for the aforementioned changes.

        Show
        hut101 Hutson Betts added a comment - Diff for the aforementioned changes.

          People

          • Assignee:
            hut101 Hutson Betts
            Reporter:
            hut101 Hutson Betts
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development