Description
Currently when we config some of the GPU devices related fields (like ) in container-executor.cfg, these fields are generated based on different driver versions or GPU device names. We want to enable regular expression matching so that user don't need to manually set up these fields when config container-executor.cfg,
Attachments
Attachments
- YARN-7626.011.patch
- 14 kB
- Zian Chen
- YARN-7626.010.patch
- 13 kB
- Zian Chen
- YARN-7626.009.patch
- 13 kB
- Zian Chen
- YARN-7626.008.patch
- 13 kB
- Zian Chen
- YARN-7626.007.patch
- 15 kB
- Zian Chen
- YARN-7626.006.patch
- 12 kB
- Zian Chen
- YARN-7626.005.patch
- 12 kB
- Zian Chen
- YARN-7626.004.patch
- 12 kB
- Zian Chen
- YARN-7626.003.patch
- 12 kB
- Zian Chen
- YARN-7626.002.patch
- 12 kB
- Zian Chen
- YARN-7626.001.patch
- 13 kB
- Zian Chen
Activity
-1 overall |
Vote | Subsystem | Runtime | Comment |
---|---|---|---|
0 | reexec | 0m 17s | Docker mode activated. |
Prechecks | |||
+1 | @author | 0m 0s | The patch does not contain any @author tags. |
+1 | test4tests | 0m 0s | The patch appears to include 1 new or modified test files. |
trunk Compile Tests | |||
+1 | mvninstall | 16m 23s | trunk passed |
+1 | compile | 0m 48s | trunk passed |
+1 | mvnsite | 0m 30s | trunk passed |
+1 | shadedclient | 26m 47s | branch has no errors when building and testing our client artifacts. |
Patch Compile Tests | |||
+1 | mvninstall | 0m 28s | the patch passed |
+1 | compile | 0m 42s | the patch passed |
+1 | cc | 0m 42s | the patch passed |
+1 | javac | 0m 42s | the patch passed |
+1 | mvnsite | 0m 28s | the patch passed |
+1 | whitespace | 0m 0s | The patch has no whitespace issues. |
+1 | shadedclient | 10m 19s | patch has no errors when building and testing our client artifacts. |
Other Tests | |||
-1 | unit | 19m 35s | hadoop-yarn-server-nodemanager in the patch failed. |
+1 | asflicense | 0m 21s | The patch does not generate ASF License warnings. |
59m 18s |
Reason | Tests |
---|---|
Failed junit tests | TEST-cetest |
Subsystem | Report/Notes |
---|---|
Docker | Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hadoop:5b98639 |
JIRA Issue | |
JIRA Patch URL | https://issues.apache.org/jira/secure/attachment/12906484/YARN-7626.001.patch |
Optional Tests | asflicense compile cc mvnsite javac unit |
uname | Linux 1223163621b1 4.4.0-64-generic #85-Ubuntu SMP Mon Feb 20 11:50:30 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux |
Build tool | maven |
Personality | /testptch/patchprocess/precommit/personality/provided.sh |
git revision | trunk / 6e42d05 |
maven | version: Apache Maven 3.3.9 |
Default Java | 1.8.0_151 |
unit | https://builds.apache.org/job/PreCommit-YARN-Build/19301/artifact/out/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-nodemanager.txt |
Test Results | https://builds.apache.org/job/PreCommit-YARN-Build/19301/testReport/ |
Max. process+thread count | 409 (vs. ulimit of 5000) |
modules | C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager |
Console output | https://builds.apache.org/job/PreCommit-YARN-Build/19301/console |
Powered by | Apache Yetus 0.8.0-SNAPSHOT http://yetus.apache.org |
This message was automatically generated.
-1 overall |
Vote | Subsystem | Runtime | Comment |
---|---|---|---|
0 | reexec | 0m 14s | Docker mode activated. |
Prechecks | |||
+1 | @author | 0m 0s | The patch does not contain any @author tags. |
+1 | test4tests | 0m 0s | The patch appears to include 1 new or modified test files. |
trunk Compile Tests | |||
+1 | mvninstall | 16m 32s | trunk passed |
+1 | compile | 0m 51s | trunk passed |
+1 | mvnsite | 0m 34s | trunk passed |
+1 | shadedclient | 27m 49s | branch has no errors when building and testing our client artifacts. |
Patch Compile Tests | |||
+1 | mvninstall | 0m 32s | the patch passed |
+1 | compile | 0m 46s | the patch passed |
+1 | cc | 0m 46s | the patch passed |
+1 | javac | 0m 46s | the patch passed |
+1 | mvnsite | 0m 29s | the patch passed |
+1 | whitespace | 0m 0s | The patch has no whitespace issues. |
+1 | shadedclient | 11m 8s | patch has no errors when building and testing our client artifacts. |
Other Tests | |||
-1 | unit | 19m 10s | hadoop-yarn-server-nodemanager in the patch failed. |
+1 | asflicense | 0m 20s | The patch does not generate ASF License warnings. |
60m 51s |
Reason | Tests |
---|---|
Failed junit tests | TEST-cetest |
Subsystem | Report/Notes |
---|---|
Docker | Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hadoop:5b98639 |
JIRA Issue | |
JIRA Patch URL | https://issues.apache.org/jira/secure/attachment/12907245/YARN-7626.002.patch |
Optional Tests | asflicense compile cc mvnsite javac unit |
uname | Linux 095e05e3611c 3.13.0-135-generic #184-Ubuntu SMP Wed Oct 18 11:55:51 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux |
Build tool | maven |
Personality | /testptch/patchprocess/precommit/personality/provided.sh |
git revision | trunk / 6347b22 |
maven | version: Apache Maven 3.3.9 |
Default Java | 1.8.0_151 |
unit | https://builds.apache.org/job/PreCommit-YARN-Build/19390/artifact/out/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-nodemanager.txt |
Test Results | https://builds.apache.org/job/PreCommit-YARN-Build/19390/testReport/ |
Max. process+thread count | 306 (vs. ulimit of 5000) |
modules | C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager |
Console output | https://builds.apache.org/job/PreCommit-YARN-Build/19390/console |
Powered by | Apache Yetus 0.8.0-SNAPSHOT http://yetus.apache.org |
This message was automatically generated.
-1 overall |
Vote | Subsystem | Runtime | Comment |
---|---|---|---|
0 | reexec | 0m 10s | Docker mode activated. |
Prechecks | |||
+1 | @author | 0m 0s | The patch does not contain any @author tags. |
+1 | test4tests | 0m 0s | The patch appears to include 1 new or modified test files. |
trunk Compile Tests | |||
+1 | mvninstall | 17m 16s | trunk passed |
+1 | compile | 0m 50s | trunk passed |
+1 | mvnsite | 0m 34s | trunk passed |
+1 | shadedclient | 28m 29s | branch has no errors when building and testing our client artifacts. |
Patch Compile Tests | |||
+1 | mvninstall | 0m 40s | the patch passed |
+1 | compile | 0m 56s | the patch passed |
+1 | cc | 0m 56s | the patch passed |
+1 | javac | 0m 56s | the patch passed |
+1 | mvnsite | 0m 35s | the patch passed |
+1 | whitespace | 0m 0s | The patch has no whitespace issues. |
+1 | shadedclient | 12m 0s | patch has no errors when building and testing our client artifacts. |
Other Tests | |||
-1 | unit | 18m 55s | hadoop-yarn-server-nodemanager in the patch failed. |
+1 | asflicense | 0m 22s | The patch does not generate ASF License warnings. |
62m 30s |
Reason | Tests |
---|---|
Failed junit tests | TEST-cetest |
Subsystem | Report/Notes |
---|---|
Docker | Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hadoop:5b98639 |
JIRA Issue | |
JIRA Patch URL | https://issues.apache.org/jira/secure/attachment/12907366/YARN-7626.003.patch |
Optional Tests | asflicense compile cc mvnsite javac unit |
uname | Linux e8775e29b5d8 3.13.0-133-generic #182-Ubuntu SMP Tue Sep 19 15:49:21 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux |
Build tool | maven |
Personality | /testptch/patchprocess/precommit/personality/provided.sh |
git revision | trunk / a72cdcc |
maven | version: Apache Maven 3.3.9 |
Default Java | 1.8.0_151 |
unit | https://builds.apache.org/job/PreCommit-YARN-Build/19409/artifact/out/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-nodemanager.txt |
Test Results | https://builds.apache.org/job/PreCommit-YARN-Build/19409/testReport/ |
Max. process+thread count | 379 (vs. ulimit of 5000) |
modules | C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager |
Console output | https://builds.apache.org/job/PreCommit-YARN-Build/19409/console |
Powered by | Apache Yetus 0.8.0-SNAPSHOT http://yetus.apache.org |
This message was automatically generated.
-1 overall |
Vote | Subsystem | Runtime | Comment |
---|---|---|---|
0 | reexec | 0m 10s | Docker mode activated. |
Prechecks | |||
+1 | @author | 0m 0s | The patch does not contain any @author tags. |
+1 | test4tests | 0m 0s | The patch appears to include 1 new or modified test files. |
trunk Compile Tests | |||
+1 | mvninstall | 17m 40s | trunk passed |
+1 | compile | 0m 51s | trunk passed |
+1 | mvnsite | 0m 35s | trunk passed |
+1 | shadedclient | 29m 18s | branch has no errors when building and testing our client artifacts. |
Patch Compile Tests | |||
+1 | mvninstall | 0m 42s | the patch passed |
+1 | compile | 1m 1s | the patch passed |
+1 | cc | 1m 1s | the patch passed |
+1 | javac | 1m 1s | the patch passed |
+1 | mvnsite | 0m 39s | the patch passed |
+1 | whitespace | 0m 0s | The patch has no whitespace issues. |
+1 | shadedclient | 12m 8s | patch has no errors when building and testing our client artifacts. |
Other Tests | |||
-1 | unit | 18m 56s | hadoop-yarn-server-nodemanager in the patch failed. |
+1 | asflicense | 0m 31s | The patch does not generate ASF License warnings. |
63m 48s |
Reason | Tests |
---|---|
Failed junit tests | hadoop.yarn.server.nodemanager.containermanager.linux.runtime.TestDockerContainerRuntime |
Subsystem | Report/Notes |
---|---|
Docker | Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hadoop:5b98639 |
JIRA Issue | |
JIRA Patch URL | https://issues.apache.org/jira/secure/attachment/12907587/YARN-7626.004.patch |
Optional Tests | asflicense compile cc mvnsite javac unit |
uname | Linux 0a0f9ed7bed6 3.13.0-133-generic #182-Ubuntu SMP Tue Sep 19 15:49:21 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux |
Build tool | maven |
Personality | /testptch/patchprocess/precommit/personality/provided.sh |
git revision | trunk / 0c559b2 |
maven | version: Apache Maven 3.3.9 |
Default Java | 1.8.0_151 |
unit | https://builds.apache.org/job/PreCommit-YARN-Build/19443/artifact/out/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-nodemanager.txt |
Test Results | https://builds.apache.org/job/PreCommit-YARN-Build/19443/testReport/ |
Max. process+thread count | 329 (vs. ulimit of 5000) |
modules | C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager |
Console output | https://builds.apache.org/job/PreCommit-YARN-Build/19443/console |
Powered by | Apache Yetus 0.8.0-SNAPSHOT http://yetus.apache.org |
This message was automatically generated.
Tested patch 005 on cluster with GPU. Looks good. Upload the patch.
-1 overall |
Vote | Subsystem | Runtime | Comment |
---|---|---|---|
0 | reexec | 23m 19s | Docker mode activated. |
Prechecks | |||
+1 | @author | 0m 0s | The patch does not contain any @author tags. |
+1 | test4tests | 0m 0s | The patch appears to include 1 new or modified test files. |
trunk Compile Tests | |||
+1 | mvninstall | 15m 52s | trunk passed |
+1 | compile | 0m 51s | trunk passed |
+1 | mvnsite | 0m 33s | trunk passed |
+1 | shadedclient | 26m 26s | branch has no errors when building and testing our client artifacts. |
Patch Compile Tests | |||
+1 | mvninstall | 0m 30s | the patch passed |
+1 | compile | 0m 46s | the patch passed |
+1 | cc | 0m 46s | the patch passed |
+1 | javac | 0m 46s | the patch passed |
+1 | mvnsite | 0m 29s | the patch passed |
+1 | whitespace | 0m 0s | The patch has no whitespace issues. |
+1 | shadedclient | 10m 11s | patch has no errors when building and testing our client artifacts. |
Other Tests | |||
-1 | unit | 20m 2s | hadoop-yarn-server-nodemanager in the patch failed. |
+1 | asflicense | 0m 21s | The patch does not generate ASF License warnings. |
82m 25s |
Reason | Tests |
---|---|
Failed junit tests | hadoop.yarn.server.nodemanager.containermanager.TestContainerManager |
Subsystem | Report/Notes |
---|---|
Docker | Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hadoop:5b98639 |
JIRA Issue | |
JIRA Patch URL | https://issues.apache.org/jira/secure/attachment/12907780/YARN-7626.005.patch |
Optional Tests | asflicense compile cc mvnsite javac unit |
uname | Linux 27841858be2a 4.4.0-89-generic #112-Ubuntu SMP Mon Jul 31 19:38:41 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux |
Build tool | maven |
Personality | /testptch/patchprocess/precommit/personality/provided.sh |
git revision | trunk / 16be42d |
maven | version: Apache Maven 3.3.9 |
Default Java | 1.8.0_151 |
unit | https://builds.apache.org/job/PreCommit-YARN-Build/19479/artifact/out/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-nodemanager.txt |
Test Results | https://builds.apache.org/job/PreCommit-YARN-Build/19479/testReport/ |
Max. process+thread count | 408 (vs. ulimit of 5000) |
modules | C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager |
Console output | https://builds.apache.org/job/PreCommit-YARN-Build/19479/console |
Powered by | Apache Yetus 0.8.0-SNAPSHOT http://yetus.apache.org |
This message was automatically generated.
Thanks Zian Chen for working on this issue and testing. I just updated the title a bit to better reflect what the patch does:
ebadger/miklos.szegedi@cloudera.com/shanekumpf@gmail.com, could you help to review the patch?
Zian Chen, thank you for the patch.
76 static int is_regex(const char *str) { 77 const char *regex_str = "^[\\^].*[\\$]$"; 78 return execute_regex_match(regex_str, str); 79 }
You could just do a simple size_t len=strlen(str); return !(len>2 && str[0]=='^' && str[len-1]=='$'); It is probably more efficient than compiling the regex, etc.
// Iterate each permitted values.
There is a missing 'through' here in the iterate through each value message.
137 if (is_regex(permitted_values[j]) == 0) {
138 ret = validate_volume_name_with_argument(values[i], permitted_values[j]);
139 }
I would put ret = strncmp(values[i], permitted_values[j], tmp_ptr - values[i]); into the else of this block.
// if it's a valid REGEX return; for user mount, we need to strictly check
Is not there a contradiction with the code? 853 if (validate_volume_name(mount) == 0) {
925 // if (permitted_mounts[i] is a REGEX): use REGEX to compare; return 926 if (is_regex(permitted_mounts[i]) == 0 && 927 validate_volume_name_with_argument(normalized_path, permitted_mounts[i]) == 0) { 928 ret = 1; 929 break; 930 }
Similarly, is not there a contradiction between the code and the comment? If the comment is right, this check should be before if (strcmp(normalized_path, permitted_mounts[i]) == 0) { and break in case of the regex, regardless of match or not.
979 ret = normalize_mounts(permitted_ro_mounts, -1);
If isUserMount is a boolean, I would use 0 or 1. -1 might be misleading to some folks.
Hi miklos.szegedi@cloudera.com , thank you so much for your comments, for is_regex function, missing "through" for iteration and isUserMount boolean value change, I'm totally agreed with your suggestions. I'll update the patch according to your comments.
For those two contradictions, I would like to explain more in details, line 853 and line 925-930 are inside function check_mount_permitted, this function accepts two params as input, permitted_mounts are mounts which are specified by admin which can be permitted for checking the user mounts, and requested are user mounts. in line
char *normalized_path = normalize_mount(requested, 0);
we call function normalize_mount which will lead to line 853, in this function we use "isUsermount" to distinguish the input mount is user mount or permitted mount, here, in this case, it's user mount, then this part of the code will not be executed for user mount,
// we only allow permitted mount to be REGEX, for permitted mount, we check // if it's a valid REGEX return; for user mount, we need to strictly check if (isUserMount != 0) { if (is_regex(mount) == 0) { return strdup(mount); } }
we will follow the original logic which checks if the user mount is a real path, if not, verify if it's a validate volume name, then do proper actions and return normalized_path for user mount.
Then line 925-930 is used to check if permitted mounts are a regular expression. If it is, we need to use regex matching to see if current permitted_mounts can be matched with normalized_path of the user mount, that's what line 925-930 trying to do. Let me give you an example for this,
for example one of the permitted_mounts is "/usr/local/.$" which is a regex and the user mount is "//usr/local/bin/", when we call function check_mount_permitted, we will call function normalize_mount for user mount "//usr/local/bin/", check if it's a real path, it is, then the rest of code will normalize the user mount into "/usr/local/bin/" then return as normalized_path, then we go back to function check_mount_permitted and loop permitted_mounts, no other can match "/usr/local/bin/" except regex "/usr/local/.$", and this is because we use line 925-930 check permitted_mounts is a regex and it can match "/usr/local/bin/" using regex matching.
However, function does not always get user mount as input, like when we call function normalize_mounts in these two lines inside function add_mounts,
ret = normalize_mounts(permitted_ro_mounts, 1); ret |= normalize_mounts(permitted_rw_mounts, 1);
normalize_mount will get permitted_mounts which let the code logic enter into this part,
// we only allow permitted mount to be REGEX, for permitted mount, we check // if it's a valid REGEX return; for user mount, we need to strictly check if (isUserMount != 0) { if (is_regex(mount) == 0) { return strdup(mount); } }
in this situation, we only accept permitted_mounts as regex when it's not a real path, otherwise, we don't allow it as a valid permitted_mount.
For the second contradiction, "this check should be before if (strcmp(normalized_path, permitted_mounts[i]) == 0)", the order of the first check and second check is not matter, because inside permitted_mounts array, we have regex format permitted_mount as well as non-regex ones, we try to use every possible permitted_mouunt to see if we could match current normalized_path, no matter the permitted_mount is regex or non-regex, if it's non-regex, we use strcmp to match, if it's regex, we use function validate_volume_name_with_argument to match.
Hope my explanation helps with the understanding of this part of the code, further comments are highly welcomed. leftnoteasy What's your opinion
Update patch 006 per Miklos's suggestions. leftnoteasy ,sunilg , could you please help review the latest patch? Thanks!
+1 overall |
Vote | Subsystem | Runtime | Comment |
---|---|---|---|
0 | reexec | 9m 42s | Docker mode activated. |
Prechecks | |||
+1 | @author | 0m 0s | The patch does not contain any @author tags. |
+1 | test4tests | 0m 0s | The patch appears to include 1 new or modified test files. |
trunk Compile Tests | |||
+1 | mvninstall | 17m 37s | trunk passed |
+1 | compile | 0m 50s | trunk passed |
+1 | mvnsite | 0m 34s | trunk passed |
+1 | shadedclient | 29m 0s | branch has no errors when building and testing our client artifacts. |
Patch Compile Tests | |||
+1 | mvninstall | 0m 32s | the patch passed |
+1 | compile | 0m 47s | the patch passed |
+1 | cc | 0m 47s | the patch passed |
+1 | javac | 0m 47s | the patch passed |
+1 | mvnsite | 0m 30s | the patch passed |
+1 | whitespace | 0m 0s | The patch has no whitespace issues. |
+1 | shadedclient | 11m 22s | patch has no errors when building and testing our client artifacts. |
Other Tests | |||
+1 | unit | 18m 19s | hadoop-yarn-server-nodemanager in the patch passed. |
+1 | asflicense | 0m 22s | The patch does not generate ASF License warnings. |
70m 57s |
Subsystem | Report/Notes |
---|---|
Docker | Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hadoop:5b98639 |
JIRA Issue | |
JIRA Patch URL | https://issues.apache.org/jira/secure/attachment/12909040/YARN-7626.006.patch |
Optional Tests | asflicense compile cc mvnsite javac unit |
uname | Linux 375642c5c406 3.13.0-139-generic #188-Ubuntu SMP Tue Jan 9 14:43:09 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux |
Build tool | maven |
Personality | /testptch/patchprocess/precommit/personality/provided.sh |
git revision | trunk / 5072388 |
maven | version: Apache Maven 3.3.9 |
Default Java | 1.8.0_151 |
Test Results | https://builds.apache.org/job/PreCommit-YARN-Build/19586/testReport/ |
Max. process+thread count | 380 (vs. ulimit of 5500) |
modules | C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager |
Console output | https://builds.apache.org/job/PreCommit-YARN-Build/19586/console |
Powered by | Apache Yetus 0.8.0-SNAPSHOT http://yetus.apache.org |
This message was automatically generated.
For the latest patch, I tested the regex expression matching on a GPU cluster for several feature requirements.,
- test if regex format permitted mounts can be normalized or not
- test if user input devices can be matched using permitted regex devices
- test if user input ro_mounts can be matched using permitted regex mounts
All these test scenarios look good.
Thank you, Zian Chen for the reply and update. Your latest patch does not seem to apply to trunk. Could you verify and rebase?
-1 overall |
Vote | Subsystem | Runtime | Comment |
---|---|---|---|
0 | reexec | 0m 0s | Docker mode activated. |
-1 | patch | 0m 4s | |
Subsystem | Report/Notes |
---|---|
JIRA Issue | |
JIRA Patch URL | https://issues.apache.org/jira/secure/attachment/12910313/YARN-7626.007.patch |
Console output | https://builds.apache.org/job/PreCommit-YARN-Build/19667/console |
Powered by | Apache Yetus 0.8.0-SNAPSHOT http://yetus.apache.org |
This message was automatically generated.
miklos.szegedi@cloudera.com , Thanks Miklos for the comments. Submitted the rebased patch.
+1 overall |
Vote | Subsystem | Runtime | Comment |
---|---|---|---|
0 | reexec | 0m 18s | Docker mode activated. |
Prechecks | |||
+1 | @author | 0m 0s | The patch does not contain any @author tags. |
+1 | test4tests | 0m 0s | The patch appears to include 1 new or modified test files. |
trunk Compile Tests | |||
+1 | mvninstall | 18m 26s | trunk passed |
+1 | compile | 0m 52s | trunk passed |
+1 | mvnsite | 0m 34s | trunk passed |
+1 | shadedclient | 30m 13s | branch has no errors when building and testing our client artifacts. |
Patch Compile Tests | |||
+1 | mvninstall | 0m 34s | the patch passed |
+1 | compile | 0m 51s | the patch passed |
+1 | cc | 0m 51s | the patch passed |
+1 | javac | 0m 51s | the patch passed |
+1 | mvnsite | 0m 31s | the patch passed |
+1 | whitespace | 0m 0s | The patch has no whitespace issues. |
+1 | shadedclient | 11m 39s | patch has no errors when building and testing our client artifacts. |
Other Tests | |||
+1 | unit | 18m 34s | hadoop-yarn-server-nodemanager in the patch passed. |
+1 | asflicense | 0m 25s | The patch does not generate ASF License warnings. |
63m 30s |
Subsystem | Report/Notes |
---|---|
Docker | Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hadoop:5b98639 |
JIRA Issue | |
JIRA Patch URL | https://issues.apache.org/jira/secure/attachment/12910347/YARN-7626.008.patch |
Optional Tests | asflicense compile cc mvnsite javac unit |
uname | Linux 9b042fad9c96 3.13.0-139-generic #188-Ubuntu SMP Tue Jan 9 14:43:09 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux |
Build tool | maven |
Personality | /testptch/patchprocess/precommit/personality/provided.sh |
git revision | trunk / 0c5d7d7 |
maven | version: Apache Maven 3.3.9 |
Default Java | 1.8.0_151 |
Test Results | https://builds.apache.org/job/PreCommit-YARN-Build/19671/testReport/ |
Max. process+thread count | 341 (vs. ulimit of 5500) |
modules | C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager |
Console output | https://builds.apache.org/job/PreCommit-YARN-Build/19671/console |
Powered by | Apache Yetus 0.8.0-SNAPSHOT http://yetus.apache.org |
This message was automatically generated.
Thank you for the patch Zian Chen.
return !(len > 2 && str[0] == '^' && str[len-1] == '$');
Optional: I think this is still misleading. is_regex should return one on success and 0 on failure.
// Iterate each permitted values.
Optional: I think it would be better to write 'Iterate through each permitted value'
'/dev/nvidia1:/dev/nvidia1' if (prefix == 0) { ret = strcmp(values[i], permitted_values[j]); } else { // If permitted-Values[j] is a REGEX, use REGEX to compare if (is_regex(permitted_values[j]) == 0) { ret = validate_volume_name_with_argument(values[i], permitted_values[j]); } else { ret = strncmp(values[i], permitted_values[j], tmp_ptr - values[i]); } }
Technically the code, where prefix is not null including the regex match, should check only the characters before the prefix :. It is checking now the whole value[i], you should apply the regex only to [values[i] ... tmp_ptr].
/** * Helper function to help normalize mounts for checking if mounts are * permitted. The function does the following - * 1. Find the canonical path for mount using realpath * 2. If the path is a directory, add a '/' at the end (if not present) * 3. Return a copy of the canonicalised path(to be freed by the caller) * @param mount path to be canonicalised * @return pointer to canonicalised path, NULL on error */ static char* normalize_mount(const char* mount, int isUserMount) {
There is no @param documentation for isUserMount, in fact I would name it isRegexAllowed to avoid confusion.
const char *container_executor_cfg_path = normalize_mount(get_config_path(""), 1);
I do not understand why the config path could be a regex.
tmp_path_buffer[0] = normalize_mount(mount_src, 1);
Should not this be 0, too?
I have a few conceptual issues with the latest patch.
- First of all, normalize_mounts, walks through the permitted mounts and it resolves symlinks but it does not resolve symlinks, if isUserMount (isRegex) is 1. What if the regex resolves to a symlink? I think it would probably be more future proof, if normalize_mounts applied the regex to the directory tree and then called the original normalize_mount on the resulting file names, that returns the real path for each. This would eliminate the need for passing isUserMount all the way through the call structure. It would also help to avoid issues that appear with invalid regexes, etc.
- Technically a regex without the ^$ pair is a valid regex. It would be more precise and future proof to mark regexes with a prefix like regex:/dev/device[0-9]+. In this case we would not need to use just a subset for matching.
Hi miklos.szegedi@cloudera.com , really appreciate your comments for the latest patch. I agree with most of the suggestions. Just have two quick questions here for the conceptual issues No. 2,
- Since current trunk code base uses ^$ pair as the judgment criteria for regex, could you kindly give us an example that there is any other type of regex that is not like ^$ pair which is useful to us in docker volume mount scenario?
- I'm also concerned using regex: as a prefix instead of ^$ pair for regex may introduce security holes, what if hackers input user mount like regex+ as a prefix?
Could you please advice? Thank you so much!
Indeed there is not there any substantial difference other than saying regex:/dev/nvidia.*. I think this latter is a bit more robust in case we try to configure regexes for other purposes in the future. This is just an opinion I let you decide.
what if hackers input user mount like regex+ as a prefix?
Regex+ won't be considered valid. What if they put ^.*$? I do not think there is a difference at least not from this point of view. But you just raised an important point. The regex has to be properly validated.
I'm changing the patch according to the comments and find some memory leak over there, I'm working on it now, will update the patch once this issue gets resolved. Thanks
-1 overall |
Vote | Subsystem | Runtime | Comment |
---|---|---|---|
0 | reexec | 0m 22s | Docker mode activated. |
Prechecks | |||
+1 | @author | 0m 0s | The patch does not contain any @author tags. |
+1 | test4tests | 0m 0s | The patch appears to include 1 new or modified test files. |
trunk Compile Tests | |||
+1 | mvninstall | 17m 20s | trunk passed |
+1 | compile | 0m 54s | trunk passed |
+1 | mvnsite | 0m 35s | trunk passed |
+1 | shadedclient | 29m 4s | branch has no errors when building and testing our client artifacts. |
Patch Compile Tests | |||
+1 | mvninstall | 0m 32s | the patch passed |
+1 | compile | 0m 48s | the patch passed |
+1 | cc | 0m 48s | the patch passed |
+1 | javac | 0m 48s | the patch passed |
+1 | mvnsite | 0m 30s | the patch passed |
+1 | whitespace | 0m 0s | The patch has no whitespace issues. |
+1 | shadedclient | 11m 22s | patch has no errors when building and testing our client artifacts. |
Other Tests | |||
-1 | unit | 18m 24s | hadoop-yarn-server-nodemanager in the patch failed. |
+1 | asflicense | 0m 24s | The patch does not generate ASF License warnings. |
61m 53s |
Reason | Tests |
---|---|
Failed junit tests | TEST-cetest |
Subsystem | Report/Notes |
---|---|
Docker | Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hadoop:5b98639 |
JIRA Issue | |
JIRA Patch URL | https://issues.apache.org/jira/secure/attachment/12912513/YARN-7626.009.patch |
Optional Tests | asflicense compile cc mvnsite javac unit |
uname | Linux 18e0514fdbda 3.13.0-139-generic #188-Ubuntu SMP Tue Jan 9 14:43:09 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux |
Build tool | maven |
Personality | /testptch/patchprocess/precommit/personality/provided.sh |
git revision | trunk / 315f48e |
maven | version: Apache Maven 3.3.9 |
Default Java | 1.8.0_151 |
unit | https://builds.apache.org/job/PreCommit-YARN-Build/19847/artifact/out/patch-unit-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-nodemanager.txt |
Test Results | https://builds.apache.org/job/PreCommit-YARN-Build/19847/testReport/ |
Max. process+thread count | 302 (vs. ulimit of 10000) |
modules | C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager |
Console output | https://builds.apache.org/job/PreCommit-YARN-Build/19847/console |
Powered by | Apache Yetus 0.8.0-SNAPSHOT http://yetus.apache.org |
This message was automatically generated.
+1 overall |
Vote | Subsystem | Runtime | Comment |
---|---|---|---|
0 | reexec | 0m 28s | Docker mode activated. |
Prechecks | |||
+1 | @author | 0m 0s | The patch does not contain any @author tags. |
+1 | test4tests | 0m 0s | The patch appears to include 1 new or modified test files. |
trunk Compile Tests | |||
+1 | mvninstall | 15m 7s | trunk passed |
+1 | compile | 0m 47s | trunk passed |
+1 | mvnsite | 0m 29s | trunk passed |
+1 | shadedclient | 24m 58s | branch has no errors when building and testing our client artifacts. |
Patch Compile Tests | |||
+1 | mvninstall | 0m 30s | the patch passed |
+1 | compile | 0m 44s | the patch passed |
+1 | cc | 0m 44s | the patch passed |
+1 | javac | 0m 44s | the patch passed |
+1 | mvnsite | 0m 28s | the patch passed |
+1 | whitespace | 0m 0s | The patch has no whitespace issues. |
+1 | shadedclient | 9m 41s | patch has no errors when building and testing our client artifacts. |
Other Tests | |||
+1 | unit | 19m 10s | hadoop-yarn-server-nodemanager in the patch passed. |
+1 | asflicense | 0m 17s | The patch does not generate ASF License warnings. |
56m 38s |
Subsystem | Report/Notes |
---|---|
Docker | Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hadoop:5b98639 |
JIRA Issue | |
JIRA Patch URL | https://issues.apache.org/jira/secure/attachment/12912683/YARN-7626.010.patch |
Optional Tests | asflicense compile cc mvnsite javac unit |
uname | Linux a55fa8e8d64c 4.4.0-89-generic #112-Ubuntu SMP Mon Jul 31 19:38:41 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux |
Build tool | maven |
Personality | /testptch/patchprocess/precommit/personality/provided.sh |
git revision | trunk / 29233c3 |
maven | version: Apache Maven 3.3.9 |
Default Java | 1.8.0_151 |
Test Results | https://builds.apache.org/job/PreCommit-YARN-Build/19864/testReport/ |
Max. process+thread count | 473 (vs. ulimit of 10000) |
modules | C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager |
Console output | https://builds.apache.org/job/PreCommit-YARN-Build/19864/console |
Powered by | Apache Yetus 0.8.0-SNAPSHOT http://yetus.apache.org |
This message was automatically generated.
Thanks Zian Chen. +1, LGTM, miklos.szegedi@cloudera.com, do you want to take a look at the latest patch?
Thank you for the patch Zian Chen and for the review leftnoteasy.
Optional: I have one style issue with the latest patch. When you refer to 6 in your patch like the one below, you should probably do sizeof("regex:"). This helps to better understand the code and it is more future proof.
132 return is_volume_name(requested) && (execute_regex_match(pattern + 6, requested) == 0);
Sure miklos.szegedi@cloudera.com totally agree with that style change. Will update the patch shortly. Thank you leftnoteasy for the review as well.
+1 overall |
Vote | Subsystem | Runtime | Comment |
---|---|---|---|
0 | reexec | 0m 33s | Docker mode activated. |
Prechecks | |||
+1 | @author | 0m 0s | The patch does not contain any @author tags. |
+1 | test4tests | 0m 0s | The patch appears to include 1 new or modified test files. |
trunk Compile Tests | |||
+1 | mvninstall | 15m 42s | trunk passed |
+1 | compile | 0m 49s | trunk passed |
+1 | mvnsite | 0m 34s | trunk passed |
+1 | shadedclient | 26m 20s | branch has no errors when building and testing our client artifacts. |
Patch Compile Tests | |||
+1 | mvninstall | 0m 29s | the patch passed |
+1 | compile | 0m 45s | the patch passed |
+1 | cc | 0m 45s | the patch passed |
+1 | javac | 0m 45s | the patch passed |
+1 | mvnsite | 0m 28s | the patch passed |
+1 | whitespace | 0m 0s | The patch has no whitespace issues. |
+1 | shadedclient | 10m 5s | patch has no errors when building and testing our client artifacts. |
Other Tests | |||
+1 | unit | 19m 55s | hadoop-yarn-server-nodemanager in the patch passed. |
+1 | asflicense | 0m 19s | The patch does not generate ASF License warnings. |
59m 18s |
Subsystem | Report/Notes |
---|---|
Docker | Client=17.05.0-ce Server=17.05.0-ce Image:yetus/hadoop:d4cc50f |
JIRA Issue | |
JIRA Patch URL | https://issues.apache.org/jira/secure/attachment/12913126/YARN-7626.011.patch |
Optional Tests | asflicense compile cc mvnsite javac unit |
uname | Linux fadf7a03e941 4.4.0-64-generic #85-Ubuntu SMP Mon Feb 20 11:50:30 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux |
Build tool | maven |
Personality | /testptch/patchprocess/precommit/personality/provided.sh |
git revision | trunk / 4971276 |
maven | version: Apache Maven 3.3.9 |
Default Java | 1.8.0_151 |
Test Results | https://builds.apache.org/job/PreCommit-YARN-Build/19894/testReport/ |
Max. process+thread count | 435 (vs. ulimit of 10000) |
modules | C: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager U: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager |
Console output | https://builds.apache.org/job/PreCommit-YARN-Build/19894/console |
Powered by | Apache Yetus 0.8.0-SNAPSHOT http://yetus.apache.org |
This message was automatically generated.
leftnoteasy , I think after changing the style issue according to Miklos's latest comments, the latest patch should be good. Could you help me do a final review and commit the patch if no other issue? Really appreciate your help!
miklos.szegedi@cloudera.com Really appreciates your help with detailed suggestions for the code changes!
Thanks miklos.szegedi@cloudera.com/Zian Chen, will commit the patch by tomorrow if no objections.
Thank you for the contribution Zian Chen and for the commit leftnoteasy.
SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #13789 (See https://builds.apache.org/job/Hadoop-trunk-Commit/13789/)
YARN-7626. Allow regular expression matching in container-executor.cfg (wangda: rev 037d7834833df2d1e60f5015b60d42550b1ddce6)
- (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/native/container-executor/test/utils/test_docker_util.cc
- (edit) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/native/container-executor/impl/utils/docker-util.c
Submit the first patch for Jenkins test.