Page MenuHomePhabricator

Depends-On should not work across branches
Open, Stalled, Needs TriagePublic

Description

I saw this on the Zuul dashboard just now:

The first change is a wmf branch change, but the second and third changes are in master. I think they are being grouped together because of a Depends-On in their commit messages, but that should not work across different branches (at least not within the same repo). Cherry-picks have the same Change-Id as their originals in master, so I can see how this is confusing.

Event Timeline

Catrope created this task.Nov 29 2018, 12:10 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptNov 29 2018, 12:10 AM

This isn't due to Depends-On, it's because gate-and-submit-* uses the dependent queue (AFAICT), which means changes are always forced into dependencies like this, just like the normal gate-and-submit queue. See T94322: Re-evaluate use of "Dependent Pipeline" in Zuul for gate-and-submit.

hashar added a subscriber: hashar.Nov 30 2018, 3:19 PM

TLDR Zuul can not make a difference since a change-id can refer to multiple changes. That ends up with nasty side effect. The issue is addressed in Zuul v3 since January 2018.

Long explanation:

The gate-and-submit-swat pipeline only accepts CR+2 comments for the wmf branches, roughly:

trigger:
  gerrit:
    - event: comment-added
      branch: wmf/\d\.\d{2}\.\d-wmf\.\d{1,2}
      approval:
        - code-review: 2

The second change Gerrit #474020 is for the master branch and Depend-On I6bd1c95548616677e1f72ba6bcfc6f2b551c1ca6.

The change got a CR+2 at 2018-11-28 20:50:44, the gate-and-submit pipeline processed the change but it got rejected due to dependent patch in wmf branch failing to merge.

2018-11-28 20:50:44,812
INFO zuul.Scheduler: Adding mediawiki/core, <Change 0x7efd949aaa90 474020,8> to <Pipeline gate-and-submit>
DEBUG zuul.DependentPipelineManager:
 Considering adding change <Change 0x7efd949aaa90 474020,8>
 Checking for changes needed by <Change 0x7efd949aaa90 474020,8>:
   Change <Change 0x7efd949aaa90 474020,8> needs change <Change 0x7efd67e1f910 476371,1>:
   Change <Change 0x7efd67e1f910 476371,1> is needed but can not be merged
 Failed to enqueue changes ahead of <Change 0x7efd949aaa90 474020,8>

That is why upon +2 it immediately reported to Gerrit: This change depends on a change that failed to merge..

That dependent change (on wmf branch) got a CR+2 https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/476371/ and it is enqueued in gate-and-submit-swat as intended:

2018-11-29 00:03:58,548 DEBUG zuul.DependentPipelineManager:
Adding change <Change 0x7efd67e1f910 476371,1> to queue <ChangeQueue gate-and-submit-swat: mediawiki>

Since the change got enqueued, Zuul then look for change that might be needing it:

2018-11-29 00:03:59,158 DEBUG zuul.DependentPipelineManager:
 Checking for changes needing <Change 0x7efd67e1f910 476371,1>:
   Change <Change 0x7efd949aaa90 474020,8> needs <Change 0x7efd67e1f910 476371,1> and is ready to merge
   Change <Change 0x7efd65fa5890 475921,4> needs <Change 0x7efd67e1f910 476371,1> and is ready to merge

And Zuul then (wrongfully) enqueue it in the same queue regardless of the pipeline filter:

2018-11-29 00:03:59,159 DEBUG zuul.DependentPipelineManager:
Considering adding change <Change 0x7efd949aaa90 474020,8>
Checking for changes needed by <Change 0x7efd949aaa90 474020,8>:
   Change <Change 0x7efd949aaa90 474020,8> needs change <Change 0x7efd67e1f910 476371,1>:
   Needed change is already ahead in the queue
  Adding change <Change 0x7efd949aaa90 474020,8> to queue <ChangeQueue gate-and-submit-swat: mediawiki>

All of that to say that Zuul v2.5, being given a change id, can not make a difference between branches. The branch filter not being applied when queuing the needing change sounds like a bug though (the swat pipeline should have filtered it out).

Zuul v3 deprecates using the Gerrit change id for Depends-On. It now favors the change URL. The new feature got announced in January 2018 in openstack-dev: New Zuul Depends-On syntax:

James E. Blair wrote:
We recently introduced a new URL-based syntax for Depends-On: footers
in commit messages:
Depends-On: https://review.openstack.org/535851
...

  • The new syntax points to a specific change on a specific branch. This means if you depend on a change to multiple branches, or changes to multiple projects, you need to list each URL. The old syntax looks for all changes with that ID, and depends on all of them. This may mean some changes need multiple Depends-On footers, however, it also means that we can express dependencies is a more fine-grained manner.

An intent is to also support other repositories (namely GitHub).

So I think upstream ran in the same trouble and went with referring the change explicitly which should address the issue. Thus the bug would be addressed if and when we upgrade to Zuul v3. Whether we will upgrade will be discussed soon among Release-Engineering-Team , but we do not know yet when we will have a decision.

hashar changed the task status from Open to Stalled.Nov 30 2018, 3:19 PM

Thank you to have taken the effort to fill the bug. Per above, the issue is solved in Zuul v3 so that is pending upgrade :/

matmarex added a subscriber: matmarex.

This just caused an issue in VisualEditor and MobileFrontend because a core change was backported to REL1_33 without its dependencies in the extensions (marked with 'Depends-On'): T225354 / T225359.

This isn't due to Depends-On, it's because gate-and-submit-* uses the dependent queue (AFAICT), which means changes are always forced into dependencies like this.

Yeah, ignoring Depends-On for a moment, the implicit chaining of changes that happen to be in the CI queue together intentionally only considers connections between jobs and repositories, not branches. This is because it cannot know that a job will only use changes from a certain branch. It can assume that for the repository where the commit originates (that is, a commit to mediawiki-core branch X should only interact with branch X of that repo). But, it cannot assume that for other repositories the job interacts with. For example, when checking out skins, extensions, and vendor, it tries multiple branches (in addition to adhering any Depends-On ref). If a branch by the same name exists, it then Zuul-cloner will use it. But if it doesn't, then it falls back to "master".

This is how feature branches can still be tested, for example. Although if there was a way to turn this off, we probably would because it's so rare that it's probably not worth slowing down CI for. (The tests would still work, it would just mean CI won't delay all other commits always to rule out a false positive if another commit could in theory affect it, e.g. it won't block a commit to branch X on a commit to branch Y, even though the branch X test might clone another repo with branch Y).