Make possible to reference previous pillars from subsequent pillars, as they specified in the top file #37003

AndreiPashkin · 2016-10-14T13:44:33Z

What does this PR do?

It implements proposal of @jberkus to make possible to reference previous pillars from the ones that are subsequent in the top file.

Even if core developers have own plans for solving #6955, this feature does not prevent any other solution from being implemented and would just give a developer another way to set up his system.

What issues does this PR fix or reference?

#6955

Previous Behavior

Previously order in which regular pillars are loaded (and become reference-able by other pillars) was undefined.

New Behavior

Now user can reference pillars, that come before current pillar in the top file.

Tests written?

Yes. I would ask core devs to advise if and what additional tests are needed.

cachedout · 2016-10-18T02:02:22Z

I would like to have @thatch45 @terminalmage and @plastikos all chime in here.

thatch45 · 2016-10-18T15:43:14Z

I like this approach and I am ok with it. It inserts the logic in the correct place and simplifies the code a little, I think it is very well done.

AndreiPashkin · 2016-10-18T15:53:18Z

The PR is not yet ready to be merged, wait until I finalize it plz.

thatch45 · 2016-10-18T16:03:56Z

Of course :)

AndreiPashkin · 2016-11-02T12:27:05Z

I'm trying to fix the tests, but when I launch them locally - they pass (unlike in Jenkins). For example this one:
https://jenkins.saltstack.com/job/PR/job/salt-pr-linode-ubuntu14-n/6299/testReport/integration.modules.state/StateModuleTest/test_onfail_requisite/

Can somebody tell, from the top of the head, what can cause this?

My versions report:

$ salt --versions-report
Salt Version:
           Salt: 2016.11.0-297-g6a42ccd

Dependency Versions:
           cffi: 1.5.2
       cherrypy: Not Installed
       dateutil: 2.4.2
          gitdb: 0.6.4
      gitpython: 1.0.1
          ioflo: Not Installed
         Jinja2: 2.8
        libgit2: Not Installed
        libnacl: Not Installed
       M2Crypto: 0.21.1
           Mako: 1.0.3
   msgpack-pure: Not Installed
 msgpack-python: 0.4.6
   mysql-python: Not Installed
      pycparser: 2.14
       pycrypto: 2.6.1
         pygit2: Not Installed
         Python: 2.7.12 (default, Jul  1 2016, 15:12:24)
   python-gnupg: Not Installed
         PyYAML: 3.11
          PyZMQ: 16.0.0
           RAET: Not Installed
          smmap: 0.9.0
        timelib: Not Installed
        Tornado: 4.2.1
            ZMQ: 4.1.5

System Versions:
           dist: LinuxMint 18 sarah
        machine: x86_64
        release: 4.4.0-45-generic
         system: Linux
        version: LinuxMint 18 sarah

Jenkins versions report:

Salt Version:
           Salt: 2016.11.0-297-g6a42ccd

Dependency Versions:
           cffi: 0.8.6
       cherrypy: 8.1.2
       dateutil: 2.5.3
          gitdb: 2.0.0
      gitpython: 2.1.0
          ioflo: 1.6.5
         Jinja2: 2.7.2
        libgit2: Not Installed
        libnacl: Not Installed
       M2Crypto: 0.21.1
           Mako: Not Installed
   msgpack-pure: Not Installed
 msgpack-python: 0.3.0
   mysql-python: 1.2.3
      pycparser: 2.10
       pycrypto: 2.6.1
         pygit2: Not Installed
         Python: 2.7.6 (default, Jun 22 2015, 17:58:13)
   python-gnupg: 0.3.9
         PyYAML: 3.10
          PyZMQ: 14.5.0
           RAET: Not Installed
          smmap: 2.0.1
        timelib: 0.2.4
        Tornado: 4.4.2
            ZMQ: 4.0.5

System Versions:
           dist: Ubuntu 14.04 trusty
        machine: x86_64
        release: 4.8.3-x86_64-linode76
         system: Linux
        version: Ubuntu 14.04 trusty

AndreiPashkin · 2016-11-02T13:14:42Z

Okay, I have updated GitPython from 1.X to 2.X and the test failed in my local environment, I'll try to handle it by myself.

AndreiPashkin · 2016-11-27T08:45:26Z

I resolved the issues with tests, but stuck with #37905.

AndreiPashkin · 2016-12-02T21:36:44Z

@cachedout, hooray, the PR is ready to be merged now!

terminalmage

Some comments about the docs, doesn't look like it would block a merge, we can take care of them afterwards.

