You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Vinod Kone <vi...@gmail.com> on 2016/12/13 22:20:36 UTC
Review Request 54720: Added a test that verifies container attach
after agent restart.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54720/
-----------------------------------------------------------
Review request for mesos, Anand Mazumdar and Kevin Klues.
Repository: mesos
Description
-------
Added a test that verifies container attach after agent restart.
Diffs
-----
src/tests/containerizer/io_switchboard_tests.cpp 5bd9dc4a776fe8a0d04ed31aa167f10c394984e6
Diff: https://reviews.apache.org/r/54720/diff/
Testing
-------
make -j20 check GTEST_FILTER="*Switchboard*" MESOS_VERBOSE=1
Thanks,
Vinod Kone
Re: Review Request 54720: Added a test that verifies container attach
after agent restart.
Posted by Vinod Kone <vi...@gmail.com>.
> On Dec. 14, 2016, 1:10 a.m., Kevin Klues wrote:
> > src/tests/containerizer/io_switchboard_tests.cpp, line 809
> > <https://reviews.apache.org/r/54720/diff/1/?file=1583056#file1583056line809>
> >
> > I think you need the new:
> > ```
> > #ifdef __linux__
> > flags.agent_subsystems = None();
> > #endif
> > ```
> >
> > here.
Oh, I just copy pasted from another test in the file. Is this being introduced in a different review?
> On Dec. 14, 2016, 1:10 a.m., Kevin Klues wrote:
> > src/tests/containerizer/io_switchboard_tests.cpp, line 886
> > <https://reviews.apache.org/r/54720/diff/1/?file=1583056#file1583056line886>
> >
> > Wait until slvae recovered? Your future is on `slave::Slave::_recover`
`_recover` execution only guarantees that containerizer is recovered. there are more steps for slave recovery to complete, which i didn't care for in this test.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54720/#review159085
-----------------------------------------------------------
On Dec. 13, 2016, 10:20 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54720/
> -----------------------------------------------------------
>
> (Updated Dec. 13, 2016, 10:20 p.m.)
>
>
> Review request for mesos, Anand Mazumdar and Kevin Klues.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a test that verifies container attach after agent restart.
>
>
> Diffs
> -----
>
> src/tests/containerizer/io_switchboard_tests.cpp 5bd9dc4a776fe8a0d04ed31aa167f10c394984e6
>
> Diff: https://reviews.apache.org/r/54720/diff/
>
>
> Testing
> -------
>
> make -j20 check GTEST_FILTER="*Switchboard*" MESOS_VERBOSE=1
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 54720: Added a test that verifies container attach
after agent restart.
Posted by Kevin Klues <kl...@gmail.com>.
> On Dec. 14, 2016, 1:10 a.m., Kevin Klues wrote:
> > src/tests/containerizer/io_switchboard_tests.cpp, line 809
> > <https://reviews.apache.org/r/54720/diff/1/?file=1583056#file1583056line809>
> >
> > I think you need the new:
> > ```
> > #ifdef __linux__
> > flags.agent_subsystems = None();
> > #endif
> > ```
> >
> > here.
>
> Vinod Kone wrote:
> Oh, I just copy pasted from another test in the file. Is this being introduced in a different review?
It was probably introduced between when you first copied the test over and when you pushed the review. All of the switchboard realted tests need this if they use the posix launcher.
- Kevin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54720/#review159085
-----------------------------------------------------------
On Dec. 13, 2016, 10:20 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54720/
> -----------------------------------------------------------
>
> (Updated Dec. 13, 2016, 10:20 p.m.)
>
>
> Review request for mesos, Anand Mazumdar and Kevin Klues.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a test that verifies container attach after agent restart.
>
>
> Diffs
> -----
>
> src/tests/containerizer/io_switchboard_tests.cpp 5bd9dc4a776fe8a0d04ed31aa167f10c394984e6
>
> Diff: https://reviews.apache.org/r/54720/diff/
>
>
> Testing
> -------
>
> make -j20 check GTEST_FILTER="*Switchboard*" MESOS_VERBOSE=1
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 54720: Added a test that verifies container attach
after agent restart.
Posted by Kevin Klues <kl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54720/#review159085
-----------------------------------------------------------
Fix it, then Ship it!
src/tests/containerizer/io_switchboard_tests.cpp (line 809)
<https://reviews.apache.org/r/54720/#comment230039>
I think you need the new:
```
#ifdef __linux__
flags.agent_subsystems = None();
#endif
```
here.
src/tests/containerizer/io_switchboard_tests.cpp (line 886)
<https://reviews.apache.org/r/54720/#comment230040>
Wait until slvae recovered? Your future is on `slave::Slave::_recover`
- Kevin Klues
On Dec. 13, 2016, 10:20 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54720/
> -----------------------------------------------------------
>
> (Updated Dec. 13, 2016, 10:20 p.m.)
>
>
> Review request for mesos, Anand Mazumdar and Kevin Klues.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a test that verifies container attach after agent restart.
>
>
> Diffs
> -----
>
> src/tests/containerizer/io_switchboard_tests.cpp 5bd9dc4a776fe8a0d04ed31aa167f10c394984e6
>
> Diff: https://reviews.apache.org/r/54720/diff/
>
>
> Testing
> -------
>
> make -j20 check GTEST_FILTER="*Switchboard*" MESOS_VERBOSE=1
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 54720: Added a test that verifies container attach
after agent restart.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54720/
-----------------------------------------------------------
(Updated Dec. 16, 2016, 6:21 p.m.)
Review request for mesos, Anand Mazumdar and Kevin Klues.
Changes
-------
addressed comments. NNFR.
Repository: mesos
Description
-------
Added a test that verifies container attach after agent restart.
Diffs (updated)
-----
src/tests/containerizer/io_switchboard_tests.cpp 5e3fb0d0003878a375bc36b2c9563e16f6ad2e2f
Diff: https://reviews.apache.org/r/54720/diff/
Testing
-------
make -j20 check GTEST_FILTER="*Switchboard*" MESOS_VERBOSE=1
Thanks,
Vinod Kone
Re: Review Request 54720: Added a test that verifies container attach
after agent restart.
Posted by Vinod Kone <vi...@gmail.com>.
> On Dec. 14, 2016, 1:34 a.m., Anand Mazumdar wrote:
> > src/tests/containerizer/io_switchboard_tests.cpp, lines 896-897
> > <https://reviews.apache.org/r/54720/diff/1/?file=1583056#file1583056line896>
> >
> > hmm, is this enough verification to ensure that a container can be attached after an agent restart? IIUC, this just validates that we are able to establish a connection. What if a subsequent request fails with a non `OK()` response?
Yes, this test verifes that `IOSwitchboardTest.Attach` test works even after agent restart. Just like that test this one doesn't send actual requests, just tests connection.
Of course, it is still valuable to write tests that send speific `ATTACH_*` calls and verify them but those to me are different tests. I will get to them after :)
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54720/#review159090
-----------------------------------------------------------
On Dec. 13, 2016, 10:20 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54720/
> -----------------------------------------------------------
>
> (Updated Dec. 13, 2016, 10:20 p.m.)
>
>
> Review request for mesos, Anand Mazumdar and Kevin Klues.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a test that verifies container attach after agent restart.
>
>
> Diffs
> -----
>
> src/tests/containerizer/io_switchboard_tests.cpp 5bd9dc4a776fe8a0d04ed31aa167f10c394984e6
>
> Diff: https://reviews.apache.org/r/54720/diff/
>
>
> Testing
> -------
>
> make -j20 check GTEST_FILTER="*Switchboard*" MESOS_VERBOSE=1
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 54720: Added a test that verifies container attach
after agent restart.
Posted by Kevin Klues <kl...@gmail.com>.
> On Dec. 14, 2016, 1:34 a.m., Anand Mazumdar wrote:
> > src/tests/containerizer/io_switchboard_tests.cpp, lines 896-897
> > <https://reviews.apache.org/r/54720/diff/1/?file=1583056#file1583056line896>
> >
> > hmm, is this enough verification to ensure that a container can be attached after an agent restart? IIUC, this just validates that we are able to establish a connection. What if a subsequent request fails with a non `OK()` response?
>
> Vinod Kone wrote:
> Yes, this test verifes that `IOSwitchboardTest.Attach` test works even after agent restart. Just like that test this one doesn't send actual requests, just tests connection.
>
> Of course, it is still valuable to write tests that send speific `ATTACH_*` calls and verify them but those to me are different tests. I will get to them after :)
>
> Anand Mazumdar wrote:
> gotcha, Dropping the issue.
Yeah, it's an `Attach` test, not an `AttachInput` test.
- Kevin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54720/#review159090
-----------------------------------------------------------
On Dec. 13, 2016, 10:20 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54720/
> -----------------------------------------------------------
>
> (Updated Dec. 13, 2016, 10:20 p.m.)
>
>
> Review request for mesos, Anand Mazumdar and Kevin Klues.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a test that verifies container attach after agent restart.
>
>
> Diffs
> -----
>
> src/tests/containerizer/io_switchboard_tests.cpp 5bd9dc4a776fe8a0d04ed31aa167f10c394984e6
>
> Diff: https://reviews.apache.org/r/54720/diff/
>
>
> Testing
> -------
>
> make -j20 check GTEST_FILTER="*Switchboard*" MESOS_VERBOSE=1
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 54720: Added a test that verifies container attach
after agent restart.
Posted by Anand Mazumdar <an...@apache.org>.
> On Dec. 14, 2016, 1:34 a.m., Anand Mazumdar wrote:
> > src/tests/containerizer/io_switchboard_tests.cpp, lines 896-897
> > <https://reviews.apache.org/r/54720/diff/1/?file=1583056#file1583056line896>
> >
> > hmm, is this enough verification to ensure that a container can be attached after an agent restart? IIUC, this just validates that we are able to establish a connection. What if a subsequent request fails with a non `OK()` response?
>
> Vinod Kone wrote:
> Yes, this test verifes that `IOSwitchboardTest.Attach` test works even after agent restart. Just like that test this one doesn't send actual requests, just tests connection.
>
> Of course, it is still valuable to write tests that send speific `ATTACH_*` calls and verify them but those to me are different tests. I will get to them after :)
gotcha, Dropping the issue.
- Anand
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54720/#review159090
-----------------------------------------------------------
On Dec. 13, 2016, 10:20 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54720/
> -----------------------------------------------------------
>
> (Updated Dec. 13, 2016, 10:20 p.m.)
>
>
> Review request for mesos, Anand Mazumdar and Kevin Klues.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a test that verifies container attach after agent restart.
>
>
> Diffs
> -----
>
> src/tests/containerizer/io_switchboard_tests.cpp 5bd9dc4a776fe8a0d04ed31aa167f10c394984e6
>
> Diff: https://reviews.apache.org/r/54720/diff/
>
>
> Testing
> -------
>
> make -j20 check GTEST_FILTER="*Switchboard*" MESOS_VERBOSE=1
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 54720: Added a test that verifies container attach
after agent restart.
Posted by Anand Mazumdar <an...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54720/#review159090
-----------------------------------------------------------
Modulo Kevin's comment around adding the agent subsystem flag.
Similar to the earlier review, looks good minus a query around if establishing a connection is enough to ensure that the client can attach?
src/tests/containerizer/io_switchboard_tests.cpp (lines 814 - 817)
<https://reviews.apache.org/r/54720/#comment230051>
Would look less jagged if all arguments are on the same line similar to the `MesosSchedulerDriver()` a few lines later. I can understand why you did it in this way since the other tests in this file do it in this way.
src/tests/containerizer/io_switchboard_tests.cpp (line 891)
<https://reviews.apache.org/r/54720/#comment230052>
s/EXPECT_EQ/ASSERT_EQ
Might be needed for the other tests in this file too. Otherwise, it might crash on the next line.
src/tests/containerizer/io_switchboard_tests.cpp (lines 896 - 897)
<https://reviews.apache.org/r/54720/#comment230053>
hmm, is this enough verification to ensure that a container can be attached after an agent restart? IIUC, this just validates that we are able to establish a connection. What if a subsequent request fails with a non `OK()` response?
- Anand Mazumdar
On Dec. 13, 2016, 10:20 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54720/
> -----------------------------------------------------------
>
> (Updated Dec. 13, 2016, 10:20 p.m.)
>
>
> Review request for mesos, Anand Mazumdar and Kevin Klues.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a test that verifies container attach after agent restart.
>
>
> Diffs
> -----
>
> src/tests/containerizer/io_switchboard_tests.cpp 5bd9dc4a776fe8a0d04ed31aa167f10c394984e6
>
> Diff: https://reviews.apache.org/r/54720/diff/
>
>
> Testing
> -------
>
> make -j20 check GTEST_FILTER="*Switchboard*" MESOS_VERBOSE=1
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 54720: Added a test that verifies container attach
after agent restart.
Posted by Anand Mazumdar <an...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54720/#review159099
-----------------------------------------------------------
Ship it!
Ship It!
- Anand Mazumdar
On Dec. 13, 2016, 10:20 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54720/
> -----------------------------------------------------------
>
> (Updated Dec. 13, 2016, 10:20 p.m.)
>
>
> Review request for mesos, Anand Mazumdar and Kevin Klues.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a test that verifies container attach after agent restart.
>
>
> Diffs
> -----
>
> src/tests/containerizer/io_switchboard_tests.cpp 5bd9dc4a776fe8a0d04ed31aa167f10c394984e6
>
> Diff: https://reviews.apache.org/r/54720/diff/
>
>
> Testing
> -------
>
> make -j20 check GTEST_FILTER="*Switchboard*" MESOS_VERBOSE=1
>
>
> Thanks,
>
> Vinod Kone
>
>