Dennis-Mircea commented on code in PR #1154:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/1154#discussion_r3561282587
##########
flink-kubernetes-operator/src/test/java/org/apache/flink/kubernetes/operator/controller/FlinkBlueGreenDeploymentControllerTest.java:
##########
@@ -558,6 +558,69 @@ public void verifyFailureDuringTransition(FlinkVersion
flinkVersion) throws Exce
testTransitionToGreen(rs, customValue, null);
}
+ @ParameterizedTest
+
@MethodSource("org.apache.flink.kubernetes.operator.TestUtils#flinkVersions")
+ public void verifyGreenNotAbortedWhenNotReadyAfterBlueDeleted(FlinkVersion
flinkVersion)
+ throws Exception {
+ var blueGreenDeployment =
+ buildSessionCluster(
+ TEST_DEPLOYMENT_NAME,
+ TEST_NAMESPACE,
+ flinkVersion,
+ null,
+ UpgradeMode.STATELESS);
+ // Tiny abort grace so the abort deadline is already expired by the
time Blue is deleted.
+ var configuration = blueGreenDeployment.getSpec().getConfiguration();
+ configuration.put(ABORT_GRACE_PERIOD.key(), "1");
+ configuration.put(DEPLOYMENT_DELETION_DELAY.key(), "0");
+
+ var rs = executeBasicDeployment(flinkVersion, blueGreenDeployment,
false, null);
+
+ // Trigger the transition to Green
+ simulateChangeInSpec(rs.deployment, UUID.randomUUID().toString(), 0,
null);
+ rs = reconcile(rs.deployment);
+ assertEquals(
+ FlinkBlueGreenDeploymentState.TRANSITIONING_TO_GREEN,
+ rs.reconciledStatus.getBlueGreenState());
+ assertEquals(2, getFlinkDeployments().size());
+
+ // Green becomes ready -> marks the ready timestamp and schedules
deletion of Blue
+ simulateSuccessfulJobStart(getFlinkDeployments().get(1));
+ rs = reconcile(rs.deployment);
+
+ // Let the (zero) deletion delay elapse
+ Thread.sleep(10);
Review Comment:
It's best to avoid using `sleep` here. Consider using an `await()` function
instead.
##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/controller/bluegreen/BlueGreenDeploymentService.java:
##########
@@ -627,17 +627,20 @@ private UpdateControl<FlinkBlueGreenDeployment>
waitBeforeDeleting(
}
private UpdateControl<FlinkBlueGreenDeployment> deleteDeployment(
- FlinkDeployment currentDeployment, BlueGreenContext context) {
+ FlinkDeployment currentDeployment,
+ BlueGreenContext context,
+ FlinkBlueGreenDeploymentState nextState) {
boolean deleted = deleteFlinkDeployment(currentDeployment, context);
if (!deleted) {
LOG.info("FlinkDeployment '{}' not deleted, will retry",
currentDeployment);
+ return UpdateControl.<FlinkBlueGreenDeployment>noUpdate()
+ .rescheduleAfter(RETRY_DELAY_MS);
} else {
LOG.info("FlinkDeployment '{}' deleted!", currentDeployment);
+ return finalizeBlueGreenDeployment(context, nextState);
Review Comment:
One concurrency gap to consider: The delete (child) and the finalize status
patch (parent) are not atomic. If the reconcile crashes or the patch 409s after
the delete is accepted but before finalize persists, recovery lands in
Blue-gone + still `TRANSITIONING_TO_*` + abort timer set. The dispatch only
checks Green readiness, not whether Blue still exists
([BlueGreenDeploymentService.java#L489](https://github.com/apache/flink-kubernetes-operator/blob/main/flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/controller/bluegreen/BlueGreenDeploymentService.java#L489)).
That still allows a rollback to an already-deleted Blue, just far more
rarely than before. I'd close it structurally rather than rely on timing: once
the previous deployment is absent during `TRANSITIONING_TO_*`, treat that as
the point of no return and always finalize forward, never abort.
##########
flink-kubernetes-operator/src/test/java/org/apache/flink/kubernetes/operator/controller/FlinkBlueGreenDeploymentControllerTest.java:
##########
@@ -558,6 +558,69 @@ public void verifyFailureDuringTransition(FlinkVersion
flinkVersion) throws Exce
testTransitionToGreen(rs, customValue, null);
}
+ @ParameterizedTest
+
@MethodSource("org.apache.flink.kubernetes.operator.TestUtils#flinkVersions")
+ public void verifyGreenNotAbortedWhenNotReadyAfterBlueDeleted(FlinkVersion
flinkVersion)
Review Comment:
The test asserts state/abort-timer/count but not the ingress flip to green,
which is part of the cutover contract. I'd say it is worth adding.
--
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]
