Re: Review Request 67965: Optimized ranges subtraction operation.

[Meng Zhu]

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

(Updated July 23, 2018, 10:29 p.m.)


Review request for mesos, Benjamin Mahler, Gastón Kleiman, and Vinod Kone.


Changes
---

- Refactored the code for better readability;
- Added tests for subtracting the empty case;
- Added changes to v1 as well.


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


Repository: mesos


Description
---

The current ranges subtraction uses boost IntervalSet.
Based on the profiling result of MESOS-8989, the ranges
subtraction operation is about 2~3 times more expensive
than that of addition. The conversion cost from Ranges
to IntervalSet may be the culprit.

This patch optimizes the ranges subtraction operation by
converting Ranges to a vector of internal sub-ranges, sorting
the vector based on sub-range start and then applying a
one-pass algorithm similar to that of addition.


Diffs (updated)
-

  src/common/values.cpp afe4137f82115dd4ec9967b5eba16a1dd15401c8 
  src/tests/values_tests.cpp 2f5abedfd461c114d03f5e941d81ebe414188033 
  src/v1/values.cpp d2c31f6c91998382dec1d8834b40613013716cdd 


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

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


Testing
---

make check

Benchmarking:

tl:dr: more than 80% performance improvment across board. Performance of 
subtraction is now on par or better than the addition, especially when there 
are a large number of sub-ranges.

Build with -O2 optimization, ran on a multicore machine with peak frequency at 
2.2GHz:

Took 1.617706ms to perform 1000 'a += b' operations on ports:[1-6, 11-16, 
21-26...91-96] and ports:[3-8, 13-18, 23-28..., 93-98] with 10 sub-ranges
Took 1.607999ms to perform 1000 'a -= b' operations on ports:[1-6, 11-16, 
21-26...91-96] and ports:[3-8, 13-18, 23-28..., 93-98] with 10 sub-ranges
Took 3.094113ms to perform 1000 'a + b' operations on ports:[1-6, 11-16, 
21-26...91-96] and ports:[3-8, 13-18, 23-28..., 93-98] with 10 sub-ranges
Took 3.152291ms to perform 1000 'a - b' operations on ports:[1-6, 11-16, 
21-26...91-96] and ports:[3-8, 13-18, 23-28..., 93-98] with 10 sub-ranges

Took 14.908924ms to perform 1000 'a += b' operations on ports:[1-6, 11-16, 
21-26...991-996] and ports:[3-8, 13-18, 23-28..., 993-998] with 100 sub-ranges
Took 13.564767ms to perform 1000 'a -= b' operations on ports:[1-6, 11-16, 
21-26...991-996] and ports:[3-8, 13-18, 23-28..., 993-998] with 100 sub-ranges
Took 25.523443ms to perform 1000 'a + b' operations on ports:[1-6, 11-16, 
21-26...991-996] and ports:[3-8, 13-18, 23-28..., 993-998] with 100 sub-ranges
Took 24.871216ms to perform 1000 'a - b' operations on ports:[1-6, 11-16, 
21-26...991-996] and ports:[3-8, 13-18, 23-28..., 993-998] with 100 sub-ranges

Took 234.22483ms to perform 1000 'a += b' operations on ports:[1-6, 11-16, 
21-26...9991-9996] and ports:[3-8, 13-18, 23-28..., 9993-9998] with 1000 
sub-ranges
Took 144.417381ms to perform 1000 'a -= b' operations on ports:[1-6, 11-16, 
21-26...9991-9996] and ports:[3-8, 13-18, 23-28..., 9993-9998] with 1000 
sub-ranges
Took 322.548021ms to perform 1000 'a + b' operations on ports:[1-6, 11-16, 
21-26...9991-9996] and ports:[3-8, 13-18, 23-28..., 9993-9998] with 1000 
sub-ranges
Took 227.835441ms to perform 1000 'a - b' operations on ports:[1-6, 11-16, 
21-26...9991-9996] and ports:[3-8, 13-18, 23-28..., 9993-9998] with 1000 
sub-ranges


Thanks,

Meng Zhu

Re: Review Request 68022: Enabled Seccomp filter in the containerizer launcher. (WIP)

[Mesos Reviewbot]

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



Bad patch!

Reviews applied: [68022, 68021, 68020, 68019, 68018, 68017, 68016, 67844]

Failed command: python support/apply-reviews.py -n -r 67844

Error:
The support scripts will be upgraded to Python 3 by July 1st.
Make sure to install Python 3.6 on your machine before.
2018-07-24 04:42:25 URL:https://reviews.apache.org/r/67844/diff/raw/ [260/260] 
-> "67844.patch" [1]
error: missing binary patch data for '3rdparty/libseccomp-2.3.3.tar.gz'
error: binary patch does not apply to '3rdparty/libseccomp-2.3.3.tar.gz'
error: 3rdparty/libseccomp-2.3.3.tar.gz: patch does not apply

Full log: https://builds.apache.org/job/Mesos-Reviewbot/22911/console

- Mesos Reviewbot


On July 23, 2018, 1:57 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68022/
> ---
> 
> (Updated July 23, 2018, 1:57 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, James Peach, and Qian Zhang.
> 
> 
> Bugs: MESOS-9106
> https://issues.apache.org/jira/browse/MESOS-9106
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Containerizer launcher creates an instance of `SeccompFilter`, which is
> used to setup Seccomp profile using `ContainerSeccompProfile` message
> prepared by the `linux/seccomp` isolator. The Seccomp filter is loaded
> right before calling `execve()`, so that a container will be running
> with a syscall filtering enabled.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/launch.cpp 
> 7193da0a094df3e441e185c62b3a0379a0bdc4a2 
> 
> 
> Diff: https://reviews.apache.org/r/68022/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 67916: Patched Google Test with upstream bugfix.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [67916]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose --disable-libtool-wrappers' ENVIRONMENT='GLOG_v=1 
MESOS_VERBOSE=1'; ./support/docker-build.sh

- Mesos Reviewbot


On July 13, 2018, 9:04 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67916/
> ---
> 
> (Updated July 13, 2018, 9:04 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Joseph Wu, and Till Toenshoff.
> 
> 
> Bugs: MESOS-8990
> https://issues.apache.org/jira/browse/MESOS-8990
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Per MESOS-8990, our Google Test dependency needs a patch from
> upstream, https://github.com/google/googletest/pull/1620, in order to
> continue building with the next version of MSVC (and potentially other
> compilers).
> 
> This patch file was generated by cherry-picking `f66ab00` from
> `master` onto `release-1.8.0` in the Google Test repo, and resolving
> the merge conflict.
> 
> 
> Diffs
> -
> 
>   3rdparty/CMakeLists.txt b58996d2ed7521e42d6298d174cc8c494b84eb8f 
>   3rdparty/googletest-release-1.8.0.patch PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/67916/diff/1/
> 
> 
> Testing
> ---
> 
> Clean build on Windows using CMake (which is the only place this patch 
> currently applies). It is not yet known if the Autotools system will also 
> need this patch. Do we want it added there anyway?
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 68029: Updated XFS disk isolator docs.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 68029 was successfully built and tested.

Reviews applied: `['67914', '68029']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/1970/mesos-review-68029

- Mesos Reviewbot Windows


On July 24, 2018, 12:45 a.m., Ilya Pronin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68029/
> ---
> 
> (Updated July 24, 2018, 12:45 a.m.)
> 
> 
> Review request for mesos and James Peach.
> 
> 
> Bugs: MESOS-9007
> https://issues.apache.org/jira/browse/MESOS-9007
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added project IDs reclaiming mechanism description.
> 
> 
> Diffs
> -
> 
>   docs/isolators/disk-xfs.md 96bb39ec51bfdc4b297099baf1a4cef3efe2c92f 
> 
> 
> Diff: https://reviews.apache.org/r/68029/diff/2/
> 
> 
> Testing
> ---
> 
> N/A.
> 
> 
> Thanks,
> 
> Ilya Pronin
> 
>

Re: Review Request 67891: Added multi-framework scalability guidelines to the documentation.

[Benjamin Mahler]

> On July 20, 2018, 3:49 p.m., Vinod Kone wrote:
> > docs/app-framework-development-guide.md
> > Lines 55 (patched)
> > 
> >
> > Do we want to give any guidance on what values we considere large? 
> > Otherwise, it might add more confusion than necessary?

Added an example timeout of 1 hour, also added a 4th guideline about not 
reviving frequently.


- Benjamin


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


On July 13, 2018, 9:33 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67891/
> ---
> 
> (Updated July 13, 2018, 9:33 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Greg Mann, Meng Zhu, and Vinod Kone.
> 
> 
> Bugs: MESOS-8238
> https://issues.apache.org/jira/browse/MESOS-8238
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Running multiple frameworks requires co-operation from framework
> developers; this outlines some initial guidelines that should be
> followed to ensure running multiple frameworks together performs
> well.
> 
> 
> Diffs
> -
> 
>   docs/app-framework-development-guide.md 
> 035ac1f80e063c75786845475d573c1ee03570c0 
> 
> 
> Diff: https://reviews.apache.org/r/67891/diff/2/
> 
> 
> Testing
> ---
> 
> N/A
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 67914: Updated XFS disk isolator to reclaim project IDs after disk GC.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 67914 was successfully built and tested.

Reviews applied: `['67914']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/1969/mesos-review-67914

- Mesos Reviewbot Windows


On July 23, 2018, 11:21 p.m., Ilya Pronin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67914/
> ---
> 
> (Updated July 23, 2018, 11:21 p.m.)
> 
> 
> Review request for mesos and James Peach.
> 
> 
> Bugs: MESOS-9007
> https://issues.apache.org/jira/browse/MESOS-9007
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently upon container destruction its project ID is unallocated by
> the isolator and removed from the container work directory. However due
> to API limitations we can't unset project IDs on symlinks that may exist
> inside the directory. Because of that the project may still exist until
> the container directory is garbage collected. If the project ID is
> reused for a new container, any lingering symlinks that still have that
> project ID will contribute to disk usage of the new container. Typically
> symlinks don't take much space, but still this leads to inaccuracy in
> disk space usage accounting.
> 
> This patch postpones project ID reclaiming until sandbox GC time. The
> isolator periodically checks if sandboxes of terminated containers still
> exist and deallocates project IDs of the ones that were removed. Checks
> are performed every "disk_watch_interval". This mechanism can be
> improved in the future if we introduce a way for the isolators to learn
> about disk GCs.
> 
> Current number of available project IDs can be tracked with the new
> "containerizer/mesos/disk/project_ids_free" and
> "containerizer/mesos/disk/project_ids_total" metrics.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/xfs/disk.hpp 
> 0891f7709aa4f98758a727856d58e6177d46adca 
>   src/slave/containerizer/mesos/isolators/xfs/disk.cpp 
> 25f52a43b34b141bdaf7c448817423cf4264e22a 
>   src/tests/containerizer/xfs_quota_tests.cpp 
> dc18a8a59d1eb7fae3592ef6ba8c046e4f46ee4a 
> 
> 
> Diff: https://reviews.apache.org/r/67914/diff/4/
> 
> 
> Testing
> ---
> 
> Added `ROOT_XFS_QuotaTest.ProjectIDReclaiming` test that verifies that 
> project ID is reclaimed and reused after sandbox GC. Ran `sudo make check`.
> 
> 
> Thanks,
> 
> Ilya Pronin
> 
>

Re: Review Request 68029: Updated XFS disk isolator docs.

[Ilya Pronin]

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

(Updated July 23, 2018, 5:45 p.m.)


Review request for mesos and James Peach.


Changes
---

Fixed grammar.


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


Repository: mesos


Description
---

Added project IDs reclaiming mechanism description.


Diffs (updated)
-

  docs/isolators/disk-xfs.md 96bb39ec51bfdc4b297099baf1a4cef3efe2c92f 


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

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


Testing
---

N/A.


Thanks,

Ilya Pronin

Review Request 68029: Updated XFS disk isolator docs.

[Ilya Pronin]

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

Review request for mesos and James Peach.


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


Repository: mesos


Description
---

Added project IDs reclaiming mechanism description.


Diffs
-

  docs/isolators/disk-xfs.md 96bb39ec51bfdc4b297099baf1a4cef3efe2c92f 


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


Testing
---

N/A.


Thanks,

Ilya Pronin

Re: Review Request 67914: Updated XFS disk isolator to reclaim project IDs after disk GC.

[Ilya Pronin]

> On July 18, 2018, 1:51 p.m., James Peach wrote:
> > src/slave/flags.cpp
> > Lines 1338 (patched)
> > 
> >
> > Rather than introducing a new flag, lets use 
> > `container_disk_watch_interval` or `disk_watch_interval` ... probably the 
> > former?
> 
> Ilya Pronin wrote:
> I think if we do this then we should rather use the latter because disk 
> GC is driven by `disk_watch_interval` and `gc_delay` (maybe use which one of 
> those 2 is the smallest?). `container_disk_watch_interval` will most likely 
> be set to a smaller value to promptly terminate containers that have breached 
> their quota (that's true in our cluster). Running sandboxes check at that 
> frequently would be wasteful.
> 
> James Peach wrote:
> OK, `disk_watch_interval` works for me too.

Done.


- Ilya


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


On July 23, 2018, 4:21 p.m., Ilya Pronin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67914/
> ---
> 
> (Updated July 23, 2018, 4:21 p.m.)
> 
> 
> Review request for mesos and James Peach.
> 
> 
> Bugs: MESOS-9007
> https://issues.apache.org/jira/browse/MESOS-9007
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently upon container destruction its project ID is unallocated by
> the isolator and removed from the container work directory. However due
> to API limitations we can't unset project IDs on symlinks that may exist
> inside the directory. Because of that the project may still exist until
> the container directory is garbage collected. If the project ID is
> reused for a new container, any lingering symlinks that still have that
> project ID will contribute to disk usage of the new container. Typically
> symlinks don't take much space, but still this leads to inaccuracy in
> disk space usage accounting.
> 
> This patch postpones project ID reclaiming until sandbox GC time. The
> isolator periodically checks if sandboxes of terminated containers still
> exist and deallocates project IDs of the ones that were removed. Checks
> are performed every "disk_watch_interval". This mechanism can be
> improved in the future if we introduce a way for the isolators to learn
> about disk GCs.
> 
> Current number of available project IDs can be tracked with the new
> "containerizer/mesos/disk/project_ids_free" and
> "containerizer/mesos/disk/project_ids_total" metrics.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/xfs/disk.hpp 
> 0891f7709aa4f98758a727856d58e6177d46adca 
>   src/slave/containerizer/mesos/isolators/xfs/disk.cpp 
> 25f52a43b34b141bdaf7c448817423cf4264e22a 
>   src/tests/containerizer/xfs_quota_tests.cpp 
> dc18a8a59d1eb7fae3592ef6ba8c046e4f46ee4a 
> 
> 
> Diff: https://reviews.apache.org/r/67914/diff/4/
> 
> 
> Testing
> ---
> 
> Added `ROOT_XFS_QuotaTest.ProjectIDReclaiming` test that verifies that 
> project ID is reclaimed and reused after sandbox GC. Ran `sudo make check`.
> 
> 
> Thanks,
> 
> Ilya Pronin
> 
>

Re: Review Request 67914: Updated XFS disk isolator to reclaim project IDs after disk GC.

[Ilya Pronin]

> On July 23, 2018, 3:22 p.m., James Peach wrote:
> > src/slave/containerizer/mesos/isolators/xfs/disk.hpp
> > Lines 24 (patched)
> > 
> >
> > Let's use a pull gauge here for sampling performance reasons.
> > 
> > We can decrement it in `recover` and `nextProjectId`, and increment it 
> > in `returnProjectId`.
> 
> James Peach wrote:
> *push gauge* :)

Done.


> On July 23, 2018, 3:22 p.m., James Peach wrote:
> > src/slave/containerizer/mesos/isolators/xfs/disk.hpp
> > Lines 124 (patched)
> > 
> >
> > Shall we add a `project_ids_total` as well?

Done.


- Ilya


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


On July 23, 2018, 4:21 p.m., Ilya Pronin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67914/
> ---
> 
> (Updated July 23, 2018, 4:21 p.m.)
> 
> 
> Review request for mesos and James Peach.
> 
> 
> Bugs: MESOS-9007
> https://issues.apache.org/jira/browse/MESOS-9007
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently upon container destruction its project ID is unallocated by
> the isolator and removed from the container work directory. However due
> to API limitations we can't unset project IDs on symlinks that may exist
> inside the directory. Because of that the project may still exist until
> the container directory is garbage collected. If the project ID is
> reused for a new container, any lingering symlinks that still have that
> project ID will contribute to disk usage of the new container. Typically
> symlinks don't take much space, but still this leads to inaccuracy in
> disk space usage accounting.
> 
> This patch postpones project ID reclaiming until sandbox GC time. The
> isolator periodically checks if sandboxes of terminated containers still
> exist and deallocates project IDs of the ones that were removed. Checks
> are performed every "disk_watch_interval". This mechanism can be
> improved in the future if we introduce a way for the isolators to learn
> about disk GCs.
> 
> Current number of available project IDs can be tracked with the new
> "containerizer/mesos/disk/project_ids_free" and
> "containerizer/mesos/disk/project_ids_total" metrics.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/xfs/disk.hpp 
> 0891f7709aa4f98758a727856d58e6177d46adca 
>   src/slave/containerizer/mesos/isolators/xfs/disk.cpp 
> 25f52a43b34b141bdaf7c448817423cf4264e22a 
>   src/tests/containerizer/xfs_quota_tests.cpp 
> dc18a8a59d1eb7fae3592ef6ba8c046e4f46ee4a 
> 
> 
> Diff: https://reviews.apache.org/r/67914/diff/4/
> 
> 
> Testing
> ---
> 
> Added `ROOT_XFS_QuotaTest.ProjectIDReclaiming` test that verifies that 
> project ID is reclaimed and reused after sandbox GC. Ran `sudo make check`.
> 
> 
> Thanks,
> 
> Ilya Pronin
> 
>

Re: Review Request 67914: Updated XFS disk isolator to reclaim project IDs after disk GC.

[Ilya Pronin]

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

(Updated July 23, 2018, 4:21 p.m.)


Review request for mesos and James Peach.


Changes
---

Addressed review comments.


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


Repository: mesos


Description (updated)
---

Currently upon container destruction its project ID is unallocated by
the isolator and removed from the container work directory. However due
to API limitations we can't unset project IDs on symlinks that may exist
inside the directory. Because of that the project may still exist until
the container directory is garbage collected. If the project ID is
reused for a new container, any lingering symlinks that still have that
project ID will contribute to disk usage of the new container. Typically
symlinks don't take much space, but still this leads to inaccuracy in
disk space usage accounting.

This patch postpones project ID reclaiming until sandbox GC time. The
isolator periodically checks if sandboxes of terminated containers still
exist and deallocates project IDs of the ones that were removed. Checks
are performed every "disk_watch_interval". This mechanism can be
improved in the future if we introduce a way for the isolators to learn
about disk GCs.

Current number of available project IDs can be tracked with the new
"containerizer/mesos/disk/project_ids_free" and
"containerizer/mesos/disk/project_ids_total" metrics.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/xfs/disk.hpp 
0891f7709aa4f98758a727856d58e6177d46adca 
  src/slave/containerizer/mesos/isolators/xfs/disk.cpp 
25f52a43b34b141bdaf7c448817423cf4264e22a 
  src/tests/containerizer/xfs_quota_tests.cpp 
dc18a8a59d1eb7fae3592ef6ba8c046e4f46ee4a 


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

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


Testing
---

Added `ROOT_XFS_QuotaTest.ProjectIDReclaiming` test that verifies that project 
ID is reclaimed and reused after sandbox GC. Ran `sudo make check`.


Thanks,

Ilya Pronin

Re: Review Request 67827: Added a helper `stripIncapableResources` in the allocator.

[Benjamin Mahler]

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


Ship it!





src/master/allocator/mesos/hierarchical.hpp
Lines 685 (patched)


const Resources& per comment below



src/master/allocator/mesos/hierarchical.cpp
Lines 1812-1813 (original), 1812-1813 (patched)


What is "normal offer behavior"? In this patch we can just adjust the 
comment to reflect that we're not doing an explicit copy anymore, and if we 
want to document *why* the current approach is used, we can do that in a 
separate patch after hearing from Yan?



src/master/allocator/mesos/hierarchical.cpp
Lines 2044-2047 (original), 2024-2027 (patched)


Ditto here.



src/master/allocator/mesos/hierarchical.cpp
Lines 2689 (patched)


This can be `const Resources&` to avoid a copy?


- Benjamin Mahler


On July 20, 2018, 11:30 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67827/
> ---
> 
> (Updated July 20, 2018, 11:30 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9104
> https://issues.apache.org/jira/browse/MESOS-9104
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This helper removes any resources that the framework is not
> capable of receiving based on the given framework capability.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 702e7c0aa84b4b672d82c759c25a28a77c78ad50 
>   src/master/allocator/mesos/hierarchical.cpp 
> 707dd6bd0f255a64d759ce87cbf75be57d86b392 
> 
> 
> Diff: https://reviews.apache.org/r/67827/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 67826: Made `Slave::getAvailable()` return all shared resources.

[Benjamin Mahler]

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



Can you clarify why this commit doesn't touch the shared resources logic in the 
allocation loop?
https://github.com/apache/mesos/blob/1.6.0/src/master/allocator/mesos/hierarchical.cpp#L2070-L2079

Does this patch pass the tests without the next patch applied? Even so, it 
seems like this patch should tidy the shared resources logic up in the 
allocation loop now that the old assumption (doesn't show up in 
`getAvailable()`) isn't true anymore?

Perhaps the following would be a bit simpler to follow, since the optimization 
seems to complicate things a bit:

Patch 1: Move shared resources logic from the allocation loop into the agent 
struct.
Patch 2: Optimize `Slave::updateAvailable` at the expense of `Slave` 
construction and `Slave::updateTotal`?


src/master/allocator/mesos/hierarchical.hpp
Line 464 (original), 471-476 (patched)


How about:

```
// Calling `nonShared()` currently copies the underlying resources
// and is therefore rather expensive. We avoid it in the common
// case that there are no shared resources.
//
// TODO(mzhu): Ideally there would be a single logical path here.
// One solution is to have Resources be copy-on-write such that
// `nonShared()` performs no copying and instead points to a
// subset of the original `Resource` objects.
```



src/master/allocator/mesos/hierarchical.hpp
Lines 474-476 (patched)


I was going to say this warrants a comment about there always being a copy 
of the shared resources available but I noticed its down below on the member 
variable.

Per the comment at the top level of the review, shouldn't the comment/logic 
in the allocation loop be migrating here in this patch?



src/master/allocator/mesos/hierarchical.hpp
Lines 509-511 (patched)


s/traversals/copying/ ?

Isn't the optimization also being done in the Slave constructor? Maybe we 
don't need to say exactly which functions the optimization is done?


- Benjamin Mahler


On July 20, 2018, 11:30 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67826/
> ---
> 
> (Updated July 20, 2018, 11:30 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-9104
> https://issues.apache.org/jira/browse/MESOS-9104
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, depending on already allocated resources,
> `HierarchicalAllocatorProcess::Slave::getAvailable()`
> may not contain all the shared resources.
> Thus it does not accurately reflect what is available.
> 
> Since shared resources are always allocatable, we should
> include all shared resources in the agent available resources.
> This would help remove the one off logic for removing and
> adding back shared resources in the allocation loop.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 702e7c0aa84b4b672d82c759c25a28a77c78ad50 
> 
> 
> Diff: https://reviews.apache.org/r/67826/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 67945: Made getters in the Slave class in the allocator return references.

[Benjamin Mahler]

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


Ship it!




Ship It!

- Benjamin Mahler


On July 20, 2018, 11:29 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67945/
> ---
> 
> (Updated July 20, 2018, 11:29 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This eliminates some unnecessary copies.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 702e7c0aa84b4b672d82c759c25a28a77c78ad50 
> 
> 
> Diff: https://reviews.apache.org/r/67945/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 67777: Added a helper to match agent-framework capabilities in the allocator.

[Benjamin Mahler]

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


Ship it!




Looks good! Will get this committed shortly.


src/master/allocator/mesos/hierarchical.cpp
Lines 2805 (patched)


Rogue newline?


- Benjamin Mahler


On July 20, 2018, 11:28 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6/
> ---
> 
> (Updated July 20, 2018, 11:28 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9104
> https://issues.apache.org/jira/browse/MESOS-9104
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `isCapableOfReceivingAgent*(` checks if a framework
> is capable of receiving resources on the agent based on
> the framework capability.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 702e7c0aa84b4b672d82c759c25a28a77c78ad50 
>   src/master/allocator/mesos/hierarchical.cpp 
> 707dd6bd0f255a64d759ce87cbf75be57d86b392 
> 
> 
> Diff: https://reviews.apache.org/r/6/diff/5/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 67980: Windows: Enabled rest of `ProcessTest` suite.

[John Kordich via Review Board]

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


Ship it!




Ship It!

- John Kordich


On July 19, 2018, 8:47 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67980/
> ---
> 
> (Updated July 19, 2018, 8:47 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Eric Mumau, John Kordich, Joseph Wu, 
> Liangyu Zhao, and Radhika Jandhyala.
> 
> 
> Bugs: MESOS-5817 and MESOS-7527
> https://issues.apache.org/jira/browse/MESOS-5817
> https://issues.apache.org/jira/browse/MESOS-7527
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Several of these worked as-is, and the two firewall tests were fixed
> by replace `path::join` (which joined with ``) with
> `strings::join("/")`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> 8baf60d8bbb675e26fea5e76c825ef73fedbc629 
> 
> 
> Diff: https://reviews.apache.org/r/67980/diff/1/
> 
> 
> Testing
> ---
> 
> `ctest -V` on Windows and Ubuntu 18.04
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 67983: Enabled `TimeTest.Now` on Windows.

[John Kordich via Review Board]

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


Ship it!




Ship It!

- John Kordich


On July 20, 2018, 5:19 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67983/
> ---
> 
> (Updated July 20, 2018, 5:19 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Eric Mumau, John Kordich, Joseph Wu, 
> Liangyu Zhao, and Radhika Jandhyala.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> While the timer resolution on Windows does not support 10
> microseconds, it does support 1000 microseconds, which is still fast
> enough for a unit test.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/tests/time_tests.cpp 
> 08ddb56f1789f400b8cd072c53e885c759f13ddc 
> 
> 
> Diff: https://reviews.apache.org/r/67983/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 67979: Windows: Documented why the `RemoteLinkLeak` test is not enabled.

[John Kordich via Review Board]

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


Ship it!




Ship It!

- John Kordich


On July 19, 2018, 8:46 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67979/
> ---
> 
> (Updated July 19, 2018, 8:46 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Eric Mumau, John Kordich, Joseph Wu, 
> Liangyu Zhao, and Radhika Jandhyala.
> 
> 
> Bugs: MESOS-9093
> https://issues.apache.org/jira/browse/MESOS-9093
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Windows: Documented why the `RemoteLinkLeak` test is not enabled.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> 8baf60d8bbb675e26fea5e76c825ef73fedbc629 
> 
> 
> Diff: https://reviews.apache.org/r/67979/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 67978: Windows: Enabled `RemoteLink` tests.

[John Kordich via Review Board]

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


Ship it!




Ship It!

- John Kordich


On July 19, 2018, 8:44 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67978/
> ---
> 
> (Updated July 19, 2018, 8:44 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Eric Mumau, John Kordich, Joseph Wu, 
> Liangyu Zhao, and Radhika Jandhyala.
> 
> 
> Bugs: MESOS-5941
> https://issues.apache.org/jira/browse/MESOS-5941
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This resolves MESOS-5941. Note that on Windows, `os::killtree()` is
> only valid for processes specifically spawned in a group via a job
> object, otherwise it will fail (and the failure is not being checked
> here). Since the `linkee` process only ever spawns the single OS
> process `test-linkee`, it is safe to switch to `os::kill()` which can
> correctly kill it.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> 8baf60d8bbb675e26fea5e76c825ef73fedbc629 
> 
> 
> Diff: https://reviews.apache.org/r/67978/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 67914: Updated XFS disk isolator to reclaim project IDs after disk GC.

[James Peach]

> On July 23, 2018, 10:22 p.m., James Peach wrote:
> > src/slave/containerizer/mesos/isolators/xfs/disk.hpp
> > Lines 24 (patched)
> > 
> >
> > Let's use a pull gauge here for sampling performance reasons.
> > 
> > We can decrement it in `recover` and `nextProjectId`, and increment it 
> > in `returnProjectId`.

*push gauge* :)


- James


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


On July 19, 2018, 11:12 p.m., Ilya Pronin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67914/
> ---
> 
> (Updated July 19, 2018, 11:12 p.m.)
> 
> 
> Review request for mesos and James Peach.
> 
> 
> Bugs: MESOS-9007
> https://issues.apache.org/jira/browse/MESOS-9007
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently upon container destruction its project ID is unallocated by
> the isolator and removed from the container work directory. However due
> to API limitations we can't unset project IDs on symlinks that may exist
> inside the directory. Because of that the project may still exist until
> the container directory is garbage collected. If the project ID is
> reused for a new container, any lingering symlinks that still have that
> project ID will contribute to disk usage of the new container. Typically
> symlinks don't take much space, but still this leads to inaccuracy in
> disk space usage accounting.
> 
> This patch postpones project ID reclaiming until sandbox GC time. The
> isolator periodically checks if sandboxes of terminated containers still
> exist and deallocates project IDs of the ones that were removed. This
> mechanism can be improved in the future if we introduce a way for the
> isolators to learn about disk GCs.
> 
> Current number of available project IDs can be tracked with the new
> "containerizer/mesos/disk/project_ids_free" metric.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/xfs/disk.hpp 
> 0891f7709aa4f98758a727856d58e6177d46adca 
>   src/slave/containerizer/mesos/isolators/xfs/disk.cpp 
> 25f52a43b34b141bdaf7c448817423cf4264e22a 
>   src/slave/flags.hpp eeb9708f9ec76d83b6719541f4a012544c7c0cbe 
>   src/slave/flags.cpp 58cdc0f1100fe244e5bf1036e1ccf39478d5d478 
>   src/tests/containerizer/xfs_quota_tests.cpp 
> dc18a8a59d1eb7fae3592ef6ba8c046e4f46ee4a 
> 
> 
> Diff: https://reviews.apache.org/r/67914/diff/3/
> 
> 
> Testing
> ---
> 
> Added `ROOT_XFS_QuotaTest.ProjectIDReclaiming` test that verifies that 
> project ID is reclaimed and reused after sandbox GC. Ran `sudo make check`.
> 
> 
> Thanks,
> 
> Ilya Pronin
> 
>

Re: Review Request 67914: Updated XFS disk isolator to reclaim project IDs after disk GC.

[James Peach]

> On July 18, 2018, 8:51 p.m., James Peach wrote:
> > src/slave/containerizer/mesos/isolators/xfs/disk.cpp
> > Lines 549 (patched)
> > 
> >
> > If possible, I think this is simple enough to inline into the check loop
> 
> Ilya Pronin wrote:
> Definitely possible, but I would prefer to keep that logical separation 
> unless you feel strongly about that :) `loop()` schedules the periodic check 
> and `checkProjectIdUsage()` actually performs that check.

Yeh I live with that :)


> On July 18, 2018, 8:51 p.m., James Peach wrote:
> > src/slave/flags.cpp
> > Lines 1338 (patched)
> > 
> >
> > Rather than introducing a new flag, lets use 
> > `container_disk_watch_interval` or `disk_watch_interval` ... probably the 
> > former?
> 
> Ilya Pronin wrote:
> I think if we do this then we should rather use the latter because disk 
> GC is driven by `disk_watch_interval` and `gc_delay` (maybe use which one of 
> those 2 is the smallest?). `container_disk_watch_interval` will most likely 
> be set to a smaller value to promptly terminate containers that have breached 
> their quota (that's true in our cluster). Running sandboxes check at that 
> frequently would be wasteful.

OK, `disk_watch_interval` works for me too.


- James


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


On July 19, 2018, 11:12 p.m., Ilya Pronin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67914/
> ---
> 
> (Updated July 19, 2018, 11:12 p.m.)
> 
> 
> Review request for mesos and James Peach.
> 
> 
> Bugs: MESOS-9007
> https://issues.apache.org/jira/browse/MESOS-9007
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently upon container destruction its project ID is unallocated by
> the isolator and removed from the container work directory. However due
> to API limitations we can't unset project IDs on symlinks that may exist
> inside the directory. Because of that the project may still exist until
> the container directory is garbage collected. If the project ID is
> reused for a new container, any lingering symlinks that still have that
> project ID will contribute to disk usage of the new container. Typically
> symlinks don't take much space, but still this leads to inaccuracy in
> disk space usage accounting.
> 
> This patch postpones project ID reclaiming until sandbox GC time. The
> isolator periodically checks if sandboxes of terminated containers still
> exist and deallocates project IDs of the ones that were removed. This
> mechanism can be improved in the future if we introduce a way for the
> isolators to learn about disk GCs.
> 
> Current number of available project IDs can be tracked with the new
> "containerizer/mesos/disk/project_ids_free" metric.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/xfs/disk.hpp 
> 0891f7709aa4f98758a727856d58e6177d46adca 
>   src/slave/containerizer/mesos/isolators/xfs/disk.cpp 
> 25f52a43b34b141bdaf7c448817423cf4264e22a 
>   src/slave/flags.hpp eeb9708f9ec76d83b6719541f4a012544c7c0cbe 
>   src/slave/flags.cpp 58cdc0f1100fe244e5bf1036e1ccf39478d5d478 
>   src/tests/containerizer/xfs_quota_tests.cpp 
> dc18a8a59d1eb7fae3592ef6ba8c046e4f46ee4a 
> 
> 
> Diff: https://reviews.apache.org/r/67914/diff/3/
> 
> 
> Testing
> ---
> 
> Added `ROOT_XFS_QuotaTest.ProjectIDReclaiming` test that verifies that 
> project ID is reclaimed and reused after sandbox GC. Ran `sudo make check`.
> 
> 
> Thanks,
> 
> Ilya Pronin
> 
>

Re: Review Request 67977: Fixed `test-linkee` logic in `ProcessRemoteLinkTest::SetUp()`.

[John Kordich via Review Board]

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


Ship it!




Ship It!

- John Kordich


On July 19, 2018, 8:44 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67977/
> ---
> 
> (Updated July 19, 2018, 8:44 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Eric Mumau, John Kordich, Joseph Wu, 
> Liangyu Zhao, and Radhika Jandhyala.
> 
> 
> Bugs: MESOS-5941 and MESOS-9097
> https://issues.apache.org/jira/browse/MESOS-5941
> https://issues.apache.org/jira/browse/MESOS-9097
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Several problems existed on Windows here:
> 
> * The `BUILD_DIR` path has forward slashes (probably fine, but wrong).
> * The executable must be named correctly, `.exe` and all.
> * We should assert that `test-linkee` exists.
> * The shell should not be used, otherwise `'(62)'` will resolve to an
>   unknown UPID inside `test-linkee` because of the single quotes.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> 8baf60d8bbb675e26fea5e76c825ef73fedbc629 
> 
> 
> Diff: https://reviews.apache.org/r/67977/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 67914: Updated XFS disk isolator to reclaim project IDs after disk GC.

[James Peach]

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




src/slave/containerizer/mesos/isolators/xfs/disk.hpp
Lines 24 (patched)


Let's use a pull gauge here for sampling performance reasons.

We can decrement it in `recover` and `nextProjectId`, and increment it in 
`returnProjectId`.



src/slave/containerizer/mesos/isolators/xfs/disk.hpp
Lines 124 (patched)


Shall we add a `project_ids_total` as well?


- James Peach


On July 19, 2018, 11:12 p.m., Ilya Pronin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67914/
> ---
> 
> (Updated July 19, 2018, 11:12 p.m.)
> 
> 
> Review request for mesos and James Peach.
> 
> 
> Bugs: MESOS-9007
> https://issues.apache.org/jira/browse/MESOS-9007
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently upon container destruction its project ID is unallocated by
> the isolator and removed from the container work directory. However due
> to API limitations we can't unset project IDs on symlinks that may exist
> inside the directory. Because of that the project may still exist until
> the container directory is garbage collected. If the project ID is
> reused for a new container, any lingering symlinks that still have that
> project ID will contribute to disk usage of the new container. Typically
> symlinks don't take much space, but still this leads to inaccuracy in
> disk space usage accounting.
> 
> This patch postpones project ID reclaiming until sandbox GC time. The
> isolator periodically checks if sandboxes of terminated containers still
> exist and deallocates project IDs of the ones that were removed. This
> mechanism can be improved in the future if we introduce a way for the
> isolators to learn about disk GCs.
> 
> Current number of available project IDs can be tracked with the new
> "containerizer/mesos/disk/project_ids_free" metric.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/xfs/disk.hpp 
> 0891f7709aa4f98758a727856d58e6177d46adca 
>   src/slave/containerizer/mesos/isolators/xfs/disk.cpp 
> 25f52a43b34b141bdaf7c448817423cf4264e22a 
>   src/slave/flags.hpp eeb9708f9ec76d83b6719541f4a012544c7c0cbe 
>   src/slave/flags.cpp 58cdc0f1100fe244e5bf1036e1ccf39478d5d478 
>   src/tests/containerizer/xfs_quota_tests.cpp 
> dc18a8a59d1eb7fae3592ef6ba8c046e4f46ee4a 
> 
> 
> Diff: https://reviews.apache.org/r/67914/diff/3/
> 
> 
> Testing
> ---
> 
> Added `ROOT_XFS_QuotaTest.ProjectIDReclaiming` test that verifies that 
> project ID is reclaimed and reused after sandbox GC. Ran `sudo make check`.
> 
> 
> Thanks,
> 
> Ilya Pronin
> 
>

Re: Review Request 67976: Windows: Added `nullptr` checks when using `libwinio_loop` pointer.

[John Kordich via Review Board]

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


Ship it!




Ship It!

- John Kordich


On July 19, 2018, 8:43 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67976/
> ---
> 
> (Updated July 19, 2018, 8:43 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Eric Mumau, John Kordich, Joseph Wu, 
> Liangyu Zhao, and Radhika Jandhyala.
> 
> 
> Bugs: MESOS-9097
> https://issues.apache.org/jira/browse/MESOS-9097
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> It was discovered that the `Socket` constructor could dereference a
> null pointer (by way of `prepare_async()`) if the Windows IOCP event
> loop had not yet been initialized. So now we check for its
> initialization before each dereference, and return an error or fatal
> log event.
> 
> In order to ensure that it is initialized in `test-linkee`, we call
> `process::initialize()`. This should be fixed in the future, per
> MESOS-9097.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/tests/test_linkee.cpp 
> cc482717290f72a5fd95fe745ac01893c0ce41f8 
>   3rdparty/libprocess/src/windows/event_loop.cpp 
> 0050ff0d87fdd01bf37742233fcd38b02f284ff3 
>   3rdparty/libprocess/src/windows/io.cpp 
> 1f9adde36192b673d7051549295a0f403be8e718 
> 
> 
> Diff: https://reviews.apache.org/r/67976/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 67951: Added optional `path_separator` parameter to `Path` constructor.

[John Kordich via Review Board]

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


Ship it!




Ship It!

- John Kordich


On July 19, 2018, 8:41 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67951/
> ---
> 
> (Updated July 19, 2018, 8:41 p.m.)
> 
> 
> Review request for mesos, Eric Mumau, John Kordich, Joseph Wu, Liangyu Zhao, 
> and Radhika Jandhyala.
> 
> 
> Bugs: MESOS-5817 and MESOS-5904
> https://issues.apache.org/jira/browse/MESOS-5817
> https://issues.apache.org/jira/browse/MESOS-5904
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This defaults to `os::PATH_SEPARATOR` and so by default retains the
> previous behavior. However, now `Path` can be arbitrarily used with,
> e.g., URLs on Windows by providing `/` as the separator.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/path.hpp 
> 27438d31617b3b78bf3d4deffd25c93340610e8d 
> 
> 
> Diff: https://reviews.apache.org/r/67951/diff/3/
> 
> 
> Testing
> ---
> 
> `stout-tests` and `libprocess-tests` passed on Windows and Ubuntu
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 67987: Added rapidjson to the mesos build.

[Benjamin Mahler]

> On July 23, 2018, 9:31 a.m., Benno Evers wrote:
> > 3rdparty/Makefile.am
> > Lines 367 (patched)
> > 
> >
> > This can be shortened a bit, since we already have the list stored in a 
> > variable:
> > 
> > ```
> > $(nodist_rapidjson_HEADERS): $(RAPIDJSON)-stamp
> > ```

Ah thanks, looks like elfio should do this too.


- Benjamin


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


On July 20, 2018, 3:38 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67987/
> ---
> 
> (Updated July 20, 2018, 3:38 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benno Evers, and James Peach.
> 
> 
> Bugs: MESOS-9092
> https://issues.apache.org/jira/browse/MESOS-9092
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This includes a stripped bundle of the latest release. Stripping
> is required for licensing (see rapidjson.md), but also helps reduce
> the bloat in the mesos git repo.
> 
> Also included is a readme for how to update the dependency.
> 
> 
> Diffs
> -
> 
>   3rdparty/CMakeLists.txt b58996d2ed7521e42d6298d174cc8c494b84eb8f 
>   3rdparty/Makefile.am 6123b8a0164d50f0bd771d2e5f485379811e7fcb 
>   3rdparty/README.md fa9a6483ca2dc4980767a2cb723d654e342e6bb8 
>   3rdparty/cmake/Versions.cmake 0a897d808cd9e05ac0d1a4e1ca61b8f3538f0c4a 
>   3rdparty/rapidjson-1.1.0.tar.gz PRE-CREATION 
>   3rdparty/rapidjson.md PRE-CREATION 
>   3rdparty/versions.am 57792800c4b4b67dd55a316ce00480db6c253d34 
>   configure.ac e86c496140ee9732ad3204ef22ae8ce60cd5c079 
>   src/Makefile.am ecb95ef152fd37e5e83c4f6415489f4cf3959578 
> 
> 
> Diff: https://reviews.apache.org/r/67987/diff/2/
> 
> 
> Testing
> ---
> 
> Tested at the end of this chain, since this is split across 
> stout/libprocess/mesos.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 67987: Added rapidjson to the mesos build.

[Benjamin Mahler]

> On July 20, 2018, 10:55 a.m., Benno Evers wrote:
> > 3rdparty/Makefile.am
> > Lines 517 (patched)
> > 
> >
> > What's the reason that we cannot use the same `DESTDIR=$(INSTALLDIR)` 
> > pattern as for the other entries here? If I remember correctly, 
> > `includedir` should default to `$(DESTDIR)/$(PREFIX)/include`, so I guess 
> > it's to get rid of some default prefix? In this case maybe `PREFIX=` would 
> > be a little bit more explicit and consistent. (especially since the 
> > rapidjson build is cmake-based and doesn't support setting `includedir` 
> > directly)
> 
> Benjamin Mahler wrote:
> This was a bad copy; rapidjson doesn't have a makefile. We could run a 
> cmake install but I noticed it's not done yet for any others? For now I 
> copied the approach taken for elfio since it uses `cp`. Let me know if that 
> makes sense, because I'm quite puzzled about the different approaches taken 
> in this file.
> 
> Benno Evers wrote:
> I think you may have confused yourself a little bit here :) Rapidjson 
> uses cmake, but at the time we want to install it should already be 
> configured and built, and the generated Makefile does indeed have a working 
> `install` target.
> 
> But the copying is also fine for a header-only library, and already has 
> precedence in this file, so I'd say feel free to choose whichever method 
> feels nicer.

Ah I didn't know that cmake outputs makefiles! I was just looking at the 
release contents and there wasn't one in there, so it looks like we'd have to 
run a cmake command to get a makefile in hand? Or directly install from the 
cmake command?

In any case, I think I'll stick to the copying approach since there's precedent 
and I'm not sure if we require cmake to be present. I left a TODO in the 
previous update to this patch to explore doing a cmake install instead of 
copying headers, since the copying approach is pretty brittle to upgrades and 
is rather verbose when there are a lot of headers.


- Benjamin


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


On July 20, 2018, 3:38 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67987/
> ---
> 
> (Updated July 20, 2018, 3:38 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benno Evers, and James Peach.
> 
> 
> Bugs: MESOS-9092
> https://issues.apache.org/jira/browse/MESOS-9092
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This includes a stripped bundle of the latest release. Stripping
> is required for licensing (see rapidjson.md), but also helps reduce
> the bloat in the mesos git repo.
> 
> Also included is a readme for how to update the dependency.
> 
> 
> Diffs
> -
> 
>   3rdparty/CMakeLists.txt b58996d2ed7521e42d6298d174cc8c494b84eb8f 
>   3rdparty/Makefile.am 6123b8a0164d50f0bd771d2e5f485379811e7fcb 
>   3rdparty/README.md fa9a6483ca2dc4980767a2cb723d654e342e6bb8 
>   3rdparty/cmake/Versions.cmake 0a897d808cd9e05ac0d1a4e1ca61b8f3538f0c4a 
>   3rdparty/rapidjson-1.1.0.tar.gz PRE-CREATION 
>   3rdparty/rapidjson.md PRE-CREATION 
>   3rdparty/versions.am 57792800c4b4b67dd55a316ce00480db6c253d34 
>   configure.ac e86c496140ee9732ad3204ef22ae8ce60cd5c079 
>   src/Makefile.am ecb95ef152fd37e5e83c4f6415489f4cf3959578 
> 
> 
> Diff: https://reviews.apache.org/r/67987/diff/2/
> 
> 
> Testing
> ---
> 
> Tested at the end of this chain, since this is split across 
> stout/libprocess/mesos.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 68001: Fix padding in MpscLinkedQueue.

[James Peach]

> On July 20, 2018, 9:27 p.m., Benjamin Bannier wrote:
> > 3rdparty/libprocess/src/mpsc_linked_queue.hpp
> > Lines 184-188 (patched)
> > 
> >
> > Wouldn't it be possible to avoid all the manual padding by aligning 
> > both `head` and `tail` on their own cache lines?
> > 
> > // We align `head` to 64 bytes (x86 cache line size) to guarantee 
> > it to
> > // be put on a new cache line. This is to prevent false sharing with
> > // other objects that could otherwise end up on the same cache line 
> > as
> > // this queue. We also align `tail` to avoid false sharing of `head`
> > // with `tail` and to avoid false sharing with other objects.
> > 
> > // We assume a x86_64 cache line size of 64 bytes.
> > //
> > // TODO(drexin): Programatically get the cache line size.
> > #define CACHE_LINE 64
> > 
> > alignas(CACHE_LINE) std::atomic*> head;
> > alignas(CACHE_LINE) Node* tail;
> > 
> > #undef CACHE_LINE
> > 
> > Wouldn't this satisfy the guarantees you list in your comment?
> > 
> > It would also allows us to avoid the macro which has a number of issues 
> > (`__COUNTER__` is a GNU extension, missing check of `sizeof(var) < 64`).
> 
> Benjamin Bannier wrote:
> I should have used a real variable for `CACHE_LINE` above to make this 
> less nasty, e.g.,
> 
> static constexpr std::size_t CACHE_LINE = 64; // Requires e.g., 
> `#include `.
> 
> Dario Rexin wrote:
> It would probably work for the padding between head and tail, but what 
> about the padding after tail? Should we `alignas(64) char tailPad;` or 
> somethign like that?
> 
> Benjamin Bannier wrote:
> If we align both `head` and `tail` on separate cache lines, I believe we 
> cannot have one queue's `tail` share a cache line with e.g., another queue's 
> `head`
> 
> Do need to worry about `tail` sharing a cache line with arbitrary other 
> data? If not it would seem we wouldn't need additional padding after `tail`.
> 
> Dario Rexin wrote:
> In general we should worry about other data. Anything that can cause 
> cache line invalidation would be problematic. I don't want to make 
> assumptions about the likelyhood of that happening. I think it's better to be 
> on the safe side.

I concur with Dario that it's better to be explicit about the padding. This 
would ensure no false sharing occurs when a `MpscLinkedQueue` instance is 
followed by another variable that has no explicit alignment. We should capture 
this rationale (and a summary of this discussion) in a code comment.

FWIW, the only usage of `MpscLinkedQueue` is:
```C
class EventQueue
{
   MpscLinkedQueue queue;
   std::atomic comissioned = ATOMIC_VAR_INIT(true);
};
```

Experimentally, gcc doesn't add the padding we want in this case:
```C
#include 
#include 
#include 

struct Event
{
alignas(64) std::atomic p1;
alignas(64) std::atomic p2;
std::atomic b;
};

Event e;

int main()
{
printf("p1 -> %zu\n", offsetof(Event, p1));
printf("p1 -> %zu\n", offsetof(Event, p2));
printf("b -> %zu\n", offsetof(Event, b));
}

$ c++ -std=c++14 ./e.cc  && ./a.out
p1 -> 0
p1 -> 64
b -> 72
```


- James


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


On July 20, 2018, 4:51 p.m., Dario Rexin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68001/
> ---
> 
> (Updated July 20, 2018, 4:51 p.m.)
> 
> 
> Review request for Benjamin Bannier.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch aligns the head of MpscLinkedQueue to a new cache line
> and adds padding between head and tail to avoid false sharing
> between to two and after tail to avoid false sharing with other
> objects that could otherwise end up on the same cache line.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/mpsc_linked_queue.hpp 48c95093d 
> 
> 
> Diff: https://reviews.apache.org/r/68001/diff/1/
> 
> 
> Testing
> ---
> 
> make check & benchmarks
> 
> 
> Thanks,
> 
> Dario Rexin
> 
>

Re: Review Request 68001: Fix padding in MpscLinkedQueue.

[Dario Rexin]

> On July 20, 2018, 9:27 p.m., Benjamin Bannier wrote:
> > 3rdparty/libprocess/src/mpsc_linked_queue.hpp
> > Lines 184-188 (patched)
> > 
> >
> > Wouldn't it be possible to avoid all the manual padding by aligning 
> > both `head` and `tail` on their own cache lines?
> > 
> > // We align `head` to 64 bytes (x86 cache line size) to guarantee 
> > it to
> > // be put on a new cache line. This is to prevent false sharing with
> > // other objects that could otherwise end up on the same cache line 
> > as
> > // this queue. We also align `tail` to avoid false sharing of `head`
> > // with `tail` and to avoid false sharing with other objects.
> > 
> > // We assume a x86_64 cache line size of 64 bytes.
> > //
> > // TODO(drexin): Programatically get the cache line size.
> > #define CACHE_LINE 64
> > 
> > alignas(CACHE_LINE) std::atomic*> head;
> > alignas(CACHE_LINE) Node* tail;
> > 
> > #undef CACHE_LINE
> > 
> > Wouldn't this satisfy the guarantees you list in your comment?
> > 
> > It would also allows us to avoid the macro which has a number of issues 
> > (`__COUNTER__` is a GNU extension, missing check of `sizeof(var) < 64`).
> 
> Benjamin Bannier wrote:
> I should have used a real variable for `CACHE_LINE` above to make this 
> less nasty, e.g.,
> 
> static constexpr std::size_t CACHE_LINE = 64; // Requires e.g., 
> `#include `.
> 
> Dario Rexin wrote:
> It would probably work for the padding between head and tail, but what 
> about the padding after tail? Should we `alignas(64) char tailPad;` or 
> somethign like that?
> 
> Benjamin Bannier wrote:
> If we align both `head` and `tail` on separate cache lines, I believe we 
> cannot have one queue's `tail` share a cache line with e.g., another queue's 
> `head`
> 
> Do need to worry about `tail` sharing a cache line with arbitrary other 
> data? If not it would seem we wouldn't need additional padding after `tail`.

In general we should worry about other data. Anything that can cause cache line 
invalidation would be problematic. I don't want to make assumptions about the 
likelyhood of that happening. I think it's better to be on the safe side.


- Dario


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


On July 20, 2018, 4:51 p.m., Dario Rexin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68001/
> ---
> 
> (Updated July 20, 2018, 4:51 p.m.)
> 
> 
> Review request for Benjamin Bannier.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch aligns the head of MpscLinkedQueue to a new cache line
> and adds padding between head and tail to avoid false sharing
> between to two and after tail to avoid false sharing with other
> objects that could otherwise end up on the same cache line.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/mpsc_linked_queue.hpp 48c95093d 
> 
> 
> Diff: https://reviews.apache.org/r/68001/diff/1/
> 
> 
> Testing
> ---
> 
> make check & benchmarks
> 
> 
> Thanks,
> 
> Dario Rexin
> 
>

Re: Review Request 68001: Fix padding in MpscLinkedQueue.

[Benjamin Bannier]

> On July 20, 2018, 11:27 p.m., Benjamin Bannier wrote:
> > 3rdparty/libprocess/src/mpsc_linked_queue.hpp
> > Lines 184-188 (patched)
> > 
> >
> > Wouldn't it be possible to avoid all the manual padding by aligning 
> > both `head` and `tail` on their own cache lines?
> > 
> > // We align `head` to 64 bytes (x86 cache line size) to guarantee 
> > it to
> > // be put on a new cache line. This is to prevent false sharing with
> > // other objects that could otherwise end up on the same cache line 
> > as
> > // this queue. We also align `tail` to avoid false sharing of `head`
> > // with `tail` and to avoid false sharing with other objects.
> > 
> > // We assume a x86_64 cache line size of 64 bytes.
> > //
> > // TODO(drexin): Programatically get the cache line size.
> > #define CACHE_LINE 64
> > 
> > alignas(CACHE_LINE) std::atomic*> head;
> > alignas(CACHE_LINE) Node* tail;
> > 
> > #undef CACHE_LINE
> > 
> > Wouldn't this satisfy the guarantees you list in your comment?
> > 
> > It would also allows us to avoid the macro which has a number of issues 
> > (`__COUNTER__` is a GNU extension, missing check of `sizeof(var) < 64`).
> 
> Benjamin Bannier wrote:
> I should have used a real variable for `CACHE_LINE` above to make this 
> less nasty, e.g.,
> 
> static constexpr std::size_t CACHE_LINE = 64; // Requires e.g., 
> `#include `.
> 
> Dario Rexin wrote:
> It would probably work for the padding between head and tail, but what 
> about the padding after tail? Should we `alignas(64) char tailPad;` or 
> somethign like that?

If we align both `head` and `tail` on separate cache lines, I believe we cannot 
have one queue's `tail` share a cache line with e.g., another queue's `head`

Do need to worry about `tail` sharing a cache line with arbitrary other data? 
If not it would seem we wouldn't need additional padding after `tail`.


- Benjamin


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


On July 20, 2018, 6:51 p.m., Dario Rexin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68001/
> ---
> 
> (Updated July 20, 2018, 6:51 p.m.)
> 
> 
> Review request for Benjamin Bannier.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch aligns the head of MpscLinkedQueue to a new cache line
> and adds padding between head and tail to avoid false sharing
> between to two and after tail to avoid false sharing with other
> objects that could otherwise end up on the same cache line.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/mpsc_linked_queue.hpp 48c95093d 
> 
> 
> Diff: https://reviews.apache.org/r/68001/diff/1/
> 
> 
> Testing
> ---
> 
> make check & benchmarks
> 
> 
> Thanks,
> 
> Dario Rexin
> 
>

Re: Review Request 67827: Added a helper `stripIncapableResources` in the allocator.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [6, 67945, 67826, 67827]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose --disable-libtool-wrappers' ENVIRONMENT='GLOG_v=1 
MESOS_VERBOSE=1'; ./support/docker-build.sh

- Mesos Reviewbot


On July 21, 2018, 1:30 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67827/
> ---
> 
> (Updated July 21, 2018, 1:30 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9104
> https://issues.apache.org/jira/browse/MESOS-9104
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This helper removes any resources that the framework is not
> capable of receiving based on the given framework capability.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 702e7c0aa84b4b672d82c759c25a28a77c78ad50 
>   src/master/allocator/mesos/hierarchical.cpp 
> 707dd6bd0f255a64d759ce87cbf75be57d86b392 
> 
> 
> Diff: https://reviews.apache.org/r/67827/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 67988: Improved performance of jsonify by integrating with rapidjson.

[Michael Park]

> On July 20, 2018, 2:29 p.m., Michael Park wrote:
> > Looks good!
> > 
> > It might be worth considering using the `OStreamWrapper` stuff for the 
> > `ostream` API.
> > I know writing to `StringBuffer` is faster than writing to 
> > `OStreamWrapper`, but
> > I think just writing to `OStreamWrapper` would be faster than writing to 
> > `StringBuffer`
> > then copying that into a `std::string` then writing that to `ostream` in 
> > the end anyway.
> 
> Benjamin Mahler wrote:
> > I think just writing to OStreamWrapper would be faster than writing to 
> StringBuffer
> 
> That had seemed doubtful to me based on their documentation:
> http://rapidjson.org/md_doc_stream.html#iostreamWrapper
> 
> I would be interesting to get the numbers! However, we don't write to 
> ostream in the end any longer:
> https://reviews.apache.org/r/67992/
> 
> I could remove the `<<` operator but I figured I would just leave it 
> untouched for now.

Yeah, you left out the second half of my sentence but I assume it wasn't 
intentional.
I didn't notice the patch to replace `ostringstream`. Sounds good to me!


> On July 20, 2018, 2:29 p.m., Michael Park wrote:
> > 3rdparty/stout/include/stout/jsonify.hpp
> > Line 137 (original), 95 (patched)
> > 
> >
> > Looks like `GetString` returns a `const char*`. We should provide the 
> > length here: `{buffer.GetString(), buffer.GetSize()}`.
> 
> Benjamin Mahler wrote:
> Ah nice catch! This should avoid walking the string right? I'll re-run 
> the numbers.

Yep, exactly.


- Michael


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


On July 19, 2018, 8:38 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67988/
> ---
> 
> (Updated July 19, 2018, 8:38 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benno Evers, and Michael Park.
> 
> 
> Bugs: MESOS-9092
> https://issues.apache.org/jira/browse/MESOS-9092
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This reduces the time needed for the client to finish receiving a
> master's /state response by 50% in the `StateQuery` benchmark:
> 
> minq1q3   max
> baseline   6.52  6.76  7.33  8.26
> rapidjson w/ SIMD  3.48  3.54  4.12  4.4
> rapidjson  3.29  3.32  3.65  3.85
> 
> SIMD is left disabled for now since it showed slightly slower
> results.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/jsonify.hpp 
> 2314980e185ee61cc2ea54f1b4d2a8b35e58121c 
> 
> 
> Diff: https://reviews.apache.org/r/67988/diff/2/
> 
> 
> Testing
> ---
> 
> Tested at the end of this chain, since this is split across 
> stout/libprocess/mesos.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 68001: Fix padding in MpscLinkedQueue.

[Dario Rexin]

> On July 20, 2018, 9:27 p.m., Benjamin Bannier wrote:
> > 3rdparty/libprocess/src/mpsc_linked_queue.hpp
> > Lines 184-188 (patched)
> > 
> >
> > Wouldn't it be possible to avoid all the manual padding by aligning 
> > both `head` and `tail` on their own cache lines?
> > 
> > // We align `head` to 64 bytes (x86 cache line size) to guarantee 
> > it to
> > // be put on a new cache line. This is to prevent false sharing with
> > // other objects that could otherwise end up on the same cache line 
> > as
> > // this queue. We also align `tail` to avoid false sharing of `head`
> > // with `tail` and to avoid false sharing with other objects.
> > 
> > // We assume a x86_64 cache line size of 64 bytes.
> > //
> > // TODO(drexin): Programatically get the cache line size.
> > #define CACHE_LINE 64
> > 
> > alignas(CACHE_LINE) std::atomic*> head;
> > alignas(CACHE_LINE) Node* tail;
> > 
> > #undef CACHE_LINE
> > 
> > Wouldn't this satisfy the guarantees you list in your comment?
> > 
> > It would also allows us to avoid the macro which has a number of issues 
> > (`__COUNTER__` is a GNU extension, missing check of `sizeof(var) < 64`).
> 
> Benjamin Bannier wrote:
> I should have used a real variable for `CACHE_LINE` above to make this 
> less nasty, e.g.,
> 
> static constexpr std::size_t CACHE_LINE = 64; // Requires e.g., 
> `#include `.

It would probably work for the padding between head and tail, but what about 
the padding after tail? Should we `alignas(64) char tailPad;` or somethign like 
that?


- Dario


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


On July 20, 2018, 4:51 p.m., Dario Rexin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68001/
> ---
> 
> (Updated July 20, 2018, 4:51 p.m.)
> 
> 
> Review request for Benjamin Bannier.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch aligns the head of MpscLinkedQueue to a new cache line
> and adds padding between head and tail to avoid false sharing
> between to two and after tail to avoid false sharing with other
> objects that could otherwise end up on the same cache line.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/mpsc_linked_queue.hpp 48c95093d 
> 
> 
> Diff: https://reviews.apache.org/r/68001/diff/1/
> 
> 
> Testing
> ---
> 
> make check & benchmarks
> 
> 
> Thanks,
> 
> Dario Rexin
> 
>

Re: Review Request 68022: Enabled Seccomp filter in the containerizer launcher. (WIP)

[Mesos Reviewbot Windows]

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



FAIL: Failed to apply the dependent review: 67844.

Failed command: `python.exe .\support\python3\apply-reviews.py -n -r 67844`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/1968/mesos-review-68022

Relevant logs:

- 
[apply-review-67844-stdout.log](http://dcos-win.westus.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/1968/mesos-review-68022/logs/apply-review-67844-stdout.log):

```
error: missing binary patch data for '3rdparty/libseccomp-2.3.3.tar.gz'
error: binary patch does not apply to '3rdparty/libseccomp-2.3.3.tar.gz'
error: 3rdparty/libseccomp-2.3.3.tar.gz: patch does not apply
```

- Mesos Reviewbot Windows


On July 23, 2018, 1:57 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68022/
> ---
> 
> (Updated July 23, 2018, 1:57 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, James Peach, and Qian Zhang.
> 
> 
> Bugs: MESOS-9106
> https://issues.apache.org/jira/browse/MESOS-9106
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Containerizer launcher creates an instance of `SeccompFilter`, which is
> used to setup Seccomp profile using `ContainerSeccompProfile` message
> prepared by the `linux/seccomp` isolator. The Seccomp filter is loaded
> right before calling `execve()`, so that a container will be running
> with a syscall filtering enabled.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/launch.cpp 
> 7193da0a094df3e441e185c62b3a0379a0bdc4a2 
> 
> 
> Diff: https://reviews.apache.org/r/68022/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Review Request 68022: Enabled Seccomp filter in the containerizer launcher. (WIP)

[Andrei Budnik]

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

Review request for mesos, Gilbert Song, Jie Yu, James Peach, and Qian Zhang.


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


Repository: mesos


Description
---

Containerizer launcher creates an instance of `SeccompFilter`, which is
used to setup Seccomp profile using `ContainerSeccompProfile` message
prepared by the `linux/seccomp` isolator. The Seccomp filter is loaded
right before calling `execve()`, so that a container will be running
with a syscall filtering enabled.


Diffs
-

  src/slave/containerizer/mesos/launch.cpp 
7193da0a094df3e441e185c62b3a0379a0bdc4a2 


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


Testing
---


Thanks,

Andrei Budnik

Review Request 68021: Added `linux/seccomp` isolator. (WIP)

[Andrei Budnik]

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

Review request for mesos, Gilbert Song, Jie Yu, James Peach, and Qian Zhang.


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


Repository: mesos


Description
---

This patch introduces `linux/seccomp` isolator which is used for
preparing `ContainerSeccompProfile` for the Mesos containerizer
launcher. If the `ContainerConfig` message has an info about Seccomp
profile name, then this info will be used to locate a Seccomp profile.
The given Seccomp profile is parsed and the resulting
`ContainerSeccompProfile` is stored in the `ContainerLaunchInfo`
message.


Diffs
-

  src/CMakeLists.txt 10b0946d6f49c7e9c201bad6f9f1b41cc8460fe5 
  src/Makefile.am ecb95ef152fd37e5e83c4f6415489f4cf3959578 
  src/slave/containerizer/mesos/containerizer.cpp 
98129d006cda9b65804b518619b6addc8990410a 
  src/slave/containerizer/mesos/isolators/linux/seccomp.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/linux/seccomp.cpp PRE-CREATION 


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


Testing
---


Thanks,

Andrei Budnik

Review Request 68020: Added new `--seccomp_config_dir` flag to the agent.

[Andrei Budnik]

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

Review request for mesos, Gilbert Song, Jie Yu, James Peach, and Qian Zhang.


Repository: mesos


Description
---

This flag is used to specify a path to the directory containing
Seccomp profiles. This flag is used by the `linux/seccomp` isolator.


Diffs
-

  src/slave/flags.hpp eeb9708f9ec76d83b6719541f4a012544c7c0cbe 
  src/slave/flags.cpp 58cdc0f1100fe244e5bf1036e1ccf39478d5d478 


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


Testing
---


Thanks,

Andrei Budnik

Review Request 68019: Added a parser for the Docker Seccomp config format.

[Andrei Budnik]

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

Review request for mesos, Gilbert Song, Jie Yu, James Peach, and Qian Zhang.


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


Repository: mesos


Description
---

Docker Seccomp config is a JSON file containing Seccomp filtering
rules. This patch introduces a parser for Docker Seccomp config format.
This parser accepts a JSON-string, parses and validates it, then
returns a prepared `ContainerSeccompProfile` message.


Diffs
-

  src/CMakeLists.txt 10b0946d6f49c7e9c201bad6f9f1b41cc8460fe5 
  src/Makefile.am ecb95ef152fd37e5e83c4f6415489f4cf3959578 
  src/linux/seccomp/seccomp_parser.hpp PRE-CREATION 
  src/linux/seccomp/seccomp_parser.cpp PRE-CREATION 


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


Testing
---


Thanks,

Andrei Budnik

Review Request 68017: Added Seccomp-related protobuf messages.

[Andrei Budnik]

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

Review request for mesos, Gilbert Song, Jie Yu, James Peach, and Qian Zhang.


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


Repository: mesos


Description
---

See summary.


Diffs
-

  include/mesos/mesos.proto 5a985fca39cdfb7e9b4775650a7e5dbe68c3b8ae 
  include/mesos/slave/containerizer.proto 
5b4dcdda0f55ea3355c78d1447c7be9ca54d9dc9 


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


Testing
---


Thanks,

Andrei Budnik

Review Request 68018: Added `SeccompFilter` class. (WIP)

[Andrei Budnik]

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

Review request for mesos, Gilbert Song, Jie Yu, James Peach, and Qian Zhang.


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


Repository: mesos


Description
---

`SeccompFilter` class is a wrapper for `libseccomp` API. Its main
purpose is to provide a translation of the `ContainerSeccompProfile`
message into calls of `libseccomp` API.


Diffs
-

  src/CMakeLists.txt 10b0946d6f49c7e9c201bad6f9f1b41cc8460fe5 
  src/Makefile.am ecb95ef152fd37e5e83c4f6415489f4cf3959578 
  src/linux/seccomp/seccomp.hpp PRE-CREATION 
  src/linux/seccomp/seccomp.cpp PRE-CREATION 


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


Testing
---


Thanks,

Andrei Budnik

Review Request 68016: Added libseccomp to the build.

[Andrei Budnik]

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

Review request for mesos, Gilbert Song, Jie Yu, James Peach, and Qian Zhang.


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


Repository: mesos


Description
---

This library is needed to implement Seccomp syscall filtering in the
Mesos containerizer. This patch introduces `seccomp-isolator` build
flag, which is used to include or exclude sources related to Seccomp
from the build. Since Seccomp is a Linux-specific feature, the flag
is disabled by default. Enabling `seccomp-isolator` means either:

1. Compiling and linking against the bundled version of libseccomp from
   sources (default).

2. Linking against the libseccomp installed in the OS,
   if `--with-libseccomp` build flag is provided.


Diffs
-

  3rdparty/CMakeLists.txt b58996d2ed7521e42d6298d174cc8c494b84eb8f 
  3rdparty/Makefile.am 6123b8a0164d50f0bd771d2e5f485379811e7fcb 
  3rdparty/cmake/Versions.cmake 0a897d808cd9e05ac0d1a4e1ca61b8f3538f0c4a 
  3rdparty/versions.am 57792800c4b4b67dd55a316ce00480db6c253d34 
  cmake/CompilationConfigure.cmake f80c265d117c70acda95da6b943cdd3e4e477daa 
  configure.ac e86c496140ee9732ad3204ef22ae8ce60cd5c079 
  src/CMakeLists.txt 10b0946d6f49c7e9c201bad6f9f1b41cc8460fe5 
  src/Makefile.am ecb95ef152fd37e5e83c4f6415489f4cf3959578 


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


Testing
---


Thanks,

Andrei Budnik

Review Request 67844: Bundled libseccomp v2.3.3 into 3rdparty libraries.

[Andrei Budnik]

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

Review request for mesos, Gilbert Song, Jie Yu, James Peach, and Qian Zhang.


Summary (updated)
-

Bundled libseccomp v2.3.3 into 3rdparty libraries.


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


Repository: mesos


Description (updated)
---

The bundled library is downloaded from the `libseccomp` github page:
https://github.com/seccomp/libseccomp/releases/tag/v2.3.3

This library is bundled for Linux only, because Seccomp is a
Linux-specific feature.


Diffs (updated)
-

  3rdparty/libseccomp-2.3.3.tar.gz PRE-CREATION 


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


Testing
---


Thanks,

Andrei Budnik

Re: Review Request 67987: Added rapidjson to the mesos build.

[Benno Evers]

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


Fix it, then Ship it!





3rdparty/Makefile.am
Lines 367 (patched)


This can be shortened a bit, since we already have the list stored in a 
variable:

```
$(nodist_rapidjson_HEADERS): $(RAPIDJSON)-stamp
```


- Benno Evers


On July 20, 2018, 3:38 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67987/
> ---
> 
> (Updated July 20, 2018, 3:38 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benno Evers, and James Peach.
> 
> 
> Bugs: MESOS-9092
> https://issues.apache.org/jira/browse/MESOS-9092
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This includes a stripped bundle of the latest release. Stripping
> is required for licensing (see rapidjson.md), but also helps reduce
> the bloat in the mesos git repo.
> 
> Also included is a readme for how to update the dependency.
> 
> 
> Diffs
> -
> 
>   3rdparty/CMakeLists.txt b58996d2ed7521e42d6298d174cc8c494b84eb8f 
>   3rdparty/Makefile.am 6123b8a0164d50f0bd771d2e5f485379811e7fcb 
>   3rdparty/README.md fa9a6483ca2dc4980767a2cb723d654e342e6bb8 
>   3rdparty/cmake/Versions.cmake 0a897d808cd9e05ac0d1a4e1ca61b8f3538f0c4a 
>   3rdparty/rapidjson-1.1.0.tar.gz PRE-CREATION 
>   3rdparty/rapidjson.md PRE-CREATION 
>   3rdparty/versions.am 57792800c4b4b67dd55a316ce00480db6c253d34 
>   configure.ac e86c496140ee9732ad3204ef22ae8ce60cd5c079 
>   src/Makefile.am ecb95ef152fd37e5e83c4f6415489f4cf3959578 
> 
> 
> Diff: https://reviews.apache.org/r/67987/diff/2/
> 
> 
> Testing
> ---
> 
> Tested at the end of this chain, since this is split across 
> stout/libprocess/mesos.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 67987: Added rapidjson to the mesos build.

[Benno Evers]

> On July 20, 2018, 10:55 a.m., Benno Evers wrote:
> > 3rdparty/Makefile.am
> > Lines 517 (patched)
> > 
> >
> > What's the reason that we cannot use the same `DESTDIR=$(INSTALLDIR)` 
> > pattern as for the other entries here? If I remember correctly, 
> > `includedir` should default to `$(DESTDIR)/$(PREFIX)/include`, so I guess 
> > it's to get rid of some default prefix? In this case maybe `PREFIX=` would 
> > be a little bit more explicit and consistent. (especially since the 
> > rapidjson build is cmake-based and doesn't support setting `includedir` 
> > directly)
> 
> Benjamin Mahler wrote:
> This was a bad copy; rapidjson doesn't have a makefile. We could run a 
> cmake install but I noticed it's not done yet for any others? For now I 
> copied the approach taken for elfio since it uses `cp`. Let me know if that 
> makes sense, because I'm quite puzzled about the different approaches taken 
> in this file.

I think you may have confused yourself a little bit here :) Rapidjson uses 
cmake, but at the time we want to install it should already be configured and 
built, and the generated Makefile does indeed have a working `install` target.

But the copying is also fine for a header-only library, and already has 
precedence in this file, so I'd say feel free to choose whichever method feels 
nicer.


- Benno


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


On July 20, 2018, 3:38 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67987/
> ---
> 
> (Updated July 20, 2018, 3:38 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benno Evers, and James Peach.
> 
> 
> Bugs: MESOS-9092
> https://issues.apache.org/jira/browse/MESOS-9092
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This includes a stripped bundle of the latest release. Stripping
> is required for licensing (see rapidjson.md), but also helps reduce
> the bloat in the mesos git repo.
> 
> Also included is a readme for how to update the dependency.
> 
> 
> Diffs
> -
> 
>   3rdparty/CMakeLists.txt b58996d2ed7521e42d6298d174cc8c494b84eb8f 
>   3rdparty/Makefile.am 6123b8a0164d50f0bd771d2e5f485379811e7fcb 
>   3rdparty/README.md fa9a6483ca2dc4980767a2cb723d654e342e6bb8 
>   3rdparty/cmake/Versions.cmake 0a897d808cd9e05ac0d1a4e1ca61b8f3538f0c4a 
>   3rdparty/rapidjson-1.1.0.tar.gz PRE-CREATION 
>   3rdparty/rapidjson.md PRE-CREATION 
>   3rdparty/versions.am 57792800c4b4b67dd55a316ce00480db6c253d34 
>   configure.ac e86c496140ee9732ad3204ef22ae8ce60cd5c079 
>   src/Makefile.am ecb95ef152fd37e5e83c4f6415489f4cf3959578 
> 
> 
> Diff: https://reviews.apache.org/r/67987/diff/2/
> 
> 
> Testing
> ---
> 
> Tested at the end of this chain, since this is split across 
> stout/libprocess/mesos.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 67916: Patched Google Test with upstream bugfix.

[Benjamin Bannier]

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


Fix it, then Ship it!




> It is not yet known if the Autotools system will also need this patch. Do we 
> want it added there anyway?

I'd vote for enabling this on all platforms to simplify the build setup.


3rdparty/googletest-release-1.8.0.patch
Lines 1 (patched)


This patch has landed upstream so let's just use the patch from upstream 
`f66ab00704cd47e4e63ef6d425ca14b9192aaebb` instead of referencing a PR. If we 
do that we should also update our commit message.

`f66ab00704c` applied cleanly with some fuzziness for me, so if possible 
just use the actual upstream patch.



3rdparty/googletest-release-1.8.0.patch
Lines 1-10 (patched)


Thanks for including the git metadata with the patch! This is much easier 
to maintain than bare diffs we have in many other patches.


- Benjamin Bannier


On July 13, 2018, 11:04 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67916/
> ---
> 
> (Updated July 13, 2018, 11:04 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Joseph Wu, and Till Toenshoff.
> 
> 
> Bugs: MESOS-8990
> https://issues.apache.org/jira/browse/MESOS-8990
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Per MESOS-8990, our Google Test dependency needs a patch from
> upstream, https://github.com/google/googletest/pull/1620, in order to
> continue building with the next version of MSVC (and potentially other
> compilers).
> 
> This patch file was generated by cherry-picking `f66ab00` from
> `master` onto `release-1.8.0` in the Google Test repo, and resolving
> the merge conflict.
> 
> 
> Diffs
> -
> 
>   3rdparty/CMakeLists.txt b58996d2ed7521e42d6298d174cc8c494b84eb8f 
>   3rdparty/googletest-release-1.8.0.patch PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/67916/diff/1/
> 
> 
> Testing
> ---
> 
> Clean build on Windows using CMake (which is the only place this patch 
> currently applies). It is not yet known if the Autotools system will also 
> need this patch. Do we want it added there anyway?
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>