Quick first feedback/questions on the proposal:

dbconfig get NAME gets you all the current configuration of a mysql instance (so either host:port or host only if it's the default port)

I think that if NAME is just the hostname, it should show the config for all the configured HOST:PORT combinations in that host.

dbconfig list [SECTION|all] shows what is configured for that specific section, or for all of them

It seems to me that show might be a better verb here, from list I would expect a list of things, not their full config. Also doesn't seem to match the actions below dbconfig section [command], so maybe could be revisited or considered a shortcut, for example of dbconfig section SECTION show.

dbconfig [depool|warmup|pool] [NAME] [selector tags]

If no selector tags are specified, does it apply to all? All configured? It's not clear.
If I do pool after a depool and a warmup with different values (0.1, 0.3, 0.6), what it does? I would expect it to reset the weight to it's configured value.

dbconfig warmup NAME 0.1 pools a depooled database with weights set to ceil(weights*0.1)

Following the above question, without specifying selector tags, how this applies to a db that is both in sectionLoads with wight 100 and in groupLoadsBySection, in multiple subsections, with different weights? Would the warmup factor apply to all of them?
I guess that if applied to an already pooled host it will just change it's wight accordingly, is that correct?

dbconfig edit LABEL

What is a LABEL here?

dbconfig section [command] SECTION [params]

Consider also the alternative dbconfig section SECTION [command] [params], it should reduce the horizontal scrolling when doing maintenance (check current value, change it, verify it, etc..). Also the values are close to their label (section SECTION, ro REASON, etc..)

This is great! Thanks for getting on with this - these are my first thoughts!

Depool/pool/warmup
dbconfig [depool|warmup|pool] [NAME] [selector tags], where selector tags are on function (main, vslow, rc, etc.)
depool means removing from sectionLoads and groupLoadsBySection
Sanity check 2: we're not leaving a section with less than N slaves (configurable)

There should be an emergency mode like --emergency
Some of our sections are a bit over provisioned (like s5), and we can probably serve them (in a degraded state) with 1-2 slaves (as was shown during https://wikitech.wikimedia.org/wiki/Incident_documentation/20171116-s5-dewiki-wikidata where most of our slaves broke).
There should be a quick way to by pass the "N min amount of slaves" if required during an emergency. Some sort of: "I know what I am doing" mode.

dbconfig section set-master SECTION NAME sets the master for that specific section to NAME
Sanity check 1: Will verify that the server is pooled for that section

This is not likely to happen in a near future, but as we are starting from 0....I would suggest that the sanity check will consider the fact that a master can be in one or multiple sections (it is unlikely that we will decide to run multi-instance on masters, but I don't think we can completely discard it).

There should also be another sanity check, which is: a sanitarium master cannot be pooled as master.
However, the only way to identify a sanitarium master right now is to scan db-eqiad.php or db-codfw.php - we do not have them anywhere else.
The reason we cannot get them as masters is because they run ROW based replication and not STATEMENT.
We actually ran ROW based replication on s5 a year ago (by mistake) and despite the fact that it worked fine, we are not completely ready for it at this point as doing schema changes the way we do them now (slave by slave) is incompatible (and caused an outage - https://wikitech.wikimedia.org/wiki/Incident_documentation/20171116-s5-dewiki-wikidata)

I think that if NAME is just the hostname, it should show the config for all the configured HOST:PORT combinations in that host.

No, NAME should be instances only, this tools should not know anything about servers. If you run something on HOST and it is not on the default port, it should give out an error and fail quickly.

After thinking for a while, pool|depool|warmup (as an interface, not as the idea) looks confusing and not adjusted to real usage, several people have objected to it- plus if you have a host fully pooled and run warmup 0.5, you are actually depooling it partially. What would happen after doing a warmup depending on the initial state is missleading. My proposal is to get rid of the warmup verb, which is confusing, and have only 2 verbs:

• pool [fraction]
• depool

depool removes an instance from a service (or from all services), pool, without parameters re-adds it with the default, pool with a parameter re-adds it with the given percentage pool == pool 1. That will make completely sure what you are doing when you are running those actions (pooling or depooling). If someone puts a fraction on depooling, it should error out directly.

Please let's use the terminology mediawiki uses:

• Section is 's1', 's2', 'm1', 'pc1', 'es1' (some of those sections are shards, some are not)
• subtypes of traffic are called 'groups' (watchlist, vslow, dump)
• Load or section load is value assigned to a combination of section + instance name. Group load is the value for a section, instance and group

Also, load 0 + pooled is a valid state, and it is different from the "depooled" state.

Sanity checks are a complex issue and should not be discussed in long here until we have something to work with. Note that when we depool a server, it is likely we reassign a host to a different group temporarily, rather than having multiple (or at least 1) host all the time. For example, s3 has no groups defined except vslow and dump. When a group doesn't exist, it uses the general/main load.

In T197126#4280521, @Volans wrote:

Quick first feedback/questions on the proposal:

dbconfig get NAME gets you all the current configuration of a mysql instance (so either host:port or host only if it's the default port)

I think that if NAME is just the hostname, it should show the config for all the configured HOST:PORT combinations in that host.

NAME is generic as a label and typically will be HOST:PORT or HOST if the port is 3306; like we do currently in mediawiki-config; I was not planning on doing "all instances on a host" automagically.

dbconfig list [SECTION|all] shows what is configured for that specific section, or for all of them

It seems to me that show might be a better verb here, from list I would expect a list of things, not their full config. Also doesn't seem to match the actions below dbconfig section [command], so maybe could be revisited or considered a shortcut, for example of dbconfig section SECTION show

yeah, let's go that way. dbconfig secition SECTION show it is.

dbconfig [depool|warmup|pool] [NAME] [selector tags]

If no selector tags are specified, does it apply to all? All configured? It's not clear.
If I do pool after a depool and a warmup with different values (0.1, 0.3, 0.6), what it does? I would expect it to reset the weight to it's configured value.

I'm not sure I get what scenario you mean, but to be more specific: let's say you have a db with weights of 100 in main and 10 in vslow. dbconfig warmup mydb 0.1 will set the weights in mediawiki to 1/10th of the weight you stored, so respectively 10 and 1. If you then do dbconfig warmup mydb 0.6, the weights will now be 60 and 6 respectively.

dbconfig warmup NAME 0.1 pools a depooled database with weights set to ceil(weights*0.1)

Following the above question, without specifying selector tags, how this applies to a db that is both in sectionLoads with wight 100 and in groupLoadsBySection, in multiple subsections, with different weights? Would the warmup factor apply to all of them?
I guess that if applied to an already pooled host it will just change it's wight accordingly, is that correct?

Yes to all, see above

dbconfig edit LABEL

What is a LABEL here?

Should've been NAME. Amended.

dbconfig section [command] SECTION [params]

Consider also the alternative dbconfig section SECTION [command] [params], it should reduce the horizontal scrolling when doing maintenance (check current value, change it, verify it, etc..). Also the values are close to their label (section SECTION, ro REASON, etc..)

ok, amended.

@Joe ack to all your replies, thanks for integrating the suggestions!

In T197126#4282045, @jcrespo wrote:

After thinking for a while, pool|depool|warmup (as an interface, not as the idea) looks confusing and not adjusted to real usage, several people have objected to it- plus if you have a host fully pooled and run warmup 0.5, you are actually depooling it partially. What would happen after doing a warmup depending on the initial state is missleading. My proposal is to get rid of the warmup verb, which is confusing, and have only 2 verbs:

• pool [fraction]
• depool

depool removes an instance from a service (or from all services), pool, without parameters re-adds it with the default, pool with a parameter re-adds it with the given percentage pool == pool 1. That will make completely sure what you are doing when you are running those actions (pooling or depooling). If someone puts a fraction on depooling, it should error out directly.

Perfect, this is much better indeed. Amending the UI specs.

Please let's use the terminology mediawiki uses:

• Section is 's1', 's2', 'm1', 'pc1', 'es1' (some of those sections are shards, some are not)
• subtypes of traffic are called 'groups' (watchlist, vslow, dump)
• Load or section load is value assigned to a combination of section + instance name. Group load is the value for a section, instance and group

Also, load 0 + pooled is a valid state, and it is different from the "depooled" state.

Yes, pooled/depooled and weight are going to be different and distinct metrics.

Sanity checks are a complex issue and should not be discussed in long here until we have something to work with. Note that when we depool a server, it is likely we reassign a host to a different group temporarily, rather than having multiple (or at least 1) host all the time. For example, s3 has no groups defined except vslow and dump. When a group doesn't exist, it uses the general/main load.

In T197126#4281781, @Marostegui wrote:

This is great! Thanks for getting on with this - these are my first thoughts!

Depool/pool/warmup
dbconfig [depool|warmup|pool] [NAME] [selector tags], where selector tags are on function (main, vslow, rc, etc.)
depool means removing from sectionLoads and groupLoadsBySection
Sanity check 2: we're not leaving a section with less than N slaves (configurable)

There should be an emergency mode like --emergency
Some of our sections are a bit over provisioned (like s5), and we can probably serve them (in a degraded state) with 1-2 slaves (as was shown during https://wikitech.wikimedia.org/wiki/Incident_documentation/20171116-s5-dewiki-wikidata where most of our slaves broke).
There should be a quick way to by pass the "N min amount of slaves" if required during an emergency. Some sort of: "I know what I am doing" mode.

The famous --bblack switch. Yes, there will be multiple ways to work around the safety checks, the easiest one will be a --unsafe switch to the dbconfig cli.

dbconfig section set-master SECTION NAME sets the master for that specific section to NAME
Sanity check 1: Will verify that the server is pooled for that section

This is not likely to happen in a near future, but as we are starting from 0....I would suggest that the sanity check will consider the fact that a master can be in one or multiple sections (it is unlikely that we will decide to run multi-instance on masters, but I don't think we can completely discard it).

Not sure I understand what I should check here: if the instance is in multiple sections, refuse to promote it to master?

There should also be another sanity check, which is: a sanitarium master cannot be pooled as master.
However, the only way to identify a sanitarium master right now is to scan db-eqiad.php or db-codfw.php - we do not have them anywhere else.

Well we can move that information to the section metadata on etcd.

Adding this.

In T197126#4282109, @Joe wrote:

This is not likely to happen in a near future, but as we are starting from 0....I would suggest that the sanity check will consider the fact that a master can be in one or multiple sections (it is unlikely that we will decide to run multi-instance on masters, but I don't think we can completely discard it).

Not sure I understand what I should check here: if the instance is in multiple sections, refuse to promote it to master?

No, just saying that when checking if the master is in an specific section, you might want to consider the fact that it could be appearing in multiple sections and act accordingly.
"I want to promote this server to master on s4 even if it is pooled as master in some other section". Again, this is unlikely to happen in a near future, but just saying that it is not a completely crazy idea.
So long story short, consider that masters can be pooled in more than one section.

Change 441396 had a related patch set uploaded (by Giuseppe Lavagetto; owner: Giuseppe Lavagetto):
[operations/software/conftool@master] [WIP] Add a WMF-specific tool for managing db config in MediaWiki

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

Change 441396 had a related patch set uploaded (by CDanis; owner: Giuseppe Lavagetto):
[operations/software/conftool@master] Add a WMF-specific tool for managing db config in MediaWiki

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

Here's my tentative plan for moving forward with this, including a rollout procedure:

• get https://gerrit.wikimedia.org/r/441396 checked in ASAP, almost certainly with a bunch of TODOs from reviewers to be fixed in followups
• add functionality for showing diffs on dbctl config commit; also add a --dry-run
• get final feedback from DBAs on interface
• get the conftool schema files checked in to puppet
• populate conftool objects with the then-current state of prod
• either as a handler on noc.wikimedia.org, or as a password-protected Mediawiki handler, show diffs between what is returned from EtcdConfig vs what is live via the usual mediawiki-config files
• have an emergency procedure for persisting and restoring a particular version of the compiled config to etcd directly
• add some client-side data validation in EtcdConfig
• add an Icinga check for diffs between etcd's instance/section config vs the compiled version
• add an Icinga check for whatever client-side validation EtcdConfig does failing on the compiled config (likely per-appserver)
• write up runbooks for the above

Once all of the above is done:

• add a config flag that can be controlled by puppet hiera to use either mediawiki-config or EtcdConfig on a per-appserver basis; make those hosts easily depool-able
• add an Icinga check for differences between mediawiki-config and compiled etcd config
• start making all database changes both places, with just a few appservers using EtcdConfig
• after a validation period, switch over the whole fleet to use EtcdConfig
• delete the obsolete stanzas from db-eqiad.php and db-codfw.php

It would be nice to have a mockup of the API to test soon (with no production effect except maybe some debug information). That will allow to test automation from scripts we have already. I think that would be step #6 ?

delete db-eqiad.php and db-codfw.php

That is unlikely to happen, there is unrelated configuration there, or which is not dynamic or undesirable to be on etcd. 90% of it could probably go away indeed.

I believe I had a set of tests to check different failure scenarios (DROP, REJECT, large timeout, etc.)

In T197126#5177169, @jcrespo wrote:

It would be nice to have a mockup of the API to test soon (with no production effect except maybe some debug information). That will allow to test automation from scripts we have already. I think that would be step #6 ?

Yeah, that is the intent once the diffs handler on noc.wm.o or MW itself is present -- so everyone can try using the 'real' tool without affecting anything aside from that.

delete db-eqiad.php and db-codfw.php

That is unlikely to happen, there is unrelated configuration there, or which is not dynamic or undesirable to be on etcd. 90% of it could probably go away indeed.

Ah, yeah of course. Updated!

I believe I had a set of tests to check different failure scenarios (DROP, REJECT, large timeout, etc.)

Definitely curious to see whatever you have here :)

EtcdConfig in MediaWiki has been extensively tested against failures before it was introduced, if that's what @jcrespo was referring to.

As for the deployment, I'd rather reason per-wiki rather than per-appserver, if possible. Although I think probably releasing the new function just on the debug servers (with an if $hostname guard in MediaWiki config) is enough.

As far as merging the first patch, I would consider you a valid reviewer given the bulk of the code was written before you looked into it, and you fixed most of the issues in the meantime.

Change 441396 merged by jenkins-bot:
[operations/software/conftool@master] Add a WMF-specific tool for managing db config in MediaWiki

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

All patches for v1 of dbconfig are merged, including the ones to make a new conftool release. I'll rebuild the packages on Monday and upload them to our APT and I'll start testing the upgrade of python3-conftool (the base package, pretty much untouched).
The testing of python3-conftool-dbctl will follow after.

Mentioned in SAL (#wikimedia-operations) [2019-07-11T14:57:52Z] <cdanis> cdanis@cumin1001.eqiad.wmnet ~ % sudo debdeploy deploy -u T197126-2019-07-11-conftool.yaml -s mw-canary

Mentioned in SAL (#wikimedia-operations) [2019-07-11T15:05:26Z] <cdanis> cdanis@cumin1001.eqiad.wmnet ~ % sudo debdeploy deploy -u T197126-2019-07-11-conftool.yaml -s cp-canary

Mentioned in SAL (#wikimedia-operations) [2019-07-11T16:19:09Z] <cdanis> cdanis@cumin1001.eqiad.wmnet ~ % sudo debdeploy deploy -u T197126-2019-07-11-conftool.yaml -s ulsfo

Mentioned in SAL (#wikimedia-operations) [2019-07-11T16:47:57Z] <cdanis> cdanis@cumin1001.eqiad.wmnet ~ % sudo debdeploy deploy -u T197126-2019-07-11-conftool.yaml -s eqsin

Mentioned in SAL (#wikimedia-operations) [2019-07-11T17:02:21Z] <cdanis> cdanis@cumin1001.eqiad.wmnet ~ % sudo debdeploy deploy -u T197126-2019-07-11-conftool.yaml -s esams

Mentioned in SAL (#wikimedia-operations) [2019-07-11T17:37:35Z] <cdanis> cdanis@cumin1001.eqiad.wmnet ~ % sudo debdeploy deploy -u T197126-2019-07-11-conftool.yaml -s codfw

Mentioned in SAL (#wikimedia-operations) [2019-07-11T18:02:04Z] <cdanis> cdanis@cumin1001.eqiad.wmnet ~ % sudo debdeploy deploy -u T197126-2019-07-11-conftool.yaml -s eqiad

Change 523943 had a related patch set uploaded (by CDanis; owner: CDanis):
[operations/puppet@production] conftool: update schemata for dbctl

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

Change 523013 had a related patch set uploaded (by CDanis; owner: CDanis):
[operations/puppet@production] dbctl: monitor for uncommitted changes

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

Change 524551 had a related patch set uploaded (by CDanis; owner: CDanis):
[operations/mediawiki-config@master] noc: allow db.php?dc=codfw instead of just eqiad

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

Change 524551 merged by jenkins-bot:
[operations/mediawiki-config@master] noc: allow db.php?dc=codfw instead of just eqiad

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

Change 523943 merged by CDanis:
[operations/puppet@production] conftool: update schemata for dbctl

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

Change 523013 merged by CDanis:
[operations/puppet@production] dbctl: monitor for uncommitted changes

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

Change 527137 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[operations/mediawiki-config@master] noc: Add cross-dc navigation links to db.php footer

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

Is there a tracking task for the features we've discussed? (depooling x1/es hosts, comments on hosts, etc..)

Change 527451 had a related patch set uploaded (by Marostegui; owner: Marostegui):
[operations/puppet@production] dbctl_client.pp: Remove dbctl diff alerts

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

Change 527451 abandoned by Marostegui:
dbctl_client.pp: Remove dbctl diff alerts

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

Change 527137 merged by jenkins-bot:
[operations/mediawiki-config@master] noc: Add cross-dc navigation links to db.php footer

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