Uploaded image for project: 'Jenkins'
  1. Jenkins
  2. JENKINS-38903

Split the Exceptions handling for node provision and adding a node to Jenkins in NodeProvisioner

    XMLWordPrintable

    Details

    • Type: Improvement
    • Status: Resolved (View Workflow)
    • Priority: Minor
    • Resolution: Fixed
    • Component/s: core
    • Labels:
      None
    • Similar Issues:

      Description

      Current implementation of NodeProvisioner.update() handles an exception from node provisioning and adding a node to Jenkins in the one block where it isn't clear from which step an Exception was thrown - it can create a confusion for ex. Provisioned slave " + f.displayName + " failed to launch in handling ExecutionException is very confusing because the mentioned provisioned slave doesn't exist, it could never exist etc.

      I'd propose to split this to two separate try/catch blocks (+ one general try/catch block) and adjust the error messages a bit:
      First part:

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

      should catch and handle InterruptedException, ExecutionException.

      Second part:

       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});
       

      should catch and handle IOException.

      Both parts are enclosed with try/catch block for catching of Error and Throwable as the last stand if they occur.

      In addition the first block shouldn't log anything here if ExecutionException.getCause() is instance of AbortException. AbortException has the unified meaning through the Jenkins - we need to signalize a situation when a provision process of a new node failed (in Future.call()); during the provision process we know about this failure, we recovered from it and we need to report that situation to NodeProvisioner. In NodeProvisioner we shouldn't make any additional action because handling and recovering is a responsibility of the reported in the place it was thrown.

      Proposed change should be fully backward compatible.

      TO-DO: When we introduce this change in the core first, we can adjust exception throwing from cloud provider plugins.

        Attachments

          Issue Links

            Activity

            Hide
            pajasoft Pavel Janoušek added a comment -

            PR sent.

            Show
            pajasoft Pavel Janoušek added a comment - PR sent.
            Hide
            oleg_nenashev Oleg Nenashev added a comment -

            The change has been integrated towards 2.37

            Show
            oleg_nenashev Oleg Nenashev added a comment - The change has been integrated towards 2.37
            Hide
            scm_issue_link SCM/JIRA link daemon added a comment -

            Code changed in jenkins
            User: Pavel Janousek
            Path:
            core/src/main/java/hudson/slaves/CloudProvisioningListener.java
            core/src/main/java/hudson/slaves/NodeProvisioner.java
            http://jenkins-ci.org/commit/jenkins/15e69874190ce56e228b823aa1584b8497fc673b
            Log:
            JENKINS-38903 Split Exception handling for node provision and adding to Jenkins (#2591)

            • JENKINS-38903 Split Exception handling for node provision and adding to Jenkins
            • Defined new static helper methods that ensure exceptions are not propagated
            • Added onCommit and onRollback signals to CloudProvisioningListener

            Added the new signals to be able to notify the state after Jenkins.addNode(Node)
            All Listener's calls moved to an exception-tolerant static helpers

            • Added @Nonnull annotation
              Changed the method signature CloudProvisioningListener.onRollback()
            • Re-throw Error in the fireOnXXX()
              Removed re-thrown Throwable in the main try/catch block
              (an instance of the Error is handled separately)
            • Handling of Error changed
            • Fixed Error instance handling in NodeProvisioner.fireOnFailure()
            Show
            scm_issue_link SCM/JIRA link daemon added a comment - Code changed in jenkins User: Pavel Janousek Path: core/src/main/java/hudson/slaves/CloudProvisioningListener.java core/src/main/java/hudson/slaves/NodeProvisioner.java http://jenkins-ci.org/commit/jenkins/15e69874190ce56e228b823aa1584b8497fc673b Log: JENKINS-38903 Split Exception handling for node provision and adding to Jenkins (#2591) JENKINS-38903 Split Exception handling for node provision and adding to Jenkins Defined new static helper methods that ensure exceptions are not propagated Added onCommit and onRollback signals to CloudProvisioningListener Added the new signals to be able to notify the state after Jenkins.addNode(Node) All Listener's calls moved to an exception-tolerant static helpers Added @Nonnull annotation Changed the method signature CloudProvisioningListener.onRollback() Re-throw Error in the fireOnXXX() Removed re-thrown Throwable in the main try/catch block (an instance of the Error is handled separately) Handling of Error changed Fixed Error instance handling in NodeProvisioner.fireOnFailure()
            Hide
            oleg_nenashev Oleg Nenashev added a comment - - edited

            Released in jenkins-2.37

            Show
            oleg_nenashev Oleg Nenashev added a comment - - edited Released in jenkins-2.37

              People

              • Assignee:
                pajasoft Pavel Janoušek
                Reporter:
                pajasoft Pavel Janoušek
              • Votes:
                0 Vote for this issue
                Watchers:
                3 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: