Skip to content

YARN-9511. Refactoring TestAuxServices to eliminate flakyness #7598

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Apr 17, 2025
Merged

YARN-9511. Refactoring TestAuxServices to eliminate flakyness #7598

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e23f4bd
YARN-9511. Refactoring TestAuxServices to eliminate flakyness
p-szucs Apr 9, 2025
7d93e6b
YARN-9511. Empty commit to trigger build
p-szucs Apr 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions ...src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/AuxServices.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 " +
Expand All @@ -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
Expand Down Expand Up @@ -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.
Expand Down
172 changes: 131 additions & 41 deletions ...test/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/TestAuxServices.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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")
Expand Down Expand Up @@ -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."
Expand All @@ -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();
Expand All @@ -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();
Expand All @@ -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();
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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();
Expand All @@ -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);
Expand All @@ -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 =
Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -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());
Expand All @@ -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 {
Expand Down