test infrastructure: inject time everywhere #4180

jmarantz · 2018-08-16T15:11:23Z

Description: This is one step toward addressing #4160 though this strategy may need to be backed out and re-worked. See the bug comments for more detail on the plan. The main value of this PR is to get rid of the Prod*TimeSource::instance_ statics, so we can inject time into tests. This PR doesn't actually inject time into tests differently; it just makes time injectable.

Apologies about the gigantic PR. The follow-ups can be more incremental.

Risk Level: medium: this should be non-functional, but a lot of lines of code changed. There is a fairly significant risk of merge issues too, so when merging, after this, we should be careful.
Testing: //test/...
Docs Changes: n/a
Release Notes: n/a

Signed-off-by: Joshua Marantz <jmarantz@google.com>

…hanged mocks. Signed-off-by: Joshua Marantz <jmarantz@google.com>

…(at least one with my own new assert). Signed-off-by: Joshua Marantz <jmarantz@google.com>

Signed-off-by: Joshua Marantz <jmarantz@google.com>

…rkerFactory::createWorker, Signed-off-by: Joshua Marantz <jmarantz@google.com>

…nt an assert and made it not fire. Signed-off-by: Joshua Marantz <jmarantz@google.com>

…eeds is a MonotonicTimeSource, so no need for the added test complexity. Signed-off-by: Joshua Marantz <jmarantz@google.com>

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz · 2018-08-17T22:16:07Z

ptal -- tsan looks like a download error.

Signed-off-by: Joshua Marantz <jmarantz@google.com>

…ock-time. Signed-off-by: Joshua Marantz <jmarantz@google.com>

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz · 2018-08-22T20:58:49Z

I added a branch from this PR where I add a format-check to discourage new source code from instantiating a real-time source.

jmarantz@c1f0dba

I'll issue a PR for that after this gets merged.

htuch

Thanks for taking this on. Overall, I like the approach of dependency injection, some interface level questions before doing the main pass.

htuch · 2018-08-22T22:54:54Z

include/envoy/api/api.h

@@ -22,7 +23,7 @@ class Api {
   * Allocate a dispatcher.
   * @return Event::DispatcherPtr which is owned by the caller.
   */
-  virtual Event::DispatcherPtr allocateDispatcher() PURE;
+  virtual Event::DispatcherPtr allocateDispatcher(TimeSource& time_source) PURE;


Nit: @param description.

done, i guess :)

htuch · 2018-08-22T22:56:08Z

include/envoy/common/time.h

