You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Qian Zhang <zh...@gmail.com> on 2017/07/11 09:03:43 UTC
Review Request 60761: Added a test
`DockerContainerizerTest.ROOT_DOCKER_DefaultDNS`.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60761/
-----------------------------------------------------------
Review request for mesos, Avinash sridharan and Jie Yu.
Bugs: MESOS-7709
https://issues.apache.org/jira/browse/MESOS-7709
Repository: mesos
Description
-------
Added a test `DockerContainerizerTest.ROOT_DOCKER_DefaultDNS`.
Diffs
-----
src/tests/containerizer/docker_containerizer_tests.cpp 1e85a79f812399270575ea4a64db10e72f40e648
Diff: https://reviews.apache.org/r/60761/diff/1/
Testing
-------
sudo make check
Thanks,
Qian Zhang
Re: Review Request 60761: Added a test
`DockerContainerizerTest.ROOT_DOCKER_DefaultDNS`.
Posted by Qian Zhang <zh...@gmail.com>.
> On July 28, 2017, 8:35 a.m., Avinash sridharan wrote:
> > src/tests/containerizer/docker_containerizer_tests.cpp
> > Lines 4357 (patched)
> > <https://reviews.apache.org/r/60761/diff/3/?file=1782044#file1782044line4357>
> >
> > Can we have a test for checking validation error on setting up the HOST MODE?
>
> Qian Zhang wrote:
> Did you mean adding a test to test the validation code in the lambda that we added in `src/slave/flags.cpp` for the `--default_container_dns` flag? If so, then I think `docker_containerizer_tests.cpp` may not be a good place for such test because `docker_containerizer_tests.cpp` should contain the end-to-end tests, but for testing the validation code we do not even need to start the agent. Maybe just add a test in `flags_tests.cpp`? And in another hand, for the other agent flags which has the validation code in lambda, it seems there is no related test for them, so do we really need such test for testing the validation code of `--default_container_dns`?
>
> BTW, I was thinking to add a test for wildcard match of Docker user-defined network, but the problem is we can not assume any user-defined networks in the test env, but there must be a bridge network, so currently I only have the test for the bridge network.
>
> Avinash sridharan wrote:
> Agreed !! if we can add the flag validation tests for the flags tests that should be good enough. Wildcard match test for BRIDGE mode is good enough I think.
I added a test https://reviews.apache.org/r/61219/ for the flag validation.
- Qian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60761/#review181628
-----------------------------------------------------------
On July 25, 2017, 2:07 p.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60761/
> -----------------------------------------------------------
>
> (Updated July 25, 2017, 2:07 p.m.)
>
>
> Review request for mesos, Avinash sridharan and Jie Yu.
>
>
> Bugs: MESOS-7709
> https://issues.apache.org/jira/browse/MESOS-7709
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a test `DockerContainerizerTest.ROOT_DOCKER_DefaultDNS`.
>
>
> Diffs
> -----
>
> src/tests/containerizer/docker_containerizer_tests.cpp 1e85a79f812399270575ea4a64db10e72f40e648
>
>
> Diff: https://reviews.apache.org/r/60761/diff/3/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 60761: Added a test
`DockerContainerizerTest.ROOT_DOCKER_DefaultDNS`.
Posted by Avinash sridharan <av...@mesosphere.io>.
> On July 28, 2017, 12:35 a.m., Avinash sridharan wrote:
> > src/tests/containerizer/docker_containerizer_tests.cpp
> > Lines 4357 (patched)
> > <https://reviews.apache.org/r/60761/diff/3/?file=1782044#file1782044line4357>
> >
> > Can we have a test for checking validation error on setting up the HOST MODE?
>
> Qian Zhang wrote:
> Did you mean adding a test to test the validation code in the lambda that we added in `src/slave/flags.cpp` for the `--default_container_dns` flag? If so, then I think `docker_containerizer_tests.cpp` may not be a good place for such test because `docker_containerizer_tests.cpp` should contain the end-to-end tests, but for testing the validation code we do not even need to start the agent. Maybe just add a test in `flags_tests.cpp`? And in another hand, for the other agent flags which has the validation code in lambda, it seems there is no related test for them, so do we really need such test for testing the validation code of `--default_container_dns`?
>
> BTW, I was thinking to add a test for wildcard match of Docker user-defined network, but the problem is we can not assume any user-defined networks in the test env, but there must be a bridge network, so currently I only have the test for the bridge network.
Agreed !! if we can add the flag validation tests for the flags tests that should be good enough. Wildcard match test for BRIDGE mode is good enough I think.
- Avinash
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60761/#review181628
-----------------------------------------------------------
On July 25, 2017, 6:07 a.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60761/
> -----------------------------------------------------------
>
> (Updated July 25, 2017, 6:07 a.m.)
>
>
> Review request for mesos, Avinash sridharan and Jie Yu.
>
>
> Bugs: MESOS-7709
> https://issues.apache.org/jira/browse/MESOS-7709
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a test `DockerContainerizerTest.ROOT_DOCKER_DefaultDNS`.
>
>
> Diffs
> -----
>
> src/tests/containerizer/docker_containerizer_tests.cpp 1e85a79f812399270575ea4a64db10e72f40e648
>
>
> Diff: https://reviews.apache.org/r/60761/diff/3/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 60761: Added a test
`DockerContainerizerTest.ROOT_DOCKER_DefaultDNS`.
Posted by Qian Zhang <zh...@gmail.com>.
> On July 28, 2017, 8:35 a.m., Avinash sridharan wrote:
> > src/tests/containerizer/docker_containerizer_tests.cpp
> > Lines 4357 (patched)
> > <https://reviews.apache.org/r/60761/diff/3/?file=1782044#file1782044line4357>
> >
> > Can we have a test for checking validation error on setting up the HOST MODE?
>
> Qian Zhang wrote:
> Did you mean adding a test to test the validation code in the lambda that we added in `src/slave/flags.cpp` for the `--default_container_dns` flag? If so, then I think `docker_containerizer_tests.cpp` may not be a good place for such test because `docker_containerizer_tests.cpp` should contain the end-to-end tests, but for testing the validation code we do not even need to start the agent. Maybe just add a test in `flags_tests.cpp`? And in another hand, for the other agent flags which has the validation code in lambda, it seems there is no related test for them, so do we really need such test for testing the validation code of `--default_container_dns`?
>
> BTW, I was thinking to add a test for wildcard match of Docker user-defined network, but the problem is we can not assume any user-defined networks in the test env, but there must be a bridge network, so currently I only have the test for the bridge network.
>
> Avinash sridharan wrote:
> Agreed !! if we can add the flag validation tests for the flags tests that should be good enough. Wildcard match test for BRIDGE mode is good enough I think.
>
> Qian Zhang wrote:
> I added a test https://reviews.apache.org/r/61219/ for the flag validation.
Sorry, the patch for the flag validation test should be: https://reviews.apache.org/r/61245/.
- Qian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60761/#review181628
-----------------------------------------------------------
On July 25, 2017, 2:07 p.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60761/
> -----------------------------------------------------------
>
> (Updated July 25, 2017, 2:07 p.m.)
>
>
> Review request for mesos, Avinash sridharan and Jie Yu.
>
>
> Bugs: MESOS-7709
> https://issues.apache.org/jira/browse/MESOS-7709
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a test `DockerContainerizerTest.ROOT_DOCKER_DefaultDNS`.
>
>
> Diffs
> -----
>
> src/tests/containerizer/docker_containerizer_tests.cpp 1e85a79f812399270575ea4a64db10e72f40e648
>
>
> Diff: https://reviews.apache.org/r/60761/diff/3/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 60761: Added a test
`DockerContainerizerTest.ROOT_DOCKER_DefaultDNS`.
Posted by Qian Zhang <zh...@gmail.com>.
> On July 28, 2017, 8:35 a.m., Avinash sridharan wrote:
> > src/tests/containerizer/docker_containerizer_tests.cpp
> > Lines 4357 (patched)
> > <https://reviews.apache.org/r/60761/diff/3/?file=1782044#file1782044line4357>
> >
> > Can we have a test for checking validation error on setting up the HOST MODE?
Did you mean adding a test to test the validation code in the lambda that we added in `src/slave/flags.cpp` for the `--default_container_dns` flag? If so, then I think `docker_containerizer_tests.cpp` may not be a good place for such test because `docker_containerizer_tests.cpp` should contain the end-to-end tests, but for testing the validation code we do not even need to start the agent. Maybe just add a test in `flags_tests.cpp`? And in another hand, for the other agent flags which has the validation code in lambda, it seems there is no related test for them, so do we really need such test for testing the validation code of `--default_container_dns`?
BTW, I was thinking to add a test for wildcard match of Docker user-defined network, but the problem is we can not assume any user-defined networks in the test env, but there must be a bridge network, so currently I only have the test for the bridge network.
- Qian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60761/#review181628
-----------------------------------------------------------
On July 25, 2017, 2:07 p.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60761/
> -----------------------------------------------------------
>
> (Updated July 25, 2017, 2:07 p.m.)
>
>
> Review request for mesos, Avinash sridharan and Jie Yu.
>
>
> Bugs: MESOS-7709
> https://issues.apache.org/jira/browse/MESOS-7709
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a test `DockerContainerizerTest.ROOT_DOCKER_DefaultDNS`.
>
>
> Diffs
> -----
>
> src/tests/containerizer/docker_containerizer_tests.cpp 1e85a79f812399270575ea4a64db10e72f40e648
>
>
> Diff: https://reviews.apache.org/r/60761/diff/3/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 60761: Added a test
`DockerContainerizerTest.ROOT_DOCKER_DefaultDNS`.
Posted by Avinash sridharan <av...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60761/#review181628
-----------------------------------------------------------
Fix it, then Ship it!
Ship It!
src/tests/containerizer/docker_containerizer_tests.cpp
Lines 4357 (patched)
<https://reviews.apache.org/r/60761/#comment257252>
Can we have a test for checking validation error on setting up the HOST MODE?
- Avinash sridharan
On July 25, 2017, 6:07 a.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60761/
> -----------------------------------------------------------
>
> (Updated July 25, 2017, 6:07 a.m.)
>
>
> Review request for mesos, Avinash sridharan and Jie Yu.
>
>
> Bugs: MESOS-7709
> https://issues.apache.org/jira/browse/MESOS-7709
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a test `DockerContainerizerTest.ROOT_DOCKER_DefaultDNS`.
>
>
> Diffs
> -----
>
> src/tests/containerizer/docker_containerizer_tests.cpp 1e85a79f812399270575ea4a64db10e72f40e648
>
>
> Diff: https://reviews.apache.org/r/60761/diff/3/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 60761: Added a test
`DockerContainerizerTest.ROOT_DOCKER_DefaultDNS`.
Posted by Avinash sridharan <av...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60761/#review181864
-----------------------------------------------------------
Ship it!
Ship It!
- Avinash sridharan
On July 25, 2017, 6:07 a.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60761/
> -----------------------------------------------------------
>
> (Updated July 25, 2017, 6:07 a.m.)
>
>
> Review request for mesos, Avinash sridharan and Jie Yu.
>
>
> Bugs: MESOS-7709
> https://issues.apache.org/jira/browse/MESOS-7709
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a test `DockerContainerizerTest.ROOT_DOCKER_DefaultDNS`.
>
>
> Diffs
> -----
>
> src/tests/containerizer/docker_containerizer_tests.cpp 1e85a79f812399270575ea4a64db10e72f40e648
>
>
> Diff: https://reviews.apache.org/r/60761/diff/3/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 60761: Added a test
`DockerContainerizerTest.ROOT_DOCKER_DefaultDNS`.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60761/
-----------------------------------------------------------
(Updated July 25, 2017, 2:07 p.m.)
Review request for mesos, Avinash sridharan and Jie Yu.
Changes
-------
Rebased.
Bugs: MESOS-7709
https://issues.apache.org/jira/browse/MESOS-7709
Repository: mesos
Description
-------
Added a test `DockerContainerizerTest.ROOT_DOCKER_DefaultDNS`.
Diffs (updated)
-----
src/tests/containerizer/docker_containerizer_tests.cpp 1e85a79f812399270575ea4a64db10e72f40e648
Diff: https://reviews.apache.org/r/60761/diff/3/
Changes: https://reviews.apache.org/r/60761/diff/2-3/
Testing
-------
sudo make check
Thanks,
Qian Zhang
Re: Review Request 60761: Added a test
`DockerContainerizerTest.ROOT_DOCKER_DefaultDNS`.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60761/
-----------------------------------------------------------
(Updated July 24, 2017, 4:03 p.m.)
Review request for mesos, Avinash sridharan and Jie Yu.
Changes
-------
Rebased.
Bugs: MESOS-7709
https://issues.apache.org/jira/browse/MESOS-7709
Repository: mesos
Description
-------
Added a test `DockerContainerizerTest.ROOT_DOCKER_DefaultDNS`.
Diffs (updated)
-----
src/tests/containerizer/docker_containerizer_tests.cpp 1e85a79f812399270575ea4a64db10e72f40e648
Diff: https://reviews.apache.org/r/60761/diff/2/
Changes: https://reviews.apache.org/r/60761/diff/1-2/
Testing
-------
sudo make check
Thanks,
Qian Zhang