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

Don't refresh modules twice per sync or refresh ops #46713

Merged

Conversation

DmitryKuzmenko
Copy link
Contributor

@DmitryKuzmenko DmitryKuzmenko commented Mar 26, 2018

What does this PR do?

Some of saltutil functions makes grains calculated twice per call. This happens because these functions fire both refresh_modules and refresh_pillar events while refresh_pillar event refreshes modules already by itself. refresh_modules reloads grains so firing both events we are getting double-grains calculation.
This PR avoids calling of refresh_modules in case refresh_pillar is called.

What issues does this PR fix or reference?

Fix #43941

Tests written?

No

Commits signed with GPG?

Yes

@DmitryKuzmenko DmitryKuzmenko requested a review from Mar 26, 2018
@rallytime
Copy link
Contributor

@rallytime rallytime commented Mar 26, 2018

@DmitryKuzmenko That seems like the right approach to me, but there's some related test failures here: https://jenkins.saltstack.com/job/PR/job/salt-pr-linode-ubuntu14-n/21119/

@saltstack/team-core Is there some expected behavior here that I am missing that these tests are looking for duplicate grain values like that?

@cachedout
Copy link
Contributor

@cachedout cachedout commented Mar 27, 2018

I think this is fine but I'm a little curious about the failing test here.

@DmitryKuzmenko
Copy link
Contributor Author

@DmitryKuzmenko DmitryKuzmenko commented Mar 27, 2018

That tests are surprising me too.. I'll work on it.

@cachedout
Copy link
Contributor

@cachedout cachedout commented Apr 5, 2018

@DmitryKuzmenko Any update here?

@DmitryKuzmenko
Copy link
Contributor Author

@DmitryKuzmenko DmitryKuzmenko commented Apr 6, 2018

@cachedout I'll probably have time for this on the next week.

@DmitryKuzmenko
Copy link
Contributor Author

@DmitryKuzmenko DmitryKuzmenko commented Apr 17, 2018

re-run py

@DmitryKuzmenko DmitryKuzmenko force-pushed the bugs/43941_grains_refresh_twice branch 2 times, most recently from da23ac4 to 3a20ca9 Compare Apr 23, 2018
@DmitryKuzmenko
Copy link
Contributor Author

@DmitryKuzmenko DmitryKuzmenko commented Apr 24, 2018

As for me it looks like removal of one unneeded refresh made the test work faster that produced the failures not because of my changes but because this made the test more reliable. I bet the updated grains module test will fail without my fixes on Jenkins.
I'll continue investigate the reason of the failure.

@DmitryKuzmenko
Copy link
Contributor Author

@DmitryKuzmenko DmitryKuzmenko commented Apr 28, 2018

Ok, here is the problem:
Each time the module function runs it runs in a standalone process so all the in-memory changes are applied to the only the execution process.
Grains changes are shared to the main process through the filesystem.
After update grains on the FS the module function sends an event to the main process to signal it to refresh the modules. This produces the main process to re-read grains file and run following processes with the updated grains data.
Between sending the event and doing the job there is time. And here's the race condition where the append function lies because it's executed earlier than the previous setvalue(grains_key, []) function is applied to the main process' data.

Tomorrow I plan to think more on the fix an provide a proposal.

@rallytime rallytime added the pending-changes label May 15, 2018
@rallytime
Copy link
Contributor

@rallytime rallytime commented May 22, 2018

@DmitryKuzmenko Any luck on this one yet?

@DmitryKuzmenko
Copy link
Contributor Author

@DmitryKuzmenko DmitryKuzmenko commented May 23, 2018

@rallytime working on it. I've wrote "tomorrow" but it was my last working day before vacation =))) Sorry for confusion

@cachedout
Copy link
Contributor

@cachedout cachedout commented Aug 15, 2018

@DmitryKuzmenko When you're ready to come back to this, we'll need a merge conflict resolved, please.

@DmitryKuzmenko
Copy link
Contributor Author

@DmitryKuzmenko DmitryKuzmenko commented Aug 15, 2018

@cachedout sure.

@cachedout
Copy link
Contributor

@cachedout cachedout commented Oct 16, 2018

@DmitryKuzmenko Any updates here?

@DmitryKuzmenko
Copy link
Contributor Author

@DmitryKuzmenko DmitryKuzmenko commented Oct 16, 2018

@cachedout currently working on Tornado 5.0. Will done this later.

@damon-atkins
Copy link
Collaborator

@damon-atkins damon-atkins commented Nov 2, 2018

re-run lint

@Ch3LL
Copy link
Contributor

@Ch3LL Ch3LL commented Mar 7, 2019

ping @DmitryKuzmenko any update here? We just branched 2017.7.9 which is the last bug fix release for 2017.7, so if this needs to get into the release it needs to be cleaned up and migrated to the 2017.7.9 release branch. thanks :)

@DmitryKuzmenko
Copy link
Contributor Author

@DmitryKuzmenko DmitryKuzmenko commented Mar 11, 2019

@Ch3LL This PR looks like a proper solution but it's not completed without fixing the race conditions I've described above. I have no solution for that race condition right now and have no time I can spend on it for now. So I don't believe we have a chance to include it to 2017.7 unless we have some free hands.

@Ch3LL
Copy link
Contributor

@Ch3LL Ch3LL commented Mar 13, 2019

no worries. When you do have time to get back to this lets move it to the 2018.3 branch, since there will be no more 2017.7 bug releases after 7.9.

@DmitryKuzmenko DmitryKuzmenko force-pushed the bugs/43941_grains_refresh_twice branch from 9bf2d79 to 7c18e3b Compare Nov 13, 2019
@DmitryKuzmenko DmitryKuzmenko changed the base branch from 2017.7 to master Nov 13, 2019
@DmitryKuzmenko
Copy link
Contributor Author

@DmitryKuzmenko DmitryKuzmenko commented Nov 13, 2019

I've rebased to master. Also cherry-picked commits from #55233.

@dwoz
Copy link
Contributor

@dwoz dwoz commented Nov 15, 2019

@dwoz dwoz merged commit a77b1b3 into saltstack:master Dec 3, 2019
49 checks passed
@DmitryKuzmenko DmitryKuzmenko deleted the bugs/43941_grains_refresh_twice branch Oct 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pending-changes Reviewers-Assigned
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants