deltacloud-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Michal Fojtik <mfoj...@redhat.com>
Subject Re: [PATCH 1/2] Deltacloud OpenStack driver - remap 'regions' to 'providers' DTACLOUD-488
Date Tue, 19 Feb 2013 17:07:00 GMT
On 02/19, marios@redhat.com wrote:
> 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.

Ah right, sorry :-( I need better lens :)

> 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
> >>
> > 
> 

-- 
Michal Fojtik <mfojtik@redhat.com>
Deltacloud API, CloudForms

Mime
View raw message