hadoop-common-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From sur...@apache.org
Subject svn commit: r1454890 - in /hadoop/common/branches/branch-1-win: ./ src/core/org/apache/hadoop/util/ src/test/org/apache/hadoop/test/ src/test/org/apache/hadoop/util/
Date Sun, 10 Mar 2013 18:30:42 GMT
Author: suresh
Date: Sun Mar 10 18:30:42 2013
New Revision: 1454890

URL: http://svn.apache.org/r1454890
Log:
HADOOP-8973. DiskChecker cannot reliably detect an inaccessible disk on Windows with NTFS
ACLs. Contributed by Chris Nauroth.

Added:
    hadoop/common/branches/branch-1-win/src/test/org/apache/hadoop/test/MockitoMaker.java
    hadoop/common/branches/branch-1-win/src/test/org/apache/hadoop/util/TestDiskChecker.java
Modified:
    hadoop/common/branches/branch-1-win/CHANGES.branch-1-win.txt
    hadoop/common/branches/branch-1-win/src/core/org/apache/hadoop/util/DiskChecker.java
    hadoop/common/branches/branch-1-win/src/core/org/apache/hadoop/util/Shell.java

Modified: hadoop/common/branches/branch-1-win/CHANGES.branch-1-win.txt
URL: http://svn.apache.org/viewvc/hadoop/common/branches/branch-1-win/CHANGES.branch-1-win.txt?rev=1454890&r1=1454889&r2=1454890&view=diff
==============================================================================
--- hadoop/common/branches/branch-1-win/CHANGES.branch-1-win.txt (original)
+++ hadoop/common/branches/branch-1-win/CHANGES.branch-1-win.txt Sun Mar 10 18:30:42 2013
@@ -345,3 +345,7 @@ Branch-hadoop-1-win (branched from branc
 
     MAPREDUCE-4969. TestKeyValueTextInputFormat test fails with Open JDK 7.
     (Arpit Agarwal via suresh)
+
+    HADOOP-8973. DiskChecker cannot reliably detect an inaccessible disk on
+    Windows with NTFS ACLs. (Chris Nauroth via suresh)
+

Modified: hadoop/common/branches/branch-1-win/src/core/org/apache/hadoop/util/DiskChecker.java
URL: http://svn.apache.org/viewvc/hadoop/common/branches/branch-1-win/src/core/org/apache/hadoop/util/DiskChecker.java?rev=1454890&r1=1454889&r2=1454890&view=diff
==============================================================================
--- hadoop/common/branches/branch-1-win/src/core/org/apache/hadoop/util/DiskChecker.java (original)
+++ hadoop/common/branches/branch-1-win/src/core/org/apache/hadoop/util/DiskChecker.java Sun
Mar 10 18:30:42 2013
@@ -19,15 +19,12 @@
 package org.apache.hadoop.util;
 
 import java.io.File;
-import java.io.FileNotFoundException;
 import java.io.IOException;
 
-import org.apache.hadoop.fs.FileStatus;
-import org.apache.hadoop.fs.FileSystem;
 import org.apache.hadoop.fs.LocalFileSystem;
 import org.apache.hadoop.fs.Path;
-import org.apache.hadoop.fs.permission.FsAction;
 import org.apache.hadoop.fs.permission.FsPermission;
+import org.apache.hadoop.util.Shell;
 
 /**
  * Class that provides utility functions for checking disk problem
@@ -35,10 +32,16 @@ import org.apache.hadoop.fs.permission.F
 
 public class DiskChecker {
 
+  private static final long SHELL_TIMEOUT = 10 * 1000;
+
   public static class DiskErrorException extends IOException {
     public DiskErrorException(String msg) {
       super(msg);
     }
+
+    public DiskErrorException(String msg, Throwable cause) {
+      super(msg, cause);
+    }
   }
     
   public static class DiskOutOfSpaceException extends IOException {
@@ -82,21 +85,11 @@ public class DiskChecker {
    * @throws DiskErrorException
    */
   public static void checkDir(File dir) throws DiskErrorException {
-    if (!mkdirsWithExistsCheck(dir))
+    if (!mkdirsWithExistsCheck(dir)) {
       throw new DiskErrorException("can not create directory: " 
                                    + dir.toString());
-        
-    if (!dir.isDirectory())
-      throw new DiskErrorException("not a directory: " 
-                                   + dir.toString());
-            
-    if (!dir.canRead())
-      throw new DiskErrorException("directory is not readable: " 
-                                   + dir.toString());
-            
-    if (!dir.canWrite())
-      throw new DiskErrorException("directory is not writable: " 
-                                   + dir.toString());
+    }
+    checkDirAccess(dir);
   }
 
   private static void checkPermission(Path dir, 
@@ -129,22 +122,17 @@ public class DiskChecker {
    * @param expected expected permission
    * @return true on success, false on failure
    */
-  public static boolean mkdirsWithExistsAndPermissionCheck(
+  public static void mkdirsWithExistsAndPermissionCheck(
       LocalFileSystem localFS, Path dir, FsPermission expected) 
-  throws IOException {
-    File directory = new File(dir.makeQualified(localFS).toUri().getPath());
-    if (!directory.exists()) {
-      boolean created = mkdirsWithExistsCheck(directory);
-      if (created) {
-        localFS.setPermission(dir, expected);
-        return true;
-      } else {
-        return false;
-      }
-    }
+      throws IOException {
+    File directory = localFS.pathToFile(dir);
+    boolean created = false;
 
-    checkPermission(dir, expected, localFS.getFileStatus(dir).getPermission());
-    return true;
+    if (!directory.exists())
+      created = mkdirsWithExistsCheck(directory);
+
+    if (created || !localFS.getFileStatus(dir).getPermission().equals(expected))
+        localFS.setPermission(dir, expected);
   }
   
   /**
@@ -159,26 +147,103 @@ public class DiskChecker {
   public static void checkDir(LocalFileSystem localFS, Path dir, 
                               FsPermission expected) 
   throws DiskErrorException, IOException {
-    if (!mkdirsWithExistsAndPermissionCheck(localFS, dir, expected))
-      throw new DiskErrorException("can not create directory: " 
+    mkdirsWithExistsAndPermissionCheck(localFS, dir, expected);
+    checkDirAccess(localFS.pathToFile(dir));
+  }
+
+  /**
+   * Checks that the given file is a directory and that the current running
+   * process can read, write, and execute it.
+   * 
+   * @param dir File to check
+   * @throws DiskErrorException if dir is not a directory, not readable, not
+   *   writable, or not executable
+   */
+  private static void checkDirAccess(File dir) throws DiskErrorException {
+    if (!dir.isDirectory()) {
+      throw new DiskErrorException("Not a directory: "
                                    + dir.toString());
+    }
 
-    FileStatus stat = localFS.getFileStatus(dir);
-    FsPermission actual = stat.getPermission();
-    
-    if (!stat.isDir())
-      throw new DiskErrorException("not a directory: " 
+    if (Shell.WINDOWS) {
+      checkAccessByFileSystemInteraction(dir);
+    } else {
+      checkAccessByFileMethods(dir);
+    }
+  }
+
+  /**
+   * Checks that the current running process can read, write, and execute the
+   * given directory by using methods of the File object.
+   * 
+   * @param dir File to check
+   * @throws DiskErrorException if dir is not readable, not writable, or not
+   *   executable
+   */
+  private static void checkAccessByFileMethods(File dir)
+      throws DiskErrorException {
+    if (!dir.canRead()) {
+      throw new DiskErrorException("Directory is not readable: "
+                                   + dir.toString());
+    }
+
+    if (!dir.canWrite()) {
+      throw new DiskErrorException("Directory is not writable: "
                                    + dir.toString());
-            
-    FsAction user = actual.getUserAction();
-    if (!user.implies(FsAction.READ))
-      throw new DiskErrorException("directory is not readable: " 
-                                   + dir.toString());
-            
-    if (!user.implies(FsAction.WRITE))
-      throw new DiskErrorException("directory is not writable: " 
+    }
+
+    if (!dir.canExecute()) {
+      throw new DiskErrorException("Directory is not executable: "
                                    + dir.toString());
+    }
   }
 
-}
+  /**
+   * Checks that the current running process can read, write, and execute the
+   * given directory by attempting each of those operations on the file system.
+   * This method contains several workarounds to known JVM bugs that cause
+   * File.canRead, File.canWrite, and File.canExecute to return incorrect results
+   * on Windows with NTFS ACLs.  See:
+   * http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6203387
+   * These bugs are supposed to be fixed in JDK7.
+   * 
+   * @param dir File to check
+   * @throws DiskErrorException if dir is not readable, not writable, or not
+   *   executable
+   */
+  private static void checkAccessByFileSystemInteraction(File dir)
+      throws DiskErrorException {
+    // Make sure we can read the directory by listing it.
+    if (dir.list() == null) {
+      throw new DiskErrorException("Directory is not readable: "
+                                   + dir.toString());
+    }
+
+    // Make sure we can write to the directory by creating a temp file in it.
+    try {
+      File tempFile = File.createTempFile("checkDirAccess", null, dir);
+      if (!tempFile.delete()) {
+        throw new DiskErrorException("Directory is not writable: "
+                                     + dir.toString());
+      }
+    } catch (IOException e) {
+      throw new DiskErrorException("Directory is not writable: "
+                                   + dir.toString(), e);
+    }
 
+    // Make sure the directory is executable by trying to cd into it.  This
+    // launches a separate process.  It does not change the working directory of
+    // the current process.
+    try {
+      String[] cdCmd = new String[] { "cmd", "/C", "cd",
+          dir.getAbsolutePath() };
+      Shell.execCommand(null, cdCmd, SHELL_TIMEOUT);
+    } catch (Shell.ExitCodeException e) {
+      throw new DiskErrorException("Directory is not executable: "
+                                   + dir.toString(), e);
+    } catch (IOException e) {
+      throw new DiskErrorException("Directory is not executable: "
+                                   + dir.toString(), e);
+    }
+  }
+}

Modified: hadoop/common/branches/branch-1-win/src/core/org/apache/hadoop/util/Shell.java
URL: http://svn.apache.org/viewvc/hadoop/common/branches/branch-1-win/src/core/org/apache/hadoop/util/Shell.java?rev=1454890&r1=1454889&r2=1454890&view=diff
==============================================================================
--- hadoop/common/branches/branch-1-win/src/core/org/apache/hadoop/util/Shell.java (original)
+++ hadoop/common/branches/branch-1-win/src/core/org/apache/hadoop/util/Shell.java Sun Mar
10 18:30:42 2013
@@ -21,6 +21,7 @@ import java.io.BufferedReader;
 import java.io.File;
 import java.io.IOException;
 import java.io.InputStreamReader;
+import java.util.Arrays;
 import java.util.Map;
 import java.util.Timer;
 import java.util.TimerTask;
@@ -201,6 +202,22 @@ abstract public class Shell {
     }
   }
 
+  /**
+   * Return a command to set permission for specific file.
+   * 
+   * @param perm String permission to set
+   * @param recursive boolean true to apply to all sub-directories recursively
+   * @param file String file to set
+   * @return String[] containing command and arguments
+   */
+  public static String[] getSetPermissionCommand(String perm, boolean recursive,
+                                                 String file) {
+    String[] baseCmd = getSetPermissionCommand(perm, recursive);
+    String[] cmdWithFile = Arrays.copyOf(baseCmd, baseCmd.length + 1);
+    cmdWithFile[cmdWithFile.length - 1] = file;
+    return cmdWithFile;
+  }
+
   /** Return a regular expression string that match environment variables */
   public static String getEnvironmentVariableRegex() {
     return (WINDOWS) ? "%([A-Za-z_][A-Za-z0-9_]*?)%" :

Added: hadoop/common/branches/branch-1-win/src/test/org/apache/hadoop/test/MockitoMaker.java
URL: http://svn.apache.org/viewvc/hadoop/common/branches/branch-1-win/src/test/org/apache/hadoop/test/MockitoMaker.java?rev=1454890&view=auto
==============================================================================
--- hadoop/common/branches/branch-1-win/src/test/org/apache/hadoop/test/MockitoMaker.java
(added)
+++ hadoop/common/branches/branch-1-win/src/test/org/apache/hadoop/test/MockitoMaker.java
Sun Mar 10 18:30:42 2013
@@ -0,0 +1,132 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.test;
+
+import static org.mockito.Mockito.*;
+
+/**
+ * Helper class to create one-liner stubs, so that instead of: <pre>
+ * SomeType someDescriptiveMock = mock(SomeType.class);
+ * when(someDescriptiveMock.someMethod()).thenReturn(someValue);</pre>
+ * <p>You can now do: <pre>
+ * SomeType someDescriptiveMock = make(stub(SomeType.class)
+ *     .returning(someValue).from.someMethod());</pre>
+ */
+public class MockitoMaker {
+
+  /**
+   * Create a mock object from a mocked method call.
+   *
+   * @param <T> type of mocked object
+   * @param methodCall  for mocked object
+   * @return mocked object
+   */
+  @SuppressWarnings("unchecked")
+  public static <T> T make(Object methodCall) {
+    StubBuilder<T> sb = StubBuilder.current();
+    when(methodCall).thenReturn(sb.firstReturn, sb.laterReturns);
+    return (T) StubBuilder.current().from;
+  }
+
+  /**
+   * Create a stub builder of a mocked object.
+   *
+   * @param <T>     type of the target object to be mocked
+   * @param target  class of the target object to be mocked
+   * @return the stub builder of the mocked object
+   */
+  public static <T> StubBuilder<T> stub(Class<T> target) {
+    return new StubBuilder<T>(mock(target));
+  }
+
+  /**
+   * Builder class for stubs
+   * @param <T> type of the object to be mocked
+   */
+  public static class StubBuilder<T> {
+
+    /**
+     * The target mock object
+     */
+    public final T from;
+
+    // We want to be able to use this even when the tests are run in parallel.
+    @SuppressWarnings("rawtypes")
+    private static final ThreadLocal<StubBuilder> tls =
+        new ThreadLocal<StubBuilder>() {
+          @Override protected StubBuilder initialValue() {
+            return new StubBuilder();
+          }
+        };
+
+    private Object firstReturn = null;
+    private Object[] laterReturns = {};
+
+    /**
+     * Default constructor for the initial stub builder
+     */
+    public StubBuilder() {
+      this.from = null;
+    }
+
+    /**
+     * Construct a stub builder with a mock instance
+     *
+     * @param mockInstance  the mock object
+     */
+    public StubBuilder(T mockInstance) {
+      tls.set(this);
+      this.from = mockInstance;
+    }
+
+    /**
+     * Get the current stub builder from thread local
+     *
+     * @param <T>
+     * @return the stub builder of the mocked object
+     */
+    @SuppressWarnings("unchecked")
+    public static <T> StubBuilder<T> current() {
+      return tls.get();
+    }
+
+    /**
+     * Set the return value for the current stub builder
+     *
+     * @param value the return value
+     * @return the stub builder
+     */
+    public StubBuilder<T> returning(Object value) {
+      this.firstReturn = value;
+      return this;
+    }
+
+    /**
+     * Set the return values for the current stub builder
+     *
+     * @param value   the first return value
+     * @param values  the return values for later invocations
+     * @return the stub builder
+     */
+    public StubBuilder<T> returning(Object value, Object... values) {
+      this.firstReturn = value;
+      this.laterReturns = values;
+      return this;
+    }
+  }
+}

Added: hadoop/common/branches/branch-1-win/src/test/org/apache/hadoop/util/TestDiskChecker.java
URL: http://svn.apache.org/viewvc/hadoop/common/branches/branch-1-win/src/test/org/apache/hadoop/util/TestDiskChecker.java?rev=1454890&view=auto
==============================================================================
--- hadoop/common/branches/branch-1-win/src/test/org/apache/hadoop/util/TestDiskChecker.java
(added)
+++ hadoop/common/branches/branch-1-win/src/test/org/apache/hadoop/util/TestDiskChecker.java
Sun Mar 10 18:30:42 2013
@@ -0,0 +1,183 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.util;
+
+import java.io.*;
+
+import org.junit.Test;
+import static org.junit.Assert.*;
+
+import static org.mockito.Mockito.*;
+
+import static org.apache.hadoop.test.MockitoMaker.*;
+import static org.apache.hadoop.fs.permission.FsAction.*;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.FileStatus;
+import org.apache.hadoop.fs.FileSystem;
+import org.apache.hadoop.fs.LocalFileSystem;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.fs.permission.FsPermission;
+import org.apache.hadoop.util.DiskChecker.DiskErrorException;
+import org.apache.hadoop.util.Shell;
+
+public class TestDiskChecker {
+  final FsPermission defaultPerm = new FsPermission("755");
+  final FsPermission invalidPerm = new FsPermission("000");
+
+  @Test (timeout = 30000)
+  public void testMkdirs_dirExists() throws Throwable {
+    _mkdirs(true, defaultPerm, defaultPerm);
+  }
+
+  @Test (timeout = 30000)
+  public void testMkdirs_noDir() throws Throwable {
+    _mkdirs(false, defaultPerm, defaultPerm);
+  }
+
+  @Test (timeout = 30000)
+  public void testMkdirs_dirExists_badUmask() throws Throwable {
+    _mkdirs(true, defaultPerm, invalidPerm);
+  }
+
+  @Test (timeout = 30000)
+  public void testMkdirs_noDir_badUmask() throws Throwable {
+    _mkdirs(false, defaultPerm, invalidPerm);
+  }
+
+  private void _mkdirs(boolean exists, FsPermission before, FsPermission after)
+      throws Throwable {
+    File localDir = make(stub(File.class).returning(exists).from.exists());
+    when(localDir.mkdir()).thenReturn(true);
+    Path dir = mock(Path.class); // use default stubs
+    LocalFileSystem fs = make(stub(LocalFileSystem.class)
+        .returning(localDir).from.pathToFile(dir));
+    FileStatus stat = make(stub(FileStatus.class)
+        .returning(after).from.getPermission());
+    when(fs.getFileStatus(dir)).thenReturn(stat);
+
+    try {
+      DiskChecker.mkdirsWithExistsAndPermissionCheck(fs, dir, before);
+
+      if (!exists)
+        verify(fs).setPermission(dir, before);
+      else {
+        verify(fs).getFileStatus(dir);
+        verify(stat).getPermission();
+      }
+    }
+    catch (DiskErrorException e) {
+      if (before != after)
+        assertTrue(e.getMessage().startsWith("Incorrect permission"));
+    }
+  }
+
+  @Test (timeout = 30000)
+  public void testCheckDir_normal() throws Throwable {
+    _checkDirs(true, new FsPermission("755"), true);
+  }
+
+  @Test (timeout = 30000)
+  public void testCheckDir_notDir() throws Throwable {
+    _checkDirs(false, new FsPermission("000"), false);
+  }
+
+  @Test (timeout = 30000)
+  public void testCheckDir_notReadable() throws Throwable {
+    _checkDirs(true, new FsPermission("000"), false);
+  }
+
+  @Test (timeout = 30000)
+  public void testCheckDir_notWritable() throws Throwable {
+    _checkDirs(true, new FsPermission("444"), false);
+  }
+
+  @Test (timeout = 30000)
+  public void testCheckDir_notListable() throws Throwable {
+    _checkDirs(true, new FsPermission("666"), false);   // not listable
+  }
+
+  private void _checkDirs(boolean isDir, FsPermission perm, boolean success)
+      throws Throwable {
+    File localDir = File.createTempFile("test", "tmp");
+    if (isDir) {
+      localDir.delete();
+      localDir.mkdir();
+    }
+    Shell.execCommand(Shell.getSetPermissionCommand(String.format("%04o",
+      perm.toShort()), false, localDir.getAbsolutePath()));
+    try {
+      DiskChecker.checkDir(FileSystem.getLocal(new Configuration()),
+        new Path(localDir.getAbsolutePath()), perm);
+      assertTrue("checkDir success", success);
+    } catch (DiskErrorException e) {
+      assertFalse("checkDir success", success);
+    }
+    localDir.delete();
+  }
+
+  /**
+   * These test cases test to test the creation of a local folder with correct
+   * permission for result of mapper.
+   */
+
+  @Test (timeout = 30000)
+  public void testCheckDir_normal_local() throws Throwable {
+    _checkDirs(true, "755", true);
+  }
+
+  @Test (timeout = 30000)
+  public void testCheckDir_notDir_local() throws Throwable {
+    _checkDirs(false, "000", false);
+  }
+
+  @Test (timeout = 30000)
+  public void testCheckDir_notReadable_local() throws Throwable {
+    _checkDirs(true, "000", false);
+  }
+
+  @Test (timeout = 30000)
+  public void testCheckDir_notWritable_local() throws Throwable {
+    _checkDirs(true, "444", false);
+  }
+
+  @Test (timeout = 30000)
+  public void testCheckDir_notListable_local() throws Throwable {
+    _checkDirs(true, "666", false);
+  }
+
+  private void _checkDirs(boolean isDir, String perm, boolean success)
+      throws Throwable {
+    File localDir = File.createTempFile("test", "tmp");
+    if (isDir) {
+      localDir.delete();
+      localDir.mkdir();
+    }
+    Shell.execCommand(Shell.getSetPermissionCommand(perm, false,
+                                                    localDir.getAbsolutePath()));
+    try {
+      DiskChecker.checkDir(localDir);
+      assertTrue("checkDir success", success);
+    } catch (DiskErrorException e) {
+      e.printStackTrace();
+      assertFalse("checkDir success", success);
+    }
+    localDir.delete();
+    System.out.println("checkDir success: " + success);
+
+  }
+}



Mime
View raw message