Tails

#1 Updated by groente 2017-08-04 14:28:11

• Target version changed from Tails_3.3 to Tails_3.2

#2 Updated by intrigeri 2017-08-05 12:35:38

• blocks Feature #13233: Core work 2017Q3: Sysadmin (Maintain our already existing services) added

#3 Updated by intrigeri 2017-08-05 12:36:59

• related to Bug #13526: apt-snapshots partition lacks disk space added

#4 Updated by intrigeri 2017-08-05 12:39:09

• Status changed from New to In Progress
• Feature Branch set to puppet-tails:bugfix/13526-cleanup-apt-snapshots-tmpdir

I’ve developed this piece of our infra so I’d like to review this proposed change myself. So bertagaz, please reassign to me for QA once you’re happy with it and before deploying. Thanks!

#5 Updated by bertagaz 2017-08-12 13:28:15

• Assignee changed from bertagaz to groente
• QA Check changed from Ready for QA to Info Needed

groente wrote:
> https://git-tails.immerda.ch/puppet-tails/commit/?h=bugfix/13526-cleanup-apt-snapshots-tmpdir&id=5673109364ad0f973b01598be7e14748270b3f7e should help with garbage collection and maintaining enough free space.

“Invalid branch: bugfix/13526-cleanup-apt-snapshots-tmpdir” it says, and indeed I can’t find it. Did you push it?

#6 Updated by bertagaz 2017-08-13 11:38:16

• Assignee changed from groente to intrigeri
• % Done changed from 20 to 30
• QA Check changed from Info Needed to Ready for QA
• Feature Branch changed from puppet-tails:bugfix/13526-cleanup-apt-snapshots-tmpdir to immerda/puppet-tails:bugfix/13526-cleanup-apt-snapshots-tmpdir

bertagaz wrote:
> groente wrote:
> > https://git-tails.immerda.ch/puppet-tails/commit/?h=bugfix/13526-cleanup-apt-snapshots-tmpdir&id=5673109364ad0f973b01598be7e14748270b3f7e should help with garbage collection and maintaining enough free space.
>
> “Invalid branch: bugfix/13526-cleanup-apt-snapshots-tmpdir” it says, and indeed I can’t find it. Did you push it?

Ah, meanwhile the branch/commit appeared. Also you pushed on the immerda repo, while the “gitolite@git.puppet.tails.boum.org:puppet-tails” repo is the canonical one. Maybe me and the sysadmin doc have been unclear on that. Yeah, that’s a bit confusing. :)

So that’s a simple cronjob, I think it should be ok like that, passing the review to intrigeri as asked to.

#7 Updated by intrigeri 2017-08-13 15:10:39

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

> Also you pushed on the immerda repo, while the “gitolite@git.puppet.tails.boum.org:puppet-tails” repo is the canonical one.

I don’t think we care about this for topic branches: as long as it’s clear where the branch to review lives, it could even be on GitHub or wherever.

> So that’s a simple cronjob, I think it should be ok like that, passing the review to intrigeri as asked to.

The proposed approach is too risky IMO: it can delete files that another operation uses them, which can cause all kinds of breakage. I think that evaluating this risk would require more work than switching to a less risky approach, e.g. cleaning up as part of / immediately after whatever operation uses temporary files in that directory. This whole thing is kinda my department so I feel responsible for implementing the approach I’m suggesting myself, if you prefer not to :)

The problem is: I could not find out what operation uses /srv/apt-snapshots/time-based/tmp. groente, do you remember what kind of files/filenames you found there on Bug #13526? Having an idea of what their mtime was would be particularly useful, but I guess it’s hard to remember.

Once we know this, I would like to fix said operation to use a per-repository temporary directory instead of a per-user one: we use the reprepro-time-based-snapshots for all repositories (debian, torproject, etc.) so if they all use /srv/apt-snapshots/time-based/tmp we can’t clean it up in a non-racy / non-risky way.

