mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Meng Zhu <m...@mesosphere.io>
Subject Re: Review Request 68354: Added a test to verify agent authentication retry backoff logic.
Date Thu, 16 Aug 2018 00:07:13 GMT


> On Aug. 15, 2018, 3:45 p.m., Benjamin Mahler wrote:
> > src/tests/authentication_tests.cpp
> > Lines 411-425 (patched)
> > <https://reviews.apache.org/r/68354/diff/2/?file=2073117#file2073117line411>
> >
> >     Can we push down the start into the loop (only for the 1st iteration) and avoid
the 1st special casing here? It also seems a little odd to be doing manual calling into the
function, can we also use the default mock behavior of calling through to avoid the manual
calling?

The first authentication is different from retires. Only advancing the clock by `authentication_backoff_factor`
will trigger the authentication. While retry is guaranteed to not happen before `authentication_timeout_min`.
There may be some way to push the logic to the loop, but I feel the current code is more readable.
We are testing retry specifically, excluding the first non-retry authentication would make
it more clear. Dropping, feel free to reopen if you think otherwise.

As for manual calling, I added some verbose comments. Hopefully, it will make it more understandable.


> On Aug. 15, 2018, 3:45 p.m., Benjamin Mahler wrote:
> > src/tests/authentication_tests.cpp
> > Lines 412-413 (patched)
> > <https://reviews.apache.org/r/68354/diff/2/?file=2073117#file2073117line412>
> >
> >     Why are the underscores needed? It's confusing since `_authenticate` is also
the continuation of `authenticate`

Done.


> On Aug. 15, 2018, 3:45 p.m., Benjamin Mahler wrote:
> > src/tests/authentication_tests.cpp
> > Lines 419 (patched)
> > <https://reviews.apache.org/r/68354/diff/2/?file=2073117#file2073117line419>
> >
> >     Do you actually still need this retirement on all of these?

Expectaions are sticky, if I do not retire it here. It will oversaturate (since `authenticate()`
will be called more times later) and fail.


> On Aug. 15, 2018, 3:45 p.m., Benjamin Mahler wrote:
> > src/tests/authentication_tests.cpp
> > Lines 427 (patched)
> > <https://reviews.apache.org/r/68354/diff/2/?file=2073117#file2073117line427>
> >
> >     Why 10 here? Does it take that many to reach the max? Should probably explain
to the reader the math on why 10?
> >     
> >     Maybe it's easier if this is just a while loop until we hit max or see two maxes?

Sounds good.


> On Aug. 15, 2018, 3:45 p.m., Benjamin Mahler wrote:
> > src/tests/authentication_tests.cpp
> > Lines 443-444 (patched)
> > <https://reviews.apache.org/r/68354/diff/2/?file=2073117#file2073117line443>
> >
> >     Maybe use milliseconds here? Not sure if this level of timer precision is supported
on windows, etc.

Done.


> On Aug. 15, 2018, 3:45 p.m., Benjamin Mahler wrote:
> > src/tests/authentication_tests.cpp
> > Lines 462-463 (patched)
> > <https://reviews.apache.org/r/68354/diff/2/?file=2073117#file2073117line462>
> >
> >     Hm.. can't we test that max follows `min+b*2^n`?

Done.


> On Aug. 15, 2018, 3:45 p.m., Benjamin Mahler wrote:
> > src/tests/authentication_tests.cpp
> > Lines 468-471 (patched)
> > <https://reviews.apache.org/r/68354/diff/2/?file=2073117#file2073117line468>
> >
> >     I guess there's no delay between authentication success and registration?
> >     
> >     Doing the advance before the FUTURE_PROTOBUF is racy, it might miss the message
and cause a flaky test.

Done.


- Meng


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


On Aug. 15, 2018, 5:06 p.m., Meng Zhu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68354/
> -----------------------------------------------------------
> 
> (Updated Aug. 15, 2018, 5:06 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Gastón Kleiman.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This test verifies that the agent backs-off properly when
> retrying authentication according to the configured parameters.
> 
> Also mocked `Slave::authenticate()` for this test.
> 
> 
> Diffs
> -----
> 
>   src/slave/slave.hpp 802d4eb9e9eba2f1175dd85e56dcc80a61e32f74 
>   src/tests/authentication_tests.cpp c9a8f85951a50e278ae509f4efa7105755015ce9 
>   src/tests/mock_slave.hpp 9a74bf35d2cab0a72ba6376392239d8080a49304 
>   src/tests/mock_slave.cpp 94a5b0d20475f49dde99108a009682b520175aa4 
> 
> 
> Diff: https://reviews.apache.org/r/68354/diff/3/
> 
> 
> Testing
> -------
> 
> make check
> 
> Added test continuously running without failure.
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message