You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Joseph Wu <jo...@mesosphere.io> on 2019/08/19 21:57:59 UTC
Review Request 71316: Added draining tests for empty agents.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71316/
-----------------------------------------------------------
Review request for mesos, Benno Evers and Greg Mann.
Bugs: MESOS-9892
https://issues.apache.org/jira/browse/MESOS-9892
Repository: mesos
Description
-------
This splits the existing agent draining tests into two variants:
1) where the agent has nothing running, and
2) where the agent has one task running.
Diffs
-----
src/tests/master_draining_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/71316/diff/1/
Testing
-------
make check
Thanks,
Joseph Wu
Re: Review Request 71316: Added draining tests for empty agents.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71316/#review217436
-----------------------------------------------------------
Ship it!
Ship It!
- Greg Mann
On Aug. 23, 2019, 7:22 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71316/
> -----------------------------------------------------------
>
> (Updated Aug. 23, 2019, 7:22 p.m.)
>
>
> Review request for mesos, Benno Evers and Greg Mann.
>
>
> Bugs: MESOS-9892
> https://issues.apache.org/jira/browse/MESOS-9892
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This splits the existing agent draining tests into two variants:
> 1) where the agent has nothing running, and
> 2) where the agent has one task running.
>
>
> Diffs
> -----
>
> src/tests/master_draining_tests.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/71316/diff/3/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 71316: Added draining tests for empty agents.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71316/
-----------------------------------------------------------
(Updated Aug. 23, 2019, 12:22 p.m.)
Review request for mesos, Benno Evers and Greg Mann.
Changes
-------
Flipped the order of some `EXPECT_EQ` statements since the type inference causes a warning.
Bugs: MESOS-9892
https://issues.apache.org/jira/browse/MESOS-9892
Repository: mesos
Description
-------
This splits the existing agent draining tests into two variants:
1) where the agent has nothing running, and
2) where the agent has one task running.
Diffs (updated)
-----
src/tests/master_draining_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/71316/diff/3/
Changes: https://reviews.apache.org/r/71316/diff/2-3/
Testing
-------
make check
Thanks,
Joseph Wu
Re: Review Request 71316: Added draining tests for empty agents.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71316/
-----------------------------------------------------------
(Updated Aug. 22, 2019, 11:34 a.m.)
Review request for mesos, Benno Evers and Greg Mann.
Changes
-------
* Increased grace period argument for normal draining test.
* Removed two (somewhat redundant) state checks in the normal draining tests.
* Added verification of unreachability in one test.
* Added verification of drained state in one test.
Bugs: MESOS-9892
https://issues.apache.org/jira/browse/MESOS-9892
Repository: mesos
Description
-------
This splits the existing agent draining tests into two variants:
1) where the agent has nothing running, and
2) where the agent has one task running.
Diffs (updated)
-----
src/tests/master_draining_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/71316/diff/2/
Changes: https://reviews.apache.org/r/71316/diff/1-2/
Testing
-------
make check
Thanks,
Joseph Wu
Re: Review Request 71316: Added draining tests for empty agents.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On Aug. 21, 2019, 2:28 p.m., Greg Mann wrote:
> > src/tests/master_draining_tests.cpp
> > Lines 178 (patched)
> > <https://reviews.apache.org/r/71316/diff/1/?file=2161830#file2161830line178>
> >
> > In this case, we probably want to either not set the max grace period, or set it to something long, since we're trying to verify that an agent with nothing running will immediately be marked drained. While the agent won't be _immediately_ marked gone if a task is running and the max grace period is set to zero, it would still happen very quickly.
The test techically isn't checking for this grace period. But it doesn't hurt to have a non-zero value here.
Increased to 10 seconds.
> On Aug. 21, 2019, 2:28 p.m., Greg Mann wrote:
> > src/tests/master_draining_tests.cpp
> > Lines 222-250 (patched)
> > <https://reviews.apache.org/r/71316/diff/1/?file=2161830#file2161830line222>
> >
> > We probably don't need to verify the state output of all 3 endpoints in both versions of this test, I think just one will do. WDYT?
That makes sense. I kept the GET_AGENTS one.
> On Aug. 21, 2019, 2:28 p.m., Greg Mann wrote:
> > src/tests/master_draining_tests.cpp
> > Lines 313-325 (patched)
> > <https://reviews.apache.org/r/71316/diff/1/?file=2161830#file2161830line313>
> >
> > Can we do something to verify that the agent has been marked unreachable here? Either specifying the type of the registry operation in the EXPECT_CALL, or inspecting some API output afterward?
I changed the mock to return the operation, which can be type-casted to the appropriate operation subclass (MarkSlaveUnreachable).
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71316/#review217362
-----------------------------------------------------------
On Aug. 22, 2019, 11:34 a.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71316/
> -----------------------------------------------------------
>
> (Updated Aug. 22, 2019, 11:34 a.m.)
>
>
> Review request for mesos, Benno Evers and Greg Mann.
>
>
> Bugs: MESOS-9892
> https://issues.apache.org/jira/browse/MESOS-9892
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This splits the existing agent draining tests into two variants:
> 1) where the agent has nothing running, and
> 2) where the agent has one task running.
>
>
> Diffs
> -----
>
> src/tests/master_draining_tests.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/71316/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 71316: Added draining tests for empty agents.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71316/#review217362
-----------------------------------------------------------
src/tests/master_draining_tests.cpp
Lines 178 (patched)
<https://reviews.apache.org/r/71316/#comment304652>
In this case, we probably want to either not set the max grace period, or set it to something long, since we're trying to verify that an agent with nothing running will immediately be marked drained. While the agent won't be _immediately_ marked gone if a task is running and the max grace period is set to zero, it would still happen very quickly.
src/tests/master_draining_tests.cpp
Lines 222-250 (patched)
<https://reviews.apache.org/r/71316/#comment304653>
We probably don't need to verify the state output of all 3 endpoints in both versions of this test, I think just one will do. WDYT?
src/tests/master_draining_tests.cpp
Lines 281 (patched)
<https://reviews.apache.org/r/71316/#comment304654>
This test is so short now :heart_eyes:
src/tests/master_draining_tests.cpp
Lines 313-325 (patched)
<https://reviews.apache.org/r/71316/#comment304656>
Can we do something to verify that the agent has been marked unreachable here? Either specifying the type of the registry operation in the EXPECT_CALL, or inspecting some API output afterward?
src/tests/master_draining_tests.cpp
Lines 360-361 (patched)
<https://reviews.apache.org/r/71316/#comment304658>
Can we also verify via master API output that the agent is DRAINED after it reregisters?
- Greg Mann
On Aug. 19, 2019, 9:57 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71316/
> -----------------------------------------------------------
>
> (Updated Aug. 19, 2019, 9:57 p.m.)
>
>
> Review request for mesos, Benno Evers and Greg Mann.
>
>
> Bugs: MESOS-9892
> https://issues.apache.org/jira/browse/MESOS-9892
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This splits the existing agent draining tests into two variants:
> 1) where the agent has nothing running, and
> 2) where the agent has one task running.
>
>
> Diffs
> -----
>
> src/tests/master_draining_tests.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/71316/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Joseph Wu
>
>