Add StateLocker interface to backend operations #17422
Conversation
jbardin
force-pushed
the
jbardin/state-locking
branch
from
3488cc7
to
0dec603
Compare
Simplify the use of clistate.Lock by creating a clistate.Locker instance, which stores the context of locking a state, to allow unlock to be called without knowledge of how the state was locked. This alows the backend code to bring the needed UI methods to the point where the state is locked, and still unlock the state from an outer scope. Provide a NoopLocker as well, so that callers can always call Unlock without verifying the status of the lock. Add the StateLocker field to the backend.Operation, so that the state lock can be carried between the different function scopes of the backend code. This will allow the backend context to lock the state before it's read, while allowing the different operations to unlock the state when they complete.
Use the new StateLocker field to provide a wrapper for locking the state during terraform.Context creation in the commands that directly manipulate the state.
jbardin
force-pushed
the
jbardin/state-locking
branch
from
0dec603
to
bdd475e
Compare
command/clistate/state.go
Outdated
| // and LockInfo for the caller, while storing any related data required for | ||
| // Unlock. | ||
| type Locker interface { | ||
| // Lock the provided state, storing the reason string in the LockInfo. |
There was a problem hiding this comment.
This comment was a little confusing to me at first because "the LockInfo" seems like an implementation detail of this function, rather than something visible on this interface. However, reading the code below I see that it gets created and passed into the LockWithContext function; I think it could help to clarify a little what side-effects this function has for the given state, since the interactions between subsystems here aren't obvious.
There was a problem hiding this comment.
Ah, I see the confusion. It's because it used to be visible in the interface -- you have to pass a LockInfo to the state Lock method. I'll clarify this a little more
|
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
The additional UI code to report the status of obtaining a lock to the user broke the state locking order by having the state refreshed without a lock when creating the initial context. While the lock was still honored for the duration of the actual operations being applied, the command could be starting with a stale version of the state. Most of the time this is not a major issue, since the refresh phase would update the state again, but strict CAS operations used by the Consul backend (and other operations strictly comparing the state.Serial) would fail writing the new state.
Fixes #17407
We start by simplifying the use of
clistate.Lockby creating aclistate.Lockerinterface, which stores the context of locking a state, to allow unlock to be called without knowledge of how the state was locked. This allows the backend code to bring the needed UI methods to the point where the state is locked, and still unlock the state from an outer scope.That new Locker is then added to the
backend.Operation, so the lowest level function scopes of the backend code can lock the state before it's read, while still allowing the different operations to unlock the state when they complete.