collectd-formula
collectd-formula copied to clipboard
fix(salt-ssh): jinja include is unusable in `file.managed` templates
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 - [x]
[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
#161
Describe the changes you're proposing
Caching the jinja include with a file.cached
workaround the issue as explained on the tracker.
Each file.managed
state must require the caching state.
Pillar / config required to test the proposed changes
N/A
Debug log showing how the proposed changes work
salt-ssh 'vm-106' state.apply collectd.disk test=True
vm-106:
----------
ID: collectd
Function: pkg.installed
Name: collectd-core
Result: None
Comment: The following packages would be installed/updated: collectd-core
Started: 09:15:51.123353
Duration: 66.7 ms
Changes:
----------
collectd-core:
----------
new:
installed
old:
----------
ID: /etc/collectd/plugins
Function: file.directory
Result: None
Comment: The following files will be changed:
/etc/collectd/plugins: directory - new
Started: 09:15:51.193806
Duration: 4.948 ms
Changes:
----------
/etc/collectd/plugins:
----------
directory:
new
----------
ID: collectd-workaround-salt-ssh-map.jinja-file.cached
Function: file.cached
Name: salt://collectd/map.jinja
Result: True
Comment: File is cached to /var/tmp/.root_8dccac_salt/running_data/var/cache/salt/minion/files/base/collectd/map.jinja
Started: 09:15:51.199325
Duration: 26.901 ms
Changes:
----------
ID: /etc/collectd/plugins/disk.conf
Function: file.managed
Result: None
Comment: The file /etc/collectd/plugins/disk.conf is set to be changed
Note: No changes made, actual changes may
be different due to other states.
Started: 09:15:51.226406
Duration: 55.482 ms
Changes:
----------
newfile:
/etc/collectd/plugins/disk.conf
----------
ID: /etc/collectd/collectd.conf
Function: file.managed
Result: None
Comment: The file /etc/collectd/collectd.conf is set to be changed
Note: No changes made, actual changes may
be different due to other states.
Started: 09:15:51.282082
Duration: 31.542 ms
Changes:
----------
newfile:
/etc/collectd/collectd.conf
----------
ID: /etc/collectd/plugins/default.conf
Function: file.managed
Result: None
Comment: The file /etc/collectd/plugins/default.conf is set to be changed
Note: No changes made, actual changes may
be different due to other states.
Started: 09:15:51.313841
Duration: 30.533 ms
Changes:
----------
newfile:
/etc/collectd/plugins/default.conf
----------
ID: collectd-service
Function: service.running
Name: collectd
Result: None
Comment: Service is set to be started
Started: 09:15:51.372553
Duration: 11.059 ms
Changes:
Summary for vm-106
------------
Succeeded: 7 (unchanged=6, changed=5)
Failed: 0
------------
Total states run: 7
Total run time: 227.165 ms
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
Hi, could this new state 'workaround' only be triggered when using salt-ssh?
About the purpose of this PR, I'm not confortable with this hacky workaround. It will need every one of the formulas to be modified with this hack. Last time I used salt-ssh, on salt 3001.3, it was possible to add a patch to salt codebase and configure extra_filerefs
to cache all salt://
files.
I feel that a single change user side is better than hundred hacks on formulas side.
Do we know if saltproject works on this problem, and salt-ssh is considered by them as a first-class salt client, or is it only a second-class one?
Hi, could this new state 'workaround' only be triggered when using salt-ssh?
It could, with the help of libsaltcli.jinja but I don't see the benefit since it's just a precaching of the file that salt on minion will does.
About the purpose of this PR, I'm not confortable with this hacky workaround. It will need every one of the formulas to be modified with this hack. Last time I used salt-ssh, on salt 3001.3, it was possible to add a patch to salt codebase and configure
extra_filerefs
to cache allsalt://
files. I feel that a single change user side is better than hundred hacks on formulas side.
Caching the whole salt://
tree on each client is a little bit too much for me.
Running salt-ssh 'vm-106' state.apply collectd.disk test=True
takes 12 minutes to run with --extra-filerefs
(and my PR) compared to 2 minutes without.
Do we know is saltproject works on this problem, and salt-ssh is considered by them as a first-class salt client, or is it only a second-class one?
I asked in salt#21370 but I really wonder in which direction it's going 😟