saltstack/salt — pull request #7630 by terminalmage (2013-10-05 01:31:29+00:00)
Title: Possible fix for custom grain chicken-and-egg issue

DO NOT BLINDLY MERGE. This needs ample discussion and review.

There is a chicken-and-egg issue with custom grains being reference in pillar data. If a newly-added custom grain is not yet synced to the minion, then due to the fact that pillar data needs to be compiled early on in the highstate call (to make pillar available in the top.sls), then if those not-yet-synced custom grains are referenced in pillar data, this results in pillar render errors. Later on, before the highstate is rendered, the custom types are synced (via ``salt.state.Highstate.load_dynamic()``), but with the pillar rendering errors still present in the pillar dict, the highstate will not be run and the highstate will exit with errors encountered in the failed initial pillar rendering.

This pull request re-gathers the pillar data if the call to ``salt.state.Highstate.load_dynamic()`` results in changes to grains. It also moves the check for pillar render errors from ``salt.modules.state.highstate()`` into ``salt.state.Highstate.call_highstate()`` (after the call to ``salt.state.Highstate.load_dynamic()``). If there are still pillar render errors, then the highstate rendering/and execution is skipped, and the pillar render errors are added to the list of errors compiled within ``salt.state.Highstate.call_highstate()``, which are eventually returned and printed to the CLI.

To replicate this bug (prior to testing this patch), do the following:

1. Make sure that ``file_roots`` and ``pillar_roots`` are set.

2. Setup your pillar ``top.sls`` targeting a single pillar SLS file, ``test``.

3. Configure the pillar ``test.sls`` like so:

```yaml
foo: {{ grains['mycustomgrain'] }}
```
4. Set up a custom grain in ``_grains/mycustomgrain.py`` like so:

```python
def mycustomgrain():
return {'mycustomgrain': 'Testing... 1... 2... 3...'}
```

5. Configure a roots ``top.sls`` targeting a single SLS file, ``test``.

6. Configure the roots ``test.sls`` like so:

```yaml
/tmp/foo.txt:
file.managed:
- contents_pillar: foo
```

7. On a minion which has not yet synced the custom grain run a ``state.highstate``, you'll see an error like the following:

```
virtucentos:
Data failed to compile:
----------
Pillar failed to render with the following messages:
----------
Rendering SLS 'test' failed, render error:
Undefined jinja variable; line 1 in template

---
foo: {{ grains['mycustomgrain'] }} <======================
---
```

8. After running a ``saltutil.sync_grains``, the above error will no longer be present on the next ``state.highstate``.

9. To revert and clear this custom grain to test again, move the ``mycustomgrain.py`` out of the ``_grains/`` dir and re-run the ``saltutil.sync_grains``, then move ``mycustomgrain.py`` back into the ``_grains/`` dir.

==========

There are a few points of discussion that need to be hit upon before we merge this, however:

1. ``load_dynamic()`` is only called for highstates, so this chicken-and-egg issue still exists when the execution functions ``state.sls`` and ``state.top`` are called, and the method I used to fix this issue for ``state.highstate`` doesn't apply to the way that states are executed using ``state.sls`` and ``state.top``. Since custom types are not synced when these two functions are run, however, I don't think that this is a problem. However, I wanted to bring this up to be sure.

2. Since I moved the check for pillar rendering errors into ``call_highstate()``, it does not have access to set ``__context__['retcode']``. I'm thinking the best/only way to maintain this functionality is to directly set the value using something like ``sys.modules[__salt__['test.ping'].__module__].__context__['retcode']``, correct? I haven't checked to see if the ``__salt__`` dict is available within state.py however.

Aside from that, I think this is good to merge, as it fixed the issue for me while still retaining proper reporting on actual pillar render errors. But, I think @thatch45 should review these changes very closely to make sure I didn't inadvertently break something in the process of making this fix.