RFC: dynamic enable/disable of ERS by keyspace in VTOrc

[timvaillancourt]

Feature Description

As a user of VTOrc, I would like to be able to disable ERS on a single keyspace. Currently disabling ERS recoveries is possible using the --allow-emergency-reparent flag (restart required), or the HTTP API (need to update every instance individually), but these approaches have the drawback of affecting all keyspaces watched

 --allow-emergency-reparent
    Whether VTOrc should be allowed to run emergency reparent operation when it detects a dead primary (default true)

This RFC/feature request proposes:

• New vtctldclient RPCs: EnableVtorcEmergencyReparent <keyspace> and DisableVtorcEmergencyReparent <keyspace> (or similar)
• New stats to indicate which keyspaces have ERS disabled. This will be useful to avoid forgetting it is disabled using external monitoring
• VTOrc stores the ERS enabled/disabled state and considers this on recoveries

The RFC in it's current state is implemented by this PR. Your feedback/review is appreciated! 🙇

Use Case(s)

A VTOrc user with many keyspaces per-instance that would like to disable ERS recoveries by-keyspace, without restarting VTOrcs

[timvaillancourt]

👋 a question to all, I think I'm happy with my simpler approach in this PR, but I wanted to call out that old-Orchestrator supported the concept of downtimes with a TTL

The simpler approach I took here just adds fields to the topo Keyspace record, but assuming topo stores support TTLs on KVs, a TTL approach is possible too

The drawback I see with that approach is needing to predict how long to disable - it's probably better to just have full control as long as there is alerting if someone forgets about it

Any feedback?

[deepthi]

May I point out that you only need one RPC "Enable..." that takes a Boolean?
Regarding TTLs I agree that we should not introduce them. Silently turning on something could be a surprise. Let it be intentionally triggered by a human or external automation versus coding it into vtorc.
Beyond that yes, it's probably useful for people to disable ERS on a specific keyspace but I can foresee the next enhancement which would be at a shard level

[timvaillancourt]

May I point out that you only need one RPC "Enable..." that takes a Boolean?

@deepthi sure, we could go that route too 👍. Maybe SetVtorcEmergencyReparent or SetKeyspaceVtorcEmergencyReparent with a required disable/enable flag?

I can foresee the next enhancement which would be at a shard level

Yes, I agree. I could add this in the initial PR or a 2nd one. I suppose that would make the full command line something like:

Single-shard:

vtctldclient <flags> SetVtorcEmergencyReparent --disable ks1/40-80

Keyspace:

vtctldclient <flags> SetVtorcEmergencyReparent --disable ks1

or

vtctldclient <flags> SetVtorcEmergencyReparent --disable ks1/-

Your thoughts appreciated 🙇!

[shlomi-noach]

@timvaillancourt just to clarify: if I run

vtctldclient <flags> SetVtorcEmergencyReparent --disable ks1/40-80

and then

vtctldclient <flags> SetVtorcEmergencyReparent --enable ks1

The 2nd command does not override the first, correct? Shard 40-80 will remain disabled?
This is fine if so, and in line with similar behavioral patterns (as with the tablet throttler). But just want to clarify that, and to make sure this is clarified in code comments as well as in the docs, as it will surely confuse some.

[timvaillancourt]

@timvaillancourt just to clarify: if I run

vtctldclient <flags> SetVtorcEmergencyReparent --disable ks1/40-80

and then

vtctldclient <flags> SetVtorcEmergencyReparent --enable ks1

The 2nd command does not override the first, correct? Shard 40-80 will remain disabled?
This is fine if so, and in line with similar behavioral patterns (as with the tablet throttler). But just want to clarify that, and to make sure this is clarified in code comments as well as in the docs, as it will surely confuse some.

@shlomi-noach correct, in the current implementation the shard and keyspace overrides are separate, so the 2nd command you mentioned would NOT undo the disabling of the 40-80 shard, but it certainly could if we feel that's more intuitive. We'd need N x shard locks but this doesn't need to be fast

Agreed there should be clarity on this in docs 👍