incubator-deltacloud-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From David Lutterkort <lut...@redhat.com>
Subject Re: OpenStack Support
Date Fri, 10 Sep 2010 23:06:46 GMT
On Fri, 2010-09-10 at 12:41 -0700, Michael Mayo wrote:
> I've attached a small patch to add OpenStack support to Deltacloud.
> Since OpenStack == Rackspace essentially, I moved the Rackspace code
> into new OpenStack code and added an option to set an auth API
> endpoint.  Now, the Rackspace classes simply subclass the OpenStack
> ones with the auth API endpoint defaulted to
> auth.api.rackspacecloud.com.

Thanks for doing this. Is there some public OpenSTack test installation
at which I could point the driver ? Ideally, Michal could do his
capturing magic to capture responses from an OpenStack cloud, so that we
can run feature tests against the canned responses.

To make the review easier, I'll quote the patch here:

> From 990009aaab537c3b1b12ab8e77e83db1756bca86 Mon Sep 17 00:00:00 2001
> From: Mike Mayo <greenisus@gmail.com>
> Date: Fri, 10 Sep 2010 12:24:26 -0700
> Subject: [PATCH] openstack support
>
> diff --git a/server/lib/deltacloud/drivers/openstack/openstack_driver.rb b/server/lib/deltacloud/drivers/openstack/openstack_driver.rb
> new file mode 100644
> index 0000000..bb4b4d2
> --- /dev/null
> +++ b/server/lib/deltacloud/drivers/openstack/openstack_driver.rb
> @@ -0,0 +1,193 @@
> +
> +module Deltacloud
> +  module Drivers
> +    module OpenStack
> +
> +class OpenStackDriver < Deltacloud::BaseDriver
> +
> +  feature :instances, :user_name
> +
> +  def hardware_profiles(credentials, opts = nil)
> +    racks = new_client( credentials, @opts )

Why pass @opts into new_client ? In fact, the second argument is always
@opts, so you could just drop it and access it directily inside the
new_client method.

> +  # OpenStack does not at this stage have realms... its all US/TX, all the time (at
least at time of writing)
> +  def realms(credentials, opts=nil)
> +    [Realm.new( {
> +      :id=>"us",
> +      :name=>"United States",
> +      :state=> "AVAILABLE"
> +    } )]
> +  end
> +

That should be changed for OpenStack -  I assume OpenStack also only has
one realm, though there's no telling where it might be. Unless OpenStack
gives you a way to learn more about the installation, I'd just return a
realm with

  :id => "default", :name => "Default realm", :state => "AVAILABLE"

This raises a question we just discovered with the EC2 driver the other
day: are there any subdivisions of an OpenStack cloud that are important
at this level ? For example, in EC2 images are only usable within the
same availability zone.

> +  def new_client(credentials, opts={})

As I said above, since every invocation of new_client has @opts as its
second argument, I would just drop the second argument and use @opts
inside new_client

> +    safely do
> +      @auth_endpoint_uri = opts[:auth_endpoint_uri]
> +      @opts = { :auth_endpoint_uri => @auth_endpoint_uri }

I am not sure why this is here - I really dislike setting @opts here as
a side effect of new_client.

There also doesn't seem to be a place in the code that initializes the
auth_endpoint_uri. Since we need to pass that in from the outside, there
are two options:

      * Pass it in through the environment, as the EC2 driver does right
        now via DCLOUD_EC2_URL
      * Add an initialize method in BaseDriver to take some sort of
        config hash that is filled from command line arguments to
        deltacloudd (or env vars that deltacloudd sets and drivers.rb
        consumes)

So my suggestion is that we add to BaseDriver

     attr_reader :endpoint_url

        def initialize(opts = {})
          @endpoint_url = opts[:endpoint_url]
        end

and in the driver method in drivers.rb something like

    opts = { :endpoint_url => ENV["DCLOUD_ENDPOINT_URL"] }

which deltacloudd sets from some appropriate command line argument; I
assume that passenger provides a way to do the equivalent from the
Apache config.

David



Mime
View raw message