For what is worth, if you need something almost equivalent right now, that is already possibile when using cookbooks using a shared custom exclusive lock in addition to the current check for clean diff. That would prevent any other cookbook to touch dbctl at the same time. If all the automated usage of dbctl will go via cookbooks that will cover most of the cases.
For the remaining use case of having to mangle with dbctl manually for more complex operations that involve touching more objects before being able to commit them (like adding a new section), that could also be covered having a few line cookbook that will just hold the lock and wait for the operator to complete the manual mangling.

Thanks for opening this @FCeratto-WMF!

There's an interesting discussion to be had about where a mechanism for isolating high-level dbctl operations from each other should live, along with the details of how they're implemented.

In the near term, making the most common operations cookbook-driven, with a custom exclusive lock (i.e., for a global "commit" scope) covering all mutations like @Volans describes and diff checks would together go a long way to making the current state of the world safer. For example, extending cookbooks.sre.mysql.(de)pool to do exactly that for mutate+commit seems like a solid quick-win, and a pattern that could be extended to other cookbooks (possibly those with more involved human-driven steps, as @Volans notes).

Of course, there are caveats to that - finding the right tradeoff for lock TTL (with the possibility of abandoned locks), risk of TOCTOU issues during pre-commit safety checks, etc. - but we can probably find decent solutions for these (e.g., for the latter, I wonder if we could make use of the CAS-like behavior for writes in the etcd v2 API for commit).

Longer term, on the v3 etcd AP, it would be interesting to see whether we could benefit from transactions or not. Those provide the ability to perform operations across multiple keys that are applied atomically, gated on a set of predicates, but are inherently "single-shot" (i.e., there is no way to hold a transaction open covering a series of related operations). Meaning, making use of them requires kind of a rethinking of how we stage / apply updates.

Change #1124797 had a related patch set uploaded (by Federico Ceratto; author: Federico Ceratto):

[operations/cookbooks@master] dbctl.py, dbctl_test.py: Serialize dbctl changes

https://gerrit.wikimedia.org/r/1124797

Based on the feedback from your comments and discussion in person and on IRC I've put together a simple cookbook that acts as a wrapper for dbctl in the related CR.
It uses a custom exclusive lock and the typical lock duration should be very short, ideally less than a second, well below the lock timeout.
In case there are unexpected changes it will wait for up to 30 seconds, still far below the lock timeout.

It exposes the usual dbctl CLI commands the team is familiar with but allows running multiple command and committing in one invocation.

Over time we can migrate manual and programmatic usage of dbctl to using the locking mechanism and it will solve TOCTOU issues and unexpected/lingering changes.
Being able to do dbctl changes reliably will unblock more automation further down the line.

To avoid any potential misunderstanding what I suggested above in this task was more along the line of:

• in any cookbook that already uses dbctl, do any dbctl change while holding an exclusive cookbook custom lock (with the same lock name in all cookbooks)
• slowly migrate all the existing automated usage of dbctl into cookbooks, also while holding the same exclusive lock
• write a few lines cookbook that just acquires the same lock and stops there with an ask_confirmation to wait for the operator to make all the manual changes in dbctl before releasing the lock. This to allow the more complex manual operations that requires multiple dbctl commands to be coordinated together and are not automated.

In my experience when multi-stage changes are needed in dbctl that's because a more delicate change is being made, like adding a new section, changing some general behavior, etc. In those cases the operator performing the changes often asks other SREs to check the changes with dbctl as a form of peer-review before committing them.

The proposed change in the CR above is much more drastic, running a series of dbctl commands from a file in a sequence and then committing them. In my opinion running commands from a file should be avoided as it's quite easy to make mistakes and run unwanted commands. An operator could easily pick the wrong file, or thinking to have modified one but not saved the changes, etc. The files will also be local to just one cumin host.

In any more drastic scenario we should also take into account what should happen to the current dbctl CLI.

