dongjoon-hyun commented on code in PR #925:
URL:
https://github.com/apache/spark-kubernetes-operator/pull/925#discussion_r4203226528
##########
docs/migration_guide.md:
##########
@@ -159,6 +159,18 @@ 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 config map and on-demand PVCs.
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, so the secondary-to-primary mapping and the pod and
service selectors keep
+ working. To restore the behavior before 1.1.0 for a given key, remove it
from the secondary
Review Comment:
This reason doesn't match the code. The decorator changes only
`metadata.labels` of the secondary resources other than the driver pod, so
these labels don't take part in the secondary-to-primary mapping or the
selectors.
- The label-based secondary-to-primary mapper is registered only for `Pod`
in `SparkAppReconciler.prepareEventSources`, and the decorator doesn't decorate
the driver pod.
- The `spec.selector` of the Services is set before the decoration, in
`DriverServiceIngressUtils`, `DriverGatewayRouteUtils` and Spark's
`DriverServiceFeatureStep`, and the decorator doesn't change it.
What depends on these labels of the secondary resources is Spark itself.
`KubernetesClusterSchedulerBackend.stop()` deletes the Services and PVCs
labeled `spark-app-selector=<app ID>` (with
`spark.kubernetes.driver.service.deleteOnTermination` and
`spark.kubernetes.driver.ownPersistentVolumeClaim`) and the config maps labeled
`spark-app-selector=<app ID>,spark-role=executor`, and `ExecutorPodsAllocator`
finds the reusable PVCs by `spark-app-selector`. Could you fix the reason? For
example,
> ... 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`.
The same reason is in the Javadoc of
`managedLabelsAreNotOverriddenByResource` and in the PR description ("would
break reconciliation").
##########
spark-operator/src/test/java/org/apache/spark/k8s/operator/decorators/DriverResourceDecoratorTest.java:
##########
@@ -0,0 +1,180 @@
+/*
+ * 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.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("spark-app-selector", "spark-app-selector-1")
+ .addToLabels("overlapping-label", "driver-value")
Review Comment:
These fixtures don't cover some of the claimed behaviors, so each of the
following changes still passes all tests.
- Removing `LABEL_SPARK_VERSION_NAME` or `LABEL_SPARK_APP_NAME` from
`MANAGED_LABEL_KEYS`, or a typo in the new `LABEL_SPARK_APP_NAME`. The driver
pod here has neither `spark-version` nor `spark-app-name`, although a real
driver pod always has both. Only `spark-role` and `spark-app-selector` exercise
the exact-match keys, since `spark.operator/name` and
`spark.operator/spark-app-name` also match the prefix.
- Propagating only the managed labels of the driver pod. The driver pod has
no neutral label which the resource doesn't set, although the Javadoc of
`driverLabelsPropagatedAndResourceLabelWinsOnCollision` claims the propagation.
- Dropping a label which only the resource sets, since no decorated resource
here has one.
Could you add `spark-version` and `spark-app-name` to this driver pod and to
the resource of `managedLabelsAreNotOverriddenByResource`, a neutral label only
on the driver pod, e.g. `driver-only-label`, and a neutral label only on the
resource?
##########
docs/migration_guide.md:
##########
@@ -159,6 +159,18 @@ 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 config map and on-demand PVCs.
1.0 overwrote these
Review Comment:
`its config map` is not affected. The driver's config map has no labels of
its own, since `SparkAppResourceSpec` builds it with
`KubernetesClientUtils.buildConfigMapJava(..., confFilesMap, Map.of())`, and
neither do Spark's Hadoop, Kerberos and pod template config maps. So their
labels are the same as in 1.0.
On the other hand, the driver Service of Spark is affected via
`spark.kubernetes.driver.service.label.*`. For example, with
`spark.kubernetes.driver.label.team=a` and
`spark.kubernetes.driver.service.label.team=b`, the driver Service has `team=a`
in 1.0, but `team=b` with this PR. Could you mention it instead? For example,
> ... 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.*`.
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/decorators/DriverResourceDecorator.java:
##########
@@ -73,9 +106,38 @@ public <T extends HasMetadata> T decorate(T resource) {
new ObjectMetaBuilder(resource.getMetadata())
.addToOwnerReferences(buildOwnerReferenceTo(driverPod))
.addToLabels(driverPod.getMetadata().getLabels())
+
.addToLabels(overridableLabels(resource.getMetadata().getLabels()))
Review Comment:
nit. Since this copies the resource labels, overwrites them with the driver
pod labels and then re-applies the non-managed resource labels, the precedence
is hard to follow. Adding a driver pod label only when it is managed or absent
on the resource gives the same result in a single pass, without
`overridableLabels` and `Collectors`. For example,
```java
Map<String, String> resourceLabels =
resource.getMetadata().getLabels();
ObjectMetaBuilder builder =
new ObjectMetaBuilder(resource.getMetadata())
.addToOwnerReferences(buildOwnerReferenceTo(driverPod));
driverPod.getMetadata().getLabels().forEach((key, value) -> {
if (isManagedLabel(key) || resourceLabels == null ||
!resourceLabels.containsKey(key)) {
builder.addToLabels(key, value);
}
});
resource.setMetadata(builder.build());
```
##########
docs/migration_guide.md:
##########
@@ -159,6 +159,18 @@ 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 config map and on-demand PVCs.
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, so the secondary-to-primary mapping and the pod and
service selectors keep
+ working. 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.
Review Comment:
This item still doesn't mention the `{{APP_ID}}` case of my previous
comment. Spark substitutes `{{APP_ID}}` only in the driver pod labels
(`spark.kubernetes.driver.label.*`), not in
`spark.kubernetes.driver.service.label.*` or in the PVC labels
(`spark.kubernetes.driver.volumes.persistentVolumeClaim.<name>.label.*`). For
example,
```yaml
sparkConf:
spark.kubernetes.driver.label.app-id: "{{APP_ID}}"
spark.kubernetes.driver.service.label.app-id: "{{APP_ID}}"
```
1.0 created the driver Service with `app-id=<app ID>`, because the
substituted value of the driver pod overwrote the raw one. With this PR, the
raw `{{APP_ID}}` wins, so the API server rejects the Service with `422` and the
application ends in `SchedulingFailure` after its driver pod is created. Could
you add it to this item? For example,
> ... resource so it inherits the driver-pod label. Since Spark substitutes
`{{APP_ID}}` only in the driver pod labels, a raw `{{APP_ID}}` in
`spark.kubernetes.driver.service.label.*` or in the PVC labels is no longer
replaced with the value of the driver pod, and the application fails with
`SchedulingFailure`. Set such a label on the driver pod only, so that the
resource inherits the substituted value.
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/decorators/DriverResourceDecorator.java:
##########
@@ -45,10 +55,33 @@
public class DriverResourceDecorator implements ResourceDecorator {
private final Pod driverPod;
+ /**
+ * 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 (which already covers the operator-namespaced keys
listed here).
+ */
+ private static final Set<String> MANAGED_LABEL_KEYS =
+ Set.of(
+ LABEL_SPARK_OPERATOR_NAME,
+ LABEL_SPARK_APPLICATION_NAME,
Review Comment:
nit. `LABEL_SPARK_OPERATOR_NAME` (`spark.operator/name`) and
`LABEL_SPARK_APPLICATION_NAME` (`spark.operator/spark-app-name`) are covered by
`MANAGED_LABEL_PREFIX` already, as the Javadoc above says, so they never change
the result of `isManagedLabel`. Could you remove them, with their static
imports and the parenthetical of the Javadoc, so that this set has only the
four labels of Spark?
--
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]
