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: [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]
