----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69590/#review211416 -----------------------------------------------------------
Thanks for this. Just some nitpicking below :) src/slave/containerizer/mesos/isolators/network/cni/cni.cpp Lines 1307-1308 (original) <https://reviews.apache.org/r/69590/#comment296498> Are we assuming that this location was never meant for users? src/slave/containerizer/mesos/isolators/network/cni/cni.cpp Lines 1312 (patched) <https://reviews.apache.org/r/69590/#comment296497> s/"network/"Network/g src/slave/containerizer/mesos/isolators/network/cni/cni.cpp Line 1779 (original) <https://reviews.apache.org/r/69590/#comment296495> See above. src/slave/containerizer/mesos/isolators/network/cni/cni.cpp Lines 1785 (patched) <https://reviews.apache.org/r/69590/#comment296496> See above. src/slave/flags.cpp Lines 1189 (patched) <https://reviews.apache.org/r/69590/#comment296493> ``` "This setting controls whether the CNI root directory\n" " persists beyond reboots or not." ``` Maybe? src/tests/containerizer/cni_isolator_tests.cpp Lines 2542 (patched) <https://reviews.apache.org/r/69590/#comment296491> Add a trailing period please. And maybe ``` // This test verifies the CNI root directory path. ``` src/tests/containerizer/cni_isolator_tests.cpp Lines 2564 (patched) <https://reviews.apache.org/r/69590/#comment296499> We can do without this, I feel. src/tests/containerizer/cni_isolator_tests.cpp Lines 2567 (patched) <https://reviews.apache.org/r/69590/#comment296501> This seems to be the most relevant part of this test - checking with and without this flag enabled. Maybe we should add a comment here? src/tests/containerizer/cni_isolator_tests.cpp Lines 2569 (patched) <https://reviews.apache.org/r/69590/#comment296500> Either remove these two please or start with capital letter and end with punctuation. Thanks. - Till Toenshoff On Dec. 19, 2018, 12:12 a.m., Deepak Goel wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69590/ > ----------------------------------------------------------- > > (Updated Dec. 19, 2018, 12:12 a.m.) > > > Review request for mesos, Gilbert Song, Jie Yu, Qian Zhang, and Sergey > Urbanovich. > > > Bugs: MESOS-9492 > https://issues.apache.org/jira/browse/MESOS-9492 > > > Repository: mesos > > > Description > ------- > > This patch moves the CNI root directory to mesos working directory > so that the network related information persist across reboot which > will allow cni isolator to do proper cleanup post reboot > > > Diffs > ----- > > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp > f19edce0671686bc2dccf5dc6d3abc40b925852e > src/slave/containerizer/mesos/isolators/network/cni/paths.hpp > 3b06fb101d03b32a3a148e2b79502413a7268d1e > src/slave/containerizer/mesos/isolators/network/cni/paths.cpp > c1cb4f7d667159abbf87219cde30528a4a7b5283 > src/slave/flags.hpp 29d8b7985ffde57da02b5fe0d3a524e98acc27c8 > src/slave/flags.cpp ccaf65029ec2d0e78041fc3992a0bf5ca0798686 > src/tests/containerizer/cni_isolator_tests.cpp > 5473737cdc71a3179c22be0d4db2060a6ef53d24 > > > Diff: https://reviews.apache.org/r/69590/diff/1/ > > > Testing > ------- > > > Thanks, > > Deepak Goel > >
