Re: Review Request 38321: Adding upgrade specifications to docs for '.json' endpoints

2015-09-12 Thread Isabel Jimenez 

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

(Updated Sept. 12, 2015, 6:08 p.m.)


Review request for mesos and Adam B.


Repository: mesos


Description
---

Adding a Note for the deprecation of endpoints with '.json' extension.


Diffs (updated)
-

  docs/upgrades.md c4b2880 

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


Testing
---


Thanks,

Isabel Jimenez

Re: Review Request 37168: MESOS-3063 (Add an example framework using dynamic reservation)

2015-09-12 Thread Michael Park 

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


Some preliminary comments.


src/examples/dynamic_reservation_framework.cpp (line 40)


Remove newline.



src/examples/dynamic_reservation_framework.cpp (line 49)


Not used.



src/examples/dynamic_reservation_framework.cpp (lines 100 - 102)


Shouldn't we also make sure we only reserve `TASK_RESOURCES` amount of 
resources on only `totalTasks` number of agents?



src/examples/dynamic_reservation_framework.cpp (lines 111 - 112)


It seems like we should stay in the `START` state until we've reserved 
`TASK_RESOURCES` amount of resources for this agent before transitioning to the 
`RESERVED` state, right?

Looking at the `reserveResources` function, it doesn't even perform the 
`acceptOffers` if the offer resources doesn't contain sufficient resources. 
Shouldn't the `state = State::RESERVED` be dependent on the result of 
`reserveResources`?



src/examples/dynamic_reservation_framework.cpp (line 180)


`s/sid/slaveId/`



src/examples/dynamic_reservation_framework.cpp (line 183)


`s/executorID/executorId/`



src/examples/dynamic_reservation_framework.cpp (line 184)


`s/slaveID/slaveId/`



src/examples/dynamic_reservation_framework.cpp (line 228)


Why is this called `remaining`? I would think the "remaining" portion is 
`TASK_RESOURCES` - amount of already reserved resources?



src/examples/dynamic_reservation_framework.cpp (line 229)


We should be careful here, as `find` does not do exact match. It can return 
resources with a non-matching role, for example.



src/examples/dynamic_reservation_framework.cpp (line 242)


Why does this function perform side-effects? I think this function should 
look exactly the same as the one in `src/tests/mesos.hpp`.



src/examples/dynamic_reservation_framework.cpp (line 252)


It doesn't makes sense to print this here as this function doesn't actually 
reserve any resources.



src/examples/dynamic_reservation_framework.cpp (line 257)


Same comment as `RESERVE`.


- Michael Park


