baby-gnu
commented
1 month ago I think I found a solution with this problem, still present in 3007.1.
I tried to simplify the code by using salt.utils.odict.OrderedDict like done in many other places to keep ordering of object.
With the example of this issue, the result order is [C, B, A, M, L, MINION_DICT].
my patch
``` diff diff --git a/salt/utils/saltclass.py b/salt/utils/saltclass.py index 7d6fec7c578..85895ec50ee 100644 --- a/salt/utils/saltclass.py +++ b/salt/utils/saltclass.py @@ -5,6 +5,7 @@ import re from jinja2 import Environment, FileSystemLoader +import salt.utils.odict import salt.utils.path import salt.utils.yaml @@ -278,8 +279,28 @@ def expand_classes_glob(classes, salt_data): def expand_classes_in_order( - minion_dict, salt_data, seen_classes, expanded_classes, classes_to_expand + minion_dict, salt_data, seen_classes, classes_to_expand ): + """ + Expand the list of `classes_to_expand` and return them in the order found + + The return order is `[C, B, A, M, L, MINION_DICT]` when: + + - minion node include classes `A` and `L` + - `A` include class `B` + - `B` include class `C` + - `L` include class `M` + + :param dict minion_dict: definition of minion node + :param dict salt_data: configuration data + :param iterable(str) seen_classes: classes already processed + :param iterable(str) classes_to_expand: classes to recursivly expand + :return: Expanded classes in proper order + :rtype: salt.utils.odict.OrderedDict + """ + + expanded_classes = salt.utils.odict.OrderedDict() + # Get classes to expand from minion dictionary if not classes_to_expand and "classes" in minion_dict: classes_to_expand = minion_dict["classes"] @@ -290,71 +311,36 @@ def expand_classes_in_order( for klass in classes_to_expand: if klass not in seen_classes: seen_classes.append(klass) - expanded_classes[klass] = get_class(klass, salt_data) + klass_dict = {klass: get_class(klass, salt_data)} # Fix corner case where class is loaded but doesn't contain anything - if expanded_classes[klass] is None: - expanded_classes[klass] = {} + if klass_dict[klass] is None: + klass_dict[klass] = {} # Merge newly found pillars into existing ones - new_pillars = expanded_classes[klass].get("pillars", {}) + new_pillars = klass_dict[klass].get("pillars", {}) if new_pillars: dict_merge(salt_data["__pillar__"], new_pillars) - # Now replace class element in classes_to_expand by expansion - if expanded_classes[klass].get("classes"): - l_id = classes_to_expand.index(klass) - classes_to_expand[l_id:l_id] = expanded_classes[klass]["classes"] - expand_classes_in_order( - minion_dict, - salt_data, - seen_classes, - expanded_classes, - classes_to_expand, - ) - else: - expand_classes_in_order( - minion_dict, + if "classes" in klass_dict[klass]: + nested_classes = expand_classes_in_order( + {}, salt_data, seen_classes, - expanded_classes, - classes_to_expand, + klass_dict[klass].get("classes", {}), + level=level+1 ) - # We may have duplicates here and we want to remove them - tmp = [] - for t_element in classes_to_expand: - if t_element not in tmp: - tmp.append(t_element) - - classes_to_expand = tmp - - # Now that we've retrieved every class in order, - # let's return an ordered list of dicts - ord_expanded_classes = [] - ord_expanded_states = [] - for ord_klass in classes_to_expand: - ord_expanded_classes.append(expanded_classes[ord_klass]) - # And be smart and sort out states list - # Address the corner case where states is empty in a class definition - if ( - "states" in expanded_classes[ord_klass] - and expanded_classes[ord_klass]["states"] is None - ): - expanded_classes[ord_klass]["states"] = {} + # Put current class after nested classes + nested_classes.update(klass_dict) + klass_dict = nested_classes - if "states" in expanded_classes[ord_klass]: - ord_expanded_states.extend(expanded_classes[ord_klass]["states"]) + expanded_classes.update(klass_dict) - # Add our minion dict as final element but check if we have states to process - if "states" in minion_dict and minion_dict["states"] is None: - minion_dict["states"] = [] + # Minion dict must be at the end + if minion_dict: + expanded_classes.update({"minion_dict": minion_dict}) - if "states" in minion_dict: - ord_expanded_states.extend(minion_dict["states"]) - - ord_expanded_classes.append(minion_dict) - - return ord_expanded_classes, classes_to_expand, ord_expanded_states + return expanded_classes def expanded_dict_from_minion(minion_id, salt_data): @@ -382,17 +368,26 @@ def expanded_dict_from_minion(minion_id, salt_data): # Get 2 ordered lists: # expanded_classes: A list of all the dicts # classes_list: List of all the classes - expanded_classes, classes_list, states_list = expand_classes_in_order( - node_dict[minion_id], salt_data, [], {}, [] + expanded_classes = expand_classes_in_order( + node_dict[minion_id], salt_data, [], [] ) # Here merge the pillars together pillars_dict = {} - for exp_dict in expanded_classes: + states_list = [] + classes_list = list(expanded_classes.keys()) + classes_values = list(expanded_classes.values()) + for exp_dict in classes_values: if "pillars" in exp_dict: dict_merge(pillars_dict, exp_dict) + if "states" in exp_dict: + states_list.extend(exp_dict["states"]) + + # Avoid duplicates, keep first + state_seen = set() + states_list = [state for state in states_list if not (state in state_seen or state_seen.add(state))] - return expanded_classes, pillars_dict, classes_list, states_list + return classes_values, pillars_dict, classes_list, states_list def get_pillars(minion_id, salt_data): ```I added some logs to make sure of the original order:
some manually added warning logs in the original code
``` [WARNING ] expand_classes_in_order: level=0: START minion_dict={'environment': 'base', 'classes': ['A', 'L']} seen_classes=[] classes_to_expand=[] [WARNING ] expand_classes_in_order: level=0: add minion classes to expand: ['A', 'L'] [WARNING ] expand_classes_in_order: level=0: loop on class A [WARNING ] expand_classes_in_order: level=0: got A: {'states': ['A.1', 'A.2'], 'classes': ['B']} [WARNING ] expand_classes_in_order: level=1: START minion_dict={'environment': 'base', 'classes': ['A', 'L']} seen_classes=['A'] classes_to_expand=['B', 'A', 'L'] [WARNING ] expand_classes_in_order: level=1: loop on class B [WARNING ] expand_classes_in_order: level=1: got B: {'states': ['B.1', 'B.2'], 'classes': ['C']} [WARNING ] expand_classes_in_order: level=2: START minion_dict={'environment': 'base', 'classes': ['A', 'L']} seen_classes=['A', 'B'] classes_to_expand=['C', 'B', 'A', 'L'] [WARNING ] expand_classes_in_order: level=2: loop on class C [WARNING ] expand_classes_in_order: level=2: got C: {'states': ['C.1', 'C.2', 'W']} [WARNING ] expand_classes_in_order: level=3: START minion_dict={'environment': 'base', 'classes': ['A', 'L']} seen_classes=['A', 'B', 'C'] classes_to_expand=['C', 'B', 'A', 'L'] [WARNING ] expand_classes_in_order: level=3: loop on class C [WARNING ] expand_classes_in_order: level=3: loop on class B [WARNING ] expand_classes_in_order: level=3: loop on class A [WARNING ] expand_classes_in_order: level=3: loop on class L [WARNING ] expand_classes_in_order: level=3: got L: {'states': ['L.1', 'L.2', 'W'], 'classes': ['M']} [WARNING ] expand_classes_in_order: level=4: START minion_dict={'environment': 'base', 'classes': ['A', 'L']} seen_classes=['A', 'B', 'C', 'L'] classes_to_expand=['C', 'B', 'A', 'M', 'L'] [WARNING ] expand_classes_in_order: level=4: loop on class C [WARNING ] expand_classes_in_order: level=4: loop on class B [WARNING ] expand_classes_in_order: level=4: loop on class A [WARNING ] expand_classes_in_order: level=4: loop on class M [WARNING ] expand_classes_in_order: level=4: got M: {'states': ['M.1', 'M.2']} [WARNING ] expand_classes_in_order: level=5: START minion_dict={'environment': 'base', 'classes': ['A', 'L']} seen_classes=['A', 'B', 'C', 'L', 'M'] classes_to_expand=['C', 'B', 'A', 'M', 'L'] [WARNING ] expand_classes_in_order: level=5: loop on class C [WARNING ] expand_classes_in_order: level=5: loop on class B [WARNING ] expand_classes_in_order: level=5: loop on class A [WARNING ] expand_classes_in_order: level=5: loop on class M [WARNING ] expand_classes_in_order: level=5: loop on class L [WARNING ] expand_classes_in_order: level=5: RETURN [{'states': ['C.1', 'C.2', 'W']}, {'states': ['B.1', 'B.2'], 'classes': ['C']}, {'states': ['A.1', 'A.2'], 'classes': ['B']}, {'states': ['M.1', 'M.2']}, {'states': ['L.1', 'L.2', 'W'], 'classes': ['M']}, {'environment': 'base', 'classes': ['A', 'L']}] [WARNING ] expand_classes_in_order: level=4: loop on class L [WARNING ] expand_classes_in_order: level=4: RETURN [{'states': ['C.1', 'C.2', 'W']}, {'states': ['B.1', 'B.2'], 'classes': ['C']}, {'states': ['A.1', 'A.2'], 'classes': ['B']}, {'states': ['M.1', 'M.2']}, {'states': ['L.1', 'L.2', 'W'], 'classes': ['M']}, {'environment': 'base', 'classes': ['A', 'L']}] [WARNING ] expand_classes_in_order: level=3: loop on class L [WARNING ] expand_classes_in_order: level=3: RETURN [{'states': ['C.1', 'C.2', 'W']}, {'states': ['B.1', 'B.2'], 'classes': ['C']}, {'states': ['A.1', 'A.2'], 'classes': ['B']}, {'states': ['M.1', 'M.2']}, {'states': ['L.1', 'L.2', 'W'], 'classes': ['M']}, {'environment': 'base', 'classes': ['A', 'L']}] [WARNING ] expand_classes_in_order: level=2: loop on class B [WARNING ] expand_classes_in_order: level=2: loop on class A [WARNING ] expand_classes_in_order: level=2: loop on class L [WARNING ] expand_classes_in_order: level=2: RETURN [{'states': ['C.1', 'C.2', 'W']}, {'states': ['B.1', 'B.2'], 'classes': ['C']}, {'states': ['A.1', 'A.2'], 'classes': ['B']}, {'states': ['L.1', 'L.2', 'W'], 'classes': ['M']}, {'environment': 'base', 'classes': ['A', 'L']}] [WARNING ] expand_classes_in_order: level=1: loop on class B [WARNING ] expand_classes_in_order: level=1: loop on class A [WARNING ] expand_classes_in_order: level=1: loop on class L [WARNING ] expand_classes_in_order: level=1: RETURN [{'states': ['C.1', 'C.2', 'W']}, {'states': ['B.1', 'B.2'], 'classes': ['C']}, {'states': ['A.1', 'A.2'], 'classes': ['B']}, {'states': ['L.1', 'L.2', 'W'], 'classes': ['M']}, {'environment': 'base', 'classes': ['A', 'L']}] [WARNING ] expand_classes_in_order: level=0: loop on class A [WARNING ] expand_classes_in_order: level=0: loop on class L [WARNING ] expand_classes_in_order: level=0: RETURN [{'states': ['B.1', 'B.2'], 'classes': ['C']}, {'states': ['A.1', 'A.2'], 'classes': ['B']}, {'states': ['L.1', 'L.2', 'W'], 'classes': ['M']}, {'environment': 'base', 'classes': ['A', 'L']}] [WARNING ] expanded_dict_from_minion: expanded_classes=[{'states': ['B.1', 'B.2'], 'classes': ['C']}, {'states': ['A.1', 'A.2'], 'classes': ['B']}, {'states': ['L.1', 'L.2', 'W'], 'classes': ['M']}, {'environment': 'base', 'classes': ['A', 'L']}] [WARNING ] expanded_dict_from_minion: RETURN STATE LIST: ['B.1', 'B.2', 'A.1', 'A.2', 'L.1', 'L.2', 'W'] test-minion: ---------- base: - B.1 - B.2 - A.1 - A.2 - L.1 - L.2 - W ```some manually added logs to the patch
``` [WARNING ] expand_classes_in_order: level=0: START minion_dict={'environment': 'base', 'classes': ['A', 'L']} seen_classes=[] classes_to_expand=[] [WARNING ] expand_classes_in_order: level=0: append minion_dict to expanded_classes : OrderedDict([('minion_dict', {'environment': 'base', 'classes': ['A', 'L']})]) [WARNING ] expand_classes_in_order: level=0: add minion classes to expand: ['A', 'L'] [WARNING ] expand_classes_in_order: level=0: loop on class A [WARNING ] expand_classes_in_order: level=0: got A: {'states': ['A.1', 'A.2'], 'classes': ['B']} [WARNING ] expand_classes_in_order: level=1: START minion_dict={} seen_classes=['A'] classes_to_expand=['B'] [WARNING ] expand_classes_in_order: level=1: loop on class B [WARNING ] expand_classes_in_order: level=1: got B: {'states': ['B.1', 'B.2'], 'classes': ['C']} [WARNING ] expand_classes_in_order: level=2: START minion_dict={} seen_classes=['A', 'B'] classes_to_expand=['C'] [WARNING ] expand_classes_in_order: level=2: loop on class C [WARNING ] expand_classes_in_order: level=2: got C: {'states': ['C.1', 'C.2', 'W']} [WARNING ] expand_classes_in_order: level=2: RETURN OrderedDict([('C', {'states': ['C.1', 'C.2', 'W']})]) [WARNING ] expand_classes_in_order: level=1: sub_classes OrderedDict([('C', {'states': ['C.1', 'C.2', 'W']})]) [WARNING ] expand_classes_in_order: level=1: RETURN OrderedDict([('C', {'states': ['C.1', 'C.2', 'W']}), ('B', {'states': ['B.1', 'B.2'], 'classes': ['C']})]) [WARNING ] expand_classes_in_order: level=0: sub_classes OrderedDict([('C', {'states': ['C.1', 'C.2', 'W']}), ('B', {'states': ['B.1', 'B.2'], 'classes': ['C']})]) [WARNING ] expand_classes_in_order: level=0: loop on class L [WARNING ] expand_classes_in_order: level=0: got L: {'states': ['L.1', 'L.2', 'W'], 'classes': ['M']} [WARNING ] expand_classes_in_order: level=1: START minion_dict={} seen_classes=['A', 'B', 'C', 'L'] classes_to_expand=['M'] [WARNING ] expand_classes_in_order: level=1: loop on class M [WARNING ] expand_classes_in_order: level=1: got M: {'states': ['M.1', 'M.2']} [WARNING ] expand_classes_in_order: level=1: RETURN OrderedDict([('M', {'states': ['M.1', 'M.2']})]) [WARNING ] expand_classes_in_order: level=0: sub_classes OrderedDict([('M', {'states': ['M.1', 'M.2']})]) [WARNING ] expand_classes_in_order: level=0: RETURN OrderedDict([('M', {'states': ['M.1', 'M.2']}), ('C', {'states': ['C.1', 'C.2', 'W']}), ('B', {'states': ['B.1', 'B.2'], 'classes': ['C']}), ('minion_dict', {'environment': 'base', 'classes': ['A', 'L']}), ('A', {'states': ['A.1', 'A.2'], 'classes': ['B']}), ('L', {'states': ['L.1', 'L.2', 'W'], 'classes': ['M']})]) [WARNING ] expanded_dict_from_minion: expanded_classes=OrderedDict([('M', {'states': ['M.1', 'M.2']}), ('C', {'states': ['C.1', 'C.2', 'W']}), ('B', {'states': ['B.1', 'B.2'], 'classes': ['C']}), ('minion_dict', {'environment': 'base', 'classes': ['A', 'L']}), ('A', {'states': ['A.1', 'A.2'], 'classes': ['B']}), ('L', {'states': ['L.1', 'L.2', 'W'], 'classes': ['M']})]) [WARNING ] expanded_dict_from_minion: extend states with ['M.1', 'M.2']: ['M.1', 'M.2'] [WARNING ] expanded_dict_from_minion: extend states with ['C.1', 'C.2', 'W']: ['M.1', 'M.2', 'C.1', 'C.2', 'W'] [WARNING ] expanded_dict_from_minion: extend states with ['B.1', 'B.2']: ['M.1', 'M.2', 'C.1', 'C.2', 'W', 'B.1', 'B.2'] [WARNING ] expanded_dict_from_minion: extend states with ['A.1', 'A.2']: ['M.1', 'M.2', 'C.1', 'C.2', 'W', 'B.1', 'B.2', 'A.1', 'A.2'] [WARNING ] expanded_dict_from_minion: extend states with ['L.1', 'L.2', 'W']: ['M.1', 'M.2', 'C.1', 'C.2', 'W', 'B.1', 'B.2', 'A.1', 'A.2', 'L.1', 'L.2', 'W'] [WARNING ] expanded_dict_from_minion: RETURN STATE LIST: ['M.1', 'M.2', 'C.1', 'C.2', 'W', 'B.1', 'B.2', 'A.1', 'A.2', 'L.1', 'L.2'] test-minion: ---------- base: - M.1 - M.2 - C.1 - C.2 - W - B.1 - B.2 - A.1 - A.2 - L.1 - L.2 ```As you can see, my code avoid many loops and recursion by avoiding to pass all accumulated classes to classes_to_expand, just the new ones found in the current class.
I'll propose a PR soon.
Regards.
Description When using saltclass with multiple levels of classes, all of which define states, the resulting state list only comes back with states for the top level classes. Looking at the code, it appears that it is ignoring the state lists returned from the recursive calls of
expand_classes_in_orderSetup
Expected behavior Running state.show_top with the above classes, one would expect a state list of
[1,2,3,4,5,6,7,8], but what comes back instead is[1,2,7]Versions Report
salt --versions-report
``` Salt Version: Salt: 3002.1 Dependency Versions: cffi: 1.14.2 cherrypy: Not Installed dateutil: 2.8.1 docker-py: Not Installed gitdb: Not Installed gitpython: Not Installed Jinja2: 2.11.1 libgit2: 0.28.2 M2Crypto: 0.35.2 Mako: Not Installed msgpack-pure: Not Installed msgpack-python: 0.6.2 mysql-python: Not Installed pycparser: 2.20 pycrypto: 2.6.1 pycryptodome: 3.9.7 pygit2: 0.28.2 Python: 3.6.8 (default, Oct 13 2020, 16:18:22) python-gnupg: Not Installed PyYAML: 3.13 PyZMQ: 17.0.0 smmap: Not Installed timelib: Not Installed Tornado: 4.5.3 ZMQ: 4.1.4 System Versions: dist: centos 7 Core locale: UTF-8 machine: x86_64 release: 3.10.0-1160.2.2.el7.x86_64 system: Linux version: CentOS Linux 7 Core ```Additional context Without knowing exactly what was intended, this is my rough guess as to what might have been meant to happen: