Miklos Szegedi commented on YARN-7626:

Thank you for the patch [~Zian Chen].
return !(len > 2 && str[0] == '^' && str[len-1] == '$');{code}
Optional: I think this is still misleading. is_regex should return one on 
success and 0 on failure.
// Iterate each permitted values.{code}
Optional: I think it would be better to write 'Iterate through each permitted 
          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], 
            } else {
              ret = strncmp(values[i], permitted_values[j], tmp_ptr - 
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(""), 
I do not understand why the config path could be a regex.
tmp_path_buffer[0] = normalize_mount(mount_src, 1);{code}
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.

> Allow regular expression matching in container-executor.cfg for devices and 
> named docker volumes mount
> ------------------------------------------------------------------------------------------------------
>                 Key: YARN-7626
>                 URL: https://issues.apache.org/jira/browse/YARN-7626
>             Project: Hadoop YARN
>          Issue Type: New Feature
>            Reporter: Zian Chen
>            Assignee: Zian Chen
>            Priority: Major
>         Attachments: YARN-7626.001.patch, YARN-7626.002.patch, 
> YARN-7626.003.patch, YARN-7626.004.patch, YARN-7626.005.patch, 
> YARN-7626.006.patch, YARN-7626.007.patch, YARN-7626.008.patch
> 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,

This message was sent by Atlassian JIRA

To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org

Reply via email to