Review Request 60724: Fixed initialization of `LIBPROCESS_IP6`.

[Avinash sridharan]

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

Review request for mesos, Benjamin Bannier and Benjamin Hindman.


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


Repository: mesos


Description
---

Fixed initialization of `LIBPROCESS_IP6`.


Diffs
-

  src/slave/main.cpp 358a4394d27d2d123c9cdc9ed3e5295ecbaf9130 


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


Testing
---

make check


Thanks,

Avinash sridharan

Re: Review Request 59525: Enabled filtering of reservations.

[Greg Mann]

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




src/master/http.cpp
Line 380 (original), 380-381 (patched)


To avoid shadowing in constructors, we use leading underscores rather than 
trailing.



src/master/http.cpp
Line 2320 (original), 2330 (patched)


Now that this is a Writer struct, perhaps it should be moved to the top of 
the file along with the others?



src/master/http.cpp
Lines 2325-2327 (original), 2333-2336 (patched)


Ditto here regarding leading underscores.



src/master/http.cpp
Lines 2374-2380 (original), 2402-2410 (patched)


For the used resources, a role can be present in both `resource.role()` and 
in `resource.allocation_info().role()`. Should we also be filtering based on 
the allocated role?



src/master/http.cpp
Lines 2384-2391 (original), 2414-2421 (patched)


Why aren't we filtering this field as well?



src/master/http.cpp
Lines 3186-3189 (original), 3255-3258 (patched)


Could you fix the indentation of this lambda? The body should be two spaces 
indented with respect to the line with the opening curly brace `{`. Or is the 
indentation unmanageable if we do that here?


- Greg Mann


On July 7, 2017, 3:05 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59525/
> ---
> 
> (Updated July 7, 2017, 3:05 p.m.)
> 
> 
> Review request for mesos, Adam B, Greg Mann, and Till Toenshoff.
> 
> 
> Bugs: MESOS-7416
> https://issues.apache.org/jira/browse/MESOS-7416
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds support of the 'VIEW_ROLE' ACL to the results generated by the
> '/slaves' as well as the 'GET_AGENTS' API v1 call, '/states' and
> '/states-summary'.
> 
> This means that calls to these endpoints and API call which uses
> reservations will only show the reserved resources the user making the
> requests is allowed to see based on the reservations roles.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 175a44ce7fb5be509453c25eaa9ec29f35adba3a 
>   src/master/master.hpp 95c2d0fab32d6b60f29a86037607ff009bd78717 
>   src/tests/master_tests.cpp c778c6c56d47c4033189912cebee6024be79106f 
> 
> 
> Diff: https://reviews.apache.org/r/59525/diff/5/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 56895: Added tests to ensure slave recovery post reboot.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [60103, 60104, 60105, 56895]

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 June 30, 2017, 8:18 a.m., Megha Sharma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56895/
> ---
> 
> (Updated June 30, 2017, 8:18 a.m.)
> 
> 
> Review request for mesos, Neil Conway, Vinod Kone, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6223
> https://issues.apache.org/jira/browse/MESOS-6223
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added tests to verify that the state is recovered post reboot and
> agentId is retained given the recovery finishes without errors and
> if the recovery fails due to slaveInfo mismatch then agent no
> state is recoverd making the agent register as a new agent.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_recovery_tests.cpp f6eafcbe3d89c7a69c03db0fd7bc10ae73d06584 
> 
> 
> Diff: https://reviews.apache.org/r/56895/diff/13/
> 
> 
> Testing
> ---
> 
> make check done together with 60104 and 60103
> 
> 
> Thanks,
> 
> Megha Sharma
> 
>

Re: Review Request 60638: Changed semantics of allocator 'updateSlave' method.

[Benjamin Bannier]

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

(Updated July 8, 2017, 2:31 a.m.)


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


Changes
---

Fixed rebase mistake.


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


Repository: mesos


Description
---

We change the semantics of the 'updateSlave' method present in the
allocator interface. While previously the passed optional resource
argument was interpreted as the amount of (new) oversubscribed
resources, it now represents the new amount of total resources on the
given agent.

We addtionally add an optimization of
'HierarchicalAllocatorProcess::updateSlaveTotal' for cases where the
passed total is identical to the current total. This operation is a
no-op now and we prevent updating the sorters.


Diffs (updated)
-

  include/mesos/allocator/allocator.hpp 
bec9e0b8ef8d51ec22f9e8af69407ed5ecaa3e8f 
  src/master/allocator/mesos/allocator.hpp 
2e780c92d5c5132abff32f1ce051c3bab2947f37 
  src/master/allocator/mesos/hierarchical.hpp 
5c58cf401de9205e54300e0ce8433995cdf5cb7a 
  src/master/allocator/mesos/hierarchical.cpp 
eb01d8e6b1108866ebc049f9f4a46157823a3541 
  src/master/master.cpp 56b170ed70722eba30d98f6e648b6a31580d6b56 
  src/tests/hierarchical_allocator_tests.cpp 
2a312a9af4bae679a0a4e7bf45a3c013513c5da2 


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

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


Testing
---

Tested with https://reviews.apache.org/r/60639/.


Thanks,

Benjamin Bannier

Re: Review Request 60641: Allowed to pass total resources in 'UpdateSlaveMessage'.

[Benjamin Bannier]

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

(Updated July 8, 2017, 2:31 a.m.)


Review request for mesos, Jie Yu and Jan Schlicht.


Changes
---

Fixed handling of optional proto field.

Previously we implicitly converted not set type fields to UNKNOWN.
Since the field is optional we can actually distinguish between an
unset and an UNKNOWN field. We now treat messages with unset type
as OVERSUBSCRIBED, but reject type UNKNOWN since it is an
indication of an incompatible caller.


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


Repository: mesos


Description
---

This commit both extends the existing 'UpdateSlaveMessage' proto
message, and adjusts its handling on the agent and master side.

To distinguish updates to 'oversubscribed_resources' from updates to
'total_resources' the message now contains a 'type' field to allow
disambiguation among an empty list of resources and an unset list of
resources. For backwards-compatibility we assume whenever the type
field was not set that caller intended to use the
'oversubscribed_resources' field as opposed to the 'total_resources'
field.

Currently, passing 'total_resources' is handled neither in the master
nor the default allocator; we will implement this in a subsequent
patch.


Diffs (updated)
-

  src/master/master.hpp 95c2d0fab32d6b60f29a86037607ff009bd78717 
  src/master/master.cpp 56b170ed70722eba30d98f6e648b6a31580d6b56 
  src/messages/messages.proto 67cee3146a45f8f720b4e60739cf85085d18259b 
  src/slave/slave.cpp 52f673633fa0041259ae40f88d4c0281a1e053b0 
  src/tests/oversubscription_tests.cpp 2266510091b49c61310a363ab6644708655787dc 


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

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


Testing
---


Thanks,

Benjamin Bannier

Review Request 60723: Made intialization for `__address__` consistent.

[Avinash sridharan]

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

Review request for mesos and Jiang Yan Xu.


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


Repository: mesos


Description
---

Made intialization for `__address__` consistent.


Diffs
-

  3rdparty/libprocess/src/process.cpp 97f737b0b7be23df32f2863d4d6891ec2518529e 


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


Testing
---

sudo ./build/bin/mesos-agent.sh  --ip=10.0.2.15 --port=5059 
--master=10.0.2.15:5050 --work_dir=/tmp/mesos
tcp0  0 10.0.2.15:5059  0.0.0.0:*   LISTEN

sudo ./build/bin/mesos-agent.sh  --port=5059 --master=10.0.2.15:5050 
--work_dir=/tmp/mesos
tcp0  0 0.0.0.0:50590.0.0.0:*   LISTEN

sudo ./build/bin/mesos-agent.sh  --master=10.0.2.15:5050 --work_dir=/tmp/mesos
tcp0  0 0.0.0.0:50510.0.0.0:*   LISTEN

sudo ./build/bin/mesos-agent.sh  --ip=10.0.2.15 --master=10.0.2.15:5050 
--work_dir=/tmp/meso
tcp0  0 10.0.2.15:5051  0.0.0.0:*   LISTEN


Thanks,

Avinash sridharan

Re: Review Request 60720: Fixed initialization of `__address__` in the abscense of `--ip` flag.

[Avinash sridharan]

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

(Updated July 8, 2017, 12:08 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Added a description about the tests as well.


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


Repository: mesos


Description
---

When we introduced the `__address6__ optional IPv6 storage into
`libprocess` we also introduced a regression because of which the `port`
doesn't get initialized to whatever is specified in the `--port` flag
until the `--ip` flag is specified.

This change fixes the initialization of the `__address__` in the
absence of the `--ip` flag.


Diffs
-

  3rdparty/libprocess/src/process.cpp 264298c16199ded4ccbf948f6893f4209c23aed0 


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


Testing (updated)
---

sudo ./build/bin/mesos-agent.sh  --port=5059 --master=10.0.2.15:5050 
--work_dir=/tmp/mesos
tcp0  0 0.0.0.0:50590.0.0.0:*   LISTEN

sudo ./build/bin/mesos-agent.sh  --ip=10.0.2.15 --port=5059 
--master=10.0.2.15:5050 --work_dir=/tmp/mesos
tcp0  0 10.0.2.15:5059  0.0.0.0:*   LISTEN


Thanks,

Avinash sridharan

Re: Review Request 60720: Fixed initialization of `__address__` in the abscense of `--ip` flag.

[Avinash sridharan]

> On July 7, 2017, 11:18 p.m., Jiang Yan Xu wrote:
> > 3rdparty/libprocess/src/process.cpp
> > Line 1191 (original), 1192 (patched)
> > 
> >
> > Should `advertise_ip` be treated the same way as `ip`? i.e., flag 
> > validation and choice between reassignment and modification.

Once we move to the pattern of doing `__address__.ip = 
libprocess_flags->ip.get()` I think handling of `__address__` for `--ip` and 
`--advertise_ip` will be the same.


- Avinash


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


On July 7, 2017, 10:04 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60720/
> ---
> 
> (Updated July 7, 2017, 10:04 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7769
> https://issues.apache.org/jira/browse/MESOS-7769
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When we introduced the `__address6__ optional IPv6 storage into
> `libprocess` we also introduced a regression because of which the `port`
> doesn't get initialized to whatever is specified in the `--port` flag
> until the `--ip` flag is specified.
> 
> This change fixes the initialization of the `__address__` in the
> absence of the `--ip` flag.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> 264298c16199ded4ccbf948f6893f4209c23aed0 
> 
> 
> Diff: https://reviews.apache.org/r/60720/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 60525: Fixed the default filter used by the allocator.

[Gastón Kleiman]

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

(Updated July 8, 2017, 12:03 a.m.)


Review request for mesos, Alexander Rukletsov, Benjamin Mahler, Greg Mann, and 
Vinod Kone.


Changes
---

Addressed feedback.


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


Repository: mesos


Description
---

If a framework accepts/refuses an offer using a very long filter, the
`HierarchicalAllocator` will use the default filter instead. Meaning
that it will filter the resources for only 5 seconds. This can happen
when a framework sets `Filter::refuse_seconds` to a number of seconds
larger than what fits in `Duration`.

This patch makes the allocator use the largest possible filter duration
in this case.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.cpp 
eb01d8e6b1108866ebc049f9f4a46157823a3541 


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

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


Testing
---

`make check`


Thanks,

Gastón Kleiman

Re: Review Request 60720: Fixed initialization of `__address__` in the abscense of `--ip` flag.

[Avinash sridharan]

> On July 7, 2017, 11:18 p.m., Jiang Yan Xu wrote:
> > Could you fill out the 'testing done' section? I know there will be a 
> > follow up test patch but for this at least `make check` and manual 
> > verification?
> 
> Jiang Yan Xu wrote:
> I committed it but could you respond to the comments afterwards and 
> follow up on them in another review (if needed)?

Thanks Yan !! Will follow it up with a patch.


> On July 7, 2017, 11:18 p.m., Jiang Yan Xu wrote:
> > 3rdparty/libprocess/src/process.cpp
> > Line 1135 (original), 1135 (patched)
> > 
> >
> > You are (re)initalizing/resetting this `__address__` but not 
> > `__address6__` (to None). Should they be consistent?
> > 
> > Given it's initialized at definition and reset during `finailize` 
> > perhaps it doesn't need to be done here?
> > 
> > If it needs to be done, can it be moved down closer to where the 
> > modifications/reassignments are done?

`__address6__` is optional hence we don't initialize it (in fact its initalized 
to `None()`)


> On July 7, 2017, 11:18 p.m., Jiang Yan Xu wrote:
> > 3rdparty/libprocess/src/process.cpp
> > Lines 1151-1157 (original), 1151-1158 (patched)
> > 
> >
> > Why do you need to modify the current instance of `__address__` in the 
> > case above but but have to create a new instance of `inet::Address` in the 
> > case below?
> > 
> > Can they be made consistent?

yeah I can just set `__address__.ip = libprocess_flags->ip.get()`


- Avinash


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


On July 7, 2017, 10:04 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60720/
> ---
> 
> (Updated July 7, 2017, 10:04 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7769
> https://issues.apache.org/jira/browse/MESOS-7769
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When we introduced the `__address6__ optional IPv6 storage into
> `libprocess` we also introduced a regression because of which the `port`
> doesn't get initialized to whatever is specified in the `--port` flag
> until the `--ip` flag is specified.
> 
> This change fixes the initialization of the `__address__` in the
> absence of the `--ip` flag.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> 264298c16199ded4ccbf948f6893f4209c23aed0 
> 
> 
> Diff: https://reviews.apache.org/r/60720/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Review Request 60721: Stout: Made `Duration::parse()` handle durations out of range.

[Gastón Kleiman]

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

Review request for mesos, Alexander Rukletsov, Benjamin Mahler, Greg Mann, and 
Vinod Kone.


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


Repository: mesos


Description
---

Made `Duration:parse()` return an error if the argument is out of the
range that a `Duration` can represent.


Diffs
-

  3rdparty/stout/include/stout/duration.hpp 
b0cd77b833f6fbf752b4db820fd43b87e1d1e476 
  3rdparty/stout/tests/duration_tests.cpp 
59b08f14849a8db31f11fbd0b2e1248c99afd9dd 


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


Testing
---

Added a new expectation to an existing test and confirmed that tests still pass.


Thanks,

Gastón Kleiman

Re: Review Request 60524: Stout: Made the `Duration` operators handle int overflows explicitly.

[Gastón Kleiman]

> On July 7, 2017, 8:36 p.m., Greg Mann wrote:
> > 3rdparty/stout/include/stout/duration.hpp
> > Line 35 (original), 35 (patched)
> > 
> >
> > Consider updating this to return an error in case of over/underflow, 
> > like `create()`.

Done in https://reviews.apache.org/r/60721/


> On July 7, 2017, 8:36 p.m., Greg Mann wrote:
> > 3rdparty/stout/include/stout/duration.hpp
> > Lines 121 (patched)
> > 
> >
> > two slashes only

:blush:


> On July 7, 2017, 8:36 p.m., Greg Mann wrote:
> > 3rdparty/stout/include/stout/duration.hpp
> > Lines 149-150 (patched)
> > 
> >
> > Investigate if this is easy to do now.

After some research on floating point operations, I learned about the existence 
of negative and positive infinity values.

If a double operation overflows, the result is a value that is bigger (or 
smaller if the number is negative) than any double.

I removed these TODOs and added extra expectations to the tests.


- Gastón


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


On July 7, 2017, 11:59 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60524/
> ---
> 
> (Updated July 7, 2017, 11:59 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Mahler, Greg Mann, 
> and Vinod Kone.
> 
> 
> Bugs: MESOS-7661
> https://issues.apache.org/jira/browse/MESOS-7661
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made the `Duration` arithmetic operators return `Duration::max()` if the
> operation would result in an integer overflow, and `Duration::min()` if
> it would result in an underflow.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/duration.hpp 
> b0cd77b833f6fbf752b4db820fd43b87e1d1e476 
>   3rdparty/stout/tests/duration_tests.cpp 
> 59b08f14849a8db31f11fbd0b2e1248c99afd9dd 
> 
> 
> Diff: https://reviews.apache.org/r/60524/diff/2/
> 
> 
> Testing
> ---
> 
> Added new Stout tests and confirmed that the Mesos test suite still passes.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 60524: Stout: Made the `Duration` operators handle int overflows explicitly.

[Gastón Kleiman]

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

(Updated July 7, 2017, 11:59 p.m.)


Review request for mesos, Alexander Rukletsov, Benjamin Mahler, Greg Mann, and 
Vinod Kone.


Changes
---

Addressed feedback.


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


Repository: mesos


Description
---

Made the `Duration` arithmetic operators return `Duration::max()` if the
operation would result in an integer overflow, and `Duration::min()` if
it would result in an underflow.


Diffs (updated)
-

  3rdparty/stout/include/stout/duration.hpp 
b0cd77b833f6fbf752b4db820fd43b87e1d1e476 
  3rdparty/stout/tests/duration_tests.cpp 
59b08f14849a8db31f11fbd0b2e1248c99afd9dd 


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

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


Testing
---

Added new Stout tests and confirmed that the Mesos test suite still passes.


Thanks,

Gastón Kleiman

Re: Review Request 60712: Added filtering support for '/slave/containers' endpoint.

[Quinn Leng]

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

(Updated July 7, 2017, 11:47 p.m.)


Review request for mesos, Anand Mazumdar, Alexander Rojas, Greg Mann, and Vinod 
Kone.


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


Repository: mesos


Description
---

Added filtering support for '/slave/containers' endpoint.

Add query parameter support for '/slave/containers' endpoint.
To get the information about a specific container, you can specify
the container_id. The request would look like
'/slave/containers?container_id=[container id]'.

When no container id is specified, it will return the information
about all containers.


Diffs (updated)
-

  src/common/http.hpp b7e4a8adcbcaa3a962af795c67694a35161b6c1a 
  src/common/http.cpp fdb591eccf273260902f3f695cf431f72ee3d817 
  src/slave/http.hpp ad412d4e2fde13a7afa3563c611ef50b11829cac 
  src/slave/http.cpp 700871e1502a65b0bb1fc31219e09219dbdb5340 
  src/tests/slave_tests.cpp 8a69cc2ede0b2f17a31986e9142aa2081691eb5e 


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

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


Testing
---

Passed 'make check -j48'
Passed 'GLOG_v=1 ./bin/mesos-test.sh 
--gtest_filter=SlaveTest.ContainersEndpoint --gtest_repeat=1000 
--gtest_break_on_failure'


Thanks,

Quinn Leng

Re: Review Request 60525: Fixed the default filter used by the allocator.

[Gastón Kleiman]

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

(Updated July 7, 2017, 11:47 p.m.)


Review request for mesos, Alexander Rukletsov, Benjamin Mahler, Greg Mann, and 
Vinod Kone.


Changes
---

Addressed feedback.


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


Repository: mesos


Description
---

If a framework accepts/refuses an offer using a very long filter, the
`HierarchicalAllocator` will use the default filter instead. Meaning
that it will filter the resources for only 5 seconds. This can happen
when a framework sets `Filter::refuse_seconds` to a number of seconds
larger than what fits in `Duration`.

This patch makes the allocator use the largest possible filter duration
in this case.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.cpp 
eb01d8e6b1108866ebc049f9f4a46157823a3541 


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

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


Testing
---

`make check`


Thanks,

Gastón Kleiman

Re: Review Request 60580: Added filtering to the '/frameworks' endpoint.

[Quinn Leng]

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

(Updated July 7, 2017, 11:20 p.m.)


Review request for mesos, Anand Mazumdar, Alexander Rojas, Greg Mann, and Vinod 
Kone.


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


Repository: mesos


Description
---

Added filtering to the '/frameworks' endpoint.

Add query parameter support for '/frameworks' endpoint.
To query the information about a specific framework, we
can use 'framework_id' to specify the framework, thus it
looks like'/frameworks?framework_id=[framework id]'

To maintain consistency, the structure for response is
reserved, thus the response still contains field 'frameworks'
and 'completed_frameworks' etc..

Since code for '/state' endpoint and '/frameworks' are coupled
(they share 'FullFrameworkWriter' class), thus the code for '/state'
endpoint is also updated.


Diffs (updated)
-

  src/master/http.cpp 175a44ce7fb5be509453c25eaa9ec29f35adba3a 
  src/tests/master_tests.cpp c778c6c56d47c4033189912cebee6024be79106f 


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

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


Testing
---

Passed 'make check -j48'
Passed 'GTEST_FILTER="MasterTest.FrameworksEndpointQueryFramework" make check 
-j48'
Passed 'GLOG_v=1 ./bin/mesos-tests.sh 
--gtest_filter="MasterTest.FrameworksEndpointQueryFramework" 
--gtest_repeat=1000 --gtest_break_on_failure'


Thanks,

Quinn Leng

Re: Review Request 60720: Fixed initialization of `__address__` in the abscense of `--ip` flag.

[Jiang Yan Xu]

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


Fix it, then Ship it!




Could you fill out the 'testing done' section? I know there will be a follow up 
test patch but for this at least `make check` and manual verification?


3rdparty/libprocess/src/process.cpp
Line 1135 (original), 1135 (patched)


You are (re)initalizing/resetting this `__address__` but not `__address6__` 
(to None). Should they be consistent?

Given it's initialized at definition and reset during `finailize` perhaps 
it doesn't need to be done here?

If it needs to be done, can it be moved down closer to where the 
modifications/reassignments are done?



3rdparty/libprocess/src/process.cpp
Lines 1151-1157 (original), 1151-1158 (patched)


Why do you need to modify the current instance of `__address__` in the case 
above but but have to create a new instance of `inet::Address` in the case 
below?

Can they be made consistent?



3rdparty/libprocess/src/process.cpp
Line 1191 (original), 1192 (patched)


Should `advertise_ip` be treated the same way as `ip`? i.e., flag 
validation and choice between reassignment and modification.


- Jiang Yan Xu


On July 7, 2017, 3:04 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60720/
> ---
> 
> (Updated July 7, 2017, 3:04 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7769
> https://issues.apache.org/jira/browse/MESOS-7769
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When we introduced the `__address6__ optional IPv6 storage into
> `libprocess` we also introduced a regression because of which the `port`
> doesn't get initialized to whatever is specified in the `--port` flag
> until the `--ip` flag is specified.
> 
> This change fixes the initialization of the `__address__` in the
> absence of the `--ip` flag.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> 264298c16199ded4ccbf948f6893f4209c23aed0 
> 
> 
> Diff: https://reviews.apache.org/r/60720/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 60581: Added filtering to the '/slaves' endpoint.

[Quinn Leng]

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

(Updated July 7, 2017, 10:36 p.m.)


Review request for mesos, Anand Mazumdar, Alexander Rojas, Greg Mann, and Vinod 
Kone.


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


Repository: mesos


Description
---

Added filtering to the '/slaves' endpoint.

Added query parameter support for 'slaves'
endpoint. We can use 'slave_id' to specify
which slave information to query. Thus it
looks like '/slaves?slave_id=[slave id]'

If not slave id is specified, it will return
information of all slaves, just like before

The structure for response is the same for both
specifying or not specifying slave id. Thus even
for request with slave id specified, the response
will still contain both fields 'slaves', 
'recovered_slaves' etc..


Diffs (updated)
-

  src/common/http.hpp b7e4a8adcbcaa3a962af795c67694a35161b6c1a 
  src/common/http.cpp fdb591eccf273260902f3f695cf431f72ee3d817 
  src/master/http.cpp 175a44ce7fb5be509453c25eaa9ec29f35adba3a 
  src/tests/master_tests.cpp c778c6c56d47c4033189912cebee6024be79106f 


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

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


Testing
---

Passed 'make check -j48'
Passed 'GTEST_FILTER="MasterTest.SlavesEndpointQuerySlave" make check -j48'
Passed 'GLOG_v=1 ./bin/mesos-tests.sh 
--gtest_filter="MasterTest.SlavesEndpointQuerySlave" --gtest_repeat=1000 
--gtest_break_on_failure'


Thanks,

Quinn Leng

Re: Review Request 60716: Cleaned up authentication acceptor to have one class.

[Quinn Leng]

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

(Updated July 7, 2017, 10:25 p.m.)


Review request for mesos, Anand Mazumdar, Alexander Rojas, Greg Mann, and Vinod 
Kone.


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


Repository: mesos


Description (updated)
---

Cleaned up authentication acceptor to have one class.

Replace different Authorization related Acceptor classes with one
AuthorizationAcceptor class.

Single static create function for AuthorizationAcceptor, single
acceptor function accepting ObjectApprover::Object.

Templated 'accept' function to take different number and types of
parameters.

Removed ObjectAcceptor parent class, since no inheritance feature
provided by it.


Diffs (updated)
-

  src/common/http.hpp b7e4a8adcbcaa3a962af795c67694a35161b6c1a 
  src/common/http.cpp fdb591eccf273260902f3f695cf431f72ee3d817 
  src/master/http.cpp 175a44ce7fb5be509453c25eaa9ec29f35adba3a 


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

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


Testing
---

Passed "make check -j48"
Passed 'GTEST_FILTER="MasterTest.TasksEndpoint" make check -j48'


Thanks,

Quinn Leng

Review Request 60720: Fixed initialization of `__address__` in the abscense of `--ip` flag.

[Avinash sridharan]

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

Review request for mesos and Jiang Yan Xu.


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


Repository: mesos


Description
---

When we introduced the `__address6__ optional IPv6 storage into
`libprocess` we also introduced a regression because of which the `port`
doesn't get initialized to whatever is specified in the `--port` flag
until the `--ip` flag is specified.

This change fixes the initialization of the `__address__` in the
absence of the `--ip` flag.


Diffs
-

  3rdparty/libprocess/src/process.cpp 264298c16199ded4ccbf948f6893f4209c23aed0 


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


Testing
---


Thanks,

Avinash sridharan

Re: Review Request 60713: Modified the access specifier for `net::IP` and `net::IP::Network`.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On July 7, 2017, 6:13 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60713/
> ---
> 
> (Updated July 7, 2017, 6:13 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-7767
> https://issues.apache.org/jira/browse/MESOS-7767
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Modified the access specifier for `net::IP` and `net::IP::Network`.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/ip.hpp 
> 6c2f323a4ccb140c54f00236fb50c21180a84ebb 
> 
> 
> Diff: https://reviews.apache.org/r/60713/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 60719: Add test infrastructure for src/python/lib/mesos.

[Eric Chung]

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

(Updated July 7, 2017, 9:44 p.m.)


Review request for mesos, Armand Grillet, Jason Lai, and Kevin Klues.


Changes
---

Add src/python/lib to mesos-style.py; fix lint errors.


Repository: mesos


Description
---

Part of MESOS-7310, this patch adds the test infrastructure necessary
for reliably running unit tests for the mesos package located under
src/python/lib.

setup.py is added under src/python/lib to both define the Python package
and to allow tests to be run via `python setup.py test`, which delegates
tests to pytest.


Diffs (updated)
-

  src/python/lib/mesos/__init__.py e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 
  src/python/lib/requirements-test.in PRE-CREATION 
  src/python/lib/requirements.in PRE-CREATION 
  src/python/lib/setup.cfg PRE-CREATION 
  src/python/lib/setup.py PRE-CREATION 
  support/mesos-style.py 48d816f72df65d77364769f812fc06afcd6b5aec 


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

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


Testing
---

1. under src/python/lib, run `virtualenv env`
2. `. env/bin/activate`
3. `pip install setuptools --upgrade`
4. `python setup.py test`


Thanks,

Eric Chung

Re: Review Request 60719: Add test infrastructure for src/python/lib/mesos.

[Eric Chung]

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

(Updated July 7, 2017, 9:32 p.m.)


Review request for mesos, Armand Grillet, Jason Lai, and Kevin Klues.


Changes
---

add .dev suffix to __version__


Repository: mesos


Description
---

Part of MESOS-7310, this patch adds the test infrastructure necessary
for reliably running unit tests for the mesos package located under
src/python/lib.

setup.py is added under src/python/lib to both define the Python package
and to allow tests to be run via `python setup.py test`, which delegates
tests to pytest.


Diffs (updated)
-

  src/python/lib/mesos/__init__.py e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 
  src/python/lib/requirements-test.in PRE-CREATION 
  src/python/lib/requirements.in PRE-CREATION 
  src/python/lib/setup.cfg PRE-CREATION 
  src/python/lib/setup.py PRE-CREATION 


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

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


Testing
---

1. under src/python/lib, run `virtualenv env`
2. `. env/bin/activate`
3. `pip install setuptools --upgrade`
4. `python setup.py test`


Thanks,

Eric Chung

Re: Review Request 60719: Add test infrastructure for src/python/lib/mesos.

[Eric Chung]

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

(Updated July 7, 2017, 9:30 p.m.)


Review request for mesos, Armand Grillet, Jason Lai, and Kevin Klues.


Changes
---

remove unnecessary comment


Repository: mesos


Description
---

Part of MESOS-7310, this patch adds the test infrastructure necessary
for reliably running unit tests for the mesos package located under
src/python/lib.

setup.py is added under src/python/lib to both define the Python package
and to allow tests to be run via `python setup.py test`, which delegates
tests to pytest.


Diffs (updated)
-

  src/python/lib/mesos/__init__.py e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 
  src/python/lib/requirements-test.in PRE-CREATION 
  src/python/lib/requirements.in PRE-CREATION 
  src/python/lib/setup.cfg PRE-CREATION 
  src/python/lib/setup.py PRE-CREATION 


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

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


Testing
---

1. under src/python/lib, run `virtualenv env`
2. `. env/bin/activate`
3. `pip install setuptools --upgrade`
4. `python setup.py test`


Thanks,

Eric Chung

Review Request 60719: Add test infrastructure for src/python/lib/mesos.

[Eric Chung]

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

Review request for mesos, Armand Grillet, Jason Lai, and Kevin Klues.


Repository: mesos


Description
---

Part of MESOS-7310, this patch adds the test infrastructure necessary
for reliably running unit tests for the mesos package located under
src/python/lib.

setup.py is added under src/python/lib to both define the Python package
and to allow tests to be run via `python setup.py test`, which delegates
tests to pytest.


Diffs
-

  src/python/lib/mesos/__init__.py e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 
  src/python/lib/requirements-test.in PRE-CREATION 
  src/python/lib/requirements.in PRE-CREATION 
  src/python/lib/setup.cfg PRE-CREATION 
  src/python/lib/setup.py PRE-CREATION 


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


Testing
---

1. under src/python/lib, run `virtualenv env`
2. `. env/bin/activate`
3. `pip install setuptools --upgrade`
4. `python setup.py test`


Thanks,

Eric Chung

Re: Review Request 60546: Harden Mesos when building with cmake.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [60546]

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 July 5, 2017, 4:06 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60546/
> ---
> 
> (Updated July 5, 2017, 4:06 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, and Joseph 
> Wu.
> 
> 
> Bugs: MESOS-7737
> https://issues.apache.org/jira/browse/MESOS-7737
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Apply stack protectors (stronger stack protectors with newer compilers), 
> position independent code suitable for linking into executables, security 
> warnings, and generate position independent executables.
> 
> 
> Diffs
> -
> 
>   cmake/CompilationConfigure.cmake 3fa2e2f3b 
>   cmake/MesosConfigure.cmake 5ecad2c0f 
> 
> 
> Diff: https://reviews.apache.org/r/60546/diff/2/
> 
> 
> Testing
> ---
> 
> `mkdir build && cd build && cmake .. && make -j$(nproc) && make check 
> -j$(nproc)`
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 60525: Fixed the default filter used by the allocator.

[Greg Mann]

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



As we discussed, let's check with benM, alexR about whether or not this new 
behavior is appropriate. Looks good to me, but I'd like to confirm with them.


src/master/allocator/mesos/hierarchical.cpp
Line 1044 (original), 1044 (patched)


;;



src/master/allocator/mesos/hierarchical.cpp
Lines 1046-1052 (patched)


Consider nesting this within the `seconds.isError` check, here and below.


- Greg Mann


On June 28, 2017, 11:25 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60525/
> ---
> 
> (Updated June 28, 2017, 11:25 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Mahler, Greg Mann, 
> and Vinod Kone.
> 
> 
> Bugs: MESOS-7660
> https://issues.apache.org/jira/browse/MESOS-7660
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If a framework accepts/refuses an offer using a very long filter, the
> `HierarchicalAllocator` will use the default filter instead. Meaning
> that it will filter the resources for only 5 seconds. This can happen
> when a framework sets `Filter::refuse_seconds` to a number of seconds
> larger than what fits in `Duration`.
> 
> This patch makes the allocator use the largest possible filter duration
> in this case.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> eb01d8e6b1108866ebc049f9f4a46157823a3541 
> 
> 
> Diff: https://reviews.apache.org/r/60525/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 60524: Stout: Made the `Duration` operators handle int overflows explicitly.

[Greg Mann]

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




3rdparty/stout/include/stout/duration.hpp
Line 35 (original), 35 (patched)


Consider updating this to return an error in case of over/underflow, like 
`create()`.



3rdparty/stout/include/stout/duration.hpp
Lines 121 (patched)


two slashes only



3rdparty/stout/include/stout/duration.hpp
Lines 136 (patched)


two slashes only



3rdparty/stout/include/stout/duration.hpp
Lines 149-150 (patched)


Investigate if this is easy to do now.



3rdparty/stout/include/stout/duration.hpp
Lines 152 (patched)


End comments with a period, here and elsewhere.


- Greg Mann


On June 28, 2017, 11:23 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60524/
> ---
> 
> (Updated June 28, 2017, 11:23 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Mahler, Greg Mann, 
> and Vinod Kone.
> 
> 
> Bugs: MESOS-7661
> https://issues.apache.org/jira/browse/MESOS-7661
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made the `Duration` arithmetic operators return `Duration::max()` if the
> operation would result in an integer overflow, and `Duration::min()` if
> it would result in an underflow.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/duration.hpp 
> b0cd77b833f6fbf752b4db820fd43b87e1d1e476 
>   3rdparty/stout/tests/duration_tests.cpp 
> 59b08f14849a8db31f11fbd0b2e1248c99afd9dd 
> 
> 
> Diff: https://reviews.apache.org/r/60524/diff/1/
> 
> 
> Testing
> ---
> 
> Added new Stout tests and confirmed that the Mesos test suite still passes.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Review Request 60716: Cleaned up authentication acceptor to have one class.

[Quinn Leng]

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

Review request for mesos, Anand Mazumdar, Alexander Rojas, Greg Mann, and Vinod 
Kone.


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


Repository: mesos


Description
---

Cleaned up authentication acceptor to have one class.

Replace different Authorization related Acceptor classes with one
AuthorizationAcceptor class.

Single static create function for AuthorizationAcceptor, single
acceptor function accepting ObjectApprover::Object.

Removed ObjectAcceptor parent class, since no inheritance feature
provided by it.


Diffs
-

  src/common/http.hpp b7e4a8adcbcaa3a962af795c67694a35161b6c1a 
  src/common/http.cpp fdb591eccf273260902f3f695cf431f72ee3d817 
  src/master/http.cpp 175a44ce7fb5be509453c25eaa9ec29f35adba3a 


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


Testing
---

Passed "make check -j48"
Passed 'GTEST_FILTER="MasterTest.TasksEndpoint" make check -j48'


Thanks,

Quinn Leng

Re: Review Request 60279: Add constructor for ObjectApprover::Object.

[Quinn Leng]

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

(Updated July 7, 2017, 8:01 p.m.)


Review request for mesos, Anand Mazumdar, Alexander Rojas, Greg Mann, and Vinod 
Kone.


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


Repository: mesos


Description (updated)
---

Add constructor for ObjectApprover::Object.

Updated all places where ObjectApprover::Objects
are constructed to use the constructor.


Diffs (updated)
-

  include/mesos/authorizer/authorizer.hpp 
95cbcf3f9a962a896c9c23e961f02eb16dff36f8 
  src/common/http.cpp fdb591eccf273260902f3f695cf431f72ee3d817 
  src/master/http.cpp 175a44ce7fb5be509453c25eaa9ec29f35adba3a 
  src/slave/http.cpp 700871e1502a65b0bb1fc31219e09219dbdb5340 


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

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


Testing
---

Passed "make check -j48"
Passed 'GTEST_FILTER="MasterTest.TasksEndpoint" make check -j48'


Thanks,

Quinn Leng

Re: Review Request 60638: Changed semantics of allocator 'updateSlave' method.

[Benjamin Bannier]

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

(Updated July 7, 2017, 9:38 p.m.)


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


Changes
---

Fixed handling of `optional` proto field.

Previously we implicitly converted not set `type` fields to `UNKNOWN`.
Since the field is `optional` we can actually distinguish between an
unset and an `UNKNOWN` field. We now treat messages with unset `type`
as `OVERSUBSCRIBED`, but reject `type` `UNKNOWN` since it is an
indication of an incompatible caller.


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


Repository: mesos


Description
---

We change the semantics of the 'updateSlave' method present in the
allocator interface. While previously the passed optional resource
argument was interpreted as the amount of (new) oversubscribed
resources, it now represents the new amount of total resources on the
given agent.

We addtionally add an optimization of
'HierarchicalAllocatorProcess::updateSlaveTotal' for cases where the
passed total is identical to the current total. This operation is a
no-op now and we prevent updating the sorters.


Diffs (updated)
-

  include/mesos/allocator/allocator.hpp 
bec9e0b8ef8d51ec22f9e8af69407ed5ecaa3e8f 
  src/master/allocator/mesos/allocator.hpp 
2e780c92d5c5132abff32f1ce051c3bab2947f37 
  src/master/allocator/mesos/hierarchical.hpp 
5c58cf401de9205e54300e0ce8433995cdf5cb7a 
  src/master/allocator/mesos/hierarchical.cpp 
eb01d8e6b1108866ebc049f9f4a46157823a3541 
  src/master/master.hpp 95c2d0fab32d6b60f29a86037607ff009bd78717 
  src/master/master.cpp 56b170ed70722eba30d98f6e648b6a31580d6b56 
  src/tests/hierarchical_allocator_tests.cpp 
2a312a9af4bae679a0a4e7bf45a3c013513c5da2 


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

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


Testing
---

Tested with https://reviews.apache.org/r/60639/.


Thanks,

Benjamin Bannier

Re: Review Request 59763: Caused master to abort when joining a mixed-region cluster.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On July 5, 2017, 10:32 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59763/
> ---
> 
> (Updated July 5, 2017, 10:32 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-7611
> https://issues.apache.org/jira/browse/MESOS-7611
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> That is, if a standby master is configured to use region X but it learns
> that the current master has region Y, the standby master will abort with
> an error message. This enforces the requirement that all masters in a
> single Mesos cluster are configured to use the same region (they can be
> configured to use different zones in that region, however).
> 
> To allow graceful upgrades, we only abort the standby master if both the
> standby master and the leading master have a configured domain; if
> either master has the unset (default) domain, the standby master does
> not abort.
> 
> NOTE: It would be nice to have unit tests to validate this behavior, but
> the current unit test infrastructure does not support starting multiple
> masters (MESOS-2976).
> 
> 
> Diffs
> -
> 
>   include/mesos/type_utils.hpp d2a6591b65b95b4e37e6a87dd3b11377834daf85 
>   include/mesos/v1/mesos.hpp 2479b004f342ffcb05279e3b12b6de6949190a93 
>   src/master/master.cpp 56b170ed70722eba30d98f6e648b6a31580d6b56 
> 
> 
> Diff: https://reviews.apache.org/r/59763/diff/3/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Manual testing.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 60693: Move permissions.hpp under posix

[Andrew Schwartzmeyer]

> On July 7, 2017, 6:43 p.m., Jie Yu wrote:
> > src/common/protobuf_utils.cpp
> > Line 41 (original), 41 (patched)
> > 
> >
> > It's really weird that this file is cross platform, but we have posix 
> > only header includes.
> > 
> > That means the permissions.hpp compiles on Windows previously?
> 
> Andrew Schwartzmeyer wrote:
> Not necessarily. With CMake, header files are not explicitly added to the 
> build system. They're only compiled if a `cpp` file includes it; so we'd need 
> to look at any file that includes `permissions.hpp` to determine if was 
> previously compiled on Windows.

Verified: at the least `src\common\protobuf_utils.cpp` is including it, so it's 
compiling on Windows.

(Oh dur, it's this file!)

It doesn't surprise me that this file compiles though. The struct `Permissions` 
is fine; the `S_IRUSR` flags etc. are stubbed in `stout/windows.hpp`, and 
`::stat` is part of the CRT. I'd be surprised if this code actually returned 
what the user expected though.


- Andrew


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


On July 6, 2017, 10:44 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60693/
> ---
> 
> (Updated July 6, 2017, 10:44 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and Jie Yu.
> 
> 
> Bugs: MESOS-7764
> https://issues.apache.org/jira/browse/MESOS-7764
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The way permissions are dealt with are not cross-compatible with Windows.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/posix/os.hpp 8511dfd41 
>   src/common/http.cpp 2f7718cbc 
>   src/common/protobuf_utils.cpp 4e5ab02c9 
>   src/credentials/credentials.hpp c790793c7 
>   src/tests/containerizer/provisioner_backend_tests.cpp e3516fca0 
>   src/tests/fetcher_tests.cpp 99149baa1 
> 
> 
> Diff: https://reviews.apache.org/r/60693/diff/1/
> 
> 
> Testing
> ---
> 
> `mkdir build && cd build && cmake .. && make -j$(nproc) && make check 
> -j$(nproc)`
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 60279: Add constructor for ObjectApprover::Object.

[Quinn Leng]

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

(Updated July 7, 2017, 6:52 p.m.)


Review request for mesos, Anand Mazumdar, Alexander Rojas, Greg Mann, and Vinod 
Kone.


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


Repository: mesos


Description (updated)
---

Add constructor for ObjectApprover::Object.

Replace different Authorization related Acceptor classes with one
AuthorizationAcceptor class.

Single static create function for AuthorizationAcceptor, single
acceptor function accepting ObjectApprover::Object.

Removed ObjectAcceptor parent class, since no inheritance feature
provided by it.


Diffs (updated)
-

  include/mesos/authorizer/authorizer.hpp 
95cbcf3f9a962a896c9c23e961f02eb16dff36f8 
  src/common/http.hpp b7e4a8adcbcaa3a962af795c67694a35161b6c1a 
  src/common/http.cpp fdb591eccf273260902f3f695cf431f72ee3d817 
  src/master/http.cpp 175a44ce7fb5be509453c25eaa9ec29f35adba3a 
  src/slave/http.cpp 700871e1502a65b0bb1fc31219e09219dbdb5340 


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

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


Testing (updated)
---

Passed "make check -j48"
Passed 'GTEST_FILTER="MasterTest.TasksEndpoint" make check -j48'


Thanks,

Quinn Leng

Re: Review Request 60693: Move permissions.hpp under posix

[Andrew Schwartzmeyer]

> On July 7, 2017, 6:43 p.m., Jie Yu wrote:
> > src/common/protobuf_utils.cpp
> > Line 41 (original), 41 (patched)
> > 
> >
> > It's really weird that this file is cross platform, but we have posix 
> > only header includes.
> > 
> > That means the permissions.hpp compiles on Windows previously?

Not necessarily. With CMake, header files are not explicitly added to the build 
system. They're only compiled if a `cpp` file includes it; so we'd need to look 
at any file that includes `permissions.hpp` to determine if was previously 
compiled on Windows.


- Andrew


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


On July 6, 2017, 10:44 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60693/
> ---
> 
> (Updated July 6, 2017, 10:44 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and Jie Yu.
> 
> 
> Bugs: MESOS-7764
> https://issues.apache.org/jira/browse/MESOS-7764
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The way permissions are dealt with are not cross-compatible with Windows.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/posix/os.hpp 8511dfd41 
>   src/common/http.cpp 2f7718cbc 
>   src/common/protobuf_utils.cpp 4e5ab02c9 
>   src/credentials/credentials.hpp c790793c7 
>   src/tests/containerizer/provisioner_backend_tests.cpp e3516fca0 
>   src/tests/fetcher_tests.cpp 99149baa1 
> 
> 
> Diff: https://reviews.apache.org/r/60693/diff/1/
> 
> 
> Testing
> ---
> 
> `mkdir build && cd build && cmake .. && make -j$(nproc) && make check 
> -j$(nproc)`
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 60693: Move permissions.hpp under posix

[Jie Yu]

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




src/common/protobuf_utils.cpp
Line 41 (original), 41 (patched)


It's really weird that this file is cross platform, but we have posix only 
header includes.

That means the permissions.hpp compiles on Windows previously?


- Jie Yu


On July 6, 2017, 10:44 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60693/
> ---
> 
> (Updated July 6, 2017, 10:44 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and Jie Yu.
> 
> 
> Bugs: MESOS-7764
> https://issues.apache.org/jira/browse/MESOS-7764
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The way permissions are dealt with are not cross-compatible with Windows.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/posix/os.hpp 8511dfd41 
>   src/common/http.cpp 2f7718cbc 
>   src/common/protobuf_utils.cpp 4e5ab02c9 
>   src/credentials/credentials.hpp c790793c7 
>   src/tests/containerizer/provisioner_backend_tests.cpp e3516fca0 
>   src/tests/fetcher_tests.cpp 99149baa1 
> 
> 
> Diff: https://reviews.apache.org/r/60693/diff/1/
> 
> 
> Testing
> ---
> 
> `mkdir build && cd build && cmake .. && make -j$(nproc) && make check 
> -j$(nproc)`
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Review Request 60713: Modified the access specifier for `net::IP` and `net::IP::Network`.

[Avinash sridharan]

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

Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Modified the access specifier for `net::IP` and `net::IP::Network`.


Diffs
-

  3rdparty/stout/include/stout/ip.hpp 6c2f323a4ccb140c54f00236fb50c21180a84ebb 


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


Testing
---

make check


Thanks,

Avinash sridharan

Review Request 60712: Added filtering support for '/slave/containers' endpoint.

[Quinn Leng]

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

Review request for mesos, Anand Mazumdar, Alexander Rojas, Greg Mann, and Vinod 
Kone.


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


Repository: mesos


Description
---

Added filtering support for '/slave/containers' endpoint.

Add query parameter support for '/slave/containers' endpoint.
To get the information about a specific container, you can specify
the container_id. The request would look like
'/slave/containers?container_id=[container id]'.

When no container id is specified, it will return the information
about all containers.


Diffs
-

  src/common/http.hpp b7e4a8adcbcaa3a962af795c67694a35161b6c1a 
  src/common/http.cpp fdb591eccf273260902f3f695cf431f72ee3d817 
  src/slave/http.hpp ad412d4e2fde13a7afa3563c611ef50b11829cac 
  src/slave/http.cpp 700871e1502a65b0bb1fc31219e09219dbdb5340 
  src/tests/slave_tests.cpp 8a69cc2ede0b2f17a31986e9142aa2081691eb5e 


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


Testing
---

Passed 'make check -j48'
Passed 'GLOG_v=1 ./bin/mesos-test.sh 
--gtest_filter=SlaveTest.ContainersEndpoint --gtest_repeat=1000 
--gtest_break_on_failure'


Thanks,

Quinn Leng

Re: Review Request 60641: Allowed to pass total resources in 'UpdateSlaveMessage'.

[Jie Yu]

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


Fix it, then Ship it!





src/master/master.cpp
Line 900 (original), 901 (patched)


Given that 'type' here might be optional. It's weird that we use this 
overload of 'install'.

Let's just use the install variant that takes the protobuf message itself.

```
install(&Master::updateSlave);

void Master::updateSlave(const UpdateSlaveMessage& message);
```


- Jie Yu


On July 7, 2017, 8:54 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60641/
> ---
> 
> (Updated July 7, 2017, 8:54 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Jan Schlicht.
> 
> 
> Bugs: MESOS-7757
> https://issues.apache.org/jira/browse/MESOS-7757
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit both extends the existing 'UpdateSlaveMessage' proto
> message, and adjusts its handling on the agent and master side.
> 
> To distinguish updates to 'oversubscribed_resources' from updates to
> 'total_resources' the message now contains a 'type' field to allow
> disambiguation among an empty list of resources and an unset list of
> resources. For backwards-compatibility we assume whenever the type
> field was not set that caller intended to use the
> 'oversubscribed_resources' field as opposed to the 'total_resources'
> field.
> 
> Currently, passing 'total_resources' is handled neither in the master
> nor the default allocator; we will implement this in a subsequent
> patch.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 95c2d0fab32d6b60f29a86037607ff009bd78717 
>   src/master/master.cpp 56b170ed70722eba30d98f6e648b6a31580d6b56 
>   src/messages/messages.proto 67cee3146a45f8f720b4e60739cf85085d18259b 
>   src/slave/slave.cpp 52f673633fa0041259ae40f88d4c0281a1e053b0 
>   src/tests/oversubscription_tests.cpp 
> 2266510091b49c61310a363ab6644708655787dc 
> 
> 
> Diff: https://reviews.apache.org/r/60641/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 59761: Added master and agent flags to specify domain.

[Neil Conway]

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

(Updated July 7, 2017, 4:20 p.m.)


Review request for mesos and Vinod Kone.


Changes
---

Rebase.


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


Repository: mesos


Description
---

Added master and agent flags to specify domain.


Diffs (updated)
-

  docs/configuration.md 007d0f556375f331148c9ad09be8b301ab31a01c 
  src/common/parse.hpp 64eabf89f81a6708cfac89081e0d9f9586721693 
  src/master/flags.hpp 8a9bd2dfa513f1de85dab2cc3cc1d78e80293837 
  src/master/flags.cpp ca5f02c728a9f39abbbee59fa169f95d20c69d3d 
  src/slave/flags.hpp 858876fe86cc50cb3655b06071db6450916cb814 
  src/slave/flags.cpp 74df647cc4f1161d061f0349011ce070db3a9789 


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

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


Testing
---

`make check`


Thanks,

Neil Conway

Re: Review Request 60397: Check perf version compatibility in tests with disabled coredumps.

[Andrei Budnik]

> On July 7, 2017, 1:15 p.m., Alexander Rojas wrote:
> > src/linux/perf.cpp
> > Lines 245 (patched)
> > 
> >
> > In this case, `LOG(ERROR)` is the wrong log level, since you may expect 
> > in certain cases for the error to occur.
> > 
> > `LOG(INFO)` seems more reasonable.

But I see `LOG(ERROR)` below in the code in `supported()`. In addition, it 
looks like it's a real issue when perf is broken or isn't installed. If perf 
isn't available, then it's the issue that should be fixed, right?


- Andrei


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


On July 3, 2017, 10:57 a.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60397/
> ---
> 
> (Updated July 3, 2017, 10:57 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, James Peach, and Neil Conway.
> 
> 
> Bugs: MESOS-7160
> https://issues.apache.org/jira/browse/MESOS-7160
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> For autotools build, the docker-build script performs a 'distcheck'
> build. This type of build warns if any unexpected files are left in
> the build directory after an uninstall, mainly to detect broken
> uninstall Makefile rules. The return status of the build container is
> the result of the distcheck.
> This fixes an issue where in some dockerized configurations
> invocations of 'perf' segfaulted (producing a core file as a
> side-effect), where the failure case was already anticipated and
> handled by the caller.
> 
> 
> Diffs
> -
> 
>   src/linux/perf.cpp b301e25d5cac6488c57e4f983e4867c72368a040 
>   src/tests/containerizer/perf_tests.cpp 
> d8aab08eb131f974821fb85662cbc6cc685d2f3e 
> 
> 
> Diff: https://reviews.apache.org/r/60397/diff/3/
> 
> 
> Testing
> ---
> 
> 1. make check (mac os x 10.12, fedora 25)
> 2. internal CI
> 
> Needs to be confirmed by Apache CI, e.g., reviewbot.
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 60397: Check perf version compatibility in tests with disabled coredumps.

[Andrei Budnik]

> On July 7, 2017, 1:15 p.m., Alexander Rojas wrote:
> > src/linux/perf.cpp
> > Lines 243 (patched)
> > 
> >
> > Why this command? two lines later `perf::version()` is called. Isn't 
> > there a way to merge this with the `perf::version()` call?

`perf::version()` also used in tests:
```cpp
// Returns the detected perf version. Exposed for testing.
process::Future version();
```
If I move this code to `perf::version()`, then its failure might be interpreted 
in multiple ways:
1. `perf --version` failed => perf isn’t installed or coredumped
2. `parseVersion()` failed
`PerfTest.Version` test checks whether we are able to parse perf version. If 
perf isn’t installed, the test shall pass. Quote from James:
“This drops the test that verifies we can actually parse perf versions that are 
found in the wild. That test is necessary to catch when new systems play games 
with the perf version; otherwise we would just detect that the perf version 
isn’t supported and it would be hard to diagnose why not.”


- Andrei


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


On July 3, 2017, 10:57 a.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60397/
> ---
> 
> (Updated July 3, 2017, 10:57 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, James Peach, and Neil Conway.
> 
> 
> Bugs: MESOS-7160
> https://issues.apache.org/jira/browse/MESOS-7160
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> For autotools build, the docker-build script performs a 'distcheck'
> build. This type of build warns if any unexpected files are left in
> the build directory after an uninstall, mainly to detect broken
> uninstall Makefile rules. The return status of the build container is
> the result of the distcheck.
> This fixes an issue where in some dockerized configurations
> invocations of 'perf' segfaulted (producing a core file as a
> side-effect), where the failure case was already anticipated and
> handled by the caller.
> 
> 
> Diffs
> -
> 
>   src/linux/perf.cpp b301e25d5cac6488c57e4f983e4867c72368a040 
>   src/tests/containerizer/perf_tests.cpp 
> d8aab08eb131f974821fb85662cbc6cc685d2f3e 
> 
> 
> Diff: https://reviews.apache.org/r/60397/diff/3/
> 
> 
> Testing
> ---
> 
> 1. make check (mac os x 10.12, fedora 25)
> 2. internal CI
> 
> Needs to be confirmed by Apache CI, e.g., reviewbot.
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 59525: Enabled filtering of reservations.

[Alexander Rojas]

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

(Updated July 7, 2017, 5:05 p.m.)


Review request for mesos, Adam B, Greg Mann, and Till Toenshoff.


Changes
---

Rebased based on the changes of reservations.


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


Repository: mesos


Description
---

Adds support of the 'VIEW_ROLE' ACL to the results generated by the
'/slaves' as well as the 'GET_AGENTS' API v1 call, '/states' and
'/states-summary'.

This means that calls to these endpoints and API call which uses
reservations will only show the reserved resources the user making the
requests is allowed to see based on the reservations roles.


Diffs (updated)
-

  src/master/http.cpp 175a44ce7fb5be509453c25eaa9ec29f35adba3a 
  src/master/master.hpp 95c2d0fab32d6b60f29a86037607ff009bd78717 
  src/tests/master_tests.cpp c778c6c56d47c4033189912cebee6024be79106f 


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

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


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 60397: Check perf version compatibility in tests with disabled coredumps.

[Alexander Rojas]

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




src/linux/perf.cpp
Lines 243 (patched)


Why this command? two lines later `perf::version()` is called. Isn't there 
a way to merge this with the `perf::version()` call?



src/linux/perf.cpp
Lines 245 (patched)


In this case, `LOG(ERROR)` is the wrong log level, since you may expect in 
certain cases for the error to occur.

`LOG(INFO)` seems more reasonable.


- Alexander Rojas


On July 3, 2017, 12:57 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60397/
> ---
> 
> (Updated July 3, 2017, 12:57 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, James Peach, and Neil Conway.
> 
> 
> Bugs: MESOS-7160
> https://issues.apache.org/jira/browse/MESOS-7160
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> For autotools build, the docker-build script performs a 'distcheck'
> build. This type of build warns if any unexpected files are left in
> the build directory after an uninstall, mainly to detect broken
> uninstall Makefile rules. The return status of the build container is
> the result of the distcheck.
> This fixes an issue where in some dockerized configurations
> invocations of 'perf' segfaulted (producing a core file as a
> side-effect), where the failure case was already anticipated and
> handled by the caller.
> 
> 
> Diffs
> -
> 
>   src/linux/perf.cpp b301e25d5cac6488c57e4f983e4867c72368a040 
>   src/tests/containerizer/perf_tests.cpp 
> d8aab08eb131f974821fb85662cbc6cc685d2f3e 
> 
> 
> Diff: https://reviews.apache.org/r/60397/diff/3/
> 
> 
> Testing
> ---
> 
> 1. make check (mac os x 10.12, fedora 25)
> 2. internal CI
> 
> Needs to be confirmed by Apache CI, e.g., reviewbot.
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 60697: Added mesos.http module for abstracting out http requests in the new cli.

[Armand Grillet]

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




src/python/lib/mesos/http.py
Lines 35 (patched)


According to `pep257`, the first line should be in imperative mood and end 
with a period. There should be one blank line between this summary line and the 
rest of the description.



src/python/lib/mesos/http.py
Lines 48 (patched)


Same comment as the one before.



src/python/lib/mesos/http.py
Lines 55 (patched)


The first line of the docstring should describe anything that you cannot 
tell from the method's signature. E.g. the choice of using a frozenset for the 
default headers.



src/python/lib/mesos/http.py
Lines 84 (patched)


You can drop `current` and then have a summary line.



src/python/lib/mesos/http.py
Lines 102 (patched)


A period is missing. The method is described as a token and not as a verb 
in https://www.w3.org/Protocols/rfc2616/rfc2616-sec5.html.


- Armand Grillet


On July 7, 2017, 1:28 a.m., Eric Chung wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60697/
> ---
> 
> (Updated July 7, 2017, 1:28 a.m.)
> 
> 
> Review request for mesos, Armand Grillet, Jason Lai, and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Part of MESOS-7310, this patch adds the mesos.http module to the 
> src/python/lib/mesos
> package. This module will encapsulate the details in making an http request, 
> such as
> auth scheme, default headers, timeouts, and so on. Retries will be added in 
> subsequent diffs.
> 
> 
> Diffs
> -
> 
>   src/python/lib/mesos/http.py PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60697/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Eric Chung
> 
>

Re: Review Request 60704: Handled EINTR for `waitpid()` in ChildHook::SUPERVISOR from subprocess.

[Benjamin Bannier]

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


Ship it!




Thanks for the cleanup!

- Benjamin Bannier


On July 7, 2017, 1:15 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60704/
> ---
> 
> (Updated July 7, 2017, 1:15 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Bannier, and James 
> Peach.
> 
> 
> Bugs: MESOS-7160
> https://issues.apache.org/jira/browse/MESOS-7160
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Handled EINTR for `waitpid()` in ChildHook::SUPERVISOR from subprocess.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/subprocess.cpp 
> 0f1532b294d6d6b1e017468cfde47362f3faa84d 
> 
> 
> Diff: https://reviews.apache.org/r/60704/diff/1/
> 
> 
> Testing
> ---
> 
> make check (mac os x, fedora 25)
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 60598: Replaced `abort()` with `_exit()` in `ChildHook::SUPERVISOR`.

[Benjamin Bannier]

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


Ship it!




- Benjamin Bannier


On July 3, 2017, 4:58 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60598/
> ---
> 
> (Updated July 3, 2017, 4:58 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Bannier, Benjamin 
> Mahler, and James Peach.
> 
> 
> Bugs: MESOS-7160
> https://issues.apache.org/jira/browse/MESOS-7160
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, `abort()` was called in `SUPERVISOR` hook when child
> process exited with an error code, or `waitpid()` failed, or parent
> process exited. All these cases shouldn't lead to abnormal program
> termination with coredumps.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/subprocess.cpp 
> 0f1532b294d6d6b1e017468cfde47362f3faa84d 
> 
> 
> Diff: https://reviews.apache.org/r/60598/diff/3/
> 
> 
> Testing
> ---
> 
> make check (mac os x, fedora 25)
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 60598: Replaced `abort()` with `_exit()` in `ChildHook::SUPERVISOR`.

[Andrei Budnik]

> On July 5, 2017, 4:29 p.m., Benjamin Bannier wrote:
> > 3rdparty/libprocess/src/subprocess.cpp
> > Lines 176 (patched)
> > 
> >
> > Any reason we cannot do a proper `exit` here?
> > 
> > Here and below.

1. `exit()` isn't async-signal-safe function.
2. Watchdog (i.e. parent) process should retransmit any failures from a child 
process, hence watchdog exits with `EXIT_FAILURE` in case of a child failure.


- Andrei


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


On July 3, 2017, 2:58 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60598/
> ---
> 
> (Updated July 3, 2017, 2:58 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Bannier, Benjamin 
> Mahler, and James Peach.
> 
> 
> Bugs: MESOS-7160
> https://issues.apache.org/jira/browse/MESOS-7160
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, `abort()` was called in `SUPERVISOR` hook when child
> process exited with an error code, or `waitpid()` failed, or parent
> process exited. All these cases shouldn't lead to abnormal program
> termination with coredumps.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/subprocess.cpp 
> 0f1532b294d6d6b1e017468cfde47362f3faa84d 
> 
> 
> Diff: https://reviews.apache.org/r/60598/diff/3/
> 
> 
> Testing
> ---
> 
> make check (mac os x, fedora 25)
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Review Request 60704: Handled EINTR for `waitpid()` in ChildHook::SUPERVISOR from subprocess.

[Andrei Budnik]

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

Review request for mesos, Alexander Rukletsov, Benjamin Bannier, and James 
Peach.


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


Repository: mesos


Description
---

Handled EINTR for `waitpid()` in ChildHook::SUPERVISOR from subprocess.


Diffs (updated)
-

  3rdparty/libprocess/src/subprocess.cpp 
0f1532b294d6d6b1e017468cfde47362f3faa84d 


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


Testing (updated)
---

make check (mac os x, fedora 25)


Thanks,

Andrei Budnik

Re: Review Request 60638: Changed semantics of allocator 'updateSlave' method.

[Benjamin Bannier]

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

(Updated July 7, 2017, 10:54 a.m.)


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


Changes
---

Added optimization for cases where new total == old total.


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


Repository: mesos


Description (updated)
---

We change the semantics of the 'updateSlave' method present in the
allocator interface. While previously the passed optional resource
argument was interpreted as the amount of (new) oversubscribed
resources, it now represents the new amount of total resources on the
given agent.

We addtionally add an optimization of
'HierarchicalAllocatorProcess::updateSlaveTotal' for cases where the
passed total is identical to the current total. This operation is a
no-op now and we prevent updating the sorters.


Diffs (updated)
-

  include/mesos/allocator/allocator.hpp 
bec9e0b8ef8d51ec22f9e8af69407ed5ecaa3e8f 
  src/master/allocator/mesos/allocator.hpp 
2e780c92d5c5132abff32f1ce051c3bab2947f37 
  src/master/allocator/mesos/hierarchical.hpp 
5c58cf401de9205e54300e0ce8433995cdf5cb7a 
  src/master/allocator/mesos/hierarchical.cpp 
eb01d8e6b1108866ebc049f9f4a46157823a3541 
  src/master/master.cpp 56b170ed70722eba30d98f6e648b6a31580d6b56 
  src/tests/hierarchical_allocator_tests.cpp 
2a312a9af4bae679a0a4e7bf45a3c013513c5da2 


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

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


Testing
---

Tested with https://reviews.apache.org/r/60639/.


Thanks,

Benjamin Bannier

Re: Review Request 60638: Changed semantics of allocator 'updateSlave' method.

[Benjamin Bannier]

> On July 7, 2017, 5:54 a.m., Jie Yu wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Line 652 (original), 652 (patched)
> > 
> >
> > Let's update `updateSlaveTotal` so that if the old total is the same as 
> > the new total, we don't need to update the sorter. That means 
> > updateSlaveTotal should probably return a bool to indicate if there is any 
> > change.
> > 
> > This is a great cleanup! Much cleaner now!

Cleaned up.


- Benjamin


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


On July 7, 2017, 10:54 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60638/
> ---
> 
> (Updated July 7, 2017, 10:54 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jie Yu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-7755
> https://issues.apache.org/jira/browse/MESOS-7755
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We change the semantics of the 'updateSlave' method present in the
> allocator interface. While previously the passed optional resource
> argument was interpreted as the amount of (new) oversubscribed
> resources, it now represents the new amount of total resources on the
> given agent.
> 
> We addtionally add an optimization of
> 'HierarchicalAllocatorProcess::updateSlaveTotal' for cases where the
> passed total is identical to the current total. This operation is a
> no-op now and we prevent updating the sorters.
> 
> 
> Diffs
> -
> 
>   include/mesos/allocator/allocator.hpp 
> bec9e0b8ef8d51ec22f9e8af69407ed5ecaa3e8f 
>   src/master/allocator/mesos/allocator.hpp 
> 2e780c92d5c5132abff32f1ce051c3bab2947f37 
>   src/master/allocator/mesos/hierarchical.hpp 
> 5c58cf401de9205e54300e0ce8433995cdf5cb7a 
>   src/master/allocator/mesos/hierarchical.cpp 
> eb01d8e6b1108866ebc049f9f4a46157823a3541 
>   src/master/master.cpp 56b170ed70722eba30d98f6e648b6a31580d6b56 
>   src/tests/hierarchical_allocator_tests.cpp 
> 2a312a9af4bae679a0a4e7bf45a3c013513c5da2 
> 
> 
> Diff: https://reviews.apache.org/r/60638/diff/3/
> 
> 
> Testing
> ---
> 
> Tested with https://reviews.apache.org/r/60639/.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 60641: Allowed to pass total resources in 'UpdateSlaveMessage'.

[Benjamin Bannier]

> On July 7, 2017, 5:41 a.m., Jie Yu wrote:
> > src/messages/messages.proto
> > Lines 628 (patched)
> > 
> >
> > s/required/optional/
> > 
> > See why enum should be optional in MESOS-4997

I made this field `optional` now.


> On July 7, 2017, 5:41 a.m., Jie Yu wrote:
> > src/slave/slave.cpp
> > Lines 1233 (patched)
> > 
> >
> > Then, you don't have to change these if type is optional.

I would prefer to still explicitly set this field as that is the now expected 
usage pattern. We should only automigrate `UNKNOWN` -> `OVERSUBSCRIBED` for 
backwards-compatibility reasons external users, not to save tying.


- Benjamin


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


On July 7, 2017, 10:54 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60641/
> ---
> 
> (Updated July 7, 2017, 10:54 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Jan Schlicht.
> 
> 
> Bugs: MESOS-7757
> https://issues.apache.org/jira/browse/MESOS-7757
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit both extends the existing 'UpdateSlaveMessage' proto
> message, and adjusts its handling on the agent and master side.
> 
> To distinguish updates to 'oversubscribed_resources' from updates to
> 'total_resources' the message now contains a 'type' field to allow
> disambiguation among an empty list of resources and an unset list of
> resources. For backwards-compatibility we assume whenever the type
> field was not set that caller intended to use the
> 'oversubscribed_resources' field as opposed to the 'total_resources'
> field.
> 
> Currently, passing 'total_resources' is handled neither in the master
> nor the default allocator; we will implement this in a subsequent
> patch.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 95c2d0fab32d6b60f29a86037607ff009bd78717 
>   src/master/master.cpp 56b170ed70722eba30d98f6e648b6a31580d6b56 
>   src/messages/messages.proto 67cee3146a45f8f720b4e60739cf85085d18259b 
>   src/slave/slave.cpp 52f673633fa0041259ae40f88d4c0281a1e053b0 
>   src/tests/oversubscription_tests.cpp 
> 2266510091b49c61310a363ab6644708655787dc 
> 
> 
> Diff: https://reviews.apache.org/r/60641/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 60639: Added MESOS-7755 to the upgrades guide and the CHANGELOG.

[Benjamin Bannier]

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

(Updated July 7, 2017, 10:54 a.m.)


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


Changes
---

Updated `CHANGELOG` as well.


Summary (updated)
-

Added MESOS-7755 to the upgrades guide and the CHANGELOG.


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


Repository: mesos


Description (updated)
---

Added MESOS-7755 to the upgrades guide and the CHANGELOG.


Diffs (updated)
-

  CHANGELOG abfded9164de64c46c0d34041a7569961bfea58b 
  docs/upgrades.md 55118805f9c522235b4e1168852b4f9c4420e300 


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

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


Testing
---

`make check`


Thanks,

Benjamin Bannier

Re: Review Request 60641: Allowed to pass total resources in 'UpdateSlaveMessage'.

[Benjamin Bannier]

- Benjamin


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


On July 7, 2017, 10:54 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60641/
> ---
> 
> (Updated July 7, 2017, 10:54 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Jan Schlicht.
> 
> 
> Bugs: MESOS-7757
> https://issues.apache.org/jira/browse/MESOS-7757
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit both extends the existing 'UpdateSlaveMessage' proto
> message, and adjusts its handling on the agent and master side.
> 
> To distinguish updates to 'oversubscribed_resources' from updates to
> 'total_resources' the message now contains a 'type' field to allow
> disambiguation among an empty list of resources and an unset list of
> resources. For backwards-compatibility we assume whenever the type
> field was not set that caller intended to use the
> 'oversubscribed_resources' field as opposed to the 'total_resources'
> field.
> 
> Currently, passing 'total_resources' is handled neither in the master
> nor the default allocator; we will implement this in a subsequent
> patch.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 95c2d0fab32d6b60f29a86037607ff009bd78717 
>   src/master/master.cpp 56b170ed70722eba30d98f6e648b6a31580d6b56 
>   src/messages/messages.proto 67cee3146a45f8f720b4e60739cf85085d18259b 
>   src/slave/slave.cpp 52f673633fa0041259ae40f88d4c0281a1e053b0 
>   src/tests/oversubscription_tests.cpp 
> 2266510091b49c61310a363ab6644708655787dc 
> 
> 
> Diff: https://reviews.apache.org/r/60641/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 60641: Allowed to pass total resources in 'UpdateSlaveMessage'.

[Benjamin Bannier]

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

(Updated July 7, 2017, 10:54 a.m.)


Review request for mesos, Jie Yu and Jan Schlicht.


Changes
---

Made new `required` field `optional`.


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


Repository: mesos


Description
---

This commit both extends the existing 'UpdateSlaveMessage' proto
message, and adjusts its handling on the agent and master side.

To distinguish updates to 'oversubscribed_resources' from updates to
'total_resources' the message now contains a 'type' field to allow
disambiguation among an empty list of resources and an unset list of
resources. For backwards-compatibility we assume whenever the type
field was not set that caller intended to use the
'oversubscribed_resources' field as opposed to the 'total_resources'
field.

Currently, passing 'total_resources' is handled neither in the master
nor the default allocator; we will implement this in a subsequent
patch.


Diffs (updated)
-

  src/master/master.hpp 95c2d0fab32d6b60f29a86037607ff009bd78717 
  src/master/master.cpp 56b170ed70722eba30d98f6e648b6a31580d6b56 
  src/messages/messages.proto 67cee3146a45f8f720b4e60739cf85085d18259b 
  src/slave/slave.cpp 52f673633fa0041259ae40f88d4c0281a1e053b0 
  src/tests/oversubscription_tests.cpp 2266510091b49c61310a363ab6644708655787dc 


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

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


Testing
---


Thanks,

Benjamin Bannier