On 07/28/2011 11:11 AM, Chris Lalancette wrote:
On 07/28/11 - 10:11:58AM, Martyn Taylor wrote:
> On 07/28/2011 09:19 AM, Chris Lalancette wrote:
>> On 07/27/11 - 04:56:14PM, mtaylor(a)redhat.com wrote:
>>> From: Martyn Taylor <mtaylor(a)redhat.com>
>>>
>>> ---
>>> src/app/util/condormatic.rb | 2 ++
>>> src/config/condor.yml | 1 +
>>> 2 files changed, 3 insertions(+), 0 deletions(-)
>>> create mode 100644 src/config/condor.yml
>>>
>>> diff --git a/src/app/util/condormatic.rb b/src/app/util/condormatic.rb
>>> index b52ff46..45aa4bd 100644
>>> --- a/src/app/util/condormatic.rb
>>> +++ b/src/app/util/condormatic.rb
>>> @@ -29,6 +29,7 @@ class Possible
>>> @hwp = hwp
>>> @provider_image = provider_image
>>> @realm = realm
>>> + @condor_config =
YAML.load_file("#{RAILS_ROOT}/config/condor.yml")
>>> end
>>> end
>>>
>>> @@ -93,6 +94,7 @@ def condormatic_instance_create(task)
>>> pipe_and_log(pipe, "DeltacloudUsername =
#{found.account.credentials_hash['username']}\n")
>>> pipe_and_log(pipe, "DeltacloudPasswordFile = #{pwfilename}")
>>> pipe_and_log(pipe, "DeltacloudImageId =
#{found.provider_image.target_identifier}\n")
>>> + pipe_and_log(pipe, "DeltacloudRetryTimeout =
#{@condor_config[:request_timeout]}\n")
>>> pipe_and_log(pipe,
>>> "DeltacloudHardwareProfile =
#{found.hwp.external_key}\n")
>>> pipe_and_log(pipe,
>>
>> Hm, I'm not sure I love these kinds of patches. In particular, I want us to
>> start getting away from using configuration files, because the administrator
>> has no idea on how to set them and they are not dynamic enough. For instance,
>> in this case, what value would you want the administrator to set? In what
>> circumstances would he change it? If you add a new cloud provider type, is
>> the timeout correct?
>>
>> In this case, I think we can do a lot better by modifying the timeout
>> ourselves. That is, by this point of the code, we know which provider backend
>> we are going to be using. So we should use that information and do a table
>> lookup.
>>
>> EC2? -> default timeout
>> VMware -> 180 seconds
>> RHEV -> 90 seconds
>> etc.
>>
>> (note that I just made the above numbers up, they can be whatever we need to
>> be correct)
>>
> Hi Chris,
>
> I think you have a fair point. Although I was specifically tasked with
> making this value configurable.
>
> How about I put a table like you described above in the config file?
> This allows us to gives the best of both worlds.
>
> I understand what you are saying with regards to configuration being
> complex, though I think an administrator would/should have enough
> information to make a decision on these types of things. Maybe that
> means we need documentation to go along side the configuration. Say my
> VMWare server, is really Slow, I might need to tinker with some of the
> values.
I guess that could be the case. I like your idea of doing the lookup table
in the config file; that is, we provide reasonable default values for the various, and
the
administrator can tinker if needed.
Great, I will amend the patch accordingly.
>
> I do think we could do with an amalgamation of the many files we have
> under config folder and we need to document the resulting file
> appropriately.
Well, there are 2 things here:
1) We should actually think about consolidating all of the configuration into
a "config.ini" file. That will just make it easier to find things.
2) We should really evaluate if these configuration type things should be
settable from the UI. It might make for a nicer user experience that way.
Yeah I definitely agree wrt UI, We did start some work on settings in UI
a while back, but as plans changed I think it was dropped.
Thanks for your comments :)
Martyn