I've updated the driver based on David's feedback and uploaded a patch:
https://issues.apache.org/jira/browse/DTACLOUD-15. I'm new to ruby, so this
feedback is really appreciated.
Some notes:
1) Michael: I'm interested in writing tests for the driver. What's involved?
2) I maintained an @last_image field to avoid an extra image lookup for
performance reasons. I made this more robust by checking for nil and
performing a lookup if necessary.
3) The online documentation states that nothing is returned from
reboot/stop/destroy. We should update the documentation to indicate an
instance is returned for each of these.
4) For instances(credentials, opts={}), this method/function is being passed
opts=nil which causes an exception, so I kept the line opts ||= {}
5) General cleanup based on David's feedback (thanks!)
- Eric W.
On Jan 28, 2011, at 7:44 PM, David Lutterkort wrote:
> 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
>
>
