Project

General

Profile

Feature #14600

Periodically run git gc in /etc

Added by groente about 1 month ago. Updated 18 days ago.

Status:
In Progress
Priority:
Normal
Assignee:
Category:
Infrastructure
Target version:
Start date:
09/04/2017
Due date:
% Done:

40%

QA Check:
Dev Needed
Feature Branch:
puppet-etckeeper:feature/14600-periodically-run-git-gc
Type of work:
Sysadmin
Blueprint:
Easy:
Affected tool:

Description

puppet-git was running out of disk space and was wasting >600MB in /etc/.git
git gc should periodically be run wherever git is being used in some automated manner.


Related issues

Blocks Tails - Feature #13242: Core work 2017Q4: Sysadmin (Maintain our already existing services) Confirmed 06/29/2017

History

#1 Updated by intrigeri about 1 month ago

git gc should periodically be run wherever git is being used in some automated manner.

Good catch!

#2 Updated by groente about 1 month ago

  • Blocks Feature #13242: Core work 2017Q4: Sysadmin (Maintain our already existing services) added

#3 Updated by groente about 1 month ago

  • Status changed from New to Confirmed

#4 Updated by groente 20 days ago

  • Status changed from Confirmed to In Progress
  • Assignee changed from groente to intrigeri
  • % Done changed from 0 to 20
  • QA Check set to Ready for QA
  • Feature Branch set to feature/14600-periodically-run-git-gc

Hey, please check commit f18e2eca900119ed65cc6cb16f5206518707ba5e in puppet-tails. I'm not sure if this is the appropriate place to include this cron entry, suggestions are welcome.

#5 Updated by intrigeri 20 days ago

  • Feature Branch changed from feature/14600-periodically-run-git-gc to puppet-tails:feature/14600-periodically-run-git-gc

#6 Updated by intrigeri 20 days ago

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

Hey, please check commit f18e2eca900119ed65cc6cb16f5206518707ba5e in puppet-tails. I'm not sure if this is the appropriate place to include this cron entry, suggestions are welcome.

Indeed, this feels out of place for two reasons:

  • in the "Include external classes" section;
  • in the middle of a piece of code that's about configuring the Puppet master/agent.

Also, I wonder why we would do that on all systems except isotesters and puppetmasters (especially since this ticket was created about problems on our puppetmaster). This feels weird, to say the least.

Either move your added code below, outside of the "Include external classes" section, e.g. somewhere in the "Miscellaneous settings" section. That's the cheap option that only benefits systems managed by tails::base. The best option would be to add this to the etckeeper module instead so everyone who uses it benefit from our improvements (that arguably are not Tails-specific). Feel free to pick whichever you want: the 1st option is much easier, the 2nd will allow you to learn a fair bit about Puppet :)

#7 Updated by groente 20 days ago

  • Assignee changed from groente to intrigeri
  • % Done changed from 20 to 30
  • QA Check changed from Dev Needed to Ready for QA
  • Feature Branch changed from puppet-tails:feature/14600-periodically-run-git-gc to puppet-etckeeper:feature/14600-periodically-run-git-gc

Ok, so I removed the branch in puppet-tails and instead added the cronjob in the etckeeper module. Can you review commit d5907f86cdf0f24be06292db7b770124244f88e1 ? Thanks!

#8 Updated by intrigeri 20 days ago

  • Assignee changed from intrigeri to groente
  • % Done changed from 30 to 50
  • QA Check changed from Ready for QA to Dev Needed

Looks better! I have a few comments:

  • Please tidy the added code (puppet-lint --fix does wonders :)
  • I think we should pass --auto to git gc to avoid spending resources (CPU, RAM) unnecessarily.
  • I think we should pass --quiet to git gc, otherwise we'll get tons of daily email from cron.

#9 Updated by groente 19 days ago

  • Assignee changed from groente to intrigeri
  • % Done changed from 50 to 30
  • QA Check changed from Dev Needed to Ready for QA

See commit ad83063f59bb329dc8e62125385d6bf474074c9a

#10 Updated by intrigeri 18 days ago

  • Assignee changed from intrigeri to groente
  • % Done changed from 30 to 40
  • QA Check changed from Ready for QA to Dev Needed

Much better (although please do atomic commits next time).

  • Please move the added section at the end of the class: granted, order does not matter in Puppet, but this is rather auxiliary so it feels weird to have it listed first.
  • This code should converge just fine but there's a race condition when applying this class for the first time. Let's fix it by adding a dependency from the cronjob on Package['etckeeper'] so we have a guarantee that the cronjob cannot run before a Git repo has been initialized.
  • On hosts with lots of VMs that all include this class, this will create a possibly big spike of I/O and CPU load because all the git gc will run at the same time => please use https://docs.puppet.com/puppet/3.8/function.html#fqdnrand

I think that will be all but it often happens that one (at least I) notices more issues only once the previous ones have been fixed.

Also available in: Atom PDF