Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

beacons.state does not save as list #41342

Closed
githubcdr opened this issue May 19, 2017 · 16 comments

Comments

@githubcdr
Copy link
Contributor

commented May 19, 2017

Description of Issue/Question

The following state file does not produce a working setup.

beacon-diskusage:
  beacon.present:
    - name: diskusage
    - interval: 123
    - /: 83%

The beacon config that gets applied is wrong and the beacon itself fails because a list is expected.

minion:
    beacons:
      diskusage:
        /: 83%
        interval: 123

It should have been a list.

minion:
  beacons:
    diskusage:
      - /: 83%
      - interval: 123

Setup

Latest Saltstack 2016.11.5 on CentOS 7

Steps to Reproduce Issue

See above

Versions Report

Salt Version:
           Salt: 2016.11.5

Dependency Versions:
           cffi: 1.9.1
       cherrypy: Not Installed
       dateutil: 1.5
      docker-py: Not Installed
          gitdb: 0.6.4
      gitpython: 1.0.1
          ioflo: 1.3.8
         Jinja2: 2.7.2
        libgit2: 0.24.6
        libnacl: 1.4.3
       M2Crypto: 0.21.1
           Mako: 1.0.1
   msgpack-pure: 0.1.3
 msgpack-python: 0.4.8
   mysql-python: 1.2.5
      pycparser: 2.17
       pycrypto: 2.6.1
   pycryptodome: 3.4.3
         pygit2: 0.24.2
         Python: 2.7.5 (default, Nov  6 2016, 00:28:07)
   python-gnupg: Not Installed
         PyYAML: 3.11
          PyZMQ: 15.3.0
           RAET: 0.6.3
          smmap: 0.9.0
        timelib: 0.2.4
        Tornado: 4.2.1
            ZMQ: 4.1.4

System Versions:
           dist: centos 7.3.1611 Core
        machine: x86_64
        release: 3.10.0-514.16.1.el7.x86_64
         system: Linux
        version: CentOS Linux 7.3.1611 Core
@githubcdr

This comment has been minimized.

Copy link
Contributor Author

commented May 19, 2017

On a side note, "salt minion beacons.delete diskusage" module does not work. User is informed that beacons has been deleted but the config is not updated.

@githubcdr githubcdr changed the title beacons.state does not safe as list beacons.state does not save as list May 19, 2017

@cetanu

This comment has been minimized.

Copy link
Contributor

commented May 21, 2017

Right... So I'm having a dig through the beacons module and I see that it fires an event which triggers the manage_beacons function in salt/minion.py, which then triggers the add_beacon function in beacons...

and it looks like the beacon_data is a dict object (passed in from modules/beacons/add()), which makes sense given what you posted above.

It seems like this would be occurring for every type of beacon, can you confirm? I don't have a test machine up right now.

I think we would need to coerce the object received via the beacon_data param into a list of dictionaries.

@githubcdr

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2017

Hi @cetanu , thanks for your response, you are correct all beacon data is set as a dict and not a list. If you coerce the object data to a list of dictionaries things will be correct.

(I have been unable to do this with my currect Python skillset ;)

@cetanu

This comment has been minimized.

Copy link
Contributor

commented May 22, 2017

I would need somebody to confirm but it looks like the beacon_data is passed here: https://github.com/saltstack/salt/blob/develop/salt/modules/beacons.py#L125

The schema of the data should be list([dict()]) ie:

[
    {'key': 'value'}
]

If a dict IS being passed into this eventer/thing... it can be transformed with:

beacon_data = [{key: value} for key, value in beacon_data.items()]

In terms of design, I think I'd like somebody from salt to weigh in on what should be done.
It can either be handled in the destination function:
(salt/minion/manage_beacons -> add/modify/delete/enable/disable/etc)

or in the source function salt/beacons/add

or some place else, but I wouldn't know where

@garethgreenaway

This comment has been minimized.

Copy link
Member

commented May 22, 2017

The original way to configure beacons was via a dictionary. That changed and now it's via a list. The various functions that are expecting a dictionary should be updated to expect a list.

@Ch3LL

This comment has been minimized.

Copy link
Contributor

commented May 22, 2017

@cetanu does @garethgreenaway 's comment help you towards a fix?

@Ch3LL Ch3LL added this to the Approved milestone May 22, 2017

@Ch3LL Ch3LL added the Core label May 22, 2017

@cetanu

This comment has been minimized.

Copy link
Contributor

commented May 23, 2017

Yep that gives me some direction. I'll see what I can do, unless someone beats me to it!

@cetanu

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2017

I've fixed up a bunch of code for this.

What I am seeing though, is a fair bit of inconsistency when modifying beacons.

I have a lot of trouble modifying directly with beacons.modify, although changes from beacons.modify seem to apply when I apply a highstate afterwards (even if the highstate is applying something different).

Highstates also seem to mimic this, but differently; they seem to apply correctly on a consecutive highstate, but not the first.

At this point in time I am not sure what is causing this.

I might be hallucinating as it's 12:30AM and some of my log messages aren't coming through. I'll see tomorrow.

@cetanu

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2017

Yep I'm definitely having some issues with persisting new beacon configuration via beacon.modify and also via highstate.

If I set up a state file with beacon.present and apply it, I can then get into an infinite loop of applying highstate, followed by checking the beacons (as one might naturally do):

  1. Change the beacon.present in the state file
  2. Apply highstate (this should say that it modified the beacon)
  3. List beacons

Step 3 will reset the beacons back to their previous config - I do not know why. I suspect it has something to do with a call to a function called config.merge, which might be getting whatever is saved in /etc/salt/minion.d/beacons.conf and applying it over the top of the current beacons!?

Edit: Oh yeah, that's exactly what's happening. I removed the file and all of a sudden, beacons.modify works just fine and dandy (Edit 2: until a beacon config is set, then the same thing appears to happen until the next salt-minion restart clears the beacon config). Gonna look further into this.

@githubcdr

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2017

Thanks for looking in to this, if you need anything tested let me know.

@stale

This comment has been minimized.

Copy link

commented Oct 30, 2018

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 stale bot added the stale label Oct 30, 2018

@travispaul

This comment has been minimized.

Copy link
Contributor

commented Oct 31, 2018

This is still broken unless some work has been done in this area since Sept 20 (and wasn't linked to this issue).

@stale

This comment has been minimized.

Copy link

commented Oct 31, 2018

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

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Nov 5, 2018

@travispaul Please see #50347

@travispaul

This comment has been minimized.

Copy link
Contributor

commented Nov 5, 2018

Awesome! Thanks @rallytime and @garethgreenaway

@garethgreenaway

This comment has been minimized.

Copy link
Member

commented Jan 9, 2019

Closing this out, if the problem persists please comment & we'll re-open the issue or feel free to open a new issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.