Bug 1829836: Remove the ansible_failed_task variable from all block/rescue instances. #1208
Conversation
…esources.
At this point in the role, we've finished overriding any variables that reference this meteringconfig_spec variable, which is a giant dictionary containing all of the potential MeteringConfig values, and we can safely store this result locally.
Before, we copied this variable to a tmp file that the reconcile_* task files reference to deploy metering-related resources:
```yaml
- name: Store MeteringConfig spec into values file
copy: content="{{ meteringconfig_spec }}" dest=/tmp/metering-values.yaml
- include_tasks: "{{ item }}"
loop:
- reconcile_metering.yml
- reconcile_monitoring.yml
- reconcile_hdfs.yml
- reconcile_hive.yml
- reconcile_presto.yml
- reconcile_reporting_operator.yml
- reconcile_reporting.yml
```
The problem, however, is that we still use this `meteringconfig_spec` variable under-the-hood to determine whether or not to create a resource:
```yaml
- name: Deploy presto resources
include_tasks: deploy_resources.yml
vars:
values_file: /tmp/metering-values.yaml
resources:
...
- template_file: templates/presto/presto-auth-secrets.yaml
apis: [ {kind: secret} ]
prune_label_value: presto-auth-secrets
create: "{{ meteringconfig_create_presto_auth_secrets }}"
```
Here, the `meteringconfig_create_presto_auth_secrets` variable is defined in the role's defaults/main.yml as the following:
```yaml
meteringconfig_create_presto_auth_secrets: "{{ _presto_spec.config.auth.enabled and _presto_spec.config.auth.createSecret | default(false) }}"
```
Where the value of this `_presto_spec` dictionary is pulled out of the result of `meteringconfig_spec.presto.spec`.
If we instead store the value of this variable locally before reconciling resources, role execution is greatly improved as we no longer need to template these variables (due to lazy evaluation) everytime we're looping over this list of resource definitions.
With the template caching changes made in 2.9.6, having to evaluate the result of the ansible_failed_task expression was greatly slowing down the role, and doesn't provide much value for debuggability purposes.
openshift-ci-robot
added
the
approved
| @@ -1,4 +1,7 @@ | |||
| --- | |||
| # | |||
| # Note: all of these tasks are wrapped in a block/rescue at the task file call site | |||
|
/lgtm |
timflannagan
changed the title
openshift-ci-robot
added
bugzilla/severity-high
bugzilla/valid-bug
|
@timflannagan1: This pull request references Bugzilla bug 1829836, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: EmilyM1, timflannagan1 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
The e2e test flaked on an irrelevant test and passed on everything else. Monday 04 May 2020 20:56:31 +0000 (0:00:00.024) 0:02:44.543 ************
===============================================================================
meteringconfig -------------------------------------------------------- 163.30s
gather_facts ------------------------------------------------------------ 1.11s
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
total ----------------------------------------------------------------- 164.41s
Playbook run took 0 days, 0 hours, 2 minutes, 44 secondsAfter making the changes to how the |
|
@timflannagan1: Overrode contexts on behalf of timflannagan1: ci/prow/metering-e2e-aws In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@timflannagan1: All pull requests linked via external trackers have merged: kube-reporting/metering-operator#1200, kube-reporting/metering-operator#1208. Bugzilla bug 1829836 has been moved to the MODIFIED state. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Overview
With the template caching changes made in 2.9.6, having to evaluate the result of the
ansible_failed_taskexpression was greatly slowing down the role and doesn't provide much value for debuggability purposes.Changed:
ansible_failed_taskreferencesmeteringconfig_specvariable locally.Storing the result of the
meteringconfig_specvariable locallyAt that point in the role, we've finished overriding any variables that reference this
meteringconfig_specvariable, which is a giant dictionary containing all of the overridden MeteringConfig default values, and we can safely store this result locally.Before this change, we copied this variable to a tmp file that the
reconcile_*task files reference to deploy metering-related resources:The problem, however, is that we still use this
meteringconfig_specvariable under-the-hood to determine whether or not to create a resource:Here, the
meteringconfig_create_presto_auth_secretsvariable is defined in the role's defaults/main.yml as the following:Where the value of this
_presto_specdictionary is pulled out of the result ofmeteringconfig_spec.presto.spec.If we instead store the value of this variable locally before reconciling resources, role execution is greatly improved as we no longer need to template these variables (due to lazy evaluation) every time we're looping over this list of resource definitions.