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-556] Azure Compute Driver Support #305

Merged
merged 1 commit into from Apr 6, 2015

Conversation

@baldwinSPC
Copy link
Contributor

baldwinSPC commented Jun 3, 2014

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

https://issues.apache.org/jira/browse/LIBCLOUD-556?jql=project%20%3D%20LIBCLOUD

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

Thanks.
-matt

from xml.sax.saxutils import escape as xml_escape
from httplib import (HTTPSConnection)

if sys.version_info < (3,):

This comment has been minimized.

Copy link
@Kami

Kami Jun 5, 2014

Member

You should use libcloud.utils.urlquote and libcloud.utils.urlunquote which handles this for you.

port=port, url=url, timeout=timeout)

self.cert_file = cert_file
self.subscription_id = subscription_id

This comment has been minimized.

Copy link
@Kami

Kami Jun 5, 2014

Member

Hm, what does subscription_id represent? It looks like it might be something which is specified to Azure and doesn't belong on a base class?

This comment has been minimized.

Copy link
@baldwinSPC

baldwinSPC Jun 6, 2014

Author Contributor

Correct. This is the Azure subscription ID.

This comment has been minimized.

Copy link
@baldwinSPC

baldwinSPC Jun 6, 2014

Author Contributor

Correct. I'll move that into azure.py.

