You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "dongjoon-hyun (via GitHub)" <gi...@apache.org> on 2026/09/18 16:13:11 UTC
[PR] [SPARK-59638] Support K8s events for Kueue admission of `SparkApplication` and `SparkCluster` [spark-kubernetes-operator]
dongjoon-hyun opened a new pull request, #847:
URL: https://github.com/apache/spark-kubernetes-operator/pull/847
### What changes were proposed in this pull request?
This PR aims to support K8s events for Kueue admission of `SparkApplication` and `SparkCluster`.
| Type | Reason | When |
|---|---|---|
| `Normal` | `KueueAdmissionPending` | A new `Workload` is created and waits for the admission |
| `Normal` | `KueueAdmitted` | Kueue admitted the `Workload` |
| `Warning` | `KueueAdmissionRequestFailed` | The admission request fails, except for transport-level errors |
To avoid extra API calls on every requeue, `KueueAdmissionPending` is published once per `Workload` via a new `AdmissionResult.QUEUED`.
### Why are the changes needed?
A resource waiting for Kueue admission has no state transition, so users cannot see why it is stuck via `kubectl describe`.
### Does this PR introduce _any_ user-facing change?
Yes. When `spark.kubernetes.operator.events.enabled` is `true`, the new events are published for resources labeled with `kueue.x-k8s.io/queue-name`.
### How was this patch tested?
Pass the CIs with the newly added test cases.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Opus 5
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59638] Support K8s events for `Kueue` admission of `SparkApplication` and `SparkCluster` [spark-kubernetes-operator]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59638] Support K8s events for `Kueue` admission of `SparkApplication` and `SparkCluster` [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #847:
URL: https://github.com/apache/spark-kubernetes-operator/pull/847#issuecomment-5751937370
Merged to main
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59638] Support K8s events for `Kueue` admission of `SparkApplication` and `SparkCluster` [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #847:
URL: https://github.com/apache/spark-kubernetes-operator/pull/847#issuecomment-5751358826
Rebased to main to resolve the conflicts.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59638] Support K8s events for `Kueue` admission of `SparkApplication` and `SparkCluster` [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59638] Support K8s events for `Kueue` admission of `SparkApplication` and `SparkCluster` [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #847:
URL: https://github.com/apache/spark-kubernetes-operator/pull/847#issuecomment-5746228234
For `ReconcilerUtils.getResource`, I created an independent PR.
- https://github.com/apache/spark-kubernetes-operator/pull/853
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59638] Support K8s events for `Kueue` admission of `SparkApplication` and `SparkCluster` [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #847:
URL: https://github.com/apache/spark-kubernetes-operator/pull/847#issuecomment-5736346584
All tests passed.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59638] Support K8s events for `Kueue` admission of `SparkApplication` and `SparkCluster` [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun closed pull request #847: [SPARK-59638] Support K8s events for `Kueue` admission of `SparkApplication` and `SparkCluster`
URL: https://github.com/apache/spark-kubernetes-operator/pull/847
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-59638] Support K8s events for `Kueue` admission of `SparkApplication` and `SparkCluster` [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #847:
URL: https://github.com/apache/spark-kubernetes-operator/pull/847#issuecomment-5751933215
Thank you so much, @peter-toth !
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org