Re: Review Request 53047: MESOS-6212: name format validation for mesos managed docker containers.

2016-10-24 Thread Manuwela Kanade 

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

(Updated Oct. 24, 2016, 8:22 a.m.)


Review request for mesos, Anand Mazumdar, haosdent huang, Timothy Chen, and 
Vinod Kone.


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


Repository: mesos


Description
---

MESOS-6212: name format validation for mesos managed docker containers.


Diffs (updated)
-

  src/slave/containerizer/docker.cpp 8ec4c0a25335fb1b36cb2ab82577f6d3e2f7f008 
  src/tests/containerizer/docker_containerizer_tests.cpp 
6d26797abf6d2b5e42b9e7743789e1edc62c9c1a 

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


Testing
---

1. Ran the "make check"

2. Manual Testing done as below:
Testing without changes: Created and ran an example container which is not run 
using mesos : named "mesos-rsyslog" which got killed once mesos-agent started 
with --containerizer=docker,mesos. 

Testing with changes: Created and ran an example container (not run using 
mesos): named "mesos-rsyslog" which keeps running and mesos-agent does not kill 
it thus avoiding the false positive

3. Added a testcase to test this scenario. Result below:
Test run with the fix passes
[==] Running 1 test from 1 test case.
[--] Global test environment set-up.
[--] 1 test from DockerContainerizerTest
[ RUN  ] DockerContainerizerTest.ROOT_DOCKER_SkipRecoverMalformedUUID
[   OK ] DockerContainerizerTest.ROOT_DOCKER_SkipRecoverMalformedUUID 
(16142 ms)
[--] 1 test from DockerContainerizerTest (16143 ms total)

[--] Global test environment tear-down
[==] 1 test from 1 test case ran. (16157 ms total)
[  PASSED  ] 1 test.


Test run without the fix fails, since container with malformed UUID also gets 
killed:
[==] Running 1 test from 1 test case.
[--] Global test environment set-up.
[--] 1 test from DockerContainerizerTest
[ RUN  ] DockerContainerizerTest.ROOT_DOCKER_SkipRecoverMalformedUUID
../../src/tests/containerizer/docker_containerizer_tests.cpp:1529: Failure
Value of: exists(docker, slaveId, containerId)
  Actual: false
Expected: true
[  FAILED  ] DockerContainerizerTest.ROOT_DOCKER_SkipRecoverMalformedUUID (9054 
ms)
[--] 1 test from DockerContainerizerTest (9055 ms total)


Thanks,

Manuwela Kanade

Re: Review Request 53047: MESOS-6212: name format validation for mesos managed docker containers.

2016-10-24 Thread Timothy Chen 

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


Fix it, then Ship it!




Ship It!


3rdparty/stout/tests/uuid_tests.cpp (line 63)


I can understand why you added this, but can you break this into a separate 
commit?


- Timothy Chen


