> On March 3, 2016, 12:07 a.m., Avinash sridharan wrote:
> > src/slave/flags.cpp, line 694
> > <https://reviews.apache.org/r/44200/diff/1/?file=1275006#file1275006line694>
> >
> >     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
> > <https://reviews.apache.org/r/44200/diff/1/?file=1275003#file1275003line91>
> >
> >     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
> > <https://reviews.apache.org/r/44200/diff/1/?file=1275004#file1275004line91>
> >
> >     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
> 
>

Reply via email to