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

Skip forking on a task with a simple when condition or include #70477

Draft
wants to merge 13 commits into
base: devel
Choose a base branch
from

Conversation

sivel
Copy link
Member

@sivel sivel commented Jul 6, 2020

SUMMARY

This PR does a conditional evaluation on tasks that do not have loops, and skips forking if the task should be skipped, or if the task is an include.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME
lib/ansible/plugins/strategy/__init__.py
ADDITIONAL INFORMATION

Using a playbook with an import_tasks and when: false with 100 ping tasks:

With this PR:

real	0m1.718s

Without:

real	0m4.134s

A playbook that I have that does 100 recursive task includes (also does some debug and set_fact):

With this PR:

real	0m16.332s

Without:

real	0m23.221s

@samdoran samdoran added the ci_verified Changes made in this PR are causing tests to fail. label Jul 6, 2020
@ansibot ansibot added affects_2.11 bug This issue/PR relates to a bug. core_review In order to be merged, this PR must follow the core review workflow. needs_triage Needs a first human triage before being processed. support:core This issue/PR relates to code supported by the Ansible Engineering Team. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. support:community This issue/PR relates to code supported by the Ansible community. test This PR relates to tests. and removed ci_verified Changes made in this PR are causing tests to fail. core_review In order to be merged, this PR must follow the core review workflow. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jul 6, 2020
@sivel sivel changed the title Skip forking on a task with a simple when condition Skip forking on a task with a simple when condition or include Jul 8, 2020
@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. core_review In order to be merged, this PR must follow the core review workflow. and removed core_review In order to be merged, this PR must follow the core review workflow. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jul 8, 2020
@samdoran samdoran removed the needs_triage Needs a first human triage before being processed. label Jul 9, 2020
raise ForkShortCircuit(tr)

def _short_circuit_fork_include(self, host, task, task_vars, templar):
vars_copy = task_vars.copy()
Copy link
Contributor

@jborean93 jborean93 Jul 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be better to do the copy in the if statement. Not sure how expensive this operation is but I would have thought task_vars could be quite large if the host has a lot of vars/facts set on it.

@@ -207,6 +207,9 @@
become: yes
shell: "echo 'SHOW SERVER_VERSION' | psql --tuples-only --no-align --dbname postgres"
register: postgres_version_resp
until: postgres_version_resp is successful
Copy link
Contributor

@jborean93 jborean93 Jul 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this meant to be here?

Copy link
Member Author

@sivel sivel Jul 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was done because the speedup we get from this results in postgres not having enough time to start up. If this ever merges, I think this incidental test will be removed by then.

include_args = task.args.copy()
include_file = include_args.pop('_raw_params', None)
if not include_file:
tr = TaskResult(
Copy link
Contributor

@jborean93 jborean93 Jul 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You set tr here but don't do anything with it. The code will just continue along and probably fail at 366 include_file = templar.template(include_file) because include_file is None.

@ansibot ansibot added stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. and removed stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Jul 17, 2020
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Jul 26, 2020
@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. pre_azp This PR was last tested before migration to Azure Pipelines. and removed core_review In order to be merged, this PR must follow the core review workflow. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Dec 9, 2020
@ansibot ansibot added collection Related to Ansible Collections work collection:ktdreyer.errata_tool_ansible labels Jan 23, 2021
@ansibot ansibot added the WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. label Feb 16, 2021
@sivel sivel force-pushed the skip-fork-simple-when branch from 8f6d77c to a47d343 Compare Dec 8, 2021
@ansibot ansibot removed pre_azp This PR was last tested before migration to Azure Pipelines. support:community This issue/PR relates to code supported by the Ansible community. labels Dec 8, 2021
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Mar 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects_2.11 bug This issue/PR relates to a bug. collection Related to Ansible Collections work needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. support:core This issue/PR relates to code supported by the Ansible Engineering Team. test This PR relates to tests. WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants