> 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 > >
