Hadoop HDFS
  1. Hadoop HDFS
  2. HDFS-1965

IPCs done using block token-based tickets can't reuse connections

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.22.0
    • Component/s: security
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      This is the reason that TestFileConcurrentReaders has been failing a lot. Reproducing a comment from HDFS-1057:

      The test has a thread which continually re-opens the file which is being written to. Since the file's in the middle of being written, it makes an RPC to the DataNode in order to determine the visible length of the file. This RPC is authenticated using the block token which came back in the LocatedBlocks object as the security ticket.

      When this RPC hits the IPC layer, it looks at its existing connections and sees none that can be re-used, since the block token differs between the two requesters. Hence, it reconnects, and we end up with hundreds or thousands of IPC connections to the datanode.

      1. hdfs-1965.txt
        11 kB
        Todd Lipcon
      2. hdfs-1965.txt
        9 kB
        Todd Lipcon
      3. hdfs-1965.txt
        8 kB
        Todd Lipcon
      4. hdfs-1965-0.22.txt
        9 kB
        Todd Lipcon

        Issue Links

          Activity

          Hide
          Todd Lipcon added a comment -

          I can think of a couple possible solutions:

          a) make the methods that operate on a block take an additional parameter to contain block tokens, rather than using the normal token selector mechanism that scopes credentials on a per-connection basis. This has the advantage that we can even re-use an IPC connection across different blocks.

          b) when the client creates an IPC proxy to a DN, it can explicitly configure the maxIdleTime to 0 so that we don't leave connections hanging around after the call completes. This is less efficient than option A above, but it probably doesn't matter much for this use case.

          Show
          Todd Lipcon added a comment - I can think of a couple possible solutions: a) make the methods that operate on a block take an additional parameter to contain block tokens, rather than using the normal token selector mechanism that scopes credentials on a per-connection basis. This has the advantage that we can even re-use an IPC connection across different blocks. b) when the client creates an IPC proxy to a DN, it can explicitly configure the maxIdleTime to 0 so that we don't leave connections hanging around after the call completes. This is less efficient than option A above, but it probably doesn't matter much for this use case.
          Hide
          Todd Lipcon added a comment -

          I implemented option (b) and have a test case that shows that it fixes the problem...

          BUT: the real DFSInputStream code seems to call RPC.stopProxy() after it uses the proxy, which should also avoid this issue. Doing so in my test case makes the case pass without any other fix. So there's still some mystery.

          Show
          Todd Lipcon added a comment - I implemented option (b) and have a test case that shows that it fixes the problem... BUT: the real DFSInputStream code seems to call RPC.stopProxy() after it uses the proxy, which should also avoid this issue. Doing so in my test case makes the case pass without any other fix. So there's still some mystery.
          Hide
          Todd Lipcon added a comment -

          Turns out the reason that RPC.stopProxy isn't effective in "real life" is that the WritableRpcEngine "Client" objects are cached in ClientCache with keys that aren't tied to principals. So, stopProxy doesn't actually cause the connection to disconnect. I'm not sure if that's a bug or by design.

          This patch now includes a regression test that simulates DFSClient closely.

          Show
          Todd Lipcon added a comment - Turns out the reason that RPC.stopProxy isn't effective in "real life" is that the WritableRpcEngine "Client" objects are cached in ClientCache with keys that aren't tied to principals. So, stopProxy doesn't actually cause the connection to disconnect. I'm not sure if that's a bug or by design. This patch now includes a regression test that simulates DFSClient closely.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12479859/hdfs-1965.txt
          against trunk revision 1125145.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 3 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          -1 javac. The applied patch generated 29 javac compiler warnings (more than the trunk's current 28 warnings).

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed these core unit tests:
          org.apache.hadoop.hdfs.TestDFSStorageStateRecovery
          org.apache.hadoop.hdfs.TestHDFSTrash

          +1 contrib tests. The patch passed contrib unit tests.

          +1 system test framework. The patch passed system test framework compile.

          Test results: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/596//testReport/
          Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/596//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/596//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12479859/hdfs-1965.txt against trunk revision 1125145. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. -1 javac. The applied patch generated 29 javac compiler warnings (more than the trunk's current 28 warnings). +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these core unit tests: org.apache.hadoop.hdfs.TestDFSStorageStateRecovery org.apache.hadoop.hdfs.TestHDFSTrash +1 contrib tests. The patch passed contrib unit tests. +1 system test framework. The patch passed system test framework compile. Test results: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/596//testReport/ Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/596//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/596//console This message is automatically generated.
          Hide
          Todd Lipcon added a comment -

          Fixes the javac warning

          Show
          Todd Lipcon added a comment - Fixes the javac warning
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12479878/hdfs-1965.txt
          against trunk revision 1125217.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 3 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed these core unit tests:
          org.apache.hadoop.hdfs.TestDFSStorageStateRecovery
          org.apache.hadoop.hdfs.TestHDFSTrash

          +1 contrib tests. The patch passed contrib unit tests.

          +1 system test framework. The patch passed system test framework compile.

          Test results: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/599//testReport/
          Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/599//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/599//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12479878/hdfs-1965.txt against trunk revision 1125217. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these core unit tests: org.apache.hadoop.hdfs.TestDFSStorageStateRecovery org.apache.hadoop.hdfs.TestHDFSTrash +1 contrib tests. The patch passed contrib unit tests. +1 system test framework. The patch passed system test framework compile. Test results: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/599//testReport/ Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/599//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/599//console This message is automatically generated.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          It seems that the reasons of TestFileConcurrentReader failing are:

          • The test open many files within a short period of time, says in a few seconds.
          • DFSClient creates a proxy for each open.
          • Since the default ipc.client.connection.maxidletime is 10 seconds, so the proxies are not yet closed.
          • Therefore, TestFileConcurrentReader fails with runtime exceptions (out of descriptors?)

          Todd, do you agree?

          Questions: We already have RPC.stopProxy(cdp) in a finally-block. Why the resource is still not released? Is it because TestFileConcurrentReader opens files so fast that the finally-block is not yet reached? Or RPC.stopProxy(..) does not work?

          Show
          Tsz Wo Nicholas Sze added a comment - It seems that the reasons of TestFileConcurrentReader failing are: The test open many files within a short period of time, says in a few seconds. DFSClient creates a proxy for each open. Since the default ipc.client.connection.maxidletime is 10 seconds, so the proxies are not yet closed. Therefore, TestFileConcurrentReader fails with runtime exceptions (out of descriptors?) Todd, do you agree? Questions : We already have RPC.stopProxy(cdp) in a finally-block. Why the resource is still not released? Is it because TestFileConcurrentReader opens files so fast that the finally-block is not yet reached? Or RPC.stopProxy(..) does not work?
          Hide
          Tsz Wo Nicholas Sze added a comment -

          > Turns out the reason that RPC.stopProxy isn't effective in "real life" is that the WritableRpcEngine "Client" objects are cached in ClientCache with keys that aren't tied to principals. So, stopProxy doesn't actually cause the connection to disconnect. I'm not sure if that's a bug or by design.

          Todd, just saw you comments. I think this is the real bug: we should fix stopProxy(..) instead of changing max idle time.

          Show
          Tsz Wo Nicholas Sze added a comment - > Turns out the reason that RPC.stopProxy isn't effective in "real life" is that the WritableRpcEngine "Client" objects are cached in ClientCache with keys that aren't tied to principals. So, stopProxy doesn't actually cause the connection to disconnect. I'm not sure if that's a bug or by design. Todd, just saw you comments. I think this is the real bug: we should fix stopProxy(..) instead of changing max idle time.
          Hide
          Todd Lipcon added a comment -

          Todd, just saw you comments. I think this is the real bug: we should fix stopProxy(..) instead of changing max idle time.

          Yes, you're probably right. But maybe we can use this as a stop-gap for 0.22 while we work on the stopProxy fix in trunk? I'm afraid the stopProxy stuff will be complicated - that IPC code is kind of spaghetti.

          Show
          Todd Lipcon added a comment - Todd, just saw you comments. I think this is the real bug: we should fix stopProxy(..) instead of changing max idle time. Yes, you're probably right. But maybe we can use this as a stop-gap for 0.22 while we work on the stopProxy fix in trunk? I'm afraid the stopProxy stuff will be complicated - that IPC code is kind of spaghetti.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          Okay, you mean this is a temporary fix. Sounds good. Some comments on the patch:

          • Instead of changing it to public, we could create add a utility method, say in DFSTestUtil, for invoking the package private method.
            +  /** Public only for tests */
            +  public static ClientDatanodeProtocol createClientDatanodeProtocolProxy(
            
          • How about putting confWithNoIpcIdle as a member field?
          • Please use CommonConfigurationKeysPublic.IPC_CLIENT_CONNECTION_MAXIDLETIME_KEY for "ipc.client.connection.maxidletime".
          • Please add a comment saying that this is a temporary fix and the corresponding codes should be removed once stopProxy(..) is fixed.
          Show
          Tsz Wo Nicholas Sze added a comment - Okay, you mean this is a temporary fix. Sounds good. Some comments on the patch: Instead of changing it to public, we could create add a utility method, say in DFSTestUtil , for invoking the package private method. + /** Public only for tests */ + public static ClientDatanodeProtocol createClientDatanodeProtocolProxy( How about putting confWithNoIpcIdle as a member field? Please use CommonConfigurationKeysPublic.IPC_CLIENT_CONNECTION_MAXIDLETIME_KEY for "ipc.client.connection.maxidletime". Please add a comment saying that this is a temporary fix and the corresponding codes should be removed once stopProxy(..) is fixed.
          Hide
          Todd Lipcon added a comment -

          Addressed all of Nicholas's feedback, except for making confWithNoIpcIdle a member variable. The function in question is static, so I'd have to change it at the call sites.

          Since this is a pretty rare call the extra object allocation doesn't seem like a big deal to me.

          Show
          Todd Lipcon added a comment - Addressed all of Nicholas's feedback, except for making confWithNoIpcIdle a member variable. The function in question is static, so I'd have to change it at the call sites. Since this is a pretty rare call the extra object allocation doesn't seem like a big deal to me.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          +1 patch looks good.

          Show
          Tsz Wo Nicholas Sze added a comment - +1 patch looks good.
          Hide
          Todd Lipcon added a comment -

          Committed to trunk after re-running the test.

          It doesn't apply directly to 0.22. Let me format a patch there and upload it soon.

          Show
          Todd Lipcon added a comment - Committed to trunk after re-running the test. It doesn't apply directly to 0.22. Let me format a patch there and upload it soon.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk-Commit #677 (See https://builds.apache.org/hudson/job/Hadoop-Hdfs-trunk-Commit/677/)
          HDFS-1965. IPCs done using block token-based tickets can't reuse connections. Contributed by Todd Lipcon.

          todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1125605
          Files :

          • /hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/DFSTestUtil.java
          • /hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/security/token/block/TestBlockToken.java
          • /hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/DFSClient.java
          • /hadoop/hdfs/trunk/CHANGES.txt
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk-Commit #677 (See https://builds.apache.org/hudson/job/Hadoop-Hdfs-trunk-Commit/677/ ) HDFS-1965 . IPCs done using block token-based tickets can't reuse connections. Contributed by Todd Lipcon. todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1125605 Files : /hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/DFSTestUtil.java /hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/security/token/block/TestBlockToken.java /hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/DFSClient.java /hadoop/hdfs/trunk/CHANGES.txt
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12479967/hdfs-1965.txt
          against trunk revision 1125217.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 6 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed these core unit tests:
          org.apache.hadoop.hdfs.TestDFSStorageStateRecovery
          org.apache.hadoop.hdfs.TestHDFSTrash

          +1 contrib tests. The patch passed contrib unit tests.

          +1 system test framework. The patch passed system test framework compile.

          Test results: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/605//testReport/
          Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/605//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/605//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12479967/hdfs-1965.txt against trunk revision 1125217. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 6 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these core unit tests: org.apache.hadoop.hdfs.TestDFSStorageStateRecovery org.apache.hadoop.hdfs.TestHDFSTrash +1 contrib tests. The patch passed contrib unit tests. +1 system test framework. The patch passed system test framework compile. Test results: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/605//testReport/ Findbugs warnings: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/605//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/605//console This message is automatically generated.
          Hide
          Todd Lipcon added a comment -

          Here's a patch for 0.22. Unfortunately I had to get rid of the part of the test case that makes the proxyToNowhere, since it was triggering getProtocolVersion on 0.22 and hence hanging the test since no one was listening at 1.1.1.1.

          I verified that removing the fix still makes this test case fail on 0.22.

          Show
          Todd Lipcon added a comment - Here's a patch for 0.22. Unfortunately I had to get rid of the part of the test case that makes the proxyToNowhere, since it was triggering getProtocolVersion on 0.22 and hence hanging the test since no one was listening at 1.1.1.1. I verified that removing the fix still makes this test case fail on 0.22.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          Hey Todd, please wait for Hadoop QA before committing the patch. It sometimes catches unexpected problems.

          Show
          Tsz Wo Nicholas Sze added a comment - Hey Todd, please wait for Hadoop QA before committing the patch. It sometimes catches unexpected problems.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12479975/hdfs-1965-0.22.txt
          against trunk revision 1125605.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 6 new or modified tests.

          -1 patch. The patch command could not apply the patch.

          Console output: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/607//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12479975/hdfs-1965-0.22.txt against trunk revision 1125605. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 6 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/hudson/job/PreCommit-HDFS-Build/607//console This message is automatically generated.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk #673 (See https://builds.apache.org/hudson/job/Hadoop-Hdfs-trunk/673/)
          HDFS-1965. IPCs done using block token-based tickets can't reuse connections. Contributed by Todd Lipcon.

          todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1125605
          Files :

          • /hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/DFSTestUtil.java
          • /hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/security/token/block/TestBlockToken.java
          • /hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/DFSClient.java
          • /hadoop/hdfs/trunk/CHANGES.txt
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #673 (See https://builds.apache.org/hudson/job/Hadoop-Hdfs-trunk/673/ ) HDFS-1965 . IPCs done using block token-based tickets can't reuse connections. Contributed by Todd Lipcon. todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1125605 Files : /hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/DFSTestUtil.java /hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/security/token/block/TestBlockToken.java /hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/DFSClient.java /hadoop/hdfs/trunk/CHANGES.txt
          Hide
          Todd Lipcon added a comment -

          Nicholas: can you please take a quick look at the 0.22 patch?

          Show
          Todd Lipcon added a comment - Nicholas: can you please take a quick look at the 0.22 patch?
          Hide
          Tsz Wo Nicholas Sze added a comment -

          Came up a question: By setting maxidletime to 0, is there a race condition that the timeout occurs before the first call, i.e. the proxy is closed before the first call?

          Show
          Tsz Wo Nicholas Sze added a comment - Came up a question: By setting maxidletime to 0, is there a race condition that the timeout occurs before the first call, i.e. the proxy is closed before the first call?
          Hide
          Todd Lipcon added a comment -

          I think in trunk, it's not possible, since the connection is only lazily opened by the actual RPC to the DataNode. Then, it won't close since there's a call outstanding.

          In 0.22, it's possible that it will open one connection for the getProtocolVersion() call and a second one for the actual RPC. Unless I'm missing something, that should only be an efficiency issue and not a correctness issue. Do you agree?

          Show
          Todd Lipcon added a comment - I think in trunk, it's not possible, since the connection is only lazily opened by the actual RPC to the DataNode. Then, it won't close since there's a call outstanding. In 0.22, it's possible that it will open one connection for the getProtocolVersion() call and a second one for the actual RPC. Unless I'm missing something, that should only be an efficiency issue and not a correctness issue. Do you agree?
          Hide
          Tsz Wo Nicholas Sze added a comment -

          Okay, I fine with it since it is only a temporary fix.

          +1 the 0.22 patch looks good.

          Show
          Tsz Wo Nicholas Sze added a comment - Okay, I fine with it since it is only a temporary fix. +1 the 0.22 patch looks good.
          Hide
          Todd Lipcon added a comment -

          Committed the 22 patch. Thanks, Nicholas. HADOOP-7317 tracks the real underlying issue.

          Show
          Todd Lipcon added a comment - Committed the 22 patch. Thanks, Nicholas. HADOOP-7317 tracks the real underlying issue.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-22-branch #61 (See https://builds.apache.org/hudson/job/Hadoop-Hdfs-22-branch/61/)

          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-22-branch #61 (See https://builds.apache.org/hudson/job/Hadoop-Hdfs-22-branch/61/ )

            People

            • Assignee:
              Todd Lipcon
              Reporter:
              Todd Lipcon
            • Votes:
              0 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development