On 08/19/2011 02:47 PM, David Lutterkort wrote:
Hi Scott,
couple questions/comments on usage of the client:
On Fri, 2011-08-19 at 14:35 -0400, Scott Seago wrote:
> diff --git a/src/app/models/provider.rb b/src/app/models/provider.rb
> index a099c56..0947a4a 100644
> --- a/src/app/models/provider.rb
> +++ b/src/app/models/provider.rb
> @@ -64,6 +64,11 @@ class Provider< ActiveRecord::Base
>
> before_destroy :destroyable?
>
> + def encoded_url_with_driver_and_provider
> + unencoded = ";driver={provider_type.deltacloud_driver}"
> + unencoded += ";provider={deltacloud_provider}" if deltacloud_provider
It seems you want '#{...}' rather than '{...}'
Heh -- good catch. As you can see I haven't fully tested it yet -- but I
wanted to get the patchset out there before I went on PTO.
> @@ -81,7 +86,12 @@ class Provider< ActiveRecord::Base
>
> def connect
> begin
> - return DeltaCloud.new(nil, nil, url)
> + opts = {:username => nil,
> + :password => nil,
> + :driver => provider_type.deltacloud_driver }
> + opts[:provider] = deltacloud_provider if deltacloud_provider
> + client = DeltaCloud.new(nil, nil, url)
> + return client.with_config(opts)
If you set driver and provider on the client, there's no need to do URI
rewriting; the client with that config will always use the driver and
provider given in opts.
Yeah -- the only reason for the uri rewriting above is for condor --
condor doesn't use the client, so the URI rewriting was the only way to
do it without rewriting the GAHP -- which I don't want to do at this
point since we're on the verge of removing it.
encoded_url_with_driver_and_provider is only called by the code that
sets up the condor job -- it can go away once we don't need that anymore.
For all Deltacloud calls executed within Conductor, we're using the
client to set it for us.
Also, as a micro-optimization, you should use client.use_config!;
that
avoids dup'ing the client. with_config is mostly meant to be used with a
block, i.e. client.with_config(opts) { |client| ... }
Yeah I was talking to mfojtik about that yesterday -- we should probably
look over our use of the deltacloud client on the conductor side in
general and refactor as needed. Here I'm just trying to get our current
way of doing this to work w/ the driver/provider headers. Any more
substantial refactoring should be a follow-on patch.
> diff --git a/src/app/models/provider_account.rb
b/src/app/models/provider_account.rb
> index 6079a55..b6b0994 100644
> --- a/src/app/models/provider_account.rb
> +++ b/src/app/models/provider_account.rb
> @@ -125,7 +125,14 @@ class ProviderAccount< ActiveRecord::Base
>
> def connect
> begin
> - return DeltaCloud.new(credentials_hash['username'],
credentials_hash['password'], provider.url)
> + opts = {:username => credentials_hash['username'],
> + :password => credentials_hash['password'],
> + :driver => provider.provider_type.deltacloud_driver }
> + opts[:provider] = provider.deltacloud_provider if
provider.deltacloud_provider
> + client = DeltaCloud.new(credentials_hash['username'],
> + credentials_hash['password'],
> + provider.url)
> + return client.with_config(opts)
I haven't looked through the Aeolus code, but should there really be
three places where a client is constructed and driver/provider
selected ? Shouldn't that live in some utility function ?
Yes, we should probably refactor that. I'm making the current calls work
w/ driver/provider here, but once we sort out the condor situation, we
should revisit/streamline our use of the deltacloud client.
Scott
David