Add Ingress to Broker and Controller helm charts #7997

dianaarnos · 2022-01-12T09:15:14Z

Description

This enables Ingress out of the box for anyone using Helm to deploy Pinot to Kubernetes.
All one need to do is override the default values inside the values.yaml file.

Ingress is disabled as default.

Important: One needs to be running Kubernetes that supports apiVersion: extensions/v1beta1.

Related issue: #7996

Sample result helm template pinot when Ingress is enabled:

# Source: pinot/templates/broker/ingress.yaml
apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  name: pinot-broker
  labels:
    helm.sh/chart: pinot-0.2.6-SNAPSHOT
    app: pinot
    chart: pinot-0.2.6-SNAPSHOT
    release: pinot
    app.kubernetes.io/version: "0.2.6-SNAPSHOT"
    app.kubernetes.io/managed-by: Helm
    heritage: Helm
    component: broker
spec:
  rules:
  - host: localhost
    http:
      paths:
        - path: /
          backend:
            serviceName: pinot-broker
            servicePort: 8099
---
# Source: pinot/templates/controller/ingress.yaml
apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  name: pinot-controller
  labels:
    helm.sh/chart: pinot-0.2.6-SNAPSHOT
    app: pinot
    chart: pinot-0.2.6-SNAPSHOT
    release: pinot
    app.kubernetes.io/version: "0.2.6-SNAPSHOT"
    app.kubernetes.io/managed-by: Helm
    heritage: Helm
    component: controller
spec:
  rules:
    - host: localhost
      http:
        paths:
          - path: /
            backend:
              serviceName: pinot-controller
              servicePort: 9000

Upgrade Notes

Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion)

Yes (Please label as backward-incompat, and complete the section below on Release Notes)

Does this PR fix a zero-downtime upgrade introduced earlier?

Yes (Please label this as backward-incompat, and complete the section below on Release Notes)

Does this PR otherwise need attention when creating release notes? Things to consider:

New configuration options
Deprecation of configurations
Signature changes to public methods/interfaces
New plugins added or old plugins removed

Yes (Please label this PR as release-notes and complete the section on Release Notes)

Release Notes

Now it is possible to enable an Ingress resource for the Broker and the Controller through the helm chart by setting it as enabled and adding the necessary configurations.

dianaarnos · 2022-01-12T09:17:30Z

cc @WyriHaximus

kubernetes/helm/pinot/Chart.yaml

kishoreg · 2022-01-13T01:53:37Z

@dianaarnos let us know when its ready for review

kubernetes/helm/pinot/values.yaml

mayankshriv · 2022-01-14T02:11:24Z

cc: @xiangfu0

codecov-commenter · 2022-01-14T02:44:44Z

Codecov Report

Merging #7997 (a37b7d4) into master (c85ff1d) will decrease coverage by 6.42%.
The diff coverage is 60.43%.

❗ Current head a37b7d4 differs from pull request most recent head 94f4cf5. Consider uploading reports for the commit 94f4cf5 to get more accurate results

@@             Coverage Diff              @@
##             master    #7997      +/-   ##
============================================
- Coverage     71.30%   64.87%   -6.43%     
- Complexity     4214     4224      +10     
============================================
  Files          1596     1554      -42     
  Lines         82756    81088    -1668     
  Branches      12348    12173     -175     
============================================
- Hits          59006    52608    -6398     
- Misses        19761    24730    +4969     
+ Partials       3989     3750     -239

Flag	Coverage Δ
integration1	`?`
integration2	`?`
unittests1	`68.12% <60.22%> (-0.01%)`	⬇️
unittests2	`14.35% <16.53%> (+0.06%)`	⬆️

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

Impacted Files	Coverage Δ
...t/broker/api/resources/PinotBrokerHealthCheck.java	`0.00% <0.00%> (ø)`
...roker/requesthandler/BaseBrokerRequestHandler.java	`21.51% <ø> (-49.76%)`	⬇️
...roker/requesthandler/GrpcBrokerRequestHandler.java	`0.00% <ø> (-78.58%)`	⬇️
...thandler/SingleConnectionBrokerRequestHandler.java	`12.96% <ø> (-74.08%)`	⬇️
...pache/pinot/common/config/provider/TableCache.java	`0.00% <0.00%> (ø)`
...e/pinot/common/utils/FileUploadDownloadClient.java	`16.92% <0.00%> (-45.53%)`	⬇️
...a/org/apache/pinot/common/utils/ServiceStatus.java	`50.95% <0.00%> (-18.44%)`	⬇️
...ller/api/resources/PinotControllerHealthCheck.java	`0.00% <0.00%> (ø)`
.../controller/helix/ControllerRequestURLBuilder.java	`74.01% <0.00%> (-9.32%)`	⬇️
...controller/helix/core/minion/PinotTaskManager.java	`41.98% <0.00%> (-23.54%)`	⬇️
... and 414 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c85ff1d...94f4cf5. Read the comment docs.

kubernetes/helm/pinot/templates/controller/ingress.yaml

rodrigorigotti · 2022-01-24T10:01:07Z

Hi @xiangfu0, do you have any updates regarding this PR? 🙂

xiangfu0 · 2022-01-28T18:25:32Z

Hi @xiangfu0, do you have any updates regarding this PR? 🙂

Overall lgtm, just wanna change the enable flag name for future update, others are fine.

WyriHaximus · 2022-02-01T15:54:05Z

Hi @xiangfu0, do you have any updates regarding this PR? slightly_smiling_face

Overall lgtm, just wanna change the enable flag name for future update, others are fine.

What would you prefer us to change it to @xiangfu0?

EDIT: You want to put the version in it correct? (Refs: https://github.com/apache/pinot/pull/7997/files#r794328610 )

This enables Ingress out of the box for anyone using Helm to deploy Pinot to Kubernetes. All one need to do is override the default values inside the values.yaml file.

xiangfu0

lgtm! Thanks for your contribution!

This enables Ingress out of the box for anyone using Helm to deploy Pinot to Kubernetes. All one need to do is override the default values inside the values.yaml file.

kodeine · 2022-03-19T20:36:16Z

@xiangfu0 @dianaarnos based on the template of ingress and documentation it wont work.

in documentation is below

  ingress:
    v1beta1:
      enabled: false
      annotations: {}
      tls: {}
      path: /
      hosts: []
    v1:
      enabled: false

template file is using syntax like

  ingress:
    v1beta1:
      enabled: false
    v1:
      enabled: false
    annotations: {}
    tls: {}
    path: /
    hosts: []

dianaarnos mentioned this pull request Jan 12, 2022

Add Ingress resource to helm chart #7996

Closed

dianaarnos force-pushed the add-ingress-helm branch from b027ef0 to 66e8684 Compare January 12, 2022 09:24

WyriHaximus reviewed Jan 12, 2022

View reviewed changes

kubernetes/helm/pinot/Chart.yaml Outdated Show resolved Hide resolved

WyriHaximus suggested changes Jan 13, 2022

View reviewed changes

kubernetes/helm/pinot/values.yaml Outdated Show resolved Hide resolved

kubernetes/helm/pinot/values.yaml Outdated Show resolved Hide resolved

kubernetes/helm/pinot/values.yaml Outdated Show resolved Hide resolved

dianaarnos force-pushed the add-ingress-helm branch from 66e8684 to deac519 Compare January 13, 2022 09:20

dianaarnos marked this pull request as ready for review January 13, 2022 09:43

xiangfu0 reviewed Jan 14, 2022

View reviewed changes