Prevent reparenting a block with itself #36075
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ansibot
added
WIP
bcoca
removed
the
needs_triage
sivel
force-pushed
the
no-parent-explosion
branch
from
1ce29cd
to
b8c190d
Compare
|
This PR does not address recursion errors when using nested includes (eg a->b->c->d...) as opposed to a flat playbook/task file with many includes. They have the same traceback, but different causes. In the nested include scenario, we should have that many parents, we just cannot copy all of the objects. Eventually we hit a recursion error. |
sivel
changed the title
|
FWIW, I tested a recursive include that made it over 250 levels deep before hitting a python recursion error, so hopefully no-one goes to that extreme with nested includes... |
sivel
added a commit
that referenced
this pull request
Feb 13, 2018
* Prevent reparenting a block with itself * Move __eq__ to Block, to avoid some unexpected problems (cherry picked from commit 76ff3e9)
sivel
added a commit
that referenced
this pull request
Feb 13, 2018
* Prevent reparenting a block with itself * Move __eq__ to Block, to avoid some unexpected problems (cherry picked from commit 76ff3e9)
sivel
added a commit
that referenced
this pull request
Feb 13, 2018
|
Since this is changing how some of the playbook objects compares work, I updated a branch with some playbook object compare tests at #36132 if it would be helpful for testing. |
ansibot
added
bug
This was referenced Apr 12, 2018
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
SUMMARY
We have been noticing that the parent tree for blocks is mutated and grows to large sizes. We have partially mitigated the problem in previous changes, but as a result the number of dynamic includes was limited in number due to recursion errors.
This change ensures that we never reparent a block with itself, causing a block explosion and recursion errors.
This also drastically speeds up dynamic task includes over time.
ISSUE TYPE
COMPONENT NAME
lib/ansible/playbook/base.py
lib/ansible/playbook/block.py
ANSIBLE VERSION
ADDITIONAL INFORMATION
Output of parent lineage from
Strategy._load_included_fileduring a successive 100include_taskss in a flat playbook (reproducer from #36053):Before this change (failing on the 42nd):
After this change (executes all 100
include_tasks):