[SPARK-29720][CORE] Add linux condition to make ProcfsMetricsGetter more complete #26365

ulysses-you · 2019-11-02T02:29:05Z

What changes were proposed in this pull request?

Add Shell.LINUX condition . Related pr 22612 .

Why are the changes needed?

The proc is only support for linux. We can just mkdir /proc on mac os and spark cannot recognize what is . So it should add a linux condition .

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Exists UT.

ulysses-you · 2019-11-02T02:33:48Z

cc @squito @rezasafi

AmplabJenkins · 2019-11-02T02:35:32Z

Can one of the admins verify this patch?

ulysses-you · 2019-11-02T02:37:09Z

Also cc @maropu @dongjoon-hyun

maropu · 2019-11-02T03:15:55Z

away from a keyboard now, so will check tonight. cc: @viirya

maropu · 2019-11-02T23:41:43Z

This is not my area though, is this any harm without this pr? It seems these metrics are turned off when exception caught:

spark/core/src/main/scala/org/apache/spark/executor/ProcfsMetricsGetter.scala

Line 81 in be022d9

case e: SparkException =>

ulysses-you · 2019-11-03T03:12:48Z

This catch just confirm echo $PPID is support for current os . But not sure whether current os support porc. E.g. mac os, echo $PPID can return the right pid.
Add Linux condition is just to avoid that other os has the same dir like /proc/$pid/stat but in different use case.

maropu

ping @squito @rezasafi

maropu · 2019-11-03T04:23:27Z

core/src/main/scala/org/apache/spark/executor/ProcfsMetricsGetter.scala

@@ -26,6 +26,8 @@ import scala.collection.mutable
 import scala.collection.mutable.ArrayBuffer
 import scala.util.Try

+import org.apache.hadoop.util.Shell


How about checking os.name instead of using hadoop-commons?

maropu · 2019-11-03T04:23:53Z

core/src/main/scala/org/apache/spark/executor/ProcfsMetricsGetter.scala

@@ -62,13 +64,14 @@ private[spark] class ProcfsMetricsGetter(procfsDir: String = "/proc/") extends L
        SparkEnv.get.conf.get(config.EVENT_LOG_STAGE_EXECUTOR_METRICS)
      val shouldLogStageExecutorProcessTreeMetrics =
        SparkEnv.get.conf.get(config.EVENT_LOG_PROCESS_TREE_METRICS)
-      procDirExists.get && shouldLogStageExecutorProcessTreeMetrics && shouldLogStageExecutorMetrics
+      procDirExists.get && shouldLogStageExecutorProcessTreeMetrics &&
+        shouldLogStageExecutorMetrics && Shell.LINUX


Let's move the os check in the first condition.

viirya

Why do we make a /proc?

viirya · 2019-11-03T04:43:02Z

core/src/main/scala/org/apache/spark/executor/ProcfsMetricsGetter.scala

@@ -62,13 +64,14 @@ private[spark] class ProcfsMetricsGetter(procfsDir: String = "/proc/") extends L
        SparkEnv.get.conf.get(config.EVENT_LOG_STAGE_EXECUTOR_METRICS)
      val shouldLogStageExecutorProcessTreeMetrics =
        SparkEnv.get.conf.get(config.EVENT_LOG_PROCESS_TREE_METRICS)
-      procDirExists.get && shouldLogStageExecutorProcessTreeMetrics && shouldLogStageExecutorMetrics
+      procDirExists.get && shouldLogStageExecutorProcessTreeMetrics &&
+        shouldLogStageExecutorMetrics && Shell.LINUX


Are we sure to support only Linux? Other Unix-like systems which have /proc?

In hadoop ProcfsMetrics, it checks this ProcfsBasedProcessTree and it already exists long time.

Looks like in isProcfsAvailable, we check /proc existence instead of checking Linux like hadoop. Isn't it enough? And for other Unit-like systems which have /proc? We do not need to follow ProcfsBasedProcessTree.

AFAIC the procfs is the linux feature, other unix-like os may be try to compatible with it. Check the /proc dir is not a better idea than check linux os. One point is that, someone can change the /proc information which os not support procfs native and it will be a vulnerability.

If /proc is in other Unix-like systems, this change actually removes the support on these systems. Unless we are sure current code does not work for such systems, I think we should not simply remove their support.

srowen · 2019-11-04T15:41:50Z

Yeah, I don't see the point here. If you've tried to fake /proc then that's the problem. I'm slightly concerned this will make it stop working on an OS that is UNIX-like but without "Linux" in the name.

rezasafi · 2019-11-04T17:54:29Z

I agree with others here. prosfs based systems are not just linux systems ( I have a vague memory that this was discussed when the pr for prosfs metrics was opened) and also this feature isn't enabled by default. But I think we need to add documentation for the config saying that the feature is only working on prosfs based systems. We missed that doc at the time of adding the feature. I mean adding a ".doc" for the config in package.scala

rezasafi · 2019-11-04T18:06:35Z

Actually I am not sure whether we will report the correct metrics in other procfsbased systems since our computation is based on http://man7.org/linux/man-pages/man5/proc.5.html which is linux dependent. I haven't tested this feature on non-linux based systems. So I think it make sense to check whether the OS is actually linux. Thanks for the PR.

ulysses-you · 2019-11-05T00:04:02Z

Thanks for review.
I agree your points otherwise. But If we not sure what will happen in other os, we should forbid the behavior. Isn't it ?

rezasafi · 2019-11-05T00:34:13Z

@ulysses-you In my last comment I said this PR make sense. Although I juts checked the Solaris. There is no stat file there so the prosfcs metrics will raise an exception and return all zero because of the check in this line. It won't give back wrong info to the user in case of solaris. The same may be correct for other OSes. Still no harm in having your change if others also agree.

ulysses-you · 2019-11-05T00:58:13Z

@rezasafi Oh I see... pending the last review.

srowen · 2019-11-06T15:13:01Z

The problem isn't Solaris, but rather, Linux distros that for whatever reason don't begin with "Linux" in os.name. I think it's not worth possibly breaking this.

squito · 2019-11-06T17:52:29Z

I agree with Sean, I'm not really sure what the value is of adding this, and it'll potentially prevent the feature from being used in cases it should be fine.

Suppose somebody does create /proc on macos, and then turns on these metrics without this change -- how bad is it? It seems like one time you'll try to grab the metrics and it'll fail. It will then log an exception and set isAvailable = false. Then it won't ever try to grab those metrics again, so its won't spam those exceptions endlessly. (if somebody has manually created a /proc that actually looks like the real /proc -- well, they'll get bogus metrics, but that's their problem.)

ulysses-you · 2019-11-07T00:11:02Z

Thanks for review, I will close this.

ghost · 2020-09-25T21:39:32Z

Is this why I keep getting the following warning on windows?

WARN ProcfsMetricsGetter: Exception when trying to compute pagesize, as a result reporting of ProcessTree metrics is stopped

Add linux condition

266bd10

dongjoon-hyun added the SPARK CORE label Nov 3, 2019

maropu reviewed Nov 3, 2019

View reviewed changes

viirya reviewed Nov 3, 2019

View reviewed changes

Fix

410281e

ulysses-you closed this Nov 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-29720][CORE] Add linux condition to make ProcfsMetricsGetter more complete #26365

[SPARK-29720][CORE] Add linux condition to make ProcfsMetricsGetter more complete #26365

ulysses-you commented Nov 2, 2019 •

edited

ulysses-you commented Nov 2, 2019

AmplabJenkins commented Nov 2, 2019

ulysses-you commented Nov 2, 2019

maropu commented Nov 2, 2019

maropu commented Nov 2, 2019

ulysses-you commented Nov 3, 2019

maropu left a comment

maropu Nov 3, 2019

maropu Nov 3, 2019

ulysses-you Nov 3, 2019

viirya left a comment

viirya Nov 3, 2019

ulysses-you Nov 3, 2019

viirya Nov 3, 2019

ulysses-you Nov 4, 2019 •

edited

viirya Nov 4, 2019 •

edited

srowen commented Nov 4, 2019

rezasafi commented Nov 4, 2019 •

edited

rezasafi commented Nov 4, 2019

ulysses-you commented Nov 5, 2019

rezasafi commented Nov 5, 2019

ulysses-you commented Nov 5, 2019

srowen commented Nov 6, 2019

squito commented Nov 6, 2019

ulysses-you commented Nov 7, 2019

ghost commented Sep 25, 2020

[SPARK-29720][CORE] Add linux condition to make ProcfsMetricsGetter more complete #26365

[SPARK-29720][CORE] Add linux condition to make ProcfsMetricsGetter more complete #26365

Conversation

ulysses-you commented Nov 2, 2019 • edited

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?

ulysses-you commented Nov 2, 2019

AmplabJenkins commented Nov 2, 2019

ulysses-you commented Nov 2, 2019

maropu commented Nov 2, 2019

maropu commented Nov 2, 2019

ulysses-you commented Nov 3, 2019

maropu left a comment

Choose a reason for hiding this comment

maropu Nov 3, 2019

Choose a reason for hiding this comment

maropu Nov 3, 2019

Choose a reason for hiding this comment

ulysses-you Nov 3, 2019

Choose a reason for hiding this comment

viirya left a comment

Choose a reason for hiding this comment

viirya Nov 3, 2019

Choose a reason for hiding this comment

ulysses-you Nov 3, 2019

Choose a reason for hiding this comment

viirya Nov 3, 2019

Choose a reason for hiding this comment

ulysses-you Nov 4, 2019 • edited

Choose a reason for hiding this comment

viirya Nov 4, 2019 • edited

Choose a reason for hiding this comment

srowen commented Nov 4, 2019

rezasafi commented Nov 4, 2019 • edited

rezasafi commented Nov 4, 2019

ulysses-you commented Nov 5, 2019

rezasafi commented Nov 5, 2019

ulysses-you commented Nov 5, 2019

srowen commented Nov 6, 2019

squito commented Nov 6, 2019

ulysses-you commented Nov 7, 2019

ghost commented Sep 25, 2020

ulysses-you commented Nov 2, 2019 •

edited

ulysses-you Nov 4, 2019 •

edited

viirya Nov 4, 2019 •

edited

rezasafi commented Nov 4, 2019 •

edited