Review Request 52671: Modified the `--network_cni_plugins_dir` flag.

[Avinash sridharan]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52671/
---

Review request for mesos, Jie Yu and Qian Zhang.


Bugs: MESOS-6344
https://issues.apache.org/jira/browse/MESOS-6344


Repository: mesos


Description
---

The `--network_cni_plugins_dir` was initially designed to take in a
single directory where all the CNI plugins were expected to be
present. This however is limiting since the operator will have to
ensure that all 3rd party plugins are installed in the same location
which a very hard constraint.

To make things simpler we are therefore converting the
`--network_cni_plugins_dir` from a single directory into a search
path.


Diffs
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
1b22b28825e8160f659c3cbac37cc576f01666d5 

Diff: https://reviews.apache.org/r/52671/diff/


Testing
---

make, make check and sudo ./bin/mesos-tests.sh --gtest_filter=Cni*

Also ran a single node cluster and tested the flags by moving the bridge plugin 
from directory to another.


Thanks,

Avinash sridharan

Re: Review Request 52671: Modified the `--network_cni_plugins_dir` flag.

[Mesos ReviewBot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52671/#review151952
---



Patch looks great!

Reviews applied: [52671]

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 Oct. 10, 2016, 12:01 a.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52671/
> ---
> 
> (Updated Oct. 10, 2016, 12:01 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Qian Zhang.
> 
> 
> Bugs: MESOS-6344
> https://issues.apache.org/jira/browse/MESOS-6344
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `--network_cni_plugins_dir` was initially designed to take in a
> single directory where all the CNI plugins were expected to be
> present. This however is limiting since the operator will have to
> ensure that all 3rd party plugins are installed in the same location
> which a very hard constraint.
> 
> To make things simpler we are therefore converting the
> `--network_cni_plugins_dir` from a single directory into a search
> path.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 1b22b28825e8160f659c3cbac37cc576f01666d5 
> 
> Diff: https://reviews.apache.org/r/52671/diff/
> 
> 
> Testing
> ---
> 
> make, make check and sudo ./bin/mesos-tests.sh --gtest_filter=Cni*
> 
> Also ran a single node cluster and tested the flags by moving the bridge 
> plugin from directory to another.
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 52671: Modified the `--network_cni_plugins_dir` flag.

[Avinash sridharan]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52671/
---

(Updated Oct. 12, 2016, 5:49 a.m.)


Review request for mesos, Jie Yu and Qian Zhang.


Changes
---

Fixed a bug and rebased.


Bugs: MESOS-6344
https://issues.apache.org/jira/browse/MESOS-6344


Repository: mesos


Description
---

The `--network_cni_plugins_dir` was initially designed to take in a
single directory where all the CNI plugins were expected to be
present. This however is limiting since the operator will have to
ensure that all 3rd party plugins are installed in the same location
which a very hard constraint.

To make things simpler we are therefore converting the
`--network_cni_plugins_dir` from a single directory into a search
path.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
1b22b28825e8160f659c3cbac37cc576f01666d5 
  src/slave/flags.cpp 491d10f6a8a7ea8adbfe0a09f5fce79943bccfac 

Diff: https://reviews.apache.org/r/52671/diff/


Testing
---

make, make check and sudo ./bin/mesos-tests.sh --gtest_filter=Cni*

Also ran a single node cluster and tested the flags by moving the bridge plugin 
from directory to another.


Thanks,

Avinash sridharan

Re: Review Request 52671: Modified the `--network_cni_plugins_dir` flag.

[Jie Yu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52671/#review152629
---


Fix it, then Ship it!





src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 145)


insert a line above.



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 149)


`if (plugin.isNone())`



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 155 - 166)


Do you still need this? os::which already did the check for you.



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 170)


insert a line above



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 174)


Ditto.



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 180 - 191)


Ditto.



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 1160)


Ditto.



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 1487)


Ditto.


- Jie Yu


On Oct. 12, 2016, 5:49 a.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52671/
> ---
> 
> (Updated Oct. 12, 2016, 5:49 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Qian Zhang.
> 
> 
> Bugs: MESOS-6344
> https://issues.apache.org/jira/browse/MESOS-6344
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `--network_cni_plugins_dir` was initially designed to take in a
> single directory where all the CNI plugins were expected to be
> present. This however is limiting since the operator will have to
> ensure that all 3rd party plugins are installed in the same location
> which a very hard constraint.
> 
> To make things simpler we are therefore converting the
> `--network_cni_plugins_dir` from a single directory into a search
> path.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 1b22b28825e8160f659c3cbac37cc576f01666d5 
>   src/slave/flags.cpp 491d10f6a8a7ea8adbfe0a09f5fce79943bccfac 
> 
> Diff: https://reviews.apache.org/r/52671/diff/
> 
> 
> Testing
> ---
> 
> make, make check and sudo ./bin/mesos-tests.sh --gtest_filter=Cni*
> 
> Also ran a single node cluster and tested the flags by moving the bridge 
> plugin from directory to another.
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 52671: Modified the `--network_cni_plugins_dir` flag.

[Avinash sridharan]

> On Oct. 14, 2016, 4:34 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, lines 198-209
> > 
> >
> > Ditto.

Yeah we actually don't need this anymore. Didn't realize `os::which` also 
checks the permissions on the binary, and not just its existence in the path.


- Avinash


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52671/#review152629
---


On Oct. 12, 2016, 5:49 a.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52671/
> ---
> 
> (Updated Oct. 12, 2016, 5:49 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Qian Zhang.
> 
> 
> Bugs: MESOS-6344
> https://issues.apache.org/jira/browse/MESOS-6344
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `--network_cni_plugins_dir` was initially designed to take in a
> single directory where all the CNI plugins were expected to be
> present. This however is limiting since the operator will have to
> ensure that all 3rd party plugins are installed in the same location
> which a very hard constraint.
> 
> To make things simpler we are therefore converting the
> `--network_cni_plugins_dir` from a single directory into a search
> path.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 1b22b28825e8160f659c3cbac37cc576f01666d5 
>   src/slave/flags.cpp 491d10f6a8a7ea8adbfe0a09f5fce79943bccfac 
> 
> Diff: https://reviews.apache.org/r/52671/diff/
> 
> 
> Testing
> ---
> 
> make, make check and sudo ./bin/mesos-tests.sh --gtest_filter=Cni*
> 
> Also ran a single node cluster and tested the flags by moving the bridge 
> plugin from directory to another.
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 52671: Modified the `--network_cni_plugins_dir` flag.

[Avinash sridharan]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52671/
---

(Updated Oct. 14, 2016, 4:01 p.m.)


Review request for mesos, Jie Yu and Qian Zhang.


Changes
---

Addressed Jie's comments.


Bugs: MESOS-6344
https://issues.apache.org/jira/browse/MESOS-6344


Repository: mesos


Description
---

The `--network_cni_plugins_dir` was initially designed to take in a
single directory where all the CNI plugins were expected to be
present. This however is limiting since the operator will have to
ensure that all 3rd party plugins are installed in the same location
which a very hard constraint.

To make things simpler we are therefore converting the
`--network_cni_plugins_dir` from a single directory into a search
path.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
1b22b28825e8160f659c3cbac37cc576f01666d5 
  src/slave/flags.cpp 7f79cfcc7939680c38a3d0cd57471cc9976aff7c 

Diff: https://reviews.apache.org/r/52671/diff/


Testing
---

make, make check and sudo ./bin/mesos-tests.sh --gtest_filter=Cni*

Also ran a single node cluster and tested the flags by moving the bridge plugin 
from directory to another.


Thanks,

Avinash sridharan