mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Avinash sridharan <avin...@mesosphere.io>
Subject Re: Review Request 62017: Allows port mapper plugin to have optional args.
Date Thu, 07 Sep 2017 04:08:55 GMT


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


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