You are viewing a plain text version of this content. The canonical link for it is here.

Posted to issues@openwhisk.apache.org by GitBox <gi...@apache.org> on 2021/01/26 04:45:40 UTC

[GitHub] [openwhisk] bdoyle0182 opened a new issue #5054: Various Potential Bugs in Container Pool and Container Proxy

bdoyle0182 opened a new issue #5054:
URL: https://github.com/apache/openwhisk/issues/5054

I've found what I believe are some potential cases and would like some confirmation that I'm in the right direction with these bugs before I start trying to remediate them. Would like to hear people's opinions on if any of these seem like they might actually be the case. These are just a few things I found after looking at these two files for a couple hours. If a lot of them are, we may want to audit the Container Pool and Container Proxy for edge cases as the logic in these FSM's and FSM management have gotten quite complex. This investigation was brought forth by the bug I opened earlier today about the healthcheck protocol and building healthcheck lag in the invoker topics.

1. On the failure of a cold start in the Container Proxy, we attempt to go to state `Removing`. I believe this results in the FSM from never being stopped as once in state `Removing` no new messages will be sent to it. The message that ultimately is sent to stop the FSM is sent from within the `destroyContainer` function and that will never be called in this case if concurrency is not used and there's only one activation in the proxy. This is the line that was added recently to move to that state on this failure:

https://github.com/apache/openwhisk/commit/4956362f8f9feb217d3c96feb644cdb2ffb7ee6c#diff-4b149cfaa5f4b88c5fe7d5fd798d4ff12ab655c7d4e4fccb47072b7763800a05R502

2. When the container pool is completely busy, it now relies on an event to hit that calls `processBufferOrFeed` to attempt to execute a new `Run` message. I'm afraid that this is error prone to dead locks even though not re-calling `Run` with the head of the buffer at the end of trying to schedule that message in the ContainerPool frees up CPU cycles. I haven't been able to pinpoint anything yet that would lead to a deadlock, but we are seeing behavior in our invokers where it appears that a deadlock is happening on loaded invokers rarely.

https://github.com/apache/openwhisk/commit/a7482ca22694eee9b578c1003cd2c847953273e8#diff-907fea4f2800af33d571845b19ddc9a24a44d481994305f344c23d33310d471aL205

3. We are seeing lots of error logs of `unhandled event Remove in state Removing`. This happens when the Container Pool sends a `Remove` message to the ContainerProxy when it's already in the `Removing` state in the fsm. My belief is that this is happening when the invoker is loaded and when some containers are failing it moves to removing early; and the container pool subsequently tries to remove the container to force a container recreation for a new action. This is probably fairly innocuous, but should be handled.

4. The `Removing` state used to stop the fsm if it received any failure message while in the state. The `destroyContainer` function forwards the `ContainerRemoved` message when all futured operations related to destroying the container completes which `Removing` receives and stops the fsm. On the unhappy case if something fails in `destroyContainer`, removing will no longer stop the fsm.

https://github.com/apache/openwhisk/commit/d4f9e8abf6739bd6ee48d64f567f93a90ef91daf#diff-4b149cfaa5f4b88c5fe7d5fd798d4ff12ab655c7d4e4fccb47072b7763800a05L541

5. This seems over eager to be calling `processBufferOrFeed`. Previously it would just try to dequeue the next item in the buffer, but by calling `processBufferOrFeed` you may be pulling from the message feed too soon since the buffer could now be empty:

https://github.com/apache/openwhisk/commit/a7482ca22694eee9b578c1003cd2c847953273e8#diff-907fea4f2800af33d571845b19ddc9a24a44d481994305f344c23d33310d471aR183

----------------------------------------------------------------
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

[GitHub] [openwhisk] bdoyle0182 edited a comment on issue #5054: Various Potential Bugs in Container Pool and Container Proxy

Posted by GitBox <gi...@apache.org>.

