This is an automated email from the ASF dual-hosted git repository. srowen pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/spark.git
The following commit(s) were added to refs/heads/master by this push: new 317407171cb [SPARK-37618][CORE][FOLLOWUP] Support cleaning up shuffle blocks from external shuffle service 317407171cb is described below commit 317407171cb36439c371153cfd45c1482bf5e425 Author: Mridul Muralidharan <mridulatgmail.com> AuthorDate: Sun May 8 08:11:19 2022 -0500 [SPARK-37618][CORE][FOLLOWUP] Support cleaning up shuffle blocks from external shuffle service ### What changes were proposed in this pull request? Fix test failure in build. Depending on the umask of the process running tests (which is typically inherited from the user's default umask), the group writable bit for the files/directories could be set or unset. The test was assuming that by default the umask will be restrictive (and so files/directories wont be group writable). Since this is not a valid assumption, we use jnr to change the umask of the process to be more restrictive - so that the test can validate the behavior change - and reset it back once [...] ### Why are the changes needed? Fix test failure in build ### Does this PR introduce _any_ user-facing change? No Adds jnr as a test scoped dependency, which does not bring in any other new dependency (asm is already a dep in spark). ``` [INFO] +- com.github.jnr:jnr-posix:jar:3.0.9:test [INFO] | +- com.github.jnr:jnr-ffi:jar:2.0.1:test [INFO] | | +- com.github.jnr:jffi:jar:1.2.7:test [INFO] | | +- com.github.jnr:jffi:jar:native:1.2.7:test [INFO] | | +- org.ow2.asm:asm:jar:5.0.3:test [INFO] | | +- org.ow2.asm:asm-commons:jar:5.0.3:test [INFO] | | +- org.ow2.asm:asm-analysis:jar:5.0.3:test [INFO] | | +- org.ow2.asm:asm-tree:jar:5.0.3:test [INFO] | | +- org.ow2.asm:asm-util:jar:5.0.3:test [INFO] | | \- com.github.jnr:jnr-x86asm:jar:1.0.2:test [INFO] | \- com.github.jnr:jnr-constants:jar:0.8.6:test ``` ### How was this patch tested? Modification to existing test. Tested on Linux, skips test when native posix env is not found. Closes #36473 from mridulm/fix-SPARK-37618-test. Authored-by: Mridul Muralidharan <mridulatgmail.com> Signed-off-by: Sean Owen <sro...@gmail.com> --- LICENSE-binary | 1 + core/pom.xml | 5 ++ .../spark/storage/DiskBlockManagerSuite.scala | 53 +++++++++++++++------- pom.xml | 6 +++ 4 files changed, 48 insertions(+), 17 deletions(-) diff --git a/LICENSE-binary b/LICENSE-binary index a090e5205cc..e8ad55c9354 100644 --- a/LICENSE-binary +++ b/LICENSE-binary @@ -546,6 +546,7 @@ jakarta.annotation:jakarta-annotation-api https://projects.eclipse.org/projects/ jakarta.servlet:jakarta.servlet-api https://projects.eclipse.org/projects/ee4j.servlet jakarta.ws.rs:jakarta.ws.rs-api https://github.com/eclipse-ee4j/jaxrs-api org.glassfish.hk2.external:jakarta.inject +com.github.jnr:jnr-posix Public Domain diff --git a/core/pom.xml b/core/pom.xml index 7d4bab55659..80578417b05 100644 --- a/core/pom.xml +++ b/core/pom.xml @@ -61,6 +61,11 @@ <groupId>com.twitter</groupId> <artifactId>chill-java</artifactId> </dependency> + <dependency> + <groupId>com.github.jnr</groupId> + <artifactId>jnr-posix</artifactId> + <scope>test</scope> + </dependency> <dependency> <groupId>org.apache.xbean</groupId> <artifactId>xbean-asm9-shaded</artifactId> diff --git a/core/src/test/scala/org/apache/spark/storage/DiskBlockManagerSuite.scala b/core/src/test/scala/org/apache/spark/storage/DiskBlockManagerSuite.scala index 58fe40f9ade..3e4002614ca 100644 --- a/core/src/test/scala/org/apache/spark/storage/DiskBlockManagerSuite.scala +++ b/core/src/test/scala/org/apache/spark/storage/DiskBlockManagerSuite.scala @@ -24,6 +24,7 @@ import java.util.HashMap import com.fasterxml.jackson.core.`type`.TypeReference import com.fasterxml.jackson.databind.ObjectMapper +import jnr.posix.{POSIX, POSIXFactory} import org.apache.commons.io.FileUtils import org.scalatest.{BeforeAndAfterAll, BeforeAndAfterEach} @@ -141,28 +142,46 @@ class DiskBlockManagerSuite extends SparkFunSuite with BeforeAndAfterEach with B assert(attemptId.equals("1")) } + // Use jnr to get and override the current process umask. + // Expects the input mask to be an octal number + private def getAndSetUmask(posix: POSIX, mask: String): String = { + val prev = posix.umask(BigInt(mask, 8).toInt) + "0" + "%o".format(prev) + } + test("SPARK-37618: Sub dirs are group writable when removing from shuffle service enabled") { val conf = testConf.clone conf.set("spark.local.dir", rootDirs) conf.set("spark.shuffle.service.enabled", "true") conf.set("spark.shuffle.service.removeShuffle", "false") - val diskBlockManager = new DiskBlockManager(conf, deleteFilesOnStop = true, isDriver = false) - val blockId = new TestBlockId("test") - val newFile = diskBlockManager.getFile(blockId) - val parentDir = newFile.getParentFile() - assert(parentDir.exists && parentDir.isDirectory) - val permission = Files.getPosixFilePermissions(parentDir.toPath) - assert(!permission.contains(PosixFilePermission.GROUP_WRITE)) - - assert(parentDir.delete()) - - conf.set("spark.shuffle.service.removeShuffle", "true") - val diskBlockManager2 = new DiskBlockManager(conf, deleteFilesOnStop = true, isDriver = false) - val newFile2 = diskBlockManager2.getFile(blockId) - val parentDir2 = newFile2.getParentFile() - assert(parentDir2.exists && parentDir2.isDirectory) - val permission2 = Files.getPosixFilePermissions(parentDir2.toPath) - assert(permission2.contains(PosixFilePermission.GROUP_WRITE)) + val posix = POSIXFactory.getPOSIX + + assume(posix.isNative, "Skipping test for SPARK-37618, native posix support not found") + + val oldUmask = getAndSetUmask(posix, "077") + try { + val diskBlockManager = new DiskBlockManager(conf, deleteFilesOnStop = true, + isDriver = false) + val blockId = new TestBlockId("test") + val newFile = diskBlockManager.getFile(blockId) + val parentDir = newFile.getParentFile() + assert(parentDir.exists && parentDir.isDirectory) + val permission = Files.getPosixFilePermissions(parentDir.toPath) + assert(!permission.contains(PosixFilePermission.GROUP_WRITE)) + + assert(parentDir.delete()) + + conf.set("spark.shuffle.service.removeShuffle", "true") + val diskBlockManager2 = new DiskBlockManager(conf, deleteFilesOnStop = true, + isDriver = false) + val newFile2 = diskBlockManager2.getFile(blockId) + val parentDir2 = newFile2.getParentFile() + assert(parentDir2.exists && parentDir2.isDirectory) + val permission2 = Files.getPosixFilePermissions(parentDir2.toPath) + assert(permission2.contains(PosixFilePermission.GROUP_WRITE)) + } finally { + getAndSetUmask(posix, oldUmask) + } } def writeToFile(file: File, numBytes: Int): Unit = { diff --git a/pom.xml b/pom.xml index 319d12a07ee..67efb7f7cb0 100644 --- a/pom.xml +++ b/pom.xml @@ -457,6 +457,12 @@ <artifactId>chill-java</artifactId> <version>${chill.version}</version> </dependency> + <dependency> + <groupId>com.github.jnr</groupId> + <artifactId>jnr-posix</artifactId> + <version>3.1.15</version> + <scope>test</scope> + </dependency> <!-- This artifact is a shaded version of ASM 9.x. The POM that was used to produce this is at https://github.com/apache/geronimo-xbean/tree/trunk/xbean-asm9-shaded For context on why we shade ASM, see SPARK-782 and SPARK-6152. --> --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@spark.apache.org For additional commands, e-mail: commits-h...@spark.apache.org