Re: Review Request 52154: Avoided modifying process environment.

[Benjamin Bannier]

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

(Updated Oct. 7, 2016, 2:55 p.m.)


Review request for mesos, Joris Van Remoortere and Till Toenshoff.


Changes
---

Rebased.


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


Repository: mesos


Description (updated)
---

This code modified the process environment in order to support upgrading
on the fly from old-style libprocess SSL variables `SSL_` to
`LIBPROCESS_SSL_`. Modifying the process environment at this point is
not safe as other actors might concurrently read out that same
environment.

Instead avoid changing the process environment altogether since flags
can just as well be read from a map.


Diffs (updated)
-

  3rdparty/libprocess/src/openssl.cpp d73abbdeeff0f69e79df94f1f128b922397cd884 

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


Testing
---

Tested on various platforms in internal CI.


Thanks,

Benjamin Bannier

Re: Review Request 52154: Avoided modifying process environment.

[Benjamin Bannier]

> On Sept. 23, 2016, 1:31 a.m., Joris Van Remoortere wrote:
> > Why this solution as opposed to ensuring we initialize at the beginning of 
> > processes like we do with `process::initialize()`?
> 
> Benjamin Bannier wrote:
> Fixing the code in this spot is slightly easier as `openssl::initialize` 
> would
> need to be called from a number of sites. Having it being called as part 
> of
> `process::initialize` would be a viable option, but would make testing 
> slightly
> harder.

I filed https://issues.apache.org/jira/browse/MESOS-6328 to make openssl 
initialization eager.


- Benjamin


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


