Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.5.0
    • Component/s: None
    • Labels:
      None
    1. TEZ-671.2.patch
      66 kB
      Hitesh Shah
    2. TEZ-671.3.patch
      90 kB
      Hitesh Shah
    3. TEZ-671.4.patch
      96 kB
      Hitesh Shah

      Issue Links

        Activity

        Hide
        Bikas Saha added a comment -

        Bulk close for jiras fixed in 0.5.0.

        Show
        Bikas Saha added a comment - Bulk close for jiras fixed in 0.5.0.
        Hide
        Johannes Zillmann added a comment -

        Created TEZ-1458 for that!

        Show
        Johannes Zillmann added a comment - Created TEZ-1458 for that!
        Hide
        Johannes Zillmann added a comment -

        Hey guys, i think that broke compilation with hadoop-2.2!
        Groups#parseStaticMapping() uses now StringUtils.getStringCollection(str, delim) which has been introduced as part of HADOOP-10142 in hadoop-2.3.
        Getting:

        2014-08-19 09:29:07,325 INFO [main] org.apache.tez.dag.app.DAGAppMaster: Created DAGAppMaster for application appattempt_1408433339532_0001_000001
        2014-08-19 09:29:07,798 WARN [main] org.apache.hadoop.util.NativeCodeLoader: Unable to load native-hadoop library for your platform... using builtin-java classes where applicable
        2014-08-19 09:29:08,046 FATAL [main] org.apache.tez.dag.app.DAGAppMaster: Error starting DAGAppMaster
        java.lang.NoSuchMethodError: org.apache.hadoop.util.StringUtils.getStringCollection(Ljava/lang/String;Ljava/lang/String;)Ljava/util/Collection;
        	at org.apache.tez.common.security.Groups.parseStaticMapping(Groups.java:102)
        	at org.apache.tez.common.security.Groups.<init>(Groups.java:78)
        	at org.apache.tez.dag.app.DAGAppMaster.serviceInit(DAGAppMaster.java:313)
        	at org.apache.hadoop.service.AbstractService.init(AbstractService.java:163)
        	at org.apache.tez.dag.app.DAGAppMaster$5.run(DAGAppMaster.java:1914)
        	at java.security.AccessController.doPrivileged(Native Method)
        	at javax.security.auth.Subject.doAs(Subject.java:394)
        	at org.apache.hadoop.security.UserGroupInformation.doAs(UserGroupInformation.java:1491)
        	at org.apache.tez.dag.app.DAGAppMaster.initAndStartAppMaster(DAGAppMaster.java:1911)
        	at org.apache.tez.dag.app.DAGAppMaster.main(DAGAppMaster.java:1727)
        2014-08-19 09:29:08,051 INFO [Thread-2] org.apache.tez.dag.app.DAGAppMaster: DAGAppMasterShutdownHook invoked
        
        
        Show
        Johannes Zillmann added a comment - Hey guys, i think that broke compilation with hadoop-2.2! Groups#parseStaticMapping() uses now StringUtils.getStringCollection(str, delim) which has been introduced as part of HADOOP-10142 in hadoop-2.3. Getting: 2014-08-19 09:29:07,325 INFO [main] org.apache.tez.dag.app.DAGAppMaster: Created DAGAppMaster for application appattempt_1408433339532_0001_000001 2014-08-19 09:29:07,798 WARN [main] org.apache.hadoop.util.NativeCodeLoader: Unable to load native -hadoop library for your platform... using builtin-java classes where applicable 2014-08-19 09:29:08,046 FATAL [main] org.apache.tez.dag.app.DAGAppMaster: Error starting DAGAppMaster java.lang.NoSuchMethodError: org.apache.hadoop.util.StringUtils.getStringCollection(Ljava/lang/ String ;Ljava/lang/ String ;)Ljava/util/Collection; at org.apache.tez.common.security.Groups.parseStaticMapping(Groups.java:102) at org.apache.tez.common.security.Groups.<init>(Groups.java:78) at org.apache.tez.dag.app.DAGAppMaster.serviceInit(DAGAppMaster.java:313) at org.apache.hadoop.service.AbstractService.init(AbstractService.java:163) at org.apache.tez.dag.app.DAGAppMaster$5.run(DAGAppMaster.java:1914) at java.security.AccessController.doPrivileged(Native Method) at javax.security.auth.Subject.doAs(Subject.java:394) at org.apache.hadoop.security.UserGroupInformation.doAs(UserGroupInformation.java:1491) at org.apache.tez.dag.app.DAGAppMaster.initAndStartAppMaster(DAGAppMaster.java:1911) at org.apache.tez.dag.app.DAGAppMaster.main(DAGAppMaster.java:1727) 2014-08-19 09:29:08,051 INFO [ Thread -2] org.apache.tez.dag.app.DAGAppMaster: DAGAppMasterShutdownHook invoked
        Hide
        Hitesh Shah added a comment -

        Committed to master. Thanks for the review Siddharth Seth

        Show
        Hitesh Shah added a comment - Committed to master. Thanks for the review Siddharth Seth
        Hide
        Siddharth Seth added a comment -

        +1. Looks good.

        Show
        Siddharth Seth added a comment - +1. Looks good.
        Hide
        Hitesh Shah added a comment -

        Siddharth Seth review please

        Show
        Hitesh Shah added a comment - Siddharth Seth review please
        Hide
        Hitesh Shah added a comment -

        Some comments addressed:

        • conf null check removed
        • add helper constructor for DAGAccessControls
        • marked DAG ACL config properties as private

        Not addressed:

        • no simple choice to define allow all as we have both view and modify acls. Would need to provide APIs such as setAllowAllForView, etc so for now not added.
        • Allow none is done by default if access controls are not specified but acls are enabled.
        Show
        Hitesh Shah added a comment - Some comments addressed: conf null check removed add helper constructor for DAGAccessControls marked DAG ACL config properties as private Not addressed: no simple choice to define allow all as we have both view and modify acls. Would need to provide APIs such as setAllowAllForView, etc so for now not added. Allow none is done by default if access controls are not specified but acls are enabled.
        Hide
        ASF GitHub Bot added a comment -

        Github user hiteshs commented on the pull request:

        https://github.com/apache/tez/pull/2#issuecomment-52241445

        Closing this out and using std patch on apache jira instead.

        Show
        ASF GitHub Bot added a comment - Github user hiteshs commented on the pull request: https://github.com/apache/tez/pull/2#issuecomment-52241445 Closing this out and using std patch on apache jira instead.
        Hide
        ASF GitHub Bot added a comment -

        Github user hiteshs closed the pull request at:

        https://github.com/apache/tez/pull/2

        Show
        ASF GitHub Bot added a comment - Github user hiteshs closed the pull request at: https://github.com/apache/tez/pull/2
        Hide
        Siddharth Seth added a comment -

        Mostly looks good.

        • Why is the configuration==null check required in ACLManager ?
          +    } else {
          +      aclsEnabled = true;
          +    }
          

        TEZ_DAG_VIEW_ACLS, TEZ_DAG_MODIFY_ACLS are private. Do we want users setting these via tez-site.xml ? If not, ideally, these could be sent via their own proto field, and then just sent into the ACLManager/Parser as strings. This is OK, but we should mark them private.

        DAGAccessControls. I'm guessing this is designed to be used as DAG.setAccessControls(new DAGAccessCOntrols().setUsersWithViewAcls(conf.getStrings(conf_property_name) ?
        Would be helpful to have some defaults like - ALLLOW_ALL, ALLOW_NONE. Also, a constructor which just accepts the standard MR ACLs may be useful, since we support that format for the AM ACLs in any case.

        Show
        Siddharth Seth added a comment - Mostly looks good. Why is the configuration==null check required in ACLManager ? + } else { + aclsEnabled = true ; + } TEZ_DAG_VIEW_ACLS, TEZ_DAG_MODIFY_ACLS are private. Do we want users setting these via tez-site.xml ? If not, ideally, these could be sent via their own proto field, and then just sent into the ACLManager/Parser as strings. This is OK, but we should mark them private. DAGAccessControls. I'm guessing this is designed to be used as DAG.setAccessControls(new DAGAccessCOntrols().setUsersWithViewAcls(conf.getStrings(conf_property_name) ? Would be helpful to have some defaults like - ALLLOW_ALL, ALLOW_NONE. Also, a constructor which just accepts the standard MR ACLs may be useful, since we support that format for the AM ACLs in any case.
        Hide
        Hitesh Shah added a comment -

        API introductions involved. Bikas Saha Siddharth Seth please review.

        Show
        Hitesh Shah added a comment - API introductions involved. Bikas Saha Siddharth Seth please review.
        Hide
        Hitesh Shah added a comment -

        Comments addressed.

        Show
        Hitesh Shah added a comment - Comments addressed.
        Hide
        Hitesh Shah added a comment -

        File HADOOP-10951 for Groups in hadoop to be made public.

        Show
        Hitesh Shah added a comment - File HADOOP-10951 for Groups in hadoop to be made public.
        Hide
        Hitesh Shah added a comment -

        getACLManager should make use of getDAG - instead of replicating it

        • being addressed.

        bq, Oleg had a comment about getting the ugi being repeated everywhere

        • already addressed.

        Are any changes required on the creation of the ApplicationSubmissionContext to set the AM ACLs for YARN

        • being addressed.

        How to DAGs supply ACLs. Do we need a method on the DAG API for this. In, which case, the DAG specific config parameters are likely not required.

        • I was under the assumption that we had support for a separate additional dag conf as a pass through for dag specific params. This seems no longer the case so I guess a new API is needed.

        Anything needed in terms of ATS, or will that be a separate patch.

        • follow-up once timeline acls supports comes into hadoop. Post 2.6 i guess.

        Ideally, when using the SimpleHistoryLoggingService - we could use HDFS ACLs to give access specific AM logs, but this would be a follow up.

        • Will file a follow-up for this too.

        In terms of Group, AclManager etc

        • Groups was the only one I would have liked to reuse. It still has features being added and is marked private so I ended up creating a copy.
        • As for AclManager and all the other common functionality, given our need for 2 level checks, I think a separate impl might be better for us.
        Show
        Hitesh Shah added a comment - getACLManager should make use of getDAG - instead of replicating it being addressed. bq, Oleg had a comment about getting the ugi being repeated everywhere already addressed. Are any changes required on the creation of the ApplicationSubmissionContext to set the AM ACLs for YARN being addressed. How to DAGs supply ACLs. Do we need a method on the DAG API for this. In, which case, the DAG specific config parameters are likely not required. I was under the assumption that we had support for a separate additional dag conf as a pass through for dag specific params. This seems no longer the case so I guess a new API is needed. Anything needed in terms of ATS, or will that be a separate patch. follow-up once timeline acls supports comes into hadoop. Post 2.6 i guess. Ideally, when using the SimpleHistoryLoggingService - we could use HDFS ACLs to give access specific AM logs, but this would be a follow up. Will file a follow-up for this too. In terms of Group, AclManager etc Groups was the only one I would have liked to reuse. It still has features being added and is marked private so I ended up creating a copy. As for AclManager and all the other common functionality, given our need for 2 level checks, I think a separate impl might be better for us.
        Hide
        Siddharth Seth added a comment -

        Comments on the patch:

        • getACLManager should make use of getDAG - instead of replicating it
        • Oleg had a comment about getting the ugi being repeated everywhere
        • Are any changes required on the creation of the ApplicationSubmissionContext to set the AM ACLs for YARN
        • How to DAGs supply ACLs. Do we need a method on the DAG API for this. In, which case, the DAG specific config parameters are likely not required.
        • Anything needed in terms of ATS, or will that be a separate patch.
        • Ideally, when using the SimpleHistoryLoggingService - we could use HDFS ACLs to give access specific AM logs, but this would be a follow up. For now, this will be created using the AM credentials correct ?, which should be accessible to the app submitter, but may not be accessible to individual DAGs
        • In terms of Group, AclManager etc - a lot of this is from Hadoop as LimitedPrivate("mapreduce"). I'm guessing that's in Tez as well to avoid potential changes in Hadoop. Would be much better if we could just depend on that code instead of having to create a copy.
        Show
        Siddharth Seth added a comment - Comments on the patch: getACLManager should make use of getDAG - instead of replicating it Oleg had a comment about getting the ugi being repeated everywhere Are any changes required on the creation of the ApplicationSubmissionContext to set the AM ACLs for YARN How to DAGs supply ACLs. Do we need a method on the DAG API for this. In, which case, the DAG specific config parameters are likely not required. Anything needed in terms of ATS, or will that be a separate patch. Ideally, when using the SimpleHistoryLoggingService - we could use HDFS ACLs to give access specific AM logs, but this would be a follow up. For now, this will be created using the AM credentials correct ?, which should be accessible to the app submitter, but may not be accessible to individual DAGs In terms of Group, AclManager etc - a lot of this is from Hadoop as LimitedPrivate("mapreduce"). I'm guessing that's in Tez as well to avoid potential changes in Hadoop. Would be much better if we could just depend on that code instead of having to create a copy.
        Hide
        ASF GitHub Bot added a comment -

        GitHub user hiteshs opened a pull request:

        https://github.com/apache/tez/pull/2

        TEZ-671. Support View/Modify ACLs for DAGs.

        Similar to patch 2. Additional changes include header additions for swimlane related python scripts.

        You can merge this pull request into a Git repository by running:

        $ git pull https://github.com/hiteshs/tez TEZ-671

        Alternatively you can review and apply these changes as the patch at:

        https://github.com/apache/tez/pull/2.patch

        To close this pull request, make a commit to your master/trunk branch
        with (at least) the following in the commit message:

        This closes #2


        commit 2467b2fb81e89c7cfc897e369d6b6beeb5b3b222
        Author: Hitesh Shah <hitesh@apache.org>
        Date: 2014-08-07T18:49:15Z

        TEZ-671. Support View/Modify ACLs for DAGs.


        Show
        ASF GitHub Bot added a comment - GitHub user hiteshs opened a pull request: https://github.com/apache/tez/pull/2 TEZ-671 . Support View/Modify ACLs for DAGs. Similar to patch 2. Additional changes include header additions for swimlane related python scripts. You can merge this pull request into a Git repository by running: $ git pull https://github.com/hiteshs/tez TEZ-671 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/tez/pull/2.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #2 commit 2467b2fb81e89c7cfc897e369d6b6beeb5b3b222 Author: Hitesh Shah <hitesh@apache.org> Date: 2014-08-07T18:49:15Z TEZ-671 . Support View/Modify ACLs for DAGs.
        Hide
        Hitesh Shah added a comment -

        Updated doc attached. Siddharth Seth Jonathan Eagles please review.

        Show
        Hitesh Shah added a comment - Updated doc attached. Siddharth Seth Jonathan Eagles please review.
        Hide
        Hitesh Shah added a comment -

        Nevermind - forgot to add more docs. Will upload a new patch with better docs shortly.

        Show
        Hitesh Shah added a comment - Nevermind - forgot to add more docs. Will upload a new patch with better docs shortly.
        Hide
        Hitesh Shah added a comment -

        Siddharth Seth Jonathan Eagles Mind taking a look?

        Show
        Hitesh Shah added a comment - Siddharth Seth Jonathan Eagles Mind taking a look?
        Hide
        Siddharth Seth added a comment -

        With session mode, and support for credentials per DAG - I think it makes sense to support DAG level ACLs. I'm not sure how we can do this for APIs like killDAG (if the AM is lost), since YARN does not support changing ACLs for a running application.
        For a user like Hive - I'm guessing App ACLs will be setup to be based on the Hive super-user - primarily for access to the RM / stop application etc. For running DAGs (and access via the AppTimelineServer), I'm assuming the per DAG ACLs will be sufficient.
        This is all assuming session mode will be used to submit DAGs on behalf of multiple users, which I believe is the case. Gunther Hagleitner ?

        Show
        Siddharth Seth added a comment - With session mode, and support for credentials per DAG - I think it makes sense to support DAG level ACLs. I'm not sure how we can do this for APIs like killDAG (if the AM is lost), since YARN does not support changing ACLs for a running application. For a user like Hive - I'm guessing App ACLs will be setup to be based on the Hive super-user - primarily for access to the RM / stop application etc. For running DAGs (and access via the AppTimelineServer), I'm assuming the per DAG ACLs will be sufficient. This is all assuming session mode will be used to submit DAGs on behalf of multiple users, which I believe is the case. Gunther Hagleitner ?
        Hide
        Hitesh Shah added a comment -

        Siddharth Seth Do you see a need to have both AM level ACLs as well as DAG level ACLs?

        Show
        Hitesh Shah added a comment - Siddharth Seth Do you see a need to have both AM level ACLs as well as DAG level ACLs?

          People

          • Assignee:
            Hitesh Shah
            Reporter:
            Siddharth Seth
          • Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development