deferred actions: load all values from deferred resources when building eval context #35526
Conversation
| @@ -73,7 +73,14 @@ func (n *nodeExpandPlannableResource) expandsInstances() { | |||
|
|
|||
| // GraphNodeAttachDependencies | |||
| func (n *nodeExpandPlannableResource) AttachDependencies(deps []addrs.ConfigResource) { | |||
| n.dependencies = deps | |||
| n.dependencies = make([]addrs.ConfigResource, 0) | |||
There was a problem hiding this comment.
While normally not an optimization that would matter in core, I would initialize this slice with the capacity of len(deps). This dependency handling is already a hot-spot in the code because large configs can add a lot of entries on a lot of nodes, so preventing the extra allocations might help at least a little.
There was a problem hiding this comment.
I ended up pushing the check back into the AttachDependencies via checking for if the source is a GraphNodeConfigResource directly. This avoids the whole problem, so seemed like the best thing to do.
| AttachDependencies([]addrs.ConfigResource) | ||
| Name() string |
There was a problem hiding this comment.
If this is only for logging, I would just use dag.VertexName instead. It does the same thing, but we don't actually enforce Name() being defined and I don't want something to slip past this interface someday because it's missing that method.
| // First, we're going to load any instances that we have written into the | ||
| // deferrals system. A deferred resource overrides anything that might be | ||
| // in the state for the resource, so we do this first. | ||
| for key, value := range d.Evaluator.Deferrals.GetDeferredResourceInstances(addr.Absolute(d.ModulePath)) { |
There was a problem hiding this comment.
I wonder if it makes sense to eventually reconcile the difference between planned and deferred instances. Planned instances historically get an empty record in state so iterating on rs.Instances works for them, but deferred instances (which are still sort of like a planned instance) do not. Not suggesting any changes here, just something to think about when we encounter this inconsistency again ;)
| if deferred.DependenciesDeferred(n.Dependencies) { | ||
| // If the output is from deferred resources then we return a | ||
| // simple null value representing that the value is really | ||
| // unknown as the dependencies were not properly computed. |
There was a problem hiding this comment.
Ah -- nice. I think this is more coherent.
|
Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch. |
|
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
This PR fixes a bug in deferred actions when they are referenced but haven't been added to state. Reasons why a deferred action might not be in state are:
Essentially, the evaluate functionality for resources looks at the state to work out the base set of values. These values were then overridden by values from the deferrals or from the plan when appropriate. But, this means that if a deferred resource wasn't already in state (eg. it's just being created) then no values would be added to the context for it. Now, we precompute the set of available instances by looking at the deferrals and state in isolation. We add all the values that are being deferred first, and then add values from the state/plan only if they haven't already been added. This ensures that even if there is nothing in the state for a deferred resource then we'll still capture values for it in the context.
This means that deferred actions are actually propagating their data properly for the first time, and this exposed a missing feature in outputs that reference deferred actions. These are supposed to be returned as null, but this was only happening because we weren't properly referencing deferred resources and the outputs were naturally returning unknown as a result. Now, the output node consults the deferred actions to check if any of the nodes it references were deferred, and replaces the output value with null properly.
To facilitate this, I changed the interface for
GraphNodeAttachDependenciesso that it can also be implemented by the output nodes. The functions for the plan and apply nodes must now manually remove themselves, but the same functionality can be shared for working out if an output node or a plan/apply node should be deferred because of dependencies.