[SPARK-27812][K8S] Bump K8S client version to 4.6.1 #26093

igorcalabria · 2019-10-11T15:45:50Z

What changes were proposed in this pull request?

Updated kubernetes client.

Why are the changes needed?

https://issues.apache.org/jira/browse/SPARK-27812
https://issues.apache.org/jira/browse/SPARK-27927

We need this fix fabric8io/kubernetes-client#1768 that was released on version 4.6 of the client. The root cause of the problem is better explained in #25785

Does this PR introduce any user-facing change?

Nope, it should be transparent to users

How was this patch tested?

This patch was tested manually using a simple pyspark job

from pyspark.sql import SparkSession

if __name__ == '__main__':
    spark = SparkSession.builder.getOrCreate()

The expected behaviour of this "job" is that both python's and jvm's process exit automatically after the main runs. This is the case for spark versions <= 2.4. On version 2.4.3, the jvm process hangs because there's a non daemon thread running

"OkHttp WebSocket https://10.96.0.1/..." #121 prio=5 os_prio=0 tid=0x00007fb27c005800 nid=0x24b waiting on condition [0x00007fb300847000]
"OkHttp WebSocket https://10.96.0.1/..." #117 prio=5 os_prio=0 tid=0x00007fb28c004000 nid=0x247 waiting on condition [0x00007fb300e4b000]

This is caused by a bug on kubernetes-client library, which is fixed on the version that we are upgrading to.

When the mentioned job is run with this patch applied, the behaviour from spark <= 2.4.3 is restored and both processes terminate successfully

dongjoon-hyun

Thank you for making a PR, @igorcalabria .
Currently, our testing Jenkins server is down. I'll review this PR after Jenkins is back.

ifilonenko · 2019-10-11T17:50:40Z

@igorcalabria thank you for writing this patch

I have ran this internally on my cluster and can confirm that non-daemon threads no longer block the JVM from exiting.

Against branch 2.4 and master:

I see the following non-daemon threads exist when running
spark-submit against the following pyspark job:

from pyspark.sql import SparkSession

if __name__ == '__main__':
    spark = SparkSession.builder.getOrCreate()

After the python process exited, the JVM process remained and I see the following using jstack:

"OkHttp WebSocket https://10.96.0.1/..." #121 prio=5 os_prio=0 tid=0x00007fb27c005800 nid=0x24b waiting on condition [0x00007fb300847000]
"OkHttp WebSocket https://10.96.0.1/..." #117 prio=5 os_prio=0 tid=0x00007fb28c004000 nid=0x247 waiting on condition [0x00007fb300e4b000]

Against this patch:

I no longer see the following non-daemon threads and the JVM is able to exit after the completion of the python process.

@mccheah @erikerlandson @felixcheung @holdenk we should backport this fix to branch 2.4 as well and cut a new release with this patch.

ifilonenko

Changes regarding consistency of kubernetes-client version and the bump of okhttp as well

ifilonenko · 2019-10-11T18:40:13Z

resource-managers/kubernetes/core/pom.xml

@@ -29,7 +29,7 @@
  <name>Spark Project Kubernetes</name>
  <properties>
    <sbt.project.name>kubernetes</sbt.project.name>
-    <kubernetes.client.version>4.4.2</kubernetes.client.version>
+    <kubernetes.client.version>4.6.0</kubernetes.client.version>


You need to change the kubernetes.client.version in resource-managers/kubernetes/integration-tests/pom.xml as well.

and bump:

<dependency> <groupId>com.squareup.okhttp3</groupId> <artifactId>okhttp</artifactId> - <version>3.8.1</version> + <version>3.12.0</version> </dependency>

as I believe there is a dependency here https://mvnrepository.com/artifact/io.fabric8/kubernetes-client/4.6.0

Ok. Is there a reason why okhttp is added as an explicit dependency(kubernetes-client already brings it)? If we really need a specific version, shouldn't we at least exclude it from kubernetes-client? I'm saying this because it's very easy to end up with a unexpected version on the classpath. kubernetes-client uses okhttp's version 3.12 since version 4.1.2 but we're only updating it now.

It may have been to make sure that another dependency on okhttp didn't win out and override to a lower version. It can just be updated here, or you can try removing it and examining the transitive dependencies afterwards. If it's already at 3.12.0, then it can likely be omitted.

That is a good point, @mccheah do we have an opinion on this? I am okay with removing the explicit dependency since kubernetes-client already brings it in. If there was a reason, I can't seem to remember, to explicitly state, let's exclude it?

