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

Bugfix: pillar refresh #51707

Merged
merged 1 commit into from Feb 21, 2019

Conversation

@isbm
Copy link
Contributor

commented Feb 19, 2019

What does this PR do?

Bugfix

What issues does this PR fix or reference?

Pillars are not fully refreshed.

Previous Behavior

Reproducer:

  1. Start master.
  2. Start minion.
  3. Add some pillar keypair, say name: toto.
  4. Verify that the new pillar is ready: salt <yourminion> pillar.items
  5. Refresh it: salt <yourminion> saltutil.refresh_pillar
  6. Try target your minion by this pillar: salt -I 'name:toto' test.ping and it won't work.
  7. The targeting above fully works the whole cycle only if you restart the minion (so it refreshes all the instances of the pillar)

While targeting by itself works, the minion won't return the results back and the master is trying to find a running job. The reason is because the minion is verifying itself if it is properly targeted against the old data. This is happening because opts instance of the Minion is newer than the copy of it as __opts__ in the matchers.

For the same reasons beacons do not fully tolerate pillar refresh.

New Behavior

Targeting and module orchestration calls is working after the saltutil.refresh_pillar is issued.

Tests written?

Existing should apply.

@isbm isbm requested a review from saltstack/team-core as a code owner Feb 19, 2019
@brejoc
brejoc approved these changes Feb 19, 2019
Copy link
Member

left a comment

lgtm

@waynew

This comment has been minimized.

Copy link
Contributor

commented Feb 19, 2019

Not sure, but would it make sense/is it possible to write an integration test that fails in the current code base, but passes with this fix?

@isbm

This comment has been minimized.

Copy link
Contributor Author

commented Feb 19, 2019

@waynew this just also refreshes what still has to be refreshed. LazyLoader still keeps old references to the __opt__, so we need to somehow invalidate them so the matchers wont lookup over the old data.

@brejoc

This comment has been minimized.

Copy link
Member

commented Feb 21, 2019

How should we proceed here? Is there a test needed or can it be merged like this?

@thatch45 thatch45 merged commit 74db589 into saltstack:2019.2 Feb 21, 2019
10 checks passed
10 checks passed
WIP Ready for review
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
jenkins/pr/docs The docs job has passed
Details
jenkins/pr/lint Python lint test has passed
Details
jenkins/pr/py2-centos-7 The py2-centos-7 job has passed
Details
jenkins/pr/py2-ubuntu-1604 The py2-ubuntu-1604 job has passed
Details
jenkins/pr/py2-windows-2016 The py2-windows-2016 job has passed
Details
jenkins/pr/py3-centos-7 The py3-centos-7 job has passed
Details
jenkins/pr/py3-ubuntu-1604 The py3-ubuntu-1604 job has passed
Details
jenkins/pr/py3-windows-2016 The py3-windows-2016 job has passed
Details
@thatch45

This comment has been minimized.

Copy link
Member

commented Feb 21, 2019

We proceed like that :)
Yes, this is basically refreshing the loader refs

@brejoc

This comment has been minimized.

Copy link
Member

commented Feb 22, 2019

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.