Add helm chart #245

luismacosta · 2024-02-12T11:04:43Z

[trino-gateway] add helm chart:

secret-bootstrap: database username + password
secret-extra: can be used for more secrets
deployment: initContainer added to initialize database, engines postgres + mysql. it's working for postgres. mysql, I'm getting the error "ERROR 1061 (42000) at line 17: Duplicate key name 'query_history_created_idx'"
ingress: expose request + admin UIs
gateway-config.yml: this was added to docker image, contains env vars. dockerize will replace the content using values.yaml. docker-image folder added just to explain this logic.

Fixes #87

cla-bot · 2024-02-12T11:04:46Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

cla-bot · 2024-02-12T11:05:56Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

cla-bot · 2024-02-12T11:07:52Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

helm/docker-image/Dockerfile

cla-bot · 2024-02-12T11:11:53Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

cla-bot · 2024-02-12T11:11:57Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

helm/docker-image/Dockerfile

cla-bot · 2024-02-12T11:13:14Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

cla-bot · 2024-02-12T11:13:19Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

cla-bot · 2024-02-12T11:22:03Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

helm/docker-image/templates/gateway-ha-config.yml

Chaho12 · 2024-02-13T01:13:59Z

helm/templates/configmap-bootstrap.yaml

+  labels:
+    {{- include "gateway.labels" . | nindent 4 }}
+data:
+  gateway-ha-persistence-postgres.sql: |


hmm is it necessary to have sql as configmap? How about copying during dockerfile as it does not change that often

well, it depends.
in that case user needs a customised postgres/mysql image.
or, using the existent postgres/mysql images, for instance, in initContainer first install curl + get sql scripts and then run psql/mysql commands.

I see. How about adding such explanation to readme? so that user can customize to fit their needs

README updated

I agree with @Chaho12 - we should not redefine the DB initialization SQL here. This risks getting out of sync when it is updated.

helm/values.yaml

cla-bot · 2024-02-13T10:31:37Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

cla-bot · 2024-02-13T10:33:29Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

cla-bot · 2024-02-13T12:13:49Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

cla-bot · 2024-02-13T12:14:38Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

cla-bot · 2024-02-13T12:17:03Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

cla-bot · 2024-02-13T12:17:49Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

cla-bot · 2024-02-13T14:49:26Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

cla-bot · 2024-02-14T01:05:49Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

cla-bot · 2024-02-17T11:24:51Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

luismacosta · 2024-02-17T11:33:54Z

Hello @Chaho12

The requested changes have been done
Helm chart successfully deployed in eks cluster v1.27 with RDS Postgresql v16.1

Chaho12 · 2024-02-19T01:48:47Z

Have you signed & sent the CLA?

Chaho12 · 2024-02-19T02:05:45Z

@vishalya @willmostly Can you guys also take a look on this?

rdsarvar

did a really quick skim, will try to give it a test if I can find some free time

rdsarvar · 2024-02-20T16:07:54Z

helm/README.md

+| backendState.password | string | `nil` |  |
+| backendState.ssl | bool | `false` |  |
+| backendState.username | string | `nil` |  |
+| bootstrap.configMap.enabled | bool | `true` |  |


nitpick: bootstrap is pretty generic, should we add everything to a subfield to the prefix of bootstrap.database. to make it a bit more evident that we're initializing the database underneath the gateway?

just throwing out some random naming ideas: bootstrap.backendDatabase, including it within backendDatabase.boostrap etc etc

rdsarvar · 2024-02-20T16:11:49Z

helm/README.md

+
+## Bootstrap database
+
+By default the chart uses a configMap with sql script + public postgres/mysql images <br />


@mosabua should we spin up a backlogged Github issue for implementing schema evolution for the Trino Gateway?

My thinking is we probably don't want to be explicitly mounting SQL files like this into the Helm chart long term. As the product takes off every change to the underlying datastore schema will be important and having an automated solution for this (Flyway, Liquibase, etc) would be beneficial for simplifying end user migrations

rdsarvar · 2024-02-20T16:56:23Z

helm/templates/configmap-bootstrap.yaml

+    user_name VARCHAR(256),
+    source VARCHAR(256)
+    );
+    CREATE INDEX query_history_created_idx ON query_history(created);


question: I'm guessing the bootstrap initContainer will fail if it's run more than once right? Should we add to the docs that the user must make sure to do explicit deployments?

Aka:

Deploy chart for the first time, bootstrap enabled.

Deploy chart for the second time, (probably nearly immediately), with the bootstrap disabled

willmostly

In addition to the comments, I believe the chart should include a way to configure arbitrary parameters. Otherwise we'll need to ensure this chart provides full coverage for all the various config options the Gateway supports.

willmostly · 2024-02-21T21:15:43Z

helm/docker-image/templates/gateway-ha-config.yml

+  port: {{ default .Env.REQUEST_ROUTER_PORT "8080" }}
+  name: trinoRouter
+  historySize: {{ default .Env.QUERY_HISTORY_SIZE "1000" }}
+  requestBufferSize: 8192


The docker image should not set configurations that already have default values provided. If you think the default requestBufferSize is too small please open a separate PR to increase it.

willmostly · 2024-02-21T21:17:57Z

helm/docker-image/templates/gateway-ha-config.yml

+  user: {{ default .Env.DB_USER "" }}
+  password: {{ default .Env.DB_PASSWORD "" }}
+  driver: {{ default .Env.JDBC_DRIVER "" }}
+  queryHistoryHoursRetention: 24


remove queryHistoryHoursRetention (see above comment on defaults)

willmostly · 2024-02-21T21:19:17Z

helm/docker-image/templates/gateway-ha-config.yml

+  ssl: {{ default .Env.BACKEND_STATE_SSL "true" }}
+
+clusterStatsConfiguration:
+  useApi: true


remove/make configurable (see above comment on defaults)

willmostly · 2024-02-21T21:34:55Z

helm/docker-image/Dockerfile

+RUN set -eux && \
+    apt-get -qq update && apt-get --no-install-recommends --no-install-suggests -yqq install curl && \
+    curl --silent --location --retry 3 -o /tmp/gateway-ha.jar https://repo1.maven.org/maven2/io/trino/gateway/gateway-ha/$GATEWAY_VERSION/gateway-ha-$GATEWAY_VERSION-jar-with-dependencies.jar && \
+    curl --silent --location --retry 3 https://github.com/jwilder/dockerize/releases/download/$DOCKERIZE_VERSION/dockerize-linux-amd64-$DOCKERIZE_VERSION.tar.gz | tar xz -C /usr/bin && \


Can we do without the dockerize utility? If it is unmaintained it could open concerns about the security of the docker image, which would prevent adoption.

Is this project still maintained or abandoned ? jwilder/dockerize#147 - This open issue raises serious doubts about whether dockerize is actively maintained

GO lang vulnerabilities jwilder/dockerize#194 - This open issue notes a critical CVE that may apply to dockerize.

This fork claims to be a replacement, however the last merged PR was 11 months ago.

willmostly · 2024-02-21T21:38:51Z

helm/docker-image/Dockerfile

+ARG FROM_REPO=<myrepo>/temurin
+ARG FROM_TAG=jre-17
+
+FROM ${FROM_REPO}:${FROM_TAG} as builder


The chart should use the standard trino gateway image. Any additional logic for configuration, etc should be handled by a separate init container.

Consider all below notes on the Dockerfile as applying to the init container.

willmostly · 2024-02-21T22:08:15Z

helm/docker-image/templates/gateway-ha-config.yml

+modules:
+  - io.trino.gateway.ha.module.HaGatewayProviderModule
+  - io.trino.gateway.ha.module.ClusterStateListenerModule
+  - io.trino.gateway.ha.module.ClusterStatsMonitorModule
+
+managedApps:
+  - io.trino.gateway.ha.GatewayManagedApp
+  - io.trino.gateway.ha.clustermonitor.ActiveClusterMonitor
+


These should be configurable. Reasonable defaults can be provided in values.yaml

willmostly · 2024-02-21T23:01:47Z

helm/templates/configmap-bootstrap.yaml

+  labels:
+    {{- include "gateway.labels" . | nindent 4 }}
+data:
+  gateway-ha-persistence-postgres.sql: |


I agree with @Chaho12 - we should not redefine the DB initialization SQL here. This risks getting out of sync when it is updated.

willmostly · 2024-02-21T23:04:58Z

helm/templates/deployment.yaml

+              value: {{ .Values.backendState.password}}
+            - name: BACKEND_STATE_SSL
+              value: {{ .Values.backendState.ssl | quote }}
+            - name: EXTRA_JVM_OPTIONS


unused currently - see above comment that this functionality should be added :)

willmostly · 2024-02-21T23:08:34Z

helm/values.yaml

@@ -0,0 +1,92 @@
+bootstrap:


Is there an option to not deploy the DB for those that want to provide their own?

willmostly · 2024-02-21T23:15:08Z

helm/values.yaml

+      admin:
+      app:
+
+securityConf: ""


This is not wired up to gateway-ha-config.yaml. Is the intent for the user to supply the entire security config as a string, and copy that directly into gateway-ha-config.yaml? That would be a good approach imo. If not, then this needs to support security configuration one way or another.

willmostly · 2024-02-21T23:24:13Z

@luismacosta I see a number of my comments are marked outdated. Could you please squash your commits?

mosabua

I have not reviewed in detail but just a first initial input.

Can we cut down the complexity and get rid of having all these separate values in values.yaml that just mirror config properties that go into the config files. I think it would be much simpler and more flexible if we just have a minimum amount of values that are basically the content of the needed config files.. similar to the config files in the Trino helm chart but even cleaner and less overhead..

And maybe even have no defaults in the chart that go beyond what the docker container already has.

willmostly · 2024-02-23T17:25:40Z

Can we cut down the complexity and get rid of having all these separate values in values.yaml that just mirror config properties that go into the config files. I think it would be much simpler and more flexible if we just have a minimum amount of values that are basically the content of the needed config files.. similar to the config files in the Trino helm chart but even cleaner and less overhead..

And maybe even have no defaults in the chart that go beyond what the docker container already has.

I second this approach. Providing the gateway configuration as a configmap seems like a nice clean approach, possibly with a mechanism for interpolating usernames and passwords from secrets.

alaturqua · 2024-03-10T22:56:57Z

Suggestion: Please consider including the following additions:

Labels: Some Kubernetes setups require defining labels for successful deployment. I think the mechanism is there but it is not in values.yaml
Annotations: Annotations provide valuable metadata and enhance compatibility with various Kubernetes configurations. It is not in values.yaml yet.

AlonHarmon · 2024-04-17T20:48:48Z

Suggestion:
add templates of statefullset + service for PostgreSQL (that can be disabled)

Then we will have all in one chart without external deps (except the trino clusters)

mosabua · 2024-04-25T15:39:48Z

Replaced by #323

Michalosu · 2024-06-03T06:50:22Z

@mosabua, @willmostly

it looks like #323 it is not fully covering scope of this PR. There is no support for bootstrapping DB. Perhaps, initContainer is not best solution for DB bootstrap, imho better to have k8s Job with pre-install, pre-upgrade Helm hook and also I would consider to have dependency to psql/mysql charts in case someone wants to quickly bootstrap some PoC on k8s (configured via values). With current state of Helm chart it requires a few manual steps to get Trino gateway working. Install psql/mysql and run manually SQL bootstrap.

Does this project support (ORM/sql module) DB migrations?

mosabua · 2024-06-03T16:45:30Z

We kept the Helm chart simple on purpose and therefore did not include the db. We have docker compose and other setups for that but in production this will vary widely. We might do more depending on community contributions and what we observe from using the chart.

Also we dont have any db migration at this stage but discussed wanting to add this in the future potentially. Current the schema is easy and if needed we add info to the release notes and docs.

mosabua · 2024-06-03T16:45:56Z

Feel free to create separate issues or PR if you want to discuss more. Also find us on trino slack

helm chart

ed606a3

helm chart

5e708f2

Delete helm/docker-image/build-vars.yml

b0735e0

luismacosta mentioned this pull request Feb 12, 2024

Add Helm chart #87

Closed

luismacosta commented Feb 12, 2024

View reviewed changes

helm/docker-image/Dockerfile Outdated Show resolved Hide resolved

helm/docker-image/Dockerfile Outdated Show resolved Hide resolved

Update helm/docker-image/Dockerfile

c998cd0

Update helm/docker-image/Dockerfile

a8f7292

luismacosta commented Feb 12, 2024

View reviewed changes

helm/docker-image/Dockerfile Outdated Show resolved Hide resolved

helm/docker-image/Dockerfile Outdated Show resolved Hide resolved

Update helm/docker-image/Dockerfile

d26c480

Update helm/docker-image/Dockerfile

6507bbe

helm chart

e133d86

fabjesus approved these changes Feb 12, 2024

View reviewed changes

Chaho12 reviewed Feb 13, 2024

View reviewed changes

ebyhr changed the title ~~[trino-gateway] add helm chart~~ Add helm chart Feb 13, 2024

PR review

a592803

PR review

38e9170

luismacosta requested review from Chaho12 and fabjesus February 13, 2024 10:35

update README

9506322

update README

1e7ef51

update README

0f79d1e

update README

f3ce888

update README

54ebc1a

move backendState to secret

e2204ac

remove docker-image folder

f159c86

rdsarvar reviewed Feb 21, 2024

View reviewed changes

willmostly requested changes Feb 21, 2024

View reviewed changes

mosabua reviewed Feb 21, 2024

View reviewed changes

mosabua closed this Apr 25, 2024


		## Bootstrap database

		By default the chart uses a configMap with sql script + public postgres/mysql images <br />

Add helm chart #245

Add helm chart #245

Conversation

luismacosta commented Feb 12, 2024 • edited by ebyhr Loading

cla-bot bot commented Feb 12, 2024

cla-bot bot commented Feb 12, 2024

cla-bot bot commented Feb 12, 2024

cla-bot bot commented Feb 12, 2024

cla-bot bot commented Feb 12, 2024

cla-bot bot commented Feb 12, 2024

cla-bot bot commented Feb 12, 2024

cla-bot bot commented Feb 12, 2024

Choose a reason for hiding this comment

luismacosta Feb 13, 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

cla-bot bot commented Feb 13, 2024

cla-bot bot commented Feb 13, 2024

cla-bot bot commented Feb 13, 2024

cla-bot bot commented Feb 13, 2024

cla-bot bot commented Feb 13, 2024

cla-bot bot commented Feb 13, 2024

cla-bot bot commented Feb 13, 2024

cla-bot bot commented Feb 14, 2024

cla-bot bot commented Feb 17, 2024

luismacosta commented Feb 17, 2024 • edited Loading

Chaho12 commented Feb 19, 2024

Chaho12 commented Feb 19, 2024

rdsarvar left a 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

willmostly left a 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

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

willmostly commented Feb 21, 2024

mosabua left a comment • edited Loading

Choose a reason for hiding this comment

willmostly commented Feb 23, 2024

alaturqua commented Mar 10, 2024 • edited Loading

AlonHarmon commented Apr 17, 2024

mosabua commented Apr 25, 2024

Michalosu commented Jun 3, 2024

mosabua commented Jun 3, 2024

mosabua commented Jun 3, 2024

luismacosta commented Feb 12, 2024 •

edited by ebyhr

Loading

luismacosta Feb 13, 2024 •

edited

Loading

luismacosta commented Feb 17, 2024 •

edited

Loading

mosabua left a comment •

edited

Loading

alaturqua commented Mar 10, 2024 •

edited

Loading