thias / puppet-sysctl

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

Fix spaces in output of sysctl -n <key> #42

Closed ghost closed 6 years ago

thias commented 8 years ago

I think I see what you're doing here, but I also think that it fixes some cases and breaks others. See #40 for some details. Basically, we would need to mangle both the value we are trying to set/compare and the value the system reports if we want to avoid breakage.

This is what my module was doing in the past, but it also had its own downsides : The code is more complex and more fragile, and AFAICT some sysctl values could contain multiple spaces on purpose (such as kernel.core_pattern), though in practice it might not be relevant...

For me it makes no doubt that the proper solution is to make sure the values being set are exactly the same as the ones sysctl reports, this means using tabs as-is. This is up to the users of the module. The real problem is that people have been used to setting mangled values, such as grouping kernel.sem with spaces instead of tabs...

Would a clear mention of this in the README be enough, perhaps?

ghost commented 8 years ago

\s includes [ \t\r\n\f]. So it should not break others.

yes you are right that both value return by the command and the one provided should be parsed or provide the same that sysctl reports. I started to look at parsing the value provided to match the one parsed with sed and i haven't finished.

ghost commented 8 years ago

i have added parsing of the value provided. Let me know what you think.

ghost commented 8 years ago

@thias que penses-tu du dernier changement?

ghost commented 8 years ago

@thias what do you think?

dantremblay commented 8 years ago

@thias please let us know what you think of this change. We could use this in our environment. Thanks!

nbentoumi commented 8 years ago

:+1:

pckls commented 8 years ago

@juliengk I was about to hack something up to solve this very issue.

One thing I've noticed from testing in a shell is that there is a difference between single vs double quotes for those sed statements - http://stackoverflow.com/questions/6697753/difference-between-single-and-double-quotes-in-bash

When I use /sbin/sysctl -n net.ipv4.tcp_wmem | sed -r 's/\\s+/ /g' the tabs are still there however /sbin/sysctl -n net.ipv4.tcp_wmem | sed -r "s/\\s+/ /g" works, I assume because it's not interpreting the backslash as a literal?

pckls commented 8 years ago

I've decided to just pass in my Hiera variables with the tabs because I think it's the cleanest solution.

benohara commented 8 years ago

I see this after an upgrade...this looks to fix it for me

-          unless  => "/usr/bin/test \"$(/sbin/sysctl -n ${qtitle})\" = ${qvalue}",
+          unless  => "/usr/bin/test \"$(/sbin/sysctl -n ${qtitle} | /bin/sed 's/\t/ /g')\" = ${qvalue}",
bschonec commented 7 years ago

I can confirm that @benohara code change made {my} runs idempotent.