Skip to content

Commit 044d607

Browse files
p-szucssteveloughran
p-szucs
authored and
steveloughran
committed
CDPD-81760. YARN-9511. Refactoring TestAuxServices to eliminate flakyness (apache#7598)
(cherry picked from commit 1f179c2) Change-Id: I2f1c1290bb0f72851f72f580f9655c86aeb6d5e7
1 parent 6e416ba commit 044d607

File tree

2 files changed

+125
-43
lines changed
  • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src

2 files changed

+125
-43
lines changed

‎hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/AuxServices.java‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -516,7 +516,8 @@ public synchronized void reload(AuxServiceRecords services) throws
516516
loadServices(services, getConfig(), true);
517517
}
518518

519-
private boolean checkManifestPermissions(FileStatus status) throws
519+
@VisibleForTesting
520+
boolean checkManifestPermissions(FileStatus status) throws
520521
IOException {
521522
if ((status.getPermission().toShort() & 0022) != 0) {
522523
LOG.error("Manifest file and parents must not be writable by group or " +
@@ -528,7 +529,7 @@ private boolean checkManifestPermissions(FileStatus status) throws
528529
if (parent == null) {
529530
return true;
530531
}
531-
return checkManifestPermissions(manifestFS.getFileStatus(parent));
532+
return checkManifestPermissions(getManifestFS().getFileStatus(parent));
532533
}
533534

534535
private boolean checkManifestOwnerAndPermissions(FileStatus status) throws
@@ -964,6 +965,10 @@ protected static void setSystemClasses(AuxServiceRecord service, String
964965
service.getConfiguration().setProperty(SYSTEM_CLASSES, systemClasses);
965966
}
966967

968+
protected FileSystem getManifestFS() {
969+
return manifestFS;
970+
}
971+
967972
/**
968973
* Class which is used by the {@link Timer} class to periodically execute the
969974
* manifest reload.

‎hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/test/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/TestAuxServices.java‎

Lines changed: 118 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,9 @@
2626
import static org.junit.Assert.assertNull;
2727
import static org.junit.Assert.assertTrue;
2828
import static org.junit.Assert.fail;
29+
import static org.mockito.Mockito.doReturn;
2930
import static org.mockito.Mockito.mock;
31+
import static org.mockito.Mockito.spy;
3032
import static org.mockito.Mockito.when;
3133
import static org.mockito.ArgumentMatchers.anyString;
3234
import static org.mockito.ArgumentMatchers.any;
@@ -123,6 +125,8 @@ public class TestAuxServices {
123125
.getSimpleName());
124126
private File manifest = new File(rootDir, "manifest.txt");
125127
private ObjectMapper mapper = new ObjectMapper();
128+
private static final FsPermission WRITABLE_BY_OWNER = FsPermission.createImmutable((short) 0755);
129+
private static final FsPermission WRITABLE_BY_GROUP = FsPermission.createImmutable((short) 0775);
126130

127131
@Parameterized.Parameters
128132
public static Collection<Boolean> getParams() {
@@ -268,6 +272,24 @@ private void writeManifestFile(AuxServiceRecords services, Configuration
268272
mapper.writeValue(manifest, services);
269273
}
270274

275+
/**
276+
* Creates a spy object of AuxServices for test cases which assume that we have proper
277+
* file system permissions by default.
278+
*
279+
* Permission checking iterates through the parents of the manifest file until it
280+
* reaches the system root, so without mocking this the success of the initialization
281+
* would heavily depend on the environment where the test is running.
282+
*
283+
* @return a spy object of AuxServices
284+
*/
285+
private AuxServices getSpyAuxServices(AuxiliaryLocalPathHandler auxiliaryLocalPathHandler,
286+
Context nmContext, DeletionService deletionService) throws IOException {
287+
AuxServices auxServices = spy(new AuxServices(auxiliaryLocalPathHandler,
288+
nmContext, deletionService));
289+
doReturn(true).when(auxServices).checkManifestPermissions(any(FileStatus.class));
290+
return auxServices;
291+
}
292+
271293
@SuppressWarnings("resource")
272294
@Test
273295
public void testRemoteAuxServiceClassPath() throws Exception {
@@ -317,7 +339,7 @@ public void testRemoteAuxServiceClassPath() throws Exception {
317339
YarnConfiguration.NM_AUX_SERVICE_REMOTE_CLASSPATH, "ServiceC"),
318340
testJar.getAbsolutePath());
319341
}
320-
aux = new AuxServices(MOCK_AUX_PATH_HANDLER,
342+
aux = getSpyAuxServices(MOCK_AUX_PATH_HANDLER,
321343
mockContext2, mockDelService2);
322344
aux.init(conf);
323345
Assert.fail("The permission of the jar is wrong."
@@ -339,7 +361,7 @@ public void testRemoteAuxServiceClassPath() throws Exception {
339361
YarnConfiguration.NM_AUX_SERVICE_REMOTE_CLASSPATH, "ServiceC"),
340362
testJar.getAbsolutePath());
341363
}
342-
aux = new AuxServices(MOCK_AUX_PATH_HANDLER,
364+
aux = getSpyAuxServices(MOCK_AUX_PATH_HANDLER,
343365
mockContext2, mockDelService2);
344366
aux.init(conf);
345367
aux.start();
@@ -356,7 +378,7 @@ public void testRemoteAuxServiceClassPath() throws Exception {
356378

357379
// initialize the same auxservice again, and make sure that we did not
358380
// re-download the jar from remote directory.
359-
aux = new AuxServices(MOCK_AUX_PATH_HANDLER,
381+
aux = getSpyAuxServices(MOCK_AUX_PATH_HANDLER,
360382
mockContext2, mockDelService2);
361383
aux.init(conf);
362384
aux.start();
@@ -377,7 +399,7 @@ public void testRemoteAuxServiceClassPath() throws Exception {
377399
FileTime fileTime = FileTime.fromMillis(time);
378400
Files.setLastModifiedTime(Paths.get(testJar.getAbsolutePath()),
379401
fileTime);
380-
aux = new AuxServices(MOCK_AUX_PATH_HANDLER,
402+
aux = getSpyAuxServices(MOCK_AUX_PATH_HANDLER,
381403
mockContext2, mockDelService2);
382404
aux.init(conf);
383405
aux.start();
@@ -416,7 +438,7 @@ public void testCustomizedAuxServiceClassPath() throws Exception {
416438
"ServiceC"), ServiceC.class, Service.class);
417439
}
418440
@SuppressWarnings("resource")
419-
AuxServices aux = new AuxServices(MOCK_AUX_PATH_HANDLER,
441+
AuxServices aux = getSpyAuxServices(MOCK_AUX_PATH_HANDLER,
420442
MOCK_CONTEXT, MOCK_DEL_SERVICE);
421443
aux.init(conf);
422444
aux.start();
@@ -469,7 +491,7 @@ public void testCustomizedAuxServiceClassPath() throws Exception {
469491
when(mockDirsHandler.getLocalPathForWrite(anyString())).thenReturn(
470492
rootAuxServiceDirPath);
471493
when(mockContext2.getLocalDirsHandler()).thenReturn(mockDirsHandler);
472-
aux = new AuxServices(MOCK_AUX_PATH_HANDLER,
494+
aux = getSpyAuxServices(MOCK_AUX_PATH_HANDLER,
473495
mockContext2, MOCK_DEL_SERVICE);
474496
aux.init(conf);
475497
aux.start();
@@ -645,7 +667,7 @@ private Configuration getABConf(String aName, String bName,
645667
@Test
646668
public void testAuxServices() throws IOException {
647669
Configuration conf = getABConf();
648-
final AuxServices aux = new AuxServices(MOCK_AUX_PATH_HANDLER,
670+
final AuxServices aux = getSpyAuxServices(MOCK_AUX_PATH_HANDLER,
649671
MOCK_CONTEXT, MOCK_DEL_SERVICE);
650672
aux.init(conf);
651673

@@ -673,7 +695,7 @@ public void testAuxServices() throws IOException {
673695
@Test
674696
public void testAuxServicesMeta() throws IOException {
675697
Configuration conf = getABConf();
676-
final AuxServices aux = new AuxServices(MOCK_AUX_PATH_HANDLER,
698+
final AuxServices aux = getSpyAuxServices(MOCK_AUX_PATH_HANDLER,
677699
MOCK_CONTEXT, MOCK_DEL_SERVICE);
678700
aux.init(conf);
679701

@@ -705,7 +727,7 @@ public void testAuxServicesMeta() throws IOException {
705727
public void testAuxUnexpectedStop() throws IOException {
706728
// AuxServices no longer expected to stop when services stop
707729
Configuration conf = getABConf();
708-
final AuxServices aux = new AuxServices(MOCK_AUX_PATH_HANDLER,
730+
final AuxServices aux = getSpyAuxServices(MOCK_AUX_PATH_HANDLER,
709731
MOCK_CONTEXT, MOCK_DEL_SERVICE);
710732
aux.init(conf);
711733
aux.start();
@@ -721,7 +743,7 @@ public void testAuxUnexpectedStop() throws IOException {
721743
public void testValidAuxServiceName() throws IOException {
722744
Configuration conf = getABConf("Asrv1", "Bsrv_2", ServiceA.class,
723745
ServiceB.class);
724-
final AuxServices aux = new AuxServices(MOCK_AUX_PATH_HANDLER,
746+
final AuxServices aux = getSpyAuxServices(MOCK_AUX_PATH_HANDLER,
725747
MOCK_CONTEXT, MOCK_DEL_SERVICE);
726748
try {
727749
aux.init(conf);
@@ -730,7 +752,7 @@ public void testValidAuxServiceName() throws IOException {
730752
}
731753

732754
//Test bad auxService Name
733-
final AuxServices aux1 = new AuxServices(MOCK_AUX_PATH_HANDLER,
755+
final AuxServices aux1 = getSpyAuxServices(MOCK_AUX_PATH_HANDLER,
734756
MOCK_CONTEXT, MOCK_DEL_SERVICE);
735757
if (useManifest) {
736758
AuxServiceRecord serviceA =
@@ -760,7 +782,7 @@ public void testAuxServiceRecoverySetup() throws IOException {
760782
conf.setBoolean(YarnConfiguration.NM_RECOVERY_ENABLED, true);
761783
conf.set(YarnConfiguration.NM_RECOVERY_DIR, TEST_DIR.toString());
762784
try {
763-
final AuxServices aux = new AuxServices(MOCK_AUX_PATH_HANDLER,
785+
final AuxServices aux = getSpyAuxServices(MOCK_AUX_PATH_HANDLER,
764786
MOCK_CONTEXT, MOCK_DEL_SERVICE);
765787
aux.init(conf);
766788
Assert.assertEquals(2, aux.getServices().size());
@@ -888,59 +910,114 @@ public void testAuxServicesConfChange() throws IOException {
888910
}
889911

890912
@Test
891-
public void testAuxServicesManifestPermissions() throws IOException {
913+
public void testAuxServicesInitWithManifestOwnerAndPermissionCheck() throws IOException {
892914
Assume.assumeTrue(useManifest);
893915
Configuration conf = getABConf();
894-
FileSystem fs = FileSystem.get(conf);
895-
fs.setPermission(new Path(manifest.getAbsolutePath()), FsPermission
896-
.createImmutable((short) 0777));
897-
AuxServices aux = new AuxServices(MOCK_AUX_PATH_HANDLER,
898-
MOCK_CONTEXT, MOCK_DEL_SERVICE);
899-
aux.init(conf);
900-
assertEquals(0, aux.getServices().size());
901-
902-
fs.setPermission(new Path(manifest.getAbsolutePath()), FsPermission
903-
.createImmutable((short) 0775));
904-
aux = new AuxServices(MOCK_AUX_PATH_HANDLER,
905-
MOCK_CONTEXT, MOCK_DEL_SERVICE);
916+
AuxServices aux = spy(new AuxServices(MOCK_AUX_PATH_HANDLER,
917+
MOCK_CONTEXT, MOCK_DEL_SERVICE));
918+
doReturn(false).when(aux).checkManifestPermissions(any(FileStatus.class));
906919
aux.init(conf);
907920
assertEquals(0, aux.getServices().size());
908921

909-
fs.setPermission(new Path(manifest.getAbsolutePath()), FsPermission
910-
.createImmutable((short) 0755));
911-
fs.setPermission(new Path(rootDir.getAbsolutePath()), FsPermission
912-
.createImmutable((short) 0775));
913-
aux = new AuxServices(MOCK_AUX_PATH_HANDLER,
914-
MOCK_CONTEXT, MOCK_DEL_SERVICE);
915-
aux.init(conf);
916-
assertEquals(0, aux.getServices().size());
917-
918-
fs.setPermission(new Path(rootDir.getAbsolutePath()), FsPermission
919-
.createImmutable((short) 0755));
920-
aux = new AuxServices(MOCK_AUX_PATH_HANDLER,
922+
aux = getSpyAuxServices(MOCK_AUX_PATH_HANDLER,
921923
MOCK_CONTEXT, MOCK_DEL_SERVICE);
922924
aux.init(conf);
923925
assertEquals(2, aux.getServices().size());
924926

925927
conf.set(YarnConfiguration.YARN_ADMIN_ACL, "");
926-
aux = new AuxServices(MOCK_AUX_PATH_HANDLER,
928+
aux = getSpyAuxServices(MOCK_AUX_PATH_HANDLER,
927929
MOCK_CONTEXT, MOCK_DEL_SERVICE);
928930
aux.init(conf);
929931
assertEquals(0, aux.getServices().size());
930932

931933
conf.set(YarnConfiguration.YARN_ADMIN_ACL, UserGroupInformation
932934
.getCurrentUser().getShortUserName());
933-
aux = new AuxServices(MOCK_AUX_PATH_HANDLER,
935+
aux = getSpyAuxServices(MOCK_AUX_PATH_HANDLER,
934936
MOCK_CONTEXT, MOCK_DEL_SERVICE);
935937
aux.init(conf);
936938
assertEquals(2, aux.getServices().size());
937939
}
938940

941+
@Test
942+
public void testCheckManifestPermissionsWhenFileIsOnlyWritableByOwner() throws IOException {
943+
Assume.assumeTrue(useManifest);
944+
final AuxServices aux = spy(new AuxServices(MOCK_AUX_PATH_HANDLER,
945+
MOCK_CONTEXT, MOCK_DEL_SERVICE));
946+
FileStatus manifestFileStatus = mock(FileStatus.class);
947+
Path manifestPath = mock(Path.class);
948+
949+
when(manifestFileStatus.getPermission()).thenReturn(WRITABLE_BY_OWNER);
950+
when(manifestFileStatus.getPath()).thenReturn(manifestPath);
951+
952+
assertTrue(aux.checkManifestPermissions(manifestFileStatus));
953+
}
954+
955+
@Test
956+
public void testCheckManifestPermissionsWhenFileIsWritableByGroup() throws IOException {
957+
Assume.assumeTrue(useManifest);
958+
final AuxServices aux = spy(new AuxServices(MOCK_AUX_PATH_HANDLER,
959+
MOCK_CONTEXT, MOCK_DEL_SERVICE));
960+
FileStatus manifestFileStatus = mock(FileStatus.class);
961+
Path manifestPath = mock(Path.class);
962+
963+
when(manifestFileStatus.getPermission()).thenReturn(WRITABLE_BY_GROUP);
964+
when(manifestFileStatus.getPath()).thenReturn(manifestPath);
965+
966+
assertFalse(aux.checkManifestPermissions(manifestFileStatus));
967+
}
968+
969+
@Test
970+
public void testCheckManifestPermissionsWhenParentIsWritableByGroup() throws IOException {
971+
Assume.assumeTrue(useManifest);
972+
final AuxServices aux = spy(new AuxServices(MOCK_AUX_PATH_HANDLER,
973+
MOCK_CONTEXT, MOCK_DEL_SERVICE));
974+
975+
FileStatus manifestFileStatus = mock(FileStatus.class);
976+
FileStatus parentFolderStatus = mock(FileStatus.class);
977+
when(manifestFileStatus.getPermission()).thenReturn(WRITABLE_BY_OWNER);
978+
when(parentFolderStatus.getPermission()).thenReturn(WRITABLE_BY_GROUP);
979+
980+
Path manifestPath = mock(Path.class);
981+
Path parentPath = mock(Path.class);
982+
when(manifestFileStatus.getPath()).thenReturn(manifestPath);
983+
when(manifestPath.getParent()).thenReturn(parentPath);
984+
985+
FileSystem manifestFs = mock(FileSystem.class);
986+
when(manifestFs.getFileStatus(parentPath)).thenReturn(parentFolderStatus);
987+
doReturn(manifestFs).when(aux).getManifestFS();
988+
989+
assertFalse(aux.checkManifestPermissions(manifestFileStatus));
990+
}
991+
992+
@Test
993+
public void testCheckManifestPermissionsWhenParentAndFileIsWritableByOwner() throws IOException {
994+
Assume.assumeTrue(useManifest);
995+
final AuxServices aux = spy(new AuxServices(MOCK_AUX_PATH_HANDLER,
996+
MOCK_CONTEXT, MOCK_DEL_SERVICE));
997+
998+
FileStatus manifestFileStatus = mock(FileStatus.class);
999+
FileStatus parentFolderStatus = mock(FileStatus.class);
1000+
when(manifestFileStatus.getPermission()).thenReturn(WRITABLE_BY_OWNER);
1001+
when(parentFolderStatus.getPermission()).thenReturn(WRITABLE_BY_OWNER);
1002+
1003+
Path manifestPath = mock(Path.class);
1004+
Path parentPath = mock(Path.class);
1005+
when(manifestFileStatus.getPath()).thenReturn(manifestPath);
1006+
when(parentFolderStatus.getPath()).thenReturn(parentPath);
1007+
when(manifestPath.getParent()).thenReturn(parentPath);
1008+
1009+
FileSystem manifestFs = mock(FileSystem.class);
1010+
when(manifestFs.getFileStatus(parentPath)).thenReturn(parentFolderStatus);
1011+
doReturn(manifestFs).when(aux).getManifestFS();
1012+
1013+
assertTrue(aux.checkManifestPermissions(manifestFileStatus));
1014+
}
1015+
9391016
@Test
9401017
public void testRemoveManifest() throws IOException {
9411018
Assume.assumeTrue(useManifest);
9421019
Configuration conf = getABConf();
943-
final AuxServices aux = new AuxServices(MOCK_AUX_PATH_HANDLER,
1020+
final AuxServices aux = getSpyAuxServices(MOCK_AUX_PATH_HANDLER,
9441021
MOCK_CONTEXT, MOCK_DEL_SERVICE);
9451022
aux.init(conf);
9461023
assertEquals(2, aux.getServices().size());
@@ -953,7 +1030,7 @@ public void testRemoveManifest() throws IOException {
9531030
public void testManualReload() throws IOException {
9541031
Assume.assumeTrue(useManifest);
9551032
Configuration conf = getABConf();
956-
final AuxServices aux = new AuxServices(MOCK_AUX_PATH_HANDLER,
1033+
final AuxServices aux = getSpyAuxServices(MOCK_AUX_PATH_HANDLER,
9571034
MOCK_CONTEXT, MOCK_DEL_SERVICE);
9581035
aux.init(conf);
9591036
try {

0 commit comments

Comments
 (0)