Re: Review Request 71024: Supported insecure registry when provisioning docker images.

2019-07-14 Thread fei long 


> On July 9, 2019, 5:44 a.m., Jie Yu wrote:
> > Can you add some unit test for this? For instance, toggle the new agent 
> > flag, test with some docker registry (ideally, an unsecure registry, but I 
> > think it's ok to test with a secure one too just to excersize the code 
> > path).
> 
> fei long wrote:
> I started a local registry(127.0.0.1:5000) in a docker container and the 
> test case passed. But I am not sure how to redo it in the CI environment 
> since I need to
> 1. start a "registry" docker container
> 2. make sure that 127.0.0.1 is not in the "insecure registries" list. 
> 
> Could you give some advice?
> 
> Jie Yu wrote:
> you don't necessarily need to test with a insecure registry. we should 
> add a test to exercise the case where this agent flag is toggled. You can 
> still use dockerhub for that.

Updated.


- fei


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


On July 14, 2019, 10:35 a.m., fei long wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71024/
> ---
> 
> (Updated July 14, 2019, 10:35 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Qian Zhang.
> 
> 
> Bugs: MESOS-6183
> https://issues.apache.org/jira/browse/MESOS-6183
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Supported insecure registry when provisioning docker images.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/docker/puller.cpp 
> d013c9d71c39c09e600f181aba31b8037aa9226a 
>   src/slave/containerizer/mesos/provisioner/docker/store.cpp 
> 286ee5f00933b4c86a96dc4e10e42f9e7eac0ce2 
>   src/slave/flags.hpp a10bf698dc447b1411c06083c2ba7585d7a78389 
>   src/slave/flags.cpp b4e3eb99221a09404dbbf813da33607867a78691 
>   src/slave/http.cpp 69e6d74e8b113cc6c937f47df8984ff9a63e5bb4 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> 5d5a355afd9c4fda1c653d6cecb75703b0fe 
>   src/uri/fetchers/docker.hpp 2bb921474f3a8147d8cbf54579452f8df216d2f1 
>   src/uri/fetchers/docker.cpp 8f5fc964f056b349ce57ced139e07f538cb1cfd2 
> 
> 
> Diff: https://reviews.apache.org/r/71024/diff/2/
> 
> 
> Testing
> ---
> 
> All testing cases passed.
> 
> 
> Thanks,
> 
> fei long
> 
>

Re: Review Request 70757: Added a NNP isolator.

2019-07-14 Thread Mesos Reviewbot 

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



Bad review!

Reviews applied: [70757]

Error:
2019-07-15 00:35:30 URL:https://reviews.apache.org/r/70757/diff/raw/ 
[12172/12172] -> "70757.patch" [1]
error: patch failed: src/slave/containerizer/mesos/launch.cpp:57
error: src/slave/containerizer/mesos/launch.cpp: patch does not apply
error: src/tests/containerizer/linux_nnp_isolator_tests.cpp: does not exist in 
index

- Mesos Reviewbot


On July 11, 2019, 4:14 p.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70757/
> ---
> 
> (Updated July 11, 2019, 4:14 p.m.)
> 
> 
> Review request for mesos, Andrei Budnik, Gilbert Song, Jie Yu, and James 
> Peach.
> 
> 
> Bugs: MESOS-9770
> https://issues.apache.org/jira/browse/MESOS-9770
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds the isolation capability of flipping the
> NO_NEW_PRIVILEGES bit for process control.
> 
> 
> Diffs
> -
> 
>   include/mesos/slave/containerizer.proto 
> 2d04f3c182a4274dda527a3da56c894c3c892a12 
>   src/CMakeLists.txt eb4e2ac699121ac16e2ac58e4606b26aae906ad8 
>   src/Makefile.am 761dde1d63e0f4f1ac4ab86f129f84f3746d3153 
>   src/slave/containerizer/mesos/containerizer.cpp 
> c9a369bcdbfc1676912430c3e84fa567bbd7f766 
>   src/slave/containerizer/mesos/isolators/linux/nnp.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/linux/nnp.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/launch.cpp 
> 0419e8e0fbf154396ab5fb5026d77b94cc9fca5b 
>   src/tests/CMakeLists.txt 5eb9c65f30d9d6ade289f9d47b18d73908b1f1db 
>   src/tests/containerizer/linux_nnp_isolator_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/70757/diff/7/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 70757: Added a NNP isolator.

2019-07-14 Thread James Peach 

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



This looks like it is in good shape, just some final nits. Looking forward to a 
later patch to add documentation and changelog updates :)

Please rebase and check the commit series. The review desn't apply because the 
nnp isolator test are a patch rather than a new file:
```
$ ./support/apply-reviews.py -r 70757
2019-07-14 17:03:13 URL:https://reviews.apache.org/r/70757/diff/raw/ 
[12172/12172] -> "70757.patch" [1]
error: patch failed: src/slave/containerizer/mesos/launch.cpp:57
error: src/slave/containerizer/mesos/launch.cpp: patch does not apply
error: src/tests/containerizer/linux_nnp_isolator_tests.cpp: does not exist in 
index
```

I think that this is also why whic review ends up depending on itself when you 
update it.


src/slave/containerizer/mesos/isolators/linux/nnp.hpp
Lines 46 (patched)


We don't need to store a copy of the flags, which also means you can remove 
the constructor.



