You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Qian Zhang <zh...@gmail.com> on 2017/08/21 07:15:38 UTC
Re: Review Request 60591: Optionally isolate only the agent network
ports.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60591/#review183303
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/network/ports.cpp
Lines 183 (patched)
<https://reviews.apache.org/r/60591/#comment259345>
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()`.
src/slave/containerizer/mesos/isolators/network/ports.cpp
Lines 239 (patched)
<https://reviews.apache.org/r/60591/#comment259342>
What if `ports.isSome()` is `false`? And what if ports resource is specified in `--resources` as `ports:[]`?
src/slave/containerizer/mesos/isolators/network/ports.cpp
Lines 245-246 (patched)
<https://reviews.apache.org/r/60591/#comment259346>
Won't agent listen on another available ephemeral port when it is restarted?
src/slave/flags.cpp
Lines 1012-1018 (patched)
<https://reviews.apache.org/r/60591/#comment259343>
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?
- Qian Zhang
On July 3, 2017, 6:30 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60591/
> -----------------------------------------------------------
>
> (Updated July 3, 2017, 6:30 p.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 `--container_ports_watch_resources_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/10/
>
>
> Testing
> -------
>
> make check (Fedora 26)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 60591: Optionally isolate only the agent network
ports.
Posted by James Peach <jp...@apache.org>.
> On Aug. 21, 2017, 7:15 a.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()`.
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.
> On Aug. 21, 2017, 7:15 a.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/network/ports.cpp
> > Lines 239 (patched)
> > <https://reviews.apache.org/r/60591/diff/10/?file=1800291#file1800291line239>
> >
> > What if `ports.isSome()` is `false`? And what if ports resource is specified in `--resources` as `ports:[]`?
Good point. In that case, we need to specify an empty ports interval set. I added a new test to [r/60765](https://reviews.apache.org/r/60765/) to cover the case where we run a test with an empty ports resource.
> On Aug. 21, 2017, 7:15 a.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/network/ports.cpp
> > Lines 245-246 (patched)
> > <https://reviews.apache.org/r/60591/diff/10/?file=1800291#file1800291line245>
> >
> > Won't agent listen on another available ephemeral port when it is restarted?
The port the agent listens on is never ephemeral; the default is `0.0.0.0:5051`. Thinking some more about this, it is quite difficult to prevent a container listening on the agent port. If the agent is running, a container could only listen on this port if `SO_REUSEPORT` is being used. If the agent isn't running, then we could not prevent a container taking this port and thereby preventing the agent starting.
> On Aug. 21, 2017, 7:15 a.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?
Yes, that's correct.
- James
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60591/#review183303
-----------------------------------------------------------
On July 3, 2017, 10:30 a.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60591/
> -----------------------------------------------------------
>
> (Updated July 3, 2017, 10:30 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 `--container_ports_watch_resources_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/11/
>
>
> Testing
> -------
>
> make check (Fedora 26)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 60591: Optionally isolate only the agent network
ports.
Posted by Qian Zhang <zh...@gmail.com>.
> 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
>
>
Re: Review Request 60591: Optionally isolate only the agent network
ports.
Posted by James Peach <jp...@apache.org>.
> On Aug. 21, 2017, 7:15 a.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.
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
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60591/#review183303
-----------------------------------------------------------
On Aug. 21, 2017, 8:36 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60591/
> -----------------------------------------------------------
>
> (Updated Aug. 21, 2017, 8:36 p.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 `--container_ports_watch_resources_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/13/
>
>
> Testing
> -------
>
> make check (Fedora 26)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 60591: Optionally isolate only the agent network
ports.
Posted by James Peach <jp...@apache.org>.
> On Aug. 21, 2017, 7:15 a.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/network/ports.cpp
> > Lines 245-246 (patched)
> > <https://reviews.apache.org/r/60591/diff/10/?file=1800291#file1800291line245>
> >
> > Won't agent listen on another available ephemeral port when it is restarted?
>
> James Peach wrote:
> The port the agent listens on is never ephemeral; the default is `0.0.0.0:5051`. Thinking some more about this, it is quite difficult to prevent a container listening on the agent port. If the agent is running, a container could only listen on this port if `SO_REUSEPORT` is being used. If the agent isn't running, then we could not prevent a container taking this port and thereby preventing the agent starting.
>
> Qian Zhang wrote:
> Agree, so I think we may not need this logic in this isolator.
I'll consider it a bit more today but I think I agree with you :)
> On Aug. 21, 2017, 7:15 a.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.
No, you'd have to enable the isolator to break your containers.
- James
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60591/#review183303
-----------------------------------------------------------
On Aug. 21, 2017, 8:36 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60591/
> -----------------------------------------------------------
>
> (Updated Aug. 21, 2017, 8:36 p.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 `--container_ports_watch_resources_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/12/
>
>
> Testing
> -------
>
> make check (Fedora 26)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 60591: Optionally isolate only the agent network
ports.
Posted by Qian Zhang <zh...@gmail.com>.
> 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.
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.
- Qian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60591/#review183303
-----------------------------------------------------------
On Aug. 22, 2017, 4:36 a.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60591/
> -----------------------------------------------------------
>
> (Updated Aug. 22, 2017, 4:36 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 `--container_ports_watch_resources_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/13/
>
>
> Testing
> -------
>
> make check (Fedora 26)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 60591: Optionally isolate only the agent network
ports.
Posted by James Peach <jp...@apache.org>.
> On Aug. 21, 2017, 7:15 a.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` :(
>
> Qian Zhang wrote:
> 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.
Yes, when I write the documentation I will try to make this clear.
- James
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60591/#review183303
-----------------------------------------------------------
On Aug. 24, 2017, 12: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, 12: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
>
>
Re: Review Request 60591: Optionally isolate only the agent network
ports.
Posted by James Peach <jp...@apache.org>.
> On Aug. 21, 2017, 7:15 a.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`.
I just realized that the reason I chose the name was that is it symmetric with `--container_ports_watch_interval` :(
- James
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60591/#review183303
-----------------------------------------------------------
On Aug. 23, 2017, 9:57 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60591/
> -----------------------------------------------------------
>
> (Updated Aug. 23, 2017, 9:57 p.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/15/
>
>
> Testing
> -------
>
> make check (Fedora 26)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 60591: Optionally isolate only the agent network
ports.
Posted by Qian Zhang <zh...@gmail.com>.
> 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.
Can you elaborate a bit on the alternative? What did you mean for `similarly-named temporaries`?
> On Aug. 21, 2017, 3:15 p.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/network/ports.cpp
> > Lines 245-246 (patched)
> > <https://reviews.apache.org/r/60591/diff/10/?file=1800291#file1800291line245>
> >
> > Won't agent listen on another available ephemeral port when it is restarted?
>
> James Peach wrote:
> The port the agent listens on is never ephemeral; the default is `0.0.0.0:5051`. Thinking some more about this, it is quite difficult to prevent a container listening on the agent port. If the agent is running, a container could only listen on this port if `SO_REUSEPORT` is being used. If the agent isn't running, then we could not prevent a container taking this port and thereby preventing the agent starting.
Agree, so I think we may not need this logic in this isolator.
> 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.
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.
- Qian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60591/#review183303
-----------------------------------------------------------
On Aug. 22, 2017, 4:36 a.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60591/
> -----------------------------------------------------------
>
> (Updated Aug. 22, 2017, 4:36 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 `--container_ports_watch_resources_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/12/
>
>
> Testing
> -------
>
> make check (Fedora 26)
>
>
> Thanks,
>
> James Peach
>
>