mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Qian Zhang <zhq527...@gmail.com>
Subject Re: Review Request 60591: Optionally isolate only the agent network ports.
Date Thu, 24 Aug 2017 07:45:33 GMT


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


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