Rename MERGER_FAILURE to MERGE_CONFLICT

This is a prelude to a change which will report a distinct buildset result
to the database if the upstream code review system is unable to merge a change.
Currently it is reported as MERGER_FAILURE which makes it difficult to
distinguish from merge conflicts.

Essentially, the two states we're interested in are when Zuul's merger is
unable to prepare a git checkout of the change (99% of the time, this is
a merge conflict).  This will be known as MERGE_CONFLICT now.

The second state is when Zuul asks Gerrit/Github/etc to submit/merge a change
and the remote system is unable (or refuses) to do so.  In a future change,
that will be reported as MERGE_FAILURE.

To avoid confusion and use names which better reflect the situation, this change
performs the rename to MERGE_CONFLICT.

Because there are pipeline configuration options tied to the MERGER_FAILURE
status (which start with 'merge-failure') they are also renamed to 'merge-conflict'.
The old names are supported for backwards compatibility.

A SQL migration takes care of updating values in the database.

The upgrade procedure is noted as being special because of the db value updates.
If an operator doesn't follow the recommended procedure, however, the consequences
are minimal (builds which won't be easily queried in the web ui; that can be
manually corrected if desired).

A model API change is not needed since the only place where we receive this value
from ZK can be updated to accept both values.

Change-Id: I3050409ed68805c748efe7a176b9755fa281536f
This commit is contained in:
James E. Blair 2022-02-24 15:38:45 -08:00
parent 99b0eade8c
commit e03d8c887c
25 changed files with 291 additions and 57 deletions

View File

@ -207,7 +207,7 @@ success, the pipeline reports back to Gerrit with ``Verified`` vote of
The introductory text in reports when an item is enqueued.
Empty by default.
.. attr:: merge-failure-message
.. attr:: merge-conflict-message
:default: Merge failed.
The introductory text in the message reported when a change
@ -329,10 +329,10 @@ success, the pipeline reports back to Gerrit with ``Verified`` vote of
These reporters describe what Zuul should do if at least one job
fails.
.. attr:: merge-failure
.. attr:: merge-conflict
These reporters describe what Zuul should do if it is unable to
merge in the patchset. If no merge-failure reporters are listed
merge in the patchset. If no merge-conflict reporters are listed
then the ``failure`` reporters will be used to notify of
unsuccessful merges.
@ -377,7 +377,7 @@ success, the pipeline reports back to Gerrit with ``Verified`` vote of
If set, a pipeline can enter a *disabled* state if too many
changes in a row fail. When this value is exceeded the pipeline
will stop reporting to any of the **success**, **failure** or
**merge-failure** reporters and instead only report to the
**merge-conflict** reporters and instead only report to the
**disabled** reporters. (No **start** reports are made when a
pipeline is disabled).

View File

@ -105,7 +105,7 @@ Reporter Configuration
This reporter is used to store build results in an Elasticsearch index.
The Elasticsearch reporter does nothing on :attr:`pipeline.start` or
:attr:`pipeline.merge-failure`; it only acts on
:attr:`pipeline.merge-conflict`; it only acts on
:attr:`pipeline.success` or :attr:`pipeline.failure` reporting stages.
.. attr:: pipeline.<reporter>.<elasticsearch source>

View File

@ -22,7 +22,7 @@ An MQTT report uses this schema:
.. attr:: action
The reporter action name, e.g.: 'start', 'success', 'failure',
'merge-failure', ...
'merge-conflict', ...
.. attr:: tenant

View File

@ -285,7 +285,7 @@ These metrics are emitted by the Zuul :ref:`scheduler`:
Incremented to represent the status of a Zuul executor's merger
operations. ``<result>`` can be either ``SUCCESS`` or ``FAILURE``.
A failed merge operation which would be accounted for as a ``FAILURE``
is what ends up being returned by Zuul as a ``MERGER_FAILURE``.
is what ends up being returned by Zuul as a ``MERGE_CONFLICT``.
.. stat:: builds
:type: counter

View File

