Skip to content
Closed
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
3f8321a
Integration of ProcessTreeMetrics with PR 21221
Jul 26, 2018
cd16a75
Changing the position of ptree and also make the computation configur…
Aug 7, 2018
94c2b04
Seperate metrics for jvm, python and others and update the tests
Aug 8, 2018
062f5d7
Update JsonProtocolSuite
Sep 25, 2018
245221d
[SPARK-24958] Add executors' process tree total memory information to…
Oct 2, 2018
c72be03
Adressing most of Imran's comments
Oct 3, 2018
8f3c938
Fixing the scala style and some minor comments
Oct 3, 2018
f2dca27
Removing types from the definitions where ever possible
Oct 4, 2018
a9f924c
Using Utils methods when possible or use ProcessBuilder
Oct 5, 2018
a11e3a2
make use of Utils.trywithresources
Oct 5, 2018
34ad625
Changing ExecutorMericType and ExecutorMetrics to use a map instead o…
Oct 9, 2018
415f976
Changing ExecutorMetric to use array instead of a map
Oct 10, 2018
067b81d
A small cosmetic change
Oct 10, 2018
18ee4ad
Merge branch 'master' of https://github.com/apache/spark into ptreeme…
Oct 17, 2018
7f7ed2b
Applying latest review commments. Using Arrays instead of Map for ret…
Oct 23, 2018
f3867ff
Merge branch 'master' of https://github.com/apache/spark into ptreeme…
Nov 5, 2018
0f8f3e2
Fix an issue with jsonProtoclSuite
Nov 5, 2018
ea08c61
Fix scalastyle issue
Nov 5, 2018
8f20857
Applying latest review comments
Nov 14, 2018
6e65360
Using the companion object and other stuff
Nov 27, 2018
4659f4a
Update the use of process builder and applying other review comments
Nov 28, 2018
ef4be38
Small style fixes based on reviews
Nov 30, 2018
805741c
Applying review comments, mostly style related
Nov 30, 2018
4c1f073
emove the unnecessary trywithresources
Nov 30, 2018
0a7402e
Applying the comment about error handling and some more style fixes
Dec 4, 2018
3d65b35
Removing a return
Dec 6, 2018
6eab315
Reordering of info in a test resource file to avoid confusion
Dec 6, 2018
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: 8 additions & 5 deletions core/src/main/scala/org/apache/spark/Heartbeater.scala
Original file line number Diff line number Diff line change
Expand Up @@ -59,12 +59,15 @@ private[spark] class Heartbeater(
heartbeater.awaitTermination(10, TimeUnit.SECONDS)
}

/**
* Get the current executor level metrics. These are returned as an array, with the index
* determined by MetricGetter.values
*/
/** Get the current executor level metrics. These are returned as an Array */
def getCurrentMetrics(): ExecutorMetrics = {
val metrics = ExecutorMetricType.values.map(_.getMetricValue(memoryManager)).toArray
val metrics = new Array[Long](ExecutorMetricType.numberOfMetrics)
var offset = 0
ExecutorMetricType.metricGetters.foreach { metric =>
val newSetOfMetrics = metric.getMetricSet(memoryManager)
Array.copy(newSetOfMetrics, 0, metrics, offset, newSetOfMetrics.size)
offset += newSetOfMetrics.length
}
new ExecutorMetrics(metrics)
}
}
Expand Down
25 changes: 12 additions & 13 deletions core/src/main/scala/org/apache/spark/executor/ExecutorMetrics.scala
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
*/
package org.apache.spark.executor

import scala.collection.mutable

import org.apache.spark.annotation.DeveloperApi
import org.apache.spark.metrics.ExecutorMetricType

