Hadoop Map/Reduce
  1. Hadoop Map/Reduce
  2. MAPREDUCE-1543

Log messages of JobACLsManager should use security logging of HADOOP-6586

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.22.0
    • Component/s: security
    • Labels:
    • Hadoop Flags:
      Reviewed
    • Release Note:
      Adds the audit logging facility to MapReduce. All authorization/authentication events are logged to audit log. Audit log entries are stored as key=value.

      Description

      JobACLsManager added in MAPREDUCE-1307 logs the successes and failures w.r.t job-level authorization in the corresponding Daemons' logs. The log messages should instead use security logging of HADOOP-6586.

      1. hadoop-mapreduce.audit.log
        7 kB
        Amar Kamat
      2. M1543-3.patch
        19 kB
        Chris Douglas
      3. mapreduce-1543-y20s.patch
        11 kB
        Hemanth Yamijala
      4. mapreduce-1543-y20s-3.patch
        18 kB
        Hemanth Yamijala
      5. mr-1543-trunk-v2.patch
        20 kB
        Luke Lu
      6. mr-1543-v1.9.2.patch
        20 kB
        Amar Kamat

        Activity

        Hide
        Vinod Kumar Vavilapalli added a comment -

        Guess we should also pipe QueueManager's access related logs also to security logging.

        Show
        Vinod Kumar Vavilapalli added a comment - Guess we should also pipe QueueManager 's access related logs also to security logging.
        Hide
        Amar Kamat added a comment -

        What to log?

        • job related operations
          • job-view
          • job-modify
          • job-submission
          • job-initialization
        • queue acl related operations
          • queue-refresh
        • cluster acl related operations
          • host-list-refresh
          • node-list-refresh

        I am in the process of merging service-acls-refresh (commons code, HADOOP-6586) with mapreduce audit logs.

        How to log?

        Considering the above mentioned scenarios, here is format for mapreduce audit (security?) logs

        <date> <log-level> <log-class>.audit <operation> by <agent> on <target> : <result> [<reason>]

        Example :

        2010-03-11 00:48:44,979 INFO org.apache.hadoop.mapred.JobTracker.audit : SUBMIT_JOB by amarrk on job_201003110048_0001 : SUCCESS [  ]
        2010-03-11 00:48:45,648 INFO org.apache.hadoop.mapred.JobInProgress.audit : INIT_JOB by amarrk on job_201003110048_0001 : SUCCESS [ maps : 1, reduces : 0 ]
        2010-03-11 10:49:01,154 INFO org.apache.hadoop.mapred.JobTracker.audit : SUBMIT_JOB by amarrk on job_201003111048_0001 : SUCCESS [  ]
        2010-03-11 10:49:01,811 INFO org.apache.hadoop.mapred.JobInProgress.audit : INIT_JOB by amarrk on job_201003111048_0001 : FAILURE [ Total tasks : 11, Max tasks : 10 ]
        2010-03-11 12:26:11,158 INFO AuditLogger: org.apache.hadoop.mapred.JobTracker : NODE_REFRESH by hacker on JobTracker : FAILURE [ Access denied ]
        

        The reason for adding '.audit' to the classnames is for the ease of filtering them out and also be consistent with the hdfs audit logging naming convention.

        Thoughts?

        Show
        Amar Kamat added a comment - What to log? job related operations job-view job-modify job-submission job-initialization queue acl related operations queue-refresh cluster acl related operations host-list-refresh node-list-refresh I am in the process of merging service-acls-refresh (commons code, HADOOP-6586 ) with mapreduce audit logs. How to log? Considering the above mentioned scenarios, here is format for mapreduce audit (security?) logs <date> <log-level> <log-class>.audit <operation> by <agent> on <target> : <result> [<reason>] Example : 2010-03-11 00:48:44,979 INFO org.apache.hadoop.mapred.JobTracker.audit : SUBMIT_JOB by amarrk on job_201003110048_0001 : SUCCESS [ ] 2010-03-11 00:48:45,648 INFO org.apache.hadoop.mapred.JobInProgress.audit : INIT_JOB by amarrk on job_201003110048_0001 : SUCCESS [ maps : 1, reduces : 0 ] 2010-03-11 10:49:01,154 INFO org.apache.hadoop.mapred.JobTracker.audit : SUBMIT_JOB by amarrk on job_201003111048_0001 : SUCCESS [ ] 2010-03-11 10:49:01,811 INFO org.apache.hadoop.mapred.JobInProgress.audit : INIT_JOB by amarrk on job_201003111048_0001 : FAILURE [ Total tasks : 11, Max tasks : 10 ] 2010-03-11 12:26:11,158 INFO AuditLogger: org.apache.hadoop.mapred.JobTracker : NODE_REFRESH by hacker on JobTracker : FAILURE [ Access denied ] The reason for adding ' .audit ' to the classnames is for the ease of filtering them out and also be consistent with the hdfs audit logging naming convention. Thoughts?
        Hide
        Amar Kamat added a comment -

        2010-03-11 10:49:01,811 INFO org.apache.hadoop.mapred.JobInProgress.audit : INIT_JOB by amarrk on job_201003111048_0001 : FAILURE [ Total tasks : 11, Max tasks : 10 ]
        2010-03-11 12:26:11,158 INFO AuditLogger: org.apache.hadoop.mapred.JobTracker : NODE_REFRESH by hacker on JobTracker : FAILURE [ Access denied ]

        There are 2 ways to print the audit log,

        1. Using AuditLogger as a keyword (better readability)
        2. Using .audit as the keyword (consistent with the hdfs audit-log naming convention)
        Show
        Amar Kamat added a comment - 2010-03-11 10:49:01,811 INFO org.apache.hadoop.mapred.JobInProgress.audit : INIT_JOB by amarrk on job_201003111048_0001 : FAILURE [ Total tasks : 11, Max tasks : 10 ] 2010-03-11 12:26:11,158 INFO AuditLogger: org.apache.hadoop.mapred.JobTracker : NODE_REFRESH by hacker on JobTracker : FAILURE [ Access denied ] There are 2 ways to print the audit log, Using AuditLogger as a keyword (better readability) Using .audit as the keyword (consistent with the hdfs audit-log naming convention)
        Hide
        Hemanth Yamijala added a comment -

        A few questions:

        • Is the format also similar to HDFS audit logs ? You mentioned about .audit suffix being based on HDFS conventions, but is there a convention in the log message format also ? To the extent if this format is similar to HDFS, it will help.
        • Do we need to include host IP of the requestor ? I don't even know if it is possible to get this information though.
        • One concern with implementation is - if some of this logging is happening under the jobtracker lock, it might impact performance adversely. Can we plan to handle this ?
        Show
        Hemanth Yamijala added a comment - A few questions: Is the format also similar to HDFS audit logs ? You mentioned about .audit suffix being based on HDFS conventions, but is there a convention in the log message format also ? To the extent if this format is similar to HDFS, it will help. Do we need to include host IP of the requestor ? I don't even know if it is possible to get this information though. One concern with implementation is - if some of this logging is happening under the jobtracker lock, it might impact performance adversely. Can we plan to handle this ?
        Hide
        Amar Kamat added a comment -

        Is the format also similar to HDFS audit logs ?

        The format for audit logs used by hdfs is hdfs friendly. They log

        ugi
        remote IP
        command
        src path
        dst path (optional)
        permissions (optional)
        

        We might try to come up with a model which both can use (and add it to commons). So here is how the mapping from hdfs audit-log-format to mapreduce audit-log-format might look like

        hdfs mapreduce
        ugi agent
        remote-ip -
        command operation
        src-path target
        dst path -
        permissions -
        - result
        - reason

        So here is a straight forward merge :

        <agent> <remote-ip> <operation> <target> <permissions*> <result*> <reason*>
        * means optional
        

        So for hdfs, target will be src-path:dest-path. And for mapreduce, we could skip permissions or print acls. But the only point that doesnt fit this model for mapreduce is the job-initialization event. For job-initialization, what should be the value of remote-ip?

        Not sure if we are doing an overfit. So for now I think we can keep it simple and have different models for hdfs and mapreduce.

        Do we need to include host IP of the requestor ? I don't even know if it is possible to get this information though.

        I am not sure how that will help. I think username should suffice. It is possible to get the IP of the caller using o.a.h.ipc.Server.getRemoteIp().

        One concern with implementation is - if some of this logging is happening under the jobtracker lock, it might impact performance adversely. Can we plan to handle this ?

        The idea here is to replace LOG.* statements with AUDIT_LOG.*. So in terms of logging overhead, it should be same. In my initial implementation exercise, I have not seen a case where I had to add extra log lines. Let me check if this needs to be addressed.

        Show
        Amar Kamat added a comment - Is the format also similar to HDFS audit logs ? The format for audit logs used by hdfs is hdfs friendly. They log ugi remote IP command src path dst path (optional) permissions (optional) We might try to come up with a model which both can use (and add it to commons). So here is how the mapping from hdfs audit-log-format to mapreduce audit-log-format might look like hdfs mapreduce ugi agent remote-ip - command operation src-path target dst path - permissions - - result - reason So here is a straight forward merge : <agent> <remote-ip> <operation> <target> <permissions*> <result*> <reason*> * means optional So for hdfs, target will be src-path:dest-path. And for mapreduce, we could skip permissions or print acls. But the only point that doesnt fit this model for mapreduce is the job-initialization event. For job-initialization, what should be the value of remote-ip? Not sure if we are doing an overfit. So for now I think we can keep it simple and have different models for hdfs and mapreduce. Do we need to include host IP of the requestor ? I don't even know if it is possible to get this information though. I am not sure how that will help. I think username should suffice. It is possible to get the IP of the caller using o.a.h.ipc.Server.getRemoteIp() . One concern with implementation is - if some of this logging is happening under the jobtracker lock, it might impact performance adversely. Can we plan to handle this ? The idea here is to replace LOG.* statements with AUDIT_LOG.*. So in terms of logging overhead, it should be same. In my initial implementation exercise, I have not seen a case where I had to add extra log lines. Let me check if this needs to be addressed.
        Hide
        Hemanth Yamijala added a comment -

        Amar,

        Primarily I am thinking that having a format close to HDFS is good, because the HDFS audit log has been around for a while now and is probably something users are used to. That said, I think we may also want to keep in mind the cost of getting all the information to keep the two logs similar.

        I had a discussion with Vinod and Ravi also about this. To me, printing UGI (in place of agent, which is just user name) and remote-ip would be good. However, opinion is not fully converging on this. Ravi and Vinod feel UGI might be too verbose and also getting the groups for a user could impact performance if the groups are not cached.

        Remote IP is very useful, IMO. If something failed, having the remote IP will help identify the source of trouble. I am even thinking there might be cases where valid users due to misconfigured nodes could face failures. And logging the remote IP will help weed out these misconfigurations.

        Given the above, one thought is to have groups and remote-ip optional, and log them only for failures.

        Permissions equals ACLs for us. ACLs can be verbose too. Hence, it falls in the same category as the above two fields.

        I would also suggest a key=value kind of format for this. If HDFS is also using the same, I think this is definitely the way to go.

        Show
        Hemanth Yamijala added a comment - Amar, Primarily I am thinking that having a format close to HDFS is good, because the HDFS audit log has been around for a while now and is probably something users are used to. That said, I think we may also want to keep in mind the cost of getting all the information to keep the two logs similar. I had a discussion with Vinod and Ravi also about this. To me, printing UGI (in place of agent, which is just user name) and remote-ip would be good. However, opinion is not fully converging on this. Ravi and Vinod feel UGI might be too verbose and also getting the groups for a user could impact performance if the groups are not cached. Remote IP is very useful, IMO. If something failed, having the remote IP will help identify the source of trouble. I am even thinking there might be cases where valid users due to misconfigured nodes could face failures. And logging the remote IP will help weed out these misconfigurations. Given the above, one thought is to have groups and remote-ip optional, and log them only for failures. Permissions equals ACLs for us. ACLs can be verbose too. Hence, it falls in the same category as the above two fields. I would also suggest a key=value kind of format for this. If HDFS is also using the same, I think this is definitely the way to go.
        Hide
        Hemanth Yamijala added a comment -

        Attaching a patch for earlier version of Hadoop, on behalf of Amar. This is not for commit here, but it is illustrative of the approach being planned for trunk as well.

        Show
        Hemanth Yamijala added a comment - Attaching a patch for earlier version of Hadoop, on behalf of Amar. This is not for commit here, but it is illustrative of the approach being planned for trunk as well.
        Hide
        Hemanth Yamijala added a comment -

        Some review comments on the patch uploaded (mapreduce-1543-y20s.patch):

        Most of the comments are minor adjustments to the log API to make it a little of cleaner.. Overall the approach in the patch seems clean to me.

        • I think AuditLogManager can be pulled into a separate class file.
        • Keeping the list of keys as javadoc comments seems like a maintenance overhead. I would instead suggest that the Keys be defined as an Enum. Then it will be easy to query the Enum names as an API. Doing this will also prevent the duplication of the key names in the logSuccess and logFailure APIs.
        • I am not sure 'agent' is conveying the right meaning. Seems like 'user' or 'userid' will be more clear. What is the equivalent in DFS ? Also, ip should be 'remote-ip'.
        • Reason seems confusing. Can we keep it generic - maybe as a string ? For e.g. in JobInProgress, where we check the conf user is the same as the submitting user, and fails, 'permission denied' is not giving a proper meaning. We can expand and say what exactly failed. In fact, maybe we can change the name of the tag 'Reason' to 'AdditionalInfo' or something like that so it is more general. Then, in QueueManager, I think it makes sense to log the jobid, when it is available (note there are cases when it is not available), maybe as additional Info. This way we can do things like grep by job-id and get a full trail.
        • Don't tabs make the lines appear too long, Blanks would suffice, no ?
        • Some values like 'permissions' and 'reason' (if this is a free format string) can have whitespaces. Would it be good to surround these with quotes for easy parsing ?
        • Suggest the parameter 'operation' could be of type String as well - if we are going to do a toString anyway.
        • If there's no cost to logging remote-ip, I'd recommend adding it everywhere as a mandatory parameter.
        • The comment '//for testcases' is misleading. I think what you mean is that the check for IP being null is required as test cases may not have this defined. I'd suggest it be documented as such.
        • Why are we logging success again in JobTracker.addJob, wouldn't the same line come up in QueueManager.checkAccess ?
        • Seems like Operations "REFRESH_NODES", "QUEUE_REFRESH" and target "Jobtracker" can be static final variables. Should we pull all such AuditLogManager related constants into some constants file ? Like AuditLogConstants or something ?
        • "REFRESH_NODES" and "QUEUE_REFRESH" seem similar operations, but they have different names, it should be "REFRESH_NODES" and "REFRESH_QUEUES"
        • JobACLsManager.checkAccess has some lines incorrectly indented.
        • I don't think we need to give 'Queue:' prefix, it is understood that if it is a queue operation, then the target will be a queue.
        • Failure logging should be at WARN level - as done in the other audit log.
        • Can we move the refreshAcls logging into QueueManager.refreshQueueAcls - so that failures can be logged too.
        • Can we add a few simple unit tests that test the formatting of the log message alone. i.e. we can define a package private API in AuditLogManager - something like String getLogLine(...). We can call these in the log*** APIs in AuditLogManager. We can also call it with various arguments in tests and verify that the return string is proper. We can test null values, values with whitespace etc.
        Show
        Hemanth Yamijala added a comment - Some review comments on the patch uploaded (mapreduce-1543-y20s.patch): Most of the comments are minor adjustments to the log API to make it a little of cleaner.. Overall the approach in the patch seems clean to me. I think AuditLogManager can be pulled into a separate class file. Keeping the list of keys as javadoc comments seems like a maintenance overhead. I would instead suggest that the Keys be defined as an Enum. Then it will be easy to query the Enum names as an API. Doing this will also prevent the duplication of the key names in the logSuccess and logFailure APIs. I am not sure 'agent' is conveying the right meaning. Seems like 'user' or 'userid' will be more clear. What is the equivalent in DFS ? Also, ip should be 'remote-ip'. Reason seems confusing. Can we keep it generic - maybe as a string ? For e.g. in JobInProgress, where we check the conf user is the same as the submitting user, and fails, 'permission denied' is not giving a proper meaning. We can expand and say what exactly failed. In fact, maybe we can change the name of the tag 'Reason' to 'AdditionalInfo' or something like that so it is more general. Then, in QueueManager, I think it makes sense to log the jobid, when it is available (note there are cases when it is not available), maybe as additional Info. This way we can do things like grep by job-id and get a full trail. Don't tabs make the lines appear too long, Blanks would suffice, no ? Some values like 'permissions' and 'reason' (if this is a free format string) can have whitespaces. Would it be good to surround these with quotes for easy parsing ? Suggest the parameter 'operation' could be of type String as well - if we are going to do a toString anyway. If there's no cost to logging remote-ip, I'd recommend adding it everywhere as a mandatory parameter. The comment '//for testcases' is misleading. I think what you mean is that the check for IP being null is required as test cases may not have this defined. I'd suggest it be documented as such. Why are we logging success again in JobTracker.addJob, wouldn't the same line come up in QueueManager.checkAccess ? Seems like Operations "REFRESH_NODES", "QUEUE_REFRESH" and target "Jobtracker" can be static final variables. Should we pull all such AuditLogManager related constants into some constants file ? Like AuditLogConstants or something ? "REFRESH_NODES" and "QUEUE_REFRESH" seem similar operations, but they have different names, it should be "REFRESH_NODES" and "REFRESH_QUEUES" JobACLsManager.checkAccess has some lines incorrectly indented. I don't think we need to give 'Queue:' prefix, it is understood that if it is a queue operation, then the target will be a queue. Failure logging should be at WARN level - as done in the other audit log. Can we move the refreshAcls logging into QueueManager.refreshQueueAcls - so that failures can be logged too. Can we add a few simple unit tests that test the formatting of the log message alone. i.e. we can define a package private API in AuditLogManager - something like String getLogLine(...). We can call these in the log*** APIs in AuditLogManager. We can also call it with various arguments in tests and verify that the return string is proper. We can test null values, values with whitespace etc.
        Hide
        Amar Kamat added a comment -

        I think AuditLogManager can be pulled into a separate class file.

        +1.

        I would instead suggest that the Keys be defined as an Enum

        +1

        I am not sure 'agent' is conveying the right meaning.

        HDFS uses ugi. As we discussed, we would be logging the usernames in MapReduce. I think agent can act as a common word for ugi or username. Agent is someone who is external to the system and tries to cause a state change. I am ok with changing it to something better. I named the IP key as 'ip' because hdfs kept it that way.

        Reason seems confusing.

        I have removed it in my next patch. It doesnt add much value to MapReduce because we are dealing with authorization events here. The reason why I made them enums was for ease of parsing. Having strings in key=value formats can lead to issues to do with spaces etc.

        Don't tabs make the lines appear too long,

        This is inline with hdfs.

        Some values like 'permissions' (if this is a free format string) can have whitespaces

        I would rather prefer having one word representation for permissions for audit logs. Example

        user{u1,u2,u3}:group{g1,g2,g3}
        

        Key=Value pairs with free format strings can lead to issues. We can quote them but the parser logic would become complex.

        Suggest the parameter 'operation' could be of type String as well.

        I would rather prefer Enums and have JobTracker.Operations and JobInProgress.Operations. Note we already have QueueManager.QueueOperations.

        If there's no cost to logging remote-ip,

        +1

        The comment '//for testcases' is misleading.

        +1

        Why are we logging success again in JobTracker.addJob, wouldn't the same line come up in QueueManager.checkAccess ?

        Because they are 2 authorization checks involved :
        1) Queue access (success or failure)
        2) Job submission (success or failure based on username in config)

        Seems like Operations "REFRESH_NODES", "QUEUE_REFRESH" and target "Jobtracker" can be static final variables. Should we pull all such AuditLogManager related constants into some constants file ? Like AuditLogConstants or something ?

        +1 for factoring it out.

        I don't think we need to give 'Queue:' prefix,

        +1

        Failure logging should be at WARN level - as done in the other audit log.

        If audit logs are important then what is the point of making some of them WARN, ERROR or FATAL? Shouldn't all the audit logs be INFO logs?

        Can we move the refreshAcls logging into QueueManager.refreshQueueAcls - so that failures can be logged too.

        +1

        Can we add a few simple unit tests that test the formatting of the log message alone

        +1

        Show
        Amar Kamat added a comment - I think AuditLogManager can be pulled into a separate class file. +1. I would instead suggest that the Keys be defined as an Enum +1 I am not sure 'agent' is conveying the right meaning. HDFS uses ugi. As we discussed, we would be logging the usernames in MapReduce. I think agent can act as a common word for ugi or username. Agent is someone who is external to the system and tries to cause a state change. I am ok with changing it to something better. I named the IP key as 'ip' because hdfs kept it that way. Reason seems confusing. I have removed it in my next patch. It doesnt add much value to MapReduce because we are dealing with authorization events here. The reason why I made them enums was for ease of parsing. Having strings in key=value formats can lead to issues to do with spaces etc. Don't tabs make the lines appear too long, This is inline with hdfs. Some values like 'permissions' (if this is a free format string) can have whitespaces I would rather prefer having one word representation for permissions for audit logs. Example user{u1,u2,u3}:group{g1,g2,g3} Key=Value pairs with free format strings can lead to issues. We can quote them but the parser logic would become complex. Suggest the parameter 'operation' could be of type String as well. I would rather prefer Enums and have JobTracker.Operations and JobInProgress.Operations. Note we already have QueueManager.QueueOperations. If there's no cost to logging remote-ip, +1 The comment '//for testcases' is misleading. +1 Why are we logging success again in JobTracker.addJob, wouldn't the same line come up in QueueManager.checkAccess ? Because they are 2 authorization checks involved : 1) Queue access (success or failure) 2) Job submission (success or failure based on username in config) Seems like Operations "REFRESH_NODES", "QUEUE_REFRESH" and target "Jobtracker" can be static final variables. Should we pull all such AuditLogManager related constants into some constants file ? Like AuditLogConstants or something ? +1 for factoring it out. I don't think we need to give 'Queue:' prefix, +1 Failure logging should be at WARN level - as done in the other audit log. If audit logs are important then what is the point of making some of them WARN, ERROR or FATAL? Shouldn't all the audit logs be INFO logs? Can we move the refreshAcls logging into QueueManager.refreshQueueAcls - so that failures can be logged too. +1 Can we add a few simple unit tests that test the formatting of the log message alone +1
        Hide
        Hemanth Yamijala added a comment -

        HDFS uses ugi. As we discussed, we would be logging the usernames in MapReduce. I think agent can act as a common word for ugi or username.

        I prefer userid. Ok with ip, if it maintains parity with HDFS.

        [tabs] inline with HDFS.

        +1

        I would rather prefer having one word representation for permissions for audit logs.

        I think the toString of ACLs should be maintained and cannot be changed, it is a constraint. Further, while permissions is one example, I think there may be other values with spaces. Since we are using tabs as delimiters, maybe this is less of an issue now.

        Reason seems confusing.

        Can you confirm if we are changing this to AdditionalInfo, I think that's more general.

        I would rather prefer Enums and have JobTracker.Operations and JobInProgress.Operations. Note we already have QueueManager.QueueOperations.

        Umm, but we'll still need to treat these as 'Object' parameters and do a toString on them to get a log, right ? I see that as no different from passing a string.

        Two logs for submitJob because they are 2 authorization checks involved.

        My concern is that in a success case, there are multiple lines indicating that job submission succeeded - one for the queue and one in submitJob itself. But possibly this is OK.

        If audit logs are important then what is the point of making some of them WARN, ERROR or FATAL? Shouldn't all the audit logs be INFO logs?

        This was inline with the other audit patch and hence my suggestion. Failure logs are more important than Success logs. Hence to optimize for e.g. we can turn off success logs by configuring log level. I would prefer this be done.

        Show
        Hemanth Yamijala added a comment - HDFS uses ugi. As we discussed, we would be logging the usernames in MapReduce. I think agent can act as a common word for ugi or username. I prefer userid. Ok with ip, if it maintains parity with HDFS. [tabs] inline with HDFS. +1 I would rather prefer having one word representation for permissions for audit logs. I think the toString of ACLs should be maintained and cannot be changed, it is a constraint. Further, while permissions is one example, I think there may be other values with spaces. Since we are using tabs as delimiters, maybe this is less of an issue now. Reason seems confusing. Can you confirm if we are changing this to AdditionalInfo, I think that's more general. I would rather prefer Enums and have JobTracker.Operations and JobInProgress.Operations. Note we already have QueueManager.QueueOperations. Umm, but we'll still need to treat these as 'Object' parameters and do a toString on them to get a log, right ? I see that as no different from passing a string. Two logs for submitJob because they are 2 authorization checks involved. My concern is that in a success case, there are multiple lines indicating that job submission succeeded - one for the queue and one in submitJob itself. But possibly this is OK. If audit logs are important then what is the point of making some of them WARN, ERROR or FATAL? Shouldn't all the audit logs be INFO logs? This was inline with the other audit patch and hence my suggestion. Failure logs are more important than Success logs. Hence to optimize for e.g. we can turn off success logs by configuring log level. I would prefer this be done.
        Hide
        Amar Kamat added a comment -

        I prefer userid.

        Is ugi fine? The reason being that we log username just for performance reasons. In near future we might include group information too. It can we viewed as a ugi with masked group information.

        I think the toString of ACLs should be maintained and cannot be changed, it is a constraint. ..... Since we are using tabs as delimiters, maybe this is less of an issue now.

        +1

        Can you confirm if we are changing this to AdditionalInfo, I think that's more general.

        We can name it as 'description' or maybe 'additional-info'. I am fine with both. +1 for making it a string.

        Umm, but we'll still need to treat these as 'Object' parameters and do a toString on them to get a log, right ? I see that as no different from passing a string.

        +1.

        This was inline with the other audit patch and hence my suggestion.... we can turn off success logs by configuring log level

        +1. Good point.

        Show
        Amar Kamat added a comment - I prefer userid. Is ugi fine? The reason being that we log username just for performance reasons. In near future we might include group information too. It can we viewed as a ugi with masked group information. I think the toString of ACLs should be maintained and cannot be changed, it is a constraint. ..... Since we are using tabs as delimiters, maybe this is less of an issue now. +1 Can you confirm if we are changing this to AdditionalInfo, I think that's more general. We can name it as 'description' or maybe 'additional-info'. I am fine with both. +1 for making it a string. Umm, but we'll still need to treat these as 'Object' parameters and do a toString on them to get a log, right ? I see that as no different from passing a string. +1. This was inline with the other audit patch and hence my suggestion.... we can turn off success logs by configuring log level +1. Good point.
        Hide
        Hemanth Yamijala added a comment -

        I hope users won't start expecting the group information will be there if we call it UGI. Currently, we are logging user everywhere, right ? Why not just call it user for now ?

        Show
        Hemanth Yamijala added a comment - I hope users won't start expecting the group information will be there if we call it UGI. Currently, we are logging user everywhere, right ? Why not just call it user for now ?
        Hide
        Hemanth Yamijala added a comment -

        Uploading a patch again on Amar's behalf. This one incorporates my review comments raised earlier. It is still for earlier versions of Hadoop, and not for commit here.

        Show
        Hemanth Yamijala added a comment - Uploading a patch again on Amar's behalf. This one incorporates my review comments raised earlier. It is still for earlier versions of Hadoop, and not for commit here.
        Hide
        Amar Kamat added a comment -

        Attaching a patch for trunk that incorporates Hemanth's review comments. test-patch and ant-tests passed. Log4j changes are commented out but gives some insight as to how to configure mapreduce audit logs to write to a separate log file.

        Show
        Amar Kamat added a comment - Attaching a patch for trunk that incorporates Hemanth's review comments. test-patch and ant-tests passed. Log4j changes are commented out but gives some insight as to how to configure mapreduce audit logs to write to a separate log file.
        Hide
        Amar Kamat added a comment -

        Attaching a sample audit log file generated using this patch.

        Show
        Amar Kamat added a comment - Attaching a sample audit log file generated using this patch .
        Hide
        Luke Lu added a comment -

        Incorporated some feedback from Chris: use LOG.is*Enabled checks to minimize impact when log is disabled.

        Show
        Luke Lu added a comment - Incorporated some feedback from Chris: use LOG.is*Enabled checks to minimize impact when log is disabled.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12444163/mr-1543-trunk-v2.patch
        against trunk revision 942764.

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

        +1 tests included. The patch appears to include 2 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 warnings.

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

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

        -1 contrib tests. The patch failed contrib unit tests.

        Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h4.grid.sp2.yahoo.net/180/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h4.grid.sp2.yahoo.net/180/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h4.grid.sp2.yahoo.net/180/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h4.grid.sp2.yahoo.net/180/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/12444163/mr-1543-trunk-v2.patch against trunk revision 942764. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 2 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 warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. -1 contrib tests. The patch failed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h4.grid.sp2.yahoo.net/180/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h4.grid.sp2.yahoo.net/180/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h4.grid.sp2.yahoo.net/180/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h4.grid.sp2.yahoo.net/180/console This message is automatically generated.
        Hide
        Chris Douglas added a comment -

        Merged with trunk.

        Show
        Chris Douglas added a comment - Merged with trunk.
        Hide
        Chris Douglas added a comment -

        +1

        I committed this. Thanks, Amar and Luke!

        Show
        Chris Douglas added a comment - +1 I committed this. Thanks, Amar and Luke!

          People

          • Assignee:
            Luke Lu
            Reporter:
            Vinod Kumar Vavilapalli
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development