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

New pillar/master_tops saltclass module #42349

Merged
merged 1 commit into from Sep 28, 2017

Conversation

Projects
None yet
8 participants
@olivier-mauras
Contributor

olivier-mauras commented Jul 17, 2017

What does this PR do?

It provides a new pillar/master_tops module called saltclass.
This module clones the behaviour of reclass, without the need of an external app, and add a couple of features to improve flexibility.

Features

  • Define your nodes through class inheritance
  • Reuse your reclass datas with minimal modifications
    • applications => states
    • parameters => pillars
      for i in $(grep -r -e applications: -e parameters: -l <your_reclass_path>); do sed -i 's/applications:/states:/g;s/parameters:/pillars:/g' $i; done
  • Use Jinja templating in your yaml definitions
  • Access to the following Salt objects in Jinja
    • __opts__
    • __salt__
    • __grains__
    • __pillars__
    • minion_id
  • Chose how to merge or override your lists using ^ character (see examples)
  • Expand variables ${} with possibility to escape them if needed \${} (see examples)
  • Ignores missing node/class and will simply return empty without breaking the pillar module completely - will be logged

Configuration

  ext_pillar:
    - saltclass:
      - path: /srv/saltclass

  master_tops:
    saltclass:
      path: /srv/saltclass

Examples

Basic jinja + grains example

# /srv/saltclass/nodes/minion.domain.yml
environment: prod
classes:
{% for class in ['default', 'app1'] %}
  - {{ class }}
{% endfor %}
  - {{ __grains__['os'] }}

pillars:
  default:
    network:
      dns:
        srv3: 192.168.1.1
    os: {{ __grains__['oscodename'] }}

List merge example

# /srv/saltclass/classes/list.yml
pillars:
  list:
    - v1
    - v2

# /srv/saltclass/nodes/minion.domain.yml
classes:
  - list

pillars:
  list:
    - v3
    - v4
# This list will be appended to the one from list class and you will end up with:
# list:
#   - v1
#   - v2
#   - v3
#   - v4
# To override the list use ^ as your first list entry
pillars:
  list:
    - ^
    - v1
    - v4
    - v5
# Will give you:
# list:
#   - v1
#   - v4
#   - v5

Variables expansion

# _test/classes/default.yml
pillars:
  default:
    test:
      list:
        - l1
        - l2
        - l3
  default2:
    expansion:
      going:
        down:
          some: more

# _test/nodes/alpine.internal.yml
pillars:
  borg:
    server: other.xxx.xxx
  override:
    testing:
      variable:
        string: ${default2:expansion:going:down:some}               # Will be expanded to 'more'
        list_: ${default:test2:list}                                # Will be expanded to ['l1', 'l2', 'l3']
        inline_expansion: 'borg server is ${borg:server}'           # Will be expanded to 'borg server is other.xxx.xxx'
        inline_expansion_escape: 'borg server is \${borg:server}'   # Will be expanded to 'borg server is ${borg:server}'
        unknown_expansion: ${class4:unknown:variables}              # Will be expanded to '${class4:unknown:variables}'

An example subset of datas is available here: http://git.mauras.ch/salt/saltclass/src/master/examples
Although useless it gives a pretty good idea how to layout your class hierarchy.

Integrated documentation is pretty scarse at the moment, I'll try to improve that ASAP

@cachedout

This comment has been minimized.

Contributor

cachedout commented Jul 18, 2017

@coredumb Very interesting!

Let's start by getting these lint errors fixed up and then we'll do a full code review here. Thanks!

https://jenkins.saltstack.com/job/PR/job/salt-pr-lint-n/12833/violations/file/salt/utils/saltclass.py/

@olivier-mauras

This comment has been minimized.

Contributor

olivier-mauras commented Jul 18, 2017

Yep I saw the result, I only used flake8 on the code, I'll work that out :)

@cachedout

This comment has been minimized.

Contributor

cachedout commented Jul 19, 2017

@coredumb Could you please add a description of this new feature to the release notes? https://github.com/saltstack/salt/blob/develop/doc/topics/releases/oxygen.rst

@olivier-mauras

This comment has been minimized.

Contributor

olivier-mauras commented Jul 19, 2017

Sure no problem. Let me just improve examples with something that make sense first :)

@olivier-mauras

