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/11/14 20:29:26 UTC

[GitHub] [flink-kubernetes-operator] gyfora opened a new pull request, #439: [FLINK-29959] Use optimistic locking when updating the status

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

   ## What is the purpose of the change
   
   Currently the operator uses `patchStatus()` without any resource version locking to update the Flink resources. This means that the operator overwrites whatever status was there previously. This was convenient so far but with leader election it is much more likely to get into a situation where this can be problematic.
   
   This change introduces optimistic locking for the status which detects if someone externally modified the status without the reconciliation logic being aware of it.
   
   There are 2 main problems that we target here:
    1. Status updates by zombie operators who has last lost leadership but not realized/dead yet.
    2. Stale status received when a new leader starts
    
   **Why would these happen?**
   
   Zombie operator: 
   It could in theory happen that an operator loses leadership in a middle of reconciliation due to a very long GC pause (or some network issue or whatever) and the current CR reconcile loop continues while the new leader already started to reconcile this resource. This is very unlikely but can happen with leader election and a standby operator. In these cases we don't want to allow the old operator who lost leadership to be able to make any status updates. The new logic guarantees that if the new leader made any status update the old would never be able to do so again.
   
   Stale status:
   When the new leader starts processing (if it was on standby) there is no guarantee that the status/spec reconciled at the first time is up to date. This can happen because due to some unlucky cache update timing or even a zombie operator submitting late status updates. The current operator logic very much relies on seeing the last status otherwise we can have some very weird cornercases that would definitely cause problems for the resources.
   
   **How the new logic tackles this in a safe way**
    
   What the new logic does is that it basically only allows status updates to go through when the operator has the latest status information. So it's sort of a locking on the current status. If anyone else changed the status in the meantime, we simply throw an error and retrigger the reconciliation. This is actually safe to do as the operator reconcile logic already runs with the assumption that the operator can fail at any time before status update, and we always use the status as a "write-ahead-log" of the actions we are taking. In these cases zombie operators who have already lost leadership would never reconcile again (the leader election guarantees that), and in other cases this would give us the latest version of the resource.
   
   *Note:*
   It's not easy to write unit tests that validate the logic as the fabric8 mockserver/utilities do not work well with optimistic locking. See https://github.com/fabric8io/kubernetes-client/issues/4573 for details.
   
   ## Brief change log
   
     - *Add optimistic locking for status updates*
     - *Remove empty strings from status*
     - *Added new integration test*
   
   ## Verifying this change
   
    - Extended the operator IT case
    - Manually verified on minikube
    - e2es
   
   ## 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 pull request #439: [FLINK-29959] Use optimistic locking when updating the status

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

   cc @tweise @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 a diff in pull request #439: [FLINK-29959] Use optimistic locking when updating the status

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


