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

Add CreateBeforeDestroy to instance state #24084

Merged
merged 5 commits into from
Mar 9, 2020
Merged

Conversation

jbardin
Copy link
Member

@jbardin jbardin commented Feb 12, 2020

Normally CreateBeforeDestroy is only used from the configuration, because it's primarily thought of in the operation ordering for instance replacement. It however also effects dependent resource operations by updating the dependent before destroying the CBD instance. We need to store CreateBeforeDestroy in the state, because that is the only way to record the proper ordering for dependent resource updates when a resource is removed from the config.

Fixes: #23635, along with numerous issues that have misdiagnosed in providers, where updates are applied in the wrong order when removing dependent resources.

@ghost ghost added the sdkv1 [PRs only] Marks changes that may potentially need to be ported to the plugi nSDK label Feb 12, 2020
@jbardin jbardin requested a review from a team February 13, 2020 19:11
Added to src and object, but still need serialization and tests.
If the Diff is only a delete action, we can't override
CreateBeforeDestroy, because it will always be false and prevent the
stored state value from being used.
Even though this is only the destroy half of CreateBeforeDestroy, the
resource may still require the same destroy order.
If the resource was stored as CreateBeforeDestroy, use the same ordering
regardless.

This reversal will be taken care if more cleanly in state-only destroys,
and with less risk. Don't use this commit as-is.
@jbardin jbardin marked this pull request as ready for review February 14, 2020 02:13
@jbardin jbardin closed this Feb 14, 2020
@jbardin jbardin deleted the jbardin/cbd-instance-state branch February 14, 2020 02:51
@jbardin jbardin restored the jbardin/cbd-instance-state branch March 4, 2020 16:08
@jbardin jbardin reopened this Mar 4, 2020
@codecov
Copy link

codecov bot commented Mar 4, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@ded207f). Click here to learn what that means.
The diff coverage is 71.42%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #24084   +/-   ##
=========================================
  Coverage          ?   56.94%           
=========================================
  Files             ?      656           
  Lines             ?    59228           
  Branches          ?        0           
=========================================
  Hits              ?    33730           
  Misses            ?    22486           
  Partials          ?     3012
Impacted Files Coverage Δ
states/instance_object.go 0% <ø> (ø)
terraform/transform_diff.go 78.82% <ø> (ø)
states/instance_object_src.go 0% <0%> (ø)
states/state_deepcopy.go 63% <100%> (ø)
terraform/node_resource_destroy.go 78.7% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ded207f...85ebaac. Read the comment docs.

@jbardin jbardin merged commit 654e880 into master Mar 9, 2020
@jbardin jbardin deleted the jbardin/cbd-instance-state branch March 9, 2020 17:16
noahmercado pushed a commit to noahmercado/terraform that referenced this pull request Apr 8, 2020
…e-state

Add CreateBeforeDestroy to instance state
@ghost
Copy link

ghost commented Apr 9, 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 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
sdkv1 [PRs only] Marks changes that may potentially need to be ported to the plugi nSDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect order of destroying resources - Flaky/inconsistent behavior
2 participants