Skip to content
Permalink
Browse files

[FIXED JENKINS-29568] A better fix

- 252e129 doesn't work for all cases as there are code paths
  where a pending launch can be removed from the pending list and not have spent() called.
- There was no reason for iterating the list twice anyway, as all of this takes place with the locks held
- My notifying each one as we process, if there is an Error, we will not leave any stranded. The next run
  through, if there is one, will cover those instances.
  • Loading branch information...
stephenc committed Jul 23, 2015
1 parent 4ece8c0 commit 4f0ca16b01abcd5ac84a9259736c2d96b1712354
Showing with 35 additions and 33 deletions.
  1. +35 −33 core/src/main/java/hudson/slaves/NodeProvisioner.java
@@ -213,48 +213,50 @@ public void run() {
// bring up.

int plannedCapacitySnapshot = 0;
List<PlannedNode> completedLaunches = new ArrayList<PlannedNode>();

for (Iterator<PlannedNode> itr = pendingLaunches.iterator(); itr.hasNext(); ) {
PlannedNode f = itr.next();
if (f.future.isDone()) {
completedLaunches.add(f);
itr.remove();
try {
Node node = f.future.get();
for (CloudProvisioningListener cl : CloudProvisioningListener.all()) {
cl.onComplete(f, node);
}

jenkins.addNode(node);
LOGGER.log(Level.INFO,
"{0} provisioning successfully completed. "
+ "We have now {1,number,integer} computer(s)",
new Object[]{f.displayName, jenkins.getComputers().length});
} catch (InterruptedException e) {
throw new AssertionError(e); // since we confirmed that the future is already done
} catch (ExecutionException e) {
LOGGER.log(Level.WARNING, "Provisioned slave " + f.displayName + " failed to launch",
e.getCause());
for (CloudProvisioningListener cl : CloudProvisioningListener.all()) {
cl.onFailure(f, e.getCause());
}
} catch (IOException e) {
LOGGER.log(Level.WARNING, "Provisioned slave " + f.displayName + " failed to launch",
e);

This comment has been minimized.

@KostyaSha

KostyaSha Jul 23, 2015

Member

imho, ignore file width for such cases. Readability should be more critical rather than blind checkstyling.

for (CloudProvisioningListener cl : CloudProvisioningListener.all()) {
cl.onFailure(f, e);
}
} catch (Error e) {
// we are not supposed to try and recover from Errors
throw e;
} catch (Throwable e) {
LOGGER.log(Level.SEVERE, "Unexpected uncaught exception encountered while "
+ "processing provisioned slave " + f.displayName, e);
} finally {
itr.remove();
f.spent();
}
} else {
plannedCapacitySnapshot += f.numExecutors;
}
}

for (PlannedNode f : completedLaunches) {
try {
Node node = f.future.get();
for (CloudProvisioningListener cl : CloudProvisioningListener.all()) {
cl.onComplete(f, node);
}

jenkins.addNode(node);
LOGGER.log(Level.INFO,
"{0} provisioning successfully completed. We have now {1,number,integer} computer"
+ "(s)",
new Object[]{f.displayName, jenkins.getComputers().length});
} catch (InterruptedException e) {
throw new AssertionError(e); // since we confirmed that the future is already done
} catch (ExecutionException e) {
LOGGER.log(Level.WARNING, "Provisioned slave " + f.displayName + " failed to launch",
e.getCause());
for (CloudProvisioningListener cl : CloudProvisioningListener.all()) {
cl.onFailure(f, e.getCause());
}
} catch (IOException e) {
LOGGER.log(Level.WARNING, "Provisioned slave " + f.displayName + " failed to launch", e);
for (CloudProvisioningListener cl : CloudProvisioningListener.all()) {
cl.onFailure(f, e);
}
} finally {
f.spent();
}
}

float plannedCapacity = plannedCapacitySnapshot;
plannedCapacitiesEMA.update(plannedCapacity);

0 comments on commit 4f0ca16

Please sign in to comment.
You can’t perform that action at this time.