[ 
https://issues.apache.org/jira/browse/YARN-9561?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16959191#comment-16959191
 ] 

Jim Brennan commented on YARN-9561:
-----------------------------------

Thanks for the updated patch [~ebadger]!

I had a few comments on patch 005:

runc_write_config()
 *  add_std_mounts_json() - should we increase shm to 8GB like we did 
internally?

main.c
 - [BUG] main() - missing {{break}} statement after 
{{RUN_AS_USER_SYNC_YARN_SYSFS}} block.

test_string_utils.cc
 - TEST_F(TestStringUtils, test_strbuf_detach)
this test would be a little better if it moved the buf contents assert to after 
the third append format and/or included a check that sb.buffer != buf (after 
that last append).
 - TEST_F(TestStringUtils, test_strbuf_realloc)
looks like this has  some left over debug std::cout lines?

test_runc_util.cc
 - build_process_struct()
returns true when is fails and false when it succeeds?
 - build_mounts_json()
 is there a reason for three options string arrays?  The first two are 
identical.  Can’t you just have options_ro and options_rw?
 - (nit) unindented line: {{remove(pid_file);}}
 - Would be nice to move these lines into a function - they are repeated a lot:

{noformat}
std::string container_executor_cfg_contents = "[runc]\n  "
                                              
"runc.allowed.rw-mounts=/opt,/var,/usr/bin/cut,/usr/bin/awk\n  "
                                              
"runc.allowed.ro-mounts=/etc/passwd";


ret = setup_container_executor_cfg(container_executor_cfg_contents);
ASSERT_EQ(ret, 0) << "Container executor cfg setup failed\n"; {noformat}

 

> Add C changes for the new RuncContainerRuntime
> ----------------------------------------------
>
>                 Key: YARN-9561
>                 URL: https://issues.apache.org/jira/browse/YARN-9561
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>            Reporter: Eric Badger
>            Assignee: Eric Badger
>            Priority: Major
>         Attachments: YARN-9561.001.patch, YARN-9561.002.patch, 
> YARN-9561.003.patch, YARN-9561.004.patch, YARN-9561.005.patch
>
>
> This JIRA will be used to add the C changes to the container-executor native 
> binary that are necessary for the new RuncContainerRuntime. There should be 
> no changes to existing code paths. 



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

---------------------------------------------------------------------
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