Re: Review Request 61982: Cleaned up DefaultExecutor tests.

[Mesos Reviewbot Windows]

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



Failed to apply patch!

Reviews applied: [61920, 61921, 61982]

Logs available here: http://104.210.40.105/logs/master/61982

- Mesos Reviewbot Windows


On Aug. 30, 2017, 1:05 a.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61982/
> ---
> 
> (Updated Aug. 30, 2017, 1:05 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Greg Mann.
> 
> 
> Bugs: MESOS-7916
> https://issues.apache.org/jira/browse/MESOS-7916
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Update the DefaultExecutor tests to use test helpers where possible.
> Also make the boilerplate initialization code consistent across tests.
> 
> 
> Diffs
> -
> 
>   src/tests/default_executor_tests.cpp 
> afe0afabf784fb65eb833beadd3c584722c321e1 
> 
> 
> Diff: https://reviews.apache.org/r/61982/diff/1/
> 
> 
> Testing
> ---
> 
> `sudo bin/mesos-tests.sh --gtest_filter="*DefaultExecutor*"` succeeds on 
> GNU/Linux
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 61473: Do not kill non partition aware tasks.

[Jiang Yan Xu]

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



Good progress! Haven't looked at tests yet. Let's address these issues first.


src/master/http.cpp
Lines 322-328 (original), 322-330 (patched)


Two cascading and adjacent ifs looks a bit odd, plus why handle 
capabilities check and authorization check any differently?

Either 

```
if (!framework_->capabilities.partitionAware) {
  continue;
}

// Skip unauthorized tasks.
if (!authorizeTask_->accept(*task.get(), framework_->info)) {
  continue;
}

writer->element(*task.get());
```

or 

```
// Skip unauthorized tasks.
if (framework_->capabilities.partitionAware &&
authorizeTask_->accept(*task.get(), framework_->info)) {
  writer->element(*task.get());
}
```

would be better.

But I guess the former is slight preferred despite being more verbose 
because it allows you to insert more "checks" in the future and it's more 
consistent with some other similar instances in this file?

Here and below.



src/master/http.cpp
Lines 343 (patched)


This deserves a comment:

```
// Unreachable tasks belonging to a non-partition-aware framework are 
exported as completed due to backward compatibility.
```



src/master/http.cpp
Lines 4216 (patched)


Use a comment similar to the one suggested above here.



src/master/master.hpp
Line 850 (original), 850 (patched)


You have to modify both `Master::removeTask` and `Framework::removeTask` 
and with `bool unreachable` arugment the APIs are harder to understand. I think 
this criteria is important: does a method provide abstraction or is merely for 
code reuse. I feel `bool unreachable` would fail this check.

Can we keep the state `TASK_UNREACHABLE` throughout the internal calls and 
only convert them to `TASK_LOST` when a status update is created or it is 
stored in the member data structures?

This way the following code would still work:

```
if (task->state() == TASK_UNREACHABLE) {
  addUnreachableTask(*task);
} else {
  addCompletedTask(*task);
}
```

Note that this is different than the previous idea which **stores** the 
`TASK_UNREACHABLE` state and requires a bunch of conversions later.



src/master/master.hpp
Lines 2826-2828 (original), 2819-2823 (patched)


Can we tweak this comment a little so it sounds less "hacky"?

```
When an agent is marked unreachable, all tasks on it are stored here. We 
only keep a fixed-size cache to avoid consuming too much memory.

NOTE: non-partition-aware unreachable tasks in this map are marked 
TASK_LOST instead of TASK_UNREACHABLE for backward compatibility.
```



src/master/master.cpp
Line 6402 (original), 6393 (patched)


s/non partition-aware tasks/non-partition-aware tasks/



src/master/master.cpp
Lines 6427-6428 (original), 6409-6410 (patched)


This comment is probably unncessary because "partition aware + 
non-partition aware" pretty much means "we don't care" so why comment about it 
:)



src/master/master.cpp
Lines 8948-8950 (patched)


If the framework is not partition aware, the `update` will already have a 
`TASK_LOST` state right?


- Jiang Yan Xu


On Aug. 28, 2017, 8:31 a.m., Megha Sharma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61473/
> ---
> 
> (Updated Aug. 28, 2017, 8:31 a.m.)
> 
> 
> Review request for mesos, Vinod Kone and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7215
> https://issues.apache.org/jira/browse/MESOS-7215
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Master will not kill the tasks for non-Partition aware frameworks
> when an unreachable agent re-registers with the master.
> Master used to send a ShutdownFrameworkMessages to the agent
> to kill the tasks from non partition aware frameworks including the
> ones that are still registered which was problematic because the offer
> from this agent could still go to the same framework which could then
> launch new tasks. The agent would then receive tasks of the same
> framework and ignore them because it thinks the framework is shutting
> down. The framework is not shutting 

Review Request 61982: Cleaned up DefaultExecutor tests.

[Gastón Kleiman]

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

Review request for mesos, Anand Mazumdar and Greg Mann.


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


Repository: mesos


Description (updated)
---

Update the DefaultExecutor tests to use test helpers where possible.
Also make the boilerplate initialization code consistent across tests.


Diffs (updated)
-

  src/tests/default_executor_tests.cpp afe0afabf784fb65eb833beadd3c584722c321e1 


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


Testing (updated)
---

`sudo bin/mesos-tests.sh --gtest_filter="*DefaultExecutor*"` succeeds on 
GNU/Linux


Thanks,

Gastón Kleiman

Re: Review Request 61921: Added tests to ensure that tasks can access their parent's volumes.

[Greg Mann]

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




src/tests/default_executor_tests.cpp
Lines 1981 (patched)


Could you say explicitly here that the tasks in this test are in the same 
task group?



src/tests/default_executor_tests.cpp
Lines 1982 (patched)


Use backticks consistently around `sandbox_path`; here and below.



src/tests/default_executor_tests.cpp
Lines 1984 (patched)


Nice test!! :)



