init: add shared target by lambdai · Pull Request #9008 · envoyproxy/envoy

lambdai · 2019-11-13T07:48:24Z

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

Extracted from #8523

@mergeconflict mentioned we can also maintain a collection of TargetImpl(and a initialize_ flag, std::once_flag) at the target owner.

I agree it works but a SharedTarget provides stronger cohesion.

Follow up:

update xds: share rds configuration among envoy #8523 on top of this PR
complete fuzz test Wip: fuzz: init manager #8714

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

lambdai · 2019-11-13T07:48:35Z

/assign @htuch

htuch

LGTM, @mergeconflict do you have any thoughts?

source/common/init/target_impl.cc

test/common/init/target_impl_test.cc

mergeconflict

Implementation looks good. I'd like to spend a bit more time focusing on cleaning up the tests; see comments below. Let me know when you're ready and I'll take another look to try and figure out whether we're covering all the possible interesting interaction sequences.

test/common/init/target_impl_test.cc

source/common/init/target_impl.cc

lambdai

Thank @mergeconflict for your advises.

I will revert the WrapUnique and leave the potential actions in other PR/Issue

source/common/init/target_impl.cc

test/common/init/target_impl_test.cc

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

htuch

LGTM; will give @mergeconflict a chance to look before merging.

source/common/init/target_impl.cc

test/mocks/init/mocks.h

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

test/common/init/target_impl_test.cc

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

mergeconflict

I think you're almost there, thanks for working on this! Just a couple more bits of cleanup.

mergeconflict · 2019-11-20T15:31:21Z

test/mocks/init/mocks.h

+class ExpectableSharedTargetImpl : public SharedTargetImpl {
+public:
+  ExpectableSharedTargetImpl(absl::string_view name = "test");
+  ExpectableSharedTargetImpl(absl::string_view name, InitializeFn fn);


This ctor defeats the purpose of the mock. The idea of this class is that it's wired up to always call the mocked initialize method, that you can set expectations on using expectInitialize. I don't think you actually use this anyway, so it should be easy to delete.

Right. Didn't use it in this PR. I will delete it for simplicity

mergeconflict · 2019-11-20T15:37:01Z

test/common/init/target_impl_test.cc

+// SharedTarget acts as TargetImpl if single watcher is provided.
+TEST(InitSharedTargetTestImpl, InitializeSingleWatcher) {
+  InSequence s;
+
+  ExpectableSharedTargetImpl target;
+  ExpectableWatcherImpl watcher;
+
+  // initializing the target through its handle should invoke initialize()...
+  target.expectInitialize();
+  EXPECT_TRUE(target.createHandle("test")->initialize(watcher));
+
+  // calling ready() on the target should invoke the saved watcher handle...
+  watcher.expectReady();
+  EXPECT_TRUE(target.ready());
+
+  // calling ready() a second time should have no effect.
+  watcher.expectReady().Times(0);
+  target.ready();
+}
+
+// Initializing TargetHandle return false if uninitialized SharedTarget is destroyed.
+TEST(InitSharedTargetTestImpl, InitializeWhenUnavailable) {
+  InSequence s;
+  ExpectableWatcherImpl watcher;
+  TargetHandlePtr handle;
+  {
+    ExpectableSharedTargetImpl target;
+
+    // initializing the target after it's been destroyed should do nothing.
+    handle = target.createHandle("test");
+    target.expectInitialize().Times(0);
+  }
+  EXPECT_FALSE(handle->initialize(watcher));
+}
+
+// Initializing TargetHandle return false if initialized SharedTarget is destroyed.
+TEST(InitSharedTargetTestImpl, ReInitializeWhenUnavailable) {
+  InSequence s;
+  ExpectableWatcherImpl w;
+  TargetHandlePtr handle;
+  {
+    ExpectableSharedTargetImpl target;
+
+    target.expectInitialize();
+    TargetHandlePtr handle1 = target.createHandle("m1");
+    ExpectableWatcherImpl w1;
+    EXPECT_TRUE(handle1->initialize(w1));
+
+    // initializing the target after it's been destroyed should do nothing.
+    handle = target.createHandle("m2");
+    target.expectInitialize().Times(0);
+    // target destroyed
+  }
+  EXPECT_FALSE(handle->initialize(w));
+}


Rather than copy-pasting these tests from above, can you use https://github.com/google/googletest/blob/master/googletest/docs/advanced.md#typed-tests to refactor? I think all the invariants held by TargetImpl must also be held by SharedTargetImpl.

You caught me :)
I delete the expectation from target.ready()

Will recover and use typed test

mergeconflict · 2019-11-20T16:04:18Z

test/common/init/target_impl_test.cc

+  EXPECT_FALSE(handle->initialize(w));
+}
+
+// SharedTarget notifies multiple watchers.


So these two new tests capture these two sequences, if I'm reading correctly:

initialize(w1) initialize(w2) ready

and

initialize(w1) ready initialize(w2)

I guess you could also write one for

ready initialize(w1) initialize(w2)

But that's probably not very interesting... I can't think of any more interesting cases, so I think you're good. Can you think of anything?

It's not hard to support a ready() prior to initialize any watcher.
The confusing part what are the expected consequences.
MUST: notify the following watchers
Question: Should the initialization function defined in SharedTarget should be called?

I am going to leave the questionable behavior as undefined and set no expectation.
Let me know what you think

lambdai · 2019-11-21T18:54:32Z

