Re: Review Request 62762: Added `LocalResourceProvider::principal()` for authentication.

[Chun-Hung Hsiao]

> On Nov. 23, 2017, 5:27 a.m., Jie Yu wrote:
> > src/resource_provider/local.cpp
> > Lines 45 (patched)
> > 
> >
> > We try to avoid non-POD (plain old data) global variables because it'll 
> > cause issues during exit (see google style guide).
> > 
> > So I'll make this a helper function.

I'm thinking about following this style: 
https://github.com/apache/mesos/blob/master/src/slave/containerizer/mesos/containerizer.cpp#L360
Let me update the patch and please see if that is better.


> On Nov. 23, 2017, 5:27 a.m., Jie Yu wrote:
> > src/resource_provider/local.cpp
> > Lines 49 (patched)
> > 
> >
> > Do we still need this? It looks to me that the current naming scheme 
> > makes it so that you don't have to customize it for each LRP?

This is for future proof. What if a new LRP needs to use some other API?


- Chun-Hung


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


On Nov. 22, 2017, 3:10 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62762/
> ---
> 
> (Updated Nov. 22, 2017, 3:10 a.m.)
> 
> 
> Review request for mesos, Alexander Rojas, Greg Mann, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-8100
> https://issues.apache.org/jira/browse/MESOS-8100
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `LocalResourceProvider::principal()` function takes a
> `ResourceProviderInfo` and generates a principal with the following
> claim:
> 
>   {"cid_prefix", ---}
> 
> For example, for resource provider with type
> 'org.apache.mesos.rp.local.storage' and name 'foo', the claim would be:
> 
>   {"cid_prefix", "org-apache-mesos-rp-local-storage-foo--"}
> 
> In the future, we could add more claims for authorizing other
> operations, such as authorization for Resource Provider API.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/local.hpp ebaa07d03ad77d516066ee2d4b60864be0611b5f 
>   src/resource_provider/local.cpp ad98f333c5668ca81de6e7ed3fc8f59323b151da 
>   src/resource_provider/storage/provider.hpp 
> 6de88c2329b358fcf48bc39ddda0132170991c3c 
>   src/resource_provider/storage/provider.cpp 
> 46224997430ac0c568904d80014166a6f059907f 
> 
> 
> Diff: https://reviews.apache.org/r/62762/diff/8/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 64014: Added default reservations in `ResourceProviderInfo`.

[Jie Yu]

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




include/mesos/mesos.proto
Line 1035 (original)


Why this move? You should put this close to CSIPluginInfo and 
CSIPluginContianerInfo.



include/mesos/resources.hpp
Lines 65 (patched)


2 lines apart



include/mesos/v1/mesos.proto
Line 1027 (original)


Ditto.



src/common/type_utils.cpp
Lines 408 (patched)


Update src/v1/mesos.cpp


- Jie Yu


On Nov. 23, 2017, 1:38 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64014/
> ---
> 
> (Updated Nov. 23, 2017, 1:38 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, James DeFelice, Jie Yu, and 
> Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `default_reservations` field is used to set up the default
> reservation stack for new resources from the resource provider.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto e194093e490741acc552fd3ad328fd710b4b4435 
>   include/mesos/resources.hpp 08c544d05b6199ac77f8a71720c13eb0f2decf60 
>   include/mesos/v1/mesos.proto 6fb1139683952877667abbcf8bf84b5b31bcd29e 
>   src/common/type_utils.cpp 14267559f228f23aff2e21b987795b9f4fee93fc 
> 
> 
> Diff: https://reviews.apache.org/r/64014/diff/3/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 63904: Updated the top-level `CSIPluginInfo` proto.

[Jie Yu]

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




src/common/type_utils.cpp
Lines 102 (patched)


you need to update src/v1/mesos.cpp as well.


- Jie Yu


On Nov. 21, 2017, 5:32 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63904/
> ---
> 
> (Updated Nov. 21, 2017, 5:32 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, James DeFelice, Jie Yu, Joseph 
> Wu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-7561
> https://issues.apache.org/jira/browse/MESOS-7561
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `type` and `name` fields are introduced to uniquely identify a CSI
> service bundle. The purpose of the new proto and these two fields is to
> decouple a CSI service bundle from a `ResoruceProviderInfo`, so we may
> allow tasks to directly use non-resource volumes (such as NFS volumes).
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto e194093e490741acc552fd3ad328fd710b4b4435 
>   include/mesos/type_utils.hpp f7f23270fadc75a8737f32ec1c3fbc01f6321248 
>   include/mesos/v1/mesos.proto 6fb1139683952877667abbcf8bf84b5b31bcd29e 
>   src/common/type_utils.cpp 14267559f228f23aff2e21b987795b9f4fee93fc 
> 
> 
> Diff: https://reviews.apache.org/r/63904/diff/3/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 62762: Added `LocalResourceProvider::principal()` for authentication.

[Jie Yu]

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




src/resource_provider/local.cpp
Lines 35-40 (patched)


see my comments below, you probably do not need this struct anymore.



src/resource_provider/local.cpp
Lines 45 (patched)


We try to avoid non-POD (plain old data) global variables because it'll 
cause issues during exit (see google style guide).

So I'll make this a helper function.



src/resource_provider/local.cpp
Lines 49 (patched)


Do we still need this? It looks to me that the current naming scheme makes 
it so that you don't have to customize it for each LRP?


- Jie Yu


On Nov. 22, 2017, 3:10 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62762/
> ---
> 
> (Updated Nov. 22, 2017, 3:10 a.m.)
> 
> 
> Review request for mesos, Alexander Rojas, Greg Mann, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-8100
> https://issues.apache.org/jira/browse/MESOS-8100
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `LocalResourceProvider::principal()` function takes a
> `ResourceProviderInfo` and generates a principal with the following
> claim:
> 
>   {"cid_prefix", ---}
> 
> For example, for resource provider with type
> 'org.apache.mesos.rp.local.storage' and name 'foo', the claim would be:
> 
>   {"cid_prefix", "org-apache-mesos-rp-local-storage-foo--"}
> 
> In the future, we could add more claims for authorizing other
> operations, such as authorization for Resource Provider API.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/local.hpp ebaa07d03ad77d516066ee2d4b60864be0611b5f 
>   src/resource_provider/local.cpp ad98f333c5668ca81de6e7ed3fc8f59323b151da 
>   src/resource_provider/storage/provider.hpp 
> 6de88c2329b358fcf48bc39ddda0132170991c3c 
>   src/resource_provider/storage/provider.cpp 
> 46224997430ac0c568904d80014166a6f059907f 
> 
> 
> Diff: https://reviews.apache.org/r/62762/diff/8/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Review Request 64044: Killed unused CSI plugin containers for storage local resource provider.

[Chun-Hung Hsiao]

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

Review request for mesos, Benjamin Bannier, Jie Yu, Joseph Wu, and Jan Schlicht.


Repository: mesos


Description
---

During start up, the storage local resource provider will recover the
list of plugin containers from
`/csi///containers/`,
and kill containers that will no longer in use.


Diffs
-

  src/resource_provider/storage/provider.cpp 
46224997430ac0c568904d80014166a6f059907f 


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


Testing
---


Thanks,

Chun-Hung Hsiao

Re: Review Request 63021: Added `getService()` function to launch CSI plugins.

[Chun-Hung Hsiao]

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

(Updated Nov. 23, 2017, 2:21 a.m.)


Review request for mesos, James DeFelice, Jie Yu, Joseph Wu, and Jan Schlicht.


Changes
---

Refactor for the next patch.


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


Repository: mesos


Description
---

The `getService()` method first checks if there is already a container
daemon for the specified plugin component, and creates a new one if not.
The post-start hook for the container daemon will call `connect()` to
wait for the endpoint socket file to appear and connect to it, then
set up the corresponding promise of CSI client. The post-stop hook will
remove the socket file and create a new promise for the next call to the
post-start hook to set it up.


Diffs (updated)
-

  include/mesos/type_utils.hpp f7f23270fadc75a8737f32ec1c3fbc01f6321248 
  src/common/type_utils.cpp 14267559f228f23aff2e21b987795b9f4fee93fc 
  src/resource_provider/storage/provider.cpp 
46224997430ac0c568904d80014166a6f059907f 


Diff: https://reviews.apache.org/r/63021/diff/12/

Changes: https://reviews.apache.org/r/63021/diff/11-12/


Testing
---


Thanks,

Chun-Hung Hsiao

Re: Review Request 64014: Added default reservations in `ResourceProviderInfo`.

[Chun-Hung Hsiao]

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

(Updated Nov. 23, 2017, 1:38 a.m.)


Review request for mesos, Benjamin Bannier, James DeFelice, Jie Yu, and Joseph 
Wu.


Changes
---

Added missing changes.


Repository: mesos


Description (updated)
---

The `default_reservations` field is used to set up the default
reservation stack for new resources from the resource provider.


Diffs (updated)
-

  include/mesos/mesos.proto e194093e490741acc552fd3ad328fd710b4b4435 
  include/mesos/resources.hpp 08c544d05b6199ac77f8a71720c13eb0f2decf60 
  include/mesos/v1/mesos.proto 6fb1139683952877667abbcf8bf84b5b31bcd29e 
  src/common/type_utils.cpp 14267559f228f23aff2e21b987795b9f4fee93fc 


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

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


Testing
---

make


Thanks,

Chun-Hung Hsiao

Re: Review Request 64012: Added new --configuration_compatibility slave flag and implementation.

[Vinod Kone]

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



Partial review.


src/Makefile.am
Lines 1015 (patched)


CMakeLists?



src/slave/flags.hpp
Lines 93 (patched)


Remove the comment.



src/slave/reconfiguration.hpp
Lines 17-18 (patched)


this should be the same as the name of the file.

s/_CONFIGURATION_COMPATIBILITY_/RECONFIGURATION/



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


s/state_recovery/configuration_compatibility/



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


Put `&&` in the line above.



src/slave/slave.cpp
Lines 1036-1042 (patched)


just EXIT here? returning here is dangerous because further detection won't 
happen.



src/slave/slave.cpp
Lines 1232-1233 (patched)


remove this?


- Vinod Kone


On Nov. 22, 2017, 11:31 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64012/
> ---
> 
> (Updated Nov. 22, 2017, 11:31 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added new --configuration_compatibility slave flag and implementation.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 49dec55ced32945ad0414c32eb4e00247f4b54f2 
>   src/slave/flags.hpp 0c02b49c054ef2597298aefe0997beacfc9efeb8 
>   src/slave/flags.cpp 0eeecdc6023443a99c8e8fe29c2f7bf791c0a36e 
>   src/slave/reconfiguration.hpp PRE-CREATION 
>   src/slave/reconfiguration.cpp PRE-CREATION 
>   src/slave/slave.hpp 40442f25ae1b223380e11f8602e6256c9203ef47 
>   src/slave/slave.cpp d8edc5e6bbfa265bca4d19bbaa7db3063949dbc0 
>   src/tests/reconfiguration_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/64012/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 64011: Updated master behaviour to update agent state on reregistration.

[Vinod Kone]

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




src/master/master.hpp
Lines 1105 (patched)


s/agentCapabilities/capabilities/



src/master/master.cpp
Lines 6320 (patched)


s/reregistration/re-registration/



src/master/master.cpp
Lines 6321 (patched)


s/. (/(/
s/)/)./
s/state/SlaveInfo/



src/master/master.cpp
Lines 6323 (patched)


Can you file a JIRA for this?



src/master/master.cpp
Line 6478 (original), 6484 (patched)


Can you make sure to reject re-registration here if the slave is in 
`markingGone` or `gone` maps.



src/master/master.cpp
Lines 6489-6490 (patched)


s/in cases 1 and 2//



src/master/master.cpp
Line 6489 (original), 6501 (patched)


log the pid and hostname.



src/master/master.cpp
Line 6610 (original), 6543 (patched)


log the pid and hostname.



src/master/master.cpp
Lines 6626-6627 (original), 6559-6560 (patched)


Remove the first sentence. 

s/In the common case,/In the common case, we are here if/



src/master/master.cpp
Line 6659 (original), 6594 (patched)


s/success/future/



src/master/master.cpp
Line 6672 (original), 6609 (patched)


Reject if agent is being marked gone or already marked gone. Can you do 
this and the previous similar change in a separate review?



src/master/master.cpp
Lines 6847 (patched)


Reject if slave is marking gone or marked gone.



src/master/master.cpp
Lines 6854 (patched)


log the pid and hostname.



src/master/master.cpp
Lines 6861-6871 (patched)


Change this to:
```
CHECK_READY(updated);
CHECK(updated.get());

VLOG(1) << Registry updated for slave " < slaveInfo.id << " at " << pid << 
" (" slaveInfo.hostname() << ")";

```



src/master/master.cpp
Lines 6873 (patched)


s/failoverSlave()/`failoverSlave()`/

Can you expand a bit on the comment for future reference?



src/master/master.cpp
Lines 6901-6902 (patched)


indentation?

can you update the comment to not say anything about checkpoint resources. 
say instead that you would recover the framework info if necessary.



src/master/master.cpp
Line 6943 (original), 6997 (patched)


just inline it.



src/master/master.cpp
Line 11091 (original), 11146 (patched)


2 blank lines.



src/master/master.cpp
Lines 11147 (patched)


return `Try` here instead and reject re-registration if it's an 
error.



src/master/master.cpp
Lines 11170 (patched)


s/   slave/slave/


- Vinod Kone


On Nov. 22, 2017, 11:31 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64011/
> ---
> 
> (Updated Nov. 22, 2017, 11:31 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When an agent reregisters, the master will now always update
> the agent information it holds in memory, and will write any
> changes back to the registry if necessary.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp a309fc78ee2613762f3d5d22ac7559afc7aac4a3 
>   src/master/master.cpp 2ddd67ada3731803b00883b6a1f32b20c1bb238f 
>   src/tests/master_tests.cpp 9c450b9f592d9e09a468f537d9b500e97acc636b 
> 
> 
> Diff: https://reviews.apache.org/r/64011/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 64014: Added default reservations in `ResourceProviderInfo`.

[Chun-Hung Hsiao]

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

(Updated Nov. 23, 2017, 12:47 a.m.)


Review request for mesos, Benjamin Bannier, James DeFelice, Jie Yu, and Joseph 
Wu.


Changes
---

Name change.


Summary (updated)
-

Added default reservations in `ResourceProviderInfo`.


Repository: mesos


Description
---

The `reservations` field is used to set up the default reservation stack
for resources reported by the resource provider.


Diffs (updated)
-

  include/mesos/mesos.proto e194093e490741acc552fd3ad328fd710b4b4435 
  include/mesos/resources.hpp 08c544d05b6199ac77f8a71720c13eb0f2decf60 
  include/mesos/v1/mesos.proto 6fb1139683952877667abbcf8bf84b5b31bcd29e 
  src/common/type_utils.cpp 14267559f228f23aff2e21b987795b9f4fee93fc 


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

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


Testing
---

make


Thanks,

Chun-Hung Hsiao

Re: Review Request 63798: Added resource provider support for all offer operations.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Nov. 22, 2017, 12:15 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63798/
> ---
> 
> (Updated Nov. 22, 2017, 12:15 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Bugs: MESOS-8218
> https://issues.apache.org/jira/browse/MESOS-8218
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `RESERVE`/`UNRESERVE`/`CREATE`/`DESTROY` operations for
> reservations and persistent volumes have been enabled for resource
> provider resources. Similar to agent resources, they are speculatively
> applied to allow offer pipelining.
> 
> 
> Diffs
> -
> 
>   src/common/resources_utils.cpp 7c48704638dbfd0e0f5890765026537caf40e73c 
>   src/master/master.cpp 7417b5d641fd4bb6d91cb0e6456c60201bbc8206 
>   src/slave/slave.hpp f86ebd6b28ba2f6dfbf5acc5371af1573d5a4e96 
>   src/slave/slave.cpp 491419443a1de92c4a77049660e7d7c6c708cd52 
>   src/tests/resource_provider_manager_tests.cpp 
> ecfe2b4c0952838d6312df603f8eb2f458725175 
>   src/tests/slave_tests.cpp 6033c7a674b12fc81bae28d73ed67d655b2b046c 
> 
> 
> Diff: https://reviews.apache.org/r/63798/diff/5/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 63751: Triggered 'ApplyOfferOperationMessage' for agent local resources.

[Jie Yu]

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


Fix it, then Ship it!





src/slave/slave.cpp
Lines 3763-3764 (patched)


THis should be `updateOfferOperation(update);`


- Jie Yu


On Nov. 22, 2017, 12:11 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63751/
> ---
> 
> (Updated Nov. 22, 2017, 12:11 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Bugs: MESOS-8211
> https://issues.apache.org/jira/browse/MESOS-8211
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Agents the have the 'RESOURCE_PROVIDER' capability set, should get sent
> 'ApplyOfferOperationMessage' instead of 'CheckpointResourcesMessage'.
> The agent will then figure out how to apply the operation. For agent
> local resources the agent will checkpoint resources.
> 
> 
> Diffs
> -
> 
>   src/common/protobuf_utils.hpp b2aa3654db2fe7d7d9d275ded81c6d54244654ee 
>   src/common/protobuf_utils.cpp 34054846f93f19ba550afe58e2a899d111ad38dc 
>   src/master/master.cpp 7417b5d641fd4bb6d91cb0e6456c60201bbc8206 
>   src/slave/slave.cpp 491419443a1de92c4a77049660e7d7c6c708cd52 
>   src/tests/persistent_volume_tests.cpp 
> acfeac16884b00581a3523607ff26f44f6dca53a 
>   src/tests/reservation_tests.cpp 470f7341686e69d0a71fb234a26b277c45c29780 
> 
> 
> Diff: https://reviews.apache.org/r/63751/diff/8/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Review Request 64012: Added new --configuration_compatibility slave flag and implementation.

[Benno Evers]

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

Review request for mesos and Vinod Kone.


Repository: mesos


Description
---

Added new --configuration_compatibility slave flag and implementation.


Diffs
-

  src/Makefile.am 49dec55ced32945ad0414c32eb4e00247f4b54f2 
  src/slave/flags.hpp 0c02b49c054ef2597298aefe0997beacfc9efeb8 
  src/slave/flags.cpp 0eeecdc6023443a99c8e8fe29c2f7bf791c0a36e 
  src/slave/reconfiguration.hpp PRE-CREATION 
  src/slave/reconfiguration.cpp PRE-CREATION 
  src/slave/slave.hpp 40442f25ae1b223380e11f8602e6256c9203ef47 
  src/slave/slave.cpp d8edc5e6bbfa265bca4d19bbaa7db3063949dbc0 
  src/tests/reconfiguration_tests.cpp PRE-CREATION 


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


Testing
---


Thanks,

Benno Evers

Review Request 64011: Updated master behaviour to update agent state on reregistration.

[Benno Evers]

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

Review request for mesos and Vinod Kone.


Repository: mesos


Description
---

When an agent reregisters, the master will now always update
the agent information it holds in memory, and will write any
changes back to the registry if necessary.


Diffs
-

  src/master/master.hpp a309fc78ee2613762f3d5d22ac7559afc7aac4a3 
  src/master/master.cpp 2ddd67ada3731803b00883b6a1f32b20c1bb238f 
  src/tests/master_tests.cpp 9c450b9f592d9e09a468f537d9b500e97acc636b 


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


Testing
---


Thanks,

Benno Evers

Re: Review Request 60471: Added tests for pruneImages for containerizer and provisioner.

[Zhitao Li]

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

(Updated Nov. 22, 2017, 10:50 p.m.)


Review request for mesos, Gilbert Song and Jason Lai.


Changes
---

Rebase.


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


Repository: mesos


Description
---

Added tests for pruneImages for containerizer and provisioner.


Diffs (updated)
-

  src/tests/containerizer/mesos_containerizer_tests.cpp 
98adcfcfba4e5ee975b7ed0c073758ec2726763d 
  src/tests/containerizer/provisioner_docker_tests.cpp 
832c81fe88d753b0f00dfab870d7725cf556fcef 


Diff: https://reviews.apache.org/r/60471/diff/12/

Changes: https://reviews.apache.org/r/60471/diff/11-12/


Testing
---


Thanks,

Zhitao Li

Re: Review Request 56722: Added a new operator API for `PRUNE_IMAGES`.

[Zhitao Li]

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

(Updated Nov. 22, 2017, 10:50 p.m.)


Review request for mesos, Gilbert Song, Jason Lai, and Jie Yu.


Changes
---

Remove unused message.


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


Repository: mesos


Description
---

Added a new operator API for `PRUNE_IMAGES`.


Diffs (updated)
-

  include/mesos/agent/agent.proto 0f92f73ba0f7729f0ba7cd89a692ab3685125e8b 
  include/mesos/v1/agent/agent.proto 012ffef5f0dd7720fa95ae484c99479aaf256d7b 
  src/slave/http.hpp a51831cdcebc1998ce6d4c3c19285e598a4ec9a3 
  src/slave/http.cpp 394e91013dc11e0a79e2e00534864281cc74ad2f 
  src/slave/validation.cpp 32781fd8f124f71e61744804aec3fe4da59a5df2 


Diff: https://reviews.apache.org/r/56722/diff/8/

Changes: https://reviews.apache.org/r/56722/diff/7-8/


Testing
---

1. New unit test to trigger the new call.
2. Manually tested that images previous pulled but not running can be purged 
through new operator API call while active images (running or being pulled) are 
not affected.


Thanks,

Zhitao Li

Re: Review Request 62853: Added test for `PRUNE_IMAGES` operator API call.

[Zhitao Li]

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

(Updated Nov. 22, 2017, 10:50 p.m.)


Review request for mesos, Gilbert Song and Jason Lai.


Changes
---

Rebase.


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


Repository: mesos


Description
---

Added test for `PRUNE_IMAGES` operator API call.


Diffs (updated)
-

  src/tests/api_tests.cpp 66cb059c96ff9ecda594e13de9e5dc3909f3 


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

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


Testing
---


Thanks,

Zhitao Li

Re: Review Request 63989: Added the OFFER_OPERATION_DROPPED state.

[Gaston Kleiman]

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


Ship it!




Ship It!

- Gaston Kleiman


On Nov. 21, 2017, 9:21 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63989/
> ---
> 
> (Updated Nov. 21, 2017, 9:21 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Gaston Kleiman, Jie Yu, and Jan 
> Schlicht.
> 
> 
> Bugs: MESOS-8190
> https://issues.apache.org/jira/browse/MESOS-8190
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds a new state for offer operations,
> OFFER_OPERATION_DROPPED, which is meant to be analogous
> to the TASK_DROPPED task state. The master and agent will
> send this state in offer operation updates when an
> operation is dropped due to a transient error.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto e194093e490741acc552fd3ad328fd710b4b4435 
>   include/mesos/v1/mesos.proto 6fb1139683952877667abbcf8bf84b5b31bcd29e 
>   src/common/protobuf_utils.cpp 34054846f93f19ba550afe58e2a899d111ad38dc 
>   src/master/master.cpp 7417b5d641fd4bb6d91cb0e6456c60201bbc8206 
>   src/slave/slave.cpp 6e9adc60f593faf1b0e56caeea04882f67af7080 
> 
> 
> Diff: https://reviews.apache.org/r/63989/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 63988: Fixed indentation in a test header.

[Gaston Kleiman]

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


Ship it!




Ship It!

- Gaston Kleiman


On Nov. 21, 2017, 9:10 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63988/
> ---
> 
> (Updated Nov. 21, 2017, 9:10 a.m.)
> 
> 
> Review request for mesos and Gaston Kleiman.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed indentation in a test header.
> 
> 
> Diffs
> -
> 
>   src/tests/mesos.hpp 345b883a8c629bf5bed83e9236632c277f2eb0eb 
> 
> 
> Diff: https://reviews.apache.org/r/63988/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 64028: Removed duplicate resources conversions.

[Benjamin Mahler]

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



Should we be implementing a += overload against `RepeatedPtrField` 
instead of doing this? Would that be more efficient than doing a conversion 
then adding? I'm also hoping to avoid code having to this type of conversion, 
ideally :)

- Benjamin Mahler


On Nov. 22, 2017, 1:24 p.m., Dmitry Zhuk wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64028/
> ---
> 
> (Updated Nov. 22, 2017, 1:24 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `RepeatedPtrField` can be implicitly converted to `Resources`,
> leading to hidden multiple resources conversions on performance-critical
> paths in master. For example, `operator +=` relies on implicit
> conversion, when invoked with `RepeatedPtrField` argument.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 2a2e830354db4a2191fb8321beb8174b80f7ba7d 
>   src/master/master.cpp 7417b5d641fd4bb6d91cb0e6456c60201bbc8206 
> 
> 
> Diff: https://reviews.apache.org/r/64028/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Dmitry Zhuk
> 
>

Re: Review Request 63959: Optimized resources logging in master.

[Benjamin Mahler]

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


Fix it, then Ship it!




Thanks! Left a comment below but will take care of this for you!


src/master/master.cpp
Lines 6869-6870 (patched)


Thanks for the comment! Looks like we would benefit from them in the three 
additional cases below as well?


- Benjamin Mahler


On Nov. 22, 2017, 1:24 p.m., Dmitry Zhuk wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63959/
> ---
> 
> (Updated Nov. 22, 2017, 1:24 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When master logs agent or task resources, it uses `operator <<` for raw
> protobuf data, which outputs resources in JSON format, and is rather
> slow. However resources are known to be valid and refined when logged by
> master, so it's faster to use `operator <<` after protobuf is converted
> to `Resources`.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 7417b5d641fd4bb6d91cb0e6456c60201bbc8206 
> 
> 
> Diff: https://reviews.apache.org/r/63959/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Dmitry Zhuk
> 
>

Re: Review Request 64033: Terminated executors if kill task received before launch task.

[Vinod Kone]

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



can you write tests please?


src/docker/executor.cpp
Lines 390 (patched)


`return` here for clarity?

Also, please log a message because this is a rare case.



src/launcher/executor.cpp
Lines 782 (patched)


`return` here?

Also, please log a message because this is a rare case.


- Vinod Kone


On Nov. 22, 2017, 5:05 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64033/
> ---
> 
> (Updated Nov. 22, 2017, 5:05 p.m.)
> 
> 
> Review request for mesos, Andrei Budnik, Armand Grillet, and Vinod Kone.
> 
> 
> Bugs: MESOS-8247
> https://issues.apache.org/jira/browse/MESOS-8247
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `ExecutorRegisteredMessage` or `RunTaskMessage` may not be delivered
> to a driver-based executor. Since these messages are not retried,
> the executor will never start a task and will remain idle, ignoring
> kill task request. This patch shutdown driver-based executors if kill
> task arrives before the task has been started.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp 3974f20052e3c12eb154a5146d19d4dc1759859f 
>   src/launcher/executor.cpp c688c04e598ac140421fd5e47359b0e48d30bcc5 
> 
> 
> Diff: https://reviews.apache.org/r/64033/diff/1/
> 
> 
> Testing
> ---
> 
> make check on MacOS 10.11.6
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 64032: Promoted log level to warning for disconnected events in exec.cpp.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Nov. 22, 2017, 4:15 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64032/
> ---
> 
> (Updated Nov. 22, 2017, 4:15 p.m.)
> 
> 
> Review request for mesos, Andrei Budnik, Armand Grillet, and Vinod Kone.
> 
> 
> Bugs: MESOS-8247
> https://issues.apache.org/jira/browse/MESOS-8247
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When the executor library receives messages while being disconnected,
> it might indicate an out-of-order message delivery or lost messages.
> This should be logged at the warning level to simplify triaging.
> 
> 
> Diffs
> -
> 
>   src/exec/exec.cpp cdbf149a9047ddad6beef64be1266e15e7643afc 
> 
> 
> Diff: https://reviews.apache.org/r/64032/diff/1/
> 
> 
> Testing
> ---
> 
> None: not a functional change.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 64033: Terminated executors if kill task received before launch task.

[Alexander Rukletsov]

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

(Updated Nov. 22, 2017, 5:05 p.m.)


Review request for mesos, Andrei Budnik, Armand Grillet, and Vinod Kone.


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


Repository: mesos


Description
---

`ExecutorRegisteredMessage` or `RunTaskMessage` may not be delivered
to a driver-based executor. Since these messages are not retried,
the executor will never start a task and will remain idle, ignoring
kill task request. This patch shutdown driver-based executors if kill
task arrives before the task has been started.


Diffs
-

  src/docker/executor.cpp 3974f20052e3c12eb154a5146d19d4dc1759859f 
  src/launcher/executor.cpp c688c04e598ac140421fd5e47359b0e48d30bcc5 


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


Testing (updated)
---

make check on MacOS 10.11.6


Thanks,

Alexander Rukletsov

Review Request 64032: Promoted log level to warning for disconnected events in exec.cpp.

[Alexander Rukletsov]

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

Review request for mesos, Andrei Budnik, Armand Grillet, and Vinod Kone.


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


Repository: mesos


Description
---

When the executor library receives messages while being disconnected,
it might indicate an out-of-order message delivery or lost messages.
This should be logged at the warning level to simplify triaging.


Diffs
-

  src/exec/exec.cpp cdbf149a9047ddad6beef64be1266e15e7643afc 


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


Testing
---

None: not a functional change.


Thanks,

Alexander Rukletsov

Review Request 64033: Terminated executors if kill task received before launch task.

[Alexander Rukletsov]

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

Review request for mesos, Andrei Budnik, Armand Grillet, and Vinod Kone.


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


Repository: mesos


Description
---

`ExecutorRegisteredMessage` or `RunTaskMessage` may not be delivered
to a driver-based executor. Since these messages are not retried,
the executor will never start a task and will remain idle, ignoring
kill task request. This patch shutdown driver-based executors if kill
task arrives before the task has been started.


Diffs
-

  src/docker/executor.cpp 3974f20052e3c12eb154a5146d19d4dc1759859f 
  src/launcher/executor.cpp c688c04e598ac140421fd5e47359b0e48d30bcc5 


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


Testing
---

In progress.


Thanks,

Alexander Rukletsov

Re: Review Request 63953: Added logging based on container class.

[Armand Grillet]

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

(Updated Nov. 22, 2017, 4 p.m.)


Review request for mesos and Alexander Rukletsov.


Changes
---

Replaced `using` by `include`.


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


Repository: mesos


Description
---

This change enables filtering when printing logs regarding
containers. If its class is `DEBUG` we print the log line at a
verbose level 1 otherwise we print is at the `INFO` level.

We use the added macro in containerizer.cpp so that COMMAND health
checks cause less INFO logs (15 instead of 26 before).


Diffs (updated)
-

  src/Makefile.am 9641ad4cf2a1f7fe56a4dcccb6eeff7130872fa8 
  src/logging/utils.hpp PRE-CREATION 
  src/slave/containerizer/mesos/containerizer.cpp 
7f3b86d87cf82429c2627d4a32eb0d5adbcc3f29 


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

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


Testing
---

Started a Mesos cluster and used `mesos-execute` with this task group to check 
that the behaviour after this patch was the one expected:

```
{
  "tasks": [
{
  "name": "Name of the task",
  "task_id": {
"value": "task-group"
  },
  "agent_id": {
"value": ""
  },
  "resources": [
{
  "name": "cpus",
  "type": "SCALAR",
  "scalar": {
"value": 0.01
  }
},
{
  "name": "mem",
  "type": "SCALAR",
  "scalar": {
"value": 2
  }
}
  ],
  "command": {
"value": "sleep 1000"
  },
  "check": {
"type": "COMMAND",
"command": {
  "command": {
"value": "echo \"Bonjour\""
  },
  "uris": []
}
  }
}
  ]
}
```

And:
```
$ nice make check
```


Thanks,

Armand Grillet

Re: Review Request 63795: Made `mesos-tcp-connect` support IPv6.

[Qian Zhang]

> On Nov. 22, 2017, 8:16 p.m., Avinash sridharan wrote:
> > src/checks/tcp_connect.cpp
> > Lines 90 (patched)
> > 
> >
> > Should have bought this up earlier. I think this code will be a lot 
> > more simpler if you use `network::Address` to represent the ip:port address.
> > 
> > https://github.com/apache/mesos/blob/master/3rdparty/libprocess/include/process/address.hpp#L277
> > 
> > You wouldn't need to maintain family specific code here and you can 
> > directly use the 
> > `sock_storage` cast operator
> > 
> > https://github.com/apache/mesos/blob/master/3rdparty/libprocess/include/process/address.hpp#L377

Good idea!


- Qian


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


On Nov. 22, 2017, 11:14 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63795/
> ---
> 
> (Updated Nov. 22, 2017, 11:14 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Avinash sridharan.
> 
> 
> Bugs: MESOS-8050
> https://issues.apache.org/jira/browse/MESOS-8050
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made `mesos-tcp-connect` support IPv6.
> 
> 
> Diffs
> -
> 
>   src/checks/tcp_connect.cpp 2514f4aebcc634b18cd2b3c36529222fe456e903 
> 
> 
> Diff: https://reviews.apache.org/r/63795/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 63795: Made `mesos-tcp-connect` support IPv6.

[Qian Zhang]

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

(Updated Nov. 22, 2017, 11:14 p.m.)


Review request for mesos, Alexander Rukletsov and Avinash sridharan.


Changes
---

Addressed review comments.


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


Repository: mesos


Description
---

Made `mesos-tcp-connect` support IPv6.


Diffs (updated)
-

  src/checks/tcp_connect.cpp 2514f4aebcc634b18cd2b3c36529222fe456e903 


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

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


Testing
---


Thanks,

Qian Zhang

Re: Review Request 63636: Added placeholder implementation.

[Dmitry Zhuk]

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

(Updated Nov. 22, 2017, 2:13 p.m.)


Review request for mesos, Benjamin Mahler and Michael Park.


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


Repository: mesos


Description
---

Added placeholder implementation.


Diffs (updated)
-

  3rdparty/stout/include/stout/lambda.hpp 
a61d97b69e7eebd057c94148d39c6e1fc3066017 


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

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


Testing
---


Thanks,

Dmitry Zhuk

Re: Review Request 63633: Added ability to protect nested bind expressions from composition.

[Dmitry Zhuk]

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

(Updated Nov. 22, 2017, 2:12 p.m.)


Review request for mesos, Benjamin Mahler and Michael Park.


Changes
---

Fixed code style.


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


Repository: mesos


Description
---

`std::bind` performs function composition when it encounters nested bind
expression. However in some cases this can have undesirable effects, and
`lambda::protect` can be used to avoid such behavior.


Diffs (updated)
-

  3rdparty/stout/include/stout/lambda.hpp 
a61d97b69e7eebd057c94148d39c6e1fc3066017 


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

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


Testing
---


Thanks,

Dmitry Zhuk

Re: Review Request 63630: Added support for callable once functors.

[Dmitry Zhuk]

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

(Updated Nov. 22, 2017, 2:10 p.m.)


Review request for mesos, Benjamin Mahler and Michael Park.


Changes
---

Fixed code style.


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


Repository: mesos


Description
---

`CallableOnce` class is similar to `std::function`, but allows it to be
called only once. Together with `partial` this provides foundation for
copy-less `defer`, `dispatch` and `Future`.


Diffs (updated)
-

  3rdparty/stout/include/stout/lambda.hpp 
a61d97b69e7eebd057c94148d39c6e1fc3066017 


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

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


Testing
---


Thanks,

Dmitry Zhuk

Re: Review Request 63629: Added partial function application implementation.

[Dmitry Zhuk]

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

(Updated Nov. 22, 2017, 2:09 p.m.)


Review request for mesos, Benjamin Mahler and Michael Park.


Changes
---

Fixed code style.


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


Repository: mesos


Description
---

`lambda::partial` performs partial function application,
similar to `std::bind`. However, unlike `std::bind`, it supports
functors accepting rvalue reference arguments. In this case `operator()`
must be called on `std::move`d partial.


Diffs (updated)
-

  3rdparty/stout/include/stout/lambda.hpp 
a61d97b69e7eebd057c94148d39c6e1fc3066017 
  3rdparty/stout/tests/lambda_tests.cpp 
ad8c2efddb6b64184670d0cfb33188ef843351ab 


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

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


Testing
---

make check


Thanks,

Dmitry Zhuk

Re: Review Request 63628: Implemented index_sequence and related functionality.

[Dmitry Zhuk]

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

(Updated Nov. 22, 2017, 2:07 p.m.)


Review request for mesos, Benjamin Mahler and Michael Park.


Changes
---

Fixed review comments.


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


Repository: mesos


Description
---

This adds implementation of C++14 STL's `index_squence`
and `make_index_sequence` templates.


Diffs (updated)
-

  3rdparty/stout/include/stout/utils.hpp 
82cab830817842c23056235973ec4419ee9ec8b1 


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

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


Testing
---


Thanks,

Dmitry Zhuk

Review Request 64028: Removed duplicate resources conversions.

[Dmitry Zhuk]

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

Review request for mesos, Benjamin Mahler and Michael Park.


Repository: mesos


Description
---

`RepeatedPtrField` can be implicitly converted to `Resources`,
leading to hidden multiple resources conversions on performance-critical
paths in master. For example, `operator +=` relies on implicit
conversion, when invoked with `RepeatedPtrField` argument.


Diffs
-

  src/master/master.hpp 2a2e830354db4a2191fb8321beb8174b80f7ba7d 
  src/master/master.cpp 7417b5d641fd4bb6d91cb0e6456c60201bbc8206 


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


Testing
---

make check


Thanks,

Dmitry Zhuk

Re: Review Request 63959: Optimized resources logging in master.

[Dmitry Zhuk]

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

(Updated Nov. 22, 2017, 1:24 p.m.)


Review request for mesos, Benjamin Mahler and Michael Park.


Summary (updated)
-

Optimized resources logging in master.


Repository: mesos


Description (updated)
---

When master logs agent or task resources, it uses `operator <<` for raw
protobuf data, which outputs resources in JSON format, and is rather
slow. However resources are known to be valid and refined when logged by
master, so it's faster to use `operator <<` after protobuf is converted
to `Resources`.


Diffs (updated)
-

  src/master/master.cpp 7417b5d641fd4bb6d91cb0e6456c60201bbc8206 


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

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


Testing
---

make check


Thanks,

Dmitry Zhuk

Re: Review Request 63910: Added 3 tests for TCP/HTTP(S) health check support for Docker container.

[Avinash sridharan]

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


Ship it!




Ship It!

- Avinash sridharan


On Nov. 19, 2017, 1:41 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63910/
> ---
> 
> (Updated Nov. 19, 2017, 1:41 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Avinash sridharan.
> 
> 
> Bugs: MESOS-8050
> https://issues.apache.org/jira/browse/MESOS-8050
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed 3 tests which only run on IPv4 Docker network.
>   ROOT_DOCKER_DockerHealthyTaskViaHTTP
>   ROOT_DOCKER_DockerHealthyTaskViaHTTPS
>   ROOT_DOCKER_DockerHealthyTaskViaTCP
> 
> Added 3 tests which run on both IPv4 and IPv6 Docker networks, so they
> should cover the above 3 tests.
>   ROOT_DOCKER_USERNETWORK_HealthyTaskViaHTTP
>   ROOT_DOCKER_USERNETWORK_HealthyTaskViaHTTPS
>   ROOT_DOCKER_USERNETWORK_HealthyTaskViaTCP
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 67945ddc4f98ffa072f584af8106967e7ff336d3 
>   src/tests/health_check_tests.cpp c0dcba265363f2149b217b189ee5a8e925e40ea1 
>   src/tests/mesos.hpp 345b883a8c629bf5bed83e9236632c277f2eb0eb 
> 
> 
> Diff: https://reviews.apache.org/r/63910/diff/2/
> 
> 
> Testing
> ---
> 
> Ran all newly added tests repeatedly (20 times), they all succeeded.
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 63910: Added 3 tests for TCP/HTTP(S) health check support for Docker container.

[Avinash sridharan]

> On Nov. 17, 2017, 7:25 p.m., Avinash sridharan wrote:
> > src/tests/health_check_tests.cpp
> > Lines 2450 (patched)
> > 
> >
> > We should try and see if we can publish this image to 
> > :https://hub.docker.com/u/mesos/
> > 
> > Also, I assuming this a custorm image. We should commit the 
> > `Dockerfile` for this image to our test repo. More importantly want to the 
> > cert being used in this test to be part of the repo.
> 
> Qian Zhang wrote:
> Agree, do you know who has the permission to publish image to 
> https://hub.docker.com/u/mesos/? And for the Dockerfile & the cert (and even 
> the Python code `https_server.py`), I think we can just put them in the 
> description of the Docker image so that we can see all of them in the same 
> place.

Qian for the time being lets just commit the Dockerfile. We can figure out the 
uploading of the image in a separate commit. We need to get consensus from 
AlexR as to where the Dockerfile should reside in the repo. If necessary we can 
this as a separate patch after we commit this patch. Please feel free to drop 
this if we are going to do this in a separate patch.


> On Nov. 17, 2017, 7:25 p.m., Avinash sridharan wrote:
> > src/tests/health_check_tests.cpp
> > Lines 2525 (patched)
> > 
> >
> > As you mentioned, given that this test is identical to the HTTP test 
> > above, can we just parameterize the `HealthCheck.type` field as well in the 
> > above test case?
> 
> Qian Zhang wrote:
> Yeah, I thought about that before, but the problem is, besides 
> `ROOT_DOCKER_USERNETWORK_HealthyTaskViaHTTP` and 
> `ROOT_DOCKER_USERNETWORK_HealthyTaskViaTCP`, we have another test 
> `ROOT_DOCKER_USERNETWORK_HealthyTaskViaHTTPS`, I am not sure how it can work 
> if we parameterize the `HealthCheck.type` field as well.

Understood, we take this up as an optimization in a separate commit.


- Avinash


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


On Nov. 19, 2017, 1:41 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63910/
> ---
> 
> (Updated Nov. 19, 2017, 1:41 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Avinash sridharan.
> 
> 
> Bugs: MESOS-8050
> https://issues.apache.org/jira/browse/MESOS-8050
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed 3 tests which only run on IPv4 Docker network.
>   ROOT_DOCKER_DockerHealthyTaskViaHTTP
>   ROOT_DOCKER_DockerHealthyTaskViaHTTPS
>   ROOT_DOCKER_DockerHealthyTaskViaTCP
> 
> Added 3 tests which run on both IPv4 and IPv6 Docker networks, so they
> should cover the above 3 tests.
>   ROOT_DOCKER_USERNETWORK_HealthyTaskViaHTTP
>   ROOT_DOCKER_USERNETWORK_HealthyTaskViaHTTPS
>   ROOT_DOCKER_USERNETWORK_HealthyTaskViaTCP
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 67945ddc4f98ffa072f584af8106967e7ff336d3 
>   src/tests/health_check_tests.cpp c0dcba265363f2149b217b189ee5a8e925e40ea1 
>   src/tests/mesos.hpp 345b883a8c629bf5bed83e9236632c277f2eb0eb 
> 
> 
> Diff: https://reviews.apache.org/r/63910/diff/2/
> 
> 
> Testing
> ---
> 
> Ran all newly added tests repeatedly (20 times), they all succeeded.
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 63795: Made `mesos-tcp-connect` support IPv6.

[Avinash sridharan]

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




src/checks/tcp_connect.cpp
Lines 90 (patched)


Should have bought this up earlier. I think this code will be a lot more 
simpler if you use `network::Address` to represent the ip:port address.

https://github.com/apache/mesos/blob/master/3rdparty/libprocess/include/process/address.hpp#L277

You wouldn't need to maintain family specific code here and you can 
directly use the 
`sock_storage` cast operator

https://github.com/apache/mesos/blob/master/3rdparty/libprocess/include/process/address.hpp#L377


- Avinash sridharan


On Nov. 17, 2017, 12:57 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63795/
> ---
> 
> (Updated Nov. 17, 2017, 12:57 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Avinash sridharan.
> 
> 
> Bugs: MESOS-8050
> https://issues.apache.org/jira/browse/MESOS-8050
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made `mesos-tcp-connect` support IPv6.
> 
> 
> Diffs
> -
> 
>   src/checks/tcp_connect.cpp 2514f4aebcc634b18cd2b3c36529222fe456e903 
> 
> 
> Diff: https://reviews.apache.org/r/63795/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 63798: Added resource provider support for all offer operations.

[Jan Schlicht]

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

(Updated Nov. 22, 2017, 1:15 p.m.)


Review request for mesos, Benjamin Bannier and Jie Yu.


Changes
---

Addressed issues.


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


Repository: mesos


Description
---

The `RESERVE`/`UNRESERVE`/`CREATE`/`DESTROY` operations for
reservations and persistent volumes have been enabled for resource
provider resources. Similar to agent resources, they are speculatively
applied to allow offer pipelining.


Diffs (updated)
-

  src/common/resources_utils.cpp 7c48704638dbfd0e0f5890765026537caf40e73c 
  src/master/master.cpp 7417b5d641fd4bb6d91cb0e6456c60201bbc8206 
  src/slave/slave.hpp f86ebd6b28ba2f6dfbf5acc5371af1573d5a4e96 
  src/slave/slave.cpp 491419443a1de92c4a77049660e7d7c6c708cd52 
  src/tests/resource_provider_manager_tests.cpp 
ecfe2b4c0952838d6312df603f8eb2f458725175 
  src/tests/slave_tests.cpp 6033c7a674b12fc81bae28d73ed67d655b2b046c 


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

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


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 63751: Triggered 'ApplyOfferOperationMessage' for agent local resources.

[Jan Schlicht]

> On Nov. 16, 2017, 12:37 a.m., Jie Yu wrote:
> > src/slave/slave.cpp
> > Lines 3743 (patched)
> > 
> >
> > For old operations, I think we should apply the conversion (i.e., 
> > change `totalResources`) in `addOfferOperation` becuase we speculatively 
> > assume that it'll succeeds.
> > 
> > This makes sense for old operation that has a resource provider. The 
> > current code is buggy because we don't update agent `totalResources` for 
> > those old operations.
> > 
> > Then, you don't have to do this assignment here anymore. We still need 
> > to calculate the checkpointed resources so that we can call the old 
> > checkpointResources handler.
> > 
> > One unfortunate thing is that `checkpointResources` method will also 
> > set `totalResources`. This is in fact a bug:
> > 
> > ```
> >   // This is a sanity check to verify that the new checkpointed
> >   // resources are compatible with the agent resources specified
> >   // through the '--resources' command line flag. The resources
> >   // should be guaranteed compatible by the master.
> >   Try _totalResources = applyCheckpointedResources(
> >   info.resources(),
> >   newCheckpointedResources);
> > 
> >   CHECK_SOME(_totalResources)
> > << "Failed to apply checkpointed resources "
> > << newCheckpointedResources << " to agent's resources "
> > << info.resources();
> > 
> >   totalResources = _totalResources.get();
> > ```
> > 
> > Setting agent resources to `_totalResources.get()` will break the agent 
> > resource accounting (if resource provider has resources). We might need to 
> > introduce a parameter to `checkpointResources` to not update 
> > `totalResources`.

This is addressed in #63798.


> On Nov. 16, 2017, 12:37 a.m., Jie Yu wrote:
> > src/tests/persistent_volume_tests.cpp
> > Lines 94 (patched)
> > 
> >
> > Can you add a comment about what that `bool` represents? Ditto in 
> > ReservationTest

Changed it to an enumeration, as that is more clear than a boolean and can be 
named.


- Jan


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


On Nov. 22, 2017, 1:11 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63751/
> ---
> 
> (Updated Nov. 22, 2017, 1:11 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Bugs: MESOS-8211
> https://issues.apache.org/jira/browse/MESOS-8211
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Agents the have the 'RESOURCE_PROVIDER' capability set, should get sent
> 'ApplyOfferOperationMessage' instead of 'CheckpointResourcesMessage'.
> The agent will then figure out how to apply the operation. For agent
> local resources the agent will checkpoint resources.
> 
> 
> Diffs
> -
> 
>   src/common/protobuf_utils.hpp b2aa3654db2fe7d7d9d275ded81c6d54244654ee 
>   src/common/protobuf_utils.cpp 34054846f93f19ba550afe58e2a899d111ad38dc 
>   src/master/master.cpp 7417b5d641fd4bb6d91cb0e6456c60201bbc8206 
>   src/slave/slave.cpp 491419443a1de92c4a77049660e7d7c6c708cd52 
>   src/tests/persistent_volume_tests.cpp 
> acfeac16884b00581a3523607ff26f44f6dca53a 
>   src/tests/reservation_tests.cpp 470f7341686e69d0a71fb234a26b277c45c29780 
> 
> 
> Diff: https://reviews.apache.org/r/63751/diff/8/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 63751: Triggered 'ApplyOfferOperationMessage' for agent local resources.

[Jan Schlicht]

> On Nov. 15, 2017, 10:53 p.m., Jie Yu wrote:
> > src/slave/slave.cpp
> > Lines 3751-3757 (patched)
> > 
> >
> > Are we leaking `OfferOperation` in the agent memory if we don't call 
> > `updateOfferOperation` and then `removeOfferOperation`?

I've changed the code to remove speculatively applied offer operation after 
sending an update to the master.


- Jan


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


On Nov. 22, 2017, 1:11 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63751/
> ---
> 
> (Updated Nov. 22, 2017, 1:11 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Bugs: MESOS-8211
> https://issues.apache.org/jira/browse/MESOS-8211
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Agents the have the 'RESOURCE_PROVIDER' capability set, should get sent
> 'ApplyOfferOperationMessage' instead of 'CheckpointResourcesMessage'.
> The agent will then figure out how to apply the operation. For agent
> local resources the agent will checkpoint resources.
> 
> 
> Diffs
> -
> 
>   src/common/protobuf_utils.hpp b2aa3654db2fe7d7d9d275ded81c6d54244654ee 
>   src/common/protobuf_utils.cpp 34054846f93f19ba550afe58e2a899d111ad38dc 
>   src/master/master.cpp 7417b5d641fd4bb6d91cb0e6456c60201bbc8206 
>   src/slave/slave.cpp 491419443a1de92c4a77049660e7d7c6c708cd52 
>   src/tests/persistent_volume_tests.cpp 
> acfeac16884b00581a3523607ff26f44f6dca53a 
>   src/tests/reservation_tests.cpp 470f7341686e69d0a71fb234a26b277c45c29780 
> 
> 
> Diff: https://reviews.apache.org/r/63751/diff/8/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 63751: Triggered 'ApplyOfferOperationMessage' for agent local resources.

[Jan Schlicht]

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

(Updated Nov. 22, 2017, 1:11 p.m.)


Review request for mesos, Benjamin Bannier and Jie Yu.


Changes
---

Addressed issues.


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


Repository: mesos


Description
---

Agents the have the 'RESOURCE_PROVIDER' capability set, should get sent
'ApplyOfferOperationMessage' instead of 'CheckpointResourcesMessage'.
The agent will then figure out how to apply the operation. For agent
local resources the agent will checkpoint resources.


Diffs (updated)
-

  src/common/protobuf_utils.hpp b2aa3654db2fe7d7d9d275ded81c6d54244654ee 
  src/common/protobuf_utils.cpp 34054846f93f19ba550afe58e2a899d111ad38dc 
  src/master/master.cpp 7417b5d641fd4bb6d91cb0e6456c60201bbc8206 
  src/slave/slave.cpp 491419443a1de92c4a77049660e7d7c6c708cd52 
  src/tests/persistent_volume_tests.cpp 
acfeac16884b00581a3523607ff26f44f6dca53a 
  src/tests/reservation_tests.cpp 470f7341686e69d0a71fb234a26b277c45c29780 


Diff: https://reviews.apache.org/r/63751/diff/8/

Changes: https://reviews.apache.org/r/63751/diff/7-8/


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 63628: Implemented index_sequence and related functionality.

[Michael Park]

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


Fix it, then Ship it!




Looks good!


3rdparty/stout/include/stout/utils.hpp
Lines 34 (patched)


`T... Ints` please



3rdparty/stout/include/stout/utils.hpp
Lines 44-52 (patched)


Let's put these in `internal` namespace, and
also refer to `std::size_t` consistently here.


- Michael Park


On Nov. 7, 2017, 8:51 a.m., Dmitry Zhuk wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63628/
> ---
> 
> (Updated Nov. 7, 2017, 8:51 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Michael Park.
> 
> 
> Bugs: MESOS-6972
> https://issues.apache.org/jira/browse/MESOS-6972
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This adds implementation of C++14 STL's `index_squence`
> and `make_index_sequence` templates.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/utils.hpp 
> 82cab830817842c23056235973ec4419ee9ec8b1 
> 
> 
> Diff: https://reviews.apache.org/r/63628/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Dmitry Zhuk
> 
>

Re: Review Request 62637: Added an object approver to authorize requests from resource providers.

[Alexander Rojas]

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



Please add unit tests.

- Alexander Rojas


On Nov. 21, 2017, 2:14 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62637/
> ---
> 
> (Updated Nov. 21, 2017, 2:14 a.m.)
> 
> 
> Review request for mesos, Alexander Rojas, Greg Mann, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-8100
> https://issues.apache.org/jira/browse/MESOS-8100
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds `LocalImplicitResourceProviderObjectApprover`, which
> authorize standalone container calls from a resource provider if the
> container IDs are prefixed with the namespace string.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.hpp 
> 40790f5801bdde8df0e2823fc8949d00b1eaa2fb 
>   src/authorizer/local/authorizer.cpp 
> 35bf03cfd4264e27235b355c1c7c3ff07f91af94 
>   src/slave/http.cpp 394e91013dc11e0a79e2e00534864281cc74ad2f 
> 
> 
> Diff: https://reviews.apache.org/r/62637/diff/3/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 62636: Generated authentication tokens for local resource providers.

[Chun-Hung Hsiao]

> On Nov. 22, 2017, 8:38 a.m., Alexander Rojas wrote:
> > src/resource_provider/daemon.cpp
> > Lines 253 (patched)
> > 
> >
> > This is a personal preference, but I don't see the reason to add more 
> > strings to a string which most likely already says it failed to validate a 
> > secret. You could just do `return error.get()`.

This is copied from `slave.cpp`.


- Chun-Hung


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


On Nov. 21, 2017, 12:10 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62636/
> ---
> 
> (Updated Nov. 21, 2017, 12:10 a.m.)
> 
> 
> Review request for mesos, Alexander Rojas, Greg Mann, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-8100
> https://issues.apache.org/jira/browse/MESOS-8100
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `LocalResourceProviderDaemon` now uses `Slave::secretGenerater` to
> generate an authentication token for each local resource provider. The
> authentication token can then be used to call the V1 agent API. In order
> to generate the tokens, `LocalResourceProviderDaemon::load()` is now an
> asynchronous function.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/daemon.hpp ef6c356cb6ddb2594d767d7dd6052e9fd8df8263 
>   src/resource_provider/daemon.cpp d584eb9d7aa75522aec97277674321061b90fbed 
>   src/resource_provider/local.hpp ebaa07d03ad77d516066ee2d4b60864be0611b5f 
>   src/resource_provider/local.cpp ad98f333c5668ca81de6e7ed3fc8f59323b151da 
>   src/resource_provider/storage/provider.hpp 
> 6de88c2329b358fcf48bc39ddda0132170991c3c 
>   src/resource_provider/storage/provider.cpp 
> 46224997430ac0c568904d80014166a6f059907f 
>   src/slave/slave.cpp d8edc5e6bbfa265bca4d19bbaa7db3063949dbc0 
> 
> 
> Diff: https://reviews.apache.org/r/62636/diff/8/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 62636: Generated authentication tokens for local resource providers.

[Chun-Hung Hsiao]

> On Nov. 22, 2017, 8:38 a.m., Alexander Rojas wrote:
> > src/resource_provider/daemon.cpp
> > Lines 78 (patched)
> > 
> >
> > This line is not needed. Mostly for consistence reason, you don't see 
> > this pattern of `nullptr` initializing anything, in this case default 
> > constructing a pointer sets it to `nullptr` but also not adding it to the 
> > initializer list will default construct it to `nullptr`

I thought that for POD fields, including pointers, the default initialization 
does nothing: http://en.cppreference.com/w/cpp/language/default_initialization. 
Am I mistaken?


> On Nov. 22, 2017, 8:38 a.m., Alexander Rojas wrote:
> > src/resource_provider/daemon.cpp
> > Lines 112 (patched)
> > 
> >
> > From the code here is not clear who owns the instance of 
> > `secretGenerator`. The fact that it is a private attribute tells me it is 
> > owned by this process, but it doesn't delete it on destruction. 
> > 
> > If you go for it, there should be a comment on why is it ok not to 
> > destroy this instance and why is not a memory leak, but really what you 
> > want is to use a managed pointer even if it is a const reference to an 
> > existing one.
> > 
> > check `process::Owned`. I'm not so sure if we can now use 
> > `std::unique_ptr` or `std::shared_ptr`.

This one is passed from the `Slave::initialize()`, and is shared with the 
slave. The slave is the one responsible to manage its lifecycle. We probably 
need some refactor on this.


> On Nov. 22, 2017, 8:38 a.m., Alexander Rojas wrote:
> > src/resource_provider/storage/provider.cpp
> > Lines 104 (patched)
> > 
> >
> > is there any situatio when the `authToken` is `None`?

Yes. When the authentication is not turned on.


- Chun-Hung


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


On Nov. 21, 2017, 12:10 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62636/
> ---
> 
> (Updated Nov. 21, 2017, 12:10 a.m.)
> 
> 
> Review request for mesos, Alexander Rojas, Greg Mann, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-8100
> https://issues.apache.org/jira/browse/MESOS-8100
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `LocalResourceProviderDaemon` now uses `Slave::secretGenerater` to
> generate an authentication token for each local resource provider. The
> authentication token can then be used to call the V1 agent API. In order
> to generate the tokens, `LocalResourceProviderDaemon::load()` is now an
> asynchronous function.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/daemon.hpp ef6c356cb6ddb2594d767d7dd6052e9fd8df8263 
>   src/resource_provider/daemon.cpp d584eb9d7aa75522aec97277674321061b90fbed 
>   src/resource_provider/local.hpp ebaa07d03ad77d516066ee2d4b60864be0611b5f 
>   src/resource_provider/local.cpp ad98f333c5668ca81de6e7ed3fc8f59323b151da 
>   src/resource_provider/storage/provider.hpp 
> 6de88c2329b358fcf48bc39ddda0132170991c3c 
>   src/resource_provider/storage/provider.cpp 
> 46224997430ac0c568904d80014166a6f059907f 
>   src/slave/slave.cpp d8edc5e6bbfa265bca4d19bbaa7db3063949dbc0 
> 
> 
> Diff: https://reviews.apache.org/r/62636/diff/8/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 62636: Generated authentication tokens for local resource providers.

[Chun-Hung Hsiao]

> On Nov. 22, 2017, 8:40 a.m., Alexander Rojas wrote:
> > There are no tests anywhere in the chain. I disapprove of code written 
> > without unit tests. Please address that in the whole chain.

Will do. Need more time to add more tests :(


- Chun-Hung


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


On Nov. 21, 2017, 12:10 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62636/
> ---
> 
> (Updated Nov. 21, 2017, 12:10 a.m.)
> 
> 
> Review request for mesos, Alexander Rojas, Greg Mann, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-8100
> https://issues.apache.org/jira/browse/MESOS-8100
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `LocalResourceProviderDaemon` now uses `Slave::secretGenerater` to
> generate an authentication token for each local resource provider. The
> authentication token can then be used to call the V1 agent API. In order
> to generate the tokens, `LocalResourceProviderDaemon::load()` is now an
> asynchronous function.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/daemon.hpp ef6c356cb6ddb2594d767d7dd6052e9fd8df8263 
>   src/resource_provider/daemon.cpp d584eb9d7aa75522aec97277674321061b90fbed 
>   src/resource_provider/local.hpp ebaa07d03ad77d516066ee2d4b60864be0611b5f 
>   src/resource_provider/local.cpp ad98f333c5668ca81de6e7ed3fc8f59323b151da 
>   src/resource_provider/storage/provider.hpp 
> 6de88c2329b358fcf48bc39ddda0132170991c3c 
>   src/resource_provider/storage/provider.cpp 
> 46224997430ac0c568904d80014166a6f059907f 
>   src/slave/slave.cpp d8edc5e6bbfa265bca4d19bbaa7db3063949dbc0 
> 
> 
> Diff: https://reviews.apache.org/r/62636/diff/8/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 63021: Added `getService()` function to launch CSI plugins.

[Chun-Hung Hsiao]

> On Oct. 20, 2017, 12:26 a.m., Jie Yu wrote:
> > src/resource_provider/storage/provider.cpp
> > Lines 236 (patched)
> > 
> >
> > YOu should follow `src/launcher/default_executor.cpp` to see how to do 
> > reliable registration (`doReliableRegistration`).
> > 
> > So basically, you need to keep a state in this process, and set it 
> > SUBSCRIBED once you received Subscribed message. You need to retry to make 
> > it reliable.

Fixed in r63823


- Chun-Hung


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


On Nov. 22, 2017, 8:47 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63021/
> ---
> 
> (Updated Nov. 22, 2017, 8:47 a.m.)
> 
> 
> Review request for mesos, James DeFelice, Jie Yu, Joseph Wu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-8032
> https://issues.apache.org/jira/browse/MESOS-8032
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `getService()` method first checks if there is already a container
> daemon for the specified plugin component, and creates a new one if not.
> The post-start hook for the container daemon will call `connect()` to
> wait for the endpoint socket file to appear and connect to it, then
> set up the corresponding promise of CSI client. The post-stop hook will
> remove the socket file and create a new promise for the next call to the
> post-start hook to set it up.
> 
> 
> Diffs
> -
> 
>   include/mesos/type_utils.hpp f7f23270fadc75a8737f32ec1c3fbc01f6321248 
>   src/common/type_utils.cpp 14267559f228f23aff2e21b987795b9f4fee93fc 
>   src/resource_provider/storage/provider.cpp 
> 46224997430ac0c568904d80014166a6f059907f 
> 
> 
> Diff: https://reviews.apache.org/r/63021/diff/10/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 63021: Added `getService()` function to launch CSI plugins.

[Chun-Hung Hsiao]

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

(Updated Nov. 22, 2017, 8:47 a.m.)


Review request for mesos, James DeFelice, Jie Yu, Joseph Wu, and Jan Schlicht.


Changes
---

Addressed comments.


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


Repository: mesos


Description
---

The `getService()` method first checks if there is already a container
daemon for the specified plugin component, and creates a new one if not.
The post-start hook for the container daemon will call `connect()` to
wait for the endpoint socket file to appear and connect to it, then
set up the corresponding promise of CSI client. The post-stop hook will
remove the socket file and create a new promise for the next call to the
post-start hook to set it up.


Diffs (updated)
-

  include/mesos/type_utils.hpp f7f23270fadc75a8737f32ec1c3fbc01f6321248 
  src/common/type_utils.cpp 14267559f228f23aff2e21b987795b9f4fee93fc 
  src/resource_provider/storage/provider.cpp 
46224997430ac0c568904d80014166a6f059907f 


Diff: https://reviews.apache.org/r/63021/diff/10/

Changes: https://reviews.apache.org/r/63021/diff/9-10/


Testing
---


Thanks,

Chun-Hung Hsiao

Re: Review Request 63823: Initialization and registration for storage local resource provider.

[Chun-Hung Hsiao]

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

(Updated Nov. 22, 2017, 8:47 a.m.)


Review request for mesos, Jie Yu, Joseph Wu, and Jan Schlicht.


Changes
---

Added reliable registration.


Summary (updated)
-

Initialization and registration for storage local resource provider.


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


Repository: mesos


Description (updated)
---

This patch validates `ResourceProviderInfo` for storage local resource
providers upon creation. During initialization, the storage local
resource provider first tries to recover its ID of the last session
through reading the actual path linked by
`/meta/resource_providers///latest`,
then do a reliable registration to the agent's resource provider
manager.

This patch is split from https://reviews.apache.org/r/63021/.


Diffs (updated)
-

  src/resource_provider/manager.cpp 6dfc42900a1e4249f37cec585f7fe50f5aa94e43 
  src/resource_provider/storage/provider.cpp 
46224997430ac0c568904d80014166a6f059907f 


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

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


Testing
---

make


Thanks,

Chun-Hung Hsiao

Re: Review Request 62636: Generated authentication tokens for local resource providers.

[Alexander Rojas]

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



There are no tests anywhere in the chain. I disapprove of code written without 
unit tests. Please address that in the whole chain.

- Alexander Rojas


On Nov. 21, 2017, 1:10 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62636/
> ---
> 
> (Updated Nov. 21, 2017, 1:10 a.m.)
> 
> 
> Review request for mesos, Alexander Rojas, Greg Mann, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-8100
> https://issues.apache.org/jira/browse/MESOS-8100
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `LocalResourceProviderDaemon` now uses `Slave::secretGenerater` to
> generate an authentication token for each local resource provider. The
> authentication token can then be used to call the V1 agent API. In order
> to generate the tokens, `LocalResourceProviderDaemon::load()` is now an
> asynchronous function.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/daemon.hpp ef6c356cb6ddb2594d767d7dd6052e9fd8df8263 
>   src/resource_provider/daemon.cpp d584eb9d7aa75522aec97277674321061b90fbed 
>   src/resource_provider/local.hpp ebaa07d03ad77d516066ee2d4b60864be0611b5f 
>   src/resource_provider/local.cpp ad98f333c5668ca81de6e7ed3fc8f59323b151da 
>   src/resource_provider/storage/provider.hpp 
> 6de88c2329b358fcf48bc39ddda0132170991c3c 
>   src/resource_provider/storage/provider.cpp 
> 46224997430ac0c568904d80014166a6f059907f 
>   src/slave/slave.cpp d8edc5e6bbfa265bca4d19bbaa7db3063949dbc0 
> 
> 
> Diff: https://reviews.apache.org/r/62636/diff/8/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 62636: Generated authentication tokens for local resource providers.

[Alexander Rojas]

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




src/resource_provider/daemon.cpp
Lines 78 (patched)


This line is not needed. Mostly for consistence reason, you don't see this 
pattern of `nullptr` initializing anything, in this case default constructing a 
pointer sets it to `nullptr` but also not adding it to the initializer list 
will default construct it to `nullptr`



src/resource_provider/daemon.cpp
Lines 112 (patched)


From the code here is not clear who owns the instance of `secretGenerator`. 
The fact that it is a private attribute tells me it is owned by this process, 
but it doesn't delete it on destruction. 

If you go for it, there should be a comment on why is it ok not to destroy 
this instance and why is not a memory leak, but really what you want is to use 
a managed pointer even if it is a const reference to an existing one.

check `process::Owned`. I'm not so sure if we can now use `std::unique_ptr` 
or `std::shared_ptr`.



src/resource_provider/daemon.cpp
Lines 237 (patched)


We don't make use of the booleaness of pointers. Replace with 
`secretGenerator == nullptr`.

Also Tthe code is more readable if you do:

```c++
  if (secretGenerator == nullptr) {
return None();
  }
  
  ...
```

Since you don't have to go to the end of the code to see what happens in 
the `else` case (which is only one line), and you save yourself one 
indentation, which with 80 characters is a great compromise.



src/resource_provider/daemon.cpp
Lines 253 (patched)


This is a personal preference, but I don't see the reason to add more 
strings to a string which most likely already says it failed to validate a 
secret. You could just do `return error.get()`.



src/resource_provider/storage/provider.cpp
Lines 104 (patched)


is there any situatio when the `authToken` is `None`?


- Alexander Rojas


On Nov. 21, 2017, 1:10 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62636/
> ---
> 
> (Updated Nov. 21, 2017, 1:10 a.m.)
> 
> 
> Review request for mesos, Alexander Rojas, Greg Mann, Jie Yu, and Joseph Wu.
> 
> 
> Bugs: MESOS-8100
> https://issues.apache.org/jira/browse/MESOS-8100
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `LocalResourceProviderDaemon` now uses `Slave::secretGenerater` to
> generate an authentication token for each local resource provider. The
> authentication token can then be used to call the V1 agent API. In order
> to generate the tokens, `LocalResourceProviderDaemon::load()` is now an
> asynchronous function.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/daemon.hpp ef6c356cb6ddb2594d767d7dd6052e9fd8df8263 
>   src/resource_provider/daemon.cpp d584eb9d7aa75522aec97277674321061b90fbed 
>   src/resource_provider/local.hpp ebaa07d03ad77d516066ee2d4b60864be0611b5f 
>   src/resource_provider/local.cpp ad98f333c5668ca81de6e7ed3fc8f59323b151da 
>   src/resource_provider/storage/provider.hpp 
> 6de88c2329b358fcf48bc39ddda0132170991c3c 
>   src/resource_provider/storage/provider.cpp 
> 46224997430ac0c568904d80014166a6f059907f 
>   src/slave/slave.cpp d8edc5e6bbfa265bca4d19bbaa7db3063949dbc0 
> 
> 
> Diff: https://reviews.apache.org/r/62636/diff/8/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>