Review Request 64909: Removed unnecessary `SocketManager::dispose` data structure.

[Benjamin Hindman]

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

Review request for mesos and Benjamin Mahler.


Repository: mesos


Description
---

We used `dispose` to capture the sockets that `HttpProxy` did not want
to persist but now that `HttpProxy` does not use `SocketManager` we no
longer need to use `dispose` because `temps` is sufficient for knowing
which sockets are temporary.


Diffs
-

  3rdparty/libprocess/src/process.cpp 75cf1d3b6d3d257ba9bc81c68017a74a6511cebf 
  3rdparty/libprocess/src/socket_manager.hpp 
dd4d111c4ae579420060e547dd12f8f0711c 


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


Testing
---

make check


Thanks,

Benjamin Hindman

Review Request 64907: Added abandonment and process exit support to `loop()`.

[Benjamin Hindman]

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

Review request for mesos and Benjamin Mahler.


Repository: mesos


Description
---

The current semantics for `loop()` mean that we'll leak resources
under two different circumstances:

  (1) If either the "iteration" or "body" functions return future's
  that are abandoned.

  (2) If the loop is using a process that exits.

This patch adds support to make sure that abandoned futures are
properly propagated in the event that either of these situations
occurs.


Diffs
-

  3rdparty/libprocess/include/process/loop.hpp 
69d90f3e1b16189e0b1a6f1981e8509c18b38465 
  3rdparty/libprocess/src/tests/loop_tests.cpp 
8d1837a0baedc12591f92c8f0f8ea83d0aa44ab0 


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


Testing
---

make check


Thanks,

Benjamin Hindman

Review Request 64908: Refactored `HttpProxy` to not rely on `SocketManager`.

[Benjamin Hindman]

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

Review request for mesos and Benjamin Mahler.


Repository: mesos


Description
---

This simplifies the introduction of `http::Server` so that it's easier
to reason about and in the future will make removing the `HttpProxy`
implementation much easier.


Diffs
-

  3rdparty/libprocess/src/http_proxy.hpp 
5b6e7e8786ed9eab50cd4c2cfdec455c92d72eca 
  3rdparty/libprocess/src/http_proxy.cpp 
f584238aadd86875d7c87736653f394e716397de 
  3rdparty/libprocess/src/process.cpp 75cf1d3b6d3d257ba9bc81c68017a74a6511cebf 
  3rdparty/libprocess/src/socket_manager.hpp 
dd4d111c4ae579420060e547dd12f8f0711c 


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


Testing
---

make check


Thanks,

Benjamin Hindman

Review Request 64906: Exposed `send()` helpers for `Socket`'s.

[Benjamin Hindman]

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

Review request for mesos and Benjamin Mahler.


Repository: mesos


Description
---

Pulled out two `send()` functions that simplify sending an
`http::Response` and an `Encoder` using a `Socket`.


Diffs
-

  3rdparty/libprocess/include/process/http.hpp 
055447e13117c0a3ba79d0fc326ece657e8f064f 
  3rdparty/libprocess/src/encoder.hpp 70b5ec479e90c0eb6ac729b465739b581729a956 
  3rdparty/libprocess/src/http.cpp f51d2aaa639ee36ce960268cb32a3aa88f14aa29 


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


Testing
---

make check


Thanks,

Benjamin Hindman

Re: Review Request 64868: Added initial doc about CSI support in Mesos.

[Jie Yu]

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

(Updated Jan. 3, 2018, 5:09 a.m.)


Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, Gaston Kleiman, 
Greg Mann, Joseph Wu, and Jan Schlicht.


Changes
---

Addressed comments.


Repository: mesos


Description
---

Added initial doc about CSI support in Mesos.


Diffs (updated)
-

  docs/csi.md PRE-CREATION 
  docs/home.md 0fa901fce91cc5e385c310f5e441c85678568547 
  docs/images/csi-architecture.png PRE-CREATION 
  docs/operator-http-api.md af6a3a6ec7d80859b640c8b7db49f93a803d55d0 


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

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


Testing
---

The rendering can be checked here:
https://github.com/jieyu/mesos/blob/csi_doc/docs/csi.md


Thanks,

Jie Yu

Re: Review Request 64899: Removed `nodist_liburi_volume_profile_SOURCES`.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Jan. 2, 2018, 10:48 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64899/
> ---
> 
> (Updated Jan. 2, 2018, 10:48 p.m.)
> 
> 
> Review request for mesos, Jie Yu and James Peach.
> 
> 
> Bugs: MESOS-8370
> https://issues.apache.org/jira/browse/MESOS-8370
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adding generated protobuf headers into `nodist_*_SOURCES` for modules
> doesn't enfource build dependencies in the generated makefile so the
> lines are removed.
> 
> NOTE: Currently `liburi_volume_profile.la` cannot be built standalone.
> The issue is tracked in MESOS-8370.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am b83cddaaf874b669c0b8f9df8805fc88e8c3e90d 
> 
> 
> Diff: https://reviews.apache.org/r/64899/diff/1/
> 
> 
> Testing
> ---
> 
> make distcheck
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 64901: Renamed ACL fields for standalone container API for consistency.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Jan. 3, 2018, 12:49 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64901/
> ---
> 
> (Updated Jan. 3, 2018, 12:49 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Renamed ACL fields for standalone container API for consistency.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/acls.proto 
> 152410f90b98ac43a45a18661c5cbd10bff1dd63 
>   src/authorizer/local/authorizer.cpp 
> b3ec601e408cd6055ba77d25773ccf35bdd7c124 
> 
> 
> Diff: https://reviews.apache.org/r/64901/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 64868: Added initial doc about CSI support in Mesos.

[Jie Yu]

- Jie


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


On Dec. 29, 2017, 4:53 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64868/
> ---
> 
> (Updated Dec. 29, 2017, 4:53 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, Gaston Kleiman, 
> Greg Mann, Joseph Wu, and Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added initial doc about CSI support in Mesos.
> 
> 
> Diffs
> -
> 
>   docs/csi.md PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/64868/diff/2/
> 
> 
> Testing
> ---
> 
> The rendering can be checked here:
> https://github.com/jieyu/mesos/blob/csi_doc/docs/csi.md
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 64868: Added initial doc about CSI support in Mesos.

[Jie Yu]

> On Dec. 29, 2017, 3:03 p.m., James DeFelice wrote:
> > docs/csi.md
> > Lines 385 (patched)
> > 
> >
> > I'd like to see it called out clearly in the text (vs. being embedded 
> > in a link) the specific version of CSI that Mesos currently supports. Maybe 
> > this is already called out and I missed it?

added somne text on the top.


- Jie


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


On Dec. 29, 2017, 4:53 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64868/
> ---
> 
> (Updated Dec. 29, 2017, 4:53 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, Gaston Kleiman, 
> Greg Mann, Joseph Wu, and Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added initial doc about CSI support in Mesos.
> 
> 
> Diffs
> -
> 
>   docs/csi.md PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/64868/diff/2/
> 
> 
> Testing
> ---
> 
> The rendering can be checked here:
> https://github.com/jieyu/mesos/blob/csi_doc/docs/csi.md
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 64868: Added initial doc about CSI support in Mesos.

[Jie Yu]

> On Dec. 29, 2017, 3:03 p.m., James DeFelice wrote:
> > docs/csi.md
> > Lines 151 (patched)
> > 
> >
> > Can we make it clear that we're talking about a hypothetical EBS 
> > plugin? AFAIK the external resource provider stuff isn't in place yet (and 
> > may contain unexpected monsters)

I used LVM instead.


> On Dec. 29, 2017, 3:03 p.m., James DeFelice wrote:
> > docs/csi.md
> > Lines 369 (patched)
> > 
> >
> > it's worth noting that because this is based on the CSI specification, 
> > there's an implicit dependency between backward compatibility of the module 
> > interface and the CSI specification version. In other words, there's 
> > already a compatiblity scheme for Mesos modules that was crafted prior to 
> > CSI. CSI doesn't provide a backward compatibility promise. As CSI evolves, 
> > this module interface may implicitly evolve in a way that deviates from the 
> > compatibility promise described here: 
> > https://github.com/apache/mesos/blob/master/docs/modules.md#module-versioning-and-backwards-compatibility
> > 
> > .. or, maybe I misunderstand our guarantees re: module backwards compat.

Clarified, module has to be rebuilt against each release of Mesos.


- Jie


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


On Dec. 29, 2017, 4:53 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64868/
> ---
> 
> (Updated Dec. 29, 2017, 4:53 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, Gaston Kleiman, 
> Greg Mann, Joseph Wu, and Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added initial doc about CSI support in Mesos.
> 
> 
> Diffs
> -
> 
>   docs/csi.md PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/64868/diff/2/
> 
> 
> Testing
> ---
> 
> The rendering can be checked here:
> https://github.com/jieyu/mesos/blob/csi_doc/docs/csi.md
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 64739: Upgraded resources that come from `protobuf::read`.

[Michael Park]

> On Jan. 2, 2018, 2:39 p.m., Benjamin Mahler wrote:
> > Is there a ticket for being more robust in the face of inability to recover 
> > a downgraded agent? Can you file one if not?
> > Also, do we want to target a ticket for 2.1 that stops downgrading? I 
> > assume the strategy to end the 1.x deprecation cycle will be that 2.1+ only 
> > supports being upgraded from 2.0.x.

Found the "downgrade capability" ticket: 
https://issues.apache.org/jira/browse/MESOS-7682


- Michael


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


On Jan. 2, 2018, 3:39 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64739/
> ---
> 
> (Updated Jan. 2, 2018, 3:39 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Upgraded resources that come from `protobuf::read`.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> ee9ac901a6f46b6c6749381d859b3b090c113673 
>   src/slave/containerizer/mesos/paths.cpp 
> 8a188a918873eef468a984b80f5ea7ebaa8fb923 
> 
> 
> Diff: https://reviews.apache.org/r/64739/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Review Request 64901: Renamed ACL fields for standalone container API for consistency.

[Chun-Hung Hsiao]

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

Review request for mesos, Jie Yu and Joseph Wu.


Repository: mesos


Description
---

Renamed ACL fields for standalone container API for consistency.


Diffs
-

  include/mesos/authorizer/acls.proto 152410f90b98ac43a45a18661c5cbd10bff1dd63 
  src/authorizer/local/authorizer.cpp b3ec601e408cd6055ba77d25773ccf35bdd7c124 


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


Testing
---

make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 64868: Added initial doc about CSI support in Mesos.

[Jie Yu]

> On Dec. 29, 2017, 3:03 p.m., James DeFelice wrote:
> > docs/csi.md
> > Lines 137 (patched)
> > 
> >
> > > Any disk resource ... inherits the same profile as the storage pool.
> > 
> > Why, and who enforces this? Is this because a single logical pool may 
> > have multiple, virtual representations (one per profile)? If so it might be 
> > worth differentiating between a logical pool and a virtual one.

I don't want to overcomplicate here. I think the definition is pretty clear "A 
RAW disk resource not backed by a CSI volume is referred to as a storage pool". 
There's a TODO below capture the explanation of correlated resources, which 
will probably clarify that.


- Jie


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


On Dec. 29, 2017, 4:53 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64868/
> ---
> 
> (Updated Dec. 29, 2017, 4:53 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, Gaston Kleiman, 
> Greg Mann, Joseph Wu, and Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added initial doc about CSI support in Mesos.
> 
> 
> Diffs
> -
> 
>   docs/csi.md PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/64868/diff/2/
> 
> 
> Testing
> ---
> 
> The rendering can be checked here:
> https://github.com/jieyu/mesos/blob/csi_doc/docs/csi.md
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 64898: Removed duplicated code that tests for removable tasks.

[James Peach]

> On Jan. 2, 2018, 10:43 p.m., Gaston Kleiman wrote:
> > I think that you need to rebase this RR.

Thanks!


- James


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


On Jan. 3, 2018, 12:26 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64898/
> ---
> 
> (Updated Jan. 3, 2018, 12:26 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When we get a task update and check whether the latest state
> indicated that the task is removable, invoke a local lambda
> function so that we don't duplicate the same code for both
> the current and latest states.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 03eb178fa1af7d55ae387e6cb42cdc8d721a2196 
> 
> 
> Diff: https://reviews.apache.org/r/64898/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 64898: Removed duplicated code that tests for removable tasks.

[James Peach]

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

(Updated Jan. 3, 2018, 12:26 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebased.


Repository: mesos


Description
---

When we get a task update and check whether the latest state
indicated that the task is removable, invoke a local lambda
function so that we don't duplicate the same code for both
the current and latest states.


Diffs (updated)
-

  src/master/master.cpp 03eb178fa1af7d55ae387e6cb42cdc8d721a2196 


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

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


Testing
---

make check


Thanks,

James Peach

Re: Review Request 64868: Added initial doc about CSI support in Mesos.

[Jie Yu]

> On Dec. 29, 2017, 3:03 p.m., James DeFelice wrote:
> > docs/csi.md
> > Lines 298 (patched)
> > 
> >
> > this feels like a separate feature. should it be documented elsewhere?

Removed the protobuf because it's not implemented yet. Just mention it is 
coming soon.


- Jie


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


On Dec. 29, 2017, 4:53 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64868/
> ---
> 
> (Updated Dec. 29, 2017, 4:53 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, Gaston Kleiman, 
> Greg Mann, Joseph Wu, and Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added initial doc about CSI support in Mesos.
> 
> 
> Diffs
> -
> 
>   docs/csi.md PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/64868/diff/2/
> 
> 
> Testing
> ---
> 
> The rendering can be checked here:
> https://github.com/jieyu/mesos/blob/csi_doc/docs/csi.md
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 64868: Added initial doc about CSI support in Mesos.

[Jie Yu]

> On Jan. 2, 2018, 10:27 p.m., Gaston Kleiman wrote:
> > docs/csi.md
> > Lines 781-783 (patched)
> > 
> >
> > We should probalby say what's the difference between removing an SLRP 
> > and marking it as gone. I guess that  a SLRP can't be re-added after having 
> > marked it as gone.

Did mention below.


- Jie


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


On Dec. 29, 2017, 4:53 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64868/
> ---
> 
> (Updated Dec. 29, 2017, 4:53 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, Gaston Kleiman, 
> Greg Mann, Joseph Wu, and Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added initial doc about CSI support in Mesos.
> 
> 
> Diffs
> -
> 
>   docs/csi.md PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/64868/diff/2/
> 
> 
> Testing
> ---
> 
> The rendering can be checked here:
> https://github.com/jieyu/mesos/blob/csi_doc/docs/csi.md
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 64868: Added initial doc about CSI support in Mesos.

[Jie Yu]

> On Jan. 2, 2018, 10:27 p.m., Gaston Kleiman wrote:
> > docs/csi.md
> > Lines 288-291 (patched)
> > 
> >
> > s/framework/scheduler/
> > s/that is available to the frameworks/that is available to them/
> > s/Here are the tips to/A tip to/

I intended to add more tips here. So keep the plural form.


- Jie


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


On Dec. 29, 2017, 4:53 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64868/
> ---
> 
> (Updated Dec. 29, 2017, 4:53 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, Gaston Kleiman, 
> Greg Mann, Joseph Wu, and Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added initial doc about CSI support in Mesos.
> 
> 
> Diffs
> -
> 
>   docs/csi.md PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/64868/diff/2/
> 
> 
> Testing
> ---
> 
> The rendering can be checked here:
> https://github.com/jieyu/mesos/blob/csi_doc/docs/csi.md
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 64868: Added initial doc about CSI support in Mesos.

[Jie Yu]

> On Jan. 2, 2018, 7:53 p.m., Greg Mann wrote:
> > Should we mention somewhere that the `--enable-grpc` configure flag must be 
> > set to enable CSI support in the agent?

Good call. Added.


- Jie


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


On Dec. 29, 2017, 4:53 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64868/
> ---
> 
> (Updated Dec. 29, 2017, 4:53 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, Gaston Kleiman, 
> Greg Mann, Joseph Wu, and Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added initial doc about CSI support in Mesos.
> 
> 
> Diffs
> -
> 
>   docs/csi.md PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/64868/diff/2/
> 
> 
> Testing
> ---
> 
> The rendering can be checked here:
> https://github.com/jieyu/mesos/blob/csi_doc/docs/csi.md
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 64868: Added initial doc about CSI support in Mesos.

[Jie Yu]

> On Jan. 2, 2018, 7:49 p.m., Greg Mann wrote:
> > docs/csi.md
> > Lines 109 (patched)
> > 
> >
> > I'm curious: is this accurate as written, or would "must not be set by 
> > frameworks" be more appropriate?
> > 
> > Similar question regarding the comment for `metadata`.

Yeah, clarified that these two fields must not be set by framweorks.


> On Jan. 2, 2018, 7:49 p.m., Greg Mann wrote:
> > docs/csi.md
> > Lines 192 (patched)
> > 
> >
> > Will we duplicate these offer operation docs in the scheduler API 
> > documentation? To avoid duplication, we could instead add these 
> > instructions to the scheduler API docs, and simply have a link here with a 
> > couple sentences explaining what these operations are used for.

the scheduler API doc is not complete currently. I'll leave this as is for now. 
The scheduler API doc (expecially ACCEPT) needs a major improvement, especially 
around `OPERATIONS`.


> On Jan. 2, 2018, 7:49 p.m., Greg Mann wrote:
> > docs/csi.md
> > Lines 289-290 (patched)
> > 
> >
> > Is it better to just say "by looking at the resources in subsequent 
> > offers"? Are there other sources of information schedulers should use?

They could use operator endpoint (e.g., state).


> On Jan. 2, 2018, 7:49 p.m., Greg Mann wrote:
> > docs/csi.md
> > Lines 291-296 (patched)
> > 
> >
> > Should we also touch on the issue of roles w.r.t. receiving the 
> > converted resource in an offer?
> > 
> > Are volumes created by the new operations similar to persistent volumes 
> > in that they can only be performed on reserved resources?

no, new operation can be used on non-reserved resources.


> On Jan. 2, 2018, 7:49 p.m., Greg Mann wrote:
> > docs/csi.md
> > Lines 298 (patched)
> > 
> >
> > I would recommend simply ommitting this section until we implement it.

OK, i'll mention the limitations and don't mention the protobuf.


> On Jan. 2, 2018, 7:49 p.m., Greg Mann wrote:
> > docs/csi.md
> > Lines 537-539 (patched)
> > 
> >
> > Are these strictly necessary for SLRP support?

Yes. It's unrelated, but these must be specified because the agent code for 
handling the old way has been removed already. add a note.


- Jie


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


On Dec. 29, 2017, 4:53 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64868/
> ---
> 
> (Updated Dec. 29, 2017, 4:53 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, Gaston Kleiman, 
> Greg Mann, Joseph Wu, and Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added initial doc about CSI support in Mesos.
> 
> 
> Diffs
> -
> 
>   docs/csi.md PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/64868/diff/2/
> 
> 
> Testing
> ---
> 
> The rendering can be checked here:
> https://github.com/jieyu/mesos/blob/csi_doc/docs/csi.md
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 64739: Upgraded resources that come from `protobuf::read`.

[Michael Park]

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

(Updated Jan. 2, 2018, 3:39 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Rebased.


Repository: mesos


Description (updated)
---

Upgraded resources that come from `protobuf::read`.


Diffs (updated)
-

  src/resource_provider/storage/provider.cpp 
ee9ac901a6f46b6c6749381d859b3b090c113673 
  src/slave/containerizer/mesos/paths.cpp 
8a188a918873eef468a984b80f5ea7ebaa8fb923 


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

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


Testing
---


Thanks,

Michael Park

Re: Review Request 63976: Modified `downgradeResources` to use protobuf reflection.

[Michael Park]

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

(Updated Jan. 2, 2018, 3:34 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Set required fields in `TaskInfo`.


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


Repository: mesos


Description
---

Previously, our `downgradeResources` function only took a pointer to
`RepeatedPtrField`. This wasn't great since we needed manually
invoke `downgradeResources` on every `Resource`s field of every message.

This patch makes it such that `downgradeResources` can take any
`protobuf::Message` or `protobuf::RepeatedPtrField`.


Diffs (updated)
-

  src/common/resources_utils.hpp 5b74ff2dd3ecb1a0101671d11ea10e29a43524b0 
  src/common/resources_utils.cpp 47ba885517bdfef4764deb0826e13538ce529902 
  src/tests/resources_tests.cpp 64bde85e0e7f0bd395189eb6a8635383095b2f07 


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

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


Testing
---

Added new tests + `make check`


Thanks,

Michael Park

Re: Review Request 63977: Used the protobuf-reflection-based 'downgradeResources' utility.

[Benjamin Mahler]

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


Fix it, then Ship it!





src/slave/slave.cpp
Lines 1294-1296 (original), 1293-1295 (patched)


Stale? References downgrade



src/slave/slave.cpp
Lines 1538-1543 (patched)


What's the consequence of sending a partially downgraded result? Maybe we 
should spell that out as it's not that obvious to me



src/slave/slave.cpp
Line 1678 (original), 1660 (patched)


nice!



src/slave/state.hpp
Lines 96 (patched)


newline between the paragraphs?

```
  // We ignore the `Try` from `downgradeResources` here because
  // for now, we checkpoint the result either way.
  //
  // TODO(mpark): Do something smarter with the result once
  // something like an agent recovery capability is introduced.
```

It's a little non-obvious to me what the implications of this are.. maybe 
document it? Does it mean that we checkpoint only partially downgraded and 
that's fine so long as we don't downgrade beyond the 1.x version that 
introduced refinement..?



src/tests/slave_recovery_tests.cpp
Lines 152-155 (patched)


Do you want a TODO to use a reflection based upgrade here?


- Benjamin Mahler


On Nov. 21, 2017, 6:07 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63977/
> ---
> 
> (Updated Nov. 21, 2017, 6:07 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-8221
> https://issues.apache.org/jira/browse/MESOS-8221
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp cf8a22b636210e9f8d326592d8b64eab2c97caa4 
>   src/slave/slave.cpp 00935616c2328e2ad9fc170199298a3630a701ab 
>   src/slave/state.hpp aaf8e5c2895e1cd88172c5bf73f028b629dc12c7 
>   src/tests/slave_recovery_tests.cpp bf2c5fcabdd4c16bdf5de1b641060e38783b8ee8 
> 
> 
> Diff: https://reviews.apache.org/r/63977/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Review Request 64899: Removed `nodist_liburi_volume_profile_SOURCES`.

[Chun-Hung Hsiao]

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

Review request for mesos, Jie Yu and James Peach.


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


Repository: mesos


Description
---

Adding generated protobuf headers into `nodist_*_SOURCES` for modules
doesn't enfource build dependencies in the generated makefile so the
lines are removed.

NOTE: Currently `liburi_volume_profile.la` cannot be built standalone.
The issue is tracked in MESOS-8370.


Diffs
-

  src/Makefile.am b83cddaaf874b669c0b8f9df8805fc88e8c3e90d 


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


Testing
---

make distcheck


Thanks,

Chun-Hung Hsiao

Re: Review Request 64898: Removed duplicated code that tests for removable tasks.

[Gaston Kleiman]

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



I think that you need to rebase this RR.

- Gaston Kleiman


On Jan. 2, 2018, 1:43 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64898/
> ---
> 
> (Updated Jan. 2, 2018, 1:43 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When we get a task update and check whether the latest state
> indicated that the task is removable, invoke a local lambda
> function so that we don't duplicate the same code for both
> the current and latest states.
> 
> 
> Diffs
> -
> 
>   docs/configuration/master.md 3ff97ea5dbae12e17cb137d1367b98130dce9596 
>   src/master/flags.hpp dabb414560f506787b6e821a27af623c8da44b11 
>   src/master/flags.cpp b49cb63bfebb286ae4f5e665652dc5f8f2964ceb 
>   src/master/master.cpp 03eb178fa1af7d55ae387e6cb42cdc8d721a2196 
>   src/tests/master_tests.cpp 5546fd937d078c1f757964d5163449ffd993388e 
> 
> 
> Diff: https://reviews.apache.org/r/64898/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 64739: Upgraded resources that come from `protobuf::read`.

[Benjamin Mahler]

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


Ship it!




Is there a ticket for being more robust in the face of inability to recover a 
downgraded agent? Can you file one if not?
Also, do we want to target a ticket for 2.1 that stops downgrading? I assume 
the strategy to end the 1.x deprecation cycle will be that 2.1+ only supports 
being upgraded from 2.0.x.


src/resource_provider/storage/provider.cpp
Line 663 (original), 663-664 (patched)


Do you want a TODO here to use a reflection based approach?



src/resource_provider/storage/provider.cpp
Lines 854-862 (patched)


Ditto here



src/slave/containerizer/mesos/paths.cpp
Lines 288-289 (patched)


Ditto here



src/slave/containerizer/mesos/paths.cpp
Lines 341-350 (patched)


Ditto here.


- Benjamin Mahler


On Dec. 20, 2017, 12:51 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64739/
> ---
> 
> (Updated Dec. 20, 2017, 12:51 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> 79d7f602bbc57bce308157dfbb6c2afaef806f23 
>   src/slave/containerizer/mesos/paths.cpp 
> 8a188a918873eef468a984b80f5ea7ebaa8fb923 
> 
> 
> Diff: https://reviews.apache.org/r/64739/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 61172: Added mesos.http and mesos.exceptions for CLI.

[Eric Chung]

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




src/python/lib/mesos/http.py
Lines 149 (patched)


actually this has to be None if we want to allow defaults and overrides. a 
None passed in indicates that we're not overriding -- if the default value is 
boolean, there is no way to tell whether this was passed in by the user or 
whether it was just the default value.


- Eric Chung


On Jan. 1, 2018, 1:52 a.m., Eric Chung wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61172/
> ---
> 
> (Updated Jan. 1, 2018, 1:52 a.m.)
> 
> 
> Review request for mesos, Armand Grillet, Jason Lai, and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Part of MESOS-7310, this patch adds the mesos.http and mesos.exceptions
> modules, which provides a Resource class and its descendants for
> abstracting away common operations over http connectioins with JSON
> serialization.
> 
> updated patch according to klueska's recommendations
> 
> 
> addressed comments
> 
> 
> Diffs
> -
> 
>   src/python/cli_new/bootstrap c84e549e59bc7a07b5b70c24c63bc0c16881e442 
>   src/python/lib/mesos/exceptions.py PRE-CREATION 
>   src/python/lib/mesos/http.py PRE-CREATION 
>   src/python/lib/requirements.in e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 
>   src/python/lib/tests/conftest.py PRE-CREATION 
>   src/python/lib/tests/test_exceptions.py PRE-CREATION 
>   src/python/lib/tests/test_http.py PRE-CREATION 
>   src/python/lib/tox.ini 8ad030d2dbf1fb7a04f1eaadb587462b4ef3f054 
>   support/pylint.config f74f553e238553bd6a6c06f4dd888cc5954a33eb 
> 
> 
> Diff: https://reviews.apache.org/r/61172/diff/8/
> 
> 
> Testing
> ---
> 
> install tox
> cd src/python/lib
> tox
> 
> 
> Thanks,
> 
> Eric Chung
> 
>

Re: Review Request 64868: Added initial doc about CSI support in Mesos.

[Gaston Kleiman]

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




docs/csi.md
Lines 21 (patched)


Another nit: s/third party/third-party/



docs/csi.md
Lines 48 (patched)


s/from the that/from that/



docs/csi.md
Lines 288-291 (patched)


s/framework/scheduler/
s/that is available to the frameworks/that is available to them/
s/Here are the tips to/A tip to/



docs/csi.md
Lines 293 (patched)


Nit: s/  Reservation/ Reservation/



docs/csi.md
Lines 298 (patched)


+1



docs/csi.md
Lines 304 (patched)


s/CSI plugin/a CSI plugin/



docs/csi.md
Lines 529 (patched)


s/SLRP/SLRPs/?



docs/csi.md
Lines 689 (patched)


s/to a directory/in a directory/



docs/csi.md
Lines 690 (patched)


s/set agent flag/set the agent flag/



docs/csi.md
Lines 698 (patched)


s/agent flag/the agent flag/



docs/csi.md
Lines 781-783 (patched)


We should probalby say what's the difference between removing an SLRP and 
marking it as gone. I guess that  a SLRP can't be re-added after having marked 
it as gone.


- Gaston Kleiman


On Dec. 28, 2017, 8:53 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64868/
> ---
> 
> (Updated Dec. 28, 2017, 8:53 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, Gaston Kleiman, 
> Greg Mann, Joseph Wu, and Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added initial doc about CSI support in Mesos.
> 
> 
> Diffs
> -
> 
>   docs/csi.md PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/64868/diff/2/
> 
> 
> Testing
> ---
> 
> The rendering can be checked here:
> https://github.com/jieyu/mesos/blob/csi_doc/docs/csi.md
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 64895: Renamed "normalize" to "upgrade" in resources adjustment operations.

[Benjamin Mahler]

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


Ship it!




Ship It!

- Benjamin Mahler


On Jan. 2, 2018, 8:41 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64895/
> ---
> 
> (Updated Jan. 2, 2018, 8:41 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We have introduced the notion of "downgrading" resources in order to
> support downgrades of various components. The opposite operation used
> to be called "normalize", but it seems "upgrade" would be simpler.
> 
> 
> Diffs
> -
> 
>   src/common/resources_utils.hpp 5b74ff2dd3ecb1a0101671d11ea10e29a43524b0 
>   src/common/resources_utils.cpp 47ba885517bdfef4764deb0826e13538ce529902 
>   src/master/http.cpp d7276e45e11f680e909027566b3cf29af10882c9 
>   src/master/master.cpp 03eb178fa1af7d55ae387e6cb42cdc8d721a2196 
> 
> 
> Diff: https://reviews.apache.org/r/64895/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 64694: Cleaned up `ResourceProviderManagerHttpApiTest.ConvertResources`.

[Greg Mann]

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


Ship it!




Ship It!

- Greg Mann


On Dec. 19, 2017, 12:04 a.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64694/
> ---
> 
> (Updated Dec. 19, 2017, 12:04 a.m.)
> 
> 
> Review request for mesos, Greg Mann, Jie Yu, and Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch makes the test follow the pattern used by the Default
> Executor tests.
> 
> 
> Diffs
> -
> 
>   src/tests/resource_provider_manager_tests.cpp 
> 4e86016a450c30fea90e9c6e5c1f2f1aab5f42c0 
> 
> 
> Diff: https://reviews.apache.org/r/64694/diff/1/
> 
> 
> Testing
> ---
> 
> The test passed 500 times on GNU/Linux.
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Re: Review Request 64694: Cleaned up `ResourceProviderManagerHttpApiTest.ConvertResources`.

[Greg Mann]

> On Jan. 2, 2018, 8:56 p.m., Greg Mann wrote:
> > src/tests/resource_provider_manager_tests.cpp
> > Line 1032 (original), 1017 (patched)
> > 
> >
> > It does work to declare this as a const ref, but a bit weird. Let's 
> > just make it
> > `const v1::Offer offer1`
> > 
> > and `offer2` below as well.
> 
> Gaston Kleiman wrote:
> This is a very common pattern in `default_executor_tests.cpp`, should we 
> change it there as well?

Ah good point, I suppose we just do it to avoid an extra copy. I'll just drop 
it.


- Greg


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


On Dec. 19, 2017, 12:04 a.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64694/
> ---
> 
> (Updated Dec. 19, 2017, 12:04 a.m.)
> 
> 
> Review request for mesos, Greg Mann, Jie Yu, and Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch makes the test follow the pattern used by the Default
> Executor tests.
> 
> 
> Diffs
> -
> 
>   src/tests/resource_provider_manager_tests.cpp 
> 4e86016a450c30fea90e9c6e5c1f2f1aab5f42c0 
> 
> 
> Diff: https://reviews.apache.org/r/64694/diff/1/
> 
> 
> Testing
> ---
> 
> The test passed 500 times on GNU/Linux.
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Review Request 64898: Removed duplicated code that tests for removable tasks.

[James Peach]

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

Review request for mesos and Jiang Yan Xu.


Repository: mesos


Description
---

When we get a task update and check whether the latest state
indicated that the task is removable, invoke a local lambda
function so that we don't duplicate the same code for both
the current and latest states.


Diffs
-

  docs/configuration/master.md 3ff97ea5dbae12e17cb137d1367b98130dce9596 
  src/master/flags.hpp dabb414560f506787b6e821a27af623c8da44b11 
  src/master/flags.cpp b49cb63bfebb286ae4f5e665652dc5f8f2964ceb 
  src/master/master.cpp 03eb178fa1af7d55ae387e6cb42cdc8d721a2196 
  src/tests/master_tests.cpp 5546fd937d078c1f757964d5163449ffd993388e 


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


Testing
---

make check


Thanks,

James Peach

Re: Review Request 64694: Cleaned up `ResourceProviderManagerHttpApiTest.ConvertResources`.

[Gaston Kleiman]

> On Jan. 2, 2018, 12:56 p.m., Greg Mann wrote:
> > src/tests/resource_provider_manager_tests.cpp
> > Line 1032 (original), 1017 (patched)
> > 
> >
> > It does work to declare this as a const ref, but a bit weird. Let's 
> > just make it
> > `const v1::Offer offer1`
> > 
> > and `offer2` below as well.

This is a very common pattern in `default_executor_tests.cpp`, should we change 
it there as well?


- Gaston


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


On Dec. 18, 2017, 4:04 p.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64694/
> ---
> 
> (Updated Dec. 18, 2017, 4:04 p.m.)
> 
> 
> Review request for mesos, Greg Mann, Jie Yu, and Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch makes the test follow the pattern used by the Default
> Executor tests.
> 
> 
> Diffs
> -
> 
>   src/tests/resource_provider_manager_tests.cpp 
> 4e86016a450c30fea90e9c6e5c1f2f1aab5f42c0 
> 
> 
> Diff: https://reviews.apache.org/r/64694/diff/1/
> 
> 
> Testing
> ---
> 
> The test passed 500 times on GNU/Linux.
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Re: Review Request 64897: Fixed CI build failure for `volume_profile.pb.h`.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Jan. 2, 2018, 9:15 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64897/
> ---
> 
> (Updated Jan. 2, 2018, 9:15 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Vinod Kone.
> 
> 
> Bugs: MESOS-8369
> https://issues.apache.org/jira/browse/MESOS-8369
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed CI build failure for `volume_profile.pb.h`.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am b83cddaaf874b669c0b8f9df8805fc88e8c3e90d 
> 
> 
> Diff: https://reviews.apache.org/r/64897/diff/1/
> 
> 
> Testing
> ---
> 
> make distcheck
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Review Request 64897: Fixed CI build failure for `volume_profile.pb.h`.

[Chun-Hung Hsiao]

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

Review request for mesos, Jie Yu and Vinod Kone.


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


Repository: mesos


Description
---

Fixed CI build failure for `volume_profile.pb.h`.


Diffs
-

  src/Makefile.am b83cddaaf874b669c0b8f9df8805fc88e8c3e90d 


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


Testing
---

make distcheck


Thanks,

Chun-Hung Hsiao

Re: Review Request 64739: Upgraded resources that come from `protobuf::read`.

[Michael Park]

> On Dec. 21, 2017, 6:55 p.m., Benjamin Mahler wrote:
> > Hm.. I was a little puzzled here, why do some use convert with POST and 
> > some use the reflection based upgrade? Shouldn't we just use the reflection 
> > based upgrade everywhere?

Sorry for the confusion. The `upgradeResources` used here is the one pulled out 
of
`validateAndUpgradeResources`, and is not "reflection based upgrade". I called
it `upgradeResources` for now in the hopes that when we do introduce 
reflection-based
`upgradeResources`, the callsite wouldn't need to change. Although looking at 
it now
we'll probably be invoking the reflection-based upgrade on all of 
`resourceProviderState`
i.e., `upgradeResources(resourceProviderState.get());`


- Michael


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


On Dec. 19, 2017, 4:51 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64739/
> ---
> 
> (Updated Dec. 19, 2017, 4:51 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> 79d7f602bbc57bce308157dfbb6c2afaef806f23 
>   src/slave/containerizer/mesos/paths.cpp 
> 8a188a918873eef468a984b80f5ea7ebaa8fb923 
> 
> 
> Diff: https://reviews.apache.org/r/64739/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 64694: Cleaned up `ResourceProviderManagerHttpApiTest.ConvertResources`.

[Greg Mann]

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




src/tests/resource_provider_manager_tests.cpp
Line 1032 (original), 1017 (patched)


It does work to declare this as a const ref, but a bit weird. Let's just 
make it
`const v1::Offer offer1`

and `offer2` below as well.


- Greg Mann


On Dec. 19, 2017, 12:04 a.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64694/
> ---
> 
> (Updated Dec. 19, 2017, 12:04 a.m.)
> 
> 
> Review request for mesos, Greg Mann, Jie Yu, and Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch makes the test follow the pattern used by the Default
> Executor tests.
> 
> 
> Diffs
> -
> 
>   src/tests/resource_provider_manager_tests.cpp 
> 4e86016a450c30fea90e9c6e5c1f2f1aab5f42c0 
> 
> 
> Diff: https://reviews.apache.org/r/64694/diff/1/
> 
> 
> Testing
> ---
> 
> The test passed 500 times on GNU/Linux.
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Re: Review Request 63976: Modified `downgradeResources` to use protobuf reflection.

[Michael Park]

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

(Updated Jan. 2, 2018, 12:55 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Addressed BenM's comments.


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


Repository: mesos


Description
---

Previously, our `downgradeResources` function only took a pointer to
`RepeatedPtrField`. This wasn't great since we needed manually
invoke `downgradeResources` on every `Resource`s field of every message.

This patch makes it such that `downgradeResources` can take any
`protobuf::Message` or `protobuf::RepeatedPtrField`.


Diffs (updated)
-

  src/common/resources_utils.hpp 5b74ff2dd3ecb1a0101671d11ea10e29a43524b0 
  src/common/resources_utils.cpp 47ba885517bdfef4764deb0826e13538ce529902 
  src/tests/resources_tests.cpp 64bde85e0e7f0bd395189eb6a8635383095b2f07 


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

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


Testing
---

Added new tests + `make check`


Thanks,

Michael Park

Re: Review Request 64590: Stopped logging optional fields unconditionally in agent handler.

[Greg Mann]

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




src/slave/slave.cpp
Lines 7294 (patched)


Since this field name has been changed, we should update this log. But we 
still want to be sure it's clear that it's the operation's UUID we're logging, 
not the status's.

Maybe s/operation_uuid/operation uuid/ ?


- Greg Mann


On Dec. 28, 2017, 8:21 p.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64590/
> ---
> 
> (Updated Dec. 28, 2017, 8:21 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Greg Mann, and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `operation_id` and `framework_id` fields are optional, so they
> should only be logged if set.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp e0806ab15be3ea790681dc97b20791c8a20512e3 
> 
> 
> Diff: https://reviews.apache.org/r/64590/diff/5/
> 
> 
> Testing
> ---
> 
> `make check` on GNU/Linux
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Review Request 64895: Renamed "normalize" to "upgrade" in resources adjustment operations.

[Michael Park]

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

Review request for mesos and Benjamin Mahler.


Repository: mesos


Description
---

We have introduced the notion of "downgrading" resources in order to
support downgrades of various components. The opposite operation used
to be called "normalize", but it seems "upgrade" would be simpler.


Diffs
-

  src/common/resources_utils.hpp 5b74ff2dd3ecb1a0101671d11ea10e29a43524b0 
  src/common/resources_utils.cpp 47ba885517bdfef4764deb0826e13538ce529902 
  src/master/http.cpp d7276e45e11f680e909027566b3cf29af10882c9 
  src/master/master.cpp 03eb178fa1af7d55ae387e6cb42cdc8d721a2196 


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


Testing
---


Thanks,

Michael Park

Re: Review Request 63976: Modified `downgradeResources` to use protobuf reflection.

[Michael Park]

> On Dec. 20, 2017, 6:24 p.m., Benjamin Mahler wrote:
> > src/common/resources_utils.hpp
> > Line 175 (original), 175-176 (patched)
> > 
> >
> > s/does/do/?
> > 
> > Maybe remove the first sentence and just directly say "all-or-nothing"? 
> > Atomicity seems a little obscure here to me

Removed the first sentence.


> On Dec. 20, 2017, 6:24 p.m., Benjamin Mahler wrote:
> > src/common/resources_utils.hpp
> > Lines 181-182 (patched)
> > 
> >
> > Does it keep going and finish the rest in the error case? Or does it 
> > stop converting and return the error? From reading the above comments, it 
> > sounds like it wouldn't break early.

The current behavior is that it stops converting and returns the error. I'll 
update the comment accordingly.


> On Dec. 20, 2017, 6:24 p.m., Benjamin Mahler wrote:
> > src/common/resources_utils.hpp
> > Lines 184-185 (patched)
> > 
> >
> > Are you saying that once a component has refined reservations, it 
> > cannot be downgraded back to.. the version before we had reservation 
> > refinement? As written, it seems to be speaking too generally about 
> > downgrades: whether 1.9 could be downgraded to 1.8 is a separate question?

Yeah. Good point. Updated to include "... to a version that does not support 
reservation refinement."


> On Dec. 20, 2017, 6:24 p.m., Benjamin Mahler wrote:
> > src/common/resources_utils.cpp
> > Lines 779-781 (patched)
> > 
> >
> > Hm.. is returning bool here an optimization? I would imagine the 
> > resultant map could (or I guess already does) contain the top level 
> > descriptor?
> > 
> > Then, downgradeResourcesImpl would first check the passed in message 
> > descriptor and bail if not contained?
> > 
> > ```
> > static Try downgradeResourcesImpl(
> > Message* message,
> > const hashmap& resourcesContainment)
> > {
> >   CHECK_NOTNULL(message);
> > 
> >   const Descriptor* descriptor = message->GetDescriptor();
> >   
> >   if (!resourcesContainment.at(descriptor)) {
> > return; // Done.
> >   }
> >   
> >   if (descriptor == mesos::Resource::descriptor()) {
> > return downgradeResources(static_cast(message));
> >   }
> >   
> >   // When recursing into fields, I guess we don't bother checking
> >   // containement before recursing since the recursive call will check?
> > ```
> > 
> > Then the top level function gets a bit simpler?
> > 
> > ```
> > Try downgradeResources(Message* message)
> > {
> >   const Descriptor* descriptor = message->GetDescriptor();
> > 
> >   hashmap resourcesContainment;
> >   internal::precomputeResourcesContainment(descriptor, 
> > );
> >   
> >   return internal::downgradeResourcesImpl(message, 
> > resourcesContainment);
> > ```
> > 
> > Just curious to get your thoughts on the two approaches.

I've removed the `bool` return since it was kind of a silly, and was not even 
an optimization.

However, I believe moving the containment check inside `downgradeResourcesImpl` 
would be
a regression in performance. Consider a protobuf message:
```
message A {
  repeated B messages;
  optional Resource resource;
}
```
and suppose `B` is not `Resource` nor does it have any `Resource`s within.

Since `A` has `resource`, we'll pass the `resourcesContainment.at(descriptor)` 
check.

Ideally we would skip `messages` entirely, since from the schema of `B` we know 
that
there aren't any `Resource`s in there. If we were to wait until the recursive 
call,
we would fully execute this loop going over each `B` in `messages`:
```
  const int size = reflection->FieldSize(*message, field);

  for (int j = 0; j < size; ++j) {
Try result = downgradeResourcesImpl(
reflection->MutableRepeatedMessage(message, field, j),
resourcesContainment);

if (result.isError()) {
  return result;
}
  }
```

I think the issue is mainly in the way in which we need to treat repeated 
fields.
If we were able to get access to a repeated field of `Message`s and pass it to
the recursive call, we could have the check at the top of 
`downgradeResourcesImpl`.
But as far as I know, the only way to get access to `Message`s of a repeated 
field
is via `MutableRepeatedMessage`.


- Michael


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


On Nov. 20, 2017, 10:07 p.m., Michael Park wrote:
> 
> 

Re: Review Request 64868: Added initial doc about CSI support in Mesos.

[Greg Mann]

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



Should we mention somewhere that the `--enable-grpc` configure flag must be set 
to enable CSI support in the agent?

- Greg Mann


On Dec. 29, 2017, 4:53 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64868/
> ---
> 
> (Updated Dec. 29, 2017, 4:53 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, Gaston Kleiman, 
> Greg Mann, Joseph Wu, and Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added initial doc about CSI support in Mesos.
> 
> 
> Diffs
> -
> 
>   docs/csi.md PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/64868/diff/2/
> 
> 
> Testing
> ---
> 
> The rendering can be checked here:
> https://github.com/jieyu/mesos/blob/csi_doc/docs/csi.md
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 64868: Added initial doc about CSI support in Mesos.

[Greg Mann]

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




docs/csi.md
Lines 21 (patched)


Nit: s/well defined/well-defined/



docs/csi.md
Lines 25 (patched)


Is "currently" appropriate here? Do we expect this to change? IIUC, this 
feature is distinct from CSI support and still remains in the codebase?



docs/csi.md
Lines 28 (patched)


s/reservation, fair/reservation, and fair/



docs/csi.md
Lines 30 (patched)


Nit: s/Volume Driver/volume driver/



docs/csi.md
Lines 39 (patched)


s/from/from the/



docs/csi.md
Lines 41 (patched)


Suggestion:

s/to have storage vendors write/to allow storage vendors to write/



docs/csi.md
Lines 45 (patched)


s/big/larger/

I would also recommend: s/very//



docs/csi.md
Lines 46 (patched)


Suggestion: s/the users/users/



docs/csi.md
Lines 101 (patched)


Nit: s/plugin specific/plugin-specific/



docs/csi.md
Lines 109 (patched)


I'm curious: is this accurate as written, or would "must not be set by 
frameworks" be more appropriate?

Similar question regarding the comment for `metadata`.



docs/csi.md
Lines 130 (patched)


Suggestion:

s/if the `RAW` disk resource is backed by a CSI Volume or not/whether or 
not the `RAW` disk resource is backed by a CSI Volume/



docs/csi.md
Lines 146 (patched)


s/using/using the/



docs/csi.md
Lines 163 (patched)


For consistency with the scheduler API docs, let's do s/framework 
API/scheduler API/

here and below.



docs/csi.md
Lines 192 (patched)


Will we duplicate these offer operation docs in the scheduler API 
documentation? To avoid duplication, we could instead add these instructions to 
the scheduler API docs, and simply have a link here with a couple sentences 
explaining what these operations are used for.



docs/csi.md
Lines 285 (patched)


either s/disks/disk/ or s/disks/disk's/



docs/csi.md
Lines 289-290 (patched)


Is it better to just say "by looking at the resources in subsequent 
offers"? Are there other sources of information schedulers should use?



docs/csi.md
Lines 291-296 (patched)


Should we also touch on the issue of roles w.r.t. receiving the converted 
resource in an offer?

Are volumes created by the new operations similar to persistent volumes in 
that they can only be performed on reserved resources?



docs/csi.md
Lines 298 (patched)


I would recommend simply ommitting this section until we implement it.



docs/csi.md
Lines 328 (patched)


Nit: s/vendor specific/vendor-specific/

Here and in the comments below.



docs/csi.md
Lines 330 (patched)


Nit: s/low level/low-level/

Here and just below the proto snippet.



docs/csi.md
Lines 360 (patched)


s/system specific/system-specific/



docs/csi.md
Lines 386 (patched)


s/will/will be/



docs/csi.md
Lines 387 (patched)


s/call/call the/



docs/csi.md
Lines 408 (patched)


s/under a/under/



docs/csi.md
Lines 408-409 (patched)


Suggestion:

s/a free-form string-string mapping/arbitrary key-value pairs/



docs/csi.md
Lines 432 (patched)


s/--uri/uri/

Here and below.



docs/csi.md
Lines 433-435 (patched)


Suggestion:

"If the poll interval has elapsed since the last fetch, then the URI is 
re-fetched; otherwise, a cached `ProfileInfo` is returned. If not specified, 
the URI is only fetched once."

Re: Review Request 64879: Updated fetcher cache tests for the default executor.

[Gaston Kleiman]

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




src/tests/fetcher_cache_tests.cpp
Lines 391-393 (original), 393-399 (patched)


Can't we just do `return os::access(path, X_OK);` here?



src/tests/fetcher_cache_tests.cpp
Line 487 (original), 504 (patched)


I think the variable should be named `task_`:

> [...]
> prepend constructor and function arguments with a leading underscore to 
avoid ambiguity and / or shadowing
> [...]
> Prefer trailing underscores for use as member fields (but not required). 
Some trailing underscores are used to distinguish between similar variables in 
the same scope (think prime symbols), but this should be avoided as much as 
possible, including removing existing instances in the code base.



src/tests/fetcher_cache_tests.cpp
Line 503 (original), 523 (patched)


you're overwriting `_task.runDirectory` here, is that intentional?


- Gaston Kleiman


On Dec. 30, 2017, 12:06 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64879/
> ---
> 
> (Updated Dec. 30, 2017, 12:06 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, and Jie Yu.
> 
> 
> Bugs: MESOS-8366
> https://issues.apache.org/jira/browse/MESOS-8366
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated the task launch helper to obtain the ContainerID of the
> launched task, which can be used to correctly construct the sandbox
> path for both the command and default executors.
> 
> Updated the test expectations in cases where the default executor
> container will invoke the fetcher more times than the test is
> expecting.
> 
> Updated various test helper functions to propagate more detailed
> error results that make test failure easier to debug.
> 
> 
> Diffs
> -
> 
>   src/tests/fetcher_cache_tests.cpp 7db7b7dcef27b1686ccae5a7408ff2811a3b9255 
> 
> 
> Diff: https://reviews.apache.org/r/64879/diff/1/
> 
> 
> Testing
> ---
> 
> make check (Fedora 27)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 64889: Fixed handling of checkpointed resources for RP-capable agents.

[Benjamin Bannier]

> On Jan. 2, 2018, 4:55 p.m., Benno Evers wrote:
> > src/master/master.cpp
> > Line 6597 (original)
> > 
> >
> > Should we issue a warning if an agent that is not resource 
> > provider-capable tries to reregister with different checkpointed resources 
> > than it had before?

In this case the master would resend all checkpointed resources to the agent 
again anyway, so I am unsure this would be very useful. It is also possible 
that the agent failed to checkpoint some resources which made it go out of sync 
with the master; currently in such scenarios the agent will fail over and rely 
on the master to send it the most current checkpointed resources state, so the 
situation you describe would not be unexpected.


> On Jan. 2, 2018, 4:55 p.m., Benno Evers wrote:
> > src/tests/slave_recovery_tests.cpp
> > Lines 4899 (patched)
> > 
> >
> > I think this might break when we add new capabilities in the future, 
> > and the master mistakenly thinks it is communicating with some legacy agent.
> > 
> > Maybe we should just call 
> > `mesos::internal::slave::AGENT_CAPABILITIES()` and then add the 
> > `RESOURCE_PROVIDER` capability to the result afterwards until it becomes 
> > enabled by default?

I posted https://reviews.apache.org/r/64891/ to make a more general cleanup, 
and adjusted the added code here to follow the same pattern.


- Benjamin


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


On Jan. 2, 2018, 6:27 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64889/
> ---
> 
> (Updated Jan. 2, 2018, 6:27 p.m.)
> 
> 
> Review request for mesos, Benno Evers, Jie Yu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-8350
> https://issues.apache.org/jira/browse/MESOS-8350
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The master will not resend checkpointed resources when a resource
> provider-capable agent reregisters. Instead the checkpointed resources
> sent as part of the agent reregistration should be evaluated by the
> master and be used to update its state.
> 
> This patch fixes the handling of checkpointed resources sent as part
> of the agent reregistration so that the resources are used to update
> the master state.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 8fe9420dbe03ea2cefc6a40b0f64284aa9fe7915 
>   src/master/master.cpp 04378a8d931ebe5f2667399ec9ce4225fa8c7c82 
>   src/tests/slave_recovery_tests.cpp bf2c5fcabdd4c16bdf5de1b641060e38783b8ee8 
> 
> 
> Diff: https://reviews.apache.org/r/64889/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 64888: Added check for handling of checkpointed resources in reregistration.

[Benjamin Bannier]

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

(Updated Jan. 2, 2018, 6:27 p.m.)


Review request for mesos, Benno Evers, Jie Yu, and Jan Schlicht.


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


Repository: mesos


Description
---

This patch adds a check that confirms that the master resends
checkpointed resources to the agent on reregistration.


Diffs (updated)
-

  src/tests/slave_recovery_tests.cpp bf2c5fcabdd4c16bdf5de1b641060e38783b8ee8 


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

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


Testing
---

`make check`


Thanks,

Benjamin Bannier

Re: Review Request 64889: Fixed handling of checkpointed resources for RP-capable agents.

[Benjamin Bannier]

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

(Updated Jan. 2, 2018, 6:27 p.m.)


Review request for mesos, Benno Evers, Jie Yu, and Jan Schlicht.


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


Repository: mesos


Description
---

The master will not resend checkpointed resources when a resource
provider-capable agent reregisters. Instead the checkpointed resources
sent as part of the agent reregistration should be evaluated by the
master and be used to update its state.

This patch fixes the handling of checkpointed resources sent as part
of the agent reregistration so that the resources are used to update
the master state.


Diffs (updated)
-

  src/master/master.hpp 8fe9420dbe03ea2cefc6a40b0f64284aa9fe7915 
  src/master/master.cpp 04378a8d931ebe5f2667399ec9ce4225fa8c7c82 
  src/tests/slave_recovery_tests.cpp bf2c5fcabdd4c16bdf5de1b641060e38783b8ee8 


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

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


Testing
---

`make check`


Thanks,

Benjamin Bannier

Review Request 64891: Future-proofed use of agent capabilities in tests.

[Benjamin Bannier]

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

Review request for mesos, Benno Evers and Jie Yu.


Repository: mesos


Description
---

Even though currently the resource provider capability is the only
capability which can be toggled by users, when examining agent flags
we expect either no capabilities at all or a full set of capabilities
(including both toggleable and required capabilities).

This patch cleans up test code agent capabilities by delegating the
bulk of the work to a helper function providing a set of
default-enabled capabilities and only adds a single capability to that
set. This not only makes it clearer which exact capability a test
cares about, but also future-proofs the code for the case where we
extend the set of required capabilities in the future.


Diffs
-

  src/tests/agent_resource_provider_config_api_tests.cpp 
f2486cbf5cf9599ed3bd1caa3b1f660ddcb9d15f 
  src/tests/api_tests.cpp 9246f4222548f8d19a8f4f89fb3c22ecae5b58b0 
  src/tests/persistent_volume_tests.cpp 
372037f5121dfcf80ec02affdb6e9eee4029dedf 
  src/tests/reservation_tests.cpp 938306ed882f5bc94cd373653af30d09dd4b42d9 
  src/tests/resource_provider_manager_tests.cpp 
096747ee0bfed953f6f72687b5f8faffaacc7f6a 
  src/tests/slave_tests.cpp fb49077d7cb81db450d9fa24f75dbd9c79ef645c 
  src/tests/storage_local_resource_provider_tests.cpp 
22b81a5c2fcca1e97a6e7d4bde790fc21022d362 


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


Testing
---

`make check`


Thanks,

Benjamin Bannier

Re: Review Request 64885: Made sure continuations using actor state are executed in actor context.

[Zhitao Li]

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


Ship it!




Ship It!


src/slave/containerizer/mesos/provisioner/provisioner.cpp
Line 568 (original), 568 (patched)


Thanks for pointing it out now. I like your rule of avoiding default 
captures in continuation since it makes such issues more readable.

I can submit a future patch to capture explicitly if you don't do it this 
patch.

Thanks for fixing this!


- Zhitao Li


On Jan. 2, 2018, 10:38 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64885/
> ---
> 
> (Updated Jan. 2, 2018, 10:38 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Zhitao Li.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In `e273efe6976434858edb85bbcf367a02e963a467` we introduced layer
> checkpointing to the provisioner, but did not make sure that
> continutations making use of internal state of the provisioner actor
> installed on futures where always executed in the actor's context.
> This was problematic as continuations could be executed while actor
> state was changing (data races), or after the the actor had terminated
> (use after free).
> 
> In this patch we instead defer execution of continutions to the actor
> owning the data.
> 
> This issue was identified with the clang-tidy `mesos-this-capture`
> check.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 
> 61e771872ba2c3f5ecabbe085db102433c7bac3f 
> 
> 
> Diff: https://reviews.apache.org/r/64885/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 64885: Made sure continuations using actor state are executed in actor context.

[Zhitao Li]

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


Ship it!




Ship It!

- Zhitao Li


On Jan. 2, 2018, 10:38 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64885/
> ---
> 
> (Updated Jan. 2, 2018, 10:38 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Zhitao Li.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In `e273efe6976434858edb85bbcf367a02e963a467` we introduced layer
> checkpointing to the provisioner, but did not make sure that
> continutations making use of internal state of the provisioner actor
> installed on futures where always executed in the actor's context.
> This was problematic as continuations could be executed while actor
> state was changing (data races), or after the the actor had terminated
> (use after free).
> 
> In this patch we instead defer execution of continutions to the actor
> owning the data.
> 
> This issue was identified with the clang-tidy `mesos-this-capture`
> check.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 
> 61e771872ba2c3f5ecabbe085db102433c7bac3f 
> 
> 
> Diff: https://reviews.apache.org/r/64885/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 64885: Made sure continuations using actor state are executed in actor context.

[Benjamin Bannier]

> On Jan. 2, 2018, 5:12 p.m., Zhitao Li wrote:
> > src/slave/containerizer/mesos/provisioner/provisioner.cpp
> > Line 568 (original), 568 (patched)
> > 
> >
> > I don't think the callback here uses any internal state of calling 
> > actor, so I don't see it really necessary that continuation *must* happen 
> > in `self()`.
> > 
> > Do we have a rule to require even continuation without side effect to 
> > calling actor be defered?

This callback make use of the member variable `rootfs` which will be accessed 
via a `this` pointer; `containerId`, `imageInfo`, and `rootfs` are all captured 
by value.

My _personal_ rule of thumb is to not use default capture, but instead list 
every captured variable explicitly in the capture list. I also like to prefix 
uses of member variables with `this` in the lambda body. I feel that makes it 
easier to spot such issues even without static analysis.


- Benjamin


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


On Jan. 2, 2018, 11:38 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64885/
> ---
> 
> (Updated Jan. 2, 2018, 11:38 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Zhitao Li.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In `e273efe6976434858edb85bbcf367a02e963a467` we introduced layer
> checkpointing to the provisioner, but did not make sure that
> continutations making use of internal state of the provisioner actor
> installed on futures where always executed in the actor's context.
> This was problematic as continuations could be executed while actor
> state was changing (data races), or after the the actor had terminated
> (use after free).
> 
> In this patch we instead defer execution of continutions to the actor
> owning the data.
> 
> This issue was identified with the clang-tidy `mesos-this-capture`
> check.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 
> 61e771872ba2c3f5ecabbe085db102433c7bac3f 
> 
> 
> Diff: https://reviews.apache.org/r/64885/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 64885: Made sure continuations using actor state are executed in actor context.

[Zhitao Li]

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




src/slave/containerizer/mesos/provisioner/provisioner.cpp
Line 568 (original), 568 (patched)


I don't think the callback here uses any internal state of calling actor, 
so I don't see it really necessary that continuation *must* happen in `self()`.

Do we have a rule to require even continuation without side effect to 
calling actor be defered?


- Zhitao Li


On Jan. 2, 2018, 10:38 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64885/
> ---
> 
> (Updated Jan. 2, 2018, 10:38 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Zhitao Li.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In `e273efe6976434858edb85bbcf367a02e963a467` we introduced layer
> checkpointing to the provisioner, but did not make sure that
> continutations making use of internal state of the provisioner actor
> installed on futures where always executed in the actor's context.
> This was problematic as continuations could be executed while actor
> state was changing (data races), or after the the actor had terminated
> (use after free).
> 
> In this patch we instead defer execution of continutions to the actor
> owning the data.
> 
> This issue was identified with the clang-tidy `mesos-this-capture`
> check.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 
> 61e771872ba2c3f5ecabbe085db102433c7bac3f 
> 
> 
> Diff: https://reviews.apache.org/r/64885/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Review Request 64888: Added check for handling of checkpointed resources in reregistration.

[Benjamin Bannier]

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

Review request for mesos, Benno Evers, Jie Yu, and Jan Schlicht.


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


Repository: mesos


Description
---

This patch adds a check that confirms that the master resends
checkpointed resources to the agent on reregistration.


Diffs
-

  src/tests/slave_recovery_tests.cpp bf2c5fcabdd4c16bdf5de1b641060e38783b8ee8 


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


Testing
---

`make check`


Thanks,

Benjamin Bannier

Review Request 64889: Fixed handling of checkpointed resources for RP-capable agents.

[Benjamin Bannier]

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

Review request for mesos, Benno Evers, Jie Yu, and Jan Schlicht.


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


Repository: mesos


Description
---

The master will not resend checkpointed resources when a resource
provider-capable agent reregisters. Instead the checkpointed resources
sent as part of the agent reregistration should be evaluated by the
master and be used to update its state.

This patch fixes the handling of checkpointed resources sent as part
of the agent reregistration so that the resources are used to update
the master state.


Diffs
-

  src/master/master.hpp 8fe9420dbe03ea2cefc6a40b0f64284aa9fe7915 
  src/master/master.cpp 04378a8d931ebe5f2667399ec9ce4225fa8c7c82 
  src/tests/slave_recovery_tests.cpp bf2c5fcabdd4c16bdf5de1b641060e38783b8ee8 


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


Testing
---

`make check`


Thanks,

Benjamin Bannier

Review Request 64885: Made sure continuations using actor state are executed in actor context.

[Benjamin Bannier]

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

Review request for mesos, Gilbert Song and Zhitao Li.


Repository: mesos


Description
---

In `e273efe6976434858edb85bbcf367a02e963a467` we introduced layer
checkpointing to the provisioner, but did not make sure that
continutations making use of internal state of the provisioner actor
installed on futures where always executed in the actor's context.
This was problematic as continuations could be executed while actor
state was changing (data races), or after the the actor had terminated
(use after free).

In this patch we instead defer execution of continutions to the actor
owning the data.

This issue was identified with the clang-tidy `mesos-this-capture`
check.


Diffs
-

  src/slave/containerizer/mesos/provisioner/provisioner.cpp 
61e771872ba2c3f5ecabbe085db102433c7bac3f 


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


Testing
---

`make check`


Thanks,

Benjamin Bannier