You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Deepak Goel <de...@gmail.com> on 2017/08/31 16:16:32 UTC
Review Request 62017: Allows port mapper plugin to have optional args.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62017/
-----------------------------------------------------------
Review request for mesos and Avinash sridharan.
Bugs: mesos-7923
https://issues.apache.org/jira/browse/mesos-7923
Repository: mesos
Description
-------
Mesos port mapper cni plugin is a wrapper around bridge plugin
to add port mapping functionality to bridge plugin. However, in
certain cases the network creator doesn't need port mapping
functionality and just want to access bridge plugin. In this case,
the creator may not supply `args` in cni config which will makes
mesos port mapper plugin to fail. This patch makes `args` in cni
config optional for mesos port mapper plugin
Diffs
-----
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp 43cf3e44a55c56dc8195c9cd05f6edd8bf13d448
Diff: https://reviews.apache.org/r/62017/diff/1/
Testing
-------
Thanks,
Deepak Goel
Re: Review Request 62017: Allows port mapper plugin to have optional
args.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62017/#review184468
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
Lines 141-143 (original), 141-144 (patched)
<https://reviews.apache.org/r/62017/#comment260618>
What about changing the comment like the below?
```
// While the 'args' field is optional in the CNI spec it is critical
// to the port-mapper plugin to learn of any port-mappings that the
// framework might have requested for this container when this plugin
// is called in the Mesos context. However, to make the port-mapper
// plugin can be used in a more generic environment rather than Mesos
// specific, we will create a fake 'args` field if it is not filled by
// the caller.
```
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
Line 146 (original), 147 (patched)
<https://reviews.apache.org/r/62017/#comment260616>
I do not think we need this comment because there is an exactly same one in the above.
- Qian Zhang
On Sept. 1, 2017, 12:16 a.m., Deepak Goel wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62017/
> -----------------------------------------------------------
>
> (Updated Sept. 1, 2017, 12:16 a.m.)
>
>
> Review request for mesos and Avinash sridharan.
>
>
> Bugs: mesos-7923
> https://issues.apache.org/jira/browse/mesos-7923
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Mesos port mapper cni plugin is a wrapper around bridge plugin
> to add port mapping functionality to bridge plugin. However, in
> certain cases the network creator doesn't need port mapping
> functionality and just want to access bridge plugin. In this case,
> the creator may not supply `args` in cni config which will makes
> mesos port mapper plugin to fail. This patch makes `args` in cni
> config optional for mesos port mapper plugin
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp 43cf3e44a55c56dc8195c9cd05f6edd8bf13d448
>
>
> Diff: https://reviews.apache.org/r/62017/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Deepak Goel
>
>
Re: Review Request 62017: Allows port mapper plugin to have optional
args.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62017/#review184603
-----------------------------------------------------------
Fix it, then Ship it!
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
Lines 145-150 (original), 148-159 (patched)
<https://reviews.apache.org/r/62017/#comment260767>
What about changing it to the below?
```
if (args.isError()) {
return PluginError(
"Failed to get the required field 'args': " + args.error(),
ERROR_BAD_ARGS);
} else if (args.isNone()) {
JSON::Object _args;
JSON::Object mesos;
mesos.values["network_info"] = JSON::Object();
_args.values["org.apache.mesos"] = mesos;
args = _args;
}
```
- Qian Zhang
On Sept. 6, 2017, 5:47 a.m., Deepak Goel wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62017/
> -----------------------------------------------------------
>
> (Updated Sept. 6, 2017, 5:47 a.m.)
>
>
> Review request for mesos and Avinash sridharan.
>
>
> Bugs: mesos-7923
> https://issues.apache.org/jira/browse/mesos-7923
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Mesos port mapper cni plugin is a wrapper around bridge plugin
> to add port mapping functionality to bridge plugin. However, in
> certain cases the network creator doesn't need port mapping
> functionality and just want to access bridge plugin. In this case,
> the creator may not supply `args` in cni config which will makes
> mesos port mapper plugin to fail. This patch makes `args` in cni
> config optional for mesos port mapper plugin
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp 43cf3e44a55c56dc8195c9cd05f6edd8bf13d448
>
>
> Diff: https://reviews.apache.org/r/62017/diff/3/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Deepak Goel
>
>
Re: Review Request 62017: Allows port mapper plugin to have optional
args.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62017/#review184625
-----------------------------------------------------------
Bad patch!
Reviews applied: [62017]
Failed command: python support/apply-reviews.py -n -r 62017
Error:
2017-09-06 03:55:20 URL:https://reviews.apache.org/r/62017/diff/raw/ [1758/1758] -> "62017.patch" [1]
error: patch failed: src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp:140
error: src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp: patch does not apply
Full log: https://builds.apache.org/job/Mesos-Reviewbot/19217/console
- Mesos Reviewbot
On Sept. 6, 2017, 12:25 a.m., Deepak Goel wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62017/
> -----------------------------------------------------------
>
> (Updated Sept. 6, 2017, 12:25 a.m.)
>
>
> Review request for mesos and Avinash sridharan.
>
>
> Bugs: mesos-7923
> https://issues.apache.org/jira/browse/mesos-7923
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Mesos port mapper cni plugin is a wrapper around bridge plugin
> to add port mapping functionality to bridge plugin. However, in
> certain cases the network creator doesn't need port mapping
> functionality and just want to access bridge plugin. In this case,
> the creator may not supply `args` in cni config which will makes
> mesos port mapper plugin to fail. This patch makes `args` in cni
> config optional for mesos port mapper plugin
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp 43cf3e44a55c56dc8195c9cd05f6edd8bf13d448
>
>
> Diff: https://reviews.apache.org/r/62017/diff/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Deepak Goel
>
>
Re: Review Request 62017: Allows port mapper plugin to have optional
args.
Posted by Avinash sridharan <av...@mesosphere.io>.
> On Sept. 6, 2017, 2:09 a.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
> > Lines 149 (patched)
> > <https://reviews.apache.org/r/62017/diff/4/?file=1816054#file1816054line149>
> >
> > Why are we setting this empty `args` here instead of setting up a `NetworkInfo networkInfo` object and passing it down into the constructor of the port-mapper?
> >
> > the `port-mapper` really doesn't care about `args` it cares about `NetworkInfo`. This code seems to conflate the whole need for not setting up `args` here.
> >
> > Also setting up `mesos.values` seems to tie the port-mapper plugin back to Mesos semantics of setting up the args which seems like a bit of a hack.
>
> Deepak Goel wrote:
> Yeah, you are right. Initially, I thought of doing that but then it led to making exceptions to various checks that happens before we reach to the constructor and the code looked even more hacky and error prone. This patch achieves the same goal with fewer lines of code change
I don't follow. Why would it be more hacky?
If we just need to setup the `NetworkInfo` all we would do:
```
NetworkInfo networkInfo
if (args.isSome()) {
// Move current code into this segement
// If `NetworkInfo` is set:
networkInfo = <parse `NetworkInfo`>
}
// create `port-mapper` with `networkInfo`
```
Not sure whats so complicated about the above ^^
- Avinash
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62017/#review184620
-----------------------------------------------------------
On Sept. 6, 2017, 12:25 a.m., Deepak Goel wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62017/
> -----------------------------------------------------------
>
> (Updated Sept. 6, 2017, 12:25 a.m.)
>
>
> Review request for mesos and Avinash sridharan.
>
>
> Bugs: mesos-7923
> https://issues.apache.org/jira/browse/mesos-7923
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Mesos port mapper cni plugin is a wrapper around bridge plugin
> to add port mapping functionality to bridge plugin. However, in
> certain cases the network creator doesn't need port mapping
> functionality and just want to access bridge plugin. In this case,
> the creator may not supply `args` in cni config which will makes
> mesos port mapper plugin to fail. This patch makes `args` in cni
> config optional for mesos port mapper plugin
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp 43cf3e44a55c56dc8195c9cd05f6edd8bf13d448
>
>
> Diff: https://reviews.apache.org/r/62017/diff/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Deepak Goel
>
>
Re: Review Request 62017: Allows port mapper plugin to have optional
args.
Posted by Deepak Goel <de...@gmail.com>.
> On Sept. 6, 2017, 2:09 a.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
> > Lines 149 (patched)
> > <https://reviews.apache.org/r/62017/diff/4/?file=1816054#file1816054line149>
> >
> > Why are we setting this empty `args` here instead of setting up a `NetworkInfo networkInfo` object and passing it down into the constructor of the port-mapper?
> >
> > the `port-mapper` really doesn't care about `args` it cares about `NetworkInfo`. This code seems to conflate the whole need for not setting up `args` here.
> >
> > Also setting up `mesos.values` seems to tie the port-mapper plugin back to Mesos semantics of setting up the args which seems like a bit of a hack.
Yeah, you are right. Initially, I thought of doing that but then it led to making exceptions to various checks that happens before we reach to the constructor and the code looked even more hacky and error prone. This patch achieves the same goal with fewer lines of code change
- Deepak
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62017/#review184620
-----------------------------------------------------------
On Sept. 6, 2017, 12:25 a.m., Deepak Goel wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62017/
> -----------------------------------------------------------
>
> (Updated Sept. 6, 2017, 12:25 a.m.)
>
>
> Review request for mesos and Avinash sridharan.
>
>
> Bugs: mesos-7923
> https://issues.apache.org/jira/browse/mesos-7923
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Mesos port mapper cni plugin is a wrapper around bridge plugin
> to add port mapping functionality to bridge plugin. However, in
> certain cases the network creator doesn't need port mapping
> functionality and just want to access bridge plugin. In this case,
> the creator may not supply `args` in cni config which will makes
> mesos port mapper plugin to fail. This patch makes `args` in cni
> config optional for mesos port mapper plugin
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp 43cf3e44a55c56dc8195c9cd05f6edd8bf13d448
>
>
> Diff: https://reviews.apache.org/r/62017/diff/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Deepak Goel
>
>
Re: Review Request 62017: Allows port mapper plugin to have optional
args.
Posted by Avinash sridharan <av...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62017/#review184620
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
Lines 149 (patched)
<https://reviews.apache.org/r/62017/#comment260797>
Why are we setting this empty `args` here instead of setting up a `NetworkInfo networkInfo` object and passing it down into the constructor of the port-mapper?
the `port-mapper` really doesn't care about `args` it cares about `NetworkInfo`. This code seems to conflate the whole need for not setting up `args` here.
Also setting up `mesos.values` seems to tie the port-mapper plugin back to Mesos semantics of setting up the args which seems like a bit of a hack.
- Avinash sridharan
On Sept. 6, 2017, 12:25 a.m., Deepak Goel wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62017/
> -----------------------------------------------------------
>
> (Updated Sept. 6, 2017, 12:25 a.m.)
>
>
> Review request for mesos and Avinash sridharan.
>
>
> Bugs: mesos-7923
> https://issues.apache.org/jira/browse/mesos-7923
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Mesos port mapper cni plugin is a wrapper around bridge plugin
> to add port mapping functionality to bridge plugin. However, in
> certain cases the network creator doesn't need port mapping
> functionality and just want to access bridge plugin. In this case,
> the creator may not supply `args` in cni config which will makes
> mesos port mapper plugin to fail. This patch makes `args` in cni
> config optional for mesos port mapper plugin
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp 43cf3e44a55c56dc8195c9cd05f6edd8bf13d448
>
>
> Diff: https://reviews.apache.org/r/62017/diff/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Deepak Goel
>
>
Re: Review Request 62017: Allows port mapper plugin to have optional
args.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62017/#review184621
-----------------------------------------------------------
FAIL: Mesos tests failed to run
Reviews applied: [62017]
Logs available here: http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/62017/logs
- Mesos Reviewbot Windows
On Sept. 6, 2017, 12:25 a.m., Deepak Goel wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62017/
> -----------------------------------------------------------
>
> (Updated Sept. 6, 2017, 12:25 a.m.)
>
>
> Review request for mesos and Avinash sridharan.
>
>
> Bugs: mesos-7923
> https://issues.apache.org/jira/browse/mesos-7923
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Mesos port mapper cni plugin is a wrapper around bridge plugin
> to add port mapping functionality to bridge plugin. However, in
> certain cases the network creator doesn't need port mapping
> functionality and just want to access bridge plugin. In this case,
> the creator may not supply `args` in cni config which will makes
> mesos port mapper plugin to fail. This patch makes `args` in cni
> config optional for mesos port mapper plugin
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp 43cf3e44a55c56dc8195c9cd05f6edd8bf13d448
>
>
> Diff: https://reviews.apache.org/r/62017/diff/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Deepak Goel
>
>
Re: Review Request 62017: Allows port mapper plugin to have optional
args.
Posted by Deepak Goel <de...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62017/
-----------------------------------------------------------
(Updated Sept. 6, 2017, 12:25 a.m.)
Review request for mesos and Avinash sridharan.
Bugs: mesos-7923
https://issues.apache.org/jira/browse/mesos-7923
Repository: mesos
Description
-------
Mesos port mapper cni plugin is a wrapper around bridge plugin
to add port mapping functionality to bridge plugin. However, in
certain cases the network creator doesn't need port mapping
functionality and just want to access bridge plugin. In this case,
the creator may not supply `args` in cni config which will makes
mesos port mapper plugin to fail. This patch makes `args` in cni
config optional for mesos port mapper plugin
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp 43cf3e44a55c56dc8195c9cd05f6edd8bf13d448
Diff: https://reviews.apache.org/r/62017/diff/4/
Changes: https://reviews.apache.org/r/62017/diff/3-4/
Testing
-------
Thanks,
Deepak Goel
Re: Review Request 62017: Allows port mapper plugin to have optional
args.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62017/#review184604
-----------------------------------------------------------
FAIL: Mesos tests failed to run
Reviews applied: [62017]
Logs available here: http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/62017/logs
- Mesos Reviewbot Windows
On Sept. 5, 2017, 9:47 p.m., Deepak Goel wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62017/
> -----------------------------------------------------------
>
> (Updated Sept. 5, 2017, 9:47 p.m.)
>
>
> Review request for mesos and Avinash sridharan.
>
>
> Bugs: mesos-7923
> https://issues.apache.org/jira/browse/mesos-7923
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Mesos port mapper cni plugin is a wrapper around bridge plugin
> to add port mapping functionality to bridge plugin. However, in
> certain cases the network creator doesn't need port mapping
> functionality and just want to access bridge plugin. In this case,
> the creator may not supply `args` in cni config which will makes
> mesos port mapper plugin to fail. This patch makes `args` in cni
> config optional for mesos port mapper plugin
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp 43cf3e44a55c56dc8195c9cd05f6edd8bf13d448
>
>
> Diff: https://reviews.apache.org/r/62017/diff/3/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Deepak Goel
>
>
Re: Review Request 62017: Allows port mapper plugin to have optional
args.
Posted by Deepak Goel <de...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62017/
-----------------------------------------------------------
(Updated Sept. 5, 2017, 9:47 p.m.)
Review request for mesos and Avinash sridharan.
Bugs: mesos-7923
https://issues.apache.org/jira/browse/mesos-7923
Repository: mesos
Description
-------
Mesos port mapper cni plugin is a wrapper around bridge plugin
to add port mapping functionality to bridge plugin. However, in
certain cases the network creator doesn't need port mapping
functionality and just want to access bridge plugin. In this case,
the creator may not supply `args` in cni config which will makes
mesos port mapper plugin to fail. This patch makes `args` in cni
config optional for mesos port mapper plugin
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp 43cf3e44a55c56dc8195c9cd05f6edd8bf13d448
Diff: https://reviews.apache.org/r/62017/diff/2/
Changes: https://reviews.apache.org/r/62017/diff/1-2/
Testing
-------
Thanks,
Deepak Goel
Re: Review Request 62017: Allows port mapper plugin to have optional
args.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62017/#review184307
-----------------------------------------------------------
Patch looks great!
Reviews applied: [62017]
Logs available here: http://104.210.40.105/logs/master/62017
- Mesos Reviewbot Windows
On Aug. 31, 2017, 9:16 a.m., Deepak Goel wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62017/
> -----------------------------------------------------------
>
> (Updated Aug. 31, 2017, 9:16 a.m.)
>
>
> Review request for mesos and Avinash sridharan.
>
>
> Bugs: mesos-7923
> https://issues.apache.org/jira/browse/mesos-7923
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Mesos port mapper cni plugin is a wrapper around bridge plugin
> to add port mapping functionality to bridge plugin. However, in
> certain cases the network creator doesn't need port mapping
> functionality and just want to access bridge plugin. In this case,
> the creator may not supply `args` in cni config which will makes
> mesos port mapper plugin to fail. This patch makes `args` in cni
> config optional for mesos port mapper plugin
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp 43cf3e44a55c56dc8195c9cd05f6edd8bf13d448
>
>
> Diff: https://reviews.apache.org/r/62017/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Deepak Goel
>
>
Re: Review Request 62017: Allows port mapper plugin to have optional
args.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62017/#review184507
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
Lines 145-147 (original), 146-148 (patched)
<https://reviews.apache.org/r/62017/#comment260671>
We should only create a fake args when `args.isNone()` but not when `args.isError()`.
- Qian Zhang
On Sept. 1, 2017, 12:16 a.m., Deepak Goel wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62017/
> -----------------------------------------------------------
>
> (Updated Sept. 1, 2017, 12:16 a.m.)
>
>
> Review request for mesos and Avinash sridharan.
>
>
> Bugs: mesos-7923
> https://issues.apache.org/jira/browse/mesos-7923
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Mesos port mapper cni plugin is a wrapper around bridge plugin
> to add port mapping functionality to bridge plugin. However, in
> certain cases the network creator doesn't need port mapping
> functionality and just want to access bridge plugin. In this case,
> the creator may not supply `args` in cni config which will makes
> mesos port mapper plugin to fail. This patch makes `args` in cni
> config optional for mesos port mapper plugin
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp 43cf3e44a55c56dc8195c9cd05f6edd8bf13d448
>
>
> Diff: https://reviews.apache.org/r/62017/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Deepak Goel
>
>
Re: Review Request 62017: Allows port mapper plugin to have optional
args.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62017/#review184310
-----------------------------------------------------------
Patch looks great!
Reviews applied: [62017]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On Aug. 31, 2017, 9:16 a.m., Deepak Goel wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62017/
> -----------------------------------------------------------
>
> (Updated Aug. 31, 2017, 9:16 a.m.)
>
>
> Review request for mesos and Avinash sridharan.
>
>
> Bugs: mesos-7923
> https://issues.apache.org/jira/browse/mesos-7923
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Mesos port mapper cni plugin is a wrapper around bridge plugin
> to add port mapping functionality to bridge plugin. However, in
> certain cases the network creator doesn't need port mapping
> functionality and just want to access bridge plugin. In this case,
> the creator may not supply `args` in cni config which will makes
> mesos port mapper plugin to fail. This patch makes `args` in cni
> config optional for mesos port mapper plugin
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp 43cf3e44a55c56dc8195c9cd05f6edd8bf13d448
>
>
> Diff: https://reviews.apache.org/r/62017/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Deepak Goel
>
>
Re: Review Request 62017: Allows port mapper plugin to have optional
args.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62017/#review184405
-----------------------------------------------------------
@Deepak, in the description, I see you mentioned:
> However, in certain cases the network creator doesn't need port mapping functionality and just want to access bridge plugin.
I am a bit confused about the real use case, if the network creator doesn't need port mapping functionality, why not just using bridge plugin directly? Using port-mapper plugin but cutting its port mapping functionality just for using bridge plugin seems not an efficient way. I am not sure why the port-mapper plugin is needed in this case.
> In this case, the creator may not supply `args` in cni config which will makes mesos port mapper plugin to fail. This patch makes `args` in cni config optional for mesos port mapper plugin
What do you mean for `the creator`? If it is Mesos agent, then I think its `network/cni` isolator will always supply `args` to port mapper plugin.
- Qian Zhang
On Sept. 1, 2017, 12:16 a.m., Deepak Goel wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62017/
> -----------------------------------------------------------
>
> (Updated Sept. 1, 2017, 12:16 a.m.)
>
>
> Review request for mesos and Avinash sridharan.
>
>
> Bugs: mesos-7923
> https://issues.apache.org/jira/browse/mesos-7923
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Mesos port mapper cni plugin is a wrapper around bridge plugin
> to add port mapping functionality to bridge plugin. However, in
> certain cases the network creator doesn't need port mapping
> functionality and just want to access bridge plugin. In this case,
> the creator may not supply `args` in cni config which will makes
> mesos port mapper plugin to fail. This patch makes `args` in cni
> config optional for mesos port mapper plugin
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp 43cf3e44a55c56dc8195c9cd05f6edd8bf13d448
>
>
> Diff: https://reviews.apache.org/r/62017/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Deepak Goel
>
>