For Kubernetes watch, add tests of backoff and reconnection on failures. Also, when watch response is >= 300, count that as a failure. by davidbtucker · Pull Request #205 · Stackdriver/metadata-agent

davidbtucker · 2018-10-16T04:50:43Z

No description provided.

bmoyles0117 · 2018-10-16T14:27:53Z

src/kubernetes.cc

+
+KubernetesReader::KubernetesReader(const Configuration& config,
+                                   HealthChecker* health_checker,
+                                   std::unique_ptr<Sleeper> sleeper)


Can we rename sleeper to be a bit more descriptive? This only seems to be used during retry backoffs.

+1. The sleep functionality is to induce the passage of time, so time-related tool names like "Timer", "Stopwatch", "AlarmClock", etc are usually appropriate. Would it make sense to reuse the existing Timer class and broaden its interface?

Happy to rename. What would you like?

bmoyles0117 · 2018-10-16T14:30:52Z

test/kubernetes_unittest.cc

+  // will return 404s.
+  //
+  // This will cause the updater to backoff 3 times and then give up.
+  server->SetResponse("/api/v1/nodes?limit=1", "{}");


Minor nit: this is a constant source of confusion for me. WDYT about moving it to a helper function such as InitHealthyServer(server) to reduce duplication and make the intent clear for the reader?

+1. This appears in another PR as well, as I recall...

StevenYCChou · 2018-10-16T18:59:45Z

test/Makefile

 purge: clean
 	$(RM) init-submodules
-	(cd .. && git submodule deinit -f $(GTEST_MODULE:../%=%))
+	(cd .. && git submodule deinit -f $(GTEST_MODULE:../%=%) && git submodule deinit -f $(GMOCK_MODULE:../%=%))


there is no GMOCK_MODULE defined yet.

if we end up deinit 2 paths, you can use single command:

git submodule deinit -f $(GTEST_MODULE:../%=%) ${GMOCK_MODULE:../%=%)}

No longer needed.

StevenYCChou · 2018-10-16T18:59:57Z

test/Makefile


 init-submodules:
-	(cd .. && git submodule update --init $(GTEST_MODULE:../%=%))
+	(cd .. && git submodule update --init $(GTEST_MODULE:../%=%) && git submodule update --init $(GMOCK_MODULE:../%=%))


No longer needed.

StevenYCChou · 2018-10-16T19:24:42Z

test/kubernetes_unittest.cc

+// Returns false if newer timestamp not found after 3 seconds (polling
+// every 100 millis).
+bool WaitForUnhealthyComponents(const HealthChecker& health_checker) {
+  for (int i = 0; i < 30; i++){


nit: need space between ) and {

StevenYCChou · 2018-10-16T19:41:13Z

test/kubernetes_unittest.cc

+};
+
+// Polls health_checker until it has at least one unhealthy component.
+// Returns false if newer timestamp not found after 3 seconds (polling


if using this_thread::sleep_for(), the comments should be at least 3 seconds (polling every 100 millis at least).
ref: https://en.cppreference.com/w/cpp/thread/sleep_for

StevenYCChou · 2018-10-16T19:44:18Z

test/kubernetes_unittest.cc

+// Polls health_checker until it has at least one unhealthy component.
+// Returns false if newer timestamp not found after 3 seconds (polling
+// every 100 millis).
+bool WaitForUnhealthyComponents(const HealthChecker& health_checker) {


I think having the timerange in function name will help reader reasoning - otherwise the reader needs to read through comments or code.

maybe WaitForUnhealthComponentsAtLeast3Seconds ? (If the function name is too long, come up with other names which makes sense...)

igorpeshansky

Some preliminary comments...

igorpeshansky · 2018-10-17T13:28:24Z

src/kubernetes.cc

+
+KubernetesReader::KubernetesReader(const Configuration& config,
+                                   HealthChecker* health_checker,
+                                   std::unique_ptr<Sleeper> sleeper)


+1. The sleep functionality is to induce the passage of time, so time-related tool names like "Timer", "Stopwatch", "AlarmClock", etc are usually appropriate. Would it make sense to reuse the existing Timer class and broaden its interface?

igorpeshansky · 2018-10-17T13:31:03Z

src/kubernetes.cc

      if (verbose) {
        LOG(INFO) << "WatchMaster(" << name << ") completed " << body(response);
      }
+      if (status(response) >= 300) {


This is an unrelated fix not mentioned in the PR description. Mitigations, in order of preference, would be to pull it (and the associated test changes) out into a separate PR, or at least mention it in the description.

Updated the PR description. The fix is intertwined with the new tests, so it's not really possible to pull out into a separate PR.

igorpeshansky · 2018-10-17T13:39:06Z

test/Makefile

-$(TESTS): $(GTEST_LIB) $(CPP_NETLIB_LIBS) $(YAML_CPP_LIBS)
+$(TESTS): $(GTEST_LIB) $(GMOCK_LIB) $(CPP_NETLIB_LIBS) $(YAML_CPP_LIBS)

 # All unittest objects depend on GTEST_LIB.


And GMOCK_LIB.

No longer needed.

igorpeshansky · 2018-10-17T13:56:20Z

test/kubernetes_unittest.cc

+  // will return 404s.
+  //
+  // This will cause the updater to backoff 3 times and then give up.
+  server->SetResponse("/api/v1/nodes?limit=1", "{}");


+1. This appears in another PR as well, as I recall...

igorpeshansky · 2018-10-17T18:02:06Z

test/kubernetes_unittest.cc

+  // SleepFor() calls are invoked.  Each is called twice, once each
+  // for the nodes & pods watchers.
+  auto sleeper = std::unique_ptr<MockSleeper>(new MockSleeper());
+  EXPECT_CALL(*sleeper, SleepFor(1.5)).Times(2);


Is there a reason why checking that Sleep was called with appropriate arguments via a mock is better than faking the passage of time and verifying that certain events happened at certain times?

I couldn't find anyway of faking out std::this_thread::sleep_for(). Got any suggestions?

davidbtucker added 2 commits October 15, 2018 14:54

Add tests for Kubernetes watch reconnection.

30dc4c5

For Kubernetes watch, add tests of backoff and reconnection on failures.

c7aa037

davidbtucker requested review from bmoyles0117 and igorpeshansky October 16, 2018 04:50

bmoyles0117 reviewed Oct 16, 2018

View reviewed changes

davidbtucker requested a review from StevenYCChou October 16, 2018 18:23

StevenYCChou reviewed Oct 16, 2018

View reviewed changes

igorpeshansky suggested changes Oct 17, 2018

View reviewed changes

igorpeshansky changed the base branch from davidbtucker-watch-reconnect-tests to master November 27, 2018 15:57

davidbtucker changed the title ~~For Kubernetes watch, add tests of backoff and reconnection on failures.~~ For Kubernetes watch, add tests of backoff and reconnection on failures. Also, when watch response is >= 300, count that as a failure. Dec 7, 2018

Address comments.

2e293d4

Conversation

davidbtucker commented Oct 16, 2018

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!

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!

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

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants