Remove queue locking requirement when adding new nodes #5450

res0nance · 2021-04-28T14:53:31Z

While investigating I saw that technically addNode can be made lock-free so this is a stab at it. This would greatly help cloud implementations when provisioning a large number of nodes but of course this will not help removal as that will still require locking.

It can be made non locking because a Node with no computers will not have any executors, this will ensure that it does not confuse Queue#maintain(). But because updateComputerList deals with other computers as well this method needs to lock. Adding a separate method for a single new computer should remove this locking requirement.

References:
Queue#maintain()

Proposed changelog entries

Entry 1: Remove the requirement for locking the queue when adding a new node

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). 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
For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@mention

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 correct
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).

res0nance · 2021-04-29T05:25:37Z

core/src/main/java/hudson/slaves/NodeProvisioner.java

-                                    }
-                                    fireOnFailure(f, cause);
-                                }
+            Jenkins jenkins = Jenkins.get();


This entire change is removing locking and changing 8 spaces of indent

res0nance · 2021-04-29T05:26:03Z

core/src/main/java/hudson/model/AbstractCIBase.java

+    protected void updateNewComputer(final Node n, boolean automaticSlaveLaunch) {
+        final String nodeName = n.getNodeName();
+        final Map<Node, Computer> computers = getComputerMap();
+        if (computers.containsKey(n)) {


Sanity checking although technically not needed

timja

Looks good, any specific testing been done?

res0nance · 2021-04-30T15:24:26Z

Looks good, any specific testing been done?

I've only managed some basic testing, I don't have the facilities to do a good enough stress test.
Not sure what's a good way to test it since it involves multiple threads.

stephenc · 2021-05-04T11:18:53Z

@jglick it is years since I looked at this code. I have forgotten most everything I knew. Removing myself from the reviewers

stephenc

No comment

jglick · 2021-05-04T11:57:38Z

🤷 Thought I would try!

timja

looks sensible 🤷

oleg-nenashev

I do not see anything wrong in this pull request. I suggest to go ahead with merging, but indeed we should be monitoring the Jenkins issue tracker for reported regressions. We have plenty of time until the next baseline, so it is a good time to take some risks

We may merge it in 24 hours if there is no negative feedback. Please see the merge process documentation for more information about the merge process

jglick · 2021-05-19T18:25:20Z

core/src/main/java/jenkins/model/Nodes.java

-            });
+            old.set(nodes.put(node.getNodeName(), node));
+            jenkins.updateNewComputer(node);
+            jenkins.trimLabels();
            // TODO there is a theoretical race whereby the node instance is updated/removed after lock release


As there is no longer a lock above, is this comment obsolete?

basil · 2021-07-28T18:05:55Z

With this PR, org.jenkinsci.plugins.pipeline.modeldefinition.AgentTest#agentOnGroup hangs. Without this PR, the test passes.

jglick · 2021-07-28T18:36:36Z

Indeed:

Still waiting to schedule task
Waiting for next available executor on ‘slave0’

Although the config screen for slave0 shows it as having 2 executors, the executor widget shows just one—which is occupied, hence the hung build I guess. If you Save that config screen, the build resumes and the test passes.

jglick · 2021-07-28T18:49:09Z

I suspect this somehow broke

jenkins/core/src/main/java/hudson/model/Computer.java

