Enabled overrides for worker and coordinator configmap resources #163

silent-lad · 2024-05-14T07:40:24Z

This is to install multiple helm releases of trino cluster in the same namespace.
Closes #159 .

cla-bot · 2024-05-14T07:40:27Z

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 cla@trino.io. 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

nineinchnick · 2024-05-14T08:09:14Z

Would be nice to have a test for this, maybe a second call to ct install in https://github.com/trinodb/charts/blob/main/.github/workflows/ci-cd.yaml#L44

sudohainguyen · 2024-05-14T10:20:32Z

charts/trino/templates/configmap-coordinator.yaml

@@ -132,7 +132,7 @@ data:
 apiVersion: v1
 kind: ConfigMap
 metadata:
-  name: trino-access-control-volume-coordinator
+  name: {{ template "trino.coordinator" . }}-access-control-volume-coordinator


could we eliminate suffix -coordinator?
since in the template already has it, similar to other confimap objects

since in the template already has it, similar to other confimap objects

I thought of it. The problem with it would be that in case someone doesn't overrides the name the default name only say "trino" which will cause duplicate configmap issues.

nineinchnick · 2024-05-22T07:23:00Z

@silent-lad I recently added a test.sh script that runs multiple tests. Can you update it to add a new test for deploying multiple releases into a single namespace? If you'd use ct install, you'd have to disable cleanup when calling it. test.sh already creates and cleans up a custom namespace.

silent-lad · 2024-05-22T08:44:13Z

Hey @nineinchnick , will do the same. Thanks for the heads up

cla-bot · 2024-05-23T10:02:59Z

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 cla@trino.io. 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-05-23T10:46:25Z

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 cla@trino.io. 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

silent-lad · 2024-05-23T10:48:37Z

@silent-lad I recently added a test.sh script that runs multiple tests. Can you update it to add a new test for deploying multiple releases into a single namespace? If you'd use ct install, you'd have to disable cleanup when calling it. test.sh already creates and cleans up a custom namespace.

This is done now.

nineinchnick

This looks great! My only ask is if you could add coordinatorNameOverride and workerNameOverride to values.yaml with empty defaults, but with a comment describing when to use them.

nineinchnick · 2024-05-23T12:14:44Z

test.sh

-    time ct install "${CT_ARGS[@]}" --helm-extra-set-args "$HELM_EXTRA_SET_ARGS ${testCases[$test_name]}"
+    echo "${testCases[$test_name]}"
+    if [ "$test_name" == "multiple_clusters" ]; then
+        time ct install "${CT_ARGS[@]}" --helm-extra-set-args "$HELM_EXTRA_SET_ARGS ${testCases[default]}"


I think we also need --skip-clean-up added for ct install.

test.sh

Co-authored-by: Jan Waś <jan@was.net.pl>

cla-bot · 2024-05-23T18:23:33Z

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 cla@trino.io. 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-05-23T18:24:31Z

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 cla@trino.io. 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-05-24T04:25:33Z

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 cla@trino.io. 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

silent-lad · 2024-05-24T04:31:13Z

Have pushed the required changes @nineinchnick

nineinchnick · 2024-05-24T07:59:13Z

charts/trino/values.yaml

+# If you want to have multiple Trino Clusters in same namespace 
+# override the following values to avoid naming conflicts
+# Example:
+# coordinatorNameOverride: trino-adhoc 
+# workerNameOverride: trino-adhoc  
+# nameOverride: trino-adhoc


Suggested change

# If you want to have multiple Trino Clusters in same namespace

# override the following values to avoid naming conflicts

# Example:

# coordinatorNameOverride: trino-adhoc

# workerNameOverride: trino-adhoc

# nameOverride: trino-adhoc

# -- Override resource names to avoid name conflicts when deploying multiple

# releases in the same namespace.

# @raw

# Example:

# ```yaml

# coordinatorNameOverride: trino-adhoc

# workerNameOverride: trino-adhoc

# nameOverride: trino-adhoc

# ```

nameOverride:

coordinatorNameOverride:

workerNameOverride:

This will also require update to README.md, but it should be automatically generated by the pre-commit hook, if you've installed it locally.

Also fix trailing spaces, see the CI errors for details.

nineinchnick · 2024-05-26T09:10:47Z

charts/trino/templates/configmap-coordinator.yaml

@@ -133,7 +133,7 @@ data:
 apiVersion: v1
 kind: ConfigMap
 metadata:
-  name: trino-access-control-volume-coordinator
+  name: {{ template "trino.coordinator" . }}-access-control-volume-coordinator


How about using trino.fullname instead of trino.coordinator?

nineinchnick · 2024-05-30T10:28:06Z

@cla-bot check

cla-bot · 2024-05-30T10:28:08Z

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 cla@trino.io. 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-05-30T10:28:09Z

The cla-bot has been summoned, and re-checked this pull request!

Enabled overrides for worker and coordinator configmap resources

f99f8e2

sudohainguyen reviewed May 14, 2024

View reviewed changes

silent-lad mentioned this pull request May 23, 2024

adds silent-lad trinodb/cla#8

Open

Merge branch 'trinodb:main' into main

fbc2bba

Adds tests

8768d9b

nineinchnick approved these changes May 23, 2024

View reviewed changes

nineinchnick reviewed May 23, 2024

View reviewed changes

test.sh Outdated Show resolved Hide resolved

Fix set value in tests for multiple_clusters

2fdaea1

Co-authored-by: Jan Waś <jan@was.net.pl>

Merge branch 'main' into main

e398138

Adds documentation for the name overrides

e692064

nineinchnick reviewed May 24, 2024

View reviewed changes

huw0 mentioned this pull request May 25, 2024

Always prefix configmap names with chart fullname #170

Closed

nineinchnick reviewed May 26, 2024

View reviewed changes

nineinchnick mentioned this pull request May 30, 2024

Enabled overrides for worker and coordinator configmap resources #173

Merged

nineinchnick closed this May 31, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enabled overrides for worker and coordinator configmap resources #163

Enabled overrides for worker and coordinator configmap resources #163

silent-lad commented May 14, 2024

cla-bot bot commented May 14, 2024

nineinchnick commented May 14, 2024

sudohainguyen May 14, 2024

silent-lad May 18, 2024

nineinchnick commented May 22, 2024

silent-lad commented May 22, 2024

cla-bot bot commented May 23, 2024

cla-bot bot commented May 23, 2024

silent-lad commented May 23, 2024

nineinchnick left a comment

nineinchnick May 23, 2024

cla-bot bot commented May 23, 2024

cla-bot bot commented May 23, 2024

cla-bot bot commented May 24, 2024

silent-lad commented May 24, 2024

nineinchnick May 24, 2024

nineinchnick May 24, 2024

nineinchnick May 24, 2024

nineinchnick May 26, 2024

nineinchnick commented May 30, 2024

cla-bot bot commented May 30, 2024

cla-bot bot commented May 30, 2024

Enabled overrides for worker and coordinator configmap resources #163

Enabled overrides for worker and coordinator configmap resources #163

Conversation

silent-lad commented May 14, 2024

cla-bot bot commented May 14, 2024

nineinchnick commented May 14, 2024

sudohainguyen May 14, 2024

Choose a reason for hiding this comment

silent-lad May 18, 2024

Choose a reason for hiding this comment

nineinchnick commented May 22, 2024

silent-lad commented May 22, 2024

cla-bot bot commented May 23, 2024

cla-bot bot commented May 23, 2024

silent-lad commented May 23, 2024

nineinchnick left a comment

Choose a reason for hiding this comment

nineinchnick May 23, 2024

Choose a reason for hiding this comment

cla-bot bot commented May 23, 2024

cla-bot bot commented May 23, 2024

cla-bot bot commented May 24, 2024

silent-lad commented May 24, 2024

nineinchnick May 24, 2024

Choose a reason for hiding this comment

nineinchnick May 24, 2024

Choose a reason for hiding this comment

nineinchnick May 24, 2024

Choose a reason for hiding this comment

nineinchnick May 26, 2024

Choose a reason for hiding this comment

nineinchnick commented May 30, 2024

cla-bot bot commented May 30, 2024

cla-bot bot commented May 30, 2024