Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

create_before_destroy across modules #22937

Merged
merged 7 commits into from
Oct 24, 2019
Merged

create_before_destroy across modules #22937

merged 7 commits into from
Oct 24, 2019

Commits on Oct 24, 2019

  1. apply edge transforamtions after references 

    We can't correctly resolve the destroy ordering if all references
haven't been assigned to each node.
    @jbardin
    jbardin committed Oct 24, 2019
    Configuration menu
    Copy the full SHA
    7c703b1 View commit details
    Browse the repository at this point in the history

  2. Use Descendants rather than UpEdges in CBD 

    When looking for dependencies to fix when handling
create_before_destroy, we need to look past more than one edge, as
dependencies may appear transitively through outputs and variables. Use
Descendants rather than UpEdges.

We have the full graph to use for the CBD transformation, so there's no
longer any need to create a temporary graph, which may differ from the
original.
    @jbardin
    jbardin committed Oct 24, 2019
    Configuration menu
    Copy the full SHA
    8f35984 View commit details
    Browse the repository at this point in the history

  3. fix CBD tests to work on real data 

    The CBDEdgeTransformer tests worked on fake data structures, with a
synthetic graph, and configs that didn't match. Update them to generate
a more complete graph, with real node implementations, from real
configs.

The output graph is filtered down to instances, and the results still
functionally match the original expected test results, with some minor
additions due to using the real implementation.
    @jbardin
    jbardin committed Oct 24, 2019
    Configuration menu
    Copy the full SHA
    470362b View commit details
    Browse the repository at this point in the history

  4. failing test for module instance replace cycle 

    Destroy-time references are not correctly or fully inverted when
crossing module boundaries, causing cycle during apply.
    @jbardin
    jbardin committed Oct 24, 2019
    Configuration menu
    Copy the full SHA
    3eb0e74 View commit details
    Browse the repository at this point in the history

  5. do not connect destroy and resource nodes 

    Destroy nodes do not need to be connected to the resource (prepare
state) node when adding them to the graph. Destroy nodes already have a
complete state in the graph (which is being destroyed), any references
will be added in the ReferenceTransformer, and the proper
connection to the create node will be added in the
DestroyEdgeTransformer.

Under normal circumstances this makes no difference, as create and
destroy nodes always have an dependency, so having the prepare state
handled before both only linearizes the operation slightly in the
normal destroy-then-create scenario.

However if there is a dependency on a resource being replaced in another
module, there will be a dependency between the destroy nodes in each
module (to complete the destroy ordering), while the resource node will
depend on the variable->output->resource chain. If both the destroy and
create nodes depend on the resource node, there will be a cycle.
    @jbardin
    jbardin committed Oct 24, 2019
    Configuration menu
    Copy the full SHA
    eb6e7bb View commit details
    Browse the repository at this point in the history

  6. failing test with destroy cycle 

    after the previous commit the cycle is broken, but we can't evaluate
resource counts that depends on data sources being destroyed.
    @jbardin
    jbardin committed Oct 24, 2019
    Configuration menu
    Copy the full SHA
    7108560 View commit details
    Browse the repository at this point in the history

  7. prune unused resources from apply 

    If a resource is only destroying instances, there is no reason to
prepare the state and we can remove the Resource (prepare state) nodes.
They normally have pose no issue, but if the instances are being
destroyed along with their dependencies, the resource node may fail to
evaluate due to the missing dependencies (since destroy happens in the
reverse order).

These failures were previously blocked by there being a cycle when the
destroy nodes were directly attached to the resource nodes.
    @jbardin
    jbardin committed Oct 24, 2019
    Configuration menu
    Copy the full SHA
    f766bb8 View commit details
    Browse the repository at this point in the history