zuul
/
zuul
Code Issues Proposed changes

Fix read-only branches error in zuul-web 

When exclude-unprotected-branche is in effect and a project doesn't have
any protected branches (e.g. a wrong branch protection rule in Github or
none at all) the branch cache will contain an empty list.

Since the empty list was so far used to indicate a fetch error, those
projects showed up with a config error in zuul-web ("Will not fetch
project branches as read-only is set").

For the branch cache we need to distinguish three different cases:
1. branche cache miss (no fetch yet)
2. branch cache hit (including empty list of branches)
3. fetch error

Instead of storing an empty list in case of a fetch error we will store
a sentinel value of `None` in those cases. However, with this change we
can't use `None` as an indicator for a cache miss anymore, so we are now
raising a `LookupError` instead.

Change-Id: I5b51805f7d0a5d916a46fe89db7b32f14063d7aa
This commit is contained in:
Simon Westphahl 2022-06-30 10:46:15 +02:00
parent 9438b47e1e
commit c98f14025a
5 changed files with 118 additions and 51 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

22
tests/unit/test_web.py
View File

@ -3270,3 +3270,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)

24
tests/unit/test_zk.py
View File

@ -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)),

47
zuul/connection/__init__.py
View File

@ -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)
branches = None
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
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:
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 = []

10
zuul/driver/github/githubconnection.py
View File

@ -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',

66
zuul/zk/branch_cache.py
View File

@ -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: