Skip to content

[SPARK-28886][K8S] Fix the DepsTestsSuite with minikube 1.3.1 #25599

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.

Sign up for GitHub

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

Closed
holdenk wants to merge 2 commits into from
Closed

[SPARK-28886][K8S] Fix the DepsTestsSuite with minikube 1.3.1 #25599

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
036827c
Fix deps test to handle minikube v1.3.1 on OSX which returns extra te…
holdenk Aug 27, 2019
3cdea4f
Long line style fix
holdenk Aug 27, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion ...ion-tests/src/test/scala/org/apache/spark/deploy/k8s/integrationtest/DepsTestsSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,9 +224,20 @@ private[spark] trait DepsTestsSuite { k8sSuite: KubernetesSuite =>
}

private def getServiceUrl(serviceName: String): String = {
val fuzzyUrlMatcher = """^(.*?)([a-zA-Z]+://.*?)(\s*)$""".r
Eventually.eventually(TIMEOUT, INTERVAL) {
// ns is always available either random or provided by the user
Minikube.minikubeServiceAction(serviceName, "-n", kubernetesTestComponents.namespace, "--url")
val rawUrl = Minikube.minikubeServiceAction(
serviceName, "-n", kubernetesTestComponents.namespace, "--url")
val url = rawUrl match {
case fuzzyUrlMatcher(junk, url, extra) =>
logDebug(s"Service url matched junk ${junk} - url ${url} - extra ${extra}")
url
case _ =>
logWarning(s"Response from minikube ${rawUrl} did not match URL regex")
rawUrl
Copy link
Member

@srowen srowen Aug 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One question -- did this ever work before? meaning, would we ever get a response that doesn't match the regex legitimately? If so should it be a warning? Or is it that this doesn't work at all right now?

Copy link
Contributor Author

@holdenk holdenk Aug 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I've only tested on OSX and just incase someone elses setup doesn't match the regex (which I'm not sure would work) I figured a fall-through with a warning was a good approach. I also considered just throwing here.

Copy link
Member

@srowen srowen Aug 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity what does the minikube command return for you? wondering if it's something that varies by version or we have a problem with the command outputting extra stuff. Seems like it intends to only return a URL.

Copy link
Contributor

@erikerlandson erikerlandson Sep 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Talking with Stavros, this was not happening on previous version of minikube, so if we strip it, we just need to be careful it also works with earlier revs.

Copy link
Member

@srowen srowen Sep 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the problem is that it's * some://url:port[whitespace] then this looks like a fine fix. Would it help unblock other PRs?

Copy link
Contributor

@erikerlandson erikerlandson Sep 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be backward compatible. No junk prefix, then that subexpression is empty.

Copy link
Member

@srowen srowen Sep 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@holdenk OK to merge? seems like a decent fix

Copy link
Contributor Author

@holdenk holdenk Sep 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad for not responding sooner, yeah it's fine :)

}
url
}
}
}
Expand Down