Tails

#2 Updated by groente 2017-09-07 13:38:04

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

#3 Updated by groente 2017-09-07 13:40:56

• Status changed from New to Confirmed

#4 Updated by groente 2017-09-29 13:46:13

• 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 2017-09-29 15:42:34

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

#6 Updated by intrigeri 2017-09-29 15:50:53

• 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 2017-09-29 17:42:29

• 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 2017-09-30 06:05:11

• 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 2017-09-30 14:15:13

• 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 2017-10-01 06:53:51

• 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.

#11 Updated by anonym 2017-11-15 11:30:57

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

#12 Updated by groente 2018-01-04 20:47:26

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

like this? (commit 0096809f185730d0d850047380a7ae4183e31a21)

#13 Updated by intrigeri 2018-01-05 08:50:25

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

Yes, much better. We’re almost there. The only remaining change I’d like to see is passing the resource name as the seed to fqdn_rand(): otherwise, all cronjob scheduled on a given node with minute => fqdn_rand(60) and hour => fqdn_rand(24) run at the same time. My understanding is that fqdn_rand(60, 'cleanup-git-in-etc') (+ same for the hour attribute) should avoid that. Feel free to merge, deploy and test with this change.

#14 Updated by groente 2018-01-05 17:49:08

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

done

#15 Updated by intrigeri 2018-01-05 18:19:49

• Status changed from Resolved to In Progress

> done

I think you’ve merged into puppet-etckeeper, but the parent manifests repo still references the old version of the (sub)module.

#16 Updated by groente 2018-01-05 19:03:15

• Status changed from In Progress to Resolved

derp. now then.