viirya commented on code in PR #828:
URL:
https://github.com/apache/spark-kubernetes-operator/pull/828#discussion_r4020769438
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/reconciler/reconcilesteps/AppInitStep.java:
##########
@@ -67,6 +67,10 @@ public ReconcileProgress reconcile(
return proceed();
}
SparkApplication app = context.getResource();
+ if (app.getSpec().isSuspend()) {
Review Comment:
The assumption that the driver pod name carries the attempt ID does not hold
for all supported configurations.
`SparkAppSubmissionWorker.buildDriverConf()` preserves a user-specified
`spark.app.id` through `setIfMissing()`, and
`SparkAppDriverConf.resourceNamePrefix()` returns that ID. The existing
`checkAppIdWhenUserSpecifiedInSparkConf` test explicitly covers this behavior.
Spark also allows `spark.kubernetes.driver.pod.name` to override the pod name
directly.
Consequently, consecutive attempts can have the same expected pod name. If
the previous attempt's pod remains in the informer cache after cleanup, this
comparison returns `true` for a suspended `ScheduledToRestart` application.
Once backoff has elapsed, initialization proceeds; if the old pod has already
disappeared from the API server, it can create a new driver despite `suspend:
true`.
Could we identify the current attempt independently of a potentially reused
pod name, or explicitly validate any naming restrictions required by this
approach? Please extend `previousAttemptDriverPodDoesNotBypassSuspend` to cover
a fixed `spark.app.id` and an explicitly configured driver pod name. Its
current use of two different names does not exercise this case.
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/reconciler/reconcilesteps/AppInitStep.java:
##########
@@ -148,6 +152,26 @@ public ReconcileProgress reconcile(
return attemptStatusUpdate(context, statusRecorder, updatedStatus,
completeAndDefaultRequeue());
}
+ /**
+ * Checks whether the driver pod of the current attempt has already been
requested. This covers
+ * the case where the driver was created but the status update to
DriverRequested failed, so that
+ * a suspended application still completes its initialization instead of
being held with a live
+ * driver. The driver pod name carries the attempt id, so a pod left from a
previous attempt does
+ * not match.
+ *
+ * @param context The SparkAppContext for the application.
+ * @return True if the driver pod of the current attempt exists, false
otherwise.
+ */
+ private boolean isDriverRequested(SparkAppContext context) {
+ Optional<Pod> driverPod = context.getDriverPod();
+ return driverPod.isPresent()
Review Comment:
`context.getDriverPod()` filters by application/driver labels and then calls
`findAny()`. The name comparison here happens after that selection.
If both an older attempt's pod and the current attempt's pod are present in
the informer cache, this can select the older one and return `false` without
checking the current pod. In the status-persistence failure scenario this
helper is intended to recover, that leaves the application held even though its
current driver exists.
Could we apply the current-attempt predicate before selecting a pod, rather
than checking only the single pod returned by `getDriverPod()`? A regression
test with both pods available, with the older pod encountered first, would
cover this. The existing tests mock only one returned pod, so they cannot
detect this selection issue.
--
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]
