bdoyle0182 commented on code in PR #5338:
URL: https://github.com/apache/openwhisk/pull/5338#discussion_r1003587786
##########
core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/v2/ActivationClientProxy.scala:
##########
@@ -279,16 +270,24 @@ class ActivationClientProxy(
warmed = true
stay
- case Event(CloseClientProxy, c: Client) =>
- safelyCloseClient(c)
+ // When disabling an invoker, there could be still activations in the
queue.
+ // Activation client keeps fetching data and forward it to the
container(parent).
Review Comment:
```suggestion
// The activation client keeps fetching data and will forward it to the
container(parent).
```
##########
core/controller/src/main/scala/org/apache/openwhisk/core/controller/Controller.scala:
##########
@@ -175,6 +193,30 @@ class Controller(val instance: ControllerInstanceId,
LogLimit.config,
runtimes,
List(apiV1.basepath()))
+
+ private val controllerUsername =
loadConfigOrThrow[String](ConfigKeys.whiskControllerUsername)
+ private val controllerPassword =
loadConfigOrThrow[String](ConfigKeys.whiskControllerPassword)
+
+ /**
+ * disable controller
+ */
+ private def disable(implicit transid: TransactionId) = {
+ implicit val executionContext = actorSystem.dispatcher
+ implicit val jsonPrettyResponsePrinter = PrettyPrinter
+ (path("disable") & post) {
+ extractCredentials {
+ case Some(BasicHttpCredentials(username, password)) =>
+ if (username == controllerUsername && password ==
controllerPassword) {
+ loadBalancer.close
Review Comment:
This looks like it shuts down the messaging feed to receive completionAcks
from the invoker, but how does it stop the controller from receiving new
activations which will then be orphaned and never be able to respond when it
receives the completionAck after restarting.
I think it would be a good idea to have an `isEnabled` route similar to the
other services so that the external monitor that is determining whether the
service is alive to direct traffic to the node can make the decision to stop
directing traffic once disabled.
##########
core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/v2/ActivationClientProxy.scala:
##########
@@ -279,16 +270,24 @@ class ActivationClientProxy(
warmed = true
stay
- case Event(CloseClientProxy, c: Client) =>
- safelyCloseClient(c)
+ // When disabling an invoker, there could be still activations in the
queue.
+ // Activation client keeps fetching data and forward it to the
container(parent).
+ // Once it receives `NoActivationMessage` from the queue, it would close
the activation client and send `ClientClosed`
+ // to the container(parent), rather than sending `RetryRequestActivation`.
+ // When a container proxy(parent) receives `ClientClosed`, it will finally
shut down.
+ case Event(GracefulShutdown, _: Client) =>
+ logging.info(this, "safely close client proxy and go to the
ClientProxyRemoving state")
Review Comment:
let's include the container id for the three new logs for traceability.
honestly would be nice to have the container id built into the logger for
`ActivationClientProxy` but not important for this pr.
##########
core/controller/src/main/scala/org/apache/openwhisk/core/controller/Controller.scala:
##########
@@ -175,6 +193,30 @@ class Controller(val instance: ControllerInstanceId,
LogLimit.config,
runtimes,
List(apiV1.basepath()))
+
+ private val controllerUsername =
loadConfigOrThrow[String](ConfigKeys.whiskControllerUsername)
+ private val controllerPassword =
loadConfigOrThrow[String](ConfigKeys.whiskControllerPassword)
+
+ /**
+ * disable controller
+ */
+ private def disable(implicit transid: TransactionId) = {
Review Comment:
I always thought when shutting down a controller that it wouldn't stop until
all inflight activations for the controller were dropped / completed. Is that
not actually the case?
##########
core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/v2/ActivationClientProxy.scala:
##########
@@ -279,16 +270,24 @@ class ActivationClientProxy(
warmed = true
stay
- case Event(CloseClientProxy, c: Client) =>
- safelyCloseClient(c)
+ // When disabling an invoker, there could be still activations in the
queue.
+ // Activation client keeps fetching data and forward it to the
container(parent).
+ // Once it receives `NoActivationMessage` from the queue, it would close
the activation client and send `ClientClosed`
Review Comment:
```suggestion
// Once it receives `NoActivationMessage` from the queue, it will close
the activation client and send `ClientClosed`
```
##########
core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/v2/ActivationClientProxy.scala:
##########
@@ -279,16 +270,24 @@ class ActivationClientProxy(
warmed = true
stay
- case Event(CloseClientProxy, c: Client) =>
- safelyCloseClient(c)
+ // When disabling an invoker, there could be still activations in the
queue.
Review Comment:
```suggestion
// When disabling an invoker, there could still be activations in the
queue.
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
