Re: [PR] [FLINK-40470] Record spec before deployment attempt in savepoint redeploy [flink-kubernetes-operator]

["Copilot (via GitHub)" <gi...@apache.org>]

Copilot commented on code in PR #1216:
URL: https://github.com/apache/flink-kubernetes-operator/pull/1216#discussion_r4165999149


##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/reconciler/deployment/AbstractJobReconciler.java:
##########
@@ -189,8 +189,11 @@ protected boolean reconcileSpecChange(
         }
 
         if (currentJobState == JobState.SUSPENDED && desiredJobState == JobState.RUNNING) {
-            // We inherit the upgrade mode unless stateless upgrade requested
-            if (currentDeploySpec.getJob().getUpgradeMode() != UpgradeMode.STATELESS) {
+            // We inherit the upgrade mode unless stateless upgrade requested. A savepoint the user
+            // explicitly requested through initialSavepointPath is always restored, regardless of
+            // the upgrade mode of the spec.
+            if (currentDeploySpec.getJob().getUpgradeMode() != UpgradeMode.STATELESS
+                    || restoringFromInitialSavepoint(resource, lastReconciledSpec)) {

Review Comment:
   A failed savepoint redeploy now resumes through the generic suspended-to-running path, but that path does not execute the `markReconciledSpecAsStable()` at the end of `redeployWithSavepoint`. After the retry deploys successfully, a JobManager that misses the readiness timeout can therefore trigger rollback to the pre-redeploy stable spec, despite the public contract that savepoint redeploys cannot be rolled back. Mark the reconciled spec stable after this explicit-initial-savepoint restore (and cover the retry with rollback enabled).



##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/reconciler/deployment/AbstractJobReconciler.java:
##########
@@ -211,6 +214,21 @@ protected boolean reconcileSpecChange(
         return true;
     }
 
+    /**
+     * Checks whether the suspended job is to be restored from the savepoint the user explicitly
+     * requested through initialSavepointPath, recorded as the upgrade savepoint by a savepoint
+     * redeploy or the first deployment. Such a savepoint is honoured even for stateless specs,
+     * otherwise a savepoint redeploy requested while suspended, or one whose deployment attempt
+     * failed, would be replaced by an empty state restore.
+     */
+    private boolean restoringFromInitialSavepoint(CR resource, SPEC lastReconciledSpec) {
+        var initialSavepointPath = resource.getSpec().getJob().getInitialSavepointPath();

Review Comment:
   This should compare the recorded `initialSavepointPath`, not the current spec. That field is an ignored diff unless the redeploy nonce changes, so if a user edits or clears it after requesting a suspended redeploy but before resuming, the recorded upgrade path is still the requested restore point; reading the current value makes a stateless resume discard that path and start empty. Use `lastReconciledSpec` so later ignored edits do not rewrite the already-recorded operation.



-- 
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: issues-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org