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

[BUG] Discrepancy between 'salt' and 'salt-ssh' results with 'slsutil.update' within a Jinja expression-statement #65067

Closed
3 tasks done
FreeOni opened this issue Aug 28, 2023 · 6 comments · Fixed by #65517
Closed
3 tasks done
Assignees
Labels
Bug broken, incorrect, or confusing behavior Salt-SSH

Comments

@FreeOni
Copy link

FreeOni commented Aug 28, 2023

Description
The slsutil.update module produces different results when called from a Jinja expression-statement depending on whether salt or salt-ssh is used.

Setup
/srv/salt/bug/init.sls

---
{% set update_map = {'key': 'original'} %}
{% set merge_map = {'key': 'original'} %}
{% set new = {'key': 'new'} %}



notify-before-update:
  test.show_notification:
    - name: 'Original update_map'
    - text: {{ update_map.key }}

{% do salt['slsutil.update'](update_map, new) %}

notify-after-update:
  test.show_notification:
    - name: 'Update result'
    - text: {{ update_map.key }}



notify-before-merge:
  test.show_notification:
    - name: 'Original merge_map'
    - text: {{ merge_map.key }}

{% do salt['slsutil.merge'](merge_map, new) %}

notify-after-merge:
  test.show_notification:
    - name: 'Merge result'
    - text: {{ merge_map.key }}

Please be as specific as possible and give set-up details.

  • (Master) on-prem machine
  • (Minion) VM (Vagrant with libvirt driver running on Master)
  • Both installed with Fedora 38: sudo dnf install salt-master salt-minion

Steps to Reproduce the behavior
Commands
sudo salt-ssh '*' state.sls bug
followed by:
sudo salt '*' state.sls bug

Output
Cleaned up salt-ssh output:

minion:
----------
          ID: notify-before-update
    Function: test.show_notification
        Name: Original update_map
     Comment: original
----------
          ID: notify-after-update
    Function: test.show_notification
        Name: Update result
     Comment: original
----------
          ID: notify-before-merge
    Function: test.show_notification
        Name: Original merge_map
     Comment: original
----------
          ID: notify-after-merge
    Function: test.show_notification
        Name: Merge result
     Comment: original

Summary for minion
------------
Succeeded: 4
Failed:    0
------------
Total states run:     4

Cleaned up salt output:

minion:
----------
          ID: notify-before-update
    Function: test.show_notification
        Name: Original update_map
     Comment: original
----------
          ID: notify-after-update
    Function: test.show_notification
        Name: Update result
     Comment: new
----------
          ID: notify-before-merge
    Function: test.show_notification
        Name: Original merge_map
     Comment: original
----------
          ID: notify-after-merge
    Function: test.show_notification
        Name: Merge result
     Comment: original

Summary for minion
------------
Succeeded: 4
Failed:    0
------------
Total states run:     4

Observe
The notify-after-update reveals different values for the update_map dictionary depending on the command used.

In contrast, notify-before-merge and notify-after-merge are the same between commands and are included to demonstrate that slsutil.merge does not have this issue.

Expected behavior
I expect the same configuration to be applied with both the salt and salt-ssh commands.

Versions Report

salt --versions-report (Provided by running salt --versions-report. Please also mention any differences in master/minion versions.)
Salt Version:
          Salt: 3006.2
 
Python Version:
        Python: 3.11.4 (main, Jun  7 2023, 00:00:00) [GCC 13.1.1 20230511 (Red Hat 13.1.1-2)]
 
Dependency Versions:
          cffi: 1.15.1
      cherrypy: Not Installed
      dateutil: 2.8.2
     docker-py: 5.0.3
         gitdb: Not Installed
     gitpython: Not Installed
        Jinja2: 3.0.3
       libgit2: Not Installed
  looseversion: 1.3.0
      M2Crypto: Not Installed
          Mako: Not Installed
       msgpack: 1.0.4
  msgpack-pure: Not Installed
  mysql-python: Not Installed
     packaging: 23.0
     pycparser: 2.20
      pycrypto: Not Installed
  pycryptodome: 3.17.0
        pygit2: Not Installed
  python-gnupg: Not Installed
        PyYAML: 6.0
         PyZMQ: 24.0.1
        relenv: Not Installed
         smmap: Not Installed
       timelib: Not Installed
       Tornado: 4.5.3
           ZMQ: 4.3.4
 
