saltstack / salt

Software to automate the management and configuration of any infrastructure or application at scale. Get access to the Salt software package repository here:
https://repo.saltproject.io/
Apache License 2.0
14.14k stars 5.47k forks source link

Support for Augeas lenses #7610

Closed pkruithof closed 10 years ago

pkruithof commented 11 years ago

Lenses make it easier to author and validate changes to a config file. It handles specific conversion between Augeas data and the configuration tree.

Unfortunately I don't really get how they are used. I know Puppet supports it, but I can't really figure out how.

For example, this does not work:

redis_conf:
  augeas.setvalue:
    - prefix: /files/etc/redis/redis.conf
    - changes:
      - bind: 0.0.0.0

In Puppet, we had to 'educate' Augeas and tell it that this file has the Spacevars format.

augeas { 'redis-conf':
  lens    => 'Spacevars.simple_lns',
  incl    => '/etc/redis/redis.conf',
  changes => [
    "set bind 0.0.0.0"
  ]
}
terminalmage commented 11 years ago

I'm not really familiar with how to implement this, so it'll take some research. I've marked this as "Approved for future release".

t184256 commented 10 years ago

I don't know much about that, but would something like:

redis_conf:
  augeas.setvalue:
    - changes:
      - /augeas/load/Xml/lens: Xml.lns
      - /augeas/load/Xml/incl: /etc/xdg/openbox/rc.xml
      - load: ''
      - /files/etc/redis/redis.conf/bind: 0.0.0.0

if the order of the changes was fixed?..

pkruithof commented 10 years ago

Good news, I think I got this working! This Augeas wiki page describes how you can load a specific file/lens, like @t184256 described. Couple of things I want to discuss before working on a PR:

Definition change

The only definition currently supported in the state is setvalue. However Augeas has more commands that can modify a config, such as rm, mv and ins. Therefore I suggest that setvalue gets deprecated in favor of a new commands definition, which adds the lens support, and changes the prefix argument to context, as it seems more semantical (and in line with Puppet). The introduction of a new definition keeps backwards compatibility with the current setvalue command.

The definition would look like this:

redis_conf:
  augeas.commands:
    - lens: Redis.lns
    - context: /files/etc/redis/redis.conf
    - changes:
      - set bind 0.0.0.0

More complex paths are matched properly, whitespace in values can be done using quotes (single or double), and commands can be combined, like this

zabbix-service:
  augeas.commands:
    - lens: Services.lns
    - context: /files/etc/services/
    - changes:
      - ins service-name after service-name[last()]
      - set service-name[last()] zabbix-agent
      - set service-name[. = 'zabbix-agent']/#comment "Zabbix Agent service"
      - set service-name[. = 'zabbix-agent']/port 10050
      - set service-name[. = 'zabbix-agent']/protocol tcp
      - rm service-name[. = 'im-obsolete']

I'm not 100% sure about the commands name, if anyone has a better suggestion I'd like to hear it.

Performance

I think I understand lenses a little bit better now, and it seems that normally Augeas autoloads all configured lenses and associated files with it. That's a lot of wasted CPU cycles and memory since you're only changing one file at a time with this state.

Take a look at the time for autoload vs noautoload:

$ time echo "quit" | augtool

real    0m0.933s
user    0m0.871s
sys 0m0.036s

$ time echo "quit" | augtool --noautoload

real    0m0.004s
user    0m0.000s
sys 0m0.004s

That's about 1 second less, keep in mind that for every Augeas call this happens, so it can easily add up!

Idempotence

As described in #7175 the state now always executes, regardless of changes. It's not really doable to check this automatically, since that would require evaluating all the commands against the current file (which is basically Augeas' job). However the match function can be included in the state so it can be used in an unless and/or onlyif section, which would be really useful I think.

1976 discusses the addition of these keys to all states. I'm inclined to add it separately to this state as that issue is still unresolved after 2 years. I'd appreciate feedback on this as well.

pkruithof commented 10 years ago

ping @terminalmage @basepi

terminalmage commented 10 years ago

As I've already mentioned, I'm not familiar with augeas.

pkruithof commented 10 years ago

I know, I was asking for some feedback on more general issues in my previous comment. Do you have any thoughts on that?

basepi commented 10 years ago

Well, we now have unless and onlyif in all states! (#13032)

As far as the rest of your questions go, I know nothing about augeas, so it sounds "fine" to me. o.O

pkruithof commented 10 years ago

Great! I'll start working on the pr then!

pkruithof commented 10 years ago

Fixed in #13773