You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by GitBox <gi...@apache.org> on 2022/09/19 14:50:20 UTC

[GitHub] [flink-kubernetes-operator] gyfora opened a new pull request, #371: [FLINK-29313] Observe spec should include latest operator configs + improved test coverage

gyfora opened a new pull request, #371:
URL: https://github.com/apache/flink-kubernetes-operator/pull/371

   ## What is the purpose of the change
   
   This PR improves the logic of the observe configuration to avoid problems with juggling operator specific confg between the new and deployed specs. We now ensure that operator related configs from the latest spec are always accounted for in the observe config.
   
   The PR includes additional test coverage to ensure that the correct config is used during JM deployment recovery + improves the savepoint cleanup logic.
   
   ## Brief change log
   
     - *Add latest operator related configs to observeconfig*
     - *Improve savepoint cleanup*
     - *Add tests to ensure that new operator configs are picked up in reconcile/observe even if they didnt lead to a job upgrade*
     - *Add config test for JM deployment recovery*
   
   ## Verifying this change
   
   Partly covered by existing tests and new tests have been added for the cases outlined in the ticket
   
   ## Does this pull request potentially affect one of the following parts:
   
     - Dependencies (does it add or upgrade a dependency): no
     - The public API, i.e., is any changes to the `CustomResourceDescriptors`: no
     - Core observer or reconciler logic that is regularly executed: yes
   
   ## Documentation
   
     - Does this pull request introduce a new feature? no
     - If yes, how is the feature documented? not applicable
   


-- 
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

[GitHub] [flink-kubernetes-operator] gyfora commented on a diff in pull request #371: [FLINK-29313] Observe spec should include latest operator configs + improved test coverage

Posted by GitBox <gi...@apache.org>.
gyfora commented on code in PR #371:
URL: https://github.com/apache/flink-kubernetes-operator/pull/371#discussion_r976182323


##########
flink-kubernetes-operator/src/test/java/org/apache/flink/kubernetes/operator/controller/RollbackTest.java:
##########
@@ -179,6 +181,17 @@ public void testRollbackFailureWithLastState(FlinkVersion flinkVersion) throws E
                     assertEquals("RUNNING", dep.getStatus().getJobStatus().getState());
                     assertEquals(1, flinkService.listJobs().size());
 
+                    // Trigger deployment recovery
+                    flinkService.clear();
+                    flinkService.setPortReady(false);
+
+                    testController.reconcile(dep, context);
+                    flinkService.setPortReady(true);
+                    testController.reconcile(dep, context);
+                    var jobs = flinkService.listJobs();
+                    // Make sure deployment was recovered with correct spec/config
+                    assertTrue(jobs.get(jobs.size() - 1).f2.containsKey("t"));

Review Comment:
   Not really, this change make it so that you pick up `kubernetes.operator.*` configs from the latest spec so even if you roll back you still pick the same.



-- 
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

[GitHub] [flink-kubernetes-operator] gyfora merged pull request #371: [FLINK-29313] Observe spec should include latest operator configs + improved test coverage

Posted by GitBox <gi...@apache.org>.
gyfora merged PR #371:
URL: https://github.com/apache/flink-kubernetes-operator/pull/371


-- 
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

[GitHub] [flink-kubernetes-operator] gyfora commented on pull request #371: [FLINK-29313] Observe spec should include latest operator configs + improved test coverage

Posted by GitBox <gi...@apache.org>.
gyfora commented on PR #371:
URL: https://github.com/apache/flink-kubernetes-operator/pull/371#issuecomment-1251131290

   cc @morhidi 


-- 
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

[GitHub] [flink-kubernetes-operator] gyfora commented on pull request #371: [FLINK-29313] Observe spec should include latest operator configs + improved test coverage

Posted by GitBox <gi...@apache.org>.
gyfora commented on PR #371:
URL: https://github.com/apache/flink-kubernetes-operator/pull/371#issuecomment-1251252453

   In the long run we probably need a small overhaul of the config handling to eliminate all weird corner cases and special handling. I opened a jira ticket and will work on this for 1.3.0:  https://issues.apache.org/jira/browse/FLINK-29338
   


-- 
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

[GitHub] [flink-kubernetes-operator] morhidi commented on a diff in pull request #371: [FLINK-29313] Observe spec should include latest operator configs + improved test coverage

Posted by GitBox <gi...@apache.org>.
morhidi commented on code in PR #371:
URL: https://github.com/apache/flink-kubernetes-operator/pull/371#discussion_r975778961


