This is an automated email from the ASF dual-hosted git repository.
bteke pushed a commit to branch trunk
in repository https://gitbox.apache.org/repos/asf/hadoop.git
The following commit(s) were added to refs/heads/trunk by this push:
new 1f179c29c20 YARN-9511. Refactoring TestAuxServices to eliminate
flakyness (#7598)
1f179c29c20 is described below
commit 1f179c29c206238f7a7c5e0cc1bd146eeec05314
Author: Peter Szucs <116345192+p-sz...@users.noreply.github.com>
AuthorDate: Thu Apr 17 14:30:43 2025 +0200
YARN-9511. Refactoring TestAuxServices to eliminate flakyness (#7598)
---
.../nodemanager/containermanager/AuxServices.java | 9 +-
.../containermanager/TestAuxServices.java | 172 ++++++++++++++++-----
2 files changed, 138 insertions(+), 43 deletions(-)
diff --git
a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/AuxServices.java
b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/AuxServices.java
index 794ef9d9a43..b70d007276a 100644
---
a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/AuxServices.java
+++
b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/AuxServices.java
@@ -516,7 +516,8 @@ public synchronized void reload(AuxServiceRecords services)
throws
loadServices(services, getConfig(), true);
}
- private boolean checkManifestPermissions(FileStatus status) throws
+ @VisibleForTesting
+ boolean checkManifestPermissions(FileStatus status) throws
IOException {
if ((status.getPermission().toShort() & 0022) != 0) {
LOG.error("Manifest file and parents must not be writable by group or " +
@@ -528,7 +529,7 @@ private boolean checkManifestPermissions(FileStatus status)
throws
if (parent == null) {
return true;
}
- return checkManifestPermissions(manifestFS.getFileStatus(parent));
+ return checkManifestPermissions(getManifestFS().getFileStatus(parent));
}
private boolean checkManifestOwnerAndPermissions(FileStatus status) throws
@@ -964,6 +965,10 @@ protected static void setSystemClasses(AuxServiceRecord
service, String
service.getConfiguration().setProperty(SYSTEM_CLASSES, systemClasses);
}
+ protected FileSystem getManifestFS() {
+ return manifestFS;
+ }
+
/**
* Class which is used by the {@link Timer} class to periodically execute the
* manifest reload.
diff --git
a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/test/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/TestAuxServices.java
b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/test/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/TestAuxServices.java
index 79a3ec482b5..cf59bcaed5c 100644
---
a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/test/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/TestAuxServices.java
+++
b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/test/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/TestAuxServices.java
@@ -28,7 +28,9 @@
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assertions.fail;
import static org.junit.jupiter.api.Assumptions.assumeTrue;
+import static org.mockito.Mockito.doReturn;
import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.spy;
import static org.mockito.Mockito.when;
import static org.mockito.ArgumentMatchers.anyString;
import static org.mockito.ArgumentMatchers.any;
@@ -122,6 +124,8 @@ public class TestAuxServices {
.getSimpleName());
private File manifest = new File(rootDir, "manifest.txt");
private ObjectMapper mapper = new ObjectMapper();
+ private static final FsPermission WRITABLE_BY_OWNER =
FsPermission.createImmutable((short) 0755);
+ private static final FsPermission WRITABLE_BY_GROUP =
FsPermission.createImmutable((short) 0775);
public static Collection<Boolean> getParams() {
return Arrays.asList(false, true);
@@ -266,6 +270,24 @@ private void writeManifestFile(AuxServiceRecords services,
Configuration
mapper.writeValue(manifest, services);
}
+ /**
+ * Creates a spy object of AuxServices for test cases which assume that we
have proper
+ * file system permissions by default.
+ *
+ * Permission checking iterates through the parents of the manifest file
until it
+ * reaches the system root, so without mocking this the success of the
initialization
+ * would heavily depend on the environment where the test is running.
+ *
+ * @return a spy object of AuxServices
+ */
+ private AuxServices getSpyAuxServices(AuxiliaryLocalPathHandler
auxiliaryLocalPathHandler,
+ Context nmContext, DeletionService deletionService) throws IOException {
+ AuxServices auxServices = spy(new AuxServices(auxiliaryLocalPathHandler,
+ nmContext, deletionService));
+
doReturn(true).when(auxServices).checkManifestPermissions(any(FileStatus.class));
+ return auxServices;
+ }
+
@SuppressWarnings("resource")
@ParameterizedTest
@MethodSource("getParams")
@@ -317,7 +339,7 @@ public void testRemoteAuxServiceClassPath(boolean
pUseManifest) throws Exception
YarnConfiguration.NM_AUX_SERVICE_REMOTE_CLASSPATH, "ServiceC"),
testJar.getAbsolutePath());
}
- aux = new AuxServices(MOCK_AUX_PATH_HANDLER,
+ aux = getSpyAuxServices(MOCK_AUX_PATH_HANDLER,
mockContext2, mockDelService2);
aux.init(conf);
fail("The permission of the jar is wrong."
@@ -339,7 +361,7 @@ public void testRemoteAuxServiceClassPath(boolean
pUseManifest) throws Exception
YarnConfiguration.NM_AUX_SERVICE_REMOTE_CLASSPATH, "ServiceC"),
testJar.getAbsolutePath());
}
- aux = new AuxServices(MOCK_AUX_PATH_HANDLER,
+ aux = getSpyAuxServices(MOCK_AUX_PATH_HANDLER,
mockContext2, mockDelService2);
aux.init(conf);
aux.start();
@@ -356,7 +378,7 @@ public void testRemoteAuxServiceClassPath(boolean
pUseManifest) throws Exception
// initialize the same auxservice again, and make sure that we did not
// re-download the jar from remote directory.
- aux = new AuxServices(MOCK_AUX_PATH_HANDLER,
+ aux = getSpyAuxServices(MOCK_AUX_PATH_HANDLER,
mockContext2, mockDelService2);
aux.init(conf);
aux.start();
@@ -377,7 +399,7 @@ public void testRemoteAuxServiceClassPath(boolean
pUseManifest) throws Exception
FileTime fileTime = FileTime.fromMillis(time);
Files.setLastModifiedTime(Paths.get(testJar.getAbsolutePath()),
fileTime);
- aux = new AuxServices(MOCK_AUX_PATH_HANDLER,
+ aux = getSpyAuxServices(MOCK_AUX_PATH_HANDLER,
mockContext2, mockDelService2);
aux.init(conf);
aux.start();
@@ -419,7 +441,7 @@ public void testCustomizedAuxServiceClassPath(boolean
pUseManifest) throws Excep
"ServiceC"), ServiceC.class, Service.class);
}
@SuppressWarnings("resource")
- AuxServices aux = new AuxServices(MOCK_AUX_PATH_HANDLER,
+ AuxServices aux = getSpyAuxServices(MOCK_AUX_PATH_HANDLER,
MOCK_CONTEXT, MOCK_DEL_SERVICE);
aux.init(conf);
aux.start();
@@ -472,7 +494,7 @@ public void testCustomizedAuxServiceClassPath(boolean
pUseManifest) throws Excep
when(mockDirsHandler.getLocalPathForWrite(anyString())).thenReturn(
rootAuxServiceDirPath);
when(mockContext2.getLocalDirsHandler()).thenReturn(mockDirsHandler);
- aux = new AuxServices(MOCK_AUX_PATH_HANDLER,
+ aux = getSpyAuxServices(MOCK_AUX_PATH_HANDLER,
mockContext2, MOCK_DEL_SERVICE);
aux.init(conf);
aux.start();
@@ -654,7 +676,7 @@ private Configuration getABConf(String aName, String bName,
public void testAuxServices(boolean pUseManifest) throws IOException {
initTestAuxServices(pUseManifest);
Configuration conf = getABConf();
- final AuxServices aux = new AuxServices(MOCK_AUX_PATH_HANDLER,
+ final AuxServices aux = getSpyAuxServices(MOCK_AUX_PATH_HANDLER,
MOCK_CONTEXT, MOCK_DEL_SERVICE);
aux.init(conf);
@@ -684,7 +706,7 @@ public void testAuxServices(boolean pUseManifest) throws
IOException {
public void testAuxServicesMeta(boolean pUseManifest) throws IOException {
initTestAuxServices(pUseManifest);
Configuration conf = getABConf();
- final AuxServices aux = new AuxServices(MOCK_AUX_PATH_HANDLER,
+ final AuxServices aux = getSpyAuxServices(MOCK_AUX_PATH_HANDLER,
MOCK_CONTEXT, MOCK_DEL_SERVICE);
aux.init(conf);
@@ -718,7 +740,7 @@ public void testAuxUnexpectedStop(boolean pUseManifest)
throws IOException {
initTestAuxServices(pUseManifest);
// AuxServices no longer expected to stop when services stop
Configuration conf = getABConf();
- final AuxServices aux = new AuxServices(MOCK_AUX_PATH_HANDLER,
+ final AuxServices aux = getSpyAuxServices(MOCK_AUX_PATH_HANDLER,
MOCK_CONTEXT, MOCK_DEL_SERVICE);
aux.init(conf);
aux.start();
@@ -736,7 +758,7 @@ public void testValidAuxServiceName(boolean pUseManifest)
throws IOException {
initTestAuxServices(pUseManifest);
Configuration conf = getABConf("Asrv1", "Bsrv_2", ServiceA.class,
ServiceB.class);
- final AuxServices aux = new AuxServices(MOCK_AUX_PATH_HANDLER,
+ final AuxServices aux = getSpyAuxServices(MOCK_AUX_PATH_HANDLER,
MOCK_CONTEXT, MOCK_DEL_SERVICE);
try {
aux.init(conf);
@@ -745,7 +767,7 @@ public void testValidAuxServiceName(boolean pUseManifest)
throws IOException {
}
//Test bad auxService Name
- final AuxServices aux1 = new AuxServices(MOCK_AUX_PATH_HANDLER,
+ final AuxServices aux1 = getSpyAuxServices(MOCK_AUX_PATH_HANDLER,
MOCK_CONTEXT, MOCK_DEL_SERVICE);
if (useManifest) {
AuxServiceRecord serviceA =
@@ -776,7 +798,7 @@ public void testAuxServiceRecoverySetup(boolean
pUseManifest) throws IOException
conf.setBoolean(YarnConfiguration.NM_RECOVERY_ENABLED, true);
conf.set(YarnConfiguration.NM_RECOVERY_DIR, TEST_DIR.toString());
try {
- final AuxServices aux = new AuxServices(MOCK_AUX_PATH_HANDLER,
+ final AuxServices aux = getSpyAuxServices(MOCK_AUX_PATH_HANDLER,
MOCK_CONTEXT, MOCK_DEL_SERVICE);
aux.init(conf);
assertEquals(2, aux.getServices().size());
@@ -906,62 +928,130 @@ public void testAuxServicesConfChange(boolean
pUseManifest) throws IOException {
@ParameterizedTest
@MethodSource("getParams")
- public void testAuxServicesManifestPermissions(boolean pUseManifest) throws
IOException {
+ public void testAuxServicesInitWithManifestOwnerAndPermissionCheck(boolean
pUseManifest)
+ throws IOException {
initTestAuxServices(pUseManifest);
assumeTrue(useManifest);
Configuration conf = getABConf();
- FileSystem fs = FileSystem.get(conf);
- fs.setPermission(new Path(manifest.getAbsolutePath()), FsPermission
- .createImmutable((short) 0777));
- AuxServices aux = new AuxServices(MOCK_AUX_PATH_HANDLER,
- MOCK_CONTEXT, MOCK_DEL_SERVICE);
+ AuxServices aux = spy(new AuxServices(MOCK_AUX_PATH_HANDLER,
+ MOCK_CONTEXT, MOCK_DEL_SERVICE));
+ doReturn(false).when(aux).checkManifestPermissions(any(FileStatus.class));
aux.init(conf);
assertEquals(0, aux.getServices().size());
- fs.setPermission(new Path(manifest.getAbsolutePath()), FsPermission
- .createImmutable((short) 0775));
- aux = new AuxServices(MOCK_AUX_PATH_HANDLER,
- MOCK_CONTEXT, MOCK_DEL_SERVICE);
- aux.init(conf);
- assertEquals(0, aux.getServices().size());
-
- fs.setPermission(new Path(manifest.getAbsolutePath()), FsPermission
- .createImmutable((short) 0755));
- fs.setPermission(new Path(rootDir.getAbsolutePath()), FsPermission
- .createImmutable((short) 0775));
- aux = new AuxServices(MOCK_AUX_PATH_HANDLER,
- MOCK_CONTEXT, MOCK_DEL_SERVICE);
- aux.init(conf);
- assertEquals(0, aux.getServices().size());
-
- fs.setPermission(new Path(rootDir.getAbsolutePath()), FsPermission
- .createImmutable((short) 0755));
- aux = new AuxServices(MOCK_AUX_PATH_HANDLER,
+ aux = getSpyAuxServices(MOCK_AUX_PATH_HANDLER,
MOCK_CONTEXT, MOCK_DEL_SERVICE);
aux.init(conf);
assertEquals(2, aux.getServices().size());
conf.set(YarnConfiguration.YARN_ADMIN_ACL, "");
- aux = new AuxServices(MOCK_AUX_PATH_HANDLER,
+ aux = getSpyAuxServices(MOCK_AUX_PATH_HANDLER,
MOCK_CONTEXT, MOCK_DEL_SERVICE);
aux.init(conf);
assertEquals(0, aux.getServices().size());
conf.set(YarnConfiguration.YARN_ADMIN_ACL, UserGroupInformation
.getCurrentUser().getShortUserName());
- aux = new AuxServices(MOCK_AUX_PATH_HANDLER,
+ aux = getSpyAuxServices(MOCK_AUX_PATH_HANDLER,
MOCK_CONTEXT, MOCK_DEL_SERVICE);
aux.init(conf);
assertEquals(2, aux.getServices().size());
}
+ @ParameterizedTest
+ @MethodSource("getParams")
+ public void
testCheckManifestPermissionsWhenFileIsOnlyWritableByOwner(boolean pUseManifest)
+ throws IOException {
+ initTestAuxServices(pUseManifest);
+ assumeTrue(useManifest);
+ final AuxServices aux = spy(new AuxServices(MOCK_AUX_PATH_HANDLER,
+ MOCK_CONTEXT, MOCK_DEL_SERVICE));
+ FileStatus manifestFileStatus = mock(FileStatus.class);
+ Path manifestPath = mock(Path.class);
+
+ when(manifestFileStatus.getPermission()).thenReturn(WRITABLE_BY_OWNER);
+ when(manifestFileStatus.getPath()).thenReturn(manifestPath);
+
+ assertTrue(aux.checkManifestPermissions(manifestFileStatus));
+ }
+
+ @ParameterizedTest
+ @MethodSource("getParams")
+ public void testCheckManifestPermissionsWhenFileIsWritableByGroup(boolean
pUseManifest)
+ throws IOException {
+ initTestAuxServices(pUseManifest);
+ assumeTrue(useManifest);
+ final AuxServices aux = spy(new AuxServices(MOCK_AUX_PATH_HANDLER,
+ MOCK_CONTEXT, MOCK_DEL_SERVICE));
+ FileStatus manifestFileStatus = mock(FileStatus.class);
+ Path manifestPath = mock(Path.class);
+
+ when(manifestFileStatus.getPermission()).thenReturn(WRITABLE_BY_GROUP);
+ when(manifestFileStatus.getPath()).thenReturn(manifestPath);
+
+ assertFalse(aux.checkManifestPermissions(manifestFileStatus));
+ }
+
+ @ParameterizedTest
+ @MethodSource("getParams")
+ public void testCheckManifestPermissionsWhenParentIsWritableByGroup(boolean
pUseManifest)
+ throws IOException {
+ initTestAuxServices(pUseManifest);
+ assumeTrue(useManifest);
+ final AuxServices aux = spy(new AuxServices(MOCK_AUX_PATH_HANDLER,
+ MOCK_CONTEXT, MOCK_DEL_SERVICE));
+
+ FileStatus manifestFileStatus = mock(FileStatus.class);
+ FileStatus parentFolderStatus = mock(FileStatus.class);
+ when(manifestFileStatus.getPermission()).thenReturn(WRITABLE_BY_OWNER);
+ when(parentFolderStatus.getPermission()).thenReturn(WRITABLE_BY_GROUP);
+
+ Path manifestPath = mock(Path.class);
+ Path parentPath = mock(Path.class);
+ when(manifestFileStatus.getPath()).thenReturn(manifestPath);
+ when(manifestPath.getParent()).thenReturn(parentPath);
+
+ FileSystem manifestFs = mock(FileSystem.class);
+ when(manifestFs.getFileStatus(parentPath)).thenReturn(parentFolderStatus);
+ doReturn(manifestFs).when(aux).getManifestFS();
+
+ assertFalse(aux.checkManifestPermissions(manifestFileStatus));
+ }
+
+ @ParameterizedTest
+ @MethodSource("getParams")
+ public void
testCheckManifestPermissionsWhenParentAndFileIsWritableByOwner(boolean
pUseManifest)
+ throws IOException {
+ initTestAuxServices(pUseManifest);
+ assumeTrue(useManifest);
+ final AuxServices aux = spy(new AuxServices(MOCK_AUX_PATH_HANDLER,
+ MOCK_CONTEXT, MOCK_DEL_SERVICE));
+
+ FileStatus manifestFileStatus = mock(FileStatus.class);
+ FileStatus parentFolderStatus = mock(FileStatus.class);
+ when(manifestFileStatus.getPermission()).thenReturn(WRITABLE_BY_OWNER);
+ when(parentFolderStatus.getPermission()).thenReturn(WRITABLE_BY_OWNER);
+
+ Path manifestPath = mock(Path.class);
+ Path parentPath = mock(Path.class);
+ when(manifestFileStatus.getPath()).thenReturn(manifestPath);
+ when(parentFolderStatus.getPath()).thenReturn(parentPath);
+ when(manifestPath.getParent()).thenReturn(parentPath);
+
+ FileSystem manifestFs = mock(FileSystem.class);
+ when(manifestFs.getFileStatus(parentPath)).thenReturn(parentFolderStatus);
+ doReturn(manifestFs).when(aux).getManifestFS();
+
+ assertTrue(aux.checkManifestPermissions(manifestFileStatus));
+ }
+
@ParameterizedTest
@MethodSource("getParams")
public void testRemoveManifest(boolean pUseManifest) throws IOException {
initTestAuxServices(pUseManifest);
assumeTrue(useManifest);
Configuration conf = getABConf();
- final AuxServices aux = new AuxServices(MOCK_AUX_PATH_HANDLER,
+ final AuxServices aux = getSpyAuxServices(MOCK_AUX_PATH_HANDLER,
MOCK_CONTEXT, MOCK_DEL_SERVICE);
aux.init(conf);
assertEquals(2, aux.getServices().size());
@@ -976,7 +1066,7 @@ public void testManualReload(boolean pUseManifest) throws
IOException {
initTestAuxServices(pUseManifest);
assumeTrue(useManifest);
Configuration conf = getABConf();
- final AuxServices aux = new AuxServices(MOCK_AUX_PATH_HANDLER,
+ final AuxServices aux = getSpyAuxServices(MOCK_AUX_PATH_HANDLER,
MOCK_CONTEXT, MOCK_DEL_SERVICE);
aux.init(conf);
try {
---------------------------------------------------------------------
To unsubscribe, e-mail: common-commits-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-commits-h...@hadoop.apache.org
