server: make overload manager non-optional #11777

akonradi · 2020-06-26T18:49:42Z

Commit Message:
Make the Overload Manager argument a reference instead of a pointer, and make the ThreadLocalOverloadState an interface.

Additional Description: Making the OM non-optional reduces special case handling in the HTTP Connection Manager impl, and making the thread-local state object an interface the makes it easier to update the OverloadManagerImpl without causing churn in header files.

Risk Level: low
Testing: ran unit tests
Docs Changes: none
Release Notes: none

The only user of a null OverloadManager was the admin console. Switch that to using a no-op OverloadManager subclass so that the OM can be provided via reference in all cases. This makes the HCM initialization simpler since it doesn't need to worry about the null pointer. Signed-off-by: Alex Konradi <akonradi@google.com>

Make ThreadLocalOverloadState an interface that the Overload Manager can return an implementation of. This decouples the behavior specified by the header from the details of the implementation. Signed-off-by: Alex Konradi <akonradi@google.com>

Signed-off-by: Alex Konradi <akonradi@google.com>

antoniovicente

Great cleanup. Some questions and nits below.

source/server/admin/admin.h

source/server/admin/admin.cc

antoniovicente · 2020-06-29T19:00:17Z

source/server/admin/admin.h

+
+    NullOverloadManager(Event::Dispatcher& dispatcher) : thread_local_overload_state_(dispatcher) {}
+    void start() override {}
+    ThreadLocalOverloadState& getThreadLocalOverloadState() override {


const ThreadLocalOverloadState& since ThreadLocalOverloadState has no mutation methods as part of the interface.

I don't want to make this const because I expect in the near future that we'll want non-const methods on here.

Tell me more about said non-const methods. I would expect OverloadManager to be the one doing all the updates to overload state resources. I'm guessing that you're referring to methods to create scalable timers, which in fact would need to be non-const.

That said, I wonder if scalable timers should be more closely integrated with the Event::Dispatcher interface.

Yep, my concern is around adding methods to create scalable timers. I think we can discuss them in a separate issue, though, and leave this as-is here.

I think my advice is to use the Event::Dispatcher interface to create scalable timers. We can discuss in the PR that defines that API.

I think I'd prefer to make this const for now and then remove it down the line if we start needing non-const access, just so we don't accidentally leave this non-const.

Finally gave it a try and it turns out that won't work since getState() is non-const. getState is non-const because it looks up a key in the internal state map and creates an entry for it if one doesn't exist.

It is strange that getState returns a reference instead of a value. State is a primitive type (an enum)

The callers hold onto the reference to avoid extra map lookups

antoniovicente · 2020-06-29T19:00:45Z

source/server/admin/admin.h

+      return false;
+    }
+
+    NullThreadOverloadState thread_local_overload_state_;


I don't think this object is a thread local. I'm not sure what's the right way to resolve this issue. ThreadLocalOverloadState only has const methods in its interface so it is safe for the null overload manager to be a singleton instead of a thread local. I wonder if renaming to OverloadManagerState would help, or further add to the confusion.

Made this object explicitly thread-local instead of relying on const accessors.

All implementations of the interface are for thread-local objects so specify that with interface inheritance. Signed-off-by: Alex Konradi <akonradi@google.com>

- rename member - use thread-local store - remove dispatcher Signed-off-by: Alex Konradi <akonradi@google.com>

source/server/admin/admin.cc

source/server/admin/admin.h

antoniovicente · 2020-06-29T21:55:55Z

source/server/admin/admin.h

