dongjoon-hyun commented on PR #794:
URL:
https://github.com/apache/spark-kubernetes-operator/pull/794#issuecomment-5535573623
Thank you for the update, @yalindogusahin. The new direction (dedicated port
+ explicit `NetworkPolicyPeer` list, nothing generated unless both are set)
looks right to me, and the generated policy is exactly what I would expect:
```yaml
ingress:
- from:
- podSelector:
matchLabels:
spark.operator/spark-cluster-name: cluster-with-jmx-exporter
- podSelector:
matchLabels:
spark-role: driver
- from:
- namespaceSelector:
matchLabels:
kubernetes.io/metadata.name: monitoring
ports:
- port: 9404
protocol: TCP
```
I built the PR locally and `:spark-operator-api:build`,
`:spark-submission-worker:build`, `javadoc`, and
`assertGeneratedCRDMatchesHelmChart` all pass, so the CRD and the Helm chart
are in sync.
However, there is one blocker before this can go in.
## The documented setup does not actually work
`docs/operations.md` (step 3) and `examples/cluster-with-jmx-exporter.yaml`
both attach the agent through `SPARK_WORKER_OPTS`, but the operator appends its
own `SPARK_WORKER_OPTS` to the worker container *after* merging the user's pod
template — see `SparkClusterResourceSpec#buildWorkerStatefulSet`:
```java
.editMatchingContainer(p -> "worker".equals(p.getName()))
.withImage(image)
.addNewEnv().withName("SPARK_LOG_DIR")...endEnv()
.addNewEnv().withName("SPARK_WORKER_OPTS").withValue(options).endEnv()
```
Feeding `examples/cluster-with-jmx-exporter.yaml` through
`SparkClusterResourceSpec` and dumping the resulting container gives:
```
container=worker
env SPARK_WORKER_OPTS =
-javaagent:/opt/jmx_exporter/jmx_prometheus_javaagent.jar=9404:/etc/metrics/jmx-exporter-config.yaml
env SPARK_LOG_DIR = /opt/spark/work/logs
env SPARK_WORKER_OPTS =
```
kubelet's `makeEnvironmentVariables` collapses duplicate names into a map,
so the operator's (here empty) value wins and the user's `-javaagent` is
silently dropped. The port would be opened, but nothing would be listening on
it.
Please use `SPARK_DAEMON_JAVA_OPTS` instead. Spark's
`SparkClassCommandBuilder` adds both `SPARK_DAEMON_JAVA_OPTS` and
`SPARK_WORKER_OPTS` for `org.apache.spark.deploy.worker.Worker`, and the
operator never sets the former, so there is no collision.
## Example / docs
- The docs call the example "complete, runnable", but the example pulls the
stock `apache/spark:{{SPARK_VERSION}}-scala` image, which does not contain
`/opt/jmx_exporter/jmx_prometheus_javaagent.jar`. Once the env-var issue above
is fixed, a `-javaagent` pointing at a missing jar aborts JVM startup. Please
drop the "runnable" wording, or note the custom-image prerequisite next to the
image field itself.
- `spark.ui.reverseProxy: "true"` in the example is unrelated to this
feature.
- The docs pin `jmx_prometheus_javaagent` 1.0.1; a version-less link would
age better.
## API shape
Since these land in the `v1` CRD and are effectively permanent, could we
group them?
```yaml
workerSpec:
networkPolicy:
metricsPort: 9404
metricsIngress:
- namespaceSelector: ...
```
This mirrors the chart's `operatorDeployment.networkPolicy.metricsIngress`
nesting more closely, keeps `WorkerSpec` from growing two flat fields that are
only meaningful together, and makes it obvious that `metricsPort` configures
the policy rather than any metrics endpoint (which is what your own naming
question was getting at).
Related: the Javadoc says the web UI port "is deliberately not usable as the
metrics port", but nothing rejects `metricsPort: 8081` — it would open the UI
to the listed peers. Either validate it or soften the wording.
## Minor
- The branch is 14 commits behind `main`; please rebase.
- No `tests/e2e` coverage for the new fields. Optional, but worth
considering for a user-facing CRD field whose effect is only observable at
runtime.
--
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]
