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 3/3] Updated openstack driver for openstack compute v2.0 API (openstack compute release 'diablo')
Date Tue, 28 Feb 2012 16:11:07 GMT
many thanks for taking the time. Comments inline (and new patches on
list with the fixes below):

On 24/02/12 02:06, David Lutterkort wrote:
> Hi Marios,
> 
> On Thu, 2012-02-23 at 15:28 +0200, marios@redhat.com wrote:
>> From: marios <marios@redhat.com>
>>
>>
>> Signed-off-by: marios <marios@redhat.com>
>> ---
> 
>       * We also need an update to the gemspec to add the openstack gems

done

>       * When supplying the wrong credentials, you get a server error
>         with a stacktrace going to new_client - we need to produce at
>         least a 401

done

>       * When I click on an image (/api/images/1358), I get a 500
>         "undefined method `each' for nil:NilClass" with a stacktrace
>         ending in server/views/images/show.html.haml:26:in
>         `evaluate_source'

done (yes, this was due to the addition of 'hardware_profiles' to
images, which was only added after I rebased my branch and made patches
for the list. i.e., the haml was doing something like
@image.hardware_profiles which was nil... added now)

> 
> More inline:
> 
>>  .../drivers/openstack/openstack_driver.rb          |  295 +++++++++++++++++++-
>>  1 files changed, 289 insertions(+), 6 deletions(-)
>>
>> +      class OpenstackDriver < Deltacloud::BaseDriver
> 
> What do we do about backward compat ? I am not sure how widely the v1.0
> API is deployed, but we now leave people who won't immediately upgrade
> to v2 in the dust. Is there any way to dynamically determine which
> version of the API we're talking to ? 

yes, based on the Auth url provided as api_provider - i.e. the way the
openstack gem does it. in theory, the openstack gem handles this so the
driver should work for both... we need some testing against both
versions and put in safety/begin-rescue clauses to handle the edge
cases. I put in a private boolean method 'api_v2' for convenience in
new_client (whether or not to blow up when its v2 and the client didn't
provide both username and authtenant).


> 
>>          define_instance_states do
>>            start.to( :pending )          .on( :create )
>>            pending.to( :running )        .automatically
> 
> Is this still up-to-date with v2 ?
> 

yes as far as I can see there are no changes here

>> @@ -33,15 +39,292 @@ module Deltacloud
>> +
>> +        def images(credentials, opts=nil)
>> +          os = new_client(credentials)
>> +          owner = extract_user_tenant(credentials.user).first
> 
> Couldn't we just use os.authuser/os.authtenant here ? That way, we could
> get rid of extract_user_tenant
> 

good point, though we still need to extract it for 'new_client' - but
that can be done once in the new_client method and we don't need
'extract_user_tenant' as seperate method


>> +          results = []
>> +          safely do
>> +            if(opts[:id])
>> +              img = os.get_image(opts[:id])
>> +              results << convert_from_image(img, owner)
>> +            else
>> +              results = os.list_images.collect do |img|
>> +                convert_from_image(img, owner)
>> +              end
>> +            end
>> +          end
>> +          filter_on(results, :id, opts)
> 
> That's not needed (I fear we have that in lots of drivers)
> 


DONE... yeah, this is a remnant from when we used to 'get all' and then
filter for opts[:id] ... i guess now we have a if(opts[:id]) clause in
all drivers.

>> +        def destroy_image(credentials, image_id)
>> +          os = new_client(credentials)
>> +          safely do
>> +            image = os.get_image(image_id)
>> +            unless image.delete!
>> +              raise "ERROR: Cannot delete image with ID:#{image_id}"
> 
> What error does that lead to in the API ? Can we pass on more
> informative details based on what the backend tells us ?
> 

actually, looking at the gem code some more
[/gems/openstack-compute-1.1.7/lib/openstack/compute/image.rb:65]:

 the safely..do will handle that. image.delete! will blow up unless the
operation is successful. In other words, the image.delete! method
returns true only if successful. Otherwise it raises an exception based
on the response from the Openstack server. This exception will be
propagated to the client because of safely..do. The actual errors that
can be raised here as defined by the API are numerous:
http://docs.openstack.org/api/openstack-compute/2/content/Delete_Image-d1e4957.html
(computeFault (400, 500, …), serviceUnavailable (503), unauthorized
(401), forbidden (403), badRequest (400), badMethod (405), overLimit
(413), itemNotFound (404) )


>> +        def realms(credentials, opts=nil)
>> +          os = new_client(credentials)
>> +          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 OpenStack_2>> Total: #{rate[:limit].first[:value]}
 Remaining: #{rate[:limit].first[:remaining]} Time Unit: per #{rate[:limit].first[:unit]}"
>> +                end
>> +              end
>> +          end
>> +          [ Realm.new( { :id=>'default',
>> +                        :name=>'default',
>> +                        :limit => limits,
>> +                        :state=>'AVAILABLE' })]
> 
> I had completely forgotten we had limits in realms; we should probably
> have a closer look at how we report that information, so that it's
> consumable in the XML. But that can be a separate cleanup patch.
> 

yes... for now I put a ![CDATA[] around the limit xml (as far as grep is
concerned... this is the only driver we have limits in right now)



>> +        def instances(credentials, opts={})
>> +          os = new_client(credentials)
>> +          insts = []
>> +          owner = extract_user_tenant(credentials.user).first
>> +          safely do
>> +            begin
>> +              if opts[:id]
>> +                server = os.get_server(opts[:id].to_i)
>> +                insts << convert_from_server(server, owner)
>> +              else
>> +                insts = os.list_servers_detail.collect do |server|
>> +                  convert_from_server(server, owner)
>> +                end
>> +              end
>> +            rescue OpenStack::Compute::Exception::ItemNotFound
> 
> Shouldn't we report a 404 in that case ? And can ItemNotFound happen
> even when opts[:id] is not set ?
> 

yeah sorry this must have been remnant of debugging. I let safely do...
handle this and put a     "   on /Exception::ItemNotFound/ do" in the
driver 'on exceptions'

>> +#NOTE: for the convert_from_foo methods below... openstack-compute
>> +#gives Hash for 'flavors' but OpenStack::Compute::Flavor for 'flavor'
>> +#hence the use of 'send' to deal with both cases and save duplication
> 
> Eeeew ... nice hack around that; is there any way this can be addressed
> upstream ? Could be as simple as implementing
> OpenStack::Compute::Flavor.[]
> 
>> +        def extract_user_tenant(user_tenant)
>> +          tokens = user_tenant.split("+")
>> +          return tokens.first, tokens.last
> 
> If we get more than two tokens from the split, we should complain very
> loudly; otherwise, people will get a 401 and not really understand why.
> 

yes done, with the added clause that '<2 tokens AND apiv2' , i.e. its
acceptable to have <2 tokens if this is api v1, cos we don't need
authtenant in that case.


marios

> David
> 


Mime
View raw message