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 2022/09/20 23:31:31 UTC

[GitHub] [openwhisk] bdoyle0182 opened a new pull request, #5326: Fix Orphaned Container Edge Case In Paused State of Container Proxy

bdoyle0182 opened a new pull request, #5326:
URL: https://github.com/apache/openwhisk/pull/5326

   ## Description
   You can read a more detailed series of events to hit this case in the corresponding issue, but here's the tldr:
   
   1. Container doesn't have activations so transitions to pause container.
   2. Container times out once paused and is ready to be deleted.
   3. In order to delete once paused, a check is required for the count of containers to determine whether it should delete.
   4. The etcd request fails and the failed future is piped to the fsm. The paused state doesn't handle this message type so it stashes it until a state transition and the container proxy will sit in this corrupted state until a new activation is received.
   5. New activation is received and the container is attempted to be unpaused and the fsm transitions back to Running while it waits for the unpause future to complete.
   6. When the fsm transitions, it unstashes the failed future message from 4. which is handled in the Running state and goes to destroy the container.
   7. The container is destroyed, but the unpause future from 5. succeeds which has a side effect to rewrite the container key to etcd and this key is now orphaned forever since the container was actually destroyed.
   8. The scheduler sees the container from the watch endpoint it's listening to and now the queue for the action is stuck thinking one container exists forever that actually doesn't exist. If activations are infrequent enough that only one container is needed by the scheduling decision maker, then this action can never be run unless the system is restarted.
   
   I've reproduced this case in my test environment many times and this change now handles everything gracefully. And added a unit test to simulate this case to verify the container proxy is gracefully torn down after a failed request to etcd.
   
   ## Related issue and scope
   - [X] I opened an issue to propose and discuss this change (#5325)
   
   ## My changes affect the following components
   - [ ] API
   - [ ] Controller
   - [ ] Message Bus (e.g., Kafka)
   - [ ] Loadbalancer
   - [ ] Scheduler
   - [X] 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 :).
   - [X] 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 merged pull request #5326: Fix Orphaned Container Edge Case In Paused State of Container Proxy

Posted by GitBox <gi...@apache.org>.
bdoyle0182 merged PR #5326:
URL: https://github.com/apache/openwhisk/pull/5326


-- 
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 #5326: Fix Orphaned Container Edge Case In Paused State of Container Proxy

Posted by GitBox <gi...@apache.org>.
bdoyle0182 commented on code in PR #5326:
URL: https://github.com/apache/openwhisk/pull/5326#discussion_r978183514


##########
core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/v2/FunctionPullingContainerProxy.scala:
##########
@@ -732,7 +735,12 @@ class FunctionPullingContainerProxy(
         data.action.fullyQualifiedName(false),
         data.action.rev,
         Some(data.clientProxy))
-
+    case Event(t: FailureMessage, data: WarmData) =>
+      logging.error(
+        this,
+        s"Failed to determine whether to keep or remove container on pause timeout for ${data.container.containerId}, retrying. Caused by: $t")
+      startSingleTimer(DetermineKeepContainer.toString, DetermineKeepContainer, 1.second)

Review Comment:
   I just changed it to retry a max of 5 times and added a test that the container deletion still occurs gracefully if the retries are exhausted. Been running this code for a few days and haven't had an issue so 100% sure now this fixes the issue. If it looks good to you now, I'll merge in the morning.



-- 
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 #5326: Fix Orphaned Container Edge Case In Paused State of Container Proxy

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #5326:
URL: https://github.com/apache/openwhisk/pull/5326#issuecomment-1253187615

   # [Codecov](https://codecov.io/gh/apache/openwhisk/pull/5326?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 [#5326](https://codecov.io/gh/apache/openwhisk/pull/5326?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (0282cab) into [master](https://codecov.io/gh/apache/openwhisk/commit/a1639f0e4d7270c9a230190ac26acb61413b6bbb?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (a1639f0) will **decrease** coverage by `76.75%`.
   > The diff coverage is `0.00%`.
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #5326       +/-   ##
   ==========================================
   - Coverage   81.31%   4.55%   -76.76%     
   ==========================================
     Files         239     239               
     Lines       14249   14258        +9     
     Branches      602     590       -12     
   ==========================================
   - Hits        11587     650    -10937     
   - Misses       2662   13608    +10946     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/openwhisk/pull/5326?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/loadBalancer/ShardingContainerPoolBalancer.scala](https://codecov.io/gh/apache/openwhisk/pull/5326/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29yZS9jb250cm9sbGVyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL2NvcmUvbG9hZEJhbGFuY2VyL1NoYXJkaW5nQ29udGFpbmVyUG9vbEJhbGFuY2VyLnNjYWxh) | `0.00% <0.00%> (-89.27%)` | :arrow_down: |
   | [...ntainerpool/v2/FunctionPullingContainerProxy.scala](https://codecov.io/gh/apache/openwhisk/pull/5326/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29yZS9pbnZva2VyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL2NvcmUvY29udGFpbmVycG9vbC92Mi9GdW5jdGlvblB1bGxpbmdDb250YWluZXJQcm94eS5zY2FsYQ==) | `0.00% <0.00%> (-77.99%)` | :arrow_down: |
   | [.../main/scala/org/apache/openwhisk/core/WarmUp.scala](https://codecov.io/gh/apache/openwhisk/pull/5326/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29tbW9uL3NjYWxhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL2NvcmUvV2FybVVwLnNjYWxh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...ain/scala/org/apache/openwhisk/spi/SpiLoader.scala](https://codecov.io/gh/apache/openwhisk/pull/5326/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29tbW9uL3NjYWxhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL3NwaS9TcGlMb2FkZXIuc2NhbGE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...n/scala/org/apache/openwhisk/utils/JsHelpers.scala](https://codecov.io/gh/apache/openwhisk/pull/5326/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29tbW9uL3NjYWxhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL3V0aWxzL0pzSGVscGVycy5zY2FsYQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...scala/org/apache/openwhisk/common/Prometheus.scala](https://codecov.io/gh/apache/openwhisk/pull/5326/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29tbW9uL3NjYWxhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL2NvbW1vbi9Qcm9tZXRoZXVzLnNjYWxh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...scala/org/apache/openwhisk/common/time/Clock.scala](https://codecov.io/gh/apache/openwhisk/pull/5326/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29tbW9uL3NjYWxhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL2NvbW1vbi90aW1lL0Nsb2NrLnNjYWxh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...scala/org/apache/openwhisk/core/FeatureFlags.scala](https://codecov.io/gh/apache/openwhisk/pull/5326/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29tbW9uL3NjYWxhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL2NvcmUvRmVhdHVyZUZsYWdzLnNjYWxh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...ala/org/apache/openwhisk/common/ConfigMXBean.scala](https://codecov.io/gh/apache/openwhisk/pull/5326/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29tbW9uL3NjYWxhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL2NvbW1vbi9Db25maWdNWEJlYW4uc2NhbGE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...la/org/apache/openwhisk/http/BasicRasService.scala](https://codecov.io/gh/apache/openwhisk/pull/5326/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29tbW9uL3NjYWxhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL2h0dHAvQmFzaWNSYXNTZXJ2aWNlLnNjYWxh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | ... and [205 more](https://codecov.io/gh/apache/openwhisk/pull/5326/diff?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] style95 commented on a diff in pull request #5326: Fix Orphaned Container Edge Case In Paused State of Container Proxy

Posted by GitBox <gi...@apache.org>.
style95 commented on code in PR #5326:
URL: https://github.com/apache/openwhisk/pull/5326#discussion_r977241514


##########
core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/v2/FunctionPullingContainerProxy.scala:
##########
@@ -732,7 +735,12 @@ class FunctionPullingContainerProxy(
         data.action.fullyQualifiedName(false),
         data.action.rev,
         Some(data.clientProxy))
-
+    case Event(t: FailureMessage, data: WarmData) =>
+      logging.error(
+        this,
+        s"Failed to determine whether to keep or remove container on pause timeout for ${data.container.containerId}, retrying. Caused by: $t")
+      startSingleTimer(DetermineKeepContainer.toString, DetermineKeepContainer, 1.second)

Review Comment:
   ok, I got confused and got another question.
   If there is a problem in a connection with ETCD so `getLiveContainerCount` keeps failing, then isn't this container proxy run forever until the ETCD connection is restored?



-- 
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 #5326: Fix Orphaned Container Edge Case In Paused State of Container Proxy

Posted by GitBox <gi...@apache.org>.
bdoyle0182 commented on code in PR #5326:
URL: https://github.com/apache/openwhisk/pull/5326#discussion_r976738766


##########
tests/src/test/scala/org/apache/openwhisk/core/containerpool/v2/test/FunctionPullingContainerProxyTests.scala:
##########
@@ -1036,6 +1048,84 @@ class FunctionPullingContainerProxyTests
     }
   }
 
+  it should "destroy container proxy when stopping due to timeout and getting live count fails" in within(timeout) {
+    val authStore = mock[ArtifactWhiskAuthStore]
+    val namespaceBlacklist: NamespaceBlacklist = new NamespaceBlacklist(authStore)
+    val get = getWhiskAction(Future(action.toWhiskAction))
+    val dataManagementService = TestProbe()
+    val container = new TestContainer
+    val factory = createFactory(Future.successful(container))
+    val acker = createAcker()
+    val store = createStore
+    val collector = createCollector()
+    val counter = getLiveContainerCountFailFirstCall(2)
+    val limit = getWarmedContainerLimit(Future.successful((1, 10.seconds)))
+    val (client, clientFactory) = testClient
+
+    val probe = TestProbe()
+    val machine =
+      probe.childActorOf(
+        FunctionPullingContainerProxy
+          .props(
+            factory,
+            entityStore,
+            namespaceBlacklist,
+            get,
+            dataManagementService.ref,
+            clientFactory,
+            acker,
+            store,
+            collector,
+            counter,
+            limit,
+            InvokerInstanceId(0, userMemory = defaultUserMemory),
+            invokerHealthManager.ref,
+            poolConfig,
+            timeoutConfig))
+
+    registerCallback(machine, probe)
+    probe watch machine
+
+    machine ! Initialize(invocationNamespace.asString, fqn, action, schedulerHost, rpcPort, messageTransId)
+    probe.expectMsg(Transition(machine, Uninitialized, CreatingClient))
+    client.expectMsg(StartClient)
+    client.send(machine, ClientCreationCompleted())
+
+    probe.expectMsg(Transition(machine, CreatingClient, ClientCreated))
+    expectInitialized(probe)
+    client.expectMsg(RequestActivation())
+    client.send(machine, message)
+
+    probe.expectMsg(Transition(machine, ClientCreated, Running))
+    client.expectMsg(ContainerWarmed)
+    client.expectMsgPF() {
+      case RequestActivation(Some(_), None) => true
+    }
+
+    machine ! StateTimeout
+    client.send(machine, RetryRequestActivation)
+    probe.expectMsg(Transition(machine, Running, Pausing))
+    probe.expectMsgType[ContainerIsPaused]
+    probe.expectMsg(Transition(machine, Pausing, Paused))
+
+    machine ! StateTimeout
+    client.expectMsg(StopClientProxy)
+    probe.expectMsgAllOf(ContainerRemoved(true), Transition(machine, Paused, Removing))

Review Comment:
   done



-- 
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 #5326: Fix Orphaned Container Edge Case In Paused State of Container Proxy

Posted by GitBox <gi...@apache.org>.
bdoyle0182 commented on code in PR #5326:
URL: https://github.com/apache/openwhisk/pull/5326#discussion_r975895167


##########
core/controller/src/main/scala/org/apache/openwhisk/core/loadBalancer/ShardingContainerPoolBalancer.scala:
##########
@@ -158,7 +158,7 @@ class ShardingContainerPoolBalancer(
     AkkaManagement(actorSystem).start()
     ClusterBootstrap(actorSystem).start()
     Some(Cluster(actorSystem))
-  } else if (loadConfigOrThrow[Seq[String]]("akka.cluster.seed-nodes").nonEmpty) {  
+  } else if (loadConfigOrThrow[Seq[String]]("akka.cluster.seed-nodes").nonEmpty) {

Review Comment:
   I think this is just a missed scalafmt



-- 
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] style95 commented on a diff in pull request #5326: Fix Orphaned Container Edge Case In Paused State of Container Proxy

Posted by GitBox <gi...@apache.org>.
style95 commented on code in PR #5326:
URL: https://github.com/apache/openwhisk/pull/5326#discussion_r977263889


##########
core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/v2/FunctionPullingContainerProxy.scala:
##########
@@ -732,7 +735,12 @@ class FunctionPullingContainerProxy(
         data.action.fullyQualifiedName(false),
         data.action.rev,
         Some(data.clientProxy))
-
+    case Event(t: FailureMessage, data: WarmData) =>
+      logging.error(
+        this,
+        s"Failed to determine whether to keep or remove container on pause timeout for ${data.container.containerId}, retrying. Caused by: $t")
+      startSingleTimer(DetermineKeepContainer.toString, DetermineKeepContainer, 1.second)

Review Comment:
   Then how about introducing a maximum retry value?
   After retrying x times to get the container count, if it fails, we can remove the container.
   I feel it would be meaningless to keep the problematic containers for the keepingWarm count if the issue persists.



-- 
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] style95 commented on a diff in pull request #5326: Fix Orphaned Container Edge Case In Paused State of Container Proxy

Posted by GitBox <gi...@apache.org>.
style95 commented on code in PR #5326:
URL: https://github.com/apache/openwhisk/pull/5326#discussion_r976038644


##########
tests/src/test/scala/org/apache/openwhisk/core/containerpool/v2/test/FunctionPullingContainerProxyTests.scala:
##########
@@ -1036,6 +1048,84 @@ class FunctionPullingContainerProxyTests
     }
   }
 
