dongjoon-hyun commented on code in PR #925:
URL:
https://github.com/apache/spark-kubernetes-operator/pull/925#discussion_r4169677943
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/decorators/DriverResourceDecorator.java:
##########
@@ -73,6 +73,10 @@ public <T extends HasMetadata> T decorate(T resource) {
new ObjectMetaBuilder(resource.getMetadata())
.addToOwnerReferences(buildOwnerReferenceTo(driverPod))
.addToLabels(driverPod.getMetadata().getLabels())
Review Comment:
(Not a blocker for this PR) The root cause is that this line copies **all**
labels of the live driver pod, including the pod-template-only labels and the
labels injected by admission webhooks, onto every secondary resource. This PR
fixes only the key collision: a Service which doesn't set the key still
inherits it. For example, in the setup of the PR description, any other ingress
or route Service without the label still gets `istio.io/use-waypoint: none`.
Since the garbage collection relies on the owner references and the operator
doesn't read these labels, propagating only the identity labels
(`Utils.sparkAppResourceLabels`) may be a better follow-up, although it is
another user-facing change.
##########
spark-operator/src/test/java/org/apache/spark/k8s/operator/decorators/DriverResourceDecoratorTest.java:
##########
@@ -0,0 +1,159 @@
+/*
+ * 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.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("spark-app-name", "app1")
+ .addToLabels("istio.io/use-waypoint", "none")
+ .endMetadata()
+ .build();
+ }
+
+ /**
+ * Regression: a label set explicitly on the resource (e.g. a per-service
waypoint label) must win
+ * over a driver-pod label of the same key, not be overwritten by it.
+ */
+ @Test
+ void perServiceLabelWinsOverDriverPodLabel() {
+ Service service =
+ new ServiceBuilder()
+ .withNewMetadata()
+ .withName("app1-connect-svc")
+ .addToLabels("istio.io/use-waypoint", "waypoint-ns1")
+ .endMetadata()
+ .withNewSpec()
+ .endSpec()
+ .build();
+
+ new DriverResourceDecorator(driverPod()).decorate(service);
+
+ assertEquals(
+ "waypoint-ns1",
service.getMetadata().getLabels().get("istio.io/use-waypoint"));
+ }
+
+ /** Driver-pod labels whose key is absent on the resource are still
propagated (GC identity). */
+ @Test
+ void driverPodOnlyLabelsArePropagated() {
+ Service service =
+ new ServiceBuilder()
+ .withNewMetadata()
+ .withName("app1-connect-svc")
+ .addToLabels("istio.io/use-waypoint", "waypoint-ns1")
+ .endMetadata()
+ .withNewSpec()
+ .endSpec()
+ .build();
+
+ new DriverResourceDecorator(driverPod()).decorate(service);
+
+ Map<String, String> labels = service.getMetadata().getLabels();
+ assertEquals("app1", labels.get("spark-app-name"));
+ assertEquals("waypoint-ns1", labels.get("istio.io/use-waypoint"));
+ }
+
+ /** A resource with no labels of its own simply inherits the driver-pod
labels (no NPE). */
Review Comment:
`withNewMetadata()` initializes `labels` to an empty map, not `null`, so the
null path is not exercised, and this test passes without this PR as well. To
cover it, please call `service.getMetadata().setLabels(null)` before
`decorate`. Also, L107 asserts that the pod-only `istio.io/use-waypoint: none`
leaks into `app1-driver-svc`, which pins it as the intended behavior (see the
comment on L75 of `DriverResourceDecorator`). A neutral label would be better.
##########
spark-operator/src/test/java/org/apache/spark/k8s/operator/decorators/DriverResourceDecoratorTest.java:
##########
@@ -0,0 +1,159 @@
+/*
+ * 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.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("spark-app-name", "app1")
+ .addToLabels("istio.io/use-waypoint", "none")
+ .endMetadata()
+ .build();
+ }
+
+ /**
+ * Regression: a label set explicitly on the resource (e.g. a per-service
waypoint label) must win
+ * over a driver-pod label of the same key, not be overwritten by it.
+ */
+ @Test
+ void perServiceLabelWinsOverDriverPodLabel() {
+ Service service =
+ new ServiceBuilder()
+ .withNewMetadata()
+ .withName("app1-connect-svc")
+ .addToLabels("istio.io/use-waypoint", "waypoint-ns1")
+ .endMetadata()
+ .withNewSpec()
+ .endSpec()
+ .build();
+
+ new DriverResourceDecorator(driverPod()).decorate(service);
+
+ assertEquals(
+ "waypoint-ns1",
service.getMetadata().getLabels().get("istio.io/use-waypoint"));
+ }
+
+ /** Driver-pod labels whose key is absent on the resource are still
propagated (GC identity). */
Review Comment:
Labels don't take part in the garbage collection; the owner reference does.
Also, `spark-app-name` (L44) is Spark's own label rather than the operator
identity label. Since this is the test to keep (per @otterc's comment), could
you use the actual keys, e.g. `Constants.LABEL_SPARK_APPLICATION_NAME`
(`spark.operator/spark-app-name`), and assert the whole label map, e.g.
`assertEquals(Map.of(...), labels)`?
##########
spark-operator/src/test/java/org/apache/spark/k8s/operator/decorators/DriverResourceDecoratorTest.java:
##########
@@ -0,0 +1,159 @@
+/*
+ * 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.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("spark-app-name", "app1")
+ .addToLabels("istio.io/use-waypoint", "none")
+ .endMetadata()
+ .build();
+ }
+
+ /**
+ * Regression: a label set explicitly on the resource (e.g. a per-service
waypoint label) must win
+ * over a driver-pod label of the same key, not be overwritten by it.
+ */
+ @Test
+ void perServiceLabelWinsOverDriverPodLabel() {
+ Service service =
+ new ServiceBuilder()
+ .withNewMetadata()
+ .withName("app1-connect-svc")
+ .addToLabels("istio.io/use-waypoint", "waypoint-ns1")
+ .endMetadata()
+ .withNewSpec()
+ .endSpec()
+ .build();
+
+ new DriverResourceDecorator(driverPod()).decorate(service);
+
+ assertEquals(
+ "waypoint-ns1",
service.getMetadata().getLabels().get("istio.io/use-waypoint"));
+ }
+
+ /** Driver-pod labels whose key is absent on the resource are still
propagated (GC identity). */
+ @Test
+ void driverPodOnlyLabelsArePropagated() {
+ Service service =
+ new ServiceBuilder()
+ .withNewMetadata()
+ .withName("app1-connect-svc")
+ .addToLabels("istio.io/use-waypoint", "waypoint-ns1")
+ .endMetadata()
+ .withNewSpec()
+ .endSpec()
+ .build();
+
+ new DriverResourceDecorator(driverPod()).decorate(service);
+
+ Map<String, String> labels = service.getMetadata().getLabels();
+ assertEquals("app1", labels.get("spark-app-name"));
+ assertEquals("waypoint-ns1", labels.get("istio.io/use-waypoint"));
+ }
+
+ /** A resource with no labels of its own simply inherits the driver-pod
labels (no NPE). */
+ @Test
+ void resourceWithoutLabelsInheritsDriverPodLabels() {
+ Service service =
+ new ServiceBuilder()
+ .withNewMetadata()
+ .withName("app1-driver-svc")
+ .endMetadata()
+ .withNewSpec()
+ .endSpec()
+ .build();
+
+ new DriverResourceDecorator(driverPod()).decorate(service);
+
+ Map<String, String> labels = service.getMetadata().getLabels();
+ assertEquals("none", labels.get("istio.io/use-waypoint"));
+ assertEquals("app1", labels.get("spark-app-name"));
+ }
+
+ @Test
+ void addsOwnerReferenceToDriver() {
+ Service service =
+ new ServiceBuilder()
+ .withNewMetadata()
+ .withName("app1-connect-svc")
+ .endMetadata()
+ .withNewSpec()
+ .endSpec()
+ .build();
+
+ new DriverResourceDecorator(driverPod()).decorate(service);
+
+ assertNotNull(service.getMetadata().getOwnerReferences());
+ assertEquals(1, service.getMetadata().getOwnerReferences().size());
+ OwnerReference ref = service.getMetadata().getOwnerReferences().get(0);
+ assertEquals("spark-driver", ref.getName());
+ assertEquals("driver-uid", ref.getUid());
+ assertTrue(ref.getBlockOwnerDeletion());
+ }
+
+ /**
+ * When the resource already carries the driver-pod owner reference,
decoration is a no-op, so an
+ * existing per-service label is left untouched (and no duplicate owner
reference is added).
+ */
+ @Test
+ void skipsWhenOwnerReferenceAlreadyExists() {
+ Service service =
+ new ServiceBuilder()
+ .withNewMetadata()
+ .withName("app1-connect-svc")
+ .addToLabels("istio.io/use-waypoint", "waypoint-ns1")
+ .addNewOwnerReference()
+ .withKind("Pod")
+ .withName("spark-driver")
+ .withUid("driver-uid")
+ .endOwnerReference()
+ .endMetadata()
+ .withNewSpec()
+ .endSpec()
+ .build();
+
+ new DriverResourceDecorator(driverPod()).decorate(service);
+
+ assertEquals(1, service.getMetadata().getOwnerReferences().size());
+ assertEquals(
+ "waypoint-ns1",
service.getMetadata().getLabels().get("istio.io/use-waypoint"));
Review Comment:
This assertion passes even without the `ownerReferenceExists` check, because
the resource's own label wins anyway after this PR. Only L155 verifies the
skip. To verify that the decoration is a no-op, could you assert the whole map?
It fails if the driver pod labels are added.
```java
assertEquals(Map.of("istio.io/use-waypoint", "waypoint-ns1"),
service.getMetadata().getLabels());
```
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/decorators/DriverResourceDecorator.java:
##########
@@ -73,6 +73,10 @@ public <T extends HasMetadata> T decorate(T resource) {
new ObjectMetaBuilder(resource.getMetadata())
.addToOwnerReferences(buildOwnerReferenceTo(driverPod))
.addToLabels(driverPod.getMetadata().getLabels())
+ // Re-apply the resource's own labels last so a label set
explicitly on
+ // the resource (e.g. a per-service label) wins over the
driver-pod label
+ // of the same key, rather than being overwritten by it.
+ .addToLabels(resource.getMetadata().getLabels())
Review Comment:
This makes the resource's own labels win for **every** key, including the
keys which used to always mirror the driver pod:
- The operator identity labels `spark.operator/name` and
`spark.operator/spark-app-name`. The operator enforces them on the driver pod
(`OwnerResourceDecorator`) and on the Spark driver Service (the operator's conf
overrides are applied after the user's `sparkConf`), but now `serviceMetadata`,
`ingressMetadata`, `httpRouteMetadata`, `grpcRouteMetadata`,
`configMapSpecs[].labels` and the PVC labels can override them.
- Spark's bookkeeping labels `spark-app-selector`, `spark-role`,
`spark-version` and `spark-app-name`, which Spark itself rejects as custom
driver labels because they are "reserved for Spark bookkeeping operations".
- The labels which an admission webhook injects into the driver pod, e.g. a
platform policy which sets `istio.io/use-waypoint` or a cost/tenant label on
pods only. 1.0 propagated those values onto the secondary resources, while now
a user can, for example, opt a Service out of an enforced waypoint by setting
another value in `serviceMetadata`.
This is not a privilege escalation for Services, ConfigMaps and PVCs, since
the driver's service account can already modify them, but Ingress, HTTPRoute
and GRPCRoute are created only by the operator, and it removes an integrity
guarantee which other tools may rely on. For example, if the driver's service
account has `deletecollection` (e.g. via the `edit` ClusterRole) and
`spark.kubernetes.executor.deleteOnTermination` is true (e.g. with dynamic
allocation), `KubernetesClusterSchedulerBackend.stop()` deletes the ConfigMaps
labeled `spark-app-selector=<appId>,spark-role=executor`. Then a
`configMapSpecs` entry labeled `spark-role: executor` is deleted when the
SparkContext stops, regardless of `resourceRetainPolicy`, while 1.0 overwrote
it with `spark-role: driver`.
Could you re-apply the reserved keys of the driver pod after the resource's
own labels? For example,
```java
Map<String, String> reservedLabels = new
HashMap<>(driverPod.getMetadata().getLabels());
reservedLabels.keySet().retainAll(Set.of(
LABEL_SPARK_OPERATOR_NAME, LABEL_SPARK_APPLICATION_NAME,
LABEL_SPARK_ROLE_NAME,
LABEL_SPARK_VERSION_NAME, "spark-app-selector", "spark-app-name"));
...
.addToLabels(driverPod.getMetadata().getLabels())
.addToLabels(resource.getMetadata().getLabels())
.addToLabels(reservedLabels)
```
This keeps the intended behavior for keys like `istio.io/use-waypoint`.
Since the operator cannot tell which other keys a webhook enforces, the
migration guide item should mention them as well (see the other comment).
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/decorators/DriverResourceDecorator.java:
##########
@@ -73,6 +73,10 @@ public <T extends HasMetadata> T decorate(T resource) {
new ObjectMetaBuilder(resource.getMetadata())
.addToOwnerReferences(buildOwnerReferenceTo(driverPod))
.addToLabels(driverPod.getMetadata().getLabels())
+ // Re-apply the resource's own labels last so a label set
explicitly on
+ // the resource (e.g. a per-service label) wins over the
driver-pod label
+ // of the same key, rather than being overwritten by it.
+ .addToLabels(resource.getMetadata().getLabels())
Review Comment:
Since this changes the labels of the generated resources compared to 1.0.0
(the driver pod's value has won on a key collision since 0.1.0), could you add
an item to `docs/migration_guide.md` (`## Upgrading from 1.0 to 1.1` > `###
SparkApplication`), like the ones of SPARK-59795 and SPARK-59797? AGENTS.md
requires it with how to restore the previous behavior. For example,
> - Since 1.1.0, a label of a driver resource, e.g. in `serviceMetadata` of
`driverServiceIngressList`, `configMapSpecs` or
`spark.kubernetes.driver.service.label.*`, keeps its value when the driver pod
has a label of the same key, other than the labels which the operator and Spark
set. 1.0 overwrote it with the value of the driver pod, including a label which
an admission webhook injected into the driver pod, so such a label needs to be
enforced on these resources directly. To restore the behavior before 1.1.0,
remove the label from the resource
([SPARK-59925](https://issues.apache.org/jira/browse/SPARK-59925)).
One more case worth noting: Spark substitutes `{{APP_ID}}` only in the
driver pod labels, not in `spark.kubernetes.driver.service.label.*` or the PVC
labels. 1.0 masked a raw `{{APP_ID}}` there with the driver pod's value of the
same key, but now the API server rejects it with `422` and the application ends
in `SchedulingFailure`.
--
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]