@@ -37,6 +42,55 @@ class MonotonicTimeSource {
  /**
   * @return the current monotonic time.
   */
-  virtual MonotonicTime currentTime() PURE;
+  virtual MonotonicTime currentTime() const PURE;


I can imagine some implementation that would have a non-const semantics, e.g. make some fixup adjustment to maintain monotonicity on each call, but probably fine.

I was questioning this decision myself and now I'm reversing it. Similarly, a simulated system-time (non-monotonic) may want to increment time a tiny bit on each call to ensure that there is entropy for random seeds.

Not sure how I feel about that; we could also rely on impls that need to keep explicit state doing so via mutable. WDYT?

htuch · 2018-08-22T22:57:34Z

include/envoy/common/time.h

+ *
+ * TODO(#4160): currently this is just a container for SystemTimeSource and
+ * MonotonicTimeSource but we should clean that up and just have this as the
+ * base class.


Yeah. I would also like to see less implementation stuff inside the interface, but this is OK for now.

This will be addressed as part of the TODO. Adding commentary to that effect.

htuch · 2018-08-22T22:58:17Z

include/envoy/common/time.h

+  MonotonicTimeSource& monotonic() { return *monotonic_; }
+
+private:
+  SystemTimeSource* system_;


Nit: prefer SystemTimeSource& etc. as per Envoy style.

can't; need to be able to assign to support some mocking in the tests. Adding comment.

Note also that this uses Envoy convention at the interfaces, and there are other instances in Envoy that use pointer member vars internally to enable shallow mutation.

htuch · 2018-08-22T23:01:21Z

include/envoy/event/dispatcher.h

+   * TODO(#4160) the implementations currently manage timer events that
+   * ignore the time-source, and thus can't be mocked or faked. So it's
+   * difficult to mock time in an integration test without mocking out
+   * the dispatcher.


Is the only benefit here then of providing the timeSource() to allow for convenient threading of this object across any object that needs time and already has a dispatcher reference?

Not the only benefit. I think the dispatcher needs to understand time to support timers in a manner consistent with that. I'm just not biting that off in this PR. This interface makes the injection significantly easier in this PR, and will be functionally needed in the future for the dispatcher to work properly with mocked or simulated time.

htuch · 2018-08-22T23:02:12Z

include/envoy/server/filter_config.h

+  /**
+   * @return TimeSource& a reference to the time source.
+   */
+  virtual TimeSource& timeSource() PURE;


Why can't this be inferred from dispatcher() above?

It can be, but I feel like it's reasonable from an API perspective that the FilterConfig should provide direct access to its time source, even if the impl delegates that as an implementation detail.

htuch · 2018-08-22T23:02:37Z

include/envoy/server/instance.h

+  /**
+   * @return the time source used for the server.
+   */
+  virtual TimeSource& timeSource() PURE;


I feel that time is an essential interface property of a server, independent of whether it delegates ownership of the TimeSource itself to the dispatcher.

htuch · 2018-08-22T23:03:01Z

include/envoy/upstream/cluster_manager.h

+  /**
+   * @return TimeSource& the time-source used with the cluster manager.
+   */
+  virtual TimeSource& timeSource() PURE;


Is time really an essential property of ClusterManager?

Offhand, I am not an expert in the class; this is the first I've touched it, IIRC.

However, it appears based on its API to care about load-balancing, which I think typically has a time-component implied, no?

From a practical perspective, I need to find a time-source from places that already have a ClusterManager plumbed in and not much else. E.g. AsyncClientImpl

IMO you don't need a lot of excuses to want to know about time. Even a pure algorithm may want to employ timeouts, etc. Something I'm not proud of here is that PerfAnnotation in this PR still needs its own prod time-source, but I can't solve all problems in this PR, and I'm not sure we have a TimeSource available everywhere I might want to measure perf.

AsyncClientImpl should have a dispatcher where it can infer TimeSource. Are there other cases where we need this?

In general, I'm of the persuasion that I agree with what you write above, but that doesn't necessitate making TimeSource a part of the interface. ClusterManagerImpl can take TimeSource as a constructor arg, it doesn't have to be in the business of exporting this out to the rest of the system.

Can you see if it's practical to avoid adding to the interface here?

It is practical but will enlarge the CL significantly from what I can tell. Can I suggest a compromise where I add a TODO to the interfaces where it doesn't make (IMO) obvious sense (Dispatcher, arguably Server) to remove them when we've got everything plumbed directly?

I think the scary one is filters; I didn't want to change how every filter gets instantiated, preferring instead to provide access to the TimeSource via FilterConfig. Happy to remove that, but it will require changing every filter.

Per IM I have a follow-up in progress to remove this: jmarantz@b9a9cee

jmarantz · 2018-08-23T13:07:56Z

Rats; I wound up committing the requested changes to a branch. Time for some git magic...

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz · 2018-08-23T13:14:21Z

I think that worked; ptal.

Signed-off-by: Joshua Marantz <jmarantz@google.com>

…4226) The management of the timer_ variable in the test class was a little funky, and looked leak-prone. This PR cleans this up a little,. Without this PR, #4180 memory-leaks on this test, I think due to overwriting timer_. This is all toward resolution of #4160 Risk Level: Low. Testing: //test/common/config:grpc_mux_impl_test Docs Changes: n/a Release Notes: n/a Signed-off-by: Joshua Marantz <jmarantz@google.com>

Signed-off-by: Joshua Marantz <jmarantz@google.com>

…to use 'SimulatedTestTime' which hasn't been built yet. Signed-off-by: Joshua Marantz <jmarantz@google.com>

…if they are not copyable. Signed-off-by: Joshua Marantz <jmarantz@google.com>

htuch

@jmarantz thanks again for working on this. This PR is at the point where any reasonable reviewer is likely to make mistakes and conflate signal/noise, so can you just go through the comments I have and make the changes and we can merge? Also, please keep merging on each push to ensure we don't end up with a merge race.

htuch · 2018-08-24T03:07:11Z

include/envoy/upstream/cluster_manager.h

+  /**
+   * @return TimeSource& the time-source used with the cluster manager.
+   */
+  virtual TimeSource& timeSource() PURE;


AsyncClientImpl should have a dispatcher where it can infer TimeSource. Are there other cases where we need this?

In general, I'm of the persuasion that I agree with what you write above, but that doesn't necessitate making TimeSource a part of the interface. ClusterManagerImpl can take TimeSource as a constructor arg, it doesn't have to be in the business of exporting this out to the rest of the system.

Can you see if it's practical to avoid adding to the interface here?

htuch · 2018-08-24T03:07:49Z

source/common/common/perf_annotation.h

  PerfAnnotationContext* context_;
+  MonotonicTime start_time_;


Nit: why reordering?

Initializing context is needed (from a lazy-init static) prior to computing the start-time.

htuch · 2018-08-24T03:08:30Z

source/common/common/token_bucket_impl.cc

@@ -4,14 +4,15 @@

 namespace Envoy {

-TokenBucketImpl::TokenBucketImpl(uint64_t max_tokens, double fill_rate,
-                                 MonotonicTimeSource& time_source)
+TokenBucketImpl::TokenBucketImpl(uint64_t max_tokens, MonotonicTimeSource& monotonic_time_source,


Nit: why reordering?

because fill_rate is optional, but time-source is not.

htuch · 2018-08-24T03:09:05Z

source/common/common/token_bucket_impl.h

@@ -16,10 +16,10 @@ class TokenBucketImpl : public TokenBucket {
   * @param max_tokens supplies the maximun number of tokens in the bucket.
   * @param fill_rate supplies the number of tokens that will return to the bucket on each second.
   * The default is 1.
-   * @param time_source supplies the time source. The default is ProdMonotonicTimeSource.
+   * @param time_source supplies the time source.


Params are now out of order..

htuch · 2018-08-24T03:10:49Z

source/common/grpc/async_client_impl.h

@@ -25,6 +25,8 @@ class AsyncClientImpl final : public AsyncClient {
  AsyncStream* start(const Protobuf::MethodDescriptor& service_method,
                     AsyncStreamCallbacks& callbacks) override;

+  TimeSource& timeSource() { return cm_.timeSource(); }


Actually, you don't have the dispatcher directly here, but see https://github.com/envoyproxy/envoy/blob/master/source/common/grpc/async_client_impl.cc#L70

that seems plausible. Actually that's effectively pulling it from cluster_manager anyway, where it looks like it goes through a process of doing a search that can throw an exception if it fails.

I'd rather just get the time-source directly.

htuch · 2018-08-24T03:12:42Z

source/extensions/tracers/zipkin/util.h

@@ -3,6 +3,8 @@
 #include <string>
 #include <vector>

+#include "envoy/common/time.h"


Should there be a corresponding BUiLD file change to deps?

the zipkin rule already had an explicit dependency to time_interface.

htuch · 2018-08-24T03:14:49Z

test/common/config/filesystem_subscription_test_harness.h

@@ -94,6 +95,7 @@ class FilesystemSubscriptionTestHarness : public SubscriptionTestHarness {

  const std::string path_;
  std::string version_;
+  RealTestTime test_time_;


"Real" doesn't seem very precise for a naming convention here. How about DangerousDeprecatedTestTime? :)

my sed script is busy working on this; will be in the upcoming commit. This is a sufficiently nasty name to dissuade people writing new tests from using it, I hope :)

I'm not 100% sure that we should never use real-time in tests; there may be some corner cases where you really want that, such as timing out an algorithm by polling real-time. But I'm happy to switch the name for now, and once most of them are removed, we can consider renaming back.

Signed-off-by: Joshua Marantz <jmarantz@google.com>

htuch

Awesome.

Follow-up to #4180 -- removes ClusterManager::timeSource() as an API. Another step toward resolving #4160 Risk Level: low Testing: //test/... Docs Changes: n/a Release Notes: n/a Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz added 6 commits August 15, 2018 23:54

Remove singletons for time-sources and inject them instead.

da7f24a

Signed-off-by: Joshua Marantz <jmarantz@google.com>

checkpoint with more stuff compiling, a few tests still fail due to c…

7d59de4

…hanged mocks. Signed-off-by: Joshua Marantz <jmarantz@google.com>

checkpoinging: all of //test/common/... compiles, 4 tests still fail …

10422d4

…(at least one with my own new assert). Signed-off-by: Joshua Marantz <jmarantz@google.com>

//test/common/... all pass.

282307f

Signed-off-by: Joshua Marantz <jmarantz@google.com>

More of //test/... builds, some of it works.

fa5c8c8

Signed-off-by: Joshua Marantz <jmarantz@google.com>

All tests compiling and working.

8d67f55

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz mentioned this pull request Aug 17, 2018

integration tests should use simulated time #4160

Open

jmarantz added 5 commits August 17, 2018 10:12

More comments and TODOs.

c9a7d62

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Pass the TimeSource into the WorkerFactory's ctor rather than into Wo…

2d9f2aa

…rkerFactory::createWorker, Signed-off-by: Joshua Marantz <jmarantz@google.com>

Fix race found in coverage tests due to order of destruction. Uncomme…

1a84061

…nt an assert and made it not fire. Signed-off-by: Joshua Marantz <jmarantz@google.com>

Don't bother using a whole TimeSource for TokenBucketImpl -- all it n…

eb2347e

…eeds is a MonotonicTimeSource, so no need for the added test complexity. Signed-off-by: Joshua Marantz <jmarantz@google.com>

More API cleanups and TODOs.

21ac418

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz changed the title ~~WIP: inject time everywhere~~ test infrastructure: inject time everywhere Aug 17, 2018

Kick CI

0a2514c

Signed-off-by: Joshua Marantz <jmarantz@google.com>

htuch self-assigned this Aug 21, 2018

htuch self-requested a review August 21, 2018 13:39

jmarantz mentioned this pull request Aug 22, 2018

grpc: Clean up initialization and allocation of timers in the test #4226

Merged

jmarantz added 4 commits August 22, 2018 12:35

Merge branch 'master' into inject-time-source

c642dbc

Signed-off-by: Joshua Marantz <jmarantz@google.com>

formatting.

646a500

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Add comment about preserving entropy in the future, when we move to m…

e65a754

…ock-time. Signed-off-by: Joshua Marantz <jmarantz@google.com>

Merge branch 'master' into inject-time-source

50dbb33

Signed-off-by: Joshua Marantz <jmarantz@google.com>

htuch suggested changes Aug 22, 2018

View reviewed changes

Address review comments.

be44f2d

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Kick CI

1488d58

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Merge branch 'master' into inject-time-source

44d880d

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz added 2 commits August 23, 2018 18:41

Rename 'TestTime' to 'RealTestTime' and add TODO to change all tests …

6d9fac6

…to use 'SimulatedTestTime' which hasn't been built yet. Signed-off-by: Joshua Marantz <jmarantz@google.com>

Don't copy TimeSource objects; it's easier to make them pure virtual …

944d75b

…if they are not copyable. Signed-off-by: Joshua Marantz <jmarantz@google.com>

htuch suggested changes Aug 24, 2018

View reviewed changes

jmarantz added 2 commits August 24, 2018 08:51

Merge branch 'master' into inject-time-source

28f41dd

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Resolve nits.

a285196

Signed-off-by: Joshua Marantz <jmarantz@google.com>

htuch approved these changes Aug 24, 2018

View reviewed changes

jmarantz mentioned this pull request Aug 24, 2018

test infra: Remove timeSource() from the ClusterManager api #4247

Merged

htuch merged commit 9281a72 into envoyproxy:master Aug 24, 2018

jmarantz deleted the inject-time-source branch August 24, 2018 19:05

test infrastructure: inject time everywhere #4180

test infrastructure: inject time everywhere #4180

Conversation

jmarantz commented Aug 16, 2018 • edited Loading

jmarantz commented Aug 17, 2018

jmarantz commented Aug 22, 2018

htuch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmarantz commented Aug 23, 2018

jmarantz commented Aug 23, 2018

htuch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htuch left a comment

Choose a reason for hiding this comment

jmarantz commented Aug 16, 2018 •

edited

Loading