-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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
Backport of Clean up handling of check-related graph nodes into v1.3 #32055
Backport of Clean up handling of check-related graph nodes into v1.3 #32055
Conversation
9155864
to
9b96761
Compare
9b96761
to
9155864
Compare
Module output may need to be evaluated during destroy in order to possibly be used by providers. The final state however is that all objects are destroyed, so preconditions should not be evaluated.
Not all root output instances were going through proper expansion when destroy operations were involved, leading to cases where they would be evaluated even though the expected result was only to remove them from the state. Normally destroy nodes stand alone in the graph, and do not produce references to other nodes. Because root output nodes were replaced by expansion nodes, these were being connected via normal references, even in the case where we were working with a destroy graph.
IsFullDestroy was a workaround during apply to detect when the change set was created by a destroy plan. This no longer works correctly, and we need to fall back to the UIMode set in the plan.
The removeRootOutputs field was not strictly used for that purpose, and was also copied to another DestroyPlan field.
Refreshing for a destroy should use the refresh-only plan to avoid planning new objects or evaluating conditions. This should also be skipped if there is no state, since there would be nothing to refresh.
NoOp changes should not participate in a destroy sequence, but because they are included as normal update nodes the usual connections were still being made.
6a9bea2
to
bcd9d80
Compare
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. |
Backport
This PR is auto-generated from #32051 to be assessed for backporting due to the inclusion of the label 1.3-backport.
The below text is copied from the body of the original PR.
This covers a number of various loosely related issues all stemming from the condition checks handling in outputs and NoOp resource nodes:
Optimally the apply graph operation should not need extra knowledge from how the plan was created, as normal, refresh, or destroy plan. The serialized plan however does not have the information necessary to make this a reality. With the addition of conditions on outputs, output nodes need to be aware of the current phase of operation to know whether to register, evaluate, or ignore the conditions in the config. Resource evaluation also requires knowing if a full destroy in effect to determine how to evaluate resources pending removal in a full destroy when a provider required them for configuration. The existing workaround of checking for only
Delete
actions no longer works, and we must again depend on thewalkOperation
. Add the plan'swalkOperation
via the plan to the graph builder, and make use of it as necessary in the various transformers. If we aim for a change set which retains the full fidelity of all objects used during the plan, then we can review the removal of thewalkOperation
based logic.Output conditions should not be checked during a full destroy. The conditions may no longer be satisfied or even valid due to the overall destroy. We can use the new
walkOperation
information to determine when checks can be skipped. One thing which is not clear here is whether condition references should be skipped during destroy as well. It seems that they should be fine as the graph must have been correct initially, but because they don't participate in the data flow, there is chance that they could interfere with a full destroy which involves intermediary providers.Replacing the root outputs with an expansion node was not complete, and singular nodes could have been inserted skipping the registrations of the condition checks. Outputs are the only type which accounts for most types of actions during all phases with a single node, so
nodeExpandOutput
must have all the same graph-building information to make the decision about what node types to return and how to evaluate checks.Checks are not needed during console eval, but rather than inserting another exception path for their evaluation, we just make sure that they are registered to prevent panics.
The
destroy
plan must refresh before creating the destroy graph, but was using a normal plan to do so. We can switch that to aRefreshOnly
plan to prevent the unnecessary evaluation and planning of resources.NoOp resource nodes should not be participating in destroy sequences. Now that they are always included in the graph, we need to check the change action to make sure not to introduce unneeded edges into the sequence. This is again especially important when destroy sequences have intermediary providers which are not at the graph root.
Fixes #31975
Fixes #31994
Fixes #32006
Fixes #32023