incubator-deltacloud-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Eric Woods <woods...@gmail.com>
Subject Re: IBM SBC Driver Available
Date Mon, 31 Jan 2011 19:37:28 GMT
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
> 
> 


Mime
View raw message