[SPARK-22813][BUILD] Use lsof or /usr/sbin/lsof in run-tests.py #19998

kiszk · 2017-12-16T02:29:12Z

What changes were proposed in this pull request?

In the environment where /usr/sbin/lsof does not exist, ./dev/run-tests.py for maven causes the following error. This is because the current ./dev/run-tests.py checks existence of only /usr/sbin/lsof and aborts immediately if it does not exist.

This PR changes to check whether lsof or /usr/sbin/lsof exists.

/bin/sh: 1: /usr/sbin/lsof: not found

Usage:
 kill [options] <pid> [...]

Options:
 <pid> [...]            send signal to every <pid> listed
 -<signal>, -s, --signal <signal>
                        specify the <signal> to be sent
 -l, --list=[<signal>]  list all signal names, or convert one to a name
 -L, --table            list all signal names in a nice table

 -h, --help     display this help and exit
 -V, --version  output version information and exit

For more details see kill(1).
Traceback (most recent call last):
  File "./dev/run-tests.py", line 626, in <module>
    main()
  File "./dev/run-tests.py", line 597, in main
    build_apache_spark(build_tool, hadoop_version)
  File "./dev/run-tests.py", line 389, in build_apache_spark
    build_spark_maven(hadoop_version)
  File "./dev/run-tests.py", line 329, in build_spark_maven
    exec_maven(profiles_and_goals)
  File "./dev/run-tests.py", line 270, in exec_maven
    kill_zinc_on_port(zinc_port)
  File "./dev/run-tests.py", line 258, in kill_zinc_on_port
    subprocess.check_call(cmd, shell=True)
  File "/usr/lib/python2.7/subprocess.py", line 541, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '/usr/sbin/lsof -P |grep 3156 | grep LISTEN | awk '{ print $2; }' | xargs kill' returned non-zero exit status 123

How was this patch tested?

manually tested

kiszk · 2017-12-16T02:36:12Z

@srowen @HyukjinKwon could you please review this?

HyukjinKwon

LGTM otherwise

HyukjinKwon · 2017-12-16T02:36:49Z

dev/run-tests.py

+        cmd = ("lsof -P |grep %s | grep LISTEN "
+               "| awk '{ print $2; }' | xargs kill") % zinc_port
+        subprocess.check_call(cmd, shell=True)
+    except:


Could we catch the explicit exception?

Also, I think we could this like:

cmd = "%s -P |grep %s | grep LISTEN | awk '{ print $2; }' | xargs kill" ... lsof = "lsof" subprocess.check_call(cmd % (lsof zinc_port), shell=True) ... lsof = "/usr/sbin/lsof" subprocess.check_call(cmd % (lsof zinc_port), shell=True)

Yes, if the command does not exist, we can catch it as an exception. Thus, we can execute one of the two cases.

Yea, to use such a cmd is fine.

HyukjinKwon · 2017-12-16T03:31:11Z

dev/run-tests.py

+    try:
+        subprocess.check_call(cmd % ("lsof", zinc_port), shell=True)
+    except:
+        subprocess.call(cmd % ("/usr/sbin/lsof", zinc_port), shell=True)


Maybe, subprocess.call -> subprocess.check_call?

I intentionally use subprocess.call to continue the execution even if lsof and /usr/sbin/lsof do not exist. This is because it is ok for other steps if we fail to kill zinc.

WDYT?

Hm, but it changes what originally kill_zinc_on_port does though because now it is not guaranteed to kill it. I see the point but let's stick to the original behaviour, for now.

I see. Since this change is not strong preference, I will revert this change to keep the original behavior.

HyukjinKwon · 2017-12-16T04:28:59Z

dev/run-tests.py

-    cmd = ("/usr/sbin/lsof -P |grep %s | grep LISTEN "
-           "| awk '{ print $2; }' | xargs kill") % zinc_port
-    subprocess.check_call(cmd, shell=True)
+    cmd = "%s -P |grep %s | grep LISTEN | awk '{ print $2; }' | xargs kill"


Ah, @kiszk, I think we can actually use sparktestsupport.shellutils.which("...") too like what we do for java:

spark/dev/run-tests.py

Line 153 in 964e5ff

return java_exe if java_exe else which("java")

So, like ..

cmd = "%s -P |grep %s | grep LISTEN | awk '{ print $2; }' | xargs kill" lsof_exe = which("lsof") subprocess.check_call(cmd % (lsof_exe if lsof_exe else "/usr/sbin/lsof", zinc_port), shell=True)

I just double checked:

>>> lsof_exe = which("lsof") >>> cmd % (lsof_exe if lsof_exe else "/usr/sbin/lsof", zinc_port) "/usr/sbin/lsof -P |grep 1234 | grep LISTEN | awk '{ print $2; }' | xargs kill" >>> lsof_exe = which("lsof") >>> cmd % (lsof_exe if lsof_exe else "/usr/bin/lsof", zinc_port) "/usr/sbin/lsof -P |grep 1234 | grep LISTEN | awk '{ print $2; }' | xargs kill"

