dongjoon-hyun commented on code in PR #909:
URL:
https://github.com/apache/spark-kubernetes-operator/pull/909#discussion_r4167541473
##########
docs/configuration.md:
##########
@@ -93,6 +93,7 @@ In addition, the operator publishes the following `Warning`
events.
| `KueueAdmissionRequestFailed` | Creating, reading or deleting a stale Kueue
`Workload` fails, or the driver or master cannot be read before the admission
is requested. Transport-level errors are skipped and retried every 5 seconds,
while a persistent failure is retried with the default interval. |
| `ClusterRequestFailed` | Applying the Services, StatefulSets, NetworkPolicy,
HorizontalPodAutoscaler or PodDisruptionBudget of a `SparkCluster` fails in a
way which may yet succeed, such as a throttled request or an internal server
error from an admission webhook, so it is retried with the default interval
instead of failing the cluster. Transport-level errors are skipped. |
| `SuspendReleaseFailed` | Deleting the master and worker StatefulSets, the
HorizontalPodAutoscaler or PodDisruptionBudget of the workers, or the Kueue
`Workload` of a `SparkCluster` suspended while running fails, or its pods
cannot be listed. A failure which may clear on its own, such as a timeout, is
skipped. Every failure is retried with the default interval. |
+| `SuspendCheckFailed` | The driver of a suspended `SparkApplication`, or the
master of a suspended `SparkCluster`, cannot be read to check whether it was
requested, so the resource is neither held, which would release its Kueue
`Workload`, nor started. Transport-level errors are skipped. Every such failure
is retried with the default interval. While the failure lasts, it is
republished on every reconciliation, subject to
[`minIntervalSeconds`](#event-frequency). Once the check succeeds again, the
unchanged `SuspendHeld` event may be dropped by the same interval until its
next repeat, so this event can stay the newest one until then. One which is no
longer refreshed has cleared, unless the check now fails only at the transport
level. |
Review Comment:
**[P1]** After the base of this PR, `main` added the `Event messages`
section (SPARK-59931), which recommends the following to keep the details of
failures in the resource status and the operator log only.
```properties
spark.kubernetes.operator.events.excludedReasons=SchedulingFailure,ReconcileError,CleanupError,StatusUpdateFailed,ClusterRequestFailed,SuspendReleaseFailed,Kueue.*Failed
```
`SuspendCheckFailed` embeds `EventUtils.describe(e)` as well, but matches
none of them, since `ConfigurableEventRecorder` matches the whole reason
(`Pattern.matches(regex, reason)`). So with the recommended setting, a 403 on
the master read of a suspended `SparkCluster` still publishes the RBAC denial
which names the ServiceAccount of the operator. With this PR, it is the only
reason with `EventUtils.describe(e)` which the list misses. After rebasing,
could you add `SuspendCheckFailed` to the list, or replace
`SuspendReleaseFailed` with `Suspend.*Failed`, which doesn't match
`SuspendHeld`?
##########
docs/configuration.md:
##########
@@ -93,6 +93,7 @@ In addition, the operator publishes the following `Warning`
events.
| `KueueAdmissionRequestFailed` | Creating, reading or deleting a stale Kueue
`Workload` fails, or the driver or master cannot be read before the admission
is requested. Transport-level errors are skipped and retried every 5 seconds,
while a persistent failure is retried with the default interval. |
| `ClusterRequestFailed` | Applying the Services, StatefulSets, NetworkPolicy,
HorizontalPodAutoscaler or PodDisruptionBudget of a `SparkCluster` fails in a
way which may yet succeed, such as a throttled request or an internal server
error from an admission webhook, so it is retried with the default interval
instead of failing the cluster. Transport-level errors are skipped. |
| `SuspendReleaseFailed` | Deleting the master and worker StatefulSets, the
HorizontalPodAutoscaler or PodDisruptionBudget of the workers, or the Kueue
`Workload` of a `SparkCluster` suspended while running fails, or its pods
cannot be listed. A failure which may clear on its own, such as a timeout, is
skipped. Every failure is retried with the default interval. |
+| `SuspendCheckFailed` | The driver of a suspended `SparkApplication`, or the
master of a suspended `SparkCluster`, cannot be read to check whether it was
requested, so the resource is neither held, which would release its Kueue
`Workload`, nor started. Transport-level errors are skipped. Every such failure
is retried with the default interval. While the failure lasts, it is
republished on every reconciliation, subject to
[`minIntervalSeconds`](#event-frequency). Once the check succeeds again, the
unchanged `SuspendHeld` event may be dropped by the same interval until its
next repeat, so this event can stay the newest one until then. One which is no
longer refreshed has cleared, unless the check now fails only at the transport
level. |
Review Comment:
**[P2]** This is a follow-up of P5 of round 1 and P4 of round 2. The row now
covers a driver or master which `cannot be read`, but `One which is no longer
refreshed has cleared, unless the check now fails only at the transport level.`
doesn't hold for a `SparkApplication` whose check keeps failing for another
reason. For example:
1. A suspended application has a live driver pod in the informer cache, its
read fails with 403, and `SuspendCheckFailed` is published.
2. Its `sparkConf` is then edited so that the driver spec no longer builds.
`SparkAppContext.getCurrentAttemptDriverPod()` now throws a `SparkException` at
L117, which bypasses the `catch (KubernetesClientException e)` of `AppInitStep`.
3. The application gets only `ReconcileError`, while it is still neither
held nor started and keeps its `Workload`, if any.
`suspendedAppWithCachedDriverAndUnbuildableSpecKeepsKueueWorkload` covers this
path.
`SuspendCheckFailed` is no longer refreshed then, and with the
`excludedReasons` which `main` recommends (see P1), `ReconcileError` is
excluded, so the stale event reads as cleared. How about dropping the sentence,
like `spark_custom_resources.md`, which says `The operator log shows whether
the check still fails.`?
##########
docs/spark_custom_resources.md:
##########
@@ -544,7 +544,13 @@ published when enabled. Since the status is not there to
fall back on, it is rep
minutes by default while the hold lasts, so that it outlives the event
retention of the API
server. An application held later, in `ScheduledToRestart`, keeps the status
its previous attempt
already wrote and gets the same event, since that status says that a restart
is due, not that the
-next attempt is withheld.
+next attempt is withheld. If the driver (or master) cannot be read to check
whether it was requested
+already, e.g. since the API server rejects the read, the resource is neither
held nor started, and
+the `SuspendCheckFailed` event is published instead when enabled, until the
check succeeds. A
+failure which may clear on its own, such as a timeout, is retried without the
event. The event is
Review Comment:
**[P4]** nit. `A failure which may clear on its own` follows the
`SuspendReleaseFailed` sentence at L577, and the code means a transport-level
failure by it (`ReconcilerUtils.isTransientError`). However,
`migration_guide.md` in `main` uses the same words for `a timeout, a 429, a 500
or a request which the API server never answered`, while a throttled read (429)
and an internal server error (500) do publish `SuspendCheckFailed`, as the new
tests assert. How about naming the transport level, like the row in
`configuration.md`?
```suggestion
failure at the transport level, such as a timeout, is retried without the
event. The event is
```
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/reconciler/reconcilesteps/ClusterInitStep.java:
##########
@@ -68,29 +68,32 @@ public ReconcileProgress reconcile(
return proceed();
}
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()) {
- 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();
- }
- if (!masterRequested) {
- // Unlike a first attempt, a cluster resumed from Suspended has
persisted this Submitted
- // state, which would keep saying that it is resumed. It goes back to
Suspended instead,
- // where ClusterSuspendStep releases whatever it holds only after its
pods are gone.
- if (cluster.getStatus().getStateTransitionHistory().lastKey() > 0) {
- return appendStateAndImmediateRequeue(
- context, statusRecorder, new ClusterState(Suspended,
CLUSTER_SUSPENDED_MESSAGE));
+ if (cluster.getStatus().getStateTransitionHistory().lastKey() == 0) {
+ 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.
+ // Going to Suspended would show a cluster which may never have
started as being
+ // released, and keep it there until it is resumed, even after a
single throttled read.
+ // So look again with the steady-state interval instead.
Review Comment:
**[P5]** nit. Now that `SuspendUtils.retryAfterCheckFailure` decides how to
retry, this restates its `@return` (`requeued after the steady-state
interval`), and would go stale if the helper changed, e.g. to retry a
transport-level failure as soon as
`KueueWorkloadUtils.retryAfterRequestFailure` does. The same goes for `so look
again with the steady-state interval instead.` in `AppInitStep` L84-85.
```suggestion
```
##########
docs/architecture.md:
##########
@@ -203,7 +203,7 @@ stateDiagram-v2
RunningHealthy --> Suspended : spec.suspend=true or Kueue eviction
Suspended --> Submitted : spec.suspend=false or after Kueue eviction
- Submitted --> Suspended : spec.suspend=true after resume
+ Submitted --> Suspended : spec.suspend=true after resume or Kueue requeue,
or once the master is requested
Review Comment:
**[P3]** This is a follow-up of P3 of round 3. `or once the master is
requested` can be read as a trigger of its own, as if a cluster went from
`Submitted` to `Suspended` when its master is requested, without
`spec.suspend`. Besides, a cluster whose master is requested normally goes
through `RunningHealthy`, since `ClusterInitStep` persists `RunningHealthy` in
the reconciliation which applies the master. After this PR, a suspended cluster
in `Submitted` goes to `Suspended` unless it is a first attempt without a
master, so how about saying that?
```suggestion
Submitted --> Suspended : spec.suspend=true, unless a first attempt has
no master yet
```
--
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]
