[GitHub] [incubator-openwhisk] markusthoemmes commented on a change in pull request #4424: Delete pod when creating timeout

[GitBox <gi...@apache.org>]

markusthoemmes commented on a change in pull request #4424: Delete pod when creating timeout
URL: https://github.com/apache/incubator-openwhisk/pull/4424#discussion_r271254777
 
 

 ##########
 File path: core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/kubernetes/KubernetesClient.scala
 ##########
 @@ -170,6 +170,15 @@ class KubernetesClient(
       }
     }.recoverWith {
       case e =>
+        Future {
+          kubeRestClient
+            .inNamespace(kubeRestClient.getNamespace)
+            .pods()
+            .withName(name)
+            .delete()
+        }.recover {
+          case ex => log.error(this, s"Failed delete pod for '$name': ${ex.getClass} - ${ex.getMessage}")
+        }
 
 Review comment:
   This is not quite how it works. First of all, you'll want to wait for this future to complete before you finish the wrapping Future of the `run`. Otherwise, this will be some random operation, running somewhere in the background uncontrollably.
   
   In this case, the sequence you'll want is as follows:
   
   1. Try to create the pod.
   2. If that failed, delete it. Ignore any errors here since we don't know if there's something to delete even.
   3. regardless of whether the delete failed or not, return the error from the pod creation.
   
   In this case, you can pull that off like so:
   
   ```scala
   Future { 
     blocking { // Try to create a pod }
   }.andThen { // Once that returns, log if it was an error but pass the original future through
     case Failure(e) => log.error(this, s"Failed create pod for '$name': ${e.getClass} - ${e.getMessage}")
   }.recoverWith { // Do something on the error and flatten with the newly created Future
     case createError =>
       Future { 
         blocking { // Try to delete the pod } 
       }.andThen { // If that failed, log. Pass through the original Future (the one from the deleting the pod)
           case Failure(ex) => log.error(this, s"Failed delete pod for '$name': ${ex.getClass} - ${ex.getMessage}")
         }
         .transformWith { _ => // Whether or not pod deletion failed, flatten its Future with a failed Future, which also fails the recoverWith again, returning a failed Future overall
           Future.failed(new Exception(s"Failed to create pod '$name'"))
         }
   }
   ```
   
   This code might not compile as is, but it should get the intent across. Does that make sense?

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services