Skip to content

Commit 542dfa6

Browse files
committed
Code Review Fixes
1 parent 8a2ed9e commit 542dfa6

3 files changed

Lines changed: 51 additions & 44 deletions

File tree

‎docs/migration_guide.md‎

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -164,13 +164,22 @@ affect building the project, CI, tests or examples.
164164
labels in `spec.driverServiceIngressList` (`serviceMetadata`, `ingressMetadata`),
165165
`spec.driverHttpRouteList` (`serviceMetadata`, `httpRouteMetadata`), `spec.driverGrpcRouteList`
166166
(`serviceMetadata`, `grpcRouteMetadata`) and `spec.configMapSpecs`, as well as the other
167-
Kubernetes resources of the driver, e.g. its config map and on-demand PVCs. 1.0 overwrote these
168-
labels with the driver-pod labels of the same key, including the ones injected into the driver pod
169-
by admission webhooks. The operator- and Spark-managed identity labels, i.e. any `spark.operator/`
167+
Kubernetes resources of the driver, e.g. its service with `spark.kubernetes.driver.service.label.*`
168+
and its on-demand PVCs with `spark.kubernetes.driver.volumes.persistentVolumeClaim.<name>.label.*`.
169+
1.0 overwrote these labels with the driver-pod labels of the same key, including the ones injected
170+
into the driver pod by admission webhooks. The operator- and Spark-managed identity labels, i.e. any `spark.operator/`
170171
key, `spark-role`, `spark-version`, `spark-app-selector` and `spark-app-name`, still take the
171-
driver-pod value, so the secondary-to-primary mapping and the pod and service selectors keep
172-
working. To restore the behavior before 1.1.0 for a given key, remove it from the secondary
173-
resource so it inherits the driver-pod label.
172+
driver-pod value, since they identify the resources of an application, e.g. Spark deletes its
173+
services, PVCs and executor config maps by `spark-app-selector` and `spark-role`. To restore the
174+
behavior before 1.1.0 for a given key, remove it from the secondary
175+
resource so it inherits the driver-pod label. Note that Spark substitutes `{{APP_ID}}` only in
176+
driver pod labels (`spark.kubernetes.driver.label.*`), not in driver service labels
177+
(`spark.kubernetes.driver.service.label.*`) or PVC labels
178+
(`spark.kubernetes.driver.volumes.persistentVolumeClaim.<name>.label.*`). A raw `{{APP_ID}}` in
179+
those is therefore no longer replaced with the substituted driver-pod value, producing an invalid
180+
label value that the API server rejects with `422`, so the application fails with
181+
`SchedulingFailure`. Set such a label on the driver pod only, and let the resource inherit the
182+
substituted value.
174183

175184
### SparkCluster
176185

‎spark-operator/src/main/java/org/apache/spark/k8s/operator/decorators/DriverResourceDecorator.java‎

Lines changed: 12 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -19,20 +19,16 @@
1919

2020
package org.apache.spark.k8s.operator.decorators;
2121

22-
import static org.apache.spark.k8s.operator.Constants.LABEL_SPARK_APPLICATION_NAME;
2322
import static org.apache.spark.k8s.operator.Constants.LABEL_SPARK_APP_NAME;
2423
import static org.apache.spark.k8s.operator.Constants.LABEL_SPARK_APP_SELECTOR;
25-
import static org.apache.spark.k8s.operator.Constants.LABEL_SPARK_OPERATOR_NAME;
2624
import static org.apache.spark.k8s.operator.Constants.LABEL_SPARK_ROLE_NAME;
2725
import static org.apache.spark.k8s.operator.Constants.LABEL_SPARK_VERSION_NAME;
2826
import static org.apache.spark.k8s.operator.utils.ModelUtils.buildOwnerReferenceTo;
2927

3028
import java.util.Map;
3129
import java.util.Set;
32-
import java.util.stream.Collectors;
3330

3431
import io.fabric8.kubernetes.api.model.HasMetadata;
35-
import io.fabric8.kubernetes.api.model.ObjectMeta;
3632
import io.fabric8.kubernetes.api.model.ObjectMetaBuilder;
3733
import io.fabric8.kubernetes.api.model.OwnerReference;
3834
import io.fabric8.kubernetes.api.model.Pod;
@@ -57,14 +53,11 @@ public class DriverResourceDecorator implements ResourceDecorator {
5753

5854
/**
5955
* Operator- and Spark-managed identity labels whose driver-pod value must always propagate to
60-
* secondary resources unchanged. A resource's own label of the same key
61-
* must never override them. Any label key under the {@value #MANAGED_LABEL_PREFIX} prefix is
62-
* protected the same way (which already covers the operator-namespaced keys listed here).
56+
* secondary resources unchanged. A resource's own label of the same key must never override them.
57+
* Any label key under the {@value #MANAGED_LABEL_PREFIX} prefix is protected the same way.
6358
*/
6459
private static final Set<String> MANAGED_LABEL_KEYS =
6560
Set.of(
66-
LABEL_SPARK_OPERATOR_NAME,
67-
LABEL_SPARK_APPLICATION_NAME,
6861
LABEL_SPARK_ROLE_NAME,
6962
LABEL_SPARK_VERSION_NAME,
7063
LABEL_SPARK_APP_SELECTOR,
@@ -102,34 +95,21 @@ public <T extends HasMetadata> T decorate(T resource) {
10295
}
10396
if (!ownerReferenceExists) {
10497
log.debug("Adding OwnerReference to driver for secondary resource");
105-
ObjectMeta metaData =
98+
Map<String, String> resourceLabels = resource.getMetadata().getLabels();
99+
ObjectMetaBuilder builder =
106100
new ObjectMetaBuilder(resource.getMetadata())
107-
.addToOwnerReferences(buildOwnerReferenceTo(driverPod))
108-
.addToLabels(driverPod.getMetadata().getLabels())
109-
.addToLabels(overridableLabels(resource.getMetadata().getLabels()))
110-
.build();
111-
resource.setMetadata(metaData);
101+
.addToOwnerReferences(buildOwnerReferenceTo(driverPod));
102+
driverPod.getMetadata().getLabels().forEach((key, value) -> {
103+
boolean resourceSetsKey = resourceLabels != null && resourceLabels.containsKey(key);
104+
if (isManagedLabel(key) || !resourceSetsKey) {
105+
builder.addToLabels(key, value);
106+
}
107+
});
108+
resource.setMetadata(builder.build());
112109
}
113110
return resource;
114111
}
115112

116-
/**
117-
* Filters the resource's own labels down to the ones that are allowed to override a driver-pod
118-
* label of the same key, dropping operator- and Spark-managed identity labels (see {@link
119-
* #MANAGED_LABEL_KEYS} and {@link #MANAGED_LABEL_PREFIX}).
120-
*
121-
* @param resourceLabels the resource's own labels (may be {@code null}).
122-
* @return the subset of labels safe to re-apply on top of the driver-pod labels.
123-
*/
124-
private static Map<String, String> overridableLabels(Map<String, String> resourceLabels) {
125-
if (resourceLabels == null || resourceLabels.isEmpty()) {
126-
return Map.of();
127-
}
128-
return resourceLabels.entrySet().stream()
129-
.filter(entry -> !isManagedLabel(entry.getKey()))
130-
.collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));
131-
}
132-
133113
/**
134114
* Returns whether the given label key is operator- or Spark-managed and must therefore keep the
135115
* driver-pod value rather than be overridden by the resource.

‎spark-operator/src/test/java/org/apache/spark/k8s/operator/decorators/DriverResourceDecoratorTest.java‎

Lines changed: 24 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,9 @@ private static Pod driverPod() {
5050
.addToLabels("spark.operator/submission-id", "sub-1")
5151
.addToLabels(LABEL_SPARK_ROLE_NAME, "driver")
5252
.addToLabels("spark-app-selector", "spark-app-selector-1")
53+
.addToLabels("spark-version", "4.2.0")
54+
.addToLabels("spark-app-name", "app1-name")
55+
.addToLabels("driver-only-label", "driver-value") // neutral driver only label
5356
.addToLabels("overlapping-label", "driver-value")
5457
.endMetadata()
5558
.build();
@@ -66,12 +69,18 @@ private static Service service(Map<String, String> labels) {
6669
}
6770

6871
/**
69-
* Driver-pod labels whose key is absent on the resource are propagated onto the resource, while a
70-
* label the resource sets itself still wins on key collision (e.g. a per-service waypoint label).
72+
* Driver-pod labels whose key is absent on the resource are propagated onto the resource
73+
* (including a neutral driver-only label), a label the resource sets itself wins on a key
74+
* collision, and a neutral label only the resource sets is
75+
* kept.
7176
*/
7277
@Test
7378
void driverLabelsPropagatedAndResourceLabelWinsOnCollision() {
74-
Service service = service(Map.of("overlapping-label", "resource-value"));
79+
Service service =
80+
service(
81+
Map.of(
82+
"overlapping-label", "resource-value",
83+
"resource-only-label", "resource-value"));
7584

7685
new DriverResourceDecorator(driverPod()).decorate(service);
7786

@@ -82,7 +91,11 @@ void driverLabelsPropagatedAndResourceLabelWinsOnCollision() {
8291
"spark.operator/submission-id", "sub-1",
8392
LABEL_SPARK_ROLE_NAME, "driver",
8493
"spark-app-selector", "spark-app-selector-1",
85-
"overlapping-label", "resource-value"),
94+
"spark-version", "4.2.0",
95+
"spark-app-name", "app1-name",
96+
"driver-only-label", "driver-value",
97+
"overlapping-label", "resource-value",
98+
"resource-only-label", "resource-value"),
8699
service.getMetadata().getLabels());
87100
}
88101

@@ -102,8 +115,8 @@ void resourceWithoutLabelsInheritsDriverPodLabels() {
102115

103116
/**
104117
* Managed identity labels (exact-match keys and anything under the {@code spark.operator} prefix)
105-
* must keep the driver-pod value even when the resource sets a different value, so owner mapping
106-
* and selectors are preserved, while a non-managed label is still allowed to win.
118+
* must keep the driver-pod value even when the resource sets a different value, while a
119+
* non-managed label is still allowed to win.
107120
*/
108121
@Test
109122
void managedLabelsAreNotOverriddenByResource() {
@@ -115,6 +128,8 @@ void managedLabelsAreNotOverriddenByResource() {
115128
"spark.operator/submission-id", "rogue-sub",
116129
LABEL_SPARK_ROLE_NAME, "rogue-role",
117130
"spark-app-selector", "rogue-selector",
131+
"spark-version", "rogue-version",
132+
"spark-app-name", "rogue-app-name",
118133
"overlapping-label", "resource-value"));
119134

120135
new DriverResourceDecorator(driverPod()).decorate(service);
@@ -126,6 +141,9 @@ void managedLabelsAreNotOverriddenByResource() {
126141
"spark.operator/submission-id", "sub-1",
127142
LABEL_SPARK_ROLE_NAME, "driver",
128143
"spark-app-selector", "spark-app-selector-1",
144+
"spark-version", "4.2.0",
145+
"spark-app-name", "app1-name",
146+
"driver-only-label", "driver-value",
129147
"overlapping-label", "resource-value"),
130148
service.getMetadata().getLabels());
131149
}

0 commit comments

Comments
 (0)