You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "manuzhang (via GitHub)" <gi...@apache.org> on 2023/05/15 16:07:25 UTC
[GitHub] [spark] manuzhang opened a new pull request, #41173: [SPARK-43510][YARN] Fix YarnAllocator internal state when adding running executor after processing completed containers
manuzhang opened a new pull request, #41173:
URL: https://github.com/apache/spark/pull/41173
### What changes were proposed in this pull request?
Keep track of completed container ids in YarnAllocator and don't update internal state of a container if it's already completed.
### Why are the changes needed?
YarnAllocator updates internal state adding running executors after executor launch in a separate thread. That can happen after the containers are already completed (e.g. preempted) and processed by YarnAllocator. Then YarnAllocator mistakenly thinks there are still running executors which are already lost. As a result, application hangs without any running executors.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Added UT.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] mridulm commented on a diff in pull request #41173: [SPARK-43510][YARN] Fix YarnAllocator internal state when adding running executor after processing completed containers
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #41173:
URL: https://github.com/apache/spark/pull/41173#discussion_r1205411956
##########
resource-managers/yarn/src/test/scala/org/apache/spark/deploy/yarn/YarnAllocatorSuite.scala:
##########
@@ -832,4 +834,28 @@ class YarnAllocatorSuite extends SparkFunSuite with Matchers {
verify(rpcEndPoint, times(1)).
send(DecommissionExecutorsOnHost(org.mockito.ArgumentMatchers.any()))
}
+
+ test("SPARK-43510: Running executors should be none when YarnAllocator adds running executors " +
+ "after processing completed containers") {
+ val (handler, _) = createAllocator(1)
+ handler.updateResourceRequests()
+ handler.getNumExecutorsRunning should be(0)
+ handler.getNumContainersPendingAllocate should be(1)
+
+ val container = createContainer("host1")
+ handler.handleAllocatedContainers(Array(container))
+ handler.getNumExecutorsRunning should be(1)
+ handler.getNumContainersPendingAllocate should be(0)
+
+ val status = ContainerStatus.newInstance(
+ container.getId, ContainerState.COMPLETE, "Finished", 0)
+ val getOrUpdateNumExecutorsStartingForRPId = PrivateMethod[AtomicInteger](
+ Symbol("getOrUpdateNumExecutorsStartingForRPId"))
+ handler.invokePrivate(getOrUpdateNumExecutorsStartingForRPId(0)).incrementAndGet()
+ handler.processCompletedContainers(Seq(status))
+ val updateInternalState = PrivateMethod[Unit](Symbol("updateInternalState"))
+ handler.invokePrivate(updateInternalState(0, "1", container))
+ handler.getNumExecutorsRunning should be(0)
+ handler.getNumExecutorsStarting should be(0)
Review Comment:
Scratch that - profile issue during tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] tgravescs commented on pull request #41173: [SPARK-43510][YARN] Fix YarnAllocator internal state when adding running executor after processing completed containers
Posted by "tgravescs (via GitHub)" <gi...@apache.org>.
tgravescs commented on PR #41173:
URL: https://github.com/apache/spark/pull/41173#issuecomment-1561159748
+1 looks good
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] manuzhang commented on pull request #41173: [SPARK-43510][YARN] Fix YarnAllocator internal state when adding running executor after processing completed containers
Posted by "manuzhang (via GitHub)" <gi...@apache.org>.
manuzhang commented on PR #41173:
URL: https://github.com/apache/spark/pull/41173#issuecomment-1548830799
cc @tgravescs @jerryshao Please help review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] manuzhang commented on pull request #41173: [SPARK-43510][YARN] Fix YarnAllocator internal state when adding running executor after processing completed containers
Posted by "manuzhang (via GitHub)" <gi...@apache.org>.
manuzhang commented on PR #41173:
URL: https://github.com/apache/spark/pull/41173#issuecomment-1560490935
@tgravescs could you take another look?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] tgravescs commented on pull request #41173: [SPARK-43510][YARN] Fix YarnAllocator internal state when adding running executor after processing completed containers
Posted by "tgravescs (via GitHub)" <gi...@apache.org>.
tgravescs commented on PR #41173:
URL: https://github.com/apache/spark/pull/41173#issuecomment-1578774958
merged to master and branch-3.4
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on a diff in pull request #41173: [SPARK-43510][YARN] Fix YarnAllocator internal state when adding running executor after processing completed containers
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #41173:
URL: https://github.com/apache/spark/pull/41173#discussion_r1197416999
##########
resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/YarnAllocator.scala:
##########
@@ -806,11 +796,31 @@ private[yarn] class YarnAllocator(
}
}
+ // visible for testing
Review Comment:
nit. Could you try to use `PrivateMethodTester`? Here is an example, cfd42355ccbdf34ca9bd5a125c4310b9da14100c
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] manuzhang commented on pull request #41173: [SPARK-43510][YARN] Fix YarnAllocator internal state when adding running executor after processing completed containers
Posted by "manuzhang (via GitHub)" <gi...@apache.org>.
manuzhang commented on PR #41173:
URL: https://github.com/apache/spark/pull/41173#issuecomment-1548929007
retest please
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] mridulm commented on a diff in pull request #41173: [SPARK-43510][YARN] Fix YarnAllocator internal state when adding running executor after processing completed containers
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #41173:
URL: https://github.com/apache/spark/pull/41173#discussion_r1205402312
##########
resource-managers/yarn/src/test/scala/org/apache/spark/deploy/yarn/YarnAllocatorSuite.scala:
##########
@@ -832,4 +834,28 @@ class YarnAllocatorSuite extends SparkFunSuite with Matchers {
verify(rpcEndPoint, times(1)).
send(DecommissionExecutorsOnHost(org.mockito.ArgumentMatchers.any()))
}
+
+ test("SPARK-43510: Running executors should be none when YarnAllocator adds running executors " +
+ "after processing completed containers") {
+ val (handler, _) = createAllocator(1)
+ handler.updateResourceRequests()
+ handler.getNumExecutorsRunning should be(0)
+ handler.getNumContainersPendingAllocate should be(1)
+
+ val container = createContainer("host1")
+ handler.handleAllocatedContainers(Array(container))
+ handler.getNumExecutorsRunning should be(1)
+ handler.getNumContainersPendingAllocate should be(0)
+
+ val status = ContainerStatus.newInstance(
+ container.getId, ContainerState.COMPLETE, "Finished", 0)
+ val getOrUpdateNumExecutorsStartingForRPId = PrivateMethod[AtomicInteger](
+ Symbol("getOrUpdateNumExecutorsStartingForRPId"))
+ handler.invokePrivate(getOrUpdateNumExecutorsStartingForRPId(0)).incrementAndGet()
+ handler.processCompletedContainers(Seq(status))
+ val updateInternalState = PrivateMethod[Unit](Symbol("updateInternalState"))
+ handler.invokePrivate(updateInternalState(0, "1", container))
+ handler.getNumExecutorsRunning should be(0)
+ handler.getNumExecutorsStarting should be(0)
Review Comment:
This test passes even without any of the changes proposed in this PR - is it actually testing the changed behavior ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] tgravescs commented on a diff in pull request #41173: [SPARK-43510][YARN] Fix YarnAllocator internal state when adding running executor after processing completed containers
Posted by "tgravescs (via GitHub)" <gi...@apache.org>.
tgravescs commented on code in PR #41173:
URL: https://github.com/apache/spark/pull/41173#discussion_r1198121817
##########
resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/YarnAllocator.scala:
##########
@@ -91,6 +91,9 @@ private[yarn] class YarnAllocator(
@GuardedBy("this")
private val releasedContainers = collection.mutable.HashSet[ContainerId]()
+ @GuardedBy("this")
+ private val completedContainerIds = collection.mutable.HashSet[ContainerId]()
Review Comment:
you never remove anything from this? Seems like we are adding a bunch of extra memory pressure for very long running applications using dynamic allocation that might have a lot of containers.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] tgravescs commented on a diff in pull request #41173: [SPARK-43510][YARN] Fix YarnAllocator internal state when adding running executor after processing completed containers
Posted by "tgravescs (via GitHub)" <gi...@apache.org>.
tgravescs commented on code in PR #41173:
URL: https://github.com/apache/spark/pull/41173#discussion_r1200912108
##########
resource-managers/yarn/src/test/scala/org/apache/spark/deploy/yarn/YarnAllocatorSuite.scala:
##########
@@ -19,10 +19,8 @@ package org.apache.spark.deploy.yarn
import java.util
import java.util.Collections
-
Review Comment:
put these new lines back
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] manuzhang commented on pull request #41173: [SPARK-43510][YARN] Fix YarnAllocator internal state when adding running executor after processing completed containers
Posted by "manuzhang (via GitHub)" <gi...@apache.org>.
manuzhang commented on PR #41173:
URL: https://github.com/apache/spark/pull/41173#issuecomment-1553978308
@tgravescs we have seen it quite often when YARN queues were full and the containers were **immediately preempted after launch**. I've updated the PR keeping track of containers launching executors which can be cleaned up.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] tgravescs commented on a diff in pull request #41173: [SPARK-43510][YARN] Fix YarnAllocator internal state when adding running executor after processing completed containers
Posted by "tgravescs (via GitHub)" <gi...@apache.org>.
tgravescs commented on code in PR #41173:
URL: https://github.com/apache/spark/pull/41173#discussion_r1199162022
##########
resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/YarnAllocator.scala:
##########
@@ -780,7 +771,7 @@ private[yarn] class YarnAllocator(
localResources,
rp.id
).run()
- updateInternalState()
+ updateInternalState(rpId, executorId, container)
} catch {
case e: Throwable =>
getOrUpdateNumExecutorsStartingForRPId(rpId).decrementAndGet()
Review Comment:
you need to handle if launching throws and remove it from launchingExecutorContainerIds
##########
resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/YarnAllocator.scala:
##########
@@ -806,11 +797,31 @@ private[yarn] class YarnAllocator(
}
}
+ private def updateInternalState(rpId: Int, executorId: String,
+ container: Container): Unit = synchronized {
Review Comment:
nit indentation should be 4 spaces here.
##########
resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/YarnAllocator.scala:
##########
@@ -806,11 +797,31 @@ private[yarn] class YarnAllocator(
}
}
+ private def updateInternalState(rpId: Int, executorId: String,
+ container: Container): Unit = synchronized {
+ val containerId = container.getId
+ if (launchingExecutorContainerIds.contains(containerId)) {
+ getOrUpdateRunningExecutorForRPId(rpId).add(executorId)
+ getOrUpdateNumExecutorsStartingForRPId(rpId).decrementAndGet()
Review Comment:
getOrUpdateNumExecutorsStartingForRPId still needs to be decremented somewhere, otherwise it leaks that container and thinks its still starting
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] manuzhang commented on pull request #41173: [SPARK-43510][YARN] Fix YarnAllocator internal state when adding running executor after processing completed containers
Posted by "manuzhang (via GitHub)" <gi...@apache.org>.
manuzhang commented on PR #41173:
URL: https://github.com/apache/spark/pull/41173#issuecomment-1567611058
@mridulm @dongjoon-hyun any more comments?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] tgravescs commented on pull request #41173: [SPARK-43510][YARN] Fix YarnAllocator internal state when adding running executor after processing completed containers
Posted by "tgravescs (via GitHub)" <gi...@apache.org>.
tgravescs commented on PR #41173:
URL: https://github.com/apache/spark/pull/41173#issuecomment-1553426997
Have you only seen this once? Do you have any repro case or tried to make a test to simulate?
I'm a bit unclear exactly how this happens, from taking a quick look, both those functions that add and remove are synchronized. Am I missing how these can be called and actually run at the same time?
You are essentially implying that the completed container is called after we launched the executor but before we could add it containerIdToExecutorIdAndResourceProfileId. I do see from your logs, all that happened with the same second. Maybe not crucial but do you know why that container was completed so quickly?
As from my comment, I don't see you cleaning up this new datastructure, I would much rather look at either preventing this from running or perhaps having another launching state that you would check before removing. But would like to understand above first.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] mridulm commented on a diff in pull request #41173: [SPARK-43510][YARN] Fix YarnAllocator internal state when adding running executor after processing completed containers
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #41173:
URL: https://github.com/apache/spark/pull/41173#discussion_r1205402312
##########
resource-managers/yarn/src/test/scala/org/apache/spark/deploy/yarn/YarnAllocatorSuite.scala:
##########
@@ -832,4 +834,28 @@ class YarnAllocatorSuite extends SparkFunSuite with Matchers {
verify(rpcEndPoint, times(1)).
send(DecommissionExecutorsOnHost(org.mockito.ArgumentMatchers.any()))
}
+
+ test("SPARK-43510: Running executors should be none when YarnAllocator adds running executors " +
+ "after processing completed containers") {
+ val (handler, _) = createAllocator(1)
+ handler.updateResourceRequests()
+ handler.getNumExecutorsRunning should be(0)
+ handler.getNumContainersPendingAllocate should be(1)
+
+ val container = createContainer("host1")
+ handler.handleAllocatedContainers(Array(container))
+ handler.getNumExecutorsRunning should be(1)
+ handler.getNumContainersPendingAllocate should be(0)
+
+ val status = ContainerStatus.newInstance(
+ container.getId, ContainerState.COMPLETE, "Finished", 0)
+ val getOrUpdateNumExecutorsStartingForRPId = PrivateMethod[AtomicInteger](
+ Symbol("getOrUpdateNumExecutorsStartingForRPId"))
+ handler.invokePrivate(getOrUpdateNumExecutorsStartingForRPId(0)).incrementAndGet()
+ handler.processCompletedContainers(Seq(status))
+ val updateInternalState = PrivateMethod[Unit](Symbol("updateInternalState"))
+ handler.invokePrivate(updateInternalState(0, "1", container))
+ handler.getNumExecutorsRunning should be(0)
+ handler.getNumExecutorsStarting should be(0)
Review Comment:
This test passes even without any of the (other) changes proposed in this PR - is it actually testing the changed behavior ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] manuzhang commented on pull request #41173: [SPARK-43510][YARN] Fix YarnAllocator internal state when adding running executor after processing completed containers
Posted by "manuzhang (via GitHub)" <gi...@apache.org>.
manuzhang commented on PR #41173:
URL: https://github.com/apache/spark/pull/41173#issuecomment-1577878358
@tgravescs any more comments? We are waiting on this patch to fix our production issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] asfgit closed pull request #41173: [SPARK-43510][YARN] Fix YarnAllocator internal state when adding running executor after processing completed containers
Posted by "asfgit (via GitHub)" <gi...@apache.org>.
asfgit closed pull request #41173: [SPARK-43510][YARN] Fix YarnAllocator internal state when adding running executor after processing completed containers
URL: https://github.com/apache/spark/pull/41173
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] mridulm commented on pull request #41173: [SPARK-43510][YARN] Fix YarnAllocator internal state when adding running executor after processing completed containers
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on PR #41173:
URL: https://github.com/apache/spark/pull/41173#issuecomment-1550575495
+CC @zhouyejoe
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org