Expand All @@ -28,16 +30,14 @@ import org.apache.spark.metrics.ExecutorMetricType
@DeveloperApi
class ExecutorMetrics private[spark] extends Serializable {

// Metrics are indexed by MetricGetter.values
private val metrics = new Array[Long](ExecutorMetricType.values.length)

private val metrics = new Array[Long](ExecutorMetricType.numberOfMetrics)
// the first element is initialized to -1, indicating that the values for the array
// haven't been set yet.
metrics(0) = -1

/** Returns the value for the specified metricType. */
def getMetricValue(metricType: ExecutorMetricType): Long = {
metrics(ExecutorMetricType.metricIdxMap(metricType))
/** Returns the value for the specified metric. */
def getMetricValue(metricName: String): Long = {
metrics(ExecutorMetricType.definedMetricsAndOffset.get(metricName).get)
}

/** Returns true if the values for the metrics have been set, false otherwise. */
Expand All @@ -49,14 +49,14 @@ class ExecutorMetrics private[spark] extends Serializable {
}

/**
* Constructor: create the ExecutorMetrics with the values specified.
* Constructor: create the ExecutorMetrics with using a given map.
*
* @param executorMetrics map of executor metric name to value
*/
private[spark] def this(executorMetrics: Map[String, Long]) {
this()
(0 until ExecutorMetricType.values.length).foreach { idx =>
metrics(idx) = executorMetrics.getOrElse(ExecutorMetricType.values(idx).name, 0L)
ExecutorMetricType.definedMetricsAndOffset.map { m =>
metrics(m._2) = executorMetrics.getOrElse(m._1, 0L)
}
}

Expand All @@ -69,11 +69,10 @@ class ExecutorMetrics private[spark] extends Serializable {
*/
private[spark] def compareAndUpdatePeakValues(executorMetrics: ExecutorMetrics): Boolean = {
var updated = false

(0 until ExecutorMetricType.values.length).foreach { idx =>
if (executorMetrics.metrics(idx) > metrics(idx)) {
ExecutorMetricType.definedMetricsAndOffset.map {m =>
if (executorMetrics.metrics(m._2) > metrics(m._2)) {
updated = true
metrics(idx) = executorMetrics.metrics(idx)
metrics(m._2) = executorMetrics.metrics(m._2)
}
}
updated
Expand Down
222 changes: 222 additions & 0 deletions core/src/main/scala/org/apache/spark/executor/ProcfsBasedSystems.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,222 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright ownership.
* The ASF licenses this file to You under the Apache License, Version 2.0
* (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package org.apache.spark.executor

import java.io._
import java.nio.charset.Charset
import java.nio.file.{Files, Paths}
import java.util.Locale

import scala.collection.mutable
import scala.collection.mutable.ArrayBuffer

import org.apache.spark.{SparkEnv, SparkException}
import org.apache.spark.internal.{config, Logging}
import org.apache.spark.util.Utils

private[spark] case class ProcfsBasedSystemsMetrics(
jvmVmemTotal: Long,
jvmRSSTotal: Long,
pythonVmemTotal: Long,
pythonRSSTotal: Long,
otherVmemTotal: Long,
otherRSSTotal: Long)

// Some of the ideas here are taken from the ProcfsBasedProcessTree class in hadoop
// project.
private[spark] class ProcfsBasedSystems(val procfsDir: String = "/proc/") extends Logging {
val procfsStatFile = "stat"
val testing = sys.env.contains("SPARK_TESTING") || sys.props.contains("spark.testing")
var pageSize = computePageSize()
var isAvailable: Boolean = isProcfsAvailable
private val pid = computePid()
Copy link
Contributor

Choose a reason for hiding this comment

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

pageSize is only a var for testing -- instead just optionally pass it in to the constructor

also I think all of these can be private.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I can't call computePageSize() in the constructor signature to compute the default value. Another solution is to check for testing inside computePageSize and if we are testing assign a value to it that is provided in the constructor (default to 4096).

Copy link
Contributor

Choose a reason for hiding this comment

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

You can't put it as a default value, but if you make it a static method, then you can provide an overloaded method which uses it, see squito@cf00835

But, I think your other proposal is even better, if its testing just give it a fixed value (no need to even make it an argument to the constructor at all).

private var ptree = mutable.Map[ Int, Set[Int]]()

var allMetrics: ProcfsBasedSystemsMetrics = ProcfsBasedSystemsMetrics(0, 0, 0, 0, 0, 0)

computeProcessTree()

private def isProcfsAvailable: Boolean = {
if (testing) {
return true
}
try {
if (!Files.exists(Paths.get(procfsDir))) {
return false
}
}
catch {
case f: FileNotFoundException => return false
}
val shouldLogStageExecutorMetrics =
SparkEnv.get.conf.get(config.EVENT_LOG_STAGE_EXECUTOR_METRICS)
val shouldLogStageExecutorProcessTreeMetrics =
SparkEnv.get.conf.get(config.EVENT_LOG_PROCESS_TREE_METRICS)
shouldLogStageExecutorProcessTreeMetrics && shouldLogStageExecutorMetrics
}

private def computePid(): Int = {
if (!isAvailable || testing) {
return -1;
}
try {
// This can be simplified in java9:
// https://docs.oracle.com/javase/9/docs/api/java/lang/ProcessHandle.html
val cmd = Array("bash", "-c", "echo $PPID")
val length = 10
val out2 = Utils.executeAndGetOutput(cmd)
Copy link
Contributor

Choose a reason for hiding this comment

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

can be out instead of out2

val pid = Integer.parseInt(out2.split("\n")(0))
return pid;
}
catch {
case e: SparkException => logWarning("Exception when trying to compute process tree." +
" As a result reporting of ProcessTree metrics is stopped", e)
isAvailable = false
return -1
}
}

private def computePageSize(): Long = {
if (testing) {
return 0;
}
try {
val cmd = Array("getconf", "PAGESIZE")
val out2 = Utils.executeAndGetOutput(cmd)
return Integer.parseInt(out2.split("\n")(0))
} catch {
case e: Exception => logWarning("Exception when trying to compute pagesize, as a" +
" result reporting of ProcessTree metrics is stopped")
isAvailable = false
return 0
}
}

private def computeProcessTree(): Unit = {
if (!isAvailable || testing) {
return
}
ptree = mutable.Map[ Int, Set[Int]]()
val queue = mutable.Queue.empty[Int]
queue += pid
while( !queue.isEmpty ) {
val p = queue.dequeue()
val c = getChildPids(p)
if(!c.isEmpty) {
queue ++= c
ptree += (p -> c.toSet)
}
else {
ptree += (p -> Set[Int]())
}
}
}

private def getChildPids(pid: Int): ArrayBuffer[Int] = {
try {
// val cmd = Array("pgrep", "-P", pid.toString)
val builder = new ProcessBuilder("pgrep", "-P", pid.toString)
val process = builder.start()
// val output = new StringBuilder()
val threadName = "read stdout for " + "pgrep"
val childPidsInInt = mutable.ArrayBuffer.empty[Int]
def appendChildPid(s: String): Unit = {
if (s != "") {
logDebug("Found a child pid:" + s)
childPidsInInt += Integer.parseInt(s)
}
}
val stdoutThread = Utils.processStreamByLine(threadName,
process.getInputStream, appendChildPid)
val exitCode = process.waitFor()
stdoutThread.join()
// pgrep will have exit code of 1 if there are more than one child process
// and it will have a exit code of 2 if there is no child process
if (exitCode != 0 && exitCode > 2) {
val cmd = builder.command().toArray.mkString(" ")
logWarning(s"Process $cmd" +
s" exited with code $exitCode, with stderr:" + s"${process.getErrorStream} ")
Copy link
Contributor

Choose a reason for hiding this comment

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

I dont' think process.getErrorStream will have a useful toString. I think you need to read all the data. You probably also have to do that before process.waitFor(), otherwise I think its possible that the process blocks forever waiting for something to read stderr.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed this in the new patch, but not sure if I addressed your concern. Please let me know

throw new SparkException(s"Process $cmd exited with code $exitCode")
}
childPidsInInt
} catch {
case e: Exception => logWarning("Exception when trying to compute process tree." +
" As a result reporting of ProcessTree metrics is stopped.", e)
isAvailable = false
return mutable.ArrayBuffer.empty[Int]
}
}

def computeProcessInfo(pid: Int): Unit = {
/*
* Hadoop ProcfsBasedProcessTree class used regex and pattern matching to retrive the memory
* info. I tried that but found it not correct during tests, so I used normal string analysis
* instead. The computation of RSS and Vmem are based on proc(5):
* http://man7.org/linux/man-pages/man5/proc.5.html
*/
try {
val pidDir = new File(procfsDir, pid.toString)
Utils.tryWithResource( new InputStreamReader(
new FileInputStream(
new File(pidDir, procfsStatFile)), Charset.forName("UTF-8"))) { fReader =>
Utils.tryWithResource( new BufferedReader(fReader)) { in =>
val procInfo = in.readLine
val procInfoSplit = procInfo.split(" ")
if (procInfoSplit != null) {
val vmem = procInfoSplit(22).toLong
val rssPages = procInfoSplit(23).toLong
if (procInfoSplit(1).toLowerCase(Locale.US).contains("java")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this just be vmem and rssPages, rather than splitting into JVM, Python, and other? Can you explain more about how the separate values would be used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is separated since it turns out knowing main actors like jvm in seperation can have some value for the user. We just consider jvm (case of pur scala) and python (case of using pyspark). Other stuff can be added per interest in future, but for now we consider everything else under "Other" category

Copy link
Contributor

Choose a reason for hiding this comment

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

@edwinalu It would be nice to have a break up of the total memory being consumed. Its easier to tune the parameters knowing what is consuming all the memory. For example if your container died OOMing - it helps to know if it was because of python or JVM. Also R fits in the other category so it makes sense to have all 3 of them as of now.

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't have much pyspark ourselves, but yes, it seems useful to have the breakdown, and it's easy to sum the values for the total.

allMetrics = allMetrics.copy(
jvmVmemTotal = allMetrics.jvmVmemTotal + vmem,
jvmRSSTotal = allMetrics.jvmRSSTotal + (rssPages*pageSize)
)
}
else if (procInfoSplit(1).toLowerCase(Locale.US).contains("python")) {
allMetrics = allMetrics.copy(
pythonVmemTotal = allMetrics.pythonVmemTotal + vmem,
pythonRSSTotal = allMetrics.pythonRSSTotal + (rssPages*pageSize)
)
}
else {
allMetrics = allMetrics.copy(
otherVmemTotal = allMetrics.otherVmemTotal + vmem,
otherRSSTotal = allMetrics.otherRSSTotal + (rssPages*pageSize)
)
}
}
}
}
} catch {
case f: FileNotFoundException => logWarning("There was a problem with reading" +
" the stat file of the process. ", f)
}
}

private[spark] def computeAllMetrics(): ProcfsBasedSystemsMetrics = {
if (!isAvailable) {
return ProcfsBasedSystemsMetrics(0, 0, 0, 0, 0, 0)
}
computeProcessTree
val pids = ptree.keySet
allMetrics = ProcfsBasedSystemsMetrics(0, 0, 0, 0, 0, 0)
for (p <- pids) {
computeProcessInfo(p)
Copy link
Contributor

Choose a reason for hiding this comment

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

the state used here is a little trickier than it needs to be.

computeProcessTree is updating a member variable, even though its only used locally -- it would be easier to follow if instead it just returned the process tree, and then you passed it around. Also I dont' think you actually care about the tree, just the set of pids?

similarly for allMetrics. it doesn't really need to be a member variable, since its use is entirely contained within this function, you could just pass it around.

val pids = discoverPids()
val allMetrics = ...
for (p <- pids) {
  allMetrics = updateMetricsForProcess(allMetrics, p)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The tree was there in case we want to do some other stuff with it, but I guess we can have a tree structure when we actually need it. Right now as you mentioned we don't need it. So I will change it.
the allMetrics was there for testing, but I can change the test anyway.

}
return allMetrics
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,11 @@ package object config {
.booleanConf
.createWithDefault(false)

private[spark] val EVENT_LOG_PROCESS_TREE_METRICS =
ConfigBuilder("spark.eventLog.logStageExecutorProcessTreeMetrics.enabled")
.booleanConf
.createWithDefault(false)

private[spark] val EVENT_LOG_OVERWRITE =
ConfigBuilder("spark.eventLog.overwrite").booleanConf.createWithDefault(false)

Expand Down
Loading