InitManager: TargetImpl::ready() behavior change #7498

lambdai · 2019-07-08T23:06:28Z

Signed-off-by: Yuchen Dai silentdai@gmail.com

Description:
TargetImpl::ready() called before Init::Manager switch the target state to ready.
This behavior change allows asynchronous TargetImpl::ready() invocation.
This PR remains the behavior that destroyed TargetImpl is considered as ready.

Risk Level: Mid

Testing: Existing tests and added unit test for InitManager

Docs Changes: Inline comment

Release Notes:
This is a developer behavior change but not a user behavior change. Not sure if release notes is needed.

This behavior is discussed in #6904

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

alyssawilk · 2019-07-09T13:43:03Z

@mergeconflict would you be up for doing first pass, and tagging Harvey/Matt when you've LGTMd?

mergeconflict

Looks like a good start. My only issue with this revision is that I don't want to change the contract between init manager and target, I want to keep the changes just in the target.

source/common/init/manager_impl.cc

source/common/init/target_impl.cc

test/common/init/manager_impl_test.cc

mergeconflict · 2019-07-09T14:52:02Z

source/common/init/target_impl.cc

@@ -27,19 +27,27 @@ TargetImpl::TargetImpl(absl::string_view name, InitializeFn fn)
      fn_(std::make_shared<InternalInitalizeFn>([this, fn](WatcherHandlePtr watcher_handle) {


This is the key spot I think you need to modify. I think it needs to look something like:

if (is_ready_) { // If we were already ready before being called by the init manager, just signal immediately... watcher_handle->ready(); } else { // ... Otherwise, hang onto the init manager's watcher handle and initialize. watcher_handle_ = std::move(watcher_handle); fn(); }

Thanks! That is close to my plan B. Well, this is even better as I somehow considered is_ready_ should be shared pointer. Will use your clean code :)

@mergeconflict It's arguable if the a ready target would invoke fn().

I prefer to keep the existing behavior that fn() would be invoked to . WDYT?

I don't know, that seems a little sketchy to me, maybe... fn() is the target's initialization function, and if the target is already ready, wouldn't that mean fn() (or its equivalent) would have already been called before?

For those user who call ready() only in fn(), both are fine.

For the users who may call ready() before fn(), the users will see fn() invoked prior to this PR. Since we don't have a strong opinion whether fn() should be called or not, let's not break the behavior that fn() is called in this situation.

For my use case, my fn in Target constructor is empty as the ready() depends on other signal. So I vote for NOT to assume ready() always comes from fn().

Whoa... this doesn't make sense to me. If the initialization function for your target is empty, and your target is always ready immediately, it seems like you have no need for Init::Manager at all.

Sorry for the unclear.

The initialization function is empty

True

your target is always ready immediately

Not always. I don't want to introduce the complexity here so let me simplify the case I was seeing.
The listener could provide the server scope Init::Manager or per listener Init::Manager. The former one could be initialized really late but my target wants to be started as early as possible but it doesn't guarantee be ready before Init::Manager::initialize().

Hmm, that's complicated... So, in this case, even though the init manager isn't telling the target when to initialize, the init manager still needs to know that the target has initialized, did I get that right?

Exactly. Actually I think the existing CDS/LDS should follow this pattern as well: cluster should be a target of listener's init manager and cluster should be updated independently. However, CDS/LDS doesn't use this at the risk that throw the ordering to ADS or user.

source/common/init/target_impl.cc

lambdai

Beside the existing comment I will move the newly added test cases to target_test as it is implementation details of Target

lambdai · 2019-07-09T16:47:48Z

source/common/init/target_impl.cc

@@ -27,19 +27,27 @@ TargetImpl::TargetImpl(absl::string_view name, InitializeFn fn)
      fn_(std::make_shared<InternalInitalizeFn>([this, fn](WatcherHandlePtr watcher_handle) {


Thanks! That is close to my plan B. Well, this is even better as I somehow considered is_ready_ should be shared pointer. Will use your clean code :)

source/common/init/manager_impl.cc

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai · 2019-07-09T17:40:45Z

test/common/init/manager_impl_test.cc

@@ -49,18 +49,21 @@ TEST(InitManagerImplTest, AddReadyTarget) {

  ExpectableWatcherImpl w;

+  t1.expectInitialize();


@mergeconflict add this not only because it is easier to coordinate but also less astonishment.

Reason:
AddTargetAndMarkReadyBeforeInitialization does call expectInitialize
Prior to this PR this function is also called anyway.

mergeconflict · 2019-07-09T17:50:45Z

source/common/init/target_impl.cc

@@ -27,6 +27,10 @@ TargetImpl::TargetImpl(absl::string_view name, InitializeFn fn)
      fn_(std::make_shared<InternalInitalizeFn>([this, fn](WatcherHandlePtr watcher_handle) {
        watcher_handle_ = std::move(watcher_handle);
        fn();
+        // It is possible that fn() mutates the is_ready_.


I don't think this logic is correct. The only way that the initialization fn() can mutate is_ready_ is by calling ready(). You don't want to call it twice.

The fact is that TargetImpl::ready() is idempotent. I see several places utilize this assumption.

Is the lambda abusing the fact? If so I can fix

True, it's idempotent, but that's for the user's benefit. To me, it feels really weird to say:

if (is_ready_) { ready(); }

It also feels wrong to call fn(), and unnecessary to hang onto the watcher handle, if is_ready_ is true.

fixed in the latest commit

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

mergeconflict · 2019-07-09T20:52:33Z

source/common/init/target_impl.cc

-        watcher_handle_ = std::move(watcher_handle);
-        fn();
+        if (is_ready_) {
+          fn();


I still don't really understand why the call to fn() is needed in the case where the target is already initialized.

IMHO it's not a must-have. I leave it in this way because that is what user will see.
Suppose we have the code and happens to be executed in below order:

Context A Context B Context C ManagerImpl m; TargetImpl t("foo", fn); m.add(t); ... X: t.ready(); Y: m.initialize(w);

Prior to this PR:

Either t.ready() happens before m.initialize(w), or the reverse, we will observe that fn will be called.

w will be triggered only if t.ready() happens after m.initialize(w)

This goal of this PR is to change item 2.

Our conflict is whether 1 should be maintained. I don't have a strong preference but I feel there is no gain to change 1.

That's a really good explanation.

My assumption has always been that fn() (or something called by it indirectly) calls t.ready(). Or, in other words, if t.ready() has been called, there's no need for fn() to be called again. You've said that in your case, fn is actually empty, and ready is called by some other code, and I still sort of think that's an abuse of the init manager, but I guess we can ignore that for now :)

Here's my thinking: we can't assume that fn is idempotent, and I think we should assume that if is_ready_ is true, that means initialization is done. Like, I think of it as really being is_initialized_. So, to your point, it would be a change in behavior to not call fn, but I think it's most consistent with the design intent of the init manager (that is, I think it's really fixing a latent bug).

but I guess we can ignore that for now :)

I haven't been fully tracking this conversation, but I don't think we should ignore this for now. Can we step back and make sure we are actually solving the right problem? However we think it's best to do that is fine with me.

we can't assume that fn is idempotent

I used to consider fn()

a signal which is harmless to call more than once.

may have side affect other than transit state from not-ready to ready.
It leads to my code that InitManager should call it at least once.

I start to understand your concern:
if is_ready is true, it doesn't make sense that the init system calls fn().

Will update following your opinion

mergeconflict · 2019-07-09T20:53:35Z

source/common/init/target_impl.h

-   * any time. Calling it before initialization begins or after initialization has already ended
-   * will have no effect.
+   * any time.
+   * Notes: Calling it before initialization begins is allowed.


Should probably clarify what happens when you call ready() before the init manager has called the target.

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

mergeconflict · 2019-07-11T13:42:16Z

source/common/init/target_impl.h

@@ -47,11 +47,32 @@ class TargetHandleImpl : public TargetHandle, Logger::Loggable<Logger::Id::init>
  const std::weak_ptr<InternalInitalizeFn> fn_;
 };

+class AbstractTarget : public Target, protected Logger::Loggable<Logger::Id::init> {


I think the Envoy naming convention for this would be TargetImplBase.

I see. Will fix

mergeconflict · 2019-07-11T13:44:04Z

source/common/init/target_impl.h

+   */
+  bool ready() override;
+};
+/**


Need blank line after TargetImpl body, and need to update comment for EagerTargetImpl.

mergeconflict · 2019-07-11T13:46:40Z

source/common/init/target_impl.h

+   *     doesn't take a WatcherHandlePtr (like TargetFn does). Managing the watcher handle is done
+   *     internally to simplify usage.
+   */
+  EagerTargetImpl(absl::string_view name, InitializeFn fn);


I think the idea we talked about is that fn isn't even needed here, since the target will initialize itself as soon as it can (eagerly), rather than being invoked by the manager.

Sorry I didn't understand. You are right, fn doesn't needed in the unique case. Will remove.

mergeconflict · 2019-07-11T13:55:00Z

source/common/init/target_impl.cc

+            fn();
+          })) {}
+
+TargetImpl::~TargetImpl() { ENVOY_LOG(debug, "{} destroyed", name_); }


I think this and ~EagerTargetImpl can be moved to the base class dtor.

great catch! will fix

mergeconflict · 2019-07-11T13:56:14Z

source/common/init/target_impl.cc

+
+EagerTargetImpl::~EagerTargetImpl() { ENVOY_LOG(debug, "{} destroyed", name_); }
+
+bool EagerTargetImpl::ready() {


Common code between here and TargetImpl::ready() can be refactored.

mergeconflict · 2019-07-11T13:57:23Z

source/common/init/target_impl.h

@@ -47,11 +47,32 @@ class TargetHandleImpl : public TargetHandle, Logger::Loggable<Logger::Id::init>
  const std::weak_ptr<InternalInitalizeFn> fn_;
 };

+class AbstractTarget : public Target, protected Logger::Loggable<Logger::Id::init> {
+public:
+  AbstractTarget(absl::string_view name, std::shared_ptr<InternalInitalizeFn> fn);


See comment in EagerTargetImpl's ctor: I don't think the base class needs an initialization function, I think only TargetImpl needs it.

mergeconflict · 2019-07-11T14:00:04Z

test/mocks/init/mocks.h

+   * calling `ready` immediately.
+   */
+  ::testing::internal::TypedExpectation<void()>& expectInitializeWillCallReady();
+};


Need blank line after class body.

mergeconflict · 2019-07-11T14:00:53Z

test/mocks/init/mocks.h

@@ -50,6 +50,23 @@ class ExpectableTargetImpl : public TargetImpl {
  ::testing::internal::TypedExpectation<void()>& expectInitializeWillCallReady();
 };

+class ExpectableEagerTargetImpl : public EagerTargetImpl {


Can the code duplication here be refactored out?

ExpectableEagerTargetImpl doesn't need fn and this mock is not helpful. will remove

mergeconflict · 2019-07-11T14:02:38Z

test/common/init/manager_impl_test.cc

+
+using TargetTypes = ::testing::Types<ExpectableTargetImpl, ExpectableEagerTargetImpl>;
+
+TYPED_TEST_SUITE(InitManagerImplTest, TargetTypes);


Neat, I hadn't seen this before. I'm not sure if you can totally get away with it, though, if you follow my suggestion of removing the initialization function from the eager target. I think the expectation is that the two target types should not behave the same in all cases.

Right. Once the fn is removed from EagerTargetImpl the existing test cases don't fit. Will refactor

mattklein123 · 2019-07-11T18:13:18Z

@silentdai can we step back for a sec?

I'm a little concerned that we are not solving the right problem here. Can you summarize what problem you are trying to solve? My understanding is that you want a resource to be ready() before initialize() is called. Is that right? For my understanding, why can you not just keep track that it is ready and then just immediately call ready() in the context of initialize()?

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai · 2019-07-12T07:41:51Z

@mattklein123 Thank you for the comment!

My understanding is that you want a resource to be ready() before initialize() is called. Is that right?

Yes!

For my understanding, why can you not just keep track that it is ready and then just immediately call ready() in the context of initialize()?

Yes. I can. I did it in #7440

EagerTarget beats my code in two ways

coherency: EagerTarget vs a ready_flag + a TargetImpl. It burns brain to understand the flag should be touched by Target. I don't want invent another GUARDED_BY :)
ownership:
There are 4 objs: Target, InitializeFn as arg of TargetImpl ctor, ready_flag, another initialize function which could set the ready_flag. Which owns/refers which?
EagerTarget is simple: InitializeFn owns EagerTargetImpl. InitializeFn doesn't care the timing of target is added to Init::Manager. If target is gone when it should be added to Manager, it is also fine.

mattklein123 · 2019-07-12T16:00:57Z

Yes. I can. I did it in #7440

My personal opinion is that init manager is already complicated enough as it is, and unless we really need to make changes to support a new use case, we probably should not. IMO it's not that complicated to just call ready() in the context of initialize(). I'm pretty sure we already did that in other places, e.g., for static clusters.

However I will defer to @mergeconflict as he has been thinking about this area a lot more closely.

mergeconflict · 2019-07-12T17:16:51Z

Yes. I can. I did it in #7440

My personal opinion is that init manager is already complicated enough as it is, and unless we really need to make changes to support a new use case, we probably should not. IMO it's not that complicated to just call ready() in the context of initialize(). I'm pretty sure we already did that in other places, e.g., for static clusters.

However I will defer to @mergeconflict as he has been thinking about this area a lot more closely.

Yeah, I honestly agree with @mattklein123. If it's possible to support your use case without changes/additions to the init code, I'd prefer that, since the init manager is already tricky enough without having to reason about multiple varieties of target with different polymorphic behaviors.

lambdai · 2019-07-12T19:53:32Z

OK. I will adopt the idea that check ready flag in initialize function.

Sorry my initial intention is to extract the logic from xDS to Init namespace since this is not a configuration business logic.

By polymorphic behaviors I also find a RAII Target which invoke ready() is also useful but I guess you don't like either.

I appreciate your comments and feedback!

lambdai added 2 commits July 8, 2019 22:52

InitManager: TargetImpl::ready() called before init

b75b329

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

erase unused target handle

e9f2d62

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

alyssawilk assigned mergeconflict Jul 9, 2019

mergeconflict suggested changes Jul 9, 2019

View reviewed changes

lambdai commented Jul 9, 2019

View reviewed changes

address comment: limit the mutation in TargetImpl

73c65a3

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai commented Jul 9, 2019

View reviewed changes

mergeconflict reviewed Jul 9, 2019

View reviewed changes

dont abuse TargetImpl::ready()

e88665c

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

mergeconflict reviewed Jul 9, 2019

View reviewed changes

lambdai added 2 commits July 10, 2019 23:57

rewrite: a new target impl

83ee7a2

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

common target tests and specialized target impl tests

28dfa28

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

mergeconflict suggested changes Jul 11, 2019

View reviewed changes

no initialization at EagerTarget

f1c1aba

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai closed this Jul 12, 2019

		@@ -27,19 +27,27 @@ TargetImpl::TargetImpl(absl::string_view name, InitializeFn fn)
		fn_(std::make_shared<InternalInitalizeFn>([this, fn](WatcherHandlePtr watcher_handle) {

		@@ -49,18 +49,21 @@ TEST(InitManagerImplTest, AddReadyTarget) {

		ExpectableWatcherImpl w;

		t1.expectInitialize();


		EagerTargetImpl::~EagerTargetImpl() { ENVOY_LOG(debug, "{} destroyed", name_); }

		bool EagerTargetImpl::ready() {


		using TargetTypes = ::testing::Types<ExpectableTargetImpl, ExpectableEagerTargetImpl>;

		TYPED_TEST_SUITE(InitManagerImplTest, TargetTypes);

InitManager: TargetImpl::ready() behavior change #7498

InitManager: TargetImpl::ready() behavior change #7498

Conversation

lambdai commented Jul 8, 2019

alyssawilk commented Jul 9, 2019

mergeconflict 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

lambdai 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

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

mattklein123 commented Jul 11, 2019

lambdai commented Jul 12, 2019

mattklein123 commented Jul 12, 2019

mergeconflict commented Jul 12, 2019

lambdai commented Jul 12, 2019