mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Qian Zhang <zhang...@cn.ibm.com>
Subject Re: Review Request 44200: Add agent flags for specifying CNI plugin and config directories.
Date Fri, 04 Mar 2016 02:26:16 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message