mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jie Yu <yujie....@gmail.com>
Subject Re: Review Request 51784: Supported merging the launch command from isolators.
Date Mon, 12 Sep 2016 16:45:00 GMT


> On Sept. 12, 2016, 1:46 p.m., Benjamin Bannier wrote:
> > I am not really sure it is a good idea to follow this approach. It might make things
simpler for anybody working on the Mesos containerizer, but I fear it might make the interfaces
we expose hard to use for everybody else since it i) doesn't try hard enough to shield isolator
developers from problems in other isolators, and ii) introduces special semantics for `CommandInfos`
when contained in certain messages.

I think my answers to those questions are the following:
1) Currently, command task is a special case in containerizer. This will change once the pod
executor is done, and we'll remove the special treatment for command tasks. And we won't have
those special cases.
2) CommandInfo is a tech debt as well. WE shouldn't have had 'user', 'env' and 'uris' in it
(I think we introduced that because we rushed the external containerizer). I wish we could
have solved that in v1, but we didn't. 

That being said, this is a short term solution to unblock us from using capabilities for command
tasks. Once pod executor is done, we can remove all the hacks in mesos containerizer regarding
command task.


> On Sept. 12, 2016, 1:46 p.m., Benjamin Bannier wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, lines 1105-1107
> > <https://reviews.apache.org/r/51784/diff/1/?file=1495422#file1495422line1105>
> >
> >     IMHO this is overly simplistic. We should always be prepared for modules being
loaded from modules, and potentially executed in any order. We should really put in a safety
net at a place where we have all information, e.g., run some sanity check in a good spot (here
seems to be one candidate). I am not sure how much we can sanitize though.

Do you have a good suggestion? I cannot come up with a better solution. See my above response.
This is a short term workaround and will be solved soon.

Keep in mind that we also need to support the cases where docker runtime isolator is not used,
but capabilities is turned on.


> On Sept. 12, 2016, 1:46 p.m., Benjamin Bannier wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, lines 1149-1154
> > <https://reviews.apache.org/r/51784/diff/1/?file=1495422#file1495422line1149>
> >
> >     While these fields are `optional` in the interface explicitly dropping them
here appears could break assumptions user made.

We never use those fields in 'launchCommand'. This just make it explicit. See my above response,
CommandInfo is a tech debt. We'll likely  to clean it up in v2.


- Jie


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


On Sept. 11, 2016, 12:17 a.m., Jie Yu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51784/
> -----------------------------------------------------------
> 
> (Updated Sept. 11, 2016, 12:17 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Gilbert Song.
> 
> 
> Bugs: MESOS-5275
>     https://issues.apache.org/jira/browse/MESOS-5275
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Previously, we only allow one isolator to specify the launch command
> for the container. This is not ideal because multiple isolators might
> want to add some flags to the command executor. For instance, the
> 'docker/runtime' isolator wants to specify '--task_command' and
> '--working_directory', and 'linux/capabilities' isolator wants to
> specify '--capabilities'.
> 
> This patch changes the semantics so that launch command from isolators
> are merged. However, it is isolator's responsibility to make sure the
> merged command is a valid command.
> 
> 
> Diffs
> -----
> 
>   src/slave/containerizer/mesos/containerizer.cpp 89b7e8db38916d69d9b2d4fe305d4397b0859a10

> 
> Diff: https://reviews.apache.org/r/51784/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>


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