| Yes, I have to agree with you there - that's a bug.
It's not a bug I have time to fix myself, but I can at least point the way forwards so, if anyone feels like having a go and submitting a PR that fixes the issue, please do. So the issue is that the provision(label,number) method isn't using the same (null-proof) logic as the getTemplates(label) method to determine if a node matches the given label or not. What's needed is to extract that null-proof logic into a method that we can call from both places, and then use it from both methods. So, I'd guess that what we need is a common method
private static boolean meetsLabelRequirements(final Label requirements, final Mode actualMode,
final Set<LabelAtom> actualLabel) {
if (actualMode == Node.Mode.NORMAL) {
if (requirements == null || requirements.matches(actualLabel)) {
return true;
}
} else if (actualMode == Node.Mode.EXCLUSIVE) {
if (requirements != null && requirements.matches(actualLabel)) {
return true;
}
}
return false;
}
...and then to refactor the getTemplates(final Label label) method so that it says
private List<vSphereCloudSlaveTemplate> getTemplates(final Label requirements) {
if (this.templates == null)
return Collections.emptyList();
List<vSphereCloudSlaveTemplate> matchingTemplates = new ArrayList<vSphereCloudSlaveTemplate>();
for (vSphereCloudSlaveTemplate t : this.templates) {
final Set<LabelAtom> actualLabel = t.getLabelSet();
final Mode actualMode = t.getMode();
if (meetsLabelRequirements(requirements, actualMode, actualLabel)) {
matchingTemplates.add(t);
}
}
return matchingTemplates;
}
to change the buggy line in the provision method to be
if (n.getComputer().isOffline() && meetsLabelRequirements(label, n.getMode(), n.getAssignedLabels())) {
...and (and this is the bit I don't have time for) test it to make damned sure that it's not broken anything and that it fixes the problem. If it's all ok, wrap it up in a pull-request referencing this issue (see https://github.com/jenkinsci/vsphere-cloud-plugin/blob/master/CONTRIBUTING.md for more details). If you, or someone else, does that, I'd be happy to review and merge it so it'll be in the next release. |
