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

2017-02-18 Thread haosdent huang 

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




src/docker/docker.cpp (line 799)


Since we use `int_fd` now, I update to
```
  Try fd = os::open(
```
here.


- haosdent huang


On Feb. 6, 2017, 9:28 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54821/
> ---
> 
> (Updated Feb. 6, 2017, 9:28 p.m.)
> 
> 
> Review request for mesos, Xiaojian Huang, haosdent huang, and Jie Yu.
> 
> 
> 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
> -
> 
>   src/docker/docker.hpp 9093371afc8ea792ba94f61c6875703e547ea6b0 
>   src/docker/docker.cpp b63b060ba1c1d39dc1702368cf667831edbd39bd 
>   src/docker/executor.cpp d8c72b081fb9a37b3927b73cea3b47956a076691 
>   src/slave/containerizer/docker.cpp 886528f0f706ef2a3c07246406f3ee1ebc45565d 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 31d63b1f239055d82470ace9024b584a2096dce4 
>   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 54821: Refactored Docker::run() to make it only aware of docker cli options.

2017-02-06 Thread Zhitao Li 

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

(Updated Feb. 6, 2017, 9:28 p.m.)


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


Changes
---

Style 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 b63b060ba1c1d39dc1702368cf667831edbd39bd 
  src/docker/executor.cpp d8c72b081fb9a37b3927b73cea3b47956a076691 
  src/slave/containerizer/docker.cpp 886528f0f706ef2a3c07246406f3ee1ebc45565d 
  src/tests/containerizer/docker_containerizer_tests.cpp 
31d63b1f239055d82470ace9024b584a2096dce4 
  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 54821: Refactored Docker::run() to make it only aware of docker cli options.

2017-02-05 Thread haosdent huang 


> On Feb. 6, 2017, 1:36 a.m., Jie Yu wrote:
> > src/docker/docker.hpp, lines 141-142
> > 
> >
> > I typically wrap comments in 70 char width. Let's do that in this file 
> > so that it's consistent with other comments in this file.
> > 
> > This will reduce the jagdness.

```
  // See https://docs.docker.com/engine/reference/run for a complete
  // explanation of each option.
```


- haosdent


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


On Feb. 5, 2017, 11:09 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54821/
> ---
> 
> (Updated Feb. 5, 2017, 11:09 p.m.)
> 
> 
> Review request for mesos, Xiaojian Huang, haosdent huang, and Jie Yu.
> 
> 
> 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
> -
> 
>   src/docker/docker.hpp 9093371afc8ea792ba94f61c6875703e547ea6b0 
>   src/docker/docker.cpp b63b060ba1c1d39dc1702368cf667831edbd39bd 
>   src/docker/executor.cpp d8c72b081fb9a37b3927b73cea3b47956a076691 
>   src/slave/containerizer/docker.cpp 886528f0f706ef2a3c07246406f3ee1ebc45565d 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 31d63b1f239055d82470ace9024b584a2096dce4 
>   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 54821: Refactored Docker::run() to make it only aware of docker cli options.

2017-02-05 Thread haosdent huang 

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


Fix it, then Ship it!




Thanks a lot for @zhitao's great refactor and @jieyu's review. Let me commit it 
after 1.2.0 release.
I would try to see if it could backport to other active branches at that time 
as well.


src/docker/docker.hpp (lines 165 - 166)


```
// Environment variable overrides. These overrides will be passed
// to docker container through "--env-file" option.
```



src/docker/docker.cpp (line 713)


Got error when apply the patch:

```
Checking 8 C++ files
src/docker/docker.cpp:713:  Lines should be <= 80 characters long  
[whitespace/line_length] [2]
Total errors found: 1
```



src/docker/executor.cpp (line 477)


nits:

```
  void _stop()
  {
```


- haosdent huang


On Feb. 5, 2017, 11:09 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54821/
> ---
> 
> (Updated Feb. 5, 2017, 11:09 p.m.)
> 
> 
> Review request for mesos, Xiaojian Huang, haosdent huang, and Jie Yu.
> 
> 
> 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
> -
> 
>   src/docker/docker.hpp 9093371afc8ea792ba94f61c6875703e547ea6b0 
>   src/docker/docker.cpp b63b060ba1c1d39dc1702368cf667831edbd39bd 
>   src/docker/executor.cpp d8c72b081fb9a37b3927b73cea3b47956a076691 
>   src/slave/containerizer/docker.cpp 886528f0f706ef2a3c07246406f3ee1ebc45565d 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 31d63b1f239055d82470ace9024b584a2096dce4 
>   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 54821: Refactored Docker::run() to make it only aware of docker cli options.

2017-02-05 Thread Jie Yu 

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


Fix it, then Ship it!




This is a great refactor! Thanks Zhitao!


src/docker/docker.hpp (lines 141 - 142)


I typically wrap comments in 70 char width. Let's do that in this file so 
that it's consistent with other comments in this file.

This will reduce the jagdness.



src/docker/docker.cpp (line 781)


`memory->bytes()`


- Jie Yu


On Feb. 5, 2017, 11:09 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54821/
> ---
> 
> (Updated Feb. 5, 2017, 11:09 p.m.)
> 
> 
> Review request for mesos, Xiaojian Huang, haosdent huang, and Jie Yu.
> 
> 
> 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
> -
> 
>   src/docker/docker.hpp 9093371afc8ea792ba94f61c6875703e547ea6b0 
>   src/docker/docker.cpp b63b060ba1c1d39dc1702368cf667831edbd39bd 
>   src/docker/executor.cpp d8c72b081fb9a37b3927b73cea3b47956a076691 
>   src/slave/containerizer/docker.cpp 886528f0f706ef2a3c07246406f3ee1ebc45565d 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 31d63b1f239055d82470ace9024b584a2096dce4 
>   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 54821: Refactored Docker::run() to make it only aware of docker cli options.

2017-02-05 Thread Zhitao Li 

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

(Updated Feb. 5, 2017, 11:09 p.m.)


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


Changes
---

Rebase.


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 b63b060ba1c1d39dc1702368cf667831edbd39bd 
  src/docker/executor.cpp d8c72b081fb9a37b3927b73cea3b47956a076691 
  src/slave/containerizer/docker.cpp 886528f0f706ef2a3c07246406f3ee1ebc45565d 
  src/tests/containerizer/docker_containerizer_tests.cpp 
31d63b1f239055d82470ace9024b584a2096dce4 
  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 54821: Refactored Docker::run() to make it only aware of docker cli options.

2017-02-05 Thread Zhitao Li 


> On Feb. 3, 2017, 7:28 p.m., Jie Yu wrote:
> > src/docker/docker.hpp, line 194
> > 
> >
> > I'd try to avoid mesos:: prefixed type in this file. Can this just be a 
> > map?
> 
> Zhitao Li wrote:
> This could be a `vector`, but a `map` would destroy initial 
> order, which used to be sensitive to docker daemon.
> 
> Jie Yu wrote:
> Can you give an example which flags are order sensitive. I am just 
> curious.
> 
> If that's the case, use LinkedHashMap in stout?

`--volumeDriver` and `--volume` are two options whose order has to be 
maintained.

