[BUG] salt.thorium.reg.clear clears a level of dict too much

[Heiko-san]

Description

I try to fire a single action in response to many events (one from each minion). So I use the following example code:

patch_grp2:
  reg.set:
    - add: host
    - match: custom/system/up
  check.contains:
    - value: mysystem1
    - value: mysystem2

shell_test:
  local.cmd:
    - tgt: '*'
    - func: cmd.run
    - arg:
      - echo 'thorium success' > /tmp/thorium.txt
    - require:
      - check: patch_grp2
  reg.clear:
    - name: patch_grp2

And on mysystem1/2 I do:

salt-call event.send custom/system/up "{host: $(hostname -s)}"

It works as expected: /tmp/thorium.txt is created after both minions sent the event. But since the action is triggered multiple times (e.g. if I rm /tmp/thorium.txt on a minion, file is instantly recreated), I do the reg.clear in my action. If seems this would solve my problem, but there seems to be a bug:

The clear triggers and I get the following error in Log

2020-07-02 14:59:11,525 [salt.state                                                         :322 ][ERROR   ][1412] An exception occurred in this state: Traceback (most recent call last):
  File "/usr/local/lib/python3.6/dist-packages/salt/state.py", line 1981, in call
    **cdata['kwargs'])
  File "/usr/local/lib/python3.6/dist-packages/salt/loader.py", line 1977, in wrapper
    return f(*args, **kwargs)
  File "/usr/local/lib/python3.6/dist-packages/salt/thorium/check.py", line 270, in contains
    if value in __reg__[name]['val']:
KeyError: 'val'

After that the Thorium register doesn't work at all until master restart. I guess you want to clear the contents of __reg__[name]['val'] but actually you do clear __reg__[name] ?

Versions Report

salt --versions-report

(Provided by running salt --versions-report. Please also mention any differences in master/minion versions.) ``` Salt Version: Salt: 3000.2 Dependency Versions: cffi: Not Installed cherrypy: unknown dateutil: 2.6.1 docker-py: Not Installed gitdb: 2.0.3 gitpython: 2.1.8 Jinja2: 2.11.2 libgit2: Not Installed M2Crypto: Not Installed Mako: Not Installed msgpack-pure: Not Installed msgpack-python: 0.6.2 mysql-python: Not Installed pycparser: Not Installed pycrypto: 2.6.1 pycryptodome: Not Installed pygit2: Not Installed Python: 3.6.9 (default, Apr 18 2020, 01:56:04) python-gnupg: 0.4.1 PyYAML: 5.3.1 PyZMQ: 19.0.0 smmap: 2.0.3 timelib: Not Installed Tornado: 4.5.3 ZMQ: 4.3.2 System Versions: dist: Ubuntu 18.04 bionic locale: UTF-8 machine: x86_64 release: 4.15.0-96-generic system: Linux version: Ubuntu 18.04 bionic ```

[Heiko-san]

btw: another "minor bug" is that save.file throws a "TypeError: Object of type 'set' is not JSON serializable" error for reg.set

A possible fix for this could be:

--- file.py.old 2020-07-02 15:41:14.722003824 +0200
+++ file.py 2020-07-02 16:12:56.586766516 +0200
@@ -41,6 +41,7 @@
 # import python libs
 from __future__ import absolute_import, print_function, unicode_literals
 import os
+import json

 # Import salt libs
 import salt.utils.data
@@ -48,6 +49,13 @@
 import salt.utils.json

