You are viewing a plain text version of this content. The canonical link for it is here.
Posted to hdfs-issues@hadoop.apache.org by "Wei-Chiu Chuang (Jira)" <ji...@apache.org> on 2019/08/23 00:09:00 UTC
[jira] [Comment Edited] (HDFS-11246) FSNameSystem#logAuditEvent
should be called outside the read or write locks
[ https://issues.apache.org/jira/browse/HDFS-11246?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16913808#comment-16913808 ]
Wei-Chiu Chuang edited comment on HDFS-11246 at 8/23/19 12:08 AM:
------------------------------------------------------------------
Almost good to go. I think there are only two missing.
* checkAccess
readUnlock() is called after logAuditEvent() upon ACE.
* truncate
truncate() looks a little inconsistent.
Most of operations are written as
{code}
try {
writeLock();
try {
...
} finally {
writeUnlock(operationName);
}
} catch (AccessControlException ace) {
logAuditEvent(false, ... );
throw ace;
}
getEditLog().logSync();
logAuditEvent(true, ... );
{code}
and read operations are:
{code}
try {
readLock();
try {
...
} finally {
readUnlock(operationName);
}
} catch (AccessControlException ace) {
logAuditEvent(false, ... );
throw ace;
}
logAuditEvent(true, ... );
{code}
and some are modeled as
{code}
readLock();
try {
....
success = true;
} finally {
readUnlock(operationName);
logAuditEvent(success);
}
{code}
Unrelated:
1. FSNamesystem is due for a refactor. Some of the methods are 6-level (if, for, ...) deep.
A few operations logs audit log before edit log sync. I don't think that's a big deal but consistency makes it easier to review.
The following are nice-to-have but I'd rather to have them addressed in a separate jira.
1. There are a few inconsistencies in 13 write-lock handlers ,
{code}
} finally {
if (success) {
getEditLog().logSync();
}
}
{code}
To keep consistency, it should be rewritten as
{code}
getEditLog().logSync();
{code}
I.e.: move it out of finally block, and remove the if check.
======
EDIT: I think the following are non-issues.
2. Following operations don't record audit log upon success.
* isFileClosed --> probably make sense
* checkAccess --> make sense
3. Following operations don't record audit log upon AccessControlException (looks like they are all superuser operations).
* metaSave
* datanodeReport
* getDatanodeStorageReport
* saveNamespace
* restoreFailedStorage
* finalizeUpgrade
* refreshNodes
* setBalancerBandwidth
* setSafeMode
* rollEditLog
* allowSnapshot
* disallowSnapshot
* queryRollingUpgrade
* startRollingUpgrade
* finalizeRollingUpgrade
was (Author: jojochuang):
Almost good to go. I think there are only two missing.
* checkAccess
readUnlock() is called after logAuditEvent() upon ACE.
* truncate
truncate() looks a little inconsistent.
Most of operations are written as
{code}
try {
writeLock();
try {
...
} finally {
writeUnlock(operationName);
}
} catch (AccessControlException ace) {
logAuditEvent(false, ... );
throw ace;
}
getEditLog().logSync();
logAuditEvent(true, ... );
{code}
and read operations are:
{code}
try {
readLock();
try {
...
} finally {
readUnlock(operationName);
}
} catch (AccessControlException ace) {
logAuditEvent(false, ... );
throw ace;
}
logAuditEvent(true, ... );
{code}
and some are modeled as
{code}
readLock();
try {
....
success = true;
} finally {
readUnlock(operationName);
logAuditEvent(success);
}
{code}
Unrelated:
1. FSNamesystem is due for a refactor. Some of the methods are 6-level (if, for, ...) deep.
A few operations logs audit log before edit log sync. I don't think that's a big deal but consistency makes it easier to review.
The following are nice-to-have but I'd rather to have them addressed in a separate jira.
1. There are a few inconsistencies in 13 write-lock handlers ,
{code}
} finally {
if (success) {
getEditLog().logSync();
}
}
{code}
To keep consistency, it should be rewritten as
{code}
getEditLog().logSync();
{code}
I.e.: move it out of finally block, and remove the if check.
2. Following operations don't record audit log upon success.
* isFileClosed
* checkAccess --> make sense
3. Following operations don't record audit log upon AccessControlException (looks like they are all superuser operations).
* metaSave
* datanodeReport
* getDatanodeStorageReport
* saveNamespace
* restoreFailedStorage
* finalizeUpgrade
* refreshNodes
* setBalancerBandwidth
* setSafeMode
* rollEditLog
* allowSnapshot
* disallowSnapshot
* queryRollingUpgrade
* startRollingUpgrade
* finalizeRollingUpgrade
> FSNameSystem#logAuditEvent should be called outside the read or write locks
> ---------------------------------------------------------------------------
>
> Key: HDFS-11246
> URL: https://issues.apache.org/jira/browse/HDFS-11246
> Project: Hadoop HDFS
> Issue Type: Bug
> Affects Versions: 2.7.3
> Reporter: Kuhu Shukla
> Assignee: He Xiaoqiao
> Priority: Major
> Attachments: HDFS-11246.001.patch, HDFS-11246.002.patch, HDFS-11246.003.patch, HDFS-11246.004.patch, HDFS-11246.005.patch, HDFS-11246.006.patch, HDFS-11246.007.patch, HDFS-11246.008.patch, HDFS-11246.009.patch
>
>
> {code}
> readLock();
> boolean success = true;
> ContentSummary cs;
> try {
> checkOperation(OperationCategory.READ);
> cs = FSDirStatAndListingOp.getContentSummary(dir, src);
> } catch (AccessControlException ace) {
> success = false;
> logAuditEvent(success, operationName, src);
> throw ace;
> } finally {
> readUnlock(operationName);
> }
> {code}
> It would be nice to have audit logging outside the lock esp. in scenarios where applications hammer a given operation several times.
--
This message was sent by Atlassian Jira
(v8.3.2#803003)
---------------------------------------------------------------------
To unsubscribe, e-mail: hdfs-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: hdfs-issues-help@hadoop.apache.org