Another reason not to use map is that certain options can be repeating (i.e. 
`--label` or `--env`. Using map would be a bit awkward.


- Zhitao


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


On Feb. 4, 2017, 12:20 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54821/
> ---
> 
> (Updated Feb. 4, 2017, 12:20 a.m.)
> 
> 
> Review request for mesos, Xiaojian Huang, haosdent huang, and Jie Yu.
> 
> 
> 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
> -
> 
>   src/docker/docker.hpp 9093371afc8ea792ba94f61c6875703e547ea6b0 
>   src/docker/docker.cpp b63b060ba1c1d39dc1702368cf667831edbd39bd 
>   src/docker/executor.cpp d8c72b081fb9a37b3927b73cea3b47956a076691 
>   src/slave/containerizer/docker.cpp 886528f0f706ef2a3c07246406f3ee1ebc45565d 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 31d63b1f239055d82470ace9024b584a2096dce4 
>   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 54821: Refactored Docker::run() to make it only aware of docker cli options.

2017-02-03 Thread Zhitao Li 

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

(Updated Feb. 4, 2017, 12:20 a.m.)


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


Changes
---

Change `parameters` to a `vector additionalOptions`;
Fix for missing MIN_MEMORY.


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 b63b060ba1c1d39dc1702368cf667831edbd39bd 
  src/docker/executor.cpp d8c72b081fb9a37b3927b73cea3b47956a076691 
  src/slave/containerizer/docker.cpp 886528f0f706ef2a3c07246406f3ee1ebc45565d 
  src/tests/containerizer/docker_containerizer_tests.cpp 
31d63b1f239055d82470ace9024b584a2096dce4 
  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 54821: Refactored Docker::run() to make it only aware of docker cli options.

2017-02-03 Thread Jie Yu 


> On Feb. 3, 2017, 7:28 p.m., Jie Yu wrote:
> > src/docker/docker.hpp, line 194
> > 
> >
> > I'd try to avoid mesos:: prefixed type in this file. Can this just be a 
> > map?
> 
> Zhitao Li wrote:
> This could be a `vector`, but a `map` would destroy initial 
> order, which used to be sensitive to docker daemon.

Can you give an example which flags are order sensitive. I am just curious.

If that's the case, use LinkedHashMap in stout?


- Jie


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


On Feb. 3, 2017, 6:55 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54821/
> ---
> 
> (Updated Feb. 3, 2017, 6:55 p.m.)
> 
> 
> Review request for mesos, Xiaojian Huang, haosdent huang, and Jie Yu.
> 
> 
> 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
> -
> 
>   src/docker/docker.hpp 9093371afc8ea792ba94f61c6875703e547ea6b0 
>   src/docker/docker.cpp b63b060ba1c1d39dc1702368cf667831edbd39bd 
>   src/docker/executor.cpp d8c72b081fb9a37b3927b73cea3b47956a076691 
>   src/slave/containerizer/docker.cpp 886528f0f706ef2a3c07246406f3ee1ebc45565d 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 31d63b1f239055d82470ace9024b584a2096dce4 
>   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 54821: Refactored Docker::run() to make it only aware of docker cli options.

2017-02-03 Thread Zhitao Li 


> On Feb. 3, 2017, 7:28 p.m., Jie Yu wrote:
> > This is a partial review. Is there way to run this in your staging/prod 
> > environment. This patch touches the core part of Docker containerizer which 
> > we don't have a good test coverage, and has a very board impact.

I will run all tests with a `DOCKER` filter, and also use `mesos-execute` with 
docker containerizer to test this out.

If we want more testing, I can try to build an actual binary to test it out in 
our staging environment but it'll take sometime to get it done, and even that 
doesn't test every combinations of arguments.


> On Feb. 3, 2017, 7:28 p.m., Jie Yu wrote:
> > src/docker/docker.hpp, line 194
> > 
> >
> > I'd try to avoid mesos:: prefixed type in this file. Can this just be a 
> > map?

This could be a `vector`, but a `map` would destroy initial order, 
which used to be sensitive to docker daemon.


> On Feb. 3, 2017, 7:28 p.m., Jie Yu wrote:
> > src/docker/docker.cpp, line 527
> > 
> >
> > This changes the logic? Where is MIN_MEMORY?

Adding the previous logic back right now.


- Zhitao


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


On Feb. 3, 2017, 6:55 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54821/
> ---
> 
> (Updated Feb. 3, 2017, 6:55 p.m.)
> 
> 
> Review request for mesos, Xiaojian Huang, haosdent huang, and Jie Yu.
> 
> 
> 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
> -
> 
>   src/docker/docker.hpp 9093371afc8ea792ba94f61c6875703e547ea6b0 
>   src/docker/docker.cpp b63b060ba1c1d39dc1702368cf667831edbd39bd 
>   src/docker/executor.cpp d8c72b081fb9a37b3927b73cea3b47956a076691 
>   src/slave/containerizer/docker.cpp 886528f0f706ef2a3c07246406f3ee1ebc45565d 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 31d63b1f239055d82470ace9024b584a2096dce4 
>   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 54821: Refactored Docker::run() to make it only aware of docker cli options.

2017-02-03 Thread Jie Yu 

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



This is a partial review. Is there way to run this in your staging/prod 
environment. This patch touches the core part of Docker containerizer which we 
don't have a good test coverage, and has a very board impact.


src/docker/docker.hpp (line 194)


I'd try to avoid mesos:: prefixed type in this file. Can this just be a map?



src/docker/docker.cpp (line 516)


This changes the logic? Where is MIN_MEMORY?


- Jie Yu


On Feb. 3, 2017, 6:55 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54821/
> ---
> 
> (Updated Feb. 3, 2017, 6:55 p.m.)
> 
> 
> Review request for mesos, Xiaojian Huang, haosdent huang, and Jie Yu.
> 
> 
> 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
> -
> 
>   src/docker/docker.hpp 9093371afc8ea792ba94f61c6875703e547ea6b0 
>   src/docker/docker.cpp b63b060ba1c1d39dc1702368cf667831edbd39bd 
>   src/docker/executor.cpp d8c72b081fb9a37b3927b73cea3b47956a076691 
>   src/slave/containerizer/docker.cpp 886528f0f706ef2a3c07246406f3ee1ebc45565d 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 31d63b1f239055d82470ace9024b584a2096dce4 
>   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 54821: Refactored Docker::run() to make it only aware of docker cli options.

2017-02-03 Thread Zhitao Li 

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

(Updated Feb. 3, 2017, 6:55 p.m.)


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


Changes
---

Rebase with master.


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 b63b060ba1c1d39dc1702368cf667831edbd39bd 
  src/docker/executor.cpp d8c72b081fb9a37b3927b73cea3b47956a076691 
  src/slave/containerizer/docker.cpp 886528f0f706ef2a3c07246406f3ee1ebc45565d 
  src/tests/containerizer/docker_containerizer_tests.cpp 
31d63b1f239055d82470ace9024b584a2096dce4 
  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 54821: Refactored Docker::run() to make it only aware of docker cli options.

2017-01-05 Thread Zhitao Li 


> On Jan. 4, 2017, 7:58 p.m., Jie Yu wrote:
> > src/docker/docker.hpp, line 149
> > 
> >
> > What is this? Should that be part of `env`?

Talked to @gilbert offline, we believe this is not necessary anymore, so 
removing this environment variable override now.


- Zhitao


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


On Jan. 5, 2017, 7:37 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54821/
> ---
> 
> (Updated Jan. 5, 2017, 7:37 p.m.)
> 
> 
> Review request for mesos, Xiaojian Huang, haosdent huang, and Jie Yu.
> 
> 
> 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
> -
> 
>   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 54821: Refactored Docker::run() to make it only aware of docker cli options.

2017-01-05 Thread Zhitao Li 

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

(Updated Jan. 5, 2017, 7:37 p.m.)


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


Changes
---

Review comments:
- reorder fields for `RunOptions` to reflect order for both `docker run` and 
how they are passed;
- make sure executor driver is stopped;
- add comments.


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 54821: Refactored Docker::run() to make it only aware of docker cli options.

2017-01-04 Thread Zhitao Li 


> On Jan. 4, 2017, 7:58 p.m., Jie Yu wrote:
> > src/docker/docker.hpp, lines 137-145
> > 
> >
> > Feel that this belongs to higher level components (i.e., docker 
> > containerizer, docker executor).
> > 
> > Can we follow with a patch to move the `create` logic to a common 
> > helper ouside this library code?

`+1` to moving it elsewhere. I feel we should create a new subdirectory 
`src/slave/containerizer/docker/` and moving all of this function, docker 
containerizer and docker executor into it.

If that sounds a right plan, I'll file a clean up issue.


> On Jan. 4, 2017, 7:58 p.m., Jie Yu wrote:
> > src/docker/docker.hpp, lines 147-169
> > 
> >
> > Let's add a comment for each field with the corresponding docker run 
> > option.
> > 
> > `name` should be optional?

`name` indeed can be optional. Will do.


> On Jan. 4, 2017, 7:58 p.m., Jie Yu wrote:
> > src/docker/docker.hpp, line 162
> > 
> >
> > We should probably introduce a Docker::PortMapping, instead of using a 
> > plain string here.

I was initially modelling all fields against `docker run ...` commands, and it 
seems that you are suggesting to model this around internal data structs of the 
`Docker` class.

I actually like this suggestion because it captures the type of each field.

I'll make this change to `devices` and `portMapping`.


> On Jan. 4, 2017, 7:58 p.m., Jie Yu wrote:
> > src/docker/docker.hpp, line 164
> > 
> >
> > Should we use `Docker::Device` here?

Good suggestion. Will do.


> On Jan. 4, 2017, 7:58 p.m., Jie Yu wrote:
> > src/docker/executor.cpp, lines 178-187
> > 
> >
> > who will trigger the shutdown of this executor in this case?

Good catch, nothing ensures the executor shutdown right now.

This function should call `_reaped()`, probably with an empty `run` future to 
ensure same status update is generated as failed `docker run ...` invocation.

Will do.


- Zhitao


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


On Jan. 3, 2017, 5:42 p.m., Zhitao Li wrote:
> 
> ---
> 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.
> 
> 
> 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
> -
> 
>   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 54821: Refactored Docker::run() to make it only aware of docker cli options.

2017-01-04 Thread Jie Yu 

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




src/docker/docker.hpp (lines 137 - 145)


Feel that this belongs to higher level components (i.e., docker 
containerizer, docker executor).

Can we follow with a patch to move the `create` logic to a common helper 
ouside this library code?



src/docker/docker.hpp (lines 147 - 169)


Let's add a comment for each field with the corresponding docker run option.

`name` should be optional?



src/docker/docker.hpp (line 149)


What is this? Should that be part of `env`?



src/docker/docker.hpp (line 160)


`network` should be optional?



src/docker/docker.hpp (line 162)


We should probably introduce a Docker::PortMapping, instead of using a 
plain string here.



src/docker/docker.hpp (line 164)


Should we use `Docker::Device` here?



src/docker/docker.hpp (line 184)


indentation should be 4 for function parameters.



src/docker/docker.cpp (line 780)


s/runOptions/options/

`run` is already part of the method name.



src/docker/executor.cpp (lines 172 - 181)


who will trigger the shutdown of this executor in this case?



src/tests/containerizer/docker_containerizer_tests.cpp (lines 1207 - 1208)


This fit in one line?


- Jie Yu


On Jan. 3, 2017, 5:42 p.m., Zhitao Li wrote:
> 
> ---
> 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.
> 
> 
> 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
> -
> 
>   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 54821: Refactored Docker::run() to make it only aware of docker cli options.

2017-01-03 Thread 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 54821: Refactored Docker::run() to make it only aware of docker cli options.

2017-01-02 Thread Mesos ReviewBot 

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



Patch looks great!

Reviews applied: [54821]

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 Dec. 31, 2016, 1:27 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54821/
> ---
> 
> (Updated Dec. 31, 2016, 1:27 a.m.)
> 
> 
> Review request for mesos, Xiaojian Huang, haosdent huang, and Jie Yu.
> 
> 
> 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
> -
> 
>   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 54821: Refactored Docker::run() to make it only aware of docker cli options.

2017-01-01 Thread haosdent huang 

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


Fix it, then Ship it!




LGTM! Verfied in Ubuntu 14.04 and CentOS 7.


src/docker/docker.hpp (lines 138 - 145)


Nit: indent with 4 spaces.



src/tests/containerizer/docker_containerizer_tests.cpp (line 1327)


Nit: indent with 4 spaces.


- haosdent huang


On Dec. 31, 2016, 1:27 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54821/
> ---
> 
> (Updated Dec. 31, 2016, 1:27 a.m.)
> 
> 
> Review request for mesos, Xiaojian Huang, haosdent huang, and Jie Yu.
> 
> 
> 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
> -
> 
>   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 54821: Refactored Docker::run() to make it only aware of docker cli options.

2016-12-30 Thread Zhitao Li 

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

(Updated Dec. 31, 2016, 1:27 a.m.)


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


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 54821: Refactored Docker::run() to make it only aware of docker cli options.

2016-12-26 Thread Mesos ReviewBot 

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



Patch looks great!

Reviews applied: [54821]

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 Dec. 22, 2016, 5:17 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54821/
> ---
> 
> (Updated Dec. 22, 2016, 5:17 p.m.)
> 
> 
> Review request for mesos, Xiaojian Huang, haosdent huang, and Jie Yu.
> 
> 
> 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
> -
> 
>   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 54821: Refactored Docker::run() to make it only aware of docker cli options.

2016-12-22 Thread haosdent huang 


> On Dec. 22, 2016, 4:21 a.m., haosdent huang wrote:
> > Basically it looks good to me.
> > 
> > Have some simple issues we would like to discuss more:
> > 
> > * Should we keep so much fields in `RunOptions`?
> > * Should we use `vector cmd` in `RunOptions`? Or use `CommandInfo` 
> > directly.
> 
> Zhitao Li wrote:
> @haosdent, my current feeling is to abstract Docker CLI at each option 
> level, that is why I decided to use many fields rather than simply abstract 
> against a `vector`.
> 
> Another reason is that this abstraction allows us to either add a builder 
> pattern for `RunOptions`, or allow modification of a `RunOptions` easily. 
> This can eventually allow us to gradually break the lengthy 
> `RunOptions::create` function into smaller and more testable pieces.
> 
> Using a `vector cmd` as abstraction interface could make the 
> above goal harder, since the caller needs to correctly parse all parameters 
> and modify them in place.
> 
> Please let me know what you think.
> 
> haosdent huang wrote:
> Suppose we define `RunOptions` like 
> 
> ```
> map env;
> vector args;
> ```
> 
> So when we would like to add a new flag to docker, we only need to change 
> `RunOptions::create` and don't need to change `Docker::run`. Do you think 
> this would be better?
> 
> For `cmd`, I agreed your opinions, how about put it in `vector 
> args` as well?
> 
> haosdent huang wrote:
> Hmm, my basic idea is `Docker::run` no need to aware so much specific 
> fields from `RunOptions`.
> I think `Docker::run` only need to know `env`, `parameters`, `command`, 
> or just `env` and `args`.

OK, chatted with @zhitao offline. I agree we should break it in different 
fields in `RunOpitions`. 

Verfied this patch by 

```
sudo ./bin/mesos-tests.sh --gtest_filter="*DOCKER*"
```

in CentOS 7 and Ubuntu 14.04


- haosdent


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


On Dec. 22, 2016, 5:17 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54821/
> ---
> 
> (Updated Dec. 22, 2016, 5:17 p.m.)
> 
> 
> Review request for mesos, Xiaojian Huang, haosdent huang, and Jie Yu.
> 
> 
> 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
> -
> 
>   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 54821: Refactored Docker::run() to make it only aware of docker cli options.

2016-12-22 Thread haosdent huang 

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




src/docker/docker.hpp (line 171)


Rename to `arguments`.



src/docker/docker.cpp (line 507)


Let's ensure we organzie the code in `RunOptions::create` to match the 
order of the fields defintions in `RunOptions`.

For exmaple,

```
options.name = xxx;
options.image = xxx;

options.home = xxx;

options.privileged = dockerInfo.privileged();
```



src/docker/docker.cpp (line 750)


I think it is OK now to move this after

```
  options.image = dockerInfo.image();
```



src/docker/docker.cpp (line 896)


Let's use another name for `cmd` here to avoid shadow variables.


- haosdent huang


On Dec. 22, 2016, 5:17 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54821/
> ---
> 
> (Updated Dec. 22, 2016, 5:17 p.m.)
> 
> 
> Review request for mesos, Xiaojian Huang, haosdent huang, and Jie Yu.
> 
> 
> 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
> -
> 
>   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 54821: Refactored Docker::run() to make it only aware of docker cli options.

2016-12-22 Thread haosdent huang 


> On Dec. 22, 2016, 4:21 a.m., haosdent huang wrote:
> > Basically it looks good to me.
> > 
> > Have some simple issues we would like to discuss more:
> > 
> > * Should we keep so much fields in `RunOptions`?
> > * Should we use `vector cmd` in `RunOptions`? Or use `CommandInfo` 
> > directly.
> 
> Zhitao Li wrote:
> @haosdent, my current feeling is to abstract Docker CLI at each option 
> level, that is why I decided to use many fields rather than simply abstract 
> against a `vector`.
> 
> Another reason is that this abstraction allows us to either add a builder 
> pattern for `RunOptions`, or allow modification of a `RunOptions` easily. 
> This can eventually allow us to gradually break the lengthy 
> `RunOptions::create` function into smaller and more testable pieces.
> 
> Using a `vector cmd` as abstraction interface could make the 
> above goal harder, since the caller needs to correctly parse all parameters 
> and modify them in place.
> 
> Please let me know what you think.
> 
> haosdent huang wrote:
> Suppose we define `RunOptions` like 
> 
> ```
> map env;
> vector args;
> ```
> 
> So when we would like to add a new flag to docker, we only need to change 
> `RunOptions::create` and don't need to change `Docker::run`. Do you think 
> this would be better?
> 
> For `cmd`, I agreed your opinions, how about put it in `vector 
> args` as well?

Hmm, my basic idea is `Docker::run` no need to aware so much specific fields 
from `RunOptions`.
I think `Docker::run` only need to know `env`, `parameters`, `command`, or just 
`env` and `args`.


- haosdent


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


On Dec. 22, 2016, 5:17 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54821/
> ---
> 
> (Updated Dec. 22, 2016, 5:17 p.m.)
> 
> 
> Review request for mesos, Xiaojian Huang, haosdent huang, and Jie Yu.
> 
> 
> 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
> -
> 
>   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 54821: Refactored Docker::run() to make it only aware of docker cli options.

2016-12-22 Thread haosdent huang 


> On Dec. 22, 2016, 4:21 a.m., haosdent huang wrote:
> > Basically it looks good to me.
> > 
> > Have some simple issues we would like to discuss more:
> > 
> > * Should we keep so much fields in `RunOptions`?
> > * Should we use `vector cmd` in `RunOptions`? Or use `CommandInfo` 
> > directly.
> 
> Zhitao Li wrote:
> @haosdent, my current feeling is to abstract Docker CLI at each option 
> level, that is why I decided to use many fields rather than simply abstract 
> against a `vector`.
> 
> Another reason is that this abstraction allows us to either add a builder 
> pattern for `RunOptions`, or allow modification of a `RunOptions` easily. 
> This can eventually allow us to gradually break the lengthy 
> `RunOptions::create` function into smaller and more testable pieces.
> 
> Using a `vector cmd` as abstraction interface could make the 
> above goal harder, since the caller needs to correctly parse all parameters 
> and modify them in place.
> 
> Please let me know what you think.

Suppose we define `RunOptions` like 

```
map env;
vector args;
```

So when we would like to add a new flag to docker, we only need to change 
`RunOptions::create` and don't need to change `Docker::run`. Do you think this 
would be better?

For `cmd`, I agreed your opinions, how about put it in `vector args` as 
well?


- haosdent


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


On Dec. 22, 2016, 5:17 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54821/
> ---
> 
> (Updated Dec. 22, 2016, 5:17 p.m.)
> 
> 
> Review request for mesos, Xiaojian Huang, haosdent huang, and Jie Yu.
> 
> 
> 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
> -
> 
>   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 54821: Refactored Docker::run() to make it only aware of docker cli options.

2016-12-22 Thread Zhitao Li 


> On Dec. 22, 2016, 4:21 a.m., haosdent huang wrote:
> > Basically it looks good to me.
> > 
> > Have some simple issues we would like to discuss more:
> > 
> > * Should we keep so much fields in `RunOptions`?
> > * Should we use `vector cmd` in `RunOptions`? Or use `CommandInfo` 
> > directly.

@haosdent, my current feeling is to abstract Docker CLI at each option level, 
that is why I decided to use many fields rather than simply abstract against a 
`vector`.

Another reason is that this abstraction allows us to either add a builder 
pattern for `RunOptions`, or allow modification of a `RunOptions` easily. This 
can eventually allow us to gradually break the lengthy `RunOptions::create` 
function into smaller and more testable pieces.

Using a `vector cmd` as abstraction interface could make the above goal 
harder, since the caller needs to correctly parse all parameters and modify 
them in place.

Please let me know what you think.


- Zhitao


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


On Dec. 22, 2016, 5:17 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54821/
> ---
> 
> (Updated Dec. 22, 2016, 5:17 p.m.)
> 
> 
> Review request for mesos, Xiaojian Huang, haosdent huang, and Jie Yu.
> 
> 
> 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
> -
> 
>   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 54821: Refactored Docker::run() to make it only aware of docker cli options.

2016-12-22 Thread Zhitao Li 

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

(Updated Dec. 22, 2016, 5:17 p.m.)


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


Changes
---

Rebase with master for the new windows support change.


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 54821: Refactored Docker::run() to make it only aware of docker cli options.

2016-12-21 Thread haosdent huang 

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



Basically it looks good to me.

Have some simple issues we would like to discuss more:

* Should we keep so much fields in `RunOptions`?
* Should we use `vector cmd` in `RunOptions`? Or use `CommandInfo` 
directly.

- haosdent huang


On Dec. 21, 2016, 6:52 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54821/
> ---
> 
> (Updated Dec. 21, 2016, 6:52 p.m.)
> 
> 
> Review request for mesos, Xiaojian Huang, haosdent huang, and Jie Yu.
> 
> 
> 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
> -
> 
>   src/docker/docker.hpp 9093371afc8ea792ba94f61c6875703e547ea6b0 
>   src/docker/docker.cpp b2a547d9b1c5ac3922ed21bceb5c9b60fa9f6568 
>   src/docker/executor.cpp 94e116f06c0c729a90e5424c37edc39c7ba4cbe6 
>   src/slave/containerizer/docker.cpp 7ef5928e3e971f7fcd59487a0242b3af689b0058 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 6035171f7d66e728e4675634af3587f9c378e1e1 
>   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 54821: Refactored Docker::run() to make it only aware of docker cli options.

2016-12-21 Thread Zhitao Li 

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

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


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


Changes
---

review comments.


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


Repository: mesos


Description (updated)
---

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 b2a547d9b1c5ac3922ed21bceb5c9b60fa9f6568 
  src/docker/executor.cpp 94e116f06c0c729a90e5424c37edc39c7ba4cbe6 
  src/slave/containerizer/docker.cpp 7ef5928e3e971f7fcd59487a0242b3af689b0058 
  src/tests/containerizer/docker_containerizer_tests.cpp 
6035171f7d66e728e4675634af3587f9c378e1e1 
  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 54821: Refactored Docker::run() to make it only aware of docker cli options.

2016-12-19 Thread Mesos ReviewBot 

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



Patch looks great!

Reviews applied: [54821]

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 Dec. 18, 2016, 5:38 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54821/
> ---
> 
> (Updated Dec. 18, 2016, 5:38 p.m.)
> 
> 
> Review request for mesos, Xiaojian Huang, haosdent huang, and Jie Yu.
> 
> 
> 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 behavior change expected this refactoring patch.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.hpp 9093371afc8ea792ba94f61c6875703e547ea6b0 
>   src/docker/docker.cpp b2a547d9b1c5ac3922ed21bceb5c9b60fa9f6568 
>   src/docker/executor.cpp 94e116f06c0c729a90e5424c37edc39c7ba4cbe6 
>   src/slave/containerizer/docker.cpp 7ef5928e3e971f7fcd59487a0242b3af689b0058 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 6035171f7d66e728e4675634af3587f9c378e1e1 
>   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 54821: Refactored Docker::run() to make it only aware of docker cli options.

2016-12-18 Thread haosdent huang 

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




src/docker/executor.cpp (line 168)


I think should not add `CHECK` here. The old behaviour is send 
`TASK_FAILED` and stop the executor.


- haosdent huang


On Dec. 18, 2016, 5:38 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54821/
> ---
> 
> (Updated Dec. 18, 2016, 5:38 p.m.)
> 
> 
> Review request for mesos, Xiaojian Huang, haosdent huang, and Jie Yu.
> 
> 
> 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 behavior change expected this refactoring patch.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.hpp 9093371afc8ea792ba94f61c6875703e547ea6b0 
>   src/docker/docker.cpp b2a547d9b1c5ac3922ed21bceb5c9b60fa9f6568 
>   src/docker/executor.cpp 94e116f06c0c729a90e5424c37edc39c7ba4cbe6 
>   src/slave/containerizer/docker.cpp 7ef5928e3e971f7fcd59487a0242b3af689b0058 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 6035171f7d66e728e4675634af3587f9c378e1e1 
>   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 54821: Refactored Docker::run() to make it only aware of docker cli options.

2016-12-18 Thread haosdent huang 

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




src/docker/docker.hpp (lines 134 - 135)


Nit:

```
class RunOptions
{
public:
```



src/docker/docker.hpp (lines 165 - 167)


Put `parameters` after `devices` seems would be better because `parameters` 
is general and other fields above it are specific.


- haosdent huang


On Dec. 18, 2016, 5:38 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54821/
> ---
> 
> (Updated Dec. 18, 2016, 5:38 p.m.)
> 
> 
> Review request for mesos, Xiaojian Huang, haosdent huang, and Jie Yu.
> 
> 
> 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 behavior change expected this refactoring patch.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.hpp 9093371afc8ea792ba94f61c6875703e547ea6b0 
>   src/docker/docker.cpp b2a547d9b1c5ac3922ed21bceb5c9b60fa9f6568 
>   src/docker/executor.cpp 94e116f06c0c729a90e5424c37edc39c7ba4cbe6 
>   src/slave/containerizer/docker.cpp 7ef5928e3e971f7fcd59487a0242b3af689b0058 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 6035171f7d66e728e4675634af3587f9c378e1e1 
>   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
> 
>

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

2016-12-18 Thread Zhitao Li 

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

Review request for mesos, haosdent huang, Jason Lai, and Jie Yu.


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 behavior change expected this refactoring patch.


Diffs
-

  src/docker/docker.hpp 9093371afc8ea792ba94f61c6875703e547ea6b0 
  src/docker/docker.cpp b2a547d9b1c5ac3922ed21bceb5c9b60fa9f6568 
  src/docker/executor.cpp 94e116f06c0c729a90e5424c37edc39c7ba4cbe6 
  src/slave/containerizer/docker.cpp 7ef5928e3e971f7fcd59487a0242b3af689b0058 
  src/tests/containerizer/docker_containerizer_tests.cpp 
6035171f7d66e728e4675634af3587f9c378e1e1 
  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