Closed LloydArmstrong closed 5 years ago
@LloydArmstrong Thanks for your report. The issue here is that the lookup
isn't being currently being merged in map.jinja
. Since there are numerous values shown under lookup
in the pillar.example
, I believe the correct way to proceed would ensure it is being merged in map.jinja
, rather than dropping overcommit_memory
from the block.
@LloydArmstrong I've proposed a fix (#76). If this passes the review and is merged, this issue should be resolved. You can test the fix in the meantime if you wish and your feedback would be very welcome.
@myii Ok great! Thanks a lot for the speedy fix. I will test it from my side and if there's still any issue, I'll let you know. Otherwise, thanks again.
@LloydArmstrong You're welcome.
@myii Hello again. I've been encountering an error since the new merging update you did, where salt fails to apply due to a has no attribute 'cfg_name'
error. I'm unsure as to whether this issue is local to myself, or is a legitimate bug related to the lookup
block. From the changes you made, nothing sticks out as obvious to me. Again, any feedback will be greatly appreciated.
@LloydArmstrong OK, cfg_name
is set in one of the following places.
Debian:
cfg_name: /etc/redis/redis.conf
RedHat:
cfg_name: /etc/redis.conf
Arch:
cfg_name: /etc/redis.conf
FreeBSD:
cfg_name: /usr/local/etc/redis.conf
Or via. the pillar:
redis:
...
lookup:
...
cfg_name: /etc/redis.conf
Questions:
redis:lookup:cfg_name
)?@myii
I am running Ubuntu 18.04.
I have cfg_name: /etc/redis/redis.conf
set in my pillar correctly within the lookup block (as in the pillar.example
file). [Just some extra info, this of course still failed with the path set to /etc/redis.conf
like in the pillar.example
file as the correct path is /etc/redis/redis.conf
for my distro]
What do you think? Remove it and let the default get set from ?osfamilymap.jinja
EDIT: I mean osfamilymap.yaml
. My mistake!
- I am running Ubuntu 18.04.
In that case, you don't need to use a value in the pillar at all -- it will be picked up automatically from osfamilymap.yaml
.
- I have
cfg_name: /etc/redis/redis.conf
set in my pillar correctly within the lookup block (as in thepillar.example
file). [Just some extra info, this of course still failed with the path set to/etc/redis.conf
like in thepillar.example
file as the correct path is /etc/redis/redis.conf]What do you think? Remove it and let the default get set from
osfamilymap.jinja
?
Probably a typo but there isn't an osfamilymap.jinja
! If you have modified the files at your end, consider resetting them to match the latest version in this repo. In any case, let it get the default value itself without resorting to setting the same value in the pillar again unnecessarily.
Hope this helps, @LloydArmstrong.
@myii Apologies for the previous error. I meant osfamilymap.yaml
. My mistake.
Ok, I have removed all unnecessary (redundant) options in my pillar so it looks as follows:
redis:
root_dir: /var/lib/redis
user: redis
port: 6379
bind: 0.0.0.0
snapshots:
- '900 1'
- '300 10'
- '60 10000'
appendonly: 'yes'
lookup:
svc_state: running
overcommit_memory: False
source_path: salt://redis/files/redis-3.2-ng.conf.jinja
In addition to this I cloned a brand new copy onto my system so as to confirm the latest version is being used. Other than this, no modifications have been made. The same error is still present:
Rendering SLS 'base:redis.server' failed: Jinja variable 'dict object' has no attribute 'cfg_name'
@LloydArmstrong Let's get the output of the rendered map.jinja
.
Create the file redis-formula/redis/test.sls
with the following content:
{% from "redis/map.jinja" import redis_settings with context %}
test-map-redis:
cmd.run:
- name: |
python -c "import pprint; pprint.pprint({{ redis_settings }})" | tee /tmp/salt_test
For even nicer output, if you have jq
installed, change the pprint
line to this instead:
python -c "import json; jd=json.dumps({{ redis_settings }}, sort_keys=True); print(jd)" | jq . | tee /tmp/salt_test.json
Run this using something like:
# salt -Cv '<minion_id>' state.sls redis.test
Open the file /tmp/salt_test
or /tmp/salt_test.json
, remove any lines with sensitive data and then paste here, please.
@myii I installed jq
and here is the output written to /tmp/salt_test.json
:
{
"aof_load_truncated": "yes",
"appendfilename": "appendonly.aof",
"appendfsync": "everysec",
"appendonly": "yes",
"auto_aof_rewrite_min_size": "64mb",
"auto_aof_rewrite_percentage": 100,
"bin": "/usr/local/bin/redis-server",
"bind": "0.0.0.0",
"daemonize": "yes",
"database_count": 16,
"dbfilename": "dump.rdb",
"group": "redis",
"home": "/var/lib/redis",
"install_from": "package",
"latency_monitor_threshold": 0,
"loglevel": "notice",
"lookup": {
"overcommit_memory": false,
"svc_state": "running"
},
"lua_time_limit": 5000,
"maxmemory_policy": "volatile-lru",
"maxmemory_samples": 3,
"no_appendfsync_on_rewrite": "no",
"notify_keyspace_events": "",
"overcommit_memory": true,
"port": 6379,
"rdbchecksum": "yes",
"rdbcompression": "yes",
"repl_disable_tcp_nodelay": "no",
"repl_diskless_sync": "no",
"repl_diskless_sync_delay": 5,
"root_dir": "/var/lib/redis",
"sentinel": {
"daemonize": "yes",
"dir": "/var/lib/redis",
"group": "redis",
"pidfile": "/var/run/redis/sentinel.pid",
"port": 26379,
"user": "redis"
},
"slave_priority": 100,
"slave_read_only": "yes",
"slave_serve_stale_data": "yes",
"slowlog_log_slower_than": 10000,
"slowlog_max_len": 128,
"snapshots": [
"900 1",
"300 10",
"60 10000"
],
"stop_writes_on_bgsave_error": "yes",
"svc_onboot": true,
"svc_state": "running",
"tcp_backlog": 511,
"tcp_keepalive": 0,
"timeout": 0,
"unixsocketperm": 755,
"user": "redis"
}
@LloydArmstrong OK, so none of the values from osfamilymap.yaml
are coming through:
For the same minion, could you paste the output of grains.get os_family
?
# salt -Cv '<minion_id>' grains.get os_family
Yeah I noticed there were variables missing.
The minion returns Debian
.
@LloydArmstrong above:
The minion returns
Debian
.
So these values shouldn't be missing.
@LloydArmstrong above:
{ ... "lookup": { "overcommit_memory": false, "svc_state": "running" }, ... "overcommit_memory": true, ... }
This definitely isn't using the latest changes, since the overcommit_memory
value from the lookup
block isn't being applied to the map properly.
So coupling this with Debian
above suggests a misconfiguration of some sort.
Another test could be to comment out the redis
pillar completely (or move the file) and see what reaches map.jinja
like that.
@LloydArmstrong above:
In addition to this I cloned a brand new copy onto my system so as to confirm the latest version is being used. Other than this, no modifications have been made. ...
Did you update this in the same directory as is configured for file_roots
in the master's config? Could also check for the value of file_roots
in the minion's config.
@myii I see your point about the redundant overcommit_memory
in the above. When I remove the pillar from the equation, and run the test.sls
, it all remains the same bar the few lines from the lookup block.
Did you update this in the same directory as is configured for file_roots in the master's config? Could also check for the value of file_roots in the minion's config.
Yes, the newly cloned formula was placed in the same location as before and the path in the master
file is correct. I though it may be an issue with the pillar not refreshing correctly on the box but that was found to be ok due to the below.
I am using salt-ssh
to control this minion as it is a development box, and all our live salt minions pull from the same repo's (I don't want to pull the latest redis-formula
onto live boxes until this issue is ironed out). Using Salt-SSH shouldn't be too much of an issue as far as I understand it as salt-ssh compiles the pillar for each run, so there should never be a need to refresh it. However I thought it was worth a mention.
@LloydArmstrong above:
... I see your point about the redundant
overcommit_memory
in the above.
It's not that there are two values for overcommit_memory
but that the value from the lookup
would have been applied to the main map if the recent changes were working. In other words, it should be:
{
"lookup": {
"overcommit_memory": false,
"svc_state": "running"
},
"overcommit_memory": false,
}
@LloydArmstrong above:
When I remove the pillar from the equation, and run the
test.sls
, it all remains the same bar the few lines from the lookup block.
So only the values from default.yaml are coming through.
@LloydArmstrong above:
... I though it may be an issue with the pillar not refreshing correctly on the box but that was found to be ok due to the below.
Since we were testing without the pillar, this doesn't matter much. It still doesn't explain why osfamilymap.yaml isn't being merged into the map.
A couple more things we can try:
ls -al redis-formula/redis/
redis-formula/redis/map.jinja
redis-formula/redis/osfamilymap.yaml
redis-formula/redis/osfingermap.yaml
1. Here is the output for ls -al redis-formula/redis/
total 24
drwxrwxrwx 1 lloyd lloyd 4096 Jan 31 11:18 .
drwxrwxrwx 1 lloyd lloyd 4096 Jan 31 10:48 ..
-rwxrwxrwx 1 lloyd lloyd 1860 Jan 31 10:48 common.sls
-rwxrwxrwx 1 lloyd lloyd 1298 Jan 31 10:48 defaults.yaml
drwxrwxrwx 1 lloyd lloyd 4096 Jan 31 10:48 files
-rwxrwxrwx 1 lloyd lloyd 26 Jan 31 10:48 init.sls
-rwxrwxrwx 1 lloyd lloyd 827 Jan 31 10:48 map.jinja
-rwxrwxrwx 1 lloyd lloyd 1226 Jan 31 10:48 osfamilymap.yaml
-rwxrwxrwx 1 lloyd lloyd 259 Jan 31 10:48 osfingermap.yaml
-rwxrwxrwx 1 lloyd lloyd 2532 Jan 31 10:48 sentinel.sls
-rwxrwxrwx 1 lloyd lloyd 3154 Jan 31 10:48 server.sls
-rwxrwxrwx 1 lloyd lloyd 239 Jan 31 11:24 test.sls
2.
i. Here is the map.jinja
file:
# -*- coding: utf-8 -*-
# vim: ft=jinja
{% import_yaml 'redis/defaults.yaml' as defaults %}
{% import_yaml 'redis/osfamilymap.yaml' as osfamilymap %}
{% import_yaml 'redis/osfingermap.yaml' as osfingermap %}
{# merge the osfamilymap #}
{% set osfamily = salt['grains.filter_by'](osfamilymap, grain='os_family') or{} %}
{% do salt['defaults.merge'](defaults['redis'], osfamily) %}
{# merge the osfingermap #}
{% set osfinger = salt['grains.filter_by'](osfingermap, grain='osfinger') or {} %}
{% do salt['defaults.merge'](defaults['redis'], osfinger) %}
{# merge the lookup #}
{% set lookup = salt['pillar.get']('redis:lookup', default={}, merge=True) %}
{% do salt['defaults.merge'](defaults['redis'], lookup) %}
{# merge all #}
{% set redis_settings = salt['pillar.get']('redis', default=defaults['redis'], merge=True) %}
ii. Here is redis-formula/redis/osfamilymap.yaml
# -*- coding: utf-8 -*-
# vim: ft=yaml
Debian:
pkg_name: redis-server
python_dev_package: python-dev
svc_name: redis-server
cfg_name: /etc/redis/redis.conf
cfg_version: '3.2'
logfile: /var/log/redis/redis-server.log
pidfile: /var/run/redis/redis-server.pid
sentinel_pkg: redis-server
sentinel_service: redis-sentinel
sentinel_cfg: /etc/redis/sentinel.conf
sentinel_logfile: /var/log/redis/redis-sentinel.log
RedHat:
pkg_name: redis
python_dev_package: python-devel
svc_name: redis
cfg_name: /etc/redis.conf
cfg_version: '3.0'
logfile: /var/log/redis/redis.log
pidfile: /var/run/redis.pid
sentinel_pkg: redis
sentinel_service: redis-sentinel
sentinel_cfg: /etc/redis-sentinel.conf
sentinel_logfile: /var/log/redis/sentinel.log
sentinel:
pidfile: /var/run/redis/sentinel.pid
Arch:
pkg_name: redis
svc_name: redis
cfg_name: /etc/redis.conf
cfg_version: '3.0'
logfile: /var/log/redis/redis.log
pidfile: /var/run/redis.pid
daemonize: no
FreeBSD:
pkg_name: redis
svc_name: redis
cfg_name: /usr/local/etc/redis.conf
cfg_version: '3.0'
logfile: /var/log/redis/redis.log
pidfile: /var/run/redis/redis.pid
overcommit_memory: False
root_dir: /var/db/redis
iii. Here is redis-formula/redis/osfingermap.yaml
# -*- coding: utf-8 -*-
# vim: ft=yaml
Debian-7:
cfg_version: '2.4'
Ubuntu-16.04:
cfg_version: '3.0'
sentinel_pkg: redis-sentinel
CentOS Linux-7:
cfg_version: '3.2'
sentinel:
group: root
daemonize: 'no'
CentOS-6:
sentinel:
group: root
@LloydArmstrong Please paste the output of:
# salt --versions-report
Salt Version:
Salt: 2018.3.2
Dependency Versions:
cffi: Not Installed
cherrypy: Not Installed
dateutil: Not Installed
docker-py: Not Installed
gitdb: Not Installed
gitpython: Not Installed
ioflo: Not Installed
Jinja2: 2.10
libgit2: Not Installed
libnacl: Not Installed
M2Crypto: Not Installed
Mako: Not Installed
msgpack-pure: Not Installed
msgpack-python: 0.5.6
mysql-python: Not Installed
pycparser: Not Installed
pycrypto: 2.6.1
pycryptodome: Not Installed
pygit2: Not Installed
Python: 2.7.15rc1 (default, Apr 15 2018, 21:51:34)
python-gnupg: Not Installed
PyYAML: 3.13
PyZMQ: Not Installed
RAET: Not Installed
smmap: Not Installed
timelib: Not Installed
Tornado: 5.1
ZMQ: Not Installed
System Versions:
dist: Ubuntu 18.04 bionic
locale: UTF-8
machine: x86_64
release: 4.4.0-17134-Microsoft
system: Linux
version: Ubuntu 18.04 bionic
@LloydArmstrong Couple of notes, may not be significant:
Salt Version: Salt: 2018.3.2 Dependency Versions: Python: 2.7.15rc1 (default, Apr 15 2018, 21:51:34) System Versions: dist: Ubuntu 18.04 bionic
2018.3.2
: Newer version available.2.7.15rc1
: Python 3 is default on Ubuntu 18.04 bionic
.Some questions/requests:
# salt --versions-report
for the minion as well.dpkg -l | grep salt
on the master and minion.test.sls
with the following, to show what values are coming through from the .yaml
files:{% import_yaml 'redis/osfamilymap.yaml' as osfamilymap %}
{% import_yaml 'redis/osfingermap.yaml' as osfingermap %}
{% set osfamily = salt['grains.filter_by'](osfamilymap, grain='os_family') or{} %}
{% set osfinger = salt['grains.filter_by'](osfingermap, grain='osfinger') or {} %}
test-map-redis:
cmd.run:
- name: |
date | tee /tmp/salt_test.json
python -c "import json; jd=json.dumps({{ osfamilymap }}, sort_keys=True); print(jd)" | jq . | tee -a /tmp/salt_test.json
python -c "import json; jd=json.dumps({{ osfingermap }}, sort_keys=True); print(jd)" | jq . | tee -a /tmp/salt_test.json
python -c "import json; jd=json.dumps({{ osfamily }}, sort_keys=True); print(jd)" | jq . | tee -a /tmp/salt_test.json
python -c "import json; jd=json.dumps({{ osfinger }}, sort_keys=True); print(jd)" | jq . | tee -a /tmp/salt_test.json
osfamilymap
and osfingermap
first.grains.filter_by
.I think there is something wrong with the logic here:
the lookup
part in pillar was meant for overriding stuff in the salt states not in the pillar config that the states are setting.
@aboe76 Thanks for this, I'm glad to hear that there is some explanation about the lookup
. This goes back to the discussion in https://github.com/saltstack-formulas/apache-formula/pull/252. The user was complaining that we had recently changed it to:
- defaults < apache:lookup < modsecurity < osfamily < oscode < osfinger < apache
So we put it back to match how it was before those changes:
- defaults < modsecurity < osfamily < oscode < osfinger < apache:lookup < apache
So what is the correct ordering? Where exactly does the lookup
fit in?
In my memory the lookup was meant to override the states part of a formula: service, pkgs, etc. But this was in a time before mapping, and merging in jinja was possible. So you needed a different entry key to override.
If done correctly you can see the lookup states in some formula's like:
But most of the formula's have dropped this in favor of defaults.yaml and osmapping. which made the lookup key redundant, because you can always override it with a normal pillar key/value.
so @myii your question which ordering, it should answer it doesn't matter because formula:lookup: key
wouldn't need to override formula:key
@aboe76 Thanks for the explanation. Unfortunately, it appears that formulas have been merging formula:lookup:key
into formula:key
and that users have constructed their pillars around this. So it appears to me that the best option to cover all situations is like:
That way, those who are still using the lookup
, will have their keys merged in. While everyone else can place their values directly in the main body of the pillar. Either way, this covers both situations without breaking anything. The only downside is the "extra" lookup
key in the map, but this is of no real consequence.
In terms of this issue, then the problem isn't the lookup
per se. @LloydArmstrong could just put his values directly under redis:
and it will all work, since his pillar seems to be getting picked up. But what's really a problem is that osfamilymap
and maybe the other os
-based maps are not being picked up.
@myii I think you are right, if we can fix the map.jinja with the lookup merging. and then we can remove lookup
key in the pillar.example
@myii I have tried this on my own machine and with the latest redis-formula, I don't have this issue....
formula:lookup:key:value overrides: formula:defaults
tested on:
@aboe76 Thanks for testing that. Need to figure out why it's not working for @LloydArmstrong...
I'm still testing:
still can't replay the issue.
Thanks for jumping on and assisting @aboe76 .
@LloydArmstrong could just put his values directly under
redis:
and it will all work, since his pillar seems to be getting picked up.
Even with a minimal pillar with only svc_state
and overcommit_memory
specified, the error for both pkg_name
and cfg_name
persists. After running my state.apply in DEBUG mode, I spotted a few errors with regards to defaults.merge
:
[DEBUG ] Could not LazyLoad defaults.merge: 'defaults.merge' is not available.
It's at this point I decided to just take the plunge and pull the new changes into our live Formulas repo and test it using a standard master-minion setup (rather than salt-ssh). Well, no errors and the pillar works as expected with values being propagated. But will continue testing and let you know if I find any missing things or discrepancies.
@LloydArmstrong above:
Even with a minimal pillar with only
svc_state
andovercommit_memory
specified, the error for bothpkg_name
andcfg_name
persists. After running my state.apply in DEBUG mode, I spotted a few errors with regards todefaults.merge
:[DEBUG ] Could not LazyLoad defaults.merge: 'defaults.merge' is not available.
I was wondering if you were using versions of salt-master
and salt-minion
that are too old. Hence, the requests above:
Some questions/requests:
- Please paste
# salt --versions-report
for the minion as well.- Please paste the output of
dpkg -l | grep salt
on the master and minion.
If you could do that, we can figure out whether the minion doesn't have defaults.merge
because it is an older version.
@myii Here is the salt-minion --versions-report
for the minion:
Salt Version:
Salt: 2018.3.2
Dependency Versions:
cffi: Not Installed
cherrypy: Not Installed
dateutil: 2.6.1
docker-py: Not Installed
gitdb: Not Installed
gitpython: Not Installed
ioflo: Not Installed
Jinja2: 2.10
libgit2: Not Installed
libnacl: Not Installed
M2Crypto: Not Installed
Mako: 1.0.7
msgpack-pure: Not Installed
msgpack-python: 0.5.6
mysql-python: Not Installed
pycparser: Not Installed
pycrypto: 2.6.1
pycryptodome: Not Installed
pygit2: Not Installed
Python: 2.7.15rc1 (default, Nov 12 2018, 14:31:15)
python-gnupg: 0.4.1
PyYAML: 3.12
PyZMQ: 16.0.2
RAET: Not Installed
smmap: Not Installed
timelib: Not Installed
Tornado: 4.5.3
ZMQ: 4.2.5
System Versions:
dist: Ubuntu 18.04 bionic
locale: UTF-8
machine: x86_64
release: 4.15.0-29-generic
system: Linux
version: Ubuntu 18.04 bionic
2. Here is dpkg -l | grep salt
for the Minion:
ii salt-common 2018.3.2+ds-1 all shared libraries that salt requires for all packages
ii salt-minion 2018.3.2+ds-1 all client package for salt, the distributed remote execution system
And the Master
ii salt-common 2018.3.3+ds-2 all shared libraries that salt requires for all packages
ii salt-master 2018.3.3+ds-2 all remote manager to administer servers via salt
ii salt-ssh 2018.3.3+ds-2 all remote manager to administer servers via salt
@LloydArmstrong Those versions are fine. I've tried a test installation with version 2018.3.2+ds-1
and the map is constructed properly. There's going to be some configuration issue somewhere.
Unfortunately, I'm going to have to leave the troubleshooting here, since the formula itself is working fine. You have got it working in the most important place, in any case. If you ever get to the bottom of why it isn't working in your broken setup, please add another comment here because it would be good to know what can cause this kind of thing.
Update: To answer some of my own questions above such as Ubuntu 18.04 vs. Python 2, the test Docker-based environment I created is nearly identical to the one @LloydArmstrong has shown above:
Salt Version:
Salt: 2018.3.2
Dependency Versions:
cffi: Not Installed
cherrypy: Not Installed
dateutil: 2.6.1
docker-py: Not Installed
gitdb: 2.0.3
gitpython: 2.1.8
ioflo: Not Installed
Jinja2: 2.10
libgit2: Not Installed
libnacl: Not Installed
M2Crypto: Not Installed
Mako: 1.0.7
msgpack-pure: Not Installed
msgpack-python: 0.5.6
mysql-python: Not Installed
pycparser: Not Installed
pycrypto: 2.6.1
pycryptodome: Not Installed
pygit2: Not Installed
Python: 2.7.15rc1 (default, Nov 12 2018, 14:31:15)
python-gnupg: 0.4.1
PyYAML: 3.12
PyZMQ: 16.0.2
RAET: Not Installed
smmap: 2.0.3
timelib: Not Installed
Tornado: 4.5.3
ZMQ: 4.2.5
System Versions:
dist: Ubuntu 18.04 bionic
locale: ANSI_X3.4-1968
machine: x86_64
release: 3.13.0-66-generic
system: Linux
version: Ubuntu 18.04 bionic
@aboe76 @LloydArmstrong Upstream bug report: https://github.com/saltstack/salt/issues/51605.
@myii nice find, hopefully saltstack fixes it quickly.
Update: To answer some of my own questions above such as Ubuntu 18.04 vs. Python 2, the test Docker-based environment I created is nearly identical to the one @LloydArmstrong has shown above:
I'm glad it was reproducible!
@aboe76 @LloydArmstrong Upstream bug report: saltstack/salt#51605.
Great! Thanks @myii and @aboe76 for all the insight and help!
Hi,
Problem: Changing the
overcommit_memory
value does not overwrite thedefaults.yaml
valueCause: The
overcommit_memory
option was added to the incorrect block (part oflookup
)Resolution: Dropping
overcommit_memory
out of thelookup
block allows the option to work as expectedIf anyone else can confirm this, I will create a Pull Request in the next few days.