Re: Review Request 55023: Windows: Fixed malformatted containerizer command in launcher.

[Joseph Wu]

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


Ship it!




Ship It!

- Joseph Wu


On Jan. 18, 2017, 12:49 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55023/
> ---
> 
> (Updated Jan. 18, 2017, 12:49 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, John Kordich, 
> and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently in `MesosContainerizerProcess::_launch`, we are passing a
> malformatted shell command to the launcher. This causes the
> containerizer process to crash immediately upon invocation in all
> executor tests.
> 
> This commit will fix this command.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 8bf8a7774a38131c53f6d91c7f09f5dedd9d4cb4 
> 
> Diff: https://reviews.apache.org/r/55023/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 55023: Windows: Fixed malformatted containerizer command in launcher.

[Alex Clemmer]

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

(Updated Jan. 18, 2017, 8:49 p.m.)


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


Changes
---

Address Joseph's comments.


Repository: mesos


Description
---

Currently in `MesosContainerizerProcess::_launch`, we are passing a
malformatted shell command to the launcher. This causes the
containerizer process to crash immediately upon invocation in all
executor tests.

This commit will fix this command.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.cpp 
8bf8a7774a38131c53f6d91c7f09f5dedd9d4cb4 

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


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 55023: Windows: Fixed malformatted containerizer command in launcher.

[Alex Clemmer]

> On Jan. 18, 2017, 12:47 a.m., Joseph Wu wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, lines 1608-1617
> > 
> >
> > The effect of this is to change `argv` from:
> > 
> > ```
> > mesos-containerizer launch
> > ```
> > 
> > To:
> > 
> > ```
> > .../mesos/libexec/mesos-containerizer launch
> > ```
> > 
> > This change is harmless on POSIX, so there's no need for ifdef-ing.
> > 
> > If so, can't you accomplish the same thing with a diff like:
> > ```
> > diff --git a/src/slave/containerizer/mesos/containerizer.cpp 
> > b/src/slave/containerizer/mesos/containerizer.cpp
> > index 8bf8a77..c760130 100644
> > --- a/src/slave/containerizer/mesos/containerizer.cpp
> > +++ b/src/slave/containerizer/mesos/containerizer.cpp
> > @@ -1603,12 +1603,12 @@ Future MesosContainerizerProcess::_launch(
> >  
> >// Fork the child using launcher.
> >vector argv(2);
> > -  argv[0] = MESOS_CONTAINERIZER;
> > +  argv[0] = path::join(flags.launcher_dir, MESOS_CONTAINERIZER);
> >argv[1] = MesosContainerizerLaunch::NAME;
> >  
> >Try forked = launcher->fork(
> >containerId,
> > -  path::join(flags.launcher_dir, MESOS_CONTAINERIZER),
> > +  argv[0],
> >argv,
> >in.isSome() ? in.get() : Subprocess::FD(STDIN_FILENO),
> >out.isSome() ? out.get() : Subprocess::FD(STDOUT_FILENO),
> > ```

We've historically been super conservative about changing the POSIX path, but 
with your sign-off, I'm happy to do something like this.


> On Jan. 18, 2017, 12:47 a.m., Joseph Wu wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, lines 1608-1609
> > 
> >
> > It would appear that this TODO no longer applies.

This issue is not yet resolved, actually. The point I was trying to get at is 
that the first argument to `subprocess` (if memory serves) is a no-op. So this 
call is likely to change when we refactor `subprocess`.

We can still delete it if you want, though, but I think it's reasonable to keep 
it.


- Alex


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


On Jan. 15, 2017, 10:46 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55023/
> ---
> 
> (Updated Jan. 15, 2017, 10:46 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, John Kordich, 
> and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently in `MesosContainerizerProcess::_launch`, we are passing a
> malformatted shell command to the launcher. This causes the
> containerizer process to crash immediately upon invocation in all
> executor tests.
> 
> This commit will fix this command.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 8bf8a7774a38131c53f6d91c7f09f5dedd9d4cb4 
> 
> Diff: https://reviews.apache.org/r/55023/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 55023: Windows: Fixed malformatted containerizer command in launcher.

[Joseph Wu]

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




src/slave/containerizer/mesos/containerizer.cpp (lines 1608 - 1617)


The effect of this is to change `argv` from:

```
mesos-containerizer launch
```

To:

```
.../mesos/libexec/mesos-containerizer launch
```

This change is harmless on POSIX, so there's no need for ifdef-ing.

If so, can't you accomplish the same thing with a diff like:
```
diff --git a/src/slave/containerizer/mesos/containerizer.cpp 
b/src/slave/containerizer/mesos/containerizer.cpp
index 8bf8a77..c760130 100644
--- a/src/slave/containerizer/mesos/containerizer.cpp
+++ b/src/slave/containerizer/mesos/containerizer.cpp
@@ -1603,12 +1603,12 @@ Future MesosContainerizerProcess::_launch(
 
   // Fork the child using launcher.
   vector argv(2);
-  argv[0] = MESOS_CONTAINERIZER;
+  argv[0] = path::join(flags.launcher_dir, MESOS_CONTAINERIZER);
   argv[1] = MesosContainerizerLaunch::NAME;
 
   Try forked = launcher->fork(
   containerId,
-  path::join(flags.launcher_dir, MESOS_CONTAINERIZER),
+  argv[0],
   argv,
   in.isSome() ? in.get() : Subprocess::FD(STDIN_FILENO),
   out.isSome() ? out.get() : Subprocess::FD(STDOUT_FILENO),
```



src/slave/containerizer/mesos/containerizer.cpp (lines 1608 - 1609)


It would appear that this TODO no longer applies.


- Joseph Wu


On Jan. 15, 2017, 2:46 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55023/
> ---
> 
> (Updated Jan. 15, 2017, 2:46 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, John Kordich, 
> and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently in `MesosContainerizerProcess::_launch`, we are passing a
> malformatted shell command to the launcher. This causes the
> containerizer process to crash immediately upon invocation in all
> executor tests.
> 
> This commit will fix this command.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 8bf8a7774a38131c53f6d91c7f09f5dedd9d4cb4 
> 
> Diff: https://reviews.apache.org/r/55023/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 55023: Windows: Fixed malformatted containerizer command in launcher.

[Alex Clemmer]

> On Dec. 24, 2016, 10:06 p.m., Daniel Pravat wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, line 1611
> > 
> >
> > This code was working before. you should not change it to make the 
> > taest work and add overhead in production.

Per Slack discussion, this is necessary. We should be launching this binary out 
of the launcher directory to match POSIX semantics.


- Alex


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


On Jan. 15, 2017, 10:46 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55023/
> ---
> 
> (Updated Jan. 15, 2017, 10:46 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, John Kordich, 
> and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently in `MesosContainerizerProcess::_launch`, we are passing a
> malformatted shell command to the launcher. This causes the
> containerizer process to crash immediately upon invocation in all
> executor tests.
> 
> This commit will fix this command.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 8bf8a7774a38131c53f6d91c7f09f5dedd9d4cb4 
> 
> Diff: https://reviews.apache.org/r/55023/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 55023: Windows: Fixed malformatted containerizer command in launcher.

[Alex Clemmer]

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

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


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


Changes
---

Address Daniel's comments.


Repository: mesos


Description
---

Currently in `MesosContainerizerProcess::_launch`, we are passing a
malformatted shell command to the launcher. This causes the
containerizer process to crash immediately upon invocation in all
executor tests.

This commit will fix this command.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.cpp 
8bf8a7774a38131c53f6d91c7f09f5dedd9d4cb4 

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


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 55023: Windows: Fixed malformatted containerizer command in launcher.

[Daniel Pravat]

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




src/slave/containerizer/mesos/containerizer.cpp (line 1604)


This change is sufficient to fix the problem. Otherwise the user must add 
flags.launcher_dir to the path.



src/slave/containerizer/mesos/containerizer.cpp (line 1614)


Not necessary to run cmd.exe (arg0) just to start mesos_containerizer.


- Daniel Pravat


On Dec. 24, 2016, 10:02 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55023/
> ---
> 
> (Updated Dec. 24, 2016, 10:02 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, John Kordich, 
> and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently in `MesosContainerizerProcess::_launch`, we are passing a
> malformatted shell command to the launcher. This causes the
> containerizer process to crash immediately upon invocation in all
> executor tests.
> 
> This commit will fix this command.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> d9d5619e45ae1199fc91878f17a33b5647f48305 
> 
> Diff: https://reviews.apache.org/r/55023/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 55023: Windows: Fixed malformatted containerizer command in launcher.

[Daniel Pravat]

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




src/slave/containerizer/mesos/containerizer.cpp (line 1611)


This code was working before. you should not change it to make the taest 
work and add overhead in production.


- Daniel Pravat


On Dec. 24, 2016, 10:02 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55023/
> ---
> 
> (Updated Dec. 24, 2016, 10:02 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, John Kordich, 
> and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently in `MesosContainerizerProcess::_launch`, we are passing a
> malformatted shell command to the launcher. This causes the
> containerizer process to crash immediately upon invocation in all
> executor tests.
> 
> This commit will fix this command.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> d9d5619e45ae1199fc91878f17a33b5647f48305 
> 
> Diff: https://reviews.apache.org/r/55023/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Review Request 55023: Windows: Fixed malformatted containerizer command in launcher.

[Alex Clemmer]

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

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


Repository: mesos


Description
---

Currently in `MesosContainerizerProcess::_launch`, we are passing a
malformatted shell command to the launcher. This causes the
containerizer process to crash immediately upon invocation in all
executor tests.

This commit will fix this command.


Diffs
-

  src/slave/containerizer/mesos/containerizer.cpp 
d9d5619e45ae1199fc91878f17a33b5647f48305 

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


Testing
---


Thanks,

Alex Clemmer