Hi David, thanks for your comments. Geemus (Wesley) just notified me that he has pulled in my edits into fog so I will sanity check that today and make the edits you suggest here before sending out the patch again, likely today but maybe tomorrow,
all the best, marios On 11/05/10 00:43, David Lutterkort wrote: > One problem I ran into: when I created a new instance (IIRC, from image > 3), the API isn't able to show that to me - it bombs with 'ERROR: > private method `split' called for nil:NilClass' and shows an empty web > page > > On Mon, 2010-05-03 at 01:04 +0100, [email protected] wrote: >> From: marios<[email protected]> >> >> --- >> .../drivers/terremark/terremark_driver.rb | 271 >> ++++++++++++++++++++ >> server/lib/drivers.rb | 1 + >> 2 files changed, 272 insertions(+), 0 deletions(-) >> create mode 100644 >> server/lib/deltacloud/drivers/terremark/terremark_driver.rb >> >> diff --git a/server/lib/deltacloud/drivers/terremark/terremark_driver.rb >> b/server/lib/deltacloud/drivers/terremark/terremark_driver.rb >> new file mode 100644 >> index 0000000..b7ff9bd >> --- /dev/null >> +++ b/server/lib/deltacloud/drivers/terremark/terremark_driver.rb >> @@ -0,0 +1,271 @@ > >> +require 'deltacloud/base_driver' >> +path = File.dirname(__FILE__) >> +$:<< path >> +begin >> + require 'fog' >> + require 'excon' >> + require 'nokogiri' >> +rescue LoadError >> + puts "ERROR: please ensure fog, excon and nokogiri gems installed. ('gem >> install fog', 'gem install excon' , 'gem install nokogiri')" >> + exit(1) >> +end > > I don't like doing this kind of idiot-proofing all over the code. Just > take out catching the LoadError, and we'll need to fix deltacloudd to > produce more understandable errors when loading fails. > >> +#-- >> +# IMAGES >> +#-- >> +#aka "vapp_templates" >> + def images(credentials, opts=nil) >> + image_list = [] >> + safely do >> + terremark_client = new_client(credentials) >> + vdc_id = terremark_client.default_vdc_id >> + catalogItems = >> terremark_client.get_catalog(vdc_id).body['CatalogItems'] >> + catalogItems.each{|catalog_item| >> + current_item_id = catalog_item['href'].split('/').last >> + current_item = >> terremark_client.get_catalog_item(current_item_id).body['Entity'] >> + if(current_item['type'] == >> 'application/vnd.vmware.vcloud.vAppTemplate+xml') >> + image_list<< convert_image(current_item, credentials.user) >> + end >> + } #end of catalogItems.each >> + end #end of safely do >> + image_list = filter_on( image_list, :id, opts ) >> + image_list = filter_on( image_list, :architecture, opts ) >> + image_list = filter_on( image_list, :owner_id, opts ) >> + image_list >> + end > > As mentioned in the other mail, the problem here is that the API > currently forces you to make n+1 calls to Terremark. Leave that as is > for now, and we'll address it separately once the driver has been > committed. > >> +#-- >> +# INSTANCES >> +#-- >> +#aka vApps >> + def instances(credentials, opts=nil) >> + instances = [] >> + safely do >> + terremark_client = new_client(credentials) >> + vdc_items = >> terremark_client.get_vdc(terremark_client.default_vdc_id()).body['ResourceEntities'] >> + vdc_items.each{|current_item| >> + if(current_item['type'] == >> 'application/vnd.vmware.vcloud.vApp+xml') >> + vapp_id = current_item['href'].split('/').last >> + vapp = terremark_client.get_vapp(vapp_id) >> + instances<< convert_instance(vapp, terremark_client, >> credentials.user) >> + end >> + }#end vdc_items.each >> + end #end safely do >> + instances = filter_on( instances, :id, opts ) >> + instances >> + end > > This has the same issue as the images call, and we should fix it up in > the same way. There's no way we can do 1 + 1000 calls to the backend > here. > >> +#-- >> +# CREATE INSTANCE >> +#-- >> +#launch a vapp template. Needs a name, ram, no. cpus, id of vapp_template >> + def create_instance(credentials, image_id, opts) >> + new_vapp = nil >> + vapp_opts = {} #assemble options to pass to >> Fog::Terremark::Real.instantiate_vapp_template >> + terremark_hwp = hardware_profiles(credentials, {:name => >> 'default'}).first #sanity check values against default >> + name = opts['name'] #name could be nil or length 0 or too long >> + name = "inst#{Time.now.to_i}" if (name.nil? || (name.length == 0)) >> + name = name.slice(0..14) #name max 15 chars >> + unless ( (terremark_hwp.include?(:cpu, opts[:hwp_cpu].to_i))&& >> + (terremark_hwp.include?(:memory, opts[:hwp_memory].to_i)) ) >> then >> + raise >> Deltacloud::Validation::Failure.new(Deltacloud::Validation::Param.new(["cpu"]), >> "Error with cpu and/or memory values. you said cpu->#{opts[:hwp_cpu]} and >> mem->#{opts[:hwp_memory]}") >> + end >> + vapp_opts['cpus'] = opts[:hwp_cpu] >> + vapp_opts['memory'] = opts[:hwp_memory] >> + terremark_client = new_client(credentials) >> + safely do >> +####### >> +#FIXME# what happens if there is an issue getting the new vapp id? (eg >> even though created succesfully) >> +####### >> + vapp_id = terremark_client.instantiate_vapp_template(name, image_id, >> vapp_opts).body['href'].split('/').last >> + new_vapp = terremark_client.get_vapp(vapp_id) >> + return convert_instance(new_vapp, terremark_client, credentials.user) >> #return an Instance object >> + end >> + end > > The above is also an issue with reliability; again, this can be > committed as is, since it requires an API change to allow us to return > VM details as an option. > >> + private >> + >> +#-- >> +# CONVERT IMAGE >> +#-- >> +#gets a vapp_template from a catalog and makes it a Image >> + def convert_image(catalog_vapp_template, account_name) >> + name = catalog_vapp_template['name'] >> + #much fudging ensues >> + arch = name.scan(/[36][24].bit/).first > > The arch we report should either be 'i386' or 'x86_64'. Also, shouldn't > the regex above be /(32|64)\.bit/ ? > >> + arch ||= "n/a" > > Just leave the architecture out if we don't get one from the backend. > >> +#-- >> +# CONVERT INSTANCE >> +#-- >> + def convert_instance(vapp, terremark_client, account_name) >> + vapp_private_ip = vapp.body['IpAddress'] >> + vapp_public_ip = >> terremark_client.get_public_ips(terremark_client.default_vdc_id).body['PublicIpAddresses'].first['name']#get_public_address(terremark_client, >> vapp_private_ip) >> + vapp_status = vapp.body['status'] >> + current_state = ((vapp_status == "0" || vapp_status == "1") ? >> "PENDING" : (vapp_status == "2" ? "STOPPED" : "RUNNING" )) #status == >> 0->BEING_CREATED 2->OFF 4->ON > > This would be clearer if you used a hash, i.e. definea constant in the > class > > VAPP_STATE_MAP = { "0" => "PENDING", "1" => "PENDING", "2" => > "STOPPED", "4" => "RUNNING" } > > and set 'current_state = VAPP_STATE_MAP[vapp_status]'. Might even be > worth a small helper method so that you can check for invalid/unexpected > states and raise an error. > >> + Instance.new( { >> + :id => vapp.body['href'].split('/').last, >> + :owner_id => "#{account_name}", >> + :image_id => "n/a", #cant get this... see >> https://community.vcloudexpress.terremark.com/en-us/discussion_forums/f/60/t/376.aspx >> + :name => vapp.body['name'], >> + :realm_id => "US-Miami", >> + :state => current_state, >> + :actions => instance_actions_for(current_state), >> + :public_addresses => vapp_public_ip, >> + :private_addresses => vapp_private_ip, >> + :instance_profile => InstanceProfile.new("default") >> + } ) >> + end >> + >> +#-- >> +# NEW CLIENT >> +#-- >> +#use supplied credentials to make a new client for talking to terremark >> + def new_client(credentials) >> + #Fog constructor expecting credentials[:terremark_password] and >> credentials[:terremark_username] >> + terremark_credentials = {:terremark_vcloud_username => >> "#{credentials.user}", :terremark_vcloud_password => >> "#{credentials.password}" } >> + safely do >> + terremark_client = Fog::Terremark::Vcloud.new(terremark_credentials) >> + vdc_id = terremark_client.default_vdc_id >> + if (vdc_id.nil?) >> + raise DeltaCloud::AuthException.new >> + end >> + return terremark_client >> + end >> + end >> + >> + def safely(&block) >> + begin >> + block.call >> +# rescue Excon::Errors.status_error => e #this is what you get from the >> fog requests if :expects status code is off >> +# raise Deltacloud::AuthException.new >> + rescue Exception => e >> + puts "ERROR: #{e.message}" >> + end >> + end > > I know we have 'safely' all over the code base, but it has to die. > Rescuing any exception and then just printing is a gigantic bug. It > really, really needs to go. > > Given the maturity of the code base, we should only ever catch > exceptions we know how to handle, and either raise another exception or > really address the exception, not print it. Just remove any use of > 'safely' from the driver, and let stuff blow up in its full glory. > > Handling 'weird' errors, i.e. errors we have no idea what to do about > belongs into server.rb; for example, for XML clients, we'd want to > return an apporpriate 500 response. > > David > > > _______________________________________________ > deltacloud-devel mailing list > [email protected] > https://fedorahosted.org/mailman/listinfo/deltacloud-devel _______________________________________________ deltacloud-devel mailing list [email protected] https://fedorahosted.org/mailman/listinfo/deltacloud-devel
