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.

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

[FIXED JENKINS-42043] Catch and log RuntimeException in setNode #2836

Merged
merged 1 commit into from Apr 14, 2017

Conversation

6 participants
@abayer
Member

abayer commented Apr 7, 2017

Description

See JENKINS-42043.

Details: Catch and log RuntimeException in call to SlaveComputer.setNode(n) in AbstractCIBase.updateComputerList(...). Also make sure we don't mark the Computer as used so that we kill any executors that may be related to it somehow.

Not sure how to test this - suggestions?

Changelog entries

Proposed changelog entries:

  • Entry 1: JENKINS-42043, RuntimeException in SlaveComputer.setNode(n) will no longer prevent Jenkins startup

Submitter checklist

  • JIRA issue is well described
  • Link to JIRA ticket in description, if appropriate
  • Appropriate autotests or explanation to why this change has no tests
  • For new API and extension points: Link to the reference implementation in open-source (or example in Javadoc)

Desired reviewers

@reviewbybees @batmat @jglick

[FIXED JENKINS-42043] Catch and log RuntimeException in setNode
Also make sure we don't mark the Computer as used so that we kill any
executors that may be related to it somehow.

@abayer abayer requested review from jglick and batmat Apr 7, 2017

@reviewbybees

This comment has been minimized.

reviewbybees commented Apr 7, 2017

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@rsandell

This comment has been minimized.

Member

rsandell commented Apr 10, 2017

🐝

@oleg-nenashev

LGTM 🐝 . Maybe c.connect() in the else clause should be handled in a similar way. NIT anyway since it is supposed to be never called

@batmat

batmat approved these changes Apr 10, 2017

🐝 small questioning on the nullable c, but anyway this was already the case before the change IIUC.

c.setNode(n); // reuse
used.add(c);
} catch (RuntimeException e) {
LOGGER.log(Level.WARNING, "Error updating node " + n.getNodeName() + ", continuing", e);

This comment has been minimized.

@batmat

batmat Apr 10, 2017

Member

Better avoid concatenating, but well, this code path will not be called often and JUL is crappily verbose to do the same the right way :-(

@@ -131,8 +137,8 @@ private void updateComputer(Node n, Map<String,Computer> byNameMap, Set<Computer
}
}
}
used.add(c);

This comment has been minimized.

@batmat

batmat Apr 10, 2017

Member

there's a chance c is going to be null here right? Granted a node with no executor should be quite rare I guess but well :).
Or maybe the passed used container accepts null values?

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Apr 10, 2017

Member

Nothing is documented about Node#createComputer() . The core itself does not operate with nulls from what I see, but there is a case for that :( https://github.com/jenkinsci/gcloud-plugin/blob/4b84dbdd2532dc65feb5bef11f82f713360622b2/src/main/java/com/cloudbees/jenkins/plugins/gcloud/GoogleSlave.java#L21

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Apr 10, 2017

Member

I suspect it deserves a Jenkins issue for proper documentation/annotation, but it is not a part of this PR IMHO.

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Apr 11, 2017

@reviewbybees done
Created https://issues.jenkins-ci.org/browse/JENKINS-43496 as a follow-up to the discussion with @batmat

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Apr 13, 2017

Will merge it tomorrow unless somebody votes against

@oleg-nenashev oleg-nenashev self-assigned this Apr 13, 2017

@jglick

jglick approved these changes Apr 13, 2017

@oleg-nenashev oleg-nenashev merged commit d48e1d6 into jenkinsci:master Apr 14, 2017

1 of 2 checks passed

Jenkins Looks like there's a problem with this pull request
Details
continuous-integration/jenkins/pr-head This commit looks good
Details

oleg-nenashev added a commit to oleg-nenashev/jenkins that referenced this pull request Jun 18, 2017

[JENKINS-43496] - Add handling of the null Node#createComputer() result.
it is a follow-up to jenkinsci#2836 (comment)

De-facto many Cloud plugins return `null` in `Node#createLauncher()`, but it has never been documented.
In order to prevent possible API misusages in the future, I have added annotations and fixed handling of the extension point in `AbstractCIBase#updateComputer()` which may fail in the case of `null` or `RuntimeException` in the Node implementation.

oleg-nenashev added a commit that referenced this pull request Jul 1, 2017

[JENKINS-43496] - Add handling of the null Node#createComputer() resu…
…lt. (#2922)

* [JENKINS-43496] - Add handling of the null Node#createComputer() result.

it is a follow-up to #2836 (comment)

De-facto many Cloud plugins return `null` in `Node#createLauncher()`, but it has never been documented.
In order to prevent possible API misusages in the future, I have added annotations and fixed handling of the extension point in `AbstractCIBase#updateComputer()` which may fail in the case of `null` or `RuntimeException` in the Node implementation.

* [JENKINS-43496] - Use ProtectedExternally to protect Node#createComputer()

* [JENKINS-43496] - Remove the erroneous Nonnull annotation after the feedback from @jglick

* [JENKINS-43496] - Fix typos noticed by @daniel-beck

olivergondza added a commit that referenced this pull request Jul 20, 2017

[JENKINS-43496] - Add handling of the null Node#createComputer() resu…
…lt. (#2922)

* [JENKINS-43496] - Add handling of the null Node#createComputer() result.

it is a follow-up to #2836 (comment)

De-facto many Cloud plugins return `null` in `Node#createLauncher()`, but it has never been documented.
In order to prevent possible API misusages in the future, I have added annotations and fixed handling of the extension point in `AbstractCIBase#updateComputer()` which may fail in the case of `null` or `RuntimeException` in the Node implementation.

* [JENKINS-43496] - Use ProtectedExternally to protect Node#createComputer()

* [JENKINS-43496] - Remove the erroneous Nonnull annotation after the feedback from @jglick

* [JENKINS-43496] - Fix typos noticed by @daniel-beck

(cherry picked from commit bcf55ec)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment