state: more robust handling of state Serial #15424
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jbardin
reviewed
Jun 29, 2017
| } else if s == nil || other == nil { | ||
| return false | ||
| } | ||
|
|
There was a problem hiding this comment.
not a big deal, but do you want to short-circuit marshaling by comparing serial/lineage first?
There was a problem hiding this comment.
There isn't really a great reason to call this without those matching, so I didn't see a reason to optimize for the edge case here... the only current practical use for this is to compare two states that have the same serial and lineage, to detect any other updates.
There was a problem hiding this comment.
true, it's just the endless optimizer in me coming out in the wrong place 👍
apparentlymart
force-pushed
the
b-apply-serial-update
branch
from
367996b
to
4f528e2
Compare
Previously we relied on a constellation of coincidences for everything to work out correctly with state serials. In particular, callers needed to be very careful about mutating states (or not) because many different bits of code shared pointers to the same objects. Here we move to a model where all of the state managers always use distinct instances of state, copied when WriteState is called. This means that they are truly a snapshot of the state as it was at that call, even if the caller goes on mutating the state that was passed. We also adjust the handling of serials so that the state managers ignore any serials in incoming states and instead just treat each Persist as the next version after what was most recently Refreshed. (An exception exists for when nothing has been refreshed, e.g. because we are writing a state to a location for the first time. In that case we _do_ trust the caller, since the given state is either a new state or it's a copy of something we're migrating from elsewhere with its state and lineage intact.) The intent here is to allow the rest of Terraform to not worry about serials and state identity, and instead just treat the state as a mutable structure. We'll just snapshot it occasionally, when WriteState is called, and deal with serials _only_ at persist time. This is intended as a more robust version of #15423, which was a quick hotfix to an issue that resulted from our previous slopping handling of state serials but arguably makes the problem worse by depending on an additional coincidental behavior of the local backend's apply implementation.
apparentlymart
force-pushed
the
b-apply-serial-update
branch
from
4f528e2
to
4d53eaa
Compare
apparentlymart
changed the title
In practice, States must all implement the full interface, so checking for each method set only leaves gaps where tests could be skipped. Change the helper to only accept a full state.State implementation. Add some Lineage, Version, and TFVersion checks to TestState to avoid regressions. Compare the copy test against the immediate State returnedm rather than our previous "current" state. Check that the states round-trip and still marhsal identically via MarshalEqual.
The state returned from the testState helper shouldn't rely on any mutations caused by WriteState. The Init function (which is analogous to NewState) shoudl set any required fields.
We have a helper function that we hardly ever use. TODO: convert the rest of the manual ReadState calls eventually.
jbardin
force-pushed
the
b-apply-serial-update
branch
from
fca3e82
to
fb39706
Compare
jbardin
approved these changes
Jul 5, 2017
|
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
locked and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously we relied on a constellation of coincidences for everything to work out correctly with state serials. In particular, callers needed to be very careful about mutating states (or not) because many different bits of code shared pointers to the same objects.
Here we move to a model where all of the state managers always use distinct instances of state, copied when
WriteStateis called. This means that they are truly a snapshot of the state as it was at that call, even if the caller goes on mutating the state that was passed.We also adjust the handling of serials so that the state managers ignore any serials in incoming states and instead just treat each Persist as the next version after what was most recently Refreshed.
(An exception exists for when nothing has been refreshed, e.g. because we are writing a state to a location for the first time. In that case we do trust the caller, since the given state is either a new state or it's a copy of something we're migrating from elsewhere with its state and lineage intact.)
The intent here is to allow the rest of Terraform to not worry about serials and state identity, and instead just treat the state as a mutable structure. We'll just snapshot it occasionally, when
WriteStateis called,and deal with serials only at persist time.
This is intended as a more robust version of #15423, which was a quick hotfix to an issue that resulted from our previous slopping handling of state serials but arguably makes the problem worse by depending on an additional coincidental behavior of the local backend's apply
implementation.