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

Compound targeting by nodegroup broken in 2019.2.0 #52678

Closed
tkaehn opened this issue Apr 24, 2019 · 9 comments · Fixed by #56592
Closed

Compound targeting by nodegroup broken in 2019.2.0 #52678

tkaehn opened this issue Apr 24, 2019 · 9 comments · Fixed by #56592
Labels
Bug broken, incorrect, or confusing behavior Confirmed Salt engineer has confirmed bug/feature - often including a MCVE Core relates to code central or existential to Salt Grains P3 Priority 3 severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around ZRelease-Sodium retired label
Projects
Milestone

Comments

@tkaehn
Copy link

tkaehn commented Apr 24, 2019

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
Copy link
Author

tkaehn commented Apr 24, 2019

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
Copy link
Contributor

max-arnold commented Apr 24, 2019

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 DmitryKuzmenko added the Confirmed Salt engineer has confirmed bug/feature - often including a MCVE label Apr 24, 2019
@DmitryKuzmenko
Copy link
Contributor

DmitryKuzmenko commented Apr 24, 2019

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.

@DmitryKuzmenko DmitryKuzmenko added Bug broken, incorrect, or confusing behavior Core relates to code central or existential to Salt Grains severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around P3 Priority 3 team-core labels Apr 24, 2019
@DmitryKuzmenko DmitryKuzmenko added this to the Approved milestone Apr 24, 2019
@mattp-
Copy link
Contributor

mattp- commented Apr 25, 2019

@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-
Copy link
Contributor

mattp- commented Apr 25, 2019

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
Copy link
Contributor

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

@mattp-
Copy link
Contributor

mattp- commented Apr 25, 2019

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 🙂

@pkramme
Copy link

pkramme commented May 19, 2020

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!).

@sagetherage sagetherage added the ZRelease-Sodium retired label label May 19, 2020
@sagetherage sagetherage added this to Done in Sodium May 19, 2020
@leifliddy
Copy link
Contributor

leifliddy commented May 28, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug broken, incorrect, or confusing behavior Confirmed Salt engineer has confirmed bug/feature - often including a MCVE Core relates to code central or existential to Salt Grains P3 Priority 3 severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around ZRelease-Sodium retired label
Projects
No open projects
Sodium
  
Done
Development

Successfully merging a pull request may close this issue.

7 participants