Re: Review Request 58942: CHANGELOG for 1.3.0 release.

[Adam B]

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




CHANGELOG
Lines 95 (patched)


I bet we can graduate a few more of these. Maybe not the ones that still 
have "Unresolved Critical Issues", but some of the others.


- Adam B


On May 2, 2017, 4:48 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58942/
> ---
> 
> (Updated May 2, 2017, 4:48 p.m.)
> 
> 
> Review request for mesos and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> CHANGELOG for 1.3.0 release.
> 
> 
> Diffs
> -
> 
>   CHANGELOG 998f30d576d87e72b1d52e8bd1e43ce6ba67a54b 
> 
> 
> Diff: https://reviews.apache.org/r/58942/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 58942: CHANGELOG for 1.3.0 release.

[Adam B]

> On May 3, 2017, 4:48 a.m., Stephan Erb wrote:
> > CHANGELOG
> > Lines 117 (patched)
> > 
> >
> > I was once confused by this "All issues" title in presence of the 
> > "Unresolved Critical Issues". 
> > 
> > Maybe change it to "Newly resolved Issues" or something similar to make 
> > it clearer?

Great feedback. We only recently added the "Unresolved Critical Issues" and 
other sections above.
I like the sound of "All newly resolved issues:" or "All issues resolved in 
this release:".
We have "All" in there because we don't bother to go through the JIRA-generated 
list to remove the issues already mentioned above; otherwise it could be "Other 
newly resolved issues:"


- Adam


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


