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

Do not cache the loop item label so that it will update with each item #36430

Merged
merged 1 commit into from Feb 21, 2018
Merged

Do not cache the loop item label so that it will update with each item #36430

merged 1 commit into from Feb 21, 2018

Conversation

jctanner
Copy link
Contributor

@jctanner jctanner commented Feb 20, 2018

SUMMARY

In an item loop, the label is used to display context to the user. The value of the label can be templated with the item. After 2.4.x, the templating operation for the label was cached, and would not update for each item. This patch disabled the caching.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

task_executor and loop labels

ANSIBLE VERSION
2.6
ADDITIONAL INFORMATION
- hosts: localhost
  connection: local
  gather_facts: False
  vars:
      loopthis:
          - name: foo
            label: foo_label
          - name: bar
            label: bar_label
  tasks:
    - name: Ensure the state of iBGP neighborship
      debug:
        msg: "{{ looped_var.name }}"
      with_items: "{{ loopthis }}"
      loop_control:
        loop_var: looped_var
        label: "looped_var {{ looped_var.label }}"

Example output before patch:

PLAY [localhost] ****************************************************************************

TASK [Ensure the state of iBGP neighborship] ************************************************
ok: [localhost] => (item=looped_var foo_label) => {
    "changed": false,
    "failed": false,
    "looped_var": {
        "label": "foo_label",
        "name": "foo"
    }
}

MSG:

foo

ok: [localhost] => (item=looped_var foo_label) => {
    "changed": false,
    "failed": false,
    "looped_var": {
        "label": "bar_label",
        "name": "bar"
    }
}

MSG:

bar


PLAY RECAP **********************************************************************************
localhost                  : ok=1    changed=0    unreachable=0    failed=0

Example after patch:

PLAY [localhost] ****************************************************************************

TASK [Ensure the state of iBGP neighborship] ************************************************
ok: [localhost] => (item=looped_var foo_label) => {
    "changed": false,
    "failed": false,
    "looped_var": {
        "label": "foo_label",
        "name": "foo"
    }
}

MSG:

foo

ok: [localhost] => (item=looped_var bar_label) => {
    "changed": false,
    "failed": false,
    "looped_var": {
        "label": "bar_label",
        "name": "bar"
    }
}

MSG:

bar


PLAY RECAP **********************************************************************************
localhost                  : ok=1    changed=0    unreachable=0    failed=0

@ansibot ansibot added bugfix_pull_request needs_triage Needs a first human triage before being processed. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Feb 20, 2018
@jctanner jctanner requested a review from bcoca February 20, 2018 13:06
mkrizek added a commit to mkrizek/ansible that referenced this pull request Feb 20, 2018
@bcoca
Copy link
Member

bcoca commented Feb 20, 2018

shouldn't the jinja2 cache invalidate when the variables involved change? aka 'item'.

@bcoca bcoca removed the needs_triage Needs a first human triage before being processed. label Feb 20, 2018
@jctanner
Copy link
Contributor Author

jctanner commented Feb 20, 2018

@bcoca jinja is not the "cacher" in this case.

https://github.com/ansible/ansible/blob/devel/lib/ansible/template/__init__.py#L470-L473

When templar.template is told to use a cache, it bypasses do_template and jinja completely. If the templated label is sent to do_template, jinja is called and returns the correct result.

With a little bit of python-q injection, here's what happens under the covers ...

# first item ...
 1.0s template: 'calling do_template'='calling do_template'
from __future__ import division
from jinja2.runtime import LoopContext, TemplateReference, Macro, Markup, TemplateRuntimeError, missing, concat, escape, markup_join, unicode_join, to_string, identity, TemplateNotFound, Namespace
name = None

def root(context, missing=missing, environment=environment):
    resolve = context.resolve_or_missing
    undefined = environment.undefined
    if 0: yield None
    l_0_looped_var = resolve('looped_var')
    pass
    yield u'looped_var '
    yield to_string(environment.finalize(environment.getattr((undefined(name='looped_var') if l_0_looped_var is missing else l_0_looped_var), 'label')))

blocks = {}
debug_info = '1=12' 1.0s compile:
      source="from __future__ import division\nfrom jinja2.runt...var), 'label')))\n\nblocks = {}\ndebug_info = '1=12'" (file:///tmp/q)
 1.0s template: result=u'looped_var foo_label'

# second item ...
  1.1s template: 'calling do_template'='calling do_template'
from __future__ import division
from jinja2.runtime import LoopContext, TemplateReference, Macro, Markup, TemplateRuntimeError, missing, concat, escape, markup_join, unicode_join, to_string, identity, TemplateNotFound, Namespace
name = None

def root(context, missing=missing, environment=environment):
    resolve = context.resolve_or_missing
    undefined = environment.undefined
    if 0: yield None
    l_0_looped_var = resolve('looped_var')
    pass
    yield u'looped_var '
    yield to_string(environment.finalize(environment.getattr((undefined(name='looped_var') if l_0_looped_var is missing else l_0_looped_var), 'label')))

blocks = {}
debug_info = '1=12' 1.1s compile:
      source="from __future__ import division\nfrom jinja2.runt...var), 'label')))\n\nblocks = {}\ndebug_info = '1=12'" (file:///tmp/q)
 1.1s template: result=u'looped_var bar_label

Without this patch, jinja's environment.compile is never invoked for bar_label. Based on what little I can find out jinja's "caching", there is no cache for results. It only caches template source.

https://stackoverflow.com/a/4214506
http://jinja.pocoo.org/docs/2.10/api/#loaders
http://jinja.pocoo.org/docs/2.10/api/#bytecode-cache

@bcoca bcoca merged commit d1f7693 into ansible:devel Feb 21, 2018
@bcoca bcoca added this to To Do in 2.5.x blocker list via automation Feb 21, 2018
@bcoca bcoca moved this from To Do to In Progress in 2.5.x blocker list Feb 21, 2018
gundalow pushed a commit to gundalow/ansible that referenced this pull request Feb 21, 2018
nitzmahone added a commit to nitzmahone/ansible that referenced this pull request Feb 22, 2018
Add integration test for ansible#36430 (ansible#36432)

(cherry picked from commit d1f7693)
(cherry picked from commit 9fced4f)
nitzmahone added a commit that referenced this pull request Feb 23, 2018
#36609)

Add integration test for #36430 (#36432)

(cherry picked from commit d1f7693)
(cherry picked from commit 9fced4f)
@nitzmahone nitzmahone moved this from In Progress to Done in 2.5.x blocker list Feb 23, 2018
@ansibot ansibot added bug This issue/PR relates to a bug. and removed bugfix_pull_request labels Mar 6, 2018
@ansible ansible locked and limited conversation to collaborators Apr 27, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug This issue/PR relates to a bug. support:core This issue/PR relates to code supported by the Ansible Engineering Team.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants