You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "csviri (via GitHub)" <gi...@apache.org> on 2026/06/15 08:23:39 UTC
[PR] Replace dynamic-config informer with mounted-ConfigMap file watcher [spark-kubernetes-operator]
csviri opened a new pull request, #707:
URL: https://github.com/apache/spark-kubernetes-operator/pull/707
### What changes were proposed in this pull request?
Reworks the dynamic-config feature so the operator no longer runs a dedicated `Operator` instance with a ConfigMap informer. Instead, the dynamic-config ConfigMap is mounted into the operator pod as a volume, and a new `DynamicConfigMonitor`
periodically re-reads the properties file from disk. When the file content changes, `SparkOperatorConfManager` is refreshed and the watched-namespaces updater is invoked, exactly as the previous `SparkOperatorConfigMapReconciler` did.
Key changes:
- **New `DynamicConfigMonitor`** (`config/DynamicConfigMonitor.java`) — single-thread scheduled reader of a properties file, with `start()` / `stop()` / `isRunning()` lifecycle and change detection against the previously loaded snapshot.
- **`SparkOperator`** simplified from a `List<Operator>` to a single `Operator` plus an optional `DynamicConfigMonitor`. The second JOSDK `Operator` instance and `overrideConfigMonitorConfigs(...)` are gone.
- **Removed** `SparkOperatorConfigMapReconciler` and its tests.
- **Config**: `spark.kubernetes.operator.dynamicConfig.selector` is replaced by:
- `spark.kubernetes.operator.dynamicConfig.filePath` (default `/opt/spark-operator/dynamic-conf/spark-operator-dynamic.properties`)
- `spark.kubernetes.operator.dynamicConfig.reloadIntervalSeconds` (default `60`)
- **Helm chart**:
- Drops the `Role` / `RoleBinding` granting `configmaps: '*'` to the operator service account (no longer needed — kubelet projects the ConfigMap onto the pod).
- Mounts the `spark-kubernetes-operator-dynamic-configuration` ConfigMap under `/opt/spark-operator/dynamic-conf` when `operatorConfiguration.dynamicConfig.enable` is true.
- The dynamic ConfigMap now stores entries under the single key `spark-operator-dynamic.properties` (a properties-formatted blob) instead of one ConfigMap key per property.
- **Probes** (`HealthProbe`, `ReadinessProbe`, `ProbeService`, `ProbeUtil`): switched from `List<Operator>` to a single `Operator`, and the dynamic-config monitor's running state is now part of both `/healthz` and `/readyz`.
`areOperatorsStarted` collapses to `isOperatorStarted`.
### Why are the changes needed?
The informer-based approach required the operator to hold cluster/namespace RBAC on ConfigMaps and ran a second JOSDK `Operator` purely to watch a single ConfigMap. Mounting the ConfigMap directly is simpler, removes the RBAC requirement,
and avoids the second informer's failure modes. The kubelet already propagates ConfigMap edits to mounted volumes, so polling the file gives equivalent behavior with much less moving machinery.
### Does this PR introduce any user-facing change?
Yes. For users with `dynamicConfig.enable: true`:
- The dynamic ConfigMap is consumed via a volume mount, not via the API server.
- `dynamicConfig.selector` no longer has any effect; use `dynamicConfig.filePath` / `dynamicConfig.reloadIntervalSeconds` instead.
- The dynamic ConfigMap data layout changes — the Helm template now renders a single `spark-operator-dynamic.properties` key. Hand-managed dynamic ConfigMaps must be migrated to the same shape.
- The `configmaps: '*'` `Role` / `RoleBinding` previously created by the chart is removed.
### How was this patch tested?
- New unit tests in `DynamicConfigMonitorTest` covering initial load, no-op reload when file is unchanged, refresh + namespace-update on change, and `isRunning()` lifecycle.
- Updated `SparkOperatorTest`, `HealthProbeTest`, `ReadinessProbeTest`, `ProbeServiceTest` for the single-operator + monitor signatures.
- Removed `SparkOperatorConfigMapReconcilerTest`.
--
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-57466] Add option for dynamic-config to use mounted-ConfigMap [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #707:
URL: https://github.com/apache/spark-kubernetes-operator/pull/707#issuecomment-4744320058
No, you were clear.
> maybe I was not clear, those are needed/used in the chainsaw test, and needs to be in this format. Or do I miss something?
Maybe, I wasn't clear enough. My perspective was the following.
- This is supposed to introduce a new alternative path which we should not affect the existing functionality and **test coverage**.
- In other words, this PR is supposed to introduce an independent set of test coverage instead of touching the existing test code and test resource.
So, if you need to change some existing test resource for any reasons like the following, it should be done before this PR independently.
> A key is always a new file.
--
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-57466] Replace dynamic-config controller with mounted-ConfigMap file watcher [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #707:
URL: https://github.com/apache/spark-kubernetes-operator/pull/707#issuecomment-4710072142
Thank you, @csviri .
For the following question, it's just time-out due to the limit of `ubuntu-slim` GitHub Actions Virtual Environment. Please re-trigger the failed pipeline. We use `ubuntu-slim` because of the cost-savings.
> this is a strange failure, tests are passing: https://github.com/apache/spark-kubernetes-operator/actions/runs/26576971235
<img width="485" height="115" alt="Screenshot 2026-06-15 at 09 28 17" src="https://github.com/user-attachments/assets/cd54fef2-1290-44b4-b6bb-6b186de8e129" />
--
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-57466] Add option for dynamic-config to use mounted-ConfigMap [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #707:
URL: https://github.com/apache/spark-kubernetes-operator/pull/707#discussion_r3424366692
##########
spark-operator/src/test/java/org/apache/spark/k8s/operator/probe/HealthProbeTest.java:
##########
@@ -86,48 +78,12 @@ public boolean isStarted() {
}
};
- RuntimeInfo runtimeInfo2 =
- new RuntimeInfo(
- new Operator(overrider -> overrider.withKubernetesClient(kubernetesClient))) {
- @Override
- public boolean isStarted() {
- return isRunning2.get();
- }
-
- @Override
- public Map<String, Map<String, InformerWrappingEventSourceHealthIndicator>>
- unhealthyInformerWrappingEventSourceHealthIndicator() {
- return unhealthyEventSources2;
- }
- };
-
when(operator.getRuntimeInfo()).thenReturn(runtimeInfo);
- when(sparkConfMonitor.getRuntimeInfo()).thenReturn(runtimeInfo2);
- }
-
- @Test
- void testHealthProbeWithInformerHealthWithMultiOperators() {
Review Comment:
The new `testHealthProbeWithDynamicConfigMonitor` test coverage contains 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-57466] Replace dynamic-config controller with mounted-ConfigMap file watcher [spark-kubernetes-operator]
Posted by "csviri (via GitHub)" <gi...@apache.org>.
csviri commented on PR #707:
URL: https://github.com/apache/spark-kubernetes-operator/pull/707#issuecomment-4708988345
@dongjoon-hyun this is a strange failure, tests are passing:
https://github.com/apache/spark-kubernetes-operator/actions/runs/26576971235
--
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-57466] Add option for dynamic-config to use mounted-ConfigMap [spark-kubernetes-operator]
Posted by "csviri (via GitHub)" <gi...@apache.org>.
csviri commented on code in PR #707:
URL: https://github.com/apache/spark-kubernetes-operator/pull/707#discussion_r3426682227
##########
tests/e2e/watched-namespaces/spark-operator-dynamic-config-2.yaml:
##########
@@ -17,7 +17,8 @@
apiVersion: v1
data:
- spark.kubernetes.operator.watchedNamespaces: default
+ spark-operator-dynamic.properties: |
+ spark.kubernetes.operator.watchedNamespaces=default
Review Comment:
Same as above: yes, afaik there is no way to mount all ConfigMap data in K8S. A key is always a new file.
--
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-57466] Support `s.k.o.dynamicConfig.source` to use mounted-`ConfigMap` [spark-kubernetes-operator]
Posted by "csviri (via GitHub)" <gi...@apache.org>.
csviri commented on code in PR #707:
URL: https://github.com/apache/spark-kubernetes-operator/pull/707#discussion_r3465808251
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/probe/HealthProbe.java:
##########
@@ -60,7 +62,6 @@ public boolean isHealthy() {
if (!areOperatorsStarted(operators).orElse(false)) {
return false;
}
-
Review Comment:
fixed, thx!
--
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-57466] Add option for dynamic-config to use mounted-ConfigMap [spark-kubernetes-operator]
Posted by "csviri (via GitHub)" <gi...@apache.org>.
csviri commented on PR #707:
URL: https://github.com/apache/spark-kubernetes-operator/pull/707#issuecomment-4719352603
@dongjoon-hyun updated
--
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-57466] Add option for dynamic-config to use mounted-ConfigMap [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #707:
URL: https://github.com/apache/spark-kubernetes-operator/pull/707#discussion_r3424363181
##########
spark-operator/src/test/java/org/apache/spark/k8s/operator/probe/HealthProbeTest.java:
##########
@@ -86,48 +78,12 @@ public boolean isStarted() {
}
};
- RuntimeInfo runtimeInfo2 =
- new RuntimeInfo(
- new Operator(overrider -> overrider.withKubernetesClient(kubernetesClient))) {
- @Override
- public boolean isStarted() {
- return isRunning2.get();
- }
-
- @Override
- public Map<String, Map<String, InformerWrappingEventSourceHealthIndicator>>
- unhealthyInformerWrappingEventSourceHealthIndicator() {
- return unhealthyEventSources2;
- }
- };
-
when(operator.getRuntimeInfo()).thenReturn(runtimeInfo);
- when(sparkConfMonitor.getRuntimeInfo()).thenReturn(runtimeInfo2);
- }
-
- @Test
- void testHealthProbeWithInformerHealthWithMultiOperators() {
Review Comment:
May I ask if this is inevitable removal of irrelevant `HealthProbe` test coverage, @csviri ?
--
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-57466] Support `s.k.o.dynamicConfig.source` to use mounted-`ConfigMap` [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #707:
URL: https://github.com/apache/spark-kubernetes-operator/pull/707#issuecomment-4847797940
Merged to main
--
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-57466] Add option for dynamic-config to use mounted-ConfigMap [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #707:
URL: https://github.com/apache/spark-kubernetes-operator/pull/707#issuecomment-4733676904
Hi, @csviri . According to your comment, the following sounds irrelevant to this PR's proposal. Could you make a new PR only for that? We can proceed that independent topic before this PR.
> yes, afaik there is not way to mount all ConfigMap data in K8S. A key is always a new file.
```yaml
data:
- spark.kubernetes.operator.watchedNamespaces: default
+ spark-operator-dynamic.properties: |
+ spark.kubernetes.operator.watchedNamespaces=default
```
--
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-57466] Add option for dynamic-config to use mounted-ConfigMap [spark-kubernetes-operator]
Posted by "csviri (via GitHub)" <gi...@apache.org>.
csviri commented on PR #707:
URL: https://github.com/apache/spark-kubernetes-operator/pull/707#issuecomment-4750353498
> No, you were clear.
>
> > maybe I was not clear, those are needed/used in the chainsaw test, and needs to be in this format. Or do I miss something?
>
> Maybe, I wasn't clear enough. My perspective was the following.
>
> * This is supposed to introduce a new alternative path which we should not affect the existing functionality and **test coverage**.
> * In other words, this PR is supposed to introduce an independent set of test coverage instead of touching the existing test code and test resource.
>
> So, if you need to change some existing test resource for any reasons like the following, it should be done before this PR independently.
>
> > A key is always a new file.
Ahh I see, change the PR! thx!
--
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-57466] Support `s.k.o.dynamicConfig.source` to use mounted-`ConfigMap` [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #707:
URL: https://github.com/apache/spark-kubernetes-operator/pull/707#discussion_r3453424912
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/config/DynamicConfigMonitor.java:
##########
@@ -0,0 +1,160 @@
+/*
+ * 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.config;
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.UncheckedIOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.time.Duration;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Properties;
+import java.util.Set;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicReference;
+import java.util.function.Consumer;
+import java.util.function.Supplier;
+
+import lombok.extern.slf4j.Slf4j;
+
+/**
+ * Periodically reloads dynamic configuration overrides from a properties file on disk. The file is
+ * expected to be populated by mounting a ConfigMap as a volume into the operator pod, so changes
+ * applied to the ConfigMap propagate to disk without requiring a Kubernetes informer. When the
+ * file contents change, {@link SparkOperatorConfManager} is refreshed and the caller-supplied
+ * namespace updater is invoked.
+ */
+@Slf4j
+public class DynamicConfigMonitor {
+
+ private final Path configFile;
+ private final Duration reloadInterval;
+ private final Supplier<Set<String>> watchedNamespacesSupplier;
+ private final Consumer<Set<String>> namespaceUpdater;
+ private final ScheduledExecutorService scheduler;
+ private final boolean ownsScheduler;
+
+ private final AtomicReference<Map<String, String>> lastLoaded = new AtomicReference<>(Map.of());
+ private final AtomicBoolean initialLoadComplete = new AtomicBoolean();
+
+ public DynamicConfigMonitor(
+ Path configFile,
+ Duration reloadInterval,
+ Supplier<Set<String>> watchedNamespacesSupplier,
+ Consumer<Set<String>> namespaceUpdater) {
+ this(configFile, reloadInterval, watchedNamespacesSupplier, namespaceUpdater, null);
+ }
+
+ DynamicConfigMonitor(
+ Path configFile,
+ Duration reloadInterval,
+ Supplier<Set<String>> watchedNamespacesSupplier,
+ Consumer<Set<String>> namespaceUpdater,
+ ScheduledExecutorService scheduler) {
+ this.configFile = configFile;
+ this.reloadInterval = reloadInterval;
+ this.watchedNamespacesSupplier = watchedNamespacesSupplier;
+ this.namespaceUpdater = namespaceUpdater;
+ if (scheduler == null) {
+ this.scheduler =
+ Executors.newSingleThreadScheduledExecutor(
+ r -> {
+ Thread t = new Thread(r, "spark-operator-dynamic-config");
+ t.setDaemon(true);
+ return t;
+ });
+ this.ownsScheduler = true;
+ } else {
+ this.scheduler = scheduler;
+ this.ownsScheduler = false;
+ }
+ }
+
+ /**
+ * Performs an initial synchronous load and schedules periodic reloads at the configured
+ * interval.
+ */
+ public void start() {
+ log.info(
+ "Starting dynamic config monitor on {} with reload interval {}",
+ configFile,
+ reloadInterval);
+ reload();
+ initialLoadComplete.set(true);
+ long millis = reloadInterval.toMillis();
+ scheduler.scheduleAtFixedRate(this::reloadSafely, millis, millis, TimeUnit.MILLISECONDS);
+ }
+
+ /** Stops the scheduler if it was created internally. */
+ public void stop() {
+ log.info("Stopping dynamic config monitor");
+ if (ownsScheduler) {
+ scheduler.shutdownNow();
+ }
+ }
+
+ /**
+ * Returns true once the initial load has completed and the underlying scheduler is still
+ * running.
+ */
+ public boolean isRunning() {
+ return initialLoadComplete.get() && !scheduler.isShutdown();
+ }
+
+ private void reloadSafely() {
+ try {
+ reload();
+ } catch (UncheckedIOException e) {
+ log.error("Failed to reload dynamic config from {}", configFile, e);
+ }
+ }
+
+ private void reload() {
+ Map<String, String> current = readProperties();
+ if (current.equals(lastLoaded.get())) {
+ return;
+ }
+ log.info(
+ "Detected dynamic config change in {}, applying {} overrides", configFile, current.size());
+ SparkOperatorConfManager.INSTANCE.refresh(current);
+ lastLoaded.set(current);
+ namespaceUpdater.accept(watchedNamespacesSupplier.get());
Review Comment:
This seems to break `reload`'s assumption. This means we will not retry when `namespaceUpdater.accept(watchedNamespacesSupplier.get())` fails, doesn't it?
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/config/DynamicConfigMonitor.java:
##########
@@ -0,0 +1,160 @@
+/*
+ * 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.config;
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.UncheckedIOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.time.Duration;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Properties;
+import java.util.Set;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicReference;
+import java.util.function.Consumer;
+import java.util.function.Supplier;
+
+import lombok.extern.slf4j.Slf4j;
+
+/**
+ * Periodically reloads dynamic configuration overrides from a properties file on disk. The file is
+ * expected to be populated by mounting a ConfigMap as a volume into the operator pod, so changes
+ * applied to the ConfigMap propagate to disk without requiring a Kubernetes informer. When the
+ * file contents change, {@link SparkOperatorConfManager} is refreshed and the caller-supplied
+ * namespace updater is invoked.
+ */
+@Slf4j
+public class DynamicConfigMonitor {
+
+ private final Path configFile;
+ private final Duration reloadInterval;
+ private final Supplier<Set<String>> watchedNamespacesSupplier;
+ private final Consumer<Set<String>> namespaceUpdater;
+ private final ScheduledExecutorService scheduler;
+ private final boolean ownsScheduler;
+
+ private final AtomicReference<Map<String, String>> lastLoaded = new AtomicReference<>(Map.of());
+ private final AtomicBoolean initialLoadComplete = new AtomicBoolean();
+
+ public DynamicConfigMonitor(
+ Path configFile,
+ Duration reloadInterval,
+ Supplier<Set<String>> watchedNamespacesSupplier,
+ Consumer<Set<String>> namespaceUpdater) {
+ this(configFile, reloadInterval, watchedNamespacesSupplier, namespaceUpdater, null);
+ }
+
+ DynamicConfigMonitor(
+ Path configFile,
+ Duration reloadInterval,
+ Supplier<Set<String>> watchedNamespacesSupplier,
+ Consumer<Set<String>> namespaceUpdater,
+ ScheduledExecutorService scheduler) {
+ this.configFile = configFile;
+ this.reloadInterval = reloadInterval;
+ this.watchedNamespacesSupplier = watchedNamespacesSupplier;
+ this.namespaceUpdater = namespaceUpdater;
+ if (scheduler == null) {
+ this.scheduler =
+ Executors.newSingleThreadScheduledExecutor(
+ r -> {
+ Thread t = new Thread(r, "spark-operator-dynamic-config");
+ t.setDaemon(true);
+ return t;
+ });
+ this.ownsScheduler = true;
+ } else {
+ this.scheduler = scheduler;
+ this.ownsScheduler = false;
+ }
+ }
+
+ /**
+ * Performs an initial synchronous load and schedules periodic reloads at the configured
+ * interval.
+ */
+ public void start() {
+ log.info(
+ "Starting dynamic config monitor on {} with reload interval {}",
+ configFile,
+ reloadInterval);
+ reload();
+ initialLoadComplete.set(true);
+ long millis = reloadInterval.toMillis();
+ scheduler.scheduleAtFixedRate(this::reloadSafely, millis, millis, TimeUnit.MILLISECONDS);
+ }
+
+ /** Stops the scheduler if it was created internally. */
+ public void stop() {
+ log.info("Stopping dynamic config monitor");
+ if (ownsScheduler) {
+ scheduler.shutdownNow();
+ }
+ }
+
+ /**
+ * Returns true once the initial load has completed and the underlying scheduler is still
+ * running.
+ */
+ public boolean isRunning() {
+ return initialLoadComplete.get() && !scheduler.isShutdown();
+ }
+
+ private void reloadSafely() {
+ try {
+ reload();
+ } catch (UncheckedIOException e) {
Review Comment:
This looks unsafe to me. For example,
- `Properties.load` can throw `IllegalArgumentException` which is not `UncheckedIOException`.
- `namespaceUpdater.accept(watchedNamespacesSupplier.get())` can throw `RuntimeException` which is not `UncheckedIOException`.
Please double-check the code patch and make this robust.
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/probe/HealthProbe.java:
##########
@@ -60,7 +62,6 @@ public boolean isHealthy() {
if (!areOperatorsStarted(operators).orElse(false)) {
return false;
}
-
Review Comment:
nit. Please don't mix a style-change into this PR. Simply, recover the original empty line and reduce the footprint (size) of this PR.
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/config/SparkOperatorConf.java:
##########
@@ -235,32 +235,71 @@ public final class SparkOperatorConf {
.build();
/**
- * When enabled, operator would use config map as source of truth for config property override.
- * The config map need to be created in spark.kubernetes.operator.namespace, and labeled with
- * operator name.
+ * When enabled, operator would load config property overrides dynamically at runtime. The source
+ * of the overrides is controlled by spark.kubernetes.operator.dynamicConfig.source.
*/
public static final ConfigOption<Boolean> DYNAMIC_CONFIG_ENABLED =
ConfigOption.<Boolean>builder()
.key("spark.kubernetes.operator.dynamicConfig.enabled")
.enableDynamicOverride(false)
.description(
- "When enabled, operator would use config map as source of truth for config "
- + "property override. The config map need to be created in "
- + "spark.kubernetes.operator.namespace, and labeled with operator name.")
+ "When enabled, operator would load config property overrides dynamically at "
+ + "runtime. The source of the overrides is controlled by "
+ + "spark.kubernetes.operator.dynamicConfig.source.")
.typeParameterClass(Boolean.class)
.defaultValue(false)
.build();
- /** The selector str applied to dynamic config map. */
+ /** Source of dynamic config overrides: {@code configMap} (informer) or {@code file} (mount). */
+ public static final ConfigOption<String> DYNAMIC_CONFIG_SOURCE =
+ ConfigOption.<String>builder()
+ .key("spark.kubernetes.operator.dynamicConfig.source")
+ .enableDynamicOverride(false)
+ .description(
+ "Source of dynamic config overrides when "
+ + "spark.kubernetes.operator.dynamicConfig.enabled is true. Supported values: "
+ + "'configMap' (default) watches a ConfigMap via a Kubernetes informer and "
+ + "requires RBAC to read ConfigMaps; 'file' periodically reloads a properties "
+ + "file mounted from a ConfigMap and requires no extra RBAC.")
+ .typeParameterClass(String.class)
+ .defaultValue("configMap")
+ .build();
+
+ /** The selector str applied to dynamic config map (used by the {@code configMap} source). */
public static final ConfigOption<String> DYNAMIC_CONFIG_SELECTOR =
ConfigOption.<String>builder()
.key("spark.kubernetes.operator.dynamicConfig.selector")
.enableDynamicOverride(false)
- .description("The selector str applied to dynamic config map.")
+ .description(
+ "The selector str applied to dynamic config map. Used by the 'configMap' source.")
.typeParameterClass(String.class)
.defaultValue(Utils.labelsAsStr(Utils.defaultOperatorConfigLabels()))
.build();
+ /** Path of the properties file that holds dynamic config overrides. */
+ public static final ConfigOption<String> DYNAMIC_CONFIG_FILE_PATH =
+ ConfigOption.<String>builder()
+ .key("spark.kubernetes.operator.dynamicConfig.filePath")
+ .enableDynamicOverride(false)
+ .description(
+ "Path of the properties file holding dynamic configuration overrides. Used by the "
+ + "'file' source, typically populated by mounting a ConfigMap as a volume.")
+ .typeParameterClass(String.class)
+ .defaultValue("/opt/spark-operator/dynamic-conf/spark-operator-dynamic.properties")
+ .build();
+
+ /** Interval at which the dynamic config file is re-read (used by the {@code file} source). */
+ public static final ConfigOption<Long> DYNAMIC_CONFIG_RELOAD_INTERVAL_SECONDS =
+ ConfigOption.<Long>builder()
+ .key("spark.kubernetes.operator.dynamicConfig.reloadIntervalSeconds")
Review Comment:
We need to accept only positive value. For 0 or negative, `operator` will fail due to `IllegalArgumentException` at start-up.
--
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-57466] Replace dynamic-config controller with mounted-ConfigMap file watcher [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #707:
URL: https://github.com/apache/spark-kubernetes-operator/pull/707#discussion_r3415028686
##########
docs/config_properties.md:
##########
@@ -10,9 +10,10 @@
| spark.kubernetes.operator.api.secondaryResourceCreateMaxAttempts | Long | 3 | false | Maximal number of retry attempts of requesting secondary resource for Spark application. This would be performed on top of k8s client spark.kubernetes.operator.retry.maxAttempts to overcome potential conflicting reconcile on the same SparkApplication, as well as API server errors (408/500/502/503/504) and network-level timeouts. Exponential backoff with jitter is applied before retrying on 409 (Conflict) and 429 (Too Many Requests) responses. Other retryable errors are retried immediately. This should be positive number. |
| spark.kubernetes.operator.api.secondaryResourceCreateMaxBackoffMillis | Long | 40000 | false | Maximum backoff (in milliseconds) between retries when creating secondary resources for Spark application. |
| spark.kubernetes.operator.api.statusPatchMaxAttempts | Long | 3 | false | Maximal number of retry attempts of requests to k8s server for resource status update. This would be performed on top of k8s client spark.kubernetes.operator.retry.maxAttempts to overcome potential conflicting update on the same SparkApplication. This should be positive number. |
- | spark.kubernetes.operator.dynamicConfig.enabled | Boolean | false | false | When enabled, operator would use config map as source of truth for config property override. The config map need to be created in spark.kubernetes.operator.namespace, and labeled with operator name. |
+ | spark.kubernetes.operator.dynamicConfig.enabled | Boolean | false | false | When enabled, operator periodically reloads property overrides from a properties file mounted from a ConfigMap. The file path is controlled by spark.kubernetes.operator.dynamicConfig.filePath. |
+ | spark.kubernetes.operator.dynamicConfig.filePath | String | /opt/spark-operator/dynamic-conf/spark-operator-dynamic.properties | false | Path of the properties file holding dynamic configuration overrides. Typically populated by mounting a ConfigMap as a volume. |
| spark.kubernetes.operator.dynamicConfig.reconcilerParallelism | Integer | 1 | false | Parallelism for dynamic config reconciler. Unbounded pool would be used if set to non-positive number. |
- | spark.kubernetes.operator.dynamicConfig.selector | String | app.kubernetes.io/name=spark-kubernetes-operator,app.kubernetes.io/component=operator-dynamic-config-overrides | false | The selector str applied to dynamic config map. |
Review Comment:
BTW, @csviri . This is intrusive. In general, Apache Spark community tries to keep old features as legacy alternatives. Please recover the old feature? I also didn't merge my original work due to that.
##########
docs/config_properties.md:
##########
@@ -10,9 +10,10 @@
| spark.kubernetes.operator.api.secondaryResourceCreateMaxAttempts | Long | 3 | false | Maximal number of retry attempts of requesting secondary resource for Spark application. This would be performed on top of k8s client spark.kubernetes.operator.retry.maxAttempts to overcome potential conflicting reconcile on the same SparkApplication, as well as API server errors (408/500/502/503/504) and network-level timeouts. Exponential backoff with jitter is applied before retrying on 409 (Conflict) and 429 (Too Many Requests) responses. Other retryable errors are retried immediately. This should be positive number. |
| spark.kubernetes.operator.api.secondaryResourceCreateMaxBackoffMillis | Long | 40000 | false | Maximum backoff (in milliseconds) between retries when creating secondary resources for Spark application. |
| spark.kubernetes.operator.api.statusPatchMaxAttempts | Long | 3 | false | Maximal number of retry attempts of requests to k8s server for resource status update. This would be performed on top of k8s client spark.kubernetes.operator.retry.maxAttempts to overcome potential conflicting update on the same SparkApplication. This should be positive number. |
- | spark.kubernetes.operator.dynamicConfig.enabled | Boolean | false | false | When enabled, operator would use config map as source of truth for config property override. The config map need to be created in spark.kubernetes.operator.namespace, and labeled with operator name. |
+ | spark.kubernetes.operator.dynamicConfig.enabled | Boolean | false | false | When enabled, operator periodically reloads property overrides from a properties file mounted from a ConfigMap. The file path is controlled by spark.kubernetes.operator.dynamicConfig.filePath. |
+ | spark.kubernetes.operator.dynamicConfig.filePath | String | /opt/spark-operator/dynamic-conf/spark-operator-dynamic.properties | false | Path of the properties file holding dynamic configuration overrides. Typically populated by mounting a ConfigMap as a volume. |
| spark.kubernetes.operator.dynamicConfig.reconcilerParallelism | Integer | 1 | false | Parallelism for dynamic config reconciler. Unbounded pool would be used if set to non-positive number. |
- | spark.kubernetes.operator.dynamicConfig.selector | String | app.kubernetes.io/name=spark-kubernetes-operator,app.kubernetes.io/component=operator-dynamic-config-overrides | false | The selector str applied to dynamic config map. |
Review Comment:
BTW, @csviri . This is intrusive. Apache Spark community keeps old features as legacy alternatives in general. Please recover the old feature.
--
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-57466] Add option for dynamic-config to use mounted-ConfigMap [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #707:
URL: https://github.com/apache/spark-kubernetes-operator/pull/707#issuecomment-4720366008
Thank you, @csviri .
cc @aaruna , too.
--
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-57466] Replace dynamic-config controller with mounted-ConfigMap file watcher [spark-kubernetes-operator]
Posted by "csviri (via GitHub)" <gi...@apache.org>.
csviri commented on code in PR #707:
URL: https://github.com/apache/spark-kubernetes-operator/pull/707#discussion_r3415470975
##########
docs/config_properties.md:
##########
@@ -10,9 +10,10 @@
| spark.kubernetes.operator.api.secondaryResourceCreateMaxAttempts | Long | 3 | false | Maximal number of retry attempts of requesting secondary resource for Spark application. This would be performed on top of k8s client spark.kubernetes.operator.retry.maxAttempts to overcome potential conflicting reconcile on the same SparkApplication, as well as API server errors (408/500/502/503/504) and network-level timeouts. Exponential backoff with jitter is applied before retrying on 409 (Conflict) and 429 (Too Many Requests) responses. Other retryable errors are retried immediately. This should be positive number. |
| spark.kubernetes.operator.api.secondaryResourceCreateMaxBackoffMillis | Long | 40000 | false | Maximum backoff (in milliseconds) between retries when creating secondary resources for Spark application. |
| spark.kubernetes.operator.api.statusPatchMaxAttempts | Long | 3 | false | Maximal number of retry attempts of requests to k8s server for resource status update. This would be performed on top of k8s client spark.kubernetes.operator.retry.maxAttempts to overcome potential conflicting update on the same SparkApplication. This should be positive number. |
- | spark.kubernetes.operator.dynamicConfig.enabled | Boolean | false | false | When enabled, operator would use config map as source of truth for config property override. The config map need to be created in spark.kubernetes.operator.namespace, and labeled with operator name. |
+ | spark.kubernetes.operator.dynamicConfig.enabled | Boolean | false | false | When enabled, operator periodically reloads property overrides from a properties file mounted from a ConfigMap. The file path is controlled by spark.kubernetes.operator.dynamicConfig.filePath. |
+ | spark.kubernetes.operator.dynamicConfig.filePath | String | /opt/spark-operator/dynamic-conf/spark-operator-dynamic.properties | false | Path of the properties file holding dynamic configuration overrides. Typically populated by mounting a ConfigMap as a volume. |
| spark.kubernetes.operator.dynamicConfig.reconcilerParallelism | Integer | 1 | false | Parallelism for dynamic config reconciler. Unbounded pool would be used if set to non-positive number. |
- | spark.kubernetes.operator.dynamicConfig.selector | String | app.kubernetes.io/name=spark-kubernetes-operator,app.kubernetes.io/component=operator-dynamic-config-overrides | false | The selector str applied to dynamic config map. |
Review Comment:
@dongjoon-hyun will do, thank you!
--
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-57466] Add option for dynamic-config to use mounted-ConfigMap [spark-kubernetes-operator]
Posted by "csviri (via GitHub)" <gi...@apache.org>.
csviri commented on code in PR #707:
URL: https://github.com/apache/spark-kubernetes-operator/pull/707#discussion_r3427359242
##########
spark-operator/src/test/java/org/apache/spark/k8s/operator/probe/HealthProbeTest.java:
##########
@@ -86,48 +78,12 @@ public boolean isStarted() {
}
};
- RuntimeInfo runtimeInfo2 =
- new RuntimeInfo(
- new Operator(overrider -> overrider.withKubernetesClient(kubernetesClient))) {
- @Override
- public boolean isStarted() {
- return isRunning2.get();
- }
-
- @Override
- public Map<String, Map<String, InformerWrappingEventSourceHealthIndicator>>
- unhealthyInformerWrappingEventSourceHealthIndicator() {
- return unhealthyEventSources2;
- }
- };
-
when(operator.getRuntimeInfo()).thenReturn(runtimeInfo);
- when(sparkConfMonitor.getRuntimeInfo()).thenReturn(runtimeInfo2);
- }
-
- @Test
- void testHealthProbeWithInformerHealthWithMultiOperators() {
Review Comment:
No, nit properly, good catch, fixed that and `ProbeServiceTest`. thx!
--
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-57466] Add option for dynamic-config to use mounted-ConfigMap [spark-kubernetes-operator]
Posted by "csviri (via GitHub)" <gi...@apache.org>.
csviri commented on code in PR #707:
URL: https://github.com/apache/spark-kubernetes-operator/pull/707#discussion_r3426681225
##########
tests/e2e/watched-namespaces/spark-operator-dynamic-config-1.yaml:
##########
@@ -17,7 +17,8 @@
apiVersion: v1
data:
- spark.kubernetes.operator.watchedNamespaces: default, spark-1
+ spark-operator-dynamic.properties: |
+ spark.kubernetes.operator.watchedNamespaces=default, spark-1
Review Comment:
yes, see below.
--
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-57466] Add option for dynamic-config to use mounted-ConfigMap [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #707:
URL: https://github.com/apache/spark-kubernetes-operator/pull/707#discussion_r3424375829
##########
tests/e2e/watched-namespaces/spark-operator-dynamic-config-1.yaml:
##########
@@ -17,7 +17,8 @@
apiVersion: v1
data:
- spark.kubernetes.operator.watchedNamespaces: default, spark-1
+ spark-operator-dynamic.properties: |
+ spark.kubernetes.operator.watchedNamespaces=default, spark-1
Review Comment:
Do we need to change 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-57466] Add option for dynamic-config to use mounted-ConfigMap [spark-kubernetes-operator]
Posted by "csviri (via GitHub)" <gi...@apache.org>.
csviri commented on code in PR #707:
URL: https://github.com/apache/spark-kubernetes-operator/pull/707#discussion_r3426681225
##########
tests/e2e/watched-namespaces/spark-operator-dynamic-config-1.yaml:
##########
@@ -17,7 +17,8 @@
apiVersion: v1
data:
- spark.kubernetes.operator.watchedNamespaces: default, spark-1
+ spark-operator-dynamic.properties: |
+ spark.kubernetes.operator.watchedNamespaces=default, spark-1
Review Comment:
yes, afaik there is not way to mount all ConfigMap data in K8S. A key is always a new file.
##########
tests/e2e/watched-namespaces/spark-operator-dynamic-config-2.yaml:
##########
@@ -17,7 +17,8 @@
apiVersion: v1
data:
- spark.kubernetes.operator.watchedNamespaces: default
+ spark-operator-dynamic.properties: |
+ spark.kubernetes.operator.watchedNamespaces=default
Review Comment:
Same as above: yes, afaik there is not way to mount all ConfigMap data in K8S. A key is always a new file.
--
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-57466] Support `s.k.o.dynamicConfig.source` to use mounted-`ConfigMap` [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun closed pull request #707: [SPARK-57466] Support `s.k.o.dynamicConfig.source` to use mounted-`ConfigMap`
URL: https://github.com/apache/spark-kubernetes-operator/pull/707
--
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-57466] Add option for dynamic-config to use mounted-ConfigMap [spark-kubernetes-operator]
Posted by "csviri (via GitHub)" <gi...@apache.org>.
csviri commented on code in PR #707:
URL: https://github.com/apache/spark-kubernetes-operator/pull/707#discussion_r3426682227
##########
tests/e2e/watched-namespaces/spark-operator-dynamic-config-2.yaml:
##########
@@ -17,7 +17,8 @@
apiVersion: v1
data:
- spark.kubernetes.operator.watchedNamespaces: default
+ spark-operator-dynamic.properties: |
+ spark.kubernetes.operator.watchedNamespaces=default
Review Comment:
Same as above: yes, afaik there is no way to mount all ConfigMap data as a single file in K8S. A key is always a new file. So we have to place the whole content under a key in ConfigMap.
--
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-57466] Support `s.k.o.dynamicConfig.source` to use mounted-`ConfigMap` [spark-kubernetes-operator]
Posted by "csviri (via GitHub)" <gi...@apache.org>.
csviri commented on code in PR #707:
URL: https://github.com/apache/spark-kubernetes-operator/pull/707#discussion_r3465812276
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/config/DynamicConfigMonitor.java:
##########
@@ -0,0 +1,160 @@
+/*
+ * 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.config;
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.UncheckedIOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.time.Duration;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Properties;
+import java.util.Set;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicReference;
+import java.util.function.Consumer;
+import java.util.function.Supplier;
+
+import lombok.extern.slf4j.Slf4j;
+
+/**
+ * Periodically reloads dynamic configuration overrides from a properties file on disk. The file is
+ * expected to be populated by mounting a ConfigMap as a volume into the operator pod, so changes
+ * applied to the ConfigMap propagate to disk without requiring a Kubernetes informer. When the
+ * file contents change, {@link SparkOperatorConfManager} is refreshed and the caller-supplied
+ * namespace updater is invoked.
+ */
+@Slf4j
+public class DynamicConfigMonitor {
+
+ private final Path configFile;
+ private final Duration reloadInterval;
+ private final Supplier<Set<String>> watchedNamespacesSupplier;
+ private final Consumer<Set<String>> namespaceUpdater;
+ private final ScheduledExecutorService scheduler;
+ private final boolean ownsScheduler;
+
+ private final AtomicReference<Map<String, String>> lastLoaded = new AtomicReference<>(Map.of());
+ private final AtomicBoolean initialLoadComplete = new AtomicBoolean();
+
+ public DynamicConfigMonitor(
+ Path configFile,
+ Duration reloadInterval,
+ Supplier<Set<String>> watchedNamespacesSupplier,
+ Consumer<Set<String>> namespaceUpdater) {
+ this(configFile, reloadInterval, watchedNamespacesSupplier, namespaceUpdater, null);
+ }
+
+ DynamicConfigMonitor(
+ Path configFile,
+ Duration reloadInterval,
+ Supplier<Set<String>> watchedNamespacesSupplier,
+ Consumer<Set<String>> namespaceUpdater,
+ ScheduledExecutorService scheduler) {
+ this.configFile = configFile;
+ this.reloadInterval = reloadInterval;
+ this.watchedNamespacesSupplier = watchedNamespacesSupplier;
+ this.namespaceUpdater = namespaceUpdater;
+ if (scheduler == null) {
+ this.scheduler =
+ Executors.newSingleThreadScheduledExecutor(
+ r -> {
+ Thread t = new Thread(r, "spark-operator-dynamic-config");
+ t.setDaemon(true);
+ return t;
+ });
+ this.ownsScheduler = true;
+ } else {
+ this.scheduler = scheduler;
+ this.ownsScheduler = false;
+ }
+ }
+
+ /**
+ * Performs an initial synchronous load and schedules periodic reloads at the configured
+ * interval.
+ */
+ public void start() {
+ log.info(
+ "Starting dynamic config monitor on {} with reload interval {}",
+ configFile,
+ reloadInterval);
+ reload();
+ initialLoadComplete.set(true);
+ long millis = reloadInterval.toMillis();
+ scheduler.scheduleAtFixedRate(this::reloadSafely, millis, millis, TimeUnit.MILLISECONDS);
+ }
+
+ /** Stops the scheduler if it was created internally. */
+ public void stop() {
+ log.info("Stopping dynamic config monitor");
+ if (ownsScheduler) {
+ scheduler.shutdownNow();
+ }
+ }
+
+ /**
+ * Returns true once the initial load has completed and the underlying scheduler is still
+ * running.
+ */
+ public boolean isRunning() {
+ return initialLoadComplete.get() && !scheduler.isShutdown();
+ }
+
+ private void reloadSafely() {
+ try {
+ reload();
+ } catch (UncheckedIOException e) {
Review Comment:
Now we are catching all RuntimeException. WDYT? thx!
--
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-57466] Support `s.k.o.dynamicConfig.source` to use mounted-`ConfigMap` [spark-kubernetes-operator]
Posted by "csviri (via GitHub)" <gi...@apache.org>.
csviri commented on code in PR #707:
URL: https://github.com/apache/spark-kubernetes-operator/pull/707#discussion_r3466732193
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/config/DynamicConfigMonitor.java:
##########
@@ -0,0 +1,160 @@
+/*
+ * 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.config;
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.UncheckedIOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.time.Duration;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Properties;
+import java.util.Set;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicReference;
+import java.util.function.Consumer;
+import java.util.function.Supplier;
+
+import lombok.extern.slf4j.Slf4j;
+
+/**
+ * Periodically reloads dynamic configuration overrides from a properties file on disk. The file is
+ * expected to be populated by mounting a ConfigMap as a volume into the operator pod, so changes
+ * applied to the ConfigMap propagate to disk without requiring a Kubernetes informer. When the
+ * file contents change, {@link SparkOperatorConfManager} is refreshed and the caller-supplied
+ * namespace updater is invoked.
+ */
+@Slf4j
+public class DynamicConfigMonitor {
+
+ private final Path configFile;
+ private final Duration reloadInterval;
+ private final Supplier<Set<String>> watchedNamespacesSupplier;
+ private final Consumer<Set<String>> namespaceUpdater;
+ private final ScheduledExecutorService scheduler;
+ private final boolean ownsScheduler;
+
+ private final AtomicReference<Map<String, String>> lastLoaded = new AtomicReference<>(Map.of());
+ private final AtomicBoolean initialLoadComplete = new AtomicBoolean();
+
+ public DynamicConfigMonitor(
+ Path configFile,
+ Duration reloadInterval,
+ Supplier<Set<String>> watchedNamespacesSupplier,
+ Consumer<Set<String>> namespaceUpdater) {
+ this(configFile, reloadInterval, watchedNamespacesSupplier, namespaceUpdater, null);
+ }
+
+ DynamicConfigMonitor(
+ Path configFile,
+ Duration reloadInterval,
+ Supplier<Set<String>> watchedNamespacesSupplier,
+ Consumer<Set<String>> namespaceUpdater,
+ ScheduledExecutorService scheduler) {
+ this.configFile = configFile;
+ this.reloadInterval = reloadInterval;
+ this.watchedNamespacesSupplier = watchedNamespacesSupplier;
+ this.namespaceUpdater = namespaceUpdater;
+ if (scheduler == null) {
+ this.scheduler =
+ Executors.newSingleThreadScheduledExecutor(
+ r -> {
+ Thread t = new Thread(r, "spark-operator-dynamic-config");
+ t.setDaemon(true);
+ return t;
+ });
+ this.ownsScheduler = true;
+ } else {
+ this.scheduler = scheduler;
+ this.ownsScheduler = false;
+ }
+ }
+
+ /**
+ * Performs an initial synchronous load and schedules periodic reloads at the configured
+ * interval.
+ */
+ public void start() {
+ log.info(
+ "Starting dynamic config monitor on {} with reload interval {}",
+ configFile,
+ reloadInterval);
+ reload();
+ initialLoadComplete.set(true);
+ long millis = reloadInterval.toMillis();
+ scheduler.scheduleAtFixedRate(this::reloadSafely, millis, millis, TimeUnit.MILLISECONDS);
+ }
+
+ /** Stops the scheduler if it was created internally. */
+ public void stop() {
+ log.info("Stopping dynamic config monitor");
+ if (ownsScheduler) {
+ scheduler.shutdownNow();
+ }
+ }
+
+ /**
+ * Returns true once the initial load has completed and the underlying scheduler is still
+ * running.
+ */
+ public boolean isRunning() {
+ return initialLoadComplete.get() && !scheduler.isShutdown();
+ }
+
+ private void reloadSafely() {
+ try {
+ reload();
+ } catch (UncheckedIOException e) {
+ log.error("Failed to reload dynamic config from {}", configFile, e);
+ }
+ }
+
+ private void reload() {
+ Map<String, String> current = readProperties();
+ if (current.equals(lastLoaded.get())) {
+ return;
+ }
+ log.info(
+ "Detected dynamic config change in {}, applying {} overrides", configFile, current.size());
+ SparkOperatorConfManager.INSTANCE.refresh(current);
+ lastLoaded.set(current);
+ namespaceUpdater.accept(watchedNamespacesSupplier.get());
Review Comment:
I altered the implementation, now it has more consistent behavior in that regard with the controller based implementation. Please let me know, if it is good enough this way. thx!
--
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-57466] Add option for dynamic-config to use mounted-ConfigMap [spark-kubernetes-operator]
Posted by "csviri (via GitHub)" <gi...@apache.org>.
csviri commented on PR #707:
URL: https://github.com/apache/spark-kubernetes-operator/pull/707#issuecomment-4739380149
> Hi, @csviri . According to your comment, the following sounds irrelevant to this PR's proposal. Could you make a new PR only for that? We can proceed that independent topic before this PR.
>
> > yes, afaik there is not way to mount all ConfigMap data in K8S. A key is always a new file.
>
> ```yaml
> data:
> - spark.kubernetes.operator.watchedNamespaces: default
> + spark-operator-dynamic.properties: |
> + spark.kubernetes.operator.watchedNamespaces=default
> ```
@dongjoon-hyun maybe I was not clear, those are needed/used in the chainsaw test, and needs to be in this format. Or do I miss something?
--
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-57466] Support `s.k.o.dynamicConfig.source` to use mounted-`ConfigMap` [spark-kubernetes-operator]
Posted by "csviri (via GitHub)" <gi...@apache.org>.
csviri commented on code in PR #707:
URL: https://github.com/apache/spark-kubernetes-operator/pull/707#discussion_r3465809500
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/config/SparkOperatorConf.java:
##########
@@ -235,32 +235,71 @@ public final class SparkOperatorConf {
.build();
/**
- * When enabled, operator would use config map as source of truth for config property override.
- * The config map need to be created in spark.kubernetes.operator.namespace, and labeled with
- * operator name.
+ * When enabled, operator would load config property overrides dynamically at runtime. The source
+ * of the overrides is controlled by spark.kubernetes.operator.dynamicConfig.source.
*/
public static final ConfigOption<Boolean> DYNAMIC_CONFIG_ENABLED =
ConfigOption.<Boolean>builder()
.key("spark.kubernetes.operator.dynamicConfig.enabled")
.enableDynamicOverride(false)
.description(
- "When enabled, operator would use config map as source of truth for config "
- + "property override. The config map need to be created in "
- + "spark.kubernetes.operator.namespace, and labeled with operator name.")
+ "When enabled, operator would load config property overrides dynamically at "
+ + "runtime. The source of the overrides is controlled by "
+ + "spark.kubernetes.operator.dynamicConfig.source.")
.typeParameterClass(Boolean.class)
.defaultValue(false)
.build();
- /** The selector str applied to dynamic config map. */
+ /** Source of dynamic config overrides: {@code configMap} (informer) or {@code file} (mount). */
+ public static final ConfigOption<String> DYNAMIC_CONFIG_SOURCE =
+ ConfigOption.<String>builder()
+ .key("spark.kubernetes.operator.dynamicConfig.source")
+ .enableDynamicOverride(false)
+ .description(
+ "Source of dynamic config overrides when "
+ + "spark.kubernetes.operator.dynamicConfig.enabled is true. Supported values: "
+ + "'configMap' (default) watches a ConfigMap via a Kubernetes informer and "
+ + "requires RBAC to read ConfigMaps; 'file' periodically reloads a properties "
+ + "file mounted from a ConfigMap and requires no extra RBAC.")
+ .typeParameterClass(String.class)
+ .defaultValue("configMap")
+ .build();
+
+ /** The selector str applied to dynamic config map (used by the {@code configMap} source). */
public static final ConfigOption<String> DYNAMIC_CONFIG_SELECTOR =
ConfigOption.<String>builder()
.key("spark.kubernetes.operator.dynamicConfig.selector")
.enableDynamicOverride(false)
- .description("The selector str applied to dynamic config map.")
+ .description(
+ "The selector str applied to dynamic config map. Used by the 'configMap' source.")
.typeParameterClass(String.class)
.defaultValue(Utils.labelsAsStr(Utils.defaultOperatorConfigLabels()))
.build();
+ /** Path of the properties file that holds dynamic config overrides. */
+ public static final ConfigOption<String> DYNAMIC_CONFIG_FILE_PATH =
+ ConfigOption.<String>builder()
+ .key("spark.kubernetes.operator.dynamicConfig.filePath")
+ .enableDynamicOverride(false)
+ .description(
+ "Path of the properties file holding dynamic configuration overrides. Used by the "
+ + "'file' source, typically populated by mounting a ConfigMap as a volume.")
+ .typeParameterClass(String.class)
+ .defaultValue("/opt/spark-operator/dynamic-conf/spark-operator-dynamic.properties")
+ .build();
+
+ /** Interval at which the dynamic config file is re-read (used by the {@code file} source). */
+ public static final ConfigOption<Long> DYNAMIC_CONFIG_RELOAD_INTERVAL_SECONDS =
+ ConfigOption.<Long>builder()
+ .key("spark.kubernetes.operator.dynamicConfig.reloadIntervalSeconds")
Review Comment:
I added check! thx!
--
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-57466] Replace dynamic-config controller with mounted-ConfigMap file watcher [spark-kubernetes-operator]
Posted by "csviri (via GitHub)" <gi...@apache.org>.
csviri commented on PR #707:
URL: https://github.com/apache/spark-kubernetes-operator/pull/707#issuecomment-4706937762
looking into failing tests
--
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-57466] Replace dynamic-config controller with mounted-ConfigMap file watcher [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #707:
URL: https://github.com/apache/spark-kubernetes-operator/pull/707#discussion_r3415028686
##########
docs/config_properties.md:
##########
@@ -10,9 +10,10 @@
| spark.kubernetes.operator.api.secondaryResourceCreateMaxAttempts | Long | 3 | false | Maximal number of retry attempts of requesting secondary resource for Spark application. This would be performed on top of k8s client spark.kubernetes.operator.retry.maxAttempts to overcome potential conflicting reconcile on the same SparkApplication, as well as API server errors (408/500/502/503/504) and network-level timeouts. Exponential backoff with jitter is applied before retrying on 409 (Conflict) and 429 (Too Many Requests) responses. Other retryable errors are retried immediately. This should be positive number. |
| spark.kubernetes.operator.api.secondaryResourceCreateMaxBackoffMillis | Long | 40000 | false | Maximum backoff (in milliseconds) between retries when creating secondary resources for Spark application. |
| spark.kubernetes.operator.api.statusPatchMaxAttempts | Long | 3 | false | Maximal number of retry attempts of requests to k8s server for resource status update. This would be performed on top of k8s client spark.kubernetes.operator.retry.maxAttempts to overcome potential conflicting update on the same SparkApplication. This should be positive number. |
- | spark.kubernetes.operator.dynamicConfig.enabled | Boolean | false | false | When enabled, operator would use config map as source of truth for config property override. The config map need to be created in spark.kubernetes.operator.namespace, and labeled with operator name. |
+ | spark.kubernetes.operator.dynamicConfig.enabled | Boolean | false | false | When enabled, operator periodically reloads property overrides from a properties file mounted from a ConfigMap. The file path is controlled by spark.kubernetes.operator.dynamicConfig.filePath. |
+ | spark.kubernetes.operator.dynamicConfig.filePath | String | /opt/spark-operator/dynamic-conf/spark-operator-dynamic.properties | false | Path of the properties file holding dynamic configuration overrides. Typically populated by mounting a ConfigMap as a volume. |
| spark.kubernetes.operator.dynamicConfig.reconcilerParallelism | Integer | 1 | false | Parallelism for dynamic config reconciler. Unbounded pool would be used if set to non-positive number. |
- | spark.kubernetes.operator.dynamicConfig.selector | String | app.kubernetes.io/name=spark-kubernetes-operator,app.kubernetes.io/component=operator-dynamic-config-overrides | false | The selector str applied to dynamic config map. |
Review Comment:
BTW, @csviri . This is intrusive. In general, Apache Spark community tries to keep old features as legacy alternatives. Please recover the old feature.
--
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-57466] Add option for dynamic-config to use mounted-ConfigMap [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #707:
URL: https://github.com/apache/spark-kubernetes-operator/pull/707#discussion_r3424376952
##########
tests/e2e/watched-namespaces/spark-operator-dynamic-config-2.yaml:
##########
@@ -17,7 +17,8 @@
apiVersion: v1
data:
- spark.kubernetes.operator.watchedNamespaces: default
+ spark-operator-dynamic.properties: |
+ spark.kubernetes.operator.watchedNamespaces=default
Review Comment:
Do we need to change 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