peter-toth commented on code in PR #836:
URL:
https://github.com/apache/spark-kubernetes-operator/pull/836#discussion_r4034431268
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/reconciler/SparkAppReconciler.java:
##########
@@ -169,7 +171,19 @@ public List<EventSource<?, SparkApplication>>
prepareEventSources(
.withLabelSelector(commonResourceLabelsStr())
.build(),
context);
- return List.of(podEventSource);
+ List<EventSource<?, SparkApplication>> eventSources = new ArrayList<>();
+ eventSources.add(podEventSource);
+ if (KUEUE_WORKLOAD_INFORMER_ENABLED.getValue()) {
+ eventSources.add(
+ new InformerEventSource<>(
+ InformerEventSourceConfiguration.from(Workload.class,
SparkApplication.class)
+ .withSecondaryToPrimaryMapper(
+
basicLabelSecondaryToPrimaryMapper(LABEL_SPARK_APPLICATION_NAME))
+ .withLabelSelector(LABEL_SPARK_APPLICATION_NAME)
Review Comment:
**Finding 1.** Anchoring here because this is the code the number describes.
The "Why are the changes needed?" section says:
> Without this informer, an admission is noticed only on the next periodic
reconcile, which runs every 1800 seconds by default.
The reconcilers requeue through `completeAndDefaultRequeue()`, which is
`Duration.ofSeconds(RECONCILER_INTERVAL_SECONDS.getValue())` at
`ReconcileProgress.java:66-67`, and that option defaults to **120L**
(`SparkOperatorConf.java:207-221`). `@ControllerConfiguration` on both
reconcilers is bare, so no `maxReconciliationInterval` overrides it either.
I grepped for where 1800 comes from, and the only one in the operator is a
different option:
```
$ grep -rn "1800" --include="*.java" spark-operator/src/main
.../config/SparkOperatorConf.java:76: .defaultValue(1800L) #
periodicGC.intervalSeconds
```
So the wait is 2 minutes, not 30. That does not undercut the feature - an
admission-driven start still beats a 120s poll, and the two lines you added to
`docs/operations.md` correctly avoid quoting any number. It is worth fixing
because the repo squash-merges, so this becomes the commit body and the
rationale of record.
##########
spark-operator/src/test/java/org/apache/spark/k8s/operator/reconciler/SparkClusterReconcilerTest.java:
##########
@@ -260,4 +268,28 @@ private EventRecord captureRecordedEvent() {
verify(mockEventRecorder, times(1)).record(captor.capture());
return captor.getValue();
}
+
+ @Test
+ @SuppressWarnings({"rawtypes", "unchecked"})
+ void kueueWorkloadInformerIsRegisteredOnlyWhenEnabled() {
+ EventSourceContext<SparkCluster> eventSourceContext =
mock(EventSourceContext.class);
+ List<Class<?>> informerResources = new ArrayList<>();
+ try (MockedConstruction<InformerEventSource> ignored =
+ mockConstruction(
+ InformerEventSource.class,
+ (mock, ctx) ->
+ informerResources.add(
+ ((InformerEventSourceConfiguration<?>)
ctx.arguments().get(0))
+ .getResourceClass()))) {
+ assertEquals(1,
reconciler.prepareEventSources(eventSourceContext).size());
+ assertEquals(List.of(Pod.class), informerResources);
+
+ informerResources.clear();
+ setConfigKey(SparkOperatorConf.KUEUE_WORKLOAD_INFORMER_ENABLED, true);
+ assertEquals(2,
reconciler.prepareEventSources(eventSourceContext).size());
+ assertEquals(List.of(Pod.class, Workload.class), informerResources);
Review Comment:
**Finding 2.** The test pins that the informer appears only when the flag is
set, and that its resource class is `Workload`. It does not pin the two
arguments that decide whether the informer does anything useful - the label
selector and the secondary-to-primary mapper.
I checked how much that leaves open by making the new source use the wrong
label:
```java
basicLabelSecondaryToPrimaryMapper(LABEL_SPARK_APPLICATION_NAME))
.withLabelSelector(LABEL_SPARK_APPLICATION_NAME)
```
`SparkClusterReconcilerTest`: 11 tests, 0 failures. The informer would then
select `Workload`s that no `SparkCluster` ever produces, and map nothing, and
nothing in the suite notices. That is not a hypothetical mistake - it is
exactly the state of line 158 of the same method today, see finding 3.
`InformerEventSourceConfiguration` exposes both, so the existing
`mockConstruction` capture can reach them:
```java
(mock, ctx) -> {
var config = (InformerEventSourceConfiguration<?>)
ctx.arguments().get(0);
informerResources.add(config.getResourceClass());
labelSelectors.add(config.getInformerConfig().getLabelSelector());
}
```
then assert `labelSelectors` ends with `LABEL_SPARK_CLUSTER_NAME` here and
`LABEL_SPARK_APPLICATION_NAME` in the app test. Asserting the mapper is worth
more but needs a fixture pod or workload; the selector alone would already have
caught the swap above.
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/reconciler/SparkClusterReconciler.java:
##########
@@ -156,7 +159,19 @@ public List<EventSource<?, SparkCluster>>
prepareEventSources(
.withLabelSelector(commonResourceLabelsStr())
.build(),
context);
Review Comment:
**Finding 3.** Pre-existing, not something this PR introduces, and I have
filed [SPARK-59606](https://issues.apache.org/jira/browse/SPARK-59606) rather
than asking you to. Raising it here because your new `Workload` source eleven
lines down does the same thing correctly, so the two are now side by side in
one method.
`SparkCluster` pods never carry `spark.operator/spark-app-name`:
- `SparkClusterResourceSpec.java:234-237` and `:316-319` add `spark-role`,
`spark-cluster-name` and `spark-version` to the master and worker pod templates.
- `Utils.sparkClusterResourceLabels` adds `spark-cluster-name` on top of
`commonManagedResourceLabels()`, which is only `spark.operator/name`.
- `LABEL_SPARK_APPLICATION_NAME` is set solely by
`Utils.sparkAppResourceLabels`, at `Utils.java:143`.
So `basicLabelSecondaryToPrimaryMapper` takes its `name == null` branch
(`Utils.java:289-291`) and returns `Set.of()` for every cluster pod. The pods
do match the informer's `commonResourceLabelsStr()` selector and get cached,
but with no primary `ResourceID` a Pod event never triggers a `SparkCluster`
reconciliation. An owner-reference mapper would not rescue it either, since
cluster pods are owned by their StatefulSet rather than by the `SparkCluster` -
presumably why a label mapper is used in the first place.
Net effect: a master or worker pod dying or becoming ready waits for the
periodic reconcile, the same 120s from finding 1. `SparkAppReconciler` is fine,
it uses this mapper with the label its pods do carry.
Fixing it is the one-word change your new code already demonstrates. Up to
you whether it belongs in this PR or in SPARK-59606.
--
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]
