Error logging with non-environment branches in gitfs

[thusoy]

Description of Issue/Question

I was seeing error logs from all minions when running highstate of the form [ERROR ] Template was specified incorrectly: False when using a gitfs-backed master. The error was logged once for each non-master branch in the gitfs repo, which I wasn't using as environments and thus didn't have a topfile (topfile is in roots, only states are loaded over gitfs), which I believe was the cause of the logging. Setting gitfs_env_whitelist to only accept base fixes it, but only after clearing the cached environments in /var/cache/salt/master/gitfs/envs.p.

I see two potential solutions to two slightly different issues here, the first being that something is logged with severity error when it doesn't really matter much, it's only erroneous if no topfile is found at all. Also related, the error message doesn't give any indication to what the problem is or how it can be remedied. The other issue is that cached data persists across master restarts, and might not be valid for the new config. This can be approached either as always clearing out cached data on restarts, assuming restarts are rare. This will probably also solve other issues where config settings seemingly have no effect due to cached data. The other approach is associating cached data with a config, thus clearing out the cache on start if the config has changed since it was written, assuming that restarts are more frequent than config changes.

Iggy suggested on IRC that #30434 might be related.

Setup

# /etc/salt/master
fileserver_backend:
    - roots
    - git
gitfs_remotes:
    - https://github.com/thusoy/salt-states
# Uncommenting these lines, restarting master and clearing cache solves the issue
#gitfs_env_whitelist:
#    - base
gitfs_root: salt
gitfs_ssl_verify: True

Steps to Reproduce Issue

Serve topfile over roots and have a gitfs repo with a non-master branch without a topfile.

Versions Report

Salt Version:
           Salt: 2015.8.7

Dependency Versions:
         Jinja2: 2.7.3
       M2Crypto: Not Installed
           Mako: Not Installed
         PyYAML: 3.11
          PyZMQ: 14.4.0
         Python: 2.7.9 (default, Mar  1 2015, 12:57:24)
           RAET: Not Installed
        Tornado: 4.2.1
            ZMQ: 4.0.5
           cffi: 1.6.0
       cherrypy: Not Installed
       dateutil: 2.5.2
          gitdb: Not Installed
      gitpython: Not Installed
          ioflo: Not Installed
        libgit2: Not Installed
        libnacl: Not Installed
   msgpack-pure: Not Installed
 msgpack-python: 0.4.2
   mysql-python: 1.2.3
      pycparser: 2.14
       pycrypto: 2.6.1
         pygit2: Not Installed
   python-gnupg: 2.0.2
          smmap: Not Installed
        timelib: Not Installed

System Versions:
           dist: debian 8.3 
        machine: x86_64
        release: 3.16.0-4-amd64
         system: debian 8.3 

Also confirmed that the same error occurs in 2016.3.

[Ch3LL]

@thusoy I am able to setup and see these errors int he logs as well:

2016-05-24 20:18:35,524 [salt.template    ][ERROR   ][7812] Template was specified incorrectly: False
2016-05-24 20:18:35,529 [salt.fileclient  ][DEBUG   ][7812] Could not find file from saltenv 'better-patch', u'salt://top.sls'
2016-05-24 20:18:35,529 [salt.state       ][DEBUG   ][7812] No contents loaded for env: better-patch
2016-05-24 20:18:35,529 [salt.template    ][DEBUG   ][7812] compile template: False
2016-05-24 20:18:35,529 [salt.template    ][ERROR   ][7812] Template was specified incorrectly: False
2016-05-24 20:18:35,533 [salt.fileclient  ][DEBUG   ][7812] Could not find file from saltenv 'ca', u'salt://top.sls'
2016-05-24 20:18:35,533 [salt.state       ][DEBUG   ][7812] No contents loaded for env: ca
2016-05-24 20:18:35,534 [salt.template    ][DEBUG   ][7812] compile template: False

And I think you are right in that its not finding a top.sls file.

Also same results when changing the gitfs_env_whitelist to base. I had to remove cache to get it to work. Looking at what gitfs_env_whitelist does it seems like this might be what this is designed for to ensure other branches are not accounted for in a repo:

Used to restrict which environments are made available. Can speed up state runs if the repos in gitfs_remotes contain many branches/tags. More information can be found in the GitFS Walkthrough.

I do think that this is all expected behavior but @terminalmage is more familiar with this and could give us some more insight:
terminalimage:

1. Do you believing having to set gitfs_env_whitelist to ensure gitfs does not look for a top file in that branch expected behavior?
2. Also do you think we could possibly add some better logging for when there is not a top file in a branch? For example maybe something like "cannot find a top file in this branch"
3. I believe having to wipe away the cache would be by design as well, but maybe we could improve our docs to state the need to wipe away the cache when this setting is changed.

[terminalmage]

I think that this is probably just well-intentioned logging gone awry. I'm investigating.

[terminalmage]

I have submitted a fix for this in #33679. It seems that we were simply unnecessarily invoking compile_template() when no top file data was found for a given environment.

[thusoy]

Great work @terminalmage, appreciated!

[terminalmage]

No problem @thusoy, thanks for alerting us to this.