+  it should "destroy container proxy when stopping due to timeout and getting live count fails" in within(timeout) {
+    val authStore = mock[ArtifactWhiskAuthStore]
+    val namespaceBlacklist: NamespaceBlacklist = new NamespaceBlacklist(authStore)
+    val get = getWhiskAction(Future(action.toWhiskAction))
+    val dataManagementService = TestProbe()
+    val container = new TestContainer
+    val factory = createFactory(Future.successful(container))
+    val acker = createAcker()
+    val store = createStore
+    val collector = createCollector()
+    val counter = getLiveContainerCountFailFirstCall(2)

Review Comment:
   👍 



##########
tests/src/test/scala/org/apache/openwhisk/core/containerpool/v2/test/FunctionPullingContainerProxyTests.scala:
##########
@@ -1036,6 +1048,84 @@ class FunctionPullingContainerProxyTests
     }
   }
 
+  it should "destroy container proxy when stopping due to timeout and getting live count fails" in within(timeout) {
+    val authStore = mock[ArtifactWhiskAuthStore]
+    val namespaceBlacklist: NamespaceBlacklist = new NamespaceBlacklist(authStore)
+    val get = getWhiskAction(Future(action.toWhiskAction))
+    val dataManagementService = TestProbe()
+    val container = new TestContainer
+    val factory = createFactory(Future.successful(container))
+    val acker = createAcker()
+    val store = createStore
+    val collector = createCollector()
+    val counter = getLiveContainerCountFailFirstCall(2)
+    val limit = getWarmedContainerLimit(Future.successful((1, 10.seconds)))
+    val (client, clientFactory) = testClient
+
+    val probe = TestProbe()
+    val machine =
+      probe.childActorOf(
+        FunctionPullingContainerProxy
+          .props(
+            factory,
+            entityStore,
+            namespaceBlacklist,
+            get,
+            dataManagementService.ref,
+            clientFactory,
+            acker,
+            store,
+            collector,
+            counter,
+            limit,
+            InvokerInstanceId(0, userMemory = defaultUserMemory),
+            invokerHealthManager.ref,
+            poolConfig,
+            timeoutConfig))
+
+    registerCallback(machine, probe)
+    probe watch machine
+
+    machine ! Initialize(invocationNamespace.asString, fqn, action, schedulerHost, rpcPort, messageTransId)
+    probe.expectMsg(Transition(machine, Uninitialized, CreatingClient))
+    client.expectMsg(StartClient)
+    client.send(machine, ClientCreationCompleted())
+
+    probe.expectMsg(Transition(machine, CreatingClient, ClientCreated))
+    expectInitialized(probe)
+    client.expectMsg(RequestActivation())
+    client.send(machine, message)
+
+    probe.expectMsg(Transition(machine, ClientCreated, Running))
+    client.expectMsg(ContainerWarmed)
+    client.expectMsgPF() {
+      case RequestActivation(Some(_), None) => true
+    }
+
+    machine ! StateTimeout
+    client.send(machine, RetryRequestActivation)
+    probe.expectMsg(Transition(machine, Running, Pausing))
+    probe.expectMsgType[ContainerIsPaused]
+    probe.expectMsg(Transition(machine, Pausing, Paused))
+
+    machine ! StateTimeout
+    client.expectMsg(StopClientProxy)
+    probe.expectMsgAllOf(ContainerRemoved(true), Transition(machine, Paused, Removing))

Review Comment:
   Can we add a check to see if `DataManagementService` properly receives the clean-up messages?



##########
core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/v2/FunctionPullingContainerProxy.scala:
##########
@@ -732,7 +735,12 @@ class FunctionPullingContainerProxy(
         data.action.fullyQualifiedName(false),
         data.action.rev,
         Some(data.clientProxy))
-
+    case Event(t: FailureMessage, data: WarmData) =>
+      logging.error(
+        this,
+        s"Failed to determine whether to keep or remove container on pause timeout for ${data.container.containerId}, retrying. Caused by: $t")
+      startSingleTimer(DetermineKeepContainer.toString, DetermineKeepContainer, 1.second)

Review Comment:
   Is there any reason to start the timer after 1 second?
   Since it will delay the deletion of the ETCD key for the problematic container, another request heading to this container can still come during that time.
   The request will be rescheduled, but it will anyway also delay container creation.
   



-- 
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 #5326: Fix Orphaned Container Edge Case In Paused State of Container Proxy

Posted by GitBox <gi...@apache.org>.
bdoyle0182 commented on code in PR #5326:
URL: https://github.com/apache/openwhisk/pull/5326#discussion_r977249445


##########
core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/v2/FunctionPullingContainerProxy.scala:
##########
@@ -732,7 +735,12 @@ class FunctionPullingContainerProxy(
         data.action.fullyQualifiedName(false),
         data.action.rev,
         Some(data.clientProxy))
-
+    case Event(t: FailureMessage, data: WarmData) =>
+      logging.error(
+        this,
+        s"Failed to determine whether to keep or remove container on pause timeout for ${data.container.containerId}, retrying. Caused by: $t")
+      startSingleTimer(DetermineKeepContainer.toString, DetermineKeepContainer, 1.second)

Review Comment:
   yes that's correct that's the current behavior I've set up. I guess in theory you could remove the container if the request fails, but you'd then break the guarantee of the keepingWarm count for the namespace



-- 
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 #5326: Fix Orphaned Container Edge Case In Paused State of Container Proxy

Posted by GitBox <gi...@apache.org>.
bdoyle0182 commented on code in PR #5326:
URL: https://github.com/apache/openwhisk/pull/5326#discussion_r976699995


##########
core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/v2/FunctionPullingContainerProxy.scala:
##########
@@ -732,7 +735,12 @@ class FunctionPullingContainerProxy(
         data.action.fullyQualifiedName(false),
         data.action.rev,
         Some(data.clientProxy))
-
+    case Event(t: FailureMessage, data: WarmData) =>
+      logging.error(
+        this,
+        s"Failed to determine whether to keep or remove container on pause timeout for ${data.container.containerId}, retrying. Caused by: $t")
+      startSingleTimer(DetermineKeepContainer.toString, DetermineKeepContainer, 1.second)

Review Comment:
   Hmm I think that shouldn't be the case. If a request comes in for the container, it will receive the `Initialize` message to unpause the container and go back to running. The retry message for `DetermineKeepContainer` after one second is cancelled at the beginning of the `Initialize` event along with the other timers so it should just gracefully unpause and go back to running so the latency is the duration of a normal unpause operation.



-- 
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 #5326: Fix Orphaned Container Edge Case In Paused State of Container Proxy

Posted by GitBox <gi...@apache.org>.
bdoyle0182 commented on code in PR #5326:
URL: https://github.com/apache/openwhisk/pull/5326#discussion_r976743330


##########
core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/v2/FunctionPullingContainerProxy.scala:
##########
@@ -732,7 +735,12 @@ class FunctionPullingContainerProxy(
         data.action.fullyQualifiedName(false),
         data.action.rev,
         Some(data.clientProxy))
-
+    case Event(t: FailureMessage, data: WarmData) =>
+      logging.error(
+        this,
+        s"Failed to determine whether to keep or remove container on pause timeout for ${data.container.containerId}, retrying. Caused by: $t")
+      startSingleTimer(DetermineKeepContainer.toString, DetermineKeepContainer, 1.second)

Review Comment:
   And if for whatever reason a new request comes in and it can't unpause the container because the container is now broken for whatever reason, the activation will get rescheduled so there would be latency for that case which is already possible on any broken paused container but it's not any additional latency from the 1 second retry and the failure case of unpausing will be graceful now to correctly delete the broken container in all cases



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