mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alex Clemmer <clemmer.alexan...@gmail.com>
Subject Re: Review Request 55023: Windows: Fixed malformatted containerizer command in launcher.
Date Wed, 18 Jan 2017 17:21:17 GMT


> On Jan. 18, 2017, 12:47 a.m., Joseph Wu wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, lines 1608-1617
> > <https://reviews.apache.org/r/55023/diff/2/?file=1605568#file1605568line1608>
> >
> >     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<bool> MesosContainerizerProcess::_launch(
> >      
> >        // Fork the child using launcher.
> >        vector<string> argv(2);
> >     -  argv[0] = MESOS_CONTAINERIZER;
> >     +  argv[0] = path::join(flags.launcher_dir, MESOS_CONTAINERIZER);
> >        argv[1] = MesosContainerizerLaunch::NAME;
> >      
> >        Try<pid_t> 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
> > <https://reviews.apache.org/r/55023/diff/2/?file=1605568#file1605568line1608>
> >
> >     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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message