-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Refresh pillar cache when saltutil.refresh_pillar is run #60975
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple docs nitpicks and one question.
changelog/60897.fixed
Outdated
@@ -0,0 +1,2 @@ | |||
Clear and update the Pillar Cache when running saltutil.refresh_pillar. This only effects users |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be "affects" here.
doc/topics/pillar/index.rst
Outdated
@@ -386,6 +386,22 @@ updated pillar data, but :py:func:`pillar.item <salt.modules.pillar.item>`, | |||
<salt.modules.pillar.raw>` will not see this data unless refreshed using | |||
:py:func:`saltutil.refresh_pillar <salt.modules.saltutil.refresh_pillar>`. | |||
|
|||
If your are using the Pillar Cache and have set :conf_master:`pillar_cache` to be `True`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/your/you/
Also, probably don't need to say "to be True
", "to True
" is probably OK.
salt/minion.py
Outdated
@@ -2482,6 +2482,7 @@ def pillar_refresh(self, force_refresh=False): | |||
self.opts["id"], | |||
self.opts["saltenv"], | |||
pillarenv=self.opts.get("pillarenv"), | |||
force_refresh=True, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
force_refresh
is passed through elsewhere, should it really be hardcoded here rather than passing it through?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good point, I will make it configurable, so the user can choose not to refresh the pillar cache. I also think I am going to rename force_refresh
, since this is a kwarg already used in saltutil.refresh_pillar
used for refreshing modules.
22ac03f
to
681d2cc
Compare
@terminalmage ready for re-review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 looks good. Checked tests against the current salt/main pillar and they just got stuck, and they pass with the current code.
What does this PR do?
When a user has
pillar_cache
set to True, the cache will not be refreshed when runningsalt \* saltutil.refresh_pillar
. This PR adds the ability for the cache to be udpated when this is run. This PR still ensures when running a state or pillar.items for example it will not update the cache. When a user is running saltutil.refresh_pillar this would be intentional and would expect the cache to be updated.What issues does this PR fix or reference?
Fixes: #60897
Previous Behavior
salt \* saltutil.refresh_pillar
would not update the pillar cache.New Behavior
refresh_pillar
no updates the pillar cache.Merge requirements satisfied?
[NOTICE] Bug fixes or features added to Salt require tests.