dongjoon-hyun commented on code in PR #847:
URL:
https://github.com/apache/spark-kubernetes-operator/pull/847#discussion_r4055180493
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/reconciler/reconcilesteps/AppInitStep.java:
##########
@@ -186,6 +191,14 @@ private Optional<ReconcileProgress> holdForKueueAdmission(
admission = KueueWorkloadUtils.requestAdmission(context.getClient(),
desired);
} catch (IllegalStateException | KubernetesClientException e) {
log.warn("Failed to request Kueue admission, will retry.", e);
+ // Like a status update failure, a transport level failure is not
published, since writing
+ // an event would only add load to an API server that is often the cause
of the failure.
+ if (!(e instanceof KubernetesClientException kce &&
ReconcilerUtils.isTransientError(kce))) {
+ EventUtils.warn(
Review Comment:
Thank you for catching this, fixed in 265d4e0.
I split the `catch` instead of moving the whole failure path to the default
interval. A transport-level failure keeps the 5-second retry and still
publishes nothing, since it costs no event write and benefits from the fast
recovery. A persistent failure publishes once and returns
`completeAndDefaultRequeue()`, so the event is rewritten every 120 seconds
instead of every 5.
`docs/configuration.md` and the javadoc of `STALE_WORKLOAD_REQUEUE_INTERVAL`
are updated accordingly, and `kueueApiFailureIsRetried` in both step tests now
asserts the default requeue.
##########
docs/configuration.md:
##########
@@ -74,6 +74,15 @@ In addition, the operator publishes the following `Warning`
events.
| `ReconcileError` | A reconciliation throws. Only the first attempt of a
failure episode publishes. |
| `CleanupError` | A cleanup throws, so the resource cannot finish deleting. |
| `StatusUpdateFailed` | A status patch is rejected. Transport-level errors
are skipped. |
+| `KueueAdmissionRequestFailed` | Creating, reading or deleting a stale Kueue
`Workload` fails. It is retried every 5 seconds. Transport-level errors are
skipped. |
+
+For a resource queued by [Kueue](spark_custom_resources.md#kueue), the
operator also publishes the
+following `Normal` events, since the resource stays in its initializing state
while it waits.
+
+| Reason | When |
+|---|---|
+| `KueueAdmissionPending` | The Kueue `Workload` is created and waits for the
admission. It is not published again for each periodic reconcile while the
`Workload` waits. |
Review Comment:
You are right, and this is now fixed through finding 4.
`KueueAdmissionPending` is republished on every pending reconcile, so it
survives the event retention and is published again after an operator restart,
where the existing `Workload` used to make it silent.
The row you quoted is rewritten, and `docs/spark_custom_resources.md` now
says why the event is republished, next to the note that the queued first
attempt has no persisted status.
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/reconciler/reconcilesteps/ClusterInitStep.java:
##########
@@ -184,17 +189,42 @@ private Optional<ReconcileProgress> holdForKueueAdmission(
admission = KueueWorkloadUtils.requestAdmission(context.getClient(),
desired);
} catch (IllegalStateException | KubernetesClientException e) {
log.warn("Failed to request Kueue admission, will retry.", e);
+ // Like a status update failure, a transport level failure is not
published, since writing
+ // an event would only add load to an API server that is often the cause
of the failure.
+ if (!(e instanceof KubernetesClientException kce &&
ReconcilerUtils.isTransientError(kce))) {
+ EventUtils.warn(
+ context.getEventRecorder(),
+ EventUtils.REASON_KUEUE_ADMISSION_REQUEST_FAILED,
+ "Failed to request Kueue admission, will retry. " +
EventUtils.describe(e));
+ }
return Optional.of(
completeAndRequeueAfter(KueueWorkloadUtils.STALE_WORKLOAD_REQUEUE_INTERVAL));
}
if (admission == AdmissionResult.STALE) {
return Optional.of(
completeAndRequeueAfter(KueueWorkloadUtils.STALE_WORKLOAD_REQUEUE_INTERVAL));
}
- if (admission == AdmissionResult.PENDING) {
+ String workloadName = desired.getMetadata().getName();
+ if (admission == AdmissionResult.QUEUED) {
+ EventUtils.record(
Review Comment:
Done in this PR rather than a follow-up, since the duplication grew with the
new events.
The body moved to `KueueWorkloadUtils.holdForAdmission(BaseContext<?>,
Workload, String)`, where `requested` is `"driver"` or `"master and workers"`,
and each init step keeps only its own guard as you sketched. The two
`log.debug` lines are merged into one that takes the `Workload` name and
`requested` as parameters.
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/kueue/KueueWorkloadUtils.java:
##########
@@ -62,6 +62,12 @@ private KueueWorkloadUtils() {}
public enum AdmissionResult {
/** Kueue admitted the Workload, so the requested resources can be
created. */
ADMITTED,
+ /**
+ * The Workload has just been created and waits for quota, so the resource
creation is held
+ * like {@link #PENDING}. It is reported apart so that callers can tell a
new request from an
+ * unchanged one.
+ */
+ QUEUED,
Review Comment:
Taking this alternative. `QUEUED`, the `absent` flag and the extra
`client.resource(desired).get()` are gone, `requestAdmission` is back to a
single `getOrCreateSecondaryResource` call, and `KueueAdmissionPending` is
published on every `PENDING` reconcile. The two `Event` API calls per queued
resource per reconcile are worth it here, since the queued first attempt has no
status to fall back on.
The strict read went away with the enum value. The
`ReconcilerUtils.getResource` issue you noted is real but independent, and
making the initial lookup in `getOrCreateSecondaryResource` strict changes
every secondary resource, so I would rather file it as its own ticket than
widen this PR.
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/reconciler/reconcilesteps/AppInitStep.java:
##########
@@ -195,10 +208,27 @@ private Optional<ReconcileProgress> holdForKueueAdmission(
ReconcileProgress.completeAndRequeueAfter(
KueueWorkloadUtils.STALE_WORKLOAD_REQUEUE_INTERVAL));
}
- if (admission == AdmissionResult.PENDING) {
+ String workloadName = desired.getMetadata().getName();
+ if (admission == AdmissionResult.QUEUED) {
+ EventUtils.record(
Review Comment:
Added in 265d4e0. `EventUtils.normal(recorder, reason, message)` sits next
to `warn(...)`, the four call sites use it, and the
`io.javaoperatorsdk.operator.api.event.EventType` import is gone from both
`AppInitStep` and `ClusterInitStep`.
--
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]
