Uploaded image for project: 'Jenkins'
  1. Jenkins
  2. JENKINS-30475

bad design for Polling vs Checkout algorithms

    • Icon: Bug Bug
    • Resolution: Unresolved
    • Icon: Blocker Blocker
    • git-plugin
    • None
    • 2.4.0/1.18.0
      1.609.3

      After long time of debugging i found that current polling algorithm works in the next way:

      Polling:

      1. Get from remote repo branchnames + sha1
      2. compare with existed BuilData from latest build (ONE logic)
      3. If sha1/branch not found -> trigger build (NO RPA attached)

      Checkout:

      1. Build is running and it calls checkout() step in GITScm
      2. checkout() calls getBuildRevision that calls DefaultBuildChooser that calls getAdvancedCandidateRevisions() that calls revs = GitUtils.filterTipBranches(revs); (SECOND logic)
      3. filter tips removes all intermediate branches and they never appears in BuildData

      That ends in situations that Polling needs branches, but checkout didn't build them.
      I fixed this issue by commenting tips filtering https://github.com/KostyaSha/git-plugin/commit/8336202ee5ec8d5a12caa875aeba27b89ac3af58 this allowed build all branches and Polling now satisfied.

      Suggestion:

      1. Put RPA for triggered build as polling result -> allows ensure that BuildData will get what polling wants
      2. Use the same logic for checkout and polling -> more or less allows hope that checkout will pick what polling mean

          [JENKINS-30475] bad design for Polling vs Checkout algorithms

          I'm bitten by this issue too (Jenkins 1.643, Git Client Plugin 1.19.4, Git Plugin 2.4.2). I worked around it by using the "Alternative build chooser" instead.

          The situation is thus:
          Just updated master, then merged into a side branch.
          Polling detects new change 074727053356fd1ee42fce0f4ee4ea7580c12db3 and kicks off a build
          Build eliminates the tip of both branches as the same commit exists on both side
          Build reverts to building an older commit

          In the next poll period, the same thing happens, resulting in the older commit being built ad-infinitum.

          Heres a log snippet from the build with verbosity enabled (formatted for readability):
          5:22:08 Starting with all the branches: [
          Revision 156807c06f43f162018875ac7e8748e098894f05 (origin/master-next),
          Revision e8edab87cb3031b14fd4e18be35649935ac80ef6 (origin/bleeding-edge),
          Revision 074727053356fd1ee42fce0f4ee4ea7580c12db3 (origin/master-ltc, origin/master),
          Revision ae914e5683c1d72f786f82b08b1c4a3e095d1ad3 (origin/master-next-ltc)]

          15:22:08 After branch filtering: [
          Revision 156807c06f43f162018875ac7e8748e098894f05 (origin/master-next),
          Revision e8edab87cb3031b14fd4e18be35649935ac80ef6 (origin/bleeding-edge),
          Revision 074727053356fd1ee42fce0f4ee4ea7580c12db3 (origin/master-ltc, origin/master),
          Revision ae914e5683c1d72f786f82b08b1c4a3e095d1ad3 (origin/master-next-ltc)]

          15:22:08 After non-tip filtering: [
          Revision ae914e5683c1d72f786f82b08b1c4a3e095d1ad3 (origin/master-next-ltc),
          Revision e8edab87cb3031b14fd4e18be35649935ac80ef6 (origin/bleeding-edge)]

          15:22:08 Removing what's already been built:

          { origin/bleeding-edge=Build #7 of Revision e8edab87cb3031b14fd4e18be35649935ac80ef6 (origin/bleeding-edge), origin/master-next-ltc=Build #1 of Revision ae914e5683c1d72f786f82b08b1c4a3e095d1ad3 (origin/master-next-ltc)}

          15:22:08 After filtering out what's already been built: []
          15:22:08 Nothing seems worth building, so falling back to the previously built revision: Revision e8edab87cb3031b14fd4e18be35649935ac80ef6 (origin/bleeding-edge)

          The relevent commit history:
          master
          commit 074727053356fd1ee42fce0f4ee4ea7580c12db3
          Merge: 396ca00 27cea07
          commit 27cea072e46bdcd9dd8bc320a84883b28609eb99
          commit 396ca006e7f9661ebd69852cc6b12f0fa1fbafbc
          Merge: 3f9d9b5 71faa0f
          commit 71faa0f76f48f415125b9f2d663c617b4131c17c
          Merge: b65cbe9 e4aa6fb

          master-ltc
          commit 074727053356fd1ee42fce0f4ee4ea7580c12db3
          Merge: 396ca00 27cea07
          commit 27cea072e46bdcd9dd8bc320a84883b28609eb99
          commit 396ca006e7f9661ebd69852cc6b12f0fa1fbafbc
          Merge: 3f9d9b5 71faa0f
          commit 71faa0f76f48f415125b9f2d663c617b4131c17c
          Merge: b65cbe9 e4aa6fb

          Alastair D'Silva added a comment - I'm bitten by this issue too (Jenkins 1.643, Git Client Plugin 1.19.4, Git Plugin 2.4.2). I worked around it by using the "Alternative build chooser" instead. The situation is thus: Just updated master, then merged into a side branch. Polling detects new change 074727053356fd1ee42fce0f4ee4ea7580c12db3 and kicks off a build Build eliminates the tip of both branches as the same commit exists on both side Build reverts to building an older commit In the next poll period, the same thing happens, resulting in the older commit being built ad-infinitum. Heres a log snippet from the build with verbosity enabled (formatted for readability): 5:22:08 Starting with all the branches: [ Revision 156807c06f43f162018875ac7e8748e098894f05 (origin/master-next), Revision e8edab87cb3031b14fd4e18be35649935ac80ef6 (origin/bleeding-edge), Revision 074727053356fd1ee42fce0f4ee4ea7580c12db3 (origin/master-ltc, origin/master), Revision ae914e5683c1d72f786f82b08b1c4a3e095d1ad3 (origin/master-next-ltc)] 15:22:08 After branch filtering: [ Revision 156807c06f43f162018875ac7e8748e098894f05 (origin/master-next), Revision e8edab87cb3031b14fd4e18be35649935ac80ef6 (origin/bleeding-edge), Revision 074727053356fd1ee42fce0f4ee4ea7580c12db3 (origin/master-ltc, origin/master), Revision ae914e5683c1d72f786f82b08b1c4a3e095d1ad3 (origin/master-next-ltc)] 15:22:08 After non-tip filtering: [ Revision ae914e5683c1d72f786f82b08b1c4a3e095d1ad3 (origin/master-next-ltc), Revision e8edab87cb3031b14fd4e18be35649935ac80ef6 (origin/bleeding-edge)] 15:22:08 Removing what's already been built: { origin/bleeding-edge=Build #7 of Revision e8edab87cb3031b14fd4e18be35649935ac80ef6 (origin/bleeding-edge), origin/master-next-ltc=Build #1 of Revision ae914e5683c1d72f786f82b08b1c4a3e095d1ad3 (origin/master-next-ltc)} 15:22:08 After filtering out what's already been built: [] 15:22:08 Nothing seems worth building, so falling back to the previously built revision: Revision e8edab87cb3031b14fd4e18be35649935ac80ef6 (origin/bleeding-edge) The relevent commit history: master commit 074727053356fd1ee42fce0f4ee4ea7580c12db3 Merge: 396ca00 27cea07 commit 27cea072e46bdcd9dd8bc320a84883b28609eb99 commit 396ca006e7f9661ebd69852cc6b12f0fa1fbafbc Merge: 3f9d9b5 71faa0f commit 71faa0f76f48f415125b9f2d663c617b4131c17c Merge: b65cbe9 e4aa6fb master-ltc commit 074727053356fd1ee42fce0f4ee4ea7580c12db3 Merge: 396ca00 27cea07 commit 27cea072e46bdcd9dd8bc320a84883b28609eb99 commit 396ca006e7f9661ebd69852cc6b12f0fa1fbafbc Merge: 3f9d9b5 71faa0f commit 71faa0f76f48f415125b9f2d663c617b4131c17c Merge: b65cbe9 e4aa6fb

          Claudio B added a comment - - edited

          This issue (i.e. differences in the algorithms used to choose the revisions to build) also affects me.

          In my case it is the filtering of already built revisions that's a blocker for me.

          I have a parameterized build, and I want to build the same revision with different parameters, but the "remove any revisions that have already been built" step exactly removes the specific revision because it had been build in the past already (with different build parameters).

          I'm worked around by disabling the relevant code section in the DefaultBuildChooser class.

          Claudio B added a comment - - edited This issue (i.e. differences in the algorithms used to choose the revisions to build) also affects me. In my case it is the filtering of already built revisions that's a blocker for me. I have a parameterized build, and I want to build the same revision with different parameters, but the "remove any revisions that have already been built" step exactly removes the specific revision because it had been build in the past already (with different build parameters). I'm worked around by disabling the relevant code section in the DefaultBuildChooser class.

          Henry Tung added a comment - - edited

          I think I'm hitting the same bug here. I'm using the Git plugin to build tags for publish. However, there's a race condition where if the new tag is pushed but so is a branch that merges that tag, then the tag is never built (because it is "no longer tip", as expressed in that code). "No longer tip" seems to me something fully expected, and should not be a disqualifier for building purposes. As far as I can tell, I can't trace a proper source for this behavior besides https://github.com/jenkinsci/git-plugin/commit/850e3f1b9db2286b56c9426d64f0ed8eeed941d7#diff-2096d6746f7caa46f4e07f46ee4def60R92, which ultimately introduced this behavior. (Unfortunately, I'm guessing code reviews/discussions around those decisions are probably lost to the aether.)

          Generally speaking, I would lean heavily towards an algorithm/model that hits a sweet spot of the following things:

          1) Simple (easy to understand + predict, well-scoped)
          2) Discoverable (easy to guess algorithm based on behavior - linked with (1))
          3) Powerful (handles common use-cases, and does not preclude uncommon ones)

          Regarding tip-filtering in the current algorithm re: concerns above:

          1) Fairly simple, once one reads the code and understands how the filtering/choosing works.
          2) Not very good right now. Branch specifiers imply a virtual forking of N versions of the job, one monitoring each branch. That illusion is "almost" true, but can be broken in non-deterministic ways (depends on scheduling of merges and pushes vs. Jenkins build order decisions). Also, the documentation/help around the "Branch selector" prompts doesn't really explain anything about this algorithm.
          3) Currently blocks out publishing of "older", but important commits as mentioned previously. Filtering can be useful, but seems to me more like an optimization for certain use cases that blocks functionality for other use cases.

          Would it make sense to introduce a boolean flag that enables or disables this filtering? Seems to make sense to include this in the branch selector section, and reimagine it as a "Commit selection and filtering" configuration section.

          Note: As a pre-emptive comment, I can see an alternate approach that excludes tag commits from tip filtering. While that would solve my case without extra configuration, I think it would make the algorithm more opaque/complex from a user standpoint, and would be a patch/anti-fix that overfits to my use case instead.

          Henry Tung added a comment - - edited I think I'm hitting the same bug here. I'm using the Git plugin to build tags for publish. However, there's a race condition where if the new tag is pushed but so is a branch that merges that tag, then the tag is never built (because it is "no longer tip", as expressed in that code). "No longer tip" seems to me something fully expected, and should not be a disqualifier for building purposes. As far as I can tell, I can't trace a proper source for this behavior besides https://github.com/jenkinsci/git-plugin/commit/850e3f1b9db2286b56c9426d64f0ed8eeed941d7#diff-2096d6746f7caa46f4e07f46ee4def60R92 , which ultimately introduced this behavior. (Unfortunately, I'm guessing code reviews/discussions around those decisions are probably lost to the aether.) Generally speaking, I would lean heavily towards an algorithm/model that hits a sweet spot of the following things: 1) Simple (easy to understand + predict, well-scoped) 2) Discoverable (easy to guess algorithm based on behavior - linked with (1)) 3) Powerful (handles common use-cases, and does not preclude uncommon ones) Regarding tip-filtering in the current algorithm re: concerns above: 1) Fairly simple, once one reads the code and understands how the filtering/choosing works. 2) Not very good right now. Branch specifiers imply a virtual forking of N versions of the job, one monitoring each branch. That illusion is "almost" true, but can be broken in non-deterministic ways (depends on scheduling of merges and pushes vs. Jenkins build order decisions). Also, the documentation/help around the "Branch selector" prompts doesn't really explain anything about this algorithm. 3) Currently blocks out publishing of "older", but important commits as mentioned previously. Filtering can be useful, but seems to me more like an optimization for certain use cases that blocks functionality for other use cases. Would it make sense to introduce a boolean flag that enables or disables this filtering? Seems to make sense to include this in the branch selector section, and reimagine it as a "Commit selection and filtering" configuration section. Note: As a pre-emptive comment, I can see an alternate approach that excludes tag commits from tip filtering. While that would solve my case without extra configuration, I think it would make the algorithm more opaque/complex from a user standpoint, and would be a patch/anti-fix that overfits to my use case instead.

          I simply wrote github branch trigger https://github.com/KostyaSha/github-integration-plugin/ and stopped using this stupid algorithms. I think nobody using git directly and there are enough triggers for git repositories managers (gitlab, github, etc).

          Kanstantsin Shautsou added a comment - I simply wrote github branch trigger https://github.com/KostyaSha/github-integration-plugin/ and stopped using this stupid algorithms. I think nobody using git directly and there are enough triggers for git repositories managers (gitlab, github, etc).

          Henry Tung added a comment -

          I'm using it

          feels lonely

          Henry Tung added a comment - I'm using it feels lonely

          Mark Waite added a comment -

          henryptung I'm unwilling to change the default behavior of the plugin, independent of whether the default behavior is right or wrong. There are too many users of the plugin with too many dependencies on the precise behavior of the plugin.

          Your suggestion to use a boolean which enables or disables filtering seems like a good suggestion to me. It would need to default to the current behavior, and would need deep testing to confirm that the new behavior meets the conditions you're describing. If you'd like to submit a pull request based on that idea, you're welcome to do so. You might consider the input from other bug reports which want to build older commits as well.

          Mark Waite added a comment - henryptung I'm unwilling to change the default behavior of the plugin, independent of whether the default behavior is right or wrong. There are too many users of the plugin with too many dependencies on the precise behavior of the plugin. Your suggestion to use a boolean which enables or disables filtering seems like a good suggestion to me. It would need to default to the current behavior, and would need deep testing to confirm that the new behavior meets the conditions you're describing. If you'd like to submit a pull request based on that idea, you're welcome to do so. You might consider the input from other bug reports which want to build older commits as well.

          Kanstantsin Shautsou added a comment - - edited

          markewaite it seems that you never had infinite builds. This default behaviour is bug blocking for usage. Tip filtering is mistake and bug and keeping it is blocking bug because everybody affected with it. Too many users has this infinite build bomb in their instances. You can sefely remove this horrible optimization or make reusable SAME algorithms for polling and checkout to exclude logic loop.
          That the same bug as recursion in java calls.

          Kanstantsin Shautsou added a comment - - edited markewaite it seems that you never had infinite builds. This default behaviour is bug blocking for usage. Tip filtering is mistake and bug and keeping it is blocking bug because everybody affected with it. Too many users has this infinite build bomb in their instances. You can sefely remove this horrible optimization or make reusable SAME algorithms for polling and checkout to exclude logic loop. That the same bug as recursion in java calls.

            Unassigned Unassigned
            integer Kanstantsin Shautsou
            Votes:
            2 Vote for this issue
            Watchers:
            9 Start watching this issue

              Created:
              Updated: