Transactionality, locking and other concurrency coordination

[pedronis]

In the managers and overlord we use the following tools to make sure operations don’t interfere with each other or produce broken state:

• locking: State.Lock and State.Unlock where the latter also serializes the state to disk, so that it can be picked up across restarts, crashes etc and operation can continue

• when an operation spans multiple tasks we use conflicts and checks for pre-existing pending/running tasks/changes to make sure for example we don’t have concurrent operations on the same snap (see e.g. snapstate.CheckChangeConflict and devicemgr.changeInFlight)

• we also use TaskRunner.SetBlocked to set predicate functions to control the serialization of tasks under the same manager, for example ifacestate conservatively serializes all of its tasks because they can touch multiple snaps

Generally we prefer correctness over concurrence, so in general tasks that are mutating global shared parts of the state should take the State lock once. Exceptions that release the lock around possibly slow operations should avoid touching shared global bits at all if possible, or use serialization otherwise carefully and sparingly.

[pedronis]

ATM as discussed with @niemeyer snapstate especially doesn’t follow “Generally we prefer correctness over concurrence” but has quite a few tasks that proceed updating the SnapState of a snap across unlocking and relocking the state, just trusting the conflict mechanism to protect this, which is something that it is a bit too implicit to trust going forward. We decided we want to fix that and I will prepare a PR making those tasks take the lock only once. If we hit real bottlenecks we will revisit this possibly using serialization instead as discussed.

[niemeyer]

Thanks for working on this, and thanks for documenting it here as well!

[chipaca]

I thought we’d be doing this after we moved to a real(ish) database for state.

[niemeyer]

@chipaca There’s probably some misunderstanding about what is being proposed above. The proposal for the time being is simply to lock a bit differently, including some relatively fast disk operations that we used to unlock on. That’s what is meant by prioritizing simplicity (implying easier correctness) rather than performance on those cases.

[pedronis]

@niemeyer @chipaca opened

overlord/snapstate: unlock/relock the state less, especially not across mutating the SnapState of a snap

[niemeyer]

The comment @pedronis added to that PR is so good I’m pulling it out here for reference and awareness

---

State Locking

do* / undo* handlers should usually lock the state just once with:

    st.Lock()
    defer st.Unlock()

For tasks doing slow operations (long i/o, networking operations) it’s OK to unlock the state temporarily:

    st.Unlock()
    err := slowIOOp()
    st.Lock()
    if err != nil {
       ...
    }

but if a task Get and then Set the SnapState of a snap it must avoid releasing the state lock in between, other tasks might have reasons to update the SnapState independently:

    // DO NOT DO THIS!:
    snapst := ...
    snapst.Attr = ...
    st.Unlock()
    ...
    st.Lock()
    Set(st, snapName, snapst)

if a task really needs to mix mutating a SnapState and releasing the state lock it should be serialized at the task runner level, see SnapManger.blockedTask and TaskRunner.SetBlocked

[niemeyer]

@pedronis PR reviewed.

[pedronis]

@niemeyer st.RequestRestart(state.RestartDaemon) is just daemon tomb.Kill(nil) through a bunch of abstraction indirections, is not waiting for the restart at that point . The system restart case is os.Execing shutdown from the call path but we want to change that, it’s also an async request though, not waiting for the actual shutdown.