----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51124/#review149863 -----------------------------------------------------------
THe patch looks good to me. I'd like to adjust the test to better catch regression. src/slave/containerizer/mesos/provisioner/backends/aufs.cpp (lines 57 - 58) <https://reviews.apache.org/r/51124/#comment217590> Hum, this compiles? src/slave/containerizer/mesos/provisioner/backends/aufs.cpp (line 108) <https://reviews.apache.org/r/51124/#comment217591> 2 lines apart. src/slave/containerizer/mesos/provisioner/backends/overlay.cpp (lines 173 - 174) <https://reviews.apache.org/r/51124/#comment217595> I'd suggest using `link -> real` to be consistent with ls result. ``` Failed to create symlink 'links -> /tmp/xxx': error ``` Please include the error message as well. src/slave/containerizer/mesos/provisioner/backends/overlay.cpp (line 177) <https://reviews.apache.org/r/51124/#comment217596> Ditto on the format 'link -> /tmp/xxx' src/slave/containerizer/mesos/provisioner/backends/overlay.cpp (line 187) <https://reviews.apache.org/r/51124/#comment217598> insert an extra line above this src/slave/containerizer/mesos/provisioner/backends/overlay.cpp (lines 190 - 191) <https://reviews.apache.org/r/51124/#comment217600> Ditto on message formatting: 'link -> xxx' src/slave/containerizer/mesos/provisioner/backends/overlay.cpp (line 193) <https://reviews.apache.org/r/51124/#comment217599> insert an extra line above this src/slave/containerizer/mesos/provisioner/backends/overlay.cpp (lines 278 - 282) <https://reviews.apache.org/r/51124/#comment217610> I warning here sounds unnecessary. This is a legit case, and a common case. I'd suggest we either use a VLOG here, or simply not logging anything. I would suggest: ``` const string tempLink = ...; if (os::exists(tempLink)) { if (!os::stat::islink(tempLink)) { return Failure("xxx"); } Result<string> realpath = os::realpath(tempLink); // NOTE: It's possible that the symlink is a dangling symlink. // This is possible if agent crashes after we remove the temp // directory but before we remove the symlink itself. if (realpath.isSome()) { Try<Nothing> rmdir = os::rmdir(realpath.get()); if (rmdir.isError()) { return Failure(""); } VLOG(1) << "..."; } Try<Nothing> rm = os::rm(tempLink); ... } ``` src/slave/containerizer/mesos/provisioner/backends/overlay.cpp (line 282) <https://reviews.apache.org/r/51124/#comment217602> Remove the tailing period. src/slave/containerizer/mesos/provisioner/backends/overlay.cpp (lines 284 - 285) <https://reviews.apache.org/r/51124/#comment217601> This fits in one line. src/tests/containerizer/provisioner_backend_tests.cpp (lines 116 - 131) <https://reviews.apache.org/r/51124/#comment217612> I'd suggest we create a separate test. Also, i suggest we construct a lot of layers (using a for loop), and make sure the test fail without applying this patch and succeed after applying this patch. This better captures the regression. - Jie Yu On Sept. 20, 2016, 5:28 p.m., Zhitao Li wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51124/ > ----------------------------------------------------------- > > (Updated Sept. 20, 2016, 5:28 p.m.) > > > Review request for mesos, Xiaojian Huang, Gilbert Song, and Jie Yu. > > > Bugs: MESOS-6000 > https://issues.apache.org/jira/browse/MESOS-6000 > > > Repository: mesos > > > Description > ------- > > Support more layers through symlink for overlay backend. > > > Diffs > ----- > > src/slave/containerizer/mesos/provisioner/backend.hpp > 93819c887f2f801f06aae7020084b56cc81ce818 > src/slave/containerizer/mesos/provisioner/backends/aufs.hpp > a5ac13bc17b8f291472b622ebfdd0cd1dd072273 > src/slave/containerizer/mesos/provisioner/backends/aufs.cpp > 4c5cdb660dea205aea29d217ba80d845d1108fdf > src/slave/containerizer/mesos/provisioner/backends/bind.hpp > 19d44c6a9a2cd5f8de79f33c76e9d79900ab003f > src/slave/containerizer/mesos/provisioner/backends/bind.cpp > 62ccaec1a5cfb466e929f4db7bf7e5376f2a0c2d > src/slave/containerizer/mesos/provisioner/backends/copy.hpp > 5bd6a5279d8dd6d088afcd58e4839bf6087ccd1e > src/slave/containerizer/mesos/provisioner/backends/copy.cpp > e57bb3d2fb4e67e9caae416824a6a748db1460a1 > src/slave/containerizer/mesos/provisioner/backends/overlay.hpp > 387f28a331813c75a509b4a31dbbdc764080b8c1 > src/slave/containerizer/mesos/provisioner/backends/overlay.cpp > e20cd48463a78ecd8a652a4d5ac923dc02cca4d9 > src/slave/containerizer/mesos/provisioner/provisioner.cpp > f37c45ccfa572876dfbba6a0797c223896db5a7f > src/tests/containerizer/provisioner_backend_tests.cpp > 7c9a7cd5c733f74e10316fc1234115e6820cc2cb > > Diff: https://reviews.apache.org/r/51124/diff/ > > > Testing > ------- > > 1. Make sure `OverlayBackendTest.*` passes; > 2. Use mesos-execute to provision a container using overlay backend. Observed > following log lines: > > ``` > I0816 01:04:27.823420 46584 overlay.cpp:167] Created tempLink at > '/var/lib/mesos/provisioner/containers/fd574bbf-4bc0-4538-9ce5-8c2cc93b94c7/backends/overlay/links' > to '/tmp/NcmRZt' > I0816 01:04:27.824834 46584 overlay.cpp:191] Provisioning image rootfs with > overlayfs: > 'lowerdir=/tmp/NcmRZt/42:/tmp/NcmRZt/41:/tmp/NcmRZt/40:/tmp/NcmRZt/39:/tmp/NcmRZt/38:/tmp/NcmRZt/37:/tmp/NcmRZt/36:/tmp/NcmRZt/35:/tmp/NcmRZt/34:/tmp/NcmRZt/33:/tmp/NcmRZt/32:/tmp/NcmRZt/31:/tmp/NcmRZt/30:/tmp/NcmRZt/29:/tmp/NcmRZt/28:/tmp/NcmRZt/27:/tmp/NcmRZt/26:/tmp/NcmRZt/25:/tmp/NcmRZt/24:/tmp/NcmRZt/23:/tmp/NcmRZt/22:/tmp/NcmRZt/21:/tmp/NcmRZt/20:/tmp/NcmRZt/19:/tmp/NcmRZt/18:/tmp/NcmRZt/17:/tmp/NcmRZt/16:/tmp/NcmRZt/15:/tmp/NcmRZt/14:/tmp/NcmRZt/13:/tmp/NcmRZt/12:/tmp/NcmRZt/11:/tmp/NcmRZt/10:/tmp/NcmRZt/9:/tmp/NcmRZt/8:/tmp/NcmRZt/7:/tmp/NcmRZt/6:/tmp/NcmRZt/5:/tmp/NcmRZt/4:/tmp/NcmRZt/3:/tmp/NcmRZt/2:/tmp/NcmRZt/1:/tmp/NcmRZt/0,upperdir=/var/lib/mesos/provisioner/containers/fd574bbf-4bc0-4538-9ce5-8c2cc93b94c7/backends/overlay/scratch/25d9b6e4-fd0e-401e-bb9e-754dbaec4f55/upperdir,workdir=/var/lib/mesos/provisioner/containers/fd574bbf-4bc0-4538-9ce5-8c2cc93b94c7/backends/overlay/scratch/2 5d9b6e4-fd0e-401e-bb9e-754dbaec4f55/workdir' > ... > (after executor exited) > I0816 01:04:34.859851 46584 overlay.cpp:281] Removed temporary directory > '/tmp/NcmRZt' pointed by > '/var/lib/mesos/provisioner/containers/fd574bbf-4bc0-4538-9ce5-8c2cc93b94c7/backends/overlay/links' > ``` > > > Thanks, > > Zhitao Li > >