This one looks okay to me.

agreed we should remove explicit dep to avoid a mismatch failure in the future

dongjoon-hyun · 2019-10-12T22:53:51Z

ok to test

dongjoon-hyun · 2019-10-12T22:57:10Z

@igorcalabria . The following claims is not matched with your PR description.

Standard kubernetes tests should validate this patch.

Please describe how to test this for the following for the reviewer. (Technically, this PR need a test case for that. Or, at least, manual testing process should be described in the PR description and should be the commit log.)

https://issues.apache.org/jira/browse/SPARK-27812
https://issues.apache.org/jira/browse/SPARK-27927

SparkQA · 2019-10-13T01:36:24Z

Test build #111979 has finished for PR 26093 at commit 7f15bac.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

igorcalabria · 2019-10-14T13:48:44Z

@igorcalabria . The following claims is not matched with your PR description.

Standard kubernetes tests should validate this patch.

Please describe how to test this for the following for the reviewer. (Technically, this PR need a test case for that. Or, at least, manual testing process should be described in the PR description and should be the commit log.)

https://issues.apache.org/jira/browse/SPARK-27812
https://issues.apache.org/jira/browse/SPARK-27927

@dongjoon-hyun A manual test is described above by @ifilonenko. If you want, I could add an integration test for this specific case. I skipped it because other PRs updating kubernetes-client version didn't add any tests and the README in kubernetes/integration-tests states that the framework is under review and subject to major changes

this matches the version used by kubernetes-client

SparkQA · 2019-10-14T14:01:39Z

Test build #112032 has finished for PR 26093 at commit 5d6bcd7.

This patch fails build dependency tests.
This patch merges cleanly.
This patch adds no public classes.

srowen · 2019-10-14T14:27:18Z

If it's easy to write a test for this, OK. If it's complex or would take a long time to run, I can see just verifying the small behavior change with a manual test.

SparkQA · 2019-10-14T14:35:07Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/17039/

SparkQA · 2019-10-14T14:51:55Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/17039/

mccheah · 2019-10-14T14:59:12Z

It's pretty difficult to cover all the code paths for changes in a downstream library. @dongjoon-hyun @srowen is there a standard protocol requiring an additional test for downstream dependencies? In this case, we're picking up a fix to an internal implementation detail in the downstream library, and I'd imagine we'd trust the downstream library to have already done its due diligence to test their fix. Regression tests exist to catch new problems introduced by the dependency bumps as we go along.

I'd agree that a manual test demonstrating the desired end behavior is sufficient here.

SparkQA · 2019-10-14T15:01:08Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/17042/

SparkQA · 2019-10-14T15:20:07Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/17042/

dongjoon-hyun · 2019-10-14T16:24:12Z

Hi, @igorcalabria and @mccheah .
I'm okay as long as the manual description becomes the PR description and the commit log.

Please describe how to test this for the following for the reviewer. (Technically, this PR need a test case for that. Or, at least, manual testing process should be described in the PR description and should be the commit log.)

SparkQA · 2019-10-14T17:12:21Z

Test build #112035 has finished for PR 26093 at commit b31b375.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

ifilonenko · 2019-10-14T18:21:26Z

+1 on my end, I have tested using manual tests. An integration test seems like a bit of overkill, although a simple jstack check in the test would be sufficient.

dongjoon-hyun

Hi, All.

Upgrading versions are okay, but the problem of this PR is to claim too much without a concrete background. This PR doesn't provide the way how to verify the claims in the PR description at all. Since the PR description will be a permanent commit log, we should not merge this PR AS-IS.

I'd like to recommend to remove all the claims from the PR description and narrow down the focus to the bug fixes inside the client library itself. Then, this PR can be considered as a proper proposal.

erikerlandson · 2019-10-15T19:21:54Z

@mccheah @erikerlandson @felixcheung @holdenk we should backport this fix to branch 2.4 as well and cut a new release with this patch.

Will there be a 2.4.5 ?

ifilonenko · 2019-10-15T19:42:54Z

@erikerlandson I think there should be. This bug will cause pyspark resources to hang unless some external process does resource reaping.

@dongjoon-hyun I agree, the PR description should change

srowen · 2019-10-15T20:25:07Z

There will be a 2.4.5 eventually, I'm sure. 2.4.x is a sort of "LTS" branch.

dongjoon-hyun · 2019-10-16T01:54:27Z

