roles: added dep and var cache to improve performance #85249
Conversation
…sions with other global names
This comment was marked as outdated.
This comment was marked as outdated.
|
I need to put down work on this for the time being due to something else coming up. I've planned to continue working on this by the end of June latest. |
| def get_all_dependencies(self): | ||
| """ | ||
| Returns a list of all deps, built recursively from all child dependencies, | ||
| in the proper order in which they should be executed or evaluated. | ||
| """ | ||
| if self._all_dependencies is None: | ||
|
|
||
| self._all_dependencies = [] | ||
| self._all_dependencies = set() |
There was a problem hiding this comment.
while sets are more efficient, they are also 'unordered' so this will break expected variable precedence and will change the display of 'role name' variables
There was a problem hiding this comment.
I changed the implementation to use a list again, but check whether the dep is already in it. Would this be alright regarding var precedence? It seemed to work ok in my test run at least.
There was a problem hiding this comment.
Unsure, I need to add precedence test for this case, but either this works or you need to reverse the operation to preserve it: add dupe, but remove previous instance
There was a problem hiding this comment.
I changed it to remove previous instances. Guess this is reflecting the current behavior in the correct way. Having a test would surely help here.
There was a problem hiding this comment.
I extended the role_dep_chain test to include different types of variables, including overwriting them in parent roles. Please check 35e35eb and let me know if this fits.
There was a problem hiding this comment.
Made a minor copy/paste error when pasting the test into my machine connected to GitHub. Should be fixed now, let's see whether the tests now pass.
|
With the current implementation I reduced the runtime of one of our deployments from 68min to 16min. This deployment in particular uses one of our high-level roles but limits what is actually been executed with tags. |
This comment was marked as outdated.
This comment was marked as outdated.
|
The test |
@monsdar if it's WIP, you could mark it as draft. Also, some people put WIP or DNM into the PR title to indicate that it's not ready to be reviewed or merged. |
test/integration/targets/roles/roles/imported_from_include/tasks/main.yml
Outdated
Show resolved
Hide resolved
| - assert: | ||
| that: | ||
| - (inherit_var is defined) and (inherit_var == 'overwritten_as_well') | ||
| - (inherit_default is defined) and (inherit_default == 'overwritten') | ||
| - (inherit_playbook is defined) and (inherit_playbook == 'playbook') | ||
| - (inherit_include is defined) and (inherit_include == 'include') | ||
| - (include_default_var is defined) and (include_default_var == 'overwritten_again') | ||
| when: (overwritten is defined) and overwritten |
There was a problem hiding this comment.
Why are the two opposite assertion variants needed in the same flow?
There was a problem hiding this comment.
The 1st flow tests the variables when they still have their initial values, the 2nd flow tests what happens when values get overwritten from a location with higher precedence. It's not a full precedence test, but it helps to check whether this works as intended when running with mixed import/include roles.
In the end this tests different cases for import/include, therefore I chose not to add a separate test for it. I can do so if requested though.
test/integration/targets/roles/roles/include_import_dep_chain/defaults/main.yml
Outdated
Show resolved
Hide resolved
|
role cache should be at play level |
PR #85346 This patch adds type-checking to the `Role.__init__()` method. Context: #85249 (comment) Co-authored-by: Sloane Hertel <19572925+s-hertel@users.noreply.github.com> Co-authored-by: Sviatoslav Sydorenko <webknjaz@redhat.com> Co-authored-by: Nils Brinkmann <nils.brinkmann@rheinmetall.com> (cherry picked from commit 578d25f)
Co-authored-by: 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua>
Co-authored-by: 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua>
Co-authored-by: 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua>
Co-authored-by: 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua>
Co-authored-by: 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua>
Co-authored-by: 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua>
Co-authored-by: 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua>
Co-authored-by: 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua>
…defaults/main.yml Co-authored-by: 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua>
|
The test |
I see that there's already a role_cache at play level. Sorry, I didn't notice when I made myself familiar with the codebase first. I think we should utilize this instead of adding a global cache.
I'll check how this could be integrated. Switching the PR back to draft status again. |
|
Seems it's enough to store the deps and vars as cached_properties, rest is done by the already existing cache. The fix is turning out to be pretty simple the more I look into it... |
SUMMARY
These changes introduce updating the plays role_cache after calculating the role dependencies, default vars and role vars to avoid those being recalculated during each Task. This PR also contains a fix to not add duplicate dependencies to the roles dep-list.
These fixes improve overhead in deployment where a lot of role dependencies are involved, see related issue.
Fixes #85206
ISSUE TYPE