#1329 update the operator to allow subpaths to be used with the spark ui ingress. #1330
Conversation
|
@liyinan926 would you mind having a look at this when you get a chance? I'm not sure what documentation I should add here, because I consider this more of a bugfix then a new feature |
…ble which is passed around, and configure the Ingress manifest to work with subpaths Signed-off-by: Tom Hellier <me@tomhellier.com>
TomHellier
force-pushed
the
1329-subpaths-with-ingress
branch
from
bf00835 to
6d47b89
Compare
|
I think this also resolves #1259 . The integration test appears to have failed because of some intermittent problem, but I cant seem to trigger a re-run. helm/kind-action#42 applying this seems to fix the kind issue |
TomHellier
force-pushed
the
1329-subpaths-with-ingress
branch
3 times, most recently
from
d2cb1f0 to
96c4129
Compare
| ingressURL, err := getSparkUIingressURL(c.ingressURLFormat, app.GetName(), app.GetNamespace()) | ||
| if err != nil { | ||
| glog.Errorf("failed to run get ingress url %s/%s: %v", app.Namespace, app.Name, err) | ||
| } |
There was a problem hiding this comment.
An else should be used here as the code below should only be run if err == nil.
| if err != nil { | ||
| return nil, err | ||
| } | ||
| var ingressURLPath = ingressURL.Path |
There was a problem hiding this comment.
ingressURLPath := ingressURI.Path
| } | ||
| var ingressURLPath = ingressURL.Path | ||
| // If we're serving on a subpath, we need to ensure we create capture groups | ||
| if ingressURL.Path != "" && ingressURL.Path != "/" { |
There was a problem hiding this comment.
Use ingressURLPath here.
| @@ -115,11 +120,19 @@ func createSparkUIIngress(app *v1beta2.SparkApplication, service SparkService, i | |||
| if len(ingressResourceAnnotations) != 0 { | |||
| ingress.ObjectMeta.Annotations = ingressResourceAnnotations | |||
| } | |||
|
|
|||
| // If we're serving on a subpath, we need to ensure we use the capture groups | |||
| if ingressURL.Path != "" && ingressURL.Path != "/" { | |||
There was a problem hiding this comment.
Ditto, use ingressURLPath .
| @@ -542,7 +554,10 @@ func TestCreateSparkUIIngress(t *testing.T) { | |||
| app: app1, | |||
| expectedIngress: SparkIngress{ | |||
| ingressName: fmt.Sprintf("%s-ui-ingress", app1.GetName()), | |||
| ingressURL: "ingress.clusterName.com/" + app1.GetNamespace() + "/" + app1.GetName(), | |||
| ingressURL: parseURLAndAssertError("ingress.clusterName.com/"+app1.GetNamespace()+"/"+app1.GetName(), t), | |||
There was a problem hiding this comment.
do you know if this will also work when there is no hostname, eg:
ingressUrlFormat="/{{$appNamespace}}/{{$appName}}"
There was a problem hiding this comment.
I tried this in the test and looks like it does! 🎉
| app.Spec.SparkConf = make(map[string]string) | ||
| } | ||
| app.Spec.SparkConf["spark.ui.proxyBase"] = ingressURL.Path | ||
| app.Spec.SparkConf["spark.ui.proxyRedirectUri"] = "/" |
There was a problem hiding this comment.
might be worth covering this in a test?
There was a problem hiding this comment.
I've written a test which covers this (sort of)... however, it looks like when I'm writing to SparkConf here, the contents of SparkConf isn't written back to the kubernetes resource. I'm unsure whether it is worth syncing this contents back to the SparkApplication when this has been done
…e subpath condition, and protect against empty paths Signed-off-by: Tom Hellier <me@tomhellier.com>
TomHellier
force-pushed
the
1329-subpaths-with-ingress
branch
from
96c4129 to
e3b0b5e
Compare
|
i'm really missing this feature! can we merge it? |
…he spark ui ingress. (kubeflow#1330) * Slightly restructure the way the ingressURL is created, and the variable which is passed around, and configure the Ingress manifest to work with subpaths Signed-off-by: Tom Hellier <me@tomhellier.com> * fix kubeflow#1329 unit tests now capture groups are being added in the subpath condition, and protect against empty paths Signed-off-by: Tom Hellier <me@tomhellier.com>
Slightly restructure the way the ingressURL is created, and the variable which is passed around, and configure the Ingress manifest to work with subpaths
Fixes #1329
This allows me to use the following ingressUrlFormat
spark.example.com/sparkjobs/{{$appName}}, and the urlspark.example.com/sparkjobs/spark-pi-1629482011066826445 now displays the spark ui correctly.