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

make cached pillars use pillarenv rather than saltenv #43689

Merged
merged 2 commits into from Nov 13, 2017
Merged

make cached pillars use pillarenv rather than saltenv #43689

merged 2 commits into from Nov 13, 2017

Conversation

The-Loeki
Copy link
Contributor

@The-Loeki The-Loeki commented Sep 21, 2017

What does this PR do?

when pillar_cache is True, the pillarenv args stop working.
This fixes the problem for me, but I'm unsure about the further ramifications.

What issues does this PR fix or reference?

fixes #42393
which is probably a duplicate of #36153

Previous / New Behavior

I did a search & replace in the affected function saltenv > pillarenv

@ghost
Copy link

ghost commented Sep 21, 2017

@The-Loeki, thanks for your PR! By analyzing the history of the files in this pull request, we identified @cachedout, @terminalmage and @thatch45 to be potential reviewers.

@cachedout
Copy link
Contributor

@terminalmage Can you help review this one?

Copy link
Contributor

@terminalmage terminalmage left a comment

Choose a reason for hiding this comment

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

I've not dealt much with the pillar cache feature, but the default value for pillarenv is None. I would assume that this would still work, since None is a hashable value and thus could occupy a spot in the cache dict. @The-Loeki have you tested this without explicitly defining the pillarenv to make sure it works? I'm particularly interested in the debug logging that will show what the cache dict looks like.

@The-Loeki
Copy link
Contributor Author

@terminalmage good point! I hadn't considered it, but as you suspect turns out it 'just works':
However, continuing on that line of reasoning, suppose someone activates one of the external caches, say Redis? Does the same apply?

2017-09-22 23:16:29,951 [salt.pillar      :245 ][DEBUG   ][2294] Pillar cache hit for minion minion.example.net and pillarenv None
2017-09-22 23:16:29,960 [salt.loaded.int.rawmodule.state:2000][DEBUG   ][13145] Remaining event matches: -1095
2017-09-22 23:16:29,963 [salt.utils.reactor:88  ][DEBUG   ][2049] Gathering reactors for tag minion/refresh/minion.example.net
2017-09-22 23:16:30,149 [salt.pillar      :50  ][DEBUG   ][2322] Determining pillar cache
2017-09-22 23:16:30,151 [salt.pillar      :52  ][INFO    ][2322] Compiling pillar from cache
2017-09-22 23:16:30,152 [salt.pillar      :53  ][DEBUG   ][2322] get_pillar using pillar cache with ext: None
2017-09-22 23:16:30,153 [salt.utils.cache :36  ][INFO    ][2322] Factory backend: disk
2017-09-22 23:16:30,156 [salt.pillar      :238 ][DEBUG   ][2322] Scanning pillar cache for information about minion minion.example.net and
 pillarenv None
2017-09-22 23:16:30,158 [salt.pillar      :239 ][DEBUG   ][2322] Scanning cache: {'minion.example.net': {None: {'s3.key': <<lots more stuff that should be in a pillar and therefore not here ;) >>

@terminalmage
Copy link
Contributor

They all use the same dictionary structure, if I understand correctly. @cachedout can you sanity-check me here?

@cachedout cachedout merged commit 1e94a5b into saltstack:2017.7 Nov 13, 2017
@The-Loeki The-Loeki deleted the cached_pilarenv branch November 14, 2017 13:28
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.

None yet

3 participants