Adds a new type of trigger to the Gerrit driver that only triggers if
the approval value was changed by the user in the comment. This is
useful if Zuul is configured to allow many different scores to trigger a
pipeline (with an additional requirement on all of them), but arbitrary
comments made while the scores are present should _not_ trigger (or
potentially re-trigger) the pipeline. This can happen because Gerrit
sends all approvals by a user on all comments, regardless of if they
were changed by the comment.
The new `approval-change` trigger requirement inspects the `oldValue`
field in the Gerrit event. The pipeline will only trigger if this value
is present and not equal to the new approval value (thus, only when the
user actually changed it).
`oldValue` has been present since at least Gerrit 3.4
Change-Id: I88cf840ae8b4e63c77f10ee68b6901e85f7c5fb1
Approximately 40% of all Gerrit events that OpenDev processes are
ref-updated events for refs/changes/.../meta refs. This is likely
due to the increased use of notedb for storing data in Gerrit. Since
Zuul users are not likely to need to trigger off of ref-updates to
the meta ref, let's avoid enqueing them into Zuul's event queue.
This will reduce ZK traffic.
Change-Id: I724f5b20790d1ad32e72b1ce642355c2257026c1
Fixes the way that missing labels are handled in Gerrit. The intention
is that labels provided by Zuul are removed from the set of missing
labels on the change (and thus ignored). The original code was using the
">" set comparison operator to do this, but this operator is actually
"issuperset()". This means that if there was any disjoint members in the
allow_needs set (that is allow_needs had labels that were not missing),
the comparison would be False, and any actual missing labels would be
ignored.
The fix is to use set difference to calculate the missing labels and
remove the allow_needs set. If any labels are left after this, they are
actually missing and the change cannot be merged
Change-Id: Ibdb5df44e80d75198493f8287443ed19bcf269f1
The only way to get reasonable performance out of all three databases
(mariadb, mysql, postgres) appears to be to make the following changes:
* Use group-by instead of distinct.
Some versions of mysql produce the wrong output with distinct;
this is more descriptive of what we want anyway.
* Split the query into two statements instead of joining on a subquery.
Mariadb seems to be unable to produce a good query plan for the
buildset list query. Neither mariadb nor mysql support using
"IN" with a subquery (so the idea of using IN instead of JOIN for
the subquery is out).
These methods now perform a query to get the ids of the builds or
buildsets that match the criteria, then perform a second query to load
the ORM objects that match those ids. This appears to be quite
fast for all three queries with the latest versions of all three database
systems.
Change-Id: I30bb3214807dfa8b26a848f85bb7a7bc660c6c1d
This restores the conditional around adding the mysql index hint
for the builds query. It was thought (incorrectly) that the hint
is now safe in all cases, but it is still the case that it can
slow down the query if some columns appear in the where clause.
Change-Id: Ieafab7e529e305f084ebf8c7844c8fa78da86902
We previously included this mysql index hint under some circumstances
for both build and buildset queries. It initially appeared to no
longer be necessary after recent table changes, however that does not
seem to be the case, and in fact, it is necessary for some simple
build table queries.
It does no longer appear to be necessary for the buildset table, so
it is not added back here.
When applied to MySQL 5.7, these hints result in the build and buildset
queries returning the first N rows rather than the last N rows as
expected.
We should be able to resolve that issue as well, but it will require
further changes. We're going to merge this as a short-term fix while
we work on a better solution. The Zuul community has been notified
that master is currently a work in progress and to temporarily avoid
continuous deployment under mysql/mariadb.
Change-Id: Ia6d962957af3cac87d174145e69571a405ed505b
https://review.opendev.org/c/908420 introduced a bug where literal
filters on builds and buildsets with sql wildcard characters (_, %) get
escaped, altering the search string. That leads to missing filter
results, e.g. when the user searches for a job_name with an underscore
in the name, because zuul escapes the underscore, searching for
`foo$_bar` instead of `foo_bar` (where `$` is the escape char). Fix this
by only applying the sanitation if we're going to filter with the sql
`LIKE` operator.
Change-Id: Iccef71fdf8a5a1150ff957be68358882e16e9da8
The current SQL query will not correctly limit the number of buildsets
when some of the buildsets are related to multiple refs (circular
dependencies). The problem is that LIMTI works on number of rows, but we
want to limit only on the number of buildsets.
This corrects the problem by using a subquery to identify the distinct
buildsets, limiting that, and then querying for all of the information
about those buildsets.
This also updates the methods which perform the same function for builds,
even though we are not yet seeing an issue in practice. It is
theoretically possible to call the getBuilds method with 'provides' and
'limit' arguments, which would produce the same problem as the buildsets
query. That is not possible in practice, as the REST API doesn't support
provides, and the scheduler which does pass the provides argument doesn't
pass limit. However, that could easily change in the future. Additionally,
this future-proofs us if we add more queryable one-to-many relationships
to builds in the future (such as if we linked builds to multiple refs).
Also, it's easier to maintain these methods if they follow the same pattern.
There does not appear to be a performance loss in either mysql or postgres
with local testing on large data sets. There may actually be an improvement
(but it's within the margin of error, so hard to say).
The index hints previously needed for mysql appear to no longer be
necessary and are removed.
Change-Id: Ib19e4cb8171f5d4d2873fb6b9c0301eb5d4ee43d
Co-Authored-By: James E. Blair <jim@acmegating.com>
New versions of docker are no longer compatible with old versions
of skopeo. To correct this, build a new version of skopeo for
the container images. We need 1.14+ which is not available in
debian yet, so we build 1.15 (the latest tagged release) from
source.
Change-Id: I5a5c351e90b06d3acdd02f3117aa29eafb72445e
Importlib is the modern replacement for pkg_resources and is bundled in
python itself. Meanwhile pkg_resources is part of setuptools which is no
longer included in python as of python3.12. Do this transition to be
ready for python3.12 but also to modernize our package introspection.
Change-Id: I9a404e34ae2a833a925dcc156073e0f3f0680a11
This error exception class went unused, likely due to complications
from circular imports.
To resolve this, move all of the configuration error exceptions
into the exceptions.py file so they can be imported in both
model.py and configloader.py.
Change-Id: I19b0f078f4d215a2e14c2c7ed893ab225d1e1084
We have some replacement fields designed to allow users to construct
deep links to the status page, but that is more difficult to do now
that items may have more than one change. Moreover, users shouldn't
need to do it at all since Zuul already knows how to do that.
Make a new field, "item_url" which is the correct status page url,
and encourage users to use that instead. Deprecate status_url,
change, and changes which are now difficult to use correctly.
Change-Id: I40e98ed220a13b8d2edddf510bab133b05845751
The schema validation for these values is correct (if a little
generous) but the docs had some incorrect example values. Update
them to match the schema.
Also, add comments to the schema validation indicating the source
of these somewhat mysterious values.
Change-Id: Idf26848a8c121d311590d07e003b53b8e867affd
The 'requested' action is deprecated in favor of 'rerequested', but the
new schema did not permit the new action name.
Change-Id: I047d2676f44151e7569d38bc1df3d26ffee83202
The schema validation for Github trigger events did not use the label
schema for 'unlabeled' actions leading to bogus config warnings.
Change-Id: I6c888d990047e611b560491be9bc784eb1981ada
Allow to search for builds and buildsets using substrings of job_name,
project, branch, and pipeline. This is done by placing wildcard
characters (*) into the filter string which get translated to SQL
wildcards (%), representing zero, one, or multiple characters.
Translating SQL style wildcards (%) to asterisks is done because
asterisks as wildcard chars might feel more intuitive, cf. shell file
globbing or regexp.
The SQL LIKE operator is only used if a wildcard is present in the
filter string. This is to not rely on the underlying SQL implementation
of optimizing queries with a LIKE op but no wildcard (ie. exact match),
so that we don't introduce unnecessary performance penalties.
Change-Id: I827a27915308f78fc01019bd988b34ea987c90ea
Since jobs are no longer identified by name but by UUID we also need to
reference job dependencies in the MQTT payload by UUID.
For backward-compatibility we'll keep the old "dependencies" field and
add a new "job_dependencies" mapping with the job names and UUIDs.
Change-Id: Ib74b11faf72602e1708ea6364cc4a1000e3f0d3b
The characters '+' and '#' have a special meaning (wildcards) and are
not allowed when publishing messages.
ERROR zuul.MQTTConnection: Could not publish message to topic 'foobar/zuul/c++-test' via mqtt
Traceback (most recent call last):
File "/opt/zuul/lib/python3.11/site-packages/zuul/driver/mqtt/mqttconnection.py", line 97, in publish
self.client.publish(topic, payload=json.dumps(message), qos=qos)
File "/opt/zuul/lib/python3.11/site-packages/paho/mqtt/client.py", line 1233, in publish
raise ValueError('Publish topic cannot contain wildcards.')
ValueError: Publish topic cannot contain wildcards.
Change-Id: Iad2ad551151284910de076cec15b3ac6b1cbda52
The "event" trigger attribute can currently be a list. Technically,
it is possible to construct a trigger with an event list, such as:
trigger:
github:
- event:
- pull_request
- pull_request_review
branch: master
Which would trigger on any pull_request or pull_request_review event
on the master branch. However in practice users typically have much
more narrow event selections, such as only triggering on pull_request
events with the opened action, or a pull_request event with a certain
comment. It is not possible to construct that example with a single
trigger; the following is invalid:
trigger:
github:
- event:
- pull_request
- pull_request_review
actions:
- opened
- commented
branch: master
comment: recheck
That will pass syntax validation but would only fire on a recheck
comment; it would never fire on a PR opened event because that event
won't have a comment.
To help users avoid these problems, or worse, let's limit the event
specifier to a single event (of course users can add more triggers for
other events). That will allow us to inform users when they use
options incompatible with the event they selected.
For now, we make this a deprecation so that in the future we can
enforce it and improve feedback.
This adds syntax validation for each of the possible event/action
combinations in the case where the user has already specified a single
event. This allows us to go ahead and issue warnings if users specify
incompatible options. Later, all of these can become errors.
Some time ago (8.3.0) we deprecated the require-status attribute. It
is eligible for removal now, but that predated the deprecation
warnings system. Since we haven't yet removed it, and we now have
that system, let's add a deprecation warning for it and give users a
little more time to notice that and remove it before it becomes an
error.
When a Github user requests that a check run start again, Github emits
a "check_run" event with a "rerequested" action. In zuul < 5.0.0, we
asked users to configure the check_run trigger with the "requested"
action and we silently translated the "rerequested" from github to the
zuul "requested". In 5.0.0, we reversed that decision in order to
match our policy of passing through data from remote systems as
closely as possible to enable users to match the corresponding
documentation of zuul and the remote system. We deprecated
"requested" and updated the examples in the documentation to say
"rerequested". Unfortunately, we left the canonical documentation of
the value as "requested". To correct this oversight, that
documentation is updated to say "rerequested" and a configuration
deprecation warning is added for uses of "requested".
The "unabel" trigger attribute is undocumented and unused. Deprecate
it from syntax checking here so we can gracefully remove it later.
Some unit tests configs are updated since they passed validation
previously but no longer do, and the actual github pull request
review state constants ('approved', etc) are updated to match
what github sends.
Change-Id: I6bf7753d74ec0c5f19dad508c33762a7803fe805
The following problem was observed:
Change A depends-on change B, which is in turn the tip of a
patch series of several changes.
Drivers warm the change cache on events by querying information
about changes related to those events. But they don't process
depends-on headers, which means most drivers only warm one change,
and while the gerrit driver will follow other types of dependency
links which are unique to it, it stops at depends-on boundaries.
So in the example above, the only change in the cach which was warm
was change A.
The triggering event was enqueued, forwarded, and processed by
two responding pipelines simultaneously on two executors.
Each of them noticed the depends-on link and started querying gerrit
for change B and its dependencies. One of the schedulers was about
1 second ahead of the other in this process.
In the gerrit driver, there is a two phase process for updating
changes. First the change itself is updated in the normal way
common to all drivers, and then gerrit-specific dependency links
are updated. That means the change is added to the change cache
with no dependencies, then mutated to add dependencies later.
The first scheduler added change B to the cache with no dependencies.
The second scheduler saw the update and refreshed its copy of B.
The second scheduler begin updating B, saw that the ltime of its
copy of B was sufficiently new it didn't need to update the cache
and stopped updating.
The second scheduler enqueued changes A and B, but no others in its
pipeline.
The first scheduler finished querying the stack of changes ending at
B, added them to the change cache, and mutated the entry for B in the
cache.
The first scheduler enqueued A, B, and the rest of the stack in its
pipeline.
The second scheduler updated its copy of B to include the new
dependencies.
The second scheduler ran a pipeline processor, noticed that B lacked
dependencies, and dequeued A and B, and reported an error to the user.
The good news is that Zuul noticed the mistake and dequeued the
changes.
To correct this, we will now collect all of the information about a
change and its gerrit-specific dependencies before writing any of
that information to the change cache. This means that in our example
above, the second scheduler would not have aborted its queries.
Eventually, both schedulers would end up with the same information
before enqueing anything.
This process is still not quite optimal, in that we will have multiple
schedulers racing to update the same changes at the same time, but
they are designed to deal with collisions like that, so it should
at least be correct.
A future area of work might be to investigate whether we can optimize
this case further.
Change-Id: I647c2b54a55789e521fca71c8c3814907df65da6
This change completes the circular dependency refactor.
The principal change is that queue items may now include
more than one change simultaneously in the case of circular
dependencies.
In dependent pipelines, the two-phase reporting process is
simplified because it happens during processing of a single
item.
In independent pipelines, non-live items are still used for
linear depnedencies, but multi-change items are used for
circular dependencies.
Previously changes were enqueued recursively and then
bundles were made out of the resulting items. Since we now
need to enqueue entire cycles in one queue item, the
dependency graph generation is performed at the start of
enqueing the first change in a cycle.
Some tests exercise situations where Zuul is processing
events for old patchsets of changes. The new change query
sequence mentioned in the previous paragraph necessitates
more accurate information about out-of-date patchsets than
the previous sequence, therefore the Gerrit driver has been
updated to query and return more data about non-current
patchsets.
This change is not backwards compatible with the existing
ZK schema, and will require Zuul systems delete all pipeline
states during the upgrade. A later change will implement
a helper command for this.
All backwards compatability handling for the last several
model_api versions which were added to prepare for this
upgrade have been removed. In general, all model data
structures involving frozen jobs are now indexed by the
frozen job's uuid and no longer include the job name since
a job name no longer uniquely identifies a job in a buildset
(either the uuid or the (job name, change) tuple must be
used to identify it).
Job deduplication is simplified and now only needs to
consider jobs within the same buildset.
The fake github driver had a bug (fakegithub.py line 694) where
it did not correctly increment the check run counter, so our
tests that verified that we closed out obsolete check runs
when re-enqueing were not valid. This has been corrected, and
in doing so, has necessitated some changes around quiet dequeing
when we re-enqueue a change.
The reporting in several drivers has been updated to support
reporting information about multiple changes in a queue item.
Change-Id: I0b9e4d3f9936b1e66a08142fc36866269dc287f1
Depends-On: https://review.opendev.org/907627
In addition to `merge_status` attribute `blocking_discussions_resolved`
should be checked to know whether it makes sense to attempt merging.
In the project setting it is possible to enable "All discussions must be
resolved" check box what will result in the attribute to be set to false
once there are open discussions. With that (while merge_status is still
can_be_merged) the merge request can not be merged.
Sadly this is another badly documented case.
Change-Id: Iba3c7b424fb8acb3134622776eb1518ffddd5374
This change fixes an unexpected behavior where Zuul triggers
the buildset when a reviewer is added or removed but also
in case of comment thread resolved.
Now the driver only rely on the 'update' action specifying
the 'oldrev' attribute for code change. Actually this is stated
in the doc that this attribute is set in case of code change.
https://docs.gitlab.com/ee/user/project/integrations/webhook_events.html#merge-request-events
Change-Id: Ibee53e3e9ead9a0bbfdc0d60a35dcdd4b0a0dba7
When a branch is renamed in Github the REST API will redirect the
request to the endpoint for the new branch name. So far the the Github
client automatically followed those redirects and we did not check if
the branch name in the response matched our request.
This lead to cases where an old branch was added to the branch cache as
protected even though the branch no longer existed. This is not a
problem for the schedulers, but since there won't be any cached config
in Zookeeper, zuul-web will display a warning about missing config files
for the branch.
Since the REST endpoint also returns the (new) name of the branch we can
validate this against the requested branch name in addition to disabling
redirects.
However, this fix is not enough as the 'cachecontrol' adapter that we
use also caches the HTTP 301 redirects which is a problem when a new
branch with the same name as the renamed branch is created. To fix this
we will use a cache busting header (no-cache) to not return a cached
response in those cases.
Change-Id: I2670f951cac1bf41c6569f5495a60e9de262d4a4
The builds query includes an outer join on the provides table for
the purpose of allowing the scheduler to find previously completed
builds which provide artifacts for items currently in the queue.
Otherwise it is unused (it is not possible to query builds by
provides in the web api).
This join can sometimes produce very slow queries, especially
under postgres with certain data characteristics (usually when there
are too few rows to prompt the query planner to use indexes). This
is similar to the problem that prompted the recent change to the
job runtime queries.
To avoid this, only join the provides table if required for the query.
Change-Id: I83810577c230bcd7365504e01e2cd1d1e642fa0e
We've added some more tables and the query used to delete old
buildsets suffered as a result. The current approach performs
quite a lot of individual queries: one for each table multiplied
by each buildset that we inspect.
A faster approach is to use one query for each table, regardless
of the number of buildsets (note, however, that sqlalchemy will
shard this in order to manage query length; in practice, this means
one query for each table for every group of 500 buildsets -- so
a 1/500 reduction in the number of queries).
In local testing, this runs in 1/3 of the time as the current
code in postgres, and between 1/5 and 2/3 of the time with
mysql (performance appears more variable than postgres).
Change-Id: Ie93318ab12832516640ee9210ec071e18623eac4
When using gerrit topics containing white spaces, zuul fails to find the
changes contained in the topic because the query it builds does not
enclose the topic in quotes. So only the first word of the topic is
considered by the gerrit driver. Fixing this by quoting the topic in the
query.
Change-Id: I99d2890d317fb8424740e25d166d17381f1319c8
Currently, the log_url for retried builds in the MQTT payload always
points to the build result page in Zuul web. As mqtt is meant to be
consumed by machines this breaks e.g. log post processing for those
builds.
To fix this, we do the same as for non-retried builds and provide a
dedicated web_url and log_url [1].
[1]: https://review.opendev.org/c/zuul/zuul/+/703983
Change-Id: I139a80d616d59e262a4f21772d7712fda3b5c03b
The existing build time estimator uses the normal getBuilds query
method. We recently added an optimized query for a new build times
API endpoint. As one would expect, the build time estimator has
limited query needs and should be able to use the new optimized
query as well.
The one thing missing is a search for a specific result (ie, SUCCESS),
so that is added. The unused sort operator is removed.
From what I can tell, switching to sorting by end_time should not
produce a reduction in performance compared to sorting by id.
Change-Id: I1096d466accad5574b6cfa226e68b070f769128f
This endpoint runs an optimized query for returning information
suitable for displaying a graph of build times.
This includes a schema migration to add some indexes to aid
the query.
Change-Id: I56e8422a599c1ee51216f26fcae5a39013066e6b
zuul/driver/gitlab/gitlabconnection.py:
Handle a 404 response when attempting to unapprove an MR which does
not have an existing approval.
Ensure that an exception is raised in otherwise unexpected
situations.
The modified codepath is exercised by
tests.unit.test_gitlab_driver.TestGitlabDriver.test_merge_request_commented
tests/fakegitlab.py:
Make GitlabWebServer.post_mr_approve()/post_mr_unapprove() act more
like real GitLab.
tests/fixtures/layouts/basic-gitlab.yaml:
Add "approval: False" to the pipeline.check.start.gitlab to ensure
that the test suite ends up trying to unapprove not-yet-approved MRs
at the start of a pipeline. This also makes the configuration more
like the reference pipeline in the documentation.
Change-Id: Ia000b55e28c9628cf97682939215430baa78d065
The parser rework for the new seeded PRNG jitter implementation in
change Idd9d3a8cfa791860e46e4fc508566417f5d5a9bf omitted the month
field and shifted the hour and day field by one. Correct this so
that the timer triggers continue to be parsed the same as before
that change.
Change-Id: If7988e28bd1e8fed7e21f2deb6eef9b51c8c2ae3
Joshua Watt