OTOH I wonder if the files you had to delete on Bug #13526 were created by regular operations, or by some manual operation performed during the initial deployment, that I’ve forgotten to clean up. If the latter, then indeed a “clean up periodically” approach is OK! And then let’s do it with a higher-level, more declarative approach (tmpfiles.d(5)) instead of re-implementing this wheel ourselves over and over again :)

#8 Updated by groente 2017-08-18 15:15:56

• Assignee changed from groente to intrigeri

I don’t remember the filenames, sorry. If memory serves me well, they were dated from last december (13th?) and june.

That said, I think operations that still need their tmp files after a week should actually be breaking, it’s quite the indicator that something is going pretty wrong. If you haven’t reached the finish after a week, it’s hardly a race condition anymore, now is it?

Eitherway, I could adjust the cronjob to report (instead of delete) when tmpfiles older than a week have been found. That would allow us to investigate and based on that, we can decide what to do next?

#9 Updated by intrigeri 2017-08-29 06:35:58

• Assignee changed from intrigeri to groente
• QA Check changed from Info Needed to Dev Needed

> I don’t remember the filenames, sorry. If memory serves me well, they were dated from last december (13th?) and june.

OK.

> That said, I think operations that still need their tmp files after a week should actually be breaking, it’s quite the indicator that something is going pretty wrong. If you haven’t reached the finish after a week, it’s hardly a race condition anymore, now is it?

I do agree. Sorry, I had read too fast and missed the -ctime +7 part initially :/

Next step is thus “let’s do it with a higher-level, more declarative approach (tmpfiles.d(5)) instead of re-implementing this wheel ourselves over and over again”.
Happy to answer any Puppet question you might have when implementing this!

#10 Updated by groente 2017-09-03 16:12:36

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

Please verify this is what you had in mind. For now I’ve only configured /srv/apt-snapshots/time-based/tmp/ , but perhaps this would be a good occasion to do the other tmp dirs aswell?

#11 Updated by intrigeri 2017-09-04 10:16:59

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

groente wrote:
> Please verify this is what you had in mind.

What it “this”? I see no change to the topic branch referenced by this ticket.

> For now I’ve only configured /srv/apt-snapshots/time-based/tmp/ , but perhaps this would be a good occasion to do the other tmp dirs aswell?

Which other tmp dirs, for example?

#12 Updated by groente 2017-09-04 10:41:25

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

Apologies, seems i forgot to push the commit. It’s commit b7b9ed1e1be4de8e50426f53e6e70c216b50117c.
As for the other tmp dirs, /tmp springs to mind ;-)

#14 Updated by intrigeri 2017-09-04 14:59:53

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

#15 Updated by groente 2017-09-05 13:28:25

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

https://git-tails.immerda.ch/puppet-tails/commit/?h=bugfix/13526-cleanup-apt-snapshots-tmpdir&id=b7b9ed1e1be4de8e50426f53e6e70c216b50117c

#16 Updated by intrigeri 2017-09-05 15:37:20

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

Hi!

Looks good! A few comments:

• apt-snapshots.conf is too generic a name for something managed by the tails::reprepro::snapshots::time_based class: we have other kinds of snapshots, managed by other classes, so let’s share that common namespace in a nicer way. I suggest time-based-apt-snapshots.conf.
• Please reference the ticket this work is about in the commit message (e.g. with (refs: <del><a class='issue tracker-2 status-3 priority-4 priority-default closed child' href='/code/issues/13579' title='Cron script cleaning up the time-based apt-snapshots tmp dir'>Feature #13579</a></del>) as we do in other repos). In my experience, this info is very useful when trying to reverse engineer a piece of infra years later, and when it’s missing when you need it, then it can quickly become very irritating :)

With these 2 issues fixed, feel free to merge yourself into the master branch of puppet-tails in the official repo (not in that mirror one that gets overwritten all the time by mirroring).

And then we’ll still need to deploy these nice improvements to production. Did you already learn how to update a submodule in our manifests repo and deploy the resulting Git tree? If not, let’s do that together on XMPP later this week!

#18 Updated by groente 2017-09-22 12:38:37

• Status changed from In Progress to Resolved
• % Done changed from 30 to 100
• QA Check changed from Dev Needed to Pass

Merged and deployed!