@ -0,0 +1,30 @@
---
upgrade:
- |
The buildset result ``MERGER_FAILURE`` has been renamed to
``MERGE_CONFLICT``, and the pipeline reporter configuration
``merge-failure`` has been renamed to ``merge-conflict``.
These are more descriptive of the most common errors actually
reported, and so are expected to be less confusing to users. This
is also in service of a future change to support a new buildset
result ``MERGE_FAILURE`` which will indicate that the change was
unable to be merged in the upstream repository.
When upgrading, it is recommended to stop all schedulers briefly
(i.e, when the first scheduler of the new version starts, there
should be no schedulers running the old version). The new
scheduler will perform a database migration when it starts and
update all existing ``MERGER_FAILURE`` buildset results to
``MERGE_CONFLICT``. If old schedulers are running, they may
continue to add ``MERGER_FAILURE`` entries which will need to be
manually updated in order to be visible in the web UI or rest API.
deprecations:
- |
The ``merge-failure`` and ``merge-failure-message`` pipeline
configuration options have been renamed to ``merge-conflict`` and
``merge-conflict-message`` respectively. The old settings are
retained for backwards compatibility, but will be removed in a
later version. Please update your usage of them as soon as
possible.

View File

@ -3087,8 +3087,8 @@ class RecordingAnsibleJob(zuul.executor.server.AnsibleJob):
# Get a merger in order to update the repos involved in this job.
commit = super(RecordingAnsibleJob, self).doMergeChanges(
*args, **kw)
if not commit: # merge conflict
self.recordResult('MERGER_FAILURE')
if not commit:
self.recordResult('MERGE_CONFLICT')
return commit

View File

@ -43,7 +43,7 @@
trigger:
zuul:
- event: project-change-merged
merge-failure:
merge-conflict:
gerrit:
Verified: -1

View File

@ -0,0 +1,82 @@
- pipeline:
name: check
manager: independent
trigger:
gerrit:
- event: patchset-created
success:
gerrit:
Verified: 1
failure:
gerrit:
Verified: -1
- pipeline:
name: post
manager: independent
trigger:
gerrit:
- event: ref-updated
ref: ^(?!refs/).*$
- pipeline:
name: gate
manager: dependent
failure-message: Build failed. For information on how to proceed, see http://wiki.example.org/Test_Failures
merge-conflict-message: The merge failed! For more information...
trigger:
gerrit:
- event: comment-added
approval:
- Approved: 1
success:
gerrit:
Verified: 2
submit: true
failure:
gerrit:
Verified: -2
merge-conflict:
gerrit:
Verified: -1
smtp:
to: you@example.com
start:
gerrit:
Verified: 0
precedence: high
- job:
name: base
parent: null
run: playbooks/base.yaml
- job:
name: project-merge
hold-following-changes: true
run: playbooks/project-merge.yaml
- job:
name: project-test1
run: playbooks/project-test1.yaml
- job:
name: project-test2
run: playbooks/project-test2.yaml
- project:
name: org/project
check:
jobs:
- project-merge
- project-test1:
dependencies: project-merge
- project-test2:
dependencies: project-merge
gate:
jobs:
- project-merge
- project-test1:
dependencies: project-merge
- project-test2:
dependencies: project-merge

View File

@ -2,7 +2,7 @@
name: merge
description: Pipeline for merging the pull request
manager: dependent
merge-failure-message: Merge failed
merge-conflict-message: Merge failed
trigger:
github:
- event: pull_request

View File

@ -98,6 +98,30 @@ class TestMysqlDatabase(DBBaseTestCase):
f"show create table {table}").one()[1]
self.compareMysql(create, sqlalchemy_tables[table])
def test_migration_4647def24b32(self):
with self.connection.engine.begin() as connection:
connection.exec_driver_sql("set foreign_key_checks=0")
for table in connection.exec_driver_sql("show tables"):
table = table[0]
connection.exec_driver_sql(f"drop table {table}")
connection.exec_driver_sql("set foreign_key_checks=1")
self.connection._migrate('c57e9e76b812')
with self.connection.engine.begin() as connection:
connection.exec_driver_sql(
"insert into zuul_buildset (result) values ('SUCCESS')")
connection.exec_driver_sql(
"insert into zuul_buildset (result) values ('MERGER_FAILURE')")
results = [r[0] for r in connection.exec_driver_sql(
"select result from zuul_buildset")]
self.assertEqual(results, ['SUCCESS', 'MERGER_FAILURE'])
self.connection._migrate()
with self.connection.engine.begin() as connection:
results = [r[0] for r in connection.exec_driver_sql(
"select result from zuul_buildset")]
self.assertEqual(results, ['SUCCESS', 'MERGE_CONFLICT'])
def test_buildsets(self):
tenant = 'tenant1',
buildset_uuid = 'deadbeef'

