[ZEPPELIN-2075] Can't stop infinite `while` statement in pyspark Interpreter. #1985

astroshim · 2017-02-07T15:02:12Z

What is this PR for?

If following code runs with Pyspark Interpreter, there is no way to cancel except Zeppelin Server restart.

%spark.pyspark
import time

while True:
    time.sleep(1)
    print("running..")

What type of PR is it?

Bug Fix | Improvement

What is the Jira issue?

https://issues.apache.org/jira/browse/ZEPPELIN-2075

How should this be tested?

Run above code with Pyspark Interpreter and try to cancel.

Screenshots (if appropriate)

before
after

Questions:

Does the licenses files need update? no
Is there breaking changes for older versions? no
Does this needs documentation? no

Leemoonsoo

Thanks for great contribution. I left few comments. Please take a look.
And would it be too difficult to have an unittest?

Leemoonsoo · 2017-02-07T22:37:27Z

spark/src/main/java/org/apache/zeppelin/spark/PySparkInterpreter.java

  @Override
  public void cancel(InterpreterContext context) {
    SparkInterpreter sparkInterpreter = getSparkInterpreter();
    sparkInterpreter.cancel(context);
+    try {
+      interrupt();


I think
In case of Spark job is running, need to wait sparkInterpreter.cancel(context) and shouldn't call interrupt.
In case of Spark job is not running, then interrupt() need to be called.
what do you think?

Leemoonsoo · 2017-02-07T22:37:52Z

spark/src/main/java/org/apache/zeppelin/spark/PySparkInterpreter.java

+    try {
+      interrupt();
+    } catch (IOException e) {
+      e.printStackTrace();


Shell we use logger to print log?

Leemoonsoo · 2017-02-07T23:19:28Z

spark/src/main/resources/python/zeppelin_pyspark.py


+
+signal.signal(signal.SIGINT, handler_stop_signals)


I think it works without this line and handler_stop_signals() function

You're right. It seems that spark has already signal handler.
Thanks.

Leemoonsoo · 2017-02-11T07:30:12Z

@astroshim can you take a look CI error ?

karuppayya · 2017-02-11T16:27:24Z

@astroshim This would be a problem in case of scala snippet also(a loop with a print in other words , that which runs only within driver). Is there a possible fix such that all interpreters(scala, python) can benefit.

felixcheung · 2017-02-12T19:45:01Z

spark/src/main/java/org/apache/zeppelin/spark/PySparkInterpreter.java

+      Runtime.getRuntime().exec("kill -SIGINT " + pythonPid);
+    } else {
+      logger.warn("Non UNIX/Linux system, close the interpreter");
+      close();


doesn't this leave the interpreter in a bad state?

Yes you're right, but ERROR status would be a problem?

felixcheung · 2017-02-12T19:46:41Z

spark/src/main/java/org/apache/zeppelin/spark/PySparkInterpreter.java

+      logger.info("Sending SIGINT signal to PID : " + pythonPid);
+      Runtime.getRuntime().exec("kill -SIGINT " + pythonPid);
+    } else {
+      logger.warn("Non UNIX/Linux system, close the interpreter");


I'm not sure - why is it going to be non Unix if pythonPid == -1?

That's because creating process in windows creates a pseudo process (thread), and the pid is a negative number. ``

astroshim · 2017-02-15T05:38:48Z

Hello. Sorry for late response.
@Leemoonsoo Let me fix CI error.
@karuppayya Let me fix scala too in another PR.

Leemoonsoo · 2017-02-18T02:49:16Z

spark/src/test/java/org/apache/zeppelin/spark/PySparkInterpreterTest.java

+      } catch (InterruptedException e) {
+        e.printStackTrace();
+      }
+      pySparkInterpreter.cancel(context);


Don't we need to something that checks if thread is actually finished or not?
I think thread.join() is required here. otherwise test will not fail even if job is not cancelled.

Thank you for pointing properly out. let me fix it.

Leemoonsoo · 2017-02-19T08:21:00Z

CI test failure is not related (fixed by #2033).

LGTM and merge to master if no further discussions.

zjffdu · 2017-02-19T08:40:08Z

@astroshim It looks like the python process will be killed, does it mean the interpreter is closed and the state of all the previous paragraphs is lost ? I try the cancel function in jupyter, it seems jupyter just cancel the current cell but keep the python process alive, the previous cell's state is still in the python process. Maybe there's a better approach for this.

zjffdu · 2017-02-19T09:12:19Z

Sorry, it's my misunderstanding. It just interrupt the process rather than kill it. The behavior is consistent with jupyter. It's awesome.

astroshim · 2017-02-19T10:02:16Z

@zjffdu yes, this PR is not kill the python process.
Thank you for reviewing!

FireArrow · 2017-02-20T09:39:48Z

Sorry for the late comment.
This solution wouldn't work with impersonation, as the zeppelin user wouldn't have permission to interrupt another users process (unless zeppelin run as root, which comes with a whole set of other problems). I suggest you read in the impersonation command, if there is one, and use that to send of the kill -SIGINT <pid>.

astroshim · 2017-02-23T05:52:29Z

@FireArrow Note has permission in the zeppelin so if user doesn't have write permission on that note, user can't cancel job.

FireArrow · 2017-02-24T10:30:53Z

@astroshim That's not what I'm talking about. Assume the following:
Zeppelin run as user "zeppelin"
In zeppelin-env.sh: export ZEPPELIN_IMPERSONATE_CMD='sudo -H -i -u ${ZEPPELIN_IMPERSONATE_USER} bash -c '
User zeppelin have permission to execute paswordless sudo to local users allowed to log into Zeppelin

Interpreters will with this run as the user starting the process, and the zeppelin user will not have permission to send any signal to them.
My suggestion is to execute the kill with $ZEPPELIN_IMPERSONATE_CMD to make sure the kill is executed in the same way as the interpreter was started in the first place.

astroshim · 2017-02-27T05:43:55Z

@FireArrow Yea, right. I understood. Let me fix it.
Thanks.

astroshim · 2017-02-27T14:38:20Z

@FireArrow I think it seems not can be possible to know if impersonation mode or not in Interpreter side(I couldn't find it) and checking cancel paragraph for impersonation is also not good idea (Interpreter sever can be launched itself not the same system with ZeppelinServer)
So I think that the canceling job can run only by user who launched interpreter process can be one option. and In this way, the other user can't stop the job in shared mode.
What do you think?

FireArrow · 2017-02-27T14:46:55Z

I have basically only set up zeppelin in isolated mode with impersonation, so I'm not sure what is the "expected" behavior is outside of that. Someone else will have to pitch in here.

astroshim · 2017-02-28T00:12:54Z

@Leemoonsoo @zjffdu @felixcheung @karuppayya Could you guys help to review about the impersonation mode?

zjffdu · 2017-02-28T01:25:16Z

Even in the impersonation mode, it is the process owner to interrupt the process. Here it is the interpreter process owner interrupt the process instead of the zeppelin server owner. But I can not verify it in impersonation mode. It seems the impersonation mode is broken. I got the following error, can anyone else verify the impersonation mode ?

ERROR [2017-02-28 09:20:18,882] ({pool-2-thread-2} Job.java[run]:194) - Job failed
org.apache.zeppelin.interpreter.InterpreterException: org.apache.hadoop.security.authorize.AuthorizationException: User: user1 is not allowed to impersonate user1

astroshim · 2017-02-28T01:47:46Z

@zjffdu I tested impersonation mode followed http://zeppelin.apache.org/docs/0.7.0/manual/userimpersonation.html and worked well.
BTW for now anybody can interrupt to interpreter process. and that is @FireArrow's worrying. Am i right?

zjffdu · 2017-02-28T02:21:24Z

User zeppelin have permission to execute paswordless sudo to local users allowed to log into Zeppelin
Interpreters will with this run as the user starting the process, and the zeppelin user will not have permission to send any signal to them.

I think @FireArrow 's concern is that zeppelin user can kill the process. Actually it is not the user zeppelin send the signal, it is the interpreter process owner. So I don't think there's any permission issue here. @FireArrow Please help confirm.

astroshim · 2017-03-02T11:49:49Z

@FireArrow ping.

FireArrow · 2017-03-02T12:06:29Z

Terribly sorry for late answer! Not sure why I didn't get any notifications about your answers 😕
@zjffdu I haven't looked into who actually sends the signal, but if it is as you say, with the user owning the interpreter process sending the signal, it should be fine.
My worry was that it was sent by the user running the server, and that if that user isn't root (which would be a terrible idea for other reasons) the OS wouldn't allow sending signals to another users process.
And it is tricky to set up impersonation, but as far as I know it is not broken. I only have my production environment with this set up right now. I suggest not using spark to test with. The sudo approach clashes with the spark impersonation, so if you really want to try that you have to set export ZEPPELIN_IMPERSONATE_SPARK_PROXY_USER=false, and find a way to make sure the user starting the interpreter has a kerberos ticket (if you need that)

astroshim · 2017-03-04T13:42:42Z

@FireArrow Thank you for confirm your idea and thank @zjffdu help to review!

jongyoul · 2017-03-14T06:31:04Z

Will merge it

…rpreter. ### What is this PR for? If following code runs with Pyspark Interpreter, there is no way to cancel except Zeppelin Server restart. ``` %spark.pyspark import time while True: time.sleep(1) print("running..") ``` ### What type of PR is it? Bug Fix | Improvement ### What is the Jira issue? https://issues.apache.org/jira/browse/ZEPPELIN-2075 ### How should this be tested? Run above code with Pyspark Interpreter and try to cancel. ### Screenshots (if appropriate) - before ![pyspark before](https://cloud.githubusercontent.com/assets/3348133/22696141/615c1206-ed90-11e6-9bbb-339ecdec73fc.gif) - after ![pyspark after](https://cloud.githubusercontent.com/assets/3348133/22696168/70899172-ed90-11e6-99e1-342eb4094b2c.gif) ### Questions: * Does the licenses files need update? no * Is there breaking changes for older versions? no * Does this needs documentation? no Author: astroshim <hsshim@zepl.com> Closes #1985 from astroshim/ZEPPELIN-2075 and squashes the following commits: 84bf09a [astroshim] fix testcase bc12eaa [astroshim] pass pid to java b60d89a [astroshim] Merge branch 'master' into ZEPPELIN-2075 f26eacf [astroshim] add test-case for canceling. c0cac4e [astroshim] fix logging 678c183 [astroshim] remove signal handler 65d8cc6 [astroshim] init python pid variable 6731e56 [astroshim] add signal to cancel job (cherry picked from commit 9f22db9) Signed-off-by: Jongyoul Lee <jongyoul@apache.org>

astroshim added 2 commits February 7, 2017 23:47

add signal to cancel job

6731e56

init python pid variable

65d8cc6

Leemoonsoo reviewed Feb 7, 2017

View reviewed changes

astroshim added 2 commits February 8, 2017 13:23

remove signal handler

678c183

fix logging

c0cac4e

add test-case for canceling.

f26eacf

astroshim closed this Feb 12, 2017

astroshim reopened this Feb 12, 2017

felixcheung reviewed Feb 12, 2017

View reviewed changes

astroshim added 2 commits February 15, 2017 15:06

Merge branch 'master' into ZEPPELIN-2075

b60d89a

pass pid to java

bc12eaa

astroshim closed this Feb 16, 2017

astroshim reopened this Feb 16, 2017

Leemoonsoo reviewed Feb 18, 2017

View reviewed changes

fix testcase

84bf09a

astroshim closed this Feb 19, 2017

astroshim reopened this Feb 19, 2017

asfgit closed this in 9f22db9 Mar 14, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ZEPPELIN-2075] Can't stop infinite `while` statement in pyspark Interpreter. #1985

[ZEPPELIN-2075] Can't stop infinite `while` statement in pyspark Interpreter. #1985

astroshim commented Feb 7, 2017

Leemoonsoo left a comment

Leemoonsoo Feb 7, 2017 •

edited

Loading

Leemoonsoo Feb 7, 2017

Leemoonsoo Feb 7, 2017 •

edited

Loading

astroshim Feb 8, 2017

Leemoonsoo commented Feb 11, 2017

karuppayya commented Feb 11, 2017

felixcheung Feb 12, 2017

astroshim Feb 16, 2017

felixcheung Feb 12, 2017

astroshim Feb 16, 2017

astroshim commented Feb 15, 2017

Leemoonsoo Feb 18, 2017

astroshim Feb 18, 2017

Leemoonsoo commented Feb 19, 2017

zjffdu commented Feb 19, 2017

zjffdu commented Feb 19, 2017

astroshim commented Feb 19, 2017

FireArrow commented Feb 20, 2017

astroshim commented Feb 23, 2017

FireArrow commented Feb 24, 2017 •

edited

Loading

astroshim commented Feb 27, 2017

astroshim commented Feb 27, 2017

FireArrow commented Feb 27, 2017

astroshim commented Feb 28, 2017

zjffdu commented Feb 28, 2017

astroshim commented Feb 28, 2017

zjffdu commented Feb 28, 2017 •

edited

Loading

astroshim commented Mar 2, 2017

FireArrow commented Mar 2, 2017

astroshim commented Mar 4, 2017

jongyoul commented Mar 14, 2017



		signal.signal(signal.SIGINT, handler_stop_signals)

[ZEPPELIN-2075] Can't stop infinite while statement in pyspark Interpreter. #1985

[ZEPPELIN-2075] Can't stop infinite while statement in pyspark Interpreter. #1985

Conversation

astroshim commented Feb 7, 2017

What is this PR for?

What type of PR is it?

What is the Jira issue?

How should this be tested?

Screenshots (if appropriate)

Questions:

Leemoonsoo left a comment

Choose a reason for hiding this comment

Leemoonsoo Feb 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Leemoonsoo Feb 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Leemoonsoo commented Feb 11, 2017

karuppayya commented Feb 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astroshim commented Feb 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Leemoonsoo commented Feb 19, 2017

zjffdu commented Feb 19, 2017

zjffdu commented Feb 19, 2017

astroshim commented Feb 19, 2017

FireArrow commented Feb 20, 2017

astroshim commented Feb 23, 2017

FireArrow commented Feb 24, 2017 • edited Loading

astroshim commented Feb 27, 2017

astroshim commented Feb 27, 2017

FireArrow commented Feb 27, 2017

astroshim commented Feb 28, 2017

zjffdu commented Feb 28, 2017

astroshim commented Feb 28, 2017

zjffdu commented Feb 28, 2017 • edited Loading

astroshim commented Mar 2, 2017

FireArrow commented Mar 2, 2017

astroshim commented Mar 4, 2017

jongyoul commented Mar 14, 2017

[ZEPPELIN-2075] Can't stop infinite `while` statement in pyspark Interpreter. #1985

[ZEPPELIN-2075] Can't stop infinite `while` statement in pyspark Interpreter. #1985

Leemoonsoo Feb 7, 2017 •

edited

Loading

Leemoonsoo Feb 7, 2017 •

edited

Loading

FireArrow commented Feb 24, 2017 •

edited

Loading

zjffdu commented Feb 28, 2017 •

edited

Loading