Of course, @erikerlandson . As @srowen said, it's the LTS branch.

Since 2.4.0 was released on November, 2018, branch-2.4 should be maintained at least May, 2020 (18 months). After that, as a LTS branch, I believe branch-2.4 will be maintained for a while.

Since 2.4.4 was released on September, 2019. 2.4.5 will arrive early 2020 and so on. I hope more regular releases. But, the 2.4.x release schedule heavily depends on the patches on branche-2.4 and the volunteer for release manager.

igorcalabria · 2019-10-16T13:41:21Z

@dongjoon-hyun I have updated the description to include a manual test. I believe that both jira issues fix claims are now supported by the manual test case provided in the description. Please let me know if you need anything else.

Cheers,

dongjoon-hyun · 2019-10-16T15:54:11Z

Thank you for updating, @igorcalabria ! I'll follow the verification steps and try to land this.

dongjoon-hyun

Unfortunately, during reviews, I found that this became outdated already. Could you update this PR with the latest version, @igorcalabria .

https://github.com/fabric8io/kubernetes-client/releases/tag/v4.6.1

dongjoon-hyun · 2019-10-16T22:51:46Z

Also, cc @jiangxb1987 since he is a release manager for 3.0.0-preview.

holdenk · 2019-10-17T06:10:40Z

I'm +1 on backporting even though it's a version change I think we need this in 2.4.X

holdenk

LGTM pending removal of explicit dep & jenkins.
Thanks for doing the manual verification @ifilonenko :)

holdenk · 2019-10-17T06:13:49Z

Actually if were going to update to 4.6.1 (and it looks like might as well) @ifilonenko would you have the cycles to manually verify with that version as well?

kubernetes-client already brings it and we don't need a specific version

igorcalabria · 2019-10-17T14:25:16Z

@dongjoon-hyun bumped version to 4.6.1. Following other reviewers suggestions, I also removed explicit okhttp dep.

srowen

Looks OK pending tests

SparkQA · 2019-10-17T15:04:49Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/17209/

SparkQA · 2019-10-17T15:21:54Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/17209/

SparkQA · 2019-10-17T16:56:25Z

Test build #112221 has finished for PR 26093 at commit a5eb702.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun · 2019-10-17T18:34:45Z

Is this tested with Python 3 on the master branch?

dongjoon-hyun

+1, LGTM. Merged to master.

dongjoon-hyun · 2019-10-17T19:25:48Z

Thank you so much everyone.
Especially, welcome, @igorcalabria . I added you to the Apache Spark contributor group.
Could you make a backporting PR against branch-2.4, @igorcalabria ?

# What changes were proposed in this pull request? Backport of #26093 to `branch-2.4` ### Why are the changes needed? https://issues.apache.org/jira/browse/SPARK-27812 https://issues.apache.org/jira/browse/SPARK-27927 We need this fix fabric8io/kubernetes-client#1768 that was released on version 4.6 of the client. The root cause of the problem is better explained in #25785 ### Does this PR introduce any user-facing change? No ### How was this patch tested? This patch was tested manually using a simple pyspark job ```python from pyspark.sql import SparkSession if __name__ == '__main__': spark = SparkSession.builder.getOrCreate() ``` The expected behaviour of this "job" is that both python's and jvm's process exit automatically after the main runs. This is the case for spark versions <= 2.4. On version 2.4.3, the jvm process hangs because there's a non daemon thread running ``` "OkHttp WebSocket https://10.96.0.1/..." #121 prio=5 os_prio=0 tid=0x00007fb27c005800 nid=0x24b waiting on condition [0x00007fb300847000] "OkHttp WebSocket https://10.96.0.1/..." #117 prio=5 os_prio=0 tid=0x00007fb28c004000 nid=0x247 waiting on condition [0x00007fb300e4b000] ``` This is caused by a bug on `kubernetes-client` library, which is fixed on the version that we are upgrading to. When the mentioned job is run with this patch applied, the behaviour from spark <= 2.4.0 is restored and both processes terminate successfully Closes #26152 from igorcalabria/k8s-client-update-2.4. Authored-by: igor.calabria <igor.calabria@ubee.in> Signed-off-by: Dongjoon Hyun <dhyun@apple.com>

Bump kubernetes client version

7f15bac

igorcalabria mentioned this pull request Oct 11, 2019

[SPARK-27812][CORE] Explicit System.exit after job's main #25785

Closed

