Hadoop Common
  1. Hadoop Common
  2. HADOOP-6566

Hadoop daemons should not start up if the ownership/permissions on the directories used at runtime are misconfigured

    Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.22.0
    • Fix Version/s: 0.21.0
    • Component/s: security
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      The Hadoop daemons (like datanode, namenode) should refuse to start up if the ownership/permissions on directories they use at runtime are misconfigured or they are not as expected. For example, the local directory where the filesystem image is stored should be owned by the user running the namenode process and should be only readable by that user. We can provide this feature in common and HDFS and MapReduce can use the same.

      1. HADOOP-6566_yhadoop20.patch
        13 kB
        Arun C Murthy
      2. HADOOP-6566_yhadoop20.patch
        11 kB
        Arun C Murthy
      3. HADOOP-6566_yhadoop20.patch
        11 kB
        Arun C Murthy
      4. HADOOP-6566_yhadoop20.patch
        11 kB
        Arun C Murthy
      5. hadoop-6566-trunk-v1.patch
        6 kB
        Luke Lu
      6. hadoop-6566-trunk-v2.patch
        6 kB
        Luke Lu
      7. hadoop-6566-trunk-v3.patch
        14 kB
        Luke Lu
      8. hadoop-6566-trunk-v4.patch
        14 kB
        Luke Lu
      9. hadoop-6566-y20s-d1.patch
        3 kB
        Luke Lu

        Issue Links

          Activity

          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk #288 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Common-trunk/288/)
          . Add methods supporting, enforcing narrower permissions on local daemon directories.
          Contributed by Arun Murthy and Luke Lu

          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk #288 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Common-trunk/288/ ) . Add methods supporting, enforcing narrower permissions on local daemon directories. Contributed by Arun Murthy and Luke Lu
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk-Commit #211 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Common-trunk-Commit/211/)
          . Add methods supporting, enforcing narrower permissions on local daemon directories.
          Contributed by Arun Murthy and Luke Lu

          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #211 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Common-trunk-Commit/211/ ) . Add methods supporting, enforcing narrower permissions on local daemon directories. Contributed by Arun Murthy and Luke Lu
          Hide
          Chris Douglas added a comment -

          +1

          I committed this. Thanks Arun and Luke!

          (compliments on the MockitoMaker class, also)

          Show
          Chris Douglas added a comment - +1 I committed this. Thanks Arun and Luke! (compliments on the MockitoMaker class, also)
          Hide
          Konstantin Boudnik added a comment -

          +1 on the patch. Looks good.

          Show
          Konstantin Boudnik added a comment - +1 on the patch. Looks good.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12439073/hadoop-6566-trunk-v4.patch
          against trunk revision 923619.

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

          +1 tests included. The patch appears to include 4 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/Hadoop-Patch-h4.grid.sp2.yahoo.net/421/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/421/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/421/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/421/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/12439073/hadoop-6566-trunk-v4.patch against trunk revision 923619. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 4 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/Hadoop-Patch-h4.grid.sp2.yahoo.net/421/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/421/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/421/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/421/console This message is automatically generated.
          Hide
          Luke Lu added a comment -

          Corrections for the last comment: asterisks and HDFS-997

          Show
          Luke Lu added a comment - Corrections for the last comment: asterisks and HDFS-997
          Hide
          Luke Lu added a comment -

          The delta (-d1.patch) patch is for yahoo 0.20.1xx branch, which already has the previous (-yhadoop20.patch) committed. The trunk patch needs to be combined with the patch for HDFS-977 (the datanode half of the change and tests.)

          Show
          Luke Lu added a comment - The delta ( -d1.patch) patch is for yahoo 0.20.1xx branch, which already has the previous ( -yhadoop20.patch) committed. The trunk patch needs to be combined with the patch for HDFS-977 (the datanode half of the change and tests.)
          Hide
          Konstantin Boudnik added a comment -

          Great! Thanks for addressing this. And the last one, I suppose: can these two different patches be joined together to make the scope of modification clearer?

          Show
          Konstantin Boudnik added a comment - Great! Thanks for addressing this. And the last one, I suppose: can these two different patches be joined together to make the scope of modification clearer?
          Hide
          Luke Lu added a comment -

          The patch should now set the permission of the dirs, if they don't match expectation, so tests should not fail more than before.

          Show
          Luke Lu added a comment - The patch should now set the permission of the dirs, if they don't match expectation, so tests should not fail more than before.
          Hide
          Konstantin Boudnik added a comment -

          Patch seems to be good and the little Mockito extension is neat. Although, it adds up a different way of wotking with mocks, which might be confusing somewhat.

          However, I still can see failing test if a user umask settings are different from expected. I again suggest to make changes in MiniDFSCluster to make sure that it creates service directories with correct permissions. Otherwise, this is the change which introduce an implicit environment assumption i.e. bad idea!

          Show
          Konstantin Boudnik added a comment - Patch seems to be good and the little Mockito extension is neat. Although, it adds up a different way of wotking with mocks, which might be confusing somewhat. However, I still can see failing test if a user umask settings are different from expected. I again suggest to make changes in MiniDFSCluster to make sure that it creates service directories with correct permissions. Otherwise, this is the change which introduce an implicit environment assumption i.e. bad idea!
          Hide
          Luke Lu added a comment -

          Thanks for the review, Dick.

          I'd like to point out that the incantation you mentioned is not the only way use the library. You can also just use:

          import org.apache.hadoop.test.MockitoMaker;
          

          and use MockitoMaker.make and MockitoMaker.stub explicitly in case of any name conflicts which is extremely rare in writing test cases. I'd rather keep these glue names short, so that tests are succinct and fun to write in common cases.

          Show
          Luke Lu added a comment - Thanks for the review, Dick. I'd like to point out that the incantation you mentioned is not the only way use the library. You can also just use: import org.apache.hadoop.test.MockitoMaker; and use MockitoMaker.make and MockitoMaker.stub explicitly in case of any name conflicts which is extremely rare in writing test cases. I'd rather keep these glue names short, so that tests are succinct and fun to write in common cases.
          Hide
          Dick King added a comment -

          I reviewed the patch. It does what it's supposed to do.

          However...

          MockitoMaker is cute enough, but given that its users are expected to incant and do incant

          import static org.apache.hadoop.test.MockitoMaker.*;

          I'd rather see names other than make and stub for its static methods.

          -dk

          Show
          Dick King added a comment - I reviewed the patch. It does what it's supposed to do. However... MockitoMaker is cute enough, but given that its users are expected to incant and do incant import static org.apache.hadoop.test.MockitoMaker.*; I'd rather see names other than make and stub for its static methods. -dk
          Hide
          Luke Lu added a comment -

          Added unit tests with a splash of mockito.

          Show
          Luke Lu added a comment - Added unit tests with a splash of mockito.
          Hide
          Konstantin Boudnik added a comment -

          I still don't see my comment above addressed in any way. This new check implicitly relies on a certain environment settings and will fail if they aren't set properly.

          This requirement either:

          • has to be documented
          • a special environment setting routing in MiniDFSCluster needs to be implemented
          Show
          Konstantin Boudnik added a comment - I still don't see my comment above addressed in any way. This new check implicitly relies on a certain environment settings and will fail if they aren't set properly. This requirement either: has to be documented a special environment setting routing in MiniDFSCluster needs to be implemented
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12438478/hadoop-6566-trunk-v2.patch
          against trunk revision 921577.

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

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no new tests are needed for this patch.
          Also please list what manual steps were performed to verify this patch.

          +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/Hadoop-Patch-h4.grid.sp2.yahoo.net/415/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/415/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/415/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/415/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/12438478/hadoop-6566-trunk-v2.patch against trunk revision 921577. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +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/Hadoop-Patch-h4.grid.sp2.yahoo.net/415/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/415/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/415/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/415/console This message is automatically generated.
          Hide
          Luke Lu added a comment -

          Removed a deprecated usage to appease hudson.

          Show
          Luke Lu added a comment - Removed a deprecated usage to appease hudson.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12438346/hadoop-6566-trunk-v1.patch
          against trunk revision 918880.

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

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no new tests are needed for this patch.
          Also please list what manual steps were performed to verify this patch.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          -1 javac. The applied patch generated 1025 javac compiler warnings (more than the trunk's current 1024 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/Hadoop-Patch-h4.grid.sp2.yahoo.net/413/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/413/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/413/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/413/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/12438346/hadoop-6566-trunk-v1.patch against trunk revision 918880. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javadoc. The javadoc tool did not generate any warning messages. -1 javac. The applied patch generated 1025 javac compiler warnings (more than the trunk's current 1024 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/Hadoop-Patch-h4.grid.sp2.yahoo.net/413/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/413/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/413/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/413/console This message is automatically generated.
          Hide
          Luke Lu added a comment -

          Ported to trunk.

          Show
          Luke Lu added a comment - Ported to trunk.
          Hide
          Konstantin Boudnik added a comment -

          Marking this as 'incompatible' for it might be affected by user env. settings and fail the tests.

          Show
          Konstantin Boudnik added a comment - Marking this as 'incompatible' for it might be affected by user env. settings and fail the tests.
          Hide
          Konstantin Boudnik added a comment -

          A nasty side effect of this modification is that now a user's environment settings affect the result of test test. Which clearly shouldn't happen. I.e. I have umask=002 and all of sadden my test runs start to fail because dfs.data.dir has incorrect permissions.

          Ideally, test environment should guarantee that environment is properly set before making any assumptions or assertions.

          Show
          Konstantin Boudnik added a comment - A nasty side effect of this modification is that now a user's environment settings affect the result of test test. Which clearly shouldn't happen. I.e. I have umask=002 and all of sadden my test runs start to fail because dfs.data.dir has incorrect permissions. Ideally, test environment should guarantee that environment is properly set before making any assumptions or assertions.
          Hide
          Arun C Murthy added a comment -

          Thanks for the review Nicholas. The patch passed 'ant test' and 'ant test-patch'.

          Show
          Arun C Murthy added a comment - Thanks for the review Nicholas. The patch passed 'ant test' and 'ant test-patch'.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          +1 patch looks good.

          Show
          Tsz Wo Nicholas Sze added a comment - +1 patch looks good.
          Hide
          Arun C Murthy added a comment -

          Thanks for the review Nicholas, I've incorporated all your comments but for:

          In DiskChecker, how about combining mkdirsWithExistsCheck(..) and checkDir(..) into a method?

          Both methods are used, and hence I've kept them as-is.

          * You may use FsAction.READ_WRITE, instead of both FsAction.READ and FsAction.WRITE.

          I've kept separate checks for better diagnostic errors.


          I've also changed the implementation of mkdirsWithExistsAndPermissionCheck to use the existing mkdirsWithExistsCheck(File) since it has very different semantics than java.io.File.mkdirs, and hence I didn't go about re-inventing it.

          Show
          Arun C Murthy added a comment - Thanks for the review Nicholas, I've incorporated all your comments but for: In DiskChecker, how about combining mkdirsWithExistsCheck(..) and checkDir(..) into a method? Both methods are used, and hence I've kept them as-is. * You may use FsAction.READ_WRITE, instead of both FsAction.READ and FsAction.WRITE. I've kept separate checks for better diagnostic errors. I've also changed the implementation of mkdirsWithExistsAndPermissionCheck to use the existing mkdirsWithExistsCheck(File) since it has very different semantics than java.io.File.mkdirs, and hence I didn't go about re-inventing it.
          Hide
          Tsz Wo Nicholas Sze added a comment -

          Patch looks mostly good. Some suggestions:

          • In DiskChecker, how about combining mkdirsWithExistsCheck(..) and checkDir(..) into a method?
          • I think it is better eliminate to DiskChecker.checkPermission(..) so that we could avoid a public method. Otherwise, it will mutate and become something completely different in the future. In the tests, we could just use assertEquals(..).
          • You may use FsAction.READ_WRITE, instead of both FsAction.READ and FsAction.WRITE.
          • Need javadoc for the public methods.
          Show
          Tsz Wo Nicholas Sze added a comment - Patch looks mostly good. Some suggestions: In DiskChecker, how about combining mkdirsWithExistsCheck(..) and checkDir(..) into a method? I think it is better eliminate to DiskChecker.checkPermission(..) so that we could avoid a public method. Otherwise, it will mutate and become something completely different in the future. In the tests, we could just use assertEquals(..). You may use FsAction.READ_WRITE, instead of both FsAction.READ and FsAction.WRITE. Need javadoc for the public methods.
          Hide
          Arun C Murthy added a comment -

          Missed adding the new config to hdfs-default.xml, fixed now.

          Show
          Arun C Murthy added a comment - Missed adding the new config to hdfs-default.xml, fixed now.
          Hide
          Arun C Murthy added a comment -

          Thanks for the review Suresh, I've incorporated your comments.

          Show
          Arun C Murthy added a comment - Thanks for the review Suresh, I've incorporated your comments.
          Hide
          Suresh Srinivas added a comment -

          The new constructor FsPermission(String mode) could be used in FsPermission.getUMask() method, for UMASK_LABEL. That way code is reused and the existing tests cover the new constructor.

          Show
          Suresh Srinivas added a comment - The new constructor FsPermission(String mode) could be used in FsPermission.getUMask() method, for UMASK_LABEL. That way code is reused and the existing tests cover the new constructor.
          Hide
          Arun C Murthy added a comment -

          Patch for yhadoop-0.20, not for commit.

          I've enhanced org.apache.hadoop.util.DiskChecker, which is already used by DataNode etc. for managing local disks, checking existence etc. to provide relevant functionality. Along the way, I had to enhance FsPermission to add a ctor from String to be used to make the permission configurable.

          For hadoop-0.20 this is a single patch, for trunk we'll need to split it into changes to DiskChecker and DataNode as separate patches - they are on the way.

          Show
          Arun C Murthy added a comment - Patch for yhadoop-0.20, not for commit. I've enhanced org.apache.hadoop.util.DiskChecker, which is already used by DataNode etc. for managing local disks, checking existence etc. to provide relevant functionality. Along the way, I had to enhance FsPermission to add a ctor from String to be used to make the permission configurable. For hadoop-0.20 this is a single patch, for trunk we'll need to split it into changes to DiskChecker and DataNode as separate patches - they are on the way.
          Hide
          Hemanth Yamijala added a comment -

          Is the intention of this JIRA that in Hadoop common we provide an API that takes a list of paths and expected permissions, that will be called by Daemons at startup, and the common API implementation will throw out if there is a mismatch ?

          Show
          Hemanth Yamijala added a comment - Is the intention of this JIRA that in Hadoop common we provide an API that takes a list of paths and expected permissions, that will be called by Daemons at startup, and the common API implementation will throw out if there is a mismatch ?
          Hide
          steve_l added a comment -

          Devaraj, this code isn't in Hadoop, it's in the stuff I wrap the Hadoop daemon classes with: for the different services (NN, DN, etc) I create different directory lists and validate them. Hence the inconsistent indentation with the main Hadoop codebase.

          I think something like this can go into hadoop-core, one less thing for me to worry about, though the new URI-driven directory lists probably complicates the logic. I suspect this code only works for non-URI lists.

          Show
          steve_l added a comment - Devaraj, this code isn't in Hadoop, it's in the stuff I wrap the Hadoop daemon classes with: for the different services (NN, DN, etc) I create different directory lists and validate them. Hence the inconsistent indentation with the main Hadoop codebase. I think something like this can go into hadoop-core, one less thing for me to worry about, though the new URI-driven directory lists probably complicates the logic. I suspect this code only works for non-URI lists.
          Hide
          Devaraj Das added a comment -

          Steve, the code that you commented with is part of Hadoop already? Looks like we can add some checks to this for the permissions aspect. Assuming that all directories are supposed to be owned by the user who is running the daemon process, this method can also check for ownership of the directories where the owner should be the same as the process owner. I don't think we want to complicate it more than that. Makes sense?

          Show
          Devaraj Das added a comment - Steve, the code that you commented with is part of Hadoop already? Looks like we can add some checks to this for the permissions aspect. Assuming that all directories are supposed to be owned by the user who is running the daemon process, this method can also check for ownership of the directories where the owner should be the same as the process owner. I don't think we want to complicate it more than that. Makes sense?
          Hide
          steve_l added a comment -

          When I start up daemons I do create the directories on demand, code does the following

          1. extracts a list of directories from the deployment config info
          2. goes through trying to create them, logging ones it can't
          3. fails if there aren't any with a useful error message that even lists the dirs
          4. returns that list of dirs in a format that can then be handled by normal Conf parsers
          5. add that attr to the live graph of system state.

          This lets me set up the directories on the fly and publishes that information. Reviewing my code I see that I am not doing enough checks that the dirs really are of type "directory", I must be assuming that the stuff later on can handle that. Which they seem to, but their error messages don't include the directory listings in " java.io.IOException: All specified directories are not accessible or do not exist", which is a shame

             /**
               * Run through the directories, create all that are there
               *
               * @param dirs       list of directories
               * @param createDirs create the directories?
               * @return the directories all converted to a list split by commas
               */
              public static String createDirectoryList(Vector<String> dirs, boolean createDirs) 
                throws FileNotFoundException {
                  StringBuilder path = new StringBuilder();
                  int failureCount = 0;
                  StringBuilder failureText = new StringBuilder();
                  for (String dir : dirs) {
                      File directory = new File(dir);
                      if (createDirs) {
                          if(!directory.mkdirs() && !directory.exists()) {
                              String message = "Failed to create directory " + directory;
                              LogFactory.getLog(HadoopComponentImpl.class.getName())
                                      .warn(message);
                              failureCount++;
                              failureText.append(message).append("\n");
                          }
                      }
                      if (path.length() > 0) {
                          path.append(',');
                      }
                      path.append(directory.getAbsolutePath());
                  }
                  if (failureCount == dirs.size()) {
                      throw new FileNotFoundException("Failed to create any of " + path.toString());
                  }
                  return path.toString();
              }
          
          Show
          steve_l added a comment - When I start up daemons I do create the directories on demand, code does the following extracts a list of directories from the deployment config info goes through trying to create them, logging ones it can't fails if there aren't any with a useful error message that even lists the dirs returns that list of dirs in a format that can then be handled by normal Conf parsers add that attr to the live graph of system state. This lets me set up the directories on the fly and publishes that information. Reviewing my code I see that I am not doing enough checks that the dirs really are of type "directory", I must be assuming that the stuff later on can handle that. Which they seem to, but their error messages don't include the directory listings in " java.io.IOException: All specified directories are not accessible or do not exist", which is a shame /** * Run through the directories, create all that are there * * @param dirs list of directories * @param createDirs create the directories? * @ return the directories all converted to a list split by commas */ public static String createDirectoryList(Vector< String > dirs, boolean createDirs) throws FileNotFoundException { StringBuilder path = new StringBuilder(); int failureCount = 0; StringBuilder failureText = new StringBuilder(); for ( String dir : dirs) { File directory = new File(dir); if (createDirs) { if (!directory.mkdirs() && !directory.exists()) { String message = "Failed to create directory " + directory; LogFactory.getLog(HadoopComponentImpl.class.getName()) .warn(message); failureCount++; failureText.append(message).append( "\n" ); } } if (path.length() > 0) { path.append(','); } path.append(directory.getAbsolutePath()); } if (failureCount == dirs.size()) { throw new FileNotFoundException( "Failed to create any of " + path.toString()); } return path.toString(); }
          Hide
          steve_l added a comment -

          options

          1. make any check downgradable to a message logged at warning/info/debug
          2. just do a check that you can read/write content in the dir
            # Make it an option to drop any dir that isn't considered secure, but only fail if the number of directories for each role falls below a specified number.

          Not being able to read/write files, bad news. Having the clock in that dir significantly off from your time, potentially trouble, though on network mounted filesystems not always a complete surprise.

          Show
          steve_l added a comment - options make any check downgradable to a message logged at warning/info/debug just do a check that you can read/write content in the dir # Make it an option to drop any dir that isn't considered secure, but only fail if the number of directories for each role falls below a specified number. Not being able to read/write files, bad news. Having the clock in that dir significantly off from your time, potentially trouble, though on network mounted filesystems not always a complete surprise.
          Hide
          steve_l added a comment -

          It may be that the user is happy with world read/write permissions, depends on the installation.

          Show
          steve_l added a comment - It may be that the user is happy with world read/write permissions, depends on the installation.

            People

            • Assignee:
              Arun C Murthy
              Reporter:
              Devaraj Das
            • Votes:
              0 Vote for this issue
              Watchers:
              13 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development