[JENKINS-68780] Blocked upstream projects in the queue block downstream projects

[Si-So]

See JENKINS-68780.

Proposed changelog entries

• Blocked upstream projects in the queue block downstream projects when the option "Block build when upstream project is building" is enabled for the downstream project.
• Blocked downstream projects in the queue block upstream projects when the option "Block build when downstream project is building" is enabled for the upstream project.

Proposed upgrade guidelines

N/A

Submitter checklist

• (If applicable) Jira issue is well described
• Changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developer, depending on the change) and are in the imperative mood. Examples
  • Fill-in the Proposed changelog entries section only if there are breaking changes or other changes which may require extra steps from users during the upgrade
• Appropriate autotests or explanation to why this change has no tests
• New public classes, fields, and methods are annotated with @Restricted or have @since TODO Javadoc, as appropriate.
• For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@res0nance
@basil

Maintainer checklist

Before the changes are marked as ready-for-merge:

• There are at least 2 approvals for the pull request and no outstanding requests for change
• Conversations in the pull request are over OR it is explicit that a reviewer does not block the change
• Changelog entries in the PR title and/or Proposed changelog entries are accurate, human-readable, and in the imperative mood
• Proper changelog labels are set so that the changelog can be generated automatically
• If the change needs additional upgrade steps from users, upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the PR title. (example)
• If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

[Si-So]

This first commit only includes a test case. This test case is a translation of the ticket's text into code.

I expect only my new test to fail.

[Si-So]

As expected only the new test failed.

I pushed a commit that fixes the failing test, but I am not sure if other tests are negatively affected. We will see...

[Si-So]

I am currently confused by the results of ci.jenkins.io. I am waiting for more runs to complete because right now I am not sure if the problems are caused by my PR or are by a problem of ci.jenkins.io itself.

[basil]

Your code is fine. The system is just unstable. 😢

[Si-So]

Thanks for the feedback :)

I will soon push a new commit that further simplifies the test case, a new test case for the reverse situation (downstream projects block upstream projects), and a fix for the reverse situation.

[Si-So]

@res0nance: Thanks for the approval, but I think it was premature: After adding the two latest commits an old test started to fail and it is quite likely that this due to my changes.

This failure was probably hidden because ci.jenkins.io was unstable.

I will look into the issue.

Changes still fail tests

[Si-So]

The core of the problem is that two contradictory requirements exist. A simplified example:

• projectA triggers projectB and projectC
• projectB triggers projectC
• projectC triggers projectB
• Both projectB and projectC have "Block build when upstream project is building" enabled

(I am not sure, if this example actually triggers the problem or if a more complex example is needed. But due to its simplicity, it makes the core of the problem easier to understand)

The current behavior of jenkins is that the circular dependency of projectB and projectC is detected and the cycle is broken and one of the projects starts to build. This is in contrast to the described behavior of "Block build when upstream project is building". When this behavior was altered in a meaningful way for the last time, the committer also mentions this. I quote:

One could argue that without this change the system is functioning correctly and that previous behaviour was a bug. On the other hand, people have come to rely on the previous behaviour.

See: 4f4a64a

This clashes with my requirement that the options work as described.

Whereas the current behavior reduces the chances of deadlocks occurring I am fine with deadlocks occurring.

I propose the following solution:
Add a "strict mode" sub-option for the option "Block build when upstream project is building". When this sub-option is enabled my desired behavior is toggled on. I like this solution because it is backward compatible. I believe the hardest part of this solution is to explain the difference between non-strict-mode and strict-mode in the UI.

I am a little bit unsure if my proposed solution will be merged since it has effects on the UI.

Will my solution be merged after I have finished it?

[Si-So]

I just pushed a new commit.
When the queue checks, if a project can be built upstream/downstream projects that are blocked themselves by upstream or downstream builds, are ignored to break deadlocks. Ignoring upstream or downstream builds is fine since getBuildingUpstream()/getBuildingDownstream() will eventually find the root cause of the blockage and will thus break the circular dependency by starting the oldest scheduled project of the cycle. This is also how jenkins master currently handles things.

[Si-So]

Ci.Jenkins.io looks good. I just pushed updated help texts.

[Si-So]

Can I support the review process in any way?

[res0nance]

LGTM

[basil]

This PR is now ready for merge. We will merge it after approximately 24 hours if there is no negative feedback. Please see the merge process documentation for more information about the merge process. Thanks!

[daniel-beck]

The labels aren't changed, so I would expect some (many) users to be surprised about the new behavior. Is this change worth it? What is the motivation for this change other than it doesn't work like the documentation says? Do we believe it should behave as documented rather than as implemented? Otherwise, changing the documentation would be the potentially less harmful choice.

[jtnord]

The labels aren't changed, so I would expect some (many) users to be surprised about the new behaviour. Is this change worth it? What is the motivation for this change other than it doesn't work like the documentation says? Do we believe it should behave as documented rather than as implemented? Otherwise, changing the documentation would be the potentially less harmful choice.

Probably needs to be answered, however the code change looks good (pending javadoc tweaks) and if it breaks installations we could revert it in future.

Of note - if I understand this correctly you should be able to reproduce the reported issue without any external plugins blocking builds. If a build is blocked because all nodes with a given label have no spare executors (yes most agents would come from some plugin but the blockage is defined in Jenkins core not the agent/cloud plugin.)

And given that - I think it makes sense that up/downstreams are blocked whilst a up/downstream is waiting for an agent.

[Si-So]

Of note - if I understand this correctly you should be able to reproduce the reported issue without any external plugins blocking builds. If a build is blocked because all nodes with a given label have no spare executors (yes most agents would come from some plugin but the blockage is defined in Jenkins core not the agent/cloud plugin.)

And given that - I think it makes sense that up/downstreams are blocked whilst a up/downstream is waiting for an agent.

I checked the bug tracker and found multiple longstanding critical bugs that very likely occur because of the current behavior of the blocking option. These are the issues:

• JENKINS-22800
• JENKINS-5150
• JENKINS-5125

Because of these issues I am strongly in favor of merging my PR.

[basil]

The motivation is clear to me; thank you for fixing this long-standing issue!

[Si-So]

FYI: I will be on vacation for the next two weeks. I won't respond to any change requests during this time.

[basil]

On July 13, @daniel-beck removed the ready-for-merge label and added the needs-justification label. On July 15, @Si-So provided a justification. As of July 18, @daniel-beck has not responded to the post providing a justification, nor has he removed the needs-justification label.

[daniel-beck]

If feedback becomes obsolete, feel free to update the PR accordingly. This is no different from a stale review that should be dismissed.

[basil]

Thanks Daniel! At this point all design feedback has been addressed, and all implementation feedback has also been addressed, so I think it is appropriate to restart the integration timer.

---

This PR is now ready for merge. We will merge it after approximately 24 hours if there is no negative feedback. Please see the merge process documentation for more information about the merge process. Thanks!