Move resource refreshing into plan #26270
Conversation
There was a problem hiding this comment.
This looks really neat, it's a clever change! Tip for others: this was much easier to review commit-by-commit (thanks in part to good commit messages). I'll hold off approving till you finish fighting with tests 😝
🤔 we may want to chat with the folks who work on sentinel and see if they'd want/expect access to the refreshed state (that's now stored in the plan) as part of the json output, instead of only the current state (tbh I'm fuzzy; would the state in the plan output today include information that was gathered during a refresh?). That's not relevant to this PR, just a thoughtl.
| @@ -107,8 +107,8 @@ func (n *NodePlannableResourceInstance) evalTreeManagedResource(addr addrs.AbsRe | |||
| var provider providers.Interface | |||
| var providerSchema *ProviderSchema | |||
| var change *plans.ResourceInstanceChange | |||
| var refreshState *states.ResourceInstanceObject | |||
| var planState *states.ResourceInstanceObject | |||
| var instanceRefreshState *states.ResourceInstanceObject | |||
There was a problem hiding this comment.
I quite appreciate this clearer variable, as you know I've been knee-deep in this code (and lost and confused) recently.
Codecov Report
|
Yes, the plan is generated from the refreshed state currently, it just happens in a single step with these changes. My wording above was not very good, and there shouldn't be any externally visible changes to the plan output from the previous release. |
This change refreshes the instance state during plan, so a complete Refresh no longer needs to happen before Plan.
Since plan uses the state as a scratch space for evaluation, we need an entirely separate state to store the refreshed resources values during planning. Add a RefreshState method to the EvalContext to retrieve a state used only for refreshing resources.
All resources use EvalWriteState to store their state, so we need a way to switch the states when the resource is refreshing vs when it is planning. (this will likely change once we factor out the EvalNode pattern)
Since the refreshed state is now an artifact of the plan process, it makes sense to add it to the Plan type, rather than adding an additional return value to the Context.Plan method.
This breaks a bunch of tests, and we need to figure out why before moving on.
We need to do this for both states during plan
We need to build a new context go get at the modified state
The prior state recorded in the plans did not match the actual prior state. Make the plans and state match depending on whether there was existing state or not.
After apply, any refreshed state from a plan would be invalid. Normal usage doesn't ever see this, bu internal tests may re-use the context.
Update the old acceptance test framework just enough to make the tests pass.
Leaving plan with -refresh=false tests failing for now.
We still need to determine if `-refresh=false` is even useful with the new planning strategy.
This was never picked up by the tests until now
jbardin
force-pushed
the
jbardin/refresh-plan
branch
from
3dc2c5f to
86dd893
Compare
|
I made the use of the |
|
Out of interest, does it mean, that providers now can be configured with datasource, which itself references another resource? Previously it required targeted apply to create dependencies of datasource first. |
|
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
locked as resolved and limited conversation to collaborators
This removes the separate
Refreshoperation that happens during planning, having each resource instance refresh "just in time" during the plan walk. The refresh walk has numerous problems, mostly stemming from the graph being generated by a mix of the state for managed resources, the configuration for temporary values, and a combination of configuration and state for data sources. This can lead to incongruities during refresh that cannot be resolved, often requiring the use ofplan -refresh=falsein order to apply some configurations. We also skip the problem of data sources reading "stale" state that will be updated during apply, by having data sources evaluated during plan where we have all the change information available.Merging of refresh into planning is primarily accomplished by moving the bulk of the refresh node eval tree from
NodeRefreshableManagedResourceInstanceinto the managed eval tree forNodePlannableResourceInstance. This alone works for the majority of test cases, and proves out the concept for merging the refresh and plan walk.The more difficult part of merging the two operations is that in order to have all the prior state information available to render a plan, the plan walk must simultaneously operate on 2 separate states. To do that we add a second "refresh state" to the global terraform
Context, along with aRefreshState()getter method. In order to save instances to this state, there is now aphaseStateflag forEvalWriteStateto switch which state to target during different phases of each instance's planning. This is admittedly clunky, but we are concurrently refactoring the EvalNode pattern and can smooth that out in tandem with upcoming changes.Another change around handling the refreshed state, is that because planning is responsible for generating the most current state, the
plans.Planstructure now includes astates.State. Rather than needing to combine the refreshed state with the plan out of band to create the plan, the refreshed state is now managed directly byContext.Plan().The ability to plan data sources introduced in 0.13 allows them to generally work correctly without the added refresh step. We can leave cleaning up data source planning for another PR to keep this one smaller.
The current
Refreshoperation will be left as-is for now, and will no longer be used in the normal operation workflow. Future plans are currently to remove the internals, and refactor the CLI command in terms of a plan and apply with no configuration changes.