Skip to content
This repository has been archived by the owner on Jan 9, 2020. It is now read-only.

Conversation

lins05
Copy link

@lins05 lins05 commented Feb 9, 2017

Fix #87

What changes were proposed in this pull request?

Update the javascript of the executors page so it can load successfully when visited via kubectl proxy.

How was this patch tested?

Manually build, deploy, and check the executors page in the browser. Tested with chrome/firefox/safari and win10 edge browser.

Before this patch: The executors page is blank

After this patch:

screen shot 2017-02-09 at 3 44 25 pm

@ash211
Copy link

ash211 commented Feb 9, 2017

Repro'd the bug before the patch, now will test if it works after the patch

@ash211
Copy link

ash211 commented Feb 9, 2017

Yep the executors page loads now and there's no longer the 404 to the malformatted endpoint! I haven't tested in YARN though to make sure this diff doesn't break viewing the page through the YARN proxy.

@lins05 do you have a YARN cluster you can run this code on to test? If not I'm comfortable merging as is.

@tnachen
Copy link

tnachen commented Feb 10, 2017

@lins05 have you tried setting APPLICATION_UI_PROXY_BASE? I'm not sure if it applies to executor UI, but I remember it was meant to be able to add a base URL for UI.

@ash211
Copy link

ash211 commented Feb 10, 2017

Couldn't find that exact config, but I do see APPLICATION_WEB_PROXY_BASE and spark.ui.proxyBase which are used in history server and the YARN AM. It's unlikely either of those are currently respected in the k8s backend though.

@lins05
Copy link
Author

lins05 commented Feb 10, 2017

@tnachen @ash211

thanks for the tips of spark.ui.proxyBase and APPLICATION_WEB_PROXY_BASE. I'll take a look of them to see if they can be used.

@ash211 I'll test it against YARN to ensure this patch doesn't cause any regression.

@iyanuobidele
Copy link

Thanks for taking this on @lins05

I would also test things from my end to make sure we don't break anything.

@ash211
Copy link

ash211 commented Feb 10, 2017

Thank you for also testing this change @iyanuobidele -- please let us know what you find.

@lins05
Copy link
Author

lins05 commented Feb 11, 2017

@ash211 I've tested this change against a yarn cluster, and the executors page loads fine through the yarn resource manager proxy.

@tnachen I have read about the APPLICATION_WEB_PROXY_BASE. Seems this environment variable is set in yarn AM when yarn starts the container (javadoc here). We may set this env when the rest submission server launches the driver process (e.g. set it to /api/v1/proxy/namespaces/default/services/<appId>:4040, so we can take advantage of the existing logic that respects it. I did that in the code and build it to run, just to find that the current javascript logic that tries to find the "proxy" keyword in the url still generates a wrong result. So I guess we still the changes in this PR.

@ash211
Copy link

ash211 commented Feb 13, 2017

Thanks for the additional testing Shuai. It sounds like APPLICATION_WEB_PROXY_BASE is only part of the solution, and not even required given that the JS change alone is enough to make this work.

I'm thinking we should merge just this JS change and if we can articulate an improvement that something like APPLICATION_WEB_PROXY_BASE would give us then we consider adding that in a subsequent PR.

Thanks for the contribution @lins05 and the config callout @tnachen !

@ash211 ash211 merged commit d297df9 into apache-spark-on-k8s:k8s-support-alternate-incremental Feb 13, 2017
@lins05 lins05 deleted the fix-web-ui-executors-page-through-kubectl-proxy branch May 29, 2017 02:57
ifilonenko pushed a commit to ifilonenko/spark that referenced this pull request Feb 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants