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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
