Re: Review Request 54996: Fix SIGBUS crash on ARM64/AArch64.

[James Peach]

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




3rdparty/stout/include/stout/os/linux.hpp (line 60)


I don't think that you need this static constructor. You can do everything 
you need in `allocate`, which is them symmetric with `deallocate`.



3rdparty/stout/include/stout/os/linux.hpp (line 63)


I'd recommend something like this:
```
Try allocate(size_t nbytes) {
int error = ::posix_memalign(, os::getpagesize(), nbytes);
if (error) {
return ErrnoError(error);
}

return Nothing();
}
```



3rdparty/stout/include/stout/os/linux.hpp (line 75)


You can simplify this to just:
```
void deallocate() {
  ::free(address);
  
  address = nullptr;
  size = 0;
}
```



3rdparty/stout/include/stout/os/linux.hpp (line 79)


You can avoid the `reinterpret_cast` by typing this as `void *`. If you add 
the `start()` method suggested below, you can also make it private.



3rdparty/stout/include/stout/os/linux.hpp (line 82)


You should explictly delete the copy constructors here to ensure the stack 
can't be leaked if the code changes:

```
Stack(const Stack&) = delete;
Stack& operator=(const Stack&) = delete;
```



3rdparty/stout/include/stout/os/linux.hpp (line 105)


Since this magic number appears twice I'd be inclined to hoist it into 
`Stack`:
```
class Stack {
  ...
  static constexpr size_t DefaultSize = 8 * 1024 * 1024;
  ...
};
```



3rdparty/stout/include/stout/os/linux.hpp (line 118)


Consider hoisting this into the `Stack` class:

```
void * Stack::start() {
  return (uint8_t *)address + size;
}
```


- James Peach


On Jan. 4, 2017, 12:26 a.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54996/
> ---
> 
> (Updated Jan. 4, 2017, 12:26 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6835
> https://issues.apache.org/jira/browse/MESOS-6835
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently in the Linux launcher when the stack is allocated and prepared for 
> a call to clone() it is not properly aligned. This is not an issue for x86 or 
> x64 but for ARM64/AArch64 it is because of the requirement of having the 
> stack aligned to a 16 byte boundary. While x86 and x64 also expect the stack 
> to have a 16 byte aligned stack, it is not enforced. An explanation of the 
> stack and requirements for ARM64 can be found here 
> http://infocenter.arm.com/help/topic/com.arm.doc.ihi0055b/IHI0055B_aapcs64.pdf
>  (specifically section 5.2.2.1 that says SP mod 16 = 0. The stack must be 
> quad-word aligned.)
> 
> Additionally, the way that the stack is currently allocated and passed to 
> clone() accidentally chops off one entry, making a stack overflow using those 
> missing 8 bytes a possibility. Fixing this while aligning the memory will fix 
> both the issue of the stack overflow issue as well as the SIGBUS crash. We 
> should also net better performance from having the stack aligned.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/linux.hpp 530f1a55b 
>   src/linux/ns.hpp 77789717e 
> 
> Diff: https://reviews.apache.org/r/54996/diff/
> 
> 
> Testing
> ---
> 
> Built Mesos from source and am currently running it in a test cluster. 
> Launched both Docker and Mesos tasks via Marathon without any resulting crash 
> (initial crash only happened with Mesos containerizer + linux_launcher, not 
> with the posix_launcher).
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Review Request 55162: Stout: Added style fixes and some useful error messages.

[Alex Clemmer]

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

Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.


Repository: mesos


Description
---

Stout: Added style fixes and some useful error messages.


Diffs
-

  3rdparty/stout/include/stout/os/windows/killtree.hpp 
2be2f8c3d58ee64410f87ee4a4b2bb54fe014748 
  3rdparty/stout/include/stout/windows/os.hpp 
5cd92545a49648e39e8eb7cf131895e9cfc97902 

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


Testing
---


Thanks,

Alex Clemmer

Review Request 55161: Fixed typo in `stout/os/windows/write.hpp`.

[Alex Clemmer]

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

Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.


Repository: mesos


Description
---

Note that this typo is benign, as it would be highly unusual to call
`os::read` with these arguments.


Diffs
-

  3rdparty/stout/include/stout/os/windows/write.hpp 
23488708ae366b8571bb8b4805f67d2054223fff 

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


Testing
---


Thanks,

Alex Clemmer

Review Request 55160: Added test for DockerContainerizer when `cgroups_enable_cfs` is set.

[Zhitao Li]

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

Review request for mesos, Gilbert Song, haosdent huang, and Jie Yu.


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


Repository: mesos


Description
---

Added test for DockerContainerizer when `cgroups_enable_cfs` is set.


Diffs
-

  src/tests/containerizer/docker_containerizer_tests.cpp 
4e3b67bbb989f9084dfdf4970839956dcb0caa0e 

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


Testing
---

Run the new test.


Thanks,

Zhitao Li

Re: Review Request 55152: Fixed nested container docs and added a link to home.md.

[Adam B]

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



minor comments


docs/nested-container-and-task-group.md (line 2)


Maybe put "pods" in the page title, for SEO?



docs/nested-container-and-task-group.md (line 125)


Have you verified this anchor link using `site/Dockerfile` to generate the 
website, rather than just following github markdown?


- Adam B


On Jan. 3, 2017, 1:44 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55152/
> ---
> 
> (Updated Jan. 3, 2017, 1:44 p.m.)
> 
> 
> Review request for mesos, Adam B, Anand Mazumdar, Avinash sridharan, Jie Yu, 
> Neil Conway, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed nested container docs and added a link to home.md.
> 
> 
> Diffs
> -
> 
>   docs/home.md 091dae20b0a26aac03186edaefab93c54b3063b4 
>   docs/nested-container-and-task-group.md 
> debf9f82622557aff915dd04ef06a50793f496b6 
> 
> Diff: https://reviews.apache.org/r/55152/diff/
> 
> 
> Testing
> ---
> 
> https://github.com/Gilbert88/mesos/blob/doc_pod/docs/nested-container-and-task-group.md
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Review Request 55157: Fixed a bug in the default executor around not committing suicide.

[Anand Mazumdar]

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

Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

This bug is only observed when the task group contains a single task.
The default executor was not committing suicide when this single task
used to exit with a non-zero status code as per the default restart
policy.


Diffs
-

  src/launcher/default_executor.cpp fd9963996c82461b60888397989e309d51b60aaa 
  src/tests/default_executor_tests.cpp 340e8c8b36a6a3cc6e5bae021e19dfa7a54852c3 

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


Testing
---

make check + added a test


Thanks,

Anand Mazumdar

Re: Review Request 54996: Fix SIGBUS crash on ARM64/AArch64.

[Aaron Wood]

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




3rdparty/stout/include/stout/os/linux.hpp (line 68)


Anyone see a way around this reinterpret_cast?


- Aaron Wood


On Jan. 4, 2017, 12:26 a.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54996/
> ---
> 
> (Updated Jan. 4, 2017, 12:26 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6835
> https://issues.apache.org/jira/browse/MESOS-6835
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently in the Linux launcher when the stack is allocated and prepared for 
> a call to clone() it is not properly aligned. This is not an issue for x86 or 
> x64 but for ARM64/AArch64 it is because of the requirement of having the 
> stack aligned to a 16 byte boundary. While x86 and x64 also expect the stack 
> to have a 16 byte aligned stack, it is not enforced. An explanation of the 
> stack and requirements for ARM64 can be found here 
> http://infocenter.arm.com/help/topic/com.arm.doc.ihi0055b/IHI0055B_aapcs64.pdf
>  (specifically section 5.2.2.1 that says SP mod 16 = 0. The stack must be 
> quad-word aligned.)
> 
> Additionally, the way that the stack is currently allocated and passed to 
> clone() accidentally chops off one entry, making a stack overflow using those 
> missing 8 bytes a possibility. Fixing this while aligning the memory will fix 
> both the issue of the stack overflow issue as well as the SIGBUS crash. We 
> should also net better performance from having the stack aligned.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/linux.hpp 530f1a55b 
>   src/linux/ns.hpp 77789717e 
> 
> Diff: https://reviews.apache.org/r/54996/diff/
> 
> 
> Testing
> ---
> 
> Built Mesos from source and am currently running it in a test cluster. 
> Launched both Docker and Mesos tasks via Marathon without any resulting crash 
> (initial crash only happened with Mesos containerizer + linux_launcher, not 
> with the posix_launcher).
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 54996: Fix SIGBUS crash on ARM64/AArch64.

[Aaron Wood]

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

(Updated Jan. 4, 2017, 12:26 a.m.)


Review request for mesos and Jie Yu.


Changes
---

Created stack class so that the logic for creating an aligned stack can be 
shared. Used this new class in another area of the codebase where a stack was 
being allocated.


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


Repository: mesos


Description
---

Currently in the Linux launcher when the stack is allocated and prepared for a 
call to clone() it is not properly aligned. This is not an issue for x86 or x64 
but for ARM64/AArch64 it is because of the requirement of having the stack 
aligned to a 16 byte boundary. While x86 and x64 also expect the stack to have 
a 16 byte aligned stack, it is not enforced. An explanation of the stack and 
requirements for ARM64 can be found here 
http://infocenter.arm.com/help/topic/com.arm.doc.ihi0055b/IHI0055B_aapcs64.pdf 
(specifically section 5.2.2.1 that says SP mod 16 = 0. The stack must be 
quad-word aligned.)

Additionally, the way that the stack is currently allocated and passed to 
clone() accidentally chops off one entry, making a stack overflow using those 
missing 8 bytes a possibility. Fixing this while aligning the memory will fix 
both the issue of the stack overflow issue as well as the SIGBUS crash. We 
should also net better performance from having the stack aligned.


Diffs (updated)
-

  3rdparty/stout/include/stout/os/linux.hpp 530f1a55b 
  src/linux/ns.hpp 77789717e 

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


Testing
---

Built Mesos from source and am currently running it in a test cluster. Launched 
both Docker and Mesos tasks via Marathon without any resulting crash (initial 
crash only happened with Mesos containerizer + linux_launcher, not with the 
posix_launcher).


Thanks,

Aaron Wood

Re: Review Request 54877: Windows: Stout: Removed dependency on Shell API.

[Alex Clemmer]

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


Ship it!




Ship It!

- Alex Clemmer


On Jan. 3, 2017, 10:14 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54877/
> ---
> 
> (Updated Jan. 3, 2017, 10:14 p.m.)
> 
> 
> Review request for mesos, Daniel Pravat, Alex Clemmer, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The API `SHGetKnownFolderPath` requires `Shell32.dll`,
> which is not available on Nano server.
> The equivalent API `GetAllUsersProfileDirectory`
> only requires `Userenv.dll`, which is available on Nano.
> 
> This API is also friendlier, as we own the allocation.
> 
> The Unicode version `GetAllUsersProfileDirectoryW` is
> explicitly used so that we are guaranteed a Unicode path,
> which we then convert from UTF-16 to UTF-8,
> instead of using the ANSI version which depends on a
> varying Windows code-page, and is not recommended.
> 
> A `vector` is used over a `wstring` to avoid dealing
> with the placement of the null-terminating character.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/windows.hpp 
> e641c46d033372e1b6c9f9c066b1ad4957d55088 
>   3rdparty/stout/include/stout/windows/os.hpp 
> 5cd92545a49648e39e8eb7cf131895e9cfc97902 
> 
> Diff: https://reviews.apache.org/r/54877/diff/
> 
> 
> Testing
> ---
> 
> cmake && msbuild, attach agent to master and check default `runtime_dir` 
> value.
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 55152: Fixed nested container docs and added a link to home.md.

[Avinash sridharan]

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




docs/nested-container-and-task-group.md (line 64)


s/of Nested Container/of a Nested Container



docs/nested-container-and-task-group.md (lines 67 - 69)


We could re-write this as follows:
In order for executors to launch nested containers, a new set of agent APIs 
have been introduced (described in the following sections), which would allow 
the exector to re-use the isolation semantics provided by the 
`MesosContainerizer` without needing to reimplement this code within the 
executor. While currently executors are assumed to be the primary consumers of 
the new agent APIs, its important to note that the API has been designed to 
allow any authorized operator to launch nested containers.



docs/nested-container-and-task-group.md (line 209)


s/2/two 
better to write the numeral in words when it modifies the noun.

Lets not mention version numbers in the docs. We can re-write this as:
Currently, this API supports only two levels of nesting.

(You can start the above in a new paragraph for emphasis)



docs/nested-container-and-task-group.md (line 211)


Theoretically, we could have up to 32 nested levels, limited by the maximum 
dept of [pid namespace]


- Avinash sridharan


On Jan. 3, 2017, 9:44 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55152/
> ---
> 
> (Updated Jan. 3, 2017, 9:44 p.m.)
> 
> 
> Review request for mesos, Adam B, Anand Mazumdar, Avinash sridharan, Jie Yu, 
> Neil Conway, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed nested container docs and added a link to home.md.
> 
> 
> Diffs
> -
> 
>   docs/home.md 091dae20b0a26aac03186edaefab93c54b3063b4 
>   docs/nested-container-and-task-group.md 
> debf9f82622557aff915dd04ef06a50793f496b6 
> 
> Diff: https://reviews.apache.org/r/55152/diff/
> 
> 
> Testing
> ---
> 
> https://github.com/Gilbert88/mesos/blob/doc_pod/docs/nested-container-and-task-group.md
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 50871: Supported auto backend in Unified Containerizer.

[Timothy Chen]

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




src/slave/containerizer/mesos/provisioner/docker/paths.cpp (line 65)


Backend no longer needed to be recorded when it's auto selected?



src/slave/containerizer/mesos/provisioner/provisioner.cpp (line 294)


Remove this


- Timothy Chen


On Jan. 3, 2017, 10:47 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50871/
> ---
> 
> (Updated Jan. 3, 2017, 10:47 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Artem Harutyunyan, Jie Yu, and 
> Timothy Chen.
> 
> 
> Bugs: MESOS-5931
> https://issues.apache.org/jira/browse/MESOS-5931
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Select a backend smartly. Currently, the logic is really
> simple:
> 1. Use overlayfs backend if it exists.
> 2. Use aufs backend if the overlayfs does not exists.
> 3. Use copy backend of both overlayfs and aufs do not exist.
> Please note that the bind backend needs to be specified explicitly
> through the agent flag '--image_provisioner_backend' since it
> requires the sandbox already existed.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md e4beb2d5a72f1c5f59b2e40f4984cc60b8437d9d 
>   src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp 
> 817e30c5d6d6a4b011193e3209301fc3cdf88b06 
>   src/slave/containerizer/mesos/provisioner/docker/paths.hpp 
> 232c027f8f96da0cb30b957bce4607d3695050d2 
>   src/slave/containerizer/mesos/provisioner/docker/paths.cpp 
> cd684b33eb308ce1eeb4539a5b2d51985d835db7 
>   src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp 
> b06ddff68a8d2df13abb838b03a8e73d4e273c31 
>   src/slave/containerizer/mesos/provisioner/docker/store.cpp 
> 9dccd0673dbc0c61abfd4b88097f86e7d7131c46 
>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 
> db2d267996959453f9896287320dca37440b499b 
>   src/slave/flags.hpp 6ac0d45072157f6741b96266886a326e9337c153 
>   src/slave/flags.cpp 1eccea920338032173be4df6c374ec50dbd2eaf3 
>   src/tests/containerizer/provisioner_appc_tests.cpp 
> 8b4b832a8fc022435b4d1ada8b8fd9c392ce685e 
> 
> Diff: https://reviews.apache.org/r/50871/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> sudo ./bin/mesos-tests.sh
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 54312: Added TASK_UNREACHABLE to master's state-summary endpoint.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Dec. 2, 2016, 5:02 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54312/
> ---
> 
> (Updated Dec. 2, 2016, 5:02 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6388
> https://issues.apache.org/jira/browse/MESOS-6388
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added TASK_UNREACHABLE to master's state-summary endpoint.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp ac560d1fdd219d0de0c5d987a32a7112e149602f 
>   src/tests/partition_tests.cpp 5a0d4bd2de6a5aa0e9fdf0d34cd10d16fd4e34a1 
> 
> Diff: https://reviews.apache.org/r/54312/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 54232: Shutdown tasks of completed frameworks on agent re-registration.

[Vinod Kone]

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




src/master/master.cpp (line 5512)


inline this?



src/tests/partition_tests.cpp (lines 1130 - 1133)


I agree. Lets move unreachable tasks to completed as well in 
`removeFramework` to avoid confusion. We can change the behavior for PA and 
non-PA together at a later time if necessary.


- Vinod Kone


On Dec. 2, 2016, 12:25 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54232/
> ---
> 
> (Updated Dec. 2, 2016, 12:25 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6602
> https://issues.apache.org/jira/browse/MESOS-6602
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, if a framework completed (e.g., due to a teardown operation
> or framework shutdown), any framework tasks running on partitioned
> agents would not be shutdown when the agent re-registered. For tasks
> that are not partition-aware, the task would be shutdown on agent
> re-registration anyway. But for partition-aware tasks, this could lead
> to orphan tasks.
> 
> Fix this by changing the master to shutdown such tasks when the agent
> reregisters.
> 
> Note that if the master fails over between the time the framework
> completes and a partitioned agent re-registers, any framework tasks
> running on the agent will NOT be shutdown. This is a known bug; fixing
> it requires persisting the framework shutdown operation to the registry
> (MESOS-1719).
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 877ca9010d0d6efc97f3d71fbd27272a255409d0 
>   src/master/master.cpp e03a2e8025943825a2902102c43dc0eb66bacb6a 
>   src/tests/partition_tests.cpp 5a0d4bd2de6a5aa0e9fdf0d34cd10d16fd4e34a1 
> 
> Diff: https://reviews.apache.org/r/54232/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 50871: Supported auto backend in Unified Containerizer.

[Gilbert Song]

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

(Updated Jan. 3, 2017, 2:47 p.m.)


Review request for mesos, Avinash sridharan, Artem Harutyunyan, Jie Yu, and 
Timothy Chen.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Select a backend smartly. Currently, the logic is really
simple:
1. Use overlayfs backend if it exists.
2. Use aufs backend if the overlayfs does not exists.
3. Use copy backend of both overlayfs and aufs do not exist.
Please note that the bind backend needs to be specified explicitly
through the agent flag '--image_provisioner_backend' since it
requires the sandbox already existed.


Diffs (updated)
-

  docs/configuration.md e4beb2d5a72f1c5f59b2e40f4984cc60b8437d9d 
  src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp 
817e30c5d6d6a4b011193e3209301fc3cdf88b06 
  src/slave/containerizer/mesos/provisioner/docker/paths.hpp 
232c027f8f96da0cb30b957bce4607d3695050d2 
  src/slave/containerizer/mesos/provisioner/docker/paths.cpp 
cd684b33eb308ce1eeb4539a5b2d51985d835db7 
  src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp 
b06ddff68a8d2df13abb838b03a8e73d4e273c31 
  src/slave/containerizer/mesos/provisioner/docker/store.cpp 
9dccd0673dbc0c61abfd4b88097f86e7d7131c46 
  src/slave/containerizer/mesos/provisioner/provisioner.cpp 
db2d267996959453f9896287320dca37440b499b 
  src/slave/flags.hpp 6ac0d45072157f6741b96266886a326e9337c153 
  src/slave/flags.cpp 1eccea920338032173be4df6c374ec50dbd2eaf3 
  src/tests/containerizer/provisioner_appc_tests.cpp 
8b4b832a8fc022435b4d1ada8b8fd9c392ce685e 

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


Testing
---

make check

sudo ./bin/mesos-tests.sh


Thanks,

Gilbert Song

Re: Review Request 54216: Fixed unsufficient root privileges check by geteuid().

[Gilbert Song]

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

(Updated Jan. 3, 2017, 2:47 p.m.)


Review request for mesos, Avinash sridharan, Artem Harutyunyan, James DeFelice, 
and Jie Yu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

We should check the root privileges by using the effective user
id, instead of comparing os::user() to 'root'.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
74ab1fda4587d887342f8e63b3a38d450d9c6941 
  src/slave/containerizer/mesos/isolators/gpu/volume.cpp 
478752f372200b79cb3bd1e1c0f254f529930b78 
  src/slave/containerizer/mesos/provisioner/backends/aufs.cpp 
c92c6c7174421158b9190ed0bfb00c1c97ef0f7b 
  src/slave/containerizer/mesos/provisioner/backends/bind.cpp 
94dac40a12b6fd2e7d9733444d84763c77785402 
  src/slave/containerizer/mesos/provisioner/backends/overlay.cpp 
a6e6c8b84adadc24b915b45533d1789f8de758dd 

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


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 54215: Fixed duplicate image layer ids returned by docker store.

[Gilbert Song]

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

(Updated Jan. 3, 2017, 2:47 p.m.)


Review request for mesos, Avinash sridharan, Artem Harutyunyan, Jie Yu, Qian 
Zhang, and Zhitao Li.


Changes
---

Rebased.


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


Repository: mesos


Description
---

This issue is exposed by pulling the 'mesosphere/inky' docker
image using registry puller. Due to the duplicate layer id
from the manifest, there are duplicate layer pathes passed
to the backend. The aufs backend cannot handle this case and
returns 'invalid arguments' error. Ideally, we should make
sure that layer paths that are passed to the backend are
unique.


Diffs (updated)
-

  src/slave/containerizer/mesos/provisioner/docker/store.cpp 
9dccd0673dbc0c61abfd4b88097f86e7d7131c46 

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


Testing
---

make check

Tested by the unit test `ROOT_CURL_INTERNET_DockerDefaultEntryptRegistryPuller`.

Manually tested using the `mesosphere/inky` image, which contains duplicate 
layer ids.


Thanks,

Gilbert Song

Re: Review Request 54214: Added unit test for aufs backend supporting many layers.

[Gilbert Song]

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

(Updated Jan. 3, 2017, 2:46 p.m.)


Review request for mesos, Avinash sridharan, Artem Harutyunyan, Jie Yu, Qian 
Zhang, and Zhitao Li.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Added unit test for aufs backend supporting many layers.


Diffs (updated)
-

  src/tests/containerizer/provisioner_backend_tests.cpp 
727bf645dd9337a94f098ab0a816b7e0e0651083 

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


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 54213: Supported more layers through symlink for aufs backend.

[Gilbert Song]

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

(Updated Jan. 3, 2017, 2:46 p.m.)


Review request for mesos, Avinash sridharan, Artem Harutyunyan, Jie Yu, Qian 
Zhang, and Zhitao Li.


Changes
---

Rebased.


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


Repository: mesos


Description
---

This issue is similar to MESOS-6000. We use the same solution
(using symlinks) to resolve many-layer image issue with aufs
backend.


Diffs (updated)
-

  src/slave/containerizer/mesos/provisioner/backends/aufs.cpp 
c92c6c7174421158b9190ed0bfb00c1c97ef0f7b 

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


Testing
---

make check

Tested by the new unit test `ROOT_AUFS_AufsBackendWithManyLayers` (see the 
following patch). 

Manually tested using a 37-layer image `gilbertsong/cirros:34`. It failed 
before the fix, and it passed after the fix.


Thanks,

Gilbert Song

Re: Review Request 54816: Replaced os::getcwd() to sandbox.get() in provisioner test.

[Gilbert Song]

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

(Updated Jan. 3, 2017, 2:46 p.m.)


Review request for mesos, Avinash sridharan, Artem Harutyunyan, Jie Yu, Shuai 
Lin, and Timothy Chen.


Changes
---

Rebased.


Repository: mesos


Description
---

Replaced os::getcwd() to sandbox.get() in provisioner test.


Diffs (updated)
-

  src/tests/containerizer/provisioner_backend_tests.cpp 
727bf645dd9337a94f098ab0a816b7e0e0651083 

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


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 54212: Fixed overlay backend provisioning multi images symlink.

[Gilbert Song]

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

(Updated Jan. 3, 2017, 2:46 p.m.)


Review request for mesos, Avinash sridharan, Artem Harutyunyan, Jie Yu, Qian 
Zhang, and Zhitao Li.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Since the fix of MESOS-6000, symlinks are used in overlayfs
backend to shorten the arguments when mounting the rootfs.
E.g., '.../backends/overlay/links' is the symlink created
for a provisioned image. It becomes problematic if a
container image is specified while some image volumes are
specified for the same container. An unique symlink is
needed for each image to be provisioned.


Diffs (updated)
-

  src/slave/containerizer/mesos/provisioner/backends/overlay.cpp 
a6e6c8b84adadc24b915b45533d1789f8de758dd 

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


Testing
---

make check

Verified that the unit test `ROOT_ImageInVolumeWithRootFilesystem` passed.


Thanks,

Gilbert Song

Re: Review Request 54537: Support 'Basic' auth docker registry on Unified Containerizer.

[Gilbert Song]

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

(Updated Jan. 3, 2017, 2:39 p.m.)


Review request for mesos, Artem Harutyunyan, Jie Yu, Shuai Lin, and Timothy 
Chen.


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


Repository: mesos


Description
---

This patch implements the support for 'Basic' docker registry
authorization. It is tested by a local authenticated private
registry using 'localhost:443/alpine' docker image.
Please note that the AWS ECS uses Basic authorization but it
does not work yet due to the redirect issue MESOS-5172.


Diffs
-

  src/uri/fetchers/docker.cpp 3f38dddfb4c089322fe4e13b1ef2070b4835885c 

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


Testing
---

make check

Tested with local authenticated registry. Please follow the steps below:

1. Start a local private registry and push an image to it.
```
docker run -d -p 443:5000 --restart=always --name registry \
  -v `pwd`/auth:/auth \
  -e "REGISTRY_AUTH=htpasswd" \
  -e "REGISTRY_AUTH_HTPASSWD_REALM=Registry Realm" \
  -e REGISTRY_AUTH_HTPASSWD_PATH=/auth/htpasswd \
  -v `pwd`/certs:/certs \
  -e REGISTRY_HTTP_TLS_CERTIFICATE=/certs/localhost.crt \
  -e REGISTRY_HTTP_TLS_KEY=/certs/localhost.key \
  registry:2
```
(*Note: need to generate TLS certificate file and key first)

Then, push an image to the registry.
```
docker push localhost:443/alpine
```

2. Use `mesos-execute` to test the `localhost:443/alpine` image.
(*Note: need to configure the curl using the curl's default RC file), e.g., in 
`~/.curlrc` file:
cacert = "/path/to/cacert.pem"


Thanks,

Gilbert Song

Re: Review Request 54537: Support 'Basic' auth docker registry on Unified Containerizer.

[Gilbert Song]

> On Dec. 16, 2016, 11:49 a.m., Jie Yu wrote:
> > src/uri/fetchers/docker.cpp, lines 809-829
> > 
> >
> > Let's add a Header abstraction in process::http and move the parsing 
> > logic there:
> > 
> > ```
> > namespace http {
> > 
> > namespace header {
> > 
> > // https://tools.ietf.org/html/rfc2617.
> > class WWWAuthenticate
> > {
> > public:
> >   constexpr char NAME[] = "WWW-Authenticate";
> >   
> >   static Try create(const string& value);
> >   
> >   string scheme();
> >   vector challenges();
> > };
> > 
> > }
> > 
> > class Headers
> > {
> >   typedef hashmap<
> >   string,
> >   string,
> >   CaseInsensitiveHash,
> >   CaseInsensitiveEqual> Type;
> >   
> >   template 
> >   Result get()
> >   {
> > Option value = get(T::NAME);
> > if (value.isNone()) {
> >   return None();
> > }
> > Try header = T::create(value.get());
> > if (header.isError()) {
> >   return Error(header.error());
> > }
> > return header.get();
> >   }
> >   
> >   Try header =
> > header::WWWAuthenticate::create(value.get());
> >   ...
> >   }
> >   
> >   Option get(const string& key);
> > 
> >   string& operator[] (const string& key);
> >   
> >   typename Type::iterator begin();
> >   typename Type::iterator end();
> >   typename Type::const_iterator begin() const;
> >   typename Type::const_iterator end() const;
> >   
> >   Type values;
> > };
> > }
> > ```

Let me detach the patch from the chain. And use another chain for `basic auth`.


- Gilbert


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


On Dec. 15, 2016, 9:55 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54537/
> ---
> 
> (Updated Dec. 15, 2016, 9:55 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Jie Yu, Shuai Lin, and Timothy 
> Chen.
> 
> 
> Bugs: MESOS-6758
> https://issues.apache.org/jira/browse/MESOS-6758
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch implements the support for 'Basic' docker registry
> authorization. It is tested by a local authenticated private
> registry using 'localhost:443/alpine' docker image.
> Please note that the AWS ECS uses Basic authorization but it
> does not work yet due to the redirect issue MESOS-5172.
> 
> 
> Diffs
> -
> 
>   src/uri/fetchers/docker.cpp 3f38dddfb4c089322fe4e13b1ef2070b4835885c 
> 
> Diff: https://reviews.apache.org/r/54537/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> Tested with local authenticated registry. Please follow the steps below:
> 
> 1. Start a local private registry and push an image to it.
> ```
> docker run -d -p 443:5000 --restart=always --name registry \
>   -v `pwd`/auth:/auth \
>   -e "REGISTRY_AUTH=htpasswd" \
>   -e "REGISTRY_AUTH_HTPASSWD_REALM=Registry Realm" \
>   -e REGISTRY_AUTH_HTPASSWD_PATH=/auth/htpasswd \
>   -v `pwd`/certs:/certs \
>   -e REGISTRY_HTTP_TLS_CERTIFICATE=/certs/localhost.crt \
>   -e REGISTRY_HTTP_TLS_KEY=/certs/localhost.key \
>   registry:2
> ```
> (*Note: need to generate TLS certificate file and key first)
> 
> Then, push an image to the registry.
> ```
> docker push localhost:443/alpine
> ```
> 
> 2. Use `mesos-execute` to test the `localhost:443/alpine` image.
> (*Note: need to configure the curl using the curl's default RC file), e.g., 
> in `~/.curlrc` file:
> cacert = "/path/to/cacert.pem"
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 54877: Windows: Stout: Removed dependency on Shell API.

[Andrew Schwartzmeyer]

> On Dec. 20, 2016, 12:29 a.m., Daniel Pravat wrote:
> > 3rdparty/stout/include/stout/windows/os.hpp, line 748
> > 
> >
> > I don't think the conversion to UTF-8 is appropiate here.
> 
> Andrew Schwartzmeyer wrote:
> What would you convert it to? It's currently in UTF-16, and Windows paths 
> are allowed to have (almost) any Unicode character.
> 
> Alex Clemmer wrote:
> I think he's saying that bad things will happen if you use strings that 
> have Unicode in them, so it's probably better to just error out.
> 
> Andrew Schwartzmeyer wrote:
> I'm not following... That is a case our codebase must deal with, NTFS 
> paths are stored in Unicode:
> 
> 
> https://msdn.microsoft.com/en-us/library/windows/desktop/dd317748(v=vs.85).aspx
> 
> and on top of that, Windows file paths can:
> 
> > Use any character in the current code page for a name, including 
> Unicode characters and characters in the extended character set (128–255)
> 
> 
> https://msdn.microsoft.com/en-us/library/windows/desktop/aa365247(v=vs.85).aspx
> 
> So what I'm saying is that bad things _must not happen_ if we have paths 
> with non-ASCII characters in them; we need to handle that at any point we 
> deal with file paths on Windows.
> 
> I'm not saying this function is likely to return a path like `C:\ÂÑÐ¥`, 
> but that would be a valid file path, and so we must be treating all our paths 
> on Windows as Unicode.
> 
> If this is not the case, are purposefully constraining ourselves to 
> ASCII, and if so, why?
> 
> Alex Clemmer wrote:
> Sorry, I meant bad things will happen to Mesos if you give it strings 
> with unicode in them. Mesos itself does not deal with Unicode gracefully.
> 
> Andrew Schwartzmeyer wrote:
> Oooh. We should probably fix that.
> 
> Andrew Schwartzmeyer wrote:
> Alex, Daniel, do we have a resolution on this? I spoke with Alex and we 
> feel that we should be doing two main things: avoid exacberating the problem 
> in MESOS-6817 by being explicit in our use of `W` APIs, rather than relying 
> on the mercurial value of `TCHAR`; and not attempt to guard against paths 
> that Mesos does not handle, as it is a bug that Mesos does not handle paths 
> with valid extended characters (since both Linux and Windows may have these 
> paths), so failing here would be inappropriate as it hides the actual bug. 
> (Not to speak for you Alex, I think this summarizes our discussion yesterday.)
> 
> Alex Clemmer wrote:
> If I'm understanding our discussion correctly, failing here on unicode 
> conversion would be preferable, since it makes a subtle bug more obvious. If 
> that's nontrivial I think we can just ship it as is, since Mesos already has 
> these problems.
> 
> Andrew Schwartzmeyer wrote:
> > failing here on unicode conversion
> 
> What would fail here?
> 
> I *think* you're implying to add a detection algorithm to see if there 
> are any characters in the path that Mesos does not believe are valid; but 
> that is not "failing on Unicode", we'd need to write our own validator for 
> Mesos.
> 
> Alex Clemmer wrote:
> I was just saying, if there's an easy way to try to interpret the string 
> as normal ASCII and return `Error` if that fails, we should do that. If not, 
> it's not that big of a deal, because Mesos will probably fail anyway.
> 
> Andrew Schwartzmeyer wrote:
> Ah I see what you want: 
> > interpret the string as normal ASCII and return Error if that fails
> 
> but I don't believe this is possible. To be clear, the non-Unicode 
> versions of these APIs do not return ASCII; they return the same data encoded 
> with the current Windows code page (which is a superset of ASCII, in much the 
> same way as UTF-8). If we used this version, we would not only hit the same 
> problem (possible non-ASCII characters), but also introduce a new problem of 
> having to decode from an arbitrary Windows code page (rather than UTF-16 
> which is at least standard). With either the short or wide version of the 
> API, detecting if the string is not ASCII would mean implmenenting a 
> validator, which I don't think we want to do here (it's the wrong solution to 
> the problem).
> 
> That said, what would you suggest?
> 
> P.S. For what it's worth, I don't expect this particular API to give us 
> non-ASCII characters, but I think we should choose a pattern for dealing with 
> Windows APIs and stick to it. This pattern gives us guaranteed UTF-8 strings 
> with whatever data we're expected to handle.
> 
> Alex Clemmer wrote:
> I suggest just punting on returning `Error` if the conversion fails, 
> since Mesos does not handle unicode anyway. Daniel's suggestion to avoid the 
> conversion to unicode should probably also be adopted.

Summarizing offline discussion:

The explicit use of `GetAllUsersProfileDirectoryW` for 

Re: Review Request 54183: Improved management of unreachable and completed tasks in master.

[Vinod Kone]

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




src/master/master.hpp (lines 2634 - 2641)


this reads like `unreachableTasks` are completed tasks of PA frameworks. 
can you split this comment up and move the unreachable specific comments to 
#2644?



src/master/master.cpp (line 5496)


it's not necessary that the agent has failed over if we are here right?



src/master/master.cpp (line 5507)


can we inline this?



src/master/master.cpp (lines 5513 - 5514)


can you make this a TODO?



src/master/master.cpp (line 5523)


not sure if this is worth making into a function?



src/master/master.cpp (line 5539)


what if the agent was marked unreachable by the previous master? do we 
still want to add all its tasks?



src/tests/partition_tests.cpp (lines 1819 - 1847)


hmm. it is unfortunate that we send TASK_LOST followed by TASK_FINISHED and 
then send TASK_LOST on reconciliation. can you remind me why the master 
forwards status updates for unknown tasks? looks like it can just drop them if 
the reason for doing so is no longer valid.


- Vinod Kone


On Dec. 18, 2016, 11:29 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54183/
> ---
> 
> (Updated Dec. 18, 2016, 11:29 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6619
> https://issues.apache.org/jira/browse/MESOS-6619
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Before partition-awareness, when an agent failed health checks, the
> master removed the agent from the registry, marked all of its tasks
> TASK_LOST, and moved them to the `completedTasks` list in the master's
> memory. Although "lost" tasks might still be running, partitioned agents
> would only be allowed to re-register if the master failed over, in which
> case the `completedTasks` map would be emptied.
> 
> When partition-awareness was introduced, we initially followed the same
> scheme, with the only difference that partition-aware tasks are marked
> TASK_UNREACHABLE, not TASK_LOST.
> 
> This scheme has a few shortcomings. First, partition-aware tasks might
> resume running when the partitioned agent re-registers. Second, we
> re-added non-partition aware tasks when the agent re-registered but then
> marked them completed when the framework is shutdown, resulting in two
> entries in `completedTasks`.
> 
> This commit introduces a separate bounded map, `unreachableTasks`. These
> tasks are reported separately via the HTTP endpoints, because they have
> different semantics (unlike completed tasks, unreachable tasks can
> resume running). The size of this map is limited by a new master flag,
> `--max_unreachable_tasks_per_framework`. This commit also changes the
> master to omit re-adding non-partition-aware tasks on re-registering
> agents (unless the master has failed over): those tasks will shortly be
> shutdown anyway.
> 
> Finally, this commit fixes a minor bug in the previous code: the
> previous coding neglected to shutdown non-partition-aware frameworks
> running on pre-1.0 Mesos agents that re-register with the master after
> a network partition.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md efe3e9bd9d203a7ba44adf4ead24f14b8b577637 
>   include/mesos/master/master.proto 483dc2f59684481c9f549de9f06eef4dda8a46e1 
>   include/mesos/v1/master/master.proto 
> 09a82af88303a2d971da7c56a7075d7005932363 
>   src/master/constants.hpp 5dd0667f62d2c0617cc0d5aed8cc005bd8344c88 
>   src/master/flags.hpp 6a17b763dc76daa10073394f416b049e97a44238 
>   src/master/flags.cpp e5edfd0a0c529a10dc602ef9a88a0ec60c69 
>   src/master/http.cpp ee559804c490d53be2d12f233ae0bfb93ed9f96d 
>   src/master/master.hpp 89b3c394b268a8645885412aeb19980db8d73c69 
>   src/master/master.cpp b664550d57ef9805bd23ea35ca7f9cd8f4b1ab78 
>   src/tests/partition_tests.cpp e1e2025bd0f078836323cbd8c6d7836815c4c38d 
> 
> Diff: https://reviews.apache.org/r/54183/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 54877: Windows: Stout: Removed dependency on Shell API.

[Andrew Schwartzmeyer]

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

(Updated Jan. 3, 2017, 10:14 p.m.)


Review request for mesos, Daniel Pravat, Alex Clemmer, and Joseph Wu.


Changes
---

Windows code-page != ASCII; typo for ANSI.


Repository: mesos


Description (updated)
---

The API `SHGetKnownFolderPath` requires `Shell32.dll`,
which is not available on Nano server.
The equivalent API `GetAllUsersProfileDirectory`
only requires `Userenv.dll`, which is available on Nano.

This API is also friendlier, as we own the allocation.

The Unicode version `GetAllUsersProfileDirectoryW` is
explicitly used so that we are guaranteed a Unicode path,
which we then convert from UTF-16 to UTF-8,
instead of using the ANSI version which depends on a
varying Windows code-page, and is not recommended.

A `vector` is used over a `wstring` to avoid dealing
with the placement of the null-terminating character.


Diffs
-

  3rdparty/stout/include/stout/windows.hpp 
e641c46d033372e1b6c9f9c066b1ad4957d55088 
  3rdparty/stout/include/stout/windows/os.hpp 
5cd92545a49648e39e8eb7cf131895e9cfc97902 

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


Testing
---

cmake && msbuild, attach agent to master and check default `runtime_dir` value.


Thanks,

Andrew Schwartzmeyer

Re: Review Request 55152: Fixed nested container docs and added a link to home.md.

[Gilbert Song]

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

(Updated Jan. 3, 2017, 1:44 p.m.)


Review request for mesos, Adam B, Anand Mazumdar, Avinash sridharan, Jie Yu, 
Neil Conway, and Vinod Kone.


Changes
---

Simplified the doc title in home.md


Repository: mesos


Description
---

Fixed nested container docs and added a link to home.md.


Diffs (updated)
-

  docs/home.md 091dae20b0a26aac03186edaefab93c54b3063b4 
  docs/nested-container-and-task-group.md 
debf9f82622557aff915dd04ef06a50793f496b6 

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


Testing
---

https://github.com/Gilbert88/mesos/blob/doc_pod/docs/nested-container-and-task-group.md


Thanks,

Gilbert Song

Review Request 55152: Fixed nested container docs and added a link to home.md.

[Gilbert Song]

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

Review request for mesos, Adam B, Anand Mazumdar, Avinash sridharan, Jie Yu, 
Neil Conway, and Vinod Kone.


Repository: mesos


Description
---

Fixed nested container docs and added a link to home.md.


Diffs
-

  docs/home.md 091dae20b0a26aac03186edaefab93c54b3063b4 
  docs/nested-container-and-task-group.md 
debf9f82622557aff915dd04ef06a50793f496b6 

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


Testing
---

https://github.com/Gilbert88/mesos/blob/doc_pod/docs/nested-container-and-task-group.md


Thanks,

Gilbert Song

Re: Review Request 54073: Added linux launcher nested container support doc.

[Gilbert Song]

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

(Updated Jan. 3, 2017, 1:35 p.m.)


Review request for mesos, Anand Mazumdar, Benjamin Mahler, Artem Harutyunyan, 
Jie Yu, Neil Conway, Qian Zhang, and Vinod Kone.


Changes
---

Rebased.


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


Repository: mesos


Description
---

This patch adds a table showing the current semantics
that the linux launcher supports namespaces for nested
container.


Diffs (updated)
-

  docs/containerizer-internals.md 97776a69e5a7df8b65f672a754d456e7b2b90320 

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


Testing
---

Tested by gist view. Here is the link:

https://github.com/Gilbert88/mesos/blob/doc_pod/docs/containerizer-internals.md#linux-launcher


Thanks,

Gilbert Song

Re: Review Request 54074: Added isolator nested aware user doc.

[Gilbert Song]

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

(Updated Jan. 3, 2017, 1:35 p.m.)


Review request for mesos, Anand Mazumdar, Benjamin Mahler, Artem Harutyunyan, 
Jie Yu, Neil Conway, Qian Zhang, and Vinod Kone.


Changes
---

Rebased.


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


Repository: mesos


Description
---

This patch adds a table for current semantics of isolators
in Mesos, as well as the guidance of how the make a custom
isolator module to be nested aware.


Diffs (updated)
-

  docs/mesos-containerizer.md d0b9d762ab83bff1cf2bd98ed566376b7bd566c6 

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


Testing
---

Tested by gist view. Here is the link:

https://github.com/Gilbert88/mesos/blob/doc_pod/docs/mesos-containerizer.md#isolator-nested-aware


Thanks,

Gilbert Song

Re: Review Request 54013: Added user doc for nested container and task group.

[Gilbert Song]

> On Dec. 4, 2016, 11:44 a.m., Vinod Kone wrote:
> > docs/nested-container-and-task-group.md, line 209
> > 
> >
> > mention that only 2 levels of nesting is supported as of 1.1?
> 
> Gilbert Song wrote:
> We already support arbitrary nested levels. There is a unit test 
> (`ROOT_CGROUPS_LaunchNestedThreeLevels`) for that support.
> 
> Adam B wrote:
> And yet 
> https://github.com/apache/mesos/blob/1.1.0/src/slave/http.cpp#L1929 says
> "We do not yet support launching containers that are nested two levels 
> beneath the executor's container."
> and errors if you try to launch a nested container with a grandparent.

Fixed on a followup patch.


- Gilbert


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


On Dec. 6, 2016, 8:28 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54013/
> ---
> 
> (Updated Dec. 6, 2016, 8:28 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Benjamin Mahler, Artem Harutyunyan, 
> Jie Yu, Neil Conway, Qian Zhang, and Vinod Kone.
> 
> 
> Bugs: MESOS-6335
> https://issues.apache.org/jira/browse/MESOS-6335
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added user doc for nested container and task group.
> 
> 
> Diffs
> -
> 
>   docs/nested-container-and-task-group.md PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/54013/diff/
> 
> 
> Testing
> ---
> 
> Tested by gist view. Here is the link:
> 
> https://github.com/Gilbert88/mesos/blob/doc_pod/docs/nested-container-and-task-group.md
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 51052: Made mesos-docker-execute understand cgroups_enable_cfs.

[Zhitao Li]

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

(Updated Jan. 3, 2017, 9:16 p.m.)


Review request for mesos, Gilbert Song, haosdent huang, and Jie Yu.


Changes
---

Redo this patch on top of r/54821


Summary (updated)
-

Made mesos-docker-execute understand cgroups_enable_cfs.


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


Repository: mesos


Description (updated)
---

This fixed cpu quota for command executor (which runs outside
of the docker container) by ensuing --cpu-quota flag to docker
run.


Diffs (updated)
-

  src/docker/docker.hpp 9093371afc8ea792ba94f61c6875703e547ea6b0 
  src/docker/docker.cpp 472cb1b4dc2b0ac65721c732fca8ec70a7470f47 
  src/docker/executor.hpp 8385631fd170f97b28c4ca3596255ab0546774d6 
  src/docker/executor.cpp 9b5c469e2d0f33e228ec746711e6bc6ed352cbc7 
  src/slave/containerizer/docker.cpp 7a8a7271b54af0b4dcdae7a2aa8a90a8a7d05fd6 
  src/tests/containerizer/docker_tests.cpp 
9667d434486c1832f180a297a39a3d5dae6a26bd 

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


Testing
---

I am now able to make docker containers launched through mesos-execute have a 
cpu quota.

Also making sure `make check` still works on mac os for the linux only flag.


Thanks,

Zhitao Li

Re: Review Request 54841: Used `loop` in implementation of io::read and io::write.

[Anand Mazumdar]

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


Ship it!




Ship It!

- Anand Mazumdar


On Dec. 18, 2016, 1:04 a.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54841/
> ---
> 
> (Updated Dec. 18, 2016, 1:04 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used `loop` in implementation of io::read and io::write.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/io.cpp 27da897894e12941a6bba5f5eda04c35100d2d73 
> 
> Diff: https://reviews.apache.org/r/54841/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 54840: Used process::loop to avoid stack overflow due to recursion.

[Anand Mazumdar]

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


Ship it!




Ship It!

- Anand Mazumdar


On Dec. 17, 2016, 9:50 p.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54840/
> ---
> 
> (Updated Dec. 17, 2016, 9:50 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used process::loop to avoid stack overflow due to recursion.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/http.cpp 97d1424be20e217401519c2bee79857bcf087023 
> 
> Diff: https://reviews.apache.org/r/54840/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Review Request 55149: Added the `mesos-cni-port-mapper` to the CMake build.

[Avinash sridharan]

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

Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Added the `mesos-cni-port-mapper` to the CMake build.


Diffs
-

  cmake/MesosConfigure.cmake 6a9ed9dc02f5c9a6d1fce0866f19ffeafec35cdc 
  src/slave/containerizer/mesos/CMakeLists.txt 
650b1a28d96bb2fb8cb43b90e2edd18694dfc09a 

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


Testing
---

make check

Ran a cmake build to ensure that the `mesos-cni-port-mapper` plugin is being 
compiled.


Thanks,

Avinash sridharan

Re: Review Request 54839: Updated Mesos process::loop uses with process::ControlFlow.

[Anand Mazumdar]

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


Ship it!




LGTM, I would also go ahead and mark the issue that Kevin raised as fixed.

- Anand Mazumdar


On Dec. 17, 2016, 9:40 p.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54839/
> ---
> 
> (Updated Dec. 17, 2016, 9:40 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Kevin Klues, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated Mesos process::loop uses with process::ControlFlow.
> 
> 
> Diffs
> -
> 
>   src/common/recordio.hpp 5a22d066bd2bcd40ede5ebf6476b9ceb972dd584 
>   src/slave/containerizer/mesos/io/switchboard.cpp 
> c80627986f729255e3b3ad1fc9cfa6213e19c521 
>   src/slave/http.cpp e1cea46cb17ea2f24fe457bb06a0a1a55c2a2bc4 
> 
> Diff: https://reviews.apache.org/r/54839/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 54877: Windows: Stout: Removed dependency on Shell API.

[Alex Clemmer]

> On Dec. 20, 2016, 12:29 a.m., Daniel Pravat wrote:
> > 3rdparty/stout/include/stout/windows/os.hpp, line 748
> > 
> >
> > I don't think the conversion to UTF-8 is appropiate here.
> 
> Andrew Schwartzmeyer wrote:
> What would you convert it to? It's currently in UTF-16, and Windows paths 
> are allowed to have (almost) any Unicode character.
> 
> Alex Clemmer wrote:
> I think he's saying that bad things will happen if you use strings that 
> have Unicode in them, so it's probably better to just error out.
> 
> Andrew Schwartzmeyer wrote:
> I'm not following... That is a case our codebase must deal with, NTFS 
> paths are stored in Unicode:
> 
> 
> https://msdn.microsoft.com/en-us/library/windows/desktop/dd317748(v=vs.85).aspx
> 
> and on top of that, Windows file paths can:
> 
> > Use any character in the current code page for a name, including 
> Unicode characters and characters in the extended character set (128–255)
> 
> 
> https://msdn.microsoft.com/en-us/library/windows/desktop/aa365247(v=vs.85).aspx
> 
> So what I'm saying is that bad things _must not happen_ if we have paths 
> with non-ASCII characters in them; we need to handle that at any point we 
> deal with file paths on Windows.
> 
> I'm not saying this function is likely to return a path like `C:\ÂÑÐ¥`, 
> but that would be a valid file path, and so we must be treating all our paths 
> on Windows as Unicode.
> 
> If this is not the case, are purposefully constraining ourselves to 
> ASCII, and if so, why?
> 
> Alex Clemmer wrote:
> Sorry, I meant bad things will happen to Mesos if you give it strings 
> with unicode in them. Mesos itself does not deal with Unicode gracefully.
> 
> Andrew Schwartzmeyer wrote:
> Oooh. We should probably fix that.
> 
> Andrew Schwartzmeyer wrote:
> Alex, Daniel, do we have a resolution on this? I spoke with Alex and we 
> feel that we should be doing two main things: avoid exacberating the problem 
> in MESOS-6817 by being explicit in our use of `W` APIs, rather than relying 
> on the mercurial value of `TCHAR`; and not attempt to guard against paths 
> that Mesos does not handle, as it is a bug that Mesos does not handle paths 
> with valid extended characters (since both Linux and Windows may have these 
> paths), so failing here would be inappropriate as it hides the actual bug. 
> (Not to speak for you Alex, I think this summarizes our discussion yesterday.)
> 
> Alex Clemmer wrote:
> If I'm understanding our discussion correctly, failing here on unicode 
> conversion would be preferable, since it makes a subtle bug more obvious. If 
> that's nontrivial I think we can just ship it as is, since Mesos already has 
> these problems.
> 
> Andrew Schwartzmeyer wrote:
> > failing here on unicode conversion
> 
> What would fail here?
> 
> I *think* you're implying to add a detection algorithm to see if there 
> are any characters in the path that Mesos does not believe are valid; but 
> that is not "failing on Unicode", we'd need to write our own validator for 
> Mesos.
> 
> Alex Clemmer wrote:
> I was just saying, if there's an easy way to try to interpret the string 
> as normal ASCII and return `Error` if that fails, we should do that. If not, 
> it's not that big of a deal, because Mesos will probably fail anyway.
> 
> Andrew Schwartzmeyer wrote:
> Ah I see what you want: 
> > interpret the string as normal ASCII and return Error if that fails
> 
> but I don't believe this is possible. To be clear, the non-Unicode 
> versions of these APIs do not return ASCII; they return the same data encoded 
> with the current Windows code page (which is a superset of ASCII, in much the 
> same way as UTF-8). If we used this version, we would not only hit the same 
> problem (possible non-ASCII characters), but also introduce a new problem of 
> having to decode from an arbitrary Windows code page (rather than UTF-16 
> which is at least standard). With either the short or wide version of the 
> API, detecting if the string is not ASCII would mean implmenenting a 
> validator, which I don't think we want to do here (it's the wrong solution to 
> the problem).
> 
> That said, what would you suggest?
> 
> P.S. For what it's worth, I don't expect this particular API to give us 
> non-ASCII characters, but I think we should choose a pattern for dealing with 
> Windows APIs and stick to it. This pattern gives us guaranteed UTF-8 strings 
> with whatever data we're expected to handle.

I suggest just punting on returning `Error` if the conversion fails, since 
Mesos does not handle unicode anyway. Daniel's suggestion to avoid the 
conversion to unicode should probably also be adopted.


- Alex


---
This is an automatically generated e-mail. To reply, 

Re: Review Request 54877: Windows: Stout: Removed dependency on Shell API.

[Andrew Schwartzmeyer]

> On Dec. 20, 2016, 12:29 a.m., Daniel Pravat wrote:
> > 3rdparty/stout/include/stout/windows/os.hpp, line 748
> > 
> >
> > I don't think the conversion to UTF-8 is appropiate here.
> 
> Andrew Schwartzmeyer wrote:
> What would you convert it to? It's currently in UTF-16, and Windows paths 
> are allowed to have (almost) any Unicode character.
> 
> Alex Clemmer wrote:
> I think he's saying that bad things will happen if you use strings that 
> have Unicode in them, so it's probably better to just error out.
> 
> Andrew Schwartzmeyer wrote:
> I'm not following... That is a case our codebase must deal with, NTFS 
> paths are stored in Unicode:
> 
> 
> https://msdn.microsoft.com/en-us/library/windows/desktop/dd317748(v=vs.85).aspx
> 
> and on top of that, Windows file paths can:
> 
> > Use any character in the current code page for a name, including 
> Unicode characters and characters in the extended character set (128–255)
> 
> 
> https://msdn.microsoft.com/en-us/library/windows/desktop/aa365247(v=vs.85).aspx
> 
> So what I'm saying is that bad things _must not happen_ if we have paths 
> with non-ASCII characters in them; we need to handle that at any point we 
> deal with file paths on Windows.
> 
> I'm not saying this function is likely to return a path like `C:\ÂÑÐ¥`, 
> but that would be a valid file path, and so we must be treating all our paths 
> on Windows as Unicode.
> 
> If this is not the case, are purposefully constraining ourselves to 
> ASCII, and if so, why?
> 
> Alex Clemmer wrote:
> Sorry, I meant bad things will happen to Mesos if you give it strings 
> with unicode in them. Mesos itself does not deal with Unicode gracefully.
> 
> Andrew Schwartzmeyer wrote:
> Oooh. We should probably fix that.
> 
> Andrew Schwartzmeyer wrote:
> Alex, Daniel, do we have a resolution on this? I spoke with Alex and we 
> feel that we should be doing two main things: avoid exacberating the problem 
> in MESOS-6817 by being explicit in our use of `W` APIs, rather than relying 
> on the mercurial value of `TCHAR`; and not attempt to guard against paths 
> that Mesos does not handle, as it is a bug that Mesos does not handle paths 
> with valid extended characters (since both Linux and Windows may have these 
> paths), so failing here would be inappropriate as it hides the actual bug. 
> (Not to speak for you Alex, I think this summarizes our discussion yesterday.)
> 
> Alex Clemmer wrote:
> If I'm understanding our discussion correctly, failing here on unicode 
> conversion would be preferable, since it makes a subtle bug more obvious. If 
> that's nontrivial I think we can just ship it as is, since Mesos already has 
> these problems.
> 
> Andrew Schwartzmeyer wrote:
> > failing here on unicode conversion
> 
> What would fail here?
> 
> I *think* you're implying to add a detection algorithm to see if there 
> are any characters in the path that Mesos does not believe are valid; but 
> that is not "failing on Unicode", we'd need to write our own validator for 
> Mesos.
> 
> Alex Clemmer wrote:
> I was just saying, if there's an easy way to try to interpret the string 
> as normal ASCII and return `Error` if that fails, we should do that. If not, 
> it's not that big of a deal, because Mesos will probably fail anyway.

Ah I see what you want: 
> interpret the string as normal ASCII and return Error if that fails

but I don't believe this is possible. To be clear, the non-Unicode versions of 
these APIs do not return ASCII; they return the same data encoded with the 
current Windows code page (which is a superset of ASCII, in much the same way 
as UTF-8). If we used this version, we would not only hit the same problem 
(possible non-ASCII characters), but also introduce a new problem of having to 
decode from an arbitrary Windows code page (rather than UTF-16 which is at 
least standard). With either the short or wide version of the API, detecting if 
the string is not ASCII would mean implmenenting a validator, which I don't 
think we want to do here (it's the wrong solution to the problem).

That said, what would you suggest?

P.S. For what it's worth, I don't expect this particular API to give us 
non-ASCII characters, but I think we should choose a pattern for dealing with 
Windows APIs and stick to it. This pattern gives us guaranteed UTF-8 strings 
with whatever data we're expected to handle.


- Andrew


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


On Dec. 19, 2016, 11:20 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> 

Re: Review Request 55023: Windows: Fixed malformatted containerizer command in launcher.

[Daniel Pravat]

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




src/slave/containerizer/mesos/containerizer.cpp (line 1604)


This change is sufficient to fix the problem. Otherwise the user must add 
flags.launcher_dir to the path.



src/slave/containerizer/mesos/containerizer.cpp (line 1614)


Not necessary to run cmd.exe (arg0) just to start mesos_containerizer.


- Daniel Pravat


On Dec. 24, 2016, 10:02 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55023/
> ---
> 
> (Updated Dec. 24, 2016, 10:02 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, John Kordich, 
> and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently in `MesosContainerizerProcess::_launch`, we are passing a
> malformatted shell command to the launcher. This causes the
> containerizer process to crash immediately upon invocation in all
> executor tests.
> 
> This commit will fix this command.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> d9d5619e45ae1199fc91878f17a33b5647f48305 
> 
> Diff: https://reviews.apache.org/r/55023/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 54877: Windows: Stout: Removed dependency on Shell API.

[Alex Clemmer]

> On Dec. 20, 2016, 12:29 a.m., Daniel Pravat wrote:
> > 3rdparty/stout/include/stout/windows/os.hpp, line 748
> > 
> >
> > I don't think the conversion to UTF-8 is appropiate here.
> 
> Andrew Schwartzmeyer wrote:
> What would you convert it to? It's currently in UTF-16, and Windows paths 
> are allowed to have (almost) any Unicode character.
> 
> Alex Clemmer wrote:
> I think he's saying that bad things will happen if you use strings that 
> have Unicode in them, so it's probably better to just error out.
> 
> Andrew Schwartzmeyer wrote:
> I'm not following... That is a case our codebase must deal with, NTFS 
> paths are stored in Unicode:
> 
> 
> https://msdn.microsoft.com/en-us/library/windows/desktop/dd317748(v=vs.85).aspx
> 
> and on top of that, Windows file paths can:
> 
> > Use any character in the current code page for a name, including 
> Unicode characters and characters in the extended character set (128–255)
> 
> 
> https://msdn.microsoft.com/en-us/library/windows/desktop/aa365247(v=vs.85).aspx
> 
> So what I'm saying is that bad things _must not happen_ if we have paths 
> with non-ASCII characters in them; we need to handle that at any point we 
> deal with file paths on Windows.
> 
> I'm not saying this function is likely to return a path like `C:\ÂÑÐ¥`, 
> but that would be a valid file path, and so we must be treating all our paths 
> on Windows as Unicode.
> 
> If this is not the case, are purposefully constraining ourselves to 
> ASCII, and if so, why?
> 
> Alex Clemmer wrote:
> Sorry, I meant bad things will happen to Mesos if you give it strings 
> with unicode in them. Mesos itself does not deal with Unicode gracefully.
> 
> Andrew Schwartzmeyer wrote:
> Oooh. We should probably fix that.
> 
> Andrew Schwartzmeyer wrote:
> Alex, Daniel, do we have a resolution on this? I spoke with Alex and we 
> feel that we should be doing two main things: avoid exacberating the problem 
> in MESOS-6817 by being explicit in our use of `W` APIs, rather than relying 
> on the mercurial value of `TCHAR`; and not attempt to guard against paths 
> that Mesos does not handle, as it is a bug that Mesos does not handle paths 
> with valid extended characters (since both Linux and Windows may have these 
> paths), so failing here would be inappropriate as it hides the actual bug. 
> (Not to speak for you Alex, I think this summarizes our discussion yesterday.)
> 
> Alex Clemmer wrote:
> If I'm understanding our discussion correctly, failing here on unicode 
> conversion would be preferable, since it makes a subtle bug more obvious. If 
> that's nontrivial I think we can just ship it as is, since Mesos already has 
> these problems.
> 
> Andrew Schwartzmeyer wrote:
> > failing here on unicode conversion
> 
> What would fail here?
> 
> I *think* you're implying to add a detection algorithm to see if there 
> are any characters in the path that Mesos does not believe are valid; but 
> that is not "failing on Unicode", we'd need to write our own validator for 
> Mesos.

I was just saying, if there's an easy way to try to interpret the string as 
normal ASCII and return `Error` if that fails, we should do that. If not, it's 
not that big of a deal, because Mesos will probably fail anyway.


- Alex


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


On Dec. 19, 2016, 11:20 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54877/
> ---
> 
> (Updated Dec. 19, 2016, 11:20 p.m.)
> 
> 
> Review request for mesos, Daniel Pravat, Alex Clemmer, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The API `SHGetKnownFolderPath` requires `Shell32.dll`,
> which is not available on Nano server.
> The equivalent API `GetAllUsersProfileDirectory`
> only requires `Userenv.dll`, which is available on Nano.
> 
> This API is also friendlier, as we own the allocation.
> 
> The Unicode version `GetAllUsersProfileDirectoryW` is
> explicitly used so that we are guaranteed a Unicode path,
> which we then convert from UTF-16 to UTF-8,
> instead of using the ASCII version which depends on a
> varying Windows code-page, and is not recommended.
> 
> A `vector` is used over a `wstring` to avoid dealing
> with the placement of the null-terminating character.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/windows.hpp 
> e641c46d033372e1b6c9f9c066b1ad4957d55088 
>   3rdparty/stout/include/stout/windows/os.hpp 
> 5cd92545a49648e39e8eb7cf131895e9cfc97902 

Re: Review Request 52639: Added test for `recovered` AgentID and `AGENT_ADDED` after reregister.

[Anand Mazumdar]

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


Fix it, then Ship it!




Looks good minus a few minor issues. I would fix them while committing.


src/tests/api_tests.cpp (line 1494)


s/Ensure/Ensure that
to be consistent with the comment on L1523



src/tests/master_tests.cpp (lines 4145 - 4146)


Let's make it consistent with a similar comment in the earlier test. Also, 
a newline after the comment since it applies to both the following code blocks.

```cpp
// After the agent has successfully re-registered with the master, the 
`recovered_slaves` field would be empty in both `/state` and `/slave` endpoints.
```



src/tests/master_tests.cpp (line 4158)


s/parse1/parse



src/tests/master_tests.cpp (line 4180)


s/parse1/parse


- Anand Mazumdar


On Dec. 30, 2016, 11:05 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52639/
> ---
> 
> (Updated Dec. 30, 2016, 11:05 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Xiaojian Huang, and Kunal Thakar.
> 
> 
> Bugs: MESOS-6177
> https://issues.apache.org/jira/browse/MESOS-6177
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test for `recovered` AgentID and `AGENT_ADDED` after reregister.
> 
> 
> Diffs
> -
> 
>   src/tests/api_tests.cpp e3138699aa883919329aee47a6d93b5a9a9794b2 
>   src/tests/master_tests.cpp 2d0cd8244ded44e76f0eee3d87327ff526db5208 
> 
> Diff: https://reviews.apache.org/r/52639/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 54877: Windows: Stout: Removed dependency on Shell API.

[Andrew Schwartzmeyer]

> On Dec. 20, 2016, 12:29 a.m., Daniel Pravat wrote:
> > 3rdparty/stout/include/stout/windows/os.hpp, line 748
> > 
> >
> > I don't think the conversion to UTF-8 is appropiate here.
> 
> Andrew Schwartzmeyer wrote:
> What would you convert it to? It's currently in UTF-16, and Windows paths 
> are allowed to have (almost) any Unicode character.
> 
> Alex Clemmer wrote:
> I think he's saying that bad things will happen if you use strings that 
> have Unicode in them, so it's probably better to just error out.
> 
> Andrew Schwartzmeyer wrote:
> I'm not following... That is a case our codebase must deal with, NTFS 
> paths are stored in Unicode:
> 
> 
> https://msdn.microsoft.com/en-us/library/windows/desktop/dd317748(v=vs.85).aspx
> 
> and on top of that, Windows file paths can:
> 
> > Use any character in the current code page for a name, including 
> Unicode characters and characters in the extended character set (128–255)
> 
> 
> https://msdn.microsoft.com/en-us/library/windows/desktop/aa365247(v=vs.85).aspx
> 
> So what I'm saying is that bad things _must not happen_ if we have paths 
> with non-ASCII characters in them; we need to handle that at any point we 
> deal with file paths on Windows.
> 
> I'm not saying this function is likely to return a path like `C:\ÂÑÐ¥`, 
> but that would be a valid file path, and so we must be treating all our paths 
> on Windows as Unicode.
> 
> If this is not the case, are purposefully constraining ourselves to 
> ASCII, and if so, why?
> 
> Alex Clemmer wrote:
> Sorry, I meant bad things will happen to Mesos if you give it strings 
> with unicode in them. Mesos itself does not deal with Unicode gracefully.
> 
> Andrew Schwartzmeyer wrote:
> Oooh. We should probably fix that.
> 
> Andrew Schwartzmeyer wrote:
> Alex, Daniel, do we have a resolution on this? I spoke with Alex and we 
> feel that we should be doing two main things: avoid exacberating the problem 
> in MESOS-6817 by being explicit in our use of `W` APIs, rather than relying 
> on the mercurial value of `TCHAR`; and not attempt to guard against paths 
> that Mesos does not handle, as it is a bug that Mesos does not handle paths 
> with valid extended characters (since both Linux and Windows may have these 
> paths), so failing here would be inappropriate as it hides the actual bug. 
> (Not to speak for you Alex, I think this summarizes our discussion yesterday.)
> 
> Alex Clemmer wrote:
> If I'm understanding our discussion correctly, failing here on unicode 
> conversion would be preferable, since it makes a subtle bug more obvious. If 
> that's nontrivial I think we can just ship it as is, since Mesos already has 
> these problems.

> failing here on unicode conversion

What would fail here?

I *think* you're implying to add a detection algorithm to see if there are any 
characters in the path that Mesos does not believe are valid; but that is not 
"failing on Unicode", we'd need to write our own validator for Mesos.


- Andrew


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


On Dec. 19, 2016, 11:20 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54877/
> ---
> 
> (Updated Dec. 19, 2016, 11:20 p.m.)
> 
> 
> Review request for mesos, Daniel Pravat, Alex Clemmer, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The API `SHGetKnownFolderPath` requires `Shell32.dll`,
> which is not available on Nano server.
> The equivalent API `GetAllUsersProfileDirectory`
> only requires `Userenv.dll`, which is available on Nano.
> 
> This API is also friendlier, as we own the allocation.
> 
> The Unicode version `GetAllUsersProfileDirectoryW` is
> explicitly used so that we are guaranteed a Unicode path,
> which we then convert from UTF-16 to UTF-8,
> instead of using the ASCII version which depends on a
> varying Windows code-page, and is not recommended.
> 
> A `vector` is used over a `wstring` to avoid dealing
> with the placement of the null-terminating character.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/windows.hpp 
> e641c46d033372e1b6c9f9c066b1ad4957d55088 
>   3rdparty/stout/include/stout/windows/os.hpp 
> 5cd92545a49648e39e8eb7cf131895e9cfc97902 
> 
> Diff: https://reviews.apache.org/r/54877/diff/
> 
> 
> Testing
> ---
> 
> cmake && msbuild, attach agent to master and check default `runtime_dir` 
> value.
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 54821: Refactored Docker::run() to make it only aware of docker cli options.

[Zhitao Li]

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

(Updated Jan. 3, 2017, 5:42 p.m.)


Review request for mesos, Xiaojian Huang, haosdent huang, and Jie Yu.


Changes
---

Intent nit fixes.


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


Repository: mesos


Description
---

This patch creates a wrapper struct for all recognizable docker cli
options, and separate logic of creating these options to a different
common function.

This also enables us to overcome gmock's 10 argument limit.

No logic change happens in this refactoring patch.


Diffs (updated)
-

  src/docker/docker.hpp 9093371afc8ea792ba94f61c6875703e547ea6b0 
  src/docker/docker.cpp 472cb1b4dc2b0ac65721c732fca8ec70a7470f47 
  src/docker/executor.cpp 9b5c469e2d0f33e228ec746711e6bc6ed352cbc7 
  src/slave/containerizer/docker.cpp 7a8a7271b54af0b4dcdae7a2aa8a90a8a7d05fd6 
  src/tests/containerizer/docker_containerizer_tests.cpp 
4e3b67bbb989f9084dfdf4970839956dcb0caa0e 
  src/tests/containerizer/docker_tests.cpp 
9667d434486c1832f180a297a39a3d5dae6a26bd 
  src/tests/mock_docker.hpp 829a760d54ad8c7b88256ae5df4c88c9fb18df71 
  src/tests/mock_docker.cpp 02b6065a01e7e52b0edb38676dfb1bb475584502 

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


Testing
---

`make check` with ROOT and DOCKER filter.


Thanks,

Zhitao Li

Re: Review Request 52877: Fixed wrong float serialization in JSON in locales different from C.

[Alexander Rojas]

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

(Updated Jan. 3, 2017, 5:06 p.m.)


Review request for mesos, Adam B, Benjamin Bannier, Joris Van Remoortere, and 
Michael Park.


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


Repository: mesos


Description
---

Resolves an isse with the JSON serialization functions where floats
would use the process active locale as a decimal separator instead
of the JSON conformant period (`.`).


Diffs
-

  3rdparty/stout/include/stout/json.hpp 
62ce15274677112d142a3c829b4a9f06258c9e2c 
  3rdparty/stout/include/stout/jsonify.hpp 
8220d001001e8b9f247c05be58534f1174611aad 

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


Testing (updated)
---

In order to test the different options the following benchmarking program was
written using [Google benchmark](https://github.com/google/benchmark):

```c++
#include 
#include 

#include 
#include 
#include 
#include 

static void BM_Jsonify(benchmark::State& state) {
  while (state.KeepRunning()) {
state.PauseTiming();

// Randomly generated set of numbers.
std::vector doubles = {
  54366462691.1798,
  3.35465250645312,
  3056184950.05953,
  74057564.7741182,
  280.445791893924,
  3446.63176368415,
  419012114.325581,
  3464212.51004162,
  1.45156507568354,
  13304750.4216248,
  7716457488648.00,
  700533630.650588,
  679.659801950981,
  307059152.688268,
  5615744.28063731,
  859.902033900705,
  293878.810967451,
  284685668.155903,
  722683811.462448,
  407.682284299325,
  9874834.88341080,
  4829649.14505646,
  3045544.72401146,
  1112707.08627010,
  8379539.79388719,
  3004161.89676627,
  382.79849617,
  3871151.73991937,
  4090119.26657417,
  4118699.88616345,
  2104416.18322520,
  9896898.63226234,
  5957851.08773457,
  3501068.52003430,
  7524714.36218293,
  4333874.01982647,
  9562008.06930384,
  3374957.45494027,
  5867075.07463260,
  815499.697450741,
  600936.470830026,
  9661425.72632153,
  6392256.71537575,
  7517969.33139398,
  9031912.03425553,
  5497593.85752735,
  815419.808032205,
  5098659.46057626,
  930160.667551563,
  5970944.98217500,
  6166534.92677787,
  3541537.67676275,
  1291933.13549156,
  9185094.72404290,
  4507338.03523123,
  9559754.89147696,
  6152898.39204769,
  2358966.41795446,
  6520510.92218183,
  2201757.78606032,
  4960487.80737309,
  1784969.91832029,
  3858390.23735070,
  1439952.27402359,
  6407199.91163080,
  6130379.95590661,
  6427890.23913244,
  2128879.29010338,
  8175291.42483598,
  1587278.27639235,
  7493343.68705307,
  4853439.25371342,
  2564845.15639735,
  1415661.63929173,
  6388168.79342734,
  3003424.90116775,
  6915390.10600792,
  7115390.65502377,
  5288515.90088063,
  1209208.86882085,
  4483111.91884606,
  5974377.97163572,
  5821054.89489766,
  8284136.84073623,
  1607044.34898051,
  3255087.31267763,
  2305369.43079747,
  1282392.57487249,
  4884797.49134467,
  5119420.62129117,
  6276725.07755672,
  3054999.92210194,
  3594116.58894982,
  6691568.49651968,
  265562.721872220,
  2864878.07276221,
  627299.902077148,
  5885179.44212665,
  7654144.98508934,
  590857.599685431
};

state.ResumeTiming();

benchmark::DoNotOptimize(jsonify(object));
  }
}

BENCHMARK(BM_Jsonify);

BENCHMARK_MAIN();
```

The program creates a JSON object with 50 floats in it, and then it creates
its string representation. Since the algorithms in `json.hpp` and `jsonify.hpp`
are similar but not the same, they were benchmarked as different. While the
original solutions are non solutions since they produce erroneous results if
localization is active, they constitute a baseline for comparison with the
alternatives. These are the benchmarks for the original algorithms and their
alternatives:

1. `json.hpp` default:

```c++
char buffer[50] {};
snprintf(
buffer,
sizeof(buffer),
"%#.*g",
std::numeric_limits::digits10,
number.value);

std::string trimmed = strings::trim(buffer, strings::SUFFIX, "0");
*stream_ << trimmed << (trimmed.back() == '.' ? "0" : "");
```

   Perfomance

```
Run on (8 X 2800 MHz CPU s)
2017-01-03 15:06:35
Benchmark   Time   CPU Iterations
-
BM_Jsonify985 ns986 ns 714227
```

2. `jsonify.hpp` default (it changes the call to `strings::trim` in order to
   avoid string copies:

```c++
char buffer[50] {};
const int size = snprintf(
buffer,

Re: Review Request 54999: Fixed test flakiness due to floating point conversions.

[Alexander Rukletsov]

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


Ship it!




Ship It!

- Alexander Rukletsov


On Dec. 28, 2016, 9:23 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54999/
> ---
> 
> (Updated Dec. 28, 2016, 9:23 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Bannier.
> 
> 
> Bugs: MESOS-6837
> https://issues.apache.org/jira/browse/MESOS-6837
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `FaultToleranceTest.FrameworkReregister` and
> `MasterTest.FailoverAgentReregisterFirst` both examined a timestamp
> value returned by an HTTP endpoint. Such values are the result of
> several conversions (`double` to `string` to `JSON::Value`); this might
> result in the returned value being an integer one larger/smaller than we
> expect. Hence, make the comparison within an epsilon of 1.
> 
> A similar issue in `SlaveTest.StateEndpoint` was fixed in
> 595c929f2816b713b4c36ce1bd23a7767afe8135.
> 
> 
> Diffs
> -
> 
>   src/tests/fault_tolerance_tests.cpp 
> 24747fab20f5b107b9c23a271b753e83f05bbee3 
>   src/tests/master_tests.cpp 2d0cd8244ded44e76f0eee3d87327ff526db5208 
>   src/tests/slave_tests.cpp 67a6aed8c66a21c94c106b52dff75cbdc41fcf69 
> 
> Diff: https://reviews.apache.org/r/54999/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Note that I wasn't able to repro the test failure on my laptop, but it was 
> observed on CI.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 54639: Implemented the 'create()' method of OCI store.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [52349, 55139, 55140, 52379, 54638, 52382, 54639]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker_build.sh

- Mesos ReviewBot


On Jan. 3, 2017, 2:19 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54639/
> ---
> 
> (Updated Jan. 3, 2017, 2:19 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-6681
> https://issues.apache.org/jira/browse/MESOS-6681
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented the 'create()' method of OCI store.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt c8d4260c03d8cdee1951a50d293e9fdabcd2cf84 
>   src/Makefile.am 6d0f77be37af9bc4e22199418796d6d0c5b6c462 
>   src/slave/containerizer/mesos/provisioner/oci/paths.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/provisioner/oci/paths.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/provisioner/oci/store.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/54639/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 52877: Fixed wrong float serialization in JSON in locales different from C.

[Alexander Rojas]

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

(Updated Jan. 3, 2017, 3:23 p.m.)


Review request for mesos, Adam B, Benjamin Bannier, Joris Van Remoortere, and 
Michael Park.


Changes
---

Rebases master.
Rewrites the tests as follows:
- Test with `jsonify` instead of `stringify` since usage of `JSON::Object` adds 
unnecesary time noise.
- Tests with a random set of strings to avoid compiler optimizations (the same 
doubles were being repeated over and over again).
- Updated proposals to their latest versions.


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


Repository: mesos


Description
---

Resolves an isse with the JSON serialization functions where floats
would use the process active locale as a decimal separator instead
of the JSON conformant period (`.`).


Diffs (updated)
-

  3rdparty/stout/include/stout/json.hpp 
62ce15274677112d142a3c829b4a9f06258c9e2c 
  3rdparty/stout/include/stout/jsonify.hpp 
8220d001001e8b9f247c05be58534f1174611aad 

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


Testing (updated)
---

In order to test the different options the following benchmarking program was
written using [Google benchmark](https://github.com/google/benchmark):

```c++
#include 
#include 

#include 
#include 
#include 
#include 

static void BM_Jsonify(benchmark::State& state) {
  while (state.KeepRunning()) {
state.PauseTiming();

// Randomly generated set of numbers.
std::vector doubles = {
  54366462691.1798,
  3.35465250645312,
  3056184950.05953,
  74057564.7741182,
  280.445791893924,
  3446.63176368415,
  419012114.325581,
  3464212.51004162,
  1.45156507568354,
  13304750.4216248,
  7716457488648.00,
  700533630.650588,
  679.659801950981,
  307059152.688268,
  5615744.28063731,
  859.902033900705,
  293878.810967451,
  284685668.155903,
  722683811.462448,
  407.682284299325,
  9874834.88341080,
  4829649.14505646,
  3045544.72401146,
  1112707.08627010,
  8379539.79388719,
  3004161.89676627,
  382.79849617,
  3871151.73991937,
  4090119.26657417,
  4118699.88616345,
  2104416.18322520,
  9896898.63226234,
  5957851.08773457,
  3501068.52003430,
  7524714.36218293,
  4333874.01982647,
  9562008.06930384,
  3374957.45494027,
  5867075.07463260,
  815499.697450741,
  600936.470830026,
  9661425.72632153,
  6392256.71537575,
  7517969.33139398,
  9031912.03425553,
  5497593.85752735,
  815419.808032205,
  5098659.46057626,
  930160.667551563,
  5970944.98217500,
  6166534.92677787,
  3541537.67676275,
  1291933.13549156,
  9185094.72404290,
  4507338.03523123,
  9559754.89147696,
  6152898.39204769,
  2358966.41795446,
  6520510.92218183,
  2201757.78606032,
  4960487.80737309,
  1784969.91832029,
  3858390.23735070,
  1439952.27402359,
  6407199.91163080,
  6130379.95590661,
  6427890.23913244,
  2128879.29010338,
  8175291.42483598,
  1587278.27639235,
  7493343.68705307,
  4853439.25371342,
  2564845.15639735,
  1415661.63929173,
  6388168.79342734,
  3003424.90116775,
  6915390.10600792,
  7115390.65502377,
  5288515.90088063,
  1209208.86882085,
  4483111.91884606,
  5974377.97163572,
  5821054.89489766,
  8284136.84073623,
  1607044.34898051,
  3255087.31267763,
  2305369.43079747,
  1282392.57487249,
  4884797.49134467,
  5119420.62129117,
  6276725.07755672,
  3054999.92210194,
  3594116.58894982,
  6691568.49651968,
  265562.721872220,
  2864878.07276221,
  627299.902077148,
  5885179.44212665,
  7654144.98508934,
  590857.599685431
};

state.ResumeTiming();

benchmark::DoNotOptimize(jsonify(object));
  }
}

BENCHMARK(BM_Jsonify);

BENCHMARK_MAIN();
```

The program creates a JSON object with 50 floats in it, and then it creates
its string representation. Since the algorithms in `json.hpp` and `jsonify.hpp`
are similar but not the same, they were benchmarked as different. While the
original solutions are non solutions since they produce erroneous results if
localization is active, they constitute a baseline for comparison with the
alternatives. These are the benchmarks for the original algorithms and their
alternatives:

1. `json.hpp` default:

```c++
char buffer[50] {};
snprintf(
buffer,
sizeof(buffer),
"%#.*g",
std::numeric_limits::digits10,
number.value);

std::string trimmed = strings::trim(buffer, strings::SUFFIX, "0");
*stream_ << trimmed << (trimmed.back() == '.' ? "0" : "");
```

   Perfomance

```
Run on (8 X 2800 MHz CPU s)

Re: Review Request 54639: Implemented the 'create()' method of OCI store.

[Qian Zhang]

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

(Updated Jan. 3, 2017, 10:19 p.m.)


Review request for mesos, Gilbert Song and Jie Yu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Implemented the 'create()' method of OCI store.


Diffs (updated)
-

  src/CMakeLists.txt c8d4260c03d8cdee1951a50d293e9fdabcd2cf84 
  src/Makefile.am 6d0f77be37af9bc4e22199418796d6d0c5b6c462 
  src/slave/containerizer/mesos/provisioner/oci/paths.hpp PRE-CREATION 
  src/slave/containerizer/mesos/provisioner/oci/paths.cpp PRE-CREATION 
  src/slave/containerizer/mesos/provisioner/oci/store.cpp PRE-CREATION 

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


Testing
---


Thanks,

Qian Zhang

Re: Review Request 52382: Added stubs for OCI store.

[Qian Zhang]

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

(Updated Jan. 3, 2017, 10:18 p.m.)


Review request for mesos, Gilbert Song and Jie Yu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Added stubs for OCI store.


Diffs (updated)
-

  src/CMakeLists.txt c8d4260c03d8cdee1951a50d293e9fdabcd2cf84 
  src/Makefile.am 6d0f77be37af9bc4e22199418796d6d0c5b6c462 
  src/slave/containerizer/mesos/provisioner/oci/store.hpp PRE-CREATION 
  src/slave/containerizer/mesos/provisioner/oci/store.cpp PRE-CREATION 

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


Testing
---


Thanks,

Qian Zhang

Re: Review Request 54638: Added agent flags '--oci_discovery' and '--oci_discovery_prefix'.

[Qian Zhang]

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

(Updated Jan. 3, 2017, 10:18 p.m.)


Review request for mesos, Gilbert Song and Jie Yu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Added agent flags '--oci_discovery' and '--oci_discovery_prefix'.


Diffs (updated)
-

  docs/configuration.md e4beb2d5a72f1c5f59b2e40f4984cc60b8437d9d 
  src/slave/flags.hpp 6ac0d45072157f6741b96266886a326e9337c153 
  src/slave/flags.cpp 1eccea920338032173be4df6c374ec50dbd2eaf3 

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


Testing
---


Thanks,

Qian Zhang

Review Request 55139: Implemented parse methods for OCI image spec.

[Qian Zhang]

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

Review request for mesos, Gilbert Song and Jie Yu.


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


Repository: mesos


Description
---

Implemented parse methods for OCI image spec.


Diffs
-

  include/mesos/oci/spec.hpp PRE-CREATION 
  src/CMakeLists.txt c8d4260c03d8cdee1951a50d293e9fdabcd2cf84 
  src/Makefile.am 6d0f77be37af9bc4e22199418796d6d0c5b6c462 
  src/oci/spec.cpp PRE-CREATION 

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


Testing
---


Thanks,

Qian Zhang

Re: Review Request 52379: Added agent flag '--oci_store_dir'.

[Qian Zhang]

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

(Updated Jan. 3, 2017, 10:17 p.m.)


Review request for mesos, Gilbert Song and Jie Yu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Added agent flag '--oci_store_dir'.


Diffs (updated)
-

  docs/configuration.md e4beb2d5a72f1c5f59b2e40f4984cc60b8437d9d 
  docs/endpoints/slave/state.json.md 0f82c1926404e79b281b2ea5f4d0ca21323aeded 
  docs/endpoints/slave/state.md b34459e8624f0b29e927ff79be7fc845ac88080b 
  src/slave/flags.hpp 6ac0d45072157f6741b96266886a326e9337c153 
  src/slave/flags.cpp 1eccea920338032173be4df6c374ec50dbd2eaf3 
  src/slave/http.cpp b6e2d4f9190358d113b39140d116b8659ddbb49b 

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


Testing
---


Thanks,

Qian Zhang

Review Request 55140: Added tests for parsing OCI image spec.

[Qian Zhang]

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

Review request for mesos, Gilbert Song and Jie Yu.


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


Repository: mesos


Description
---

Added tests for parsing OCI image spec.


Diffs
-

  src/Makefile.am 6d0f77be37af9bc4e22199418796d6d0c5b6c462 
  src/tests/CMakeLists.txt b034f1b88337b003d01450eadd262b9bc763545c 
  src/tests/containerizer/oci_spec_tests.cpp PRE-CREATION 

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


Testing
---


Thanks,

Qian Zhang

Re: Review Request 52349: Add protobuf messages for OCI image spec.

[Qian Zhang]

> On Dec. 16, 2016, 8:32 a.m., Jie Yu wrote:
> > include/mesos/oci/spec.proto, line 90
> > 
> >
> > How do we support parsing `annotations`?
> > 
> > Should we use a different name so we can do manual parsing?
> > 
> > Also, i'd like to introduce a common `Label` message instead of 
> > `EntryWithStringValue`
> 
> Qian Zhang wrote:
> > How do we support parsing `annotations`?
> 
> That's the suggested way to handle `map` in protobuf, see 
> https://developers.google.com/protocol-buffers/docs/proto#maps (the backwards 
> compatible way) for details, I think `annotations` will be automatically 
> parsed in this way.
> 
> > Also, i'd like to introduce a common `Label` message instead of 
> `EntryWithStringValue`
> 
> Do you mean we rename `EntryWithStringValue` to `Label` and define it in 
> a common place rather than in this file?
> 
> Jie Yu wrote:
> Ah, didn't realize that Map is already supported in proto2. We definitely 
> need to add parsing support for that (i.e., json -> protobuf). Not sure how 
> backwards compatibility will be handled. However, if it is used in agent 
> only, it should not be a problem.
> 
> Qian Zhang wrote:
> The protobuf that we are using (protobuf-2.6.1) does not support `map`, 
> as MPark mentioned in the dev list, we need to upgrade to protobuf 3.0.0 to 
> have `map` support. Do you think we need to do the upgrade right away, or we 
> can use the backwards compatible way mentioned in the above link first in 
> this project and do the upgrade later?

Let's follow the similar way of https://reviews.apache.org/r/47199/ to do the 
manual parsing for now, and in future when we upgrade the protobuf to a newer 
version in Mesos to support `Map`, we can remove that manual parsing logic and 
just use the `Map` keyword.


- Qian


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


On Jan. 3, 2017, 9:53 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52349/
> ---
> 
> (Updated Jan. 3, 2017, 9:53 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6681
> https://issues.apache.org/jira/browse/MESOS-6681
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add protobuf messages for OCI image spec.
> 
> 
> Diffs
> -
> 
>   include/mesos/oci/spec.hpp PRE-CREATION 
>   include/mesos/oci/spec.proto PRE-CREATION 
>   src/Makefile.am 6d0f77be37af9bc4e22199418796d6d0c5b6c462 
> 
> Diff: https://reviews.apache.org/r/52349/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 52349: Add protobuf messages for OCI image spec.

[Qian Zhang]

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

(Updated Jan. 3, 2017, 9:53 p.m.)


Review request for mesos and Jie Yu.


Changes
---

Addressed Jie and Avinash's comments.


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


Repository: mesos


Description
---

Add protobuf messages for OCI image spec.


Diffs (updated)
-

  include/mesos/oci/spec.hpp PRE-CREATION 
  include/mesos/oci/spec.proto PRE-CREATION 
  src/Makefile.am 6d0f77be37af9bc4e22199418796d6d0c5b6c462 

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


Testing
---


Thanks,

Qian Zhang