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

For secure environments, the Map/Reduce debug script must be run as the user.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 0.21.0
    • Fix Version/s: 0.21.0
    • Component/s: security, tasktracker
    • Labels:
      None

      Description

      The Taskcontroller model allows admins to set up a cluster configuration that runs tasks as users. The debug script feature of Map/Reduce provided by the configuration options: mapred.map.task.debug.script and mapred.reduce.task.debug.script need to be run as the user as well in such environments, rather than as the tasktracker user.

      1. 915.1.patch
        26 kB
        Devaraj Das
      2. 915.2.patch
        38 kB
        Devaraj Das
      3. 915.5.patch
        46 kB
        Devaraj Das
      4. 915.patch
        17 kB
        Devaraj Das
      5. 915-4.patch
        36 kB
        Devaraj Das

        Issue Links

          Activity

          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk-Commit #122 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Mapreduce-trunk-Commit/122/)
          . The debug scripts are run as the job user. Contributed by Devaraj Das.

          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk-Commit #122 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Mapreduce-trunk-Commit/122/ ) . The debug scripts are run as the job user. Contributed by Devaraj Das.
          Hide
          Devaraj Das added a comment -

          I just committed this.

          Show
          Devaraj Das added a comment - I just committed this.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12425191/915.5.patch
          against trunk revision 881095.

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

          +1 tests included. The patch appears to include 18 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 passed contrib unit tests.

          Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/248/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/248/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/248/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/248/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/12425191/915.5.patch against trunk revision 881095. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 18 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 passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/248/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/248/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/248/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h6.grid.sp2.yahoo.net/248/console This message is automatically generated.
          Hide
          Vinod Kumar Vavilapalli added a comment -

          Patch looks good. +1 from my side. Hoping Hudson blesses it, let's wait and see.

          Show
          Vinod Kumar Vavilapalli added a comment - Patch looks good. +1 from my side. Hoping Hudson blesses it, let's wait and see.
          Hide
          Devaraj Das added a comment -

          The deltas in this patch are:
          1) LOG.info added in LinuxTaskController.java for the commands that's being written to taskjvm.sh
          2) Logs added in run_process_as_user indicating the context of invocation
          3) The MiniMRCluster in testDebugScript is configured with TT_SLEEP_TIME_BEFORE_SIG_KILL of 0
          4) The typo fix in verifyDebugScriptOutput to do with using '&' instead of '&&'

          Show
          Devaraj Das added a comment - The deltas in this patch are: 1) LOG.info added in LinuxTaskController.java for the commands that's being written to taskjvm.sh 2) Logs added in run_process_as_user indicating the context of invocation 3) The MiniMRCluster in testDebugScript is configured with TT_SLEEP_TIME_BEFORE_SIG_KILL of 0 4) The typo fix in verifyDebugScriptOutput to do with using '&' instead of '&&'
          Hide
          Vinod Kumar Vavilapalli added a comment -

          I can log the command lines that is written to taskjvm.sh. For the purpose of debugging, that seems to make more sense than relying on the taskjvm.sh's contents.. I will upload a patch soon.

          I guess the advantage of using the same file is, well, less files to manage, and perhaps, a bit of code-reuse.

          I can't think of more uses for taskjvm.sh besides debugging. So +1 for the idea of logging the commands to TT's log and use the same file for debug-script command too.

          Show
          Vinod Kumar Vavilapalli added a comment - I can log the command lines that is written to taskjvm.sh. For the purpose of debugging, that seems to make more sense than relying on the taskjvm.sh's contents.. I will upload a patch soon. I guess the advantage of using the same file is, well, less files to manage, and perhaps, a bit of code-reuse. I can't think of more uses for taskjvm.sh besides debugging. So +1 for the idea of logging the commands to TT's log and use the same file for debug-script command too.
          Hide
          Devaraj Das added a comment -

          Oops sorry. I forgot to add the Test* files before i created the last patch.
          I can log the command lines that is written to taskjvm.sh. For the purpose of debugging, that seems to make more sense than relying on the taskjvm.sh's contents.. I will upload a patch soon.

          Show
          Devaraj Das added a comment - Oops sorry. I forgot to add the Test* files before i created the last patch. I can log the command lines that is written to taskjvm.sh. For the purpose of debugging, that seems to make more sense than relying on the taskjvm.sh's contents.. I will upload a patch soon.
          Hide
          Vinod Kumar Vavilapalli added a comment -
          • The newly added tests TestDebugScript and TestDebugScriptWithLinuxTaskController are missing from the latest patch. Please also do not forget to include the minor comments I've put up earlier w.r.t the first of these.
          • Please include a info log statement in src/c++/task-controller/task-controller.c, in function run_process_as_user() as to which of the two commands it is actually running. Either in this function, or the callers.

          The one thing which I and Owen felt should be left as is is to have the same file for writing the commands for launching tasks and debug scripts.

          The taskjvm.sh file helps a lot during debugging of edge cases. If debug-script overwrites it, we lose this small but useful help. Logging the contents of taskjvm.sh to TT log file before the taskjvm.sh is actually written may help in these situations. I'll ask Hemanth's opinion about this too.

          Show
          Vinod Kumar Vavilapalli added a comment - The newly added tests TestDebugScript and TestDebugScriptWithLinuxTaskController are missing from the latest patch. Please also do not forget to include the minor comments I've put up earlier w.r.t the first of these. Please include a info log statement in src/c++/task-controller/task-controller.c , in function run_process_as_user() as to which of the two commands it is actually running. Either in this function, or the callers. The one thing which I and Owen felt should be left as is is to have the same file for writing the commands for launching tasks and debug scripts. The taskjvm.sh file helps a lot during debugging of edge cases. If debug-script overwrites it, we lose this small but useful help. Logging the contents of taskjvm.sh to TT log file before the taskjvm.sh is actually written may help in these situations. I'll ask Hemanth's opinion about this too.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12424820/915-4.patch
          against trunk revision 835237.

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

          +1 tests included. The patch appears to include 12 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 passed contrib unit tests.

          Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/138/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/138/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/138/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/138/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/12424820/915-4.patch against trunk revision 835237. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 12 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 passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/138/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/138/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/138/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Mapreduce-Patch-h3.grid.sp2.yahoo.net/138/console This message is automatically generated.
          Hide
          Devaraj Das added a comment -

          Passing through hudson

          Show
          Devaraj Das added a comment - Passing through hudson
          Hide
          Devaraj Das added a comment -

          Thanks for the review, Vinod! Attached is the patch with the suggested changes. The one thing which I and Owen felt should be left as is is to have the same file for writing the commands for launching tasks and debug scripts.

          Show
          Devaraj Das added a comment - Thanks for the review, Vinod! Attached is the patch with the suggested changes. The one thing which I and Owen felt should be left as is is to have the same file for writing the commands for launching tasks and debug scripts.
          Hide
          Vinod Kumar Vavilapalli added a comment -

          Did a review of the patch. It looks mostly good. Also ran the tests, and verified that the debug scrip is indeed run as user by putting in hard-coded sleeps.

          Some code-l comments, mostly minor:

          • Rename DebugControllerContext to DebugScriptContext?
          • Rename LAUNCH_DEBUG_SCRIPT to RUN_DEBUG_SCRIPT everywhere?

          TaskController.java, DefaultTaskController.java

          • Rename launchDebugScript() to runDebugScript(). We are actually running the script, i.e. waiting for it's finish.
          • Remove the internal-usage javadoc and tag it with @InterfaceAudience.Private?

          TaskTracker.java

          • +2372 Code related to adding debug out to diagnostics isn't needed if TaskController.launchDebugScript() fails with an exception.
          • A simple log msg in tasktracker's log when running debug script for a task?
          • Can you also refactor the whole debug-script-business into a single method? It is a good enough amount of isolated code that warrants its own home, me
            thinks.

          LinuxTaskController.java

          • I think that we should write the debug command to a new file other than taskjvm.sh? What do others think? This would also mean changes to run_debug_script_as_user() in task-controller.c
          • In any case, I feel we should try to reuse code across run_task_as_user() and run_debug_script_as_user().
          • Much of the code in launchDebugScript() can be replace by a call to LinuxTaskController.runCommand()

          TaskLog.java

          • Make readTaskLog() package-private? And mark it as test-only method? Nowhere else is it used now. In fact, why not move it to the test-case that uses it?

          TestDebugScript.java

          • Set TTConfig.TT_SLEEP_TIME_BEFORE_SIG_KILL to zero and pass it at MiniMR's cluster-startup? Saves five seconds on the test-time.
          • Typo at +155? "&&" instead of "&"?
          Show
          Vinod Kumar Vavilapalli added a comment - Did a review of the patch. It looks mostly good. Also ran the tests, and verified that the debug scrip is indeed run as user by putting in hard-coded sleeps. Some code-l comments, mostly minor: Rename DebugControllerContext to DebugScriptContext? Rename LAUNCH_DEBUG_SCRIPT to RUN_DEBUG_SCRIPT everywhere? TaskController.java, DefaultTaskController.java Rename launchDebugScript() to runDebugScript(). We are actually running the script, i.e. waiting for it's finish. Remove the internal-usage javadoc and tag it with @InterfaceAudience.Private? TaskTracker.java +2372 Code related to adding debug out to diagnostics isn't needed if TaskController.launchDebugScript() fails with an exception. A simple log msg in tasktracker's log when running debug script for a task? Can you also refactor the whole debug-script-business into a single method? It is a good enough amount of isolated code that warrants its own home, me thinks. LinuxTaskController.java I think that we should write the debug command to a new file other than taskjvm.sh? What do others think? This would also mean changes to run_debug_script_as_user() in task-controller.c In any case, I feel we should try to reuse code across run_task_as_user() and run_debug_script_as_user(). Much of the code in launchDebugScript() can be replace by a call to LinuxTaskController.runCommand() TaskLog.java Make readTaskLog() package-private? And mark it as test-only method? Nowhere else is it used now. In fact, why not move it to the test-case that uses it? TestDebugScript.java Set TTConfig.TT_SLEEP_TIME_BEFORE_SIG_KILL to zero and pass it at MiniMR's cluster-startup? Saves five seconds on the test-time. Typo at +155? "&&" instead of "&"?
          Hide
          Devaraj Das added a comment -

          Ok this patch adds the permission/ownership checks of the debugout file. I also removed TestMiniMRMapRedDebugScript.java since the new tests cover the test there and more..

          Show
          Devaraj Das added a comment - Ok this patch adds the permission/ownership checks of the debugout file. I also removed TestMiniMRMapRedDebugScript.java since the new tests cover the test there and more..
          Hide
          Vinod Kumar Vavilapalli added a comment -

          We should have a test to verify the ownership of the debug output file as well as its file permissions. That will kind of assert that the debug script is indeed run as the user and not the TT. We can add/modify tests in TestTaskTrackerLocalization for this.

          Show
          Vinod Kumar Vavilapalli added a comment - We should have a test to verify the ownership of the debug output file as well as its file permissions. That will kind of assert that the debug script is indeed run as the user and not the TT. We can add/modify tests in TestTaskTrackerLocalization for this.
          Hide
          Devaraj Das added a comment -

          Attaching a patch with testcases (thanks Sreekanth for the testcases).

          Show
          Devaraj Das added a comment - Attaching a patch with testcases (thanks Sreekanth for the testcases).
          Hide
          Sreekanth Ramakrishnan added a comment -

          Sorry raising the fuss, talked with Devaraj and Hemanth offline, we can merge MAPREDUCE-913 after MAPREDUCE-915 is commited and merge effort would be lesser. Changing the issue link.

          Show
          Sreekanth Ramakrishnan added a comment - Sorry raising the fuss, talked with Devaraj and Hemanth offline, we can merge MAPREDUCE-913 after MAPREDUCE-915 is commited and merge effort would be lesser. Changing the issue link.
          Hide
          Sreekanth Ramakrishnan added a comment -

          Currently the debug script relies on the proper launch of the task JVM in order to run, and makes assumption as the log folder and log index is created properly before debug script is run. We should run it only if the task JVM is launched otherwise the code path which launches debug script can cause issue as described in MAPREDUCE-913.

          Show
          Sreekanth Ramakrishnan added a comment - Currently the debug script relies on the proper launch of the task JVM in order to run, and makes assumption as the log folder and log index is created properly before debug script is run. We should run it only if the task JVM is launched otherwise the code path which launches debug script can cause issue as described in MAPREDUCE-913 .
          Hide
          Sreekanth Ramakrishnan added a comment -

          Marking MAPREDUCE-913 as blocker for this issue.

          Show
          Sreekanth Ramakrishnan added a comment - Marking MAPREDUCE-913 as blocker for this issue.
          Hide
          Devaraj Das added a comment -

          Attaching a patch for review. Testing in progress.

          Show
          Devaraj Das added a comment - Attaching a patch for review. Testing in progress.
          Hide
          Vinod Kumar Vavilapalli added a comment -

          This has another deeper implication. In particular, for the launched debug script processes, we will have to do the memory management that we do for the regular java tasks. Argh.

          Created MAPREDUCE-1087 for the same. MAPREDUCE-1087 may not be a blocker for 0.21. Thoughts?

          Show
          Vinod Kumar Vavilapalli added a comment - This has another deeper implication. In particular, for the launched debug script processes, we will have to do the memory management that we do for the regular java tasks. Argh. Created MAPREDUCE-1087 for the same. MAPREDUCE-1087 may not be a blocker for 0.21. Thoughts?
          Hide
          Devaraj Das added a comment -

          This has another deeper implication. In particular, for the launched debug script processes, we will have to do the memory management that we do for the regular java tasks. Argh.

          Show
          Devaraj Das added a comment - This has another deeper implication. In particular, for the launched debug script processes, we will have to do the memory management that we do for the regular java tasks. Argh.

            People

            • Assignee:
              Devaraj Das
              Reporter:
              Hemanth Yamijala
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development