Re: Review Request 36282: Remove os environment for docker executor enviornment setup.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36282/#review90802 --- Question about MESOS-2832, but otherwise looks good. src/slave/containerizer/docker.hpp (line 239) https://reviews.apache.org/r/36282/#comment143934 Should executorEnvironment be static as well? src/slave/containerizer/docker.cpp (line 1552) https://reviews.apache.org/r/36282/#comment143931 Filter? How does this cooperate with our fix for MESOS-2832 where `--executor_environment_variables` which lets an admin specify the environment variables that should be passed to an executor? Should we filter even if that flag is set? - Adam B On July 7, 2015, 3:10 p.m., Timothy Chen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36282/ --- (Updated July 7, 2015, 3:10 p.m.) Review request for mesos, Adam B, Benjamin Hindman, and Joerg Schad. Bugs: MESOS-2996 https://issues.apache.org/jira/browse/MESOS-2996 Repository: mesos Description --- Remove os environment when calling executorEnvironment when running docker executors, since all the environment variables will be passed into the container and causes bad behavior such as overriding hostname. Diffs - src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba Diff: https://reviews.apache.org/r/36282/diff/ Testing --- make check Thanks, Timothy Chen
Re: Review Request 36282: Remove os environment for docker executor enviornment setup.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36282/ --- (Updated July 7, 2015, 11:05 p.m.) Review request for mesos, Adam B, Benjamin Hindman, and Joerg Schad. Bugs: MESOS-2996 https://issues.apache.org/jira/browse/MESOS-2996 Repository: mesos Description --- Remove os environment when calling executorEnvironment when running docker executors, since all the environment variables will be passed into the container and causes bad behavior such as overriding hostname. Diffs (updated) - src/slave/containerizer/containerizer.hpp 0ee17e6bc52d1e3acefad6bda3a1b7ba64a8a54b src/slave/containerizer/containerizer.cpp 69dfac04cfd9c388fc908b68ac7abbc14304e621 src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba Diff: https://reviews.apache.org/r/36282/diff/ Testing --- make check Thanks, Timothy Chen
Re: Review Request 36282: Remove os environment for docker executor enviornment setup.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36282/#review90821 --- Updated the review to use a new optional boolean flag. The docker bridge test passes with this as well. Once I have a new ship it then I can merge. - Timothy Chen On July 7, 2015, 11:05 p.m., Timothy Chen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36282/ --- (Updated July 7, 2015, 11:05 p.m.) Review request for mesos, Adam B, Benjamin Hindman, and Joerg Schad. Bugs: MESOS-2996 https://issues.apache.org/jira/browse/MESOS-2996 Repository: mesos Description --- Remove os environment when calling executorEnvironment when running docker executors, since all the environment variables will be passed into the container and causes bad behavior such as overriding hostname. Diffs - src/slave/containerizer/containerizer.hpp 0ee17e6bc52d1e3acefad6bda3a1b7ba64a8a54b src/slave/containerizer/containerizer.cpp 69dfac04cfd9c388fc908b68ac7abbc14304e621 src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba Diff: https://reviews.apache.org/r/36282/diff/ Testing --- make check Thanks, Timothy Chen
Review Request 36282: Remove os environment for docker executor enviornment setup.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36282/ --- Review request for mesos, Benjamin Hindman and Joerg Schad. Bugs: MESOS-2996 https://issues.apache.org/jira/browse/MESOS-2996 Repository: mesos Description --- Remove os environment when calling executorEnvironment when running docker executors, since all the environment variables will be passed into the container and causes bad behavior such as overriding hostname. Diffs - src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba Diff: https://reviews.apache.org/r/36282/diff/ Testing --- make check Thanks, Timothy Chen
Re: Review Request 36282: Remove os environment for docker executor enviornment setup.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36282/#review90796 --- Ship it! Looks good but see the issue below about moving this inside the translation unit. src/slave/containerizer/docker.hpp (line 239) https://reviews.apache.org/r/36282/#comment143920 Let's not expose this in a public header since it's only used internally and instead let's just keep it as a static function inside of src/slave/containerizer/docker.cpp. src/slave/containerizer/docker.cpp (line 1551) https://reviews.apache.org/r/36282/#comment143921 Newline above this please! src/slave/containerizer/docker.cpp (line 1552) https://reviews.apache.org/r/36282/#comment143923 s/the os/the current process/ - Benjamin Hindman On July 7, 2015, 10:10 p.m., Timothy Chen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36282/ --- (Updated July 7, 2015, 10:10 p.m.) Review request for mesos, Adam B, Benjamin Hindman, and Joerg Schad. Bugs: MESOS-2996 https://issues.apache.org/jira/browse/MESOS-2996 Repository: mesos Description --- Remove os environment when calling executorEnvironment when running docker executors, since all the environment variables will be passed into the container and causes bad behavior such as overriding hostname. Diffs - src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba Diff: https://reviews.apache.org/r/36282/diff/ Testing --- make check Thanks, Timothy Chen
Re: Review Request 36282: Remove os environment for docker executor enviornment setup.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36282/#review90812 --- src/slave/containerizer/docker.hpp (line 239) https://reviews.apache.org/r/36282/#comment143952 It's actually being used in the header if you see below. - Timothy Chen On July 7, 2015, 10:10 p.m., Timothy Chen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36282/ --- (Updated July 7, 2015, 10:10 p.m.) Review request for mesos, Adam B, Benjamin Hindman, and Joerg Schad. Bugs: MESOS-2996 https://issues.apache.org/jira/browse/MESOS-2996 Repository: mesos Description --- Remove os environment when calling executorEnvironment when running docker executors, since all the environment variables will be passed into the container and causes bad behavior such as overriding hostname. Diffs - src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba Diff: https://reviews.apache.org/r/36282/diff/ Testing --- make check Thanks, Timothy Chen
Re: Review Request 36282: Remove os environment for docker executor enviornment setup.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36282/#review90814 --- src/slave/containerizer/docker.cpp (line 1544) https://reviews.apache.org/r/36282/#comment143955 Wouldn't it be easier --especially considering Adam's comment about MESOS-2832 and the --executor_environment_variable --- to add an additional (optional) flag to executorEnvironment intialize=true. if explicitly set to false (such as in this case, we would not initialize environment with os::environment() - Joerg Schad On July 7, 2015, 10:10 p.m., Timothy Chen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36282/ --- (Updated July 7, 2015, 10:10 p.m.) Review request for mesos, Adam B, Benjamin Hindman, and Joerg Schad. Bugs: MESOS-2996 https://issues.apache.org/jira/browse/MESOS-2996 Repository: mesos Description --- Remove os environment when calling executorEnvironment when running docker executors, since all the environment variables will be passed into the container and causes bad behavior such as overriding hostname. Diffs - src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba Diff: https://reviews.apache.org/r/36282/diff/ Testing --- make check Thanks, Timothy Chen
Re: Review Request 36282: Remove os environment for docker executor enviornment setup.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36282/#review90826 --- Ship it! Barring Adams comment. src/slave/containerizer/containerizer.hpp (line 152) https://reviews.apache.org/r/36282/#comment143967 Shouldnt it be called `includeOSEnvironment` instead? Bit unsure right now but please consider that :) - Till Toenshoff On July 7, 2015, 11:05 p.m., Timothy Chen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36282/ --- (Updated July 7, 2015, 11:05 p.m.) Review request for mesos, Adam B, Benjamin Hindman, and Joerg Schad. Bugs: MESOS-2996 https://issues.apache.org/jira/browse/MESOS-2996 Repository: mesos Description --- Remove os environment when calling executorEnvironment when running docker executors, since all the environment variables will be passed into the container and causes bad behavior such as overriding hostname. Diffs - src/slave/containerizer/containerizer.hpp 0ee17e6bc52d1e3acefad6bda3a1b7ba64a8a54b src/slave/containerizer/containerizer.cpp 69dfac04cfd9c388fc908b68ac7abbc14304e621 src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba Diff: https://reviews.apache.org/r/36282/diff/ Testing --- make check Thanks, Timothy Chen
Re: Review Request 36282: Remove os environment for docker executor enviornment setup.
On July 7, 2015, 10:58 p.m., Joerg Schad wrote: src/slave/containerizer/docker.cpp, line 1544 https://reviews.apache.org/r/36282/diff/1/?file=1001811#file1001811line1544 Wouldn't it be easier --especially considering Adam's comment about MESOS-2832 and the --executor_environment_variable --- to add an additional (optional) flag to executorEnvironment intialize=true. if explicitly set to false (such as in this case, we would not initialize environment with os::environment() I like this approach better as well - seems much cleaner to me. - Till --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36282/#review90814 --- On July 7, 2015, 10:10 p.m., Timothy Chen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36282/ --- (Updated July 7, 2015, 10:10 p.m.) Review request for mesos, Adam B, Benjamin Hindman, and Joerg Schad. Bugs: MESOS-2996 https://issues.apache.org/jira/browse/MESOS-2996 Repository: mesos Description --- Remove os environment when calling executorEnvironment when running docker executors, since all the environment variables will be passed into the container and causes bad behavior such as overriding hostname. Diffs - src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba Diff: https://reviews.apache.org/r/36282/diff/ Testing --- make check Thanks, Timothy Chen
Re: Review Request 36282: Remove os environment for docker executor enviornment setup.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36282/#review90811 --- src/slave/containerizer/docker.cpp (line 1552) https://reviews.apache.org/r/36282/#comment143951 We have to filter even if the flag is set, since we're actually including more than what the flags has specified. What's interesting though is that we might unintentionally filter env variables if it's set from the flags and it's also included already in the os environment with the same key and value. I can further check with the flags.executor_environment_variables so I don't filter a env variable that's also set in there, but I think it might be a lot easier to introduce a optional boolean flag (includeOsEnvironment) on executorEnvironment method to be able to not include os::environment(), and it's set to true by default? Then all the DockerContainerizer has to do is to set that extra flag to false. What you think Ben? - Timothy Chen On July 7, 2015, 10:10 p.m., Timothy Chen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36282/ --- (Updated July 7, 2015, 10:10 p.m.) Review request for mesos, Adam B, Benjamin Hindman, and Joerg Schad. Bugs: MESOS-2996 https://issues.apache.org/jira/browse/MESOS-2996 Repository: mesos Description --- Remove os environment when calling executorEnvironment when running docker executors, since all the environment variables will be passed into the container and causes bad behavior such as overriding hostname. Diffs - src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba Diff: https://reviews.apache.org/r/36282/diff/ Testing --- make check Thanks, Timothy Chen
Re: Review Request 36282: Remove os environment for docker executor enviornment setup.
On July 7, 2015, 10:58 p.m., Joerg Schad wrote: src/slave/containerizer/docker.cpp, line 1544 https://reviews.apache.org/r/36282/diff/1/?file=1001811#file1001811line1544 Wouldn't it be easier --especially considering Adam's comment about MESOS-2832 and the --executor_environment_variable --- to add an additional (optional) flag to executorEnvironment intialize=true. if explicitly set to false (such as in this case, we would not initialize environment with os::environment() Till Toenshoff wrote: I like this approach better as well - seems much cleaner to me. flag - parameter... - Joerg --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36282/#review90814 --- On July 7, 2015, 10:10 p.m., Timothy Chen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36282/ --- (Updated July 7, 2015, 10:10 p.m.) Review request for mesos, Adam B, Benjamin Hindman, and Joerg Schad. Bugs: MESOS-2996 https://issues.apache.org/jira/browse/MESOS-2996 Repository: mesos Description --- Remove os environment when calling executorEnvironment when running docker executors, since all the environment variables will be passed into the container and causes bad behavior such as overriding hostname. Diffs - src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba Diff: https://reviews.apache.org/r/36282/diff/ Testing --- make check Thanks, Timothy Chen
Re: Review Request 36282: Remove os environment for docker executor enviornment setup.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36282/#review90822 --- Ship it! Looks like a simpler, more generic fix. Maybe external_containerizer will want to use includeOsEnvironment flag too. src/slave/containerizer/containerizer.cpp (lines 248 - 253) https://reviews.apache.org/r/36282/#comment143962 Seems like one of these could be an else if, since they both wipe `environment` and start from scratch. I would probably start with `if(flags.executor_environment_variables)` then `else if(includeOsEnvironment)`. - Adam B On July 7, 2015, 4:05 p.m., Timothy Chen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36282/ --- (Updated July 7, 2015, 4:05 p.m.) Review request for mesos, Adam B, Benjamin Hindman, and Joerg Schad. Bugs: MESOS-2996 https://issues.apache.org/jira/browse/MESOS-2996 Repository: mesos Description --- Remove os environment when calling executorEnvironment when running docker executors, since all the environment variables will be passed into the container and causes bad behavior such as overriding hostname. Diffs - src/slave/containerizer/containerizer.hpp 0ee17e6bc52d1e3acefad6bda3a1b7ba64a8a54b src/slave/containerizer/containerizer.cpp 69dfac04cfd9c388fc908b68ac7abbc14304e621 src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba Diff: https://reviews.apache.org/r/36282/diff/ Testing --- make check Thanks, Timothy Chen
