[HELM] Fix Kubernetes Routing Issue #13450

piby180 · 2024-06-20T17:30:34Z

This PR aims to partially solve the issue mentioned here

With publishNotReadyAddresses set to true in service resource, kubernetes will publish the DNS address irrespective of whether the pod is in Ready state or not. Then the sole responsibility of when to start making requests to the newly restarted server/broker lies on Pinot.

The following error will be fixed with this PR

Unable to resolve host pinot-server-2.pinot-server-headless.pinot-dev.svc.cluster.local
or 
Unable to resolve host pinot-broker-2.pinot-server-headless.pinot-dev.svc.cluster.local

abhioncbr · 2024-06-20T20:13:28Z

Does it mean that with this change, pods are going to get the request before they get into Ready Stage?

piby180 · 2024-06-20T20:44:09Z

Currently, Pinot assumes that the server is online as soon as all the segments have been made online on zookeeper and it starts to send the requests to the newly restarted server immediately.
Kubernetes however marks the server pod as "Ready" only when the first readiness probe is succeeded.
Since pinot makes request to headless service, in order for the request to resolve, the DNS for each server is needed to be resolved. So all the queries made to Pinot between "the time the server is online as per zookeeper" and "the time kubernetes mark it as Ready" fails with error.

Unable to resolve host pinot-server-2.pinot-server-headless.pinot-dev.svc.cluster.local
or 
Unable to resolve host pinot-broker-2.pinot-server-headless.pinot-dev.svc.cluster.local

This causes downtime in Pinot whenver a pod (server or broker) is restarted. This can be seconds or minutes depending on how the probes are configured.

With this change, the DNS for each server will always resolve irrespective of whether the pod is in "Ready" state or not. Pinot will however only make requests to the server when it is online as per Zookeeper.

abhioncbr · 2024-06-20T21:22:09Z

Currently, Pinot assumes that the server is online as soon as all the segments have been made online on zookeeper and it starts to send the requests to the newly restarted server immediately.

Kubernetes however marks the server pod as "Ready" only when the first readiness probe is succeeded.

Since pinot makes request to headless service, in order for the request to resolve, the DNS for each server is needed to be resolved. So all the queries made to Pinot between "the time the server is online as per zookeeper" and "the time kubernetes mark it as Ready" fails with error.
Unable to resolve host pinot-server-2.pinot-server-headless.pinot-dev.svc.cluster.local
or 
Unable to resolve host pinot-broker-2.pinot-server-headless.pinot-dev.svc.cluster.local
This causes downtime in Pinot whenver a pod (server or broker) is restarted. This can be seconds or minutes depending on how the probes are configured.

With this change, the DNS for each server will always resolve irrespective of whether the pod is in "Ready" state or not. Pinot will however only make requests to the server when it is online as per Zookeeper.

Thanks for the explanation. I just checked, and the zookeeper is doing the same for the headless service.
Question: Should it be added only in headless services or in all services?

piby180 · 2024-06-20T21:41:06Z

Currently, Pinot assumes that the server is online as soon as all the segments have been made online on zookeeper and it starts to send the requests to the newly restarted server immediately.

Kubernetes however marks the server pod as "Ready" only when the first readiness probe is succeeded.

Since pinot makes request to headless service, in order for the request to resolve, the DNS for each server is needed to be resolved. So all the queries made to Pinot between "the time the server is online as per zookeeper" and "the time kubernetes mark it as Ready" fails with error.
Unable to resolve host pinot-server-2.pinot-server-headless.pinot-dev.svc.cluster.local
or 
Unable to resolve host pinot-broker-2.pinot-server-headless.pinot-dev.svc.cluster.local
This causes downtime in Pinot whenver a pod (server or broker) is restarted. This can be seconds or minutes depending on how the probes are configured.

With this change, the DNS for each server will always resolve irrespective of whether the pod is in "Ready" state or not. Pinot will however only make requests to the server when it is online as per Zookeeper.
Thanks for the explanation. I just checked, and the zookeeper is doing the same for the headless service. Question: Should it be added only in headless services or in all services?

Change to just headless services is sufficient. This has been tested on our cluster.
The "normal" service endpoints are being used by ingress resource.

If you agree, I can remove this change from "headful" services.

abhioncbr · 2024-06-21T00:07:37Z

Currently, Pinot assumes that the server is online as soon as all the segments have been made online on zookeeper and it starts to send the requests to the newly restarted server immediately.

Kubernetes however marks the server pod as "Ready" only when the first readiness probe is succeeded.

Since pinot makes request to headless service, in order for the request to resolve, the DNS for each server is needed to be resolved. So all the queries made to Pinot between "the time the server is online as per zookeeper" and "the time kubernetes mark it as Ready" fails with error.
Unable to resolve host pinot-server-2.pinot-server-headless.pinot-dev.svc.cluster.local
or 
Unable to resolve host pinot-broker-2.pinot-server-headless.pinot-dev.svc.cluster.local
This causes downtime in Pinot whenver a pod (server or broker) is restarted. This can be seconds or minutes depending on how the probes are configured.

With this change, the DNS for each server will always resolve irrespective of whether the pod is in "Ready" state or not. Pinot will however only make requests to the server when it is online as per Zookeeper.
Thanks for the explanation. I just checked, and the zookeeper is doing the same for the headless service. Question: Should it be added only in headless services or in all services?
Change to just headless services is sufficient. This has been tested on our cluster. The "normal" service endpoints are being used by ingress resource.

If you agree, I can remove this change from "headful" services.

In my opinion, it should be only in headless svc, but I suggest we wait for others' input as well.

zhtaoxiang

LGTM

soumitra-st

xiangfu0 · 2024-08-16T18:37:14Z

helm/pinot/templates/broker/service-external.yaml

@@ -29,6 +29,7 @@ metadata:
    {{- include "pinot.brokerLabels" . | nindent 4 }}
 spec:
  type: {{ .Values.broker.external.type }}
+  publishNotReadyAddresses: true


let's make default external to false here.

xiangfu0 · 2024-08-16T18:38:02Z

I think we should only make default value for service-headless and server service to true, all others to false.

xiangfu0 · 2024-08-16T18:40:37Z

Pinot handles all the internal readiness for routing request to servers, so it's ok to expose all the server/minion ips once the pod is up but not ready.
For controller and broker, they all start up pretty fast and you don't want them to get request until up, so it's ok to wait until health check finishing to mark them online.

xiangfu0 · 2024-08-16T18:40:53Z

helm/pinot/templates/broker/service.yaml

@@ -27,6 +27,7 @@ metadata:
    {{- include "pinot.brokerLabels" . | nindent 4 }}
 spec:
  type: {{ .Values.broker.service.type }}
+  publishNotReadyAddresses: true


default to false

xiangfu0 · 2024-08-16T18:40:58Z

helm/pinot/templates/controller/service-external.yaml

@@ -29,6 +29,7 @@ metadata:
    {{- include "pinot.controllerLabels" . | nindent 4 }}
 spec:
  type: {{ .Values.controller.external.type }}
+  publishNotReadyAddresses: true


default to false

xiangfu0 · 2024-08-16T18:41:04Z

helm/pinot/templates/controller/service.yaml

@@ -27,6 +27,7 @@ metadata:
    {{- include "pinot.controllerLabels" . | nindent 4 }}
 spec:
  type: {{ .Values.controller.service.type }}
+  publishNotReadyAddresses: true


default to false

codecov-commenter · 2024-08-17T07:52:21Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 64.00%. Comparing base (59551e4) to head (398a40a).
Report is 1094 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #13450      +/-   ##
============================================
+ Coverage     61.75%   64.00%   +2.25%     
- Complexity      207     1539    +1332     
============================================
  Files          2436     2596     +160     
  Lines        133233   143295   +10062     
  Branches      20636    21948    +1312     
============================================
+ Hits          82274    91718    +9444     
+ Misses        44911    44833      -78     
- Partials       6048     6744     +696

Flag	Coverage Δ
custom-integration1	`100.00% <ø> (+99.99%)`	⬆️
integration	`100.00% <ø> (+99.99%)`	⬆️
integration1	`100.00% <ø> (+99.99%)`	⬆️
integration2	`0.00% <ø> (ø)`
java-11	`63.95% <ø> (+2.24%)`	⬆️
java-21	`63.90% <ø> (+2.27%)`	⬆️
skip-bytebuffers-false	`63.97% <ø> (+2.22%)`	⬆️
skip-bytebuffers-true	`63.88% <ø> (+36.15%)`	⬆️
temurin	`64.00% <ø> (+2.25%)`	⬆️
unittests	`64.00% <ø> (+2.25%)`	⬆️
unittests1	`55.61% <ø> (+8.71%)`	⬆️
unittests2	`34.47% <ø> (+6.74%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

piby180 · 2024-09-26T11:35:47Z

@xiangfu0 Can we merge this?

Fix Kubernetes Routing Issue

5cde9b6

abhioncbr added helm kubernetes labels Jun 20, 2024

abhioncbr requested a review from xiangfu0 June 21, 2024 00:07

AustinRP-Primer approved these changes Aug 13, 2024

View reviewed changes

zhtaoxiang approved these changes Aug 16, 2024

View reviewed changes

xiangfu0 approved these changes Aug 16, 2024

View reviewed changes

soumitra-st approved these changes Aug 16, 2024

View reviewed changes

xiangfu0 reviewed Aug 16, 2024

View reviewed changes

Setting default to false for non headless services

f070ae5

Merge branch 'apache:master' into fix_kubernetes_routing_issue

398a40a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HELM] Fix Kubernetes Routing Issue #13450

[HELM] Fix Kubernetes Routing Issue #13450

piby180 commented Jun 20, 2024

abhioncbr commented Jun 20, 2024

piby180 commented Jun 20, 2024

abhioncbr commented Jun 20, 2024

piby180 commented Jun 20, 2024

abhioncbr commented Jun 21, 2024

zhtaoxiang left a comment

soumitra-st left a comment

xiangfu0 Aug 16, 2024

piby180 Aug 16, 2024

xiangfu0 commented Aug 16, 2024 •

edited

Loading

xiangfu0 commented Aug 16, 2024 •

edited

Loading

xiangfu0 Aug 16, 2024

piby180 Aug 16, 2024

xiangfu0 Aug 16, 2024

piby180 Aug 16, 2024

xiangfu0 Aug 16, 2024

piby180 Aug 16, 2024

codecov-commenter commented Aug 17, 2024 •

edited

Loading

piby180 commented Sep 26, 2024

[HELM] Fix Kubernetes Routing Issue #13450

Are you sure you want to change the base?

[HELM] Fix Kubernetes Routing Issue #13450

Conversation

piby180 commented Jun 20, 2024

abhioncbr commented Jun 20, 2024

piby180 commented Jun 20, 2024

abhioncbr commented Jun 20, 2024

piby180 commented Jun 20, 2024

abhioncbr commented Jun 21, 2024

zhtaoxiang left a comment

Choose a reason for hiding this comment

soumitra-st left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiangfu0 commented Aug 16, 2024 • edited Loading

xiangfu0 commented Aug 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Aug 17, 2024 • edited Loading

Codecov Report

piby180 commented Sep 26, 2024

xiangfu0 commented Aug 16, 2024 •

edited

Loading

xiangfu0 commented Aug 16, 2024 •

edited

Loading

codecov-commenter commented Aug 17, 2024 •

edited

Loading