Re: Review Request 68426: Refactored some cgroups helpers to do verify from callers.

2018-08-19 Thread Jie Yu

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

(Updated Aug. 20, 2018, 4:46 a.m.)


Review request for mesos, Benjamin Mahler, Gilbert Song, and James Peach.


Bugs: MESOS-8418 and MESOS-9081
https://issues.apache.org/jira/browse/MESOS-8418
https://issues.apache.org/jira/browse/MESOS-9081


Repository: mesos


Description
---

Prior to this patch, some of the cgroups helpers call `cgroups::verify`
to make sure the hierarchy and cgroup are valid. This causes some
performance issues for the agent because `cgroups::verify` will read the
entire mount table, and mount table read is expensive if the mount table
is large. See more details in MESOS-8418.

In most of the cases, the verification has already been done when those
cgroups helpers are called. Therefore, it is better to let the caller do
the verification and make those cgroups helpers pure helpers.

Also, the verification is subject to race anyway. The cgroups global
state might change (e.g., operators modify the cgroups) after the
verification is done, making the verification itself not that super
useful.

This patch refactored the cgroups helpers and moved the verifications to
the callers.


Diffs
-

  src/linux/cgroups.hpp 282b8e783e8c70742e4388a19003382bc57db8d5 
  src/linux/cgroups.cpp 86b0306eee804732a55041d2d82c14db35ec5095 
  src/linux/systemd.cpp 2444ff4e432e61071c62e8e1ee94d62ac6b96c88 
  src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
1444c0543540cf61e253579e9e49d8687bd4ed3a 
  src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpu.cpp 
321671f18b72edec1f8ed00f0069b539a698c4af 
  src/slave/containerizer/mesos/isolators/gpu/isolator.cpp 
d84f78c99eb118eba1a5bc3e74a2cf57556a325a 
  src/slave/containerizer/mesos/linux_launcher.cpp 
e51352af12743cc35f09b9fbb7d770f0108f908c 
  src/slave/main.cpp 489e87522588be259d382f588b66907ba29f1788 
  src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
b80e40b6f4b18e076ebb3c1668f475286c337127 


Diff: https://reviews.apache.org/r/68426/diff/1/


Testing (updated)
---

sudo make check

I also used the benchmark I developed in https://reviews.apache.org/r/68266/ to 
see the speedup

Before the patch
Took 56.xxx secs to launch 1000 containers
Took 55.xxx secs to destroy 1000 containers

After the patch
Took 37.xxx secs to launch 1000 containers
Took 26.xxx secs to destroy 1000 containers


Thanks,

Jie Yu



Re: Review Request 68426: Refactored some cgroups helpers to do verify from callers.

2018-08-19 Thread Jie Yu

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

(Updated Aug. 20, 2018, 4:36 a.m.)


Review request for mesos, Benjamin Mahler, Gilbert Song, and James Peach.


Bugs: MESOS-8418 and MESOS-9081
https://issues.apache.org/jira/browse/MESOS-8418
https://issues.apache.org/jira/browse/MESOS-9081


Repository: mesos


Description
---

Prior to this patch, some of the cgroups helpers call `cgroups::verify`
to make sure the hierarchy and cgroup are valid. This causes some
performance issues for the agent because `cgroups::verify` will read the
entire mount table, and mount table read is expensive if the mount table
is large. See more details in MESOS-8418.

In most of the cases, the verification has already been done when those
cgroups helpers are called. Therefore, it is better to let the caller do
the verification and make those cgroups helpers pure helpers.

Also, the verification is subject to race anyway. The cgroups global
state might change (e.g., operators modify the cgroups) after the
verification is done, making the verification itself not that super
useful.

This patch refactored the cgroups helpers and moved the verifications to
the callers.


Diffs
-

  src/linux/cgroups.hpp 282b8e783e8c70742e4388a19003382bc57db8d5 
  src/linux/cgroups.cpp 86b0306eee804732a55041d2d82c14db35ec5095 
  src/linux/systemd.cpp 2444ff4e432e61071c62e8e1ee94d62ac6b96c88 
  src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
1444c0543540cf61e253579e9e49d8687bd4ed3a 
  src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpu.cpp 
321671f18b72edec1f8ed00f0069b539a698c4af 
  src/slave/containerizer/mesos/isolators/gpu/isolator.cpp 
d84f78c99eb118eba1a5bc3e74a2cf57556a325a 
  src/slave/containerizer/mesos/linux_launcher.cpp 
e51352af12743cc35f09b9fbb7d770f0108f908c 
  src/slave/main.cpp 489e87522588be259d382f588b66907ba29f1788 
  src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
b80e40b6f4b18e076ebb3c1668f475286c337127 


Diff: https://reviews.apache.org/r/68426/diff/1/


Testing
---

sudo make check


Thanks,

Jie Yu



Review Request 68426: Refactored some cgroups helpers to do verify from callers.

2018-08-19 Thread Jie Yu

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

Review request for mesos, Benjamin Mahler, Gilbert Song, and James Peach.


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


Repository: mesos


Description
---

Prior to this patch, some of the cgroups helpers call `cgroups::verify`
to make sure the hierarchy and cgroup are valid. This causes some
performance issues for the agent because `cgroups::verify` will read the
entire mount table, and mount table read is expensive if the mount table
is large. See more details in MESOS-8418.

In most of the cases, the verification has already been done when those
cgroups helpers are called. Therefore, it is better to let the caller do
the verification and make those cgroups helpers pure helpers.

Also, the verification is subject to race anyway. The cgroups global
state might change (e.g., operators modify the cgroups) after the
verification is done, making the verification itself not that super
useful.

This patch refactored the cgroups helpers and moved the verifications to
the callers.


Diffs
-

  src/linux/cgroups.hpp 282b8e783e8c70742e4388a19003382bc57db8d5 
  src/linux/cgroups.cpp 86b0306eee804732a55041d2d82c14db35ec5095 
  src/linux/systemd.cpp 2444ff4e432e61071c62e8e1ee94d62ac6b96c88 
  src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
1444c0543540cf61e253579e9e49d8687bd4ed3a 
  src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpu.cpp 
321671f18b72edec1f8ed00f0069b539a698c4af 
  src/slave/containerizer/mesos/isolators/gpu/isolator.cpp 
d84f78c99eb118eba1a5bc3e74a2cf57556a325a 
  src/slave/containerizer/mesos/linux_launcher.cpp 
e51352af12743cc35f09b9fbb7d770f0108f908c 
  src/slave/main.cpp 489e87522588be259d382f588b66907ba29f1788 
  src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
b80e40b6f4b18e076ebb3c1668f475286c337127 


Diff: https://reviews.apache.org/r/68426/diff/1/


Testing
---

sudo make check


Thanks,

Jie Yu



Re: Review Request 67931: Windows: Added support for Docker Image Manifest Version 2 Schema 2.

2018-08-17 Thread Jie Yu


> On Aug. 16, 2018, 10:34 p.m., Jie Yu wrote:
> > src/uri/fetchers/docker.cpp
> > Lines 682 (patched)
> > <https://reviews.apache.org/r/67931/diff/6/?file=2071252#file2071252line683>
> >
> > I'd rather treating fetching v2 schema 2 manifest as an impl. details 
> > of supporting fetching foreign urls on windows. Because I don't want the 
> > reader to be confused that we support v2 schema 2 properly on windows.
> > 
> > So the concrete suggestion here is to always fetch v2 schema 1 manifest 
> > first here (no change to this line) as you need the information in either 
> > case. And then, on windows, additionally fetch the v2 scheme 2 manifest (in 
> > `__fetch`).
> 
> Jie Yu wrote:
> Correction, you should fetch v2_2 manifest in fetchBlob, not in `__fetch`
> 
> Liangyu Zhao wrote:
> The manifest uri cannot be derived from blob uri unfortunately. The query 
> part of the manifest uri or digest/tag of the image reference is lost when 
> generating the blob uri, because blob is fetched based on the digest of the 
> blob. This makes `fetchblob` unable to fetch v2_2 manifest.
> 
> A solution to this, I suggest, is to fetch v2_2 manifest along with v2_1 
> manifest, and save both on disk during fetching manifest, so `fetchBlob` can 
> read the v2_2 manifest from disk without trying to derive the manifest uri.

That sounds ok, this will be my suggestion to make that logic Windows only.

In `__fetch`, right after the schema1 manifest is saved, do the following:
```
Future DockerFetcherPluginProcess::__fetch(...)
{
  Try schema1Manifest =
saveSchema1Manifest(directory, response);

  if (schema1Manifest.isError()) {
return Failure(...);
  }
  
#ifdef WINDOWS
  http::Headers manifestHeaders = {
{"Accept", "application/vnd.docker.distribution.manifest.v2+json"}
  };
  
  return curl(manifestUri, manifestHeaders + authHeaders, stallTimeout)
.then(defer(self(),
::saveSchema2Manifest,
directory,
lambda::_1))
.then(defer(self(),
::fetchBlobs,
schema1Manifest.get(),
lambda::_1)); // `lambda::_1` is schema2 manifest protobuf.
#else
  return fetchBlobs(schema1Manifest.get(), None());
#endif // WINDOWS
}

Future DockerFetcherPluginProcess::fetchBlobs(
const spec::v2::ImageManifest& schema1Manifest,
const Option saveSchema1Manifest(...) {}
Try saveSchema2Manifest(...) {}
```


- Jie


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


On Aug. 6, 2018, 6:45 p.m., Liangyu Zhao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67931/
> -----------
> 
> (Updated Aug. 6, 2018, 6:45 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, Gilbert Song, 
> Jie Yu, Joseph Wu, and Qian Zhang.
> 
> 
> Bugs: MESOS-9159
> https://issues.apache.org/jira/browse/MESOS-9159
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for parsing schema 2 and fetching blob with multiple
> URLs as specified in schema 2. Also updated `curl` version to 7.61.0
> because of a bug encountered in version 7.57.0.
> 
> 
> Diffs
> -
> 
>   3rdparty/CMakeLists.txt 629b1968775da4d128b8d36c77d75efa303b0b7c 
>   3rdparty/cmake/Versions.cmake c65d7b3d2d48ce9d0b4777aeb12fdf5b605803ac 
>   include/mesos/docker/spec.hpp 2879414dc42ffe633ac74b51e1bb116698c41162 
>   include/mesos/docker/v2_2.hpp PRE-CREATION 
>   include/mesos/docker/v2_2.proto PRE-CREATION 
>   include/mesos/uri/fetcher.hpp 760d6b33234d8efdc533c0c6f05e83a5c7d1f56b 
>   src/CMakeLists.txt a80b011bbd5c418ae66eb8dd7697d070168462bb 
>   src/Makefile.am cf0cf22b3f582d3d4427f13288af3845aef45263 
>   src/docker/spec.cpp 96fbf1f9cf1c2c4b2383607a97990f3a9156e6d9 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 98129d006cda9b65804b518619b6addc8990410a 
>   src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp 
> a5683e3fe15dd35596122fcc0c580ae9d3adf7f2 
>   src/uri/fetcher.hpp fbf64c6767dea3aa0798f68db8322ce47cd8ad36 
>   src/uri/fetcher.cpp 489c7ce0198ee6803dcc8eb9710b292fa743a0e8 
>   src/uri/fetchers/docker.hpp 7a6193dd75dc10846f52cb039f6a162cc8805c16 
>   src/uri/fetchers/docker.cpp 55ca118660872a933a2dc186723bec6a39ee80f7 
> 
> 
> Diff: https://reviews.apache.org/r/67931/diff/6/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Liangyu Zhao
> 
>



Re: Review Request 68417: Ignored pre-existing CSI volumes known to SLRP.

2018-08-17 Thread Jie Yu

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


Ship it!




Ship It!

- Jie Yu


On Aug. 17, 2018, 9:12 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68417/
> ---
> 
> (Updated Aug. 17, 2018, 9:12 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Bugs: MESOS-9166
> https://issues.apache.org/jira/browse/MESOS-9166
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If a pre-existing volume is known to SLRP, i.e., the SLRP keeps a CSI
> volume state checkpoint for the volume, but there is no corresponding
> resource checkpoint, then this means the volume was created by a
> previous SLRP instance, but the SLRP lost the state checkpoint
> (typically happens when the agent ID changes). In such a case, we should
> not report this volume as an unmanaged pre-existing volume. For now we
> simply ingore such a volume.
> 
> See MESOS-9167 for future work.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> fc48072aac531bac3cbffc3ba089b8dfa2a2f200 
> 
> 
> Diff: https://reviews.apache.org/r/68417/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>



Re: Review Request 63270: Updated `os::pipe()` to always return O_CLOEXEC descriptors.

2018-08-17 Thread Jie Yu

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


Ship it!




Ship It!

- Jie Yu


On Aug. 17, 2018, 5:53 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63270/
> ---
> 
> (Updated Aug. 17, 2018, 5:53 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Benjamin 
> Mahler, Greg Mann, and Jie Yu.
> 
> 
> Bugs: MESOS-8128
> https://issues.apache.org/jira/browse/MESOS-8128
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated `os::pipe()` to always return O_CLOEXEC descriptors,
> atomically if we are on Linux or FreeBSD and the `pipe2(2)`
> system call is available.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/posix/pipe.hpp 
> ac76224ff8fa1cc535c78731580e77a27967136a 
> 
> 
> Diff: https://reviews.apache.org/r/63270/diff/4/
> 
> 
> Testing
> ---
> 
> make check (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>



Re: Review Request 63270: Updated `os::pipe()` to always return O_CLOEXEC descriptors.

2018-08-17 Thread Jie Yu


> On Aug. 17, 2018, 8:02 p.m., Benjamin Mahler wrote:
> > Hm.. it feels wrong to take the control away from the caller, since this 
> > was intended as just a wrapper, like open.hpp:
> > 
> > https://github.com/apache/mesos/blob/1.6.1/3rdparty/stout/include/stout/os/posix/open.hpp#L29-L43
> > 
> > Can we follow the approach there? Do we actually support systems without 
> > pipe2?
> 
> James Peach wrote:
> I didn't add the option because there's not really a safe way to use 
> this, except to start with O_CLOEXEC and then clear it after the fork. I 
> think that it's preferable to be safe by default and for the callers that 
> need to be special to add the extra code.
> 
> > Do we actually support systems without pipe2?
> 
> macOS, for example.

I am actually fine with this change. I think it's safe to use O_CLOEXEC by 
default, and explicilty mark some fd as inheritable if needed.


- Jie


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


On Aug. 17, 2018, 5:53 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63270/
> ---
> 
> (Updated Aug. 17, 2018, 5:53 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Benjamin 
> Mahler, Greg Mann, and Jie Yu.
> 
> 
> Bugs: MESOS-8128
> https://issues.apache.org/jira/browse/MESOS-8128
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated `os::pipe()` to always return O_CLOEXEC descriptors,
> atomically if we are on Linux or FreeBSD and the `pipe2(2)`
> system call is available.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/posix/pipe.hpp 
> ac76224ff8fa1cc535c78731580e77a27967136a 
> 
> 
> Diff: https://reviews.apache.org/r/63270/diff/4/
> 
> 
> Testing
> ---
> 
> make check (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>



Re: Review Request 63280: Made the containerizer launch be explicit about O_CLOEXEC.

2018-08-17 Thread Jie Yu

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


Ship it!




Ship It!

- Jie Yu


On Aug. 17, 2018, 6:14 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63280/
> ---
> 
> (Updated Aug. 17, 2018, 6:14 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Benjamin 
> Mahler, Gilbert Song, Greg Mann, and Jie Yu.
> 
> 
> Bugs: MESOS-8128
> https://issues.apache.org/jira/browse/MESOS-8128
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since the containerizer launch depends on the inherited pipe to signal
> the forked child, be explicit about manipulating O_CLOEXEC on the pipe
> file descriptors. Make sure to close the pipe on the error paths.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 98129d006cda9b65804b518619b6addc8990410a 
>   src/slave/containerizer/mesos/launcher.cpp 
> ed41a14892bdfa74d28bd0541ca188b86f8f96f8 
>   src/slave/containerizer/mesos/linux_launcher.cpp 
> d1f8d3f56a9ef3aa98012c9395e74732e49c264d 
> 
> 
> Diff: https://reviews.apache.org/r/63280/diff/4/
> 
> 
> Testing
> ---
> 
> sudo make check (Fedora 28)
> 
> 
> Thanks,
> 
> James Peach
> 
>



Re: Review Request 68413: Only update profiles when getting `200 OK` in `UriDiskProfileAdaptor`.

2018-08-17 Thread Jie Yu

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


Ship it!




Ship It!

- Jie Yu


On Aug. 17, 2018, 5:22 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68413/
> ---
> 
> (Updated Aug. 17, 2018, 5:22 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Joseph Wu.
> 
> 
> Bugs: MESOS-9163
> https://issues.apache.org/jira/browse/MESOS-9163
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Only updated profiles when getting `200 OK` in `UriDiskProfileAdaptor`.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/uri_disk_profile_adaptor.cpp 
> f686a43199d25d68227c7f24258e390455130dde 
> 
> 
> Diff: https://reviews.apache.org/r/68413/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>



Re: Review Request 68396: Updated the ::pipe() system calls to pipe2 in posix subprocess.

2018-08-16 Thread Jie Yu

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


Ship it!




Ship It!

- Jie Yu


On Aug. 16, 2018, 11:18 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68396/
> ---
> 
> (Updated Aug. 16, 2018, 11:18 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Benjamin 
> Mahler, Jie Yu, James Peach, and Qian Zhang.
> 
> 
> Bugs: MESOS-9151
> https://issues.apache.org/jira/browse/MESOS-9151
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated the ::pipe() system calls to pipe2 in posix subprocess.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/posix/subprocess.cpp 
> 01e3272fccda6ff66e1629fb10d22d8c4967b22a 
> 
> 
> Diff: https://reviews.apache.org/r/68396/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>



Re: Review Request 67931: Windows: Added support for Docker Image Manifest Version 2 Schema 2.

2018-08-16 Thread Jie Yu


> On Aug. 16, 2018, 10:34 p.m., Jie Yu wrote:
> > src/uri/fetchers/docker.cpp
> > Lines 682 (patched)
> > <https://reviews.apache.org/r/67931/diff/6/?file=2071252#file2071252line683>
> >
> > I'd rather treating fetching v2 schema 2 manifest as an impl. details 
> > of supporting fetching foreign urls on windows. Because I don't want the 
> > reader to be confused that we support v2 schema 2 properly on windows.
> > 
> > So the concrete suggestion here is to always fetch v2 schema 1 manifest 
> > first here (no change to this line) as you need the information in either 
> > case. And then, on windows, additionally fetch the v2 scheme 2 manifest (in 
> > `__fetch`).

Correction, you should fetch v2_2 manifest in fetchBlob, not in `__fetch`


- Jie


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


On Aug. 6, 2018, 6:45 p.m., Liangyu Zhao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67931/
> ---
> 
> (Updated Aug. 6, 2018, 6:45 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, Gilbert Song, 
> Jie Yu, Joseph Wu, and Qian Zhang.
> 
> 
> Bugs: MESOS-9159
> https://issues.apache.org/jira/browse/MESOS-9159
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for parsing schema 2 and fetching blob with multiple
> URLs as specified in schema 2. Also updated `curl` version to 7.61.0
> because of a bug encountered in version 7.57.0.
> 
> 
> Diffs
> -
> 
>   3rdparty/CMakeLists.txt 629b1968775da4d128b8d36c77d75efa303b0b7c 
>   3rdparty/cmake/Versions.cmake c65d7b3d2d48ce9d0b4777aeb12fdf5b605803ac 
>   include/mesos/docker/spec.hpp 2879414dc42ffe633ac74b51e1bb116698c41162 
>   include/mesos/docker/v2_2.hpp PRE-CREATION 
>   include/mesos/docker/v2_2.proto PRE-CREATION 
>   include/mesos/uri/fetcher.hpp 760d6b33234d8efdc533c0c6f05e83a5c7d1f56b 
>   src/CMakeLists.txt a80b011bbd5c418ae66eb8dd7697d070168462bb 
>   src/Makefile.am cf0cf22b3f582d3d4427f13288af3845aef45263 
>   src/docker/spec.cpp 96fbf1f9cf1c2c4b2383607a97990f3a9156e6d9 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 98129d006cda9b65804b518619b6addc8990410a 
>   src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp 
> a5683e3fe15dd35596122fcc0c580ae9d3adf7f2 
>   src/uri/fetcher.hpp fbf64c6767dea3aa0798f68db8322ce47cd8ad36 
>   src/uri/fetcher.cpp 489c7ce0198ee6803dcc8eb9710b292fa743a0e8 
>   src/uri/fetchers/docker.hpp 7a6193dd75dc10846f52cb039f6a162cc8805c16 
>   src/uri/fetchers/docker.cpp 55ca118660872a933a2dc186723bec6a39ee80f7 
> 
> 
> Diff: https://reviews.apache.org/r/67931/diff/6/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Liangyu Zhao
> 
>



Re: Review Request 67931: Windows: Added support for Docker Image Manifest Version 2 Schema 2.

2018-08-16 Thread Jie Yu

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




src/docker/spec.cpp
Lines 347 (patched)
<https://reviews.apache.org/r/67931/#comment290818>

Please add unit tests like Ilya did in 
https://reviews.apache.org/r/53849/

In order to make patch more reviewable for reviewers, please split big 
patche like this into smaller logical pieces. For instance, the first patch can 
just be adding v2_2 parsing and validation support.

See section "Create your patch", bullet #4 in the following link 
http://mesos.apache.org/documentation/latest/advanced-contribution/



src/docker/spec.cpp
Lines 390 (patched)
<https://reviews.apache.org/r/67931/#comment290829>

nits: 2 lines apart. Please stick to our style guide



src/docker/spec.cpp
Lines 407 (patched)
<https://reviews.apache.org/r/67931/#comment290830>

Ditto.



src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp
Lines 472-503 (patched)
<https://reviews.apache.org/r/67931/#comment290843>

See my comment below. I think changes to registry puller can be avoided.



src/uri/fetchers/docker.cpp
Lines 682 (patched)
<https://reviews.apache.org/r/67931/#comment290837>

I'd rather treating fetching v2 schema 2 manifest as an impl. details of 
supporting fetching foreign urls on windows. Because I don't want the reader to 
be confused that we support v2 schema 2 properly on windows.

So the concrete suggestion here is to always fetch v2 schema 1 manifest 
first here (no change to this line) as you need the information in either case. 
And then, on windows, additionally fetch the v2 scheme 2 manifest (in 
`__fetch`).



src/uri/fetchers/docker.cpp
Line 681 (original), 779 (patched)
<https://reviews.apache.org/r/67931/#comment290838>

With my suggestion above, you probably does not need the Variant anymore. 
Here, you'll still parse v2_1 manifest. Also, see my comment below to see how 
to make less intrusive change.



src/uri/fetchers/docker.cpp
Lines 704-720 (original), 853-880 (patched)
<https://reviews.apache.org/r/67931/#comment290842>

This gets quite ugly TBH. I'd suggest add some windows specific code in 
`__fetchBlob`. Basically, if you got a 404 on windows, do the extra steps to 
fetch the v2_2 manifest, and try to download the uri.

In this way, you don't need to change the core logic.


- Jie Yu


On Aug. 6, 2018, 6:45 p.m., Liangyu Zhao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67931/
> ---
> 
> (Updated Aug. 6, 2018, 6:45 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, Gilbert Song, 
> Jie Yu, Joseph Wu, and Qian Zhang.
> 
> 
> Bugs: MESOS-9159
> https://issues.apache.org/jira/browse/MESOS-9159
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for parsing schema 2 and fetching blob with multiple
> URLs as specified in schema 2. Also updated `curl` version to 7.61.0
> because of a bug encountered in version 7.57.0.
> 
> 
> Diffs
> -
> 
>   3rdparty/CMakeLists.txt 629b1968775da4d128b8d36c77d75efa303b0b7c 
>   3rdparty/cmake/Versions.cmake c65d7b3d2d48ce9d0b4777aeb12fdf5b605803ac 
>   include/mesos/docker/spec.hpp 2879414dc42ffe633ac74b51e1bb116698c41162 
>   include/mesos/docker/v2_2.hpp PRE-CREATION 
>   include/mesos/docker/v2_2.proto PRE-CREATION 
>   include/mesos/uri/fetcher.hpp 760d6b33234d8efdc533c0c6f05e83a5c7d1f56b 
>   src/CMakeLists.txt a80b011bbd5c418ae66eb8dd7697d070168462bb 
>   src/Makefile.am cf0cf22b3f582d3d4427f13288af3845aef45263 
>   src/docker/spec.cpp 96fbf1f9cf1c2c4b2383607a97990f3a9156e6d9 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 98129d006cda9b65804b518619b6addc8990410a 
>   src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp 
> a5683e3fe15dd35596122fcc0c580ae9d3adf7f2 
>   src/uri/fetcher.hpp fbf64c6767dea3aa0798f68db8322ce47cd8ad36 
>   src/uri/fetcher.cpp 489c7ce0198ee6803dcc8eb9710b292fa743a0e8 
>   src/uri/fetchers/docker.hpp 7a6193dd75dc10846f52cb039f6a162cc8805c16 
>   src/uri/fetchers/docker.cpp 55ca118660872a933a2dc186723bec6a39ee80f7 
> 
> 
> Diff: https://reviews.apache.org/r/67931/diff/6/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Liangyu Zhao
> 
>



Re: Review Request 67931: Windows: Added support for Docker Image Manifest Version 2 Schema 2.

2018-08-15 Thread Jie Yu


> On Aug. 15, 2018, 8:50 p.m., Jie Yu wrote:
> > what’s the filesystem layout for the cache when v2 schema 2 is used?
> > The layout for schema 1 is here:
> > https://github.com/apache/mesos/blob/master/src/slave/containerizer/mesos/provisioner/docker/paths.hpp#L31-L46
> > 
> > as far as I know, layer_id does not apply to schema 2. What do you use for 
> > the key?
> 
> Jie Yu wrote:
> I think the layout we're currently using in the cache is actually v1. The 
> reason for that is legacy (because we want to also support local puller, and 
> at that time, the layout is v1).
> 
> And we wanted to switch to use v2 instead for a long time. But I don't 
> think we should mix v1 and v2 in the same `layers` dir.
> 
> I'd rather see v2 being a totally separate cache that's content 
> addressible.
> 
> I think @gilbert has some thoughts on the v2 (or unified artifacts store) 
> cache layout, @gilbert, can you share it here?
> 
> Liangyu Zhao wrote:
> The filesystem layout is in fact not changed. What we are doing right now 
> is pulling both schema 1 and schema 2 manifests. All the previous schema 1 
> logic including filesystem layout is unchanged, and schema 2 manifest is only 
> used to support features not supported by schema 1, for example, pulling 
> layers with urls in this patch. According to the backward compatibility of 
> [schema 
> 2](https://docs.docker.com/registry/spec/manifest-v2-2/#backward-compatibility),
>  schema 1 is always present, but schema 2 may not be present. Therefore, it 
> is reasonable to keep the schema 1 logic, and use schema 2 only for 
> additional support.

Can you describe in the description that why windows need the feature of 
"pulling layers with urls"? It's not clear to me.

ok, so basically, you need information from both v2.1 and v2.2 manifest so that 
you can re-use the code we have for v2.1. This is indeed very differnt than 
what Ilya does in https://reviews.apache.org/r/53850/

I would suggest we have a sync with all stakeholders see if this is the long 
term solution we want to support.


- Jie


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


On Aug. 6, 2018, 6:45 p.m., Liangyu Zhao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67931/
> -----------
> 
> (Updated Aug. 6, 2018, 6:45 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, Gilbert Song, 
> Jie Yu, Joseph Wu, and Qian Zhang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for parsing schema 2 and fetching blob with multiple
> URLs as specified in schema 2. Also updated `curl` version to 7.61.0
> because of a bug encountered in version 7.57.0.
> 
> 
> Diffs
> -
> 
>   3rdparty/CMakeLists.txt 629b1968775da4d128b8d36c77d75efa303b0b7c 
>   3rdparty/cmake/Versions.cmake c65d7b3d2d48ce9d0b4777aeb12fdf5b605803ac 
>   include/mesos/docker/spec.hpp 2879414dc42ffe633ac74b51e1bb116698c41162 
>   include/mesos/docker/v2_2.hpp PRE-CREATION 
>   include/mesos/docker/v2_2.proto PRE-CREATION 
>   include/mesos/uri/fetcher.hpp 760d6b33234d8efdc533c0c6f05e83a5c7d1f56b 
>   src/CMakeLists.txt a80b011bbd5c418ae66eb8dd7697d070168462bb 
>   src/Makefile.am cf0cf22b3f582d3d4427f13288af3845aef45263 
>   src/docker/spec.cpp 96fbf1f9cf1c2c4b2383607a97990f3a9156e6d9 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 98129d006cda9b65804b518619b6addc8990410a 
>   src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp 
> a5683e3fe15dd35596122fcc0c580ae9d3adf7f2 
>   src/uri/fetcher.hpp fbf64c6767dea3aa0798f68db8322ce47cd8ad36 
>   src/uri/fetcher.cpp 489c7ce0198ee6803dcc8eb9710b292fa743a0e8 
>   src/uri/fetchers/docker.hpp 7a6193dd75dc10846f52cb039f6a162cc8805c16 
>   src/uri/fetchers/docker.cpp 55ca118660872a933a2dc186723bec6a39ee80f7 
> 
> 
> Diff: https://reviews.apache.org/r/67931/diff/6/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Liangyu Zhao
> 
>



Re: Review Request 67931: Windows: Added support for Docker Image Manifest Version 2 Schema 2.

2018-08-15 Thread Jie Yu


> On Aug. 15, 2018, 8:50 p.m., Jie Yu wrote:
> > what’s the filesystem layout for the cache when v2 schema 2 is used?
> > The layout for schema 1 is here:
> > https://github.com/apache/mesos/blob/master/src/slave/containerizer/mesos/provisioner/docker/paths.hpp#L31-L46
> > 
> > as far as I know, layer_id does not apply to schema 2. What do you use for 
> > the key?

I think the layout we're currently using in the cache is actually v1. The 
reason for that is legacy (because we want to also support local puller, and at 
that time, the layout is v1).

And we wanted to switch to use v2 instead for a long time. But I don't think we 
should mix v1 and v2 in the same `layers` dir.

I'd rather see v2 being a totally separate cache that's content addressible.

I think @gilbert has some thoughts on the v2 (or unified artifacts store) cache 
layout, @gilbert, can you share it here?


- Jie


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


On Aug. 6, 2018, 6:45 p.m., Liangyu Zhao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67931/
> ---
> 
> (Updated Aug. 6, 2018, 6:45 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, Gilbert Song, 
> Jie Yu, Joseph Wu, and Qian Zhang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for parsing schema 2 and fetching blob with multiple
> URLs as specified in schema 2. Also updated `curl` version to 7.61.0
> because of a bug encountered in version 7.57.0.
> 
> 
> Diffs
> -
> 
>   3rdparty/CMakeLists.txt 629b1968775da4d128b8d36c77d75efa303b0b7c 
>   3rdparty/cmake/Versions.cmake c65d7b3d2d48ce9d0b4777aeb12fdf5b605803ac 
>   include/mesos/docker/spec.hpp 2879414dc42ffe633ac74b51e1bb116698c41162 
>   include/mesos/docker/v2_2.hpp PRE-CREATION 
>   include/mesos/docker/v2_2.proto PRE-CREATION 
>   include/mesos/uri/fetcher.hpp 760d6b33234d8efdc533c0c6f05e83a5c7d1f56b 
>   src/CMakeLists.txt a80b011bbd5c418ae66eb8dd7697d070168462bb 
>   src/Makefile.am cf0cf22b3f582d3d4427f13288af3845aef45263 
>   src/docker/spec.cpp 96fbf1f9cf1c2c4b2383607a97990f3a9156e6d9 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 98129d006cda9b65804b518619b6addc8990410a 
>   src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp 
> a5683e3fe15dd35596122fcc0c580ae9d3adf7f2 
>   src/uri/fetcher.hpp fbf64c6767dea3aa0798f68db8322ce47cd8ad36 
>   src/uri/fetcher.cpp 489c7ce0198ee6803dcc8eb9710b292fa743a0e8 
>   src/uri/fetchers/docker.hpp 7a6193dd75dc10846f52cb039f6a162cc8805c16 
>   src/uri/fetchers/docker.cpp 55ca118660872a933a2dc186723bec6a39ee80f7 
> 
> 
> Diff: https://reviews.apache.org/r/67931/diff/6/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Liangyu Zhao
> 
>



Re: Review Request 67931: Windows: Added support for Docker Image Manifest Version 2 Schema 2.

2018-08-15 Thread Jie Yu

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



what’s the filesystem layout for the cache when v2 schema 2 is used?
The layout for schema 1 is here:
https://github.com/apache/mesos/blob/master/src/slave/containerizer/mesos/provisioner/docker/paths.hpp#L31-L46

as far as I know, layer_id does not apply to schema 2. What do you use for the 
key?

- Jie Yu


On Aug. 6, 2018, 6:45 p.m., Liangyu Zhao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67931/
> ---
> 
> (Updated Aug. 6, 2018, 6:45 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, Gilbert Song, 
> Jie Yu, Joseph Wu, and Qian Zhang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for parsing schema 2 and fetching blob with multiple
> URLs as specified in schema 2. Also updated `curl` version to 7.61.0
> because of a bug encountered in version 7.57.0.
> 
> 
> Diffs
> -
> 
>   3rdparty/CMakeLists.txt 629b1968775da4d128b8d36c77d75efa303b0b7c 
>   3rdparty/cmake/Versions.cmake c65d7b3d2d48ce9d0b4777aeb12fdf5b605803ac 
>   include/mesos/docker/spec.hpp 2879414dc42ffe633ac74b51e1bb116698c41162 
>   include/mesos/docker/v2_2.hpp PRE-CREATION 
>   include/mesos/docker/v2_2.proto PRE-CREATION 
>   include/mesos/uri/fetcher.hpp 760d6b33234d8efdc533c0c6f05e83a5c7d1f56b 
>   src/CMakeLists.txt a80b011bbd5c418ae66eb8dd7697d070168462bb 
>   src/Makefile.am cf0cf22b3f582d3d4427f13288af3845aef45263 
>   src/docker/spec.cpp 96fbf1f9cf1c2c4b2383607a97990f3a9156e6d9 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 98129d006cda9b65804b518619b6addc8990410a 
>   src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp 
> a5683e3fe15dd35596122fcc0c580ae9d3adf7f2 
>   src/uri/fetcher.hpp fbf64c6767dea3aa0798f68db8322ce47cd8ad36 
>   src/uri/fetcher.cpp 489c7ce0198ee6803dcc8eb9710b292fa743a0e8 
>   src/uri/fetchers/docker.hpp 7a6193dd75dc10846f52cb039f6a162cc8805c16 
>   src/uri/fetchers/docker.cpp 55ca118660872a933a2dc186723bec6a39ee80f7 
> 
> 
> Diff: https://reviews.apache.org/r/67931/diff/6/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Liangyu Zhao
> 
>



Re: Review Request 68355: Added a CNI test for networking statistics.

2018-08-15 Thread Jie Yu

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




src/tests/containerizer/cni_isolator_tests.cpp
Lines 220 (patched)
<https://reviews.apache.org/r/68355/#comment290694>

Errors must be indicated by a non-zero return code


- Jie Yu


On Aug. 15, 2018, 6:02 a.m., Sergey Urbanovich wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68355/
> ---
> 
> (Updated Aug. 15, 2018, 6:02 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Qian Zhang.
> 
> 
> Bugs: MESOS-5647
> https://issues.apache.org/jira/browse/MESOS-5647
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is a veth CNI plugin that is written in bash. It creates a veth
> virtual network pair, one end of the pair will be moved to container
> network namespace.
> 
> The veth CNI plugin uses 203.0.113.0/24 subnet, it is reserved for
> documentation and examples [rfc5737]. The plugin can allocate up to
> 128 veth pairs.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/cni_isolator_tests.cpp 
> cb22e73b4215b5b0a49ac610e5f657b73d38963b 
> 
> 
> Diff: https://reviews.apache.org/r/68355/diff/2/
> 
> 
> Testing
> ---
> 
> bin/mesos-tests.sh --verbose 
> --gtest_filter="CniIsolatorTest.ROOT_VerifyResourceStatistics" 
> --gtest_break_on_failure --gtest_repeat=100
> 
> 
> Thanks,
> 
> Sergey Urbanovich
> 
>



Re: Review Request 68355: Added a CNI test for networking statistics.

2018-08-14 Thread Jie Yu

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




src/tests/containerizer/cni_isolator_tests.cpp
Lines 203 (patched)
<https://reviews.apache.org/r/68355/#comment290645>

nits: I'd suggest we don't to link break here. Lint won't check c++11 
string literals.



src/tests/containerizer/cni_isolator_tests.cpp
Lines 207 (patched)
<https://reviews.apache.org/r/68355/#comment290646>

nits: Ditto. Let's use one line for those command



src/tests/containerizer/cni_isolator_tests.cpp
Lines 216-219 (patched)
<https://reviews.apache.org/r/68355/#comment290647>

Why you need this?



src/tests/containerizer/cni_isolator_tests.cpp
Lines 229 (patched)
<https://reviews.apache.org/r/68355/#comment290644>

I'd suggest we use `trap` to do the cleanup in case any command throw an 
error:
```
...
ln -sf $CNI_NETNS /var/run/netns/$NETNS

function cleanup() {
  rm -f /var/run/netns/$NETNS
}

trap cleanup EXIT
```



src/tests/containerizer/cni_isolator_tests.cpp
Lines 231-234 (patched)
<https://reviews.apache.org/r/68355/#comment290649>

I think CNI plugin's error message should be in stdout according to spec (a 
json)



src/tests/containerizer/cni_isolator_tests.cpp
Lines 236-240 (patched)
<https://reviews.apache.org/r/68355/#comment290648>

CNI DEL shouldn't output this?



src/tests/containerizer/cni_isolator_tests.cpp
Lines 2418-2421 (patched)
<https://reviews.apache.org/r/68355/#comment290641>

No need for this.



src/tests/containerizer/cni_isolator_tests.cpp
Lines 2428 (patched)
<https://reviews.apache.org/r/68355/#comment290642>

No need for this.


- Jie Yu


On Aug. 15, 2018, 1:47 a.m., Sergey Urbanovich wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68355/
> ---
> 
> (Updated Aug. 15, 2018, 1:47 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Qian Zhang.
> 
> 
> Bugs: MESOS-5647
> https://issues.apache.org/jira/browse/MESOS-5647
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is a veth CNI plugin that is written in bash. It creates a veth
> virtual network pair, one end of the pair will be moved to container
> network namespace.
> 
> The veth CNI plugin uses 203.0.113.0/24 subnet, it is reserved for
> documentation and examples [rfc5737]. The plugin can allocate up to
> 128 veth pairs.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/cni_isolator_tests.cpp 
> cb22e73b4215b5b0a49ac610e5f657b73d38963b 
> 
> 
> Diff: https://reviews.apache.org/r/68355/diff/1/
> 
> 
> Testing
> ---
> 
> bin/mesos-tests.sh --verbose 
> --gtest_filter="CniIsolatorTest.ROOT_VerifyResourceStatistics" 
> --gtest_break_on_failure --gtest_repeat=100
> 
> 
> Thanks,
> 
> Sergey Urbanovich
> 
>



Re: Review Request 68054: Added networking statistics to cni isolator.

2018-08-14 Thread Jie Yu

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


Ship it!




Ship It!

- Jie Yu


On Aug. 15, 2018, 1:46 a.m., Sergey Urbanovich wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68054/
> ---
> 
> (Updated Aug. 15, 2018, 1:46 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Qian Zhang.
> 
> 
> Bugs: MESOS-5647
> https://issues.apache.org/jira/browse/MESOS-5647
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> On receiving a request for getting `usage` for a given container
> the `network/cni` isolator uses getifaddrs(3) glibc function. The
> function returns basic networking metrics for each networking
> interface in the container networking namespace. It should work
> right out of the box on all modern Linux-based systems.
> 
> To get more networking metrics please use Netlink Protocol Library.
> However, you will have to open NETLINK sockets in each networking
> namespace and manage them from the `network/cni` isolator.
> 
> JIRA: https://issues.apache.org/jira/browse/MESOS-5647
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> 31ec4ddb1049b7259b0784e5e40b002e29f6a8da 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> f46c962d8f044092aaa113fafc536c6b25bab45c 
>   src/slave/flags.hpp 88c35da5fd754abbd4bd316e1fa9efa4a70a6b8c 
>   src/slave/flags.cpp 54d9acc8693f53294bdc2a88183cac84a8dfbfd9 
> 
> 
> Diff: https://reviews.apache.org/r/68054/diff/7/
> 
> 
> Testing
> ---
> 
> https://reviews.apache.org/r/68355
> 
> 
> Thanks,
> 
> Sergey Urbanovich
> 
>



Re: Review Request 68335: Added a CNI test to verify destroy while preparing.

2018-08-14 Thread Jie Yu

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

(Updated Aug. 14, 2018, 7:05 p.m.)


Review request for mesos, Deepak Goel, Qian Zhang, and Sergey Urbanovich.


Changes
---

Addressed comments.


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


Repository: mesos


Description
---

This test is used to catch the regression described in MESOS-9142.


Diffs (updated)
-

  src/tests/containerizer/cni_isolator_tests.cpp 
419c94d601cd588c893a5324c4aef71c40ec0f61 


Diff: https://reviews.apache.org/r/68335/diff/2/

Changes: https://reviews.apache.org/r/68335/diff/1-2/


Testing
---

sudo make check


Thanks,

Jie Yu



Re: Review Request 68334: Used state::checkpoint instead in CNI isolator.

2018-08-14 Thread Jie Yu


> On Aug. 14, 2018, 7:30 a.m., Qian Zhang wrote:
> > Ship It!
> 
> Qian Zhang wrote:
> BTW, what about the other files (i.e., hosts, hostname and resolv.conf) 
> that CNI isolator persists in the container dir? Should we change from 
> `os::write()` to `state::checkpoint()` for them too?

Those are not needed because it won't be used after recovery. Those are only 
used during launching.


- Jie


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


On Aug. 13, 2018, 11:04 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68334/
> ---
> 
> (Updated Aug. 13, 2018, 11:04 p.m.)
> 
> 
> Review request for mesos, Deepak Goel, Qian Zhang, and Sergey Urbanovich.
> 
> 
> Bugs: MESOS-9142
> https://issues.apache.org/jira/browse/MESOS-9142
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is to ensure all or nothing semantics. We don't want to deal with a
> particially written file in case agent crashes.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> f46c962d8f044092aaa113fafc536c6b25bab45c 
> 
> 
> Diff: https://reviews.apache.org/r/68334/diff/3/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>



Re: Review Request 68054: Added networking statistics to cni isolator.

2018-08-13 Thread Jie Yu

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



I hit this issue on my box
```
I0813 16:42:29.687569 31007 linux_launcher.cpp:503] Launching container 
fae92a9a-561f-40a5-b49e-896be36fcf8b and cloning with namespaces CLONE_NEWNS | 
CLONE_NEWUTS | CLONE_NEWNET
I0813 16:42:29.700687 31005 containerizer.cpp:3021] Transitioning the state of 
container fae92a9a-561f-40a5-b49e-896be36fcf8b from PREPARING to ISOLATING
I0813 16:42:29.702626 30978 cni.cpp:954] Bind mounted '/proc/31055/ns/net' to 
'/run/mesos/isolators/network/cni/fae92a9a-561f-40a5-b49e-896be36fcf8b/ns' for 
container fae92a9a-561f-40a5-b49e-896be36fcf8b
I0813 16:42:29.899713 30981 cni.cpp:1385] Got assigned IPv4 address 
'203.0.113.2/24' from CNI network 'veth' for container 
fae92a9a-561f-40a5-b49e-896be36fcf8b
I0813 16:42:29.900835 30982 cni.cpp:1094] Unable to find DNS nameservers for 
container fae92a9a-561f-40a5-b49e-896be36fcf8b, using host '/etc/resolv.conf'
I0813 16:42:30.102488 30996 containerizer.cpp:3021] Transitioning the state of 
container fae92a9a-561f-40a5-b49e-896be36fcf8b from ISOLATING to FETCHING
I0813 16:42:30.105840 30992 containerizer.cpp:3021] Transitioning the state of 
container fae92a9a-561f-40a5-b49e-896be36fcf8b from FETCHING to RUNNING
Marked '/' as rslave
I0813 16:42:30.236037 31088 exec.cpp:162] Version: 1.7.0
W0813 16:42:31.245690 31126 process.cpp:1449] Failed to link to 
'10.0.49.2:44965', connect: Failed to connect to 10.0.49.2:44965: No route to 
host
I0813 16:42:31.246233 31106 exec.cpp:527] Agent exited ... shutting down
I0813 16:42:31.246580 31106 v0_v1executor.cpp:172] Implicitly connecting the 
executor to shut it down
I0813 16:42:31.247351 31110 executor.cpp:182] Received SHUTDOWN event
I0813 16:42:31.248729 31110 executor.cpp:796] Shutting down
I0813 16:42:31.250615 31126 process.cpp:926] Stopped the socket accept loop
I0813 16:42:31.308015 30980 containerizer.cpp:2860] Container 
fae92a9a-561f-40a5-b49e-896be36fcf8b has exited
I0813 16:42:31.308157 30980 containerizer.cpp:2407] Destroying container 
fae92a9a-561f-40a5-b49e-896be36fcf8b in RUNNING state
I0813 16:42:31.308187 30980 containerizer.cpp:3021] Transitioning the state of 
container fae92a9a-561f-40a5-b49e-896be36fcf8b from RUNNING to DESTROYING
I0813 16:42:31.309753 31017 linux_launcher.cpp:582] Asked to destroy container 
fae92a9a-561f-40a5-b49e-896be36fcf8b
I0813 16:42:31.311748 31017 linux_launcher.cpp:629] Destroying cgroup 
'/sys/fs/cgroup/freezer/mesos/fae92a9a-561f-40a5-b49e-896be36fcf8b'
```


src/slave/containerizer/mesos/isolators/network/cni/cni.hpp
Lines 191 (patched)
<https://reviews.apache.org/r/68054/#comment290484>

nits: kill this line.



src/tests/containerizer/cni_isolator_tests.cpp
Lines 2256 (patched)
<https://reviews.apache.org/r/68054/#comment290488>

I would like to add the cleanup code to `CniIsolatorTest::cleanup`.

It's possible that the test is ended with sigkill or ctrl+c. And `ip link 
add name vethmesostest0 type veth peer name vethmesostestns` might fail if the 
veth hasn't been cleaned up


- Jie Yu


On Aug. 10, 2018, 7:08 p.m., Sergey Urbanovich wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68054/
> ---
> 
> (Updated Aug. 10, 2018, 7:08 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Qian Zhang.
> 
> 
> Bugs: MESOS-5647
> https://issues.apache.org/jira/browse/MESOS-5647
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> On receiving a request for getting `usage` for a given container
> the `network/cni` isolator uses getifaddrs(3) glibc function. The
> function returns basic networking metrics for each networking
> interface in the container networking namespace. It should work
> right out of the box on all modern Linux-based systems.
> 
> To get more networking metrics please use Netlink Protocol Library.
> However, you will have to open NETLINK sockets in each networking
> namespace and manage them from the `network/cni` isolator.
> 
> JIRA: https://issues.apache.org/jira/browse/MESOS-5647
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> 31ec4ddb1049b7259b0784e5e40b002e29f6a8da 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> f46c962d8f044092aaa113fafc536c6b25bab45c 
>   src/slave/flags.hpp eeb9708f9ec76d83b6719541f4a012544c7c0cbe 
>   src/slave/flags.cpp 58cdc0f1100fe244e5bf1036e1ccf39478d5d478 
>   src/tests/containerizer/cni_isolator_tests.cpp 
> cae2c1950c6e5218f7fc7cebd9

Re: Review Request 68053: Call any function in a specified namespace.

2018-08-13 Thread Jie Yu

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


Ship it!




Ship It!

- Jie Yu


On July 31, 2018, 5:30 p.m., Sergey Urbanovich wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68053/
> ---
> 
> (Updated July 31, 2018, 5:30 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Chun-Hung Hsiao, Jie Yu, and Qian 
> Zhang.
> 
> 
> Bugs: MESOS-5647
> https://issues.apache.org/jira/browse/MESOS-5647
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The NamespaceRunner runs any function in a specified namespace. To do
> that it manages a separate thread which would be re-associated with
> that namespace.
> 
> 
> Diffs
> -
> 
>   src/linux/ns.hpp 0b4136bd3cc2d3e0cfee163d89469558e699b5f2 
>   src/tests/containerizer/ns_tests.cpp 
> fa4349e29b975550e05b00a1f848a24cd8e4f0de 
> 
> 
> Diff: https://reviews.apache.org/r/68053/diff/6/
> 
> 
> Testing
> ---
> 
> bin/mesos-tests.sh --verbose --gtest_filter="NsTest*" 
> --gtest_break_on_failure --gtest_repeat=100
> 
> 
> Thanks,
> 
> Sergey Urbanovich
> 
>



Re: Review Request 68334: Used state::checkpoint instead in CNI isolator.

2018-08-13 Thread Jie Yu

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

(Updated Aug. 13, 2018, 11:04 p.m.)


Review request for mesos, Deepak Goel, Qian Zhang, and Sergey Urbanovich.


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


Repository: mesos


Description
---

This is to ensure all or nothing semantics. We don't want to deal with a
particially written file in case agent crashes.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
f46c962d8f044092aaa113fafc536c6b25bab45c 


Diff: https://reviews.apache.org/r/68334/diff/3/

Changes: https://reviews.apache.org/r/68334/diff/2-3/


Testing
---

sudo make check


Thanks,

Jie Yu



Re: Review Request 68334: Used state::checkpoint instead in CNI isolator.

2018-08-13 Thread Jie Yu

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

(Updated Aug. 13, 2018, 11:03 p.m.)


Review request for mesos, Deepak Goel, Qian Zhang, and Sergey Urbanovich.


Summary (updated)
-

Used state::checkpoint instead in CNI isolator.


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


Repository: mesos


Description
---

This is to ensure all or nothing semantics. We don't want to deal with a
particially written file in case agent crashes.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
f46c962d8f044092aaa113fafc536c6b25bab45c 


Diff: https://reviews.apache.org/r/68334/diff/2/

Changes: https://reviews.apache.org/r/68334/diff/1-2/


Testing
---

sudo make check


Thanks,

Jie Yu



Review Request 68335: Added a CNI test to verify destroy while preparing.

2018-08-13 Thread Jie Yu

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

Review request for mesos, Deepak Goel, Qian Zhang, and Sergey Urbanovich.


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


Repository: mesos


Description
---

This test is used to catch the regression described in MESOS-9142.


Diffs
-

  src/slave/containerizer/mesos/containerizer.hpp 
7711d463c8ed92e2580c56e88d7f372c6dfaeb2b 
  src/tests/containerizer/cni_isolator_tests.cpp 
cb22e73b4215b5b0a49ac610e5f657b73d38963b 


Diff: https://reviews.apache.org/r/68335/diff/1/


Testing
---

sudo make check


Thanks,

Jie Yu



Review Request 68334: Used state::checkpoint intead in CNI isolator.

2018-08-13 Thread Jie Yu

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

Review request for mesos, Deepak Goel, Qian Zhang, and Sergey Urbanovich.


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


Repository: mesos


Description
---

This is to ensure all or nothing semantics. We don't want to deal with a
particially written file in case agent crashes.


Diffs
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
f46c962d8f044092aaa113fafc536c6b25bab45c 


Diff: https://reviews.apache.org/r/68334/diff/1/


Testing
---

sudo make check


Thanks,

Jie Yu



Review Request 68333: Made CNI isolator cleanup more robust.

2018-08-13 Thread Jie Yu

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

Review request for mesos, Deepak Goel, Qian Zhang, and Sergey Urbanovich.


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


Repository: mesos


Description
---

If the container is destroyed while in isolator preparing state, the
cleanup might fail due to missing files or directories. This patch makes
the cleanup path in CNI isolator more robust so that the cleanup does
not fail in those scenarios.


Diffs
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
f46c962d8f044092aaa113fafc536c6b25bab45c 


Diff: https://reviews.apache.org/r/68333/diff/1/


Testing
---

sudo make check


Thanks,

Jie Yu



Re: Review Request 68266: Added a benchmark for Mesos containerizer.

2018-08-08 Thread Jie Yu

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

(Updated Aug. 8, 2018, 11:19 p.m.)


Review request for mesos, Benjamin Mahler and Gilbert Song.


Repository: mesos


Description
---

The benchmark uses the standalone container agent API so that it avoids
the irrelevant agent code paths.


Diffs (updated)
-

  src/Makefile.am cf0cf22b3f582d3d4427f13288af3845aef45263 
  src/tests/CMakeLists.txt fed072a8a761c6b9e65b2c75f2e92facd8353f7c 
  src/tests/containerizer/mesos_containerizer_benchmarks.cpp PRE-CREATION 


Diff: https://reviews.apache.org/r/68266/diff/5/

Changes: https://reviews.apache.org/r/68266/diff/4-5/


Testing
---

sudo  bin/mesos-tests.sh --gtest_filter=*MesosContainerizer_BENCHMARK_Test* 
--benchmark


Thanks,

Jie Yu



Re: Review Request 68266: Added a benchmark for Mesos containerizer.

2018-08-07 Thread Jie Yu

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

(Updated Aug. 8, 2018, 4:59 a.m.)


Review request for mesos, Benjamin Mahler and Gilbert Song.


Repository: mesos


Description
---

The benchmark uses the standalone container agent API so that it avoids
the irrelevant agent code paths.


Diffs (updated)
-

  src/Makefile.am cf0cf22b3f582d3d4427f13288af3845aef45263 
  src/tests/CMakeLists.txt fed072a8a761c6b9e65b2c75f2e92facd8353f7c 
  src/tests/containerizer/mesos_containerizer_benchmarks.cpp PRE-CREATION 


Diff: https://reviews.apache.org/r/68266/diff/4/

Changes: https://reviews.apache.org/r/68266/diff/3-4/


Testing
---

sudo  bin/mesos-tests.sh --gtest_filter=*MesosContainerizer_BENCHMARK_Test* 
--benchmark


Thanks,

Jie Yu



Re: Review Request 68266: Added a benchmark for Mesos containerizer.

2018-08-07 Thread Jie Yu

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

(Updated Aug. 8, 2018, 4:33 a.m.)


Review request for mesos, Benjamin Mahler and Gilbert Song.


Repository: mesos


Description
---

The benchmark uses the standalone container agent API so that it avoids
the irrelevant agent code paths.


Diffs (updated)
-

  src/Makefile.am cf0cf22b3f582d3d4427f13288af3845aef45263 
  src/tests/CMakeLists.txt fed072a8a761c6b9e65b2c75f2e92facd8353f7c 
  src/tests/containerizer/mesos_containerizer_benchmarks.cpp PRE-CREATION 


Diff: https://reviews.apache.org/r/68266/diff/3/

Changes: https://reviews.apache.org/r/68266/diff/2-3/


Testing
---

sudo  bin/mesos-tests.sh --gtest_filter=*MesosContainerizer_BENCHMARK_Test* 
--benchmark


Thanks,

Jie Yu



Re: Review Request 68266: Added a benchmark for Mesos containerizer.

2018-08-07 Thread Jie Yu

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

(Updated Aug. 8, 2018, 4:26 a.m.)


Review request for mesos, Benjamin Mahler and Gilbert Song.


Repository: mesos


Description
---

The benchmark uses the standalone container agent API so that it avoids
the irrelevant agent code paths.


Diffs (updated)
-

  src/Makefile.am cf0cf22b3f582d3d4427f13288af3845aef45263 
  src/tests/CMakeLists.txt fed072a8a761c6b9e65b2c75f2e92facd8353f7c 
  src/tests/containerizer/mesos_containerizer_benchmarks.cpp PRE-CREATION 


Diff: https://reviews.apache.org/r/68266/diff/2/

Changes: https://reviews.apache.org/r/68266/diff/1-2/


Testing
---

sudo  bin/mesos-tests.sh --gtest_filter=*MesosContainerizer_BENCHMARK_Test* 
--benchmark


Thanks,

Jie Yu



Review Request 68266: Added a benchmark for Mesos containerizer.

2018-08-07 Thread Jie Yu

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

Review request for mesos, Benjamin Mahler and Gilbert Song.


Repository: mesos


Description
---

The benchmark uses the standalone container agent API so that it avoids
the irrelevant agent code paths.


Diffs
-

  src/Makefile.am cf0cf22b3f582d3d4427f13288af3845aef45263 
  src/tests/CMakeLists.txt fed072a8a761c6b9e65b2c75f2e92facd8353f7c 
  src/tests/containerizer/mesos_containerizer_benchmarks.cpp PRE-CREATION 


Diff: https://reviews.apache.org/r/68266/diff/1/


Testing
---

sudo  bin/mesos-tests.sh --gtest_filter=*MesosContainerizer_BENCHMARK_Test* 
--benchmark


Thanks,

Jie Yu



Re: Review Request 68239: Updated port mapper CNI test.

2018-08-06 Thread Jie Yu

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

(Updated Aug. 6, 2018, 11:42 p.m.)


Review request for mesos, Chun-Hung Hsiao and Greg Mann.


Changes
---

Addressed comments.


Repository: mesos


Description
---

This patch updated the port mapper CNI test to launch multiple
containers concurrently. This would allow us to catch the scenarios
where multiple iptables commands are executed concurrently.

This test fails if the fix for MESOS-9125 is not included.


Diffs (updated)
-

  src/common/values.hpp 39487b955fc1a3c963f69de66ba0da869dd3ab2e 
  src/tests/containerizer/cni_isolator_tests.cpp 
90d2d4103c8136d2dd883318acc135f7efca80d8 


Diff: https://reviews.apache.org/r/68239/diff/3/

Changes: https://reviews.apache.org/r/68239/diff/2-3/


Testing
---

sudo make check


Thanks,

Jie Yu



Re: Review Request 68158: Fixed the iptables deadlock in CNI port mapper plugin.

2018-08-06 Thread Jie Yu

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

(Updated Aug. 6, 2018, 11:42 p.m.)


Review request for mesos, Avinash sridharan, Chun-Hung Hsiao, and Greg Mann.


Changes
---

Addressed comments.


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


Repository: mesos


Description (updated)
---

It is possible that the port mapping cleanup command will cause iptables
to deadlock if there are a lot of entires in the iptables, because the
`sed` won't process the next line while executing `iptables -w -t nat -D
...`. But the executing of `iptables -w -t nat -D ...` might get stuck
if the first command `iptables -w -t nat -S ` didn't finish
(because the xtables lock is not released). The first command might not
finish if it has a lot of output, filling the pipe that `sed` hasn't had
a chance to process yet. See more details in MESOS-9127.

This patch fixed the issue by writing the commands to a file and then
executing them.


Diffs (updated)
-

  
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
 f1a3d263b7baa3ccbf270426745022d42fcc66ed 


Diff: https://reviews.apache.org/r/68158/diff/5/

Changes: https://reviews.apache.org/r/68158/diff/4-5/


Testing
---

sudo make check
```
[   OK ] CniIsolatorPortMapperTest.ROOT_INTERNET_CURL_PortMapper (8827 ms)
```


Thanks,

Jie Yu



Re: Review Request 68239: Updated port mapper CNI test.

2018-08-06 Thread Jie Yu


> On Aug. 6, 2018, 10:35 p.m., Chun-Hung Hsiao wrote:
> > src/tests/containerizer/cni_isolator_tests.cpp
> > Lines 1799 (patched)
> > <https://reviews.apache.org/r/68239/diff/2/?file=2069278#file2069278line1799>
> >
> > What's the reason of doing a pre-test cleanup first? To prevent 
> > residual rules from the previous run?

Yeah, that's the typical pattern we used in the test.


> On Aug. 6, 2018, 10:35 p.m., Chun-Hung Hsiao wrote:
> > src/tests/containerizer/cni_isolator_tests.cpp
> > Lines 1929-1932 (patched)
> > <https://reviews.apache.org/r/68239/diff/2/?file=2069278#file2069278line1932>
> >
> > Why do we need to consume a new port ID for the container port?

Because container is running on the host network `__MESOS_TEST__` network does 
not fork a new network namespace.


> On Aug. 6, 2018, 10:35 p.m., Chun-Hung Hsiao wrote:
> > src/tests/containerizer/cni_isolator_tests.cpp
> > Lines 1988 (patched)
> > <https://reviews.apache.org/r/68239/diff/2/?file=2069278#file2069278line1995>
> >
> > Nit: s/`containerId`/`containerIds`/
> > 
> > Also, it seems we don't use `containerId` in this test once we get them 
> > at all?

Removed.


> On Aug. 6, 2018, 10:35 p.m., Chun-Hung Hsiao wrote:
> > src/tests/containerizer/cni_isolator_tests.cpp
> > Lines 1957-1958 (original), 1990-1996 (patched)
> > <https://reviews.apache.org/r/68239/diff/2/?file=2069278#file2069278line1997>
> >
> > This test waits for all `TASK_RUNNING` before any `TASK_FINISHED`. What 
> > if the first `TASK_FINISHED` is delivered before the last `TASK_RUNNING`?
> 
> Chun-Hung Hsiao wrote:
> I missed the fact that the tasks won't finish until the `echo foo | nc` 
> commands happen. Dropping this.

I added some comment about the nc server behavior.


> On Aug. 6, 2018, 10:35 p.m., Chun-Hung Hsiao wrote:
> > src/tests/containerizer/cni_isolator_tests.cpp
> > Lines 2000-2004 (patched)
> > <https://reviews.apache.org/r/68239/diff/2/?file=2069278#file2069278line2007>
> >
> > Is it possible to just use this loop to get the container IDs and 
> > network infos?

It's logically a bit weird to combine the loop. I'll stick to the current way.


> On Aug. 6, 2018, 10:35 p.m., Chun-Hung Hsiao wrote:
> > src/tests/containerizer/cni_isolator_tests.cpp
> > Lines 2006-2012 (patched)
> > <https://reviews.apache.org/r/68239/diff/2/?file=2069278#file2069278line2013>
> >
> > How about setting up expectations for `lostExecutor` instead?

I'll use this for now since this is how it was the case before.


> On Aug. 6, 2018, 10:35 p.m., Chun-Hung Hsiao wrote:
> > src/tests/containerizer/cni_isolator_tests.cpp
> > Lines 1984-1987 (original), 2040-2043 (patched)
> > <https://reviews.apache.org/r/68239/diff/2/?file=2069278#file2069278line2047>
> >
> > If we use a matcher above, and use a`vector` instead of an array we can 
> > simply do `AWAIT_READY(collect(statusesFinished))` here. But I'm fine using 
> > loop here so not opening an issue.

Good idea!


> On Aug. 6, 2018, 10:35 p.m., Chun-Hung Hsiao wrote:
> > src/tests/containerizer/cni_isolator_tests.cpp
> > Line 2000 (original), 2045-2047 (patched)
> > <https://reviews.apache.org/r/68239/diff/2/?file=2069278#file2069278line2063>
> >
> > We could do `AWAIT_READY(collect(gcSchedule))` if `gcSchedule` is a 
> > vector.

Good idea!


- Jie


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


On Aug. 6, 2018, 8:31 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68239/
> ---
> 
> (Updated Aug. 6, 2018, 8:31 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Greg Mann.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch updated the port mapper CNI test to launch multiple
> containers concurrently. This would allow us to catch the scenarios
> where multiple iptables commands are executed concurrently.
> 
> This test fails if the fix for MESOS-9125 is not included.
> 
> 
> Diffs
> -
> 
>   src/common/values.hpp 39487b955fc1a3c963f69de66ba0da869dd3ab2e 
>   src/tests/containerizer/cni_isolator_tests.cpp 
> 90d2d4103c8136d2dd883318acc135f7efca80d8 
> 
> 
> Diff: https://reviews.apache.org/r/68239/diff/2/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>



Re: Review Request 68158: Fixed the iptables deadlock in CNI port mapper plugin.

2018-08-06 Thread Jie Yu


> On Aug. 6, 2018, 8:40 p.m., Chun-Hung Hsiao wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
> > Lines 382 (patched)
> > <https://reviews.apache.org/r/68158/diff/4/?file=2069276#file2069276line383>
> >
> > If `iptables` prints something then exits abnormally,
> > do we want to exit this script immediately, or run `sh $FILE` to do 
> > partial cleanup?

I'd rather fail immediately. The output is not reliable if the iptables command 
exits abnormally.


- Jie


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


On Aug. 6, 2018, 8:30 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68158/
> ---
> 
> (Updated Aug. 6, 2018, 8:30 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Chun-Hung Hsiao, and Greg Mann.
> 
> 
> Bugs: MESOS-9127
> https://issues.apache.org/jira/browse/MESOS-9127
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> It is possible that the port mapping cleanup command will cause iptables
> to deadlock if there are a lot of entires in the iptables, because the
> `sed` won't process the next line while executing 'iptables -w -t nat -D
> ...'. But the executing of 'iptables -w -t nat -D ...' might get stuck
> if the first command 'iptables -w -t nat -S %s' didn't finish (because
> the xtables lock is not released). The first command might not finish if
> it has a lot of output, filling the pipe that `sed` hasn't had a chance
> to process yet. See more details in MESOS-9127.
> 
> This patch fixed the issue by writing the commands to a file and then
> executing them.
> 
> 
> Diffs
> -
> 
>   
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
>  f1a3d263b7baa3ccbf270426745022d42fcc66ed 
> 
> 
> Diff: https://reviews.apache.org/r/68158/diff/4/
> 
> 
> Testing
> ---
> 
> sudo make check
> ```
> [   OK ] CniIsolatorPortMapperTest.ROOT_INTERNET_CURL_PortMapper (8827 ms)
> ```
> 
> 
> Thanks,
> 
> Jie Yu
> 
>



Re: Review Request 68158: Fixed the iptables deadlock in CNI port mapper plugin.

2018-08-06 Thread Jie Yu

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

(Updated Aug. 6, 2018, 8:30 p.m.)


Review request for mesos, Avinash sridharan, Chun-Hung Hsiao, and Greg Mann.


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


Repository: mesos


Description
---

It is possible that the port mapping cleanup command will cause iptables
to deadlock if there are a lot of entires in the iptables, because the
`sed` won't process the next line while executing 'iptables -w -t nat -D
...'. But the executing of 'iptables -w -t nat -D ...' might get stuck
if the first command 'iptables -w -t nat -S %s' didn't finish (because
the xtables lock is not released). The first command might not finish if
it has a lot of output, filling the pipe that `sed` hasn't had a chance
to process yet. See more details in MESOS-9127.

This patch fixed the issue by writing the commands to a file and then
executing them.


Diffs (updated)
-

  
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
 f1a3d263b7baa3ccbf270426745022d42fcc66ed 


Diff: https://reviews.apache.org/r/68158/diff/4/

Changes: https://reviews.apache.org/r/68158/diff/3-4/


Testing
---

sudo make check
```
[   OK ] CniIsolatorPortMapperTest.ROOT_INTERNET_CURL_PortMapper (8827 ms)
```


Thanks,

Jie Yu



Re: Review Request 68239: Updated port mapper CNI test.

2018-08-06 Thread Jie Yu

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

(Updated Aug. 6, 2018, 8:31 p.m.)


Review request for mesos, Chun-Hung Hsiao and Greg Mann.


Repository: mesos


Description
---

This patch updated the port mapper CNI test to launch multiple
containers concurrently. This would allow us to catch the scenarios
where multiple iptables commands are executed concurrently.

This test fails if the fix for MESOS-9125 is not included.


Diffs (updated)
-

  src/common/values.hpp 39487b955fc1a3c963f69de66ba0da869dd3ab2e 
  src/tests/containerizer/cni_isolator_tests.cpp 
90d2d4103c8136d2dd883318acc135f7efca80d8 


Diff: https://reviews.apache.org/r/68239/diff/2/

Changes: https://reviews.apache.org/r/68239/diff/1-2/


Testing
---

sudo make check


Thanks,

Jie Yu



Re: Review Request 68239: Updated port mapper CNI test.

2018-08-06 Thread Jie Yu

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




src/tests/containerizer/cni_isolator_tests.cpp
Lines 2002 (patched)
<https://reviews.apache.org/r/68239/#comment290017>

indent


- Jie Yu


On Aug. 6, 2018, 4:53 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68239/
> ---
> 
> (Updated Aug. 6, 2018, 4:53 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Greg Mann.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch updated the port mapper CNI test to launch multiple
> containers concurrently. This would allow us to catch the scenarios
> where multiple iptables commands are executed concurrently.
> 
> This test fails if the fix for MESOS-9125 is not included.
> 
> 
> Diffs
> -
> 
>   src/common/values.hpp 39487b955fc1a3c963f69de66ba0da869dd3ab2e 
>   src/tests/containerizer/cni_isolator_tests.cpp 
> 90d2d4103c8136d2dd883318acc135f7efca80d8 
> 
> 
> Diff: https://reviews.apache.org/r/68239/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>



Review Request 68239: Updated port mapper CNI test.

2018-08-06 Thread Jie Yu

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

Review request for mesos, Chun-Hung Hsiao and Greg Mann.


Repository: mesos


Description
---

This patch updated the port mapper CNI test to launch multiple
containers concurrently. This would allow us to catch the scenarios
where multiple iptables commands are executed concurrently.

This test fails if the fix for MESOS-9125 is not included.


Diffs
-

  src/common/values.hpp 39487b955fc1a3c963f69de66ba0da869dd3ab2e 
  src/tests/containerizer/cni_isolator_tests.cpp 
90d2d4103c8136d2dd883318acc135f7efca80d8 


Diff: https://reviews.apache.org/r/68239/diff/1/


Testing
---

sudo make check


Thanks,

Jie Yu



Re: Review Request 68158: Fixed the iptables deadlock in CNI port mapper plugin.

2018-08-06 Thread Jie Yu

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

(Updated Aug. 6, 2018, 4:52 p.m.)


Review request for mesos, Avinash sridharan, Chun-Hung Hsiao, and Greg Mann.


Changes
---

Rebased.


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


Repository: mesos


Description
---

It is possible that the port mapping cleanup command will cause iptables
to deadlock if there are a lot of entires in the iptables, because the
`sed` won't process the next line while executing 'iptables -w -t nat -D
...'. But the executing of 'iptables -w -t nat -D ...' might get stuck
if the first command 'iptables -w -t nat -S %s' didn't finish (because
the xtables lock is not released). The first command might not finish if
it has a lot of output, filling the pipe that `sed` hasn't had a chance
to process yet. See more details in MESOS-9127.

This patch fixed the issue by writing the commands to a file and then
executing them.


Diffs (updated)
-

  
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
 f1a3d263b7baa3ccbf270426745022d42fcc66ed 


Diff: https://reviews.apache.org/r/68158/diff/3/

Changes: https://reviews.apache.org/r/68158/diff/2-3/


Testing
---

sudo make check
```
[   OK ] CniIsolatorPortMapperTest.ROOT_INTERNET_CURL_PortMapper (8827 ms)
```


Thanks,

Jie Yu



Re: Review Request 68203: Updated volume isolators to honor volume mode.

2018-08-04 Thread Jie Yu


> On Aug. 5, 2018, 5:46 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/volume/host_path.cpp
> > Lines 318 (patched)
> > <https://reviews.apache.org/r/68203/diff/2/?file=2067547#file2067547line319>
> >
> > I'd suggest we separate this patch, and have one patch for each 
> > isolator (we might want to backport a few, like host path volume).
> > 
> > Also, add unit test for each isolator.

NVM, i saw the subsequent tests. Let's separate this patch please. we might 
want to backport a few to old releases


- Jie


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


On Aug. 4, 2018, 9:58 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68203/
> ---
> 
> (Updated Aug. 4, 2018, 9:58 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Greg Mann, Ilya Pronin, and Jie Yu.
> 
> 
> Bugs: MESOS-8814
> https://issues.apache.org/jira/browse/MESOS-8814
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated volume isolators to honor volume mode.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp 
> 76f1a5243c8d5028157f795d851b547a5ce57ac9 
>   src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 
> ab749be6234a5eedc0617a131c126129f43f8d62 
>   src/slave/containerizer/mesos/isolators/volume/host_path.cpp 
> 9127cf4a9d8aa2b16bb5b9903103f1f76a1e2b1a 
>   src/slave/containerizer/mesos/isolators/volume/image.hpp 
> 706b8ff28e1b1c8d15606d54d40622bc09885667 
>   src/slave/containerizer/mesos/isolators/volume/image.cpp 
> 345772439b73b4816f71e15bb4e43a5d67c51c02 
>   src/slave/containerizer/mesos/isolators/volume/sandbox_path.cpp 
> 4896c6811c2c59dcf00871b7a8b6b9b50da0f062 
>   src/slave/containerizer/mesos/isolators/volume/secret.cpp 
> 663aafc7173667f8f78b26bdc824a3ab2e04ed25 
> 
> 
> Diff: https://reviews.apache.org/r/68203/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>



Re: Review Request 68203: Updated volume isolators to honor volume mode.

2018-08-04 Thread Jie Yu

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




src/slave/containerizer/mesos/isolators/volume/host_path.cpp
Lines 318 (patched)
<https://reviews.apache.org/r/68203/#comment289964>

I'd suggest we separate this patch, and have one patch for each isolator 
(we might want to backport a few, like host path volume).

Also, add unit test for each isolator.


- Jie Yu


On Aug. 4, 2018, 9:58 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68203/
> ---
> 
> (Updated Aug. 4, 2018, 9:58 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Greg Mann, Ilya Pronin, and Jie Yu.
> 
> 
> Bugs: MESOS-8814
> https://issues.apache.org/jira/browse/MESOS-8814
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated volume isolators to honor volume mode.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp 
> 76f1a5243c8d5028157f795d851b547a5ce57ac9 
>   src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 
> ab749be6234a5eedc0617a131c126129f43f8d62 
>   src/slave/containerizer/mesos/isolators/volume/host_path.cpp 
> 9127cf4a9d8aa2b16bb5b9903103f1f76a1e2b1a 
>   src/slave/containerizer/mesos/isolators/volume/image.hpp 
> 706b8ff28e1b1c8d15606d54d40622bc09885667 
>   src/slave/containerizer/mesos/isolators/volume/image.cpp 
> 345772439b73b4816f71e15bb4e43a5d67c51c02 
>   src/slave/containerizer/mesos/isolators/volume/sandbox_path.cpp 
> 4896c6811c2c59dcf00871b7a8b6b9b50da0f062 
>   src/slave/containerizer/mesos/isolators/volume/secret.cpp 
> 663aafc7173667f8f78b26bdc824a3ab2e04ed25 
> 
> 
> Diff: https://reviews.apache.org/r/68203/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>



Re: Review Request 68158: Fixed the iptables deadlock in CNI port mapper plugin.

2018-08-04 Thread Jie Yu

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

(Updated Aug. 5, 2018, 5:30 a.m.)


Review request for mesos, Avinash sridharan, Chun-Hung Hsiao, and Greg Mann.


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


Repository: mesos


Description
---

It is possible that the port mapping cleanup command will cause iptables
to deadlock if there are a lot of entires in the iptables, because the
`sed` won't process the next line while executing 'iptables -w -t nat -D
...'. But the executing of 'iptables -w -t nat -D ...' might get stuck
if the first command 'iptables -w -t nat -S %s' didn't finish (because
the xtables lock is not released). The first command might not finish if
it has a lot of output, filling the pipe that `sed` hasn't had a chance
to process yet. See more details in MESOS-9127.

This patch fixed the issue by writing the commands to a file and then
executing them.


Diffs
-

  
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
 c40b57f78193520f9f0b901201b5c4c855cde8b3 


Diff: https://reviews.apache.org/r/68158/diff/2/


Testing
---

sudo make check
```
[   OK ] CniIsolatorPortMapperTest.ROOT_INTERNET_CURL_PortMapper (8827 ms)
```


Thanks,

Jie Yu



Re: Review Request 68158: Fixed the iptables deadlock in CNI port mapper plugin.

2018-08-02 Thread Jie Yu

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

(Updated Aug. 2, 2018, 7:17 p.m.)


Review request for mesos, Avinash sridharan, Chun-Hung Hsiao, and Greg Mann.


Changes
---

Fixed a stupid bug.


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


Repository: mesos


Description (updated)
---

It is possible that the port mapping cleanup command will cause iptables
to deadlock if there are a lot of entires in the iptables, because the
`sed` won't process the next line while executing 'iptables -w -t nat -D
...'. But the executing of 'iptables -w -t nat -D ...' might get stuck
if the first command 'iptables -w -t nat -S %s' didn't finish (because
the xtables lock is not released). The first command might not finish if
it has a lot of output, filling the pipe that `sed` hasn't had a chance
to process yet. See more details in MESOS-9127.

This patch fixed the issue by writing the commands to a file and then
executing them.


Diffs (updated)
-

  
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
 c40b57f78193520f9f0b901201b5c4c855cde8b3 


Diff: https://reviews.apache.org/r/68158/diff/2/

Changes: https://reviews.apache.org/r/68158/diff/1-2/


Testing
---

sudo make check
```
[   OK ] CniIsolatorPortMapperTest.ROOT_INTERNET_CURL_PortMapper (8827 ms)
```


Thanks,

Jie Yu



Re: Review Request 68167: Avoided hostname lookups in the CNI port mapper.

2018-08-02 Thread Jie Yu

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


Ship it!




Ship It!

- Jie Yu


On Aug. 2, 2018, 4:30 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68167/
> ---
> 
> (Updated Aug. 2, 2018, 4:30 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-9129
> https://issues.apache.org/jira/browse/MESOS-9129
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Using the `-n` option with `iptables --list` prevents potentially
> costly reverse hostname lookups. This patch adds this option to the
> relevant invocation of `iptables` in the CNI port mapper.
> 
> 
> Diffs
> -
> 
>   
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
>  c40b57f78193520f9f0b901201b5c4c855cde8b3 
> 
> 
> Diff: https://reviews.apache.org/r/68167/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Greg Mann
> 
>



Review Request 68158: Fixed the iptables deadlock in CNI port mapper plugin.

2018-08-01 Thread Jie Yu

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

Review request for mesos, Avinash sridharan, Chun-Hung Hsiao, and Greg Mann.


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


Repository: mesos


Description
---

It is possible that the port mapping cleanup command will cause iptables
to deadlock if there are a lot of entires in the iptables, because the
`sed` won't process the next line while executing `iptables -w -t nat -D
...`. But the executing of `iptables -w -t nat -D ... ` might get stuck
if the first command `iptables -w -t nat -S %s` didn't finish (because
the xtables lock is not released). The first command might not finish if
it has a lot of output, filling the pipe that `sed` hasn't had a chance
to process yet. See more details in MESOS-9127.

This patch fixed the issue by writing the commands to a file and then
executing them.


Diffs
-

  
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
 c40b57f78193520f9f0b901201b5c4c855cde8b3 


Diff: https://reviews.apache.org/r/68158/diff/1/


Testing
---

sudo make check
```
[   OK ] CniIsolatorPortMapperTest.ROOT_INTERNET_CURL_PortMapper (8827 ms)
```


Thanks,

Jie Yu



Re: Review Request 68152: Added missing iptables argument in CNI port mapper.

2018-08-01 Thread Jie Yu

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


Ship it!




I'd be a bit verbose on the commit message:
```
Previously, we are missing a `-w` option for one iptable command, which will 
cause the iptables command to fail if any application is holding the xtables 
lock. By using `-w`, the iptables will wait for the xtables lock to be 
released, instead of failing.
```

- Jie Yu


On Aug. 1, 2018, 10:23 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68152/
> ---
> 
> (Updated Aug. 1, 2018, 10:23 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-9125
> https://issues.apache.org/jira/browse/MESOS-9125
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added missing iptables argument in CNI port mapper.
> 
> 
> Diffs
> -
> 
>   
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
>  c40b57f78193520f9f0b901201b5c4c855cde8b3 
> 
> 
> Diff: https://reviews.apache.org/r/68152/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Greg Mann
> 
>



Re: Review Request 68054: Added networking statistics to cni isolator.

2018-07-27 Thread Jie Yu

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



Havent finish reviewing the test yet. Will resume next week.


src/slave/containerizer/mesos/isolators/network/cni/cni.cpp
Lines 1506-1507 (patched)
<https://reviews.apache.org/r/68054/#comment289535>

No need for this temp variable



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp
Lines 1515 (patched)
<https://reviews.apache.org/r/68054/#comment289534>

Instead of passing in container network struct, i'd suggest we just pass in 
ifName:
```
Try NetworkCniIsolatorProcess::_usage(
const hashmap ifNames)
    ```


- Jie Yu


On July 27, 2018, 11:36 p.m., Sergey Urbanovich wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68054/
> ---
> 
> (Updated July 27, 2018, 11:36 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Qian Zhang.
> 
> 
> Bugs: MESOS-5647
> https://issues.apache.org/jira/browse/MESOS-5647
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> On receiving a request for getting `usage` for a given container
> the `network/cni` isolator uses getifaddrs(3) glibc function. The
> function returns basic networking metrics for each networking
> interface in the container networking namespace. It should work
> right out of the box on all modern Linux-based systems.
> 
> To get more networking metrics please use Netlink Protocol Library.
> However, you will have to open NETLINK sockets in each networking
> namespace and manage them from the `network/cni` isolator.
> 
> JIRA: https://issues.apache.org/jira/browse/MESOS-5647
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> 31ec4ddb1049b7259b0784e5e40b002e29f6a8da 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> f46c962d8f044092aaa113fafc536c6b25bab45c 
>   src/tests/containerizer/cni_isolator_tests.cpp 
> 90d2d4103c8136d2dd883318acc135f7efca80d8 
> 
> 
> Diff: https://reviews.apache.org/r/68054/diff/2/
> 
> 
> Testing
> ---
> 
> bin/mesos-tests.sh --verbose 
> --gtest_filter="CniIsolatorTest.ROOT_VerifyResourceStatistics" 
> --gtest_break_on_failure --gtest_repeat=100
> 
> 
> Thanks,
> 
> Sergey Urbanovich
> 
>



Re: Review Request 68053: Call any function in a specified namespace.

2018-07-27 Thread Jie Yu

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



I was chatting with Sergey offline on this. Record the findings here for 
posterity:

TL;DR: calling `Future::await()` in looper thread might cause the looper thread 
to be donated to a libprocess Process. If that Process is very busy (e.g., 
master or agent Process), it's possible that the looper thread will never 
re-gain control.

In this particular case, the namespce setting requests might never be fulfilled 
if the looper thread is donated to a busy Process.

This is why this could have happened:
1) `Future::await()` is called
2) Since future is not ready, `latch->await()` will be called.
3) `Latch` will create a new libprocess Process and call `process::wait` on the 
created Process 
(https://github.com/apache/mesos/blob/1.6.0/3rdparty/libprocess/src/process.cpp#L3165)
4) The current thread will be donated to a Process since it has nothing to do 
(https://github.com/apache/mesos/blob/1.6.0/3rdparty/libprocess/src/process.cpp#L3206)
5) If the Process to be resumed on the looper thread is a super busy thread, 
meaning that the message queue is never empty, the looper thread won't get a 
chance to execute its tasks on its stack.

- Jie Yu


On July 27, 2018, 8:35 p.m., Sergey Urbanovich wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68053/
> ---
> 
> (Updated July 27, 2018, 8:35 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Qian Zhang.
> 
> 
> Bugs: MESOS-5647
> https://issues.apache.org/jira/browse/MESOS-5647
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The NamespaceRunner runs any function in a specified namespace. To do
> that it manages a separate thread which would be re-associated with
> that namespace.
> 
> 
> Diffs
> -
> 
>   src/linux/ns.hpp 0b4136bd3cc2d3e0cfee163d89469558e699b5f2 
>   src/tests/containerizer/ns_tests.cpp 
> fa4349e29b975550e05b00a1f848a24cd8e4f0de 
> 
> 
> Diff: https://reviews.apache.org/r/68053/diff/3/
> 
> 
> Testing
> ---
> 
> bin/mesos-tests.sh --verbose --gtest_filter="NsTest*" 
> --gtest_break_on_failure --gtest_repeat=100
> 
> 
> Thanks,
> 
> Sergey Urbanovich
> 
>



Re: Review Request 68053: Call any function in a specified namespace.

2018-07-27 Thread Jie Yu

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




src/linux/ns.hpp
Lines 201 (patched)
<https://reviews.apache.org/r/68053/#comment289525>

period at the end of a comment. see style guide. Fix all occurances.



src/linux/ns.hpp
Lines 227-228 (patched)
<https://reviews.apache.org/r/68053/#comment289526>

can we do the following?
```
promise->set(func());
```



src/linux/ns.hpp
Lines 243 (patched)
<https://reviews.apache.org/r/68053/#comment289527>

I think there's a race here. It's possible that some one is deleting 
`NamespaceRunner`. It triggers the destructor `~NamespaceRunner` first. 
Assuming there T1 is the thread that's doing the `delete`, and T2 is the looper 
thread.

```
[T2] while (queue). Enter the loop body
[T1] queue.reset() in ~NamespaceRunner
[T2] queue->get() segfault
```



src/tests/containerizer/ns_tests.cpp
Line 268 (original), 268 (patched)
<https://reviews.apache.org/r/68053/#comment289521>

nits: 2 lines apart according to mesos style guide



src/tests/containerizer/ns_tests.cpp
Lines 274 (patched)
<https://reviews.apache.org/r/68053/#comment289522>

nits: period at the end of each comment according to style guide



src/tests/containerizer/ns_tests.cpp
Lines 277 (patched)
<https://reviews.apache.org/r/68053/#comment289523>

Ditto on period.



src/tests/containerizer/ns_tests.cpp
Lines 286 (patched)
<https://reviews.apache.org/r/68053/#comment289524>

Ditto on period.


- Jie Yu


On July 27, 2018, 8:35 p.m., Sergey Urbanovich wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68053/
> ---
> 
> (Updated July 27, 2018, 8:35 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Qian Zhang.
> 
> 
> Bugs: MESOS-5647
> https://issues.apache.org/jira/browse/MESOS-5647
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The NamespaceRunner runs any function in a specified namespace. To do
> that it manages a separate thread which would be re-associated with
> that namespace.
> 
> 
> Diffs
> -
> 
>   src/linux/ns.hpp 0b4136bd3cc2d3e0cfee163d89469558e699b5f2 
>   src/tests/containerizer/ns_tests.cpp 
> fa4349e29b975550e05b00a1f848a24cd8e4f0de 
> 
> 
> Diff: https://reviews.apache.org/r/68053/diff/2/
> 
> 
> Testing
> ---
> 
> bin/mesos-tests.sh --verbose --gtest_filter="NsTest*" 
> --gtest_break_on_failure --gtest_repeat=100
> 
> 
> Thanks,
> 
> Sergey Urbanovich
> 
>



Re: Review Request 67936: Fixed ephemeral ports deallocation in network/port_mapping isolator.

2018-07-24 Thread Jie Yu

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


Ship it!




Ship It!

- Jie Yu


On July 17, 2018, 8:12 p.m., Ilya Pronin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67936/
> ---
> 
> (Updated July 17, 2018, 8:12 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-9080
> https://issues.apache.org/jira/browse/MESOS-9080
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Ephemeral ports are allocated for a container during the preparation
> phase and have to be deallocated during container cleanup regardless of
> whether the container process was isolated or not. Otherwise those ports
> can be leaked if the container is destroyed during preparation.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/port_mapping.cpp 
> a29282ed0488d83966f222084031ed1d2b6ab1f5 
>   src/tests/containerizer/port_mapping_tests.cpp 
> 553f53043a52cfe235fc2b2e88fb54d0f2725d3e 
> 
> 
> Diff: https://reviews.apache.org/r/67936/diff/2/
> 
> 
> Testing
> ---
> 
> Added `PortMappingIsolatorTest.ROOT_CleanupNotIsolated` test to verify that 
> ephemeral ports are properly deallocated when the container was not isolated 
> and manually checked that if fails without the fix. Ran `sudo make check`.
> 
> 
> Thanks,
> 
> Ilya Pronin
> 
>



Re: Review Request 67936: Fixed ephemeral ports deallocation in network/port_mapping isolator.

2018-07-16 Thread Jie Yu

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


Fix it, then Ship it!





src/tests/containerizer/port_mapping_tests.cpp
Lines 1826 (patched)
<https://reviews.apache.org/r/67936/#comment289044>

instead of hard code 512 which is fragile, i'd get `ephemeral_ports` 
resources, and use more than half of that.


- Jie Yu


On July 16, 2018, 11:14 p.m., Ilya Pronin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67936/
> ---
> 
> (Updated July 16, 2018, 11:14 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-9080
> https://issues.apache.org/jira/browse/MESOS-9080
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Ephemeral ports are allocated for a container during the preparation
> phase and have to be deallocated during container cleanup regardless of
> whether the container process was isolated or not. Otherwise those ports
> can be leaked if the container is destroyed during preparation.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/port_mapping.cpp 
> a29282ed0488d83966f222084031ed1d2b6ab1f5 
>   src/tests/containerizer/port_mapping_tests.cpp 
> 553f53043a52cfe235fc2b2e88fb54d0f2725d3e 
> 
> 
> Diff: https://reviews.apache.org/r/67936/diff/1/
> 
> 
> Testing
> ---
> 
> Added `PortMappingIsolatorTest.ROOT_CleanupNotIsolated` test to verify that 
> ephemeral ports are properly deallocated when the container was not isolated 
> and manually checked that if fails without the fix. Ran `sudo make check`.
> 
> 
> Thanks,
> 
> Ilya Pronin
> 
>



Re: Review Request 67923: Improved performance of cgroups::read by verifying after failure.

2018-07-16 Thread Jie Yu

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


Ship it!




LGTM! Thanks Ben. I think longer term, we will modify caller to call `verify`, 
making read/write/create pure helper for writing to cgroup filesystem.

- Jie Yu


On July 16, 2018, 2:03 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67923/
> ---
> 
> (Updated July 16, 2018, 2:03 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-8418
> https://issues.apache.org/jira/browse/MESOS-8418
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> It turns out that cgroups::verify is expensive and leads to a severe
> performance issue on the agent during container metrics collection
> if there are a lot of containers on the agent. See MESOS-8418.
> 
> Since cgroups::verify serves to provide a helpful error message,
> this patch preserves the error message, but only if the read fails.
> 
> Longer term, there probably needs to be some re-structuring of the
> code to make verification caller-controlled, or perhaps the verify
> code can occur consistently post-operation (as done in this patch),
> or perhaps verify can be optimized substantially.
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.cpp b12e63c112a7aa7a6f7150359ff5409f8214067e 
> 
> 
> Diff: https://reviews.apache.org/r/67923/diff/1/
> 
> 
> Testing
> ---
> 
> Ran through internal CI.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>



Re: Review Request 67892: Made disk type checks in SLRP hard assertions.

2018-07-13 Thread Jie Yu

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


Ship it!




Ship It!

- Jie Yu


On July 13, 2018, 6:39 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67892/
> ---
> 
> (Updated July 13, 2018, 6:39 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jie Yu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-9066
> https://issues.apache.org/jira/browse/MESOS-9066
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since the disk types of the `CREATE_DISK` and `DESTROY_DISK` should have
> been validated by the master, we now use `CHECK`s instead of returning
> failures.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> b90a4b81838fec410a97a10ce44a811bb81c87eb 
> 
> 
> Diff: https://reviews.apache.org/r/67892/diff/2/
> 
> 
> Testing
> ---
> 
> Tests done later in chain.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>



Re: Review Request 67669: Allowed resources to be removed when updating the sorter.

2018-07-13 Thread Jie Yu

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


Ship it!




Ship It!

- Jie Yu


On July 10, 2018, 8:31 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67669/
> ---
> 
> (Updated July 10, 2018, 8:31 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Jie Yu, and Meng 
> Zhu.
> 
> 
> Bugs: MESOS-9015
> https://issues.apache.org/jira/browse/MESOS-9015
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch allows the allocator to remove resources when calling
> `Sorter::update`, so Mesos can remove resources that are no longer
> available through resource conversions.
> 
> An example usage is that we can make `DESTROY_VOLUME` to convert a
> volume with a stale profile to an empty resource, so the disk space
> will disappear instead of being offered out to other frameworks.
> 
> 
> Diffs
> -
> 
>   CHANGELOG 4aef45a28c2127587b003dc0a5e2855ac108e6ff 
>   docs/upgrades.md ef89a7250fba002b0b41030f19c78268190d2671 
>   src/master/allocator/sorter/drf/sorter.cpp 
> 755a36ead42a91053c881fc46dc6acb39c819cac 
>   src/master/allocator/sorter/sorter.hpp 
> e5e86513dc4023d9c471753897635923c6c9203f 
>   src/tests/sorter_tests.cpp 266a9e72597b3aadc9756f61736c0adf0b1a5831 
> 
> 
> Diff: https://reviews.apache.org/r/67669/diff/5/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>



Re: Review Request 67876: Allowed resources to be removed in the hierarchical allocator.

2018-07-13 Thread Jie Yu

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


Fix it, then Ship it!




LGTM!


src/master/allocator/mesos/hierarchical.cpp
Line 925 (original), 938 (patched)
<https://reviews.apache.org/r/67876/#comment289018>

indentation should be 4 here since we treat `CHECK_EQ` as a function


- Jie Yu


On July 10, 2018, 9:13 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67876/
> ---
> 
> (Updated July 10, 2018, 9:13 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Jie Yu, and Meng 
> Zhu.
> 
> 
> Bugs: MESOS-9015
> https://issues.apache.org/jira/browse/MESOS-9015
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When applying `DESTROY_VOLUME` or `DESTROY_BLOCK` on a CSI volume with a
> stale profile, the volume will be converted to an empty resource to
> avoid it to be offered to frameworks. This patch updates the
> hierarchical allocator to allow such behaviors in `updateAllocation`.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 5a6cd3d2fc5bdbaaee2d05b9be9e83d4107c749b 
>   src/tests/hierarchical_allocator_tests.cpp 
> 597469e0e897e10e9805798a4ec3f445c7dc28b0 
> 
> 
> Diff: https://reviews.apache.org/r/67876/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>



Re: Review Request 67881: Fixed the error log in POSIX `os::rmdir`.

2018-07-10 Thread Jie Yu

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


Ship it!




Ship It!

- Jie Yu


On July 10, 2018, 11:14 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67881/
> ---
> 
> (Updated July 10, 2018, 11:14 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Meng Zhu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The POSIX implementation of `os::rmdir` uses FTS functions to walk
> through all paths under a given directory. According to the manpage,
> the `fts_path` field "contains the path specified to fts_open() as a
> prefix," so should not be joined with the given directory again. See:
> http://man7.org/linux/man-pages/man3/fts.3.html
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/posix/rmdir.hpp 
> 947bb243ca3ad070497369f8108064d9f4c58b30 
> 
> 
> Diff: https://reviews.apache.org/r/67881/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>



Re: Review Request 67769: Made CNI isolator recovery waits until unknown orphan cleanup is done.

2018-06-28 Thread Jie Yu

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


Fix it, then Ship it!





src/slave/containerizer/mesos/isolators/network/cni/cni.cpp
Lines 443 (patched)
<https://reviews.apache.org/r/67769/#comment288441>

I would print out LOG(ERROR) in the continuation if cleanup fails.


- Jie Yu


On June 29, 2018, 1:41 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67769/
> ---
> 
> (Updated June 29, 2018, 1:41 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-9039
> https://issues.apache.org/jira/browse/MESOS-9039
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made CNI isolator recovery waits until unknown orphan cleanup is done.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> dfa26c682333e19d246442def8482b782e7b0d24 
> 
> 
> Diff: https://reviews.apache.org/r/67769/diff/2/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>



Re: Review Request 67769: Made CNI isolator recovery waits until unknown orphan cleanup is done.

2018-06-28 Thread Jie Yu

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




src/slave/containerizer/mesos/isolators/network/cni/cni.cpp
Line 440 (original), 442 (patched)
<https://reviews.apache.org/r/67769/#comment288414>

I would use 'await' here so that the agent will still start if cleanup 
fails.


- Jie Yu


On June 28, 2018, 1:32 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67769/
> ---
> 
> (Updated June 28, 2018, 1:32 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made CNI isolator recovery waits until unknown orphan cleanup is done.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> dfa26c682333e19d246442def8482b782e7b0d24 
> 
> 
> Diff: https://reviews.apache.org/r/67769/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>



Re: Review Request 67737: Updated CNI slave recovery test.

2018-06-26 Thread Jie Yu


> On June 26, 2018, 7:44 a.m., Qian Zhang wrote:
> > Do we still need to kill the task and wait for `TASK_KILLED`?
> 
> Qian Zhang wrote:
> And is it possible for CNI DEL command gets called after 
> `reregisterExecutorMessage` is received?

It's not possible. recover containerizer should happen first before executors 
are allowed to re-register.


- Jie


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


On June 26, 2018, 5:36 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67737/
> ---
> 
> (Updated June 26, 2018, 5:36 a.m.)
> 
> 
> Review request for mesos and Qian Zhang.
> 
> 
> Bugs: MESOS-9025
> https://issues.apache.org/jira/browse/MESOS-9025
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated the test so that it can catch regression described in
> MESOS-9025. This test fails without the fix for MESOS-9025 and passes
> after the fix.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/cni_isolator_tests.cpp 
> b58a9caca136cfa42689159389bfdcb3f92f05ee 
> 
> 
> Diff: https://reviews.apache.org/r/67737/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>



Review Request 67737: Updated CNI slave recovery test.

2018-06-25 Thread Jie Yu

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

Review request for mesos and Qian Zhang.


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


Repository: mesos


Description
---

Updated the test so that it can catch regression described in
MESOS-9025. This test fails without the fix for MESOS-9025 and passes
after the fix.


Diffs
-

  src/tests/containerizer/cni_isolator_tests.cpp 
b58a9caca136cfa42689159389bfdcb3f92f05ee 


Diff: https://reviews.apache.org/r/67737/diff/1/


Testing
---

sudo make check


Thanks,

Jie Yu



Re: Review Request 67728: Fixed orphan container cleanup issue in CNI isolator.

2018-06-25 Thread Jie Yu


> On June 25, 2018, 7:55 p.m., Stéphane Cottin wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp
> > Line 433 (original), 432 (patched)
> > <https://reviews.apache.org/r/67728/diff/1/?file=2044786#file2044786line433>
> >
> > This test makes sense only if the containerID is removed from orphans 
> > during the _recover call. 
> > 
> > I may miss something, but it seems _recover() only updates infos, not 
> > orphans.

The `containerIdToState.contains(containerId)` above makes sure that the else 
branch is for orphan containers (both known or unknown).

The `orphans` parameter to `recover` method are those containers that the 
containerizer knows about, but does not tie to any checkpointed executor or 
standalone container (this is called "known orphans").

So this check makes sure we only call `cleanup` for unknown orphans 
(containerizer does not know about, but isolator notices it from isolator 
perspective. this is possible for those pre-1.3 containers, or operator 
accidentally remove some checkpoint data in the runtime dir).


- Jie


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


On June 25, 2018, 7 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67728/
> ---
> 
> (Updated June 25, 2018, 7 p.m.)
> 
> 
> Review request for mesos, Qian Zhang and Sagar Patwardhan.
> 
> 
> Bugs: MESOS-9025
> https://issues.apache.org/jira/browse/MESOS-9025
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The bug was introduced in https://reviews.apache.org/r/65987/ when we
> want to allow nested container to have a separate network namespace than
> its parent (MESOS-8534).
> 
> The original code misses a `continue` statement after recovering regular
> containers.
> 
> I am surprised that the unit tests didn't catch the bug. We'll add a new
> unit test for catching the regression.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> b6ad4fcf9f1596c07ddeb9bbb134f4619d189671 
> 
> 
> Diff: https://reviews.apache.org/r/67728/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>



Review Request 67728: Fixed orphan container cleanup issue in CNI isolator.

2018-06-25 Thread Jie Yu

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

Review request for mesos, Qian Zhang and Sagar Patwardhan.


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


Repository: mesos


Description
---

The bug was introduced in https://reviews.apache.org/r/65987/ when we
want to allow nested container to have a separate network namespace than
its parent (MESOS-8534).

The original code misses a `continue` statement after recovering regular
containers.

I am surprised that the unit tests didn't catch the bug. We'll add a new
unit test for catching the regression.


Diffs
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
b6ad4fcf9f1596c07ddeb9bbb134f4619d189671 


Diff: https://reviews.apache.org/r/67728/diff/1/


Testing
---

sudo make check


Thanks,

Jie Yu



Re: Review Request 67398: Changed default executor tests to not use pipes for synchronization.

2018-06-21 Thread Jie Yu

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


Ship it!




Ship It!

- Jie Yu


On June 15, 2018, 1 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67398/
> ---
> 
> (Updated June 15, 2018, 1 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-8917
> https://issues.apache.org/jira/browse/MESOS-8917
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Some tests of nested container functionality used pipes passed to
> launched tasks to detect whether a task has actually started executing
> the workload (`TASK_RUNNING` updates might be sent before the task
> workload is actually started).
> 
> Once we avoid leaking unspecified file descriptors into forked
> processes, this test setup will be broken. In this patch we replace
> the use of pipes for synchronization with HTTP requests to an actor
> running in the tests, or wait on other observable side effects.
> 
> 
> Diffs
> -
> 
>   src/tests/check_tests.cpp d48febfca220a9633b9884963bcf5a205db7f5e5 
>   src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
> 6050e6ebed87249382d56aedb6d98d3cf9812bb9 
> 
> 
> Diff: https://reviews.apache.org/r/67398/diff/4/
> 
> 
> Testing
> ---
> 
> `sudo make check`
> 
> Tested in internal CI on a number of platforms.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>



Re: Review Request 67596: Fixed the flakiness in the `NVIDIA_GPU_NvidiaDockerImage` test.

2018-06-15 Thread Jie Yu

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


Ship it!




Ship It!

- Jie Yu


On June 14, 2018, 3:47 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67596/
> ---
> 
> (Updated June 14, 2018, 3:47 a.m.)
> 
> 
> Review request for mesos, Jie Yu, Joseph Wu, and Kevin Klues.
> 
> 
> Bugs: MESOS-6622
> https://issues.apache.org/jira/browse/MESOS-6622
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This test is flaky because it tries to download the 1GB 'nvidia/cuda'
> image from Docker Hub, which might take more than 1 minute and cause
> the command executor unable to register in time.
> 
> This patch fixes this problem by using the default executor, which does
> not wait for fetching task images before registration. If the image
> fetch stalls (i.e. makes no progress) more than 1 minute, the container
> will fail because of the `--fetcher_stall_timeout` agent flag.
> 
> The time we wait for `TASK_FINISHED` is also extended to 180 seconds.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/nvidia_gpu_isolator_tests.cpp 
> d8c3e6d08a70bd129d8ac9c336be7a2bf7a4b0b2 
> 
> 
> Diff: https://reviews.apache.org/r/67596/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>



Re: Review Request 67398: Changed default executor tests to not use pipes for synchronization.

2018-06-12 Thread Jie Yu

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




src/tests/containerizer/nested_mesos_containerizer_tests.cpp
Lines 1017-1028 (original), 1012-1028 (patched)
<https://reviews.apache.org/r/67398/#comment287214>

Any reason we cannot use a named pipe as well here?


- Jie Yu


On June 7, 2018, 12:40 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67398/
> ---
> 
> (Updated June 7, 2018, 12:40 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-8917
> https://issues.apache.org/jira/browse/MESOS-8917
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Some tests of nested container functionality used pipes passed to
> launched tasks to detect whether a task has actually started executing
> the workload (`TASK_RUNNING` updates might be sent before the task
> workload is actually started).
> 
> Once we avoid leaking unspecified file descriptors into forked
> processes, this test setup will be broken. In this patch we replace
> the use of pipes for synchronization with HTTP requests to an actor
> running in the tests, or wait on other observable side effects.
> 
> 
> Diffs
> -
> 
>   src/tests/check_tests.cpp d48febfca220a9633b9884963bcf5a205db7f5e5 
>   src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
> 6050e6ebed87249382d56aedb6d98d3cf9812bb9 
> 
> 
> Diff: https://reviews.apache.org/r/67398/diff/3/
> 
> 
> Testing
> ---
> 
> `sudo make check`
> 
> Tested in internal CI on a number of platforms.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>



Re: Review Request 67421: Rewrote the `ROOT_BusyMountPoint` test to reflect updated behavior.

2018-06-12 Thread Jie Yu

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


Fix it, then Ship it!





src/tests/gc_tests.cpp
Lines 912 (patched)
<https://reviews.apache.org/r/67421/#comment287212>

Can you use `DEFAULT_TEST_ROLE` here?

```
flags.resources = strings::format(
"disk(%s):1024",
DEFAULT_TEST_ROLE).get();
    ```


- Jie Yu


On June 6, 2018, 12:08 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67421/
> ---
> 
> (Updated June 6, 2018, 12:08 a.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Jason Lai, and Jie Yu.
> 
> 
> Bugs: MESOS-8830
> https://issues.apache.org/jira/browse/MESOS-8830
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The current `ROOT_BusyMountPoint` test would fail because we added
> support for unmounting dangling mount points in directory to gc. This
> patch rewrote this test to reflect that after unmounting, the gc
> succeeded, directory was gone and metrics were correctly reported.
> 
> 
> Diffs
> -
> 
>   src/tests/gc_tests.cpp 619ed22edd9b3909ea24cdcbf62c354420a8d031 
> 
> 
> Diff: https://reviews.apache.org/r/67421/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>



Re: Review Request 67423: Skipped metric for non existing paths in gc.

2018-06-12 Thread Jie Yu


> On June 12, 2018, 6:17 p.m., Jie Yu wrote:
> > src/slave/gc.cpp
> > Lines 262 (patched)
> > <https://reviews.apache.org/r/67423/diff/2/?file=2035399#file2035399line262>
> >
> > This is a bit hacky. A better way should be modify `os::rmdir` to 
> > return `Try`. The second template parameter is the error code.
> > 
> > Can you add a TODO?

In fact, it should return `Try`


- Jie


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


On June 2, 2018, 5:30 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67423/
> ---
> 
> (Updated June 2, 2018, 5:30 a.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Jason Lai, and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, agent gc would increment the "failed" counter if the path
> does not exist, but this should not be an issue. This patch skipped such
> paths in both "failed" and "succeeded" counters.
> 
> 
> Diffs
> -
> 
>   src/slave/gc.cpp 390b35e6d17d6614a73c9548decbf10739560106 
> 
> 
> Diff: https://reviews.apache.org/r/67423/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>



Re: Review Request 67423: Skipped metric for non existing paths in gc.

2018-06-12 Thread Jie Yu

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


Fix it, then Ship it!





src/slave/gc.cpp
Lines 262 (patched)
<https://reviews.apache.org/r/67423/#comment287207>

This is a bit hacky. A better way should be modify `os::rmdir` to return 
`Try`. The second template parameter is the error code.

Can you add a TODO?


- Jie Yu


On June 2, 2018, 5:30 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67423/
> ---
> 
> (Updated June 2, 2018, 5:30 a.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Jason Lai, and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, agent gc would increment the "failed" counter if the path
> does not exist, but this should not be an issue. This patch skipped such
> paths in both "failed" and "succeeded" counters.
> 
> 
> Diffs
> -
> 
>   src/slave/gc.cpp 390b35e6d17d6614a73c9548decbf10739560106 
> 
> 
> Diff: https://reviews.apache.org/r/67423/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>



Re: Review Request 67264: Unmounted any mount points in gc paths.

2018-06-12 Thread Jie Yu

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


Fix it, then Ship it!




LGTM overall!


src/slave/gc.cpp
Lines 213 (patched)
<https://reviews.apache.org/r/67264/#comment287189>

`if (mountTable.isError())`



src/slave/gc.cpp
Lines 226-228 (patched)
<https://reviews.apache.org/r/67264/#comment287197>

This double for loop will be quite expensive if we have a lot of 
directories to delete, and quite a lot of mount table entries.

I'd suggest we some short cut like the following:
```
foreach (const fs::MountInfoTable::Entry& entry,
 adaptor::reverse(mountTable->entries)) {
  // Ignore mounts whose targets are not under `workDir`.
  if (!strings::startsWith(
  path::join(entry.target, ""),
  path::join(workDir, "")) {
continue;
  }
  
  for (auto it = infos.begin(); it != infos.end(); ) {
...
  }
}
```



src/slave/gc.cpp
Lines 231 (patched)
<https://reviews.apache.org/r/67264/#comment287191>

Can you add a TODO to validate that `info->path` is a realpath. I think we 
should just make sure slave's workdir is a realpath.



src/slave/paths.cpp
Lines 725-728 (patched)
<https://reviews.apache.org/r/67264/#comment287206>

DO you still need this?


- Jie Yu


On June 11, 2018, 11:04 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67264/
> ---
> 
> (Updated June 11, 2018, 11:04 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Jason Lai, and Jie Yu.
> 
> 
> Bugs: MESOS-8830
> https://issues.apache.org/jira/browse/MESOS-8830
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In various corner cases, agent may not get chance to properly unmount
> persistent volumes mounted inside an executor's sandbox. When GC later
> gets to these sandbox directories, permanent data loss can happen (see
> MESOS-8830).
> 
> Currently, the only mounts in the host mount namespace under the sandbox
> directories are persistent volumes, so this diff added protection to
> unmount any dangling mount points before calling `rmdir` on the
> directory.
> 
> NOTE: this means agent will not garbage collect any path if it cannot
> read its own `mountinfo` table.
> 
> 
> Diffs
> -
> 
>   src/local/local.cpp afff54653e8e659d947ddbee6dc38ba2715f2a78 
>   src/slave/gc.hpp df40165bb8a23f065156bf6c5f354b143d88c088 
>   src/slave/gc.cpp 390b35e6d17d6614a73c9548decbf10739560106 
>   src/slave/gc_process.hpp 20374ad91820341282fdf18ecade60a020e26cea 
>   src/slave/main.cpp 646125344d590b28256d8ee684d7e51a90e82f23 
>   src/slave/paths.hpp 015896453410a33923eed07b3e676be19af62a48 
>   src/slave/paths.cpp ed0b1276908f4990ce7a24c96aea20e8c79d3126 
>   src/tests/cluster.cpp 01eb0950e687227dac81b1cdb9eaba3379cf5dbb 
>   src/tests/gc_tests.cpp 619ed22edd9b3909ea24cdcbf62c354420a8d031 
>   src/tests/mesos.hpp 733344a2f07ebd9d841a55fb9bbfda2e3c1a1eb2 
>   src/tests/mesos.cpp d3c87c295429481c59d5a49398e289a4b84e4496 
>   src/tests/slave_tests.cpp 3d67511de5abd3466eeb5ad1daf318209bd69eed 
> 
> 
> Diff: https://reviews.apache.org/r/67264/diff/7/
> 
> 
> Testing
> ---
> 
> Added a unit test in following patch.
> 
> Tested with following procedures:
> 1. Start a test master and agent;
> 2. Created a persistent volume on agent through operator API;
> 3. Use `mesos-execute` to run a task;
> 4. Stop the agent;
> 5. Manually bind mount persistent volume path into a `volume` directory 
> inside the executor sandbox (to simulate a dangling mount in MESOS-8830);
> 6. Restart agent with `--gc_disk_headroom=1.0 --gc_delay=1secs` to force it 
> gc the path immediately.
> 
> With this fix, we observed that the dangling mount is automatically cleaned 
> up, and agent produces log line:
> ```
> W0523 06:00:04.001075 82745 gc.cpp:229] Unmounting dangling mount point 
> '/home/zhitao/mesos-workdir/slaves/b3eb3aff-d19d-45ff-8113-f0316462d3fa-S0/frameworks/b3eb3aff-d19d-45ff-8113-f0316462d3fa-/executors/test_id/runs/1cd3bd06-2632-4541-a708-80c7cd51c74b/volume'
>  of persistent volume '/home/zhitao/mesos-workdir/volumes/roles/role/id1' 
> inside garbage collected path 
> '/home/zhitao/mesos-workdir/slaves/b3eb3aff-d19d-45ff-8113-f0316462d3fa-S0'
> ```
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>



Re: Review Request 67541: Made `NoopResourceEstimator` return a forever-pending future.

2018-06-11 Thread Jie Yu

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


Ship it!




Ship It!

- Jie Yu


On June 11, 2018, 11:51 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67541/
> ---
> 
> (Updated June 11, 2018, 11:51 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Gastón Kleiman, and Jie Yu.
> 
> 
> Bugs: MESOS-8873
> https://issues.apache.org/jira/browse/MESOS-8873
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> There is no need for the noop resource estimator to return a ready
> future of empty resource since it is only activated when the user
> does not want to use an resource estimator.
> 
> 
> Diffs
> -
> 
>   src/slave/resource_estimators/noop.cpp 
> c3ddc0149b2545183a105eb2b9dd31488f40 
> 
> 
> Diff: https://reviews.apache.org/r/67541/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>



Re: Review Request 67264: Unmounted any dangling persistent volume in gc paths.

2018-06-05 Thread Jie Yu

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




src/slave/gc.cpp
Lines 225 (patched)
<https://reviews.apache.org/r/67264/#comment286822>

`isPersistentVolumePath` won't work for MOUNT type persistent volumes.

Currently, the only mounts in the host mount namespace under the sandbox 
directories (i.e., `/var/lib/mesos/slaves/...` are persistent volume mounts. 
I'd suggest we compare `entry.target` with that to determine if we need to 
unmount.



src/tests/gc_tests.cpp
Line 99 (original), 99 (patched)
<https://reviews.apache.org/r/67264/#comment286766>

remove one `;`?



src/tests/mesos.cpp
Line 694 (original), 694 (patched)
<https://reviews.apache.org/r/67264/#comment286767>

Please move `:` to the next line to follow our style guide


- Jie Yu


On May 31, 2018, 8:38 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67264/
> ---
> 
> (Updated May 31, 2018, 8:38 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Jason Lai, and Jie Yu.
> 
> 
> Bugs: MESOS-8830
> https://issues.apache.org/jira/browse/MESOS-8830
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In various corner cases, agent may not get chance to properly unmount
> persistent volumes mounted inside an executor's sandbox. When GC later
> gets to these sandbox directories, permanent data loss can happen (see
> MESOS-8830).
> 
> This patch added some protection to unmount possible persistent
> volumes inside a path to gc, and skipped the path if unmount failed.
> 
> NOTE: this means agent will not garbage collect any path if it cannot
> read its own `mountinfo` table.
> 
> 
> Diffs
> -
> 
>   src/local/local.cpp afff54653e8e659d947ddbee6dc38ba2715f2a78 
>   src/slave/gc.hpp df40165bb8a23f065156bf6c5f354b143d88c088 
>   src/slave/gc.cpp 390b35e6d17d6614a73c9548decbf10739560106 
>   src/slave/gc_process.hpp 20374ad91820341282fdf18ecade60a020e26cea 
>   src/slave/main.cpp 646125344d590b28256d8ee684d7e51a90e82f23 
>   src/slave/paths.hpp 015896453410a33923eed07b3e676be19af62a48 
>   src/slave/paths.cpp ed0b1276908f4990ce7a24c96aea20e8c79d3126 
>   src/tests/cluster.cpp b56212f6529a4d307e65797ad9bb34f2104fc832 
>   src/tests/gc_tests.cpp 619ed22edd9b3909ea24cdcbf62c354420a8d031 
>   src/tests/mesos.hpp 733344a2f07ebd9d841a55fb9bbfda2e3c1a1eb2 
>   src/tests/mesos.cpp d3c87c295429481c59d5a49398e289a4b84e4496 
>   src/tests/slave_tests.cpp 65d860594572b58a50a89358e31e97fd2a10bf08 
> 
> 
> Diff: https://reviews.apache.org/r/67264/diff/4/
> 
> 
> Testing
> ---
> 
> Added a unit test in following patch.
> 
> Tested with following procedures:
> 1. Start a test master and agent;
> 2. Created a persistent volume on agent through operator API;
> 3. Use `mesos-execute` to run a task;
> 4. Stop the agent;
> 5. Manually bind mount persistent volume path into a `volume` directory 
> inside the executor sandbox (to simulate a dangling mount in MESOS-8830);
> 6. Restart agent with `--gc_disk_headroom=1.0 --gc_delay=1secs` to force it 
> gc the path immediately.
> 
> With this fix, we observed that the dangling mount is automatically cleaned 
> up, and agent produces log line:
> ```
> W0523 06:00:04.001075 82745 gc.cpp:229] Unmounting dangling mount point 
> '/home/zhitao/mesos-workdir/slaves/b3eb3aff-d19d-45ff-8113-f0316462d3fa-S0/frameworks/b3eb3aff-d19d-45ff-8113-f0316462d3fa-/executors/test_id/runs/1cd3bd06-2632-4541-a708-80c7cd51c74b/volume'
>  of persistent volume '/home/zhitao/mesos-workdir/volumes/roles/role/id1' 
> inside garbage collected path 
> '/home/zhitao/mesos-workdir/slaves/b3eb3aff-d19d-45ff-8113-f0316462d3fa-S0'
> ```
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>



Re: Review Request 67398: Changed default executor tests to not use pipes for synchronization.

2018-06-04 Thread Jie Yu


> On June 4, 2018, 11:13 p.m., Andrew Schwartzmeyer wrote:
> > src/tests/containerizer/nested_mesos_containerizer_tests.cpp
> > Line 461 (original), 486 (patched)
> > <https://reviews.apache.org/r/67398/diff/2/?file=2034482#file2034482line486>
> >
> > wget is not usually installed by default...

That's true. One possibility is to use named pipe
https://linux.die.net/man/3/mkfifo


- Jie


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


On June 1, 2018, 3:55 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67398/
> ---
> 
> (Updated June 1, 2018, 3:55 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-8917
> https://issues.apache.org/jira/browse/MESOS-8917
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Some tests of nested container functionality used pipes passed to
> launched tasks to detect whether a task has actually started executing
> the workload (`TASK_RUNNING` updates might be sent before the task
> workload is actually started).
> 
> Once we avoid leaking unspecified file descriptors into forked
> processes, this test setup will be broken. In this patch we replace
> the use of pipes for synchronization with HTTP requests to an actor
> running in the tests, or wait on other observable side effects.
> 
> 
> Diffs
> -
> 
>   src/tests/check_tests.cpp d48febfca220a9633b9884963bcf5a205db7f5e5 
>   src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
> 6050e6ebed87249382d56aedb6d98d3cf9812bb9 
> 
> 
> Diff: https://reviews.apache.org/r/67398/diff/2/
> 
> 
> Testing
> ---
> 
> `sudo make check`
> 
> Tested in internal CI on a number of platforms.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>



Re: Review Request 67398: Changed default executor tests to not use pipes for synchronization.

2018-06-04 Thread Jie Yu

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




src/tests/containerizer/nested_mesos_containerizer_tests.cpp
Lines 583-585 (original), 584-586 (patched)
<https://reviews.apache.org/r/67398/#comment286701>

I think there's a race here. If the process is spawned, but the initailize 
is not called yet, the command might return a failure?

I think a better way is to have a helper in the Waiter so that we can wait 
until the route has been setup.



src/tests/containerizer/nested_mesos_containerizer_tests.cpp
Line 1435 (original), 1426-1427 (patched)
<https://reviews.apache.org/r/67398/#comment286702>

Hum, I think if the route is not setup, the wget will get a 404?

My suggestion is to make this more explicit. For instance, have a rourte 
called `/trigger` and a Promise called `triggered` (for client to trigger an 
event that test is waiting), and have another route called `/wait` and have a 
Promise called `notify` (for test to notify the client).


- Jie Yu


On June 1, 2018, 3:55 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67398/
> ---
> 
> (Updated June 1, 2018, 3:55 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-8917
> https://issues.apache.org/jira/browse/MESOS-8917
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Some tests of nested container functionality used pipes passed to
> launched tasks to detect whether a task has actually started executing
> the workload (`TASK_RUNNING` updates might be sent before the task
> workload is actually started).
> 
> Once we avoid leaking unspecified file descriptors into forked
> processes, this test setup will be broken. In this patch we replace
> the use of pipes for synchronization with HTTP requests to an actor
> running in the tests, or wait on other observable side effects.
> 
> 
> Diffs
> -
> 
>   src/tests/check_tests.cpp d48febfca220a9633b9884963bcf5a205db7f5e5 
>   src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
> 6050e6ebed87249382d56aedb6d98d3cf9812bb9 
> 
> 
> Diff: https://reviews.apache.org/r/67398/diff/2/
> 
> 
> Testing
> ---
> 
> `sudo make check`
> 
> Tested in internal CI on a number of platforms.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>



Re: Review Request 67305: Moved the "Resource Provider" section to `resource-provider.md`.

2018-05-31 Thread Jie Yu

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


Ship it!




Ship It!

- Jie Yu


On May 25, 2018, 1:24 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67305/
> ---
> 
> (Updated May 25, 2018, 1:24 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Greg Mann, and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The concept of resource providers should have a documentation file
> separated from `csi.md`. This patch moves the "Resource Provider"
> section from `csi.md` to `resource-provider.md` as a start, and also for
> preventing dangling links from other documents.
> 
> 
> Diffs
> -
> 
>   docs/csi.md 9d06da0a4494a7e3bd86e28757846d2ee72efbf3 
>   docs/resource-provider.md PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/67305/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>



Re: Review Request 67303: Added documentation for resource provider and CSI plugin metrics.

2018-05-31 Thread Jie Yu

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


Fix it, then Ship it!




Thanks!


docs/monitoring.md
Lines 1771 (patched)
<https://reviews.apache.org/r/67303/#comment286543>

"that apply to the resources provided by a resource provider with the given 
type and name"



docs/monitoring.md
Lines 1813 (patched)
<https://reviews.apache.org/r/67303/#comment286544>

may vary



docs/monitoring.md
Lines 1814-1816 (patched)
<https://reviews.apache.org/r/67303/#comment286545>

the following is a comprehensive list of operation types and the 
corresponding resource providers that support them. Note that the name column 
is for the operation placeholder in the above metrics.


- Jie Yu


On May 25, 2018, 6:30 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67303/
> ---
> 
> (Updated May 25, 2018, 6:30 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Greg Mann, Jie Yu, and Joerg 
> Schad.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added documentation for resource provider and CSI plugin metrics.
> 
> 
> Diffs
> -
> 
>   docs/home.md 5471c70d4023f88adae2234aec267e4d6fea83df 
>   docs/monitoring.md c20b99eb32a50830ea6f66b5bdbfcabe45fc6ecb 
> 
> 
> Diff: https://reviews.apache.org/r/67303/diff/3/
> 
> 
> Testing
> ---
> 
> Spell checked through VIM's `:set spell` and manually tested the links.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>



Re: Review Request 67328: Used italic fonts to denote placeholdes in metrics.

2018-05-31 Thread Jie Yu

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


Ship it!




Ship It!

- Jie Yu


On May 25, 2018, 6:28 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67328/
> ---
> 
> (Updated May 25, 2018, 6:28 p.m.)
> 
> 
> Review request for mesos, Greg Mann and Joerg Schad.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used italic fonts to denote placeholdes in metrics.
> 
> 
> Diffs
> -
> 
>   docs/monitoring.md c20b99eb32a50830ea6f66b5bdbfcabe45fc6ecb 
> 
> 
> Diff: https://reviews.apache.org/r/67328/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>



Re: Review Request 67258: Fixed flakiness for some `AgentResourceProviderConfigApiTest` tests.

2018-05-31 Thread Jie Yu

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


Ship it!




Ship It!

- Jie Yu


On May 22, 2018, 11:39 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67258/
> ---
> 
> (Updated May 22, 2018, 11:39 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Gaston Kleiman, and Greg Mann.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `ROOT_Add`, `ROOT_Update` and `ROOT_Remove` tests mistakenly use
> `WillOnce` to declient unwanted offers. This patch changes them to
> `WillRepeatedly`.
> 
> 
> Diffs
> -
> 
>   src/tests/agent_resource_provider_config_api_tests.cpp 
> 7d4d3b9d21c3a0c603f805f47c57a7fa67db9d08 
> 
> 
> Diff: https://reviews.apache.org/r/67258/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>



Re: Review Request 67257: Fixed filters in test `ROOT_ReconcileDroppedOperation` for consistency.

2018-05-31 Thread Jie Yu

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


Ship it!




Ship It!

- Jie Yu


On May 22, 2018, 11:38 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67257/
> ---
> 
> (Updated May 22, 2018, 11:38 p.m.)
> 
> 
> Review request for mesos, Gaston Kleiman and Greg Mann.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed filters in test `ROOT_ReconcileDroppedOperation` for consistency.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 45cb38922f78941e82667a60b3b71ce220c9202f 
> 
> 
> Diff: https://reviews.apache.org/r/67257/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>



Re: Review Request 67256: Added a unit test for CSI plugin RPC metrics.

2018-05-31 Thread Jie Yu

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


Ship it!




Ship It!

- Jie Yu


On May 25, 2018, 12:24 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67256/
> ---
> 
> (Updated May 25, 2018, 12:24 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Greg Mann, Jie Yu, and Jan 
> Schlicht.
> 
> 
> Bugs: MESOS-8943
> https://issues.apache.org/jira/browse/MESOS-8943
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds the `ROOT_CsiPluginRpcMetrics` test that issues a
> `CREATE_VOLUME` followed by a `DESTROY_VOLUME`, which would fail due to
> an out-of-band deletion of the actual volume.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 45cb38922f78941e82667a60b3b71ce220c9202f 
> 
> 
> Diff: https://reviews.apache.org/r/67256/diff/2/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>



Re: Review Request 67255: Added per-CSI-call RPC metrics for SLRP.

2018-05-31 Thread Jie Yu

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


Fix it, then Ship it!





src/csi/client.hpp
Lines 68 (patched)
<https://reviews.apache.org/r/67255/#comment286533>

nits: I'd just make this consistent with others :)

```
template <>
process::Future
Client::call(
const DeleteVolumeRequest& request);
```

Ditto below.



src/resource_provider/storage/provider.cpp
Lines 1776 (patched)
<https://reviews.apache.org/r/67255/#comment286534>

Does this work? Can you make it more verbose if that's your intention to 
log here.


- Jie Yu


On May 30, 2018, 3:01 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67255/
> ---
> 
> (Updated May 30, 2018, 3:01 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Greg Mann, Jie Yu, and Jan 
> Schlicht.
> 
> 
> Bugs: MESOS-8943
> https://issues.apache.org/jira/browse/MESOS-8943
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> For each CSI call, e.g., `csi.v0.Identity.Probe`, we the following
> metrics for SLRP:
> `csi_plugin/rpcs/csi.v0.Identity.Probe/pending`
> `csi_plugin/rpcs/csi.v0.Identity.Probe/successes`
> `csi_plugin/rpcs/csi.v0.Identity.Probe/errors`
> `csi_plugin/rpcs/csi.v0.Identity.Probe/cancelled`
> 
> To add these per-CSI-call metrics, each method in `csi::v0::Client`,
> e.g., `csi::v0::Client::Probe`, is changed to
> `csi::v0::Client::call`, to make RPC calls based on the RPC enum
> value. A `call` helper function in SLRP is also added to intercept CSI
> calls and update the corresponding metrics.
> 
> 
> Diffs
> -
> 
>   src/csi/client.hpp 9d7019afb08168d18e5a78831a45af88a75bf809 
>   src/csi/client.cpp a4ba1f12a79354f81b29924b19b119fad31a06b9 
>   src/resource_provider/storage/provider.cpp 
> 63b5d7e5f10d6ad02b5cd11b119def3b4abf4180 
>   src/tests/csi_client_tests.cpp d5993d617056e86ae5a1224258cc6e0741466166 
> 
> 
> Diff: https://reviews.apache.org/r/67255/diff/8/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> A unit test for the metrics will be introduced in the next patch.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>



Re: Review Request 67375: Added the `RPC` enum and `RPCTraits` helper.

2018-05-31 Thread Jie Yu

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


Ship it!





src/csi/rpc.cpp
Lines 31 (patched)
<https://reviews.apache.org/r/67375/#comment286524>

nits: I suggest we align the statements here using the same way:
```
return stream
  << Identity::service_full_name()
  << ".GetPluginInfo";
```


- Jie Yu


On May 30, 2018, 2:57 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67375/
> ---
> 
> (Updated May 30, 2018, 2:57 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Greg Mann, and Jie Yu.
> 
> 
> Bugs: MESOS-8943
> https://issues.apache.org/jira/browse/MESOS-8943
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To make it easy to enumerate all types of CSI RPC calls, the `RPC` enum
> is introduced. The `RPCTraits` helper class can be used to determine the
> request and response type of a particular RPC.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt f86884de2beb946c8bfc2bb8260e09a9c98ce625 
>   src/Makefile.am b7184ceccef5f2e985d905c155156f95c7a7c7b4 
>   src/csi/rpc.hpp PRE-CREATION 
>   src/csi/rpc.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/67375/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>



Re: Review Request 67274: Rejected non-zero scalar resource values which be represented as zero.

2018-05-30 Thread Jie Yu

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


Ship it!




Ship It!

- Jie Yu


On May 30, 2018, 9:50 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67274/
> ---
> 
> (Updated May 30, 2018, 9:50 p.m.)
> 
> 
> Review request for mesos, Jie Yu, Joris Van Remoortere, Michael Park, and 
> Neil Conway.
> 
> 
> Bugs: MESOS-8945
> https://issues.apache.org/jira/browse/MESOS-8945
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Internally values of scalar resources are stored in a fixed point
> representation. This can lead to (expected) precision loss.
> 
> This patch updates the resource validation ensuring that values of
> scalar resources are either zero or large enough so that they are
> still represented as non-zero values after conversion to the internal
> fixed point format. With that we can still parse resources containing
> resource values explictly set to zero to be able to distinguish empty
> and abset resources, but prevent propagation of values accidentially
> rounded to an effectively empty result.
> 
> While this patch should not change valid, intended use cases, it might
> change the behavior for erroneous, already broken framework workflows.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp 192d086e60b81c0ae8e26d4120beee2034f63d3e 
>   src/tests/master_tests.cpp 6efcbd12902de32428cb7f23ed0d3967a18f749d 
>   src/tests/resources_tests.cpp 1201efec3ff0abc268f896c1fe2330571390b3fd 
> 
> 
> Diff: https://reviews.apache.org/r/67274/diff/5/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>



Re: Review Request 67274: Rejected non-zero scalar resource values which be represented as zero.

2018-05-30 Thread Jie Yu


> On May 30, 2018, 6:17 p.m., Benjamin Mahler wrote:
> > src/common/resources.cpp
> > Lines 866-868 (original), 875-883 (patched)
> > <https://reviews.apache.org/r/67274/diff/4/?file=2032028#file2032028line875>
> >
> > Can we reject 0? We were previously allowing it?

Yes, we debated that. I feel that specifing the resource to be zero is not a 
valid use case anyway (simply not specify the resource in that case). We plan 
to send an email to user list.


- Jie


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


On May 28, 2018, 3:11 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67274/
> ---
> 
> (Updated May 28, 2018, 3:11 p.m.)
> 
> 
> Review request for mesos, Jie Yu, Joris Van Remoortere, Michael Park, and 
> Neil Conway.
> 
> 
> Bugs: MESOS-8945
> https://issues.apache.org/jira/browse/MESOS-8945
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Internally values of scalar resources are stored in a fixed point
> representation. This can lead to (expected) precision loss.
> 
> This patch adds resource validation ensuring that values of scalar
> resources are either zero or large enough so that they are still
> represented as non-zero values after conversion to the internal fixed
> point format.
> 
> While this patch should not change valid, intended use cases, it might
> change the behavior for erroneous, already broken framework workflows.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp 192d086e60b81c0ae8e26d4120beee2034f63d3e 
>   src/tests/resources_tests.cpp 1201efec3ff0abc268f896c1fe2330571390b3fd 
> 
> 
> Diff: https://reviews.apache.org/r/67274/diff/4/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>



Re: Review Request 67360: Added link targets to master and agent flags.

2018-05-29 Thread Jie Yu

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


Ship it!




Ship It!

- Jie Yu


On May 29, 2018, 5:46 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67360/
> ---
> 
> (Updated May 29, 2018, 5:46 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jason Lai, and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added link targets for the master and agent flag reference. This pattern
> should be maintained so that documentation authors can link to flags in
> a standard way.
> 
> 
> Diffs
> -
> 
>   docs/configuration/agent.md af0c0405da54bad748dcf4de1af0d1cad2dd4661 
>   docs/configuration/master-and-agent.md 
> df1868161a683a9df4f8afa2647ddfe5d224987f 
>   docs/configuration/master.md 6f55ad9100ac920c7d630414a9f1ff2720a505ef 
> 
> 
> Diff: https://reviews.apache.org/r/67360/diff/1/
> 
> 
> Testing
> ---
> 
> Manual inspection of the generated site.
> 
> 
> Thanks,
> 
> James Peach
> 
>



Re: Review Request 67363: Switched test devices for the `linux/devices` isolator tests.

2018-05-29 Thread Jie Yu

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


Ship it!




Ship It!

- Jie Yu


On May 29, 2018, 8:36 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67363/
> ---
> 
> (Updated May 29, 2018, 8:36 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Jie Yu.
> 
> 
> Bugs: MESOS-8958
> https://issues.apache.org/jira/browse/MESOS-8958
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `cpuid` device is not always available when running tests
> in a VM, so switch to `/dev/net/tun`. For the purposes of this
> test, is doesn't matter what the device is as long as it is
> in a subdirectory.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/linux_devices_isolator_tests.cpp 
> efaa43b8b7b7c73c35d995bfbb81fcd620143ea1 
> 
> 
> Diff: https://reviews.apache.org/r/67363/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check (Fedora 27)
> 
> 
> Thanks,
> 
> James Peach
> 
>



Re: Review Request 67362: Updated `linux/devices` isolator tests for compiler compatibility.

2018-05-29 Thread Jie Yu

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


Ship it!




Ship It!

- Jie Yu


On May 29, 2018, 8:20 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67362/
> ---
> 
> (Updated May 29, 2018, 8:20 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Jie Yu.
> 
> 
> Bugs: MESOS-8958
> https://issues.apache.org/jira/browse/MESOS-8958
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `linux/devices` isolator tests fail to build on on
> Ubuntu 14.04 with `gcc (Ubuntu 4.8.4-2ubuntu1~14.04.4)
> 4.8.4` due to what seems to me a compiler bug, possibly
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=57824.
> 
> Work around any compiler issues by defining the test cases
> in a separate vector so that the copmiler doesn't have any
> raw strings in the macro expansion.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/linux_devices_isolator_tests.cpp 
> efaa43b8b7b7c73c35d995bfbb81fcd620143ea1 
> 
> 
> Diff: https://reviews.apache.org/r/67362/diff/2/
> 
> 
> Testing
> ---
> 
> sudo make check (Fedora 27)
> 
> 
> Thanks,
> 
> James Peach
> 
>



Re: Review Request 67274: Rejected non-zero scalar resource values which be represented as zero.

2018-05-29 Thread Jie Yu

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


Ship it!




Ship It!

- Jie Yu


On May 28, 2018, 3:11 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67274/
> ---
> 
> (Updated May 28, 2018, 3:11 p.m.)
> 
> 
> Review request for mesos, Jie Yu, Joris Van Remoortere, Michael Park, and 
> Neil Conway.
> 
> 
> Bugs: MESOS-8945
> https://issues.apache.org/jira/browse/MESOS-8945
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Internally values of scalar resources are stored in a fixed point
> representation. This can lead to (expected) precision loss.
> 
> This patch adds resource validation ensuring that values of scalar
> resources are either zero or large enough so that they are still
> represented as non-zero values after conversion to the internal fixed
> point format.
> 
> While this patch should not change valid, intended use cases, it might
> change the behavior for erroneous, already broken framework workflows.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp 192d086e60b81c0ae8e26d4120beee2034f63d3e 
>   src/tests/resources_tests.cpp 1201efec3ff0abc268f896c1fe2330571390b3fd 
> 
> 
> Diff: https://reviews.apache.org/r/67274/diff/3/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>



Re: Review Request 67331: Documented the `linux/devices` isolator.

2018-05-25 Thread Jie Yu

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


Fix it, then Ship it!





docs/isolators/linux-devices.md
Lines 14 (patched)
<https://reviews.apache.org/r/67331/#comment286302>

Can you add some example of `--allowed_devices`, or provide a link to the 
example?


- Jie Yu


On May 25, 2018, 10:11 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67331/
> ---
> 
> (Updated May 25, 2018, 10:11 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jason Lai, and Jie Yu.
> 
> 
> Bugs: MESOS-8792
> https://issues.apache.org/jira/browse/MESOS-8792
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Documented the `linux/devices` isolator in the Mesos containerizer
> documentation, upgrade guide and CHANGELOG.
> 
> 
> Diffs
> -
> 
>   CHANGELOG b78915daeddc5126c5db0ada2de1bcd79caf2d6b 
>   docs/isolators/linux-devices.md PRE-CREATION 
>   docs/mesos-containerizer.md 42a09771000f6fdebc660f7d771e4cc17fd40ded 
>   docs/upgrades.md fb6b865af5caa4ec7181842a257a8cf0aefd18f3 
> 
> 
> Diff: https://reviews.apache.org/r/67331/diff/1/
> 
> 
> Testing
> ---
> 
> Manually build and viewed the website.
> 
> 
> Thanks,
> 
> James Peach
> 
>



Re: Review Request 67224: Combined and renamed `csi_*_plugin_terminations` metrics.

2018-05-25 Thread Jie Yu

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


Ship it!




Ship It!

- Jie Yu


On May 24, 2018, 12:47 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67224/
> ---
> 
> (Updated May 24, 2018, 12:47 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Greg Mann, Jie Yu, and Jan 
> Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To avoid double counting when the operator aggregates the
> `csi_controller_plugin_terminations` and `csi_node_plugin_terminations`,
> these two are now merged into `csi_plugin/container_terminations`.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> 63b5d7e5f10d6ad02b5cd11b119def3b4abf4180 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 45cb38922f78941e82667a60b3b71ce220c9202f 
> 
> 
> Diff: https://reviews.apache.org/r/67224/diff/3/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>



Re: Review Request 65666: Added a unit test for SLRP operation state metrics.

2018-05-25 Thread Jie Yu

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


Ship it!




Ship It!

- Jie Yu


On May 22, 2018, 11:33 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65666/
> ---
> 
> (Updated May 22, 2018, 11:33 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Greg Mann, Jie Yu, and Jan 
> Schlicht.
> 
> 
> Bugs: MESOS-8383
> https://issues.apache.org/jira/browse/MESOS-8383
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds the `ROOT_OperationStateMetrics` test that issues a
> `CREATE_VOLUME` followed by two `DESTROY_VOLUME`s. The first one will
> fail due to an out-of-band deletion of the actual volume, and the second
> one will fail due to modifying the resource version.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 45cb38922f78941e82667a60b3b71ce220c9202f 
> 
> 
> Diff: https://reviews.apache.org/r/65666/diff/6/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>



Re: Review Request 65665: Added operation state metrics in SLRP.

2018-05-25 Thread Jie Yu

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


Fix it, then Ship it!





src/resource_provider/storage/provider.cpp
Lines 3446 (patched)
<https://reviews.apache.org/r/65665/#comment286268>

operationTypes



src/resource_provider/storage/provider.cpp
Lines 3463 (patched)
<https://reviews.apache.org/r/65665/#comment286269>

Does this compile?


- Jie Yu


On May 24, 2018, 3:19 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65665/
> ---
> 
> (Updated May 24, 2018, 3:19 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Greg Mann, Jie Yu, and Jan 
> Schlicht.
> 
> 
> Bugs: MESOS-8383
> https://issues.apache.org/jira/browse/MESOS-8383
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds `operations_pending`, `operations_finished`,
> `operations_failed`, and `operations_dropped` metrics to count the
> occurances of these operation states.
> 
> Additionally, An error log in `_applyOperation()` is removed because the
> error is already logged at the call site.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> 63b5d7e5f10d6ad02b5cd11b119def3b4abf4180 
> 
> 
> Diff: https://reviews.apache.org/r/65665/diff/11/
> 
> 
> Testing
> ---
> 
> sudo make check
> A unit test is added in the next patch in chain.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>



Re: Review Request 67145: Added `linux/devices` isolator whitelist tests.

2018-05-25 Thread Jie Yu

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


Ship it!




Ship It!

- Jie Yu


On May 24, 2018, 12:28 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67145/
> ---
> 
> (Updated May 24, 2018, 12:28 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jason Lai, and Jie Yu.
> 
> 
> Bugs: MESOS-8792
> https://issues.apache.org/jira/browse/MESOS-8792
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test to verify that the `linux/devices` isolator supports
> populating devices that are whitelisted by the `allowed_devices`
> agent flag.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am c08ac6e2f5deec4d05f59f71ff6c51382f216708 
>   src/tests/CMakeLists.txt 1fef06089bc48fdc462eeef5a6bdbf28bf7e09fc 
>   src/tests/containerizer/linux_devices_isolator_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/67145/diff/7/
> 
> 
> Testing
> ---
> 
> make check (Fedora 27)
> 
> 
> Thanks,
> 
> James Peach
> 
>



Re: Review Request 67098: Updated the container launcher mount sequence.

2018-05-25 Thread Jie Yu

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


Ship it!




Ship It!

- Jie Yu


On May 24, 2018, 12:27 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67098/
> ---
> 
> (Updated May 24, 2018, 12:27 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jason Lai, and Jie Yu.
> 
> 
> Bugs: MESOS-8792
> https://issues.apache.org/jira/browse/MESOS-8792
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `linux/devices` isolator needs to make bind mounts into
> the `/dev` directory of the container. However, the container
> mounts are made before the container `/dev` is mounted as part
> of the chroot preparation. We need to prepare the chroot,
> then make any necessary container mounts, and finally enter
> the chroot. This sequence of operations also requires us to
> touch the target mount point, since we can't do it from the
> isolator because the '/dev' directory doesn't exist yet.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/launch.cpp 
> f25d90651ef32495c9161c3eaed8a327d1b2b926 
> 
> 
> Diff: https://reviews.apache.org/r/67098/diff/4/
> 
> 
> Testing
> ---
> 
> make check (Fedora 27)
> 
> 
> Thanks,
> 
> James Peach
> 
>



Re: Review Request 67291: Adjusted the tests that use nobody.

2018-05-24 Thread Jie Yu

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

(Updated May 24, 2018, 8:19 p.m.)


Review request for mesos, Andrew Schwartzmeyer and James Peach.


Changes
---

Addressed the comments.


Repository: mesos


Description
---

Used `$SUDO_USER` instead because `nobody` sometimes cannot access
direcotries under `$HOME` of the current user running the tests.


Diffs (updated)
-

  src/tests/container_logger_tests.cpp 7ec6f8701ecbc8f43ef41056fcbc17566c7db416 
  src/tests/containerizer/capabilities_tests.cpp 
734aa21c5bda19a1f8d958707249f2dc2b4de6d4 
  src/tests/containerizer/cgroups_isolator_tests.cpp 
40c18a1528f2da32c608b6c774500933a08562ea 
  src/tests/containerizer/docker_containerizer_tests.cpp 
e37a9c11fe67846ceeeb6cfbf8a75d1e5753e185 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
01f2b38cfa67b144298c361e92170322864ac201 
  src/tests/containerizer/provisioner_docker_tests.cpp 
c664ff807583d587d94b0ab797330d5d3daf7657 
  src/tests/containerizer/volume_sandbox_path_isolator_tests.cpp 
0400052a1c73ccc15c932c573cfb324ca83e2e59 
  src/tests/environment.cpp a5a255fc3e92991a201bb6cb4af9a625805395e0 
  src/tests/fetcher_tests.cpp 95359ec84ed631bdd7b4fb5192e03f68cc57ed61 
  src/tests/slave_tests.cpp 65d860594572b58a50a89358e31e97fd2a10bf08 


Diff: https://reviews.apache.org/r/67291/diff/3/

Changes: https://reviews.apache.org/r/67291/diff/2-3/


Testing
---

sudo make check


Thanks,

Jie Yu



Re: Review Request 67291: Adjusted the tests that use nobody.

2018-05-24 Thread Jie Yu


> On May 24, 2018, 7:29 p.m., Chun-Hung Hsiao wrote:
> > src/tests/environment.cpp
> > Lines 885 (patched)
> > <https://reviews.apache.org/r/67291/diff/2/?file=2028397#file2028397line885>
> >
> > Not a real issue here, but I'm thinking if just `UNPRIVILEDGED_` is 
> > clear enough. `UNPRIVILEDGED_USER_` is definitely more elaborative, but 
> > it's longer and contains a `_` which we also use as keyword deliminators 
> > for test filters. Although we already have examples like `NVEDIA_GPU_`, I 
> > feel that avoiding `_` in the matching string would also make it easier to 
> > see what filters are applyed on a test.

I was using `UNPRIVILEDGED_` initially, but found it quite confusing because 
having `ROOT_UNPRIVILEGED` is definite weird.


- Jie


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


On May 24, 2018, 5:49 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67291/
> ---
> 
> (Updated May 24, 2018, 5:49 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and James Peach.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used `$SUDO_USER` instead because `nobody` sometimes cannot access
> direcotries under `$HOME` of the current user running the tests.
> 
> 
> Diffs
> -
> 
>   src/tests/container_logger_tests.cpp 
> 7ec6f8701ecbc8f43ef41056fcbc17566c7db416 
>   src/tests/containerizer/capabilities_tests.cpp 
> 734aa21c5bda19a1f8d958707249f2dc2b4de6d4 
>   src/tests/containerizer/cgroups_isolator_tests.cpp 
> 40c18a1528f2da32c608b6c774500933a08562ea 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> e37a9c11fe67846ceeeb6cfbf8a75d1e5753e185 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 01f2b38cfa67b144298c361e92170322864ac201 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> c664ff807583d587d94b0ab797330d5d3daf7657 
>   src/tests/containerizer/volume_sandbox_path_isolator_tests.cpp 
> 0400052a1c73ccc15c932c573cfb324ca83e2e59 
>   src/tests/environment.cpp a5a255fc3e92991a201bb6cb4af9a625805395e0 
>   src/tests/fetcher_tests.cpp 95359ec84ed631bdd7b4fb5192e03f68cc57ed61 
>   src/tests/slave_tests.cpp 65d860594572b58a50a89358e31e97fd2a10bf08 
> 
> 
> Diff: https://reviews.apache.org/r/67291/diff/2/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>



Re: Review Request 67291: Adjusted the tests that use nobody.

2018-05-24 Thread Jie Yu


> On May 24, 2018, 7:29 p.m., Chun-Hung Hsiao wrote:
> > src/tests/environment.cpp
> > Lines 871 (patched)
> > <https://reviews.apache.org/r/67291/diff/2/?file=2028397#file2028397line871>
> >
> > Should we also test that `$SUDO_USER` is not `root`?

Yes! good catch!


- Jie


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


On May 24, 2018, 5:49 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67291/
> ---
> 
> (Updated May 24, 2018, 5:49 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and James Peach.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used `$SUDO_USER` instead because `nobody` sometimes cannot access
> direcotries under `$HOME` of the current user running the tests.
> 
> 
> Diffs
> -
> 
>   src/tests/container_logger_tests.cpp 
> 7ec6f8701ecbc8f43ef41056fcbc17566c7db416 
>   src/tests/containerizer/capabilities_tests.cpp 
> 734aa21c5bda19a1f8d958707249f2dc2b4de6d4 
>   src/tests/containerizer/cgroups_isolator_tests.cpp 
> 40c18a1528f2da32c608b6c774500933a08562ea 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> e37a9c11fe67846ceeeb6cfbf8a75d1e5753e185 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 01f2b38cfa67b144298c361e92170322864ac201 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> c664ff807583d587d94b0ab797330d5d3daf7657 
>   src/tests/containerizer/volume_sandbox_path_isolator_tests.cpp 
> 0400052a1c73ccc15c932c573cfb324ca83e2e59 
>   src/tests/environment.cpp a5a255fc3e92991a201bb6cb4af9a625805395e0 
>   src/tests/fetcher_tests.cpp 95359ec84ed631bdd7b4fb5192e03f68cc57ed61 
>   src/tests/slave_tests.cpp 65d860594572b58a50a89358e31e97fd2a10bf08 
> 
> 
> Diff: https://reviews.apache.org/r/67291/diff/2/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>



Re: Review Request 67291: Adjusted the tests that use nobody.

2018-05-24 Thread Jie Yu


> On May 24, 2018, 7:29 p.m., Chun-Hung Hsiao wrote:
> > It seems to me that most of the modified tests work just fine with 
> > `nobody`. For example, the `FetcherTest` and `CapabilitiesTest` don't need 
> > an unpriviledged user that can access test launcher's home. Are we 
> > advocating a new pattern here and want to avoid `nobody` for all tests?

using `nobody` is weird because it does not necessarily exist. Using $SUDO_USER 
is better in that case. I think we should avoid using nobody in all our tests.


- Jie


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


On May 24, 2018, 5:49 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67291/
> ---
> 
> (Updated May 24, 2018, 5:49 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and James Peach.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used `$SUDO_USER` instead because `nobody` sometimes cannot access
> direcotries under `$HOME` of the current user running the tests.
> 
> 
> Diffs
> -
> 
>   src/tests/container_logger_tests.cpp 
> 7ec6f8701ecbc8f43ef41056fcbc17566c7db416 
>   src/tests/containerizer/capabilities_tests.cpp 
> 734aa21c5bda19a1f8d958707249f2dc2b4de6d4 
>   src/tests/containerizer/cgroups_isolator_tests.cpp 
> 40c18a1528f2da32c608b6c774500933a08562ea 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> e37a9c11fe67846ceeeb6cfbf8a75d1e5753e185 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 01f2b38cfa67b144298c361e92170322864ac201 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> c664ff807583d587d94b0ab797330d5d3daf7657 
>   src/tests/containerizer/volume_sandbox_path_isolator_tests.cpp 
> 0400052a1c73ccc15c932c573cfb324ca83e2e59 
>   src/tests/environment.cpp a5a255fc3e92991a201bb6cb4af9a625805395e0 
>   src/tests/fetcher_tests.cpp 95359ec84ed631bdd7b4fb5192e03f68cc57ed61 
>   src/tests/slave_tests.cpp 65d860594572b58a50a89358e31e97fd2a10bf08 
> 
> 
> Diff: https://reviews.apache.org/r/67291/diff/2/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>



Re: Review Request 67291: Adjusted the tests that use nobody.

2018-05-24 Thread Jie Yu

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

(Updated May 24, 2018, 5:49 p.m.)


Review request for mesos, Andrew Schwartzmeyer and James Peach.


Changes
---

Used os::getenv instead.


Repository: mesos


Description
---

Used `$SUDO_USER` instead because `nobody` sometimes cannot access
direcotries under `$HOME` of the current user running the tests.


Diffs (updated)
-

  src/tests/container_logger_tests.cpp 7ec6f8701ecbc8f43ef41056fcbc17566c7db416 
  src/tests/containerizer/capabilities_tests.cpp 
734aa21c5bda19a1f8d958707249f2dc2b4de6d4 
  src/tests/containerizer/cgroups_isolator_tests.cpp 
40c18a1528f2da32c608b6c774500933a08562ea 
  src/tests/containerizer/docker_containerizer_tests.cpp 
e37a9c11fe67846ceeeb6cfbf8a75d1e5753e185 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
01f2b38cfa67b144298c361e92170322864ac201 
  src/tests/containerizer/provisioner_docker_tests.cpp 
c664ff807583d587d94b0ab797330d5d3daf7657 
  src/tests/containerizer/volume_sandbox_path_isolator_tests.cpp 
0400052a1c73ccc15c932c573cfb324ca83e2e59 
  src/tests/environment.cpp a5a255fc3e92991a201bb6cb4af9a625805395e0 
  src/tests/fetcher_tests.cpp 95359ec84ed631bdd7b4fb5192e03f68cc57ed61 
  src/tests/slave_tests.cpp 65d860594572b58a50a89358e31e97fd2a10bf08 


Diff: https://reviews.apache.org/r/67291/diff/2/

Changes: https://reviews.apache.org/r/67291/diff/1-2/


Testing
---

sudo make check


Thanks,

Jie Yu



  1   2   3   4   5   6   7   8   9   10   >