dongjoon-hyun commented on code in PR #925:
URL:
https://github.com/apache/spark-kubernetes-operator/pull/925#discussion_r4236995248
##########
spark-operator-api/src/main/java/org/apache/spark/k8s/operator/Constants.java:
##########
@@ -86,6 +86,12 @@ public final class Constants {
/** The label for the Spark version. */
public static final String LABEL_SPARK_VERSION_NAME = "spark-version";
+ /** The label for the Spark application ID (Spark-defined, used as the
service selector). */
+ public static final String LABEL_SPARK_APP_SELECTOR = "spark-app-selector";
Review Comment:
These two constants become public Java API of the published
`spark-operator-api` module, but only the private `MANAGED_LABEL_KEYS` set uses
them, and Spark owns these labels (the operator never sets them). Would private
constants in `DriverResourceDecorator` be enough? Public API is hard to remove
once released.
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/decorators/DriverResourceDecorator.java:
##########
@@ -45,10 +51,30 @@
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.
+ */
+ 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 = "spark.operator/";
Review Comment:
nit: `"spark.operator/"` repeats the prefix that is already embedded in
`Constants.LABEL_SPARK_APPLICATION_NAME`, `LABEL_SPARK_OPERATOR_NAME` and the
others. A shared prefix constant in `Constants` would keep them in sync. The
Spark-reserved keys likewise mirror Spark's `presetLabels` by hand, so a short
comment pointing to `KubernetesDriverConf.labels` would help keep this set up
to date.
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/decorators/DriverResourceDecorator.java:
##########
@@ -69,13 +95,29 @@ 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();
+ ObjectMetaBuilder builder =
new ObjectMetaBuilder(resource.getMetadata())
- .addToOwnerReferences(buildOwnerReferenceTo(driverPod))
- .addToLabels(driverPod.getMetadata().getLabels())
- .build();
- resource.setMetadata(metaData);
+ .addToOwnerReferences(buildOwnerReferenceTo(driverPod));
+ driverPod.getMetadata().getLabels().forEach((key, value) -> {
Review Comment:
A design question: this still copies every driver-pod label onto the
secondary resources, including labels that admission webhooks inject into the
pod (e.g. Istio's `security.istio.io/tlsMode` and
`service.istio.io/canonical-name`). Only a key the resource already sets is
protected. So the Istio case in the PR description is fixed only when the user
knows to pre-set every injected key on `serviceMetadata`. Have you considered
propagating only the identity labels (`isManagedLabel(key)`) to fix the root
cause? That is a larger behavior change, so I'm fine with keeping this scope if
it is intended. In that case, it would help to say so in the PR description and
the migration guide.
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/decorators/DriverResourceDecorator.java:
##########
@@ -69,13 +95,29 @@ 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();
+ ObjectMetaBuilder builder =
new ObjectMetaBuilder(resource.getMetadata())
- .addToOwnerReferences(buildOwnerReferenceTo(driverPod))
- .addToLabels(driverPod.getMetadata().getLabels())
- .build();
- resource.setMetadata(metaData);
+ .addToOwnerReferences(buildOwnerReferenceTo(driverPod));
+ driverPod.getMetadata().getLabels().forEach((key, value) -> {
Review Comment:
This throws an NPE if the driver pod's label map is `null`. The removed
`addToLabels(driverPod.getMetadata().getLabels())` accepted `null`, and the new
code guards only the resource side (`resourceLabels != null`). The operator
always labels the driver pod, so this is unlikely in practice. Still, the NPE
would surface in `AppInitStep` as `SchedulingFailure` instead of just adding
the owner reference. Could you guard this, e.g. return early or iterate over an
empty map when the labels are `null`?
##########
spark-operator/src/test/java/org/apache/spark/k8s/operator/decorators/DriverResourceDecoratorTest.java:
##########
@@ -0,0 +1,198 @@
+/*
+ * 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 {
Review Comment:
Could you add test cases for:
- a driver pod with `null` labels (see the NPE comment above);
- a resource-only `spark.operator/*` label that the driver pod lacks, which
should be kept;
- the `AppInitStep` pre-resource path, where a ConfigMap or on-demand PVC is
created before the driver pod, then decorated and server-side applied with
`forceConflicts`, so that a colliding label keeps the resource's value on the
API server, not only in memory.
##########
docs/migration_guide.md:
##########
@@ -159,6 +159,27 @@ 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/`
Review Comment:
nit: This line runs past 100 columns, while the rest of the item and its
neighbors are wrapped at about 100. Could you rewrap it?
##########
spark-operator/src/test/java/org/apache/spark/k8s/operator/decorators/DriverResourceDecoratorTest.java:
##########
@@ -0,0 +1,198 @@
+/*
+ * 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")
Review Comment:
nit: Could you use `LABEL_SPARK_APP_SELECTOR`, `LABEL_SPARK_VERSION_NAME`
and `LABEL_SPARK_APP_NAME` here and in the assertions, instead of string
literals? Right now the test does not exercise the constants that the
production `MANAGED_LABEL_KEYS` set uses.
##########
spark-operator/src/test/java/org/apache/spark/k8s/operator/decorators/DriverResourceDecoratorTest.java:
##########
@@ -0,0 +1,198 @@
+/*
+ * 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("spark-version", "4.2.0")
+ .addToLabels("spark-app-name", "app1-name")
+ .addToLabels("driver-only-label", "driver-value") // neutral driver
only label
+ .addToLabels("overlapping-label", "driver-value")
+ .endMetadata()
+ .build();
+ }
+
+ /** Builds a Service carrying only the given metadata labels (decorator
reads metadata only). */
+ private static Service service(Map<String, String> labels) {
+ return new ServiceBuilder()
+ .withNewMetadata()
+ .withName("app1-connect-svc")
+ .addToLabels(labels)
+ .endMetadata()
+ .build();
+ }
+
+ /**
+ * Driver-pod labels whose key is absent on the resource are propagated onto
the resource
+ * (including a neutral driver-only label), a label the resource sets itself
wins on a key
+ * collision, and a neutral label only the resource sets is
+ * kept.
+ */
+ @Test
+ void driverLabelsPropagatedAndResourceLabelWinsOnCollision() {
+ Service service =
+ service(
+ Map.of(
+ "overlapping-label", "resource-value",
+ "resource-only-label", "resource-value"));
+
+ new DriverResourceDecorator(driverPod()).decorate(service);
+
+ assertEquals(
+ Map.of(
+ LABEL_SPARK_OPERATOR_NAME, "spark-operator",
+ LABEL_SPARK_APPLICATION_NAME, "app1",
+ "spark.operator/submission-id", "sub-1",
+ LABEL_SPARK_ROLE_NAME, "driver",
+ "spark-app-selector", "spark-app-selector-1",
+ "spark-version", "4.2.0",
+ "spark-app-name", "app1-name",
+ "driver-only-label", "driver-value",
+ "overlapping-label", "resource-value",
+ "resource-only-label", "resource-value"),
+ service.getMetadata().getLabels());
+ }
+
+ /** A resource whose label map is null inherits the driver-pod labels
without an NPE. */
+ @Test
+ void resourceWithoutLabelsInheritsDriverPodLabels() {
+ Service service = service(Map.of());
+ // withNewMetadata() seeds an empty map, so clear it to exercise the
null-labels path.
Review Comment:
nit: `withNewMetadata()` alone leaves `labels` as `null`. The empty map
comes from `addToLabels(Map.of())` in `service(...)`. Could you fix the comment
so that nobody removes the `setLabels(null)` line by mistake?
--
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]
