fix(libtofs): “files_switch” mess up the variable exported by “map.jinja” #187

baby-gnu · 2020-02-13T20:08:58Z

PR progress checklist (to be filled in by reviewers)

Changes to documentation are appropriate (or tick if not required)
Changes to tests are appropriate (or tick if not required)
Reviews completed

What type of PR is this?

Primary type

[build] Changes related to the build system
[chore] Changes to the build process or auxiliary tools and libraries such as documentation generation
[ci] Changes to the continuous integration configuration
[feat] A new feature
[fix] A bug fix
[perf] A code change that improves performance
[refactor] A code change that neither fixes a bug nor adds a feature
[revert] A change used to revert a previous commit
[style] Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc.)

Secondary type

[docs] Documentation changes
[test] Adding missing or correcting existing tests

Does this PR introduce a `BREAKING CHANGE`?

No.

Related issues and/or pull requests

This fix was exposed by #186 and must be merged before it.

Describe the changes you're proposing

files_switch mess up the variable exported by map.jinja

map.jinja export a variable name as <tplroot>
import of libtofs.jinja is done with context and has access to <tplroot> variable
in files_switch, appending the empty string to the fsl variable modify globally <tplroot>['tofs']['files_switch']

TEMPLATE/libtofs.jinja: initialise fsl with a copy of files_switch_list instead of a reference.

Pillar / config required to test the proposed changes

Debug log showing how the proposed changes work

Documentation checklist

Updated the README (e.g. Available states).
Updated pillar.example.

Testing checklist

Included in Kitchen (i.e. under state_top).
Covered by new/existing tests (e.g. InSpec, Serverspec, etc.).
Updated the relevant test pillar.

Additional context

baby-gnu · 2020-02-13T20:44:50Z

I need to find a way to copy under python 2…

- “map.jinja” export a variable name as “<tplroot>” - import of “libtofs.jinja” is done with context and has access to “<tplroot>” variable - in “files_switch”, appending the empty string to the “fsl” variable modify globally “<tplroot>['tofs']['files_switch']” * TEMPLATE/libtofs.jinja: do not use inplace “append” to avoid side effect.

baby-gnu · 2020-02-14T07:58:57Z

I think it's better to just avoid inplace fsl.append instead of doing an obscure slice ;-)

myii · 2020-02-14T12:48:21Z

@baby-gnu Is this ready for review now?

baby-gnu · 2020-02-14T13:28:53Z

@baby-gnu Is this ready for review now?

Yes, this fix is finished.

baby-gnu · 2020-02-14T13:37:49Z

The only hypothesis I have for this problem is that salt['config.get'] use some kind of caching and return the same object when called multiple times.

Combined with possible differences between salt['grains.filter_by'] (which build new objects) and salt.slsutil.merge (which probably only copy references directly).

myii · 2020-02-14T16:36:51Z

@baby-gnu Try as I might, I can't reproduce the original issue. Would you mind sharing the steps to reproduce it? I've tested the changes and the output of the map before and after are identical (using pillar.example via. config.get):

[ERROR   ] map
added_in_defaults: defaults_value
added_in_lookup: lookup_value
added_in_pillar: pillar_value
arch: amd64
config: /etc/template-formula.conf
lookup:
  added_in_lookup: lookup_value
  master: template-master
  winner: lookup
master: template-master
pkg:
  name: bash
rootgroup: root
service:
  name: systemd-udevd
subcomponent:
  config: /etc/TEMPLATE-subcomponent-formula.conf
tofs:
  files_switch:
  - any/path/can/be/used/here
  - id
  - roles
  - osfinger
  - os
  - os_family
  source_files:
    TEMPLATE-config-file-file-managed:
    - example.tmpl.jinja
    TEMPLATE-subcomponent-config-file-file-managed:
    - subcomponent-example.tmpl.jinja

baby-gnu · 2020-02-14T17:10:42Z

I had the problem when implementing #186.

The problem can be view by testing #186 without the fix proposed by this PR.

myii · 2020-02-14T17:12:14Z

@baby-gnu OK, I'll try that when I get a chance.

baby-gnu · 2020-02-14T18:00:40Z

I discovered the problem when implementing #186, there is a trailing "" at the end of tofs:files_switch list.

