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.

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

store absolute addresses for resource dependencies in the state #23252

Merged
merged 8 commits into from Nov 8, 2019

Conversation

@jbardin
Copy link
Member

jbardin commented Nov 1, 2019

We need to be able to reference all possible dependencies for ordering
when the configuration is no longer present, which means that absolute
addresses must be used. Since this is only to recreate the proper
ordering for instance destruction, only resources addresses need to be
listed rather than individual instance addresses.

The state version remains the same with this change, since it's only
adding a field, provides an easier upgrade path for users, and
provides the ability for refresh/apply to handle the "upgrade" by
reading the old depends_on field and writing out the new
dependencies .

The inter-instance dependencies will be determined from the complete
reference graph, so that absolute addresses can be stored, rather than
just references within a module. The Dependencies are added to the node
in the same manner as state, i.e. via an "attacher" interface and
transformer. This is because dependencies must be calculated from the
graph itself, and not from the config.

The new StateDependencies are used in the DestroyEdgeTransformer
to connect both dependent destroyers, as well as creators depending on
another instance's destroyer.

The old code in DestroyEdgeTransformer may no longer be needed in the
long run, but that can be determined separately, since too many of the
tests start with an incomplete state and rely on the Dependencies being
determined from the configuration alone.

Fixes #18408
Fixes #20196
Fixes #21497
Fixes #21630
Fixes #23169
Fixes #22928
Closes #8617 (fixes the remaining issue described in the comments)
Might take care of #20823 (The linked issue sounds like a 0.11 bug. If it's still broken in 0.12 it would probably fall into this same category)

We need to be able to reference all possible dependencies for ordering
when the configuration is no longer present, which means that absolute
addresses must be used. Since this is only to recreate the proper
ordering for instance destruction, only resources addresses need to be
listed rather than individual instance addresses.
@jbardin jbardin requested a review from hashicorp/terraform-core Nov 1, 2019
@hashibot hashibot bot added the sdkv1 label Nov 1, 2019
@jbardin jbardin changed the title Jbardin/abs state dependencies store absolute addresses for resource dependencies in the state Nov 1, 2019
Copy link
Member

apparentlymart left a comment

Hmm sorry I just found this pending review that I apparently didn't submit when I originally wrote it 😖

@@ -202,6 +205,20 @@ func prepareStateV4(sV4 *stateV4) (*File, tfdiags.Diagnostics) {
}
deps = append(deps, ref.Subject)
}
obj.DependsOn = deps

This comment has been minimized.

Copy link
@apparentlymart

apparentlymart Nov 7, 2019

Member

It looks like in principle we have enough information here to convert from an addrs.Resource or addrs.ResourceInstance to the absolute equivalent by calling addr.Absolute(moduleAddr). Would it be reasonable to normalize this difference away in the loader so that the rest of Terraform can deal only in Dependencies, or is there information captured in depends_on that can't be mapped directly to an addrs.AbsResource? (Perhaps depends_on sometimes contains non-resource references that we need to preserve for correct behavior until it gets upgraded?)

This comment has been minimized.

Copy link
@jbardin

jbardin Nov 7, 2019

Author Member

Yeah, depends_on contains references to modules and values as well. We need to load those into the graph to find the absolute resources they will eventually connect.

This comment has been minimized.

Copy link
@apparentlymart

apparentlymart Nov 7, 2019

Member

That makes sense... thanks.

As a compromise, can we document the old field as being deprecated and read-only and make sure that any time we update a state object we'll force the old one to nil and migrate everything into the new one? At least then that puts us on a path to removing the deprecated field, because (if I'm understanding correctly) everything would be migrated over into the new field by the next refresh walk.

(I suspect this might already be true, because IIRC the apply step constructs a new state object from scratch each time rather than modifying the old one in-place, but I wanted to mention it explicitly just because I couldn't see any tests here that prove it to be true by.)

// found dependencies to those saved in the state. The existing dependencies
// are retained, as they may be missing from the config, and will be required
// for the updates and destroys during the next apply.
type EvalRefreshDependencies struct {

This comment has been minimized.

Copy link
@apparentlymart

apparentlymart Nov 7, 2019

Member

I feel a little unsure about this one, though perhaps I'm misunderstanding it. Is this updating the state during refresh to include new dependencies in the configuration? If so, it seems like this would be the first time we've made refresh take something from the configuration and represent it in the state, as opposed to taking information from upstream systems (via providers) and representing it in the state. Perhaps that's okay, but I'm a little nervous about creating that precedent if we can avoid it, to avoid confusing the user model of what terraform refresh does. 🤔

This comment has been minimized.

Copy link
@jbardin

jbardin Nov 7, 2019

Author Member

After trying out this idea without refresh writing out dependencies, I think it's probably worth the slightly odd idea of the refreshed state being altered by the config, even if it's not really the instance state itself. Since refresh is the only moment we have to re-write resource state (apply does not write state if there's no changes), if we don't do this existing resources would never have the depends_on field fixed up.

@jbardin jbardin force-pushed the jbardin/abs-state-dependencies branch 2 times, most recently from 292ac66 to af12cfe Nov 7, 2019
jbardin added 7 commits Oct 15, 2019
The test fixture did not like having modules when using the generic json
map, so read and compare the states in the final *File datastructure.
Make use of the new Dependencies field in the instance state.

The inter-instance dependencies will be determined from the complete
reference graph, so that absolute addresses can be stored, rather than
just references within a module. The Dependencies are added to the node
in the same manner as state, i.e. via an "attacher" interface and
transformer.  This is because dependencies are calculated from the graph
itself, and not from the config.
Refresh should load any new dependencies found because of configuration
or state changes, but retain any dependencies already in the state.
Orphaned resources would not be in config, but we do not want to lose
the destroy ordering for the later apply.
Updates resulting from orphaned instances should happen after the
deletion of the instances.
The DestroyEdgeTransformer cannot determine ordering from the graph when
the destroyers are from orphaned resources, because there are no
references to resolve. The new stored Dependencies provides what we need
to connect the instances in this case.

We also add the StateDependencies method directly in the
GraphNodeResourceInstance interface, since all instances already
implement this, and we don't need another optional interface to check.

The old code in DestroyEdgeTransformer may no longer be needed in the
long run, but that can be determined separately, since too many of the
tests start with an incomplete state and rely on the Dependencies being
determined from the configuration alone.
@jbardin jbardin force-pushed the jbardin/abs-state-dependencies branch from af12cfe to 46dbb3d Nov 7, 2019
@jbardin jbardin merged commit bee7033 into master Nov 8, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
@jbardin jbardin deleted the jbardin/abs-state-dependencies branch Nov 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.