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

terraform: don't prune state on init() #10504

Merged
merged 1 commit into from
Dec 2, 2016
Merged

terraform: don't prune state on init() #10504

merged 1 commit into from
Dec 2, 2016

Conversation

mitchellh
Copy link
Contributor

@mitchellh mitchellh commented Dec 2, 2016

During graph execution, there are steps that expect that a state isn't
being actively pruned out from under it. Namely: writing deposed states.

Writing deposed states has no way to handle if a state changes
underneath it because the only way to uniquely identify a deposed state
is its index in the deposed array. When destroying deposed resources, we
set the value to <nil>. If the array is pruned before the next deposed
destroy, then the indexes have changed, and this can cause a crash.

This PR does the following (with more details below):

  • init() no longer prunes.

  • ReadState() always prunes before returning. I can't think of a
    scenario where this is unsafe since generally we can always START
    from a pruned state, its just causing problems to prune
    mid-execution.

  • Exported State APIs updated to be robust against nil ModuleStates.

Instead, I think we should adopt the following semantics for init/prune
in our structures that support it (Diff, for example). By having
consistent semantics around these functions, we can avoid this in the
future and have set expectations working with them.

  • init() (in anything) will only ever be additive, and won't change
    ordering or existing values. It won't remove values.

  • prune() is destructive, expectedly.

  • Deserialization can prune, but it also might not. No expectation should be made either way and anything that uses a deserialized structure should expect either (basically same as last point here to be robust).

  • Serialization must not prune. We often serialize mid-execution to back up and pruning mid-execution is not allowed. If you must, then DeepCopy prior to serializing. Though in cases like State, this is prohibitively expensive.

  • Functions on a structure must not assume a pruned structure 100% of
    the time. They must be robust to handle nils. This is especially
    important because in many cases values such as Modules in state
    are exported so end users can simply modify them outside of the
    exported APIs.

This PR may expose us to unknown crashes but I've tried to cover our
cases in exposed APIs by checking for nil.

Init should only _add_ values, not remove them.

During graph execution, there are steps that expect that a state isn't
being actively pruned out from under it. Namely: writing deposed states.

Writing deposed states has no way to handle if a state changes
underneath it because the only way to uniquely identify a deposed state
is its index in the deposed array. When destroying deposed resources, we
set the value to `<nil>`. If the array is pruned before the next deposed
destroy, then the indexes have changed, and this can cause a crash.

This PR does the following (with more details below):

  * `init()` no longer prunes.

  * `ReadState()` always prunes before returning. I can't think of a
    scenario where this is unsafe since generally we can always START
    from a pruned state, its just causing problems to prune
    mid-execution.

  * Exported State APIs updated to be robust against nil ModuleStates.

Instead, I think we should adopt the following semantics for init/prune
in our structures that support it (Diff, for example). By having
consistent semantics around these functions, we can avoid this in the
future and have set expectations working with them.

  * `init()` (in anything) will only ever be additive, and won't change
    ordering or existing values. It won't remove values.

  * `prune()` is destructive, expectedly.

  * Functions on a structure must not assume a pruned structure 100% of
    the time. They must be robust to handle nils. This is especially
    important because in many cases values such as `Modules` in state
    are exported so end users can simply modify them outside of the
    exported APIs.

This PR may expose us to unknown crashes but I've tried to cover our
cases in exposed APIs by checking for nil.
Copy link
Member

@jbardin jbardin left a comment

Choose a reason for hiding this comment

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

LGTM

@mitchellh mitchellh merged commit be012a5 into master Dec 2, 2016
@mitchellh mitchellh deleted the b-no-prune branch December 2, 2016 18:13
@ghost
Copy link

ghost commented Apr 19, 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 19, 2020
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