beacon.present should merge configs, not overwrite them

[dkfsalt]

Description of Issue/Question

Enhancement Request: Multiple State files that use beacon.present on the same type of beacon overwrite each other. Ideally, the two configs would merge together in the beacons.conf, rather than overwritting them.

This would give users greater flexibility in their ability to manage beacons on an application basis rather than globally (which increases the complexity of configuration and management of the beacons).

Setup

# Because the inotify beacon relies on the python-inotify
# package, we must install it first
install_python_inotify:
  pkg.installed:
    - name: python-inotify

# Watch infs.conf
inotify_ssh:
  beacon.present:
    - name: inotify
    - save: True
    - enable: True
    - files:
       /etc/infs.conf:
         mask:
           - create
           - attrib
           - delete
           - delete_self
           - modify
           - moved_from
           - moved_to
           - move_self
         recurse: True
         auto_add: True
    - interval: 10
    - disable_during_state_run: True
    - require:
      - pkg: install_python_inotify

---------------
# Because the inotify beacon relies on the python-inotify
# package, we must install it first
install_python_inotify:
  pkg.installed:
    - name: python-inotify

# Watch ntp.conf
inotify_ntp:
  beacon.present:
    - name: inotify
    - save: True
    - enable: True
    - files:
       /etc/ntp.conf:
         mask:
           - create
           - attrib
           - delete
           - delete_self
           - modify
           - moved_from
           - moved_to
           - move_self
         recurse: True
         auto_add: True
    - interval: 10
    - disable_during_state_run: True
    - require:
      - pkg: install_python_inotify

The ideal result of running these two states would be an /etc/salt/minion.d/beacons.conf which looks like:

beacons:
  inotify:
  - files:
      /etc/ntp.conf:
        auto_add: true
        mask:
        - create
        - attrib
        - delete
        - delete_self
        - modify
        - moved_from
        - moved_to
        - move_self
        recurse: true
        enable: true
        interval: 10
        disable_during_state_run: true
       /etc/infs.conf:
         mask:
           - create
           - attrib
           - delete
           - delete_self
           - modify
           - moved_from
           - moved_to
           - move_self
         recurse: True
         auto_add: True
        enable: true
        interval: 50
        disable_during_state_run: true

Notice the difference in the "scope" of the "enable", "interval", "disable_during_state_run" parameters - in order to make this work, they would need to run on a per "files" definition basis and that pattern would need to be applied to other beacon types as well (like services).

Ideally, though, there would be a standard for beacon methods such that the states.beacon module simply calls the corresponding methods of the beacon module. So that all beacons have the following methods:

• present
• enabled
• disabled
• validate

eg: beacon.present() would simply determine the type of module and call the corresponding present method of the beacon type (eg - "service.present()" or "inotify.present()".

Note: I suspect the impact of this would be far reaching for the minion itself, but I think that the current pattern for beacons is broken and causes unnecessary complexity for management.

Steps to Reproduce Issue

Put the two beacon.present configs into separate state files and apply them to a minion; the first config is overwritten by the second.

Versions Report

Master 2018.3.3, Minion 2018.3.4 / 2019.02

[garethgreenaway]

@dkfsalt There is a feature I added to develop which I think might accomplish what you were looking for. It allows you to specify a beacon_module in the configuration, eg.

beacons:
  watch_opt_special:
    - files:
        /opt/special:
          mask:
            - delete
            - close_write
          auto_add: True
    - coalesce: True
    - beacon_type: inotify

This would allow you to include multiple instances of the inotify beacon each with a different name. I need to verify that it works when using beacon.present and properly document it.

[dkfsalt]

@garethgreenaway - Nice one! I will give it a try in Apr (I'm currently on holiday :)

[garethgreenaway]

@dkfsalt Enjoy the holiday 😄

[dkfsalt]

Hi Gareth, I've finally had a chance to look at this (sorry for the delay) and I that's not quite what I was getting at.

If you think about nearly all other state modules, the "present" methods adhere to the following patterns:

1. Ensures that the defined object exists (creates it if it doesn't)
2. Ensures that the existing object matches declared state (if the existing object's state differs from the declared state, bring it into compliance)

The current implementation of this functionality violates both of those patterns.

Adding the "coalesce" and "beacon_type" properties doesn't help solve that issue. What it does is create a new edge-case parsing pattern that is non-intuitive and (IMHO) is likely to create another pattern that we must maintain downstream.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

If this issue is closed prematurely, please leave a comment and we will gladly reopen the issue.

[stale[bot]]

Thank you for updating this issue. It is no longer marked as stale.