zuul/zuul
Code Issues Proposed changes

Fix AtributeError when canceling merge requests

Browse Source 
When trying to cancel a merge request, Zuul raises the following
exception:

    Traceback (most recent call last):
      File "/opt/zuul/lib/python3.11/site-packages/zuul/configloader.py", line 2500, in _cacheTenantYAML
        self.merger.cancel(cancel_job)
      File "/opt/zuul/lib/python3.11/site-packages/zuul/merger/client.py", line 264, in cancel
        request = self.merger_api.get(job.request_path)
                  ^^^^^^^^^^^^^^^^^^^
    AttributeError: 'MergerApi' object has no attribute 'get'

The MergerApi.get() method was renamed to MergerApi.getRequest() in
Ie3d62dadd9990c847df8302d4da801a990394d2c, but apparently this call in
the cancel procedure was forgotten.

Since getRequest is using the job UUID rather than the request_path, we
first have to extract the UUID from the path.

This enhances the TestingMergerApi to allow explicitly failing merge
jobs for a given job type. To make this work properly we also have to
use the TestingMergerApi on the executor server that is used within the
Zuul tests.

Change-Id: Ib2f81959202adc08aff5eb905f3d73b10cd2891c
This commit is contained in:
Felix Edel
2025-04-24 10:23:19 +02:00
parent b70717e9aa
commit cef8f49859
4 changed files with 69 additions and 2 deletions
Download Patch File Download Diff File Expand all files Collapse all files

29
tests/base.py
View File

@ -866,6 +866,10 @@ class TestingMergerApi(HoldableMergerApi):
log = logging.getLogger("zuul.test.TestingMergerApi")
def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)
self.job_types_to_fail = []
def _test_getMergeJobsInState(self, *states):
# As this method is used for assertions in the tests, it should look up
# the merge requests directly from ZooKeeper and not from a cache
@ -879,6 +883,29 @@ class TestingMergerApi(HoldableMergerApi):
return sorted(all_merge_requests)
def failJobs(self, job_type):
"""
Lets all merge jobs of the given type fail.
The actual implementation is done by overriding reportResult().
"""
# As the params of a merge job are cleared directly after it's
# picked up by a merger, we don't have much information to
# identify a running merge job. The easiest solution is to
# fail all merge jobs of a given type, which should be fine
# for test purposes.
# We are failing merge jobs by type as the params are directly
# cleared when the merge job is picked up by a merger. Thus, we
# cannot compare information like project or branch in
# reportResult() where we set the merge job's result to failed.
self.job_types_to_fail.append(job_type)
def reportResult(self, request, result):
# Set updated to False to mark a merge job as failed
if request.job_type in self.job_types_to_fail:
result["updated"] = False
return super().reportResult(request, result)
def release(self, merge_request=None):
"""
Releases a merge request which was previously put on hold for testing.
@ -1016,6 +1043,7 @@ class RecordingExecutorServer(zuul.executor.server.ExecutorServer):
"""
_job_class = RecordingAnsibleJob
_merger_api_class = TestingMergerApi
def __init__(self, *args, **kw):
self._run_ansible = kw.pop('_run_ansible', False)
@ -3128,6 +3156,7 @@ class ZuulTestCase(BaseTestCase):
self.merger_api.hold_in_queue = hold_in_queue
for app in self.scheds:
app.sched.merger.merger_api.hold_in_queue = hold_in_queue
self.executor_server.merger_api.hold_in_queue = hold_in_queue
@property
def hold_nodeset_requests_in_queue(self):

36
tests/unit/test_configloader.py
View File

@ -66,6 +66,42 @@ class TestConfigLoader(ZuulTestCase):
self.assertEqual(tenant.name, tenant_name)
@okay_tracebacks(' _processCatJob')
def test_merge_job_cleanup(self):
# When issuing a full reconfiguration or a tenant validation, we
# cannot hold the merge jobs in queue as the configloader is
# explicitly waiting for the cat jobs to complete. Thus, instead
# of holding the merge jobs we directly fail those cat jobs to
# trigger the merge request cleanup.
self.merger_api.failJobs(MergeRequest.CAT)
self.executor_server.merger_api.failJobs(MergeRequest.CAT)
self.assertRaises(
Exception,
lambda: self.scheds.execute(lambda app: app.sched.validateTenants(
app.config, {'tenant-one'})),
)
self.waitUntilSettled()
# Even if the merge jobs are canceled, there could be a race
# condition where an executor picks up a merge request and
# re-creates the result znode in ZK after the request got
# deleted. To prevent assertCleanZooKeeper to fail this test
# even if the cancelation was successful, we have to manually
# clean up lost merge requests.
self.merger_api.cleanup()
# As we are explicitly failing cat jobs we are expecting
# validateTenants() to fail with an exception.
# As this catches other exceptions that might be raised when
# canceling the merge jobs, there is no proper way to validate
# this apart from checking the test/exception logs for specific
# messages.
for record in self._exception_logs:
if "Unable to cancel job" in record.msg:
self.fail(
f"Exception while canceling merge jobs: {record.msg}")
class TenantParserTestCase(ZuulTestCase):
create_project_keys = True

3
zuul/merger/client.py
View File

@ -261,7 +261,8 @@ class MergeClient(object):
def cancel(self, job):
try:
# Try to remove the request first
request = self.merger_api.get(job.request_path)
request_uuid = job.request_path.split('/')[-1]
request = self.merger_api.getRequest(request_uuid)
if request:
if self.merger_api.lock(request, blocking=False):
try:

3
zuul/merger/server.py
View File

@ -70,6 +70,7 @@ class BaseMergeServer(metaclass=ABCMeta):
log = logging.getLogger("zuul.BaseMergeServer")
_repo_locks_class = BaseRepoLocks
_merger_api_class = MergerApi
def __init__(
self,
@ -110,7 +111,7 @@ class BaseMergeServer(metaclass=ABCMeta):
self.merger_loop_wake_event = threading.Event()
self.merger_api = MergerApi(
self.merger_api = self._merger_api_class(
self.zk_client,
merge_request_callback=self.merger_loop_wake_event.set,
)