wangyang0918 commented on a change in pull request #131:
URL:
https://github.com/apache/flink-kubernetes-operator/pull/131#discussion_r840312565
##########
File path:
flink-kubernetes-operator/src/test/java/org/apache/flink/kubernetes/operator/controller/FlinkDeploymentControllerTest.java
##########
@@ -397,7 +402,59 @@ public void testUpgradeNotReadyCluster() {
testUpgradeNotReadyCluster(appCluster, false);
}
- public void testUpgradeNotReadyCluster(FlinkDeployment appCluster, boolean
allowUpgrade) {
+ @Test
+ public void verifyReconcileWithBadConfig() {
+
+ FlinkDeployment appCluster = TestUtils.buildApplicationCluster();
+ UpdateControl<FlinkDeployment> updateControl;
+ // Override rest port, and it should be saved in lastReconciledSpec
once a successful
+ // reconcile() finishes.
+
appCluster.getSpec().getFlinkConfiguration().put(RestOptions.PORT.key(),
"8088");
+ updateControl = testController.reconcile(appCluster, context);
+ assertTrue(updateControl.isUpdateStatus());
+ assertEquals(
+ JobManagerDeploymentStatus.DEPLOYING,
+ appCluster.getStatus().getJobManagerDeploymentStatus());
+
+ // Check when the bad config is applied, observe() will change the
cluster state correctly
+ appCluster.getSpec().getJobManager().setReplicas(-1);
+ // Next reconcile will set error msg and observe with previous
validated config
+ updateControl = testController.reconcile(appCluster, context);
+ assertEquals(
+ "JobManager replicas should not be configured less than one.",
+ appCluster.getStatus().getReconciliationStatus().getError());
+ assertTrue(updateControl.isUpdateStatus());
+ assertEquals(
+ JobManagerDeploymentStatus.DEPLOYED_NOT_READY,
+ appCluster.getStatus().getJobManagerDeploymentStatus());
+
+ // Check the exception
+ appCluster.getSpec().getJobManager().setReplicas(1);
+ appCluster.getSpec().getJob().setJarURI(null);
+ assertDoesNotThrow(
+ () -> {
+ testController.reconcile(appCluster, context);
+ });
+ assertEquals(
+ JobManagerDeploymentStatus.READY,
+ appCluster.getStatus().getJobManagerDeploymentStatus());
+ // Verify the saved rest port in lastReconciledSpec is actually used
in observe().
+ // Otherwise, the NaN rest port
+ // would lead to exception
+
appCluster.getSpec().getFlinkConfiguration().put(RestOptions.PORT.key(), "NaN");
Review comment:
I am afraid we still not verify the observer is using last reconciled
config. I suggest to do it in the `TestingFlinkService#listJobs`. To achieve
that, we could set a consumer of `TestingFlinkService`, just like
`TestingClusterClient`.
##########
File path:
flink-kubernetes-operator/src/test/java/org/apache/flink/kubernetes/operator/controller/FlinkDeploymentControllerTest.java
##########
@@ -397,7 +402,59 @@ public void testUpgradeNotReadyCluster() {
testUpgradeNotReadyCluster(appCluster, false);
}
- public void testUpgradeNotReadyCluster(FlinkDeployment appCluster, boolean
allowUpgrade) {
+ @Test
+ public void verifyReconcileWithBadConfig() {
+
+ FlinkDeployment appCluster = TestUtils.buildApplicationCluster();
+ UpdateControl<FlinkDeployment> updateControl;
+ // Override rest port, and it should be saved in lastReconciledSpec
once a successful
+ // reconcile() finishes.
+
appCluster.getSpec().getFlinkConfiguration().put(RestOptions.PORT.key(),
"8088");
+ updateControl = testController.reconcile(appCluster, context);
+ assertTrue(updateControl.isUpdateStatus());
+ assertEquals(
+ JobManagerDeploymentStatus.DEPLOYING,
+ appCluster.getStatus().getJobManagerDeploymentStatus());
+
+ // Check when the bad config is applied, observe() will change the
cluster state correctly
+ appCluster.getSpec().getJobManager().setReplicas(-1);
+ // Next reconcile will set error msg and observe with previous
validated config
+ updateControl = testController.reconcile(appCluster, context);
+ assertEquals(
+ "JobManager replicas should not be configured less than one.",
+ appCluster.getStatus().getReconciliationStatus().getError());
+ assertTrue(updateControl.isUpdateStatus());
+ assertEquals(
+ JobManagerDeploymentStatus.DEPLOYED_NOT_READY,
+ appCluster.getStatus().getJobManagerDeploymentStatus());
+
+ // Check the exception
+ appCluster.getSpec().getJobManager().setReplicas(1);
+ appCluster.getSpec().getJob().setJarURI(null);
+ assertDoesNotThrow(
Review comment:
What's the purpose of this assert?
--
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]
