from:"Anindya Sinha"

Re: Review Request 45962: Updated a persistent volume test framework to include shared volumes.

2016-11-07 Thread Anindya Sinha


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

(Updated Nov. 8, 2016, 5:50 a.m.)


Review request for mesos, Greg Mann, Jie Yu, and Jiang Yan Xu.


Changes
---

Addressed review comments.


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


Repository: mesos


Description
---

Updated a persistent volume test framework to include shared volumes.


Diffs (updated)
-

  src/examples/persistent_volume_framework.cpp 
9d45bb496c4cf378af429b5aa970bf81450e482a 

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


Testing
---

New test framework for shared resources added.
Tests successful.


Thanks,

Anindya Sinha

Re: Review Request 53096: Fix handling in shared count in total resources in the sorter.

2016-11-07 Thread Anindya Sinha


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

(Updated Nov. 8, 2016, 5:50 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebased


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


Repository: mesos


Description
---

We maintain a single copy of shared resource in the role and quota
sorter's total resources. So, when we update these resources, we ensure
that we only count a single copy even though the framework sorter
may be returned multiple copies of a shared resource.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.cpp 
c8f9492ee1b69e125a1e841116d22a578a9b524e 
  src/tests/persistent_volume_tests.cpp 
8651b2c5455089041f16d091c90a7e0d948191b8 

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


Testing
---

Tests passed.


Thanks,

Anindya Sinha

Re: Review Request 45962: Updated a persistent volume test framework to include shared volumes.

2016-11-07 Thread Anindya Sinha

 The shard is fully running (all its tasks are 
> > launched).
> >   TERMINATING, // The shard is terminating and needs to clean up 
> > its persistent volume.
> >   DONE // The shard is terminated.
> > ```
> > 
> > This translates to:
> > 
> > ```
> >   STAGING = 0, // In resourceOffers: launch tasks
> >   RUNNING, // In resourceOffers: noop; In statusUpdate: check 
> > shard TERMINATING condition.
> >   TERMINATING, // In resourceOffers: DESTROY
> >   DONE // Test terminal condition.
> > ```

To accomplish this state, I changed shard's state to `Option state` so 
we start the state machine if `shared.state == None()`. Also, if for some 
reason `DESTROY` fails continuously, the test would not exit. We can fail the 
test if it fails on n attempts of `DESTROY` by keeping track of number of 
`DESTROY` attempts but maybe not required. Comments?


> On Nov. 7, 2016, 5:25 a.m., Jiang Yan Xu wrote:
> > src/examples/persistent_volume_framework.cpp, line 473
> > <https://reviews.apache.org/r/45962/diff/19/?file=1543197#file1543197line473>
> >
> >

`WAITING` state removed.


> On Nov. 7, 2016, 5:25 a.m., Jiang Yan Xu wrote:
> > src/examples/persistent_volume_framework.cpp, line 474
> > <https://reviews.apache.org/r/45962/diff/19/?file=1543197#file1543197line474>
> >
> > To represent a state, use a `-ing` word?
> > 
> > See the overall comment on the enum.

Changed this to `TERMINATING` state.


> On Nov. 7, 2016, 5:25 a.m., Jiang Yan Xu wrote:
> > src/examples/persistent_volume_framework.cpp, lines 475-476
> > <https://reviews.apache.org/r/45962/diff/19/?file=1543197#file1543197line475>
> >
> > Why both `WAIT_DONE` and `DONE`?
> > 
> > Doesn't look like we need to treat shared pv and regular pv differently.
> > 
> > See the overall comment on the enum.

Removed `WAIT_DONE`. I wanted to keep track of re-attempts of `DESTROY` in 
`WAIT_DONE` but maybe it is not needed (and if needed, it can be done without a 
new state).


- Anindya


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


On Nov. 7, 2016, 5:26 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45962/
> -------
> 
> (Updated Nov. 7, 2016, 5:26 a.m.)
> 
> 
> Review request for mesos, Greg Mann, Jie Yu, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4431
> https://issues.apache.org/jira/browse/MESOS-4431
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated a persistent volume test framework to include shared volumes.
> 
> 
> Diffs
> -
> 
>   src/examples/persistent_volume_framework.cpp 
> 9d45bb496c4cf378af429b5aa970bf81450e482a 
> 
> Diff: https://reviews.apache.org/r/45962/diff/
> 
> 
> Testing
> ---
> 
> New test framework for shared resources added.
> Tests successful.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 53501: Added a test to catch regressions to MESOS-4975.

2016-11-05 Thread Anindya Sinha


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


Fix it, then Ship it!




Ship It!


src/tests/master_tests.cpp (line 3118)
<https://reviews.apache.org/r/53501/#comment224930>

I do not think we need `masterFlags` in this test. So, we should get rid of 
`masterFlags` and use `Try> master = StartMaster()` 
instead?



src/tests/master_tests.cpp (line 3144)
<https://reviews.apache.org/r/53501/#comment224931>

nitpik: s/registered/frameworkRegistered, since we use `slaveRegistered` in 
this test?


- Anindya Sinha


On Nov. 4, 2016, 10:12 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53501/
> ---
> 
> (Updated Nov. 4, 2016, 10:12 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Anindya Sinha, and Vinod Kone.
> 
> 
> Bugs: MESOS-4975
> https://issues.apache.org/jira/browse/MESOS-4975
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test to catch regressions to MESOS-4975.
> 
> 
> Diffs
> -
> 
>   src/tests/master_tests.cpp 3143af15cf7a589e3f469b8e07f0b398e252ee79 
> 
> Diff: https://reviews.apache.org/r/53501/diff/
> 
> 
> Testing
> ---
> 
> make check with this test.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 52828: Allow the chown code in fetcher to be executed.

2016-11-02 Thread Anindya Sinha



> On Nov. 2, 2016, 10:36 p.m., Joseph Wu wrote:
> > src/slave/containerizer/fetcher.cpp, lines 788-790
> > 
> >
> > This short-circuit leaks the two FDs opened above.

RR for this fix: https://reviews.apache.org/r/53413


- Anindya


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


On Oct. 14, 2016, 6:14 p.m., Megha Sharma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52828/
> ---
> 
> (Updated Oct. 14, 2016, 6:14 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: mesos-5218
> https://issues.apache.org/jira/browse/mesos-5218
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Moved the uri.size() check in fetcher so that the chown to
> task user of stdout/stderr in sandbox directory happens even
> when there is no uri to be fetched.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/fetcher.cpp 
> 11104d66e6dd05d8eb1d37a2e3250aca19278110 
>   src/tests/slave_tests.cpp 93b81d3e5b90d8036409e943f598c865fe335bcc 
> 
> Diff: https://reviews.apache.org/r/52828/diff/
> 
> 
> Testing
> ---
> 
> make check on macOs and linux
> 
> 
> Thanks,
> 
> Megha Sharma
> 
>

Re: Review Request 53413: Fix FD leak due to exiting early in mesos fetcher.

2016-11-02 Thread Anindya Sinha


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

(Updated Nov. 2, 2016, 11:48 p.m.)


Review request for mesos, Joseph Wu and Jiang Yan Xu.


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


Repository: mesos


Description
---

Fix FD leak due to exiting early in mesos fetcher.


Diffs (updated)
-

  src/slave/containerizer/fetcher.cpp ea8eaa6371fc322c240e17b4bb9ba417c45ceaca 

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


Testing
---

Tests passed.


Thanks,

Anindya Sinha

Review Request 53413: Fix FD leak due to exiting early in mesos fetcher.

2016-11-02 Thread Anindya Sinha


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

Review request for mesos, Joseph Wu and Jiang Yan Xu.


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


Repository: mesos


Description
---

Fix FD leak due to exiting early in mesos fetcher.


Diffs
-

  CHANGELOG ed78657492eb3a708cdeaf9cf40d0a70270a5699 
  src/master/registrar.cpp d7134eea34102ab7b24d2f0131363bdd9005cfd3 
  src/slave/containerizer/fetcher.cpp ea8eaa6371fc322c240e17b4bb9ba417c45ceaca 

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


Testing
---

Tests passed.


Thanks,

Anindya Sinha

Re: Review Request 53369: Agent cgroup assignment should precede agent initialization.

2016-11-02 Thread Anindya Sinha



> On Nov. 2, 2016, 5:08 a.m., Jie Yu wrote:
> > The agent subsystems is a hack to me. I think we should consider support 
> > running systemd (or other init system) to manage agent process and put it 
> > under proper cgroup using the init system, rather than doing it ourself.

Agreed. But since this flag exists, I think we should address this condition? 
Or are you suggesting to deprecate this flag?


- Anindya


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


On Nov. 2, 2016, 4:43 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53369/
> ---
> 
> (Updated Nov. 2, 2016, 4:43 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6523
> https://issues.apache.org/jira/browse/MESOS-6523
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is to ensure that we do not accept traffic on the agent by opening
> up libprocess port only after cgroup assigment for the agent is done.
> 
> 
> Diffs
> -
> 
>   src/slave/main.cpp 8010f8e229e2d820649750c9db0456ecd1b854d3 
>   src/slave/slave.cpp d6c337345707993b0729e9eaf36b5a9ecc52dc72 
> 
> Diff: https://reviews.apache.org/r/53369/diff/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Review Request 53369: Agent cgroup assignment should precede agent initialization.

2016-11-01 Thread Anindya Sinha


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

Review request for mesos and Jiang Yan Xu.


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


Repository: mesos


Description
---

This is to ensure that we do not accept traffic on the agent by opening
up libprocess port only after cgroup assigment for the agent is done.


Diffs
-

  src/slave/main.cpp 8010f8e229e2d820649750c9db0456ecd1b854d3 
  src/slave/slave.cpp d6c337345707993b0729e9eaf36b5a9ecc52dc72 

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


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 52587: Allow CREATE of shared volumes based on capability of framework.

2016-11-01 Thread Anindya Sinha


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

(Updated Nov. 1, 2016, 4:54 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Addressed review comments.


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


Repository: mesos


Description
---

When a framework issues a CREATE for a shared volume, we allow that
operation only if the framework has opted in for the capability of
SHARED_RESOURCES. However, for HTTP operator (/create-volumes), we
do not check as the operator API is not related to a specific
framework.


Diffs (updated)
-

  src/master/master.cpp 013bb592ba47b785c552e199633e4784e8aa71b1 
  src/master/validation.hpp 035f721c610ae566c89a1cf0e65ff0df11679f15 
  src/master/validation.cpp f690a9eacd278b51a52f5588dbeea377df074435 
  src/tests/master_validation_tests.cpp 
a5d8610bd61822cdf55cbc8d7056e5cf8fecfa54 
  src/tests/persistent_volume_tests.cpp 
6289009fe9ed0a57ba5eff46dbbe0633a75d2616 

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


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 52934: Explain read-only mode of persistent volumes in shared-resources.md.

2016-10-27 Thread Anindya Sinha

> On Oct. 17, 2016, 6:22 p.m., Neil Conway wrote:
> > docs/shared-resources.md, line 168
> > <https://reviews.apache.org/r/52934/diff/1/?file=1539702#file1539702line168>
> >
> > I think we should discuss what happens if you try to write to a 
> > read-only volume.
> 
> Jiang Yan Xu wrote:
> SG.

We only allow persistent volumes as `RW`, they can never exist as `RO` (as in 
`Option validatePersistentVolume(const RepeatedPtrField& 
volumes)`. MESOS-6374 once implemented would not require 
`Resource::DiskInfo::Volume` to be included while doing a `CREATE` of 
persistent volumes (mode shall default to `RW`).

Tasks can request through the resources in `LAUNCH` if it wishes to use the 
volume as `RO` or `RW`. In case the task tries to write to a persistent volume 
it had requested `RO` access for, the task fails. Is there any other potential 
usage behavior that can be incorporated here as far as the task is concerned?

- Anindya

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

On Oct. 17, 2016, 6:15 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52934/
> ---
> 
> (Updated Oct. 17, 2016, 6:15 p.m.)
> 
> 
> Review request for mesos, (Disabled_DoNotUse) Anindya Sinha and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Explain read-only mode of persistent volumes in shared-resources.md.
> 
> 
> Diffs
> -
> 
>   docs/shared-resources.md 29e43389c530f816118a32dcd7c2f1b82b0af431 
> 
> Diff: https://reviews.apache.org/r/52934/diff/
> 
> 
> Testing
> ---
> 
> N/A
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 53208: Fixed Master that leaks empty entries in its hashmaps.

2016-10-27 Thread Anindya Sinha


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


Ship it!




Ship It!

- Anindya Sinha


On Oct. 27, 2016, 7:06 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53208/
> ---
> 
> (Updated Oct. 27, 2016, 7:06 p.m.)
> 
> 
> Review request for mesos, (Disabled_DoNotUse) Anindya Sinha, Benjamin Mahler, 
> Megha Sharma, and Neil Conway.
> 
> 
> Bugs: MESOS-4975 and MESOS-6482
> https://issues.apache.org/jira/browse/MESOS-4975
> https://issues.apache.org/jira/browse/MESOS-6482
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This has also caused CHECK failures mentioned in MESOS-6482.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 23ddb995b4ad0fcdb589974308a2e81ececdad31 
> 
> Diff: https://reviews.apache.org/r/53208/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> No new test is added for this fix (MESOS-4975) along, but will add one for 
> MESOS-6482 (to verify that this patch fixes it).
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 52071: Updated docs to handle resources with no size in agent flags.

2016-10-25 Thread Anindya Sinha


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

(Updated Oct. 25, 2016, 6:20 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Minor updates.


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


Repository: mesos


Description
---

When a resource with no size is specified in `--resources` startup
flag of mesos agent, the size is auto detected by the agent. This
is enabled for all known resource types (except gpus).
For disk resources, this is done for both root disks as well as
MOUNT disks, but is not allowed for PATH disks since PATH disks can
be carved into smaller volumes.


Diffs (updated)
-

  docs/attributes-resources.md 2caa44927d45c0ab13f8160794b50f4fde3711aa 

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


Testing
---

Documentation change only. All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 51880: Added unit tests to determine disk size for MOUNT disks.

2016-10-25 Thread Anindya Sinha


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

(Updated Oct. 25, 2016, 6:19 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Minor updates to tests based on modifications in other commits in this chain.


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


Repository: mesos


Description
---

Added unit tests to determine disk size for MOUNT disks.


Diffs (updated)
-

  src/Makefile.am 769e998d80fde17bcb1ee6c5091ce13a1ad16137 
  src/tests/containerizer/common_containerizer_tests.cpp PRE-CREATION 
  src/tests/resources_tests.cpp d6eb7787bac58c1133a4bab0fc17df49117fed87 

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


Testing
---

All tests including the additional tests in this RR passed.


Thanks,

Anindya Sinha

Re: Review Request 51879: Autodetect value of resource when not specified in static resources.

2016-10-25 Thread Anindya Sinha


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

(Updated Oct. 25, 2016, 6:19 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Addressed review comments.


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


Repository: mesos


Description (updated)
---

When static resources indicate resources with a positive size, we use
that for the resources on the agent. However, --resources can include
resources with no size, which indicates that mesos agent determine the
size of those resources from the agent and uses that information.

With this change, JSON or textual representation for disk resources
that do not specify any value would not result in an error, but those
resources will not be accounted for until a valid size is determined
for such resources. A scalar value of -1 in JSON or textual formats
still results in an invalid resource.


Diffs (updated)
-

  include/mesos/resources.hpp f569c931ff7db8d51dfd7c96f4f2addab05df85d 
  include/mesos/v1/resources.hpp f60ab794a0c7c24885c49cc47b798c363e3279e7 
  src/common/resources.cpp 4bb9beffcb3509f4226b4985e05eccec01412d0f 
  src/slave/containerizer/containerizer.cpp 
d46882baa904fd439bffb23c324828b777228f1c 
  src/slave/containerizer/mesos/isolators/gpu/allocator.hpp 
b2eabfebef99ccebef427d144bb816adc0175ecf 
  src/slave/containerizer/mesos/isolators/gpu/allocator.cpp 
2e722691475c84afae14009014ea70cc0fdd0e65 
  src/v1/resources.cpp 46cc00f2f453f5eb4ddc4b0b9b89be2bd89f05d9 

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


Testing
---

Tests passed.


Thanks,

Anindya Sinha

Re: Review Request 51879: Autodetect value of resource when not specified in static resources.

2016-10-25 Thread Anindya Sinha

 workflow. It would be nice if we can 
> > confine the special processing of each kind of resources to within this 
> > method.
> > 2. `parse()` encapsulates auto-detection but the main body of this 
> > method is also auto-detection, just from a different source syntactically. 
> > It would be nice we can do auto-detection from only one method.
> > 
> > 
> > Would the following workflow be simpler for 
> > `Containerizer::resources()`?
> > 
> > 1. Parse the flags into a vector of Resource objects.
> > 2. For each kind of resources, first figure out the need for 
> > auto-detection from the two cases, then do auto-detection there.
> > 3. Aggregate resources and validate them.
> > 
> > Take 'mem' as an example:
> > 
> > ```
> >   // Memory resources.
> >   vector allMem;
> >   
> >   // Filter in all mem resources.
> >   foreach (const Resource& r, resources) {
> > if (r.name() == "mem") {
> >   allMem.push_back(r);
> > }
> >   }
> > 
> >   // If no mem is specified, create one with no value for 
> > auto-detection.
> >   if (allMem.empty()) {
> > allMem.push_back(Resources::parse(
> > "mem",
> > "",
> > flags.default_role).get());
> >   }
> > 
> >   // Fill in auto-detected mem values.
> >   foreach (Resource& mem, allMem) {
> > if (!mem.has_scalar()) {
> >   Try _mem =
> > Resources::parse("mem", systemMemAmount().megabytes(), 
> > mem.role());
> >   CHECK_SOME(_mem);
> >   mem.CopyFrom(_mem.get());
> > }
> >   }
> > 
> >   resources = allMem.get() + resources.filter(
> >   [](const Resource& resource) {
> > return resource.name() != "mem";
> >   });
> > ```
> > 
> > In addition, it would be great if we can abstract out the common 
> > structure in the above code block shared by different kinds of resources. 
> > We can pass in the auto-detection methods like `systemMemAmount()` as an 
> > argument.
> > 
> > If not, the body of `Resources::resources()` still tells me the 
> > workflow/algorithm for processing resources with simple helpers for as 
> > detectors.
> > 
> > Let me know what you think.

Refer to my previous comment on the refactor.


> On Sept. 28, 2016, 10:37 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/mesos/isolators/gpu/allocator.cpp, lines 141-161
> > <https://reviews.apache.org/r/51879/diff/6/?file=1510296#file1510296line141>
> >
> > With the flags, the method here already has all information it needs. 
> > Let's delay the changes to gpus as this review is already large and more 
> > work is needed to make this better.

If `flags.resources` contains non-gpus resources with no value, then this would 
fail here (owing to invalid resources) since the auto-detection logic is 
contained here but local to `containerizer.cpp`. Hence, I exposed the optional 
`Resources` arg for this function. If present, we just use that here since the 
auto-detection logic has already been done for the call-site in that case. If 
this arg is not provided or is `None()`, we continue with the original flow.

Note that I have not added auto-detection of gpus in this chain.


- Anindya


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


On Oct. 6, 2016, 2 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51879/
> ---
> 
> (Updated Oct. 6, 2016, 2 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When static resources indicate resources with a positive size, we use
> that for the resources on the agent. However, --resources can include
> resources with no size, which indicates that mesos agent determine the
> size of those resources from the agent and uses that information. For
> disk resources, this is not allowed for PATH disks since PATH disks
> can be carved into smaller chunks and we cannot assume that the whole
> physical disk is available to the PATH disk.
> 
> With this change, JSON or textual representation for disk resources
> that specify scalar value of 0 would not result in an error, but those
> resources will not be accounted for until a valid size is determined
> for such resources. A scalar value of -1 in JSON or textual formats
> still results in an invalid resource.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
>   include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
>   src/common/resources.cpp 0774ff0669e831494d5b12b88e19dfa0a4a3f757 
>   src/slave/containerizer/containerizer.cpp 
> d46882baa904fd439bffb23c324828b777228f1c 
>   src/slave/containerizer/mesos/isolators/gpu/allocator.hpp 
> b2eabfebef99ccebef427d144bb816adc0175ecf 
>   src/slave/containerizer/mesos/isolators/gpu/allocator.cpp 
> 2e722691475c84afae14009014ea70cc0fdd0e65 
>   src/v1/resources.cpp 62a644ebbd13cfc0862bd118ba16c43e0f6aaf90 
> 
> Diff: https://reviews.apache.org/r/51879/diff/
> 
> 
> Testing
> ---
> 
> Tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 52002: Added helper methods to determine types of disk resources.

2016-10-25 Thread Anindya Sinha



> On Oct. 5, 2016, 8:56 p.m., Jiang Yan Xu wrote:
> > include/mesos/resources.hpp, lines 278-280
> > <https://reviews.apache.org/r/52002/diff/7/?file=1519241#file1519241line278>
> >
> > I still have reservations about this method.
> > 
> > As we discussed, a predicate returning a `bool` is consistent with 
> > other similar methods and can be used more generally, e.g.,
> > 
> > ```
> > resources.filter([](const Resource& resource) { return 
> > isMountDisk(resource); });
> > ```
> > 
> > Your main concern (from our conversations) is that it's possible that 
> > we add more disk types and it would be unwieldy if we have 17 similar calls 
> > in the form of `bool Resources::isXYZ()`.
> > 
> > My take is that, we currently have only a handful of predefined first 
> > class resource types (Mesos has special logic for them) and it's not 
> > unreasonble to have first class support (such as having an `isXYZ()` 
> > dedicated for it) for all of them. If in the future we have a lot of them, 
> > we should still provide first class support for **all** of them, but before 
> > this list gets too large we should probably improve the abstraction to 
> > support it in a different, more structured way, e.g., a `Disk` class for 
> > disks.
> > 
> > ---
> > 
> > The problems of adding this method right now include:
> > 
> > ## 1. We shouldn't use Try::error when it's not really an error.
> > 
> > Looking at
> > 
> > ```
> > static Try getDiskSource(const 
> > Resource& resource);
> > ```
> > 
> > I wouldn't know how to deal with the error, is it because the Resource 
> > is invalid? Is it not a disk? It being a valid disk but without a 
> > `Source::Type` is not one of the top things spring to mind.
> > 
> > To get the disk type, it may be  (slightly) better to have:
> > 
> > ```
> > static Option getDiskSourceType(const 
> > Resource& resource);
> > ```
> > 
> > With None meaning "there's no source type". But the problem is that 
> > with a generic `resource` argument, what if the resource is valid but not a 
> > disk? What if the resource is invalid?
> > 
> > ## 2. The real problem is a flat Resource type with methods that make 
> > sense only to a special kind of Resource.
> > 
> > Due to the different shapes of pre-defined resources, I find it 
> > unrealistic/difficult to use/implement methods that handle/expose all 
> > error/edge cases in one call.
> > 
> > It would have made sense to have
> > 
> > ```
> > Disk::Type Disk::getType();
> > ```
> > 
> > where we know disk is valid. `Disk` can be a subclass of C++ 
> > (non-protobuf) `Resource` which is guaranteed to be valid.
> > 
> > ---
> > 
> > For now, I think we should aim for consistency by sticking to the 
> > `isXYZ()` methods. It makes using and understanding of the methods easier 
> > both for the current users and for refactor in the future. (After all I 
> > think manging the complexity of current use and complexity of change is all 
> > that we are doing here). In most cases the client has to know if the 
> > Resource represents a (valid) disk first, they can do that and then use the 
> > set of `isXYZ()` to determine the type.
> > 
> > ---
> > 
> > 
> > Finally, we may not even need `isMountDisk()` or `isPathDisk` given how 
> > /r/51879/ is going. It may make sense to not specially treat PATH disk 
> > differently but let's disucss that in /r/51879/. With this review we can 
> > just have a `bool isRootDisk()` first if you like.

I removed the API `getDiskSource()` based on feedback from 
https://reviews.apache.org/r/51879/.


- Anindya


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


On Oct. 6, 2016, 2 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52002/
> ---
> 
> (Updated Oct. 6, 2016, 2 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
>

Re: Review Request 52002: Added helper methods to determine types of disk resources.

2016-10-25 Thread Anindya Sinha


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

(Updated Oct. 25, 2016, 6:19 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Addressed review comments.


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


Repository: mesos


Description
---

Added helper methods to determine types of disk resources.


Diffs (updated)
-

  include/mesos/resources.hpp f569c931ff7db8d51dfd7c96f4f2addab05df85d 
  include/mesos/v1/resources.hpp f60ab794a0c7c24885c49cc47b798c363e3279e7 
  src/common/resources.cpp 4bb9beffcb3509f4226b4985e05eccec01412d0f 
  src/tests/resources_tests.cpp d6eb7787bac58c1133a4bab0fc17df49117fed87 
  src/v1/resources.cpp 46cc00f2f453f5eb4ddc4b0b9b89be2bd89f05d9 

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


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 52002: Added helper methods to determine types of disk resources.

2016-10-25 Thread Anindya Sinha



> On Oct. 2, 2016, 3:14 a.m., Guangya Liu wrote:
> > src/tests/resources_tests.cpp, line 2182
> > <https://reviews.apache.org/r/52002/diff/6/?file=1513455#file1513455line2182>
> >
> > Here we should keep 4 space start from the `(` above or else you can:
> > 
> > ```
> > ASSERT_TRUE(
> > Resources::isRootDisk(
> > createDiskResource("100", "role1", None(), None(;
> >     ```
> > 
> > Ditto for others which has two `(` in one line.
> 
> Anindya Sinha wrote:
> Can you point to the specific style from the style guide 
> (http://mesos.apache.org/documentation/latest/c++-style-guide/) which is 
> being violated here?
> I agree your option would satisfy the style guide but am curious why do 
> you think the current version violates it. AFAICT, there is no specific rule 
> for 2 "(" in the same line.
> 
> Guangya Liu wrote:
> I did not found this in C++ style, but in a review here 
> https://reviews.apache.org/r/52020/

I think I would leave it the way it is since there are numerous places where 
this pattern is used. As one example: 
https://github.com/apache/mesos/blob/master/src/tests/persistent_volume_tests.cpp#L539-L541


- Anindya


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


On Oct. 6, 2016, 2 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52002/
> ---
> 
> (Updated Oct. 6, 2016, 2 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added helper methods to determine types of disk resources.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
>   include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
>   src/common/resources.cpp 0774ff0669e831494d5b12b88e19dfa0a4a3f757 
>   src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
>   src/v1/resources.cpp 62a644ebbd13cfc0862bd118ba16c43e0f6aaf90 
> 
> Diff: https://reviews.apache.org/r/52002/diff/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Review Request 53096: Fix handling in shared count in total resources in the sorter.

2016-10-21 Thread Anindya Sinha


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

Review request for mesos and Jiang Yan Xu.


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


Repository: mesos


Description
---

We maintain a single copy of shared resource in the role and quota
sorter's total resources. So, when we update these resources, we ensure
that we only count a single copy even though the framework sorter
may be returned multiple copies of a shared resource.


Diffs
-

  src/master/allocator/mesos/hierarchical.cpp 
c8f9492ee1b69e125a1e841116d22a578a9b524e 
  src/tests/persistent_volume_tests.cpp 
6289009fe9ed0a57ba5eff46dbbe0633a75d2616 

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


Testing
---

Tests passed.


Thanks,

Anindya Sinha

Re: Review Request 49571: Added a benchmark test for allocations.

2016-10-21 Thread Anindya Sinha


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

(Updated Oct. 21, 2016, 6:52 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Allocations test has the following resource configurations:
(1) REGULAR: Offers from every slave have regular resources.
(2) SHARED: Offers from every slave include a shared resource.
(3) REGULAR: Offers from every alternate slave contain only regular
resources; and offers from every other alternate slave contains
a shared resource.

This test is parameterized based on number of agents, number of
frameworks and resource configuration.


Diffs (updated)
-

  src/tests/hierarchical_allocator_tests.cpp 
2e979d784b8e6cdacebac78a67498b5f4d023540 

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


Testing
---

All tests passed.

Allocations benchmark test results
==
Support of shared resources has a small impact (roughly 10%) on runtime 
performance in allocations as compared to HEAD (without shared resources). 
Also, there is no visible impact in performance when shared resources are added 
in the tests.

Following is a snapshot with 1000 agents and 200 frameworks.

With the patch (and no shared resources)

[ RUN  ] AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/9
Using 1000 agents and 200 frameworks with resource type 0
Added 200 frameworks in 6907us
Added 1000 agents in 2.057098secs
round 0 allocate() took 1.689164secs to make 1000 offers
round 50 allocate() took 1.672373secs to make 1000 offers
round 100 allocate() took 1.680571secs to make 1000 offers
round 150 allocate() took 1.674683secs to make 1000 offers
round 199 allocate() took 1.671525secs to make 1000 offers

With the patch (and shared resources on all agents)
---
[ RUN  ] AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/10
Using 1000 agents and 200 frameworks with resource type 1
Added 200 frameworks in 6888us
Added 1000 agents in 2.096218secs
round 0 allocate() took 1.704491secs to make 1000 offers
round 50 allocate() took 1.718623secs to make 1000 offers
round 100 allocate() took 1.716224secs to make 1000 offers
round 150 allocate() took 1.707343secs to make 1000 offers
round 199 allocate() took 1.727467secs to make 1000 offers

With the patch (and shared resources on alternate agents)
-
[ RUN  ] AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/11
Using 1000 agents and 200 frameworks with resource type 2
Added 200 frameworks in 7304us
Added 1000 agents in 2.071009secs
round 0 allocate() took 1.689045secs to make 1000 offers
round 50 allocate() took 1.691524secs to make 1000 offers
round 100 allocate() took 1.688873secs to make 1000 offers
round 150 allocate() took 1.688713secs to make 1000 offers
round 199 allocate() took 1.691223secs to make 1000 offers

Based on HEAD, with all regular resources (no shared resources in HEAD 
supported)
-
[ RUN  ] AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/9
Using 1000 agents and 200 frameworks with resource type 0
Added 200 frameworks in 6801us
Added 1000 agents in 1.721447secs
round 0 allocate() took 1.502953secs to make 1000 offers
round 50 allocate() took 1.520157secs to make 1000 offers
round 100 allocate() took 1.517221secs to make 1000 offers
round 150 allocate() took 1.526446secs to make 1000 offers
round 199 allocate() took 1.538005secs to make 1000 offers


Thanks,

Anindya Sinha

Re: Review Request 45962: Updated a persistent volume test framework to include shared volumes.

2016-10-21 Thread Anindya Sinha


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

(Updated Oct. 21, 2016, 6:52 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Consolidated shared and regular persistent example frameworks into a single 
framework.


Summary (updated)
-

Updated a persistent volume test framework to include shared volumes.


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


Repository: mesos


Description (updated)
---

Updated a persistent volume test framework to include shared volumes.


Diffs (updated)
-

  src/examples/persistent_volume_framework.cpp 
9d45bb496c4cf378af429b5aa970bf81450e482a 

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


Testing
---

New test framework for shared resources added.
Tests successful.


Thanks,

Anindya Sinha

Re: Review Request 45962: Added a persistent volume test framework for shared volumes.

2016-10-21 Thread Anindya Sinha



> On Oct. 7, 2016, 5:20 p.m., Greg Mann wrote:
> > Given how similar the code in the new example framework is to the existing 
> > persistent volumes framework, I think it could make sense to add shared 
> > volume features to the existing framework, which could be enabled/disabled 
> > via command-line flags. Especially since you add some nice new DESTROY 
> > logic in the new framework; it would be great to have that change in the 
> > existing one, and avoid duplicating a lot of code. What do you think?
> 
> Anindya Sinha wrote:
> Agreed. Makes sense. Will do that in the next update.

Updated with this.


- Anindya


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


On Sept. 28, 2016, 12:15 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45962/
> ---
> 
> (Updated Sept. 28, 2016, 12:15 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4431
> https://issues.apache.org/jira/browse/MESOS-4431
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a persistent volume test framework for shared volumes.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am fba488f9d676851dd046a8b8c7dd175b3c0d9ef0 
>   src/examples/persistent_shared_volume_framework.cpp PRE-CREATION 
>   src/tests/examples_tests.cpp 52fac33733cc86dd718c7836c5031999f1597a5c 
>   src/tests/persistent_shared_volume_framework_test.sh PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/45962/diff/
> 
> 
> Testing
> ---
> 
> New test framework for shared resources added.
> Tests successful.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 53031: Failure in cleanup of non-existing cgroups is treated as success.

2016-10-19 Thread Anindya Sinha


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

(Updated Oct. 20, 2016, 6:20 a.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Addressed review comments.


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


Repository: mesos


Description
---

Failure in cleanup of non-existing cgroups is treated as success.


Diffs (updated)
-

  src/linux/cgroups.cpp 1475c8eb848ece2a093e8243b51e9ce08981dd7d 

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


Testing
---

All existing tests passed.


Thanks,

Anindya Sinha

Review Request 53031: Failure in cleanup of non-existing cgroups is treated as success.

2016-10-19 Thread Anindya Sinha


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

Review request for mesos, Jie Yu and Jiang Yan Xu.


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


Repository: mesos


Description
---

Failure in cleanup of non-existing cgroups is treated as success.


Diffs
-

  src/linux/cgroups.cpp 1475c8eb848ece2a093e8243b51e9ce08981dd7d 

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


Testing
---

All existing tests passed.


Thanks,

Anindya Sinha

Re: Review Request 52980: Updated os::mkdir() to take the mode as an input.

2016-10-18 Thread Anindya Sinha



> On Oct. 18, 2016, 7:58 p.m., Alex Clemmer wrote:
> > 3rdparty/stout/include/stout/os/mkdir.hpp, line 43
> > <https://reviews.apache.org/r/52980/diff/1/?file=1540377#file1540377line43>
> >
> > Permissions is a bit of a subtle issue in Mesos, which we've largely 
> > punted on. I'm a bit worried that adding a pluggable `mode` here will cause 
> > us to add code that only works on Unix. How do you see this API evolving to 
> > accommodate both?
> 
> Alex Clemmer wrote:
> Also, sorry, I should have been more clear: I mean that permissions _on 
> Windows_ is a subtle issue in Mesos. My worry is rooted in the fact that this 
> API also has to support Windows.

So, er either extend `mkdir()` to include a `mode` (which as you indicated 
would need a change in the windows api also), OR call `chmod()` recursively 
after the call to the existing `mkdir()` [ie. without `mode`]. Currently, linux 
implementation in stout does not have a recursive `chmod()` which needs to be 
added, and I think windows just punts on `chmod()` calls.


- Anindya


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


On Oct. 18, 2016, 7:43 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52980/
> ---
> 
> (Updated Oct. 18, 2016, 7:43 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4893
> https://issues.apache.org/jira/browse/MESOS-4893
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Mode still defaults to 755, but we now enable the caller to specify
> a different mode for creation of directories.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/mkdir.hpp 
> fe86864c8b480993c8f052f39b2fd3ece23798da 
> 
> Diff: https://reviews.apache.org/r/52980/diff/
> 
> 
> Testing
> ---
> 
> Tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 52980: Updated os::mkdir() to take the mode as an input.

2016-10-18 Thread Anindya Sinha



> On Oct. 18, 2016, 7:58 p.m., Alex Clemmer wrote:
> > 3rdparty/stout/include/stout/os/mkdir.hpp, line 43
> > <https://reviews.apache.org/r/52980/diff/1/?file=1540377#file1540377line43>
> >
> > Permissions is a bit of a subtle issue in Mesos, which we've largely 
> > punted on. I'm a bit worried that adding a pluggable `mode` here will cause 
> > us to add code that only works on Unix. How do you see this API evolving to 
> > accommodate both?
> 
> Alex Clemmer wrote:
> Also, sorry, I should have been more clear: I mean that permissions _on 
> Windows_ is a subtle issue in Mesos. My worry is rooted in the fact that this 
> API also has to support Windows.
> 
> Anindya Sinha wrote:
> So, er either extend `mkdir()` to include a `mode` (which as you 
> indicated would need a change in the windows api also), OR call `chmod()` 
> recursively after the call to the existing `mkdir()` [ie. without `mode`]. 
> Currently, linux implementation in stout does not have a recursive `chmod()` 
> which needs to be added, and I think windows just punts on `chmod()` calls.

s/er/we. So to restate my comments once again:

So, we either extend `mkdir()` to include a `mode` (which as you indicated 
would need a change in the windows api also), OR call `chmod()` recursively 
after the call to the existing `mkdir()` [ie. without `mode`]. Currently, linux 
implementation in stout does not have a recursive `chmod()` which needs to be 
added, and I think windows just punts on chmod() calls.


- Anindya


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


On Oct. 18, 2016, 7:43 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52980/
> ---
> 
> (Updated Oct. 18, 2016, 7:43 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4893
> https://issues.apache.org/jira/browse/MESOS-4893
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Mode still defaults to 755, but we now enable the caller to specify
> a different mode for creation of directories.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/mkdir.hpp 
> fe86864c8b480993c8f052f39b2fd3ece23798da 
> 
> Diff: https://reviews.apache.org/r/52980/diff/
> 
> 
> Testing
> ---
> 
> Tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 46230: Updated docs to reflect user in persistent volumes.

2016-10-18 Thread Anindya Sinha


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

(Updated Oct. 18, 2016, 7:46 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Updated docs to reflect user in persistent volumes.


Diffs (updated)
-

  docs/persistent-volume.md 5dfbbf1fb08cdf47f97ea6fb286e21ab26235d62 

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


Testing
---

Documentation change only. All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 46229: Add unit tests for adding a user for persistent volumes.

2016-10-18 Thread Anindya Sinha


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

(Updated Oct. 18, 2016, 7:46 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Add unit tests for adding a user for persistent volumes.


Diffs (updated)
-

  src/tests/containerizer/linux_filesystem_isolator_tests.cpp 
f17ed4437a5e1366f85803ce7e29bee24162504c 
  src/tests/hierarchical_allocator_tests.cpp 
2e979d784b8e6cdacebac78a67498b5f4d023540 
  src/tests/master_validation_tests.cpp 
da43f990e3b8f61e27b22d551cd3e07638c7ff37 
  src/tests/mesos.hpp 9309b5a985c0d7136a2ee5aa1598b4fee6194816 
  src/tests/persistent_volume_tests.cpp 
6289009fe9ed0a57ba5eff46dbbe0633a75d2616 
  src/tests/resources_tests.cpp 6a12783c26f359dda835b4866b299a8fcfb3f972 
  src/tests/sorter_tests.cpp 1f17c011898836ea9159661dde7d544cb0d8ea83 

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


Testing
---

All tests passed (including the newly added tests).


Thanks,

Anindya Sinha

Re: Review Request 46228: Create persistent volume with a supplied user.

2016-10-18 Thread Anindya Sinha


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

(Updated Oct. 18, 2016, 7:45 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Addressed review comments, and rebased.


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


Repository: mesos


Description (updated)
---

If user is specified in Resource::DiskInfo::Persistence, set the
ownership of the volume to that user. Tasks should have to comply to
ownership of the volume before they can use the volume.
If user is not specified in Resource::DiskInfo::Persistence, it is
created with the user mesos-slave process runs as. When the first
task is launched, it updates the ownership of the persistent volume
with its user so as to be able to write into that volume.
Note that tasks can fail to use the volume when they actually try
to access the volume is that task's ownership is not compatible
with that of the volume.


Diffs (updated)
-

  src/common/resources.cpp 4bb9beffcb3509f4226b4985e05eccec01412d0f 
  src/slave/containerizer/docker.cpp 8ec4c0a25335fb1b36cb2ab82577f6d3e2f7f008 
  src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
df16b8fee6799a69c7d96f33a5049bd9787c48f5 
  src/slave/containerizer/mesos/isolators/filesystem/posix.cpp 
270d2aa6e06f323bfb6eee3b703a24a600a55871 
  src/slave/slave.cpp 6bd9b49c3bbdb973a0d03552ae8fe55b33371083 
  src/v1/resources.cpp 46cc00f2f453f5eb4ddc4b0b9b89be2bd89f05d9 

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


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Review Request 52980: Updated os::mkdir() to take the mode as an input.

2016-10-18 Thread Anindya Sinha


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

Review request for mesos and Jiang Yan Xu.


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


Repository: mesos


Description
---

Mode still defaults to 755, but we now enable the caller to specify
a different mode for creation of directories.


Diffs
-

  3rdparty/stout/include/stout/os/mkdir.hpp 
fe86864c8b480993c8f052f39b2fd3ece23798da 

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


Testing
---

Tests passed.


Thanks,

Anindya Sinha

Re: Review Request 46227: Added an user to indicate owner of persistent volume.

2016-10-18 Thread Anindya Sinha


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

(Updated Oct. 18, 2016, 7:42 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Addressed review comments, and rebased.


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


Repository: mesos


Description
---

When a persistent volume is created, it is created with the user
indicated in Resource::DiskInfo::Persistence. If missing, it would
fall back to make the user of the mesos-slave process as the owner
of the persistent volume.


Diffs (updated)
-

  include/mesos/mesos.proto 0017d3d9d46433b391703025f611ce437bbc7ebe 
  include/mesos/v1/mesos.proto def33ef5e446576c86da0498e8a24e2e2de17918 

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


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 45963: Allow tasks to set persistent volume as readonly or readwrite resource.

2016-10-13 Thread Anindya Sinha



> On Oct. 13, 2016, 9:58 p.m., Jiang Yan Xu wrote:
> > src/master/validation.cpp, lines 503-504
> > <https://reviews.apache.org/r/45963/diff/16/?file=1533957#file1533957line503>
> >
> > MESOS-6374 is where we want eventually but for now the same check is in 
> > `validateDiskInfo()` as well so we don't have to duplicate it here?

Check for `volume.disk().has_volume()` is done since the next line calls 
`volume.disk().volume().mode()`.


> On Oct. 13, 2016, 9:58 p.m., Jiang Yan Xu wrote:
> > src/master/validation.cpp, lines 505-506
> > <https://reviews.apache.org/r/45963/diff/16/?file=1533957#file1533957line505>
> >
> > This is called when you destroy a volume as well. So if the framework 
> > is has changed the volume to RO after creating it then it cannot destroy it 
> > (at least in the form it was created)...
> > 
> > I think we should just validate this in the following method?
> > 
> > ```
> > Option validate(
> > const Offer::Operation::Create& create,
> > const Resources& checkpointedResources,
> > const Option& principal) {}
> > ```

Since the offer to frameworks would only contain `Mode::RW`, this check is 
valid for both `CREATE` and `DESTROY` operations. So, we should not allow this 
volume as valid if the `DESTROY` operation marks this as `Mode::RO`.


> On Oct. 13, 2016, 9:58 p.m., Jiang Yan Xu wrote:
> > src/tests/containerizer/linux_filesystem_isolator_tests.cpp, line 1455
> > <https://reviews.apache.org/r/45963/diff/16/?file=1533961#file1533961line1455>
> >
> > Nit: use `frameworkInfo.set_role(DEFAULT_TEST_ROLE);`?

I will leave it as `role1` instead of `DEFAULT_TEST_ROLE` since the remaining 
tests in this file use `role1`. We can change to `DEFAULT_TEST_ROLE` in a 
follow up commit when all tests move to using `DEFAULT_TEST_ROLE`.


- Anindya


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


On Oct. 13, 2016, 5:24 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45963/
> ---
> 
> (Updated Oct. 13, 2016, 5:24 a.m.)
> 
> 
> Review request for mesos, Greg Mann and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4324
> https://issues.apache.org/jira/browse/MESOS-4324
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allow the task to specify the persistent volume access to be read-only
> or read-write. Note that the persistent volume is always created as
> read-write.
> If the task is the first consumer of the shared persistent volume, then
> set the ownership of the persistent volume to match that of the task.
> Otherwise, launch the task but if the task is unable to read/write the
> persistent volume, it would fail at that point of time.
> 
> 
> Diffs
> -
> 
>   src/master/validation.cpp e5da3c9bf0a1042b42522f1ab74ce798fbb1738d 
>   src/slave/containerizer/docker.cpp d71386089bf7677872bcb1bd36e07da9263dcf0d 
>   src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
> 8f62162519f12a157c28ca5f2a76502e466c1481 
>   src/slave/containerizer/mesos/isolators/filesystem/posix.cpp 
> af427c6e5691f1770ab3ebef79502eb2c2176c4a 
>   src/tests/containerizer/linux_filesystem_isolator_tests.cpp 
> eb191a32381f9d1ca84ec29adf352dde375c2f2d 
>   src/tests/master_validation_tests.cpp 
> 99e350e0587e73e9ee25ef20dd369cd146bd446a 
> 
> Diff: https://reviews.apache.org/r/45963/diff/
> 
> 
> Testing
> ---
> 
> Tests successful.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 45963: Allow tasks to set persistent volume as readonly or readwrite resource.

2016-10-13 Thread Anindya Sinha


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

(Updated Oct. 14, 2016, 3:23 a.m.)


Review request for mesos, Greg Mann and Jiang Yan Xu.


Changes
---

Addressed comments.


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


Repository: mesos


Description
---

Allow the task to specify the persistent volume access to be read-only
or read-write. Note that the persistent volume is always created as
read-write.
If the task is the first consumer of the shared persistent volume, then
set the ownership of the persistent volume to match that of the task.
Otherwise, launch the task but if the task is unable to read/write the
persistent volume, it would fail at that point of time.


Diffs (updated)
-

  src/master/validation.cpp e5da3c9bf0a1042b42522f1ab74ce798fbb1738d 
  src/slave/containerizer/docker.cpp 9a1a8712bf4aba27927136b3a61beaca1b1af997 
  src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
8f62162519f12a157c28ca5f2a76502e466c1481 
  src/slave/containerizer/mesos/isolators/filesystem/posix.cpp 
af427c6e5691f1770ab3ebef79502eb2c2176c4a 
  src/tests/containerizer/linux_filesystem_isolator_tests.cpp 
eb191a32381f9d1ca84ec29adf352dde375c2f2d 
  src/tests/master_validation_tests.cpp 
99e350e0587e73e9ee25ef20dd369cd146bd446a 

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


Testing
---

Tests successful.


Thanks,

Anindya Sinha

Re: Review Request 45963: Allow tasks to set persistent volume as readonly or readwrite resource.

2016-10-12 Thread Anindya Sinha



> On Oct. 7, 2016, 5:49 p.m., Greg Mann wrote:
> > src/slave/containerizer/mesos/isolators/filesystem/posix.hpp, line 55
> > <https://reviews.apache.org/r/45963/diff/15/?file=1511373#file1511373line55>
> >
> > If you end up removing the check for persistent volumes in this 
> > function per Yan's comment, then perhaps we could also add a comment here 
> > saying that the function assumes that its input is a persistent volume. 
> > Renaming the resource parameter to `persistentVolume` could help make this 
> > clear as well.

I moved the functionality inline since the resource being looked into is a 
persistent volume.


> On Oct. 7, 2016, 5:49 p.m., Greg Mann wrote:
> > src/examples/persistent_shared_volume_framework.cpp, line 191
> > <https://reviews.apache.org/r/45963/diff/15/?file=1511368#file1511368line191>
> >
> > s/used/uses/
> 
> Greg Mann wrote:
> It looks like this typo is still present? Or perhaps you meant to drop 
> the issue instead of resolve?

Will fix in https://reviews.apache.org/r/45962 when I update that. Please feel 
free to add that comment in https://reviews.apache.org/r/45962 so that it is 
not forgotten.


- Anindya


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


On Oct. 13, 2016, 5:24 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45963/
> ---
> 
> (Updated Oct. 13, 2016, 5:24 a.m.)
> 
> 
> Review request for mesos, Greg Mann and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4324
> https://issues.apache.org/jira/browse/MESOS-4324
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allow the task to specify the persistent volume access to be read-only
> or read-write. Note that the persistent volume is always created as
> read-write.
> If the task is the first consumer of the shared persistent volume, then
> set the ownership of the persistent volume to match that of the task.
> Otherwise, launch the task but if the task is unable to read/write the
> persistent volume, it would fail at that point of time.
> 
> 
> Diffs
> -
> 
>   src/master/validation.cpp e5da3c9bf0a1042b42522f1ab74ce798fbb1738d 
>   src/slave/containerizer/docker.cpp d71386089bf7677872bcb1bd36e07da9263dcf0d 
>   src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
> 8f62162519f12a157c28ca5f2a76502e466c1481 
>   src/slave/containerizer/mesos/isolators/filesystem/posix.cpp 
> af427c6e5691f1770ab3ebef79502eb2c2176c4a 
>   src/tests/containerizer/linux_filesystem_isolator_tests.cpp 
> eb191a32381f9d1ca84ec29adf352dde375c2f2d 
>   src/tests/master_validation_tests.cpp 
> 99e350e0587e73e9ee25ef20dd369cd146bd446a 
> 
> Diff: https://reviews.apache.org/r/45963/diff/
> 
> 
> Testing
> ---
> 
> Tests successful.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 45967: Added documentation for shareable resources.

2016-10-12 Thread Anindya Sinha


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

(Updated Oct. 13, 2016, 5:25 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Addressed review comments.


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


Repository: mesos


Description
---

Added documentation for shareable resources.


Diffs (updated)
-

  docs/home.md ad59eb1722b49cd72454795c095ddbd1ec1eb4dd 
  docs/shared-resources.md PRE-CREATION 

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


Testing
---

Tests successful.


Thanks,

Anindya Sinha

Re: Review Request 45963: Allow tasks to set persistent volume as readonly or readwrite resource.

2016-10-12 Thread Anindya Sinha


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

(Updated Oct. 13, 2016, 5:24 a.m.)


Review request for mesos, Greg Mann and Jiang Yan Xu.


Changes
---

Addressed review comments.


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


Repository: mesos


Description (updated)
---

Allow the task to specify the persistent volume access to be read-only
or read-write. Note that the persistent volume is always created as
read-write.
If the task is the first consumer of the shared persistent volume, then
set the ownership of the persistent volume to match that of the task.
Otherwise, launch the task but if the task is unable to read/write the
persistent volume, it would fail at that point of time.


Diffs (updated)
-

  src/master/validation.cpp e5da3c9bf0a1042b42522f1ab74ce798fbb1738d 
  src/slave/containerizer/docker.cpp d71386089bf7677872bcb1bd36e07da9263dcf0d 
  src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
8f62162519f12a157c28ca5f2a76502e466c1481 
  src/slave/containerizer/mesos/isolators/filesystem/posix.cpp 
af427c6e5691f1770ab3ebef79502eb2c2176c4a 
  src/tests/containerizer/linux_filesystem_isolator_tests.cpp 
eb191a32381f9d1ca84ec29adf352dde375c2f2d 
  src/tests/master_validation_tests.cpp 
99e350e0587e73e9ee25ef20dd369cd146bd446a 

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


Testing
---

Tests successful.


Thanks,

Anindya Sinha

Re: Review Request 45963: Allow tasks to set persistent volume as readonly or readwrite resource.

2016-10-12 Thread Anindya Sinha



> On Oct. 7, 2016, 4:49 p.m., Jiang Yan Xu wrote:
> > src/master/validation.hpp, lines 72-74
> > <https://reviews.apache.org/r/45963/diff/15/?file=1511369#file1511369line72>
> >
> > Not quite sure what `isTaskContext` is?

We want to make sure persistent volumes when CREATEd have a mode as `RW`, but 
for tasks it can be either `RO` or `RW`. As discussed, we shall not check for 
mode in this function. Instead, that check shall be contained within 
`Option validatePersistentVolume(const RepeatedPtrField& 
volumes)` which is only called when validating volumes in `CREATE` or `DESTROY` 
calls.


> On Oct. 7, 2016, 4:49 p.m., Jiang Yan Xu wrote:
> > src/master/validation.cpp, lines 426-428
> > <https://reviews.apache.org/r/45963/diff/15/?file=1511370#file1511370line426>
> >
> > Not sure about the meaning of `isTaskContext` but for simplicty I think 
> > we can just remove this check. Previously it didn't make sense to ever 
> > allow the volume to be RO but now the situation is changed with shared 
> > persistent volumes. Even when the persistent volume is not shared, the task 
> > is just going to fail if it attempts to write to a read-only volume so it 
> > seems no harm to me that we remove this check.

See previous response.


> On Oct. 7, 2016, 4:49 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/mesos/isolators/filesystem/linux.cpp, lines 659-660
> > <https://reviews.apache.org/r/45963/diff/15/?file=1511372#file1511372line659>
> >
> > What's the problem with the previous formatting?

It crosses 80 chars mainly because it is not in a nested block.


> On Oct. 7, 2016, 4:49 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/mesos/isolators/filesystem/linux.cpp, lines 698-701
> > <https://reviews.apache.org/r/45963/diff/15/?file=1511372#file1511372line698>
> >
> > This may not work, IIRC I needed to mount twice previously:
> > 
> > 
> > https://github.com/apache/mesos/blob/4ea9651aabd01f623f2578d2823271488d924c5b/src/slave/containerizer/mesos/provisioner/backends/bind.cpp#L133-L158
> > 
> > But could you verify? i.e., a unit test?

Great catch. Apparently, this inconsistency has been fixed in util-linux v2.27. 
Prior to this version, we need to do a bind mount followed by a remount as 
read-only. Post util-linux v2.27, a single bind mount as read-only should 
suffice.
So, I moved this to a 2-stage mount.


- Anindya


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


On Oct. 13, 2016, 5:24 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45963/
> ---
> 
> (Updated Oct. 13, 2016, 5:24 a.m.)
> 
> 
> Review request for mesos, Greg Mann and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4324
> https://issues.apache.org/jira/browse/MESOS-4324
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allow the task to specify the persistent volume access to be read-only
> or read-write. Note that the persistent volume is always created as
> read-write.
> If the task is the first consumer of the shared persistent volume, then
> set the ownership of the persistent volume to match that of the task.
> Otherwise, launch the task but if the task is unable to read/write the
> persistent volume, it would fail at that point of time.
> 
> 
> Diffs
> -
> 
>   src/master/validation.cpp e5da3c9bf0a1042b42522f1ab74ce798fbb1738d 
>   src/slave/containerizer/docker.cpp d71386089bf7677872bcb1bd36e07da9263dcf0d 
>   src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
> 8f62162519f12a157c28ca5f2a76502e466c1481 
>   src/slave/containerizer/mesos/isolators/filesystem/posix.cpp 
> af427c6e5691f1770ab3ebef79502eb2c2176c4a 
>   src/tests/containerizer/linux_filesystem_isolator_tests.cpp 
> eb191a32381f9d1ca84ec29adf352dde375c2f2d 
>   src/tests/master_validation_tests.cpp 
> 99e350e0587e73e9ee25ef20dd369cd146bd446a 
> 
> Diff: https://reviews.apache.org/r/45963/diff/
> 
> 
> Testing
> ---
> 
> Tests successful.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 49571: Added a benchmark test for allocations.

2016-10-12 Thread Anindya Sinha


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

(Updated Oct. 13, 2016, 5:24 a.m.)


Review request for mesos and Jiang Yan Xu.


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


Repository: mesos


Description
---

Allocations test has the following resource configurations:
(1) REGULAR: Offers from every slave have regular resources.
(2) SHARED: Offers from every slave include a shared resource.
(3) REGULAR: Offers from every alternate slave contain only regular
resources; and offers from every other alternate slave contains
a shared resource.

This test is parameterized based on number of agents, number of
frameworks and resource configuration.


Diffs
-

  src/tests/hierarchical_allocator_tests.cpp 
46553f6d501deb37862dd5ba48be1c114f6e6cb8 

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


Testing
---

All tests passed.

Allocations benchmark test results
==
Support of shared resources has a small impact (roughly 10%) on runtime 
performance in allocations as compared to HEAD (without shared resources). 
Also, there is no visible impact in performance when shared resources are added 
in the tests.

Following is a snapshot with 1000 agents and 200 frameworks.

With the patch (and no shared resources)

[ RUN  ] AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/9
Using 1000 agents and 200 frameworks with resource type 0
Added 200 frameworks in 6907us
Added 1000 agents in 2.057098secs
round 0 allocate() took 1.689164secs to make 1000 offers
round 50 allocate() took 1.672373secs to make 1000 offers
round 100 allocate() took 1.680571secs to make 1000 offers
round 150 allocate() took 1.674683secs to make 1000 offers
round 199 allocate() took 1.671525secs to make 1000 offers

With the patch (and shared resources on all agents)
---
[ RUN  ] AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/10
Using 1000 agents and 200 frameworks with resource type 1
Added 200 frameworks in 6888us
Added 1000 agents in 2.096218secs
round 0 allocate() took 1.704491secs to make 1000 offers
round 50 allocate() took 1.718623secs to make 1000 offers
round 100 allocate() took 1.716224secs to make 1000 offers
round 150 allocate() took 1.707343secs to make 1000 offers
round 199 allocate() took 1.727467secs to make 1000 offers

With the patch (and shared resources on alternate agents)
-
[ RUN  ] AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/11
Using 1000 agents and 200 frameworks with resource type 2
Added 200 frameworks in 7304us
Added 1000 agents in 2.071009secs
round 0 allocate() took 1.689045secs to make 1000 offers
round 50 allocate() took 1.691524secs to make 1000 offers
round 100 allocate() took 1.688873secs to make 1000 offers
round 150 allocate() took 1.688713secs to make 1000 offers
round 199 allocate() took 1.691223secs to make 1000 offers

Based on HEAD, with all regular resources (no shared resources in HEAD 
supported)
-
[ RUN  ] AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/9
Using 1000 agents and 200 frameworks with resource type 0
Added 200 frameworks in 6801us
Added 1000 agents in 1.721447secs
round 0 allocate() took 1.502953secs to make 1000 offers
round 50 allocate() took 1.520157secs to make 1000 offers
round 100 allocate() took 1.517221secs to make 1000 offers
round 150 allocate() took 1.526446secs to make 1000 offers
round 199 allocate() took 1.538005secs to make 1000 offers


Thanks,

Anindya Sinha

Re: Review Request 45962: Added a persistent volume test framework for shared volumes.

2016-10-12 Thread Anindya Sinha



> On Oct. 7, 2016, 5:20 p.m., Greg Mann wrote:
> > Given how similar the code in the new example framework is to the existing 
> > persistent volumes framework, I think it could make sense to add shared 
> > volume features to the existing framework, which could be enabled/disabled 
> > via command-line flags. Especially since you add some nice new DESTROY 
> > logic in the new framework; it would be great to have that change in the 
> > existing one, and avoid duplicating a lot of code. What do you think?

Agreed. Makes sense. Will do that in the next update.


- Anindya


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


On Sept. 28, 2016, 12:15 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45962/
> ---
> 
> (Updated Sept. 28, 2016, 12:15 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4431
> https://issues.apache.org/jira/browse/MESOS-4431
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a persistent volume test framework for shared volumes.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am fba488f9d676851dd046a8b8c7dd175b3c0d9ef0 
>   src/examples/persistent_shared_volume_framework.cpp PRE-CREATION 
>   src/tests/examples_tests.cpp 52fac33733cc86dd718c7836c5031999f1597a5c 
>   src/tests/persistent_shared_volume_framework_test.sh PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/45962/diff/
> 
> 
> Testing
> ---
> 
> New test framework for shared resources added.
> Tests successful.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 52288: Recover resources when offer is rescinded on DESTROY of shared volume.

2016-10-07 Thread Anindya Sinha


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

(Updated Oct. 7, 2016, 3:12 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Removed the invalid file from the patch.


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


Repository: mesos


Description
---

When a framework issues a DESTROY of a shared volume, and that volume
is not in use by a running or a pending task, we rescind the offers
from frameworks to which the shared volume is present in pending offers
so that the volume is not assigned to any task in a future ACCEPT call.
At that time, we need to recover the resources as well for proper
accounting of such resources by the allocator.


Diffs (updated)
-

  src/master/master.cpp c7e74df71aa31edb490f4f3bd95f2d5aa94b4324 
  src/tests/persistent_volume_tests.cpp 
e10a79e9662530e143ccaa2aa2506a4d25158364 

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


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 52288: Recover resources when offer is rescinded on DESTROY of shared volume.

2016-10-06 Thread Anindya Sinha


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

(Updated Oct. 7, 2016, 6:40 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Addressed review comments.


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


Repository: mesos


Description
---

When a framework issues a DESTROY of a shared volume, and that volume
is not in use by a running or a pending task, we rescind the offers
from frameworks to which the shared volume is present in pending offers
so that the volume is not assigned to any task in a future ACCEPT call.
At that time, we need to recover the resources as well for proper
accounting of such resources by the allocator.


Diffs (updated)
-

  src/master/master.cpp c7e74df71aa31edb490f4f3bd95f2d5aa94b4324 
  src/tests/.slave_recovery_tests.cpp.swp PRE-CREATION 
  src/tests/persistent_volume_tests.cpp 
e10a79e9662530e143ccaa2aa2506a4d25158364 

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


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 52288: Recover resources when offer is rescinded on DESTROY of shared volume.

2016-10-06 Thread Anindya Sinha



> On Oct. 6, 2016, 6:09 p.m., Jiang Yan Xu wrote:
> > src/tests/persistent_volume_tests.cpp, lines 1062-1066
> > <https://reviews.apache.org/r/52288/diff/1/?file=1510563#file1510563line1062>
> >
> > It's important that the task uses only a portion of cpus/mem so the 
> > rest can be offered to framework2. Perhaps emphasize this in the comment as 
> > well?
> > 
> > The benefit of not using the the shared volume in the task is actually 
> > not obvious to me: we kill the task before destroying the volume anyways. 
> > If it's for simplicity that we don't use the volume, we should comment on 
> > the reason being so.

Yes, I did not add the shared volume for this task as it is not needed for 
verifying rescind  of offers. Added a comment regarding that.


> On Oct. 6, 2016, 6:09 p.m., Jiang Yan Xu wrote:
> > src/tests/persistent_volume_tests.cpp, lines 1068-1071
> > <https://reviews.apache.org/r/52288/diff/1/?file=1510563#file1510563line1068>
> >
> > Don't reuse the variable `offers` (even though it's reset to pending, 
> > this detail is very implicit).

Based on the pattern in the tests, the variable `offers` and `offer` are 
reused. I agree it might make it cleaner to have different variables used but I 
think we should do it as a larger refactor (if  we do it at all). For now, I 
have a `offers1` and `offers2` which represent offers from the 2 frameworks 
respectively.


> On Oct. 6, 2016, 6:09 p.m., Jiang Yan Xu wrote:
> > src/tests/persistent_volume_tests.cpp, lines 1102-1103
> > <https://reviews.apache.org/r/52288/diff/1/?file=1510563#file1510563line1102>
> >
> > Since `task` doesn't use all resources, we should expect `famework2` to 
> > receive the offer immediately right?

Updated based on the minor modification in flow.


- Anindya


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


On Sept. 26, 2016, 11:58 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52288/
> ---
> 
> (Updated Sept. 26, 2016, 11:58 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6257
> https://issues.apache.org/jira/browse/MESOS-6257
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When a framework issues a DESTROY of a shared volume, and that volume
> is not in use by a running or a pending task, we rescind the offers
> from frameworks to which the shared volume is present in pending offers
> so that the volume is not assigned to any task in a future ACCEPT call.
> At that time, we need to recover the resources as well for proper
> accounting of such resources by the allocator.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 66a672f6d16233e96b29e330a9e6c474546fa851 
>   src/tests/persistent_volume_tests.cpp 
> 2ab44d11d159162dfcac9d0791b651ed059b8164 
> 
> Diff: https://reviews.apache.org/r/52288/diff/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 52587: Allow CREATE of shared volumes based on capability of framework.

2016-10-06 Thread Anindya Sinha


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

(Updated Oct. 6, 2016, 8:16 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

When a framework issues a CREATE for a shared volume, we allow that
operation only if the framework has opted in for the capability of
SHARED_RESOURCES. However, for HTTP operator (/create-volumes), we
do not check as the operator API is not related to a specific
framework.


Diffs (updated)
-

  src/master/master.cpp 02a2fb29bdd8484fc90e5cb033ac29b49a141860 
  src/master/validation.hpp 035f721c610ae566c89a1cf0e65ff0df11679f15 
  src/master/validation.cpp e5da3c9bf0a1042b42522f1ab74ce798fbb1738d 
  src/tests/master_validation_tests.cpp 
99e350e0587e73e9ee25ef20dd369cd146bd446a 
  src/tests/persistent_volume_tests.cpp 
e10a79e9662530e143ccaa2aa2506a4d25158364 

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


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 51880: Added unit tests to determine disk size for MOUNT disks.

2016-10-05 Thread Anindya Sinha


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

(Updated Oct. 6, 2016, 2 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Added unit tests to determine disk size for MOUNT disks.


Diffs (updated)
-

  src/Makefile.am 184afb59fc42de00db85d95fff3fbc4992be7f3b 
  src/tests/containerizer/common_containerizer_tests.cpp PRE-CREATION 
  src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 

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


Testing
---

All tests including the additional tests in this RR passed.


Thanks,

Anindya Sinha

Re: Review Request 52071: Updated docs to handle resources with no size in agent flags.

2016-10-05 Thread Anindya Sinha


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

(Updated Oct. 6, 2016, 2:01 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

When a resource with no size is specified in `--resources` startup
flag of mesos agent, the size is auto detected by the agent. This
is enabled for all known resource types (except gpus).
For disk resources, this is done for both root disks as well as
MOUNT disks, but is not allowed for PATH disks since PATH disks can
be carved into smaller volumes.


Diffs (updated)
-

  docs/attributes-resources.md 2caa44927d45c0ab13f8160794b50f4fde3711aa 

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


Testing
---

Documentation change only. All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 51999: Refactor parsing of resources.

2016-10-05 Thread Anindya Sinha


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

(Updated Oct. 6, 2016, 2 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Addressed review comments, and rebased.


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


Repository: mesos


Description (updated)
---

Refactored `Resources::parse()` into 2 separate static functions:
1. Resources::fromJSON() to parse JSON representation of resources.
2. Resources::fromSimpleString() to parse text representation of
   resources.

Since these 2 new functions return a `Try>`, the
existing `Resources::parse()` implicitly converts that to a
`Resources` object. This refactor is done to retrieve all resources
(include empty resources) required for auto detection of root
and MOUNT disks.


Diffs (updated)
-

  include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
  include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
  src/common/resources.cpp 0774ff0669e831494d5b12b88e19dfa0a4a3f757 
  src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
  src/v1/resources.cpp 62a644ebbd13cfc0862bd118ba16c43e0f6aaf90 

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


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 51879: Autodetect value of resource when not specified in static resources.

2016-10-05 Thread Anindya Sinha


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

(Updated Oct. 6, 2016, 2 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Updates based on previous changes in this chain, and rebased.


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


Repository: mesos


Description
---

When static resources indicate resources with a positive size, we use
that for the resources on the agent. However, --resources can include
resources with no size, which indicates that mesos agent determine the
size of those resources from the agent and uses that information. For
disk resources, this is not allowed for PATH disks since PATH disks
can be carved into smaller chunks and we cannot assume that the whole
physical disk is available to the PATH disk.

With this change, JSON or textual representation for disk resources
that specify scalar value of 0 would not result in an error, but those
resources will not be accounted for until a valid size is determined
for such resources. A scalar value of -1 in JSON or textual formats
still results in an invalid resource.


Diffs (updated)
-

  include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
  include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
  src/common/resources.cpp 0774ff0669e831494d5b12b88e19dfa0a4a3f757 
  src/slave/containerizer/containerizer.cpp 
d46882baa904fd439bffb23c324828b777228f1c 
  src/slave/containerizer/mesos/isolators/gpu/allocator.hpp 
b2eabfebef99ccebef427d144bb816adc0175ecf 
  src/slave/containerizer/mesos/isolators/gpu/allocator.cpp 
2e722691475c84afae14009014ea70cc0fdd0e65 
  src/v1/resources.cpp 62a644ebbd13cfc0862bd118ba16c43e0f6aaf90 

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


Testing
---

Tests passed.


Thanks,

Anindya Sinha

Re: Review Request 52002: Added helper methods to determine types of disk resources.

2016-10-05 Thread Anindya Sinha


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

(Updated Oct. 6, 2016, 2 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Added helper methods to determine types of disk resources.


Diffs (updated)
-

  include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
  include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
  src/common/resources.cpp 0774ff0669e831494d5b12b88e19dfa0a4a3f757 
  src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
  src/v1/resources.cpp 62a644ebbd13cfc0862bd118ba16c43e0f6aaf90 

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


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Review Request 52587: Allow CREATE of shared volumes based on capability of framework.

2016-10-05 Thread Anindya Sinha


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

Review request for mesos and Jiang Yan Xu.


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


Repository: mesos


Description
---

When a framework issues a CREATE for a shared volume, we allow that
operation only if the framework has opted in for the capability of
SHARED_RESOURCES. However, for HTTP operator (/create-volumes), we
do not check as the operator API is not related to a specific
framework.


Diffs
-

  src/master/master.cpp 02a2fb29bdd8484fc90e5cb033ac29b49a141860 
  src/master/validation.hpp 035f721c610ae566c89a1cf0e65ff0df11679f15 
  src/master/validation.cpp e5da3c9bf0a1042b42522f1ab74ce798fbb1738d 
  src/tests/master_validation_tests.cpp 
99e350e0587e73e9ee25ef20dd369cd146bd446a 
  src/tests/persistent_volume_tests.cpp 
e10a79e9662530e143ccaa2aa2506a4d25158364 

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


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 51880: Added unit tests to determine disk size for MOUNT disks.

2016-10-03 Thread Anindya Sinha


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

(Updated Oct. 3, 2016, 11:46 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Added unit tests to determine disk size for MOUNT disks.


Diffs (updated)
-

  src/Makefile.am c897d863bde284de41c99ed50ccbbdfd2dcad23b 
  src/tests/containerizer/common_containerizer_tests.cpp PRE-CREATION 
  src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 

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


Testing
---

All tests including the additional tests in this RR passed.


Thanks,

Anindya Sinha

Re: Review Request 52071: Updated docs to handle resources with no size in agent flags.

2016-10-03 Thread Anindya Sinha


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

(Updated Oct. 3, 2016, 11:46 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

When a resource with no size is specified in `--resources` startup
flag of mesos agent, the size is auto detected by the agent. This
is enabled for all known resource types (except gpus).
For disk resources, this is done for both root disks as well as
MOUNT disks, but is not allowed for PATH disks since PATH disks can
be carved into smaller volumes.


Diffs (updated)
-

  docs/attributes-resources.md 2caa44927d45c0ab13f8160794b50f4fde3711aa 

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


Testing
---

Documentation change only. All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 51879: Autodetect value of resource when not specified in static resources.

2016-10-03 Thread Anindya Sinha


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

(Updated Oct. 3, 2016, 11:46 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

When static resources indicate resources with a positive size, we use
that for the resources on the agent. However, --resources can include
resources with no size, which indicates that mesos agent determine the
size of those resources from the agent and uses that information. For
disk resources, this is not allowed for PATH disks since PATH disks
can be carved into smaller chunks and we cannot assume that the whole
physical disk is available to the PATH disk.

With this change, JSON or textual representation for disk resources
that specify scalar value of 0 would not result in an error, but those
resources will not be accounted for until a valid size is determined
for such resources. A scalar value of -1 in JSON or textual formats
still results in an invalid resource.


Diffs (updated)
-

  include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
  include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
  src/common/resources.cpp 0774ff0669e831494d5b12b88e19dfa0a4a3f757 
  src/slave/containerizer/containerizer.cpp 
d46882baa904fd439bffb23c324828b777228f1c 
  src/slave/containerizer/mesos/isolators/gpu/allocator.hpp 
b2eabfebef99ccebef427d144bb816adc0175ecf 
  src/slave/containerizer/mesos/isolators/gpu/allocator.cpp 
2e722691475c84afae14009014ea70cc0fdd0e65 
  src/v1/resources.cpp 62a644ebbd13cfc0862bd118ba16c43e0f6aaf90 

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


Testing
---

Tests passed.


Thanks,

Anindya Sinha

Re: Review Request 52002: Added helper methods to determine types of disk resources.

2016-10-03 Thread Anindya Sinha


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

(Updated Oct. 3, 2016, 11:46 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Addressed review comments, and rebased.


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


Repository: mesos


Description
---

Added helper methods to determine types of disk resources.


Diffs (updated)
-

  include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
  include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
  src/common/resources.cpp 0774ff0669e831494d5b12b88e19dfa0a4a3f757 
  src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
  src/v1/resources.cpp 62a644ebbd13cfc0862bd118ba16c43e0f6aaf90 

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


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 52002: Added helper methods to determine types of disk resources.

2016-10-03 Thread Anindya Sinha



> On Oct. 2, 2016, 3:14 a.m., Guangya Liu wrote:
> > src/tests/resources_tests.cpp, line 2182
> > <https://reviews.apache.org/r/52002/diff/6/?file=1513455#file1513455line2182>
> >
> > Here we should keep 4 space start from the `(` above or else you can:
> > 
> > ```
> > ASSERT_TRUE(
> > Resources::isRootDisk(
> > createDiskResource("100", "role1", None(), None(;
> > ```
> > 
> > Ditto for others which has two `(` in one line.

Can you point to the specific style from the style guide 
(http://mesos.apache.org/documentation/latest/c++-style-guide/) which is being 
violated here?
I agree your option would satisfy the style guide but am curious why do you 
think the current version violates it. AFAICT, there is no specific rule for 2 
"(" in the same line.


- Anindya


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


On Sept. 28, 2016, 7:24 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52002/
> ---
> 
> (Updated Sept. 28, 2016, 7:24 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added helper methods to determine types of disk resources.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
>   include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
>   src/common/resources.cpp 0774ff0669e831494d5b12b88e19dfa0a4a3f757 
>   src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
>   src/v1/resources.cpp 62a644ebbd13cfc0862bd118ba16c43e0f6aaf90 
> 
> Diff: https://reviews.apache.org/r/52002/diff/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 51999: Refactor parsing of resources.

2016-10-03 Thread Anindya Sinha


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

(Updated Oct. 3, 2016, 11:46 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Addresed review comments, and rebased.


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


Repository: mesos


Description
---

Refactored `Resources::parse()` into 2 separate static functions:
1. Resources::fromJSONString() to parse JSON representation of
   resources.
2. Resources::fromSimpleString() to parse text representation of
   resources.

Since these 2 new functions return a `Try>`, the
existing `Resources::parse()` implicitly converts that to a
`Resources` object. This refactor is done to retrieve all resources
(include empty resources) required for auto detection of root
and MOUNT disks.


Diffs (updated)
-

  include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
  include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
  src/common/resources.cpp 0774ff0669e831494d5b12b88e19dfa0a4a3f757 
  src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
  src/v1/resources.cpp 62a644ebbd13cfc0862bd118ba16c43e0f6aaf90 

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


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 51999: Refactor parsing of resources.

2016-10-03 Thread Anindya Sinha

> On Oct. 2, 2016, 1:56 a.m., Guangya Liu wrote:
> > src/common/resources.cpp, line 587
> > <https://reviews.apache.org/r/51999/diff/6/?file=1513449#file1513449line587>
> >
> > I think it is not good to update here in this patch as I cannot see the 
> > reason why do we want to update here in this patch, can we move this update 
> > to the patch where it is needed?

I think the change is appropriate here since the calling functions now return 
`Try>`.

> On Oct. 2, 2016, 1:56 a.m., Guangya Liu wrote:
> > src/common/resources.cpp, line 645
> > <https://reviews.apache.org/r/51999/diff/6/?file=1513449#file1513449line645>
> >
> > As the API `fromJSONString(fromJSONArry as I proposed)` will also be 
> > called by agent to parse agent resource flags, so here may not

The `Resources::validate()` call is moved to `Resources::parse()` and also 
added in the private function in containerizer.cpp in 
https://reviews.apache.org/r/51879/ (since that patch is where we introduced 
auto detection if value is not specified). But I moved that change now in this 
patch to follow the chain easily.

> On Oct. 2, 2016, 1:56 a.m., Guangya Liu wrote:
> > src/common/resources.cpp, line 652
> > <https://reviews.apache.org/r/51999/diff/6/?file=1513449#file1513449line652>
> >
> > As the `fromJSONString` (I prefer we rename this as fromJSONArray) will 
> > also be called in containerizer to parse resources from agent flag, so here 
> > we also need to call `validateCommandLineResources` to make sure there is 
> > no persistent volume, revocable resources etc.
> > 
> > Also if we add the `validateCommandLineResources` here we can remove it 
> > from #553 to #556.

I would keep the call to `internal::validateCommandLineResources()` from the 
top level function. The 2 functions `fromJSONArray()` and `fromSimpleString()` 
returns an unvalidated `Try>` (also added that in the header 
of the respective function definition in resources.hpp), and the top level 
function needs to ensure it is valid in its own context by calling:
(1) `Resources::validate()` for generic validation of each `Resource` object.
(2) `internal::validateCommandLineResources()` to take care of persistent 
volumes, dynamic reservations and so on.

The top level functions are `Resources::parse()` as in this patch and from the 
private static function in containerizer.cpp (`parse()` which is added in 
https://reviews.apache.org/r/51879/ which handles parsing agent 
`flags.resources`.

> On Oct. 2, 2016, 1:56 a.m., Guangya Liu wrote:
> > src/common/resources.cpp, lines 697-698
> > <https://reviews.apache.org/r/51999/diff/6/?file=1513449#file1513449line697>
> >
> > I think we do not need validate here as here the `Resource` is get from 
> > `name, value, role`.

Agreed. See my previous response for why it was the way it is. However, I moved 
that change now in this patch to follow the chain easily.

> On Oct. 2, 2016, 1:56 a.m., Guangya Liu wrote:
> > src/common/resources.cpp, line 708
> > <https://reviews.apache.org/r/51999/diff/6/?file=1513449#file1513449line708>
> >
> > Seems we still need `validateCommandLineResources` here as 
> > `fromSimpleString` will also be called in containerizer when parsing 
> > resources from agent flag, we should make sure there is no persistent 
> > volume, revocable etc.
> > 
> > After add `validateCommandLineResources` here, we can remove the 
> > validation from #553 to #556

As indicated before (and specified in the function signature in the header 
files), the functions `fromJSONArray()` and `fromSimpleString()` return an 
unvalidated `Try>` which is validated by calling 
`Resources::validate()` and `internal::validateCommandLineResources()` from the 
top level functions, viz. `Resources::parse()` [in this patch], and from the 
private static function in containerizer.cpp (parse() which is added in 
https://reviews.apache.org/r/51879/.

- Anindya

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

On Sept. 28, 2016, 7:24 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51999/
> ---
> 
> (Updated Sept. 28, 2016, 7:24 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Descripti

Re: Review Request 51999: Refactor parsing of resources.

2016-09-28 Thread Anindya Sinha



> On Sept. 27, 2016, 5:35 p.m., Jiang Yan Xu wrote:
> > include/mesos/resources.hpp, lines 195-196
> > <https://reviews.apache.org/r/51999/diff/5/?file=1510281#file1510281line195>
> >
> > We should probably add a note below
> > 
> > ```
> > The returned vector of Resource objects haven't gone through 
> > validations such as `Resources::validate(resource)`.
> > ```
> > 
> > Same for `fromSimpleString()`.

I added this in https://reviews.apache.org/r/51879/ since that is where the 
change regarding validation lies.


- Anindya


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


On Sept. 28, 2016, 7:24 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51999/
> ---
> 
> (Updated Sept. 28, 2016, 7:24 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored `Resources::parse()` into 2 separate static functions:
> 1. Resources::fromJSONString() to parse JSON representation of
>resources.
> 2. Resources::fromSimpleString() to parse text representation of
>resources.
> 
> Since these 2 new functions return a `Try>`, the
> existing `Resources::parse()` implicitly converts that to a
> `Resources` object. This refactor is done to retrieve all resources
> (include empty resources) required for auto detection of root
> and MOUNT disks.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
>   include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
>   src/common/resources.cpp 0774ff0669e831494d5b12b88e19dfa0a4a3f757 
>   src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
>   src/v1/resources.cpp 62a644ebbd13cfc0862bd118ba16c43e0f6aaf90 
> 
> Diff: https://reviews.apache.org/r/51999/diff/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 52071: Updated docs to handle resources with no size in agent flags.

2016-09-28 Thread Anindya Sinha


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

(Updated Sept. 28, 2016, 7:25 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

When a resource with no size is specified in `--resources` startup
flag of mesos agent, the size is auto detected by the agent. This
is enabled for all known resource types (except gpus).
For disk resources, this is done for both root disks as well as
MOUNT disks, but is not allowed for PATH disks since PATH disks can
be carved into smaller volumes.


Diffs (updated)
-

  docs/attributes-resources.md 2caa44927d45c0ab13f8160794b50f4fde3711aa 

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


Testing
---

Documentation change only. All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 51880: Added unit tests to determine disk size for MOUNT disks.

2016-09-28 Thread Anindya Sinha


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

(Updated Sept. 28, 2016, 7:25 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Added unit tests to determine disk size for MOUNT disks.


Diffs (updated)
-

  src/Makefile.am fba488f9d676851dd046a8b8c7dd175b3c0d9ef0 
  src/tests/containerizer/common_containerizer_tests.cpp PRE-CREATION 
  src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 

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


Testing
---

All tests including the additional tests in this RR passed.


Thanks,

Anindya Sinha

Re: Review Request 51999: Refactor parsing of resources.

2016-09-28 Thread Anindya Sinha


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

(Updated Sept. 28, 2016, 7:24 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Addressed review comments.


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


Repository: mesos


Description
---

Refactored `Resources::parse()` into 2 separate static functions:
1. Resources::fromJSONString() to parse JSON representation of
   resources.
2. Resources::fromSimpleString() to parse text representation of
   resources.

Since these 2 new functions return a `Try>`, the
existing `Resources::parse()` implicitly converts that to a
`Resources` object. This refactor is done to retrieve all resources
(include empty resources) required for auto detection of root
and MOUNT disks.


Diffs (updated)
-

  include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
  include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
  src/common/resources.cpp 0774ff0669e831494d5b12b88e19dfa0a4a3f757 
  src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
  src/v1/resources.cpp 62a644ebbd13cfc0862bd118ba16c43e0f6aaf90 

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


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 52002: Added helper methods to determine types of disk resources.

2016-09-28 Thread Anindya Sinha


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

(Updated Sept. 28, 2016, 7:24 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Added helper methods to determine types of disk resources.


Diffs (updated)
-

  include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
  include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
  src/common/resources.cpp 0774ff0669e831494d5b12b88e19dfa0a4a3f757 
  src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
  src/v1/resources.cpp 62a644ebbd13cfc0862bd118ba16c43e0f6aaf90 

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


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 51879: Autodetect value of resource when not specified in static resources.

2016-09-28 Thread Anindya Sinha


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

(Updated Sept. 28, 2016, 7:25 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Modified based on review comments in previous commits in this chain.


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


Repository: mesos


Description
---

When static resources indicate resources with a positive size, we use
that for the resources on the agent. However, --resources can include
resources with no size, which indicates that mesos agent determine the
size of those resources from the agent and uses that information. For
disk resources, this is not allowed for PATH disks since PATH disks
can be carved into smaller chunks and we cannot assume that the whole
physical disk is available to the PATH disk.

With this change, JSON or textual representation for disk resources
that specify scalar value of 0 would not result in an error, but those
resources will not be accounted for until a valid size is determined
for such resources. A scalar value of -1 in JSON or textual formats
still results in an invalid resource.


Diffs (updated)
-

  include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
  include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
  src/common/resources.cpp 0774ff0669e831494d5b12b88e19dfa0a4a3f757 
  src/slave/containerizer/containerizer.cpp 
d46882baa904fd439bffb23c324828b777228f1c 
  src/slave/containerizer/mesos/isolators/gpu/allocator.hpp 
b2eabfebef99ccebef427d144bb816adc0175ecf 
  src/slave/containerizer/mesos/isolators/gpu/allocator.cpp 
c1a87e9e5c07529bc1d077f68477108a40506806 
  src/v1/resources.cpp 62a644ebbd13cfc0862bd118ba16c43e0f6aaf90 

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


Testing
---

Tests passed.


Thanks,

Anindya Sinha

Re: Review Request 51999: Refactor parsing of resources.

2016-09-28 Thread Anindya Sinha

NString()`, that is an error]; and assume it is text and process in 
`fromSimpleString()` if JSON conversion fails.

@gyliu: 
(1) This patch still has the validation in individual functions so validation 
in `Resources::parse()` is not needed but in  
https://reviews.apache.org/r/51879/, we move the validation out of the 
individual functions and consolidate into `Resources::parse()` [since we can 
have `Resource` with no value].
(2) Using `result.add()` now.


> On Sept. 27, 2016, 5:28 p.m., Jiang Yan Xu wrote:
> > src/tests/resources_tests.cpp, lines 630-657
> > <https://reviews.apache.org/r/51999/diff/5/?file=1510284#file1510284line630>
> >
> > With out refactor, the result of `Resources::parse()` doesn't change 
> > right?
> 
> Jiang Yan Xu wrote:
> s/out/our/

Im original code: If a resource in text format is encountered with value of 0, 
it is dropped; but in JSON, it flags an error. This patch makes the behavior 
same, ie. just drop if a `Resource` has a value of 0. Hence, this test would 
fail for value of 0. So I modified this test to test for negative values which 
is obviously an Error.


> On Sept. 27, 2016, 5:28 p.m., Jiang Yan Xu wrote:
> > include/mesos/resources.hpp, lines 169-172
> > <https://reviews.apache.org/r/51999/diff/5/?file=1510281#file1510281line169>
> >
> > Now that we've moved these words to avoid duplication, we'd better 
> > refer to the place this is moved to.
> > 
> > ```
> >   /**
> >* Parses Resources from text in the form of a JSON array. If that 
> > fails, 
> >* text in the form "name(role):value;name:value;...". i.e, this 
> > method
> >* calls `fromJSONString()` or `fromSimpleString()` (see their 
> > documentation
> >* for for details) and validates the result before converting it 
> > into a 
> >* Resources object.
> >*/
> > ```

Added the last comment regarding "validates the result before converting it 
into a Resources object" in https://reviews.apache.org/r/51879/ because that is 
where we made that change.


- Anindya


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


On Sept. 26, 2016, 8:58 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51999/
> ---
> 
> (Updated Sept. 26, 2016, 8:58 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored `Resources::parse()` into 2 separate static functions:
> 1. Resources::fromJSONString() to parse JSON representation of
>resources.
> 2. Resources::fromSimpleString() to parse text representation of
>resources.
> 
> Since these 2 new functions return a `Try>`, the
> existing `Resources::parse()` implicitly converts that to a
> `Resources` object. This refactor is done to retrieve all resources
> (include empty resources) required for auto detection of root
> and MOUNT disks.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
>   include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
>   src/common/resources.cpp 0774ff0669e831494d5b12b88e19dfa0a4a3f757 
>   src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
>   src/v1/resources.cpp 62a644ebbd13cfc0862bd118ba16c43e0f6aaf90 
> 
> Diff: https://reviews.apache.org/r/51999/diff/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 49571: Added a benchmark test for allocations.

2016-09-27 Thread Anindya Sinha


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

(Updated Sept. 28, 2016, 12:16 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Allocations test has the following resource configurations:
(1) REGULAR: Offers from every slave have regular resources.
(2) SHARED: Offers from every slave include a shared resource.
(3) REGULAR: Offers from every alternate slave contain only regular
resources; and offers from every other alternate slave contains
a shared resource.

This test is parameterized based on number of agents, number of
frameworks and resource configuration.


Diffs (updated)
-

  src/tests/hierarchical_allocator_tests.cpp 
46553f6d501deb37862dd5ba48be1c114f6e6cb8 

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


Testing
---

All tests passed.

Allocations benchmark test results
==
Support of shared resources has a small impact (roughly 10%) on runtime 
performance in allocations as compared to HEAD (without shared resources). 
Also, there is no visible impact in performance when shared resources are added 
in the tests.

Following is a snapshot with 1000 agents and 200 frameworks.

With the patch (and no shared resources)

[ RUN  ] AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/9
Using 1000 agents and 200 frameworks with resource type 0
Added 200 frameworks in 6907us
Added 1000 agents in 2.057098secs
round 0 allocate() took 1.689164secs to make 1000 offers
round 50 allocate() took 1.672373secs to make 1000 offers
round 100 allocate() took 1.680571secs to make 1000 offers
round 150 allocate() took 1.674683secs to make 1000 offers
round 199 allocate() took 1.671525secs to make 1000 offers

With the patch (and shared resources on all agents)
---
[ RUN  ] AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/10
Using 1000 agents and 200 frameworks with resource type 1
Added 200 frameworks in 6888us
Added 1000 agents in 2.096218secs
round 0 allocate() took 1.704491secs to make 1000 offers
round 50 allocate() took 1.718623secs to make 1000 offers
round 100 allocate() took 1.716224secs to make 1000 offers
round 150 allocate() took 1.707343secs to make 1000 offers
round 199 allocate() took 1.727467secs to make 1000 offers

With the patch (and shared resources on alternate agents)
-
[ RUN  ] AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/11
Using 1000 agents and 200 frameworks with resource type 2
Added 200 frameworks in 7304us
Added 1000 agents in 2.071009secs
round 0 allocate() took 1.689045secs to make 1000 offers
round 50 allocate() took 1.691524secs to make 1000 offers
round 100 allocate() took 1.688873secs to make 1000 offers
round 150 allocate() took 1.688713secs to make 1000 offers
round 199 allocate() took 1.691223secs to make 1000 offers

Based on HEAD, with all regular resources (no shared resources in HEAD 
supported)
-
[ RUN  ] AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/9
Using 1000 agents and 200 frameworks with resource type 0
Added 200 frameworks in 6801us
Added 1000 agents in 1.721447secs
round 0 allocate() took 1.502953secs to make 1000 offers
round 50 allocate() took 1.520157secs to make 1000 offers
round 100 allocate() took 1.517221secs to make 1000 offers
round 150 allocate() took 1.526446secs to make 1000 offers
round 199 allocate() took 1.538005secs to make 1000 offers


Thanks,

Anindya Sinha

Re: Review Request 45967: Added documentation for shareable resources.

2016-09-27 Thread Anindya Sinha


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

(Updated Sept. 28, 2016, 12:16 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Added documentation for shareable resources.


Diffs (updated)
-

  docs/home.md ad59eb1722b49cd72454795c095ddbd1ec1eb4dd 
  docs/shared-resources.md PRE-CREATION 

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


Testing
---

Tests successful.


Thanks,

Anindya Sinha

Re: Review Request 45963: Allow tasks to set persistent volume as readonly or readwrite resource.

2016-09-27 Thread Anindya Sinha


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

(Updated Sept. 28, 2016, 12:15 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Allow the task to specify the persistent volume access to be read-only
or read-write. Note that the persistent volume is always created as
read-write.
If the task is the first consumer of the shared persistent volume, then
set the ownership of the persistent volume to match that of the task.
Otherwise, allow the task to be executed only if the ownership of the
persistent volume matches that of the task.
Added an option to run the test in mixed (default) mode or shared-only
mode. In mixed mode, multiple shards alternate between shared and
unshared persistent volumes for the tasks. In shared-only mode, all
shards use shared persistent volumes for the tasks.


Diffs (updated)
-

  src/examples/persistent_shared_volume_framework.cpp PRE-CREATION 
  src/master/validation.hpp 035f721c610ae566c89a1cf0e65ff0df11679f15 
  src/master/validation.cpp e5da3c9bf0a1042b42522f1ab74ce798fbb1738d 
  src/slave/containerizer/mesos/isolators/filesystem/linux.hpp 
0a85935550e36c9142d845465cfa70a1634a647a 
  src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
ea418252956c8089acc5a491888ed7f6df6cafcd 
  src/slave/containerizer/mesos/isolators/filesystem/posix.hpp 
794b6e5990db5f8eb21a6535872f284ca02e0553 
  src/slave/containerizer/mesos/isolators/filesystem/posix.cpp 
af427c6e5691f1770ab3ebef79502eb2c2176c4a 

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


Testing
---

Tests successful.


Thanks,

Anindya Sinha

Re: Review Request 45962: Added a persistent volume test framework for shared volumes.

2016-09-27 Thread Anindya Sinha


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

(Updated Sept. 28, 2016, 12:15 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Added a persistent volume test framework for shared volumes.


Diffs (updated)
-

  src/Makefile.am fba488f9d676851dd046a8b8c7dd175b3c0d9ef0 
  src/examples/persistent_shared_volume_framework.cpp PRE-CREATION 
  src/tests/examples_tests.cpp 52fac33733cc86dd718c7836c5031999f1597a5c 
  src/tests/persistent_shared_volume_framework_test.sh PRE-CREATION 

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


Testing
---

New test framework for shared resources added.
Tests successful.


Thanks,

Anindya Sinha

Review Request 52295: Added additional unit tests for shared resources.

2016-09-26 Thread Anindya Sinha


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

Review request for mesos and Jiang Yan Xu.


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


Repository: mesos


Description
---

Tests include the following:
- Allocator tests when dealing with multiple frameworks of the same
  role using the same shared volume at the same time.
- Validity of DESTROY shared volume when there are pending tasks
  that have been assigned that shared volume.
- Handlng of master failover.
- Sorter tests covering shared resources.


Diffs
-

  src/tests/hierarchical_allocator_tests.cpp 
46553f6d501deb37862dd5ba48be1c114f6e6cb8 
  src/tests/master_validation_tests.cpp 
16c5773aa44016f923e00cb348ded6b8c46d4b4b 
  src/tests/persistent_volume_tests.cpp 
2ab44d11d159162dfcac9d0791b651ed059b8164 
  src/tests/sorter_tests.cpp 1f17c011898836ea9159661dde7d544cb0d8ea83 

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


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Review Request 52288: Recover resources when offer is rescinded on DESTROY of shared volume.

2016-09-26 Thread Anindya Sinha


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

Review request for mesos and Jiang Yan Xu.


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


Repository: mesos


Description
---

When a framework issues a DESTROY of a shared volume, and that volume
is not in use by a running or a pending task, we rescind the offers
from frameworks to which the shared volume is present in pending offers
so that the volume is not assigned to any task in a future ACCEPT call.
At that time, we need to recover the resources as well for proper
accounting of such resources by the allocator.


Diffs
-

  src/master/master.cpp 66a672f6d16233e96b29e330a9e6c474546fa851 
  src/tests/persistent_volume_tests.cpp 
2ab44d11d159162dfcac9d0791b651ed059b8164 

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


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 49571: Added a benchmark test for allocations.

2016-09-26 Thread Anindya Sinha


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

(Updated Sept. 26, 2016, 11:50 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Allocations test has the following resource configurations:
(1) REGULAR: Offers from every slave have regular resources.
(2) SHARED: Offers from every slave include a shared resource.
(3) REGULAR: Offers from every alternate slave contain only regular
resources; and offers from every other alternate slave contains
a shared resource.

This test is parameterized based on number of agents, number of
frameworks and resource configuration.


Diffs (updated)
-

  src/tests/hierarchical_allocator_tests.cpp 
46553f6d501deb37862dd5ba48be1c114f6e6cb8 

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


Testing
---

All tests passed.

Allocations benchmark test results
==
Support of shared resources has a small impact (roughly 10%) on runtime 
performance in allocations as compared to HEAD (without shared resources). 
Also, there is no visible impact in performance when shared resources are added 
in the tests.

Following is a snapshot with 1000 agents and 200 frameworks.

With the patch (and no shared resources)

[ RUN  ] AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/9
Using 1000 agents and 200 frameworks with resource type 0
Added 200 frameworks in 6907us
Added 1000 agents in 2.057098secs
round 0 allocate() took 1.689164secs to make 1000 offers
round 50 allocate() took 1.672373secs to make 1000 offers
round 100 allocate() took 1.680571secs to make 1000 offers
round 150 allocate() took 1.674683secs to make 1000 offers
round 199 allocate() took 1.671525secs to make 1000 offers

With the patch (and shared resources on all agents)
---
[ RUN  ] AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/10
Using 1000 agents and 200 frameworks with resource type 1
Added 200 frameworks in 6888us
Added 1000 agents in 2.096218secs
round 0 allocate() took 1.704491secs to make 1000 offers
round 50 allocate() took 1.718623secs to make 1000 offers
round 100 allocate() took 1.716224secs to make 1000 offers
round 150 allocate() took 1.707343secs to make 1000 offers
round 199 allocate() took 1.727467secs to make 1000 offers

With the patch (and shared resources on alternate agents)
-
[ RUN  ] AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/11
Using 1000 agents and 200 frameworks with resource type 2
Added 200 frameworks in 7304us
Added 1000 agents in 2.071009secs
round 0 allocate() took 1.689045secs to make 1000 offers
round 50 allocate() took 1.691524secs to make 1000 offers
round 100 allocate() took 1.688873secs to make 1000 offers
round 150 allocate() took 1.688713secs to make 1000 offers
round 199 allocate() took 1.691223secs to make 1000 offers

Based on HEAD, with all regular resources (no shared resources in HEAD 
supported)
-
[ RUN  ] AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/9
Using 1000 agents and 200 frameworks with resource type 0
Added 200 frameworks in 6801us
Added 1000 agents in 1.721447secs
round 0 allocate() took 1.502953secs to make 1000 offers
round 50 allocate() took 1.520157secs to make 1000 offers
round 100 allocate() took 1.517221secs to make 1000 offers
round 150 allocate() took 1.526446secs to make 1000 offers
round 199 allocate() took 1.538005secs to make 1000 offers


Thanks,

Anindya Sinha

Re: Review Request 45967: Added documentation for shareable resources.

2016-09-26 Thread Anindya Sinha


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

(Updated Sept. 26, 2016, 11:50 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Added documentation for shareable resources.


Diffs (updated)
-

  docs/home.md ad59eb1722b49cd72454795c095ddbd1ec1eb4dd 
  docs/shared-resources.md PRE-CREATION 

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


Testing
---

Tests successful.


Thanks,

Anindya Sinha

Re: Review Request 45963: Allow tasks to set persistent volume as readonly or readwrite resource.

2016-09-26 Thread Anindya Sinha


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

(Updated Sept. 26, 2016, 11:50 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Allow the task to specify the persistent volume access to be read-only
or read-write. Note that the persistent volume is always created as
read-write.
If the task is the first consumer of the shared persistent volume, then
set the ownership of the persistent volume to match that of the task.
Otherwise, allow the task to be executed only if the ownership of the
persistent volume matches that of the task.
Added an option to run the test in mixed (default) mode or shared-only
mode. In mixed mode, multiple shards alternate between shared and
unshared persistent volumes for the tasks. In shared-only mode, all
shards use shared persistent volumes for the tasks.


Diffs (updated)
-

  src/examples/persistent_shared_volume_framework.cpp PRE-CREATION 
  src/master/validation.hpp 035f721c610ae566c89a1cf0e65ff0df11679f15 
  src/master/validation.cpp aa743d730bebebc0353fa0a7a31f68bc94e7e25d 
  src/slave/containerizer/mesos/isolators/filesystem/linux.hpp 
0a85935550e36c9142d845465cfa70a1634a647a 
  src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
ea418252956c8089acc5a491888ed7f6df6cafcd 
  src/slave/containerizer/mesos/isolators/filesystem/posix.hpp 
794b6e5990db5f8eb21a6535872f284ca02e0553 
  src/slave/containerizer/mesos/isolators/filesystem/posix.cpp 
af427c6e5691f1770ab3ebef79502eb2c2176c4a 

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


Testing
---

Tests successful.


Thanks,

Anindya Sinha

Re: Review Request 45962: Added a persistent volume test framework for shared volumes.

2016-09-26 Thread Anindya Sinha


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

(Updated Sept. 26, 2016, 11:49 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Added MESOS_RUNTIME_DIR to point to an existing directory for this test 
framework to run.


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


Repository: mesos


Description
---

Added a persistent volume test framework for shared volumes.


Diffs (updated)
-

  src/Makefile.am 410b5f2e5bc50a5aa7856645c8cf4a3e43505a84 
  src/examples/persistent_shared_volume_framework.cpp PRE-CREATION 
  src/tests/examples_tests.cpp 52fac33733cc86dd718c7836c5031999f1597a5c 
  src/tests/persistent_shared_volume_framework_test.sh PRE-CREATION 

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


Testing
---

New test framework for shared resources added.
Tests successful.


Thanks,

Anindya Sinha

Re: Review Request 52058: Fixed fetcher to not call chown on sandbox again.

2016-09-26 Thread Anindya Sinha


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




src/slave/containerizer/fetcher.cpp (line 765)
<https://reviews.apache.org/r/52058/#comment218472>

Add a line here.



src/slave/containerizer/fetcher.cpp (line 770)
<https://reviews.apache.org/r/52058/#comment218470>

I think this should be more like:
```
if (chownOut.isError() || chownErr.isError()) {
  os::close(out.get());
  os::close(err.get());
  return Failure(...);
}
```

And the `Failure` message can indicate which one failed, ie. `stdout` or 
`stderr` or both (if you want to expose that bit of information).



src/slave/containerizer/fetcher.cpp (line 772)
<https://reviews.apache.org/r/52058/#comment218467>

If there is an error in either `chownOut` or `chownErr`, there is a 
`os::close(out.get())` and then we return `Failure`... which means we never get 
to do a `os::close(err.get())`. So, a potential leak in file descriptor!



src/slave/containerizer/fetcher.cpp (line 774)
<https://reviews.apache.org/r/52058/#comment218468>

Also, the `Failure` message indicates the failure is with `stdout` which 
may not be true if only `chownErr.isError()` is true.



src/slave/containerizer/fetcher.cpp (line 779)
<https://reviews.apache.org/r/52058/#comment218471>

Kill this block if you consolidate the error handling for `chownOut` and 
`chownErr`.


- Anindya Sinha


On Sept. 20, 2016, 11:32 p.m., Megha Sharma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52058/
> ---
> 
> (Updated Sept. 20, 2016, 11:32 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-5218
> https://issues.apache.org/jira/browse/MESOS-5218
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fetcher code changes the ownership of entire sandbox directory
> recursively to the taskuser and as a resut also changes the
> ownership of files laid out by other entities in the sandbox.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/fetcher.cpp 
> 4d3fc521bf8a7c438c48e31c549f108ac3602b3f 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 96e24500a12825161553eb050da389088b122695 
> 
> Diff: https://reviews.apache.org/r/52058/diff/
> 
> 
> Testing
> ---
> 
> make check on linux
> 
> 
> Thanks,
> 
> Megha Sharma
> 
>

Re: Review Request 52071: Updated docs to handle resources with no size in agent flags.

2016-09-26 Thread Anindya Sinha


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

(Updated Sept. 26, 2016, 9 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Fixed the commit summary, and the fix in a RR in this chain should fix the 
build issue.


Summary (updated)
-

Updated docs to handle resources with no size in agent flags.


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


Repository: mesos


Description (updated)
---

When a resource with no size is specified in `--resources` startup
flag of mesos agent, the size is auto detected by the agent. This
is enabled for all known resource types (except gpus).
For disk resources, this is done for both root disks as well as
MOUNT disks, but is not allowed for PATH disks since PATH disks can
be carved into smaller volumes.


Diffs (updated)
-

  docs/attributes-resources.md 2caa44927d45c0ab13f8160794b50f4fde3711aa 

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


Testing
---

Documentation change only. All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 51879: Autodetect value of resource when not specified in static resources.

2016-09-26 Thread Anindya Sinha


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

(Updated Sept. 26, 2016, 8:59 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Fixed the build issue on linux platform in parsing for gpus.


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


Repository: mesos


Description
---

When static resources indicate resources with a positive size, we use
that for the resources on the agent. However, --resources can include
resources with no size, which indicates that mesos agent determine the
size of those resources from the agent and uses that information. For
disk resources, this is not allowed for PATH disks since PATH disks
can be carved into smaller chunks and we cannot assume that the whole
physical disk is available to the PATH disk.

With this change, JSON or textual representation for disk resources
that specify scalar value of 0 would not result in an error, but those
resources will not be accounted for until a valid size is determined
for such resources. A scalar value of -1 in JSON or textual formats
still results in an invalid resource.


Diffs (updated)
-

  include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
  include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
  src/common/resources.cpp 0774ff0669e831494d5b12b88e19dfa0a4a3f757 
  src/slave/containerizer/containerizer.cpp 
d46882baa904fd439bffb23c324828b777228f1c 
  src/slave/containerizer/mesos/isolators/gpu/allocator.hpp 
b2eabfebef99ccebef427d144bb816adc0175ecf 
  src/slave/containerizer/mesos/isolators/gpu/allocator.cpp 
c1a87e9e5c07529bc1d077f68477108a40506806 
  src/v1/resources.cpp 62a644ebbd13cfc0862bd118ba16c43e0f6aaf90 

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


Testing
---

Tests passed.


Thanks,

Anindya Sinha

Re: Review Request 51880: Added unit tests to determine disk size for MOUNT disks.

2016-09-26 Thread Anindya Sinha


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

(Updated Sept. 26, 2016, 8:59 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Added unit tests to determine disk size for MOUNT disks.


Diffs (updated)
-

  src/Makefile.am 410b5f2e5bc50a5aa7856645c8cf4a3e43505a84 
  src/tests/containerizer/common_containerizer_tests.cpp PRE-CREATION 
  src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 

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


Testing
---

All tests including the additional tests in this RR passed.


Thanks,

Anindya Sinha

Re: Review Request 52002: Added helper methods to determine types of disk resources.

2016-09-26 Thread Anindya Sinha


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

(Updated Sept. 26, 2016, 8:58 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Added helper methods to determine types of disk resources.


Diffs (updated)
-

  include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
  include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
  src/common/resources.cpp 0774ff0669e831494d5b12b88e19dfa0a4a3f757 
  src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
  src/v1/resources.cpp 62a644ebbd13cfc0862bd118ba16c43e0f6aaf90 

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


Testing (updated)
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 51999: Refactor parsing of resources.

2016-09-26 Thread Anindya Sinha


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

(Updated Sept. 26, 2016, 8:58 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Refactored `Resources::parse()` into 2 separate static functions:
1. Resources::fromJSONString() to parse JSON representation of
   resources.
2. Resources::fromSimpleString() to parse text representation of
   resources.

Since these 2 new functions return a `Try>`, the
existing `Resources::parse()` implicitly converts that to a
`Resources` object. This refactor is done to retrieve all resources
(include empty resources) required for auto detection of root
and MOUNT disks.


Diffs (updated)
-

  include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
  include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
  src/common/resources.cpp 0774ff0669e831494d5b12b88e19dfa0a4a3f757 
  src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
  src/v1/resources.cpp 62a644ebbd13cfc0862bd118ba16c43e0f6aaf90 

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


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 52071: Updated docs to reflect handling of disk resource with size of 0.

2016-09-25 Thread Anindya Sinha


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

(Updated Sept. 26, 2016, 6:52 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Updated docs.


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


Repository: mesos


Description
---

When a disk resource with size of 0 is specified in `--resources`
startup flag of mesos agent, the disk size is auto detected by the
agent. This is done for both root disks as well as MOUNT disks, but
is not allowed for PATH disks since PATH disks can be carved into
smaller volumes.


Diffs (updated)
-

  docs/attributes-resources.md 2caa44927d45c0ab13f8160794b50f4fde3711aa 

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


Testing
---

Documentation change only. All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 51880: Added unit tests to determine disk size for MOUNT disks.

2016-09-25 Thread Anindya Sinha


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

(Updated Sept. 26, 2016, 6:52 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Addressed review comments, and changes based on updates in the preceding 
changes in this review chain.


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


Repository: mesos


Description
---

Added unit tests to determine disk size for MOUNT disks.


Diffs (updated)
-

  src/Makefile.am 410b5f2e5bc50a5aa7856645c8cf4a3e43505a84 
  src/tests/containerizer/common_containerizer_tests.cpp PRE-CREATION 
  src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 

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


Testing
---

All tests including the additional tests in this RR passed.


Thanks,

Anindya Sinha

Re: Review Request 51879: Autodetect value of resource when not specified in static resources.

2016-09-25 Thread Anindya Sinha


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

(Updated Sept. 26, 2016, 6:51 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Added support for auto detection when value is not specified in 
`flags.resources` for all known resource types (except gpus).


Summary (updated)
-

Autodetect value of resource when not specified in static resources.


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


Repository: mesos


Description (updated)
---

When static resources indicate resources with a positive size, we use
that for the resources on the agent. However, --resources can include
resources with no size, which indicates that mesos agent determine the
size of those resources from the agent and uses that information. For
disk resources, this is not allowed for PATH disks since PATH disks
can be carved into smaller chunks and we cannot assume that the whole
physical disk is available to the PATH disk.

With this change, JSON or textual representation for disk resources
that specify scalar value of 0 would not result in an error, but those
resources will not be accounted for until a valid size is determined
for such resources. A scalar value of -1 in JSON or textual formats
still results in an invalid resource.


Diffs (updated)
-

  include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
  include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
  src/common/resources.cpp 0774ff0669e831494d5b12b88e19dfa0a4a3f757 
  src/slave/containerizer/containerizer.cpp 
d46882baa904fd439bffb23c324828b777228f1c 
  src/v1/resources.cpp 62a644ebbd13cfc0862bd118ba16c43e0f6aaf90 

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


Testing
---

Tests passed.


Thanks,

Anindya Sinha

Re: Review Request 51879: Determine disk size when not specified in static resources.

2016-09-25 Thread Anindya Sinha



> On Sept. 22, 2016, 6:02 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/containerizer.cpp, lines 275-280
> > <https://reviews.apache.org/r/51879/diff/4/?file=1506819#file1506819line275>
> >
> > Mulling it over, I think we can achieve the consistency of "honoring 
> > explicitly supplied values by the operator even if it's zero" by removing 
> > validation from the parsing functions except for those that return 
> > `Resources`. i.e., 
> > 
> > If `try> fromJSONString()` and `try> 
> > fromSimpleString()` do not drop `Resource` objects that couldn't pass 
> > `Resources::validate(const Resource& resource)` check, here we can identity 
> > the disk Resources that do not have the value specified and fill in the 
> > auto-detected value. Eventually we will turn them into `Resources` and the 
> > validation can happen there.
> > 
> > If we do this, then the operator can specify the mount disk without the 
> > value to have it auto-detected.
> > 
> > ```
> > {
> >   "name" : "disk",
> >   "type" : "SCALAR",
> >   "disk" : {
> > "source" : {
> >   "type" : "MOUNT",
> >   "mount" : { "root" : "/mnt/data" }
> > }
> >   }
> > }
> > ```
> > 
> > (Frankly we can auto-detect the "type" as well but allowing it to be 
> > specified is fine too)
> > 
> > 
> > This allows us to fix the issue for other kinds of resources too 
> > (MESOS-6219).

SGTM. We have the following 3 cases now for each resource type (eg. `cpus`):
1) `cpus:0` => Do not include `cpus` in agent's resources.
2) `cpus` with no value specified (indicated by `cpus:` in text format) => Ask 
agent to auto detect amount of `cpus`.
3) No `cpus` specified in the `flags.resources` => Ask agent to auto detect 
amount of `cpus`.

We should be able to extend this to all known resource types: `cpus`, `mem`, 
`disk` (root disks and PATH/MOUNT disks) and `ports`. Still need to look at 
`gpus` a little more.

And ensured all existing call sites for `Resources::parse()` have the same 
expectation as today, ie. returns a `Resources` object with no empty and 
invalid resources.


- Anindya


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


On Sept. 21, 2016, 4:15 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51879/
> ---
> 
> (Updated Sept. 21, 2016, 4:15 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When static resources indicate disks with a positive size, we use that
> for the disk resources on the agent. However, --resources can include
> disks with size of 0, which indicates that mesos agent determine the
> size of those disks from the host and uses that information instead.
> Note that this is not allowed for PATH disks since PATH disks can be
> carved into smaller chunks and we cannot assume that the whole
> physical disk is available to the PATH disk.
> 
> With this change, JSON or textual representation for disk resources
> that specify scalar value of 0 would not result in an error, but those
> resources will not be accounted for until a valid size is determined
> for such resources. A scalar value of -1 in JSON or textual formats
> still results in an invalid resource.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/containerizer.cpp 
> d46882baa904fd439bffb23c324828b777228f1c 
> 
> Diff: https://reviews.apache.org/r/51879/diff/
> 
> 
> Testing
> ---
> 
> Tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 52002: Added helper methods to determine types of disk resources.

2016-09-25 Thread Anindya Sinha


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

(Updated Sept. 26, 2016, 6:48 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Modified `isRootDisk()` to return `bool` instead of `Try`.


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


Repository: mesos


Description
---

Added helper methods to determine types of disk resources.


Diffs (updated)
-

  include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
  include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
  src/common/resources.cpp 0774ff0669e831494d5b12b88e19dfa0a4a3f757 
  src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
  src/v1/resources.cpp 62a644ebbd13cfc0862bd118ba16c43e0f6aaf90 

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


Testing
---


Thanks,

Anindya Sinha

Re: Review Request 51999: Refactor parsing of resources.

2016-09-25 Thread Anindya Sinha


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

(Updated Sept. 26, 2016, 6:46 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Updated based on review comments.


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


Repository: mesos


Description
---

Refactored `Resources::parse()` into 2 separate static functions:
1. Resources::fromJSONString() to parse JSON representation of
   resources.
2. Resources::fromSimpleString() to parse text representation of
   resources.

Since these 2 new functions return a `Try>`, the
existing `Resources::parse()` implicitly converts that to a
`Resources` object. This refactor is done to retrieve all resources
(include empty resources) required for auto detection of root
and MOUNT disks.


Diffs (updated)
-

  include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
  include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
  src/common/resources.cpp 0774ff0669e831494d5b12b88e19dfa0a4a3f757 
  src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
  src/v1/resources.cpp 62a644ebbd13cfc0862bd118ba16c43e0f6aaf90 

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


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 51999: Refactor parsing of resources.

2016-09-25 Thread Anindya Sinha



> On Sept. 22, 2016, 10:14 p.m., Guangya Liu wrote:
> > include/mesos/resources.hpp, lines 183-217
> > <https://reviews.apache.org/r/51999/diff/3/?file=1506809#file1506809line183>
> >
> > I think we do not intend to expose those two APIs, so `private` them?

We would call these two APIs from `Containerizer()` to parse `flags.resources`. 
So we cannot `private` them.


> On Sept. 22, 2016, 10:14 p.m., Guangya Liu wrote:
> > src/common/resources.cpp, line 677
> > <https://reviews.apache.org/r/51999/diff/3/?file=1506811#file1506811line677>
> >
> > I think that we should still use 
> > `internal::validateCommandLineResources` as the command line resources 
> > should not have dynamic resevations etc.
> > 
> > But there is a bug in `internal::validateCommandLineResources` now, as 
> > when I set a resource string as `cpus:45.55;ports:[1-2, 
> > 3-5];disks:{-xx}`, there will be no erros if using 
> > `internal::validateCommandLineResources` to validate.
> > 
> > So I would suggest the following:
> > 1) Continue use `internal::validateCommandLineResources` to validate 
> > resource.
> > 2) Add `role` validation in `internal::validateCommandLineResources`.

I am not sure I understand your concern. In `fromJsonString()` [actually 
`convertJson()`] and `fromSimpleString()`, we call `Resources::validate()` to 
validate each of the `Resource` objects (which captures negative valued 
resource, etc. as an Error). In addition, we call 
`internal::validateCommandLineResources` from `Resources::parse()` after 
getting the `vector` from `fromJsonString()` and `fromSimpleString()` 
to validate again the resource containing persistent volume, dynamic 
reservation, etc. So, I think we are covered on your case.

Secondly, can you please explain what you mean by 'role' validation from point 
# 2 above.


> On Sept. 22, 2016, 10:14 p.m., Guangya Liu wrote:
> > include/mesos/resources.hpp, line 200
> > <https://reviews.apache.org/r/51999/diff/3/?file=1506809#file1506809line200>
> >
> > This was only called by `Try parse` and the `Try 
> > parse` already defined the default value as `defaultRole` as star role, so 
> > we do not need to set the parameter `defaultRole` default to star role. But 
> > adding a default value may make the code more easy to understand.

Although I do not see the need to do so, I can add the default `defaultRole` 
for clarity.


> On Sept. 22, 2016, 10:14 p.m., Guangya Liu wrote:
> > src/common/resources.cpp, line 587
> > <https://reviews.apache.org/r/51999/diff/3/?file=1506811#file1506811line587>
> >
> > Why not 
> > 
> > ```
> > Resources resources;
> > ```

For this patch, I agreee that using a `Resourecs` object here would be fine. 
However, if you look at https://reviews.apache.org/r/51879/ (which is in this 
chain), we would need this to be a `vector`. So I would not modify it 
here.


- Anindya


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


On Sept. 21, 2016, 4:14 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51999/
> ---
> 
> (Updated Sept. 21, 2016, 4:14 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored `Resources::parse()` into 2 separate static functions:
> 1. Resources::fromJSONString() to parse JSON representation of
>resources.
> 2. Resources::fromSimpleString() to parse text representation of
>resources.
> 
> Since these 2 new functions return a `Try>`, the
> existing `Resources::parse()` implicitly converts that to a
> `Resources` object. This refactor is done to retrieve all resources
> (include empty resources) required for auto detection of root
> and MOUNT disks.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
>   include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
>   src/common/resources.cpp 4602bff22ec7ff77f069056085ad92bfa04595f3 
>   src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
>   src/v1/resources.cpp 7c4db44d4c31a8295cc16b0ce1ef5adf314e6ac6 
> 
> Diff: https://reviews.apache.org/r/51999/diff/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 51999: Refactor parsing of resources.

2016-09-25 Thread Anindya Sinha


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




include/mesos/resources.hpp (lines 179 - 213)
<https://reviews.apache.org/r/51999/#comment218188>

We would need to call these APIs from `Containerizer` for parsing of 
`flags.resources` so I do not think we can `private` these APIs.



src/common/resources.cpp (line 670)
<https://reviews.apache.org/r/51999/#comment218189>

I am not sure I understand your concern. In `fromJsonString()` [actually 
`convertJson()`] and `fromSimpleString()`, individual `Resource` object is 
validated. In `Resources::parse()`, we call 
`internal::validateCommandLineResources(resources)` to check for dynamic 
reservation, persistent volume, etc.

Secondly, what are you implying by `role` validation? Would you please 
elaborate?


- Anindya Sinha


On Sept. 26, 2016, 6:46 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51999/
> ---
> 
> (Updated Sept. 26, 2016, 6:46 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored `Resources::parse()` into 2 separate static functions:
> 1. Resources::fromJSONString() to parse JSON representation of
>resources.
> 2. Resources::fromSimpleString() to parse text representation of
>resources.
> 
> Since these 2 new functions return a `Try>`, the
> existing `Resources::parse()` implicitly converts that to a
> `Resources` object. This refactor is done to retrieve all resources
> (include empty resources) required for auto detection of root
> and MOUNT disks.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
>   include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
>   src/common/resources.cpp 0774ff0669e831494d5b12b88e19dfa0a4a3f757 
>   src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
>   src/v1/resources.cpp 62a644ebbd13cfc0862bd118ba16c43e0f6aaf90 
> 
> Diff: https://reviews.apache.org/r/51999/diff/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 49571: Added a benchmark test for allocations.

2016-09-21 Thread Anindya Sinha


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

(Updated Sept. 22, 2016, 12:35 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Allocations test has the following resource configurations:
(1) REGULAR: Offers from every slave have regular resources.
(2) SHARED: Offers from every slave include a shared resource.
(3) REGULAR: Offers from every alternate slave contain only regular
resources; and offers from every other alternate slave contains
a shared resource.

This test is parameterized based on number of agents, number of
frameworks and resource configuration.


Diffs (updated)
-

  src/tests/hierarchical_allocator_tests.cpp 
46553f6d501deb37862dd5ba48be1c114f6e6cb8 

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


Testing
---

All tests passed.

Allocations benchmark test results
==
Support of shared resources has a small impact (roughly 10%) on runtime 
performance in allocations as compared to HEAD (without shared resources). 
Also, there is no visible impact in performance when shared resources are added 
in the tests.

Following is a snapshot with 1000 agents and 200 frameworks.

With the patch (and no shared resources)

[ RUN  ] AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/9
Using 1000 agents and 200 frameworks with resource type 0
Added 200 frameworks in 6907us
Added 1000 agents in 2.057098secs
round 0 allocate() took 1.689164secs to make 1000 offers
round 50 allocate() took 1.672373secs to make 1000 offers
round 100 allocate() took 1.680571secs to make 1000 offers
round 150 allocate() took 1.674683secs to make 1000 offers
round 199 allocate() took 1.671525secs to make 1000 offers

With the patch (and shared resources on all agents)
---
[ RUN  ] AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/10
Using 1000 agents and 200 frameworks with resource type 1
Added 200 frameworks in 6888us
Added 1000 agents in 2.096218secs
round 0 allocate() took 1.704491secs to make 1000 offers
round 50 allocate() took 1.718623secs to make 1000 offers
round 100 allocate() took 1.716224secs to make 1000 offers
round 150 allocate() took 1.707343secs to make 1000 offers
round 199 allocate() took 1.727467secs to make 1000 offers

With the patch (and shared resources on alternate agents)
-
[ RUN  ] AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/11
Using 1000 agents and 200 frameworks with resource type 2
Added 200 frameworks in 7304us
Added 1000 agents in 2.071009secs
round 0 allocate() took 1.689045secs to make 1000 offers
round 50 allocate() took 1.691524secs to make 1000 offers
round 100 allocate() took 1.688873secs to make 1000 offers
round 150 allocate() took 1.688713secs to make 1000 offers
round 199 allocate() took 1.691223secs to make 1000 offers

Based on HEAD, with all regular resources (no shared resources in HEAD 
supported)
-
[ RUN  ] AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/9
Using 1000 agents and 200 frameworks with resource type 0
Added 200 frameworks in 6801us
Added 1000 agents in 1.721447secs
round 0 allocate() took 1.502953secs to make 1000 offers
round 50 allocate() took 1.520157secs to make 1000 offers
round 100 allocate() took 1.517221secs to make 1000 offers
round 150 allocate() took 1.526446secs to make 1000 offers
round 199 allocate() took 1.538005secs to make 1000 offers


Thanks,

Anindya Sinha

Re: Review Request 45967: Added documentation for shareable resources.

2016-09-21 Thread Anindya Sinha


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

(Updated Sept. 22, 2016, 12:35 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Added documentation for shareable resources.


Diffs (updated)
-

  docs/home.md ad59eb1722b49cd72454795c095ddbd1ec1eb4dd 
  docs/shared-resources.md PRE-CREATION 

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


Testing
---

Tests successful.


Thanks,

Anindya Sinha

Re: Review Request 45962: Added a persistent volume test framework for shared volumes.

2016-09-21 Thread Anindya Sinha


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

(Updated Sept. 22, 2016, 12:34 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Addressed comments and rebased.


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


Repository: mesos


Description
---

Added a persistent volume test framework for shared volumes.


Diffs (updated)
-

  src/Makefile.am 478fb5ae01dbdf1a735680edf7c5f30867726e07 
  src/examples/persistent_shared_volume_framework.cpp PRE-CREATION 
  src/tests/examples_tests.cpp 52fac33733cc86dd718c7836c5031999f1597a5c 
  src/tests/persistent_shared_volume_framework_test.sh PRE-CREATION 

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


Testing
---

New test framework for shared resources added.
Tests successful.


Thanks,

Anindya Sinha

Re: Review Request 45963: Allow tasks to set persistent volume as readonly or readwrite resource.

2016-09-21 Thread Anindya Sinha


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

(Updated Sept. 22, 2016, 12:35 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Allow the task to specify the persistent volume access to be read-only
or read-write. Note that the persistent volume is always created as
read-write.
If the task is the first consumer of the shared persistent volume, then
set the ownership of the persistent volume to match that of the task.
Otherwise, allow the task to be executed only if the ownership of the
persistent volume matches that of the task.
Added an option to run the test in mixed (default) mode or shared-only
mode. In mixed mode, multiple shards alternate between shared and
unshared persistent volumes for the tasks. In shared-only mode, all
shards use shared persistent volumes for the tasks.


Diffs (updated)
-

  src/examples/persistent_shared_volume_framework.cpp PRE-CREATION 
  src/master/validation.hpp 035f721c610ae566c89a1cf0e65ff0df11679f15 
  src/master/validation.cpp aa743d730bebebc0353fa0a7a31f68bc94e7e25d 
  src/slave/containerizer/mesos/isolators/filesystem/linux.hpp 
0a85935550e36c9142d845465cfa70a1634a647a 
  src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
ea418252956c8089acc5a491888ed7f6df6cafcd 
  src/slave/containerizer/mesos/isolators/filesystem/posix.hpp 
794b6e5990db5f8eb21a6535872f284ca02e0553 
  src/slave/containerizer/mesos/isolators/filesystem/posix.cpp 
af427c6e5691f1770ab3ebef79502eb2c2176c4a 

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


Testing
---

Tests successful.


Thanks,

Anindya Sinha

Re: Review Request 45962: Added a persistent volume test framework for shared volumes.

2016-09-21 Thread Anindya Sinha



> On Sept. 18, 2016, 6:35 p.m., haosdent huang wrote:
> > src/examples/persistent_shared_volume_framework.cpp, lines 207-209
> > <https://reviews.apache.org/r/45962/diff/15/?file=1498936#file1498936line207>
> >
> > Nit:
> > ```
> >   "COUNTER=0;"
> >   "while [ $COUNTER -lt 5 ];"
> >   "do"
> >   "  echo " + task_id + ":Writing from main 
> > task=$COUNTER"
> >   "  >> volume/persistent.dat;"
> >   "  COUNTER=$[COUNTER+1];"
> >   "  sleep 2;"
> >   "done");
> > ```

I do not think it adds a lot of value to change this.


> On Sept. 18, 2016, 6:35 p.m., haosdent huang wrote:
> > src/examples/persistent_shared_volume_framework.cpp, lines 249-251
> > <https://reviews.apache.org/r/45962/diff/15/?file=1498936#file1498936line249>
> >
> > Nits:
> > 
> > ```
> >   "COUNTER=0;"
> >   "while [ $COUNTER -lt 5 ];"
> >   "do"
> >   "  tail -n 1 volume/persistent.dat;"
> >   "  COUNTER=$[COUNTER+1];"
> >   "  sleep 2;"
> >   "done");
> > ```

Ditto as above.


- Anindya


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


On Sept. 15, 2016, 4:47 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45962/
> ---
> 
> (Updated Sept. 15, 2016, 4:47 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4431
> https://issues.apache.org/jira/browse/MESOS-4431
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a persistent volume test framework for shared volumes.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am f1d202ae08d6bb4fd9e11eb1eae75dd9d5d9d8d5 
>   src/examples/persistent_shared_volume_framework.cpp PRE-CREATION 
>   src/tests/examples_tests.cpp 52fac33733cc86dd718c7836c5031999f1597a5c 
>   src/tests/persistent_shared_volume_framework_test.sh PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/45962/diff/
> 
> 
> Testing
> ---
> 
> New test framework for shared resources added.
> Tests successful.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 52002: Added helper methods to determine types of disk resources.

2016-09-21 Thread Anindya Sinha



> On Sept. 20, 2016, 10:29 p.m., Jiang Yan Xu wrote:
> > include/mesos/resources.hpp, lines 276-278
> > <https://reviews.apache.org/r/52002/diff/2/?file=1504213#file1504213line276>
> >
> > Would it be simpler to just have 
> > 
> > ```
> > static bool Resources::isMountDisk(const Resource& resource);
> > static bool Resources::isPathDisk(const Resource& resource);
> > ```
> > 
> > ?
> > 
> > We have already lost enumerability (or swtichability) given that the 
> > root disk does not have a type. :)
> 
> Anindya Sinha wrote:
> I prefer this way so as to not add functions when we add more disk types, 
> say for block devices.
> 
> Jiang Yan Xu wrote:
> Yes there's block devices but I don't imagine there being too many 
> others. Otherwise I wouldn't suggest this. :)
> 
> Beyond disks we really only have a few kinds of predefined resources so I 
> think supporting them directly wouldn't break the elegance of the API.
> 
> It would have been more consistent to if ROOT is a 
> `DiskInfo::Source::Type` but otherwise having `isRootDisk` and 
> `getDiskSource()` feels inconsistent.

Exactly. If you can achieve all current and future disk source types by a 
single API, I am not sure why you would need multiple functions?
Just as an analogy, for `Resource::DiskInfo::Source`, protobuf exposes a 
function `type()` that returns one of `Resource::DiskInfo::Source::MOUNT` or 
`Resource::DiskInfo::Source::PATH`; and not functions like `isMount()` and 
`isPath()`?


> On Sept. 20, 2016, 10:29 p.m., Jiang Yan Xu wrote:
> > include/mesos/resources.hpp, lines 273-274
> > <https://reviews.apache.org/r/52002/diff/2/?file=1504213#file1504213line273>
> >
> > Does it need to be a Try? i.e., if it's not a disk, it's not a root 
> > disk, right? We have `validate()` to make sure resources are valid.
> 
> Anindya Sinha wrote:
> If `resource.name() != "disk"`, we can skip this resource altogether. But 
> if this is a `disk` but not a root disk (ie. `resource.has_disk()` is 
> `true`), then we also check for `MOUNT` or `PATH`. So by returning a 
> `Try`, we are able to decide if we should continue with this resource 
> or not. Ofcourse, we can check for `resource.has_disk()` outside of these 
> functions but adding that check makes this function self-contained.
> 
> Jiang Yan Xu wrote:
> Returning an Error on a simple predicate feels odd to me. You don't need 
> to use an error to capture that. If necessary `isDisk()` would have been 
> better.
> 
> Similar examples can be found in the same class, e.g.,:
> 
> ```
> bool isPersistentVolume(const Resource& resource);
> ```
> 
> Plus we can use `filter()` on these predicates.
> 
> Having to first check if the resource is a disk and then decide whether 
> to call getDiskSource() on the caller side or having to deal the error 
> returned by getDiskSource() sounds like to reason we'll want to hide them.
> 
> The following is very simple right?
> 
> ```
> // By now we already know it's a disk, if it is a gpu we wouldn't call 
> `isRootDisk()` even if it returns an error, right?
> 
> if (Resources::isRootDisk()) {
> ...
> } else if (Resources::isMountDisk()) {
> ...
> } else {
>   // Hypothetical TODO: Handle block devices.
>   CHECK(Resources::isPathDisk());
>   ...
> }
> ```

> On `bool isPersistentVolume(const Resource& resource);`:

I understand that the call sites from where this is called already ensures it 
is a disk resource, but if we can embed in the API, I think that is better.

> On "Having to first check if the resource is a disk and then decide whether 
> to call getDiskSource() on the caller side or having to deal the error 
> returned by getDiskSource() sounds like to reason we'll want to hide them.":

I am not sure I understand this. The caller calls `isRootDisk()` or 
`getDiskSource()`. If it gets an Error, it is an invalid resource within the 
context of the called APIs, otherwise it processes that resource. The caller 
does not need to check if the resource is a disk.

Going on the same principles, I believe verifying the resource is a disk before 
calling `isRootDisk()`, etc. is a better option. It keeps the API self 
contained. Although all the call sites would ensure that this is a disk 
resource, it does not need to if the API itself encompasses that. eg. If the 
input resource is actually a "cpus" resource, then `isRootDisk()` would return 
`true` if we do not check for the actua

Re: Review Request 52002: Added helper methods to determine types of disk resources.

2016-09-21 Thread Anindya Sinha



> On Sept. 20, 2016, 10:29 p.m., Jiang Yan Xu wrote:
> > include/mesos/resources.hpp, lines 273-274
> > <https://reviews.apache.org/r/52002/diff/2/?file=1504213#file1504213line273>
> >
> > Does it need to be a Try? i.e., if it's not a disk, it's not a root 
> > disk, right? We have `validate()` to make sure resources are valid.

If `resource.name() != "disk"`, we can skip this resource altogether. But if 
this is a `disk` but not a root disk (ie. `resource.has_disk()` is `true`), 
then we also check for `MOUNT` or `PATH`. So by returning a `Try`, we are 
able to decide if we should continue with this resource or not. Ofcourse, we 
can check for `resource.has_disk()` outside of these functions but adding that 
check makes this function self-contained.


> On Sept. 20, 2016, 10:29 p.m., Jiang Yan Xu wrote:
> > include/mesos/resources.hpp, lines 276-278
> > <https://reviews.apache.org/r/52002/diff/2/?file=1504213#file1504213line276>
> >
> > Would it be simpler to just have 
> > 
> > ```
> > static bool Resources::isMountDisk(const Resource& resource);
> > static bool Resources::isPathDisk(const Resource& resource);
> > ```
> > 
> > ?
> > 
> > We have already lost enumerability (or swtichability) given that the 
> > root disk does not have a type. :)

I prefer this way so as to not add functions when we add more disk types, say 
for block devices.


> On Sept. 20, 2016, 10:29 p.m., Jiang Yan Xu wrote:
> > src/common/resources.cpp, line 920
> > <https://reviews.apache.org/r/52002/diff/2/?file=1504215#file1504215line920>
> >
> > Not `!resource.has_disk()` but rather `!resource.disk().has_source()` 
> > right?
> > 
> > See examples below.

I think `!resource.has_disk()` is fine. Why should a root disk have 
`Persistence` and/or `Volume`? `Persistence` is to define characteristics of a 
persistent volume, where as `Volume` is how the disk resource is mounted in a 
container.


> On Sept. 20, 2016, 10:29 p.m., Jiang Yan Xu wrote:
> > src/common/resources.cpp, lines 924-943
> > <https://reviews.apache.org/r/52002/diff/2/?file=1504215#file1504215line924>
> >
> > It seems to be cleaner to implement the following instead:
> > 
> > ```
> > static bool Resources::isMountDisk(const Resource& resource)
> > {
> >   return resource.has_disk() && 
> >  resource.disk().has_source() &&
> >  resource.disk().source().type() == 
> > Resource::DiskInfo::Source::MOUNT;
> > }
> > 
> > static bool Resources::isPathDisk(const Resource& resource)
> > {
> >   return resource.has_disk() && 
> >  resource.disk().has_source() &&
> >  resource.disk().source().type() == 
> > Resource::DiskInfo::Source::PATH;
> > }
> > ```

See my response above. I think having a single function to encapsulate all 
source types is fine.


- Anindya


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


On Sept. 21, 2016, 4:14 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52002/
> ---
> 
> (Updated Sept. 21, 2016, 4:14 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added helper methods to determine types of disk resources.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
>   include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
>   src/common/resources.cpp 4602bff22ec7ff77f069056085ad92bfa04595f3 
>   src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
>   src/v1/resources.cpp 7c4db44d4c31a8295cc16b0ce1ef5adf314e6ac6 
> 
> Diff: https://reviews.apache.org/r/52002/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 52071: Updated docs to reflect handling of disk resource with size of 0.

2016-09-20 Thread Anindya Sinha


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

(Updated Sept. 21, 2016, 4:15 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

When a disk resource with size of 0 is specified in `--resources`
startup flag of mesos agent, the disk size is auto detected by the
agent. This is done for both root disks as well as MOUNT disks, but
is not allowed for PATH disks since PATH disks can be carved into
smaller volumes.


Diffs (updated)
-

  docs/attributes-resources.md 2caa44927d45c0ab13f8160794b50f4fde3711aa 

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


Testing
---

Documentation change only. All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 51879: Determine disk size when not specified in static resources.

2016-09-20 Thread Anindya Sinha


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

(Updated Sept. 21, 2016, 4:15 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Refactored based on revised APIs in resources.


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


Repository: mesos


Description
---

When static resources indicate disks with a positive size, we use that
for the disk resources on the agent. However, --resources can include
disks with size of 0, which indicates that mesos agent determine the
size of those disks from the host and uses that information instead.
Note that this is not allowed for PATH disks since PATH disks can be
carved into smaller chunks and we cannot assume that the whole
physical disk is available to the PATH disk.

With this change, JSON or textual representation for disk resources
that specify scalar value of 0 would not result in an error, but those
resources will not be accounted for until a valid size is determined
for such resources. A scalar value of -1 in JSON or textual formats
still results in an invalid resource.


Diffs (updated)
-

  src/slave/containerizer/containerizer.cpp 
d46882baa904fd439bffb23c324828b777228f1c 

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


Testing
---

Tests passed.


Thanks,

Anindya Sinha

Re: Review Request 51880: Added unit tests to determine disk size for MOUNT disks.

2016-09-20 Thread Anindya Sinha


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

(Updated Sept. 21, 2016, 4:15 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Updated based on review comments.


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


Repository: mesos


Description
---

Added unit tests to determine disk size for MOUNT disks.


Diffs (updated)
-

  src/Makefile.am 478fb5ae01dbdf1a735680edf7c5f30867726e07 
  src/tests/containerizer/common_containerizer_tests.cpp PRE-CREATION 

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


Testing
---

All tests including the additional tests in this RR passed.


Thanks,

Anindya Sinha

Re: Review Request 52002: Added helper methods to determine types of disk resources.

2016-09-20 Thread Anindya Sinha


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

(Updated Sept. 21, 2016, 4:14 a.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Added helper methods to determine types of disk resources.


Diffs (updated)
-

  include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
  include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
  src/common/resources.cpp 4602bff22ec7ff77f069056085ad92bfa04595f3 
  src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
  src/v1/resources.cpp 7c4db44d4c31a8295cc16b0ce1ef5adf314e6ac6 

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


Testing
---


Thanks,

Anindya Sinha

Re: Review Request 51999: Refactor parsing of resources to include all valid resources.

2016-09-20 Thread Anindya Sinha



> On Sept. 20, 2016, 3:43 a.m., Guangya Liu wrote:
> > include/mesos/resources.hpp, lines 187-188
> > <https://reviews.apache.org/r/51999/diff/2/?file=1504205#file1504205line187>
> >
> > Why update here, I prefer
> > 
> > ```
> > parses text in the form "name(role):value;name:value;...".
> > ```

I do not think it matters. The change was done since the previous line did not 
fit this within 80 chars.
Anyways with the refactor, this is not an issue anymore.


> On Sept. 20, 2016, 3:43 a.m., Guangya Liu wrote:
> > include/mesos/resources.hpp, line 201
> > <https://reviews.apache.org/r/51999/diff/2/?file=1504205#file1504205line201>
> >
> > I think that we should find a better name for this, the above API 
> > `parse` is actually also parsing a text.
> > 
> > What about `parseResourceVector` or else some other meaningful name in 
> > your mind?

Resolved with the refactor.


> On Sept. 20, 2016, 3:43 a.m., Guangya Liu wrote:
> > src/slave/containerizer/containerizer.cpp, line 64
> > <https://reviews.apache.org/r/51999/diff/2/?file=1504208#file1504208line64>
> >
> > I suggest that we move this to 
> > https://reviews.apache.org/r/51879/diff/3#index_header

Resolved with the refactor.


- Anindya


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


On Sept. 19, 2016, 10:42 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51999/
> ---
> 
> (Updated Sept. 19, 2016, 10:42 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> During parsing of resources in the startup flags of mesos agent, all
> the valid resources (including empty resources) are accumulated in a
> vector of Resource protobufs.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
>   include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
>   src/common/resources.cpp 4602bff22ec7ff77f069056085ad92bfa04595f3 
>   src/slave/containerizer/containerizer.cpp 
> d46882baa904fd439bffb23c324828b777228f1c 
>   src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
>   src/v1/resources.cpp 7c4db44d4c31a8295cc16b0ce1ef5adf314e6ac6 
> 
> Diff: https://reviews.apache.org/r/51999/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

< 1 2 3 4 5 6 7 8 >

301 - 400 of 761 matches

Mail list logo