In the failing logs we have:

        TEMPLATE-config-file-file-managed:
		[...]
        TEMPLATE: {"added_in_defaults": "defaults_value", "added_in_lookup": "lookup_value", "added_in_pillar": "pillar_value", "arch": "amd64", "config": "/etc/template-formula.conf", "lookup": {"added_in_lookup": "lookup_value", "master": "template-master", "winner": "lookup"}, "master": "template-master", "pkg": {"name": "bash"}, "rootgroup": "root", "service": {"name": "systemd-udevd"}, "subcomponent": {"config": "/etc/TEMPLATE-subcomponent-formula.conf"}, "tofs": {"files_switch": ["any/path/can/be/used/here", "id", "roles", "osfinger", "os", "os_family", ""], "source_files": {"TEMPLATE-config-file-file-managed": ["example.tmpl.jinja"], "TEMPLATE-subcomponent-config-file-file-managed": ["subcomponent-example.tmpl.jinja"]}}, "winner": "pillar"}

To reproduce:

Create a pseudo formula TEST:
```
mkdir -p /srv/salt/TEST/files/default/
```
Install libtofs.jinja in /srv/salt/TEST

Create a minimalist sls /srv/salt/TEST/init.sls:

# -*- coding: utf-8 -*-
# vim: ft=sls

