Open Stack

On 23/03/16 13:14, Steven Hardy wrote:
> Hi all,
>
> I'm looking for some help and additional input on this bug:
>
> https://bugs.launchpad.net/heat/+bug/1559807

Hmm, I was wondering how this ever worked, but it appears you're making 
particularly aggressive use of the list_join and map_merge Functions 
there - where you're not only getting the elements in the list of things 
to merge (as presumably originally envisioned) but actually getting the 
list itself from an intrinsic function. If we're going to support that 
then those functions need to handle the fact that the input argument may 
be None, just as they do for the list members (see the ensure_string() 
and ensure_map() functions inside the result() methods of those two 
Functions).

> Basically, we have multiple issues due to the fact that we consider
> get_attr to resolve to None at any point before a resource is actually
> instantiated.
>
> It's due to this:
>
> https://github.com/openstack/heat/blob/master/heat/engine/hot/functions.py#L163
>
> This then causes problems during validation of several intrinsic functions,
> because if they reference get_attr, they have to contain hacks and
> special-cases to work around the validate-time None value (or, as reported
> in the bug, fail to validate when all would be fine at runtime).
>
> https://github.com/openstack/heat/blob/master/heat/engine/resource.py#L1333
>
> I started digging into fixes, and there are probably a few possible
> approaches, e.g setting stack.Stack.strict_validate always to False, or
> reworking the intrinsic function validation to always work with the
> temporary None value.
>
> However, it's a more widespread issue than just validation - this affects
> any action which happens before the actual stack gets created, so things
> like preview updates are also broken, e.g consider this:
>
> resources:
>    random:
>      type: OS::Heat::RandomString
>
>    config:
>      type: OS::Heat::StructuredConfig
>      properties:
>        group: script
>        config:
>          foo: {get_attr: [random, value]}
>
>    deployment:
>      type: OS::Heat::StructuredDeployment
>      properties:
>        config:
>          get_resource: config
>        server: "dummy"
>
> On update, nothing is replaced, but if you do e.g:
>
>    heat stack-update -x --dry-run
>
> You see this:
>
> | replaced  | config        | OS::Heat::StructuredConfig     |
>
> Which occurs due to the false comparison between the current value of
> "random" and the None value we get from get_attr in the temporary stack
> used for preview comparison:
>
> https://github.com/openstack/heat/blob/master/heat/engine/resource.py#L528
>
> after_props.get(key) returns None, which makes us falsely declare the
> "config" resource gets replaced :(
>
> I'm looking for ideas on how we solve this - it's clearly a major issue
> which completely invalidates the results of validate and preview operations
> in many cases.

I've been thinking about this (for about 2 years).

My first thought (it seemed like a good idea at the time, 2 years ago, 
for some reason) was for Function objects themselves to take on the 
types of their return values, so e.g. a Function returning a list would 
have a __getitem__ method and generally act like a list. Don't try this 
at home, BTW, it doesn't work.

I now think the right answer is to return some placeholder object (but 
not None). Then the validating code can detect the placeholder and do 
some checks. e.g. we would be able to say that the placeholder for 
get_resource on a Cinder volume would have type 'cinder.volume' and any 
property with a custom constraint would check that type to see if it 
matches (and fall back to accepting any text type if the placeholder 
doesn't have a type associated). get_param would get its type from the 
parameter schema (including any custom constraints). For get_attr we 
could make it part of the attribute schema.

The hard part obviously would be getting this to work with deeply-nested 
trees of data and across nested stacks. We could probably get the easy 
parts going and incrementally improve from there though. Worst case we 
just return None and get the same behaviour as now.

cheers,
Zane.