CORE-17476 Modifying helm charts to create ClusterIP services for workers, passing to FlowWorker #4777
Conversation
| @@ -5,5 +5,5 @@ internal const val HTTP_SERVICE_UNAVAILABLE_CODE = 503 | |||
| internal const val HTTP_HEALTH_ROUTE = "/isHealthy" | |||
| internal const val HTTP_METRICS_ROUTE = "/metrics" | |||
| internal const val HTTP_STATUS_ROUTE = "/status" | |||
| internal const val WORKER_SERVER_PORT = 7000 | |||
| internal const val WORKER_SERVER_PORT = 7000 // Note: This variable is defined in charts/corda-lib/templates/_helpers.kt:622 | |||
There was a problem hiding this comment.
This is really the definition of the port. The Helm chart variable tells the rest of the world what this value is. I don't know whether this port is configurable on a worker but, if it is, the Helm chart could pass the value in and then be sure that everything ties up nicely (even if the value we pass in is still 7000!).
There was a problem hiding this comment.
It defaults to 7000, but you can override it using the -p param:
(just FYI)
There was a problem hiding this comment.
Good to know! 🤔 I'll remove the comment then, as it's not adding any clarity to the code 😅
charts/corda/templates/workers.yaml
Outdated
| {{- include "corda.worker" ( list $ .Values.workers.flow "flow" | ||
| ( dict "stateManagerDbAccess" true ) | ||
| ( dict "stateManagerDbAccess" true "additionalWorkerArgs" $args ) |
There was a problem hiding this comment.
It probably doesn't make much difference while the flow worker is the only worker doing this but I'd still be tempted to pass servicesAccessed in the dictionary and then do the processing of that within corda.worker rather than here.
charts/corda/templates/workers.yaml
Outdated
| {{- $endpoint := include "corda.getWorkerEndpoint" (dict "context" $ "worker" $worker) }} | ||
| {{- $args = append $args (printf "--endpoint=%s" $endpoint) }} |
There was a problem hiding this comment.
nit: just for readability, I'd be tempted to indent these lines to indicate that they're nested within the range (the white space being eaten by {{- anyway).
| @@ -5,5 +5,5 @@ internal const val HTTP_SERVICE_UNAVAILABLE_CODE = 503 | |||
| internal const val HTTP_HEALTH_ROUTE = "/isHealthy" | |||
| internal const val HTTP_METRICS_ROUTE = "/metrics" | |||
| internal const val HTTP_STATUS_ROUTE = "/status" | |||
| internal const val WORKER_SERVER_PORT = 7000 | |||
| internal const val WORKER_SERVER_PORT = 7000 // Note: This variable is defined in charts/corda-lib/templates/_helpers.kt:622 | |||
There was a problem hiding this comment.
It defaults to 7000, but you can override it using the -p param:
(just FYI)
| {{/* | ||
| The port which should be used to connect to Corda worker instances | ||
| */}} | ||
| {{- define "worker.port" -}} |
There was a problem hiding this comment.
Bit nitpicky (and don't propose it needs to be changed), but I guess technically this is the LB port, not the worker port?
There was a problem hiding this comment.
Technically its both the port for the worker and for the load balancer; the LB forwards port :7000 to port :7000 on the respective worker. I'm on the fence about it, but I figure worker.port makes the overall intent more clear.
There was a problem hiding this comment.
oh - but now you've highlighted it again, I realise this breaks the naming convention for functions. They are all corda. something. This is because the functions are in a global namespace when charts are combined together.
|
|
||
| @Option(names = ["--endpoint"], description = ["Internal RPC endpoints for Corda workers"], required = true) |
There was a problem hiding this comment.
Nitpick: can we update the description to HTTP endpoints instead, as that what they are?. Just trying to avoid weeks of works in the future trying to refactor from RPC to HTTP again (it already happened for the old rpc-worker)?.
…kers, passing endpoints to FlowWorker
ben-millar
force-pushed
the
bm/CORE-17476/load-balancer
branch
from
869fdac to
702ae56
Compare
ben-millar
dismissed stale reviews from jujoramos, davidcurrie, and driessamyn
via
9072689
|
Jenkins build for PR 4777 build 7 Build Successful: |
As part of the Corda topology redesign, communication between the flow engine and Corda workers is moving from a Kafka message bus to synchronous RPC calls. In order to support this, we're adding a load balancing layer (Here, a
ClusterIPservice per worker) which will expose a single endpoint which distributes calls across all worker instances.The endpoint for this ClusterIP service is now being passed into the additional startup args of the
FlowWorker; a future PR will use these to define a routing component for external messages.Related PRs: