You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by James Peach <jp...@apache.org> on 2017/02/01 23:51:38 UTC

Re: Review Request 53791: Use the stout ELF parser to collect Linux rootfs files.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53791/
-----------------------------------------------------------

(Updated Feb. 1, 2017, 11:51 p.m.)


Review request for mesos, Gilbert Song, Jie Yu, Kevin Klues, and Jiang Yan Xu.


Changes
-------

Rebase and address review feedback.


Bugs: MESOS-6588
    https://issues.apache.org/jira/browse/MESOS-6588


Repository: mesos


Description
-------

The dependencies for the programs we need in the Linux root
filesystem vary over time and across distributions. Since Stout
has support for parsing the library dependencies of ELF binaries,
use this to collect the necessary dependencies when constructing
a root filesystem for the tests.


Diffs (updated)
-----

  src/tests/containerizer/rootfs.hpp 9648638c1d4b1f1c49708e058e9b930c5ae32138 
  src/tests/containerizer/rootfs.cpp f11f126fa4f3130b6778bfe7e7d0f025a638f06e 

Diff: https://reviews.apache.org/r/53791/diff/


Testing
-------

sudo make check (Fedora 24)


Thanks,

James Peach

Re: Review Request 53791: Use the stout ELF parser to collect Linux rootfs files.

Posted by Jiang Yan Xu <xu...@apple.com>.

> On Feb. 1, 2017, 3:51 p.m., James Peach wrote:
> > src/tests/containerizer/rootfs.cpp, line 90
> > <https://reviews.apache.org/r/53791/diff/8/?file=1589112#file1589112line90>
> >
> >     What are you asking for here? This can fail for a number of reasons and `errno` describes them. Squashing that to a fixed string would be misleading.
> 
> Jiang Yan Xu wrote:
>     I am saying we know "This can fail for a number of reasons and errno describes them" because we are breaking the abstraction here. Perhaps my arugment seems silly because `os::exists()` has a 9-line implementation and we can look at the source. But if it's 900 lines or we can't see the source the same principle still applies right?
>     
>     `os::exists()` current exposes a `bool` and expects it to be used to interpret what's happened. If we peek inside the implementation and write code based on the internals, it's likely to break when the implementation changes.
>     
>     Perhaps `bool os::exists()` is currently lacking in ability to reveal errors, should we then change it to `Try<bool> exists()`? If that's more suitable as a TODO or we are not sure, I think we should just leave it as it is right now. There are hundreds of other uses in the codebase which may be susceptible to conditions like "the path does exist but we are not permitted to access it" so maybe it's worth a JIRA to fix them anyways? (Of course that shouldn't block our change here)

Filed https://issues.apache.org/jira/browse/MESOS-7052 for this.


- Jiang Yan


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53791/#review163834
-----------------------------------------------------------


On Feb. 1, 2017, 3:51 p.m., James Peach wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53791/
> -----------------------------------------------------------
> 
> (Updated Feb. 1, 2017, 3:51 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, Kevin Klues, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6588
>     https://issues.apache.org/jira/browse/MESOS-6588
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The dependencies for the programs we need in the Linux root
> filesystem vary over time and across distributions. Since Stout
> has support for parsing the library dependencies of ELF binaries,
> use this to collect the necessary dependencies when constructing
> a root filesystem for the tests.
> 
> 
> Diffs
> -----
> 
>   src/tests/containerizer/rootfs.hpp 9648638c1d4b1f1c49708e058e9b930c5ae32138 
>   src/tests/containerizer/rootfs.cpp f11f126fa4f3130b6778bfe7e7d0f025a638f06e 
> 
> Diff: https://reviews.apache.org/r/53791/diff/
> 
> 
> Testing
> -------
> 
> sudo make check (Fedora 24)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 53791: Use the stout ELF parser to collect Linux rootfs files.

Posted by Jiang Yan Xu <xu...@apple.com>.

> On Feb. 1, 2017, 3:51 p.m., James Peach wrote:
> >

Could you directly reply to the comment in the future? It's pretty hard to follow with this format...


> On Feb. 1, 2017, 3:51 p.m., James Peach wrote:
> > src/tests/containerizer/rootfs.hpp, line 50
> > <https://reviews.apache.org/r/53791/diff/8/?file=1589111#file1589111line50>
> >
> >     `copy()` would normally take a const reference to something and return a copy of that. Since this is not what we are doing, best not to use that name.

Alright.


> On Feb. 1, 2017, 3:51 p.m., James Peach wrote:
> > src/tests/containerizer/rootfs.cpp, line 90
> > <https://reviews.apache.org/r/53791/diff/8/?file=1589112#file1589112line90>
> >
> >     What are you asking for here? This can fail for a number of reasons and `errno` describes them. Squashing that to a fixed string would be misleading.

I am saying we know "This can fail for a number of reasons and errno describes them" because we are breaking the abstraction here. Perhaps my arugment seems silly because `os::exists()` has a 9-line implementation and we can look at the source. But if it's 900 lines or we can't see the source the same principle still applies right?

`os::exists()` current exposes a `bool` and expects it to be used to interpret what's happened. If we peek inside the implementation and write code based on the internals, it's likely to break when the implementation changes.

Perhaps `bool os::exists()` is currently lacking in ability to reveal errors, should we then change it to `Try<bool> exists()`? If that's more suitable as a TODO or we are not sure, I think we should just leave it as it is right now. There are hundreds of other uses in the codebase which may be susceptible to conditions like "the path does exist but we are not permitted to access it" so maybe it's worth a JIRA to fix them anyways? (Of course that shouldn't block our change here)


- Jiang Yan


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53791/#review163834
-----------------------------------------------------------


On Feb. 1, 2017, 3:51 p.m., James Peach wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53791/
> -----------------------------------------------------------
> 
> (Updated Feb. 1, 2017, 3:51 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, Kevin Klues, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6588
>     https://issues.apache.org/jira/browse/MESOS-6588
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The dependencies for the programs we need in the Linux root
> filesystem vary over time and across distributions. Since Stout
> has support for parsing the library dependencies of ELF binaries,
> use this to collect the necessary dependencies when constructing
> a root filesystem for the tests.
> 
> 
> Diffs
> -----
> 
>   src/tests/containerizer/rootfs.hpp 9648638c1d4b1f1c49708e058e9b930c5ae32138 
>   src/tests/containerizer/rootfs.cpp f11f126fa4f3130b6778bfe7e7d0f025a638f06e 
> 
> Diff: https://reviews.apache.org/r/53791/diff/
> 
> 
> Testing
> -------
> 
> sudo make check (Fedora 24)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 53791: Use the stout ELF parser to collect Linux rootfs files.

Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53791/#review163834
-----------------------------------------------------------




src/tests/containerizer/rootfs.hpp (line 50)
<https://reviews.apache.org/r/53791/#comment235333>

    `copy()` would normally take a const reference to something and return a copy of that. Since this is not what we are doing, best not to use that name.



src/tests/containerizer/rootfs.cpp (lines 61 - 63)
<https://reviews.apache.org/r/53791/#comment235322>

    It's better to format consistently than to micro-manage line lengths.



src/tests/containerizer/rootfs.cpp (line 90)
<https://reviews.apache.org/r/53791/#comment235324>

    What are you asking for here? This can fail for a number of reasons and `errno` describes them. Squashing that to a fixed string would be misleading.


- James Peach


On Feb. 1, 2017, 11:51 p.m., James Peach wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53791/
> -----------------------------------------------------------
> 
> (Updated Feb. 1, 2017, 11:51 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, Kevin Klues, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6588
>     https://issues.apache.org/jira/browse/MESOS-6588
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The dependencies for the programs we need in the Linux root
> filesystem vary over time and across distributions. Since Stout
> has support for parsing the library dependencies of ELF binaries,
> use this to collect the necessary dependencies when constructing
> a root filesystem for the tests.
> 
> 
> Diffs
> -----
> 
>   src/tests/containerizer/rootfs.hpp 9648638c1d4b1f1c49708e058e9b930c5ae32138 
>   src/tests/containerizer/rootfs.cpp f11f126fa4f3130b6778bfe7e7d0f025a638f06e 
> 
> Diff: https://reviews.apache.org/r/53791/diff/
> 
> 
> Testing
> -------
> 
> sudo make check (Fedora 24)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 53791: Use the stout ELF parser to collect Linux rootfs files.

Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53791/#review163947
-----------------------------------------------------------



Patch looks great!

Reviews applied: [53790, 54712, 54878, 53791]

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 Feb. 1, 2017, 11:51 p.m., James Peach wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53791/
> -----------------------------------------------------------
> 
> (Updated Feb. 1, 2017, 11:51 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, Kevin Klues, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6588
>     https://issues.apache.org/jira/browse/MESOS-6588
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The dependencies for the programs we need in the Linux root
> filesystem vary over time and across distributions. Since Stout
> has support for parsing the library dependencies of ELF binaries,
> use this to collect the necessary dependencies when constructing
> a root filesystem for the tests.
> 
> 
> Diffs
> -----
> 
>   src/tests/containerizer/rootfs.hpp 9648638c1d4b1f1c49708e058e9b930c5ae32138 
>   src/tests/containerizer/rootfs.cpp f11f126fa4f3130b6778bfe7e7d0f025a638f06e 
> 
> Diff: https://reviews.apache.org/r/53791/diff/
> 
> 
> Testing
> -------
> 
> sudo make check (Fedora 24)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 53791: Use the stout ELF parser to collect Linux rootfs files.

Posted by Jiang Yan Xu <xu...@apple.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53791/#review164243
-----------------------------------------------------------


Ship it!




Ship It!

- Jiang Yan Xu


On Feb. 3, 2017, 4:44 p.m., James Peach wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53791/
> -----------------------------------------------------------
> 
> (Updated Feb. 3, 2017, 4:44 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, Kevin Klues, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6588
>     https://issues.apache.org/jira/browse/MESOS-6588
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The dependencies for the programs we need in the Linux root
> filesystem vary over time and across distributions. Since Stout
> has support for parsing the library dependencies of ELF binaries,
> use this to collect the necessary dependencies when constructing
> a root filesystem for the tests.
> 
> 
> Diffs
> -----
> 
>   src/tests/containerizer/rootfs.hpp 9648638c1d4b1f1c49708e058e9b930c5ae32138 
>   src/tests/containerizer/rootfs.cpp f11f126fa4f3130b6778bfe7e7d0f025a638f06e 
> 
> Diff: https://reviews.apache.org/r/53791/diff/
> 
> 
> Testing
> -------
> 
> sudo make check (Fedora 24)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 53791: Use the stout ELF parser to collect Linux rootfs files.

Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53791/
-----------------------------------------------------------

(Updated Feb. 4, 2017, 12:44 a.m.)


Review request for mesos, Gilbert Song, Jie Yu, Kevin Klues, and Jiang Yan Xu.


Bugs: MESOS-6588
    https://issues.apache.org/jira/browse/MESOS-6588


Repository: mesos


Description
-------

The dependencies for the programs we need in the Linux root
filesystem vary over time and across distributions. Since Stout
has support for parsing the library dependencies of ELF binaries,
use this to collect the necessary dependencies when constructing
a root filesystem for the tests.


Diffs (updated)
-----

  src/tests/containerizer/rootfs.hpp 9648638c1d4b1f1c49708e058e9b930c5ae32138 
  src/tests/containerizer/rootfs.cpp f11f126fa4f3130b6778bfe7e7d0f025a638f06e 

Diff: https://reviews.apache.org/r/53791/diff/


Testing
-------

sudo make check (Fedora 24)


Thanks,

James Peach

Re: Review Request 53791: Use the stout ELF parser to collect Linux rootfs files.

Posted by Jiang Yan Xu <xu...@apple.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53791/#review164172
-----------------------------------------------------------


Fix it, then Ship it!




Just a few nits.


src/tests/containerizer/rootfs.cpp (lines 62 - 63)
<https://reviews.apache.org/r/53791/#comment235767>

    This is not a valid indentation style.
    
    ```
    return Error(
        "Failed to resolve '" + path + "': " + target.error());
    ```
    
    or 
    
    ```
    return Error("Failed to resolve '" + path + "': " +
                 target.error());
    ```
    
    here and below.



src/tests/containerizer/rootfs.cpp (line 79)
<https://reviews.apache.org/r/53791/#comment235811>

    In Mesos full spelling of variable names is prefered. 
    
    s/src/source/
    s/dst/destination/



src/tests/containerizer/rootfs.cpp (lines 89 - 90)
<https://reviews.apache.org/r/53791/#comment235828>

    Given the comment above:
    
    `destinationDirectory` is at least no worse than: https://github.com/apache/mesos/blob/3a537a1d18e500fb1efe2f4d4f0718fb25c51848/src/launcher/fetcher.cpp#L65
    
    `destinationFullPath` slightly better? (i.e., what are the differences between destination vs. destinationPath?)


- Jiang Yan Xu


On Feb. 2, 2017, 9:32 a.m., James Peach wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53791/
> -----------------------------------------------------------
> 
> (Updated Feb. 2, 2017, 9:32 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, Kevin Klues, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6588
>     https://issues.apache.org/jira/browse/MESOS-6588
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The dependencies for the programs we need in the Linux root
> filesystem vary over time and across distributions. Since Stout
> has support for parsing the library dependencies of ELF binaries,
> use this to collect the necessary dependencies when constructing
> a root filesystem for the tests.
> 
> 
> Diffs
> -----
> 
>   src/tests/containerizer/rootfs.hpp 9648638c1d4b1f1c49708e058e9b930c5ae32138 
>   src/tests/containerizer/rootfs.cpp f11f126fa4f3130b6778bfe7e7d0f025a638f06e 
> 
> Diff: https://reviews.apache.org/r/53791/diff/
> 
> 
> Testing
> -------
> 
> sudo make check (Fedora 24)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 53791: Use the stout ELF parser to collect Linux rootfs files.

Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53791/#review164013
-----------------------------------------------------------



Patch looks great!

Reviews applied: [53790, 54712, 54878, 53791]

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 Feb. 2, 2017, 5:32 p.m., James Peach wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53791/
> -----------------------------------------------------------
> 
> (Updated Feb. 2, 2017, 5:32 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, Kevin Klues, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6588
>     https://issues.apache.org/jira/browse/MESOS-6588
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The dependencies for the programs we need in the Linux root
> filesystem vary over time and across distributions. Since Stout
> has support for parsing the library dependencies of ELF binaries,
> use this to collect the necessary dependencies when constructing
> a root filesystem for the tests.
> 
> 
> Diffs
> -----
> 
>   src/tests/containerizer/rootfs.hpp 9648638c1d4b1f1c49708e058e9b930c5ae32138 
>   src/tests/containerizer/rootfs.cpp f11f126fa4f3130b6778bfe7e7d0f025a638f06e 
> 
> Diff: https://reviews.apache.org/r/53791/diff/
> 
> 
> Testing
> -------
> 
> sudo make check (Fedora 24)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 53791: Use the stout ELF parser to collect Linux rootfs files.

Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53791/
-----------------------------------------------------------

(Updated Feb. 2, 2017, 5:32 p.m.)


Review request for mesos, Gilbert Song, Jie Yu, Kevin Klues, and Jiang Yan Xu.


Changes
-------

Addressed review feedback.


Bugs: MESOS-6588
    https://issues.apache.org/jira/browse/MESOS-6588


Repository: mesos


Description
-------

The dependencies for the programs we need in the Linux root
filesystem vary over time and across distributions. Since Stout
has support for parsing the library dependencies of ELF binaries,
use this to collect the necessary dependencies when constructing
a root filesystem for the tests.


Diffs (updated)
-----

  src/tests/containerizer/rootfs.hpp 9648638c1d4b1f1c49708e058e9b930c5ae32138 
  src/tests/containerizer/rootfs.cpp f11f126fa4f3130b6778bfe7e7d0f025a638f06e 

Diff: https://reviews.apache.org/r/53791/diff/


Testing
-------

sudo make check (Fedora 24)


Thanks,

James Peach