-
Notifications
You must be signed in to change notification settings - Fork 29.3k
[SPARK-24248][K8S] Use level triggering and state reconciliation in scheduling and lifecycle #21366
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 21 commits
310263c
60990f1
f3bb80a
3343ba6
30b7f17
522b079
600e25f
931529a
9e5abfb
2156a20
aabc187
ee0d196
c2b9733
caffe23
ca3fdb3
79ebaf3
fadbe9f
4f58393
2a2374c
5850439
d4cf40f
c398ebb
45a02de
a8a3539
4a49677
57ea5dd
b30ed39
5b9c00f
260d82c
bd03451
f294dca
7bf49ba
b5c0fbf
c4b87d8
8615c06
0a205f6
3b85ab5
edc982b
e077c7e
a97fc5d
bd7b0d3
e9d7c8f
0fac4d5
e42dd4f
03b1064
c1b8431
108181d
9e0b758
8b0a211
1a99dce
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,120 @@ | ||
| /* | ||
| * 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.scheduler.cluster.k8s | ||
|
|
||
| import java.util.concurrent.atomic.{AtomicInteger, AtomicLong} | ||
|
|
||
| import io.fabric8.kubernetes.api.model.{Pod, PodBuilder} | ||
| import io.fabric8.kubernetes.client.KubernetesClient | ||
| import scala.collection.mutable | ||
|
|
||
| import org.apache.spark.{SparkConf, SparkException} | ||
| import org.apache.spark.deploy.k8s.Config._ | ||
| import org.apache.spark.deploy.k8s.Constants._ | ||
| import org.apache.spark.deploy.k8s.KubernetesConf | ||
| import org.apache.spark.internal.Logging | ||
|
|
||
| private[spark] class ExecutorPodsAllocator( | ||
| conf: SparkConf, | ||
| executorBuilder: KubernetesExecutorBuilder, | ||
| kubernetesClient: KubernetesClient, | ||
| eventQueue: ExecutorPodsEventQueue) extends Logging { | ||
|
|
||
| private val EXECUTOR_ID_COUNTER = new AtomicLong(0L) | ||
|
|
||
| private val totalExpectedExecutors = new AtomicInteger(0) | ||
|
|
||
| private val podAllocationSize = conf.get(KUBERNETES_ALLOCATION_BATCH_SIZE) | ||
|
|
||
| private val podAllocationDelay = conf.get(KUBERNETES_ALLOCATION_BATCH_DELAY) | ||
|
|
||
| private val kubernetesDriverPodName = conf | ||
| .get(KUBERNETES_DRIVER_POD_NAME) | ||
| .getOrElse(throw new SparkException("Must specify the driver pod name")) | ||
|
|
||
| private val driverPod = kubernetesClient.pods() | ||
| .withName(kubernetesDriverPodName) | ||
| .get() | ||
|
|
||
| // Use sets of ids instead of counters to be able to handle duplicate events. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: hanging comment?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's in reference to both mutable hash sets below. A suggestion on how to better format this? |
||
|
|
||
| // Executor IDs that have been requested from Kubernetes but are not running yet. | ||
| private val pendingExecutors = mutable.Set.empty[Long] | ||
|
|
||
| // We could use CoarseGrainedSchedulerBackend#totalRegisteredExecutors here for tallying the | ||
| // executors that are running. But, here we choose instead to maintain all state within this | ||
| // class from the persecptive of the k8s API. Therefore whether or not this scheduler loop | ||
| // believes an executor is running is dictated by the K8s API rather than Spark's RPC events. | ||
| // We may need to consider where these perspectives may differ and which perspective should | ||
| // take precedence. | ||
| private val runningExecutors = mutable.Set.empty[Long] | ||
|
|
||
| def start(applicationId: String): Unit = { | ||
| eventQueue.addSubscriber(podAllocationDelay) { updatedPods => | ||
| processUpdatedPodEvents(applicationId, updatedPods) | ||
| } | ||
| } | ||
|
|
||
| def setTotalExpectedExecutors(total: Int): Unit = totalExpectedExecutors.set(total) | ||
|
|
||
| private def processUpdatedPodEvents(applicationId: String, updatedPods: Seq[Pod]): Unit = { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We're actually processing pods here right? Not the events themselves from the watch. |
||
| updatedPods.foreach { updatedPod => | ||
| val execId = updatedPod.getMetadata.getLabels.get(SPARK_EXECUTOR_ID_LABEL).toLong | ||
| val phase = updatedPod.getStatus.getPhase.toLowerCase | ||
| phase match { | ||
| case "running" => | ||
| pendingExecutors -= execId | ||
| runningExecutors += execId | ||
| case "failed" | "succeeded" | "error" => | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see "error" listed as one of the pod phases: https://kubernetes.io/docs/concepts/workloads/pods/pod-lifecycle/#pod-phase |
||
| pendingExecutors -= execId | ||
| runningExecutors -= execId | ||
| } | ||
| } | ||
|
|
||
| val currentRunningExecutors = runningExecutors.size | ||
| val currentTotalExpectedExecutors = totalExpectedExecutors.get | ||
| if (pendingExecutors.isEmpty && currentRunningExecutors < currentTotalExpectedExecutors) { | ||
| val numExecutorsToAllocate = math.min( | ||
| currentTotalExpectedExecutors - currentRunningExecutors, podAllocationSize) | ||
| logInfo(s"Going to request $numExecutorsToAllocate executors from Kubernetes.") | ||
| val newExecutorIds = mutable.Buffer.empty[Long] | ||
| val podsToAllocate = mutable.Buffer.empty[Pod] | ||
| for ( _ <- 0 until numExecutorsToAllocate) { | ||
| val newExecutorId = EXECUTOR_ID_COUNTER.incrementAndGet() | ||
| val executorConf = KubernetesConf.createExecutorConf( | ||
| conf, | ||
| newExecutorId.toString, | ||
| applicationId, | ||
| driverPod) | ||
| val executorPod = executorBuilder.buildFromFeatures(executorConf) | ||
| val podWithAttachedContainer = new PodBuilder(executorPod.pod) | ||
| .editOrNewSpec() | ||
| .addToContainers(executorPod.container) | ||
| .endSpec() | ||
| .build() | ||
| kubernetesClient.pods().create(podWithAttachedContainer) | ||
| pendingExecutors += newExecutorId | ||
| } | ||
| } else if (currentRunningExecutors == currentTotalExpectedExecutors) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| logDebug("Current number of running executors is equal to the number of requested" + | ||
| " executors. Not scaling up further.") | ||
| } else if (pendingExecutors.nonEmpty) { | ||
| logInfo(s"Still waiting for ${pendingExecutors.size} executors to begin running before" + | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably should |
||
| s" requesting for more executors.") | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no |
||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| /* | ||
| * 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.scheduler.cluster.k8s | ||
|
|
||
| import io.fabric8.kubernetes.api.model.Pod | ||
|
|
||
| private[spark] trait ExecutorPodsEventQueue { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just curious - is there no event queue mechanism within Spark itself for reuse here? Somewhat tangentially, there looks to be EventLoop in
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is but they don't have the concept of processing the same events at different time intervals in different components. This is what RxJava buys us that the existing event systems in the codebase do not. |
||
|
|
||
| def addSubscriber(processBatchIntervalMillis: Long)(onNextBatch: Seq[Pod] => Unit): Unit | ||
|
|
||
| def stopProcessingEvents(): Unit | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think |
||
|
|
||
| def pushPodUpdate(updatedPod: Pod): Unit | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we simply name it |
||
|
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,77 @@ | ||
| /* | ||
| * 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.scheduler.cluster.k8s | ||
|
|
||
| import java.util.concurrent.{Executor, ScheduledExecutorService, TimeUnit} | ||
|
|
||
| import com.google.common.collect.Lists | ||
| import io.fabric8.kubernetes.api.model.Pod | ||
| import io.reactivex.disposables.Disposable | ||
| import io.reactivex.functions.Consumer | ||
| import io.reactivex.schedulers.Schedulers | ||
| import io.reactivex.subjects.PublishSubject | ||
| import scala.collection.JavaConverters._ | ||
| import scala.collection.mutable | ||
|
|
||
| import org.apache.spark.util.Utils | ||
|
|
||
| private[spark] class ExecutorPodsEventQueueImpl( | ||
| bufferEventsExecutor: ScheduledExecutorService, | ||
| executeSubscriptionsExecutor: Executor) | ||
| extends ExecutorPodsEventQueue { | ||
|
|
||
| private val eventsObservable = PublishSubject.create[Pod]() | ||
| private val observedDisposables = mutable.Buffer.empty[Disposable] | ||
|
|
||
| def addSubscriber(processBatchIntervalMillis: Long)(onNextBatch: Seq[Pod] => Unit): Unit = { | ||
| observedDisposables += eventsObservable | ||
| // Group events in the time window given by the caller. These buffers are then sent | ||
| // to the caller's lambda at the given interval, with the pod updates that occurred | ||
| // in that given interval. | ||
| .buffer( | ||
| processBatchIntervalMillis, | ||
| TimeUnit.MILLISECONDS, | ||
| // For testing - specifically use the given scheduled executor service to trigger | ||
| // buffer boundaries. Allows us to inject a deterministic scheduler here. | ||
| Schedulers.from(bufferEventsExecutor)) | ||
| // Trigger an event cycle immediately. Not strictly required to be fully correct, but | ||
| // in particular the pod allocator should try to request executors immediately instead | ||
| // of waiting for one pod allocation delay. | ||
| .startWith(Lists.newArrayList[Pod]()) | ||
| // Force all triggered events - both the initial event above and the buffered ones in | ||
| // the following time windows - to execute asynchronously to this call's thread. | ||
| .observeOn(Schedulers.from(executeSubscriptionsExecutor)) | ||
| .subscribe(toReactivexConsumer { (pods: java.util.List[Pod]) => | ||
| Utils.tryLogNonFatalError { | ||
| onNextBatch(pods.asScala) | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| def stopProcessingEvents(): Unit = { | ||
| observedDisposables.foreach(_.dispose()) | ||
| eventsObservable.onComplete() | ||
| } | ||
|
|
||
| def pushPodUpdate(updatedPod: Pod): Unit = eventsObservable.onNext(updatedPod) | ||
|
|
||
| private def toReactivexConsumer[T](consumer: T => Unit): Consumer[T] = { | ||
| new Consumer[T] { | ||
| override def accept(item: T): Unit = consumer(item) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why adding this to the top level pom?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We always add to the top level and then in the lower level poms, we reference the dependent modules without listing their versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'm a bit concerned adding rxjava to the top level pom and to dev/deps/spark-deps-hadoop-*
can it be just a
<arrow.version>0.8.0</arrow.version>thing and not a dependency?it might possibly conflict with calling Spark from the Reactive Stream stack? @skonto what do you think?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also added dependency should have its LICENSE added under /license
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unsure what you mean here - we're using rxjava itself specifically to do the event handling in this PR. See https://github.com/apache/spark/pull/21366/files#diff-ae4cd884779fb4c3db58958ab984db59R40. If we wanted an alternative approach we can build something from first principles (executor service / manual linked blocking queues) but I like the elegance that rx-java buys us here. The code we'd save building ourselves seems worthwhile.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It depends on reactive streams library so you dont need to bring rx-Java in. @ktoso correct me if I am wrong.
Have an example of a specific controller to get a better understanding?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Akka Streams does not depend on Rx of course, they both alternative implementations of Reactive Streams ( http://reactive-streams.org/ ) which have been included in JDK9 as
java.util.concurrent.Flow.*and Akka also implements those, but does not require JDK9; you can use JDK8 + RS and if you use JDK9 you could use the JDK's types but it's not required. Both Akka and Rx implement the respective interfaces in RS / JDK, so can inter-op thanks to that (see the RS site for details).Anything else I should clarify or review here? For inter-op purposes it would be good to not expose on a specific implementation but expose the reactive-streams types (
org.reactivestreams.Publisheretc), but that only matters if the types are exposed. As for including dependencies in core Spark -- I would expect this to carry quite a bit of implications though don't know Spark's rules about it (ofc less dependencies == better for users, since less chances to version-clash with libraries they'd use)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These types are not exposed - they're only implementation details in the Kubernetes module. Furthermore the RxJava dependency will be in the Spark distribution but is not a dependency pulled in by spark-core.
It sounds like there is some contention with the extra dependency though, so should we be considering implementing our own mechanisms from the ground up? I think the bottom line question is: can spark-kubernetes, NOT spark-core, pull in RxJava?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ended up just removing reactive programming entirely - the buffering is implemented manually. Please take a look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanx @ktoso!