Re: Review Request 72066: Improved performance of v1 agent operator API GET_TASKS call.

[Benjamin Mahler]

> On Feb. 24, 2020, 7:28 p.m., Andrei Sekretenko wrote:
> > src/slave/http.cpp
> > Lines 2325-2351 (patched)
> > 
> >
> > Similarly to r72065 - is it possible to factor this out to make clear 
> > that framework/executor lists in json and protobuf code are composed in 
> > indentical way?

Yes, we definitely have a lot of these at this point! I will look into cleaning 
this up after these patches. It also makes me wonder if the existing code makes 
sense. For example, if I'm viewing executors, should the (intermediate) 
framework list construction go through VIEW_FRAMEWORK approval? What would it 
mean if I could VIEW_EXECUTOR but I can't VIEW_FRAMEWORK for its framework? 
Should I be able to view the executor in that case? Not sure.. but it has 
implications on the refactoring.


> On Feb. 24, 2020, 7:28 p.m., Andrei Sekretenko wrote:
> > src/slave/http.cpp
> > Lines 2332 (patched)
> > 
> >
> > newline?

I cut the newlines out to make it clearer that the block was building the 
vector, on all of these similar cases. This seemed clearer, or a lambda could 
be used to better scope it, but ultimately we'll probably add a helper for this.


> On Feb. 24, 2020, 7:28 p.m., Andrei Sekretenko wrote:
> > src/slave/http.cpp
> > Lines 2379 (patched)
> > 
> >
> > Given that our styleguide builds on top of Google's C++ styleguide, 
> > shouldn't `using` be preferred over `typedef` for type aliases?

yes probably! we started using that at the top of files in lieu of typedefs but 
we haven't made use of them for the foreach workaround, I will leave as is 
since I'm just moving the code but would be nice to sweep the code base for 
this change!


> On Feb. 24, 2020, 7:28 p.m., Andrei Sekretenko wrote:
> > src/slave/http.cpp
> > Lines 2445-2446 (patched)
> > 
> >
> > CHECK_NOTNULL when calling `approved()`?

I will actually remove these, we're inconsistently using them and it's a pain 
to actually use them before every pointer de-reference in all of this code.


> On Feb. 24, 2020, 7:28 p.m., Andrei Sekretenko wrote:
> > src/slave/http.cpp
> > Lines 2461 (patched)
> > 
> >
> > Technically, shared_ptr can also store nullptr -  why have 
> > CHECK_NOTNULL in the loop before and don't have it here?

See comment above about the inconsistency.


- Benjamin


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


On Jan. 30, 2020, 9:31 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72066/
> ---
> 
> (Updated Jan. 30, 2020, 9:31 p.m.)
> 
> 
> Review request for mesos, Andrei Sekretenko and Greg Mann.
> 
> 
> Bugs: MESOS-10026
> https://issues.apache.org/jira/browse/MESOS-10026
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This follow the same approach used for the master's v1 calls:
> 
> https://github.com/apache/mesos/commit/d7dd4d0e8493331d7b7a21b504eb
> https://github.com/apache/mesos/commit/3dda3622f5ed01e8c132dc5ca594
> 
> That is, serializing directly to protobuf or json from the in-memory
> v0 state.
> 
> 
> Diffs
> -
> 
>   src/slave/http.hpp 0afdad9479f0cc2c94452b6b1f2289dd6ea01494 
>   src/slave/http.cpp 04ad0d816618a1880913857a6f0ff38c4643c488 
> 
> 
> Diff: https://reviews.apache.org/r/72066/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 71886: Set container's `cpu.cfs_quota_us` to its CPU resource limit.

[Qian Zhang]

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

(Updated Feb. 25, 2020, 9:57 a.m.)


Review request for mesos, Andrei Budnik and Greg Mann.


Changes
---

Accommodated infinite value.


Summary (updated)
-

Set container's `cpu.cfs_quota_us` to its CPU resource limit.


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


Repository: mesos


Description (updated)
---

Set container's `cpu.cfs_quota_us` to its CPU resource limit.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpu.cpp 
960bd141430387e076a8fab1948d07719613ed90 


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

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


Testing
---


Thanks,

Qian Zhang

Re: Review Request 71858: Set resource limits when launching executor container.

[Qian Zhang]

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

(Updated Feb. 25, 2020, 9:46 a.m.)


Review request for mesos, Andrei Budnik and Greg Mann.


Changes
---

Minor change.


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


Repository: mesos


Description
---

Set resource limits when launching executor container.


Diffs (updated)
-

  src/slave/slave.hpp 03279db082bdba23dbfeb2d93081a908e609aec2 
  src/slave/slave.cpp cce275a504effae7a6b71dd333ce8a300d1ce5be 


Diff: https://reviews.apache.org/r/71858/diff/10/

Changes: https://reviews.apache.org/r/71858/diff/9-10/


Testing
---


Thanks,

Qian Zhang

Re: Review Request 72065: Improved performance of v1 agent operator API GET_EXECUTORS call.

[Benjamin Mahler]

> On Feb. 24, 2020, 7:26 p.m., Andrei Sekretenko wrote:
> > src/slave/http.cpp
> > Lines 1978-1988 (patched)
> > 
> >
> > Is it possible to factor this out to make it clear that framework lists 
> > for `jsonifyGetExectorts` and `serializeGetExecutors` are identical?

Yeah, this could also be done in the master code, but since I simplified it to 
a pretty small piece of code, I left it duplicated for now.


- Benjamin


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


On Jan. 30, 2020, 9:31 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72065/
> ---
> 
> (Updated Jan. 30, 2020, 9:31 p.m.)
> 
> 
> Review request for mesos, Andrei Sekretenko and Greg Mann.
> 
> 
> Bugs: MESOS-10026
> https://issues.apache.org/jira/browse/MESOS-10026
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This follow the same approach used for the master's v1 calls:
> 
> https://github.com/apache/mesos/commit/6ab835459a452e53fec8982a5aaa
> https://github.com/apache/mesos/commit/3dda3622f5ed01e8c132dc5ca594
> 
> That is, serializing directly to protobuf or json from the in-memory
> v0 state.
> 
> 
> Diffs
> -
> 
>   src/slave/http.hpp 0afdad9479f0cc2c94452b6b1f2289dd6ea01494 
>   src/slave/http.cpp 04ad0d816618a1880913857a6f0ff38c4643c488 
> 
> 
> Diff: https://reviews.apache.org/r/72065/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 71858: Set resource limits when launching executor container.

[Qian Zhang]

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

(Updated Feb. 25, 2020, 9:43 a.m.)


Review request for mesos, Andrei Budnik and Greg Mann.


Changes
---

Accommodated infinite value.


Summary (updated)
-

Set resource limits when launching executor container.


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


Repository: mesos


Description (updated)
---

Set resource limits when launching executor container.


Diffs (updated)
-

  src/slave/slave.hpp 03279db082bdba23dbfeb2d93081a908e609aec2 
  src/slave/slave.cpp cce275a504effae7a6b71dd333ce8a300d1ce5be 


Diff: https://reviews.apache.org/r/71858/diff/9/

Changes: https://reviews.apache.org/r/71858/diff/8-9/


Testing
---


Thanks,

Qian Zhang

Review Request 72162: Accommodated the "Infinity" value in the JSON <-> Protobuf conversion.

[Qian Zhang]

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

Review request for mesos, Andrei Budnik and Greg Mann.


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


Repository: mesos


Description
---

Accommodated the "Infinity" value in the JSON <-> Protobuf conversion.


Diffs
-

  3rdparty/stout/include/stout/jsonify.hpp 
7a239d846de5679e9c2b80aa4e617a72fdeac92c 
  3rdparty/stout/include/stout/protobuf.hpp 
fcd91d5ea822612e4b32d157fcbc5f0dedeba823 
  3rdparty/stout/tests/protobuf_tests.cpp 
55889dc432a9459965d3dd75052d672828cdea90 
  3rdparty/stout/tests/protobuf_tests.proto 
5e20215fcc69e400358847d0ce943d8c18f2a9e0 


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


Testing
---


Thanks,

Qian Zhang

Review Request 72161: Added patch for RapidJSON.

[Qian Zhang]

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

Review request for mesos, Andrei Budnik and Greg Mann.


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


Repository: mesos


Description
---

This commit updates the writer of RapidJSON to write infinite
floating point numbers as "Infinity" and "-Infinity" (i.e.,
with double quotes) rather than Infinity and -Infinity. This
is to ensure the strings converted from JSON objects conform
to the rule defined by Protobuf:
https://developers.google.com/protocol-buffers/docs/proto3#json


Diffs
-

  3rdparty/CMakeLists.txt c45d742684ba4b3b4abc57ae0bcb85a879c68bfd 
  3rdparty/rapidjson-1.1.0.patch PRE-CREATION 


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


Testing
---


Thanks,

Qian Zhang

Re: Review Request 72056: Improved performance of v1 agent operator API GET_METRICS call.

[Andrei Sekretenko]

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




src/slave/http.cpp
Lines 1079 (patched)


Hmm... do I understand correctly that code for master's GET_METRICS differs 
only in the type of the `Response` protobuf?

Did you consider templatizing metrics serialization on `Response` and 
reusing it for both calls?


- Andrei Sekretenko


On Jan. 30, 2020, 9:31 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72056/
> ---
> 
> (Updated Jan. 30, 2020, 9:31 p.m.)
> 
> 
> Review request for mesos, Andrei Sekretenko and Greg Mann.
> 
> 
> Bugs: MESOS-10026
> https://issues.apache.org/jira/browse/MESOS-10026
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This follow the same approach used for the master's GET_METRICS call:
> 
> https://github.com/apache/mesos/commit/469f2ebaf65b1642d1eb4a1df81a
> https://github.com/apache/mesos/commit/3dda3622f5ed01e8c132dc5ca594
> 
> That is, serializing directly to protobuf or json from the in-memory
> v0 state.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 04ad0d816618a1880913857a6f0ff38c4643c488 
> 
> 
> Diff: https://reviews.apache.org/r/72056/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 72064: Improved performance of v1 agent operator API GET_FRAMEWORKS call.

[Andrei Sekretenko]

> On Feb. 24, 2020, 7:26 p.m., Andrei Sekretenko wrote:
> > src/slave/http.cpp
> > Lines 1776 (patched)
> > 
> >
> > I'm afraid this can mislead readers into thinking that 
> > `jsonifyGetFramework(...)` writes json on its own, whereas in reality it 
> > returns a lambda.
> > 
> > Maybe some other name, like `getFrameworkElementWriter` or even 
> > `getJsonifyFramework` could be more transpatent?

Thinking of it again, readers should be more or less used to the convention 
that `jsonifySomething()` returns a lambda throughout the codebase. Looks like 
it's better to leave this as `jsonifyGetFramework()`.


- Andrei


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


On Jan. 30, 2020, 9:45 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72064/
> ---
> 
> (Updated Jan. 30, 2020, 9:45 p.m.)
> 
> 
> Review request for mesos, Andrei Sekretenko and Greg Mann.
> 
> 
> Bugs: MESOS-10026
> https://issues.apache.org/jira/browse/MESOS-10026
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This follow the same approach used for the master's v1 calls:
> 
> https://github.com/apache/mesos/commit/4f4dab961bd45ca444d13b831cdb
> https://github.com/apache/mesos/commit/3dda3622f5ed01e8c132dc5ca594
> 
> That is, serializing directly to protobuf or json from the in-memory
> v0 state.
> 
> 
> Diffs
> -
> 
>   src/slave/http.hpp 0afdad9479f0cc2c94452b6b1f2289dd6ea01494 
>   src/slave/http.cpp 04ad0d816618a1880913857a6f0ff38c4643c488 
> 
> 
> Diff: https://reviews.apache.org/r/72064/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 72067: Improved performance of v1 agent operator API GET_STATE call.

[Andrei Sekretenko]

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


Ship it!




Ship It!

- Andrei Sekretenko


On Jan. 30, 2020, 9:31 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72067/
> ---
> 
> (Updated Jan. 30, 2020, 9:31 p.m.)
> 
> 
> Review request for mesos, Andrei Sekretenko and Greg Mann.
> 
> 
> Bugs: MESOS-10026
> https://issues.apache.org/jira/browse/MESOS-10026
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This follow the same approach used for the master's v1 calls:
> 
> https://github.com/apache/mesos/commit/1c60f0e4acbac96c34bd90e26515
> https://github.com/apache/mesos/commit/3dda3622f5ed01e8c132dc5ca594
> 
> That is, serializing directly to protobuf or json from the in-memory
> v0 state.
> 
> 
> Diffs
> -
> 
>   src/slave/http.hpp 0afdad9479f0cc2c94452b6b1f2289dd6ea01494 
>   src/slave/http.cpp 04ad0d816618a1880913857a6f0ff38c4643c488 
> 
> 
> Diff: https://reviews.apache.org/r/72067/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 72066: Improved performance of v1 agent operator API GET_TASKS call.

[Andrei Sekretenko]

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


Fix it, then Ship it!





src/slave/http.cpp
Lines 2325-2351 (patched)


Similarly to r72065 - is it possible to factor this out to make clear that 
framework/executor lists in json and protobuf code are composed in indentical 
way?



src/slave/http.cpp
Lines 2332 (patched)


newline?



src/slave/http.cpp
Lines 2379 (patched)


Given that our styleguide builds on top of Google's C++ styleguide, 
shouldn't `using` be preferred over `typedef` for type aliases?



src/slave/http.cpp
Lines 2428 (patched)


`const Task*` here and below?



src/slave/http.cpp
Lines 2445-2446 (patched)


CHECK_NOTNULL when calling `approved()`?



src/slave/http.cpp
Lines 2461 (patched)


Technically, shared_ptr can also store nullptr -  why have CHECK_NOTNULL in 
the loop before and don't have it here?


- Andrei Sekretenko


On Jan. 30, 2020, 9:31 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72066/
> ---
> 
> (Updated Jan. 30, 2020, 9:31 p.m.)
> 
> 
> Review request for mesos, Andrei Sekretenko and Greg Mann.
> 
> 
> Bugs: MESOS-10026
> https://issues.apache.org/jira/browse/MESOS-10026
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This follow the same approach used for the master's v1 calls:
> 
> https://github.com/apache/mesos/commit/d7dd4d0e8493331d7b7a21b504eb
> https://github.com/apache/mesos/commit/3dda3622f5ed01e8c132dc5ca594
> 
> That is, serializing directly to protobuf or json from the in-memory
> v0 state.
> 
> 
> Diffs
> -
> 
>   src/slave/http.hpp 0afdad9479f0cc2c94452b6b1f2289dd6ea01494 
>   src/slave/http.cpp 04ad0d816618a1880913857a6f0ff38c4643c488 
> 
> 
> Diff: https://reviews.apache.org/r/72066/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 72064: Improved performance of v1 agent operator API GET_FRAMEWORKS call.

[Andrei Sekretenko]

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


Fix it, then Ship it!





src/slave/http.cpp
Lines 1748 (patched)


Does this lambda actually capture anything? If yes, then why is it safe to 
use it from the lambda returned by `jsonifyGetFrameworks`? If no, then I'd 
suggest to remove the default capture (s/[&]/[]/) for clarity.



src/slave/http.cpp
Lines 1753-1755 (patched)


`int field = ...` ?



src/slave/http.cpp
Lines 1756 (patched)


newline?



src/slave/http.cpp
Lines 1776 (patched)


I'm afraid this can mislead readers into thinking that 
`jsonifyGetFramework(...)` writes json on its own, whereas in reality it 
returns a lambda.

Maybe some other name, like `getFrameworkElementWriter` or even 
`getJsonifyFramework` could be more transpatent?


- Andrei Sekretenko


On Jan. 30, 2020, 9:45 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72064/
> ---
> 
> (Updated Jan. 30, 2020, 9:45 p.m.)
> 
> 
> Review request for mesos, Andrei Sekretenko and Greg Mann.
> 
> 
> Bugs: MESOS-10026
> https://issues.apache.org/jira/browse/MESOS-10026
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This follow the same approach used for the master's v1 calls:
> 
> https://github.com/apache/mesos/commit/4f4dab961bd45ca444d13b831cdb
> https://github.com/apache/mesos/commit/3dda3622f5ed01e8c132dc5ca594
> 
> That is, serializing directly to protobuf or json from the in-memory
> v0 state.
> 
> 
> Diffs
> -
> 
>   src/slave/http.hpp 0afdad9479f0cc2c94452b6b1f2289dd6ea01494 
>   src/slave/http.cpp 04ad0d816618a1880913857a6f0ff38c4643c488 
> 
> 
> Diff: https://reviews.apache.org/r/72064/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 72065: Improved performance of v1 agent operator API GET_EXECUTORS call.

[Andrei Sekretenko]

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


Fix it, then Ship it!





src/slave/http.cpp
Lines 1978-1988 (patched)


Is it possible to factor this out to make it clear that framework lists for 
`jsonifyGetExectorts` and `serializeGetExecutors` are identical?



src/slave/http.cpp
Lines 1994 (patched)


See questions regarding `jsonifyGetFramework` in previous patch (r72064).



src/slave/http.cpp
Lines 2104 (patched)


`const Executor*` ?


- Andrei Sekretenko


On Jan. 30, 2020, 9:31 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72065/
> ---
> 
> (Updated Jan. 30, 2020, 9:31 p.m.)
> 
> 
> Review request for mesos, Andrei Sekretenko and Greg Mann.
> 
> 
> Bugs: MESOS-10026
> https://issues.apache.org/jira/browse/MESOS-10026
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This follow the same approach used for the master's v1 calls:
> 
> https://github.com/apache/mesos/commit/6ab835459a452e53fec8982a5aaa
> https://github.com/apache/mesos/commit/3dda3622f5ed01e8c132dc5ca594
> 
> That is, serializing directly to protobuf or json from the in-memory
> v0 state.
> 
> 
> Diffs
> -
> 
>   src/slave/http.hpp 0afdad9479f0cc2c94452b6b1f2289dd6ea01494 
>   src/slave/http.cpp 04ad0d816618a1880913857a6f0ff38c4643c488 
> 
> 
> Diff: https://reviews.apache.org/r/72065/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 72158: Fixed systemd socket activation on old systemd versions.

[Andrei Sekretenko]

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


Ship it!




Ship It!

- Andrei Sekretenko


On Feb. 24, 2020, 4:39 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72158/
> ---
> 
> (Updated Feb. 24, 2020, 4:39 p.m.)
> 
> 
> Review request for mesos, Andrei Sekretenko, Greg Mann, and Qian Zhang.
> 
> 
> Bugs: MESOS-10098
> https://issues.apache.org/jira/browse/MESOS-10098
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch fixes the bug when `listenFdsWithName` function returns
> an empty set of file descriptors on pre-227 systemd versions when
> `domain_socket_location` value is not equals to the "systemd:unknown".
> This happens when a user expects a newer version of systemd and
> specifies a "systemd/", but
> the actual systemd version does not support `FileDescriptorName`.
> In this case, `LISTEN_FDNAMES` env variable is not specified,
> so all socket FDs, which are specified by the `LISTEN_FDS`,
> must be used to locate the path to the domain socket.
> 
> 
> Diffs
> -
> 
>   src/linux/systemd.hpp ba960009d4e47f253fccfcd0edeedf4e6cdc0dca 
>   src/linux/systemd.cpp 9897473115ac0f9809734c109ba412eefd32e59e 
>   src/slave/main.cpp c1e65519f4c684dcd608cbd1a67d7f5945161af4 
> 
> 
> Diff: https://reviews.apache.org/r/72158/diff/2/
> 
> 
> Testing
> ---
> 
> internal CI (including CoreOS)
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 72158: Fixed systemd socket activation on old systemd versions.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [72157, 72158]

Passed command: export OS='ubuntu:16.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose --disable-libtool-wrappers 
--disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/jenkins/buildbot.sh

- Mesos Reviewbot


On Feb. 24, 2020, 4:39 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72158/
> ---
> 
> (Updated Feb. 24, 2020, 4:39 p.m.)
> 
> 
> Review request for mesos, Andrei Sekretenko, Greg Mann, and Qian Zhang.
> 
> 
> Bugs: MESOS-10098
> https://issues.apache.org/jira/browse/MESOS-10098
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch fixes the bug when `listenFdsWithName` function returns
> an empty set of file descriptors on pre-227 systemd versions when
> `domain_socket_location` value is not equals to the "systemd:unknown".
> This happens when a user expects a newer version of systemd and
> specifies a "systemd/", but
> the actual systemd version does not support `FileDescriptorName`.
> In this case, `LISTEN_FDNAMES` env variable is not specified,
> so all socket FDs, which are specified by the `LISTEN_FDS`,
> must be used to locate the path to the domain socket.
> 
> 
> Diffs
> -
> 
>   src/linux/systemd.hpp ba960009d4e47f253fccfcd0edeedf4e6cdc0dca 
>   src/linux/systemd.cpp 9897473115ac0f9809734c109ba412eefd32e59e 
>   src/slave/main.cpp c1e65519f4c684dcd608cbd1a67d7f5945161af4 
> 
> 
> Diff: https://reviews.apache.org/r/72158/diff/2/
> 
> 
> Testing
> ---
> 
> internal CI (including CoreOS)
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 72158: Fixed systemd socket activation on old systemd versions.

[Andrei Budnik]

> On Фев. 24, 2020, 1:49 п.п., Andrei Sekretenko wrote:
> > src/linux/systemd.cpp
> > Line 400 (original), 400 (patched)
> > 
> >
> > If this function starts to behave this way to work around older 
> > systemds, at the bare minimum it should be renamed. Otherwise the code in 
> > `slave/main.cpp` becomes totally misleading.
> > 
> > Also, this workaround (and, honestly, the whole situation with pre-277 
> > systemds) looks rather fragile. 
> > This makes me wonder if there is a simple reliable way to detect 
> > systemd version (I'm looking at some other code in this file that seems to 
> > do this) and suppress this workaround for newer versions (and log warning 
> > for older versions!). 
> > If there is no simple way, this should probably be left as TODO...
> > 
> > 
> > I see at least two options how to avoid misleading readers of 
> > `slave/main.cpp`.
> > 
> > 1) change signature to `listenFdsWithNames(const std::set& 
> > names)` so that the code in main.cpp looks like 
> > ```
> > // NOTE: pre-277 systemd versions do not support FileDescriptorName, 
> > thus we also need to listen on descriptiors with name "unknown"
> > Try> socketFds =
> > systemd::socket_activation::listenFdsWithNames({name , "unknown"})
> > ```
> > 
> > 2) Rename this function (with added `== "unknown"`) into 
> > `listenFds(const string& name)` and, optionally, make `listenFds()` with no 
> > arguments static.
> > This way, systemd version check, if we decide to add it, can be 
> > introduced into this fucntion, instead of going somewhere outside.

Updated the function (choose option 1 from your suggestion).


- Andrei


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


On Фев. 24, 2020, 4:39 п.п., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72158/
> ---
> 
> (Updated Фев. 24, 2020, 4:39 п.п.)
> 
> 
> Review request for mesos, Andrei Sekretenko, Greg Mann, and Qian Zhang.
> 
> 
> Bugs: MESOS-10098
> https://issues.apache.org/jira/browse/MESOS-10098
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch fixes the bug when `listenFdsWithName` function returns
> an empty set of file descriptors on pre-227 systemd versions when
> `domain_socket_location` value is not equals to the "systemd:unknown".
> This happens when a user expects a newer version of systemd and
> specifies a "systemd/", but
> the actual systemd version does not support `FileDescriptorName`.
> In this case, `LISTEN_FDNAMES` env variable is not specified,
> so all socket FDs, which are specified by the `LISTEN_FDS`,
> must be used to locate the path to the domain socket.
> 
> 
> Diffs
> -
> 
>   src/linux/systemd.hpp ba960009d4e47f253fccfcd0edeedf4e6cdc0dca 
>   src/linux/systemd.cpp 9897473115ac0f9809734c109ba412eefd32e59e 
>   src/slave/main.cpp c1e65519f4c684dcd608cbd1a67d7f5945161af4 
> 
> 
> Diff: https://reviews.apache.org/r/72158/diff/2/
> 
> 
> Testing
> ---
> 
> internal CI (including CoreOS)
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 72158: Fixed systemd socket activation on old systemd versions.

[Andrei Budnik]

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

(Updated Фев. 24, 2020, 4:39 п.п.)


Review request for mesos, Andrei Sekretenko, Greg Mann, and Qian Zhang.


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


Repository: mesos


Description
---

This patch fixes the bug when `listenFdsWithName` function returns
an empty set of file descriptors on pre-227 systemd versions when
`domain_socket_location` value is not equals to the "systemd:unknown".
This happens when a user expects a newer version of systemd and
specifies a "systemd/", but
the actual systemd version does not support `FileDescriptorName`.
In this case, `LISTEN_FDNAMES` env variable is not specified,
so all socket FDs, which are specified by the `LISTEN_FDS`,
must be used to locate the path to the domain socket.


Diffs (updated)
-

  src/linux/systemd.hpp ba960009d4e47f253fccfcd0edeedf4e6cdc0dca 
  src/linux/systemd.cpp 9897473115ac0f9809734c109ba412eefd32e59e 
  src/slave/main.cpp c1e65519f4c684dcd608cbd1a67d7f5945161af4 


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

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


Testing
---

internal CI (including CoreOS)


Thanks,

Andrei Budnik

Re: Review Request 72158: Fixed systemd socket activation on old systemd versions.

[Andrei Budnik]

> On Фев. 24, 2020, 1:49 п.п., Andrei Sekretenko wrote:
> > Do we have a MESOS bug for this? Looks like we should...

Added a link to the https://issues.apache.org/jira/browse/MESOS-10098


- Andrei


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


On Фев. 21, 2020, 12:30 п.п., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72158/
> ---
> 
> (Updated Фев. 21, 2020, 12:30 п.п.)
> 
> 
> Review request for mesos, Andrei Sekretenko, Greg Mann, and Qian Zhang.
> 
> 
> Bugs: MESOS-10098
> https://issues.apache.org/jira/browse/MESOS-10098
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch fixes the bug when `listenFdsWithName` function returns
> an empty set of file descriptors on pre-227 systemd versions when
> `domain_socket_location` value is not equals to the "systemd:unknown".
> This happens when a user expects a newer version of systemd and
> specifies a "systemd/", but
> the actual systemd version does not support `FileDescriptorName`.
> In this case, `LISTEN_FDNAMES` env variable is not specified,
> so all socket FDs, which are specified by the `LISTEN_FDS`,
> must be used to locate the path to the domain socket.
> 
> 
> Diffs
> -
> 
>   src/linux/systemd.cpp 9897473115ac0f9809734c109ba412eefd32e59e 
> 
> 
> Diff: https://reviews.apache.org/r/72158/diff/1/
> 
> 
> Testing
> ---
> 
> internal CI (including CoreOS)
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 72157: Removed reimplementation of `cloexec` from systemd activation code.

[Andrei Budnik]

> On Фев. 24, 2020, 1:10 п.п., Andrei Sekretenko wrote:
> > src/linux/systemd.cpp
> > Line 352 (original)
> > 
> >
> > `cloexec(...)` from stout doesn't have this optimization and always 
> > calls `fcntl(fd, F_SETFD, ...)`; makes me wonder if we would want to port 
> > it there now or later...

I think this optimisation is not necessary here. If it is found that this 
optimisation is important, we can add it later.


- Andrei


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


On Фев. 21, 2020, 12:30 п.п., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72157/
> ---
> 
> (Updated Фев. 21, 2020, 12:30 п.п.)
> 
> 
> Review request for mesos, Andrei Sekretenko, Greg Mann, and Qian Zhang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed reimplementation of `cloexec` from systemd activation code.
> 
> 
> Diffs
> -
> 
>   src/linux/systemd.cpp 9897473115ac0f9809734c109ba412eefd32e59e 
> 
> 
> Diff: https://reviews.apache.org/r/72157/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 72157: Removed reimplementation of `cloexec` from systemd activation code.

[Andrei Budnik]

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

(Updated Фев. 24, 2020, 2:35 п.п.)


Review request for mesos, Andrei Sekretenko, Greg Mann, and Qian Zhang.


Repository: mesos


Description
---

Removed reimplementation of `cloexec` from systemd activation code.


Diffs (updated)
-

  src/linux/systemd.cpp 9897473115ac0f9809734c109ba412eefd32e59e 


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

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


Testing
---


Thanks,

Andrei Budnik

Re: Review Request 72158: Fixed systemd socket activation on old systemd versions.

[Andrei Sekretenko]

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



Do we have a MESOS bug for this? Looks like we should...


src/linux/systemd.cpp
Line 400 (original), 400 (patched)


If this function starts to behave this way to work around older systemds, 
at the bare minimum it should be renamed. Otherwise the code in 
`slave/main.cpp` becomes totally misleading.

Also, this workaround (and, honestly, the whole situation with pre-277 
systemds) looks rather fragile. 
This makes me wonder if there is a simple reliable way to detect systemd 
version (I'm looking at some other code in this file that seems to do this) and 
suppress this workaround for newer versions (and log warning for older 
versions!). 
If there is no simple way, this should probably be left as TODO...

I see at least two options how to avoid misleading readers of 
`slave/main.cpp`.

1) change signature to `listenFdsWithNames(const std::set& names)` 
so that the code in main.cpp looks like 
```
// NOTE: pre-277 systemd versions do not support FileDescriptorName, thus 
we also need to listen on descriptiors with name "unknown"
Try> socketFds =
systemd::socket_activation::listenFdsWithNames({name , "unknown"})
```

2) Rename this function (with added `== "unknown"`) into `listenFds(const 
string& name)` and, optionally, make `listenFds()` with no arguments static.
This way, systemd version check, if we decide to add it, can be introduced 
into this fucntion, instead of going somewhere outside.


- Andrei Sekretenko


On Feb. 21, 2020, 12:30 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72158/
> ---
> 
> (Updated Feb. 21, 2020, 12:30 p.m.)
> 
> 
> Review request for mesos, Andrei Sekretenko, Greg Mann, and Qian Zhang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch fixes the bug when `listenFdsWithName` function returns
> an empty set of file descriptors on pre-227 systemd versions when
> `domain_socket_location` value is not equals to the "systemd:unknown".
> This happens when a user expects a newer version of systemd and
> specifies a "systemd/", but
> the actual systemd version does not support `FileDescriptorName`.
> In this case, `LISTEN_FDNAMES` env variable is not specified,
> so all socket FDs, which are specified by the `LISTEN_FDS`,
> must be used to locate the path to the domain socket.
> 
> 
> Diffs
> -
> 
>   src/linux/systemd.cpp 9897473115ac0f9809734c109ba412eefd32e59e 
> 
> 
> Diff: https://reviews.apache.org/r/72158/diff/1/
> 
> 
> Testing
> ---
> 
> internal CI (including CoreOS)
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 72157: Removed reimplementation of `cloexec` from systemd activation code.

[Andrei Sekretenko]

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


Fix it, then Ship it!





src/linux/systemd.cpp
Lines 31 (patched)


Hmm... including `stout/posix/os.hpp` looks inconsistent with existing code 
- do we really need this include, given that `stout/os.hpp` includes it on 
non-windows?

Alternatively, if you want to be explicit about the origin of 
`os::cloexec()`, then shouldn't that be `stout/os/posix/fcntl.hpp`?



src/linux/systemd.cpp
Line 352 (original)


`cloexec(...)` from stout doesn't have this optimization and always calls 
`fcntl(fd, F_SETFD, ...)`; makes me wonder if we would want to port it there 
now or later...


- Andrei Sekretenko


On Feb. 21, 2020, 12:30 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72157/
> ---
> 
> (Updated Feb. 21, 2020, 12:30 p.m.)
> 
> 
> Review request for mesos, Andrei Sekretenko, Greg Mann, and Qian Zhang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed reimplementation of `cloexec` from systemd activation code.
> 
> 
> Diffs
> -
> 
>   src/linux/systemd.cpp 9897473115ac0f9809734c109ba412eefd32e59e 
> 
> 
> Diff: https://reviews.apache.org/r/72157/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>