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

map.jinja and yaml speed improvements on large formula's #24

Open
aboe76 opened this issue Feb 12, 2019 · 9 comments
Open

map.jinja and yaml speed improvements on large formula's #24

aboe76 opened this issue Feb 12, 2019 · 9 comments

Comments

@aboe76
Copy link
Member

aboe76 commented Feb 12, 2019

see saltstack/salt#39017

if we can convert some formula's which have a big or multiple yaml files
we can convert these to json and import those.

oneliner to convert

python -c 'import sys, yaml, json; json.dump(yaml.load(sys.stdin), sys.stdout, indent=4)' < file.yaml > file.json

We could keep the yaml files for easier maintenance but have a nice readme about why
this formula needs json files for importing...

@myii
Copy link
Member

myii commented Feb 12, 2019

@aboe76 Sounds good! Thanks for adding that to the plan.


BTW, there's been a discussion on Slack about using the PillarStack_SDB Module alongside TOFS for even greater efficiency. I remember you mentioning that you use (or have used) PillarStack. I'm going to add this to the plan as something to be evaluated. Dominik Bessler has offered to give us some feedback about this combination as well. Any thoughts from your experience?

@daks
Copy link
Member

daks commented Feb 13, 2019

So the idea is to use JSON instead of YAML because parsing is faster?

I'm not completely ok with keeping both JSON and YAML files with the same information inside, it will just burden the repository and introduce confusion. If we just want to ease maintenance, maybe add doc about converting JSON to YAML, edit YAML, and then convert YAML to JSON.

@myii
Copy link
Member

myii commented Feb 13, 2019

@daks Thanks for sharing your views. Always helps when others share their expertise.

Just to be clear, @aboe76 is discussing this in relation to formulas which contain an excessive amount of YAML. It may be worth exploring the performance gains from using JSON in those cases. Just sharing ideas for the time being as we explore how SaltStack Formulas can be improved.

I agree with you about avoiding duplication at all costs. I'm sure we can find a solution for this if we decide that we would like to go forward with it.

@noelmcloughlin
Copy link
Member

Many programmers consider YAML controversial and prefer JSON. This is another consideration.

@myii
Copy link
Member

myii commented Feb 25, 2019

@noelmcloughlin added a decent comment on a PR that needs to be included here:

One more thought (per the vision stuff, not this PR) is that salt can support both yaml and json configuration. All formulas use yaml/ today but anticipating json/ or other data serialisation technology language in the design may make sense - something like this?

   template-formula/
        template/
          .. etc ..
             yaml/
                  defaults.yaml
                  osmap.yaml
                  osfamilymap.yaml
            json:
                 defaults.json
                 osmap.json
                 osfamilymap.json

@alxwr
Copy link
Member

alxwr commented May 15, 2019

Maybe #116 is of interest here.

@baby-gnu
Copy link
Contributor

Hello.

I personally completely prefer YAML since it's much more human readable and support comments.

My 2¢.

Regards.

@myii
Copy link
Member

myii commented Apr 8, 2020

This conversation came up again in a Slack thread starting from this comment:

18h Nate
Hello, everyone.
I’ve been working with the SaltStack container to try and familiarize myself with Salt. One thing I’m having a hard time finding solid documentation on, is the structure of config files in /etc/salt/master.d/ The container writes your env var for master_config in JSON. However, I’ve seen some docs that state config files there need to be YAML. Does anyone have an opinion on how to handle those config files?

18h Imran Iqbal
Simply copy the settings you'd like to override from /etc/salt/master and modify them in file(s) under /etc/salt/master.d.

17h Nate
Thanks for the feedback! That makes sense. Do you know if the files can be both JSON and YAML?

17h Imran Iqbal
I've never seen them in JSON format. Why would you want to override it like that? Wouldn't keeping it in the same format as /etc/salt/master be the simplest thing to do?

17h David Murphy
you can feed them in JSON format since that is what the YAML parsed is turned into but it has been 4 years since done anything like that. Note YAML is a little easier to deal with, but JSON is doable

17h Nate
I absolutely agree with both of you. To be honest, it’s been a slight struggle working with the container and converting to JSON. For reference, you can see the container taking JSON input and writing it to master.d. I may just move away from using the env vars, and use a dockerfile to add the config in as /etc/salt/master
Thanks again for your input and insight!
https://github.com/saltstack/saltdocker/blob/master/saltinit.py


32m Wayne Werner
also FWIW putting them in JSON format eliminates the YAML parsing overhead. If you have a lot of things, using JSON can significantly improve startup times. I'm not sure what the exact figures are, but I know they're significant

28m Imran Iqbal
@​Wayne Werner How much time would be saved with the default configuration files still provided in YAML? Or are you suggesting to effectively override entire configuration files for this performance gain?

20m Wayne Werner
IIUC everything YAML in Salt gets parsed into a python dictionary, and JSON parsing is way faster than YAML. For basic config (<20-30 lines) this isn't a big deal. For a couple of thousand lines of pillar data & config, there's more advantage to using JSON ahead of time

5m Imran Iqbal
Yes, I heard about this before. We were thinking about tackling this as some point: #24.

I've had a little thought about this. How about we still use YAML for development but we convert them to JSON for processing? We ensure that these generated files are also merged into the formula as artifacts to be consumed by map.jinja in production. A one-way conversion only, a bit like how JS files are minified, for performance gains. We could use the one-liner that @aboe76 provided in the first post of this issue.

@dafyddj
Copy link
Contributor

dafyddj commented Apr 9, 2020

Perhaps this could be done when packing into an spm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants