[HELM] Update template on port-foward usage guide NOTES.txt #4589

phyyou · 2023-03-23T16:18:06Z

Fix Stale Docs URL in Helm Chart
~~Update kyuubiDefaults structure~~
Update NOTES.txt
- When kyuubi.kyuubiConf.kyuubiDefaults."kyuubi.frontend.bind.host" set localhost, render port-forward example in NOTES.txt for development usage guide
~~Add extraFiles for config mount~~
- ~~extraFiles for another file mount in kyuubi conf directory.~~

Why are the changes needed?

I think Helm chart notes had been wrong from this commit.

kyuubi/charts/kyuubi/templates/NOTES.txt

Lines 33 to 34 in dbb741f

- To access {{ $.Release.Name }}-{{ $name | kebabcase }} service from outside the cluster for debugging, run the following command:

kubectl port-forward svc/{{ $.Release.Name }}-{{ $name | kebabcase }} {{ tpl $frontend.service.port $ }}:{{ tpl $frontend.service.port $ }} -n {{ $.Release.Namespace }}

Can we delete this note or update this note if --address option is work like this? :

 - To access {{ $.Release.Name }}-{{ $name | kebabcase }} service from outside the cluster for debugging, run the following > > 
command: 
    kubectl port-forward svc/{{ $.Release.Name }}-{{ $name | kebabcase }} {{ tpl $frontend.service.port $ }}:{{ tpl >?> >$frontend.service.port $ }} -n {{ $.Release.Namespace }} --address <your-ip>

~~and more changes:~~

~~Update .Values.kyuubiConf.kyuubiDefaults to dict for more variant usage in templates. (like NOTES.txt changes)~~
~~Add extraFiles in configmap for like mounting hive-site-xml on kyuubi config directory.~~

How was this patch tested?

Run test locally before make a pull request

When kyuubi.kyuubiConf.kyuubiDefaults set localhost, NOTES.txt render for development usage guide

codecov-commenter · 2023-03-24T03:00:07Z

Codecov Report

Merging #4589 (966e74c) into master (1457a2f) will decrease coverage by 0.01%.
The diff coverage is n/a.

❗ Current head 966e74c differs from pull request most recent head 2533a93. Consider uploading reports for the commit 2533a93 to get more accurate results

@@             Coverage Diff              @@
##             master    #4589      +/-   ##
============================================
- Coverage     53.32%   53.31%   -0.01%     
  Complexity       13       13              
============================================
  Files           577      577              
  Lines         31557    31557              
  Branches       4244     4244              
============================================
- Hits          16827    16826       -1     
+ Misses        13147    13144       -3     
- Partials       1583     1587       +4

see 7 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

zwangsheng · 2023-03-24T03:01:18Z

Hi @phyyou, thanks for your PR.

I install your changes in local minikube cluster, i find kyuubi register self to zookeeper with localhost:10009, which may cause can't find kyuubi instance.

2023-03-24 02:56:27.968 INFO org.apache.kyuubi.ha.client.zookeeper.ZookeeperDiscoveryClient: Created a /kyuubi/serviceUri=localhost:10009;version=1.7.0;sequence=0000000000 on ZooKeeper for KyuubiServer uri: localhost:10009

You can find the corresponding logic in TFrontendService.scala#connectionUrl.

pan3793 · 2023-03-24T05:17:41Z

Update .Values.kyuubiConf.kyuubiDefaults to dict for more variant usage in templates. (like NOTES.txt changes)

I'm neutral on this change, @dnskr WDYT?

dnskr · 2023-03-24T07:09:36Z

Update .Values.kyuubiConf.kyuubiDefaults to dict for more variant usage in templates. (like NOTES.txt changes)

I'm neutral on this change, @dnskr WDYT?

It's against recommended best practices: https://helm.sh/docs/chart_best_practices/values/#naming-conventions
It looks natural in this case, but it brings many inconveniences with setting values, for example:

Copy/Paste a.b.c to IntelliJ IDEA becomes

  a:
    b:
      c:

Set property value in cli: https://helm.sh/docs/chart_best_practices/values/#consider-how-users-will-use-your-values

helm install kyuubi --set kyuubiConf.kyuubiDefaults."kyuubi\.frontend\.bind\.host"=localhost

Overwriting many default values.

I know only one mature chart which still uses similar property names, but looks like it was a mistake they did at the beginning: https://github.com/argoproj/argo-helm/blob/ccef4448748601bb253a89fa86ed036b0e620cc1/charts/argo-cd/values.yaml#L136

phyyou · 2023-03-24T10:35:33Z

It looks natural in this case, but it brings many inconveniences with setting values,

Thanks @dnskr. I definitely agree.
When I first thought of and created it, it seemed like a good idea, but when I actually tried Template Rendering with helm, and I realized it was inconvenient.

I will to revert this,
However, I would like to think of a way to conveniently specify the configuration values in the Helm Chart.

Do you have any other good ideas for kyuubiConf in Helm Chart? @dnskr @pan3793

phyyou · 2023-03-24T10:46:02Z

I just got a lot of inspiration Chart of Apache Airflow (https://github.com/apache/airflow/blob/main/chart/values.yaml#L1834-L1909).

What do you think about kyuubiDefaults values with a similar structure? @dnskr

https://airflow.apache.org/docs/helm-chart/stable/parameters-ref.html

Update NOTES.txt for Reverted values

pan3793 · 2023-03-24T11:15:06Z

charts/kyuubi/values.yaml

+  # See https://kyuubi.readthedocs.io/en/master/deployment/settings.html#logging for more details
  log4j2: ~

+  extraFiles: {}


it's valuable to change in a separate PR. let's focus on one thing in each PR, to make the future visitor easy to understand what we were doing.

Thanks! got it!

what is the use case for this property?

Users may use Kyuubi/Spark w/ Hive MetaStore, then hive-site.xml is required

And another use case is FileSessionConfAdvisor introduced in #3742, and emm... it isn't mentioned in docs, and we appreciate contributors who can help us to improvement the docs 😄

@pan3793 Would it be better to add specific properties instead? I.e. hiveSite etc

@pan3793 Would it be better to add specific properties instead? I.e. hiveSite etc

I agree to use specific properties for specific files, but there are some non-enumerable files, e.g. in FileSessionConfAdvisor use case, there may have

kyuubi-session-etl.conf

kyuubi-session-ad-hoc.conf

kyuubi-session-admin.conf

kyuubi-session-<...>.conf

pan3793 · 2023-03-24T11:23:37Z

https://airflow.apache.org/docs/helm-chart/stable/parameters-ref.html

I like the idea of "examples", actually, I was asked, "I get no idea w/ a placeholder ~, what should I assign to kyuubiDefaults?"

~

Examples:

extraEnvFrom: |-
  - secretRef:
      name: '{{ .Release.Name }}-airflow-connections'
extraEnvFrom: |-
  - configMapRef:
      name: '{{ .Release.Name }}-airflow-variables'

charts/kyuubi/values.yaml

pan3793 · 2023-03-24T11:44:40Z

Do you have Kerberos use case? There are some extra work, like keytab, krb5.conf, to make the Helm chart work out-of-box

(It from airflow chart README.md)

including the "key" for users who are not familiar w/ yaml will confusing how to handle Co-authored-by: Cheng Pan <[email protected]>

dnskr · 2023-03-24T11:59:46Z

I just got a lot of inspiration Chart of Apache Airflow (https://github.com/apache/airflow/blob/main/chart/values.yaml#L1834-L1909).
What do you think about kyuubiDefaults values with a similar structure? @dnskr

It might be a good idea, but managing many properties also might be exhausting. Also from my POV this requires stable configuration mechanism.

phyyou · 2023-03-24T12:09:51Z

It might be a good idea, but managing many properties also might be exhausting. Also from my POV this requires stable configuration mechanism.

I agree with the previous comment that managing a large number of properties can be quite challenging and exhausting, especially if there is no stable configuration mechanism in place.

I think It meens while it could be a good long-term goal to move towards a more structured approach. I also agree that it is a good solution at the moment.

So, I think also that while a structured approach like the Apache Airflow chart could be useful in the future, it is not a good solution at the moment, and we should focus on creating a stable configuration mechanism first.

phyyou · 2023-03-24T12:23:06Z

Do you have Kerberos use case? There are some extra work, like keytab, krb5.conf, to make the Helm chart work out-of-box

@pan3793

I don't have a complete understanding of the Kerberos use case, but I believe that we can refer to the configuration used in the Apache Airflow chart (https://github.com/apache/airflow/blob/main/chart/values.yaml#L413-L446) as a starting point.

So, I'd like to submit a new PR for krb5.conf with kyuubiConf.extraFiles.

charts/kyuubi/values.yaml

charts/kyuubi/templates/NOTES.txt

pan3793

LGTM except for minor comments, thanks

pan3793 · 2023-03-24T12:30:13Z

@phyyou thanks for finding an example for kerberos, we can defer it if you don't have kerberos use case

charts/kyuubi/values.yaml

pan3793 · 2023-03-24T13:02:47Z

LGTM on my side, also cc @zwangsheng

phyyou · 2023-03-24T13:03:42Z

@pan3793

I'm sorry for the frequent mentions,

I fixed the when there was no equal sign now.

phyyou · 2023-03-24T13:07:21Z

I install your changes in local minikube cluster, i find kyuubi register self to zookeeper with localhost:10009, which may cause can't find kyuubi instance.

2023-03-24 02:56:27.968 INFO org.apache.kyuubi.ha.client.zookeeper.ZookeeperDiscoveryClient: Created a /kyuubi/serviceUri=localhost:10009;version=1.7.0;sequence=0000000000 on ZooKeeper for KyuubiServer uri: localhost:10009 You can find the corresponding logic in TFrontendService.scala#connectionUrl.

@zwangsheng

Sorry for the late response.
I have also experienced this error before.
Specifically, I encountered this error when set kyuubi.frontend.connection.url.use.hostname=false.,
but, it may require a deeper investigation to identify the root cause.

zwangsheng · 2023-03-24T13:28:28Z

Sorry for the late response. I have also experienced this error before. Specifically, I encountered this error when set kyuubi.frontend.connection.url.use.hostname=false., but, it may require a deeper investigation to identify the root cause.

Well, seems we revert set kyuubi.frontend.bind.host=localhost, which cause the problem.

LGTM, thanks.

…S.txt - Fix Stale Docs URL in Helm Chart - ~~Update kyuubiDefaults structure~~ - Update NOTES.txt - When kyuubi.kyuubiConf.kyuubiDefaults."kyuubi.frontend.bind.host" set localhost, render port-forward example in NOTES.txt for development usage guide - ~~Add extraFiles for config mount~~ - ~~extraFiles for another file mount in kyuubi conf directory.~~ ### _Why are the changes needed?_ Based on #4561 (comment) > I think Helm chart notes had been wrong from this commit. >https://github.com/apache/kyuubi/blob/dbb741fc5b0d2fd4b0640ff70ea12aca75864166/charts/kyuubi/templates/NOTES.txt#L33-L34 > Can we delete this note or update this note if `--address` option is work like this? : > ```suggestion > - To access {{ $.Release.Name }}-{{ $name | kebabcase }} service from outside the cluster for debugging, run the following > > > command: > kubectl port-forward svc/{{ $.Release.Name }}-{{ $name | kebabcase }} {{ tpl $frontend.service.port $ }}:{{ tpl >?> >$frontend.service.port $ }} -n {{ $.Release.Namespace }} --address <your-ip> > ``` ~~and more changes:~~ - ~~Update `.Values.kyuubiConf.kyuubiDefaults` to dict for more variant usage in templates. (like NOTES.txt changes)~~ - ~~Add extraFiles in configmap for like mounting hive-site-xml on kyuubi config directory.~~ ### _How was this patch tested?_ - [x] [Run test](https://kyuubi.readthedocs.io/en/master/develop_tools/testing.html#running-tests) locally before make a pull request Closes #4589 from phyyou/feat/helm-chart-improve. Closes #4589 d7ee29e [phyyou] Fix regex when no equal sign 0b86f28 [phyyou] Update condition for edge case 4edf904 [phyyou] chore aabe93c [PHYYOU] Update kyuubiDefaults example guide in Helm Chart 4de8964 [phyyou] Add README.md for Kyuubi Helm Chart (It from airflow chart README.md) abacd32 [phyyou] Add Example Usecases in kyuubiConf ebcb231 [phyyou] Update empty string to nil 2533a93 [phyyou] Revert extraFiles 56fa525 [phyyou] Revert kyuubiConf Structure Update Update NOTES.txt for Reverted values 966e74c [phyyou] fix: template => include 03d6984 [phyyou] Fix: Add EOF e6affdd [phyyou] Add extraFiles for config mount b0963fd [phyyou] Update NOTES.txt; ac9766a [phyyou] Update kyuubiDefaults structure 77eaa05 [phyyou] Fix Stale Docs URL Lead-authored-by: phyyou <[email protected]> Co-authored-by: PHYYOU <[email protected]> Signed-off-by: Cheng Pan <[email protected]> (cherry picked from commit 0ebedda) Signed-off-by: Cheng Pan <[email protected]>

pan3793 · 2023-03-24T13:30:59Z

Thanks all, merged to master/1.7

phyyou · 2023-03-24T13:32:13Z

@zwangsheng @pan3793

Since this commit,
kyuubi.frontend.connection.url.use.hostname=false is default value for helm chart.

In my opinion, it would be better not to use local kyuubi server Zookeeper in the Helm chart.

For development environments, I think it seems like a good idea to have an extra deployment option to turn on and off the Zookeeper deployment in the Helm chart.

pan3793 · 2023-03-24T13:40:35Z

For development environments, I think it seems like a good idea to have an extra deployment option to turn on and off the Zookeeper deployment in the Helm chart.

SGTM.

yaooqinn · 2023-04-13T02:41:36Z

charts/kyuubi/README.md

+
+# Helm Chart for Apache Kyuubi
+
+[Apache Kyuubi](https://airflow.apache.org/) is a distributed and multi-tenant gateway to provide serverless SQL on Data Warehouses and Lakehouses.


There is a typo here https://airflow.apache.org/ cc @phyyou @zwangsheng @pan3793 @dnskr

Thanks, I'll submit new PR soon.

phyyou added 4 commits March 23, 2023 23:25

Fix Stale Docs URL

77eaa05

Update kyuubiDefaults structure

ac9766a

Update NOTES.txt;

b0963fd

When kyuubi.kyuubiConf.kyuubiDefaults set localhost, NOTES.txt render for development usage guide

Add extraFiles for config mount

e6affdd

github-actions bot added kind:build module:kubernetes labels Mar 23, 2023

phyyou changed the title ~~[HELM] Add condition for render port-foward usage guide NOTES.txt and Update KyuubiDefaults Value Guide~~ [HELM] Add condition for render port-foward usage guide NOTES.txt and Update KyuubiDefaults Value Mar 23, 2023

phyyou changed the title ~~[HELM] Add condition for render port-foward usage guide NOTES.txt and Update KyuubiDefaults Value~~ [HELM] Add render port-foward usage guide NOTES.txt and Update KyuubiDefaults Value Mar 23, 2023

phyyou changed the title ~~[HELM] Add render port-foward usage guide NOTES.txt and Update KyuubiDefaults Value~~ [HELM] Add template on port-foward usage guide NOTES.txt and Update KyuubiDefaults Value Mar 23, 2023

phyyou added 2 commits March 24, 2023 01:21

Fix: Add EOF

03d6984

fix: template => include

966e74c

Revert kyuubiConf Structure Update

56fa525

Update NOTES.txt for Reverted values

phyyou force-pushed the feat/helm-chart-improve branch from 96aca65 to 56fa525 Compare March 24, 2023 11:09

phyyou changed the title ~~[HELM] Add template on port-foward usage guide NOTES.txt and Update KyuubiDefaults Value~~ [HELM] Add template on port-foward usage guide NOTES.txt Mar 24, 2023

phyyou changed the title ~~[HELM] Add template on port-foward usage guide NOTES.txt~~ [HELM] Add template on port-foward usage guide NOTES.txt and Add extraFiles for config mount Mar 24, 2023

pan3793 reviewed Mar 24, 2023

View reviewed changes

Revert extraFiles

2533a93

pan3793 reviewed Mar 24, 2023

View reviewed changes

charts/kyuubi/values.yaml Outdated Show resolved Hide resolved

phyyou changed the title ~~[HELM] Add template on port-foward usage guide NOTES.txt and Add extraFiles for config mount~~ [HELM] Add template on port-foward usage guide NOTES.txt Mar 24, 2023

pan3793 reviewed Mar 24, 2023

View reviewed changes

charts/kyuubi/values.yaml Outdated Show resolved Hide resolved

Update empty string to nil

ebcb231

Add Example Usecases in kyuubiConf

abacd32

phyyou and others added 2 commits March 24, 2023 20:54

Add README.md for Kyuubi Helm Chart

4de8964

(It from airflow chart README.md)

Update kyuubiDefaults example guide in Helm Chart

aabe93c

including the "key" for users who are not familiar w/ yaml will confusing how to handle Co-authored-by: Cheng Pan <[email protected]>

phyyou changed the title ~~[HELM] Add template on port-foward usage guide NOTES.txt~~ [HELM] Update template on port-foward usage guide NOTES.txt Mar 24, 2023

pan3793 reviewed Mar 24, 2023

View reviewed changes

charts/kyuubi/values.yaml Outdated Show resolved Hide resolved

pan3793 reviewed Mar 24, 2023

View reviewed changes

charts/kyuubi/templates/NOTES.txt Outdated Show resolved Hide resolved

pan3793 approved these changes Mar 24, 2023

View reviewed changes

pan3793 reviewed Mar 24, 2023

View reviewed changes

charts/kyuubi/values.yaml Show resolved Hide resolved

phyyou added 2 commits March 24, 2023 21:43

chore

4edf904

Update condition for edge case

0b86f28

pan3793 approved these changes Mar 24, 2023

View reviewed changes

pan3793 assigned phyyou Mar 24, 2023

pan3793 added this to the v1.7.1 milestone Mar 24, 2023

Fix regex when no equal sign

d7ee29e

zwangsheng approved these changes Mar 24, 2023

View reviewed changes

pan3793 closed this in 0ebedda Mar 24, 2023

pan3793 mentioned this pull request Mar 26, 2023

[Umbrella] Helm Chart improvements #4473

Open

4 tasks

yaooqinn reviewed Apr 13, 2023

View reviewed changes

	- To access {{ $.Release.Name }}-{{ $name \| kebabcase }} service from outside the cluster for debugging, run the following command:
	kubectl port-forward svc/{{ $.Release.Name }}-{{ $name \| kebabcase }} {{ tpl $frontend.service.port $ }}:{{ tpl $frontend.service.port $ }} -n {{ $.Release.Namespace }}


		# Helm Chart for Apache Kyuubi

		[Apache Kyuubi](https://airflow.apache.org/) is a distributed and multi-tenant gateway to provide serverless SQL on Data Warehouses and Lakehouses.

[HELM] Update template on port-foward usage guide NOTES.txt #4589

[HELM] Update template on port-foward usage guide NOTES.txt #4589

Uh oh!

Conversation

phyyou commented Mar 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why are the changes needed?

How was this patch tested?

Uh oh!

codecov-commenter commented Mar 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

zwangsheng commented Mar 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pan3793 commented Mar 24, 2023

Uh oh!

dnskr commented Mar 24, 2023

Uh oh!

phyyou commented Mar 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

phyyou commented Mar 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pan3793 commented Mar 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pan3793 commented Mar 24, 2023

Uh oh!

dnskr commented Mar 24, 2023

Uh oh!

phyyou commented Mar 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

phyyou commented Mar 24, 2023

Uh oh!

Uh oh!

Uh oh!

pan3793 left a comment

Choose a reason for hiding this comment

Uh oh!

pan3793 commented Mar 24, 2023

Uh oh!

Uh oh!

pan3793 commented Mar 24, 2023

Uh oh!

phyyou commented Mar 24, 2023

Uh oh!

phyyou commented Mar 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zwangsheng commented Mar 24, 2023

Uh oh!

pan3793 commented Mar 24, 2023

Uh oh!

phyyou commented Mar 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pan3793 commented Mar 24, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phyyou commented Mar 23, 2023 •

edited

Loading

codecov-commenter commented Mar 24, 2023 •

edited

Loading

zwangsheng commented Mar 24, 2023 •

edited

Loading

phyyou commented Mar 24, 2023 •

edited

Loading

phyyou commented Mar 24, 2023 •

edited

Loading

pan3793 commented Mar 24, 2023 •

edited

Loading

phyyou commented Mar 24, 2023 •

edited

Loading

phyyou commented Mar 24, 2023 •

edited

Loading

phyyou commented Mar 24, 2023 •

edited

Loading

phyyou Apr 14, 2023 •

edited

Loading