Tails

#1 Updated by groente 2018-12-07 17:05:18

• Tracker changed from Bug to Feature

#2 Updated by groente 2018-12-07 17:26:23

• Assignee changed from groente to intrigeri
• QA Check set to Ready for QA
• Feature Branch set to puppet-tails:feature/16202-create-resource-for-backups

this should take care of the initialisation of a backup repo and setting up a cronjob for it. the passphrase should be set in hiera. let me know what you think!

#3 Updated by intrigeri 2018-12-08 07:43:54

• Assignee changed from intrigeri to groente
• QA Check changed from Ready for QA to Dev Needed

> let me know what you think!

There we go!

I’ve pushed a couple nitpicking commits on top.

About the shell script:

• Regarding error handling, if guestmount fails, then we’re leaving a LVM snapshot around, which can cause all kinds of trouble further down the road. One solution would be to do trap cleanup EXIT earlier and make it support more failure modes (at least LVM snapshot set up but guestmount failed).
• $TMPDIR is a special variable that’s used by all kinds of programs if set. Is this really what we want, or do we merely need a temporary working directory? If the latter, then I’d rather see the variable renamed to avoid confusion.
• I think exit 0 on the last line is a no-op under set -e: either borg create succeeds and then we’ll return 0 anyway, or borg create fails and then set -e will abort the script and exit 0 won’t be executed.
• Apart of that, the remaining issues I’ve found “by hand” were also spotted by shellcheck, which I warmly recommend (I should get used to running it before I do the code review myself):
  • Please quote variables consistently (SC2086): that’s cheaper than reasoning about which one exactly is safe to leave unquoted or not and maintaining these pre-conditions over time.
  • The command line option parsing merely warns when invalid options are passed. I recommend adding a *) case to handle this better (SC2220).
  • Please use $() instead of backticks (SC2006).

About the Puppet code:

• I think you’re a little bit confused wrt. classes vs. defined resources vs. Hiera. We’re going to need this multiple times so it makes sense to have a defined resource for it; so far, so good. But we cannot use this defined resource multiple times in its current state, because it manages other unique resources such as /usr/local/sbin/runbackuplv.sh. So I think we need to move the common bits to a tails::borgbackup class, that should be managed only once: backup script, secrets file, packages. If you feel it’s worth it, we can even split the common borg bits in one class and the “borg for LVM” bits (runbackuplv.sh, libguestfs-tools) in another one. Once we have this, every defined resource should do include tails::borgbackup and then:
  • either we decide to use a single passphrase for all LV backups, in which case we can pass it to the class via Hiera, and the ::lv defined resource can look it up from the class parameters with $tails::borgbackup::borgpassphrase or similar
  • or we want more flexibility, in which case the passphrase does not belong to the class, and then in our manifests repo we have to use create_resources to declare the N instances of the ::lv defined resource with data coming from Hiera (there are alternatives but AFAICT they’re all seriously ugly and bad design)
  • in any case, use create_resources to declare the N instances of the ::lv defined resource with data coming from Hiera would be neat: adding/removing/updating the list of LVs we want to backup can then be done 100% in Hiera, without any Puppet code change; we don’t have to do that if we go the “single passphrase” road, but we have to if we want per-LV passphrase :)
• I’d rather see $borgpassphrase be made a mandatory parameter, to ensure we never, even, use this defined resource with a default passphrase.
• The name of the defined resource does not match the name of the file it’s in (spotted by puppet-lint; BTW, I believe all major editors support running it on-the-fly).
• In File { '/usr/local/sbin/runbackuplv.sh': and File { '/etc/borgbackup.secret':, I suspect you mean file instead of File. Same for Exec { "/usr/bin/borg init […].
• The cron resource lacks a dependency on File['/usr/local/sbin/runbackuplv.sh']
• The File['/usr/local/sbin/runbackuplv.sh'] resource lacks a dependency on the 2 packages.
• The Exec resource lacks a dependency on the borgbackup package.
• Who’s responsible for creating $basedir? Will borg create it automatically with the permissions we want? Maybe it would be better to manage it with Puppet.

#4 Updated by groente 2018-12-08 12:38:57

• Assignee changed from groente to intrigeri
• QA Check changed from Dev Needed to Ready for QA

> * Regarding error handling, if guestmount fails, then we’re leaving a LVM snapshot around, which can cause all kinds of trouble further down the road. One solution would be to do trap cleanup EXIT earlier and make it support more failure modes (at least LVM snapshot set up but guestmount failed).

okay, i’ve moved the trap upward and added some conditions for various stages of where things could’ve gone wrong in the cleanup function.

> * $TMPDIR is a special variable that’s used by all kinds of programs if set. Is this really what we want, or do we merely need a temporary working directory? If the latter, then I’d rather see the variable renamed to avoid confusion.

renamed!

> * I think exit 0 on the last line is a no-op under set -e: either borg create succeeds and then we’ll return 0 anyway, or borg create fails and then set -e will abort the script and exit 0 won’t be executed.

fair enough, removed.

> * Apart of that, the remaining issues I’ve found “by hand” were also spotted by shellcheck, which I warmly recommend (I should get used to running it before I do the code review myself):

aha, i didn’t know about shellcheck, but it seems happy now.

So I think we need to move the common bits to a tails::borgbackup class, that should be managed only once: backup script, secrets file, packages.

done.

> If you feel it’s worth it, we can even split the common borg bits in one class and the “borg for LVM” bits (runbackuplv.sh, libguestfs-tools) in another one.

maybe later if we actually need it…

> Once we have this, every defined resource should do include tails::borgbackup and then:
> either we decide to use a single passphrase for all LV backups, in which case we can pass it to the class via Hiera, and the ::lv defined resource can look it up from the class parameters with $tails::borgbackup::borgpassphrase or similar

since the backup cronscript requires the passphrase to be stored in a file, i’d rather have just one passphrase. having multiple does not add any extra security, since any attack vector on these passphrases will affect all of them equally (if one is lost, all are lost, so might aswell just have one).

> * I’d rather see $borgpassphrase be made a mandatory parameter, to ensure we never, even, use this defined resource with a default passphrase.

done.

> * The name of the defined resource does not match the name of the file it’s in (spotted by puppet-lint; BTW, I believe all major editors support running it on-the-fly).

fixed.

> * In File { '/usr/local/sbin/runbackuplv.sh': and File { '/etc/borgbackup.secret':, I suspect you mean file instead of File. Same for Exec { "/usr/bin/borg init […].

fixed.

> * The cron resource lacks a dependency on File['/usr/local/sbin/runbackuplv.sh']
> * The File['/usr/local/sbin/runbackuplv.sh'] resource lacks a dependency on the 2 packages.
> * The Exec resource lacks a dependency on the borgbackup package.

fixed.

> * Who’s responsible for creating $basedir? Will borg create it automatically with the permissions we want? Maybe it would be better to manage it with Puppet.

stone is responsible for creating $basedir, it’s in the stone.pp manifest.

#5 Updated by intrigeri 2018-12-08 17:52:22

• Assignee changed from intrigeri to groente
• QA Check changed from Ready for QA to Dev Needed

> okay, i’ve moved the trap upward and added some conditions for various stages of where things could’ve gone wrong in the cleanup function.

Great!

Just one thing: /bin/mount |grep -q "${MOUNTDIR}" is a somewhat vague approximation of what mountpoint --quiet does correctly, so please use that one instead.

>> * Who’s responsible for creating $basedir? Will borg create it automatically with the permissions we want? Maybe it would be better to manage it with Puppet.

> stone is responsible for creating $basedir, it’s in the stone.pp manifest.

Ah right, that’s the destination base directory, I was confused :)

Other than that, all good, good fixes! I’ve pushed one small commit on top (I wrote my previous comments about missing dependencies before I realized we needed a class).

#6 Updated by groente 2018-12-08 18:27:52

• Assignee changed from groente to intrigeri
• QA Check changed from Dev Needed to Ready for QA

> Just one thing: /bin/mount |grep -q "${MOUNTDIR}" is a somewhat vague approximation of what mountpoint --quiet does correctly, so please use that one instead.

aha, TIL. fixed!

> Other than that, all good, good fixes! I’ve pushed one small commit on top (I wrote my previous comments about missing dependencies before I realized we needed a class).

\o/ ready to merge now?

#7 Updated by intrigeri 2018-12-08 19:57:03

• Status changed from Confirmed to Resolved

>> Just one thing: /bin/mount |grep -q "${MOUNTDIR}" is a somewhat vague approximation of what mountpoint --quiet does correctly, so please use that one instead.

> aha, TIL. fixed!

:)

>> Other than that, all good, good fixes! I’ve pushed one small commit on top (I wrote my previous comments about missing dependencies before I realized we needed a class).

> \o/ ready to merge now?

Merged!

#8 Updated by intrigeri 2018-12-09 07:29:12

• Assignee deleted (intrigeri)
• % Done changed from 0 to 100
• QA Check changed from Ready for QA to Pass

#9 Updated by groente 2018-12-09 13:15:16

• Status changed from Resolved to In Progress
• % Done changed from 100 to 80

actually, we need a second resource for logical volumes that serve as disks which are directly mounted inside the vm.

the only difference with the existing ::lv resource is that this resource will create a backup cronjob that uses mount instead of guestmount.

#10 Updated by intrigeri 2018-12-09 13:25:15

• Assignee set to groente

OK!

#11 Updated by groente 2018-12-09 13:44:48

• Assignee changed from groente to intrigeri
• QA Check changed from Pass to Ready for QA

added a parameter to the resource to handle backups of logical volumes that have a filesystem directly on them, instead of going through lvm.

#12 Updated by intrigeri 2018-12-09 15:44:50

• Assignee changed from intrigeri to groente

> added a parameter to the resource to handle backups of logical volumes that have a filesystem directly on them, instead of going through lvm.

LGTM modulo I’ve removed the code duplication and pointed you (in the commit message) to the relevant Puppet doc, as part of the great “groente learns Puppet” journey.
Feel free to merge if happy with it.

#13 Updated by groente 2018-12-09 17:24:25

• Status changed from In Progress to Resolved
• Assignee deleted (groente)
• % Done changed from 80 to 100
• QA Check deleted (Ready for QA)