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
This adds support for the hashtags-changed trigger event as well
as using hashtags as pipeline and trigger requirements.
Change-Id: I1f6628d7c227d12355f651c3c822b06e2d5c5562
We receive some events from Gerrit (such as the currently-unhandled
hashtags-changed or currently-partially-handled topic-changed) events
without a patchset, only a change number. This makes sense from a
data model perspective, but is not useful in Zuul since we must have
a patchset to actually enqueue something.
Currently we will handle these events by querying for the change and
storing the results in the change cache under the key (change, None).
Note that the change itself will have the current patchset assigned
as a result of the query. But this "upgrade" of information applies
only to the change itself, not the change key, so this up-to-date
change object will never be used for anything productive in Zuul.
In order to actually trigger off of these events, let's "upgrade" the
change key as well after performing the query. To do that, we will
use a new change key with patchset information when storing the change
object in the cache if our initial change key lacked patchset info but
the resulting change has it. But only in the specific case where we
are performing our first query after receiving an event. We will also
update the event with the same patchset information. This should
mean that after receiving an event and performing the initial query,
we should be guaranteed to have patchset information about the change
and therefore Zuul should never see a (change, None) tuple for a
change key any more.
* (Change keys have more information than that tuple, but those are
the relevant parts for this change.)
Change-Id: I6f077376044ffbbd3853e2050c507f449da77962
This refactors the the config error handling based on nested
context managers that add increasing amounts of information
about error locations. In other words, when we start processing
a Job object, we will:
with ParseContext.errorContext(info about job):
do some stuff
with ParseContext.errorContext(info about job attr):
do some stuff regarding the job attribute
with ParseContext.errorContext(next job attr):
do stuff with a different attr
We store a stack of error contexts on the parse context, and at any
point we can access the accumulator for the most recent one with
ParseContext.accumulator in order to add a warning or error. If we
have an attribute line number, we'll use it, otherwise we'll just
use the object-level information.
We also collapse the exception hnadlers into a single context
manager which catches exceptions and adds them to the accumulator.
This lets us decide when to catch an exception and skip to the next
phase of processing separately from where we narrow our focus to
a new object or attribute. These two actions often happen together,
but not always.
This should serve to simplify the configloader code and make it
easier to have consistent error handling within.
Change-Id: I180f9b271acd4b62039003fa8b38900f9863bad8
This adds a configuration warning (viewable in the web UI) for any
regular expressions found in in-repo configuration that can not
be compiled and executed with RE2.
Change-Id: I092b47e9b43e9548cafdcb65d5d21712fc6cc3af
The re2 library does not support negative lookahead expressions.
Expressions such as "(?!stable/)", "(?!master)", and "(?!refs/)" are
very useful branch specifiers with likely many instances in the wild.
We need to provide a migration path for these.
This updates the configuration options which currently accepts Python
regular expressions to additionally accept a nested dictionary which
allows specifying that the regex should be negated. In the future,
other options (global, multiline, etc) could be added.
A very few options are currently already compiled with re2. These are
left alone for now, but once the transition to re2 is complete, they
can be upgraded to use this syntax as well.
Change-Id: I509c9821993e1886cef1708ddee6d62d1a160bb0
This extends the previous change to include project default branch
support for the Gerrit driver as well as GitHub.
Change-Id: I2b1f6feed72277f5e61a2789d8af5276ee4c7b05
This updates the Gerrit driver to match the pattern in the GitHub
driver where instead of specifying individual trigger
requirements such as "require-approvals", instead a complete ref
filter (a la "requirements") can be embedded in the trigger
filter.
The "require-approvals" and "reject-approvals" attributes are
deprecated in favor of the new approach.
Additionally, all require filters in Gerrit are now available as
reject filters.
And finally, the Gerrit filters are updated to return
FalseWithReason so that log messages are more useful, and the
Github filters are updated to improve the language, avoid
apostraphes for ease of grepping, and match the new Gerrit
filters.
Change-Id: Ia9c749f1c8e318fe01e84e52831a9d0d2c10b203
With newer versions of Gerrit, we are increasingly likely to encounter
systems where the traditional label requirements are minimized in favor
of the new submit requirements rules. If Gerrit is configured to use
a submit requirement instead of a traditional label blocking rule, that
is typically done by switching the label function to "NoBlock", which,
like the "NoOp" function, will still cause the label to appear in the
"submit_record" field, but with a value of "MAY" instead of "OK", "NEED",
or "REJECT".
Instead, the interesting information will be in the "submit_requirements"
field. In this field we can see the individual submit requirement rules
and whether they are satisfied or not.
Since submit requirements do not have a 1:1 mapping with labels, determining
whether an "UNSATISFIED" submit requirement should be ignored (because it
pertains to a label that Zuul will alter, like "Verified") is not as
straightforward is it is for submit records. To be conservative, this
change looks for any of the "allow needs" labels (typically "Verified") in
each unsatisfied submit record and if it finds one, it ignores that record.
With this change in place, we can avoid enqueing changes which we are certain
can not be merged into gate pipelines, and will continue to enqueue changes
about which we are uncertain.
Change-Id: I667181565684d97c1d036e2db6193dc606c76c57
Sometimes Gerrit events may arrive in batches (for example, an
automated process modifies several related changes nearly
simultaneously). Because of our inbuilt delay (10 seconds by
default), it's possible that in these cases, many or all of the
updates represented by these events will have settled on the Gerrit
server before we even start processing the first event. In these
cases, we don't need to query the same changes multiple times.
Take for example a stack of 10 changes. Someone approves all 10
simultaneously. That would produce (at least) 10 events for Zuul
to process. Each event would cause Zuul to query all 10 changes in
the series (since they are related). That's 100 change queries
(and each change query requires 2 or 3 HTTP queries).
But if we know that all the event arrived before our first set of
change queries, we can reduce that set of 100 queries to 10 by
suppressing any queries after the first.
This change generates a logical timestamp (ltime) immediately
before querying Gerrit for a change, and stores that ltime in the
change cache. Whenever an event arrives for processing with an
ltime later than the query ltime, we assume the change is already
up to date with that event and do not perform another query.
Change-Id: Ib1b9245cc84ab3f5a0624697f4e3fc73bc8e03bd
This adds a pipeline queue setting to emulate the Gerrit behavior
of submitWholeTopic without needing to enable it site-wide in Gerrit.
Change-Id: Icb33a1e87d15229e6fb3aa1e4b1ad14a60623a29
The drivers are expected to populate the change cache before
passing trigger events to the scheduler so that all the difficult
work is done outside the main loop. Further, the cache cleanup
is designed to accomodate this so that events in-flight don't have
their change cache entries removed early.
However, at several points since moving the change cache into ZK,
programming errors have caused us to encounter enqueued changes
without entries in the cache. This usually causes Zuul to abort
pipeline processing and is unrecoverable.
We should continue to address all incidences of those since they
represent Zuul not working as designed. However, it would be nice
if Zuul was able to recover from this.
To that end, this change allows missing changes to be added to the
change cache.
That is primarily accomplished by adjusting the Source.getChange
method to accept a ChangeKey instead of an Event. Events are only
available when the triggering event happens, whereas a ChangeKey
is available when loading the pipeline state.
A ChangeKey represents the minimal distinguishing characteristics
of a change, and so can be used in all cases. Some drivers obtain
extra information from events, so we still pass it into the getChange
method if available, but it's entirely optional -- we should still
get a workable Change object whether or not it's supplied.
Ref (and derived: Branch, Tag) objects currently only store their
newrev attribute in the ChangeKey, however we need to be able to
create Ref objects with an oldrev as well. Since the old and new
revs of a Ref are not inherent to the ref but rather the generating
event, we can't get that from the source system. So we need to
extend the ChangeKey object to include that. Adding an extra
attribute is troublesome since the ChangeKey is not a ZKObject and
therefore doesn't have access to the model api version. However,
it's not too much of a stretch to say that the "revision" field
(which like all ChangeKey fileds is driver-dependent) should include
the old and new revs. Therefore, in these cases the field is
upgraded in a backwards compatible way to include old and newrev
in the standard "old..new" git encoding format. We also need to
support "None" since that is a valid value in Zuul.
So that we can continue to identify cache errors, any time we encounter
a change key that is not in the cache and we also don't have an
event object, we log an error.
Almost all of this commit is the refactor to accept change keys
instead of events in getChange. The functional change to populate
the cache if it's missing basically consists of just removing
getChangeByKey and replacing it with getChange. A test which deletes
the cache midway through is added.
Change-Id: I4252bea6430cd434dbfaacd583db584cc796dfaa
It is possible to push no file commits to Gerrit. We had previous code
looking for this case by checking for the special '/COMMIT_MSG' file
entry as the only entry in change.files. Unfortunately, the gerrit http
connection queries were returning a dict of files in the form of
{'name': {attrs}' and this broke the special checking for the single
commit message entry.
We fix this by coercing the type of the returned files data from a dict
to a list of file names.
Change-Id: Ie475911b1c9668bd831c4d31213a2fd3a8c049a7
The Gerrit change cache is no longer nested (previously by change number
and patchset) and will use the flat cache structure provided by the
Zookeeper change cache.
Change-Id: I3fa7e8f2b865765d8f6c8fee35c1ad16b70eb45b
If we're performing SSH change queries with gerrit, we can't
get the full file list of merge commits, but we can ask the mergers
to do it for us.
Change-Id: Ie4d2daead41fcf8143cbd8c028e4edf38fd8f50a
The /changes/x/revisions/y/files endpoint in the Gerrit REST API
can be used to obtain a list of files modified not only by the
commit itself but also files which have changed compared to the
target branch if the commit is a merge commit. No other methods
of file listing (SSH or the plain /changes endpoint) provide this
information. This means that changes to zuul.yaml files that
arrive via merge commits which themselves don't alter the file
are invisible to Zuul.
The Gerrit project's workflow of merging up changes from stable
branches to master is particularly suceptible to this problem.
To correct this, query the files endpoint explicitly. The test
framework is updated to support creating merge commits and
simulating the Gerrit query results as well. Note that these
methods are not completely fleshed out -- they are only sufficient
to support this simple case.
Change-Id: I6eb91abd46f0d68ef1e5c2f500ff16e8d094693e
Currently if two triggers of the same connection type need to trigger on
different events it's not possible to do so since the events are never
filtered on which connection they came from.
For example with the following setup where gerrit-org-1 only wants to
trigger on changes to 'master' and gerrit-org-2 only wants to trigger on
changes to 'develop' they will instead both trigger on 'master' and
'develop'since the events are never filtered on which connection they
came from.
- pipeline:
name: check
trigger:
gerrit-org-1:
- event: patchset-created
branch: 'master'
gerrit-org-2:
- event: patchset-created
branch: 'develop'
Change-Id: Ia0476d71dee59c8b80db7630ac7a524bce87e6f9
Since trigger events will be stored in Zookeeper we need a way to
(de-)serialze them from/to dictionaries.
Change-Id: I1698e22b61947761ddeb10264b84ec157609495b
Newer versions of Gerrit have a built-in work-in-progress flag
which, when set, makes the change unsubmittable. Update the
canMerge method in the Gerrit driver to recognize the new flag.
Change-Id: Ia8e5e50086933215c45e92b419a04c1117880a34
The 'status' field is a driver specific field and should therefore be
defined and initialized in the driver specific subclass of a change and
not in the 'Change' base class.
Change-Id: I0a20041ae8b9f5e4c359fea86c21d1c5c5cbfaca
Consider a "pending-check" event to be a type of patchset-created
event, so that the scheduler will perform its check to remove
obsolete patchsets from pipelines.
Change-Id: Ie808deaa8615d85ebda232e095c12b90c3617f6b
Prior to this change we looked for the current change/PR's url in any
other change/PR's message body. This meant any cross referencing of urls
would create further lookups to determine if there was a real dependency
there. Restrict this a bit more to require the Depends-On string too
when searching to limit the number of spidering queries that must be
done.
This is particularly useful for the github driver because queries are
expensive there and may be rate limited.
Change-Id: Ie49fe1a72dc844b14003d942684fd3d2a9478d21
When getting additional change data from the Gerrit HTTP api we weren't
setting the scheme on the resulting change.url value. This resulted in
urls without a scheme which browsers would treat as relative path
components. Fix this by setting a scheme based on the connection
baseurl.
Change-Id: I9e907c79fa531d15c80e593ca7f7cebc5c516165
This corrects the following errors.
Test fixture errors:
* Unlike the SSH API, the Gerrit REST API does not include
COMMIT_MSG in the list of files.
* We weren't checking all elements of the (change id, branch,
project) tuple when searching for change ids
* I observed a race around poll synchronization; I can't figure it out
so I've added more debug log lines.
Production errors:
* Break after the first successful submit rather than attemping the
call multiple times in all cases. This was non-fatal.
* Only attempt to get the remote version if using HTTP. This was
non-fatal.
* URLquote search queries (they may contain '#'). The additional CRD
tests cover this now.
* Coerce the list of files to a list rather than a dict_keys instance.
The new single file test covers this.
Change-Id: I89a68b2d189b82f0bc64e78fbc875d5628cc2cf0
This adds support for performing queries and git clones over HTTP
(reporting over HTTP was already supported). This will happen
automatically for any connection with a password configured.
Otherwise, the SSH connection will be used as before.
Change-Id: I11920a13615de103eb3d8fb305eacbbcb30e5e40
This adds initial support for the Gerrit checks plugin.
Development of that plugin is still in progress, and hopefully it
(and our support for it) will change over time. Because we expect
to change how we interact with it in the near future, this is
documented as experimental support for now. A release note is
intentionally omitted -- that's more appropriate when we remove
the 'experimental' label.
Change-Id: Ida0cdef682ca2ce117617eacfb67f371426a3131
This change fixes a behavior that might be surprising to a
user: If a reject clause is configured for a pipeline, the reject clause
is always true even if no approval is set. If, however, an unrelated
approval is added to the change, the reject clause is evaluated as
expected.
The root cause was a check that assumed that the absence of approvals
will lead to a negative filter result. However, this is only true for
requirements.
Change-Id: I220bbed931ced7e807febefbf2b1b33af53a8da0
In case the layout includes some regex approval filters (like
username) the dynamic layout creation fails during deepcopy of the
pipelines [1]. This is caused by Github/GerritApprovalFilter overwriting
stuff in the original data structure. Fix this by using the already
existing deepcopied data structures.
To test this an unused pipeline with approval filters for Github and
Gerrit is added to the dynamic layout test cases. This triggers the
deepcopy error without the fix in every test_dynamic_* test case.
[1] Traceback (most recent call last):
File "/usr/lib/python3.6/site-packages/zuul/manager/__init__.py", line 453, in _loadDynamicLayout
include_config_projects=False)
File "/usr/lib/python3.6/site-packages/zuul/configloader.py", line 1478, in createDynamicLayout
config = tenant.config_projects_config.copy()
File "/usr/lib/python3.6/site-packages/zuul/model.py", line 2150, in copy
r.pipelines = copy.deepcopy(self.pipelines)
File "/usr/lib/python3.6/copy.py", line 150, in deepcopy
y = copier(x, memo)
...
File "/usr/lib/python3.6/copy.py", line 240, in _deepcopy_dict
y[deepcopy(key, memo)] = deepcopy(value, memo)
File "/usr/lib/python3.6/copy.py", line 161, in deepcopy
y = copier(memo)
TypeError: cannot deepcopy this pattern object
Change-Id: I4f7f74787aa91e938d0e2f07bd15b4e21d49cb88
After upgrading Gerrit to 2.13 our gate stopped working. The reason
for this is that after a successful gate run zuul does something like
'gerrit review --label verified=2 --submit'. The verified label in
Gerrit by default is configured as 'Verified'. The newer version of
gerrit behaves different now. It accepts the +2 vote on verified but
doesn't submit the patch anymore if the casing is not correct. This
forces us to specify the label in the same casing as gerrit
expects. In that case the tolower() in canMerge prevents the patch
from entering the gate.
In order to avoid confusion and be consistent, avoid any case
conversions and use the labels exactly as defined in Gerrit.
Note that this patch requires changes to the pipelines such that the
labels are spelled exactly as defined in Gerrit.
Change-Id: I9713a075e07b268e4f2620c0862c128158283c7c
Push and ref-updated events do not have an associated change or
pull-request. Thus, requirements that are specific to changes or
pull-requests cannot possibly be met.
To make this work, Ref filters should only be applied to changes that
come from a matching connection. This was not previously enforced when
filters became connection specific.
Change-Id: I5d67bef264db0ad5ba3a7180ce74e3670ba822ce
Story: 2000774
Task: 4624
As we expand the Github driver, we're seeing a need to specify driver-specific
pipeline requirements. To accomplish this, bump the require/reject pipeline
keywords down a level underneath connection names. This lets users specify
per-source pipeline requirements.
This adds new API methods for sources to create the new pipeline filters
(by returning instances or subclasses of RefFilter, which used to be called
ChangeishFilter).
This change also creates and/or moves driver-specific subclasses of EventFilter
and TriggerEvent in(to) their respective drivers.
Change-Id: Ia56c254e3aa591a688103db5b04b3dddae7b2da4
James E. Blair