So right now the idea of leaving dbctl CLI is very unrealistic, we don't have cookbooks for everything and we use dbctl on a daily basis (true, that most of the operations are depool/repool) but we do change weights, change masters, etc.
Whatever solution we decide to implement, will need to keep in mind that there're chances that the cookbook may run into race conditions with someone manually doing things via dbctl CLI.

I have a few questions on the task itself, rather than on the implementation (I have those too, but they're not important until we've clarified the problem statement a bit).

What problem are we trying to solve? I see probably two things listed in this task, please correct me if I'm wrong:

• We want to ensure that there are no uncommitted changes for too long (how long?)
• We want to make sure that people can't overwrite each other's changes with their respective concurrent commits

If I have understood this correctly - then I see better avenues to obtain what we want:

• Create monitoring checks for uncommitted changes in dbctl and alert on those. It will obtain the result without reducing the flexibility in the use of dbctl
• As @Scott_French was suggesting, adding a proper CAS version of the update verb in conftool is the cleaner solution for the problem of race conditions in commits. It will also allow better error management and feedback to the user, and/or to autocorrect show the actual diffs again in interactive mode.

I must add: the only reason I see of going in the direction you took is if this is the first step in making dbctl just a library and drive all interactions with it via cookbooks. While I'm not against this idea in principle, if that's the case I'd like to see it explained in a brief design document/separate task.

In T387209#10625277, @Joe wrote:

• We want to ensure that there are no uncommitted changes for too long (how long?)

[...snip...]

• Create monitoring checks for uncommitted changes in dbctl and alert on those. It will obtain the result without reducing the flexibility in the use of dbctl

FWIW that's already the case, we have icinga checks configured in modules/profile/manifests/conftool/dbctl_client.pp.

Coming back to this, I want to touch on a couple of things.

First, for clarity, the concept I had in mind as a near-term solution for mutual exclusion across automated use cases in T387209#10581041 is quite similar to what @Volans describes in T387209#10617070:

1. introduce a custom exclusive lock that can be adopted by existing cookbooks that implement dbctl workflows (e.g., cookbooks.sre.mysql.(de)pool) and
2. over time, migrate other dbctl workflows into cookbooks, adopting the same strategy.

As, @Volans notes, a short-term option for #2 might be a lock-only cookbook, which is used to exclude still-manual dbctl workflows from each other. While that has the downside of being sort of an administrative control (i.e., operator needs to acquire the lock before starting work), it has the benefit of ensuring the same ergonomics and feedback for individual operations as exist now.

In a sense, https://gerrit.wikimedia.org/r/1124797 takes a different approach, where those individual operations are now delegated to the cookbook for execution. While that ensures the first part (lock is acquired), it's a significant departure in terms of the second part (ergonomics and feedback).

Zooming out, there are a couple core question here, closely related to what @Joe touches on in T387209#10625277:

What should automated dbctl-based workflows to look like ideally?

Regardless of whether today's versions of these workflows are fully manual based on a runbook, shell-script driven, etc., I suspect (hope?) one key part of this is we want to evolve toward relying only on reviewed and committed code. In that sense, investing in supporting a new mode of ad-hoc dbctl command sequence execution seems like effort that could be refocused elsewhere.

In the near term, it seems like the direction is to base this on cookbooks, which as @Joe notes, would imply that dbctl evolves to primarily be used as library.

Since I'm not a DBA, I don't grasp the full range of "dbctl critical sections" (for lack of a better term) that exist in workflows now. However, it feels like mapping those out sooner rather than later is critical to understanding what's achievable with cookbooks (and would thus be able to benefit from an exclusion mechanism implemented there).

What exclusion guarantees do we want to make, and what features are necessary to achieve that?

At a high level, we do not want concurrent mutations of etcd state by different workflows - e.g., ones that could either conflict directly (i.e., mutate the same keys) or cause one workflow to inadvertently commit the result of another's (disjoint) mutations, all or in part.

As @Joe notes, augmenting the commit operation with CAS-like behavior (e.g., etcd v2 prevValue write) that asserts that the committed diff is the "expected" diff (confirmed either by a human or an automated safety check inspecting a structured diff, as in the parent task) should be able to achieve that (note: here, I'm leaning heavily on "expected" which might be non-trivial to automate in the general case).

In a sense, mutual exclusion based on a distributed lock in this form (e.g., spicerack lock) is an "optimization" - albeit a very effective one: it helps us avoid getting into that situation by making it very unlikely to have two workflows mutating etcd state at the same time (e.g., resulting in interleaved / conflicting ops that fail the diff check, or that cause CAS write to fail) that we then have to clean up.

In an etcd v3 world, we could take that one step further, by preventing writes entirely when the writer no longer holds the lock (since we'll have multi-key transactions that would allow the lock to function as a fencing token). This would not fully solve the problem, though, as writes that have already happened are still there (so human intervention for cleanup is still needed).

In any case, I think what I'm trying to say is, it would be helpful to document the specific guarantees we want to achieve, and how that maps onto the tools available to us.

Additional points of note:

• Just to highlight, outside of DBA, effectively no one is going to touch dbctl other than to depool a misbehaving host. Thus, if you're able to extend this functionality at least to cover that use case (including the parsercache use case, which I realize isn't a cookbook as of yet), then you've basically completely ruled out manual operations by non-DBAs from causing conflicts.

In T387209#10625905, @Volans wrote:

In T387209#10625277, @Joe wrote:

• We want to ensure that there are no uncommitted changes for too long (how long?)

[...snip...]

• Create monitoring checks for uncommitted changes in dbctl and alert on those. It will obtain the result without reducing the flexibility in the use of dbctl

FWIW that's already the case, we have icinga checks configured in modules/profile/manifests/conftool/dbctl_client.pp.

Ok, so that problem is already resolved. Thanks.

Regarding the last addition to the task description:

[1]: The discussion was around this bug T383760#10461481 that is blocking pool-in automation requiring manual input

@FCeratto-WMF That is only marginally related to this task. The (de)pool cookbook checks and waits until the dbctl diff is clean before proceeding, reducing the risk of a race condition with other changes considerably. The specific error message you linked is due to the diff generated by the (de)pool action itself in some specific cases due to JSON lack of commas in the last item of objects/lists. The solution for that is discussed in the task and Scott has kindly implemented two possible solutions and AFAIK that task is actually waiting for your input (see T383760#10569125 ) to get one of the two solution merged and deployed so that it can be used in the (de)pool cookbook.

Based on a meeting with @Scott_French I created T389947 and reparented T387209 under it. I was told that having T387209 as a child of T383760 created confusion, sorry about that.

I was asked to provide my opinion. As a DBA, I really prefer to have a simple solution. For many automation (which I mean some bash scripts and simple python scripts) we have built over the years, what we did was something simple: If we wanted to make a dbctl change, we checked the diff (sudo dbctl config diff i.e. checking for uncommitted stuff), if it was clean (empty diff), then immediately we made the change (depool/pool) and committed it right away. If the diff was not clean (there were some uncommitted change), it patiently waited for some time before trying again (in a loop). What I mean is that, how it should be diffed or content of the diff doesn't matter.

Of course that had failure scenarios such as "what if something happened between issuing the diff command and commit" but given that all of it was done automatically and less than a second, it never happened in practice and we have been fine in the past 3+ years with a lot of automation in place.

So IMHO, we can really simplify everything and just do this in a cookbook, the failure scenarios are not really realistic and even if they happened, they are not really causing issues (or we have way bigger problems in production).

In T387209#10674140, @Ladsgroup wrote:

So IMHO, we can really simplify everything and just do this in a cookbook

Just for historical context this is already what the (de)pool cookbooks do since their first version.

In T387209#10674186, @Volans wrote:

In T387209#10674140, @Ladsgroup wrote:

So IMHO, we can really simplify everything and just do this in a cookbook

Just for historical context this is already what the (de)pool cookbooks do since their first version.

Yes! My point is that wait_diff_clean is enough and we should get rid of check_diff altogether (even check_action_result too)

We discussed next steps on IRC in #wikimedia-data-persistence minutes ago and agreed that for the time being we are ok with removing check_diff to unblock depooling in the cookbook.