Page MenuHomePhabricator
Create Task
Maniphest T229708

Jenkins 'test' pipeline should cancel old jobs after new patch sets
Open, MediumPublic
Actions

Description

Noticed today that when on https://gerrit.wikimedia.org/r/489443 Patch Set 24 was uploaded a few minutes after Patch Set 23, that the old jobs weren't cancelled.

Afaik Zuul used to properly cancel old jobs. If it no longer that, that would certainly increase load and traffic significantly given that it's fairly common to do a rebase followed by an amendment, and then maybe a commit message inline edit after that (that's three patch set versions, and thus three times the 25-minute whole pipeline being queued up with 5 job executors each).

Screenshot 2019-08-03 at 00.16.40.png (1×1 px, 228 KB)
Screenshot 2019-08-03 at 00.17.34.png (1×990 px, 214 KB)
Running (after 2 minutes)..Still running (after 5 minutes)..

Related Objects

Event Timeline

Krinkle created this task.Aug 2 2019, 11:20 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 2 2019, 11:20 PM
Krinkle updated the task description. (Show Details)Aug 2 2019, 11:21 PM
Krinkle mentioned this in T230019: Stop/Restart tests for zuul.Aug 7 2019, 12:43 PM
Lucas_Werkmeister_WMDE subscribed.Aug 7 2019, 1:49 PM
greg triaged this task as Medium priority.Sep 6 2019, 6:05 AM
greg edited projects, added Release-Engineering-Team (CI & Testing services), Release-Engineering-Team-TODO; removed Release-Engineering-Team.
greg moved this task from Should be empty (use Release-Engineering-Team) to Later / Need volunteer on the Release-Engineering-Team-TODO board.Sep 6 2019, 6:08 AM
Jdforrester-WMF moved this task from Untriaged to Backlog on the Continuous-Integration-Infrastructure board.Jun 25 2020, 5:07 PM
hashar subscribed.Jun 25 2020, 5:08 PM

We would need to reproduce the issue to gather some debug logs then dig into Zuul debug and see what is going on. It should indeed cancel old changes.

thcipriani removed a project: Release-Engineering-Team (CI & Testing services).Apr 20 2021, 1:09 AM
thcipriani edited projects, added Release-Engineering-Team (thcipriani-workboard-fiddling); removed Release-Engineering-Team-TODO.Apr 20 2021, 3:41 AM
thcipriani moved this task from thcipriani-workboard-fiddling to Seen (ARCHIVE) on the Release-Engineering-Team board.Apr 20 2021, 3:46 AM
thcipriani edited projects, added Release-Engineering-Team; removed Release-Engineering-Team (thcipriani-workboard-fiddling).
thcipriani edited projects, added Release-Engineering-Team (Seen); removed Release-Engineering-Team.Apr 20 2021, 3:23 PM
hashar closed this task as Declined.Mar 29 2024, 3:09 PM

Declining since we could not reproduce the issue.

Dreamy_Jazz subscribed.Mar 30 2024, 12:35 PM

I've seen this happening for CheckUser recently.

Dreamy_Jazz added a comment.Apr 15 2024, 6:15 PM

@hashar Example: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/CheckUser/+/1018767

In https://gerrit.wikimedia.org/r/c/mediawiki/extensions/CheckUser/+/1018767/21#message-a407d1165047a0b8dca50a780ee03d416120d499 I upload a new patchset, however, then in https://gerrit.wikimedia.org/r/c/mediawiki/extensions/CheckUser/+/1018767/21#message-069e300a9828834f75205efc736d499897e7f2ff I get the tests failing for a previous patchset around ~15 min after I uploaded the new patchset.

Dreamy_Jazz reopened this task as Open.Apr 15 2024, 6:17 PM

Marking this as open again because I have an example of this.

hashar added a comment.Apr 15 2024, 7:43 PM

@Dreamy_Jazz awesome! Everything ends up easier when there is a very recent case since that means we still have the debug logs from Zuul. So here we go.

Patches for 1018767 got sent in a quick succession:

17:54:42 Patchset 18 (which is a rebase)
17:56:08 Patchset 19 (which is a rebase)
17:56:17 Patchset 20 (which is a rebase)
17:57:14 Patchset 21

On the Zuul side (/var/log/zuul/debug.log):

2024-04-15 17:56:14,597 DEBUG zuul.Scheduler: Processing trigger event <TriggerEvent patchset-created mediawiki/extensions/CheckUser master 1018767,19>
2024-04-15 17:56:14,598 DEBUG zuul.IndependentPipelineManager: Change <Change 0x7f46b4375450 1018767,19> is a new version of <Change 0x7f468613d110 1018767,18>, removing <QueueItem 0x7f46658ea690 for <Change 0x7f468613d110 1018767,18> in test>
2024-04-15 17:56:25,365 DEBUG zuul.Scheduler: Processing trigger event <TriggerEvent patchset-created mediawiki/extensions/CheckUser master 1018767,20>
2024-04-15 17:57:19,984 DEBUG zuul.Scheduler: Processing trigger event <TriggerEvent patchset-created mediawiki/extensions/CheckUser master 1018767,21>
2024-04-15 17:57:19,984 DEBUG zuul.IndependentPipelineManager: Change <Change 0x7f46710ec210 1018767,21> is a new version of <Change 0x7f46b4375450 1018767,19>, removing <QueueItem 0x7f46608f5650 for <Change 0x7f46b4375450 1018767,19> in test>

When patchset 20 got sent, that did not cause patchset 19 to be removed for whatever reason (time based race condition?)?

When patchset 21 is sent, there are 19 and 20 in the queue however the Zuul code is only removing ONE item for that change (since there is supposedly never more than one copy).

def process_event_queue(self):
        self.log.debug("Processing trigger event %s" % event)
...
                if event.type == 'patchset-created':
                    pipeline.manager.removeOldVersionsOfChange(change)
def removeOldVersionsOfChange(self, change):
    if not self.pipeline.dequeue_on_new_patchset:
        return
    old_item = self.findOldVersionOfChangeAlreadyInQueue(change)
    if old_item:
        self.log.debug("Change %s is a new version of %s, removing %s" %
                       (change, old_item.change, old_item))
        self.removeItem(old_item)

That removes one item and move on.

def findOldVersionOfChangeAlreadyInQueue(self, change):
    for item in self.pipeline.getAllItems():
        if not item.live:
            continue
        if change.isUpdateOf(item.change):
            return item
    return None

Iterates through all items in the pipeline and return the first one matching. The isUpdateOf() is:

def isUpdateOf(self, other):
    if ((hasattr(other, 'number') and self.number == other.number) and
        (hasattr(other, 'patchset') and
         self.patchset is not None and
         other.patchset is not None and
         int(self.patchset) > int(other.patchset))):
        return True
    return False

I don't have an explanation as to why patchset 19 was not removed.

Dreamy_Jazz added a comment.Apr 24 2024, 5:01 PM

Another example where the failure for the first patchset occurred after the third had been uploaded for at least 7 or so minutes: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/CheckUser/+/1023890/3

Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL · Credits