On Sept. 6, 2015, 4:11 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37168/
> ---
> 
> (Updated Sept. 6, 2015, 4:11 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-3063
> https://issues.apache.org/jira/browse/MESOS-3063
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Provide example for dynamic reservation features.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 5fdca0f 
>   src/examples/dynamic_reservation_framework.cpp PRE-CREATION 
>   src/tests/dynamic_reservation_framework_test.sh PRE-CREATION 
>   src/tests/examples_tests.cpp 3f56b30 
>   src/tests/flags.hpp 06da36d 
>   src/tests/script.cpp bcc1fab 
> 
> Diff: https://reviews.apache.org/r/37168/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 38321: Adding upgrade specifications to docs for '.json' endpoints

2015-09-12 Thread Mesos ReviewBot 

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


Patch looks great!

Reviews applied: [36126, 36125, 36127, 38321]

All tests passed.

- Mesos ReviewBot


On Sept. 12, 2015, 6:08 p.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38321/
> ---
> 
> (Updated Sept. 12, 2015, 6:08 p.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adding a Note for the deprecation of endpoints with '.json' extension.
> 
> 
> Diffs
> -
> 
>   docs/upgrades.md c4b2880 
> 
> Diff: https://reviews.apache.org/r/38321/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 37873: Add quiesce logic in allocator

2015-09-12 Thread Robert Lacroix 

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

Ship it!


Ship It!

- Robert Lacroix


On Sept. 6, 2015, 2:40 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37873/
> ---
> 
> (Updated Sept. 6, 2015, 2:40 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Ben Mahler, Jie Yu, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-3037
> https://issues.apache.org/jira/browse/MESOS-3037
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add quiesce logic in allocator
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 659f37b3f9d9fa02da9bdb6c85cd3c180a24b73a 
>   src/master/allocator/mesos/allocator.hpp 
> aa55755a9c3250579e9366bdbc17a2449e95d659 
>   src/master/allocator/mesos/hierarchical.hpp 
> cb4020dea897ef198cd9898cabecf61edfade834 
>   src/master/master.cpp 5589eca4317b597de509f3387cfc349083b361ac 
>   src/tests/mesos.hpp 906948d459b5a88a4ad7952801eb8c540b58c569 
> 
> Diff: https://reviews.apache.org/r/37873/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 38259: [MESOS-3340] Command-line flags should take precedence over OS Env variables

2015-09-12 Thread Klaus Ma 

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

(Updated Sept. 13, 2015, 4:34 a.m.)


Review request for mesos, Bernd Mathiske, Marco Massenzio, and Michael Park.


Changes
---

Update the code to overwrite env by cli


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


Repository: mesos


Description
---

Currently, it appears that re-defining a flag on the command-line that was 
already defined via a OS Env var (MESOS_*) causes the Master to fail with a not 
very helpful message.

For example, if one has MESOS_QUORUM defined, this happens:

$ ./mesos-master --zk=zk://192.168.1.4/mesos --quorum=1 
--hostname=192.168.1.4 --ip=192.168.1.4
Duplicate flag 'quorum' on command line

which is not very helpful.

Current solution is to throw error if any duplication; over-write may make user 
confused about the result.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 9da213f 
  3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp ebf8cd6 

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


Testing
---

make
make check


Thanks,

Klaus Ma

Re: Review Request 38329: Ensured that the device containing slave work_dir is a shared mount when LinuxFilesystemIsolator is used.

2015-09-12 Thread Jie Yu 


> On Sept. 12, 2015, 6:38 a.m., Timothy Chen wrote:
> > src/slave/containerizer/isolators/filesystem/linux.cpp, line 91
> > 
> >
> > Should we check we did successfully found one that we can mark shared?
> 
> Jie Yu wrote:
> Good catch! Yeah, let me at that check!

In fact, that should be a CHECK because we should always find one.


- Jie


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


On Sept. 12, 2015, 6:36 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38329/
> ---
> 
> (Updated Sept. 12, 2015, 6:36 a.m.)
> 
> 
> Review request for mesos, haosdent huang, Timothy Chen, Vinod Kone, and Jiang 
> Yan Xu.
> 
> 
> Bugs: MESOS-3349
> https://issues.apache.org/jira/browse/MESOS-3349
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Ensured that the device containing slave work_dir is a shared mount when 
> LinuxFilesystemIsolator is used.
> 
> See the discussion thread in the ticket for motivation (also the NOTE in the 
> code).
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/isolators/filesystem/linux.cpp 
> 0970b3d48b13d5e9d2e0160df5cf14a3dcd0acc9 
> 
> Diff: https://reviews.apache.org/r/38329/diff/
> 
> 
> Testing
> ---
> 
> sudo make check on CentOS5 and CentOS6
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 38328: Added a helper to get stat.st_dev in stout.

2015-09-12 Thread haosdent huang 

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

Ship it!


Ship It!

- haosdent huang


On Sept. 12, 2015, 6:36 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38328/
> ---
> 
> (Updated Sept. 12, 2015, 6:36 a.m.)
> 
> 
> Review request for mesos, haosdent huang, Timothy Chen, Vinod Kone, and Jiang 
> Yan Xu.
> 
> 
> Bugs: MESOS-3349
> https://issues.apache.org/jira/browse/MESOS-3349
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a helper to get stat.st_dev in stout.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/stat.hpp 
> c7084f1d3672f5610de1cb47e275cb67a9cac1d5 
> 
> Diff: https://reviews.apache.org/r/38328/diff/
> 
> 
> Testing
> ---
> 
> trivial.
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 38329: Ensured that the device containing slave work_dir is a shared mount when LinuxFilesystemIsolator is used.

2015-09-12 Thread haosdent huang 


> On Sept. 12, 2015, 6:38 a.m., Timothy Chen wrote:
> > src/slave/containerizer/isolators/filesystem/linux.cpp, line 91
> > 
> >
> > Should we check we did successfully found one that we can mark shared?
> 
> Jie Yu wrote:
> Good catch! Yeah, let me at that check!
> 
> Jie Yu wrote:
> In fact, that should be a CHECK because we should always find one.

In fact, we have another way. We could 

```
fs::mount(target, target, None(), MS_BIND, NULL);
fs::mount(None(), target, None(), MS_SHARED, NULL);
```

to mount self. So that we don't need affect the exist system mount points.


- haosdent


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


On Sept. 12, 2015, 6:36 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38329/
> ---
> 
> (Updated Sept. 12, 2015, 6:36 a.m.)
> 
> 
> Review request for mesos, haosdent huang, Timothy Chen, Vinod Kone, and Jiang 
> Yan Xu.
> 
> 
> Bugs: MESOS-3349
> https://issues.apache.org/jira/browse/MESOS-3349
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Ensured that the device containing slave work_dir is a shared mount when 
> LinuxFilesystemIsolator is used.
> 
> See the discussion thread in the ticket for motivation (also the NOTE in the 
> code).
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/isolators/filesystem/linux.cpp 
> 0970b3d48b13d5e9d2e0160df5cf14a3dcd0acc9 
> 
> Diff: https://reviews.apache.org/r/38329/diff/
> 
> 
> Testing
> ---
> 
> sudo make check on CentOS5 and CentOS6
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 38329: Ensured that the device containing slave work_dir is a shared mount when LinuxFilesystemIsolator is used.

2015-09-12 Thread Jie Yu 


> On Sept. 12, 2015, 6:38 a.m., Timothy Chen wrote:
> > src/slave/containerizer/isolators/filesystem/linux.cpp, line 91
> > 
> >
> > Should we check we did successfully found one that we can mark shared?
> 
> Jie Yu wrote:
> Good catch! Yeah, let me at that check!
> 
> Jie Yu wrote:
> In fact, that should be a CHECK because we should always find one.
> 
> haosdent huang wrote:
> In fact, we have another way. We could 
> 
> ```
> fs::mount(target, target, None(), MS_BIND, NULL);
> fs::mount(None(), target, None(), MS_SHARED, NULL);
> ```
> 
> to mount self. So that we don't need affect the exist system mount points.

I thought about that too. But that's more complicated to me because we need to 
do a recursive self bind mount of the work_dir (in case volumes have already 
been mounted, think about the recovery case), which sounds more invasive to me.


- Jie


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


On Sept. 12, 2015, 6:36 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38329/
> ---
> 
> (Updated Sept. 12, 2015, 6:36 a.m.)
> 
> 
> Review request for mesos, haosdent huang, Timothy Chen, Vinod Kone, and Jiang 
> Yan Xu.
> 
> 
> Bugs: MESOS-3349
> https://issues.apache.org/jira/browse/MESOS-3349
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Ensured that the device containing slave work_dir is a shared mount when 
> LinuxFilesystemIsolator is used.
> 
> See the discussion thread in the ticket for motivation (also the NOTE in the 
> code).
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/isolators/filesystem/linux.cpp 
> 0970b3d48b13d5e9d2e0160df5cf14a3dcd0acc9 
> 
> Diff: https://reviews.apache.org/r/38329/diff/
> 
> 
> Testing
> ---
> 
> sudo make check on CentOS5 and CentOS6
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 38329: Ensured that the device containing slave work_dir is a shared mount when LinuxFilesystemIsolator is used.

2015-09-12 Thread haosdent huang 

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

Ship it!


Ship It!


src/slave/containerizer/isolators/filesystem/linux.cpp (line 190)


"parent mount" should be "parent mount point"?


- haosdent huang


On Sept. 12, 2015, 7:26 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38329/
> ---
> 
> (Updated Sept. 12, 2015, 7:26 a.m.)
> 
> 
> Review request for mesos, haosdent huang, Timothy Chen, Vinod Kone, and Jiang 
> Yan Xu.
> 
> 
> Bugs: MESOS-3349
> https://issues.apache.org/jira/browse/MESOS-3349
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Ensured that the device containing slave work_dir is a shared mount when 
> LinuxFilesystemIsolator is used.
> 
> See the discussion thread in the ticket for motivation (also the NOTE in the 
> code).
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/isolators/filesystem/linux.cpp 
> 0970b3d48b13d5e9d2e0160df5cf14a3dcd0acc9 
> 
> Diff: https://reviews.apache.org/r/38329/diff/
> 
> 
> Testing
> ---
> 
> sudo make check on CentOS5 and CentOS6
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Review Request 38328: Added a helper to get stat.st_dev in stout.

2015-09-12 Thread Jie Yu 

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

Review request for mesos, haosdent huang, Timothy Chen, Vinod Kone, and Jiang 
Yan Xu.


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


Repository: mesos


Description
---

Added a helper to get stat.st_dev in stout.


Diffs
-

  3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/stat.hpp 
c7084f1d3672f5610de1cb47e275cb67a9cac1d5 

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


Testing
---

trivial.


Thanks,

Jie Yu

Review Request 38329: Ensured that the device containing slave work_dir is a shared mount when LinuxFilesystemIsolator is used.

2015-09-12 Thread Jie Yu 

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

Review request for mesos, haosdent huang, Timothy Chen, Vinod Kone, and Jiang 
Yan Xu.


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


Repository: mesos


Description
---

Ensured that the device containing slave work_dir is a shared mount when 
LinuxFilesystemIsolator is used.

See the discussion thread in the ticket for motivation (also the NOTE in the 
code).


Diffs
-

  src/slave/containerizer/isolators/filesystem/linux.cpp 
0970b3d48b13d5e9d2e0160df5cf14a3dcd0acc9 

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


Testing
---

sudo make check on CentOS5 and CentOS6


Thanks,

Jie Yu

Re: Review Request 38329: Ensured that the device containing slave work_dir is a shared mount when LinuxFilesystemIsolator is used.

2015-09-12 Thread Timothy Chen 

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



src/slave/containerizer/isolators/filesystem/linux.cpp (line 91)


Should we check we did successfully found one that we can mark shared?


- Timothy Chen


On Sept. 12, 2015, 6:36 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38329/
> ---
> 
> (Updated Sept. 12, 2015, 6:36 a.m.)
> 
> 
> Review request for mesos, haosdent huang, Timothy Chen, Vinod Kone, and Jiang 
> Yan Xu.
> 
> 
> Bugs: MESOS-3349
> https://issues.apache.org/jira/browse/MESOS-3349
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Ensured that the device containing slave work_dir is a shared mount when 
> LinuxFilesystemIsolator is used.
> 
> See the discussion thread in the ticket for motivation (also the NOTE in the 
> code).
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/isolators/filesystem/linux.cpp 
> 0970b3d48b13d5e9d2e0160df5cf14a3dcd0acc9 
> 
> Diff: https://reviews.apache.org/r/38329/diff/
> 
> 
> Testing
> ---
> 
> sudo make check on CentOS5 and CentOS6
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 38329: Ensured that the device containing slave work_dir is a shared mount when LinuxFilesystemIsolator is used.

2015-09-12 Thread Jie Yu 


> On Sept. 12, 2015, 6:38 a.m., Timothy Chen wrote:
> > src/slave/containerizer/isolators/filesystem/linux.cpp, line 91
> > 
> >
> > Should we check we did successfully found one that we can mark shared?

Good catch! Yeah, let me at that check!


- Jie


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


On Sept. 12, 2015, 6:36 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38329/
> ---
> 
> (Updated Sept. 12, 2015, 6:36 a.m.)
> 
> 
> Review request for mesos, haosdent huang, Timothy Chen, Vinod Kone, and Jiang 
> Yan Xu.
> 
> 
> Bugs: MESOS-3349
> https://issues.apache.org/jira/browse/MESOS-3349
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Ensured that the device containing slave work_dir is a shared mount when 
> LinuxFilesystemIsolator is used.
> 
> See the discussion thread in the ticket for motivation (also the NOTE in the 
> code).
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/isolators/filesystem/linux.cpp 
> 0970b3d48b13d5e9d2e0160df5cf14a3dcd0acc9 
> 
> Diff: https://reviews.apache.org/r/38329/diff/
> 
> 
> Testing
> ---
> 
> sudo make check on CentOS5 and CentOS6
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 38329: Ensured that the device containing slave work_dir is a shared mount when LinuxFilesystemIsolator is used.

2015-09-12 Thread Jie Yu 

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

(Updated Sept. 12, 2015, 7:26 a.m.)


Review request for mesos, haosdent huang, Timothy Chen, Vinod Kone, and Jiang 
Yan Xu.


Changes
---

Review comments.

Moved the logics to recover because we want to make sure work_dir exists.


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


Repository: mesos


Description
---

Ensured that the device containing slave work_dir is a shared mount when 
LinuxFilesystemIsolator is used.

See the discussion thread in the ticket for motivation (also the NOTE in the 
code).


Diffs (updated)
-

  src/slave/containerizer/isolators/filesystem/linux.cpp 
0970b3d48b13d5e9d2e0160df5cf14a3dcd0acc9 

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


Testing
---

sudo make check on CentOS5 and CentOS6


Thanks,

Jie Yu

Re: Review Request 38329: Ensured that the device containing slave work_dir is a shared mount when LinuxFilesystemIsolator is used.

2015-09-12 Thread haosdent huang 


> On Sept. 12, 2015, 6:38 a.m., Timothy Chen wrote:
> > src/slave/containerizer/isolators/filesystem/linux.cpp, line 91
> > 
> >
> > Should we check we did successfully found one that we can mark shared?
> 
> Jie Yu wrote:
> Good catch! Yeah, let me at that check!
> 
> Jie Yu wrote:
> In fact, that should be a CHECK because we should always find one.
> 
> haosdent huang wrote:
> In fact, we have another way. We could 
> 
> ```
> fs::mount(target, target, None(), MS_BIND, NULL);
> fs::mount(None(), target, None(), MS_SHARED, NULL);
> ```
> 
> to mount self. So that we don't need affect the exist system mount points.
> 
> Jie Yu wrote:
> I thought about that too. But that's more complicated to me because we 
> need to do a recursive self bind mount of the work_dir (in case volumes have 
> already been mounted, think about the recovery case), which sounds more 
> invasive to me.

Oh, thank you for explanation.


- haosdent


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


On Sept. 12, 2015, 6:36 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38329/
> ---
> 
> (Updated Sept. 12, 2015, 6:36 a.m.)
> 
> 
> Review request for mesos, haosdent huang, Timothy Chen, Vinod Kone, and Jiang 
> Yan Xu.
> 
> 
> Bugs: MESOS-3349
> https://issues.apache.org/jira/browse/MESOS-3349
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Ensured that the device containing slave work_dir is a shared mount when 
> LinuxFilesystemIsolator is used.
> 
> See the discussion thread in the ticket for motivation (also the NOTE in the 
> code).
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/isolators/filesystem/linux.cpp 
> 0970b3d48b13d5e9d2e0160df5cf14a3dcd0acc9 
> 
> Diff: https://reviews.apache.org/r/38329/diff/
> 
> 
> Testing
> ---
> 
> sudo make check on CentOS5 and CentOS6
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 38329: Ensured that the device containing slave work_dir is a shared mount when LinuxFilesystemIsolator is used.

2015-09-12 Thread Mesos ReviewBot 

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


Patch looks great!

Reviews applied: [38328, 38329]

All tests passed.

- Mesos ReviewBot


On Sept. 12, 2015, 7:26 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38329/
> ---
> 
> (Updated Sept. 12, 2015, 7:26 a.m.)
> 
> 
> Review request for mesos, haosdent huang, Timothy Chen, Vinod Kone, and Jiang 
> Yan Xu.
> 
> 
> Bugs: MESOS-3349
> https://issues.apache.org/jira/browse/MESOS-3349
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Ensured that the device containing slave work_dir is a shared mount when 
> LinuxFilesystemIsolator is used.
> 
> See the discussion thread in the ticket for motivation (also the NOTE in the 
> code).
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/isolators/filesystem/linux.cpp 
> 0970b3d48b13d5e9d2e0160df5cf14a3dcd0acc9 
> 
> Diff: https://reviews.apache.org/r/38329/diff/
> 
> 
> Testing
> ---
> 
> sudo make check on CentOS5 and CentOS6
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 38137: Added Docker provisioner and local store

2015-09-12 Thread Jiang Yan Xu 


> On Sept. 10, 2015, 3:04 a.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/provisioners/docker.cpp, lines 104-107
> > 
> >
> > I guess you are modeling this after the `docker pull` syntax here: 
> > `[REGISTRY_HOST[:REGISTRY_PORT]/]NAME[:TAG]`
> > 
> > I found that some docker help pages have conflicting use of repository 
> > vs. name and we should probably refer to the spec.
> > 
> > I think the following questions need to be answered:
> > 
> > 1) If we use image name as the indentifier, it defines "what the image 
> > is". The registry specifies "where the image comes from". If an identical 
> > image is hosted on two registries, should they have the same name?
> > 
> > 2) ID. `docker pull` supports @sha256 syntax and `repositories` file 
> > (which our docker metadata file is modeled after) maps repo:tag to an image 
> > ID. Shouldn't the ID be part of the name? If ubuntu:latest changes when use 
> > releases come out, I don't think repo:tag uniquely identifies the image.
> 
> Timothy Chen wrote:
> I think I'm trying to conform to the docker client experience, which is 
> what users perceive is the standard.
> Although docker supports sha256 syntax, users still all the time refers 
> to images as [registry_host:port]/repository:tag. When images are conflicting 
> between two registries, the user is usually impliciting defining which one to 
> pick based on the registry source. If user swithces the default registry and 
> we are now not in sync, this is at least identical to what docker client 
> behaves today and users should expect the same. We will provide comments in 
> the user guide.
> 
> The current docker client simpliy matches the full name like we do in the 
> code here, and caches that tag until a docker pull happens that attempts to 
> get the latest tag.
> 
> And yes Docker doesn't consistently name things correctly themselves :( 
> I'm going with the Docker registry API naming for now, which is repository 
> instead of name.
> 
> Jiang Yan Xu wrote:
> I am all for not diverging from the "standard" docker user experience 
> here but I think 
> 1) We have to support specifying image by ID.
> 
> Docker's tags are imprecise and mutable, as a large organization running 
> production services we have to have the option to be very precise about what 
> to deploy. If users don't want to be precise, they don't have to use it.
> 
> 2) From what I can tell, registry is not persisted in the "repositories" 
> file, so how does Docker engine know the locally cached image with the same 
> repo:tag matches a request for "myregistry/repo:tag" or not?
> 
> I am actually digging through docker code so feel free to share the link.

