+1 for creating a deb. I can give you an introduction on how to do that if you want.

So my initial suggestion was to create a debian package for the following reasons:

• Source control patches on a separate repo so upgrades are easy (giiven the package is mostly a patched upstream source with extra patches that fix our bugs or add new functionality)
• Complexity can be hidden on the .deb and puppet only has to do require_package(new package)
• Avoid ugly puppet code handling the systemd unit and other things

+1 to create the package too, I like the idea of having the "patched" versions of pt-XXX in our own package.

In T203674#4563061, @jcrespo wrote:

So my initial suggestion was to create a debian package for the following reasons:

• Source control patches on a separate repo so upgrades are easy (giiven the package is mostly a patched upstream source with extra patches that fix our bugs or add new functionality)

This is not true if a binary debian package is built, as proposed. In fact, you can consider a binary-only package (built with dpkg-deb) a glorified tarball. I don't see any advantage in using it over puppet, and a loss of standardization.

• Complexity can be hidden on the .deb and puppet only has to do require_package(new package)
• Avoid ugly puppet code handling the systemd unit and other things

The puppet code to handle systemd::service will be needed anyways, the only difference being the unit file will be bundled in said archive

I would also prefer a deb over slapping the script in puppet if:

• we do a proper debian package
• we maintain our patches to upstream with quilt (in debian/patches)

else I see no advantage, and quite some disadvantages (obscurity, non-conformity to how we do things) to this approach.

This is not true if a binary debian package is built, as proposed. In fact, you can consider a binary-only package (built with dpkg-deb) a glorified tarball.

I don't see how? I said to banyek that if a .deb was decided, we would source-control it in a new repo on operations/software/package name or operation/debs/package name (e.g. like we do with pybal). Leaving intact the upstream control, and pushing on top our changes. Doing that on puppet source control is quite messing- do you want to import all upsteam changes on our puppet? Patch changes? Separate there is a better control.

The puppet code to handle systemd::service will be needed anyways

I don't see how that will be needed, unless non-standard changes are needed. Have you heard or post-inst (which is what every debian package does)? XD

we do a proper debian package
we maintain our patches to upstream with quilt (in debian/patches)

Why would you assume we won't be doing that?

else I see no advantage

I can see a lot of disadvantages of using puppet- while the visibility may be reduced of the changes (something solved by a good changelog+reprepro logs)- adding pure software (beyond "glue scripts") to puppet version control is (in my opinion) bad. I've been in the past reprehended (rightly) for doing this with pt-heartbeat, backup generation, etc., so I am showing banyek what I was told it was the "right way" CC @Volans @fgiunchedi @faidon @MoritzMuehlenhoff

In any case, let's bring the discussion outside of this ticket, as I think this is could be a core disagreement about software deployment in our infrsatructure that would be worth discussing on a separate medium (but let's not spook banyek here :-D).

In T203674#4565565, @jcrespo wrote:

This is not true if a binary debian package is built, as proposed. In fact, you can consider a binary-only package (built with dpkg-deb) a glorified tarball.

I don't see how? I said to banyek that if a .deb was decided, we would source-control it in a new repo on operations/software/package name or operation/debs/package name (e.g. like we do with pybal). Leaving intact the upstream control, and pushing on top our changes. Doing that on puppet source control is quite messing- do you want to import all upsteam changes on our puppet? Patch changes? Separate there is a better control.

If you build a debian package with dpkg-deb, your patch will not be separated from the source; you are basically building a tar.gz with an added script to be executed on extraction this way.

The puppet code to handle systemd::service will be needed anyways

I don't see how that will be needed, unless non-standard changes are needed. Have you heard or post-inst (which is what every debian package does)? XD

Yes, I did hear about post-inst, quite incredibly; still you need to declare the service in puppet, because you want to ensure that that service is running on your system, so you need to declare it in puppet. You can just use service { "whatever"": ensure => running} instead of doing systemd::service { 'whatever': ensure => present, content => <your-service-unit> }, because your service unit will be maintained by a different source.

we do a proper debian package
we maintain our patches to upstream with quilt (in debian/patches)

Why would you assume we won't be doing that?

Because of what @Banyek showed me the other day when I told him to start this discussion was a debian package built with dpkg-deb. That is not what people assume would be done when building a debian package. It is actively considered deprecated as a method to build debian packages, even.

Of course, it requires slightly less work than building a proper debian package - an option I would probably prefer overall.

else I see no advantage

I can see a lot of disadvantages of using puppet- while the visibility may be reduced of the changes (something solved by a good changelog+reprepro logs)- adding pure software (beyond "glue scripts") to puppet version control is (in my opinion) bad. I've been in the past reprehended (rightly) for doing this with pt-heartbeat, backup generation, etc., so I am showing banyek what I was told it was the "right way" CC @Volans @fgiunchedi @faidon @MoritzMuehlenhoff

In any case, let's bring the discussion outside of this ticket, as I think this is could be a core disagreement about software deployment in our infrsatructure that would be worth discussing on a separate medium (but let's not spook banyek here :-D).

There is really no disagreement; I would prefer a proper debian package to be built, but my order of preference is:

1 - Proper debian package (following the rules for perl debian packaging, with our patches to the upstream code maintained as patches in the repository, etc)
2 - Slap all in puppet
3 - debian package built with dpkg-deb

because the latter has, in my opinion, only disadvantages compared to any other solution.

There is really no disagreement; I would prefer a proper debian package to be built, but my order of preference is:

1 - Proper debian package (following the rules for perl debian packaging, with our patches to the upstream code maintained as patches in the repository, etc)
2 - Slap all in puppet
3 - debian package built with dpkg-deb

because the latter has, in my opinion, only disadvantages compared to any other solution.

So we are actually agreeing, if he showed you a .deb with that, that is basically a blob/black box and not ok- note I didn't ask him to do that- but we should be understanding that new hires may not know that- I literally said to him we would ask to create a new repo to source control a debian source package files, in the format you suggest, and then use our production build workflow. :-) Moritz offered to help him do one "the right way"- Let's do that, ok? :-D

Indeed, let's go with a "proper" Debian package, imho the cleanest way to go and conforming to how we do things.

Hopefully Percona also will fix the pt-kill bug in a definitive way (there have been two attempts T183983#3983838 T183983#4145153 T183983#4267429 ) so we'll not need to use our own patched version.

But we'll still need the service definition after - which reads the config file

The debian package is done, it's repository can be found on https://gerrit.wikimedia.org/r/#/admin/projects/operations/debs/wmf-pt-kill

I need the cloud people approval on https://gerrit.wikimedia.org/r/#/c/operations/puppet/+/458810/ and then we can put this on production

You probably want to add @Bstorm and @Andrew to review that Gerrit patch.

The change will be merged on 2018-10-01 (Monday) morning.
First only on one of the hosts (I recommend to pick one of the 'wikireplica_web' role hosts. Let's say labsdb1010)

https://gerrit.wikimedia.org/r/#/c/operations/puppet/+/458810/

The change is out, the debian package works as expected, but the template generation had a typo which was fixed in https://gerrit.wikimedia.org/r/#/c/operations/puppet/+/463712/
However The wmf-pt-kill service not works as expected: it doesn't kills the running queries, but if I copy the exact same command from the output of 'ps' it works.
The temporary solution is that the packages are installed but the systemd service is kept disabled, so the pt-kill still runs from screen.
Also the 'log' option is not working not from command line nor from systemd service.

I did some debug, and found solution for both of the problems:

wmf-pt-kill not killing queries:

• The config file contains the MATCH_USER and MATCH_COMMAND variables double quoted which is extracted good on the shell, but when started via systemd the MATCH_USER variable wont be "^[spu][0-9]" but "\'^[spu][0-9]\'" and the same pattern applies to MATCH_COMMAND instead of "Query|Execute" it becomes "\'Query|Execute\'"

Fixing this we only need to make a small change in the .erb template for the config file

Log file doesn't get written to /var/log/wmf-pt-kill.log

• The systemd service we use in a single type service, and keeps wmf-pt-kill to run in foreground, but the --log option only applies if we add the --daemonize cli option to wmf-pt-kill. In this case we have to change the service type to forking. This needs the debian package to rebuild. Until we don't do that the logs of wmf-pt-kill accessible via journalctl -u.

Hopefully tomorrow I can deploy them both.

This needs the debian package to rebuild.

Please setup a specific system user (with post-inst or puppet) to run the script and give a passwordless login and an account with the appropiate grants of the same name to mysql.

Change 463900 had a related patch set uploaded (by Banyek; owner: Banyek):
[operations/puppet@production] wikireplicas: config file changed for wmf-pt-kill

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

Change 463900 merged by Banyek:
[operations/puppet@production] wikireplicas: config file changed for wmf-pt-kill

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

In T203674#4632896, @jcrespo wrote:

This needs the debian package to rebuild.

Please setup a specific system user (with post-inst or puppet) to run the script and give a passwordless login and an account with the appropiate grants of the same name to mysql.

ok

Just to be clear, passwordless here means "using socket authentication", not literally passwordless :-)

Mentioned in SAL (#wikimedia-operations) [2018-10-05T09:37:11Z] <banyek> disabling puppet on labsdb1009,labsdb1010,labsdb1011 (T203674)

Change 464781 had a related patch set uploaded (by Banyek; owner: Banyek):
[operations/puppet@production] wikirepicas: wmf-pt-kill template data only from hiera

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

Change 464781 merged by Banyek:
[operations/puppet@production] wikirepicas: wmf-pt-kill template data only from hiera

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

Change 464821 had a related patch set uploaded (by Banyek; owner: Banyek):
[operations/puppet@production] wikireplicas: enable wmf-pt-kill service

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

In T203674#4645494, @gerritbot wrote:

Change 464821 had a related patch set uploaded (by Banyek; owner: Banyek):
[operations/puppet@production] wikireplicas: enable wmf-pt-kill service

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

Merging this patch will turn on the wmf-pt-kill service.
Before merging that, puppet has to be disabled on the labsdb hosts.
After merging, on one host the pt-kill running in screen should be stopped, then puppet can be enabled and run puppet agent -tv
If everything goes well, the on other two hosts has to be the pt-kill stopped, and then the puppet can be enabled

In T203674#4646056, @Banyek wrote:

After merging, on one host the pt-kill running in screen should be stopped, then puppet can be enabled and run puppet agent -tv

FYI we usually don't run the puppet agent directly, but always through a custom small wrapper: run-puppet-agent

Change 464821 merged by Banyek:
[operations/puppet@production] wikireplicas: enable wmf-pt-kill service

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

Mentioned in SAL (#wikimedia-operations) [2018-10-08T12:49:48Z] <banyek> pt-kill-wmf enabled on the wikireplicas (T203674)

this ticket is closed but more follow-up will appear in T206521