Hadoop Common
  1. Hadoop Common
  2. HADOOP-6521

FsPermission:SetUMask not updated to use new-style umask setting.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.21.0, 0.22.0
    • Fix Version/s: 0.21.0
    • Component/s: fs
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      FsPermission:

      221   /** Set the user file creation mask (umask) */
      222   public static void setUMask(Configuration conf, FsPermission umask) {                                    
      223     conf.setInt(UMASK_LABEL, umask.toShort());
      224   }
      

      Needs to be updated to not use a decimal value. This is a bug introduced by HADOOP-6234.

      1. hadoop-6521.1.patch
        8 kB
        Suresh Srinivas
      2. hadoop-6521.patch
        22 kB
        Suresh Srinivas
      3. ASF.LICENSE.NOT.GRANTED--hadoop-6521.patch
        6 kB
        Suresh Srinivas
      4. hadoop-6521.rel20.1.patch
        4 kB
        Suresh Srinivas
      5. hadoop-6521.rel20.patch
        4 kB
        Suresh Srinivas
      6. hadoop-6521.rel20.patch
        4 kB
        Suresh Srinivas
      7. hadoop-6521.rel20.patch
        4 kB
        Suresh Srinivas

        Issue Links

          Activity

          Jakob Homan created issue -
          Hide
          Suresh Srinivas added a comment -

          Patch for release 20 attached. The approach is:

          1. To ensure backward compatibility, when applications use the old key "dfs.umask" and the server default has "dfs.umaskmode", old key overrides the new key. That is first the old key is used for getting umask and then the new key.
          2. FsPermission.setUMask(Configuration conf, FsPermission umask) currently has a bug. When setting the "dfs.umaskmode" param in the configuration, it should convert the umask decimal value to octal.

          This patch is not applicable to release 21, since it uses deprecation mechanism introduced in Hadoop-6105 and more work is needed to make this solution work in that realm.

          Show
          Suresh Srinivas added a comment - Patch for release 20 attached. The approach is: To ensure backward compatibility, when applications use the old key "dfs.umask" and the server default has "dfs.umaskmode", old key overrides the new key. That is first the old key is used for getting umask and then the new key. FsPermission.setUMask(Configuration conf, FsPermission umask) currently has a bug. When setting the "dfs.umaskmode" param in the configuration, it should convert the umask decimal value to octal. This patch is not applicable to release 21, since it uses deprecation mechanism introduced in Hadoop-6105 and more work is needed to make this solution work in that realm.
          Suresh Srinivas made changes -
          Field Original Value New Value
          Attachment hadoop-6521.rel20.patch [ 12434443 ]
          Suresh Srinivas made changes -
          Link This issue blocks HADOOP-6234 [ HADOOP-6234 ]
          Hide
          Suresh Srinivas added a comment -

          Ran some manual tests just to verify the fix. Set in the hdfs-site.xml "dfs.umaskmode" as 077 and run the following tests:

          1. bin/hadoop dfs -put /tmp/test /tmp/test; bin/hadoop dfs -ls /tmp/test;bin/hadoop dfs -rmr /tmp/test
            • File mode should be -rw-------
          2. bin/hadoop dfs -Ddfs.umask=18 -put /tmp/test /tmp/test; bin/hadoop dfs -ls /tmp/test;bin/hadoop dfs -rmr /tmp/test
            • File mode should be -rw-r--r--
          3. bin/hadoop dfs -Ddfs.umaskmode=222 -put /tmp/test /tmp/test; bin/hadoop dfs -ls /tmp/test;bin/hadoop dfs -rmr /tmp/test
            • File mode should be -r--r--r--
          Show
          Suresh Srinivas added a comment - Ran some manual tests just to verify the fix. Set in the hdfs-site.xml "dfs.umaskmode" as 077 and run the following tests: bin/hadoop dfs -put /tmp/test /tmp/test; bin/hadoop dfs -ls /tmp/test;bin/hadoop dfs -rmr /tmp/test File mode should be -rw------- bin/hadoop dfs -Ddfs.umask=18 -put /tmp/test /tmp/test; bin/hadoop dfs -ls /tmp/test;bin/hadoop dfs -rmr /tmp/test File mode should be -rw-r--r-- bin/hadoop dfs -Ddfs.umaskmode=222 -put /tmp/test /tmp/test; bin/hadoop dfs -ls /tmp/test;bin/hadoop dfs -rmr /tmp/test File mode should be -r--r--r--
          Hide
          Tsz Wo Nicholas Sze added a comment -
          • I think it is better to update both UMASK_LABEL and DEPRECATED_UMASK_LABEL in setUMask(..).
          • Also when both UMASK_LABEL and DEPRECATED_UMASK_LABEL are set in conf and they have different values, should we throw an exception?
          Show
          Tsz Wo Nicholas Sze added a comment - I think it is better to update both UMASK_LABEL and DEPRECATED_UMASK_LABEL in setUMask(..). Also when both UMASK_LABEL and DEPRECATED_UMASK_LABEL are set in conf and they have different values, should we throw an exception?
          Hide
          Suresh Srinivas added a comment -

          > I think it is better to update both UMASK_LABEL and DEPRECATED_UMASK_LABEL in setUMask(..).
          sounds reasonable. Though only one of them is going to be used

          > Also when both UMASK_LABEL and DEPRECATED_UMASK_LABEL are set in conf and they have different values, should we throw an exception?
          In release 20, the server config will be setup with UMASK_LABEL. The user can override the default by:

          1. Calling FsPermission.setUMask() (this has been addressed)
          2. By setting configuration param either by calling conf.set() or by specifying umask in command line (see manual tests posted above).

          Given that I think we should allow these two configurations to have separate value. Further, giving higher priority to deprecated key, assuming the user is likely to use it is the right choice. If the user happens to use the new key there is no conflict.

          Show
          Suresh Srinivas added a comment - > I think it is better to update both UMASK_LABEL and DEPRECATED_UMASK_LABEL in setUMask(..). sounds reasonable. Though only one of them is going to be used > Also when both UMASK_LABEL and DEPRECATED_UMASK_LABEL are set in conf and they have different values, should we throw an exception? In release 20, the server config will be setup with UMASK_LABEL. The user can override the default by: Calling FsPermission.setUMask() (this has been addressed) By setting configuration param either by calling conf.set() or by specifying umask in command line (see manual tests posted above). Given that I think we should allow these two configurations to have separate value. Further, giving higher priority to deprecated key, assuming the user is likely to use it is the right choice. If the user happens to use the new key there is no conflict.
          Hide
          Suresh Srinivas added a comment -

          New patch addresses comments from Nicholas.

          Show
          Suresh Srinivas added a comment - New patch addresses comments from Nicholas.
          Suresh Srinivas made changes -
          Attachment hadoop-6521.rel20.patch [ 12434450 ]
          Hide
          Jakob Homan added a comment -

          Setting both is a good approach, but will have the unfortunate side effect that if the user is correctly using the new key, the old key will also be set and the user will then get undeserved deprecation warnings. An offline discussion c/ Nicholas and Suresh suggests that comparing the values of both the old and new keys may miss some valid cases where a warning is warranted, but will avoid invalid warnings. This is probably the best approach to take.

          Show
          Jakob Homan added a comment - Setting both is a good approach, but will have the unfortunate side effect that if the user is correctly using the new key, the old key will also be set and the user will then get undeserved deprecation warnings. An offline discussion c/ Nicholas and Suresh suggests that comparing the values of both the old and new keys may miss some valid cases where a warning is warranted, but will avoid invalid warnings. This is probably the best approach to take.
          Hide
          Suresh Srinivas added a comment -

          New patch addresses comments from Jakob and Nicholas.

          Show
          Suresh Srinivas added a comment - New patch addresses comments from Jakob and Nicholas.
          Suresh Srinivas made changes -
          Attachment hadoop-6521.rel20.patch [ 12434458 ]
          Tsz Wo Nicholas Sze made changes -
          Assignee Suresh Srinivas [ sureshms ]
          Description FsPermission:
          221 /** Set the user file creation mask (umask) */
          222 public static void setUMask(Configuration conf, FsPermission umask) {
          223 conf.setInt(UMASK_LABEL, umask.toShort());
          224 }
          Needs to be updated to not use a decimal value. This is a bug introduced by HADOOP-6234.
          FsPermission:
          {code}
          221 /** Set the user file creation mask (umask) */
          222 public static void setUMask(Configuration conf, FsPermission umask) {
          223 conf.setInt(UMASK_LABEL, umask.toShort());
          224 }
          {code}
          Needs to be updated to not use a decimal value. This is a bug introduced by HADOOP-6234.
          Component/s fs [ 12310689 ]
          Hide
          Suresh Srinivas added a comment -

          New patch.

          Show
          Suresh Srinivas added a comment - New patch.
          Suresh Srinivas made changes -
          Attachment hadoop-6521.rel20.1.patch [ 12434469 ]
          Hide
          Tsz Wo Nicholas Sze added a comment -

          +1 the new patch looks good.

          Show
          Tsz Wo Nicholas Sze added a comment - +1 the new patch looks good.
          Hide
          Suresh Srinivas added a comment -

          Changes in the patch:

          1. Symbolic and octal umask mode was added configurable with param name "dfs.umaskmode". It deprecates the old param "dfs.umask". Previously for backward compatibility new deprecation mechanism introduced in 0.21 was used. This mechanism is useful when a config param name changes, to map the old config name to the new. However in case of umask, the semantics of the param also changes. Old param expects decimal value and the new param expects symbolic or octal value. The deprecation mechanism in addition to mapping old to new names, must also translated the old value to new value. Given the lack of this capability, umask will no longer use config deprecation mechanism.
          1. Following changes from 0.20 version of the patch is carried forward, with the exception of test changes. Test changes are needed in HDFS. I will created a separate jira for that.

            1. To ensure backward compatibility, when applications use the old key "dfs.umask" and the server default has "dfs.umaskmode", old key overrides the new key. That is first the old key is used for getting umask and then the new key.
            2. FsPermission.setUMask(Configuration conf, FsPermission umask) currently has a bug. When setting the "dfs.umaskmode" param in the configuration, it should convert the umask decimal value to octal.

          Show
          Suresh Srinivas added a comment - Changes in the patch: Symbolic and octal umask mode was added configurable with param name "dfs.umaskmode". It deprecates the old param "dfs.umask". Previously for backward compatibility new deprecation mechanism introduced in 0.21 was used. This mechanism is useful when a config param name changes, to map the old config name to the new. However in case of umask, the semantics of the param also changes. Old param expects decimal value and the new param expects symbolic or octal value. The deprecation mechanism in addition to mapping old to new names, must also translated the old value to new value. Given the lack of this capability, umask will no longer use config deprecation mechanism. Following changes from 0.20 version of the patch is carried forward, with the exception of test changes. Test changes are needed in HDFS. I will created a separate jira for that. 1. To ensure backward compatibility, when applications use the old key "dfs.umask" and the server default has "dfs.umaskmode", old key overrides the new key. That is first the old key is used for getting umask and then the new key. 2. FsPermission.setUMask(Configuration conf, FsPermission umask) currently has a bug. When setting the "dfs.umaskmode" param in the configuration, it should convert the umask decimal value to octal.
          Suresh Srinivas made changes -
          Attachment hadoop-6521.patch [ 12441897 ]
          Suresh Srinivas made changes -
          Link This issue blocks HDFS-1099 [ HDFS-1099 ]
          Hide
          Jakob Homan added a comment -

          If we're - correctly - not using the deprecatedKeyWasSet method, and this was the only place it was used and it's not yet been released, might we just as well remove it? Otherwise it's an unused, potentially buggy API. Otherwise, +1.

          Show
          Jakob Homan added a comment - If we're - correctly - not using the deprecatedKeyWasSet method, and this was the only place it was used and it's not yet been released, might we just as well remove it? Otherwise it's an unused, potentially buggy API. Otherwise, +1.
          Jakob Homan made changes -
          Hadoop Flags [Reviewed]
          Hide
          Suresh Srinivas added a comment -

          Removed deprecateKeyWasSet() method and related tests.

          Show
          Suresh Srinivas added a comment - Removed deprecateKeyWasSet() method and related tests.
          Suresh Srinivas made changes -
          Attachment hadoop-6521.patch [ 12442708 ]
          Suresh Srinivas made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12442708/hadoop-6521.patch
          against trunk revision 937183.

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

          +1 tests included. The patch appears to include 3 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-h1.grid.sp2.yahoo.net/50/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h1.grid.sp2.yahoo.net/50/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h1.grid.sp2.yahoo.net/50/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h1.grid.sp2.yahoo.net/50/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/12442708/hadoop-6521.patch against trunk revision 937183. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 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-h1.grid.sp2.yahoo.net/50/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h1.grid.sp2.yahoo.net/50/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h1.grid.sp2.yahoo.net/50/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h1.grid.sp2.yahoo.net/50/console This message is automatically generated.
          Suresh Srinivas made changes -
          Status Patch Available [ 10002 ] Open [ 1 ]
          Hide
          Suresh Srinivas added a comment -

          Attaching the right patch.

          Show
          Suresh Srinivas added a comment - Attaching the right patch.
          Suresh Srinivas made changes -
          Attachment hadoop-6521.1.patch [ 12442715 ]
          Suresh Srinivas made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12442715/hadoop-6521.1.patch
          against trunk revision 937183.

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

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

          I committed this change.

          Show
          Suresh Srinivas added a comment - I committed this change.
          Suresh Srinivas made changes -
          Resolution Fixed [ 1 ]
          Status Patch Available [ 10002 ] Resolved [ 5 ]
          Fix Version/s 0.22.0 [ 12314296 ]
          Suresh Srinivas made changes -
          Issue Type Bug [ 1 ] Test [ 6 ]
          Affects Version/s 0.21.0 [ 12313563 ]
          Affects Version/s 0.22.0 [ 12314296 ]
          Suresh Srinivas made changes -
          Issue Type Test [ 6 ] Bug [ 1 ]
          Tom White made changes -
          Fix Version/s 0.21.0 [ 12313563 ]
          Fix Version/s 0.22.0 [ 12314296 ]
          Tom White made changes -
          Status Resolved [ 5 ] Closed [ 6 ]
          Gavin made changes -
          Link This issue blocks HDFS-1099 [ HDFS-1099 ]
          Gavin made changes -
          Link This issue is depended upon by HDFS-1099 [ HDFS-1099 ]

            People

            • Assignee:
              Suresh Srinivas
              Reporter:
              Jakob Homan
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development