peter-toth commented on code in PR #847:
URL:
https://github.com/apache/spark-kubernetes-operator/pull/847#discussion_r4052542520
##########
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:
**Finding 1.** This publishes the warning on every retry of the same
failure, and the retry runs every 5 seconds, so a persistent failure is an
unbounded event-write loop.
I ran 12 reconciles of one `AppInitStep` against a client whose `create()`
always returns 403: the recorder got 12 `record` calls. Each of those is not
one API call. `DefaultEventSink.emit` GETs the `Event` by name and then creates
or patches it, so it is two. At the 5 s requeue that is about 24 `Event` API
calls a minute per stuck resource, and the rate limiter does not clamp it (3
loops per 15 s against the 5-per-15-s default).
The failures that reach here are persistent, not momentary:
- `operatorRbac.kueue.enabled` defaults to `false`, so a resource labeled
with a queue name on a chart-default install gets 403 on the `Workload`.
- Kueue not installed at all gives 404 on the create.
Neither is transient by `isTransientError`, so both publish. The resource
stays in its initializing state, since the first attempt's `Submitted` is never
persisted, so nothing ends the loop but a user fixing the RBAC or deleting the
resource. Compare `ReconcileError` two rows above in the same doc table, which
publishes only on the first attempt of a failure episode.
The 5 s interval is also off-label here. Its own javadoc scopes it to
`STALE`: *"It is short because the stale Workload goes away shortly, while an
unchanged admission is watched with the default interval."* A failed request is
the opposite case. Requeueing it at the default interval fixes the cadence and
the event volume together, 120 s instead of 5 s:
```java
return Optional.of(completeAndDefaultRequeue());
```
Same for `ClusterInitStep.java:200-201`. `docs/configuration.md:77` then
needs its "It is retried every 5 seconds" updated too.
##########
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:
**Finding 4.** Worth weighing whether `QUEUED` needs to exist at all.
`DefaultEventSink.emit` already does the deduplication this enum value buys. It
GETs the `Event` by a name derived from the resource and the record's `key`
(which `EventUtils.record` sets to the reason), patches `count`, `message` and
`lastTimestamp` when it is there, and creates it when it is not.
So publishing `KueueAdmissionPending` on every `PENDING` reconcile gives one
`Event` object per resource with a climbing `count`, not one object per
reconcile. It also means an `Event` the API server has already dropped is
re-created on the next reconcile, which is what finding 2 needs.
What it removes: this enum value, the `absent` flag, the extra
`client.resource(desired).get()` at line 105, and the `QUEUED || PENDING`
branching in both init steps. `requestAdmission` goes back to a single
`getOrCreateSecondaryResource` call.
The honest cost is two `Event` API calls per queued resource per reconcile
instead of zero. At the 120 s default interval that is one write a minute per
queued resource, so 1000 queued resources is roughly 17 calls/s. That may well
be the wrong trade for you, in which case finding 2 needs a different answer.
One part of the strict read is worth keeping either way.
`ReconcilerUtils.getResource` swallows a non-404 failure and reports the
resource as missing, which is a bug independent of the events. It would fit as
a strict variant used for the initial lookup in `getOrCreateSecondaryResource`
itself, so every secondary resource benefits.
##########
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:
**Finding 3.** With this PR `holdForKueueAdmission` is about 50 lines here
and the same 50 in `AppInitStep.java:183-233`, identical apart from two nouns
in the messages and the two `log.debug` lines. The new parts are duplicated
verbatim, including the transient-error test and its two-line comment.
Both `SparkAppContext` and `SparkClusterContext` extend `BaseContext`, which
already exposes `getClient()` and `getEventRecorder()`, so the whole body moves:
```java
// KueueWorkloadUtils
public static Optional<ReconcileProgress> holdForAdmission(
BaseContext<?> context, Workload desired, String requested) { ... }
```
where `requested` is `"driver"` or `"master and workers"`. Each step then
keeps only its own guard:
```java
if (!KueueWorkloadFactory.hasQueueName(cluster) ||
isMasterRequested(context)) {
return Optional.empty();
}
return KueueWorkloadUtils.holdForAdmission(
context, KueueWorkloadFactory.buildWorkload(cluster), "master and
workers");
```
A follow-up PR is fine. The point is that the divergence risk grows with
each event added to one copy only.
##########
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:
**Finding 2.** "It is not published again for each periodic reconcile" is
the part I want to push back on, because of what the closing note four lines
below says: *"the resource status remains the source of truth"*. For a
Kueue-queued first attempt the status is exactly what is missing.
`spark_custom_resources.md:592` in this PR says `kubectl get` shows an empty
`Current State`, because that attempt's `Submitted` is never persisted.
So the single `KueueAdmissionPending` event is the only signal there is, and
the API server drops it after `--event-ttl` (one hour by default). A resource
queued behind a full `ClusterQueue` for longer than that ends up with no status
and no event, which is the state the "Why are the changes needed?" section
describes.
There is a second hole with the same cause. When the operator restarts while
a resource is queued, `requestAdmission` reads an existing `Workload` and
returns `PENDING`, so nothing is ever published for that resource.
Finding 4 is the simplest way out, since it makes the republish free of new
state.
##########
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:
**Finding 5.** `EventUtils` already has `warn(recorder, reason, message)`
for a fixed type, and before this PR the only `record(..., type, ...)` caller
was `StatusRecorder.java:179`, where the type is computed by
`eventTypeOf(...)`. The four new call sites have a fixed `NORMAL` and pull
`io.javaoperatorsdk.operator.api.event.EventType` into both init steps only to
name it.
```java
/** Publishes a normal event about a Spark resource. See {@link #warn}. */
public static void normal(ResourceEventRecorder recorder, String reason,
String message) {
record(recorder, EventType.NORMAL, reason, message);
}
```
The import then goes away in 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]
