Open Stack

On 01/04/16 12:31, Steven Hardy wrote:
> On Fri, Apr 01, 2016 at 04:15:30PM +0200, Thomas Herve wrote:
>> On Fri, Apr 1, 2016 at 3:21 AM, Zane Bitter <zbitter at redhat.com> wrote:
>>> On 31/03/16 18:10, Zane Bitter wrote:
>>>>
>>>>
>>>> I'm in favour of some sort of variable-based implementation for a few
>>>> reasons. One is that (5) seems to come up fairly regularly in a complex
>>>> deployment like TripleO. Another is that Fn::If feels awkward compared
>>>> to get_variable.
>>>
>>>
>>> I actually have to revise this last part after reviewing the patches.
>>> get_variable can't replace Fn::If, because we'd still need to handle stuff
>>> of the form:
>>>
>>>      some_property: {if: [{get_variable: some_var},
>>>                           {get_resource: res1},
>>>                           {get_resource: res2}]
>>>
>>> where the alternatives can't come from a variable because they contain
>>> resource references and we have said we'd constrain variables to be static.
>>>
>>> In fact the intrinsic functions that could be allowed in the first argument
>>> to the {if: } function would have to constrained in the same way as the
>>> constraint field in the resource, because we should only validate and obtain
>>> dependencies from _one_ of the alternates, so we need to be able to
>>> determine which one statically and not have to wait until the actual value
>>> is resolved. This is possibly the strongest argument for keeping on the cfn
>>> implementation course.
>>
>> We talked about another possibilities on IRC: instead of having a new
>> section, create a new resource OS::Heat::Value which can hold some
>> data. It would look like that:
>>
>> resources:
>>      is_prod:
>>          type: OS::Heat::Value
>>          properties:
>>              value: {equals: {get_param, env}, prod}}
>>
>>      my_resource:
>>          condition: {get_attr: [is_prod, value}}
>
> Another alternative (which maybe you discussed, sorry I missed the IRC
> chat) would be just to use parameters, as that's already conceptually where
> we obtain values that are input to resources.
>
> E.g:
>
> parameters:
>    env:
>     type: string
>     default: prod
>
>    is_prod:
>      type: boolean
>      default: {equals: {get_param, env}}
>
>  From an interface standpoint this seems much cleaner and more intuitive than
> the other solutions discussed IMHO, but I suspect it's potentially harder to
> implement.

Yeah, the problems to solve here would be:

1) Need to define some subset of functions that are valid in templates 
(for HOT, probably all except get_resource and get_attr).

   - Easy enough, just add a separate method called param_functions or 
something to the Template class. We'd have to do the same for the 
"conditions" section anyway.

2) Need to prevent circular references between parameters.

   - Hard. Since currently no templates support parsing parameters for 
functions, we can add a separate implementation without breaking the 
third-party Template API though, so that helps. Lots of edge cases though.

3) Need to make clear to users that only a parameter is a valid input to 
a resource condition.

   - Could be done with something like:

  my_resource:
    condition_parameter: is_prod

     instead of:

  my_resource:
    condition: {get_param: is_prod}

     and something similar for {if: [is_prod, <if_true>, <if_false>]}

4) Need to implement the conditions section in CloudFormation templates.

   - Could have some sort of hacky pseudo-parameter thing.

> Your original example gets much cleaner too if we allow all intrinsic function
> (except get_attr) in the scope of parameters:
>
> parameters:
>    host:
>      type: string
>    port:
>      type: string
>    endpoint:
>      type: string
>      default:
>        str_replace:
>          template:
>             http://HOSTORT/
>          params:
>             HOST: {get_param: host}
>             PORT: {get_param: port}

So the alternative is:

   parameters:
     host:
       type: string
     port:
       type: string

   resources:
     endpoint:
       type: OS::Heat::Value
       properties:
         type: string
         value:
           str_replace:
             template:
               http://HOST:PORT/
             params:
               HOST: {get_param: host}
               PORT: {get_param: port}

which is known to be trivial to implement, and has the advantage that it 
can also combine in data that is only available at runtime (e.g. it's 
easy to imagine that HOST might come from within the template, but that 
you'd still need to use the endpoint URL in enough places that it's not 
worth copying and pasting the whole str_replace function). Also, every 
template format would get it for free, which is nice.

The only downside is that you can't replace the whole value definition 
with a parameter value passed in from outside. Given that it would have 
to be a complete override including the substitute values (i.e. we can't 
parse functions from template _values_, only defaults), I'm not sure 
that is a big loss.

In my view it is almost a toss-up, but I am marginally in favour of a 
conditions section (per the current proposed implementation) plus a 
OS::Heat::Value resource over computable parameter defaults, mainly 
because the implementation would be simpler.

cheers,
Zane.