##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/crd/spec/AbstractFlinkSpec.java:
##########
@@ -51,8 +53,7 @@ public abstract class AbstractFlinkSpec implements Diffable<AbstractFlinkSpec> {
     /** Flink configuration overrides for the Flink deployment or Flink session job. */
     @SpecDiff.Config({
         @SpecDiff.Entry(prefix = "parallelism.default", type = DiffType.IGNORE),
-        @SpecDiff.Entry(prefix = "kubernetes.operator", type = DiffType.IGNORE),
-        @SpecDiff.Entry(prefix = "metrics.scope.k8soperator", type = DiffType.IGNORE)
+        @SpecDiff.Entry(prefix = K8S_OP_CONF_PREFIX, type = DiffType.IGNORE),

Review Comment:
   Shell we keep the the deprecated configs e.g. `metrics.scope.k8soperator` here for a while, or we simply say that deprecated operator configs will trigger an upgrade?



-- 
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

[GitHub] [flink-kubernetes-operator] morhidi commented on pull request #371: [FLINK-29313] Observe spec should include latest operator configs + improved test coverage

Posted by GitBox <gi...@apache.org>.
morhidi commented on PR #371:
URL: https://github.com/apache/flink-kubernetes-operator/pull/371#issuecomment-1251456662

   Thanks, will test this tomorrow. 


-- 
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

[GitHub] [flink-kubernetes-operator] gyfora commented on pull request #371: [FLINK-29313] Observe spec should include latest operator configs + improved test coverage

Posted by GitBox <gi...@apache.org>.
gyfora commented on PR #371:
URL: https://github.com/apache/flink-kubernetes-operator/pull/371#issuecomment-1253357595

   > Went through the PR, the situation with the configs actually not that bad. Just by looking at the code I was not able to figure out what happens with the operator configs in case of a rollback. I guess we can potentially loose some in case those are not kept in the new spec too.
   
   I don't think we have any problems with rollbacks because now we always use the latest operator related configs. 


-- 
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

[GitHub] [flink-kubernetes-operator] morhidi commented on a diff in pull request #371: [FLINK-29313] Observe spec should include latest operator configs + improved test coverage

Posted by GitBox <gi...@apache.org>.
morhidi commented on code in PR #371:
URL: https://github.com/apache/flink-kubernetes-operator/pull/371#discussion_r975795996


##########
flink-kubernetes-operator/src/test/java/org/apache/flink/kubernetes/operator/controller/RollbackTest.java:
##########
@@ -179,6 +181,17 @@ public void testRollbackFailureWithLastState(FlinkVersion flinkVersion) throws E
                     assertEquals("RUNNING", dep.getStatus().getJobStatus().getState());
                     assertEquals(1, flinkService.listJobs().size());
 
+                    // Trigger deployment recovery
+                    flinkService.clear();
+                    flinkService.setPortReady(false);
+
+                    testController.reconcile(dep, context);
+                    flinkService.setPortReady(true);
+                    testController.reconcile(dep, context);
+                    var jobs = flinkService.listJobs();
+                    // Make sure deployment was recovered with correct spec/config
+                    assertTrue(jobs.get(jobs.size() - 1).f2.containsKey("t"));

Review Comment:
   Do we loose every `kubernetes.operator*` setting for a job in case of a rollback?



-- 
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

[GitHub] [flink-kubernetes-operator] morhidi commented on a diff in pull request #371: [FLINK-29313] Observe spec should include latest operator configs + improved test coverage

Posted by GitBox <gi...@apache.org>.
morhidi commented on code in PR #371:
URL: https://github.com/apache/flink-kubernetes-operator/pull/371#discussion_r975795996


##########
flink-kubernetes-operator/src/test/java/org/apache/flink/kubernetes/operator/controller/RollbackTest.java:
##########
@@ -179,6 +181,17 @@ public void testRollbackFailureWithLastState(FlinkVersion flinkVersion) throws E
                     assertEquals("RUNNING", dep.getStatus().getJobStatus().getState());
                     assertEquals(1, flinkService.listJobs().size());
 
+                    // Trigger deployment recovery
+                    flinkService.clear();
+                    flinkService.setPortReady(false);
+
+                    testController.reconcile(dep, context);
+                    flinkService.setPortReady(true);
+                    testController.reconcile(dep, context);
+                    var jobs = flinkService.listJobs();
+                    // Make sure deployment was recovered with correct spec/config
+                    assertTrue(jobs.get(jobs.size() - 1).f2.containsKey("t"));

Review Comment:
   Do we loose any `kubernetes.operator*` setting for a job in case of a rollback?



-- 
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

[GitHub] [flink-kubernetes-operator] morhidi commented on pull request #371: [FLINK-29313] Observe spec should include latest operator configs + improved test coverage

Posted by GitBox <gi...@apache.org>.
morhidi commented on PR #371:
URL: https://github.com/apache/flink-kubernetes-operator/pull/371#issuecomment-1252899555

   Went through the PR, the situation with the configs actually not that bad. Just by looking at the code I was not able to figure out what happens with the operator configs in case of a rollback. I guess we can potentially loose some in case those are not kept in the new spec too.


-- 
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

[GitHub] [flink-kubernetes-operator] gyfora commented on a diff in pull request #371: [FLINK-29313] Observe spec should include latest operator configs + improved test coverage

Posted by GitBox <gi...@apache.org>.
gyfora commented on code in PR #371:
URL: https://github.com/apache/flink-kubernetes-operator/pull/371#discussion_r976322892


##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/crd/spec/AbstractFlinkSpec.java:
##########
@@ -51,8 +53,7 @@ public abstract class AbstractFlinkSpec implements Diffable<AbstractFlinkSpec> {
     /** Flink configuration overrides for the Flink deployment or Flink session job. */
     @SpecDiff.Config({
         @SpecDiff.Entry(prefix = "parallelism.default", type = DiffType.IGNORE),
-        @SpecDiff.Entry(prefix = "kubernetes.operator", type = DiffType.IGNORE),
-        @SpecDiff.Entry(prefix = "metrics.scope.k8soperator", type = DiffType.IGNORE)
+        @SpecDiff.Entry(prefix = K8S_OP_CONF_PREFIX, type = DiffType.IGNORE),

Review Comment:
   I think we can say that they trigger an upgrade to keep the code clean, I doubt that anyone would set/change these very often



-- 
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