System Versions:
          dist: fedora 38 
        locale: utf-8
       machine: x86_64
       release: 6.4.11-200.fc38.x86_64
        system: Linux
       version: Fedora Linux 38

Additional context
{% do salt['slsutil.update'](update_map, new) %} functions as I would expect when called with salt. By simply changing the command to salt-ssh, a different configuration is applied as the dictionary merge functions differently. This is troublesome when states are built upon this do salt['slsutil.update'] statement and successfully deployed with salt. If the need arises to use salt-ssh, the same input configuration produces a different output in an unexpected way.

Notable situations where salt and salt-ssh produce the same result:
{% do salt['slsutil.merge'](update_map, new) %}: slsutil.merge creates a deepcopy before manipulating the dictionary. This produces consistent results.
{% set final_map = salt['slsutil.update'](update_map, new) %}: Explicitly assigning the results to a variable using set rather than do produces consistent results.

Thank you.

@FreeOni FreeOni added Bug broken, incorrect, or confusing behavior needs-triage labels Aug 28, 2023
@welcome
Copy link

welcome bot commented Aug 28, 2023

Hi there! Welcome to the Salt Community! Thank you for making your first contribution. We have a lengthy process for issues and PRs. Someone from the Core Team will follow up as soon as possible. In the meantime, here’s some information that may help as you continue your Salt journey.
Please be sure to review our Code of Conduct. Also, check out some of our community resources including:

There are lots of ways to get involved in our community. Every month, there are around a dozen opportunities to meet with other contributors and the Salt Core team and collaborate in real time. The best way to keep track is by subscribing to the Salt Community Events Calendar.
If you have additional questions, email us at saltproject@vmware.com. We’re glad you’ve joined our community and look forward to doing awesome things with you!

@anilsil anilsil added this to the Sulfur v3006.4 milestone Aug 29, 2023
@lkubb
Copy link
Contributor

lkubb commented Aug 29, 2023

This is caused by a missing SSH wrapper module for slsutil. Most of its functions should be executed on the caller's side (that does the rendering/compilation into lowstate), not the target.

@dwoz
Copy link
Contributor

dwoz commented Jun 18, 2024

@lkubb Can this fix be back ported to 3006.x? If not, I think we should close this issue as resolved in 3007.x

@dwoz dwoz unassigned Ch3LL Jun 18, 2024
@lkubb
Copy link
Contributor

lkubb commented Jun 18, 2024

@dwoz I can backport this fix.

It's part of a whole list of Salt-SSH issues that are resolved in either 3006 or 3007, but still open:

Already fixed in 3006.5+:

#36796
#59802
#60002
#61861
#62230
#64575

Fixed in 3007.0+:

#48067 (backport a bit more involved, adds 2 wrapper modules)
#50196 (backport possible, adds 1 wrapper)
#51605 (backport possible, adds 1 wrapper)
#56441 (backport possible, updates existing wrapper)
#61100 (backport possible, updates existing wrapper)
#61143 (backport possible, adds 1 wrapper)
#62313 (backport more involved)
#65067 (this one, backport possible, adds 1 wrapper)

Fixed in 3007.0+, already closed:
#65630 (backport possible, adds 1 wrapper)

I can add the trivial ones to the backport PR as well if desired.

@dwoz
Copy link
Contributor

dwoz commented Jun 22, 2024

I can add the trivial ones to the backport PR as well if desired.

Please do, thank you for the help.

@dwoz
Copy link
Contributor

dwoz commented Jul 29, 2024

This is fixed in 3006.9.

@dwoz dwoz closed this as completed Jul 29, 2024
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 Salt-SSH
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants