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