I take back my point 2). Registry is **part of** of the "repo".


- Jiang Yan


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


On Sept. 11, 2015, 12:42 a.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38137/
> ---
> 
> (Updated Sept. 11, 2015, 12:42 a.m.)
> 
> 
> Review request for mesos, Jie Yu, Jojy Varghese, Till Toenshoff, and Jiang 
> Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Joining all the commits around provisioner and local store into one review so 
> it's easier to review, as patches
> are changing code on top of each other.
> 
> All the commits are going to committed together.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 8963cea 
>   src/slave/containerizer/provisioner.hpp 9e0e0b8 
>   src/slave/containerizer/provisioner.cpp 2ac9008 
>   src/slave/containerizer/provisioners/docker/local_store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/local_store.cpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/message.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/message.proto PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/metadata_manager.hpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/metadata_manager.cpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/paths.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/paths.cpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/provisioner.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/provisioner.cpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/store.cpp PRE-CREATION 
>   src/slave/flags.hpp 799c963 
>   src/slave/flags.cpp b676bac 
>

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

2015-09-12 Thread Adam B 


> On July 7, 2015, 11:40 a.m., Vinod Kone wrote:
> > CHANGELOG, line 337
> > 
> >
> > I would just put this under deprecations section.
> > 
> > Also, mind updating MESOS-2058 in deprecation section to do 
> > s/deprecate/remove/ because its been removed.
> 
> Jiang Yan Xu wrote:
> Adam is against putting MESOS-2640 in 0.23.0 because it's committed after 
> rc1 is cut. This review doesn't even need to land now.
> 
> s/deprecate/remove/ on MESOS-2058 can be done separately.
> 
> Vinod Kone wrote:
> SG. Yea I wanted a WIP 24.0 CHANGELOG too when I originially made that 
> comment on the older review. Forgot about that.

