vm.overcommit_memory is in incorrect block in pillar
#75
Comments
|
@LloydArmstrong Thanks for your report. The issue here is that the |
|
@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. |
Fix `redis:lookup` regression (fixes #75)
|
@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 |
|
@LloydArmstrong OK, 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.confOr via. the pillar: redis:
...
lookup:
...
cfg_name: /etc/redis.confQuestions:
|
What do you think? Remove it and let the default get set from EDIT: I mean |
In that case, you don't need to use a value in the pillar at all -- it will be picked up automatically from
Probably a typo but there isn't an Hope this helps, @LloydArmstrong. |
|
@myii Apologies for the previous error. I meant Ok, I have removed all unnecessary (redundant) options in my pillar so it looks as follows: 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: |
|
@LloydArmstrong Let's get the output of the rendered Create the file {% 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_testFor even nicer output, if you have python -c "import json; jd=json.dumps({{ redis_settings }}, sort_keys=True); print(jd)" | jq . | tee /tmp/salt_test.jsonRun this using something like: # salt -Cv '<minion_id>' state.sls redis.test
Open the file |
|
@myii I installed |
|
@LloydArmstrong OK, so none of the values from redis-formula/redis/osfamilymap.yaml Lines 3 to 14 in 4830a21 For the same minion, could you paste the output of # salt -Cv '<minion_id>' grains.get os_family |
|
Yeah I noticed there were variables missing. The minion returns |
|
@LloydArmstrong above:
So these values shouldn't be missing. @LloydArmstrong above:
This definitely isn't using the latest changes, since the So coupling this with Another test could be to comment out the @LloydArmstrong above:
Did you update this in the same directory as is configured for |
|
@myii I see your point about the redundant
Yes, the newly cloned formula was placed in the same location as before and the path in the I am using |
|
@LloydArmstrong above:
It's not that there are two values for {
"lookup": {
"overcommit_memory": false,
"svc_state": "running"
},
"overcommit_memory": false,
}@LloydArmstrong above:
So only the values from default.yaml are coming through. @LloydArmstrong above:
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:
|
|
1. Here is the output for 2. ii. Here is iii. Here is |
|
@LloydArmstrong Please paste the output of: # salt --versions-report |
|
|
@LloydArmstrong Couple of notes, may not be significant:
Some questions/requests:
{% 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
|
|
I think there is something wrong with the logic here: |
|
@aboe76 Thanks for this, I'm glad to hear that there is some explanation about the
So we put it back to match how it was before those changes:
So what is the correct ordering? Where exactly does the |
|
In my memory the lookup was meant to override the states part of a formula: 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 |
|
@aboe76 Thanks for the explanation. Unfortunately, it appears that formulas have been merging
That way, those who are still using the In terms of this issue, then the problem isn't the |
|
@myii I think you are right, if we can fix the map.jinja with the lookup merging. and then we can remove |
|
@myii I have tried this on my own machine and with the latest redis-formula, formula:lookup:key:value 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 .
Even with a minimal pillar with only 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:
I was wondering if you were using versions of
If you could do that, we can figure out whether the minion doesn't have |
|
@myii Here is the 2. Here is And the Master |
|
@LloydArmstrong Those versions are fine. I've tried a test installation with version 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: saltstack/salt#51605. |
|
@myii nice find, hopefully saltstack fixes it quickly. |
I'm glad it was reproducible!
Great! Thanks @myii and @aboe76 for all the insight and help! |
Hi,
Problem: Changing the
overcommit_memoryvalue does not overwrite thedefaults.yamlvalueCause: The
overcommit_memoryoption was added to the incorrect block (part oflookup)Resolution: Dropping
overcommit_memoryout of thelookupblock allows the option to work as expectedIf anyone else can confirm this, I will create a Pull Request in the next few days.
The text was updated successfully, but these errors were encountered: