Re: Review Request 52866: Refactored HealthChecker::reschedule to take duration as an argument.

[Benjamin Mahler]

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


Ship it!





src/health-check/health_checker.hpp (line 122)


maybe call this `schedule()`?


- Benjamin Mahler


On Nov. 14, 2016, 10:35 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52866/
> ---
> 
> (Updated Nov. 14, 2016, 10:35 a.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and haosdent huang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To facilitate code reuse, HealthChecker::reschedule() is generalized.
> This will become even more valuable when we add pause/resume functions.
> 
> 
> Diffs
> -
> 
>   src/health-check/health_checker.hpp 
> a1dc72493ff31b87068d5691f4d5b794392caf76 
>   src/health-check/health_checker.cpp 
> e2b32e2d57515202f547d12ba492ad8eb633641b 
> 
> Diff: https://reviews.apache.org/r/52866/diff/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/52873/.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 52866: Refactored HealthChecker::reschedule to take duration as an argument.

[Benjamin Mahler]

> On Oct. 21, 2016, 7:35 p.m., Benjamin Mahler wrote:
> > src/health-check/health_checker.cpp, line 188
> > 
> >
> > Isn't this going to lead to some slightly confusing logging where we 
> > say "Rescheduling" for the first health check?
> 
> Alexander Rukletsov wrote:
> ```
> I 15:07:43.066488 1601536 health_checker.cpp:193] Health check 
> starting in 0ns, grace period 0ns
> I 15:07:43.066573 1601536 health_checker.cpp:618] Rescheduling health 
> check in 0ns
> W 15:07:43.158612 3747840 health_checker.cpp:215] Health check failed 
> 1 times consecutively: COMMAND health check failed: Command returned exited 
> with status 1
> I 15:07:43.158704 3747840 health_checker.cpp:618] Rescheduling health 
> check in 0ns
> ```
> I don't think it is very confusing, but if you have a strong opinion, 
> I'll change that.

Could we ensure (separately from this change) that we log the entire health 
check config when using vlog level 1?

Seems like it would be an easy improvement to go from 
s/Rescheduling/Scheduling/ for every instance of this log line?


- Benjamin


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


On Nov. 14, 2016, 10:35 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52866/
> ---
> 
> (Updated Nov. 14, 2016, 10:35 a.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and haosdent huang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To facilitate code reuse, HealthChecker::reschedule() is generalized.
> This will become even more valuable when we add pause/resume functions.
> 
> 
> Diffs
> -
> 
>   src/health-check/health_checker.hpp 
> a1dc72493ff31b87068d5691f4d5b794392caf76 
>   src/health-check/health_checker.cpp 
> e2b32e2d57515202f547d12ba492ad8eb633641b 
> 
> Diff: https://reviews.apache.org/r/52866/diff/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/52873/.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 53837: Added a per container mesos-io-switchboard process.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [53704, 53837]

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 Nov. 18, 2016, 5:07 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53837/
> ---
> 
> (Updated Nov. 18, 2016, 5:07 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Jie Yu, and Vinod Kone.
> 
> 
> Bugs: MESOS-6467
> https://issues.apache.org/jira/browse/MESOS-6467
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, this process simply intercepts the stdout/stderr of a
> container and writes it to the stdout/stderr FDs set up by the
> container logger. In future commits, this process will be expanded to
> allow dynamically attaching new input / output sources to a container
> to support `docker attach` like functionality for a running container.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 5a47c93388234a68c3c486a021ccdbe3213c5bac 
>   src/slave/containerizer/mesos/containerizer.cpp 
> ec4ae32485a7ab6c9f73c512004d1220482a188e 
>   src/slave/containerizer/mesos/io_switchboard/io_switchboard.hpp 
> PRE-CREATION 
>   src/slave/containerizer/mesos/io_switchboard/io_switchboard.cpp 
> PRE-CREATION 
>   src/slave/containerizer/mesos/io_switchboard/main.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/io_switchboard/main.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/53837/diff/
> 
> 
> Testing
> ---
> 
> GTEST_FILTER="" make -j check
> sudo src/mesos-tests
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 52865: Refactored HealthChecker to never stop health checking.

[Benjamin Mahler]

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


Ship it!




We might want to update the description of the commit to mention where we're 
going with this change (i.e. allow health checks to run forever and let the 
scheduler make the decision about how to deal with unhealthy tasks).


src/health-check/health_checker.hpp (line 71)


How about '`initialize`'?

Later, when you introduce pause/resume functionality, we could call them 
start/stop and avoid the need for an initialize function, right?


- Benjamin Mahler


On Nov. 14, 2016, 10:27 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52865/
> ---
> 
> (Updated Nov. 14, 2016, 10:27 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Benjamin Mahler, Gastón Kleiman, 
> and haosdent huang.
> 
> 
> Bugs: MESOS-5963
> https://issues.apache.org/jira/browse/MESOS-5963
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Prior to this patch, HealthChecker would stop performing health
> checks after it marks the task for kill. Since tasks' lifecycle
> is managed by scheduler-executor, HealthChecker should never stop
> health checking on its own.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp eefbc0c2936eb93044c2d8a1b50155171f398562 
>   src/health-check/health_checker.hpp 
> a1dc72493ff31b87068d5691f4d5b794392caf76 
>   src/health-check/health_checker.cpp 
> e2b32e2d57515202f547d12ba492ad8eb633641b 
>   src/launcher/default_executor.cpp f4e1ea4816fee9a2f6d59bb534933b66013f11cf 
>   src/launcher/executor.cpp ce0b199551447504bb95743df4ce9ec4a0443cd4 
> 
> Diff: https://reviews.apache.org/r/52865/diff/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/52873/.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 49571: Added a benchmark test for allocations.

[Anindya Sinha]

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

(Updated Nov. 18, 2016, 5:14 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

rebased.


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


Repository: mesos


Description
---

Allocations test has the following resource configurations:
(1) REGULAR: Offers from every slave have regular resources.
(2) SHARED: Offers from every slave include a shared resource.
(3) REGULAR: Offers from every alternate slave contain only regular
resources; and offers from every other alternate slave contains
a shared resource.

This test is parameterized based on number of agents, number of
frameworks and resource configuration.


Diffs (updated)
-

  src/tests/hierarchical_allocator_tests.cpp 
edd5cea8996d7c616cf9428f0f1c05d70c37c307 
  src/tests/resources_utils.hpp 18dcca7f171102df8fe88f10785f70c5d1cf5b32 
  src/tests/resources_utils.cpp be1feb97be552af582dbba3a54fa5fa0714b3eb2 

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


Testing
---

All tests passed.

Allocations benchmark test results
==
Support of shared resources has a small impact (roughly 10%) on runtime 
performance in allocations as compared to HEAD (without shared resources). 
Also, there is no visible impact in performance when shared resources are added 
in the tests.

Following is a snapshot with 1000 agents and 200 frameworks.

With the patch (and no shared resources)

[ RUN  ] AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/9
Using 1000 agents and 200 frameworks with resource type 0
Added 200 frameworks in 6907us
Added 1000 agents in 2.057098secs
round 0 allocate() took 1.689164secs to make 1000 offers
round 50 allocate() took 1.672373secs to make 1000 offers
round 100 allocate() took 1.680571secs to make 1000 offers
round 150 allocate() took 1.674683secs to make 1000 offers
round 199 allocate() took 1.671525secs to make 1000 offers

With the patch (and shared resources on all agents)
---
[ RUN  ] AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/10
Using 1000 agents and 200 frameworks with resource type 1
Added 200 frameworks in 6888us
Added 1000 agents in 2.096218secs
round 0 allocate() took 1.704491secs to make 1000 offers
round 50 allocate() took 1.718623secs to make 1000 offers
round 100 allocate() took 1.716224secs to make 1000 offers
round 150 allocate() took 1.707343secs to make 1000 offers
round 199 allocate() took 1.727467secs to make 1000 offers

With the patch (and shared resources on alternate agents)
-
[ RUN  ] AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/11
Using 1000 agents and 200 frameworks with resource type 2
Added 200 frameworks in 7304us
Added 1000 agents in 2.071009secs
round 0 allocate() took 1.689045secs to make 1000 offers
round 50 allocate() took 1.691524secs to make 1000 offers
round 100 allocate() took 1.688873secs to make 1000 offers
round 150 allocate() took 1.688713secs to make 1000 offers
round 199 allocate() took 1.691223secs to make 1000 offers

Based on HEAD, with all regular resources (no shared resources in HEAD 
supported)
-
[ RUN  ] AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/9
Using 1000 agents and 200 frameworks with resource type 0
Added 200 frameworks in 6801us
Added 1000 agents in 1.721447secs
round 0 allocate() took 1.502953secs to make 1000 offers
round 50 allocate() took 1.520157secs to make 1000 offers
round 100 allocate() took 1.517221secs to make 1000 offers
round 150 allocate() took 1.526446secs to make 1000 offers
round 199 allocate() took 1.538005secs to make 1000 offers


Thanks,

Anindya Sinha

Re: Review Request 45962: Updated a persistent volume test framework to include shared volumes.

[Anindya Sinha]

> On Nov. 17, 2016, 5:20 p.m., Jiang Yan Xu wrote:
> > src/examples/persistent_volume_framework.cpp, line 201
> > 
> >
> > Consolidate this with `case Shard::STAGING`? 
> > 
> > We can just do:
> > 
> > ```
> > if (shard.launched == 0) {
> >   // Create volume, add to `operations`.
> >   // Update info in Shard.
> >   // Add producer task to operations.
> > } else {
> >   // Modify volume to RO.
> >   // Add consumer task to operations.
> > }
> > ```

To do this, we need to init `Shard` with state of `Shard::STAGING`. I do not 
think we should init state to `Shard::STAGING` (syntactically incorrect). So, 
either we have a `Shard::INIT` (like we had before) or use the fact that 
`Shard.state` is `None()`.
So, if we do not want to add back `Shard::INIT`, we would need to handle the 
case of the 1st task outside the `switch`.


- Anindya


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


On Nov. 18, 2016, 5:13 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45962/
> ---
> 
> (Updated Nov. 18, 2016, 5:13 a.m.)
> 
> 
> Review request for mesos, Greg Mann, Jie Yu, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4431
> https://issues.apache.org/jira/browse/MESOS-4431
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated a persistent volume test framework to include shared volumes.
> 
> 
> Diffs
> -
> 
>   src/examples/persistent_volume_framework.cpp 
> 9d45bb496c4cf378af429b5aa970bf81450e482a 
> 
> Diff: https://reviews.apache.org/r/45962/diff/
> 
> 
> Testing
> ---
> 
> New test framework for shared resources added.
> Tests successful.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 45962: Updated a persistent volume test framework to include shared volumes.

[Anindya Sinha]

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

(Updated Nov. 18, 2016, 5:13 a.m.)


Review request for mesos, Greg Mann, Jie Yu, and Jiang Yan Xu.


Changes
---

addressed review comments.


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


Repository: mesos


Description
---

Updated a persistent volume test framework to include shared volumes.


Diffs (updated)
-

  src/examples/persistent_volume_framework.cpp 
9d45bb496c4cf378af429b5aa970bf81450e482a 

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


Testing
---

New test framework for shared resources added.
Tests successful.


Thanks,

Anindya Sinha

Re: Review Request 53096: Fix handling in shared count in total resources in the sorter.

[Anindya Sinha]

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

(Updated Nov. 18, 2016, 5:14 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

rebased.


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


Repository: mesos


Description
---

We maintain a single copy of shared resource in the role and quota
sorter's total resources. So, when we update these resources, we ensure
that we only count a single copy even though the framework sorter
may be returned multiple copies of a shared resource.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.cpp 
c8f9492ee1b69e125a1e841116d22a578a9b524e 
  src/tests/persistent_volume_tests.cpp 
8651b2c5455089041f16d091c90a7e0d948191b8 

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


Testing
---

Tests passed.


Thanks,

Anindya Sinha

Re: Review Request 53837: Added a per container mesos-io-switchboard process.

[Kevin Klues]

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

(Updated Nov. 18, 2016, 5:07 a.m.)


Review request for mesos, Benjamin Hindman, Jie Yu, and Vinod Kone.


Changes
---

Updated to include full functionality for dynamically adding multiple `fd` 
targets and writing to them asynchronously (though no one actually calls these 
functions yet). Also added code to dynamically change the read end of the stdin 
stream so we can eventually allow this to be swapped out based on `attachInput` 
calls.


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


Repository: mesos


Description
---

Currently, this process simply intercepts the stdout/stderr of a
container and writes it to the stdout/stderr FDs set up by the
container logger. In future commits, this process will be expanded to
allow dynamically attaching new input / output sources to a container
to support `docker attach` like functionality for a running container.


Diffs (updated)
-

  src/Makefile.am 5a47c93388234a68c3c486a021ccdbe3213c5bac 
  src/slave/containerizer/mesos/containerizer.cpp 
ec4ae32485a7ab6c9f73c512004d1220482a188e 
  src/slave/containerizer/mesos/io_switchboard/io_switchboard.hpp PRE-CREATION 
  src/slave/containerizer/mesos/io_switchboard/io_switchboard.cpp PRE-CREATION 
  src/slave/containerizer/mesos/io_switchboard/main.hpp PRE-CREATION 
  src/slave/containerizer/mesos/io_switchboard/main.cpp PRE-CREATION 

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


Testing
---

GTEST_FILTER="" make -j check
sudo src/mesos-tests


Thanks,

Kevin Klues

Re: Review Request 53096: Fix handling in shared count in total resources in the sorter.

[Anindya Sinha]

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

(Updated Nov. 18, 2016, 4:32 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

rebased.


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


Repository: mesos


Description
---

We maintain a single copy of shared resource in the role and quota
sorter's total resources. So, when we update these resources, we ensure
that we only count a single copy even though the framework sorter
may be returned multiple copies of a shared resource.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.cpp 
c8f9492ee1b69e125a1e841116d22a578a9b524e 
  src/tests/persistent_volume_tests.cpp 
8651b2c5455089041f16d091c90a7e0d948191b8 

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


Testing
---

Tests passed.


Thanks,

Anindya Sinha

Re: Review Request 53699: Add test cases to test logrotate with switch_user set to true and false.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [52308, 52310, 53473, 53699]

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 Nov. 17, 2016, 8:42 p.m., Sivaram Kannan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53699/
> ---
> 
> (Updated Nov. 17, 2016, 8:42 p.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Bugs: MESOS-5856
> https://issues.apache.org/jira/browse/MESOS-5856
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add test cases to test logrotate with switch_user set to true and false.
> 
> 
> Diffs
> -
> 
>   src/tests/container_logger_tests.cpp 
> 1bb94a8461e481983f25a44737e4011ed5fc4b1f 
> 
> Diff: https://reviews.apache.org/r/53699/diff/
> 
> 
> Testing
> ---
> 
> Run the mesos-logrotate-logger with un-priviledged user and verify whether 
> the logs are getting rotated.
> Run the mesos-logrotate-logger as root user and verify whether the logs are 
> getting rotated.
> 
> 
> Thanks,
> 
> Sivaram Kannan
> 
>

Re: Review Request 53791: Use the stout ELF parser to collect Linux rootfs files.

[Jiang Yan Xu]

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




src/tests/containerizer/rootfs.cpp (line 58)


This feels like an implementation of `ldd()` that coule be generally useful.

Would it make sense to rename ldcache.hpp|cpp as ld.hpp|cpp and add `ldd()` 
as a method in ld.hpp|cpp make sense?



src/tests/containerizer/rootfs.cpp (lines 58 - 61)


This feels like an intermediate method for recursion. Would be nice to put 
a front-end method above it:

```
Try ldd(const string& path);
```



src/tests/containerizer/rootfs.cpp (line 61)


Using `hashset` would make it more explicit that no order is implied 
(otherwise this has to be a vector).

And we can use `contains()`!



src/tests/containerizer/rootfs.cpp (lines 69 - 71)


It feels like it's not worth extracting out the logic

(similar code from volume.cpp)
```
Try load = elf::File::load(realpath.get());
if (load.isError()) {
  return Error("Failed to elf::File::load '" + realpath.get() + "':"
   " " + load.error());
}

Owned file(load.get());
```

into a 10-line helper method and spend 4 lines here?



src/tests/containerizer/rootfs.cpp (line 73)


Turn it into a hashset for easier checking?



src/tests/containerizer/rootfs.cpp (lines 81 - 84)


If we have to do this, format it this way

```
auto entry = std::find_if(
cache.begin(),
cache.end(),
[dependency](const ldcache::Entry& e) {
  return strings::startsWith(e.name, dependency);
});
```

But is prefix matching necessary? Can't we do extract matching 
(i.e.,`hashset.contains()`)?

Note that our seed list are just programs without versions.



src/tests/containerizer/rootfs.cpp (line 82)


No space.



src/tests/containerizer/rootfs.cpp (line 107)


Not convinced that we need to extract it out since it's used only in a 
single place?



src/tests/containerizer/rootfs.cpp (line 196)


The following is more widely used.

```
#ifdef __linux__
```



src/tests/containerizer/rootfs.cpp (lines 196 - 201)


The whole file should just be linux only so we don't have to do it here.



src/tests/containerizer/rootfs.cpp (line 223)


Would the following look better?

```
hashset needed(programs.begin(), programs.end());

foreach (const string& program, programs) {
  Try dependencies = ldd(file);

  // error handling.

  needed = needed | dependencies;
}
```


- Jiang Yan Xu


On Nov. 15, 2016, 3:34 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53791/
> ---
> 
> (Updated Nov. 15, 2016, 3:34 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, Kevin Klues, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6588
> https://issues.apache.org/jira/browse/MESOS-6588
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The dependencies for the programs we need in the Linux root
> filesystem vary over time and across distributions. Since Stout
> has support for parsing the library dependencies of ELF binaries,
> use this to collect the necessary dependencies when constructing
> a root filesystem for the tests.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/rootfs.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/53791/diff/
> 
> 
> Testing
> ---
> 
> sudo make check (Fedora 24)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 53853: Expanded the comment around `ContainerInfo` protobuf.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [53853]

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 Nov. 17, 2016, 4:58 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53853/
> ---
> 
> (Updated Nov. 17, 2016, 4:58 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Gilbert Song, Jie Yu, and Jan 
> Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 380575904a5a29ee53bdc87ae8791ed14e3cafca 
>   include/mesos/v1/mesos.proto 5b542ff9cd0b5f301c088d3f4b2fe17ae760d368 
> 
> Diff: https://reviews.apache.org/r/53853/diff/
> 
> 
> Testing
> ---
> 
> None: not a functional change.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 53474: Support explicit error codes in ErrnoError and SocketError.

[Michael Park]

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


Ship it!





3rdparty/stout/include/stout/windows/error.hpp (lines 110 - 114)


Will commit with these flipped.


- Michael Park


On Nov. 17, 2016, 1:26 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53474/
> ---
> 
> (Updated Nov. 17, 2016, 1:26 p.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Michael Park.
> 
> 
> Bugs: MESOS-6520
> https://issues.apache.org/jira/browse/MESOS-6520
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Pass the error code down to the Error object explicitly, rather than
> setting the per-thread error and using it implicitly.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/errorbase.hpp 
> 96d395d37cdad706c2f23fdd9caeefc2a33e0541 
>   3rdparty/stout/include/stout/os/posix/rmdir.hpp 
> 5657b5a18aaedf842b7b780ea7eada73118788cb 
>   3rdparty/stout/include/stout/os/windows/su.hpp 
> 777140e1139d6eeab20780e8c0d0a273ce6a8125 
>   3rdparty/stout/include/stout/windows/error.hpp 
> f296b82c47ef5ae36f6e5ee4dd289cb15bd200a6 
>   3rdparty/stout/include/stout/windows/fs.hpp 
> 06c46db54e8deebe5e6715d281db699f8c4c5a58 
>   3rdparty/stout/include/stout/windows/os.hpp 
> 7ca0b5dc9793369ea142684e3614e8f33cac64b6 
>   3rdparty/stout/tests/error_tests.cpp 
> 25b50141dd9bbf163aa816750210b298456740a8 
> 
> Diff: https://reviews.apache.org/r/53474/diff/
> 
> 
> Testing
> ---
> 
> make check on Fedora 24
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 52652: Changed Bytes operators to take unsigned integers.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Oct. 7, 2016, 3:19 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52652/
> ---
> 
> (Updated Oct. 7, 2016, 3:19 p.m.)
> 
> 
> Review request for mesos, Daniel Pravat, Artem Harutyunyan, and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The multiplication and division operators for `Bytes` take a `double`
> argument.  Changing this to an unsigned integer removes the implicit
> `double` to `uint64_t` casting.  These operators are currently only
> used in tests.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/bytes.hpp 
> 9debe2f5579b9b2d67933ecc2bfcc40c2730f7f1 
>   3rdparty/stout/tests/bytes_tests.cpp 
> 9d32ea141cdb0ec3f03a8dad6b567afdd1e026c4 
> 
> Diff: https://reviews.apache.org/r/52652/diff/
> 
> 
> Testing
> ---
> 
> See next review.
> 
> Supercedes this review: https://reviews.apache.org/r/52192/
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 52653: Modified a test to use the updated Bytes operators.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Oct. 7, 2016, 3:19 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52653/
> ---
> 
> (Updated Oct. 7, 2016, 3:19 p.m.)
> 
> 
> Review request for mesos, Daniel Pravat, Artem Harutyunyan, and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is the only test that multiplies/divides Bytes by a decimal value.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 7957873102b3777d47fa9304b85a93d58a746b9d 
> 
> Diff: https://reviews.apache.org/r/52653/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 52696: Harden stout

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Nov. 9, 2016, 11:05 a.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52696/
> ---
> 
> (Updated Nov. 9, 2016, 11:05 a.m.)
> 
> 
> Review request for mesos, James Peach, Michael Park, and Neil Conway.
> 
> 
> Bugs: MESOS-6229
> https://issues.apache.org/jira/browse/MESOS-6229
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add hardened flags for stout.
> Take compile flag macro at 1a869696e4129279f7b99c3f9052717354b79a86.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/Makefile.am 4e10ae2 
>   3rdparty/stout/configure.ac f071f61 
>   3rdparty/stout/m4/ax_check_compile_flag.m4 PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/52696/diff/
> 
> 
> Testing
> ---
> 
> Compared the benchmarks with and without the flags being used. Also did a 
> comparsion with the flags being used with and without optimizations and 
> without the flags being used with and without optimizations. Overall the 
> performance hit was very small with a 3-8% overhead (optimizations brings 
> this down slightly). Most benchmarks were about 5% (or less) slower.
> 
> 
> File Attachments
> 
> 
> --enable-optimized with hardening applied
>   
> https://reviews.apache.org/media/uploaded/files/2016/11/02/18a2f590-75ad-49c5-a697-56b746f28cae__hardened-optimized.txt
> Hardening applied but no --enable-optimized
>   
> https://reviews.apache.org/media/uploaded/files/2016/11/02/a6e07766-80cc-4bd7-856d-8952cac12562__hardened-unoptimized.txt
> --enable-optimized with no hardening applied
>   
> https://reviews.apache.org/media/uploaded/files/2016/11/02/046b37a9-5aff-4543-b3bb-5ac60daaf498__optimized.txt
> No hardening applied and no --enable-optimized
>   
> https://reviews.apache.org/media/uploaded/files/2016/11/02/3baa96cf-be05-4ac0-ad4c-ef571386e8f4__unoptimized.txt
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 53850: Implemented fetching Docker images that have V2 schema 2 manifests.

[Mesos ReviewBot]

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



Bad review!

Reviews applied: []

Error:
No reviewers specified. Please find a reviewer by asking on JIRA or the mailing 
list.

- Mesos ReviewBot


On Nov. 17, 2016, 2:58 p.m., Ilya Pronin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53850/
> ---
> 
> (Updated Nov. 17, 2016, 2:58 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-3505
> https://issues.apache.org/jira/browse/MESOS-3505
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented fetching Docker images that have V2 schema 2 manifests.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp 
> b06ddff68a8d2df13abb838b03a8e73d4e273c31 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> 96821692a8b2080a25997afa66a0b5e6699c95c4 
>   src/tests/uri_fetcher_tests.cpp 3c1bd33137612de90d65b7af288bb81ac9876c8b 
>   src/uri/fetchers/docker.cpp 3f38dddfb4c089322fe4e13b1ef2070b4835885c 
> 
> Diff: https://reviews.apache.org/r/53850/diff/
> 
> 
> Testing
> ---
> 
> Added test to verify that V2 schema 2 image manifest can be pulled with all 
> image blobs. Modified previously added "pull image by digest" test to pull an 
> image that has schema 2 manifest. Ran `make check`.
> 
> 
> Thanks,
> 
> Ilya Pronin
> 
>

Re: Review Request 52695: Harden libprocess

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Nov. 9, 2016, 11:07 a.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52695/
> ---
> 
> (Updated Nov. 9, 2016, 11:07 a.m.)
> 
> 
> Review request for mesos, James Peach, Michael Park, and Neil Conway.
> 
> 
> Bugs: MESOS-6229
> https://issues.apache.org/jira/browse/MESOS-6229
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add hardened flags for libprocess.
> Take compile flag macro at 1a869696e4129279f7b99c3f9052717354b79a86.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am 7131989 
>   3rdparty/libprocess/configure.ac e65e5ca 
>   3rdparty/libprocess/m4/ax_check_compile_flag.m4 PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/52695/diff/
> 
> 
> Testing
> ---
> 
> Compared the benchmarks with and without the flags being used. Also did a 
> comparsion with the flags being used with and without optimizations and 
> without the flags being used with and without optimizations. Overall the 
> performance hit was very small with a 3-8% overhead (optimizations brings 
> this down slightly). Most benchmarks were about 5% (or less) slower.
> 
> 
> File Attachments
> 
> 
> --enable-optimized with hardening applied
>   
> https://reviews.apache.org/media/uploaded/files/2016/11/02/875c9e6e-c73b-4e3c-8265-0f7c6dc00351__hardened-optimized.txt
> Hardening applied but no --enable-optimized
>   
> https://reviews.apache.org/media/uploaded/files/2016/11/02/932d28a7-2d31-471a-b438-647841a6853c__hardened-unoptimized.txt
> --enable-optimized with no hardening applied
>   
> https://reviews.apache.org/media/uploaded/files/2016/11/02/896944ea-9b31-4d62-b1b9-97fb4700a882__optimized.txt
> No hardening applied and no --enable-optimized
>   
> https://reviews.apache.org/media/uploaded/files/2016/11/02/b32667ce-3e3b-4d2b-b4f8-4c2404a0fc1c__unoptimized.txt
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 52754: Remove unused code which now throws errors with the new hardening flags

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Nov. 8, 2016, 9:40 a.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52754/
> ---
> 
> (Updated Nov. 8, 2016, 9:40 a.m.)
> 
> 
> Review request for mesos, James Peach, Michael Park, and Neil Conway.
> 
> 
> Bugs: MESOS-6239
> https://issues.apache.org/jira/browse/MESOS-6239
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The hardening flags produced many new errors about unused functions, 
> variables, etc. that need to be fixed for Mesos to compile/run.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/profiler.hpp f6fccfb 
>   3rdparty/libprocess/src/profiler.cpp 0c4949e 
>   3rdparty/libprocess/src/tests/benchmarks.cpp 945007c 
>   3rdparty/libprocess/src/tests/process_tests.cpp a4af54a 
>   3rdparty/libprocess/src/tests/subprocess_tests.cpp 0dc1c62 
> 
> Diff: https://reviews.apache.org/r/52754/diff/
> 
> 
> Testing
> ---
> 
> Made sure compilation, tests, and benchmarks worked with both gcc and clang. 
> Ran make && make check && make bench.
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 52645: Harden Mesos

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Nov. 9, 2016, 11:37 a.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52645/
> ---
> 
> (Updated Nov. 9, 2016, 11:37 a.m.)
> 
> 
> Review request for mesos, James Peach, Michael Park, and Neil Conway.
> 
> 
> Bugs: MESOS-6229
> https://issues.apache.org/jira/browse/MESOS-6229
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add hardened flags for Mesos.
> Take compile flag macro at 1a869696e4129279f7b99c3f9052717354b79a86.
> 
> 
> Diffs
> -
> 
>   configure.ac 5380cbc 
>   m4/ax_check_compile_flag.m4 PRE-CREATION 
>   src/Makefile.am 5a47c93 
> 
> Diff: https://reviews.apache.org/r/52645/diff/
> 
> 
> Testing
> ---
> 
> Compared the benchmarks with and without the flags being used. Also did a 
> comparsion with the flags being used with and without optimizations and 
> without the flags being used with and without optimizations. Overall the 
> performance hit was very small with a 3-8% overhead (optimizations brings 
> this down slightly). Most benchmarks were about 5% (or less) slower.
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 53790: Move containerizer Rootfs support to a cpp file.

[Jiang Yan Xu]

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


Fix it, then Ship it!




This patch is code movement only right? Would be more clear to state "no 
logical change" in the summary.


src/Makefile.am 


Keep the header file here?



src/tests/containerizer/rootfs.cpp (line 34)


One line here.


- Jiang Yan Xu


On Nov. 15, 2016, 3:35 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53790/
> ---
> 
> (Updated Nov. 15, 2016, 3:35 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, Kevin Klues, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6588
> https://issues.apache.org/jira/browse/MESOS-6588
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Move containerizer Rootfs support to a cpp file.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 5a47c93388234a68c3c486a021ccdbe3213c5bac 
>   src/tests/containerizer/rootfs.hpp 6bc3835cbb62536ec933ef38c9e15138b8611e5f 
>   src/tests/containerizer/rootfs.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/53790/diff/
> 
> 
> Testing
> ---
> 
> sudo make check (Fedora 24)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 53474: Support explicit error codes in ErrnoError and SocketError.

[James Peach]

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

(Updated Nov. 17, 2016, 9:26 p.m.)


Review request for mesos, Alex Clemmer and Michael Park.


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


Repository: mesos


Description
---

Pass the error code down to the Error object explicitly, rather than
setting the per-thread error and using it implicitly.


Diffs (updated)
-

  3rdparty/stout/include/stout/errorbase.hpp 
96d395d37cdad706c2f23fdd9caeefc2a33e0541 
  3rdparty/stout/include/stout/os/posix/rmdir.hpp 
5657b5a18aaedf842b7b780ea7eada73118788cb 
  3rdparty/stout/include/stout/os/windows/su.hpp 
777140e1139d6eeab20780e8c0d0a273ce6a8125 
  3rdparty/stout/include/stout/windows/error.hpp 
f296b82c47ef5ae36f6e5ee4dd289cb15bd200a6 
  3rdparty/stout/include/stout/windows/fs.hpp 
06c46db54e8deebe5e6715d281db699f8c4c5a58 
  3rdparty/stout/include/stout/windows/os.hpp 
7ca0b5dc9793369ea142684e3614e8f33cac64b6 
  3rdparty/stout/tests/error_tests.cpp 25b50141dd9bbf163aa816750210b298456740a8 

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


Testing
---

make check on Fedora 24


Thanks,

James Peach

Re: Review Request 53475: Use explicit error codes in ErrnoError and SocketError.

[James Peach]

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

(Updated Nov. 17, 2016, 9:25 p.m.)


Review request for mesos and Michael Park.


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


Repository: mesos


Description
---

Pass the error code down to the Error object explicitly, rather than
setting the per-thread error and using it implicitly.


Diffs (updated)
-

  3rdparty/libprocess/src/libevent_ssl_socket.cpp 
21f878ee81db32ad35878ec053c3f2de3637196c 
  3rdparty/libprocess/src/poll_socket.cpp 
f0ee1490e6fccb038f64a27b2c71458ad5b5e5a1 

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


Testing
---

make check on Fedora 24


Thanks,

James Peach

Re: Review Request 52886: Fix new sign comparison errors in stout produced by hardened flags

[Michael Park]

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


Fix it, then Ship it!





3rdparty/stout/tests/json_tests.cpp (line 196)


This should stay as `7` I think?


- Michael Park


On Nov. 8, 2016, 9:39 a.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52886/
> ---
> 
> (Updated Nov. 8, 2016, 9:39 a.m.)
> 
> 
> Review request for mesos, James Peach, Michael Park, and Neil Conway.
> 
> 
> Bugs: MESOS-6239
> https://issues.apache.org/jira/browse/MESOS-6239
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The hardening flags produced many new sign comparison errors in stout that 
> need to be fixed for Mesos to compile/run.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/tests/cache_tests.cpp 0950c85 
>   3rdparty/stout/tests/flags_tests.cpp da4deb9 
>   3rdparty/stout/tests/hashmap_tests.cpp 2626d67 
>   3rdparty/stout/tests/hashset_tests.cpp 66e59db 
>   3rdparty/stout/tests/ip_tests.cpp b5a206f 
>   3rdparty/stout/tests/json_tests.cpp 2bc4c88 
>   3rdparty/stout/tests/linkedhashmap_tests.cpp 7a80769 
>   3rdparty/stout/tests/multimap_tests.cpp 488991b 
>   3rdparty/stout/tests/os/process_tests.cpp 4cb3b5f 
>   3rdparty/stout/tests/os/sendfile_tests.cpp e221689 
>   3rdparty/stout/tests/os/systems_tests.cpp 110ba5b 
>   3rdparty/stout/tests/os_tests.cpp 0b7ee07 
>   3rdparty/stout/tests/strings_tests.cpp 7dd3301 
> 
> Diff: https://reviews.apache.org/r/52886/diff/
> 
> 
> Testing
> ---
> 
> Made sure compilation, tests, and benchmarks worked with both gcc and clang. 
> Ran make && make check && make bench.
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 53842: Add role specific metrics for sorting runs.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [53839, 53840, 53841, 53842]

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 Nov. 17, 2016, 6:53 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53842/
> ---
> 
> (Updated Nov. 17, 2016, 6:53 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6579
> https://issues.apache.org/jira/browse/MESOS-6579
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added the following 2 metrics which is maintained on a role level
> (as long as there is at least one framework of that role):
> a) allocator/mesos/frameworks/role//sort_runs: Number of framework
>level sorts (based on role) in DRF Sorter.
> b) allocator/mesos/frameworks/role//sort_run: Latency in framework
>level sorts (based on role) in DRF Sorter.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> c8f9492ee1b69e125a1e841116d22a578a9b524e 
>   src/master/allocator/mesos/metrics.hpp 
> 753f90acc52ada84883cbbe3350e61d1e1eaff48 
>   src/master/allocator/mesos/metrics.cpp 
> a36d21c297160bc1c9f43a22743fd5448d7ae5ac 
>   src/tests/hierarchical_allocator_tests.cpp 
> edd5cea8996d7c616cf9428f0f1c05d70c37c307 
> 
> Diff: https://reviews.apache.org/r/53842/diff/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 53699: Add test cases to test logrotate with switch_user set to true and false.

[Sivaram Kannan]

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

(Updated Nov. 17, 2016, 8:42 p.m.)


Review request for mesos and Joseph Wu.


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


Repository: mesos


Description
---

Add test cases to test logrotate with switch_user set to true and false.


Diffs (updated)
-

  src/tests/container_logger_tests.cpp 1bb94a8461e481983f25a44737e4011ed5fc4b1f 

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


Testing
---

Run the mesos-logrotate-logger with un-priviledged user and verify whether the 
logs are getting rotated.
Run the mesos-logrotate-logger as root user and verify whether the logs are 
getting rotated.


Thanks,

Sivaram Kannan

Re: Review Request 53475: Use explicit error codes in ErrnoError and SocketError.

[Michael Park]

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




3rdparty/libprocess/src/libevent_ssl_socket.cpp (line 166)


`s/ENOTCON/ENOTCONN/`


- Michael Park


On Nov. 17, 2016, 11:08 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53475/
> ---
> 
> (Updated Nov. 17, 2016, 11:08 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-6520
> https://issues.apache.org/jira/browse/MESOS-6520
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Pass the error code down to the Error object explicitly, rather than
> setting the per-thread error and using it implicitly.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/libevent_ssl_socket.cpp 
> 21f878ee81db32ad35878ec053c3f2de3637196c 
>   3rdparty/libprocess/src/poll_socket.cpp 
> f0ee1490e6fccb038f64a27b2c71458ad5b5e5a1 
> 
> Diff: https://reviews.apache.org/r/53475/diff/
> 
> 
> Testing
> ---
> 
> make check on Fedora 24
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 53474: Support explicit error codes in ErrnoError and SocketError.

[James Peach]

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

(Updated Nov. 17, 2016, 7:07 p.m.)


Review request for mesos, Alex Clemmer and Michael Park.


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


Repository: mesos


Description (updated)
---

Pass the error code down to the Error object explicitly, rather than
setting the per-thread error and using it implicitly.


Diffs (updated)
-

  3rdparty/stout/include/stout/errorbase.hpp 
96d395d37cdad706c2f23fdd9caeefc2a33e0541 
  3rdparty/stout/include/stout/os/posix/rmdir.hpp 
5657b5a18aaedf842b7b780ea7eada73118788cb 
  3rdparty/stout/include/stout/os/windows/su.hpp 
777140e1139d6eeab20780e8c0d0a273ce6a8125 
  3rdparty/stout/include/stout/windows/error.hpp 
f296b82c47ef5ae36f6e5ee4dd289cb15bd200a6 
  3rdparty/stout/include/stout/windows/fs.hpp 
06c46db54e8deebe5e6715d281db699f8c4c5a58 
  3rdparty/stout/include/stout/windows/os.hpp 
7ca0b5dc9793369ea142684e3614e8f33cac64b6 
  3rdparty/stout/tests/error_tests.cpp 25b50141dd9bbf163aa816750210b298456740a8 

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


Testing
---

make check on Fedora 24


Thanks,

James Peach

Re: Review Request 53475: Use explicit error codes in ErrnoError and SocketError.

[James Peach]

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

(Updated Nov. 17, 2016, 7:08 p.m.)


Review request for mesos and Michael Park.


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


Repository: mesos


Description
---

Pass the error code down to the Error object explicitly, rather than
setting the per-thread error and using it implicitly.


Diffs (updated)
-

  3rdparty/libprocess/src/libevent_ssl_socket.cpp 
21f878ee81db32ad35878ec053c3f2de3637196c 
  3rdparty/libprocess/src/poll_socket.cpp 
f0ee1490e6fccb038f64a27b2c71458ad5b5e5a1 

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


Testing
---

make check on Fedora 24


Thanks,

James Peach

Re: Review Request 53474: Support explicit error codes in ErrnoError and SocketError.

[James Peach]

> On Nov. 16, 2016, 10:25 p.m., Michael Park wrote:
> > 3rdparty/stout/include/stout/os/posix/rmdir.hpp, lines 57-58
> > 
> >
> > This is great!
> > 
> > It seems like we have more opportunities to update here:
> > 
> > ```
> > 3rdparty/libprocess/src/libevent_ssl_socket.cpp
> > 166:  errno = ENOTCONN;
> > 167-  return ErrnoError();
> > ```
> > ```
> > 3rdparty/libprocess/src/poll_socket.cpp
> > 138-#ifdef __WINDOWS__
> > 139-::WSASetLastError(opt);
> > 140-#else
> > 141:errno = opt;
> > 142-#endif
> > 143-
> > 144-return Failure(SocketError("Failed to connect to " + 
> > stringify(to)));
> > ```
> > ```
> > 3rdparty/stout/include/stout/windows/os.hpp
> > 254:errno = ENOSYS;
> > 255-return ErrnoError(
> > 261:errno = ENOSYS;
> > 262-return ErrnoError(
> > 294:errno = ECHILD;
> > 295-return WindowsError(
> > 305:errno = ECHILD;
> > 306-return WindowsError(
> > 324:errno = ECHILD;
> > 325-return WindowsError(
> > ```
> > ```
> > 3rdparty/stout/include/stout/os/windows/su.hpp
> > 55:  SetLastError(ERROR_NOT_SUPPORTED);
> > 56-  return WindowsError();
> > ```
> > ```
> > 3rdparty/stout/include/stout/windows/fs.hpp
> > 122:::SetLastError(error);
> > 123-return WindowsError(
> > 124-"'fs::list': 'FindNextFile' failed when searching for files 
> > with "
> > 125-"'pattern '" + pattern + "'");
> > ```
> 
> Michael Park wrote:
> Ah, I see that the first two `libevent_ssl_socket.cpp` and 
> `poll_socket.cpp` are covered by https://reviews.apache.org/r/53475/.

The Windows implementation of `os::waitpid` deliberately sets `errno` in an 
attempt to be compatible with the the POSIX version. Callers assume that it 
will set `errno`. I think it is best to leave this alone for now.


- James


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


On Nov. 4, 2016, 11:03 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53474/
> ---
> 
> (Updated Nov. 4, 2016, 11:03 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-6520
> https://issues.apache.org/jira/browse/MESOS-6520
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Support explicit error codes in ErrnoError and SocketError.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/errorbase.hpp 
> 96d395d37cdad706c2f23fdd9caeefc2a33e0541 
>   3rdparty/stout/include/stout/os/posix/rmdir.hpp 
> 5657b5a18aaedf842b7b780ea7eada73118788cb 
>   3rdparty/stout/include/stout/windows/error.hpp 
> f296b82c47ef5ae36f6e5ee4dd289cb15bd200a6 
>   3rdparty/stout/tests/error_tests.cpp 
> 25b50141dd9bbf163aa816750210b298456740a8 
> 
> Diff: https://reviews.apache.org/r/53474/diff/
> 
> 
> Testing
> ---
> 
> make check on Fedora 24
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 53837: Added a per container mesos-io-switchboard process.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [53704, 53837]

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 Nov. 17, 2016, 5:05 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53837/
> ---
> 
> (Updated Nov. 17, 2016, 5:05 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Jie Yu, and Vinod Kone.
> 
> 
> Bugs: MESOS-6467
> https://issues.apache.org/jira/browse/MESOS-6467
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, this process simply intercepts the stdout/stderr of a
> container and writes it to the stdout/stderr FDs set up by the
> container logger. In future commits, this process will be expanded to
> allow dynamically attaching new input / output sources to a container
> to support `docker attach` like functionality for a running container.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 5a47c93388234a68c3c486a021ccdbe3213c5bac 
>   src/slave/containerizer/mesos/containerizer.cpp 
> ec4ae32485a7ab6c9f73c512004d1220482a188e 
>   src/slave/containerizer/mesos/io_switchboard/io_switchboard.hpp 
> PRE-CREATION 
>   src/slave/containerizer/mesos/io_switchboard/io_switchboard.cpp 
> PRE-CREATION 
>   src/slave/containerizer/mesos/io_switchboard/main.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/io_switchboard/main.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/53837/diff/
> 
> 
> Testing
> ---
> 
> GTEST_FILTER="" make -j check
> sudo src/mesos-tests
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 45962: Updated a persistent volume test framework to include shared volumes.

[Jiang Yan Xu]

> On Nov. 6, 2016, 9:25 p.m., Jiang Yan Xu wrote:
> > src/examples/persistent_volume_framework.cpp, lines 470-476
> > 
> >
> > I think it's sufficient to have the following states. (We should use a 
> > minimun number of state, we can always add more when more features are 
> > added to persistent volumes which demand more state but starting with a 
> > large number of states makes it difficult to evolve the test).
> > 
> > ```
> >   STAGING = 0, // The shard is awaiting offers to launch more tasks.
> >   RUNNING, // The shard is fully running (all its tasks are 
> > launched).
> >   TERMINATING, // The shard is terminating and needs to clean up 
> > its persistent volume.
> >   DONE // The shard is terminated.
> > ```
> > 
> > This translates to:
> > 
> > ```
> >   STAGING = 0, // In resourceOffers: launch tasks
> >   RUNNING, // In resourceOffers: noop; In statusUpdate: check 
> > shard TERMINATING condition.
> >   TERMINATING, // In resourceOffers: DESTROY
> >   DONE // Test terminal condition.
> > ```
> 
> Anindya Sinha wrote:
> To accomplish this state, I changed shard's state to `Option 
> state` so we start the state machine if `shared.state == None()`. Also, if 
> for some reason `DESTROY` fails continuously, the test would not exit. We can 
> fail the test if it fails on n attempts of `DESTROY` by keeping track of 
> number of `DESTROY` attempts but maybe not required. Comments?
> 
> Jiang Yan Xu wrote:
> Keeping track of the number of destroys may be too much IMO :) We don't 
> terminate when the agent fails to receive the tasks either (we don't 
> reconcile) but I think these scenarios are too rear for a simple example 
> framework.

s/rear/rare/ :)


- Jiang Yan


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


On Nov. 7, 2016, 9:50 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45962/
> ---
> 
> (Updated Nov. 7, 2016, 9:50 p.m.)
> 
> 
> Review request for mesos, Greg Mann, Jie Yu, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4431
> https://issues.apache.org/jira/browse/MESOS-4431
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated a persistent volume test framework to include shared volumes.
> 
> 
> Diffs
> -
> 
>   src/examples/persistent_volume_framework.cpp 
> 9d45bb496c4cf378af429b5aa970bf81450e482a 
> 
> Diff: https://reviews.apache.org/r/45962/diff/
> 
> 
> Testing
> ---
> 
> New test framework for shared resources added.
> Tests successful.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 45962: Updated a persistent volume test framework to include shared volumes.

[Jiang Yan Xu]

> On Nov. 6, 2016, 9:25 p.m., Jiang Yan Xu wrote:
> > src/examples/persistent_volume_framework.cpp, lines 470-476
> > 
> >
> > I think it's sufficient to have the following states. (We should use a 
> > minimun number of state, we can always add more when more features are 
> > added to persistent volumes which demand more state but starting with a 
> > large number of states makes it difficult to evolve the test).
> > 
> > ```
> >   STAGING = 0, // The shard is awaiting offers to launch more tasks.
> >   RUNNING, // The shard is fully running (all its tasks are 
> > launched).
> >   TERMINATING, // The shard is terminating and needs to clean up 
> > its persistent volume.
> >   DONE // The shard is terminated.
> > ```
> > 
> > This translates to:
> > 
> > ```
> >   STAGING = 0, // In resourceOffers: launch tasks
> >   RUNNING, // In resourceOffers: noop; In statusUpdate: check 
> > shard TERMINATING condition.
> >   TERMINATING, // In resourceOffers: DESTROY
> >   DONE // Test terminal condition.
> > ```
> 
> Anindya Sinha wrote:
> To accomplish this state, I changed shard's state to `Option 
> state` so we start the state machine if `shared.state == None()`. Also, if 
> for some reason `DESTROY` fails continuously, the test would not exit. We can 
> fail the test if it fails on n attempts of `DESTROY` by keeping track of 
> number of `DESTROY` attempts but maybe not required. Comments?

Keeping track of the number of destroys may be too much IMO :) We don't 
terminate when the agent fails to receive the tasks either (we don't reconcile) 
but I think these scenarios are too rear for a simple example framework.


- Jiang Yan


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


On Nov. 7, 2016, 9:50 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45962/
> ---
> 
> (Updated Nov. 7, 2016, 9:50 p.m.)
> 
> 
> Review request for mesos, Greg Mann, Jie Yu, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4431
> https://issues.apache.org/jira/browse/MESOS-4431
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated a persistent volume test framework to include shared volumes.
> 
> 
> Diffs
> -
> 
>   src/examples/persistent_volume_framework.cpp 
> 9d45bb496c4cf378af429b5aa970bf81450e482a 
> 
> Diff: https://reviews.apache.org/r/45962/diff/
> 
> 
> Testing
> ---
> 
> New test framework for shared resources added.
> Tests successful.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 45962: Updated a persistent volume test framework to include shared volumes.

[Jiang Yan Xu]

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




src/examples/persistent_volume_framework.cpp (lines 33 - 37)


Are these used?



src/examples/persistent_volume_framework.cpp (line 128)


`immediately` is not accurate? (it still needs to wait until the next offer 
cycle). 

I guess it sufficies to just say that "in the case of shared persistent 
volumes, tasks can access the shared volume simutaneously".



src/examples/persistent_volume_framework.cpp (line 201)


Consolidate this with `case Shard::STAGING`? 

We can just do:

```
if (shard.launched == 0) {
  // Create volume, add to `operations`.
  // Update info in Shard.
  // Add producer task to operations.
} else {
  // Modify volume to RO.
  // Add consumer task to operations.
}
```



src/examples/persistent_volume_framework.cpp (lines 205 - 208)


Remove this?



src/examples/persistent_volume_framework.cpp (lines 235 - 236)


Kill this since it's agnostic about whether the volume is shared?



src/examples/persistent_volume_framework.cpp (lines 259 - 267)


Is this necessary? We use unique persistence IDs.



src/examples/persistent_volume_framework.cpp (lines 269 - 280)


The need to look up the volume Resource here and below in TERMINATING 
justifies saving it in `Volume`?

i.e.,

```
struct Volume
{
  Option resource;
  bool isShared;
}
```

(Doesn't look like we even need to record the persistent ID or SlaveID 
separately?)

Then here:

```
Resource volume = shard.volume.resources.get();

// Strip out the volume; modify mode; add it back.
Resources taskResources = shard.resources - volume;
volume.mutable_disk()->mutable_volume()->set_mode(Volume::RO);
taskResources += volume;
```

It simplifies the DESTROY logic as well.



src/examples/persistent_volume_framework.cpp (lines 292 - 300)


Can we consolidate the two into one and use a 15 second timeout (a more 
standard timeout in Mesos tests)?

The consumer command in the case of a shared persistent volume should work 
in the regular persistent volume case. When we allow it to be specified via a 
flag, there should be just a single flag. (A TODO for it is fine, although it 
should be pretty trivial to add it)



src/examples/persistent_volume_framework.cpp (lines 313 - 315)


Seems like we can rely on the state `Shard::TERMINATING` to guarantee 
`shard.terminated == shard.tasks` if we define it as "all tasks have 
terminated" and transition the state accordingly.



src/examples/persistent_volume_framework.cpp (lines 325 - 327)


This `operations` is for all shards, `operations.size() == 0` is not 
intuitive and is going to take multiple cycles to settle.

Can we just directly translate the condition for DONE into code: "the 
shard's persistent volume no longer exists in the offer when the state is 
TERMINATING".

If we store the volume resource in `Volume` like I suggested above, we can 
just do `if (!offered.contains(volume)) { /* Done. */ }`.



src/examples/persistent_volume_framework.cpp (lines 372 - 374)


Can we just use `hashset` for taskIds given we have to do this?



src/examples/persistent_volume_framework.cpp (lines 377 - 383)


No need to differentiate since we have defined `Shard::RUNNING` to mean 
"The shard is running (i.e., **all** tasks have launched)". This can apply to 
nonshared persistent volumes too.

So we can 

```
if (shard.launched == shard.tasks &&
shard.state == Shard::STAGING) {
  shard.state = Shard::RUNNING;
}
```



src/examples/persistent_volume_framework.cpp (line 378)


Can `shard.launched > shard.tasks`? If impossible you can `CHECK`.



src/examples/persistent_volume_framework.cpp (lines 387 - 396)


For nonshared persistent volumes no need for transition

STAGING -> RUNNING -> STAGING -> RUNNING ... -> TERMINATING

The following can work in both cases?

```
if 

Re: Review Request 53756: CMake: Added logrotate container logger module to the build.

[Alex Clemmer]

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




src/slave/container_loggers/CMakeLists.txt (line 25)


For my own education, this must be shared, rather than 
`MESOS_DEFAULT_LIBRARY_LINKAGE`? Or am I misunderstanding?



src/slave/container_loggers/CMakeLists.txt (line 30)


Seems like it might be easier to just `if` out the line that includes this 
`CMakeLists.txt`?


- Alex Clemmer


On Nov. 15, 2016, 10:56 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53756/
> ---
> 
> (Updated Nov. 15, 2016, 10:56 p.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-5792
> https://issues.apache.org/jira/browse/MESOS-5792
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This review replaces: https://reviews.apache.org/r/49874/
> 
> 
> Diffs
> -
> 
>   src/slave/CMakeLists.txt 33120ace79bce449329a7cc4b7ef264d2867fc13 
>   src/slave/cmake/SlaveConfigure.cmake 
> b339239761a5de321d65b92376dae69c339bee5c 
>   src/slave/container_loggers/CMakeLists.txt PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/53756/diff/
> 
> 
> Testing
> ---
> 
> cmake ..
> make
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 53761: CMake: Add a target between MESOS_TARGET and MESOS_PROTOBUFs.

[Alex Clemmer]

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




cmake/MesosConfigure.cmake (line 145)


Hmm, can you explain the decision to add `ALL` here? It's not clear to me 
what the implications are.


- Alex Clemmer


On Nov. 15, 2016, 3:11 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53761/
> ---
> 
> (Updated Nov. 15, 2016, 3:11 a.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-5792
> https://issues.apache.org/jira/browse/MESOS-5792
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We need this extra level of targets in order to model the
> dependency chain for binaries and libraries.  An example
> is the mesos-containerizer binary, which depends on libmesos
> (i.e. MESOS_TARGET) but does not get included in the dependency
> chain if you only depend on MESOS_TARGET.
> 
> 
> Diffs
> -
> 
>   cmake/MesosConfigure.cmake de04389b016b22bdff69e94d37fad4eedbda5874 
> 
> Diff: https://reviews.apache.org/r/53761/diff/
> 
> 
> Testing
> ---
> 
> See next review.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 53759: CMake: Change libprocess to a shared library.

[Alex Clemmer]

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




3rdparty/libprocess/src/CMakeLists.txt (line 93)


Seems like this should be using `MESOS_DEFAULT_LIBRARY_LINKAGE`?


- Alex Clemmer


On Nov. 15, 2016, 3:09 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53759/
> ---
> 
> (Updated Nov. 15, 2016, 3:09 a.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-5792
> https://issues.apache.org/jira/browse/MESOS-5792
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In order to load modules that are themselves based on libprocess,
> we must link libprocess as a shared library.  Since modules are
> only supported on non-Windows platforms, this changes the default
> linking mode to SHARED on non-Windows.
> 
> This review replaces: https://reviews.apache.org/r/49924/
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/CMakeLists.txt 
> d1547ef6a8762385f653d3824307727e4d0a7e71 
> 
> Diff: https://reviews.apache.org/r/53759/diff/
> 
> 
> Testing
> ---
> 
> cmake ..
> make
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 53754: CMake: Added test modules that are loaded by mesos tests.

[Alex Clemmer]

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




src/examples/CMakeLists.txt (line 24)


In the other blocks we seem to put this one first. Maybe consider putting 
it first here, too?


- Alex Clemmer


On Nov. 15, 2016, 3:08 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53754/
> ---
> 
> (Updated Nov. 15, 2016, 3:08 a.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-5792
> https://issues.apache.org/jira/browse/MESOS-5792
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Builds shared libraries that are dynamically loaded in the
> module tests.  These are excluded on Windows, which only
> supports static libraries.
> 
> This review replaces: https://reviews.apache.org/r/49863/
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt 1362d65afc23816c34173866a3fdbce45936921d 
>   cmake/MesosConfigure.cmake de04389b016b22bdff69e94d37fad4eedbda5874 
>   src/CMakeLists.txt aef9ae6d2872dc15725c01ce85b657965485605f 
>   src/examples/CMakeLists.txt PRE-CREATION 
>   src/examples/cmake/ExamplesConfigure.cmake PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/53754/diff/
> 
> 
> Testing
> ---
> 
> cmake ..
> make
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Review Request 53853: Expanded the comment around `ContainerInfo` protobuf.

[Alexander Rukletsov]

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

Review request for mesos, Benjamin Bannier, Gilbert Song, and Jie Yu.


Repository: mesos


Description
---

See summary.


Diffs
-

  include/mesos/mesos.proto 380575904a5a29ee53bdc87ae8791ed14e3cafca 
  include/mesos/v1/mesos.proto 5b542ff9cd0b5f301c088d3f4b2fe17ae760d368 

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


Testing
---

None: not a functional change.


Thanks,

Alexander Rukletsov

Re: Review Request 53327: CMake: Added build variables for Mesos tests.

[Alex Clemmer]

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




src/tests/cmake/MesosTestsConfigure.cmake (lines 70 - 71)


It's debatable, I think, whether we should be re-defining these for every 
test package. It seems more likely that we want to put them in some common 
place, since we re-use them for every test package. There's already some 
precedent for this because most lflags are defined by stout configuration.


- Alex Clemmer


On Nov. 15, 2016, 3:08 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53327/
> ---
> 
> (Updated Nov. 15, 2016, 3:08 a.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-5792
> https://issues.apache.org/jira/browse/MESOS-5792
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The patch simply adds the mesos-tests target and renames the
> existing `TEST_HELPER_*` definitions to `MESOS_TEST_*`.
> The dependencies for the `test-helper` and the actual tests will
> be identical.
> 
> This review replaces: https://reviews.apache.org/r/49688/
> 
> 
> Diffs
> -
> 
>   src/tests/CMakeLists.txt cf583ea3f10482b510459fb11a7ecaf76e498391 
>   src/tests/cmake/MesosTestsConfigure.cmake 
> 3f543c010ae87ff04e6b45745bc49ef65b6590ff 
> 
> Diff: https://reviews.apache.org/r/53327/diff/
> 
> 
> Testing
> ---
> 
> cmake ..
> make
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 53832: Fixing broken link for the app frameowrks development guide page.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [53832]

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 Nov. 17, 2016, 12:32 a.m., Miguel Bernadin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53832/
> ---
> 
> (Updated Nov. 17, 2016, 12:32 a.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixing broken link for the app frameowrks development guide. 
> http://mesos.apache.org/documentation/latest/app-framework-development-guide/
> 
> 
> Diffs
> -
> 
>   docs/app-framework-development-guide.md 
> de92c7570cd0d0ac8639ce50a79e5158844ac53c 
> 
> Diff: https://reviews.apache.org/r/53832/diff/
> 
> 
> Testing
> ---
> 
> Validated the files exist locally and contained the correct name. Ensured the 
> correction followed the existing standard.
> 
> 
> Thanks,
> 
> Miguel Bernadin
> 
>

Re: Review Request 52308: Add variable user to handle switchUser passed from executor.

[Sivaram Kannan]

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

(Updated Nov. 17, 2016, 3:38 p.m.)


Review request for mesos and Joseph Wu.


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


Repository: mesos


Description
---

Add variable user to handle switchUser passed from executor.


Diffs (updated)
-

  src/slave/container_loggers/logrotate.hpp 
d1db69236f5a9b1dbb3113ad02218a512afdb46b 

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


Testing
---

1. Run the mesos-logrotate-logger with un-priviledged user and verify whether 
the logs are getting rotated. 
2. Run the mesos-logrotate-logger as root user and verify whether the logs are 
getting rotated.


Thanks,

Sivaram Kannan

Re: Review Request 52310: Switch the uid of the binary if a user is passed from the lib_logrotate.

[Sivaram Kannan]

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

(Updated Nov. 17, 2016, 3:38 p.m.)


Review request for mesos and Joseph Wu.


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


Repository: mesos


Description
---

Switch the uid of the binary if a user is passed from the lib_logrotate.


Diffs (updated)
-

  src/slave/container_loggers/logrotate.cpp 
431bc3cbb54e94359078e4dae0b32ad301393640 

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


Testing
---

1. Run the mesos-logrotate-logger with un-priviledged user and verify whether 
the logs are getting rotated. 
2. Run the mesos-logrotate-logger as root user and verify whether the logs are 
getting rotated.


Thanks,

Sivaram Kannan

Re: Review Request 53473: Add new param user to logrotate's prepare function.

[Sivaram Kannan]

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

(Updated Nov. 17, 2016, 3:38 p.m.)


Review request for mesos and Joseph Wu.


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


Repository: mesos


Description
---

Add new param user to logrotate's prepare function.


Diffs (updated)
-

  include/mesos/slave/container_logger.hpp 
939974736f9eb744c83036e074718d2a1eba8b0a 
  src/slave/container_loggers/lib_logrotate.hpp 
28fdf3bdcc66d473521b377f66ab0b48f6900f58 
  src/slave/container_loggers/lib_logrotate.cpp 
53698d339f0f4d2dc916b53239ca0c36bbebcd42 
  src/slave/container_loggers/logrotate.hpp 
d1db69236f5a9b1dbb3113ad02218a512afdb46b 
  src/slave/container_loggers/sandbox.hpp 
e0aeb32a9ec83af049af8a10010b819c1d8b25d8 
  src/slave/container_loggers/sandbox.cpp 
cc263ebef7e0c3e778fabafa49faa6dd315adc45 
  src/slave/containerizer/docker.cpp c2ed5240aab9ea9d1a386c44c94e5ae7e98d313c 
  src/slave/containerizer/mesos/containerizer.cpp 
ec4ae32485a7ab6c9f73c512004d1220482a188e 
  src/tests/container_logger_tests.cpp 1bb94a8461e481983f25a44737e4011ed5fc4b1f 

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


Testing
---

Run the mesos-logrotate-logger with un-priviledged user and verify whether the 
logs are getting rotated.
Run the mesos-logrotate-logger as root user and verify whether the logs are 
getting rotated.


Thanks,

Sivaram Kannan

Re: Review Request 53699: Add test cases to test logrotate with switch_user set to true and false.

[Sivaram Kannan]

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

(Updated Nov. 17, 2016, 3:38 p.m.)


Review request for mesos and Joseph Wu.


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


Repository: mesos


Description
---

Add test cases to test logrotate with switch_user set to true and false.


Diffs (updated)
-

  src/tests/container_logger_tests.cpp 1bb94a8461e481983f25a44737e4011ed5fc4b1f 

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


Testing
---

Run the mesos-logrotate-logger with un-priviledged user and verify whether the 
logs are getting rotated.
Run the mesos-logrotate-logger as root user and verify whether the logs are 
getting rotated.


Thanks,

Sivaram Kannan

Review Request 53848: Added support for pulling Docker images by digest.

[Ilya Pronin]

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

Review request for mesos.


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


Repository: mesos


Description
---

For now we can only use digests to pull images that were pushed with
Docker 1.9 and older or from Registry 2.2.1 and older. Newer versions
use Schema 2 manifests that are not converted by the registry when
pulling by digest.


Diffs
-

  src/docker/spec.cpp 2f2c32e9b7d78debb31dcc1aa91a4d45c1ced192 
  src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp 
b06ddff68a8d2df13abb838b03a8e73d4e273c31 
  src/tests/containerizer/provisioner_docker_tests.cpp 
96821692a8b2080a25997afa66a0b5e6699c95c4 
  src/uri/fetchers/docker.cpp 3f38dddfb4c089322fe4e13b1ef2070b4835885c 

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


Testing
---

Added a test to verify that an image that has V2 schema 1 manifest can be 
pulled from the repository by digest. Ran `make check`.


Thanks,

Ilya Pronin

Review Request 53849: Added parsing of V2 schema 2 Docker image manifests.

[Ilya Pronin]

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

Review request for mesos.


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


Repository: mesos


Description
---

Added parsing of V2 schema 2 Docker image manifests.


Diffs
-

  include/mesos/docker/spec.hpp b90f731ef07c22259715543526eaf25cb94eab03 
  include/mesos/docker/v2.hpp abab12b8f73564a6da1a0265a503d407a2849b3f 
  src/Makefile.am 5a47c93388234a68c3c486a021ccdbe3213c5bac 
  src/docker/spec.cpp 2f2c32e9b7d78debb31dcc1aa91a4d45c1ced192 
  src/tests/containerizer/docker_spec_tests.cpp 
82bddd228df3db95a00eb277ff9c380039b70b1e 

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


Testing
---

Added tests to verify that V2 schema 2 image manifest can be successfully 
parsed and errors like missing required fields or empty layers list are caught. 
Ran `make check`.


Thanks,

Ilya Pronin

Re: Review Request 53541: WIP: Added authorization actions for debug API.

[Adam B]

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



Nice work with the subclassed NestedContainerObjectApprover.
I'd really like to see at least LaunchNestedContainer wired up in the existing 
v1 API Calls, so we can see how the approver will be called in context.


include/mesos/authorizer/acls.proto (lines 260 - 261)


s/running with/running as/



include/mesos/authorizer/acls.proto (lines 266 - 267)


Ojbects: The operating system users (e.g. linux users) to run the nested 
container command as.



include/mesos/authorizer/acls.proto (lines 271 - 272)


"Which principals are authorized to launch nested containers under a 
top-level container running as the given user."



include/mesos/authorizer/acls.proto (lines 277 - 278)


"Objects: The operating system users (e.g. linux users) of the top-level 
containers under which the principal may launch a nested container."



include/mesos/authorizer/acls.proto (lines 282 - 283)


"Which principals are authorized to launch nested container sessions 
running as the given users."



include/mesos/authorizer/acls.proto (lines 293 - 294)


"Which principals are authorized to launch nested container sessions under 
a top-level container running as the given user."

or "... top-level container whose executor was launched with the given 
user."?



include/mesos/authorizer/acls.proto (lines 386 - 387)


Is this really proper indentation for a Mesos protobuf? Doesn't look like 
we've had to wrap before..



include/mesos/authorizer/authorizer.proto (lines 158 - 159)


"This action will always set `ExecutorInfo` [and `FrameworkInfo`] in the 
object, and optionally `CommandInfo` if available."



include/mesos/authorizer/authorizer.proto (line 160)


Since LNC, KillNC, and WaitNC already exist, could you wire up the 
authorization check for those Calls as a separate patch, so we can see how 
these fields will be filled in?



include/mesos/authorizer/authorizer.proto (line 166)


with `ExecutorInfo` and `FrameworkInfo` set, in case the authorizer wants 
the FrameworkInfo.role? VIEW_EXECUTOR and ACCESS_SANDBOX both set both.



src/authorizer/local/authorizer.cpp (line 502)


Maybe just call it a LocalNestedContainerObjectApprover? (to match 
`getNestedContainerObjectApprover()`)



src/authorizer/local/authorizer.cpp (line 528)


Remind me, are we guaranteed to have an executor_info->command.user at this 
point? Or do we need to check FrameworkInfo.user as a backup?



src/authorizer/local/authorizer.cpp (line 999)


"specialized"


- Adam B


On Nov. 16, 2016, 7:32 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53541/
> ---
> 
> (Updated Nov. 16, 2016, 7:32 a.m.)
> 
> 
> Review request for mesos, Adam B, Kapil Arya, Kevin Klues, and Till Toenshoff.
> 
> 
> Bugs: MESOS-6474
> https://issues.apache.org/jira/browse/MESOS-6474
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> **WIP: Do not commit**
> 
> Added authorization actions for debug API.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/acls.proto 
> e3fd6a4a1b617a75714ebd6e08ab10cffa1a7d1b 
>   include/mesos/authorizer/authorizer.hpp 
> 7217600351bb089dbd5728ce015d962be7498665 
>   include/mesos/authorizer/authorizer.proto 
> 0696a629ac2d2b9950e20708f0c3666b58ff7ca0 
>   src/authorizer/local/authorizer.cpp 
> 77e05dd2475d6e7511e7c7eeea578ec31ff3d198 
>   src/tests/authorization_tests.cpp d23f551c2caa454da0c0f6cb7d77a8c2bd75a474 
> 
> Diff: https://reviews.apache.org/r/53541/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>