On Oct. 22, 2016, 5:54 p.m., Manuwela Kanade wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53047/
> ---
> 
> (Updated Oct. 22, 2016, 5:54 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, haosdent huang, Timothy Chen, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-6212
> https://issues.apache.org/jira/browse/MESOS-6212
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-6212: name format validation for mesos managed docker containers.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/tests/uuid_tests.cpp 
> fe9894af93df9cb9b12390d8d7e7885525db384d 
>   src/slave/containerizer/docker.cpp 8ec4c0a25335fb1b36cb2ab82577f6d3e2f7f008 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 6d26797abf6d2b5e42b9e7743789e1edc62c9c1a 
> 
> Diff: https://reviews.apache.org/r/53047/diff/
> 
> 
> Testing
> ---
> 
> 1. Ran the "make check"
> 
> 2. Manual Testing done as below:
> Testing without changes: Created and ran an example container which is not 
> run using mesos : named "mesos-rsyslog" which got killed once mesos-agent 
> started with --containerizer=docker,mesos. 
> 
> Testing with changes: Created and ran an example container (not run using 
> mesos): named "mesos-rsyslog" which keeps running and mesos-agent does not 
> kill it thus avoiding the false positive
> 
> 3. Added a testcase to test this scenario. Result below:
> Test run with the fix passes
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from DockerContainerizerTest
> [ RUN  ] DockerContainerizerTest.ROOT_DOCKER_SkipRecoverMalformedUUID
> [   OK ] DockerContainerizerTest.ROOT_DOCKER_SkipRecoverMalformedUUID 
> (16142 ms)
> [--] 1 test from DockerContainerizerTest (16143 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (16157 ms total)
> [  PASSED  ] 1 test.
> 
> 
> Test run without the fix fails, since container with malformed UUID also gets 
> killed:
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from DockerContainerizerTest
> [ RUN  ] DockerContainerizerTest.ROOT_DOCKER_SkipRecoverMalformedUUID
> ../../src/tests/containerizer/docker_containerizer_tests.cpp:1529: Failure
> Value of: exists(docker, slaveId, containerId)
>   Actual: false
> Expected: true
> [  FAILED  ] DockerContainerizerTest.ROOT_DOCKER_SkipRecoverMalformedUUID 
> (9054 ms)
> [--] 1 test from DockerContainerizerTest (9055 ms total)
> 
> 
> Thanks,
> 
> Manuwela Kanade
> 
>

Re: Review Request 53047: MESOS-6212: name format validation for mesos managed docker containers.

2016-10-22 Thread Manuwela Kanade 

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

(Updated Oct. 22, 2016, 5:46 p.m.)


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


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


Repository: mesos


Description
---

MESOS-6212: name format validation for mesos managed docker containers.


Diffs (updated)
-

  3rdparty/stout/tests/uuid_tests.cpp fe9894af93df9cb9b12390d8d7e7885525db384d 
  src/slave/containerizer/docker.cpp 8ec4c0a25335fb1b36cb2ab82577f6d3e2f7f008 
  src/tests/containerizer/docker_containerizer_tests.cpp 
6d26797abf6d2b5e42b9e7743789e1edc62c9c1a 

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


Testing
---

1. Ran the "make check"

2. Manual Testing done as below:
Testing without changes: Created and ran an example container which is not run 
using mesos : named "mesos-rsyslog" which got killed once mesos-agent started 
with --containerizer=docker,mesos. 

Testing with changes: Created and ran an example container (not run using 
mesos): named "mesos-rsyslog" which keeps running and mesos-agent does not kill 
it thus avoiding the false positive

3. Added a testcase to test this scenario. Result below:
Test run with the fix passes
[==] Running 1 test from 1 test case.
[--] Global test environment set-up.
[--] 1 test from DockerContainerizerTest
[ RUN  ] DockerContainerizerTest.ROOT_DOCKER_SkipRecoverMalformedUUID
[   OK ] DockerContainerizerTest.ROOT_DOCKER_SkipRecoverMalformedUUID 
(16142 ms)
[--] 1 test from DockerContainerizerTest (16143 ms total)

[--] Global test environment tear-down
[==] 1 test from 1 test case ran. (16157 ms total)
[  PASSED  ] 1 test.


Test run without the fix fails, since container with malformed UUID also gets 
killed:
[==] Running 1 test from 1 test case.
[--] Global test environment set-up.
[--] 1 test from DockerContainerizerTest
[ RUN  ] DockerContainerizerTest.ROOT_DOCKER_SkipRecoverMalformedUUID
../../src/tests/containerizer/docker_containerizer_tests.cpp:1529: Failure
Value of: exists(docker, slaveId, containerId)
  Actual: false
Expected: true
[  FAILED  ] DockerContainerizerTest.ROOT_DOCKER_SkipRecoverMalformedUUID (9054 
ms)
[--] 1 test from DockerContainerizerTest (9055 ms total)


Thanks,

Manuwela Kanade

Re: Review Request 53047: MESOS-6212: name format validation for mesos managed docker containers.

2016-10-22 Thread Manuwela Kanade 


> On Oct. 22, 2016, 1:42 p.m., Timothy Chen wrote:
> > src/slave/containerizer/docker.cpp, line 133
> > 
> >
> > Does all the other tests pass with this change? If I remember correctly 
> > in the tests we don't assume the slave and container ids passed in has to 
> > be valid UUIDs.
> 
> haosdent huang wrote:
> All docker related test cases could pass according to my test.
> 
> ```
> [--] Global test environment tear-down
> [==] 46 tests from 4 test cases ran. (229644 ms total)
> [  PASSED  ] 46 tests.
> ```

All DockerContainerizer, MesosContainerizer and DockerTests have passed as 
below:

---] 28 tests from DockerContainerizerTest (120067 ms total)

[--] Global test environment tear-down
[==] 28 tests from 1 test case ran. (120076 ms total)
[  PASSED  ] 28 tests.


[==] 24 tests from 10 test cases ran. (14594 ms total)
[  PASSED  ] 24 tests.

[--] 12 tests from DockerTest (11598 ms total)

[--] Global test environment tear-down
[==] 12 tests from 1 test case ran. (11620 ms total)
[  PASSED  ] 12 tests.


- Manuwela


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


On Oct. 21, 2016, 9:34 a.m., Manuwela Kanade wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53047/
> ---
> 
> (Updated Oct. 21, 2016, 9:34 a.m.)
> 
> 
> Review request for mesos, haosdent huang, Timothy Chen, and Vinod Kone.
> 
> 
> Bugs: MESOS-6212
> https://issues.apache.org/jira/browse/MESOS-6212
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-6212: name format validation for mesos managed docker containers.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/tests/uuid_tests.cpp 
> fe9894af93df9cb9b12390d8d7e7885525db384d 
>   src/slave/containerizer/docker.cpp 8ec4c0a25335fb1b36cb2ab82577f6d3e2f7f008 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 6d26797abf6d2b5e42b9e7743789e1edc62c9c1a 
> 
> Diff: https://reviews.apache.org/r/53047/diff/
> 
> 
> Testing
> ---
> 
> 1. Ran the "make check"
> 
> 2. Manual Testing done as below:
> Testing without changes: Created and ran an example container which is not 
> run using mesos : named "mesos-rsyslog" which got killed once mesos-agent 
> started with --containerizer=docker,mesos. 
> 
> Testing with changes: Created and ran an example container (not run using 
> mesos): named "mesos-rsyslog" which keeps running and mesos-agent does not 
> kill it thus avoiding the false positive
> 
> 3. Added a testcase to test this scenario. Result below:
> Test run with the fix passes
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from DockerContainerizerTest
> [ RUN  ] DockerContainerizerTest.ROOT_DOCKER_SkipRecoverMalformedUUID
> [   OK ] DockerContainerizerTest.ROOT_DOCKER_SkipRecoverMalformedUUID 
> (16142 ms)
> [--] 1 test from DockerContainerizerTest (16143 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (16157 ms total)
> [  PASSED  ] 1 test.
> 
> 
> Test run without the fix fails, since container with malformed UUID also gets 
> killed:
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from DockerContainerizerTest
> [ RUN  ] DockerContainerizerTest.ROOT_DOCKER_SkipRecoverMalformedUUID
> ../../src/tests/containerizer/docker_containerizer_tests.cpp:1529: Failure
> Value of: exists(docker, slaveId, containerId)
>   Actual: false
> Expected: true
> [  FAILED  ] DockerContainerizerTest.ROOT_DOCKER_SkipRecoverMalformedUUID 
> (9054 ms)
> [--] 1 test from DockerContainerizerTest (9055 ms total)
> 
> 
> Thanks,
> 
> Manuwela Kanade
> 
>

Re: Review Request 53047: MESOS-6212: name format validation for mesos managed docker containers.

2016-10-22 Thread haosdent huang 


> On Oct. 22, 2016, 1:42 p.m., Timothy Chen wrote:
> > src/slave/containerizer/docker.cpp, line 133
> > 
> >
> > Does all the other tests pass with this change? If I remember correctly 
> > in the tests we don't assume the slave and container ids passed in has to 
> > be valid UUIDs.

All docker related test cases could pass according to my test.

```
[--] Global test environment tear-down
[==] 46 tests from 4 test cases ran. (229644 ms total)
[  PASSED  ] 46 tests.
```


- haosdent


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


On Oct. 21, 2016, 9:34 a.m., Manuwela Kanade wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53047/
> ---
> 
> (Updated Oct. 21, 2016, 9:34 a.m.)
> 
> 
> Review request for mesos, haosdent huang, Timothy Chen, and Vinod Kone.
> 
> 
> Bugs: MESOS-6212
> https://issues.apache.org/jira/browse/MESOS-6212
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-6212: name format validation for mesos managed docker containers.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/tests/uuid_tests.cpp 
> fe9894af93df9cb9b12390d8d7e7885525db384d 
>   src/slave/containerizer/docker.cpp 8ec4c0a25335fb1b36cb2ab82577f6d3e2f7f008 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 6d26797abf6d2b5e42b9e7743789e1edc62c9c1a 
> 
> Diff: https://reviews.apache.org/r/53047/diff/
> 
> 
> Testing
> ---
> 
> 1. Ran the "make check"
> 
> 2. Manual Testing done as below:
> Testing without changes: Created and ran an example container which is not 
> run using mesos : named "mesos-rsyslog" which got killed once mesos-agent 
> started with --containerizer=docker,mesos. 
> 
> Testing with changes: Created and ran an example container (not run using 
> mesos): named "mesos-rsyslog" which keeps running and mesos-agent does not 
> kill it thus avoiding the false positive
> 
> 3. Added a testcase to test this scenario. Result below:
> Test run with the fix passes
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from DockerContainerizerTest
> [ RUN  ] DockerContainerizerTest.ROOT_DOCKER_SkipRecoverMalformedUUID
> [   OK ] DockerContainerizerTest.ROOT_DOCKER_SkipRecoverMalformedUUID 
> (16142 ms)
> [--] 1 test from DockerContainerizerTest (16143 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (16157 ms total)
> [  PASSED  ] 1 test.
> 
> 
> Test run without the fix fails, since container with malformed UUID also gets 
> killed:
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from DockerContainerizerTest
> [ RUN  ] DockerContainerizerTest.ROOT_DOCKER_SkipRecoverMalformedUUID
> ../../src/tests/containerizer/docker_containerizer_tests.cpp:1529: Failure
> Value of: exists(docker, slaveId, containerId)
>   Actual: false
> Expected: true
> [  FAILED  ] DockerContainerizerTest.ROOT_DOCKER_SkipRecoverMalformedUUID 
> (9054 ms)
> [--] 1 test from DockerContainerizerTest (9055 ms total)
> 
> 
> Thanks,
> 
> Manuwela Kanade
> 
>

Re: Review Request 53047: MESOS-6212: name format validation for mesos managed docker containers.

2016-10-22 Thread Timothy Chen 

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




src/slave/containerizer/docker.cpp (line 131)


End comment with period.



src/slave/containerizer/docker.cpp (line 132)


Does all the other tests pass with this change? If I remember correctly in 
the tests we don't assume the slave and container ids passed in has to be valid 
UUIDs.


- Timothy Chen


On Oct. 21, 2016, 9:34 a.m., Manuwela Kanade wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53047/
> ---
> 
> (Updated Oct. 21, 2016, 9:34 a.m.)
> 
> 
> Review request for mesos, haosdent huang, Timothy Chen, and Vinod Kone.
> 
> 
> Bugs: MESOS-6212
> https://issues.apache.org/jira/browse/MESOS-6212
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-6212: name format validation for mesos managed docker containers.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/tests/uuid_tests.cpp 
> fe9894af93df9cb9b12390d8d7e7885525db384d 
>   src/slave/containerizer/docker.cpp 8ec4c0a25335fb1b36cb2ab82577f6d3e2f7f008 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 6d26797abf6d2b5e42b9e7743789e1edc62c9c1a 
> 
> Diff: https://reviews.apache.org/r/53047/diff/
> 
> 
> Testing
> ---
> 
> 1. Ran the "make check"
> 
> 2. Manual Testing done as below:
> Testing without changes: Created and ran an example container which is not 
> run using mesos : named "mesos-rsyslog" which got killed once mesos-agent 
> started with --containerizer=docker,mesos. 
> 
> Testing with changes: Created and ran an example container (not run using 
> mesos): named "mesos-rsyslog" which keeps running and mesos-agent does not 
> kill it thus avoiding the false positive
> 
> 3. Added a testcase to test this scenario. Result below:
> Test run with the fix passes
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from DockerContainerizerTest
> [ RUN  ] DockerContainerizerTest.ROOT_DOCKER_SkipRecoverMalformedUUID
> [   OK ] DockerContainerizerTest.ROOT_DOCKER_SkipRecoverMalformedUUID 
> (16142 ms)
> [--] 1 test from DockerContainerizerTest (16143 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (16157 ms total)
> [  PASSED  ] 1 test.
> 
> 
> Test run without the fix fails, since container with malformed UUID also gets 
> killed:
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from DockerContainerizerTest
> [ RUN  ] DockerContainerizerTest.ROOT_DOCKER_SkipRecoverMalformedUUID
> ../../src/tests/containerizer/docker_containerizer_tests.cpp:1529: Failure
> Value of: exists(docker, slaveId, containerId)
>   Actual: false
> Expected: true
> [  FAILED  ] DockerContainerizerTest.ROOT_DOCKER_SkipRecoverMalformedUUID 
> (9054 ms)
> [--] 1 test from DockerContainerizerTest (9055 ms total)
> 
> 
> Thanks,
> 
> Manuwela Kanade
> 
>

Re: Review Request 53047: MESOS-6212: name format validation for mesos managed docker containers.

2016-10-22 Thread haosdent huang 

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


Fix it, then Ship it!





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


Nit: Add an extra blank line above.



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


Let's add

```
flags.docker_kill_orphans = true;
```

here to make it obviously.



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


Nit:

```
// Clean up artifacts if the container still exists.
```

The word `containers` should not use with `exists` together?



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


Add 

```
  // TODO(tnachen): Use local image to test if possible.
```

above.



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


Let's use `sleep 1000` here to keep consistent with other cases. We perfer 
to use a longer time to avoid flaky test cases because 15s may not enough on a 
slow machine.



src/tests/containerizer/docker_containerizer_tests.cpp (lines 1507 - 1508)


I think may remove these because

```
  ASSERT_TRUE(
exists(docker, slaveId, containerId, ContainerState::RUNNING));
```

make sure the container status is running.



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


Nit:

For comments, we should add a peroid at the end of this.

```
  // The container should still exist and should not get killed
  // by containerizer recovery.
```



src/tests/containerizer/docker_containerizer_tests.cpp (lines 1531 - 1533)


I think we should remove this because 

```
ASSERT_TRUE(exists(docker, slaveId, containerId));
```

make sure the container is still running.


- haosdent huang


On Oct. 21, 2016, 9:34 a.m., Manuwela Kanade wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53047/
> ---
> 
> (Updated Oct. 21, 2016, 9:34 a.m.)
> 
> 
> Review request for mesos, haosdent huang, Timothy Chen, and Vinod Kone.
> 
> 
> Bugs: MESOS-6212
> https://issues.apache.org/jira/browse/MESOS-6212
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-6212: name format validation for mesos managed docker containers.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/tests/uuid_tests.cpp 
> fe9894af93df9cb9b12390d8d7e7885525db384d 
>   src/slave/containerizer/docker.cpp 8ec4c0a25335fb1b36cb2ab82577f6d3e2f7f008 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 6d26797abf6d2b5e42b9e7743789e1edc62c9c1a 
> 
> Diff: https://reviews.apache.org/r/53047/diff/
> 
> 
> Testing
> ---
> 
> 1. Ran the "make check"
> 
> 2. Manual Testing done as below:
> Testing without changes: Created and ran an example container which is not 
> run using mesos : named "mesos-rsyslog" which got killed once mesos-agent 
> started with --containerizer=docker,mesos. 
> 
> Testing with changes: Created and ran an example container (not run using 
> mesos): named "mesos-rsyslog" which keeps running and mesos-agent does not 
> kill it thus avoiding the false positive
> 
> 3. Added a testcase to test this scenario. Result below:
> Test run with the fix passes
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from DockerContainerizerTest
> [ RUN  ] DockerContainerizerTest.ROOT_DOCKER_SkipRecoverMalformedUUID
> [   OK ] DockerContainerizerTest.ROOT_DOCKER_SkipRecoverMalformedUUID 
> (16142 ms)
> [--] 1 test from DockerContainerizerTest (16143 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (16157 ms total)
> [  PASSED  ] 1 test.
> 
> 
> Test run without the fix fails, since container with malformed UUID also gets 
> killed:
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from DockerContainerizerTest
> [ RUN  ] DockerContainerizerTest.ROOT_DOCKER_SkipRecoverMalformedUUID
> ../../src/tests/containerizer/docker_containerizer_tests.cpp:1529: Failure
> Value of: exists(docker, slaveId, containerId)
>   Actual: false
> Expected: true
> [  FAILED  ]

Re: Review Request 53047: MESOS-6212: name format validation for mesos managed docker containers.

2016-10-21 Thread Mesos ReviewBot 

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



Patch looks great!

Reviews applied: [53047]

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

- Mesos ReviewBot


On Oct. 21, 2016, 9:34 a.m., Manuwela Kanade wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53047/
> ---
> 
> (Updated Oct. 21, 2016, 9:34 a.m.)
> 
> 
> Review request for mesos, haosdent huang, Timothy Chen, and Vinod Kone.
> 
> 
> Bugs: MESOS-6212
> https://issues.apache.org/jira/browse/MESOS-6212
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-6212: name format validation for mesos managed docker containers.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/tests/uuid_tests.cpp 
> fe9894af93df9cb9b12390d8d7e7885525db384d 
>   src/slave/containerizer/docker.cpp 8ec4c0a25335fb1b36cb2ab82577f6d3e2f7f008 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 6d26797abf6d2b5e42b9e7743789e1edc62c9c1a 
> 
> Diff: https://reviews.apache.org/r/53047/diff/
> 
> 
> Testing
> ---
> 
> 1. Ran the "make check"
> 
> 2. Manual Testing done as below:
> Testing without changes: Created and ran an example container which is not 
> run using mesos : named "mesos-rsyslog" which got killed once mesos-agent 
> started with --containerizer=docker,mesos. 
> 
> Testing with changes: Created and ran an example container (not run using 
> mesos): named "mesos-rsyslog" which keeps running and mesos-agent does not 
> kill it thus avoiding the false positive
> 
> 3. Added a testcase to test this scenario. Result below:
> Test run with the fix passes
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from DockerContainerizerTest
> [ RUN  ] DockerContainerizerTest.ROOT_DOCKER_SkipRecoverMalformedUUID
> [   OK ] DockerContainerizerTest.ROOT_DOCKER_SkipRecoverMalformedUUID 
> (16142 ms)
> [--] 1 test from DockerContainerizerTest (16143 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (16157 ms total)
> [  PASSED  ] 1 test.
> 
> 
> Test run without the fix fails, since container with malformed UUID also gets 
> killed:
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from DockerContainerizerTest
> [ RUN  ] DockerContainerizerTest.ROOT_DOCKER_SkipRecoverMalformedUUID
> ../../src/tests/containerizer/docker_containerizer_tests.cpp:1529: Failure
> Value of: exists(docker, slaveId, containerId)
>   Actual: false
> Expected: true
> [  FAILED  ] DockerContainerizerTest.ROOT_DOCKER_SkipRecoverMalformedUUID 
> (9054 ms)
> [--] 1 test from DockerContainerizerTest (9055 ms total)
> 
> 
> Thanks,
> 
> Manuwela Kanade
> 
>

Re: Review Request 53047: MESOS-6212: name format validation for mesos managed docker containers.

2016-10-21 Thread Manuwela Kanade 


> On Oct. 21, 2016, 8:58 a.m., haosdent huang wrote:
> > src/slave/containerizer/docker.cpp, line 62
> > 
> >
> > Nit: We sort the headers according some rules which `stout` come first. 
> > Please move this after 
> > 
> > ```
> > #include 
> > #include 
> > ```

Fixed and retested


> On Oct. 21, 2016, 8:58 a.m., haosdent huang wrote:
> > src/slave/containerizer/docker.cpp, line 133
> > 
> >
> > `containerId.get()` may failed here if `contaienrId` is `None`.
> > 
> > ```
> > } else {
> > 
> >   vector parts = strings::split(name.get(), 
> > DOCKER_NAME_SEPERATOR);
> >  if (parts.size() == 2 || parts.size() == 3) { <-- if this 
> > condition is `false`, `containerId` would be `None`. So the 
> > `containerId.get()` would failed in following code.
> > ContainerID id;
> > id.set_value(parts[1]);
> > containerId = id;
> >   }
> > }
> > ```
> > 
> > I suggest to put this above `return`. It may looks like
> > 
> > ```
> >   }
> > 
> >   if (containerId.isSome()) {
> > // Check if id is a valid UUID
> > Try uuid = UUID::fromString(containerId.get().value());
> > if (uuid.isError()) {
> >   return None();
> > }
> >   }
> > 
> >   return containerId;
> > ```

Fixed and retested


- Manuwela


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


On Oct. 21, 2016, 8:24 a.m., Manuwela Kanade wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53047/
> ---
> 
> (Updated Oct. 21, 2016, 8:24 a.m.)
> 
> 
> Review request for mesos, haosdent huang, Timothy Chen, and Vinod Kone.
> 
> 
> Bugs: MESOS-6212
> https://issues.apache.org/jira/browse/MESOS-6212
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-6212: name format validation for mesos managed docker containers.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/tests/uuid_tests.cpp 
> fe9894af93df9cb9b12390d8d7e7885525db384d 
>   src/slave/containerizer/docker.cpp 8ec4c0a25335fb1b36cb2ab82577f6d3e2f7f008 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 6d26797abf6d2b5e42b9e7743789e1edc62c9c1a 
> 
> Diff: https://reviews.apache.org/r/53047/diff/
> 
> 
> Testing
> ---
> 
> 1. Ran the "make check"
> 
> 2. Manual Testing done as below:
> Testing without changes: Created and ran an example container which is not 
> run using mesos : named "mesos-rsyslog" which got killed once mesos-agent 
> started with --containerizer=docker,mesos. 
> 
> Testing with changes: Created and ran an example container (not run using 
> mesos): named "mesos-rsyslog" which keeps running and mesos-agent does not 
> kill it thus avoiding the false positive
> 
> 3. Added a testcase to test this scenario. Result below:
> Test run with the fix passes
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from DockerContainerizerTest
> [ RUN  ] DockerContainerizerTest.ROOT_DOCKER_SkipRecoverMalformedUUID
> [   OK ] DockerContainerizerTest.ROOT_DOCKER_SkipRecoverMalformedUUID 
> (16142 ms)
> [--] 1 test from DockerContainerizerTest (16143 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (16157 ms total)
> [  PASSED  ] 1 test.
> 
> 
> Test run without the fix fails, since container with malformed UUID also gets 
> killed:
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from DockerContainerizerTest
> [ RUN  ] DockerContainerizerTest.ROOT_DOCKER_SkipRecoverMalformedUUID
> ../../src/tests/containerizer/docker_containerizer_tests.cpp:1529: Failure
> Value of: exists(docker, slaveId, containerId)
>   Actual: false
> Expected: true
> [  FAILED  ] DockerContainerizerTest.ROOT_DOCKER_SkipRecoverMalformedUUID 
> (9054 ms)
> [--] 1 test from DockerContainerizerTest (9055 ms total)
> 
> 
> Thanks,
> 
> Manuwela Kanade
> 
>

Re: Review Request 53047: MESOS-6212: name format validation for mesos managed docker containers.

2016-10-21 Thread Manuwela Kanade 

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

(Updated Oct. 21, 2016, 9:34 a.m.)


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


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


Repository: mesos


Description
---

MESOS-6212: name format validation for mesos managed docker containers.


Diffs (updated)
-

  3rdparty/stout/tests/uuid_tests.cpp fe9894af93df9cb9b12390d8d7e7885525db384d 
  src/slave/containerizer/docker.cpp 8ec4c0a25335fb1b36cb2ab82577f6d3e2f7f008 
  src/tests/containerizer/docker_containerizer_tests.cpp 
6d26797abf6d2b5e42b9e7743789e1edc62c9c1a 

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


Testing
---

1. Ran the "make check"

2. Manual Testing done as below:
Testing without changes: Created and ran an example container which is not run 
using mesos : named "mesos-rsyslog" which got killed once mesos-agent started 
with --containerizer=docker,mesos. 

Testing with changes: Created and ran an example container (not run using 
mesos): named "mesos-rsyslog" which keeps running and mesos-agent does not kill 
it thus avoiding the false positive

3. Added a testcase to test this scenario. Result below:
Test run with the fix passes
[==] Running 1 test from 1 test case.
[--] Global test environment set-up.
[--] 1 test from DockerContainerizerTest
[ RUN  ] DockerContainerizerTest.ROOT_DOCKER_SkipRecoverMalformedUUID
[   OK ] DockerContainerizerTest.ROOT_DOCKER_SkipRecoverMalformedUUID 
(16142 ms)
[--] 1 test from DockerContainerizerTest (16143 ms total)

[--] Global test environment tear-down
[==] 1 test from 1 test case ran. (16157 ms total)
[  PASSED  ] 1 test.


Test run without the fix fails, since container with malformed UUID also gets 
killed:
[==] Running 1 test from 1 test case.
[--] Global test environment set-up.
[--] 1 test from DockerContainerizerTest
[ RUN  ] DockerContainerizerTest.ROOT_DOCKER_SkipRecoverMalformedUUID
../../src/tests/containerizer/docker_containerizer_tests.cpp:1529: Failure
Value of: exists(docker, slaveId, containerId)
  Actual: false
Expected: true
[  FAILED  ] DockerContainerizerTest.ROOT_DOCKER_SkipRecoverMalformedUUID (9054 
ms)
[--] 1 test from DockerContainerizerTest (9055 ms total)


Thanks,

Manuwela Kanade

Re: Review Request 53047: MESOS-6212: name format validation for mesos managed docker containers.

2016-10-21 Thread haosdent huang 

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



The patch looks pretty good. For the test case part, it should be the correct 
way to verify if `parse()` works except some nits. I would continue to review 
after compile and test it in my machine.


src/slave/containerizer/docker.cpp (line 62)


Nit: We sort the headers according some rules which `stout` come first. 
Please move this after 

```
#include 
#include 
```



src/slave/containerizer/docker.cpp (line 132)


`containerId.get()` may failed here if `contaienrId` is `None`.

```
} else {

  vector parts = strings::split(name.get(), 
DOCKER_NAME_SEPERATOR);
 if (parts.size() == 2 || parts.size() == 3) { <-- if this condition is 
`false`, `containerId` would be `None`. So the `containerId.get()` would failed 
in following code.
ContainerID id;
id.set_value(parts[1]);
containerId = id;
  }
}
```

I suggest to put this above `return`. It may looks like

```
  }

  if (containerId.isSome()) {
// Check if id is a valid UUID
Try uuid = UUID::fromString(containerId.get().value());
if (uuid.isError()) {
  return None();
}
  }

  return containerId;
```


- haosdent huang


On Oct. 21, 2016, 8:24 a.m., Manuwela Kanade wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53047/
> ---
> 
> (Updated Oct. 21, 2016, 8:24 a.m.)
> 
> 
> Review request for mesos, haosdent huang, Timothy Chen, and Vinod Kone.
> 
> 
> Bugs: MESOS-6212
> https://issues.apache.org/jira/browse/MESOS-6212
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-6212: name format validation for mesos managed docker containers.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/tests/uuid_tests.cpp 
> fe9894af93df9cb9b12390d8d7e7885525db384d 
>   src/slave/containerizer/docker.cpp 8ec4c0a25335fb1b36cb2ab82577f6d3e2f7f008 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 6d26797abf6d2b5e42b9e7743789e1edc62c9c1a 
> 
> Diff: https://reviews.apache.org/r/53047/diff/
> 
> 
> Testing
> ---
> 
> 1. Ran the "make check"
> 
> 2. Manual Testing done as below:
> Testing without changes: Created and ran an example container which is not 
> run using mesos : named "mesos-rsyslog" which got killed once mesos-agent 
> started with --containerizer=docker,mesos. 
> 
> Testing with changes: Created and ran an example container (not run using 
> mesos): named "mesos-rsyslog" which keeps running and mesos-agent does not 
> kill it thus avoiding the false positive
> 
> 3. Added a testcase to test this scenario. Result below:
> Test run with the fix passes
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from DockerContainerizerTest
> [ RUN  ] DockerContainerizerTest.ROOT_DOCKER_SkipRecoverMalformedUUID
> [   OK ] DockerContainerizerTest.ROOT_DOCKER_SkipRecoverMalformedUUID 
> (16142 ms)
> [--] 1 test from DockerContainerizerTest (16143 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (16157 ms total)
> [  PASSED  ] 1 test.
> 
> 
> Test run without the fix fails, since container with malformed UUID also gets 
> killed:
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from DockerContainerizerTest
> [ RUN  ] DockerContainerizerTest.ROOT_DOCKER_SkipRecoverMalformedUUID
> ../../src/tests/containerizer/docker_containerizer_tests.cpp:1529: Failure
> Value of: exists(docker, slaveId, containerId)
>   Actual: false
> Expected: true
> [  FAILED  ] DockerContainerizerTest.ROOT_DOCKER_SkipRecoverMalformedUUID 
> (9054 ms)
> [--] 1 test from DockerContainerizerTest (9055 ms total)
> 
> 
> Thanks,
> 
> Manuwela Kanade
> 
>

Re: Review Request 53047: MESOS-6212: name format validation for mesos managed docker containers.

2016-10-21 Thread Manuwela Kanade 


> On Oct. 20, 2016, 9:55 a.m., haosdent huang wrote:
> > src/tests/containerizer/docker_containerizer_tests.cpp, line 1501
> > 
> >
> > This test case would not go through `parse()` method. Since we only use 
> > state to construct the `Container` then put it in `containers_` directly 
> > which would bypass `parse`.
> > 
> > 
> > ```
> > // Create and store a container.
> > Container* container = new Container(containerId);
> > containers_[containerId] = container;
> > container->slaveId = state->id;
> > container->state = Container::RUNNING;
> > container->launchesExecutorContainer =
> >   executorContainers.contains(containerId);
> > ```
> 
> Manuwela Kanade wrote:
> I will work on changing the testcase such that it goes through the 
> parse() method. Will update soon

I have changed the testcase such that it will go through the parse method, and 
also made sure that this testcase fails without my changes and passes with my 
changes.


- Manuwela


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


On Oct. 21, 2016, 8:24 a.m., Manuwela Kanade wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53047/
> ---
> 
> (Updated Oct. 21, 2016, 8:24 a.m.)
> 
> 
> Review request for mesos, haosdent huang, Timothy Chen, and Vinod Kone.
> 
> 
> Bugs: MESOS-6212
> https://issues.apache.org/jira/browse/MESOS-6212
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-6212: name format validation for mesos managed docker containers.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/tests/uuid_tests.cpp 
> fe9894af93df9cb9b12390d8d7e7885525db384d 
>   src/slave/containerizer/docker.cpp 8ec4c0a25335fb1b36cb2ab82577f6d3e2f7f008 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 6d26797abf6d2b5e42b9e7743789e1edc62c9c1a 
> 
> Diff: https://reviews.apache.org/r/53047/diff/
> 
> 
> Testing
> ---
> 
> 1. Ran the "make check"
> 
> 2. Manual Testing done as below:
> Testing without changes: Created and ran an example container which is not 
> run using mesos : named "mesos-rsyslog" which got killed once mesos-agent 
> started with --containerizer=docker,mesos. 
> 
> Testing with changes: Created and ran an example container (not run using 
> mesos): named "mesos-rsyslog" which keeps running and mesos-agent does not 
> kill it thus avoiding the false positive
> 
> 3. Added a testcase to test this scenario. Result below:
> Test run with the fix passes
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from DockerContainerizerTest
> [ RUN  ] DockerContainerizerTest.ROOT_DOCKER_SkipRecoverMalformedUUID
> [   OK ] DockerContainerizerTest.ROOT_DOCKER_SkipRecoverMalformedUUID 
> (16142 ms)
> [--] 1 test from DockerContainerizerTest (16143 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (16157 ms total)
> [  PASSED  ] 1 test.
> 
> 
> Test run without the fix fails, since container with malformed UUID also gets 
> killed:
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from DockerContainerizerTest
> [ RUN  ] DockerContainerizerTest.ROOT_DOCKER_SkipRecoverMalformedUUID
> ../../src/tests/containerizer/docker_containerizer_tests.cpp:1529: Failure
> Value of: exists(docker, slaveId, containerId)
>   Actual: false
> Expected: true
> [  FAILED  ] DockerContainerizerTest.ROOT_DOCKER_SkipRecoverMalformedUUID 
> (9054 ms)
> [--] 1 test from DockerContainerizerTest (9055 ms total)
> 
> 
> Thanks,
> 
> Manuwela Kanade
> 
>

Re: Review Request 53047: MESOS-6212: name format validation for mesos managed docker containers.

2016-10-21 Thread Manuwela Kanade 

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

(Updated Oct. 21, 2016, 8:24 a.m.)


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


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


Repository: mesos


Description
---

MESOS-6212: name format validation for mesos managed docker containers.


Diffs (updated)
-

  3rdparty/stout/tests/uuid_tests.cpp fe9894af93df9cb9b12390d8d7e7885525db384d 
  src/slave/containerizer/docker.cpp 8ec4c0a25335fb1b36cb2ab82577f6d3e2f7f008 
  src/tests/containerizer/docker_containerizer_tests.cpp 
6d26797abf6d2b5e42b9e7743789e1edc62c9c1a 

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


Testing (updated)
---

1. Ran the "make check"

2. Manual Testing done as below:
Testing without changes: Created and ran an example container which is not run 
using mesos : named "mesos-rsyslog" which got killed once mesos-agent started 
with --containerizer=docker,mesos. 

Testing with changes: Created and ran an example container (not run using 
mesos): named "mesos-rsyslog" which keeps running and mesos-agent does not kill 
it thus avoiding the false positive

3. Added a testcase to test this scenario. Result below:
Test run with the fix passes
[==] Running 1 test from 1 test case.
[--] Global test environment set-up.
[--] 1 test from DockerContainerizerTest
[ RUN  ] DockerContainerizerTest.ROOT_DOCKER_SkipRecoverMalformedUUID
[   OK ] DockerContainerizerTest.ROOT_DOCKER_SkipRecoverMalformedUUID 
(16142 ms)
[--] 1 test from DockerContainerizerTest (16143 ms total)

[--] Global test environment tear-down
[==] 1 test from 1 test case ran. (16157 ms total)
[  PASSED  ] 1 test.


Test run without the fix fails, since container with malformed UUID also gets 
killed:
[==] Running 1 test from 1 test case.
[--] Global test environment set-up.
[--] 1 test from DockerContainerizerTest
[ RUN  ] DockerContainerizerTest.ROOT_DOCKER_SkipRecoverMalformedUUID
../../src/tests/containerizer/docker_containerizer_tests.cpp:1529: Failure
Value of: exists(docker, slaveId, containerId)
  Actual: false
Expected: true
[  FAILED  ] DockerContainerizerTest.ROOT_DOCKER_SkipRecoverMalformedUUID (9054 
ms)
[--] 1 test from DockerContainerizerTest (9055 ms total)


Thanks,

Manuwela Kanade

Re: Review Request 53047: MESOS-6212: name format validation for mesos managed docker containers.

2016-10-20 Thread Mesos ReviewBot 

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



Patch looks great!

Reviews applied: [53047]

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

- Mesos ReviewBot


On Oct. 20, 2016, 11:46 a.m., Manuwela Kanade wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53047/
> ---
> 
> (Updated Oct. 20, 2016, 11:46 a.m.)
> 
> 
> Review request for mesos, haosdent huang, Timothy Chen, and Vinod Kone.
> 
> 
> Bugs: MESOS-6212
> https://issues.apache.org/jira/browse/MESOS-6212
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-6212: name format validation for mesos managed docker containers.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/tests/uuid_tests.cpp 
> fe9894af93df9cb9b12390d8d7e7885525db384d 
>   src/slave/containerizer/docker.cpp 8ec4c0a25335fb1b36cb2ab82577f6d3e2f7f008 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 6d26797abf6d2b5e42b9e7743789e1edc62c9c1a 
> 
> Diff: https://reviews.apache.org/r/53047/diff/
> 
> 
> Testing
> ---
> 
> 1. Ran the "make check"
> 
> 2. Manual Testing done as below:
> Testing without changes: Created and ran an example container which is not 
> run using mesos : named "mesos-rsyslog" which got killed once mesos-agent 
> started with --containerizer=docker,mesos. 
> 
> Testing with changes: Created and ran an example container (not run using 
> mesos): named "mesos-rsyslog" which keeps running and mesos-agent does not 
> kill it thus avoiding the false positive
> 
> 
> Thanks,
> 
> Manuwela Kanade
> 
>

Re: Review Request 53047: MESOS-6212: name format validation for mesos managed docker containers.

2016-10-20 Thread Manuwela Kanade 


> On Oct. 20, 2016, 9:55 a.m., haosdent huang wrote:
> > src/tests/containerizer/docker_containerizer_tests.cpp, line 1501
> > 
> >
> > This test case would not go through `parse()` method. Since we only use 
> > state to construct the `Container` then put it in `containers_` directly 
> > which would bypass `parse`.
> > 
> > 
> > ```
> > // Create and store a container.
> > Container* container = new Container(containerId);
> > containers_[containerId] = container;
> > container->slaveId = state->id;
> > container->state = Container::RUNNING;
> > container->launchesExecutorContainer =
> >   executorContainers.contains(containerId);
> > ```

I will work on changing the testcase such that it goes through the parse() 
method. Will update soon


- Manuwela


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


On Oct. 20, 2016, 11:46 a.m., Manuwela Kanade wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53047/
> ---
> 
> (Updated Oct. 20, 2016, 11:46 a.m.)
> 
> 
> Review request for mesos, haosdent huang, Timothy Chen, and Vinod Kone.
> 
> 
> Bugs: MESOS-6212
> https://issues.apache.org/jira/browse/MESOS-6212
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-6212: name format validation for mesos managed docker containers.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/tests/uuid_tests.cpp 
> fe9894af93df9cb9b12390d8d7e7885525db384d 
>   src/slave/containerizer/docker.cpp 8ec4c0a25335fb1b36cb2ab82577f6d3e2f7f008 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 6d26797abf6d2b5e42b9e7743789e1edc62c9c1a 
> 
> Diff: https://reviews.apache.org/r/53047/diff/
> 
> 
> Testing
> ---
> 
> 1. Ran the "make check"
> 
> 2. Manual Testing done as below:
> Testing without changes: Created and ran an example container which is not 
> run using mesos : named "mesos-rsyslog" which got killed once mesos-agent 
> started with --containerizer=docker,mesos. 
> 
> Testing with changes: Created and ran an example container (not run using 
> mesos): named "mesos-rsyslog" which keeps running and mesos-agent does not 
> kill it thus avoiding the false positive
> 
> 
> Thanks,
> 
> Manuwela Kanade
> 
>

Re: Review Request 53047: MESOS-6212: name format validation for mesos managed docker containers.

2016-10-20 Thread Manuwela Kanade 


> On Oct. 20, 2016, 8:58 a.m., haosdent huang wrote:
> > src/slave/containerizer/docker.cpp, lines 124-130
> > 
> >
> > I think need to put this in an `else` block, otherwise it would be 
> > perform although we get `containerId` above.

Added the else block


- Manuwela


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


On Oct. 20, 2016, 11:46 a.m., Manuwela Kanade wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53047/
> ---
> 
> (Updated Oct. 20, 2016, 11:46 a.m.)
> 
> 
> Review request for mesos, haosdent huang, Timothy Chen, and Vinod Kone.
> 
> 
> Bugs: MESOS-6212
> https://issues.apache.org/jira/browse/MESOS-6212
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-6212: name format validation for mesos managed docker containers.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/tests/uuid_tests.cpp 
> fe9894af93df9cb9b12390d8d7e7885525db384d 
>   src/slave/containerizer/docker.cpp 8ec4c0a25335fb1b36cb2ab82577f6d3e2f7f008 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 6d26797abf6d2b5e42b9e7743789e1edc62c9c1a 
> 
> Diff: https://reviews.apache.org/r/53047/diff/
> 
> 
> Testing
> ---
> 
> 1. Ran the "make check"
> 
> 2. Manual Testing done as below:
> Testing without changes: Created and ran an example container which is not 
> run using mesos : named "mesos-rsyslog" which got killed once mesos-agent 
> started with --containerizer=docker,mesos. 
> 
> Testing with changes: Created and ran an example container (not run using 
> mesos): named "mesos-rsyslog" which keeps running and mesos-agent does not 
> kill it thus avoiding the false positive
> 
> 
> Thanks,
> 
> Manuwela Kanade
> 
>

Re: Review Request 53047: MESOS-6212: name format validation for mesos managed docker containers.

2016-10-20 Thread Manuwela Kanade 

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

(Updated Oct. 20, 2016, 11:46 a.m.)


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


Summary (updated)
-

MESOS-6212: name format validation for mesos managed docker containers.


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


Repository: mesos


Description (updated)
---

MESOS-6212: name format validation for mesos managed docker containers.


Diffs (updated)
-

  3rdparty/stout/tests/uuid_tests.cpp fe9894af93df9cb9b12390d8d7e7885525db384d 
  src/slave/containerizer/docker.cpp 8ec4c0a25335fb1b36cb2ab82577f6d3e2f7f008 
  src/tests/containerizer/docker_containerizer_tests.cpp 
6d26797abf6d2b5e42b9e7743789e1edc62c9c1a 

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


Testing
---

1. Ran the "make check"

2. Manual Testing done as below:
Testing without changes: Created and ran an example container which is not run 
using mesos : named "mesos-rsyslog" which got killed once mesos-agent started 
with --containerizer=docker,mesos. 

Testing with changes: Created and ran an example container (not run using 
mesos): named "mesos-rsyslog" which keeps running and mesos-agent does not kill 
it thus avoiding the false positive


Thanks,

Manuwela Kanade

Re: Review Request 53047: MESOS-6212: name format validation for mesos managed docker containers

2016-10-20 Thread haosdent huang 

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




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


This test case would not go through `parse()` method. Since we only use 
state to construct the `Container` then put it in `containers_` directly which 
would bypass `parse`.

```
// Create and store a container.
Container* container = new Container(containerId);
containers_[containerId] = container;
container->slaveId = state->id;
container->state = Container::RUNNING;
container->launchesExecutorContainer =
  executorContainers.contains(containerId);
```


- haosdent huang


On Oct. 20, 2016, 7:24 a.m., Manuwela Kanade wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53047/
> ---
> 
> (Updated Oct. 20, 2016, 7:24 a.m.)
> 
> 
> Review request for mesos, haosdent huang, Timothy Chen, and Vinod Kone.
> 
> 
> Bugs: MESOS-6212
> https://issues.apache.org/jira/browse/MESOS-6212
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-6212: name format validation for mesos managed docker containers
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/tests/uuid_tests.cpp 
> fe9894af93df9cb9b12390d8d7e7885525db384d 
>   src/slave/containerizer/docker.cpp 8ec4c0a25335fb1b36cb2ab82577f6d3e2f7f008 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 6d26797abf6d2b5e42b9e7743789e1edc62c9c1a 
> 
> Diff: https://reviews.apache.org/r/53047/diff/
> 
> 
> Testing
> ---
> 
> 1. Ran the "make check"
> 
> 2. Manual Testing done as below:
> Testing without changes: Created and ran an example container which is not 
> run using mesos : named "mesos-rsyslog" which got killed once mesos-agent 
> started with --containerizer=docker,mesos. 
> 
> Testing with changes: Created and ran an example container (not run using 
> mesos): named "mesos-rsyslog" which keeps running and mesos-agent does not 
> kill it thus avoiding the false positive
> 
> 
> Thanks,
> 
> Manuwela Kanade
> 
>

Re: Review Request 53047: MESOS-6212: name format validation for mesos managed docker containers

2016-10-20 Thread haosdent huang 

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




src/slave/containerizer/docker.cpp (lines 124 - 129)


I think need to put this in an `else` block, otherwise it would be perform 
although we get `containerId` above.


- haosdent huang


On Oct. 20, 2016, 7:24 a.m., Manuwela Kanade wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53047/
> ---
> 
> (Updated Oct. 20, 2016, 7:24 a.m.)
> 
> 
> Review request for mesos, haosdent huang, Timothy Chen, and Vinod Kone.
> 
> 
> Bugs: MESOS-6212
> https://issues.apache.org/jira/browse/MESOS-6212
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-6212: name format validation for mesos managed docker containers
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/tests/uuid_tests.cpp 
> fe9894af93df9cb9b12390d8d7e7885525db384d 
>   src/slave/containerizer/docker.cpp 8ec4c0a25335fb1b36cb2ab82577f6d3e2f7f008 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 6d26797abf6d2b5e42b9e7743789e1edc62c9c1a 
> 
> Diff: https://reviews.apache.org/r/53047/diff/
> 
> 
> Testing
> ---
> 
> 1. Ran the "make check"
> 
> 2. Manual Testing done as below:
> Testing without changes: Created and ran an example container which is not 
> run using mesos : named "mesos-rsyslog" which got killed once mesos-agent 
> started with --containerizer=docker,mesos. 
> 
> Testing with changes: Created and ran an example container (not run using 
> mesos): named "mesos-rsyslog" which keeps running and mesos-agent does not 
> kill it thus avoiding the false positive
> 
> 
> Thanks,
> 
> Manuwela Kanade
> 
>

Review Request 53047: MESOS-6212: name format validation for mesos managed docker containers

2016-10-20 Thread Manuwela Kanade 

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

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


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


Repository: mesos


Description
---

MESOS-6212: name format validation for mesos managed docker containers


Diffs
-

  3rdparty/stout/tests/uuid_tests.cpp fe9894af93df9cb9b12390d8d7e7885525db384d 
  src/slave/containerizer/docker.cpp 8ec4c0a25335fb1b36cb2ab82577f6d3e2f7f008 
  src/tests/containerizer/docker_containerizer_tests.cpp 
6d26797abf6d2b5e42b9e7743789e1edc62c9c1a 

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


Testing
---

1. Ran the "make check"

2. Manual Testing done as below:
Testing without changes: Created and ran an example container which is not run 
using mesos : named "mesos-rsyslog" which got killed once mesos-agent started 
with --containerizer=docker,mesos. 

Testing with changes: Created and ran an example container (not run using 
mesos): named "mesos-rsyslog" which keeps running and mesos-agent does not kill 
it thus avoiding the false positive


Thanks,

Manuwela Kanade