introduce singleton manager and fix multiple TLS date providers by mattklein123 · Pull Request #1410 · envoyproxy/envoy

mattklein123 · 2017-08-07T19:56:22Z

It is a common problem that there needs to be a single instance of
something in the server. This change introduces the concept of the
singleton manager which can be used for that purpose. All singletons
are looked up by name and must be statically registered like existing
filters. This avoids collision.

This change uses the singleton manager to create a single TLS date provider
for the entire server.

Fixes #1362.

It is a common problem that there needs to be a single instance of something in the server. This change introduces the concept of the singleton manager which can be used for that purpose. All singletons are looked up by name and must be statically registered like existing filters. This avoids collision. This change uses the singleton manager to create a single TLS date provider for the entire server. Fixes #1362.

mattklein123 · 2017-08-07T19:56:45Z

@lyft/network-team @htuch @dnoe

@bplotnick this should fix your issue.

htuch · 2017-08-07T20:19:29Z

include/envoy/server/instance.h


+  /**
+   * @return Singleton::Manager& the server-wide singleton manager.
+   */


High level question: should this subsume existing singleton state on the server? E.g. initManager(), listenerManager() etc.

It probably could/should. I basically did this because I did not want to add another top level singleton and plumb it through everywhere. I don't think it's super critical that we back-fill but could open an issue to track.

I think this will be generally useful for things that are added on or not core to the server such that everyone will use no matter what (.e.g, redis, mongo, etc.). Everyone will use the cluster manager, init manager, listener manager, etc. so those are a gray area IMO.

Thanks for the clarification. Yeah, either an issue or maybe just some TODO to move the other ones under here later.

bplotnick · 2017-08-07T22:16:30Z

I only took a brief look through the code (and C++ is certainly not my specialty), but I pulled this branch and it looks like the CPU usage is significantly lower in my high listener count use case 👍

htuch · 2017-08-08T00:57:47Z

include/envoy/singleton/manager.h

+ * and must be statically registered ahead of time. This can be done like so:
+ *
+ * static constexpr char foo_singleton_name[] = "foo_singleton";
+ * static Registry::RegisterFactory<Singleton::RegistrationImpl<foo_singleton_name>,


Can you comment on the tradeoffs of using a string in the template vs. a concrete type as done in Registry::RegisterFactory?

I did this because it's less typing (technically it is a concrete type, it's just an instantiation of RegistrationImpl). We only need name() which is required by registry. I could probably modify Registry to support a name only registration but that seemed like overkill. If you feel otherwise I can do that.

Sure, less boiler plate is always better, thanks for the clarification.

htuch · 2017-08-08T00:59:39Z

include/envoy/singleton/manager.h

+  /**
+   * This is a helper on top of get() that casts the object stored to the specified type. No type
+   * information is specified explicitly in code so dynamic_cast provides some level of protection
+   * via RTTI.


Not sure I follow the second sentence here (the "no type information is specified explicitly"), unless this has to do with the template string arg.

I will clarify.

htuch · 2017-08-08T01:00:44Z

include/envoy/singleton/manager.h

+   * @param name supplies the singleton name. Must be registered via RegistrationImpl.
+   * @param singleton supplies the singleton. NOTE: The manager only stores a weak pointer. This
+   *        allows a singleton to be cleaned up if it is not needed any more. All code that uses
+   *        singletons must check for validity and create a new singleton if needed. It must also


Would it make sense to have the check for validity + create a utility function in the manager, to avoid repeating this pattern in lots of places?

I could do this, but it would involve passing a lambda into this function. If you like that better happy to do it.

It seems a bit cleaner to centralize it to me, so I'd have a slight preference to do it this way all things being equal.

htuch · 2017-08-08T01:01:24Z

source/common/singleton/manager_impl.cc

+}
+
+void ManagerImpl::verifyRegistration(const std::string& name) {
+  if (nullptr == Registry::FactoryRegistry<Registration>::getFactory(name)) {


Should this be an ASSERT? I.e., is this something that could happen if not programmer error.

I think it probably needs to be more than an ASSERT since we want to verify the name is unique. I could make it a PANIC() with appropriate message. I just did exception since it's easier to test. No real preference either way (could also switch to an illegal arg exception which would be more likely to crash).

We'd probably catch most of the core Envoy violations of this in the CI runs if it was an ASSERT, but I see the value of making it an exception or panic when Envoy is extended by other folks. So, up to you what you feel is best on this, just wanted to clarify my understanding.

htuch · 2017-08-08T01:02:40Z

source/common/singleton/manager_impl.cc

+namespace Singleton {
+
+InstancePtr ManagerImpl::get(const std::string& name) {
+  verifyRegistration(name);


Does it make sense to have any thread checks here? Can anyone other than the main thread access this? IF so, do we need to lock to avoid racing on get/set?

My intention was for this to be used by the main thread only, though it doesn't really matter. I can either add appropriate asserts or add locking? Which do you prefer?

Asserts make more sense if this is the expected use.

mattklein123 · 2017-08-09T03:30:06Z

@htuch updated

htuch

LGTM

htuch · 2017-08-09T14:15:07Z

include/envoy/singleton/manager.h

+   * @param singleton supplies the singleton creation callback. This will only be called if the
+   *        singleton does not already exist. NOTE: The manager only stores a weak pointer. This
+   *        allows a singleton to be cleaned up if it is not needed any more. All code that uses
+   *        singletons must check for validity and create a new singleton if needed. It must also


Do we still need to have callers check for validity?

htuch · 2017-08-09T14:17:24Z

include/envoy/singleton/instance.h

+#pragma once
+
+namespace Envoy {
+namespace Singleton {


Can you cleanup the TODO in https://github.com/lyft/envoy/blob/master/source/common/common/singleton.h#L19 and and add a pointer in the comments on that class to where to go for mutable singletons.

If we didn't want cleanup on last dropped reference, and only had default constructors, we could do something simple like what is done for ConstSingleton perhaps.

mattklein123 · 2017-08-09T15:07:26Z

@htuch updated

Signed-off-by: Mike Schore <[email protected]> Signed-off-by: JP Simard <[email protected]>

**Description** Envoy AI Gateway currently requires an envoy gateway config map to be overriden. The better approach is to pass the configmap as helm values during envoy gateway helm install. This PR makes changes to the documentation and installation files to do the same. **Related Issues/PRs (if applicable)** Fixes: envoyproxy/ai-gateway#1191 --------- Signed-off-by: sailesh duddupudi <[email protected]>

**Description** Since #1410, the deployment created by EG for the rate limit server is in crashing loop until redis server is up and running in the rate limit e2e test setup, which caused a bit of flake in the test. This fixes it by restarting the deployment just in case. This also removes the only remaining use of t.Context in test cleanup function in e2e tests. Signed-off-by: Takeshi Yoneda <[email protected]>

htuch reviewed Aug 7, 2017

View reviewed changes

fix

87638f8

ryan-lane closed this Aug 7, 2017

ryan-lane reopened this Aug 7, 2017

htuch reviewed Aug 8, 2017

View reviewed changes

mattklein123 mentioned this pull request Aug 8, 2017

server: add server owned http route manager #1345

Merged

mattklein123 added 5 commits August 8, 2017 09:48

Merge branch 'master' into fix_date_tls

9f09d35

Merge remote-tracking branch 'origin/master' into fix_date_tls

83bffc7

comments

39780b7

Merge branch 'master' into fix_date_tls

a2fac3c

comments

2d3ba5c

fix

3e113b7

htuch reviewed Aug 9, 2017

View reviewed changes

comment

42257d1

htuch approved these changes Aug 9, 2017

View reviewed changes

mattklein123 merged commit 2ad6cfd into master Aug 9, 2017

mattklein123 deleted the fix_date_tls branch August 9, 2017 17:02

junr03 mentioned this pull request Aug 11, 2017

move route config provider manager to singleton manager registry #1449

Merged

jpsim pushed a commit that referenced this pull request Nov 28, 2022

examples: fix swift app (#1410)

c5bd450

Signed-off-by: Mike Schore <[email protected]> Signed-off-by: JP Simard <[email protected]>

jpsim pushed a commit that referenced this pull request Nov 29, 2022

examples: fix swift app (#1410)

5d741bf

Signed-off-by: Mike Schore <[email protected]> Signed-off-by: JP Simard <[email protected]>

wbpcode mentioned this pull request Jun 16, 2024

singleton manager: use ASSERT_IS_MAIN_OR_TEST_THREAD rather than comparing tid directly #34766

Merged

Conversation

mattklein123 commented Aug 7, 2017

Uh oh!

mattklein123 commented Aug 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bplotnick commented Aug 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 commented Aug 9, 2017

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 commented Aug 9, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bplotnick commented Aug 7, 2017 •

edited

Loading