This comment has been minimized.

Contributor

olivier-mauras commented Jul 30, 2017

Ok so I've made much better examples, and made modifications to oxygen.rst.
For some reason all the code blocks are not rendered though :(

Edit: Ok I've fixed the code blocks, let me know if documentation is clear enough

@cachedout cachedout requested review from terminalmage and thatch45 Jul 31, 2017

@thatch45

While I really love this addition, I think we should get some tests in before we accept it. When we add syntactic features like this to Salt they can be easily broken in the future without tests.
So in a nutshell, I want this in, but if we merge it without tests it WILL end up broken.

def ext_pillar(minion_id, pillar, *args, **kwargs):
'''

This comment has been minimized.

@thatch45

thatch45 Aug 1, 2017

Member

We should probably add some docs here, it does not need to be a lot, but just some info on expected data etc.

@olivier-mauras

This comment has been minimized.

Contributor

olivier-mauras commented Aug 1, 2017

@thatch45 I'd be very happy to write tests for the modules if someone could point me in the right direction.
First I've no idea how tests are handled at salt level, second the scope of testing can be quite large and difficult as a lot of grains can be used in Jinja rendering.
That said, I've a wrapper script in my repo - http://git.mauras.ch/salt/saltclass/src/master/salt/test.py - that I use to validate my example subset over some of the example minions defined where I can enter some fake grains to test out results. Could this fit as a test? How should/could this be integrated?

About the documentation, indeed it needs some more informations, and maybe some more robust module initialization. I'll work on that.

@thatch45

This comment has been minimized.

Member

thatch45 commented Aug 1, 2017

@coredumb that script would basically work, and it could be safely added to the unit tests. Take a look in here:
https://github.com/saltstack/salt/tree/develop/tests/unit/pillar
It is completely fine if the tests for this are using mocked grains and are in the unit tests since this is something that can be tested without needed integration with salt running.

@olivier-mauras

This comment has been minimized.

Contributor

olivier-mauras commented Aug 1, 2017

@thatch45 As the modules are reading yaml files, is it OK to provide a test subset in the unit/pillar directory?

@thatch45

This comment has been minimized.

Member

thatch45 commented Aug 1, 2017

Yes, although @cachedout might recall better where to put those for unit tests

@terminalmage

Looks good, but needs one minor change in the release notes. We can make it when merging if there are no other objections.

----------------------------------------------
This module clones the behaviour of reclass (http://reclass.pantsfullofunix.net/), without the need of an external app, and add several features to improve flexibility.
Saltclass let's you define your nodes from simple ``yaml`` files (``.yml``) through hierarchical class inheritance with the possibility to override pillars down the tree.

This comment has been minimized.

@terminalmage

terminalmage Aug 7, 2017

Member

should be lets instead of let's

This comment has been minimized.

@olivier-mauras

olivier-mauras Aug 7, 2017

Contributor

Ooops let me correct this one

@cachedout

This comment has been minimized.

Contributor

cachedout commented Aug 21, 2017

@coredumb You could put them here: https://github.com/saltstack/salt/tree/develop/tests/integration/files

Also, we need a merge conflict resolved here. Thanks!

@cachedout

This comment has been minimized.

Contributor

cachedout commented Aug 28, 2017

@coredumb Did you see my comment above?

@olivier-mauras

This comment has been minimized.

Contributor

olivier-mauras commented Aug 28, 2017

@cachedout Sorry for not replying sooner, I got super busy on other tasks lately.
I'll try to find some time this week to finalise this.

@cachedout

This comment has been minimized.

Contributor

cachedout commented Aug 28, 2017

@coredumb Totally understandable. Thanks for the quick reply!

@cachedout

This comment has been minimized.

Contributor

cachedout commented Sep 5, 2017

Hi @coredumb. Any update here?

@olivier-mauras

This comment has been minimized.

Contributor

olivier-mauras commented Sep 6, 2017

@cachedout Again sorry for the delay!
I've fixed the merge conflict
I've added some inline documentation for ext_pillar/master_tops functions as requested by @thatch45
I've put example definitions under tests/integration/files/saltclass
I've created a unit test file test_saltclass.py based on what I could grasp from the other tests...

I honestly have no idea how to run the test itself to ensure that it actually is valid... I've looked around the documentation but I must be blind...

@cachedout

This comment has been minimized.

Contributor

cachedout commented Sep 11, 2017

@olivier-mauras

This comment has been minimized.

Contributor

olivier-mauras commented Sep 12, 2017

@cachedout let me fix that :)
What's the best way to run the tests myself?

@olivier-mauras

This comment has been minimized.

Contributor

olivier-mauras commented Sep 12, 2017

@cachedout OK sorry for not being able to find the documentation sooner...

% ./tests/runtests.py -n unit.pillar.test_saltclass                                                                                                                                                                                                                
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 * Python Version: 2.7.13 (default, Jul 16 2017, 01:33:52) [GCC 6.3.0]
 * Transplanting configuration files to '/tmp/salt-tests-tmpdir/config'
 * Current Directory: /home/coredumb/stuff/virtualenvs/salt-test/salt
 * Test suite is running under PID 27073
 * Logging tests on /tmp/salt-runtests.log
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Starting unit.pillar.test_saltclass Tests
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
.
----------------------------------------------------------------------
Ran 1 test in 0.015s

OK

============================================================================================================================  Overall Tests Report  =============================================================================================================================
***  No Problems Found While Running Tests  *************************************************************************************************************************************************************************************************************************************
=================================================================================================================================================================================================================================================================================
OK (total=1, skipped=0, passed=1, failures=0, errors=0) 
============================================================================================================================  Overall Tests Report  =============================================================================================================================
@epcim

minor comments, suggesting some features

# Renders jinja from a template file
def render_jinja(_file, salt_data):

This comment has been minimized.

@epcim

epcim Sep 20, 2017

Contributor

Would it be possibe to configure jinja the same way as for SLS/Pillars.
Especially I am talking about the jinja line statements setup. See #42930 for details.

This comment has been minimized.

@olivier-mauras

olivier-mauras Sep 20, 2017

Contributor

It should not be too hard to implement

if sub_init in l_files:
return render_yaml(sub_init, salt_data)
log.warning('{0}: Class definition not found'.format(_class))

This comment has been minimized.

@epcim

epcim Sep 20, 2017

Contributor

We recently added to original "reclass" the feature to ignore some missing classes: https://github.com/salt-formulas/reclass/blob/master/reclass/core.py#L103

At first it might sound strange, the use case is for bootstrap. We ship metadata classes in the formulas. While the node refer to these it's pillar can't be valid until all the dependecy formulas are installed. As we install salt formulas with salt according to "reclass" pillar we have chicken-egg problem.

Way to resolve is quite simple:

  • preinstall only formulas required to bootstrap salt master (in our case salt,linux,git,reclass)
  • allow ignoring some missing classes with specific pattern (aka service.xyz.*)
  • apply core slat master states (that will install all dependencies according a pillar/model)
  • update configuration to fail on any missing class
  • finish w/highstate/orchestrate

This comment has been minimized.

@olivier-mauras

olivier-mauras Sep 20, 2017

Contributor

Yeah bootstrapping usually requires to preinstall something, and saltclass doesn't cut it ;)

@epcim

This comment has been minimized.

Contributor

epcim commented Sep 20, 2017

I have a question, will it load map.jinja (defaults options used to render it) if it's found in saltclass root?

@olivier-mauras

This comment has been minimized.

Contributor

olivier-mauras commented Sep 20, 2017

It will not load any map.jinja. If you want something to be loaded it must be so as a class.

@olivier-mauras

This comment has been minimized.

Contributor

olivier-mauras commented Sep 25, 2017

@thatch45 @cachedout Do I still need to fix something for the merge to happen?

@terminalmage terminalmage requested a review from thatch45 Sep 26, 2017

@terminalmage

This comment has been minimized.

Member

terminalmage commented Sep 26, 2017

I'd like to get @thatch45 to take another swing by here and re-review.

@thatch45

I think this looks good now!

@cachedout cachedout merged commit 6a25bf4 into saltstack:develop Sep 28, 2017

6 checks passed

default Build finished.
Details
jenkins/PR/salt-pr-clone Pull Requests » Salt PR - Clone #17220 — SUCCESS
Details
jenkins/PR/salt-pr-docs-n Pull Requests » Salt PR - Docs #9978 — SUCCESS
Details
jenkins/PR/salt-pr-linode-ubuntu14-n Pull Requests » Salt PR - Linode Ubuntu14.04 #14626 — SUCCESS
Details
jenkins/PR/salt-pr-linode-ubuntu16-py3 Pull Requests » Salt PR - Linode Ubuntu16.04 - PY3 #1836 — SUCCESS
Details
jenkins/PR/salt-pr-lint-n Pull Requests » Salt PR - Code Lint #14521 — SUCCESS
Details

@olivier-mauras olivier-mauras deleted the olivier-mauras:saltclass branch Oct 3, 2017

@bbinet

This comment has been minimized.

Contributor

bbinet commented Feb 8, 2018

@olivier-mauras don't you think saltclass should be also directly compatible with reclass yaml files?
Why not supporting both applications + states and parameters + pillars ?
This could help reclass users to try (and maybe adopt) saltclass?

For example if the states keyword does not exist, it could fallback to the applications keyword, and if the pillars keyword does not exist, it could fallback to the parameters keyword.

@olivier-mauras

This comment has been minimized.

Contributor

olivier-mauras commented Feb 9, 2018

@bbinet I must admit this is something I didn't even think about as I thought that a simple grep/sed onelining was easy enough to try out

@epcim

This comment has been minimized.

Contributor

epcim commented Feb 28, 2018

Backward compatibility would be nice, but nothing is easy and as saltclass has a lot new features and it may not play well in your current ecosystem anyway. I rather prefer not to look back.

@bbinet I build the images with to test saltclass with that grep/sed employed against my formulas and it works great. https://github.com/epcim/docker-salt-formulas/blob/master/DockerMake.yml
(NOTE: Be aware, in my builds, as of now, salt version installed is not "develop" even "develop" required)

unknown_expansion: ${class4:unknown:variables}              # Will be expanded to '${class4:unknown:variables}'

BTW folks, Saltclass return reference string '${refference:that:is:not:defined:xyz}' instead of throw an error. Is that supposed to be a feature? IMHO That may case rendering nonsense in system critical files.

@olivier-mauras

This comment has been minimized.

Contributor

olivier-mauras commented Feb 28, 2018

@epcim Yes this is expected because I hate to see the pillar crash because of a typo.
Should we be more verbose in logs about that?

@epcim

This comment has been minimized.

Contributor

epcim commented Feb 28, 2018

Well worth to print a warning at least. Replace it with empty string might be an option as well but then we lose a track of its name in the final output. Possibly have this behaviour configurable. And have the option to throw an error during CI, test=true run?, for example.

@max-arnold

This comment has been minimized.

Contributor

max-arnold commented May 20, 2018

Imho, pillar should crash because of a typo. Same with missing classes. Otherwise, this can easily lead to silent failures.

Please provide a way to enable strict checks by default.

@max-arnold

This comment has been minimized.

Contributor

max-arnold commented Jun 15, 2018

Is it really necessary to rescan the directory tree on each get_class() invocation?

> /Users/user/.virtualenvs/salt-2018/lib/python2.7/site-packages/salt/utils/saltclass.py(55)get_class()
     54     for root, dirs, files in salt.utils.path.os_walk(os.path.join(saltclass_path, 'classes')):
     55         for l_file in files:
     56             l_files.append(os.path.join(root, l_file))

I can't say this is the only source of performance regression, but salt is definitely slower:

Salt 2017.7.0, reclass:

salt-call --id example.com state.sls ntp test=True  1,65s user 0,79s system 89% cpu 2,728 total
salt-call --id example.com pillar.data  1,39s user 0,51s system 86% cpu 2,197 total

Salt 2018.3.1, saltclass:

salt-call --id example.com state.sls ntp test=True  2,54s user 1,20s system 92% cpu 4,053 total
salt-call --id example.com pillar.data  2,65s user 1,17s system 91% cpu 4,188 total
@olivier-mauras

This comment has been minimized.

Contributor

olivier-mauras commented Jun 15, 2018

From my testings over the same dataset of 27 Class files, reclass takes 0.588s to process against 0.181s for the saltclass code.
This shouldn't even be a source of performance regression

@astorath

This comment has been minimized.

astorath commented Jun 21, 2018

Is there any way to combine saltclass with git_pillar to store saltclass data in git?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment