Re: Review Request 58408: Overwriting Directories with Files in Copy Provisioner.

[Jie Yu]

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


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.

[Chun-Hung Hsiao]

> On April 17, 2017, 12:03 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/provisioner/backends/copy.cpp
> > Lines 193 (patched)
> > 
> >
> > 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.

[Chun-Hung Hsiao]

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

[Jie Yu]

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


Let's use `Option 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)


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)


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.

[Chun-Hung Hsiao]

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

[Chun-Hung Hsiao]

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

[Chun-Hung Hsiao]

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

[Mesos Reviewbot]

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

Review Request 58408: Overwriting Directories with Files in Copy Provisioner.

[Chun-Hung Hsiao]

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