Gitfs cleans up wrong directories

[nasenbaer13]

When running a file recurse state, copying some files fails because the wrong directory is cleaned up.

Sep 17 18:55:12 [DEBUG   ] LazyLoaded local_cache.clean_old_jobs
Sep 17 18:55:12 [DEBUG   ] Updating fileserver for git_pillar module
Sep 17 18:55:12 [DEBUG   ] Updating fileserver for git_pillar module
Sep 17 18:55:12 [DEBUG   ] Updating fileserver for git_pillar module
Sep 17 18:55:12 [DEBUG   ] Updating fileserver for git_pillar module
Sep 17 18:55:12 [DEBUG   ] Updating fileserver for git_pillar module
Sep 17 18:55:12 [DEBUG   ] Updating fileserver for git_pillar module
Sep 17 18:55:12 [DEBUG   ] Could not LazyLoad config.merge
Sep 17 18:55:12 [DEBUG   ] Updating git fileserver cache
Sep 17 18:55:12 [DEBUG   ] gitfs removed old cachedir /var/cache/salt/master/gitfs/gitfs
Sep 17 18:55:12 [INFO    ] Wrote new gitfs remote map to /var/cache/salt/master/gitfs/remote_map.txt
Sep 17 18:55:12 [DEBUG   ] Set lock for file:///repo.git
Sep 17 18:55:12 [DEBUG   ] gitfs is fetching from 'file:///repo.git'
Sep 17 18:55:12 [ERROR   ] Error in function _file_hash:
Sep 17 18:55:12 Traceback (most recent call last):
Sep 17 18:55:12   File "/usr/lib/python2.7/dist-packages/salt/master.py", line 1379, in run_func
Sep 17 18:55:12     ret = getattr(self, func)(load)
Sep 17 18:55:12   File "/usr/lib/python2.7/dist-packages/salt/fileserver/__init__.py", line 528, in file_hash
Sep 17 18:55:12     fnd = self.find_file(load['path'], load['saltenv'])
Sep 17 18:55:12   File "/usr/lib/python2.7/dist-packages/salt/fileserver/__init__.py", line 482, in find_file
Sep 17 18:55:12     fnd = self.servers[fstr](path, saltenv, **kwargs)
Sep 17 18:55:12   File "/usr/lib/python2.7/dist-packages/salt/fileserver/gitfs.py", line 143, in find_file
Sep 17 18:55:12     return gitfs.find_file(path, tgt_env=tgt_env, **kwargs)
Sep 17 18:55:12   File "/usr/lib/python2.7/dist-packages/salt/utils/gitfs.py", line 2100, in find_file
Sep 17 18:55:12     with salt.utils.fopen(lk_fn, 'w+') as fp_:
Sep 17 18:55:12   File "/usr/lib/python2.7/dist-packages/salt/utils/__init__.py", line 1208, in fopen
Sep 17 18:55:12     fhandle = open(*args, **kwargs)
Sep 17 18:55:12 IOError: [Errno 2] No such file or directory: '/var/cache/salt/master/gitfs/gitfs/hash/path/to/my/file.jinja.lk'
Sep 17 18:55:12 [DEBUG   ] Removed lock for file:///repo.git

The crucial line here is:

gitfs removed old cachedir /var/cache/salt/master/gitfs/gitfs

which removes the gitfs directory.

[jfindlay]

@nasenbaer13, thanks for the report.

[terminalmage]

No, it was correct the way it was. Can you please post your master config, as well as the version of Salt you are running? You shouldn't have the second "gitfs" there unless there is some misconfiguration.

[nasenbaer13]

We're running the 2015.8 branch.

root@53d3e44a05f4:/# salt-master --versions-report
Salt Version:
           Salt: 2015.8.0-43-g7834a93

Dependency Versions:
         Jinja2: 2.7.2
       M2Crypto: 0.21.1
           Mako: Not Installed
         PyYAML: 3.10
          PyZMQ: 14.0.1
         Python: 2.7.6 (default, Jun 22 2015, 17:58:13)
           RAET: Not Installed
        Tornado: 4.2.1
            ZMQ: 4.0.4
           cffi: Not Installed
       cherrypy: Not Installed
       dateutil: Not Installed
          gitdb: 0.5.4
      gitpython: Not Installed
          ioflo: Not Installed
        libnacl: Not Installed
   msgpack-pure: Not Installed
 msgpack-python: 0.3.0
   mysql-python: Not Installed
      pycparser: Not Installed
       pycrypto: 2.6.1
         pygit2: Not Installed
   python-gnupg: Not Installed
          smmap: 0.8.2
        timelib: Not Installed

System Versions:
           dist: Ubuntu 14.04 trusty
        machine: x86_64
        release: 3.13.0-62-generic
         system: Ubuntu 14.04 trusty

Our master config looks like this:

fileserver_backend:
  - git
  - roots

gitfs_provider: gitpython

gitfs_env_whitelist:
  - salt-*

gitfs_remotes:

# State tree used by the supermaster itself:
  - file:///repo.git:
    - name: commonstates
    - root: deployment/salt/common/states
  - file:///repo.git:
    - name: supermasterstates
    - root: deployment/salt/supermaster/states

# State trees and Pillar trees for export to minions:
  - file:///repo.git:
    - name: common
    - root: deployment/salt/common
    - mountpoint: salt://common
  - file:///repo.git:
    - name: instance-masters
    - root: deployment/salt/instance-masters
    - mountpoint: salt://instance-masters

file_roots:
  base:
    - /packages

ext_pillar:
  - git:
    - salt-beta file:///repo.git:
      - name: beta
      - root: deployment/salt/supermaster/pillar/beta
    - salt-staging file:///repo.git:
      - name: staging
      - root: deployment/salt/supermaster/pillar/staging
    - salt-www file:///repo.git:
      - name: www
      - root: deployment/salt/supermaster/pillar/www

I can also confirm, that this fixes our problem with the gitfs directory vanishing during file transfer.

[nasenbaer13]

For some reason, gitpython is not included in the output of --versions-report

root@53d3e44a05f4:/# pip freeze
GitPython==0.3.2.RC1
Jinja2==2.7.2
M2Crypto==0.21.1
MarkupSafe==0.18
PyYAML==3.10
apache-libcloud==0.18.0
argparse==1.2.1
async==0.6.1
backports.ssl-match-hostname==3.4.0.2
boto==2.38.0
certifi==2015.9.6.2
chardet==2.0.1
colorama==0.2.5
gitdb==0.5.4
html5lib==0.999
msgpack-python==0.3.0
pycrypto==2.6.1
pyzmq==14.0.1
requests==2.2.1
salt==2015.8.0-43-g7834a93
six==1.5.2
smmap==0.8.2
tornado==4.2.1
urllib3==1.7.1
wheel==0.24.0
wsgiref==0.1.2

[terminalmage]

It's not the correct fix, trust me. I'm not saying there's not a bug here, in fact I'm pretty sure the custom-named cachedir code (which was added for winrepo) just doesn't work properly with gitfs. I had a hunch you were using this, and while it's undocumented and not intended to be used for gitfs, it should still work if used.

[nasenbaer13]

Oh ok. we introduced the name parameters in our attempt to fix the issue. As far as I remember, we had the issue as well before using the repo names. I can verify this next week.

I am not sure how we should prevent the code above from deleting the gitfs directory during file transfer. It's cleaned up at that point. Or is the directory just wrong and has one gitfs too much?

/var/cache/salt/master/gitfs/gitfs/hash  -> /var/cache/salt/master/gitfs/hash

In the latter case, it will not be cleaned up by the code.

[terminalmage]

There is one too many "gitfs" there, and I'm not sure what would cause it as I can't reproduce this locally.

[terminalmage]

Are you doing anything with root_dir or cachedir in the master config?

[nasenbaer13]

What do you mean by 'doing anything'? The posted master config is complete. We'll have a look into the origin of the extra gitfs next week.

[terminalmage]

I didn't know if that was a complete master config, since you appear to have redacted information from at least the gitfs_remotes section. Modifying root_dir or cachedir in the master config would cause something like this.

[nasenbaer13]

You mean the space in line 11? This is actually present in our master config. So is this valid yaml? I also replaced two strings with company internals but nothing close to 'gitfs' ...

[terminalmage]

I don't know what you mean, line 11 appears to be empty.

[nasenbaer13]

OK, so I guess that should be no problem, since you were refering to missing information in the gitfs_remotes section. The master config above is the complete configuration file.

[terminalmage]

Here's what I'm seeing:

[root@cent6 ~]# mkdir /var/cache/salt/master/gitfs/someotherdir; salt-run -l debug fileserver.update 2>&1 | grep someotherdir
[DEBUG   ] gitfs removed old cachedir /var/cache/salt/master/gitfs/someotherdir

The extra "gitfs" on your system I can't explain, nor can I reproduce it.

[terminalmage]

@nasenbaer13 Can you update your master to the latest 2015.8 branch (which reverts your pull request), then before starting up the master again run rm -rf /var/cache/salt/master/gitfs and see how Salt creates the gitfs cache? Please also remove the "name" lines. When I looked at the code I didn't see anything that should make the "name" parameter interfere, but I'd like to remove as many variables as possible, and as you said earlier you were seeing this problem before you added those lines to your master config.

[nasenbaer13]

@terminalmage What about the GitBase initialization? Could the following code be the problem:

class GitBase(object):
    '''
    Base class for gitfs/git_pillar
    '''
    def __init__(self, opts, valid_providers=VALID_PROVIDERS, cache_root=None):
        self.opts = opts
        self.valid_providers = valid_providers
        self.get_provider()
        if cache_root is not None:
            self.cache_root = cache_root
        else:
            self.cache_root = os.path.join(self.opts['cachedir'], self.role)
        self.env_cache = os.path.join(self.cache_root, 'envs.p')
        self.hash_cachedir = os.path.join(
            self.cache_root, self.role, 'hash')
        self.file_list_cachedir = os.path.join(
            self.opts['cachedir'], 'file_lists', self.role)

If self.role is 'gitfs' and 'self.cache_root' is None, then gitfs is appended (path.join) to cache_root first. It is appended again two lines below when self.hash_cachedir is set. Thus self.hash_cachedir has gitfs/gitfs in its path which causes the directory to be cleaned up accidentally.

[nasenbaer13]

@jfindlay, since the 'fix' was reverted. I also think one should re-open the bug until a proper solution is merged. I'll test and prepare a pull request in time.

[nasenbaer13]

eyj@e6b3620

if @terminalmage considers this as a proper fix I would create a pull request vs. saltstack/salt/2015.8.

[terminalmage]

@nasenbaer13 I'll look at this today, working on a couple other things at the moment. Thanks very much for your time and effort!

[nasenbaer13]

No, problem. Just make sure that we fixed the correct position this time ;-)

[terminalmage]

@nasenbaer13 I was able to trigger the original issue, and I can confirm that your commit fixes it. I opened a pull request using your commit because I wanted to get the fix in as soon as possible: #27477

Thank you for providing this fix.

[nasenbaer13]

Thx for merging!

[terminalmage]

No problem, thanks again for all your help.