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:55:49 UTC
Review Request 54717: Modified `network/cni` isolator for dynamic
addition of CNI networks.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54717/
-----------------------------------------------------------
Review request for mesos, Jie Yu and Qian Zhang.
Bugs: MESOS-6567
https://issues.apache.org/jira/browse/MESOS-6567
Repository: mesos
Description
-------
If the `network/cni` isolator sees a cache-miss during the `prepare`
phase, it will try to look for the CNI network on disk before giving
up. This allows for the dynamic addition of CNI networks without the
need for agent restart.
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/54717/diff/
Testing
-------
make check
Manual testing with addition of a non-existent CNI configuration.
Thanks,
Avinash sridharan
Re: Review Request 54717: Modified `network/cni` isolator for dynamic
addition of CNI networks.
Posted by Avinash sridharan <av...@mesosphere.io>.
> On Dec. 15, 2016, 10:32 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, lines 1152-1172
> > <https://reviews.apache.org/r/54717/diff/2/?file=1583519#file1583519line1152>
> >
> > I would introduce a helper:
> > ```
> > Try<JSON::Object> networkConfigJson = getNetworkConfigJson(networkName);
> > ```
> >
> > The helper will cross check the name in the config with the cache, and do a reload if necessary.
sounds good.
- Avinash
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54717/#review159378
-----------------------------------------------------------
On Dec. 14, 2016, 12:28 a.m., Avinash sridharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54717/
> -----------------------------------------------------------
>
> (Updated Dec. 14, 2016, 12:28 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
> -------
>
> If the `network/cni` isolator sees a cache-miss during the `prepare`
> phase, it will try to look for the CNI network on disk before giving
> up. This allows for the dynamic addition of CNI networks without the
> need for agent restart.
>
>
> 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/54717/diff/
>
>
> Testing
> -------
>
> make check
>
> Manual testing with addition of a non-existent CNI configuration.
>
>
> Thanks,
>
> Avinash sridharan
>
>
Re: Review Request 54717: Modified `network/cni` isolator for dynamic
addition of CNI networks.
Posted by Avinash sridharan <av...@mesosphere.io>.
> On Dec. 15, 2016, 10:32 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, lines 596-599
> > <https://reviews.apache.org/r/54717/diff/2/?file=1583519#file1583519line596>
> >
> > I would probably just get rid of this check here. It looks to me that even if we did the check here, things might change in 'attach'. Why bother checking it here?
What about the case, where the CNI network is actually not found ? IN that case we should just throw an error and not let the prepare succeed right?
- Avinash
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54717/#review159378
-----------------------------------------------------------
On Dec. 14, 2016, 12:28 a.m., Avinash sridharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54717/
> -----------------------------------------------------------
>
> (Updated Dec. 14, 2016, 12:28 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
> -------
>
> If the `network/cni` isolator sees a cache-miss during the `prepare`
> phase, it will try to look for the CNI network on disk before giving
> up. This allows for the dynamic addition of CNI networks without the
> need for agent restart.
>
>
> 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/54717/diff/
>
>
> Testing
> -------
>
> make check
>
> Manual testing with addition of a non-existent CNI configuration.
>
>
> Thanks,
>
> Avinash sridharan
>
>
Re: Review Request 54717: Modified `network/cni` isolator for dynamic
addition of CNI networks.
Posted by Jie Yu <yu...@gmail.com>.
> On Dec. 15, 2016, 10:32 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, lines 596-599
> > <https://reviews.apache.org/r/54717/diff/2/?file=1583519#file1583519line596>
> >
> > I would probably just get rid of this check here. It looks to me that even if we did the check here, things might change in 'attach'. Why bother checking it here?
>
> Avinash sridharan wrote:
> What about the case, where the CNI network is actually not found ? IN that case we should just throw an error and not let the prepare succeed right?
I guess you're right. I think we might need to fail early if the network is indeed not found. What I was thinking is that even if it is not found for now, it might be found in 'attach'. But I guess this should be extremely rare. Feel free to drop the issue. Let's call `getNetworkConfigJson` here.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54717/#review159378
-----------------------------------------------------------
On Dec. 14, 2016, 12:28 a.m., Avinash sridharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54717/
> -----------------------------------------------------------
>
> (Updated Dec. 14, 2016, 12:28 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
> -------
>
> If the `network/cni` isolator sees a cache-miss during the `prepare`
> phase, it will try to look for the CNI network on disk before giving
> up. This allows for the dynamic addition of CNI networks without the
> need for agent restart.
>
>
> 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/54717/diff/
>
>
> Testing
> -------
>
> make check
>
> Manual testing with addition of a non-existent CNI configuration.
>
>
> Thanks,
>
> Avinash sridharan
>
>
Re: Review Request 54717: Modified `network/cni` isolator for dynamic
addition of CNI networks.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54717/#review159378
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 596 - 599)
<https://reviews.apache.org/r/54717/#comment230361>
I would probably just get rid of this check here. It looks to me that even if we did the check here, things might change in 'attach'. Why bother checking it here?
src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 1152 - 1172)
<https://reviews.apache.org/r/54717/#comment230363>
I would introduce a helper:
```
Try<JSON::Object> networkConfigJson = getNetworkConfigJson(networkName);
```
The helper will cross check the name in the config with the cache, and do a reload if necessary.
- Jie Yu
On Dec. 14, 2016, 12:28 a.m., Avinash sridharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54717/
> -----------------------------------------------------------
>
> (Updated Dec. 14, 2016, 12:28 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
> -------
>
> If the `network/cni` isolator sees a cache-miss during the `prepare`
> phase, it will try to look for the CNI network on disk before giving
> up. This allows for the dynamic addition of CNI networks without the
> need for agent restart.
>
>
> 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/54717/diff/
>
>
> Testing
> -------
>
> make check
>
> Manual testing with addition of a non-existent CNI configuration.
>
>
> Thanks,
>
> Avinash sridharan
>
>
Re: Review Request 54717: Modified isolator for dynamic
addition/deletion of CNI networks.
Posted by Avinash sridharan <av...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54717/
-----------------------------------------------------------
(Updated Dec. 19, 2016, 9:53 p.m.)
Review request for mesos, Jie Yu and Qian Zhang.
Changes
-------
Addressed Jie's and Qian's comments.
Bugs: MESOS-6567
https://issues.apache.org/jira/browse/MESOS-6567
Repository: mesos
Description
-------
If the `network/cni` isolator sees a cache-miss during the `prepare`
phase, it will try to look for the CNI network on disk before giving
up. This allows for the dynamic addition of CNI networks without the
need for agent restart.
During `isolate` or `prepare` if for a given network, the isolator is
unable to read the corresponding configuration file, or finds and
error in the existing configuration file, it will remove the network
from the in-memory cache. This allows dynamic deletion of CNI
configurations from the `network/cni` isolator.
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/54717/diff/
Testing
-------
make check
Manual testing with addition of a non-existent CNI configuration.
Thanks,
Avinash sridharan
Re: Review Request 54717: Modified isolator for dynamic
addition/deletion of CNI networks.
Posted by Avinash sridharan <av...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54717/
-----------------------------------------------------------
(Updated Dec. 18, 2016, 2:23 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
-------
If the `network/cni` isolator sees a cache-miss during the `prepare`
phase, it will try to look for the CNI network on disk before giving
up. This allows for the dynamic addition of CNI networks without the
need for agent restart.
During `isolate` or `prepare` if for a given network, the isolator is
unable to read the corresponding configuration file, or finds and
error in the existing configuration file, it will remove the network
from the in-memory cache. This allows dynamic deletion of CNI
configurations from the `network/cni` isolator.
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/54717/diff/
Testing
-------
make check
Manual testing with addition of a non-existent CNI configuration.
Thanks,
Avinash sridharan
Re: Review Request 54717: Modified isolator for dynamic
addition/deletion of CNI networks.
Posted by Qian Zhang <zh...@gmail.com>.
> On Dec. 17, 2016, 11:28 p.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, lines 1159-1170
> > <https://reviews.apache.org/r/54717/diff/4/?file=1588480#file1588480line1159>
> >
> > Just curious in which case `getNetwork` will succeeds but `getNetworkConfigJSON()` will fail?
>
> Avinash sridharan wrote:
> One case I can think of is if the operator ends up removing the file between a call to `getNetwork` and `getNetworkConfigJSON` . Highly unlikely, but possible.
I do not think we need to handle the case like that. In `getNetwork`, you already call `getNetworkConfigJSON` if the `network` is in the hashmap `networkConfigs` and remove the `network` from `networkConfigs` if `getNetworkConfigJSON` fails, I think we do not need to duplicate such logic outside after `getNetwork` is called.
- Qian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54717/#review159554
-----------------------------------------------------------
On Dec. 18, 2016, 10:23 a.m., Avinash sridharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54717/
> -----------------------------------------------------------
>
> (Updated Dec. 18, 2016, 10:23 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
> -------
>
> If the `network/cni` isolator sees a cache-miss during the `prepare`
> phase, it will try to look for the CNI network on disk before giving
> up. This allows for the dynamic addition of CNI networks without the
> need for agent restart.
>
> During `isolate` or `prepare` if for a given network, the isolator is
> unable to read the corresponding configuration file, or finds and
> error in the existing configuration file, it will remove the network
> from the in-memory cache. This allows dynamic deletion of CNI
> configurations from the `network/cni` isolator.
>
>
> 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/54717/diff/
>
>
> Testing
> -------
>
> make check
>
> Manual testing with addition of a non-existent CNI configuration.
>
>
> Thanks,
>
> Avinash sridharan
>
>
Re: Review Request 54717: Modified isolator for dynamic
addition/deletion of CNI networks.
Posted by Avinash sridharan <av...@mesosphere.io>.
> On Dec. 17, 2016, 3:28 p.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, lines 1159-1170
> > <https://reviews.apache.org/r/54717/diff/4/?file=1588480#file1588480line1159>
> >
> > Just curious in which case `getNetwork` will succeeds but `getNetworkConfigJSON()` will fail?
>
> Avinash sridharan wrote:
> One case I can think of is if the operator ends up removing the file between a call to `getNetwork` and `getNetworkConfigJSON` . Highly unlikely, but possible.
>
> Qian Zhang wrote:
> I do not think we need to handle the case like that. In `getNetwork`, you already call `getNetworkConfigJSON` if the `network` is in the hashmap `networkConfigs` and remove the `network` from `networkConfigs` if `getNetworkConfigJSON` fails, I think we do not need to duplicate such logic outside after `getNetwork` is called.
Should be fixed withe overload I introduced. Have another look. Thanks.
- Avinash
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54717/#review159554
-----------------------------------------------------------
On Dec. 19, 2016, 9:53 p.m., Avinash sridharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54717/
> -----------------------------------------------------------
>
> (Updated Dec. 19, 2016, 9:53 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
> -------
>
> If the `network/cni` isolator sees a cache-miss during the `prepare`
> phase, it will try to look for the CNI network on disk before giving
> up. This allows for the dynamic addition of CNI networks without the
> need for agent restart.
>
> During `isolate` or `prepare` if for a given network, the isolator is
> unable to read the corresponding configuration file, or finds and
> error in the existing configuration file, it will remove the network
> from the in-memory cache. This allows dynamic deletion of CNI
> configurations from the `network/cni` isolator.
>
>
> 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/54717/diff/
>
>
> Testing
> -------
>
> make check
>
> Manual testing with addition of a non-existent CNI configuration.
>
>
> Thanks,
>
> Avinash sridharan
>
>
Re: Review Request 54717: Modified isolator for dynamic
addition/deletion of CNI networks.
Posted by Avinash sridharan <av...@mesosphere.io>.
> On Dec. 17, 2016, 3:28 p.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, lines 1159-1170
> > <https://reviews.apache.org/r/54717/diff/4/?file=1588480#file1588480line1159>
> >
> > Just curious in which case `getNetwork` will succeeds but `getNetworkConfigJSON()` will fail?
One case I can think of is if the operator ends up removing the file between a call to `getNetwork` and `getNetworkConfigJSON` . Highly unlikely, but possible.
- Avinash
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54717/#review159554
-----------------------------------------------------------
On Dec. 17, 2016, 8:19 a.m., Avinash sridharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54717/
> -----------------------------------------------------------
>
> (Updated Dec. 17, 2016, 8:19 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
> -------
>
> If the `network/cni` isolator sees a cache-miss during the `prepare`
> phase, it will try to look for the CNI network on disk before giving
> up. This allows for the dynamic addition of CNI networks without the
> need for agent restart.
>
> During `isolate` or `prepare` if for a given network, the isolator is
> unable to read the corresponding configuration file, or finds and
> error in the existing configuration file, it will remove the network
> from the in-memory cache. This allows dynamic deletion of CNI
> configurations from the `network/cni` isolator.
>
>
> 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/54717/diff/
>
>
> Testing
> -------
>
> make check
>
> Manual testing with addition of a non-existent CNI configuration.
>
>
> Thanks,
>
> Avinash sridharan
>
>
Re: Review Request 54717: Modified isolator for dynamic
addition/deletion of CNI networks.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54717/#review159554
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/network/cni/cni.hpp (line 184)
<https://reviews.apache.org/r/54717/#comment230587>
In the implementation of this method, I do not see you validate the existence of the plugin.
src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 1159 - 1170)
<https://reviews.apache.org/r/54717/#comment230588>
Just curious in which case `getNetwork` will succeeds but `getNetworkConfigJSON()` will fail?
- Qian Zhang
On Dec. 17, 2016, 4:19 p.m., Avinash sridharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54717/
> -----------------------------------------------------------
>
> (Updated Dec. 17, 2016, 4:19 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
> -------
>
> If the `network/cni` isolator sees a cache-miss during the `prepare`
> phase, it will try to look for the CNI network on disk before giving
> up. This allows for the dynamic addition of CNI networks without the
> need for agent restart.
>
> During `isolate` or `prepare` if for a given network, the isolator is
> unable to read the corresponding configuration file, or finds and
> error in the existing configuration file, it will remove the network
> from the in-memory cache. This allows dynamic deletion of CNI
> configurations from the `network/cni` isolator.
>
>
> 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/54717/diff/
>
>
> Testing
> -------
>
> make check
>
> Manual testing with addition of a non-existent CNI configuration.
>
>
> Thanks,
>
> Avinash sridharan
>
>
Re: Review Request 54717: Modified isolator for dynamic
addition/deletion of CNI networks.
Posted by Avinash sridharan <av...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54717/
-----------------------------------------------------------
(Updated Dec. 17, 2016, 8:19 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
-------
If the `network/cni` isolator sees a cache-miss during the `prepare`
phase, it will try to look for the CNI network on disk before giving
up. This allows for the dynamic addition of CNI networks without the
need for agent restart.
During `isolate` or `prepare` if for a given network, the isolator is
unable to read the corresponding configuration file, or finds and
error in the existing configuration file, it will remove the network
from the in-memory cache. This allows dynamic deletion of CNI
configurations from the `network/cni` isolator.
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/54717/diff/
Testing
-------
make check
Manual testing with addition of a non-existent CNI configuration.
Thanks,
Avinash sridharan
Re: Review Request 54717: Modified isolator for dynamic
addition/deletion of CNI networks.
Posted by Avinash sridharan <av...@mesosphere.io>.
> On Dec. 17, 2016, 12:09 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, lines 250-252
> > <https://reviews.apache.org/r/54717/diff/3/?file=1587921#file1587921line250>
> >
> > In fact, can we just merge this with `getNetwork` below. We have a single helper `getNetworkConfigJson` which will refresh the cache if necessary.
I was thinking about that as well. The only reason I kept them separate was because I was using `getNetworkConfigJSON` as is in `detach` ? In `detach` we are relying on the checkpointed configuration and not the configuration learnt from `networkConfigs.`
- Avinash
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54717/#review159519
-----------------------------------------------------------
On Dec. 16, 2016, 8:34 p.m., Avinash sridharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54717/
> -----------------------------------------------------------
>
> (Updated Dec. 16, 2016, 8:34 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
> -------
>
> If the `network/cni` isolator sees a cache-miss during the `prepare`
> phase, it will try to look for the CNI network on disk before giving
> up. This allows for the dynamic addition of CNI networks without the
> need for agent restart.
>
> During `isolate` or `prepare` if for a given network, the isolator is
> unable to read the corresponding configuration file, or finds and
> error in the existing configuration file, it will remove the network
> from the in-memory cache. This allows dynamic deletion of CNI
> configurations from the `network/cni` isolator.
>
>
> 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/54717/diff/
>
>
> Testing
> -------
>
> make check
>
> Manual testing with addition of a non-existent CNI configuration.
>
>
> Thanks,
>
> Avinash sridharan
>
>
Re: Review Request 54717: Modified isolator for dynamic
addition/deletion of CNI networks.
Posted by Avinash sridharan <av...@mesosphere.io>.
> On Dec. 17, 2016, 12:09 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, lines 250-252
> > <https://reviews.apache.org/r/54717/diff/3/?file=1587921#file1587921line250>
> >
> > In fact, can we just merge this with `getNetwork` below. We have a single helper `getNetworkConfigJson` which will refresh the cache if necessary.
>
> Avinash sridharan wrote:
> I was thinking about that as well. The only reason I kept them separate was because I was using `getNetworkConfigJSON` as is in `detach` ? In `detach` we are relying on the checkpointed configuration and not the configuration learnt from `networkConfigs.`
Would it make sense if we make the `path` and `option` so the helper would be:
```
Try<JSON::Object> NetworkCniIsolatorProcess::getNetworkConfigJSON(
251
const string& network,
252
Option<string> path = None())
```
If path is not set then we will basically search the `networkConfigs` else we will simply get the JSON config from the path. We can then have a single helper function here.
- Avinash
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54717/#review159519
-----------------------------------------------------------
On Dec. 17, 2016, 8:19 a.m., Avinash sridharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54717/
> -----------------------------------------------------------
>
> (Updated Dec. 17, 2016, 8:19 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
> -------
>
> If the `network/cni` isolator sees a cache-miss during the `prepare`
> phase, it will try to look for the CNI network on disk before giving
> up. This allows for the dynamic addition of CNI networks without the
> need for agent restart.
>
> During `isolate` or `prepare` if for a given network, the isolator is
> unable to read the corresponding configuration file, or finds and
> error in the existing configuration file, it will remove the network
> from the in-memory cache. This allows dynamic deletion of CNI
> configurations from the `network/cni` isolator.
>
>
> 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/54717/diff/
>
>
> Testing
> -------
>
> make check
>
> Manual testing with addition of a non-existent CNI configuration.
>
>
> Thanks,
>
> Avinash sridharan
>
>
Re: Review Request 54717: Modified isolator for dynamic
addition/deletion of CNI networks.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54717/#review159519
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/network/cni/cni.hpp (line 179)
<https://reviews.apache.org/r/54717/#comment230532>
Remove the tailing space.
You should be able to find such errors by using `git diff --check`
src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 250 - 252)
<https://reviews.apache.org/r/54717/#comment230541>
In fact, can we just merge this with `getNetwork` below. We have a single helper `getNetworkConfigJson` which will refresh the cache if necessary.
- Jie Yu
On Dec. 16, 2016, 8:34 p.m., Avinash sridharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54717/
> -----------------------------------------------------------
>
> (Updated Dec. 16, 2016, 8:34 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
> -------
>
> If the `network/cni` isolator sees a cache-miss during the `prepare`
> phase, it will try to look for the CNI network on disk before giving
> up. This allows for the dynamic addition of CNI networks without the
> need for agent restart.
>
> During `isolate` or `prepare` if for a given network, the isolator is
> unable to read the corresponding configuration file, or finds and
> error in the existing configuration file, it will remove the network
> from the in-memory cache. This allows dynamic deletion of CNI
> configurations from the `network/cni` isolator.
>
>
> 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/54717/diff/
>
>
> Testing
> -------
>
> make check
>
> Manual testing with addition of a non-existent CNI configuration.
>
>
> Thanks,
>
> Avinash sridharan
>
>
Re: Review Request 54717: Modified isolator for dynamic
addition/deletion of CNI networks.
Posted by Avinash sridharan <av...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54717/
-----------------------------------------------------------
(Updated Dec. 16, 2016, 8:34 p.m.)
Review request for mesos, Jie Yu and Qian Zhang.
Changes
-------
Addressed Jie's comments.
Summary (updated)
-----------------
Modified isolator for dynamic addition/deletion of CNI networks.
Bugs: MESOS-6567
https://issues.apache.org/jira/browse/MESOS-6567
Repository: mesos
Description (updated)
-------
If the `network/cni` isolator sees a cache-miss during the `prepare`
phase, it will try to look for the CNI network on disk before giving
up. This allows for the dynamic addition of CNI networks without the
need for agent restart.
During `isolate` or `prepare` if for a given network, the isolator is
unable to read the corresponding configuration file, or finds and
error in the existing configuration file, it will remove the network
from the in-memory cache. This allows dynamic deletion of CNI
configurations from the `network/cni` isolator.
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/54717/diff/
Testing
-------
make check
Manual testing with addition of a non-existent CNI configuration.
Thanks,
Avinash sridharan
Re: Review Request 54717: Modified `network/cni` isolator for dynamic
addition of CNI networks.
Posted by Avinash sridharan <av...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54717/
-----------------------------------------------------------
(Updated Dec. 14, 2016, 12:28 a.m.)
Review request for mesos, Jie Yu and Qian Zhang.
Changes
-------
Fixed some stylistic error.s
Bugs: MESOS-6567
https://issues.apache.org/jira/browse/MESOS-6567
Repository: mesos
Description
-------
If the `network/cni` isolator sees a cache-miss during the `prepare`
phase, it will try to look for the CNI network on disk before giving
up. This allows for the dynamic addition of CNI networks without the
need for agent restart.
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/54717/diff/
Testing
-------
make check
Manual testing with addition of a non-existent CNI configuration.
Thanks,
Avinash sridharan