fqshopify commented on code in PR #1154:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/1154#discussion_r3579993452
##########
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:
Fixed
##########
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:
Fixed
##########
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:
> The delete (child) and the finalize status patch (parent) are not atomic.
Very true. I like your suggestion: once the previous deployment is gone
mid-transition, treat that as the point of no return and always finalize
forward instead of aborting.
I think that's a significant enough change to warrant it's own PR though so
let's handle it separately.
--
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]
