incubator-libcloud mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Viktor Stanchev <vik...@enomaly.com>
Subject Re: [libcloud] Re: svn commit: r938156 - in /incubator/libcloud/trunk: libcloud/ libcloud/drivers/ test/ test/fixtures/ecp/
Date Tue, 27 Apr 2010 19:15:51 GMT
Paul,

I made all the changes to the ECP driver that were suggested. They are in
the attached patch. I tested it live and everything appears to work
including InvalidCredsException, common errors and all features. I moved
error checking to the universal error checking function rather than having
it for each request.

I have a few questions though.

- When creating a VM, should the driver wait for a VM to be finished
creating before returning?
If so, then I'd have to modify the driver slightly. If not, it seems that
there is no way to check if the creation is still in progress or if it
failed eventually. Maybe you should look into adding a feature to check the
status of transactions?

- The ECP API has no way to "restart" a VM, instead, I'm turning it off and
then back on. This takes a bit longer than normal, but is inevitable. I
guess I can make the reboot happen in a separate thread, but it doesn't seem
that useful and the error checking won't be reliable. Should I try that?

- The ECP software is used by multiple service providers. We only create the
software. Therefore there are multiple URLs that it would connect to. How do
you plan to address this issue? I think adding a wrapper class that sets a
different URL as the "host" would be the best solution, but it's also
possible to allow the user of libcloud to select an arbitrary URL.

- The node states are not used very well right now, but I don't know if
there's anything I can do about it. The node states in ECP don't match the
node states in libcloud.

Viktor

On Tue, Apr 27, 2010 at 12:04 PM, Viktor Stanchev <viktor@enomaly.com>wrote:

> Paul,
>
> Thanks for reviewing my code. Replies to specific comments below:
>
> On Mon, Apr 26, 2010 at 4:10 PM, Paul Querna <paul@querna.org> wrote:
>
>> On Mon, Apr 26, 2010 at 10:58 AM,  <oremj@apache.org> wrote:
>> > Author: oremj
>> > Date: Mon Apr 26 17:58:12 2010
>> > New Revision: 938156
>> >
>> > URL: http://svn.apache.org/viewvc?rev=938156&view=rev
>> > Log:
>> > Add Enomaly ECP Driver.
>> >
>> > Author:    Viktor Stanchev <viktor@enomaly.com>
>> > Signed-off-by: Jeremy Orem <jeremy.orem@gmail.com>
>> ...snip...
>> > +    def request(self, *args, **kwargs):
>> > +        return super(ECPConnection, self).request(*args, **kwargs)
>>
>> Any reason this needs to be in there?
>>
>>
> It seems like it doesn't do anything. I guess I was copying and pasting and
> then deleting things that were unnecessary and that was left over. I'll
> remove it.
>
>
>> ....
>> > +    def _encode_multipart_formdata(self, fields):
>> > +        BOUNDARY = '----------ThIs_Is_tHe_bouNdaRY_$'
>> > +        CRLF = '\r\n'
>> > +        L = []
>> > +        for i in fields.keys():
>> > +            L.append('--' + BOUNDARY)
>> > +            L.append('Content-Disposition: form-data; name="%s"' % i)
>> > +            L.append('')
>> > +            L.append(fields[i])
>> > +        L.append('--' + BOUNDARY + '--')
>> > +        L.append('')
>> > +        body = CRLF.join(L)
>> > +        content_type = 'multipart/form-data; boundary=%s' % BOUNDARY
>> > +        header = {'Content-Type':content_type}
>> > +        return header, body
>>
>> I thought that httplib would support doing this, but it appears it
>> doesn't support the multipart form data encoding:
>> http://bugs.python.org/issue3244
>>
>> I personally kinda wish the boundary was randomly generated, and
>> checked to make sure its content was not inside the content.
>>
>> I guess i would at least be more happy with something like this:
>> boundary = os.urandom(16).encode('hex')
>>
>> (Related, BOUNDARY isn't really a good style, it should be lower case,
>> and I am concerned that this code was copied from
>> <http://code.activestate.com/recipes/146306/> without any attribution)
>>
>>
> You seem to be correct that the code is from there. Sorry about that. I
> didn't copy it intentionally. It was given to me without any context by
> someone when I was trying to figure out why the request wasn't working
> without it. I don't mind adding a link/reference in the code to indicate its
> source. I'll look into making the boundary random, but the benefit is very
> little.
>
>
>> .... snip ....
>> > +class ECPNodeDriver(NodeDriver):
>> > +    def __init__(self, user_name, password):
>> > +        """
>> > +        Sets the username and password on creation. Also creates the
>> connection
>> > +        object
>> > +        """
>> > +        self.user_name = user_name
>> > +        self.password = password
>>
>> Shouldn't these just use the built in key / secret variables, and let
>> the default NodeDriver __init__ do this?
>>
>>
> That would make sense. I'm not sure why I did this. Maybe
> because ECPConnection doesn't have a method called "connect", but I guess I
> can just add to it "def connect(self): pass"
>
>
>> ... snip ...
>> > +    def _to_node(self, vm):
>> > +        """
>> > +        Turns a (json) dictionary into a Node object.
>> > +        This returns only running VMs.
>> > +        """
>> > +
>> > +        #Check state
>> > +        if not vm['state'] == "running":
>> > +            return None
>> > +
>> > +        #IPs
>> > +        iplist = [interface['ip'] for interface in vm['interfaces']  if
>> interface['ip'] != '127.0.0.1']
>> > +
>> > +        #Create the node object
>> > +        n = Node(
>> > +          id=vm['uuid'],
>> > +          name=vm['name'],
>> > +          state=NodeState.RUNNING,
>> > +          public_ip=iplist,
>> > +          private_ip=iplist,
>> > +          driver=self,
>> > +        )
>>
>> This seems less than ideal, setting public_ip and private_ip to the
>> same lists.  It likely needs code similiar to the slicehost driver,
>> which splits the two based on the published private subnets.  We can
>> likely move that function from the slicehost driver to a util area in
>> the base library.
>>
>>
> We don't distinguish between public and private IPs either, so that would
> be nice. Meanwhile, can I copy the function?
>
>
>> It also does not throw InvalidCredsException anywhere, which is bad
>> because then we can't tell if there is a error in the username or
>> password -- so parse_error should be extended to detect those errors
>> and throw the right exception.
>>
>>
> I'll add the error handling.
>
>
>> One last thing, uuid is imported, but unused, so pyflakes complains.
>>
>>
> I'll remove it. I didn't notice that.
>
>
>> Thanks for getting the driver in,
>>
>> Paul
>>
>
> No problem. I'll work on fixing everything mentioned. I'll assume it's ok
> to copy the code from slicehost unless you tell me otherwise.
>
> By the way, I would appreciate it if you could provide some testing code
> that I can run to make sure the driver works on a live server. I would
> imagine I'm not the first person to need to do that.
>
> - Viktor
>

Mime
  • Unnamed multipart/mixed (inline, None, 0 bytes)
View raw message