You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Avinash sridharan <av...@mesosphere.io> on 2016/12/13 23:51:59 UTC
Review Request 54716: Modified the initialization logic for
`network/cni` isolator.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54716/
-----------------------------------------------------------
Review request for mesos, Jie Yu and Qian Zhang.
Bugs: MESOS-6567
https://issues.apache.org/jira/browse/MESOS-6567
Repository: mesos
Description
-------
Since the `network/cni` isolator will soon be able to
add/modify/delete CNI configuration files without the need for agent
restart, we are changing the initialization logic to not bail out if
we see errors while reading a CNI configuration file or don't find a
CNI plugin for a given CNI configuration. If either of these errors
occur the `network/cni` isolator would simply skip the specific CNI
network and complete the initialization.
Diffs
-----
src/slave/containerizer/mesos/isolators/network/cni/cni.hpp b8fc755a8dd4757d904f7e97a71d3cf7f29d2033
src/slave/containerizer/mesos/isolators/network/cni/cni.cpp ddb4d33e9ecd0ffd118e8e68cb7ec90346b08049
Diff: https://reviews.apache.org/r/54716/diff/
Testing
-------
sudo make check --gtest_filter=*CNI*
Thanks,
Avinash sridharan
Re: Review Request 54716: Modified the initialization logic for
`network/cni` isolator.
Posted by Avinash sridharan <av...@mesosphere.io>.
> On Dec. 16, 2016, 11:39 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, line 1183
> > <https://reviews.apache.org/r/54716/diff/2/?file=1587919#file1587919line1183>
> >
> > let's use networkConfigPath consistently
The reason I have `cniConfigPath` here is that we use `networkConfigPath` to denote the checkpointed CNI configuration (line 1189 in `attach`). This seemed consistent with what we were doing in `detach`.
- Avinash
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54716/#review159510
-----------------------------------------------------------
On Dec. 17, 2016, 8:16 a.m., Avinash sridharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54716/
> -----------------------------------------------------------
>
> (Updated Dec. 17, 2016, 8:16 a.m.)
>
>
> Review request for mesos, Jie Yu and Qian Zhang.
>
>
> Bugs: MESOS-6567
> https://issues.apache.org/jira/browse/MESOS-6567
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Since the `network/cni` isolator will soon be able to
> add/modify/delete CNI configuration files without the need for agent
> restart, we are changing the initialization logic to not bail out if
> we see errors while reading a CNI configuration file or don't find a
> CNI plugin for a given CNI configuration. If either of these errors
> occur the `network/cni` isolator would simply skip the specific CNI
> network and complete the initialization.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/network/cni/cni.hpp b8fc755a8dd4757d904f7e97a71d3cf7f29d2033
> src/slave/containerizer/mesos/isolators/network/cni/cni.cpp ddb4d33e9ecd0ffd118e8e68cb7ec90346b08049
>
> Diff: https://reviews.apache.org/r/54716/diff/
>
>
> Testing
> -------
>
> sudo make check --gtest_filter=*CNI*
>
>
> Thanks,
>
> Avinash sridharan
>
>
Re: Review Request 54716: Modified the initialization logic for
`network/cni` isolator.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54716/#review159510
-----------------------------------------------------------
Fix it, then Ship it!
src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 110)
<https://reviews.apache.org/r/54716/#comment230527>
I would use `networkConfig` here and use networkConfig.get() below.
src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 251)
<https://reviews.apache.org/r/54716/#comment230523>
4 spaces indentation
src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 1123)
<https://reviews.apache.org/r/54716/#comment230524>
let's use networkConfigPath consistently
- Jie Yu
On Dec. 16, 2016, 8:28 p.m., Avinash sridharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54716/
> -----------------------------------------------------------
>
> (Updated Dec. 16, 2016, 8:28 p.m.)
>
>
> Review request for mesos, Jie Yu and Qian Zhang.
>
>
> Bugs: MESOS-6567
> https://issues.apache.org/jira/browse/MESOS-6567
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Since the `network/cni` isolator will soon be able to
> add/modify/delete CNI configuration files without the need for agent
> restart, we are changing the initialization logic to not bail out if
> we see errors while reading a CNI configuration file or don't find a
> CNI plugin for a given CNI configuration. If either of these errors
> occur the `network/cni` isolator would simply skip the specific CNI
> network and complete the initialization.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/network/cni/cni.hpp b8fc755a8dd4757d904f7e97a71d3cf7f29d2033
> src/slave/containerizer/mesos/isolators/network/cni/cni.cpp ddb4d33e9ecd0ffd118e8e68cb7ec90346b08049
>
> Diff: https://reviews.apache.org/r/54716/diff/
>
>
> Testing
> -------
>
> sudo make check --gtest_filter=*CNI*
>
>
> Thanks,
>
> Avinash sridharan
>
>
Re: Review Request 54716: Modified the initialization logic for
`network/cni` isolator.
Posted by Avinash sridharan <av...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54716/
-----------------------------------------------------------
(Updated Dec. 18, 2016, 2:03 a.m.)
Review request for mesos, Jie Yu and Qian Zhang.
Changes
-------
Addressed Qian's comments.
Bugs: MESOS-6567
https://issues.apache.org/jira/browse/MESOS-6567
Repository: mesos
Description
-------
Since the `network/cni` isolator will soon be able to
add/modify/delete CNI configuration files without the need for agent
restart, we are changing the initialization logic to not bail out if
we see errors while reading a CNI configuration file or don't find a
CNI plugin for a given CNI configuration. If either of these errors
occur the `network/cni` isolator would simply skip the specific CNI
network and complete the initialization.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/network/cni/cni.hpp b8fc755a8dd4757d904f7e97a71d3cf7f29d2033
src/slave/containerizer/mesos/isolators/network/cni/cni.cpp ddb4d33e9ecd0ffd118e8e68cb7ec90346b08049
Diff: https://reviews.apache.org/r/54716/diff/
Testing
-------
sudo make check --gtest_filter=*CNI*
Thanks,
Avinash sridharan
Re: Review Request 54716: Modified the initialization logic for
`network/cni` isolator.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54716/#review159552
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/network/cni/cni.hpp (line 177)
<https://reviews.apache.org/r/54716/#comment230585>
I think this comment needs to be updated since you have changed the value of this hashmap from network configuration info to the path of network configuration file.
src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 269)
<https://reviews.apache.org/r/54716/#comment230581>
s/errors/error/
src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 303)
<https://reviews.apache.org/r/54716/#comment230583>
Do we need to mention the same network name twice in this message? I think for the later one we can just mention it as "it", like:
```
Skipping network xxx from configuration file '/xxx', since we failed to find CNI plugin 'yyy' used by it.
```
src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 319)
<https://reviews.apache.org/r/54716/#comment230584>
Ditto.
- Qian Zhang
On Dec. 17, 2016, 4:16 p.m., Avinash sridharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54716/
> -----------------------------------------------------------
>
> (Updated Dec. 17, 2016, 4:16 p.m.)
>
>
> Review request for mesos, Jie Yu and Qian Zhang.
>
>
> Bugs: MESOS-6567
> https://issues.apache.org/jira/browse/MESOS-6567
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Since the `network/cni` isolator will soon be able to
> add/modify/delete CNI configuration files without the need for agent
> restart, we are changing the initialization logic to not bail out if
> we see errors while reading a CNI configuration file or don't find a
> CNI plugin for a given CNI configuration. If either of these errors
> occur the `network/cni` isolator would simply skip the specific CNI
> network and complete the initialization.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/network/cni/cni.hpp b8fc755a8dd4757d904f7e97a71d3cf7f29d2033
> src/slave/containerizer/mesos/isolators/network/cni/cni.cpp ddb4d33e9ecd0ffd118e8e68cb7ec90346b08049
>
> Diff: https://reviews.apache.org/r/54716/diff/
>
>
> Testing
> -------
>
> sudo make check --gtest_filter=*CNI*
>
>
> Thanks,
>
> Avinash sridharan
>
>
Re: Review Request 54716: Modified the initialization logic for
`network/cni` isolator.
Posted by Avinash sridharan <av...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54716/
-----------------------------------------------------------
(Updated Dec. 17, 2016, 8:16 a.m.)
Review request for mesos, Jie Yu and Qian Zhang.
Changes
-------
Addressed Jie's comments.
Bugs: MESOS-6567
https://issues.apache.org/jira/browse/MESOS-6567
Repository: mesos
Description
-------
Since the `network/cni` isolator will soon be able to
add/modify/delete CNI configuration files without the need for agent
restart, we are changing the initialization logic to not bail out if
we see errors while reading a CNI configuration file or don't find a
CNI plugin for a given CNI configuration. If either of these errors
occur the `network/cni` isolator would simply skip the specific CNI
network and complete the initialization.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/network/cni/cni.hpp b8fc755a8dd4757d904f7e97a71d3cf7f29d2033
src/slave/containerizer/mesos/isolators/network/cni/cni.cpp ddb4d33e9ecd0ffd118e8e68cb7ec90346b08049
Diff: https://reviews.apache.org/r/54716/diff/
Testing
-------
sudo make check --gtest_filter=*CNI*
Thanks,
Avinash sridharan
Re: Review Request 54716: Modified the initialization logic for
`network/cni` isolator.
Posted by Avinash sridharan <av...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54716/
-----------------------------------------------------------
(Updated Dec. 16, 2016, 8:28 p.m.)
Review request for mesos, Jie Yu and Qian Zhang.
Changes
-------
Addressed Jie's comments.
Bugs: MESOS-6567
https://issues.apache.org/jira/browse/MESOS-6567
Repository: mesos
Description
-------
Since the `network/cni` isolator will soon be able to
add/modify/delete CNI configuration files without the need for agent
restart, we are changing the initialization logic to not bail out if
we see errors while reading a CNI configuration file or don't find a
CNI plugin for a given CNI configuration. If either of these errors
occur the `network/cni` isolator would simply skip the specific CNI
network and complete the initialization.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/network/cni/cni.hpp b8fc755a8dd4757d904f7e97a71d3cf7f29d2033
src/slave/containerizer/mesos/isolators/network/cni/cni.cpp ddb4d33e9ecd0ffd118e8e68cb7ec90346b08049
Diff: https://reviews.apache.org/r/54716/diff/
Testing
-------
sudo make check --gtest_filter=*CNI*
Thanks,
Avinash sridharan
Re: Review Request 54716: Modified the initialization logic for
`network/cni` isolator.
Posted by Avinash sridharan <av...@mesosphere.io>.
> On Dec. 15, 2016, 10:05 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.hpp, lines 126-129
> > <https://reviews.apache.org/r/54716/diff/1/?file=1583415#file1583415line126>
> >
> > I would probably use the following interface:
> > ```
> > static Try<hashmap<string, string>> loadNetworkConfigs(
> > const string& configDir,
> > const string& pluginDir);
> > ```
> >
> > It seems weird to me to only cache 'type' of the config. I'd change the existing `networkConfigs` to be a hashmap of name to path. And we always validate the name in the config in `prepare`.
Agreed. Will change that.
- Avinash
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54716/#review159374
-----------------------------------------------------------
On Dec. 13, 2016, 11:51 p.m., Avinash sridharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54716/
> -----------------------------------------------------------
>
> (Updated Dec. 13, 2016, 11:51 p.m.)
>
>
> Review request for mesos, Jie Yu and Qian Zhang.
>
>
> Bugs: MESOS-6567
> https://issues.apache.org/jira/browse/MESOS-6567
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Since the `network/cni` isolator will soon be able to
> add/modify/delete CNI configuration files without the need for agent
> restart, we are changing the initialization logic to not bail out if
> we see errors while reading a CNI configuration file or don't find a
> CNI plugin for a given CNI configuration. If either of these errors
> occur the `network/cni` isolator would simply skip the specific CNI
> network and complete the initialization.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/network/cni/cni.hpp b8fc755a8dd4757d904f7e97a71d3cf7f29d2033
> src/slave/containerizer/mesos/isolators/network/cni/cni.cpp ddb4d33e9ecd0ffd118e8e68cb7ec90346b08049
>
> Diff: https://reviews.apache.org/r/54716/diff/
>
>
> Testing
> -------
>
> sudo make check --gtest_filter=*CNI*
>
>
> Thanks,
>
> Avinash sridharan
>
>
Re: Review Request 54716: Modified the initialization logic for
`network/cni` isolator.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54716/#review159374
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/network/cni/cni.hpp (lines 126 - 129)
<https://reviews.apache.org/r/54716/#comment230357>
I would probably use the following interface:
```
static Try<hashmap<string, string>> loadNetworkConfigs(
const string& configDir,
const string& pluginDir);
```
It seems weird to me to only cache 'type' of the config. I'd change the existing `networkConfigs` to be a hashmap of name to path. And we always validate the name in the config in `prepare`.
- Jie Yu
On Dec. 13, 2016, 11:51 p.m., Avinash sridharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54716/
> -----------------------------------------------------------
>
> (Updated Dec. 13, 2016, 11:51 p.m.)
>
>
> Review request for mesos, Jie Yu and Qian Zhang.
>
>
> Bugs: MESOS-6567
> https://issues.apache.org/jira/browse/MESOS-6567
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Since the `network/cni` isolator will soon be able to
> add/modify/delete CNI configuration files without the need for agent
> restart, we are changing the initialization logic to not bail out if
> we see errors while reading a CNI configuration file or don't find a
> CNI plugin for a given CNI configuration. If either of these errors
> occur the `network/cni` isolator would simply skip the specific CNI
> network and complete the initialization.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/network/cni/cni.hpp b8fc755a8dd4757d904f7e97a71d3cf7f29d2033
> src/slave/containerizer/mesos/isolators/network/cni/cni.cpp ddb4d33e9ecd0ffd118e8e68cb7ec90346b08049
>
> Diff: https://reviews.apache.org/r/54716/diff/
>
>
> Testing
> -------
>
> sudo make check --gtest_filter=*CNI*
>
>
> Thanks,
>
> Avinash sridharan
>
>