On 02/19, mar...@redhat.com wrote:
> 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.
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 <mfoj...@redhat.com>
Deltacloud API, CloudForms
