mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Qian Zhang <zhq527...@gmail.com>
Subject Re: Review Request 53115: Implemented handling AUFS whiteouts for copy backend.
Date Fri, 28 Oct 2016 09:45:10 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message