Init manager checking unready targets with target-aware watchers by ping-sun · Pull Request #12035 · envoyproxy/envoy

ping-sun · 2020-07-10T20:15:17Z

Linked Issue: #11963

Description: The current init manager only knows how many registered targets are not ready via its count_ field. This pull request mainly helps init manager to check which specific targets are not ready.

Key idea: Init manager stores the unready_target_name:count key-value pair in a hash map to check which registered targets are not ready. Enable passing target name into watcher's callback function.

When a new target is added into the targets list, increase the occurrence of the target's name in the hash map ++target_name_count_[target_name].
When a target is ready, it informs the manager's watcher and finally, the watcher notifies the manager(that's why we need to pass a string_view name parameter to the manager's callback). And decrease the count of the target's name by 1.
--target_name_count_[target_name].

Most recent update:
[Latest Implementation] Wrapped the old ReadyFn type function into the new TargetAwareReadyFn. Now WatcherImpl can take both types of function in its Ctor.

[Outdated Implementation] Split WatcherImpl into two classes, the old one and a new TargetAwareWatcherImpl. As we now limit the watcher_ inside ManagerImpl to receive a string_view parameter, we declare it to be of TargetAwareWatcherImpl.

In the current code base, watchers are basically constructed with std::function<void()>. In order to adopt the new feature(pass a string_view parameter to the manager's callback, I wrote a new constructor for WatcherImpl with std::function<void(absl::string_view)> parameter. In addition, I added a onTargetReadySendTargetName(string_view) adopted from the original callback onTargetReady(). So now both types of callbacks are supported. I've also updated code in the constructor of ManagerImpl, and now the watcher_ of a manager is constructed with the new type of function(with string_view parameter). Therefore, the manager will always get target_name from the watcher's callback.

Followup: Unready targets config dumps #12554

Risk Level: Low

The diagram below illustrates the outline of this PR

Signed-off-by: ping sun <pingsun@google.com>

lambdai

Great start!

source/common/init/manager_impl.h

source/common/init/manager_impl.cc

lambdai · 2020-07-10T21:12:46Z

CC @jplevyak

Signed-off-by: ping sun <pingsun@google.com>

include/envoy/runtime/runtime.h

source/common/init/manager_impl.h

source/common/init/manager_impl.cc

…view; targetHandle->name(); remove target_names_ vector Signed-off-by: pingsun <pingsun@google.com>

ping-sun · 2020-07-10T23:40:03Z

Thank you guys for the great reviews~ 'checkUnreadyTargets()' is just a basic function to check if the method could work. I am working on dumping the info to protobuf message and finally be used by admin handler.

Signed-off-by: ping sun <pingsun@google.com>

junr03 · 2020-07-13T18:24:12Z

/wait

lambdai

LGTM

Could you link the issue or address the follow up in description?

source/common/init/manager_impl.cc

source/server/server.cc

test/common/init/manager_impl_test.cc

Signed-off-by: pingsun <pingsun@google.com>

repokitteh-read-only · 2020-07-14T19:42:22Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #12035 was synchronize by ASOPVII.

see: more, trace.

ping-sun · 2020-07-14T20:18:39Z

LGTM

Could you link the issue or address the follow up in description?

Sure, updated.

Signed-off-by: pingsun <pingsun@google.com>

ping-sun · 2020-07-14T23:14:11Z

New Design:
Most recent update:
In the current code base, watchers are basically constructed with std::function<void()>. In order to adopt the new feature(pass a string_view parameter to the manager's callback, I wrote a new constructor for WatcherImpl with std::function<void(absl::string_view)> parameter. In addition, I added a onTargetReadySendTargetName(string_view) adopted from the original callback onTargetReady(). So now both types of callbacks are supported. I've also updated code in the constructor of ManagerImpl, and now the watcher_ of a manager is constructed with the new type of function(with string_view parameter).

Signed-off-by: pingsun <pingsun@google.com>

…r-query-unready-targets

Signed-off-by: pingsun <pingsun@google.com>

lambdai

This LGTM now. WDYT @lizan ?

lizan

Thanks, this is on the right track.

source/common/init/manager_impl.h

source/common/init/manager_impl.cc

source/common/init/watcher_impl.cc

source/server/admin/admin.cc

…alize unreadyTargetsConfigDump Signed-off-by: pingsun <pingsun@google.com>

lizan

Can we split the init manager refactoring part to another PR?

ping-sun · 2020-08-05T22:17:03Z

Can we split the init manager refactoring part to another PR?

But I don't think init manager's watcher

Can we split the init manager refactoring part to another PR?

You mean 1 PR for TargetAwareWatcher and 1 PR for manager's onTargetReady? SGTM

lizan · 2020-08-05T22:29:38Z

You can put both of Init manager / target to a single PR, I meant splitting this PR to the following two:

refactoring init manager / target / handler to optionally take names
dump unready targets in admin endpoint

…r-query-unready-targets

Signed-off-by: pingsun <pingsun@google.com>

ping-sun · 2020-08-05T23:16:50Z

@lizan I've updated this PR and separated the config dump into another branch, please review the recent commit.

lizan

Thanks

source/common/init/manager_impl.cc

source/common/init/watcher_impl.h

include/envoy/init/target.h

lizan · 2020-08-06T02:16:18Z

Can you also update PR title and description?

Signed-off-by: pingsun <pingsun@google.com>

ping-sun · 2020-08-06T02:48:43Z

Can you also update PR title and description?

Sure, updated. @lizan Please help review, and I will move on to the "unready target config dump" part.

Signed-off-by: pingsun <pingsun@google.com>

ping-sun · 2020-08-07T19:21:26Z

Can you also update PR title and description?

I think everything is completed now~

lizan · 2020-08-07T21:03:53Z

Thanks!

Description: Taking advantage of the new feature introduced in [#12035](#12035), which introduced quick visibility for init managers to check unready targets, this pull request adds protobuf message for unready targets and enables admin to dump configs of unready targets. An example of config dump for listeners’ unready targets is given in this pull request. Introduce ```InitDumpHandler``` with ```handlerInitDump``` method to help dump information of unready targets. Add ```dumpUnreadyTargets``` function for ```init::manager```. Risk Level: Low Docs Changes: protodoc Release Notes: Added Signed-off-by: pingsun <pingsun@google.com>

Description: Taking advantage of the new feature introduced in [#12035](envoyproxy/envoy#12035), which introduced quick visibility for init managers to check unready targets, this pull request adds protobuf message for unready targets and enables admin to dump configs of unready targets. An example of config dump for listeners’ unready targets is given in this pull request. Introduce ```InitDumpHandler``` with ```handlerInitDump``` method to help dump information of unready targets. Add ```dumpUnreadyTargets``` function for ```init::manager```. Risk Level: Low Docs Changes: protodoc Release Notes: Added Signed-off-by: pingsun <pingsun@google.com> Mirrored from https://github.com/envoyproxy/envoy @ 8aef76370877c66b09f7791f0577ca83aad7d608

ping-sun added 2 commits July 9, 2020 23:50

init-manager-query-unready-targets

10249c2

Signed-off-by: ping sun <pingsun@google.com>

init-manager-query-unready-targets

1de879e

Signed-off-by: ping sun <pingsun@google.com>

lambdai suggested changes Jul 10, 2020

View reviewed changes

init-manager-query-unready-targets

5a7c308

Signed-off-by: ping sun <pingsun@google.com>

mergeconflict reviewed Jul 10, 2020

View reviewed changes

include/envoy/runtime/runtime.h Outdated Show resolved Hide resolved

source/common/init/manager_impl.h Outdated Show resolved Hide resolved

source/common/init/manager_impl.cc Outdated Show resolved Hide resolved

source/common/init/manager_impl.cc Outdated Show resolved Hide resolved

update: unordered_map->flat_hash_map; const std::string->absl:string_…

562cc49

…view; targetHandle->name(); remove target_names_ vector Signed-off-by: pingsun <pingsun@google.com>

ping-sun added 2 commits July 10, 2020 16:45

init-manager-query-unready-targets

a023f16

Signed-off-by: ping sun <pingsun@google.com>

init-manager-query-unready-targets

e7ac416

Signed-off-by: ping sun <pingsun@google.com>

junr03 assigned lambdai and mergeconflict Jul 13, 2020

repokitteh-read-only bot added the waiting label Jul 13, 2020

lambdai previously approved these changes Jul 13, 2020

View reviewed changes

mergeconflict suggested changes Jul 13, 2020

View reviewed changes

source/common/init/manager_impl.cc Outdated Show resolved Hide resolved

source/common/init/manager_impl.cc Outdated Show resolved Hide resolved

source/server/server.cc Outdated Show resolved Hide resolved

test/common/init/manager_impl_test.cc Outdated Show resolved Hide resolved

ping-sun added 2 commits July 14, 2020 19:13

add dumpUnreadyTargetsConfig; remove checkUnreadyTargets()

b7fb8d8

Signed-off-by: pingsun <pingsun@google.com>

add dumpUnreadyTargetsConfigs()

c20129d

Signed-off-by: pingsun <pingsun@google.com>

ping-sun dismissed lambdai’s stale review via c20129d July 14, 2020 19:42

repokitteh-read-only bot added api and removed waiting labels Jul 14, 2020

ping-sun added 2 commits July 14, 2020 20:25

add dumpUnreadyTargetsConfigs()

6458f27

Signed-off-by: pingsun <pingsun@google.com>

add dumpUnreadyTargetsConfigs()

452a264

Signed-off-by: pingsun <pingsun@google.com>

ping-sun added 4 commits July 14, 2020 23:37

Merge branch 'master' of github.com:envoyproxy/envoy into exp

dc362e2

add extra constructor

0b7b75f

Signed-off-by: pingsun <pingsun@google.com>

add extra constructor

80af9b5

Signed-off-by: pingsun <pingsun@google.com>

add WatcherImpl constructors

2653323

Signed-off-by: pingsun <pingsun@google.com>

ping-sun added 2 commits August 1, 2020 04:15

Merge branch 'master' of github.com:envoyproxy/envoy into init-manage…

1794f68

…r-query-unready-targets

update integration stats

76aa72b

Signed-off-by: pingsun <pingsun@google.com>

lambdai previously approved these changes Aug 5, 2020

View reviewed changes

lizan suggested changes Aug 5, 2020

View reviewed changes

delete add string_view param into ManagerImpl::onTargetReady(); gener…

c2af7fe

…alize unreadyTargetsConfigDump Signed-off-by: pingsun <pingsun@google.com>

ping-sun dismissed lambdai’s stale review via c2af7fe August 5, 2020 22:12

lizan reviewed Aug 5, 2020

View reviewed changes

ping-sun added 2 commits August 5, 2020 22:48

Merge branch 'master' of github.com:envoyproxy/envoy into init-manage…

0d3269b

…r-query-unready-targets

target-aware-watcher

19b2cdd

Signed-off-by: pingsun <pingsun@google.com>

lizan reviewed Aug 6, 2020

View reviewed changes

source/common/init/manager_impl.cc Outdated Show resolved Hide resolved

source/common/init/watcher_impl.h Outdated Show resolved Hide resolved

include/envoy/init/target.h Outdated Show resolved Hide resolved

ping-sun changed the title ~~Init manager query unready targets~~ init managers check unready targets with target-aware watchers Aug 6, 2020

ping-sun changed the title ~~init managers check unready targets with target-aware watchers~~ Init manager checking unready targets with target-aware watchers Aug 6, 2020

remove/update comments

5b08552

Signed-off-by: pingsun <pingsun@google.com>

ping-sun requested review from lambdai and lizan August 6, 2020 03:16

move ++target_names_count[name] pos

cbe2ea4

Signed-off-by: pingsun <pingsun@google.com>

lizan approved these changes Aug 7, 2020

View reviewed changes

lizan merged commit 739f484 into envoyproxy:master Aug 7, 2020

ping-sun mentioned this pull request Aug 7, 2020

API: Unready Targets Dump #12554

Merged

jmarantz mentioned this pull request Aug 10, 2020

stats: keep a set of ref-counted parent histograms in ThreadLocalStore so that two with the same name map to the same histogram object. #12275

Merged

Conversation

ping-sun commented Jul 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lambdai left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lambdai commented Jul 10, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ping-sun commented Jul 10, 2020

Uh oh!

junr03 commented Jul 13, 2020

Uh oh!

lambdai left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

repokitteh-read-only bot commented Jul 14, 2020

Uh oh!

ping-sun commented Jul 14, 2020

Uh oh!

ping-sun commented Jul 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lambdai left a comment

Choose a reason for hiding this comment

Uh oh!

lizan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lizan left a comment

Choose a reason for hiding this comment

Uh oh!

ping-sun commented Aug 5, 2020

Uh oh!

lizan commented Aug 5, 2020

Uh oh!

ping-sun commented Aug 5, 2020

Uh oh!

lizan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lizan commented Aug 6, 2020

Uh oh!

ping-sun commented Aug 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ping-sun commented Aug 7, 2020

Uh oh!

lizan commented Aug 7, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

ping-sun commented Jul 10, 2020 •

edited

Loading

ping-sun commented Jul 14, 2020 •

edited

Loading

ping-sun commented Aug 6, 2020 •

edited

Loading