From b57b5cec277671d5e3ae199a026958c3d2810b7d Mon Sep 17 00:00:00 2001 From: Daryn Sharp Date: Mon, 4 Mar 2013 18:48:16 +0000 Subject: [PATCH] svn merge -c 1452435 FIXES: HDFS-4532. RPC call queue may fill due to current user lookup (daryn) git-svn-id: https://svn.apache.org/repos/asf/hadoop/common/branches/branch-2@1452438 13f79535-47bb-0310-9956-ffa450edef68 --- hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt | 2 + .../hdfs/server/namenode/FSNamesystem.java | 249 ++++++------------ 2 files changed, 82 insertions(+), 169 deletions(-) diff --git a/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt b/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt index 84c63605997..72d8498478e 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt +++ b/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt @@ -2088,6 +2088,8 @@ Release 0.23.7 - UNRELEASED OPTIMIZATIONS + HDFS-4532. RPC call queue may fill due to current user lookup (daryn) + BUG FIXES HDFS-4288. NN accepts incremental BR as IBR in safemode (daryn via kihwal) diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java index af8a0dd5a74..8c114c34644 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java @@ -238,10 +238,23 @@ private boolean isAuditEnabled() { return !isDefaultAuditLogger || auditLog.isInfoEnabled(); } - private void logAuditEvent(UserGroupInformation ugi, - InetAddress addr, String cmd, String src, String dst, - HdfsFileStatus stat) { - logAuditEvent(true, ugi, addr, cmd, src, dst, stat); + private HdfsFileStatus getAuditFileInfo(String path, boolean resolveSymlink) + throws IOException { + return (isAuditEnabled() && isExternalInvocation()) + ? dir.getFileInfo(path, resolveSymlink) : null; + } + + private void logAuditEvent(boolean succeeded, String cmd, String src) + throws IOException { + logAuditEvent(succeeded, cmd, src, null, null); + } + + private void logAuditEvent(boolean succeeded, String cmd, String src, + String dst, HdfsFileStatus stat) throws IOException { + if (isAuditEnabled() && isExternalInvocation()) { + logAuditEvent(succeeded, getRemoteUser(), getRemoteIp(), + cmd, src, dst, stat); + } } private void logAuditEvent(boolean succeeded, @@ -1130,11 +1143,7 @@ void setPermission(String src, FsPermission permission) try { setPermissionInt(src, permission); } catch (AccessControlException e) { - if (isAuditEnabled() && isExternalInvocation()) { - logAuditEvent(false, UserGroupInformation.getCurrentUser(), - getRemoteIp(), - "setPermission", src, null, null); - } + logAuditEvent(false, "setPermission", src); throw e; } } @@ -1153,18 +1162,12 @@ private void setPermissionInt(String src, FsPermission permission) } checkOwner(pc, src); dir.setPermission(src, permission); - if (isAuditEnabled() && isExternalInvocation()) { - resultingStat = dir.getFileInfo(src, false); - } + resultingStat = getAuditFileInfo(src, false); } finally { writeUnlock(); } getEditLog().logSync(); - if (isAuditEnabled() && isExternalInvocation()) { - logAuditEvent(UserGroupInformation.getCurrentUser(), - getRemoteIp(), - "setPermission", src, null, resultingStat); - } + logAuditEvent(true, "setPermission", src, null, resultingStat); } /** @@ -1177,11 +1180,7 @@ void setOwner(String src, String username, String group) try { setOwnerInt(src, username, group); } catch (AccessControlException e) { - if (isAuditEnabled() && isExternalInvocation()) { - logAuditEvent(false, UserGroupInformation.getCurrentUser(), - getRemoteIp(), - "setOwner", src, null, null); - } + logAuditEvent(false, "setOwner", src); throw e; } } @@ -1208,18 +1207,12 @@ private void setOwnerInt(String src, String username, String group) } } dir.setOwner(src, username, group); - if (isAuditEnabled() && isExternalInvocation()) { - resultingStat = dir.getFileInfo(src, false); - } + resultingStat = getAuditFileInfo(src, false); } finally { writeUnlock(); } getEditLog().logSync(); - if (isAuditEnabled() && isExternalInvocation()) { - logAuditEvent(UserGroupInformation.getCurrentUser(), - getRemoteIp(), - "setOwner", src, null, resultingStat); - } + logAuditEvent(true, "setOwner", src, null, resultingStat); } /** @@ -1259,11 +1252,7 @@ LocatedBlocks getBlockLocations(String src, long offset, long length, return getBlockLocationsInt(pc, src, offset, length, doAccessTime, needBlockToken, checkSafeMode); } catch (AccessControlException e) { - if (isAuditEnabled() && isExternalInvocation()) { - logAuditEvent(false, UserGroupInformation.getCurrentUser(), - getRemoteIp(), - "open", src, null, null); - } + logAuditEvent(false, "open", src); throw e; } } @@ -1286,11 +1275,7 @@ private LocatedBlocks getBlockLocationsInt(FSPermissionChecker pc, } final LocatedBlocks ret = getBlockLocationsUpdateTimes(src, offset, length, doAccessTime, needBlockToken); - if (isAuditEnabled() && isExternalInvocation()) { - logAuditEvent(UserGroupInformation.getCurrentUser(), - getRemoteIp(), - "open", src, null, null); - } + logAuditEvent(true, "open", src); if (checkSafeMode && isInSafeMode()) { for (LocatedBlock b : ret.getLocatedBlocks()) { // if safemode & no block locations yet then throw safemodeException @@ -1367,11 +1352,7 @@ void concat(String target, String [] srcs) try { concatInt(target, srcs); } catch (AccessControlException e) { - if (isAuditEnabled() && isExternalInvocation()) { - logAuditEvent(false, UserGroupInformation.getLoginUser(), - getRemoteIp(), - "concat", Arrays.toString(srcs), target, null); - } + logAuditEvent(false, "concat", Arrays.toString(srcs), target, null); throw e; } } @@ -1411,18 +1392,12 @@ private void concatInt(String target, String [] srcs) throw new SafeModeException("Cannot concat " + target, safeMode); } concatInternal(pc, target, srcs); - if (isAuditEnabled() && isExternalInvocation()) { - resultingStat = dir.getFileInfo(target, false); - } + resultingStat = getAuditFileInfo(target, false); } finally { writeUnlock(); } getEditLog().logSync(); - if (isAuditEnabled() && isExternalInvocation()) { - logAuditEvent(UserGroupInformation.getLoginUser(), - getRemoteIp(), - "concat", Arrays.toString(srcs), target, resultingStat); - } + logAuditEvent(true, "concat", Arrays.toString(srcs), target, resultingStat); } /** See {@link #concat(String, String[])} */ @@ -1539,11 +1514,7 @@ void setTimes(String src, long mtime, long atime) try { setTimesInt(src, mtime, atime); } catch (AccessControlException e) { - if (isAuditEnabled() && isExternalInvocation()) { - logAuditEvent(false, UserGroupInformation.getCurrentUser(), - getRemoteIp(), - "setTimes", src, null, null); - } + logAuditEvent(false, "setTimes", src); throw e; } } @@ -1554,6 +1525,7 @@ private void setTimesInt(String src, long mtime, long atime) throw new IOException("Access time for hdfs is not configured. " + " Please set " + DFS_NAMENODE_ACCESSTIME_PRECISION_KEY + " configuration parameter."); } + HdfsFileStatus resultingStat = null; FSPermissionChecker pc = getPermissionChecker(); writeLock(); try { @@ -1566,18 +1538,14 @@ private void setTimesInt(String src, long mtime, long atime) INode inode = dir.getINode(src); if (inode != null) { dir.setTimes(src, inode, mtime, atime, true); - if (isAuditEnabled() && isExternalInvocation()) { - final HdfsFileStatus stat = dir.getFileInfo(src, false); - logAuditEvent(UserGroupInformation.getCurrentUser(), - getRemoteIp(), - "setTimes", src, null, stat); - } + resultingStat = getAuditFileInfo(src, false); } else { throw new FileNotFoundException("File/Directory " + src + " does not exist."); } } finally { writeUnlock(); } + logAuditEvent(true, "setTimes", src, null, resultingStat); } /** @@ -1589,11 +1557,7 @@ void createSymlink(String target, String link, try { createSymlinkInt(target, link, dirPerms, createParent); } catch (AccessControlException e) { - if (isAuditEnabled() && isExternalInvocation()) { - logAuditEvent(false, UserGroupInformation.getCurrentUser(), - getRemoteIp(), - "createSymlink", link, target, null); - } + logAuditEvent(false, "createSymlink", link, target, null); throw e; } } @@ -1611,18 +1575,12 @@ private void createSymlinkInt(String target, String link, verifyParentDir(link); } createSymlinkInternal(pc, target, link, dirPerms, createParent); - if (isAuditEnabled() && isExternalInvocation()) { - resultingStat = dir.getFileInfo(link, false); - } + resultingStat = getAuditFileInfo(link, false); } finally { writeUnlock(); } getEditLog().logSync(); - if (isAuditEnabled() && isExternalInvocation()) { - logAuditEvent(UserGroupInformation.getCurrentUser(), - getRemoteIp(), - "createSymlink", link, target, resultingStat); - } + logAuditEvent(true, "createSymlink", link, target, resultingStat); } /** @@ -1674,11 +1632,7 @@ boolean setReplication(final String src, final short replication) try { return setReplicationInt(src, replication); } catch (AccessControlException e) { - if (isAuditEnabled() && isExternalInvocation()) { - logAuditEvent(false, UserGroupInformation.getCurrentUser(), - getRemoteIp(), - "setReplication", src, null, null); - } + logAuditEvent(false, "setReplication", src); throw e; } } @@ -1709,10 +1663,8 @@ private boolean setReplicationInt(final String src, final short replication) } getEditLog().logSync(); - if (isFile && isAuditEnabled() && isExternalInvocation()) { - logAuditEvent(UserGroupInformation.getCurrentUser(), - getRemoteIp(), - "setReplication", src, null, null); + if (isFile) { + logAuditEvent(true, "setReplication", src); } return isFile; } @@ -1767,11 +1719,7 @@ void startFile(String src, PermissionStatus permissions, String holder, startFileInt(src, permissions, holder, clientMachine, flag, createParent, replication, blockSize); } catch (AccessControlException e) { - if (isAuditEnabled() && isExternalInvocation()) { - logAuditEvent(false, UserGroupInformation.getCurrentUser(), - getRemoteIp(), - "create", src, null, null); - } + logAuditEvent(false, "create", src); throw e; } } @@ -1799,13 +1747,8 @@ private void startFileInt(String src, PermissionStatus permissions, String holde getEditLog().logSync(); } } - - if (isAuditEnabled() && isExternalInvocation()) { - final HdfsFileStatus stat = dir.getFileInfo(src, false); - logAuditEvent(UserGroupInformation.getCurrentUser(), - getRemoteIp(), - "create", src, null, stat); - } + final HdfsFileStatus stat = getAuditFileInfo(src, false); + logAuditEvent(true, "create", src, null, stat); } /** @@ -2102,11 +2045,7 @@ LocatedBlock appendFile(String src, String holder, String clientMachine) try { return appendFileInt(src, holder, clientMachine); } catch (AccessControlException e) { - if (isAuditEnabled() && isExternalInvocation()) { - logAuditEvent(false, UserGroupInformation.getCurrentUser(), - getRemoteIp(), - "append", src, null, null); - } + logAuditEvent(false, "append", src); throw e; } } @@ -2149,11 +2088,7 @@ private LocatedBlock appendFileInt(String src, String holder, String clientMachi +" block size " + lb.getBlock().getNumBytes()); } } - if (isAuditEnabled() && isExternalInvocation()) { - logAuditEvent(UserGroupInformation.getCurrentUser(), - getRemoteIp(), - "append", src, null, null); - } + logAuditEvent(true, "append", src); return lb; } @@ -2643,11 +2578,7 @@ boolean renameTo(String src, String dst) try { return renameToInt(src, dst); } catch (AccessControlException e) { - if (isAuditEnabled() && isExternalInvocation()) { - logAuditEvent(false, UserGroupInformation.getCurrentUser(), - getRemoteIp(), - "rename", src, dst, null); - } + logAuditEvent(false, "rename", src, dst, null); throw e; } } @@ -2666,17 +2597,15 @@ private boolean renameToInt(String src, String dst) checkOperation(OperationCategory.WRITE); status = renameToInternal(pc, src, dst); - if (status && isAuditEnabled() && isExternalInvocation()) { - resultingStat = dir.getFileInfo(dst, false); + if (status) { + resultingStat = getAuditFileInfo(dst, false); } } finally { writeUnlock(); } getEditLog().logSync(); - if (status && isAuditEnabled() && isExternalInvocation()) { - logAuditEvent(UserGroupInformation.getCurrentUser(), - getRemoteIp(), - "rename", src, dst, resultingStat); + if (status) { + logAuditEvent(true, "rename", src, dst, resultingStat); } return status; } @@ -2723,20 +2652,17 @@ void renameTo(String src, String dst, Options.Rename... options) try { checkOperation(OperationCategory.WRITE); renameToInternal(pc, src, dst, options); - if (isAuditEnabled() && isExternalInvocation()) { - resultingStat = dir.getFileInfo(dst, false); - } + resultingStat = getAuditFileInfo(dst, false); } finally { writeUnlock(); } getEditLog().logSync(); - if (isAuditEnabled() && isExternalInvocation()) { + if (resultingStat != null) { StringBuilder cmd = new StringBuilder("rename options="); for (Rename option : options) { cmd.append(option.value()).append(" "); } - logAuditEvent(UserGroupInformation.getCurrentUser(), getRemoteIp(), - cmd.toString(), src, dst, resultingStat); + logAuditEvent(true, cmd.toString(), src, dst, resultingStat); } } @@ -2769,11 +2695,7 @@ boolean delete(String src, boolean recursive) try { return deleteInt(src, recursive); } catch (AccessControlException e) { - if (isAuditEnabled() && isExternalInvocation()) { - logAuditEvent(false, UserGroupInformation.getCurrentUser(), - getRemoteIp(), - "delete", src, null, null); - } + logAuditEvent(false, "delete", src); throw e; } } @@ -2785,10 +2707,8 @@ private boolean deleteInt(String src, boolean recursive) NameNode.stateChangeLog.debug("DIR* NameSystem.delete: " + src); } boolean status = deleteInternal(src, recursive, true); - if (status && isAuditEnabled() && isExternalInvocation()) { - logAuditEvent(UserGroupInformation.getCurrentUser(), - getRemoteIp(), - "delete", src, null, null); + if (status) { + logAuditEvent(true, "delete", src); } return status; } @@ -2944,20 +2864,12 @@ HdfsFileStatus getFileInfo(String src, boolean resolveLink) } stat = dir.getFileInfo(src, resolveLink); } catch (AccessControlException e) { - if (isAuditEnabled() && isExternalInvocation()) { - logAuditEvent(false, UserGroupInformation.getCurrentUser(), - getRemoteIp(), - "getfileinfo", src, null, null); - } + logAuditEvent(false, "getfileinfo", src); throw e; } finally { readUnlock(); } - if (isAuditEnabled() && isExternalInvocation()) { - logAuditEvent(UserGroupInformation.getCurrentUser(), - getRemoteIp(), - "getfileinfo", src, null, null); - } + logAuditEvent(true, "getfileinfo", src); return stat; } @@ -2969,17 +2881,14 @@ boolean mkdirs(String src, PermissionStatus permissions, try { return mkdirsInt(src, permissions, createParent); } catch (AccessControlException e) { - if (isAuditEnabled() && isExternalInvocation()) { - logAuditEvent(false, UserGroupInformation.getCurrentUser(), - getRemoteIp(), - "mkdirs", src, null, null); - } + logAuditEvent(false, "mkdirs", src); throw e; } } private boolean mkdirsInt(String src, PermissionStatus permissions, boolean createParent) throws IOException, UnresolvedLinkException { + HdfsFileStatus resultingStat = null; boolean status = false; if(NameNode.stateChangeLog.isDebugEnabled()) { NameNode.stateChangeLog.debug("DIR* NameSystem.mkdirs: " + src); @@ -2989,15 +2898,15 @@ private boolean mkdirsInt(String src, PermissionStatus permissions, try { checkOperation(OperationCategory.WRITE); status = mkdirsInternal(pc, src, permissions, createParent); + if (status) { + resultingStat = dir.getFileInfo(src, false); + } } finally { writeUnlock(); } getEditLog().logSync(); - if (status && isAuditEnabled() && isExternalInvocation()) { - final HdfsFileStatus stat = dir.getFileInfo(src, false); - logAuditEvent(UserGroupInformation.getCurrentUser(), - getRemoteIp(), - "mkdirs", src, null, stat); + if (status) { + logAuditEvent(true, "mkdirs", src, null, resultingStat); } return status; } @@ -3426,11 +3335,7 @@ DirectoryListing getListing(String src, byte[] startAfter, try { return getListingInt(src, startAfter, needLocation); } catch (AccessControlException e) { - if (isAuditEnabled() && isExternalInvocation()) { - logAuditEvent(false, UserGroupInformation.getCurrentUser(), - getRemoteIp(), - "listStatus", src, null, null); - } + logAuditEvent(false, "listStatus", src); throw e; } } @@ -3451,11 +3356,7 @@ private DirectoryListing getListingInt(String src, byte[] startAfter, checkTraverse(pc, src); } } - if (isAuditEnabled() && isExternalInvocation()) { - logAuditEvent(UserGroupInformation.getCurrentUser(), - getRemoteIp(), - "listStatus", src, null, null); - } + logAuditEvent(true, "listStatus", src); dl = dir.getListing(src, startAfter, needLocation); } finally { readUnlock(); @@ -5202,7 +5103,7 @@ Token getDelegationToken(Text renewer) return null; } - UserGroupInformation ugi = UserGroupInformation.getCurrentUser(); + UserGroupInformation ugi = getRemoteUser(); String user = ugi.getUserName(); Text owner = new Text(user); Text realUser = null; @@ -5243,7 +5144,7 @@ long renewDelegationToken(Token token) throw new IOException( "Delegation Token can be renewed only with kerberos or web authentication"); } - String renewer = UserGroupInformation.getCurrentUser().getShortUserName(); + String renewer = getRemoteUser().getShortUserName(); expiryTime = dtSecretManager.renewToken(token, renewer); DelegationTokenIdentifier id = new DelegationTokenIdentifier(); ByteArrayInputStream buf = new ByteArrayInputStream(token.getIdentifier()); @@ -5271,7 +5172,7 @@ void cancelDelegationToken(Token token) if (isInSafeMode()) { throw new SafeModeException("Cannot cancel delegation token", safeMode); } - String canceller = UserGroupInformation.getCurrentUser().getUserName(); + String canceller = getRemoteUser().getUserName(); DelegationTokenIdentifier id = dtSecretManager .cancelToken(token, canceller); getEditLog().logCancelDelegationToken(id); @@ -5340,7 +5241,7 @@ private boolean isAllowedDelegationTokenOp() throws IOException { */ private AuthenticationMethod getConnectionAuthenticationMethod() throws IOException { - UserGroupInformation ugi = UserGroupInformation.getCurrentUser(); + UserGroupInformation ugi = getRemoteUser(); AuthenticationMethod authMethod = ugi.getAuthenticationMethod(); if (authMethod == AuthenticationMethod.PROXY) { authMethod = ugi.getRealUser().getAuthenticationMethod(); @@ -5364,12 +5265,22 @@ private static InetAddress getRemoteIp() { return NamenodeWebHdfsMethods.getRemoteIp(); } + // optimize ugi lookup for RPC operations to avoid a trip through + // UGI.getCurrentUser which is synch'ed + private static UserGroupInformation getRemoteUser() throws IOException { + UserGroupInformation ugi = null; + if (Server.isRpcInvocation()) { + ugi = Server.getRemoteUser(); + } + return (ugi != null) ? ugi : UserGroupInformation.getCurrentUser(); + } + /** * Log fsck event in the audit log */ void logFsckEvent(String src, InetAddress remoteAddress) throws IOException { if (isAuditEnabled()) { - logAuditEvent(UserGroupInformation.getCurrentUser(), + logAuditEvent(true, getRemoteUser(), remoteAddress, "fsck", src, null, null); }