> On Aug. 21, 2017, 3:15 p.m., Qian Zhang wrote: > > src/slave/containerizer/mesos/isolators/network/ports.cpp > > Lines 183 (patched) > > <https://reviews.apache.org/r/60591/diff/10/?file=1800291#file1800291line183> > > > > This method is only called by `NetworkPortsIsolatorProcess::create()` > > and its logic is pretty straightforward, so I would suggest to kill it and > > move its logic into `NetworkPortsIsolatorProcess::create()`. > > James Peach wrote: > Actually, keeping this in a helper function makes the caller > significantly less complex. We can put the condition directly in the `if` > statement and give it a meaningful name. The alternative is to have a number > of similarly-named temporaries and a comment explaining what is going on. > This is much cleaner. > > Qian Zhang wrote: > Can you elaborate a bit on the alternative? What did you mean for > `similarly-named temporaries`? > > James Peach wrote: > The alternative looks like this: > > ```C > if (flags.container_ports_watch_resources_only) { > Try<Resources> resources = Resources::parse( > flags.resources.getOrElse(""), > flags.default_role); > > if (resources.isError()) { > return Error( > "Failed to parse agent resources: " + resources.error()); > } > > vector<Resource> resourceList = Resources::fromString( > flags.resources.getOrElse(""), flags.default_role).get(); > bool havePorts = false; > > foreach(const auto& resource, resourceList) { > if (resource.name() == "ports") { > havePorts = true; > break > } > } > > if (!havePorts) { > ... > } else > ``` > > You can see how the we accumulate 2 unnecessary temporaries and how the > conditional flow is obscured by inlining this.
Yeah, that's what I meant for merging the logic of `havePortsResource()` into `NetworkPortsIsolatorProcess::create()`. Anyway, I am OK with your current implementation as well. > On Aug. 21, 2017, 3:15 p.m., Qian Zhang wrote: > > src/slave/flags.cpp > > Lines 1012-1018 (patched) > > <https://reviews.apache.org/r/60591/diff/10/?file=1800293#file1800293line1012> > > > > So by default this flag is not enabled, that means any libprocess-based > > exectuors (e.g., command executor and default executor) will be killed > > since they will listen on ephemeral port? > > James Peach wrote: > Yes, that's correct. > > Qian Zhang wrote: > That is a breaking change. I mean once this code is merged, any > frameworks which uses command executor or default executor (e.g., Marathon) > will be broken since the tasks that they launch will be killed. > > James Peach wrote: > No, you'd have to enable the isolator to break your containers. > > Qian Zhang wrote: > Agree. > > And do you think `--check_agent_port_range_only` would be a better name? > If set to `true`, the `network/ports` isolator will do the check only within > agent's port range to restrict containers to only listen on the ports that > they have been assigned resources for, so containers are allowed to listen on > additional ports outside agent's port range. If set to `false`, the > `network/ports` isolator will restrict containers to only listen on ports > that they have been assigned resources for regardless of agent's port range. > > And the default value of `--check_agent_port_range_only` should be > `true`, which means `network/ports` isolator will run in soft mode, so when > operator enable `network/ports` isolator but does not set this flag, the > frameworks using libprocess-based executor will not be broken. > > James Peach wrote: > I can live with `--check_agent_port_range_only`. However, this should be > secure by default so we have to keep the default as `false`. > > James Peach wrote: > I just realized that the reason I chose the name was that is it symmetric > with `--container_ports_watch_interval` :( If we keep the default as `false`, then I think we need to mention somewhere (doc of `network/ports` isolator or this flag's help message) that any libprocess-based executor will not work in this case. - Qian ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60591/#review183303 ----------------------------------------------------------- On Aug. 24, 2017, 8:32 a.m., James Peach wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60591/ > ----------------------------------------------------------- > > (Updated Aug. 24, 2017, 8:32 a.m.) > > > Review request for mesos, Qian Zhang and Jiang Yan Xu. > > > Bugs: MESOS-7675 > https://issues.apache.org/jira/browse/MESOS-7675 > > > Repository: mesos > > > Description > ------- > > Normally, the `network/ports` isolator will kill any task that > listens on a port that it does not have resources for. However, > executors that are based on the libprocess API will always > listen on a port in the ephemeral range, and we want to make > it possible to use libprocess-based executors. > > Added the `--check_agent_port_range_only` option to only kill > tasks when they listen on un-allocated ports within the port > range published by the agent resources. This still prevents > port collisions between tasks, but doesn't kill them just > because the executor is listening on a port. > > > Diffs > ----- > > src/slave/containerizer/mesos/isolators/network/ports.hpp PRE-CREATION > src/slave/containerizer/mesos/isolators/network/ports.cpp PRE-CREATION > src/slave/flags.hpp 2970fea0cfac6af275a758d4bfedfe9a943c2b60 > src/slave/flags.cpp 3b02f3e909a554f15104739832ae3f252926b45f > > > Diff: https://reviews.apache.org/r/60591/diff/16/ > > > Testing > ------- > > make check (Fedora 26) > > > Thanks, > > James Peach > >
