Add pillar_cache functionality to minion.

[davisj]

Allows minon to save rendered pillar data to cachedir for later use
when file_client is set to local.

[cachedout]

Hi @davisj

Thanks for sending this in. I can certainly use the use case for something like this.

A few considerations:

• I don't see anywhere that you're using the cache that you write out. Could you clarify how that is supposed to work?
• We should be writing these caches out using msgpack instead of raw YAML, as we do the others.
• We definitely need to include some documentation about the security implications of storing pillar data on the minion so that users are aware.
• Some sort of cache invalidation or cache clearing from the master-side would be helpful, but certainly isn't a pre-requisite for this PR.
• This does have some lint errors. Please take a look at the failing test.

Thoughts?

[davisj]

@cachedout Thanks for the thoughtful feedback. I had a feeling my first salt pull request would be educational and so far I'm not disappointed :)

My use case is a minion that is normally (or just initially) connected to the salt event bus but which may be disconnected for extended periods. If file_client is 'remote' (the default case when talking to the master) then the minion will always use pillar data straight from the master. If pillar_cache is also enabled, we just stash a copy of the latest pillar data received from the master (maybe that negates the need for cache clearing/invalidation?). The local pillar copy would only be used when explicitly invoking 'salt-call --local --pillar-root=$cachdir/pillar' or with equivalent config file options set.

In my own case, a check (cron) could then run minion side to determine the last successful salt run and, if it's older than $threshold, trigger 'salt-call --local --pillar-root=$cachdir/pillar'. So being disconnected from the master would still allow for a fall back execution method for states that require pillar data.

Msgpack is new to me. Does it still make sense to use msgpack in this scenario where the data is not going back across the wire? (It occurred to me that human readability/editiability might be desirable). If so, would the minion be able to parse the msgpack'd pillar as is or would that require additional handling?

I'll add some verbiage regarding security implications. And fix the lint error.

Thanks!

[cachedout]

@davisj Is this ready for another look or do you have more commits incoming?

[davisj]

No more commits.

[basepi]

Hey @davisj this looks solid, I'm reviewing it now. The first concern I have is that we may want to add a pillar cache master-side at some point so that pillar isn't as expensive to retrieve. As a result I'd love it if you could rename your current pillar_cache configs to minion_pillar_cache or something similar.

[basepi]

Additionally, from the looks of it @cachedout's concerns are still valid. We like to store caches using msgpack, rather than yaml. Additionally, I don't see any mechanism by which this cache is actually used.

[davisj]

@basepi Thank you for taking a look. I'm happy to change the option name.

As noted above the cache is put to use like so 'salt-call --local --pillar-root=$cachdir/pillar'.

That doesn't work (currently) if the data is msgpack'd. The intent is to allow a minion to arbitrarily switch between a master and masterless mode without having to manage the distribution of pillar data. I also thought having human editable output would be a bonus. I wasn't concerned about caching for performance though I see how that'd be useful. Perhaps I need a completely different option name that better expresses this intent? Thoughts?

[basepi]

Hmmm, interesting. Sorry I missed your note above. Since it's a very manual process, that also informs my further questions about cache invalidation and so forth -- if you want to invalidate the cache you just have to run it without checking the cachedir?

I think you've resolved all my concerns, once you rename the option. Thanks, @davisj !

[davisj]

Rock on. Thanks @basepi

[davisj]

if you want to invalidate the cache you just have to run it without checking the cachedir?

That's right.

My last commit changes parameter name to "minion_pillar_cache".