dongjoon-hyun commented on code in PR #58696:
URL: https://github.com/apache/spark/pull/58696#discussion_r4049163884
##########
core/src/main/scala/org/apache/spark/internal/config/package.scala:
##########
@@ -2831,6 +2831,17 @@ package object config {
.booleanConf
.createWithDefault(false)
+ private[spark] val STANDALONE_SUBMIT_FILTER_ENVIRONMENT =
+ ConfigBuilder("spark.standalone.submit.filterEnvironment")
+ .doc("In standalone cluster mode, controls whether the client forwards
only " +
+ "Spark-related environment variables (e.g. SPARK_* excluding " +
+ "SPARK_ENV_LOADED, SPARK_HOME, SPARK_CONF_DIR) to the driver, " +
+ "matching the REST submission client. If set to false, the full
environment is " +
+ "forwarded to the driver. This does not impact REST submissions except
if they fall back.")
+ .version("4.3.0")
+ .booleanConf
+ .createWithDefault(true)
Review Comment:
This fails `SparkConfigBindingPolicySuite` ("Config enforcement for
bindingPolicy") in the `hive - other tests` job. Every new config needs to
declare a `ConfigBindingPolicy`, and the exceptions file is frozen. Since this
is a submission-side config, `NOT_APPLICABLE` looks right, like the other core
configs in this file.
```suggestion
.withBindingPolicy(ConfigBindingPolicy.NOT_APPLICABLE)
.booleanConf
.createWithDefault(true)
```
##########
docs/core-migration-guide.md:
##########
@@ -24,6 +24,8 @@ license: |
## Upgrading from Core 4.2 to 4.3
+- Since Spark 4.3, standalone deployments now only forward SPARK_* environment
variables by default. If you need to forward additional environment variables
set `spark.standalone.submit.filterEnvironment` to `true`.
Review Comment:
The default is already `true`, so the opt-out value should be `false` here.
```suggestion
- Since Spark 4.3, standalone deployments now only forward SPARK_*
environment variables by default. If you need to forward additional environment
variables set `spark.standalone.submit.filterEnvironment` to `false`.
```
##########
core/src/main/scala/org/apache/spark/deploy/Client.scala:
##########
@@ -275,6 +277,21 @@ object Client {
// scalastyle:on println
new ClientApp().start(args, new SparkConf())
}
+
+ /**
+ * Environment variables to forward to the driver. Only Spark-related
variables are forwarded,
+ * matching the REST submission client, unless
`spark.standalone.submit.filterEnvironment` is
+ * disabled, in which case the full environment of the submitting process is
forwarded.
+ */
+ private[deploy] def driverEnvironment(
+ conf: SparkConf,
+ env: Map[String, String]): Map[String, String] = {
+ if (conf.get(config.STANDALONE_SUBMIT_FILTER_ENVIRONMENT)) {
+ RestSubmissionClient.filterSystemEnvironment(env)
Review Comment:
This is still not quite the same set as the REST path.
`StandaloneRestServer.handleSubmit` also drops `SPARK_LOCAL_(IP|HOSTNAME)` on
the server side, so the submitter's `SPARK_LOCAL_IP` never reaches a driver
that runs on another worker host. The legacy path still forwards it. That was
already the case before this PR, but since the PR says both paths now forward
the same variables, shall we drop them here too, or mention the difference in
the doc?
##########
docs/spark-standalone.md:
##########
@@ -575,6 +575,16 @@ Spark applications supports the following configuration
properties specific to s
</td>
<td>3.1.0</td>
</tr>
+ <tr>
+ <td><code>spark.standalone.submit.filterEnvironment</code></td>
+ <td><code>true</code></td>
+ <td>
+ In standalone cluster mode, controls whether the client forwards only
Spark-related environment
+ variables to the driver (e.g. SPARK_* excluding SPARK_ENV_LOADED SPARK_HOME
and SPARK_CONF_DIR), matching the REST submission client. If set to
<code>false</code>,
Review Comment:
nit: missing commas (`SPARK_ENV_LOADED, SPARK_HOME, and SPARK_CONF_DIR`),
like in `docs/configuration.md`. Also, the new lines in both doc files are much
longer than the surrounding rows. Could we wrap them to match?
##########
core/src/test/scala/org/apache/spark/deploy/ClientSuite.scala:
##########
@@ -48,4 +57,43 @@ class ClientSuite extends SparkFunSuite with Matchers {
// Invalid syntax.
ClientArguments.isValidJarUrl("hdfs:") should be (false)
}
+
+ /**
+ * Launches a driver through a [[ClientEndpoint]] wired to a fake master and
returns the
+ * [[Command]] carried by the [[RequestSubmitDriver]] message the client
sends.
+ */
+ private def submittedCommand(conf: SparkConf): Command = {
+ val env = RpcEnv.create("ClientSuite", "localhost", 0, conf, new
SecurityManager(conf))
+ try {
+ val submitted = Promise[RequestSubmitDriver]()
+ val master = env.setupEndpoint(Master.ENDPOINT_NAME, new RpcEndpoint {
+ override val rpcEnv: RpcEnv = env
+ // Record the submission without replying, so the client neither polls
the driver
+ // status nor exits the JVM.
+ override def receiveAndReply(context: RpcCallContext):
PartialFunction[Any, Unit] = {
+ case request: RequestSubmitDriver => submitted.success(request)
+ }
+ })
+ val args = new ClientArguments(
+ Array("launch", "spark://localhost:7077", "file:///path/to/app.jar",
"MainClass"))
+ env.setupEndpoint("client", new ClientEndpoint(env, args, Seq(master),
conf))
+ ThreadUtils.awaitResult(submitted.future,
10.seconds).driverDescription.command
+ } finally {
+ env.shutdown()
+ env.awaitTermination()
+ }
+ }
+
+ test("SPARK-59404: forward only Spark-related environment variables to the
driver") {
+ // The submitting process always has non-Spark variables such as PATH, so
forwarding
+ // sys.env unfiltered would fail the assertion below.
+ assert(sys.env.keys.exists(!_.startsWith("SPARK_")))
+ val command = submittedCommand(new SparkConf())
+ command.environment should be
(RestSubmissionClient.filterSystemEnvironment(sys.env))
Review Comment:
nit: the expected value comes from `filterSystemEnvironment` itself, so this
would still pass if the filter were wrong. Could we add a check that does not
depend on the implementation, e.g.
```scala
command.environment.keys.forall(_.startsWith("SPARK_")) should be (true)
command.environment should not contain key ("PATH")
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
