StackResource updates can destroy more than is needed

+1 for the concept

My best guess is that this would be feasible... we'd need to move where we check that the type is the same from before it is resolved with the environment to after.

The scenario where I see this useful is the one where we try to decouple two resources previously described both by the same type.

In that case, a stack update would two resources A and B, previously defined by the same resource type, now defined by different resource types yet both pointed to the same yaml.

Worked example:

Create a stack which looks like:

resource_registry:
  A: a.yaml

resources:
  the_a:
    type: A

  another_a:
    type: A

Then update to look like this:

resource_registry:
  A: a.yaml
  B: a.yaml

resources:
  the_a:
    type: B

  another_a:
    type: A

Here "the_a" will be completely replaced, even though the update is really a no-op, and could be handled non-destructively.

It would be better if we could simply do the stack-update on the underlying stack, and flip the type to the new alias.

So looking at the code, it seems one option would be to make use of the existing abandon/adopt code-paths for this, e.g internally (in update.py I guess), we'd do something like:

1. Get abandon data for TemplateResource the_a, type==A
2. Set deletion policy to RETAIN for the_a and delete it
3. Pass the data from (1) as adopt_data into the create for the new the_a resource, type==B
4. Update the new the_a with the new resource definition.

I think that gets us what we want, e.g the nested stack survives the update, but gets updated such that the end-state is the same as now.

Not tried this in practice yet, and obviously it'd likely introduce a dependency on enabling abandon/adopt in heat.conf, and the reason we disabled it was $bugs. I'm not sure those bugs will necessarily block this use-case tho.

I probably don't have time to work on this in the next few days - I started a functional test and poked at the code leading to the above analysis - I'll push the WIP test then unassign myself in case anyone else has bandwidth to pick this up.

Related fix proposed to branch: master
Review: https://review.openstack.org/238194

Dug a bit more into this, turns out ref bug #1509912 that abandon/adopt is destructive wrt the nested stacks, with no way to opt out of that behavior.

Maybe just maintaining the resources will be enough, but atm it's not looking like adopt is an easy way to maintain the entire nested stack over the update - so mangling the type/resource_definition in update.py may be required if that's what we want.

[Pasting comments from the review here for reference]

On further reflection, this shouldn't actually be a special case at all. The bug applies to all kinds of changes. For example:

- You have e.g. an OS::Nova::Server resource. You want to rename the type of My::Custom::Type and map that to OS::Nova::Server in the environment. That should be allowed.
- You have an OS::Heat::SoftwareDeployments resource and you want to rename the type to OS::Heat::SoftwareDeploymentGroup (which is an alias for the same type). That should be allowed too - in fact, *needs* to be allowed if we ever want to remove the deprecated one.

I think the general rule "if the type string doesn't match, replace it" is wrong. It should be "if the type string maps to a different class, replace it". That would solve the TemplateResource case, and also do the right thing for every other case too.

[I'm going to pretend I didn't just read that abandon/adopt stuff O.o]

> I'm going to pretend I didn't just read that abandon/adopt stuff

Ha! To be fair, at the time it seemed mildly preferable to figuring out the convoluted StackUpdate code, e.g

 - _process_new_resource_update doesn't only process new resources, it can either update in place, or raise UpdateReplace which results in creating a new resource, with a subsequent unfathomable dance exchanging stacks and resources.

 - _process_existing_resource_update - appears to only handle the case where a resource needs to be removed?!

Anyway, my point is whichever way you go, figuring this out is hard so contemplating per-resource abandon wasn't *that* insane of an idea (well, maybe it was, given that it doesn't work, but anyway..)

