Re: Review Request 46798: Introduced linux capabilities support for mesos containerizer.

2016-05-26 Thread Jojy Varghese

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

(Updated May 26, 2016, 3:03 p.m.)


Review request for mesos and Jie Yu.


Changes
---

rebased.


Repository: mesos


Description
---

This change introduces linux capability based security for unified
containerizer. A new agent flag \`allowed_capabilities\` has been
introduced to override the default capabilities of the user or the
capabilities requested by the user.

This feature is only available on linux.


Diffs (updated)
-

  src/common/parse.hpp 19e56dbeb765f8bec92e0a3615f6f7c12466fa9e 
  src/launcher/executor.cpp 7d111e668e0a139a98bdeb959997843180b40452 
  src/slave/containerizer/mesos/containerizer.hpp 
a1a00020668f6da8d611f26e5637afffc87d09ba 
  src/slave/containerizer/mesos/containerizer.cpp 
75e5a32a3e70ec60a6800e21a621673184ea0956 
  src/slave/containerizer/mesos/launch.hpp 
c716e0396736d1f2f60ec31540f12f4f7597d081 
  src/slave/containerizer/mesos/launch.cpp 
e22106b014c871e2184a15c2ab154a0674874e47 
  src/slave/flags.hpp 80ba2887448e91c40ae68fc2d9f0c0bee1a49f48 
  src/slave/flags.cpp b7df8f760d0f75459f1e80e3d8e18d49a3995df8 
  src/tests/container_logger_tests.cpp efadceafca5721bce4dbffadb35f54fd5365abb0 
  src/tests/containerizer/docker_volume_isolator_tests.cpp 
c524f42743bf08ee54f1cbb083d0d3c85a8b70c9 
  src/tests/containerizer/filesystem_isolator_tests.cpp 
4293416ac8434e9eb7e80724480a54936a2fe24a 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
09742ff21513dc2570684d384b257868dd57a9ce 

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


Testing
---

make check; used mesos cli to test end to end functionality.


Thanks,

Jojy Varghese



Re: Review Request 46371: Added basic tests for capabilities API.

2016-05-26 Thread Jojy Varghese

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

(Updated May 26, 2016, 3:01 p.m.)


Review request for mesos and Jie Yu.


Repository: mesos


Description
---

Added basic tests for capabilities API.


Diffs (updated)
-

  src/Makefile.am ce5245883f3d2661812272702c0d2060513b6d88 
  src/tests/capabilities_test_helper.hpp PRE-CREATION 
  src/tests/capabilities_test_helper.cpp PRE-CREATION 
  src/tests/capabilities_test_helper_main.cpp PRE-CREATION 
  src/tests/capabilities_tests.cpp PRE-CREATION 

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


Testing
---

make check.


Thanks,

Jojy Varghese



Re: Review Request 46370: Introduced linux capabilities API.

2016-05-26 Thread Jojy Varghese

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

(Updated May 26, 2016, 3:01 p.m.)


Review request for mesos, Jie Yu and Kevin Klues.


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


Repository: mesos


Description
---

This change introduces basic API for linux capabilities. This is not a
comprehensive API but is strictly limited to the need for securing Mesos
containers using linux capabilities.


Diffs (updated)
-

  src/CMakeLists.txt e0c538d9e6542fbe279bfbf6f20172e4c611c859 
  src/Makefile.am ce5245883f3d2661812272702c0d2060513b6d88 
  src/linux/capabilities.hpp PRE-CREATION 
  src/linux/capabilities.cpp PRE-CREATION 

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


Testing
---

make check.


Thanks,

Jojy Varghese



Re: Review Request 46371: Added basic tests for capabilities API.

2016-05-25 Thread Jojy Varghese

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

(Updated May 25, 2016, 6:39 p.m.)


Review request for mesos and Jie Yu.


Repository: mesos


Description
---

Added basic tests for capabilities API.


Diffs (updated)
-

  src/Makefile.am ce5245883f3d2661812272702c0d2060513b6d88 
  src/tests/capabilities_test_helper.hpp PRE-CREATION 
  src/tests/capabilities_test_helper.cpp PRE-CREATION 
  src/tests/capabilities_test_helper_main.cpp PRE-CREATION 
  src/tests/capabilities_tests.cpp PRE-CREATION 

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


Testing
---

make check.


Thanks,

Jojy Varghese



Re: Review Request 46370: Introduced linux capabilities API.

2016-05-25 Thread Jojy Varghese

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

(Updated May 25, 2016, 6:38 p.m.)


Review request for mesos, Jie Yu and Kevin Klues.


Changes
---

review addressed.


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


Repository: mesos


Description
---

This change introduces basic API for linux capabilities. This is not a
comprehensive API but is strictly limited to the need for securing Mesos
containers using linux capabilities.


Diffs (updated)
-

  src/CMakeLists.txt e0c538d9e6542fbe279bfbf6f20172e4c611c859 
  src/Makefile.am ce5245883f3d2661812272702c0d2060513b6d88 
  src/linux/capabilities.hpp PRE-CREATION 
  src/linux/capabilities.cpp PRE-CREATION 

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


Testing
---

make check.


Thanks,

Jojy Varghese



Re: Review Request 45949: Implemented docker config get credential helper.

2016-05-23 Thread Jojy Varghese

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




src/docker/spec.cpp (line 162)
<https://reviews.apache.org/r/45949/#comment199300>

Would reference do? Say, `config&`?



src/docker/spec.cpp (line 169)
<https://reviews.apache.org/r/45949/#comment199298>

Do we care about just `contains` or does the string have an expected 
mimimum structure (say, `startswith` for example).



src/docker/spec.cpp (line 185)
<https://reviews.apache.org/r/45949/#comment199302>

s/encode/encoded ?


- Jojy Varghese


On May 23, 2016, 9:51 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45949/
> ---
> 
> (Updated May 23, 2016, 9:51 p.m.)
> 
> 
> Review request for mesos, Guangya Liu, Artem Harutyunyan, Jie Yu, and Timothy 
> Chen.
> 
> 
> Bugs: MESOS-4938
> https://issues.apache.org/jira/browse/MESOS-4938
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented docker config get credential helper.
> 
> 
> Diffs
> -
> 
>   include/mesos/docker/spec.hpp 2ebacc70d92a593c8dd006b34519c3a2a5225481 
>   src/docker/spec.cpp ac28331a17edb8c2ff81d5a2f79a794f869a3e5d 
> 
> Diff: https://reviews.apache.org/r/45949/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>



Re: Review Request 47199: Implemented parsing docker labels in v1 spec.

2016-05-23 Thread Jojy Varghese

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




src/docker/spec.cpp (lines 207 - 230)
<https://reviews.apache.org/r/47199/#comment199283>

Looks like there is scope for de-duplicating code between this block and 
the block above(`config` and `container_config`)?


- Jojy Varghese


On May 13, 2016, 8:49 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47199/
> ---
> 
> (Updated May 13, 2016, 8:49 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Artem Harutyunyan, Jie Yu, and 
> Kevin Klues.
> 
> 
> Bugs: MESOS-5272
> https://issues.apache.org/jira/browse/MESOS-5272
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented parsing docker labels in v1 spec.
> 
> 
> Diffs
> -
> 
>   src/docker/spec.cpp ac28331a17edb8c2ff81d5a2f79a794f869a3e5d 
> 
> Diff: https://reviews.apache.org/r/47199/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>



Re: Review Request 47608: Add Labels from TaskInfo into TaskStatus message.

2016-05-23 Thread Jojy Varghese

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




src/tests/slave_tests.cpp (line 3265)
<https://reviews.apache.org/r/47608/#comment199175>

If this is a WIP, I would recommend to limit the reviewers list. Its 
currently `mesos` group.

If this review is intended for `mesos` group, I would recommend taking out 
debug statements like these. 

I would also recommend to use meaningful variable names (unlike L3318). 
(L3318 in particular, does not need the extra variable. See other similar code.)


- Jojy Varghese


On May 20, 2016, 6:38 p.m., Srinivas Brahmaroutu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47608/
> ---
> 
> (Updated May 20, 2016, 6:38 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jojy Varghese.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> WIP: Add Labels from TaskInfo into TaskStatus message.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp ce0e7b1f1d17c3b82d835b0a6296ed7b1e9eeac1 
>   src/tests/hook_tests.cpp c6b4e8a50534e455cf44f427b09f74eef71177b2 
>   src/tests/slave_tests.cpp e1f5bfe074c357f46403887365b3f9ae554000b4 
> 
> Diff: https://reviews.apache.org/r/47608/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Srinivas Brahmaroutu
> 
>



Re: Review Request 47608: Add Labels from TaskInfo into TaskStatus message.

2016-05-23 Thread Jojy Varghese

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



Is there a use case or JIRA ticket for this change? Wanted to understand the 
problem we are trying to solve.

- Jojy Varghese


On May 20, 2016, 6:38 p.m., Srinivas Brahmaroutu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47608/
> ---
> 
> (Updated May 20, 2016, 6:38 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jojy Varghese.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> WIP: Add Labels from TaskInfo into TaskStatus message.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp ce0e7b1f1d17c3b82d835b0a6296ed7b1e9eeac1 
>   src/tests/hook_tests.cpp c6b4e8a50534e455cf44f427b09f74eef71177b2 
>   src/tests/slave_tests.cpp e1f5bfe074c357f46403887365b3f9ae554000b4 
> 
> Diff: https://reviews.apache.org/r/47608/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Srinivas Brahmaroutu
> 
>



Re: Review Request 46369: Added capabilities support in ContanerInfo protobuf.

2016-05-18 Thread Jojy Varghese

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

(Updated May 19, 2016, 5:06 a.m.)


Review request for mesos and Jie Yu.


Changes
---

review addressed.


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


Repository: mesos


Description
---

Added capabilities support in ContanerInfo protobuf.


Diffs (updated)
-

  include/mesos/mesos.proto 887ffe9c5f04e25539a4c6b3d52ce5299c65e8d3 
  include/mesos/v1/mesos.proto 9e59aed20965d50ee10989ff6b75db742cf2b83b 

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


Testing
---

make check.


Thanks,

Jojy Varghese



Re: Review Request 47482: Added preliminary support for parsing ELF files in stout.

2016-05-18 Thread Jojy Varghese

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




3rdparty/stout/include/stout/elf.hpp (line 56)
<https://reviews.apache.org/r/47482/#comment198369>

Wondering why the implementation is in the header file.



3rdparty/stout/include/stout/elf.hpp (line 63)
<https://reviews.apache.org/r/47482/#comment198367>

Its recommended to use a smart pointer here. They provide the RAII property 
that you need here. Also, the pattern in Mesos is to use factory methods that 
return a `Owned`.



3rdparty/stout/include/stout/elf.hpp (line 188)
<https://reviews.apache.org/r/47482/#comment198368>

The advantage of emplace_back here would be that the `string` object will 
be created only once as opposed to being first created and then copied.


- Jojy Varghese


On May 18, 2016, 3:16 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47482/
> ---
> 
> (Updated May 18, 2016, 3:16 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-5400
> https://issues.apache.org/jira/browse/MESOS-5400
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Right now we are able to parse ELF formatted shared libraries and
> extract their canonical SONAME and external library dependencies. In
> the future, we should add support for fully parsing an ELf file for
> easy access to all of its contents.
> 
> The current implementation relies on libelf. We should probably remove
> this dependency in future versions (mostly since the headers for
> libelf are not installed on a standard Linux distribution by default).
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/configure.ac ada1e22c72875fe9d557f07e4846128be0bcef13 
>   3rdparty/stout/include/stout/elf.hpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/47482/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>



Re: Review Request 47482: Added preliminary support for parsing ELF files in stout.

2016-05-17 Thread Jojy Varghese

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




3rdparty/stout/include/stout/elf.hpp (line 77)
<https://reviews.apache.org/r/47482/#comment198213>

Dont we have to close the open file in error cases?



3rdparty/stout/include/stout/elf.hpp (line 94)
<https://reviews.apache.org/r/47482/#comment198215>

Maybe break out after finding DYNAMIC section?



3rdparty/stout/include/stout/elf.hpp (line 184)
<https://reviews.apache.org/r/47482/#comment198216>

emplace_back?



3rdparty/stout/include/stout/elf.hpp (line 196)
<https://reviews.apache.org/r/47482/#comment198214>

const?


- Jojy Varghese


On May 17, 2016, 7:17 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47482/
> ---
> 
> (Updated May 17, 2016, 7:17 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-5400
> https://issues.apache.org/jira/browse/MESOS-5400
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Right now we are able to parse ELF formatted shared libraries and
> extract their canonical SONAME and external library dependencies. In
> the future, we should add support for fully parsing an ELf file for
> easy access to all of its contents.
> 
> The current implementation relies on libelf. We should probably remove
> this dependency in future versions (mostly since the headers for
> libelf are not installed on a standard Linux distribution by default).
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/configure.ac ada1e22c72875fe9d557f07e4846128be0bcef13 
>   3rdparty/stout/include/stout/elf.hpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/47482/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>



Re: Review Request 47481: Rewrote os::read() to be friendlier to reading binary data.

2016-05-17 Thread Jojy Varghese

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




3rdparty/stout/include/stout/os/read.hpp (line 113)
<https://reviews.apache.org/r/47481/#comment198170>

Does this assume a certain stack size?


- Jojy Varghese


On May 17, 2016, 8:36 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47481/
> ---
> 
> (Updated May 17, 2016, 8:36 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Joris Van Remoortere, and switched 
> to 'mcypark'.
> 
> 
> Bugs: MESOS-5398
> https://issues.apache.org/jira/browse/MESOS-5398
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The previous read() implementation was based on calling getline() to
> read in chunks of data from a file. This is fine for text-based files,
> but is a little strange for binary files.
> 
> The new implementation reads in chunks of raw bytes into a stack
> allocated buffer before copying them into their final location. I
> usually don't like stack allocated buffers because of their potential
> security implications when it comes to stack buffer overflows, but I
> took extra care here to make sure there won't be any.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/read.hpp 
> e1e97c1bcb7493a734fc77721a83c230b1a23724 
> 
> Diff: https://reviews.apache.org/r/47481/diff/
> 
> 
> Testing
> ---
> 
> make check -j
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>



Re: Review Request 46798: Introduced linux capabilities support for mesos containerizer.

2016-05-17 Thread Jojy Varghese

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

(Updated May 17, 2016, 2:59 p.m.)


Review request for mesos and Jie Yu.


Changes
---

rebased


Repository: mesos


Description
---

This change introduces linux capability based security for unified
containerizer. A new agent flag \`allowed_capabilities\` has been
introduced to override the default capabilities of the user or the
capabilities requested by the user.

This feature is only available on linux.


Diffs (updated)
-

  src/launcher/executor.cpp 7d111e668e0a139a98bdeb959997843180b40452 
  src/slave/containerizer/mesos/containerizer.hpp 
a1a00020668f6da8d611f26e5637afffc87d09ba 
  src/slave/containerizer/mesos/containerizer.cpp 
75e5a32a3e70ec60a6800e21a621673184ea0956 
  src/slave/containerizer/mesos/launch.hpp 
c716e0396736d1f2f60ec31540f12f4f7597d081 
  src/slave/containerizer/mesos/launch.cpp 
e22106b014c871e2184a15c2ab154a0674874e47 
  src/slave/flags.hpp 80ba2887448e91c40ae68fc2d9f0c0bee1a49f48 
  src/slave/flags.cpp b7df8f760d0f75459f1e80e3d8e18d49a3995df8 
  src/tests/container_logger_tests.cpp efadceafca5721bce4dbffadb35f54fd5365abb0 
  src/tests/containerizer/docker_volume_isolator_tests.cpp 
c524f42743bf08ee54f1cbb083d0d3c85a8b70c9 
  src/tests/containerizer/filesystem_isolator_tests.cpp 
4293416ac8434e9eb7e80724480a54936a2fe24a 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
09742ff21513dc2570684d384b257868dd57a9ce 

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


Testing
---

make check; used mesos cli to test end to end functionality.


Thanks,

Jojy Varghese



Re: Review Request 46370: Introduced linux capabilities API.

2016-05-17 Thread Jojy Varghese

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

(Updated May 17, 2016, 2:59 p.m.)


Review request for mesos, Jie Yu and Kevin Klues.


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


Repository: mesos


Description
---

This change introduces basic API for linux capabilities. This is not a
comprehensive API but is strictly limited to the need for securing Mesos
containers using linux capabilities.


Diffs (updated)
-

  src/CMakeLists.txt e0c538d9e6542fbe279bfbf6f20172e4c611c859 
  src/Makefile.am ce5245883f3d2661812272702c0d2060513b6d88 
  src/linux/capabilities.hpp PRE-CREATION 
  src/linux/capabilities.cpp PRE-CREATION 

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


Testing
---

make check.


Thanks,

Jojy Varghese



Re: Review Request 46498: Add runtime for Appc Spec ex: command, workingdir and environment.

2016-05-16 Thread Jojy Varghese

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




src/slave/containerizer/mesos/containerizer.cpp (line 953)
<https://reviews.apache.org/r/46498/#comment197768>

It would be readable if we separat the two `if` blocks with a blanlk line.



src/slave/containerizer/mesos/isolators/appc/runtime.hpp (line 26)
<https://reviews.apache.org/r/46498/#comment197770>

s/APPC/AppC? Please look for more occurrences.



src/slave/containerizer/mesos/isolators/appc/runtime.hpp (line 29)
<https://reviews.apache.org/r/46498/#comment197774>

s/Appc/AppC as Guangya proposed?



src/slave/containerizer/mesos/isolators/appc/runtime.hpp (lines 64 - 73)
<https://reviews.apache.org/r/46498/#comment197786>

Do these need to be member functions? Could they be functions in `cpp` file?



src/slave/containerizer/mesos/isolators/appc/runtime.cpp (line 50)
<https://reviews.apache.org/r/46498/#comment19>

No need to wrap to newline. It fits within 80 chars.



src/slave/containerizer/mesos/isolators/appc/runtime.cpp (line 147)
<https://reviews.apache.org/r/46498/#comment197787>

blank line above.



src/slave/containerizer/mesos/isolators/appc/runtime.cpp (line 171)
<https://reviews.apache.org/r/46498/#comment197788>

blank line above.



src/slave/containerizer/mesos/isolators/appc/runtime.cpp (line 174)
<https://reviews.apache.org/r/46498/#comment197789>

blank line above



src/slave/containerizer/mesos/isolators/appc/runtime.cpp (line 246)
<https://reviews.apache.org/r/46498/#comment197791>

I think you should use `string::empty`



src/slave/containerizer/mesos/provisioner/appc/store.cpp (line 201)
<https://reviews.apache.org/r/46498/#comment197795>

Do you need this variable?



src/slave/containerizer/mesos/provisioner/appc/store.cpp (line 203)
<https://reviews.apache.org/r/46498/#comment197796>

I think you just need the base image manifest here and not all the 
manifests. Right?


- Jojy Varghese


On May 14, 2016, 4:20 a.m., Srinivas Brahmaroutu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46498/
> ---
> 
> (Updated May 14, 2016, 4:20 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Jojy Varghese.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add runtime for Appc Spec ex: command, workingdir and environment.
> 
> 
> Diffs
> -
> 
>   include/mesos/appc/spec.proto 4ef0473748c2f47aff1729c1d969185803fe72d4 
>   include/mesos/slave/isolator.proto 60a9bb637e12593a97ed1a7c510ebccd4e5a9615 
>   src/Makefile.am 447bc2ab511ad173d3d911be10992be0974f4584 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 75e5a32a3e70ec60a6800e21a621673184ea0956 
>   src/slave/containerizer/mesos/isolators/appc/runtime.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/appc/runtime.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/provisioner/appc/store.cpp 
> 33e97fc645a9b7fbc9ae47f67c1b5dacf999fce0 
>   src/slave/containerizer/mesos/provisioner/provisioner.hpp 
> 5c96e9f6603d39889e6bc807874d35d0cb3556be 
>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 
> dcbbbaf797d4467bfd0bb1ee91ee9ce843e7d546 
>   src/slave/containerizer/mesos/provisioner/store.hpp 
> 1d477ef13ddd24fd8badae0decaa4a2271ecc746 
>   src/tests/containerizer/provisioner_appc_tests.cpp 
> 84fe52b6937c3b7d7628b17a2f045eec2f386b4d 
> 
> Diff: https://reviews.apache.org/r/46498/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Srinivas Brahmaroutu
> 
>



Re: Review Request 46799: Added capabilities support to mesos-execute.

2016-05-13 Thread Jojy Varghese

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

(Updated May 13, 2016, 4:46 p.m.)


Review request for mesos and Jie Yu.


Repository: mesos


Description
---

This change introduces two flags: `user` and `capabilities`.

   user:  used to specify the user name for the command.
   capabilities: comma separated list of capabilities the task
 requires.


Diffs (updated)
-

  src/cli/execute.cpp 4711e80314e2fc4cde077bebd9a8167324c3254e 

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


Testing
---

make check.


Thanks,

Jojy Varghese



Re: Review Request 46798: Introduced linux capabilities support for mesos containerizer.

2016-05-12 Thread Jojy Varghese

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

(Updated May 13, 2016, 12:50 a.m.)


Review request for mesos and Jie Yu.


Repository: mesos


Description (updated)
---

This change introduces linux capability based security for unified
containerizer. A new agent flag \`allowed_capabilities\` has been
introduced to override the default capabilities of the user or the
capabilities requested by the user.

This feature is only available on linux.


Diffs (updated)
-

  src/launcher/executor.cpp fa4a89e97b0c427b4595a6adebc397aeb5bfaaa5 
  src/slave/containerizer/mesos/containerizer.hpp 
a1a00020668f6da8d611f26e5637afffc87d09ba 
  src/slave/containerizer/mesos/containerizer.cpp 
75e5a32a3e70ec60a6800e21a621673184ea0956 
  src/slave/containerizer/mesos/launch.hpp 
c716e0396736d1f2f60ec31540f12f4f7597d081 
  src/slave/containerizer/mesos/launch.cpp 
e22106b014c871e2184a15c2ab154a0674874e47 
  src/slave/flags.hpp 4fa3213545d4bd3525d85c3f71749f00f08dc998 
  src/slave/flags.cpp 6fde51fc61cfcad61d4085c208bd2eca2eae8f14 
  src/tests/container_logger_tests.cpp efadceafca5721bce4dbffadb35f54fd5365abb0 
  src/tests/containerizer/filesystem_isolator_tests.cpp 
4293416ac8434e9eb7e80724480a54936a2fe24a 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
09742ff21513dc2570684d384b257868dd57a9ce 

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


Testing
---

make check; used mesos cli to test end to end functionality.


Thanks,

Jojy Varghese



Re: Review Request 46799: Added capabilities support to mesos-execute.

2016-05-12 Thread Jojy Varghese

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

(Updated May 13, 2016, 12:50 a.m.)


Review request for mesos and Jie Yu.


Repository: mesos


Description
---

This change introduces two flags: `user` and `capabilities`.

   user:  used to specify the user name for the command.
   capabilities: comma separated list of capabilities the task
 requires.


Diffs (updated)
-

  src/cli/execute.cpp 4711e80314e2fc4cde077bebd9a8167324c3254e 

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


Testing
---

make check.


Thanks,

Jojy Varghese



Re: Review Request 46370: Introduced linux capabilities API.

2016-05-12 Thread Jojy Varghese

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

(Updated May 13, 2016, 12:45 a.m.)


Review request for mesos, Jie Yu and Kevin Klues.


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


Repository: mesos


Description
---

This change introduces basic API for linux capabilities. This is not a
comprehensive API but is strictly limited to the need for securing Mesos
containers using linux capabilities.


Diffs (updated)
-

  src/CMakeLists.txt e0c538d9e6542fbe279bfbf6f20172e4c611c859 
  src/Makefile.am 447bc2ab511ad173d3d911be10992be0974f4584 
  src/linux/capabilities.hpp PRE-CREATION 
  src/linux/capabilities.cpp PRE-CREATION 

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


Testing
---

make check.


Thanks,

Jojy Varghese



Re: Review Request 46498: Add runtime for Appc Spec ex: command, workingdir and environment.

2016-05-12 Thread Jojy Varghese

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



Srinivas, thanks for taking this on. It looks like you combined your previous 
patches (https://reviews.apache.org/r/46107) here. I would advice you to split 
this patch into 2:
 - Protobuf changes
 - Other changes
 

-jojy

- Jojy Varghese


On May 12, 2016, 9:23 p.m., Srinivas Brahmaroutu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46498/
> ---
> 
> (Updated May 12, 2016, 9:23 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Jojy Varghese.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add runtime for Appc Spec ex: command, workingdir and environment.
> 
> 
> Diffs
> -
> 
>   include/mesos/appc/spec.proto 4ef0473748c2f47aff1729c1d969185803fe72d4 
>   include/mesos/slave/isolator.proto 60a9bb637e12593a97ed1a7c510ebccd4e5a9615 
>   src/Makefile.am 922b5984b67ace97c142b52aaa8221323147a7ff 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 1e1a36903f4377497bb72b69e4ead63675d453c0 
>   src/slave/containerizer/mesos/isolators/appc/runtime.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/appc/runtime.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/provisioner/appc/store.cpp 
> 33e97fc645a9b7fbc9ae47f67c1b5dacf999fce0 
>   src/slave/containerizer/mesos/provisioner/provisioner.hpp 
> 5c96e9f6603d39889e6bc807874d35d0cb3556be 
>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 
> dcbbbaf797d4467bfd0bb1ee91ee9ce843e7d546 
>   src/slave/containerizer/mesos/provisioner/store.hpp 
> 1d477ef13ddd24fd8badae0decaa4a2271ecc746 
>   src/tests/containerizer/provisioner_appc_tests.cpp 
> 84fe52b6937c3b7d7628b17a2f045eec2f386b4d 
> 
> Diff: https://reviews.apache.org/r/46498/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Srinivas Brahmaroutu
> 
>



Re: Review Request 46371: Added basic tests for capabilities API.

2016-05-10 Thread Jojy Varghese

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

(Updated May 11, 2016, 4:34 a.m.)


Review request for mesos and Jie Yu.


Changes
---

rebased.


Repository: mesos


Description
---

Added basic tests for capabilities API.


Diffs (updated)
-

  src/Makefile.am 53de98f43629dc94f7619324369caf88407b2f41 
  src/tests/capabilities_test_helper.hpp PRE-CREATION 
  src/tests/capabilities_test_helper.cpp PRE-CREATION 
  src/tests/capabilities_test_helper_main.cpp PRE-CREATION 
  src/tests/capabilities_tests.cpp PRE-CREATION 

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


Testing
---

make check.


Thanks,

Jojy Varghese



Re: Review Request 46370: Introduced linux capabilities API.

2016-05-10 Thread Jojy Varghese

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

(Updated May 11, 2016, 4:34 a.m.)


Review request for mesos, Jie Yu and Kevin Klues.


Changes
---

review addressed.


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


Repository: mesos


Description
---

This change introduces basic API for linux capabilities. This is not a
comprehensive API but is strictly limited to the need for securing Mesos
containers using linux capabilities.


Diffs (updated)
-

  src/CMakeLists.txt f991743f1a467f24a786e925983390a411792327 
  src/Makefile.am 53de98f43629dc94f7619324369caf88407b2f41 
  src/linux/capabilities.hpp PRE-CREATION 
  src/linux/capabilities.cpp PRE-CREATION 

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


Testing
---

make check.


Thanks,

Jojy Varghese



Re: Review Request 46369: Added capabilities support in ContanerInfo protobuf.

2016-05-10 Thread Jojy Varghese

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

(Updated May 11, 2016, 4:27 a.m.)


Review request for mesos and Jie Yu.


Changes
---

review addressed.


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


Repository: mesos


Description
---

Added capabilities support in ContanerInfo protobuf.


Diffs (updated)
-

  include/mesos/mesos.proto 9a180304996895e2e003085690a7dff9ec561e9c 
  include/mesos/v1/mesos.proto 44b4f8a059f9dfdcbf02f0c30c1b859898c2e617 

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


Testing
---

make check.


Thanks,

Jojy Varghese



Re: Review Request 46370: Introduced linux capabilities API.

2016-05-09 Thread Jojy Varghese


> On May 7, 2016, 7:14 p.m., Jie Yu wrote:
> > src/linux/capabilities.hpp, line 194
> > <https://reviews.apache.org/r/46370/diff/7/?file=1374923#file1374923line194>
> >
> > When I review the code, I found very confusing. We have 
> > `CapabilityInfo, CapabilityInfo::Capability, Capability, Capabilities, 
> > CapabilityFlags, CapabilitySet`. Also, looking at the interfaces of 
> > `Capabilities`, it's not straightfoward to extend it to support file 
> > capabilities. If possible, I'd like the library here that's easily 
> > extensible to support file capabilities.
> > 
> > In fact, I like the abstraction in the go library
> > 
> > https://github.com/syndtr/gocapability/blob/master/capability/capability.go
> > 
> > So, basically, `Capabilities` will become the struct that contains all 
> > capabilities (permitted, inheritted, etc) if applied. and it has methods 
> > defined allowing callers to modify any of them. Finally, there's an `Apply` 
> > method which can be used to apply the change. The methods will be virtual 
> > so that in the future, we can use that to support file capabilities as well:
> > ```
> > typedef int Capability
> > 
> > constexpr int CHOWN = 0;
> > ...
> > 
> > 
> > enum Type
> > {
> >   EFFECTIVE,
> >   PERMITTED,
> >   INHERITABLE,
> >   BOUNDING
> > };
> > 
> > 
> > class Capabilities
> > {
> > public:
> >   static Try<Owned> fromPid(const Option& pid = 
> > None());
> >   static Try<Owned> fromFile(const string& path); // TODO.
> >   
> >   virtual bool test(Type type, Capability cap);
> >   virtual void set(Type type, const hashset& caps);
> >   virtual void unset(Type type, const hashset& caps);
> >   virtual void fill(Type type);
> >   virtual void clear(Type type);
> >   
> >   virtual Try apply();
> > };
> > 
> > 
> > // Returns the set of capabilities specified in the protobuf.
> > hashset parse(const CapabilityInfo& info);
> > ```
> > 
> > In the cpp file, you can create a `class ProcessCapabilities : public 
> > Capabilities` that implements the above virtual methods. 
> > 
> > For version and lastCapability, i think it can be a global variable in 
> > the cpp file which will be set the first time `version` is called. Any 
> > subsequent calling of `version` will use the cached value:
> > 
> > ```
> > Try version();
> > Try lastCapability();
> > 
> > Try<Owned> ProcessCapabilities::create(const 
> > Option& pid = None())
> > {
> >   // you can save version and lastCap in ProcessCapabilities.
> > }
> > ```
> 
> Jojy Varghese wrote:
> Thanks for the review Jie. 
> 
> The idea behind separate classes is to keep the functionality cohesive 
> which I think is an important property of any API. I believe that `test`, 
> `fill`, `clear` etc is not part of the `capabilities` domain  but they change 
> state of `flags`. `Flags` are first class citizens of the capabilities 
> domain. Also, ideally the `capabilities` API should only have `set` and 
> `get`, i had to add `drop` and others to satisfy some of our other 
> requirements. I think if i pull them out into  functions(not part of the 
> class), the `capabilities` class will have only `set` and `get` of flags.
> 
> Also, `set` is an array of `capabilities`. In linux, libcap is supposed 
> to be the standard portable API and I tried to follow that API more that the 
> `go` API.  In libcap, there are functions like `cap_set_flag` to set a 
> particular capabilility (CHOWN) in a set 
> (`http://linux.die.net/man/3/cap_set_flag`). I have tried to put those as 
> part of the `CapabiityFlag` and `CapabilitySet` classes.
> 
> Also, I believe that we should have `Capability` (NET_RAW, CHOWN etc) as 
> first class citizens of the API. For example libcap uses `cap_value_t` as the 
> type for the capabilities. I added `enum Capability` as the type.
> 
> Jie Yu wrote:
> I understand that your interface is modeled after libcap. I looked at 
> both and found that libcap interfaces are a bit confusing and hard to use. In 
> fact, if you looked at https://people.redhat.com/sgrubb/libcap-ng/, "The 
> libcap-ng library is intended to make programming with posix capabilities 
> much easier than the traditional libcap library

