search

thias / puppet-sysctl

Puppet module to manage sysctl parameters
Other
35 stars 86 forks source link

purging /etc/sysctl.d #1

Closed linuxsquad closed 11 years ago

linuxsquad commented 11 years ago

1- Why do you need to purge /etc/sysctl.d? You can make it optional and keep existing files by default.

2- I would go with

file { "/etc/sysctl.d/60-${title}.conf":

vs.

file { "/etc/sysctl.d/${title}.conf":

per RHEL README recommendations

thias commented 11 years ago
  1. The purge is useful to avoid having to always be careful to "ensure => absent" a particular entry in order to have it removed. A typical case would be when you stop applying a class where a sysctl value was set from : It's better to have the change reverted automatically.
  2. Well, it would make sense... if the directory wasn't getting purged :-)

Does commit 737da8c4239b58a71dca37d374064816cec3aded fix this for you? With puppet 3+ you can just set the purge to false using hiera (though it might still be broken, I recall puppet 3.1.1's hiera has a problem with booleans, which will be fixed in a future release).

linuxsquad commented 11 years ago

well... 1- if you stop applying class with a sysctl, you still have to have at least one instance sysctl::base to ensure purging. Am I correct? 2- purging making sure sysctl value won't survive reboot, but meanwhile sysctl/variable remains setup 'till the next reboot.

thias commented 11 years ago
  1. Correct. And it's mentioned in the README. But the solution is as easy as making sure sysctl::base is included on all nodes.
  2. The problem is even more complicated than that : When you change a sysctl variable to a given value, you no longer know the previous value, nor the original default value (which could or could not be the same, depending on if there are intermediate changes). And this problem isn't puppet-specific.

Overall, I can't think of any perfect way of managing sysctl values. There would be the option of adding some sort of init script which would dump the initial values for all keys somewhere, to be later used in order to reverse no longer set values. That's overkill and still not perfect, as some keys don't exist until specific modules are loaded, and that can happen at any time after boot. The best I could think of for my use cases is what I've implemented in my module, obviously. Yes, I depend on a reboot to get back to an original value...

For your original observation of the missing numbered prefix in the file name, would you like me to add the feature to pass an optional prefix to the sysctl definition? The default would stay the same for backwards compatibility, but you could use "Sysctl { prefix => '60' }" to have all files prefixed with "60-", or pass the "prefix => 'xx'" when declaring each instance. For me, this isn't really useful, as it doesn't make much sense when all of these files are managed by puppet, since you know that none are trying to override the others, but it could make your life easier if you're using "purge => false" and mixing non puppet-managed files inside /etc/sysctl.d/.

linuxsquad commented 11 years ago

I will take on your offer to add "60-" prefix since I will be using this classes with "purge => false".

thias commented 11 years ago

If you could try commit 3c0a59ebc2720f67613b025dd8176a8fe575ad37 that would be great, as it should implement what you asked for, and might even work... it's 100% untested, as I'm away with limited Internet access, but I've been able to push these few changes. Please let me know the result!

justinclayton commented 11 years ago

:+1: for changing the default for purge to false. While it can be extremely useful and the class parameter should definitely be there, I think modules should default to being non-destructive and respecting vendor space on the filesystem. If, say, an rpm were to drop its own settings in the .d dir (which is a perfectly valid use of the .d facility), those settings wouldn't survive the next puppet run, and would likely cause confusion at best, and system issues at worst.

Puppetlabs has adopted this philosophy with their modules (such as puppetlabs/apache), and it seems like they are working towards this becoming a recommended best practice for module development.

By the way, great module!

thias commented 11 years ago

It looks like the push of the commit I mentioned previously never happened. It might still be on my laptop, but in doubt, I've re-implemented the change. Please look at 1c3d3f722371de4e37031d626de660dafc14b6f9 since that should do what we agreed on for the prefix. I've also released 0.2.0 to the forge with this change.