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

[Discussion] TOFS/pillar.stack/stack-formula for map.jinja #116

Open
1 task done
alxwr opened this issue May 15, 2019 · 11 comments
Open
1 task done

[Discussion] TOFS/pillar.stack/stack-formula for map.jinja #116

alxwr opened this issue May 15, 2019 · 11 comments

Comments

@alxwr
Copy link
Member

alxwr commented May 15, 2019

Currently map.jinja relies heavily on Pillar data. This shifts the load to the master which does not scale well. With the recent introduction of config.get by @myii other data sources are accessible¹, but there are some caveats:

  • no merging (not implemented in salt-ssh)
  • merging not configurable (as in pillar.stack
  • fixed lookup sequence¹

@claudekenni implemented a prototype mimicking pillar.stack: https://github.com/claudekenni/stack-formula
He also wrote an execution module https://github.com/claudekenni/stack_module which provides stack.items, stack.get and others,
and a SDB module: https://github.com/claudekenni/sdbstack.

How can we take the experience of TOFS, pillar.stack and @claudekenni's prototypes and move ~80% of our Pillar data onto the minions?

  • with configurable lookup sequence
  • with configurable merging
  • with support for various data sources (Like Pillar, which supports i.e. Jinja and the Py renderer.)
  • with computation on the minion
  • with support for salt (ZMQ), salt-call (local) and salt-ssh (SSH)
  • while still supporting Pillar for secrets
  • while still keeping the formulas independent of one another and independent of code which is not part of the Salt Module Index

¹ SDB <; Minion configuration <; Minion's grains <; Minion's pillar data < ... (credit: @myii)


Common checklist

@alxwr Hope you don't mind me hijacking this part of the comment as a common checklist for anyone to edit! It shows up properly in the GitHub UI if it's in the first comment:

  • [Ref] Make the tests pass with the new config.get on existing formulas with semantic-release and a valid test suite -- PR.
@alxwr
Copy link
Member Author

alxwr commented May 15, 2019

Maybe it's a good idea to implement #103 on the way.

@myii
Copy link
Member

myii commented May 15, 2019

This is an incredibly important discussion, so I'm just going to invite the cavalry to start bringing in their opinions (only inviting to the discussion, not forcing to participate). By the way, I reckon we're going have to break this down into multiple stages. There's already a lot going on at a basic level, with the introduction of config.get. There are so many options available, that it may take some time to work through them.

@myii
Copy link
Member

myii commented May 15, 2019

Please don't be offended if I've invited you (when you rather wouldn't have been) or if I've forgotten to invite you (because salt-ssh has blown a hole through my brain!).

TL;DR: config.get has opened up a whole new direction for SaltStack Formulas to develop towards. We can help one another find the right path(s) to take.

@n-rodriguez
Copy link
Member

n-rodriguez commented May 15, 2019

Congratulations at @myii, @alxwr and others for their work.

But I would say as a fist step : why not make the tests pass with the new config.get on exisiting formulas with semantic-release and a valid test suite :)

@myii
Copy link
Member

myii commented May 15, 2019

Thanks @n-rodriguez, that's prompted me to start the checklist at the top of this issue.

@alxwr
Copy link
Member Author

alxwr commented May 15, 2019

@myii You are allowed to hijack the first comment. 👍 😄

@daks
Copy link
Member

daks commented May 16, 2019

Currently map.jinja relies heavily on Pillar data.

Can we really say that?
map.jinja relies on pillars when user wants to override default values with the nearly-standard <formula>:lookup dict. As this means overriding core configuration, I consider it to be for edge-cases not standard formula use.
map.jinja itself in 'normal' use is YAML merging, pillars are not involved.

This shifts the load to the master which does not scale well. With the recent introduction of config.get by @myii other data sources are accessible¹,

As I already said before: we (Saltstack itself, community...) don't already have solutions to propose to get out of pillars, so it always seems premature to me to push people out of pillars when we don't have anything to propose.
And this had a lot of code, so complexity to the formula, with no real benefit at the moment.

This all config.get change seems today really buggy (the hacks inside template-formula to manage salt-ssh are really hacky/ugly)[1] and premature.

[1] Those should not be in the formulas but in salt itself, in my opinion

@baby-gnu
Copy link
Contributor

Hello.

I'm seting up of a new infrastructure and I have a little time to think about how to properly do things.

I thought quite a lot abouth differents discussions on the maintainability of the formula:

In the maintainance of libvirt-formula, one of the problem I have is “how to maintain only the distributions in which I'm interested whithout putting barrier for people to manage others?”.

I think we could start by generalizing how map.jinja works the same way we do with libtofs.jinja:

  1. load <tplroot>/defaults.yaml as default_settings
  2. use config.get to retrieve a list of places to look for, for example <tplroot>:values:sources with the default value ['osarch', 'os_family', 'os', 'osfinger']
  3. for each <item> return by step 2
    1. import_yaml the file <tplroot>/<item>/<config.get(<item>)>.yaml
    2. merge the imported values
  4. retrieve with config.get the lookup dict and merge it

The actual os*map.yaml will result in the following tree:

<tplroot>/
├── map.jinja
├── defaults.yaml
├── os
│   ├── Amazon.yaml
│   ├── CentOS.yaml
│   ├── Fedora.yaml
│   ├── Funtoo.yaml
│   ├── Manjaro.yaml
│   ├── openSUSE.yaml
│   ├── Raspbian.yaml
│   ├── SmartOS.yaml
│   ├── SUSE.yaml
│   └── Ubuntu.yaml
├── osarch
│   ├── 386.yaml
│   ├── amd64.yaml
│   ├── arm64.yaml
│   ├── armv6l.yaml
│   ├── armv7l.yaml
│   ├── ppc64le.yaml
│   ├── s390x.yaml
│   └── x86_64.yaml
├── osfamily
│   ├── Alpine.yaml
│   ├── Arch.yaml
│   ├── Debian.yaml
│   ├── FreeBSD.yaml
│   ├── Gentoo.yaml
│   ├── MacOS.yaml
│   ├── OpenBSD.yaml
│   ├── RedHat.yaml
│   ├── Solaris.yaml
│   ├── Suse.yaml
│   └── Windows.yaml
└── osfinger
    ├── Amazon Linux-2.yaml
    ├── Amazon Linux AMI-2018.yaml
    ├── CentOS-6.yaml
    ├── CentOS Linux-7.yaml
    ├── CentOS Linux-8.yaml
    ├── Debian-10.yaml
    ├── Debian-8.yaml
    ├── Debian-9.yaml
    ├── Fedora-30.yaml
    ├── Fedora-31.yaml
    ├── FreeBSD-12.yaml
    ├── Gentoo-2.yaml
    ├── Leap-15.yaml
    ├── Ubuntu-16.04.yaml
    ├── Ubuntu-18.04.yaml
    └── Windows-8.1.yaml

NB: we could groups defaults.yaml and all the directories under a parameters/ directory (or any other meaningful name)

This will permits:

  • to maintain only the supported distributions
  • to let people provide missing YAML by putting the file in the proper place in file_roots
  • to override single YAML without the need to maintain others by putting the file in proper place in file_roots (before to override)
  • to avoid using pillars for non secret values by modifing the <tplroot>:values:sources, for example:
    • add some value sources: ['osarch', 'os_family', 'os', 'osfinger', 'domain', 'roles', 'id']
    • add the proper YAML files: domain/example.net.yaml, domain/example.org.yaml, roles/web.yaml, roles/ssh.yaml, id/minion1.example.org.yaml
  • to make a great use of gitfs

We could think about adding a config.get option to make map.jinja use import_json instead of import_yaml if required.

The only thing I don't know is how to configure the merging. Do we have access to some salt functions to do that like pillar.stack does?

Regards.

@baby-gnu
Copy link
Contributor

We could do the merge with salt.slsutil.merge which support:

  • merging strategy (aggregate, list, overwrite, recurse, smart)
  • merging of lists

For example, YAML files could look like:

strategy: 'overwrite'
merge_lists: 'true'
values:
  foo: 1
  bar:
    - 'a'
    - 'b'
    - 'c'

we then load the YAML files with something like:

{%- import_yaml tlproot ~ "/defaults.yaml" as default_settings %}

{%- import_yaml tlproot ~ "/somefile.yaml" as somefile %}
{%- set config = salt.slsutil.merge(default_settings,
                                    somefile.get('values', {}),
                                    strategy=somefile.get('strategy', 'smart'),
                                    merge_lists=somefile.get('merge_lists', False) | to_bool
                                   ) %}

I made some tests and this is working with salt-ssh.

@baby-gnu
Copy link
Contributor

Now, I need a way to make it works even when the file does not exists.

@baby-gnu
Copy link
Contributor

I found a way to handle missing files:

{%- import_yaml tlproot ~ "/defaults.yaml" as default_settings %}

{%- set yamlfiles = ["file1.yaml", "file2.yaml", "file3.yaml"] %}
{%- set _config = {'values': default_settings} %}

{%- for yamlfile in yamlfiles %}
  {%- load_yaml as  loaded_values %}
    {%- include yamlfile ignore missing %}
  {%- endload %}

  {%- if loaded_values %}
    {%- do _config.update({'values': salt.slsutil.merge(_config['values'],
                                                        loaded_values.get('values', {}),
                                                        strategy=loaded_values.get('strategy', 'smart'),
                                                        merge_lists=loaded_values.get('merge_lists', False) | to_bool)})
      %}
  {%- endif %}
{%- endfor %}

{%- set config = _config['values'] %}

So, now I'll be able to provide a pull request ;-)

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

9 participants