Re: Review Request 46370: Introduced linux capabilities API.

2016-05-08 Thread Jojy Varghese


> On May 7, 2016, 7:14 p.m., Jie Yu wrote:
> > src/linux/capabilities.hpp, line 194
> > <https://reviews.apache.org/r/46370/diff/7/?file=1374923#file1374923line194>
> >
> > When I review the code, I found very confusing. We have 
> > `CapabilityInfo, CapabilityInfo::Capability, Capability, Capabilities, 
> > CapabilityFlags, CapabilitySet`. Also, looking at the interfaces of 
> > `Capabilities`, it's not straightfoward to extend it to support file 
> > capabilities. If possible, I'd like the library here that's easily 
> > extensible to support file capabilities.
> > 
> > In fact, I like the abstraction in the go library
> > 
> > https://github.com/syndtr/gocapability/blob/master/capability/capability.go
> > 
> > So, basically, `Capabilities` will become the struct that contains all 
> > capabilities (permitted, inheritted, etc) if applied. and it has methods 
> > defined allowing callers to modify any of them. Finally, there's an `Apply` 
> > method which can be used to apply the change. The methods will be virtual 
> > so that in the future, we can use that to support file capabilities as well:
> > ```
> > typedef int Capability
> > 
> > constexpr int CHOWN = 0;
> > ...
> > 
> > 
> > enum Type
> > {
> >   EFFECTIVE,
> >   PERMITTED,
> >   INHERITABLE,
> >   BOUNDING
> > };
> > 
> > 
> > class Capabilities
> > {
> > public:
> >   static Try<Owned> fromPid(const Option& pid = 
> > None());
> >   static Try<Owned> fromFile(const string& path); // TODO.
> >   
> >   virtual bool test(Type type, Capability cap);
> >   virtual void set(Type type, const hashset& caps);
> >   virtual void unset(Type type, const hashset& caps);
> >   virtual void fill(Type type);
> >   virtual void clear(Type type);
> >   
> >   virtual Try apply();
> > };
> > 
> > 
> > // Returns the set of capabilities specified in the protobuf.
> > hashset parse(const CapabilityInfo& info);
> > ```
> > 
> > In the cpp file, you can create a `class ProcessCapabilities : public 
> > Capabilities` that implements the above virtual methods. 
> > 
> > For version and lastCapability, i think it can be a global variable in 
> > the cpp file which will be set the first time `version` is called. Any 
> > subsequent calling of `version` will use the cached value:
> > 
> > ```
> > Try version();
> > Try lastCapability();
> > 
> > Try<Owned> ProcessCapabilities::create(const 
> > Option& pid = None())
> > {
> >   // you can save version and lastCap in ProcessCapabilities.
> > }
> > ```
> 
> Jojy Varghese wrote:
> Thanks for the review Jie. 
> 
> The idea behind separate classes is to keep the functionality cohesive 
> which I think is an important property of any API. I believe that `test`, 
> `fill`, `clear` etc is not part of the `capabilities` domain  but they change 
> state of `flags`. `Flags` are first class citizens of the capabilities 
> domain. Also, ideally the `capabilities` API should only have `set` and 
> `get`, i had to add `drop` and others to satisfy some of our other 
> requirements. I think if i pull them out into  functions(not part of the 
> class), the `capabilities` class will have only `set` and `get` of flags.
> 
> Also, `set` is an array of `capabilities`. In linux, libcap is supposed 
> to be the standard portable API and I tried to follow that API more that the 
> `go` API.  In libcap, there are functions like `cap_set_flag` to set a 
> particular capabilility (CHOWN) in a set 
> (`http://linux.die.net/man/3/cap_set_flag`). I have tried to put those as 
> part of the `CapabiityFlag` and `CapabilitySet` classes.
> 
> Also, I believe that we should have `Capability` (NET_RAW, CHOWN etc) as 
> first class citizens of the API. For example libcap uses `cap_value_t` as the 
> type for the capabilities. I added `enum Capability` as the type.
> 
> Jie Yu wrote:
> I understand that your interface is modeled after libcap. I looked at 
> both and found that libcap interfaces are a bit confusing and hard to use. In 
> fact, if you looked at https://people.redhat.com/sgrubb/libcap-ng/, "The 
> libcap-ng library is intended to make programming with posix capabilities 
> much easier than the traditional libcap library