{#- Get the `tplroot` from `tpldir` #}
{%- set tplroot = tpldir.split('/')[0] %}

{%- set TEST_VAR = salt.config.get('TEST', {}) %}

{%- from tplroot ~ "/libtofs.jinja" import files_switch with context %}

Test a simple file:
  file.managed:
    - name: /tmp/test.txt
    - source: {{ files_switch(['test.txt.jinja']) }}
    - template: jinja
    - context:
        TEST: {{ TEST_VAR }}

Define a dedicated pillar /srv/pillar/TEST.sls:

TEST:
  tofs:
    files_switch:
      - 'a'
      - 'b'
      - 'c'

Associate the pillar to your host in /srv/pillar/top.sls

Here is the debug output of salt-call --local -l debug state.show_sls TEST showing the mess up:

[DEBUG   ] compile template: /var/cache/salt/minion/files/base/TEST/init.sls
[DEBUG   ] Jinja search path: ['/var/cache/salt/minion/files/base']
[DEBUG   ] Created gitfs object with uninitialized remotes
[DEBUG   ] LazyLoaded gitfs.envs
[DEBUG   ] LazyLoaded roots.envs
[DEBUG   ] Re-using gitfs object for process 5534
[DEBUG   ] Could not LazyLoad roots.init: 'roots.init' is not available.
[DEBUG   ] Re-using gitfs object for process 5534
[DEBUG   ] Re-using gitfs object for process 5534
[DEBUG   ] In saltenv 'base', looking at rel_path 'TEST/libtofs.jinja' to resolve 'salt://TEST/libtofs.jinja'
[DEBUG   ] In saltenv 'base', ** considering ** path '/var/cache/salt/minion/files/base/TEST/libtofs.jinja' to resolve 'salt://TEST/libtofs.jinja'
[DEBUG   ] key: TEST:tofs:path_prefix, ret: _|-
[DEBUG   ] key: TEST:tofs:dirs:files, ret: _|-
[DEBUG   ] key: TEST:tofs:files:None, ret: _|-
[DEBUG   ] key: TEST:tofs:source_files:None, ret: _|-
[DEBUG   ] key: TEST:files_switch, ret: _|-
[DEBUG   ] key: a, ret: _|-
[DEBUG   ] key: b, ret: _|-
[DEBUG   ] key: c, ret: _|-
[DEBUG   ] key: TEST:tofs:dirs:default, ret: _|-
[PROFILE ] Time (in seconds) to render '/var/cache/salt/minion/files/base/TEST/init.sls' using 'jinja' renderer: 0.07607769966125488
[DEBUG   ] Rendered data from file: /var/cache/salt/minion/files/base/TEST/init.sls:
# -*- coding: utf-8 -*-
# vim: ft=sls

Test a simple file:
  file.managed:
    - name: /tmp/test.txt

    - source: 
      - salt://TEST/files/a/test.txt.jinja
      - salt://TEST/files/b/test.txt.jinja
      - salt://TEST/files/c/test.txt.jinja
      - salt://TEST/files/default/test.txt.jinja
    - template: jinja
    - context:
        TEST: {'tofs': {'files_switch': ['a', 'b', 'c', '']}}

To prove that several calls to salt.config.get return the same objects, so the fsl.append('') in libtofs.jinja modify the list in some cache, I defined a new sls /srv/salt/TEST/test2.sls:

# -*- coding: utf-8 -*-
# vim: ft=sls

{#- Get the `tplroot` from `tpldir` #}
{%- set tplroot = tpldir.split('/')[0] %}

{%- set TEST_VAR = salt.config.get('TEST', {}) %}
{%- do salt.log.debug('TEST_VAR is: ' ~ TEST_VAR) %}

{%- set TEST_VAR2 = salt.config.get('TEST', {}) %}
{%- do salt.log.debug('TEST_VAR2 is: ' ~ TEST_VAR2) %}

{%- set fsl = TEST_VAR2['tofs']['files_switch'] %}
{%- do salt.log.debug('fsl is: ' ~ fsl) %}

{%- do fsl.append('x') %}
{%- do salt.log.debug('fsl after append is: ' ~ fsl) %}
{%- do salt.log.debug('TEST_VAR after append is: ' ~ TEST_VAR) %}
{%- do salt.log.debug('TEST_VAR2 after append is: ' ~ TEST_VAR2) %}

{%- set TEST_VAR3 = salt.config.get('TEST', {}) %}
{%- do salt.log.debug('TEST_VAR3 is: ' ~ TEST_VAR3) %}

Test which do nothing:
  test.nop:
    - name: {{ TEST_VAR }}

Here is the output of salt-call --local -l debug state.show_sls TEST.test2:

[DEBUG   ] compile template: /var/cache/salt/minion/files/base/TEST/test2.sls
[DEBUG   ] Jinja search path: ['/var/cache/salt/minion/files/base']
[DEBUG   ] Created gitfs object with uninitialized remotes
[DEBUG   ] LazyLoaded gitfs.envs
[DEBUG   ] LazyLoaded roots.envs
[DEBUG   ] Re-using gitfs object for process 5886
[DEBUG   ] Could not LazyLoad roots.init: 'roots.init' is not available.
[DEBUG   ] TEST_VAR is: OrderedDict([('tofs', OrderedDict([('files_switch', ['a', 'b', 'c'])]))])
[DEBUG   ] TEST_VAR2 is: OrderedDict([('tofs', OrderedDict([('files_switch', ['a', 'b', 'c'])]))])
[DEBUG   ] fsl is: ['a', 'b', 'c']
[DEBUG   ] fsl after append is: ['a', 'b', 'c', 'x']
[DEBUG   ] TEST_VAR after append is: OrderedDict([('tofs', OrderedDict([('files_switch', ['a', 'b', 'c', 'x'])]))])
[DEBUG   ] TEST_VAR2 after append is: OrderedDict([('tofs', OrderedDict([('files_switch', ['a', 'b', 'c', 'x'])]))])
[DEBUG   ] TEST_VAR3 is: OrderedDict([('tofs', OrderedDict([('files_switch', ['a', 'b', 'c', 'x'])]))])
[PROFILE ] Time (in seconds) to render '/var/cache/salt/minion/files/base/TEST/test2.sls' using 'jinja' renderer: 0.05097055435180664
[DEBUG   ] Rendered data from file: /var/cache/salt/minion/files/base/TEST/test2.sls:
# -*- coding: utf-8 -*-
# vim: ft=sls

Test which do nothing:
  test.nop:
    - name: {'tofs': {'files_switch': ['a', 'b', 'c', 'x']}}

[DEBUG   ] Results of YAML rendering: 
OrderedDict([('Test which do nothing', OrderedDict([('test.nop', [OrderedDict([('name', OrderedDict([('tofs', OrderedDict([('files_switch', ['a', 'b', 'c', 'x'])]))]))])])]))])
[PROFILE ] Time (in seconds) to render '/var/cache/salt/minion/files/base/TEST/test2.sls' using 'yaml' renderer: 0.0007266998291015625
[DEBUG   ] LazyLoaded nested.output
local:
    ----------
    Test which do nothing:
        ----------
        test:
            |_
              ----------
              name:
                  ----------
                  tofs:
                      ----------
                      files_switch:
                          - a
                          - b
                          - c
                          - x
            - nop
            |_
              ----------
              order:
                  10000
        __sls__:
            TEST.test2
        __env__:
            base

myii · 2020-02-14T18:20:22Z

@baby-gnu Thanks for the detailed explanation, I was finally able to reproduce it locally (and confirm the fix), but only when using file.managed with context. Next step is to propagate this through all of the formulas using libtofs.jinja...

saltstack-formulas-travis · 2020-02-14T18:30:21Z

🎉 This PR is included in version 4.0.4 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

…inja” * saltstack-formulas/template-formula#187

myii · 2020-02-14T19:11:11Z

@baby-gnu The change has been propagated throughout the formulas using myii/ssf-formula#131.

baby-gnu · 2020-02-14T22:19:21Z

Thanks a lot @myii

baby-gnu requested a review from myii February 13, 2020 20:11

baby-gnu force-pushed the bugfix/libtofs-side-effect branch from a2d4a47 to e1629a1 Compare February 13, 2020 20:12

baby-gnu changed the title ~~bug(tofs): “files_switch” mess up the variable exported by “map.jinja”~~ bug(libtofs): “files_switch” mess up the variable exported by “map.jinja” Feb 13, 2020

baby-gnu force-pushed the bugfix/libtofs-side-effect branch from e1629a1 to 3dc1f03 Compare February 13, 2020 20:17

baby-gnu changed the title ~~bug(libtofs): “files_switch” mess up the variable exported by “map.jinja”~~ fix(libtofs): “files_switch” mess up the variable exported by “map.jinja” Feb 13, 2020

baby-gnu mentioned this pull request Feb 13, 2020

feat(map.jinja): load configuration values from configurable sources #186

Closed

19 tasks

baby-gnu force-pushed the bugfix/libtofs-side-effect branch from 3dc1f03 to 80d58d2 Compare February 13, 2020 21:56

baby-gnu force-pushed the bugfix/libtofs-side-effect branch from 80d58d2 to ab4ce75 Compare February 14, 2020 07:57

myii approved these changes Feb 14, 2020

View reviewed changes

myii merged commit 475fe40 into saltstack-formulas:master Feb 14, 2020

baby-gnu deleted the bugfix/libtofs-side-effect branch February 14, 2020 18:21

saltstack-formulas-travis added the released label Feb 14, 2020

myii added a commit to myii/ssf-formula that referenced this pull request Feb 14, 2020

feat(libtofs): “files_switch” mess up the variable exported by “map.j…

c56ed35

…inja” * saltstack-formulas/template-formula#187

myii mentioned this pull request Feb 14, 2020

feat(libtofs): “files_switch” mess up the variable exported by “map.jinja” myii/ssf-formula#131

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(libtofs): “files_switch” mess up the variable exported by “map.jinja” #187

fix(libtofs): “files_switch” mess up the variable exported by “map.jinja” #187

baby-gnu commented Feb 13, 2020

baby-gnu commented Feb 13, 2020

baby-gnu commented Feb 14, 2020

myii commented Feb 14, 2020

baby-gnu commented Feb 14, 2020

baby-gnu commented Feb 14, 2020

myii commented Feb 14, 2020

baby-gnu commented Feb 14, 2020

myii commented Feb 14, 2020

baby-gnu commented Feb 14, 2020

myii commented Feb 14, 2020

saltstack-formulas-travis commented Feb 14, 2020

myii commented Feb 14, 2020

baby-gnu commented Feb 14, 2020

fix(libtofs): “files_switch” mess up the variable exported by “map.jinja” #187

fix(libtofs): “files_switch” mess up the variable exported by “map.jinja” #187

Conversation

baby-gnu commented Feb 13, 2020

PR progress checklist (to be filled in by reviewers)

What type of PR is this?

Primary type

Secondary type

Does this PR introduce a BREAKING CHANGE?

Related issues and/or pull requests

Describe the changes you're proposing

Pillar / config required to test the proposed changes

Debug log showing how the proposed changes work

Documentation checklist

Testing checklist

Additional context

baby-gnu commented Feb 13, 2020

baby-gnu commented Feb 14, 2020

myii commented Feb 14, 2020

baby-gnu commented Feb 14, 2020

baby-gnu commented Feb 14, 2020

myii commented Feb 14, 2020

baby-gnu commented Feb 14, 2020

myii commented Feb 14, 2020

baby-gnu commented Feb 14, 2020

myii commented Feb 14, 2020

saltstack-formulas-travis commented Feb 14, 2020

myii commented Feb 14, 2020

baby-gnu commented Feb 14, 2020

Does this PR introduce a `BREAKING CHANGE`?