You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Greg Mann <gr...@mesosphere.io> on 2015/08/21 18:46:56 UTC

Review Request 37684: Added symlink test for /bin, lib, and /lib64.

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

Review request for mesos, Marco Massenzio and Vinod Kone.


Bugs: MESOS-3296 and MESOS-3297
    https://issues.apache.org/jira/browse/MESOS-3296
    https://issues.apache.org/jira/browse/MESOS-3297


Repository: mesos


Description
-------

Added symlink test for /bin, /lib, and /lib64.


Diffs
-----

  src/tests/containerizer/rootfs.hpp 961003fd6768ee0f7effd2804d1c453f8d45e058 

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


Testing
-------

On CentOS 7.1 and other distros with symlinked /bin, /lib, & /lib64:

sudo make check

Some tests will fail due to other root test issues being tracked at MESOS-3292, MESOS-3293, MESOS-3294, MESOS-3295

This patch solves bugs in MesosContainerizerLaunchTest.ROOT_ChangeRootfs and the LinuxFilesystemIsolatorTest.* tests, so those should all pass.


Thanks,

Greg Mann

Re: Review Request 37684: Added symlink test for /bin, lib, and /lib64.

Posted by Greg Mann <gr...@mesosphere.io>.

> On Aug. 21, 2015, 6:14 p.m., Jie Yu wrote:
> > src/tests/containerizer/rootfs.hpp, lines 111-126
> > <https://reviews.apache.org/r/37684/diff/1/?file=1046779#file1046779line111>
> >
> >     Can you combine the this loop with the loop below (like the following). Also, could you use 'realpath' here to resolve the symlink.
> >     
> >     ```
> >     foreach (const std::string& directory, directories) {
> >       // Some linux distros are moving all binaries and libraries
> >       // to /usr, in which case /bin, /lib, and /lib64 will be
> >       // symlinks to their equivalent directories in /usr. So we
> >       // call the realpath here first. 
> >       Result<std::string> realpath = os::realpath(directory);
> >       if (!realpath.isSome()) {
> >         return Error("Failed to get realpath for '" + directory + "': " +
> >                      (realpath.isError() ? realpath.error() : "No such directory");
> >       }
> >       
> >       Try<Nothing> result = rootfs->add(realpath.get());
> >       ...
> >     }
> >     ```

Thanks Jie, that's much better! :-) Comments addressed.


- Greg


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


On Aug. 21, 2015, 7:15 p.m., Greg Mann wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37684/
> -----------------------------------------------------------
> 
> (Updated Aug. 21, 2015, 7:15 p.m.)
> 
> 
> Review request for mesos, Marco Massenzio and Vinod Kone.
> 
> 
> Bugs: MESOS-3296 and MESOS-3297
>     https://issues.apache.org/jira/browse/MESOS-3296
>     https://issues.apache.org/jira/browse/MESOS-3297
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added symlink test for /bin, /lib, and /lib64.
> 
> 
> Diffs
> -----
> 
>   src/tests/containerizer/rootfs.hpp 961003fd6768ee0f7effd2804d1c453f8d45e058 
> 
> Diff: https://reviews.apache.org/r/37684/diff/
> 
> 
> Testing
> -------
> 
> On CentOS 7.1 and other distros with symlinked /bin, /lib, & /lib64:
> 
> "sudo make check"
> 
> 
> Some tests will fail due to other root test issues being tracked at MESOS-3292, MESOS-3293, MESOS-3294, MESOS-3295
> 
> This patch solves bugs in MesosContainerizerLaunchTest.ROOT_ChangeRootfs and the LinuxFilesystemIsolatorTest.* tests, so those should all pass.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 37684: Added symlink test for /bin, lib, and /lib64.

Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37684/#review96068
-----------------------------------------------------------

Ship it!


Thanks


src/tests/containerizer/rootfs.hpp (lines 111 - 126)
<https://reviews.apache.org/r/37684/#comment151272>

    Can you combine the this loop with the loop below (like the following). Also, could you use 'realpath' here to resolve the symlink.
    
    ```
    foreach (const std::string& directory, directories) {
      // Some linux distros are moving all binaries and libraries
      // to /usr, in which case /bin, /lib, and /lib64 will be
      // symlinks to their equivalent directories in /usr. So we
      // call the realpath here first. 
      Result<std::string> realpath = os::realpath(directory);
      if (!realpath.isSome()) {
        return Error("Failed to get realpath for '" + directory + "': " +
                     (realpath.isError() ? realpath.error() : "No such directory");
      }
      
      Try<Nothing> result = rootfs->add(realpath.get());
      ...
    }
    ```


- Jie Yu


