roles: added dep and var cache to improve performance - #85249
Conversation
ansibot
added
feature
…sions with other global names
ansibot
added
the
needs_revision
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. |
|
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.
ansibot
removed
the
ci_verified
@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. |
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. |
ansibot
removed
the
needs_revision
|
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... |
ansibot
added
the
stale_ci
ansibot
removed
the
stale_ci
|
@sivel with 2.19 out the door would you have time to look into this PR again? I think the implementation is as simple as it gets now, while providing a significant performance boost. If there are worries about potential breaking changes it'd be possible to move the caching behind a feature flag, e.g. using an env var |
ansibot
added
the
stale_ci
ansibot
added
needs_revision
ansibot
removed
the
needs_revision
ansibot
added
the
stale_ci
There was a problem hiding this comment.
I believe this breaks backward compatibility, see my comments.
I have a draft pull request that is less invasive and deals just with not re-loading variables needlessly: #85418. The performance improvement is 4x in my testing just with that change. But I have not done extensive testing.
I wonder if we should just leave duplicates in the dependency tree and let them be skipped based on allow_duplicates as we do with "normal" roles.
| for dep in self.get_direct_dependencies(): | ||
| for child_dep in dep._all_dependencies: | ||
| if child_dep in self._all_dependencies: | ||
| self._all_dependencies.remove(child_dep) |
There was a problem hiding this comment.
This does not take allow_duplicates into account. This looks like a breaking change.
There was a problem hiding this comment.
I thought this is just to get the dependencies of a role, not about getting roles that should be run before this role. Is there a case where a role has a dependency onto another role multiple times? Perhaps when defining a dependency with different variables?
The check in L577 (if child_dep in self._all_dependencies) could account for that by extending the comparison operator.
| self._all_dependencies.remove(child_dep) | ||
| self._all_dependencies.append(child_dep) | ||
| if dep in self._all_dependencies: | ||
| self._all_dependencies.remove(child_dep) |
There was a problem hiding this comment.
I suspect this meant to say:
| self._all_dependencies.remove(child_dep) | |
| self._all_dependencies.remove(dep) |
Otherwise I get:
self._all_dependencies.remove(child_dep)
^^^^^^^^^
UnboundLocalError: cannot access local variable 'child_dep' where it is not associated with a value
There was a problem hiding this comment.
You're right. I wonder why I didn't catch that before. The code is running for me for several deployments I'm doing 🤔
| for role in play.roles: | ||
| if role.public: | ||
| all_vars = _combine_and_track(all_vars, role.get_default_vars(), "role '%s' defaults" % role.name) | ||
| all_vars = _combine_and_track(all_vars, role.get_default_vars(), f"role '{role!r}' defaults") |
There was a problem hiding this comment.
Are any changes in lib/ansible/vars/manager.py necessary?
There was a problem hiding this comment.
No, these are all cosmetic changes allowing for easier debugging. During the lifetime of this MR all other changes have been removed from manager.py
| # available by using forward references this seems not to work well with commonly used IDEs. | ||
| # Therefore the TYPE_CHECKING hack seems to be a more universal approach, even if not being very elegant. | ||
| # References: | ||
| # Refs: |
There was a problem hiding this comment.
I am curious as to what is incorrect about "References"?
There was a problem hiding this comment.
You're right. I wrote the NOTE... comment while working on this and it got merged through another MR. I think this change is the result of me working between those MRs.
ansibot
added
needs_revision
7 participants
SUMMARY
These changes introduce caching 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