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: [PATCH core 3/6] Added initial IP managment for GoGrid
Date Thu, 26 Aug 2010 23:38:30 GMT
On Wed, 2010-08-25 at 16:42 +0200, mfojtik@redhat.com wrote:
> ---
>  server/deltacloud.rb                               |    1 +
>  server/lib/deltacloud/base_driver/base_driver.rb   |    4 ++
>  .../lib/deltacloud/drivers/gogrid/gogrid_driver.rb |   36 ++++++++++++++++++-
>  .../lib/deltacloud/helpers/application_helper.rb   |    1 +
>  server/lib/deltacloud/helpers/conversion_helper.rb |    2 +-
>  server/lib/deltacloud/models/ip_address.rb         |   26 +++++++++++++
>  server/server.rb                                   |   38 ++++++++++++++++++++
>  server/views/ip_addresses/index.html.haml          |   23 ++++++++++++
>  server/views/ip_addresses/show.html.haml           |   20 ++++++++++
>  9 files changed, 149 insertions(+), 2 deletions(-)
>  create mode 100644 server/lib/deltacloud/models/ip_address.rb
>  create mode 100644 server/views/ip_addresses/index.html.haml
>  create mode 100644 server/views/ip_addresses/show.html.haml

Couple of comments:

> diff --git a/server/lib/deltacloud/drivers/gogrid/gogrid_driver.rb b/server/lib/deltacloud/drivers/gogrid/gogrid_driver.rb
> index e6c99c5..8cb0137 100644
> --- a/server/lib/deltacloud/drivers/gogrid/gogrid_driver.rb
> +++ b/server/lib/deltacloud/drivers/gogrid/gogrid_driver.rb
> @@ -192,6 +192,31 @@ class GogridDriver < Deltacloud::BaseDriver
>      return creds
>    end
>  
> +  def ip_addresses(credentials, opts=nil)
> +    gogrid = new_client( credentials )
> +    ips = []
> +    safely do
> +      gogrid.request('grid/ip/list')['list'].each do |ip|
> +        ips << convert_ip_address(ip)
> +      end
> +    end
> +    ips = filter_on( ips, :type, opts )
> +    ips = filter_on( ips, :state, opts )
> +    return ips
> +  end
> +
> +  def ip_address(credentials, opts={})
> +    ip_addresses(credentials).select { |ip| ip.id.eql?(opts[:id].to_i)}.first
> +  end

On the one hand, I like sending back a 501 to indicate that the driver
does not support this operation. But I think it's not enough: so far,
we've been very careful to make sure that clients can discover which
calls are possible before they actually make them.

How will this shake out for, e.g, the Ruby client ? A web UI that uses
the ruby client should at least be able to decide whether to show a
'Create IP Address' dialog to the user before it actually makes any
calls into the ip_addresses collection.

So, we need some metadata for the top-level entrypoint to indicate at
the least whether create is allowed or not.

Destroy is easier since it operates on an existing object, and we can
therefore handle that by conditionally including a destroy action in the
details for each address. That has the advantage that the client needs
to _only_ look at the possible actions for the IP address - it's
conceivable that a cloud might support destroying addresses in general,
but the current user doesn't have permissions to do that. Looks the same
from the user's POV.

> +  def create_ip_address(credentials, opts)
> +    throw BackendFeatureUnsupported.new(501, "GoGrid not supports creating new IP addresses")
> +  end
> +
> +  def destroy_ip_address(credentials, opts)
> +    throw BackendFeatureUnsupported.new(501, "GoGrid not supports destroying new IP
addresses")
> +  end

The messages have some typos: they should be "GoGrid does not support
creating new IP addresses" and "GoGrid does not support destroying IP
addresses".

David



Mime
View raw message