You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "yalindogusahin (via GitHub)" <gi...@apache.org> on 2026/08/10 12:47:43 UTC
[PR] [SPARK-58691] Allow metrics scraping ingress to SparkCluster worker web port [spark-kubernetes-operator]
yalindogusahin opened a new pull request, #794:
URL: https://github.com/apache/spark-kubernetes-operator/pull/794
### What changes were proposed in this pull request?
This PR adds a second ingress rule to the worker `NetworkPolicy` generated for every
`SparkCluster`, admitting any source on the worker's `web` port (8081) — the port that
serves the worker web UI and, when `PrometheusServlet` is configured via
`spark.metrics.conf`, the `/metrics/prometheus` scrape endpoint. The existing rule
(cluster label / driver label allow-list, unrestricted ports) is untouched, so RPC,
shuffle, and block-manager ports stay locked down exactly as before.
### Why are the changes needed?
The worker `NetworkPolicy` generated in `SparkClusterResourceSpec#buildWorkerNetworkPolicy`
only admits ingress from pods carrying the cluster label or, since SPARK-58649, the driver
role label. Prometheus scraper pods carry neither, so they're indistinguishable from any
other unrelated pod and are locked out entirely — including from the metrics endpoint,
which is not sensitive and is meant to be broadly scrapable.
Verified against a live cluster running an affected build: with only the pre-fix policy in
place, `curl http://<worker-ip>:8081/metrics/prometheus` from an unlabeled pod times out.
The worker itself serves real metrics fine locally (`PrometheusServlet` works as configured
via `sparkConf`); the `NetworkPolicy` is what blocks the scrape. Masters are unaffected
since they have no `NetworkPolicy` applied.
### Does this PR introduce _any_ user-facing change?
Yes. The generated worker `NetworkPolicy` now also admits ingress on the `web` port from
any source. Since NetworkPolicies are additive this can only widen ingress on that one
port, so it cannot deny traffic that was previously allowed. Clusters running on a CNI that
does not enforce NetworkPolicy are unaffected either way.
### How was this patch tested?
Extended `SparkClusterResourceSpecTest#testWorkerNetworkPolicy` to assert the new ingress
rule (no `from` restriction, single port named `web`).
```
./gradlew :spark-submission-worker:test --tests '*SparkClusterResourceSpecTest*'
./gradlew :spark-submission-worker:spotlessCheck
```
Both pass.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Sonnet 5
--
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
Re: [PR] [SPARK-58691] Add configurable worker metrics NetworkPolicy ingress [spark-kubernetes-operator]
Posted by "yalindogusahin (via GitHub)" <gi...@apache.org>.
yalindogusahin commented on PR #794:
URL: https://github.com/apache/spark-kubernetes-operator/pull/794#issuecomment-5715124469
Thanks for the detailed review. I rebased and pushed 79d6571.
The example now matches actual worker gauge MBeans, uses a worker-only JmxSink, and limits JMX queries. With the exporter jar supplied, I scraped a local Spark 4.1.2 worker and saw spark_worker_coresFree, spark_worker_coresUsed, spark_worker_executors, spark_worker_memFree_MB, and spark_worker_memUsed_MB.
I also made metricsIngress nonempty in the CRD, kept the Java policy builder fail closed, moved NetworkPolicy creation before the StatefulSets, and documented that changing scraper peers requires recreating the cluster. I updated the PR description to reflect these changes.
--
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
Re: [PR] [SPARK-58691] Add configurable worker metrics NetworkPolicy ingress [spark-kubernetes-operator]
Posted by "yalindogusahin (via GitHub)" <gi...@apache.org>.
yalindogusahin commented on code in PR #794:
URL: https://github.com/apache/spark-kubernetes-operator/pull/794#discussion_r4015275206
##########
spark-submission-worker/src/main/java/org/apache/spark/k8s/operator/SparkClusterResourceSpec.java:
##########
@@ -467,35 +469,67 @@ private static Optional<PodDisruptionBudget> buildPodDisruptionBudget(
* resource and does not carry the cluster label, yet the driver must reach the executors' block
* manager to fetch task results larger than {@code spark.task.maxDirectResultSize}.
*
+ * <p>If both {@link WorkerNetworkPolicySpec#getMetricsPort()} and {@link
+ * WorkerNetworkPolicySpec#getMetricsIngress()} are set, a separate rule admits the listed peers
+ * on that port only. Both are required: the port alone would open the endpoint to every source in
+ * the cluster, and the peers alone would grant them every port. The worker web UI port is
+ * deliberately not usable as the metrics port: it is served by the same embedded HTTP server as
+ * the UI itself, so admitting it would also expose the full UI, not just a metrics endpoint. A
+ * dedicated metrics port (e.g. a JMX-to-Prometheus exporter agent) keeps metrics scraping
+ * separate from the UI.
+ *
* @param clusterName The name of the SparkCluster.
* @param namespace The namespace of the SparkApplication.
+ * @param workerSpec The WorkerSpec, used to look up the optional metrics port and its peers.
* @return A NetworkPolicy object.
+ * @throws IllegalArgumentException if a metrics ingress rule is requested on the worker web UI
+ * port.
*/
- private NetworkPolicy buildWorkerNetworkPolicy(String clusterName, String namespace) {
- return new NetworkPolicyBuilder()
- .withNewMetadata()
- .withName(clusterName + "-worker")
- .withNamespace(namespace)
- .addToLabels(LABEL_SPARK_CLUSTER_NAME, clusterName)
- .endMetadata()
- .withNewSpec()
- .withNewPodSelector()
- .addToMatchLabels(LABEL_SPARK_ROLE_NAME, LABEL_SPARK_ROLE_WORKER_VALUE)
- .addToMatchLabels(LABEL_SPARK_CLUSTER_NAME, clusterName)
- .endPodSelector()
- .addNewIngress()
- .addNewFrom()
- .withNewPodSelector()
- .addToMatchLabels(LABEL_SPARK_CLUSTER_NAME, clusterName)
- .endPodSelector()
- .endFrom()
- .addNewFrom()
- .withNewPodSelector()
- .addToMatchLabels(LABEL_SPARK_ROLE_NAME, LABEL_SPARK_ROLE_DRIVER_VALUE)
- .endPodSelector()
- .endFrom()
- .endIngress()
- .endSpec()
- .build();
+ private NetworkPolicy buildWorkerNetworkPolicy(
+ String clusterName, String namespace, WorkerSpec workerSpec) {
+ var builder =
+ new NetworkPolicyBuilder()
+ .withNewMetadata()
+ .withName(clusterName + "-worker")
+ .withNamespace(namespace)
+ .addToLabels(LABEL_SPARK_CLUSTER_NAME, clusterName)
+ .endMetadata()
+ .withNewSpec()
+ .withNewPodSelector()
+ .addToMatchLabels(LABEL_SPARK_ROLE_NAME, LABEL_SPARK_ROLE_WORKER_VALUE)
+ .addToMatchLabels(LABEL_SPARK_CLUSTER_NAME, clusterName)
+ .endPodSelector()
+ .addNewIngress()
+ .addNewFrom()
+ .withNewPodSelector()
+ .addToMatchLabels(LABEL_SPARK_CLUSTER_NAME, clusterName)
+ .endPodSelector()
+ .endFrom()
+ .addNewFrom()
+ .withNewPodSelector()
+ .addToMatchLabels(LABEL_SPARK_ROLE_NAME, LABEL_SPARK_ROLE_DRIVER_VALUE)
+ .endPodSelector()
+ .endFrom()
+ .endIngress();
+ WorkerNetworkPolicySpec networkPolicy = workerSpec.getNetworkPolicy();
+ if (networkPolicy != null
+ && networkPolicy.getMetricsPort() != null
+ && networkPolicy.getMetricsIngress() != null
+ && !networkPolicy.getMetricsIngress().isEmpty()) {
+ if (networkPolicy.getMetricsPort() == 8081) {
Review Comment:
Removed it and updated the docs/Javadoc to recommend a dedicated exporter port and clarify that the operator doesn't verify this.
--
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
Re: [PR] [SPARK-58691] Add configurable worker metrics NetworkPolicy ingress [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #794:
URL: https://github.com/apache/spark-kubernetes-operator/pull/794#discussion_r4036153420
##########
examples/cluster-with-jmx-exporter.yaml:
##########
@@ -0,0 +1,78 @@
+# 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.
+apiVersion: v1
+kind: ConfigMap
+metadata:
+ name: jmx-exporter-config
+data:
+ jmx-exporter-config.yaml: |
+ rules:
+ - pattern: 'java.lang<type=Memory><HeapMemoryUsage>(\w+)'
+ name: jvm_memory_heap_$1
+ type: GAUGE
+ - pattern: 'java.lang<type=GarbageCollector, name=(\w+)><>CollectionCount'
+ name: jvm_gc_collection_count
+ labels:
+ gc: "$1"
+ type: COUNTER
+ - pattern: 'metrics<name=(\S+)><>Value'
Review Comment:
This rule still never matches, and my previous comment was incomplete: `JmxSink` alone does not fix it. Spark bundles Dropwizard `metrics-jmx` 4.2.37, whose `DefaultObjectNameFactory` puts both `name` and `type` into the ObjectName, e.g. `metrics:name=worker.coresFree,type=gauges`. jmx_exporter renders that as `metrics<name=worker.coresFree, type=gauges><>Value` and wraps the rule as `^.*(?:pattern).*$`, so `(\S+)>` cannot get past the `, `. I checked the regex against both property orders on a local JVM and it is false for both. With a non-empty `rules` list unmatched beans are dropped, so the scrape has no `spark_worker_*` series at all.
Suggested:
```yaml
- pattern: 'metrics<name=worker\.(\w+), type=gauges><>Value'
name: spark_worker_$1
type: GAUGE
```
A plain `(\S+), type=gauges` would produce `spark_worker_worker_coresFree`, because `WorkerSource.sourceName` is already `worker`. The sentence in `docs/operations.md` ("Without this sink, the exporter only exposes JVM metrics") is right, but please make sure the example actually delivers the other half.
##########
examples/cluster-with-jmx-exporter.yaml:
##########
@@ -0,0 +1,78 @@
+# 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.
+apiVersion: v1
+kind: ConfigMap
+metadata:
+ name: jmx-exporter-config
+data:
+ jmx-exporter-config.yaml: |
+ rules:
+ - pattern: 'java.lang<type=Memory><HeapMemoryUsage>(\w+)'
+ name: jvm_memory_heap_$1
+ type: GAUGE
+ - pattern: 'java.lang<type=GarbageCollector, name=(\w+)><>CollectionCount'
Review Comment:
`name=(\w+)` does not match the default collectors. On a JVM with `-XX:+UseG1GC` the beans are `G1 Young Generation`, `G1 Concurrent GC`, and `G1 Old Generation`; the spaces make this rule fail, and it only matches SerialGC names like `Copy`. G1 is the default on the JDK 17/21 `apache/spark` images once the pod has 2+ CPUs and about 1.8 GB, so `jvm_gc_collection_count` is never emitted. Either use `name=([^>]+)`, or drop the two JVM rules altogether: the javaagent already exports `jvm_memory_*` and `jvm_gc_*` from its built-in collectors.
##########
examples/cluster-with-jmx-exporter.yaml:
##########
@@ -0,0 +1,78 @@
+# 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.
+apiVersion: v1
+kind: ConfigMap
+metadata:
+ name: jmx-exporter-config
+data:
+ jmx-exporter-config.yaml: |
+ rules:
+ - pattern: 'java.lang<type=Memory><HeapMemoryUsage>(\w+)'
+ name: jvm_memory_heap_$1
+ type: GAUGE
+ - pattern: 'java.lang<type=GarbageCollector, name=(\w+)><>CollectionCount'
+ name: jvm_gc_collection_count
+ labels:
+ gc: "$1"
+ type: COUNTER
+ - pattern: 'metrics<name=(\S+)><>Value'
+ name: spark_worker_$1
+ type: GAUGE
+---
+apiVersion: spark.apache.org/v1
+kind: SparkCluster
+metadata:
+ name: cluster-with-jmx-exporter
+spec:
+ runtimeVersions:
+ sparkVersion: "4.2.0"
+ clusterTolerations:
+ instanceConfig:
+ initWorkers: 1
+ minWorkers: 1
+ maxWorkers: 1
+ workerSpec:
+ networkPolicy:
+ metricsPort: 9404
+ # Allow scraper pods in "monitoring", in addition to the default cluster/driver peers.
+ metricsIngress:
+ - namespaceSelector:
+ matchLabels:
+ kubernetes.io/metadata.name: "monitoring"
+ statefulSetSpec:
+ template:
+ spec:
+ containers:
+ - name: worker
+ # Requires a custom image that has jmx_prometheus_javaagent pre-baked at this path
+ # (the stock apache/spark image does not include it).
+ env:
+ - name: SPARK_DAEMON_JAVA_OPTS
+ value: "-javaagent:/opt/jmx_exporter/jmx_prometheus_javaagent.jar=9404:/etc/metrics/jmx-exporter-config.yaml"
+ ports:
+ - name: jmx-metrics
+ containerPort: 9404
+ volumeMounts:
+ - name: jmx-exporter-config
+ mountPath: /etc/metrics
+ readOnly: true
+ volumes:
+ - name: jmx-exporter-config
+ configMap:
+ name: jmx-exporter-config
+ sparkConf:
+ spark.metrics.conf.*.sink.jmx.class: "org.apache.spark.metrics.sink.JmxSink"
Review Comment:
Two small things on the example config:
- `sparkConf` entries are forwarded as `-D` to both `SPARK_MASTER_OPTS` and `SPARK_WORKER_OPTS`, so the `*` instance also starts `JmxSink` for the master's `master` and `applications` metrics systems, where no agent reads the MBeans. `spark.metrics.conf.worker.sink.jmx.class` is enough.
- Rules only filter output. Without `includeObjectNames` the agent queries every MBean on every scrape and then discards almost all of it. Something like `includeObjectNames: ["metrics:type=gauges,*"]` (plus the `java.lang` entries if you keep the JVM rules) keeps the scrape cheap.
Also, `docs/spark_custom_resources.md` already documents a `JmxSink` recipe via a mounted `metrics.properties`. A cross-link from the new section would avoid having two unconnected recipes.
##########
spark-submission-worker/src/main/java/org/apache/spark/k8s/operator/SparkClusterResourceSpec.java:
##########
@@ -467,35 +469,53 @@ private static Optional<PodDisruptionBudget> buildPodDisruptionBudget(
* resource and does not carry the cluster label, yet the driver must reach the executors' block
* manager to fetch task results larger than {@code spark.task.maxDirectResultSize}.
*
+ * <p>If the worker network policy is configured with metrics ingress peers, a separate rule
+ * admits those peers on the configured port only. This should be a dedicated exporter port, not
+ * the worker web UI port; the operator does not verify this.
+ *
* @param clusterName The name of the SparkCluster.
* @param namespace The namespace of the SparkApplication.
+ * @param workerSpec The WorkerSpec, used to look up the optional metrics port and its peers.
* @return A NetworkPolicy object.
*/
- private NetworkPolicy buildWorkerNetworkPolicy(String clusterName, String namespace) {
- return new NetworkPolicyBuilder()
- .withNewMetadata()
- .withName(clusterName + "-worker")
- .withNamespace(namespace)
- .addToLabels(LABEL_SPARK_CLUSTER_NAME, clusterName)
- .endMetadata()
- .withNewSpec()
- .withNewPodSelector()
- .addToMatchLabels(LABEL_SPARK_ROLE_NAME, LABEL_SPARK_ROLE_WORKER_VALUE)
- .addToMatchLabels(LABEL_SPARK_CLUSTER_NAME, clusterName)
- .endPodSelector()
+ private NetworkPolicy buildWorkerNetworkPolicy(
+ String clusterName, String namespace, WorkerSpec workerSpec) {
+ var builder =
+ new NetworkPolicyBuilder()
+ .withNewMetadata()
+ .withName(clusterName + "-worker")
+ .withNamespace(namespace)
+ .addToLabels(LABEL_SPARK_CLUSTER_NAME, clusterName)
+ .endMetadata()
+ .withNewSpec()
+ .withNewPodSelector()
+ .addToMatchLabels(LABEL_SPARK_ROLE_NAME, LABEL_SPARK_ROLE_WORKER_VALUE)
+ .addToMatchLabels(LABEL_SPARK_CLUSTER_NAME, clusterName)
+ .endPodSelector()
+ .addNewIngress()
+ .addNewFrom()
+ .withNewPodSelector()
+ .addToMatchLabels(LABEL_SPARK_CLUSTER_NAME, clusterName)
+ .endPodSelector()
+ .endFrom()
+ .addNewFrom()
+ .withNewPodSelector()
+ .addToMatchLabels(LABEL_SPARK_ROLE_NAME, LABEL_SPARK_ROLE_DRIVER_VALUE)
+ .endPodSelector()
+ .endFrom()
+ .endIngress();
+ WorkerNetworkPolicySpec networkPolicy = workerSpec.getNetworkPolicy();
+ if (networkPolicy == null || networkPolicy.getMetricsIngress().isEmpty()) {
Review Comment:
Optional. I agree nulls cannot arrive through the API server: an old CRD or a `v1beta1` write prunes the whole block, and an explicit `null` is dropped and then fails `required`. So I am not asking for the null checks back. Two notes though:
- This guard is security-relevant, not cosmetic: an ingress rule with an empty or absent `from` admits **all** sources, and fabric8 omits an empty list when serializing. A one-line comment saying so would stop a future cleanup from removing it on the grounds that the schema guarantees a non-empty list.
- For Java callers, `new IntOrString((Integer) null)` is accepted by fabric8 and yields `ports: [{protocol: TCP}]`, i.e. every TCP port for the listed peers. That is fail-open, and nothing pins the `required` list today, so it would become reachable if `@Required` were ever dropped.
##########
spark-operator-api/src/main/java/org/apache/spark/k8s/operator/spec/WorkerNetworkPolicySpec.java:
##########
@@ -0,0 +1,62 @@
+/*
+ * 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.k8s.operator.spec;
+
+import java.util.List;
+
+import com.fasterxml.jackson.annotation.JsonInclude;
+import io.fabric8.generator.annotation.Max;
+import io.fabric8.generator.annotation.Min;
+import io.fabric8.generator.annotation.Required;
+import io.fabric8.kubernetes.api.model.networking.v1.NetworkPolicyPeer;
+import lombok.AllArgsConstructor;
+import lombok.Builder;
+import lombok.Data;
+import lombok.NoArgsConstructor;
+
+/**
+ * Network policy for the Spark workers.
+ *
+ * @since 1.1.0
+ */
+@Data
+@NoArgsConstructor
+@AllArgsConstructor
+@Builder
+@JsonInclude(JsonInclude.Include.NON_NULL)
+public class WorkerNetworkPolicySpec {
+ /**
+ * Port of a metrics endpoint (e.g. a JMX-to-Prometheus exporter agent) running in the worker
+ * container. This should be a dedicated exporter port, not the worker web UI port; the operator
+ * does not verify this. The caller is responsible for making the container listen on this port.
+ */
+ @Required
+ @Min(1)
+ @Max(65535)
+ protected Integer metricsPort;
+
+ /**
+ * Sources allowed to scrape {@link #metricsPort} on the worker pods, in addition to the sources
+ * the generated worker NetworkPolicy admits by default. When this list is empty, no metrics
+ * ingress rule is generated.
+ */
+ @Required
+ protected List<NetworkPolicyPeer> metricsIngress;
Review Comment:
`{metricsPort: 9404, metricsIngress: []}` still passes the schema and silently does nothing, which is the same silent no-op we removed for partial blocks. In the Helm chart an empty list means "deny" around a metrics rule that always exists; here the block's only purpose is to add a rule, so an empty list has no meaning. `@Size(min = 1)` (it is in the same `generator-annotations` jar) emits `minItems: 1`, so the block is either effective or rejected at apply time. Please keep the Java `isEmpty()` guard regardless, see the comment there.
##########
docs/operations.md:
##########
@@ -174,6 +174,55 @@ Note that this requires a CNI plugin that enforces NetworkPolicy; on clusters wi
a plugin the policy is silently ignored. Egress traffic of the operator (Kubernetes API
server, DNS) is not restricted by this policy.
+## Exposing SparkCluster Worker Metrics
+
+Every `SparkCluster` gets a generated worker `NetworkPolicy` that only admits ingress from pods
+carrying the cluster label or the driver-role label, so a Prometheus scraper is locked out by
+default. Opening the worker web UI port (`8081` by default) is not a safe fix: Spark's built-in
+`PrometheusServlet` metrics endpoint is served by the same embedded HTTP server as the web UI, so
+admitting that port to any source would expose the whole UI, not just metrics.
+
+Use the [Prometheus JMX Exporter](https://github.com/prometheus/jmx_exporter)
+(`jmx_prometheus_javaagent`) to serve metrics on a dedicated HTTP port. See
+[examples/cluster-with-jmx-exporter.yaml](../examples/cluster-with-jmx-exporter.yaml) for the full
+`ConfigMap` and `SparkCluster` configuration. Replace the example's image with a custom Spark image
+containing the exporter jar at `/opt/jmx_exporter/jmx_prometheus_javaagent.jar`; the stock
+`apache/spark` image does not include it.
+
+The example mounts the exporter rules and attaches the agent through `SPARK_DAEMON_JAVA_OPTS`.
+The operator sets `SPARK_WORKER_OPTS` itself, so a value there would be overwritten. It also sets
+`spark.metrics.conf.*.sink.jmx.class` to `org.apache.spark.metrics.sink.JmxSink` in `sparkConf` to
+register Spark metrics as MBeans. Without this sink, the exporter only exposes JVM metrics.
+
+Set `workerSpec.networkPolicy.metricsPort` to the exporter port and list the scraper's peers under
+`metricsIngress`. The generated policy adds ingress on that port for those peers, alongside the
+existing cluster/driver label allow-list. This mirrors
+`operatorDeployment.networkPolicy.metricsIngress`
+([above](#restricting-network-access-to-the-operator)) and accepts the same
+[`NetworkPolicyPeer`](https://kubernetes.io/docs/reference/kubernetes-api/policy-resources/network-policy-v1/#NetworkPolicyPeer)
+entries:
+
+```yaml
+spec:
+ workerSpec:
+ networkPolicy:
+ metricsPort: 9404
+ metricsIngress:
+ - namespaceSelector:
+ matchLabels:
+ kubernetes.io/metadata.name: "monitoring"
+```
+
+The `networkPolicy` block is optional. When present, the API server requires both fields and a
+port between 1 and 65535. An empty `metricsIngress` list adds no rule. Choose a dedicated exporter
Review Comment:
Please add one sentence that the policy is generated when the cluster is created. The `NetworkPolicy` is applied only in `ClusterInitStep` (state `Submitted`) and nothing is reconciled once the cluster is `RunningHealthy`, so a `kubectl edit` that adds or removes a peer is accepted by the API server and then never takes effect. That is existing behavior for every `SparkCluster` field, but scraper peers are the one knob people will edit on a long-running cluster. Changing them means recreating the `SparkCluster`.
--
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
Re: [PR] [SPARK-58691] Add configurable worker metrics NetworkPolicy ingress [spark-kubernetes-operator]
Posted by "yalindogusahin (via GitHub)" <gi...@apache.org>.
yalindogusahin commented on code in PR #794:
URL: https://github.com/apache/spark-kubernetes-operator/pull/794#discussion_r4015313521
##########
examples/cluster-with-jmx-exporter.yaml:
##########
@@ -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.
+apiVersion: v1
+kind: ConfigMap
+metadata:
+ name: jmx-exporter-config
+data:
+ jmx-exporter-config.yaml: |
+ rules:
+ - pattern: 'java.lang<type=Memory><HeapMemoryUsage>(\w+)'
+ name: jvm_memory_heap_$1
+ type: GAUGE
+ - pattern: 'java.lang<type=GarbageCollector, name=(\w+)><>CollectionCount'
+ name: jvm_gc_collection_count
+ labels:
+ gc: "$1"
+ type: COUNTER
+ - pattern: 'metrics<name=(\S+)><>Value'
Review Comment:
Added spark.metrics.conf.*.sink.jmx.class to the example and explained why it's needed in the docs.
--
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
Re: [PR] [SPARK-58691] Add configurable worker metrics NetworkPolicy ingress [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #794:
URL: https://github.com/apache/spark-kubernetes-operator/pull/794#discussion_r4055526804
##########
examples/cluster-with-jmx-exporter.yaml:
##########
@@ -0,0 +1,71 @@
+# 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.
+apiVersion: v1
+kind: ConfigMap
+metadata:
+ name: jmx-exporter-config
+data:
+ jmx-exporter-config.yaml: |
+ includeObjectNames: ["metrics:type=gauges,*"]
Review Comment:
Minor: the example asks the user to bake their own `jmx_prometheus_javaagent` jar but never names a version, and `includeObjectNames` is the newer key name. Older agent releases used `whitelistObjectNames` and reject unrecognized config keys outright. If someone picks an old jar, the agent fails to load this config, `-javaagent` initialization fails and the worker JVM aborts, so the pods CrashLoopBackOff while the `SparkCluster` still reports `RunningHealthy` (the operator sets that as soon as the applies succeed).
A minimum version next to the jar path comment would close this, e.g. "requires jmx_prometheus_javaagent 1.x; the `includeObjectNames` key is not accepted by older releases". I confirmed the key and the pattern work, but not which release introduced `includeObjectNames`, so please use whatever floor you are confident in.
##########
docs/operations.md:
##########
@@ -205,6 +205,58 @@ It is still honored: the NetworkPolicy is created when either key is `true`, so
`enable: true` in a base values file wins over `enabled: false` and must be removed to turn
the feature off. The same rule applies to `operatorConfiguration.dynamicConfig.enable`.
+## Exposing SparkCluster Worker Metrics
+
+Every `SparkCluster` gets a generated worker `NetworkPolicy` that only admits ingress from pods
+carrying the cluster label or the driver-role label, so a Prometheus scraper is locked out by
+default. Opening the worker web UI port (`8081` by default) is not a safe fix: Spark's built-in
+`PrometheusServlet` metrics endpoint is served by the same embedded HTTP server as the web UI, so
+admitting that port to any source would expose the whole UI, not just metrics.
+
+Use the [Prometheus JMX Exporter](https://github.com/prometheus/jmx_exporter)
+(`jmx_prometheus_javaagent`) to serve metrics on a dedicated HTTP port. See
+[examples/cluster-with-jmx-exporter.yaml](../examples/cluster-with-jmx-exporter.yaml) for the full
+`ConfigMap` and `SparkCluster` configuration. Replace the example's image with a custom Spark image
+containing the exporter jar at `/opt/jmx_exporter/jmx_prometheus_javaagent.jar`; the stock
+`apache/spark` image does not include it.
+
+The example mounts the exporter rules and attaches the agent through `SPARK_DAEMON_JAVA_OPTS`.
+The operator sets `SPARK_WORKER_OPTS` itself, so a value there would be overwritten. It also sets
Review Comment:
This describes the wrong failure mode. The operator does not replace a user-provided `SPARK_WORKER_OPTS`; it appends a second env entry with the same name (`addNewEnv()` in `SparkClusterResourceSpec.java:340`), and `ClusterInitStep` applies the StatefulSet with `serverSideApply()`, which rejects duplicate keys in the `env` associative list.
Checked against a live API server:
```
$ kubectl apply --server-side --dry-run=server -f dup-env.yaml
Error from server: failed to create typed patch object (default/dup-env-probe; apps/v1, Kind=StatefulSet):
.spec.template.spec.containers[name="worker"].env: duplicate entries for key [name="SPARK_WORKER_OPTS"]
```
So a reader who takes "would be overwritten" at face value and sets it anyway does not get a silently ignored value. The whole worker StatefulSet apply is rejected, the cluster goes to `SchedulingFailure` and then `Failed`, and the status message is a stack trace that never mentions the duplicate variable.
The advice to use `SPARK_DAEMON_JAVA_OPTS` is right, only the reason is wrong. Something like: "`SPARK_WORKER_OPTS` must not be set there: the operator appends its own, and server-side apply rejects duplicate env names."
--
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
Re: [PR] [SPARK-58691] Add configurable worker metrics NetworkPolicy ingress [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #794:
URL: https://github.com/apache/spark-kubernetes-operator/pull/794#discussion_r4055288831
##########
spark-operator-api/src/main/java/org/apache/spark/k8s/operator/spec/WorkerSpec.java:
##########
@@ -45,4 +45,10 @@ public class WorkerSpec {
protected ServiceSpec serviceSpec;
protected ObjectMeta serviceMetadata;
protected HorizontalPodAutoscalerSpec horizontalPodAutoscalerSpec;
+
+ /**
+ * Network policy controlling ingress to the worker pods. When null, the generated worker
+ * NetworkPolicy stays as restrictive as it is without these fields.
+ */
Review Comment:
```suggestion
```
--
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
Re: [PR] [SPARK-58691] Add configurable worker metrics NetworkPolicy ingress [spark-kubernetes-operator]
Posted by "yalindogusahin (via GitHub)" <gi...@apache.org>.
yalindogusahin commented on code in PR #794:
URL: https://github.com/apache/spark-kubernetes-operator/pull/794#discussion_r4015310002
##########
spark-operator-api/src/main/java/org/apache/spark/k8s/operator/spec/WorkerNetworkPolicySpec.java:
##########
@@ -0,0 +1,56 @@
+/*
+ * 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.k8s.operator.spec;
+
+import java.util.List;
+
+import com.fasterxml.jackson.annotation.JsonInclude;
+import io.fabric8.kubernetes.api.model.networking.v1.NetworkPolicyPeer;
+import lombok.AllArgsConstructor;
+import lombok.Builder;
+import lombok.Data;
+import lombok.NoArgsConstructor;
+
+/**
+ * Network policy for the Spark workers.
+ *
+ * @since 0.1.0
+ */
+@Data
+@NoArgsConstructor
+@AllArgsConstructor
+@Builder
+@JsonInclude(JsonInclude.Include.NON_NULL)
+public class WorkerNetworkPolicySpec {
+ /**
+ * Port of a metrics endpoint (e.g. a JMX-to-Prometheus exporter agent) running in the worker
+ * container, outside Spark's own web UI. The caller is responsible for making the container
+ * actually listen on this port. Takes effect only together with {@link #metricsIngress}.
+ */
+ protected Integer metricsPort;
Review Comment:
Added the annotations and updated the Helm CRD. Partial blocks are now rejected by the API server. Kept coverage for an explicitly empty peer list, which still adds no rule.
--
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
Re: [PR] [SPARK-58691] Add configurable worker metrics NetworkPolicy ingress [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #794:
URL: https://github.com/apache/spark-kubernetes-operator/pull/794#discussion_r4055288831
##########
spark-operator-api/src/main/java/org/apache/spark/k8s/operator/spec/WorkerSpec.java:
##########
@@ -45,4 +45,10 @@ public class WorkerSpec {
protected ServiceSpec serviceSpec;
protected ObjectMeta serviceMetadata;
protected HorizontalPodAutoscalerSpec horizontalPodAutoscalerSpec;
+
+ /**
+ * Network policy controlling ingress to the worker pods. When null, the generated worker
+ * NetworkPolicy stays as restrictive as it is without these fields.
+ */
Review Comment:
Shall we remove these lines like the other field?
```suggestion
```
--
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
Re: [PR] [SPARK-58691] Add configurable worker metrics NetworkPolicy ingress [spark-kubernetes-operator]
Posted by "yalindogusahin (via GitHub)" <gi...@apache.org>.
yalindogusahin commented on code in PR #794:
URL: https://github.com/apache/spark-kubernetes-operator/pull/794#discussion_r4015271654
##########
spark-operator-api/src/main/java/org/apache/spark/k8s/operator/spec/WorkerNetworkPolicySpec.java:
##########
@@ -0,0 +1,56 @@
+/*
+ * 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.k8s.operator.spec;
+
+import java.util.List;
+
+import com.fasterxml.jackson.annotation.JsonInclude;
+import io.fabric8.kubernetes.api.model.networking.v1.NetworkPolicyPeer;
+import lombok.AllArgsConstructor;
+import lombok.Builder;
+import lombok.Data;
+import lombok.NoArgsConstructor;
+
+/**
+ * Network policy for the Spark workers.
+ *
+ * @since 0.1.0
Review Comment:
Sorry about this one, that was my mistake I should have checked because the model hallucinated for the version.
--
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
Re: [PR] [SPARK-58691] Add configurable worker metrics NetworkPolicy ingress [spark-kubernetes-operator]
Posted by "yalindogusahin (via GitHub)" <gi...@apache.org>.
yalindogusahin commented on code in PR #794:
URL: https://github.com/apache/spark-kubernetes-operator/pull/794#discussion_r4015319467
##########
spark-submission-worker/src/test/java/org/apache/spark/k8s/operator/SparkClusterResourceSpecTest.java:
##########
@@ -337,6 +344,62 @@ void testWorkerNetworkPolicy() {
from.get(1).getPodSelector().getMatchLabels());
}
+ @Test
+ void testWorkerNetworkPolicyWithMetricsIngress() {
+ var scraper = new NetworkPolicyPeerBuilder()
+ .withNewNamespaceSelector()
+ .addToMatchLabels("kubernetes.io/metadata.name", "monitoring")
+ .endNamespaceSelector()
+ .build();
+ var networkPolicySpec = mock(WorkerNetworkPolicySpec.class);
Review Comment:
Switched the tests to WorkerNetworkPolicySpec.builder() and removed the redundant stubs.
--
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
Re: [PR] [SPARK-58691] Allow metrics scraping ingress to SparkCluster worker web port [spark-kubernetes-operator]
Posted by "yalindogusahin (via GitHub)" <gi...@apache.org>.
yalindogusahin commented on PR #794:
URL: https://github.com/apache/spark-kubernetes-operator/pull/794#issuecomment-5329219493
Thank you for the pointer. It makes sense to restrict the metrics port to explicitly selected peers rather than opening it up, so I have switched to that direction.
The generated worker `NetworkPolicy ` now carries a from: peer list supplied by the user under a new `workerSpec.metricsIngress` field, deliberately named and typed to match `operatorDeployment.networkPolicy.metricsIngress` in the chart, so the two features read the same way:
```yaml
spec:
workerSpec:
metricsPort: 9404
metricsIngress:
- namespaceSelector:
matchLabels:
kubernetes.io/metadata.name: "monitoring"
```
Both fields are required before any rule is generated: the port alone would admit every source, and the peers alone would grant them every worker port. When either is missing, worker ingress is byte-for-byte what it is on main.
One naming question: `metricsIngress` echoes the chart, but ingress may read as the Kubernetes Ingress resource rather than a NetworkPolicy peer list. I am happy to rename it if you consider that clearer.
--
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
Re: [PR] [SPARK-58691] Allow metrics scraping ingress to SparkCluster worker web port [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
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: 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
Re: [PR] [SPARK-58691] Add configurable worker metrics NetworkPolicy ingress [spark-kubernetes-operator]
Posted by "yalindogusahin (via GitHub)" <gi...@apache.org>.
yalindogusahin commented on PR #794:
URL: https://github.com/apache/spark-kubernetes-operator/pull/794#issuecomment-5679809597
Thanks for the detailed review. Pushed the fixes in 0f96579, including moving the policy assertion into the existing state-transition suite and updating the PR title and description. The full build, Javadoc, formatting, Helm lint, and schema checks pass. I haven't run the e2e suite locally because Chainsaw isn't installed.
--
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
Re: [PR] [SPARK-58691] Add configurable worker metrics NetworkPolicy ingress [spark-kubernetes-operator]
Posted by "yalindogusahin (via GitHub)" <gi...@apache.org>.
yalindogusahin commented on code in PR #794:
URL: https://github.com/apache/spark-kubernetes-operator/pull/794#discussion_r4015280307
##########
spark-submission-worker/src/main/java/org/apache/spark/k8s/operator/SparkClusterResourceSpec.java:
##########
@@ -467,35 +469,67 @@ private static Optional<PodDisruptionBudget> buildPodDisruptionBudget(
* resource and does not carry the cluster label, yet the driver must reach the executors' block
* manager to fetch task results larger than {@code spark.task.maxDirectResultSize}.
*
+ * <p>If both {@link WorkerNetworkPolicySpec#getMetricsPort()} and {@link
+ * WorkerNetworkPolicySpec#getMetricsIngress()} are set, a separate rule admits the listed peers
+ * on that port only. Both are required: the port alone would open the endpoint to every source in
+ * the cluster, and the peers alone would grant them every port. The worker web UI port is
+ * deliberately not usable as the metrics port: it is served by the same embedded HTTP server as
+ * the UI itself, so admitting it would also expose the full UI, not just a metrics endpoint. A
+ * dedicated metrics port (e.g. a JMX-to-Prometheus exporter agent) keeps metrics scraping
+ * separate from the UI.
+ *
* @param clusterName The name of the SparkCluster.
* @param namespace The namespace of the SparkApplication.
+ * @param workerSpec The WorkerSpec, used to look up the optional metrics port and its peers.
* @return A NetworkPolicy object.
+ * @throws IllegalArgumentException if a metrics ingress rule is requested on the worker web UI
+ * port.
*/
- private NetworkPolicy buildWorkerNetworkPolicy(String clusterName, String namespace) {
- return new NetworkPolicyBuilder()
- .withNewMetadata()
- .withName(clusterName + "-worker")
- .withNamespace(namespace)
- .addToLabels(LABEL_SPARK_CLUSTER_NAME, clusterName)
- .endMetadata()
- .withNewSpec()
- .withNewPodSelector()
- .addToMatchLabels(LABEL_SPARK_ROLE_NAME, LABEL_SPARK_ROLE_WORKER_VALUE)
- .addToMatchLabels(LABEL_SPARK_CLUSTER_NAME, clusterName)
- .endPodSelector()
- .addNewIngress()
- .addNewFrom()
- .withNewPodSelector()
- .addToMatchLabels(LABEL_SPARK_CLUSTER_NAME, clusterName)
- .endPodSelector()
- .endFrom()
- .addNewFrom()
- .withNewPodSelector()
- .addToMatchLabels(LABEL_SPARK_ROLE_NAME, LABEL_SPARK_ROLE_DRIVER_VALUE)
- .endPodSelector()
- .endFrom()
- .endIngress()
- .endSpec()
- .build();
+ private NetworkPolicy buildWorkerNetworkPolicy(
+ String clusterName, String namespace, WorkerSpec workerSpec) {
+ var builder =
+ new NetworkPolicyBuilder()
+ .withNewMetadata()
+ .withName(clusterName + "-worker")
+ .withNamespace(namespace)
+ .addToLabels(LABEL_SPARK_CLUSTER_NAME, clusterName)
+ .endMetadata()
+ .withNewSpec()
+ .withNewPodSelector()
+ .addToMatchLabels(LABEL_SPARK_ROLE_NAME, LABEL_SPARK_ROLE_WORKER_VALUE)
+ .addToMatchLabels(LABEL_SPARK_CLUSTER_NAME, clusterName)
+ .endPodSelector()
+ .addNewIngress()
+ .addNewFrom()
+ .withNewPodSelector()
+ .addToMatchLabels(LABEL_SPARK_CLUSTER_NAME, clusterName)
+ .endPodSelector()
+ .endFrom()
+ .addNewFrom()
+ .withNewPodSelector()
+ .addToMatchLabels(LABEL_SPARK_ROLE_NAME, LABEL_SPARK_ROLE_DRIVER_VALUE)
+ .endPodSelector()
+ .endFrom()
+ .endIngress();
+ WorkerNetworkPolicySpec networkPolicy = workerSpec.getNetworkPolicy();
+ if (networkPolicy != null
+ && networkPolicy.getMetricsPort() != null
+ && networkPolicy.getMetricsIngress() != null
+ && !networkPolicy.getMetricsIngress().isEmpty()) {
+ if (networkPolicy.getMetricsPort() == 8081) {
+ throw new IllegalArgumentException(
Review Comment:
Moved validation into the CRD schema. Both fields are now required when the block is present, and the port is constrained to 1–65535. Removed the constructor exception.
--
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
Re: [PR] [SPARK-58691] Add configurable worker metrics NetworkPolicy ingress [spark-kubernetes-operator]
Posted by "yalindogusahin (via GitHub)" <gi...@apache.org>.
yalindogusahin commented on code in PR #794:
URL: https://github.com/apache/spark-kubernetes-operator/pull/794#discussion_r4015306148
##########
spark-submission-worker/src/main/java/org/apache/spark/k8s/operator/SparkClusterResourceSpec.java:
##########
@@ -467,35 +469,67 @@ private static Optional<PodDisruptionBudget> buildPodDisruptionBudget(
* resource and does not carry the cluster label, yet the driver must reach the executors' block
* manager to fetch task results larger than {@code spark.task.maxDirectResultSize}.
*
+ * <p>If both {@link WorkerNetworkPolicySpec#getMetricsPort()} and {@link
+ * WorkerNetworkPolicySpec#getMetricsIngress()} are set, a separate rule admits the listed peers
+ * on that port only. Both are required: the port alone would open the endpoint to every source in
+ * the cluster, and the peers alone would grant them every port. The worker web UI port is
+ * deliberately not usable as the metrics port: it is served by the same embedded HTTP server as
+ * the UI itself, so admitting it would also expose the full UI, not just a metrics endpoint. A
+ * dedicated metrics port (e.g. a JMX-to-Prometheus exporter agent) keeps metrics scraping
+ * separate from the UI.
+ *
* @param clusterName The name of the SparkCluster.
* @param namespace The namespace of the SparkApplication.
+ * @param workerSpec The WorkerSpec, used to look up the optional metrics port and its peers.
* @return A NetworkPolicy object.
+ * @throws IllegalArgumentException if a metrics ingress rule is requested on the worker web UI
+ * port.
*/
- private NetworkPolicy buildWorkerNetworkPolicy(String clusterName, String namespace) {
- return new NetworkPolicyBuilder()
- .withNewMetadata()
- .withName(clusterName + "-worker")
- .withNamespace(namespace)
- .addToLabels(LABEL_SPARK_CLUSTER_NAME, clusterName)
- .endMetadata()
- .withNewSpec()
- .withNewPodSelector()
- .addToMatchLabels(LABEL_SPARK_ROLE_NAME, LABEL_SPARK_ROLE_WORKER_VALUE)
- .addToMatchLabels(LABEL_SPARK_CLUSTER_NAME, clusterName)
- .endPodSelector()
- .addNewIngress()
- .addNewFrom()
- .withNewPodSelector()
- .addToMatchLabels(LABEL_SPARK_CLUSTER_NAME, clusterName)
- .endPodSelector()
- .endFrom()
- .addNewFrom()
- .withNewPodSelector()
- .addToMatchLabels(LABEL_SPARK_ROLE_NAME, LABEL_SPARK_ROLE_DRIVER_VALUE)
- .endPodSelector()
- .endFrom()
- .endIngress()
- .endSpec()
- .build();
+ private NetworkPolicy buildWorkerNetworkPolicy(
+ String clusterName, String namespace, WorkerSpec workerSpec) {
+ var builder =
Review Comment:
Removed the reassignment and added an early return when the policy is absent or the peer list is empty.
--
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
Re: [PR] [SPARK-58691] Allow metrics scraping ingress to SparkCluster worker web port [spark-kubernetes-operator]
Posted by "yalindogusahin (via GitHub)" <gi...@apache.org>.
yalindogusahin commented on PR #794:
URL: https://github.com/apache/spark-kubernetes-operator/pull/794#issuecomment-5250159857
Thank you for reviewing and that is correct my bad at all I haven't checked the metric names.
However, if we decide to use jmx exporter to expose spark related metrics we should enable this network policy. For the statsd thats not the case I believe.
If we decide to go with jmx exporter we should bake jmx exporter to the image itself and + mapping config.
Looking forward to hear you opinion about this.
Kind regards,
--
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
Re: [PR] [SPARK-58691] Allow metrics scraping ingress to SparkCluster worker web port [spark-kubernetes-operator]
Posted by "yalindogusahin (via GitHub)" <gi...@apache.org>.
yalindogusahin commented on PR #794:
URL: https://github.com/apache/spark-kubernetes-operator/pull/794#issuecomment-5251133873
Pushed a redesign that addresses this directly: added an optional `workerSpec.metricsPort` field. When set, the generated `NetworkPolicy` opens ingress on *that specific port only* — not port 8081/the web UI. The idea is to pair this with a JMX-to-Prometheus exporter agent (`jmx_prometheus_javaagent`) attached to the worker JVM as a `-javaagent`, which serves metrics on its own dedicated port, fully decoupled from the web UI. Default behavior (no `metricsPort` set) is unchanged from before this PR — the web UI port is never touched. Added docs (`docs/operations.md`, new "Exposing SparkCluster Worker Metrics" section) and a runnable example (`examples/cluster-with-jmx-exporter.yaml`) showing the full setup. Let me know if this addresses your concern or if you'd prefer a different shape for the config.
--
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
Re: [PR] [SPARK-58691] Allow metrics scraping ingress to SparkCluster worker web port [spark-kubernetes-operator]
Posted by "yalindogusahin (via GitHub)" <gi...@apache.org>.
yalindogusahin commented on PR #794:
URL: https://github.com/apache/spark-kubernetes-operator/pull/794#issuecomment-5568010138
Thanks for the detailed review. I went through each point and pushed the fixes.
**Env-var collision**
The docs and example now use `SPARK_DAEMON_JAVA_OPTS` instead of `SPARK_WORKER_OPTS`. The operator appends its own (sometimes empty) `SPARK_WORKER_OPTS` to the worker container after merging the pod template, which silently overwrote the user value. `SPARK_DAEMON_JAVA_OPTS` is read by Spark`s `SparkClassCommandBuilder` for the Worker and the operator never sets it, so nothing collides.
**API shape**
Both fields now sit under `workerSpec.networkPolicy.metricsPort` and `workerSpec.networkPolicy.metricsIngress`, matching `operatorDeployment.networkPolicy.metricsIngress`. It also clears up the naming confusion; `networkPolicy` now reads as a policy block rather than two loose metrics fields.
`metricsPort: 8081` (the web UI port) is now rejected with an `IllegalArgumentException` and a unit test, since the UI and its metrics endpoint share the same embedded HTTP server.
**Docs / example**
Dropped the "complete, runnable" wording and put a note about the custom-image requirement next to the image field. Removed the unrelated `spark.ui.reverseProxy`.
I kept the `jmx_prometheus_javaagent` version in the Dockerfile `ADD` URL because that needs a concrete artifact. Happy to remove it if you would rather not pin it.
**Housekeeping**
Rebased onto the latest main, and added a `tests/e2e/worker-metrics-network-policy/` Chainsaw suite that checks the generated worker NetworkPolicy.
Local build is green: `:spark-operator-api:build`, `:spark-submission-worker:build`, `javadoc`, and `assertGeneratedCRDMatchesHelmChart` all pass, and the CRD is in sync with the Helm chart.
--
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
Re: [PR] [SPARK-58691] Add configurable worker metrics NetworkPolicy ingress [spark-kubernetes-operator]
Posted by "yalindogusahin (via GitHub)" <gi...@apache.org>.
yalindogusahin commented on code in PR #794:
URL: https://github.com/apache/spark-kubernetes-operator/pull/794#discussion_r4015317040
##########
docs/operations.md:
##########
@@ -174,6 +174,123 @@ Note that this requires a CNI plugin that enforces NetworkPolicy; on clusters wi
a plugin the policy is silently ignored. Egress traffic of the operator (Kubernetes API
server, DNS) is not restricted by this policy.
+## Exposing SparkCluster Worker Metrics
+
+Every `SparkCluster` gets a generated worker `NetworkPolicy` that only admits ingress from pods
+carrying the cluster label or the driver-role label, so a Prometheus scraper is locked out by
+default. Opening the worker web UI port (`8081`) is not a safe fix: Spark's built-in
+`PrometheusServlet` metrics endpoint is served by the same embedded HTTP server as the web UI, so
+admitting that port to any source would expose the whole UI, not just metrics.
+
+The recommended approach is to attach the community
+[Prometheus JMX Exporter](https://github.com/prometheus/jmx_exporter)
+(`jmx_prometheus_javaagent`) to the worker JVM as a `-javaagent`. The agent opens its own
+dedicated HTTP port that serves Prometheus-format metrics, completely decoupled from the web UI
+port. Setting this up takes four steps:
+
+1. Bake the exporter jar into the Spark image used by the cluster:
+
+ ```dockerfile
+ FROM apache/spark:4.2.0
+ ADD https://repo1.maven.org/maven2/io/prometheus/jmx/jmx_prometheus_javaagent/1.0.1/jmx_prometheus_javaagent-1.0.1.jar \
+ /opt/jmx_exporter/jmx_prometheus_javaagent.jar
+ ```
+
+2. Create a `ConfigMap` holding the exporter's mapping/rules config (which MBeans to expose and
+ how to name them), and mount it into the worker container via a `volumes` / `volumeMounts`
+ override under `workerSpec.statefulSetSpec.template.spec`:
+
+ ```yaml
+ apiVersion: v1
+ kind: ConfigMap
+ metadata:
+ name: jmx-exporter-config
+ data:
+ jmx-exporter-config.yaml: |
+ lowercaseOutputName: true
+ rules:
+ - pattern: ".*"
Review Comment:
Simplified the docs to show the network policy fragment and link to the full example. Also removed the catch-all exporter rule.
--
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
Re: [PR] [SPARK-58691] Allow metrics scraping ingress to SparkCluster worker web port [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #794:
URL: https://github.com/apache/spark-kubernetes-operator/pull/794#discussion_r4012465885
##########
spark-submission-worker/src/main/java/org/apache/spark/k8s/operator/SparkClusterResourceSpec.java:
##########
@@ -467,35 +469,67 @@ private static Optional<PodDisruptionBudget> buildPodDisruptionBudget(
* resource and does not carry the cluster label, yet the driver must reach the executors' block
* manager to fetch task results larger than {@code spark.task.maxDirectResultSize}.
*
+ * <p>If both {@link WorkerNetworkPolicySpec#getMetricsPort()} and {@link
+ * WorkerNetworkPolicySpec#getMetricsIngress()} are set, a separate rule admits the listed peers
+ * on that port only. Both are required: the port alone would open the endpoint to every source in
+ * the cluster, and the peers alone would grant them every port. The worker web UI port is
+ * deliberately not usable as the metrics port: it is served by the same embedded HTTP server as
+ * the UI itself, so admitting it would also expose the full UI, not just a metrics endpoint. A
+ * dedicated metrics port (e.g. a JMX-to-Prometheus exporter agent) keeps metrics scraping
+ * separate from the UI.
+ *
* @param clusterName The name of the SparkCluster.
* @param namespace The namespace of the SparkApplication.
+ * @param workerSpec The WorkerSpec, used to look up the optional metrics port and its peers.
* @return A NetworkPolicy object.
+ * @throws IllegalArgumentException if a metrics ingress rule is requested on the worker web UI
+ * port.
*/
- private NetworkPolicy buildWorkerNetworkPolicy(String clusterName, String namespace) {
- return new NetworkPolicyBuilder()
- .withNewMetadata()
- .withName(clusterName + "-worker")
- .withNamespace(namespace)
- .addToLabels(LABEL_SPARK_CLUSTER_NAME, clusterName)
- .endMetadata()
- .withNewSpec()
- .withNewPodSelector()
- .addToMatchLabels(LABEL_SPARK_ROLE_NAME, LABEL_SPARK_ROLE_WORKER_VALUE)
- .addToMatchLabels(LABEL_SPARK_CLUSTER_NAME, clusterName)
- .endPodSelector()
- .addNewIngress()
- .addNewFrom()
- .withNewPodSelector()
- .addToMatchLabels(LABEL_SPARK_CLUSTER_NAME, clusterName)
- .endPodSelector()
- .endFrom()
- .addNewFrom()
- .withNewPodSelector()
- .addToMatchLabels(LABEL_SPARK_ROLE_NAME, LABEL_SPARK_ROLE_DRIVER_VALUE)
- .endPodSelector()
- .endFrom()
- .endIngress()
- .endSpec()
- .build();
+ private NetworkPolicy buildWorkerNetworkPolicy(
+ String clusterName, String namespace, WorkerSpec workerSpec) {
+ var builder =
Review Comment:
Nit: the `builder = builder.addNewIngress()...endIngress();` reassignment below is a no-op. fabric8's `endIngress()` calls `setToIngress` on the enclosing fluent and returns that same instance, so a plain statement works. An early `return builder.endSpec().build();` when the block is absent would also put the rule-add at one nesting level.
##########
spark-submission-worker/src/main/java/org/apache/spark/k8s/operator/SparkClusterResourceSpec.java:
##########
@@ -467,35 +469,67 @@ private static Optional<PodDisruptionBudget> buildPodDisruptionBudget(
* resource and does not carry the cluster label, yet the driver must reach the executors' block
* manager to fetch task results larger than {@code spark.task.maxDirectResultSize}.
*
+ * <p>If both {@link WorkerNetworkPolicySpec#getMetricsPort()} and {@link
+ * WorkerNetworkPolicySpec#getMetricsIngress()} are set, a separate rule admits the listed peers
+ * on that port only. Both are required: the port alone would open the endpoint to every source in
+ * the cluster, and the peers alone would grant them every port. The worker web UI port is
+ * deliberately not usable as the metrics port: it is served by the same embedded HTTP server as
+ * the UI itself, so admitting it would also expose the full UI, not just a metrics endpoint. A
+ * dedicated metrics port (e.g. a JMX-to-Prometheus exporter agent) keeps metrics scraping
+ * separate from the UI.
+ *
* @param clusterName The name of the SparkCluster.
* @param namespace The namespace of the SparkApplication.
+ * @param workerSpec The WorkerSpec, used to look up the optional metrics port and its peers.
* @return A NetworkPolicy object.
+ * @throws IllegalArgumentException if a metrics ingress rule is requested on the worker web UI
+ * port.
*/
- private NetworkPolicy buildWorkerNetworkPolicy(String clusterName, String namespace) {
- return new NetworkPolicyBuilder()
- .withNewMetadata()
- .withName(clusterName + "-worker")
- .withNamespace(namespace)
- .addToLabels(LABEL_SPARK_CLUSTER_NAME, clusterName)
- .endMetadata()
- .withNewSpec()
- .withNewPodSelector()
- .addToMatchLabels(LABEL_SPARK_ROLE_NAME, LABEL_SPARK_ROLE_WORKER_VALUE)
- .addToMatchLabels(LABEL_SPARK_CLUSTER_NAME, clusterName)
- .endPodSelector()
- .addNewIngress()
- .addNewFrom()
- .withNewPodSelector()
- .addToMatchLabels(LABEL_SPARK_CLUSTER_NAME, clusterName)
- .endPodSelector()
- .endFrom()
- .addNewFrom()
- .withNewPodSelector()
- .addToMatchLabels(LABEL_SPARK_ROLE_NAME, LABEL_SPARK_ROLE_DRIVER_VALUE)
- .endPodSelector()
- .endFrom()
- .endIngress()
- .endSpec()
- .build();
+ private NetworkPolicy buildWorkerNetworkPolicy(
+ String clusterName, String namespace, WorkerSpec workerSpec) {
+ var builder =
+ new NetworkPolicyBuilder()
+ .withNewMetadata()
+ .withName(clusterName + "-worker")
+ .withNamespace(namespace)
+ .addToLabels(LABEL_SPARK_CLUSTER_NAME, clusterName)
+ .endMetadata()
+ .withNewSpec()
+ .withNewPodSelector()
+ .addToMatchLabels(LABEL_SPARK_ROLE_NAME, LABEL_SPARK_ROLE_WORKER_VALUE)
+ .addToMatchLabels(LABEL_SPARK_CLUSTER_NAME, clusterName)
+ .endPodSelector()
+ .addNewIngress()
+ .addNewFrom()
+ .withNewPodSelector()
+ .addToMatchLabels(LABEL_SPARK_CLUSTER_NAME, clusterName)
+ .endPodSelector()
+ .endFrom()
+ .addNewFrom()
+ .withNewPodSelector()
+ .addToMatchLabels(LABEL_SPARK_ROLE_NAME, LABEL_SPARK_ROLE_DRIVER_VALUE)
+ .endPodSelector()
+ .endFrom()
+ .endIngress();
+ WorkerNetworkPolicySpec networkPolicy = workerSpec.getNetworkPolicy();
+ if (networkPolicy != null
+ && networkPolicy.getMetricsPort() != null
+ && networkPolicy.getMetricsIngress() != null
+ && !networkPolicy.getMetricsIngress().isEmpty()) {
+ if (networkPolicy.getMetricsPort() == 8081) {
Review Comment:
This compares against a literal, not against the worker UI port. Line 77 above forwards every `sparkConf` entry as `-D` into `SPARK_WORKER_OPTS`, and Spark's `WorkerArguments` reads `spark.worker.ui.port` (and `SPARK_WORKER_WEBUI_PORT`), so `spark.worker.ui.port: "9404"` + `metricsPort: 9404` passes here and opens the whole UI to the peers. I would drop the throw and soften the docs/Javadoc instead. If a check is kept, derive it from `conf` and share one constant with the `8081` at lines 190/350.
##########
spark-submission-worker/src/main/java/org/apache/spark/k8s/operator/SparkClusterResourceSpec.java:
##########
@@ -467,35 +469,67 @@ private static Optional<PodDisruptionBudget> buildPodDisruptionBudget(
* resource and does not carry the cluster label, yet the driver must reach the executors' block
* manager to fetch task results larger than {@code spark.task.maxDirectResultSize}.
*
+ * <p>If both {@link WorkerNetworkPolicySpec#getMetricsPort()} and {@link
+ * WorkerNetworkPolicySpec#getMetricsIngress()} are set, a separate rule admits the listed peers
+ * on that port only. Both are required: the port alone would open the endpoint to every source in
+ * the cluster, and the peers alone would grant them every port. The worker web UI port is
+ * deliberately not usable as the metrics port: it is served by the same embedded HTTP server as
+ * the UI itself, so admitting it would also expose the full UI, not just a metrics endpoint. A
+ * dedicated metrics port (e.g. a JMX-to-Prometheus exporter agent) keeps metrics scraping
+ * separate from the UI.
+ *
* @param clusterName The name of the SparkCluster.
* @param namespace The namespace of the SparkApplication.
+ * @param workerSpec The WorkerSpec, used to look up the optional metrics port and its peers.
* @return A NetworkPolicy object.
+ * @throws IllegalArgumentException if a metrics ingress rule is requested on the worker web UI
+ * port.
*/
- private NetworkPolicy buildWorkerNetworkPolicy(String clusterName, String namespace) {
- return new NetworkPolicyBuilder()
- .withNewMetadata()
- .withName(clusterName + "-worker")
- .withNamespace(namespace)
- .addToLabels(LABEL_SPARK_CLUSTER_NAME, clusterName)
- .endMetadata()
- .withNewSpec()
- .withNewPodSelector()
- .addToMatchLabels(LABEL_SPARK_ROLE_NAME, LABEL_SPARK_ROLE_WORKER_VALUE)
- .addToMatchLabels(LABEL_SPARK_CLUSTER_NAME, clusterName)
- .endPodSelector()
- .addNewIngress()
- .addNewFrom()
- .withNewPodSelector()
- .addToMatchLabels(LABEL_SPARK_CLUSTER_NAME, clusterName)
- .endPodSelector()
- .endFrom()
- .addNewFrom()
- .withNewPodSelector()
- .addToMatchLabels(LABEL_SPARK_ROLE_NAME, LABEL_SPARK_ROLE_DRIVER_VALUE)
- .endPodSelector()
- .endFrom()
- .endIngress()
- .endSpec()
- .build();
+ private NetworkPolicy buildWorkerNetworkPolicy(
+ String clusterName, String namespace, WorkerSpec workerSpec) {
+ var builder =
+ new NetworkPolicyBuilder()
+ .withNewMetadata()
+ .withName(clusterName + "-worker")
+ .withNamespace(namespace)
+ .addToLabels(LABEL_SPARK_CLUSTER_NAME, clusterName)
+ .endMetadata()
+ .withNewSpec()
+ .withNewPodSelector()
+ .addToMatchLabels(LABEL_SPARK_ROLE_NAME, LABEL_SPARK_ROLE_WORKER_VALUE)
+ .addToMatchLabels(LABEL_SPARK_CLUSTER_NAME, clusterName)
+ .endPodSelector()
+ .addNewIngress()
+ .addNewFrom()
+ .withNewPodSelector()
+ .addToMatchLabels(LABEL_SPARK_CLUSTER_NAME, clusterName)
+ .endPodSelector()
+ .endFrom()
+ .addNewFrom()
+ .withNewPodSelector()
+ .addToMatchLabels(LABEL_SPARK_ROLE_NAME, LABEL_SPARK_ROLE_DRIVER_VALUE)
+ .endPodSelector()
+ .endFrom()
+ .endIngress();
+ WorkerNetworkPolicySpec networkPolicy = workerSpec.getNetworkPolicy();
+ if (networkPolicy != null
+ && networkPolicy.getMetricsPort() != null
+ && networkPolicy.getMetricsIngress() != null
+ && !networkPolicy.getMetricsIngress().isEmpty()) {
+ if (networkPolicy.getMetricsPort() == 8081) {
+ throw new IllegalArgumentException(
Review Comment:
Throwing from the resource-spec constructor surfaces as a generic `SchedulingFailure` (stack trace in the status message) in `ClusterInitStep`, and from the next reconcile on as `Failed` / `Cannot process cluster status` via `ClusterUnknownStateStep`, which immediate-requeues forever and appends a `Failed` entry to `stateTransitionHistory` each time. Spec validation belongs in `ClusterValidateStep` (currently a no-op placeholder; `AppValidateStep.validateGatewayRoutes` shows the pattern) or in the CRD schema, so this constructor keeps no exception path.
##########
docs/operations.md:
##########
@@ -174,6 +174,123 @@ Note that this requires a CNI plugin that enforces NetworkPolicy; on clusters wi
a plugin the policy is silently ignored. Egress traffic of the operator (Kubernetes API
server, DNS) is not restricted by this policy.
+## Exposing SparkCluster Worker Metrics
+
+Every `SparkCluster` gets a generated worker `NetworkPolicy` that only admits ingress from pods
+carrying the cluster label or the driver-role label, so a Prometheus scraper is locked out by
+default. Opening the worker web UI port (`8081`) is not a safe fix: Spark's built-in
+`PrometheusServlet` metrics endpoint is served by the same embedded HTTP server as the web UI, so
+admitting that port to any source would expose the whole UI, not just metrics.
+
+The recommended approach is to attach the community
+[Prometheus JMX Exporter](https://github.com/prometheus/jmx_exporter)
+(`jmx_prometheus_javaagent`) to the worker JVM as a `-javaagent`. The agent opens its own
+dedicated HTTP port that serves Prometheus-format metrics, completely decoupled from the web UI
+port. Setting this up takes four steps:
+
+1. Bake the exporter jar into the Spark image used by the cluster:
+
+ ```dockerfile
+ FROM apache/spark:4.2.0
+ ADD https://repo1.maven.org/maven2/io/prometheus/jmx/jmx_prometheus_javaagent/1.0.1/jmx_prometheus_javaagent-1.0.1.jar \
+ /opt/jmx_exporter/jmx_prometheus_javaagent.jar
+ ```
+
+2. Create a `ConfigMap` holding the exporter's mapping/rules config (which MBeans to expose and
+ how to name them), and mount it into the worker container via a `volumes` / `volumeMounts`
+ override under `workerSpec.statefulSetSpec.template.spec`:
+
+ ```yaml
+ apiVersion: v1
+ kind: ConfigMap
+ metadata:
+ name: jmx-exporter-config
+ data:
+ jmx-exporter-config.yaml: |
+ lowercaseOutputName: true
+ rules:
+ - pattern: ".*"
Review Comment:
This catch-all config differs from the targeted rules in `examples/cluster-with-jmx-exporter.yaml`, and it exports every MBean attribute on each scrape. I would keep the full CR in the example only and show just the `networkPolicy` fragment here.
##########
spark-operator-api/src/main/java/org/apache/spark/k8s/operator/spec/WorkerNetworkPolicySpec.java:
##########
@@ -0,0 +1,56 @@
+/*
+ * 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.k8s.operator.spec;
+
+import java.util.List;
+
+import com.fasterxml.jackson.annotation.JsonInclude;
+import io.fabric8.kubernetes.api.model.networking.v1.NetworkPolicyPeer;
+import lombok.AllArgsConstructor;
+import lombok.Builder;
+import lombok.Data;
+import lombok.NoArgsConstructor;
+
+/**
+ * Network policy for the Spark workers.
+ *
+ * @since 0.1.0
+ */
+@Data
+@NoArgsConstructor
+@AllArgsConstructor
+@Builder
+@JsonInclude(JsonInclude.Include.NON_NULL)
+public class WorkerNetworkPolicySpec {
+ /**
+ * Port of a metrics endpoint (e.g. a JMX-to-Prometheus exporter agent) running in the worker
+ * container, outside Spark's own web UI. The caller is responsible for making the container
+ * actually listen on this port. Takes effect only together with {@link #metricsIngress}.
+ */
+ protected Integer metricsPort;
Review Comment:
Setting only one of `metricsPort` / `metricsIngress` is silently ignored: no rule, no status, no event. Since this block is optional as a whole, `@Required` on both fields (already used on `RuntimeVersions.sparkVersion`; nested `required:` lists exist under the optional `workerSpec.instanceConfig`) plus `@Min(1) @Max(65535)` on `metricsPort` lets the API server reject a partial or out-of-range block at apply time while leaving an omitted block valid. The null-guard, the "both required" docs paragraph, and `testWorkerNetworkPolicyWith*ButNo*` can then go.
##########
examples/cluster-with-jmx-exporter.yaml:
##########
@@ -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.
+apiVersion: v1
+kind: ConfigMap
+metadata:
+ name: jmx-exporter-config
+data:
+ jmx-exporter-config.yaml: |
+ rules:
+ - pattern: 'java.lang<type=Memory><HeapMemoryUsage>(\w+)'
+ name: jvm_memory_heap_$1
+ type: GAUGE
+ - pattern: 'java.lang<type=GarbageCollector, name=(\w+)><>CollectionCount'
+ name: jvm_gc_collection_count
+ labels:
+ gc: "$1"
+ type: COUNTER
+ - pattern: 'metrics<name=(\S+)><>Value'
Review Comment:
This rule matches nothing as configured. Spark's `MetricsConfig` registers only the servlet sink by default, so the `metrics` JMX domain stays empty unless `spark.metrics.conf.*.sink.jmx.class: org.apache.spark.metrics.sink.JmxSink` is added to `sparkConf` (it is forwarded to the worker as `-D`). Without it the exporter serves `jvm_*` only and no `spark_worker_*` series.
##########
spark-operator-api/src/main/java/org/apache/spark/k8s/operator/spec/WorkerNetworkPolicySpec.java:
##########
@@ -0,0 +1,56 @@
+/*
+ * 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.k8s.operator.spec;
+
+import java.util.List;
+
+import com.fasterxml.jackson.annotation.JsonInclude;
+import io.fabric8.kubernetes.api.model.networking.v1.NetworkPolicyPeer;
+import lombok.AllArgsConstructor;
+import lombok.Builder;
+import lombok.Data;
+import lombok.NoArgsConstructor;
+
+/**
+ * Network policy for the Spark workers.
+ *
+ * @since 0.1.0
Review Comment:
This class is new; `@since 1.1.0`.
##########
spark-submission-worker/src/test/java/org/apache/spark/k8s/operator/SparkClusterResourceSpecTest.java:
##########
@@ -337,6 +344,62 @@ void testWorkerNetworkPolicy() {
from.get(1).getPodSelector().getMatchLabels());
}
+ @Test
+ void testWorkerNetworkPolicyWithMetricsIngress() {
+ var scraper = new NetworkPolicyPeerBuilder()
+ .withNewNamespaceSelector()
+ .addToMatchLabels("kubernetes.io/metadata.name", "monitoring")
+ .endNamespaceSelector()
+ .build();
+ var networkPolicySpec = mock(WorkerNetworkPolicySpec.class);
Review Comment:
`WorkerNetworkPolicySpec` is a Lombok `@Builder` POJO; `WorkerNetworkPolicySpec.builder().metricsPort(9404).metricsIngress(List.of(scraper)).build()` removes the getter stubs in all four tests, the `Mockito defaults Integer to 0` comment, and the redundant `thenReturn(null)` added to `setUp` (that is already Mockito's default). `KueueWorkloadFactoryTest` uses `WorkerSpec.builder()` the same way.
--
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