On 19/02/13 18:54, Michal Fojtik wrote:
> On 02/19, mar...@redhat.com wrote:
>
> ACK, some minor coding/formatting hints inline :)
>
> -- Michal
>
>> From: marios <mar...@redhat.com>
>>
>> https://issues.apache.org/jira/browse/DTACLOUD-488
>>
>> Signed-off-by: marios <mar...@redhat.com>
>> ---
>> .../drivers/openstack/openstack_driver.rb | 114
>> +++++++++++----------
>> 1 file changed, 58 insertions(+), 56 deletions(-)
>>
>> diff --git a/server/lib/deltacloud/drivers/openstack/openstack_driver.rb
>> b/server/lib/deltacloud/drivers/openstack/openstack_driver.rb
>> index ec5cd67..248cf2a 100644
>> --- a/server/lib/deltacloud/drivers/openstack/openstack_driver.rb
>> +++ b/server/lib/deltacloud/drivers/openstack/openstack_driver.rb
>> @@ -43,11 +43,25 @@ module Deltacloud
>> end
>>
>> define_hardware_profile('default')
>> +
>> def supported_collections(credentials)
>> #get the collections as defined by 'capability' and 'respond_to?'
>> blocks
>> super_collections = super
>> - super_collections = super_collections -
>> [Sinatra::Rabbit::BucketsCollection] if regions_for(credentials,
>> "object-store").empty?
>> - super_collections = super_collections -
>> [Sinatra::Rabbit::StorageVolumesCollection] if regions_for(credentials,
>> "volume").empty?
>
> This could be optimized a bit:
>
> if regions_for(credentials, 'object-store').empty?
> super_collections.delete!(Sinatra::Rabbit::BucketsCollection)
> end
>
> if regions_for(credentials, 'volume').empty?
> super_collections.delete!(Sinatra::Rabbit::StorageVolumesCollection)
> end
>
thanks - the regions_for stuff is actually being removed now (you must
have missed the '-' - it was used when 'realms' was actually being
populated with 'regions' and we wanted to keep track of which 'regions'
offer which services. This now goes away as for a given openstack
provider (i.e. region) you only get the 'right' collections exposed at /api.
I agree with the 'begin rescue' stuff here - i may push these as is if
qe agree it works ok - and then look at optimising/tidy up,
marios
>
>> + begin
>> + new_client(credentials, "compute")
>> + rescue Deltacloud::Exceptions::NotImplemented
>> + super_collections = super_collections -
>> [Sinatra::Rabbit::ImagesCollection, Sinatra::Rabbit::InstancesCollection,
>> Sinatra::Rabbit::InstanceStatesCollection,Sinatra::Rabbit::KeysCollection,Sinatra::Rabbit::RealmsCollection,
>> Sinatra::Rabbit::HardwareProfilesCollection]
>
> Just a minor nit, we should try to keep the code < then 75 columns here ^^^
>
>> + end
>> + begin
>> + new_client(credentials, "object-store")
>> + rescue Deltacloud::Exceptions::NotImplemented
>> #OpenStack::Exception::NotImplemented...
>> + super_collections = super_collections -
>> [Sinatra::Rabbit::BucketsCollection]
>> + end
>> + begin
>> + new_client(credentials, "volume")
>> + rescue Deltacloud::Exceptions::NotImplemented
>> + super_collections = super_collections -
>> [Sinatra::Rabbit::StorageVolumesCollection,
>> Sinatra::Rabbit::StorageSnapshotsCollection]
>> + end
>> super_collections
>> end
>
> In general I think this begin/rescue stuff is not very readable, maybe you
> should consider to wrap that into some separate method.
>
>>
>> @@ -115,44 +129,35 @@ module Deltacloud
>> end
>> end
>>
>> + def providers(credentials, opts={})
>> + os = new_client(credentials, "compute", true)
>> + providers = []
>> + os.connection.regions_list.each_pair do |region, services|
>> + resource_types = services.inject([]){|res, cur| res <<
>> cur[:service] if ["compute", "volume",
>> "object-store"].include?(cur[:service]); res }
>> + next if resource_types.empty? #nothing here deltacloud manages
>> + providers << convert_provider(region)
>> + end
>> + providers
>> + end
>> +
>> def realms(credentials, opts={})
>> os = new_client(credentials)
>> realms = []
>> - if opts[:id]
>> - resource_types = []
>> - (os.connection.regions_list[opts[:id]] || []).each do |service|
>> - resource_types << service[:service] if ["compute", "volume",
>> "object-store"].include?(service[:service])
>> - realms << Realm.new( { :id => opts[:id],
>> - :name => opts[:id],
>> - :state =>'AVAILABLE',
>> - :resource_types => resource_types})
>> unless resource_types.empty?
>> - end
>> - else
>> - os.connection.regions_list.each_pair do |region, services|
>> - resource_types = services.inject([]){|res, cur| res <<
>> cur[:service] if ["compute", "volume",
>> "object-store"].include?(cur[:service]); res }
>> - next if resource_types.empty? #nothing here deltacloud manages
>> - realms << Realm.new( { :id => region,
>> - :name => region,
>> - :state =>'AVAILABLE',
>> - :resource_types => resource_types})
>> - end
>> + limits = ""
>> + safely do
>> + lim = os.limits
>> + limits << "ABSOLUTE >> Max. Instances:
>> #{lim[:absolute][:maxTotalInstances]} Max. RAM:
>> #{lim[:absolute][:maxTotalRAMSize]} || "
>> + lim[:rate].each do |rate|
>> + if rate[:regex] =~ /servers/
>> + limits << "SERVERS >> Total:
>> #{rate[:limit].first[:value]} Remaining: #{rate[:limit].first[:remaining]}
>> Time Unit: per #{rate[:limit].first[:unit]}"
>> + end
>> + end
>> end
>> - realms
>> -# limits = ""
>> -# safely do
>> -# lim = os.limits
>> -# limits << "ABSOLUTE >> Max. Instances:
>> #{lim[:absolute][:maxTotalInstances]} Max. RAM:
>> #{lim[:absolute][:maxTotalRAMSize]} || "
>> -# lim[:rate].each do |rate|
>> -# if rate[:regex] =~ /servers/
>> -# limits << "SERVERS >> Total:
>> #{rate[:limit].first[:value]} Remaining: #{rate[:limit].first[:remaining]}
>> Time Unit: per #{rate[:limit].first[:unit]}"
>> -# end
>> -# end
>> -# end
>> -# return [] if opts[:id] and opts[:id] != 'default'
>> -# [ Realm.new( { :id=>'default',
>> -# :name=>'default',
>> -# :limit => limits,
>> -# :state=>'AVAILABLE' })]
>> + return [] if opts[:id] and opts[:id] != 'default'
>> + [ Realm.new( { :id=>'default',
>> + :name=>'default',
>> + :limit => limits,
>> + :state=>'AVAILABLE' })]
>> end
>>
>> def instances(credentials, opts={})
>> @@ -178,13 +183,7 @@ module Deltacloud
>> end
>>
>> def create_instance(credentials, image_id, opts)
>> - if opts[:realm_id] && opts[:realm_id].length > 0
>> - os = new_client( credentials, "compute", opts[:realm_id])
>> - else
>> - #choose a random realm:
>> - available_realms = regions_for(credentials, "compute")
>> - os = new_client(credentials, "compute",
>> available_realms.sample.id)
>> - end
>> + os = new_client( credentials, "compute")
>> result = nil
>> #opts[:personality]: path1='server_path1'. content1='contents1',
>> path2='server_path2', content2='contents2' etc
>> params = {}
>> @@ -484,17 +483,8 @@ module Deltacloud
>> end
>>
>> private
>> -
>> - def region_specified?
>> - api_provider.split(";").last
>> - end
>> -
>> - def regions_for(credentials, service="compute")
>> - realms(credentials).select{|region|
>> region.resource_types.include?(service)}
>> - end
>> -
>> #for v2 authentication credentials.name == "username+tenant_name"
>> - def new_client(credentials, type="compute", realm_id=nil)
>> + def new_client(credentials, type="compute", ignore_provider=false)
>> tokens = credentials.user.split("+")
>> if credentials.user.empty?
>> raise AuthenticationFailure.new(Exception.new("Error: you must
>> supply the username"))
>> @@ -506,12 +496,12 @@ private
>> end
>> #check if region specified with provider:
>> provider = api_provider
>> - if (realm_id || provider.include?(";"))
>> - region = realm_id || provider.split(";").last
>> + if (provider.include?(";"))
>> + region = provider.split(";").last
>> provider = provider.chomp(";#{region}")
>> end
>> connection_params = {:username => user_name, :api_key =>
>> credentials.password, :authtenant => tenant_name, :auth_url => provider,
>> :service_type => type}
>> - connection_params.merge!({:region => region}) if region
>> + connection_params.merge!({:region => region}) if region &&
>> !ignore_provider # hack needed for 'def providers'
>> safely do
>> raise ValidationFailure.new(Exception.new("Error: tried to
>> initialise Openstack connection using" +
>> " an unknown service_type: #{type}")) unless ["volume",
>> "compute", "object-store"].include? type
>> @@ -667,6 +657,14 @@ private
>> )
>> end
>>
>> + def convert_provider(region)
>> + Provider.new(
>> + :id => region,
>> + :name => region,
>> + :url => [api_provider.split(';').first, region].join(';')
>> + )
>> + end
>> +
>> #IN: path1='server_path1'. content1='contents1',
>> path2='server_path2', content2='contents2' etc
>> #OUT:{local_path=>server_path, local_path1=>server_path2 etc}
>> def extract_personality(opts)
>> @@ -705,6 +703,10 @@ private
>> status 401
>> end
>>
>> + on /No API endpoint for region/ do
>> + status 501
>> + end
>> +
>> on /OpenStack::Exception::Authentication/ do
>> status 401
>> end
>> --
>> 1.7.11.7
>>
>
