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

Fix for 54941 pillar_refresh regression #54942

Merged
merged 3 commits into from Oct 11, 2019

Conversation

@dwoz
Copy link
Contributor

dwoz commented Oct 9, 2019

What does this PR do?

Only refresh pillar data when BaseMinion.gen_modules is called for the first time. Subsequent pillar refreshes are done when the pillar_refresh event if fired.

What issues does this PR fix or reference?

#54941

Tests written?

Yes

Commits signed with GPG?

Yes

@dwoz dwoz requested a review from saltstack/team-core as a code owner Oct 9, 2019
@pull-assigner pull-assigner bot requested a review from Ch3LL Oct 9, 2019
@dwoz dwoz force-pushed the dwoz:fix-54941 branch from f8e862a to 649d4c7 Oct 10, 2019
@dwoz dwoz changed the title [WIP] Fix for 54941 Fix for 54941 pillar_refresh regression Oct 10, 2019
@dwoz dwoz force-pushed the dwoz:fix-54941 branch from 649d4c7 to 9fe0d6a Oct 10, 2019
@dwoz dwoz force-pushed the dwoz:fix-54941 branch from 9fe0d6a to 348d1c4 Oct 10, 2019
@Ch3LL
Ch3LL approved these changes Oct 10, 2019
Copy link
Member

UtahDave left a comment

There's one more scenario I think I'd love to see tested. I'm not sure exactly where you would add this, but it would be great if in one of these spots you add this workflow:

  1. Add a key/value pair to the pillar sls file
  2. Execute a test.ping
  3. Test that the pillar data has NOT been updated with the new key/value pair yet.
  4. Run a saltutil.refresh_pillar
  5. Then verify that the pillar data has been updated.

The reason for this is it would be great to make sure that a full pillar refresh isn't happening during a regular salt command.

Is this scenario already covered by one of these tests you already have?

@dwoz

This comment has been minimized.

Copy link
Contributor Author

dwoz commented Oct 10, 2019

There's one more scenario I think I'd love to see tested. I'm not sure exactly where you would add this, but it would be great if in one of these spots you add this workflow:

  1. Add a key/value pair to the pillar sls file
  2. Execute a test.ping
  3. Test that the pillar data has NOT been updated with the new key/value pair yet.
  4. Run a saltutil.refresh_pillar
  5. Then verify that the pillar data has been updated.

The reason for this is it would be great to make sure that a full pillar refresh isn't happening during a regular salt command.

Is this scenario already covered by one of these tests you already have?

I thought about adding this exact test because it most closely mirrors the way we found the regression. The only reason I didn't is that we're essentially using pillar.get, pillar.item, and pillar.raw calls in place of test.ping.

@UtahDave

This comment has been minimized.

Copy link
Member

UtahDave commented Oct 10, 2019

There's one more scenario I think I'd love to see tested. I'm not sure exactly where you would add this, but it would be great if in one of these spots you add this workflow:

  1. Add a key/value pair to the pillar sls file
  2. Execute a test.ping
  3. Test that the pillar data has NOT been updated with the new key/value pair yet.
  4. Run a saltutil.refresh_pillar
  5. Then verify that the pillar data has been updated.

The reason for this is it would be great to make sure that a full pillar refresh isn't happening during a regular salt command.
Is this scenario already covered by one of these tests you already have?

I thought about adding this exact test because it most closely mirrors the way we found the regression. The only reason I didn't is that we're essentially using pillar.get, pillar.item, and pillar.raw calls in place of test.ping.

OK, valid point. You are correct. Those calls would trigger the bad behavior, if present.

@waynew
waynew approved these changes Oct 10, 2019
@dwoz dwoz merged commit 2f817bc into saltstack:2019.2.1 Oct 11, 2019
24 checks passed
24 checks passed
WIP Ready for review
Details
ci/docs This commit looks good
Details
ci/lint This commit looks good
Details
ci/py2/amazon2 This commit looks good
Details
ci/py2/centos6 This commit looks good
Details
ci/py2/centos7 This commit looks good
Details
ci/py2/centos7/tcp This commit looks good
Details
ci/py2/debian8 This commit looks good
Details
ci/py2/debian9 This commit looks good
Details
ci/py2/fedora29 This commit looks good
Details
ci/py2/ubuntu1604 This commit looks good
Details
ci/py2/ubuntu1604/tcp This commit looks good
Details
ci/py2/ubuntu1804 This commit looks good
Details
ci/py2/windows2016 This commit looks good
Details
ci/py3/amazon2 This commit looks good
Details
ci/py3/centos7 This commit looks good
Details
ci/py3/centos7/tcp This commit looks good
Details
ci/py3/debian8 This commit looks good
Details
ci/py3/debian9 This commit looks good
Details
ci/py3/fedora29 This commit looks good
Details
ci/py3/ubuntu1604 This commit looks good
Details
ci/py3/ubuntu1604/tcp This commit looks good
Details
ci/py3/ubuntu1804 This commit looks good
Details
ci/py3/windows2016 This commit looks good
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.