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

Stored dependency handling during plan #28228

Merged
merged 3 commits into from
Mar 30, 2021
Merged

Stored dependency handling during plan #28228

merged 3 commits into from
Mar 30, 2021

Conversation

jbardin
Copy link
Member

@jbardin jbardin commented Mar 29, 2021

The current configuration was always being used to determine dependencies during refresh, because if there were no changes to a resource, there was no chance to replace any incorrect stored dependencies. Always refreshing dependencies prevented situations that required manual state manipulation while refactoring complex configurations.

The problem this caused however, was that dependencies to resources which had been removed from the configuration but not yet destroy were not being handled in the correct order. To handle orphaned dependencies, they must be maintained in the refreshed state all the way through apply. Now that we are refreshing during plan, we can always merge all the known dependencies (invalid dependency targets are not an error, they are ignored), and only if there is no change to the resource will the current config dependencies be stored immediately.

We can also clean up the writeResourceInstanceState calls to no longer modify the resource instance state argument, and remove the dependencies argument. Callers are now expected to set the Dependencies field as needed.

Fixes #28193

missing CreateBeforeDestroy
A stored dependency is documented as being honored even after it is
removed from the configuration, until the referenced resource is
destroyed.
The resource configuration was always being used to determine
dependencies during refresh, because if there were no changes to a
resource, there was no chance to replace any incorrect stored
dependencies. Now that we are refreshing during plan, we can determine
that a resource has no changes and opt to store the new dependencies
immediately.

Here we clean up the writeResourceInstanceState calls to no longer
modify the resource instance state, removing the `dependencies`
argument. Callers are now expected to set the Dependencies field as
needed.
@jbardin jbardin requested a review from a team March 29, 2021 18:34
@codecov
Copy link

codecov bot commented Mar 29, 2021

Codecov Report

Merging #28228 (7964328) into main (6f35c28) will increase coverage by 0.04%.
The diff coverage is 91.30%.

Impacted Files Coverage Δ
terraform/node_resource_abstract_instance.go 73.04% <ø> (-0.11%) ⬇️
terraform/node_resource_plan_instance.go 74.28% <87.50%> (+4.15%) ⬆️
states/state_deepcopy.go 79.31% <100.00%> (+15.35%) ⬆️
terraform/node_resource_apply_instance.go 50.47% <100.00%> (+0.47%) ⬆️
terraform/node_resource_destroy.go 61.36% <100.00%> (ø)
terraform/node_resource_plan_orphan.go 73.91% <100.00%> (ø)
terraform/transform_import_state.go 87.12% <100.00%> (ø)
backend/remote/backend_common.go 49.82% <0.00%> (-0.70%) ⬇️
dag/marshal.go 63.49% <0.00%> (+1.58%) ⬆️
internal/providercache/dir.go 73.46% <0.00%> (+6.12%) ⬆️
... and 1 more

@jbardin jbardin added the 0.15-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged label Mar 29, 2021
Copy link
Contributor

@alisdair alisdair left a comment

Choose a reason for hiding this comment

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

This makes sense to me!

@jbardin jbardin merged commit 1a15874 into main Mar 30, 2021
@jbardin jbardin deleted the jbardin/destroy-deps branch March 30, 2021 13:19
@jbardin jbardin mentioned this pull request Mar 30, 2021
@ghost
Copy link

ghost commented Apr 30, 2021

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 as resolved and limited conversation to collaborators Apr 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
0.15-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CBD side effect that "update depending resource first then delete depended resource" not work since v0.14
3 participants