You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "smengcl (via GitHub)" <gi...@apache.org> on 2024/02/27 19:21:54 UTC

[PR] HDDS-10430. Data race with nodes array causing race condition in Pipeline.getProtobufMessage [ozone]

smengcl opened a new pull request, #6285:
URL: https://github.com/apache/ozone/pull/6285

   ## What changes were proposed in this pull request?
   
   Eliminate data race with `nodes` array (which caused race condition) in `Pipeline` instances. See the jira description for a stack trace and analysis.
   
   ## What is the link to the Apache JIRA
   
   https://issues.apache.org/jira/browse/HDDS-10430
   
   ## How was this patch tested?
   
   - Existing tests


-- 
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@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-10430. Data race with nodes array causing race condition in Pipeline.getProtobufMessage [ozone]

Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on PR #6285:
URL: https://github.com/apache/ozone/pull/6285#issuecomment-1970138296

   LGTM


-- 
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@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-10430. Race condition around Pipeline#nodesInOrder [ozone]

Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #6285:
URL: https://github.com/apache/ozone/pull/6285#discussion_r1508120661


##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/pipeline/Pipeline.java:
##########
@@ -72,14 +72,63 @@ public static Codec<Pipeline> getCodec() {
   }
 
   private static final Logger LOG = LoggerFactory.getLogger(Pipeline.class);