The update code starts making a lot more sense when you think of it in terms of where the nodes in the update graph come from (see https://github.com/zaneb/presentations/blob/heat-workflow/update.svg). So _process_new_resource_update() processes a node contributed by the new template's graph, and first attempts to co-opt an existing resource (by updating it in place) or, failing that, falls back to creating the new one. Similarly, _process_existing_resource_update() processes a node contributed by the existing template's (reversed) graph, so it does nothing unless there is a replaced resource that needs to be cleaned up. I'd be all for changing the names if you can think of better ones.

In any event, as you've discovered. all of the logic we want already exists and the fix to this problem is a simple (ha!) matter of figuring out which values to compare in a single line of code.

Reviewed: https://review.openstack.org/238194
Committed: https://git.openstack.org/cgit/openstack/heat/commit/?id=98044dbd17c703e6062752f043e1cac6563e3589
Submitter: Jenkins
Branch: master

commit 98044dbd17c703e6062752f043e1cac6563e3589
Author: Steven Hardy <email address hidden>
Date: Wed Oct 21 18:56:01 2015 +0100

    Allow in-place updates for all compatible types

    We previously only allowed a resource to update in-place when the 'type'
    field in the old and new templates matched. In an age of environment type
    mappings, this is incorrect. Instead, allow a resource to be updated
    in-place when the old and new resource plugins are of the same class, since
    this is actually what determines whether handle_update is capable of
    correctly updating the resource in-place.

    There is a special-case for TemplateResource as we currently subclass it
    for each template type, but all TemplateResource subclasses are in fact
    capable of converting between each other via a stack update.

    Change-Id: Iba4abf5efd9737ca6a17d8c91d5c54ab6d3f12e0
    Co-Authored-By: Zane Bitter <email address hidden>
    Closes-Bug: #1508115

Fix proposed to branch: stable/liberty
Review: https://review.openstack.org/246612

Fix proposed to branch: stable/kilo
Review: https://review.openstack.org/246614

Reviewed: https://review.openstack.org/246612
Committed: https://git.openstack.org/cgit/openstack/heat/commit/?id=531e6ce3082153dc84145798efc07a70ca957b18
Submitter: Jenkins
Branch: stable/liberty

commit 531e6ce3082153dc84145798efc07a70ca957b18
Author: Steven Hardy <email address hidden>
Date: Wed Oct 21 18:56:01 2015 +0100

    Allow in-place updates for all compatible types

    We previously only allowed a resource to update in-place when the 'type'
    field in the old and new templates matched. In an age of environment type
    mappings, this is incorrect. Instead, allow a resource to be updated
    in-place when the old and new resource plugins are of the same class, since
    this is actually what determines whether handle_update is capable of
    correctly updating the resource in-place.

    There is a special-case for TemplateResource as we currently subclass it
    for each template type, but all TemplateResource subclasses are in fact
    capable of converting between each other via a stack update.

    Change-Id: Iba4abf5efd9737ca6a17d8c91d5c54ab6d3f12e0
    Co-Authored-By: Zane Bitter <email address hidden>
    Closes-Bug: #1508115
    (cherry picked from commit 98044dbd17c703e6062752f043e1cac6563e3589)

Reviewed: https://review.openstack.org/246614
Committed: https://git.openstack.org/cgit/openstack/heat/commit/?id=67e9b5bf1131019ed0746fde7e56c60462327c53
Submitter: Jenkins
Branch: stable/kilo

commit 67e9b5bf1131019ed0746fde7e56c60462327c53
Author: Steven Hardy <email address hidden>
Date: Wed Oct 21 18:56:01 2015 +0100

    Allow in-place updates for all compatible types

    We previously only allowed a resource to update in-place when the 'type'
    field in the old and new templates matched. In an age of environment type
    mappings, this is incorrect. Instead, allow a resource to be updated
    in-place when the old and new resource plugins are of the same class, since
    this is actually what determines whether handle_update is capable of
    correctly updating the resource in-place.

    There is a special-case for TemplateResource as we currently subclass it
    for each template type, but all TemplateResource subclasses are in fact
    capable of converting between each other via a stack update.

    Change-Id: Iba4abf5efd9737ca6a17d8c91d5c54ab6d3f12e0
    Co-Authored-By: Zane Bitter <email address hidden>
    Closes-Bug: #1508115
    (cherry picked from commit 98044dbd17c703e6062752f043e1cac6563e3589)

Related fix proposed to branch: master
Review: https://review.openstack.org/250053

Related fix proposed to branch: master
Review: https://review.openstack.org/250054

This issue was fixed in the openstack/heat 6.0.0.0b1 development milestone.

Reviewed: https://review.openstack.org/250053
Committed: https://git.openstack.org/cgit/openstack/heat/commit/?id=06a713c4456203cd561f16721dc8ac3bcbb37a39
Submitter: Jenkins
Branch: master

commit 06a713c4456203cd561f16721dc8ac3bcbb37a39
Author: Zane Bitter <email address hidden>
Date: Tue Nov 24 12:29:38 2015 -0500

    Add a separate get_class_to_instantiate() method to Environment

    We were previously using get_class for two different purposes - to get a
    resource plugin on which we could perform introspection to obtain the
    properties and attributes schema, and to get a resource plugin we could
    instantiate to create a Resource object. These are both the same except in
    the case of a TemplateResource, where having two different use cases for
    the same piece of code was adding considerable extra complexity. Combining
    the use cases in this way also made the error handling confusing (leading
    to bug 1518458).

    This change separates out the two cases.

    Change-Id: I3bde081cd4537810c8c6a0948ab447c3fb7ca9bc
    Related-Bug: #1447194
    Related-Bug: #1518458
    Related-Bug: #1508115

Reviewed: https://review.openstack.org/250054
Committed: https://git.openstack.org/cgit/openstack/heat/commit/?id=9a3c3c0577c12aa02c6571ae908e7ea64d5008b4
Submitter: Jenkins
Branch: master

commit 9a3c3c0577c12aa02c6571ae908e7ea64d5008b4
Author: Zane Bitter <email address hidden>
Date: Tue Nov 24 12:29:38 2015 -0500

    Get rid of the Resource.resource_class() method

    Now that we have get_class_to_instantiate() and it always returns
    TemplateResource (instead of a custom subclass) for template resources, we
    can just compare the types of resources directly.

    Change-Id: I96275c3ead3ea3565e55b99a8c5a9948eaa15b78
    Related-Bug: #1508115
    Related-Bug: #1447194
    Related-Bug: #1518458

Related fix proposed to branch: stable/liberty
Review: https://review.openstack.org/269691

Related fix proposed to branch: stable/kilo
Review: https://review.openstack.org/269692

Reviewed: https://review.openstack.org/269691
Committed: https://git.openstack.org/cgit/openstack/heat/commit/?id=87116e262329e5cd309a0f1e35ffefd06080022c
Submitter: Jenkins
Branch: stable/liberty

commit 87116e262329e5cd309a0f1e35ffefd06080022c
Author: Zane Bitter <email address hidden>
Date: Tue Nov 24 12:29:38 2015 -0500

    Load template files only from their known source

    Modify get_class to ensure that user-defined resources cannot result in
    reads from the local filesystem. Only resources defined by the operator
    in the global environment should read local files.

    To make this work, this patch also adds a separate
    get_class_to_instantiate() method to the Environment.

    We were previously using get_class for two different purposes - to get a
    resource plugin on which we could perform introspection to obtain the
    properties and attributes schema, and to get a resource plugin we could
    instantiate to create a Resource object. These are both the same except in
    the case of a TemplateResource, where having two different use cases for
    the same piece of code was adding considerable extra complexity. Combining
    the use cases in this way also made the error handling confusing (leading
    to bug 1518458).

    This change separates out the two cases.

    Change-Id: I845e7d23c73242a4a4c9c40599690ab705c75caa
    Closes-Bug: #1496277
    Related-Bug: #1447194
    Related-Bug: #1518458
    Related-Bug: #1508115
    (cherry picked from commit 06a713c4456203cd561f16721dc8ac3bcbb37a3
                           and 26e6d5f6d776c1027c4f27058767952a58d15e25)

Reviewed: https://review.openstack.org/269692
Committed: https://git.openstack.org/cgit/openstack/heat/commit/?id=fa19a617a79fd1cb0d892bb8ea87c4b9f6398c34
Submitter: Jenkins
Branch: stable/kilo

commit fa19a617a79fd1cb0d892bb8ea87c4b9f6398c34
Author: Zane Bitter <email address hidden>
Date: Tue Nov 24 12:29:38 2015 -0500

    Load template files only from their known source

    Modify get_class to ensure that user-defined resources cannot result in
    reads from the local filesystem. Only resources defined by the operator
    in the global environment should read local files.

    To make this work, this patch also adds a separate
    get_class_to_instantiate() method to the Environment.

    We were previously using get_class for two different purposes - to get a
    resource plugin on which we could perform introspection to obtain the
    properties and attributes schema, and to get a resource plugin we could
    instantiate to create a Resource object. These are both the same except in
    the case of a TemplateResource, where having two different use cases for
    the same piece of code was adding considerable extra complexity. Combining
    the use cases in this way also made the error handling confusing (leading
    to bug 1518458).

    This change separates out the two cases.

    Change-Id: I845e7d23c73242a4a4c9c40599690ab705c75caa
    Closes-Bug: #1496277
    Related-Bug: #1447194
    Related-Bug: #1518458
    Related-Bug: #1508115
    (cherry picked from commit 06a713c4456203cd561f16721dc8ac3bcbb37a3
                           and 26e6d5f6d776c1027c4f27058767952a58d15e25)

This issue was fixed in the openstack/heat 5.0.1 release.