Skip to content

Commit 1b1ced8

Browse files
committed
[SPARK-59296][CORE][FOLLOWUP] Address round-2 review: hoist UserCredentialManager lifecycle into SupportsDelegationToken and tighten local-mode wiring
Addresses the second round of review on PR #58737: - Hoist the UserCredentialManager lifecycle (field + setup/stopUserCredentialManager) into SupportsDelegationToken, next to the sibling HadoopDelegationTokenManager, so both CoarseGrainedSchedulerBackend and LocalSchedulerBackend share one implementation. The two backends now differ only via a `propagateUserCredentials(version, credentials)` hook: CGSB updates the driver store then broadcasts via its DriverEndpoint; LocalSchedulerBackend (no remote executors) updates the shared store directly. The trait gains an abstract `scheduler` accessor (both backends already hold `scheduler`). - Bind the manager to `scheduler.sc.conf` (the live SparkConf) rather than the constructor `conf`. LocalSchedulerBackend receives `sc.getConf`, a clone; the resolution-time fallback in UserCredentialManager.start() (which applies provider-declared spark.* keys for schemes the selection phase could not) wrote into that clone and never reached sc.conf, so it silently did nothing in local mode while working in cluster mode. Using scheduler.sc.conf restores parity. - Use `scheduler.sc.env.userCredentials` (this backend's own env store) instead of the global SparkEnv.get, so a renewal that outlives this SparkContext cannot write into a different SparkContext's store created later in the same JVM. - Drop the now-redundant separate initial synchronous store: start() invokes the callback synchronously, and for both backends the callback updates the store, so foreach(_.start()) is enough (CGSB's callback still updates the store synchronously before the async broadcast, keeping the no-null-window guarantee). - LocalSchedulerBackend.stop(): guard localEndpoint (ask() only throws synchronously when localEndpoint is still null, i.e. a launcher stop before start()) and stop the managers in a finally, matching CGSB. Add a `stopped` flag checked in start() so a stop(KILLED) arriving before start() does not bring up the renewal thread (network I/O) on an already-killed app. - Tests/docs: strengthen the selection/resolution ordering test to assert init count 0 after selection and 1 only after start() (proving loader reuse, not just loader idempotency), using the test-only 4-arg constructor and a mock ingestor (no token file) with loader.closeAll() in finally; build JWTs with jjwt and use withTempDir in LocalSchedulerBackendSuite; drop a duplicate applyProviderProperties test; and note LocalSchedulerBackend in the UserCredentialManager class scaladoc.
1 parent e07a9b2 commit 1b1ced8

6 files changed

Lines changed: 166 additions & 198 deletions

File tree

core/src/main/scala/org/apache/spark/deploy/security/UserCredentialManager.scala

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -51,8 +51,9 @@ import org.apache.spark.util.{ThreadUtils, Utils}
5151
* safetyMargin`
5252
* 5. Retries with exponential backoff on failure
5353
*
54-
* Intended to be started from `CoarseGrainedSchedulerBackend.start()` when
55-
* `spark.security.oidc.enabled=true`, independently of
54+
* Intended to be started from a scheduler backend -- `CoarseGrainedSchedulerBackend.start()` or
55+
* `LocalSchedulerBackend.start()` (both via `SupportsDelegationToken.setupUserCredentialManager()`)
56+
* -- when `spark.security.oidc.enabled=true`, independently of
5657
* `UserGroupInformation.isSecurityEnabled()`.
5758
*
5859
* Lifecycle: call `start()` exactly once, then `stop()` to shut down.

core/src/main/scala/org/apache/spark/scheduler/SupportsDelegationToken.scala

Lines changed: 46 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ package org.apache.spark.scheduler
2020
import org.apache.hadoop.security.UserGroupInformation
2121

2222
import org.apache.spark.deploy.SparkHadoopUtil
23-
import org.apache.spark.deploy.security.HadoopDelegationTokenManager
23+
import org.apache.spark.deploy.security.{HadoopDelegationTokenManager, UserCredentialManager}
2424

2525
/**
2626
* A mix-in trait for SchedulerBackend that supports delegation tokens.
@@ -30,6 +30,20 @@ private[spark] trait SupportsDelegationToken {
3030
// The token manager used to create security tokens.
3131
protected var delegationTokenManager: Option[HadoopDelegationTokenManager] = None
3232

33+
// The OIDC user-credential manager, a sibling of the Kerberos delegation token manager.
34+
// Its lifecycle lives here (like delegationTokenManager) so that both
35+
// CoarseGrainedSchedulerBackend and LocalSchedulerBackend share a single implementation; the
36+
// two backends differ only in how they propagate credentials, expressed via
37+
// propagateUserCredentials().
38+
protected var userCredentialManager: Option[UserCredentialManager] = None
39+
40+
/**
41+
* The task scheduler this backend belongs to. Implemented by the mixing-in backend (both
42+
* CoarseGrainedSchedulerBackend and LocalSchedulerBackend already hold a `scheduler` field).
43+
* Used to reach `scheduler.sc.conf` / `scheduler.sc.env` / the OIDC credential provider loader.
44+
*/
45+
protected def scheduler: TaskSchedulerImpl
46+
3347
/**
3448
* Create the delegation token manager to be used for the application. This method is called
3549
* once during the start of the scheduler backend (so after the object has already been
@@ -42,6 +56,15 @@ private[spark] trait SupportsDelegationToken {
4256
*/
4357
protected def updateDelegationTokens(tokens: Array[Byte]): Unit
4458

59+
/**
60+
* Propagate a freshly acquired set of OIDC user credentials. Called on the driver by the
61+
* [[UserCredentialManager]] (initially from `start()`, then on each renewal). Implemented per
62+
* backend: `CoarseGrainedSchedulerBackend` updates the driver store and broadcasts to executors
63+
* via its `DriverEndpoint`; `LocalSchedulerBackend` (no remote executors) updates the shared
64+
* credential store directly.
65+
*/
66+
protected def propagateUserCredentials(version: Long, credentials: Array[Byte]): Unit
67+
4568
/**
4669
* Whether the token manager should be started. The default implementation returns true when
4770
* Hadoop security is enabled. Backends that support direct credential providers override this
@@ -75,4 +98,26 @@ private[spark] trait SupportsDelegationToken {
7598
protected def stopTokenManager(): Unit = {
7699
delegationTokenManager.foreach(_.stop())
77100
}
101+
102+
/**
103+
* Start the [[UserCredentialManager]] if OIDC credential propagation is enabled. Called once
104+
* during scheduler-backend start, independently of the Kerberos delegation token manager.
105+
*
106+
* Binds the manager to `scheduler.sc.conf` (the live SparkConf, not a clone) so the
107+
* resolution-time fallback in `UserCredentialManager.start()` -- which applies provider-declared
108+
* `spark.*` properties for any scheme the selection phase could not -- reaches the same conf on
109+
* both backends. Reuses the [[org.apache.spark.security.CredentialProviderLoader]] produced by
110+
* SparkContext's selection phase so providers are initialized exactly once; SparkContext remains
111+
* the loader's single owner. `start()` invokes `propagateUserCredentials` synchronously for the
112+
* initial credentials, so no separate initial store is needed here.
113+
*/
114+
protected def setupUserCredentialManager(): Unit = {
115+
userCredentialManager = UserCredentialManager.create(
116+
scheduler.sc.conf, propagateUserCredentials, scheduler.sc.userCredentialProviderLoader)
117+
userCredentialManager.foreach(_.start())
118+
}
119+
120+
protected def stopUserCredentialManager(): Unit = {
121+
userCredentialManager.foreach(_.stop())
122+
}
78123
}

core/src/main/scala/org/apache/spark/scheduler/cluster/CoarseGrainedSchedulerBackend.scala

Lines changed: 14 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,6 @@ import com.google.common.cache.CacheBuilder
2828

2929
import org.apache.spark.{ExecutorAllocationClient, SparkEnv, TaskState, VersionedCredentials}
3030
import org.apache.spark.deploy.SparkHadoopUtil
31-
import org.apache.spark.deploy.security.UserCredentialManager
3231
import org.apache.spark.errors.SparkCoreErrors
3332
import org.apache.spark.executor.ExecutorLogUrlHandler
3433
import org.apache.spark.internal.{config, Logging}
@@ -54,7 +53,7 @@ import org.apache.spark.util.ArrayImplicits._
5453
* Spark's standalone deploy mode (spark.deploy.*).
5554
*/
5655
private[spark]
57-
class CoarseGrainedSchedulerBackend(scheduler: TaskSchedulerImpl, val rpcEnv: RpcEnv)
56+
class CoarseGrainedSchedulerBackend(protected val scheduler: TaskSchedulerImpl, val rpcEnv: RpcEnv)
5857
extends ExecutorAllocationClient with SchedulerBackend
5958
with SupportsDelegationToken with Logging {
6059

@@ -144,9 +143,6 @@ class CoarseGrainedSchedulerBackend(scheduler: TaskSchedulerImpl, val rpcEnv: Rp
144143
// Current set of delegation tokens to send to executors.
145144
private val delegationTokens = new AtomicReference[Array[Byte]]()
146145

147-
// UserCredentialManager for OIDC credential propagation (if enabled).
148-
private var userCredentialManager: Option[UserCredentialManager] = None
149-
150146
private val reviveThread =
151147
ThreadUtils.newDaemonSingleThreadScheduledExecutor("driver-revive-thread")
152148

@@ -1252,42 +1248,26 @@ class CoarseGrainedSchedulerBackend(scheduler: TaskSchedulerImpl, val rpcEnv: Rp
12521248
* Called from the DriverEndpoint receive loop (thread-safe access to executorDataMap).
12531249
*/
12541250
private def updateUserCredentials(version: Long, credentials: Array[Byte]): Unit = {
1255-
VersionedCredentials.updateIfNewer(SparkEnv.get.userCredentials, version, credentials)
1251+
VersionedCredentials.updateIfNewer(scheduler.sc.env.userCredentials, version, credentials)
12561252
executorDataMap.values.foreach { ed =>
12571253
ed.executorEndpoint.send(UpdateUserCredentials(version, credentials))
12581254
}
12591255
}
12601256

12611257
/**
1262-
* Start the UserCredentialManager if OIDC credential propagation is enabled.
1263-
* Called from start(), independently of Kerberos/HadoopDelegationTokenManager.
1258+
* Propagate OIDC user credentials to executors. Called on the driver by the
1259+
* [[org.apache.spark.deploy.security.UserCredentialManager]] (initially and on each renewal).
1260+
*
1261+
* Updates the driver's own credential store synchronously so the credentials are available for
1262+
* `SparkAppConfig` (late-registering executors) and `TaskDescription` (task dispatch) with no
1263+
* null window, then broadcasts to registered executors via the `DriverEndpoint` (mirroring
1264+
* `HadoopDelegationTokenManager`'s `UpdateDelegationTokens` path) to ensure thread-safe access
1265+
* to `executorDataMap`.
12641266
*/
1265-
private def setupUserCredentialManager(): Unit = {
1266-
// Reuse the loader from SparkContext's selection phase (Some when OIDC is enabled, None
1267-
// otherwise). Passing the Option straight through keeps SparkContext as the single owner of
1268-
// the loader: create() enforces that an enabled configuration has a loader, rather than
1269-
// silently allocating one here that no one would close.
1270-
userCredentialManager = UserCredentialManager.create(conf, { (version, credentials) =>
1271-
// Send to DriverEndpoint to ensure thread-safe access to executorDataMap.
1272-
// This mirrors HadoopDelegationTokenManager's pattern of sending
1273-
// UpdateDelegationTokens via schedulerRef.
1274-
driverEndpoint.send(UpdateUserCredentials(version, credentials))
1275-
}, scheduler.sc.userCredentialProviderLoader)
1276-
userCredentialManager.foreach { manager =>
1277-
val (version, initialCredentials) = manager.start()
1278-
// Store initial credentials synchronously so they are available for SparkAppConfig
1279-
// (late-registering executors) and TaskDescription (task dispatch) immediately.
1280-
// Note: the onCredentialsUpdate callback above also triggers an async
1281-
// UpdateUserCredentials message that will redundantly call updateIfNewer.
1282-
// The synchronous set here ensures no null window before the async message
1283-
// is processed by DriverEndpoint.
1284-
VersionedCredentials.updateIfNewer(
1285-
SparkEnv.get.userCredentials, version, initialCredentials)
1286-
}
1287-
}
1288-
1289-
private def stopUserCredentialManager(): Unit = {
1290-
userCredentialManager.foreach(_.stop())
1267+
override protected def propagateUserCredentials(
1268+
version: Long, credentials: Array[Byte]): Unit = {
1269+
VersionedCredentials.updateIfNewer(scheduler.sc.env.userCredentials, version, credentials)
1270+
driverEndpoint.send(UpdateUserCredentials(version, credentials))
12911271
}
12921272

12931273
/**

core/src/main/scala/org/apache/spark/scheduler/local/LocalSchedulerBackend.scala

Lines changed: 40 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ import java.nio.ByteBuffer
2424
import org.apache.spark.{SparkConf, SparkContext, SparkEnv, TaskState, VersionedCredentials}
2525
import org.apache.spark.TaskState.TaskState
2626
import org.apache.spark.deploy.SparkHadoopUtil
27-
import org.apache.spark.deploy.security.{HadoopDelegationTokenManager, UserCredentialManager}
27+
import org.apache.spark.deploy.security.HadoopDelegationTokenManager
2828
import org.apache.spark.executor.{Executor, ExecutorBackend}
2929
import org.apache.spark.internal.{config, Logging, LogKeys}
3030
import org.apache.spark.launcher.{LauncherBackend, SparkAppHandle}
@@ -112,20 +112,19 @@ private[spark] class LocalEndpoint(
112112
*/
113113
private[spark] class LocalSchedulerBackend(
114114
conf: SparkConf,
115-
scheduler: TaskSchedulerImpl,
115+
protected val scheduler: TaskSchedulerImpl,
116116
val totalCores: Int)
117117
extends SchedulerBackend with ExecutorBackend with SupportsDelegationToken with Logging {
118118

119119
private val appId = conf.get("spark.test.appId", "local-" + System.currentTimeMillis)
120120
private var localEndpoint: RpcEndpointRef = null
121+
// Set true by stop() so that a stop request arriving before start() (e.g. a launcher KILLED
122+
// request; launcherBackend.connect() runs in the constructor and onStopRequest fires on its own
123+
// thread) prevents start() from bringing up the executor endpoint and, in particular, the
124+
// UserCredentialManager renewal thread (which performs network I/O) on an already-killed app.
125+
@volatile private var stopped = false
121126
private val userClassPath = getUserClasspath(conf)
122127
private val listenerBus = scheduler.sc.listenerBus
123-
124-
// UserCredentialManager for OIDC credential propagation (if enabled). Started in start() and
125-
// stopped in stop(), mirroring CoarseGrainedSchedulerBackend so that OIDC credentials are
126-
// acquired and renewed in local mode too, for parity with HadoopDelegationTokenManager (which
127-
// this backend already runs via createTokenManager()).
128-
private var userCredentialManager: Option[UserCredentialManager] = None
129128
private val launcherBackend = new LauncherBackend() {
130129
override def conf: SparkConf = LocalSchedulerBackend.this.conf
131130
override def onStopRequest(): Unit = stop(SparkAppHandle.State.KILLED)
@@ -144,45 +143,22 @@ private[spark] class LocalSchedulerBackend(
144143
}
145144

146145
/**
147-
* Start the UserCredentialManager if OIDC credential propagation is enabled, mirroring
148-
* CoarseGrainedSchedulerBackend. Runs independently of Kerberos/HadoopDelegationTokenManager.
146+
* Propagate OIDC user credentials in local mode. There are no remote executors here: the driver
147+
* and the single in-JVM executor share `scheduler.sc.env.userCredentials`, so this updates that
148+
* store directly and subsequently dispatched tasks (and driver-side access) observe the new
149+
* credentials. `UserCredentialManager.start()` calls this synchronously for the initial
150+
* credentials, so they are in the store before any task is offered.
149151
*
150-
* In local mode the driver and the single executor share this JVM and the same
151-
* `SparkEnv.get.userCredentials`, so the propagation callback simply updates that reference
152-
* (there is no remote executor to message); the in-JVM Executor picks up credentials from the
153-
* same store via TaskDescription. Driver-side filesystem access uses the provider wiring that
154-
* the selection phase (UserCredentialManager.applyProviderProperties) already applied to the
155-
* driver's Hadoop Configuration.
152+
* The store is taken from `scheduler.sc.env` (this backend's own env), not the global
153+
* `SparkEnv.get`: `UserCredentialManager.stop()` only waits a bounded time for the renewal
154+
* thread, so a renewal that outlives this SparkContext must not write into a different
155+
* SparkContext's store (e.g. a new context created in the same JVM by a notebook, test, or
156+
* Spark Connect session). Binding to this env's store confines a late renewal to this
157+
* application's (by then unused) store.
156158
*/
157-
private def setupUserCredentialManager(): Unit = {
158-
// Capture this backend's SparkEnv once, rather than looking up the global SparkEnv.get on
159-
// every callback. stop() only waits a bounded time for the renewal thread, so a renewal that
160-
// outlives this SparkContext must not write into a *different* SparkContext's credential
161-
// store (e.g. a new context created in the same JVM by a notebook, test, or Spark Connect
162-
// session). Binding to this env ensures a late renewal updates only this application's store,
163-
// which is harmless once this env is stopped. (CoarseGrainedSchedulerBackend avoids the issue
164-
// differently, by routing updates through its own already-stopped driverEndpoint.)
165-
val env = SparkEnv.get
166-
// Reuse the loader from SparkContext's selection phase (Some when OIDC is enabled, None
167-
// otherwise). Passing the Option straight through keeps SparkContext as the single owner of
168-
// the loader: create() enforces that an enabled configuration has a loader rather than
169-
// silently allocating one here that no one would close.
170-
userCredentialManager = UserCredentialManager.create(conf, { (version, credentials) =>
171-
// No remote executors in local mode; update the shared credential store directly so that
172-
// subsequently dispatched tasks (and driver-side access) observe the new credentials.
173-
VersionedCredentials.updateIfNewer(env.userCredentials, version, credentials)
174-
}, scheduler.sc.userCredentialProviderLoader)
175-
userCredentialManager.foreach { manager =>
176-
val (version, initialCredentials) = manager.start()
177-
// Store initial credentials synchronously so they are available for TaskDescription
178-
// (task dispatch) immediately. The onCredentialsUpdate callback above also runs the same
179-
// updateIfNewer, so this is idempotent.
180-
VersionedCredentials.updateIfNewer(env.userCredentials, version, initialCredentials)
181-
}
182-
}
183-
184-
private def stopUserCredentialManager(): Unit = {
185-
userCredentialManager.foreach(_.stop())
159+
override protected def propagateUserCredentials(
160+
version: Long, credentials: Array[Byte]): Unit = {
161+
VersionedCredentials.updateIfNewer(scheduler.sc.env.userCredentials, version, credentials)
186162
}
187163

188164
/**
@@ -198,6 +174,12 @@ private[spark] class LocalSchedulerBackend(
198174
launcherBackend.connect()
199175

200176
override def start(): Unit = {
177+
// If a stop request already arrived (e.g. launcher KILLED before start()), do not bring up the
178+
// executor endpoint or the token/credential managers on an app that is already stopping.
179+
if (stopped) {
180+
logInfo("Not starting LocalSchedulerBackend because it was already stopped")
181+
return
182+
}
201183
val rpcEnv = SparkEnv.get.rpcEnv
202184
val executorEndpoint = new LocalEndpoint(rpcEnv, userClassPath, scheduler, this, totalCores)
203185
localEndpoint = rpcEnv.setupEndpoint("LocalSchedulerBackendEndpoint", executorEndpoint)
@@ -246,13 +228,19 @@ private[spark] class LocalSchedulerBackend(
246228
}
247229

248230
private def stop(finalState: SparkAppHandle.State): Unit = {
249-
// Ensure both managers are always stopped, even if stopping the executor endpoint throws.
250-
// The UserCredentialManager renewal thread must be shut down before SparkContext.stop()
251-
// closes the shared CredentialProviderLoader, otherwise a renewal task could race against an
252-
// already-closed loader. Each step is isolated so that a failure in one does not skip the
253-
// others (mirrors CoarseGrainedSchedulerBackend.stop, which stops the managers in a finally).
254-
Utils.tryLogNonFatalError {
255-
localEndpoint.ask(StopExecutor)
231+
// Mark stopped so a start() that has not run yet becomes a no-op (a KILLED request can race
232+
// ahead of start()). The executor endpoint only needs stopping if start() already created it;
233+
// guard on localEndpoint being non-null, mirroring CoarseGrainedSchedulerBackend's
234+
// `if (driverEndpoint != null)`. The token and user-credential managers are always stopped
235+
// (each isolated with tryLogNonFatalError so a failure in one does not skip the other or the
236+
// launcher state update below); the UserCredentialManager renewal thread must be shut down
237+
// before SparkContext.stop() closes the shared CredentialProviderLoader, otherwise a renewal
238+
// task could race against an already-closed loader.
239+
stopped = true
240+
if (localEndpoint != null) {
241+
Utils.tryLogNonFatalError {
242+
localEndpoint.ask(StopExecutor)
243+
}
256244
}
257245
Utils.tryLogNonFatalError {
258246
stopTokenManager()

0 commit comments

Comments
 (0)