Re: Review Request 44029: Fetcher::basename should ignore query strings and fragments.

[Jiang Yan Xu]

> On Feb. 26, 2016, 5:40 a.m., Bernd Mathiske wrote:
> > There is a more elaborate solution to this problem 
> > (https://reviews.apache.org/r/40054), but it requires a lot of code to 
> > implement URL parsing. Until we finalize that, I think the patch at hand 
> > gets the most urgent job done.

The concern here is that it changes the current behavior of the fetcher output. 
Although we agree that probably most people would find the query string in the 
filename annoying, there could be people who rely on this (and with a 
workaround in place) and this will suddenly break things.

The explicit output filename proposed in 
https://issues.apache.org/jira/browse/MESOS-3367 sounds like the right approach.


- Jiang Yan


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


On Feb. 25, 2016, 10:32 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44029/
> ---
> 
> (Updated Feb. 25, 2016, 10:32 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Jie Yu, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4779
> https://issues.apache.org/jira/browse/MESOS-4779
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Ignore URL query parameters and fragments when determining this
> base name. This enables the fetcher to subsequently examine the
> file extension and extract archives correctly.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/fetcher.cpp 
> 33dfcade6beb53a5a6dbc41a8f3380f5cb30a161 
>   src/tests/fetcher_tests.cpp fb47706eb90ae5808bafe13c681d609a808b0c8e 
> 
> Diff: https://reviews.apache.org/r/44029/diff/
> 
> 
> Testing
> ---
> 
> ``make check`` on OS X.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44029: Fetcher::basename should ignore query strings and fragments.

[Jiang Yan Xu]

> On Feb. 26, 2016, 5:40 a.m., Bernd Mathiske wrote:
> > There is a more elaborate solution to this problem 
> > (https://reviews.apache.org/r/40054), but it requires a lot of code to 
> > implement URL parsing. Until we finalize that, I think the patch at hand 
> > gets the most urgent job done.
> 
> Jiang Yan Xu wrote:
> The concern here is that it changes the current behavior of the fetcher 
> output. Although we agree that probably most people would find the query 
> string in the filename annoying, there could be people who rely on this (and 
> with a workaround in place) and this will suddenly break things.
> 
> The explicit output filename proposed in 
> https://issues.apache.org/jira/browse/MESOS-3367 sounds like the right 
> approach.

When we have an alternative approach available to users, it's easier to then 
come back and impose a deprecate cycle for the default behavior. What do you 
think?


- Jiang Yan


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


On Feb. 25, 2016, 10:32 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44029/
> ---
> 
> (Updated Feb. 25, 2016, 10:32 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Jie Yu, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4779
> https://issues.apache.org/jira/browse/MESOS-4779
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Ignore URL query parameters and fragments when determining this
> base name. This enables the fetcher to subsequently examine the
> file extension and extract archives correctly.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/fetcher.cpp 
> 33dfcade6beb53a5a6dbc41a8f3380f5cb30a161 
>   src/tests/fetcher_tests.cpp fb47706eb90ae5808bafe13c681d609a808b0c8e 
> 
> Diff: https://reviews.apache.org/r/44029/diff/
> 
> 
> Testing
> ---
> 
> ``make check`` on OS X.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44029: Fetcher::basename should ignore query strings and fragments.

[Jiang Yan Xu]

> On Feb. 26, 2016, 5:40 a.m., Bernd Mathiske wrote:
> > There is a more elaborate solution to this problem 
> > (https://reviews.apache.org/r/40054), but it requires a lot of code to 
> > implement URL parsing. Until we finalize that, I think the patch at hand 
> > gets the most urgent job done.
> 
> Jiang Yan Xu wrote:
> The concern here is that it changes the current behavior of the fetcher 
> output. Although we agree that probably most people would find the query 
> string in the filename annoying, there could be people who rely on this (and 
> with a workaround in place) and this will suddenly break things.
> 
> The explicit output filename proposed in 
> https://issues.apache.org/jira/browse/MESOS-3367 sounds like the right 
> approach.
> 
> Jiang Yan Xu wrote:
> When we have an alternative approach available to users, it's easier to 
> then come back and impose a deprecate cycle for the default behavior. What do 
> you think?
> 
> James Peach wrote:
> Yes, there's definitely compatibility concern (for example, 
> ``http://foo/bar/baz?filename.zip`` would no longer work. After chatting with 
> Yan, maybe it is sufficient to ignore the extension and just try all the 
> extractors. If it is extractable one of them will succeed. I don't think 
> there's any compatibility concerns here and we wouldn't need to do a 
> deprecation cycle.

So it occurred to me that here's an edge case:

Some executable archieves are in fact zip files, e.g., jar and pex files. In 
the past they wouldn't get automatically decompressed due to their suffixes and 
not that unzip couldn't decomopress them.

The original file is still going to be there but this implicates disk quota, 
etc. Seemingly not an extremely dangerous case but probably still warrants a 
deprecation cycle.


- Jiang Yan


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


On Feb. 25, 2016, 10:32 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44029/
> -----------
> 
> (Updated Feb. 25, 2016, 10:32 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Jie Yu, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4779
> https://issues.apache.org/jira/browse/MESOS-4779
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Ignore URL query parameters and fragments when determining this
> base name. This enables the fetcher to subsequently examine the
> file extension and extract archives correctly.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/fetcher.cpp 
> 33dfcade6beb53a5a6dbc41a8f3380f5cb30a161 
>   src/tests/fetcher_tests.cpp fb47706eb90ae5808bafe13c681d609a808b0c8e 
> 
> Diff: https://reviews.apache.org/r/44029/diff/
> 
> 
> Testing
> ---
> 
> ``make check`` on OS X.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44342: XFS disk resource isolator.

[Jiang Yan Xu]

/44342/#comment184357>

This looks like a generic utility which we can leverage & add to ls.hpp.



src/slave/containerizer/mesos/isolators/disk/xfs.cpp (line 396)
<https://reviews.apache.org/r/44342/#comment184360>

It's unclear why this additional check is needed here so can you comment on 
it?



src/slave/containerizer/mesos/isolators/disk/xfs.cpp (line 456)
<https://reviews.apache.org/r/44342/#comment184364>

The naming is inconsistent with the reset of xfs* methods but moreover, can 
we pull out things that aren't XFS specific to stout headers and group XFS 
methods under namespace `mesos::internal::slave::xfs` instead? In fact perhaps 
put them under `linux/xfs/`?



src/slave/containerizer/mesos/isolators/disk/xfs.cpp (line 470)
<https://reviews.apache.org/r/44342/#comment184802>

Use `lambda::_1` instead.



src/slave/containerizer/mesos/isolators/disk/xfs.cpp (lines 489 - 493)
<https://reviews.apache.org/r/44342/#comment184385>

This feels like a TODO.

FWIW currently persistent volumes are not removed at all. We can try to 
make the behavior consistent with posix/disk and also enforce quota on them and 
update its logic later when we work on the ticket that handles persitent volume 
deletion.

Otherwise such limitation needs to be prominently documentated. (e.g. in 
the class-level comments and user docs) with a TODO or JIRA.

Let's evaluate what to do for now.



src/slave/containerizer/mesos/isolators/disk/xfs.cpp (line 513)
<https://reviews.apache.org/r/44342/#comment184492>

What privledges does XFS isolator require?

Looks to me some operations require CAP_SYS_ADMIN. We often just check if 
the user is root.



src/slave/containerizer/mesos/isolators/disk/xfs.cpp (line 531)
<https://reviews.apache.org/r/44342/#comment184798>

Are we losing the last element of each range if the interval set is open on 
the right side?



src/slave/containerizer/mesos/isolators/disk/xfs.cpp (line 553)
<https://reviews.apache.org/r/44342/#comment184649>

We'll likely see this a lot when we first turn on this feature. 
LOG(WARNING) is better IMO.



src/slave/containerizer/mesos/isolators/disk/xfs.cpp (line 573)
<https://reviews.apache.org/r/44342/#comment184653>

No biggie but it's more customary to add a `:` after TODO(jpeach).



src/slave/containerizer/mesos/isolators/disk/xfs.cpp (lines 573 - 576)
<https://reviews.apache.org/r/44342/#comment184680>

We do need to handle orphans, as you've commented in `cleanup()`.

`orphans` here are known to the containerizer so `cleanup()` will be called 
by it to remove the quota. However if there's not an entry for it in `infos`, 
`cleanup()` ignores it.

The concern is that if the project ID of the orphan is not unassigned from 
the sandbox and the ID is not tracked by the isolator, next time it's used by a 
new task, it'll share the same quota with the orphan.


- Jiang Yan Xu


On March 7, 2016, 10:38 a.m., James Peach wrote:
> 
> -----------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44342/
> ---
> 
> (Updated March 7, 2016, 10:38 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4828
> https://issues.apache.org/jira/browse/MESOS-4828
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Track sandbox directory usage by dynamically assigning XFS project
> quotas. We track a range of XFS project IDs, assigning a project ID
> and a project quota to each sandbox as it is created. When the task
> reaches the quota, writes will fail with EDQUOT, and the task will have
> an opportunity to handle that.
> 
> Quotas are not applied to volume resources since the isolator interface
> has no insight into the volume lifecycle. Thus it is not currently
> possible to accurately assign and reclaim project IDs.
> 
> If LOW is the lower bound of the project ID range and HIGH is the upper
> bound, you can show the currently allocated project quotas using the
> xfs_quota command:
> 
>   $ xfs_quota -x -c "report -a -n -L LOW -U HIGH"
> 
> To show the project ID assigned to the file PATH, use the xfs_io command:
> 
>   $ xfs_io -r -c stat PATH
> 
> 
> Diffs
> -
> 
>   configure.ac a20382e8d425eb297492a6e6c2c75ea59be097c2 
>   docs/configuration.md 305ba2c801c2060db6dcb4ef83c1043aaa7d520c 
>   docs/mesos-containerizer.md 15fb5bdbe74e059614b8948108f32cd04b623305 
>   src/Makefile.am a41e95ddeb838fdebf4ced953c4a29181916e261 
>   src/slave/containerizer/mesos/containerizer.cpp 
> af3ff5750649497d8852b4761c78d4cae5455a02 
>   src

Re: Review Request 44342: XFS disk resource isolator.

[Jiang Yan Xu]

> On March 9, 2016, 10:03 a.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/mesos/isolators/disk/xfs.cpp, lines 573-576
> > <https://reviews.apache.org/r/44342/diff/3/?file=1279808#file1279808line573>
> >
> > We do need to handle orphans, as you've commented in `cleanup()`.
> > 
> > `orphans` here are known to the containerizer so `cleanup()` will be 
> > called by it to remove the quota. However if there's not an entry for it in 
> > `infos`, `cleanup()` ignores it.
> > 
> > The concern is that if the project ID of the orphan is not unassigned 
> > from the sandbox and the ID is not tracked by the isolator, next time it's 
> > used by a new task, it'll share the same quota with the orphan.

OK it looks to me that we do need to recursively scan all the sandboxes (we can 
probably `glob` them) and determine whether they belong to an recovered 
container, known orphan and unknown orphan and then clean up the extraneous 
ones.


- Jiang Yan


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


On March 7, 2016, 10:38 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44342/
> -----------
> 
> (Updated March 7, 2016, 10:38 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4828
> https://issues.apache.org/jira/browse/MESOS-4828
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Track sandbox directory usage by dynamically assigning XFS project
> quotas. We track a range of XFS project IDs, assigning a project ID
> and a project quota to each sandbox as it is created. When the task
> reaches the quota, writes will fail with EDQUOT, and the task will have
> an opportunity to handle that.
> 
> Quotas are not applied to volume resources since the isolator interface
> has no insight into the volume lifecycle. Thus it is not currently
> possible to accurately assign and reclaim project IDs.
> 
> If LOW is the lower bound of the project ID range and HIGH is the upper
> bound, you can show the currently allocated project quotas using the
> xfs_quota command:
> 
>   $ xfs_quota -x -c "report -a -n -L LOW -U HIGH"
> 
> To show the project ID assigned to the file PATH, use the xfs_io command:
> 
>   $ xfs_io -r -c stat PATH
> 
> 
> Diffs
> -
> 
>   configure.ac a20382e8d425eb297492a6e6c2c75ea59be097c2 
>   docs/configuration.md 305ba2c801c2060db6dcb4ef83c1043aaa7d520c 
>   docs/mesos-containerizer.md 15fb5bdbe74e059614b8948108f32cd04b623305 
>   src/Makefile.am a41e95ddeb838fdebf4ced953c4a29181916e261 
>   src/slave/containerizer/mesos/containerizer.cpp 
> af3ff5750649497d8852b4761c78d4cae5455a02 
>   src/slave/containerizer/mesos/isolators/disk/xfs.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/disk/xfs.cpp PRE-CREATION 
>   src/slave/flags.hpp feb095da4521f678c96f4cc53bdfda262d350388 
>   src/slave/flags.cpp 6e3fd69c06eefd40bc0e5c222ea72f34144c5534 
> 
> Diff: https://reviews.apache.org/r/44342/diff/
> 
> 
> Testing
> ---
> 
> Manual testing on Fedora 23 w/ XFS. Make check on Fedora and OS X.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44945: Add autoconf tests for XFS project quotas.

[Jiang Yan Xu]

> On March 18, 2016, 11:36 a.m., Jiang Yan Xu wrote:
> > configure.ac, line 923
> > <https://reviews.apache.org/r/44945/diff/3/?file=1304828#file1304828line923>
> >
> > Sorry this should have been a continued discussion on 
> > https://reviews.apache.org/r/44342/#comment184051 but anyways:
> > 
> > Your response to my initial comments:
> > > There's no AC_ARG_WITH. If the dependencies area available we will 
> > build the isolator and the operator can configure it at runtime. I don't 
> > think there's any need to disable this at build time since it is not 
> > enabled by default anyway.
> > 
> > I am still favoring building XFS isolator based on 
> > `--with-xfs-isolator` (AC_ARG_ENABLE is probably better than AC_ARG_WITH) 
> > for the following reasons:
> > 
> > 
> > 1. Explicitness: In general, if we build things solely based on the 
> > availability of headers, users can inadvertently build more things into the 
> > deployed binary which could result in larger binaries and perhaps other 
> > side effects (through bugs, etc.).
> > 2. Preventing user mistake: I am thinking about how we would advise 
> > users on enabling this feature:
> >   1. "The XFS isolator feature is disabled by default, to enable it in 
> > build, install these packages whose name could be different for different 
> > distros and if you haven't made mistakes in finding them, the feature will 
> > be built." vs. 
> >   2. "The XFS isolator feature is disabled by default, to enable it in 
> > build, install these packages (which could be named differently depending 
> > on your distro), if the dependencies are not satisfied, the build aborts 
> > with an message stating such error." The latter feels better to me.
> > 3. Consistency: Other optional features of Mesos follow this pattern.

Sorry I meant `--enable-xfs-isolator`.


- Jiang Yan


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


On March 17, 2016, 8:45 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44945/
> ---
> 
> (Updated March 17, 2016, 8:45 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4828
> https://issues.apache.org/jira/browse/MESOS-4828
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add autoconf tests for XFS project quotas.
> 
> 
> Diffs
> -
> 
>   configure.ac 8e4f03593df4a8ba13f00292963e351acc3f71c1 
> 
> Diff: https://reviews.apache.org/r/44945/diff/
> 
> 
> Testing
> ---
> 
> Make check. Manual verification.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44945: Add autoconf tests for XFS project quotas.

[Jiang Yan Xu]

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




configure.ac (line 923)
<https://reviews.apache.org/r/44945/#comment186696>

Sorry this should have been a continued discussion on 
https://reviews.apache.org/r/44342/#comment184051 but anyways:

Your response to my initial comments:
> There's no AC_ARG_WITH. If the dependencies area available we will build 
the isolator and the operator can configure it at runtime. I don't think 
there's any need to disable this at build time since it is not enabled by 
default anyway.

I am still favoring building XFS isolator based on `--with-xfs-isolator` 
(AC_ARG_ENABLE is probably better than AC_ARG_WITH) for the following reasons:

1. Explicitness: In general, if we build things solely based on the 
availability of headers, users can inadvertently build more things into the 
deployed binary which could result in larger binaries and perhaps other side 
effects (through bugs, etc.).
2. Preventing user mistake: I am thinking about how we would advise users 
on enabling this feature:
  1. "The XFS isolator feature is disabled by default, to enable it in 
build, install these packages whose name could be different for different 
distros and if you haven't made mistakes in finding them, the feature will be 
built." vs. 
  2. "The XFS isolator feature is disabled by default, to enable it in 
build, install these packages (which could be named differently depending on 
your distro), if the dependencies are not satisfied, the build aborts with an 
message stating such error." The latter feels better to me.
3. Consistency: Other optional features of Mesos follow this pattern.


- Jiang Yan Xu


On March 17, 2016, 8:45 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44945/
> ---
> 
> (Updated March 17, 2016, 8:45 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4828
> https://issues.apache.org/jira/browse/MESOS-4828
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add autoconf tests for XFS project quotas.
> 
> 
> Diffs
> -
> 
>   configure.ac 8e4f03593df4a8ba13f00292963e351acc3f71c1 
> 
> Diff: https://reviews.apache.org/r/44945/diff/
> 
> 
> Testing
> ---
> 
> Make check. Manual verification.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44945: Add autoconf tests for XFS project quotas.

[Jiang Yan Xu]

> On March 18, 2016, 11:36 a.m., Jiang Yan Xu wrote:
> > configure.ac, line 923
> > <https://reviews.apache.org/r/44945/diff/3/?file=1304828#file1304828line923>
> >
> > Sorry this should have been a continued discussion on 
> > https://reviews.apache.org/r/44342/#comment184051 but anyways:
> > 
> > Your response to my initial comments:
> > > There's no AC_ARG_WITH. If the dependencies area available we will 
> > build the isolator and the operator can configure it at runtime. I don't 
> > think there's any need to disable this at build time since it is not 
> > enabled by default anyway.
> > 
> > I am still favoring building XFS isolator based on 
> > `--with-xfs-isolator` (AC_ARG_ENABLE is probably better than AC_ARG_WITH) 
> > for the following reasons:
> > 
> > 
> > 1. Explicitness: In general, if we build things solely based on the 
> > availability of headers, users can inadvertently build more things into the 
> > deployed binary which could result in larger binaries and perhaps other 
> > side effects (through bugs, etc.).
> > 2. Preventing user mistake: I am thinking about how we would advise 
> > users on enabling this feature:
> >   1. "The XFS isolator feature is disabled by default, to enable it in 
> > build, install these packages whose name could be different for different 
> > distros and if you haven't made mistakes in finding them, the feature will 
> > be built." vs. 
> >   2. "The XFS isolator feature is disabled by default, to enable it in 
> > build, install these packages (which could be named differently depending 
> > on your distro), if the dependencies are not satisfied, the build aborts 
> > with an message stating such error." The latter feels better to me.
> > 3. Consistency: Other optional features of Mesos follow this pattern.
> 
> Jiang Yan Xu wrote:
> Sorry I meant `--enable-xfs-isolator`.
> 
> Jie Yu wrote:
> For consistency: I think the network isolator is using 
> `--with-network-isolator` (`AC_ARG_WITH([network-isolator],`), so it'e better 
> to be consistent and use 'AC_ARG_WITH' here.
> 
> I agree with Yan that we should be more explicit, instead of relying on 
> headers.
> 
> James Peach wrote:
> My view is that unless there is a tradeof (performance, security, etc), 
> then every optional feature should be available at runtime. The alternative 
> imposes an undue burden on operators because they will have to repackage if 
> they want to enable a feature.
> 
> However, I'm not going to argue this any furthere here. I'll just guard 
> this with ``AC_ARG_WITH`` (@xujyan is right this should have been 
> ``AC_ARG_ENABLE``).

We are already inconsistent in the use of enable vs. with: `--enable-ssl`, 
`--enable-libevent`, `--enable-debug`, etc.

I think the convention (not in Mesos) is:

- "Build this problem with some **internal** feature xyz of this program 
enabled", i.e., `--enable-xyz`
- "Build this program with some (potetally also specify its location) 
**external** dependency abc". i.e, `--with-abc`.

I'd prefer `--enable` here. What do you guys think?


- Jiang Yan


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


On March 20, 2016, 6:59 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44945/
> ---
> 
> (Updated March 20, 2016, 6:59 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4828
> https://issues.apache.org/jira/browse/MESOS-4828
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add autoconf tests for XFS project quotas.
> 
> 
> Diffs
> -
> 
>   configure.ac 9ec4bc1cff3b0b46dd2e7ece2c1f2d19ffb8 
> 
> Diff: https://reviews.apache.org/r/44945/diff/
> 
> 
> Testing
> ---
> 
> Make check. Manual verification.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44946: Add utility functions to manipulate XFS project quotas.

[Jiang Yan Xu]

ember of the struct has special some meaning/usage.



src/slave/containerizer/mesos/isolators/disk/xfs/utils.cpp (line 127)
<https://reviews.apache.org/r/44946/#comment187682>

s/solf/soft/.



src/slave/containerizer/mesos/isolators/disk/xfs/utils.cpp (line 133)
<https://reviews.apache.org/r/44946/#comment187680>

The magic number 512 is used enough times to warrant a constexpr variable 
IMO. Plus it's not clear in the comments in this file that the **fixed** *basic 
block size* as opposed to the **configurable** *block size* is used in these 
measurements.

By definiting such a constant you can also clearly document this.



src/slave/containerizer/mesos/isolators/disk/xfs/utils.cpp (lines 295 - 297)
<https://reviews.apache.org/r/44946/#comment187706>

```
Try status = getXfsAttributes(fd.get(), attr);
    ```



src/slave/containerizer/mesos/isolators/disk/xfs/utils.cpp (line 317)
<https://reviews.apache.org/r/44946/#comment187683>

Kill the line.


- Jiang Yan Xu


On March 22, 2016, 4:24 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44946/
> ---
> 
> (Updated March 22, 2016, 4:24 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4828
> https://issues.apache.org/jira/browse/MESOS-4828
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add utility functions to manipulate XFS project quotas.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 6552e48eab2708a28dd69adba3ec759cb5aeca4c 
>   src/slave/containerizer/mesos/isolators/disk/xfs/utils.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/disk/xfs/utils.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44946/diff/
> 
> 
> Testing
> ---
> 
> Make check. Manual verification. Tests in subsequent patches.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44946: Add utility functions to manipulate XFS project quotas.

[Jiang Yan Xu]

> On March 23, 2016, 10:39 a.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/mesos/isolators/disk/xfs/utils.cpp, line 119
> > <https://reviews.apache.org/r/44946/diff/9/?file=1311203#file1311203line119>
> >
> > I know I asked this previoiusly but what's the difference between this 
> > and:
> > 
> > ```
> > fs_disk_quota_t quota;
> > ```
> > 
> > If it's the same, then the more common syntax is preferred because this 
> > syntax reads like the 1st member of the struct has special some 
> > meaning/usage.
> 
> James Peach wrote:
> This is a ``C`` zero-initialization. I can either do this or ``memset`` 
> the structure to zero, to get the same result.

Forgot this is a C struct. Alright.


- Jiang Yan


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


On March 22, 2016, 4:24 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44946/
> -----------
> 
> (Updated March 22, 2016, 4:24 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4828
> https://issues.apache.org/jira/browse/MESOS-4828
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add utility functions to manipulate XFS project quotas.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 6552e48eab2708a28dd69adba3ec759cb5aeca4c 
>   src/slave/containerizer/mesos/isolators/disk/xfs/utils.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/disk/xfs/utils.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44946/diff/
> 
> 
> Testing
> ---
> 
> Make check. Manual verification. Tests in subsequent patches.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44948: Add XFS disk resource isolator.

[Jiang Yan Xu]

s` cleanup for the containerizer once it successfully kills the cgroup.



src/slave/containerizer/mesos/isolators/disk/xfs.cpp (lines 381 - 382)
<https://reviews.apache.org/r/44948/#comment188006>

This is not a valid indentation style. Use the following instead.

```
return Failure("Failed to scan sandbox directories: " +
   sandboxes.error());
```



src/slave/containerizer/mesos/isolators/disk/xfs.cpp (lines 399 - 408)
<https://reviews.apache.org/r/44948/#comment188008>

We are doing this twice if the sandbox didn't have a project ID assigned 
before the agent restart.

Let's do the following:

```
foreach (const string& sandbox, sandboxes.get()) {
  // Put the info for sandbox in infos regardless whether it is in 
`states`, `orphans` or not.
  // If sandbox is an unknown orphan, call cleanup(containerId).
}
```

We may need to call cleanup asynchronously (i.e., `async(cleanup)`) because 
the cleanup could be expensive if there are a lot of files in the sandbox and 
we don't need to wait for it to complete (because it's an unknown orphan).



src/slave/containerizer/mesos/isolators/disk/xfs.cpp (line 448)
<https://reviews.apache.org/r/44948/#comment187733>

s/" : "/": "/



src/slave/containerizer/mesos/isolators/disk/xfs.cpp (lines 501 - 503)
<https://reviews.apache.org/r/44948/#comment188028>

If the new resources have no disk resources, shouldn't we cleanup the 
sandbox?



src/slave/containerizer/mesos/isolators/disk/xfs.cpp (line 512)
<https://reviews.apache.org/r/44948/#comment188029>

s/" : "/": "/



src/slave/containerizer/mesos/isolators/disk/xfs.cpp (lines 552 - 553)
<https://reviews.apache.org/r/44948/#comment188062>

Nit: this fits in one line.



src/slave/containerizer/mesos/isolators/disk/xfs.cpp (lines 565 - 566)
<https://reviews.apache.org/r/44948/#comment188020>

Recover at GC time: this is unlikely to be implemented given the way GC 
works right now and I don't see a major drawback in doing the cleanup there so 
the comment is a bit confusing. It's either a TODO or a description of the 
current behavior. If this is just to help readers understand the design 
decision, I would be explicit: "We could do ... but chose to do ... because ..."



src/slave/containerizer/mesos/isolators/disk/xfs.cpp (line 568)
<https://reviews.apache.org/r/44948/#comment188023>

It seems that we don't need to modify `totalProjectIds` (which can be a 
const) here, this change doesn't survive agent restart anyways and we use 
`freeProjectIds` to assign IDs.



src/slave/containerizer/mesos/isolators/disk/xfs.cpp (lines 588 - 589)
<https://reviews.apache.org/r/44948/#comment188021>

It seems that if we trust `freeProjectIds` to be initialized correctly, 
then `projectId` is never going to be zero here. Normally we do such checks 
before we are about to use a variable in a way that assumes the CHECK condition 
holds so it doesn't look like we need to CHECK here.

I am trying to see if we can eliminate the sentinel value from the isolator 
and confine it in the utils.



src/slave/containerizer/mesos/isolators/disk/xfs.cpp (line 602)
<https://reviews.apache.org/r/44948/#comment188017>

s/changes/changed/


- Jiang Yan Xu


On March 22, 2016, 4:24 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44948/
> ---
> 
> (Updated March 22, 2016, 4:24 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOs-4828
> https://issues.apache.org/jira/browse/MESOs-4828
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Track sandbox directory usage by dynamically assigning XFS project
> quotas. We track a range of XFS project IDs, assigning a project ID
> and a project quota to each sandbox as it is created. When the task
> reaches the quota, writes will fail with EDQUOT, and the task will have
> an opportunity to handle that.
> 
> Quotas are not applied to volume resources since the isolator interface
> has no insight into the volume lifecycle. Thus it is not currently
> possible to accurately assign and reclaim project IDs.
> 
> If LOW is the lower bound of the project ID range and HIGH is the upper
> bound, you can show the currently allocated project quotas using the
> xfs_quota command:
> 
>   $ xfs_quota -x -c "report -a -n -L LOW -U HIGH"
> 
> To show the project ID assigned to the file PATH, use the xfs_io command:
> 
>   $ xfs_io -r -c stat PATH
> 
&g

Re: Review Request 44946: Add utility functions to manipulate XFS project quotas.

[Jiang Yan Xu]

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




src/slave/containerizer/mesos/isolators/disk/xfs/utils.cpp (line 305)
<https://reviews.apache.org/r/44946/#comment187813>

More strictly: `0u`.

Plus, it would be clearer is it is defined as a constant.

// A sentinel project ID that indicates that the file/directory is not 
associated with a valid project ID.
constexpr prid_t NON_PROJECT_ID = 0u;

As mentioned in another review, I think we can limit the use of this 
sentinel in the util.


- Jiang Yan Xu


On March 22, 2016, 4:24 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44946/
> ---
> 
> (Updated March 22, 2016, 4:24 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4828
> https://issues.apache.org/jira/browse/MESOS-4828
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add utility functions to manipulate XFS project quotas.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 6552e48eab2708a28dd69adba3ec759cb5aeca4c 
>   src/slave/containerizer/mesos/isolators/disk/xfs/utils.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/disk/xfs/utils.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44946/diff/
> 
> 
> Testing
> ---
> 
> Make check. Manual verification. Tests in subsequent patches.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44945: Add autoconf tests for XFS project quotas.

[Jiang Yan Xu]

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




configure.ac (line 258)
<https://reviews.apache.org/r/44945/#comment188243>

Two spaces to the right.

Also, append "default: no"



configure.ac (line 259)
<https://reviews.apache.org/r/44945/#comment188248>

We probably shouldn't take option arugments, i.e., 
`--enable-xfs-disk-isolator` is sufficient. Otherwise there is confusion with 
`--enable-xfs-disk-isolator=yes|true|1|absolutely`. :)

s/[enable_xfs_disk_isolator=no]/[] for consistency.



configure.ac (line 930)
<https://reviews.apache.org/r/44945/#comment188258>

Kill this blank line.

Also, I know the headers below imply Linux but I guess it won't hurt to add 

```
  AS_IF([test "$OS_NAME" = "linux"],
[],
[AC_MSG_ERROR([cannot build xfs disk isolator
---
XFS disk isolator is only supported on Linux.
---
  ])])
```

so the error message is clearer.



configure.ac (line 934)
<https://reviews.apache.org/r/44945/#comment188262>

Move one space to the right.



configure.ac (line 954)
<https://reviews.apache.org/r/44945/#comment188263>

This could be the place we insert 

```
AC_DEFINE([ENABLE_XFS_DISK_ISOLATOR])
```



configure.ac (lines 957 - 960)
<https://reviews.apache.org/r/44945/#comment188321>

Can we put AC_MSG_CHECKING before `AC_CHECK_HEADERS` and inside `AS_IF` 
above?

i.e., AC_MSG_CHECKING/AC_MSG_CHECKING/AC_MSG_ERROR inform user what they 
don't already know (whether the system dependencies are met), not what they 
already know (the fact that they provided the `--enable_xfs_disk_isolator` 
flag).

And this check is conditional: we only print `AC_MSG_CHECKING` and do these 
checks when `test "x$enable_xfs_disk_isolator" = "xyes"`; we print 
`AC_MSG_ERROR([...])` when such checks fail and `AC_MSG_RESULT([yes])` when all 
checks succeed.

Would this work?

If we do this, then the message could be `AC_MSG_CHECKING([whether we can 
enable the XFS disk isolator])`



configure.ac (lines 962 - 963)
<https://reviews.apache.org/r/44945/#comment188322>

If we pull this up into the aforementioned place we don't need to `test 
"x$enable_xfs_disk_isolator" = "xyes"` repeatedly and therefore be more concise.


- Jiang Yan Xu


On March 22, 2016, 4:21 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44945/
> -------
> 
> (Updated March 22, 2016, 4:21 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4828
> https://issues.apache.org/jira/browse/MESOS-4828
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add autoconf tests for XFS project quotas.
> 
> 
> Diffs
> -
> 
>   configure.ac 9ec4bc1cff3b0b46dd2e7ece2c1f2d19ffb8 
> 
> Diff: https://reviews.apache.org/r/44945/diff/
> 
> 
> Testing
> ---
> 
> Make check. Manual verification.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44945: Add autoconf tests for XFS project quotas.

[Jiang Yan Xu]

> On March 25, 2016, 11:43 p.m., Jiang Yan Xu wrote:
> > configure.ac, lines 957-960
> > <https://reviews.apache.org/r/44945/diff/8/?file=1311200#file1311200line957>
> >
> > Can we put AC_MSG_CHECKING before `AC_CHECK_HEADERS` and inside `AS_IF` 
> > above?
> > 
> > i.e., AC_MSG_CHECKING/AC_MSG_CHECKING/AC_MSG_ERROR inform user what 
> > they don't already know (whether the system dependencies are met), not what 
> > they already know (the fact that they provided the 
> > `--enable_xfs_disk_isolator` flag).
> > 
> > And this check is conditional: we only print `AC_MSG_CHECKING` and do 
> > these checks when `test "x$enable_xfs_disk_isolator" = "xyes"`; we print 
> > `AC_MSG_ERROR([...])` when such checks fail and `AC_MSG_RESULT([yes])` when 
> > all checks succeed.
> > 
> > Would this work?
> > 
> > If we do this, then the message could be `AC_MSG_CHECKING([whether we 
> > can enable the XFS disk isolator])`
> 
> James Peach wrote:
> No, this message should be outside the check so that the builder always 
> knows what happened, independent of what configure flags they passed. 
> Typically, people start by not passing any flags, and it is helpful to 
> explicitly log that a feature was not enabled. Even if you pass the --enable 
> flags, it is comforting to get an explicit confirmation that the feature you 
> asked for is enabled.

I agree that explicitly confirming that the user has enabled something is 
better. 

I still don't think we should print it after we've done all the dependency 
checks for it and have potentially aborted configure with error messages but 
without telling the error is due to "whether to enable the XFS disk isolator..."

i.e., the following is better.

```
checking whether to enable the XFS disk isolator... yes

OR 

checking whether to enable the XFS disk isolator... missing libblkid headers
---
Please install the libblkid development package.
---

OR

checking whether to enable the XFS disk isolator... no / checking whether to 
enable the XFS disk isolator... not enabled by user

```

Agreed?


- Jiang Yan


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


On March 26, 2016, 10:05 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44945/
> ---
> 
> (Updated March 26, 2016, 10:05 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4828
> https://issues.apache.org/jira/browse/MESOS-4828
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add autoconf tests for XFS project quotas.
> 
> 
> Diffs
> -
> 
>   configure.ac 9ec4bc1cff3b0b46dd2e7ece2c1f2d19ffb8 
> 
> Diff: https://reviews.apache.org/r/44945/diff/
> 
> 
> Testing
> ---
> 
> Make check. Manual verification.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44947: Add tests for XFS project quota utilities.

[Jiang Yan Xu]

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



Minor comments. More thorough review once the tests reflect the proposed new 
APIs.


src/tests/containerizer/xfs_quota_tests.cpp (line 17)
<https://reviews.apache.org/r/44947/#comment188324>

Insert a blank line below.



src/tests/containerizer/xfs_quota_tests.cpp (line 51)
<https://reviews.apache.org/r/44947/#comment188325>

`mesos:internal::xfs` goes alphabetically before `process` and leave a 
blank line in between.



src/tests/containerizer/xfs_quota_tests.cpp (line 102)
<https://reviews.apache.org/r/44947/#comment188410>

End the sentence with '.'.



src/tests/containerizer/xfs_quota_tests.cpp (line 115)
<https://reviews.apache.org/r/44947/#comment188414>

In the Mesos codebase C++ style argument comment is preferred: `0, // 
Flags.`



src/tests/containerizer/xfs_quota_tests.cpp (lines 133 - 135)
<https://reviews.apache.org/r/44947/#comment188547>

The indentation here treats `subprocess(` as if it was a method argument.

Suggestion:

```
Try s = subprocess(
"losetup -d " + loopDevice.get(),
Subprocess::PATH("/dev/null"));
```



src/tests/containerizer/xfs_quota_tests.cpp (line 138)
<https://reviews.apache.org/r/44947/#comment188423>

We should give the wait a timeout. e.g., Seconds(15) as we don't want the 
tests to block forever under any circumstances.



src/tests/containerizer/xfs_quota_tests.cpp (line 150)
<https://reviews.apache.org/r/44947/#comment188539>

s/a/an/



src/tests/containerizer/xfs_quota_tests.cpp (line 167)
<https://reviews.apache.org/r/44947/#comment188540>

Doesn't look like we need `ftruncate` if we use `posix_fallocate`?

And you are not checking the exit status here.

IIUC, `os::ftruncate()` is not going to trigger ENOSPC but 
`::posix_fallocate` is. However the method is going to return `Nothing()`.



src/tests/containerizer/xfs_quota_tests.cpp (line 180)
<https://reviews.apache.org/r/44947/#comment188544>

s/ctlfd/fd/



src/tests/containerizer/xfs_quota_tests.cpp (line 189)
<https://reviews.apache.org/r/44947/#comment188412>

s/r//



src/tests/environment.cpp (line 467)
<https://reviews.apache.org/r/44947/#comment188323>

Looks like #if also works but it's more standard to use #ifdef 

http://www.faqs.org/docs/Linux-HOWTO/GCC-HOWTO.html


- Jiang Yan Xu


On March 26, 2016, 10:05 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44947/
> -----------
> 
> (Updated March 26, 2016, 10:05 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4828
> https://issues.apache.org/jira/browse/MESOS-4828
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add tests for XFS project quota utilities.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 6552e48eab2708a28dd69adba3ec759cb5aeca4c 
>   src/tests/containerizer/xfs_quota_tests.cpp PRE-CREATION 
>   src/tests/environment.cpp 7617e43587cb81104786d06f753f08565a6c2d0a 
> 
> Diff: https://reviews.apache.org/r/44947/diff/
> 
> 
> Testing
> ---
> 
> Make check. Manual testing. These tests.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44945: Add autoconf tests for XFS project quotas.

[Jiang Yan Xu]

> On March 25, 2016, 11:43 p.m., Jiang Yan Xu wrote:
> > configure.ac, lines 957-960
> > <https://reviews.apache.org/r/44945/diff/8/?file=1311200#file1311200line957>
> >
> > Can we put AC_MSG_CHECKING before `AC_CHECK_HEADERS` and inside `AS_IF` 
> > above?
> > 
> > i.e., AC_MSG_CHECKING/AC_MSG_CHECKING/AC_MSG_ERROR inform user what 
> > they don't already know (whether the system dependencies are met), not what 
> > they already know (the fact that they provided the 
> > `--enable_xfs_disk_isolator` flag).
> > 
> > And this check is conditional: we only print `AC_MSG_CHECKING` and do 
> > these checks when `test "x$enable_xfs_disk_isolator" = "xyes"`; we print 
> > `AC_MSG_ERROR([...])` when such checks fail and `AC_MSG_RESULT([yes])` when 
> > all checks succeed.
> > 
> > Would this work?
> > 
> > If we do this, then the message could be `AC_MSG_CHECKING([whether we 
> > can enable the XFS disk isolator])`
> 
> James Peach wrote:
> No, this message should be outside the check so that the builder always 
> knows what happened, independent of what configure flags they passed. 
> Typically, people start by not passing any flags, and it is helpful to 
> explicitly log that a feature was not enabled. Even if you pass the --enable 
> flags, it is comforting to get an explicit confirmation that the feature you 
> asked for is enabled.
> 
> Jiang Yan Xu wrote:
> I agree that explicitly confirming that the user has enabled something is 
> better. 
> 
> I still don't think we should print it after we've done all the 
> dependency checks for it and have potentially aborted configure with error 
> messages but without telling the error is due to "whether to enable the XFS 
> disk isolator..."
> 
> i.e., the following is better.
> 
> ```
> checking whether to enable the XFS disk isolator... yes
> 
> OR 
> 
> checking whether to enable the XFS disk isolator... missing libblkid 
> headers
> ---
> Please install the libblkid development package.
> ---
> 
> OR
> 
> checking whether to enable the XFS disk isolator... no / checking whether 
> to enable the XFS disk isolator... not enabled by user
> 
> ```
> 
> Agreed?
> 
> James Peach wrote:
> If you do it this way, you get the headers, etc check output intermingled 
> in the middle of your nice clean feature check message.

I see. Seems like technology is really lacking in this! In the absence of a 
perfect solution, IMO this is better:

checking whether we are asked to enable the XFS disk isolator... yes
checking for xfs/xfs.h... yes
// Move on to other headers

OR

checking whether we are asked to enable the XFS disk isolator... yes
checking for xfs/xfs.h... missing XFS quota headers
---
Please install the Linux kernel headers and xfsprogs development
packages for XFS disk isolator support.
---

OR 
checking whether we are asked to enable the XFS disk isolator... no
// Move on to other feature checking


TBH I haven't seen this style of using AC_MSG_CHECKING to check a user supplied 
flag so I am not convinced it's idiomatic but I do agree with the comforting 
factor. So let's at least make the message honest about what it's doing.

The above style still doesn't print anything when it has finished checking all 
of the dependencies for XFS but it's not like autoconf has ever done a good job 
of this: this is lack of information.

At least we are not printing all these header checking messages and potentially 
fail before we print "checking whether to enable the XFS disk isolator", which 
implies could be misleading.


- Jiang Yan


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


On March 29, 2016, 1:55 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44945/
> ---
> 
> (Updated March 29, 2016, 1:55 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4828
> https://issues.apache.org/jira/browse/MESOS-4828
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add autoconf tests for XFS project quotas.
> 
> 
> Diffs
> -
> 
>   configure.ac 9ec4bc1cff3b0b46dd2e7ece2c1f2d19ffb8 
> 
> Diff: https://reviews.apache.org/r/44945/diff/
> 
> 
> Testing
> ---
> 
> Make check. Manual verification.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44945: Add autoconf tests for XFS project quotas.

[Jiang Yan Xu]

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


Fix it, then Ship it!





configure.ac (line 946)
<https://reviews.apache.org/r/44945/#comment189065>

End with " for XFS disk isolator support".



configure.ac (line 952)
<https://reviews.apache.org/r/44945/#comment189066>

Ditto.



configure.ac (line 960)
<https://reviews.apache.org/r/44945/#comment189067>

Ditto.



configure.ac (lines 967 - 970)
<https://reviews.apache.org/r/44945/#comment189068>

As suggsted in an earlier comment, pulled this up and change the wording 
slightly.


- Jiang Yan Xu


On March 29, 2016, 1:55 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44945/
> ---
> 
> (Updated March 29, 2016, 1:55 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4828
> https://issues.apache.org/jira/browse/MESOS-4828
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add autoconf tests for XFS project quotas.
> 
> 
> Diffs
> -
> 
>   configure.ac 9ec4bc1cff3b0b46dd2e7ece2c1f2d19ffb8 
> 
> Diff: https://reviews.apache.org/r/44945/diff/
> 
> 
> Testing
> ---
> 
> Make check. Manual verification.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44946: Add utility functions to manipulate XFS project quotas.

[Jiang Yan Xu]

ojectQuota()`. (Because this 
entire file is for XFS so a `xfs` prefix doesn't disambiguate as effectively. 
The same applies to using "assign vs. set" to disambiguate.



src/slave/containerizer/mesos/isolators/xfs/utils.cpp (line 172)
<https://reviews.apache.org/r/44946/#comment189217>

Pull this down to right above `info.limit = ...`.



src/slave/containerizer/mesos/isolators/xfs/utils.cpp (line 222)
<https://reviews.apache.org/r/44946/#comment189298>

Align it with the open paren above. We don't use 4 space indentation if 
this is not a list of function arguments.



src/slave/containerizer/mesos/isolators/xfs/utils.cpp (line 245)
<https://reviews.apache.org/r/44946/#comment189218>

As suggested above, we can detect `isdir` inside the method.



src/slave/containerizer/mesos/isolators/xfs/utils.cpp (lines 271 - 273)
<https://reviews.apache.org/r/44946/#comment189297>

Move this to the public version of `setProjectId()`



src/slave/containerizer/mesos/isolators/xfs/utils.cpp (line 302)
<https://reviews.apache.org/r/44946/#comment189221>

As suggested above, kill this method in favor of reusing 
`intenral::setProjectId()`.



src/slave/containerizer/mesos/isolators/xfs/utils.cpp (line 302)
<https://reviews.apache.org/r/44946/#comment189222>





src/slave/containerizer/mesos/isolators/xfs/utils.cpp (lines 333 - 334)
<https://reviews.apache.org/r/44946/#comment189124>

There is a lot of code duplication in the following two methods.

I think it should be reasonable to stick to the previous approach to have a 
common method:

```
Try traverse(
const string& directory,
std::function(const string&, const struct stat&)> callback)
{
}
```

For this review let's keep the helper in this file but it's nevertheless 
more elegant and mantainable.



src/slave/containerizer/mesos/isolators/xfs/utils.cpp (line 335)
<https://reviews.apache.org/r/44946/#comment189133>

rmdir.hpp has this:

```
// NOTE: `fts_open` will not always return `NULL` if the path does not
// exist. We manually induce an error here to indicate that we can't 
remove
// a directory that does not exist.
if (!os::exists(directory)) {
  errno = ENOENT;
  return ErrnoError();
}
```

I think we can do something similar. Plus, will fts_open this fail with 
ENOTDIR if it's not a dir?

I think we do a more explicitly check:

```
if (!stat::isdir(directory)) {
  return Error("'" + directory + "' is not a directory");
}
```



src/slave/containerizer/mesos/isolators/xfs/utils.cpp (line 336)
<https://reviews.apache.org/r/44946/#comment189125>

Plural?

We often use a trailing understore: `char* directory_[]`. 
(s/path/directory/ is due to the suggestion on generalizing the method)

Also, no padding space in `{}`.

Therefore:

```
char* directory_[] = {const_cast(path.c_str()), nullptr};
```



src/slave/containerizer/mesos/isolators/xfs/utils.cpp (line 339)
<https://reviews.apache.org/r/44946/#comment189126>

You've been using nullptr so s/NULL/pullptr/ for consistency.



src/slave/containerizer/mesos/isolators/xfs/utils.cpp (line 344)
<https://reviews.apache.org/r/44946/#comment189128>

One more space to the right to align it with the open paren. (Since this is 
not a function call).



src/slave/containerizer/mesos/isolators/xfs/utils.cpp (lines 346 - 347)
<https://reviews.apache.org/r/44946/#comment189130>

Here we can `callback(node->fts_path, *node->fts_statp);`.



src/slave/containerizer/mesos/isolators/xfs/utils.cpp (line 357)
<https://reviews.apache.org/r/44946/#comment189131>

Here we could have said

```
"Failed to fully process the directory tree '" + path + "'"
```

but we are going to prepend caller specific message anyway so I think 

```
Error error = ErrnoError();
```

is sufficient.



src/slave/containerizer/mesos/isolators/xfs/utils.cpp (lines 366 - 368)
<https://reviews.apache.org/r/44946/#comment189220>

In this method we can just validate the projectId first and then 

```
traverse(
path,
std::bind(internal::setProjectId,
lambda::_1,
projectId,
lambda::_2));
```


- Jiang Yan Xu


On March 30, 2016, 3:18 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44946/
> ---
> 
> (Updated March 30, 2016, 3:18 p.m.)
> 
> 
> Review request for mesos, Jie 

Re: Review Request 44946: Add utility functions to manipulate XFS project quotas.

[Jiang Yan Xu]

> On March 31, 2016, 10:12 a.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/mesos/isolators/xfs/utils.cpp, lines 366-368
> > <https://reviews.apache.org/r/44946/diff/11/?file=1318424#file1318424line366>
> >
> > In this method we can just validate the projectId first and then 
> > 
> > ```
> > traverse(
> > path,
> > std::bind(internal::setProjectId,
> > lambda::_1,
> > projectId,
> > lambda::_2));
> > ```

The above wasn't indented correctly...

```
traverse(
path,
std::bind(
internal::setProjectId,
lambda::_1,
projectId,
lambda::_2));
```


- Jiang Yan


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


On March 30, 2016, 3:18 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44946/
> -----------
> 
> (Updated March 30, 2016, 3:18 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4828
> https://issues.apache.org/jira/browse/MESOS-4828
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add utility functions to manipulate XFS project quotas.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
>   src/slave/containerizer/mesos/isolators/xfs/utils.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/xfs/utils.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44946/diff/
> 
> 
> Testing
> ---
> 
> Make check. Manual verification. Tests in subsequent patches.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44946: Add utility functions to manipulate XFS project quotas.

[Jiang Yan Xu]

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




src/slave/containerizer/mesos/isolators/xfs/utils.hpp (lines 45 - 47)
<https://reviews.apache.org/r/44946/#comment189495>

Sorry it didn't occur to to me earlier but looking at the tests I realized 
that we need to return a Result.

i.e.,

```
Result getProjectQuota(
const std::string& path,
prid_t projectId);
```

because a zero limit is "no limit", hence `None`. We use `setProjectQuota` 
and `clearProjectQuota` to carefully spearate the two use cases so here I think 
it's justified to make zero quota a special type.

(Plus we do this for `getProjectId()` already).


- Jiang Yan Xu


On March 31, 2016, 5:01 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44946/
> ---
> 
> (Updated March 31, 2016, 5:01 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4828
> https://issues.apache.org/jira/browse/MESOS-4828
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add utility functions to manipulate XFS project quotas.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
>   src/slave/containerizer/mesos/isolators/xfs/utils.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/xfs/utils.cpp PRE-CREATION 
>   src/tests/environment.cpp 90dbe9488bda6af26143934e196aab0d69dccec3 
> 
> Diff: https://reviews.apache.org/r/44946/diff/
> 
> 
> Testing
> ---
> 
> Make check. Manual verification. Tests in subsequent patches.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44947: Add tests for XFS project quota utilities.

[Jiang Yan Xu]

his way in the `tearDown()` method we can go through all the them and call 
`clearProjectQuota()` on each.



src/tests/containerizer/xfs_quota_tests.cpp (line 321)
<https://reviews.apache.org/r/44947/#comment189493>

```
EXPECT_SOME_EQ(
makeQuotaInfo(limit, Megabytes(2)),
getProjectQuota(rootA, projectA));
```

Here and below.



src/tests/containerizer/xfs_quota_tests.cpp (lines 348 - 350)
<https://reviews.apache.org/r/44947/#comment189497>

This should be taken care of by TearDown().



src/tests/containerizer/xfs_quota_tests.cpp (lines 359 - 363)
<https://reviews.apache.org/r/44947/#comment189498>

```
EXPECT_SOME_EQ(
makeQuotaInfo(limit, Bytes(0)),
getProjectQuota(path, projectId));
```

As you did above.



src/tests/containerizer/xfs_quota_tests.cpp (line 371)
<https://reviews.apache.org/r/44947/#comment189502>

s/projectId/process/ for consistency.



src/tests/containerizer/xfs_quota_tests.cpp (line 386)
<https://reviews.apache.org/r/44947/#comment189504>

Nit: s/in/to/?



src/tests/containerizer/xfs_quota_tests.cpp (line 387)
<https://reviews.apache.org/r/44947/#comment189501>

Just 

```
EXPECT_SOME(makeFile("file", used));
```

would be sufficient because you only have one file.



src/tests/containerizer/xfs_quota_tests.cpp (lines 390 - 391)
<https://reviews.apache.org/r/44947/#comment189499>

```
EXPECT_SOME_EQ(
makeQuotaInfo(limit, used),
getProjectQuota(path, projectId));
```

The style you used is in the style guide but shouldn't be used if it 
results in too much "jaggedness" (as per the guide).



src/tests/containerizer/xfs_quota_tests.cpp (line 396)
<https://reviews.apache.org/r/44947/#comment189500>

Kill line.


- Jiang Yan Xu


On March 31, 2016, 5 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44947/
> ---
> 
> (Updated March 31, 2016, 5 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4828
> https://issues.apache.org/jira/browse/MESOS-4828
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add tests for XFS project quota utilities.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
>   src/tests/cluster.cpp 2da0bd7612d571277e76d0a95ad8e776434af323 
>   src/tests/containerizer/xfs_quota_tests.cpp PRE-CREATION 
>   src/tests/environment.cpp 90dbe9488bda6af26143934e196aab0d69dccec3 
> 
> Diff: https://reviews.apache.org/r/44947/diff/
> 
> 
> Testing
> ---
> 
> Make check. Manual testing. These tests.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44948: Add XFS disk resource isolator.

[Jiang Yan Xu]

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




src/slave/containerizer/mesos/isolators/disk/xfs.cpp (lines 501 - 503)
<https://reviews.apache.org/r/44948/#comment189628>

Chatted offline. 

Feels like the right thing to do here is to treat a container with no disk 
resource as `limit == 0`, whether the container started with zero disk resource 
or is updated to zero.

Given XFS' lack of ability to enforce disk quota below a basic block 
(512bytes), I think it's OK to do so and in the user doc make it clear that 
"with XFS isolator when no disk resource or zero disk resource is given, XFS 
will limit it at 512bytes (the size of a basic block)." In fact, XFS cannot do 
enforcement at a granularity below 512bytes. However since Mesos' disk resource 
granularity is more coarse than it (at 1KB, or 0.001 of 1MB), this shouldn't 
violate any expectations.

Given that this is a pretty strict behavior (in most cases the task will 
fail immediately because of stdout/stderr/downloads) I think we should act this 
way based on a flag, maybe only if `flags.enforce_container_disk_quota == 
true`, but it raises another question of whether XFS isolator should support 
monitoring-only mode. Therefore I think it can be punted to a later review, 
I'll raise a JIRA intead.


- Jiang Yan Xu


On March 31, 2016, 5:01 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44948/
> ---
> 
> (Updated March 31, 2016, 5:01 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOs-4828
> https://issues.apache.org/jira/browse/MESOs-4828
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Track sandbox directory usage by dynamically assigning XFS project
> quotas. We track a range of XFS project IDs, assigning a project ID
> and a project quota to each sandbox as it is created. When the task
> reaches the quota, writes will fail with EDQUOT, and the task will have
> an opportunity to handle that.
> 
> Quotas are not applied to volume resources since the isolator interface
> has no insight into the volume lifecycle. Thus it is not currently
> possible to accurately assign and reclaim project IDs.
> 
> If LOW is the lower bound of the project ID range and HIGH is the upper
> bound, you can show the currently allocated project quotas using the
> xfs_quota command:
> 
>   $ xfs_quota -x -c "report -a -n -L LOW -U HIGH"
> 
> To show the project ID assigned to the file PATH, use the xfs_io command:
> 
>   $ xfs_io -r -c stat PATH
> 
> 
> Diffs
> -
> 
>   src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
>   src/slave/containerizer/mesos/containerizer.cpp 
> a5dd22380066aa85de04d485052084e2629681c0 
>   src/slave/containerizer/mesos/isolators/xfs/disk.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/xfs/disk.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/xfs/utils.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/xfs/utils.cpp PRE-CREATION 
>   src/slave/flags.hpp d0c606eea74e1a2e69067c43a267047e65a22a04 
>   src/slave/flags.cpp 0551ec334c6747507bf7bb068d27d67f3fdd6c83 
> 
> Diff: https://reviews.apache.org/r/44948/diff/
> 
> 
> Testing
> ---
> 
> Make check. Manual testing. Tests in subsequent patches.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 45689: Make Cluster::Slave more tolerant of start failures.

[Jiang Yan Xu]

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



Not insisting on addressing it here but just would like to mention that we'd 
better not use assertions in here because we want to clean up all containers 
and not terminate early.

This can be done by addressing this 
[TODO](https://github.com/apache/mesos/blob/f921302b0a0d7c37ba1205ad12857ade614d04ff/3rdparty/libprocess/include/process/gtest.hpp#L206)
 and using it:
```
TODO(bmahler): Differentiate EXPECT and ASSERT here.
```

- Jiang Yan Xu


On April 4, 2016, 10:31 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45689/
> ---
> 
> (Updated April 4, 2016, 10:31 a.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere, Joseph Wu, and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If Cluster::Slave::start() fails, make sure we don't crash in the
> destructor.
> 
> 
> Diffs
> -
> 
>   src/tests/cluster.cpp 14d0d34fcb4c408ad996672394c39c84fd2be918 
> 
> Diff: https://reviews.apache.org/r/45689/diff/
> 
> 
> Testing
> ---
> 
> Make check.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 45689: Make Cluster::Slave more tolerant of start failures.

[Jiang Yan Xu]

> On April 4, 2016, 10:35 a.m., haosdent huang wrote:
> > src/tests/cluster.cpp, line 437
> > <https://reviews.apache.org/r/45689/diff/1/?file=1324705#file1324705line437>
> >
> > how about
> > 
> > ```
> > if (!containerizer) {
> >   return;
> > }
> > ```

+1 this is better.


- Jiang Yan


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


On April 4, 2016, 10:31 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45689/
> ---
> 
> (Updated April 4, 2016, 10:31 a.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere, Joseph Wu, and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If Cluster::Slave::start() fails, make sure we don't crash in the
> destructor.
> 
> 
> Diffs
> -
> 
>   src/tests/cluster.cpp 14d0d34fcb4c408ad996672394c39c84fd2be918 
> 
> Diff: https://reviews.apache.org/r/45689/diff/
> 
> 
> Testing
> ---
> 
> Make check.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 45689: Make Cluster::Slave more tolerant of start failures.

[Jiang Yan Xu]

> On April 4, 2016, 10:35 a.m., haosdent huang wrote:
> > src/tests/cluster.cpp, line 437
> > <https://reviews.apache.org/r/45689/diff/1/?file=1324705#file1324705line437>
> >
> > how about
> > 
> > ```
> > if (!containerizer) {
> >   return;
> > }
> > ```
> 
> Jiang Yan Xu wrote:
> +1 this is better.
> 
> James Peach wrote:
> You can't early return because you have to call ``terminate()``.

Alright, overlooked it :) However it looks like the lines above this suffer 
from the same problem:

```
if (!cleanUpContainersInDestructor) {
  return;
}
```

In generl I think this is OK:


How about 

```
if (!containerizer || !cleanUpContainersInDestructor) {
  terminate();
  return;
}

...
```


- Jiang Yan


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


On April 4, 2016, 10:31 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45689/
> ---
> 
> (Updated April 4, 2016, 10:31 a.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere, Joseph Wu, and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If Cluster::Slave::start() fails, make sure we don't crash in the
> destructor.
> 
> 
> Diffs
> -
> 
>   src/tests/cluster.cpp 14d0d34fcb4c408ad996672394c39c84fd2be918 
> 
> Diff: https://reviews.apache.org/r/45689/diff/
> 
> 
> Testing
> ---
> 
> Make check.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 45046: Created URI.filename to name fetched files in sandbox where appropriate.

[Jiang Yan Xu]

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




src/slave/containerizer/fetcher.cpp (line 150)
<https://reviews.apache.org/r/45046/#comment190155>

What does this check against?

`Path(filename).basename()` never returns an Error().


- Jiang Yan Xu


On March 20, 2016, 8:14 p.m., Michael Browning wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45046/
> ---
> 
> (Updated March 20, 2016, 8:14 p.m.)
> 
> 
> Review request for mesos, Vinod Kone and Zhitao Li.
> 
> 
> Bugs: MESOS-4735
> https://issues.apache.org/jira/browse/MESOS-4735
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Created URI.filename to name fetched files in sandbox where appropriate.
> 
> 
> Diffs
> -
> 
>   CHANGELOG 761238c48332bcce0bff6c411225fdb4176ddca6 
>   docs/fetcher.md f70939d8410516c9387a8cba86b5b75539a5fe9a 
>   include/mesos/mesos.proto deb9c0910a27afd67276f54b3f666a878212727b 
>   include/mesos/v1/mesos.proto a981e750c24cfc48177bbc9ca56f0c3ecfae1a1b 
>   src/launcher/fetcher.cpp f85b118fb19cf9d4563f89847a783be35067e815 
>   src/slave/containerizer/fetcher.hpp 
> bbdce88da6e41dbb88681bc9d604b00923033b3d 
>   src/slave/containerizer/fetcher.cpp 
> 33dfcade6beb53a5a6dbc41a8f3380f5cb30a161 
>   src/tests/fetcher_cache_tests.cpp 645dae208cb2b0aa2d2181d96eb1fd8893975430 
>   src/tests/fetcher_tests.cpp fb47706eb90ae5808bafe13c681d609a808b0c8e 
> 
> Diff: https://reviews.apache.org/r/45046/diff/
> 
> 
> Testing
> ---
> 
> There are two paths by which a file gets fetched to the executor sandbox: the 
> without-cache path, where the fetcher downloads the file directly from the 
> specified URI, and the with-cache path, where it copies it from the cache. In 
> both cases, we verify that the file is saved to the sandbox directory with 
> the name specified by the "filename" field in the CommandInfo.URI proto.
> 
> 
> Thanks,
> 
> Michael Browning
> 
>

Re: Review Request 44948: Add XFS disk resource isolator.

[Jiang Yan Xu]

ase `xfs::setProjectId()` could partially fail which makes it 
unsafe to return the project ID.

I think we can just do a hard CHECK before calling to make sure the 
directory is empty. If no such method is directory usable right now, add a TODO 
and let's add one to stout.



src/slave/containerizer/mesos/isolators/xfs/disk.cpp (lines 332 - 333)
<https://reviews.apache.org/r/44948/#comment190201>

The 'resources' here can be long as it includes all resources. We probably 
don't need to log here as we are already logging all outcomes.



src/slave/containerizer/mesos/isolators/xfs/disk.cpp (lines 338 - 340)
<https://reviews.apache.org/r/44948/#comment190203>

We can take care of this in a subsequent review but let's insert a 
LOG(WARNING) here and add a TODO: semantially we should set the quota to 0 but 
given XFS' enforceability limitation it's going to be limited at 1 basic block.



src/slave/containerizer/mesos/isolators/xfs/disk.cpp (lines 436 - 440)
<https://reviews.apache.org/r/44948/#comment190204>

We don't need to dispatch again. `_cleanup`'s content can just be moved 
here.


- Jiang Yan Xu


On April 4, 2016, 10:28 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44948/
> ---
> 
> (Updated April 4, 2016, 10:28 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOs-4828
> https://issues.apache.org/jira/browse/MESOs-4828
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Track sandbox directory usage by dynamically assigning XFS project
> quotas. We track a range of XFS project IDs, assigning a project ID
> and a project quota to each sandbox as it is created. When the task
> reaches the quota, writes will fail with EDQUOT, and the task will have
> an opportunity to handle that.
> 
> Quotas are not applied to volume resources since the isolator interface
> has no insight into the volume lifecycle. Thus it is not currently
> possible to accurately assign and reclaim project IDs.
> 
> If LOW is the lower bound of the project ID range and HIGH is the upper
> bound, you can show the currently allocated project quotas using the
> xfs_quota command:
> 
>   $ xfs_quota -x -c "report -a -n -L LOW -U HIGH"
> 
> To show the project ID assigned to the file PATH, use the xfs_io command:
> 
>   $ xfs_io -r -c stat PATH
> 
> 
> Diffs
> -
> 
>   src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
>   src/slave/containerizer/mesos/containerizer.cpp 
> a5dd22380066aa85de04d485052084e2629681c0 
>   src/slave/containerizer/mesos/isolators/xfs/disk.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/xfs/disk.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/xfs/utils.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/xfs/utils.cpp PRE-CREATION 
>   src/slave/flags.hpp d0c606eea74e1a2e69067c43a267047e65a22a04 
>   src/slave/flags.cpp 0551ec334c6747507bf7bb068d27d67f3fdd6c83 
> 
> Diff: https://reviews.apache.org/r/44948/diff/
> 
> 
> Testing
> ---
> 
> Make check. Manual testing. Tests in subsequent patches.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44948: Add XFS disk resource isolator.

[Jiang Yan Xu]

> On March 24, 2016, 9:54 a.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/mesos/isolators/disk/xfs.cpp, line 568
> > <https://reviews.apache.org/r/44948/diff/8/?file=1311210#file1311210line568>
> >
> > It seems that we don't need to modify `totalProjectIds` (which can be a 
> > const) here, this change doesn't survive agent restart anyways and we use 
> > `freeProjectIds` to assign IDs.
> 
> James Peach wrote:
> I think it is better to handle this consistently. We use 
> ``totalProjectIds`` to bound the set of possible IDs and this ID is not 
> longer possible.

I am not sure about the intended consistency here because I can't think of a 
case where in the following place:

```
if (totalProjectIds.contains(projectId)) {
  freeProjectIds += projectId;
}
```

`projectId` could be in `totalProjectIds` (and therefore incorrectly returned) 
if `totalProjectIds` was always the initial value but would not be in 
`totalProjectIds` if we modify `totalProjectIds` like we currently do in the 
code.

Can you think of any?


> On March 24, 2016, 9:54 a.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/mesos/isolators/disk/xfs.cpp, lines 501-503
> > <https://reviews.apache.org/r/44948/diff/8/?file=1311210#file1311210line501>
> >
> > If the new resources have no disk resources, shouldn't we cleanup the 
> > sandbox?
> 
> James Peach wrote:
> It's not clear to me what this means. If you had a disk resource and now 
> you don't are you unrestricted or do you have zero disk? I think it is better 
> to only remove the project ID in cleanup, but maybe we should still adjust 
> the quota here?

Dropping this for a new suggestion in the new review.


- Jiang Yan


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


On April 4, 2016, 10:28 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44948/
> ---
> 
> (Updated April 4, 2016, 10:28 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOs-4828
> https://issues.apache.org/jira/browse/MESOs-4828
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Track sandbox directory usage by dynamically assigning XFS project
> quotas. We track a range of XFS project IDs, assigning a project ID
> and a project quota to each sandbox as it is created. When the task
> reaches the quota, writes will fail with EDQUOT, and the task will have
> an opportunity to handle that.
> 
> Quotas are not applied to volume resources since the isolator interface
> has no insight into the volume lifecycle. Thus it is not currently
> possible to accurately assign and reclaim project IDs.
> 
> If LOW is the lower bound of the project ID range and HIGH is the upper
> bound, you can show the currently allocated project quotas using the
> xfs_quota command:
> 
>   $ xfs_quota -x -c "report -a -n -L LOW -U HIGH"
> 
> To show the project ID assigned to the file PATH, use the xfs_io command:
> 
>   $ xfs_io -r -c stat PATH
> 
> 
> Diffs
> -
> 
>   src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
>   src/slave/containerizer/mesos/containerizer.cpp 
> a5dd22380066aa85de04d485052084e2629681c0 
>   src/slave/containerizer/mesos/isolators/xfs/disk.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/xfs/disk.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/xfs/utils.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/xfs/utils.cpp PRE-CREATION 
>   src/slave/flags.hpp d0c606eea74e1a2e69067c43a267047e65a22a04 
>   src/slave/flags.cpp 0551ec334c6747507bf7bb068d27d67f3fdd6c83 
> 
> Diff: https://reviews.apache.org/r/44948/diff/
> 
> 
> Testing
> ---
> 
> Make check. Manual testing. Tests in subsequent patches.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44946: Add utility functions to manipulate XFS project quotas.

[Jiang Yan Xu]

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


Fix it, then Ship it!




Looking good. Just a few minor comments, plus one mentioned in [another 
review](https://reviews.apache.org/r/44948/diff/11/?file=1321656#file1321656line140)
 and we are good to go.


src/slave/containerizer/mesos/isolators/xfs/utils.hpp (lines 61 - 62)
<https://reviews.apache.org/r/44946/#comment190394>

s/directory/path/, this breaks the symmetry in the `*projectId` methods but 
this doesn't need to be a directory and we actually use it to test files in 
tests.



src/slave/containerizer/mesos/isolators/xfs/utils.hpp (line 72)
<https://reviews.apache.org/r/44946/#comment190312>

Kill line.



src/slave/containerizer/mesos/isolators/xfs/utils.hpp (line 78)
<https://reviews.apache.org/r/44946/#comment190395>

`s/__XFS_HPP__/__XFS_UTILS_HPP__`.



src/slave/containerizer/mesos/isolators/xfs/utils.cpp (line 59)
<https://reviews.apache.org/r/44946/#comment190321>

Add an empty line.



src/slave/containerizer/mesos/isolators/xfs/utils.cpp (lines 154 - 155)
<https://reviews.apache.org/r/44946/#comment190404>

I didn't mention this earlier but it would be nice if all these internal 
arithmetics use the Bytes object and only convert to a uint when it calls 
underlying systems calls.

A TODO would be fine.



src/slave/containerizer/mesos/isolators/xfs/utils.cpp (line 265)
<https://reviews.apache.org/r/44946/#comment190398>

"larger than or equal to" or ">=" so it matches the condition check.



src/slave/containerizer/mesos/isolators/xfs/utils.cpp (line 266)
<https://reviews.apache.org/r/44946/#comment190406>

Nit: `stringify(Bytes(BASIC_BLOCK_SIZE))`. 

I have another comment about a TODO for using Bytes consistently, including 
the type of BASIC_BLOCK_SIZE. But OK to not do it in this review.



src/slave/containerizer/mesos/isolators/xfs/utils.cpp (line 286)
<https://reviews.apache.org/r/44946/#comment190396>

`s/directory/path`.



src/tests/environment.cpp (lines 468 - 471)
<https://reviews.apache.org/r/44946/#comment190387>

This is unrelated?


- Jiang Yan Xu


On April 4, 2016, 10:27 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44946/
> -----------
> 
> (Updated April 4, 2016, 10:27 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4828
> https://issues.apache.org/jira/browse/MESOS-4828
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add utility functions to manipulate XFS project quotas.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
>   src/slave/containerizer/mesos/isolators/xfs/utils.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/xfs/utils.cpp PRE-CREATION 
>   src/tests/environment.cpp 90dbe9488bda6af26143934e196aab0d69dccec3 
> 
> Diff: https://reviews.apache.org/r/44946/diff/
> 
> 
> Testing
> ---
> 
> Make check. Manual verification. Tests in subsequent patches.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 45689: Make Cluster::Slave more tolerant of start failures.

[Jiang Yan Xu]

> On April 5, 2016, 12:52 p.m., Joseph Wu wrote:
> > src/tests/cluster.cpp, lines 364-366
> > <https://reviews.apache.org/r/45689/diff/1/?file=1324705#file1324705line364>
> >
> > The destructor will only dereference a null `containerizer` if this 
> > error case is hit (or if you pass in a `nullptr` cast to 
> > `slave::Containerizer*`).
> > 
> > At this point in the code, we haven't done anything other than create 
> > some objects.  So it should be ok to do something like this in the 
> > destructor:
> > ```
> > if (containerizer == nullptr) {
> >   return;
> > }
> > ```
> > The call to `terminate()` only matters if the `cluster::Slave` is 
> > created successfully.

I agree that Haosdent's suggestion works for the purpose of this fix. But if 
"The call to terminate() only matters if the cluster::Slave is created 
successfully", then we should call terminate() under the condition that `pid` 
is not empty in `~Slave()`. We should fix it in a separate review.


- Jiang Yan


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


On April 4, 2016, 10:31 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45689/
> -----------
> 
> (Updated April 4, 2016, 10:31 a.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere, Joseph Wu, and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If Cluster::Slave::start() fails, make sure we don't crash in the
> destructor.
> 
> 
> Diffs
> -
> 
>   src/tests/cluster.cpp 14d0d34fcb4c408ad996672394c39c84fd2be918 
> 
> Diff: https://reviews.apache.org/r/45689/diff/
> 
> 
> Testing
> ---
> 
> Make check.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44947: Add tests for XFS project quota utilities.

[Jiang Yan Xu]

> On April 1, 2016, 12:27 a.m., Jiang Yan Xu wrote:
> > src/tests/containerizer/xfs_quota_tests.cpp, line 71
> > <https://reviews.apache.org/r/44947/diff/11/?file=1320117#file1320117line71>
> >
> > We have 
> > 
> > `makeQuotaInfo` vs. `mkfile` & `mkloop`. Can we make the use of the 
> > word `make` or `mk` consistent? Generally we avoid abbreviations so using 
> > `make` (and camelCasing) is preferred.
> 
> James Peach wrote:
> ``mkfile`` and ``mkloop`` are named after ``mkfile(1)``, ``mknod(1)``, 
> etc. I expected that would be a fairly familiar nomenclature.
> 
> ``makeQuotaInfo`` is substantially different so there's no good reason to 
> use the same naming scheme. It is following the ``std::make_pair`` pattern, 
> but using Mesos naming conventions.

SGTM!


- Jiang Yan


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


On April 4, 2016, 10:27 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44947/
> -----------
> 
> (Updated April 4, 2016, 10:27 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4828
> https://issues.apache.org/jira/browse/MESOS-4828
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add tests for XFS project quota utilities.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
>   src/tests/containerizer/xfs_quota_tests.cpp PRE-CREATION 
>   src/tests/environment.cpp 90dbe9488bda6af26143934e196aab0d69dccec3 
> 
> Diff: https://reviews.apache.org/r/44947/diff/
> 
> 
> Testing
> ---
> 
> Make check. Manual testing. These tests.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 45689: Make Cluster::Slave more tolerant of start failures.

[Jiang Yan Xu]

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


Ship it!





src/tests/cluster.hpp (line 188)
<https://reviews.apache.org/r/45689/#comment190480>

These are default initialized anyways but explicitness is good.


- Jiang Yan Xu


On April 5, 2016, 4:41 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45689/
> ---
> 
> (Updated April 5, 2016, 4:41 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere, Joseph Wu, and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If Cluster::Slave::start() fails, make sure we don't crash in the
> destructor.
> 
> 
> Diffs
> -
> 
>   src/tests/cluster.hpp 91d3998176115002ea22a0825b1aaaee3fe0d768 
>   src/tests/cluster.cpp eefc2fa55bca1ad6a53047046fa4f5996d5c3fef 
> 
> Diff: https://reviews.apache.org/r/45689/diff/
> 
> 
> Testing
> ---
> 
> Make check.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44947: Add tests for XFS project quota utilities.

[Jiang Yan Xu]

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


Fix it, then Ship it!





src/tests/containerizer/xfs_quota_tests.cpp (line 154)
<https://reviews.apache.org/r/44947/#comment190551>

Methods inside a class should be separated by one blank line. Here and 
below.



src/tests/containerizer/xfs_quota_tests.cpp (line 163)
<https://reviews.apache.org/r/44947/#comment190482>

We changed this to "xfs/disk". Strictly speaking this belongs to 
<https://reviews.apache.org/r/44949/> as it's not used here.



src/tests/containerizer/xfs_quota_tests.cpp (line 215)
<https://reviews.apache.org/r/44947/#comment190548>

Move this one to the bottom as it is the most complex test.



src/tests/containerizer/xfs_quota_tests.cpp (lines 271 - 282)
<https://reviews.apache.org/r/44947/#comment190550>

These checks are not related to the rest of the test above and should be in 
a separate test. Maybe

```
TEST_F(ROOT_XFS_QuotaTest, ProjectIdErrors)
{
}

```

It can be the placed at the top as the first test.



src/tests/containerizer/xfs_quota_tests.cpp (line 306)
<https://reviews.apache.org/r/44947/#comment190547>

Add a check that expects mkfile() to return an error when it goes past the 
limit.


- Jiang Yan Xu


On April 5, 2016, 4:40 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44947/
> ---
> 
> (Updated April 5, 2016, 4:40 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4828
> https://issues.apache.org/jira/browse/MESOS-4828
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add tests for XFS project quota utilities.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
>   src/tests/containerizer/xfs_quota_tests.cpp PRE-CREATION 
>   src/tests/environment.cpp 2afaa328a0fb226a2d1ca35a4754ccb274bc075d 
> 
> Diff: https://reviews.apache.org/r/44947/diff/
> 
> 
> Testing
> ---
> 
> Make check. Manual testing. These tests.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44948: Add XFS disk resource isolator.

[Jiang Yan Xu]

(lines 419 - 430)
<https://reviews.apache.org/r/44948/#comment190678>

This seems better:

```
if (freeProjectIds.empty()) {
  return None();
}

prid_t projectId = freeProjectIds.begin()->lower();
freeProjectIds -= projectId;
return projectId;
```



src/slave/containerizer/mesos/isolators/xfs/disk.cpp (lines 420 - 422)
<https://reviews.apache.org/r/44948/#comment190672>

Took another look at this: it seems that this won't happen. A few tests I 
wrote (plus the ones already in interval_tests.cpp show that empty intervals 
get eliminated automatically.



src/slave/flags.cpp (line 777)
<https://reviews.apache.org/r/44948/#comment190651>

s/range/ranges/ since this is what we actually support.


- Jiang Yan Xu


On April 6, 2016, 11:43 a.m., James Peach wrote:
> 
> -------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44948/
> ---
> 
> (Updated April 6, 2016, 11:43 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOs-4828
> https://issues.apache.org/jira/browse/MESOs-4828
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Track sandbox directory usage by dynamically assigning XFS project
> quotas. We track a range of XFS project IDs, assigning a project ID
> and a project quota to each sandbox as it is created. When the task
> reaches the quota, writes will fail with EDQUOT, and the task will have
> an opportunity to handle that.
> 
> Quotas are not applied to volume resources since the isolator interface
> has no insight into the volume lifecycle. Thus it is not currently
> possible to accurately assign and reclaim project IDs.
> 
> If LOW is the lower bound of the project ID range and HIGH is the upper
> bound, you can show the currently allocated project quotas using the
> xfs_quota command:
> 
>   $ xfs_quota -x -c "report -a -n -L LOW -U HIGH"
> 
> To show the project ID assigned to the file PATH, use the xfs_io command:
> 
>   $ xfs_io -r -c stat PATH
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 55d3b341361bed25f3aa966d77060c88be29e5b0 
>   src/slave/containerizer/mesos/containerizer.cpp 
> a5dd22380066aa85de04d485052084e2629681c0 
>   src/slave/containerizer/mesos/isolators/xfs/disk.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/xfs/disk.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/xfs/utils.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/xfs/utils.cpp PRE-CREATION 
>   src/slave/flags.hpp 69e1b01e09d2a15bee5e0745b751f47aaefe3fbe 
>   src/slave/flags.cpp 315cf47d268bce0a0255a061d64e414c736c8125 
> 
> Diff: https://reviews.apache.org/r/44948/diff/
> 
> 
> Testing
> ---
> 
> Make check. Manual testing. Tests in subsequent patches.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44946: Add utility functions to manipulate XFS project quotas.

[Jiang Yan Xu]

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


Fix it, then Ship it!





src/slave/containerizer/mesos/isolators/xfs/utils.cpp (line 58)
<https://reviews.apache.org/r/44946/#comment190806>

Nit: 

`static constexpr Bytes BASIC_BLOCK_SIZE = Bytes(512);` 

for consistency just because this is how constants in Mesos are generally 
constructed (copy constructed).


- Jiang Yan Xu


On April 6, 2016, 11:42 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44946/
> ---
> 
> (Updated April 6, 2016, 11:42 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4828
> https://issues.apache.org/jira/browse/MESOS-4828
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add utility functions to manipulate XFS project quotas.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 55d3b341361bed25f3aa966d77060c88be29e5b0 
>   src/slave/containerizer/mesos/isolators/xfs/utils.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/xfs/utils.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44946/diff/
> 
> 
> Testing
> ---
> 
> Make check. Manual verification. Tests in subsequent patches.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44949: Add XFS disk isolator tests.

[Jiang Yan Xu]

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




src/tests/containerizer/xfs_quota_tests.cpp (line 338)
<https://reviews.apache.org/r/44949/#comment190850>

These more complex and integration oriented tests should all have a header 
comment so it makes it easier for 

Something like:
```
// This test verifies that a task fails to write to disk when it 
// exceeds the requested disk quota.
```



src/tests/containerizer/xfs_quota_tests.cpp (line 345)
<https://reviews.apache.org/r/44949/#comment190811>

Two spaces to continue after `=`.

Here and elsewhere in this test.



src/tests/containerizer/xfs_quota_tests.cpp (line 357)
<https://reviews.apache.org/r/44949/#comment190812>

One space between `;` and `//`.

I saw some other test code that does this but it's probably due to copying 
and pasting of bad examples. Unless it's to align with some other comments, use 
one space.

Here and elsewhere.



src/tests/containerizer/xfs_quota_tests.cpp (line 390)
<https://reviews.apache.org/r/44949/#comment190818>

s/a/an.



src/tests/containerizer/xfs_quota_tests.cpp (line 425)
<https://reviews.apache.org/r/44949/#comment190837>

`s/Future >/Future>/` here and elsewhere.



src/tests/containerizer/xfs_quota_tests.cpp (line 461)
<https://reviews.apache.org/r/44949/#comment190838>

`Timeout::in()` and `Timeout::expired()` is more suitable for this.



src/tests/containerizer/xfs_quota_tests.cpp (line 486)
<https://reviews.apache.org/r/44949/#comment190851>

With Timeout you don't need to do this.



src/tests/containerizer/xfs_quota_tests.cpp (lines 526 - 531)
<https://reviews.apache.org/r/44949/#comment190917>

Here we seem to be implying that we are expecting the task to die because 
it exceeds the disk quota but in fact we are just testing slave recovery and 
the task doesn't die because of the `;`.

How about we just request 1MB and use 1MB (or not use anything at all (just 
sleep) since we are mainly verifying if the project ID is cleared afterwards.



src/tests/containerizer/xfs_quota_tests.cpp (line 545)
<https://reviews.apache.org/r/44949/#comment190852>

Two spaces.



src/tests/containerizer/xfs_quota_tests.cpp (line 551)
<https://reviews.apache.org/r/44949/#comment190915>

This is not guaranteed if `dd` is slow to write to the disk and the rest of 
the test proceeds quickly.

I think it's OK if we don't verify this here because the two tests above 
already verified that disk usage is correctly reported and correctly capped at 
the limit.



src/tests/containerizer/xfs_quota_tests.cpp (lines 565 - 566)
<https://reviews.apache.org/r/44949/#comment190857>

This is not necessary if we

```
AWAIT_READY(slaveReregisteredMessage);
```

because reregistration only happens after `Slave::_recover`.



src/tests/containerizer/xfs_quota_tests.cpp (line 590)
<https://reviews.apache.org/r/44949/#comment190925>

Additionally we should assert there is exactly one container. Otherwise the 
foreach below can fall through without triggering any expectations if 
`sandboxees` is empty.



src/tests/containerizer/xfs_quota_tests.cpp (line 593)
<https://reviews.apache.org/r/44949/#comment190926>

`s/foreach(/foreach (/`



src/tests/containerizer/xfs_quota_tests.cpp (lines 680 - 691)
<https://reviews.apache.org/r/44949/#comment190922>

This part is not necessary. Slave considers itself fully recovered when the 
containers are fully recovered, not when the executors have all reregistered. 
Also the exchange of reconnect and reregister messages doesn't go through a 
delay so no need to advance (only the `reregisterExecutorTimeout` goes through 
a delay).

Simply removing these lines should do it.



src/tests/containerizer/xfs_quota_tests.cpp (line 702)
<https://reviews.apache.org/r/44949/#comment190923>

This is again not guaranteed. I think it's good enough if we just verify 
the limits.



src/tests/containerizer/xfs_quota_tests.cpp (line 718)
<https://reviews.apache.org/r/44949/#comment190927>

`s/foreach(/foreach (/`


- Jiang Yan Xu


On April 6, 2016, 3:37 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44949/
> ---
> 
> (Updated April 6, 2016, 3:37 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4828
> https://issues.apache.org/jira/browse/MESOS-4828
> 
> 

Re: Review Request 44950: Add XFS disk isolator documentation.

[Jiang Yan Xu]

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


Fix it, then Ship it!




It's not mentioned in the test summary so could you also verify the formatting 
in a markdown viewer?

We should follow this up with https://issues.apache.org/jira/browse/MESOS-5140.


docs/configuration.md (line 1746)
<https://reviews.apache.org/r/44950/#comment190936>

s/range/ranges/



docs/configuration.md (lines 1746 - 1747)
<https://reviews.apache.org/r/44950/#comment190939>

Also mention "Valid project IDs range from 1 to max(uint32)" before 
"(default: ...)"



docs/configuration.md (line 1747)
<https://reviews.apache.org/r/44950/#comment190938>

s/The default is `[5000-1]`/(default: `[5000-1]`)/ so it looks the 
same as the rest of doc.



docs/mesos-containerizer.md (line 94)
<https://reviews.apache.org/r/44950/#comment190940>

"not be terminated by the containerizer."



docs/mesos-containerizer.md (line 106)
<https://reviews.apache.org/r/44950/#comment190945>

Mention:

1. The XFS disk isolator is functionally similar to Posix disk isolator but 
avoids the `du` cost. It's not recommended to use them together, even though 
they shouldn't interfere with each other.
2. The XFS disk isolator doesn't natively currently support accounting-only 
mode like Posix isolator does. (I guess we don't need to mention the 
`gqnoenforce` option just yet) 
3. The slave flags mentioned in the "Posix Disk Isolator" section are not 
applicable to the XFS disk isolator.

Change the wording as you see fit.


- Jiang Yan Xu


On April 6, 2016, 3:36 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44950/
> -----------
> 
> (Updated April 6, 2016, 3:36 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOs-4828
> https://issues.apache.org/jira/browse/MESOs-4828
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add XFS disk isolator documentation.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 309a5a05eab386c8943ba6bdee8d5efeb448aa0c 
>   docs/mesos-containerizer.md 2fde74362f35568bb944da80a1db17e6d3ba9d7b 
> 
> Diff: https://reviews.apache.org/r/44950/diff/
> 
> 
> Testing
> ---
> 
> Make check. Source inspection.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44948: Add XFS disk resource isolator.

[Jiang Yan Xu]

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


Ship it!




Ship It!

- Jiang Yan Xu


On April 6, 2016, 3:36 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44948/
> ---
> 
> (Updated April 6, 2016, 3:36 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOs-4828
> https://issues.apache.org/jira/browse/MESOs-4828
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Track sandbox directory usage by dynamically assigning XFS project
> quotas. We track a range of XFS project IDs, assigning a project ID
> and a project quota to each sandbox as it is created. When the task
> reaches the quota, writes will fail with EDQUOT, and the task will have
> an opportunity to handle that.
> 
> Quotas are not applied to volume resources since the isolator interface
> has no insight into the volume lifecycle. Thus it is not currently
> possible to accurately assign and reclaim project IDs.
> 
> If LOW is the lower bound of the project ID range and HIGH is the upper
> bound, you can show the currently allocated project quotas using the
> xfs_quota command:
> 
>   $ xfs_quota -x -c "report -a -n -L LOW -U HIGH"
> 
> To show the project ID assigned to the file PATH, use the xfs_io command:
> 
>   $ xfs_io -r -c stat PATH
> 
> 
> Diffs
> -
> 
>   src/Makefile.am ba9cc8b683bba2ae8fe9d9c58642690f5b80afaf 
>   src/slave/containerizer/mesos/containerizer.cpp 
> a5dd22380066aa85de04d485052084e2629681c0 
>   src/slave/containerizer/mesos/isolators/xfs/disk.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/xfs/disk.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/xfs/utils.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/xfs/utils.cpp PRE-CREATION 
>   src/slave/flags.hpp 69e1b01e09d2a15bee5e0745b751f47aaefe3fbe 
>   src/slave/flags.cpp 315cf47d268bce0a0255a061d64e414c736c8125 
> 
> Diff: https://reviews.apache.org/r/44948/diff/
> 
> 
> Testing
> ---
> 
> Make check. Manual testing. Tests in subsequent patches.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44949: Add XFS disk isolator tests.

[Jiang Yan Xu]

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


Ship it!




Ship It!

- Jiang Yan Xu


On April 7, 2016, 2:50 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44949/
> ---
> 
> (Updated April 7, 2016, 2:50 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4828
> https://issues.apache.org/jira/browse/MESOS-4828
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add basic XFS disk isolator tests by cloning the POSIX disk isolator
> tests and making minor changes for the differences in semantics.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/xfs_quota_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44949/diff/
> 
> 
> Testing
> ---
> 
> Make check. Manual testing.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44949: Add XFS disk isolator tests.

[Jiang Yan Xu]

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


Ship it!




Only some nits which I'll fix when committing.


src/tests/containerizer/xfs_quota_tests.cpp (line 344)
<https://reviews.apache.org/r/44949/#comment191301>

s/that/than/.



src/tests/containerizer/xfs_quota_tests.cpp (line 345)
<https://reviews.apache.org/r/44949/#comment191299>

s/consumed/consume/.



src/tests/containerizer/xfs_quota_tests.cpp (line 597)
<https://reviews.apache.org/r/44949/#comment191300>

A little explanaion of `2u` would be make it clearer.



src/tests/containerizer/xfs_quota_tests.cpp (line 654)
<https://reviews.apache.org/r/44949/#comment191305>

Let's do `count=1`, same as the previous test.


- Jiang Yan Xu


On April 7, 2016, 2:50 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44949/
> ---
> 
> (Updated April 7, 2016, 2:50 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4828
> https://issues.apache.org/jira/browse/MESOS-4828
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add basic XFS disk isolator tests by cloning the POSIX disk isolator
> tests and making minor changes for the differences in semantics.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/xfs_quota_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44949/diff/
> 
> 
> Testing
> ---
> 
> Make check. Manual testing.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 45959: Support arithmetic operations for shared resources with consumer counts.

[Jiang Yan Xu]

`a` has 4 counts of `sr` and `b` has 3, then `a` contains `b` and `b` 
doesn't contain `a`, right?



src/common/resources.cpp (lines 1259 - 1286)
<https://reviews.apache.org/r/45959/#comment192685>

No need for these.



src/common/resources.cpp (lines 1259 - 1286)
<https://reviews.apache.org/r/45959/#comment192686>

No need for this.



src/common/resources.cpp (line 1430)
<https://reviews.apache.org/r/45959/#comment192687>

Also kill this hunk.



src/common/resources.cpp (line 1505)
<https://reviews.apache.org/r/45959/#comment192688>

Add a blank line above.



src/common/resources.cpp (line 1545)
<https://reviews.apache.org/r/45959/#comment192647>

The logic that involves updating the counter should be inside `Resource_` 
and it can rely on the `Resource` operators. 

i.e. something like

```
Resource_& Resource_::operator+=(const Resource_& that)
{
  if (!isShared() && !that.isShared()) {
this.resource += that.resource;  
  } else if (resource == that.resource) {
sharedCount += that.sharedCount;
  }
  
  return *this;
}
```

Furthermore, comparing the two methods

```
Resources& Resources::operator+=(const Resource& that)
Resources& Resources::operator+=(const Resource_& that)
```

Since Resource to implicitly convertible to Resource_, we don't need the 
first method.



src/common/resources.cpp (line 1591)
<https://reviews.apache.org/r/45959/#comment192689>

Per the comment above, we can modify this method to take `const Resource_&` 
and keep most of the method's body unchanged.



src/common/resources.cpp (lines 1596 - 1603)
<https://reviews.apache.org/r/45959/#comment192665>

So there:

```
if (internal::addable(resource_.resource, that)) {
  // This applies to both shared and nonshared resources.
  resource_ += that; // This should be implemented in Resource_.
  found = true;
  break;
}
```

Most of these don't have to change.



src/common/resources.cpp (line 1609)
<https://reviews.apache.org/r/45959/#comment192666>

Here simply:

```
resources.push_back(that);
```



src/common/resources.cpp (line 1619)
<https://reviews.apache.org/r/45959/#comment192690>

s/that.resources/that/



src/common/resources.cpp (line 1651)
<https://reviews.apache.org/r/45959/#comment192668>

Use the same approach as in `+=`.



src/common/resources.cpp (line 1847)
<https://reviews.apache.org/r/45959/#comment192691>

Expose this to the header and friend it from the `Resource_` class so it 
can acess the private counter.



src/common/resources.cpp (lines 1850 - 1851)
<https://reviews.apache.org/r/45959/#comment192692>

The two ifs are equivalent. 

`Resource_` should have a `isShared()` method so the `<<` and other callers 
have one canonical way of doing this.

e.g.,
```
bool isShared()
{
      reutrn sharedCount.isSome();
}
```



src/common/resources.cpp (line 1852)
<https://reviews.apache.org/r/45959/#comment192699>

`()` is used by roles. Let's use `<>` here. I know that we are running out 
of bracket symbols soon but at least we are OK for now.



src/common/resources.cpp (lines 1853 - 1855)
<https://reviews.apache.org/r/45959/#comment192693>

Don't add anything special for nonshared resource since it's optional.


- Jiang Yan Xu


On April 11, 2016, 10 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45959/
> ---
> 
> (Updated April 11, 2016, 10 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4892
> https://issues.apache.org/jira/browse/MESOS-4892
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> A new struct Resoure_ is added to keep track of Resource and its
> consumer count. As a result, Resources maintains a single container
> for all resources. Private operators for addition and subtraction
> to/from Resource_ have been added.
> 
> All resources have consumer counts that is tracked within Resources. For
> resource addition and subtraction, the consumer counts are adjusted for
> shared resources as follows:
> a) Addition: If shared resource is absent from original, then the
>resource is added with a consumer count of 0. Otherwise, the consumer
>count for the shared resource is incremented by 1.
> b) Subtraction: If shared resource's consumer count is already 0, then
>the shared resource is removed from the orig

Re: Review Request 45958: Updated protobuf Resource to mark the resource as shareable.

[Jiang Yan Xu]

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




include/mesos/mesos.proto (line 762)
<https://reviews.apache.org/r/45958/#comment192708>

How about `optional SharedInfo shared`?

[shareable](http://www.merriam-webster.com/dictionary/shareable): capable 
of being shared

The following sentences read pretty clear to me.
- Some resource types in Mesos are shareable.
- Currently only persistent volumes are shareable. (This has nothing to do 
with whether `SHARE` operation has been applied, just that this type of 
resource can be made shared.)
- The `SHARE` operation makes a nonshared resource **shared**. 
- The `UNSHARE` operation makes **shared** resource nonshared.
- `SharedInfo` is currently empty but in the future we may add policies 
around how this resource should be **shared**.

Plus we can compare this with `shared_ptr` which is semantically very 
similar.

If we agree to this please also change the use of these words elsewhere 
appropriately.


- Jiang Yan Xu


On April 8, 2016, 4:16 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45958/
> ---
> 
> (Updated April 8, 2016, 4:16 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4892
> https://issues.apache.org/jira/browse/MESOS-4892
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added ShareInfo in Resource protobuf to allow for sharing of resources.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 63c181ae0a1e350fc27e36b1698e02db100b8861 
>   include/mesos/v1/mesos.proto a60a834e2538d54db7f257a0d4adfbb503ec1b0f 
> 
> Diff: https://reviews.apache.org/r/45958/diff/
> 
> 
> Testing
> ---
> 
> Tests successful.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 45958: Updated protobuf Resource to mark the resource as shareable.

[Jiang Yan Xu]

> On April 9, 2016, 12:08 a.m., Guangya Liu wrote:
> > include/mesos/mesos.proto, line 756
> > <https://reviews.apache.org/r/45958/diff/1/?file=1337790#file1337790line756>
> >
> > s/containers/tasks ? Tasks may be more clear here.

+1.

There are quite a number of details about how tasks and containers relate here 
w.r.t shared resources (e.g., even within a container multiple tasks can use 
the same shared resource) but here I think to avoid getting into too much 
details `tasks` is easier to understand and is certainly correct.


> On April 9, 2016, 12:08 a.m., Guangya Liu wrote:
> > include/mesos/mesos.proto, line 761
> > <https://reviews.apache.org/r/45958/diff/1/?file=1337790#file1337790line761>
> >
> > Can you please add a note here that this pareameter is only work for 
> > persiste volume for now?

+1.


- Jiang Yan


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


On April 8, 2016, 4:16 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45958/
> -------
> 
> (Updated April 8, 2016, 4:16 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4892
> https://issues.apache.org/jira/browse/MESOS-4892
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added ShareInfo in Resource protobuf to allow for sharing of resources.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 63c181ae0a1e350fc27e36b1698e02db100b8861 
>   include/mesos/v1/mesos.proto a60a834e2538d54db7f257a0d4adfbb503ec1b0f 
> 
> Diff: https://reviews.apache.org/r/45958/diff/
> 
> 
> Testing
> ---
> 
> Tests successful.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 46168: Add subdirectory support to URI.filename field.

[Jiang Yan Xu]

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




docs/fetcher.md (line 89)
<https://reviews.apache.org/r/46168/#comment192972>

I feel that "filename" is a bit odd when it can be a path. Many utilities 
simply call this a file, or output file to be more generic and flexible (when 
they do take a path).

e.g.,
```
wget --output-document file
url --output file
tar --file file
gcc -o outfile
clang -o output-file
```

I think `output_file` sounds good. (Notice the snake_casing in proto). What 
do you think?

We need to change docs and code (including CHANGELOG) elsewhere too but 
luckily the API is not released yet.

In CHANGELOG we can still group things under [MESOS-4735], just 
s/'filename'/'output_file'/.



src/launcher/fetcher.cpp (line 252)
<https://reviews.apache.org/r/46168/#comment192927>

`basename` usually specifically refers to the `the component following the 
final '/'`.

So here s/basename/outputFile/



src/launcher/fetcher.cpp (lines 253 - 268)
<https://reviews.apache.org/r/46168/#comment192969>

To avoid setting a Try with a "" when Try explicity doesn't have a default 
contructor, here we can keep the conditional operator but separately mkdir if 
needed.

```
// Prepare output directory if necessary.
if (uri.has_filename()) {
  string dirname = Path(uri.filename()).dirname();
  if (dirname != ".") {
Try result =
  os::mkdir(path::join(sandboxDirectory, uriDirname), true);

if (result.isError()) {
  return Error("Unable to create subdirectory in sandbox");
}
  }
}

Try outputFile = uri.has_filename()
  ? uri.filename()
  : Fetcher::basename(uri.value());
```



src/launcher/fetcher.cpp (line 255)
<https://reviews.apache.org/r/46168/#comment192926>

s/uriDirname/dirname/



src/launcher/fetcher.cpp (line 258)
<https://reviews.apache.org/r/46168/#comment192925>

2 space indentation here.



src/launcher/fetcher.cpp (line 261)
<https://reviews.apache.org/r/46168/#comment192975>

Here let's add the `dirname` to the error mesasge.



src/launcher/fetcher.cpp (line 308)
<https://reviews.apache.org/r/46168/#comment192977>

Same as above.

It sucks that a lot of code is duplicated here but the newly added code is 
only a part of it. Let's add a TODO to "refactor common logic in fetchFromCache 
and fetchBypassingCache into a helper".



src/slave/containerizer/fetcher.cpp (lines 150 - 153)
<https://reviews.apache.org/r/46168/#comment192923>

What does this check against?

Path(filename).basename() never returns an Error().



src/slave/containerizer/fetcher.cpp (lines 155 - 158)
<https://reviews.apache.org/r/46168/#comment192924>

`filename` is not guaranteed to be at least one character right?

Plus this doesn't prevent users from specifying something like 
`subdir/../../../../`. 

For the latter issue ideally there should be a method `Path::canonical` 
that resolves it but it's OK if we punt on it for now. Leave a TODO here for it 
here.

Also s/validateFilename/validateOutputFile/



src/tests/fetcher_tests.cpp (lines 105 - 106)
<https://reviews.apache.org/r/46168/#comment192978>

Nothing wrong with this but the source doesn't have to reside in a subdir. 
:)

We can remove this so there's no confusion.


- Jiang Yan Xu


On April 13, 2016, 5:06 p.m., Michael Browning wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46168/
> ---
> 
> (Updated April 13, 2016, 5:06 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Jiang Yan Xu, and Zhitao Li.
> 
> 
> Bugs: MESOS-5119
> https://issues.apache.org/jira/browse/MESOS-5119
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add subdirectory support to URI.filename field.
> 
> URI.filename allows the user to specify the name of the file that will
> be saved in the sandbox when the URI is fetched, but previously it would
> fail at fetch time if "filename" had a directory component. This change
> allows users to specify a relative path for custom filenames within the
> sandbox.
> 
> 
> Diffs
> -
> 
>   docs/fetcher.md fd6d8a78bd35c5644dceff7005dd7dfd9f5f2171 
>   src/launcher/fetcher.cpp 47583eeaed53b3e7ed4db26fee7cdd2fae5e0c9d 
>   src/sl

Review Request 35024: Fixed _resources_used() to include only *regular* resources.

[Jiang Yan Xu]

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

Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

- Otherwise the result `_resources_percent()` is wrong because the 
`_resources_total()` only includes *regular* resources (from 
slave->info.resources()).


Diffs
-

  src/master/master.cpp 710b8149c9d855d0f47cb2952366be10bc78c74d 

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


Testing
---

make check


Thanks,

Jiang Yan Xu

Review Request 35118: Made updateSlave() update its 'totalResources'.

[Jiang Yan Xu]

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

Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

- This way Master::Slave::totalResources includes revocable resources, which we 
need for metrics for revocable resources.
- Changed updateSlave() argument to use `const Resources& 
oversubscribedResources` instead of `const std::vector& 
oversubscribedResources` because `Resources` provides convenience methods such 
as `revocable()`.


Diffs
-

  src/master/master.hpp deeb0d8c87a13315206556e1d0974cdd13e8224f 
  src/master/master.cpp be0db42da3c59761aa154439653d715556465256 

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


Testing
---

make check.


Thanks,

Jiang Yan Xu

Review Request 35119: Introduced metrics for revocable resources.

[Jiang Yan Xu]

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

Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

state.json changes are in a subsequent review.


Diffs
-

  src/master/master.hpp deeb0d8c87a13315206556e1d0974cdd13e8224f 
  src/master/master.cpp be0db42da3c59761aa154439653d715556465256 
  src/master/metrics.hpp 833033c1912daee429b45423d24d365d8699a428 
  src/master/metrics.cpp 264252c5159990fdf7a4569933a305d07bd7dd6e 
  src/tests/oversubscription_tests.cpp afd7ff4f2b50cb20cc2c8865b655ad1f8eb0c8b7 

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


Testing
---

make check.

- Modified a test to test the `total` resources metrics.
- We don't have unit tests that use the revocable resources yet, when we add 
that we should check `used` resources metrics too.


Thanks,

Jiang Yan Xu

Re: Review Request 35119: Introduced metrics for revocable resources.

[Jiang Yan Xu]

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

(Updated June 7, 2015, 1:32 p.m.)


Review request for mesos and Vinod Kone.


Changes
---

Vinod's comments.


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


Repository: mesos


Description
---

state.json changes are in a subsequent review.


Diffs (updated)
-

  src/master/master.hpp deeb0d8c87a13315206556e1d0974cdd13e8224f 
  src/master/master.cpp d436f845341ca33a534127da3bad8d8b2aa1175b 
  src/master/metrics.hpp 833033c1912daee429b45423d24d365d8699a428 
  src/master/metrics.cpp 264252c5159990fdf7a4569933a305d07bd7dd6e 
  src/tests/oversubscription_tests.cpp afd7ff4f2b50cb20cc2c8865b655ad1f8eb0c8b7 

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


Testing
---

make check.

- Modified a test to test the `total` resources metrics.
- We don't have unit tests that use the revocable resources yet, when we add 
that we should check `used` resources metrics too.


Thanks,

Jiang Yan Xu

Re: Review Request 35118: Made updateSlave() update its 'totalResources'.

[Jiang Yan Xu]

> On June 5, 2015, 2:58 p.m., Vinod Kone wrote:
> > src/master/master.cpp, line 3517
> > <https://reviews.apache.org/r/35118/diff/1/?file=980131#file980131line3517>
> >
> > while you are at it, can you just send slave->totalResources here 
> > instead of oversubscribedResources? this will address my TODO on this 
> > method.

The allocator expects only oversubscribedResources and does a CHECK on it. The 
logic in Allocator::updateSlave(...) needs a little tweaking as well. I'd be 
happy to do it in a subsequent review.

SG?


- Jiang Yan


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


On June 5, 2015, 2:09 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35118/
> ---
> 
> (Updated June 5, 2015, 2:09 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-2776
> https://issues.apache.org/jira/browse/MESOS-2776
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - This way Master::Slave::totalResources includes revocable resources, which 
> we need for metrics for revocable resources.
> - Changed updateSlave() argument to use `const Resources& 
> oversubscribedResources` instead of `const std::vector& 
> oversubscribedResources` because `Resources` provides convenience methods 
> such as `revocable()`.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp deeb0d8c87a13315206556e1d0974cdd13e8224f 
>   src/master/master.cpp be0db42da3c59761aa154439653d715556465256 
> 
> Diff: https://reviews.apache.org/r/35118/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Review Request 35239: Update the JSON model for Resources to display their revocablility attribute.

[Jiang Yan Xu]

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

Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

See summary.


Diffs
-

  src/common/http.cpp 2ac7fba7a3aac913540f1b09768777393b79284a 
  src/master/http.cpp f8ac30934352db859e73819e0656a70047bb0dc5 
  src/tests/common/http_tests.cpp f087b2313a13c3199b70b3d7feb728e1449a52e7 

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


Testing
---

Added a test.

Also tested with a master/slave pair.

e.g., The following state.json output shows a slave using a fixed estimator 
with `cpus:1;mem:512` revocable resources.
```json
  "slaves": [
{
...
  "resources": {
"cpus": 24,
"disk": 454767,
"mem": 71322,
"ports": "[31000-32000]"
  },
  "total_resources": {
"cpus": 24,
"cpus{REV}": 1,
"disk": 454767,
"mem": 71322,
"mem{REV}": 512,
"ports": "[31000-32000]"
  },
  "used_resources": {
"cpus": 0,
"disk": 0,
    "mem": 0
  }
}
  ],
```

Note that `resources` only looks at the resources from SlaveInfo while 
`total_resources` reads Master::Slave::totalResources.


Thanks,

Jiang Yan Xu

Re: Review Request 35118: Made updateSlave() update its 'totalResources'.

[Jiang Yan Xu]

> On June 9, 2015, 10:36 a.m., Niklas Nielsen wrote:
> > src/master/master.cpp, line 3511
> > <https://reviews.apache.org/r/35118/diff/1/?file=980131#file980131line3511>
> >
> > Isn't this a bit harsh? Shouldn't we log a warning and return instead? 
> > Also, could this be done earlier on?

Harshness: I think it's fine a) because it's internal communication not 
involving the frameworks and b) it's a new message handler so backwards 
compatibility is covered.


Done earlier on: Agreed, will change this in a new review. (This one is already 
committed, sorry I haven't flagged it as such).


> On June 9, 2015, 10:36 a.m., Niklas Nielsen wrote:
> > src/master/master.cpp, lines 3513-3514
> > <https://reviews.apache.org/r/35118/diff/1/?file=980131#file980131line3513>
> >
> > Does it make sense to point to some documentation (if it exists 
> > already, or inline) about how this resource math will work?

Can you suggest something here?

FWIW I also updated the comment on `totalResources`:
```
  // The current total resources of the slave. Note that this is
  // different from 'info.resources()' because this also considers
  // operations (e.g., CREATE, RESERVE) that have been applied and
  // includes revocable resources as well.
  Resources totalResources;
```


- Jiang Yan


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


On June 5, 2015, 2:09 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35118/
> ---
> 
> (Updated June 5, 2015, 2:09 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-2776
> https://issues.apache.org/jira/browse/MESOS-2776
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - This way Master::Slave::totalResources includes revocable resources, which 
> we need for metrics for revocable resources.
> - Changed updateSlave() argument to use `const Resources& 
> oversubscribedResources` instead of `const std::vector& 
> oversubscribedResources` because `Resources` provides convenience methods 
> such as `revocable()`.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp deeb0d8c87a13315206556e1d0974cdd13e8224f 
>   src/master/master.cpp be0db42da3c59761aa154439653d715556465256 
> 
> Diff: https://reviews.apache.org/r/35118/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 35239: Update the JSON model for Resources to display their revocablility attribute.

[Jiang Yan Xu]

> On June 9, 2015, 11:36 a.m., Ben Mahler wrote:
> > Any reason not to just add a 'revocable_resources' instead of 
> > 'total_resources'? Looking to avoid the magic "REV" string proliferating, 
> > when we're not yet printing role here either.
> > 
> > Ideally, when we add 'total_resources', we can do it in a way that captures 
> > everything we want (role, disk info, revocable info, etc) in a parseable 
> > way. Otherwise, might become difficult to make further changes without 
> > breaking people. For example, if we add 'total_resources' as done here, but 
> > then we want to show the role, how do we do that without breaking people's 
> > code that exactly matches "cpus" ?

It would be easy to do so if this were for the 
`total_resources/revocable_resources/resources` alone but the fact is that all 
Resources models are affected by this.

Additionally the following metrics also contain recovable resources and are 
affected one way or another.

- Slave: used_resources, offered_resources
- Task: resources
- Framework: offered_resources

Since revocable resources and un-revocable resources are not addable, they will 
be shown either as
```
  "used_resources": {
"cpus": 24,
"cpus{REV}": 1,
"disk": 454767,
"mem": 71322,
"mem{REV}": 512,
"ports": "[31000-32000]"
  },
```
Or as the following if we don't add the {REV} attribute,

```
  "used_resources": {
"cpus": 24,
"cpus": 1,
"disk": 454767,
"mem": 71322,
"mem": 512,
"ports": "[31000-32000]"
  },
```
, which is more confusing.

To make sure existing metrics stay unchanged we would need to filter out 
revocable resources from them explicitly.

```
object.values["offered_resources"] = model(slave.offeredResources - 
slave.offeredResources.revocable());
```

And then add a revocable version for each of them. I don't think that's what we 
want here?


- Jiang Yan


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


On June 8, 2015, 5:38 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35239/
> ---
> 
> (Updated June 8, 2015, 5:38 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-2776
> https://issues.apache.org/jira/browse/MESOS-2776
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/common/http.cpp 2ac7fba7a3aac913540f1b09768777393b79284a 
>   src/master/http.cpp f8ac30934352db859e73819e0656a70047bb0dc5 
>   src/tests/common/http_tests.cpp f087b2313a13c3199b70b3d7feb728e1449a52e7 
> 
> Diff: https://reviews.apache.org/r/35239/diff/
> 
> 
> Testing
> ---
> 
> Added a test.
> 
> Also tested with a master/slave pair.
> 
> e.g., The following state.json output shows a slave using a fixed estimator 
> with `cpus:1;mem:512` revocable resources.
> ```json
>   "slaves": [
> {
> ...
>   "resources": {
> "cpus": 24,
> "disk": 454767,
> "mem": 71322,
>     "ports": "[31000-32000]"
>   },
>   "total_resources": {
> "cpus": 24,
> "cpus{REV}": 1,
> "disk": 454767,
> "mem": 71322,
> "mem{REV}": 512,
> "ports": "[31000-32000]"
>   },
>   "used_resources": {
> "cpus": 0,
> "disk": 0,
> "mem": 0
>   }
> }
>   ],
> ```
> 
> Note that `resources` only looks at the resources from SlaveInfo while 
> `total_resources` reads Master::Slave::totalResources.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 35119: Introduced metrics for revocable resources.

[Jiang Yan Xu]

> On June 9, 2015, 10:41 a.m., Niklas Nielsen wrote:
> > src/master/master.cpp, lines 5442-5444
> > <https://reviews.apache.org/r/35119/diff/2/?file=980495#file980495line5442>
> >
> > No need for the else block here.

True. I am dropping this because it's already committed. Will add you as a 
review for future oversubscription changes. I would stay away from an `else` 
here to avoid over-indentation which could constain the flow of (complicated) 
logic if being put inside the else block.

Here the code is clear either way and I like the symmetric look.


- Jiang Yan


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


On June 8, 2015, 12:55 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35119/
> ---
> 
> (Updated June 8, 2015, 12:55 p.m.)
> 
> 
> Review request for mesos, Niklas Nielsen and Vinod Kone.
> 
> 
> Bugs: MESOS-2776
> https://issues.apache.org/jira/browse/MESOS-2776
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> state.json changes are in a subsequent review.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp deeb0d8c87a13315206556e1d0974cdd13e8224f 
>   src/master/master.cpp d436f845341ca33a534127da3bad8d8b2aa1175b 
>   src/master/metrics.hpp 833033c1912daee429b45423d24d365d8699a428 
>   src/master/metrics.cpp 264252c5159990fdf7a4569933a305d07bd7dd6e 
>   src/tests/oversubscription_tests.cpp 
> afd7ff4f2b50cb20cc2c8865b655ad1f8eb0c8b7 
> 
> Diff: https://reviews.apache.org/r/35119/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> - Modified a test to test the `total` resources metrics.
> - We don't have unit tests that use the revocable resources yet, when we add 
> that we should check `used` resources metrics too.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 35239: Update the JSON model for Resources to display their revocablility attribute.

[Jiang Yan Xu]

> On June 9, 2015, 11:33 a.m., Vinod Kone wrote:
> > src/master/http.cpp, lines 226-227
> > <https://reviews.apache.org/r/35239/diff/1/?file=981171#file981171line226>
> >
> > why not just
> > 
> > object.values["resources"] = model(slave.totalResources);
> > 
> > because total resources represent the current state of the slave's 
> > resources.
> > 
> > also, this is more consistent with "used_resources" and 
> > "offered_resources" which will now have revocable resources in them.

I was aiming for consistency between the metrics and state.json. In metrics, 
existing metrics `(cpus|mem|disk)_total` stay unchanged and only included data 
from `slave.info.resources()`.

But in replying to Ben's comment below I began to think it's probably better to 
just do this so I'll just do `object.values["resources"] = 
model(slave.totalResources);`.


- Jiang Yan


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


On June 8, 2015, 5:38 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35239/
> ---
> 
> (Updated June 8, 2015, 5:38 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-2776
> https://issues.apache.org/jira/browse/MESOS-2776
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/common/http.cpp 2ac7fba7a3aac913540f1b09768777393b79284a 
>   src/master/http.cpp f8ac30934352db859e73819e0656a70047bb0dc5 
>   src/tests/common/http_tests.cpp f087b2313a13c3199b70b3d7feb728e1449a52e7 
> 
> Diff: https://reviews.apache.org/r/35239/diff/
> 
> 
> Testing
> ---
> 
> Added a test.
> 
> Also tested with a master/slave pair.
> 
> e.g., The following state.json output shows a slave using a fixed estimator 
> with `cpus:1;mem:512` revocable resources.
> ```json
>   "slaves": [
> {
> ...
>   "resources": {
> "cpus": 24,
> "disk": 454767,
> "mem": 71322,
> "ports": "[31000-32000]"
>   },
>   "total_resources": {
> "cpus": 24,
> "cpus{REV}": 1,
> "disk": 454767,
> "mem": 71322,
> "mem{REV}": 512,
> "ports": "[31000-32000]"
>   },
>   "used_resources": {
> "cpus": 0,
> "disk": 0,
> "mem": 0
>   }
> }
>   ],
> ```
> 
> Note that `resources` only looks at the resources from SlaveInfo while 
> `total_resources` reads Master::Slave::totalResources.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 35239: Update the JSON model for Resources to display their revocablility attribute.

[Jiang Yan Xu]

> On June 9, 2015, 11:36 a.m., Ben Mahler wrote:
> > Any reason not to just add a 'revocable_resources' instead of 
> > 'total_resources'? Looking to avoid the magic "REV" string proliferating, 
> > when we're not yet printing role here either.
> > 
> > Ideally, when we add 'total_resources', we can do it in a way that captures 
> > everything we want (role, disk info, revocable info, etc) in a parseable 
> > way. Otherwise, might become difficult to make further changes without 
> > breaking people. For example, if we add 'total_resources' as done here, but 
> > then we want to show the role, how do we do that without breaking people's 
> > code that exactly matches "cpus" ?
> 
> Jiang Yan Xu wrote:
> It would be easy to do so if this were for the 
> `total_resources/revocable_resources/resources` alone but the fact is that 
> all Resources models are affected by this.
> 
> Additionally the following metrics also contain recovable resources and 
> are affected one way or another.
> 
> - Slave: used_resources, offered_resources
> - Task: resources
> - Framework: offered_resources
> 
> Since revocable resources and un-revocable resources are not addable, 
> they will be shown either as
> ```
>   "used_resources": {
> "cpus": 24,
> "cpus{REV}": 1,
> "disk": 454767,
> "mem": 71322,
> "mem{REV}": 512,
> "ports": "[31000-32000]"
>   },
> ```
> Or as the following if we don't add the {REV} attribute,
> 
> ```
>   "used_resources": {
> "cpus": 24,
> "cpus": 1,
> "disk": 454767,
> "mem": 71322,
> "mem": 512,
> "ports": "[31000-32000]"
>   },
> ```
> , which is more confusing.
> 
> To make sure existing metrics stay unchanged we would need to filter out 
> revocable resources from them explicitly.
> 
> ```
> object.values["offered_resources"] = model(slave.offeredResources - 
> slave.offeredResources.revocable());
> ```
> 
> And then add a revocable version for each of them. I don't think that's 
> what we want here?

Another thing is that existing readings of `cpus`, `mem` and `disk` actually 
stay unchagned. So I think it's relatively safe. It only affects people who do 
`len(used_resources) == 3` which is unlikely. (Of course it's always a good 
idea to announce this explicitly).

It'll be involve a deprecation cycle to add the `role` attribute so I didn't 
add it here. However I agree that we should think about the path forward for 
modeling more extended resources attributes.


- Jiang Yan


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


On June 8, 2015, 5:38 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35239/
> ---
> 
> (Updated June 8, 2015, 5:38 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-2776
> https://issues.apache.org/jira/browse/MESOS-2776
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/common/http.cpp 2ac7fba7a3aac913540f1b09768777393b79284a 
>   src/master/http.cpp f8ac30934352db859e73819e0656a70047bb0dc5 
>   src/tests/common/http_tests.cpp f087b2313a13c3199b70b3d7feb728e1449a52e7 
> 
> Diff: https://reviews.apache.org/r/35239/diff/
> 
> 
> Testing
> ---
> 
> Added a test.
> 
> Also tested with a master/slave pair.
> 
> e.g., The following state.json output shows a slave using a fixed estimator 
> with `cpus:1;mem:512` revocable resources.
> ```json
>   "slaves": [
>     {
> ...
>   "resources": {
> "cpus": 24,
> "disk": 454767,
> "mem": 71322,
> "ports": "[31000-32000]"
>   },
>   "total_resources": {
> "cpus": 24,
> "cpus{REV}": 1,
> "disk": 454767,
> "mem": 71322,
> "mem{REV}": 512,
> "ports": "[31000-32000]"
>   },
>   "used_resources": {
> "cpus": 0,
> "disk": 0,
> "mem": 0
>   }
> }
>   ],
> ```
> 
> Note that `resources` only looks at the resources from SlaveInfo while 
> `total_resources` reads Master::Slave::totalResources.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 35118: Made updateSlave() update its 'totalResources'.

[Jiang Yan Xu]

> On June 9, 2015, 10:36 a.m., Niklas Nielsen wrote:
> > src/master/master.cpp, lines 3513-3514
> > <https://reviews.apache.org/r/35118/diff/1/?file=980131#file980131line3513>
> >
> > Does it make sense to point to some documentation (if it exists 
> > already, or inline) about how this resource math will work?
> 
> Jiang Yan Xu wrote:
> Can you suggest something here?
> 
> FWIW I also updated the comment on `totalResources`:
> ```
>   // The current total resources of the slave. Note that this is
>   // different from 'info.resources()' because this also considers
>   // operations (e.g., CREATE, RESERVE) that have been applied and
>   // includes revocable resources as well.
>   Resources totalResources;
> ```
> 
> Ben Mahler wrote:
> Given the check above, how about calling .revocable to make it symmetric:
> 
> ```
>   slave->totalResources -= slave->totalResources.revocable();
>   slave->totalResources += oversubscribedResources.revocable();
> ```
> 
> Should be a bit clearer to the reader that what we're doing here is 
> updating the revocable resources, without needing a big comment.

SGTM. Will do.


- Jiang Yan


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


On June 5, 2015, 2:09 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35118/
> ---
> 
> (Updated June 5, 2015, 2:09 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-2776
> https://issues.apache.org/jira/browse/MESOS-2776
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - This way Master::Slave::totalResources includes revocable resources, which 
> we need for metrics for revocable resources.
> - Changed updateSlave() argument to use `const Resources& 
> oversubscribedResources` instead of `const std::vector& 
> oversubscribedResources` because `Resources` provides convenience methods 
> such as `revocable()`.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp deeb0d8c87a13315206556e1d0974cdd13e8224f 
>   src/master/master.cpp be0db42da3c59761aa154439653d715556465256 
> 
> Diff: https://reviews.apache.org/r/35118/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 35312: Minor cleanup to master/slave metric logic.

[Jiang Yan Xu]

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

Ship it!


Ship It!

- Jiang Yan Xu


On June 10, 2015, 12:50 p.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35312/
> ---
> 
> (Updated June 10, 2015, 12:50 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> No functional changes, some cleanups along the way to r/35313
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp e3b7081e252b2b1d582e6c6c88a433217a31fa3e 
>   src/master/metrics.hpp 3d389e68f22e3d1e00bde0db4e25f897c79a2316 
>   src/master/metrics.cpp d2489c8897fb2f2f21f021ac5e7a2ada7997ea06 
>   src/slave/metrics.cpp 7a31ce7e32c1fd61256927cd37d84a646bf5dbda 
>   src/slave/slave.cpp 98036b2d5f2c765aef4a416c3cbc082df77ab3ac 
> 
> Diff: https://reviews.apache.org/r/35312/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 35313: Added slave metrics for revocable resources.

[Jiang Yan Xu]

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

Ship it!



src/slave/slave.cpp
<https://reviews.apache.org/r/35313/#comment139784>

Same as the one Vinod pointed out.



src/slave/slave.cpp
<https://reviews.apache.org/r/35313/#comment139785>

s/std::// 

Here and two below.


- Jiang Yan Xu


On June 10, 2015, 12:50 p.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35313/
> ---
> 
> (Updated June 10, 2015, 12:50 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-2775
> https://issues.apache.org/jira/browse/MESOS-2775
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Same approach as done in [r/35119/](https://reviews.apache.org/r/35119/) for 
> [MESOS-2776](https://issues.apache.org/jira/browse/MESOS-2776)].
> 
> Note that the existing used metric was not ignoring revocable resources.
> 
> 
> Diffs
> -
> 
>   src/slave/metrics.hpp 6af7f074d4e41225867e482241988bec3a9806e9 
>   src/slave/metrics.cpp 7a31ce7e32c1fd61256927cd37d84a646bf5dbda 
>   src/slave/slave.hpp 4d2c31688b19f101ec851c0d94e7d45aa2f8a76e 
>   src/slave/slave.cpp 98036b2d5f2c765aef4a416c3cbc082df77ab3ac 
> 
> Diff: https://reviews.apache.org/r/35313/diff/
> 
> 
> Testing
> ---
> 
> Manual testing, I will follow up to get this tested within vinod/jie's 
> integration tests.
> 
> Note that in the process of testing this, I realized that the command 
> executor leads to a mixing of revocable / non-revocable resources in the 
> slave. Will file a ticket.
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Review Request 35327: Fixed a bug that allows different resources of the same name to have different types.

[Jiang Yan Xu]

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

Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

See summary.


Diffs
-

  src/common/resources.cpp c168bd83d0e0f07fb8f3a70114dd854cad5f5140 
  src/tests/resources_tests.cpp b1e4483695eda998129db2c89a9dce044607c8b0 

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


Testing
---

make check.

Updated test `ResourcesTest.ParseError` to cover this case.


Thanks,

Jiang Yan Xu

Re: Review Request 35327: Fixed a bug that allows different resources of the same name to have different types.

[Jiang Yan Xu]

> On June 10, 2015, 3:56 p.m., Vinod Kone wrote:
> > src/common/resources.cpp, lines 383-388
> > <https://reviews.apache.org/r/35327/diff/1/?file=982274#file982274line383>
> >
> > No need for else if.
> > 
> > if () {
> > 
> > }
> > 
> > nameTypes[name] = resource.get().type();

Just that the if ... else ... structure logically separates the chunk of code 
that does the bookkeeping from the rest of the sequence of logic.

e.g. 
```cpp
if () {
}

nameTypes[name] = resource.get().type();

resources += resource.get();
```

I could to remove the blank line between the if block and the subsquence line 
like this:

```cpp
if () {
}
nameTypes[name] = resource.get().type();
```

But we generally don't do this, I think.


- Jiang Yan


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


On June 10, 2015, 3:34 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35327/
> ---
> 
> (Updated June 10, 2015, 3:34 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-2854
> https://issues.apache.org/jira/browse/MESOS-2854
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp c168bd83d0e0f07fb8f3a70114dd854cad5f5140 
>   src/tests/resources_tests.cpp b1e4483695eda998129db2c89a9dce044607c8b0 
> 
> Diff: https://reviews.apache.org/r/35327/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> Updated test `ResourcesTest.ParseError` to cover this case.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 35327: Fixed a bug that allows different resources of the same name to have different types.

[Jiang Yan Xu]

> On June 10, 2015, 3:56 p.m., Vinod Kone wrote:
> > src/common/resources.cpp, lines 383-388
> > <https://reviews.apache.org/r/35327/diff/1/?file=982274#file982274line383>
> >
> > No need for else if.
> > 
> > if () {
> > 
> > }
> >     
> > nameTypes[name] = resource.get().type();
> 
> Jiang Yan Xu wrote:
> Just that the if ... else ... structure logically separates the chunk of 
> code that does the bookkeeping from the rest of the sequence of logic.
> 
> e.g. 
> ```cpp
> if () {
> }
> 
> nameTypes[name] = resource.get().type();
> 
> resources += resource.get();
> ```
> 
> I could to remove the blank line between the if block and the subsquence 
> line like this:
> 
> ```cpp
> if () {
> }
> nameTypes[name] = resource.get().type();
> ```
> 
> But we generally don't do this, I think.

Oh I didn't even realize that it's an "else if" rather than "else". 

In this case `nameTypes[name] = resource.get().type();` is a (logical) noop if 
`nameTypes.contains(name)` but anyways, I think in this case leaving the `else` 
there is fine.


- Jiang Yan


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


On June 10, 2015, 3:34 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35327/
> ---
> 
> (Updated June 10, 2015, 3:34 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-2854
> https://issues.apache.org/jira/browse/MESOS-2854
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp c168bd83d0e0f07fb8f3a70114dd854cad5f5140 
>   src/tests/resources_tests.cpp b1e4483695eda998129db2c89a9dce044607c8b0 
> 
> Diff: https://reviews.apache.org/r/35327/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> Updated test `ResourcesTest.ParseError` to cover this case.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 35327: Fixed a bug that allows different resources of the same name to have different types.

[Jiang Yan Xu]

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

(Updated June 10, 2015, 4:14 p.m.)


Review request for mesos and Vinod Kone.


Changes
---

Vinod's comments. NNFR.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/common/resources.cpp c168bd83d0e0f07fb8f3a70114dd854cad5f5140 
  src/tests/resources_tests.cpp b1e4483695eda998129db2c89a9dce044607c8b0 

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


Testing
---

make check.

Updated test `ResourcesTest.ParseError` to cover this case.


Thanks,

Jiang Yan Xu

Re: Review Request 35309: Added Resources::get() and Resources::names().

[Jiang Yan Xu]

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

Ship it!


I can use the code in this review if we get it committed soon.

- Jiang Yan Xu


On June 10, 2015, 12:08 p.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35309/
> ---
> 
> (Updated June 10, 2015, 12:08 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Jie Yu.
> 
> 
> Bugs: MESOS-2753
> https://issues.apache.org/jira/browse/MESOS-2753
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Need this for the subsequent review.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 6feb1be5a1463ae1a2d932b2ff696f79c7d9afb5 
>   src/common/resources.cpp 01c79fbbf16bcbdc7a25953c49107863fce3e87f 
>   src/tests/resources_tests.cpp 4952e1b5409103087ab7d7ad91bf907515d3c567 
> 
> Diff: https://reviews.apache.org/r/35309/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Review Request 35333: Small fix in updateSlave() to make resource math clearer.

[Jiang Yan Xu]

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

Review request for mesos and Ben Mahler.


Repository: mesos


Description
---

A follow-up on BenM's suggestion in /r/35118/.


Diffs
-

  src/master/master.cpp 95ca2e5f6c903dea7528e559f86639e78ec92b9b 

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


Testing
---

make check.


Thanks,

Jiang Yan Xu

Review Request 35367: Changed Resourcs JSON model() to combine non-revocable resources and ignore revocable resources.

[Jiang Yan Xu]

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

Review request for mesos, Niklas Nielsen and Vinod Kone.


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


Repository: mesos


Description
---

"Combine" means taking the sum of scalars or coalescing ranges and sets of all 
resources of the same name regardless of their other attributes.


Diffs
-

  include/mesos/resources.hpp 7da6e6e8bc1fa1d62fc8be2b5169b68e10392b73 
  src/common/http.cpp 2ac7fba7a3aac913540f1b09768777393b79284a 
  src/common/resources.cpp 05969199889c6bc1421c8e5be10c74a50660719f 
  src/tests/common/http_tests.cpp f087b2313a13c3199b70b3d7feb728e1449a52e7 

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


Testing
---

make check.

Added a test `HTTP.ModelResources` for this.


Thanks,

Jiang Yan Xu

Re: Review Request 35367: Changed Resourcs JSON model() to combine non-revocable resources and ignore revocable resources.

[Jiang Yan Xu]

> On June 11, 2015, 4:36 p.m., Vinod Kone wrote:
> > src/tests/common/http_tests.cpp, line 127
> > <https://reviews.apache.org/r/35367/diff/1/?file=983104#file983104line127>
> >
> > camelcase?

Wrote too much Python lately.


- Jiang Yan


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


On June 11, 2015, 5:32 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35367/
> ---
> 
> (Updated June 11, 2015, 5:32 p.m.)
> 
> 
> Review request for mesos, Niklas Nielsen and Vinod Kone.
> 
> 
> Bugs: MESOS-2838
> https://issues.apache.org/jira/browse/MESOS-2838
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> "Combine" means taking the sum of scalars or coalescing ranges and sets of 
> all resources of the same name regardless of their other attributes.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 7da6e6e8bc1fa1d62fc8be2b5169b68e10392b73 
>   src/common/http.cpp 2ac7fba7a3aac913540f1b09768777393b79284a 
>   src/common/resources.cpp 05969199889c6bc1421c8e5be10c74a50660719f 
>   src/tests/common/http_tests.cpp f087b2313a13c3199b70b3d7feb728e1449a52e7 
>   src/tests/resources_tests.cpp ad12648a08ebc90da6bc9b9aa196fcef5ebe9f11 
> 
> Diff: https://reviews.apache.org/r/35367/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> Added a test `HTTP.ModelResources` and `ResourcesTest.Types` for this.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 35367: Changed Resourcs JSON model() to combine non-revocable resources and ignore revocable resources.

[Jiang Yan Xu]

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

(Updated June 11, 2015, 5:32 p.m.)


Review request for mesos, Niklas Nielsen and Vinod Kone.


Changes
---

Vinod's comments.


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


Repository: mesos


Description
---

"Combine" means taking the sum of scalars or coalescing ranges and sets of all 
resources of the same name regardless of their other attributes.


Diffs (updated)
-

  include/mesos/resources.hpp 7da6e6e8bc1fa1d62fc8be2b5169b68e10392b73 
  src/common/http.cpp 2ac7fba7a3aac913540f1b09768777393b79284a 
  src/common/resources.cpp 05969199889c6bc1421c8e5be10c74a50660719f 
  src/tests/common/http_tests.cpp f087b2313a13c3199b70b3d7feb728e1449a52e7 
  src/tests/resources_tests.cpp ad12648a08ebc90da6bc9b9aa196fcef5ebe9f11 

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


Testing (updated)
---

make check.

Added a test `HTTP.ModelResources` and `ResourcesTest.Types` for this.


Thanks,

Jiang Yan Xu

Re: Review Request 34135: Add filesystem/ isolators for persistent volumes.

[Jiang Yan Xu]

> On June 2, 2015, 2:45 p.m., Timothy Chen wrote:
> > src/slave/containerizer/isolators/filesystem/linux.cpp, line 95
> > 
> >
> > actually I'm wrong, I was reading the old style guide. The newest style 
> > guide we do put a space, ignore my earler comment.

?

Doesn't 
https://github.com/apache/mesos/blob/master/docs/mesos-c%2B%2B-style-guide.md 
say: "Don't put a space between the capture list and the parameter list"?


- Jiang Yan


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


On May 12, 2015, 5:47 p.m., Ian Downes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34135/
> ---
> 
> (Updated May 12, 2015, 5:47 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Moved code from Mesos Containerizer to filesystem isolators
>  - filesystem/posix (symlinks, doesn't support container rootfs)
>  - filesystem/linux (bind mounts, does support container rootfs)
> 
> The filesystem/posix isolator will be automatically included if no 
> filesystem/ isolator is specified.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 14bc976a7b6a656fb58085484d25c3de3cf0f693 
>   src/slave/containerizer/isolators/filesystem/linux.hpp PRE-CREATION 
>   src/slave/containerizer/isolators/filesystem/linux.cpp PRE-CREATION 
>   src/slave/containerizer/isolators/filesystem/posix.hpp PRE-CREATION 
>   src/slave/containerizer/isolators/filesystem/posix.cpp PRE-CREATION 
>   src/slave/containerizer/linux_launcher.cpp 
> b9e22e3c70bed0c29e2ca8632411789d33f779a8 
>   src/slave/containerizer/mesos/containerizer.cpp 
> b644b9c74bc23cf78c0a53284544be6cdaef2f8a 
> 
> Diff: https://reviews.apache.org/r/34135/diff/
> 
> 
> Testing
> ---
> 
> existing persistent volumes tests.
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 34136: Add ContainerImage protobuf.

[Jiang Yan Xu]

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



include/mesos/mesos.proto (lines 1212 - 1214)
<https://reviews.apache.org/r/34136/#comment142204>

Is it the intention that Image type is **defined** outside MesosInfo 
because DockerInfo can later reference it?

Otherwise it feels more natual to define Image within MesosInfo.


- Jiang Yan Xu


On June 22, 2015, 9:42 a.m., Ian Downes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34136/
> ---
> 
> (Updated June 22, 2015, 9:42 a.m.)
> 
> 
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add ContainerImage protobuf.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 8df1211165169c9595e0e6e85b5ddc404345ff70 
> 
> Diff: https://reviews.apache.org/r/34136/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Review Request 36005: Removed obsolete ec2 scripts.

[Jiang Yan Xu]

/deploy.centos64/root/mesos-ec2/start-mesos 
cc309cc44ecf5b67536f275083dca22948926f6b 
  ec2/deploy.centos64/root/mesos-ec2/stop-hypertable 
7280dc11bfc53ae84b7ecaba34c84810461ed7f4 
  ec2/deploy.centos64/root/mesos-ec2/stop-mesos 
9fdb8753dffc5115f94582753e0860538be6232b 
  ec2/deploy.centos64/root/mesos-ec2/zoo 
efc961b502f30a6f187f7e50c2b92b302203d89e 
  ec2/deploy.centos64/root/persistent-hdfs/conf/core-site.xml 
b23aef258a13e4bc4cbf27099f7aa68419ab0825 
  ec2/deploy.centos64/root/persistent-hdfs/conf/hadoop-env.sh 
854c61f1af772c84a54ef6ae78442dc4844e9e73 
  ec2/deploy.centos64/root/persistent-hdfs/conf/hdfs-site.xml 
86c9f78801218581d0d6f139b87522a1c4edaf45 
  ec2/deploy.centos64/root/persistent-hdfs/conf/mapred-site.xml 
b1637dc8ce9cb49ebaa684a182cc8c94ac7c1b7b 
  ec2/deploy.centos64/root/persistent-hdfs/conf/masters 
d26a1943aeb47ceeff904e593793e08c07eee9cc 
  ec2/deploy.centos64/root/persistent-hdfs/conf/slaves 
05f969e0c667629041099e97dd8d6a8168e211a1 
  ec2/deploy.generic/root/mesos-ec2/ec2-variables.sh 
1f76e61b2feffa4eadc53745caa5033dde182531 
  ec2/deploy.lucid64/root/hadoop-0.20.2/conf/core-site.xml 
0fc1402c1ae5f0813177ee1713573b9f10dcd325 
  ec2/deploy.lucid64/root/hadoop-0.20.2/conf/hadoop-env.sh 
bfd431e1d96806675fbf404a6217f76ae7abe6a4 
  ec2/deploy.lucid64/root/hadoop-0.20.2/conf/hdfs-site.xml 
46318c7c30c5cfe771e7882a9b4153d15fd70151 
  ec2/deploy.lucid64/root/hadoop-0.20.2/conf/masters 
d26a1943aeb47ceeff904e593793e08c07eee9cc 
  ec2/deploy.lucid64/root/hadoop-0.20.2/conf/slaves 
05f969e0c667629041099e97dd8d6a8168e211a1 
  ec2/deploy.lucid64/root/mesos-ec2/cluster-url 
fcf8b41aed4b47a7b78d0c5a0042678f40059a5f 
  ec2/deploy.lucid64/root/mesos-ec2/copy-dir 
02b6e642f6348b3b4cdc4ff5adb2b0070c1dc1cc 
  ec2/deploy.lucid64/root/mesos-ec2/hadoop-framework-conf/core-site.xml 
818ed1030d3dd97467c4d4c83735cb7bda5afa80 
  ec2/deploy.lucid64/root/mesos-ec2/hadoop-framework-conf/hadoop-env.sh 
811f4030ea5b1a8589b41374519ed9b0a99d756d 
  ec2/deploy.lucid64/root/mesos-ec2/hadoop-framework-conf/mapred-site.xml 
8d6240c19c52d83a530563b1ec9f01c3bd3187cc 
  ec2/deploy.lucid64/root/mesos-ec2/haproxy+apache/haproxy.config.template 
957c3f6a6b2a2f658e076337d88e69447b2a3341 
  ec2/deploy.lucid64/root/mesos-ec2/masters 
c531652dc1ef9e6d6ca73cec389b6433f35ae9cf 
  ec2/deploy.lucid64/root/mesos-ec2/mesos-daemon 
177265eb8d025c15ff68f973af347bb95cd0d1fe 
  ec2/deploy.lucid64/root/mesos-ec2/redeploy-mesos 
941d783d82f0708a6da0f4677c3364537dfded63 
  ec2/deploy.lucid64/root/mesos-ec2/setup 
cd11d65254fa03a066994537b7bfd0f16ccb2622 
  ec2/deploy.lucid64/root/mesos-ec2/setup-slave 
519219319d3e6cdaf88c63fc0937c1737e431b17 
  ec2/deploy.lucid64/root/mesos-ec2/setup-torque 
2ac8fd3546063d3ba391147383de53b7824c7c8c 
  ec2/deploy.lucid64/root/mesos-ec2/slaves 
05f969e0c667629041099e97dd8d6a8168e211a1 
  ec2/deploy.lucid64/root/mesos-ec2/ssh-no-keychecking 
3daf46fe988b4b7dac4879f4ea31b6ffca1dc02a 
  ec2/deploy.lucid64/root/mesos-ec2/start-mesos 
cc309cc44ecf5b67536f275083dca22948926f6b 
  ec2/deploy.lucid64/root/mesos-ec2/stop-mesos 
9fdb8753dffc5115f94582753e0860538be6232b 
  ec2/deploy.lucid64/root/mesos-ec2/zoo 
efc961b502f30a6f187f7e50c2b92b302203d89e 
  ec2/mesos-ec2 e3c0c14fe26d260c39f44d3779c41e9b32436fc0 
  ec2/mesos_ec2.py 15d85b52cc58c8efaf7a8b771b4d5616282cf3b7 

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


Testing
---

make check. The scripts are not referenced from elsewhere in the codebase and 
the doc ec2-scripts.md is not linked from the website.


Thanks,

Jiang Yan Xu

Re: Review Request 33090: Removed unmaintained frameworks code.

[Jiang Yan Xu]

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

(Updated June 29, 2015, 11:11 a.m.)


Review request for mesos and Vinod Kone.


Changes
---

Also removed docs/using-the-mesos-submit-tool.md, NNRF.


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


Repository: mesos


Description (updated)
---

See summary.


Diffs (updated)
-

  docs/using-the-mesos-submit-tool.md 2ba4acc7423dae7ff67c1bc64ba3982aa1318b53 
  frameworks/deploy_jar/README.txt 37ccc1733a0689ab0eea802b91e76d8d7eb71a82 
  frameworks/deploy_jar/daemon_executor.py 
4720a6e972308ef324f3f84642607072f0e69f3b 
  frameworks/deploy_jar/daemon_executor.sh 
ac54d73fbcd225463e47fbad8ca5c4ffac48ca93 
  frameworks/deploy_jar/daemon_framework 
2bb9107791ffe8fd0bd7a97667f6a2c85993e511 
  frameworks/deploy_jar/daemon_scheduler.py 
a919717a25b050b465202c1de7be6dfeb5720cfa 
  frameworks/deploy_jar/haproxy.config.template 
4d537e9abf29c34f9b709384a5f259c6108d05a2 
  frameworks/deploy_jar/hw.jar aa134596f5d4f8dd4c7a2b3c99b7ba9f18159965 
  frameworks/haproxy+apache/README.txt 37ccc1733a0689ab0eea802b91e76d8d7eb71a82 
  frameworks/haproxy+apache/haproxy+apache 
05b975989fbec97d13eb0a181cd9d85c8c7c564e 
  frameworks/haproxy+apache/haproxy+apache.py 
39d4e7ba681b6437e978fadbf58e92e7e32345f1 
  frameworks/haproxy+apache/haproxy.config.template 
ca82ba998d645dbfb3b057afa873d1379543ff43 
  frameworks/haproxy+apache/startapache.py 
dce0ddd0e5bd655f9d745e88eefab1603a6a5e62 
  frameworks/haproxy+apache/startapache.sh 
6eb24560130f5aa6eafb1ed6abecd8ab804e1e85 
  frameworks/mesos-submit/executor dd8b98c8a1971dd4b1a5504ba607f2d052837f22 
  frameworks/mesos-submit/executor.py 38813b9f9d151fa5293b710bc077075a5b1571cf 
  frameworks/mesos-submit/mesos-submit 8cd5a674ced6a0441eb692db3bbf67a0f2bb7dfd 
  frameworks/mesos-submit/mesos_submit.py 
94d6232da504a86aca558b3655b3dc881e6d981d 
  frameworks/torque/README.txt dd671e221dc448f89f593017fdcc81fae2ab9aa8 
  frameworks/torque/hpl-24node.qsub 3df1a45126e71c17fd642e90352eca733fbd0006 
  frameworks/torque/hpl-48node.qsub 105a7bbe4d4fdafd4984ec170790591a1a11160f 
  frameworks/torque/hpl-8node-small.qsub 
b14dc95a172bcb61d3c631968106689dcc8b5172 
  frameworks/torque/hpl-8node.qsub b14dc95a172bcb61d3c631968106689dcc8b5172 
  frameworks/torque/mesos-hpl/24node/HPL.dat 
e9ab3fdc2295419a84540b9f0bd03b09a1527758 
  frameworks/torque/mesos-hpl/48node/HPL.dat 
d5c3e85b171698d86c8f588502b6a3839d23d248 
  frameworks/torque/mesos-hpl/8node-small/HPL.dat 
941726c1d77534928d01cdbc5fc1a74c91cd635f 
  frameworks/torque/mesos-hpl/8node/HPL.dat 
250563c4d5a021119d89578110b32d4d031c3d04 
  frameworks/torque/mesos-hpl/Make.Linux_PII_CBLAS 
de646bab3bb26edbc086b245b407c190c6bbd77f 
  frameworks/torque/mesos-hpl/README 17b7b3be298f2e9d8802e9a64b792a83a66551da 
  frameworks/torque/mpi_example.c 3f3bcd08978aee7dc6803fabdbf171353a6728e8 
  frameworks/torque/start_pbs_mom.py ef8ed7cd8df8ff7d8c5e7720a823be6e967b121c 
  frameworks/torque/start_pbs_mom.sh e0c81d0899e1f7a01590fec11cbc61e299ebef57 
  frameworks/torque/test_date_sleep_date_1node.qsub 
99d91da292e950b72ca527fc6d94b924b1a57ea4 
  frameworks/torque/test_date_sleep_date_2node.qsub 
bd7ea0f6d737c5ec30f0930538e9484b691e0066 
  frameworks/torque/test_date_sleep_date_3node.qsub 
b8391da262ce2d5d01e63a4427b0614aeb59c2f3 
  frameworks/torque/test_date_sleep_date_5node_10sec.qsub 
c0e6d28e24b514d6b440f4b54b4db96ba55777a0 
  frameworks/torque/test_date_sleep_date_5node_60sec.qsub 
17cd352ebeac5bbf5a68ed2d06e99a032ded35e4 
  frameworks/torque/torquelib.py b69df1c528c7ce32103d23e16bee8276cf3bffaf 
  frameworks/torque/torquesched.py c22ef9180c3aaf447c0288ee4948d0c8067791b7 
  frameworks/torque/torquesched.sh a40c7eafd71a11c8f974aa49a612fa81d0016102 

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


Testing
---

make check


Thanks,

Jiang Yan Xu

Re: Review Request 36005: Removed obsolete ec2 scripts.

[Jiang Yan Xu]

x27;subdir-objects' is disabled
> > Makefile.am:33: warning: source file 'src/profiler.cpp' is in a 
> > subdirectory,
> > Makefile.am:33: but option 'subdir-objects' is disabled
> > Makefile.am:33: warning: source file 'src/process.cpp' is in a subdirectory,
> > Makefile.am:33: but option 'subdir-objects' is disabled
> > Makefile.am:33: warning: source file 'src/reap.cpp' is in a subdirectory,
> > Makefile.am:33: but option 'subdir-objects' is disabled
> > Makefile.am:33: warning: source file 'src/socket.cpp' is in a subdirectory,
> > Makefile.am:33: but option 'subdir-objects' is disabled
> > Makefile.am:33: warning: source file 'src/subprocess.cpp' is in a 
> > subdirectory,
> > Makefile.am:33: but option 'subdir-objects' is disabled
> > Makefile.am:33: warning: source file 'src/timeseries.cpp' is in a 
> > subdirectory,
> > Makefile.am:33: but option 'subdir-objects' is disabled
> > Makefile.am:67: warning: source file 'src/libevent_ssl_socket.cpp' is in a 
> > subdirectory,
> > Makefile.am:67: but option 'subdir-objects' is disabled
> > Makefile.am:67: warning: source file 'src/openssl.cpp' is in a subdirectory,
> > Makefile.am:67: but option 'subdir-objects' is disabled
> > Makefile.am:67: warning: source file 'src/openssl_util.cpp' is in a 
> > subdirectory,
> > Makefile.am:67: but option 'subdir-objects' is disabled
> > Makefile.am:85: warning: source file 'src/libevent.cpp' is in a 
> > subdirectory,
> > Makefile.am:85: but option 'subdir-objects' is disabled
> > Makefile.am:85: warning: source file 'src/libevent_poll.cpp' is in a 
> > subdirectory,
> > Makefile.am:85: but option 'subdir-objects' is disabled
> > Makefile.am:90: warning: source file 'src/libev.cpp' is in a subdirectory,
> > Makefile.am:90: but option 'subdir-objects' is disabled
> > Makefile.am:90: warning: source file 'src/libev_poll.cpp' is in a 
> > subdirectory,
> > Makefile.am:90: but option 'subdir-objects' is disabled
> > Makefile.am:169: warning: source file 'src/tests/benchmarks.cpp' is in a 
> > subdirectory,
> > Makefile.am:169: but option 'subdir-objects' is disabled
> > Makefile.am:161: warning: source file 'src/tests/ssl_client.cpp' is in a 
> > subdirectory,
> > Makefile.am:161: but option 'subdir-objects' is disabled
> > Makefile.am:125: warning: source file 'src/tests/decoder_tests.cpp' is in a 
> > subdirectory,
> > Makefile.am:125: but option 'subdir-objects' is disabled
> > Makefile.am:125: warning: source file 'src/tests/encoder_tests.cpp' is in a 
> > subdirectory,
> > Makefile.am:125: but option 'subdir-objects' is disabled
> > Makefile.am:125: warning: source file 'src/tests/http_tests.cpp' is in a 
> > subdirectory,
> > Makefile.am:125: but option 'subdir-objects' is disabled
> > Makefile.am:125: warning: source file 'src/tests/io_tests.cpp' is in a 
> > subdirectory,
> > Makefile.am:125: but option 'subdir-objects' is disabled
> > Makefile.am:125: warning: source file 'src/tests/limiter_tests.cpp' is in a 
> > subdirectory,
> > Makefile.am:125: but option 'subdir-objects' is disabled
> > Makefile.am:125: warning: source file 'src/tests/main.cpp' is in a 
> > subdirectory,
> > Makefile.am:125: but option 'subdir-objects' is disabled
> > Makefile.am:125: warning: source file 'src/tests/mutex_tests.cpp' is in a 
> > subdirectory,
> > Makefile.am:125: but option 'subdir-objects' is disabled
> > Makefile.am:125: warning: source file 'src/tests/metrics_tests.cpp' is in a 
> > subdirectory,
> > Makefile.am:125: but option 'subdir-objects' is disabled
> > Makefile.am:125: warning: source file 'src/tests/owned_tests.cpp' is in a 
> > subdirectory,
> > Makefile.am:125: but option 'subdir-objects' is disabled
> > Makefile.am:125: warning: source file 'src/tests/process_tests.cpp' is in a 
> > subdirectory,
> > Makefile.am:125: but option 'subdir-objects' is disabled
> > Makefile.am:125: warning: source file 'src/tests/queue_tests.cpp' is in a 
> > subdirectory,
> > Makefile.am:125: but option 'subdir-objects' is disabled
> > Makefile.am:125: warni

Re: Review Request 36005: Removed obsolete ec2 scripts.

[Jiang Yan Xu]

/root/mesos-ec2/slaves 
05f969e0c667629041099e97dd8d6a8168e211a1 
  ec2/deploy.centos64/root/mesos-ec2/ssh-no-keychecking 
3daf46fe988b4b7dac4879f4ea31b6ffca1dc02a 
  ec2/deploy.centos64/root/mesos-ec2/start-hypertable 
0e2593f66b34cd39730b05d5e84a000945510749 
  ec2/deploy.centos64/root/mesos-ec2/start-mesos 
cc309cc44ecf5b67536f275083dca22948926f6b 
  ec2/deploy.centos64/root/mesos-ec2/stop-hypertable 
7280dc11bfc53ae84b7ecaba34c84810461ed7f4 
  ec2/deploy.centos64/root/mesos-ec2/stop-mesos 
9fdb8753dffc5115f94582753e0860538be6232b 
  ec2/deploy.centos64/root/mesos-ec2/zoo 
efc961b502f30a6f187f7e50c2b92b302203d89e 
  ec2/deploy.centos64/root/persistent-hdfs/conf/core-site.xml 
b23aef258a13e4bc4cbf27099f7aa68419ab0825 
  ec2/deploy.centos64/root/persistent-hdfs/conf/hadoop-env.sh 
854c61f1af772c84a54ef6ae78442dc4844e9e73 
  ec2/deploy.centos64/root/persistent-hdfs/conf/hdfs-site.xml 
86c9f78801218581d0d6f139b87522a1c4edaf45 
  ec2/deploy.centos64/root/persistent-hdfs/conf/mapred-site.xml 
b1637dc8ce9cb49ebaa684a182cc8c94ac7c1b7b 
  ec2/deploy.centos64/root/persistent-hdfs/conf/masters 
d26a1943aeb47ceeff904e593793e08c07eee9cc 
  ec2/deploy.centos64/root/persistent-hdfs/conf/slaves 
05f969e0c667629041099e97dd8d6a8168e211a1 
  ec2/deploy.generic/root/mesos-ec2/ec2-variables.sh 
1f76e61b2feffa4eadc53745caa5033dde182531 
  ec2/deploy.lucid64/root/hadoop-0.20.2/conf/core-site.xml 
0fc1402c1ae5f0813177ee1713573b9f10dcd325 
  ec2/deploy.lucid64/root/hadoop-0.20.2/conf/hadoop-env.sh 
bfd431e1d96806675fbf404a6217f76ae7abe6a4 
  ec2/deploy.lucid64/root/hadoop-0.20.2/conf/hdfs-site.xml 
46318c7c30c5cfe771e7882a9b4153d15fd70151 
  ec2/deploy.lucid64/root/hadoop-0.20.2/conf/masters 
d26a1943aeb47ceeff904e593793e08c07eee9cc 
  ec2/deploy.lucid64/root/hadoop-0.20.2/conf/slaves 
05f969e0c667629041099e97dd8d6a8168e211a1 
  ec2/deploy.lucid64/root/mesos-ec2/cluster-url 
fcf8b41aed4b47a7b78d0c5a0042678f40059a5f 
  ec2/deploy.lucid64/root/mesos-ec2/copy-dir 
02b6e642f6348b3b4cdc4ff5adb2b0070c1dc1cc 
  ec2/deploy.lucid64/root/mesos-ec2/hadoop-framework-conf/core-site.xml 
818ed1030d3dd97467c4d4c83735cb7bda5afa80 
  ec2/deploy.lucid64/root/mesos-ec2/hadoop-framework-conf/hadoop-env.sh 
811f4030ea5b1a8589b41374519ed9b0a99d756d 
  ec2/deploy.lucid64/root/mesos-ec2/hadoop-framework-conf/mapred-site.xml 
8d6240c19c52d83a530563b1ec9f01c3bd3187cc 
  ec2/deploy.lucid64/root/mesos-ec2/haproxy+apache/haproxy.config.template 
957c3f6a6b2a2f658e076337d88e69447b2a3341 
  ec2/deploy.lucid64/root/mesos-ec2/masters 
c531652dc1ef9e6d6ca73cec389b6433f35ae9cf 
  ec2/deploy.lucid64/root/mesos-ec2/mesos-daemon 
177265eb8d025c15ff68f973af347bb95cd0d1fe 
  ec2/deploy.lucid64/root/mesos-ec2/redeploy-mesos 
941d783d82f0708a6da0f4677c3364537dfded63 
  ec2/deploy.lucid64/root/mesos-ec2/setup 
cd11d65254fa03a066994537b7bfd0f16ccb2622 
  ec2/deploy.lucid64/root/mesos-ec2/setup-slave 
519219319d3e6cdaf88c63fc0937c1737e431b17 
  ec2/deploy.lucid64/root/mesos-ec2/setup-torque 
2ac8fd3546063d3ba391147383de53b7824c7c8c 
  ec2/deploy.lucid64/root/mesos-ec2/slaves 
05f969e0c667629041099e97dd8d6a8168e211a1 
  ec2/deploy.lucid64/root/mesos-ec2/ssh-no-keychecking 
3daf46fe988b4b7dac4879f4ea31b6ffca1dc02a 
  ec2/deploy.lucid64/root/mesos-ec2/start-mesos 
cc309cc44ecf5b67536f275083dca22948926f6b 
  ec2/deploy.lucid64/root/mesos-ec2/stop-mesos 
9fdb8753dffc5115f94582753e0860538be6232b 
  ec2/deploy.lucid64/root/mesos-ec2/zoo 
efc961b502f30a6f187f7e50c2b92b302203d89e 
  ec2/mesos-ec2 e3c0c14fe26d260c39f44d3779c41e9b32436fc0 
  ec2/mesos_ec2.py 15d85b52cc58c8efaf7a8b771b4d5616282cf3b7 

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


Testing
---

make check. The scripts are not referenced from elsewhere in the codebase and 
the doc ec2-scripts.md is not linked from the website.


Thanks,

Jiang Yan Xu

Re: Review Request 34137: Add support for container image provisioners.

[Jiang Yan Xu]

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



src/slave/containerizer/mesos/containerizer.hpp (lines 319 - 320)
<https://reviews.apache.org/r/34137/#comment142551>

Why not store ContainerInfo directly?



src/slave/containerizer/mesos/containerizer.cpp (lines 642 - 646)
<https://reviews.apache.org/r/34137/#comment142560>

Indent two more spaces.



src/slave/containerizer/mesos/containerizer.cpp (lines 789 - 790)
<https://reviews.apache.org/r/34137/#comment142225>

```
map env = executorEnvironment(
executorInfo,
containers_[containerId]->rootfs.isSome()
  ? flags.sandbox_directory
  : directory,
slaveId,
slavePid,
checkpoint,
flags.recovery_timeout);
```
?

Just a comment, not an issue.



src/slave/containerizer/mesos/containerizer.cpp (lines 815 - 816)
<https://reviews.apache.org/r/34137/#comment142224>

Does

```
containers_[containerId]->rootfs.isSome()
  ? flags.sandbox_directory
  : directory;
```

look better?
Just thought this boarders "too much jaggedness" in the sytle guilde.



src/slave/containerizer/mesos/containerizer.cpp (lines 1254 - 1259)
<https://reviews.apache.org/r/34137/#comment142213>

Shift one more space to the right for alignment.



src/slave/containerizer/mesos/containerizer.cpp (lines 1278 - 1280)
<https://reviews.apache.org/r/34137/#comment142214>

Four space indentation.



src/slave/containerizer/mesos/containerizer.cpp (line 1279)
<https://reviews.apache.org/r/34137/#comment142223>

s/" :"/": "/



src/slave/containerizer/provisioner.cpp (lines 46 - 47)
<https://reviews.apache.org/r/34137/#comment142194>

Seems like this belongs to a later patch. AppcProvisioner is not introduced 
yet.



src/slave/paths.cpp (lines 243 - 247)
<https://reviews.apache.org/r/34137/#comment141883>

Indent two more spaces.



src/tests/containerizer_tests.cpp (line 344)
<https://reviews.apache.org/r/34137/#comment142215>

Kill empty line.


- Jiang Yan Xu


On June 22, 2015, 9:44 a.m., Ian Downes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34137/
> ---
> 
> (Updated June 22, 2015, 9:44 a.m.)
> 
> 
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The MesosContainerizer can optionally provision a root filesystem for the 
> containers.
> 
> 
> Diffs
> -
> 
>   include/mesos/slave/isolator.hpp 18edc030367e42240090f2f3dbc92aec7a4c6234 
>   src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 3ac2387eabded61c897a5016655aae10cd1bca91 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 8c102fb7d1f79ee768cb06de3a976ea12f958712 
>   src/slave/containerizer/provisioner.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner.cpp PRE-CREATION 
>   src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b 
>   src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 
>   src/slave/paths.hpp 00476f107ed8233123a6eab0925c9f28aac0a86f 
>   src/slave/paths.cpp 0741616b656e947cb460dd6ee6a9a4852be001c2 
>   src/slave/state.hpp fed4b7ecf9572a8dbb1a99dbb1769d3e55ef7bc5 
>   src/slave/state.cpp 8eda22a550e5add0f84c46cc2ed762b006c0dcec 
>   src/tests/containerizer_tests.cpp 0cdb2d2a3f19a4835e85c6b040759019b03f051e 
> 
> Diff: https://reviews.apache.org/r/34137/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Review Request 36023: Improved the documentation of Containerizer::launch() to clarify the failure cases.

[Jiang Yan Xu]

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

Review request for mesos and Ian Downes.


Repository: mesos


Description
---

Improved the documentation of Containerizer::launch() to clarify the failure 
cases.


Diffs
-

  src/slave/containerizer/containerizer.hpp 
0ee17e6bc52d1e3acefad6bda3a1b7ba64a8a54b 

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


Testing
---

N/A


Thanks,

Jiang Yan Xu

Re: Review Request 31444: Support chrooting in MesosContainerizer launch helper.

[Jiang Yan Xu]

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



src/slave/containerizer/mesos/launch.cpp (lines 64 - 65)
<https://reviews.apache.org/r/31444/#comment142621>

"must be relative to" is really "is interpreted as relative to" right?

Just wanted be sure clarify:
1) Should the user specify an absolute path with a preceding /?
2) The directory path as observed by processes outside the choot jail is 
`path::join(rootfs, directory)` right?



src/slave/containerizer/mesos/launch.cpp (lines 259 - 260)
<https://reviews.apache.org/r/31444/#comment142612>

"This must be an absolute path"

As in, if the flags specifies a path without a preceding slash this throws 
an error? 

    This is not enforced is it?


- Jiang Yan Xu


On June 22, 2015, 9:38 a.m., Ian Downes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31444/
> ---
> 
> (Updated June 22, 2015, 9:38 a.m.)
> 
> 
> Review request for mesos, Chi Zhang, Dominic Hamon, Jay Buffington, Jie Yu, 
> and James Peach.
> 
> 
> Bugs: MESOS-2350
> https://issues.apache.org/jira/browse/MESOS-2350
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Optionally take a path that the launch helper should chroot to before 
> exec'ing the executor. It is assumed that the work directory is mounted to 
> the appropriate location under the chroot. In particular, the path to the 
> executor must be relative to the chroot.
> 
> Configuration that should be private to the chroot is done during the launch, 
> e.g. mounting proc and statically configuring basic devices. It is assumed 
> that other configuration, e.g., preparing the image, mounting in volumes or 
> persistent resources, is done by the caller.
> 
> Mounts can be made to the chroot (e.g., updating the volumes or persistent 
> resources) and they will propagate in to the container but mounts made inside 
> the container will not propagate out to the host.
> 
> It currently assumes that at least {{chroot}}/tmp is writeable and that mount 
> points {{chroot}}/{tmp,dev,proc,sys} exist in the chroot.
> 
> This is specific to Linux.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 
>   src/linux/fs.cpp 568565f878b34708170a886dc4d62849aa01f263 
>   src/slave/containerizer/mesos/launch.hpp 
> 7c8b535746b5ce9add00afef86fdb6faefb5620e 
>   src/slave/containerizer/mesos/launch.cpp 
> 2f2d60e2011f60ec711d3b29fd2c157e30c83c34 
>   src/tests/launch_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/31444/diff/
> 
> 
> Testing
> ---
> 
> Manual testing only so far. This is harder to automate because we need a 
> self-contained chroot to execute something in... Suggestions welcome.
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Review Request 36269: Update CHANGELOG to reflect obsolete code cleanup

[Jiang Yan Xu]

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

Review request for mesos and Adam B.


Repository: mesos


Description
---

Per Vinod's comment on /r/36005

The `**Cleanup` section makes sense?


Diffs
-

  CHANGELOG 433924a0e614f061c448a8e85d0a2825567150dc 

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


Testing
---


Thanks,

Jiang Yan Xu

Re: Review Request 36269: Update CHANGELOG to reflect obsolete code cleanup

[Jiang Yan Xu]

> On July 7, 2015, 11:35 a.m., Adam B wrote:
> > CHANGELOG, line 337
> > <https://reviews.apache.org/r/36269/diff/1/?file=1001355#file1001355line337>
> >
> > Do you actually want/need MESOS-2640 to go into 0.23.0?
> > If so, MESOS-2640 should have its Target Version and Fix Version set to 
> > 0.23.0 and the release manager (me) should be notified. Since we've already 
> > cut rc1, we are only cherry-picking select patches into 0.23.0-rc2. Does 
> > this need to be one?
> > 
> > If not, create a new `(WIP) Release Notes - Mesos - Version 0.24.0` 
> > section at the top to place your Cleanup ticket under.

I see. ```(WIP) Release Notes - Mesos - Version 0.24.0``` sounds good.


- Jiang Yan


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


On July 7, 2015, 10:59 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36269/
> ---
> 
> (Updated July 7, 2015, 10:59 a.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Per Vinod's comment on /r/36005
> 
> The `**Cleanup` section makes sense?
> 
> 
> Diffs
> -
> 
>   CHANGELOG 433924a0e614f061c448a8e85d0a2825567150dc 
> 
> Diff: https://reviews.apache.org/r/36269/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 36269: Update CHANGELOG to reflect obsolete code cleanup

[Jiang Yan Xu]

> On July 7, 2015, 11:40 a.m., Vinod Kone wrote:
> > CHANGELOG, line 337
> > <https://reviews.apache.org/r/36269/diff/1/?file=1001355#file1001355line337>
> >
> > I would just put this under deprecations section.
> > 
> > Also, mind updating MESOS-2058 in deprecation section to do 
> > s/deprecate/remove/ because its been removed.

Adam is against putting MESOS-2640 in 0.23.0 because it's committed after rc1 
is cut. This review doesn't even need to land now.

s/deprecate/remove/ on MESOS-2058 can be done separately.


- Jiang Yan


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


On July 7, 2015, 10:59 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36269/
> ---
> 
> (Updated July 7, 2015, 10:59 a.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Per Vinod's comment on /r/36005
> 
> The `**Cleanup` section makes sense?
> 
> 
> Diffs
> -
> 
>   CHANGELOG 433924a0e614f061c448a8e85d0a2825567150dc 
> 
> Diff: https://reviews.apache.org/r/36269/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 34138: AppC hash computation.

[Jiang Yan Xu]

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


1. Agree that this is useful as a utility in libprocess. Not much overhead to 
move it over right?
2. It feels like something that could be exposed as a function rather than 
class, maybe a TODO.


src/slave/containerizer/provisioners/appc/hash.hpp (lines 22 - 31)
<https://reviews.apache.org/r/34138/#comment143922>

```
#include 
#include 

#include 

#include 

...
```

According to the style guide.



src/slave/containerizer/provisioners/appc/hash.hpp (lines 45 - 55)
<https://reviews.apache.org/r/34138/#comment143917>

I would do 

```
if (!system("sha512sum -h &> /dev/null")) {...}
```

to avoid fixing the binary location.



src/slave/containerizer/provisioners/appc/hash.hpp (lines 89 - 91)
<https://reviews.apache.org/r/34138/#comment143948>

I think we use 4 spaces to continue a left angle  bracket the same way we 
continue an left paren.



src/slave/containerizer/provisioners/appc/hash.hpp (line 97)
<https://reviews.apache.org/r/34138/#comment143939>

The `command` is not necessarily `sha512sum`. Maybe use it a static member 
so we detect once and use it with every hash() invocation?



src/slave/containerizer/provisioners/appc/hash.hpp (line 98)
<https://reviews.apache.org/r/34138/#comment143940>

Misaligned indentation.



src/slave/containerizer/provisioners/appc/hash.hpp (line 102)
<https://reviews.apache.org/r/34138/#comment143949>

The `command` is not necessarily `sha512sum`.



src/slave/containerizer/provisioners/appc/hash.hpp (line 109)
<https://reviews.apache.org/r/34138/#comment143941>

Misaligned indentation.



src/slave/containerizer/provisioners/appc/hash.hpp (lines 112 - 122)
<https://reviews.apache.org/r/34138/#comment143946>

This check doesn't work with openssl.

```
/usr/bin/openssl dgst -sha512 somefile.txt
SHA512(somefile.txt)= 
5a73e55fd845981be5d5b87039c678b87404405d5d054c579cf684a18893d181085b9afde535c034221f858d2bcc2b14978b4d5f4d6facfaa1f81e727a010f3c
```

I think we only need shasum and sha512sum to cover both Linux and OSX.


- Jiang Yan Xu


On July 7, 2015, 12:42 p.m., Ian Downes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34138/
> ---
> 
> (Updated July 7, 2015, 12:42 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> AppC hash computation.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 
>   src/slave/containerizer/provisioners/appc/hash.hpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/34138/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 34139: AppC image discovery.

[Jiang Yan Xu]

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



src/slave/containerizer/provisioners/appc/discovery.hpp (lines 43 - 46)
<https://reviews.apache.org/r/34139/#comment143985>

Some comments explaining that the return value is a URI string would be 
nice.



src/slave/containerizer/provisioners/appc/discovery.hpp (line 46)
<https://reviews.apache.org/r/34139/#comment143994>

The `id` is not use in this review and not specified in 
https://github.com/appc/spec/blob/master/spec/discovery.md
I assume its the sha512 but is it used during discovery?



src/slave/containerizer/provisioners/appc/discovery.hpp (line 79)
<https://reviews.apache.org/r/34139/#comment144003>

No need for the trailing `;`



src/slave/containerizer/provisioners/appc/discovery.cpp (line 21)
<https://reviews.apache.org/r/34139/#comment143999>

Kill empty line.



src/slave/containerizer/provisioners/appc/discovery.cpp (line 60)
<https://reviews.apache.org/r/34139/#comment143977>

canonicalize() not introduced until /r/34142/.
Is there anything else outside discovery that uses the method? Can it be 
moved to class `Discovery` (so we don't have to deal with cyclic review 
dependency)?



src/slave/containerizer/provisioners/appc/discovery.cpp (line 90)
<https://reviews.apache.org/r/34139/#comment143998>

FWIW https://github.com/appc/spec/blob/master/spec/discovery.md uses https 
exclusively.



src/slave/flags.cpp (line 63)
<https://reviews.apache.org/r/34139/#comment144000>

List all supported values?



src/slave/flags.cpp (line 68)
<https://reviews.apache.org/r/34139/#comment144001>

State that this is only for local discovery?

The sentences already mentions 'local images' but I think 
--appc_discovery=local is more explict in telling what the operator should do.


- Jiang Yan Xu


On July 7, 2015, 12:42 p.m., Ian Downes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34139/
> ---
> 
> (Updated July 7, 2015, 12:42 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Local and simple discovery only.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 
>   src/slave/containerizer/provisioners/appc/discovery.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/appc/discovery.cpp PRE-CREATION 
>   src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b 
>   src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 
> 
> Diff: https://reviews.apache.org/r/34139/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 34138: AppC hash computation.

[Jiang Yan Xu]

> On July 7, 2015, 3:56 p.m., Jiang Yan Xu wrote:
> > 1. Agree that this is useful as a utility in libprocess. Not much overhead 
> > to move it over right?
> > 2. It feels like something that could be exposed as a function rather than 
> > class, maybe a TODO.

OK I realized that doing the aforementioned refactorings is not as simple as 
moving the file so probably punting it is the right thing to do for now.

1. As a generic utility it's probably giong to be SHA instead SHA512.
2. OK SHA512::hash() is already static but I meant if made more generic like 
SHA(alorightm).hash() then shorthand functions like `sha1()`, `sha512()` is 
easier to use.


- Jiang Yan


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


On July 7, 2015, 12:42 p.m., Ian Downes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34138/
> ---
> 
> (Updated July 7, 2015, 12:42 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> AppC hash computation.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 
>   src/slave/containerizer/provisioners/appc/hash.hpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/34138/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 34140: AppC image store

[Jiang Yan Xu]

<https://reviews.apache.org/r/34140/#comment144199>

Why have "sha512-" as id prefix but not on the path? It would be nice to 
have them be consistent.

If id with sha512- prefix is clearer about the semantics of the id, perhaps 
using the full id (i.e. with the prefix) in the path also helps readability of 
the path.

Can we can have a common helper method to convert hash to id, instead of 
currently there are two places that prepend the "sha512-" prefix.



src/slave/containerizer/provisioners/appc/store.cpp (line 466)
<https://reviews.apache.org/r/34140/#comment144485>

No need for const.


- Jiang Yan Xu


On July 7, 2015, 12:43 p.m., Ian Downes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34140/
> ---
> 
> (Updated July 7, 2015, 12:43 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Images are fetched into the store (after discovery). Stored images are
> currently kept indefinitely.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 
>   src/slave/containerizer/provisioners/appc/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/appc/store.cpp PRE-CREATION 
>   src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b 
>   src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 
> 
> Diff: https://reviews.apache.org/r/34140/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 34141: AppC provsioning backend.

[Jiang Yan Xu]

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



src/slave/containerizer/provisioners/appc/backend.hpp (lines 25 - 28)
<https://reviews.apache.org/r/34141/#comment144904>

Alphabetic order.



src/slave/containerizer/provisioners/appc/backend.hpp (line 49)
<https://reviews.apache.org/r/34141/#comment144691>

s/image/images.



src/slave/containerizer/provisioners/appc/backend.hpp (lines 98 - 99)
<https://reviews.apache.org/r/34141/#comment144685>

Indentation.



src/slave/containerizer/provisioners/appc/backend.cpp (line 1)
<https://reviews.apache.org/r/34141/#comment144693>

Kill this line.



src/slave/containerizer/provisioners/appc/backend.cpp (lines 20 - 28)
<https://reviews.apache.org/r/34141/#comment144907>

Reorder includes.



src/slave/containerizer/provisioners/appc/backend.cpp (line 109)
<https://reviews.apache.org/r/34141/#comment144713>

Indentation.



src/slave/containerizer/provisioners/appc/backend.cpp (lines 119 - 120)
<https://reviews.apache.org/r/34141/#comment144722>

Indentation.



src/slave/containerizer/provisioners/appc/backend.cpp (line 130)
<https://reviews.apache.org/r/34141/#comment144923>

OSX [doesn't take this 
flag](https://developer.apple.com/library/mac/documentation/Darwin/Reference/ManPages/man1/cp.1.html).

How about `-a`?



src/slave/containerizer/provisioners/appc/backend.cpp (lines 147 - 156)
<https://reviews.apache.org/r/34141/#comment144924>

The style guilde requires:

```
// 2: OK (when no chaining, compare to 1).
instance.method([]() {
  ...;
});
```

i.e. 2 space indentation for the body and have the closing brace aligned 
with the dot in ".then".



src/slave/containerizer/provisioners/appc/backend.cpp (lines 179 - 188)
<https://reviews.apache.org/r/34141/#comment144925>

Ditto.


- Jiang Yan Xu


On July 7, 2015, 12:43 p.m., Ian Downes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34141/
> ---
> 
> (Updated July 7, 2015, 12:43 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Simple copy backend that forms the rootfs by copying each layer.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 
>   src/slave/containerizer/provisioners/appc/backend.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/appc/backend.cpp PRE-CREATION 
>   src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b 
>   src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 
> 
> Diff: https://reviews.apache.org/r/34141/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 29437: Bug fix: Start the executor registration timer, only when the container has launched successfully

[Jiang Yan Xu]

> On July 14, 2015, 10:21 a.m., Timothy Chen wrote:
> > Nishant are you still around to help rebase this? Sorry I think we dropped 
> > this review somehow.

He responded on https://issues.apache.org/jira/browse/MESOS-999. I did some 
work on it but ended up not pushing it. Would you like to comment on the ticket?


- Jiang Yan


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


On Jan. 7, 2015, 11:54 p.m., Nishant Suneja wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29437/
> ---
> 
> (Updated Jan. 7, 2015, 11:54 p.m.)
> 
> 
> Review request for mesos and Timothy Chen.
> 
> 
> Bugs: MESOS-999
> https://issues.apache.org/jira/browse/MESOS-999
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> As part of this bug fix, I have trigerred the executor registration timeout 
> timer after the container's future object is set, instead of starting the 
> timer when the container launch is still pending.
> 
> Also, a new executor launch timer has been added. This timer gates the time 
> in which a successful executor container launch should happen. The executor 
> registration timer starts after the successful container launch.
> 
> 
> Diffs
> -
> 
>   src/slave/constants.hpp fd1c1aba0aa62372ab399bee5709ce81b8e92cec 
>   src/slave/constants.cpp 2a99b1105af0e2d62b956fa96988f477937a46bd 
>   src/slave/flags.hpp 670997dc3a702cd5edf33f2e5824c5e4dfe4ecef 
>   src/slave/slave.hpp 70bd8c1fde4ea09fa54c76aa93424a1adb0309f6 
>   src/slave/slave.cpp 50b57819b55bdcdb9f49f20648199badc4d3f37b 
>   src/tests/composing_containerizer_tests.cpp 
> 5ab5a36cadb7f8622bad0c5814e9a5fb338753ad 
>   src/tests/containerizer.hpp 24b014f44d9eec56840e18cf39fbf9100f2c0711 
>   src/tests/slave_tests.cpp c50cbc799d4793243f184f9fe628b69a020adc66 
> 
> Diff: https://reviews.apache.org/r/29437/diff/
> 
> 
> Testing
> ---
> 
> Added the unit test : SlaveTest::ExecutorRegistrationTimeoutTrigger
> Added the unit test : SlaveTest:: ExecutorLaunchTimeoutTrigger
> Added the unit test : SlaveTest:: ExecutorNoLaunchTimeoutTrigger
> make check succeeds.
> 
> 
> Thanks,
> 
> Nishant Suneja
> 
>

Re: Review Request 34136: Add ContainerImage protobuf.

[Jiang Yan Xu]

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


Sorry I didn't notice it in my original review but I traced the issue back from 
future reviews.


include/mesos/mesos.proto (lines 1211 - 1213)
<https://reviews.apache.org/r/34136/#comment145213>

So I found the use of the field `id` inconsistent in the code.

Sometimes `id` has the `sha512-` prefix and sometimes not.

I think we should consistently refer to `id` using the definition in the 
[spec](https://github.com/appc/spec/blob/806b17c86ba5e5d595fca3f7ed339c8a22fb46c3/spec/aci.md#image-id),
 i.e., with the prefix.

The fact that the ID is computed by the image creator using sha512 and that 
the provisioner validates it using sha512 is merely an implementation detail 
that is not a conern of higher level abstractions / APIs.

So here I think in the comments we should not call it "Image hash" but 
rather refer to the spec for its full definition. We can of course call out the 
fact that it should have the "sha512-" perfix.
    
    What do you think?


- Jiang Yan Xu


On July 11, 2015, 9:47 p.m., Ian Downes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34136/
> ---
> 
> (Updated July 11, 2015, 9:47 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add ContainerImage protobuf.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 1763129da535561503e89cbd8c4a371f8553d8d6 
> 
> Diff: https://reviews.apache.org/r/34136/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 34136: Add ContainerImage protobuf.

[Jiang Yan Xu]

> On June 26, 2015, 2:57 p.m., Jiang Yan Xu wrote:
> > include/mesos/mesos.proto, lines 1212-1214
> > <https://reviews.apache.org/r/34136/diff/2/?file=989752#file989752line1212>
> >
> > Is it the intention that Image type is **defined** outside MesosInfo 
> > because DockerInfo can later reference it?
> > 
> > Otherwise it feels more natual to define Image within MesosInfo.
> 
> Vinod Kone wrote:
> +1 to put it inside MesosInfo.
> 
> Ian Downes wrote:
> I wanted it to be outside MesosInfo so it could be used by other 
> container types, i.e., I don't see it as specific to MesosInfo, e.g., we 
> could also support an AppcInfo which contained an Image::APPC.

OK SGTM.


- Jiang Yan


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


On July 11, 2015, 9:47 p.m., Ian Downes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34136/
> ---
> 
> (Updated July 11, 2015, 9:47 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add ContainerImage protobuf.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 1763129da535561503e89cbd8c4a371f8553d8d6 
> 
> Diff: https://reviews.apache.org/r/34136/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 34136: Add ContainerImage protobuf.

[Jiang Yan Xu]

> On July 14, 2015, 2:03 p.m., Jiang Yan Xu wrote:
> > include/mesos/mesos.proto, lines 1211-1213
> > <https://reviews.apache.org/r/34136/diff/3/?file=1009139#file1009139line1211>
> >
> > So I found the use of the field `id` inconsistent in the code.
> > 
> > Sometimes `id` has the `sha512-` prefix and sometimes not.
> > 
> > I think we should consistently refer to `id` using the definition in 
> > the 
> > [spec](https://github.com/appc/spec/blob/806b17c86ba5e5d595fca3f7ed339c8a22fb46c3/spec/aci.md#image-id),
> >  i.e., with the prefix.
> > 
> > The fact that the ID is computed by the image creator using sha512 and 
> > that the provisioner validates it using sha512 is merely an implementation 
> > detail that is not a conern of higher level abstractions / APIs.
> > 
> > So here I think in the comments we should not call it "Image hash" but 
> > rather refer to the spec for its full definition. We can of course call out 
> > the fact that it should have the "sha512-" perfix.
> > 
> > What do you think?
> 
> Timothy Chen wrote:
> Hi there,
> I'm going to commit this for Ian and just saw your comment.
> How about I reword the comment here to "// The ID of the Image. Please 
> refer to the Appc spec for its definition."?
> 
> Timothy Chen wrote:
> Actually I mis-read what you meant, how about:
> 
>  // The ID of the Image.
>   // An image ID is canonically represented as a string prefixed by
>   // the algorithm used and the hash output (e.g. sha512-a83...).

Or we could just copy the definition here verbatim. :)

https://github.com/appc/spec/blob/master/spec/types.md#image-id-type

An image ID is a string of the format "hash-value", where "hash" is the hash 
algorithm used and "value" is the hex encoded string of the digest. Currently 
the only permitted hash algorithm is sha512.

Thanks, please commit it!


- Jiang Yan


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


On July 11, 2015, 9:47 p.m., Ian Downes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34136/
> ---
> 
> (Updated July 11, 2015, 9:47 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add ContainerImage protobuf.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 1763129da535561503e89cbd8c4a371f8553d8d6 
> 
> Diff: https://reviews.apache.org/r/34136/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 34142: AppC provisioner.

[Jiang Yan Xu]

;

Misaligned.



src/slave/containerizer/provisioners/appc.cpp (line 423)
<https://reviews.apache.org/r/34142/#comment145463>

fetchAll() already recursively traverses the dependency graph right?



src/slave/containerizer/provisioners/appc.cpp (line 424)
<https://reviews.apache.org/r/34142/#comment145797>

There is possibility of poentially downloading identical images if done in 
parallel right? Also we need to maintain the image provisioning order.



src/slave/containerizer/provisioners/appc.cpp (line 446)
<https://reviews.apache.org/r/34142/#comment145775>

Be more explicit with `Option::none()`?


- Jiang Yan Xu


On July 7, 2015, 12:43 p.m., Ian Downes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34142/
> ---
> 
> (Updated July 7, 2015, 12:43 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Discovers image(s), fetches to the image store, then provisions using
> a backend.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 8df1211165169c9595e0e6e85b5ddc404345ff70 
>   src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 8c102fb7d1f79ee768cb06de3a976ea12f958712 
>   src/slave/containerizer/provisioners/appc.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/appc.cpp PRE-CREATION 
>   src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b 
>   src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 
> 
> Diff: https://reviews.apache.org/r/34142/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 34140: AppC image store

[Jiang Yan Xu]

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


Not necessary to be done in this review but we should allow `putting` images 
that are already in the staging folder, this will support mechanisms that 
download stuff out-of-band.
Also we should rely on a `paths.hpp` when doing all the path manipulation.


src/slave/containerizer/provisioners/appc/store.hpp (line 50)
<https://reviews.apache.org/r/34140/#comment145928>

It's worth noting that the id is computed from the sha512 hash here as this 
is actually the only place where the mechanism by which that this id is derived 
matters. Everywhere else it should be understood as an opaque string.



src/slave/containerizer/provisioners/appc/store.hpp (lines 80 - 83)
<https://reviews.apache.org/r/34140/#comment145766>

Who uses this?



src/slave/containerizer/provisioners/appc/store.cpp (line 148)
<https://reviews.apache.org/r/34140/#comment146052>

Here if the `stage` directory is moved into the store then `rmdir()` 
returns Error. Of course we don't check the result but maybe checking its 
existence before rmdir and log other rmdir Errors is better?



src/slave/containerizer/provisioners/appc/store.cpp (line 397)
<https://reviews.apache.org/r/34140/#comment145936>

So at this point we have already downloaded the image, we might as well 
just go ahead and overwrite the existing folder. If not, we probably don't want 
to spend all the resources to download it in the first place (i.e. directly 
return in `put()`).

But I think it would make sense to overwrite if we are going to open up 
some HTTP API to allow operators to resue some faulty images.



src/slave/containerizer/provisioners/appc/store.cpp (lines 406 - 415)
<https://reviews.apache.org/r/34140/#comment145933>

Manifest validation is only one type of validation.
We should also do other validation checks. e.g. whether rootfs exists; 
whether there are additional files outside of rootfs, etc.


- Jiang Yan Xu


On July 7, 2015, 12:43 p.m., Ian Downes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34140/
> ---
> 
> (Updated July 7, 2015, 12:43 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Images are fetched into the store (after discovery). Stored images are
> currently kept indefinitely.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 
>   src/slave/containerizer/provisioners/appc/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/appc/store.cpp PRE-CREATION 
>   src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b 
>   src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 
> 
> Diff: https://reviews.apache.org/r/34140/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 34142: AppC provisioner.

[Jiang Yan Xu]

> On July 2, 2015, 1:48 a.m., Timothy Chen wrote:
> > include/mesos/mesos.proto, line 1300
> > 
> >
> > I believe we discussed this, but different acVersion will most likely 
> > have different schema.
> > 
> > Unless we intend to only support one version (or anything that's 
> > backward compatible), we should version the protobuf too.
> 
> Timothy Chen wrote:
> Actually at this point seems like AppC image spec most likely won't have 
> a new version with OCP, so perhaps we don't have to. I'll let you decide :)

We should at least say in the comment what acVersion this proto definiton is 
based on.


- Jiang Yan


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


On July 7, 2015, 12:43 p.m., Ian Downes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34142/
> ---
> 
> (Updated July 7, 2015, 12:43 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Discovers image(s), fetches to the image store, then provisions using
> a backend.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 8df1211165169c9595e0e6e85b5ddc404345ff70 
>   src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 8c102fb7d1f79ee768cb06de3a976ea12f958712 
>   src/slave/containerizer/provisioners/appc.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/appc.cpp PRE-CREATION 
>   src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b 
>   src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 
> 
> Diff: https://reviews.apache.org/r/34142/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 34137: Add support for container image provisioners.

[Jiang Yan Xu]

> On July 16, 2015, 6:36 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, line 630
> > 
> >
> > Hum, looks like a bug since, for example, slaveId is a reference and 
> > will be invalid when the lambda is called. In general, I think we should 
> > avoid using [=] for lambdas because its dangeous!
> > 
> > I would prefer we resort to our old style defer style (e.g., introduce 
> > `_provision`).

[=] captures slaveId by value (copy) so it won't be invalid?

But after when to use lambdas, I think this is a good point and we should 
establish some best practices. 
Google style guide has these guidelines: 
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Lambda_expressions
 

Avoiding default captures is one of them; limiting the length of lambdas is 
another.

We should document these, at least reference Google's.

And how about lambdas that simply call another method vs. bind? :)


- Jiang Yan


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


On July 11, 2015, 9:47 p.m., Ian Downes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34137/
> ---
> 
> (Updated July 11, 2015, 9:47 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The MesosContainerizer can optionally provision a root filesystem for the 
> containers.
> 
> 
> Diffs
> -
> 
>   include/mesos/slave/isolator.hpp ef2205dd7f4619e53e6cca7cac301f874d08c036 
>   src/Makefile.am e5b5d36f0ac160e5a3a9fdc50b31c060a413ce2c 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 3ac2387eabded61c897a5016655aae10cd1bca91 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 47d146125dfd4ea909e7ec9d94f41cfa11d035e5 
>   src/slave/containerizer/provisioner.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner.cpp PRE-CREATION 
>   src/slave/flags.hpp 26c778db2303167369af8675fe0441a00a1e9151 
>   src/slave/flags.cpp 8632677ebbdbfef8ffa45204b6f63a700baff7f3 
>   src/slave/paths.hpp 00476f107ed8233123a6eab0925c9f28aac0a86f 
>   src/slave/paths.cpp 0741616b656e947cb460dd6ee6a9a4852be001c2 
>   src/slave/state.hpp bb0eee78e118210ccd7fcbd909029412ff106128 
>   src/slave/state.cpp f8a9514f52bf9f886171c2a0e674e5a89f8dbea7 
>   src/tests/containerizer_tests.cpp 0cdb2d2a3f19a4835e85c6b040759019b03f051e 
> 
> Diff: https://reviews.apache.org/r/34137/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 34141: AppC provsioning backend.

[Jiang Yan Xu]

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



src/slave/containerizer/provisioners/appc/backend.hpp (line 41)
<https://reviews.apache.org/r/34141/#comment145761>

In terms of the name `Backend`, I am not sure if it captures what it does 
precisely.

There is Provisioner::provision() and there is Backend::provision().

If "provision" means the overarching action then this specific step is 
really "installing" the downloaded images.

Thoughts?



src/slave/containerizer/provisioners/appc/backend.cpp (lines 128 - 133)
<https://reviews.apache.org/r/34141/#comment145250>

For this to work, should we check the pre-condition: `directory` has to 
already exist,  otherwise `rootfs` is copied to `directory` rather than 
`directory/rootfs`.

I assume this is what we want given the bind mount backend implemetation:

A simple illustration of directory layout (in `paths.hpp`) is hugely 
helpful.


- Jiang Yan Xu


On July 7, 2015, 12:43 p.m., Ian Downes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34141/
> ---
> 
> (Updated July 7, 2015, 12:43 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Simple copy backend that forms the rootfs by copying each layer.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 
>   src/slave/containerizer/provisioners/appc/backend.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/appc/backend.cpp PRE-CREATION 
>   src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b 
>   src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 
> 
> Diff: https://reviews.apache.org/r/34141/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 34142: AppC provisioner.

[Jiang Yan Xu]

> On July 1, 2015, 5:40 p.m., Lily Chen wrote:
> > src/slave/containerizer/provisioners/appc.cpp, lines 143-151
> > 
> >
> > What if the candidate is over-specified (has more labels)? Should this 
> > still be a match?

According to the spec it should be a match.


- Jiang Yan


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


On July 7, 2015, 12:43 p.m., Ian Downes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34142/
> ---
> 
> (Updated July 7, 2015, 12:43 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Discovers image(s), fetches to the image store, then provisions using
> a backend.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 8df1211165169c9595e0e6e85b5ddc404345ff70 
>   src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 8c102fb7d1f79ee768cb06de3a976ea12f958712 
>   src/slave/containerizer/provisioners/appc.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/appc.cpp PRE-CREATION 
>   src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b 
>   src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 
> 
> Diff: https://reviews.apache.org/r/34142/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 34140: AppC image store

[Jiang Yan Xu]

> On May 27, 2015, 4:10 p.m., Paul Brett wrote:
> > src/slave/containerizer/provisioners/appc/store.cpp, line 267
> > 
> >
> > Why not do the decompress, hash & untar as a pipeline to reduce disk 
> > usage?
> 
> Ian Downes wrote:
> Because we need to hash the (decrypted (uncompressed)) tarball, i.e., an 
> intermediate object, and because we don't know how it is compressed.

It may take more effort but I think it can be done so maybe this is what we 
should eventually do?

1) detecting the encryption and compression type up front.
2) `tee` the decompression output to do the hash and write it to a file.

For bigger images I do think the saves both temporary disk space as well as 
reduce latency (less disk writes).

Thoughts?


- Jiang Yan


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


On July 7, 2015, 12:43 p.m., Ian Downes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34140/
> ---
> 
> (Updated July 7, 2015, 12:43 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Images are fetched into the store (after discovery). Stored images are
> currently kept indefinitely.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 
>   src/slave/containerizer/provisioners/appc/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/appc/store.cpp PRE-CREATION 
>   src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b 
>   src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 
> 
> Diff: https://reviews.apache.org/r/34140/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 34136: Add ContainerImage protobuf.

[Jiang Yan Xu]

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



include/mesos/mesos.proto (line 1213)
<https://reviews.apache.org/r/34136/#comment147861>

Hmm... I don't think this should be required. It's too inflexible and tasks 
likely will use name and labels.


- Jiang Yan Xu


On July 11, 2015, 9:47 p.m., Ian Downes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34136/
> ---
> 
> (Updated July 11, 2015, 9:47 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add ContainerImage protobuf.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 1763129da535561503e89cbd8c4a371f8553d8d6 
> 
> Diff: https://reviews.apache.org/r/34136/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 36929: Fixed a few issues in test launcher header.

[Jiang Yan Xu]

> On July 30, 2015, 9:45 a.m., Vinod Kone wrote:
> > src/tests/containerizer/launcher.hpp, lines 19-37
> > 
> >
> > why did you remove these headers?
> > 
> > i think we decided to explicitly include all the headers that are used 
> > in a file instead of depending on transitive includes.
> 
> Jie Yu wrote:
> Is there a discussion somewhere? I think explicitly including all headers 
> make it hard to maintain (you'll need to adjust the header when the dependent 
> header changes). Also, it slows down the compilation.
> 
> Vinod Kone wrote:
> This is the pattern we followed in the code base. I think it improves 
> readability and avoids gotchas. For example, it is not clear at all that the 
> headers you kept provide a string or a vector definition (What if those 
> headers don't "#include " at a later time?). This is the relevant 
> blurb from the google style guide
> 
> ```
> You should include all the headers that define the symbols you rely upon 
> (except in cases of forward declaration). If you rely on symbols from bar.h, 
> don't count on the fact that you included foo.h which (currently) includes 
> bar.h: include bar.h yourself, unless foo.h explicitly demonstrates its 
> intent to provide you the symbols of bar.h. However, any includes present in 
> the related header do not need to be included again in the related cc (i.e., 
> foo.cc can rely on foo.h's includes).
> 
> ```
> 
> Some relevant discussion: 
> http://www.mail-archive.com/dev%40mesos.apache.org/msg32694.html
> 
> Jie Yu wrote:
> I have to disagree. C++ is not like Java, we don't have an automated tool 
> to help you include all necessary headers (java does). As a result, that 
> makes maintaining the includes very tedious. Any time someone is doing a 
> refactor, he/she has to manually check all the includes to make sure it's 
> up-to-date. Unless we have an automated tool to help you include all 
> necessary headers, I think having less includes makes it easy to maintain.
> 
> The pattern I suggest here is:
> ```
> base.hpp
> 
> #include 
> 
> class Base {
>   virtual void foo(const string& str);
> };
> 
> derived.hpp
> 
> #include "base.hpp"
> 
> class Derived : public Base {
>   virtual void foo(const string& str);
> };
> ```
> 
> We suggest don't include `` in derived.hpp. This does not violate 
> google style as it states: "unless foo.h explicitly demonstrates its intent 
> to provide you the symbols of bar.h". I think here, base.hpp clearly 
> demonstrates its intent to provide you the symbols for all its public 
> interfaces.
> 
> Vinod Kone wrote:
> It's fine if we want do it that way, but lets do that after we have a 
> discussion on the dev list. It would be great to see if we can get some build 
> improvement numbers.
> 
> For now though I prefer we keep the codebase consistent. OK with you?

FWIW here is what include-what-you-use interprets what consititutes an 
"intent": https://code.google.com/p/include-what-you-use/wiki/WhatIsAUse, which 
is in line with Jie's interpretation: if the dependency uses something on the 
"interface" it's considered exporting the symbols whereas using it in its 
implementation is not.

+1 for seeking agreement from dev@.


- Jiang Yan


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


On July 29, 2015, 5:14 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36929/
> ---
> 
> (Updated July 29, 2015, 5:14 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed a few issues in test launcher header.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/launcher.hpp 
> b80e84196f8b494e6e5a3c96c86f63fe432a7bb0 
> 
> Diff: https://reviews.apache.org/r/36929/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 36954: Performed a self bind mount of rootfs itself in fs::chroot::enter().

[Jiang Yan Xu]

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

Ship it!



src/linux/fs.cpp (lines 515 - 516)
<https://reviews.apache.org/r/36954/#comment148114>

The exact words on the [man page](http://linux.die.net/man/2/pivot_root):

> new_root and put_old must not be on the same file system as the current 
root

So here the comment should be 

`... requires 'root' to be not on the same filesystem as the process' 
current root`.


- Jiang Yan Xu


On July 30, 2015, 3:09 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36954/
> ---
> 
> (Updated July 30, 2015, 3:09 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Timothy Chen, Vinod Kone, and Jiang Yan 
> Xu.
> 
> 
> Bugs: MESOS-3178
> https://issues.apache.org/jira/browse/MESOS-3178
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Performed a self bind mount of rootfs itself in fs::chroot::enter().
> 
> See the attached ticket for the motivation.
> 
> 
> Diffs
> -
> 
>   src/linux/fs.cpp ea0891e320154b85a21ed2d138c192821efae9cd 
> 
> Diff: https://reviews.apache.org/r/36954/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 34427: AppC provisioner backend using bind mounts.

[Jiang Yan Xu]

> On May 21, 2015, 12:29 p.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioners/appc/bind_backend.hpp, line 70
> > 
> >
> > Should we make "rootfs" a constant somewhere?

Yeah, I think there should be a paths.hpp utility to do all path-related work.


- Jiang Yan


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


On June 22, 2015, 9:53 a.m., Ian Downes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34427/
> ---
> 
> (Updated June 22, 2015, 9:53 a.m.)
> 
> 
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Note: This is a specialized backend; see notes in bind_backend.hpp.
>  
> Reproduced here for your convenience:
> 
> This is a specialized backend that may be useful for deployments
> using large (multi-GB) single-layer images *and* where more
> recent kernel features such as overlayfs are not available. For small
> images (10's to 100's of MB) the Copy backend may be sufficient.
> 1) It supports only a single layer. Multi-layer images will fail
>to provision and the container will fail to launch!
> 2) The filesystem is read-only because all containers using this
>image share the source. Select writable areas can be achieved
>by
>mounting read-write volumes to places like /tmp, /var/tmp,
>/home, etc. using the ContainerInfo. These can be relative to
>the executor work directory.
> 3) It relies on the image persisting in the store.
> 4) It's fast because the bind mount requires (nearly) zero IO.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 
>   src/slave/containerizer/provisioners/appc/backend.cpp PRE-CREATION 
>   src/slave/containerizer/provisioners/appc/bind_backend.hpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/34427/diff/
> 
> 
> Testing
> ---
> 
> manual testing of a single layer image with RW relative bind mount for /tmp.
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 36956: Created a test abstraction for preparing test rootfs.

[Jiang Yan Xu]

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

Ship it!



src/tests/containerizer/rootfs.hpp (line 66)
<https://reviews.apache.org/r/36956/#comment148358>

Just wanted to comment on this because this is related to a previous 
discussion.

For `Path`,  is not directly included and I don't think 
os.hpp is explicitly "exporting" `Path` through its APIs.

I am less certain about what to do with the calls to the functions under 
`os::`. We have intentionally separated out these headers such as  
but in order to use `os::mkdir` we need to include  which includes 
everything under `stout/os/`.

Maybe we should continue to refactor all these basic functions out of 
os.hpp so we only include os.hpp when we intentionally want to include 
everything.


- Jiang Yan Xu


On Aug. 3, 2015, 10:18 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36956/
> ---
> 
> (Updated Aug. 3, 2015, 10:18 a.m.)
> 
> 
> Review request for mesos, Ian Downes, Timothy Chen, Vinod Kone, and Jiang Yan 
> Xu.
> 
> 
> Bugs: MESOS-3179
> https://issues.apache.org/jira/browse/MESOS-3179
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Created a test abstraction for preparing test rootfs.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 54eaf205eecb6bf1a9a5c4b5ddad55f46ad635ec 
>   src/tests/containerizer/launch_tests.cpp 
> 73c8c5fa17936b1bab4817e9f1e691144e87c35f 
>   src/tests/containerizer/rootfs.hpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/36956/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>