On Aug. 21, 2015, 4:51 p.m., Greg Mann wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37684/
> -----------------------------------------------------------
> 
> (Updated Aug. 21, 2015, 4:51 p.m.)
> 
> 
> Review request for mesos, Marco Massenzio and Vinod Kone.
> 
> 
> Bugs: MESOS-3296 and MESOS-3297
>     https://issues.apache.org/jira/browse/MESOS-3296
>     https://issues.apache.org/jira/browse/MESOS-3297
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added symlink test for /bin, /lib, and /lib64.
> 
> 
> Diffs
> -----
> 
>   src/tests/containerizer/rootfs.hpp 961003fd6768ee0f7effd2804d1c453f8d45e058 
> 
> Diff: https://reviews.apache.org/r/37684/diff/
> 
> 
> Testing
> -------
> 
> On CentOS 7.1 and other distros with symlinked /bin, /lib, & /lib64:
> 
> "sudo make check"
> 
> 
> Some tests will fail due to other root test issues being tracked at MESOS-3292, MESOS-3293, MESOS-3294, MESOS-3295
> 
> This patch solves bugs in MesosContainerizerLaunchTest.ROOT_ChangeRootfs and the LinuxFilesystemIsolatorTest.* tests, so those should all pass.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 37684: Added symlink test for /bin, lib, and /lib64.

Posted by Marco Massenzio <ma...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37684/#review96054
-----------------------------------------------------------

Ship it!


Ship It!

- Marco Massenzio


On Aug. 21, 2015, 4:51 p.m., Greg Mann wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37684/
> -----------------------------------------------------------
> 
> (Updated Aug. 21, 2015, 4:51 p.m.)
> 
> 
> Review request for mesos, Marco Massenzio and Vinod Kone.
> 
> 
> Bugs: MESOS-3296 and MESOS-3297
>     https://issues.apache.org/jira/browse/MESOS-3296
>     https://issues.apache.org/jira/browse/MESOS-3297
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added symlink test for /bin, /lib, and /lib64.
> 
> 
> Diffs
> -----
> 
>   src/tests/containerizer/rootfs.hpp 961003fd6768ee0f7effd2804d1c453f8d45e058 
> 
> Diff: https://reviews.apache.org/r/37684/diff/
> 
> 
> Testing
> -------
> 
> On CentOS 7.1 and other distros with symlinked /bin, /lib, & /lib64:
> 
> "sudo make check"
> 
> 
> Some tests will fail due to other root test issues being tracked at MESOS-3292, MESOS-3293, MESOS-3294, MESOS-3295
> 
> This patch solves bugs in MesosContainerizerLaunchTest.ROOT_ChangeRootfs and the LinuxFilesystemIsolatorTest.* tests, so those should all pass.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 37684: Added symlink test for /bin, lib, and /lib64.

Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37684/
-----------------------------------------------------------

(Updated Aug. 21, 2015, 7:15 p.m.)


Review request for mesos, Marco Massenzio and Vinod Kone.


Bugs: MESOS-3296 and MESOS-3297
    https://issues.apache.org/jira/browse/MESOS-3296
    https://issues.apache.org/jira/browse/MESOS-3297


Repository: mesos


Description
-------

Added symlink test for /bin, /lib, and /lib64.


Diffs (updated)
-----

  src/tests/containerizer/rootfs.hpp 961003fd6768ee0f7effd2804d1c453f8d45e058 

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


Testing
-------

On CentOS 7.1 and other distros with symlinked /bin, /lib, & /lib64:

"sudo make check"


Some tests will fail due to other root test issues being tracked at MESOS-3292, MESOS-3293, MESOS-3294, MESOS-3295

This patch solves bugs in MesosContainerizerLaunchTest.ROOT_ChangeRootfs and the LinuxFilesystemIsolatorTest.* tests, so those should all pass.


Thanks,

Greg Mann

Re: Review Request 37684: Added symlink test for /bin, lib, and /lib64.

Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37684/
-----------------------------------------------------------

(Updated Aug. 21, 2015, 4:51 p.m.)


Review request for mesos, Marco Massenzio and Vinod Kone.


Bugs: MESOS-3296 and MESOS-3297
    https://issues.apache.org/jira/browse/MESOS-3296
    https://issues.apache.org/jira/browse/MESOS-3297


Repository: mesos


Description
-------

Added symlink test for /bin, /lib, and /lib64.


Diffs
-----

  src/tests/containerizer/rootfs.hpp 961003fd6768ee0f7effd2804d1c453f8d45e058 

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


Testing (updated)
-------

On CentOS 7.1 and other distros with symlinked /bin, /lib, & /lib64:

"sudo make check"


Some tests will fail due to other root test issues being tracked at MESOS-3292, MESOS-3293, MESOS-3294, MESOS-3295

This patch solves bugs in MesosContainerizerLaunchTest.ROOT_ChangeRootfs and the LinuxFilesystemIsolatorTest.* tests, so those should all pass.


Thanks,

Greg Mann