Lines 882 to 895 in 20082d4

    
               /** 
        
                * Calling path, *means protected by Queue.withLock 
        
                * 
        
                * Computer.doConfigSubmit -> Computer.replaceBy ->Jenkins.setNodes* ->Computer.setNode 
        
                * AbstractCIBase.updateComputerList->Computer.inflictMortalWound* 
        
                * AbstractCIBase.updateComputerList->AbstractCIBase.updateComputer* ->Computer.setNode 
        
                * AbstractCIBase.updateComputerList->AbstractCIBase.killComputer->Computer.kill 
        
                * Computer.constructor->Computer.setNode 
        
                * Computer.kill is called after numExecutors set to zero(Computer.inflictMortalWound) so not need the Queue.lock 
        
                * 
        
                * @param n number of executors 
        
                */ 
        
               @GuardedBy("hudson.model.Queue.lock") 
        
               private void setNumExecutors(int n) {

res0nance · 2021-07-31T11:04:38Z

I suspect this somehow broke

jenkins/core/src/main/java/hudson/model/Computer.java

Lines 882 to 895 in 20082d4

/**

* Calling path, *means protected by Queue.withLock

*

* Computer.doConfigSubmit -> Computer.replaceBy ->Jenkins.setNodes* ->Computer.setNode

* AbstractCIBase.updateComputerList->Computer.inflictMortalWound*

* AbstractCIBase.updateComputerList->AbstractCIBase.updateComputer* ->Computer.setNode

* AbstractCIBase.updateComputerList->AbstractCIBase.killComputer->Computer.kill

* Computer.constructor->Computer.setNode

* Computer.kill is called after numExecutors set to zero(Computer.inflictMortalWound) so not need the Queue.lock

*

* @param n number of executors

*/

@GuardedBy("hudson.model.Queue.lock")

private void setNumExecutors(int n) {

Not sure how this causes a problem, but the test issue seems to indicate as much

basil · 2022-09-06T21:47:00Z

Appears to cause JENKINS-69534

jglick · 2024-02-14T17:54:25Z

core/src/main/java/jenkins/model/Nodes.java

@@ -141,17 +141,10 @@ public void addNode(final @NonNull Node node) throws IOException {

        Node oldNode = nodes.get(node.getNodeName());
        if (node != oldNode) {
-            // TODO we should not need to lock the queue for adding nodes but until we have a way to update the
-            // computer list for just the new node
            AtomicReference<Node> old = new AtomicReference<>();


With the inner class gone, there is no more need for an AtomicReference so this could be simplified.

Add lock free new computer update

9398633

res0nance marked this pull request as draft April 28, 2021 14:53

Raihaan Shouhell added 2 commits April 29, 2021 12:17

Name cleanups

ac24849

Fix sanity check to be lock free

ce4dca6

res0nance changed the title ~~Add lock free new computer update~~ Remove queue locking requirement when adding new nodes Apr 29, 2021

res0nance added rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted squash-merge-me Unclean or useless commit history, should be merged only with squash-merge labels Apr 29, 2021

res0nance commented Apr 29, 2021

View reviewed changes

res0nance marked this pull request as ready for review April 29, 2021 06:21

timja reviewed Apr 29, 2021

View reviewed changes

timja requested review from jglick and a team May 3, 2021 20:20

jglick mentioned this pull request May 3, 2021

[JENKINS-27565] Fix threading issues with Nodes and Queue #1596

Merged

11 tasks

jglick requested a review from stephenc May 3, 2021 20:47

stephenc reviewed May 4, 2021

View reviewed changes

timja requested a review from a team May 4, 2021 14:22

timja approved these changes May 7, 2021

View reviewed changes

oleg-nenashev self-requested a review May 13, 2021 21:26

oleg-nenashev approved these changes May 16, 2021

View reviewed changes

oleg-nenashev added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label May 16, 2021

timja merged commit b1aeef9 into jenkinsci:master May 19, 2021

jglick reviewed May 19, 2021

View reviewed changes

res0nance deleted the lock-free-node-add branch May 30, 2021 09:04

basil mentioned this pull request Jul 28, 2021

Initialize new LTS line (2.302) jenkinsci/bom#582

Merged

6 tasks

jglick mentioned this pull request Jul 28, 2021

Call Jenkins.setNodes to adapt to core change in 2.302 jenkinsci/pipeline-model-definition-plugin#441

Merged

jglick reviewed Feb 14, 2024

View reviewed changes

jglick mentioned this pull request Feb 21, 2024

Nodes persistence cleanup, APIs to control loading #8979

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove queue locking requirement when adding new nodes #5450

Remove queue locking requirement when adding new nodes #5450

res0nance commented Apr 28, 2021 •

edited

Loading

res0nance Apr 29, 2021

res0nance Apr 29, 2021

timja left a comment

res0nance commented Apr 30, 2021

stephenc commented May 4, 2021

stephenc left a comment

jglick commented May 4, 2021

timja left a comment

oleg-nenashev left a comment

jglick May 19, 2021

basil commented Jul 28, 2021

jglick commented Jul 28, 2021

jglick commented Jul 28, 2021 •

edited

Loading

res0nance commented Jul 31, 2021

basil commented Sep 6, 2022

jglick Feb 14, 2024

Remove queue locking requirement when adding new nodes #5450

Remove queue locking requirement when adding new nodes #5450

Conversation

res0nance commented Apr 28, 2021 • edited Loading

Proposed changelog entries

Proposed upgrade guidelines

Submitter checklist

Desired reviewers

Maintainer checklist

res0nance Apr 29, 2021

Choose a reason for hiding this comment

res0nance Apr 29, 2021

Choose a reason for hiding this comment

timja left a comment

Choose a reason for hiding this comment

res0nance commented Apr 30, 2021

stephenc commented May 4, 2021

stephenc left a comment

Choose a reason for hiding this comment

jglick commented May 4, 2021

timja left a comment

Choose a reason for hiding this comment

oleg-nenashev left a comment

Choose a reason for hiding this comment

jglick May 19, 2021

Choose a reason for hiding this comment

basil commented Jul 28, 2021

jglick commented Jul 28, 2021

jglick commented Jul 28, 2021 • edited Loading

res0nance commented Jul 31, 2021

basil commented Sep 6, 2022

jglick Feb 14, 2024

Choose a reason for hiding this comment

res0nance commented Apr 28, 2021 •

edited

Loading

jglick commented Jul 28, 2021 •

edited

Loading