src/slave/containerizer/mesos/isolators/linux/nnp.cpp
Lines 22 (patched)


Just 1 empty line here.



src/slave/containerizer/mesos/launch.cpp
Lines 1162 (patched)


As per Andrei, if the message field is present, toggle the NNP flag 
depending on its value. e.g.
```
int value = launchInfo.no_new_privileges() ? 1 : 0;

prctl(PR_SET_NO_NEW_PRIVS, value, 0, 0, 0)
```



src/slave/containerizer/mesos/launch.cpp
Lines 1163 (patched)


We should include `os::strerror(errno)` here:
```
cerr << "foo foo foo: " << os::strerror(errno) << endl;
```


- James Peach


On July 11, 2019, 11:14 p.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70757/
> ---
> 
> (Updated July 11, 2019, 11:14 p.m.)
> 
> 
> Review request for mesos, Andrei Budnik, Gilbert Song, Jie Yu, and James 
> Peach.
> 
> 
> Bugs: MESOS-9770
> https://issues.apache.org/jira/browse/MESOS-9770
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds the isolation capability of flipping the
> NO_NEW_PRIVILEGES bit for process control.
> 
> 
> Diffs
> -
> 
>   include/mesos/slave/containerizer.proto 
> 2d04f3c182a4274dda527a3da56c894c3c892a12 
>   src/CMakeLists.txt eb4e2ac699121ac16e2ac58e4606b26aae906ad8 
>   src/Makefile.am 761dde1d63e0f4f1ac4ab86f129f84f3746d3153 
>   src/slave/containerizer/mesos/containerizer.cpp 
> c9a369bcdbfc1676912430c3e84fa567bbd7f766 
>   src/slave/containerizer/mesos/isolators/linux/nnp.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/linux/nnp.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/launch.cpp 
> 0419e8e0fbf154396ab5fb5026d77b94cc9fca5b 
>   src/tests/CMakeLists.txt 5eb9c65f30d9d6ade289f9d47b18d73908b1f1db 
>   src/tests/containerizer/linux_nnp_isolator_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/70757/diff/7/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 71024: Supported insecure registry when provisioning docker images.

2019-07-14 Thread Mesos Reviewbot 

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



Patch looks great!

Reviews applied: [71024]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose --disable-libtool-wrappers 
--disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker-build.sh

- Mesos Reviewbot


On July 14, 2019, 3:35 a.m., fei long wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71024/
> ---
> 
> (Updated July 14, 2019, 3:35 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Qian Zhang.
> 
> 
> Bugs: MESOS-6183
> https://issues.apache.org/jira/browse/MESOS-6183
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Supported insecure registry when provisioning docker images.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/docker/puller.cpp 
> d013c9d71c39c09e600f181aba31b8037aa9226a 
>   src/slave/containerizer/mesos/provisioner/docker/store.cpp 
> 286ee5f00933b4c86a96dc4e10e42f9e7eac0ce2 
>   src/slave/flags.hpp a10bf698dc447b1411c06083c2ba7585d7a78389 
>   src/slave/flags.cpp b4e3eb99221a09404dbbf813da33607867a78691 
>   src/slave/http.cpp 69e6d74e8b113cc6c937f47df8984ff9a63e5bb4 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> 5d5a355afd9c4fda1c653d6cecb75703b0fe 
>   src/uri/fetchers/docker.hpp 2bb921474f3a8147d8cbf54579452f8df216d2f1 
>   src/uri/fetchers/docker.cpp 8f5fc964f056b349ce57ced139e07f538cb1cfd2 
> 
> 
> Diff: https://reviews.apache.org/r/71024/diff/2/
> 
> 
> Testing
> ---
> 
> All testing cases passed.
> 
> 
> Thanks,
> 
> fei long
> 
>

Re: Review Request 71024: Supported insecure registry when provisioning docker images.

2019-07-14 Thread fei long 

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

(Updated July 14, 2019, 10:35 a.m.)


Review request for mesos, Gilbert Song, Jie Yu, and Qian Zhang.


Changes
---

Changed insecure_docker_registry(string) to use_insecure_docker_registry(bool) 
and added a unit test.


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


Repository: mesos


Description
---

Supported insecure registry when provisioning docker images.


Diffs (updated)
-

  src/slave/containerizer/mesos/provisioner/docker/puller.cpp 
d013c9d71c39c09e600f181aba31b8037aa9226a 
  src/slave/containerizer/mesos/provisioner/docker/store.cpp 
286ee5f00933b4c86a96dc4e10e42f9e7eac0ce2 
  src/slave/flags.hpp a10bf698dc447b1411c06083c2ba7585d7a78389 
  src/slave/flags.cpp b4e3eb99221a09404dbbf813da33607867a78691 
  src/slave/http.cpp 69e6d74e8b113cc6c937f47df8984ff9a63e5bb4 
  src/tests/containerizer/provisioner_docker_tests.cpp 
5d5a355afd9c4fda1c653d6cecb75703b0fe 
  src/uri/fetchers/docker.hpp 2bb921474f3a8147d8cbf54579452f8df216d2f1 
  src/uri/fetchers/docker.cpp 8f5fc964f056b349ce57ced139e07f538cb1cfd2 


Diff: https://reviews.apache.org/r/71024/diff/2/

Changes: https://reviews.apache.org/r/71024/diff/1-2/


Testing
---

All testing cases passed.


Thanks,

fei long