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/15 15:49:36 UTC
[PR] [SPARK-59542] Support `spec.suspend` to hold driver and master/worker creation in `AppInitStep` and `ClusterInitStep` [spark-kubernetes-operator]
dongjoon-hyun opened a new pull request, #828:
URL: https://github.com/apache/spark-kubernetes-operator/pull/828
### What changes were proposed in this pull request?
This PR aims to honor `spec.suspend` in `AppInitStep` and `ClusterInitStep`.
| Resource | State | `spec.suspend: true` |
|--------------------|---------------------------------|---------------------------------------------------|
| `SparkApplication` | `Submitted`, `ScheduledToRestart` | Skip driver creation, `completeAndDefaultRequeue()` |
| `SparkCluster` | `Submitted` | Skip master / worker creation, `completeAndDefaultRequeue()` |
- The status is not touched while suspended.
- Setting `spec.suspend` back to `false` triggers a reconcile and enters the existing init path.
- Validation, cleanup and deletion run before the init steps and are unaffected by `spec.suspend`.
- Add `tests/e2e/suspend` and a `Suspend` section to `docs/spark_custom_resources.md`.
Suspending an already running application or cluster is out of scope and will be handled in a follow-up.
### Why are the changes needed?
`spec.suspend` was added in SPARK-59475 but the operator does not read it yet. Job queueing systems such as Kueue create a workload with `suspend: true` and flip it to `false` once quota is available.
### Does this PR introduce _any_ user-facing change?
No. `spec.suspend` is not released yet.
| `spec.suspend` | Before | After |
|----------------|-----------------------------------------|----------------------------------------------|
| `true` | Ignored, resources created immediately | Stays in `Submitted`, no resources created |
| `false` | Resources created | Resources created (unchanged) |
### How was this patch tested?
| Test | Coverage |
|---------------------------------------|--------------------------------------------------------------------------|
| `AppInitStepTest` | Hold in `Submitted` and `ScheduledToRestart`, resume after `suspend: false` |
| `ClusterInitStepTest` | Hold in `Submitted`, proceed in `RunningHealthy` |
| `tests/e2e/suspend` | Suspended app / cluster has no pod / StatefulSet, then patch and run to completion |
`gradle build` passes locally.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Fable 5.1
--
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-59542] Support `spec.suspend` to hold driver and master/worker creation in `AppInitStep` and `ClusterInitStep` [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #828:
URL: https://github.com/apache/spark-kubernetes-operator/pull/828#issuecomment-5688475605
Thank you for the second round, @peter-toth and @viirya. All findings are addressed in 34dbda1.
**6. Docs / `Submitted` is in-memory** (peter-toth, viirya): added a paragraph to the `Suspend` section. A resource created with `suspend: true` has no `.status` on the API server and `kubectl get` shows an empty `Current State` until it resumes, while an application held in `ScheduledToRestart` keeps the status its previous attempt wrote. The PR description is limited to the initial-submission case the same way.
**7. `*-suspended.yaml` cannot fail** (peter-toth, viirya): deleted both files and replaced them with the `kubectl get ... | jq .status` / `contains($stdout, 'null')` check already used in `watched-namespaces`. As @viirya noted, this pins today's observable behavior rather than proving the hold was reconciled; the behavioral coverage stays with the `error:` checks on the driver pod / StatefulSets and the resume to completion. The null check will need updating when the `Suspended` state lands, which seems right.
**8. Hold keyed on state, not on whether the driver was requested** (peter-toth, viirya): the hold now applies only when the driver pod of the current attempt does not exist. `AppInitStep` compares the informer pod's name with `getDriverPodSpec()`'s name rather than using `getDriverPod().isEmpty()`, so a pod left from a previous attempt (same labels, different attempt id in the name) does not bypass the hold. `ClusterInitStep` skips the hold when the master StatefulSet already exists. This also covers the operator-restart variant, since the check does not depend on the status cache. Regression tests: `suspendAfterDriverRequestedCompletesInitialization` (create succeeds, status persist fails, suspend enabled, next reconcile reaches `DriverRequested`), `previousAttemptDriverPodDoesNotBypassSuspend`, and `suspendAfterMasterRequestedCompletesInitialization`.
**9. Kueue bullet** (peter-toth, viirya): applied the suggestion. The PR description now says this is a prerequisite and that `KueueWorkloadFactory` is not wired into workload creation or admission yet.
**Suspended time counted toward `restartCounterResetMillis`** (viirya): `calculateCurrentAttemptDuration()` now measures from the first state after `Submitted` / `ScheduledToRestart` (normally `DriverRequested`) instead of the initializing state itself. The initializing timestamp and the history are untouched, so restart backoff keeps its base. Time spent suspended, and time spent in restart backoff, no longer counts as a successful run. Added `testTimeHeldInScheduledToRestartDoesNotResetRestartCounter` for both trimmed and untrimmed histories (2h hold in `ScheduledToRestart`, resume, immediate failure, `maxRestartAttempts: 1` with a 1h reset window ends in `ResourceReleased`), and updated `testCalculateCurrentAttemptDuration` for the new start point. The `Restart Counter reset` docs mention the measurement point.
--
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-59542] Support `spec.suspend` to hold driver and master/worker creation in `AppInitStep` and `ClusterInitStep` [spark-kubernetes-operator]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #828:
URL: https://github.com/apache/spark-kubernetes-operator/pull/828#discussion_r4020769438
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/reconciler/reconcilesteps/AppInitStep.java:
##########
@@ -67,6 +67,10 @@ public ReconcileProgress reconcile(
return proceed();
}
SparkApplication app = context.getResource();
+ if (app.getSpec().isSuspend()) {
Review Comment:
The assumption that the driver pod name carries the attempt ID does not hold for all supported configurations.
`SparkAppSubmissionWorker.buildDriverConf()` preserves a user-specified `spark.app.id` through `setIfMissing()`, and `SparkAppDriverConf.resourceNamePrefix()` returns that ID. The existing `checkAppIdWhenUserSpecifiedInSparkConf` test explicitly covers this behavior. Spark also allows `spark.kubernetes.driver.pod.name` to override the pod name directly.
Consequently, consecutive attempts can have the same expected pod name. If the previous attempt's pod remains in the informer cache after cleanup, this comparison returns `true` for a suspended `ScheduledToRestart` application. Once backoff has elapsed, initialization proceeds; if the old pod has already disappeared from the API server, it can create a new driver despite `suspend: true`.
Could we identify the current attempt independently of a potentially reused pod name, or explicitly validate any naming restrictions required by this approach? Please extend `previousAttemptDriverPodDoesNotBypassSuspend` to cover a fixed `spark.app.id` and an explicitly configured driver pod name. Its current use of two different names does not exercise this case.
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/reconciler/reconcilesteps/AppInitStep.java:
##########
@@ -148,6 +152,26 @@ public ReconcileProgress reconcile(
return attemptStatusUpdate(context, statusRecorder, updatedStatus, completeAndDefaultRequeue());
}
+ /**
+ * 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. The driver pod name carries the attempt id, so a pod left from a previous attempt does
+ * not match.
+ *
+ * @param context The SparkAppContext for the application.
+ * @return True if the driver pod of the current attempt exists, false otherwise.
+ */
+ private boolean isDriverRequested(SparkAppContext context) {
+ Optional<Pod> driverPod = context.getDriverPod();
+ return driverPod.isPresent()
Review Comment:
`context.getDriverPod()` filters by application/driver labels and then calls `findAny()`. The name comparison here happens after that selection.
If both an older attempt's pod and the current attempt's pod are present in the informer cache, this can select the older one and return `false` without checking the current pod. In the status-persistence failure scenario this helper is intended to recover, that leaves the application held even though its current driver exists.
Could we apply the current-attempt predicate before selecting a pod, rather than checking only the single pod returned by `getDriverPod()`? A regression test with both pods available, with the older pod encountered first, would cover this. The existing tests mock only one returned pod, so they cannot detect this selection issue.
--
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-59542] Support `spec.suspend` to hold driver and master/worker creation in `AppInitStep` and `ClusterInitStep` [spark-kubernetes-operator]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #828:
URL: https://github.com/apache/spark-kubernetes-operator/pull/828#discussion_r4021047657
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/reconciler/reconcilesteps/AppInitStep.java:
##########
@@ -67,6 +67,10 @@ public ReconcileProgress reconcile(
return proceed();
}
SparkApplication app = context.getResource();
+ if (app.getSpec().isSuspend()) {
Review Comment:
This handles a previous attempt's pod once the informer has observed its deletion, but the stale pre-deletion snapshot can still pass this filter.
The remaining sequence is:
1. Consecutive attempts reuse the driver pod name.
2. Cleanup successfully deletes the old pod and persists `ScheduledToRestart`.
3. With `suspend: true` and zero or elapsed backoff, the next reconcile still sees the old informer snapshot: the expected name and a null `deletionTimestamp`.
4. This helper returns the old pod, so `AppInitStep` bypasses the hold.
5. Initialization queries the API server, finds that the pod is gone, and creates a new driver despite suspend.
I agree that this is informer lag, but the missing-driver grace period does not make this path safe. That flow defers a decision and eventually performs live verification; this path immediately authorizes resource creation. The grace-period observer also does not run for `ScheduledToRestart`.
Could we perform a live lookup by the desired pod name before allowing the suspend bypass, require the pod to exist and not be terminating, and requeue if verification fails?
The regression test should keep the **pre-deletion** pod snapshot in the informer while the API server reports the pod absent, then verify that a suspended application creates no driver. The current test sets `deletionTimestamp` on the cached object, so it covers the already-updated cache rather than this remaining window.
--
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-59542] Support `spec.suspend` to hold driver and master/worker creation in `AppInitStep` and `ClusterInitStep` [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #828:
URL: https://github.com/apache/spark-kubernetes-operator/pull/828#issuecomment-5734405739
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-59542] Support `spec.suspend` to hold driver and master/worker creation in `AppInitStep` and `ClusterInitStep` [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun closed pull request #828: [SPARK-59542] Support `spec.suspend` to hold driver and master/worker creation in `AppInitStep` and `ClusterInitStep`
URL: https://github.com/apache/spark-kubernetes-operator/pull/828
--
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-59542] Support `spec.suspend` to hold driver and master/worker creation in `AppInitStep` and `ClusterInitStep` [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #828:
URL: https://github.com/apache/spark-kubernetes-operator/pull/828#issuecomment-5690574379
Thank you, @viirya and @peter-toth ! This is the first step and I'm going to add future steps. It will become more mature than the AS-IS status.
--
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-59542] Support `spec.suspend` to hold driver and master/worker creation in `AppInitStep` and `ClusterInitStep` [spark-kubernetes-operator]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #828:
URL: https://github.com/apache/spark-kubernetes-operator/pull/828#issuecomment-5685239752
I will take a look.
--
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-59542] Support `spec.suspend` to hold driver and master/worker creation in `AppInitStep` and `ClusterInitStep` [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #828:
URL: https://github.com/apache/spark-kubernetes-operator/pull/828#issuecomment-5684897938
Could you review this PR when you have some time, @viirya ?
--
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-59542] Support `spec.suspend` to hold driver and master/worker creation in `AppInitStep` and `ClusterInitStep` [spark-kubernetes-operator]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on code in PR #828:
URL: https://github.com/apache/spark-kubernetes-operator/pull/828#discussion_r4018588596
##########
docs/spark_custom_resources.md:
##########
@@ -525,6 +525,34 @@ Note that `ttlAfterStopMillis` applies to the app as well as its secondary resou
latter is smaller, then it takes higher precedence: operator would remove all resources related
to this app after `ttlAfterStopMillis`.
+## Suspend
+
+Both `SparkApplication` and `SparkCluster` support `.spec.suspend`. When it is set to `true`, the
+operator keeps the resource in its initializing state (`Submitted`, or `ScheduledToRestart` for an
+application that is scheduled to restart) and does not request the driver pod or the master / worker
+StatefulSets. Setting it back to `false` resumes the regular lifecycle.
Review Comment:
**Finding 6.** `Submitted` is not something the user can observe here. With the dedicated `Suspended` state deferred to a follow-up ([#issuecomment-5684180823](https://github.com/apache/spark-kubernetes-operator/pull/828#issuecomment-5684180823)), the docs are the only place that can say so.
For a `SparkApplication` created with `suspend: true`, no step in the pipeline writes a status. `AppValidateStep` persists only when the status is invalid and it never is. `AppCleanUpStep` returns `proceed()` for `Submitted`. The new branch returns before any persist. `toUpdateControl` is `noUpdate()`. So `.status` is absent on the API server and the `Current State` printer column is blank.
That is indistinguishable from "the operator is not watching this namespace". `tests/e2e/watched-namespaces/chainsaw-test.yaml:54-58` asserts `.status == null` as the signature of exactly that case.
Worth stating precisely, because the paragraph is only wrong for one of the two holds. An application suspended later, in `ScheduledToRestart`, does have a status on the server. `AppCleanUpStep` wrote it before the app got there.
```suggestion
StatefulSets. Setting it back to `false` resumes the regular lifecycle.
`Submitted` here is the operator's in-memory view. A resource created with `suspend: true` has no
`.status` on the API server at all, so `kubectl get` shows an empty `Current State` until it
resumes. An application suspended later, in `ScheduledToRestart`, keeps the status its previous
attempt already wrote.
```
##########
tests/e2e/suspend/spark-application-suspended.yaml:
##########
@@ -0,0 +1,24 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+apiVersion: spark.apache.org/v1
+kind: SparkApplication
+metadata:
+ name: spark-job-suspend-test
+ namespace: default
+spec:
+ suspend: true
Review Comment:
**Finding 7.** This file, and `spark-cluster-suspended.yaml`, now assert only `spec.suspend: true`. That is the value `spark-example-suspend.yaml` applied two steps earlier, on a field the operator never writes. The assertion cannot fail.
Dropping the `status:` blocks was the right call. What is left is a 24-line file that adds nothing on top of the `error:` checks below it.
Two options. Delete both files and let the `error:` checks carry the step. Or make them assert the hold, using the idiom already in the tree at `tests/e2e/watched-namespaces/chainsaw-test.yaml:54-58`:
```yaml
- script:
timeout: 30s
content:
kubectl get sparkapplication spark-job-suspend-test -n default -o json | jq ".status"
check:
(contains($stdout, 'null')): true
```
That pins today's behavior positively rather than by the absence of a pod. It will need updating when the `Suspended` state lands, which seems right. That follow-up is exactly the kind of change this test should notice.
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/reconciler/reconcilesteps/AppInitStep.java:
##########
@@ -67,6 +67,10 @@ public ReconcileProgress reconcile(
return proceed();
}
SparkApplication app = context.getResource();
+ if (app.getSpec().isSuspend()) {
Review Comment:
**Finding 8.** The gate asks "is the app initializing and suspended?", not "has anything been requested yet?". Those two differ in one window, and `appInitStepShouldBeIdempotentWhenStatusUpdateFails` (`AppInitStepTest.java:206`) exists because the codebase already knows about it.
The sequence that produces it:
1. Reconcile N creates the pre-resources, the driver pod and the driver resources.
2. `attemptStatusUpdate` fails to persist `DriverRequested` and returns `completeAndImmediateRequeue()`.
3. `StatusRecorder.persistStatus` never updated `statusCache`, so `updateStatusFromCache` puts the resource back to `Submitted` on reconcile N+1.
4. A driver pod is live and the app reads as `Submitted`.
An operator restart between the pod create and the status patch lands in the same place. The server has no `.status`, so `initStatus()` hands back a fresh `Submitted`.
If `suspend` goes to `true` in that window, this branch fires and the app never leaves `Submitted`. `SparkAppReconciler.getReconcileSteps` adds only `AppInitStep` for `Submitted`, so no observer and no timeout ever looks at that pod. It runs to completion unnoticed and the resource is held forever. That contradicts `docs/spark_custom_resources.md:548`, "`suspend` only takes effect before the driver ... resources are requested".
I reproduced it as a unit test at head. Reconcile 1 with `persistStatus` returning `false`, reset the status the way `updateStatusFromCache` would, set `suspend`, reconcile 2:
```java
ReconcileProgress p2 = appInitStep.reconcile(ctx, recorder);
Assertions.assertEquals(ReconcileProgress.completeAndDefaultRequeue(), p2);
Assertions.assertEquals(
ApplicationStateSummary.Submitted,
application.getStatus().getCurrentState().getCurrentStateSummary());
// the driver pod from reconcile 1 is still there
Assertions.assertNotNull(
kubernetesClient.pods().inNamespace("default").withName("driver-pod").get());
```
It passes, so the hold wins over the live driver.
The narrow fix here is `if (app.getSpec().isSuspend() && context.getDriverPod().isEmpty())`, which makes the code match the documented contract. One caveat I could not rule out. `getDriverPod()` reads the informer cache, and in `ScheduledToRestart` a just-deleted pod from the previous attempt could still be in it, which would make the operator start a driver for a suspended app.
If that risk is not worth taking in this PR, the running-attempt suspend you have planned is the natural home for this window. A suspend that can stop a live attempt stops this one too. Worth carrying into that work rather than leaving it only in this thread.
##########
docs/spark_custom_resources.md:
##########
@@ -525,6 +525,34 @@ Note that `ttlAfterStopMillis` applies to the app as well as its secondary resou
latter is smaller, then it takes higher precedence: operator would remove all resources related
to this app after `ttlAfterStopMillis`.
+## Suspend
+
+Both `SparkApplication` and `SparkCluster` support `.spec.suspend`. When it is set to `true`, the
+operator keeps the resource in its initializing state (`Submitted`, or `ScheduledToRestart` for an
+application that is scheduled to restart) and does not request the driver pod or the master / worker
+StatefulSets. Setting it back to `false` resumes the regular lifecycle.
+
+``` yaml
+apiVersion: spark.apache.org/v1
+kind: SparkApplication
+metadata:
+ name: suspended-pi
+spec:
+ suspend: true
+ mainClass: "org.apache.spark.examples.SparkPi"
+ jars: "local:///opt/spark/examples/jars/spark-examples.jar"
+ runtimeVersions:
+ sparkVersion: "4.2.0"
+```
+
+* `suspend` only takes effect before the driver (or master / worker) resources are requested.
+ Setting it to `true` on a running application does not stop the current attempt. If the
+ application is configured to restart, the next attempt is held until `suspend` is set back to
+ `false`. Setting it to `true` on a running cluster has no effect in the current version.
+* Deleting a suspended resource works as usual.
+* This is the building block for external job queueing systems such as
+ [Kueue](https://kueue.sigs.k8s.io/), which admit a workload by flipping `suspend` to `false`.
Review Comment:
**Finding 9.** Reads as if Kueue does this today. `KueueWorkloadFactory` is the only code outside the two init steps that touches `spec.isSuspend()`, and it has no caller in `src/main`. Nothing creates a `Workload`, and nothing flips `suspend` back.
```suggestion
[Kueue](https://kueue.sigs.k8s.io/), which admit a workload by flipping `suspend` to `false`.
The operator does not integrate with such a system 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: 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-59542] Support `spec.suspend` to hold driver and master/worker creation in `AppInitStep` and `ClusterInitStep` [spark-kubernetes-operator]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #828:
URL: https://github.com/apache/spark-kubernetes-operator/pull/828#discussion_r4018838860
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/reconciler/reconcilesteps/AppInitStep.java:
##########
@@ -67,6 +67,10 @@ public ReconcileProgress reconcile(
return proceed();
}
SparkApplication app = context.getResource();
+ if (app.getSpec().isSuspend()) {
Review Comment:
I agree with this finding, but I think it should be addressed before merging rather than deferred to support for suspending running attempts.
Resource creation and status persistence are separate operations. If the driver is created but persisting `DriverRequested` fails, the next reconciliation restores `Submitted` from the status cache. An operator restart between those operations produces the same discrepancy.
If `suspend` becomes `true` at that point, this branch prevents initialization recovery. `SparkAppReconciler` selects only validation, cleanup, and initialization for `Submitted`, so the existing driver is no longer observed for completion or timeout while the flag remains set. The equivalent window also exists in `ClusterInitStep` between resource creation and persisting `RunningHealthy`.
Could we distinguish an initialization that has already started from one that has not requested resources yet, and allow the former to recover? Please add a regression test covering successful resource creation, failed status persistence, and then enabling suspend.
I would avoid fixing this solely with `getDriverPod().isEmpty()`: that lookup uses the informer cache, and the driver labels do not distinguish attempts, so a previous attempt's pod could incorrectly bypass the hold.
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/reconciler/reconcilesteps/AppInitStep.java:
##########
@@ -67,6 +67,10 @@ public ReconcileProgress reconcile(
return proceed();
}
SparkApplication app = context.getResource();
+ if (app.getSpec().isSuspend()) {
+ log.debug("Application is suspended, driver resources would not be requested.");
+ return completeAndDefaultRequeue();
Review Comment:
Holding the application here also preserves the initializing state's timestamp. This affects the existing restart-counter logic: `ApplicationStatus.calculateCurrentAttemptDuration()` measures from the latest `Submitted` or `ScheduledToRestart`, and `terminateOrRestart()` applies the time-based counter reset before checking retry limits.
For example, with `maxRestartAttempts: 1` and `restartCounterResetMillis: 3600000`, hold the permitted retry in `ScheduledToRestart` for over an hour, then resume it and let it fail immediately. The suspended hour satisfies the reset threshold, so the counter resets and another retry is allowed instead of exhausting the configured limit.
This makes time spent waiting for admission count toward the documented reset for a long-running attempt. Could we exclude suspended time from that calculation and cover this sequence with both trimmed and untrimmed histories?
Simply changing the initializing timestamp on resume would also change restart-backoff and history semantics, so those should be preserved when addressing this.
##########
docs/spark_custom_resources.md:
##########
@@ -525,6 +525,34 @@ Note that `ttlAfterStopMillis` applies to the app as well as its secondary resou
latter is smaller, then it takes higher precedence: operator would remove all resources related
to this app after `ttlAfterStopMillis`.
+## Suspend
+
+Both `SparkApplication` and `SparkCluster` support `.spec.suspend`. When it is set to `true`, the
+operator keeps the resource in its initializing state (`Submitted`, or `ScheduledToRestart` for an
+application that is scheduled to restart) and does not request the driver pod or the master / worker
+StatefulSets. Setting it back to `false` resumes the regular lifecycle.
Review Comment:
This distinction is still missing from the documentation at the current head. A valid resource created with `suspend: true` does not have its initial status persisted, so `Submitted` is an in-memory state and the `Current State` column remains blank. An application held in `ScheduledToRestart`, however, retains the status already written by cleanup.
Deferring a dedicated `Suspended` state seems reasonable for this PR, but please document the observable behavior explicitly. For example:
> For a valid resource created with `suspend: true`, the initial `Submitted` status is not persisted to the API server, so `kubectl get` shows an empty `Current State` until initialization resumes. An application held in `ScheduledToRestart` retains its previously persisted status.
The PR description's statement that a held resource has no `.status` should likewise be limited to the initial-submission case.
--
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-59542] Support `spec.suspend` to hold driver and master/worker creation in `AppInitStep` and `ClusterInitStep` [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #828:
URL: https://github.com/apache/spark-kubernetes-operator/pull/828#issuecomment-5689696036
Thank you, @viirya. You are right that the cached `deletionTimestamp` only covers a cache that has already observed the delete, and that this path authorizes creation immediately rather than deferring. Addressed in 18636ac.
**Live verification before the bypass**: `SparkAppContext.getCurrentAttemptDriverPod()` still uses the informer cache to find a candidate (driver labels, the desired driver pod name, no `deletionTimestamp`), but it now verifies that candidate against the API server with a `GET` by the desired pod name and returns it only if the pod exists there and is not terminating. A stale pre-deletion snapshot therefore no longer bypasses the hold: the live lookup finds nothing, `AppInitStep` keeps the hold and requeues at the regular interval. A `KubernetesClientException` during the lookup is logged and treated as absent, so an API error keeps the hold for that reconciliation instead of authorizing creation. When the cache has no candidate, neither the driver spec is built nor the API is called, so a held application still costs nothing per reconcile.
**Regression coverage**:
- `AppInitStepTest.staleInformerSnapshotDoesNotBypassSuspend` runs the step against a real `SparkAppContext` whose informer stream returns the pre-deletion snapshot (same name, no `deletionTimestamp`) while the mock API server has no such pod, with `suspend: true` and an elapsed backoff in `ScheduledToRestart`. It asserts that no driver pod is created, no status is persisted and the app stays in `ScheduledToRestart`.
- `SparkAppContextTest` adds `stalePreDeletionSnapshotIsNotCurrentAttemptDriver`, `podTerminatingOnApiServerIsNotCurrentAttemptDriver` and `apiErrorDuringVerificationIsTreatedAsAbsent`, and checks that no API call is made when the cache has no matching candidate.
The javadoc and the PR description describe the verification step. The Java 21 CI failure on the previous run was `SparkOperatorConfigMapReconcilerTest.initializationError` failing to download the kubeapitest binary, unrelated to this change.
--
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-59542] Support `spec.suspend` to hold driver and master/worker creation in `AppInitStep` and `ClusterInitStep` [spark-kubernetes-operator]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on code in PR #828:
URL: https://github.com/apache/spark-kubernetes-operator/pull/828#discussion_r4017891284
##########
tests/e2e/suspend/spark-application-suspended.yaml:
##########
@@ -0,0 +1,30 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+apiVersion: spark.apache.org/v1
+kind: SparkApplication
+metadata:
+ name: spark-job-suspend-test
+ namespace: default
+spec:
+ suspend: true
+status:
Review Comment:
**Finding 1.** Nothing in a suspended reconcile ever writes `.status` to the API server, so this assertion (and the `spark-cluster-suspended.yaml` twin) has no `status` to match.
The trace, for a `SparkApplication` created with `suspend: true`:
- `AppValidateStep` persists only when the status is *invalid*, and it never is. `CustomResource`'s constructor calls `initStatus()`, so `getStatus()` returns a `Submitted` `ApplicationStatus` even for a CR the server stored without one. `isValidApplicationStatus` passes and the step returns `proceed()`.
- `AppCleanUpStep` returns `proceed()` for `Submitted` without persisting.
- the new branch returns `completeAndDefaultRequeue()` before any persist.
- `ReconcilerUtils.toUpdateControl` returns `UpdateControl.noUpdate()`, and the CRD declares `subresources: status: {}`, so JOSDK writes nothing either.
`SparkCluster` is the same, except `ClusterValidateStep` is an unconditional `proceed()`, so there is not even a reset path.
I ran the three steps of each pipeline against a mock recorder:
```java
SparkApplication app = new SparkApplication();
app.setMetadata(new ObjectMetaBuilder().withName("a").withNamespace("default").build());
app.getSpec().setSuspend(true);
SparkAppContext ctx = mock(SparkAppContext.class);
when(ctx.getResource()).thenReturn(app);
SparkAppStatusRecorder recorder = mock(SparkAppStatusRecorder.class);
new AppValidateStep().reconcile(ctx, recorder);
new AppCleanUpStep().reconcile(ctx, recorder);
new AppInitStep().reconcile(ctx, recorder);
verifyNoInteractions(recorder); // passes; the ClusterValidate/Terminated/Init trio passes too
```
So a held resource has no `status` on the server and an empty `Current State` printer column. Chainsaw then fails here on `status.currentState`, and on `(*.currentStateSummary)` over a missing `stateTransitionHistory`. I have no cluster here, so that last hop is read from the assertion files rather than observed.
Two ways out:
- Persist the initial status once when entering the hold, so the resource really is `Submitted` on the server. Careful: `StatusRecorder.updateStatusFromCache` seeds `statusCache` with the current status on a cache miss, and `patchAndStatusWithVersionLocked` short-circuits on `newStatusNode.equals(previousStatusNode)`. A plain `persistStatus(context, app.getStatus())` in the new branch is therefore dropped silently on the first reconcile.
- Or drop the `status:` blocks from `spark-application-suspended.yaml` and `spark-cluster-suspended.yaml` and assert only `spec.suspend: true` plus the existing `error:` checks on the driver pod and the StatefulSets.
The first also fixes the operator-facing half. With Kueue holding a queue of workloads, `kubectl get sparkapp` currently shows a blank state for every one of them.
##########
docs/spark_custom_resources.md:
##########
@@ -525,6 +525,32 @@ Note that `ttlAfterStopMillis` applies to the app as well as its secondary resou
latter is smaller, then it takes higher precedence: operator would remove all resources related
to this app after `ttlAfterStopMillis`.
+## Suspend
+
+Both `SparkApplication` and `SparkCluster` support `.spec.suspend`. When it is set to `true`, the
+operator keeps the resource in its initializing state (`Submitted`, or `ScheduledToRestart` for an
+application that is scheduled to restart) and does not request the driver pod or the master / worker
+StatefulSets. Setting it back to `false` resumes the regular lifecycle.
+
+``` yaml
+apiVersion: spark.apache.org/v1
+kind: SparkApplication
+metadata:
+ name: suspended-pi
+spec:
+ suspend: true
+ mainClass: "org.apache.spark.examples.SparkPi"
+ jars: "local:///opt/spark/examples/jars/spark-examples.jar"
+ runtimeVersions:
+ sparkVersion: "4.2.0"
+```
+
+* `suspend` only takes effect before the driver (or master / worker) resources are requested.
+ Setting it to `true` on a running application or cluster has no effect in the current version.
Review Comment:
**Finding 2.** Not true for a `SparkApplication` configured with a restart policy.
`AppCleanUpStep` has no suspend check, so a running attempt that fails still goes through `terminateOrRestart` and lands in `ScheduledToRestart` (`AppCleanUpStep.java:186-193`). The next reconcile hits the new gate and holds there until someone flips the flag back. `suspendedAppScheduledToRestartDoesNotRequestDriver` asserts exactly that hold. So `suspend: true` on a running app does not stop the current attempt, but it does stop every attempt after it.
For `SparkCluster` the bullet is accurate: `SparkClusterReconciler.getReconcileSteps` only adds `ClusterInitStep` for `Submitted`, and a `RunningHealthy` cluster never goes back there.
```suggestion
Setting it to `true` on a running application does not stop the current attempt. If the
application is configured to restart, the next attempt is held until `suspend` is set back to
`false`. Setting it to `true` on a running cluster has no effect in the current version.
```
The PR description's "Suspending an already running application or cluster is out of scope" reads the same way and is worth the same correction.
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/reconciler/reconcilesteps/ClusterInitStep.java:
##########
@@ -59,6 +59,10 @@ public ReconcileProgress reconcile(
return proceed();
}
SparkCluster cluster = context.getResource();
+ if (cluster.getSpec().isSuspend()) {
+ log.info("Cluster is suspended, master and worker resources would not be requested.");
Review Comment:
**Finding 4.** A held resource logs this on every reconcile, so once per `spark.kubernetes.operator.reconciler.intervalSeconds` (120s by default) for as long as it stays suspended. With Kueue holding a few hundred workloads that is steady INFO traffic for a no-op. The other steady-state no-op paths report at debug (`AppCleanUpStep.java:132`, `AppReconcileStep.java:86`). Same line at `AppInitStep.java:71`.
##########
spark-operator/src/test/java/org/apache/spark/k8s/operator/reconciler/reconcilesteps/AppInitStepTest.java:
##########
@@ -342,4 +344,102 @@ void nonTrimModeRestartBackoffActiveRequeuesWithDelay() {
ApplicationStateSummary.ScheduledToRestart,
application.getStatus().getCurrentState().getCurrentStateSummary());
}
+
+ @Test
+ void suspendedAppDoesNotRequestDriver() {
+ AppInitStep appInitStep = new AppInitStep();
+ SparkAppContext mockContext = mock(SparkAppContext.class);
+ SparkAppStatusRecorder recorder = mock(SparkAppStatusRecorder.class);
+ SparkApplication application = new SparkApplication();
+ application.setMetadata(applicationMetadata);
+ application.getSpec().setSuspend(true);
+ when(mockContext.getResource()).thenReturn(application);
+
+ ReconcileProgress progress = appInitStep.reconcile(mockContext, recorder);
+
+ Assertions.assertEquals(ReconcileProgress.completeAndDefaultRequeue(), progress);
+ verify(mockContext, never()).getDriverPreResourcesSpec();
+ verify(mockContext, never()).getDriverPodSpec();
+ verify(mockContext, never()).getClient();
+ verifyNoInteractions(recorder);
+ Assertions.assertEquals(
+ ApplicationStateSummary.Submitted,
+ application.getStatus().getCurrentState().getCurrentStateSummary());
+ }
+
+ @Test
+ void suspendedAppScheduledToRestartDoesNotRequestDriver() {
+ // ScheduledToRestart with an elapsed backoff: suspend takes precedence over restart.
+ AppInitStep appInitStep = new AppInitStep();
+ SparkAppContext mockContext = mock(SparkAppContext.class);
+ SparkAppStatusRecorder recorder = mock(SparkAppStatusRecorder.class);
+ SparkApplication application = new SparkApplication();
+ application.setMetadata(applicationMetadata);
+ application.getSpec().setSuspend(true);
+ application.getSpec().setApplicationTolerations(
+ ApplicationTolerations.builder()
+ .restartConfig(RestartConfig.builder().restartBackoffMillis(5000L).build())
+ .build());
+ ApplicationState timedOutState =
+ new ApplicationState(ApplicationStateSummary.DriverStartTimedOut, "timed out");
+ ApplicationState scheduledState =
+ new ApplicationState(ApplicationStateSummary.ScheduledToRestart, "restarting");
+ scheduledState.setLastTransitionTime(Instant.now().minusMillis(60000L).toString());
+ Map<Long, ApplicationState> history = new TreeMap<>();
+ history.put(0L, timedOutState);
+ history.put(1L, scheduledState);
+ application.setStatus(new ApplicationStatus(
+ scheduledState, history,
+ new ApplicationAttemptSummary(), new ApplicationAttemptSummary()));
+ when(mockContext.getResource()).thenReturn(application);
+
+ ReconcileProgress progress = appInitStep.reconcile(mockContext, recorder);
+
+ Assertions.assertEquals(ReconcileProgress.completeAndDefaultRequeue(), progress);
+ verify(mockContext, never()).getDriverPodSpec();
+ verify(mockContext, never()).getClient();
+ verifyNoInteractions(recorder);
+ Assertions.assertEquals(
+ ApplicationStateSummary.ScheduledToRestart,
+ application.getStatus().getCurrentState().getCurrentStateSummary());
+ }
+
+ @Test
+ void unsuspendedAppRequestsDriverOnNextReconcile() {
+ AppInitStep appInitStep = new AppInitStep();
+ SparkAppContext mockContext = mock(SparkAppContext.class);
+ SparkAppStatusRecorder recorder = mock(SparkAppStatusRecorder.class);
+ SparkApplication application = new SparkApplication();
+ application.setMetadata(applicationMetadata);
+ application.getSpec().setSuspend(true);
+ when(mockContext.getResource()).thenReturn(application);
+ when(mockContext.getDriverPreResourcesSpec()).thenReturn(List.of());
+ when(mockContext.getDriverPodSpec()).thenReturn(driverPodSpec);
+ when(mockContext.getDriverResourcesSpec()).thenReturn(List.of());
+ when(mockContext.getClient()).thenReturn(kubernetesClient);
+ when(recorder.persistStatus(any(), any())).thenAnswer(invocation -> {
+ ApplicationStatus newStatus = invocation.getArgument(1);
+ application.setStatus(newStatus);
+ return true;
+ });
+
+ // Suspended: nothing is created and the app stays Submitted
+ ReconcileProgress progress1 = appInitStep.reconcile(mockContext, recorder);
+ Assertions.assertEquals(ReconcileProgress.completeAndDefaultRequeue(), progress1);
+ Assertions.assertNull(
+ kubernetesClient.pods().inNamespace("default").withName("driver-pod").get());
+ Assertions.assertEquals(
+ ApplicationStateSummary.Submitted,
+ application.getStatus().getCurrentState().getCurrentStateSummary());
+
+ // Unsuspended: the regular init path requests the driver
+ application.getSpec().setSuspend(false);
+ ReconcileProgress progress2 = appInitStep.reconcile(mockContext, recorder);
+ Assertions.assertEquals(ReconcileProgress.completeAndDefaultRequeue(), progress2);
+ Assertions.assertNotNull(
+ kubernetesClient.pods().inNamespace("default").withName("driver-pod").get());
+ Assertions.assertEquals(
+ ApplicationStateSummary.DriverRequested,
+ application.getStatus().getCurrentState().getCurrentStateSummary());
+ }
Review Comment:
**Finding 3.** Worth an app-side twin of `ClusterInitStepTest.nonInitializingClusterProceeds`: a `RunningHealthy` app with `suspend: true` must still return `proceed()`. That is the boundary finding 2 is about, and right now it is pinned only for `SparkCluster`.
I ran this against the PR head and it passes:
```java
@Test
void suspendedNonInitializingAppProceeds() {
AppInitStep appInitStep = new AppInitStep();
SparkAppContext mockContext = mock(SparkAppContext.class);
SparkAppStatusRecorder recorder = mock(SparkAppStatusRecorder.class);
SparkApplication application = new SparkApplication();
application.setMetadata(applicationMetadata);
application.getSpec().setSuspend(true);
application.setStatus(
application
.getStatus()
.appendNewState(
new ApplicationState(ApplicationStateSummary.RunningHealthy, "running")));
when(mockContext.getResource()).thenReturn(application);
Assertions.assertEquals(
ReconcileProgress.proceed(), appInitStep.reconcile(mockContext, recorder));
verifyNoInteractions(recorder);
}
```
##########
tests/e2e/suspend/spark-application-state-transition.yaml:
##########
@@ -0,0 +1,34 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+apiVersion: spark.apache.org/v1
+kind: SparkApplication
+metadata:
+ name: spark-job-suspend-test
+ namespace: default
+spec:
+ suspend: false
+status:
+ stateTransitionHistory:
Review Comment:
**Finding 5.** This is `tests/e2e/assertions/spark-application/spark-state-transition.yaml` with a different resource name, and `spark-cluster-state-transition.yaml` is the cluster one. AGENTS.md puts shared assertions in `tests/e2e/assertions/`. Binding the name there (`name: ($SPARK_APPLICATION_NAME)`, as the namespace already is) would let this group reuse both files the way `state-transition/chainsaw-test.yaml` does, leaving only the two `*-suspended.yaml` files here. The extra `spec.suspend: false` check can move into an inline `resource:` assert.
--
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-59542] Support `spec.suspend` to hold driver and master/worker creation in `AppInitStep` and `ClusterInitStep` [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #828:
URL: https://github.com/apache/spark-kubernetes-operator/pull/828#issuecomment-5684180823
Thank you for the review, @peter-toth. All five findings are addressed in 5351a9a.
1. **No `.status` while suspended**: confirmed. `updateStatusFromCache` seeds the cache with the in-memory `Submitted` status and `patchAndStatusWithVersionLocked` short-circuits on equality, so a plain `persistStatus` in the hold branch is dropped. I took the second option for this PR: the two `*-suspended.yaml` assertions now check `spec.suspend: true` plus the existing `error:` checks on the driver pod and the StatefulSets. A dedicated `Suspended` state, which also fixes the blank state column, is planned as a follow-up together with the running-attempt suspend.
2. **Doc**: applied your suggestion. The PR description is updated the same way.
3. **App-side test**: added `AppInitStepTest.suspendedNonInitializingAppProceeds`.
4. **Log level**: both hold branches now log at `debug`.
5. **Shared assertions**: `spark-state-transition.yaml`, `spark-cluster-state-transition.yaml` and `spark-cluster-worker-statefulset.yaml` now take `($SPARK_APPLICATION_NAME)` / `($SPARK_CLUSTER_NAME)`, and `tests/e2e/suspend` reuses them. The `watched-namespaces` and `watched-namespaces-file` groups pass the new binding explicitly. The `spec.suspend: false` check moved into an inline `resource:` assert.
--
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-59542] Support `spec.suspend` to hold driver and master/worker creation in `AppInitStep` and `ClusterInitStep` [spark-kubernetes-operator]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #828:
URL: https://github.com/apache/spark-kubernetes-operator/pull/828#discussion_r4021376209
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/reconciler/reconcilesteps/AppInitStep.java:
##########
@@ -67,6 +67,10 @@ public ReconcileProgress reconcile(
return proceed();
}
SparkApplication app = context.getResource();
+ if (app.getSpec().isSuspend()) {
Review Comment:
Confirmed resolved in `18636ac`. The live GET prevents the stale pre-deletion snapshot from authorizing initialization, and lookup failures keep the hold. `staleInformerSnapshotDoesNotBypassSuspend` now covers the specific sequence discussed here and verifies that no driver is created and the application stays in `ScheduledToRestart`. Thanks for adding this coverage.
--
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-59542] Support `spec.suspend` to hold driver and master/worker creation in `AppInitStep` and `ClusterInitStep` [spark-kubernetes-operator]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #828:
URL: https://github.com/apache/spark-kubernetes-operator/pull/828#issuecomment-5689157868
Thank you for the careful follow-up, @viirya. Both points are addressed in 67d1cfc.
**Selection before the name check**: `SparkAppContext` now has `getCurrentAttemptDriverPod()`. It collects every driver-labeled pod from the informer cache and applies the current-attempt predicate to all of them before selecting one, instead of checking only the single pod that `getDriverPod().findAny()` happens to return. `AppInitStep` uses that method; the name comparison is gone from the step. `SparkAppContextTest.currentAttemptDriverPodIsFoundBehindPreviousAttemptPod` has the older pod first in the stream and asserts the current one is found.
**Reused pod names (`spark.app.id` / `spark.kubernetes.driver.pod.name`)**: rather than assuming the name carries the attempt id, the predicate now requires the pod to have the desired driver pod spec's name **and** no `deletionTimestamp`. `AppCleanUpStep` deletes the previous attempt's driver before the application transitions to `ScheduledToRestart`, and `deleteResourceIfExists` rethrows anything but 404 so the transition cannot happen without the delete being accepted. Once the app is in `ScheduledToRestart`, the previous attempt's pod is therefore either gone or terminating, even when its name is identical to the current spec's. `Submitted` has no previous attempt. `previousAttemptDriverPodDoesNotBypassSuspend` now uses a terminating pod with the very same name as the spec, and `SparkAppContextTest.terminatingPreviousAttemptPodWithSameNameIsNotCurrentAttemptDriver` covers the selection for that case. The remaining window is an informer cache that has not yet reflected the acce
pted delete, which is the same class of lag the missing-driver grace period already tolerates.
I considered labeling the driver pod with the attempt id, which would make the identification exact regardless of naming. That changes every driver pod the operator creates and is independent of `spec.suspend`, so it is better done as its own change if we want it. The javadoc and the PR description describe the current rule.
The `pi-with-gluten` failure on the previous run was a state-transition timeout in that group's own assertion, unrelated to this change.
--
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