Salt: Compound targeting by nodegroup broken in 2019.2.0

[tkaehn]

Description of Issue/Question

Compound targeting by nodegroup broken in 2019.2.0. Please see details below.

Setup

root@master:/etc/salt/master.d# cat /etc/salt/master.d/nodegroups.conf 
nodegroups:
  testgroup: "G@virtual:VirtualBox"

root@master:/etc/salt/master.d# /etc/init.d/salt-master restart
[ ok ] Restarting salt-master (via systemctl): salt-master.service.

Steps to Reproduce Issue

root@master:/etc/salt/master.d# salt -N testgroup test.ping
master:
    True
root@master:/etc/salt/master.d# salt -C 'G@virtual:VirtualBox' test.ping
master:
    True
root@master:/etc/salt/master.d# salt -C 'N@testgroup' test.ping
master:
    Minion did not return. [No response]

Versions Report

root@master:/etc/salt/master.d# salt --versions-report
Salt Version:
           Salt: 2019.2.0

Dependency Versions:
           cffi: Not Installed
       cherrypy: Not Installed
       dateutil: 2.5.3
      docker-py: Not Installed
          gitdb: 2.0.0
      gitpython: 2.1.1
          ioflo: Not Installed
         Jinja2: 2.9.4
        libgit2: Not Installed
        libnacl: Not Installed
       M2Crypto: Not Installed
           Mako: Not Installed
   msgpack-pure: Not Installed
 msgpack-python: 0.4.8
   mysql-python: Not Installed
      pycparser: Not Installed
       pycrypto: 2.6.1
   pycryptodome: Not Installed
         pygit2: Not Installed
         Python: 2.7.13 (default, Sep 26 2018, 18:42:22)
   python-gnupg: Not Installed
         PyYAML: 3.12
          PyZMQ: 16.0.2
           RAET: Not Installed
          smmap: 2.0.1
        timelib: Not Installed
        Tornado: 4.4.3
            ZMQ: 4.2.1

System Versions:
           dist: debian 9.8 
         locale: UTF-8
        machine: x86_64
        release: 4.9.0-8-amd64
         system: Linux
        version: debian 9.8 

[tkaehn]

The following is logged to /var/log/salt/minion:

2019-04-24 08:34:49,562 [salt.utils.minions:107 ][ERROR   ][703] Failed nodegroup expansion: unknown nodegroup "testgroup"
2019-04-24 08:34:49,563 [salt.loaded.int.matchers.compound_match:110 ][ERROR   ][703] Invalid compound target: N@testgroup for results: 

[max-arnold]

I tried this feature a while ago, and it worked only when I put the same nodegroups into both master and minion configs.

@mattp- Could you please clarify if this is the way you intended it to work? Or this is an unfortunate effect of #48809 applied on top of your #47421?

[DmitryKuzmenko]

I can confirm this is a bug. I've used a group grp: 'G@os:Arch' The difference in master log is the following:
Master log for command ./scripts/salt -N grp test.ping:

[DEBUG   ] minions: {'alpha'}
[DEBUG   ] Attempting to match 'Arch' in 'os' using delimiter ':'
[DEBUG   ] Evaluating final compound matching expr: {'alpha'}
[DEBUG   ] Sending event: tag = 20190424180427236314; data = {'minions': ['alpha'], '_stamp': '2019-04-24T15:04:27.236536'}
[DEBUG   ] Sending event: tag = salt/job/20190424180427236314/new; data = {'jid': '20190424180427236314', 'tgt_type': 'compound', 'tgt': ['G@os:Arch'], 'user': 'dimm', 'fun': 'test.ping', 'arg': [], 'minions': ['alpha'], 'missing': [], '_stamp': '2019-04-24T15:04:27.236965'}

Master log for command ./scripts/salt -C 'N@grp' test.ping:

[DEBUG   ] minions: {'alpha'}
[DEBUG   ] nodegroup_comp(grp) => ['(', 'G@os:Arch', ')']
[DEBUG   ] No nested nodegroups detected. Using original nodegroup definition: G@os:Arch
[DEBUG   ] Attempting to match 'Arch' in 'os' using delimiter ':'
[DEBUG   ] Evaluating final compound matching expr: {'alpha'}
[DEBUG   ] Sending event: tag = 20190424180910690279; data = {'minions': ['alpha'], '_stamp': '2019-04-24T15:09:10.690501'}
[DEBUG   ] Sending event: tag = salt/job/20190424180910690279/new; data = {'jid': '20190424180910690279', 'tgt_type': 'compound', 'tgt': 'N@grp', 'user': 'dimm', 'fun': 'test.ping', 'arg': [], 'minions': ['alpha'], 'missing': [], '_stamp': '2019-04-24T15:09:10.690894'}

So in the second case master sends the original target definition not expanding the nodegroup definition. Since minion knows nothing about nodegroups it can't match itself against the target.

[mattp-]

@max-arnold my change was to fix compound node group matching specifically in pillar top files, this looks like a different worse / unrelated regression. So i suspect your guess in that other feature change introducing the bug is correct

[mattp-]

Actually took a quick look at my original changeset it is possible that the shallow copy I introduced removed some action at a distance of “expanding” node groups in tgt. I can’t remember if that happens explicitly elsewhere in the codebase, I’ll try to take a peek tomorrow.

[DmitryKuzmenko]

@mattp- thank you for taking care of this. May it be assigned to you?

[mattp-]

I don't think this ever worked, I tried a checkout of v2017.7.2 (pre my change) and the behavior is the same. I think what needs to happen is:
https://github.com/saltstack/salt/blob/develop/salt/utils/minions.py#L91
nodegroup_comp reduces nodegroups to their equivalent real representation. it just needs to be able to do the same thing for a compound expression, then,
https://github.com/saltstack/salt/blob/develop/salt/client/__init__.py#L1645
_prep_pub can reduce those nodegroups the same way its happening for the direct nodegroup and cidr tgts now. I don't think I've got time to tackle that any time in the next few weeks though, its a fair chunk of new dev, sorry :slightly_smiling_face:

[pkramme]

Currently the documentation says

The N@ classifier historically could not be used in compound matches within the CLI or top file, it was only recognized in the nodegroups master config file parameter. As of the 2019.2.0 release, this limitation no longer exists.

It would be great if the documentation could either be fixed or the compound matcher fix be backported to 2019.2.* (which we'd really prefer!).

[leifliddy]

Are there any plans to backport this PR to previous salt releases as it's classified as a high severity bug?