Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add workaround to prevent simultaneous builds from starting on the wrong node (#30) #54

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from
Draft

Add workaround to prevent simultaneous builds from starting on the wrong node (#30) #54

wants to merge 8 commits into from

Conversation

voruti
Copy link

@voruti voruti commented Jun 22, 2024 •
edited
Loading

see Discussion at #30 (comment)
closes #30

Testing done

With this workaround, all builds are scheduled on the correct node, but there also is an additional/artificial delay added to the later builds:

Jun 22, 2024 11:48:00 AM INFO jp.ikedam.jenkins.plugins.scoringloadbalancer.ScoringLoadBalancer reportScores
Scoring for hudson.model.FreeStyleProject@559cbb7c[TestJob1]:
          jenkinsagent: 1000
                      : -500

Jun 22, 2024 11:48:03 AM INFO jp.ikedam.jenkins.plugins.scoringloadbalancer.ScoringLoadBalancer reportScores
Scoring for hudson.model.FreeStyleProject@60ed73e0[TestJob2]:
          jenkinsagent: 1000
                      : -500

Submitter checklist

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

viceice
viceice reviewed Jun 23, 2024
View reviewed changes
Copy link
Member

@viceice viceice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good so far. I think a second delay is acceptable. we should make it configurable.

this also needs some tests, I don't know how to fake time inside Java tests.

I'll ask on Jenkins mailing list later if there's a better way

@viceice
Copy link
Member

viceice commented Jul 8, 2024

Can you please fix the conflict?

@voruti voruti force-pushed the feature/upstream-issue-30-workaround-missing-executors branch 2 times, most recently from d921efe to 72726d3 Compare July 13, 2024 10:37
@voruti voruti force-pushed the feature/upstream-issue-30-workaround-missing-executors branch from 72726d3 to 7e8c9ce Compare July 13, 2024 12:05
@voruti
Copy link
Author

voruti commented Jul 13, 2024 •
edited
Loading

  • make it configurable
  • Tests
  • Japanese translation/strings (I can't do this myself)

@voruti
Copy link
Author

voruti commented Sep 26, 2024

@viceice Any suggestion on how we can proceed with this? Should we skip the missing translation?

@viceice
Copy link
Member

viceice commented Dec 2, 2024

@daniel-beck @timja Sorry for pinging, but do you have any suggestions about this particular issue? This workaround looks wiered to me.

@timja
Copy link
Member

timja commented Dec 2, 2024

I don't understand it enough, better to ask on the mailing list with a concise description of the problem, linking here

@viceice viceice marked this pull request as ready for review January 23, 2025 10:14
@viceice viceice requested a review from a team as a code owner January 23, 2025 10:14
viceice
viceice requested changes Jan 23, 2025
View reviewed changes
...ins/scoringloadbalancer/ScoringLoadBalancer/help-simultaneousBuildsWorkaroundEnabled_ja.html
@@ -0,0 +1,8 @@
<div>
<p>
TODO
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't know the best practices, but shouldn't we at lease use the english text here or remove the file to have the default help message?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know either. Who can help?

...coringloadbalancer/ScoringLoadBalancer/help-simultaneousBuildsWorkaroundThrottleTime_ja.html
@@ -0,0 +1,3 @@
<div>
TODO
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

@viceice
Copy link
Member

viceice commented Jan 23, 2025

it seems it needs somwe test fixes too

@viceice viceice marked this pull request as draft January 23, 2025 10:17
@voruti
Copy link
Author

voruti commented Jan 23, 2025

it seems it needs somwe test fixes too

The failing test testTwoSimultaneousBuilds_runsOnWrongNodeWithoutWorkaround indicates that the workaround is no longer necessary. I'll dive into the Jenkins source code to find out what's going on...

@voruti
Copy link
Author

voruti commented Jan 23, 2025

I'll dive into the Jenkins source code to find out what's going on...

Strange and unstable things are going on. The following log is without the workaround:

2025-01-23 21:35:00.547+0000 [id=142]   INFO    j.i.j.p.s.ScoringLoadBalancer#reportScores: Scoring for hudson.model.FreeStyleProject@17dddd48[TestJob1]:
                 Node3: 1000
                      : -500
2025-01-23 21:35:00.550+0000 [id=142]   INFO    j.i.j.p.s.ScoringLoadBalancer#reportScores: Scoring for hudson.model.FreeStyleProject@44b16915[TestJob2]:
                      : -500
2025-01-23 21:35:30.757+0000 [id=147]   INFO    j.i.j.p.s.ScoringLoadBalancer#reportScores: Scoring for hudson.model.FreeStyleProject@44b16915[TestJob2]:
                 Node3: 1000
                      : -500
2025-01-23 21:35:32.538+0000 [id=149]   INFO    j.i.j.p.s.ScoringLoadBalancer#reportScores: Scoring for hudson.model.FreeStyleProject@17dddd48[TestJob1]:
                 Node3: 1000
                      : -500
2025-01-23 21:36:02.463+0000 [id=152]   INFO    j.i.j.p.s.ScoringLoadBalancer#reportScores: Scoring for hudson.model.FreeStyleProject@44b16915[TestJob2]:
                 Node3: 1000
                      : -500
2025-01-23 21:36:04.246+0000 [id=154]   INFO    j.i.j.p.s.ScoringLoadBalancer#reportScores: Scoring for hudson.model.FreeStyleProject@17dddd48[TestJob1]:
                 Node3: 1000
                      : -500
2025-01-23 21:37:00.534+0000 [id=159]   INFO    j.i.j.p.s.ScoringLoadBalancer#reportScores: Scoring for hudson.model.FreeStyleProject@17dddd48[TestJob1]:
                 Node3: 1000
                      : -500
2025-01-23 21:37:00.535+0000 [id=159]   INFO    j.i.j.p.s.ScoringLoadBalancer#reportScores: Scoring for hudson.model.FreeStyleProject@44b16915[TestJob2]:
                      : -500
2025-01-23 21:38:00.534+0000 [id=166]   INFO    j.i.j.p.s.ScoringLoadBalancer#reportScores: Scoring for hudson.model.FreeStyleProject@17dddd48[TestJob1]:
                 Node3: 1000
                      : -500
2025-01-23 21:38:00.534+0000 [id=166]   INFO    j.i.j.p.s.ScoringLoadBalancer#reportScores: Scoring for hudson.model.FreeStyleProject@44b16915[TestJob2]:
                      : -500
2025-01-23 21:39:00.534+0000 [id=174]   INFO    j.i.j.p.s.ScoringLoadBalancer#reportScores: Scoring for hudson.model.FreeStyleProject@17dddd48[TestJob1]:
                 Node3: 1000
                      : -500
2025-01-23 21:39:00.534+0000 [id=174]   INFO    j.i.j.p.s.ScoringLoadBalancer#reportScores: Scoring for hudson.model.FreeStyleProject@44b16915[TestJob2]:
                      : -500

Sometimes the (new) default behaviour of Jenkins chooses the correct nodes/executors, sometimes it doesn't, just as before.

So the workaround is still required to get to choosing correct nodes all the time.
Testing that the workaround is required is just not that easy, as the results are unstable.

@viceice
Copy link
Member

viceice commented Jan 27, 2025

do you find the users who changed the Jenkins code recently? so we could ask them for help?

@voruti
Copy link
Author

voruti commented Jan 28, 2025

do you find the users who changed the Jenkins code recently? so we could ask them for help?

No, unfortunately I didn't find a code location that would explain this behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@viceice viceice viceice requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Simultaneous timed builds omit the first selected node from the executor list for the second scored build
3 participants
@voruti @viceice @timja