You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Chun-Hung Hsiao <ch...@mesosphere.io> on 2017/04/13 01:23:45 UTC
Review Request 58408: Overwriting Directories with Files in Copy
Provisioner.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58408/
-----------------------------------------------------------
Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.
Bugs: MESOS-5028
https://issues.apache.org/jira/browse/MESOS-5028
Repository: mesos
Description
-------
When a layer overwrites a directory with a regular file or symbolic link
(or vice versa), the old dir/file need to be removed before copying the
layer into the rootfs. This is processed together with whiteout:
The copy provisioner find all files to remove, including files
marked as whiteout and the files described above, and remove them
before the copy process.
Diffs
-----
src/slave/containerizer/mesos/provisioner/backends/copy.cpp 584cc6524f81cc1bc231b105507dbfe51fec991d
Diff: https://reviews.apache.org/r/58408/diff/1/
Testing
-------
make check
Manually tested on the following images:
https://hub.docker.com/r/gilbertsong/whiteout/
https://hub.docker.com/r/chhsiao/mycirros/
Thanks,
Chun-Hung Hsiao
Re: Review Request 58408: Overwriting Directories with Files in Copy
Provisioner.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58408/#review172169
-----------------------------------------------------------
Fix it, then Ship it!
src/slave/containerizer/mesos/provisioner/backends/copy.cpp
Lines 200 (patched)
<https://reviews.apache.org/r/58408/#comment245282>
I would add extra parathesis for the second half:
```
if (os::exists(rootfsPath) &&
(os::stat::isdir(rootfsPath) != ftsIsDir)) {
...
}
```
This improves readability because the reader might not understand the operator priority.
- Jie Yu
On April 17, 2017, 9:44 p.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58408/
> -----------------------------------------------------------
>
> (Updated April 17, 2017, 9:44 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.
>
>
> Bugs: MESOS-5028
> https://issues.apache.org/jira/browse/MESOS-5028
>
>
> Repository: mesos
>
>
> Description
> -------
>
> When a layer overwrites a directory with a regular file or symbolic link
> (or vice versa), the old dir/file need to be removed before copying the
> layer into the rootfs. This is processed together with whiteout:
> The copy provisioner find all files to remove, including files
> marked as whiteout and the files described above, and remove them
> before the copy process.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/provisioner/backends/copy.cpp 584cc6524f81cc1bc231b105507dbfe51fec991d
>
>
> Diff: https://reviews.apache.org/r/58408/diff/4/
>
>
> Testing
> -------
>
> make check
> Manually tested on the following images:
> https://hub.docker.com/r/gilbertsong/whiteout/
> https://hub.docker.com/r/chhsiao/overwrite/
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 58408: Overwriting Directories with Files in Copy
Provisioner.
Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58408/
-----------------------------------------------------------
(Updated April 17, 2017, 9:44 p.m.)
Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.
Bugs: MESOS-5028
https://issues.apache.org/jira/browse/MESOS-5028
Repository: mesos
Description
-------
When a layer overwrites a directory with a regular file or symbolic link
(or vice versa), the old dir/file need to be removed before copying the
layer into the rootfs. This is processed together with whiteout:
The copy provisioner find all files to remove, including files
marked as whiteout and the files described above, and remove them
before the copy process.
Diffs (updated)
-----
src/slave/containerizer/mesos/provisioner/backends/copy.cpp 584cc6524f81cc1bc231b105507dbfe51fec991d
Diff: https://reviews.apache.org/r/58408/diff/4/
Changes: https://reviews.apache.org/r/58408/diff/3-4/
Testing
-------
make check
Manually tested on the following images:
https://hub.docker.com/r/gilbertsong/whiteout/
https://hub.docker.com/r/chhsiao/overwrite/
Thanks,
Chun-Hung Hsiao
Re: Review Request 58408: Overwriting Directories with Files in Copy
Provisioner.
Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.
> On April 17, 2017, 12:03 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/provisioner/backends/copy.cpp
> > Lines 193 (patched)
> > <https://reviews.apache.org/r/58408/diff/3/?file=1692747#file1692747line203>
> >
> > What if `node->fts_info` is things like `FTS_DNR`, `FTS_ERR`, `FTS_NS`, etc. Do we accidentally remove directories?
> >
> > I think we should explicitly check all fts_info type. I also think that we should return Failure if we encounter error conditions like FTS_ERR, FTS_NDR. (This is not your fault, ths original code does not handle that).
I added failure check for FTS_DNR, FTS_ERR and FTS_NS, and considered FTS_DC as well (didn't consider it previously because I assumed that the layer extracted by tar should contain no such structure), thus all directory-related types are considered. For non-directory types, we can just delegate them to cp.
- Chun-Hung
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58408/#review172055
-----------------------------------------------------------
On April 17, 2017, 9:44 p.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58408/
> -----------------------------------------------------------
>
> (Updated April 17, 2017, 9:44 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.
>
>
> Bugs: MESOS-5028
> https://issues.apache.org/jira/browse/MESOS-5028
>
>
> Repository: mesos
>
>
> Description
> -------
>
> When a layer overwrites a directory with a regular file or symbolic link
> (or vice versa), the old dir/file need to be removed before copying the
> layer into the rootfs. This is processed together with whiteout:
> The copy provisioner find all files to remove, including files
> marked as whiteout and the files described above, and remove them
> before the copy process.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/provisioner/backends/copy.cpp 584cc6524f81cc1bc231b105507dbfe51fec991d
>
>
> Diff: https://reviews.apache.org/r/58408/diff/4/
>
>
> Testing
> -------
>
> make check
> Manually tested on the following images:
> https://hub.docker.com/r/gilbertsong/whiteout/
> https://hub.docker.com/r/chhsiao/overwrite/
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 58408: Overwriting Directories with Files in Copy
Provisioner.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58408/#review172055
-----------------------------------------------------------
src/slave/containerizer/mesos/provisioner/backends/copy.cpp
Lines 165 (patched)
<https://reviews.apache.org/r/58408/#comment245117>
Let's use `Option<string> removePath` here, instead of relying on removePath being an empty string. We always prefer more explicit check.
src/slave/containerizer/mesos/provisioner/backends/copy.cpp
Line 166 (original), 177 (patched)
<https://reviews.apache.org/r/58408/#comment245119>
Not yours, but I think `Path(whiteout)` is not necessary. We should just use `whiteout.dirname()`
src/slave/containerizer/mesos/provisioner/backends/copy.cpp
Lines 193 (patched)
<https://reviews.apache.org/r/58408/#comment245131>
What if `node->fts_info` is things like `FTS_DNR`, `FTS_ERR`, `FTS_NS`, etc. Do we accidentally remove directories?
I think we should explicitly check all fts_info type. I also think that we should return Failure if we encounter error conditions like FTS_ERR, FTS_NDR. (This is not your fault, ths original code does not handle that).
- Jie Yu
On April 14, 2017, 6:51 p.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58408/
> -----------------------------------------------------------
>
> (Updated April 14, 2017, 6:51 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.
>
>
> Bugs: MESOS-5028
> https://issues.apache.org/jira/browse/MESOS-5028
>
>
> Repository: mesos
>
>
> Description
> -------
>
> When a layer overwrites a directory with a regular file or symbolic link
> (or vice versa), the old dir/file need to be removed before copying the
> layer into the rootfs. This is processed together with whiteout:
> The copy provisioner find all files to remove, including files
> marked as whiteout and the files described above, and remove them
> before the copy process.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/provisioner/backends/copy.cpp 584cc6524f81cc1bc231b105507dbfe51fec991d
>
>
> Diff: https://reviews.apache.org/r/58408/diff/3/
>
>
> Testing
> -------
>
> make check
> Manually tested on the following images:
> https://hub.docker.com/r/gilbertsong/whiteout/
> https://hub.docker.com/r/chhsiao/overwrite/
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 58408: Overwriting Directories with Files in Copy
Provisioner.
Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58408/
-----------------------------------------------------------
(Updated April 14, 2017, 6:51 p.m.)
Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.
Bugs: MESOS-5028
https://issues.apache.org/jira/browse/MESOS-5028
Repository: mesos
Description
-------
When a layer overwrites a directory with a regular file or symbolic link
(or vice versa), the old dir/file need to be removed before copying the
layer into the rootfs. This is processed together with whiteout:
The copy provisioner find all files to remove, including files
marked as whiteout and the files described above, and remove them
before the copy process.
Diffs (updated)
-----
src/slave/containerizer/mesos/provisioner/backends/copy.cpp 584cc6524f81cc1bc231b105507dbfe51fec991d
Diff: https://reviews.apache.org/r/58408/diff/3/
Changes: https://reviews.apache.org/r/58408/diff/2-3/
Testing
-------
make check
Manually tested on the following images:
https://hub.docker.com/r/gilbertsong/whiteout/
https://hub.docker.com/r/chhsiao/overwrite/
Thanks,
Chun-Hung Hsiao
Re: Review Request 58408: Overwriting Directories with Files in Copy
Provisioner.
Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58408/
-----------------------------------------------------------
(Updated April 14, 2017, 12:24 a.m.)
Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.
Bugs: MESOS-5028
https://issues.apache.org/jira/browse/MESOS-5028
Repository: mesos
Description
-------
When a layer overwrites a directory with a regular file or symbolic link
(or vice versa), the old dir/file need to be removed before copying the
layer into the rootfs. This is processed together with whiteout:
The copy provisioner find all files to remove, including files
marked as whiteout and the files described above, and remove them
before the copy process.
Diffs (updated)
-----
src/slave/containerizer/mesos/provisioner/backends/copy.cpp 584cc6524f81cc1bc231b105507dbfe51fec991d
Diff: https://reviews.apache.org/r/58408/diff/2/
Changes: https://reviews.apache.org/r/58408/diff/1-2/
Testing
-------
make check
Manually tested on the following images:
https://hub.docker.com/r/gilbertsong/whiteout/
https://hub.docker.com/r/chhsiao/overwrite/
Thanks,
Chun-Hung Hsiao
Re: Review Request 58408: Overwriting Directories with Files in Copy
Provisioner.
Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58408/
-----------------------------------------------------------
(Updated April 13, 2017, 8:37 p.m.)
Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.
Bugs: MESOS-5028
https://issues.apache.org/jira/browse/MESOS-5028
Repository: mesos
Description
-------
When a layer overwrites a directory with a regular file or symbolic link
(or vice versa), the old dir/file need to be removed before copying the
layer into the rootfs. This is processed together with whiteout:
The copy provisioner find all files to remove, including files
marked as whiteout and the files described above, and remove them
before the copy process.
Diffs
-----
src/slave/containerizer/mesos/provisioner/backends/copy.cpp 584cc6524f81cc1bc231b105507dbfe51fec991d
Diff: https://reviews.apache.org/r/58408/diff/1/
Testing (updated)
-------
make check
Manually tested on the following images:
https://hub.docker.com/r/gilbertsong/whiteout/
https://hub.docker.com/r/chhsiao/overwrite/
Thanks,
Chun-Hung Hsiao
Re: Review Request 58408: Overwriting Directories with Files in Copy
Provisioner.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58408/#review171840
-----------------------------------------------------------
Patch looks great!
Reviews applied: [58408]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On April 13, 2017, 1:23 a.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58408/
> -----------------------------------------------------------
>
> (Updated April 13, 2017, 1:23 a.m.)
>
>
> Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.
>
>
> Bugs: MESOS-5028
> https://issues.apache.org/jira/browse/MESOS-5028
>
>
> Repository: mesos
>
>
> Description
> -------
>
> When a layer overwrites a directory with a regular file or symbolic link
> (or vice versa), the old dir/file need to be removed before copying the
> layer into the rootfs. This is processed together with whiteout:
> The copy provisioner find all files to remove, including files
> marked as whiteout and the files described above, and remove them
> before the copy process.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/provisioner/backends/copy.cpp 584cc6524f81cc1bc231b105507dbfe51fec991d
>
>
> Diff: https://reviews.apache.org/r/58408/diff/1/
>
>
> Testing
> -------
>
> make check
> Manually tested on the following images:
> https://hub.docker.com/r/gilbertsong/whiteout/
> https://hub.docker.com/r/chhsiao/mycirros/
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>