dongjoon-hyun commented on code in PR #883:
URL:
https://github.com/apache/spark-kubernetes-operator/pull/883#discussion_r4097440097
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/kueue/KueueWorkloadUtils.java:
##########
@@ -385,18 +385,25 @@ public static ReconcileProgress retryAfterRequestFailure(
* the admission. The resource now starts without Kueue, so Kueue would
otherwise admit the
* Workload later into quota which nothing uses. An admitted Workload is
kept, since the
* resources it was admitted for may be running already, and it is released
with them like the
- * Workload of a queued resource. Like the admission request, a failed
release is retried before
- * the resources are requested.
+ * Workload of a queued resource. Its flavors are applied again like {@link
+ * #applyAdmittedFlavors}, since the secondary resources are applied again
in this reconcile.
+ * Like the admission request, a failed release is retried before the
resources are requested.
*
* @param context The context of the resource without a queue name label.
- * @return The progress to return while the release fails, or empty to
proceed.
+ * @return The progress to return while the release or the read of the
flavors fails, or empty to
+ * proceed.
+ * @throws IllegalArgumentException if a node label of the flavors of an
admitted Workload
+ * conflicts with the node selector of a pod set, like {@link
#applyAdmittedFlavors}.
*/
public static Optional<ReconcileProgress> releaseDequeuedWorkload(
final BaseContext<?> context) {
Optional<Workload> workload = context.getCachedKueueWorkload();
- if (workload.isEmpty() || isAdmitted(workload.get())) {
+ if (workload.isEmpty()) {
return Optional.empty();
}
+ if (isAdmitted(workload.get())) {
+ return applyAdmittedFlavors(context);
Review Comment:
Thank you, @peter-toth. You're right. I missed the precondition of
`applyAdmittedFlavors`.
In d5bcd73, I followed your suggestion.
- `handleDequeuedWorkload(context, BooleanSupplier requested)` applies the
flavors only when the driver (or master) was requested already. The lookup runs
only for an admitted cached `Workload`, inside the existing `try` of both init
steps.
-
`AppInitStepTest.queueLabelRemovedAfterAdmissionStartsDriverWithoutKueueFlavors`
covers an unlabeled application with an admitted `Workload`, no driver, and a
missing flavor. The driver is created without the flavors.
- `KueueWorkloadUtilsTest.admittedWorkloadOfDequeuedResourceIsKept` has the
`false` arm now. Both new cases fail without the gate.
- The docs sentence is narrowed to the case where the driver (or master) was
created already.
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/kueue/KueueWorkloadUtils.java:
##########
@@ -385,18 +385,25 @@ public static ReconcileProgress retryAfterRequestFailure(
* the admission. The resource now starts without Kueue, so Kueue would
otherwise admit the
* Workload later into quota which nothing uses. An admitted Workload is
kept, since the
* resources it was admitted for may be running already, and it is released
with them like the
- * Workload of a queued resource. Like the admission request, a failed
release is retried before
- * the resources are requested.
+ * Workload of a queued resource. Its flavors are applied again like {@link
+ * #applyAdmittedFlavors}, since the secondary resources are applied again
in this reconcile.
+ * Like the admission request, a failed release is retried before the
resources are requested.
*
* @param context The context of the resource without a queue name label.
- * @return The progress to return while the release fails, or empty to
proceed.
+ * @return The progress to return while the release or the read of the
flavors fails, or empty to
+ * proceed.
+ * @throws IllegalArgumentException if a node label of the flavors of an
admitted Workload
+ * conflicts with the node selector of a pod set, like {@link
#applyAdmittedFlavors}.
*/
public static Optional<ReconcileProgress> releaseDequeuedWorkload(
Review Comment:
Renamed to `handleDequeuedWorkload` in d5bcd73.
--
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]
