Re: Review Request 44200: Add agent flags for specifying CNI plugin and config directories.

[Jie Yu]

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


Fix it, then Ship it!





src/slave/flags.hpp (lines 133 - 134)


Can you put these right under `network_enable_snmp_statistics` so that all 
network related knobs are grouped together?



src/slave/flags.cpp (line 694)


Ditto on ordering.


- Jie Yu


On March 4, 2016, 4:14 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44200/
> ---
> 
> (Updated March 4, 2016, 4:14 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4761
> https://issues.apache.org/jira/browse/MESOS-4761
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add agent flags for specifying CNI plugin and config directories.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 305ba2c801c2060db6dcb4ef83c1043aaa7d520c 
>   src/slave/flags.hpp feb095da4521f678c96f4cc53bdfda262d350388 
>   src/slave/flags.cpp 6e3fd69c06eefd40bc0e5c222ea72f34144c5534 
> 
> Diff: https://reviews.apache.org/r/44200/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44200: Add agent flags for specifying CNI plugin and config directories.

[Gilbert Song]

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


Ship it!




Ship It!

- Gilbert Song


On March 4, 2016, 8:14 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44200/
> ---
> 
> (Updated March 4, 2016, 8:14 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4761
> https://issues.apache.org/jira/browse/MESOS-4761
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add agent flags for specifying CNI plugin and config directories.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 305ba2c801c2060db6dcb4ef83c1043aaa7d520c 
>   src/slave/flags.hpp feb095da4521f678c96f4cc53bdfda262d350388 
>   src/slave/flags.cpp 6e3fd69c06eefd40bc0e5c222ea72f34144c5534 
> 
> Diff: https://reviews.apache.org/r/44200/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44200: Add agent flags for specifying CNI plugin and config directories.

[Avinash sridharan]

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


Ship it!




Ship It!

- Avinash sridharan


On March 4, 2016, 4:14 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44200/
> ---
> 
> (Updated March 4, 2016, 4:14 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4761
> https://issues.apache.org/jira/browse/MESOS-4761
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add agent flags for specifying CNI plugin and config directories.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 305ba2c801c2060db6dcb4ef83c1043aaa7d520c 
>   src/slave/flags.hpp feb095da4521f678c96f4cc53bdfda262d350388 
>   src/slave/flags.cpp 6e3fd69c06eefd40bc0e5c222ea72f34144c5534 
> 
> Diff: https://reviews.apache.org/r/44200/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44200: Add agent flags for specifying CNI plugin and config directories.

[Qian Zhang]

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

(Updated March 5, 2016, 12:14 a.m.)


Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.


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


Repository: mesos


Description
---

Add agent flags for specifying CNI plugin and config directories.


Diffs (updated)
-

  docs/configuration.md 305ba2c801c2060db6dcb4ef83c1043aaa7d520c 
  src/slave/flags.hpp feb095da4521f678c96f4cc53bdfda262d350388 
  src/slave/flags.cpp 6e3fd69c06eefd40bc0e5c222ea72f34144c5534 

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


Testing
---

make check


Thanks,

Qian Zhang

Re: Review Request 44200: Add agent flags for specifying CNI plugin and config directories.

[Avinash sridharan]

> On March 2, 2016, 4:07 p.m., Avinash sridharan wrote:
> > src/slave/flags.cpp, line 694
> > 
> >
> > s/directory/location
> > 
> > remove this line:
> > This flag is used for\n"
> >   "the `network/cni` isolator.\n",
> >   "/tmp/mesos/cni/plugins"
> 
> Qian Zhang wrote:
> Did you mean removing the line: `This flag is used for the 'network/cni' 
> isolator.`? I think this is the existing convention, please see the help 
> message of `Flags::eth0_name`, `Flags::network_enable_snmp_statistics`, 
> `Flags::container_disk_watch_interval`, etc. They all have the line like: 
> `This flag is used for the 'xxx' isolator.`.
> 
> Avinash sridharan wrote:
> Not sure what you mean by convention in help strings? The sentence is 
> redundant (doesn't given any more information than the previous sentences). 
> Lets use the correct form here.  Help strings should be concise.
> 
> Qian Zhang wrote:
> What I mean by convention in help strings is, if a flag is specific to an 
> isolator, then we should mention that in the help strings of that flag, 
> please see the following code for an example (and there are more in agent's 
> flags):
> https://github.com/apache/mesos/blob/0.27.1/src/slave/flags.cpp#L586:L587
> 
> So your comment is that I should remove the line `This flag is used for 
> the 'network/cni' isolator.` from the help strings of both 
> `Flags::network_cni_plugins_dir` and `Flags::network_cni_config_dir`?

Yup. Just remove that line.


- Avinash


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


On March 4, 2016, 2:36 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44200/
> ---
> 
> (Updated March 4, 2016, 2:36 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4761
> https://issues.apache.org/jira/browse/MESOS-4761
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add agent flags for specifying CNI plugin and config directories.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 305ba2c801c2060db6dcb4ef83c1043aaa7d520c 
>   src/slave/flags.hpp feb095da4521f678c96f4cc53bdfda262d350388 
>   src/slave/flags.cpp 6e3fd69c06eefd40bc0e5c222ea72f34144c5534 
> 
> Diff: https://reviews.apache.org/r/44200/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44200: Add agent flags for specifying CNI plugin and config directories.

[Qian Zhang]

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

(Updated March 4, 2016, 10:36 a.m.)


Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.


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


Repository: mesos


Description
---

Add agent flags for specifying CNI plugin and config directories.


Diffs (updated)
-

  docs/configuration.md 305ba2c801c2060db6dcb4ef83c1043aaa7d520c 
  src/slave/flags.hpp feb095da4521f678c96f4cc53bdfda262d350388 
  src/slave/flags.cpp 6e3fd69c06eefd40bc0e5c222ea72f34144c5534 

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


Testing
---

make check


Thanks,

Qian Zhang

Re: Review Request 44200: Add agent flags for specifying CNI plugin and config directories.

[Qian Zhang]

> On March 3, 2016, 12:07 a.m., Avinash sridharan wrote:
> > src/slave/flags.cpp, line 694
> > 
> >
> > s/directory/location
> > 
> > remove this line:
> > This flag is used for\n"
> >   "the `network/cni` isolator.\n",
> >   "/tmp/mesos/cni/plugins"
> 
> Qian Zhang wrote:
> Did you mean removing the line: `This flag is used for the 'network/cni' 
> isolator.`? I think this is the existing convention, please see the help 
> message of `Flags::eth0_name`, `Flags::network_enable_snmp_statistics`, 
> `Flags::container_disk_watch_interval`, etc. They all have the line like: 
> `This flag is used for the 'xxx' isolator.`.
> 
> Avinash sridharan wrote:
> Not sure what you mean by convention in help strings? The sentence is 
> redundant (doesn't given any more information than the previous sentences). 
> Lets use the correct form here.  Help strings should be concise.

What I mean by convention in help strings is, if a flag is specific to an 
isolator, then we should mention that in the help strings of that flag, please 
see the following code for an example (and there are more in agent's flags):
https://github.com/apache/mesos/blob/0.27.1/src/slave/flags.cpp#L586:L587

So your comment is that I should remove the line `This flag is used for the 
'network/cni' isolator.` from the help strings of both 
`Flags::network_cni_plugins_dir` and `Flags::network_cni_config_dir`?


> On March 3, 2016, 12:07 a.m., Avinash sridharan wrote:
> > docs/endpoints/slave/state.json.md, line 91
> > 
> >
> > Do we need to change some install scripts to create these paths? I 
> > think we should leave the paths as empty. Giving a default path implies 
> > that the path should exist during mesos installation. Leaving the path 
> > empty would force the operator to specify it while launching `network/cni` 
> > isolator.
> 
> Qian Zhang wrote:
> I think operator will be forced to specify it if the default path 
> `/tmp/mesos/cni/plugins` does not exist because there is a check in 
> `NetworkCniIsolatorProcess::create()`, please see 
> https://reviews.apache.org/r/44269/ for details, if the default path does not 
> exist, the agent will eventually exit with a meaningful error message to 
> operator.
> 
> Avinash sridharan wrote:
> Firstly having an install path for plugins in /tmp doesn't make sense. 
> /tmp is usually mounted as tmpfs (ramfs) so its not going to be persistent 
> across reboots.  Also, if we are not creating /tmp/mesos/cni/plugins then by 
> default this path is not going to exist, so having a default path here does 
> not make sense. Lets keep the default path as empty, and as you mentioned, 
> let the isolator bail out if the path is not specified.

OK, let me make it as no default value, and accordingly I will remove the 
following two lines from state.json.md and state.md since it seems the flags 
without default value should not appear there.
`"network_cni_plugins_dir" : "/tmp/mesos/cni/plugins",`
`"network_cni_config_dir" : "/tmp/mesos/cni/net_configs",`


> On March 3, 2016, 12:07 a.m., Avinash sridharan wrote:
> > docs/endpoints/slave/state.md, line 91
> > 
> >
> > We should leave the directories empty.

Ditto.


- Qian


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


On March 1, 2016, 3:04 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44200/
> ---
> 
> (Updated March 1, 2016, 3:04 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4761
> https://issues.apache.org/jira/browse/MESOS-4761
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add agent flags for specifying CNI plugin and config directories.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md c4d094aac4acb95fd6648071413a3d5160dbf381 
>   docs/endpoints/slave/state.json.md a6403eea3426bdec81d170d0829ffe7174862e55 
>   docs/endpoints/slave/state.md bdd555e0231935c668dea1ba63582348db2b3629 
>   src/slave/flags.hpp c07932157ffe36ea36f1def4c7d92b79d2219c48 
>   src/slave/flags.cpp 7f139e99e8392fe95bb30070896ed9d95dfe5701 
>   src/slave/http.cpp 4eb1fafdfa72094511b0b2684a3c2705bd8c7c5e 
> 
> Diff: https://reviews.apache.org/r/44200/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44200: Add agent flags for specifying CNI plugin and config directories.

[Qian Zhang]

> On March 2, 2016, 1:03 a.m., Gilbert Song wrote:
> > src/slave/flags.cpp, lines 698-705
> > 
> >
> > While users can config the CNI network form a JSON file, is it possible 
> > to provide an option to define the CNI config from cli?
> 
> Qian Zhang wrote:
> Yes, I think it is possible and there are already a couple of flags 
> defined in this way, e.g., `--resources`, `--credential`. @Jie, do you think 
> we should switch to this way?
> 
> Avinash sridharan wrote:
> I think this question was raised during the design review. Jie had 
> mentioned its a bit of a pain to parse JSON config from command line and 
> doesn't add any functional value to the MVP at this point. Its just semanitc 
> sugar that can be done later.

OK, then let's stick on the original design.


- Qian


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


On March 1, 2016, 3:04 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44200/
> ---
> 
> (Updated March 1, 2016, 3:04 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4761
> https://issues.apache.org/jira/browse/MESOS-4761
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add agent flags for specifying CNI plugin and config directories.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md c4d094aac4acb95fd6648071413a3d5160dbf381 
>   docs/endpoints/slave/state.json.md a6403eea3426bdec81d170d0829ffe7174862e55 
>   docs/endpoints/slave/state.md bdd555e0231935c668dea1ba63582348db2b3629 
>   src/slave/flags.hpp c07932157ffe36ea36f1def4c7d92b79d2219c48 
>   src/slave/flags.cpp 7f139e99e8392fe95bb30070896ed9d95dfe5701 
>   src/slave/http.cpp 4eb1fafdfa72094511b0b2684a3c2705bd8c7c5e 
> 
> Diff: https://reviews.apache.org/r/44200/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44200: Add agent flags for specifying CNI plugin and config directories.

[Avinash sridharan]

> On March 1, 2016, 5:03 p.m., Gilbert Song wrote:
> > src/slave/flags.cpp, lines 698-705
> > 
> >
> > While users can config the CNI network form a JSON file, is it possible 
> > to provide an option to define the CNI config from cli?
> 
> Qian Zhang wrote:
> Yes, I think it is possible and there are already a couple of flags 
> defined in this way, e.g., `--resources`, `--credential`. @Jie, do you think 
> we should switch to this way?

I think this question was raised during the design review. Jie had mentioned 
its a bit of a pain to parse JSON config from command line and doesn't add any 
functional value to the MVP at this point. Its just semanitc sugar that can be 
done later.


- Avinash


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


On March 1, 2016, 7:04 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44200/
> ---
> 
> (Updated March 1, 2016, 7:04 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4761
> https://issues.apache.org/jira/browse/MESOS-4761
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add agent flags for specifying CNI plugin and config directories.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md c4d094aac4acb95fd6648071413a3d5160dbf381 
>   docs/endpoints/slave/state.json.md a6403eea3426bdec81d170d0829ffe7174862e55 
>   docs/endpoints/slave/state.md bdd555e0231935c668dea1ba63582348db2b3629 
>   src/slave/flags.hpp c07932157ffe36ea36f1def4c7d92b79d2219c48 
>   src/slave/flags.cpp 7f139e99e8392fe95bb30070896ed9d95dfe5701 
>   src/slave/http.cpp 4eb1fafdfa72094511b0b2684a3c2705bd8c7c5e 
> 
> Diff: https://reviews.apache.org/r/44200/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44200: Add agent flags for specifying CNI plugin and config directories.

[Avinash sridharan]

> On March 2, 2016, 4:07 p.m., Avinash sridharan wrote:
> > docs/endpoints/slave/state.json.md, line 91
> > 
> >
> > Do we need to change some install scripts to create these paths? I 
> > think we should leave the paths as empty. Giving a default path implies 
> > that the path should exist during mesos installation. Leaving the path 
> > empty would force the operator to specify it while launching `network/cni` 
> > isolator.
> 
> Qian Zhang wrote:
> I think operator will be forced to specify it if the default path 
> `/tmp/mesos/cni/plugins` does not exist because there is a check in 
> `NetworkCniIsolatorProcess::create()`, please see 
> https://reviews.apache.org/r/44269/ for details, if the default path does not 
> exist, the agent will eventually exit with a meaningful error message to 
> operator.

Firstly having an install path for plugins in /tmp doesn't make sense. /tmp is 
usually mounted as tmpfs (ramfs) so its not going to be persistent across 
reboots.  Also, if we are not creating /tmp/mesos/cni/plugins then by default 
this path is not going to exist, so having a default path here does not make 
sense. Lets keep the default path as empty, and as you mentioned, let the 
isolator bail out if the path is not specified.


> On March 2, 2016, 4:07 p.m., Avinash sridharan wrote:
> > src/slave/flags.cpp, line 694
> > 
> >
> > s/directory/location
> > 
> > remove this line:
> > This flag is used for\n"
> >   "the `network/cni` isolator.\n",
> >   "/tmp/mesos/cni/plugins"
> 
> Qian Zhang wrote:
> Did you mean removing the line: `This flag is used for the 'network/cni' 
> isolator.`? I think this is the existing convention, please see the help 
> message of `Flags::eth0_name`, `Flags::network_enable_snmp_statistics`, 
> `Flags::container_disk_watch_interval`, etc. They all have the line like: 
> `This flag is used for the 'xxx' isolator.`.

Not sure what you mean by convention in help strings? The sentence is redundant 
(doesn't given any more information than the previous sentences). Lets use the 
correct form here.  Help strings should be concise.


- Avinash


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


On March 1, 2016, 7:04 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44200/
> ---
> 
> (Updated March 1, 2016, 7:04 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4761
> https://issues.apache.org/jira/browse/MESOS-4761
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add agent flags for specifying CNI plugin and config directories.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md c4d094aac4acb95fd6648071413a3d5160dbf381 
>   docs/endpoints/slave/state.json.md a6403eea3426bdec81d170d0829ffe7174862e55 
>   docs/endpoints/slave/state.md bdd555e0231935c668dea1ba63582348db2b3629 
>   src/slave/flags.hpp c07932157ffe36ea36f1def4c7d92b79d2219c48 
>   src/slave/flags.cpp 7f139e99e8392fe95bb30070896ed9d95dfe5701 
>   src/slave/http.cpp 4eb1fafdfa72094511b0b2684a3c2705bd8c7c5e 
> 
> Diff: https://reviews.apache.org/r/44200/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44200: Add agent flags for specifying CNI plugin and config directories.

[Qian Zhang]

> On March 3, 2016, 12:07 a.m., Avinash sridharan wrote:
> > src/slave/flags.cpp, line 690
> > 
> >
> > Maybe s/Directory path/Location ?

I just followed the existing convention, please see the help message of 
`Flags::launcher_dir` and `Flags::work_dir`, they all use the words "Directory 
path".


> On March 3, 2016, 12:07 a.m., Avinash sridharan wrote:
> > src/slave/flags.cpp, line 694
> > 
> >
> > s/directory/location
> > 
> > remove this line:
> > This flag is used for\n"
> >   "the `network/cni` isolator.\n",
> >   "/tmp/mesos/cni/plugins"

Did you mean removing the line: `This flag is used for the 'network/cni' 
isolator.`? I think this is the existing convention, please see the help 
message of `Flags::eth0_name`, `Flags::network_enable_snmp_statistics`, 
`Flags::container_disk_watch_interval`, etc. They all have the line like: `This 
flag is used for the 'xxx' isolator.`.


> On March 3, 2016, 12:07 a.m., Avinash sridharan wrote:
> > docs/configuration.md, line 1305
> > 
> >
> > Update the documentation based on the comments in `flags.cpp`

Thanks for your comments in `flags.cpp` :-) I have replied your comments there, 
please let me know if you have further comments.


> On March 3, 2016, 12:07 a.m., Avinash sridharan wrote:
> > docs/endpoints/slave/state.json.md, line 91
> > 
> >
> > Do we need to change some install scripts to create these paths? I 
> > think we should leave the paths as empty. Giving a default path implies 
> > that the path should exist during mesos installation. Leaving the path 
> > empty would force the operator to specify it while launching `network/cni` 
> > isolator.

I think operator will be forced to specify it if the default path 
`/tmp/mesos/cni/plugins` does not exist because there is a check in 
`NetworkCniIsolatorProcess::create()`, please see 
https://reviews.apache.org/r/44269/ for details, if the default path does not 
exist, the agent will eventually exit with a meaningful error message to 
operator.


- Qian


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


On March 1, 2016, 3:04 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44200/
> ---
> 
> (Updated March 1, 2016, 3:04 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4761
> https://issues.apache.org/jira/browse/MESOS-4761
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add agent flags for specifying CNI plugin and config directories.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md c4d094aac4acb95fd6648071413a3d5160dbf381 
>   docs/endpoints/slave/state.json.md a6403eea3426bdec81d170d0829ffe7174862e55 
>   docs/endpoints/slave/state.md bdd555e0231935c668dea1ba63582348db2b3629 
>   src/slave/flags.hpp c07932157ffe36ea36f1def4c7d92b79d2219c48 
>   src/slave/flags.cpp 7f139e99e8392fe95bb30070896ed9d95dfe5701 
>   src/slave/http.cpp 4eb1fafdfa72094511b0b2684a3c2705bd8c7c5e 
> 
> Diff: https://reviews.apache.org/r/44200/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44200: Add agent flags for specifying CNI plugin and config directories.

[Qian Zhang]

> On March 2, 2016, 1:03 a.m., Gilbert Song wrote:
> > src/slave/flags.hpp, lines 132-133
> > 
> >
> > Thinking about a proper naming. Consider remove `_dir`?

I see there are already a couple of flags named with `_dir` as suffix, e.g., 
`appc_store_dir`, `launcher_dir`, etc., so I just followed them :-)


> On March 2, 2016, 1:03 a.m., Gilbert Song wrote:
> > src/slave/flags.cpp, lines 698-705
> > 
> >
> > While users can config the CNI network form a JSON file, is it possible 
> > to provide an option to define the CNI config from cli?

Yes, I think it is possible and there are already a couple of flags defined in 
this way, e.g., `--resources`, `--credential`. @Jie, do you think we should 
switch to this way?


- Qian


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


On March 1, 2016, 3:04 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44200/
> ---
> 
> (Updated March 1, 2016, 3:04 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4761
> https://issues.apache.org/jira/browse/MESOS-4761
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add agent flags for specifying CNI plugin and config directories.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md c4d094aac4acb95fd6648071413a3d5160dbf381 
>   docs/endpoints/slave/state.json.md a6403eea3426bdec81d170d0829ffe7174862e55 
>   docs/endpoints/slave/state.md bdd555e0231935c668dea1ba63582348db2b3629 
>   src/slave/flags.hpp c07932157ffe36ea36f1def4c7d92b79d2219c48 
>   src/slave/flags.cpp 7f139e99e8392fe95bb30070896ed9d95dfe5701 
>   src/slave/http.cpp 4eb1fafdfa72094511b0b2684a3c2705bd8c7c5e 
> 
> Diff: https://reviews.apache.org/r/44200/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44200: Add agent flags for specifying CNI plugin and config directories.

[Avinash sridharan]

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




docs/configuration.md (line 1305)


Update the documentation based on the comments in `flags.cpp`



docs/endpoints/slave/state.json.md (line 91)


Do we need to change some install scripts to create these paths? I think we 
should leave the paths as empty. Giving a default path implies that the path 
should exist during mesos installation. Leaving the path empty would force the 
operator to specify it while launching `network/cni` isolator.



docs/endpoints/slave/state.md (line 91)


We should leave the directories empty.



src/slave/flags.cpp (line 690)


Maybe s/Directory path/Location ?



src/slave/flags.cpp (line 694)


s/directory/location

remove this line:
This flag is used for\n"
  "the `network/cni` isolator.\n",
  "/tmp/mesos/cni/plugins"



src/slave/http.cpp (line 476)


Should not have default values for the directories.



src/slave/http.cpp (lines 476 - 477)


Should not have default values for the directories.


- Avinash sridharan


On March 1, 2016, 7:04 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44200/
> ---
> 
> (Updated March 1, 2016, 7:04 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4761
> https://issues.apache.org/jira/browse/MESOS-4761
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add agent flags for specifying CNI plugin and config directories.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md c4d094aac4acb95fd6648071413a3d5160dbf381 
>   docs/endpoints/slave/state.json.md a6403eea3426bdec81d170d0829ffe7174862e55 
>   docs/endpoints/slave/state.md bdd555e0231935c668dea1ba63582348db2b3629 
>   src/slave/flags.hpp c07932157ffe36ea36f1def4c7d92b79d2219c48 
>   src/slave/flags.cpp 7f139e99e8392fe95bb30070896ed9d95dfe5701 
>   src/slave/http.cpp 4eb1fafdfa72094511b0b2684a3c2705bd8c7c5e 
> 
> Diff: https://reviews.apache.org/r/44200/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44200: Add agent flags for specifying CNI plugin and config directories.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [44200]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On March 1, 2016, 7:04 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44200/
> ---
> 
> (Updated March 1, 2016, 7:04 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4761
> https://issues.apache.org/jira/browse/MESOS-4761
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add agent flags for specifying CNI plugin and config directories.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md c4d094aac4acb95fd6648071413a3d5160dbf381 
>   docs/endpoints/slave/state.json.md a6403eea3426bdec81d170d0829ffe7174862e55 
>   docs/endpoints/slave/state.md bdd555e0231935c668dea1ba63582348db2b3629 
>   src/slave/flags.hpp c07932157ffe36ea36f1def4c7d92b79d2219c48 
>   src/slave/flags.cpp 7f139e99e8392fe95bb30070896ed9d95dfe5701 
>   src/slave/http.cpp 4eb1fafdfa72094511b0b2684a3c2705bd8c7c5e 
> 
> Diff: https://reviews.apache.org/r/44200/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Review Request 44200: Add agent flags for specifying CNI plugin and config directories.

[Qian Zhang]

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

Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.


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


Repository: mesos


Description
---

Add agent flags for specifying CNI plugin and config directories.


Diffs
-

  docs/configuration.md c4d094aac4acb95fd6648071413a3d5160dbf381 
  docs/endpoints/slave/state.json.md a6403eea3426bdec81d170d0829ffe7174862e55 
  docs/endpoints/slave/state.md bdd555e0231935c668dea1ba63582348db2b3629 
  src/slave/flags.hpp c07932157ffe36ea36f1def4c7d92b79d2219c48 
  src/slave/flags.cpp 7f139e99e8392fe95bb30070896ed9d95dfe5701 
  src/slave/http.cpp 4eb1fafdfa72094511b0b2684a3c2705bd8c7c5e 

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


Testing
---

make check


Thanks,

Qian Zhang