+class SetEncoder(json.JSONEncoder):
+    def default(self, obj):
+        if isinstance(obj, set):
+            return str(obj)
+        return json.JSONEncoder.default(self, obj)
+
+
 def save(name, filter=False):
     '''
     Save the register to <salt cachedir>/thorium/saves/<name>, or to an
@@ -80,7 +88,7 @@
         os.makedirs(tgt_dir)
     with salt.utils.files.fopen(fn_, 'w+') as fp_:
         if filter is True:
-            salt.utils.json.dump(salt.utils.data.simple_types_filter(__reg__), fp_)
+            salt.utils.json.dump(salt.utils.data.simple_types_filter(__reg__), fp_, cls=SetEncoder)
         else:
-            salt.utils.json.dump(__reg__, fp_)
+            salt.utils.json.dump(__reg__, fp_, cls=SetEncoder)
     return ret

I intentionally added this logic to thorium/file.py instead of utils/json.py since I can not assess the possible side effects.

[Heiko-san]

Yes, this seems to fix the issue:

/usr/local/lib/python3.6/dist-packages/salt/thorium/reg.py

--- reg.py.old  2020-07-02 15:17:42.970803604 +0200
+++ reg.py  2020-07-02 15:13:40.268064514 +0200
@@ -150,7 +150,7 @@
            'comment': '',
            'result': True}
     if name in __reg__:
-        __reg__[name].clear()
+        __reg__[name]['val'].clear()
     return ret

[Heiko-san]

Actually by fixing the issues I recognized my code does not work. I have to add a require to reg.clear to prevent it running each time. And value mysystem2 overrides mysystem1 so , it is waiting for just mysystem2 event instead of both. The following notation doesn't work either.

patch_grp2:
  reg.set:
    - add: host
    - match: custom/system/up
  check.contains:
    - value:
      - mysystem1
      - mysystem2

shell_test:
  local.cmd:
    - tgt: '*'
    - func: cmd.run
    - arg:
      - echo 'thorium success' > /tmp/thorium.txt
    - require:
      - check: patch_grp2
  reg.clear:
    - name: patch_grp2
    - require:
      - check: patch_grp2

Deal: I fixed 2 of your issues , can you please help me with mine? :)

[Heiko-san]

Ok I found the solution by looking into thorium/check.py (use eq instead of contains), and actually another bug, too. ;)

And here is the fix:

--- check.py.old    2020-07-02 16:29:24.054136343 +0200
+++ check.py    2020-07-02 16:34:35.558769666 +0200
@@ -42,6 +42,8 @@
         ret['result'] = False
         ret['comment'] = 'Value {0} not in register'.format(name)
         return ret
+    if isinstance(__reg__[name]['val'], set):
+        value = set(value)
     if __reg__[name]['val'] > value:
         ret['result'] = True
     return ret
@@ -75,6 +77,8 @@
         ret['result'] = False
         ret['comment'] = 'Value {0} not in register'.format(name)
         return ret
+    if isinstance(__reg__[name]['val'], set):
+        value = set(value)
     if __reg__[name]['val'] >= value:
         ret['result'] = True
     return ret
@@ -108,6 +112,8 @@
         ret['result'] = False
         ret['comment'] = 'Value {0} not in register'.format(name)
         return ret
+    if isinstance(__reg__[name]['val'], set):
+        value = set(value)
     if __reg__[name]['val'] < value:
         ret['result'] = True
     return ret
@@ -141,6 +147,8 @@
         ret['result'] = False
         ret['comment'] = 'Value {0} not in register'.format(name)
         return ret
+    if isinstance(__reg__[name]['val'], set):
+        value = set(value)
     if __reg__[name]['val'] <= value:
         ret['result'] = True
     return ret
@@ -174,6 +182,8 @@
         ret['result'] = False
         ret['comment'] = 'Value {0} not in register'.format(name)
         return ret
+    if isinstance(__reg__[name]['val'], set):
+        value = set(value)
     if __reg__[name]['val'] == value:
         ret['result'] = True
     return ret
@@ -207,6 +217,8 @@
         ret['result'] = False
         ret['comment'] = 'Value {0} not in register'.format(name)
         return ret
+    if isinstance(__reg__[name]['val'], set):
+        value = set(value)
     if __reg__[name]['val'] != value:
         ret['result'] = True
     return ret

Disclaimer: I didn't check if '<' and '>' are actually defined for sets, but eq (and therefore ne) works.

[cmcmarrow]

@Heiko-San thanks for the bug repot and possible fixes for multiple issues. For your disclaimer sets don’t have __lt__ or __gt__