Re: Review Request 69405: Refactored createAuthorizationCallbacks into common/authorization.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 69405 was successfully built and tested.

Reviews applied: `['69368', '69369', '69384', '69385', '69386', '69405']`

All the build artifacts available at: 
http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2626/mesos-review-69405

- Mesos Reviewbot Windows


On Nov. 20, 2018, 2:35 a.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69405/
> ---
> 
> (Updated Nov. 20, 2018, 2:35 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Alexander Rojas, and Greg Mann.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Moves 'createAuthorizationCallbacks' out of common/http into
> common/authorization.
> 
> 
> Diffs
> -
> 
>   src/authorizer/local/authorizer.cpp 
> 83944b9a7322707609c2d4e13b84ab3a44cf4e25 
>   src/common/authorization.hpp PRE-CREATION 
>   src/common/authorization.cpp PRE-CREATION 
>   src/common/http.hpp 6ca54a641d1ea1625c70518f8870516664741e9a 
>   src/common/http.cpp d9703f80880850b8a1ec9ddd2b1e8f125f36 
>   src/master/main.cpp 2c7b1bb492a0655dec9280e98ff942a15e2ae8f0 
>   src/slave/main.cpp e774092ff2c3941f17cdebfb26d80c05a26497c6 
>   src/tests/cluster.cpp 2b351ca70d8e80008e49722aa7d46918b5ecd9b0 
>   src/tests/logging_tests.cpp 30bcdc7f4aa9d6a39c5ef6e825357815bf4f9f19 
> 
> 
> Diff: https://reviews.apache.org/r/69405/diff/1/
> 
> 
> Testing
> ---
> 
> `make check` and internal CI
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 69163: Set agent and/or resource provider ID in operation status updates.

[Chun-Hung Hsiao]

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




src/master/master.cpp
Lines 8192-8201 (patched)


We can get rid of this snippet and simply use `providerId`. Or, validate 
that `resourceProviderId` is always the same as `providerId`.

We should probably validate that both resources and operations have their 
resource provider id set properly in the resource provider manager, and issue 
an `ERROR` event back to the resource provider, instead of crashing the agent: 
https://github.com/apache/mesos/blob/master/src/resource_provider/manager.cpp#L907
I created https://issues.apache.org/jira/browse/MESOS-9407 to capture this. 
No need to address it in this patch.



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


How about inlining this ternary operation into the use below so we can get 
rid of `resourceProviderId_`?



src/resource_provider/manager.cpp
Lines 887-888 (patched)


It seems not cohesive that we include both the agent ID and the resource 
provider ID when generating the `UpdateOperationStatusMessage` in SLRP, but 
drop them because `Call::UpdateOperationStatus` does not have corresponding 
fields, then:
1. Recover the resource provider ID in the RP manager, and
2. Recover the agent ID in `Slave::handleResourceProviderMessage`.

I was wondering if it is a good idea to add `slave_id` and 
`resource_provider_id` in `OperationStatus` instead, then generate 
`OperationStatusUpdateMessage` based on `OperationStatus` for backward 
compatibility.



src/resource_provider/storage/provider.cpp
Line 3079 (original), 3080-3081 (patched)


We don't need to set up the slave ID and resource provider ID since dropped 
operations will not be bookkept and retried.



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


How about inlining the ternary operation into its use below so we don't 
need this `resourceProviderId_`?



src/tests/master_tests.cpp
Lines 9036 (patched)


We might want to backport this patch back to 1.7.x. Can you split the tests 
into another patch so we can minimize the backport?



src/tests/master_tests.cpp
Lines 9038 (patched)


Since the resource provider manager is the one adding the resource provider 
ID, does it make sense to move this test to 
`resource_provider_manager_tests.cpp`?

If we make the decision to add `slave_id` and `resource_provider_id` in 
`OperationStatus` and make the SLRP generates them, we should move this test to 
`storage_local_resource_provider_tests.cpp`.



src/tests/master_tests.cpp
Lines 9066-9070 (patched)


Nit: they fit into a line. ;)


- Chun-Hung Hsiao


On Nov. 12, 2018, 8:49 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69163/
> ---
> 
> (Updated Nov. 12, 2018, 8:49 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gastón Kleiman, and James DeFelice.
> 
> 
> Bugs: MESOS-9293
> https://issues.apache.org/jira/browse/MESOS-9293
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch sets agent and/or resource provider ID operation status
> update messages. This is not always possible, e.g., some operations
> might fail validation so that no corresponding IDs can be extracted.
> 
> Since operations failing validation are currently directly rejected by
> the master without going through a status update manager, they are not
> retried either. If a master status update manager for operations is
> introduced at a later point it should be possible to forward
> acknowledgements for updates to the master's update manager (no agent
> ID, not resource provider ID).
> 
> 
> Diffs
> -
> 
>   src/common/protobuf_utils.hpp 1662125ed3e47b179ee32d08c1d3af75553066ba 
>   src/common/protobuf_utils.cpp a45607eed4c4bae5010bcc3f3ffeabd6d911062a 
>   src/master/master.cpp 1e326ec42a7f79a0835529a4655e7ec272f1cf40 
>   src/resource_provider/manager.cpp 6c81c430e9e1205d71982a7fa2bcd9aa15fc01b2 
>   src/resource_provider/storage/provider.cpp 
> c137fa4f13edc58d93c03a9dd32fdf9d38b38316 
>   src/slave/slave.cpp 74f6fb9036a9ac4f587f53ec2df04eeb4c167bfb 
>   src/tests/master_tests.cpp ac6bf379c5906cf9612284911c121c9457f648a0 
> 
> 
> Diff: https://reviews.apache.org/r/69163/diff/3/
> 
> 
> Testing
> ---
> 
> `make check`

Review Request 69405: Refactored createAuthorizationCallbacks into common/authorization.

[Till Toenshoff via Review Board]

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

Review request for mesos, Alexander Rukletsov, Alexander Rojas, and Greg Mann.


Repository: mesos


Description
---

Moves 'createAuthorizationCallbacks' out of common/http into
common/authorization.


Diffs
-

  src/authorizer/local/authorizer.cpp 83944b9a7322707609c2d4e13b84ab3a44cf4e25 
  src/common/authorization.hpp PRE-CREATION 
  src/common/authorization.cpp PRE-CREATION 
  src/common/http.hpp 6ca54a641d1ea1625c70518f8870516664741e9a 
  src/common/http.cpp d9703f80880850b8a1ec9ddd2b1e8f125f36 
  src/master/main.cpp 2c7b1bb492a0655dec9280e98ff942a15e2ae8f0 
  src/slave/main.cpp e774092ff2c3941f17cdebfb26d80c05a26497c6 
  src/tests/cluster.cpp 2b351ca70d8e80008e49722aa7d46918b5ecd9b0 
  src/tests/logging_tests.cpp 30bcdc7f4aa9d6a39c5ef6e825357815bf4f9f19 


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


Testing
---

`make check` and internal CI


Thanks,

Till Toenshoff

Re: Review Request 69384: Introduced common/authorization and refactored collectAuthorizations.

[Till Toenshoff via Review Board]

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

(Updated Nov. 20, 2018, 2:26 a.m.)


Review request for mesos, Alexander Rukletsov, Alexander Rojas, and Benjamin 
Bannier.


Changes
---

Addressed comment.


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


Repository: mesos


Description
---

Adds a new collection of authorization specific helper/s to reduce code
duplication and increase efficient test coverage.

Moves the newly introduced 'collectAuthorizations' helper into this new
authorization source unit.


Diffs (updated)
-

  src/CMakeLists.txt 732368293049b7d9d6f62057344d433637ad44e8 
  src/Makefile.am 2d9c81b149a5764dc82593bef102f5568847daa2 
  src/common/authorization.hpp PRE-CREATION 
  src/common/authorization.cpp PRE-CREATION 
  src/master/master.hpp e77babf22126838c63cd05e483875c9beb3ac5ff 
  src/master/master.cpp 9458ff10999d48e40a8596ec4edf1243591bc0d4 
  src/master/weights_handler.cpp 222ec754e216da195250d1895a728294a076ee5d 
  src/tests/master_tests.cpp ac6bf379c5906cf9612284911c121c9457f648a0 


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

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


Testing
---

make check and private ci


Thanks,

Till Toenshoff

Re: Review Request 69162: Added agent and resource provider IDs to operation status messages.

[Chun-Hung Hsiao]

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




include/mesos/scheduler/scheduler.proto
Line 140 (original), 140 (patched)


It seems more consistent with `TaskStatus` if we put `slave_id` and 
`resource_provider_id` in `OperationStatus`. What's the pros and cons of doing 
that instead?



include/mesos/scheduler/scheduler.proto
Lines 146 (patched)


s/ a / an /



include/mesos/scheduler/scheduler.proto
Lines 149 (patched)


Should we be explicit about what cases they are? If it's not possible, 
maybe say the following instead?
```
// In certain cases, e.g., invalid operations, neither `slave_id` nor
// `resource_provider_id` will be set, and the scheduler does not need to
// acknowledge this status update.
```



include/mesos/v1/scheduler/scheduler.proto
Lines 144 (patched)


s/ a / an /



include/mesos/v1/scheduler/scheduler.proto
Lines 147 (patched)


Ditto.



src/messages/messages.cpp
Lines 82-84 (patched)


Nit: How about following what clang-format gives us here?
```
if (left.has_resource_provider_id() &&
left.resource_provider_id() != right.resource_provider_id()) {
```
The conditions and the body can be easily distinguished by different 
indentations above.



src/messages/messages.cpp
Lines 146-149 (original), 156-162 (patched)


How about the following:
```
  if (update.has_resource_provider_id()) {
stream << " from resource provider " << update.resource_provider_id();
  }

  if (update.has_slave_id()) {
stream << " on agent " << update.slave_id();
  }
```


- Chun-Hung Hsiao


On Nov. 12, 2018, 8:49 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69162/
> ---
> 
> (Updated Nov. 12, 2018, 8:49 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gastón Kleiman, and James DeFelice.
> 
> 
> Bugs: MESOS-9293
> https://issues.apache.org/jira/browse/MESOS-9293
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch add agent and resource provide IDs to
> `UpdateOperationStatus` and `UpdateOperationStatusMessage`. With that
> frameworks are able to reconcile enough information after failover to
> construct operation acknowledgements.
> 
> We will add code to populate these fields in a follow-up patch.
> 
> 
> Diffs
> -
> 
>   include/mesos/scheduler/scheduler.proto 
> f6a780a7b75878b9e74402a28a25bb868f7ac36f 
>   include/mesos/v1/scheduler/scheduler.proto 
> fcfec5e417463103e98dd6917722b4fde41cac7c 
>   src/internal/evolve.cpp aa60efed5a28ed4d847a4a27aa8e994cda82175d 
>   src/messages/messages.cpp dd8f60ecdbc06d10be1152bee1ddb65feaaf8fbb 
>   src/messages/messages.proto 41e6a8a2eab0ae7c2878c1d3286c5dea0eb68ed7 
> 
> 
> Diff: https://reviews.apache.org/r/69162/diff/3/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 69403: Removed empty filters in SLRP tests.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 69403 was successfully built and tested.

Reviews applied: `['69036', '69357', '69359', '69360', '69361', '69356', 
'69363', '69362', '69400', '69402', '69364', '69365', '69366', '69403']`

All the build artifacts available at: 
http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2625/mesos-review-69403

- Mesos Reviewbot Windows


On Nov. 19, 2018, 11:37 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69403/
> ---
> 
> (Updated Nov. 19, 2018, 11:37 p.m.)
> 
> 
> Review request for mesos and Benjamin Bannier.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed empty filters in SLRP tests.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 077a46585bd56181ba199dc529e09f38f4971338 
> 
> 
> Diff: https://reviews.apache.org/r/69403/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 69157: Fixed handling for offer operation updates.

[Chun-Hung Hsiao]

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




src/slave/slave.cpp
Line 8017 (original), 8017 (patched)


If the `latest_status` is not in sync with `status`, then this would lead 
to an inconcistency between the resource bookkeeping of the master and the 
agent:
https://github.com/apache/mesos/blob/master/src/master/master.cpp#L11212

Since the master recovers and re-offers the converted resources based on 
the latest status, the framework might get the converted resources and use them 
before it acks any intermediate nonterminal status, which would lead to a 
failure at the agent side since the agent's bookkeeping is not based on the 
latest status.

Instead we should do the following since `latest_status` has been updated 
in Line 7964--7976:
```
switch (operation->latest_status().state()) {
```


- Chun-Hung Hsiao


On Nov. 13, 2018, 9:10 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69157/
> ---
> 
> (Updated Nov. 13, 2018, 9:10 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.
> 
> 
> Bugs: MESOS-8403
> https://issues.apache.org/jira/browse/MESOS-8403
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The handling of offer operation updates introduced in `c946615ec6d`
> made use of an update's `latest_status` without making sure that any
> value was set. This could lead to situation where an uninitialized
> enum value was switched on which would have caused a fatal error at
> runtime.
> 
> This patch replaces uses of `latest_status` with `state` which does
> contain the information we care about. We also adjust the error
> logging so we log the value that lead to the error, not some other
> value.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp 74f6fb9036a9ac4f587f53ec2df04eeb4c167bfb 
> 
> 
> Diff: https://reviews.apache.org/r/69157/diff/3/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 69366: Used `OperationID` instead of `string` in test helpers.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 69366 was successfully built and tested.

Reviews applied: `['69036', '69357', '69359', '69360', '69361', '69356', 
'69363', '69362', '69364', '69365', '69366']`

All the build artifacts available at: 
http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2624/mesos-review-69366

- Mesos Reviewbot Windows


On Nov. 19, 2018, 9:51 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69366/
> ---
> 
> (Updated Nov. 19, 2018, 9:51 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Gastón Kleiman, and Greg Mann.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change makes the helper more consistent with other helpers, and is
> more future-proof to changes in `OperationID`.
> 
> 
> Diffs
> -
> 
>   src/tests/master_slave_reconciliation_tests.cpp 
> 064b4d47598b1d0e18d7bef14ac62d7e5f2c1102 
>   src/tests/master_tests.cpp ac6bf379c5906cf9612284911c121c9457f648a0 
>   src/tests/mesos.hpp 576f4bde88c069ee2fa0dd33912a034437338e7e 
>   src/tests/operation_reconciliation_tests.cpp 
> 37d38b3df8c162bd1baa5ce557e54baa5c23a006 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 077a46585bd56181ba199dc529e09f38f4971338 
> 
> 
> Diff: https://reviews.apache.org/r/69366/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Review Request 69403: Removed empty filters in SLRP tests.

[Chun-Hung Hsiao]

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

Review request for mesos and Benjamin Bannier.


Repository: mesos


Description
---

Removed empty filters in SLRP tests.


Diffs
-

  src/tests/storage_local_resource_provider_tests.cpp 
077a46585bd56181ba199dc529e09f38f4971338 


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


Testing
---

make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 69364: Added the `--create_parameters` flag to the test CSI plugin.

[Chun-Hung Hsiao]

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

(Updated Nov. 19, 2018, 11:35 p.m.)


Review request for mesos, Benjamin Bannier and Jie Yu.


Changes
---

Refactored the patch into r/69400, r/69402 and this one, as suggested by 
Benjamin.


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


Repository: mesos


Description (updated)
---

When the flag is specified, `CreateVolume` and `GetCapacity` work only
if the `parameters` argument matches this flag. This will be used to
test the checkpointing of create parameters of CSI volumes in SLRP.


Diffs (updated)
-

  src/examples/test_csi_plugin.cpp 66f4ee0e2381e94bf9110d1e6e9f79eac939f683 


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

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


Testing
---

make check


Thanks,

Chun-Hung Hsiao

Review Request 69402: Fixed `CreateVolume` of the test CSI plugin.

[Chun-Hung Hsiao]

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

Review request for mesos, Benjamin Bannier and Jie Yu.


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


Repository: mesos


Description
---

This patch makes sure that `CreateVolume` is idempotent, and check if
the specified volume capability is supported.


Diffs
-

  src/examples/test_csi_plugin.cpp 66f4ee0e2381e94bf9110d1e6e9f79eac939f683 


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


Testing
---

make check


Thanks,

Chun-Hung Hsiao

Review Request 69400: Refactored the test CSI plugin.

[Chun-Hung Hsiao]

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

Review request for mesos, Benjamin Bannier and Jie Yu.


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


Repository: mesos


Description
---

This patch does not introduce any functional change to the test CSI
plugin. It simply refactored the check for the default mount volume
capability and the parsing of `--volumes` flag.


Diffs
-

  src/examples/test_csi_plugin.cpp 66f4ee0e2381e94bf9110d1e6e9f79eac939f683 


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


Testing
---

make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 69362: Checkpointed creation parameters for CSI volumes.

[Chun-Hung Hsiao]

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

(Updated Nov. 19, 2018, 11:30 p.m.)


Review request for mesos, Benjamin Bannier and Jie Yu.


Changes
---

Addressed Benjamin's comments.


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


Repository: mesos


Description
---

The parameters of CSI volumes created by SLRPs are now checkpointed, and
used to validate volumes created from previous SLRP runs.


Diffs (updated)
-

  src/csi/state.proto 8445399ac17cfef740c2523172b34fd5439f2d3a 
  src/resource_provider/storage/provider.cpp 
c137fa4f13edc58d93c03a9dd32fdf9d38b38316 


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

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


Testing
---

make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 69356: Added valiadtion for `Offer.Operation.CreateDisk.target_profile`.

[Chun-Hung Hsiao]

> On Nov. 16, 2018, 12:01 p.m., Benjamin Bannier wrote:
> > src/master/validation.cpp
> > Lines 2531-2535 (patched)
> > 
> >
> > I wonder whether performing this validation in the master is the right 
> > thing to do.
> > 
> > Profiles are currently applied by SLRP in the agent, so doing 
> > SLRP-specific validation here not only scatters logic in different places, 
> > but could also make making breaking changes in the future harder than 
> > necessary (agent and master are upgraded independently).
> > 
> > Should we move this into the agent, e.g., SLRP?
> 
> Chun-Hung Hsiao wrote:
> It seems to me that if this is the semantics in the API, then it seems to 
> me that we should validate this as early as possible to fail-fast. WDYT?
> 
> Benjamin Bannier wrote:
> I agree with failing as early as possible. This should not happen at the 
> cost of making master even more of an God object than it already is, though. 
> Does that make sense?
> 
> James DeFelice wrote:
> AFAICT there's already a bunch of validation in the master, specifically 
> for this API. If we want to move this validation out of the master and into 
> SLRP, shouldn't that be tackled separately? It sounds like a bigger 
> (philosophical) change than simply adding an additional validation rule to a 
> set of rules that already exist on the master.
> 
> Benjamin Bannier wrote:
> Chun, could you create a follow-up ticket to possible move validation 
> beyond resource and proto validity to SLRP?
> 
> Dropping for now.

https://issues.apache.org/jira/browse/MESOS-9403


- Chun-Hung


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


On Nov. 19, 2018, 9:24 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69356/
> ---
> 
> (Updated Nov. 19, 2018, 9:24 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, James DeFelice, and Jie Yu.
> 
> 
> Bugs: MESOS-9275
> https://issues.apache.org/jira/browse/MESOS-9275
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added validation for `Offer.Operation.CreateDisk.target_profile`.
> 
> 
> Diffs
> -
> 
>   src/master/validation.cpp 5768ac8fe802f28855fbd7be135c75532771 
>   src/tests/master_validation_tests.cpp 
> aa7c8f70c09459be32c6c415497e95fcdc218efd 
> 
> 
> Diff: https://reviews.apache.org/r/69356/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 69356: Added valiadtion for `Offer.Operation.CreateDisk.target_profile`.

[Benjamin Bannier]

> On Nov. 16, 2018, 1:01 p.m., Benjamin Bannier wrote:
> > src/master/validation.cpp
> > Lines 2531-2535 (patched)
> > 
> >
> > I wonder whether performing this validation in the master is the right 
> > thing to do.
> > 
> > Profiles are currently applied by SLRP in the agent, so doing 
> > SLRP-specific validation here not only scatters logic in different places, 
> > but could also make making breaking changes in the future harder than 
> > necessary (agent and master are upgraded independently).
> > 
> > Should we move this into the agent, e.g., SLRP?
> 
> Chun-Hung Hsiao wrote:
> It seems to me that if this is the semantics in the API, then it seems to 
> me that we should validate this as early as possible to fail-fast. WDYT?
> 
> Benjamin Bannier wrote:
> I agree with failing as early as possible. This should not happen at the 
> cost of making master even more of an God object than it already is, though. 
> Does that make sense?
> 
> James DeFelice wrote:
> AFAICT there's already a bunch of validation in the master, specifically 
> for this API. If we want to move this validation out of the master and into 
> SLRP, shouldn't that be tackled separately? It sounds like a bigger 
> (philosophical) change than simply adding an additional validation rule to a 
> set of rules that already exist on the master.

Chun, could you create a follow-up ticket to possible move validation beyond 
resource and proto validity to SLRP?

Dropping for now.


- Benjamin


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


On Nov. 19, 2018, 10:24 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69356/
> ---
> 
> (Updated Nov. 19, 2018, 10:24 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, James DeFelice, and Jie Yu.
> 
> 
> Bugs: MESOS-9275
> https://issues.apache.org/jira/browse/MESOS-9275
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added validation for `Offer.Operation.CreateDisk.target_profile`.
> 
> 
> Diffs
> -
> 
>   src/master/validation.cpp 5768ac8fe802f28855fbd7be135c75532771 
>   src/tests/master_validation_tests.cpp 
> aa7c8f70c09459be32c6c415497e95fcdc218efd 
> 
> 
> Diff: https://reviews.apache.org/r/69356/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 69377: Added blog post for Mesos Mini.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 69377 was successfully built and tested.

All the build artifacts available at: 
http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2623/mesos-review-69377

- Mesos Reviewbot Windows


On Nov. 19, 2018, 8:28 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69377/
> ---
> 
> (Updated Nov. 19, 2018, 8:28 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Chun-Hung Hsiao, Greg Mann, Jason 
> Lai, James DeFelice, James Peach, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added blog post for Mesos Mini.
> 
> 
> Diffs
> -
> 
>   site/source/blog/2018-11-19-mesos-mini.md PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/69377/diff/4/
> 
> 
> Testing
> ---
> 
> Markdown rendering can be viewed here:
> https://github.com/jieyu/mesos/blob/mesos_mini_blog/site/source/blog/2018-11-19-mesos-mini.md
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 69356: Added valiadtion for `Offer.Operation.CreateDisk.target_profile`.

[James DeFelice]

> On Nov. 16, 2018, 12:01 p.m., Benjamin Bannier wrote:
> > src/master/validation.cpp
> > Lines 2531-2535 (patched)
> > 
> >
> > I wonder whether performing this validation in the master is the right 
> > thing to do.
> > 
> > Profiles are currently applied by SLRP in the agent, so doing 
> > SLRP-specific validation here not only scatters logic in different places, 
> > but could also make making breaking changes in the future harder than 
> > necessary (agent and master are upgraded independently).
> > 
> > Should we move this into the agent, e.g., SLRP?
> 
> Chun-Hung Hsiao wrote:
> It seems to me that if this is the semantics in the API, then it seems to 
> me that we should validate this as early as possible to fail-fast. WDYT?
> 
> Benjamin Bannier wrote:
> I agree with failing as early as possible. This should not happen at the 
> cost of making master even more of an God object than it already is, though. 
> Does that make sense?

AFAICT there's already a bunch of validation in the master, specifically for 
this API. If we want to move this validation out of the master and into SLRP, 
shouldn't that be tackled separately? It sounds like a bigger (philosophical) 
change than simply adding an additional validation rule to a set of rules that 
already exist on the master.


- James


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


On Nov. 19, 2018, 9:24 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69356/
> ---
> 
> (Updated Nov. 19, 2018, 9:24 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, James DeFelice, and Jie Yu.
> 
> 
> Bugs: MESOS-9275
> https://issues.apache.org/jira/browse/MESOS-9275
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added validation for `Offer.Operation.CreateDisk.target_profile`.
> 
> 
> Diffs
> -
> 
>   src/master/validation.cpp 5768ac8fe802f28855fbd7be135c75532771 
>   src/tests/master_validation_tests.cpp 
> aa7c8f70c09459be32c6c415497e95fcdc218efd 
> 
> 
> Diff: https://reviews.apache.org/r/69356/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 69364: Added the `--create_parameters` flag to the test CSI plugin.

[Benjamin Bannier]

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



This patch seems to contain at the same time 1) addition of the new flag, 2) 
new capability handling, and 3) fix for the bug mentioned in the commit 
message. Could you break it apart?

- Benjamin Bannier


On Nov. 16, 2018, 1:48 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69364/
> ---
> 
> (Updated Nov. 16, 2018, 1:48 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Bugs: MESOS-9275
> https://issues.apache.org/jira/browse/MESOS-9275
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When the flag is specified, `CreateVolume` and `GetCapacity` work only
> if the `parameters` argument matches this flag. This will be used to
> test the checkpointing of create parameters of CSI volumes in SLRP.
> 
> The patch also fixes the bug that the `CreateVolume` call is not
> idempotent.
> 
> 
> Diffs
> -
> 
>   src/examples/test_csi_plugin.cpp 66f4ee0e2381e94bf9110d1e6e9f79eac939f683 
> 
> 
> Diff: https://reviews.apache.org/r/69364/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 69362: Checkpointed creation parameters for CSI volumes.

[Benjamin Bannier]

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



This patch fails to compile without r/69363, could you move the needed changes 
from there into this patch?


src/csi/state.proto
Lines 46 (patched)


Let's describe the semantics around empty vs.  populated `parameters` maps 
instead of introducing `OPTIONAL` here (which doesn't fit and is confusing).



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


Can we add a comment here mentioning what is supported?



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


Can we add a comment here mentioning what is valid?



src/resource_provider/storage/provider.cpp
Line 2821 (original)


Keep this?


- Benjamin Bannier


On Nov. 19, 2018, 10:29 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69362/
> ---
> 
> (Updated Nov. 19, 2018, 10:29 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Bugs: MESOS-9275
> https://issues.apache.org/jira/browse/MESOS-9275
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The parameters of CSI volumes created by SLRPs are now checkpointed, and
> used to validate volumes created from previous SLRP runs.
> 
> 
> Diffs
> -
> 
>   src/csi/state.proto 8445399ac17cfef740c2523172b34fd5439f2d3a 
>   src/resource_provider/storage/provider.cpp 
> c137fa4f13edc58d93c03a9dd32fdf9d38b38316 
> 
> 
> Diff: https://reviews.apache.org/r/69362/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 69362: Checkpointed creation parameters for CSI volumes.

[Chun-Hung Hsiao]

> On Nov. 19, 2018, 9:48 p.m., Benjamin Bannier wrote:
> > This patch fails to compile without r/69363, could you move the needed 
> > changes from there into this patch?

Just reordered. Is that okay?


> On Nov. 19, 2018, 9:48 p.m., Benjamin Bannier wrote:
> > src/csi/state.proto
> > Lines 46 (patched)
> > 
> >
> > Let's describe the semantics around empty vs.  populated `parameters` 
> > maps instead of introducing `OPTIONAL` here (which doesn't fit and is 
> > confusing).

This is a proto3 message, where there's no distinguish between empty and 
optional fields. I'm following the same principle used in CSI: 
https://github.com/container-storage-interface/spec/blob/master/spec.md#required-vs-optional


- Chun-Hung


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


On Nov. 19, 2018, 9:29 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69362/
> ---
> 
> (Updated Nov. 19, 2018, 9:29 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Bugs: MESOS-9275
> https://issues.apache.org/jira/browse/MESOS-9275
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The parameters of CSI volumes created by SLRPs are now checkpointed, and
> used to validate volumes created from previous SLRP runs.
> 
> 
> Diffs
> -
> 
>   src/csi/state.proto 8445399ac17cfef740c2523172b34fd5439f2d3a 
>   src/resource_provider/storage/provider.cpp 
> c137fa4f13edc58d93c03a9dd32fdf9d38b38316 
> 
> 
> Diff: https://reviews.apache.org/r/69362/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 69362: Checkpointed creation parameters for CSI volumes.

[Chun-Hung Hsiao]

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

(Updated Nov. 19, 2018, 9:29 p.m.)


Review request for mesos, Benjamin Bannier and Jie Yu.


Changes
---

Reordered.


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


Repository: mesos


Description
---

The parameters of CSI volumes created by SLRPs are now checkpointed, and
used to validate volumes created from previous SLRP runs.


Diffs
-

  src/csi/state.proto 8445399ac17cfef740c2523172b34fd5439f2d3a 
  src/resource_provider/storage/provider.cpp 
c137fa4f13edc58d93c03a9dd32fdf9d38b38316 


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


Testing
---

make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 69366: Used `OperationID` instead of `string` in test helpers.

[Chun-Hung Hsiao]

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

(Updated Nov. 19, 2018, 9:51 p.m.)


Review request for mesos, Benjamin Bannier, Gastón Kleiman, and Greg Mann.


Changes
---

Addressed Benjamin's comments.


Repository: mesos


Description (updated)
---

This change makes the helper more consisent with other helpers, and is
more future-proof to changes in `OperationID`.


Diffs (updated)
-

  src/tests/master_slave_reconciliation_tests.cpp 
064b4d47598b1d0e18d7bef14ac62d7e5f2c1102 
  src/tests/master_tests.cpp ac6bf379c5906cf9612284911c121c9457f648a0 
  src/tests/mesos.hpp 576f4bde88c069ee2fa0dd33912a034437338e7e 
  src/tests/operation_reconciliation_tests.cpp 
37d38b3df8c162bd1baa5ce557e54baa5c23a006 
  src/tests/storage_local_resource_provider_tests.cpp 
077a46585bd56181ba199dc529e09f38f4971338 


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

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


Testing
---

make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 69366: Used `OperationID` instead of `string` in test helpers.

[Chun-Hung Hsiao]

> On Nov. 16, 2018, 8:32 a.m., Benjamin Bannier wrote:
> > The commit message talks about `profile` -- could you fix that?

The description was accurate: if we passed in operation ID, then compiler will 
reject the following call:
```
CREATE_DISK(source, Resource::DiskInfo::Source::MOUNT, operationId);
```

But since the commit message does not clearly deliver the above idea, I'll 
remove the part about `profile`.


> On Nov. 16, 2018, 8:32 a.m., Benjamin Bannier wrote:
> > src/tests/storage_local_resource_provider_tests.cpp
> > Lines 4323-4326 (original), 4325-4328 (patched)
> > 
> >
> > Nit: Indented by 1 space too much? `clang-format` would have picked the 
> > old indent.

Dropping this and created https://issues.apache.org/jira/browse/MESOS-9402.


- Chun-Hung


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


On Nov. 16, 2018, 12:57 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69366/
> ---
> 
> (Updated Nov. 16, 2018, 12:57 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Gastón Kleiman, and Greg Mann.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change makes the helper more consisent with other helpers, and
> can avoid one to pass in a string operation ID as a profile.
> 
> 
> Diffs
> -
> 
>   src/tests/master_slave_reconciliation_tests.cpp 
> 064b4d47598b1d0e18d7bef14ac62d7e5f2c1102 
>   src/tests/master_tests.cpp ac6bf379c5906cf9612284911c121c9457f648a0 
>   src/tests/mesos.hpp 576f4bde88c069ee2fa0dd33912a034437338e7e 
>   src/tests/operation_reconciliation_tests.cpp 
> 37d38b3df8c162bd1baa5ce557e54baa5c23a006 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 077a46585bd56181ba199dc529e09f38f4971338 
> 
> 
> Diff: https://reviews.apache.org/r/69366/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 69363: Cleaned up `include/mesos/type_utils.hpp`.

[Benjamin Bannier]

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


Ship it!




Ship It!

- Benjamin Bannier


On Nov. 19, 2018, 10:29 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69363/
> ---
> 
> (Updated Nov. 19, 2018, 10:29 p.m.)
> 
> 
> Review request for mesos and Benjamin Bannier.
> 
> 
> Bugs: MESOS-9275
> https://issues.apache.org/jira/browse/MESOS-9275
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch does the following cleanups:
> 
> 1. Moved `google::protobuf::Map` equality operator to `type_utils.hpp`.
> 2. Moved the type helper templates for the protobuf library that do not
>involve mesos protobufs into the `google::protobuf` namespaces so ADL
>works appropriately.
> 3. Removed the type helper templates for the protobuf library from
>`mesos/v1/mesos.hpp` to avoid redefinition.
> 
> 
> Diffs
> -
> 
>   include/mesos/type_utils.hpp aa61c0c88be6a885d4392b1e0dedf52150fb1e31 
>   include/mesos/v1/mesos.hpp 452bcf2699506c15f410434100525b3ced7c4405 
>   src/resource_provider/storage/uri_disk_profile_adaptor.cpp 
> 6c998ef81ce369ea8677beeb3a0c9fdbb789 
> 
> 
> Diff: https://reviews.apache.org/r/69363/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 69363: Cleaned up `include/mesos/type_utils.hpp`.

[Chun-Hung Hsiao]

> On Nov. 16, 2018, 3:28 p.m., Benjamin Bannier wrote:
> > include/mesos/type_utils.hpp
> > Lines 490 (patched)
> > 
> >
> > Not yours, just making an issue for visibility. Feel free to drop.
> > 
> > Fits on one line.
> > 
> > This also performs two lookups for `it->first` into `right` which can 
> > both cost `O(log N)`. I think in general one should cache iterators,
> > ```
> > for (auto l = left.begin(); l != left.end(); ++l) {
> > auto r = right.find(l->first);
> >  
> > if (r == right.end()) {
> >   return false;
> > }
> >  
> > if (r->first != l->first) {
> >   return false;
> > }
> > }
> > ```

I believe you mean `r->second != l->second` ;)


- Chun-Hung


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


On Nov. 16, 2018, 12:47 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69363/
> ---
> 
> (Updated Nov. 16, 2018, 12:47 a.m.)
> 
> 
> Review request for mesos and Benjamin Bannier.
> 
> 
> Bugs: MESOS-9275
> https://issues.apache.org/jira/browse/MESOS-9275
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch does the following cleanups:
> 
> 1. Moved `google::protobuf::Map` equality operator to `type_utils.hpp`.
> 2. Moved the type helper templates that do not involve mesos protobufs
>into appropriate namespaces so ADL works appropriately.
> 3. Removed the type helpers in 2 from `mesos/v1/mesos.hpp` to avoid
>redefinition.
> 
> 
> Diffs
> -
> 
>   include/mesos/type_utils.hpp aa61c0c88be6a885d4392b1e0dedf52150fb1e31 
>   include/mesos/v1/mesos.hpp 452bcf2699506c15f410434100525b3ced7c4405 
>   src/resource_provider/storage/uri_disk_profile_adaptor.cpp 
> 6c998ef81ce369ea8677beeb3a0c9fdbb789 
> 
> 
> Diff: https://reviews.apache.org/r/69363/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 69363: Cleaned up `include/mesos/type_utils.hpp`.

[Chun-Hung Hsiao]

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

(Updated Nov. 19, 2018, 9:29 p.m.)


Review request for mesos and Benjamin Bannier.


Changes
---

Addressed Benjamin's comments and changed the order (since r/69362 depends on 
this patch).


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


Repository: mesos


Description (updated)
---

This patch does the following cleanups:

1. Moved `google::protobuf::Map` equality operator to `type_utils.hpp`.
2. Moved the type helper templates for the protobuf library that do not
   involve mesos protobufs into the `google::protobuf` namespaces so ADL
   works appropriately.
3. Removed the type helper templates for the protobuf library from
   `mesos/v1/mesos.hpp` to avoid redefinition.


Diffs (updated)
-

  include/mesos/type_utils.hpp aa61c0c88be6a885d4392b1e0dedf52150fb1e31 
  include/mesos/v1/mesos.hpp 452bcf2699506c15f410434100525b3ced7c4405 
  src/resource_provider/storage/uri_disk_profile_adaptor.cpp 
6c998ef81ce369ea8677beeb3a0c9fdbb789 


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

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


Testing
---

make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 69356: Added valiadtion for `Offer.Operation.CreateDisk.target_profile`.

[Chun-Hung Hsiao]

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

(Updated Nov. 19, 2018, 9:24 p.m.)


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


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


Repository: mesos


Description (updated)
---

Added validation for `Offer.Operation.CreateDisk.target_profile`.


Diffs
-

  src/master/validation.cpp 5768ac8fe802f28855fbd7be135c75532771 
  src/tests/master_validation_tests.cpp 
aa7c8f70c09459be32c6c415497e95fcdc218efd 


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


Testing (updated)
---

make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 69356: Added valiadtion for `Offer.Operation.CreateDisk.target_profile`.

[Chun-Hung Hsiao]

> On Nov. 16, 2018, 12:01 p.m., Benjamin Bannier wrote:
> > This patch breaks a number of tests. Could you make sure that they are 
> > updated with this patch or earlier?
> > 
> > Please spell-check the commit message.

Fixed the typo and the order.


- Chun-Hung


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


On Nov. 15, 2018, 11:55 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69356/
> ---
> 
> (Updated Nov. 15, 2018, 11:55 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, James DeFelice, and Jie Yu.
> 
> 
> Bugs: MESOS-9275
> https://issues.apache.org/jira/browse/MESOS-9275
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added valiadtion for `Offer.Operation.CreateDisk.target_profile`.
> 
> 
> Diffs
> -
> 
>   src/master/validation.cpp 5768ac8fe802f28855fbd7be135c75532771 
>   src/tests/master_validation_tests.cpp 
> aa7c8f70c09459be32c6c415497e95fcdc218efd 
> 
> 
> Diff: https://reviews.apache.org/r/69356/diff/1/
> 
> 
> Testing
> ---
> 
> Tests fixed later in chain.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 69359: Rewrote test `ReconcileDroppedOperation` for `CREATE_DISK`.

[Chun-Hung Hsiao]

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

(Updated Nov. 19, 2018, 9:12 p.m.)


Review request for mesos, Benjamin Bannier, Gastón Kleiman, and Greg Mann.


Changes
---

Addressed Benjamin's comments.


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


Repository: mesos


Description
---

Previously the `ReconcileDroppedOperation` test relies on converting
preprovisioned volumes. To adapt the new semantics for `CREATE_DISK`,
this test is rewritten to create two disks from a storage pool, with one
operation dropped.


Diffs (updated)
-

  src/tests/storage_local_resource_provider_tests.cpp 
077a46585bd56181ba199dc529e09f38f4971338 


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

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


Testing
---

make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 69360: Rewrote test `ConvertPreExistingVolume` for `CREATE_DISK`.

[Chun-Hung Hsiao]

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

(Updated Nov. 19, 2018, 9:19 p.m.)


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


Changes
---

Addressed James' and Benjamin's comments.


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


Repository: mesos


Description
---

Due to the changes of the `CREATE_DISK` semantics, this test is
rewritten to convert a preprovisioned volume to a profile volumes, and
then to destroy it to return the space back to the storage pool.


Diffs (updated)
-

  src/tests/storage_local_resource_provider_tests.cpp 
077a46585bd56181ba199dc529e09f38f4971338 


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

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


Testing
---

make check

This test passes iff the next patch is applied.


Thanks,

Chun-Hung Hsiao

Re: Review Request 69357: Added profiles to storage pools in tests for `CREATE_DISK`.

[Chun-Hung Hsiao]

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

(Updated Nov. 19, 2018, 9:09 p.m.)


Review request for mesos, Benjamin Bannier, Greg Mann, and Jan Schlicht.


Changes
---

Addressed Benjamin's comments.


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


Repository: mesos


Description
---

This patch adds a new `targetProfile` parameter to the `CREATE_DISK`
test helper, and add profiles to all storage pools in tests.

Tests relying on preprovisioned volumes are fixed in subsequent patches.


Diffs (updated)
-

  src/tests/api_tests.cpp fdd9f871f75617fc26a28679e2a1e41f506c6133 
  src/tests/master_tests.cpp ac6bf379c5906cf9612284911c121c9457f648a0 
  src/tests/mesos.hpp 576f4bde88c069ee2fa0dd33912a034437338e7e 
  src/tests/operation_reconciliation_tests.cpp 
37d38b3df8c162bd1baa5ce557e54baa5c23a006 
  src/tests/resource_provider_manager_tests.cpp 
5bb740edc7c3cc8698aede4f2ed57c21232fe378 
  src/tests/storage_local_resource_provider_tests.cpp 
077a46585bd56181ba199dc529e09f38f4971338 


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

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


Testing
---

`make check`

Tests `ReconcileDroppedOperation` and `ConvertPreExistingVolume` are fixed 
later in chain.


Thanks,

Chun-Hung Hsiao

Re: Review Request 69357: Added profiles to storage pools in tests for `CREATE_DISK`.

[Chun-Hung Hsiao]

> On Nov. 16, 2018, 12:01 p.m., Benjamin Bannier wrote:
> > Can you make sure that this patch does not produce any failing tests (might 
> > be triggered by earlier patch)?

Done for most patches, except for r/69360 and r/69361.


- Chun-Hung


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


On Nov. 15, 2018, 11:58 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69357/
> ---
> 
> (Updated Nov. 15, 2018, 11:58 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Greg Mann, and Jan Schlicht.
> 
> 
> Bugs: MESOS-9275
> https://issues.apache.org/jira/browse/MESOS-9275
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds a new `targetProfile` parameter to the `CREATE_DISK`
> test helper, and add profiles to all storage pools in tests.
> 
> Tests relying on preprovisioned volumes are fixed in subsequent patches.
> 
> 
> Diffs
> -
> 
>   src/tests/api_tests.cpp fdd9f871f75617fc26a28679e2a1e41f506c6133 
>   src/tests/master_tests.cpp ac6bf379c5906cf9612284911c121c9457f648a0 
>   src/tests/mesos.hpp 576f4bde88c069ee2fa0dd33912a034437338e7e 
>   src/tests/operation_reconciliation_tests.cpp 
> 37d38b3df8c162bd1baa5ce557e54baa5c23a006 
>   src/tests/resource_provider_manager_tests.cpp 
> 5bb740edc7c3cc8698aede4f2ed57c21232fe378 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 077a46585bd56181ba199dc529e09f38f4971338 
> 
> 
> Diff: https://reviews.apache.org/r/69357/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Tests `ReconcileDroppedOperation` and `ConvertPreExistingVolume` are fixed 
> later in chain.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 68131: Added MasterActorResponsiveness_BENCHMARK_Test.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [68131]

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

- Mesos Reviewbot


On Nov. 18, 2018, 8:10 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68131/
> ---
> 
> (Updated Nov. 18, 2018, 8:10 p.m.)
> 
> 
> Review request for mesos, Benno Evers and Benjamin Mahler.
> 
> 
> Bugs: MESOS-8975
> https://issues.apache.org/jira/browse/MESOS-8975
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/master_benchmarks.cpp fbfffb69930c30b038f74e0b831fc0ae41c820f0 
> 
> 
> Diff: https://reviews.apache.org/r/68131/diff/5/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/68132/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 69377: Added blog post for Mesos Mini.

[Jie Yu]

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

(Updated Nov. 19, 2018, 8:28 p.m.)


Review request for mesos, Benjamin Mahler, Chun-Hung Hsiao, Greg Mann, Jason 
Lai, James DeFelice, James Peach, and Vinod Kone.


Changes
---

Addressed comments.


Repository: mesos


Description
---

Added blog post for Mesos Mini.


Diffs (updated)
-

  site/source/blog/2018-11-19-mesos-mini.md PRE-CREATION 


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

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


Testing
---

Markdown rendering can be viewed here:
https://github.com/jieyu/mesos/blob/mesos_mini_blog/site/source/blog/2018-11-19-mesos-mini.md


Thanks,

Jie Yu

Re: Review Request 69377: Added blog post for Mesos Mini.

[Jie Yu]

> On Nov. 19, 2018, 8:02 a.m., Joerg Schad wrote:
> > site/source/blog/2018-11-19-mesos-mini.md
> > Lines 12 (patched)
> > 
> >
> > s/Mesos/Mesos and Marathon ?

Marathon is just an example framework. I mentioned below. I want to install 
more example frameworks there (e.g., Aurora, etc.)


> On Nov. 19, 2018, 8:02 a.m., Joerg Schad wrote:
> > site/source/blog/2018-11-19-mesos-mini.md
> > Lines 16 (patched)
> > 
> >
> > Should we explicitly call out limitations?

Yes, I'll add a section about current limitations.


> On Nov. 19, 2018, 8:02 a.m., Joerg Schad wrote:
> > site/source/blog/2018-11-19-mesos-mini.md
> > Lines 40 (patched)
> > 
> >
> > Are there configuration options for more agents?

Not yet.


- Jie


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


On Nov. 19, 2018, 5:47 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69377/
> ---
> 
> (Updated Nov. 19, 2018, 5:47 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Chun-Hung Hsiao, Greg Mann, Jason 
> Lai, James DeFelice, James Peach, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added blog post for Mesos Mini.
> 
> 
> Diffs
> -
> 
>   site/source/blog/2018-11-19-mesos-mini.md PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/69377/diff/3/
> 
> 
> Testing
> ---
> 
> Markdown rendering can be viewed here:
> https://github.com/jieyu/mesos/blob/mesos_mini_blog/site/source/blog/2018-11-19-mesos-mini.md
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 69356: Added valiadtion for `Offer.Operation.CreateDisk.target_profile`.

[Benjamin Bannier]

> On Nov. 16, 2018, 1:01 nachm., Benjamin Bannier wrote:
> > src/master/validation.cpp
> > Lines 2531-2535 (patched)
> > 
> >
> > I wonder whether performing this validation in the master is the right 
> > thing to do.
> > 
> > Profiles are currently applied by SLRP in the agent, so doing 
> > SLRP-specific validation here not only scatters logic in different places, 
> > but could also make making breaking changes in the future harder than 
> > necessary (agent and master are upgraded independently).
> > 
> > Should we move this into the agent, e.g., SLRP?
> 
> Chun-Hung Hsiao wrote:
> It seems to me that if this is the semantics in the API, then it seems to 
> me that we should validate this as early as possible to fail-fast. WDYT?

I agree with failing as early as possible. This should not happen at the cost 
of making master even more of an God object than it already is, though. Does 
that make sense?


- Benjamin


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


On Nov. 16, 2018, 12:55 vorm., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69356/
> ---
> 
> (Updated Nov. 16, 2018, 12:55 vorm.)
> 
> 
> Review request for mesos, Benjamin Bannier, James DeFelice, and Jie Yu.
> 
> 
> Bugs: MESOS-9275
> https://issues.apache.org/jira/browse/MESOS-9275
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added valiadtion for `Offer.Operation.CreateDisk.target_profile`.
> 
> 
> Diffs
> -
> 
>   src/master/validation.cpp 5768ac8fe802f28855fbd7be135c75532771 
>   src/tests/master_validation_tests.cpp 
> aa7c8f70c09459be32c6c415497e95fcdc218efd 
> 
> 
> Diff: https://reviews.apache.org/r/69356/diff/1/
> 
> 
> Testing
> ---
> 
> Tests fixed later in chain.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 69359: Rewrote test `ReconcileDroppedOperation` for `CREATE_DISK`.

[Chun-Hung Hsiao]

> On Nov. 16, 2018, 12:02 p.m., Benjamin Bannier wrote:
> > src/tests/storage_local_resource_provider_tests.cpp
> > Line 3899 (original), 3895 (patched)
> > 
> >
> > Before dereferencing `begin` we should `ASSERT_FALSE(raw.empty())`.

`ASSERT_SOME_EQ(Gigabytes(4), raw.disk())` alrealy guarantees that `raw` is not 
empty. But let me do a `ASSERT_EQ(1u, offersBeforeOperations->size())` to make 
sure that the offer contains raw disk resources.


> On Nov. 16, 2018, 12:02 p.m., Benjamin Bannier wrote:
> > src/tests/storage_local_resource_provider_tests.cpp
> > Lines 3899 (patched)
> > 
> >
> > Let's check before dereferencing `begin()` that `raw - source1` is not 
> > empty.

`ASSERT_SOME_EQ(Gigabytes(4), raw.disk())` would have guaranteed this. I prefer 
dropping this to keep the test succinct, unless you strongly prefer adding the 
extra check.


- Chun-Hung


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


On Nov. 16, 2018, midnight, Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69359/
> ---
> 
> (Updated Nov. 16, 2018, midnight)
> 
> 
> Review request for mesos, Benjamin Bannier, Gastón Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-9275
> https://issues.apache.org/jira/browse/MESOS-9275
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously the `ReconcileDroppedOperation` test relies on converting
> preprovisioned volumes. To adapt the new semantics for `CREATE_DISK`,
> this test is rewritten to create two disks from a storage pool, with one
> operation dropped.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 077a46585bd56181ba199dc529e09f38f4971338 
> 
> 
> Diff: https://reviews.apache.org/r/69359/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 69360: Rewrote test `ConvertPreExistingVolume` for `CREATE_DISK`.

[Chun-Hung Hsiao]

> On Nov. 16, 2018, 12:02 p.m., Benjamin Bannier wrote:
> > This test does not pass for me, could you fix that?

This modified test won't pass without the next patch. However, if we switch the 
order, the unmodified test won't pass. So no I cannot fix this, unfortunately :(


- Chun-Hung


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


On Nov. 16, 2018, 12:02 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69360/
> ---
> 
> (Updated Nov. 16, 2018, 12:02 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, James DeFelice, and Jie Yu.
> 
> 
> Bugs: MESOS-9275
> https://issues.apache.org/jira/browse/MESOS-9275
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Due to the changes of the `CREATE_DISK` semantics, this test is
> rewritten to convert a preprovisioned volume to a profile volumes, and
> then to destroy it to return the space back to the storage pool.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 077a46585bd56181ba199dc529e09f38f4971338 
> 
> 
> Diff: https://reviews.apache.org/r/69360/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> This test passes iff the next patch is applied.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 69386: Added test for ACCESS_MESOS_LOG authorization.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [69368, 69369, 69384, 69385, 69386]

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

- Mesos Reviewbot


On Nov. 18, 2018, 3:11 p.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69386/
> ---
> 
> (Updated Nov. 18, 2018, 3:11 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Alexander Rojas.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test for ACCESS_MESOS_LOG authorization.
> 
> 
> Diffs
> -
> 
>   src/tests/authorization_tests.cpp ac52181aa29bb5a5e4197cd90f32330aeb59385f 
> 
> 
> Diff: https://reviews.apache.org/r/69386/diff/1/
> 
> 
> Testing
> ---
> 
> `make check` and internal CI
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 69356: Added valiadtion for `Offer.Operation.CreateDisk.target_profile`.

[Chun-Hung Hsiao]

> On Nov. 16, 2018, 12:01 p.m., Benjamin Bannier wrote:
> > src/master/validation.cpp
> > Lines 2531-2535 (patched)
> > 
> >
> > I wonder whether performing this validation in the master is the right 
> > thing to do.
> > 
> > Profiles are currently applied by SLRP in the agent, so doing 
> > SLRP-specific validation here not only scatters logic in different places, 
> > but could also make making breaking changes in the future harder than 
> > necessary (agent and master are upgraded independently).
> > 
> > Should we move this into the agent, e.g., SLRP?

It seems to me that if this is the semantics in the API, then it seems to me 
that we should validate this as early as possible to fail-fast. WDYT?


- Chun-Hung


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


On Nov. 15, 2018, 11:55 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69356/
> ---
> 
> (Updated Nov. 15, 2018, 11:55 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, James DeFelice, and Jie Yu.
> 
> 
> Bugs: MESOS-9275
> https://issues.apache.org/jira/browse/MESOS-9275
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added valiadtion for `Offer.Operation.CreateDisk.target_profile`.
> 
> 
> Diffs
> -
> 
>   src/master/validation.cpp 5768ac8fe802f28855fbd7be135c75532771 
>   src/tests/master_validation_tests.cpp 
> aa7c8f70c09459be32c6c415497e95fcdc218efd 
> 
> 
> Diff: https://reviews.apache.org/r/69356/diff/1/
> 
> 
> Testing
> ---
> 
> Tests fixed later in chain.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 69398: Added validation for `FrameworkID`s.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 69398 was successfully built and tested.

Reviews applied: `['69397', '69398']`

All the build artifacts available at: 
http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2622/mesos-review-69398

- Mesos Reviewbot Windows


On Nov. 19, 2018, 3:52 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69398/
> ---
> 
> (Updated Nov. 19, 2018, 3:52 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Meng Zhu.
> 
> 
> Bugs: MESOS-8470
> https://issues.apache.org/jira/browse/MESOS-8470
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We never explicitly supported `FrameworkID`s containing literal `/`.
> Moreover, since the introduction of hierarchical roles such
> `FrameworkID`s would cause fatal errors.
> 
> This patch adds validation for `FrameworkID`s so such IDs are
> rejected.
> 
> 
> Diffs
> -
> 
>   src/master/validation.hpp 9af903970795a5c8c479d1984a580e41d91f6c91 
>   src/master/validation.cpp 5768ac8fe802f28855fbd7be135c75532771 
>   src/tests/master_validation_tests.cpp 
> aa7c8f70c09459be32c6c415497e95fcdc218efd 
> 
> 
> Diff: https://reviews.apache.org/r/69398/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 69255: Updated PyInstaller requirement for new CLI to support Python 3.7.

[Kevin Klues]

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


Ship it!




Ship It!

- Kevin Klues


On Nov. 5, 2018, 6:13 nachm., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69255/
> ---
> 
> (Updated Nov. 5, 2018, 6:13 nachm.)
> 
> 
> Review request for mesos, Benjamin Bannier and Kevin Klues.
> 
> 
> Bugs: MESOS-9370
> https://issues.apache.org/jira/browse/MESOS-9370
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated PyInstaller requirement for new CLI to support Python 3.7.
> 
> 
> Diffs
> -
> 
>   src/python/cli_new/pip-requirements.txt 
> 520dc6131cf05e8f4626a1c673be9b6a59766a8d 
> 
> 
> Diff: https://reviews.apache.org/r/69255/diff/2/
> 
> 
> Testing
> ---
> 
> ```
> $ ./bootstrap
> $ mkdir build
> $ cd build
> $ PYTHON_3=python37 ../configure --enable-new-cli
> $ make check
> $ ./src/mesos
> ```
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 69395: Added '--all' flag to 'mesos task list'.

[Kevin Klues]

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




src/python/cli_new/lib/cli/tests/task.py
Lines 238 (patched)


This will be flakey.


- Kevin Klues


On Nov. 19, 2018, 2:17 nachm., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69395/
> ---
> 
> (Updated Nov. 19, 2018, 2:17 nachm.)
> 
> 
> Review request for mesos and Kevin Klues.
> 
> 
> Bugs: MESOS-9399
> https://issues.apache.org/jira/browse/MESOS-9399
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> With this option, the command can show all the tasks that have ever been
> run. This makes the command's behavior closer to the one of 'docker ps'.
> 
> 
> Diffs
> -
> 
>   src/python/cli_new/lib/cli/plugins/task/main.py 
> 0536ccb29551400ce59713017c86eb5e858aeb5f 
>   src/python/cli_new/lib/cli/tests/task.py 
> f033b4a7663279c8a7c49ed86b3acdda45ceb107 
> 
> 
> Diff: https://reviews.apache.org/r/69395/diff/1/
> 
> 
> Testing
> ---
> 
> ```
> (mesos-cli) ?  cli_new (MESOS-9399) ? mesos-cli-tests
> Running the Mesos CLI unit tests
> 
> TestAgentPlugin
> test_list (cli.tests.agent.TestAgentPlugin) ... ok
> 
> TestInfrastructure
> test_capture_output (cli.tests.tests.TestInfrastructure) ... ok
> test_launch_binaries (cli.tests.tests.TestInfrastructure) ... ok
> 
> TestTaskPlugin
> test_exec (cli.tests.task.TestTaskPlugin) ... ok
> test_exec_exit_status (cli.tests.task.TestTaskPlugin) ... ok
> test_exec_interactive (cli.tests.task.TestTaskPlugin) ... ok
> test_list (cli.tests.task.TestTaskPlugin) ... ok
> test_list_all (cli.tests.task.TestTaskPlugin) ... ok
> 
> --
> Ran 8 tests in 20.889s
> 
> OK
> ```
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 69393: Displayed 'State' field when using 'mesos task list'.

[Armand Grillet]

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

(Updated Nov. 19, 2018, 5:14 p.m.)


Review request for mesos and Kevin Klues.


Changes
---

Fixed issues.


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


Repository: mesos


Description
---

Displayed 'State' field when using 'mesos task list'.


Diffs (updated)
-

  src/python/cli_new/lib/cli/plugins/task/main.py 
0536ccb29551400ce59713017c86eb5e858aeb5f 
  src/python/cli_new/lib/cli/tests/task.py 
f033b4a7663279c8a7c49ed86b3acdda45ceb107 


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

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


Testing
---

```
(mesos-cli) ?  cli_new (MESOS-9399) ? mesos-cli-tests
Running the Mesos CLI unit tests

TestAgentPlugin
test_list (cli.tests.agent.TestAgentPlugin) ... ok

TestInfrastructure
test_capture_output (cli.tests.tests.TestInfrastructure) ... ok
test_launch_binaries (cli.tests.tests.TestInfrastructure) ... ok

TestTaskPlugin
test_exec (cli.tests.task.TestTaskPlugin) ... ok
test_exec_exit_status (cli.tests.task.TestTaskPlugin) ... ok
test_exec_interactive (cli.tests.task.TestTaskPlugin) ... ok
test_list (cli.tests.task.TestTaskPlugin) ... ok

--
Ran 7 tests in 20.812s

OK
```


Thanks,

Armand Grillet

Re: Review Request 69393: Displayed 'State' field when using 'mesos task list'.

[Kevin Klues]

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




src/python/cli_new/lib/cli/plugins/task/main.py
Lines 125 (patched)


Let's make the default display "UNKNOWN" instead of just blank.



src/python/cli_new/lib/cli/plugins/task/main.py
Lines 127 (patched)


We should use [-1] here instaead of pop().



src/python/cli_new/lib/cli/tests/task.py
Line 188 (original), 189 (patched)


Should now be UNKNOWN

Keep in mind though, that this is flakey, because we don't know when the 
task will transition states.


- Kevin Klues


On Nov. 19, 2018, 2:11 nachm., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69393/
> ---
> 
> (Updated Nov. 19, 2018, 2:11 nachm.)
> 
> 
> Review request for mesos and Kevin Klues.
> 
> 
> Bugs: MESOS-9399
> https://issues.apache.org/jira/browse/MESOS-9399
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Displayed 'State' field when using 'mesos task list'.
> 
> 
> Diffs
> -
> 
>   src/python/cli_new/lib/cli/plugins/task/main.py 
> 0536ccb29551400ce59713017c86eb5e858aeb5f 
>   src/python/cli_new/lib/cli/tests/task.py 
> f033b4a7663279c8a7c49ed86b3acdda45ceb107 
> 
> 
> Diff: https://reviews.apache.org/r/69393/diff/1/
> 
> 
> Testing
> ---
> 
> ```
> (mesos-cli) ?  cli_new (MESOS-9399) ? mesos-cli-tests
> Running the Mesos CLI unit tests
> 
> TestAgentPlugin
> test_list (cli.tests.agent.TestAgentPlugin) ... ok
> 
> TestInfrastructure
> test_capture_output (cli.tests.tests.TestInfrastructure) ... ok
> test_launch_binaries (cli.tests.tests.TestInfrastructure) ... ok
> 
> TestTaskPlugin
> test_exec (cli.tests.task.TestTaskPlugin) ... ok
> test_exec_exit_status (cli.tests.task.TestTaskPlugin) ... ok
> test_exec_interactive (cli.tests.task.TestTaskPlugin) ... ok
> test_list (cli.tests.task.TestTaskPlugin) ... ok
> 
> --
> Ran 7 tests in 20.812s
> 
> OK
> ```
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 69374: Updated new CLI test step to use binary created by PyInstaller.

[Kevin Klues]

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


Ship it!




I have made the changes listed above locally before committing.

- Kevin Klues


On Nov. 16, 2018, 9:36 nachm., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69374/
> ---
> 
> (Updated Nov. 16, 2018, 9:36 nachm.)
> 
> 
> Review request for mesos, Benjamin Bannier and Kevin Klues.
> 
> 
> Bugs: MESOS-9396
> https://issues.apache.org/jira/browse/MESOS-9396
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The integration tests for the new CLI running while building Mesos now
> directly use the binary created during the build. That way we make sure
> that the binary created using PyInstaller is usable, which is the
> artifact that we want to distribute to users in the future.
> 
> Previously, we were only activating the virtual environment to run the
> tests thus the binary created by PyInstaller was never properly tested.
> To use the binary created by PyInstaller, we simply update the PATH
> before running 'mesos-cli-tests'.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am c17eae4ff1d019d515f67d81821e933ecb5dc190 
>   src/python/cli_new/tests/CMakeLists.txt 
> 19119d1d1d640c10ef4ec8e245773920359ccb75 
> 
> 
> Diff: https://reviews.apache.org/r/69374/diff/1/
> 
> 
> Testing
> ---
> 
> The main test for this change is to make sure that we do not rely on the 
> `mesos` in `src/python/cli_new/bin/` anymore. To do so, I have followed these 
> steps:
> 
> For Autotools:
> ```
> $ ./bootstrap
> $ mkdir build
> $ cd build
> $ PYTHON_3=python36 ../configure --enable-new-cli
> $ make check
> $ mv /home/agrillet/mesos/src/python/cli_new/bin/mesos 
> /home/agrillet/mesos/src/python/cli_new/bin/mesos2
> $ make check
> ```
> 
> Then I put `/home/agrillet/mesos/src/python/cli_new/bin/mesos` back.
> 
> For CMake:
> ```
> $ ./bootstrap
> $ mkdir build
> $ cd build
> $ cmake .. -DENABLE_NEW_CLI=1 -DPYTHON_3=python36
> $ cmake --build . --target tests
> $ ctest -R CLITests -V
> $ mv /home/agrillet/mesos/src/python/cli_new/bin/mesos 
> /home/agrillet/mesos/src/python/cli_new/bin/mesos2
> $ ctest -R CLITests -V
> ```
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Review Request 69398: Added validation for `FrameworkID`s.

[Benjamin Bannier]

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

Review request for mesos and Meng Zhu.


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


Repository: mesos


Description
---

We never explicitly supported `FrameworkID`s containing literal `/`.
Moreover, since the introduction of hierarchical roles such
`FrameworkID`s would cause fatal errors.

This patch adds validation for `FrameworkID`s so such IDs are
rejected.


Diffs
-

  src/master/validation.hpp 9af903970795a5c8c479d1984a580e41d91f6c91 
  src/master/validation.cpp 5768ac8fe802f28855fbd7be135c75532771 
  src/tests/master_validation_tests.cpp 
aa7c8f70c09459be32c6c415497e95fcdc218efd 


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


Testing
---

`make check`


Thanks,

Benjamin Bannier

Re: Review Request 69390: Added docs describing how to use the new CLI.

[Kevin Klues]

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




docs/cli.md
Lines 17 (patched)


Can you add something like ...
```
For now, the CLi is still under development, and not built as part of a 
standard mesos distribution. To use it ...
```



docs/cli.md
Lines 29 (patched)


newline after this?



docs/cli.md
Lines 54 (patched)


Let's move these instructions up somehwere near where I mentioned that its 
still under development, etc.


- Kevin Klues


On Nov. 19, 2018, 10:31 vorm., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69390/
> ---
> 
> (Updated Nov. 19, 2018, 10:31 vorm.)
> 
> 
> Review request for mesos and Kevin Klues.
> 
> 
> Bugs: MESOS-9333
> https://issues.apache.org/jira/browse/MESOS-9333
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The documentation describes the main commands of the CLI, how to enable
> it and how to build Mesos including this component.
> 
> 
> Diffs
> -
> 
>   docs/cli.md PRE-CREATION 
>   docs/home.md e05b65d55176a706c072f73904a8e0f4365a8cb2 
> 
> 
> Diff: https://reviews.apache.org/r/69390/diff/1/
> 
> 
> Testing
> ---
> 
> Checked the rendered Markdown code.
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Review Request 69397: Used high-level `FrameworkInfo` validation function in tests.

[Benjamin Bannier]

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

Review request for mesos and Meng Zhu.


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


Repository: mesos


Description
---

This patch replaces uses of `framework::validateRoles` with the more
high-level `framework::validate`. Since e.g., the master invokes
`validate` instead of directly invoking `validateRoles` this ensures
that our tests more closely match master behavior, and helps
preventing that we test code which is not invoked in reality.


Diffs
-

  src/tests/master_validation_tests.cpp 
aa7c8f70c09459be32c6c415497e95fcdc218efd 


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


Testing
---

`make check`


Thanks,

Benjamin Bannier

Re: Review Request 69381: Updated configuration docs describing how to build the new CLI.

[Kevin Klues]

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


Ship it!




Ship It!

- Kevin Klues


On Nov. 18, 2018, 5:58 nachm., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69381/
> ---
> 
> (Updated Nov. 18, 2018, 5:58 nachm.)
> 
> 
> Review request for mesos and Kevin Klues.
> 
> 
> Bugs: MESOS-9333
> https://issues.apache.org/jira/browse/MESOS-9333
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated configuration docs describing how to build the new CLI.
> 
> 
> Diffs
> -
> 
>   docs/configuration/autotools.md 3c3c59a6343e00df16f4d0698fd61fb5ef792c1c 
>   docs/configuration/cmake.md 0cba405c53c5e2efdc7170abdafdd4a5db331804 
> 
> 
> Diff: https://reviews.apache.org/r/69381/diff/1/
> 
> 
> Testing
> ---
> 
> Checked the rendered Markdown code.
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 69380: Added configuration docs describing how to use Python 3.

[Kevin Klues]

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


Ship it!




Ship It!

- Kevin Klues


On Nov. 18, 2018, 5:58 nachm., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69380/
> ---
> 
> (Updated Nov. 18, 2018, 5:58 nachm.)
> 
> 
> Review request for mesos and Kevin Klues.
> 
> 
> Bugs: MESOS-9333
> https://issues.apache.org/jira/browse/MESOS-9333
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> For Autotools, this means how to use 'PYTHON_3' and 'PYTHON_3_VERSION'.
> 
> For CMake, this means how to use '-DPYTHON_3'.
> 
> 
> Diffs
> -
> 
>   docs/configuration/autotools.md 3c3c59a6343e00df16f4d0698fd61fb5ef792c1c 
>   docs/configuration/cmake.md 0cba405c53c5e2efdc7170abdafdd4a5db331804 
> 
> 
> Diff: https://reviews.apache.org/r/69380/diff/1/
> 
> 
> Testing
> ---
> 
> Checked the rendered Markdown code.
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 69395: Added '--all' flag to 'mesos task list'.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 69395 was successfully built and tested.

Reviews applied: `['69393', '69394', '69395']`

All the build artifacts available at: 
http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2621/mesos-review-69395

- Mesos Reviewbot Windows


On Nov. 19, 2018, 2:17 p.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69395/
> ---
> 
> (Updated Nov. 19, 2018, 2:17 p.m.)
> 
> 
> Review request for mesos and Kevin Klues.
> 
> 
> Bugs: MESOS-9399
> https://issues.apache.org/jira/browse/MESOS-9399
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> With this option, the command can show all the tasks that have ever been
> run. This makes the command's behavior closer to the one of 'docker ps'.
> 
> 
> Diffs
> -
> 
>   src/python/cli_new/lib/cli/plugins/task/main.py 
> 0536ccb29551400ce59713017c86eb5e858aeb5f 
>   src/python/cli_new/lib/cli/tests/task.py 
> f033b4a7663279c8a7c49ed86b3acdda45ceb107 
> 
> 
> Diff: https://reviews.apache.org/r/69395/diff/1/
> 
> 
> Testing
> ---
> 
> ```
> (mesos-cli) ?  cli_new (MESOS-9399) ? mesos-cli-tests
> Running the Mesos CLI unit tests
> 
> TestAgentPlugin
> test_list (cli.tests.agent.TestAgentPlugin) ... ok
> 
> TestInfrastructure
> test_capture_output (cli.tests.tests.TestInfrastructure) ... ok
> test_launch_binaries (cli.tests.tests.TestInfrastructure) ... ok
> 
> TestTaskPlugin
> test_exec (cli.tests.task.TestTaskPlugin) ... ok
> test_exec_exit_status (cli.tests.task.TestTaskPlugin) ... ok
> test_exec_interactive (cli.tests.task.TestTaskPlugin) ... ok
> test_list (cli.tests.task.TestTaskPlugin) ... ok
> test_list_all (cli.tests.task.TestTaskPlugin) ... ok
> 
> --
> Ran 8 tests in 20.889s
> 
> OK
> ```
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 69374: Updated new CLI test step to use binary created by PyInstaller.

[Kevin Klues]

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




src/Makefile.am
Lines 2827 (patched)


Since we are doing the postactivate, we don't need the full path to 
mesos-cli-tests anymore, let's remove it...



src/python/cli_new/tests/CMakeLists.txt
Lines 26 (patched)


Ditto


- Kevin Klues


On Nov. 16, 2018, 9:36 nachm., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69374/
> ---
> 
> (Updated Nov. 16, 2018, 9:36 nachm.)
> 
> 
> Review request for mesos, Benjamin Bannier and Kevin Klues.
> 
> 
> Bugs: MESOS-9396
> https://issues.apache.org/jira/browse/MESOS-9396
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The integration tests for the new CLI running while building Mesos now
> directly use the binary created during the build. That way we make sure
> that the binary created using PyInstaller is usable, which is the
> artifact that we want to distribute to users in the future.
> 
> Previously, we were only activating the virtual environment to run the
> tests thus the binary created by PyInstaller was never properly tested.
> To use the binary created by PyInstaller, we simply update the PATH
> before running 'mesos-cli-tests'.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am c17eae4ff1d019d515f67d81821e933ecb5dc190 
>   src/python/cli_new/tests/CMakeLists.txt 
> 19119d1d1d640c10ef4ec8e245773920359ccb75 
> 
> 
> Diff: https://reviews.apache.org/r/69374/diff/1/
> 
> 
> Testing
> ---
> 
> The main test for this change is to make sure that we do not rely on the 
> `mesos` in `src/python/cli_new/bin/` anymore. To do so, I have followed these 
> steps:
> 
> For Autotools:
> ```
> $ ./bootstrap
> $ mkdir build
> $ cd build
> $ PYTHON_3=python36 ../configure --enable-new-cli
> $ make check
> $ mv /home/agrillet/mesos/src/python/cli_new/bin/mesos 
> /home/agrillet/mesos/src/python/cli_new/bin/mesos2
> $ make check
> ```
> 
> Then I put `/home/agrillet/mesos/src/python/cli_new/bin/mesos` back.
> 
> For CMake:
> ```
> $ ./bootstrap
> $ mkdir build
> $ cd build
> $ cmake .. -DENABLE_NEW_CLI=1 -DPYTHON_3=python36
> $ cmake --build . --target tests
> $ ctest -R CLITests -V
> $ mv /home/agrillet/mesos/src/python/cli_new/bin/mesos 
> /home/agrillet/mesos/src/python/cli_new/bin/mesos2
> $ ctest -R CLITests -V
> ```
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Review Request 69395: Added '--all' flag to 'mesos task list'.

[Armand Grillet]

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

Review request for mesos and Kevin Klues.


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


Repository: mesos


Description
---

With this option, the command can show all the tasks that have ever been
run. This makes the command's behavior closer to the one of 'docker ps'.


Diffs
-

  src/python/cli_new/lib/cli/plugins/task/main.py 
0536ccb29551400ce59713017c86eb5e858aeb5f 
  src/python/cli_new/lib/cli/tests/task.py 
f033b4a7663279c8a7c49ed86b3acdda45ceb107 


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


Testing
---

```
(mesos-cli) ?  cli_new (MESOS-9399) ? mesos-cli-tests
Running the Mesos CLI unit tests

TestAgentPlugin
test_list (cli.tests.agent.TestAgentPlugin) ... ok

TestInfrastructure
test_capture_output (cli.tests.tests.TestInfrastructure) ... ok
test_launch_binaries (cli.tests.tests.TestInfrastructure) ... ok

TestTaskPlugin
test_exec (cli.tests.task.TestTaskPlugin) ... ok
test_exec_exit_status (cli.tests.task.TestTaskPlugin) ... ok
test_exec_interactive (cli.tests.task.TestTaskPlugin) ... ok
test_list (cli.tests.task.TestTaskPlugin) ... ok
test_list_all (cli.tests.task.TestTaskPlugin) ... ok

--
Ran 8 tests in 20.889s

OK
```


Thanks,

Armand Grillet

Review Request 69394: Updated 'mesos task list' to only display running tasks.

[Armand Grillet]

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

Review request for mesos and Kevin Klues.


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


Repository: mesos


Description
---

Updated 'mesos task list' to only display running tasks.


Diffs
-

  src/python/cli_new/lib/cli/plugins/task/main.py 
0536ccb29551400ce59713017c86eb5e858aeb5f 
  src/python/cli_new/lib/cli/tests/task.py 
f033b4a7663279c8a7c49ed86b3acdda45ceb107 


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


Testing
---

(mesos-cli) ?  cli_new (MESOS-9399) ? mesos-cli-tests
Running the Mesos CLI unit tests

TestAgentPlugin
test_list (cli.tests.agent.TestAgentPlugin) ... ok

TestInfrastructure
test_capture_output (cli.tests.tests.TestInfrastructure) ... ok
test_launch_binaries (cli.tests.tests.TestInfrastructure) ... ok

TestTaskPlugin
test_exec (cli.tests.task.TestTaskPlugin) ... ok
test_exec_exit_status (cli.tests.task.TestTaskPlugin) ... ok
test_exec_interactive (cli.tests.task.TestTaskPlugin) ... ok
test_list (cli.tests.task.TestTaskPlugin) ... ok

--
Ran 7 tests in 19.845s

OK


Thanks,

Armand Grillet

Review Request 69393: Displayed 'State' field when using 'mesos task list'.

[Armand Grillet]

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

Review request for mesos and Kevin Klues.


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


Repository: mesos


Description
---

Displayed 'State' field when using 'mesos task list'.


Diffs
-

  src/python/cli_new/lib/cli/plugins/task/main.py 
0536ccb29551400ce59713017c86eb5e858aeb5f 
  src/python/cli_new/lib/cli/tests/task.py 
f033b4a7663279c8a7c49ed86b3acdda45ceb107 


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


Testing
---

```
(mesos-cli) ?  cli_new (MESOS-9399) ? mesos-cli-tests
Running the Mesos CLI unit tests

TestAgentPlugin
test_list (cli.tests.agent.TestAgentPlugin) ... ok

TestInfrastructure
test_capture_output (cli.tests.tests.TestInfrastructure) ... ok
test_launch_binaries (cli.tests.tests.TestInfrastructure) ... ok

TestTaskPlugin
test_exec (cli.tests.task.TestTaskPlugin) ... ok
test_exec_exit_status (cli.tests.task.TestTaskPlugin) ... ok
test_exec_interactive (cli.tests.task.TestTaskPlugin) ... ok
test_list (cli.tests.task.TestTaskPlugin) ... ok

--
Ran 7 tests in 20.812s

OK
```


Thanks,

Armand Grillet

Re: Review Request 69389: Added a test `ROOT_UNPRIVILEGED_USER_SandboxOwnership`.

[Gilbert Song]

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


Ship it!




Ship It!

- Gilbert Song


On Nov. 18, 2018, 11:33 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69389/
> ---
> 
> (Updated Nov. 18, 2018, 11:33 p.m.)
> 
> 
> Review request for mesos and Gilbert Song.
> 
> 
> Bugs: MESOS-9332
> https://issues.apache.org/jira/browse/MESOS-9332
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test `ROOT_UNPRIVILEGED_USER_SandboxOwnership`.
> 
> 
> Diffs
> -
> 
>   src/tests/default_executor_tests.cpp 
> f23cb07fbdc9dac0acf75714c835e0eedd7dce6f 
> 
> 
> Diff: https://reviews.apache.org/r/69389/diff/1/
> 
> 
> Testing
> ---
> 
> Ran this test repeatedly, all succeeded.
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 68131: Added MasterActorResponsiveness_BENCHMARK_Test.

[Benno Evers]

> On Nov. 6, 2018, 12:19 p.m., Benno Evers wrote:
> > src/tests/master_benchmarks.cpp
> > Lines 616 (patched)
> > 
> >
> > Last time we tried running this benchmark, we discovered a dead-lock 
> > caused by the interaction between the use of libprocess worker threads in 
> > both our test code and in the mesos code. 
> > 
> > Ideally we wouldn't use libprocess at all here, but to avoid another 
> > big change late in the review cycle, adding a check like this should be 
> > sufficient:
> > ```
> > if (numClients >= LIBPROCESS_NUM_WORKER_THREADS - 3) {
> >   cerr << "Not enough worker threads for the desired number of 
> > clients.";
> >   exit(1);
> > }
> > ```
> 
> Alexander Rukletsov wrote:
> Though I'm in favour of the idea, I'm not sure we can reliably get 
> `LIBPROCESS_NUM_WORKER_THREADS` in tests. Do you think we should try find 
> some substitute or punt on this and leave a comment?

I think it's fine to punt for now, assuming that the comment WILL BE VERY 
NOTICABLE. I've created https://issues.apache.org/jira/browse/MESOS-9400 for 
this purpose.


> On Nov. 6, 2018, 12:19 p.m., Benno Evers wrote:
> > src/tests/master_benchmarks.cpp
> > Line 646 (original), 702 (patched)
> > 
> >
> > Given that we're capturing and printing baseline statistics anyways, 
> > would it make sense to print the relative slowdown between baseline 
> > performance and performance under load here, to get a metric that is 
> > comparable across Mesos versions?
> 
> Alexander Rukletsov wrote:
> I'm not sure what a formual for a standard relative slowdown would be : 
> ). Do you have a suggestion? I'm inclining to leave it as is.

I'm itching to sit down with a statistics textbook to figure this out, but for 
efficiency's sake let's leave it as is for now :)


- Benno


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


On Nov. 18, 2018, 8:10 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68131/
> ---
> 
> (Updated Nov. 18, 2018, 8:10 p.m.)
> 
> 
> Review request for mesos, Benno Evers and Benjamin Mahler.
> 
> 
> Bugs: MESOS-8975
> https://issues.apache.org/jira/browse/MESOS-8975
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/master_benchmarks.cpp fbfffb69930c30b038f74e0b831fc0ae41c820f0 
> 
> 
> Diff: https://reviews.apache.org/r/68131/diff/5/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/68132/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 69390: Added docs describing how to use the new CLI.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 69390 was successfully built and tested.

Reviews applied: `['69380', '69381', '69390']`

All the build artifacts available at: 
http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2620/mesos-review-69390

- Mesos Reviewbot Windows


On Nov. 19, 2018, 10:31 a.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69390/
> ---
> 
> (Updated Nov. 19, 2018, 10:31 a.m.)
> 
> 
> Review request for mesos and Kevin Klues.
> 
> 
> Bugs: MESOS-9333
> https://issues.apache.org/jira/browse/MESOS-9333
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The documentation describes the main commands of the CLI, how to enable
> it and how to build Mesos including this component.
> 
> 
> Diffs
> -
> 
>   docs/cli.md PRE-CREATION 
>   docs/home.md e05b65d55176a706c072f73904a8e0f4365a8cb2 
> 
> 
> Diff: https://reviews.apache.org/r/69390/diff/1/
> 
> 
> Testing
> ---
> 
> Checked the rendered Markdown code.
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 69386: Added test for ACCESS_MESOS_LOG authorization.

[Alexander Rukletsov]

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


Ship it!




Appreciate you cleaning up things around you even though they are not directly 
related to the ticket you're working on. Thank you, Till!

- Alexander Rukletsov


On Nov. 18, 2018, 11:11 p.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69386/
> ---
> 
> (Updated Nov. 18, 2018, 11:11 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Alexander Rojas.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test for ACCESS_MESOS_LOG authorization.
> 
> 
> Diffs
> -
> 
>   src/tests/authorization_tests.cpp ac52181aa29bb5a5e4197cd90f32330aeb59385f 
> 
> 
> Diff: https://reviews.apache.org/r/69386/diff/1/
> 
> 
> Testing
> ---
> 
> `make check` and internal CI
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Review Request 69390: Added docs describing how to use the new CLI.

[Armand Grillet]

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

Review request for mesos and Kevin Klues.


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


Repository: mesos


Description
---

The documentation describes the main commands of the CLI, how to enable
it and how to build Mesos including this component.


Diffs
-

  docs/cli.md PRE-CREATION 
  docs/home.md e05b65d55176a706c072f73904a8e0f4365a8cb2 


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


Testing
---

Checked the rendered Markdown code.


Thanks,

Armand Grillet

Re: Review Request 69368: Added test reproducing crash on authorization failure.

[Till Toenshoff via Review Board]

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




src/tests/master_tests.cpp
Line 10101 (original), 10101 (patched)


Lacks period.


- Till Toenshoff


On Nov. 18, 2018, 10:48 p.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69368/
> ---
> 
> (Updated Nov. 18, 2018, 10:48 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Alexander Rojas, and Benjamin 
> Bannier.
> 
> 
> Bugs: MESOS-9317
> https://issues.apache.org/jira/browse/MESOS-9317
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This test reproduces the scenario as described in MESOS-9317. The test
> attempts to create a persistent volume by a web request to the
> authorized V1 operator endpoint. The test assures that the underlying
> authorization request fails as it can in production due to failures in
> the authorization backend.
> 
> Without fixing MESOS-9317, this test crashes the master process as the
> code-path involved will attempt to access the contents of the awaited
> future even though the future had failed.
> 
> 
> Diffs
> -
> 
>   src/tests/master_tests.cpp ac6bf379c5906cf9612284911c121c9457f648a0 
> 
> 
> Diff: https://reviews.apache.org/r/69368/diff/3/
> 
> 
> Testing
> ---
> 
> `make check` before applying the fix for MESOS-9317 and after doing so.
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 69384: Introduced common/authorization and refactored collectAuthorizations.

[Alexander Rukletsov]

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


Fix it, then Ship it!





src/master/weights_handler.cpp
Lines 33 (patched)


This should go before "master/..."


- Alexander Rukletsov


On Nov. 18, 2018, 10:52 p.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69384/
> ---
> 
> (Updated Nov. 18, 2018, 10:52 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Alexander Rojas, and Benjamin 
> Bannier.
> 
> 
> Bugs: MESOS-9317
> https://issues.apache.org/jira/browse/MESOS-9317
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds a new collection of authorization specific helper/s to reduce code
> duplication and increase efficient test coverage.
> 
> Moves the newly introduced 'collectAuthorizations' helper into this new
> authorization source unit.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 732368293049b7d9d6f62057344d433637ad44e8 
>   src/Makefile.am c17eae4ff1d019d515f67d81821e933ecb5dc190 
>   src/common/authorization.hpp PRE-CREATION 
>   src/common/authorization.cpp PRE-CREATION 
>   src/master/master.hpp e77babf22126838c63cd05e483875c9beb3ac5ff 
>   src/master/master.cpp 9458ff10999d48e40a8596ec4edf1243591bc0d4 
>   src/master/weights_handler.cpp 222ec754e216da195250d1895a728294a076ee5d 
>   src/tests/master_tests.cpp ac6bf379c5906cf9612284911c121c9457f648a0 
> 
> 
> Diff: https://reviews.apache.org/r/69384/diff/1/
> 
> 
> Testing
> ---
> 
> make check and private ci
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 69385: Refactored createSubject and authorizeLogAccess to common/authorization.

[Alexander Rukletsov]

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


Fix it, then Ship it!





src/common/http.cpp
Lines 905-906 (original), 880-881 (patched)


Looks like another candidate for "common/authorization.hpp". Mind leaving a 
TODO?



src/master/master.cpp
Line 1077 (original), 1077 (patched)


Let's leave a TODO here (it will also help explaining why you do this):
```
  // TODO(tillt): Use lambda named captures for
  // these cached values once it is available.
```



src/slave/slave.cpp
Line 839 (original), 840 (patched)


Ditto


- Alexander Rukletsov


On Nov. 18, 2018, 11:10 p.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69385/
> ---
> 
> (Updated Nov. 18, 2018, 11:10 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Alexander Rojas, Benjamin 
> Bannier, and Greg Mann.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Moves 'createSubject' out of common/http into common/authorization.
> 
> Removes duplicate 'authorizeLogAccess' out of master.cpp and slave.cpp.
> Introduces 'authorizeLogAccess' within common/authorization.
> 
> 
> Diffs
> -
> 
>   src/common/authorization.hpp PRE-CREATION 
>   src/common/authorization.cpp PRE-CREATION 
>   src/common/http.hpp 6ca54a641d1ea1625c70518f8870516664741e9a 
>   src/common/http.cpp d9703f80880850b8a1ec9ddd2b1e8f125f36 
>   src/master/http.cpp 75ab6ea422a77e25049cd6afe007507d11650a06 
>   src/master/master.hpp e77babf22126838c63cd05e483875c9beb3ac5ff 
>   src/master/master.cpp 9458ff10999d48e40a8596ec4edf1243591bc0d4 
>   src/master/quota_handler.cpp a4975fd50a2be20f3af8bbcfa81255b5dcc64fed 
>   src/slave/http.cpp 816aed1607bb7d25851052662b6ffa306c62b983 
>   src/slave/slave.hpp 0bd340176e2a8cefdfa7ef71e059441fb171aff6 
>   src/slave/slave.cpp 74f6fb9036a9ac4f587f53ec2df04eeb4c167bfb 
> 
> 
> Diff: https://reviews.apache.org/r/69385/diff/1/
> 
> 
> Testing
> ---
> 
> `make check` and internal CI validation
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 69389: Added a test `ROOT_UNPRIVILEGED_USER_SandboxOwnership`.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 69389 was successfully built and tested.

Reviews applied: `['69389']`

All the build artifacts available at: 
http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2619/mesos-review-69389

- Mesos Reviewbot Windows


On Nov. 19, 2018, 7:33 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69389/
> ---
> 
> (Updated Nov. 19, 2018, 7:33 a.m.)
> 
> 
> Review request for mesos and Gilbert Song.
> 
> 
> Bugs: MESOS-9332
> https://issues.apache.org/jira/browse/MESOS-9332
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test `ROOT_UNPRIVILEGED_USER_SandboxOwnership`.
> 
> 
> Diffs
> -
> 
>   src/tests/default_executor_tests.cpp 
> f23cb07fbdc9dac0acf75714c835e0eedd7dce6f 
> 
> 
> Diff: https://reviews.apache.org/r/69389/diff/1/
> 
> 
> Testing
> ---
> 
> Ran this test repeatedly, all succeeded.
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 69377: Added blog post for Mesos Mini.

[Joerg Schad]

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




site/source/blog/2018-11-19-mesos-mini.md
Lines 12 (patched)


s/Mesos/Mesos and Marathon ?



site/source/blog/2018-11-19-mesos-mini.md
Lines 16 (patched)


Should we explicitly call out limitations?



site/source/blog/2018-11-19-mesos-mini.md
Lines 40 (patched)


Are there configuration options for more agents?



site/source/blog/2018-11-19-mesos-mini.md
Lines 79 (patched)


Does it make sense to give an example for the date format used?



site/source/blog/2018-11-19-mesos-mini.md
Lines 103 (patched)


s/we embed Docker/we embed a Docker Daemon



site/source/blog/2018-11-19-mesos-mini.md
Lines 111 (patched)


when *the* Mesos Docker container finishes.


- Joerg Schad


On Nov. 19, 2018, 5:47 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69377/
> ---
> 
> (Updated Nov. 19, 2018, 5:47 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Chun-Hung Hsiao, Greg Mann, Jason 
> Lai, James DeFelice, James Peach, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added blog post for Mesos Mini.
> 
> 
> Diffs
> -
> 
>   site/source/blog/2018-11-19-mesos-mini.md PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/69377/diff/3/
> 
> 
> Testing
> ---
> 
> Markdown rendering can be viewed here:
> https://github.com/jieyu/mesos/blob/mesos_mini_blog/site/source/blog/2018-11-19-mesos-mini.md
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 69377: Added blog post for Mesos Mini.

[Joerg Schad]

> On Nov. 19, 2018, 8:02 a.m., Joerg Schad wrote:
> >

Excited about this, just some nits!


- Joerg


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


On Nov. 19, 2018, 5:47 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69377/
> ---
> 
> (Updated Nov. 19, 2018, 5:47 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Chun-Hung Hsiao, Greg Mann, Jason 
> Lai, James DeFelice, James Peach, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added blog post for Mesos Mini.
> 
> 
> Diffs
> -
> 
>   site/source/blog/2018-11-19-mesos-mini.md PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/69377/diff/3/
> 
> 
> Testing
> ---
> 
> Markdown rendering can be viewed here:
> https://github.com/jieyu/mesos/blob/mesos_mini_blog/site/source/blog/2018-11-19-mesos-mini.md
> 
> 
> Thanks,
> 
> Jie Yu
> 
>