+    NullOverloadManager(ThreadLocal::SlotAllocator& slot_allocator)
+        : tls_(slot_allocator.allocateSlot()) {}
+
+    void start() override {


is start() called?

Nope, moved the TLS initialization to the constructor, which seems to work fine.

I think you opted to use start() in the end.

What was the reasoning to keep this in start vs the ctor?

The constructor gets invoked with AdminImpl's constructor, which is invoked in server.cc before the TLS state is initialized. Putting this in start lets us defer it.

Signed-off-by: Alex Konradi <akonradi@google.com>

source/server/admin/admin.h

Signed-off-by: Alex Konradi <akonradi@google.com>

Doing it in the constructor was causing ASAN errors. Signed-off-by: Alex Konradi <akonradi@google.com>

Initialize the AdminImpl after the dispatchers so that it can create thread-local data structures. Without this, integration tests fail ASAN runs. Signed-off-by: Alex Konradi <akonradi@google.com>

Signed-off-by: Alex Konradi <akonradi@google.com>

akonradi · 2020-07-07T17:38:42Z

@antoniovicente can I get another pass on this? I had to make some changes to get the ASAN integration tests to pass.

antoniovicente · 2020-07-07T17:56:55Z

source/server/admin/admin.h

+
+    NullOverloadManager(Event::Dispatcher& dispatcher) : thread_local_overload_state_(dispatcher) {}
+    void start() override {}
+    ThreadLocalOverloadState& getThreadLocalOverloadState() override {


I think my advice is to use the Event::Dispatcher interface to create scalable timers. We can discuss in the PR that defines that API.

antoniovicente · 2020-07-07T17:59:52Z

source/server/overload_manager_impl.cc

+      actions_[action] = state;
+    } else {
+      it->second = state;
+    }


I'm unsure why the body of this function isn't simply:

actions_[action] = state;

antoniovicente · 2020-07-07T21:20:36Z

source/server/server.cc

@@ -428,6 +411,24 @@ void InstanceImpl::initialize(const Options& options,
    dispatcher_->initializeStats(stats_store_, "server.");
  }

+  if (initial_config.admin().address()) {


What specific action you need to happen first? Is it the "thread_local_.registerThread(*dispatcher_, true);"

Does worker initialization happen before or after this point?

I wonder if initialization is still happening too early and possible consequences for internal admin API users which bypass request handling on the admin port.

Yep, that's the line. The worker initialization happens before, in the ListenerManagerImpl constructor.

Signed-off-by: Alex Konradi <akonradi@google.com>

akonradi · 2020-07-09T15:49:42Z

@mattklein123 can I get a review?

antoniovicente · 2020-07-09T16:51:46Z

source/server/admin/admin.h

+    NullOverloadManager(ThreadLocal::SlotAllocator& slot_allocator)
+        : tls_(slot_allocator.allocateSlot()) {}
+
+    void start() override {


I think you opted to use start() in the end.

akonradi · 2020-07-09T18:01:34Z

@ggreenway I've been informed Matt is OOO, and Alyssa is with the same org as me and Antonio. Can I ask you to assign a senior maintainer to review this?

snowp

Thanks this looks good overall, just a few comments

snowp · 2020-07-10T15:24:59Z

source/server/admin/admin.h

+    NullOverloadManager(ThreadLocal::SlotAllocator& slot_allocator)
+        : tls_(slot_allocator.allocateSlot()) {}
+
+    void start() override {


What was the reasoning to keep this in start vs the ctor?

snowp · 2020-07-10T15:27:12Z

source/server/admin/admin.h

+
+    NullOverloadManager(Event::Dispatcher& dispatcher) : thread_local_overload_state_(dispatcher) {}
+    void start() override {}
+    ThreadLocalOverloadState& getThreadLocalOverloadState() override {


I think I'd prefer to make this const for now and then remove it down the line if we start needing non-const access, just so we don't accidentally leave this non-const.

snowp · 2020-07-10T15:28:33Z

source/server/admin/admin.h

+
+    bool registerForAction(const std::string&, Event::Dispatcher&, OverloadActionCb) override {
+      // This method shouldn't be called by the admin listener
+      ASSERT(false);


typically we'd use NOT_REACHED_GCOVR_EXCL_LINE to exclude this line from coverage in addition to assert that it wont get called

Signed-off-by: Alex Konradi <akonradi@google.com>

akonradi · 2020-07-13T19:18:54Z

@snowp it looks like the TSAN failure is due to #9784. Also, the /retest command appears to only re-run CircleCI tests, not the Azure pipelines.

snowp

Thanks, this LGTM.

I think its failing on a well known flake, I'll try to rerun the test a few times.

The only user of a null OverloadManager was the admin console. Switch that to using a no-op OverloadManager subclass so that the OM can be provided via reference in all cases. This makes the HCM initialization simpler since it doesn't need to worry about the null pointer. Signed-off-by: Alex Konradi <akonradi@google.com> Signed-off-by: scheler <santosh.cheler@appdynamics.com>

akonradi requested review from alyssawilk and mattklein123 as code owners June 26, 2020 18:49

akonradi force-pushed the non-optional-om branch 2 times, most recently from e994350 to ef0f99b Compare June 26, 2020 18:52

Make ThreadLocalOverloadState virtual

76f494b

Make ThreadLocalOverloadState an interface that the Overload Manager can return an implementation of. This decouples the behavior specified by the header from the details of the implementation. Signed-off-by: Alex Konradi <akonradi@google.com>

akonradi force-pushed the non-optional-om branch from ef0f99b to 76f494b Compare June 26, 2020 18:59

Fix formatting

0a402c4

Signed-off-by: Alex Konradi <akonradi@google.com>

asraa assigned antoniovicente Jun 26, 2020

antoniovicente reviewed Jun 29, 2020

View reviewed changes

akonradi added 2 commits June 29, 2020 16:00

Mark ThreadLocalOverloadState as thread-local

2609dbe

All implementations of the interface are for thread-local objects so specify that with interface inheritance. Signed-off-by: Alex Konradi <akonradi@google.com>

Address feedback on null overload manager

dac81f8

- rename member - use thread-local store - remove dispatcher Signed-off-by: Alex Konradi <akonradi@google.com>

antoniovicente reviewed Jun 29, 2020

View reviewed changes

Address NullOverloadManager feedback

159f964

Signed-off-by: Alex Konradi <akonradi@google.com>

antoniovicente previously approved these changes Jul 1, 2020

View reviewed changes

source/server/admin/admin.h Outdated Show resolved Hide resolved

Fix clang tidy issue

5f40260

Signed-off-by: Alex Konradi <akonradi@google.com>

akonradi dismissed antoniovicente’s stale review via 5f40260 July 1, 2020 20:23

akonradi added 4 commits July 1, 2020 16:24

Remove unused #include

44ac271

Signed-off-by: Alex Konradi <akonradi@google.com>

Move TLS initialization to start()

7334861

Doing it in the constructor was causing ASAN errors. Signed-off-by: Alex Konradi <akonradi@google.com>

Move admin initialization later

438f711

Initialize the AdminImpl after the dispatchers so that it can create thread-local data structures. Without this, integration tests fail ASAN runs. Signed-off-by: Alex Konradi <akonradi@google.com>

Merge remote-tracking branch 'upstream/master' into non-optional-om

ed7e988

Signed-off-by: Alex Konradi <akonradi@google.com>

antoniovicente previously approved these changes Jul 9, 2020

View reviewed changes

akonradi dismissed antoniovicente’s stale review via 9fbd32b July 9, 2020 13:30

Simplify ThreadLocalOverloadStateImpl::setState

3bc1f88

Signed-off-by: Alex Konradi <akonradi@google.com>

akonradi force-pushed the non-optional-om branch from 9fbd32b to 3bc1f88 Compare July 9, 2020 13:32

Fix formatting

04053d4

Signed-off-by: Alex Konradi <akonradi@google.com>

antoniovicente previously approved these changes Jul 9, 2020

View reviewed changes

ggreenway assigned snowp Jul 9, 2020

snowp suggested changes Jul 10, 2020

View reviewed changes

Use correct macro for "this is never called"

82146f6

Signed-off-by: Alex Konradi <akonradi@google.com>

akonradi dismissed antoniovicente’s stale review via 82146f6 July 10, 2020 17:54

Fix formatting

c190617

Signed-off-by: Alex Konradi <akonradi@google.com>

snowp approved these changes Jul 13, 2020

View reviewed changes

mattklein123 merged commit 0c6523c into envoyproxy:master Jul 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

server: make overload manager non-optional #11777

server: make overload manager non-optional #11777

akonradi commented Jun 26, 2020

antoniovicente left a comment

antoniovicente Jun 29, 2020

akonradi Jun 29, 2020

antoniovicente Jul 1, 2020

akonradi Jul 1, 2020

antoniovicente Jul 7, 2020

snowp Jul 10, 2020

akonradi Jul 10, 2020

antoniovicente Jul 10, 2020

akonradi Jul 10, 2020

antoniovicente Jun 29, 2020

akonradi Jun 29, 2020

antoniovicente Jun 29, 2020

akonradi Jul 1, 2020

antoniovicente Jul 9, 2020

snowp Jul 10, 2020

akonradi Jul 10, 2020

akonradi commented Jul 7, 2020

antoniovicente Jul 7, 2020

antoniovicente Jul 7, 2020

antoniovicente Jul 7, 2020

akonradi Jul 9, 2020

akonradi commented Jul 9, 2020

antoniovicente Jul 9, 2020

akonradi commented Jul 9, 2020

snowp left a comment

snowp Jul 10, 2020

snowp Jul 10, 2020

snowp Jul 10, 2020

akonradi commented Jul 13, 2020

snowp left a comment

server: make overload manager non-optional #11777

server: make overload manager non-optional #11777

Conversation

akonradi commented Jun 26, 2020

antoniovicente 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

akonradi commented Jul 7, 2020

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

akonradi commented Jul 9, 2020

Choose a reason for hiding this comment

akonradi commented Jul 9, 2020

snowp 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

akonradi commented Jul 13, 2020

snowp left a comment

Choose a reason for hiding this comment