bdoyle0182 edited a comment on issue #5054:
URL: https://github.com/apache/openwhisk/issues/5054#issuecomment-767957171

1. I agree that was my conclusion on what was meant to be included there.
2. Are you saying we should add back the `processBufferOrFeed` there? If so I think I agree.
4. The issue in my opinion is that the cases that are handled by the Removing state have changed so it's not a catch all for all future failures in the commit I linked for this one. If that `container.destroy()` fails, it will pipe the failure to `Removing` but only a failure when the container is `WarmedData` is caught.

I'm also a little concerned that we give up so easily on attempting to destroy the container. If that `container.destroy()` call fails, we cease tracking of the container immediately. I'm not sure if there's a more graceful or more guaranteed way of handling this other than coding in retries.

5. I agree this isn't a major concern. My only concern was it pulling extra things into memory too early, but not too big of a deal. One thing I was thinking is this could cause more container recreations if things are pulled early and sitting in the buffer and continuously retried they may eagerly tear down a container when a warm container would have been ready shortly had it waited to pull something from kafka, but that's conjecture.

Thanks for taking the time to sanity check all of these for me. At the very least we should fix 1. asap. If I find any other logic that I question in the container management logic I'll be sure to add again to get some other opinions.

You're no longer using the container proxy / pool, is that because of the new scheduler? Will that be a part of the scheduler change?

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] bdoyle0182 commented on issue #5054: Various Potential Bugs in Container Pool and Container Proxy

Posted by GitBox <gi...@apache.org>.

bdoyle0182 commented on issue #5054:
URL: https://github.com/apache/openwhisk/issues/5054#issuecomment-767957171

5. I agree this isn't a major concern. My only concern was it pulling extra things into memory too early, but not too big of a deal.

You're no longer using the container proxy / pool, is that because of the new scheduler? Will that be a part of the scheduler change?

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] style95 commented on issue #5054: Various Potential Bugs in Container Pool and Container Proxy

Posted by GitBox <gi...@apache.org>.

style95 commented on issue #5054:
URL: https://github.com/apache/openwhisk/issues/5054#issuecomment-768016085


   Yes, we are using another version of container pool / proxy which are aligned with the scheduler and are part of the scheduler contribution.
   


----------------------------------------------------------------
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

[GitHub] [openwhisk] style95 commented on issue #5054: Various Potential Bugs in Container Pool and Container Proxy

Posted by GitBox <gi...@apache.org>.

style95 commented on issue #5054:
URL: https://github.com/apache/openwhisk/issues/5054#issuecomment-767950783


   IMHO,
   
   1. It looks like a bug to me.
   I think it is supposed to see if `activeCount` is 0 and terminate the container if it is 0 like this.
   https://github.com/apache/openwhisk/blob/master/core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/ContainerProxy.scala#L485
   
   
   2. `processBufferOrFeed`.
   
   Since I haven't visited the code as we no longer use them I looked into it.
   But not quite sure what is wrong for now.
   
   3. Yes that log clearly denotes we need to add more cases in the `Removing` state.
   
   4. That looks reasonable to me.
   In case of any failures, it should be piped to the `Removing` state and handled there.
   
   In the following codes,
   
   ```scala
       val unpause = stateName match {
         case Paused => container.resume()(TransactionId.invokerNanny)
         case _      => Future.successful(())
       }
   
       unpause
         .flatMap(_ => container.destroy()(TransactionId.invokerNanny))
         .flatMap(_ => abortProcess)
         .map(_ => ContainerRemoved(replacePrewarm))
         .pipeTo(self)
   ```
   
   `container.destory()` is suspicious as `container.resume()` always return `Future.successful({})`.
   
   5. Eagerly pulling a message.
   Are you concerning about the message loss by pulling it too eagerly and keep it in memory?
   Or are there any other downsides from eagerly pulling it?
   
   
   I believe @tysonnorris has better understanding on those codes.
   


----------------------------------------------------------------
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