diff options
| -rw-r--r-- | tests/unit/test_web.py | 22 | ||||
| -rw-r--r-- | tests/unit/test_zk.py | 24 | ||||
| -rw-r--r-- | zuul/connection/__init__.py | 49 | ||||
| -rw-r--r-- | zuul/driver/github/githubconnection.py | 10 | ||||
| -rw-r--r-- | zuul/zk/branch_cache.py | 66 |
5 files changed, 119 insertions, 52 deletions
diff --git a/tests/unit/test_web.py b/tests/unit/test_web.py index 5d38c5b19..ba0c10488 100644 --- a/tests/unit/test_web.py +++ b/tests/unit/test_web.py @@ -3282,3 +3282,25 @@ class TestWebStartup(ZuulTestCase): # If the config didn't load correctly, we won't have the jobs jobs = self.get_url("api/tenant/tenant-one/jobs").json() self.assertEqual(len(jobs), 10) + + +class TestWebUnprotectedBranches(BaseWithWeb): + config_file = 'zuul-github-driver.conf' + tenant_config_file = 'config/unprotected-branches/main.yaml' + + def test_no_protected_branches(self): + """Regression test to check that zuul-web doesn't display + config errors when no protected branch exists.""" + self.startWebServer() + tenant = self.scheds.first.sched.abide.tenants.get('tenant-one') + + project2 = tenant.untrusted_projects[1] + tpc2 = tenant.project_configs[project2.canonical_name] + + # project2 should have no parsed branch + self.assertEqual(0, len(tpc2.parsed_branch_config.keys())) + + # Zuul-web should not display any config errors + config_errors = self.get_url( + "api/tenant/tenant-one/config-errors").json() + self.assertEqual(len(config_errors), 0) diff --git a/tests/unit/test_zk.py b/tests/unit/test_zk.py index e2893e9df..e45629bef 100644 --- a/tests/unit/test_zk.py +++ b/tests/unit/test_zk.py @@ -1834,9 +1834,9 @@ class TestBranchCache(ZooKeeperBaseTestCase): sorted(cache.getProjectBranches('project1', True)), test_data['project1']['protected'] ) - self.assertEqual( - cache.getProjectBranches('project1', False), - None, + self.assertRaises( + LookupError, + lambda: cache.getProjectBranches('project1', False) ) cache.setProjectBranches('project1', False, @@ -1862,21 +1862,21 @@ class TestBranchCache(ZooKeeperBaseTestCase): }, } - self.assertEqual( - cache.getProjectBranches('project1', True), - None + self.assertRaises( + LookupError, + lambda: cache.getProjectBranches('project1', True) ) - self.assertEqual( - cache.getProjectBranches('project1', False), - None + self.assertRaises( + LookupError, + lambda: cache.getProjectBranches('project1', False) ) # Test the other order; all followed by protected-only cache.setProjectBranches('project1', False, test_data['project1']['all']) - self.assertEqual( - cache.getProjectBranches('project1', True), - None + self.assertRaises( + LookupError, + lambda: cache.getProjectBranches('project1', True) ) self.assertEqual( sorted(cache.getProjectBranches('project1', False)), diff --git a/zuul/connection/__init__.py b/zuul/connection/__init__.py index fb43fad4d..53be27d38 100644 --- a/zuul/connection/__init__.py +++ b/zuul/connection/__init__.py @@ -208,9 +208,9 @@ class ZKBranchCacheMixin: """ # Figure out which queries we have a cache for protected_branches = self._branch_cache.getProjectBranches( - project.name, True) + project.name, True, default=None) all_branches = self._branch_cache.getProjectBranches( - project.name, False) + project.name, False, default=None) # Update them if we have them if protected_branches is not None: @@ -238,24 +238,26 @@ class ZKBranchCacheMixin: :returns: The list of branch names. """ exclude_unprotected = tenant.getExcludeUnprotectedBranches(project) - if self._branch_cache: - branches = self._branch_cache.getProjectBranches( - project.name, exclude_unprotected, min_ltime) - else: - # Handle the case where tenant validation doesn't use the cache - branches = None + branches = None - if branches: + if self._branch_cache: + try: + branches = self._branch_cache.getProjectBranches( + project.name, exclude_unprotected, min_ltime) + except LookupError: + if self.read_only: + # A scheduler hasn't attempted to fetch them yet + raise ReadOnlyBranchCacheError( + "Will not fetch project branches as read-only is set") + else: + branches = None + + if branches is not None: return sorted(branches) - - if self.read_only: - if branches is None: - # A scheduler hasn't attempted to fetch them yet - raise ReadOnlyBranchCacheError( - "Will not fetch project branches as read-only is set") + elif self.read_only: # A scheduler has previously attempted a fetch, but got - # the empty list due to an error; we can't retry since - # we're read-only + # the None due to an error; we can't retry since we're + # read-only. raise RuntimeError( "Will not fetch project branches as read-only is set") @@ -265,14 +267,12 @@ class ZKBranchCacheMixin: except Exception: # We weren't able to get the branches. We need to tell # future schedulers to try again but tell zuul-web that we - # tried and failed. Set the branches to the empty list to - # indicate that we have performed a fetch and retrieved no - # data. Any time we encounter the empty list in the - # cache, we will try again (since it is not reasonable to - # have a project with no branches). + # tried and failed. Set the branches to None to indicate + # that we have performed a fetch and retrieved no data. Any + # time we encounter None in the cache, we will try again. if self._branch_cache: self._branch_cache.setProjectBranches( - project.name, exclude_unprotected, []) + project.name, exclude_unprotected, None) raise self.log.info("Got branches for %s" % project.name) @@ -313,7 +313,8 @@ class ZKBranchCacheMixin: # cache, so we never clear it. branches = self._branch_cache.getProjectBranches( - project_name, True) + project_name, True, default=None) + if not branches: branches = [] diff --git a/zuul/driver/github/githubconnection.py b/zuul/driver/github/githubconnection.py index 99631dda2..04759c15f 100644 --- a/zuul/driver/github/githubconnection.py +++ b/zuul/driver/github/githubconnection.py @@ -613,9 +613,13 @@ class GithubEventProcessor(object): project = self.connection.source.getProject(project_name) # Save all protected branches - old_protected_branches = set( - self.connection._branch_cache.getProjectBranches( - project_name, True)) + cached_branches = self.connection._branch_cache.getProjectBranches( + project_name, True, default=None) + + if cached_branches is None: + raise RuntimeError(f"No branches for project {project_name}") + old_protected_branches = set(cached_branches) + # Update the project banches self.log.debug('Updating branches for %s after ' 'branch protection rule "%s" was %s', diff --git a/zuul/zk/branch_cache.py b/zuul/zk/branch_cache.py index 08f0d6046..35d0c69cd 100644 --- a/zuul/zk/branch_cache.py +++ b/zuul/zk/branch_cache.py @@ -21,6 +21,9 @@ from zuul.zk.locks import SessionAwareReadLock, SessionAwareWriteLock, locked from kazoo.exceptions import NoNodeError +# Default marker to raise an exception on cache miss in getProjectBranches() +RAISE_EXCEPTION = object() + class BranchCacheZKObject(ShardedZKObject): """Store the branch cache in ZK @@ -42,6 +45,9 @@ class BranchCacheZKObject(ShardedZKObject): If a project is absent from the dict, it needs to be queried from the source. + If there was an error fetching the branches, None will be stored + as a sentinel value. + When performing an exclude_unprotected query, remove any duplicate branches from remaider to save space. When determining the full list of branches, combine both lists. @@ -117,44 +123,78 @@ class BranchCache: self.cache.remainder.pop(p, None) def getProjectBranches(self, project_name, exclude_unprotected, - min_ltime=-1): + min_ltime=-1, default=RAISE_EXCEPTION): """Get the branch names for the given project. + Checking the branch cache we need to distinguish three different + cases: + + 1. cache miss (not queried yet) + 2. cache hit (including empty list of branches) + 3. error when fetching branches + + If the cache doesn't contain any branches for the project and no + default value is provided a LookupError is raised. + + If there was an error fetching the branches, the return value + will be None. + + Otherwise the list of branches will be returned. + :param str project_name: The project for which the branches are returned. :param bool exclude_unprotected: Whether to return all or only protected branches. :param int min_ltime: The minimum cache ltime to consider the cache valid. + :param any default: + Optional default value to return if no cache entry exits. - :returns: The list of branch names, or None if the cache - cannot satisfy the request. + :returns: The list of branch names, or None if there was + an error when fetching the branches. """ if self.ltime < min_ltime: with locked(self.rlock): self.cache.refresh(self.zk_context) - protected_branches = self.cache.protected.get(project_name) - remainder_branches = self.cache.remainder.get(project_name) + protected_branches = None + try: + protected_branches = self.cache.protected[project_name] + except KeyError: + if exclude_unprotected: + if default is RAISE_EXCEPTION: + raise LookupError( + f"No branches for project {project_name}") + else: + return default + + if not exclude_unprotected: + try: + remainder_branches = self.cache.remainder[project_name] + except KeyError: + if default is RAISE_EXCEPTION: + raise LookupError( + f"No branches for project {project_name}") + else: + return default - if exclude_unprotected: - if protected_branches is not None: - return protected_branches - else: if remainder_branches is not None: return (protected_branches or []) + remainder_branches - return None + return protected_branches def setProjectBranches(self, project_name, exclude_unprotected, branches): """Set the branch names for the given project. + Use None as a sentinel value for the branches to indicate that + there was a fetch error. + :param str project_name: The project for the branches. :param bool exclude_unprotected: Whether this is a list of all or only protected branches. :param list[str] branches: - The list of branches + The list of branches or None to indicate a fetch error. """ with locked(self.wlock): @@ -162,13 +202,13 @@ class BranchCache: if exclude_unprotected: self.cache.protected[project_name] = branches remainder_branches = self.cache.remainder.get(project_name) - if remainder_branches: + if remainder_branches and branches: remainder = list(set(remainder_branches) - set(branches)) self.cache.remainder[project_name] = remainder else: protected_branches = self.cache.protected.get(project_name) - if protected_branches: + if protected_branches and branches: remainder = list(set(branches) - set(protected_branches)) else: |