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

feat(map.jinja): load a configurable list of YAML files #236

Conversation

baby-gnu
Copy link
Contributor

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
  • [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

Describe the changes you're proposing

I propose to modify map.jinja to load a configurable list of YAML files.

The configuration of map.jinja is lookup in the following order:

  1. formula specific defaults.yaml for the maintainer (can be overrode by the user)
  2. pillar map.jinja:sources to override globally the configuration for all formulas
  3. pillar mysql:map.jinja:sources to override the configuration only for this formula

I split all the os*map.yaml under parameters/<config>/<configvalue>.yaml.

  • this is an answer to the common complain about using too heavily pillars in formula
  • each osarch, os_family, os, osfinger, … is split in a separate file instead of a single YAML file per configuration
    • a user can override parameters for Debian os_family without the need to maintain the others
    • a use can easily add support for a new os_family (or os, …) by providing a single file in its file_roots
  • avoid using non secret values by modifying the map.jinja:sources (or the formula specific mysql:map.jinja: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
  • make great use of gitfs

Pillar / config required to test the proposed changes

Debug log showing how the proposed changes work

The first sls using mapj.jinja is mysql.databases, here is the debug log during the import of map.jinja

       [DEBUG   ] In saltenv 'base', looking at rel_path 'mysql/database.sls' to resolve 'salt://mysql/database.sls'
       [DEBUG   ] In saltenv 'base', ** considering ** path '/tmp/kitchen/var/cache/salt/minion/files/base/mysql/database.sls' to resolve 'salt://mysql/database.sls'
       [DEBUG   ] In saltenv 'base', looking at rel_path 'mysql/map.jinja' to resolve 'salt://mysql/map.jinja'
       [DEBUG   ] In saltenv 'base', ** considering ** path '/tmp/kitchen/var/cache/salt/minion/files/base/mysql/map.jinja' to resolve 'salt://mysql/map.jinja'
       [DEBUG   ] Fetching file from saltenv 'base', ** attempting ** 'salt://mysql/map.jinja'
       [DEBUG   ] No dest file found
       [INFO    ] Fetching file from saltenv 'base', ** done ** 'mysql/map.jinja'
       [DEBUG   ] map.jinja: initialise parameters from mysql/parameters/defaults.yaml
       [DEBUG   ] In saltenv 'base', looking at rel_path 'mysql/parameters/defaults.yaml' to resolve 'salt://mysql/parameters/defaults.yaml'
       [DEBUG   ] In saltenv 'base', ** considering ** path '/tmp/kitchen/var/cache/salt/minion/files/base/mysql/parameters/defaults.yaml' to resolve 'salt://mysql/parameters/defaults.yaml'
       [DEBUG   ] map.jinja: lookup map.jinja configuration sources
       [DEBUG   ] LazyLoaded config.get
       [DEBUG   ] key: map.jinja:sources, ret: _|-
       [DEBUG   ] key: mysql:map.jinja:sources, ret: _|-
       [DEBUG   ] map.jinja: load parameters with sources from ['osarch', 'os_family', 'os', 'osfinger']
       [DEBUG   ] map.jinja: load parameters from file mysql/parameters/osarch/amd64.yaml
       [DEBUG   ] Could not find file 'salt://mysql/parameters/osarch/amd64.yaml' in saltenv 'base'
       [DEBUG   ] map.jinja: load parameters from file mysql/parameters/os_family/Debian.yaml
       [DEBUG   ] Could not find file 'salt://mysql/parameters/os_family/Debian.yaml' in saltenv 'base'
       [DEBUG   ] map.jinja: load parameters from file mysql/parameters/os/Debian.yaml
       [DEBUG   ] In saltenv 'base', looking at rel_path 'mysql/parameters/os/Debian.yaml' to resolve 'salt://mysql/parameters/os/Debian.yaml'
       [DEBUG   ] In saltenv 'base', ** considering ** path '/tmp/kitchen/var/cache/salt/minion/files/base/mysql/parameters/os/Debian.yaml' to resolve 'salt://mysql/parameters/os/Debian.yaml'
       [DEBUG   ] map.jinja: merge parameters from mysql/parameters/os/Debian.yaml
       [DEBUG   ] map.jinja: load parameters from file mysql/parameters/osfinger/Debian-9.yaml
       [DEBUG   ] In saltenv 'base', looking at rel_path 'mysql/parameters/osfinger/Debian-9.yaml' to resolve 'salt://mysql/parameters/osfinger/Debian-9.yaml'
       [DEBUG   ] In saltenv 'base', ** considering ** path '/tmp/kitchen/var/cache/salt/minion/files/base/mysql/parameters/osfinger/Debian-9.yaml' to resolve 'salt://mysql/parameters/osfinger/Debian-9.yaml'
       [DEBUG   ] map.jinja: merge parameters from mysql/parameters/osfinger/Debian-9.yaml
       [DEBUG   ] map.jinja: save parameters in variable "mysql"

The following import of map.jinja is less verbose, for example, this is the mysql.user debug log comming just after the mysql.database:

       [DEBUG   ] In saltenv 'base', looking at rel_path 'mysql/user.sls' to resolve 'salt://mysql/user.sls'
       [DEBUG   ] In saltenv 'base', ** considering ** path '/tmp/kitchen/var/cache/salt/minion/files/base/mysql/user.sls' to resolve 'salt://mysql/user.sls'
       [DEBUG   ] map.jinja: initialise parameters from mysql/parameters/defaults.yaml
       [DEBUG   ] map.jinja: lookup map.jinja configuration sources
       [DEBUG   ] key: map.jinja:sources, ret: _|-
       [DEBUG   ] key: mysql:map.jinja:sources, ret: _|-
       [DEBUG   ] map.jinja: load parameters with sources from ['osarch', 'os_family', 'os', 'osfinger']
       [DEBUG   ] map.jinja: load parameters from file mysql/parameters/osarch/amd64.yaml
       [DEBUG   ] map.jinja: load parameters from file mysql/parameters/os_family/Debian.yaml
       [DEBUG   ] map.jinja: load parameters from file mysql/parameters/os/Debian.yaml
       [DEBUG   ] map.jinja: merge parameters from mysql/parameters/os/Debian.yaml
       [DEBUG   ] map.jinja: load parameters from file mysql/parameters/osfinger/Debian-9.yaml
       [DEBUG   ] map.jinja: merge parameters from mysql/parameters/osfinger/Debian-9.yaml
       [DEBUG   ] map.jinja: save parameters in variable "mysql"

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

@baby-gnu
Copy link
Contributor Author

This is a first draft to see if it pass CI and to show to the community that it can be used quite easily.

I'll rebase this PR with properly split commits when everybody will agree.

The map.jinja for mysql-formula has few differences with the one from template-formula:

  1. the pythonpkg trick
  2. the postprocessing
--- template-formula/TEMPLATE/map.jinja	2020-02-15 13:37:13.000000000 +0100
+++ mysql-formula/mysql/map.jinja	2020-02-15 15:33:39.000000000 +0100
@@ -1,6 +1,13 @@
 # -*- coding: utf-8 -*-
 # vim: ft=jinja
 
+{# mysql-formula specific initialisation #}
+{%- set py_ver_settings = {
+      2: {'pythonpkg': 'python-mysqldb'},
+      3: {'pythonpkg': 'python3-mysqldb'},
+    } %}
+
+
 {#- Get the `tplroot` from `tpldir` #}
 {%- set tplroot = tpldir.split('/')[0] %}
 
@@ -12,6 +19,10 @@
 {%- do salt.log.debug('map.jinja: initialise parameters from ' ~ _defaults_filename ) %}
 {%- import_yaml _defaults_filename as default_settings %}
 
+{# Reproduce previous behaviour #}
+{%- set default_settings = salt.slsutil.merge(py_ver_settings[grains.pythonversion[0]],
+                                              default_settings) %}
+
 {# List of sources to lookup for parameters #}
 {%- do salt.log.debug('map.jinja: lookup map.jinja configuration sources') %}
 {# Fallback to previously used grains #}
@@ -66,12 +77,13 @@
 {# Merge the config dict #}
 {%- set config = salt.slsutil.merge(config, _config['formula']) %}
 
-{#- Change **TEMPLATE** to match with your formula's name and then remove this line #}
-{%- do salt.log.debug('map.jinja: save parameters in variable "TEMPLATE"') %}
-{%- set TEMPLATE = config %}
+{%- do salt.log.debug('map.jinja: save parameters in variable "mysql"') %}
+{%- set mysql = config %}
 
 {#- Post-processing for specific non-YAML customisations #}
 {%- if grains.os == 'MacOS' %}
-{%-   set macos_group = salt['cmd.run']("stat -f '%Sg' /dev/console") %}
-{%-   do TEMPLATE.update({'rootgroup': macos_group}) %}
+{%-   set macos_user = salt['pillar.get']('mysql:user', salt['cmd.run']("stat -f '%Su' /dev/console")) %}
+{%-   set macos_group = salt['pillar.get']('mysql:group', salt['cmd.run']("stat -f '%Sg' /dev/console")) %}
+{%-   do mysql.macos.update({'user': macos_user}) %}
+{%-   do mysql.macos.update({'group': macos_group}) %}
 {%- endif %}

Maybe we could have a uniq map.jinja loading an optional per formula custom.jinja?

Regards.

@baby-gnu baby-gnu force-pushed the feature/configurable-map.jinja branch 2 times, most recently from d5a1f78 to 5219dce Compare February 15, 2020 14:57
@myii
Copy link
Member

myii commented Feb 15, 2020

@baby-gnu Thanks for setting this up. I've run a quick initial test locally and the map appears to be identical. When I get a chance, I intend to test this across all of the platforms, to compare the map output before and after.

@baby-gnu
Copy link
Contributor Author

Thanks @myii.

I reproduce the failures on default-debian-10-master-py3 and default-ubuntu-1804-master-py3 with the branch master. So they do not look to be related to this PR.

@baby-gnu baby-gnu force-pushed the feature/configurable-map.jinja branch from 5677401 to 53e94f4 Compare February 15, 2020 17:55
Copy link
Member

@aboe76 aboe76 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, this looks so much better, finding out which parameter is set as default.

@myii
Copy link
Member

myii commented Feb 16, 2020

@baby-gnu So I've set up two jobs in Travis to show us the map.jinja output, before and after this PR (yd01 & yd02 respectively):

I'm running one instance for each platform and the same instances in both, so that we can get a comparison. So near the end of each Travis log you'll find the map output in YAML (the so called yaml_dump), for example (debian-10):

Results in the following diff:

--- yd01
+++ yd02
@@ -19,35 +19,13 @@
            client:
              port: 33306
              socket: /var/lib/mysql-socket/mysql.sock
-           isamchk:
-             key_buffer_size: 16M
            mysqld:
-             basedir: /usr
-             bind_address: 127.0.0.1
              datadir: ~/mysql/datadir
-             expire_logs_days: 10
-             key_buffer_size: 16M
-             lc_messages_dir: /usr/share/mysql
-             max_allowed_packet: 16M
-             max_binlog_size: 100M
-             pid_file: /var/run/mysqld/mysqld.pid
              port: 33306
-             query_cache_limit: 1M
-             query_cache_size: 16M
-             skip_external_locking: noarg_present
              socket: /var/run/mysqld/mysqld.sock
-             thread_cache_size: 8
-             thread_stack: 192K
-             tmpdir: /tmp
              user: myself
            mysqld_safe:
-             nice: 0
              plugin-dir: ~/mysql/plugins
-             socket: /var/run/mysqld/mysqld.sock
-           mysqldump:
-             max_allowed_packet: 16M
-             quick: noarg_present
-             quote_names: noarg_present
        database:
        - foo
        - name: bar

So the idea is to compare each platform to ensure that the map remains identical for each and every platform. At the moment, it will have to be a manual diff. Previously, I've been able to do something a little more sophisticated locally but I'm short on time at the moment, so this will have to do.

If you can look at the above and any other diff issues, that would be really useful. I'll try to help with the comparisons later on, when I get some time.

@baby-gnu
Copy link
Contributor Author

baby-gnu commented Feb 16, 2020

This is because I made a huge mistake: osfamily instead of os_family :-/

https://travis-ci.org/myii/mysql-formula/jobs/651071506#L1217

@myii
Copy link
Member

myii commented Feb 16, 2020

@baby-gnu No problem, let me re-run yd02 again with the fix.

@myii
Copy link
Member

myii commented Feb 16, 2020

@myii
Copy link
Member

myii commented Feb 16, 2020

@baby-gnu Excellent, that's doing better. debian-10, ubuntu-1804 & centos-8 are all identical. I'll return back to the rest of them later.

@myii
Copy link
Member

myii commented Feb 16, 2020

I reproduce the failures on default-debian-10-master-py3 and default-ubuntu-1804-master-py3 with the branch master. So they do not look to be related to this PR.

@baby-gnu This is a bug reported upstream (for 3000) with a PR fix pending:

We could attempt to run the patch here to see if it works for us but that will have to be in a different PR.

@baby-gnu
Copy link
Contributor Author

We could attempt to run the patch here to see if it works for us but that will have to be in a different PR.

I was wondering if it was due to my PR and it's not, I think we have enough work to do ;-)

@myii
Copy link
Member

myii commented Feb 16, 2020

@baby-gnu @aboe76 Perfect! All 14 platforms are identical, so we're well on the way. So the remaining stages:

  • Use traverse.
  • [myii] Test use of id, alongside PillarStack (splitting pillar.example with only the secrets in pillars).
  • [myii] Test the above with salt-ssh.
  • Get the actual implementation reviewed by the main contributors, to make sure everything is OK.
  • Make any necessary changes to both PRs based on feedback.
  • Merge both PRs.
  • [myii] Update ssf-formula accordingly.

Please let me know if I missed something.

Lookup the configuration for `map.jinja` from:

1. builtin default `['osarch', 'os_family', 'os', 'osfinger', 'id']`

2. `defaults.yaml`: optional define a formula specific
   `map.jinja:sources`

3. global configuration lookup `map.jinja:sources`

4. formula specific `<tplroot>:map.jinja:sources` to find which
   configuration YAML files to load.

The hard coded default is backward compatible and add the minion id at
the end of the list.

If an entry does not match a `salt['config.get']` parameter, it's used
as a literal file path (with `.yaml` added if it's missing).

Each YAML file are formatted with the following top level keys:

- `values`: contains the the parameter values

- `strategy` (optional): define the merge strategy for the function
  `salt.slsutils.merge` (aggregate, list, overwrite, recurse,
  smart). The default is `smart`.

- `merge_lists` (optional): boolean to merge lists or overwrite them

The parameters values are merged in the following order:

1. initialize default values from `defaults.yaml`

2. merge the values from the YAML file
   `<tplroot>/parameters/<config>/<configvalue>.yaml` in the order
   defined by `map.jinja:sources`

3. merge the values from `<tplroot>:lookup`

4. merge the values from `salt['config.get'](<tplroot>)`
* mysql/parameters/defaults.yaml: remove the top level key.
@baby-gnu baby-gnu force-pushed the feature/configurable-map.jinja branch from 590184c to 2458f1c Compare February 16, 2020 20:10
@baby-gnu baby-gnu changed the title feat(map.jinja): first draft of splitting os*map.yaml feat(map.jinja): load a configurable list of YAML files Feb 17, 2020
@baby-gnu
Copy link
Contributor Author

baby-gnu commented Apr 3, 2021

I'll do a newt PR based on v5 map.jinja.

@baby-gnu baby-gnu closed this Apr 3, 2021
@baby-gnu baby-gnu deleted the feature/configurable-map.jinja branch April 8, 2021 15:54
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

Successfully merging this pull request may close these issues.

3 participants