mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Avinash sridharan <avin...@mesosphere.io>
Subject Re: Review Request 44200: Add agent flags for specifying CNI plugin and config directories.
Date Thu, 03 Mar 2016 16:51:32 GMT


> On March 2, 2016, 4:07 p.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.

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

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


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