From 78f92e9ab2bb1781686dd65a6904218cadf0647a Mon Sep 17 00:00:00 2001 From: Kris Raney Date: Thu, 28 Jan 2016 09:51:35 -0600 Subject: [PATCH 1/4] Fix for gitfs ext_pillar on standalone minion When configured to use a gitpython-based ext_pillar, salt-call --local (I don't have a master) was giving the error: Failed to checkout from git_pillar remote '': remote ref does not exist regardless of whether the branch existed or not. Poking around in the cache directory, I found that the cloned repository had a remote named 'origin' set, but no fetch had ever been done, so 'git checkout' would always fail. It doesn't know about any branches yet. Adding the fetch here, immediately after creating the remote, resolves the problem. I'm not totally sure this is the _correct_ fix as I'm not familiar with this code, and this code is used in many situations, not just minced. But I believe it to be harmless in the worst case, a redundant fetch. --- salt/utils/gitfs.py | 1 + 1 file changed, 1 insertion(+) diff --git a/salt/utils/gitfs.py b/salt/utils/gitfs.py index 10c656230662..00fb860a6121 100644 --- a/salt/utils/gitfs.py +++ b/salt/utils/gitfs.py @@ -565,6 +565,7 @@ def init_remote(self): 'remote.origin.fetch', '+refs/tags/*:refs/tags/*') self.repo.git.config('http.sslVerify', self.ssl_verify) + self.fetch() except os.error: # This exception occurs when two processes are trying to write # to the git config at once, go ahead and pass over it since From 53c4b4aaa406a2bd74decc6272424f3f1069ee5d Mon Sep 17 00:00:00 2001 From: Erik Johnson Date: Mon, 1 Feb 2016 23:56:05 -0600 Subject: [PATCH 2/4] gitfs: add initial fetch to pygit2 and dulwich --- salt/utils/gitfs.py | 43 ++++++++++++++++++++++++++++--------------- 1 file changed, 28 insertions(+), 15 deletions(-) diff --git a/salt/utils/gitfs.py b/salt/utils/gitfs.py index 00fb860a6121..1453590d3540 100644 --- a/salt/utils/gitfs.py +++ b/salt/utils/gitfs.py @@ -565,12 +565,13 @@ def init_remote(self): 'remote.origin.fetch', '+refs/tags/*:refs/tags/*') self.repo.git.config('http.sslVerify', self.ssl_verify) - self.fetch() except os.error: # This exception occurs when two processes are trying to write # to the git config at once, go ahead and pass over it since # this is the only write. This should place a lock down. pass + else: + new = True return new def dir_list(self, tgt_env): @@ -943,6 +944,8 @@ def init_remote(self): # to the git config at once, go ahead and pass over it since # this is the only write. This should place a lock down. pass + else: + new = True return new def dir_list(self, tgt_env): @@ -1614,10 +1617,24 @@ def init_remote(self): new = False if not os.listdir(self.cachedir): # Repo cachedir is empty, initialize a new repo there + self.repo = dulwich.repo.Repo.init(self.cachedir) + new = True + else: + # Repo cachedir exists, try to attach + try: + self.repo = dulwich.repo.Repo(self.cachedir) + except dulwich.repo.NotGitRepository: + log.error(_INVALID_REPO.format(self.cachedir, self.url)) + return new + + self.lockfile = os.path.join(self.repo.path, 'update.lk') + + # Read in config file and look for the remote + try: + conf = self.get_conf() + conf.get(('remote', 'origin'), 'url') + except KeyError: try: - self.repo = dulwich.repo.Repo.init(self.cachedir) - new = True - conf = self.get_conf() conf.set('http', 'sslVerify', self.ssl_verify) # Add remote manually, there is no function/object to do this conf.set( @@ -1630,17 +1647,10 @@ def init_remote(self): conf.write_to_path() except os.error: pass - else: - # Repo cachedir exists, try to attach - try: - self.repo = dulwich.repo.Repo(self.cachedir) - except dulwich.repo.NotGitRepository: - log.error(_INVALID_REPO.format(self.cachedir, self.url)) - return new - - self.lockfile = os.path.join(self.repo.path, 'update.lk') - - # No way to interact with remotes, so just assume success + else: + new = True + except os.error: + pass return new def walk_tree(self, tree, path): @@ -1748,6 +1758,9 @@ def init_remotes(self, remotes, per_remote_overrides): repo_obj.root = repo_obj.root.rstrip(os.path.sep) # Sanity check and assign the credential parameter repo_obj.verify_auth() + if repo_obj.new: + # Perform initial fetch + repo_obj.fetch() self.remotes.append(repo_obj) # Don't allow collisions in cachedir naming From 17dfec2dd434289d38601b33fa02e0bb9c244284 Mon Sep 17 00:00:00 2001 From: Erik Johnson Date: Tue, 2 Feb 2016 19:56:52 -0600 Subject: [PATCH 3/4] Only perform initial fetch when running on a minion This prevents the master's maintenance process from attempting to run a fetch while GitBase.init_remotes() is in the process fetching a newly-added remote. We have proper locking in place, so there is no danger of a race condition, but this does suppress the warning generated when an update lock is in place at the time a fetch is attempted. --- salt/utils/gitfs.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/salt/utils/gitfs.py b/salt/utils/gitfs.py index 1453590d3540..9c93a949d7dd 100644 --- a/salt/utils/gitfs.py +++ b/salt/utils/gitfs.py @@ -1758,7 +1758,7 @@ def init_remotes(self, remotes, per_remote_overrides): repo_obj.root = repo_obj.root.rstrip(os.path.sep) # Sanity check and assign the credential parameter repo_obj.verify_auth() - if repo_obj.new: + if self.opts['__role'] == 'minion' and repo_obj.new: # Perform initial fetch repo_obj.fetch() self.remotes.append(repo_obj) From 58c4c017435c95814f28c7d61d40b4ea61ab1d0c Mon Sep 17 00:00:00 2001 From: Erik Johnson Date: Tue, 2 Feb 2016 20:52:27 -0600 Subject: [PATCH 4/4] Add __role to master opts for gitfs integration tests --- tests/integration/fileserver/gitfs_test.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/tests/integration/fileserver/gitfs_test.py b/tests/integration/fileserver/gitfs_test.py index 6791f5d98571..925bcf323a47 100644 --- a/tests/integration/fileserver/gitfs_test.py +++ b/tests/integration/fileserver/gitfs_test.py @@ -93,7 +93,8 @@ def setUp(self): with patch.dict(gitfs.__opts__, {'cachedir': self.master_opts['cachedir'], 'gitfs_remotes': ['file://' + self.tmp_repo_dir], - 'sock_dir': self.master_opts['sock_dir']}): + 'sock_dir': self.master_opts['sock_dir'], + '__role': self.master_opts['__role']}): gitfs.update() def tearDown(self): @@ -108,7 +109,8 @@ def tearDown(self): def test_file_list(self): with patch.dict(gitfs.__opts__, {'cachedir': self.master_opts['cachedir'], 'gitfs_remotes': ['file://' + self.tmp_repo_dir], - 'sock_dir': self.master_opts['sock_dir']}): + 'sock_dir': self.master_opts['sock_dir'], + '__role': self.master_opts['__role']}): ret = gitfs.file_list(LOAD) self.assertIn('testfile', ret) @@ -116,7 +118,8 @@ def test_file_list(self): def test_dir_list(self): with patch.dict(gitfs.__opts__, {'cachedir': self.master_opts['cachedir'], 'gitfs_remotes': ['file://' + self.tmp_repo_dir], - 'sock_dir': self.master_opts['sock_dir']}): + 'sock_dir': self.master_opts['sock_dir'], + '__role': self.master_opts['__role']}): ret = gitfs.dir_list(LOAD) self.assertIn('grail', ret) @@ -124,7 +127,8 @@ def test_dir_list(self): def test_envs(self): with patch.dict(gitfs.__opts__, {'cachedir': self.master_opts['cachedir'], 'gitfs_remotes': ['file://' + self.tmp_repo_dir], - 'sock_dir': self.master_opts['sock_dir']}): + 'sock_dir': self.master_opts['sock_dir'], + '__role': self.master_opts['__role']}): ret = gitfs.envs() self.assertIn('base', ret)