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

core: New refresh graph building behaviour #14098

Merged
merged 5 commits into from
May 12, 2017
Merged

core: New refresh graph building behaviour #14098

merged 5 commits into from
May 12, 2017

Conversation

vancluever
Copy link
Contributor

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 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.

@vancluever
Copy link
Contributor Author

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!

@vancluever vancluever changed the title [WIP] core: New refresh graph building behaviour core: New refresh graph building behaviour May 3, 2017
@vancluever
Copy link
Contributor Author

Now done, tests added, WIP removed and ready.

For the TestRefreshGraphBuilder_configOrphans test, I was surprised to not see a "explicit" root there - but I think that's because the builder was able to calculate the root as being the AWS provider closer due to the fact that it's a very basic config, versus a more complex one with variables, etc. But a double check would be nice :)

@grubernaut grubernaut requested a review from mitchellh May 3, 2017 18:35
@apparentlymart apparentlymart self-requested a review May 4, 2017 17:27
@apparentlymart
Copy link
Contributor

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.

apparentlymart
apparentlymart approved these changes May 9, 2017
View reviewed changes
Copy link
Contributor

@apparentlymart apparentlymart left a 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.

terraform/node_resource_refresh.go

// 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
Copy link
Contributor

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?

Copy link
Contributor Author

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.

@vancluever
Copy link
Contributor Author

vancluever commented May 11, 2017 •
edited
Loading

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! :)

@apparentlymart
Copy link
Contributor

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!

@dkiser
Copy link

dkiser commented May 12, 2017

@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 resource.bar.*.baz[count.index] syntax. I stumbled across this PR and decided to build/retest and viola...no more out of range errors on a state that has 200+ resources.

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.

@vancluever @apparentlymart
Rename NodeRefreshableResource to NodeRefreshableResourceInstance
dfb5be2 
In prep for NodeRefreshableResource becoming an
NodeAbstractCountResource and implementing GraphNodeDynamicExpandable.
@vancluever @apparentlymart
core: New refresh graph building behaviour
b807505 
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.
@vancluever @apparentlymart
core: Fix destroy factory in data source refresh expander
7b1618e
@vancluever @apparentlymart
providers/test: Add count resource <-> data source dep count scale tests
f63ad1d 
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).
@vancluever @apparentlymart
core: Test for new refresh graph behaviour
11b4794 
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 apparentlymart merged commit 87e98d6 into hashicorp:master May 12, 2017
@vancluever
Copy link
Contributor Author

@apparentlymart glad everything worked out and thanks for the merge!
And @dkiser you are very welcome! Really glad to see it's making people's lives easier!
👍

@apparentlymart apparentlymart mentioned this pull request May 15, 2017
Closed
@ghost
Copy link

ghost commented Apr 12, 2020

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.

@ghost ghost locked and limited conversation to collaborators Apr 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@apparentlymart apparentlymart apparentlymart approved these changes

@mitchellh mitchellh Awaiting requested review from mitchellh

Assignees
No one assigned
Labels
core enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@vancluever @apparentlymart @dkiser @radeksimko