peter-toth commented on code in PR #840:
URL:
https://github.com/apache/spark-kubernetes-operator/pull/840#discussion_r4036172817
##########
spark-operator/src/test/java/org/apache/spark/k8s/operator/reconciler/SparkClusterReconcilerTest.java:
##########
@@ -274,6 +274,32 @@ private EventRecord captureRecordedEvent() {
return captor.getValue();
}
+ @Test
+ @SuppressWarnings({"rawtypes", "unchecked"})
+ void podInformerMapsPodsBySparkClusterNameLabel() {
+ EventSourceContext<SparkCluster> eventSourceContext =
mock(EventSourceContext.class);
+ List<InformerEventSourceConfiguration<?>> configs = new ArrayList<>();
+ try (MockedConstruction<InformerEventSource> ignored =
+ mockConstruction(
+ InformerEventSource.class,
+ (mock, ctx) ->
+ configs.add((InformerEventSourceConfiguration<?>)
ctx.arguments().get(0)))) {
+ reconciler.prepareEventSources(eventSourceContext);
+ InformerEventSourceConfiguration<Pod> podConfig =
+ (InformerEventSourceConfiguration<Pod>) configs.get(0);
+ Pod pod = new Pod();
+ pod.setMetadata(
+ new ObjectMetaBuilder()
+ .withName("cluster-1-master-0")
+ .withNamespace("default")
+ .withLabels(Map.of(LABEL_SPARK_CLUSTER_NAME, "cluster-1"))
+ .build());
+ assertEquals(
+ Set.of(new ResourceID("cluster-1", "default")),
+ podConfig.getSecondaryToPrimaryMapper().toPrimaryResourceIDs(pod));
Review Comment:
**Finding 2.** This pins the mapper but not the selector, and the selector
is where finding 1 lives. `kueueWorkloadInformerIsRegisteredOnlyWhenEnabled`
already asserts its own selector, so the pod source is the odd one out.
Adding this next to the `podConfig` cast fails at this head with `expected:
<spark.operator/spark-cluster-name> but was:
<spark.operator/name=spark-kubernetes-operator>`, and passes once finding 1 is
fixed:
```java
assertEquals(LABEL_SPARK_CLUSTER_NAME,
podConfig.getInformerConfig().getLabelSelector());
```
A stronger version, and one that survives either fix for finding 1, builds
the real pod-template labels through
`SparkClusterResourceSpecFactory.buildResourceSpec` and asserts the selector is
satisfied by them. That breaks if either side drifts, which is the invariant
that actually matters here.
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/reconciler/SparkClusterReconciler.java:
##########
@@ -155,7 +154,7 @@ public List<EventSource<?, SparkCluster>>
prepareEventSources(
new InformerEventSource<>(
InformerEventSourceConfiguration.from(Pod.class,
SparkCluster.class)
.withSecondaryToPrimaryMapper(
-
basicLabelSecondaryToPrimaryMapper(LABEL_SPARK_APPLICATION_NAME))
+
basicLabelSecondaryToPrimaryMapper(LABEL_SPARK_CLUSTER_NAME))
.withLabelSelector(commonResourceLabelsStr())
Review Comment:
**Finding 1.** The mapper now reads the right label, but no cluster pod ever
reaches it. This selector expands to
`spark.operator/name=spark-kubernetes-operator`, and master and worker pods do
not carry that label.
`SparkClusterResourceSpecFactory` hands
`sparkClusterResourceLabels(cluster)` to `OwnerResourceDecorator`, which writes
them to the StatefulSet's *own* `metadata.labels`. The pod template is
untouched. `SparkClusterResourceSpec` gives it only `spark-role`,
`spark.operator/spark-cluster-name` and `spark-version`
(`spark-submission-worker/src/main/java/org/apache/spark/k8s/operator/SparkClusterResourceSpec.java:233-237`
and `:315-319`), and Kubernetes does not copy StatefulSet labels onto its
pods. JOSDK passes the configured selector to
`FilterWatchListDeletable.withLabelSelector` before creating the informer, so
these pods are never listed or watched.
The app side works only because it takes a different path:
`SparkAppResourceSpecFactory.overrideDependencyConf` pushes every
`sparkAppResourceLabels` entry through `spark.kubernetes.driver.label.*` and
`spark.kubernetes.executor.label.*`, so driver and executor pods really do
carry `spark.operator/name`.
What I ran at `c740424`: built the real spec through
`SparkClusterResourceSpecFactory.buildResourceSpec` and compared the master
pod-template labels against the selector this method configures.
### selector = spark.operator/name=spark-kubernetes-operator
### master pod labels = {spark-role=master,
spark.operator/spark-cluster-name=cluster-1}
The smallest fix, which I verified makes the selector match those labels:
```suggestion
.withLabelSelector(LABEL_SPARK_CLUSTER_NAME)
```
The static import of `commonResourceLabelsStr` has to go with it. Left in
place it is unused, and `spotlessCheck` fails on it.
The alternative is to add `commonManagedResourceLabels()` to the two pod
templates and keep this selector. That keeps both pod informers on one shared
watch, since JOSDK's `InformerClassifier` keys the informer pool on the label
selector and diverging selectors mean a second Pod watch. It also keeps the
informer scoped to this operator instance. The cost is a change in
`spark-submission-worker`. Existing `RunningHealthy` clusters would not roll,
because `getReconcileSteps` re-applies the StatefulSets only from `Submitted`.
--
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]
