Re: Review Request 55037: Added default PATH value in `launch.cpp` for Windows case.

[Alex Clemmer]

> On Jan. 21, 2017, 12:30 a.m., Joseph Wu wrote:
> > src/slave/containerizer/mesos/launch.cpp, line 53
> > 
> >
> > This looks like an erroneous addition.

This doesn't appear in my source file, and when I push it doesn't go away. I 
think it must be a RB error.


- Alex


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


On Jan. 19, 2017, 2:13 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55037/
> ---
> 
> (Updated Jan. 19, 2017, 2:13 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, John Kordich, 
> and Joseph Wu.
> 
> 
> Bugs: MESOS-6839
> https://issues.apache.org/jira/browse/MESOS-6839
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, when the containerizer launches a process with the POSIX
> launcher, it will check if the `$PATH` environment variable (or `%PATH%`
> in the case of Windows) is set in the `LaunchInfo`. If it is not, we
> supply a default path. Unfortunately, this default path is specific to
> POSIX. In many of our tests, this causes many of our tests to be unable
> to find Windows-standard executables like `ping`, and subsequently fail.
> 
> This commit will introduce a function, `defaultPath` that returns a
> sensible default path for both POSIX and Windows. Since the Windows
> implementation of this depends on the configuration of the host running
> the containerizer (rather than, say, the one creating the `TaskInfo`),
> we choose to implement this in `launch.cpp` instead of Stout, where a
> user could mistakenly call it and expect the same output on all hosts.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.cpp 7a8a7271b54af0b4dcdae7a2aa8a90a8a7d05fd6 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> ea91c71fdfac48a2fc1d31a0ee088a73244be367 
>   
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
>  ab4b88acddc7503e16e3730320df39a2f104539a 
>   src/slave/containerizer/mesos/launch.cpp 
> e482ab8bdfc358f695b87cda72ca59fb64cd8c4d 
> 
> Diff: https://reviews.apache.org/r/55037/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 55037: Added default PATH value in `launch.cpp` for Windows case.

[Joseph Wu]

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


Ship it!




The review description here is now out-of-date.

Something like this would be more appropriate:
```
Used `os::host_default_path()` in several locations.

Several of the codepaths that eventually launch a subprocess will check
for a `PATH` environment variable.  If this value is not set, it uses
a default value.

This commit replaces these hard-coded default values with the newly
added stout function `host_default_path`.  The return value of 
this function differs depending on the host (and the OS), meaning that
the return value of `host_default_path` may or may not be portable.
```

I can remove the below before committing.


src/slave/containerizer/mesos/launch.cpp (line 53)


This looks like an erroneous addition.


- Joseph Wu


On Jan. 18, 2017, 6:13 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55037/
> ---
> 
> (Updated Jan. 18, 2017, 6:13 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, John Kordich, 
> and Joseph Wu.
> 
> 
> Bugs: MESOS-6839
> https://issues.apache.org/jira/browse/MESOS-6839
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, when the containerizer launches a process with the POSIX
> launcher, it will check if the `$PATH` environment variable (or `%PATH%`
> in the case of Windows) is set in the `LaunchInfo`. If it is not, we
> supply a default path. Unfortunately, this default path is specific to
> POSIX. In many of our tests, this causes many of our tests to be unable
> to find Windows-standard executables like `ping`, and subsequently fail.
> 
> This commit will introduce a function, `defaultPath` that returns a
> sensible default path for both POSIX and Windows. Since the Windows
> implementation of this depends on the configuration of the host running
> the containerizer (rather than, say, the one creating the `TaskInfo`),
> we choose to implement this in `launch.cpp` instead of Stout, where a
> user could mistakenly call it and expect the same output on all hosts.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.cpp 7a8a7271b54af0b4dcdae7a2aa8a90a8a7d05fd6 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> ea91c71fdfac48a2fc1d31a0ee088a73244be367 
>   
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
>  ab4b88acddc7503e16e3730320df39a2f104539a 
>   src/slave/containerizer/mesos/launch.cpp 
> e482ab8bdfc358f695b87cda72ca59fb64cd8c4d 
> 
> Diff: https://reviews.apache.org/r/55037/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 55037: Added default PATH value in `launch.cpp` for Windows case.

[Alex Clemmer]

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

(Updated Jan. 19, 2017, 2:13 a.m.)


Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, John Kordich, 
and Joseph Wu.


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


Repository: mesos


Description
---

Currently, when the containerizer launches a process with the POSIX
launcher, it will check if the `$PATH` environment variable (or `%PATH%`
in the case of Windows) is set in the `LaunchInfo`. If it is not, we
supply a default path. Unfortunately, this default path is specific to
POSIX. In many of our tests, this causes many of our tests to be unable
to find Windows-standard executables like `ping`, and subsequently fail.

This commit will introduce a function, `defaultPath` that returns a
sensible default path for both POSIX and Windows. Since the Windows
implementation of this depends on the configuration of the host running
the containerizer (rather than, say, the one creating the `TaskInfo`),
we choose to implement this in `launch.cpp` instead of Stout, where a
user could mistakenly call it and expect the same output on all hosts.


Diffs (updated)
-

  src/slave/containerizer/docker.cpp 7a8a7271b54af0b4dcdae7a2aa8a90a8a7d05fd6 
  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
ea91c71fdfac48a2fc1d31a0ee088a73244be367 
  
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
 ab4b88acddc7503e16e3730320df39a2f104539a 
  src/slave/containerizer/mesos/launch.cpp 
e482ab8bdfc358f695b87cda72ca59fb64cd8c4d 

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


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 55037: Added default PATH value in `launch.cpp` for Windows case.

[Alex Clemmer]

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

(Updated Jan. 19, 2017, 2:09 a.m.)


Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, John Kordich, 
and Joseph Wu.


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


Repository: mesos


Description
---

Currently, when the containerizer launches a process with the POSIX
launcher, it will check if the `$PATH` environment variable (or `%PATH%`
in the case of Windows) is set in the `LaunchInfo`. If it is not, we
supply a default path. Unfortunately, this default path is specific to
POSIX. In many of our tests, this causes many of our tests to be unable
to find Windows-standard executables like `ping`, and subsequently fail.

This commit will introduce a function, `defaultPath` that returns a
sensible default path for both POSIX and Windows. Since the Windows
implementation of this depends on the configuration of the host running
the containerizer (rather than, say, the one creating the `TaskInfo`),
we choose to implement this in `launch.cpp` instead of Stout, where a
user could mistakenly call it and expect the same output on all hosts.


Diffs (updated)
-

  src/slave/containerizer/docker.cpp 7a8a7271b54af0b4dcdae7a2aa8a90a8a7d05fd6 
  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
ea91c71fdfac48a2fc1d31a0ee088a73244be367 
  
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
 ab4b88acddc7503e16e3730320df39a2f104539a 
  src/slave/containerizer/mesos/launch.cpp 
e482ab8bdfc358f695b87cda72ca59fb64cd8c4d 
  src/slave/containerizer/mesos/utils.hpp 
a54106dc4893bb222f42ede936ac9029e817faf9 

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


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 55037: Added default PATH value in `launch.cpp` for Windows case.

[Alex Clemmer]

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

(Updated Jan. 19, 2017, 2:06 a.m.)


Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, John Kordich, 
and Joseph Wu.


Changes
---

Addressed Joseph's comments.


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


Repository: mesos


Description
---

Currently, when the containerizer launches a process with the POSIX
launcher, it will check if the `$PATH` environment variable (or `%PATH%`
in the case of Windows) is set in the `LaunchInfo`. If it is not, we
supply a default path. Unfortunately, this default path is specific to
POSIX. In many of our tests, this causes many of our tests to be unable
to find Windows-standard executables like `ping`, and subsequently fail.

This commit will introduce a function, `defaultPath` that returns a
sensible default path for both POSIX and Windows. Since the Windows
implementation of this depends on the configuration of the host running
the containerizer (rather than, say, the one creating the `TaskInfo`),
we choose to implement this in `launch.cpp` instead of Stout, where a
user could mistakenly call it and expect the same output on all hosts.


Diffs (updated)
-

  src/slave/containerizer/docker.cpp 7a8a7271b54af0b4dcdae7a2aa8a90a8a7d05fd6 
  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
ea91c71fdfac48a2fc1d31a0ee088a73244be367 
  
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
 ab4b88acddc7503e16e3730320df39a2f104539a 
  src/slave/containerizer/mesos/launch.cpp 
e482ab8bdfc358f695b87cda72ca59fb64cd8c4d 
  src/slave/containerizer/mesos/utils.hpp 
a54106dc4893bb222f42ede936ac9029e817faf9 

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


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 55037: Added default PATH value in `launch.cpp` for Windows case.

[Joseph Wu]

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



This helper method belongs in stout, as it is precisely something which may 
give different results on different hosts.  (i.e. `os::var`)

Also, there are a couple places that could also use the function:
* src/slave/containerizer/docker.cpp
* 
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp

- Joseph Wu


On Jan. 15, 2017, 2:17 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55037/
> ---
> 
> (Updated Jan. 15, 2017, 2:17 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, John Kordich, 
> and Joseph Wu.
> 
> 
> Bugs: MESOS-6839
> https://issues.apache.org/jira/browse/MESOS-6839
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, when the containerizer launches a process with the POSIX
> launcher, it will check if the `$PATH` environment variable (or `%PATH%`
> in the case of Windows) is set in the `LaunchInfo`. If it is not, we
> supply a default path. Unfortunately, this default path is specific to
> POSIX. In many of our tests, this causes many of our tests to be unable
> to find Windows-standard executables like `ping`, and subsequently fail.
> 
> This commit will introduce a function, `defaultPath` that returns a
> sensible default path for both POSIX and Windows. Since the Windows
> implementation of this depends on the configuration of the host running
> the containerizer (rather than, say, the one creating the `TaskInfo`),
> we choose to implement this in `launch.cpp` instead of Stout, where a
> user could mistakenly call it and expect the same output on all hosts.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> ea91c71fdfac48a2fc1d31a0ee088a73244be367 
>   src/slave/containerizer/mesos/launch.cpp 
> e482ab8bdfc358f695b87cda72ca59fb64cd8c4d 
>   src/slave/containerizer/mesos/utils.hpp 
> a54106dc4893bb222f42ede936ac9029e817faf9 
> 
> Diff: https://reviews.apache.org/r/55037/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 55037: Added default PATH value in `launch.cpp` for Windows case.

[Alex Clemmer]

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

(Updated Jan. 15, 2017, 10:17 a.m.)


Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, John Kordich, 
and Joseph Wu.


Changes
---

Add headers, add qualified namespaces.


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


Repository: mesos


Description
---

Currently, when the containerizer launches a process with the POSIX
launcher, it will check if the `$PATH` environment variable (or `%PATH%`
in the case of Windows) is set in the `LaunchInfo`. If it is not, we
supply a default path. Unfortunately, this default path is specific to
POSIX. In many of our tests, this causes many of our tests to be unable
to find Windows-standard executables like `ping`, and subsequently fail.

This commit will introduce a function, `defaultPath` that returns a
sensible default path for both POSIX and Windows. Since the Windows
implementation of this depends on the configuration of the host running
the containerizer (rather than, say, the one creating the `TaskInfo`),
we choose to implement this in `launch.cpp` instead of Stout, where a
user could mistakenly call it and expect the same output on all hosts.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
ea91c71fdfac48a2fc1d31a0ee088a73244be367 
  src/slave/containerizer/mesos/launch.cpp 
e482ab8bdfc358f695b87cda72ca59fb64cd8c4d 
  src/slave/containerizer/mesos/utils.hpp 
a54106dc4893bb222f42ede936ac9029e817faf9 

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


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 55037: Added default PATH value in `launch.cpp` for Windows case.

[Alex Clemmer]

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

(Updated Jan. 15, 2017, 9:22 a.m.)


Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, John Kordich, 
and Joseph Wu.


Changes
---

Moved `defaultPath` location around, again.


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


Repository: mesos


Description
---

Currently, when the containerizer launches a process with the POSIX
launcher, it will check if the `$PATH` environment variable (or `%PATH%`
in the case of Windows) is set in the `LaunchInfo`. If it is not, we
supply a default path. Unfortunately, this default path is specific to
POSIX. In many of our tests, this causes many of our tests to be unable
to find Windows-standard executables like `ping`, and subsequently fail.

This commit will introduce a function, `defaultPath` that returns a
sensible default path for both POSIX and Windows. Since the Windows
implementation of this depends on the configuration of the host running
the containerizer (rather than, say, the one creating the `TaskInfo`),
we choose to implement this in `launch.cpp` instead of Stout, where a
user could mistakenly call it and expect the same output on all hosts.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
ea91c71fdfac48a2fc1d31a0ee088a73244be367 
  src/slave/containerizer/mesos/launch.cpp 
e482ab8bdfc358f695b87cda72ca59fb64cd8c4d 
  src/slave/containerizer/mesos/utils.hpp 
a54106dc4893bb222f42ede936ac9029e817faf9 

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


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 55037: Added default PATH value in `launch.cpp` for Windows case.

[Alex Clemmer]

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

(Updated Jan. 15, 2017, 9:02 a.m.)


Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, John Kordich, 
and Joseph Wu.


Changes
---

Fix typos in commit message.


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


Repository: mesos


Description (updated)
---

Currently, when the containerizer launches a process with the POSIX
launcher, it will check if the `$PATH` environment variable (or `%PATH%`
in the case of Windows) is set in the `LaunchInfo`. If it is not, we
supply a default path. Unfortunately, this default path is specific to
POSIX. In many of our tests, this causes many of our tests to be unable
to find Windows-standard executables like `ping`, and subsequently fail.

This commit will introduce a function, `defaultPath` that returns a
sensible default path for both POSIX and Windows. Since the Windows
implementation of this depends on the configuration of the host running
the containerizer (rather than, say, the one creating the `TaskInfo`),
we choose to implement this in `launch.cpp` instead of Stout, where a
user could mistakenly call it and expect the same output on all hosts.


Diffs
-

  src/slave/containerizer/mesos/containerizer.hpp 
9fcad933e1ec3b52d4dc366ba80d282deea04c0b 
  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
ea91c71fdfac48a2fc1d31a0ee088a73244be367 
  src/slave/containerizer/mesos/launch.cpp 
e482ab8bdfc358f695b87cda72ca59fb64cd8c4d 

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


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 55037: Added default PATH value in `launch.cpp` for Windows case.

[Alex Clemmer]

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

(Updated Jan. 15, 2017, 9 a.m.)


Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, John Kordich, 
and Joseph Wu.


Changes
---

Address Till's comments.


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


Repository: mesos


Description
---

Currently, when the containerizer launches a process with the POSIX
launcher, it will check if the `$PATH` environment variable (or `%PATH%`
in the case of Windows is set in the `LaunchInfo`. If it is not, we
supply a default path. Unfortunately, this default path is specific to
POSIX. In many of our tests, this causes many of our tests to be unable
to find Windows-standard executables like `ping`, and subsequently fail.

This commit will introduce a function, `default_path` that returns a
sensible default path for both POSIX and Windows. Since the Windows
implementation of this depends on the configuration of the host running
the containerizer (rather than, say, the one creating the `TaskInfo`),
we choose to implement this in `launch.cpp` instead of Stout, where a
user could mistakenly call it and expect the same output on all hosts.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.hpp 
9fcad933e1ec3b52d4dc366ba80d282deea04c0b 
  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
ea91c71fdfac48a2fc1d31a0ee088a73244be367 
  src/slave/containerizer/mesos/launch.cpp 
e482ab8bdfc358f695b87cda72ca59fb64cd8c4d 

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


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 55037: Added default PATH value in `launch.cpp` for Windows case.

[Alex Clemmer]

> On Dec. 27, 2016, 2:12 p.m., Till Toenshoff wrote:
> > src/slave/containerizer/mesos/launch.cpp, line 148
> > 
> >
> > By not adding `syswow64` we are excluding 32bit runnables, is this 
> > intentional and documented? Is this still a thing on windows?
> 
> Joseph Wu wrote:
> Actually, in the current implementation of subprocess on Windows, any 
> user-specified environment variables that conflict with system environment 
> variables will be overridden at subprocess launch.  This means you can 
> specify a `PATH=nowhere` and we'll just ignore it.
> 
> At some point, we may loosen this behavior, as we only did this because 
> the test coverage of the Windows code at the time was nil, and a bunch of 
> other changes kept breaking the environment variables on Windows :)

Actually, the issue is that I'm not sure how to get that folder name without 
depending on the Shell API. (Not a Windows expert, let me know if you have a 
good way to do this.) We are very careful not to depend on the Shell because 
it's not available on Server Nano.

I'll drop it for now, but I'd be quite happy to add this if there was a 
suggestion for doing it that worked around this constraint.


- Alex


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


On Dec. 26, 2016, 9:53 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55037/
> ---
> 
> (Updated Dec. 26, 2016, 9:53 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, John Kordich, 
> and Joseph Wu.
> 
> 
> Bugs: MESOS-6839
> https://issues.apache.org/jira/browse/MESOS-6839
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, when the containerizer launches a process with the POSIX
> launcher, it will check if the `$PATH` environment variable (or `%PATH%`
> in the case of Windows is set in the `LaunchInfo`. If it is not, we
> supply a default path. Unfortunately, this default path is specific to
> POSIX. In many of our tests, this causes many of our tests to be unable
> to find Windows-standard executables like `ping`, and subsequently fail.
> 
> This commit will introduce a function, `default_path` that returns a
> sensible default path for both POSIX and Windows. Since the Windows
> implementation of this depends on the configuration of the host running
> the containerizer (rather than, say, the one creating the `TaskInfo`),
> we choose to implement this in `launch.cpp` instead of Stout, where a
> user could mistakenly call it and expect the same output on all hosts.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/launch.cpp 
> e482ab8bdfc358f695b87cda72ca59fb64cd8c4d 
> 
> Diff: https://reviews.apache.org/r/55037/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 55037: Added default PATH value in `launch.cpp` for Windows case.

[Joseph Wu]

> On Dec. 27, 2016, 6:12 a.m., Till Toenshoff wrote:
> > src/slave/containerizer/mesos/launch.cpp, line 148
> > 
> >
> > By not adding `syswow64` we are excluding 32bit runnables, is this 
> > intentional and documented? Is this still a thing on windows?

Actually, in the current implementation of subprocess on Windows, any 
user-specified environment variables that conflict with system environment 
variables will be overridden at subprocess launch.  This means you can specify 
a `PATH=nowhere` and we'll just ignore it.

At some point, we may loosen this behavior, as we only did this because the 
test coverage of the Windows code at the time was nil, and a bunch of other 
changes kept breaking the environment variables on Windows :)


- Joseph


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


On Dec. 26, 2016, 1:53 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55037/
> ---
> 
> (Updated Dec. 26, 2016, 1:53 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, John Kordich, 
> and Joseph Wu.
> 
> 
> Bugs: MESOS-6839
> https://issues.apache.org/jira/browse/MESOS-6839
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, when the containerizer launches a process with the POSIX
> launcher, it will check if the `$PATH` environment variable (or `%PATH%`
> in the case of Windows is set in the `LaunchInfo`. If it is not, we
> supply a default path. Unfortunately, this default path is specific to
> POSIX. In many of our tests, this causes many of our tests to be unable
> to find Windows-standard executables like `ping`, and subsequently fail.
> 
> This commit will introduce a function, `default_path` that returns a
> sensible default path for both POSIX and Windows. Since the Windows
> implementation of this depends on the configuration of the host running
> the containerizer (rather than, say, the one creating the `TaskInfo`),
> we choose to implement this in `launch.cpp` instead of Stout, where a
> user could mistakenly call it and expect the same output on all hosts.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/launch.cpp 
> e482ab8bdfc358f695b87cda72ca59fb64cd8c4d 
> 
> Diff: https://reviews.apache.org/r/55037/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 55037: Added default PATH value in `launch.cpp` for Windows case.

[Till Toenshoff]

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


Fix it, then Ship it!




While the below might never be implemented for Windows I think we should still 
unify them and switch all PATH defaults to the new function.

https://github.com/apache/mesos/blob/master/src/slave/containerizer/docker.cpp#L1408
https://github.com/apache/mesos/blob/master/src/slave/containerizer/mesos/isolators/network/cni/cni.cpp#L1118
https://github.com/apache/mesos/blob/master/src/slave/containerizer/mesos/isolators/network/cni/cni.cpp#L1502


src/slave/containerizer/mesos/launch.cpp (line 141)


s/off/of/ ?



src/slave/containerizer/mesos/launch.cpp (line 148)


By not adding `syswow64` we are excluding 32bit runnables, is this 
intentional and documented? Is this still a thing on windows?


- Till Toenshoff


On Dec. 26, 2016, 9:53 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55037/
> ---
> 
> (Updated Dec. 26, 2016, 9:53 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, John Kordich, 
> and Joseph Wu.
> 
> 
> Bugs: MESOS-6839
> https://issues.apache.org/jira/browse/MESOS-6839
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, when the containerizer launches a process with the POSIX
> launcher, it will check if the `$PATH` environment variable (or `%PATH%`
> in the case of Windows is set in the `LaunchInfo`. If it is not, we
> supply a default path. Unfortunately, this default path is specific to
> POSIX. In many of our tests, this causes many of our tests to be unable
> to find Windows-standard executables like `ping`, and subsequently fail.
> 
> This commit will introduce a function, `default_path` that returns a
> sensible default path for both POSIX and Windows. Since the Windows
> implementation of this depends on the configuration of the host running
> the containerizer (rather than, say, the one creating the `TaskInfo`),
> we choose to implement this in `launch.cpp` instead of Stout, where a
> user could mistakenly call it and expect the same output on all hosts.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/launch.cpp 
> e482ab8bdfc358f695b87cda72ca59fb64cd8c4d 
> 
> Diff: https://reviews.apache.org/r/55037/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Review Request 55037: Added default PATH value in `launch.cpp` for Windows case.

[Alex Clemmer]

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

Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, John Kordich, 
and Joseph Wu.


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


Repository: mesos


Description
---

Currently, when the containerizer launches a process with the POSIX
launcher, it will check if the `$PATH` environment variable (or `%PATH%`
in the case of Windows is set in the `LaunchInfo`. If it is not, we
supply a default path. Unfortunately, this default path is specific to
POSIX. In many of our tests, this causes many of our tests to be unable
to find Windows-standard executables like `ping`, and subsequently fail.

This commit will introduce a function, `default_path` that returns a
sensible default path for both POSIX and Windows. Since the Windows
implementation of this depends on the configuration of the host running
the containerizer (rather than, say, the one creating the `TaskInfo`),
we choose to implement this in `launch.cpp` instead of Stout, where a
user could mistakenly call it and expect the same output on all hosts.


Diffs
-

  src/slave/containerizer/mesos/launch.cpp 
e482ab8bdfc358f695b87cda72ca59fb64cd8c4d 

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


Testing
---


Thanks,

Alex Clemmer