@xujyan: I don't think this ever got added to the CHANGELOG, even with 0.24 
released now. Can you please revive this patch so we can update the changelog 
appropriately? Thanks.


- Adam


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


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

Re: Review Request 21713: Route overload to add auth HTTP Response + tests

2015-09-12 Thread Adam B 


> On June 3, 2014, 10:29 a.m., Adam B wrote:
> > Great stuff. Just a couple of comments from me.

@ijimenez Is this patch still necessary? If not, let's discard it. If so, 
please rebase and revive it. Thanks.


- Adam


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


On May 20, 2014, 10:20 a.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21713/
> ---
> 
> (Updated May 20, 2014, 10:20 a.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Ben Mahler, and Dominic 
> Hamon.
> 
> 
> Bugs: MESOS-1131
> https://issues.apache.org/jira/browse/MESOS-1131
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Depends on : https://reviews.apache.org/r/21324/
> 3rd phase following Ben's comments on https://reviews.apache.org/r/19575/
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/base64.hpp 1f0944a 
>   3rdparty/libprocess/include/process/process.hpp c29cd21 
>   3rdparty/libprocess/src/process.cpp 58bae5b 
>   3rdparty/libprocess/src/tests/http_tests.cpp f58a129 
> 
> Diff: https://reviews.apache.org/r/21713/diff/
> 
> 
> Testing
> ---
> 
> Make check
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 32700: Removed FrameworkID from FrameworkState.

2015-09-12 Thread Adam B 


> On Aug. 3, 2015, 6:41 p.m., Adam B wrote:
> > Looks great! Just a couple of questions.
> > Did you run `sudo make check`, since you modified `ROOT_` tests?

@karya, should we revive this patch now, or discard it?


- Adam


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


On Aug. 1, 2015, 12:13 p.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32700/
> ---
> 
> (Updated Aug. 1, 2015, 12:13 p.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Bugs: MESOS-905
> https://issues.apache.org/jira/browse/MESOS-905
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> FrameworkState already has FrameworkInfo which will have a valid FrameworkID.
> 
> NOTE: This patch is only to be merged _ONLY_ after all the dependent patches 
> have shipped, i.e. after 0.23.0 (tracked here: 
> https://issues.apache.org/jira/browse/MESOS-2561) has released.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.cpp 8f5d302477b216df9ac2f59156304bbc4a96f24b 
>   src/slave/containerizer/external_containerizer.cpp 
> d3640e7beaa7a99d7c8938025af879e19cd6b470 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 6d07ff151770bac4eeeb7cd8c9d03f54f2e78ec1 
>   src/slave/slave.cpp 6b21db973dc95dd5eb2238eebe697db9dd063ef1 
>   src/slave/state.hpp cecf200e6e79172af57ae195a73a5161be7e604a 
>   src/slave/state.cpp b9f2d8a0d6395b92bd50f1e0927f3ae9fd04b81c 
>   src/slave/status_update_manager.cpp 
> 1978ac8ab370f3e20f5c3c803beda468edf31e2c 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 80ed60e2b0fa39e8302867a7cb6a7388c25f9a40 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 213fa4b0b9c50eba941ef6b52497eb32d539 
> 
> Diff: https://reviews.apache.org/r/32700/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 38137: Added Docker provisioner and local store

2015-09-12 Thread Jiang Yan Xu 


> On Sept. 10, 2015, 3:04 a.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/provisioners/docker/local_store.cpp, lines 158-160
> > 
> >
> > Can we actually use fetcher here?
> 
> Timothy Chen wrote:
> Not sure what you're referring to? We use the fetcher to fetch local 
> images.

No. The fetcher pointer passed into the store is simply ignored in the code.


> On Sept. 10, 2015, 3:04 a.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/provisioners/docker/metadata_manager.cpp, lines 
> > 78-81
> > 
> >
> > How is the "latest" tag handled in this case? If I have downloaded an 
> > ubuntu:latest and 1 month later I want another ubuntu:latest, the 
> > store/slave is always using to use the old ubuntu:latest?
> 
> Timothy Chen wrote:
> Yep, that's how docker client works for now. We will provide the option 
> to pull again, or always pull latest which will attempt to update the 
> metadata.
> 
> Jiang Yan Xu wrote:
> I see. But this kind of sucks...
> 
> I am all for stick

Sorry for the partial comment, I didn't finish it.

I meant to say that it's unfortunate because as a user I don't know when to do 
a force pull unless I always force a pull. It's probably less of an issue if I 
am a casual user doing this on a single host. (I can inspect the local cache 
with a sequence of docker commands.) If I have O(1) hosts and serve O(100) 
users it's going to be messy. I wouldn't know if 
`ubuntu-patched-for-my-company:vivid` cached on any machine is really want I 
think it is.

So to reiterate my point, I think an ID is necessary (and Docker engine 
supports it to enable preciseness). If an ID is used, here we need to rethink 
about the bookkeeping data structure and get() semantics. FWICT Docker does two 
lookups: first look it up by using repo:tag against the tag store and if not 
found, go look for it in the graph data structure which maintains an index of 
IDs.


> On Sept. 10, 2015, 3:04 a.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/provisioners/docker.hpp, line 66
> > 
> >
> > Should registry be part of the name?
> > 
> > I know it has significance in discovery but it doesn't appear to be 
> > part of the name.
> > 
> > I would suggest we model our data structure close to the spec so when 
> > there's confusion it's easy to refer to something that's definitive.
> > 
> > I have another comment below about the "registry" in "name".
> 
> Timothy Chen wrote:
> I think registry should be part of the name since it's what uniquely 
> identifies a "tag" of a image.
> A name of a docker image, is merely a tag instead of a immutable id, and 
> that's very different than how Appc works.
> Only layers have uniquely identifable ids.

Fine about registry being part of the repo name then. 

Is the ID of the image the ID of its leaf layer then? Image ID is definitely 
used throughout Docker's documentation, code, the "repositories" files and is 
what uniquely identify an image.

Docker code (here and in various other 
places)[https://github.com/docker/docker/blob/9d0954a83d6c7b52287a62f8ab7ad42d544322a0/pkg/parsers/parsers.go#L104]
 kind of treats `sha256:` as a special case of tag, which I don't think is 
a good practice. It also treats registry as **part of** the repo name.

So if we are separating the registry out as a field, we should separate out the 
ID as a separate field.


- Jiang Yan


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


On Sept. 11, 2015, 12:42 a.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38137/
> ---
> 
> (Updated Sept. 11, 2015, 12:42 a.m.)
> 
> 
> Review request for mesos, Jie Yu, Jojy Varghese, Till Toenshoff, and Jiang 
> Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Joining all the commits around provisioner and local store into one review so 
> it's easier to review, as patches
> are changing code on top of each other.
> 
> All the commits are going to committed together.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 8963cea 
>   src/slave/containerizer/provisioner.hpp 9e0e0b8 
>   src/slave/containerizer/provisioner.cpp 2ac9008 
>   src/slave/containerizer/provisioners/docker/local_store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/local_store.cpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/message.hpp PRE-CREATION 
>