dongjoon-hyun reviewed Oct 11, 2019

View reviewed changes

dongjoon-hyun added the KUBERNETES label Oct 11, 2019

ifilonenko suggested changes Oct 11, 2019

View reviewed changes

bump k8s client version in integration tests

7148bc8

bump okhttp version to 3.12

5d6bcd7

this matches the version used by kubernetes-client

update dep manifests

b31b375

Fokko approved these changes Oct 15, 2019

View reviewed changes

dongjoon-hyun requested changes Oct 15, 2019

View reviewed changes

srowen changed the title ~~[SPARK-27812][K8s] Bump client version~~ [SPARK-27812][K8S] Bump K8S client version to 4.6.0 Oct 15, 2019

dongjoon-hyun requested changes Oct 16, 2019

View reviewed changes

holdenk reviewed Oct 17, 2019

View reviewed changes

igorcalabria added 2 commits October 17, 2019 11:19

bump kubernetes-client to 4.6.1

d1e1b52

remove explict okhttp version

a5eb702

kubernetes-client already brings it and we don't need a specific version

srowen changed the title ~~[SPARK-27812][K8S] Bump K8S client version to 4.6.0~~ [SPARK-27812][K8S] Bump K8S client version to 4.6.1 Oct 17, 2019

srowen reviewed Oct 17, 2019

View reviewed changes

dongjoon-hyun approved these changes Oct 17, 2019

View reviewed changes

dongjoon-hyun closed this in 78bdcfa Oct 17, 2019

igorcalabria mentioned this pull request Oct 17, 2019

[SPARK-27812][K8S][2.4] Bump K8S client version to 4.6.1 #26152

Closed

[SPARK-27812][K8S] Bump K8S client version to 4.6.1 #26093

[SPARK-27812][K8S] Bump K8S client version to 4.6.1 #26093

Conversation

igorcalabria commented Oct 11, 2019 • edited Loading

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

dongjoon-hyun left a comment

Choose a reason for hiding this comment

ifilonenko commented Oct 11, 2019

Against branch 2.4 and master:

Against this patch:

ifilonenko left a comment

Choose a reason for hiding this comment

ifilonenko Oct 11, 2019

Choose a reason for hiding this comment

igorcalabria Oct 11, 2019

Choose a reason for hiding this comment

srowen Oct 14, 2019

Choose a reason for hiding this comment

ifilonenko Oct 14, 2019

Choose a reason for hiding this comment

dongjoon-hyun Oct 15, 2019

Choose a reason for hiding this comment

erikerlandson Oct 15, 2019

Choose a reason for hiding this comment

dongjoon-hyun commented Oct 12, 2019

dongjoon-hyun commented Oct 12, 2019 • edited Loading

SparkQA commented Oct 13, 2019

igorcalabria commented Oct 14, 2019

SparkQA commented Oct 14, 2019

srowen commented Oct 14, 2019

SparkQA commented Oct 14, 2019

SparkQA commented Oct 14, 2019

mccheah commented Oct 14, 2019

SparkQA commented Oct 14, 2019

SparkQA commented Oct 14, 2019

dongjoon-hyun commented Oct 14, 2019

SparkQA commented Oct 14, 2019

ifilonenko commented Oct 14, 2019

dongjoon-hyun left a comment • edited Loading

Choose a reason for hiding this comment

erikerlandson commented Oct 15, 2019

ifilonenko commented Oct 15, 2019

srowen commented Oct 15, 2019

dongjoon-hyun commented Oct 16, 2019

igorcalabria commented Oct 16, 2019

dongjoon-hyun commented Oct 16, 2019

dongjoon-hyun left a comment

Choose a reason for hiding this comment

dongjoon-hyun commented Oct 16, 2019

holdenk commented Oct 17, 2019

holdenk left a comment

Choose a reason for hiding this comment

holdenk commented Oct 17, 2019

igorcalabria commented Oct 17, 2019

srowen left a comment

Choose a reason for hiding this comment

SparkQA commented Oct 17, 2019

SparkQA commented Oct 17, 2019

SparkQA commented Oct 17, 2019

dongjoon-hyun commented Oct 17, 2019

dongjoon-hyun left a comment

Choose a reason for hiding this comment

dongjoon-hyun commented Oct 17, 2019

igorcalabria commented Oct 11, 2019 •

edited

Loading

dongjoon-hyun commented Oct 12, 2019 •

edited

Loading

dongjoon-hyun left a comment •

edited

Loading