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.

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: update destroy graph pruning to handle ephemeral resources #35890

Merged
merged 4 commits into from
Oct 24, 2024

Conversation

jbardin
Copy link
Member

@jbardin jbardin commented Oct 22, 2024

Ephemeral resources were the straw that broke the pruneUnusedNodesTransformer's back. This transformer has always been in a tough spot, trying to maintain legacy behaviors around destroy, while keeping destroy operations from failing entirely from configuration evaluation errors. As we've tidied up graph creation over many prior versions, there were just a few node types which were incorrectly classified that were preventing us from inverting the pruneUnusedNodesTransformer logic to the more reasonable task of finding what to keep, rather than finding what to exclude.

With the missing destroy node types sorted out, the pruneUnusedNodesTransformer no longer needs to keep up with the whack-a-mole game of detecting nodes which might need to be skipped. When executing a destroy operation, we can never be certain that the state will contain all the data required to evaluate the entire configuration. This could be because of a previously failed apply operation, or external changes made out of band. This means that once we process the configuration to get the providers hooked up, we need to somehow remove any nodes that correspond to configuration values that may not be available to evaluate.

In a destroy operation we primarily need three categories of nodes to complete the plan and apply:

  • Nodes corresponding to values in the state
  • Providers required to destroy those values obtained from the state
  • Any node which a provider depends on, to ensure its configuration can be fully evaluated

Other than that minimal set of required nodes, the rest are no longer necessary and should be skipped to avoid any errors that could block the apply operation.

With the missing destroy node types sorted out, this transformer can now
be designed around finding the nodes we need to keep for destroy
operations, rather than the whack-a-mole game of detecting nodes we
don't want to keep.

When executing a destroy operation, we can never be certain that the
state will contain all the data required to evaluate the entire
configuration. This could be because of a previously failed apply
operation, or external changes made out of band. This means that once we
process the configuration to get the providers hooked up, we need to
remove any nodes that correspond to configuration values which may not
be available to evaluate.

In a destroy operation we primarily need three categories of nodes to
complete the plan and apply:
 - Nodes corresponding to values in the state
 - Providers required to destroy those values obtained from the state
 - Any node which a provider depends on, to ensure its configuration can
   be fully evaluated

Other than that minimal set of nodes, the rest are no longer necessary
and should be skipped to avoid errors which could block the apply
operation.
@jbardin jbardin requested a review from a team October 23, 2024 19:46
@jbardin jbardin marked this pull request as ready for review October 23, 2024 19:48
Copy link
Collaborator

@DanielMSchmidt DanielMSchmidt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds reasonable to me although I don't fully grasp it

@jbardin jbardin merged commit d73372a into main Oct 24, 2024
10 checks passed
@jbardin jbardin deleted the jbardin/unused-ephemeral-destroy branch October 24, 2024 13:04
Copy link
Contributor

Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch.

Copy link
Contributor

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.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants