Review Request 70161: Added validation for `QuotaConfig`.

[Meng Zhu]

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

A `QuotaConfig` is valid if the following conditions are met:

 (1) The config has a valid non-"*" role.

 (2) Resource scalar values are non-negative and finite.

 (3) If both guarantees and limits are set for a particular
 resource, then guarantee <= limit for that resource.


Diffs
-

  src/master/quota.hpp 5cd2bb0e8669ee0290a4c1fe1058b87251772939 
  src/master/quota.cpp 671626c01ada595d7557d5266e39a17cce243b94 


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


Testing
---

make check
dedicated test added in subsequent patches


Thanks,

Meng Zhu

Review Request 70160: Added `<<` operator to `protobuf::Map`.

[Meng Zhu]

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

Review request for mesos and Benjamin Mahler.


Repository: mesos


Description
---

Added `<<` operator to `protobuf::Map`.


Diffs
-

  include/mesos/type_utils.hpp 630f21f34c2215092e5ca14ffc5c966cce2b63e4 


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


Testing
---

make check


Thanks,

Meng Zhu

Review Request 70159: Updated `UPDATE_QUOTA` call with a new schema.

[Meng Zhu]

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

A new message `QuotaConfig` is introduced to describe
a role's quota configuration. It uses name-value pairs
to specify resources in the quota. This will replace the
existing `QuotaInfo` message in APIs and registries.


Diffs
-

  include/mesos/master/master.proto 0dc2d686c723ed960a8947ff48652bbdcf5135eb 
  include/mesos/quota/quota.proto 3e6b899e7f64c2c618190dd447856129122a47aa 
  include/mesos/v1/master/master.proto 3816ab477d75ff26446159bda072766554ab9f77 
  include/mesos/v1/quota/quota.proto 44a416dd2159fb9fd6c87236ac8fbc3916a1822a 


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


Testing
---

make check


Thanks,

Meng Zhu

Re: Review Request 70138: Replaced reading mounttable with getting path gid in volume gid manager.

[Qian Zhang]

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

(Updated March 7, 2019, 4:57 p.m.)


Review request for mesos, Andrei Budnik and Gilbert Song.


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


Repository: mesos


Description
---

Replaced reading mounttable with getting path gid in volume gid manager.


Diffs
-

  src/slave/volume_gid_manager/volume_gid_manager.cpp 
d8873874f81f5eccb713a58e9714e25711baa415 


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


Testing
---


Thanks,

Qian Zhang

Re: Review Request 70140: Updated UNPRIVILEGED_USER_PersistentVolumes to cover non-shared PV.

[Qian Zhang]

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

(Updated March 7, 2019, 4:57 p.m.)


Review request for mesos, Andrei Budnik and Gilbert Song.


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


Repository: mesos


Description
---

Updated UNPRIVILEGED_USER_PersistentVolumes to cover non-shared PV.


Diffs
-

  src/tests/persistent_volume_tests.cpp 
8a5672ef117f64c173113df903d9e62bef7253f1 


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


Testing
---

sudo make check


Thanks,

Qian Zhang

Re: Review Request 70139: Updated ROOT_UNPRIVILEGED_USER_PersistentVolumes to cover non-shared PV.

[Qian Zhang]

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

(Updated March 7, 2019, 4:56 p.m.)


Review request for mesos, Andrei Budnik and Gilbert Song.


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


Repository: mesos


Description
---

Updated ROOT_UNPRIVILEGED_USER_PersistentVolumes to cover non-shared PV.


Diffs
-

  src/tests/containerizer/linux_filesystem_isolator_tests.cpp 
d32bf74f64a2486b409ea6c34f5c41c553a4af6a 


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


Testing
---

sudo make check


Thanks,

Qian Zhang

Re: Review Request 70137: Made volume gid manager allocate & deallocate gid to non-shared PV.

[Qian Zhang]

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

(Updated March 7, 2019, 4:55 p.m.)


Review request for mesos, Andrei Budnik and Gilbert Song.


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


Repository: mesos


Description
---

Made volume gid manager allocate & deallocate gid to non-shared PV.


Diffs
-

  src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
341853a2df74f6ec3135e942b59a5da9d8f8460e 
  src/slave/containerizer/mesos/isolators/filesystem/posix.cpp 
08449e269b550ba41f63b909ffbd5f7a7a83e32b 
  src/slave/slave.cpp f9b58175ad5433bdfc15a312dfce6e972dea7fa3 


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


Testing
---


Thanks,

Qian Zhang

Re: Review Request 70139: Updated ROOT_UNPRIVILEGED_USER_PersistentVolumes to cover non-shared PV.

[Gilbert Song]

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


Ship it!




Ship It!

- Gilbert Song


On March 6, 2019, 6:10 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70139/
> ---
> 
> (Updated March 6, 2019, 6:10 a.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Gilbert Song.
> 
> 
> Bugs: MESOS-8813
> https://issues.apache.org/jira/browse/MESOS-8813
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated ROOT_UNPRIVILEGED_USER_PersistentVolumes to cover non-shared PV.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/linux_filesystem_isolator_tests.cpp 
> d32bf74f64a2486b409ea6c34f5c41c553a4af6a 
> 
> 
> Diff: https://reviews.apache.org/r/70139/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 70140: Updated UNPRIVILEGED_USER_PersistentVolumes to cover non-shared PV.

[Gilbert Song]

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


Ship it!




Ship It!

- Gilbert Song


On March 6, 2019, 6:36 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70140/
> ---
> 
> (Updated March 6, 2019, 6:36 a.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Gilbert Song.
> 
> 
> Bugs: MESOS-8813
> https://issues.apache.org/jira/browse/MESOS-8813
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated UNPRIVILEGED_USER_PersistentVolumes to cover non-shared PV.
> 
> 
> Diffs
> -
> 
>   src/tests/persistent_volume_tests.cpp 
> 8a5672ef117f64c173113df903d9e62bef7253f1 
> 
> 
> Diff: https://reviews.apache.org/r/70140/diff/2/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 70138: Replaced reading mounttable with getting path gid in volume gid manager.

[Gilbert Song]

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


Ship it!




Ship It!

- Gilbert Song


On March 6, 2019, 6:05 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70138/
> ---
> 
> (Updated March 6, 2019, 6:05 a.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Gilbert Song.
> 
> 
> Bugs: MESOS-8813
> https://issues.apache.org/jira/browse/MESOS-8813
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Replaced reading mounttable with getting path gid in volume gid manager.
> 
> 
> Diffs
> -
> 
>   src/slave/volume_gid_manager/volume_gid_manager.cpp 
> d8873874f81f5eccb713a58e9714e25711baa415 
> 
> 
> Diff: https://reviews.apache.org/r/70138/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 70116: Added metrics for offer operation feedback.

[Gastón Kleiman]

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




src/master/master.hpp
Lines 941-944 (patched)


Is this used anywhere?



src/master/metrics.hpp
Lines 83 (patched)


s/per operation/per operation type/



src/master/metrics.cpp
Lines 158-159 (original), 160-161 (patched)


Should we add a similar metric for operation status updates?



src/master/metrics.cpp
Lines 174-177 (original), 176-179 (patched)


Should we add a similar metrics for operation status updates?



src/master/metrics.cpp
Lines 386-388 (patched)


This is weirdly formatted, I think the following follows the style guide 
and is more readable:

```
if (type == Offer::Operation::UNKNOWN ||
type == Offer::Operation::LAUNCH ||
type == Offer::Operation::LAUNCH_GROUP) {
```



src/master/metrics.cpp
Lines 637-651 (patched)


Can these counters be decremented? If not, should we add a CHECK to ensure 
that `delta` is always positive?



src/master/metrics.cpp
Lines 684 (patched)


Can counters for terminal states be decremented? I guess not? If I'm 
correct, then we could add a `CHECK` here.


- Gastón Kleiman


On March 7, 2019, 9:01 a.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70116/
> ---
> 
> (Updated March 7, 2019, 9:01 a.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Greg Mann, and Joseph Wu.
> 
> 
> Bugs: MESOS-8241
> https://issues.apache.org/jira/browse/MESOS-8241
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit adds additional metrics counting the
> number of operations in each state.
> 
> Unit tests are added in the subsequent commit.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp aceab3455adfdf7ff6e168e033316997a4d36fb5 
>   src/master/master.cpp 665c1c7979a5ae4ecec0d5a68e59c5419049a4d5 
>   src/master/metrics.hpp 4495e65b6bb11f7236335a702c4f61e7c3f9b0aa 
>   src/master/metrics.cpp 4dd73fb18a06ce8f75c4c1435dba84ade123bee9 
> 
> 
> Diff: https://reviews.apache.org/r/70116/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 70132: Do not implicitly refuse speculatively converted resources.

[Chun-Hung Hsiao]

> On March 7, 2019, 11 p.m., Benjamin Mahler wrote:
> > src/master/master.cpp
> > Lines 5919-5927 (original), 5924-5937 (patched)
> > 
> >
> > I'm lost at what's happening here and why, can you add a guiding 
> > comment?

I added some more high-level comments explaining that we should implicitly 
decline unused but not converted resources. Is it now clear, or do you think I 
should add more explanations here?


- Chun-Hung


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


On March 8, 2019, 12:23 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70132/
> ---
> 
> (Updated March 8, 2019, 12:23 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Meng Zhu.
> 
> 
> Bugs: MESOS-9616
> https://issues.apache.org/jira/browse/MESOS-9616
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently if a framework accepts an offer to perform pipelined
> operations, e.g., reserving resource, without a final consumer, the
> converted resources will be implicitly refused. This is an undesired
> behavior as the framework might want to reserve one resource first but
> launch a task later in the next allocation cycle. This patch fixes this
> behavior.
> 
> But, if the framework accepts an offers with multiple operations that
> cancel out each other, the resources consumed by these operations are
> still considered unused and will be refused.
> 
> 
> Diffs
> -
> 
>   docs/scheduler-http-api.md 8384336bbecf2ca38a3cd203f9db28d931812d65 
>   src/master/master.cpp b9db4ffd4ee8ea4a8e44a35d1afb6c1b8e03d74d 
>   src/tests/slave_tests.cpp 5ee5609af0861e9aecf02a5eaefafe137bd9b843 
> 
> 
> Diff: https://reviews.apache.org/r/70132/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 70132: Do not implicitly refuse speculatively converted resources.

[Chun-Hung Hsiao]

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

(Updated March 8, 2019, 12:23 a.m.)


Review request for mesos, Benjamin Mahler and Meng Zhu.


Changes
---

Addressed BenM's comments.


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


Repository: mesos


Description
---

Currently if a framework accepts an offer to perform pipelined
operations, e.g., reserving resource, without a final consumer, the
converted resources will be implicitly refused. This is an undesired
behavior as the framework might want to reserve one resource first but
launch a task later in the next allocation cycle. This patch fixes this
behavior.

But, if the framework accepts an offers with multiple operations that
cancel out each other, the resources consumed by these operations are
still considered unused and will be refused.


Diffs (updated)
-

  docs/scheduler-http-api.md 8384336bbecf2ca38a3cd203f9db28d931812d65 
  src/master/master.cpp b9db4ffd4ee8ea4a8e44a35d1afb6c1b8e03d74d 
  src/tests/slave_tests.cpp 5ee5609af0861e9aecf02a5eaefafe137bd9b843 


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

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


Testing
---

make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 70137: Made volume gid manager allocate & deallocate gid to non-shared PV.

[Gilbert Song]

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


Ship it!




Ship It!

- Gilbert Song


On March 6, 2019, 6:03 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70137/
> ---
> 
> (Updated March 6, 2019, 6:03 a.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Gilbert Song.
> 
> 
> Bugs: MESOS-8813
> https://issues.apache.org/jira/browse/MESOS-8813
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made volume gid manager allocate & deallocate gid to non-shared PV.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
> 341853a2df74f6ec3135e942b59a5da9d8f8460e 
>   src/slave/containerizer/mesos/isolators/filesystem/posix.cpp 
> 08449e269b550ba41f63b909ffbd5f7a7a83e32b 
>   src/slave/slave.cpp f9b58175ad5433bdfc15a312dfce6e972dea7fa3 
> 
> 
> Diff: https://reviews.apache.org/r/70137/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 70113: Verified volume gid manager's metrics in the related tests.

[Gilbert Song]

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


Ship it!




Ship It!

- Gilbert Song


On March 5, 2019, 6:15 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70113/
> ---
> 
> (Updated March 5, 2019, 6:15 a.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Gilbert Song.
> 
> 
> Bugs: MESOS-9620
> https://issues.apache.org/jira/browse/MESOS-9620
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Verified volume gid manager's metrics in the related tests.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/linux_filesystem_isolator_tests.cpp 
> d32bf74f64a2486b409ea6c34f5c41c553a4af6a 
>   src/tests/containerizer/volume_gid_manager_tests.cpp 
> 129e7018b4829bfa131e5511ae0c388c1736eb35 
>   src/tests/containerizer/volume_sandbox_path_isolator_tests.cpp 
> 9238d58a219c2e46e2e9d492f886a24482935768 
>   src/tests/default_executor_tests.cpp 
> 045e211c948266c6dba0440e75c13a6c7821f793 
>   src/tests/persistent_volume_tests.cpp 
> 8a5672ef117f64c173113df903d9e62bef7253f1 
> 
> 
> Diff: https://reviews.apache.org/r/70113/diff/2/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 70156: Added helper to test for metrics values.

[Gastón Kleiman]

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




src/tests/utils.hpp
Lines 39 (patched)


Nit: I'd name this `EXPECT_METRIC_EQ`.


- Gastón Kleiman


On March 7, 2019, 8:59 a.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70156/
> ---
> 
> (Updated March 7, 2019, 8:59 a.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Greg Mann, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added helper to test for metrics values.
> 
> 
> Diffs
> -
> 
>   src/tests/utils.hpp b2f22cd4db223d167aa35109cd8de6df82ed1f4d 
> 
> 
> Diff: https://reviews.apache.org/r/70156/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 70115: Updated comment about operations.

[Gastón Kleiman]

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


Ship it!




Ship It!

- Gastón Kleiman


On March 4, 2019, 9 a.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70115/
> ---
> 
> (Updated March 4, 2019, 9 a.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Greg Mann, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> A comment about operation feedback mentioned the master sending 
> `OPERATION_DROPPED` updates for certain operations. However, such updates are 
> only sent by agents; the master will send `OPERATION_ERROR` updates instead.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 665c1c7979a5ae4ecec0d5a68e59c5419049a4d5 
> 
> 
> Diff: https://reviews.apache.org/r/70115/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 70132: Do not implicitly refuse speculatively converted resources.

[Benjamin Mahler]

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



Thank you for finding this and fixing it!

Can you expand the description here about why these bugs happen? Was it that as 
new changes were done, the existing logic didn't behave as one would expect? 
Something else? I can't quite follow what the issue is and why, even though it 
was already explained to me and I understood it at one point, so someone fresh 
is probably going to be puzzled.


docs/scheduler-http-api.md
Line 132 (original), 132 (patched)


Not yours, but we should really split up this very long line and break at 
80 or so (thought we had a markdown linter..), it would have the nice 
additional effect of making the diff work in reviewboard



src/master/master.cpp
Lines 5918-5919 (patched)


Some additional context here would be great, because the variable names 
seem unhelpful? The distinction between `_offeredResources` and 
`offeredResoures` isn't obvious and looking at the code above it seems as 
though `_offeredResources` is already representing "unused resources"? 
("consumed resources" are subtracted from it)

A guiding high level comment would help here, I'm left puzzled by how these 
two calculations are computing what their names describe.



src/master/master.cpp
Lines 5919-5927 (original), 5924-5937 (patched)


I'm lost at what's happening here and why, can you add a guiding comment?



src/master/master.cpp
Line 10812 (original), 10824 (patched)


Can you just commit this directly so that we don't pollute this patch with 
this unrelated typo fix?


- Benjamin Mahler


On March 6, 2019, 5:07 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70132/
> ---
> 
> (Updated March 6, 2019, 5:07 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Meng Zhu.
> 
> 
> Bugs: MESOS-9616
> https://issues.apache.org/jira/browse/MESOS-9616
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently if a framework accepts an offer to perform pipelined
> operations, e.g., reserving resource, without a final consumer, the
> converted resources will be implicitly refused. This is an undesired
> behavior as the framework might want to reserve one resource first but
> launch a task later in the next allocation cycle. This patch fixes this
> behavior.
> 
> But, if the framework accepts an offers with multiple operations that
> cancel out each other, the resources consumed by these operations are
> still considered unused and will be refused.
> 
> 
> Diffs
> -
> 
>   docs/scheduler-http-api.md 8384336bbecf2ca38a3cd203f9db28d931812d65 
>   src/master/master.cpp 015da54583448a8d102d8e401e48bd228baf6dd6 
>   src/tests/slave_tests.cpp 22a0295086ae4f4ec26df00a0e077eecfa27f1fb 
> 
> 
> Diff: https://reviews.apache.org/r/70132/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 70142: Added ARCHIVE_EXTRACT_SECURE_NODOTDOT flag to archiver default.

[Gilbert Song]

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


Ship it!




Ship It!

- Gilbert Song


On March 6, 2019, 11:35 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70142/
> ---
> 
> (Updated March 6, 2019, 11:35 a.m.)
> 
> 
> Review request for mesos, Andrei Budnik, Gilbert Song, and Greg Mann.
> 
> 
> Bugs: MESOS-9610
> https://issues.apache.org/jira/browse/MESOS-9610
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This enables a security flag provided by libarchive, which disallows
> extraction of archives that contain '..' in hardlinks or files.
> Without this flag, it is possible to provide the archiver with
> an archive and overwrite arbitrary files in the user's parent directory
> or further up.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/archiver.hpp 
> 2447797ee05f48ab6d8b046d862aede8dec36bea 
>   3rdparty/stout/tests/archiver_tests.cpp 
> cdf24a5d9accb1082e8bf3809c865a92d93e63d8 
> 
> 
> Diff: https://reviews.apache.org/r/70142/diff/1/
> 
> 
> Testing
> ---
> 
> ```
> cmake --build . --target stout-tests
> 3rdparty/stout/tests/stout-tests --gtest_filter="*Archiver*"
> ```
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 70154: Properly handled disk resources in operator API `CREATE` handler.

[Benjamin Bannier]

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

(Updated March 7, 2019, 10:50 p.m.)


Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.


Changes
---

Use getConsumedResources as suggested by Chun


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


Repository: mesos


Description
---

This code was previously assuming that any `CREATE` operation would add
a `DiskInfo` to resources (to hold the `PersistenceInfo`). To compute
the resources required to perform the operation it then just removed the
full `DiskInfo`. This is incorrect as e.g., CSI disk resources carry
information unrelated to persistence in their `DiskInfo`.

This patch fixes `CREATE` handling by leveraging resource conversions
which were introduced in the meantime. This allows extracting the
required resource in a lower scope which allows us refactor handlers for
other operator API calls. With that less information about the layout of
operations is needed here.


Diffs (updated)
-

  src/master/http.cpp 012ee4f205ab3f2678b058fb0f3ebdac4336eca4 
  src/master/master.hpp 90e08149ece595147ca4a93da215385917a0f372 


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

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


Testing
---

`make check`


Thanks,

Benjamin Bannier

Review Request 70158: WIP.

[Vinod Kone]

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

Review request for mesos.


Repository: mesos


Description
---

WIP.


Diffs
-

  NOTICE aa2fc8b2124bac35eb6e22c7188a8e38ef4d37a2 


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


Testing
---

Testing RB webhook trigger.


Thanks,

Vinod Kone

Re: Review Request 70103: Removed outdated scaling framework.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On March 3, 2019, 12:06 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70103/
> ---
> 
> (Updated March 3, 2019, 12:06 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This old code was never updated for recent changes, is not documented,
> and currently does not work anymore. Removing it for now. Should we
> decide to bring it back in the future we should make sure it is tested
> automatically so it does not go out of sync again, and also that it is
> documented and discoverable by users.
> 
> 
> Diffs
> -
> 
>   src/scaling/nested_exec 45ba1260e476ee20dfe013a0406f650639e9a800 
>   src/scaling/nested_exec.py 28ccc3f5af2d293be8d810f7082dd43388ab4435 
>   src/scaling/scaling_exec 3be8bc8b5bf7a1f4386db2dda82b8663d522d065 
>   src/scaling/scaling_exec.py e9f4a86b29485559323241587879fab946e69841 
>   src/scaling/scaling_sched 5c6c2476ed6a0b1275011c394f6834eef7b3a134 
>   src/scaling/scaling_sched.py 011e353b12465076d96378cccbc5a0ff88231844 
> 
> 
> Diff: https://reviews.apache.org/r/70103/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 70154: Properly handled disk resources in operator API `CREATE` handler.

[Chun-Hung Hsiao]

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


Fix it, then Ship it!




I didn't notice until now that we don't have `src/master/http.hpp` lol.


src/master/http.cpp
Lines 3991-4003 (patched)


We can use `protobuf::getConsumedResources` instead.


- Chun-Hung Hsiao


On March 7, 2019, 1:41 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70154/
> ---
> 
> (Updated March 7, 2019, 1:41 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.
> 
> 
> Bugs: MESOS-9637
> https://issues.apache.org/jira/browse/MESOS-9637
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This code was previously assuming that any `CREATE` operation would add
> a `DiskInfo` to resources (to hold the `PersistenceInfo`). To compute
> the resources required to perform the operation it then just removed the
> full `DiskInfo`. This is incorrect as e.g., CSI disk resources carry
> information unrelated to persistence in their `DiskInfo`.
> 
> This patch fixes `CREATE` handling by leveraging resource conversions
> which were introduced in the meantime. This allows extracting the
> required resource in a lower scope which allows us refactor handlers for
> other operator API calls. With that less information about the layout of
> operations is needed here.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 012ee4f205ab3f2678b058fb0f3ebdac4336eca4 
>   src/master/master.hpp 90e08149ece595147ca4a93da215385917a0f372 
> 
> 
> Diff: https://reviews.apache.org/r/70154/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 70156: Added helper to test for metrics values.

[Joseph Wu]

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



Hum... I wonder if it is worthwhile to make this a normal function call instead.
```

bool metricsEqual(string metric, T value) {
  JSON::Object metrics = Metrics();
  if (metrics.count(metric) != 1) {
return false;
  }
  
  return metrics.at(metric)->as() == value;
}
```
With usage like:
```
ASSERT_TRUE(metricsEqual("metric/foo", 1));
```


src/tests/utils.hpp
Lines 39 (patched)


Nit: A glance at some other macros show that we space-align the `` 
characters to the right side.



src/tests/utils.hpp
Lines 40 (patched)


Is it necessary to make this a `do { ... } while (0);` instead of just 
enclosing this in a `{ ... }`?



src/tests/utils.hpp
Lines 41 (patched)


I can imagine a name conflict if the test is particularly metric heavy.  
i.e. if the test defines another `metrics` local variable.


- Joseph Wu


On March 7, 2019, 8:59 a.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70156/
> ---
> 
> (Updated March 7, 2019, 8:59 a.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Greg Mann, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added helper to test for metrics values.
> 
> 
> Diffs
> -
> 
>   src/tests/utils.hpp b2f22cd4db223d167aa35109cd8de6df82ed1f4d 
> 
> 
> Diff: https://reviews.apache.org/r/70156/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 70115: Updated comment about operations.

[Joseph Wu]

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


Ship it!




Ship It!

- Joseph Wu


On March 4, 2019, 9 a.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70115/
> ---
> 
> (Updated March 4, 2019, 9 a.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Greg Mann, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> A comment about operation feedback mentioned the master sending 
> `OPERATION_DROPPED` updates for certain operations. However, such updates are 
> only sent by agents; the master will send `OPERATION_ERROR` updates instead.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 665c1c7979a5ae4ecec0d5a68e59c5419049a4d5 
> 
> 
> Diff: https://reviews.apache.org/r/70115/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 70142: Added ARCHIVE_EXTRACT_SECURE_NODOTDOT flag to archiver default.

[Joseph Wu]

> On March 7, 2019, 6:12 a.m., Andrei Budnik wrote:
> > 3rdparty/stout/include/stout/archiver.hpp
> > Line 43 (original), 43 (patched)
> > 
> >
> > `libarchive` supports the 
> > `ARCHIVE_EXTRACT_SECURE_NOABSOLUTEPATHS` flag:
> > 
> > https://github.com/libarchive/libarchive/blob/f77e06a338a9b541de406da5a03f0bda8c00/libarchive/archive.h#L692-L693
> > 
> > Should we use this flag by default as well?

We should.

I'll write a separate patch to add this however, since the unit test will 
(probably) exercise a different exploit.


- Joseph


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


On March 6, 2019, 11:35 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70142/
> ---
> 
> (Updated March 6, 2019, 11:35 a.m.)
> 
> 
> Review request for mesos, Andrei Budnik, Gilbert Song, and Greg Mann.
> 
> 
> Bugs: MESOS-9610
> https://issues.apache.org/jira/browse/MESOS-9610
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This enables a security flag provided by libarchive, which disallows
> extraction of archives that contain '..' in hardlinks or files.
> Without this flag, it is possible to provide the archiver with
> an archive and overwrite arbitrary files in the user's parent directory
> or further up.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/archiver.hpp 
> 2447797ee05f48ab6d8b046d862aede8dec36bea 
>   3rdparty/stout/tests/archiver_tests.cpp 
> cdf24a5d9accb1082e8bf3809c865a92d93e63d8 
> 
> 
> Diff: https://reviews.apache.org/r/70142/diff/1/
> 
> 
> Testing
> ---
> 
> ```
> cmake --build . --target stout-tests
> 3rdparty/stout/tests/stout-tests --gtest_filter="*Archiver*"
> ```
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 70123: Added rvalue overload for `Result::get()`.

[Benjamin Mahler]

> On March 5, 2019, 10:09 p.m., Benjamin Bannier wrote:
> > 3rdparty/stout/include/stout/result.hpp
> > Lines 129 (patched)
> > 
> >
> > ```
> > -> decltype(std::forward(self)
> >   .data
> >   .get() // NOLINT(mesos-redundant-get)
> >   .get())
> > ```
> > 
> > See below.

Can't we just use two star operators here given the other outstanding patches?


- Benjamin


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


On March 4, 2019, 11:53 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70123/
> ---
> 
> (Updated March 4, 2019, 11:53 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added rvalue overload for `Result::get()`.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/result.hpp 
> c1387ae957edffc31250b9b236b5f1fd8ff0acd3 
> 
> 
> Diff: https://reviews.apache.org/r/70123/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 70067: Added star operator for `Result`, `Try` and `Option`.

[Benjamin Mahler]

> On March 2, 2019, 3:24 p.m., Benjamin Mahler wrote:
> > 3rdparty/stout/tests/option_tests.cpp
> > Lines 137 (patched)
> > 
> >
> > this doesn't test that moving is working correctly? Can we test to 
> > ensure that this operator lets just move?
> > 
> > ```
> > Option> moved = {{1,1}};
> > 
> > hashmap map = *std::move(moved);
> > 
> > // Expect moved is empty
> > // Expect map is {1,1}
> > ```
> 
> Benjamin Bannier wrote:
> Hmm, is there anything in the standard promising that the source of a 
> `move` gets emptied out? I am not sure adding an expectation related to 
> `move`.
> 
> Meng Zhu wrote:
> I do not think we need to test content after the move, just need to test 
> the returned content of the `*` which the code has already tested. Dropping, 
> Bmahler please feel free to re-open if you think otherwise.

As it stands, the test would compile if it actually was copying instead of 
moving? Or no?

Seems prudent to test a move is actually occurring, if there's nothing in std 
containers to rely on w.r.t post-move invariants, we can just have our own 
struct that stores a boolean indicating that it's been moved and we can test 
that to make sure the move actually occurs?


- Benjamin


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


On March 4, 2019, 10:50 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70067/
> ---
> 
> (Updated March 4, 2019, 10:50 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7124
> https://issues.apache.org/jira/browse/MESOS-7124
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added star operator for `Result`, `Try` and `Option`.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/option.hpp 
> 8feed012a55fed6eab89c883958324f3345e46e9 
>   3rdparty/stout/include/stout/result.hpp 
> c1387ae957edffc31250b9b236b5f1fd8ff0acd3 
>   3rdparty/stout/include/stout/try.hpp 
> 30cce7e27fa040d0ce5a86efc4820f8c39178444 
>   3rdparty/stout/tests/option_tests.cpp 
> 815d40e0bf342998634dc69a1719c3f717c7202c 
>   3rdparty/stout/tests/result_tests.cpp 
> 1750e6b4a90afec3b7de0621779f8b69a856da41 
>   3rdparty/stout/tests/try_tests.cpp 99b7b2a3f968abb87549fb9075d65bc5bcc827f1 
> 
> 
> Diff: https://reviews.apache.org/r/70067/diff/4/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 70148: Added `>`, `<` and `>=` operators to `Value::Scalar`.

[Benjamin Mahler]

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


Ship it!




Ship It!

- Benjamin Mahler


On March 7, 2019, 2:38 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70148/
> ---
> 
> (Updated March 7, 2019, 2:38 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `>`, `<` and `>=` operators to `Value::Scalar`.
> 
> 
> Diffs
> -
> 
>   include/mesos/v1/values.hpp 03a19ee44b3d7f801f7ca2a61f1d42d51e0c144b 
>   include/mesos/values.hpp 0d09729ccfa84c7ebb11e005fb30b8bb529d3120 
>   src/common/values.cpp c788302b928747a1aed66efd4d654711410928f1 
>   src/v1/values.cpp 5fd9dc5a03ec7b8f6ed31acb3f868764a45b2bfd 
> 
> 
> Diff: https://reviews.apache.org/r/70148/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 70116: Added metrics for offer operation feedback.

[Benno Evers]

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

(Updated March 7, 2019, 5:01 p.m.)


Review request for mesos, Gastón Kleiman, Greg Mann, and Joseph Wu.


Changes
---

Added per-operation metrics; addressed comments.


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


Repository: mesos


Description (updated)
---

This commit adds additional metrics counting the
number of operations in each state.

Unit tests are added in the subsequent commit.


Diffs (updated)
-

  src/master/master.hpp aceab3455adfdf7ff6e168e033316997a4d36fb5 
  src/master/master.cpp 665c1c7979a5ae4ecec0d5a68e59c5419049a4d5 
  src/master/metrics.hpp 4495e65b6bb11f7236335a702c4f61e7c3f9b0aa 
  src/master/metrics.cpp 4dd73fb18a06ce8f75c4c1435dba84ade123bee9 


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

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


Testing
---


Thanks,

Benno Evers

Re: Review Request 70117: WIP: Add unit tests for offer operation metrics.

[Benno Evers]

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

(Updated March 7, 2019, 5:01 p.m.)


Review request for mesos, Gastón Kleiman, Greg Mann, and Joseph Wu.


Changes
---

Use re-named metrics names.


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


Repository: mesos


Description
---

WIP: Add unit tests for offer operation metrics.


Diffs (updated)
-

  src/tests/agent_operation_feedback_tests.cpp 
5a8f54c7c53272e90ed5fa6366e8154cedf1375f 
  src/tests/api_tests.cpp f241064dc8597972299a424958e759588f7e4fd2 
  src/tests/master_slave_reconciliation_tests.cpp 
002be27bf0731e2dba8937697b347cd1dd0a 
  src/tests/master_tests.cpp 5ae8e1cea3ca87551093bd63d744ac807ac7797a 
  src/tests/operation_reconciliation_tests.cpp 
6a815ad694e2a608ce324715c920833f825793a0 
  src/tests/persistent_volume_endpoints_tests.cpp 
40d7e6a30c9c11eb84f4bd5aca92cfcecb3e0eb7 
  src/tests/reservation_endpoints_tests.cpp 
b1897592797c40574de7995b2335f2b4bc5fc699 
  src/tests/scheduler_tests.cpp 5fb696061248c877bfa86727f146051aee26cb58 
  src/tests/slave_tests.cpp 22a0295086ae4f4ec26df00a0e077eecfa27f1fb 
  src/tests/storage_local_resource_provider_tests.cpp 
75f0d818a94d7a15d1df169683724294e07fdb69 


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

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


Testing
---


Thanks,

Benno Evers

Review Request 70156: Added helper to test for metrics values.

[Benno Evers]

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

Review request for mesos, Gastón Kleiman, Greg Mann, and Joseph Wu.


Repository: mesos


Description
---

Added helper to test for metrics values.


Diffs
-

  src/tests/utils.hpp b2f22cd4db223d167aa35109cd8de6df82ed1f4d 


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


Testing
---


Thanks,

Benno Evers

Re: Review Request 70154: Properly handled disk resources in operator API `CREATE` handler.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [70154]

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

- Mesos Reviewbot


On March 7, 2019, 2:41 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70154/
> ---
> 
> (Updated March 7, 2019, 2:41 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.
> 
> 
> Bugs: MESOS-9637
> https://issues.apache.org/jira/browse/MESOS-9637
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This code was previously assuming that any `CREATE` operation would add
> a `DiskInfo` to resources (to hold the `PersistenceInfo`). To compute
> the resources required to perform the operation it then just removed the
> full `DiskInfo`. This is incorrect as e.g., CSI disk resources carry
> information unrelated to persistence in their `DiskInfo`.
> 
> This patch fixes `CREATE` handling by leveraging resource conversions
> which were introduced in the meantime. This allows extracting the
> required resource in a lower scope which allows us refactor handlers for
> other operator API calls. With that less information about the layout of
> operations is needed here.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 012ee4f205ab3f2678b058fb0f3ebdac4336eca4 
>   src/master/master.hpp 90e08149ece595147ca4a93da215385917a0f372 
> 
> 
> Diff: https://reviews.apache.org/r/70154/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 70142: Added ARCHIVE_EXTRACT_SECURE_NODOTDOT flag to archiver default.

[Andrei Budnik]

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




3rdparty/stout/include/stout/archiver.hpp
Line 43 (original), 43 (patched)


`libarchive` supports the 
`ARCHIVE_EXTRACT_SECURE_NOABSOLUTEPATHS` flag:

https://github.com/libarchive/libarchive/blob/f77e06a338a9b541de406da5a03f0bda8c00/libarchive/archive.h#L692-L693

Should we use this flag by default as well?


- Andrei Budnik


On March 6, 2019, 7:35 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70142/
> ---
> 
> (Updated March 6, 2019, 7:35 p.m.)
> 
> 
> Review request for mesos, Andrei Budnik, Gilbert Song, and Greg Mann.
> 
> 
> Bugs: MESOS-9610
> https://issues.apache.org/jira/browse/MESOS-9610
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This enables a security flag provided by libarchive, which disallows
> extraction of archives that contain '..' in hardlinks or files.
> Without this flag, it is possible to provide the archiver with
> an archive and overwrite arbitrary files in the user's parent directory
> or further up.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/archiver.hpp 
> 2447797ee05f48ab6d8b046d862aede8dec36bea 
>   3rdparty/stout/tests/archiver_tests.cpp 
> cdf24a5d9accb1082e8bf3809c865a92d93e63d8 
> 
> 
> Diff: https://reviews.apache.org/r/70142/diff/1/
> 
> 
> Testing
> ---
> 
> ```
> cmake --build . --target stout-tests
> 3rdparty/stout/tests/stout-tests --gtest_filter="*Archiver*"
> ```
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 70154: Properly handled disk resources in operator API `CREATE` handler.

[Jan Schlicht]

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


Ship it!




Ship It!

- Jan Schlicht


On March 7, 2019, 2:41 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70154/
> ---
> 
> (Updated March 7, 2019, 2:41 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.
> 
> 
> Bugs: MESOS-9637
> https://issues.apache.org/jira/browse/MESOS-9637
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This code was previously assuming that any `CREATE` operation would add
> a `DiskInfo` to resources (to hold the `PersistenceInfo`). To compute
> the resources required to perform the operation it then just removed the
> full `DiskInfo`. This is incorrect as e.g., CSI disk resources carry
> information unrelated to persistence in their `DiskInfo`.
> 
> This patch fixes `CREATE` handling by leveraging resource conversions
> which were introduced in the meantime. This allows extracting the
> required resource in a lower scope which allows us refactor handlers for
> other operator API calls. With that less information about the layout of
> operations is needed here.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 012ee4f205ab3f2678b058fb0f3ebdac4336eca4 
>   src/master/master.hpp 90e08149ece595147ca4a93da215385917a0f372 
> 
> 
> Diff: https://reviews.apache.org/r/70154/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 70154: Properly handled disk resources in operator API `CREATE` handler.

[Benjamin Bannier]

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

(Updated March 7, 2019, 2:41 p.m.)


Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.


Changes
---

Push handling to a smaller scope, refactor an additional handler


Summary (updated)
-

Properly handled disk resources in operator API `CREATE` handler.


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


Repository: mesos


Description (updated)
---

This code was previously assuming that any `CREATE` operation would add
a `DiskInfo` to resources (to hold the `PersistenceInfo`). To compute
the resources required to perform the operation it then just removed the
full `DiskInfo`. This is incorrect as e.g., CSI disk resources carry
information unrelated to persistence in their `DiskInfo`.

This patch fixes `CREATE` handling by leveraging resource conversions
which were introduced in the meantime. This allows extracting the
required resource in a lower scope which allows us refactor handlers for
other operator API calls. With that less information about the layout of
operations is needed here.


Diffs (updated)
-

  src/master/http.cpp 012ee4f205ab3f2678b058fb0f3ebdac4336eca4 
  src/master/master.hpp 90e08149ece595147ca4a93da215385917a0f372 


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

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


Testing
---

`make check`


Thanks,

Benjamin Bannier

Review Request 70154: Properly handled disk resources in operator API.

[Benjamin Bannier]

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

Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.


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


Repository: mesos


Description
---

This code was previously assuming that any `CREATE` operation would add
a `DiskInfo` to resources (to hold the `PersistenceInfo`). To compute
the resources required to perform the operation it then just removed the
full `DiskInfo`. This is incorrect as e.g., CSI disk resources carry
information unrelated to persistence in their `DiskInfo`.

This patch fixes `CREATE` handling by leveraging resource conversions
which were introduced in the meantime. We also refactor handlers for
other operator API calls to use the same helpers. With that less
information about the layout of operations is needed here.


Diffs
-

  src/master/http.cpp 012ee4f205ab3f2678b058fb0f3ebdac4336eca4 


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


Testing
---

`make check`


Thanks,

Benjamin Bannier

Re: Review Request 70152: Added tests for `ResourceLimits`.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [70061, 70062, 70069, 70063, 70148, 70149, 70150, 70151, 70152]

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

- Mesos Reviewbot


On March 7, 2019, 8:03 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70152/
> ---
> 
> (Updated March 7, 2019, 8:03 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9608
> https://issues.apache.org/jira/browse/MESOS-9608
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added tests for `ResourceLimits`.
> 
> 
> Diffs
> -
> 
>   src/tests/resource_quantities_tests.cpp 
> 435a4949b95e9a83be73781388eb4be9c7da695b 
> 
> 
> Diff: https://reviews.apache.org/r/70152/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 70098: Removed `support/cpplint.patch`.

[Alexander Rukletsov]

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


Ship it!




Ship It!

- Alexander Rukletsov


On March 3, 2019, 1:01 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70098/
> ---
> 
> (Updated March 3, 2019, 1:01 a.m.)
> 
> 
> Review request for mesos and Armand Grillet.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change removes explicit tracking of a patch against upstream
> cpplint in the tree and replaces it with a clear reference against the
> upstream repository and revision used in the modified file.
> 
> The patch was never used by us against any upstream version, and was
> not e.g., a legal requirement. The process around creating the patch
> was cumbersome and error prone. Removing the patch from the tree
> should not only make it easier to evolve our modifications going
> forward, but also remove a lot of noise from changes.
> 
> 
> Diffs
> -
> 
>   support/cpplint.patch 2427be87559e6dd21c8841f1719064b5d46e4fc7 
>   support/cpplint.py d3e7aaf71255cf4efd0239434f7630afd6ea47d0 
> 
> 
> Diff: https://reviews.apache.org/r/70098/diff/2/
> 
> 
> Testing
> ---
> 
> N/A
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 70150: Added a constructor for `ResourceQuantities`.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [70061, 70062, 70069, 70063, 70148, 70149, 70150]

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

- Mesos Reviewbot


On March 6, 2019, 11:04 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70150/
> ---
> 
> (Updated March 6, 2019, 11:04 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The constructor constructs `ResourceQuantities`
> from `google::protobuf::Map`.
> It replaces one of the current constructors that takes
> in `OfferFilters::ResourceQuantities`.
> 
> 
> Diffs
> -
> 
>   src/common/resource_quantities.hpp 31ce7b98a8256173d6ad26e2f095373a01d7baae 
>   src/common/resource_quantities.cpp 1c8eec03580abf86df4ce947c517a74b0a8e09a7 
>   src/master/allocator/mesos/hierarchical.cpp 
> 7085c23eeb163854712391e24d902902b8aaf162 
> 
> 
> Diff: https://reviews.apache.org/r/70150/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Review Request 70152: Added tests for `ResourceLimits`.

[Meng Zhu]

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

Added tests for `ResourceLimits`.


Diffs
-

  src/tests/resource_quantities_tests.cpp 
435a4949b95e9a83be73781388eb4be9c7da695b 


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


Testing
---

make check


Thanks,

Meng Zhu