##########
flink-kubernetes-operator-api/src/main/java/org/apache/flink/kubernetes/operator/api/status/SavepointInfo.java:
##########
@@ -63,7 +63,7 @@ public void setTrigger(
     }
 
     public void resetTrigger() {
-        this.triggerId = "";
+        this.triggerId = null;
         this.triggerTimestamp = 0L;

Review Comment:
   The benefit is that there would not be `triggerTimestamp` in the status when there is nothing triggered.
   
   I agree with @morhidi I will change this, originally I thought this would be a Java API breaking change but I believe Kubernetes never sends `null` values so it's not going to be a problem. 



-- 
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 #439: [FLINK-29959] Use optimistic locking when updating the status

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


##########
flink-kubernetes-operator-api/src/main/java/org/apache/flink/kubernetes/operator/api/status/SavepointInfo.java:
##########
@@ -63,7 +63,7 @@ public void setTrigger(
     }
 
     public void resetTrigger() {
-        this.triggerId = "";
+        this.triggerId = null;
         this.triggerTimestamp = 0L;

Review Comment:
   It'll keep already stored `0` values however, so no business logic should check `(triggerTimestamp == null)`, it'll be `0`



-- 
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 #439: [FLINK-29959] Use optimistic locking when updating the status

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

   @morhidi I updated the PR and I made some more savepoint triggering related fields nullable.


-- 
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 #439: [FLINK-29959] Use optimistic locking when updating the status

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


##########
flink-kubernetes-operator-api/src/main/java/org/apache/flink/kubernetes/operator/api/status/SavepointInfo.java:
##########
@@ -63,7 +63,7 @@ public void setTrigger(
     }
 
     public void resetTrigger() {
-        this.triggerId = "";
+        this.triggerId = null;
         this.triggerTimestamp = 0L;

Review Comment:
   Can't we use `Long` and `null`s as default for the timestamps as well (lastPeriodicSavepointTimestamp,triggerTimestamp) or it would break backward compatibility?



##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/utils/StatusRecorder.java:
##########
@@ -97,20 +96,79 @@ public void patchAndCacheStatus(CR resource) {
 
         Exception err = null;
         for (int i = 0; i < 3; i++) {
-            // In any case we retry the status update 3 times to avoid some intermittent
-            // connectivity errors if any
+            // We retry the status update 3 times to avoid some intermittent connectivity errors
             try {
-                client.resource(resource).patchStatus();
-                statusUpdateListener.accept(resource, prevStatus);
-                metricManager.onUpdate(resource);
-                return;
-            } catch (Exception e) {
+                replaceStatus(resource, prevStatus);
+                err = null;
+            } catch (KubernetesClientException e) {
                 LOG.error("Error while patching status, retrying {}/3...", (i + 1), e);
                 Thread.sleep(1000);
                 err = e;
             }
         }
-        throw err;
+

Review Comment:
   I trust you with this @gyfora. Do you think we should add some multithreaded unit tests for this or you verified it locally.



-- 
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 #439: [FLINK-29959] Use optimistic locking when updating the status

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

   > Makes sense, note that this just needed in case a persistent state is managed in status. If that state would be in a separate ConfigMap/Secret might feel less "unnatural" to update it with optimistic locking. Status in Kubernetes by definition is "best effort", basically product of latest observation, so storing state little bit out of this definition.
   > 
   > Also note that it is not guaranteed that in next reconciliation you will receive the latest object unless you call one of these: https://github.com/java-operator-sdk/java-operator-sdk/blob/80e0696e4f3af371be21c15ced805ba4c2e9a1b6/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/ManagedInformerEventSource.java#L80-L91 For `ControllerResourceEventSource` in this case.
   
   Thanks @csviri , I will check the cache update methods, but currently we have our own status cache in the `StatusRecorder` that we use to always set the latest status on the received object to get the same effect.


-- 
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] gaborgsomogyi commented on a diff in pull request #439: [FLINK-29959] Use optimistic locking when updating the status

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


##########
flink-kubernetes-operator-api/src/main/java/org/apache/flink/kubernetes/operator/api/status/SavepointInfo.java:
##########
@@ -63,7 +63,7 @@ public void setTrigger(
     }
 
     public void resetTrigger() {
-        this.triggerId = "";
+        this.triggerId = null;
         this.triggerTimestamp = 0L;

Review Comment:
   Adding this would end-up in additional code like null checks, etc..., but what would be the benefit?



-- 
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 #439: [FLINK-29959] Use optimistic locking when updating the status

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


-- 
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 #439: [FLINK-29959] Use optimistic locking when updating the status

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


##########
flink-kubernetes-operator-api/src/main/java/org/apache/flink/kubernetes/operator/api/status/SavepointInfo.java:
##########
@@ -63,7 +63,7 @@ public void setTrigger(
     }
 
     public void resetTrigger() {
-        this.triggerId = "";
+        this.triggerId = null;
         this.triggerTimestamp = 0L;

Review Comment:
   at least until the next trigger



-- 
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 #439: [FLINK-29959] Use optimistic locking when updating the status

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


##########
flink-kubernetes-operator/src/main/java/org/apache/flink/kubernetes/operator/utils/StatusRecorder.java:
##########
@@ -97,20 +96,79 @@ public void patchAndCacheStatus(CR resource) {
 
         Exception err = null;
         for (int i = 0; i < 3; i++) {
-            // In any case we retry the status update 3 times to avoid some intermittent
-            // connectivity errors if any
+            // We retry the status update 3 times to avoid some intermittent connectivity errors
             try {
-                client.resource(resource).patchStatus();
-                statusUpdateListener.accept(resource, prevStatus);
-                metricManager.onUpdate(resource);
-                return;
-            } catch (Exception e) {
+                replaceStatus(resource, prevStatus);
+                err = null;
+            } catch (KubernetesClientException e) {
                 LOG.error("Error while patching status, retrying {}/3...", (i + 1), e);
                 Thread.sleep(1000);
                 err = e;
             }
         }
-        throw err;
+

Review Comment:
   This is not really a question of multithreading, the main problem with unit tests is that the fabric8 mockservers cannot really handle the resource locking.
   
   I have added a case to the Operator integration test which validates the core conflict detection. Otherwise the e2e tests and manual testing should be enough to cover the basic behaviour (we don't expect any errors in the logs normally related to this part)



-- 
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] gaborgsomogyi commented on pull request #439: [FLINK-29959] Use optimistic locking when updating the status

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

   Not sure why couple of CI things cancelled :/


-- 
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] gaborgsomogyi commented on a diff in pull request #439: [FLINK-29959] Use optimistic locking when updating the status

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


##########
flink-kubernetes-operator-api/src/main/java/org/apache/flink/kubernetes/operator/api/status/SavepointInfo.java:
##########
@@ -63,7 +63,7 @@ public void setTrigger(
     }
 
     public void resetTrigger() {
-        this.triggerId = "";
+        this.triggerId = null;
         this.triggerTimestamp = 0L;

Review Comment:
   Sounds fair.



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