You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@openwhisk.apache.org by "bdoyle0182 (via GitHub)" <gi...@apache.org> on 2023/03/25 19:29:35 UTC
[GitHub] [openwhisk] bdoyle0182 opened a new pull request, #5388: fix memory queue stuck in removed state edge case
bdoyle0182 opened a new pull request, #5388:
URL: https://github.com/apache/openwhisk/pull/5388
## Description
There is an edge case in the queue manager / memory queue where when the memory queue transitions from Idle to Removed, the action can get into an indefinite stuck state if it starts receiving activations again. This is because when transitioning from Idle to Removed, the `QueueRemoved` message to be sent from the MemoryQueue -> QueueManager is never sent which will remove the entry of the actor from the `QueuePool` trie map in the QueueManager. If the entry remains in `QueuePool`, the manager can still forward activations to the child memory queue fsm. The QueueManager receiving `QueueRemoved` is also responsible for sending `QueueRemovedCompleted` back to the MemoryQueue fsm which is what will actually make the memory queue stop itself. Since this series of events will never occur when transitioning from Idle to Removed, the Removed state thus becomes dependent on the `StateTimeout` to occur to actually stop the actor which _then_ will send the `QueueRemoved` message to the pare
nt to have it removed from the `QueuePool`. It's important to note that the whole etcd series of events to remove the queue keys are successful and the queue manager acknowledges it with the `WatchEndpointRemoved`, but that does not remove the entry from `QueuePool`.
Here is where things get catastrophic. Since the fsm actor entry will remain in the `QueuePool` trie map and the memory queue never sends the QueueRemoved message, the activations will continue sending to the memory queue IF a new activation comes in the five second window of the default configuration to time out the removed state. Akka fsm state timeouts work such that the timeout message is only sent IF no message is received otherwise the timer is reset every time a new message is received. The case in the removed state if receiving an activation will forward it back to the queue manager which will just send it back to the memory queue creating an indefinite cycle until the activation times out. Now if the action has a multi minute gap from having activations it will self heal because after the activation times out and no more come in, then it will self heal after five seconds. However, if that never happens the action will remain in a stuck state never executing activations un
til the service is restarted. This makes this bug particularly hard to track down because sometimes it could self heal and sometimes it remains stuck forever.
This StateTimeout behavior resetting the timer on each new message is already actually accounted for in the `Flushing` state so I've just added the same safe guard here to guarantee self recovery and that the queue will definitely shut down correctly after the stop grace time. However that's just a safeguard, I think there is still a remaining issue to solve that `QueueRemoved` needs to be sent to the `QueueManager` somewhere on transition from `Idle` to `Removed`, not only once the `Removed` state times out; otherwise transition from `Idle` -> `Removed` will always rely on the StateTimeout for the fsm to stop itself.
## Related issue and scope
- [ ] I opened an issue to propose and discuss this change (#????)
## My changes affect the following components
- [ ] API
- [ ] Controller
- [ ] Message Bus (e.g., Kafka)
- [ ] Loadbalancer
- [X] Scheduler
- [ ] Invoker
- [ ] Intrinsic actions (e.g., sequences, conductors)
- [ ] Data stores (e.g., CouchDB)
- [ ] Tests
- [ ] Deployment
- [ ] CLI
- [ ] General tooling
- [ ] Documentation
## Types of changes
- [X] Bug fix (generally a non-breaking change which closes an issue).
- [ ] Enhancement or new feature (adds new functionality).
- [ ] Breaking change (a bug fix or enhancement which changes existing behavior).
## Checklist:
- [X] I signed an [Apache CLA](https://github.com/apache/openwhisk/blob/master/CONTRIBUTING.md).
- [X] I reviewed the [style guides](https://github.com/apache/openwhisk/blob/master/CONTRIBUTING.md#coding-standards) and followed the recommendations (Travis CI will check :).
- [ ] I added tests to cover my changes.
- [ ] My changes require further changes to the documentation.
- [ ] I updated the documentation where necessary.
--
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-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] bdoyle0182 commented on a diff in pull request #5388: fix memory queue stuck in removed state edge case
Posted by "bdoyle0182 (via GitHub)" <gi...@apache.org>.
bdoyle0182 commented on code in PR #5388:
URL: https://github.com/apache/openwhisk/pull/5388#discussion_r1148421740
##########
core/scheduler/src/main/scala/org/apache/openwhisk/core/scheduler/queue/MemoryQueue.scala:
##########
@@ -456,6 +456,9 @@ class MemoryQueue(private val etcdClient: EtcdClient,
// This is not supposed to happen. This will ensure the queue does not run forever.
Review Comment:
@style95 I don't think this is actually true since there is no `context.parent ! queueRemovedMsg` when the `Idle` state times out which is fairly frequent behavior and thus `QueueRemoved` is never sent to the manager until `Removed` state times out which wasn't guaranteed.
--
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-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] codecov-commenter commented on pull request #5388: fix memory queue stuck in removed state edge case
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #5388:
URL: https://github.com/apache/openwhisk/pull/5388#issuecomment-1483909189
## [Codecov](https://codecov.io/gh/apache/openwhisk/pull/5388?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#5388](https://codecov.io/gh/apache/openwhisk/pull/5388?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6747370) into [master](https://codecov.io/gh/apache/openwhisk/commit/60ca6605bb081f99906cff1a21caf75d47e414fa?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (60ca660) will **decrease** coverage by `72.48%`.
> The diff coverage is `0.00%`.
> :exclamation: Current head 6747370 differs from pull request most recent head 768a3cb. Consider uploading reports for the commit 768a3cb to get more accurate results
```diff
@@ Coverage Diff @@
## master #5388 +/- ##
==========================================
- Coverage 76.91% 4.44% -72.48%
==========================================
Files 240 240
Lines 14588 14590 +2
Branches 629 624 -5
==========================================
- Hits 11221 648 -10573
- Misses 3367 13942 +10575
```
| [Impacted Files](https://codecov.io/gh/apache/openwhisk/pull/5388?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...e/openwhisk/core/scheduler/queue/MemoryQueue.scala](https://codecov.io/gh/apache/openwhisk/pull/5388?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29yZS9zY2hlZHVsZXIvc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9vcGVud2hpc2svY29yZS9zY2hlZHVsZXIvcXVldWUvTWVtb3J5UXVldWUuc2NhbGE=) | `0.00% <0.00%> (-82.41%)` | :arrow_down: |
... and [204 files with indirect coverage changes](https://codecov.io/gh/apache/openwhisk/pull/5388/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] bdoyle0182 merged pull request #5388: fix memory queue stuck in removed state edge case
Posted by "bdoyle0182 (via GitHub)" <gi...@apache.org>.
bdoyle0182 merged PR #5388:
URL: https://github.com/apache/openwhisk/pull/5388
--
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-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org