Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide a way to disable the fix for LIVY-697 #388

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

idzikovsky
Copy link
Contributor

@idzikovsky idzikovsky commented Jan 31, 2023

The fix for the LIVY-697 (#246) works fine for the problem it was designed to solve.
But it causes another problem:
the host reported to Livy Server from the RSC Driver is never used.

I know that this is kind of unsupported for now, but I'm trying to run Livy in Kubernetes in the namespace with Istio injections enabled.

And after fix for LIVY-697 it stopped working (I use Livy 0.7 with patches from #249), howerver when I used patches from #167 applied on Livy 0.5 everything was fine.

Root cause
In the fix for #167 we are explicitly setting the host that will be reported by RSC Driver back to Livy Server. And because of the fix for LIVY-697 a value of this property is ignored.

And even in that case, everything works fine, until we try to enable Istio: as Istio wraps all TCP traffic through a proxy, the following piece of code would set the host property not to the host of the actual RSC Driver, but to the IP address of TCP proxy server which would be 127.0.0.6:

        InetSocketAddress insocket = (InetSocketAddress) ctx.channel().remoteAddress();
        host = insocket.getAddress().getHostAddress();

So it would be good to add some option to disable the workaround that was made in the scope of LIVY-697.
Especially taking into account that initially Livy was designed to connect to the driver using host/port reported by the driver, and not by using hacks that get the host from a remote connection channel.

In my fix, I'm providing an option to enable the fix for LIVY-697 and leave it enabled by default to keep the current behavior as is.

How was this patch tested?

Manual

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant