-
Notifications
You must be signed in to change notification settings - Fork 9.5k
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
core: New refresh graph building behaviour #14098
Conversation
Got some higher-level tests added to the test provider, want to work on some more lower-level node tests though, just groking the code on how to do that and got a pretty good grip, will add them when I have a chance! |
Now done, tests added, WIP removed and ready. For the |
Hi @vancluever! Sorry for the silence here. I just wanted to let you know that this is on my list to take a look at, but I want to make sure I have time to sit down and understand this properly to give a good review. I'll get back to you on this as soon as I can. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all makes sense to me. Great work, @vancluever!
I left one minor question inline... I did lots of testing and found it behaved as I expected in all cases, so I'm sure that's just a case of an outdated comment after some refactoring, but I figured I'd check with you first because this sort of change tends to have non-obvious edge cases.
|
||
// The concrete resource factory we'll use | ||
concreteResource := func(a *NodeAbstractResource) dag.Vertex { | ||
// Add the config and state since we don't do that via transforms |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment suggests that the state should be attached here too... is this just an outdated comment with state now handled by AttachStateTransformer
below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops! Yeah probably should have caught that - confirmed that it's outdated - state is not being added here. I sourced this bit from existing code too so it might be worthwhile to look for other outdated comments.
Hey @apparentlymart, thanks for the review and positive feedback! I'm on vacation right now, so it's hard for me to double check what I mentioned about outdated comments being in pre-existing code, but again, pretty sure that's the case (will check tonight if I have time). For sure state isn't being attached there though. Thanks again! :) |
Hi @vancluever! Don't disturb your vacation for this... I just wanted to clarify that I was understanding things correctly. I'll do one more pass myself and look out for suspicious comments and just fix them myself. This seems ready to merge but I'm going to leave that until tomorrow rather than merging this just before I quit for the day. Thanks again for working on this! |
@vancluever @apparentlymart - I'm a new terraform user (2weeks) and have written a custom provider for an internal api. Tonight I was testing scale in/out with resources and data sources that reference these resources (using counts). I ran into many race conditions with "index out of range" even when using the new Just wanted to drop my thanks for the effort going into this one and the recent additions to 0.9.5 to pave the way such as #14135. |
In prep for NodeRefreshableResource becoming an NodeAbstractCountResource and implementing GraphNodeDynamicExpandable.
Currently, the refresh graph uses the resources from state as a base, with data sources then layered on. Config is not consulted for resources and hence new resources that are added with count (or any new resource from config, for that matter) do not get added to the graph during refresh. This is leading to issues with scale in and scale out when the same value for count is used in both resources, and data sources that may depend on that resource (and possibly vice versa). While the resources exist in config and can be used, the fact that ConfigTransformer for resources is missing means that they don't get added into the graph, leading to "index out of range" errors and what not. Further to that, if we add these new resources to the graph for scale out, considerations need to be taken for scale in as well, which are not being caught 100% by the current implementation of NodeRefreshableDataResource. Scale-in resources should be treated as orphans, which according to the instance-form NodeRefreshableResource node, should be NodeDestroyableDataResource nodes, but this this logic is currently not rolled into NodeRefreshableDataResource. This causes issues on scale-in in the form of race-ish "index out of range" errors again. This commit updates the refresh graph so that StateTransformer is no longer used as the base of the graph. Instead, we add resources from the state and config in a hybrid fashion: * First off, resource nodes are added from config, but only if resources currently exist in state. NodeRefreshableManagedResource is a new expandable resource node that will expand count and add orphans from state. Any count-expanded node that has config but no state is also transformed into a plannable resource, via a new ResourceRefreshPlannableTransformer. * The NodeRefreshableDataResource node type will now add count orphans as NodeDestroyableDataResource nodes. This achieves the same effect as if the data sources were added by StateTransformer, but ensures there are no races in the dependency chain, with the added benefit of directing these nodes straight to the proper NodeDestroyableDataResource node. * Finally, config orphans (nodes that don't exist in config anymore period) are then added, to complete the graph. This should ensure as much as possible that there is a refresh graph that best represents both the current state and config with updated variables and counts.
These tests cover the new refresh behaviour and would fail with "index out of range" if the refresh graph is not expanded to take new resources into account as well (scale out), or if it does not with expanded count orphans in a way that makes sure they don't get interpolated when walked (scale in).
Tests on DynamicExpand for both resources and data sources, cover scale in/out scenarios, and also a verification for the behaviour of config orphans.
@apparentlymart glad everything worked out and thanks for the merge! |
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
WIP - tests pending on this - but can reference #13828 for a repro
Currently, the refresh graph uses the resources from state as a base,
with data sources then layered on. Config is not consulted for resources
and hence new resources that are added with
count
(or any new resourcefrom config, for that matter) do not get added to the graph during
refresh.
This is leading to issues with scale in and scale out when the same
value for
count
is used in both resources, and data sources that maydepend on that resource (and possibly vice versa). While the resources
exist in config and can be used, the fact that
ConfigTransformer
forresources is missing means that they don't get added into the graph,
leading to "index out of range" errors and what not.
Further to that, if we add these new resources to the graph for scale
out, considerations need to be taken for scale in as well, which are not
being caught 100% by the current implementation of
NodeRefreshableDataResource
. Scale-in resources should be treated asorphans, which according to the instance-form
NodeRefreshableResource
node, should be
NodeDestroyableDataResource
nodes, but this this logicis currently not rolled into
NodeRefreshableDataResource
. This causesissues on scale-in in the form of race-ish "index out of range" errors
again.
This commit updates the refresh graph so that
StateTransformer
is nolonger used as the base of the graph. Instead, we add resources from the
state and config in a hybrid fashion:
resources currently exist in state.
NodeRefreshableManagedResource
is a new expandable resource node that will expand
count
and addorphans from state. Any
count
-expanded node that has config but nostate is also transformed into a plannable resource, via a new
ResourceRefreshPlannableTransformer
.NodeRefreshableDataResource
node type will now add count orphansas
NodeDestroyableDataResource
nodes. This achieves the same effectas if the data sources were added by
StateTransformer
, but ensuresthere are no races in the dependency chain, with the added benefit of
directing these nodes straight to the proper
NodeDestroyableDataResource
node.period) are then added, to complete the graph.
This should ensure as much as possible that there is a refresh graph
that best represents both the current state and config with updated
variables and counts.