dongjoon-hyun commented on code in PR #925:
URL:
https://github.com/apache/spark-kubernetes-operator/pull/925#discussion_r4268228508
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/decorators/DriverResourceDecorator.java:
##########
@@ -69,13 +109,33 @@ public <T extends HasMetadata> T decorate(T resource) {
}
if (!ownerReferenceExists) {
log.debug("Adding OwnerReference to driver for secondary resource");
- ObjectMeta metaData =
+ Map<String, String> resourceLabels = resource.getMetadata().getLabels();
+ Map<String, String> driverPodLabels =
driverPod.getMetadata().getLabels();
+ ObjectMetaBuilder builder =
new ObjectMetaBuilder(resource.getMetadata())
- .addToOwnerReferences(buildOwnerReferenceTo(driverPod))
- .addToLabels(driverPod.getMetadata().getLabels())
- .build();
- resource.setMetadata(metaData);
+ .addToOwnerReferences(buildOwnerReferenceTo(driverPod));
+ if (driverPodLabels != null) {
+ driverPodLabels.forEach(
+ (key, value) -> {
+ boolean resourceSetsKey = resourceLabels != null &&
resourceLabels.containsKey(key);
Review Comment:
This turns a previously working configuration into a failure. Spark
substitutes `{{APP_ID}}` only in the driver pod labels
(`KubernetesConf.labels`), not in `serviceLabels` or the PVC labels. If an app
sets both `spark.kubernetes.driver.label.run={{APP_ID}}` and
`spark.kubernetes.driver.service.label.run={{APP_ID}}`, 1.0 overwrote the raw
service value with the substituted driver-pod value. With this change, the raw
`{{APP_ID}}` is kept, the Service creation in `AppInitStep` fails with `422`,
and the app ends in `SchedulingFailure`. The migration guide documents it, but
could we avoid the regression instead?
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/decorators/DriverResourceDecorator.java:
##########
@@ -45,10 +50,45 @@
public class DriverResourceDecorator implements ResourceDecorator {
private final Pod driverPod;
+ /**
+ * Spark-owned driver label for the application ID, used as the service
selector. Spark sets it;
+ * the operator only propagates it. Package-private so tests can exercise
the managed-label keys.
+ */
+ static final String LABEL_SPARK_APP_SELECTOR = "spark-app-selector";
Review Comment:
Could you move `spark-app-selector` and `spark-app-name` to `Constants`,
next to `LABEL_SPARK_ROLE_NAME` and `LABEL_SPARK_VERSION_NAME`, which are also
Spark-owned keys? Then the test does not need to import package-private
internals of this class.
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/decorators/DriverResourceDecorator.java:
##########
@@ -69,13 +109,33 @@ public <T extends HasMetadata> T decorate(T resource) {
}
if (!ownerReferenceExists) {
log.debug("Adding OwnerReference to driver for secondary resource");
- ObjectMeta metaData =
+ Map<String, String> resourceLabels = resource.getMetadata().getLabels();
+ Map<String, String> driverPodLabels =
driverPod.getMetadata().getLabels();
+ ObjectMetaBuilder builder =
new ObjectMetaBuilder(resource.getMetadata())
- .addToOwnerReferences(buildOwnerReferenceTo(driverPod))
- .addToLabels(driverPod.getMetadata().getLabels())
- .build();
- resource.setMetadata(metaData);
+ .addToOwnerReferences(buildOwnerReferenceTo(driverPod));
+ if (driverPodLabels != null) {
+ driverPodLabels.forEach(
Review Comment:
As the PR description says, the root cause is that every driver-pod label,
including the ones injected by admission webhooks (e.g. Istio
`security.istio.io/tlsMode`, `service.istio.io/canonical-name`) and
`spark.kubernetes.driver.label.*`, is copied to all Services, Ingresses,
ConfigMaps and PVCs. This PR only changes the precedence on a key collision, so
users still need to set each injected key on each resource to keep it off.
Propagating only the `isManagedLabel(key)` labels, which are what GC and Spark
bookkeeping need, would fix the reported Istio issue directly and also avoid
the `{{APP_ID}}` regression.
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/decorators/DriverResourceDecorator.java:
##########
@@ -69,13 +109,33 @@ public <T extends HasMetadata> T decorate(T resource) {
}
if (!ownerReferenceExists) {
log.debug("Adding OwnerReference to driver for secondary resource");
- ObjectMeta metaData =
+ Map<String, String> resourceLabels = resource.getMetadata().getLabels();
+ Map<String, String> driverPodLabels =
driverPod.getMetadata().getLabels();
+ ObjectMetaBuilder builder =
new ObjectMetaBuilder(resource.getMetadata())
- .addToOwnerReferences(buildOwnerReferenceTo(driverPod))
- .addToLabels(driverPod.getMetadata().getLabels())
- .build();
- resource.setMetadata(metaData);
+ .addToOwnerReferences(buildOwnerReferenceTo(driverPod));
+ if (driverPodLabels != null) {
+ driverPodLabels.forEach(
+ (key, value) -> {
Review Comment:
nit. This can be simpler with a merged map, e.g. `merged = new
HashMap<>(resourceLabels)` (or an empty map) and `driverPodLabels.forEach((k,
v) -> { if (isManagedLabel(k)) merged.put(k, v); else merged.putIfAbsent(k, v);
})`. It expresses the precedence directly without `resourceSetsKey`.
##########
spark-operator-api/src/main/java/org/apache/spark/k8s/operator/Constants.java:
##########
@@ -32,17 +32,22 @@ public final class Constants {
/** The API version for Spark K8s Operator CRD resources. */
public static final String API_VERSION = "v1";
+ /** The shared prefix for all operator-defined labels. */
+ public static final String LABEL_SPARK_OPERATOR_PREFIX = "spark.operator/";
+
/** The label for the Spark Application name. */
- public static final String LABEL_SPARK_APPLICATION_NAME =
"spark.operator/spark-app-name";
+ public static final String LABEL_SPARK_APPLICATION_NAME =
Review Comment:
The rewrite of the four existing constants is unrelated to this fix and
makes the literal label values harder to grep (`spark.operator/spark-app-name`
no longer appears verbatim). `ANNOTATION_POD_SETS_HASH` in `KueueWorkloadUtils`
still uses a literal, too. Shall we keep the existing constants as they are and
add only the new prefix constant?
##########
spark-operator/src/test/java/org/apache/spark/k8s/operator/decorators/DriverResourceDecoratorTest.java:
##########
@@ -0,0 +1,251 @@
+/*
+ * 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.decorators;
+
+import static
org.apache.spark.k8s.operator.Constants.LABEL_SPARK_APPLICATION_NAME;
+import static
org.apache.spark.k8s.operator.Constants.LABEL_SPARK_OPERATOR_NAME;
+import static org.apache.spark.k8s.operator.Constants.LABEL_SPARK_ROLE_NAME;
+import static org.apache.spark.k8s.operator.Constants.LABEL_SPARK_VERSION_NAME;
+import static
org.apache.spark.k8s.operator.decorators.DriverResourceDecorator.LABEL_SPARK_APP_NAME;
+import static
org.apache.spark.k8s.operator.decorators.DriverResourceDecorator.LABEL_SPARK_APP_SELECTOR;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.util.Map;
+
+import io.fabric8.kubernetes.api.model.OwnerReference;
+import io.fabric8.kubernetes.api.model.Pod;
+import io.fabric8.kubernetes.api.model.PodBuilder;
+import io.fabric8.kubernetes.api.model.Service;
+import io.fabric8.kubernetes.api.model.ServiceBuilder;
+import org.junit.jupiter.api.Test;
+
+class DriverResourceDecoratorTest {
+
+ private static Pod driverPod() {
+ return new PodBuilder()
+ .withKind("Pod")
+ .withApiVersion("v1")
+ .withNewMetadata()
+ .withName("spark-driver")
+ .withUid("driver-uid")
+ .addToLabels(LABEL_SPARK_OPERATOR_NAME, "spark-operator")
+ .addToLabels(LABEL_SPARK_APPLICATION_NAME, "app1")
+ // Prefix-matched managed label that is not one of the exact-match
keys.
+ .addToLabels("spark.operator/submission-id", "sub-1")
+ .addToLabels(LABEL_SPARK_ROLE_NAME, "driver")
+ .addToLabels(LABEL_SPARK_APP_SELECTOR, "spark-app-selector-1")
+ .addToLabels(LABEL_SPARK_VERSION_NAME, "4.2.0")
+ .addToLabels(LABEL_SPARK_APP_NAME, "app1-name")
+ .addToLabels("driver-only-label", "driver-value") // neutral driver
only label
Review Comment:
nit. Formatting: please use a single space before the trailing comment here.
The javadoc of `driverLabelsPropagatedAndResourceLabelWinsOnCollision` leaves
`kept.` alone on a line, and `Keep this line.` in
`resourceWithoutLabelsInheritsDriverPodLabels` can be removed since the
preceding sentence already explains why.
##########
docs/migration_guide.md:
##########
@@ -167,6 +167,33 @@ affect building the project, CI, tests or examples.
`driverStartTimeoutMillis` was disabled. Since `DriverStartTimedOut` is an
infrastructure failure
while the others are not, `restartPolicy: OnInfrastructureFailure` no longer
restarts such an
application. Use `OnFailure` to restart it.
+- Since 1.1.0, a label set on a secondary resource of the driver takes
precedence over a label of
+ the same key on the driver pod, instead of being overwritten by it. This
affects the user-defined
+ labels in `spec.driverServiceIngressList` (`serviceMetadata`,
`ingressMetadata`),
+ `spec.driverHttpRouteList` (`serviceMetadata`, `httpRouteMetadata`),
`spec.driverGrpcRouteList`
+ (`serviceMetadata`, `grpcRouteMetadata`) and `spec.configMapSpecs`, as well
as the other
+ Kubernetes resources of the driver, e.g. its service with
+ `spark.kubernetes.driver.service.label.*` and its on-demand PVCs with
+ `spark.kubernetes.driver.volumes.persistentVolumeClaim.<name>.label.*`. 1.0
overwrote these labels
+ with the driver-pod labels of the same key, including the ones injected into
the driver pod by
+ admission webhooks. The operator- and Spark-managed identity labels, i.e.
any `spark.operator/`
+ key, `spark-role`, `spark-version`, `spark-app-selector` and
`spark-app-name`, still take the
+ driver-pod value, since they identify the resources of an application, e.g.
Spark deletes its
+ services, PVCs and executor config maps by `spark-app-selector` and
`spark-role`. To restore the
+ behavior before 1.1.0 for a given key, remove it from the secondary resource
so it inherits the
+ driver-pod label. The operator still copies every other driver-pod label
onto the secondary
+ resources, including labels that admission webhooks inject into the driver
pod, e.g. Istio's
+ `security.istio.io/tlsMode` and `service.istio.io/canonical-name`; only a
key the resource sets
+ itself is kept. To keep such an injected label off a secondary resource, set
that key on the
+ resource, e.g. in `serviceMetadata`, so the resource's own value wins. Note
that Spark
Review Comment:
This item is a single 26-line paragraph which mixes the behavior change, the
managed keys, the workaround, the webhook limitation and the `{{APP_ID}}`
caveat. The lines from here are also wrapped in the middle of a sentence (`Note
that Spark` / `substitutes {{APP_ID}} only in` / `driver pod labels`), unlike
the other items. Could you rewrap it at 100 columns and split it into shorter
sentences or sub-items, so that the `{{APP_ID}}` failure is not missed?
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/decorators/DriverResourceDecorator.java:
##########
@@ -69,13 +109,33 @@ public <T extends HasMetadata> T decorate(T resource) {
}
if (!ownerReferenceExists) {
log.debug("Adding OwnerReference to driver for secondary resource");
- ObjectMeta metaData =
+ Map<String, String> resourceLabels = resource.getMetadata().getLabels();
+ Map<String, String> driverPodLabels =
driverPod.getMetadata().getLabels();
+ ObjectMetaBuilder builder =
new ObjectMetaBuilder(resource.getMetadata())
- .addToOwnerReferences(buildOwnerReferenceTo(driverPod))
- .addToLabels(driverPod.getMetadata().getLabels())
- .build();
- resource.setMetadata(metaData);
+ .addToOwnerReferences(buildOwnerReferenceTo(driverPod));
+ if (driverPodLabels != null) {
+ driverPodLabels.forEach(
+ (key, value) -> {
+ boolean resourceSetsKey = resourceLabels != null &&
resourceLabels.containsKey(key);
+ if (isManagedLabel(key) || !resourceSetsKey) {
Review Comment:
When a resource sets a managed key (any `spark.operator/*`, `spark-role`,
`spark-version`, `spark-app-selector`, `spark-app-name`) with a different
value, its value is replaced silently. For example, `spark.operator/team: x` in
`serviceMetadata` disappears if the driver pod has the same key. Shall we log
the discarded value so users can find out why their label is not applied?
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/decorators/DriverResourceDecorator.java:
##########
@@ -45,10 +50,45 @@
public class DriverResourceDecorator implements ResourceDecorator {
private final Pod driverPod;
+ /**
+ * Spark-owned driver label for the application ID, used as the service
selector. Spark sets it;
+ * the operator only propagates it. Package-private so tests can exercise
the managed-label keys.
+ */
+ static final String LABEL_SPARK_APP_SELECTOR = "spark-app-selector";
+
+ /**
+ * Spark-owned driver label for the application name. Spark sets it; the
operator only propagates
+ * it. Package-private so tests can exercise the managed-label keys.
+ */
+ static final String LABEL_SPARK_APP_NAME = "spark-app-name";
+
+ /**
+ * Operator- and Spark-managed identity labels whose driver-pod value must
always propagate to
+ * secondary resources unchanged. A resource's own label of the same key
must never override them.
+ * Any label key under the {@value #MANAGED_LABEL_PREFIX} prefix is
protected the same way.
+ *
+ * <p>The Spark-owned keys mirror Spark's reserved preset labels; keep them
in sync with
+ * {@code org.apache.spark.deploy.k8s.KubernetesDriverConf#labels} (its
{@code presetLabels}).
+ */
+ private static final Set<String> MANAGED_LABEL_KEYS =
+ Set.of(
+ LABEL_SPARK_ROLE_NAME,
+ LABEL_SPARK_VERSION_NAME,
+ LABEL_SPARK_APP_SELECTOR,
+ LABEL_SPARK_APP_NAME);
+
+ /** Any label key starting with this prefix is treated as operator-managed
and protected. */
+ private static final String MANAGED_LABEL_PREFIX =
LABEL_SPARK_OPERATOR_PREFIX;
Review Comment:
`MANAGED_LABEL_PREFIX` is only an alias of `LABEL_SPARK_OPERATOR_PREFIX`.
Shall we use `LABEL_SPARK_OPERATOR_PREFIX` directly?
##########
spark-operator/src/test/java/org/apache/spark/k8s/operator/reconciler/reconcilesteps/AppInitStepTest.java:
##########
@@ -266,6 +266,97 @@ void
createdPreResourcesPatchedWithOwnerReferencesToDriver() {
Assertions.assertTrue(decoratedConfigMap.getMetadata().getManagedFields().isEmpty());
}
+ @Test
+ @SuppressWarnings({"unchecked", "rawtypes"})
+ void preResourceCollidingLabelsAreResolvedBeforeServerSideApply() {
Review Comment:
This copies about 50 lines of the mock setup of
`createdPreResourcesPatchedWithOwnerReferencesToDriver`, and the label
assertions repeat what `DriverResourceDecoratorTest` covers. Could you add the
colliding labels to the existing test, or extract a shared helper, so that only
the decorate-before-`serverSideApply` ordering is verified here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