@mergeconflict Thank you! I am updating the PR

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

mergeconflict

Looking good, just a few last nits now. Thanks!

mergeconflict · 2019-11-22T14:46:25Z

source/common/init/target_impl.cc

 bool SharedTargetImpl::ready() {
  initialized_ = true;
-  bool all_notified = true;
+  bool all_notified = !watcher_handles_.empty();


Interesting... I guess this is for consistency with TargetImpl::ready returning false if there's no watcher handle. But now I'm wondering if I made the right choice there. What do you think?

Yes it is.
It seems no production code is relying on this return value.
It's OK to leave the return value here since so far it is easy to maintain.

test/common/init/target_impl_test.cc

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

lambdai · 2019-11-22T23:01:45Z

@mergeconflict So the latest tests are

[ RUN      ] Init/TargetImplTest/0.Name
[ RUN      ] Init/TargetImplTest/0.InitializeWhenAvailable
[ RUN      ] Init/TargetImplTest/0.InitializeWhenUnavailable
[ RUN      ] Init/TargetImplTest/0.ReadyWhenWatcherUnavailable
[ RUN      ] Init/TargetImplTest/1.Name
[ RUN      ] Init/TargetImplTest/1.InitializeWhenAvailable
[ RUN      ] Init/TargetImplTest/1.InitializeWhenUnavailable
[ RUN      ] Init/TargetImplTest/1.ReadyWhenWatcherUnavailable
[ RUN      ] SharedTargetImplTest.ReInitializeWhenUnavailable
[ RUN      ] SharedTargetImplTest.NotifyAllWatcherWhenInitialization
[ RUN      ] SharedTargetImplTest.InitializedSharedTargetNotifyWatcherWhenAddedAgain
[ RUN      ] SharedTargetImplTest.EarlySharedTargetReadyNotifyWatchers

mergeconflict

LGTM, thanks for all the work on this!

htuch

Thanks @lambdai! Also thanks @mergeconflict for the deep dive.

htuch · 2019-11-25T17:58:14Z

test/common/init/target_impl_test.cc

+template <typename T> class TargetImplTest : public ::testing::Test {};
+TYPED_TEST_SUITE_P(TargetImplTest);
+
+template <typename T> std::string getName() { return ""; }


FWIW, you could avoid templates here and do something similar to what was done in https://github.com/envoyproxy/envoy/blob/master/test/common/config/subscription_test_harness.h, but I think this isn't too bad and makes things nice and clean.

Full context is here The above PR is split into #9008 and this one. In this PR envoy will maintain only one copy of rds config for each resource name. Also fixed a the bug of early listener notified ready describe in #8781 The test case is included in this PR. Signed-off-by: Yuchen Dai <silentdai@gmail.com>

Full context is here The above PR is split into envoyproxy#9008 and this one. In this PR envoy will maintain only one copy of rds config for each resource name. Also fixed a the bug of early listener notified ready describe in envoyproxy#8781 The test case is included in this PR. Signed-off-by: Yuchen Dai <silentdai@gmail.com> Signed-off-by: Prakhar <prakhar_au@yahoo.com>

init: add shared target

eb835ee

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

repokitteh-read-only bot assigned htuch Nov 13, 2019

htuch reviewed Nov 14, 2019

View reviewed changes

source/common/init/target_impl.cc Show resolved Hide resolved

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

mergeconflict suggested changes Nov 14, 2019

View reviewed changes

lambdai commented Nov 14, 2019

View reviewed changes

Expectable

73fdbad

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

htuch previously approved these changes Nov 15, 2019

View reviewed changes

mergeconflict suggested changes Nov 15, 2019

View reviewed changes

source/common/init/target_impl.cc Show resolved Hide resolved

source/common/init/target_impl.cc Show resolved Hide resolved

test/mocks/init/mocks.h Outdated Show resolved Hide resolved

lambdai added 2 commits November 15, 2019 18:14

an example of too robust test

9cddbda

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

revert to count_ test

fb6569b

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

lambdai dismissed htuch’s stale review via fb6569b November 15, 2019 18:21

lambdai commented Nov 15, 2019

View reviewed changes

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

lambdai commented Nov 15, 2019

View reviewed changes

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

lambdai requested review from htuch and mergeconflict November 15, 2019 18:31

lambdai added 2 commits November 20, 2019 05:52

rewrite tests

7ac1d51

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

more clean up

5761ba2

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

mergeconflict reviewed Nov 20, 2019

View reviewed changes

lambdai added 2 commits November 21, 2019 21:09

refactor test

444530f

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

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

8c81fb9

mergeconflict suggested changes Nov 22, 2019

View reviewed changes

beautify test cases

a817033

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

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

523958d

mergeconflict approved these changes Nov 25, 2019

View reviewed changes

htuch approved these changes Nov 25, 2019

View reviewed changes

htuch merged commit 95dfc51 into envoyproxy:master Nov 25, 2019

lambdai mentioned this pull request Dec 3, 2019

xds: share rds configuration among envoy 2/2 #9209

Merged

Conversation

lambdai commented Nov 13, 2019

Uh oh!

lambdai commented Nov 13, 2019

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mergeconflict 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!

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!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mergeconflict 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!

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!

lambdai commented Nov 21, 2019

Uh oh!

mergeconflict 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lambdai commented Nov 22, 2019

Uh oh!

mergeconflict left a comment

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects