Re: Review Request 33452: Fixed the python bindings to use implicit acknoweldgements by default.

[Alexander Rojas]

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

Ship it!


Ship It!

- Alexander Rojas


On April 22, 2015, 11:53 p.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33452/
> ---
> 
> (Updated April 22, 2015, 11:53 p.m.)
> 
> 
> Review request for mesos, Alexander Rojas and Vinod Kone.
> 
> 
> Bugs: MESOS-2643
> https://issues.apache.org/jira/browse/MESOS-2643
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See [MESOS-2643](https://issues.apache.org/jira/browse/MESOS-2643) and [this 
> thread](http://www.mail-archive.com/dev@mesos.apache.org/msg32291.html) for 
> the context.
> 
> 
> Diffs
> -
> 
>   src/python/native/src/mesos/native/mesos_scheduler_driver_impl.cpp 
> b0a5ef49ba7ff7cf52d2ec649045a48f7f692014 
> 
> Diff: https://reviews.apache.org/r/33452/diff/
> 
> 
> Testing
> ---
> 
> Tested manually through `test_framework.py`.
> 
> I removed the explicit argument passing and ran with and without this patch 
> to ensure the default is picked up correctly.
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 33109: Allow setting environment variables in mesos-execute

[Adam B]

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


Looks great! Just a few style/naming nits and then I think we're ready to 
ShipIt!


src/cli/execute.cpp
<https://reviews.apache.org/r/33109/#comment131582>

s/tmpEnv/environment_/



src/cli/execute.cpp
<https://reviews.apache.org/r/33109/#comment131583>

s/envVar/environmentVariable/



src/cli/execute.cpp
<https://reviews.apache.org/r/33109/#comment131584>

Was this namespacing change necessary?



src/cli/execute.cpp
<https://reviews.apache.org/r/33109/#comment131581>

Let's call this 'environment' in our variable names, to be more 
explicit/descriptive. We tend to dislike abbrevs. We can keep `--env` though 
because that's easier for the cli user.



src/cli/execute.cpp
<https://reviews.apache.org/r/33109/#comment131585>

s/env/environment/



src/common/parse.hpp
<https://reviews.apache.org/r/33109/#comment131579>

What happens if the same variable is listed multiple times? Probably worth 
calling out somewhere (maybe just a comment here, and/or in the flag itself) 
that only the first/last value will be used.


- Adam B


On April 22, 2015, 10:38 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33109/
> ---
> 
> (Updated April 22, 2015, 10:38 a.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Bugs: MESOS-2023
> https://issues.apache.org/jira/browse/MESOS-2023
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allow setting environment variables in mesos-execute
> 
> 
> Diffs
> -
> 
>   include/mesos/type_utils.hpp cdf5864389a72002b538c263d70bcade2bdffa45 
>   src/cli/execute.cpp 84f70dccbc2c5dd43f68105d967f4488c82f582b 
>   src/common/parse.hpp 547b32041f39f0ff0c38179b66a32b2239134abc 
> 
> Diff: https://reviews.apache.org/r/33109/diff/
> 
> 
> Testing
> ---
> 
> ### Test without env
> 
> ```
> $ ./src/mesos-execute  --master=127.0.0.1:5050 --name=test --command='echo $a'
> ```
> 
> The log from mesos, we could see the output is empty
> 
> ```
> Registered executor on localhostI0423 01:27:37.180140 14061 slave.cpp:1540] 
> Sending queued task 'test' to executor 'test' of framework 
> 20150423-012731-16777343-5050-14024-
> Starting task test
> Forked command at 14133
> sh -c 'echo $a'
> 
> I0423 01:27:37.183823 14064 slave.cpp:2507] Handling status update 
> TASK_RUNNING (UUID: be8664ab-a4ca-409e-87e4-de3756506c4a) for task test of 
> framework 20150423-012731-16777343-5050-14024- from 
> executor(1)@127.0.0.1:33700
> ```
> 
> ### Test with env from command
> 
> ```
> $ ./src/mesos-execute  --master=127.0.0.1:5050 --name=test --command='echo 
> $a' --env="{\"a\": \"stdin\"}"
> ```
> 
> The log from mesos, we could see the output is "stdin"
> 
> ```
> Registered executor on localhost
> Starting task test
> sh -c 'echo $a'
> Forked command at 14783
> "stdin"
> I0423 01:30:41.750128 14063 slave.cpp:2507] Handling status update 
> TASK_RUNNING (UUID: e8b6cf52-9635-4393-94de-b16869e50c91) for task test of 
> framework 20150423-012731-16777343-5050-14024-0001 from 
> executor(1)@127.0.0.1:40051
> ```
> 
> ### Test with env from file
> 
> ```
> $ cat /tmp/env
> {"a": "file"}
> ```
> 
> ```
> $ ./src/mesos-execute  --master=127.0.0.1:5050 --name=test --command='echo 
> $a' --env="file:///tmp/env"
> ```
> 
> The log from mesos, we could see the output is "file"
> 
> ```
> Registered executor on localhost
> Starting task testsh -c 'echo $a'
> Forked command at 15258
> "file"
> I0423 01:32:49.817705 14065 slave.cpp:2507] Handling status update 
> TASK_RUNNING (UUID: 9e8945d0-4921-41e8-a8e2-d64ec36eea16) for task test of 
> framework 20150423-012731-16777343-5050-14024-0002 from 
> executor(1)@127.0.0.1:56334
> ```
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 33358: Moved implementation of StatusUpdateStream to a compilation unit.

[Alexander Rojas]

> On April 22, 2015, 11:24 a.m., Alexander Rukletsov wrote:
> > Let's not repeat summary in description. How about the following:
> > "StatusUpdateStream is not a template, hence we can reduce compilation time 
> > by moving method definitions into a `.cpp` file. As a drive-by change 
> > headers are cleaned up."
> > 
> > Thank you for putting effort into reducing compilation time!
> 
> Ben Mahler wrote:
> > Thank you for putting effort into reducing compilation time!
> 
> Was it measured? :)

I did measure it, but status_update_manager is not widely included, so in 
practice there was not really performance gain. Still I think it's not only 
about compilation time, but reducing the ammount of inlined code.


- Alexander


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


On April 22, 2015, 11:09 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33358/
> ---
> 
> (Updated April 22, 2015, 11:09 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Joerg Schad, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-2609
> https://issues.apache.org/jira/browse/MESOS-2609
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Moves the implementation of StatusUpdateStream to a compilation unit.
> Also cleans up headers.
> 
> 
> Diffs
> -
> 
>   src/slave/status_update_manager.hpp 
> b4d91b22b515195fdb69c89af9c2864e469e7e54 
>   src/slave/status_update_manager.cpp 
> fab8c22d46b8ab0a3c3745541ddc650b574bfbd4 
> 
> Diff: https://reviews.apache.org/r/33358/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 33257: Fixed recover tasks only by the intiated containerizer.

[Adam B]

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


Looks reasonable to me, but I'm no containerizer expert.


src/slave/containerizer/docker.cpp


Why can this be removed in 0.24? Why 0.24?
Can you create a JIRA and target it to 0.24 so we don't forget?



src/slave/containerizer/docker.cpp


`continue` seems unnecessary for this tiny loop. How about:
```cpp
if (id.isSome()) {
  existingContainers.insert(id.get());
}
```



src/slave/containerizer/docker.cpp


s/it/its/



src/slave/slave.cpp


s/recovering/to recover/


- Adam B


On April 20, 2015, 2:18 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33257/
> ---
> 
> (Updated April 20, 2015, 2:18 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, Ian Downes, Jie 
> Yu, and Till Toenshoff.
> 
> 
> Bugs: MESOS-2601
> https://issues.apache.org/jira/browse/MESOS-2601
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed recover tasks only by the intiated containerizer.
> Currently both mesos and docker containerizer recovers tasks that wasn't 
> started by themselves.
> The proposed fix is to record the intended containerizer in the checkpointed 
> executorInfo, and reuse that information on recover to know if the 
> containerizer should recover or not. We are free to modify the executorInfo 
> since it's not being used to relaunch any task.
> The external containerizer doesn't need to change since it is only recovering 
> containers that are returned by the containers script.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.hpp 0d5289c626bdf22420759485f2146abfb6bdaffc 
>   src/slave/containerizer/docker.cpp f9fb07806e3b7d7d2afc1be3b8756eac23b32dcd 
>   src/slave/containerizer/mesos/containerizer.cpp 
> e4136095fca55637864f495098189ab3ad8d8fe7 
>   src/slave/slave.cpp 8ec80ed26f338690e0a1e712065750ab77a724cd 
>   src/tests/containerizer_tests.cpp 5991aa628083dac7c5e8bf7ba297f4f9edeec05f 
>   src/tests/docker_containerizer_tests.cpp 
> b119a174de79c2f98a0c575e6be2f59649f509ef 
> 
> Diff: https://reviews.apache.org/r/33257/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 32140: Enabled 'Resources' to handle 'Resource::ReservationInfo'.

[Alexander Rukletsov]

> On April 15, 2015, 5:36 p.m., Alexander Rukletsov wrote:
> > src/common/resources.cpp, lines 93-105
> > 
> >
> > Can we replace it by
> > ```
> > if !addable(left, right) {
> >   return false;
> > }
> > ```
> > ? Or even add `addable()` check to the chain of checks below?
> 
> Michael Park wrote:
> Each of `operator==`, `addable` and `subtractable` has subtle differences.
> 
> `operator==` and `addable` in specific are different in that equal 
> persistent volumes (same persistence ID) are not addable.
> Here's Jie's comment:
> 
> ```
> // TODO(jieyu): Even if two Resource objects with DiskInfo have the
> // same persistence ID, they cannot be added together. In fact, this
> // shouldn't happen if we do not add resources from different
> // namespaces (e.g., across slave). Consider adding a warning.
> ```

Got it.


> On April 15, 2015, 5:36 p.m., Alexander Rukletsov wrote:
> > src/tests/mesos.hpp, lines 376-378
> > 
> >
> > Why not putting the definition into `tests/mesos.cpp`? Here and below.
> 
> Michael Park wrote:
> I kept it consistent with other utility functions that live in this file: 
> `createTask`, `createDiskInfo`, `createPersistentVolume`. I think we can fix 
> this as a batch in a separate patch.

Mind creating a JIRA?


- Alexander


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


On April 22, 2015, 8:36 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32140/
> ---
> 
> (Updated April 22, 2015, 8:36 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
> 
> 
> Bugs: MESOS-2476
> https://issues.apache.org/jira/browse/MESOS-2476
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `Resource::ReservationInfo` was introduced in 
> [r32139](https://reviews.apache.org/r/32139). We need to consider it in our 
> `Resources` class implementation.
> 
> ### `Resources::flatten`
> 
> `flatten` is used as the utility function to cross reservation boundaries 
> without affecting the given resources. Since the reservation is now specified 
> by the (`role`, `reservation`) pair, `flatten` needs to consider 
> `ReservationInfo` as well.
> 
> ### `Resources::validate`
> 
> If `role == "*"`, then `reservation` field must not be set.
> 
> ### `Resources` comparators
> 
> `operator==`, `addable` and `substractable` need to test for 
> `ReservationInfo` as well.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 56affd45e1e71e96ff5778b43271f81b9b9a9e4d 
>   src/common/resources.cpp 2c99b6884d7296099e19e2e3182cbe11b5e1e059 
>   src/tests/mesos.hpp 4294e28f904161b473c0cfa7feea4eaa4e7c97e3 
>   src/tests/resources_tests.cpp 7e0ad98c3366f647f190363a0e6b576dbfc7d415 
> 
> Diff: https://reviews.apache.org/r/32140/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 33109: Allow setting environment variables in mesos-execute

[haosdent huang]

> On April 23, 2015, 7:25 a.m., Adam B wrote:
> > src/cli/execute.cpp, line 249
> > <https://reviews.apache.org/r/33109/diff/5/?file=939687#file939687line249>
> >
> > Was this namespacing change necessary?

Because when we add "common/parse.hpp" in the headers. And we has 
"" in "parse.hpp", so protobuf namespace are confilct here.


- haosdent


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


On April 22, 2015, 5:38 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33109/
> ---
> 
> (Updated April 22, 2015, 5:38 p.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Bugs: MESOS-2023
> https://issues.apache.org/jira/browse/MESOS-2023
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allow setting environment variables in mesos-execute
> 
> 
> Diffs
> -
> 
>   include/mesos/type_utils.hpp cdf5864389a72002b538c263d70bcade2bdffa45 
>   src/cli/execute.cpp 84f70dccbc2c5dd43f68105d967f4488c82f582b 
>   src/common/parse.hpp 547b32041f39f0ff0c38179b66a32b2239134abc 
> 
> Diff: https://reviews.apache.org/r/33109/diff/
> 
> 
> Testing
> ---
> 
> ### Test without env
> 
> ```
> $ ./src/mesos-execute  --master=127.0.0.1:5050 --name=test --command='echo $a'
> ```
> 
> The log from mesos, we could see the output is empty
> 
> ```
> Registered executor on localhostI0423 01:27:37.180140 14061 slave.cpp:1540] 
> Sending queued task 'test' to executor 'test' of framework 
> 20150423-012731-16777343-5050-14024-
> Starting task test
> Forked command at 14133
> sh -c 'echo $a'
> 
> I0423 01:27:37.183823 14064 slave.cpp:2507] Handling status update 
> TASK_RUNNING (UUID: be8664ab-a4ca-409e-87e4-de3756506c4a) for task test of 
> framework 20150423-012731-16777343-5050-14024- from 
> executor(1)@127.0.0.1:33700
> ```
> 
> ### Test with env from command
> 
> ```
> $ ./src/mesos-execute  --master=127.0.0.1:5050 --name=test --command='echo 
> $a' --env="{\"a\": \"stdin\"}"
> ```
> 
> The log from mesos, we could see the output is "stdin"
> 
> ```
> Registered executor on localhost
> Starting task test
> sh -c 'echo $a'
> Forked command at 14783
> "stdin"
> I0423 01:30:41.750128 14063 slave.cpp:2507] Handling status update 
> TASK_RUNNING (UUID: e8b6cf52-9635-4393-94de-b16869e50c91) for task test of 
> framework 20150423-012731-16777343-5050-14024-0001 from 
> executor(1)@127.0.0.1:40051
> ```
> 
> ### Test with env from file
> 
> ```
> $ cat /tmp/env
> {"a": "file"}
> ```
> 
> ```
> $ ./src/mesos-execute  --master=127.0.0.1:5050 --name=test --command='echo 
> $a' --env="file:///tmp/env"
> ```
> 
> The log from mesos, we could see the output is "file"
> 
> ```
> Registered executor on localhost
> Starting task testsh -c 'echo $a'
> Forked command at 15258
> "file"
> I0423 01:32:49.817705 14065 slave.cpp:2507] Handling status update 
> TASK_RUNNING (UUID: 9e8945d0-4921-41e8-a8e2-d64ec36eea16) for task test of 
> framework 20150423-012731-16777343-5050-14024-0002 from 
> executor(1)@127.0.0.1:56334
> ```
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 33358: Moved implementation of StatusUpdateStream to a compilation unit.

[Till Toenshoff]

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

Ship it!


Ship It!

- Till Toenshoff


On April 22, 2015, 9:09 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33358/
> ---
> 
> (Updated April 22, 2015, 9:09 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Joerg Schad, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-2609
> https://issues.apache.org/jira/browse/MESOS-2609
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Moves the implementation of StatusUpdateStream to a compilation unit.
> Also cleans up headers.
> 
> 
> Diffs
> -
> 
>   src/slave/status_update_manager.hpp 
> b4d91b22b515195fdb69c89af9c2864e469e7e54 
>   src/slave/status_update_manager.cpp 
> fab8c22d46b8ab0a3c3745541ddc650b574bfbd4 
> 
> Diff: https://reviews.apache.org/r/33358/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 33295: Added firewall mechanism to control access on libprocess http endpoints.

[Till Toenshoff]

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


This review was pending a couple of days in my outbox, hence it may be 
partially or entirely invalid by now given that you had updated the RR - sry 
for that. Feel free to drop any outdated comments without further ado.


3rdparty/libprocess/include/process/firewall.hpp


That is the wrong header, we have to use the "Apache License V.2" header 
for stout and libprocess. The "ASF" header you are using is for mesos only.

The correct header would be:
```
/**
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 * http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License
 */
```



3rdparty/libprocess/include/process/firewall.hpp


I forgot the verbally proposed name for this function, but maybe a negated 
result and the name `reject()` would fit better.



3rdparty/libprocess/include/process/firewall.hpp


Even though this is implementation is tiny, would it still make sense to 
move it into a cpp-file?



3rdparty/libprocess/include/process/process.hpp


Could you elaborate on the term "__top__ firewall rule"?



3rdparty/libprocess/include/process/process.hpp


The doxygen rendered output IMHO looks better when using a capital letter 
on the start of description -- however, it seems the other argument 
descriptions start with a lower case.



3rdparty/libprocess/src/process.cpp


`<< "': firewall rule forbids connection";` might be a bit more mesos'esque.



3rdparty/libprocess/src/tests/process_tests.cpp


const string



3rdparty/libprocess/src/tests/process_tests.cpp


const string



3rdparty/libprocess/src/process.cpp


Given that you simply take a copy of the firewallRule instance var, the 
"right" name would be `firewallRule_`, no?


- Till Toenshoff


On April 22, 2015, 2:35 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33295/
> ---
> 
> (Updated April 22, 2015, 2:35 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, and Till Toenshoff.
> 
> 
> Bugs: MESOS-2620
> https://issues.apache.org/jira/browse/MESOS-2620
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduces the interface `FirewallRule` which will be matched against 
> incoming connections in order to allow them to be served or being blocked. 
> For details, check the [design 
> doc](https://docs.google.com/document/d/1JSJTJMJ6ZXLkCSmvOIabTLrjtqqr0E-u99Rx2BHR1hs/edit).
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/Makefile.am 
> 3da3e6cef1b5cb66c223425744d84741846ea730 
>   3rdparty/libprocess/include/process/firewall.hpp PRE-CREATION 
>   3rdparty/libprocess/include/process/process.hpp 
> 392c74df3e8a122aecd3633dffdeec4bcbd1f097 
>   3rdparty/libprocess/src/process.cpp 
> 97ac09fd10b767bc46387abc3657d005a00830c8 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> eb38edce2c483ba7f963a826893a15a075238618 
> 
> Diff: https://reviews.apache.org/r/33295/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 29748: Added tests for dynamic reservation.

[Michael Park]

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


As of now, the tests seem to take a long time to complete. We should 
investigate what the issue is before committing this patch.

- Michael Park


On April 8, 2015, 5:05 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29748/
> ---
> 
> (Updated April 8, 2015, 5:05 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
> 
> 
> Bugs: MESOS-2489
> https://issues.apache.org/jira/browse/MESOS-2489
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 9c01f5d6c692f835100e7cade928748cc4763cc8 
>   src/tests/reservation_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/29748/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 29748: Added tests for dynamic reservation.

[Alexander Rukletsov]

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


How about a test trying to unreserve statically reserved resources? Let's 
document the expected behaviour explicitly in the test.


src/tests/reservation_tests.cpp


How about we use a single resource string for clarity? Here we start a 
slave with `"cpus:1;mem:512;disk:0;ports:[]"` resources, while expect 
`"cpus:1;mem:512"` in the offer.



src/tests/reservation_tests.cpp


The default for `Flags::allocation_interval` is 1s, but I hope the test is 
faster, right? Why do we get the next offer earlier than after 1s?



src/tests/reservation_tests.cpp


... and decline it.



src/tests/reservation_tests.cpp


Isn't setting this param to 0 tells master that framework1 can be offered 
the same resource immediately? If you want framework2 to get the offer, this 
looks flaky.



src/tests/reservation_tests.cpp


... and reserve it for a different role.



src/tests/reservation_tests.cpp


I'm not sure what is the preferred way of doing this in our codebase, but I 
would rather prefer having "role1" and "role2" defined in the beginning of the 
test with descriptive names, e.g.
```
const std::string frameworkRole = "role1";
const std::string reservationRole = "role2";
```



src/tests/reservation_tests.cpp


Mind leaving a comment here or above why updateAllocation should not be 
called? Like you do in the next test.



src/tests/reservation_tests.cpp


// In the next offer, still expect an offer with the unreserved resources.



src/tests/reservation_tests.cpp


Mind leaving a comment that we have a default credential set there and it 
is different to the one we are going to use to reserve?



src/tests/reservation_tests.cpp


Again, let's move it to the beginning, closer to `DEFAULT_FRAMEWORK_INFO`.



src/tests/reservation_tests.cpp


Mind leaving a comment here or above why updateAllocation should not be 
called? Like you do in the next test.


- Alexander Rukletsov


On April 8, 2015, 5:05 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29748/
> ---
> 
> (Updated April 8, 2015, 5:05 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
> 
> 
> Bugs: MESOS-2489
> https://issues.apache.org/jira/browse/MESOS-2489
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 9c01f5d6c692f835100e7cade928748cc4763cc8 
>   src/tests/reservation_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/29748/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 33296: Added a flag which controls libprocess firewall initialzation.

[Till Toenshoff]

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



include/mesos/mesos.proto


Check the comment formatting, please.



include/mesos/mesos.proto


Might be a good idea to add a comment on why this message name is using 
plural even though it currently contains only a single rule.



include/mesos/mesos.proto


I am unsure if `include/mesos.proto` is the _right_ place for this given 
that this message does not need to get exposed. As this forms the base of more 
definitions to come, I would like to propose adding a new .proto file 
specifically for this. How about using  `src/messages/firewall.proto` & 
`src/messages/firewall.hpp` with the package / namespace 
`mesos.internal.firewall` and use `Rules` as the name instead?

So now we have `process::FirewallRule` and `mesos::FirewallRules` (proto 
message). That seems a bit unfortunate, especially due to the fact hat 
`FirewallRules` is __not__ composed by repeated `FirewallRule`.
How about `mesos::internal::firewall::Rules` (proto message) and 
`process::FirewallRule` instead? Note however that I feel rather unsure about 
the best way here.



src/master/flags.cpp


s/FirewallRule/FirewallRules/



src/master/main.cpp





- Till Toenshoff


On April 22, 2015, 2:36 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33296/
> ---
> 
> (Updated April 22, 2015, 2:36 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, and Till Toenshoff.
> 
> 
> Bugs: MESOS-2620
> https://issues.apache.org/jira/browse/MESOS-2620
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 3a8e8bf303e0576c212951f6028af77e54d93537 
>   include/mesos/type_utils.hpp cdf5864389a72002b538c263d70bcade2bdffa45 
>   src/common/parse.hpp 547b32041f39f0ff0c38179b66a32b2239134abc 
>   src/master/flags.hpp 996cf38c88f9718e55e88d6e906b5e3d1989478a 
>   src/master/flags.cpp 5798989d3f135978ec3d5f714b1cd8d84180fc90 
>   src/master/main.cpp 88bb39d57ea7d654bea11a3db0cf0903b4b22d99 
>   src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032 
>   src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932 
>   src/slave/main.cpp c62d3ab9825bf952071e8e312d383a0cb46547d2 
> 
> Diff: https://reviews.apache.org/r/33296/diff/
> 
> 
> Testing
> ---
> 
> make check and manual tests.
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 33109: Allow setting environment variables in mesos-execute

[Adam B]

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



include/mesos/type_utils.hpp
<https://reviews.apache.org/r/33109/#comment131657>

Double blank line between top-level methods, please.


- Adam B


On April 22, 2015, 10:38 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33109/
> ---
> 
> (Updated April 22, 2015, 10:38 a.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Bugs: MESOS-2023
> https://issues.apache.org/jira/browse/MESOS-2023
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allow setting environment variables in mesos-execute
> 
> 
> Diffs
> -
> 
>   include/mesos/type_utils.hpp cdf5864389a72002b538c263d70bcade2bdffa45 
>   src/cli/execute.cpp 84f70dccbc2c5dd43f68105d967f4488c82f582b 
>   src/common/parse.hpp 547b32041f39f0ff0c38179b66a32b2239134abc 
> 
> Diff: https://reviews.apache.org/r/33109/diff/
> 
> 
> Testing
> ---
> 
> ### Test without env
> 
> ```
> $ ./src/mesos-execute  --master=127.0.0.1:5050 --name=test --command='echo $a'
> ```
> 
> The log from mesos, we could see the output is empty
> 
> ```
> Registered executor on localhostI0423 01:27:37.180140 14061 slave.cpp:1540] 
> Sending queued task 'test' to executor 'test' of framework 
> 20150423-012731-16777343-5050-14024-
> Starting task test
> Forked command at 14133
> sh -c 'echo $a'
> 
> I0423 01:27:37.183823 14064 slave.cpp:2507] Handling status update 
> TASK_RUNNING (UUID: be8664ab-a4ca-409e-87e4-de3756506c4a) for task test of 
> framework 20150423-012731-16777343-5050-14024- from 
> executor(1)@127.0.0.1:33700
> ```
> 
> ### Test with env from command
> 
> ```
> $ ./src/mesos-execute  --master=127.0.0.1:5050 --name=test --command='echo 
> $a' --env="{\"a\": \"stdin\"}"
> ```
> 
> The log from mesos, we could see the output is "stdin"
> 
> ```
> Registered executor on localhost
> Starting task test
> sh -c 'echo $a'
> Forked command at 14783
> "stdin"
> I0423 01:30:41.750128 14063 slave.cpp:2507] Handling status update 
> TASK_RUNNING (UUID: e8b6cf52-9635-4393-94de-b16869e50c91) for task test of 
> framework 20150423-012731-16777343-5050-14024-0001 from 
> executor(1)@127.0.0.1:40051
> ```
> 
> ### Test with env from file
> 
> ```
> $ cat /tmp/env
> {"a": "file"}
> ```
> 
> ```
> $ ./src/mesos-execute  --master=127.0.0.1:5050 --name=test --command='echo 
> $a' --env="file:///tmp/env"
> ```
> 
> The log from mesos, we could see the output is "file"
> 
> ```
> Registered executor on localhost
> Starting task testsh -c 'echo $a'
> Forked command at 15258
> "file"
> I0423 01:32:49.817705 14065 slave.cpp:2507] Handling status update 
> TASK_RUNNING (UUID: 9e8945d0-4921-41e8-a8e2-d64ec36eea16) for task test of 
> framework 20150423-012731-16777343-5050-14024-0002 from 
> executor(1)@127.0.0.1:56334
> ```
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 33263: Extended SlaveTest.ShutdownUnregisteredExecutor test with a reason check.

[Till Toenshoff]

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

Ship it!


Ship It!

- Till Toenshoff


On April 16, 2015, 2:31 p.m., Andrey Dyatlov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33263/
> ---
> 
> (Updated April 16, 2015, 2:31 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-2625
> https://issues.apache.org/jira/browse/MESOS-2625
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Extended SlaveTest.ShutdownUnregisteredExecutor test with a reason check. 
> Check that the reason is REASON_COMMAND_EXECUTOR_FAILED. According to the 
> Slave::sendExecutorTerminatedStatusUpdate member function, this reason is 
> expected instead of more general REASON_EXECUTOR_TERMINATED because the 
> command executer is used in this test.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_tests.cpp b826000e0a4221690f956ea51f49ad4c99d5e188 
> 
> Diff: https://reviews.apache.org/r/33263/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Andrey Dyatlov
> 
>

Re: Review Request 32140: Enabled 'Resources' to handle 'Resource::ReservationInfo'.

[Michael Park]

> On April 15, 2015, 5:36 p.m., Alexander Rukletsov wrote:
> > src/tests/mesos.hpp, lines 376-378
> > 
> >
> > Why not putting the definition into `tests/mesos.cpp`? Here and below.
> 
> Michael Park wrote:
> I kept it consistent with other utility functions that live in this file: 
> `createTask`, `createDiskInfo`, `createPersistentVolume`. I think we can fix 
> this as a batch in a separate patch.
> 
> Alexander Rukletsov wrote:
> Mind creating a JIRA?

Added [MESOS-2658](https://issues.apache.org/jira/browse/MESOS-2658) as a 
subtask of [MESOS-1622](https://issues.apache.org/jira/browse/MESOS-1622).


- Michael


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


On April 22, 2015, 8:36 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32140/
> ---
> 
> (Updated April 22, 2015, 8:36 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
> 
> 
> Bugs: MESOS-2476
> https://issues.apache.org/jira/browse/MESOS-2476
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `Resource::ReservationInfo` was introduced in 
> [r32139](https://reviews.apache.org/r/32139). We need to consider it in our 
> `Resources` class implementation.
> 
> ### `Resources::flatten`
> 
> `flatten` is used as the utility function to cross reservation boundaries 
> without affecting the given resources. Since the reservation is now specified 
> by the (`role`, `reservation`) pair, `flatten` needs to consider 
> `ReservationInfo` as well.
> 
> ### `Resources::validate`
> 
> If `role == "*"`, then `reservation` field must not be set.
> 
> ### `Resources` comparators
> 
> `operator==`, `addable` and `substractable` need to test for 
> `ReservationInfo` as well.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 56affd45e1e71e96ff5778b43271f81b9b9a9e4d 
>   src/common/resources.cpp 2c99b6884d7296099e19e2e3182cbe11b5e1e059 
>   src/tests/mesos.hpp 4294e28f904161b473c0cfa7feea4eaa4e7c97e3 
>   src/tests/resources_tests.cpp 7e0ad98c3366f647f190363a0e6b576dbfc7d415 
> 
> Diff: https://reviews.apache.org/r/32140/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 32149: Enabled 'Resources::apply' to handle reservation operations.

[Michael Park]

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

(Updated April 23, 2015, 4:38 p.m.)


Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.


Changes
---

Added a test for `unreserve`.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/common/resources.cpp 2c99b6884d7296099e19e2e3182cbe11b5e1e059 
  src/tests/resources_tests.cpp 7e0ad98c3366f647f190363a0e6b576dbfc7d415 

Diff: https://reviews.apache.org/r/32149/diff/


Testing
---

make check


Thanks,

Michael Park

Re: Review Request 33295: Added firewall mechanism to control access on libprocess http endpoints.

[Adam B]

> On April 23, 2015, 5:49 a.m., Till Toenshoff wrote:
> > 3rdparty/libprocess/include/process/firewall.hpp, line 37
> > 
> >
> > I forgot the verbally proposed name for this function, but maybe a 
> > negated result and the name `reject()` would fit better.

Somebody suggested just using the () operator, aka the "apply" operator.


- Adam


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


On April 22, 2015, 7:35 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33295/
> ---
> 
> (Updated April 22, 2015, 7:35 a.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, and Till Toenshoff.
> 
> 
> Bugs: MESOS-2620
> https://issues.apache.org/jira/browse/MESOS-2620
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduces the interface `FirewallRule` which will be matched against 
> incoming connections in order to allow them to be served or being blocked. 
> For details, check the [design 
> doc](https://docs.google.com/document/d/1JSJTJMJ6ZXLkCSmvOIabTLrjtqqr0E-u99Rx2BHR1hs/edit).
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/Makefile.am 
> 3da3e6cef1b5cb66c223425744d84741846ea730 
>   3rdparty/libprocess/include/process/firewall.hpp PRE-CREATION 
>   3rdparty/libprocess/include/process/process.hpp 
> 392c74df3e8a122aecd3633dffdeec4bcbd1f097 
>   3rdparty/libprocess/src/process.cpp 
> 97ac09fd10b767bc46387abc3657d005a00830c8 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> eb38edce2c483ba7f963a826893a15a075238618 
> 
> Diff: https://reviews.apache.org/r/33295/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 33295: Added firewall mechanism to control access on libprocess http endpoints.

[Joris Van Remoortere]

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


Please move the `*` for pointers to the left as per our style guide:
change from `FirewallRule *rule` to `FirewallRule* rule`.

- Joris Van Remoortere


On April 22, 2015, 2:35 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33295/
> ---
> 
> (Updated April 22, 2015, 2:35 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, and Till Toenshoff.
> 
> 
> Bugs: MESOS-2620
> https://issues.apache.org/jira/browse/MESOS-2620
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduces the interface `FirewallRule` which will be matched against 
> incoming connections in order to allow them to be served or being blocked. 
> For details, check the [design 
> doc](https://docs.google.com/document/d/1JSJTJMJ6ZXLkCSmvOIabTLrjtqqr0E-u99Rx2BHR1hs/edit).
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/Makefile.am 
> 3da3e6cef1b5cb66c223425744d84741846ea730 
>   3rdparty/libprocess/include/process/firewall.hpp PRE-CREATION 
>   3rdparty/libprocess/include/process/process.hpp 
> 392c74df3e8a122aecd3633dffdeec4bcbd1f097 
>   3rdparty/libprocess/src/process.cpp 
> 97ac09fd10b767bc46387abc3657d005a00830c8 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> eb38edce2c483ba7f963a826893a15a075238618 
> 
> Diff: https://reviews.apache.org/r/33295/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 33295: Added firewall mechanism to control access on libprocess http endpoints.

[Michael Park]

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



3rdparty/libprocess/src/process.cpp


In other parts of our codebase, we simply perform the `initialize` rather 
than just return for functions that require initialization to have happened. I 
think we should keep that behavior consistent here.


- Michael Park


On April 22, 2015, 2:35 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33295/
> ---
> 
> (Updated April 22, 2015, 2:35 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, and Till Toenshoff.
> 
> 
> Bugs: MESOS-2620
> https://issues.apache.org/jira/browse/MESOS-2620
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduces the interface `FirewallRule` which will be matched against 
> incoming connections in order to allow them to be served or being blocked. 
> For details, check the [design 
> doc](https://docs.google.com/document/d/1JSJTJMJ6ZXLkCSmvOIabTLrjtqqr0E-u99Rx2BHR1hs/edit).
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/Makefile.am 
> 3da3e6cef1b5cb66c223425744d84741846ea730 
>   3rdparty/libprocess/include/process/firewall.hpp PRE-CREATION 
>   3rdparty/libprocess/include/process/process.hpp 
> 392c74df3e8a122aecd3633dffdeec4bcbd1f097 
>   3rdparty/libprocess/src/process.cpp 
> 97ac09fd10b767bc46387abc3657d005a00830c8 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> eb38edce2c483ba7f963a826893a15a075238618 
> 
> Diff: https://reviews.apache.org/r/33295/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 33296: Added a flag which controls libprocess firewall initialzation.

[Joerg Schad]

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



src/master/flags.cpp


Could you add these flags also to the documentation? I.e. 
http://mesos.apache.org/documentation/latest/configuration/



src/slave/flags.cpp


Please add to documentation 
http://mesos.apache.org/documentation/latest/configuration/


- Joerg Schad


On April 22, 2015, 2:36 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33296/
> ---
> 
> (Updated April 22, 2015, 2:36 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, and Till Toenshoff.
> 
> 
> Bugs: MESOS-2620
> https://issues.apache.org/jira/browse/MESOS-2620
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 3a8e8bf303e0576c212951f6028af77e54d93537 
>   include/mesos/type_utils.hpp cdf5864389a72002b538c263d70bcade2bdffa45 
>   src/common/parse.hpp 547b32041f39f0ff0c38179b66a32b2239134abc 
>   src/master/flags.hpp 996cf38c88f9718e55e88d6e906b5e3d1989478a 
>   src/master/flags.cpp 5798989d3f135978ec3d5f714b1cd8d84180fc90 
>   src/master/main.cpp 88bb39d57ea7d654bea11a3db0cf0903b4b22d99 
>   src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032 
>   src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932 
>   src/slave/main.cpp c62d3ab9825bf952071e8e312d383a0cb46547d2 
> 
> Diff: https://reviews.apache.org/r/33296/diff/
> 
> 
> Testing
> ---
> 
> make check and manual tests.
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 33296: Added a flag which controls libprocess firewall initialzation.

[Michael Park]

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



src/common/parse.hpp


nit: `s/template<>/template <>/`



src/common/parse.hpp


nit: `s/template<>/template <>/`


- Michael Park


On April 22, 2015, 2:36 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33296/
> ---
> 
> (Updated April 22, 2015, 2:36 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, and Till Toenshoff.
> 
> 
> Bugs: MESOS-2620
> https://issues.apache.org/jira/browse/MESOS-2620
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 3a8e8bf303e0576c212951f6028af77e54d93537 
>   include/mesos/type_utils.hpp cdf5864389a72002b538c263d70bcade2bdffa45 
>   src/common/parse.hpp 547b32041f39f0ff0c38179b66a32b2239134abc 
>   src/master/flags.hpp 996cf38c88f9718e55e88d6e906b5e3d1989478a 
>   src/master/flags.cpp 5798989d3f135978ec3d5f714b1cd8d84180fc90 
>   src/master/main.cpp 88bb39d57ea7d654bea11a3db0cf0903b4b22d99 
>   src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032 
>   src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932 
>   src/slave/main.cpp c62d3ab9825bf952071e8e312d383a0cb46547d2 
> 
> Diff: https://reviews.apache.org/r/33296/diff/
> 
> 
> Testing
> ---
> 
> make check and manual tests.
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 33295: Added firewall mechanism to control access on libprocess http endpoints.

[Benjamin Hindman]

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



3rdparty/libprocess/include/process/firewall.hpp


Can we add some documentation for why this exists, how it's going to get 
called, what are it's semantics (around 'apply in particular), etc please?



3rdparty/libprocess/include/process/firewall.hpp


How about adding parameter names?

Who wants to take this on in our style guide?



3rdparty/libprocess/include/process/firewall.hpp


s/DisabledEndpointsRule/DisabledEndpointsFirewallRule/



3rdparty/libprocess/include/process/firewall.hpp


virtual



3rdparty/libprocess/include/process/process.hpp


void install(const std::vector& rules);



3rdparty/libprocess/src/process.cpp


s/Firewall */Firewall* /

(Here and everywhere else please!)



3rdparty/libprocess/src/process.cpp


s/synchronized(/synchronized (/

(Here and everywhere else please!)



3rdparty/libprocess/src/tests/process_tests.cpp


s/ &/& /



3rdparty/libprocess/src/tests/process_tests.cpp


Extra newline please.


- Benjamin Hindman


On April 22, 2015, 2:35 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33295/
> ---
> 
> (Updated April 22, 2015, 2:35 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, and Till Toenshoff.
> 
> 
> Bugs: MESOS-2620
> https://issues.apache.org/jira/browse/MESOS-2620
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduces the interface `FirewallRule` which will be matched against 
> incoming connections in order to allow them to be served or being blocked. 
> For details, check the [design 
> doc](https://docs.google.com/document/d/1JSJTJMJ6ZXLkCSmvOIabTLrjtqqr0E-u99Rx2BHR1hs/edit).
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/Makefile.am 
> 3da3e6cef1b5cb66c223425744d84741846ea730 
>   3rdparty/libprocess/include/process/firewall.hpp PRE-CREATION 
>   3rdparty/libprocess/include/process/process.hpp 
> 392c74df3e8a122aecd3633dffdeec4bcbd1f097 
>   3rdparty/libprocess/src/process.cpp 
> 97ac09fd10b767bc46387abc3657d005a00830c8 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> eb38edce2c483ba7f963a826893a15a075238618 
> 
> Diff: https://reviews.apache.org/r/33295/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 33296: Added a flag which controls libprocess firewall initialzation.

[Benjamin Hindman]

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



src/common/parse.hpp


s/template<>/template <>/

(Here and everywhere else please!)



src/master/main.cpp


const?


- Benjamin Hindman


On April 22, 2015, 2:36 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33296/
> ---
> 
> (Updated April 22, 2015, 2:36 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, and Till Toenshoff.
> 
> 
> Bugs: MESOS-2620
> https://issues.apache.org/jira/browse/MESOS-2620
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 3a8e8bf303e0576c212951f6028af77e54d93537 
>   include/mesos/type_utils.hpp cdf5864389a72002b538c263d70bcade2bdffa45 
>   src/common/parse.hpp 547b32041f39f0ff0c38179b66a32b2239134abc 
>   src/master/flags.hpp 996cf38c88f9718e55e88d6e906b5e3d1989478a 
>   src/master/flags.cpp 5798989d3f135978ec3d5f714b1cd8d84180fc90 
>   src/master/main.cpp 88bb39d57ea7d654bea11a3db0cf0903b4b22d99 
>   src/slave/flags.hpp d3b1ce117fbb4e0b97852ef150b63f35cc991032 
>   src/slave/flags.cpp d0932b04e3825abb6173efe0d1aee199aa356932 
>   src/slave/main.cpp c62d3ab9825bf952071e8e312d383a0cb46547d2 
> 
> Diff: https://reviews.apache.org/r/33296/diff/
> 
> 
> Testing
> ---
> 
> make check and manual tests.
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 32140: Enabled 'Resources' to handle 'Resource::ReservationInfo'.

[Jie Yu]

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



src/common/resources.cpp


The DiskInfo should match at this point. So
```
if (left.has_disk() && left.disk().has_persistence())
```
should be sufficient.



src/common/resources.cpp


See my comments in https://reviews.apache.org/r/32150/

Can we move this to master validation? Thoughts?



src/tests/resources_tests.cpp


Move this to src/tests/mesos.hpp close to createPersistentVolume?



src/tests/resources_tests.cpp


Nits. Just to be consistent, please use the same style:
```
EXPECT_NONE(Resources::validate(createReservedResource(
"cpus", ..., ..., ...));
```



src/tests/resources_tests.cpp


Please move '{' to the next line.



src/tests/resources_tests.cpp


Ditto.



src/tests/resources_tests.cpp


Mind splitting into two tests?

AdditionStaticallyReserved
AdditionDynamicallyReserved



src/tests/resources_tests.cpp


Ditto.


- Jie Yu


On April 22, 2015, 8:36 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32140/
> ---
> 
> (Updated April 22, 2015, 8:36 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
> 
> 
> Bugs: MESOS-2476
> https://issues.apache.org/jira/browse/MESOS-2476
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `Resource::ReservationInfo` was introduced in 
> [r32139](https://reviews.apache.org/r/32139). We need to consider it in our 
> `Resources` class implementation.
> 
> ### `Resources::flatten`
> 
> `flatten` is used as the utility function to cross reservation boundaries 
> without affecting the given resources. Since the reservation is now specified 
> by the (`role`, `reservation`) pair, `flatten` needs to consider 
> `ReservationInfo` as well.
> 
> ### `Resources::validate`
> 
> If `role == "*"`, then `reservation` field must not be set.
> 
> ### `Resources` comparators
> 
> `operator==`, `addable` and `substractable` need to test for 
> `ReservationInfo` as well.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 56affd45e1e71e96ff5778b43271f81b9b9a9e4d 
>   src/common/resources.cpp 2c99b6884d7296099e19e2e3182cbe11b5e1e059 
>   src/tests/mesos.hpp 4294e28f904161b473c0cfa7feea4eaa4e7c97e3 
>   src/tests/resources_tests.cpp 7e0ad98c3366f647f190363a0e6b576dbfc7d415 
> 
> Diff: https://reviews.apache.org/r/32140/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 29748: Added tests for dynamic reservation.

[Jie Yu]

> On April 23, 2015, 1:48 p.m., Michael Park wrote:
> > As of now, the tests seem to take a long time to complete. We should 
> > investigate what the issue is before committing this patch.

I suspect this is due to the default allocation interval (1 secs by default).


- Jie


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


On April 8, 2015, 5:05 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29748/
> ---
> 
> (Updated April 8, 2015, 5:05 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
> 
> 
> Bugs: MESOS-2489
> https://issues.apache.org/jira/browse/MESOS-2489
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 9c01f5d6c692f835100e7cade928748cc4763cc8 
>   src/tests/reservation_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/29748/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 32149: Enabled 'Resources::apply' to handle reservation operations.

[Timothy Chen]

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

Ship it!


Ship It!

- Timothy Chen


On April 23, 2015, 4:38 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32149/
> ---
> 
> (Updated April 23, 2015, 4:38 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
> 
> 
> Bugs: MESOS-2477
> https://issues.apache.org/jira/browse/MESOS-2477
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp 2c99b6884d7296099e19e2e3182cbe11b5e1e059 
>   src/tests/resources_tests.cpp 7e0ad98c3366f647f190363a0e6b576dbfc7d415 
> 
> Diff: https://reviews.apache.org/r/32149/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 33295: Added firewall mechanism to control access on libprocess http endpoints.

[Joris Van Remoortere]

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



3rdparty/libprocess/src/process.cpp


For this synchronized block, and above:

In order to ensure we don't end up in a deadlock:

We should either:
1) make it clear that all virtual destructors of FirewallRules should never 
do a dispatch (i.e. async)
2) use a recursive mutex
3) take a copy of anything that could destroy in the synchronized block so 
that the destructor is delayed till after we exit the synchronized block


- Joris Van Remoortere


On April 22, 2015, 2:35 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33295/
> ---
> 
> (Updated April 22, 2015, 2:35 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, and Till Toenshoff.
> 
> 
> Bugs: MESOS-2620
> https://issues.apache.org/jira/browse/MESOS-2620
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduces the interface `FirewallRule` which will be matched against 
> incoming connections in order to allow them to be served or being blocked. 
> For details, check the [design 
> doc](https://docs.google.com/document/d/1JSJTJMJ6ZXLkCSmvOIabTLrjtqqr0E-u99Rx2BHR1hs/edit).
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/Makefile.am 
> 3da3e6cef1b5cb66c223425744d84741846ea730 
>   3rdparty/libprocess/include/process/firewall.hpp PRE-CREATION 
>   3rdparty/libprocess/include/process/process.hpp 
> 392c74df3e8a122aecd3633dffdeec4bcbd1f097 
>   3rdparty/libprocess/src/process.cpp 
> 97ac09fd10b767bc46387abc3657d005a00830c8 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> eb38edce2c483ba7f963a826893a15a075238618 
> 
> Diff: https://reviews.apache.org/r/33295/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 32149: Enabled 'Resources::apply' to handle reservation operations.

[Jie Yu]

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



src/tests/resources_tests.cpp


We do not use `snake_case`. Please use unreservedCpus and unreservedMem.



src/tests/resources_tests.cpp


Ditto.


- Jie Yu


On April 23, 2015, 4:38 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32149/
> ---
> 
> (Updated April 23, 2015, 4:38 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
> 
> 
> Bugs: MESOS-2477
> https://issues.apache.org/jira/browse/MESOS-2477
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp 2c99b6884d7296099e19e2e3182cbe11b5e1e059 
>   src/tests/resources_tests.cpp 7e0ad98c3366f647f190363a0e6b576dbfc7d415 
> 
> Diff: https://reviews.apache.org/r/32149/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 32140: Enabled 'Resources' to handle 'Resource::ReservationInfo'.

[Alexander Rukletsov]

> On April 15, 2015, 5:36 p.m., Alexander Rukletsov wrote:
> > src/tests/mesos.hpp, lines 376-378
> > 
> >
> > Why not putting the definition into `tests/mesos.cpp`? Here and below.
> 
> Michael Park wrote:
> I kept it consistent with other utility functions that live in this file: 
> `createTask`, `createDiskInfo`, `createPersistentVolume`. I think we can fix 
> this as a batch in a separate patch.
> 
> Alexander Rukletsov wrote:
> Mind creating a JIRA?
> 
> Michael Park wrote:
> Added [MESOS-2658](https://issues.apache.org/jira/browse/MESOS-2658) as a 
> subtask of [MESOS-1622](https://issues.apache.org/jira/browse/MESOS-1622).

Awesome!


- Alexander


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


On April 22, 2015, 8:36 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32140/
> ---
> 
> (Updated April 22, 2015, 8:36 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
> 
> 
> Bugs: MESOS-2476
> https://issues.apache.org/jira/browse/MESOS-2476
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `Resource::ReservationInfo` was introduced in 
> [r32139](https://reviews.apache.org/r/32139). We need to consider it in our 
> `Resources` class implementation.
> 
> ### `Resources::flatten`
> 
> `flatten` is used as the utility function to cross reservation boundaries 
> without affecting the given resources. Since the reservation is now specified 
> by the (`role`, `reservation`) pair, `flatten` needs to consider 
> `ReservationInfo` as well.
> 
> ### `Resources::validate`
> 
> If `role == "*"`, then `reservation` field must not be set.
> 
> ### `Resources` comparators
> 
> `operator==`, `addable` and `substractable` need to test for 
> `ReservationInfo` as well.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 56affd45e1e71e96ff5778b43271f81b9b9a9e4d 
>   src/common/resources.cpp 2c99b6884d7296099e19e2e3182cbe11b5e1e059 
>   src/tests/mesos.hpp 4294e28f904161b473c0cfa7feea4eaa4e7c97e3 
>   src/tests/resources_tests.cpp 7e0ad98c3366f647f190363a0e6b576dbfc7d415 
> 
> Diff: https://reviews.apache.org/r/32140/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 33465: Removed 'uuid' field from UPDATE call.

[Ben Mahler]

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



src/scheduler/scheduler.cpp


I realize this hack is necessary to accomplish this in 1 version, but 
should we have a TODO here for the master to not set the 'uuid' for non-retried 
updates? Similarly, if there are other cases (e.g. in the slave) where 
non-retried updates are generated, can we omit the 'uuid'?

Even better, why not make that change now in 0.23.0 and leave the TODO here 
for 0.24.0 to rely on uuid directly?
That will also help us move away from the similar hack in sched.cpp 
`statusUpdate` (see my TODO). :)

Also, how did you test the non-acknowledgement case? :)


- Ben Mahler


On April 23, 2015, 5:48 a.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33465/
> ---
> 
> (Updated April 23, 2015, 5:48 a.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-1127
> https://issues.apache.org/jira/browse/MESOS-1127
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Now that TaskStatus has 'uuid', we don't need it in UPDATE. It will also make 
> the ACKNOWLEDGE semantics easier.
> 
> 
> Diffs
> -
> 
>   include/mesos/scheduler/scheduler.proto 
> 783a63ad1cc0edd86605d638046fb959cb6e97e8 
>   src/examples/low_level_scheduler_libprocess.cpp 
> 63d34eefb60d13fe2b82905c1cec9b762340e997 
>   src/examples/low_level_scheduler_pthread.cpp 
> 6d1f938660c02db75bfcbf7c8de0d941cff1920d 
>   src/scheduler/scheduler.cpp bd9fced0f58aa3bc0ff147dbefb77cea4734a79e 
> 
> Diff: https://reviews.apache.org/r/33465/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 33241: docs: Add documentation on observability metrics.

[Ben Mahler]

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


Thanks for sending this!

Can you include a link to the rendered markdown please? Makes it easier for us 
to review :)

- Ben Mahler


On April 21, 2015, 10:09 p.m., Ricardo Cervera-Navarro wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33241/
> ---
> 
> (Updated April 21, 2015, 10:09 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-2621
> https://issues.apache.org/jira/browse/MESOS-2621
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> docs: Add documentation on observability metrics.
> 
> 
> Diffs
> -
> 
>   docs/home.md 6ab61f85aa7d62e0f19267b886dffb4e0aa826ea 
>   docs/metrics.md PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/33241/diff/
> 
> 
> Testing
> ---
> 
> - Previewed in markdown editor.
> - Replaced page content using the Element Inspector in Chrome to ensure that 
> the tables look good.
> 
> 
> Thanks,
> 
> Ricardo Cervera-Navarro
> 
>

Re: Review Request 32996: Updated roadmap document to link to 'Epic' tickets.

[Ben Mahler]

> On April 20, 2015, 7:01 p.m., Dave Lester wrote:
> > Since you're updating this doc, can you add a link to the release planning 
> > schedule? 
> > https://cwiki.apache.org/confluence/display/MESOS/Mesos+Release+Planning 
> > Seems directly related to this.

Will do.


- Ben


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


On April 9, 2015, 12:30 a.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32996/
> ---
> 
> (Updated April 9, 2015, 12:30 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Epic tickets provide a good view of upcoming and ongoing projects.
> 
> 
> Diffs
> -
> 
>   docs/home.md 6ab61f85aa7d62e0f19267b886dffb4e0aa826ea 
>   docs/mesos-roadmap.md 150fad84d5ad264929775365fd1b941aefb89ec4 
> 
> Diff: https://reviews.apache.org/r/32996/diff/
> 
> 
> Testing
> ---
> 
> N/A
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 32996: Updated roadmap document to link to 'Epic' tickets.

[Ben Mahler]

> On April 20, 2015, 6:56 p.m., Niklas Nielsen wrote:
> > docs/mesos-roadmap.md, line 11
> > 
> >
> > It sounds a bit hostile (it may just be me), how about something like 
> > "For comments and suggestions for the Mesos roadmap, feel free to reach out 
> > to the Mesos dev list"
> 
> Dave Lester wrote:
> I like Niklas' friendlier version :)

Sounds good!


- Ben


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


On April 9, 2015, 12:30 a.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32996/
> ---
> 
> (Updated April 9, 2015, 12:30 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Epic tickets provide a good view of upcoming and ongoing projects.
> 
> 
> Diffs
> -
> 
>   docs/home.md 6ab61f85aa7d62e0f19267b886dffb4e0aa826ea 
>   docs/mesos-roadmap.md 150fad84d5ad264929775365fd1b941aefb89ec4 
> 
> Diff: https://reviews.apache.org/r/32996/diff/
> 
> 
> Testing
> ---
> 
> N/A
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Review Request 33490: MESOS-2633 Avoid adding duplicate tasks

[Marco Massenzio]

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

Review request for mesos, Adam B and Joris Van Remoortere.


Repository: mesos


Description
---

In Framework::addCompletedTask(const Task& task) we did not check
for duplicated tasks, so they could be added more than once.

A simple check has now been added (there still is the issue
of whether the `operator ==` on `Task` is too strict, so as
never to cause a match).


Diffs
-

  src/master/framework.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/33490/diff/


Testing
---

buildbot make check pass
http://build.mesosphere.com:/builders/dev_test/builds/13


Thanks,

Marco Massenzio

Re: Review Request 32140: Enabled 'Resources' to handle 'Resource::ReservationInfo'.

[Jie Yu]

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

Ship it!


Discussed with Mpark offline. We agreed that rule for Resources::validate is 
that it should only perform necessary validation to make sure all methods in 
Resources are well hahaved, and the validation around * and reservation info is 
necessary for 'reserved/unreserved' to work properly. Thus dropping the issue 
around validation.

- Jie Yu


On April 22, 2015, 8:36 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32140/
> ---
> 
> (Updated April 22, 2015, 8:36 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
> 
> 
> Bugs: MESOS-2476
> https://issues.apache.org/jira/browse/MESOS-2476
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `Resource::ReservationInfo` was introduced in 
> [r32139](https://reviews.apache.org/r/32139). We need to consider it in our 
> `Resources` class implementation.
> 
> ### `Resources::flatten`
> 
> `flatten` is used as the utility function to cross reservation boundaries 
> without affecting the given resources. Since the reservation is now specified 
> by the (`role`, `reservation`) pair, `flatten` needs to consider 
> `ReservationInfo` as well.
> 
> ### `Resources::validate`
> 
> If `role == "*"`, then `reservation` field must not be set.
> 
> ### `Resources` comparators
> 
> `operator==`, `addable` and `substractable` need to test for 
> `ReservationInfo` as well.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 56affd45e1e71e96ff5778b43271f81b9b9a9e4d 
>   src/common/resources.cpp 2c99b6884d7296099e19e2e3182cbe11b5e1e059 
>   src/tests/mesos.hpp 4294e28f904161b473c0cfa7feea4eaa4e7c97e3 
>   src/tests/resources_tests.cpp 7e0ad98c3366f647f190363a0e6b576dbfc7d415 
> 
> Diff: https://reviews.apache.org/r/32140/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 33490: MESOS-2633 Avoid adding duplicate tasks

[Mesos ReviewBot]

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


Bad patch!

Reviews applied: [33490]

Failed command: ./support/apply-review.sh -n -r 33490

Error:
 2015-04-23 19:43:56 URL:https://reviews.apache.org/r/33490/diff/raw/ 
[1294/1294] -> "33490.patch" [1]
error: src/master/framework.cpp: does not exist in index
Failed to apply patch

- Mesos ReviewBot


On April 23, 2015, 7:36 p.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33490/
> ---
> 
> (Updated April 23, 2015, 7:36 p.m.)
> 
> 
> Review request for mesos, Adam B and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In Framework::addCompletedTask(const Task& task) we did not check
> for duplicated tasks, so they could be added more than once.
> 
> A simple check has now been added (there still is the issue
> of whether the `operator ==` on `Task` is too strict, so as
> never to cause a match).
> 
> 
> Diffs
> -
> 
>   src/master/framework.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/33490/diff/
> 
> 
> Testing
> ---
> 
> buildbot make check pass
> http://build.mesosphere.com:/builders/dev_test/builds/13
> 
> 
> Thanks,
> 
> Marco Massenzio
> 
>

Re: Review Request 32140: Enabled 'Resources' to handle 'Resource::ReservationInfo'.

[Michael Park]

> On April 23, 2015, 5:35 p.m., Jie Yu wrote:
> > src/tests/resources_tests.cpp, line 835
> > 
> >
> > Move this to src/tests/mesos.hpp close to createPersistentVolume?

I believe you're referring to `createDiskResource`. I kept 
`createReservedResource` close to the tests that use it.

```cpp
static Resource createReservedResource(...) { ... }

// Tests that use createReservedResource...

static Resource createDiskResource(...) { ... }

// Tests that use createDiskResource...
```

Would it be better to pull up the `createDiskResource` to be:

```cpp
static Resource createReservedResource(...) { ... }

static Resource createDiskResource(...) { ... }

// Tests that use createReservedResource...

// Tests that use createDiskResource...
```


- Michael


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


On April 22, 2015, 8:36 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32140/
> ---
> 
> (Updated April 22, 2015, 8:36 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
> 
> 
> Bugs: MESOS-2476
> https://issues.apache.org/jira/browse/MESOS-2476
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `Resource::ReservationInfo` was introduced in 
> [r32139](https://reviews.apache.org/r/32139). We need to consider it in our 
> `Resources` class implementation.
> 
> ### `Resources::flatten`
> 
> `flatten` is used as the utility function to cross reservation boundaries 
> without affecting the given resources. Since the reservation is now specified 
> by the (`role`, `reservation`) pair, `flatten` needs to consider 
> `ReservationInfo` as well.
> 
> ### `Resources::validate`
> 
> If `role == "*"`, then `reservation` field must not be set.
> 
> ### `Resources` comparators
> 
> `operator==`, `addable` and `substractable` need to test for 
> `ReservationInfo` as well.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 56affd45e1e71e96ff5778b43271f81b9b9a9e4d 
>   src/common/resources.cpp 2c99b6884d7296099e19e2e3182cbe11b5e1e059 
>   src/tests/mesos.hpp 4294e28f904161b473c0cfa7feea4eaa4e7c97e3 
>   src/tests/resources_tests.cpp 7e0ad98c3366f647f190363a0e6b576dbfc7d415 
> 
> Diff: https://reviews.apache.org/r/32140/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 33490: MESOS-2633 Avoid adding duplicate tasks

[Marco Massenzio]

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

(Updated April 23, 2015, 8:03 p.m.)


Review request for mesos, Adam B and Joris Van Remoortere.


Repository: mesos


Description
---

In Framework::addCompletedTask(const Task& task) we did not check
for duplicated tasks, so they could be added more than once.

A simple check has now been added (there still is the issue
of whether the `operator ==` on `Task` is too strict, so as
never to cause a match).


Diffs
-

  src/master/framework.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/33490/diff/


Testing
---

buildbot make check pass
http://build.mesosphere.com:/builders/dev_test/builds/13


Thanks,

Marco Massenzio

Review Request 33491: Set the supplementary groups list when switching users.

[James Peach]

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

Review request for mesos.


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


Repository: mesos


Description
---

Make sure we call initgroups(3) after calling setgid(2) so that the
auxiliary groups for the su'd user are set.

This is particularly important for docker support because it lets
you add your mesos user to the docker group so it can talk to docker
through /var/run/docker.sock (which is owned by a docker group by
default in most installations.) Without initgroups, the Mesos user
only has its primary GID set.


Diffs
-

  3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 
02db3c587e3f9a40282405e9496bde30e251f8bb 

Diff: https://reviews.apache.org/r/33491/diff/


Testing
---

Ran "make check" on CentOS 7 & and OS X 10.10.3.


Thanks,

James Peach

Re: Review Request 32140: Enabled 'Resources' to handle 'Resource::ReservationInfo'.

[Michael Park]

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

(Updated April 23, 2015, 8:25 p.m.)


Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.


Changes
---

Addressed some of Jie's comments.


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


Repository: mesos


Description
---

`Resource::ReservationInfo` was introduced in 
[r32139](https://reviews.apache.org/r/32139). We need to consider it in our 
`Resources` class implementation.

### `Resources::flatten`

`flatten` is used as the utility function to cross reservation boundaries 
without affecting the given resources. Since the reservation is now specified 
by the (`role`, `reservation`) pair, `flatten` needs to consider 
`ReservationInfo` as well.

### `Resources::validate`

If `role == "*"`, then `reservation` field must not be set.

### `Resources` comparators

`operator==`, `addable` and `substractable` need to test for `ReservationInfo` 
as well.


Diffs (updated)
-

  include/mesos/resources.hpp 56affd45e1e71e96ff5778b43271f81b9b9a9e4d 
  src/common/resources.cpp 2c99b6884d7296099e19e2e3182cbe11b5e1e059 
  src/tests/mesos.hpp 4294e28f904161b473c0cfa7feea4eaa4e7c97e3 
  src/tests/resources_tests.cpp 7e0ad98c3366f647f190363a0e6b576dbfc7d415 

Diff: https://reviews.apache.org/r/32140/diff/


Testing
---

make check


Thanks,

Michael Park

Re: Review Request 32140: Enabled 'Resources' to handle 'Resource::ReservationInfo'.

[Michael Park]

> On April 23, 2015, 5:35 p.m., Jie Yu wrote:
> > src/common/resources.cpp, lines 445-449
> > 
> >
> > See my comments in https://reviews.apache.org/r/32150/
> > 
> > Can we move this to master validation? Thoughts?

Synced with Jie on IRC regarding this topic. We agreed that 
`Resources::validate` needs to capture the invariant of the `Resource` object 
which means it needs to invalidate the `role == "*" && has_reservation()` 
state. This invariant is required for all the predicates as well as functions 
such as `reserved()` and `unreserved()` to have well-defined behavior.


- Michael


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


On April 23, 2015, 8:25 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32140/
> ---
> 
> (Updated April 23, 2015, 8:25 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
> 
> 
> Bugs: MESOS-2476
> https://issues.apache.org/jira/browse/MESOS-2476
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `Resource::ReservationInfo` was introduced in 
> [r32139](https://reviews.apache.org/r/32139). We need to consider it in our 
> `Resources` class implementation.
> 
> ### `Resources::flatten`
> 
> `flatten` is used as the utility function to cross reservation boundaries 
> without affecting the given resources. Since the reservation is now specified 
> by the (`role`, `reservation`) pair, `flatten` needs to consider 
> `ReservationInfo` as well.
> 
> ### `Resources::validate`
> 
> If `role == "*"`, then `reservation` field must not be set.
> 
> ### `Resources` comparators
> 
> `operator==`, `addable` and `substractable` need to test for 
> `ReservationInfo` as well.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 56affd45e1e71e96ff5778b43271f81b9b9a9e4d 
>   src/common/resources.cpp 2c99b6884d7296099e19e2e3182cbe11b5e1e059 
>   src/tests/mesos.hpp 4294e28f904161b473c0cfa7feea4eaa4e7c97e3 
>   src/tests/resources_tests.cpp 7e0ad98c3366f647f190363a0e6b576dbfc7d415 
> 
> Diff: https://reviews.apache.org/r/32140/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 32140: Enabled 'Resources' to handle 'Resource::ReservationInfo'.

[Michael Park]

> On April 8, 2015, 8:16 p.m., Jie Yu wrote:
> > src/common/resources.cpp, lines 450-459
> > 
> >
> > The semantics of this function becomes a little weired now. For 
> > example, for a resource that has `role == "*"` and has reservation set, 
> > `isReserved(resource, "*")` is going to return `true`? Given that 
> > 'resource' is invalid, we should return a `false` in that case?
> 
> Alexander Rukletsov wrote:
> Can we have a resource with `role == "*"` and reservations set?
> 
> Alexander Rukletsov wrote:
> Excuse my premature comment earlier, I'm slowly starting to understand 
> what's going on here. It looks like in the case Jie describes, the function 
> returns `true` indeed. Which is invalid by now, but _may_ become valid in the 
> future. On the other side, I'm not sure that returning `false` in this case 
> is an alternative: it will read as unreserved resource, not an invalid 
> resource. We can wrap the return value in `Try` but I prefer the way it's 
> done now.
> 
> Michael Park wrote:
> Hey guys, this predicate, as with other predicates assume that the 
> resources have already been validated.
> The following note can be found at the top of the predicate declarations 
> in the header.
> 
> ```
> // NOTE: The following predicate functions assume that the given
> // resource is validated.
> ```
> 
> Is it still weird with this assumption? If it was just that the note is 
> difficult to find, I could maybe put the note as a comment on every predicate?
> 
> Jie Yu wrote:
> See my first comment in https://reviews.apache.org/r/32150/
> 
> If we move the validation to master, then this function becomes weired.

Refer to https://reviews.apache.org/r/32140/#comment131682


- Michael


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


On April 23, 2015, 8:25 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32140/
> ---
> 
> (Updated April 23, 2015, 8:25 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
> 
> 
> Bugs: MESOS-2476
> https://issues.apache.org/jira/browse/MESOS-2476
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `Resource::ReservationInfo` was introduced in 
> [r32139](https://reviews.apache.org/r/32139). We need to consider it in our 
> `Resources` class implementation.
> 
> ### `Resources::flatten`
> 
> `flatten` is used as the utility function to cross reservation boundaries 
> without affecting the given resources. Since the reservation is now specified 
> by the (`role`, `reservation`) pair, `flatten` needs to consider 
> `ReservationInfo` as well.
> 
> ### `Resources::validate`
> 
> If `role == "*"`, then `reservation` field must not be set.
> 
> ### `Resources` comparators
> 
> `operator==`, `addable` and `substractable` need to test for 
> `ReservationInfo` as well.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 56affd45e1e71e96ff5778b43271f81b9b9a9e4d 
>   src/common/resources.cpp 2c99b6884d7296099e19e2e3182cbe11b5e1e059 
>   src/tests/mesos.hpp 4294e28f904161b473c0cfa7feea4eaa4e7c97e3 
>   src/tests/resources_tests.cpp 7e0ad98c3366f647f190363a0e6b576dbfc7d415 
> 
> Diff: https://reviews.apache.org/r/32140/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 33490: MESOS-2633 Avoid adding duplicate tasks

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [33376, 33490]

All tests passed.

- Mesos ReviewBot


On April 23, 2015, 8:03 p.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33490/
> ---
> 
> (Updated April 23, 2015, 8:03 p.m.)
> 
> 
> Review request for mesos, Adam B and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In Framework::addCompletedTask(const Task& task) we did not check
> for duplicated tasks, so they could be added more than once.
> 
> A simple check has now been added (there still is the issue
> of whether the `operator ==` on `Task` is too strict, so as
> never to cause a match).
> 
> 
> Diffs
> -
> 
>   src/master/framework.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/33490/diff/
> 
> 
> Testing
> ---
> 
> buildbot make check pass
> http://build.mesosphere.com:/builders/dev_test/builds/13
> 
> 
> Thanks,
> 
> Marco Massenzio
> 
>

Re: Review Request 32150: Enabled the master to handle reservation operations.

[Michael Park]

> On April 8, 2015, 10:38 p.m., Jie Yu wrote:
> > src/master/validation.cpp, line 61
> > 
> >
> > Do you want to add a `CHECK_NE(resource.role(), "*");` here.
> > 
> > That makes me wonder should we move the check in Resources::validate 
> > about `"*"` and `ReservationInfo` here. This sounds like a more high level 
> > semantical check, while checks in Resources::validate are all quite low 
> > level (e.g., check for invalid name, type name mismatch, etc.).
> > 
> > What do you think?

Synced with Jie on IRC regarding this. The summary is captured 
[here](https://reviews.apache.org/r/32140/#comment131682).


- Michael


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


On April 15, 2015, 3:55 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32150/
> ---
> 
> (Updated April 15, 2015, 3:55 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Ben Mahler, and Jie Yu.
> 
> 
> Bugs: MESOS-2139
> https://issues.apache.org/jira/browse/MESOS-2139
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Handled reservation operations in `Master::_accept`.
> 
> Added `validate` functions in `src/master/validation.{hpp,cpp}`.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 618db68ee4163b06e479cf3413eda4b63c9c5a4b 
>   src/master/validation.hpp 2d7416c053f82d6316542fa9c35b0e7bc605abec 
>   src/master/validation.cpp 2f2e4ea8ea123c5a0d01446cdec8b308ea60932e 
>   src/tests/master_validation_tests.cpp 
> 4f2ad58c3ae0f611fb476c4d91a37dd6a5541395 
> 
> Diff: https://reviews.apache.org/r/32150/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 33491: Set the supplementary groups list when switching users.

[Ben Mahler]

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


Thanks James!


3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp


I'm curious why you look at EPERM here, but not in the call to `::setgid` 
above and `::setuid` below? Is it possible for only some of these calls to fail 
with EPERM?


- Ben Mahler


On April 23, 2015, 8:16 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33491/
> ---
> 
> (Updated April 23, 2015, 8:16 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Timothy St. Clair.
> 
> 
> Bugs: MESOS-719
> https://issues.apache.org/jira/browse/MESOS-719
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Make sure we call initgroups(3) after calling setgid(2) so that the
> auxiliary groups for the su'd user are set.
> 
> This is particularly important for docker support because it lets
> you add your mesos user to the docker group so it can talk to docker
> through /var/run/docker.sock (which is owned by a docker group by
> default in most installations.) Without initgroups, the Mesos user
> only has its primary GID set.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 
> 02db3c587e3f9a40282405e9496bde30e251f8bb 
> 
> Diff: https://reviews.apache.org/r/33491/diff/
> 
> 
> Testing
> ---
> 
> Ran "make check" on CentOS 7 & and OS X 10.10.3.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 33491: Set the supplementary groups list when switching users.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [33491]

All tests passed.

- Mesos ReviewBot


On April 23, 2015, 8:16 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33491/
> ---
> 
> (Updated April 23, 2015, 8:16 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Timothy St. Clair.
> 
> 
> Bugs: MESOS-719
> https://issues.apache.org/jira/browse/MESOS-719
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Make sure we call initgroups(3) after calling setgid(2) so that the
> auxiliary groups for the su'd user are set.
> 
> This is particularly important for docker support because it lets
> you add your mesos user to the docker group so it can talk to docker
> through /var/run/docker.sock (which is owned by a docker group by
> default in most installations.) Without initgroups, the Mesos user
> only has its primary GID set.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 
> 02db3c587e3f9a40282405e9496bde30e251f8bb 
> 
> Diff: https://reviews.apache.org/r/33491/diff/
> 
> 
> Testing
> ---
> 
> Ran "make check" on CentOS 7 & and OS X 10.10.3.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 33491: Set the supplementary groups list when switching users.

[James Peach]

> On April 23, 2015, 9:16 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp, line 730
> > 
> >
> > I'm curious why you look at EPERM here, but not in the call to 
> > `::setgid` above and `::setuid` below? Is it possible for only some of 
> > these calls to fail with EPERM?

If you do a ``setgid`` to your current GID, it succeeds and does nothing, but 
``initgroups`` ends up calling ``setgroups`` which fails with EPERM. If you 
were trying to switch to a different user and you were not already privileged, 
the ``setgid`` would already have failed.


- James


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


On April 23, 2015, 8:16 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33491/
> ---
> 
> (Updated April 23, 2015, 8:16 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Timothy St. Clair.
> 
> 
> Bugs: MESOS-719
> https://issues.apache.org/jira/browse/MESOS-719
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Make sure we call initgroups(3) after calling setgid(2) so that the
> auxiliary groups for the su'd user are set.
> 
> This is particularly important for docker support because it lets
> you add your mesos user to the docker group so it can talk to docker
> through /var/run/docker.sock (which is owned by a docker group by
> default in most installations.) Without initgroups, the Mesos user
> only has its primary GID set.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 
> 02db3c587e3f9a40282405e9496bde30e251f8bb 
> 
> Diff: https://reviews.apache.org/r/33491/diff/
> 
> 
> Testing
> ---
> 
> Ran "make check" on CentOS 7 & and OS X 10.10.3.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 33193: Warn if g++ < 4.8, C++ standard library is too old

[Cody Maloney]

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

(Updated April 23, 2015, 9:38 p.m.)


Review request for mesos, Benjamin Hindman, Joris Van Remoortere, and Michael 
Park.


Changes
---

tmp


Repository: mesos


Description
---

Warn if g++ < 4.8, C++ standard library is too old

Not ready for merging. Want the Clang 3.6 patchset to land first: 
https://reviews.apache.org/r/32749/

A whole bunch more of the C++11 checks can be removed, we can unconditionally 
use -std=c++11, among other things with this change. I'm trying to keep the 
patch relatively minimal though unless we hit a problem after application and 
have to roll it back.

Explicitly don't check clang version number, since extracting it is hard (OS X 
clang behaves differently than Linux clang), and 'clang -dumpversion' always 
reports 4.2.1 for compatibility with some random tools...


Diffs (updated)
-

  configure.ac 7f9e52916b9d78f2bbff9d6ed9871444a0fda629 

Diff: https://reviews.apache.org/r/33193/diff/


Testing
---

Basic hand testing gcc 4.9.2, gcc 4.4.7, clang 3.6 all on Windows


Thanks,

Cody Maloney

Re: Review Request 33193: Warn if g++ < 4.8, C++ standard library is too old

[Benjamin Hindman]

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

Ship it!


I'll add the period and get this committed!


configure.ac


s/supported/supported./


- Benjamin Hindman


On April 23, 2015, 9:38 p.m., Cody Maloney wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33193/
> ---
> 
> (Updated April 23, 2015, 9:38 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Joris Van Remoortere, and Michael 
> Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Warn if g++ < 4.8, C++ standard library is too old
> 
> Not ready for merging. Want the Clang 3.6 patchset to land first: 
> https://reviews.apache.org/r/32749/
> 
> A whole bunch more of the C++11 checks can be removed, we can unconditionally 
> use -std=c++11, among other things with this change. I'm trying to keep the 
> patch relatively minimal though unless we hit a problem after application and 
> have to roll it back.
> 
> Explicitly don't check clang version number, since extracting it is hard (OS 
> X clang behaves differently than Linux clang), and 'clang -dumpversion' 
> always reports 4.2.1 for compatibility with some random tools...
> 
> 
> Diffs
> -
> 
>   configure.ac 7f9e52916b9d78f2bbff9d6ed9871444a0fda629 
> 
> Diff: https://reviews.apache.org/r/33193/diff/
> 
> 
> Testing
> ---
> 
> Basic hand testing gcc 4.9.2, gcc 4.4.7, clang 3.6 all on Windows
> 
> 
> Thanks,
> 
> Cody Maloney
> 
>

Re: Review Request 33491: Set the supplementary groups list when switching users.

[Ben Mahler]

> On April 23, 2015, 9:16 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp, line 730
> > 
> >
> > I'm curious why you look at EPERM here, but not in the call to 
> > `::setgid` above and `::setuid` below? Is it possible for only some of 
> > these calls to fail with EPERM?
> 
> James Peach wrote:
> If you do a ``setgid`` to your current GID, it succeeds and does nothing, 
> but ``initgroups`` ends up calling ``setgroups`` which fails with EPERM. If 
> you were trying to switch to a different user and you were not already 
> privileged, the ``setgid`` would already have failed.

I see, in that case, could we express it in the structure of the code more 
explicitly? Is there an easy way to capture whether we need to perform the 
operation? For example:

```
inline Try su(const std::string& user)
{
  Result gid = os::getgid(user);
  Result uid = os::getuid(user);

  if (gid.isError() || gid.isNone()) {
return Error("Failed to getgid: " +
(gid.isError() ? gid.error() : "unknown user"));
  } else if (uid.isError() || uid.isNone()) {
return Error("Failed to getuid: " +
(uid.isError() ? uid.error() : "unknown user"));
  }

  if (gid.get() != ::getgid()) {
if (::setgid(gid.get()) == -1) {
  return ErrnoError("Failed to set gid");
}
  
if (::initgroups(user.c_str(), gid.get()) == -1) {
  return ErrnoError("Failed to set supplementary groups");
}
  }

  if (uid.get() != ::getuid()) {
if (::setuid(uid.get())) {
  return ErrnoError("Failed to setuid");
}
  }

  return Nothing();
}
```

How does this interact with real, effective, and saved set-group/set-user IDs?


- Ben


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


On April 23, 2015, 8:16 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33491/
> ---
> 
> (Updated April 23, 2015, 8:16 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Timothy St. Clair.
> 
> 
> Bugs: MESOS-719
> https://issues.apache.org/jira/browse/MESOS-719
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Make sure we call initgroups(3) after calling setgid(2) so that the
> auxiliary groups for the su'd user are set.
> 
> This is particularly important for docker support because it lets
> you add your mesos user to the docker group so it can talk to docker
> through /var/run/docker.sock (which is owned by a docker group by
> default in most installations.) Without initgroups, the Mesos user
> only has its primary GID set.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 
> 02db3c587e3f9a40282405e9496bde30e251f8bb 
> 
> Diff: https://reviews.apache.org/r/33491/diff/
> 
> 
> Testing
> ---
> 
> Ran "make check" on CentOS 7 & and OS X 10.10.3.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 33491: Set the supplementary groups list when switching users.

[James Peach]

> On April 23, 2015, 9:16 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp, line 730
> > 
> >
> > I'm curious why you look at EPERM here, but not in the call to 
> > `::setgid` above and `::setuid` below? Is it possible for only some of 
> > these calls to fail with EPERM?
> 
> James Peach wrote:
> If you do a ``setgid`` to your current GID, it succeeds and does nothing, 
> but ``initgroups`` ends up calling ``setgroups`` which fails with EPERM. If 
> you were trying to switch to a different user and you were not already 
> privileged, the ``setgid`` would already have failed.
> 
> Ben Mahler wrote:
> I see, in that case, could we express it in the structure of the code 
> more explicitly? Is there an easy way to capture whether we need to perform 
> the operation? For example:
> 
> ```
> inline Try su(const std::string& user)
> {
>   Result gid = os::getgid(user);
>   Result uid = os::getuid(user);
> 
>   if (gid.isError() || gid.isNone()) {
> return Error("Failed to getgid: " +
> (gid.isError() ? gid.error() : "unknown user"));
>   } else if (uid.isError() || uid.isNone()) {
> return Error("Failed to getuid: " +
> (uid.isError() ? uid.error() : "unknown user"));
>   }
> 
>   if (gid.get() != ::getgid()) {
> if (::setgid(gid.get()) == -1) {
>   return ErrnoError("Failed to set gid");
> }
>   
> if (::initgroups(user.c_str(), gid.get()) == -1) {
>   return ErrnoError("Failed to set supplementary groups");
> }
>   }
> 
>   if (uid.get() != ::getuid()) {
> if (::setuid(uid.get())) {
>   return ErrnoError("Failed to setuid");
> }
>   }
> 
>   return Nothing();
> }
> ```
> 
> How does this interact with real, effective, and saved set-group/set-user 
> IDs?

``setgid`` sets all the GIDs (real, effective and saved) according to 
[POSIX](http://pubs.opengroup.org/onlinepubs/9699919799/functions/setgid.html). 
In the code you propose above, it's possible that because the real GID matches, 
we would fail to set the remaining GIDs. I don't know whether that *really* 
matters, since in practice I guess only the test suite would hit that case.


- James


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


On April 23, 2015, 8:16 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33491/
> ---
> 
> (Updated April 23, 2015, 8:16 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Timothy St. Clair.
> 
> 
> Bugs: MESOS-719
> https://issues.apache.org/jira/browse/MESOS-719
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Make sure we call initgroups(3) after calling setgid(2) so that the
> auxiliary groups for the su'd user are set.
> 
> This is particularly important for docker support because it lets
> you add your mesos user to the docker group so it can talk to docker
> through /var/run/docker.sock (which is owned by a docker group by
> default in most installations.) Without initgroups, the Mesos user
> only has its primary GID set.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 
> 02db3c587e3f9a40282405e9496bde30e251f8bb 
> 
> Diff: https://reviews.apache.org/r/33491/diff/
> 
> 
> Testing
> ---
> 
> Ran "make check" on CentOS 7 & and OS X 10.10.3.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 32999: Added a document for engineering principles and practices.

[Ben Mahler]

> On April 20, 2015, 7:32 p.m., Niklas Nielsen wrote:
> > docs/engineering-principles-and-practices.md, line 7
> > 
> >
> > Long lines :) Do you think it is worth applying the 80 col style? If 
> > so, we should do a scan.
> > 
> > s/**high quality**, **robust** code/**high quality** and **robust** 
> > code/?

Thanks! Line-wrapped these manually, would love to automate our markdown 
formatting.


- Ben


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


On April 9, 2015, 12:30 a.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32999/
> ---
> 
> (Updated April 9, 2015, 12:30 a.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Jie Yu, Niklas Nielsen, 
> and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a document for engineering principles and practices.
> 
> 
> Diffs
> -
> 
>   docs/engineering-principles-and-practices.md PRE-CREATION 
>   docs/home.md 6ab61f85aa7d62e0f19267b886dffb4e0aa826ea 
> 
> Diff: https://reviews.apache.org/r/32999/diff/
> 
> 
> Testing
> ---
> 
> N/A
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 32998: Split committer's guide into code reviewing and committing documents.

[Ben Mahler]

> On April 13, 2015, 6:30 p.m., haosdent huang wrote:
> > docs/effective-code-reviewing.md, line 14
> > 
> >
> > Should we add a requirement that "pass unit tests before post reviews" 
> > here?

Thanks! Haven't seen this one to be a common mistake so I left it out, but 
let's keep an eye open for extra things to add here!


- Ben


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


On April 9, 2015, 12:30 a.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32998/
> ---
> 
> (Updated April 9, 2015, 12:30 a.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Jie Yu, Niklas Nielsen, 
> and Vinod Kone.
> 
> 
> Bugs: MESOS-2581
> https://issues.apache.org/jira/browse/MESOS-2581
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> "Committer's Guide" was too generic. This names the documents after "what" 
> the reader is looking for: doing effective reviews, and how to commit changes 
> (for committers only).
> 
> 
> Diffs
> -
> 
>   docs/committers-guide.md c016ee9cb3290d7788ed258904547b59bbea4f11 
>   docs/committing.md PRE-CREATION 
>   docs/effective-code-reviewing.md PRE-CREATION 
>   docs/home.md 6ab61f85aa7d62e0f19267b886dffb4e0aa826ea 
> 
> Diff: https://reviews.apache.org/r/32998/diff/
> 
> 
> Testing
> ---
> 
> N/A
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 32998: Split committer's guide into code reviewing and committing documents.

[Ben Mahler]

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

(Updated April 23, 2015, 11:06 p.m.)


Review request for mesos, Adam B, Benjamin Hindman, Jie Yu, Niklas Nielsen, and 
Vinod Kone.


Changes
---

Review comments.


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


Repository: mesos


Description
---

"Committer's Guide" was too generic. This names the documents after "what" the 
reader is looking for: doing effective reviews, and how to commit changes (for 
committers only).


Diffs (updated)
-

  docs/committers-guide.md c016ee9cb3290d7788ed258904547b59bbea4f11 
  docs/committing.md PRE-CREATION 
  docs/effective-code-reviewing.md PRE-CREATION 
  docs/home.md 9fabf775206131ffdc36141c3b5e45f15a45a113 

Diff: https://reviews.apache.org/r/32998/diff/


Testing
---

N/A


Thanks,

Ben Mahler

Re: Review Request 32998: Split committer's guide into code reviewing and committing documents.

[Ben Mahler]

> On April 9, 2015, 12:51 a.m., Adam B wrote:
> > docs/committing.md, line 18
> > 
> >
> > Would like to formalize what kinds of changes you "don't worry about 
> > going through a review cycle". I'd propose that typo/comment/doc changes 
> > under 5 lines, or obvious build fixes are immune. Anything more complex 
> > than that deserves at least a cursory review.

I'm with you, although it will be tough to define the criteria for this, let 
alone enforce it. Use your judgement for now. :)


> On April 9, 2015, 12:51 a.m., Adam B wrote:
> > docs/effective-code-reviewing.md, line 13
> > 
> >
> > Keep in mind that the review Summary + Description gets used as the 
> > commit message, so don't put unnecessary fluff in there.
> > The testing message doesn't go into the commit section, so it can be 
> > used for notes to the reviewers.
> > Also, please provide details about the testing done and new tests 
> > added; hopefully more than just `make check`.

Added a new bullet related to self-review before publishing, that captures some 
of this.


- Ben


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


On April 9, 2015, 12:30 a.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32998/
> ---
> 
> (Updated April 9, 2015, 12:30 a.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Jie Yu, Niklas Nielsen, 
> and Vinod Kone.
> 
> 
> Bugs: MESOS-2581
> https://issues.apache.org/jira/browse/MESOS-2581
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> "Committer's Guide" was too generic. This names the documents after "what" 
> the reader is looking for: doing effective reviews, and how to commit changes 
> (for committers only).
> 
> 
> Diffs
> -
> 
>   docs/committers-guide.md c016ee9cb3290d7788ed258904547b59bbea4f11 
>   docs/committing.md PRE-CREATION 
>   docs/effective-code-reviewing.md PRE-CREATION 
>   docs/home.md 6ab61f85aa7d62e0f19267b886dffb4e0aa826ea 
> 
> Diff: https://reviews.apache.org/r/32998/diff/
> 
> 
> Testing
> ---
> 
> N/A
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 32998: Split committer's guide into code reviewing and committing documents.

[Ben Mahler]

> On April 20, 2015, 7:28 p.m., Niklas Nielsen wrote:
> > docs/effective-code-reviewing.md, line 19
> > 
> >
> > s/, this is not meant to be a sparring match!/./

Hm.. this was copy/pasted from benh's original committer's guide. Can you tell 
me more about why you think it should be omitted? FWIW, I don't see it 
happening lately, but it has come up in the past which is why I think it was 
added :)


> On April 20, 2015, 7:28 p.m., Niklas Nielsen wrote:
> > docs/effective-code-reviewing.md, line 21
> > 
> >
> > s/**//g

`**` is bold, you want to remove all the emphasis in this document..?


> On April 20, 2015, 7:28 p.m., Niklas Nielsen wrote:
> > docs/committing.md, line 13
> > 
> >
> > Very long line. Do we have a style guide for markdown somewhere? I 
> > would think that 80 cols would work :)

Would love to have a markdown `fmt` / linter that we can run!


- Ben


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


On April 9, 2015, 12:30 a.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32998/
> ---
> 
> (Updated April 9, 2015, 12:30 a.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Jie Yu, Niklas Nielsen, 
> and Vinod Kone.
> 
> 
> Bugs: MESOS-2581
> https://issues.apache.org/jira/browse/MESOS-2581
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> "Committer's Guide" was too generic. This names the documents after "what" 
> the reader is looking for: doing effective reviews, and how to commit changes 
> (for committers only).
> 
> 
> Diffs
> -
> 
>   docs/committers-guide.md c016ee9cb3290d7788ed258904547b59bbea4f11 
>   docs/committing.md PRE-CREATION 
>   docs/effective-code-reviewing.md PRE-CREATION 
>   docs/home.md 6ab61f85aa7d62e0f19267b886dffb4e0aa826ea 
> 
> Diff: https://reviews.apache.org/r/32998/diff/
> 
> 
> Testing
> ---
> 
> N/A
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 32999: Added a document for engineering principles and practices.

[Ben Mahler]

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

(Updated April 23, 2015, 11:15 p.m.)


Review request for mesos, Adam B, Benjamin Hindman, Jie Yu, Niklas Nielsen, and 
Vinod Kone.


Changes
---

Formatting cleanup.


Repository: mesos


Description
---

Added a document for engineering principles and practices.


Diffs (updated)
-

  docs/engineering-principles-and-practices.md PRE-CREATION 
  docs/home.md 641ee8f5e7cc2f9ccd62a5c4236912886aaa7a1d 

Diff: https://reviews.apache.org/r/32999/diff/


Testing
---

N/A


Thanks,

Ben Mahler

Re: Review Request 28257: Allow prefix paths in libprocess

[Cody Maloney]

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

(Updated April 23, 2015, 11:32 p.m.)


Review request for mesos, Benjamin Hindman, Dominic Hamon, and Joris Van 
Remoortere.


Changes
---

Sharing

Changed name to routeWithPrefix.

Added a helper addRoute for common implementation bits between route and 
routeWithPrefx


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


Repository: mesos


Description
---

Allow prefix paths in libprocess


Diffs (updated)
-

  3rdparty/libprocess/include/process/process.hpp 
392c74df3e8a122aecd3633dffdeec4bcbd1f097 
  3rdparty/libprocess/src/process.cpp cf4e36489be2c6aa01e838c1c71383f248deab5b 

Diff: https://reviews.apache.org/r/28257/diff/


Testing
---

make distcheck ubuntu 14.04

Manually browse the mesos web UI and verify that things seem to generally work


Thanks,

Cody Maloney

Re: Review Request 32999: Added a document for engineering principles and practices.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [32995, 32996, 32997, 32998, 32999]

All tests passed.

- Mesos ReviewBot


On April 23, 2015, 11:15 p.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32999/
> ---
> 
> (Updated April 23, 2015, 11:15 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Jie Yu, Niklas Nielsen, 
> and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a document for engineering principles and practices.
> 
> 
> Diffs
> -
> 
>   docs/engineering-principles-and-practices.md PRE-CREATION 
>   docs/home.md 641ee8f5e7cc2f9ccd62a5c4236912886aaa7a1d 
> 
> Diff: https://reviews.apache.org/r/32999/diff/
> 
> 
> Testing
> ---
> 
> N/A
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>