You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Jason Lai <ja...@jasonlai.net> on 2018/03/05 22:01:07 UTC
Review Request 65900: Defer creation of volume target paths to
container launch.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65900/
-----------------------------------------------------------
Review request for mesos, Anish Gupta, Eric Chung, Gilbert Song, Jie Yu, James Peach, and Zhitao Li.
Bugs: MESOS-8257
https://issues.apache.org/jira/browse/MESOS-8257
Repository: mesos
Description
-------
Defer creation of volume target paths to container launch.
Diffs
-----
src/Makefile.am a2ba1e6de0868c7a4fe58304ec479dce039bc288
src/slave/containerizer/mesos/isolators/volume/host_path.cpp 9127cf4a9d8aa2b16bb5b9903103f1f76a1e2b1a
src/slave/containerizer/mesos/isolators/volume/image.cpp 8ca3d55d5792ed1cfc8e49df40587ac2abc83fec
src/slave/containerizer/mesos/isolators/volume/utils.hpp PRE-CREATION
Diff: https://reviews.apache.org/r/65900/diff/1/
Testing
-------
Thanks,
Jason Lai
Re: Review Request 65900: Defer creation of volume target paths to
container launch.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65900/#review198663
-----------------------------------------------------------
PASS: Mesos patch 65900 was successfully built and tested.
Reviews applied: `['65811', '65812', '65898', '65899', '65900']`
All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/65900
- Mesos Reviewbot Windows
On March 5, 2018, 2:01 p.m., Jason Lai wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65900/
> -----------------------------------------------------------
>
> (Updated March 5, 2018, 2:01 p.m.)
>
>
> Review request for mesos, Anish Gupta, Eric Chung, Gilbert Song, Jie Yu, James Peach, and Zhitao Li.
>
>
> Bugs: MESOS-8257
> https://issues.apache.org/jira/browse/MESOS-8257
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Defer creation of volume target paths to container launch.
>
>
> Diffs
> -----
>
> src/Makefile.am a2ba1e6de0868c7a4fe58304ec479dce039bc288
> src/slave/containerizer/mesos/isolators/volume/host_path.cpp 9127cf4a9d8aa2b16bb5b9903103f1f76a1e2b1a
> src/slave/containerizer/mesos/isolators/volume/image.cpp 8ca3d55d5792ed1cfc8e49df40587ac2abc83fec
> src/slave/containerizer/mesos/isolators/volume/utils.hpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/65900/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jason Lai
>
>
Re: Review Request 65900: Defer creation of volume target paths to
container launch.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65900/#review203611
-----------------------------------------------------------
This commit contains both refactoring and functional changes. We should split the refactoring into a separate patch so that they can be reviewed and tested independently. The commit message for that should explain the context of why the refactoring is necessary.
Once that is done, then we can make the functional changes in this patch, but I think the ordering is wrong. This patch removes the code that creates the mount points, and that functionality is not restored until later in the series. We need to add all the necessary support earlier in the series, then switch the callers over to it in a single patch so that there is never a commit where things are not working.
Once you tackle the above points, I think that the commit message for this patch needs a lot more detail. We need to explain the context of this change and give the reader information about why this change is important (ie. what is the benefit of making the change) and some more concrete information about what is being changed.
src/slave/containerizer/mesos/isolators/volume/utils.hpp
Lines 18 (patched)
<https://reviews.apache.org/r/65900/#comment285900>
There's no need for this to be header-only, let's make a .cpp and a .hpp.
src/slave/containerizer/mesos/isolators/volume/utils.hpp
Lines 40 (patched)
<https://reviews.apache.org/r/65900/#comment285901>
This really boils down to separate code paths depending on whether there is a `rootfs`. Can we break this into 2 separate functions, mirroring the original code? That's easier to understand, since we don't have a function that is performing double-duty.
- James Peach
On May 17, 2018, 1:16 a.m., Jason Lai wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65900/
> -----------------------------------------------------------
>
> (Updated May 17, 2018, 1:16 a.m.)
>
>
> Review request for mesos, Anish Gupta, Eric Chung, Gilbert Song, Jie Yu, James Peach, and Zhitao Li.
>
>
> Bugs: MESOS-8257
> https://issues.apache.org/jira/browse/MESOS-8257
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Defer creation of volume target paths to container launch.
>
>
> Diffs
> -----
>
> src/Makefile.am c08ac6e2f5deec4d05f59f71ff6c51382f216708
> src/slave/containerizer/mesos/isolators/volume/host_path.cpp 9127cf4a9d8aa2b16bb5b9903103f1f76a1e2b1a
> src/slave/containerizer/mesos/isolators/volume/image.cpp 631553e2f61a1b95dd93d333b547ff237f65f59e
> src/slave/containerizer/mesos/isolators/volume/sandbox_path.cpp e0cae1036e2e49b4f61705c77f31ae166d1b1380
> src/slave/containerizer/mesos/isolators/volume/utils.hpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/65900/diff/3/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jason Lai
>
>
Re: Review Request 65900: Defer creation of volume target paths to
container launch.
Posted by Jason Lai <ja...@jasonlai.net>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65900/
-----------------------------------------------------------
(Updated May 17, 2018, 1:16 a.m.)
Review request for mesos, Anish Gupta, Eric Chung, Gilbert Song, Jie Yu, James Peach, and Zhitao Li.
Changes
-------
* Rebased to the latest `master` branch;
* Renamed `getAbsoluteMountPoint` to `getAbsolutePathForMountPoint`;
* Removed references to `ContainerConfig` in `getAbsolutePathForMountPoint`;
* Minor refactors on `isolators/volume/sandbox_path.cpp`, some code extracted to `isolators/volume/utils.cpp`;
Bugs: MESOS-8257
https://issues.apache.org/jira/browse/MESOS-8257
Repository: mesos
Description
-------
Defer creation of volume target paths to container launch.
Diffs (updated)
-----
src/Makefile.am c08ac6e2f5deec4d05f59f71ff6c51382f216708
src/slave/containerizer/mesos/isolators/volume/host_path.cpp 9127cf4a9d8aa2b16bb5b9903103f1f76a1e2b1a
src/slave/containerizer/mesos/isolators/volume/image.cpp 631553e2f61a1b95dd93d333b547ff237f65f59e
src/slave/containerizer/mesos/isolators/volume/sandbox_path.cpp e0cae1036e2e49b4f61705c77f31ae166d1b1380
src/slave/containerizer/mesos/isolators/volume/utils.hpp PRE-CREATION
Diff: https://reviews.apache.org/r/65900/diff/3/
Changes: https://reviews.apache.org/r/65900/diff/2-3/
Testing
-------
Thanks,
Jason Lai
Re: Review Request 65900: Defer creation of volume target paths to
container launch.
Posted by Jason Lai <ja...@jasonlai.net>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65900/#review203156
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/volume/utils.hpp
Lines 39-69 (patched)
<https://reviews.apache.org/r/65900/#comment285233>
@jieyu: If you really think we should get rid of `containerConfig` from `getAbsoluteMountPoint`, we can do things this way:
```
inline Try<std::string> getAbsolutePathForMountPoint(
const std::string& mountPoint,
const Option<std::string>& rootfs,
const std::string& containerSandboxPath,
const std::string& hostSandboxPath)
{
if (path::absolute(mountPoint)) {
if (rootfs.isSome()) {
return path::join(rootfs.get(), mountPoint);
}
if (!os::exists(mountPoint)) {
return Error(
"Mount point '" + mountPoint + "' is an absolute path. "
"It must exist if the container shares the host filesystem");
}
return mountPoint;
}
if (rootfs.isSome()) {
return path::join(rootfs.get(), containerSandboxPath, mountPoint);
}
return path::join(hostSandboxPath, mountPoint);
}
```
- Jason Lai
On May 8, 2018, 7:07 p.m., Jason Lai wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65900/
> -----------------------------------------------------------
>
> (Updated May 8, 2018, 7:07 p.m.)
>
>
> Review request for mesos, Anish Gupta, Eric Chung, Gilbert Song, Jie Yu, James Peach, and Zhitao Li.
>
>
> Bugs: MESOS-8257
> https://issues.apache.org/jira/browse/MESOS-8257
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Defer creation of volume target paths to container launch.
>
>
> Diffs
> -----
>
> src/Makefile.am a2ba1e6de0868c7a4fe58304ec479dce039bc288
> src/slave/containerizer/mesos/isolators/volume/host_path.cpp 9127cf4a9d8aa2b16bb5b9903103f1f76a1e2b1a
> src/slave/containerizer/mesos/isolators/volume/image.cpp 8ca3d55d5792ed1cfc8e49df40587ac2abc83fec
> src/slave/containerizer/mesos/isolators/volume/utils.hpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/65900/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jason Lai
>
>
Re: Review Request 65900: Defer creation of volume target paths to
container launch.
Posted by Jason Lai <ja...@jasonlai.net>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65900/
-----------------------------------------------------------
(Updated May 8, 2018, 7:07 p.m.)
Review request for mesos, Anish Gupta, Eric Chung, Gilbert Song, Jie Yu, James Peach, and Zhitao Li.
Changes
-------
Refactored and extracted bidirectional mount propagation code to `utils.hpp`.
Bugs: MESOS-8257
https://issues.apache.org/jira/browse/MESOS-8257
Repository: mesos
Description
-------
Defer creation of volume target paths to container launch.
Diffs (updated)
-----
src/Makefile.am a2ba1e6de0868c7a4fe58304ec479dce039bc288
src/slave/containerizer/mesos/isolators/volume/host_path.cpp 9127cf4a9d8aa2b16bb5b9903103f1f76a1e2b1a
src/slave/containerizer/mesos/isolators/volume/image.cpp 8ca3d55d5792ed1cfc8e49df40587ac2abc83fec
src/slave/containerizer/mesos/isolators/volume/utils.hpp PRE-CREATION
Diff: https://reviews.apache.org/r/65900/diff/2/
Changes: https://reviews.apache.org/r/65900/diff/1-2/
Testing
-------
Thanks,
Jason Lai
Re: Review Request 65900: Defer creation of volume target paths to
container launch.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65900/#review198676
-----------------------------------------------------------
Patch looks great!
Reviews applied: [65811, 65812, 65898, 65899, 65900]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On March 5, 2018, 10:01 p.m., Jason Lai wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65900/
> -----------------------------------------------------------
>
> (Updated March 5, 2018, 10:01 p.m.)
>
>
> Review request for mesos, Anish Gupta, Eric Chung, Gilbert Song, Jie Yu, James Peach, and Zhitao Li.
>
>
> Bugs: MESOS-8257
> https://issues.apache.org/jira/browse/MESOS-8257
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Defer creation of volume target paths to container launch.
>
>
> Diffs
> -----
>
> src/Makefile.am a2ba1e6de0868c7a4fe58304ec479dce039bc288
> src/slave/containerizer/mesos/isolators/volume/host_path.cpp 9127cf4a9d8aa2b16bb5b9903103f1f76a1e2b1a
> src/slave/containerizer/mesos/isolators/volume/image.cpp 8ca3d55d5792ed1cfc8e49df40587ac2abc83fec
> src/slave/containerizer/mesos/isolators/volume/utils.hpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/65900/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jason Lai
>
>