Re: Review Request 69286: Allowed creating non-existing host paths for Mesos Containerizer.

2019-02-21 Thread Gilbert Song 

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


Ship it!




Ship It!

- Gilbert Song


On Feb. 21, 2019, 5:43 p.m., Jason Lai wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69286/
> ---
> 
> (Updated Feb. 21, 2019, 5:43 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Eric Chung, Gilbert Song, Jie Yu, 
> and James Peach.
> 
> 
> Bugs: MESOS-9009
> https://issues.apache.org/jira/browse/MESOS-9009
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a new agent flag `--host_path_volume_force_creation` for
> the `volume/host_path` isolator. The flag takes a colon-separated
> whitelist of paths, under which non-existing host paths are allowed to
> be created.
> 
> If the flag is not specified, the isolator behaves in the original way
> of prohibiting all non-existing host paths from being created.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 732368293049b7d9d6f62057344d433637ad44e8 
>   src/Makefile.am c17eae4ff1d019d515f67d81821e933ecb5dc190 
>   src/slave/containerizer/mesos/isolators/volume/host_path.hpp 
> 4b509e91a056381ca90293d16a400ea4368234a3 
>   src/slave/containerizer/mesos/isolators/volume/host_path.cpp 
> 88ecf91d91e2bebd484a4ac94510a14b3500dbfb 
>   src/slave/containerizer/mesos/isolators/volume/utils.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/volume/utils.cpp PRE-CREATION 
>   src/slave/flags.hpp 29d8b7985ffde57da02b5fe0d3a524e98acc27c8 
>   src/slave/flags.cpp ccaf65029ec2d0e78041fc3992a0bf5ca0798686 
> 
> 
> Diff: https://reviews.apache.org/r/69286/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jason Lai
> 
>

Re: Review Request 69286: Allowed creating non-existing host paths for Mesos Containerizer.

2019-02-21 Thread Jason Lai 

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

(Updated Feb. 22, 2019, 1:43 a.m.)


Review request for mesos, Chun-Hung Hsiao, Eric Chung, Gilbert Song, Jie Yu, 
and James Peach.


Changes
---

Address review comment from @gilbert.


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


Repository: mesos


Description
---

Added a new agent flag `--host_path_volume_force_creation` for
the `volume/host_path` isolator. The flag takes a colon-separated
whitelist of paths, under which non-existing host paths are allowed to
be created.

If the flag is not specified, the isolator behaves in the original way
of prohibiting all non-existing host paths from being created.


Diffs (updated)
-

  src/CMakeLists.txt 732368293049b7d9d6f62057344d433637ad44e8 
  src/Makefile.am c17eae4ff1d019d515f67d81821e933ecb5dc190 
  src/slave/containerizer/mesos/isolators/volume/host_path.hpp 
4b509e91a056381ca90293d16a400ea4368234a3 
  src/slave/containerizer/mesos/isolators/volume/host_path.cpp 
88ecf91d91e2bebd484a4ac94510a14b3500dbfb 
  src/slave/containerizer/mesos/isolators/volume/utils.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/volume/utils.cpp PRE-CREATION 
  src/slave/flags.hpp 29d8b7985ffde57da02b5fe0d3a524e98acc27c8 
  src/slave/flags.cpp ccaf65029ec2d0e78041fc3992a0bf5ca0798686 


Diff: https://reviews.apache.org/r/69286/diff/4/

Changes: https://reviews.apache.org/r/69286/diff/3-4/


Testing
---


Thanks,

Jason Lai

Re: Review Request 69286: Allowed creating non-existing host paths for Mesos Containerizer.

2019-02-21 Thread Jason Lai 


> On Feb. 21, 2019, 11:58 p.m., Gilbert Song wrote:
> > src/slave/containerizer/mesos/isolators/volume/host_path.cpp
> > Lines 165-167 (original), 181-183 (patched)
> > 
> >
> > Sorry, I was wrong. Would you mind removing `createHostPathIfNotExists` 
> > again?
> > 
> > We probably want to distinguish the case of `pathValidator` exist or 
> > not explicitly:
> > 
> > Could you do your original logic in 
> > https://reviews.apache.org/r/69286/diff/2/ ?
> > 
> > Just change the return type of validate().
> > 
> > Sorry for the overhead

Sure thing. No worries.


- Jason


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


On Feb. 21, 2019, 11:34 p.m., Jason Lai wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69286/
> ---
> 
> (Updated Feb. 21, 2019, 11:34 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Eric Chung, Gilbert Song, Jie Yu, 
> and James Peach.
> 
> 
> Bugs: MESOS-9009
> https://issues.apache.org/jira/browse/MESOS-9009
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a new agent flag `--host_path_volume_force_creation` for
> the `volume/host_path` isolator. The flag takes a colon-separated
> whitelist of paths, under which non-existing host paths are allowed to
> be created.
> 
> If the flag is not specified, the isolator behaves in the original way
> of prohibiting all non-existing host paths from being created.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 732368293049b7d9d6f62057344d433637ad44e8 
>   src/Makefile.am c17eae4ff1d019d515f67d81821e933ecb5dc190 
>   src/slave/containerizer/mesos/isolators/volume/host_path.hpp 
> 4b509e91a056381ca90293d16a400ea4368234a3 
>   src/slave/containerizer/mesos/isolators/volume/host_path.cpp 
> 88ecf91d91e2bebd484a4ac94510a14b3500dbfb 
>   src/slave/containerizer/mesos/isolators/volume/utils.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/volume/utils.cpp PRE-CREATION 
>   src/slave/flags.hpp 29d8b7985ffde57da02b5fe0d3a524e98acc27c8 
>   src/slave/flags.cpp ccaf65029ec2d0e78041fc3992a0bf5ca0798686 
> 
> 
> Diff: https://reviews.apache.org/r/69286/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jason Lai
> 
>

Re: Review Request 69286: Allowed creating non-existing host paths for Mesos Containerizer.

2019-02-21 Thread Gilbert Song 

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


Fix it, then Ship it!





src/slave/containerizer/mesos/isolators/volume/host_path.cpp
Lines 165-167 (original), 181-183 (patched)


Sorry, I was wrong. Would you mind removing `createHostPathIfNotExists` 
again?

We probably want to distinguish the case of `pathValidator` exist or not 
explicitly:

Could you do your original logic in 
https://reviews.apache.org/r/69286/diff/2/ ?

Just change the return type of validate().

Sorry for the overhead


- Gilbert Song


On Feb. 21, 2019, 3:34 p.m., Jason Lai wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69286/
> ---
> 
> (Updated Feb. 21, 2019, 3:34 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Eric Chung, Gilbert Song, Jie Yu, 
> and James Peach.
> 
> 
> Bugs: MESOS-9009
> https://issues.apache.org/jira/browse/MESOS-9009
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a new agent flag `--host_path_volume_force_creation` for
> the `volume/host_path` isolator. The flag takes a colon-separated
> whitelist of paths, under which non-existing host paths are allowed to
> be created.
> 
> If the flag is not specified, the isolator behaves in the original way
> of prohibiting all non-existing host paths from being created.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 732368293049b7d9d6f62057344d433637ad44e8 
>   src/Makefile.am c17eae4ff1d019d515f67d81821e933ecb5dc190 
>   src/slave/containerizer/mesos/isolators/volume/host_path.hpp 
> 4b509e91a056381ca90293d16a400ea4368234a3 
>   src/slave/containerizer/mesos/isolators/volume/host_path.cpp 
> 88ecf91d91e2bebd484a4ac94510a14b3500dbfb 
>   src/slave/containerizer/mesos/isolators/volume/utils.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/volume/utils.cpp PRE-CREATION 
>   src/slave/flags.hpp 29d8b7985ffde57da02b5fe0d3a524e98acc27c8 
>   src/slave/flags.cpp ccaf65029ec2d0e78041fc3992a0bf5ca0798686 
> 
> 
> Diff: https://reviews.apache.org/r/69286/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jason Lai
> 
>

Re: Review Request 69286: Allowed creating non-existing host paths for Mesos Containerizer.

2019-02-21 Thread Jason Lai 


> On Feb. 21, 2019, 8:06 p.m., Gilbert Song wrote:
> > src/slave/containerizer/mesos/isolators/volume/host_path.hpp
> > Lines 23 (patched)
> > 
> >
> > nits:
> > 
> > newline above (usually we do that if the hierarchy of the file is 
> > different)
> > 
> > thanks!

Makes sense. Done.


> On Feb. 21, 2019, 8:06 p.m., Gilbert Song wrote:
> > src/slave/containerizer/mesos/isolators/volume/host_path.hpp
> > Lines 47 (patched)
> > 
> >
> > Nits:
> > 
> > could you do?
> > ```
> > const volume::PathValidator& pathValidator
> > ```

Done.


> On Feb. 21, 2019, 8:06 p.m., Gilbert Song wrote:
> > src/slave/containerizer/mesos/isolators/volume/host_path.hpp
> > Lines 50 (patched)
> > 
> >
> > ditto

Done.


> On Feb. 21, 2019, 8:06 p.m., Gilbert Song wrote:
> > src/slave/containerizer/mesos/isolators/volume/host_path.cpp
> > Lines 70-71 (original), 73-78 (patched)
> > 
> >
> > nits:
> > 
> > probably I would prefer more explicit:
> > ```
> >   Owned process;
> >   if (flags.host_path_volume_force_creation.isSome()) {
> > process = new VolumeHostPathIsolatorProcess(
> > flags,
> > 
> > PathValidator::parse(flags.host_path_volume_force_creation.get()));
> >   } else {
> > process = new VolumeHostPathIsolatorProcess(flags);
> >   }
> > ```

`Owned` doesn't support assignment after initialization, so the previous 
ternary-operator way would make more sense.

However, I adjusted my code to the styling you suggested in our private 
discussion.


> On Feb. 21, 2019, 8:06 p.m., Gilbert Song wrote:
> > src/slave/containerizer/mesos/isolators/volume/host_path.cpp
> > Lines 189 (patched)
> > 
> >
> > nits:
> > 
> > I would suggest to be more explicit on error msg.
> > 
> > ```
> >   return Failure("Failed to normalize the host path '" + hostPath.get() 
> > + "': " + normalizedPath.error());
> > ```

Sure thing.


> On Feb. 21, 2019, 8:06 p.m., Gilbert Song wrote:
> > src/slave/containerizer/mesos/isolators/volume/utils.cpp
> > Lines 41 (patched)
> > 
> >
> > instead of returning a boolean, do you think it is better to return a 
> > `Try` (if we always regard `false` as a error case)?

Makes sense. Done.


- Jason


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


On Feb. 11, 2019, 11:12 p.m., Jason Lai wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69286/
> ---
> 
> (Updated Feb. 11, 2019, 11:12 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Eric Chung, Gilbert Song, Jie Yu, 
> and James Peach.
> 
> 
> Bugs: MESOS-9009
> https://issues.apache.org/jira/browse/MESOS-9009
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a new agent flag `--host_path_volume_force_creation` for
> the `volume/host_path` isolator. The flag takes a colon-separated
> whitelist of paths, under which non-existing host paths are allowed to
> be created.
> 
> If the flag is not specified, the isolator behaves in the original way
> of prohibiting all non-existing host paths from being created.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 732368293049b7d9d6f62057344d433637ad44e8 
>   src/Makefile.am c17eae4ff1d019d515f67d81821e933ecb5dc190 
>   src/slave/containerizer/mesos/isolators/volume/host_path.hpp 
> 4b509e91a056381ca90293d16a400ea4368234a3 
>   src/slave/containerizer/mesos/isolators/volume/host_path.cpp 
> 88ecf91d91e2bebd484a4ac94510a14b3500dbfb 
>   src/slave/containerizer/mesos/isolators/volume/utils.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/volume/utils.cpp PRE-CREATION 
>   src/slave/flags.hpp 29d8b7985ffde57da02b5fe0d3a524e98acc27c8 
>   src/slave/flags.cpp ccaf65029ec2d0e78041fc3992a0bf5ca0798686 
> 
> 
> Diff: https://reviews.apache.org/r/69286/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jason Lai
> 
>

Re: Review Request 69286: Allowed creating non-existing host paths for Mesos Containerizer.

2019-02-21 Thread Jason Lai 

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

(Updated Feb. 21, 2019, 11:34 p.m.)


Review request for mesos, Chun-Hung Hsiao, Eric Chung, Gilbert Song, Jie Yu, 
and James Peach.


Changes
---

Address review comments from @gilbert.


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


Repository: mesos


Description
---

Added a new agent flag `--host_path_volume_force_creation` for
the `volume/host_path` isolator. The flag takes a colon-separated
whitelist of paths, under which non-existing host paths are allowed to
be created.

If the flag is not specified, the isolator behaves in the original way
of prohibiting all non-existing host paths from being created.


Diffs (updated)
-

  src/CMakeLists.txt 732368293049b7d9d6f62057344d433637ad44e8 
  src/Makefile.am c17eae4ff1d019d515f67d81821e933ecb5dc190 
  src/slave/containerizer/mesos/isolators/volume/host_path.hpp 
4b509e91a056381ca90293d16a400ea4368234a3 
  src/slave/containerizer/mesos/isolators/volume/host_path.cpp 
88ecf91d91e2bebd484a4ac94510a14b3500dbfb 
  src/slave/containerizer/mesos/isolators/volume/utils.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/volume/utils.cpp PRE-CREATION 
  src/slave/flags.hpp 29d8b7985ffde57da02b5fe0d3a524e98acc27c8 
  src/slave/flags.cpp ccaf65029ec2d0e78041fc3992a0bf5ca0798686 


Diff: https://reviews.apache.org/r/69286/diff/3/

Changes: https://reviews.apache.org/r/69286/diff/2-3/


Testing
---


Thanks,

Jason Lai

Re: Review Request 69286: Allowed creating non-existing host paths for Mesos Containerizer.

2019-02-21 Thread Gilbert Song 

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


Fix it, then Ship it!




The patch LGTM. Thanks for working on it!


src/slave/containerizer/mesos/isolators/volume/host_path.hpp
Lines 23 (patched)


nits:

newline above (usually we do that if the hierarchy of the file is different)

thanks!



src/slave/containerizer/mesos/isolators/volume/host_path.hpp
Lines 47 (patched)


Nits:

could you do?
```
const volume::PathValidator& pathValidator
```



src/slave/containerizer/mesos/isolators/volume/host_path.hpp
Lines 50 (patched)


ditto



src/slave/containerizer/mesos/isolators/volume/host_path.cpp
Lines 70-71 (original), 73-78 (patched)


nits:

probably I would prefer more explicit:
```
  Owned process;
  if (flags.host_path_volume_force_creation.isSome()) {
process = new VolumeHostPathIsolatorProcess(
flags,
PathValidator::parse(flags.host_path_volume_force_creation.get()));
  } else {
process = new VolumeHostPathIsolatorProcess(flags);
  }
```



src/slave/containerizer/mesos/isolators/volume/host_path.cpp
Lines 189 (patched)


nits:

I would suggest to be more explicit on error msg.

```
  return Failure("Failed to normalize the host path '" + hostPath.get() + 
"': " + normalizedPath.error());
```



src/slave/containerizer/mesos/isolators/volume/utils.cpp
Lines 41 (patched)


instead of returning a boolean, do you think it is better to return a 
`Try` (if we always regard `false` as a error case)?


- Gilbert Song


On Feb. 11, 2019, 3:12 p.m., Jason Lai wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69286/
> ---
> 
> (Updated Feb. 11, 2019, 3:12 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Eric Chung, Gilbert Song, Jie Yu, 
> and James Peach.
> 
> 
> Bugs: MESOS-9009
> https://issues.apache.org/jira/browse/MESOS-9009
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a new agent flag `--host_path_volume_force_creation` for
> the `volume/host_path` isolator. The flag takes a colon-separated
> whitelist of paths, under which non-existing host paths are allowed to
> be created.
> 
> If the flag is not specified, the isolator behaves in the original way
> of prohibiting all non-existing host paths from being created.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 732368293049b7d9d6f62057344d433637ad44e8 
>   src/Makefile.am c17eae4ff1d019d515f67d81821e933ecb5dc190 
>   src/slave/containerizer/mesos/isolators/volume/host_path.hpp 
> 4b509e91a056381ca90293d16a400ea4368234a3 
>   src/slave/containerizer/mesos/isolators/volume/host_path.cpp 
> 88ecf91d91e2bebd484a4ac94510a14b3500dbfb 
>   src/slave/containerizer/mesos/isolators/volume/utils.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/volume/utils.cpp PRE-CREATION 
>   src/slave/flags.hpp 29d8b7985ffde57da02b5fe0d3a524e98acc27c8 
>   src/slave/flags.cpp ccaf65029ec2d0e78041fc3992a0bf5ca0798686 
> 
> 
> Diff: https://reviews.apache.org/r/69286/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jason Lai
> 
>

Re: Review Request 69286: Allowed creating non-existing host paths for Mesos Containerizer.

2019-02-11 Thread Jason Lai 

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

(Updated Feb. 11, 2019, 11:12 p.m.)


Review request for mesos, Chun-Hung Hsiao, Eric Chung, Gilbert Song, Jie Yu, 
and James Peach.


Changes
---

Created `mesos::internal::slave::volume::PathValidator` that encapsulates the 
validation logic for whitelisted paths.


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


Repository: mesos


Description
---

Added a new agent flag `--host_path_volume_force_creation` for
the `volume/host_path` isolator. The flag takes a colon-separated
whitelist of paths, under which non-existing host paths are allowed to
be created.

If the flag is not specified, the isolator behaves in the original way
of prohibiting all non-existing host paths from being created.


Diffs (updated)
-

  src/CMakeLists.txt 732368293049b7d9d6f62057344d433637ad44e8 
  src/Makefile.am c17eae4ff1d019d515f67d81821e933ecb5dc190 
  src/slave/containerizer/mesos/isolators/volume/host_path.hpp 
4b509e91a056381ca90293d16a400ea4368234a3 
  src/slave/containerizer/mesos/isolators/volume/host_path.cpp 
88ecf91d91e2bebd484a4ac94510a14b3500dbfb 
  src/slave/containerizer/mesos/isolators/volume/utils.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/volume/utils.cpp PRE-CREATION 
  src/slave/flags.hpp 29d8b7985ffde57da02b5fe0d3a524e98acc27c8 
  src/slave/flags.cpp ccaf65029ec2d0e78041fc3992a0bf5ca0798686 


Diff: https://reviews.apache.org/r/69286/diff/2/

Changes: https://reviews.apache.org/r/69286/diff/1-2/


Testing
---


Thanks,

Jason Lai

Re: Review Request 69286: Allowed creating non-existing host paths for Mesos Containerizer.

2019-02-11 Thread Jason Lai 


> On Nov. 8, 2018, 6:33 a.m., Gilbert Song wrote:
> >

Hi, @gilbert. I updated the patch a bit differently than you originally 
requested by creating a `PathValidator` that encapsulate the validation logic 
instead. Please check if this revision rings a bell to you.


> On Nov. 8, 2018, 6:33 a.m., Gilbert Song wrote:
> > src/slave/containerizer/mesos/isolators/volume/host_path.hpp
> > Lines 46 (patched)
> > 
> >
> > s/hostPathWhitelist/forceCreatedHostPaths/g?

Created a validator class for this purpose instead.


> On Nov. 8, 2018, 6:33 a.m., Gilbert Song wrote:
> > src/slave/containerizer/mesos/isolators/volume/host_path.cpp
> > Lines 56-57 (patched)
> > 
> >
> > do we still need them if we remove the volume namespace?

I created a `mesos::internal::slave::volume::PathValidator` helper for this 
purpose instead.


> On Nov. 8, 2018, 6:33 a.m., Gilbert Song wrote:
> > src/slave/containerizer/mesos/isolators/volume/host_path.cpp
> > Lines 86 (patched)
> > 
> >
> > I would move the parse() to create()

Done. I created an overloaded constructor that takes a parsed `PathValidator` 
instance in the case where `_flags.host_path_volume_force_creation` is not 
`None`.


> On Nov. 8, 2018, 6:33 a.m., Gilbert Song wrote:
> > src/slave/containerizer/mesos/isolators/volume/utils.cpp
> > Lines 31 (patched)
> > 
> >
> > probably we do not need this method?

True. I dropped it in favor of a factory method as part of the `PathValidator` 
class.


> On Nov. 8, 2018, 6:33 a.m., Gilbert Song wrote:
> > src/slave/flags.cpp
> > Lines 657 (patched)
> > 
> >
> > if directories do not exist

Done.


> On Nov. 8, 2018, 6:33 a.m., Gilbert Song wrote:
> > src/slave/flags.cpp
> > Lines 658-660 (patched)
> > 
> >
> > Remove these lines?

Done.


- Jason


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


On Nov. 7, 2018, 10:03 p.m., Jason Lai wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69286/
> ---
> 
> (Updated Nov. 7, 2018, 10:03 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Eric Chung, Gilbert Song, Jie Yu, 
> and James Peach.
> 
> 
> Bugs: MESOS-9009
> https://issues.apache.org/jira/browse/MESOS-9009
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a new agent flag `--host_path_volume_force_creation` for
> the `volume/host_path` isolator. The flag takes a colon-separated
> whitelist of paths, under which non-existing host paths are allowed to
> be created.
> 
> If the flag is not specified, the isolator behaves in the original way
> of prohibiting all non-existing host paths from being created.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 732368293049b7d9d6f62057344d433637ad44e8 
>   src/Makefile.am c17eae4ff1d019d515f67d81821e933ecb5dc190 
>   src/slave/containerizer/mesos/isolators/volume/host_path.hpp 
> 4b509e91a056381ca90293d16a400ea4368234a3 
>   src/slave/containerizer/mesos/isolators/volume/host_path.cpp 
> 88ecf91d91e2bebd484a4ac94510a14b3500dbfb 
>   src/slave/containerizer/mesos/isolators/volume/utils.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/volume/utils.cpp PRE-CREATION 
>   src/slave/flags.hpp 29d8b7985ffde57da02b5fe0d3a524e98acc27c8 
>   src/slave/flags.cpp ccaf65029ec2d0e78041fc3992a0bf5ca0798686 
> 
> 
> Diff: https://reviews.apache.org/r/69286/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jason Lai
> 
>

Re: Review Request 69286: Allowed creating non-existing host paths for Mesos Containerizer.

2018-11-08 Thread Gilbert Song 

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




src/slave/containerizer/mesos/isolators/volume/host_path.hpp
Lines 46 (patched)


s/hostPathWhitelist/forceCreatedHostPaths/g?



src/slave/containerizer/mesos/isolators/volume/host_path.cpp
Lines 56-57 (patched)


do we still need them if we remove the volume namespace?



src/slave/containerizer/mesos/isolators/volume/host_path.cpp
Lines 86 (patched)


I would move the parse() to create()



src/slave/containerizer/mesos/isolators/volume/utils.cpp
Lines 31 (patched)


probably we do not need this method?



src/slave/flags.cpp
Lines 657 (patched)


if directories do not exist



src/slave/flags.cpp
Lines 658-660 (patched)


Remove these lines?


- Gilbert Song


On Nov. 7, 2018, 2:03 p.m., Jason Lai wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69286/
> ---
> 
> (Updated Nov. 7, 2018, 2:03 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Eric Chung, Gilbert Song, Jie Yu, 
> and James Peach.
> 
> 
> Bugs: MESOS-9009
> https://issues.apache.org/jira/browse/MESOS-9009
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a new agent flag `--host_path_volume_force_creation` for
> the `volume/host_path` isolator. The flag takes a colon-separated
> whitelist of paths, under which non-existing host paths are allowed to
> be created.
> 
> If the flag is not specified, the isolator behaves in the original way
> of prohibiting all non-existing host paths from being created.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 732368293049b7d9d6f62057344d433637ad44e8 
>   src/Makefile.am c17eae4ff1d019d515f67d81821e933ecb5dc190 
>   src/slave/containerizer/mesos/isolators/volume/host_path.hpp 
> 4b509e91a056381ca90293d16a400ea4368234a3 
>   src/slave/containerizer/mesos/isolators/volume/host_path.cpp 
> 88ecf91d91e2bebd484a4ac94510a14b3500dbfb 
>   src/slave/containerizer/mesos/isolators/volume/utils.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/volume/utils.cpp PRE-CREATION 
>   src/slave/flags.hpp 29d8b7985ffde57da02b5fe0d3a524e98acc27c8 
>   src/slave/flags.cpp ccaf65029ec2d0e78041fc3992a0bf5ca0798686 
> 
> 
> Diff: https://reviews.apache.org/r/69286/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jason Lai
> 
>