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

Recursion almost broken #16749

Closed
hryamzik opened this Issue Jul 18, 2016 · 10 comments

Comments

Projects
None yet
5 participants
@hryamzik
Contributor

hryamzik commented Jul 18, 2016

ISSUE TYPE
  • Bug Report
ANSIBLE VERSION
ansible 2.1.0.0
OS / ENVIRONMENT

Mac OS

SUMMARY

Recursion includes gets slower and slower till become useless.

STEPS TO REPRODUCE
- hosts: localhost
  become: no
  become_method: sudo
  vars:
      i: 0
  tasks:
      - include: include_me.yml

      - debug: msg="Exited, i={{ i }}"

include_me.yml:

- debug: var=i

- set_fact:
    i: "{{ (i|int) + 1 }}"

- include: include_me.yml
  when: "{{ i|int < 30 }}"

Playbook run took 0 days, 1 hours, 38 minutes, 3637 seconds

This simple sequence takes forever to execute on my retina MBP. In my real life scenario hundreds of iterations are used to download a full backup with all the incrementals so this issue breaks the whole idea of searching for previous backup with ansible.

@jimi-c

This comment has been minimized.

Member

jimi-c commented Aug 3, 2016

So testing out my performance branch (https://github.com/ansible/ansible/compare/performance_improvements), and upping the level of recursion even to 200 levels deep results in an execution time of about 33 seconds:

$ time ansible-playbook -vv test.yml -e max_depth=200
...
PLAY RECAP *********************************************************************
localhost                  : ok=601  changed=0    unreachable=0    failed=0   
real    0m33.785s
user    0m24.153s
sys    0m6.773s

If you'd like to help test out that branch, I'd appreciate it!

@jeremyeder

This comment has been minimized.

jeremyeder commented Aug 3, 2016

James -- hopefully you saw the results I sent you by email.

On Aug 3, 2016 5:46 PM, "James Cammarata" notifications@github.com wrote:

So testing out my performance branch (
https://github.com/ansible/ansible/compare/performance_improvements), and
upping the level of recursion even to 200 levels deep results in an
execution time of about 33 seconds:

$ time ansible-playbook -vv test.yml -e max_depth=200
...
PLAY RECAP *********************************************************************
localhost : ok=601 changed=0 unreachable=0 failed=0

real 0m33.785s
user 0m24.153s
sys 0m6.773s

If you'd like to help test out that branch, I'd appreciate it!


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#16749 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AB1KmNHlr6eBaGJPHBqffFodszKKaKCAks5qcQwkgaJpZM4JO5Ns
.

@jimi-c

This comment has been minimized.

Member

jimi-c commented Aug 4, 2016

@jeremyeder I did, those were using this performance branch? I was under the impression that those were with restructured playbooks rather than a different branch.

@hryamzik

This comment has been minimized.

Contributor

hryamzik commented Aug 4, 2016

Is there any ETA for getting this in released version?

@jeremyeder

This comment has been minimized.

jeremyeder commented Aug 4, 2016

Those were using both restructured playbooks and your branch.

@jimi-c

This comment has been minimized.

Member

jimi-c commented Aug 4, 2016

@hryamzik the goal is to get this into 2.2, which is aiming for a September release. Since this patch introduces some fairly major restructuring internally, I'd really like some additional testing on it to make sure we have a good level of confidence that nothing was horribly broken before merging it into devel.

@jeremyeder excellent, I didn't realize Andrew had gotten past some of the early bugs in that branch. There was a lot of work later in the day yesterday, so I'd appreciate it if you re-ran those tests with the most recent version as well.

@jeremyeder

This comment has been minimized.

jeremyeder commented Aug 4, 2016

Will do...but not until end of next week /cc @mffiedler @timothysc @abutcher

@jimi-c jimi-c added the performance label Aug 5, 2016

@jimi-c jimi-c added this to the 2.2.0 milestone Aug 5, 2016

@jimi-c jimi-c self-assigned this Aug 5, 2016

@jeremyeder

This comment has been minimized.

jeremyeder commented Aug 10, 2016

Here is the resource usage of ansible2.2-performance_improvements branch. To deploy a 100-node openshift cluster, the install took 21m48s.

3 masters
3 etcd
1 apiserver load balancer
2 infrastructure nodes
100 compute nodes

ansible22-pi

Overall this is a significant improvement over ansible-2.1 where the same work would not complete given 64GB of RAM and over 8 hours of runtime before OOM.

/cc @mffiedler @timothysc @abutcher

@jimi-c

This comment has been minimized.

Member

jimi-c commented Aug 10, 2016

@jeremyeder it looks like it shaved about 20 minutes off the execution time from the previous results you emailed me?

@jimi-c

This comment has been minimized.

Member

jimi-c commented Aug 10, 2016

As the performance improvements branch has been merged into devel now, and as the deeply nested include speed issue has been resolved, we're going to go ahead and close this issue now. These improvements will be included in 2.2.0 (currently targeted for release in September of 2016).

If you continue seeing any problems related to this issue, or if you have any further questions, please let us know by stopping by one of the two mailing lists, as appropriate:

Because this project is very active, we're unlikely to see comments made on closed tickets, but the mailing list is a great way to ask questions, or post if you don't think this particular issue is resolved.

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment