[KYUUBI #4557][HELM] Kyuubi server should bind Pod IP by default #4561

pan3793 · 2023-03-20T01:14:33Z

Why are the changes needed?

How was this patch tested?

Add some test cases that check the changes thoroughly including negative and positive cases if possible
Add screenshots for manual tests if appropriate

After changing, the Kyuubi Server correctly binds the Pod IP.

KyuubiTBinaryFrontendService#64 - Starting and exposing JDBC connection at: jdbc:hive2://10.88.224.214:10009/

Run test locally before make a pull request

pan3793 · 2023-03-20T01:17:25Z

cc @dnskr @zwangsheng

codecov-commenter · 2023-03-20T02:17:59Z

Codecov Report

Merging #4561 (018a3a0) into master (301a05a) will decrease coverage by 0.11%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master    #4561      +/-   ##
============================================
- Coverage     53.29%   53.19%   -0.11%     
  Complexity       13       13              
============================================
  Files           573      573              
  Lines         31498    31498              
  Branches       4237     4237              
============================================
- Hits          16788    16756      -32     
- Misses        13129    13161      +32     
  Partials       1581     1581

see 10 files with indirect coverage changes

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

dnskr · 2023-03-20T09:25:22Z

I've tested it quickly and it doesn't work if I install Kyuubi chart and try to connect from Intellij IDEA using kubectl port-forward:

helm install kyuubi charts/kyuubi
...
kubectl port-forward svc/kyuubi-thrift-binary 10009:10009 -n default
Forwarding from 127.0.0.1:10009 -> 10009
Forwarding from [::1]:10009 -> 10009
Handling connection for 10009
E0320 10:16:55.856000   18444 portforward.go:406] an error occurred forwarding 10009 -> 10009: error forwarding port 10009 to pod 790be50024590075e03f7f0540e464838943ac6ebc370d440836a3de4535f60f, uid : exit status 1: 2023/03/20 09:16:56 socat[8520] E connect(5, AF=2 127.0.0.1:10009, 16): Connection refused
E0320 10:16:55.857032   18444 portforward.go:234] lost connection to pod

pan3793 · 2023-03-20T09:27:56Z

Do you see the similar log I pasted in the PR description? Does it bind to Pod IP or something else in your case?

dnskr · 2023-03-20T09:46:27Z

Yes, there is IP instead of hostname:

INFO org.apache.kyuubi.server.KyuubiTBinaryFrontendService: Starting and exposing JDBC connection at: jdbc:hive2://10.244.0.11:10009/
INFO org.apache.kyuubi.ha.client.zookeeper.ZookeeperDiscoveryClient: Created a /kyuubi/serviceUri=10.244.0.11:10009;version=1.7.0;sequence=0000000000 on ZooKeeper for KyuubiServer uri: 10.244.0.11:10009
INFO org.apache.kyuubi.ha.client.KyuubiServiceDiscovery: Service[KyuubiServiceDiscovery] is started.

pan3793 · 2023-03-22T16:30:51Z

@dnskr I can reproduce the issue, and I have no idea why kubectl port-forward does not work. We use ingress in production, and it works well.

@Yikun, do you have any idea why kubectl port-forward does not work when the applicaiton inside Pod listening Pod IP, but works on listening localhost?

Yikun · 2023-03-23T02:41:35Z

do you have any idea why kubectl port-forward does not work when the applicaiton inside Pod listening Pod IP, but works on listening localhost?

Not sure, specified address help or not?

# all ip
kubectl port-forward svc/kyuubi-thrift-binary --address 0.0.0.0 10009:10009 -n default
# local
kubectl port-forward svc/kyuubi-thrift-binary --address 127.0.0.1 10009:10009 -n default
# your specified ip
kubectl port-forward svc/kyuubi-thrift-binary --address your_ip 10009:10009 -n default

zwangsheng · 2023-03-23T03:01:20Z

I reproduce the issue in Mac m1 Minikube Cluster with follow command.

helm install kyuubi # with apachekyuubi:1.7.0-spark

# Kyuubi Log
2023-03-23 02:40:14.116 INFO org.apache.kyuubi.server.KyuubiTBinaryFrontendService: Starting and exposing JDBC connection at: jdbc:hive2://172.17.0.2:10009/
2023-03-23 02:40:14.153 INFO org.apache.curator.framework.state.ConnectionStateManager: State change: CONNECTED
2023-03-23 02:40:14.202 INFO org.apache.kyuubi.ha.client.zookeeper.ZookeeperDiscoveryClient: Zookeeper client connection state changed to: CONNECTED
2023-03-23 02:40:14.361 INFO org.apache.kyuubi.ha.client.zookeeper.ZookeeperDiscoveryClient: Created a /kyuubi/serviceUri=172.17.0.2:10009;version=1.7.0;sequence=0000000000 on ZooKeeper for KyuubiServer uri: 172.17.0.2:10009
2023-03-23 02:40:14.369 INFO org.apache.kyuubi.ha.client.KyuubiServiceDiscovery: Service[KyuubiServiceDiscovery] is started.
2023-03-23 02:40:14.369 INFO org.apache.kyuubi.server.KyuubiTBinaryFrontendService: Service[KyuubiTBinaryFrontend] is started.
2023-03-23 02:40:14.369 INFO org.apache.kyuubi.server.KyuubiServer: Service[KyuubiServer] is started.
2023-03-23 02:40:14.607 INFO org.apache.kyuubi.Utils: Loading Kyuubi properties from /opt/kyuubi/conf/kyuubi-defaults.con

kubectl port-forward kyuubi-7957995b66-8w4sn 10009:10009

# and start another term run
./bin/beeline -u "jdbc:hive2://127.0.0.1:10009/;#spark.master=local"

The port forward occurred

Forwarding from 127.0.0.1:10009 -> 10009
Forwarding from [::1]:10009 -> 10009
Handling connection for 10009
E0323 10:51:18.750107   86266 portforward.go:407] an error occurred forwarding 10009 -> 10009: error forwarding port 10009 to pod e49c1342e3e1a9c0e6c0d6a30d1ba57695be2c77b11820181987a54826bbdf26, uid : exit status 1: 2023/03/23 02:51:17 socat[46240] E connect(5, AF=2 127.0.0.1:10009, 16): Connection refused
E0323 10:51:18.751436   86266 portforward.go:233] lost connection to pod

And find some same issue kubernetes/kubectl#1363.

Client Version: version.Info{Major:"1", Minor:"26", GitVersion:"v1.26.0", GitCommit:"b46a3f887ca979b1a5d14fd39cb1af43e7e5d12d", GitTreeState:"clean", BuildDate:"2022-12-08T19:58:30Z", GoVersion:"go1.19.4", Compiler:"gc", Platform:"darwin/amd64"}
Kustomize Version: v4.5.7
Server Version: version.Info{Major:"1", Minor:"25", GitVersion:"v1.25.3", GitCommit:"434bfd82814af038ad94d62ebe59b133fcb50506", GitTreeState:"clean", BuildDate:"2022-10-12T10:49:09Z", GoVersion:"go1.19.2", Compiler:"gc", Platform:"linux/arm64"}

pan3793 · 2023-03-23T04:01:09Z

Based on [1], kubectl port forward is designed to only support pod/localhost as destination.

So there is an issue for kubectl port forward, but considering issue described in #4557, I prefer to get this change in. Anyway, user can set kyuubi.frontend.bind.host=localhost in values.yaml to address the kubectl port forward issue. @dnskr WDYT?

[1] https://atbug.com/how-kubectl-port-forward-works/

phyyou · 2023-03-23T05:37:30Z

@pan3793 Hi, I deployed with my chart and in EKS cluster.

kubectl port-forward (localhost) and k8s service(pod ip) works on kyuubi.frontend.bind.host=0.0.0.0

why not use host = 0.0.0.0?

# parts of my kyuubi defaults in helm chart
        kyuubiDefaults: |
          # Kyuubi configurations https://kyuubi.apache.org/docs/latest/deployment/settings.html
          kyuubi.authentication=NONE
          # kyuubi.engine.share.level=CONNECTION -- Default USER
          kyuubi.metrics.reporters=PROMETHEUS
          kyuubi.frontend.bind.host=0.0.0.0
          kyuubi.frontend.bind.port=10009
          ##################################################
          # Required for k8s and spark CLUSTER mode
          kyuubi.engine.connection.url.use.hostname=false
          kyuubi.frontend.connection.url.use.hostname=false
          kyuubi.session.engine.startup.waitCompletion=true
          ##################################################

pan3793 · 2023-03-23T05:46:19Z

why not use host = 0.0.0.0?

Then the registered service info becomes /kyuubi/serviceUri=0.0.0.0:10009;version=1.7.0;sequence=0000000000, which does not make since

dnskr · 2023-03-23T11:02:35Z

So there is an issue for kubectl port forward, but considering issue described in #4557, I prefer to get this change in. Anyway, user can set kyuubi.frontend.bind.host=localhost in values.yaml to address the kubectl port forward issue. @dnskr WDYT?

Let's go forward and merge the change because kubectl port-forward issue has external root cause and should not affect production environments.

dnskr · 2023-03-23T11:02:54Z

LGTM

### _Why are the changes needed?_ Fix #4557. ### _How was this patch tested?_ - [ ] Add some test cases that check the changes thoroughly including negative and positive cases if possible - [x] Add screenshots for manual tests if appropriate After changing, the Kyuubi Server correctly binds the Pod IP. ``` KyuubiTBinaryFrontendService#64 - Starting and exposing JDBC connection at: jdbc:hive2://10.88.224.214:10009/ ``` - [ ] [Run test](https://kyuubi.readthedocs.io/en/master/develop_tools/testing.html#running-tests) locally before make a pull request Closes #4561 from pan3793/helm-host. Closes #4557 018a3a0 [Cheng Pan] [KYUUBI #4557][HELM] Kyuubi server should bind Pod IP by default Authored-by: Cheng Pan <chengpan@apache.org> Signed-off-by: Cheng Pan <chengpan@apache.org> (cherry picked from commit dbb741f) Signed-off-by: Cheng Pan <chengpan@apache.org>

pan3793 · 2023-03-23T11:08:32Z

Thanks all! Merged to master/1.7

phyyou · 2023-03-23T11:31:53Z

@pan3793

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>

pan3793 · 2023-03-23T11:40:52Z

@phyyou good catch, I forget this place.

Now, the kubectl port-forward works when user explicitly defines kyuubi.frontend.bind.host=localhost in values.yaml, is it easy to write a conditional expression in NOTES.txt? If yes, we can conditionally print it, otherwise I think we can simply delete it.

Would you like to send a follow up PR to update it?

phyyou · 2023-03-23T11:55:09Z

Thanks, @pan3793 . I'm glad I could help.

Regarding the conditional expression in NOTES.txt, I think it would be great to have it if it's feasible.

I'm definitely interested in contributing more to the improvement of the Helm chart. I'll send a follow-up PR with the changes and other improvements that I have in mind.

I think I can improve the kyuubiDefaults value

pan3793 · 2023-03-23T12:02:41Z

I think kyuubiDefaults option in values.yaml is not perfect structure and I can improve it.

Please go ahead, Kyuubi community is open to different thoughts, we appreciate the efforts of all contributors.

phyyou · 2023-03-23T12:15:22Z

Thanks @pan3793.

I went to just too far 😂...
I think it will be just simple improvements.

…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 <gydudwls@gmail.com> Co-authored-by: PHYYOU <34825352+phyyou@users.noreply.github.com> Signed-off-by: Cheng Pan <chengpan@apache.org>

…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 <gydudwls@gmail.com> Co-authored-by: PHYYOU <34825352+phyyou@users.noreply.github.com> Signed-off-by: Cheng Pan <chengpan@apache.org> (cherry picked from commit 0ebedda) Signed-off-by: Cheng Pan <chengpan@apache.org>

[KYUUBI apache#4557][HELM] Kyuubi server should bind Pod IP by default

018a3a0

pan3793 marked this pull request as ready for review March 20, 2023 01:14

zwangsheng approved these changes Mar 20, 2023

View reviewed changes

cxzl25 approved these changes Mar 20, 2023

View reviewed changes

pan3793 self-assigned this Mar 23, 2023

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

pan3793 added the module:kubernetes label Mar 23, 2023

pan3793 closed this in dbb741f Mar 23, 2023

phyyou mentioned this pull request Mar 23, 2023

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

Closed

1 task

pan3793 mentioned this pull request Mar 26, 2023

[Umbrella] Helm Chart improvements #4473

Open

4 tasks

[KYUUBI #4557][HELM] Kyuubi server should bind Pod IP by default #4561

[KYUUBI #4557][HELM] Kyuubi server should bind Pod IP by default #4561

Uh oh!

Conversation

pan3793 commented Mar 20, 2023

Why are the changes needed?

How was this patch tested?

Uh oh!

pan3793 commented Mar 20, 2023

Uh oh!

codecov-commenter commented Mar 20, 2023

Codecov Report

Uh oh!

dnskr commented Mar 20, 2023

Uh oh!

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

Uh oh!

Uh oh!

dnskr commented Mar 20, 2023

Uh oh!

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

Uh oh!

Uh oh!

Yikun commented Mar 23, 2023

Uh oh!

zwangsheng commented Mar 23, 2023

Uh oh!

pan3793 commented Mar 23, 2023

Uh oh!

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

Uh oh!

Uh oh!

pan3793 commented Mar 23, 2023

Uh oh!

dnskr commented Mar 23, 2023

Uh oh!

dnskr commented Mar 23, 2023

Uh oh!

pan3793 commented Mar 23, 2023

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

pan3793 commented Mar 23, 2023

Uh oh!

phyyou commented Mar 23, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

pan3793 commented Mar 20, 2023 •

edited

Loading

pan3793 commented Mar 22, 2023 •

edited

Loading

phyyou commented Mar 23, 2023 •

edited

Loading

phyyou commented Mar 23, 2023 •

edited

Loading

pan3793 commented Mar 23, 2023 •

edited

Loading

phyyou commented Mar 23, 2023 •

edited

Loading