@staticmethod
def data_virtual_hard_disk_to_xml(host_caching, disk_label, disk_name, lun,
logical_disk_size_in_gb, media_link,

This comment has been minimized.

Copy link
@Kami

Kami Jun 5, 2014

Member

Please use elementree module (https://docs.python.org/2/library/xml.etree.elementtree.html) for building XML.

Directly concatenating strings is error prone and can be dangerous (security wise).

This comment has been minimized.

Copy link
@baldwinSPC

baldwinSPC Aug 26, 2014

Author Contributor

@azurecoder this is the comment I mentioned in my email.

@Kami
Copy link
Member

Kami commented Jun 5, 2014

Thanks!

Just added some quick comments. Will do a more complete review later on.

@baldwinSPC
Copy link
Contributor Author

baldwinSPC commented Jun 6, 2014

Thanks Tomaz. I'll begin to address these items.

-matt

On Thu, Jun 5, 2014 at 2:58 AM, Tomaz Muraus notifications@github.com
wrote:

Thanks!

Just added some quick comments. Will do a more complete review later on.


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

@jclark360
Copy link

jclark360 commented Jun 24, 2014

Looking forward to seeing this in a release

@baldwinSPC
Copy link
Contributor Author

baldwinSPC commented Jul 1, 2014

I'm back from being out of town on business for too many weeks and have turned to squashing these items. Should have something shortly.

@phamnamlong
Copy link

phamnamlong commented Sep 23, 2014

Is there anyone following up with the merge of this pull request ?

@baldwinSPC
Copy link
Contributor Author

baldwinSPC commented Sep 24, 2014

Hi Pham:

Yes, we're currently re-working this compute driver to utilize ET versus
how it is currently implemented. Unfortunately, I've been delayed in being
able to return to this. I'm hoping I will have more time in mid-October to
pick this back up and knock it out.

Thanks.
-matt

On Mon, Sep 22, 2014 at 6:48 PM, Pham Nam Long notifications@github.com
wrote:

Is there anyone following up with the merge of this pull request ?


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

@azurecoder
Copy link
Contributor

azurecoder commented Sep 24, 2014

Started the ET support this week Matt - will commit this weekend and pickup with you.

@baldwinSPC
Copy link
Contributor Author

baldwinSPC commented Sep 24, 2014

Sweet. Thanks Richard!

On Wed, Sep 24, 2014 at 12:51 AM, Richard Conway notifications@github.com
wrote:

Started the ET support this week Matt - will commit this weekend and
pickup with you.


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

@davidcrossland
Copy link
Contributor

davidcrossland commented Oct 3, 2014

Can you take a look at the PR now - we've updated the xml serialization to use element tree as advised as can successfully run all compute functions and all unit tests pass

@baldwinSPC
Copy link
Contributor Author

baldwinSPC commented Oct 9, 2014

Thanks David. I'll review this over the weekend. Then we can go from there.

On Fri, Oct 3, 2014 at 2:24 AM, davidcrossland notifications@github.com
wrote:

Can you take a look at the PR now - we've updated the xml serialization to
use element tree as advised as can successfully run all compute functions
and all unit tests pass


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

@bennettaur
Copy link
Contributor

bennettaur commented Oct 29, 2014

I've been working on a branch of this, adding in some functionality I needed for a project. You can see my branch here: https://github.com/SecurityCompass/libcloud/commits/LIBCLOUD-556_azure_compute_driver_rebased

I've added some functionality like displaying custom VM images along with the azure provided ones, creating nodes using custom VMs, and using NodeSize, NodeLocation, and NodeImage for the arguments to create node. I also rebased this branch on almost the latest trunk and fixed some issues there.

It looks like there might be some overlap in some of the current efforts and I imagine the easiest approach is for me to pull in the changes and then open a PR back to this branch. Is anything else being actively worked on that I should wait for?

@azurecoder
Copy link
Contributor

azurecoder commented Nov 1, 2014

Hi Michael – we did a recent commit to add ET. We need this functionality too as does Matt B. We’ve got a full test coverage currently. Please feel free to pull in the changes. We’re still waiting for this branch to be merged as it happens.

All the best,

Richard Conway
Head of Product Development | Elastacloud Limited |
Windows Azure MVP and Insider
T. 0208 530 3999 (office) | 07590333990 (mobile)
W. Web Sitehttp://www.elastacloud.com/ | My Linked Inapplewebdata://D40E0893-DB28-4B52-94DE-FC6823F51ADC/uk.linkedin.com/in/richardelastacloud/ | My Bloghttp://azurecoder.azurewebsites.net/ | My Twitterapplewebdata://D40E0893-DB28-4B52-94DE-FC6823F51ADC/twitter.com/azurecoder | UK Azure User Group Twitterapplewebdata://D40E0893-DB28-4B52-94DE-FC6823F51ADC/twitter.com/ukwaug

From: Michael Bennett [mailto:notifications@github.com]
Sent: Wednesday, October 29, 2014 4:00 PM
To: apache/libcloud
Cc: Richard Conway
Subject: Re: [libcloud] [Libcloud-556] Azure Compute Driver Support (#305)

I've working on a branch of this, adding in some functionality I needed for a project. You can see my branch here: https://github.com/SecurityCompass/libcloud/commits/LIBCLOUD-556_azure_compute_driver_rebased

I've added some functionality like displaying custom VM images along with the azure provided ones, creating nodes using custom VMs, and using NodeSize, NodeLocation, and NodeImage for the arguments to create node. I also rebased this branch on almost the latest trunk and fixed some issues there.

It looks like there might be some overlap in some of the current efforts and I imagine the easiest approach is for me to pull in the changes and then open a PR back to this branch. Is anything else being actively worked on that I should wait for?


Reply to this email directly or view it on GitHubhttps://github.com//pull/305#issuecomment-60950727.

@bennettaur
Copy link
Contributor

bennettaur commented Nov 24, 2014

Hey so I created this pull request against your branch: baldwinSPC#3

I merged in the update for XML generation and also added support for handling 307 responses from Azure (before the driver had 307 as a successful call but threw an exception when handling it). I was getting these during testing which was getting in the way.

I've also rebased this onto apache's trunk which makes the branch able to be automatically merged so hopefully we can get this merged (I checked that it indeed can be) in soon :)

@baldwinSPC
Copy link
Contributor Author

baldwinSPC commented Dec 5, 2014

Hey Michael:

Just updating the thread. Thanks for sending this over. I see it and will
have a chance to work on it over this weekend. I was overseas when you sent
this over originally.

Thanks.
-matt

On Mon, Nov 24, 2014 at 7:04 AM, Michael Bennett notifications@github.com
wrote:

Hey so I created this pull request against your branch: baldwinSPC#3
baldwinSPC#3

I merged in the update for XML generation and also added support for
handling 307 responses from Azure (before the driver had 307 as a
successful call but threw an exception when handling it). I was getting
these during testing which was getting in the way.

I've also rebased this onto apache's trunk which makes the branch able to
be automatically merged so hopefully we can get this merged (I checked that
it indeed can be) in soon :)


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

@sebgoa
Copy link
Member

sebgoa commented Feb 27, 2015

Hi folks, can you rebase and squash all commits. If it contains tests and passes the travis builds, I am happy to merge it.

@sebgoa
Copy link
Member

sebgoa commented Feb 27, 2015

@Kami can you check this. It's large...

@bennettaur
Copy link
Contributor

bennettaur commented Feb 27, 2015

There is still a pending pull request on their branch here: baldwinSPC#3 which included a rebase. For the branch waiting to be merged (https://github.com/SecurityCompass/libcloud/tree/LIBCLOUD-556_azure_compute_driver_rebased) it looks like it can still be merged automatically (just checked) so it's a matter of whether it passes a travis build.

I can either create a new PR for my branch and address any issues there, or we can stick with this PR

@sebgoa
Copy link
Member

sebgoa commented Feb 27, 2015

@bennettaur in order to give credits to the right folks, it'd be better if you created another PR with your work. We can merge that, and then folks can patch it with new PR.

@baldwinSPC
Copy link
Contributor Author

baldwinSPC commented Feb 27, 2015

Sorry for the other PR being stuck. I have no time to commit to it at this
point due to way to many work obligations. How would you like to move
forward?
On Feb 27, 2015 7:34 AM, "runseb" notifications@github.com wrote:

@bennettaur https://github.com/bennettaur in order to give credits to
the right folks, it'd be better if you created another PR with your work.
We can merge that, and then folks can patch it with new PR.


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

@sebgoa
Copy link
Member

sebgoa commented Feb 27, 2015

@bennettaur @baldwinSPC so you have a pending PR on a fork of libcloud. In any case PR from different individuals should come straight upstream so we can properly keep track or authorship. So tell me what I can merge in libcloud upstream right now, and I will do so. Then you can patch this upstream driver. Does that make sense ?

@azurecoder
Copy link
Contributor

azurecoder commented Feb 27, 2015

@davidcrossland and I did quite a bit of initial work on this but I think all code is in @baldwinSPC branch so a single PR from there should suffice.

…_driver_rebased

Support for Virtual Machine Images and handling of Temp Redirects
@baldwinSPC
Copy link
Contributor Author

baldwinSPC commented Feb 27, 2015

I just merged your PR @bennettaur into the branch. @Runseb PR 305 is
currently being re-validated by Travis. Will this be good for you @Runseb?

On Fri, Feb 27, 2015 at 9:07 AM, Richard Conway notifications@github.com
wrote:

@davidcrossland https://github.com/davidcrossland and I did quite a bit
of initial work on this but I think all code is in @baldwinSPC
https://github.com/baldwinSPC branch so a single PR from there should
suffice.


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

@sebgoa
Copy link
Member

sebgoa commented Feb 27, 2015

Let me check with @Kami cause we may have an authorship tracking problem here.

@baldwinSPC
Copy link
Contributor Author

baldwinSPC commented Feb 27, 2015

OK, thanks @Runseb I believe the current PR has all authors in it, who are,
if I'm not mistaken:

@bennettaur
@davidcrossland
@azurecoder
@baldwinSPC
@baldwinmathew

-matt

On Fri, Feb 27, 2015 at 9:35 AM, runseb notifications@github.com wrote:

Let me check with @Kami https://github.com/Kami cause we may have an
authorship tracking problem here.


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

@Kami
Copy link
Member

Kami commented Apr 4, 2015

Sorry everyone for the delay. The pull request and the patch itself is just big and I'm busy and had problems finding a big enough chunk of time which would allow me to thoroughly review the changes.

I finally started working on it today though. I created a new branch based on this pull request where I will push any cleanup or changes which are needed (I will open a new PR shortly). Hopefully patch will finally land in trunk soon.

@Kami Kami mentioned this pull request Apr 5, 2015
@Kami
Copy link
Member

Kami commented Apr 5, 2015

Lets continue in #499.

@asfgit asfgit merged commit 33e0090 into apache:trunk Apr 6, 2015
1 check failed
1 check failed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
asfgit pushed a commit that referenced this pull request Apr 6, 2015
Closes #305
Closes #499
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

9 participants
You can’t perform that action at this time.