bdoyle0182 commented on code in PR #5320: URL: https://github.com/apache/openwhisk/pull/5320#discussion_r959200114
########## core/scheduler/src/main/scala/org/apache/openwhisk/core/scheduler/container/ContainerManager.scala: ########## @@ -144,6 +144,11 @@ class ContainerManager(jobManagerFactory: ActorRefFactory => ActorRef, logging.info(this, s"received ${msgs.size} creation message [${msgs.head.invocationNamespace}:${msgs.head.action}]") ContainerManager .getAvailableInvokers(etcdClient, memory, invocationNamespace) + .recover({ + case t: Throwable => + logging.error(this, s"Unable to get available invokers: ${t.getMessage}.") + List.empty[InvokerHealth] + }) Review Comment: A request is done to etcd to get the list of healthy invokers with `.getAvailableInvokers`. If the future for the request to etcd fails for any reason, there was no failure handing on that call prior to this. This function is just a synchronous `unit` so if there's no failure handling on that future it just completes and never makes an acknowledgement back to the memory queue that the creation message has been processed either successfully or failed for the memory queue to properly decrement `creationIds`. Also should clarify if it silently fails at this point, it hasn't yet registered the creation job so the akka timer to timeout the creation job to make the call back to MemoryQueue on timeout is not created. The memory queue indefinitely thinks that there is a container creation in progress. If the action never needs more than one container, it will never be able to execute because the memory queue thinks one is in progress. Also the memory queue can not be stopped on timeout in this case because of the `creationIds` not being 0. ``` else { logging.info( this, s"[$invocationNamespace:$action:$stateName] The queue is timed out but there are still ${queue.size} activation messages or (running: ${containers.size}, in-progress: ${creationIds.size}) containers") stay } ``` So while I think this covers the only edge case I know of, we really need an additional safeguard in `MemoryQueue` to eventually clear out knowledge of in progress containers if things get out of sync as there's no way to guarantee creationIds is perfectly in sync when it's dependent essentially on a fire and forget successfully making the callback at some point which is prone to introducing bugs even if this pr covers every case for now. -- 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. To unsubscribe, e-mail: issues-unsubscr...@openwhisk.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org