+
+  /**
+   * Inner class that provides thread-safe access to DN list.
+   */
+  static class NodesInOrder {
+    private final List<DatanodeDetails> nodes = new ArrayList<>();
+
+    synchronized DatanodeDetails getClosest(PipelineID id, Set<DatanodeDetails> excluded) throws IOException {
+      if (nodes.isEmpty()) {
+        LOG.debug("Nodes in order is empty, delegate to getFirstNode");

Review Comment:
   I've added the logging in `getClosestNode`.
   
   But I kept the `isEmpty()` check inside `getClosest` as well to facilitate debugging (because there is still a none-zero chance of `nodes` becoming empty between the locks, TOCTOU).



-- 
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@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-10430. Race condition around Pipeline#nodesInOrder [ozone]

Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #6285:
URL: https://github.com/apache/ozone/pull/6285#discussion_r1507332264


##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/pipeline/Pipeline.java:
##########
@@ -72,14 +72,63 @@ public static Codec<Pipeline> getCodec() {
   }
 
   private static final Logger LOG = LoggerFactory.getLogger(Pipeline.class);
+
+  /**
+   * Inner class that provides thread-safe access to DN list.
+   */
+  static class NodesInOrder {
+    private final List<DatanodeDetails> nodes = new ArrayList<>();
+
+    synchronized DatanodeDetails getClosest(PipelineID id, Set<DatanodeDetails> excluded) throws IOException {
+      if (nodes.isEmpty()) {
+        LOG.debug("Nodes in order is empty, delegate to getFirstNode");

Review Comment:
   nit: move log to caller `getClosestNode`, similar to how log is in `getNodesInOrder`, not in `getAll`.  Mostly to avoid logging while holding the lock.



-- 
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@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-10430. Data race with nodes array causing race condition in Pipeline.getProtobufMessage [ozone]

Posted by "kerneltime (via GitHub)" <gi...@apache.org>.
kerneltime commented on PR #6285:
URL: https://github.com/apache/ozone/pull/6285#issuecomment-1970149954

   cc @tanvipenumudy 


-- 
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@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-10430. Race condition around Pipeline#nodesInOrder [ozone]

Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #6285:
URL: https://github.com/apache/ozone/pull/6285#discussion_r1508201669


##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/pipeline/Pipeline.java:
##########
@@ -72,14 +72,63 @@ public static Codec<Pipeline> getCodec() {
   }
 
   private static final Logger LOG = LoggerFactory.getLogger(Pipeline.class);
+
+  /**
+   * Inner class that provides thread-safe access to DN list.
+   */
+  static class NodesInOrder {
+    private final List<DatanodeDetails> nodes = new ArrayList<>();
+
+    synchronized DatanodeDetails getClosest(PipelineID id, Set<DatanodeDetails> excluded) throws IOException {
+      if (nodes.isEmpty()) {
+        LOG.debug("Nodes in order is empty, delegate to getFirstNode");

Review Comment:
   ah, my bad. done



-- 
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@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-10430. Data race with nodes array causing race condition in Pipeline.getProtobufMessage [ozone]

Posted by "jojochuang (via GitHub)" <gi...@apache.org>.
jojochuang commented on code in PR #6285:
URL: https://github.com/apache/ozone/pull/6285#discussion_r1506819273


##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/pipeline/Pipeline.java:
##########
@@ -72,14 +72,63 @@ public static Codec<Pipeline> getCodec() {
   }
 
   private static final Logger LOG = LoggerFactory.getLogger(Pipeline.class);
+
+  /**
+   * Inner class that provides thread-safe access to DN list.

Review Comment:
   ```suggestion
      * Inner class that provides thread-safe access to DN list, ordered by distance.
   ```



-- 
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@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-10430. Race condition around Pipeline#nodesInOrder [ozone]

Posted by "duongkame (via GitHub)" <gi...@apache.org>.
duongkame commented on PR #6285:
URL: https://github.com/apache/ozone/pull/6285#issuecomment-1972244719

   Adding synchronized to `getProtobufMessage` surely impacts OM performance in case there's a hot Pipeline. E.g. many concurrent reads to the same keys, or keys in the same container.
   A few thought:
   - If it's caused by [HDDS-9848](https://issues.apache.org/jira/browse/HDDS-9848)? Why not just revert it?
   - Can we do it another way? I.g. favor immutabily/copy instead of changing the same instance?
   - Use ReadWriteLock instead of synchronization?
   
   @smengcl @kerneltime 


-- 
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@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-10430. Race condition around Pipeline#nodesInOrder [ozone]

Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #6285:
URL: https://github.com/apache/ozone/pull/6285#issuecomment-1972664592

   > Can we do it another way? I.g. favor immutabily/copy instead of changing the same instance?
   
   In the long term I think we should move to immutable.


-- 
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@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-10430. Race condition around Pipeline#nodesInOrder [ozone]

Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on PR #6285:
URL: https://github.com/apache/ozone/pull/6285#issuecomment-1972355206

   Thanks @duongkame for the comment.
   
   > * If it's caused by [HDDS-9848](https://issues.apache.org/jira/browse/HDDS-9848)? Why not just revert it?
   
   Because the whole point of HDDS-9848 is to share `nodesInOrder` among threads.
   
   > * Can we do it another way? I.g. favor immutabily/copy instead of changing the same instance?
   > * Use ReadWriteLock instead of synchronization?
   
   I feel like if we need to get rid the lock (whether intrinsic or reentrant) here we may need to reconsider the design here, e.g. why do we need to sort this DN list every now and then. The goal of this PR is to fix the bug first.


-- 
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@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-10430. Data race with nodes array causing race condition in Pipeline.getProtobufMessage [ozone]

Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on PR #6285:
URL: https://github.com/apache/ozone/pull/6285#issuecomment-1970041867

   > @smengcl , thanks for working on this! The change looks good. How about move the `nodesInOrder` to a new class? See [issues.apache.org/jira/secure/attachment/13067089/6285_review.patch](https://issues.apache.org/jira/secure/attachment/13067089/6285_review.patch)
   
   Good idea! Thanks. That makes the code much cleaner. Great use of `Collections.unmodifiableList()` as well. Waiting for CI.


-- 
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@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-10430. Race condition around Pipeline#nodesInOrder [ozone]

Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #6285:
URL: https://github.com/apache/ozone/pull/6285#issuecomment-1974776727

   Please see #6316 for alternative approach (making `nodesInOrder` immutable and copy `Pipeline` when needed).


-- 
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@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-10430. Race condition around Pipeline#nodesInOrder [ozone]

Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on PR #6285:
URL: https://github.com/apache/ozone/pull/6285#issuecomment-1977029646

   > Please see #6316 for alternative approach (making `nodesInOrder` immutable and copy `Pipeline` when needed).
   
   Thanks @adoroszlai . Closing this in favor of #6316 


-- 
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@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-10430. Race condition around Pipeline#nodesInOrder [ozone]

Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl closed pull request #6285: HDDS-10430. Race condition around Pipeline#nodesInOrder
URL: https://github.com/apache/ozone/pull/6285


-- 
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@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-10430. Race condition around Pipeline#nodesInOrder [ozone]

Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #6285:
URL: https://github.com/apache/ozone/pull/6285#discussion_r1508131191


##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/pipeline/Pipeline.java:
##########
@@ -72,14 +72,63 @@ public static Codec<Pipeline> getCodec() {
   }
 
   private static final Logger LOG = LoggerFactory.getLogger(Pipeline.class);
+
+  /**
+   * Inner class that provides thread-safe access to DN list.
+   */
+  static class NodesInOrder {
+    private final List<DatanodeDetails> nodes = new ArrayList<>();
+
+    synchronized DatanodeDetails getClosest(PipelineID id, Set<DatanodeDetails> excluded) throws IOException {
+      if (nodes.isEmpty()) {
+        LOG.debug("Nodes in order is empty, delegate to getFirstNode");

Review Comment:
   Let me clarify.  `getClosestNode` should be:
   
   ```java
       final DatanodeDetails closest = nodesInOrder.getClosest(id, excluded);
       if (closest == null) {
         LOG.debug("Nodes in order is empty, delegate to getFirstNode");
         return getFirstNode(excluded);
       }
       return closest;
   ```
   
   No "between the locks" here.  Then `LOG.debug` can be removed from `getClosest`.



-- 
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@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-10430. Race condition around Pipeline#nodesInOrder [ozone]

Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #6285:
URL: https://github.com/apache/ozone/pull/6285#discussion_r1508120661


##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/pipeline/Pipeline.java:
##########
@@ -72,14 +72,63 @@ public static Codec<Pipeline> getCodec() {
   }
 
   private static final Logger LOG = LoggerFactory.getLogger(Pipeline.class);
+
+  /**
+   * Inner class that provides thread-safe access to DN list.
+   */
+  static class NodesInOrder {
+    private final List<DatanodeDetails> nodes = new ArrayList<>();
+
+    synchronized DatanodeDetails getClosest(PipelineID id, Set<DatanodeDetails> excluded) throws IOException {
+      if (nodes.isEmpty()) {
+        LOG.debug("Nodes in order is empty, delegate to getFirstNode");

Review Comment:
   I've added the logging in `getClosestNode`.
   
   But I kept the `isEmpty()` check inside `getClosest` as well to facilitate debugging (because there is still a none-zero chance of TOCTOU).



-- 
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@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-10430. Race condition around Pipeline#nodesInOrder [ozone]

Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on PR #6285:
URL: https://github.com/apache/ozone/pull/6285#issuecomment-1973793422

   I found that there is a bigger problem: SCM has a `PipelineStateMap` so if `nodesInOrder` is set for one client, another client may overwrite it.  Then, the first client will see a different order.
   
   The original implementation (before HDDS-9848) using `ThreadLocal` may assume that only one thread processes a request from the beginning to the end.  This assumption is current for HDFS namenode  but not sure if it holds for OM, SCM and Ozone client.


-- 
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@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org