Code looks OK, the only thing that has me worried about performance is the invocations of the loader in render_pillar(), this might put extra load on the master at scale when compiling pillar data for a large number of minions.

@thatch45 is this a valid concern, or am I just being too cautious?

terminalmage · 2016-12-02T21:39:51Z

doc/topics/pillar/index.rst

+Referencing Other Pillars
+=========================
+
+.. versionadded:: ?????


Now that we're past the Carbon release cycle, this can be changed to .. versionadded:: Nitrogen

terminalmage · 2016-12-02T21:40:20Z

doc/topics/pillar/index.rst

+
+.. versionadded:: ?????
+
+It is possible to reference pillar values, that are defined in other


The comma here is unnecessary.

terminalmage · 2016-12-02T21:42:54Z

doc/topics/pillar/index.rst

+.. versionadded:: ?????
+
+It is possible to reference pillar values, that are defined in other
+pillar files. To do that, the pillar with referenced pillars should


This could be worded better. Perhaps:

To do this, make sure that any pillar variables which are being referenced by other pillar SLS files are defined higher up in the pillar Top file, so that they are evaluated first.

AndreiPashkin · 2016-12-05T09:39:38Z

@terminalmage

the only thing that has me worried about performance is the invocations of the loader in render_pillar(), this might put extra load on the master at scale when compiling pillar data for a large number of minions.

What solutions do you see? To make loaders copy-able and mutable?

cachedout · 2016-12-12T10:13:29Z

That concern is very valid. Given that this is in a loop, this has the potential to totally destroy master performance at scale. Why not use the copy of the minion functions we already have in the pillar instance?

AndreiPashkin · 2016-12-12T11:13:47Z

@cachedout

Why not use the copy of the minion functions we already have in the pillar instance?

Because without that salt['pillar.get'](...) won't be able to retrieve updated pillar values (from the other pillar SLS).

cachedout · 2016-12-12T11:20:21Z

I'm not following why that would be. This looks like we're just using the recomputed functions to generate new renderers. Forgive my lack of understanding here but what's different about the renderers on each iteration?

AndreiPashkin · 2016-12-12T11:26:55Z

Take a look at this integration test:
https://github.com/saltstack/salt/pull/37003/files#diff-6ee6bddf72661e11ee47ae68054d55b2R127

And at how pillars are rendered for it:
https://github.com/saltstack/salt/pull/37003/files#diff-84dcf1edfcafc9be25c02685b9631767R2

Without recreating functions uppercase_knights would not be computed correctly, because salt['pillar.get']('knights') would return None (or something like that). Because functions loader would not have recomputed pillars within it and so pillar.get function, while looking up __pillar__ dict, would find old version of pillars. So the test would fail.

But lowercase_knights would be computed correctly, because pillars are recreated each iteration and passed down to a renderer.

TL;DR To make functions like pillar.get and others "see" updated pillar values from previous iterations over SLS files, we need to recreate a function loader and pass the new pillars to it.

cachedout · 2016-12-12T11:29:56Z

K, that makes sense. Thanks for the explanation.

AndreiPashkin · 2016-12-13T21:18:49Z

I think the solution is to add update() method to LazyLoader, that would make possible to update a bound data of the instance, so there would be no need in re-creating it every iteration.

cachedout · 2016-12-20T15:12:57Z

@AndrewPashkin Yes, I agree. Exactly what I was thinking. Do you want to take a swing at that? Might be a bit before I can get to it.

AndreiPashkin · 2016-12-27T22:28:26Z

@cachedout, I'd want to do that, but I have bad shortage of time, so I can't really promise anything.

AndreiPashkin · 2017-04-15T19:15:55Z

@cachedout, @terminalmage,

Hi, guys, I decided to get back to this PR.

Here is the quick summary of the current state of things:

Everything is working.
But there are concerns about performance: within render_pillar() there are places where construction of LazyLoader, which loads the modules, occur. This patch makes them be recreated in a loop where each iteration is the processing of each pillar SLS. So it makes modules to be reloaded for rendering of each SLS which is not good.
We came up with the idea of making LazyLoader mutable, so instead of recreating it it would be possible to update it.

BUT. I've found that it seems like it is fundamentally impossible to make them updateable, because there is the place when the options (the thing that is actually needs to be made updateable in LazyLoader) are passed to an external module's initializer. So if we are passing the options to an external module and it does everything it wants with it we should re-initialize it in case if we are updated the options. It means that we can not escape the performance penalty and this is not a programming limitation but rather a fundamental one.

What are your thoughts on that?

AndreiPashkin · 2017-04-15T19:45:46Z

The only idea that I have for now is that it's possible to factor out actual loading of the modules out of LazyLoader._load_module() and make this function/method results cached. And so LazyLoader recreation will become less costly.

AndreiPashkin · 2017-06-30T03:04:23Z

@cachedout, @terminalmage,

I think, that it might be the case, that the performance impact is insignificant.

The bottle neck here is LazyLoader._load_module() - it performs actual loading of a Python module from disk. Before this change modules used in the SLS files were cached between rendering of the SLS files. Now they are reloaded each time if they are called in an SLS file.

But considering that the modules are loaded lazily, on demand, and that the amount of SLS files is not something that usually scales up to really big numbers (like number of minions), the overhead in real life scenarios might be not noticeable.

This is also supported by the fact that test runs times are not increased (both ~1:15), and I also did local benchmarks with multiple minions and multiple SLS files invoking the same module and I haven't found any difference in performance between develop and my patch. Another thing is that loading of a regular Python module doesn't seem to be an expensive operation, for example loading of state Salt module takes under 1 microsecond on my PC. My benchmarks are not comprehensive though.

cachedout · 2017-07-03T20:29:02Z

Hmm, I think you might be right here.

@terminalmage and @thatch45 what do you think?

thatch45 · 2017-07-03T21:32:37Z

I think he is right, and yes, in the old days we would cache the pillar modules, any you still can with the lazy loader, but the modules (if I recall) need to be loaded on a per pillar generation so the right variables are set in the loaded modules. Lazy loading then helps because you don't need to reload all the modules, just the needed ones

AndreiPashkin · 2017-07-03T21:40:20Z

That's correct, if a module defines __init__ function it is called after loading.
Regarding caching of modules - how it's possible if modules are mutated to set globals like __pillar__, __salt__, etc? The only way is to use deepcopy, but not all modules could be deepcopied (try to deepcopy collections for example).

thatch45 · 2017-07-03T21:48:28Z

this is why we reload pillar modules with each request to the pillar, this was actually a bug in much earlier versions of salt where the cached modules did not always properly contain the right pillar data. So you are spot on once again

AndreiPashkin · 2017-07-05T20:57:04Z

I see that the Python 3 tests are failing and not only in my PR. Should I wait until they are fixed?

…r method

…n the top file

AndreiPashkin · 2017-07-12T20:29:55Z

Tests are fixed, let's 🚢 it?

AndreiPashkin changed the title ~~[WIP] Make possible to reference previous pillars from subsequent pillars, as they specified in the top file~~ Make possible to reference previous pillars from subsequent pillars, as they specified in the top file Oct 14, 2016

AndreiPashkin changed the title ~~Make possible to reference previous pillars from subsequent pillars, as they specified in the top file~~ [WIP] Make possible to reference previous pillars from subsequent pillars, as they specified in the top file Oct 14, 2016

AndreiPashkin changed the title ~~[WIP] Make possible to reference previous pillars from subsequent pillars, as they specified in the top file~~ Make possible to reference previous pillars from subsequent pillars, as they specified in the top file Nov 26, 2016

AndreiPashkin changed the title ~~Make possible to reference previous pillars from subsequent pillars, as they specified in the top file~~ [WIP] Make possible to reference previous pillars from subsequent pillars, as they specified in the top file Nov 27, 2016

AndreiPashkin mentioned this pull request Nov 28, 2016

Impossible to retrieve external pillars with pillar.get when ext_pillar_first is True #37905

Closed

AndreiPashkin changed the title ~~[WIP] Make possible to reference previous pillars from subsequent pillars, as they specified in the top file~~ Make possible to reference previous pillars from subsequent pillars, as they specified in the top file Dec 2, 2016

terminalmage reviewed Dec 2, 2016

View reviewed changes

AndreiPashkin added 2 commits July 12, 2017 10:33

Factor out pillar merging in salt.pillar.Pillar into a separate helpe…

e2cc3e1

…r method

Make possible to reference pillars, that come before current pillar i…

7d59f7a

…n the top file

cachedout merged commit 1b683f2 into saltstack:develop Jul 18, 2017

AndreiPashkin mentioned this pull request Jul 19, 2017

Revert "Make possible to reference previous pillars from subsequent pillars, as they specified in the top file" #42383

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make possible to reference previous pillars from subsequent pillars, as they specified in the top file #37003

Make possible to reference previous pillars from subsequent pillars, as they specified in the top file #37003

AndreiPashkin commented Oct 14, 2016 •

edited

Loading

cachedout commented Oct 18, 2016

thatch45 commented Oct 18, 2016

AndreiPashkin commented Oct 18, 2016

thatch45 commented Oct 18, 2016

AndreiPashkin commented Nov 2, 2016 •

edited

Loading

AndreiPashkin commented Nov 2, 2016

AndreiPashkin commented Nov 27, 2016

AndreiPashkin commented Dec 2, 2016

terminalmage left a comment

terminalmage Dec 2, 2016

AndreiPashkin Dec 3, 2016

terminalmage Dec 2, 2016

AndreiPashkin Dec 3, 2016

terminalmage Dec 2, 2016

AndreiPashkin Dec 3, 2016

AndreiPashkin commented Dec 5, 2016

cachedout commented Dec 12, 2016

AndreiPashkin commented Dec 12, 2016 •

edited

Loading

cachedout commented Dec 12, 2016

AndreiPashkin commented Dec 12, 2016 •

edited

Loading

cachedout commented Dec 12, 2016

AndreiPashkin commented Dec 13, 2016 •

edited

Loading

cachedout commented Dec 20, 2016

AndreiPashkin commented Dec 27, 2016

AndreiPashkin commented Apr 15, 2017 •

edited

Loading

AndreiPashkin commented Apr 15, 2017

AndreiPashkin commented Jun 30, 2017 •

edited

Loading

cachedout commented Jul 3, 2017

thatch45 commented Jul 3, 2017

AndreiPashkin commented Jul 3, 2017 •

edited

Loading

thatch45 commented Jul 3, 2017

AndreiPashkin commented Jul 5, 2017

AndreiPashkin commented Jul 12, 2017


		.. versionadded:: ?????

		It is possible to reference pillar values, that are defined in other

Make possible to reference previous pillars from subsequent pillars, as they specified in the top file #37003

Make possible to reference previous pillars from subsequent pillars, as they specified in the top file #37003

Conversation

AndreiPashkin commented Oct 14, 2016 • edited Loading

What does this PR do?

What issues does this PR fix or reference?

Previous Behavior

New Behavior

Tests written?

cachedout commented Oct 18, 2016

thatch45 commented Oct 18, 2016

AndreiPashkin commented Oct 18, 2016

thatch45 commented Oct 18, 2016

AndreiPashkin commented Nov 2, 2016 • edited Loading

AndreiPashkin commented Nov 2, 2016

AndreiPashkin commented Nov 27, 2016

AndreiPashkin commented Dec 2, 2016

terminalmage left a comment

Choose a reason for hiding this comment

terminalmage Dec 2, 2016

Choose a reason for hiding this comment

AndreiPashkin Dec 3, 2016

Choose a reason for hiding this comment

terminalmage Dec 2, 2016

Choose a reason for hiding this comment

AndreiPashkin Dec 3, 2016

Choose a reason for hiding this comment

terminalmage Dec 2, 2016

Choose a reason for hiding this comment

AndreiPashkin Dec 3, 2016

Choose a reason for hiding this comment

AndreiPashkin commented Dec 5, 2016

cachedout commented Dec 12, 2016

AndreiPashkin commented Dec 12, 2016 • edited Loading

cachedout commented Dec 12, 2016

AndreiPashkin commented Dec 12, 2016 • edited Loading

cachedout commented Dec 12, 2016

AndreiPashkin commented Dec 13, 2016 • edited Loading

cachedout commented Dec 20, 2016

AndreiPashkin commented Dec 27, 2016

AndreiPashkin commented Apr 15, 2017 • edited Loading

AndreiPashkin commented Apr 15, 2017

AndreiPashkin commented Jun 30, 2017 • edited Loading

cachedout commented Jul 3, 2017

thatch45 commented Jul 3, 2017

AndreiPashkin commented Jul 3, 2017 • edited Loading

thatch45 commented Jul 3, 2017

AndreiPashkin commented Jul 5, 2017

AndreiPashkin commented Jul 12, 2017

AndreiPashkin commented Oct 14, 2016 •

edited

Loading

AndreiPashkin commented Nov 2, 2016 •

edited

Loading

AndreiPashkin commented Dec 12, 2016 •

edited

Loading

AndreiPashkin commented Dec 12, 2016 •

edited

Loading

AndreiPashkin commented Dec 13, 2016 •

edited

Loading

AndreiPashkin commented Apr 15, 2017 •

edited

Loading

AndreiPashkin commented Jun 30, 2017 •

edited

Loading

AndreiPashkin commented Jul 3, 2017 •

edited

Loading