src/tests/default_executor_tests.cpp
Lines 2014 (patched)


Is this needed?



src/tests/default_executor_tests.cpp
Lines 2072-2073 (patched)


This comment is a little confusing. Maybe something like:

"The test will only succeed if the executor and tasks share the same 
volume."



src/tests/default_executor_tests.cpp
Lines 2106-2108 (patched)


Fits on two lines.



src/tests/default_executor_tests.cpp
Lines 2107 (patched)


s/for for/for/



src/tests/default_executor_tests.cpp
Lines 2112-2113 (patched)


Could you either merge "true" into the first line of the command, or indent 
the line containing "true" to make it clear that it is part of a single 
function parameter?



src/tests/default_executor_tests.cpp
Lines 2119 (patched)


Could we use a `std::vector` here instead of a C-style array?

i.e.,

```
vector updates(4);
```



src/tests/default_executor_tests.cpp
Lines 2122-2123 (patched)


Could you elaborate on why this variable is necessary?



src/tests/default_executor_tests.cpp
Lines 2133-2134 (patched)


Is this needed?



src/tests/default_executor_tests.cpp
Lines 2204-2205 (patched)


Do you think we should combine these tests into one? The tests are nearly 
the same, so it would eliminate some duplicated code if we combined them, at 
the cost of some ambiguity in the case of test failure.

What do you think?


- Greg Mann


On Aug. 30, 2017, 12:39 a.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61921/
> ---
> 
> (Updated Aug. 30, 2017, 12:39 a.m.)
> 
> 
> Review request for mesos and Greg Mann.
> 
> 
> Bugs: MESOS-7916
> https://issues.apache.org/jira/browse/MESOS-7916
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> These tests verifies that sibling tasks can share a Volume owned by
> their parent executor using 'sandbox_path' volumes.
> 
> 
> Diffs
> -
> 
>   src/tests/default_executor_tests.cpp 
> afe0afabf784fb65eb833beadd3c584722c321e1 
> 
> 
> Diff: https://reviews.apache.org/r/61921/diff/2/
> 
> 
> Testing
> ---
> 
> `sudo bin/mesos-tests.sh --gtest_filter="*TasksSharingViaSandboxVolumes*" 
> --gtest_repeat=500 --gtest_break_on_failure` on GNU/Linux.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 61921: Added tests to ensure that tasks can access their parent's volumes.

[Gastón Kleiman]

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

(Updated Aug. 30, 2017, 12:39 a.m.)


Review request for mesos and Greg Mann.


Changes
---

Rebsae.


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


Repository: mesos


Description
---

These tests verifies that sibling tasks can share a Volume owned by
their parent executor using 'sandbox_path' volumes.


Diffs (updated)
-

  src/tests/default_executor_tests.cpp afe0afabf784fb65eb833beadd3c584722c321e1 


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

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


Testing
---

`sudo bin/mesos-tests.sh --gtest_filter="*TasksSharingViaSandboxVolumes*" 
--gtest_repeat=500 --gtest_break_on_failure` on GNU/Linux.


Thanks,

Gastón Kleiman

Re: Review Request 61799: Replaced ABORT with _EXIT in `childMain` to handle `os::execvpe` error.

[Benjamin Mahler]

> On Aug. 29, 2017, 3:27 p.m., James Peach wrote:
> > 3rdparty/libprocess/src/subprocess_posix.hpp
> > Line 194 (original), 195 (patched)
> > 
> >
> > Hmm. I thought about this some more and I think we should retain the 
> > `strerror` here.
> > 
> > 1. We still used it in other async-signal-safe situations.
> > 2. In glibc it only mallocs when you give it an unknown error number.
> > 3. In practice on Linux, malloc after fork is safe.
> > 4. The error message is pretty useful.
> > 
> > Whay do you think?
> 
> Andrei Budnik wrote:
> Ok, reverted.

> In practice on Linux, malloc after fork is safe.

Is this true? I seem to recall seeing potential deadlocks deadlocks due to 
this. One example was 
[MESOS-7858](https://issues.apache.org/jira/browse/MESOS-7858), where some code 
in glibc was doing an bad assert. Sometimes when this fails, the malloc needed 
for assert to print would deadlock.


- Benjamin


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


On Aug. 21, 2017, 8:54 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61799/
> ---
> 
> (Updated Aug. 21, 2017, 8:54 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Alexander Rojas, Benjamin 
> Bannier, Benjamin Mahler, and James Peach.
> 
> 
> Bugs: MESOS-7791
> https://issues.apache.org/jira/browse/MESOS-7791
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Using ABORT causes coredump when path to an executable is invalid.
> In addition, using of _EXIT allows us to get rid of string
> concatenation, thereby making `childMain` more async-signal safe.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/subprocess_posix.hpp 
> b478beccff22e2ffa08b9b91cfd5b9c6ada9b697 
> 
> 
> Diff: https://reviews.apache.org/r/61799/diff/4/
> 
> 
> Testing
> ---
> 
> sudo make check (mac os x, fedora 25)
> internal CI
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 61801: Used _EXIT macro in `CgroupsAnyHierarchyTest.ROOT_CGROUPS_Write` test.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [61797, 61798, 61799, 61800, 61801]

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

- Mesos Reviewbot


On Aug. 21, 2017, 1:54 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61801/
> ---
> 
> (Updated Aug. 21, 2017, 1:54 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Alexander Rojas, Benjamin 
> Bannier, Benjamin Mahler, and James Peach.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used _EXIT macro in `CgroupsAnyHierarchyTest.ROOT_CGROUPS_Write` test.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/cgroups_tests.cpp 
> c5d83e667da0b2c8c03937c66e2a973d547cd3d2 
> 
> 
> Diff: https://reviews.apache.org/r/61801/diff/3/
> 
> 
> Testing
> ---
> 
> sudo make check (mac os x, fedora 25)
> internal CI
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 61801: Used _EXIT macro in `CgroupsAnyHierarchyTest.ROOT_CGROUPS_Write` test.

[Mesos Reviewbot Windows]

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



Bad patch!

Reviews applied: [61797, 61798, 61799, 61800, 61801]

Logs available here: http://104.210.40.105/logs/master/61801

- Mesos Reviewbot Windows


On Aug. 21, 2017, 8:54 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61801/
> ---
> 
> (Updated Aug. 21, 2017, 8:54 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Alexander Rojas, Benjamin 
> Bannier, Benjamin Mahler, and James Peach.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used _EXIT macro in `CgroupsAnyHierarchyTest.ROOT_CGROUPS_Write` test.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/cgroups_tests.cpp 
> c5d83e667da0b2c8c03937c66e2a973d547cd3d2 
> 
> 
> Diff: https://reviews.apache.org/r/61801/diff/3/
> 
> 
> Testing
> ---
> 
> sudo make check (mac os x, fedora 25)
> internal CI
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 61799: Replaced ABORT with _EXIT in `childMain` to handle `os::execvpe` error.

[Andrei Budnik]

> On Aug. 29, 2017, 3:27 p.m., James Peach wrote:
> > 3rdparty/libprocess/src/subprocess_posix.hpp
> > Line 194 (original), 195 (patched)
> > 
> >
> > Hmm. I thought about this some more and I think we should retain the 
> > `strerror` here.
> > 
> > 1. We still used it in other async-signal-safe situations.
> > 2. In glibc it only mallocs when you give it an unknown error number.
> > 3. In practice on Linux, malloc after fork is safe.
> > 4. The error message is pretty useful.
> > 
> > Whay do you think?

Ok, reverted.


- Andrei


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


On Aug. 21, 2017, 8:54 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61799/
> ---
> 
> (Updated Aug. 21, 2017, 8:54 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Alexander Rojas, Benjamin 
> Bannier, Benjamin Mahler, and James Peach.
> 
> 
> Bugs: MESOS-7791
> https://issues.apache.org/jira/browse/MESOS-7791
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Using ABORT causes coredump when path to an executable is invalid.
> In addition, using of _EXIT allows us to get rid of string
> concatenation, thereby making `childMain` more async-signal safe.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/subprocess_posix.hpp 
> b478beccff22e2ffa08b9b91cfd5b9c6ada9b697 
> 
> 
> Diff: https://reviews.apache.org/r/61799/diff/4/
> 
> 
> Testing
> ---
> 
> sudo make check (mac os x, fedora 25)
> internal CI
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 61798: Added _EXIT as alternative to ABORT.

[Andrei Budnik]

> On Aug. 29, 2017, 2:47 p.m., James Peach wrote:
> > 3rdparty/stout/include/stout/exit.hpp
> > Lines 37 (patched)
> > 
> >
> > What do you think about re-declaring `google::RawLog__` here with the 
> > [format 
> > attribute](https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-format-function-attribute)?
> > 
> > It is a little gnarly, but worth it IME.

`RAW_LOG` 
(https://github.com/google/glog/blob/master/src/glog/raw_logging.h.in#L172) 
checks format attributes already.


> On Aug. 29, 2017, 2:47 p.m., James Peach wrote:
> > 3rdparty/stout/include/stout/exit.hpp
> > Lines 44 (patched)
> > 
> >
> > Can we make this emit the same format as `EXIT()`?
> > 
> > ```
> > #define _Exit(status, fmt, ...)
> > ...
> >   RAW_LOG(level, "Exit with status %d: " fmt, status ...);
> > ...
> > ```

Done.


- Andrei


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


On Aug. 21, 2017, 8:54 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61798/
> ---
> 
> (Updated Aug. 21, 2017, 8:54 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Alexander Rojas, Benjamin 
> Bannier, Benjamin Mahler, and James Peach.
> 
> 
> Bugs: MESOS-7791
> https://issues.apache.org/jira/browse/MESOS-7791
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> EXIT macro can't be used when async-signal safety is required.
> 
> Currently, ABORT is used for terminating a program after both
> unexpected and expected errors, when async-signal safety is required.
> In case of expected errors, `abort` causes coredumps, which might be
> undesirable.
> 
> In addition, _EXIT supports formatted output conversion,
> thereby _EXIT interface doesn't force users to concatenate strings,
> thus making its usage more async-signal safe.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/exit.hpp 
> e5c2d3440a85ab2d2cae551ee4094cd965e38dfc 
> 
> 
> Diff: https://reviews.apache.org/r/61798/diff/4/
> 
> 
> Testing
> ---
> 
> sudo make check (mac os x, fedora 25)
> internal CI
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 61849: Improved consistency of cout/cerr and glog usage in main functions.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [61849]

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

- Mesos Reviewbot


On Aug. 29, 2017, 3:56 p.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61849/
> ---
> 
> (Updated Aug. 29, 2017, 3:56 p.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Alexander Rukletsov.
> 
> 
> Bugs: MESOS-7586
> https://issues.apache.org/jira/browse/MESOS-7586
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The main functions now have a common initialization pattern:
>   1. Load flags.
>   2. Check if the --help flag has been passed.
>   3. Check if the flags are correct.
>   4. Initialize logging.
>   5. Parse and setup the environment variables.
>   6. Initialize libprocess.
> 
> This change reduces the number of messages "WARNING: Logging before
> InitGoogleLogging() is written to STDERR" as we now use glog only after
> the fourth step. This forces all glog messages to end up in a logdir.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp aff97939b088e3747111ca6ec00fe76c59e9b396 
>   src/launcher/default_executor.cpp 30bae5c931c81b81ff9590eea8051b2232388813 
>   src/launcher/executor.cpp 65b9a2c52ba5a5a514fff992018e356f388468fc 
>   src/local/main.cpp f0d7e8ca1a64b5409632ea0c0894a1dc84f9796e 
>   src/master/main.cpp 1a78a55fd52c5eebad689c2a7f16c95979260d79 
>   src/slave/main.cpp a4a8ced6825ece1b25003824d987ff83fb799ad7 
> 
> 
> Diff: https://reviews.apache.org/r/61849/diff/5/
> 
> 
> Testing
> ---
> 
> ```
> $ make check
> ```
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 61965: Moved `--zk_session_timeout` flag docs to the common flags section.

[Andrew Schwartzmeyer]

> On Aug. 29, 2017, 7:14 a.m., Mesos Reviewbot Windows wrote:
> > Bad patch!
> > 
> > Reviews applied: [61689, 61690, 61965]
> > 
> > Logs available here: http://104.210.40.105/logs/master/61965
> 
> Ilya Pronin wrote:
> No space left on the build host:
> ```
> (ClCompile target) ->
>   C:\mesos\build\master\61965\mesos\src\master\master.cpp(9865): fatal 
> error C1088: Cannot flush compiler intermediate file: 
> 'C:\Users\mesos\AppData\Local\Temp\2_CL_cc2c1dd7gl': No space left on device 
> [C:\mesos\build\master\61965\build\src\mesos-1.5.0.vcxproj]
>   C:\mesos\build\master\61965\mesos\src\scheduler\scheduler.cpp(878): 
> fatal error C1088: Cannot flush compiler intermediate file: 
> 'C:\Users\mesos\AppData\Local\Temp\2_CL_c70ddc2fgl': No space left on device 
> [C:\mesos\build\master\61965\build\src\mesos-1.5.0.vcxproj]
>   C:\mesos\build\master\61965\mesos\src\uri\fetchers\curl.cpp(177): fatal 
> error C1088: Cannot flush compiler intermediate file: 
> 'C:\Users\mesos\AppData\Local\Temp\2_CL_de54a61agl': No space left on device 
> [C:\mesos\build\master\61965\build\src\mesos-1.5.0.vcxproj]
>   
> C:\mesos\build\master\61965\mesos\src\slave\containerizer\mesos\isolators\filesystem\posix.cpp(290):
>  error C2471: cannot update program database 
> 'C:\mesos\build\master\61965\build\src\mesos-1.5.0.dir\Debug\mesos-1.5.0.pdb' 
> [C:\mesos\build\master\61965\build\src\mesos-1.5.0.vcxproj]
>   
> C:\mesos\build\master\61965\mesos\src\slave\containerizer\mesos\isolators\environment_secret.cpp(139):
>  fatal error C1088: Cannot flush compiler intermediate file: 
> 'C:\Users\mesos\AppData\Local\Temp\2_CL_886abca7sy': No space left on device 
> [C:\mesos\build\master\61965\build\src\mesos-1.5.0.vcxproj]
>   C:\Program Files (x86)\Microsoft Visual 
> Studio\2017\Community\VC\Tools\MSVC\14.10.25017\include\tuple(423): fatal 
> error C1088: Cannot flush compiler intermediate file: 
> 'C:\Users\mesos\AppData\Local\Temp\2_CL_838792afex': No space left on device 
> (compiling source file 
> C:\mesos\build\master\61965\mesos\src\zookeeper\group.cpp) 
> [C:\mesos\build\master\61965\build\src\mesos-1.5.0.vcxproj]
> 
> 193 Warning(s)
> 6 Error(s)
> ```

Ah, interesting. I've emailed Mihai about it.


- Andrew


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


On Aug. 29, 2017, 5:51 a.m., Ilya Pronin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61965/
> ---
> 
> (Updated Aug. 29, 2017, 5:51 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7895
> https://issues.apache.org/jira/browse/MESOS-7895
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Mesos agents now accept this flag.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 9cadf0f7559a3aeba3f35477e8f7eac67d3fa5cf 
> 
> 
> Diff: https://reviews.apache.org/r/61965/diff/1/
> 
> 
> Testing
> ---
> 
> N/A
> 
> 
> Thanks,
> 
> Ilya Pronin
> 
>

Re: Review Request 61849: Improved consistency of cout/cerr and glog usage in main functions.

[Mesos Reviewbot Windows]

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



Patch looks great!

Reviews applied: [61849]

Logs available here: http://104.210.40.105/logs/master/61849

- Mesos Reviewbot Windows


On Aug. 29, 2017, 3:56 p.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61849/
> ---
> 
> (Updated Aug. 29, 2017, 3:56 p.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Alexander Rukletsov.
> 
> 
> Bugs: MESOS-7586
> https://issues.apache.org/jira/browse/MESOS-7586
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The main functions now have a common initialization pattern:
>   1. Load flags.
>   2. Check if the --help flag has been passed.
>   3. Check if the flags are correct.
>   4. Initialize logging.
>   5. Parse and setup the environment variables.
>   6. Initialize libprocess.
> 
> This change reduces the number of messages "WARNING: Logging before
> InitGoogleLogging() is written to STDERR" as we now use glog only after
> the fourth step. This forces all glog messages to end up in a logdir.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp aff97939b088e3747111ca6ec00fe76c59e9b396 
>   src/launcher/default_executor.cpp 30bae5c931c81b81ff9590eea8051b2232388813 
>   src/launcher/executor.cpp 65b9a2c52ba5a5a514fff992018e356f388468fc 
>   src/local/main.cpp f0d7e8ca1a64b5409632ea0c0894a1dc84f9796e 
>   src/master/main.cpp 1a78a55fd52c5eebad689c2a7f16c95979260d79 
>   src/slave/main.cpp a4a8ced6825ece1b25003824d987ff83fb799ad7 
> 
> 
> Diff: https://reviews.apache.org/r/61849/diff/5/
> 
> 
> Testing
> ---
> 
> ```
> $ make check
> ```
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 61849: Improved consistency of cout/cerr and glog usage in main functions.

[Armand Grillet]

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

(Updated Aug. 29, 2017, 3:56 p.m.)


Review request for mesos, Andrei Budnik and Alexander Rukletsov.


Changes
---

Fixed issue and rebased.


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


Repository: mesos


Description
---

The main functions now have a common initialization pattern:
  1. Load flags.
  2. Check if the --help flag has been passed.
  3. Check if the flags are correct.
  4. Initialize logging.
  5. Parse and setup the environment variables.
  6. Initialize libprocess.

This change reduces the number of messages "WARNING: Logging before
InitGoogleLogging() is written to STDERR" as we now use glog only after
the fourth step. This forces all glog messages to end up in a logdir.


Diffs (updated)
-

  src/docker/executor.cpp aff97939b088e3747111ca6ec00fe76c59e9b396 
  src/launcher/default_executor.cpp 30bae5c931c81b81ff9590eea8051b2232388813 
  src/launcher/executor.cpp 65b9a2c52ba5a5a514fff992018e356f388468fc 
  src/local/main.cpp f0d7e8ca1a64b5409632ea0c0894a1dc84f9796e 
  src/master/main.cpp 1a78a55fd52c5eebad689c2a7f16c95979260d79 
  src/slave/main.cpp a4a8ced6825ece1b25003824d987ff83fb799ad7 


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

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


Testing
---

```
$ make check
```


Thanks,

Armand Grillet

Re: Review Request 61666: Added test to verify filtering of resource reservations & allocations.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [60915, 61666]

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

- Mesos Reviewbot


On Aug. 15, 2017, 5:59 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61666/
> ---
> 
> (Updated Aug. 15, 2017, 5:59 p.m.)
> 
> 
> Review request for mesos, Alexander Rojas and Greg Mann.
> 
> 
> Bugs: MESOS-7892
> https://issues.apache.org/jira/browse/MESOS-7892
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch extends `SlaveAuthorizerTest.FilterStateEndpoint` test to
> check that agent's `/state` endpoint shows resource reservations and
> allocations based on the 'VIEW_ROLE' permissions of the principal
> doing the request.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_authorization_tests.cpp 
> 536a8efda0cc1ba08285c379b01af6ec64a82117 
> 
> 
> Diff: https://reviews.apache.org/r/61666/diff/4/
> 
> 
> Testing
> ---
> 
> make check (mac os x, fedora 25)
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 61849: Improved consistency of cout/cerr and glog usage in main functions.

[Till Toenshoff]

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


Fix it, then Ship it!




Thanks for this cleanup.

We should consider cleaning up the examples as well. Also we should consider 
adding propper logging-flags to the logrotate runnable as it uses GLOG as well. 
However, that is kinda out of scope of this patch and can be solved 
successively, I feel. Could you please create a JIRA for the logrotate runnable 
and maybe another one for cleaning up the examples? The latter should be marked 
with lowest possible priority ;).


src/launcher/default_executor.cpp
Lines 1476 (patched)


You moved the Flags down to their first use, but left the other prematurely 
instantiated objects up - can you elaborate this preference?


- Till Toenshoff


On Aug. 25, 2017, 5:34 p.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61849/
> ---
> 
> (Updated Aug. 25, 2017, 5:34 p.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Alexander Rukletsov.
> 
> 
> Bugs: MESOS-7586
> https://issues.apache.org/jira/browse/MESOS-7586
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The main functions now have a common initialization pattern:
>   1. Load flags.
>   2. Check if the --help flag has been passed.
>   3. Check if the flags are correct.
>   4. Initialize logging.
>   5. Parse and setup the environment variables.
>   6. Initialize libprocess.
> 
> This change reduces the number of messages "WARNING: Logging before
> InitGoogleLogging() is written to STDERR" as we now use glog only after
> the fourth step. This forces all glog messages to end up in a logdir.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp aff97939b088e3747111ca6ec00fe76c59e9b396 
>   src/launcher/default_executor.cpp 30bae5c931c81b81ff9590eea8051b2232388813 
>   src/launcher/executor.cpp 65b9a2c52ba5a5a514fff992018e356f388468fc 
>   src/local/main.cpp f0d7e8ca1a64b5409632ea0c0894a1dc84f9796e 
>   src/master/main.cpp 1a78a55fd52c5eebad689c2a7f16c95979260d79 
>   src/slave/main.cpp a4a8ced6825ece1b25003824d987ff83fb799ad7 
> 
> 
> Diff: https://reviews.apache.org/r/61849/diff/4/
> 
> 
> Testing
> ---
> 
> ```
> $ make check
> ```
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 61799: Replaced ABORT with _EXIT in `childMain` to handle `os::execvpe` error.

[James Peach]

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


Fix it, then Ship it!





3rdparty/libprocess/src/subprocess_posix.hpp
Line 194 (original), 195 (patched)


Hmm. I thought about this some more and I think we should retain the 
`strerror` here.

1. We still used it in other async-signal-safe situations.
2. In glibc it only mallocs when you give it an unknown error number.
3. In practice on Linux, malloc after fork is safe.
4. The error message is pretty useful.

Whay do you think?


- James Peach


On Aug. 21, 2017, 8:54 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61799/
> ---
> 
> (Updated Aug. 21, 2017, 8:54 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Alexander Rojas, Benjamin 
> Bannier, Benjamin Mahler, and James Peach.
> 
> 
> Bugs: MESOS-7791
> https://issues.apache.org/jira/browse/MESOS-7791
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Using ABORT causes coredump when path to an executable is invalid.
> In addition, using of _EXIT allows us to get rid of string
> concatenation, thereby making `childMain` more async-signal safe.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/subprocess_posix.hpp 
> b478beccff22e2ffa08b9b91cfd5b9c6ada9b697 
> 
> 
> Diff: https://reviews.apache.org/r/61799/diff/3/
> 
> 
> Testing
> ---
> 
> sudo make check (mac os x, fedora 25)
> internal CI
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 61797: Added async-signal safe implementation of `write` function in stout.

[James Peach]

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


Ship it!




Ship It!

- James Peach


On Aug. 21, 2017, 8:54 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61797/
> ---
> 
> (Updated Aug. 21, 2017, 8:54 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Alexander Rojas, Benjamin 
> Bannier, Benjamin Mahler, and James Peach.
> 
> 
> Bugs: MESOS-7791
> https://issues.apache.org/jira/browse/MESOS-7791
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Async-signal safe `write` function, which can take variable number of
> arguments, should be used to improve signal safety. Async-signal safe
> `write` can't return `Try`, because it might cause a memory
> allocation, so we've introduced a set of async-signal safe `write`
> functions which are defined in `signal_safe` namespace. The new `write`
> mimics POSIX `write` for returning status and error.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/abort.hpp 
> d3291dce8ec3db6141124e9cd231d34409998a18 
>   3rdparty/stout/include/stout/os/write.hpp 
> beb5bd83b52565a75e34d32b5bb17951bc799578 
> 
> 
> Diff: https://reviews.apache.org/r/61797/diff/6/
> 
> 
> Testing
> ---
> 
> sudo make check (mac os x, fedora 25)
> internal CI
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 61798: Added _EXIT as alternative to ABORT.

[James Peach]

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


Fix it, then Ship it!





3rdparty/stout/include/stout/exit.hpp
Lines 37 (patched)


What do you think about re-declaring `google::RawLog__` here with the 
[format 
attribute](https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-format-function-attribute)?

It is a little gnarly, but worth it IME.



3rdparty/stout/include/stout/exit.hpp
Lines 44 (patched)


Can we make this emit the same format as `EXIT()`?

```
#define _Exit(status, fmt, ...)
...
  RAW_LOG(level, "Exit with status %d: " fmt, status ...);
...
```


- James Peach


On Aug. 21, 2017, 8:54 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61798/
> ---
> 
> (Updated Aug. 21, 2017, 8:54 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Alexander Rojas, Benjamin 
> Bannier, Benjamin Mahler, and James Peach.
> 
> 
> Bugs: MESOS-7791
> https://issues.apache.org/jira/browse/MESOS-7791
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> EXIT macro can't be used when async-signal safety is required.
> 
> Currently, ABORT is used for terminating a program after both
> unexpected and expected errors, when async-signal safety is required.
> In case of expected errors, `abort` causes coredumps, which might be
> undesirable.
> 
> In addition, _EXIT supports formatted output conversion,
> thereby _EXIT interface doesn't force users to concatenate strings,
> thus making its usage more async-signal safe.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/exit.hpp 
> e5c2d3440a85ab2d2cae551ee4094cd965e38dfc 
> 
> 
> Diff: https://reviews.apache.org/r/61798/diff/3/
> 
> 
> Testing
> ---
> 
> sudo make check (mac os x, fedora 25)
> internal CI
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 61666: Added test to verify filtering of resource reservations & allocations.

[Andrei Budnik]

> On Aug. 29, 2017, 11:55 a.m., Alexander Rojas wrote:
> > src/tests/slave_authorization_tests.cpp
> > Lines 104-105 (original), 104-105 (patched)
> > 
> >
> > Remove this, it is not relevant anymore.

I've updated the checks. So, this comment is relevant again.


> On Aug. 29, 2017, 11:55 a.m., Alexander Rojas wrote:
> > src/tests/slave_authorization_tests.cpp
> > Lines 106-109 (patched)
> > 
> >
> > Mention that two frameworks are launched with one task on each.

Done.


> On Aug. 29, 2017, 11:55 a.m., Alexander Rojas wrote:
> > src/tests/slave_authorization_tests.cpp
> > Line 168 (original), 197 (patched)
> > 
> >
> > I'm wondering if instead of _framework1_ and _framework2_, we can name 
> > it _frameworkSuperhero_ and _frameworkMuggle_. It will make everything 
> > easier to read.
> > 
> > Same for executors and tasks.

Fixed.


> On Aug. 29, 2017, 11:55 a.m., Alexander Rojas wrote:
> > src/tests/slave_authorization_tests.cpp
> > Line 212 (original), 277 (patched)
> > 
> >
> > Perhaps a comment indicating that this task will run on executor1 of 
> > framework1

Done.


- Andrei


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


On Aug. 15, 2017, 5:59 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61666/
> ---
> 
> (Updated Aug. 15, 2017, 5:59 p.m.)
> 
> 
> Review request for mesos, Alexander Rojas and Greg Mann.
> 
> 
> Bugs: MESOS-7892
> https://issues.apache.org/jira/browse/MESOS-7892
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch extends `SlaveAuthorizerTest.FilterStateEndpoint` test to
> check that agent's `/state` endpoint shows resource reservations and
> allocations based on the 'VIEW_ROLE' permissions of the principal
> doing the request.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_authorization_tests.cpp 
> 536a8efda0cc1ba08285c379b01af6ec64a82117 
> 
> 
> Diff: https://reviews.apache.org/r/61666/diff/4/
> 
> 
> Testing
> ---
> 
> make check (mac os x, fedora 25)
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 61965: Moved `--zk_session_timeout` flag docs to the common flags section.

[Ilya Pronin]

> On Aug. 29, 2017, 3:14 p.m., Mesos Reviewbot Windows wrote:
> > Bad patch!
> > 
> > Reviews applied: [61689, 61690, 61965]
> > 
> > Logs available here: http://104.210.40.105/logs/master/61965

No space left on the build host:
```
(ClCompile target) ->
  C:\mesos\build\master\61965\mesos\src\master\master.cpp(9865): fatal error 
C1088: Cannot flush compiler intermediate file: 
'C:\Users\mesos\AppData\Local\Temp\2_CL_cc2c1dd7gl': No space left on device 
[C:\mesos\build\master\61965\build\src\mesos-1.5.0.vcxproj]
  C:\mesos\build\master\61965\mesos\src\scheduler\scheduler.cpp(878): fatal 
error C1088: Cannot flush compiler intermediate file: 
'C:\Users\mesos\AppData\Local\Temp\2_CL_c70ddc2fgl': No space left on device 
[C:\mesos\build\master\61965\build\src\mesos-1.5.0.vcxproj]
  C:\mesos\build\master\61965\mesos\src\uri\fetchers\curl.cpp(177): fatal error 
C1088: Cannot flush compiler intermediate file: 
'C:\Users\mesos\AppData\Local\Temp\2_CL_de54a61agl': No space left on device 
[C:\mesos\build\master\61965\build\src\mesos-1.5.0.vcxproj]
  
C:\mesos\build\master\61965\mesos\src\slave\containerizer\mesos\isolators\filesystem\posix.cpp(290):
 error C2471: cannot update program database 
'C:\mesos\build\master\61965\build\src\mesos-1.5.0.dir\Debug\mesos-1.5.0.pdb' 
[C:\mesos\build\master\61965\build\src\mesos-1.5.0.vcxproj]
  
C:\mesos\build\master\61965\mesos\src\slave\containerizer\mesos\isolators\environment_secret.cpp(139):
 fatal error C1088: Cannot flush compiler intermediate file: 
'C:\Users\mesos\AppData\Local\Temp\2_CL_886abca7sy': No space left on device 
[C:\mesos\build\master\61965\build\src\mesos-1.5.0.vcxproj]
  C:\Program Files (x86)\Microsoft Visual 
Studio\2017\Community\VC\Tools\MSVC\14.10.25017\include\tuple(423): fatal error 
C1088: Cannot flush compiler intermediate file: 
'C:\Users\mesos\AppData\Local\Temp\2_CL_838792afex': No space left on device 
(compiling source file 
C:\mesos\build\master\61965\mesos\src\zookeeper\group.cpp) 
[C:\mesos\build\master\61965\build\src\mesos-1.5.0.vcxproj]

193 Warning(s)
6 Error(s)
```


- Ilya


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


On Aug. 29, 2017, 1:51 p.m., Ilya Pronin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61965/
> ---
> 
> (Updated Aug. 29, 2017, 1:51 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7895
> https://issues.apache.org/jira/browse/MESOS-7895
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Mesos agents now accept this flag.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 9cadf0f7559a3aeba3f35477e8f7eac67d3fa5cf 
> 
> 
> Diff: https://reviews.apache.org/r/61965/diff/1/
> 
> 
> Testing
> ---
> 
> N/A
> 
> 
> Thanks,
> 
> Ilya Pronin
> 
>

Re: Review Request 61965: Moved `--zk_session_timeout` flag docs to the common flags section.

[Mesos Reviewbot Windows]

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



Bad patch!

Reviews applied: [61689, 61690, 61965]

Logs available here: http://104.210.40.105/logs/master/61965

- Mesos Reviewbot Windows


On Aug. 29, 2017, 5:51 a.m., Ilya Pronin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61965/
> ---
> 
> (Updated Aug. 29, 2017, 5:51 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7895
> https://issues.apache.org/jira/browse/MESOS-7895
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Mesos agents now accept this flag.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 9cadf0f7559a3aeba3f35477e8f7eac67d3fa5cf 
> 
> 
> Diff: https://reviews.apache.org/r/61965/diff/1/
> 
> 
> Testing
> ---
> 
> N/A
> 
> 
> Thanks,
> 
> Ilya Pronin
> 
>

Re: Review Request 58021: Added storage-related offer operations.

[Jan Schlicht]

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

(Updated Aug. 29, 2017, 3:23 p.m.)


Review request for mesos, Benjamin Bannier and Jie Yu.


Changes
---

Addressed issues.


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


Repository: mesos


Description
---

Added storage-related offer operations.


Diffs (updated)
-

  include/mesos/mesos.proto 86936cf9ad9406fa4c3358ccb3c58e2f3259d65f 
  include/mesos/v1/mesos.proto 20d9ecff68465c6517a5a239a4bd0f42f82d0cc6 
  src/common/protobuf_utils.cpp 3ae68e93a985a4cfe23be9c9bd8f92e418102a39 
  src/common/resources.cpp 8d4388935ada6be60448e6f6a88db0e5fc4ad4a1 
  src/common/resources_utils.cpp 821bd0967d55f7fbcb57a4efae5fc390af53ca79 
  src/master/master.cpp 183f53070848853b50bfb02bcd6b458ac0b2f685 
  src/v1/resources.cpp 508f3f85c8388a41f57f964b0f6df3b4708d3442 


Diff: https://reviews.apache.org/r/58021/diff/6/

Changes: https://reviews.apache.org/r/58021/diff/5-6/


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 60890: WIP: Defined API for launching standalone containers.

[James DeFelice]

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




include/mesos/agent/agent.proto
Line 62 (original), 62 (patched)


curious, why the British spelling "favour" instead of "favor"?



include/mesos/agent/agent.proto
Line 184 (original), 248 (patched)


should these `xxxNestedContainer` optional fields be deprecated as well?



include/mesos/agent/agent.proto
Line 354 (original), 427 (patched)


deprecated?


- James DeFelice


On Aug. 29, 2017, 12:26 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60890/
> ---
> 
> (Updated Aug. 29, 2017, 12:26 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-7305
> https://issues.apache.org/jira/browse/MESOS-7305
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Launching a standalone container is very similar to launching a
> nested container, except that the caller must specify some Resources.
> As such, this patch deprecates some nested container APIs
> and replaces them with more generically named APIs.
> 
> This applies to the Launch, Wait, and Kill (nested) container APIs.
> 
> 
> Diffs
> -
> 
>   include/mesos/agent/agent.proto 7c8c8a7d8298e91e4e002327b3b27d4c74b5cbae 
>   include/mesos/v1/agent/agent.proto 3e199124b23fa027232790d99370fe2f33660096 
> 
> 
> Diff: https://reviews.apache.org/r/60890/diff/2/
> 
> 
> Testing
> ---
> 
> make
> 
> See later patches in chain.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Review Request 61965: Moved `--zk_session_timeout` flag docs to the common flags section.

[Ilya Pronin]

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

Review request for mesos and Jiang Yan Xu.


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


Repository: mesos


Description
---

Mesos agents now accept this flag.


Diffs
-

  docs/configuration.md 9cadf0f7559a3aeba3f35477e8f7eac67d3fa5cf 


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


Testing
---

N/A


Thanks,

Ilya Pronin

Re: Review Request 61946: Added validation of resource provider operations.

[Jan Schlicht]

> On Aug. 28, 2017, 11:31 p.m., Jie Yu wrote:
> > src/master/validation.cpp
> > Lines 2205 (patched)
> > 
> >
> > I think `checkpointedResources` should not be used for Resource 
> > Provider provided resources. It should only apply to agent default 
> > resources. The checkpointing should be done by the corresponding resource 
> > provider, not the agent for RP provided resources.
> > 
> > As a result, for operations like RESERVE/UNRESERVE/CREATE/DESTROY, we 
> > need to send operation to the corresponding resource provider as well. This 
> > does make sense. If we ask agent to persist those information, what will be 
> > the semantics if the resource provider is marked as gone?
> > 
> > However, this does get complicated if we want to guarantee ordering for 
> > operations in one `acceptOffers` call (for backwards compatibility), and we 
> > do want to allow frameworks to launch a task right after reserve operation 
> > (the current semantics).
> > 
> > To support that, I think we need to speculatively assume the operation 
> > will be sucessful (thus allow a subsequent launch immediately at the master 
> > side). However, when the checkpointing fails, we need a way to abort the 
> > subsequent launch at the agent side. This is essentially why we CHECK fail 
> > if the checkpointing fails at the agent previously for 
> > `checkpointedResources`.
> > 
> > For the resource provider case, we should do the same thing. We can 
> > abort the agent if a checkpointing fails. However, this only applies to the 
> > local resource provider that lives in the agent process. If a LRP is 
> > outside of the agent process, how to abort the subsequent task launch if a 
> > previous operation fails is something we should think about. For instance, 
> > always reject operations from the agent's RP manager if the operation is 
> > for a stale stream ID?

Fully agreed, thanks for bringing up the challenged with handling 
`RESERVE`/`UNRESERVE`/`CREATE`/`DESTROY` with local and external resource 
providers. An idea for solving this with external resource providers could be 
to rescind a launch, similar to how we rescind offers. E.g. an ERP would send a 
rescind message to the master which then instructs the agent to stop the launch.


- Jan


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


On Aug. 28, 2017, 5:28 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61946/
> ---
> 
> (Updated Aug. 28, 2017, 5:28 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added validation of resource provider operations.
> 
> 
> Diffs
> -
> 
>   src/master/validation.hpp f4925752f20ae8ca4de1d9b4a3d5ffc394db9585 
>   src/master/validation.cpp 7c3247d407c9e6aa8cce457d6c6be0c39f4b532f 
> 
> 
> Diff: https://reviews.apache.org/r/61946/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 61666: Added test to verify filtering of resource reservations & allocations.

[Alexander Rojas]

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




src/tests/slave_authorization_tests.cpp
Lines 104-105 (original), 104-105 (patched)


Remove this, it is not relevant anymore.



src/tests/slave_authorization_tests.cpp
Lines 106-109 (patched)


Mention that two frameworks are launched with one task on each.



src/tests/slave_authorization_tests.cpp
Line 168 (original), 197 (patched)


I'm wondering if instead of _framework1_ and _framework2_, we can name it 
_frameworkSuperhero_ and _frameworkMuggle_. It will make everything easier to 
read.

Same for executors and tasks.



src/tests/slave_authorization_tests.cpp
Line 212 (original), 277 (patched)


Perhaps a comment indicating that this task will run on executor1 of 
framework1


- Alexander Rojas


On Aug. 15, 2017, 7:59 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61666/
> ---
> 
> (Updated Aug. 15, 2017, 7:59 p.m.)
> 
> 
> Review request for mesos, Alexander Rojas and Greg Mann.
> 
> 
> Bugs: MESOS-7892
> https://issues.apache.org/jira/browse/MESOS-7892
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch extends `SlaveAuthorizerTest.FilterStateEndpoint` test to
> check that agent's `/state` endpoint shows resource reservations and
> allocations based on the 'VIEW_ROLE' permissions of the principal
> doing the request.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_authorization_tests.cpp 
> 536a8efda0cc1ba08285c379b01af6ec64a82117 
> 
> 
> Diff: https://reviews.apache.org/r/61666/diff/3/
> 
> 
> Testing
> ---
> 
> make check (mac os x, fedora 25)
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 61189: Added authorization for V1 events.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [61262, 61189]

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

- Mesos Reviewbot


On Aug. 18, 2017, 8:33 p.m., Quinn Leng wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61189/
> ---
> 
> (Updated Aug. 18, 2017, 8:33 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Alexander Rojas, and Greg Mann.
> 
> 
> Bugs: MESOS-7785
> https://issues.apache.org/jira/browse/MESOS-7785
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added authorization filtering for the V1 operator events on the
> master, the subscriber should only receive events that are
> authorized based on their principal and ACLs. Since the authorizer
> is called for every event emitted on the stream, the change of
> authorization rule will affect events that the subscribers can
> receive.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp b09a9d0ab5fab179d0e400932dce7d3ca958d7a8 
>   src/master/master.hpp d9cfc42646c874661e4ec79322126d93a2caf604 
>   src/master/master.cpp 7f38a5e21884546d4b4c866ca5918db779af8f99 
>   src/tests/api_tests.cpp 34480ea6818c904c64cb678562866f0b43dae0d6 
> 
> 
> Diff: https://reviews.apache.org/r/61189/diff/6/
> 
> 
> Testing
> ---
> 
> make check
> GLOG_v=2 ./bin/mesos-tests.sh 
> --gtest_filter="ContentType/MasterAPITest.EventsAuthorizationFiltering*" 
> --verbose --gtest_repeat=100 --gtest_break_on_failure
> 
> 
> Thanks,
> 
> Quinn Leng
> 
>