honeycombio / helm-charts

Helm repository and charts for Honeycomb
Apache License 2.0
30 stars 39 forks source link

feat: Create refinery headless service. #337

Closed gjacquet closed 9 months ago

gjacquet commented 10 months ago

Which problem is this PR solving?

We have noticed that all our otel collectors tend to send spans to only a single refinery instance. This typically occurs after a refinery rolling restart where otel collector tends to establish connection to a single pod, usually one of the first one that was created during the restart.

Short description of the changes

Allow creating a headless service for refinery.

How to verify that this has the expected result

Set headlessService.enabled: true and check headless service was created.

gjacquet commented 9 months ago

@TylerHelmuth that would be more a question for you with your knowledge of all the use cases for refinery.

I have seen different charts maintainer go different routes. E.g Bitnami's Thanos chart has a whole set of services to accommodate all possible use cases within a single deployment: https://github.com/bitnami/charts/tree/main/bitnami/thanos/templates/query, on the other hand, otel collector has only a single service that can optinally be headless: https://github.com/open-telemetry/opentelemetry-helm-charts/blob/main/charts/opentelemetry-collector/templates/service.yaml#L19.

The driving factor would be how many different client you expect and how they expect load balancing to be handled.

If there will for sure be only one client type, e.g. all client side load balanced grpc, then single service is good. If you expect a more heterogeneous setup with a mix client side load balanced clients for gprc and kube proxy balanced http clients then it would be better to support both cluster IP and headless.

TylerHelmuth commented 9 months ago

For now, since the Refinery deployment options aren't very complex, I think a single service with some configuration options is the best play. Let's implement something similar to the OpenTelemetry Collector chart where ClusterIp is a configurable option on the service section.

gjacquet commented 9 months ago

removed the extra service and added support for the cluster IP under the existing service config