You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@libcloud.apache.org by Viktor Stanchev <vi...@enomaly.com> on 2010/04/27 18:04:55 UTC
Re: [libcloud] Re: svn commit: r938156 - in /incubator/libcloud/trunk:
libcloud/ libcloud/drivers/ test/ test/fixtures/ecp/
Paul,
Thanks for reviewing my code. Replies to specific comments below:
On Mon, Apr 26, 2010 at 4:10 PM, Paul Querna <pa...@querna.org> wrote:
> On Mon, Apr 26, 2010 at 10:58 AM, <or...@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 <vi...@enomaly.com>
> > Signed-off-by: Jeremy Orem <je...@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
Re: [libcloud] Re: svn commit: r938156 - in /incubator/libcloud/trunk:
libcloud/ libcloud/drivers/ test/ test/fixtures/ecp/
Posted by Paul Querna <pa...@querna.org>.
On Tue, Apr 27, 2010 at 12:15 PM, Viktor Stanchev <vi...@enomaly.com> wrote:
> 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?
As long as the Node.id returned from create_node is correct, so a
future call to list_nodes will return a node with the same ID, we
consider this 'good enough'. If you need to block in create_node
until some object is created on the backend, you can see how the
Softlayer driver does this because we can't get the node ID right
away. It isn't ideal, but we are trying to enforce that while
create_node might return a Node object in a state of pre-booting, the
ID attribute is accurate.
> - 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?
I would avoid spinning up a thread, and instead just block. We are
trying to provide an abstraction layer, and sometimes that might be a
little slower, but it would be best to be able to still catch errors
in a consistent manner.
> - 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 constructor should take an URL. We are dealing with this
right now in the Eucalyptus and OpenNebula drivers too. I think we
will end up with a custom subclass of some kind, CustomURLConnection
or some-such in the long run, but for now I'd suggest just copying
what the Eucalyptus driver does in trunk.
> - 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.
What states does ECP expose for a node?
Thanks,
Paul
Re: [libcloud] Re: svn commit: r938156 - in /incubator/libcloud/trunk:
libcloud/ libcloud/drivers/ test/ test/fixtures/ecp/
Posted by Viktor Stanchev <vi...@enomaly.com>.
Paul,
The node.id is consistent, so I'll keep creation the way it is. The libcloud
user would have to check periodically if the VM is done provisioning. The
only thing that concerns me is that they'd have no way of knowing that a
provision failed. It's not common for that to happen though, so it should
be fine.
I made new nodes returned by create_node() be in the PENDING state when the
provision command is sent. I guess the returned node would only be used to
keep track of which nodes the libcloud user is waiting to be provisioned.
The reboot works by blocking so I'm leaving it that way.
Destroy doesn't actually check if the node was successfully deleted. Should
I do that? It seems unnecessary and slows down the call, but it would be
good for error checking. Actually, rebooting doesn't check if the node is
back to "running" either.
The problem with rebooting and destroying is that if rebooting fails to shut
down the node, it will be stuck waiting for it to turn off. Same with
destroying a node. I can set a timeout. Would 30 seconds sound reasonable? 1
minute? This can be cause problems if the connection is very slow.
I don't know where to find the Eucapliptus driver. I don't see eucaliptus.py
in trunk. Can you send me a link? I'm not sure I understand how you want me
to implement custom URLs.
Your states are:
RUNNING = 0
REBOOTING = 1
TERMINATED = 2
PENDING = 3
UNKNOWN = 4
Our states are:
running
off
paused
unknown
The only state that matches directly is "running".
The only time an ECP node is "rebooting" is in the middle of a node.reboot()
call. So basically the only time a node will be left in the "rebooting"
state is when it fails to start after being shut down (shouldn't happen).
I don't think TERMINATED matches our off state. I'm guessing TERMINATED is
the state a node goes into while waiting to be deleted. In ECP this is such
a short period of time that we don't have a state for it. In the driver it
is set when deleting a node.
I mentioned abode how I'm using PENDING.
I'm not sure if I should let the driver return nodes in the "unknown" state.
That statue usually indicates that the node is in the middle of
transitioning from one state to another or the machine it's running on is
not responding. The driver currently doesn't do that, but I can add it if
you think it is necessary.
It would be nice if there was support for turning nodes on and off because I
can imagine we're not the only ones who have this feature.
I've submitted the patch to jira.
https://issues.apache.org/jira/secure/ManageAttachments.jspa?id=12463218
- Viktor
On Tue, Apr 27, 2010 at 8:04 PM, Paul Querna <pa...@querna.org> wrote:
> On Tue, Apr 27, 2010 at 12:15 PM, Viktor Stanchev <vi...@enomaly.com>
> wrote:
> > 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
>
> Seems like the patch didn't make it to the list, maybe attach it to jira
> again?
>
> Thanks,
>
> Paul
>
Re: [libcloud] Re: svn commit: r938156 - in /incubator/libcloud/trunk:
libcloud/ libcloud/drivers/ test/ test/fixtures/ecp/
Posted by Paul Querna <pa...@querna.org>.
On Tue, Apr 27, 2010 at 12:15 PM, Viktor Stanchev <vi...@enomaly.com> wrote:
> 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
Seems like the patch didn't make it to the list, maybe attach it to jira again?
Thanks,
Paul
Re: [libcloud] Re: svn commit: r938156 - in /incubator/libcloud/trunk:
libcloud/ libcloud/drivers/ test/ test/fixtures/ecp/
Posted by Viktor Stanchev <vi...@enomaly.com>.
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 <vi...@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 <pa...@querna.org> wrote:
>
>> On Mon, Apr 26, 2010 at 10:58 AM, <or...@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 <vi...@enomaly.com>
>> > Signed-off-by: Jeremy Orem <je...@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
>