Re: Review Request 46370: Introduced linux capabilities API.

2016-05-07 Thread Jojy Varghese


> On May 7, 2016, 7:14 p.m., Jie Yu wrote:
> > src/linux/capabilities.hpp, line 194
> > <https://reviews.apache.org/r/46370/diff/7/?file=1374923#file1374923line194>
> >
> > When I review the code, I found very confusing. We have 
> > `CapabilityInfo, CapabilityInfo::Capability, Capability, Capabilities, 
> > CapabilityFlags, CapabilitySet`. Also, looking at the interfaces of 
> > `Capabilities`, it's not straightfoward to extend it to support file 
> > capabilities. If possible, I'd like the library here that's easily 
> > extensible to support file capabilities.
> > 
> > In fact, I like the abstraction in the go library
> > 
> > https://github.com/syndtr/gocapability/blob/master/capability/capability.go
> > 
> > So, basically, `Capabilities` will become the struct that contains all 
> > capabilities (permitted, inheritted, etc) if applied. and it has methods 
> > defined allowing callers to modify any of them. Finally, there's an `Apply` 
> > method which can be used to apply the change. The methods will be virtual 
> > so that in the future, we can use that to support file capabilities as well:
> > ```
> > typedef int Capability
> > 
> > constexpr int CHOWN = 0;
> > ...
> > 
> > 
> > enum Type
> > {
> >   EFFECTIVE,
> >   PERMITTED,
> >   INHERITABLE,
> >   BOUNDING
> > };
> > 
> > 
> > class Capabilities
> > {
> > public:
> >   static Try<Owned> fromPid(const Option& pid = 
> > None());
> >   static Try<Owned> fromFile(const string& path); // TODO.
> >   
> >   virtual bool test(Type type, Capability cap);
> >   virtual void set(Type type, const hashset& caps);
> >   virtual void unset(Type type, const hashset& caps);
> >   virtual void fill(Type type);
> >   virtual void clear(Type type);
> >   
> >   virtual Try apply();
> > };
> > 
> > 
> > // Returns the set of capabilities specified in the protobuf.
> > hashset parse(const CapabilityInfo& info);
> > ```
> > 
> > In the cpp file, you can create a `class ProcessCapabilities : public 
> > Capabilities` that implements the above virtual methods. 
> > 
> > For version and lastCapability, i think it can be a global variable in 
> > the cpp file which will be set the first time `version` is called. Any 
> > subsequent calling of `version` will use the cached value:
> > 
> > ```
> > Try version();
> > Try lastCapability();
> > 
> > Try<Owned> ProcessCapabilities::create(const 
> > Option& pid = None())
> > {
> >   // you can save version and lastCap in ProcessCapabilities.
> > }
> > ```
> 
> Jojy Varghese wrote:
> Thanks for the review Jie. 
> 
> The idea behind separate classes is to keep the functionality cohesive 
> which I think is an important property of any API. I believe that `test`, 
> `fill`, `clear` etc is not part of the `capabilities` domain  but they change 
> state of `flags`. `Flags` are first class citizens of the capabilities 
> domain. Also, ideally the `capabilities` API should only have `set` and 
> `get`, i had to add `drop` and others to satisfy some of our other 
> requirements. I think if i pull them out into  functions(not part of the 
> class), the `capabilities` class will have only `set` and `get` of flags.
> 
> Also, `set` is an array of `capabilities`. In linux, libcap is supposed 
> to be the standard portable API and I tried to follow that API more that the 
> `go` API.  In libcap, there are functions like `cap_set_flag` to set a 
> particular capabilility (CHOWN) in a set 
> (`http://linux.die.net/man/3/cap_set_flag`). I have tried to put those as 
> part of the `CapabiityFlag` and `CapabilitySet` classes.
> 
> Also, I believe that we should have `Capability` (NET_RAW, CHOWN etc) as 
> first class citizens of the API. For example libcap uses `cap_value_t` as the 
> type for the capabilities. I added `enum Capability` as the type.
> 
> Jie Yu wrote:
> I understand that your interface is modeled after libcap. I looked at 
> both and found that libcap interfaces are a bit confusing and hard to use. In 
> fact, if you looked at https://people.redhat.com/sgrubb/libcap-ng/, "The 
> libcap-ng library is intended to make programming with posix capabilities 
> much easier than the traditional libc

Re: Review Request 46370: Introduced linux capabilities API.

2016-05-07 Thread Jojy Varghese


> On May 7, 2016, 7:14 p.m., Jie Yu wrote:
> > src/linux/capabilities.hpp, line 194
> > <https://reviews.apache.org/r/46370/diff/7/?file=1374923#file1374923line194>
> >
> > When I review the code, I found very confusing. We have 
> > `CapabilityInfo, CapabilityInfo::Capability, Capability, Capabilities, 
> > CapabilityFlags, CapabilitySet`. Also, looking at the interfaces of 
> > `Capabilities`, it's not straightfoward to extend it to support file 
> > capabilities. If possible, I'd like the library here that's easily 
> > extensible to support file capabilities.
> > 
> > In fact, I like the abstraction in the go library
> > 
> > https://github.com/syndtr/gocapability/blob/master/capability/capability.go
> > 
> > So, basically, `Capabilities` will become the struct that contains all 
> > capabilities (permitted, inheritted, etc) if applied. and it has methods 
> > defined allowing callers to modify any of them. Finally, there's an `Apply` 
> > method which can be used to apply the change. The methods will be virtual 
> > so that in the future, we can use that to support file capabilities as well:
> > ```
> > typedef int Capability
> > 
> > constexpr int CHOWN = 0;
> > ...
> > 
> > 
> > enum Type
> > {
> >   EFFECTIVE,
> >   PERMITTED,
> >   INHERITABLE,
> >   BOUNDING
> > };
> > 
> > 
> > class Capabilities
> > {
> > public:
> >   static Try<Owned> fromPid(const Option& pid = 
> > None());
> >   static Try<Owned> fromFile(const string& path); // TODO.
> >   
> >   virtual bool test(Type type, Capability cap);
> >   virtual void set(Type type, const hashset& caps);
> >   virtual void unset(Type type, const hashset& caps);
> >   virtual void fill(Type type);
> >   virtual void clear(Type type);
> >   
> >   virtual Try apply();
> > };
> > 
> > 
> > // Returns the set of capabilities specified in the protobuf.
> > hashset parse(const CapabilityInfo& info);
> > ```
> > 
> > In the cpp file, you can create a `class ProcessCapabilities : public 
> > Capabilities` that implements the above virtual methods. 
> > 
> > For version and lastCapability, i think it can be a global variable in 
> > the cpp file which will be set the first time `version` is called. Any 
> > subsequent calling of `version` will use the cached value:
> > 
> > ```
> > Try version();
> > Try lastCapability();
> > 
> > Try<Owned> ProcessCapabilities::create(const 
> > Option& pid = None())
> > {
> >   // you can save version and lastCap in ProcessCapabilities.
> > }
> > ```

Thanks for the review Jie. 

The idea behind separate classes is to keep the functionality cohesive which I 
think is an important property of any API. I believe that `test`, `fill`, 
`clear` etc is not part of the `capabilities` domain  but they change state of 
`flags`. `Flags` are first class citizens of the capabilities domain. Also, 
ideally the `capabilities` API should only have `set` and `get`, i had to add 
`drop` and others to satisfy some of our other requirements. I think if i pull 
them out into  functions(not part of the class), the `capabilities` class will 
have only `set` and `get` of flags.

Also, `set` is an array of `capabilities`. In linux, libcap is supposed to be 
the standard portable API and I tried to follow that API more that the `go` 
API.  In libcap, there are functions like `cap_set_flag` to set a particular 
capabilility (CHOWN) in a set (`http://linux.die.net/man/3/cap_set_flag`). I 
have tried to put those as part of the `CapabiityFlag` and `CapabilitySet` 
classes.

Also, I believe that we should have `Capability` (NET_RAW, CHOWN etc) as first 
class citizens of the API. For example libcap uses `cap_value_t` as the type 
for the capabilities. I added `enum Capability` as the type.


- Jojy


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


