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

    • Icon: Improvement Improvement
    • Resolution: Fixed
    • Icon: Minor Minor
    • core
    • None

      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.

          [JENKINS-38903] Split the Exceptions handling for node provision and adding a node to Jenkins in NodeProvisioner

          Pavel Janoušek created issue -
          Pavel Janoušek made changes -
          Description Original: Current implementation of {{[NodeProvisioner.update()|https://github.com/jenkinsci/jenkins/blob/master/core/src/main/java/hudson/slaves/NodeProvisioner.java#L188]}} 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 and adjust the error messages a bit:
          First part: {code}
          Node node = f.future.get();
          for (CloudProvisioningListener cl : CloudProvisioningListener.all()) {
              cl.onComplete(f, node);
          }{code}
          should catch and handle {{InterruptedException}}, {{ExecutionException}}, {{Error}} and {{Throwable}}.

          Send part:{code}
           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});
           {code}
          should catch and handle {{IOException}}, {{Error}} and {{Throwable}}.

          In addition the first part should handle the new type of Exception {{AbortException}} which has the unified meaning through the Jenkins - we need to signalize a situation when a provision process of a new node failed; 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 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.
          New: Current implementation of {{[NodeProvisioner.update()|https://github.com/jenkinsci/jenkins/blob/master/core/src/main/java/hudson/slaves/NodeProvisioner.java#L188]}} 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 and adjust the error messages a bit:
          First part: {code}
          Node node = f.future.get();
          for (CloudProvisioningListener cl : CloudProvisioningListener.all()) {
              cl.onComplete(f, node);
          }{code}
          should catch and handle {{InterruptedException}}, {{ExecutionException}}, {{Error}} and {{Throwable}}.

          Second part:{code}
           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});
           {code}
          should catch and handle {{IOException}}, {{Error}} and {{Throwable}}.

          In addition the first part should handle the new type of Exception {{AbortException}} which has the unified meaning through the Jenkins - we need to signalize a situation when a provision process of a new node failed; 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 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.
          Pavel Janoušek made changes -
          Description Original: Current implementation of {{[NodeProvisioner.update()|https://github.com/jenkinsci/jenkins/blob/master/core/src/main/java/hudson/slaves/NodeProvisioner.java#L188]}} 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 and adjust the error messages a bit:
          First part: {code}
          Node node = f.future.get();
          for (CloudProvisioningListener cl : CloudProvisioningListener.all()) {
              cl.onComplete(f, node);
          }{code}
          should catch and handle {{InterruptedException}}, {{ExecutionException}}, {{Error}} and {{Throwable}}.

          Second part:{code}
           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});
           {code}
          should catch and handle {{IOException}}, {{Error}} and {{Throwable}}.

          In addition the first part should handle the new type of Exception {{AbortException}} which has the unified meaning through the Jenkins - we need to signalize a situation when a provision process of a new node failed; 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 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.
          New: Current implementation of {{[NodeProvisioner.update()|https://github.com/jenkinsci/jenkins/blob/master/core/src/main/java/hudson/slaves/NodeProvisioner.java#L188]}} 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: {code}
          Node node = f.future.get();
          for (CloudProvisioningListener cl : CloudProvisioningListener.all()) {
              cl.onComplete(f, node);
          }{code}
          should catch and handle {{InterruptedException}}, {{ExecutionException}}.

          Second part:{code}
           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});
           {code}
          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 part should handle the new type of Exception {{AbortException}} which has the unified meaning through the Jenkins - we need to signalize a situation when a provision process of a new node failed; 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 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.
          Pavel Janoušek made changes -
          Description Original: Current implementation of {{[NodeProvisioner.update()|https://github.com/jenkinsci/jenkins/blob/master/core/src/main/java/hudson/slaves/NodeProvisioner.java#L188]}} 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: {code}
          Node node = f.future.get();
          for (CloudProvisioningListener cl : CloudProvisioningListener.all()) {
              cl.onComplete(f, node);
          }{code}
          should catch and handle {{InterruptedException}}, {{ExecutionException}}.

          Second part:{code}
           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});
           {code}
          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 part should handle the new type of Exception {{AbortException}} which has the unified meaning through the Jenkins - we need to signalize a situation when a provision process of a new node failed; 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 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.
          New: Current implementation of {{[NodeProvisioner.update()|https://github.com/jenkinsci/jenkins/blob/master/core/src/main/java/hudson/slaves/NodeProvisioner.java#L188]}} 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: {code}
          Node node = f.future.get();
          for (CloudProvisioningListener cl : CloudProvisioningListener.all()) {
              cl.onComplete(f, node);
          }{code}
          should catch and handle {{InterruptedException}}, {{ExecutionException}}.

          Second part:{code}
           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});
           {code}
          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 part should handle the new type of Exception - {{AbortException}} which 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 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.
          Pavel Janoušek made changes -
          Description Original: Current implementation of {{[NodeProvisioner.update()|https://github.com/jenkinsci/jenkins/blob/master/core/src/main/java/hudson/slaves/NodeProvisioner.java#L188]}} 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: {code}
          Node node = f.future.get();
          for (CloudProvisioningListener cl : CloudProvisioningListener.all()) {
              cl.onComplete(f, node);
          }{code}
          should catch and handle {{InterruptedException}}, {{ExecutionException}}.

          Second part:{code}
           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});
           {code}
          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 part should handle the new type of Exception - {{AbortException}} which 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 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.
          New: Current implementation of {{[NodeProvisioner.update()|https://github.com/jenkinsci/jenkins/blob/master/core/src/main/java/hudson/slaves/NodeProvisioner.java#L188]}} 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: {code}
          Node node = f.future.get();
          for (CloudProvisioningListener cl : CloudProvisioningListener.all()) {
              cl.onComplete(f, node);
          }{code}
          should catch and handle {{InterruptedException}}, {{ExecutionException}}.

          Second part:{code}
           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});
           {code}
          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.
          Pavel Janoušek made changes -
          Status Original: Open [ 1 ] New: In Progress [ 3 ]
          Pavel Janoušek made changes -
          Status Original: In Progress [ 3 ] New: In Review [ 10005 ]
          Oleg Nenashev made changes -
          Remote Link New: This issue links to "jenkins/pull/2591 (Web Link)" [ 15133 ]
          Oleg Nenashev made changes -
          Resolution New: Fixed [ 1 ]
          Status Original: In Review [ 10005 ] New: Resolved [ 5 ]

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

              Created:
              Updated:
              Resolved: