> On Oct. 28, 2016, 10:03 a.m., Jie Yu wrote: > > src/slave/containerizer/mesos/provisioner/backends/copy.cpp, line 213 > > <https://reviews.apache.org/r/53115/diff/3/?file=1544890#file1544890line213> > > > > Hum, IIUC, looks like this is not entirely correct. According to this: > > > > https://github.com/opencontainers/image-spec/blob/master/layer.md#whiteouts > > > > "Files that are present in the same layer as a whiteout file can only > > be hidden by whiteout files in subsequent layers." > > > > So we cannot cp first and then process whiteouts. Looks like before cp > > each layer, we need to do a pre-processing to handle whiteouts first (and > > remove the whiteouts), and then do the cp.
Thanks for pointing this out! I think the flow should be: 1. Delete the related files in rootfs based on the whiteout files in the layer. 2. Copy the layer to rootfs. 3. Once we have done 1 and 2 for all layers, traverse rootfs to remove all the whiteout files. Or maybe in 2 we can do a seletively copy by `find` + `cp` to only copy non-whiteout files from layer to rootfs? In this way, we do not need 3. > On Oct. 28, 2016, 10:03 a.m., Jie Yu wrote: > > src/slave/containerizer/mesos/provisioner/backends/copy.cpp, line 133 > > <https://reviews.apache.org/r/53115/diff/3/?file=1544890#file1544890line133> > > > > I am not sure if we need to pass in 'image' to backend. If we added OCI > > support, do we need to add another check here? > > > > I think we can simply assume the layer's whiteout is in aufs whiteout > > format, no matter what image type it is. I think APPC, we probably needs to > > do the same, right? > > > > You probably add a NOTE there saying that we assume all image type uses > > aufs whiteout format. > > > > In that way, no need to pass 'image' around to backend. The reason I passed in `image` and added this check is that previously we have such check in `ProvisionerProcess::__provision()`: https://github.com/apache/mesos/blob/1.0.1/src/slave/containerizer/mesos/provisioner/provisioner.cpp#L334 I think one of the purposes of this check is to ensure we will NOT go through the logic to handle whiteout files for APPC. I think this is correct because APPC seems have a totally different way for whiteout, in its image manifest (https://github.com/appc/spec/blob/master/spec/aci.md ), there is a field `pathWhitelist`: ``` pathWhitelist (list of strings, optional): whitelist of absolute paths that will exist in the app's rootfs after rendering. This must be a complete and absolute set. An empty list is equivalent to an absent value and means that all files in this image and any dependencies will be available in the rootfs. ``` And there is PR in APPC to translate Docker whiteout files to `pathWhitelist`: https://github.com/appc/docker2aci/pull/36 So I think if we do not pass in `image`, then copy backend will try to handle whiteout files for APPC image which is not necessary since there is no whiteout files to handle, instead, we should support `pathWhitelist` which seems missed in https://github.com/apache/mesos/blob/1.0.1/include/mesos/appc/spec.proto , that is another issue. - Qian ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53115/#review154084 ----------------------------------------------------------- On Oct. 25, 2016, 11:47 a.m., Qian Zhang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53115/ > ----------------------------------------------------------- > > (Updated Oct. 25, 2016, 11:47 a.m.) > > > Review request for mesos and Jie Yu. > > > Bugs: MESOS-6360 > https://issues.apache.org/jira/browse/MESOS-6360 > > > Repository: mesos > > > Description > ------- > > Implemented handling AUFS whiteouts for copy backend. > > > Diffs > ----- > > src/slave/containerizer/mesos/provisioner/backend.hpp > 7257d3a962ecdf87fe9d52facbd6a2619311a018 > src/slave/containerizer/mesos/provisioner/backends/aufs.hpp > a3c924195ae5eecc1caca9cd6fc0f6dc0df0a741 > src/slave/containerizer/mesos/provisioner/backends/aufs.cpp > c92c6c7174421158b9190ed0bfb00c1c97ef0f7b > src/slave/containerizer/mesos/provisioner/backends/bind.hpp > 54c88d940aa64d13114fc5d79ecbc1d474d169a6 > src/slave/containerizer/mesos/provisioner/backends/bind.cpp > 94dac40a12b6fd2e7d9733444d84763c77785402 > src/slave/containerizer/mesos/provisioner/backends/copy.hpp > 131d75521ca38afae651e8d885ebedad77d86a3e > src/slave/containerizer/mesos/provisioner/backends/copy.cpp > 9c5354e5fea488618ebdecf0aef9dd2fce555d20 > src/slave/containerizer/mesos/provisioner/backends/overlay.hpp > 4d591c5f988d87e0e905f973df5ab15a3386d676 > src/slave/containerizer/mesos/provisioner/backends/overlay.cpp > b71a31323aef376c9a28e1d52322a1802fb212ad > src/slave/containerizer/mesos/provisioner/provisioner.cpp > de2c12140652244bd3de9763ced203b144688ab2 > src/tests/containerizer/provisioner_backend_tests.cpp > 727bf645dd9337a94f098ab0a816b7e0e0651083 > > Diff: https://reviews.apache.org/r/53115/diff/ > > > Testing > ------- > > make check. > > > Thanks, > > Qian Zhang > >
