Re: Review Request 71024: Supported insecure registry when provisioning docker images.
> On July 9, 2019, 5:44 a.m., Jie Yu wrote: > > Can you add some unit test for this? For instance, toggle the new agent > > flag, test with some docker registry (ideally, an unsecure registry, but I > > think it's ok to test with a secure one too just to excersize the code > > path). > > fei long wrote: > I started a local registry(127.0.0.1:5000) in a docker container and the > test case passed. But I am not sure how to redo it in the CI environment > since I need to > 1. start a "registry" docker container > 2. make sure that 127.0.0.1 is not in the "insecure registries" list. > > Could you give some advice? you don't necessarily need to test with a insecure registry. we should add a test to exercise the case where this agent flag is toggled. You can still use dockerhub for that. - Jie --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71024/#review216441 --- On July 9, 2019, 5:44 a.m., fei long wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71024/ > --- > > (Updated July 9, 2019, 5:44 a.m.) > > > Review request for mesos, Gilbert Song, Jie Yu, and Qian Zhang. > > > Bugs: MESOS-6183 > https://issues.apache.org/jira/browse/MESOS-6183 > > > Repository: mesos > > > Description > --- > > Supported insecure registry when provisioning docker images. > > > Diffs > - > > src/slave/containerizer/mesos/provisioner/docker/puller.cpp > d013c9d71c39c09e600f181aba31b8037aa9226a > src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp > 35b6afbb6b22575b90963927352443a8ddaf9885 > src/slave/containerizer/mesos/provisioner/docker/store.cpp > 286ee5f00933b4c86a96dc4e10e42f9e7eac0ce2 > src/slave/flags.hpp a10bf698dc447b1411c06083c2ba7585d7a78389 > src/slave/flags.cpp b4e3eb99221a09404dbbf813da33607867a78691 > src/slave/http.cpp 69e6d74e8b113cc6c937f47df8984ff9a63e5bb4 > src/uri/fetchers/docker.hpp 2bb921474f3a8147d8cbf54579452f8df216d2f1 > src/uri/fetchers/docker.cpp 8f5fc964f056b349ce57ced139e07f538cb1cfd2 > > > Diff: https://reviews.apache.org/r/71024/diff/1/ > > > Testing > --- > > All testing cases passed. > > > Thanks, > > fei long > >
Re: Review Request 71024: Supported insecure registry when provisioning docker images.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71024/#review216441 --- Can you add some unit test for this? For instance, toggle the new agent flag, test with some docker registry (ideally, an unsecure registry, but I think it's ok to test with a secure one too just to excersize the code path). src/slave/flags.hpp Lines 61 (patched) <https://reviews.apache.org/r/71024/#comment303682> I would rather make this a boolean - Jie Yu On July 9, 2019, 3:24 a.m., fei long wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71024/ > --- > > (Updated July 9, 2019, 3:24 a.m.) > > > Review request for mesos and Jie Yu. > > > Bugs: MESOS-6183 > https://issues.apache.org/jira/browse/MESOS-6183 > > > Repository: mesos > > > Description > --- > > Supported insecure registry when provisioning docker images. > > > Diffs > - > > src/slave/containerizer/mesos/provisioner/docker/puller.cpp > d013c9d71c39c09e600f181aba31b8037aa9226a > src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp > 35b6afbb6b22575b90963927352443a8ddaf9885 > src/slave/containerizer/mesos/provisioner/docker/store.cpp > 286ee5f00933b4c86a96dc4e10e42f9e7eac0ce2 > src/slave/flags.hpp a10bf698dc447b1411c06083c2ba7585d7a78389 > src/slave/flags.cpp b4e3eb99221a09404dbbf813da33607867a78691 > src/slave/http.cpp 69e6d74e8b113cc6c937f47df8984ff9a63e5bb4 > src/uri/fetchers/docker.hpp 2bb921474f3a8147d8cbf54579452f8df216d2f1 > src/uri/fetchers/docker.cpp 8f5fc964f056b349ce57ced139e07f538cb1cfd2 > > > Diff: https://reviews.apache.org/r/71024/diff/1/ > > > Testing > --- > > All testing cases passed. > > > Thanks, > > fei long > >
Re: Review Request 71005: Used euid to determine subprocess' user when launching tasks.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71005/#review216440 --- Ship it! Ship It! - Jie Yu On July 3, 2019, 11:12 a.m., fei long wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71005/ > --- > > (Updated July 3, 2019, 11:12 a.m.) > > > Review request for mesos and Jie Yu. > > > Bugs: mesos-9876 > https://issues.apache.org/jira/browse/mesos-9876 > > > Repository: mesos > > > Description > --- > > Used euid to determine subprocess' user when launching tasks. > > > Diffs > - > > src/slave/containerizer/mesos/launch.cpp > b29ec556399a40aa662987a11a2b64e6a16de889 > > > Diff: https://reviews.apache.org/r/71005/diff/1/ > > > Testing > --- > > All testing cases passed. > > > Thanks, > > fei long > >
Re: Review Request 70712: Added filesystem operations to the `ContainerLaunchInfo`.
> On May 24, 2019, 10:45 p.m., Jiang Yan Xu wrote:
> > include/mesos/slave/containerizer.proto
> > Lines 207 (patched)
> > <https://reviews.apache.org/r/70712/diff/1/?file=2146613#file2146613line207>
> >
> > Nit: not all possible file operation have a source and a target? Would
> > something like
> >
> > ```
> > message ContainerFileOperation {
> > ...
> > message Symlink {
> > optional string target = 1;
> > }
> >
> > optional string path = 2;
> > ```
> >
> > make more sense?
Yeah, I think this is a good point! Maybe we should just introduce inline types
specifically for Symlink and kill top level fields
- Jie
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70712/#review215515
---
On May 24, 2019, 6:46 a.m., James Peach wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70712/
> -----------
>
> (Updated May 24, 2019, 6:46 a.m.)
>
>
> Review request for mesos, Xudong Ni, Gilbert Song, Jie Yu, Jacob Janco, and
> Jiang Yan Xu.
>
>
> Bugs: MESOS-9769
> https://issues.apache.org/jira/browse/MESOS-9769
>
>
> Repository: mesos
>
>
> Description
> ---
>
> The `filesystem/linux` isolator was using pre-exec commands
> to set up Linux ABI symlinks. Not only is this inefficient,
> it has the undesirable security property of running programs
> in a user-controlled container image.
>
> The fix added a new `ContainerFileOperation` message to the
> containerizer launch information. The containerizer executes
> the requested file operation after performing the container
> mounts.
>
>
> Diffs
> -
>
> include/mesos/slave/containerizer.proto
> e9924489000efabebd55bf070f18149f23e4a510
> src/common/protobuf_utils.hpp 273ae270695db33b6c9d8b32cb38f8840a815787
> src/common/protobuf_utils.cpp 8b252cb11e17356836988dfc44a63953579a1def
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp
> 8f94453a7354927ae918d3f2fd746cdf5ef63cb7
> src/slave/containerizer/mesos/isolators/filesystem/linux.cpp
> 190054c26b949aa9ba0f49377b77d9e472edb95a
> src/slave/containerizer/mesos/launch.cpp
> 5ddb4c7d998c17b59164825acc0627a1311b691b
>
>
> Diff: https://reviews.apache.org/r/70712/diff/1/
>
>
> Testing
> ---
>
> sudo make check (Fedora 30)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 70712: Added filesystem operations to the `ContainerLaunchInfo`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70712/#review215514 --- Fix it, then Ship it! This LGTM! Thanks James! include/mesos/slave/containerizer.proto Lines 272 (patched) <https://reviews.apache.org/r/70712/#comment302248> typo `conttainer` - Jie Yu On May 24, 2019, 6:46 a.m., James Peach wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70712/ > --- > > (Updated May 24, 2019, 6:46 a.m.) > > > Review request for mesos, Xudong Ni, Gilbert Song, Jie Yu, Jacob Janco, and > Jiang Yan Xu. > > > Bugs: MESOS-9769 > https://issues.apache.org/jira/browse/MESOS-9769 > > > Repository: mesos > > > Description > --- > > The `filesystem/linux` isolator was using pre-exec commands > to set up Linux ABI symlinks. Not only is this inefficient, > it has the undesirable security property of running programs > in a user-controlled container image. > > The fix added a new `ContainerFileOperation` message to the > containerizer launch information. The containerizer executes > the requested file operation after performing the container > mounts. > > > Diffs > - > > include/mesos/slave/containerizer.proto > e9924489000efabebd55bf070f18149f23e4a510 > src/common/protobuf_utils.hpp 273ae270695db33b6c9d8b32cb38f8840a815787 > src/common/protobuf_utils.cpp 8b252cb11e17356836988dfc44a63953579a1def > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp > 8f94453a7354927ae918d3f2fd746cdf5ef63cb7 > src/slave/containerizer/mesos/isolators/filesystem/linux.cpp > 190054c26b949aa9ba0f49377b77d9e472edb95a > src/slave/containerizer/mesos/launch.cpp > 5ddb4c7d998c17b59164825acc0627a1311b691b > > > Diff: https://reviews.apache.org/r/70712/diff/1/ > > > Testing > --- > > sudo make check (Fedora 30) > > > Thanks, > > James Peach > >
Review Request 70356: Mounted /proc properly a container shares pid namespace with its parent.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70356/ --- Review request for mesos, Gilbert Song, James Peach, and Qian Zhang. Bugs: MESOS-9529 https://issues.apache.org/jira/browse/MESOS-9529 Repository: mesos Description --- If a container shares the same pid namespace as its parent and is not a top level container. It might or might not share the same pid namespace as the agent. In this case, we need to re-mount `/proc`. One caveat here is that: in the case where this container does share the pid namespace of the agent (because its parent shares the same pid namespace of the agent), mounting `/proc` at the same place will result in EBUSY. As a result, we need to "move" (MS_MOVE) the mounts under `/proc` to a new location and mount the `/proc` again at the old location. See MESOS-9529 for details. Diffs - src/slave/containerizer/mesos/isolators/namespaces/pid.cpp 5df31227454c5865ad13c0c334168938c1bc6cad src/slave/containerizer/mesos/paths.hpp 2dc222e8db2e27a41b5dd1da01095005d76bcd80 src/slave/containerizer/mesos/paths.cpp 94ab921a79ff62bd43b5c72a368ec8b3e37ef110 Diff: https://reviews.apache.org/r/70356/diff/1/ Testing --- sudo make check Thanks, Jie Yu
Review Request 70355: Switched to used `/proc/1/ns/pid` to test pid namespaces.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70355/ --- Review request for mesos, Gilbert Song, James Peach, and Qian Zhang. Bugs: MESOS-9529 https://issues.apache.org/jira/browse/MESOS-9529 Repository: mesos Description --- Previously, we're using `/proc/self/ns/pid` to test pid namespaces. This is proven to be problematic because the kernel will resolve correctly even if the `/proc` is not re-mounted in the new pid namespace. Diffs - src/tests/containerizer/isolator_tests.cpp 5446dd9f9842c6ce100f7b2e4c2b120ebce41e43 src/tests/containerizer/nested_mesos_containerizer_tests.cpp ac9a6b803a6ba22a4e1fda96c825dbbfd779d915 src/tests/default_executor_tests.cpp 7d7b11383e955ebaecdc37bbde5ed39a36154e28 Diff: https://reviews.apache.org/r/70355/diff/1/ Testing --- sudo make check This test will fail on some PidNamespace tests, which will be fixed in a subsequent patch. Thanks, Jie Yu
Re: Review Request 70235: Fixed the persistent volume ownership to allow apps to chown.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70235/#review213788 --- Can you link a ticket to the bug and provide some brief description of the issue? - Jie Yu On March 18, 2019, 10:20 p.m., Gilbert Song wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70235/ > --- > > (Updated March 18, 2019, 10:20 p.m.) > > > Review request for mesos, Andrei Budnik, Chun-Hung Hsiao, Jie Yu, and Qian > Zhang. > > > Repository: mesos > > > Description > --- > > Fixed the persistent volume ownership to allow apps to chown. > > > Diffs > - > > src/slave/containerizer/mesos/isolators/filesystem/linux.cpp > 341853a2df74f6ec3135e942b59a5da9d8f8460e > > > Diff: https://reviews.apache.org/r/70235/diff/1/ > > > Testing > --- > > make check > > > Thanks, > > Gilbert Song > >
Re: Review Request 70017: Added a unit test to run Tensorflow on a GPU.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70017/#review213172 --- Ship it! Ship It! - Jie Yu On Feb. 20, 2019, 6:27 a.m., Chun-Hung Hsiao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70017/ > --- > > (Updated Feb. 20, 2019, 6:27 a.m.) > > > Review request for mesos, Gilbert Song, Jie Yu, and Kevin Klues. > > > Bugs: MESOS-9549 > https://issues.apache.org/jira/browse/MESOS-9549 > > > Repository: mesos > > > Description > --- > > Test `TensorflowGpuImage` leverages the Tensorflow GPU image, which is > based on the `nvidia/cuda` image, to launch a task that consumes a GPU. > > > Diffs > - > > src/tests/containerizer/nvidia_gpu_isolator_tests.cpp > 040453e8d9538cb0f534a8188a89c864d5ddef3f > src/tests/mesos.hpp b10ec0ad62efa8980ca12693939970c2252ee814 > > > Diff: https://reviews.apache.org/r/70017/diff/1/ > > > Testing > --- > > `sudo make check` on an EC2 P3 VM w/ CUDA 10. > > > Thanks, > > Chun-Hung Hsiao > >
Re: Review Request 70018: Avoided waiting for 180s when test `NvidiaDockerImage` fails.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70018/#review213173 --- Ship it! Ship It! - Jie Yu On Feb. 20, 2019, 6:27 a.m., Chun-Hung Hsiao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70018/ > --- > > (Updated Feb. 20, 2019, 6:27 a.m.) > > > Review request for mesos, Gilbert Song, Jie Yu, and Kevin Klues. > > > Repository: mesos > > > Description > --- > > Originally if test `NvidiaDockerImage` fails to wait for the expected > `TASK_FINISHED` and `TASK_FAILED` states, it would wait for 180s so the > `AWAIT_READY` would time out. This patch fixes this issue by matching > any terminal task status updates. > > > Diffs > - > > src/tests/containerizer/nvidia_gpu_isolator_tests.cpp > 040453e8d9538cb0f534a8188a89c864d5ddef3f > > > Diff: https://reviews.apache.org/r/70018/diff/1/ > > > Testing > --- > > sudo make check > > > Thanks, > > Chun-Hung Hsiao > >
Re: Review Request 70016: Supported CUDA 10+ images that are based on nvidia-container-runtime.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70016/#review213171 --- Fix it, then Ship it! src/slave/containerizer/mesos/isolators/gpu/volume.cpp Lines 493 (patched) <https://reviews.apache.org/r/70016/#comment299042> Can you also add a TODO here about relying on `NVIDIA_DRIVER_CAPABILITIES` env var instead in the future? - Jie Yu On Feb. 21, 2019, 11:03 p.m., Chun-Hung Hsiao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70016/ > --- > > (Updated Feb. 21, 2019, 11:03 p.m.) > > > Review request for mesos, Gilbert Song, Jie Yu, and Kevin Klues. > > > Bugs: MESOS-9549 > https://issues.apache.org/jira/browse/MESOS-9549 > > > Repository: mesos > > > Description > --- > > Nvidia's CUDA 10+ images are based on nvidia-container-runtime and thus > the runtime injection are from the images themselves. To adapt this > change, we adjusted the binaries and libraries and injected the `PATH` > and `LD_LIBRARY_PATH` environment variables in the `gpu/nvidia` > isolator. > > > Diffs > - > > src/slave/containerizer/mesos/isolators/gpu/isolator.cpp > f39e7c3d1ccfe097116fe59b05c22fbb3f83b698 > src/slave/containerizer/mesos/isolators/gpu/volume.hpp > e71fe95234ff10c72cfaa4ad39591f70a531c383 > src/slave/containerizer/mesos/isolators/gpu/volume.cpp > 0d0d778d6a8467c1ac87286e75d47faf8243afa4 > > > Diff: https://reviews.apache.org/r/70016/diff/5/ > > > Testing > --- > > `sudo make check` > > More testing done later in chain. > > > Thanks, > > Chun-Hung Hsiao > >
Review Request 70037: Bumped SVN yum repo URL.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70037/ --- Review request for mesos, James DeFelice and Vinod Kone. Repository: mesos Description --- SVN 1.9 has been removed from the yum repo. This patch bumps it to SVN 1.11. Diffs - support/packaging/centos/centos6.dockerfile 457f48de8390988c88581683a67bf9e023e2117c support/packaging/centos/centos7.dockerfile b69cf3e6dfb098beffb5114f92a11105319ac303 Diff: https://reviews.apache.org/r/70037/diff/1/ Testing --- tested locally by running support/packaging/centos/build-docker-rpmbuild.sh Thanks, Jie Yu
Re: Review Request 70016: Supported nvidia-docker 2.0 for CUDA 10+.
> On Feb. 20, 2019, 6:34 a.m., Jie Yu wrote: > > src/slave/containerizer/mesos/isolators/gpu/isolator.cpp > > Lines 418 (patched) > > <https://reviews.apache.org/r/70016/diff/1/?file=2125909#file2125909line418> > > > > Ideally, for PATH and LD_LIBRARY_PATH, we don't overwrite but append. > > Imagine a docker container that already defines LD_LIBRARY_PATH by itself. > > Doing this will probably break the image? > > > > Curious what's your thoughts on this. Discussed offline. I'll be fine with a TODO here. - Jie --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70016/#review212943 --- On Feb. 20, 2019, 6:26 a.m., Chun-Hung Hsiao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70016/ > --- > > (Updated Feb. 20, 2019, 6:26 a.m.) > > > Review request for mesos, Gilbert Song, Jie Yu, and Kevin Klues. > > > Bugs: MESOS-9549 > https://issues.apache.org/jira/browse/MESOS-9549 > > > Repository: mesos > > > Description > --- > > nvidia-docker 2.0, which is used by CUDA 10+, moves some of the runtime > injection that was originally done in the image to its new nvidia > container runtime. To adapt this change, we adjusted the binaries and > libraries and injected the `PATH` and `LD_LIBRARY_PATH` environment > variables in the `gpu/nvidia` isolator. > > > Diffs > - > > src/slave/containerizer/mesos/isolators/gpu/isolator.cpp > f39e7c3d1ccfe097116fe59b05c22fbb3f83b698 > src/slave/containerizer/mesos/isolators/gpu/volume.hpp > e71fe95234ff10c72cfaa4ad39591f70a531c383 > src/slave/containerizer/mesos/isolators/gpu/volume.cpp > 0d0d778d6a8467c1ac87286e75d47faf8243afa4 > > > Diff: https://reviews.apache.org/r/70016/diff/1/ > > > Testing > --- > > `sudo make check` > > More testing done later in chain. > > > Thanks, > > Chun-Hung Hsiao > >
Review Request 70025: Blacklisted the "ubuntu-4" Jenkins box.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70025/ --- Review request for mesos and James DeFelice. Repository: mesos Description --- The git version installed on the box is too low. Diffs - support/jenkins/Jenkinsfile-docker-centos 99575c93dcbe2988e4f67dff613f8cc635c8da27 support/jenkins/Jenkinsfile-mini 6790d8957e76c9c1f74abf2aa40f55cb733cb7c3 Diff: https://reviews.apache.org/r/70025/diff/1/ Testing --- n/a Thanks, Jie Yu
Review Request 70024: Failed the scripts if `--points-at` is not supported.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70024/ --- Review request for mesos and James DeFelice. Repository: mesos Description --- On some Jenkins boxes, the git installed on the box does not support `--points-at`. Instead of silently assume the 'master' branch in the scripts (which could be wrong), we fail hard here. Diffs - support/jenkins/docker-centos.sh c0612d0043aeafbc7ed325e8de513e7c4c7de1ef support/jenkins/mini.sh a012c6c35c4ae113eec541d93a8bc8005c566a75 Diff: https://reviews.apache.org/r/70024/diff/1/ Testing --- run the script locally. Thanks, Jie Yu
Re: Review Request 70016: Supported nvidia-docker 2.0 for CUDA 10+.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70016/#review212943 --- src/slave/containerizer/mesos/isolators/gpu/isolator.cpp Lines 418 (patched) <https://reviews.apache.org/r/70016/#comment298818> Ideally, for PATH and LD_LIBRARY_PATH, we don't overwrite but append. Imagine a docker container that already defines LD_LIBRARY_PATH by itself. Doing this will probably break the image? Curious what's your thoughts on this. - Jie Yu On Feb. 20, 2019, 6:26 a.m., Chun-Hung Hsiao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70016/ > --- > > (Updated Feb. 20, 2019, 6:26 a.m.) > > > Review request for mesos, Gilbert Song, Jie Yu, and Kevin Klues. > > > Bugs: MESOS-9549 > https://issues.apache.org/jira/browse/MESOS-9549 > > > Repository: mesos > > > Description > --- > > nvidia-docker 2.0, which is used by CUDA 10+, moves some of the runtime > injection that was originally done in the image to its new nvidia > container runtime. To adapt this change, we adjusted the binaries and > libraries and injected the `PATH` and `LD_LIBRARY_PATH` environment > variables in the `gpu/nvidia` isolator. > > > Diffs > - > > src/slave/containerizer/mesos/isolators/gpu/isolator.cpp > f39e7c3d1ccfe097116fe59b05c22fbb3f83b698 > src/slave/containerizer/mesos/isolators/gpu/volume.hpp > e71fe95234ff10c72cfaa4ad39591f70a531c383 > src/slave/containerizer/mesos/isolators/gpu/volume.cpp > 0d0d778d6a8467c1ac87286e75d47faf8243afa4 > > > Diff: https://reviews.apache.org/r/70016/diff/1/ > > > Testing > --- > > `sudo make check` > > More testing done later in chain. > > > Thanks, > > Chun-Hung Hsiao > >
Re: Review Request 69946: Updated handleWhitelistFds() to avoid closing FDs with FD_CLOEXEC bit.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69946/#review212706 --- Fix it, then Ship it! 3rdparty/libprocess/src/posix/subprocess.hpp Lines 137-138 (patched) <https://reviews.apache.org/r/69946/#comment298595> I'd add a TODO here to cleanup the use of os::strerror here as it's not async signal safe. We have the same issue above. So you can follow up with a patch adding TODO. - Jie Yu On Feb. 11, 2019, 7:21 p.m., Gilbert Song wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69946/ > --- > > (Updated Feb. 11, 2019, 7:21 p.m.) > > > Review request for mesos, Andrei Budnik, Jason Lai, Jie Yu, and James Peach. > > > Repository: mesos > > > Description > --- > > Since this helper is only called right before exec in child process, > and for those open FDs that is set with FD_CLOEXEC flag, they will > be closed during exec, so that we could skip closing these FDs in > the helper. The motivation of this change is to avoid whitelisting > those FDs that have to survive until exec while we do not want to > expose these FDs to user applications. > > > Diffs > - > > 3rdparty/libprocess/src/posix/subprocess.hpp > 7bbf91e857538c2a030199a529287efa8ef2c604 > > > Diff: https://reviews.apache.org/r/69946/diff/1/ > > > Testing > --- > > > Thanks, > > Gilbert Song > >
Re: Review Request 69947: Cloned a sealed file of launcher binary.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69947/#review212707 --- Ship it! Ship It! - Jie Yu On Feb. 11, 2019, 7:21 p.m., Gilbert Song wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69947/ > --- > > (Updated Feb. 11, 2019, 7:21 p.m.) > > > Review request for mesos, Andrei Budnik, Jason Lai, Jie Yu, and James Peach. > > > Repository: mesos > > > Description > --- > > Cloned this binary during mesos containerizer create and command > executor launch. > > This change would copy the mesos-containerizer binary in memory, > which helps avoid the binary being overwritten. > > > Diffs > - > > src/launcher/executor.cpp f962e800f23d5582b1bc04a263253893492a5054 > src/slave/containerizer/mesos/containerizer.hpp > 3102b8755c1fa3b205081d0198c6021c02d15ec6 > src/slave/containerizer/mesos/containerizer.cpp > 35f51ad33da53b3e6a8eec275fbf3e77782b0fba > > > Diff: https://reviews.apache.org/r/69947/diff/1/ > > > Testing > --- > > > Thanks, > > Gilbert Song > >
Re: Review Request 69948: Cleaned up command executor redundant command string.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69948/#review212708 --- Ship it! Ship It! - Jie Yu On Feb. 11, 2019, 7:21 p.m., Gilbert Song wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69948/ > --- > > (Updated Feb. 11, 2019, 7:21 p.m.) > > > Review request for mesos, Andrei Budnik, Jason Lai, Jie Yu, and James Peach. > > > Repository: mesos > > > Description > --- > > Cleaned up command executor redundant command string. > > > Diffs > - > > src/launcher/executor.cpp f962e800f23d5582b1bc04a263253893492a5054 > > > Diff: https://reviews.apache.org/r/69948/diff/1/ > > > Testing > --- > > > Thanks, > > Gilbert Song > >
Review Request 69945: Added a test to test memfd file clone.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69945/ --- Review request for mesos, Andrei Budnik, Gilbert Song, and James Peach. Repository: mesos Description --- This patch added a test to test the memfd::cloneFile. Diffs - src/Makefile.am 3f7daf2cca63c1b9c9e78264f241892327741aa0 src/tests/CMakeLists.txt 42f820715ac43dc70a776f30783d9bc078ef99a5 src/tests/containerizer/linux_memfd_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/69945/diff/1/ Testing --- sudo make check Thanks, Jie Yu
Review Request 69943: Added Linux memfd support.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69943/ --- Review request for mesos, Andrei Budnik, Gilbert Song, and James Peach. Repository: mesos Description --- This patch adds the memfd support to allow us to copy a regular file into a sealed memfd. This can be useful when one wants to protect a file from being overwritten. Diffs - src/CMakeLists.txt 528390a4e36965b195fa5a08225e4ad922ae8938 src/Makefile.am 3f7daf2cca63c1b9c9e78264f241892327741aa0 src/linux/memfd.hpp PRE-CREATION src/linux/memfd.cpp PRE-CREATION Diff: https://reviews.apache.org/r/69943/diff/1/ Testing --- sudo make check Thanks, Jie Yu
Review Request 69944: Made the code more robust related to sendfile.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69944/ --- Review request for mesos, Andrei Budnik, Gilbert Song, and James Peach. Repository: mesos Description --- It's possible that sendfile returns less bytes written than requested. Previously, we simply return an error in that case. This patch makes it more robust by retrying sendfile if less bytes were written than requested. Diffs - src/linux/memfd.cpp PRE-CREATION Diff: https://reviews.apache.org/r/69944/diff/1/ Testing --- sudo make check Thanks, Jie Yu
Re: Review Request 69810: Fixed some typo in agent reboot tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69810/#review212246 --- THis looks fine, but what's the motivation? - Jie Yu On Jan. 23, 2019, 1:15 a.m., Gilbert Song wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69810/ > --- > > (Updated Jan. 23, 2019, 1:15 a.m.) > > > Review request for mesos, Andrei Budnik, Jie Yu, and Qian Zhang. > > > Repository: mesos > > > Description > --- > > Fixed some typo in agent reboot tests. > > > Diffs > - > > src/tests/slave_recovery_tests.cpp 11666f1be3df9ac9df3e1bb9cdef826a9ee4afc8 > > > Diff: https://reviews.apache.org/r/69810/diff/1/ > > > Testing > --- > > > Thanks, > > Gilbert Song > >
Re: Review Request 69809: Fixed a test flakiness in ROOT_CleanupAfterReboot.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69809/#review212211 --- Ship it! Ship It! - Jie Yu On Jan. 23, 2019, 1:15 a.m., Gilbert Song wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69809/ > --- > > (Updated Jan. 23, 2019, 1:15 a.m.) > > > Review request for mesos, Andrei Budnik, Jie Yu, and Qian Zhang. > > > Bugs: MESOS-9532 > https://issues.apache.org/jira/browse/MESOS-9532 > > > Repository: mesos > > > Description > --- > > Fixed a test flakiness in ROOT_CleanupAfterReboot. > > > Diffs > - > > src/tests/containerizer/cni_isolator_tests.cpp > f9fcd99c6176649da13c8e40909266dff5a1c6df > > > Diff: https://reviews.apache.org/r/69809/diff/1/ > > > Testing > --- > > > Thanks, > > Gilbert Song > >
Re: Review Request 69776: Fixed flakiness by adding per agent config dir for mesos test.
> On Jan. 16, 2019, 10:10 p.m., Jie Yu wrote: > > src/tests/mesos.cpp > > Line 173 (original), 173 (patched) > > <https://reviews.apache.org/r/69776/diff/1/?file=2120114#file2120114line173> > > > > s/agentDir/configDir/ > > Gilbert Song wrote: > I thought about `configDir`, sounds not appropriate to me, since store > dir, fetcher dir are not config specific. While we are putting everything > together, agentDir is the only name I can think of. ok, sure! - Jie --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69776/#review212083 --- On Jan. 16, 2019, 10:08 p.m., Gilbert Song wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69776/ > --- > > (Updated Jan. 16, 2019, 10:08 p.m.) > > > Review request for mesos, Andrei Budnik, Greg Mann, Jie Yu, and Qian Zhang. > > > Repository: mesos > > > Description > --- > > In mesos tests, there are some cases that multiple agents are > running simultanuously. From commit 07bccc63, we start to have > agents share the same config dir in the test sandbox. Unit > tests that use the same credicial file dir may fail if there > are agents read and write to the same credential file > simultanuously. We should set unique dir per agent. > > > Diffs > - > > src/tests/mesos.cpp 0d51687f2fc05804d2668837f0cbec4bc2b2e35a > > > Diff: https://reviews.apache.org/r/69776/diff/1/ > > > Testing > --- > > make check > > > Thanks, > > Gilbert Song > >
Re: Review Request 69776: Fixed flakiness by adding per agent config dir for mesos test.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69776/#review212083 --- Fix it, then Ship it! src/tests/mesos.cpp Line 173 (original), 173 (patched) <https://reviews.apache.org/r/69776/#comment297689> s/agentDir/configDir/ - Jie Yu On Jan. 16, 2019, 10:08 p.m., Gilbert Song wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69776/ > --- > > (Updated Jan. 16, 2019, 10:08 p.m.) > > > Review request for mesos, Andrei Budnik, Greg Mann, Jie Yu, and Qian Zhang. > > > Repository: mesos > > > Description > --- > > In mesos tests, there are some cases that multiple agents are > running simultanuously. From commit 07bccc63, we start to have > agents share the same config dir in the test sandbox. Unit > tests that use the same credicial file dir may fail if there > are agents read and write to the same credential file > simultanuously. We should set unique dir per agent. > > > Diffs > - > > src/tests/mesos.cpp 0d51687f2fc05804d2668837f0cbec4bc2b2e35a > > > Diff: https://reviews.apache.org/r/69776/diff/1/ > > > Testing > --- > > make check > > > Thanks, > > Gilbert Song > >
Review Request 69727: Compared the device number of namespace handle instead of /proc.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69727/ --- Review request for mesos, Deepak Goel and Gilbert Song. Bugs: MESOS-9518 https://issues.apache.org/jira/browse/MESOS-9518 Repository: mesos Description --- In recent versions of kernels, the device number of '/proc//ns/net' is different than that of '/proc'. It shows up as "nsfs" instead of "proc" like the old kernels. For instance: Newer kernel: ``` $ uname -nr ubuntu-xenial 4.4.0-83-generic $ stat -L -c %d /proc/self/ns/net 3 $ stat -L -c %d /proc 4 ``` Older kernel: ``` $ uname -nr core-dev 3.10.0-693.5.2.el7.x86_64 $ stat -L -c %d /proc/self/ns/net 3 $ stat -L -c %d /proc 3 ``` As a result, we should compare the device number directly against the namespace handle, instead of `/proc`. Diffs - src/slave/containerizer/mesos/isolators/network/cni/cni.cpp a1130a58553fb67bd8c212498b98978f116d7b0c Diff: https://reviews.apache.org/r/69727/diff/1/ Testing --- sudo make check Thanks, Jie Yu
Re: Review Request 69728: Invoked base test `SetUp` and `TearDown` methods in derived tests.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69728/#review211917
---
Ship it!
Ship It!
- Jie Yu
On Jan. 11, 2019, 11:17 p.m., Benjamin Bannier wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69728/
> ---
>
> (Updated Jan. 11, 2019, 11:17 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> ---
>
> This patch adds required invocations of base class `Setup` and
> `TearDown` in more derived classes.Unfortunately googletest provides no
> indirection(e.g., via applying the template method pattern) and we do
> have to care of this ourself.
>
> These missing cases were identified with the following clang query with
> `METHOD` either `"SetUp"` or `"TearDown"`
>
> match cxxMethodDecl(
> ofClass(isDerivedFrom("::testing::Test")),
> unless(isImplicit()),
> hasName(METHOD),
> isOverride(),
> unless(hasDescendant(cxxMemberCallExpr(
> hasDeclaration(cxxMethodDecl(hasName(METHOD), isVirtual()))
>
> and subsequentially fixing all true positives.
>
>
> Diffs
> -
>
> src/tests/containerizer/docker_tests.cpp
> 0eefbd9fd842632fc183c3d7d2874428d866dfd1
> src/tests/csi_client_tests.cpp 3d4a0626c1d60e723487f99aee26d92064f82298
>
>
> Diff: https://reviews.apache.org/r/69728/diff/1/
>
>
> Testing
> ---
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 69712: Added a CNI reboot test.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69712/ --- (Updated Jan. 11, 2019, 6:49 p.m.) Review request for mesos, Deepak Goel, Gilbert Song, James Peach, and Qian Zhang. Bugs: MESOS-9518 https://issues.apache.org/jira/browse/MESOS-9518 Repository: mesos Description --- This test verifies that CNI DEL is properly called even after the agent host is rebooted, assuming `--network_cni_root_dir_persist` flag is set to true. Diffs (updated) - src/tests/containerizer/cni_isolator_tests.cpp eb20e637ecbe1b39e2dbb274c5198828f2fdf62f Diff: https://reviews.apache.org/r/69712/diff/3/ Changes: https://reviews.apache.org/r/69712/diff/2-3/ Testing --- sudo make check Thanks, Jie Yu
Re: Review Request 69712: Added a CNI reboot test.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69712/ --- (Updated Jan. 11, 2019, 6:09 p.m.) Review request for mesos, Deepak Goel, Gilbert Song, James Peach, and Qian Zhang. Bugs: MESOS-9518 https://issues.apache.org/jira/browse/MESOS-9518 Repository: mesos Description --- This test verifies that CNI DEL is properly called even after the agent host is rebooted, assuming `--network_cni_root_dir_persist` flag is set to true. Diffs (updated) - src/tests/containerizer/cni_isolator_tests.cpp eb20e637ecbe1b39e2dbb274c5198828f2fdf62f Diff: https://reviews.apache.org/r/69712/diff/2/ Changes: https://reviews.apache.org/r/69712/diff/1-2/ Testing --- sudo make check Thanks, Jie Yu
Re: Review Request 69715: Fixed the CNI_NETNS handling in port mapper CNI plugin.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69715/ --- (Updated Jan. 11, 2019, 6:10 p.m.) Review request for mesos, Deepak Goel, Gilbert Song, and Qian Zhang. Bugs: MESOS-9518 https://issues.apache.org/jira/browse/MESOS-9518 Repository: mesos Description --- According CNI spec, it is possible that the container runtime does not set CNI_NETNS environment variable when it is not available. This is possible in scenarios like a host reboot. In that case, the CNI plugin should do best effort cleanup, instead of failing. Diffs (updated) - src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.hpp 25f49f4b90ec6d0d55fc306b6ab324ba5b4e7403 src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp 4e784ffb4ac29861c888fdbed4fcf9902bf4182a Diff: https://reviews.apache.org/r/69715/diff/2/ Changes: https://reviews.apache.org/r/69715/diff/1-2/ Testing --- sudo make check Thanks, Jie Yu
Re: Review Request 69710: Switched to use ContainerizerTest for CNI tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69710/ --- (Updated Jan. 11, 2019, 6:09 p.m.) Review request for mesos, Deepak Goel, Gilbert Song, James Peach, and Qian Zhang. Bugs: MESOS-9518 https://issues.apache.org/jira/browse/MESOS-9518 Repository: mesos Description --- This makes sure that cgroups for each test is independent. Diffs (updated) - src/tests/containerizer/cni_isolator_tests.cpp eb20e637ecbe1b39e2dbb274c5198828f2fdf62f Diff: https://reviews.apache.org/r/69710/diff/2/ Changes: https://reviews.apache.org/r/69710/diff/1-2/ Testing --- sudo make check Thanks, Jie Yu
Re: Review Request 69711: Separated runtime dirs from other dirs in MesosTest.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69711/ --- (Updated Jan. 11, 2019, 6:09 p.m.) Review request for mesos, Deepak Goel, Gilbert Song, James Peach, and Qian Zhang. Bugs: MESOS-9518 https://issues.apache.org/jira/browse/MESOS-9518 Repository: mesos Description --- Previously, lots of other directories are created inside the agent's runtime_dir. This makes it hard to cleanup agent's runtime dir without affecting other files for the test. This patch makes the runtime directory a separate directory. Diffs (updated) - src/tests/mesos.cpp 3a1101cf41995733fe7b6492781def6ac09c6130 Diff: https://reviews.apache.org/r/69711/diff/2/ Changes: https://reviews.apache.org/r/69711/diff/1-2/ Testing --- sudo make check Thanks, Jie Yu
Re: Review Request 69715: Fixed the CNI_NETNS handling in port mapper CNI plugin.
> On Jan. 11, 2019, 9:53 a.m., Qian Zhang wrote: > > src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp > > Lines 72-77 (original), 72 (patched) > > <https://reviews.apache.org/r/69715/diff/1/?file=2119178#file2119178line72> > > > > I think we still need to make sure `cniNetNs` is not `None()` if > > `CNI_COMMAND` is `ADD`. Yeah, good point. Let me add the check - Jie --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69715/#review211875 ----------- On Jan. 11, 2019, 6:14 a.m., Jie Yu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69715/ > --- > > (Updated Jan. 11, 2019, 6:14 a.m.) > > > Review request for mesos, Deepak Goel, Gilbert Song, and Qian Zhang. > > > Bugs: MESOS-9518 > https://issues.apache.org/jira/browse/MESOS-9518 > > > Repository: mesos > > > Description > --- > > According CNI spec, it is possible that the container runtime does not > set CNI_NETNS environment variable when it is not available. This is > possible in scenarios like a host reboot. In that case, the CNI plugin > should do best effort cleanup, instead of failing. > > > Diffs > - > > > src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.hpp > 25f49f4b90ec6d0d55fc306b6ab324ba5b4e7403 > > src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp > 4e784ffb4ac29861c888fdbed4fcf9902bf4182a > > > Diff: https://reviews.apache.org/r/69715/diff/1/ > > > Testing > --- > > sudo make check > > > Thanks, > > Jie Yu > >
Re: Review Request 69712: Added a CNI reboot test.
> On Jan. 11, 2019, 9:38 a.m., Qian Zhang wrote: > > src/tests/containerizer/cni_isolator_tests.cpp > > Lines 2612-2613 (patched) > > <https://reviews.apache.org/r/69712/diff/1/?file=2119174#file2119174line2612> > > > > `%s` is the total size, should we check the device ID instead? err, this is unforntate, this %s is used to print %d. But I figured out a better way to use `%%d` > On Jan. 11, 2019, 9:38 a.m., Qian Zhang wrote: > > src/tests/containerizer/cni_isolator_tests.cpp > > Lines 2763-2764 (patched) > > <https://reviews.apache.org/r/69712/diff/1/?file=2119174#file2119174line2763> > > > > This comment seems not correct. GOod catch! copy paste error :facepalm - Jie --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69712/#review211871 ------- On Jan. 11, 2019, 5:13 a.m., Jie Yu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69712/ > --- > > (Updated Jan. 11, 2019, 5:13 a.m.) > > > Review request for mesos, Deepak Goel, Gilbert Song, James Peach, and Qian > Zhang. > > > Bugs: MESOS-9518 > https://issues.apache.org/jira/browse/MESOS-9518 > > > Repository: mesos > > > Description > --- > > This test verifies that CNI DEL is properly called even after the agent > host is rebooted, assuming `--network_cni_root_dir_persist` flag is set > to true. > > > Diffs > - > > src/tests/containerizer/cni_isolator_tests.cpp > eb20e637ecbe1b39e2dbb274c5198828f2fdf62f > > > Diff: https://reviews.apache.org/r/69712/diff/1/ > > > Testing > --- > > sudo make check > > > Thanks, > > Jie Yu > >
Re: Review Request 69710: Switched to use ContainerizerTest for CNI tests.
> On Jan. 11, 2019, 8:50 a.m., Qian Zhang wrote: > > src/tests/containerizer/cni_isolator_tests.cpp > > Line 138 (original), 138 (patched) > > <https://reviews.apache.org/r/69710/diff/1/?file=2119172#file2119172line138> > > > > Should we call `ContainerizerTest::SetUp()` instead? Good catch! - Jie --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69710/#review211869 ------- On Jan. 11, 2019, 5:13 a.m., Jie Yu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69710/ > --- > > (Updated Jan. 11, 2019, 5:13 a.m.) > > > Review request for mesos, Deepak Goel, Gilbert Song, James Peach, and Qian > Zhang. > > > Bugs: MESOS-9518 > https://issues.apache.org/jira/browse/MESOS-9518 > > > Repository: mesos > > > Description > --- > > This makes sure that cgroups for each test is independent. > > > Diffs > - > > src/tests/containerizer/cni_isolator_tests.cpp > eb20e637ecbe1b39e2dbb274c5198828f2fdf62f > > > Diff: https://reviews.apache.org/r/69710/diff/1/ > > > Testing > --- > > sudo make check > > > Thanks, > > Jie Yu > >
Review Request 69715: Fixed the CNI_NETNS handling in port mapper CNI plugin.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69715/ --- Review request for mesos, Deepak Goel, Gilbert Song, and Qian Zhang. Bugs: MESOS-9518 https://issues.apache.org/jira/browse/MESOS-9518 Repository: mesos Description --- According CNI spec, it is possible that the container runtime does not set CNI_NETNS environment variable when it is not available. This is possible in scenarios like a host reboot. In that case, the CNI plugin should do best effort cleanup, instead of failing. Diffs - src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.hpp 25f49f4b90ec6d0d55fc306b6ab324ba5b4e7403 src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp 4e784ffb4ac29861c888fdbed4fcf9902bf4182a Diff: https://reviews.apache.org/r/69715/diff/1/ Testing --- sudo make check Thanks, Jie Yu
Review Request 69714: Fixed a bug in docker_containerizer_tests.cpp.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69714/ --- Review request for mesos, Deepak Goel, Gilbert Song, and Qian Zhang. Repository: mesos Description --- Forgot to call MesosTest::SetUp() and MesosTest::TearDown() in the override methods. Diffs - src/tests/containerizer/docker_containerizer_tests.cpp 2feead9ace26542821002531a6006fd00f7088b3 Diff: https://reviews.apache.org/r/69714/diff/1/ Testing --- sudo make check Thanks, Jie Yu
Review Request 69713: Fixed a bug in health_check_tests.cpp.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69713/ --- Review request for mesos and Qian Zhang. Repository: mesos Description --- We forgot to call MesosTest::SetUp() and MesosTest::TearDown() in the override methods. Diffs - src/tests/health_check_tests.cpp 3e9b2da5aa1602a5dd24007d9b14cab74e7d02ae Diff: https://reviews.apache.org/r/69713/diff/1/ Testing --- sudo make check Thanks, Jie Yu
Re: Review Request 69710: Switched to use ContainerizerTest for CNI tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69710/ --- (Updated Jan. 11, 2019, 5:13 a.m.) Review request for mesos, Deepak Goel, Gilbert Song, James Peach, and Qian Zhang. Bugs: MESOS-9518 https://issues.apache.org/jira/browse/MESOS-9518 Repository: mesos Description --- This makes sure that cgroups for each test is independent. Diffs - src/tests/containerizer/cni_isolator_tests.cpp eb20e637ecbe1b39e2dbb274c5198828f2fdf62f Diff: https://reviews.apache.org/r/69710/diff/1/ Testing --- sudo make check Thanks, Jie Yu
Re: Review Request 69711: Separated runtime dirs from other dirs in MesosTest.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69711/ --- (Updated Jan. 11, 2019, 5:13 a.m.) Review request for mesos, Deepak Goel, Gilbert Song, James Peach, and Qian Zhang. Bugs: MESOS-9518 https://issues.apache.org/jira/browse/MESOS-9518 Repository: mesos Description --- Previously, lots of other directories are created inside the agent's runtime_dir. This makes it hard to cleanup agent's runtime dir without affecting other files for the test. This patch makes the runtime directory a separate directory. Diffs - src/tests/mesos.cpp 3a1101cf41995733fe7b6492781def6ac09c6130 Diff: https://reviews.apache.org/r/69711/diff/1/ Testing --- sudo make check Thanks, Jie Yu
Review Request 69710: Switched to use ContainerizerTest for CNI tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69710/ --- Review request for mesos, Deepak Goel, Gilbert Song, James Peach, and Qian Zhang. Repository: mesos Description --- This makes sure that cgroups for each test is independent. Diffs - src/tests/containerizer/cni_isolator_tests.cpp eb20e637ecbe1b39e2dbb274c5198828f2fdf62f Diff: https://reviews.apache.org/r/69710/diff/1/ Testing --- sudo make check Thanks, Jie Yu
Review Request 69711: Separated runtime dirs from other dirs in MesosTest.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69711/ --- Review request for mesos, Deepak Goel, Gilbert Song, James Peach, and Qian Zhang. Repository: mesos Description --- Previously, lots of other directories are created inside the agent's runtime_dir. This makes it hard to cleanup agent's runtime dir without affecting other files for the test. This patch makes the runtime directory a separate directory. Diffs - src/tests/mesos.cpp 3a1101cf41995733fe7b6492781def6ac09c6130 Diff: https://reviews.apache.org/r/69711/diff/1/ Testing --- sudo make check Thanks, Jie Yu
Review Request 69712: Added a CNI reboot test.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69712/ --- Review request for mesos, Deepak Goel, Gilbert Song, James Peach, and Qian Zhang. Bugs: MESOS-9518 https://issues.apache.org/jira/browse/MESOS-9518 Repository: mesos Description --- This test verifies that CNI DEL is properly called even after the agent host is rebooted, assuming `--network_cni_root_dir_persist` flag is set to true. Diffs - src/tests/containerizer/cni_isolator_tests.cpp eb20e637ecbe1b39e2dbb274c5198828f2fdf62f Diff: https://reviews.apache.org/r/69712/diff/1/ Testing --- sudo make check Thanks, Jie Yu
Re: Review Request 69706: Kept `CNI_NETNS` unset in detach if network namespace is gone.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69706/ --- (Updated Jan. 11, 2019, 5:08 a.m.) Review request for mesos, Deepak Goel, Gilbert Song, James Peach, and Qian Zhang. Changes --- Need to reorder the code a bit so that `stat` is not called if detach is to be skipped. Also, factored out a helper, which is used in cleanup to skip unmount if needed. Bugs: MESOS-9518 https://issues.apache.org/jira/browse/MESOS-9518 Repository: mesos Description --- We introduced a new agent flag in MESOS-9492 so that CNI configs can be persisted across reboot. This is for some CNI plugins to be able to cleanup IP allocated to the containers after a sudden reboot of the host (not all CNI plugins need this). It's important to unset `CNI_NETNS` environment variable after reboot when invoking CNI plugin "DEL" command so that it conforms to the spec. Diffs (updated) - src/slave/containerizer/mesos/isolators/network/cni/cni.cpp cc23428d27d40be8c4ff1476e6e984c7d12760c4 Diff: https://reviews.apache.org/r/69706/diff/2/ Changes: https://reviews.apache.org/r/69706/diff/1-2/ Testing --- sudo make check Thanks, Jie Yu
Review Request 69706: Kept `CNI_NETNS` unset in detach if network namespace is gone.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69706/ --- Review request for mesos, Deepak Goel, James Peach, and Qian Zhang. Bugs: MESOS-9518 https://issues.apache.org/jira/browse/MESOS-9518 Repository: mesos Description --- We introduced a new agent flag in MESOS-9492 so that CNI configs can be persisted across reboot. This is for some CNI plugins to be able to cleanup IP allocated to the containers after a sudden reboot of the host (not all CNI plugins need this). It's important to unset `CNI_NETNS` environment variable after reboot when invoking CNI plugin "DEL" command so that it conforms to the spec. Diffs - src/slave/containerizer/mesos/isolators/network/cni/cni.cpp cc23428d27d40be8c4ff1476e6e984c7d12760c4 Diff: https://reviews.apache.org/r/69706/diff/1/ Testing --- sudo make check Thanks, Jie Yu
Re: Review Request 69690: Changed the gRPC make variables for protoc detection.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69690/#review211738 --- Ship it! Ship It! - Jie Yu On Jan. 7, 2019, 8:19 p.m., Chun-Hung Hsiao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69690/ > --- > > (Updated Jan. 7, 2019, 8:19 p.m.) > > > Review request for mesos and Jie Yu. > > > Repository: mesos > > > Description > --- > > Since gRPC's Makefile use `HAS_XXX` as public-facing third-party > library control flags, we should use them to detect protoc instead of > setting the `NO_PROTOC` internal variable. > > > Diffs > - > > 3rdparty/Makefile.am a14216cf98f6638da06aa3dfc49e6b319fea7f87 > > > Diff: https://reviews.apache.org/r/69690/diff/1/ > > > Testing > --- > > make check > > > Thanks, > > Chun-Hung Hsiao > >
Re: Review Request 69671: Always build gRPC with its embedded c-ares.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69671/#review211701 --- Fix it, then Ship it! 3rdparty/Makefile.am Lines 500 (patched) <https://reviews.apache.org/r/69671/#comment297209> Can you add some comments about this one? Or just separate this change in a different patch? - Jie Yu On Jan. 5, 2019, 1:24 a.m., Chun-Hung Hsiao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69671/ > --- > > (Updated Jan. 5, 2019, 1:24 a.m.) > > > Review request for mesos and Jie Yu. > > > Bugs: MESOS-9505 > https://issues.apache.org/jira/browse/MESOS-9505 > > > Repository: mesos > > > Description > --- > > We compile gRPC with its embedded c-ares library to avoid a link error > because Mesos is not aware of the library. > > > Diffs > - > > 3rdparty/Makefile.am a14216cf98f6638da06aa3dfc49e6b319fea7f87 > > > Diff: https://reviews.apache.org/r/69671/diff/1/ > > > Testing > --- > > make check with c-ares pre-installed on mac and ubuntu. > > > Thanks, > > Chun-Hung Hsiao > >
Re: Review Request 69665: Added missing 3rdparty patches to the distribution.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69665/#review211663 --- Ship it! Ship It! - Jie Yu On Jan. 4, 2019, 12:57 a.m., Chun-Hung Hsiao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69665/ > --- > > (Updated Jan. 4, 2019, 12:57 a.m.) > > > Review request for mesos, Benjamin Bannier and Jie Yu. > > > Bugs: MESOS-9508 > https://issues.apache.org/jira/browse/MESOS-9508 > > > Repository: mesos > > > Description > --- > > Added missing 3rdparty patches to the distribution. > > > Diffs > - > > 3rdparty/Makefile.am a14216cf98f6638da06aa3dfc49e6b319fea7f87 > > > Diff: https://reviews.apache.org/r/69665/diff/1/ > > > Testing > --- > > `make dist`, then build from the created tarball. > > > Thanks, > > Chun-Hung Hsiao > >
Re: Review Request 69590: Moves CNI root directory to a persistent location.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69590/#review211423 --- Fix it, then Ship it! src/slave/containerizer/mesos/isolators/network/cni/cni.cpp Lines 1312-1313 (patched) <https://reviews.apache.org/r/69590/#comment296517> This log line along is not enough information. You need to at containerId and network name so that folks can correlate. src/slave/containerizer/mesos/isolators/network/cni/cni.cpp Lines 1785 (patched) <https://reviews.apache.org/r/69590/#comment296518> Ditto src/slave/containerizer/mesos/isolators/network/cni/paths.hpp Line 35 (original), 37 (patched) <https://reviews.apache.org/r/69590/#comment296516> Please update the comments here. src/slave/containerizer/mesos/isolators/network/cni/paths.hpp Line 48 (original), 50 (patched) <https://reviews.apache.org/r/69590/#comment296515> No need for this variable anymore. See my comments below. src/slave/containerizer/mesos/isolators/network/cni/paths.cpp Lines 37 (patched) <https://reviews.apache.org/r/69590/#comment296514> instead of use `paths::ROOT_DIR`, you should use `flags.runtime_dir` - Jie Yu On Dec. 19, 2018, 4:52 a.m., Deepak Goel wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69590/ > --- > > (Updated Dec. 19, 2018, 4:52 a.m.) > > > Review request for mesos, Gilbert Song, Jie Yu, Qian Zhang, and Sergey > Urbanovich. > > > Bugs: MESOS-9492 > https://issues.apache.org/jira/browse/MESOS-9492 > > > Repository: mesos > > > Description > --- > > This patch moves the CNI root directory to mesos working directory > so that the network related information persist across reboot which > will allow cni isolator to do proper cleanup post reboot > > > Diffs > - > > docs/cni.md 4a0e0bf57c6ff8d2bcc9fdf30d20672324da224c > docs/configuration/agent.md 7a8df6852dc2af174a6c5a552dca88fa1b1c29f3 > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp > f19edce0671686bc2dccf5dc6d3abc40b925852e > src/slave/containerizer/mesos/isolators/network/cni/paths.hpp > 3b06fb101d03b32a3a148e2b79502413a7268d1e > src/slave/containerizer/mesos/isolators/network/cni/paths.cpp > c1cb4f7d667159abbf87219cde30528a4a7b5283 > src/slave/flags.hpp 29d8b7985ffde57da02b5fe0d3a524e98acc27c8 > src/slave/flags.cpp ccaf65029ec2d0e78041fc3992a0bf5ca0798686 > src/tests/containerizer/cni_isolator_tests.cpp > 5473737cdc71a3179c22be0d4db2060a6ef53d24 > > > Diff: https://reviews.apache.org/r/69590/diff/4/ > > > Testing > --- > > > Thanks, > > Deepak Goel > >
Re: Review Request 69540: Added `ROOT_PseudoDevicesWithRootFilesystem` test.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69540/#review211151 --- Ship it! Ship It! - Jie Yu On Dec. 10, 2018, 4:41 p.m., Andrei Budnik wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69540/ > --- > > (Updated Dec. 10, 2018, 4:41 p.m.) > > > Review request for mesos, Jie Yu and James Peach. > > > Bugs: MESOS-9462 > https://issues.apache.org/jira/browse/MESOS-9462 > > > Repository: mesos > > > Description > --- > > This test verifies that pseudo devices like /dev/random are properly > mounted in the container's root filesystem. > > > Diffs > - > > src/tests/containerizer/linux_filesystem_isolator_tests.cpp > 84b342cdd4b8ef4803725ecfa9f922687ccdadd8 > src/tests/containerizer/rootfs.cpp 83662fc2c2b9ea902b444bab9c2911df01ee11d5 > > > Diff: https://reviews.apache.org/r/69540/diff/1/ > > > Testing > --- > > internal CI > > > Thanks, > > Andrei Budnik > >
Review Request 69538: Added UCR bridge network for Mesos Mini.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69538/
---
Review request for mesos, Deepak Goel and James Peach.
Repository: mesos
Description
---
This patch adds the UCR bridge network support for Mesos Mini using CNI
bridge plugin and Mesos port mapper CNI plugin.
Diffs
-
support/mesos-mini/Dockerfile f8af422d8b8b0be4c462d40bce2d1337ab8b6e95
support/mesos-mini/mesos/agent_environment
48767281987c2e70b3a64f28c5eac139435c17f6
support/mesos-mini/mesos/master_environment
0382558744cb693c862246856b8c16ff42a95d19
support/mesos-mini/mesos/ucr-default-bridge.json PRE-CREATION
Diff: https://reviews.apache.org/r/69538/diff/1/
Testing
---
Tested using this Marathon app
```json
{
"id": "/test",
"instances": 1,
"container": {
"type": "MESOS",
"volumes": [],
"portMappings": [],
"docker": {
"image": "alpine"
}
},
"cpus": 0.1,
"mem": 128,
"requirePorts": false,
"networks": [
{
"mode": "container/bridge"
}
],
"healthChecks": [],
"fetch": [],
"constraints": [],
"cmd": "sleep 10"
}
```
Thanks,
Jie Yu
Re: Review Request 69532: Made sure containers runtime dir has device file access.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69532/
---
(Updated Dec. 10, 2018, 3:16 a.m.)
Review request for mesos, Andrei Budnik and James Peach.
Changes
---
Addressed comments.
Bugs: MESOS-9462
https://issues.apache.org/jira/browse/MESOS-9462
Repository: mesos
Description
---
Make sure that container's runtime dir has device file access. Some
Linux distributions will mount `/run` with `nodev`, restricting
accessing to device files under `/run`. However, Mesos prepares device
files for containers under container's runtime dir (which is typically
under `/run`) and bind mount into container root filesystems. Therefore,
we need to make sure those device files can be accessed by the
container. We need to do a self bind mount and remount with proper
options if necessary. See MESOS-9462 for more details.
Diffs (updated)
-
src/linux/fs.hpp 04bd706447ec7edf6b1f05fb416c834d189b2218
src/linux/fs.cpp 6ff8b4dd8ac69f4afd5fa24bb38eb26753e6bcdd
src/slave/containerizer/mesos/isolators/filesystem/linux.cpp
c318fb956e22f07f32fd18b238b86f6a1ee0b2bf
src/slave/containerizer/mesos/isolators/volume/host_path.cpp
2e4cd2b9b852b0c95a2957f4bed33fe54f093e3d
src/slave/containerizer/mesos/launch.cpp
6aa4397aa0fc9e1c93a0f3a13fbe0a55c5a02f0c
Diff: https://reviews.apache.org/r/69532/diff/3/
Changes: https://reviews.apache.org/r/69532/diff/2-3/
Testing
---
sudo make check
Built a new Mesos Mini image and tested. The workload runs fine. The same
workload fails on `mesos/mesos-mini-2018-12-06`
```
docker run --rm --privileged -p 5050:5050 -p 5051:5051 -p 8080:8080
jieyu/mesos-mini
```
```json
{
"id": "/test",
"cmd": "dd if=/dev/zero of=file bs=1024 count=1 oflag=dsync",
"cpus": 1,
"mem": 128,
"disk": 128,
"instances": 1,
"container": {
"type": "MESOS",
"docker": {
"image": "ubuntu:18.04"
}
}
}
```
Thanks,
Jie Yu
Review Request 69537: Used strings::format in os::shell.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69537/ --- Review request for mesos and James Peach. Repository: mesos Description --- Previously, `strings::internal::format` was used. It causes issues when std::string is passed in as parameters. Switched to use `strings::format` instead in `os::shell` implementation. Diffs - 3rdparty/stout/include/stout/os/posix/shell.hpp de6ef23b9925c240d7621b3acd8c6b195bb1b662 3rdparty/stout/include/stout/os/windows/shell.hpp b9e06d667159d2fb5e266ce7f7e633deb1237a78 Diff: https://reviews.apache.org/r/69537/diff/1/ Testing --- make check Thanks, Jie Yu
Re: Review Request 69532: Made sure containers runtime dir has device file access.
> On Dec. 8, 2018, 11:14 p.m., James Peach wrote:
> > src/slave/containerizer/mesos/isolators/filesystem/linux.cpp
> > Lines 404 (patched)
> > <https://reviews.apache.org/r/69532/diff/2/?file=2112671#file2112671line427>
> >
> > I think the control flow here would be simpler if you unconditionally
> > do the nodev remount. Something like this:
> >
> > ```
> > if (targetDirMount->target != targetDir.get()) {
> > LOG(INFO) << "Self bind mounting";
> > ...
> > mount --bind
> > }
> >
> > LOG(INFO) << "remounting nodev";
> > mount -o remount,dev
> >
> > ```
I'd prefer to be consistent with shared mount case, which is based on
necessarity.
- Jie
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69532/#review211130
---
On Dec. 8, 2018, 4:45 a.m., Jie Yu wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69532/
> ---
>
> (Updated Dec. 8, 2018, 4:45 a.m.)
>
>
> Review request for mesos, Andrei Budnik and James Peach.
>
>
> Bugs: MESOS-9462
> https://issues.apache.org/jira/browse/MESOS-9462
>
>
> Repository: mesos
>
>
> Description
> ---
>
> Make sure that container's runtime dir has device file access. Some
> Linux distributions will mount `/run` with `nodev`, restricting
> accessing to device files under `/run`. However, Mesos prepares device
> files for containers under container's runtime dir (which is typically
> under `/run`) and bind mount into container root filesystems. Therefore,
> we need to make sure those device files can be accessed by the
> container. We need to do a self bind mount and remount with proper
> options if necessary. See MESOS-9462 for more details.
>
>
> Diffs
> -
>
> src/linux/fs.hpp 04bd706447ec7edf6b1f05fb416c834d189b2218
> src/linux/fs.cpp 6ff8b4dd8ac69f4afd5fa24bb38eb26753e6bcdd
> src/slave/containerizer/mesos/isolators/filesystem/linux.cpp
> c318fb956e22f07f32fd18b238b86f6a1ee0b2bf
> src/slave/containerizer/mesos/isolators/volume/host_path.cpp
> 2e4cd2b9b852b0c95a2957f4bed33fe54f093e3d
> src/slave/containerizer/mesos/launch.cpp
> 6aa4397aa0fc9e1c93a0f3a13fbe0a55c5a02f0c
>
>
> Diff: https://reviews.apache.org/r/69532/diff/2/
>
>
> Testing
> ---
>
> sudo make check
>
> Built a new Mesos Mini image and tested. The workload runs fine. The same
> workload fails on `mesos/mesos-mini-2018-12-06`
> ```
> docker run --rm --privileged -p 5050:5050 -p 5051:5051 -p 8080:8080
> jieyu/mesos-mini
> ```
>
> ```json
> {
> "id": "/test",
> "cmd": "dd if=/dev/zero of=file bs=1024 count=1 oflag=dsync",
> "cpus": 1,
> "mem": 128,
> "disk": 128,
> "instances": 1,
> "container": {
> "type": "MESOS",
> "docker": {
> "image": "ubuntu:18.04"
> }
> }
> }
> ```
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 69532: Made sure containers runtime dir has device file access.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69532/
---
(Updated Dec. 8, 2018, 4:45 a.m.)
Review request for mesos, Andrei Budnik and James Peach.
Bugs: MESOS-9462
https://issues.apache.org/jira/browse/MESOS-9462
Repository: mesos
Description
---
Make sure that container's runtime dir has device file access. Some
Linux distributions will mount `/run` with `nodev`, restricting
accessing to device files under `/run`. However, Mesos prepares device
files for containers under container's runtime dir (which is typically
under `/run`) and bind mount into container root filesystems. Therefore,
we need to make sure those device files can be accessed by the
container. We need to do a self bind mount and remount with proper
options if necessary. See MESOS-9462 for more details.
Diffs
-
src/linux/fs.hpp 04bd706447ec7edf6b1f05fb416c834d189b2218
src/linux/fs.cpp 6ff8b4dd8ac69f4afd5fa24bb38eb26753e6bcdd
src/slave/containerizer/mesos/isolators/filesystem/linux.cpp
c318fb956e22f07f32fd18b238b86f6a1ee0b2bf
src/slave/containerizer/mesos/isolators/volume/host_path.cpp
2e4cd2b9b852b0c95a2957f4bed33fe54f093e3d
src/slave/containerizer/mesos/launch.cpp
6aa4397aa0fc9e1c93a0f3a13fbe0a55c5a02f0c
Diff: https://reviews.apache.org/r/69532/diff/2/
Testing (updated)
---
sudo make check
Built a new Mesos Mini image and tested. The workload runs fine. The same
workload fails on `mesos/mesos-mini-2018-12-06`
```
docker run --rm --privileged -p 5050:5050 -p 5051:5051 -p 8080:8080
jieyu/mesos-mini
```
```json
{
"id": "/test",
"cmd": "dd if=/dev/zero of=file bs=1024 count=1 oflag=dsync",
"cpus": 1,
"mem": 128,
"disk": 128,
"instances": 1,
"container": {
"type": "MESOS",
"docker": {
"image": "ubuntu:18.04"
}
}
}
```
Thanks,
Jie Yu
Re: Review Request 69532: Made sure containers runtime dir has device file access.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69532/ --- (Updated Dec. 8, 2018, 1:55 a.m.) Review request for mesos, Andrei Budnik and James Peach. Bugs: MESOS-9462 https://issues.apache.org/jira/browse/MESOS-9462 Repository: mesos Description --- Make sure that container's runtime dir has device file access. Some Linux distributions will mount `/run` with `nodev`, restricting accessing to device files under `/run`. However, Mesos prepares device files for containers under container's runtime dir (which is typically under `/run`) and bind mount into container root filesystems. Therefore, we need to make sure those device files can be accessed by the container. We need to do a self bind mount and remount with proper options if necessary. See MESOS-9462 for more details. Diffs (updated) - src/linux/fs.hpp 04bd706447ec7edf6b1f05fb416c834d189b2218 src/linux/fs.cpp 6ff8b4dd8ac69f4afd5fa24bb38eb26753e6bcdd src/slave/containerizer/mesos/isolators/filesystem/linux.cpp c318fb956e22f07f32fd18b238b86f6a1ee0b2bf src/slave/containerizer/mesos/isolators/volume/host_path.cpp 2e4cd2b9b852b0c95a2957f4bed33fe54f093e3d src/slave/containerizer/mesos/launch.cpp 6aa4397aa0fc9e1c93a0f3a13fbe0a55c5a02f0c Diff: https://reviews.apache.org/r/69532/diff/2/ Changes: https://reviews.apache.org/r/69532/diff/1-2/ Testing --- sudo make check Thanks, Jie Yu
Review Request 69532: Made sure containers runtime dir has device file access.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69532/ --- Review request for mesos, Andrei Budnik and James Peach. Bugs: MESOS-9462 https://issues.apache.org/jira/browse/MESOS-9462 Repository: mesos Description --- Make sure that container's runtime dir has device file access. Some Linux distributions will mount `/run` with `nodev`, restricting accessing to device files under `/run`. However, Mesos prepares device files for containers under container's runtime dir (which is typically under `/run`) and bind mount into container root filesystems. Therefore, we need to make sure those device files can be accessed by the container. We need to do a self bind mount and remount with proper options if necessary. See MESOS-9462 for more details. Diffs - src/linux/fs.hpp 04bd706447ec7edf6b1f05fb416c834d189b2218 src/linux/fs.cpp 6ff8b4dd8ac69f4afd5fa24bb38eb26753e6bcdd src/slave/containerizer/mesos/isolators/filesystem/linux.cpp c318fb956e22f07f32fd18b238b86f6a1ee0b2bf src/slave/containerizer/mesos/isolators/volume/host_path.cpp 2e4cd2b9b852b0c95a2957f4bed33fe54f093e3d src/slave/containerizer/mesos/launch.cpp 6aa4397aa0fc9e1c93a0f3a13fbe0a55c5a02f0c Diff: https://reviews.apache.org/r/69532/diff/1/ Testing --- sudo make check Thanks, Jie Yu
Re: Review Request 69528: Fixed a regression in binding GPU container devices.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69528/#review211120 --- Ship it! Ship It! - Jie Yu On Dec. 7, 2018, 7:41 p.m., James Peach wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69528/ > --- > > (Updated Dec. 7, 2018, 7:41 p.m.) > > > Review request for mesos, Jie Yu and Jiang Yan Xu. > > > Bugs: MESOS-9319 > https://issues.apache.org/jira/browse/MESOS-9319 > > > Repository: mesos > > > Description > --- > > When we changed container devices to be bind mounts, we added an extra > `/dev` path component to the container moount point. This resulted in > devices being mounted as `/dev/dev/nvidia0`. > > > Diffs > - > > src/slave/containerizer/mesos/isolators/gpu/isolator.cpp > 14871287db8be96b3bb4d655caf43f92f0dbce13 > > > Diff: https://reviews.apache.org/r/69528/diff/1/ > > > Testing > --- > > make check (Fedora 29) > > Tested manually on a GPU host. > > Before: > ``` > |-/dev tmpfs > | |-/dev/pts devpts > | |-/dev/shm tmpfs > | |-/dev/full > /dev/mapper/rootvg-rootlv[/var/run/mesos/containers/28f9b34e-9cd4-4b1a-8bb1-237743e4a483/devices/full] > | |-/dev/null > /dev/mapper/rootvg-rootlv[/var/run/mesos/containers/28f9b34e-9cd4-4b1a-8bb1-237743e4a483/devices/null] > | |-/dev/random > /dev/mapper/rootvg-rootlv[/var/run/mesos/containers/28f9b34e-9cd4-4b1a-8bb1-237743e4a483/devices/random] > | |-/dev/tty > /dev/mapper/rootvg-rootlv[/var/run/mesos/containers/28f9b34e-9cd4-4b1a-8bb1-237743e4a483/devices/tty] > | |-/dev/urandom > /dev/mapper/rootvg-rootlv[/var/run/mesos/containers/28f9b34e-9cd4-4b1a-8bb1-237743e4a483/devices/urandom] > | |-/dev/zero > /dev/mapper/rootvg-rootlv[/var/run/mesos/containers/28f9b34e-9cd4-4b1a-8bb1-237743e4a483/devices/zero] > | |-/dev/fuse devtmpfs[/fuse] > | |-/dev/dev/nvidia-uvm > /dev/mapper/rootvg-rootlv[/var/run/mesos/containers/28f9b34e-9cd4-4b1a-8bb1-237743e4a483/devices/nvidia-uvm/dev/nvidia-uvm] > | |-/dev/dev/nvidiactl > /dev/mapper/rootvg-rootlv[/var/run/mesos/containers/28f9b34e-9cd4-4b1a-8bb1-237743e4a483/devices/nvidiactl/dev/nvidiactl] > | |-/dev/dev/nvidia1 > /dev/mapper/rootvg-rootlv[/var/run/mesos/containers/28f9b34e-9cd4-4b1a-8bb1-237743e4a483/devices/nvidia1/dev/nvidia1] > | `-/dev/dev/nvidia0 > /dev/mapper/rootvg-rootlv[/var/run/mesos/containers/28f9b34e-9cd4-4b1a-8bb1-237743e4a483/devices/nvidia0/dev/nvidia0] > ``` > > After: > ``` > |-/dev tmpfs > | |-/dev/pts devpts > | |-/dev/shm tmpfs > | |-/dev/full > /dev/mapper/rootvg-rootlv[/var/run/mesos/containers/450a0823-78bf-4556-9221-eb1d569ee097/devices/full] > | |-/dev/null > /dev/mapper/rootvg-rootlv[/var/run/mesos/containers/450a0823-78bf-4556-9221-eb1d569ee097/devices/null] > | |-/dev/random > /dev/mapper/rootvg-rootlv[/var/run/mesos/containers/450a0823-78bf-4556-9221-eb1d569ee097/devices/random] > | |-/dev/tty > /dev/mapper/rootvg-rootlv[/var/run/mesos/containers/450a0823-78bf-4556-9221-eb1d569ee097/devices/tty] > | |-/dev/urandom > /dev/mapper/rootvg-rootlv[/var/run/mesos/containers/450a0823-78bf-4556-9221-eb1d569ee097/devices/urandom] > | |-/dev/zero > /dev/mapper/rootvg-rootlv[/var/run/mesos/containers/450a0823-78bf-4556-9221-eb1d569ee097/devices/zero] > | |-/dev/fuse devtmpfs[/fuse] > | |-/dev/nvidia-uvm > /dev/mapper/rootvg-rootlv[/var/run/mesos/containers/450a0823-78bf-4556-9221-eb1d569ee097/devices/nvidia-uvm/dev/nvidia-uvm] > | |-/dev/nvidiactl > /dev/mapper/rootvg-rootlv[/var/run/mesos/containers/450a0823-78bf-4556-9221-eb1d569ee097/devices/nvidiactl/dev/nvidiactl] > | |-/dev/nvidia1 > /dev/mapper/rootvg-rootlv[/var/run/mesos/containers/450a0823-78bf-4556-9221-eb1d569ee097/devices/nvidia1/dev/nvidia1] > | `-/dev/nvidia0 > /dev/mapper/rootvg-rootlv[/var/run/mesos/containers/450a0823-78bf-4556-9221-eb1d569ee097/devices/nvidia0/dev/nvidia0] > ``` > > > Thanks, > > James Peach > >
Re: Review Request 69210: Removed unnecesssarily verbose container mount logging.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69210/#review211022 --- Ship it! Ship It! - Jie Yu On Nov. 17, 2018, 12:49 a.m., James Peach wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69210/ > --- > > (Updated Nov. 17, 2018, 12:49 a.m.) > > > Review request for mesos, Gilbert Song, Jason Lai, Jie Yu, and Jiang Yan Xu. > > > Bugs: MESOS-9319 > https://issues.apache.org/jira/browse/MESOS-9319 > > > Repository: mesos > > > Description > --- > > The logs from making container mounts can be fairly verbose, and > we are primarily interested in failures. This change removes the > default logging, and only logs container mount errors. > > > Diffs > - > > src/slave/containerizer/mesos/launch.cpp > 882bcdf89e2b0cca3d3f62e6d017849a51ceaead > > > Diff: https://reviews.apache.org/r/69210/diff/10/ > > > Testing > --- > > make check (Fedora 28) > > > Thanks, > > James Peach > >
Re: Review Request 69211: Improved the code comments for `getContainerDevicesPath`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69211/#review211015 --- Ship it! Ship It! - Jie Yu On Dec. 4, 2018, 1:19 a.m., James Peach wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69211/ > --- > > (Updated Dec. 4, 2018, 1:19 a.m.) > > > Review request for mesos, Gilbert Song, Jason Lai, Jie Yu, and Jiang Yan Xu. > > > Bugs: MESOS-9319 > https://issues.apache.org/jira/browse/MESOS-9319 > > > Repository: mesos > > > Description > --- > > Improved the code comments for `getContainerDevicesPath`. > > > Diffs > - > > src/slave/containerizer/mesos/paths.hpp > de3981db7eb08e53901547037c947f594c8d46ab > > > Diff: https://reviews.apache.org/r/69211/diff/10/ > > > Testing > --- > > make check (Fedora 28) > > > Thanks, > > James Peach > >
Re: Review Request 69450: Applied the `ContainerMountInfo` protobuf helper.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69450/#review211014 --- Ship it! Ship It! - Jie Yu On Nov. 27, 2018, 12:49 a.m., James Peach wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69450/ > --- > > (Updated Nov. 27, 2018, 12:49 a.m.) > > > Review request for mesos, Gilbert Song, Jason Lai, Jie Yu, and Jiang Yan Xu. > > > Bugs: MESOS-9319 > https://issues.apache.org/jira/browse/MESOS-9319 > > > Repository: mesos > > > Description > --- > > Now that there is a protobuf helper to manufacture `ContainerMountInfo` > messages, apply it where appropriate. > > > Diffs > - > > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp > 507665cdd3c08f6716840f2b10f2b9659e7fcf1a > src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp > d1938836997b177ec89c1e8d671dc6e0379aa061 > src/slave/containerizer/mesos/isolators/filesystem/shared.cpp > 5b481b5e8e66104b8b7af6fa97dcb411fb0a1f5e > src/slave/containerizer/mesos/isolators/namespaces/pid.cpp > 73e09636808081ce411c022bf2bcb2a157a3ad25 > src/slave/containerizer/mesos/isolators/volume/host_path.cpp > 88ecf91d91e2bebd484a4ac94510a14b3500dbfb > src/slave/containerizer/mesos/isolators/volume/image.cpp > bb3fc659f1322d26fe3d035c961d3942c73353f0 > src/slave/containerizer/mesos/isolators/volume/sandbox_path.cpp > 300b3d95d74b73fbe0221096f3f3f172be745081 > > > Diff: https://reviews.apache.org/r/69450/diff/2/ > > > Testing > --- > > sudo make check (Fedora 29) > > > Thanks, > > James Peach > >
Re: Review Request 69210: Used the MS_SILENT mount flag to elide unwanted logging.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69210/#review211013 --- src/slave/containerizer/mesos/isolators/filesystem/linux.cpp Line 189 (original), 189 (patched) <https://reviews.apache.org/r/69210/#comment295866> This is a bit weird to me. I'd prefer just add a new optional field in ContainerMountInfo - Jie Yu On Nov. 17, 2018, 12:49 a.m., James Peach wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69210/ > --- > > (Updated Nov. 17, 2018, 12:49 a.m.) > > > Review request for mesos, Gilbert Song, Jason Lai, Jie Yu, and Jiang Yan Xu. > > > Bugs: MESOS-9319 > https://issues.apache.org/jira/browse/MESOS-9319 > > > Repository: mesos > > > Description > --- > > After moving the container root filesystem mounts to the > `filesystem/linux` isolator, these mounts are not logged into the task > sandbox by the container launcher. Since these logs are not generally > useful to tasks, and we did't previously emit them, use the `MS_SILENT` > flag to indicate that they don't need to be logged. Since the kernel > doesn't use `MS_SILENT` for anything, we can safely use it internally. > > > Diffs > - > > src/slave/containerizer/mesos/isolators/filesystem/linux.cpp > c7d753ac2e5575a8d687600bfb9e0617fa72c990 > src/slave/containerizer/mesos/launch.cpp > 882bcdf89e2b0cca3d3f62e6d017849a51ceaead > > > Diff: https://reviews.apache.org/r/69210/diff/9/ > > > Testing > --- > > make check (Fedora 28) > > > Thanks, > > James Peach > >
Re: Review Request 69086: Moved the container root construction to the isolators.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69086/#review211012 --- Ship it! Ship It! - Jie Yu On Nov. 27, 2018, 12:49 a.m., James Peach wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69086/ > --- > > (Updated Nov. 27, 2018, 12:49 a.m.) > > > Review request for mesos, Gilbert Song, Jason Lai, Jie Yu, and Jiang Yan Xu. > > > Bugs: MESOS-9319 > https://issues.apache.org/jira/browse/MESOS-9319 > > > Repository: mesos > > > Description > --- > > Previously, if the container was configured with a root filesytem, > the root was populated by a combination of the `fs::chroot:prepare` > API and the various isolators. The implementation details of some > isolators had leaked into the chroot code, which had a special case > for adding GPU devices. > > This change moves all the responsibility for defining the > root filesystem from the `fs::chroot::prepare()` API to the > `filesystem/linux` isolator. The `filesystem/linux` isolator is > now the single place that captures how to mount the container > pseudo-filesystems as well as how to construct a proper `/dev` > directory. > > Since the `linux/filesystem` isolator is now entirely responsible > for creating and mounting the container `/dev`, any other isolators > that enable access to devices should populate device nodes in the > container devices directory and add a corresponding bind mount. > > > Diffs > - > > src/common/protobuf_utils.hpp 1662125ed3e47b179ee32d08c1d3af75553066ba > src/common/protobuf_utils.cpp a45607eed4c4bae5010bcc3f3ffeabd6d911062a > src/linux/fs.hpp 31969f6ba82bf5ee549bfdf9698a21adaa486a90 > src/linux/fs.cpp 5cdffe1f4c7f00aee5b8f640e7cfa4a0018cfa0a > src/slave/containerizer/mesos/isolators/filesystem/linux.cpp > c7d753ac2e5575a8d687600bfb9e0617fa72c990 > src/slave/containerizer/mesos/isolators/gpu/isolator.hpp > 4645c625877d9451516133b24bd3959e0f49c0a9 > src/slave/containerizer/mesos/isolators/gpu/isolator.cpp > 56d835779618fd965d928c6926664583e9141f79 > src/slave/containerizer/mesos/isolators/linux/devices.cpp > 8f8ff95ec3856ba06647637a80315365d0e66e23 > src/slave/containerizer/mesos/launch.cpp > 882bcdf89e2b0cca3d3f62e6d017849a51ceaead > > > Diff: https://reviews.apache.org/r/69086/diff/13/ > > > Testing > --- > > sudo make check (Fedora 28) > > > Thanks, > > James Peach > >
Re: Review Request 69377: Added blog post for Mesos Mini.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69377/ --- (Updated Nov. 19, 2018, 8:28 p.m.) Review request for mesos, Benjamin Mahler, Chun-Hung Hsiao, Greg Mann, Jason Lai, James DeFelice, James Peach, and Vinod Kone. Changes --- Addressed comments. Repository: mesos Description --- Added blog post for Mesos Mini. Diffs (updated) - site/source/blog/2018-11-19-mesos-mini.md PRE-CREATION Diff: https://reviews.apache.org/r/69377/diff/4/ Changes: https://reviews.apache.org/r/69377/diff/3-4/ Testing --- Markdown rendering can be viewed here: https://github.com/jieyu/mesos/blob/mesos_mini_blog/site/source/blog/2018-11-19-mesos-mini.md Thanks, Jie Yu
Re: Review Request 69377: Added blog post for Mesos Mini.
> On Nov. 19, 2018, 8:02 a.m., Joerg Schad wrote: > > site/source/blog/2018-11-19-mesos-mini.md > > Lines 12 (patched) > > <https://reviews.apache.org/r/69377/diff/3/?file=2108621#file2108621line12> > > > > s/Mesos/Mesos and Marathon ? Marathon is just an example framework. I mentioned below. I want to install more example frameworks there (e.g., Aurora, etc.) > On Nov. 19, 2018, 8:02 a.m., Joerg Schad wrote: > > site/source/blog/2018-11-19-mesos-mini.md > > Lines 16 (patched) > > <https://reviews.apache.org/r/69377/diff/3/?file=2108621#file2108621line16> > > > > Should we explicitly call out limitations? Yes, I'll add a section about current limitations. > On Nov. 19, 2018, 8:02 a.m., Joerg Schad wrote: > > site/source/blog/2018-11-19-mesos-mini.md > > Lines 40 (patched) > > <https://reviews.apache.org/r/69377/diff/3/?file=2108621#file2108621line40> > > > > Are there configuration options for more agents? Not yet. - Jie --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69377/#review210643 --- On Nov. 19, 2018, 5:47 a.m., Jie Yu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69377/ > --- > > (Updated Nov. 19, 2018, 5:47 a.m.) > > > Review request for mesos, Benjamin Mahler, Chun-Hung Hsiao, Greg Mann, Jason > Lai, James DeFelice, James Peach, and Vinod Kone. > > > Repository: mesos > > > Description > --- > > Added blog post for Mesos Mini. > > > Diffs > - > > site/source/blog/2018-11-19-mesos-mini.md PRE-CREATION > > > Diff: https://reviews.apache.org/r/69377/diff/3/ > > > Testing > --- > > Markdown rendering can be viewed here: > https://github.com/jieyu/mesos/blob/mesos_mini_blog/site/source/blog/2018-11-19-mesos-mini.md > > > Thanks, > > Jie Yu > >
Re: Review Request 69377: Added blog post for Mesos Mini.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69377/ --- (Updated Nov. 19, 2018, 5:47 a.m.) Review request for mesos, Benjamin Mahler, Chun-Hung Hsiao, Greg Mann, Jason Lai, James DeFelice, James Peach, and Vinod Kone. Repository: mesos Description --- Added blog post for Mesos Mini. Diffs (updated) - site/source/blog/2018-11-19-mesos-mini.md PRE-CREATION Diff: https://reviews.apache.org/r/69377/diff/3/ Changes: https://reviews.apache.org/r/69377/diff/2-3/ Testing --- Markdown rendering can be viewed here: https://github.com/jieyu/mesos/blob/mesos_mini_blog/site/source/blog/2018-11-19-mesos-mini.md Thanks, Jie Yu
Re: Review Request 65811: Added Stout `path::normalize` function for POSIX paths.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65811/#review210638 --- Ship it! Ship It! - Jie Yu On Nov. 7, 2018, 10:10 p.m., Jason Lai wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/65811/ > --- > > (Updated Nov. 7, 2018, 10:10 p.m.) > > > Review request for mesos, Anish Gupta, Eric Chung, Gilbert Song, Jie Yu, > James Peach, and Zhitao Li. > > > Bugs: MESOS-8257 and MESOS-9009 > https://issues.apache.org/jira/browse/MESOS-8257 > https://issues.apache.org/jira/browse/MESOS-9009 > > > Repository: mesos > > > Description > --- > > Added `path::normalize` to normalize a given pathname and remove > redundant separators and up-level references. > > This function follows the rules described in `path_resolution(7)` > for Linux. However, it only performs pure lexical processing without > touching the actual filesystem. > > > Diffs > - > > 3rdparty/stout/include/stout/path.hpp > ef5a2f3d14cc8cf7416ded5cad00d30a49fd5cf7 > > > Diff: https://reviews.apache.org/r/65811/diff/9/ > > > Testing > --- > > `make tests and make check` with https://reviews.apache.org/r/68832/ > > > Thanks, > > Jason Lai > >
Re: Review Request 68832: Add unit tests for Stout `path::normalize` function in POSIX.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68832/#review210639 --- Ship it! Ship It! - Jie Yu On Nov. 7, 2018, 10:29 p.m., Jason Lai wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68832/ > --- > > (Updated Nov. 7, 2018, 10:29 p.m.) > > > Review request for mesos, Chun-Hung Hsiao, Eric Chung, Gilbert Song, Jie Yu, > James Peach, and Zhitao Li. > > > Bugs: MESOS-8257 and MESOS-9009 > https://issues.apache.org/jira/browse/MESOS-8257 > https://issues.apache.org/jira/browse/MESOS-9009 > > > Repository: mesos > > > Description > --- > > Add unit tests for Stout `path::normalize` function in POSIX. > > > Diffs > - > > 3rdparty/stout/tests/path_tests.cpp > 452865b919c0d3644eb0ece0e17e402318aaff41 > > > Diff: https://reviews.apache.org/r/68832/diff/2/ > > > Testing > --- > > `make check` > > > Thanks, > > Jason Lai > >
Re: Review Request 69377: Added blog post for Mesos Mini.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69377/ --- (Updated Nov. 18, 2018, 5:06 a.m.) Review request for mesos, Benjamin Mahler, Chun-Hung Hsiao, Greg Mann, James DeFelice, and James Peach. Repository: mesos Description --- Added blog post for Mesos Mini. Diffs (updated) - site/source/blog/2018-11-19-mesos-mini.md PRE-CREATION Diff: https://reviews.apache.org/r/69377/diff/2/ Changes: https://reviews.apache.org/r/69377/diff/1-2/ Testing --- Markdown rendering can be viewed here: https://github.com/jieyu/mesos/blob/mesos_mini_blog/site/source/blog/2018-11-19-mesos-mini.md Thanks, Jie Yu
Review Request 69377: Added blog post for Mesos Mini.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69377/ --- Review request for mesos, Benjamin Mahler, Chun-Hung Hsiao, James DeFelice, and James Peach. Repository: mesos Description --- Added blog post for Mesos Mini. Diffs - site/source/blog/2018-11-19-mesos-mini.md PRE-CREATION Diff: https://reviews.apache.org/r/69377/diff/1/ Testing --- Markdown rendering can be viewed here: https://github.com/jieyu/mesos/blob/mesos_mini_blog/site/source/blog/2018-11-19-mesos-mini.md Thanks, Jie Yu
Re: Review Request 69086: Moved container root construction to the isolators.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69086/#review210318 --- src/slave/containerizer/mesos/isolators/filesystem/linux.cpp Lines 73-80 (patched) <https://reviews.apache.org/r/69086/#comment294976> I would prefer simply use `ContainerMountInfo`, instead of introducing another struct. src/slave/containerizer/mesos/isolators/filesystem/linux.cpp Lines 179 (patched) <https://reviews.apache.org/r/69086/#comment294977> You don't need this function if you use `ContainerMountInfo` directly src/slave/containerizer/mesos/isolators/filesystem/linux.cpp Lines 181 (patched) <https://reviews.apache.org/r/69086/#comment294975> Please fix indentation (2 spaces) src/slave/containerizer/mesos/isolators/filesystem/linux.cpp Lines 623-631 (patched) <https://reviews.apache.org/r/69086/#comment294979> I don't think this is needed. `prepareMount` in launch.cpp will actual do this implicitly. Bindly doing rslave will cause shared mount propagation feature to not work (needed by CSI integration) src/slave/containerizer/mesos/launch.cpp Line 466 (original), 471 (patched) <https://reviews.apache.org/r/69086/#comment294978> Do we still need this function? Looks like this is just a verification now. Can you simply move the logic to the main `execute()` method? - Jie Yu On Oct. 30, 2018, 9:03 p.m., James Peach wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69086/ > --- > > (Updated Oct. 30, 2018, 9:03 p.m.) > > > Review request for mesos, Gilbert Song, Jason Lai, Jie Yu, and Jiang Yan Xu. > > > Bugs: MESOS-9319 > https://issues.apache.org/jira/browse/MESOS-9319 > > > Repository: mesos > > > Description > --- > > Previously, if the container was configured with a root filesytem, > the root was populated by a combination of the `fs::chroot:prepare` > API and the various isolators. The implementation details of some > isolators had leaked into the chroot code, which had a special case > for adding GPU devices. > > This change moves all the responsibility for defining the > root filesystem from the `fs::chroot::prepare()` API to the > `filesystem/linux` isolator. The `filesystem/linux` isolator is > now the single place that captures how to mount the container > pseudo-filesystems as well as how to construct a proper `/dev` > directory. > > Since the `linux/filesystem` isolator is now entirely responsible > for creating and mounting the container `/dev`, any other isolators > that enable access to devices can simply populate device nodes in > the container devices directory. After this change, the container > `/dev` is mounted read-only so that this cannot be used to escape > any disk quota. > > > Diffs > - > > src/linux/fs.hpp 31969f6ba82bf5ee549bfdf9698a21adaa486a90 > src/linux/fs.cpp 5cdffe1f4c7f00aee5b8f640e7cfa4a0018cfa0a > src/slave/containerizer/mesos/isolators/filesystem/linux.cpp > c7d753ac2e5575a8d687600bfb9e0617fa72c990 > src/slave/containerizer/mesos/isolators/gpu/isolator.hpp > 4645c625877d9451516133b24bd3959e0f49c0a9 > src/slave/containerizer/mesos/isolators/gpu/isolator.cpp > 56d835779618fd965d928c6926664583e9141f79 > src/slave/containerizer/mesos/isolators/linux/devices.cpp > 8f8ff95ec3856ba06647637a80315365d0e66e23 > src/slave/containerizer/mesos/launch.cpp > 7193da0a094df3e441e185c62b3a0379a0bdc4a2 > > > Diff: https://reviews.apache.org/r/69086/diff/6/ > > > Testing > --- > > sudo make check (Fedora 28) > > > Thanks, > > James Peach > >
Re: Review Request 69214: Mounted devpts with `gid=5` by default.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69214/#review210175 --- Ship it! Can you add a comment in the code about this gid=5 decision? - Jie Yu On Oct. 30, 2018, 4:46 p.m., James Peach wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69214/ > --- > > (Updated Oct. 30, 2018, 4:46 p.m.) > > > Review request for mesos, Gilbert Song, Jason Lai, Jie Yu, and Jiang Yan Xu. > > > Bugs: MESOS-8547 > https://issues.apache.org/jira/browse/MESOS-8547 > > > Repository: mesos > > > Description > --- > > Some versions of glibc (e.g. 2.17 from CentOS 7) are hard-coded > to expect that terminal devices are owned by the tty group. This > causes containers that allocate TTYs to expect to have to chown > the TTY (see grantpt code in glibc), but it may not be possible > to launch a privileged helper to perform the chown (e.g. because > capabilities have been dropped). > > Mounting devpts with `gid=5` is the default in CentOS, Docker, > Fedora and Ubuntu, so this should not cause any compatibility > problems. > > > Diffs > - > > src/linux/fs.cpp 3a58bf9a44c4e1d454f3d754952705b1fd0a0b1d > > > Diff: https://reviews.apache.org/r/69214/diff/1/ > > > Testing > --- > > sudo make check (Fedora 28) > > > Thanks, > > James Peach > >
Re: Review Request 69086: Move the container `/dev` construction to the isolators.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69086/#review210131 --- src/linux/fs.hpp Lines 397-401 (patched) <https://reviews.apache.org/r/69086/#comment294766> Any reason need this option? I was thinking just doing dev mounts always from linux fileystem isolator. src/linux/fs.cpp Line 697 (original), 675 (patched) <https://reviews.apache.org/r/69086/#comment294767> Can we do those in linux filesystem isolator too? src/slave/containerizer/mesos/isolators/filesystem/linux.cpp Lines 414 (patched) <https://reviews.apache.org/r/69086/#comment294759> Not related to this patch. When I review this patch, I was looking at paths.hpp and couldn't find any comments related to `devices` folder. Can you update the comments there (in the begining of `src/slave/containerizer/mesos/paths.hpp`) src/slave/containerizer/mesos/isolators/filesystem/linux.cpp Lines 417-420 (patched) <https://reviews.apache.org/r/69086/#comment294760> This sounds unnecessary given we just created an empty `launchInfo` above. We don't yet pass `launchInfo` to other isolators yet. src/slave/containerizer/mesos/isolators/filesystem/linux.cpp Lines 433 (patched) <https://reviews.apache.org/r/69086/#comment294763> Instead of CHECK_SOME, i'd still prefer we return a Failure here. src/slave/containerizer/mesos/isolators/filesystem/linux.cpp Lines 436 (patched) <https://reviews.apache.org/r/69086/#comment294764> Ditto. src/slave/containerizer/mesos/launch.cpp Lines 510 (patched) <https://reviews.apache.org/r/69086/#comment294765> Is this intentional? - Jie Yu On Oct. 19, 2018, 5:38 p.m., James Peach wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69086/ > --- > > (Updated Oct. 19, 2018, 5:38 p.m.) > > > Review request for mesos, Gilbert Song, Jason Lai, Jie Yu, and Jiang Yan Xu. > > > Bugs: MESOS-9319 > https://issues.apache.org/jira/browse/MESOS-9319 > > > Repository: mesos > > > Description > --- > > Previously, if the container was configured with a root filesystem, > the container `/dev` was populated by the chroot API and this API > had a special case for adding GPU devices. This change extends > the approach that was introduced in the `linux/devices` isolator > to construct the whole of the Linux container `/dev` hierarchy > before launching the container. The `linux/filesystem` isolator is > now responsible for mounting the container `/dev`, and any other > isolators that enable access to devices can simply populate device > nodes in the container devices directory. After this change, the > container `/dev` is mounted read-only so that this cannot be used > to escape any disk quota. > > > Diffs > - > > src/linux/fs.hpp 502f85c4a32d8658bdd701975dd5ac3d802d308e > src/linux/fs.cpp 9055ef42edd1fb90e1026d1d603a9ba902cfc1fd > src/slave/containerizer/mesos/isolators/filesystem/linux.cpp > a47899cb528eef103f299def3bd3466905ac5b51 > src/slave/containerizer/mesos/isolators/gpu/isolator.hpp > 4645c625877d9451516133b24bd3959e0f49c0a9 > src/slave/containerizer/mesos/isolators/gpu/isolator.cpp > dbbf92ffbe4a46cedca5b53f6ba172bfb308100e > src/slave/containerizer/mesos/isolators/linux/devices.cpp > 8f8ff95ec3856ba06647637a80315365d0e66e23 > src/slave/containerizer/mesos/launch.cpp > 7193da0a094df3e441e185c62b3a0379a0bdc4a2 > > > Diff: https://reviews.apache.org/r/69086/diff/3/ > > > Testing > --- > > sudo make check (Fedora 28) > > > Thanks, > > James Peach > >
Re: Review Request 69103: Fixed bug in 'execute.cpp' with tty-based tasks and no 'containerInfo'.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69103/#review209946 --- Ship it! Ship It! - Jie Yu On Oct. 20, 2018, 9:52 a.m., Kevin Klues wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69103/ > --- > > (Updated Oct. 20, 2018, 9:52 a.m.) > > > Review request for mesos, Armand Grillet, Gilbert Song, and Jie Yu. > > > Bugs: MESOS-8978 > https://issues.apache.org/jira/browse/MESOS-8978 > > > Repository: mesos > > > Description > --- > > Previously, we could only launch tasks using the '--tty' flag if they > had a backing docker image (or some other combination of other flags set > that would cause the task to have a 'containerInfo' created for it). > > This commit makes sure that if '--tty' is passed, that a containerInfo > gets created and its TTYInfo field gets populated. > > > Diffs > - > > src/cli/execute.cpp eb5c5646ed84533d39ad2588c78f1c0caa5fccfb > > > Diff: https://reviews.apache.org/r/69103/diff/1/ > > > Testing > --- > > sudo bin/mesos-master.sh \ > --ip=127.0.0.1 \ > --work_dir=/var/lib/mesos > > > sudo bin/mesos-agent.sh > --master=127.0.0.1:5050 \ > --work_dir=/var/lib/mesos > > > sudo src/mesos-execute \ > --master=127.0.0.1:5050 \ > --name=tty-test \ > --tty \ > --command="sh -i" > > $ mesos task attach tty-test > sh-4.3$ > > > Thanks, > > Kevin Klues > >
Re: Review Request 69009: Stout: Added a sync option for `write` and `rename`.
> On Oct. 15, 2018, 2:21 p.m., Benjamin Bannier wrote: > > 3rdparty/stout/include/stout/os/posix/rename.hpp > > Lines 41 (patched) > > <https://reviews.apache.org/r/69009/diff/2/?file=2097010#file2097010line41> > > > > While POSIX guarantees that `rename` e.g., does not see inconsistent > > state, there is nothing preventing `to` from being deleted once we execute > > the conditional code here. > > > > Since it is hard to know what semantics users expect _in general_, it > > might make more sense to not add the sync behavior to `rename`, but to > > e.g., ask users to perform `fsync` themself. > > Chun-Hung Hsiao wrote: > I considered this before. But `fsync`ing the directory is a > POSIX-specific thing, then the caller (e.g. `slave.cpp`) needs to write two > snippets for POSIX and Windows. Adding the `sync` option seems a better > option for making the code cross-platform. > > Also I don't understand your concern about the deletion. Can you > elaborate? > > Benjamin Bannier wrote: > Re:deletion, imagine us working in a temp dir which is aggressively > garbage-collected. We `::rename` `/tmp/mesos/a` to `/tmp/mesos/b`, but before > we `os::fsync` the directory `/tmp/mesos` gets garbage-collected; this would > make the `os::open` fail, even though the `::rename` finished successfully. > > It is not immediately obvious to me when such a scenario is a failure > when `sync=true`, which lead me to suggest to let callers handle the `sync` > (we could introduce a helper `sync(const& Path)` in addition to > `sync(int_fd)`. I also prefer introducing sync behavior in rename, for two reasons: 1) consistent with windows API 2) force the caller to think if `sync` is needed or not. If we ask callers to do sync as a second step, my worry is that most developers will forget. > On Oct. 15, 2018, 2:21 p.m., Benjamin Bannier wrote: > > 3rdparty/stout/include/stout/os/write.hpp > > Lines 119 (patched) > > <https://reviews.apache.org/r/69009/diff/2/?file=2097012#file2097012line119> > > > > I personally find flag arguments hard to read (`write(.., .., false)` > > or `write(.., .., true)` are not obvious). I'd suggest to either introduce > > a dedicated `write_sync` function or let callers trigger the `fsync`. > > Chun-Hung Hsiao wrote: > I agree that flag arguments sometimes are not easy to use but in this > case it seems fine to me. Let me also discuss this with Jie. I think this is fine and is consistent with others. I hope C++ language can address this (similar to python, groovy that allow .sync= true clause for function parameters) - Jie --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69009/#review209538 --- On Oct. 12, 2018, 11:53 p.m., Chun-Hung Hsiao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69009/ > --- > > (Updated Oct. 12, 2018, 11:53 p.m.) > > > Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Jie Yu, and > Jan Schlicht. > > > Bugs: MESOS-9281 > https://issues.apache.org/jira/browse/MESOS-9281 > > > Repository: mesos > > > Description > --- > > This patch adds an option for the caller to sync the file and directory > contents to the disk after a write to prevent filesystem inconsistency > against reboots. > > > Diffs > - > > 3rdparty/stout/include/stout/os/posix/rename.hpp > 9cff6db16c8a4e5a79bf0082e18a1133bd287796 > 3rdparty/stout/include/stout/os/windows/rename.hpp > 523912ac3bf315f70f542e8eab7d2d02249909b4 > 3rdparty/stout/include/stout/os/write.hpp > cd35285a9595004bac627abf687588050aef8161 > 3rdparty/stout/include/stout/protobuf.hpp > 1d03e1e3a8dd642f7239d777fb04759caf100a8b > > > Diff: https://reviews.apache.org/r/69009/diff/2/ > > > Testing > --- > > make check > > > Thanks, > > Chun-Hung Hsiao > >
Re: Review Request 68987: Fixed protobuf map equality check in the URI disk profile adaptor.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68987/#review209433 --- Ship it! Ship It! - Jie Yu On Oct. 11, 2018, 5:18 a.m., Chun-Hung Hsiao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68987/ > --- > > (Updated Oct. 11, 2018, 5:18 a.m.) > > > Review request for mesos and Jie Yu. > > > Bugs: MESOS-9308 > https://issues.apache.org/jira/browse/MESOS-9308 > > > Repository: mesos > > > Description > --- > > Fixed protobuf map equality check in the URI disk profile adaptor. > > > Diffs > - > > src/resource_provider/storage/uri_disk_profile_adaptor.cpp > d214af89f20f3e8184df83e89512e39e3daa1fc1 > > > Diff: https://reviews.apache.org/r/68987/diff/1/ > > > Testing > --- > > make check > > > Thanks, > > Chun-Hung Hsiao > >
Re: Review Request 68923: Updated Docker library to avoid 'os::killtree()' when discarding.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68923/#review209247 --- Ship it! Ship It! - Jie Yu On Oct. 4, 2018, 10:14 p.m., Greg Mann wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68923/ > --- > > (Updated Oct. 4, 2018, 10:14 p.m.) > > > Review request for mesos, Gilbert Song and Jie Yu. > > > Bugs: MESOS-9283 > https://issues.apache.org/jira/browse/MESOS-9283 > > > Repository: mesos > > > Description > --- > > This patch updates the Docker library to consistently use the > overload of subprocess() which runs its binary via exec() > rather than through a shell. This makes it safe to use > os::kill() rather than os::killtree() when discarding > these commands, which this patch also accomplishes. > > > Diffs > - > > src/docker/docker.hpp 25d9ca662fa5d99b32c668a5fdfc75584132cc38 > src/docker/docker.cpp fb39f7480045c225096e07d7d55cd3aa7b870bc5 > > > Diff: https://reviews.apache.org/r/68923/diff/2/ > > > Testing > --- > > `sudo bin/mesos-tests.sh --gtest_filter="*DOCKER*"` > > > Thanks, > > Greg Mann > >
Re: Review Request 68924: Updated the Docker library to avoid 'os::killtree()'.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68924/#review209233 --- Ship it! Ship It! - Jie Yu On Oct. 4, 2018, 5:26 a.m., Greg Mann wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68924/ > --- > > (Updated Oct. 4, 2018, 5:26 a.m.) > > > Review request for mesos, Gilbert Song and Jie Yu. > > > Bugs: MESOS-9283 > https://issues.apache.org/jira/browse/MESOS-9283 > > > Repository: mesos > > > Description > --- > > This patch updates the Docker library to consistently use the > overload of `subprocess()` which runs its binary via `exec()` > rather than through a shell. This makes it safe to use > `os::kill()` rather than `os::killtree()` when discarding > these commands, which this patch also accomplishes. > > > Diffs > - > > src/docker/docker.cpp fb39f7480045c225096e07d7d55cd3aa7b870bc5 > > > Diff: https://reviews.apache.org/r/68924/diff/1/ > > > Testing > --- > > `sudo bin/mesos-tests.sh --gtest_filter="*DOCKER*"` > > > Thanks, > > Greg Mann > >
Re: Review Request 68923: Updated 'Docker::inspect()' to avoid 'os::killtree()'.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68923/#review209231 --- Ship it! Ship It! - Jie Yu On Oct. 4, 2018, 5:22 a.m., Greg Mann wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68923/ > --- > > (Updated Oct. 4, 2018, 5:22 a.m.) > > > Review request for mesos, Gilbert Song and Jie Yu. > > > Bugs: MESOS-9283 > https://issues.apache.org/jira/browse/MESOS-9283 > > > Repository: mesos > > > Description > --- > > When futures returned by `Docker::inspect()` are discarded > frequently, agent node performance can be negatively > impacted due to the `os::killtree()` call in the discard > handler. > > This patch avoids running `docker inspect` commands through > a shell so that it's safe to use `os::kill()` when > discarding the returned futures. > > This change is being made independently from similar > changes to the rest of the Docker library so that it can be > more easily backported to previous versions, since issues > related to `Docker::inspect()` in particular have been > observed. > > > Diffs > - > > src/docker/docker.hpp 25d9ca662fa5d99b32c668a5fdfc75584132cc38 > src/docker/docker.cpp fb39f7480045c225096e07d7d55cd3aa7b870bc5 > > > Diff: https://reviews.apache.org/r/68923/diff/1/ > > > Testing > --- > > `sudo bin/mesos-tests.sh --gtest_filter="*DOCKER*"` > > > Thanks, > > Greg Mann > >
Re: Review Request 68810: Removed call to 'setsid()' in command executor if TTY attached.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68810/#review209133 --- Ship it! Ship It! - Jie Yu On Sept. 29, 2018, 12:05 p.m., Kevin Klues wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68810/ > --- > > (Updated Sept. 29, 2018, 12:05 p.m.) > > > Review request for mesos, Gilbert Song and Jie Yu. > > > Bugs: MESOS-8978 > https://issues.apache.org/jira/browse/MESOS-8978 > > > Repository: mesos > > > Description > --- > > Previously, we would unconditionally call 'setsid()' in the command > executor whenever we launched a process. This causes the process it > launches to start a new session and NOT inherit the controlling TTY > from it's parent. This obviously causes problems, if the goal of > attaching a TTY to a task is so that we can actually give it control > of that TTY while it is executing. > > Interestingly, even if process does not control its TTY, it can still > read and write from the file descriptors associated with it (it just > can't receive any signals associated with it, such as SIGWINCH, > SIGHUP, etc.). This is why things "appeared" to mostly work until this > point because stdin/stdout/stderr were all being redirected properly > to it. > > Where we saw problems was with trying to 'attach' to an already > running process launched via the command executor using the > ATTACH_NESTED_CONTAINER_INPUT/OUTPUT calls. If you ran something like > 'bash', you would not be able to do job control, and you could not > resize the screen properly when running things like 'vim'. > > This commit fixes this issue by checking to see if a TTY has been > attached to a task launched by the command executor, and skipping the > 'setsid()' call when forking it. We considered a number of alternative > approaches, but finally settled on this one since it was the least > invasive. I.e. only tasks launched with a TTY (which is a failry new > concept in Mesos) will be affected by this change; the semantics of > all traditional tasks launched via the command executor will go > unchanged. > > Note: this problem does not exists for the default executor because > the agent does the job of launching all containers, and there is no > executor sitting between the containerizer and the actual process of a > task intervening to call an extra 'setsid()'. This is why containers > launched via LAUNCH_NESTED_CONTAINER_SESSION have always worked as > expected. > > > Diffs > - > > src/launcher/executor.cpp 86fb87dfbaa4daba22ec491ca5fca0e562797521 > src/slave/containerizer/mesos/io/switchboard.cpp > 498c008c36725b4ed43b4905d3640508beaaae93 > > > Diff: https://reviews.apache.org/r/68810/diff/2/ > > > Testing > --- > > $ make -j 40 check > > > Thanks, > > Kevin Klues > >
Re: Review Request 68880: Cached the cgroup results in Docker containerizer.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68880/ --- (Updated Sept. 28, 2018, 11:09 p.m.) Review request for mesos, Gilbert Song, Greg Mann, and Vinod Kone. Bugs: MESOS-9279 https://issues.apache.org/jira/browse/MESOS-9279 Repository: mesos Description --- Since the cgroup hierarchy results won't change, it does not make sense to compute it every time `usage` is called. It will get quite expensivie when the host mount table is big (e.g., MESOS-8418). This patch uses the static local variable to cache the result. Diffs - src/slave/containerizer/docker.cpp 277a1550e199097ebc3c47d0a6c0d258bac90da5 Diff: https://reviews.apache.org/r/68880/diff/1/ Testing --- sudo make check Thanks, Jie Yu
Review Request 68880: Cached the cgroup results in Docker containerizer.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68880/ --- Review request for mesos, Gilbert Song, Greg Mann, and Vinod Kone. Repository: mesos Description --- Since the cgroup hierarchy results won't change, it does not make sense to compute it every time `usage` is called. It will get quite expensivie when the host mount table is big (e.g., MESOS-8418). This patch uses the static local variable to cache the result. Diffs - src/slave/containerizer/docker.cpp 277a1550e199097ebc3c47d0a6c0d258bac90da5 Diff: https://reviews.apache.org/r/68880/diff/1/ Testing --- sudo make check Thanks, Jie Yu
Review Request 68862: Removed unneeded pluginDir field from CNI isolator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68862/ --- Review request for mesos, Gilbert Song, Qian Zhang, and Sergey Urbanovich. Repository: mesos Description --- The field member is redundant as it's already included in the flags. Diffs - src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 4ac55c8989f80c1d6df05919a55ec40391d415f8 src/slave/containerizer/mesos/isolators/network/cni/cni.cpp ed470465726227a61216b467ad1a7f030806008f Diff: https://reviews.apache.org/r/68862/diff/1/ Testing --- sudo make check Thanks, Jie Yu
Review Request 68861: Skipped CNI config load if named network is not enabled.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68861/ --- Review request for mesos, Gilbert Song, Qian Zhang, and Sergey Urbanovich. Bugs: MESOS-9267 https://issues.apache.org/jira/browse/MESOS-9267 Repository: mesos Description --- If the operator didn't turn on named CNI network support (i.e., both agent flags `network_cni_config_dir` and `network_cni_plugins_dir` are not specified), the CNI should not attempt to load the network configs. This patch fixed a potential CHECK failure. Diffs - src/slave/containerizer/mesos/isolators/network/cni/cni.cpp ed470465726227a61216b467ad1a7f030806008f Diff: https://reviews.apache.org/r/68861/diff/1/ Testing --- sudo make check Thanks, Jie Yu
Re: Review Request 68810: Removed call to 'setsid()' in command executor if TTY attached.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68810/#review209034
---
src/launcher/executor.cpp
Lines 520 (patched)
<https://reviews.apache.org/r/68810/#comment293304>
ttySlavePath.isNone()
src/slave/containerizer/mesos/io/switchboard.cpp
Line 446 (original), 446-448 (patched)
<https://reviews.apache.org/r/68810/#comment293303>
This applies to default executor as well. You should only do this for
command executor. I.e.,
```
if (containerConfig.has_task_info()) {
launchInfo.mutable_command()->add_arguments(
"--tty_slave_path=" + slavePath.get());
}
```
- Jie Yu
On Sept. 22, 2018, 12:36 a.m., Kevin Klues wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68810/
> ---
>
> (Updated Sept. 22, 2018, 12:36 a.m.)
>
>
> Review request for mesos, Gilbert Song and Jie Yu.
>
>
> Bugs: MESOS-8978
> https://issues.apache.org/jira/browse/MESOS-8978
>
>
> Repository: mesos
>
>
> Description
> ---
>
> Previously, we would unconditionally call 'setsid()' in the command
> executor whenever we launched a process. This causes the process it
> launches to start a new session and NOT inherit the controlling TTY
> from it's parent. This obviously causes problems, if the goal of
> attaching a TTY to a task is so that we can actually give it control
> of that TTY while it is executing.
>
> Interestingly, even if process does not control its TTY, it can still
> read and write from the file descriptors associated with it (it just
> can't receive any signals associated with it, such as SIGWINCH, NOHUP,
> etc.). This is why things "appeared" to mostly work until this point
> because stdin/stdout/stderr were all being redirected properly to it.
>
> Where we saw problems was with trying to 'attach' to an already
> running process launched via the command executor using the
> ATTACH_NESTED_CONTAINER_INPUT/OUTPUT calls. If you ran something like
> 'bash', you would not be able to do job control, and you could not
> resize the screen properly when running things like 'vim'.
>
> This commit fixes this issue by checking to see if a TTY has been
> attached to a tasng launched by the command executor, and skipping the
> 'setsid()' call when forking it. We considered a number of alternative
> approaches, but finally settled on this one since it was the least
> invasive. I.e. only tasks launched with a TTY (which is a failry new
> concept in Mesos) will be affected by this change; the semantics of
> all traditional tasks launched via the command executor will go
> unchanged.
>
> Note: this problem does not exists for the default executor because
> the agent does the job of launching all containers, and there is no
> executor sitting between the containerizer and the actual process of a
> task intervening to call an extra 'setsid()'. This is why containers
> launched via LAUNCH_NESTED_CONTAINER_SESSION have always worked as
> expected.
>
>
> Diffs
> -
>
> src/launcher/executor.cpp 86fb87dfbaa4daba22ec491ca5fca0e562797521
> src/slave/containerizer/mesos/io/switchboard.cpp
> 52b0e521ed1c651c90b3a3df7c4df576288bf400
>
>
> Diff: https://reviews.apache.org/r/68810/diff/1/
>
>
> Testing
> ---
>
> $ make -j 40 check
>
>
> Thanks,
>
> Kevin Klues
>
>
Re: Review Request 68724: Added the ability to launch tasks with a TTY attached to mesos-execute.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68724/#review209030
---
Ship it!
Ship It!
- Jie Yu
On Sept. 22, 2018, 12:35 a.m., Kevin Klues wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68724/
> ---
>
> (Updated Sept. 22, 2018, 12:35 a.m.)
>
>
> Review request for mesos, Gilbert Song and Jie Yu.
>
>
> Bugs: MESOS-8978
> https://issues.apache.org/jira/browse/MESOS-8978
>
>
> Repository: mesos
>
>
> Description
> ---
>
> Added the ability to launch tasks with a TTY attached to mesos-execute.
>
>
> Diffs
> -
>
> src/cli/execute.cpp e0a8fc89112223c6b0516ddec0587047338fa264
>
>
> Diff: https://reviews.apache.org/r/68724/diff/1/
>
>
> Testing
> ---
>
> sudo bin/mesos-master.sh \
> --ip=127.0.0.1 \
> --work_dir=/var/lib/mesos
>
> sudo bin/mesos-agent.sh
> --master=127.0.0.1:5050 \
> --work_dir=/var/lib/mesos \
> --image_providers=docker \
> --executor_environment_variables="{}" \
> --isolation="cgroups/all,filesystem/linux,docker/runtime,namespaces/pid"
>
>
> sudo src/mesos-execute \
> --master=127.0.0.1:5050 \
> --name=tty-test \
> --docker_image=library/alpine \
> --no-shell \
> --tty \
> --command="sh -i"
>
>
> Thanks,
>
> Kevin Klues
>
>
Re: Review Request 68763: Cleaned up residual containers when removing resource provider configs.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68763/#review208891 --- Fix it, then Ship it! src/resource_provider/daemon.cpp Lines 129 (patched) <https://reviews.apache.org/r/68763/#comment293164> s/removed/removing/ ? src/resource_provider/daemon.cpp Lines 556 (patched) <https://reviews.apache.org/r/68763/#comment293183> It's very unfortunate that we have this SLRP specific code in the general LRP daemon. I'd add a very big TODO here about the proposed solution for refactoring. I think it's possible that we can intorduce a virtual method for LRP interface (e.g., `shutdown`). If the RP instance is not yet available, the `remove` will simply skip the `shutdown` step. Since the launch of an RP is async (due to `generateAuthToken(data.info)`), as a result, we need to be careful there. We can either fail the launch by checking the state in `_launch`, or chain remove under launch. Sounds like the former is easier. Anyway, let's record a big TODO here. src/resource_provider/daemon.cpp Lines 686-688 (patched) <https://reviews.apache.org/r/68763/#comment293184> Can we LOG all errors in the `futures` list, not just the first one? You need to combine the error messages. - Jie Yu On Sept. 21, 2018, 7:47 p.m., Chun-Hung Hsiao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68763/ > --- > > (Updated Sept. 21, 2018, 7:47 p.m.) > > > Review request for mesos, Benjamin Bannier, Jie Yu, and Jan Schlicht. > > > Bugs: MESOS-9228 > https://issues.apache.org/jira/browse/MESOS-9228 > > > Repository: mesos > > > Description > --- > > When processing `REMOVE_RESOURCE_PROVIDER_CONFIG`, the local resource > provider daemon now performs a best-effort cleanup by killing all > standalone containers prefixed by the 'cid_prefix' of the resource > provider. During the cleanup, no resource provider config with the same > type and name can be added. > > > Diffs > - > > src/resource_provider/daemon.cpp 0a76cc6d1d34413674d1af1aa514679a4d2b7b55 > > > Diff: https://reviews.apache.org/r/68763/diff/3/ > > > Testing > --- > > make check > > > Thanks, > > Chun-Hung Hsiao > >
Re: Review Request 68769: Fixed rtnl_act leak in routing::filter::internal::attach().
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68769/#review208771 --- Ship it! Ship It! - Jie Yu On Sept. 19, 2018, 6:27 p.m., Ilya Pronin wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68769/ > --- > > (Updated Sept. 19, 2018, 6:27 p.m.) > > > Review request for mesos and Jie Yu. > > > Repository: mesos > > > Description > --- > > We should free the action once we've attached it to a filter, because > rtnl_basic_add_action() and rtnl_u32_add_action() increase the > refcounter of rtnl_act object upon adding it to the filter's list. > > Reference grabbing was added to libnl along with a corresponding > capability NL_CAPABILITY_ROUTE_LINK_CLS_ADD_ACT_OWN_REFERENCE. Its > support is already checked by Mesos in routing::check(). It seems that > we simply forgot to adapt our code. > > > Diffs > - > > src/linux/routing/filter/internal.hpp > 4be4797f84947c46bf3bf629719c13370be5ec1b > > > Diff: https://reviews.apache.org/r/68769/diff/1/ > > > Testing > --- > > Tested with Valgrind Memcheck. Ran `sudo make check`. > > > Thanks, > > Ilya Pronin > >
Review Request 68728: Added rapidjson to RPM spec file.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68728/ --- Review request for mesos, Benjamin Mahler and Till Toenshoff. Bugs: MESOS-9238 https://issues.apache.org/jira/browse/MESOS-9238 Repository: mesos Description --- We didn't update the RPM spec file when we added the rapidjson dependencies. This will cause RPM build to fail like the following: RPM build errors: error: Installed (but unpackaged) file(s) found: /usr/include/rapidjson/allocators.h /usr/include/rapidjson/document.h ... This patch fixed it by adding the rapidjson headers to the %files section accordingly. Diffs - support/packaging/centos/mesos.spec 053298522ee198b3b23004ff9ce73aa60a39e8a4 Diff: https://reviews.apache.org/r/68728/diff/1/ Testing --- Thanks, Jie Yu
Re: Review Request 68614: Implicitly authorized `VIEW_STANDALONE_CONTAINER` for SLRPs.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68614/#review208288 --- Ship it! Ship It! - Jie Yu On Sept. 3, 2018, 10:22 p.m., Chun-Hung Hsiao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68614/ > --- > > (Updated Sept. 3, 2018, 10:22 p.m.) > > > Review request for mesos, Benjamin Bannier and Jie Yu. > > > Bugs: MESOS-8429 > https://issues.apache.org/jira/browse/MESOS-8429 > > > Repository: mesos > > > Description > --- > > Implicitly authorized `VIEW_STANDALONE_CONTAINER` for SLRPs. > > > Diffs > - > > src/authorizer/local/authorizer.cpp > f99b88e10df1e0959f1ddd2e45374862c2dc0a5b > src/slave/http.cpp d0f6e1c5ffcc22cd280c64b20473f279ab4c14cc > > > Diff: https://reviews.apache.org/r/68614/diff/1/ > > > Testing > --- > > make check > > > Thanks, > > Chun-Hung Hsiao > >
Re: Review Request 68615: Added the `devolve` helper for agent v1 API responses.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68615/#review208283 --- Ship it! Ship It! - Jie Yu On Sept. 3, 2018, 10:25 p.m., Chun-Hung Hsiao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68615/ > --- > > (Updated Sept. 3, 2018, 10:25 p.m.) > > > Review request for mesos, Benjamin Bannier and Jie Yu. > > > Bugs: MESOS-8429 > https://issues.apache.org/jira/browse/MESOS-8429 > > > Repository: mesos > > > Description > --- > > Added the `devolve` helper for agent v1 API responses. > > > Diffs > - > > src/internal/devolve.hpp 1bc2a32e70600c785660d0b1d5d63378ffe344cc > src/internal/devolve.cpp 93bd975b64a9d83e34f4d7420b3b49f06d1f > > > Diff: https://reviews.apache.org/r/68615/diff/1/ > > > Testing > --- > > make check > > > Thanks, > > Chun-Hung Hsiao > >
Re: Review Request 68601: Cleaned up residual CSI endpoint sockets for terminated plugins.
> On Sept. 2, 2018, 3:37 a.m., Jie Yu wrote: > > src/authorizer/local/authorizer.cpp > > Lines 1138 (patched) > > <https://reviews.apache.org/r/68601/diff/1/?file=2079385#file2079385line1138> > > > > you might also need VIEW_CONTAINERS because `GetContainers` will always > > show executor containers. > > Chun-Hung Hsiao wrote: > When showing the executor containers, the agent won't approve > `VIEW_CONTAINERS`, and they will be simply ignored, so it seems that > `VIEW_CONTAINERS` is not needed: > > https://github.com/apache/mesos/blob/6f010b6b61433fd783e5619ed0cada298b8fe30e/src/slave/http.cpp#L2177 IC, so basically those containers won't be shown if not authorized. This is a bit hacky, but I think it's fine for now. Let's add a TODO to the `GetContainers` API to allow us to disallow showing executor containers. In that way, we can only view standalone containers. Then, we don't have this issue. - Jie --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68601/#review208234 --- On Sept. 3, 2018, 10:31 p.m., Chun-Hung Hsiao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68601/ > --- > > (Updated Sept. 3, 2018, 10:31 p.m.) > > > Review request for mesos, Benjamin Bannier and Jie Yu. > > > Bugs: MESOS-8429 > https://issues.apache.org/jira/browse/MESOS-8429 > > > Repository: mesos > > > Description > --- > > If a CSI plugin is crashed during agent failover, the residual socket > file would exist during SLRP recovery, which may in turn make the plugin > fail to restart. This patch cleans up the residual socket files to avoid > such failures. > > > Diffs > - > > src/resource_provider/storage/provider.cpp > ab1467c22ded269cb42fc52d3f00fb936fc42c7f > > > Diff: https://reviews.apache.org/r/68601/diff/3/ > > > Testing > --- > > sudo make check > > > Thanks, > > Chun-Hung Hsiao > >
Re: Review Request 68601: Cleaned up residual CSI endpoint sockets for terminated plugins.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68601/#review208234 --- src/authorizer/local/authorizer.cpp Lines 1138 (patched) <https://reviews.apache.org/r/68601/#comment291964> you might also need VIEW_CONTAINERS because `GetContainers` will always show executor containers. src/resource_provider/storage/provider.cpp Lines 2051 (patched) <https://reviews.apache.org/r/68601/#comment291962> I'd explictly set `show_nested` to be false so that we are not affected if the default behavior changes. src/resource_provider/storage/provider.cpp Lines 2158-2175 (patched) <https://reviews.apache.org/r/68601/#comment291963> Do we need this? I was thinking that re-using the same sandbox could allow us to see the log history. - Jie Yu On Sept. 1, 2018, 5:31 p.m., Chun-Hung Hsiao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68601/ > --- > > (Updated Sept. 1, 2018, 5:31 p.m.) > > > Review request for mesos, Benjamin Bannier and Jie Yu. > > > Bugs: MESOS-8429 > https://issues.apache.org/jira/browse/MESOS-8429 > > > Repository: mesos > > > Description > --- > > If a CSI plugin is crashed during agent failover, the residual socket > file would exist during SLRP recovery, which may in turn make the plugin > fail to restart. This patch cleans up the residual socket files to avoid > such failures. > > > Diffs > - > > src/authorizer/local/authorizer.cpp > f99b88e10df1e0959f1ddd2e45374862c2dc0a5b > src/internal/devolve.hpp 1bc2a32e70600c785660d0b1d5d63378ffe344cc > src/internal/devolve.cpp 93bd975b64a9d83e34f4d7420b3b49f06d1f > src/resource_provider/storage/provider.cpp > ab1467c22ded269cb42fc52d3f00fb936fc42c7f > src/slave/http.cpp d0f6e1c5ffcc22cd280c64b20473f279ab4c14cc > > > Diff: https://reviews.apache.org/r/68601/diff/1/ > > > Testing > --- > > sudo make check > > > Thanks, > > Chun-Hung Hsiao > >
Re: Review Request 68600: Added a unit test for plugin crash during an agent failover.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68600/#review208233 --- Ship it! Ship It! - Jie Yu On Sept. 1, 2018, 5:31 p.m., Chun-Hung Hsiao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68600/ > --- > > (Updated Sept. 1, 2018, 5:31 p.m.) > > > Review request for mesos, Benjamin Bannier and Jie Yu. > > > Bugs: MESOS-8429 > https://issues.apache.org/jira/browse/MESOS-8429 > > > Repository: mesos > > > Description > --- > > If a CSI plugin is crashed during agent failover, the residual socket > file would exist during SLRP recovery. This test verifies that the > plugin is properly cleaned up during recovery so the plugin can be > restarted. > > > Diffs > - > > src/examples/test_csi_plugin.cpp 93301f8af9b494bbcecb10e6cd4f4073b6a05070 > src/tests/storage_local_resource_provider_tests.cpp > 9d0020cb22d8f674c6271712a93db7f446824d55 > > > Diff: https://reviews.apache.org/r/68600/diff/1/ > > > Testing > --- > > make check > NOTE: This test will fail without the following patch. > > > Thanks, > > Chun-Hung Hsiao > >
Re: Review Request 68599: Added an unit test for rootfs cleanup EBUSY fix.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68599/#review208232
---
Fix it, then Ship it!
src/tests/containerizer/provisioner_docker_tests.cpp
Lines 1140-1142 (patched)
<https://reviews.apache.org/r/68599/#comment291961>
I'd swap the order of these two.
- Jie Yu
On Sept. 1, 2018, 6:15 p.m., Gilbert Song wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68599/
> ---
>
> (Updated Sept. 1, 2018, 6:15 p.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao and Jie Yu.
>
>
> Bugs: MESOS-9196
> https://issues.apache.org/jira/browse/MESOS-9196
>
>
> Repository: mesos
>
>
> Description
> ---
>
> Added an unit test for rootfs cleanup EBUSY fix.
>
>
> Diffs
> -
>
> src/tests/containerizer/provisioner_docker_tests.cpp
> bf56d603fcc1d99f73beca1871be45787fa1640d
>
>
> Diff: https://reviews.apache.org/r/68599/diff/2/
>
>
> Testing
> ---
>
> make check
>
> Verified that the test failed on aufs and overlayfs without the EBUSY fix:
> ```
> [ RUN ]
> BackendFlag/ProvisionerDockerBackendTest.ROOT_INTERNET_CURL_DTYPE_RootfsCleanup/0
> sh: 1: hadoop: not found
> Marked '/' as rslave
> Prepared mount
> '{"flags":20480,"source":"/tmp/BackendFlag_ProvisionerDockerBackendTest_ROOT_INTERNET_CURL_DTYPE_RootfsCleanup_0_LFYXIA/slaves/e27d1fe4-708d-4d13-9df2-92f89eb50597-S0/frameworks/e27d1fe4-708d-4d13-9df2-92f89eb50597-/executors/8438ddd3-101b-448a-8a01-09f65e39d82f/runs/b51eead9-eb1a-4764-9200-7ddb9944daa2","target":"/tmp/BackendFlag_ProvisionerDockerBackendTest_ROOT_INTERNET_CURL_DTYPE_RootfsCleanup_0_LFYXIA/provisioner/containers/b51eead9-eb1a-4764-9200-7ddb9944daa2/backends/copy/rootfses/d89a35dd-5ac1-4a01-8ee8-fd757fa2cc1b/mnt/mesos/sandbox"}'
> I0901 09:13:02.950115 2165 exec.cpp:162] Version: 1.8.0
> I0901 09:13:02.998127 2190 exec.cpp:236] Executor registered on agent
> e27d1fe4-708d-4d13-9df2-92f89eb50597-S0
> I0901 09:13:03.003624 2189 executor.cpp:182] Received SUBSCRIBED event
> I0901 09:13:03.005172 2189 executor.cpp:186] Subscribed executor on
> vagrant-ubuntu-wily-64
> I0901 09:13:03.005786 2189 executor.cpp:182] Received LAUNCH event
> I0901 09:13:03.008103 2189 executor.cpp:679] Starting task
> 8438ddd3-101b-448a-8a01-09f65e39d82f
> I0901 09:13:03.010752 2189 executor.cpp:499] Running
> '/vagrant/mesos/build/src/mesos-containerizer launch
> '
> I0901 09:13:03.046313 2189 executor.cpp:693] Forked command at 2192
> [ OK ]
> BackendFlag/ProvisionerDockerBackendTest.ROOT_INTERNET_CURL_DTYPE_RootfsCleanup/0
> (14789 ms)
> [ RUN ]
> BackendFlag/ProvisionerDockerBackendTest.ROOT_INTERNET_CURL_DTYPE_RootfsCleanup/1
> sh: 1: hadoop: not found
> Marked '/' as rslave
> Prepared mount
> '{"flags":20480,"source":"/tmp/BackendFlag_ProvisionerDockerBackendTest_ROOT_INTERNET_CURL_DTYPE_RootfsCleanup_1_M7142L/slaves/514ddbb6-76d5-4014-b568-86be14c7c72e-S0/frameworks/514ddbb6-76d5-4014-b568-86be14c7c72e-/executors/eaea3cb2-e0fc-4460-8a53-805ec5e6c609/runs/f714ee27-d960-455a-9821-c6dd8ebaeea8","target":"/tmp/BackendFlag_ProvisionerDockerBackendTest_ROOT_INTERNET_CURL_DTYPE_RootfsCleanup_1_M7142L/provisioner/containers/f714ee27-d960-455a-9821-c6dd8ebaeea8/backends/aufs/rootfses/7731d007-43f7-4bd4-8557-e76d5fac0bfa/mnt/mesos/sandbox"}'
> I0901 09:13:10.636891 2286 exec.cpp:162] Version: 1.8.0
> I0901 09:13:10.661113 2289 exec.cpp:236] Executor registered on agent
> 514ddbb6-76d5-4014-b568-86be14c7c72e-S0
> I0901 09:13:10.666904 2287 executor.cpp:182] Received SUBSCRIBED event
> I0901 09:13:10.667912 2287 executor.cpp:186] Subscribed executor on
> vagrant-ubuntu-wily-64
> I0901 09:13:10.668098 2287 executor.cpp:182] Received LAUNCH event
> I0901 09:13:10.670292 2287 executor.cpp:679] Starting task
> eaea3cb2-e0fc-4460-8a53-805ec5e6c609
> I0901 09:13:10.673161 2287 executor.cpp:499] Running
> '/vagrant/mesos/build/src/mesos-containerizer launch
> '
> I0901 09:13:10.681861 2287 executor.cpp:693] Forked command at 2291
> ../../src/tests/containerizer/provisioner_docker_tests.cpp:1143: Failure
> (wait).failure(): Failed to destroy the provisioned rootfs when destroying
> container: Collect failed: Failed to destroy aufs-mounted rootfs
> '/tmp/BackendFlag_ProvisionerDockerBackendTest_ROOT_INTERNET_CURL_DTYPE_RootfsCleanup_1_M7142L/provi
Review Request 68598: Added a missing failure message in overlay backend.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68598/ --- Review request for mesos, Gilbert Song and Zhitao Li. Repository: mesos Description --- Added a missing failure message in overlay backend. Diffs - src/slave/containerizer/mesos/provisioner/backends/overlay.cpp e32b9918ac15122477e05ebe701c9425bbefa809 Diff: https://reviews.apache.org/r/68598/diff/1/ Testing --- Thanks, Jie Yu
Re: Review Request 68597: Made copy backend destroy more robust.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68597/ --- (Updated Sept. 1, 2018, 4:50 a.m.) Review request for mesos, Chun-Hung Hsiao and Gilbert Song. Changes --- Fixed typos. Bugs: MESOS-9196 https://issues.apache.org/jira/browse/MESOS-9196 Repository: mesos Description --- Do not return hard failure if `rm -rf` failed. It's also possible that it fails if some other processes are accessing the files there. The cleanup will be re-attempted during provisioner destroy and agent recovery. Diffs (updated) - src/slave/containerizer/mesos/provisioner/backends/copy.cpp d3eb6b443d49e1cd6cfd7fa3be1776155e8a1bb0 Diff: https://reviews.apache.org/r/68597/diff/2/ Changes: https://reviews.apache.org/r/68597/diff/1-2/ Testing --- sudo make check Thanks, Jie Yu
Re: Review Request 68594: Made overlay backend destroy more robust.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68594/ --- (Updated Sept. 1, 2018, 4:50 a.m.) Review request for mesos, Chun-Hung Hsiao and Gilbert Song. Changes --- Fixed typos. Bugs: MESOS-9196 https://issues.apache.org/jira/browse/MESOS-9196 Repository: mesos Description (updated) --- Use MNT_DETACH for `unmount` so that if there are still processes holding files or directories in the rootfs (e.g., regular filesystem indexing), the unmount will still be successful. The kernel will cleanup the mount when the number of references reach zero. Also, do not return hard failure if `rmdir` failed. It's also possible that `rmdir` returns EBUSY. See more details in MESOS-9196. Diffs (updated) - src/slave/containerizer/mesos/provisioner/backends/overlay.cpp e32b9918ac15122477e05ebe701c9425bbefa809 Diff: https://reviews.apache.org/r/68594/diff/2/ Changes: https://reviews.apache.org/r/68594/diff/1-2/ Testing --- sudo make check Thanks, Jie Yu
Re: Review Request 68596: Made bind backend destroy more robust.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68596/ --- (Updated Sept. 1, 2018, 4:50 a.m.) Review request for mesos, Chun-Hung Hsiao and Gilbert Song. Changes --- Fixed typos Bugs: MESOS-9196 https://issues.apache.org/jira/browse/MESOS-9196 Repository: mesos Description (updated) --- Use MNT_DETACH for `unmount` so that if there are still processes holding files or directories in the rootfs (e.g., regular filesystem indexing), the unmount will still be successful. The kernel will cleanup the mount when the number of references reach zero. See more details in MESOS-9196. Diffs (updated) - src/slave/containerizer/mesos/provisioner/backends/bind.cpp 6ad97f98ceb8ceae50d786bfbbf49fd0eebc5c21 Diff: https://reviews.apache.org/r/68596/diff/2/ Changes: https://reviews.apache.org/r/68596/diff/1-2/ Testing --- sudo make check Thanks, Jie Yu