On May 2, 2017, 4:48 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58942/
> ---
> 
> (Updated May 2, 2017, 4:48 p.m.)
> 
> 
> Review request for mesos and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> CHANGELOG for 1.3.0 release.
> 
> 
> Diffs
> -
> 
>   CHANGELOG 998f30d576d87e72b1d52e8bd1e43ce6ba67a54b 
> 
> 
> Diff: https://reviews.apache.org/r/58942/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 58953: WIP: Added transitional allocator overloads.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [57911, 57997, 57998, 57999, 58048, 58047, 58021, 58071, 
58072, 58949, 58950, 58951, 58952, 58953]

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 May 3, 2017, 12:08 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58953/
> ---
> 
> (Updated May 3, 2017, 12:08 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jie Yu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-7388
> https://issues.apache.org/jira/browse/MESOS-7388
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commits adds allocator overloads working with resource providers
> instead of agents which currently are assumed to be called with agent
> resource providers. This is so that users of the allocator can
> transition to the overloads taking resource providers.
> 
> In a subsequent commit the allocator overloads directly taking agent
> variables will be removed in favor of the ones taking resource
> providers; at the same time all methods in the allocator interface
> will be made virtual again in order to call specific derived
> implementation methods.
> 
> 
> Diffs
> -
> 
>   include/mesos/allocator/allocator.hpp 
> dc34a1b6f0c2bdf0d653bd53394364cfa9873ca7 
> 
> 
> Diff: https://reviews.apache.org/r/58953/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 58923: Added new ContainerLaunchInfo task_environment.

[Adam B]

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



The terminology in this area of the code confuses me. Do we have a glossary for 
the different executors, command tasks, etc. somewhere?


include/mesos/slave/containerizer.proto
Lines 166 (patched)


+1, or just drop "already".
And tell me which non-custom executors do decide to pass their environment 
on to the tasks.

Also, the "Additional environment" confuses me. What is it in addition to? 
The new `task_environment`? Comment should explain the relationship.



include/mesos/slave/containerizer.proto
Lines 180-184 (original), 184-191 (patched)


What's the difference between a `task command` and a `command task`?



include/mesos/slave/containerizer.proto
Lines 192 (patched)


What order did you insert this in? I'd say either go numeric and put it at 
the end, or put it next to `environment` so you can explain the difference.


- Adam B


On May 2, 2017, 10:28 a.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58923/
> ---
> 
> (Updated May 2, 2017, 10:28 a.m.)
> 
> 
> Review request for mesos, Adam B, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-7429
> https://issues.apache.org/jira/browse/MESOS-7429
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/slave/containerizer.proto 
> c30b1fc659ee9b3cd343899638ced6408d8b51a2 
> 
> 
> Diff: https://reviews.apache.org/r/58923/diff/1/
> 
> 
> Testing
> ---
> 
> make check and functional testing on chain end.
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 58224: Optionally verify the source IP address for libprocess messages.

[James Peach]

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

(Updated May 4, 2017, 12:31 a.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Rebase and make pricess flags static.


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


Repository: mesos


Description
---

In general, libprocess is unable to validate that a peer
is a legitimate owner of the UPID it claims in a libprocess
message. This change adds a check that the IP address in the
UPID matches the peer address. This makes spoofing the UPID
harder (eg. to send authenticated messages), but also breaks
some legitimate configurations, particularly on multihomed
hosts.


Diffs (updated)
-

  3rdparty/libprocess/src/process.cpp f5b666f894215cb1861c244c94b382e0739bc5c9 


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

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


Testing (updated)
---

make check (Fedora 25). Light manual testing.

With LIBPROCESS_require_peer_address_ip_match=true, all Mesos tests pass except 
``ExamplesTest.DiskFullFramework``, however enabling this will definitely break 
some libprocess APIs (though not in the way that Mesos uses them) and 
legitimate multi-homed configurations. Note that setting 
LIBPROCESS_ip=127.0.0.1 makes you multihomed for this purpose, which is why 
``ExamplesTest.DiskFullFramework`` breaks.


Thanks,

James Peach

Re: Review Request 58224: Optionally verify the source IP address for libprocess messages.

[James Peach]

> On May 2, 2017, 12:34 a.m., Benjamin Mahler wrote:
> > 3rdparty/libprocess/src/process.cpp
> > Lines 480-492 (patched)
> > 
> >
> > I'm wondering if we can eliminate the need for this via global flag 
> > access and peer caching in the Socket implementation, see other comments.

Fixed the global flag, filed MESOS-7452 for the peer address caching.


> On May 2, 2017, 12:34 a.m., Benjamin Mahler wrote:
> > 3rdparty/libprocess/src/process.cpp
> > Line 942 (original), 965 (patched)
> > 
> >
> > Per our offline discussion, could we acheive the elimination of 
> > `getpeername`-per-read by having the `Socket` perform this optimization for 
> > a connected socket? That would also avoid the need to carry the peer around.

I filed [MESOS-7452](https://issues.apache.org/jira/browse/MESOS-7452) to 
implement the `Socket` peer address caching.


- James


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


On May 3, 2017, 10:35 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58224/
> ---
> 
> (Updated May 3, 2017, 10:35 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7401
> https://issues.apache.org/jira/browse/MESOS-7401
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In general, libprocess is unable to validate that a peer
> is a legitimate owner of the UPID it claims in a libprocess
> message. This change adds a check that the IP address in the
> UPID matches the peer address. This makes spoofing the UPID
> harder (eg. to send authenticated messages), but also breaks
> some legitimate configurations, particularly on multihomed
> hosts.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> f5b666f894215cb1861c244c94b382e0739bc5c9 
> 
> 
> Diff: https://reviews.apache.org/r/58224/diff/5/
> 
> 
> Testing
> ---
> 
> make check (Fedora 25). Light manual testing.
> 
> With LIBPROCESS_pin_peer_address=true, all Mesos tests pass except 
> ``ExamplesTest.DiskFullFramework``, however enabling this will definitely 
> break some libprocess APIs (though not in the way that Mesos uses them) and 
> legitimate multi-homed configurations. Note that setting 
> LIBPROCESS_ip=127.0.0.1 makes you multihomed for this purpose, which is why 
> ``ExamplesTest.DiskFullFramework`` breaks.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Review Request 58980: Passed '--default_container_info' to the command executor.

[Chun-Hung Hsiao]

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

Review request for mesos, Anand Mazumdar, Gilbert Song, and Jie Yu.


Bugs: mesos-7007
https://issues.apache.org/jira/browse/mesos-7007


Repository: mesos


Description
---

When there is no container info in the task info, copy that from the executor 
info so the command executor can set up the container correctly.


Diffs
-

  src/slave/containerizer/mesos/containerizer.cpp 
b58baed64480e22f640a4852537f85922ed382ae 


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


Testing
---

sudo make check
Manually tested on the scenario provided in mesos-7007.


Thanks,

Chun-Hung Hsiao

Re: Review Request 58925: Updated runtime isolators to use new task_environment member.

[Kapil Arya]

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


Ship it!




Ship It!

- Kapil Arya


On May 2, 2017, 1:28 p.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58925/
> ---
> 
> (Updated May 2, 2017, 1:28 p.m.)
> 
> 
> Review request for mesos, Adam B, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-7429
> https://issues.apache.org/jira/browse/MESOS-7429
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> see summary.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/appc/runtime.cpp 
> ffaec5a0112db36c1e9123e361ada63a4aedf0ad 
>   src/slave/containerizer/mesos/isolators/docker/runtime.cpp 
> 08350e638a0f20746e369cdc78c96126f2e1df3f 
> 
> 
> Diff: https://reviews.apache.org/r/58925/diff/1/
> 
> 
> Testing
> ---
> 
> make check and functional testing on chain end.
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 56889: Added setting volume mode and ownership in LinuxFilesystemIsolator.

[Chun-Hung Hsiao]

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




src/slave/containerizer/mesos/isolators/filesystem/linux.cpp
Lines 490 (patched)


The mount target might not be just volumn.container_path().


- Chun-Hung Hsiao


On Feb. 22, 2017, 10:58 a.m., Ilya Pronin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56889/
> ---
> 
> (Updated Feb. 22, 2017, 10:58 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-6563 and MESOS-7069
> https://issues.apache.org/jira/browse/MESOS-6563
> https://issues.apache.org/jira/browse/MESOS-7069
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `LinuxFilesystemIsolator` now sets ownership and permissions of the newly 
> created relative host path to match the absolute container path.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
> ae0031d8d8d6dfe0334b605fbb85e83de88ab436 
>   src/tests/containerizer/linux_filesystem_isolator_tests.cpp 
> c27335731924509632ec96cc01a4b4415f108a30 
> 
> 
> Diff: https://reviews.apache.org/r/56889/diff/1/
> 
> 
> Testing
> ---
> 
> Added permissions and ownerchip check to 
> `LinuxFilesystemIsolatorTest.ROOT_VolumeFromSandbox`. Ran `make check`.
> 
> 
> Thanks,
> 
> Ilya Pronin
> 
>

Re: Review Request 56889: Added setting volume mode and ownership in LinuxFilesystemIsolator.

[Chun-Hung Hsiao]

> On May 3, 2017, 11:12 p.m., Chun-Hung Hsiao wrote:
> > src/tests/containerizer/linux_filesystem_isolator_tests.cpp
> > Lines 253 (patched)
> > 
> >
> > We should check the mode of the "tmp" directory against the container 
> > directory, not "/tmp". Also, shouldn't we use a non-root test to test the 
> > ownership?
> 
> Chun-Hung Hsiao wrote:
> Sorry I mean a root test with a task executed by some other user. Not 
> sure if we can do this in the unit test.

Oops. I misunderstood the ownership. I was thinking about chown/chmod to be the 
same as the sandbox.


- Chun-Hung


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


On Feb. 22, 2017, 10:58 a.m., Ilya Pronin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56889/
> ---
> 
> (Updated Feb. 22, 2017, 10:58 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-6563 and MESOS-7069
> https://issues.apache.org/jira/browse/MESOS-6563
> https://issues.apache.org/jira/browse/MESOS-7069
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `LinuxFilesystemIsolator` now sets ownership and permissions of the newly 
> created relative host path to match the absolute container path.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
> ae0031d8d8d6dfe0334b605fbb85e83de88ab436 
>   src/tests/containerizer/linux_filesystem_isolator_tests.cpp 
> c27335731924509632ec96cc01a4b4415f108a30 
> 
> 
> Diff: https://reviews.apache.org/r/56889/diff/1/
> 
> 
> Testing
> ---
> 
> Added permissions and ownerchip check to 
> `LinuxFilesystemIsolatorTest.ROOT_VolumeFromSandbox`. Ran `make check`.
> 
> 
> Thanks,
> 
> Ilya Pronin
> 
>

Re: Review Request 58955: Adds authorization for the master's v1 API call SET_LOGGING_LEVEL.

[Greg Mann]

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



LGTM. Minor points below.


src/tests/api_tests.cpp
Lines 741 (patched)


s/launch nested container sessions/set the logging level/



src/tests/api_tests.cpp
Lines 742-744 (patched)


Could fit on one line?



src/tests/api_tests.cpp
Lines 757-774 (original), 768-803 (patched)


What do you think about placing each of these requests into an enclosing 
scope? It's a pattern we follow elsewhere, and I think it makes the code a bit 
more readable.

i.e.
```
{
  http::Headers headers = stuff;
  Future response = http::post( ... );
  AWAIT_READY(response);
}

{
  http::Headers headers = stuff;
  Future response = http::post( ... );
  AWAIT_READY(response);
  ASSERT_EQ( ... );
}
```


- Greg Mann


On May 3, 2017, 9:26 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58955/
> ---
> 
> (Updated May 3, 2017, 9:26 a.m.)
> 
> 
> Review request for mesos, Adam B and Greg Mann.
> 
> 
> Bugs: MESOS-7414
> https://issues.apache.org/jira/browse/MESOS-7414
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Enables authorization in the v1 API call `SET_LOGGING_LEVEL` following
> the existing implementation for the agent call with the same name.
> Likewise it reuses the authorizer action `SET_LOG_LEVEL`.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 5aae52870451d883ef1ea1fda5a27764d7f318e8 
>   src/tests/api_tests.cpp faf3242f9c86d866c4bb5e457fcfe47c1063cc09 
> 
> 
> Diff: https://reviews.apache.org/r/58955/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 56889: Added setting volume mode and ownership in LinuxFilesystemIsolator.

[Chun-Hung Hsiao]

> On May 3, 2017, 11:12 p.m., Chun-Hung Hsiao wrote:
> > src/tests/containerizer/linux_filesystem_isolator_tests.cpp
> > Lines 253 (patched)
> > 
> >
> > We should check the mode of the "tmp" directory against the container 
> > directory, not "/tmp". Also, shouldn't we use a non-root test to test the 
> > ownership?

Sorry I mean a root test with a task executed by some other user. Not sure if 
we can do this in the unit test.


- Chun-Hung


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


On Feb. 22, 2017, 10:58 a.m., Ilya Pronin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56889/
> ---
> 
> (Updated Feb. 22, 2017, 10:58 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-6563 and MESOS-7069
> https://issues.apache.org/jira/browse/MESOS-6563
> https://issues.apache.org/jira/browse/MESOS-7069
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `LinuxFilesystemIsolator` now sets ownership and permissions of the newly 
> created relative host path to match the absolute container path.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
> ae0031d8d8d6dfe0334b605fbb85e83de88ab436 
>   src/tests/containerizer/linux_filesystem_isolator_tests.cpp 
> c27335731924509632ec96cc01a4b4415f108a30 
> 
> 
> Diff: https://reviews.apache.org/r/56889/diff/1/
> 
> 
> Testing
> ---
> 
> Added permissions and ownerchip check to 
> `LinuxFilesystemIsolatorTest.ROOT_VolumeFromSandbox`. Ran `make check`.
> 
> 
> Thanks,
> 
> Ilya Pronin
> 
>

Re: Review Request 56889: Added setting volume mode and ownership in LinuxFilesystemIsolator.

[Chun-Hung Hsiao]

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




src/tests/containerizer/linux_filesystem_isolator_tests.cpp
Lines 253 (patched)


We should check the mode of the "tmp" directory against the container 
directory, not "/tmp". Also, shouldn't we use a non-root test to test the 
ownership?


- Chun-Hung Hsiao


On Feb. 22, 2017, 10:58 a.m., Ilya Pronin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56889/
> ---
> 
> (Updated Feb. 22, 2017, 10:58 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-6563 and MESOS-7069
> https://issues.apache.org/jira/browse/MESOS-6563
> https://issues.apache.org/jira/browse/MESOS-7069
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `LinuxFilesystemIsolator` now sets ownership and permissions of the newly 
> created relative host path to match the absolute container path.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
> ae0031d8d8d6dfe0334b605fbb85e83de88ab436 
>   src/tests/containerizer/linux_filesystem_isolator_tests.cpp 
> c27335731924509632ec96cc01a4b4415f108a30 
> 
> 
> Diff: https://reviews.apache.org/r/56889/diff/1/
> 
> 
> Testing
> ---
> 
> Added permissions and ownerchip check to 
> `LinuxFilesystemIsolatorTest.ROOT_VolumeFromSandbox`. Ran `make check`.
> 
> 
> Thanks,
> 
> Ilya Pronin
> 
>

Re: Review Request 58928: Update process tests to use a non-zero UPID.

[James Peach]

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

(Updated May 3, 2017, 10:36 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Rebased and addressed review feedback.


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


Repository: mesos


Description
---

Some of the remote process tests were using a UPID with a zero
IP address field. In order to enable subsequent IP address
validation in the libprocess receiver, update these tests to
use a UPID containing the real IP address of the sender.


Diffs (updated)
-

  3rdparty/libprocess/src/tests/process_tests.cpp 
bf90c7e78fd50ad7e16cc0a69a248ba71e2a7115 
  3rdparty/libprocess/src/tests/test_linkee.cpp 
921d67695bc0e4d601e9f74fbc625d69bf36ba50 


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

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


Testing
---

make check (Fedora 25)


Thanks,

James Peach

Re: Review Request 58224: Optionally verify the source IP address for libprocess messages.

[James Peach]

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

(Updated May 3, 2017, 10:35 p.m.)


Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

In general, libprocess is unable to validate that a peer
is a legitimate owner of the UPID it claims in a libprocess
message. This change adds a check that the IP address in the
UPID matches the peer address. This makes spoofing the UPID
harder (eg. to send authenticated messages), but also breaks
some legitimate configurations, particularly on multihomed
hosts.


Diffs (updated)
-

  3rdparty/libprocess/src/process.cpp f5b666f894215cb1861c244c94b382e0739bc5c9 


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

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


Testing
---

make check (Fedora 25). Light manual testing.

With LIBPROCESS_pin_peer_address=true, all Mesos tests pass except 
``ExamplesTest.DiskFullFramework``, however enabling this will definitely break 
some libprocess APIs (though not in the way that Mesos uses them) and 
legitimate multi-homed configurations. Note that setting 
LIBPROCESS_ip=127.0.0.1 makes you multihomed for this purpose, which is why 
``ExamplesTest.DiskFullFramework`` breaks.


Thanks,

James Peach

Review Request 58977: Add local and peer address accessors to http::Connection.

[James Peach]

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

Add local and peer address accessors to http::Connection.


Diffs
-

  3rdparty/libprocess/include/process/http.hpp 
650b9d8aaba5636e1a445abf9e27e018ff82e610 
  3rdparty/libprocess/src/http.cpp 9789607933745f1fc4e37f47ce1be6aecb33a6e6 


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


Testing
---

make check (Fedora 25)


Thanks,

James Peach

Re: Review Request 58923: Added new ContainerLaunchInfo task_environment.

[Kapil Arya]

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


Fix it, then Ship it!





include/mesos/slave/containerizer.proto
Lines 166 (patched)


s/for the executor already/only for the executor/?


- Kapil Arya


On May 2, 2017, 1:28 p.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58923/
> ---
> 
> (Updated May 2, 2017, 1:28 p.m.)
> 
> 
> Review request for mesos, Adam B, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-7429
> https://issues.apache.org/jira/browse/MESOS-7429
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/slave/containerizer.proto 
> c30b1fc659ee9b3cd343899638ced6408d8b51a2 
> 
> 
> Diff: https://reviews.apache.org/r/58923/diff/1/
> 
> 
> Testing
> ---
> 
> make check and functional testing on chain end.
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 58924: Updated containerizer for isolator task_environment merge.

[Kapil Arya]

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


Ship it!




Ship It!

- Kapil Arya


On May 2, 2017, 1:28 p.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58924/
> ---
> 
> (Updated May 2, 2017, 1:28 p.m.)
> 
> 
> Review request for mesos, Adam B, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-7429
> https://issues.apache.org/jira/browse/MESOS-7429
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> b58baed64480e22f640a4852537f85922ed382ae 
> 
> 
> Diff: https://reviews.apache.org/r/58924/diff/1/
> 
> 
> Testing
> ---
> 
> make check and functional testing on chain end.
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 58939: Filesystem isolation check for Mesos image provisioner.

[Chun-Hung Hsiao]

> On May 3, 2017, 7:46 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/containerizer.cpp
> > Lines 1178-1186 (patched)
> > 
> >
> > This might be too late. We want to check during startup time of the 
> > agent, rather than wait until the container launch time.

The mount namespace is required only when provisioning images. I think checking 
at startup would introduce unnecessary failures for agents running tasks that 
do not use images.


- Chun-Hung


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


On May 3, 2017, 10:07 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58939/
> ---
> 
> (Updated May 3, 2017, 10:07 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: mesos-7374
> https://issues.apache.org/jira/browse/mesos-7374
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Checked if the 'filesystem/linux' isolator is enabled and the 'linux'
> launcher is used when launching a mesos containerizer with an image
> under Linux. This prevents the executor from messing up with the host
> filesystem. The check is in `MesosContainerizerProcess::prepare()`
> after provisioning and before launching, since provisioning itself
> does not depend on the filesystem isolator.
> 
> Also checked that the 'filesystem/linux' is enabled and the 'linux'
> launcher is used when enabling the 'docker/runtime' isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> b58baed64480e22f640a4852537f85922ed382ae 
>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 
> be45fc59027f176b43b767e9441fd8089ceec7b4 
> 
> 
> Diff: https://reviews.apache.org/r/58939/diff/2/
> 
> 
> Testing
> ---
> 
> sudo make check
> Manually tested on a simplified case of mesos-7374.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 58939: Filesystem isolation check for Mesos image provisioner.

[Chun-Hung Hsiao]

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

(Updated May 3, 2017, 10:07 p.m.)


Review request for mesos, Anand Mazumdar, Gilbert Song, and Jie Yu.


Changes
---

Added checks for 'linux' launcher dependencies.


Summary (updated)
-

Filesystem isolation check for Mesos image provisioner.


Bugs: mesos-7374
https://issues.apache.org/jira/browse/mesos-7374


Repository: mesos


Description (updated)
---

Checked if the 'filesystem/linux' isolator is enabled and the 'linux'
launcher is used when launching a mesos containerizer with an image
under Linux. This prevents the executor from messing up with the host
filesystem. The check is in `MesosContainerizerProcess::prepare()`
after provisioning and before launching, since provisioning itself
does not depend on the filesystem isolator.

Also checked that the 'filesystem/linux' is enabled and the 'linux'
launcher is used when enabling the 'docker/runtime' isolator.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.cpp 
b58baed64480e22f640a4852537f85922ed382ae 
  src/slave/containerizer/mesos/provisioner/provisioner.cpp 
be45fc59027f176b43b767e9441fd8089ceec7b4 


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

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


Testing
---

sudo make check
Manually tested on a simplified case of mesos-7374.


Thanks,

Chun-Hung Hsiao

Re: Review Request 58310: Removed workaround for lack of `thread_local` on OS X.

[Anand Mazumdar]

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


Ship it!




Ship It!

- Anand Mazumdar


On April 10, 2017, 8:06 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58310/
> ---
> 
> (Updated April 10, 2017, 8:06 p.m.)
> 
> 
> Review request for mesos and Anand Mazumdar.
> 
> 
> Bugs: MESOS-7373
> https://issues.apache.org/jira/browse/MESOS-7373
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Until XCode 8, Apple's build toolchain did not support the
> `thread_local` feature from C++11. Hence, Stout provided a
> `THREAD_LOCAL` macro, which expanded to `__thread` on OS X and
> `thread_local` on other platforms.
> 
> XCode 8 added support for `thread_local`, so we can remove this
> workaround. Note that XCode 8 requires OS X 10.11.5 or newer.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/thread_local.hpp 
> 7ffc809c720abe477fb92851d530ae49dcb23f49 
> 
> 
> Diff: https://reviews.apache.org/r/58310/diff/1/
> 
> 
> Testing
> ---
> 
> `make check` on OSX with XCode 8, internal CI build, `make check` on CentOS 
> 6.6
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 58314: Removed `thread_local.hpp` from stout.

[Anand Mazumdar]

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


Ship it!




Ship It!

- Anand Mazumdar


On May 3, 2017, 9:15 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58314/
> ---
> 
> (Updated May 3, 2017, 9:15 p.m.)
> 
> 
> Review request for mesos and Anand Mazumdar.
> 
> 
> Bugs: MESOS-7373
> https://issues.apache.org/jira/browse/MESOS-7373
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed `thread_local.hpp` from stout.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/Makefile.am 04273ee65388e09f365ed730aeac757638552473 
>   3rdparty/stout/include/stout/thread_local.hpp 
> 7ffc809c720abe477fb92851d530ae49dcb23f49 
> 
> 
> Diff: https://reviews.apache.org/r/58314/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 58314: Removed `thread_local.hpp` from stout.

[Neil Conway]

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

(Updated May 3, 2017, 9:15 p.m.)


Review request for mesos and Anand Mazumdar.


Changes
---

Rebase.


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


Repository: mesos


Description
---

Removed `thread_local.hpp` from stout.


Diffs (updated)
-

  3rdparty/stout/include/Makefile.am 04273ee65388e09f365ed730aeac757638552473 
  3rdparty/stout/include/stout/thread_local.hpp 
7ffc809c720abe477fb92851d530ae49dcb23f49 


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

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


Testing
---

`make check`


Thanks,

Neil Conway

Re: Review Request 58313: Replaced `THREAD_LOCAL` with `thread_local` in stout.

[Anand Mazumdar]

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


Ship it!




Ship It!

- Anand Mazumdar


On April 10, 2017, 8:07 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58313/
> ---
> 
> (Updated April 10, 2017, 8:07 p.m.)
> 
> 
> Review request for mesos and Anand Mazumdar.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Replaced `THREAD_LOCAL` with `thread_local` in stout.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/windows/mkdtemp.hpp 
> ef6a78a5bf8f5bb7cab7f3941695d277c28b8c6c 
>   3rdparty/stout/include/stout/uuid.hpp 
> b84299caf576a1a0431d4192c08e1384fb7aa3ab 
> 
> 
> Diff: https://reviews.apache.org/r/58313/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 58312: Replaced `THREAD_LOCAL` with `thread_local` in libprocess.

[Anand Mazumdar]

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


Ship it!




Ship It!

- Anand Mazumdar


On April 10, 2017, 8:07 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58312/
> ---
> 
> (Updated April 10, 2017, 8:07 p.m.)
> 
> 
> Review request for mesos and Anand Mazumdar.
> 
> 
> Bugs: MESOS-7373
> https://issues.apache.org/jira/browse/MESOS-7373
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Replaced `THREAD_LOCAL` with `thread_local` in libprocess.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/executor.hpp 
> 7faa2ab3d6828f0c7b411341e6a58bb8d0429ebc 
>   3rdparty/libprocess/include/process/process.hpp 
> 401510c8f7b7d546d5364756dcab3245207ca5ab 
>   3rdparty/libprocess/src/libev.hpp 2e67967b7aa06f48e7c6cd22436879ab7278e9ad 
>   3rdparty/libprocess/src/libev.cpp 8db9c3b92b591c6bb8fe9f647355922e1d803547 
>   3rdparty/libprocess/src/libevent.hpp 
> e7f273eac8b78da730873bcc040ebaac98a52b39 
>   3rdparty/libprocess/src/libevent.cpp 
> 093569ec06dbfe45bf7b6f90a3ec69b3490319e6 
>   3rdparty/libprocess/src/process.cpp 
> d0cba0c2299bddfedeb8bfde5b93aae733a9cd5b 
> 
> 
> Diff: https://reviews.apache.org/r/58312/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 58311: Updated "getting started" instructions for XCode 8.0 dependency.

[Anand Mazumdar]

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


Ship it!




Ship It!

- Anand Mazumdar


On April 10, 2017, 8:06 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58311/
> ---
> 
> (Updated April 10, 2017, 8:06 p.m.)
> 
> 
> Review request for mesos and Anand Mazumdar.
> 
> 
> Bugs: MESOS-7373
> https://issues.apache.org/jira/browse/MESOS-7373
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated "getting started" instructions for XCode 8.0 dependency.
> 
> 
> Diffs
> -
> 
>   docs/getting-started.md 2578a291e242e21caea0ef24f8fff820d9428f76 
> 
> 
> Diff: https://reviews.apache.org/r/58311/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Review Request 58974: Fixed flakiness in HierarchicalAllocatorTest.NestedRoleQuota.

[Neil Conway]

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

Review request for mesos and Alexander Rukletsov.


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


Repository: mesos


Description
---

Fixed flakiness in HierarchicalAllocatorTest.NestedRoleQuota.


Diffs
-

  src/tests/hierarchical_allocator_tests.cpp 
ebc4868a6b7e2a04cc202a74a4633969ade40aca 


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


Testing
---

`make check`

I wasn't able to repro the flakiness on my local box, but the previous test 
coding does seem wrong, now that `setQuota` doesn't trigger a batch allocation.


Thanks,

Neil Conway

Re: Review Request 58971: Switched to using unsigned types to represent versions in stout.

[Neil Conway]

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

(Updated May 3, 2017, 7:55 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Tweak comment.


Repository: mesos


Description
---

Switched to using unsigned types to represent versions in stout.


Diffs (updated)
-

  3rdparty/stout/include/stout/version.hpp 
7717c85b95d29cefe8f19f3cada4b7402d4d446f 


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

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


Testing
---

`make check`


Thanks,

Neil Conway

Re: Review Request 58971: Switched to using unsigned types to represent versions in stout.

[Neil Conway]

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

(Updated May 3, 2017, 7:51 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Switch to using `uint32_t`.


Repository: mesos


Description
---

Switched to using unsigned types to represent versions in stout.


Diffs (updated)
-

  3rdparty/stout/include/stout/version.hpp 
7717c85b95d29cefe8f19f3cada4b7402d4d446f 


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

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


Testing
---

`make check`


Thanks,

Neil Conway

Re: Review Request 58971: Switched to using unsigned types to represent versions in stout.

[Neil Conway]

> On May 3, 2017, 7:41 p.m., Benjamin Mahler wrote:
> > 3rdparty/stout/include/stout/version.hpp
> > Lines 282-283 (original), 283-284 (patched)
> > 
> >
> > Looks like we're a bit inconsistent about this in the code base, but it 
> > seems to me that being explicit about this being an 'int' is more readable 
> > than relying on the implicit 'int' (i.e. reader doesn't have to know that 
> > the equivalent type of 'unsigned' is 'unsigned int').
> > 
> > That would mean we only use the 'Equivalent Type's in the table here 
> > (outside of the fixed width types like uint32_t): 
> > http://en.cppreference.com/w/cpp/language/types
> > 
> > Which also seems like an easy code style rule to enforce?
> > 
> > Alternatively, uint32_t or uint64_t works here if you want the width to 
> > be clear.

Fair enough -- I decided to go with `uint32_t`.


- Neil


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


On May 3, 2017, 6:28 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58971/
> ---
> 
> (Updated May 3, 2017, 6:28 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Switched to using unsigned types to represent versions in stout.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/version.hpp 
> 7717c85b95d29cefe8f19f3cada4b7402d4d446f 
> 
> 
> Diff: https://reviews.apache.org/r/58971/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 58939: Filesystem isolation check for Mesos containerizer under Linux.

[Jie Yu]

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




src/slave/containerizer/mesos/containerizer.cpp
Lines 1178-1186 (patched)


This might be too late. We want to check during startup time of the agent, 
rather than wait until the container launch time.


- Jie Yu


On May 2, 2017, 11:59 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58939/
> ---
> 
> (Updated May 2, 2017, 11:59 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: mesos-7374
> https://issues.apache.org/jira/browse/mesos-7374
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Checked if the 'filesystem/linux' isolator is active when launching a
> mesos containerizer with an image under Linux. This prevents the
> executor from messing up with the host filesystem when launching nested
> containers with failures.
> 
> The check is placed in `MesosContainerizerProcess::prepare()`, which is
> executed after provisioning but before launching, since the isolator is
> not required for the provisioner, so the existing unit tests for
> provisioners won't fail.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> b58baed64480e22f640a4852537f85922ed382ae 
>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 
> be45fc59027f176b43b767e9441fd8089ceec7b4 
> 
> 
> Diff: https://reviews.apache.org/r/58939/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> Manually tested on a simplified case of mesos-7374.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 58874: Stripped spaces between directory elements in WebUI.

[Benjamin Mahler]

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




src/webui/master/static/browse.html
Lines 15-16 (original), 15-16 (patched)


Looking at the gif, it seems the slashes weren't copied before? Do you want 
to clarify that here? Specifically, it seems this also ensures the slashes are 
being copied?



src/webui/master/static/css/mesos.css
Lines 179-181 (patched)


Can you add a comment on what this does and when one would need to use it?


- Benjamin Mahler


On May 3, 2017, 4:29 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58874/
> ---
> 
> (Updated May 3, 2017, 4:29 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Tomasz Janiszewski.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stripped spaces between directory elements in WebUI.
> 
> 
> Diffs
> -
> 
>   src/webui/master/static/browse.html 
> b9849197227b06df348789a49348e2b5d4cfd2ae 
>   src/webui/master/static/css/mesos.css 
> 9f3de5427071fc61d3791c4bc2a660368c2cd3c2 
> 
> 
> Diff: https://reviews.apache.org/r/58874/diff/2/
> 
> 
> Testing
> ---
> 
> 
> File Attachments
> 
> 
> strip_space.gif
>   
> https://reviews.apache.org/media/uploaded/files/2017/05/03/79afd0b1-5fb1-437c-a91c-732009af8fe3__strip_space.gif
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 58971: Switched to using unsigned types to represent versions in stout.

[Benjamin Mahler]

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


Fix it, then Ship it!





3rdparty/stout/include/stout/version.hpp
Lines 282-283 (original), 283-284 (patched)


Looks like we're a bit inconsistent about this in the code base, but it 
seems to me that being explicit about this being an 'int' is more readable than 
relying on the implicit 'int' (i.e. reader doesn't have to know that the 
equivalent type of 'unsigned' is 'unsigned int').

That would mean we only use the 'Equivalent Type's in the table here 
(outside of the fixed width types like uint32_t): 
http://en.cppreference.com/w/cpp/language/types

Which also seems like an easy code style rule to enforce?

Alternatively, uint32_t or uint64_t works here if you want the width to be 
clear.


- Benjamin Mahler


On May 3, 2017, 6:28 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58971/
> ---
> 
> (Updated May 3, 2017, 6:28 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Switched to using unsigned types to represent versions in stout.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/version.hpp 
> 7717c85b95d29cefe8f19f3cada4b7402d4d446f 
> 
> 
> Diff: https://reviews.apache.org/r/58971/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 58874: Stripped spaces between directory elements in WebUI.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [58874]

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 May 3, 2017, 4:29 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58874/
> ---
> 
> (Updated May 3, 2017, 4:29 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Tomasz Janiszewski.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stripped spaces between directory elements in WebUI.
> 
> 
> Diffs
> -
> 
>   src/webui/master/static/browse.html 
> b9849197227b06df348789a49348e2b5d4cfd2ae 
>   src/webui/master/static/css/mesos.css 
> 9f3de5427071fc61d3791c4bc2a660368c2cd3c2 
> 
> 
> Diff: https://reviews.apache.org/r/58874/diff/2/
> 
> 
> Testing
> ---
> 
> 
> File Attachments
> 
> 
> strip_space.gif
>   
> https://reviews.apache.org/media/uploaded/files/2017/05/03/79afd0b1-5fb1-437c-a91c-732009af8fe3__strip_space.gif
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 58709: Prevent old Mesos agents from registering or re-registering.

[Neil Conway]

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

(Updated May 3, 2017, 6:44 p.m.)


Review request for mesos, Benjamin Mahler and Kapil Arya.


Changes
---

Fix test flakiness.


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


Repository: mesos


Description
---

Officially, Mesos 1.0.0 (and newer) masters do not support pre-1.0.0
Mesos agents. However, we previously allowed old agents to register,
which resulted in several master crashes. As a short-term solution, we
fixed those crashes by adding backward compatibility mechanisms into the
master, but that backward compatibility code has made the master logic
more complicated and difficult to understand.

This commit changes the master to ignore registration attempts by Mesos
agents that precede Mesos 1.0.0. Now that this safety check is in place,
master logic can safely assume that all agents are running at least
Mesos 1.0.0, which will allow several simplifications to be made.


Diffs (updated)
-

  src/master/constants.hpp 7edf9f65f6615b67ad0663f75ea7ee72a6a558cb 
  src/master/master.cpp 87c647b9494628fff57fdb45f919e8ae73b467b0 
  src/tests/master_tests.cpp 7cb4774fae1feff007adacf6521fadde2f58bee7 


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

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


Testing
---

`make check`


Thanks,

Neil Conway

Re: Review Request 58944: Fixed compilation error on recent glibc for device whitelist.

[Zhongbo Tian]

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

(Updated May 3, 2017, 6:40 p.m.)


Review request for mesos.


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


Repository: mesos


Description
---

Fixed compilation error on recent glibc for device whitelist.


Diffs
-

  src/slave/containerizer/mesos/isolators/cgroups/subsystems/devices.cpp 
3055b7d41d31a353d672c8934b8545c01b70d2de 


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


Testing
---


Thanks,

Zhongbo Tian

Review Request 58971: Switched to using unsigned types to represent versions in stout.

[Neil Conway]

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

Review request for mesos and Benjamin Mahler.


Repository: mesos


Description
---

Switched to using unsigned types to represent versions in stout.


Diffs
-

  3rdparty/stout/include/stout/version.hpp 
7717c85b95d29cefe8f19f3cada4b7402d4d446f 


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


Testing
---

`make check`


Thanks,

Neil Conway

Re: Review Request 58707: Enhanced stout's Version to support prerelease and build labels.

[Neil Conway]

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

(Updated May 3, 2017, 6:23 p.m.)


Review request for mesos, Benjamin Mahler and Kapil Arya.


Changes
---

Change handling of prerelease identifiers that start with hyphens.


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


Repository: mesos


Description
---

Previously, Stout's `Version` abstraction only supported a subset of
Semver: version numbers with three numeric components (an optional
trailing "label" with a leading hyphen was supported but ignored).

This commit adds support for SemVer 2.0.0, which defines two additional
optional fields: a "prerelease label" and a "build metadata label",
e.g., "1.2.3-alpha.1+foo". Both labels consist of a series of
dot-separated identifiers.


Diffs (updated)
-

  3rdparty/stout/include/stout/version.hpp 
7717c85b95d29cefe8f19f3cada4b7402d4d446f 
  3rdparty/stout/tests/version_tests.cpp 
925f3833fd8b1732c35663df3e63c161261e9bff 


Diff: https://reviews.apache.org/r/58707/diff/7/

Changes: https://reviews.apache.org/r/58707/diff/6-7/


Testing
---

`make check`


Thanks,

Neil Conway

Re: Review Request 58707: Enhanced stout's Version to support prerelease and build labels.

[Neil Conway]

> On May 2, 2017, 9:13 p.m., Benjamin Mahler wrote:
> > 3rdparty/stout/include/stout/version.hpp
> > Lines 102 (patched)
> > 
> >
> > I guess this breaks for the negative case? E.g. 1.0.0--1.-1
> > 
> > We'll treat the -1's as large numbers based on the numify test case you 
> > showed earlier? Rather than being invalid (no negative allowed) or 
> > non-numeric (due to the hyphen). Not clear which one is correct from the 
> > spec AFAICT, unfortunately :(

Per offline discussion (and https://github.com/mojombo/semver/issues/324), 
prerelease identifiers that begin with hyphens must be supported but should be 
treated as non-numeric.


> On May 2, 2017, 9:13 p.m., Benjamin Mahler wrote:
> > 3rdparty/stout/include/stout/version.hpp
> > Lines 243-264 (patched)
> > 
> >
> > Should we keep the notion of "other" here rather than "1" and "2"?
> > 
> > I found the nesting here to make this a little hard to follow, maybe we 
> > can use a flat list of cases to make this easier to read?
> > 
> > ```
> >   if (identifier.isSome() && other_identifier.isSome()) {
> > // Both are numeric.
> > if (identifier.get() != other_identifier.get()) {
> >   return identifier.get() < other_identifier.get();
> > }
> >   } else if (identifier.isSome()) {
> > // If `this` identifier is numeric but `other` is not, `this < 
> > other`.
> > return true;
> >   } else if (other_identifier.isSome()) {
> > // If `other` identifier is numeric but `this` is not, `other < 
> > this`.
> > return false;
> >   } else {
> > // Neither identifier is numeric, so compare them via ASCII 
> > sort.
> > if (prerelease.at(i) != other.prerelease.at(i)) {
> >   return prerelease.at(i) < other.prerelease.at(i);
> > }
> >   }
> > ```

Thanks, that is a nice cleanup.


- Neil


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


On May 3, 2017, 5:30 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58707/
> ---
> 
> (Updated May 3, 2017, 5:30 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Kapil Arya.
> 
> 
> Bugs: MESOS-1987
> https://issues.apache.org/jira/browse/MESOS-1987
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, Stout's `Version` abstraction only supported a subset of
> Semver: version numbers with three numeric components (an optional
> trailing "label" with a leading hyphen was supported but ignored).
> 
> This commit adds support for SemVer 2.0.0, which defines two additional
> optional fields: a "prerelease label" and a "build metadata label",
> e.g., "1.2.3-alpha.1+foo". Both labels consist of a series of
> dot-separated identifiers.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/version.hpp 
> 7717c85b95d29cefe8f19f3cada4b7402d4d446f 
>   3rdparty/stout/tests/version_tests.cpp 
> 925f3833fd8b1732c35663df3e63c161261e9bff 
> 
> 
> Diff: https://reviews.apache.org/r/58707/diff/6/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 58707: Enhanced stout's Version to support prerelease and build labels.

[Neil Conway]

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

(Updated May 3, 2017, 5:30 p.m.)


Review request for mesos, Benjamin Mahler and Kapil Arya.


Changes
---

Address review comments.


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


Repository: mesos


Description
---

Previously, Stout's `Version` abstraction only supported a subset of
Semver: version numbers with three numeric components (an optional
trailing "label" with a leading hyphen was supported but ignored).

This commit adds support for SemVer 2.0.0, which defines two additional
optional fields: a "prerelease label" and a "build metadata label",
e.g., "1.2.3-alpha.1+foo". Both labels consist of a series of
dot-separated identifiers.


Diffs (updated)
-

  3rdparty/stout/include/stout/version.hpp 
7717c85b95d29cefe8f19f3cada4b7402d4d446f 
  3rdparty/stout/tests/version_tests.cpp 
925f3833fd8b1732c35663df3e63c161261e9bff 


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

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


Testing
---

`make check`


Thanks,

Neil Conway

Re: Review Request 58820: Explained how container working directory differs from its sandbox.

[Alexander Rukletsov]

> On April 28, 2017, 8:37 p.m., Vinod Kone wrote:
> > include/mesos/slave/containerizer.proto
> > Line 186 (original), 186 (patched)
> > 
> >
> > s/sandbox/sandbox in the host (or is it container?) mount namespace/ ?
> > 
> > also, not clear what "Mesos sandbox" means? do you mean the value of 
> > 'MESOS_SANDBOX' env variable?

Well, the container can always access its sandbox, regardless whether it shares 
the host filesystem or not. I'm not sure we should refer to the env variable 
name here, even though we now indeed set both to the same value (modulo the 
comment). Jie, what do you think?


- Alexander


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


On May 3, 2017, 4:45 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58820/
> ---
> 
> (Updated May 3, 2017, 4:45 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/slave/containerizer.proto 
> c30b1fc659ee9b3cd343899638ced6408d8b51a2 
>   src/slave/containerizer/mesos/containerizer.cpp 
> b58baed64480e22f640a4852537f85922ed382ae 
> 
> 
> Diff: https://reviews.apache.org/r/58820/diff/3/
> 
> 
> Testing
> ---
> 
> None: not a functional change.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 58818: Ensured DEBUG container shares MESOS_SANDBOX with its parent.

[Alexander Rukletsov]

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

(Updated May 3, 2017, 4:45 p.m.)


Review request for mesos, Gastón Kleiman, Jie Yu, Kevin Klues, and Vinod Kone.


Changes
---

Rebased, NNTR.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.cpp 
b58baed64480e22f640a4852537f85922ed382ae 
  src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
29007898ec04e922266068a8519731b13d351a82 


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

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


Testing
---

`make check` on Mac OS and Fedora 24


Thanks,

Alexander Rukletsov

Re: Review Request 58819: Clarified the comment about Container.directory.

[Alexander Rukletsov]

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

(Updated May 3, 2017, 4:45 p.m.)


Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.


Changes
---

Rebased, NNTR.


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.hpp 
29a99f33e646593127b9dc126f398f7bca88e21d 


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

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


Testing
---

None: not a functional change.


Thanks,

Alexander Rukletsov

Re: Review Request 58821: Added a test that verifies a task and its check share the work dir.

[Alexander Rukletsov]

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

(Updated May 3, 2017, 4:46 p.m.)


Review request for mesos, Gastón Kleiman, Jie Yu, and Vinod Kone.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/tests/check_tests.cpp ec0d5ee78a94991c68f38174d09b41e8f2e4be35 
  src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
29007898ec04e922266068a8519731b13d351a82 


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

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


Testing
---

`make check` on Mac OS and Fedora 24


Thanks,

Alexander Rukletsov

Re: Review Request 58820: Explained how container working directory differs from its sandbox.

[Alexander Rukletsov]

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

(Updated May 3, 2017, 4:45 p.m.)


Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.


Changes
---

Addressed comments.


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  include/mesos/slave/containerizer.proto 
c30b1fc659ee9b3cd343899638ced6408d8b51a2 
  src/slave/containerizer/mesos/containerizer.cpp 
b58baed64480e22f640a4852537f85922ed382ae 


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

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


Testing
---

None: not a functional change.


Thanks,

Alexander Rukletsov

Re: Review Request 58847: Checkpointed and recovered ContainerLaunchInfo for non-orphans.

[Alexander Rukletsov]

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

(Updated May 3, 2017, 4:45 p.m.)


Review request for mesos, Gastón Kleiman, Jie Yu, and Vinod Kone.


Changes
---

Addressed comments.


Repository: mesos


Description
---

In mesos containerizer, cache each container's launch info and
recover it for non-orphan containers. This is a prerequisite for
persisting some container characteristics across agent failover.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.hpp 
29a99f33e646593127b9dc126f398f7bca88e21d 
  src/slave/containerizer/mesos/containerizer.cpp 
b58baed64480e22f640a4852537f85922ed382ae 
  src/slave/containerizer/mesos/paths.hpp 
d85fd34660faacd9c73de2ba7b87b3bbd4b6007b 
  src/slave/containerizer/mesos/paths.cpp 
ed4bbd2491e71ad1e4a41e0575b514377d02da9b 


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

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


Testing
---

See https://reviews.apache.org/r/58718/


Thanks,

Alexander Rukletsov

Re: Review Request 58817: Captured AgentID from the offer by reference in tests.

[Alexander Rukletsov]

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

(Updated May 3, 2017, 4:45 p.m.)


Review request for mesos, Gastón Kleiman and Vinod Kone.


Changes
---

Rebased, NNTR.


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/tests/check_tests.cpp ec0d5ee78a94991c68f38174d09b41e8f2e4be35 
  src/tests/slave_tests.cpp 8c97dc6d088708d301dc3ccf90d413fd785b782f 


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

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


Testing
---

make check


Thanks,

Alexander Rukletsov

Re: Review Request 58262: Inherited environment from parent when launching a DEBUG container.

[Alexander Rukletsov]

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

(Updated May 3, 2017, 4:45 p.m.)


Review request for mesos, Gastón Kleiman, Jie Yu, Kevin Klues, and Vinod Kone.


Changes
---

Moved recovery to the previous patch; addressed comments.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.cpp 
b58baed64480e22f640a4852537f85922ed382ae 
  src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
29007898ec04e922266068a8519731b13d351a82 


Diff: https://reviews.apache.org/r/58262/diff/7/

Changes: https://reviews.apache.org/r/58262/diff/6-7/


Testing
---

See https://reviews.apache.org/r/58718/


Thanks,

Alexander Rukletsov

Re: Review Request 58263: Inherited working dir from parent when launching a DEBUG container.

[Alexander Rukletsov]

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

(Updated May 3, 2017, 4:46 p.m.)


Review request for mesos, Gastón Kleiman, Jie Yu, Kevin Klues, and Vinod Kone.


Changes
---

Moved recovery out of this patch.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  include/mesos/slave/containerizer.proto 
c30b1fc659ee9b3cd343899638ced6408d8b51a2 
  src/slave/containerizer/mesos/containerizer.cpp 
b58baed64480e22f640a4852537f85922ed382ae 
  src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
29007898ec04e922266068a8519731b13d351a82 


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

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


Testing
---

See https://reviews.apache.org/r/58821/


Thanks,

Alexander Rukletsov

Re: Review Request 58718: Added a test that verifies a task's env var is seen by its check.

[Alexander Rukletsov]

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

(Updated May 3, 2017, 4:45 p.m.)


Review request for mesos, Gastón Kleiman, Jie Yu, and Vinod Kone.


Changes
---

Rebased, NNTR.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/tests/check_tests.cpp ec0d5ee78a94991c68f38174d09b41e8f2e4be35 


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

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


Testing
---

`make check` on various Linux distributions.


Thanks,

Alexander Rukletsov

Review Request 58967: Set the working directory to parent task's for DEBUG containers.

[Alexander Rukletsov]

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

Review request for mesos, Gastón Kleiman, Jie Yu, and Vinod Kone.


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


Repository: mesos


Description
---

A DEBUG container's working directory should be set to the parent
task's working directory. For the command executor case, even if
the task itself has a root filesystem, the executor container still
uses the host filesystem, hence
`ContainerLaunchInfo.working_directory`, pointing to the executor's
working directory in the host filesystem, may be different from the
task's working directory in the root filesystem.


Diffs
-

  include/mesos/slave/containerizer.proto 
c30b1fc659ee9b3cd343899638ced6408d8b51a2 
  src/slave/containerizer/mesos/containerizer.cpp 
b58baed64480e22f640a4852537f85922ed382ae 


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


Testing
---

make check on Mac OS and Fedora 24


Thanks,

Alexander Rukletsov

Review Request 58964: Added authorization support for operator endpoints.

[Alexander Rojas]

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

Review request for mesos, Adam B and Greg Mann.


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


Repository: mesos


Description
---

Adds the actions `UPDATE_MAINTENANCE_SCHEDULE`,
`GET_MAINTENANCE_SCHEDULE`, `START_MAINTENANCE`, `STOP_MAINTENANCE`
and `GET_MAINTENANCE_STATUS` to the authorizer API as well as the
necesary code to handle these new actions.


Diffs
-

  include/mesos/authorizer/acls.proto ae0b1ea2e6417d186b1606542d75f3a20e0811db 
  include/mesos/authorizer/authorizer.hpp 
4a7376fb6ca2be0a513ad54f56eea3cf8cdd024d 
  include/mesos/authorizer/authorizer.proto 
c9184d151befa4cea9bdebb36a315c760e6424b2 
  src/authorizer/local/authorizer.cpp 89aaf4b712d337d519445c922606789c334e5101 


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


Testing
---

make check

specific tests not yet written.


Thanks,

Alexander Rojas

Re: Review Request 53479: Perform agent GC asynchronously.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [53479]

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 May 2, 2017, 9:33 p.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53479/
> ---
> 
> (Updated May 2, 2017, 9:33 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6549
> https://issues.apache.org/jira/browse/MESOS-6549
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - Previously, rmdir operations were serialized
>   through the garbage collector. Expensive removal
>   events had the potential to delay task launch.
> - MESOS-6549
> 
> 
> Diffs
> -
> 
>   src/slave/gc.hpp 5ea82456cffa374203371f227b6f0ee00296553e 
>   src/slave/gc.cpp 961f547793984449ea113d9664b12b5033638c58 
> 
> 
> Diff: https://reviews.apache.org/r/53479/diff/3/
> 
> 
> Testing
> ---
> 
> `./bin/mesos-tests.sh --gtest_filter="GarbageCollector*" --gtest_repeat=100 
> --gtest_break_on_failure`
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Review Request 58949: WIP: Introduced ResourceProviderInfo proto.

[Benjamin Bannier]

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

Review request for mesos, Jie Yu and Jan Schlicht.


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


Repository: mesos


Description
---

Introduced ResourceProviderInfo proto.


Diffs
-

  include/mesos/mesos.proto 1935f47a52840f6b395ecb2d2829551fa691 
  include/mesos/type_utils.hpp 4f0a59cf7bc4172eb98db01cf73bf285c931b4d0 
  include/mesos/v1/mesos.hpp e665ce7046eb6e9c4f0e896e4f8cf9e976c40454 
  include/mesos/v1/mesos.proto c7f0bec5c96f2f41344d4261d0696f9fe0421db7 
  src/common/type_utils.cpp 6bfb558d9f7dcdf921acdf737515a25c3a0bdaa0 
  src/v1/mesos.cpp babe39e0e29ae7ff35360d9dfdabf771fdc940ea 


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


Testing
---


Thanks,

Benjamin Bannier

Review Request 58950: WIP: Made persistent volume endpoints tests independent from allocator.

[Benjamin Bannier]

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

Review request for mesos, Alexander Rukletsov and Benjamin Mahler.


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


Repository: mesos


Description
---

The tests in the case often require an agent ID. To obtain the ID they
were using a mock allocator to grab agent ID, but not other operations
with the allocator were performed.

Instead we now just capture the SlaveRegisteredMessage in order to
learn an agent's ID.


Diffs
-

  src/tests/persistent_volume_endpoints_tests.cpp 
1237d081d781948975f66a8240ae9bdb5e819a3b 


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


Testing
---

`make check`, also in repetition.

While this patch is part of this series since later patches depend on it, it 
could be commited independently.


Thanks,

Benjamin Bannier

Review Request 58951: WIP: Made reservation endpoints tests independent from allocator.

[Benjamin Bannier]

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

Review request for mesos, Alexander Rukletsov and Benjamin Mahler.


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


Repository: mesos


Description
---

The tests in the case often require an agent ID. To obtain the ID they
were using a mock allocator to grab agent ID, but not other operations
with the allocator were performed.

Instead we now just capture the SlaveRegisteredMessage in order to
learn an agent's ID.


Diffs
-

  src/tests/reservation_endpoints_tests.cpp 
cc8499a5ec05cf7b2283c075e47298918f50bd24 


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


Testing
---

`make check`, also in repetition.

While this patch is part of this series since later patches depend on it, it 
could be commited independently.


Thanks,

Benjamin Bannier

Review Request 58953: WIP: Added transitional allocator overloads.

[Benjamin Bannier]

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

Review request for mesos, Benjamin Mahler, Jie Yu, and Jan Schlicht.


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


Repository: mesos


Description
---

This commits adds allocator overloads working with resource providers
instead of agents which currently are assumed to be called with agent
resource providers. This is so that users of the allocator can
transition to the overloads taking resource providers.

In a subsequent commit the allocator overloads directly taking agent
variables will be removed in favor of the ones taking resource
providers; at the same time all methods in the allocator interface
will be made virtual again in order to call specific derived
implementation methods.


Diffs
-

  include/mesos/allocator/allocator.hpp 
dc34a1b6f0c2bdf0d653bd53394364cfa9873ca7 


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


Testing
---

`make check`


Thanks,

Benjamin Bannier

Review Request 58952: WIP: Added test helper to disambiguate allocator method call.

[Benjamin Bannier]

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

Review request for mesos, Benjamin Mahler, Jie Yu, and Jan Schlicht.


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


Repository: mesos


Description
---

In a subsequent commit we will transitionally introduce a second
'addFramework' allocator method overload where the 'used' set refers
to 'ResourceProviderID's instead of 'SlaveID's. Introduce a test
helper here to remove a future ambiguity when an empty set '{}' is
passed. We introduce this in a separate patch so that this change can
more easily be removed one the old allocator method is removed and the
ambiguity vanishes.


Diffs
-

  src/tests/hierarchical_allocator_tests.cpp 
ebc4868a6b7e2a04cc202a74a4633969ade40aca 


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


Testing
---

`make check`


Thanks,

Benjamin Bannier

Re: Review Request 58942: CHANGELOG for 1.3.0 release.

[Stephan Erb]

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




CHANGELOG
Lines 117 (patched)


I was once confused by this "All issues" title in presence of the 
"Unresolved Critical Issues". 

Maybe change it to "Newly resolved Issues" or something similar to make it 
clearer?


- Stephan Erb


On May 3, 2017, 1:48 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58942/
> ---
> 
> (Updated May 3, 2017, 1:48 a.m.)
> 
> 
> Review request for mesos and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> CHANGELOG for 1.3.0 release.
> 
> 
> Diffs
> -
> 
>   CHANGELOG 998f30d576d87e72b1d52e8bd1e43ce6ba67a54b 
> 
> 
> Diff: https://reviews.apache.org/r/58942/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 58224: Optionally verify the source IP address for libprocess messages.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [58928, 58224]

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 May 2, 2017, 9:02 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58224/
> ---
> 
> (Updated May 2, 2017, 9:02 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7401
> https://issues.apache.org/jira/browse/MESOS-7401
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In general, libprocess is unable to validate that a peer
> is a legitimate owner of the UPID it claims in a libprocess
> message. This change adds a check that the IP address in the
> UPID matches the peer address. This makes spoofing the UPID
> harder (eg. to send authenticated messages), but also breaks
> some legitimate configurations, particularly on multihomed
> hosts.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> f5b666f894215cb1861c244c94b382e0739bc5c9 
> 
> 
> Diff: https://reviews.apache.org/r/58224/diff/4/
> 
> 
> Testing
> ---
> 
> make check (Fedora 25). Light manual testing.
> 
> With LIBPROCESS_pin_peer_address=true, all Mesos tests pass except 
> ``ExamplesTest.DiskFullFramework``, however enabling this will definitely 
> break some libprocess APIs (though not in the way that Mesos uses them) and 
> legitimate multi-homed configurations. Note that setting 
> LIBPROCESS_ip=127.0.0.1 makes you multihomed for this purpose, which is why 
> ``ExamplesTest.DiskFullFramework`` breaks.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 58872: Ensured sandbox URI request reroute after fetched `$scope.state`.

[Tomasz Janiszewski]

> On Maj 1, 2017, 8:45 po południu, Tomasz Janiszewski wrote:
> > src/webui/master/static/js/controllers.js
> > Lines 1024 (patched)
> > 
> >
> > It's not obious why we remove update listener on route change start. 
> > Can you add a comment with clarification?
> 
> haosdent huang wrote:
> It is because we no need to call this again if route change, so we remove 
> it here. We do it in almost places which depends on `$scope.state`.
> 
> ```
> var removeListener = $scope.$on('state_updated', update);
> $scope.$on('$routeChangeStart', removeListener);
> ```
> 
> Do you think we need to add comment about this here?

No that's fine. Just spoted it's a template.


- Tomasz


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


On Maj 3, 2017, 4:15 rano, haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58872/
> ---
> 
> (Updated Maj 3, 2017, 4:15 rano)
> 
> 
> Review request for mesos, Benjamin Mahler and Tomasz Janiszewski.
> 
> 
> Bugs: MESOS-4992
> https://issues.apache.org/jira/browse/MESOS-4992
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Open sandbox link in a new tab would fail since it depends on agents
> information loading finish. This patch fixes it by ensuring the agents
> information loaded first and then rerouting the sandbox request.
> 
> 
> Diffs
> -
> 
>   src/webui/master/static/js/controllers.js 
> a021962573d452de1581e6a7717016eac7d0cd85 
> 
> 
> Diff: https://reviews.apache.org/r/58872/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 58874: Stripped spaces between directory elements in WebUI.

[Tomasz Janiszewski]

> On Maj 1, 2017, 8:14 po południu, Tomasz Janiszewski wrote:
> > src/webui/master/static/browse.html
> > Line 17 (original), 17 (patched)
> > 
> >
> > Could you keep original formatting?
> 
> haosdent huang wrote:
> Change to
> 
> ```
>  href="#/agents/{{agent_id}}/browse?path={{
> encodeURIComponent(path.split('/').slice(0, $index + 
> 1).join('/'))
> }}">{{dir}}/
> ```
> 
> Is it match you expection here?

That's OK. We must keep everything in one line to prevent newlines and spaces 
to appear.


> On Maj 1, 2017, 8:14 po południu, Tomasz Janiszewski wrote:
> > src/webui/master/static/css/mesos.css
> > Lines 180 (patched)
> > 
> >
> > I think this should be replaced with `visibility: hidden;`. With 
> > `font-size: 0;` there is different (smaller) padding then before the change.
> 
> haosdent huang wrote:
> Oh, I didn't use it because with `visibility: hidden`, the copy content 
> would not include `` in my browser. Is it work in your side?

My mistake, I'm sorry.


- Tomasz


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


On Maj 3, 2017, 4:29 rano, haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58874/
> ---
> 
> (Updated Maj 3, 2017, 4:29 rano)
> 
> 
> Review request for mesos, Benjamin Mahler and Tomasz Janiszewski.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stripped spaces between directory elements in WebUI.
> 
> 
> Diffs
> -
> 
>   src/webui/master/static/browse.html 
> b9849197227b06df348789a49348e2b5d4cfd2ae 
>   src/webui/master/static/css/mesos.css 
> 9f3de5427071fc61d3791c4bc2a660368c2cd3c2 
> 
> 
> Diff: https://reviews.apache.org/r/58874/diff/2/
> 
> 
> Testing
> ---
> 
> 
> File Attachments
> 
> 
> strip_space.gif
>   
> https://reviews.apache.org/media/uploaded/files/2017/05/03/79afd0b1-5fb1-437c-a91c-732009af8fe3__strip_space.gif
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Review Request 58955: Adds authorization for the master's v1 API call SET_LOGGING_LEVEL.

[Alexander Rojas]

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

Review request for mesos, Adam B and Greg Mann.


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


Repository: mesos


Description
---

Enables authorization in the v1 API call `SET_LOGGING_LEVEL` following
the existing implementation for the agent call with the same name.
Likewise it reuses the authorizer action `SET_LOG_LEVEL`.


Diffs
-

  src/master/http.cpp 5aae52870451d883ef1ea1fda5a27764d7f318e8 
  src/tests/api_tests.cpp faf3242f9c86d866c4bb5e457fcfe47c1063cc09 


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


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 57964: Added a test to verify metrics when shared resources are present.

[Jiang Yan Xu]

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


Ship it!




Committing with minor edit.


src/tests/persistent_volume_tests.cpp
Lines 1192 (patched)


s/populated/correctly populated/


- Jiang Yan Xu


On May 2, 2017, 10:54 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57964/
> ---
> 
> (Updated May 2, 2017, 10:54 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7186
> https://issues.apache.org/jira/browse/MESOS-7186
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test to verify metrics when shared resources are present.
> 
> 
> Diffs
> -
> 
>   src/tests/persistent_volume_tests.cpp 
> d42fd18fd5b36f5970e91e60b84e839aeedfc34b 
> 
> 
> Diff: https://reviews.apache.org/r/57964/diff/4/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 57963: Metrics for used resources should incorporate shared resources.

[Jiang Yan Xu]

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


Ship it!




Committing with minor edits.


src/master/master.cpp
Line 8901 (original), 9083 (patched)


s/executors/frameworks/ 

This map is keyed by frameworks.



src/master/master.cpp
Line 8950 (original), 9132 (patched)


Ditto.


- Jiang Yan Xu


On May 2, 2017, 10:53 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57963/
> ---
> 
> (Updated May 2, 2017, 10:53 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7186
> https://issues.apache.org/jira/browse/MESOS-7186
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The following metrics take the shared counts of shared resources into
> account in determination of the actual amount of used resources:
> a) `master/_used`
> b) `master/_revocable_used`
> c) `slave/_used`
> d) `slave/_revocable_used`
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 31a7a2fcf905c0c35e80692a69c290d4094deded 
>   src/slave/slave.cpp 8b8078dbb656e9db2efa53cc4ec5bed2cc01d49a 
> 
> 
> Diff: https://reviews.apache.org/r/57963/diff/4/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 58587: Clarified comments about resource operations through operator API.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [58587]

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 May 1, 2017, 9:29 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58587/
> ---
> 
> (Updated May 1, 2017, 9:29 p.m.)
> 
> 
> Review request for mesos, Anindya Sinha and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Seems like the comments about "virtually always win the race against
> 'allocate'" could be made more precise in helping people understand
> how it works.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 5aae52870451d883ef1ea1fda5a27764d7f318e8 
> 
> 
> Diff: https://reviews.apache.org/r/58587/diff/1/
> 
> 
> Testing
> ---
> 
> N/A
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 58942: CHANGELOG for 1.3.0 release.

[Alexander Rukletsov]

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




CHANGELOG
Lines 115-116 (patched)


Please add
```
  * [MESOS-6906] - Introduce a general non-interpreting task check.
```


- Alexander Rukletsov


On May 2, 2017, 11:48 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58942/
> ---
> 
> (Updated May 2, 2017, 11:48 p.m.)
> 
> 
> Review request for mesos and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> CHANGELOG for 1.3.0 release.
> 
> 
> Diffs
> -
> 
>   CHANGELOG 998f30d576d87e72b1d52e8bd1e43ce6ba67a54b 
> 
> 
> Diff: https://reviews.apache.org/r/58942/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>