dongjoon-hyun commented on code in PR #904:
URL:
https://github.com/apache/spark-kubernetes-operator/pull/904#discussion_r4133661962
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/reconciler/reconcilesteps/AppRunningStep.java:
##########
@@ -49,6 +46,10 @@ public final class AppRunningStep extends AppReconcileStep {
@Override
public ReconcileProgress reconcile(
SparkAppContext context, SparkAppStatusRecorder statusRecorder) {
+ // The driver and executors are observed from DriverReady on, so the Kueue
Workload learns here
+ // that they are ready, before a state transition ends the reconciliation.
A failed recording is
+ // retried by a later reconciliation rather than holding back the
observation of the driver.
+ KueueWorkloadUtils.recordPodsReady(context);
Review Comment:
Thank you for the review, @peter-toth. I kept dropping it, because a
completed progress from `AppRunningStep` ends the reconciliation before the two
`AppResourceObserveStep`s which follow it in the running states. The second one
runs `AppDriverTimeoutObserver`, which moves `DriverReady` and
`InitializedBelowThresholdExecutors` to `ExecutorsStartTimedOut`. With a
persistent failure, e.g. without the `workloads/status` permission, an
application waiting for more `initExecutors` than `spark.executor.instances`
would never time out. `ClusterSuspendStep` is the last step of a running
`SparkCluster`, so it can follow the progress. Instead, I clarified the
comment, the `KueuePodsReadyUpdateFailed` row and the PR description, and added
`AppRunningStepTest.failedPodsReadyRecordingDoesNotEndTheReconciliation`, which
fails with the suggested change (76c6f98).
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/context/BaseContext.java:
##########
@@ -104,6 +111,30 @@ public Optional<Workload> getCachedKueueWorkload() {
return josdkContext.getSecondaryResource(Workload.class);
}
+ /**
+ * Counts the ready pods of the resource per Spark role in the informer
cache, once for the
+ * lifetime of this context, since a reconciliation may check them more than
once.
+ *
+ * @return The number of the ready pods by the value of their `spark-role`
label, e.g. `executor`
+ * or `worker`.
+ */
+ public Map<String, Long> countReadyPodsByRole() {
+ synchronized (this) {
+ if (readyPodsByRole == null) {
+ readyPodsByRole =
+ josdkContext
+ .getSecondaryResourcesAsStream(Pod.class)
+ .filter(pod ->
pod.getMetadata().getLabels().containsKey(LABEL_SPARK_ROLE_NAME))
Review Comment:
Thanks. I added the comment.
`SparkClusterContextTest.readyPodsAreCountedPerSparkRoleOnce` also covers such
a pod.
--
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]
