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/20 21:56:22 UTC
[PR] [SPARK-59680] Do not release the Kueue quota of a running driver or master on a failed read [spark-kubernetes-operator]
dongjoon-hyun opened a new pull request, #857:
URL: https://github.com/apache/spark-kubernetes-operator/pull/857
### What changes were proposed in this pull request?
This PR stops `AppInitStep` and `ClusterInitStep` from taking a failed read of the driver pod or the master `StatefulSet` for a resource that was never requested.
`SparkAppContext.getCurrentAttemptDriverPod` propagates a failed verification instead of reporting the pod as absent, and `ClusterInitStep.isMasterRequested` reads the `StatefulSet` directly rather than through the lenient `ReconcilerUtils.getResource`. Both steps now requeue on such a failure: the suspend branch, moved into a `holdForSuspend` method, neither releases the Kueue Workload nor requests the resources, and the Kueue admission path retries like a failed admission request instead of letting `catch (Exception)` turn it into the terminal `SchedulingFailure`.
### Why are the changes needed?
A suspended resource whose driver or master was requested before has to complete its attempt rather than be held, so both steps read the live resource to tell the two cases apart. That read reported every failure as "never requested", so an unavailable API server was enough to release the Kueue quota reservation of a resource whose driver or master is actually running, which is what the check above that branch guards against.
[SPARK-59667](https://issues.apache.org/jira/browse/SPARK-59667) made the initial read of `getOrCreateSecondaryResource` strict for the same reason, and deliberately kept `ReconcilerUtils.getResource` lenient, so these two callers ask for the strict read themselves.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Pass the CIs.
### 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-59680] Do not release the Kueue quota of a running driver or master on a failed read [spark-kubernetes-operator]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on PR #857:
URL: https://github.com/apache/spark-kubernetes-operator/pull/857#issuecomment-5763171675
Yeah, got it approved already.
--
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-59680] Do not release the Kueue quota of a running driver or master on a failed read [spark-kubernetes-operator]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on code in PR #857:
URL: https://github.com/apache/spark-kubernetes-operator/pull/857#discussion_r4060941725
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/reconciler/reconcilesteps/AppInitStep.java:
##########
@@ -175,9 +205,22 @@ public ReconcileProgress reconcile(
*/
private Optional<ReconcileProgress> holdForKueueAdmission(
SparkAppContext context, SparkApplication app) {
- if (!KueueWorkloadFactory.hasQueueName(app) || isDriverRequested(context)) {
+ if (!KueueWorkloadFactory.hasQueueName(app)) {
return Optional.empty();
}
+ try {
+ if (isDriverRequested(context)) {
+ return Optional.empty();
+ }
+ } catch (KubernetesClientException e) {
+ // Requesting the admission of a driver which is already running would be wrong, and the
+ // failure goes away like a failed admission request, so it is retried rather than failing
+ // the application with the terminal SchedulingFailure.
+ log.error("Failed to check whether the driver exists before requesting admission.", e);
+ return Optional.of(
Review Comment:
**Finding 1.** This takes the short interval for every `KubernetesClientException`, so a failure that does not go away on its own is retried every 5 seconds forever, and nothing reports it. `ClusterInitStep.java:214` is the same.
The description says the path "retries like a failed admission request", but a failed admission request is *classified*. `KueueWorkloadUtils.holdForAdmission` splits the two cases and says why in its own comment: transient gets `STALE_WORKLOAD_REQUEUE_INTERVAL` and no event, while "a persistent failure, such as a missing Kueue or the RBAC rules for it, is retried with the default interval, so that its event is not rewritten every few seconds until a user fixes the cause."
Observed on this head, with `getCurrentAttemptDriverPod` answering 403:
```
PROBE-403-PROGRESS >>> ReconcileProgress(completed=true, requeue=true, requeueAfterDuration=PT5S)
PROBE-403-TRANSIENT >>> false
PROBE-403-TRANSIENT-429 >>> false
PROBE-403-NO-EVENT >>> confirmed
```
`ReconcilerUtils.isTransientError` is true only for code 0, 408, 502, 503 and 504, so 403 and 429 both land here. The other arm is already pinned in this file: `kueueApiFailureIsRetried` asserts that a 403 reaching `holdForAdmission` gives `completeAndDefaultRequeue()` and a `KueueAdmissionRequestFailed` warning event.
Two consequences. A resource whose pod read is refused sits in `Submitted` with nothing in `kubectl describe`, only a log line. And the request rate against an API server that is already refusing goes from one read per 120 seconds per resource on base to one per 5 seconds, which is the wrong direction for a 429 in particular — `getResourceStrictly` lists `HTTP_TOO_MANY_REQUESTS` in its lenient bucket for exactly that reason. Base ignored the failure altogether, so it is not a correctness baseline, but the 24x rate increase is real.
Mirroring the sibling:
```java
} catch (KubernetesClientException e) {
log.warn("Failed to check whether the driver exists before requesting admission.", e);
if (ReconcilerUtils.isTransientError(e)) {
return Optional.of(
ReconcileProgress.completeAndRequeueAfter(
KueueWorkloadUtils.STALE_WORKLOAD_REQUEUE_INTERVAL));
}
EventUtils.warn(
context.getEventRecorder(),
EventUtils.REASON_KUEUE_ADMISSION_REQUEST_FAILED,
"Failed to check whether the driver exists before requesting Kueue admission, will "
+ "retry. "
+ EventUtils.describe(e));
return Optional.of(completeAndDefaultRequeue());
}
```
`EventUtils` needs importing in both steps, and `ReconcilerUtils` in `ClusterInitStep` (this PR drops that import). Reusing `REASON_KUEUE_ADMISSION_REQUEST_FAILED` keeps the reason list as is, but its `docs/configuration.md` row currently reads "Creating, reading or deleting a stale Kueue `Workload` fails", which would need widening to cover the pre-admission lookup.
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/reconciler/reconcilesteps/ClusterInitStep.java:
##########
@@ -185,9 +224,13 @@ private Optional<ReconcileProgress> holdForKueueAdmission(
*
* @param context The SparkClusterContext for the cluster.
* @return True if the master StatefulSet exists, false otherwise.
+ * @throws KubernetesClientException if the lookup fails, so that a running master is not
+ * mistaken for one that was never requested.
*/
private boolean isMasterRequested(SparkClusterContext context) {
- return ReconcilerUtils.getResource(context.getClient(), context.getMasterStatefulSetSpec())
- .isPresent();
+ // The lenient ReconcilerUtils.getResource is not used here, since it reports a StatefulSet it
+ // could not read as absent, which would release the Kueue quota of a running master. The
+ // client itself reports a StatefulSet which does not exist as null.
+ return context.getClient().resource(context.getMasterStatefulSetSpec()).get() != null;
Review Comment:
**Finding 2.** With this line, `ReconcilerUtils.getResource` loses its last caller outside `ReconcilerUtils` itself. What is left is two internal uses in the `getOrCreateSecondaryResource` retry loop (`ReconcilerUtils.java:140` and `:149`) and one direct test.
That is the shape worth locking in. The lenient read exists for the create loop, which re-reads on an `AlreadyExists` conflict and resolves the real state anyway, and it is wrong for anything that makes a decision from the answer — which is what this PR just finished proving twice. Leaving it `public` invites the next caller to pick it up again.
`ReconcilerUtilsTest` is in `org.apache.spark.k8s.operator.utils`, so dropping `getResource` to `private static` keeps that test compiling and makes the misuse impossible from outside. Its javadoc could then say it is the create loop's read rather than a general one.
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/reconciler/reconcilesteps/ClusterInitStep.java:
##########
@@ -185,9 +224,13 @@ private Optional<ReconcileProgress> holdForKueueAdmission(
*
* @param context The SparkClusterContext for the cluster.
* @return True if the master StatefulSet exists, false otherwise.
+ * @throws KubernetesClientException if the lookup fails, so that a running master is not
+ * mistaken for one that was never requested.
*/
private boolean isMasterRequested(SparkClusterContext context) {
- return ReconcilerUtils.getResource(context.getClient(), context.getMasterStatefulSetSpec())
- .isPresent();
+ // The lenient ReconcilerUtils.getResource is not used here, since it reports a StatefulSet it
Review Comment:
**Finding 3.** Contrasting this only with the lenient `getResource` understates what changed, and the description has the same gap where it says the two callers "ask for the strict read themselves". Neither caller reaches `getResourceStrictly`, and that method is not fully strict either: it reports 503, 500 and 429 as absent too (`ReconcilerUtils.java:233-241`), deliberately, because the create path it serves re-reads on a conflict.
So the raw read here is stricter than both `ReconcilerUtils` entry points, and that is the point — the 503 in `suspendedClusterWithFailedMasterLookupKeepsKueueWorkload` is a code `getResourceStrictly` would still have swallowed. Worth saying so, since a reader checking "what happens on a 503" would otherwise reason from `getResourceStrictly` and get the opposite answer:
```java
// Neither ReconcilerUtils read is used here: getResource reports a StatefulSet it could not
// read as absent, and getResourceStrictly does the same for a transient failure, a 500 or a
// 429, since the create path it serves re-reads anyway. Either would release the Kueue quota
// of a running master. Only a 404 may mean absent, which the client reports as null.
```
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/reconciler/reconcilesteps/AppInitStep.java:
##########
@@ -162,6 +159,39 @@ public ReconcileProgress reconcile(
return attemptStatusUpdate(context, statusRecorder, updatedStatus, completeAndDefaultRequeue());
}
+ /**
+ * Holds a suspended application, unless its driver has been requested before: the driver was
+ * created but the status update to DriverRequested failed, so the attempt has to complete
+ * instead of being held with a live driver.
+ *
+ * @param context The SparkAppContext for the application.
+ * @param app The SparkApplication.
+ * @return The progress to return while the application is held, or empty to proceed.
+ */
+ private Optional<ReconcileProgress> holdForSuspend(
+ SparkAppContext context, SparkApplication app) {
+ if (!app.getSpec().isSuspend()) {
+ return Optional.empty();
+ }
+ try {
+ if (isDriverRequested(context)) {
+ return Optional.empty();
+ }
+ } catch (KubernetesClientException e) {
+ // Neither holding nor requesting the driver is safe while the lookup fails: a failed lookup
+ // of a running driver would release its Kueue quota, and requesting the resources of a
+ // suspended application is exactly what the hold guards against.
+ log.error("Failed to check whether the driver of the suspended application exists.", e);
Review Comment:
**Finding 4.** `error` for a failure the next line retries is louder than the siblings, and a 503 blip on a suspended resource would page someone. `KueueWorkloadUtils.holdForAdmission` logs the same class of failure as `log.warn("Failed to request Kueue admission, will retry.", e)`, and the `catch` this PR removes from `SparkAppContext` was a `log.warn` too. `warn` at all four new sites (`AppInitStep.java:184` and `:219`, `ClusterInitStep.java:178` and `:213`) matches them.
--
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-59680] Do not release the Kueue quota of a running driver or master on a failed read [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #857:
URL: https://github.com/apache/spark-kubernetes-operator/pull/857#issuecomment-5763214818
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-59680] Do not release the Kueue quota of a running driver or master on a failed read [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #857:
URL: https://github.com/apache/spark-kubernetes-operator/pull/857#issuecomment-5762533565
Thanks, @peter-toth! Findings 5 and 6 are addressed in `a6c5ec6`, and 7 is left for a follow-up as you offered.
**5 (Blocking)** — Folded the lenient removal in, as you suggested. With the wrapper gone there is only one variant left, so the `Strictly` suffix has nothing to contrast with and the method takes the plain `getCurrentAttemptDriverPod` name back. That keeps the six behaviour cases in `SparkAppContextTest` as they are, now pointed at the method production actually calls, and turns `apiErrorDuringVerificationIsTreatedAsAbsent` into `apiErrorDuringVerificationIsPropagated` with the `assertThrows` you wrote.
I ran your mutant both ways to confirm the hole is closed: with the body catching `KubernetesClientException` and returning `Optional.empty()`, `SparkAppContextTest.apiErrorDuringVerificationIsPropagated` fails, and it passes on the restored source. `SparkAppContext` also loses its `@Slf4j`, since the wrapper held its only log call.
**6 (Non-blocking)** — Added `retryAfterRequestFailure` next to `holdForAdmission` and pointed all three catches at it, which takes the two init-step catches from 21 lines to 8 each. The `IllegalStateException` arm of `holdForAdmission` stays as it is, since a malformed Workload is not classified. Two adjustments to your snippet:
- It has to be `public static`. Both callers are in `reconciler.reconcilesteps`, so package-private does not compile — the same shape as finding 2, one package up.
- The extra method puts `KueueWorkloadUtils` at `WMC=47`, so PMD's `GodClass` trips. I annotated the class with `@SuppressWarnings("PMD.GodClass")`, following what SPARK-59667 did to `ReconcilerUtils` one commit earlier for the same reason.
**7 (Non-blocking)** — Agreed, and thanks for raising it here rather than against #854. Since `AppInitStep.java:78-84` has the same gap and a new `SUSPEND_CHECK_FAILED` reason is user-facing on its own, I would rather add the reason, both call sites and the `docs/configuration.md` row in one follow-up than widen this PR, which is already two rebases past what it started as.
--
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-59680] Do not release the Kueue quota of a running driver or master on a failed read [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #857:
URL: https://github.com/apache/spark-kubernetes-operator/pull/857#issuecomment-5763149279
To @peter-toth , as I already mentioned you at first, GodClass PR is waiting for this PR because this PR introduced a new GodClass instance. That's the reason why I made it as a `Draft`. The PR will be rebased and remove the instance of this PR.
- https://github.com/apache/spark-kubernetes-operator/pull/858
<img width="602" height="89" alt="Screenshot 2026-09-21 at 08 35 08" src="https://github.com/user-attachments/assets/fe2d5dfa-d8c5-4448-8082-3e289b991d9b" />
--
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-59680] Do not release the Kueue quota of a running driver or master on a failed read [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #857:
URL: https://github.com/apache/spark-kubernetes-operator/pull/857#issuecomment-5762578257
FYI, I started to exclude `GodClass` rule. After landing this PR, I'll make a PR for it.
- [SPARK-59690 Exclude PMD `GodClass` rule](https://issues.apache.org/jira/browse/SPARK-59690)
--
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-59680] Do not release the Kueue quota of a running driver or master on a failed read [spark-kubernetes-operator]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on code in PR #857:
URL: https://github.com/apache/spark-kubernetes-operator/pull/857#discussion_r4061719940
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/reconciler/reconcilesteps/AppInitStep.java:
##########
@@ -194,14 +222,16 @@ private Optional<ReconcileProgress> holdForKueueAdmission(
* Checks whether the driver pod of the current attempt has already been requested. This covers
* the case where the driver was created but the status update to DriverRequested failed, so that
* a suspended application still completes its initialization instead of being held with a live
- * driver. See {@link SparkAppContext#getCurrentAttemptDriverPod()} for how a pod left from a
- * previous attempt is told apart.
+ * driver. See {@link SparkAppContext#getCurrentAttemptDriverPodStrictly()} for how a pod left
+ * from a previous attempt is told apart.
*
* @param context The SparkAppContext for the application.
* @return True if the driver pod of the current attempt exists, false otherwise.
+ * @throws KubernetesClientException if the lookup fails, so that a running driver is not
+ * mistaken for one that was never requested.
*/
private boolean isDriverRequested(SparkAppContext context) {
- return context.getCurrentAttemptDriverPod().isPresent();
+ return context.getCurrentAttemptDriverPodStrictly().isPresent();
Review Comment:
**Finding 5.** Nothing pins the throw this line depends on. I replaced the body of `getCurrentAttemptDriverPodStrictly` with a version that catches `KubernetesClientException` and returns `Optional.empty()` — the lenient behaviour this PR exists to get off the app path — and the entire `:spark-operator` suite passed.
Why the five new tests do not cover it: they all stub a `mock(SparkAppContext.class)`, so `when(mockContext.getCurrentAttemptDriverPodStrictly()).thenThrow(...)` tests `AppInitStep`'s reaction to a throwing context, never that the real method throws. And every one of the seven `SparkAppContextTest` cases calls the lenient `getCurrentAttemptDriverPod()`, including `apiErrorDuringVerificationIsTreatedAsAbsent`, which asserts the *opposite* contract. So the strict variant has no caller in any test.
The cluster side does not have this hole: `isMasterRequested` is real code in `ClusterInitStep`, and `suspendedClusterWithUnverifiableMasterIsNotHeld` makes the client itself throw, so the read and the decision are covered together.
This is new rather than something I missed last round — `e90b0c6` had `SparkAppContextTest.apiErrorDuringVerificationIsPropagated` doing exactly this, and the rebase onto #854 replaced it with the lenient-wrapper version.
Minimum fix, which I ran both ways (fails on the swallowing mutant, passes on `3ab7498`):
```java
@Test
void apiErrorDuringVerificationIsPropagatedStrictly() {
SparkAppContext context = buildContext(List.of(driverPodSpec), null);
when(context.getClient().pods().inNamespace("default").withName(anyString()).get())
.thenThrow(new KubernetesClientException("boom", 500, null));
Assertions.assertThrows(
KubernetesClientException.class, context::getCurrentAttemptDriverPodStrictly);
}
```
Dropping the lenient wrapper instead gets this for free: the six behaviour cases retarget onto the strict method, and `apiErrorDuringVerificationIsTreatedAsAbsent` becomes the assertion above.
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/reconciler/reconcilesteps/AppInitStep.java:
##########
@@ -183,9 +184,36 @@ public ReconcileProgress reconcile(
*/
private Optional<ReconcileProgress> holdForKueueAdmission(
SparkAppContext context, SparkApplication app) {
- if (!KueueWorkloadFactory.hasQueueName(app) || isDriverRequested(context)) {
+ if (!KueueWorkloadFactory.hasQueueName(app)) {
return Optional.empty();
}
+ try {
+ if (isDriverRequested(context)) {
+ return Optional.empty();
+ }
+ } catch (KubernetesClientException e) {
+ // Requesting the admission of a driver which is already running would be wrong, so the
+ // lookup is retried rather than failing the application with the terminal
+ // SchedulingFailure. Like a failed admission request, 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, and it goes away on its own, so it keeps the short interval.
+ log.warn("Failed to check whether the driver exists before requesting admission.", e);
+ if (ReconcilerUtils.isTransientError(e)) {
Review Comment:
**Finding 6.** This block, `ClusterInitStep.java:194-215` and `KueueWorkloadUtils.java:184-201` are now the same shape three times: `log.warn`, `isTransientError` to the short interval with no event, otherwise a `KueueAdmissionRequestFailed` warning and the default interval. Round 1 had one copy of it; this PR adds the second and third, which is the point at which it is worth naming.
The risk is drift rather than volume. A future change to the classification — adding 429 to `isTransientError`, changing the persistent interval, adding a metric — has to be made in three places, and two of them are in files whose tests would not notice.
Since all three live in front of `holdForAdmission` and share its reason, the helper belongs next to it:
```java
/**
* Reports a failed Kueue admission request, or a failed read that has to happen before one, and
* returns the progress to retry it with. 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 it, and it
* goes away on its own, so it keeps the short interval. Anything else, such as a missing RBAC
* rule, is retried with the default interval so that its event is not rewritten every few
* seconds until a user fixes the cause.
*/
static ReconcileProgress retryAfterRequestFailure(
final BaseContext<?> context, final KubernetesClientException e, final String what) {
log.warn("{}, will retry.", what, e);
if (ReconcilerUtils.isTransientError(e)) {
return ReconcileProgress.completeAndRequeueAfter(STALE_WORKLOAD_REQUEUE_INTERVAL);
}
EventUtils.warn(
context.getEventRecorder(),
EventUtils.REASON_KUEUE_ADMISSION_REQUEST_FAILED,
what + ", will retry. " + EventUtils.describe(e));
return ReconcileProgress.completeAndDefaultRequeue();
}
```
Each catch then reads `return Optional.of(KueueWorkloadUtils.retryAfterRequestFailure(context, e, "Failed to check whether the driver exists before requesting Kueue admission"));`, and `holdForAdmission`'s own catch passes `"Failed to request Kueue admission"`. The five new tests already assert both arms for both steps, so they pin the extraction as it stands.
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/reconciler/reconcilesteps/ClusterInitStep.java:
##########
@@ -65,8 +67,20 @@ public ReconcileProgress reconcile(
SparkCluster cluster = context.getResource();
// A cluster whose master StatefulSet already exists has been requested before (e.g. the status
// update to RunningHealthy failed), so let it complete its initialization even if suspended.
- if (cluster.getSpec().isSuspend() && !isMasterRequested(context)) {
- return SuspendUtils.holdForSuspend(context, "master and workers");
+ if (cluster.getSpec().isSuspend()) {
+ final boolean masterRequested;
+ try {
+ masterRequested = isMasterRequested(context);
+ } catch (KubernetesClientException e) {
+ // Whether the master is live is unknown, not answered. Holding would claim in an event
+ // that none was requested, and would release the Kueue quota of a running master, so
+ // look again with the steady-state interval instead.
+ log.warn("Failed to check whether the master of a suspended cluster exists.", e);
+ return completeAndDefaultRequeue();
Review Comment:
**Finding 7.** Not publishing `SuspendHeld` here is right, since it would claim no master was requested. But publishing nothing leaves a suspended cluster with no signal at all when the failure is persistent, and `suspendedClusterWithUnverifiableMasterIsNotHeld` pins that silence with `verifyNoInteractions(eventRecorder)`.
Three facts stack up. A suspended resource never gets its `Submitted` status persisted, which `docs/spark_custom_resources.md` states outright. This branch returns before `SuspendUtils.holdForSuspend`, so there is no event. And `verifyNoInteractions(recorder)` in that test confirms no status write. So for a 403 on `StatefulSet` reads the user sees an empty `Current State`, no events, and a 120-second loop that never resolves — while the operator cannot decide whether the master is live.
For a 503 silence is the right answer, and for the same reason the admission branch gives: do not add event writes to an API server that is already failing. It is the persistent half that is asymmetric, because the admission branch fourteen lines down classifies the identical exception and reports it.
`KueueAdmissionRequestFailed` does not fit, since no admission is involved, so this needs its own reason:
```java
} catch (KubernetesClientException e) {
log.warn("Failed to check whether the master of a suspended cluster exists.", e);
if (!ReconcilerUtils.isTransientError(e)) {
EventUtils.warn(
context.getEventRecorder(),
EventUtils.REASON_SUSPEND_CHECK_FAILED,
"Cannot tell whether the master was requested before, so the suspend hold is "
+ "deferred. " + EventUtils.describe(e));
}
return completeAndDefaultRequeue();
}
```
plus the `REASON_SUSPEND_CHECK_FAILED` constant and a row in the `Warning` table of `docs/configuration.md`. `AppInitStep.java:78-84` has the same gap and came in with #854, so the same helper covers both. If you would rather not add a reason in this PR, a follow-up is fine — the cluster half is what this PR adds, so I am raising it here rather than against #854.
--
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-59680] Do not release the Kueue quota of a running driver or master on a failed read [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun closed pull request #857: [SPARK-59680] Do not release the Kueue quota of a running driver or master on a failed read
URL: https://github.com/apache/spark-kubernetes-operator/pull/857
--
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-59680] Do not release the Kueue quota of a running driver or master on a failed read [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #857:
URL: https://github.com/apache/spark-kubernetes-operator/pull/857#issuecomment-5763169720
Thank you, @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
Re: [PR] [SPARK-59680] Do not release the Kueue quota of a running driver or master on a failed read [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #857:
URL: https://github.com/apache/spark-kubernetes-operator/pull/857#issuecomment-5763205939
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-59680] Do not release the Kueue quota of a running driver or master on a failed read [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #857:
URL: https://github.com/apache/spark-kubernetes-operator/pull/857#issuecomment-5759301975
Thanks for the review, @peter-toth! All four findings are addressed in `3ab7498`, and the PR is rebased onto SPARK-59677 (#854), which landed first.
**1 (Blocking)** — Fixed as suggested. Both admission branches now classify the failure like the `KueueWorkloadUtils.holdForAdmission` they sit in front of: `ReconcilerUtils.isTransientError` keeps `STALE_WORKLOAD_REQUEUE_INTERVAL` with no event, and everything else — 403 and 429 included — gets `completeAndDefaultRequeue()` plus a `KueueAdmissionRequestFailed` warning. The 403 arm is pinned by `refusedDriverLookupBeforeKueueAdmissionPublishesEvent` and `refusedMasterLookupBeforeKueueAdmissionPublishesEvent`, and the 503 tests now also assert `verifyNoInteractions(eventRecorder)`. The `docs/configuration.md` row for the reason is widened to cover the pre-admission read, as you noted.
**2 (Non-blocking)** — Applied, with one correction: `private` does not compile. `ReconcilerUtilsTest` is in the same package, but package-private access is not `private`, so `compileTestJava` fails with `getResource(KubernetesClient,T) has private access in ReconcilerUtils`. It is package-private now, which still makes the misuse impossible from outside `org.apache.spark.k8s.operator.utils`, and its javadoc says it is the create loop's read.
**3 (Minor)** — Took your wording. The PR description had the same gap and is updated as well.
**4 (Minor)** — `warn` at all the new sites.
On the sequencing: since #854 landed first, this PR is rebased onto it. The driver suspend branch is yours now, `isDriverRequested` moved to the `getCurrentAttemptDriverPodStrictly` it added, and my `SparkAppContext` change is gone. The cluster suspend branch mirrors your driver one: a failed read returns `completeAndDefaultRequeue()` without going through `SuspendUtils.holdForSuspend`, so no `SuspendHeld` event claims that no master was requested.
One leftover follows from that: `SparkAppContext.getCurrentAttemptDriverPod` no longer has a production caller, since `isDriverRequested` was the last one — the same shape as your finding 2. I left the lenient variant alone and noted it in the description as a follow-up, since removing it touches the context and its tests rather than the two decisions this PR fixes. Happy to fold it in here if you would rather see it in one go.
--
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-59680] Do not release the Kueue quota of a running driver or master on a failed read [spark-kubernetes-operator]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on code in PR #857:
URL: https://github.com/apache/spark-kubernetes-operator/pull/857#discussion_r4063652776
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/kueue/KueueWorkloadUtils.java:
##########
@@ -60,6 +60,7 @@
/** Utilities to create, check and release Kueue Workloads. */
@Slf4j
+@SuppressWarnings("PMD.GodClass")
Review Comment:
**Finding 8.** #858 makes this annotation a build failure rather than a leftover, because `config/pmd/ruleset.xml` excludes a long list of design rules but **not** `UnnecessaryWarningSuppression`.
That is why #858 has to remove the two annotations it touches — and it cannot reach this one, since `KueueWorkloadUtils` only grows it here. `git merge-tree` says the two PRs merge cleanly, so nothing warns about it.
I materialised the merge (this head, plus #858's `ruleset.xml` and its two annotation removals) and ran `./gradlew :spark-operator:pmdMain`:
```
KueueWorkloadUtils.java:63: UnnecessaryWarningSuppression: Unnecessary @SuppressWarnings annotation
> 1 PMD rule violations were found.
BUILD FAILED
```
Both orders break, symmetrically. If #858 lands first, this PR's PMD gate fails on rebase. If this one lands first, #858's does, since its diff has no hunk for this file.
The cheapest resolution is to land #858 first and drop the annotation from this PR on the rebase — the rule is gone, so nothing needs suppressing and nothing needs relocating. If you would rather keep this PR's order, then #858 needs a third hunk for this file.
For the record, since the suppression reads as forced: without it the class reports `GodClass: Possible God Class (WMC=47, ATFD=78, TCC=7.692%)`, and 47 is exactly PMD's threshold, so `retryAfterRequestFailure` is the single unit that crosses it. I did try moving that method into a small `KueueAdmissionFailures` class in the same package — PMD then passes with no suppression anywhere, and the rest of the gate and all 333 tests pass. Not worth doing now that the rule is being excluded, but it means the choice was never "suppress or fail".
--
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