View File

@ -5082,8 +5082,8 @@ For CI problems and help debugging, contact ci@example.org"""
self.assertEqual(0, len(A.messages))
@simple_layout('layouts/merge-failure.yaml')
def test_merge_failure_reporters(self):
@simple_layout('layouts/merge-conflict.yaml')
def test_merge_conflict_reporters(self):
"""Check that the config is set up correctly"""
tenant = self.scheds.first.sched.abide.tenants.get('tenant-one')
@ -5092,34 +5092,77 @@ For CI problems and help debugging, contact ci@example.org"""
"dependencies was unable to be automatically merged with the "
"current state of its repository. Please rebase the change and "
"upload a new patchset.",
tenant.layout.pipelines['check'].merge_failure_message)
tenant.layout.pipelines['check'].merge_conflict_message)
self.assertEqual(
"The merge failed! For more information...",
tenant.layout.pipelines['gate'].merge_failure_message)
tenant.layout.pipelines['gate'].merge_conflict_message)
self.assertEqual(
len(tenant.layout.pipelines['check'].merge_failure_actions), 1)
len(tenant.layout.pipelines['check'].merge_conflict_actions), 1)
self.assertEqual(
len(tenant.layout.pipelines['gate'].merge_failure_actions), 2)
len(tenant.layout.pipelines['gate'].merge_conflict_actions), 2)
self.assertTrue(isinstance(
tenant.layout.pipelines['check'].merge_failure_actions[0],
tenant.layout.pipelines['check'].merge_conflict_actions[0],
gerritreporter.GerritReporter))
self.assertTrue(
(
isinstance(tenant.layout.pipelines['gate'].
merge_failure_actions[0],
merge_conflict_actions[0],
zuul.driver.smtp.smtpreporter.SMTPReporter) and
isinstance(tenant.layout.pipelines['gate'].
merge_failure_actions[1],
merge_conflict_actions[1],
gerritreporter.GerritReporter)
) or (
isinstance(tenant.layout.pipelines['gate'].
merge_failure_actions[0],
merge_conflict_actions[0],
gerritreporter.GerritReporter) and
isinstance(tenant.layout.pipelines['gate'].
merge_failure_actions[1],
merge_conflict_actions[1],
zuul.driver.smtp.smtpreporter.SMTPReporter)
)
)
@simple_layout('layouts/merge-failure.yaml')
def test_merge_failure_reporters(self):
"""Check that the config is set up correctly"""
# TODO: Remove this backwards compat test in v6.0
tenant = self.scheds.first.sched.abide.tenants.get('tenant-one')
self.assertEqual(
"Merge Failed.\n\nThis change or one of its cross-repo "
"dependencies was unable to be automatically merged with the "
"current state of its repository. Please rebase the change and "
"upload a new patchset.",
tenant.layout.pipelines['check'].merge_conflict_message)
self.assertEqual(
"The merge failed! For more information...",
tenant.layout.pipelines['gate'].merge_conflict_message)
self.assertEqual(
len(tenant.layout.pipelines['check'].merge_conflict_actions), 1)
self.assertEqual(
len(tenant.layout.pipelines['gate'].merge_conflict_actions), 2)
self.assertTrue(isinstance(
tenant.layout.pipelines['check'].merge_conflict_actions[0],
gerritreporter.GerritReporter))
self.assertTrue(
(
isinstance(tenant.layout.pipelines['gate'].
merge_conflict_actions[0],
zuul.driver.smtp.smtpreporter.SMTPReporter) and
isinstance(tenant.layout.pipelines['gate'].
merge_conflict_actions[1],
gerritreporter.GerritReporter)
) or (
isinstance(tenant.layout.pipelines['gate'].
merge_conflict_actions[0],
gerritreporter.GerritReporter) and
isinstance(tenant.layout.pipelines['gate'].
merge_conflict_actions[1],
zuul.driver.smtp.smtpreporter.SMTPReporter)
)
)
@ -5128,7 +5171,7 @@ For CI problems and help debugging, contact ci@example.org"""
"""Check that when a change fails to merge the correct message is sent
to the correct reporter"""
self.commitConfigUpdate('common-config',
'layouts/merge-failure.yaml')
'layouts/merge-conflict.yaml')
self.scheds.execute(lambda app: app.sched.reconfigure(app.config))
# Check a test failure isn't reported to SMTP

View File

@ -58,7 +58,7 @@ const RESULT_ICON_CONFIGS = {
color: 'var(--pf-global--info-color--100)',
badgeColor: 'yellow',
},
MERGER_FAILURE: {
MERGE_CONFLICT: {
icon: ExclamationIcon,
color: 'var(--pf-global--warning-color--100)',
badgeColor: 'orange',

View File

@ -41,7 +41,7 @@ const buildResultLegendData = [
symbol: { fill: '#F6D173' },
},
{
name: 'MERGER_FAILURE',
name: 'MERGE_CONFLICT',
// PF orange-200
symbol: { fill: '#EF9234' },
},
@ -81,4 +81,4 @@ const buildsBarStyle = {
}
}
export { buildResultLegendData, buildsBarStyleMap, buildsBarStyle }
export { buildResultLegendData, buildsBarStyleMap, buildsBarStyle }

View File

@ -82,7 +82,7 @@ class BuildsPage extends React.Component {
'POST_FAILURE',
'SKIPPED',
'NODE_FAILURE',
'MERGER_FAILURE',
'MERGE_CONFLICT',
'CONFIG_ERROR',
'TIMED_OUT',
'CANCELED',

View File

@ -70,7 +70,7 @@ class BuildsetsPage extends React.Component {
options: [
'SUCCESS',
'FAILURE',
'MERGER_FAILURE',
'MERGE_CONFLICT',
'DEQUEUED',
]
},

View File

@ -1148,7 +1148,7 @@ class PipelineParser(object):
'start': 'start_actions',
'success': 'success_actions',
'failure': 'failure_actions',
'merge-failure': 'merge_failure_actions',
'merge-conflict': 'merge_conflict_actions',
'no-jobs': 'no_jobs_actions',
'disabled': 'disabled_actions',
'dequeue': 'dequeue_actions',
@ -1198,6 +1198,7 @@ class PipelineParser(object):
'success-message': str,
'failure-message': str,
'start-message': str,
'merge-conflict-message': str,
'merge-failure-message': str,
'no-jobs-message': str,
'footer-message': str,
@ -1220,7 +1221,8 @@ class PipelineParser(object):
pipeline['reject'] = self.getDriverSchema('reject')
pipeline['trigger'] = vs.Required(self.getDriverSchema('trigger'))
for action in ['enqueue', 'start', 'success', 'failure',
'merge-failure', 'no-jobs', 'disabled', 'dequeue']:
'merge-conflict', 'merge-failure', 'no-jobs',
'disabled', 'dequeue']:
pipeline[action] = self.getDriverSchema('reporter')
return vs.Schema(pipeline)
@ -1236,12 +1238,16 @@ class PipelineParser(object):
pipeline.precedence = precedence
pipeline.failure_message = conf.get('failure-message',
"Build failed.")
pipeline.merge_failure_message = conf.get(
# TODO: Remove in Zuul v6.0
backwards_compat_merge_message = conf.get(
'merge-failure-message', "Merge Failed.\n\nThis change or one "
"of its cross-repo dependencies was unable to be "
"automatically merged with the current state of its "
"repository. Please rebase the change and upload a new "
"patchset.")
pipeline.merge_conflict_message = conf.get(
'merge-conflict-message', backwards_compat_merge_message)
pipeline.success_message = conf.get('success-message',
"Build succeeded.")
pipeline.footer_message = conf.get('footer-message', "")
@ -1259,12 +1265,18 @@ class PipelineParser(object):
pipeline.post_review = conf.get(
'post-review', False)
# TODO: Remove in Zuul v6.0
# Make a copy to manipulate for backwards compat.
conf_copy = conf.copy()
if 'merge-failure' in conf_copy and 'merge-conflict' not in conf_copy:
conf_copy['merge-conflict'] = conf_copy['merge-failure']
for conf_key, action in self.reporter_actions.items():
reporter_set = []
allowed_reporters = self.pcontext.tenant.allowed_reporters
if conf.get(conf_key):
if conf_copy.get(conf_key):
for reporter_name, params \
in conf.get(conf_key).items():
in conf_copy.get(conf_key).items():
if allowed_reporters is not None and \
reporter_name not in allowed_reporters:
raise UnknownConnection(reporter_name)
@ -1274,9 +1286,9 @@ class PipelineParser(object):
reporter_set.append(reporter)
setattr(pipeline, action, reporter_set)
# If merge-failure actions aren't explicit, use the failure actions
if not pipeline.merge_failure_actions:
pipeline.merge_failure_actions = pipeline.failure_actions
# If merge-conflict actions aren't explicit, use the failure actions
if not pipeline.merge_conflict_actions:
pipeline.merge_conflict_actions = pipeline.failure_actions
pipeline.disable_at = conf.get(
'disable-after-consecutive-failures', None)

View File

@ -69,7 +69,7 @@ class GitlabReporter(BaseReporter):
if self._merge:
self.mergeMR(item)
if not item.change.is_merged:
msg = self._formatItemReportMergeFailure(item)
msg = self._formatItemReportMergeConflict(item)
self.addMRComment(item, msg)
def addMRComment(self, item, comment=None):

View File

@ -60,7 +60,7 @@ class PagureReporter(BaseReporter):
if self._merge:
self.mergePull(item)
if not item.change.is_merged:
msg = self._formatItemReportMergeFailure(item)
msg = self._formatItemReportMergeConflict(item)
self.addPullComment(item, msg)
def _formatItemReportJobs(self, item):

View File

@ -0,0 +1,42 @@
# Licensed under the Apache License, Version 2.0 (the "License"); you may
# not use this file except in compliance with the License. You may obtain
# a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
# License for the specific language governing permissions and limitations
# under the License.
"""merge_conflict_rename
Revision ID: 4647def24b32
Revises: c57e9e76b812
Create Date: 2022-02-24 14:56:52.597907
"""
# revision identifiers, used by Alembic.
revision = '4647def24b32'
down_revision = 'c57e9e76b812'
branch_labels = None
depends_on = None
from alembic import op
import sqlalchemy as sa
BUILDSET_TABLE = 'zuul_buildset'
def upgrade(table_prefix=''):
buildset = sa.table(table_prefix + BUILDSET_TABLE, sa.column('result'))
op.execute(buildset.update().
where(buildset.c.result ==
op.inline_literal('MERGER_FAILURE')).
values({'result': op.inline_literal('MERGE_CONFLICT')}))
def downgrade():
raise Exception("Downgrades not supported")

View File

@ -284,7 +284,7 @@ class SQLConnection(BaseConnection):
def getSession(self):
return DatabaseSession(self)
def _migrate(self):
def _migrate(self, revision='head'):
"""Perform the alembic migrations for this connection"""
with self.engine.begin() as conn:
context = alembic.migration.MigrationContext.configure(conn)
@ -304,9 +304,9 @@ class SQLConnection(BaseConnection):
if current_rev is None and not self.force_migrations:
self.metadata.create_all(self.engine)
alembic.command.stamp(config, "head", tag=tag)
alembic.command.stamp(config, revision, tag=tag)
else:
alembic.command.upgrade(config, 'head', tag=tag)
alembic.command.upgrade(config, revision, tag=tag)
def onLoad(self, zk_client, component_registry=None):
safe_connection = quote_plus(self.connection_name)

View File

@ -1616,7 +1616,7 @@ class AnsibleJob(object):
self.executor_server.completeBuild(self.build_request, result)
return None
if not ret: # merge conflict
result = dict(result='MERGER_FAILURE')
result = dict(result='MERGE_CONFLICT')
if self.executor_server.statsd:
base_key = "zuul.executor.{hostname}.merger"
self.executor_server.statsd.incr(base_key + ".FAILURE")

View File

@ -1862,14 +1862,14 @@ class PipelineManager(metaclass=ABCMeta):
elif item.getConfigErrors():
log.debug("Invalid config for change %s", item.change)
# TODOv3(jeblair): consider a new reporter action for this
action = 'merge-failure'
actions = self.pipeline.merge_failure_actions
action = 'merge-conflict'
actions = self.pipeline.merge_conflict_actions
item.setReportedResult('CONFIG_ERROR')
elif item.didMergerFail():
log.debug("Merger failure")
action = 'merge-failure'
actions = self.pipeline.merge_failure_actions
item.setReportedResult('MERGER_FAILURE')
log.debug("Merge conflict")
action = 'merge-conflict'
actions = self.pipeline.merge_conflict_actions
item.setReportedResult('MERGE_CONFLICT')
elif item.wasDequeuedNeedingChange():
log.debug("Dequeued needing change")
action = 'failure'

View File

@ -430,7 +430,7 @@ class Pipeline(object):
self.start_mark = None
self.description = None
self.failure_message = None
self.merge_failure_message = None
self.merge_conflict_message = None
self.success_message = None
self.footer_message = None
self.enqueue_message = None
@ -448,7 +448,7 @@ class Pipeline(object):
self.start_actions = []
self.success_actions = []
self.failure_actions = []
self.merge_failure_actions = []
self.merge_conflict_actions = []
self.no_jobs_actions = []
self.disabled_actions = []
self.dequeue_actions = []
@ -473,7 +473,7 @@ class Pipeline(object):
self.start_actions +
self.success_actions +
self.failure_actions +
self.merge_failure_actions +
self.merge_conflict_actions +
self.no_jobs_actions +
self.disabled_actions +
self.dequeue_actions

View File

@ -122,7 +122,7 @@ class BaseReporter(object, metaclass=abc.ABCMeta):
'start': self._formatItemReportStart,
'success': self._formatItemReportSuccess,
'failure': self._formatItemReportFailure,
'merge-failure': self._formatItemReportMergeFailure,
'merge-conflict': self._formatItemReportMergeConflict,
'no-jobs': self._formatItemReportNoJobs,
'disabled': self._formatItemReportDisabled,
'dequeue': self._formatItemReportDequeue,
@ -186,7 +186,7 @@ class BaseReporter(object, metaclass=abc.ABCMeta):
msg = "{}\n\n{}".format(
msg, self._formatItemReportOtherBundleItems(item))
elif item.didMergerFail():
msg = item.pipeline.merge_failure_message
msg = item.pipeline.merge_conflict_message
elif item.getConfigErrors():
msg = str(item.getConfigErrors()[0].error)
else:
@ -195,8 +195,8 @@ class BaseReporter(object, metaclass=abc.ABCMeta):
msg += '\n\n' + self._formatItemReportJobs(item)
return msg
def _formatItemReportMergeFailure(self, item, with_jobs=True):
return item.pipeline.merge_failure_message
def _formatItemReportMergeConflict(self, item, with_jobs=True):
return item.pipeline.merge_conflict_message
def _formatItemReportNoJobs(self, item, with_jobs=True):
status_url = get_default(self.connection.sched.config,

View File

@ -2447,8 +2447,9 @@ class Scheduler(threading.Thread):
if result == "ABORTED":
# Always retry if the executor just went away
build.retry = True
if result == "MERGER_FAILURE":
# The build result MERGER_FAILURE is a bit misleading here
# TODO: Remove merger_failure in v6.0
if result in ["MERGE_CONFLICT", "MERGER_FAILURE"]:
# The build result MERGE_CONFLICT is a bit misleading here
# because when we got here we know that there are no merge
# conflicts. Instead this is most likely caused by some
# infrastructure failure. This can be anything like connection