deltacloud-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "marios@redhat.com" <mandr...@redhat.com>
Subject Re: [PATCH 1/2] Deltacloud OpenStack driver - remap 'regions' to 'providers' DTACLOUD-488
Date Tue, 19 Feb 2013 17:01:38 GMT
On 19/02/13 18:54, Michal Fojtik wrote:
> On 02/19, marios@redhat.com wrote:
> 
> ACK, some minor coding/formatting hints inline :)
> 
>   -- Michal
> 
>> From: marios <marios@redhat.com>
>>
>> https://issues.apache.org/jira/browse/DTACLOUD-488
>>
>> Signed-off-by: marios <marios@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
>>
> 


Mime
View raw message