On Fri, 2011-01-28 at 17:51 -0500, Eric Woods wrote:
> Hi All,
>
> As promised, I've submitted a patch for the IBM SBC driver to JIRA:
> https://issues.apache.org/jira/browse/DTACLOUD-15.
Excellent ! Thanks a ton for writing the driver.
> The patch is fully functional but has a few remaining (minor) todos.
> I'd like to reach out for help on two issues:
>
> 1) sbc_client.rb:180: For error handling, I raise
> Deltacloud:BackendErrors with error codes & messages that align with
> IBM's cloud. These attributes are always ignored and the client sees
> a 500 Internal Server Error. Is this the expected (generic) behavior,
> or should the client see error codes & messages specific to each cloud
> provider? Perhaps this could be a standardization opportunity.
What we do in other places is return a 502 Bad Gateway to indicate a
generic backend error. Of course, if, for example, authentication fails,
you should raise a Deltacloud::AuthException.
There's a bit of machinery for that: you can implement
catched_excpetion_list, and enclose anything where you want to have
backend errors mapped in a safely do .. end. (see the end of
server/lib/deltacloud/base_driver/base_driver.rb for details)
Or, in this case, you can raise the proper Deltacloud exception directly
from the SBC client.
> 2) sbc_driver.rb:198: While converting each IBM instance to
> DeltaCloud's instances, I construct the an InstanceProfile by passing
> a HardwareProfile's name (the hardware profiles are defined at the
> bottom of the driver). However, InstanceProfile does not to correctly
> lookup the corresponding HardwareProfile by name. The
> InstanceProfile's id is set correctly, but cpu, storage, memory, and
> architecture are all null. I can't find what I'm doing wrong or
> different from the other drivers.
That's the right behavior. cpu/storage/memory in InstanceProfile are
only set if the instance's sizing deviates from what's already in the
HWP. For SBC, that's not possible, but some other clouds let you choose,
for example, memory in some range.
> Help with these issues and technical feedback is very appreciated. Thanks!
One general thing: can you talk to Michal and figure out a way to add
cucumber tests for the IBM SBC cloud ? We need to make sure we have some
minimal test coverage to spot bitrot early on.
Also, is there some sort of test account we could use to run the driver
against the actual cloud ?
Some more detailed comments on the driver:
First nit: please do not use tabs in the source; the rest of the code
uses 2 space indentation - please reformat accordingly.
Index: server/lib/drivers.rb
===================================================================
--- server/lib/drivers.rb (revision 1064909)
+++ server/lib/drivers.rb (working copy)
@@ -20,6 +20,7 @@
module Deltacloud
DRIVERS = {
:ec2 => { :name => "EC2" },
+ :sbc => { :name => "SBC" },
:rackspace => { :name => "Rackspace" },
:gogrid => { :name => "Gogrid" },
:rhevm => { :name => "RHEVM" },
Index: server/lib/deltacloud/drivers/sbc/sbc_client.rb
===================================================================
--- server/lib/deltacloud/drivers/sbc/sbc_client.rb (revision 0)
+++ server/lib/deltacloud/drivers/sbc/sbc_client.rb (revision 0)
@@ -0,0 +1,199 @@
+#
+# Copyright (C) 2009, 2010 Red Hat, Inc.
That should be either copyright you or IBM, depending what your IP
agreement is with your employer. And it should be 'Copyright (C) 2011'
+module Deltacloud
+ module Drivers
+ module SBC
+
+#
+# Client for the IBM Smart Business Cloud (SBC).
+#
+# Author: Eric Woods
+# Date: 18 January 2011
+#
+class SBCClient
+
+ @@API_URL =
URI.parse('https://www-147.ibm.com/computecloud/enterprise/api/rest/20100331')
This should be a constant ('API_URL = ' rather than '@@API_URL = ')
+ #
+ # Retrieve instances
+ #
+ def list_instances(instance_id=nil)
+ if instance_id.nil?
+ JSON.parse(get('/instances',
default_headers))['instances']
+ else
+ instances = []
+ instances << JSON.parse(get('/instances/' +
instance_id, default_headers))
+ instances
You can write the else branch more concisely as
else
[ JSON.parse(get('/instances/' + instance_id,
default_headers)) ]
end
+ #
+ # Retrieve locations; returns an XML document.
+ #
+ def list_locations
+ puts 'retrieving locations...'
Leftover from debugging ?
+
+ #
+ # Utility to URL encode a hash.
+ #
+ def urlencode(hash)
+ query = ''
+ hash.each do |name, value|
+ if !query.empty?
+ query += '&'
+ end
+ query += URI.encode(name.to_s) + '=' +
URI.encode(value.to_s)
+ end
+ query
+ end
Here's the more Rubyish oneliner to do the same:
def urlencode(hash)
hash.keys.map { |k| "#{k}=#{hash[k]}" }.join("&")
end
Index: server/lib/deltacloud/drivers/sbc/sbc_driver.rb
===================================================================
--- server/lib/deltacloud/drivers/sbc/sbc_driver.rb (revision 0)
+++ server/lib/deltacloud/drivers/sbc/sbc_driver.rb (revision 0)
@@ -0,0 +1,300 @@
+#
+# Copyright (C) 2009, 2010 Red Hat, Inc.
Same comment about copyright notice.
+class SBCDriver < Deltacloud::BaseDriver
+ #
+ # Retrieves images
+ #
+ def images(credentials, opts={})
+ sbc_client = new_client(credentials)
+ opts ||= {}
+ images = []
+ images = sbc_client.list_images(opts[:id]).map do |image|
+ @last_image = image
+ convert_image(image)
+ end
Don't store @last_image ... code farther down will blow up if it is
called before images is ever called. The above should simply be
images = sbc_client.list_images(opts[:id]).map { |img|
convert_image(img) }
+ images = filter_on(images, :architecture, opts)
+ images = filter_on(images, :owner_id, opts)
+ images
+ end
+
+ #
+ # Retrieves realms
+ #
+ def realms(credentials, opts={})
+ sbc_client = new_client(credentials)
+ locations = []
+ doc = sbc_client.list_locations
+ doc.xpath('ns2:DescribeLocationsResponse/Location').each do
|location|
+ locations << convert_location(location)
+ end
+ locations
+ end
+
There's no need for an explicit locations array; the function can be simplified
to
def realms(credentials, opts={})
sbc_client = new_client(credentials)
doc = sbc_client.list_locations
doc.xpath('ns2:DescribeLocationsResponse/Location').map
{ |loc| convert_location(location) }
end
+ #
+ # Retrieves instances
+ #
+ def instances(credentials, opts={})
+ sbc_client = new_client(credentials)
+ opts ||= {}
This should be unnecessary unless somebody passes in an explicit nil (in
which case they deserve the ensuing exception)
+ instances = []
+ instances = sbc_client.list_instances(opts[:id]).map do
|instance|
+ convert_instance(instance)
+ end
+ instances
The variable 'instances' isn't needed.
+ #
+ # Creates an instance
+ #
+ def create_instance(credentials, image_id, opts={})
+ sbc_client = new_client(credentials)
+
+ # Copy opts to body; keywords are mapped later
+ body = opts.clone
You mean opts.dup here (it's subtle, just believe me ;)
+ body.delete('image_id')
+ body.delete('hwp_id')
+ body.delete('realm_id')
+
+ # Map DeltaCloud keywords to SBC
+ body['imageID'] = opts[:image_id]
+ body['location'] = opts[:realm_id] || @last_image['location']
+ body['instanceType'] = opts[:hwp_id] ||
@last_image['supportedInstanceTypes'][0]['id']
This is very dangerous, since @last_image might be nil. There's two ways
to get out of missing realm_id/hwp_id parameters: (1) Hardcode a default
in the driver or (2) look up the image that gets passed in and use its
location/instanceType.
+ # Submit instance
+ instances = []
+ instances = sbc_client.create_instance(body).map do |instance|
+ convert_instance(instance)
+ end
+ instances
You can simply return "sbc_client.create_instance(body)" and omit any mention
of instances.
+ end
+
+ #
+ # Reboots an instance
+ #
+ def reboot_instance(credentials, instance_id)
+ sbc_client = new_client(credentials)
+ sbc_client.reboot_instance(instance_id)
+ end
The client simply returns the body of the response from reboot_instance;
this method though needs to return an object of class Instance. The same
is true for hte other instance actions stop and destroy.
David