>>> lsof_exe = which("foo") >>> cmd % (lsof_exe if lsof_exe else "/usr/sbin/lsof", zinc_port) "/usr/sbin/lsof -P |grep 1234 | grep LISTEN | awk '{ print $2; }' | xargs kill" >>> lsof_exe = which("bar") >>> cmd % (lsof_exe if lsof_exe else "/usr/bin/lsof", zinc_port) "/usr/bin/lsof -P |grep 1234 | grep LISTEN | awk '{ print $2; }' | xargs kill"

Great catch, I also checked it.

>>> print(which("lsof")) /usr/bin/lsof >>> % ls /usr/bin/lsof /usr/sbin/lsof ls: cannot access '/usr/sbin/lsof': No such file or directory /usr/bin/lsof

SparkQA · 2017-12-16T05:45:25Z

Test build #84990 has finished for PR 19998 at commit 969bc22.

This patch fails SparkR unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-12-16T06:40:15Z

Test build #84992 has finished for PR 19998 at commit b384336.

This patch fails SparkR unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-12-16T07:16:02Z

Test build #84993 has finished for PR 19998 at commit 964e5ff.

This patch fails SparkR unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-12-16T08:05:01Z

Test build #84997 has finished for PR 19998 at commit 6c29a11.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

kiszk · 2017-12-16T08:12:39Z

retest this please

SparkQA · 2017-12-16T11:41:03Z

Test build #84999 has finished for PR 19998 at commit 6c29a11.

This patch fails SparkR unit tests.
This patch merges cleanly.
This patch adds no public classes.

kiszk · 2017-12-16T15:45:14Z

Should we retest this again?

HyukjinKwon · 2017-12-16T15:50:15Z

Let's wait for a bit. Let me restart this once we started to get passed. Seems globally failed in R's CRAN check.

kiszk · 2017-12-16T16:15:07Z

I see, thank you very much.

HyukjinKwon · 2017-12-17T15:25:48Z

retest this please

SparkQA · 2017-12-17T19:04:02Z

Test build #85026 has finished for PR 19998 at commit 6c29a11.

This patch fails SparkR unit tests.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2017-12-18T08:40:39Z

retest this please

SparkQA · 2017-12-18T12:01:32Z

Test build #85050 has finished for PR 19998 at commit 6c29a11.

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

HyukjinKwon · 2017-12-18T22:35:25Z

Merged to master.

initial commit

969bc22

kiszk changed the title ~~[SPARK-22377][BUILD] Use lsof or /usr/sbin/lsof in run-tests.py~~ [SPARK-22813][BUILD] Use lsof or /usr/sbin/lsof in run-tests.py Dec 16, 2017

HyukjinKwon approved these changes Dec 16, 2017

View reviewed changes

address review comment

b384336

HyukjinKwon reviewed Dec 16, 2017

View reviewed changes

address review comment

964e5ff

HyukjinKwon reviewed Dec 16, 2017

View reviewed changes

address review comment

6c29a11

asfgit closed this in 3a07eff Dec 18, 2017

[SPARK-22813][BUILD] Use lsof or /usr/sbin/lsof in run-tests.py #19998

[SPARK-22813][BUILD] Use lsof or /usr/sbin/lsof in run-tests.py #19998

Uh oh!

Conversation

kiszk commented Dec 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

kiszk commented Dec 16, 2017

Uh oh!

HyukjinKwon left a comment

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon Dec 16, 2017

Choose a reason for hiding this comment

Uh oh!

kiszk Dec 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon Dec 16, 2017

Choose a reason for hiding this comment

Uh oh!

kiszk Dec 16, 2017

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon Dec 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kiszk Dec 16, 2017

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon Dec 16, 2017

Choose a reason for hiding this comment

Uh oh!

kiszk Dec 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Dec 16, 2017

Uh oh!

SparkQA commented Dec 16, 2017

Uh oh!

SparkQA commented Dec 16, 2017

Uh oh!

SparkQA commented Dec 16, 2017

Uh oh!

kiszk commented Dec 16, 2017

Uh oh!

SparkQA commented Dec 16, 2017

Uh oh!

kiszk commented Dec 16, 2017

Uh oh!

HyukjinKwon commented Dec 16, 2017

Uh oh!

kiszk commented Dec 16, 2017

Uh oh!

HyukjinKwon commented Dec 17, 2017

Uh oh!

SparkQA commented Dec 17, 2017

Uh oh!

HyukjinKwon commented Dec 18, 2017

Uh oh!

SparkQA commented Dec 18, 2017

Uh oh!

HyukjinKwon commented Dec 18, 2017

Uh oh!

Uh oh!

kiszk commented Dec 16, 2017 •

edited

Loading

kiszk Dec 16, 2017 •

edited

Loading

HyukjinKwon Dec 16, 2017 •

edited

Loading

kiszk Dec 16, 2017 •

edited

Loading