dongjoon-hyun commented on code in PR #909:
URL:
https://github.com/apache/spark-kubernetes-operator/pull/909#discussion_r4161309308
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/reconciler/reconcilesteps/ClusterInitStep.java:
##########
@@ -68,29 +68,35 @@ 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()) {
+ // Unlike a first attempt, a cluster resumed from Suspended or queued
again after an eviction
+ // has persisted this Submitted state, which would keep saying that it
is resumed or queued
+ // again. It goes back to Suspended instead, where ClusterSuspendStep
deletes the master and
+ // worker StatefulSets by name and releases whatever it holds only after
its pods are gone.
+ // That happens whether or not the master was requested, so it is not
looked up.
+ if (cluster.getStatus().getStateTransitionHistory().lastKey() > 0) {
Review Comment:
**[P1]** This is a follow-up of P8 of round 2. The new paragraph of the
user-facing section covers part of it, but `What changes were proposed in this
pull request?` still says the following, which becomes the commit message.
```
- The requeue behavior is unchanged: both cases are still retried with the
default interval, and the resource is neither held nor started.
```
That no longer holds for a suspended `SparkCluster` which was resumed from
`Suspended` or queued again after an eviction. Before this PR, a failed lookup
kept it in `Submitted` and retried it with the default interval, while now it
goes to `Suspended` here without the lookup. A first attempt whose master
exists also goes to `Suspended` at L90-97 now, instead of completing its
initialization to `RunningHealthy`. That section mentions neither change, and
the new paragraph doesn't say that a resumed or requeued cluster is no longer
looked up at all. In addition:
- `How was this patch tested?` ends with `Pass the CIs with the newly added
test cases:` and nothing after it. It could list the two new parameterized
tests, and say that `suspendAfterMasterRequestedGoesToSuspended` replaces
`suspendAfterMasterRequestedCompletesInitialization` and that
`resumedClusterSuspendedAgainGoesBackToSuspended` now asserts that the master
is not looked up.
- nit. The doc changes are not mentioned, and `spec.suspend` and these
events are not released yet in 1.0.0, which the template asks to clarify.
Could you update the description once more?
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/reconciler/reconcilesteps/SuspendUtils.java:
##########
@@ -88,4 +91,37 @@ static ReconcileProgress holdForSuspend(final BaseContext<?>
context, final Stri
return completeAndRequeueAfter(
Review Comment:
**[P2]** P1 of round 2 makes the Javadoc of this method stale. L48-49 still
say:
```
* queued with outlives the Workload. Callers keep their own guard for
resources requested
* before, which must complete their initialization instead of being held.
```
That now holds for `AppInitStep` only, since `ClusterInitStep` L90-97 sends
a suspended first attempt whose master exists to `Suspended` rather than
completing its initialization, as `suspendAfterMasterRequestedGoesToSuspended`
asserts. The same commit already dropped the matching `Like the suspend hold, a
master requested before must complete its initialization` from the Javadoc of
`ClusterInitStep.holdForKueueAdmission`, so only this end is left. L48-49 are
not in the diff, so I'm leaving this here. How about the following?
```java
* queued with outlives the Workload. Callers keep their own guard for
resources requested
* before, which are not held: an application whose driver was requested
completes its
* initialization, while a cluster whose master was requested goes to
Suspended instead.
```
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/reconciler/reconcilesteps/ClusterInitStep.java:
##########
@@ -68,29 +68,35 @@ 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()) {
+ // Unlike a first attempt, a cluster resumed from Suspended or queued
again after an eviction
+ // has persisted this Submitted state, which would keep saying that it
is resumed or queued
+ // again. It goes back to Suspended instead, where ClusterSuspendStep
deletes the master and
+ // worker StatefulSets by name and releases whatever it holds only after
its pods are gone.
+ // That happens whether or not the master was requested, so it is not
looked up.
+ if (cluster.getStatus().getStateTransitionHistory().lastKey() > 0) {
+ return appendStateAndImmediateRequeue(
+ context, statusRecorder, new ClusterState(Suspended,
CLUSTER_SUSPENDED_MESSAGE));
+ }
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();
+ return SuspendUtils.retryAfterCheckFailure(context, e, "master");
}
- 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));
- }
- return SuspendUtils.holdForSuspend(context, "master and workers");
+ if (masterRequested) {
+ // A first attempt whose master StatefulSet already exists has been
requested before, e.g.
+ // the status update to RunningHealthy failed. It goes to Suspended as
well, rather than
+ // completing its initialization: that would apply every resource
again only for
+ // ClusterSuspendStep to delete them, and a failure to apply them
would keep the cluster
+ // from being suspended, or fail it, although it was only suspended.
+ return appendStateAndImmediateRequeue(
+ context, statusRecorder, new ClusterState(Suspended,
CLUSTER_SUSPENDED_MESSAGE));
Review Comment:
**[P5]** nit. This exit is the same as L78-79, and their comments frame the
same outcome in opposite terms: L72 starts with `Unlike a first attempt`, while
L91-92 say that a first attempt whose master exists `goes to Suspended as
well`. The rule is now that only a first attempt whose master is known to be
absent is held. How about nesting the lookup under `lastKey() == 0` and keeping
a single transition with one merged comment? It behaves the same: a resumed or
requeued cluster still skips the lookup, which
`resumedClusterSuspendedAgainGoesBackToSuspended` checks, and only a
`KubernetesClientException` is caught.
```java
if (cluster.getSpec().isSuspend()) {
if (cluster.getStatus().getStateTransitionHistory().lastKey() == 0) {
final boolean masterRequested;
try {
masterRequested = isMasterRequested(context);
} catch (KubernetesClientException e) {
// (the current comment)
return SuspendUtils.retryAfterCheckFailure(context, e, "master");
}
if (!masterRequested) {
return SuspendUtils.holdForSuspend(context, "master and workers");
}
}
// (the comments of L72-76 and L91-95, merged)
return appendStateAndImmediateRequeue(
context, statusRecorder, new ClusterState(Suspended,
CLUSTER_SUSPENDED_MESSAGE));
}
```
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/reconciler/reconcilesteps/ClusterInitStep.java:
##########
@@ -68,29 +68,35 @@ 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()) {
+ // Unlike a first attempt, a cluster resumed from Suspended or queued
again after an eviction
+ // has persisted this Submitted state, which would keep saying that it
is resumed or queued
+ // again. It goes back to Suspended instead, where ClusterSuspendStep
deletes the master and
+ // worker StatefulSets by name and releases whatever it holds only after
its pods are gone.
+ // That happens whether or not the master was requested, so it is not
looked up.
+ if (cluster.getStatus().getStateTransitionHistory().lastKey() > 0) {
+ return appendStateAndImmediateRequeue(
+ context, statusRecorder, new ClusterState(Suspended,
CLUSTER_SUSPENDED_MESSAGE));
+ }
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();
+ return SuspendUtils.retryAfterCheckFailure(context, e, "master");
Review Comment:
**[P4]** A design question, which can be a follow-up. After P1 of round 2,
this lookup only decides between `holdForSuspend` and `Suspended`, and the
comments at L72-76 and L91-95 explain that `Suspended` is correct whether or
not the master was requested: `ClusterSuspendStep` deletes the `StatefulSet`s
by name, which is a no-op for a missing master, and releases the `Workload`
only after the pods are gone. So a failed lookup could go to `Suspended` as
well. That would persist a status, which `kubectl get` shows even though events
are disabled by default, end the retry, and release a pending `Workload`
instead of keeping it while it may be admitted
(`docs/spark_custom_resources.md` L692-694).
On the other hand, a cluster which may never have started would then show
`Suspended` (`its master and workers are being released`) instead of the
unpersisted `Submitted` which the Suspend section describes, and
`isTransientError` counts a single 429 or 500 as persistent, so one throttled
read would leave it there until it is resumed. The same RBAC or API server
failure also tends to fail the status patch and the deletions, and P4 of round
1 kept `SuspendCheckFailed` for exactly this first attempt. So I'm fine with
keeping the current behavior. In that case, could the comment here say why such
a cluster doesn't go to `Suspended` either, since the comments around it
suggest that it could?
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/reconciler/reconcilesteps/ClusterInitStep.java:
##########
@@ -68,29 +68,35 @@ 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()) {
+ // Unlike a first attempt, a cluster resumed from Suspended or queued
again after an eviction
+ // has persisted this Submitted state, which would keep saying that it
is resumed or queued
+ // again. It goes back to Suspended instead, where ClusterSuspendStep
deletes the master and
+ // worker StatefulSets by name and releases whatever it holds only after
its pods are gone.
+ // That happens whether or not the master was requested, so it is not
looked up.
+ if (cluster.getStatus().getStateTransitionHistory().lastKey() > 0) {
+ return appendStateAndImmediateRequeue(
+ context, statusRecorder, new ClusterState(Suspended,
CLUSTER_SUSPENDED_MESSAGE));
+ }
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();
+ return SuspendUtils.retryAfterCheckFailure(context, e, "master");
}
- 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));
- }
- return SuspendUtils.holdForSuspend(context, "master and workers");
+ if (masterRequested) {
Review Comment:
**[P3]** This adds another path from `Submitted` to `Suspended`, which the
docs don't show yet. Neither line below is in the diff, so I'm leaving this
here.
- `docs/architecture.md` L206 labels the only `Submitted --> Suspended` edge
`spec.suspend=true after resume`. A first attempt whose master already exists
now takes it as well, from its unpersisted `Submitted` straight to `Suspended`,
without `RunningHealthy`. The label also misses a cluster queued again after a
Kueue eviction, which predates this PR.
- `docs/spark_custom_resources.md` L588-589 say "Setting it to `true` again
before the master is requested moves the cluster back to `Suspended`." Since
L77-80, a resumed or requeued cluster goes back to `Suspended` whether or not
the master was requested, so `before the master is requested` is no longer the
condition.
How about `Submitted --> Suspended : spec.suspend=true after resume or Kueue
requeue, or once the master is requested` and "Setting it to `true` again moves
the cluster back to `Suspended`."?
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/reconciler/reconcilesteps/ClusterInitStep.java:
##########
@@ -68,29 +68,35 @@ 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()) {
+ // Unlike a first attempt, a cluster resumed from Suspended or queued
again after an eviction
+ // has persisted this Submitted state, which would keep saying that it
is resumed or queued
+ // again. It goes back to Suspended instead, where ClusterSuspendStep
deletes the master and
+ // worker StatefulSets by name and releases whatever it holds only after
its pods are gone.
+ // That happens whether or not the master was requested, so it is not
looked up.
+ if (cluster.getStatus().getStateTransitionHistory().lastKey() > 0) {
+ return appendStateAndImmediateRequeue(
+ context, statusRecorder, new ClusterState(Suspended,
CLUSTER_SUSPENDED_MESSAGE));
+ }
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();
+ return SuspendUtils.retryAfterCheckFailure(context, e, "master");
}
- 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));
- }
- return SuspendUtils.holdForSuspend(context, "master and workers");
+ if (masterRequested) {
+ // A first attempt whose master StatefulSet already exists has been
requested before, e.g.
+ // the status update to RunningHealthy failed. It goes to Suspended as
well, rather than
+ // completing its initialization: that would apply every resource
again only for
+ // ClusterSuspendStep to delete them, and a failure to apply them
would keep the cluster
+ // from being suspended, or fail it, although it was only suspended.
Review Comment:
**[P6]** nit. `ClusterSuspendStep.releaseResources` deletes only the
`StatefulSet`s and the HPA and PDB of the workers, and keeps the Services and
the NetworkPolicy (`The Services and the NetworkPolicy are kept, since they
hold no quota and are applied again on resume.`), so `only for
ClusterSuspendStep to delete them` overstates it. The same goes for
`ClusterSuspendStep deletes what was applied` in the comment of
`suspendAfterMasterRequestedGoesToSuspended`. Also, `since it is held or goes
to Suspended first` at L220 leaves out a failed lookup, which returns
`retryAfterCheckFailure` at L88. How about the following, and e.g. `since every
suspend branch of reconcile returns first` at L220?
```suggestion
// A first attempt whose master StatefulSet already exists has been
requested before, e.g.
// the status update to RunningHealthy failed. It goes to Suspended
as well, rather than
// completing its initialization: that would apply every resource
again only for
// ClusterSuspendStep to release the master and workers, and a
failure to apply them would
// keep the cluster from being suspended, or fail it, although it
was only suspended.
```
--
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]