On May 6, 2016, 5:03 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46370/
> ---
> 
> (Updated May 6, 

Re: Review Request 46799: Added capabilities support to mesos-execute.

2016-05-06 Thread Jojy Varghese


> On April 29, 2016, 12:19 a.m., Qian Zhang wrote:
> > src/cli/execute.cpp, lines 172-175
> > <https://reviews.apache.org/r/46799/diff/1/?file=1365019#file1365019line172>
> >
> > Can we only add this flag in Linux? Then user will not see this flag at 
> > all when running `mesos-execute` in other platorms.
> 
> Jojy Varghese wrote:
> I avoided that logic as it would mean spreading `ifdef` all over the file.
> 
> Qian Zhang wrote:
> Right, but I think that is the common way to handle the platform-specific 
> logic, you can take a look at: 
> https://github.com/apache/mesos/blob/master/src/slave/containerizer/mesos/containerizer.cpp,
>  in this file, we use `#ifdef __linux__` for all the isolators which can be 
> only enabled on Linux.

I have looked at the platform speific ifdefs in the code and I am not sure if 
that is the right way to do platform specific code. Linux kernel is a good 
example of how it actually should be done. But I am changing the code in cli 
for now for uniformity.


- Jojy


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


On May 6, 2016, 5:06 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46799/
> ---
> 
> (Updated May 6, 2016, 5:06 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change introduces two flags: `user` and `capabilities`.
> 
>user:  used to specify the user name for the command.
>capabilities: comma separated list of capabilities the task
>  requires.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp 4711e80314e2fc4cde077bebd9a8167324c3254e 
> 
> Diff: https://reviews.apache.org/r/46799/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>



Re: Review Request 46799: Added capabilities support to mesos-execute.

2016-05-06 Thread Jojy Varghese

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

(Updated May 6, 2016, 5:06 p.m.)


Review request for mesos and Jie Yu.


Repository: mesos


Description
---

This change introduces two flags: `user` and `capabilities`.

   user:  used to specify the user name for the command.
   capabilities: comma separated list of capabilities the task
 requires.


Diffs (updated)
-

  src/cli/execute.cpp 4711e80314e2fc4cde077bebd9a8167324c3254e 

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


Testing
---

make check.


Thanks,

Jojy Varghese



Re: Review Request 46798: Introduced linux capabilities support for mesos containerizer.

2016-05-06 Thread Jojy Varghese

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

(Updated May 6, 2016, 5:05 p.m.)


Review request for mesos and Jie Yu.


Repository: mesos


Description
---

This change introduces linux capability based security for unified
containerizer. A new agent flag `allowed_capabilities` has been
introduced to override the default capabilities of the user or the
capabilities requested by the user.

This feature is only available on linux.


Diffs (updated)
-

  src/launcher/executor.cpp 9f1d2168bc4ddbce1bcd25ff38dc1c34714eb28b 
  src/slave/containerizer/mesos/containerizer.hpp 
13399f014dcd85defbff79f3b5aa4e7e75d41fd1 
  src/slave/containerizer/mesos/containerizer.cpp 
8d538954d6e1f13e833d75c2eaa37e700278ee0c 
  src/slave/containerizer/mesos/launch.hpp 
c716e0396736d1f2f60ec31540f12f4f7597d081 
  src/slave/containerizer/mesos/launch.cpp 
e22106b014c871e2184a15c2ab154a0674874e47 
  src/slave/flags.hpp 4fa3213545d4bd3525d85c3f71749f00f08dc998 
  src/slave/flags.cpp 6fde51fc61cfcad61d4085c208bd2eca2eae8f14 
  src/tests/container_logger_tests.cpp efadceafca5721bce4dbffadb35f54fd5365abb0 
  src/tests/containerizer/filesystem_isolator_tests.cpp 
29d313051865761306029f331eb36684c3252ffb 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
09742ff21513dc2570684d384b257868dd57a9ce 

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


Testing
---

make check; used mesos cli to test end to end functionality.


Thanks,

Jojy Varghese



Re: Review Request 46370: Introduced linux capabilities API.

2016-05-06 Thread Jojy Varghese

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

(Updated May 6, 2016, 5:03 p.m.)


Review request for mesos, Jie Yu and Kevin Klues.


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


Repository: mesos


Description
---

This change introduces basic API for linux capabilities. This is not a
comprehensive API but is strictly limited to the need for securing Mesos
containers using linux capabilities.


Diffs (updated)
-

  src/CMakeLists.txt f991743f1a467f24a786e925983390a411792327 
  src/Makefile.am 53de98f43629dc94f7619324369caf88407b2f41 
  src/linux/capabilities.hpp PRE-CREATION 
  src/linux/capabilities.cpp PRE-CREATION 

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


Testing
---

make check.


Thanks,

Jojy Varghese



Re: Review Request 46369: Added capabilities support in ContanerInfo protobuf.

2016-05-06 Thread Jojy Varghese

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

(Updated May 6, 2016, 5:02 p.m.)


Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Added capabilities support in ContanerInfo protobuf.


Diffs (updated)
-

  include/mesos/mesos.proto 9a180304996895e2e003085690a7dff9ec561e9c 
  include/mesos/v1/mesos.proto 44b4f8a059f9dfdcbf02f0c30c1b859898c2e617 

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


Testing
---

make check.


Thanks,

Jojy Varghese



Re: Review Request 46371: Added basic tests for capabilities API.

2016-05-05 Thread Jojy Varghese

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

(Updated May 5, 2016, 9:01 p.m.)


Review request for mesos and Jie Yu.


Repository: mesos


Description
---

Added basic tests for capabilities API.


Diffs (updated)
-

  src/Makefile.am 53de98f43629dc94f7619324369caf88407b2f41 
  src/tests/capabilities_test_helper.hpp PRE-CREATION 
  src/tests/capabilities_test_helper.cpp PRE-CREATION 
  src/tests/capabilities_test_helper_main.cpp PRE-CREATION 
  src/tests/capabilities_tests.cpp PRE-CREATION 

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


Testing
---

make check.


Thanks,

Jojy Varghese



Re: Review Request 46798: Introduced linux capabilities support for mesos containerizer.

2016-05-05 Thread Jojy Varghese

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

(Updated May 5, 2016, 9 p.m.)


Review request for mesos and Jie Yu.


Repository: mesos


Description
---

This change introduces linux capability based security for unified
containerizer. A new agent flag `allowed_capabilities` has been
introduced to override the default capabilities of the user or the
capabilities requested by the user.

This feature is only available on linux.


Diffs (updated)
-

  src/launcher/executor.cpp 9f1d2168bc4ddbce1bcd25ff38dc1c34714eb28b 
  src/slave/containerizer/mesos/containerizer.hpp 
13399f014dcd85defbff79f3b5aa4e7e75d41fd1 
  src/slave/containerizer/mesos/containerizer.cpp 
8d538954d6e1f13e833d75c2eaa37e700278ee0c 
  src/slave/containerizer/mesos/launch.hpp 
c716e0396736d1f2f60ec31540f12f4f7597d081 
  src/slave/containerizer/mesos/launch.cpp 
e22106b014c871e2184a15c2ab154a0674874e47 
  src/slave/flags.hpp 4fa3213545d4bd3525d85c3f71749f00f08dc998 
  src/slave/flags.cpp 6fde51fc61cfcad61d4085c208bd2eca2eae8f14 
  src/tests/container_logger_tests.cpp efadceafca5721bce4dbffadb35f54fd5365abb0 
  src/tests/containerizer/filesystem_isolator_tests.cpp 
29d313051865761306029f331eb36684c3252ffb 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
09742ff21513dc2570684d384b257868dd57a9ce 

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


Testing
---

make check; used mesos cli to test end to end functionality.


Thanks,

Jojy Varghese



Re: Review Request 46370: Introduced linux capabilities API.

2016-05-05 Thread Jojy Varghese

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

(Updated May 5, 2016, 7:42 p.m.)


Review request for mesos, Jie Yu and Kevin Klues.


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


Repository: mesos


Description
---

This change introduces basic API for linux capabilities. This is not a
comprehensive API but is strictly limited to the need for securing Mesos
containers using linux capabilities.


Diffs (updated)
-

  src/CMakeLists.txt f991743f1a467f24a786e925983390a411792327 
  src/Makefile.am 53de98f43629dc94f7619324369caf88407b2f41 
  src/linux/capabilities.hpp PRE-CREATION 
  src/linux/capabilities.cpp PRE-CREATION 

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


Testing
---

make check.


Thanks,

Jojy Varghese



Re: Review Request 46107: Add Appc runtime spec for command, working directory and environment.

2016-05-03 Thread Jojy Varghese

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




include/mesos/appc/spec.proto (line 71)
<https://reviews.apache.org/r/46107/#comment195542>

I think the message ids should ordered by their order in the spec (see 
`https://github.com/appc/spec/blob/master/spec/aci.md`). So `app` should be 
`5`. But we have `annotation` labeled 5 unfortunately. 

In `App`, lets try to follow the order specified in the spec. So IMO, 
`workingDirectory` should be atleast `6`.



include/mesos/slave/isolator.proto (line 22)
<https://reviews.apache.org/r/46107/#comment195539>

Please sort the include files in alphabetical order. So it would be:
```
import "mesos/appc/spec.proto";

import "mesos/docker/v1.proto";
```



include/mesos/slave/isolator.proto (line 94)
<https://reviews.apache.org/r/46107/#comment195537>

I would place this message above the `Docker` messgae.


- Jojy Varghese


On April 12, 2016, 6:32 p.m., Srinivas Brahmaroutu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46107/
> ---
> 
> (Updated April 12, 2016, 6:32 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add Appc runtime spec for command, working directory and environment.
> 
> 
> Diffs
> -
> 
>   include/mesos/appc/spec.proto 4ef0473748c2f47aff1729c1d969185803fe72d4 
>   include/mesos/slave/isolator.proto 60a9bb637e12593a97ed1a7c510ebccd4e5a9615 
> 
> Diff: https://reviews.apache.org/r/46107/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Srinivas Brahmaroutu
> 
>



Re: Review Request 46798: Introduced linux capabilities support for mesos containerizer.

2016-05-02 Thread Jojy Varghese

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

(Updated May 2, 2016, 5:12 p.m.)


Review request for mesos and Jie Yu.


Repository: mesos


Description
---

This change introduces linux capability based security for unified
containerizer. A new agent flag `allowed_capabilities` has been
introduced to override the default capabilities of the user or the
capabilities requested by the user.

This feature is only available on linux.


Diffs (updated)
-

  src/launcher/executor.cpp 9f1d2168bc4ddbce1bcd25ff38dc1c34714eb28b 
  src/slave/containerizer/mesos/containerizer.hpp 
13399f014dcd85defbff79f3b5aa4e7e75d41fd1 
  src/slave/containerizer/mesos/containerizer.cpp 
8d538954d6e1f13e833d75c2eaa37e700278ee0c 
  src/slave/containerizer/mesos/launch.cpp 
e22106b014c871e2184a15c2ab154a0674874e47 
  src/slave/flags.hpp 4fa3213545d4bd3525d85c3f71749f00f08dc998 
  src/slave/flags.cpp 6fde51fc61cfcad61d4085c208bd2eca2eae8f14 
  src/tests/container_logger_tests.cpp efadceafca5721bce4dbffadb35f54fd5365abb0 
  src/tests/containerizer/filesystem_isolator_tests.cpp 
29d313051865761306029f331eb36684c3252ffb 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
09742ff21513dc2570684d384b257868dd57a9ce 

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


Testing
---

make check; used mesos cli to test end to end functionality.


Thanks,

Jojy Varghese



Re: Review Request 46799: Added capabilities support to mesos-execute.

2016-04-29 Thread Jojy Varghese

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

(Updated April 29, 2016, 6:41 p.m.)


Review request for mesos and Jie Yu.


Repository: mesos


Description
---

This change introduces two flags: `user` and `capabilities`.

   user:  used to specify the user name for the command.
   capabilities: comma separated list of capabilities the task
 requires.


Diffs (updated)
-

  src/cli/execute.cpp 4711e80314e2fc4cde077bebd9a8167324c3254e 

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


Testing
---

make check.


Thanks,

Jojy Varghese



Re: Review Request 46799: Added capabilities support to mesos-execute.

2016-04-28 Thread Jojy Varghese


> On April 29, 2016, 12:19 a.m., Qian Zhang wrote:
> > src/cli/execute.cpp, lines 172-175
> > <https://reviews.apache.org/r/46799/diff/1/?file=1365019#file1365019line172>
> >
> > Can we only add this flag in Linux? Then user will not see this flag at 
> > all when running `mesos-execute` in other platorms.

I avoided that logic as it would mean spreading `ifdef` all over the file.


- Jojy


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


On April 28, 2016, 9:49 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46799/
> ---
> 
> (Updated April 28, 2016, 9:49 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change introduces two flags: `user` and `capabilities`.
> 
>user:  used to specify the user name for the command.
>capabilities: comma separated list of capabilities the task
>  requires.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp 4711e80314e2fc4cde077bebd9a8167324c3254e 
> 
> Diff: https://reviews.apache.org/r/46799/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>



Review Request 46799: Added capabilities support to mesos-execute.

2016-04-28 Thread Jojy Varghese

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

Review request for mesos and Jie Yu.


Repository: mesos


Description
---

This change introduces two flags: `user` and `capabilities`.

   user:  used to specify the user name for the command.
   capabilities: comma separated list of capabilities the task
 requires.


Diffs
-

  src/cli/execute.cpp 4711e80314e2fc4cde077bebd9a8167324c3254e 

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


Testing
---

make check.


Thanks,

Jojy Varghese



Review Request 46798: Introduced linux capabilities support for mesos containerizer.

2016-04-28 Thread Jojy Varghese

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

Review request for mesos and Jie Yu.


Repository: mesos


Description
---

This change introduces linux capability based security for unified
containerizer. A new agent flag `allowed_capabilities` has been
introduced to override the default capabilities of the user or the
capabilities requested by the user.

This feature is only available on linux.


Diffs
-

  src/launcher/executor.cpp 9f1d2168bc4ddbce1bcd25ff38dc1c34714eb28b 
  src/slave/containerizer/mesos/containerizer.hpp 
13399f014dcd85defbff79f3b5aa4e7e75d41fd1 
  src/slave/containerizer/mesos/containerizer.cpp 
8d538954d6e1f13e833d75c2eaa37e700278ee0c 
  src/slave/containerizer/mesos/launch.cpp 
e22106b014c871e2184a15c2ab154a0674874e47 
  src/slave/flags.hpp 4fa3213545d4bd3525d85c3f71749f00f08dc998 
  src/slave/flags.cpp 6fde51fc61cfcad61d4085c208bd2eca2eae8f14 
  src/tests/container_logger_tests.cpp efadceafca5721bce4dbffadb35f54fd5365abb0 
  src/tests/containerizer/filesystem_isolator_tests.cpp 
29d313051865761306029f331eb36684c3252ffb 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
09742ff21513dc2570684d384b257868dd57a9ce 

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


Testing
---

make check; used mesos cli to test end to end functionality.


Thanks,

Jojy Varghese



Re: Review Request 46370: Introduced linux capabilities API.

2016-04-28 Thread Jojy Varghese

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

(Updated April 28, 2016, 9:44 p.m.)


Review request for mesos, Jie Yu and Kevin Klues.


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


Repository: mesos


Description
---

This change introduces basic API for linux capabilities. This is not a
comprehensive API but is strictly limited to the need for securing Mesos
containers using linux capabilities.


Diffs (updated)
-

  src/CMakeLists.txt f991743f1a467f24a786e925983390a411792327 
  src/Makefile.am 53de98f43629dc94f7619324369caf88407b2f41 
  src/linux/capabilities.hpp PRE-CREATION 
  src/linux/capabilities.cpp PRE-CREATION 

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


Testing
---

make check.


Thanks,

Jojy Varghese



Re: Review Request 46370: Introduced linux capabilities API.

2016-04-25 Thread Jojy Varghese

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

(Updated April 26, 2016, 1:23 a.m.)


Review request for mesos, Jie Yu and Kevin Klues.


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


Repository: mesos


Description
---

This change introduces basic API for linux capabilities. This is not a
comprehensive API but is strictly limited to the need for securing Mesos
containers using linux capabilities.


Diffs (updated)
-

  src/CMakeLists.txt 8e3eecb172e6c6b5256313f0937dc1156e91 
  src/Makefile.am 08d5279bbc0c763255fda3c3360d8f24f41bccda 
  src/linux/capabilities.hpp PRE-CREATION 
  src/linux/capabilities.cpp PRE-CREATION 

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


Testing
---

make check.


Thanks,

Jojy Varghese



Re: Review Request 46182: Adding app runtime processing to appc store and provisioner.

2016-04-21 Thread Jojy Varghese

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




include/mesos/appc/spec.proto (line 62)
<https://reviews.apache.org/r/46182/#comment193475>

Todo comment style is `TODO(name): `. Also we end comments with 
period.



src/slave/containerizer/mesos/provisioner/appc/store.cpp (line 199)
<https://reviews.apache.org/r/46182/#comment193481>

Why this variable?



src/slave/containerizer/mesos/provisioner/appc/store.cpp (line 202)
<https://reviews.apache.org/r/46182/#comment193478>

indentation.



src/slave/containerizer/mesos/provisioner/appc/store.cpp (line 203)
<https://reviews.apache.org/r/46182/#comment193479>

recommend better name for the variable `manifest1`



src/slave/containerizer/mesos/provisioner/appc/store.cpp (line 213)
<https://reviews.apache.org/r/46182/#comment193480>

This could return the default contructed ImageManifest object if none of 
the images's manifest could be found.



src/slave/containerizer/mesos/provisioner/provisioner.cpp (line 307)
<https://reviews.apache.org/r/46182/#comment193476>

Fix indentation



src/slave/containerizer/mesos/provisioner/store.hpp (line 26)
<https://reviews.apache.org/r/46182/#comment193477>

headers should be alphabetically sorted.


- Jojy Varghese


On April 15, 2016, 6:23 p.m., Srinivas Brahmaroutu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46182/
> ---
> 
> (Updated April 15, 2016, 6:23 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adding app runtime processing to appc store and provisioner.
> 
> 
> Diffs
> -
> 
>   include/mesos/appc/spec.proto 4ef0473748c2f47aff1729c1d969185803fe72d4 
>   include/mesos/slave/isolator.proto 60a9bb637e12593a97ed1a7c510ebccd4e5a9615 
>   src/slave/containerizer/mesos/provisioner/appc/store.cpp 
> 33e97fc645a9b7fbc9ae47f67c1b5dacf999fce0 
>   src/slave/containerizer/mesos/provisioner/provisioner.hpp 
> 5c96e9f6603d39889e6bc807874d35d0cb3556be 
>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 
> dcbbbaf797d4467bfd0bb1ee91ee9ce843e7d546 
>   src/slave/containerizer/mesos/provisioner/store.hpp 
> 1d477ef13ddd24fd8badae0decaa4a2271ecc746 
> 
> Diff: https://reviews.apache.org/r/46182/diff/
> 
> 
> Testing
> ---
> 
> make check on linux passes 
> AppcProvisionerIntegrationTest.ROOT_SimpleLinuxImageTest
> 
> 
> Thanks,
> 
> Srinivas Brahmaroutu
> 
>



Re: Review Request 46369: Added capabilities support in ContanerInfo protobuf.

2016-04-21 Thread Jojy Varghese

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

(Updated April 21, 2016, 4:58 p.m.)


Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Added capabilities support in ContanerInfo protobuf.


Diffs (updated)
-

  include/mesos/mesos.proto 87af4a06db8cc3889fe4d3b314206103f5ce5f2d 
  include/mesos/v1/mesos.proto 34da0a1484dc2f71262d8b97484b8edaae35bb6c 

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


Testing
---

make check.


Thanks,

Jojy Varghese



Re: Review Request 46370: Introduced linux capabilities API.

2016-04-20 Thread Jojy Varghese


> On April 20, 2016, 3:15 a.m., Kevin Klues wrote:
> > I added a few comments below, but in general, I feel like there are places 
> > this code could be greatly simplified.  Specifically, it's not obvious to 
> > me why we need all of the different classes you define (or maybe more about 
> > why they need to be part of the public interface).  Could you clarify the 
> > rational here a bit?

Classes like `CapabilityFlags` allows good encapsulation of the bitmap and 
makes the API more fiendly to deal with. Leaving them as bitmap would leave the 
complexity to the caller.

Same idea with other classes.


- Jojy


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


On April 20, 2016, 8:19 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46370/
> ---
> 
> (Updated April 20, 2016, 8:19 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Kevin Klues.
> 
> 
> Bugs: MESOS-5051
> https://issues.apache.org/jira/browse/MESOS-5051
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change introduces basic API for linux capabilities. This is not a
> comprehensive API but is strictly limited to the need for securing Mesos
> containers using linux capabilities.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 8e3eecb172e6c6b5256313f0937dc1156e91 
>   src/Makefile.am 08d5279bbc0c763255fda3c3360d8f24f41bccda 
>   src/linux/capabilities.hpp PRE-CREATION 
>   src/linux/capabilities.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46370/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>



Re: Review Request 46370: Introduced linux capabilities API.

2016-04-20 Thread Jojy Varghese


> On April 20, 2016, 3:15 a.m., Kevin Klues wrote:
> > src/linux/capabilities.cpp, lines 124-125
> > <https://reviews.apache.org/r/46370/diff/1/?file=1350685#file1350685line124>
> >
> > This should be unnecessary. See: 
> > https://github.com/klueska-mesosphere/mesos/blob/master/src/linux/cgroups.cpp#L2435

Not sure about this as  I like to lean towards good programming practice than 
depending on compiler flags.


- Jojy


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


On April 20, 2016, 8:19 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46370/
> ---
> 
> (Updated April 20, 2016, 8:19 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Kevin Klues.
> 
> 
> Bugs: MESOS-5051
> https://issues.apache.org/jira/browse/MESOS-5051
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change introduces basic API for linux capabilities. This is not a
> comprehensive API but is strictly limited to the need for securing Mesos
> containers using linux capabilities.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 8e3eecb172e6c6b5256313f0937dc1156e91 
>   src/Makefile.am 08d5279bbc0c763255fda3c3360d8f24f41bccda 
>   src/linux/capabilities.hpp PRE-CREATION 
>   src/linux/capabilities.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46370/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>



Re: Review Request 46370: Introduced linux capabilities API.

2016-04-20 Thread Jojy Varghese

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

(Updated April 20, 2016, 8:19 p.m.)


Review request for mesos, Jie Yu and Kevin Klues.


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


Repository: mesos


Description
---

This change introduces basic API for linux capabilities. This is not a
comprehensive API but is strictly limited to the need for securing Mesos
containers using linux capabilities.


Diffs (updated)
-

  src/CMakeLists.txt 8e3eecb172e6c6b5256313f0937dc1156e91 
  src/Makefile.am 08d5279bbc0c763255fda3c3360d8f24f41bccda 
  src/linux/capabilities.hpp PRE-CREATION 
  src/linux/capabilities.cpp PRE-CREATION 

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


Testing
---

make check.


Thanks,

Jojy Varghese



Re: Review Request 46371: Added basic tests for capabilities API.

2016-04-20 Thread Jojy Varghese

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

(Updated April 20, 2016, 7:12 p.m.)


Review request for mesos and Jie Yu.


Repository: mesos


Description
---

Added basic tests for capabilities API.


Diffs (updated)
-

  src/Makefile.am 08d5279bbc0c763255fda3c3360d8f24f41bccda 
  src/tests/capabilities_test_helper.hpp PRE-CREATION 
  src/tests/capabilities_test_helper.cpp PRE-CREATION 
  src/tests/capabilities_test_helper_main.cpp PRE-CREATION 
  src/tests/capabilities_tests.cpp PRE-CREATION 

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


Testing
---

make check.


Thanks,

Jojy Varghese



Re: Review Request 46370: Introduced linux capabilities API.

2016-04-20 Thread Jojy Varghese

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

(Updated April 20, 2016, 7:05 p.m.)


Review request for mesos, Jie Yu and Kevin Klues.


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


Repository: mesos


Description
---

This change introduces basic API for linux capabilities. This is not a
comprehensive API but is strictly limited to the need for securing Mesos
containers using linux capabilities.


Diffs (updated)
-

  src/CMakeLists.txt 8e3eecb172e6c6b5256313f0937dc1156e91 
  src/Makefile.am 08d5279bbc0c763255fda3c3360d8f24f41bccda 
  src/linux/capabilities.hpp PRE-CREATION 
  src/linux/capabilities.cpp PRE-CREATION 

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


Testing
---

make check.


Thanks,

Jojy Varghese



Re: Review Request 46370: Introduced linux capabilities API.

2016-04-20 Thread Jojy Varghese


> On April 20, 2016, 3:15 a.m., Kevin Klues wrote:
> > src/linux/capabilities.hpp, lines 94-99
> > <https://reviews.apache.org/r/46370/diff/1/?file=1350684#file1350684line94>
> >
> > From my reading of: 
> > http://man7.org/linux/man-pages/man7/capabilities.7.html
> > 
> > this enum should probably be called `Set`.
> > 
> > Note, the name `Capability` at the front is unnecessary if we embed 
> > this in the `capabilities` namespace.
> > 
> > Also, it's pretty standard practice in C++ to define an `enum` as a 
> > `enum class` for better type checking.  As such, you can define the final 
> > element with a common name of `COUNT` to get at the size of the enum.
> > 
> > For example, you can get at the size of the enum as: 
> > `capabilities::Set::COUNT` instead of relying on the `const` for 
> > `NUMBER_OF_CAP_SETS` defined above.
> 
> Jojy Varghese wrote:
> Although i agree that C++11 supports enum classes, couple of reasons for 
> using plain enum here:
> 1. Other places in the code mostly use plain old enums.
> 2. Its hard(not impossible) to get the value of the enum class's element 
> (say for printing).

regarding naming, If you look at the code of libcap (a standard portable 
capability interface) 
(https://github.com/abstrakraft/lxc-android-libcap/blob/master/libcap/libcap.h),
 the name `set` is referenced for the value and not the type. In fact the 
`SystemCallPayload` structure in my code is same from libcap.


- Jojy


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


On April 19, 2016, 5:02 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46370/
> ---
> 
> (Updated April 19, 2016, 5:02 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Kevin Klues.
> 
> 
> Bugs: MESOS-5051
> https://issues.apache.org/jira/browse/MESOS-5051
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change introduces basic API for linux capabilities. This is not a
> comprehensive API but is strictly limited to the need for securing Mesos
> containers using linux capabilities.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt ca59a1824352423f4db6ef8bb41acc6fe602c041 
>   src/Makefile.am a8f68316c5d4d4c82d99363535a97fa5b1caafc5 
>   src/linux/capabilities.hpp PRE-CREATION 
>   src/linux/capabilities.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46370/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>



Re: Review Request 46370: Introduced linux capabilities API.

2016-04-20 Thread Jojy Varghese


> On April 20, 2016, 3:15 a.m., Kevin Klues wrote:
> > src/linux/capabilities.hpp, lines 32-33
> > <https://reviews.apache.org/r/46370/diff/1/?file=1350684#file1350684line32>
> >
> > This should all probably live in the mesos::internal::capabilities 
> > namespace.

The reasons why I chose to have it in mesos::internal is that we have 
Capabilities class (explained in the class's documentation why we need a class).


> On April 20, 2016, 3:15 a.m., Kevin Klues wrote:
> > src/linux/capabilities.hpp, lines 94-99
> > <https://reviews.apache.org/r/46370/diff/1/?file=1350684#file1350684line94>
> >
> > From my reading of: 
> > http://man7.org/linux/man-pages/man7/capabilities.7.html
> > 
> > this enum should probably be called `Set`.
> > 
> > Note, the name `Capability` at the front is unnecessary if we embed 
> > this in the `capabilities` namespace.
> > 
> > Also, it's pretty standard practice in C++ to define an `enum` as a 
> > `enum class` for better type checking.  As such, you can define the final 
> > element with a common name of `COUNT` to get at the size of the enum.
> > 
> > For example, you can get at the size of the enum as: 
> > `capabilities::Set::COUNT` instead of relying on the `const` for 
> > `NUMBER_OF_CAP_SETS` defined above.

Although i agree that C++11 supports enum classes, couple of reasons for using 
plain enum here:
1. Other places in the code mostly use plain old enums.
2. Its hard(not impossible) to get the value of the enum class's element (say 
for printing).


> On April 20, 2016, 3:15 a.m., Kevin Klues wrote:
> > src/linux/capabilities.hpp, line 178
> > <https://reviews.apache.org/r/46370/diff/1/?file=1350684#file1350684line178>
> >
> > Didn't we discuss not making this a class, and only having get()/set() 
> > calls as part of the namespace?

Explained in the class's documentation.


> On April 20, 2016, 3:15 a.m., Kevin Klues wrote:
> > src/linux/capabilities.hpp, line 209
> > <https://reviews.apache.org/r/46370/diff/1/?file=1350684#file1350684line209>
> >
> > What did we decide about the `add()` pairing to this `drop()` call?

As mentioned in the documentation of `drop`, the `drop` API is for dropping 
`bounding` capabilities.


> On April 20, 2016, 3:15 a.m., Kevin Klues wrote:
> > src/linux/capabilities.cpp, lines 36-38
> > <https://reviews.apache.org/r/46370/diff/1/?file=1350685#file1350685line36>
> >
> > Is there not a header file you can just include here?

No the standard header files dont provide the syscall declaration.


> On April 20, 2016, 3:15 a.m., Kevin Klues wrote:
> > src/linux/capabilities.hpp, lines 50-90
> > <https://reviews.apache.org/r/46370/diff/1/?file=1350684#file1350684line50>
> >
> > Since we should probably be embedding this in a `capabilities` 
> > namespace, it is redundant to call this enum `Capability`. I'd sugggest 
> > `Privilege`.  That way one of these can be accessed as e.g. 
> > `capabiliites::Privilege::SETGID`.
> > 
> > Also, as mentioned in a comment below, this should probably be declared 
> > as an `enum class` for better type checking.
> > 
> > The `COUNT` trick mentioned below should probably be applied here as 
> > well.

I like Capability because that is what its referenced as in every documentation 
and literature.


- Jojy


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


On April 19, 2016, 5:02 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46370/
> ---
> 
> (Updated April 19, 2016, 5:02 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Kevin Klues.
> 
> 
> Bugs: MESOS-5051
> https://issues.apache.org/jira/browse/MESOS-5051
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change introduces basic API for linux capabilities. This is not a
> comprehensive API but is strictly limited to the need for securing Mesos
> containers using linux capabilities.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt ca59a1824352423f4db6ef8bb41acc6fe602c041 
>   src/Makefile.am a8f68316c5d4d4c82d99363535a97fa5b1caafc5 
>   src/linux/capabilities.hpp PRE-CREATION 
>   src/linux/capabilities.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46370/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>



Review Request 46371: Added basic tests for capabilities API.

2016-04-19 Thread Jojy Varghese

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

Review request for mesos and Jie Yu.


Repository: mesos


Description
---

Added basic tests for capabilities API.


Diffs
-

  src/Makefile.am a8f68316c5d4d4c82d99363535a97fa5b1caafc5 
  src/tests/capabilities_tests.cpp PRE-CREATION 

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


Testing
---

make check.


Thanks,

Jojy Varghese



Review Request 46370: Introduced linux capabilities API.

2016-04-19 Thread Jojy Varghese

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

Review request for mesos, Jie Yu and Kevin Klues.


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


Repository: mesos


Description
---

This change introduces basic API for linux capabilities. This is not a
comprehensive API but is strictly limited to the need for securing Mesos
containers using linux capabilities.


Diffs
-

  src/CMakeLists.txt ca59a1824352423f4db6ef8bb41acc6fe602c041 
  src/Makefile.am a8f68316c5d4d4c82d99363535a97fa5b1caafc5 
  src/linux/capabilities.hpp PRE-CREATION 
  src/linux/capabilities.cpp PRE-CREATION 

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


Testing
---

make check.


Thanks,

Jojy Varghese



Review Request 46369: Added capabilities support in ContanerInfo protobuf.

2016-04-19 Thread Jojy Varghese

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

Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Added capabilities support in ContanerInfo protobuf.


Diffs
-

  include/mesos/mesos.proto 63c181ae0a1e350fc27e36b1698e02db100b8861 

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


Testing
---

make check.


Thanks,

Jojy Varghese



Re: Review Request 46044: Cleaned up c-tor signature in mesos-execute.

2016-04-11 Thread Jojy Varghese


> On April 11, 2016, 9:53 p.m., Jojy Varghese wrote:
> > src/cli/execute.cpp, line 196
> > <https://reviews.apache.org/r/46044/diff/1/?file=1339684#file1339684line196>
> >
> > I like the idea of simplifying the ctor. I am not too excited about the 
> > idea of moving everything to 'flag'. A `CommandScheduler` object should 
> > have some properties like `command`, `master`, `name`. Others like 'image' 
> > information should be moved to its own class/struct (say `ContainerInfo`). 
> > Just my 2 cents.
> 
> Joseph Wu wrote:
> I partially agree, but in this case, the `Flags` class is effectively the 
> class/struct you're asking for.  It just happens to have everything packed 
> along with it :)

hmm i could extend that argument to get away with any type system or class 
design and replace all types with a `flag` type. Good design would allow 
translation between a user input (flag) to a CommandScheduler object. Passing a 
user input to CommandScheduler does not seem right. I think this was the reason 
the original author wanted input validation to happen before the object was 
created. CommandScheduler should only accept a strongly typed input in its 
ctor. I exceeded my 2 cents :|


- Jojy


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


On April 11, 2016, 7:01 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46044/
> ---
> 
> (Updated April 11, 2016, 7:01 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Pass complete `flags` instance rather than each flag value separately
> to `CommandScheduler` in mesos-execute for brevity.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp 763dd26c359d1dd92c6e0365e4808b673efb1f40 
> 
> Diff: https://reviews.apache.org/r/46044/diff/
> 
> 
> Testing
> ---
> 
> On Mac OS 10.10.4:
> make check
> 
> Additionally manually tested mesos-execute with both responsive and 
> unresponsive (https://github.com/rukletsov/unresponsive-process) tasks:
> ./src/mesos-execute --master=127.0.0.1:5050 --name=test --command="sleep 10" 
> --env='{"GLOG_v": "2"}' --kill_after=2secs
> ./src/mesos-execute --master=127.0.0.1:5050 --name=test 
> --command="/Users/alex/bin/unresponsive_process" --env='{"GLOG_v": "2"}' 
> --kill_after=2secs
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>



Re: Review Request 46044: Cleaned up c-tor signature in mesos-execute.

2016-04-11 Thread Jojy Varghese

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




src/cli/execute.cpp 
<https://reviews.apache.org/r/46044/#comment191684>

I like the idea of simplifying the ctor. I am not too excited about the 
idea of moving everything to 'flag'. A `CommandScheduler` object should have 
some properties like `command`, `master`, `name`. Others like 'image' 
information should be moved to its own class/struct (say `ContainerInfo`). Just 
my 2 cents.


- Jojy Varghese


On April 11, 2016, 7:01 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46044/
> ---
> 
> (Updated April 11, 2016, 7:01 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Pass complete `flags` instance rather than each flag value separately
> to `CommandScheduler` in mesos-execute for brevity.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp 763dd26c359d1dd92c6e0365e4808b673efb1f40 
> 
> Diff: https://reviews.apache.org/r/46044/diff/
> 
> 
> Testing
> ---
> 
> On Mac OS 10.10.4:
> make check
> 
> Additionally manually tested mesos-execute with both responsive and 
> unresponsive (https://github.com/rukletsov/unresponsive-process) tasks:
> ./src/mesos-execute --master=127.0.0.1:5050 --name=test --command="sleep 10" 
> --env='{"GLOG_v": "2"}' --kill_after=2secs
> ./src/mesos-execute --master=127.0.0.1:5050 --name=test 
> --command="/Users/alex/bin/unresponsive_process" --env='{"GLOG_v": "2"}' 
> --kill_after=2secs
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>



Re: Review Request 44934: Updated mesos-execute to add support for Appc.

2016-04-01 Thread Jojy Varghese

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

(Updated April 1, 2016, 8:05 p.m.)


Review request for mesos and Jie Yu.


Changes
---

rebased.


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


Repository: mesos


Description
---

Updated mesos-execute to add support for Appc.


Diffs (updated)
-

  src/cli/execute.cpp af62f41622e1c43acd8f257c54f8754162c433b8 

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


Testing
---

Tested with various Appc images.


Thanks,

Jojy Varghese



Re: Review Request 45002: Added FTS_PHYSICAL option to fts_open for rmdir.

2016-03-23 Thread Jojy Varghese

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

(Updated March 24, 2016, 1:34 a.m.)


Review request for mesos, Jie Yu and Neil Conway.


Changes
---

review addressed.


Repository: mesos


Description
---

According to the documentation for fts_open, either FTS_PHYSICAL or FTS_LOGICAL
SHOULD be provided. We need FTS_PHYSICAL for the case of rmdir as we dont want
to resolve the symlink targets while deleting them.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/rmdir.hpp 
cbc97596cd8ed1e6d4261568fd0086c86e063232 
  3rdparty/libprocess/3rdparty/stout/tests/os/rmdir_tests.cpp 
5bd154e40c6b45e74967b620141cc373bdee 

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


Testing
---

make check.


Thanks,

Jojy Varghese



Re: Review Request 45003: Fixed rmdir comment for FTS_SLNONE as per coding guidelines.

2016-03-23 Thread Jojy Varghese


> On March 20, 2016, 2:33 a.m., Neil Conway wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/rmdir.hpp, line 71
> > <https://reviews.apache.org/r/45003/diff/1/?file=1304569#file1304569line71>
> >
> > This comment seems incorrect:
> > 
> > (1) The relevant parameters are `FTS_PHYSICAL` vs. `FTS_LOGICAL`, as 
> > well as `FTS_COMFOLLOW`.
> > 
> > (2) Since we _do_ set `FTS_PHYSICAL`, isn't it possible for 
> > `FTS_SLNONE` to be returned?
> 
> Jojy Varghese wrote:
> I believe FTS_LOGICAL or FTS_COMFOLLOW needs to be set in order for 
> FTS_SLNONE to be returned.
> 
> Jie Yu wrote:
> Can you do a simple test to validate that (see if FTS_SLNONE is hit or 
> not)? My understanding is that FTS_SLNONE will still be hit if we're using 
> FTS_PHYSICAL.
> 
> Cong Wang wrote:
> Looking at 
> https://sourceware.org/git/?p=glibc.git;a=blob;f=io/fts.c;h=0c5abfcdd660871d876751fba351ab25b921e88a;hb=HEAD#l901
>  I think Jojy is correct here, I don't see FTS_SLNONE could be set without 
> FTS_LOGICAL or FTS_COMFOLLOW.
> 
> Jojy Varghese wrote:
> @jie. I tested it already and verified that FTS_SLNONE  is not hit (using 
> logs).
> 
> Neil Conway wrote:
> Can we add a unit test anyway, please? This is corner-case behavior that 
> we should be validating we handle correctly, regardless of whether 
> `FTS_SLNONE` is actually set.
> 
> Jojy Varghese wrote:
> I have added unit tests in patch https://reviews.apache.org/r/45002/. 
> Would love to hear more test ideas.
> 
> Neil Conway wrote:
> I'd like a test case that covers `rmdir` of a symbolic link that points 
> to a non-existent file/directory; as far as I can see, the existing tests do 
> not cover this case.

What do you think about the test case `RemoveDirectoryWithNoTargetSymbolicLink` 
in the test file?


- Jojy


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


On March 24, 2016, 1:10 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45003/
> ---
> 
> (Updated March 24, 2016, 1:10 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed rmdir comment for FTS_SLNONE as per coding guidelines.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/rmdir.hpp 
> cbc97596cd8ed1e6d4261568fd0086c86e063232 
> 
> Diff: https://reviews.apache.org/r/45003/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>



Re: Review Request 45003: Fixed rmdir comment for FTS_SLNONE as per coding guidelines.

2016-03-23 Thread Jojy Varghese


> On March 20, 2016, 2:33 a.m., Neil Conway wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/rmdir.hpp, line 71
> > <https://reviews.apache.org/r/45003/diff/1/?file=1304569#file1304569line71>
> >
> > This comment seems incorrect:
> > 
> > (1) The relevant parameters are `FTS_PHYSICAL` vs. `FTS_LOGICAL`, as 
> > well as `FTS_COMFOLLOW`.
> > 
> > (2) Since we _do_ set `FTS_PHYSICAL`, isn't it possible for 
> > `FTS_SLNONE` to be returned?
> 
> Jojy Varghese wrote:
> I believe FTS_LOGICAL or FTS_COMFOLLOW needs to be set in order for 
> FTS_SLNONE to be returned.
> 
> Jie Yu wrote:
> Can you do a simple test to validate that (see if FTS_SLNONE is hit or 
> not)? My understanding is that FTS_SLNONE will still be hit if we're using 
> FTS_PHYSICAL.
> 
> Cong Wang wrote:
> Looking at 
> https://sourceware.org/git/?p=glibc.git;a=blob;f=io/fts.c;h=0c5abfcdd660871d876751fba351ab25b921e88a;hb=HEAD#l901
>  I think Jojy is correct here, I don't see FTS_SLNONE could be set without 
> FTS_LOGICAL or FTS_COMFOLLOW.
> 
> Jojy Varghese wrote:
> @jie. I tested it already and verified that FTS_SLNONE  is not hit (using 
> logs).
> 
> Neil Conway wrote:
> Can we add a unit test anyway, please? This is corner-case behavior that 
> we should be validating we handle correctly, regardless of whether 
> `FTS_SLNONE` is actually set.

I have added unit tests in patch https://reviews.apache.org/r/45002/. Would 
love to hear more test ideas.


- Jojy


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


On March 24, 2016, 1:10 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45003/
> ---
> 
> (Updated March 24, 2016, 1:10 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed rmdir comment for FTS_SLNONE as per coding guidelines.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/rmdir.hpp 
> cbc97596cd8ed1e6d4261568fd0086c86e063232 
> 
> Diff: https://reviews.apache.org/r/45003/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>



Re: Review Request 45003: Fixed rmdir comment for FTS_SLNONE as per coding guidelines.

2016-03-23 Thread Jojy Varghese

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

(Updated March 24, 2016, 1:10 a.m.)


Review request for mesos, Jie Yu and Neil Conway.


Changes
---

review addressed.


Repository: mesos


Description
---

Fixed rmdir comment for FTS_SLNONE as per coding guidelines.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/rmdir.hpp 
cbc97596cd8ed1e6d4261568fd0086c86e063232 

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


Testing
---

make check.


Thanks,

Jojy Varghese



Re: Review Request 45002: Added FTS_PHYSICAL option to fts_open for rmdir.

2016-03-23 Thread Jojy Varghese

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

(Updated March 24, 2016, 1:07 a.m.)


Review request for mesos, Jie Yu and Neil Conway.


Changes
---

review addressed.


Repository: mesos


Description
---

According to the documentation for fts_open, either FTS_PHYSICAL or FTS_LOGICAL
SHOULD be provided. We need FTS_PHYSICAL for the case of rmdir as we dont want
to resolve the symlink targets while deleting them.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/rmdir.hpp 
cbc97596cd8ed1e6d4261568fd0086c86e063232 
  3rdparty/libprocess/3rdparty/stout/tests/os/rmdir_tests.cpp 
5bd154e40c6b45e74967b620141cc373bdee 
  src/tests/containerizer/provisioner_appc_tests.cpp 
23a5b1059b4d9fde1e4a1aab5cd4fa6d05862332 

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


Testing
---

make check.


Thanks,

Jojy Varghese



Review Request 45259: Added Appc provisioner integration test.

2016-03-23 Thread Jojy Varghese

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

Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Added Appc provisioner integration test.


Diffs
-

  src/tests/containerizer/provisioner_appc_tests.cpp 
23a5b1059b4d9fde1e4a1aab5cd4fa6d05862332 

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


Testing
---

make check.


Thanks,

Jojy Varghese



Re: Review Request 45003: Fixed rmdir comment for FTS_SLNONE as per coding guidelines.

2016-03-23 Thread Jojy Varghese


> On March 20, 2016, 2:33 a.m., Neil Conway wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/rmdir.hpp, line 71
> > <https://reviews.apache.org/r/45003/diff/1/?file=1304569#file1304569line71>
> >
> > This comment seems incorrect:
> > 
> > (1) The relevant parameters are `FTS_PHYSICAL` vs. `FTS_LOGICAL`, as 
> > well as `FTS_COMFOLLOW`.
> > 
> > (2) Since we _do_ set `FTS_PHYSICAL`, isn't it possible for 
> > `FTS_SLNONE` to be returned?
> 
> Jojy Varghese wrote:
> I believe FTS_LOGICAL or FTS_COMFOLLOW needs to be set in order for 
> FTS_SLNONE to be returned.
> 
> Jie Yu wrote:
> Can you do a simple test to validate that (see if FTS_SLNONE is hit or 
> not)? My understanding is that FTS_SLNONE will still be hit if we're using 
> FTS_PHYSICAL.
> 
> Cong Wang wrote:
> Looking at 
> https://sourceware.org/git/?p=glibc.git;a=blob;f=io/fts.c;h=0c5abfcdd660871d876751fba351ab25b921e88a;hb=HEAD#l901
>  I think Jojy is correct here, I don't see FTS_SLNONE could be set without 
> FTS_LOGICAL or FTS_COMFOLLOW.

@jie. I tested it already and verified that FTS_SLNONE  is not hit (using logs).


- Jojy


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


On March 18, 2016, 12:17 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45003/
> ---
> 
> (Updated March 18, 2016, 12:17 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed rmdir comment for FTS_SLNONE as per coding guidelines.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/rmdir.hpp 
> cbc97596cd8ed1e6d4261568fd0086c86e063232 
> 
> Diff: https://reviews.apache.org/r/45003/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>



Re: Review Request 44934: Updated mesos-execute to add support for Appc.

2016-03-22 Thread Jojy Varghese


> On March 22, 2016, 5:10 p.m., Tom Runyon wrote:
> > It would be helpful to update docs/container-image.md to include example 
> > appc deployments using mesos-execute.

Created MESOS-5006.


- Jojy


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


On March 20, 2016, 4:19 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44934/
> ---
> 
> (Updated March 20, 2016, 4:19 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4978
> https://issues.apache.org/jira/browse/MESOS-4978
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated mesos-execute to add support for Appc.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp ed42cb568f3d16856f48b3bbd354cb2b0fb83e8e 
> 
> Diff: https://reviews.apache.org/r/44934/diff/
> 
> 
> Testing
> ---
> 
> Tested with various Appc images.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>



Review Request 44934: Updated mesos-execute to add support for Appc.

2016-03-20 Thread Jojy Varghese

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

Review request for mesos and Jie Yu.


Repository: mesos


Description
---

Updated mesos-execute to add support for Appc.


Diffs
-

  src/cli/execute.cpp ed42cb568f3d16856f48b3bbd354cb2b0fb83e8e 

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


Testing
---

Tested with various Appc images.


Thanks,

Jojy Varghese



Review Request 45002: Added FTS_PHYSICAL option to fts_open for rmdir.

2016-03-20 Thread Jojy Varghese

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

Review request for mesos, Jie Yu and Neil Conway.


Repository: mesos


Description
---

According to the documentation for fts_open, either FTS_PHYSICAL or FTS_LOGICAL
SHOULD be provided. We need FTS_PHYSICAL for the case of rmdir as we dont want
to resolve the symlink targets while deleting them.


Diffs
-

  3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/rmdir.hpp 
cbc97596cd8ed1e6d4261568fd0086c86e063232 
  3rdparty/libprocess/3rdparty/stout/tests/os/rmdir_tests.cpp 
daa46e05d113fd62ea9dad042ec41aaab28ad003 

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


Testing
---

make check.


Thanks,

Jojy Varghese



Re: Review Request 44934: Updated mesos-execute to add support for Appc.

2016-03-20 Thread Jojy Varghese

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

(Updated March 18, 2016, 5:26 p.m.)


Review request for mesos and Jie Yu.


Repository: mesos


Description
---

Updated mesos-execute to add support for Appc.


Diffs (updated)
-

  src/cli/execute.cpp ed42cb568f3d16856f48b3bbd354cb2b0fb83e8e 

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


Testing
---

Tested with various Appc images.


Thanks,

Jojy Varghese



Re: Review Request 44934: Updated mesos-execute to add support for Appc.

2016-03-19 Thread Jojy Varghese

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

(Updated March 20, 2016, 4:19 a.m.)


Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Updated mesos-execute to add support for Appc.


Diffs (updated)
-

  src/cli/execute.cpp ed42cb568f3d16856f48b3bbd354cb2b0fb83e8e 

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


Testing
---

Tested with various Appc images.


Thanks,

Jojy Varghese



Re: Review Request 45002: Added FTS_PHYSICAL option to fts_open for rmdir.

2016-03-19 Thread Jojy Varghese


> On March 20, 2016, 2:36 a.m., Neil Conway wrote:
> > Per discussion in https://reviews.apache.org/r/45003/, if we can actually 
> > run into `FTS_SLNONE`, we should have a test-case for it.

I dont think with FTS_PHYSICAL (and without FTW_COMFOLLOW) we will get 
FTS_SLNONE. I could be wrong but having a peek inside the glibc code for 
fts_open, I believe thats the case.


- Jojy


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


On March 18, 2016, 12:16 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45002/
> ---
> 
> (Updated March 18, 2016, 12:16 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> According to the documentation for fts_open, either FTS_PHYSICAL or 
> FTS_LOGICAL
> SHOULD be provided. We need FTS_PHYSICAL for the case of rmdir as we dont want
> to resolve the symlink targets while deleting them.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/rmdir.hpp 
> cbc97596cd8ed1e6d4261568fd0086c86e063232 
>   3rdparty/libprocess/3rdparty/stout/tests/os/rmdir_tests.cpp 
> daa46e05d113fd62ea9dad042ec41aaab28ad003 
> 
> Diff: https://reviews.apache.org/r/45002/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>



Re: Review Request 45003: Fixed rmdir comment for FTS_SLNONE as per coding guidelines.

2016-03-19 Thread Jojy Varghese


> On March 20, 2016, 2:33 a.m., Neil Conway wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/rmdir.hpp, line 71
> > <https://reviews.apache.org/r/45003/diff/1/?file=1304569#file1304569line71>
> >
> > This comment seems incorrect:
> > 
> > (1) The relevant parameters are `FTS_PHYSICAL` vs. `FTS_LOGICAL`, as 
> > well as `FTS_COMFOLLOW`.
> > 
> > (2) Since we _do_ set `FTS_PHYSICAL`, isn't it possible for 
> > `FTS_SLNONE` to be returned?

I believe FTS_LOGICAL or FTS_COMFOLLOW needs to be set in order for FTS_SLNONE 
to be returned.


- Jojy


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


On March 18, 2016, 12:17 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45003/
> ---
> 
> (Updated March 18, 2016, 12:17 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed rmdir comment for FTS_SLNONE as per coding guidelines.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/rmdir.hpp 
> cbc97596cd8ed1e6d4261568fd0086c86e063232 
> 
> Diff: https://reviews.apache.org/r/45003/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>



Review Request 45010: Fixed newlines for include directives.

2016-03-19 Thread Jojy Varghese

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

Review request for mesos, Jie Yu and Neil Conway.


Repository: mesos


Description
---

Fixed newlines for include directives.


Diffs
-

  3rdparty/libprocess/3rdparty/stout/tests/os/rmdir_tests.cpp 
daa46e05d113fd62ea9dad042ec41aaab28ad003 

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


Testing
---

make check.


Thanks,

Jojy Varghese



Review Request 45003: Fixed rmdir comment for FTS_SLNONE as per coding guidelines.

2016-03-19 Thread Jojy Varghese

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

Review request for mesos, Jie Yu and Neil Conway.


Repository: mesos


Description
---

Fixed rmdir comment for FTS_SLNONE as per coding guidelines.


Diffs
-

  3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/rmdir.hpp 
cbc97596cd8ed1e6d4261568fd0086c86e063232 

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


Testing
---

make check.


Thanks,

Jojy Varghese



Re: Review Request 44934: Updated mesos-execute to add support for Appc.

2016-03-19 Thread Jojy Varghese

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

(Updated March 18, 2016, 5:39 p.m.)


Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Updated mesos-execute to add support for Appc.


Diffs
-

  src/cli/execute.cpp ed42cb568f3d16856f48b3bbd354cb2b0fb83e8e 

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


Testing
---

Tested with various Appc images.


Thanks,

Jojy Varghese



Re: Review Request 44755: Added getAppcImage for Appc provisioning tests.

2016-03-19 Thread Jojy Varghese

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

(Updated March 17, 2016, 4:44 p.m.)


Review request for mesos and Jie Yu.


Repository: mesos


Description
---

Added helper function to get an Appc image protobuf object with given
parameters.


Diffs (updated)
-

  src/tests/containerizer/provisioner_appc_tests.cpp 
069af8a0b5e031d9032fd13154cee250f279b404 

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


Testing
---

make check.


Thanks,

Jojy Varghese



Re: Review Request 44934: Updated mesos-execute to add support for Appc.

2016-03-19 Thread Jojy Varghese


> On March 17, 2016, 7:48 a.m., Guangya Liu wrote:
> > src/cli/execute.cpp, line 241
> > <https://reviews.apache.org/r/44934/diff/2/?file=1302214#file1302214line241>
> >
> > The default value of `containerizer` is `mesos`, do we need to check 
> > this?
> > 
> > What about make the logic as this? This will involve less code change 
> > and the logic may be more clear?
> > 
> > // Do not touch old dockerImage logic.
> > if (dockerImage.isSome()) {
> >   old logic here;
> > }
> > 
> > if (appcImage.isSome() && containerizer == "mesos") {
> >   ContainerInfo containerInfo;
> >   containerInfo.set_type(ContainerInfo::MESOS);
> >   ContainerInfo::MesosInfo mesosInfo;
> > 
> >   Image::Appc appc;
> > 
> >   appc.set_name(appcImage.get());
> > 
> >   // TODO(jojy): Labels are hard coded right now.
> >   // Consider adding label flags for customization.
> >   Label arch;
> >   arch.set_key("arch");
> >   arch.set_value("amd64");
> > 
> >   Label os;
> >   os.set_key("os");
> >   os.set_value("linux");
> > 
> >   Labels labels;
> >   labels.add_labels()->CopyFrom(os);
> >   labels.add_labels()->CopyFrom(arch);
> > 
> >   appc.mutable_labels()->CopyFrom(labels);
> > 
> >   Image mesosImage;
> >   
> >   mesosImage.set_type(Image::APPC);
> >   mesosImage.mutable_appc()->CopyFrom(appc);
> >   
> >   mesosInfo.mutable_image()->CopyFrom(mesosImage);
> >   
> >   containerInfo.mutable_mesos()->CopyFrom(mesosInfo);
> > 
> >   task.mutable_container()->CopyFrom(containerInfo);
> > }
> 
> Jojy Varghese wrote:
> Then we will have to repeat the `if (containerizer == "mesos")` for both 
> dockerImage case and appcImage case. By seperating the logic by cotainerizer, 
> we now can add more image types into the "mesos" containerizer block (say OCI 
> image).
> 
> Guangya Liu wrote:
> Yes, that's also my concern when I open this issue, I think it is ok to 
> keep your current logic.
> 
> The logic of checking `containerizer.empty()` should be removed?

We could remove that but the idea is that the block can be cleanly separated 
out now to a method (say `handleContainerizer`) that handles the containerizers.


- Jojy


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


On March 17, 2016, 6:54 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44934/
> -------
> 
> (Updated March 17, 2016, 6:54 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated mesos-execute to add support for Appc.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp ed42cb568f3d16856f48b3bbd354cb2b0fb83e8e 
> 
> Diff: https://reviews.apache.org/r/44934/diff/
> 
> 
> Testing
> ---
> 
> Tested with various Appc images.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>



Review Request 45001: Replaced const ref to temporary in tests for consistency.

2016-03-19 Thread Jojy Varghese

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

Review request for mesos, Jie Yu and Neil Conway.


Repository: mesos


Description
---

Replaced const ref to temporary in tests for consistency.


Diffs
-

  3rdparty/libprocess/3rdparty/stout/tests/os/rmdir_tests.cpp 
daa46e05d113fd62ea9dad042ec41aaab28ad003 
  3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 
a69fd2d08838e9f0b5738531b29dbca996b70dcb 

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


Testing
---

make check.


Thanks,

Jojy Varghese



Re: Review Request 44934: Updated mesos-execute to add support for Appc.

2016-03-19 Thread Jojy Varghese


> On March 17, 2016, 7:48 a.m., Guangya Liu wrote:
> > src/cli/execute.cpp, line 241
> > <https://reviews.apache.org/r/44934/diff/2/?file=1302214#file1302214line241>
> >
> > The default value of `containerizer` is `mesos`, do we need to check 
> > this?
> > 
> > What about make the logic as this? This will involve less code change 
> > and the logic may be more clear?
> > 
> > // Do not touch old dockerImage logic.
> > if (dockerImage.isSome()) {
> >   old logic here;
> > }
> > 
> > if (appcImage.isSome() && containerizer == "mesos") {
> >   ContainerInfo containerInfo;
> >   containerInfo.set_type(ContainerInfo::MESOS);
> >   ContainerInfo::MesosInfo mesosInfo;
> > 
> >   Image::Appc appc;
> > 
> >   appc.set_name(appcImage.get());
> > 
> >   // TODO(jojy): Labels are hard coded right now.
> >   // Consider adding label flags for customization.
> >   Label arch;
> >   arch.set_key("arch");
> >   arch.set_value("amd64");
> > 
> >   Label os;
> >   os.set_key("os");
> >   os.set_value("linux");
> > 
> >   Labels labels;
> >   labels.add_labels()->CopyFrom(os);
> >   labels.add_labels()->CopyFrom(arch);
> > 
> >   appc.mutable_labels()->CopyFrom(labels);
> > 
> >   Image mesosImage;
> >   
> >   mesosImage.set_type(Image::APPC);
> >   mesosImage.mutable_appc()->CopyFrom(appc);
> >   
> >   mesosInfo.mutable_image()->CopyFrom(mesosImage);
> >   
> >   containerInfo.mutable_mesos()->CopyFrom(mesosInfo);
> > 
> >   task.mutable_container()->CopyFrom(containerInfo);
> > }

Then we will have to repeat the `if (containerizer == "mesos")` for both 
dockerImage case and appcImage case. By seperating the logic by cotainerizer, 
we now can add more image types into the "mesos" containerizer block (say OCI 
image).


- Jojy


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


On March 17, 2016, 6:54 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44934/
> ---
> 
> (Updated March 17, 2016, 6:54 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated mesos-execute to add support for Appc.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp ed42cb568f3d16856f48b3bbd354cb2b0fb83e8e 
> 
> Diff: https://reviews.apache.org/r/44934/diff/
> 
> 
> Testing
> ---
> 
> Tested with various Appc images.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>



Re: Review Request 44934: Updated mesos-execute to add support for Appc.

2016-03-19 Thread Jojy Varghese


> On March 17, 2016, 7:48 a.m., Guangya Liu wrote:
> > src/cli/execute.cpp, line 241
> > <https://reviews.apache.org/r/44934/diff/2/?file=1302214#file1302214line241>
> >
> > The default value of `containerizer` is `mesos`, do we need to check 
> > this?
> > 
> > What about make the logic as this? This will involve less code change 
> > and the logic may be more clear?
> > 
> > // Do not touch old dockerImage logic.
> > if (dockerImage.isSome()) {
> >   old logic here;
> > }
> > 
> > if (appcImage.isSome() && containerizer == "mesos") {
> >   ContainerInfo containerInfo;
> >   containerInfo.set_type(ContainerInfo::MESOS);
> >   ContainerInfo::MesosInfo mesosInfo;
> > 
> >   Image::Appc appc;
> > 
> >   appc.set_name(appcImage.get());
> > 
> >   // TODO(jojy): Labels are hard coded right now.
> >   // Consider adding label flags for customization.
> >   Label arch;
> >   arch.set_key("arch");
> >   arch.set_value("amd64");
> > 
> >   Label os;
> >   os.set_key("os");
> >   os.set_value("linux");
> > 
> >   Labels labels;
> >   labels.add_labels()->CopyFrom(os);
> >   labels.add_labels()->CopyFrom(arch);
> > 
> >   appc.mutable_labels()->CopyFrom(labels);
> > 
> >   Image mesosImage;
> >   
> >   mesosImage.set_type(Image::APPC);
> >   mesosImage.mutable_appc()->CopyFrom(appc);
> >   
> >   mesosInfo.mutable_image()->CopyFrom(mesosImage);
> >   
> >   containerInfo.mutable_mesos()->CopyFrom(mesosInfo);
> > 
> >   task.mutable_container()->CopyFrom(containerInfo);
> > }
> 
> Jojy Varghese wrote:
> Then we will have to repeat the `if (containerizer == "mesos")` for both 
> dockerImage case and appcImage case. By seperating the logic by cotainerizer, 
> we now can add more image types into the "mesos" containerizer block (say OCI 
> image).
> 
> Guangya Liu wrote:
> Yes, that's also my concern when I open this issue, I think it is ok to 
> keep your current logic.
> 
> The logic of checking `containerizer.empty()` should be removed?
> 
> Jojy Varghese wrote:
> We could remove that but the idea is that the block can be cleanly 
> separated out now to a method (say `handleContainerizer`) that handles the 
> containerizers.

cleaned up/refactored the flow to make it easy to follow.


- Jojy


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


On March 18, 2016, 5:26 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44934/
> ---
> 
> (Updated March 18, 2016, 5:26 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated mesos-execute to add support for Appc.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp ed42cb568f3d16856f48b3bbd354cb2b0fb83e8e 
> 
> Diff: https://reviews.apache.org/r/44934/diff/
> 
> 
> Testing
> ---
> 
> Tested with various Appc images.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>



Re: Review Request 44755: Added getAppcImage for Appc provisioning tests.

2016-03-19 Thread Jojy Varghese

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

(Updated March 18, 2016, 12:48 a.m.)


Review request for mesos and Jie Yu.


Repository: mesos


Description
---

Added helper function to get an Appc image protobuf object with given
parameters.


Diffs (updated)
-

  src/tests/containerizer/provisioner_appc_tests.cpp 
069af8a0b5e031d9032fd13154cee250f279b404 

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


Testing
---

make check.


Thanks,

Jojy Varghese



Re: Review Request 44934: Updated mesos-execute to add support for Appc.

2016-03-19 Thread Jojy Varghese

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

(Updated March 17, 2016, 6:54 a.m.)


Review request for mesos and Jie Yu.


Repository: mesos


Description
---

Updated mesos-execute to add support for Appc.


Diffs (updated)
-

  src/cli/execute.cpp ed42cb568f3d16856f48b3bbd354cb2b0fb83e8e 

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


Testing
---

Tested with various Appc images.


Thanks,

Jojy Varghese



Re: Review Request 44934: Updated mesos-execute to add support for Appc.

2016-03-18 Thread Jojy Varghese


> On March 17, 2016, 2:16 a.m., Guangya Liu wrote:
> > src/cli/execute.cpp, lines 251-281
> > <https://reviews.apache.org/r/44934/diff/1/?file=1301837#file1301837line251>
> >
> > What is the behavior of this if with mesos containerizer but without a 
> > docker/appc image?
> > 
> > The current logic is a bit different from previous logic, the mesos 
> > execute will not construct the containerInfo if no docker image.

Thanks for the review ! Updated the patch.


- Jojy


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


On March 16, 2016, 9:21 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44934/
> ---
> 
> (Updated March 16, 2016, 9:21 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated mesos-execute to add support for Appc.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp ed42cb568f3d16856f48b3bbd354cb2b0fb83e8e 
> 
> Diff: https://reviews.apache.org/r/44934/diff/
> 
> 
> Testing
> ---
> 
> Tested with various Appc images.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>



Re: Review Request 44874: Added support for FTS_SLNONE in rmdir.

2016-03-15 Thread Jojy Varghese


> On March 16, 2016, 12:29 a.m., Neil Conway wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/os/rmdir_tests.cpp, line 222
> > <https://reviews.apache.org/r/44874/diff/1/?file=1300429#file1300429line222>
> >
> > Why is this a `string&` and `newDirectory` is a `string`? I'd think 
> > both should probably be `string`.
> 
> Jie Yu wrote:
> Good catch, we should use 'string'. We avoid using const ref to capture 
> temp variable.

A pattern I copied from `os_tests.cpp`. Copy paste horror. I think we need to 
fix it there as well as we fix it here.


- Jojy


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


On March 15, 2016, 11:47 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44874/
> ---
> 
> (Updated March 15, 2016, 11:47 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for FTS_SLNONE in rmdir.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/rmdir.hpp 
> cf21e7fe458626c7533e596997cab3afdabb55f4 
>   3rdparty/libprocess/3rdparty/stout/tests/os/rmdir_tests.cpp 
> 291a22b5aae53b0bc32ae18b9343ceb5a638b37b 
> 
> Diff: https://reviews.apache.org/r/44874/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>



Re: Review Request 44874: Added support for FTS_SLNONE in rmdir.

2016-03-15 Thread Jojy Varghese


> On March 16, 2016, 12:25 a.m., Neil Conway wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/rmdir.hpp, line 71
> > <https://reviews.apache.org/r/44874/diff/1/?file=1300428#file1300428line71>
> >
> > "don't". Also, we should use backticks for `FTS_COMFOLLOW` for 
> > consistency.
> > 
> > Is that comment correct? ISTM that the relevant option that controls 
> > whether symbolic links are resolved is `FTS_LOGICAL` or `FTS_PHYSICAL`; 
> > `FTS_COMFOLLOW` is a special-case for the root path that is passed to 
> > `fts_open`.
> > 
> > While we're at it, the OSX and Linux versions of the manpage for 
> > `fts_open` both claim that "either FTS_LOGICAL or FTS_PHYSICAL" *must* be 
> > specified. Seems like we're specifying neither, both here and in the other 
> > call-site to `fts_open` in `src/linux/cgroups.cpp`?
> 
> Jie Yu wrote:
> Looks like we should use `FTS_PHYSICAL` in rmdir?

I made the assumption looking at some implementation source code at 
http://osxr.org:8080/glibc/source/io/fts.c?v=glibc-2.17. L147 is where 
`fts_open` calls  `fts_stat` with a check for `FTS_COMFOLLOW`. In `fts_stat`, 
L879, the check is made for `logical` or `follow`.


- Jojy


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


On March 15, 2016, 11:47 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44874/
> ---
> 
> (Updated March 15, 2016, 11:47 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for FTS_SLNONE in rmdir.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/rmdir.hpp 
> cf21e7fe458626c7533e596997cab3afdabb55f4 
>   3rdparty/libprocess/3rdparty/stout/tests/os/rmdir_tests.cpp 
> 291a22b5aae53b0bc32ae18b9343ceb5a638b37b 
> 
> Diff: https://reviews.apache.org/r/44874/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>



Review Request 44874: Added support for FTS_SLNONE in rmdir.

2016-03-15 Thread Jojy Varghese

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

Review request for mesos, Jie Yu and Neil Conway.


Repository: mesos


Description
---

Added support for FTS_SLNONE in rmdir.


Diffs
-

  3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/rmdir.hpp 
cf21e7fe458626c7533e596997cab3afdabb55f4 
  3rdparty/libprocess/3rdparty/stout/tests/os/rmdir_tests.cpp 
291a22b5aae53b0bc32ae18b9343ceb5a638b37b 

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


Testing
---

make check.


Thanks,

Jojy Varghese



Review Request 44873: Added test for rmdir with device file.

2016-03-15 Thread Jojy Varghese

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

Review request for mesos, Jie Yu and Neil Conway.


Repository: mesos


Description
---

Existing tests did not cover the case of removing directories with
special files like device files.


Diffs
-

  3rdparty/libprocess/3rdparty/stout/tests/os/rmdir_tests.cpp 
291a22b5aae53b0bc32ae18b9343ceb5a638b37b 

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


Testing
---

make check.


Thanks,

Jojy Varghese



Re: Review Request 44230: Added FS_DEFAULT case in rmdir.

2016-03-15 Thread Jojy Varghese
Yes thats correct. I am planning to submit a patch that will add the test. Will 
also address the symlink case.

-jojy

> On Mar 15, 2016, at 3:33 PM, Cong Wang <xiyou.wangc...@gmail.com> wrote:
> 
> This is an automatically generated e-mail. To reply, 
> visit:https://reviews.apache.org/r/44230/ 
> <https://reviews.apache.org/r/44230/>
> On March 15th, 2016, 8:02 p.m. UTC, Neil Conway wrote:
> 
> Is it feasible/portable to have a test case for this change?
> On March 15th, 2016, 8:58 p.m. UTC, Cong Wang wrote:
> 
> Yes, like in our case, you can create some socket or device file and try to 
> remove the directory contains it, it would fail without this patch.
> On March 15th, 2016, 9:26 p.m. UTC, David Robinson wrote:
> 
> AFAICT tests have already been added:
> https://github.com/apache/mesos/commit/c55ca2941b0119c13764bccdebcea46119e69e4e
> None of these tests covers this bug.
> 
> - Cong
> 
> 
> On March 1st, 2016, 10:01 p.m. UTC, Jojy Varghese wrote:
> 
> Review request for mesos and Jie Yu.
> By Jojy Varghese.
> Updated March 1, 2016, 10:01 p.m.
> 
> Repository: mesos
> Description
> 
> We currently dont handle special files like device files in rmdir. This change
> adds FS_DEFAULT as one of the cases where we try to unlink a file.
> 
> Reference: http://man7.org/linux/man-pages/man3/fts.3.html
> Testing
> 
> make check.
> Diffs
> 
> 3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/rmdir.hpp 
> (bc420c9c10d93ddd619a9eb2c5f4db67f31d722f)
> View Diff <https://reviews.apache.org/r/44230/diff/>


Re: Review Request 44230: Added FS_DEFAULT case in rmdir.

2016-03-15 Thread Jojy Varghese
Would be happy to add test. Patch forthcoming. Will also address FTS_SLNONE.

-Jojy


> On Mar 15, 2016, at 2:26 PM, David Robinson <drobin...@twopensource.com> 
> wrote:
> 
> 
> 
>> On March 15, 2016, 8:02 p.m., Neil Conway wrote:
>>> Is it feasible/portable to have a test case for this change?
>> 
>> Cong Wang wrote:
>>Yes, like in our case, you can create some socket or device file and try 
>> to remove the directory contains it, it would fail without this patch.
> 
> AFAICT tests have already been added:
> 
> https://github.com/apache/mesos/commit/c55ca2941b0119c13764bccdebcea46119e69e4e
> 
> 
> - David
> 
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44230/#review123733
> -------
> 
> 
> On March 1, 2016, 10:01 p.m., Jojy Varghese wrote:
>> 
>> ---
>> This is an automatically generated e-mail. To reply, visit:
>> https://reviews.apache.org/r/44230/
>> ---
>> 
>> (Updated March 1, 2016, 10:01 p.m.)
>> 
>> 
>> Review request for mesos and Jie Yu.
>> 
>> 
>> Repository: mesos
>> 
>> 
>> Description
>> ---
>> 
>> We currently dont handle special files like device files in rmdir. This 
>> change
>> adds FS_DEFAULT as one of the cases where we try to unlink a file.
>> 
>> Reference: http://man7.org/linux/man-pages/man3/fts.3.html
>> 
>> 
>> Diffs
>> -
>> 
>>  3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/rmdir.hpp 
>> bc420c9c10d93ddd619a9eb2c5f4db67f31d722f 
>> 
>> Diff: https://reviews.apache.org/r/44230/diff/
>> 
>> 
>> Testing
>> ---
>> 
>> make check.
>> 
>> 
>> Thanks,
>> 
>> Jojy Varghese
>> 
>> 
> 



Re: Review Request 44755: Added getAppcImage for Appc provisioning tests.

2016-03-15 Thread Jojy Varghese


> On March 15, 2016, 4:56 p.m., Jie Yu wrote:
> > src/tests/containerizer/provisioner_appc_tests.cpp, lines 587-610
> > <https://reviews.apache.org/r/44755/diff/1/?file=1296713#file1296713line587>
> >
> > Can you move this function to the top of this file and make it 'static'?
> 
> Anand Mazumdar wrote:
> Wondering why does this function needs to be a global and can't be part 
> of the test fixture i.e. `AppcImageFetcherTest`?
> 
> Jojy Varghese wrote:
> The intend is to use it in all fixtures. Eg, for integegration tests, we 
> have a separate fixture.
> 
> Anand Mazumdar wrote:
> I see. If the function has static scope, how would we use it across 
> separate fixtures i.e. in some other test file? 
> 
> This is assuming that the integration tests would be part of a separate 
> test file since we typically avoid having multiple test fixtures in a single 
> file.

Right now the integration tests are in the same file (in another patch). Not 
sure if having multiple fixtures in a test file is a no-no (eg. 
provisioner_docker_tests).


- Jojy


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


On March 15, 2016, 6:26 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44755/
> ---
> 
> (Updated March 15, 2016, 6:26 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added helper function to get an Appc image protobuf object with given
> parameters.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/provisioner_appc_tests.cpp 
> 069af8a0b5e031d9032fd13154cee250f279b404 
> 
> Diff: https://reviews.apache.org/r/44755/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>



Re: Review Request 44755: Added getAppcImage for Appc provisioning tests.

2016-03-15 Thread Jojy Varghese


> On March 15, 2016, 4:56 p.m., Jie Yu wrote:
> > src/tests/containerizer/provisioner_appc_tests.cpp, lines 587-610
> > <https://reviews.apache.org/r/44755/diff/1/?file=1296713#file1296713line587>
> >
> > Can you move this function to the top of this file and make it 'static'?
> 
> Anand Mazumdar wrote:
> Wondering why does this function needs to be a global and can't be part 
> of the test fixture i.e. `AppcImageFetcherTest`?

The intend is to use it in all fixtures. Eg, for integegration tests, we have a 
separate fixture.


- Jojy


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


On March 12, 2016, 5:11 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44755/
> ---
> 
> (Updated March 12, 2016, 5:11 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added helper function to get an Appc image protobuf object with given
> parameters.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/provisioner_appc_tests.cpp 
> 069af8a0b5e031d9032fd13154cee250f279b404 
> 
> Diff: https://reviews.apache.org/r/44755/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>



Re: Review Request 44755: Added getAppcImage for Appc provisioning tests.

2016-03-15 Thread Jojy Varghese

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

(Updated March 15, 2016, 6:26 p.m.)


Review request for mesos and Jie Yu.


Changes
---

review addressed.


Repository: mesos


Description
---

Added helper function to get an Appc image protobuf object with given
parameters.


Diffs (updated)
-

  src/tests/containerizer/provisioner_appc_tests.cpp 
069af8a0b5e031d9032fd13154cee250f279b404 

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


Testing
---

make check.


Thanks,

Jojy Varghese



Re: Review Request 44456: Added Appc provisioner integration test.

2016-03-12 Thread Jojy Varghese

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

(Updated March 12, 2016, 5:14 p.m.)


Review request for mesos and Jie Yu.


Changes
---

review addressed.


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


Repository: mesos


Description
---

Added Appc provisioner integration test.


Diffs (updated)
-

  src/tests/containerizer/provisioner_appc_tests.cpp 
069af8a0b5e031d9032fd13154cee250f279b404 

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


Testing
---

make check.


Thanks,

Jojy Varghese



Review Request 44755: Added getAppcImage for Appc provisioning tests.

2016-03-12 Thread Jojy Varghese

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

Review request for mesos and Jie Yu.


Repository: mesos


Description
---

Added helper function to get an Appc image protobuf object with given
parameters.


Diffs
-

  src/tests/containerizer/provisioner_appc_tests.cpp 
069af8a0b5e031d9032fd13154cee250f279b404 

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


Testing
---

make check.


Thanks,

Jojy Varghese



Re: Review Request 44456: Added Appc provisioner integration test.

2016-03-11 Thread Jojy Varghese

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

(Updated March 11, 2016, 5:06 p.m.)


Review request for mesos and Jie Yu.


Changes
---

review addressed.


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


Repository: mesos


Description
---

Added Appc provisioner integration test.


Diffs (updated)
-

  src/tests/containerizer/provisioner_appc_tests.cpp 
6c8087e17aa8b7139ba12337d5be472b7099e77f 

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


Testing
---

make check.


Thanks,

Jojy Varghese



Re: Review Request 44299: Added unit test for file URI fetcher.

2016-03-08 Thread Jojy Varghese


> On March 8, 2016, 1:57 a.m., Jie Yu wrote:
> > src/tests/containerizer/provisioner_appc_tests.cpp, lines 590-591
> > <https://reviews.apache.org/r/44299/diff/2/?file=1280158#file1280158line590>
> >
> > Looking at the original code, why do we need this id? Should that 
> > always be the same? Why do we need to pass it in?

moved the image id up to `prepareImage` method.


- Jojy


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


On March 8, 2016, 10:49 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44299/
> ---
> 
> (Updated March 8, 2016, 10:49 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added unit test for file URI fetcher.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/provisioner_appc_tests.cpp 
> 6c8087e17aa8b7139ba12337d5be472b7099e77f 
> 
> Diff: https://reviews.apache.org/r/44299/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>



Re: Review Request 44456: Added Appc provisioner integration test.

2016-03-08 Thread Jojy Varghese

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

(Updated March 8, 2016, 10:50 p.m.)


Review request for mesos and Jie Yu.


Changes
---

rebased.


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


Repository: mesos


Description (updated)
---

Added Appc provisioner integration test.


Diffs (updated)
-

  src/tests/containerizer/provisioner_appc_tests.cpp 
6c8087e17aa8b7139ba12337d5be472b7099e77f 

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


Testing
---

make check.


Thanks,

Jojy Varghese



Re: Review Request 44299: Added unit test for file URI fetcher.

2016-03-08 Thread Jojy Varghese

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

(Updated March 8, 2016, 10:49 p.m.)


Review request for mesos and Jie Yu.


Repository: mesos


Description
---

Added unit test for file URI fetcher.


Diffs (updated)
-

  src/tests/containerizer/provisioner_appc_tests.cpp 
6c8087e17aa8b7139ba12337d5be472b7099e77f 

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


Testing
---

make check.


Thanks,

Jojy Varghese



Review Request 44534: Refactored AppcImageFetcherTest.

2016-03-08 Thread Jojy Varghese

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

Review request for mesos and Jie Yu.


Repository: mesos


Description
---

Changes:
  - Changed TestAppcImageServer mock methods to real implementation that
  serves http requests. This will allow the TestAppcImageServer to serve
  multiple images in a test.
  - Made TestAppcImageServer loadable so that it can serve images from its
  'images' directory (currently defaulted to 'server').
  - Renamed prepareServerImage to prepareImage to reflect the semantics.


Diffs
-

  src/tests/containerizer/provisioner_appc_tests.cpp 
6c8087e17aa8b7139ba12337d5be472b7099e77f 

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


Testing
---

make check.


Thanks,

Jojy Varghese



  1   2   3   4   5   6   7   8   >