On Sept. 26, 2016, 6:16 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52154/
> ---
> 
> (Updated Sept. 26, 2016, 6:16 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Till Toenshoff.
> 
> 
> Bugs: MESOS-6216
> https://issues.apache.org/jira/browse/MESOS-6216
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This code modified the process environment in order to support upgrading
> on the fly from old-style libprocess SSL variables `SSL_` to
> `LIBPROCESS_SSL_`. Modifying the process environment at this point is
> not safe as other actors might concurrently read out that same
> environment.
> 
> Instead avoid changing the process environment altogether since flags
> can just as well be read from a map.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/openssl.cpp 
> c09cdc89509e4e4ca4c8a0f4fb0a57156a3a6091 
> 
> Diff: https://reviews.apache.org/r/52154/diff/
> 
> 
> Testing
> ---
> 
> Tested on various platforms in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 52154: Avoided modifying process environment.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [52256, 52154]

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 Sept. 26, 2016, 4:16 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52154/
> ---
> 
> (Updated Sept. 26, 2016, 4:16 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Till Toenshoff.
> 
> 
> Bugs: MESOS-6216
> https://issues.apache.org/jira/browse/MESOS-6216
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This code modified the process environment in order to support upgrading
> on the fly from old-style libprocess SSL variables `SSL_` to
> `LIBPROCESS_SSL_`. Modifying the process environment at this point is
> not safe as other actors might concurrently read out that same
> environment.
> 
> Instead avoid changing the process environment altogether since flags
> can just as well be read from a map.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/openssl.cpp 
> c09cdc89509e4e4ca4c8a0f4fb0a57156a3a6091 
> 
> Diff: https://reviews.apache.org/r/52154/diff/
> 
> 
> Testing
> ---
> 
> Tested on various platforms in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 52154: Avoided modifying process environment.

[Benjamin Bannier]

> On Sept. 22, 2016, 2:15 p.m., Till Toenshoff wrote:
> > 3rdparty/libprocess/src/openssl.cpp, lines 317-318
> > 
> >
> > Can we have a short comment here explaining that `load` overload? Does 
> > it still even try to check the process environment or is it totally relying 
> > on that map being a complete environment view?

Thanks for bringing this up. I went and looked more carefully at how
`FlagsBase::extract` is implemented and found that it actually does some
perform some slightly specific magic where the case of environment variables
does partly ignore case. I prefixed this review with
https://reviews.apache.org/r/52256/ which exposes `FlagsBase::extract` so we
can avoid reimplementing this code here.


- Benjamin


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


On Sept. 26, 2016, 6:16 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52154/
> ---
> 
> (Updated Sept. 26, 2016, 6:16 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Till Toenshoff.
> 
> 
> Bugs: MESOS-6216
> https://issues.apache.org/jira/browse/MESOS-6216
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This code modified the process environment in order to support upgrading
> on the fly from old-style libprocess SSL variables `SSL_` to
> `LIBPROCESS_SSL_`. Modifying the process environment at this point is
> not safe as other actors might concurrently read out that same
> environment.
> 
> Instead avoid changing the process environment altogether since flags
> can just as well be read from a map.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/openssl.cpp 
> c09cdc89509e4e4ca4c8a0f4fb0a57156a3a6091 
> 
> Diff: https://reviews.apache.org/r/52154/diff/
> 
> 
> Testing
> ---
> 
> Tested on various platforms in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 52154: Avoided modifying process environment.

[Benjamin Bannier]

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

(Updated Sept. 26, 2016, 6:16 p.m.)


Review request for mesos, Joris Van Remoortere and Till Toenshoff.


Changes
---

Fixed to correctly reproduce overriding behavior of old implementation when 
variables with different case were found, e.g., `SSL_foo` and 
`LIBPROCESS_SSL_Foo` would attempt to update the same variable `foo` and should 
contain compatible values, or `SSL_foo` would likely be overriden by `SSL_Foo` 
(at least in the `C` locale), which would e.g., need to be compatible with 
`LIBPROCESS_SSL_FOO`.


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


Repository: mesos


Description
---

This code modified the process environment in order to support upgrading
on the fly from old-style libprocess SSL variables `SSL_` to
`LIBPROCESS_SSL_`. Modifying the process environment at this point is
not safe as other actors might concurrently read out that same
environment.

Instead avoid changing the process environment altogether since flags
can just as well be read from a map.


Diffs (updated)
-

  3rdparty/libprocess/src/openssl.cpp c09cdc89509e4e4ca4c8a0f4fb0a57156a3a6091 

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


Testing
---

Tested on various platforms in internal CI.


Thanks,

Benjamin Bannier

Re: Review Request 52154: Avoided modifying process environment.

[Joris Van Remoortere]

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



Why this solution as opposed to ensuring we initialize at the beginning of 
processes like we do with `process::initialize()`?

- Joris Van Remoortere


On Sept. 22, 2016, 12:06 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52154/
> ---
> 
> (Updated Sept. 22, 2016, 12:06 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Till Toenshoff.
> 
> 
> Bugs: MESOS-6216
> https://issues.apache.org/jira/browse/MESOS-6216
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This code modified the process environment in order to support upgrading
> on the fly from old-style libprocess SSL variables `SSL_` to
> `LIBPROCESS_SSL_`. Modifying the process environment at this point is
> not safe as other actors might concurrently read out that same
> environment.
> 
> Instead avoid changing the process environment altogether since flags
> can just as well be read from a map.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/openssl.cpp 
> c09cdc89509e4e4ca4c8a0f4fb0a57156a3a6091 
>   src/master/allocator/mesos/hierarchical.cpp 
> 3f51f4194c1ba7c1e4f08c3dd623281ca5754d39 
>   src/master/master.hpp 48011eabda03986df3dfac124506645a398eaff4 
> 
> Diff: https://reviews.apache.org/r/52154/diff/
> 
> 
> Testing
> ---
> 
> Tested on various platforms in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 52154: Avoided modifying process environment.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [52154]

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 Sept. 22, 2016, 12:06 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52154/
> ---
> 
> (Updated Sept. 22, 2016, 12:06 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Till Toenshoff.
> 
> 
> Bugs: MESOS-6216
> https://issues.apache.org/jira/browse/MESOS-6216
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This code modified the process environment in order to support upgrading
> on the fly from old-style libprocess SSL variables `SSL_` to
> `LIBPROCESS_SSL_`. Modifying the process environment at this point is
> not safe as other actors might concurrently read out that same
> environment.
> 
> Instead avoid changing the process environment altogether since flags
> can just as well be read from a map.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/openssl.cpp 
> c09cdc89509e4e4ca4c8a0f4fb0a57156a3a6091 
>   src/master/allocator/mesos/hierarchical.cpp 
> 3f51f4194c1ba7c1e4f08c3dd623281ca5754d39 
>   src/master/master.hpp 48011eabda03986df3dfac124506645a398eaff4 
> 
> Diff: https://reviews.apache.org/r/52154/diff/
> 
> 
> Testing
> ---
> 
> Tested on various platforms in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 52154: Avoided modifying process environment.

[Till Toenshoff]

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


Fix it, then Ship it!




Thanks for tackling that one Benjamin!


3rdparty/libprocess/src/openssl.cpp (lines 317 - 318)


Can we have a short comment here explaining that `load` overload? Does it 
still even try to check the process environment or is it totally relying on 
that map being a complete environment view?



src/master/allocator/mesos/hierarchical.cpp 


Rebasing?


- Till Toenshoff


On Sept. 22, 2016, 12:06 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52154/
> ---
> 
> (Updated Sept. 22, 2016, 12:06 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Till Toenshoff.
> 
> 
> Bugs: MESOS-6216
> https://issues.apache.org/jira/browse/MESOS-6216
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This code modified the process environment in order to support upgrading
> on the fly from old-style libprocess SSL variables `SSL_` to
> `LIBPROCESS_SSL_`. Modifying the process environment at this point is
> not safe as other actors might concurrently read out that same
> environment.
> 
> Instead avoid changing the process environment altogether since flags
> can just as well be read from a map.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/openssl.cpp 
> c09cdc89509e4e4ca4c8a0f4fb0a57156a3a6091 
>   src/master/allocator/mesos/hierarchical.cpp 
> 3f51f4194c1ba7c1e4f08c3dd623281ca5754d39 
>   src/master/master.hpp 48011eabda03986df3dfac124506645a398eaff4 
> 
> Diff: https://reviews.apache.org/r/52154/diff/
> 
> 
> Testing
> ---
> 
> Tested on various platforms in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>