From 016ac6aa69ccc3de9179754dd3960ffb4a2359ac Mon Sep 17 00:00:00 2001
From: Igor Peshansky <igorpeshansky@github.com>
Date: Tue, 1 May 2018 05:21:43 -0400
Subject: [PATCH 1/6] Change ValidateConfiguration to throw ValidationError on
 invalid configs. Return false to stop or true to proceed.

---
 src/docker.cc            | 60 +++++++++++++++++++++---------
 src/docker.h             |  5 ++-
 src/kubernetes.cc        | 79 ++++++++++++++++++++++++++++++----------
 src/kubernetes.h         |  5 ++-
 src/updater.cc           | 17 ++++++---
 src/updater.h            | 24 +++++++++---
 test/Makefile            |  2 +-
 test/updater_unittest.cc |  9 +++--
 8 files changed, 143 insertions(+), 58 deletions(-)

diff --git a/src/docker.cc b/src/docker.cc
index cd77ecc4..2ea1e7a2 100644
--- a/src/docker.cc
+++ b/src/docker.cc
@@ -36,6 +36,9 @@ namespace google {
 
 namespace {
 
+constexpr const int kDockerValidationRetryLimit = 10;
+constexpr const int kDockerValidationRetryDelaySeconds = 1;
+
 #if 0
 constexpr const char kDockerEndpointHost[] = "unix://%2Fvar%2Frun%2Fdocker.sock/";
 constexpr const char kDockerApiVersion[] = "1.23";
@@ -45,24 +48,42 @@ constexpr const char kDockerContainerResourcePrefix[] = "container";
 
 }
 
+// A subclass of QueryException to represent non-retriable errors.
+class DockerReader::NonRetriableError : public DockerReader::QueryException {
+ public:
+  NonRetriableError(const std::string& what) : QueryException(what) {}
+};
+
 DockerReader::DockerReader(const Configuration& config)
     : config_(config), environment_(config) {}
 
-bool DockerReader::ValidateConfiguration() const {
-  try {
-    const std::string container_filter(
-        config_.DockerContainerFilter().empty()
-        ? "" : "&" + config_.DockerContainerFilter());
-
-    // A limit may exist in the container_filter, however, the docker API only
-    // uses the first limit provided in the query params.
-    (void) QueryDocker(std::string(kDockerEndpointPath) + 
-                       "/json?all=true&limit=1" + container_filter);
-
-    return true;
-  } catch (const QueryException& e) {
-    // Already logged.
-    return false;
+bool DockerReader::ValidateConfiguration() const
+    throw(MetadataUpdater::ValidationError) {
+  for (int i = 0; true; ++i) {
+    try {
+      const std::string container_filter(
+          config_.DockerContainerFilter().empty()
+          ? "" : "&" + config_.DockerContainerFilter());
+
+      // A limit may exist in the container_filter, however, the docker API only
+      // uses the first limit provided in the query params.
+      (void) QueryDocker(std::string(kDockerEndpointPath) +
+                         "/json?all=true&limit=1" + container_filter);
+
+      return true;
+    } catch (const NonRetriableError& e) {
+      throw MetadataUpdater::ValidationError(
+          "Docker query validation failed: " + e.what());
+    } catch (const QueryException& e) {
+      // Already logged.
+      if (i < kDockerValidationRetryLimit) {
+        std::this_thread::sleep_for(
+          time::seconds(kDockerValidationRetryDelaySeconds));
+      } else {
+        throw MetadataUpdater::ValidationError(
+            "Docker query retry limit reached: " + e.what());
+      }
+    }
   }
 }
 
@@ -190,7 +211,12 @@ json::value DockerReader::QueryDocker(const std::string& path) const
   }
   try {
     http::local_client::response response = client.get(request);
-    if (status(response) >= 300) {
+    if (status(response) >= 400 && status(response) <= 403) {
+      throw NonRetriableError(
+          format::Substitute("Server responded with '{{message}}' ({{code}})",
+                             {{"message", status_message(response)},
+                              {"code", format::str(status(response))}}));
+    } else if (status(response) >= 300) {
       throw boost::system::system_error(
           boost::system::errc::make_error_code(boost::system::errc::not_connected),
           format::Substitute("Server responded with '{{message}}' ({{code}})",
@@ -207,7 +233,7 @@ json::value DockerReader::QueryDocker(const std::string& path) const
   }
 }
 
-bool DockerUpdater::ValidateConfiguration() const {
+bool DockerUpdater::ValidateConfiguration() const throw(ValidationError) {
   if (!PollingMetadataUpdater::ValidateConfiguration()) {
     return false;
   }
diff --git a/src/docker.h b/src/docker.h
index ea62a765..e72bacd6 100644
--- a/src/docker.h
+++ b/src/docker.h
@@ -37,7 +37,7 @@ class DockerReader {
 
   // Validates the relevant configuration and returns true if it's correct.
   // Returns a bool that represents if it's configured properly.
-  bool ValidateConfiguration() const;
+  bool ValidateConfiguration() const throw(MetadataUpdater::ValidationError);
 
  private:
   // A representation of all query-related errors.
@@ -48,6 +48,7 @@ class DockerReader {
    private:
     std::string explanation_;
   };
+  class NonRetriableError;
 
   // Issues a Docker API query at a given path and returns a parsed
   // JSON response. The path has to start with "/".
@@ -72,7 +73,7 @@ class DockerUpdater : public PollingMetadataUpdater {
           [=]() { return reader_.MetadataQuery(); }) { }
 
  protected:
-  bool ValidateConfiguration() const;
+  bool ValidateConfiguration() const throw(ValidationError);
 
  private:
   DockerReader reader_;
diff --git a/src/kubernetes.cc b/src/kubernetes.cc
index 2168768e..93f480fc 100644
--- a/src/kubernetes.cc
+++ b/src/kubernetes.cc
@@ -43,6 +43,9 @@ namespace google {
 
 namespace {
 
+constexpr const int kKubernetesValidationRetryLimit = 10;
+constexpr const int kKubernetesValidationRetryDelaySeconds = 1;
+
 #if 0
 constexpr const char kKubernetesEndpointHost[] = "https://kubernetes.default.svc";
 #endif
@@ -89,6 +92,13 @@ bool ReadServiceAccountSecret(
 
 }
 
+// A subclass of QueryException to represent non-retriable errors.
+class KubernetesReader::NonRetriableError
+    : public KubernetesReader::QueryException {
+ public:
+  NonRetriableError(const std::string& what) : QueryException(what) {}
+};
+
 KubernetesReader::KubernetesReader(const Configuration& config,
                                    HealthChecker* health_checker)
     : config_(config), environment_(config), health_checker_(health_checker) {}
@@ -654,7 +664,12 @@ json::value KubernetesReader::QueryMaster(const std::string& path) const
   }
   try {
     http::client::response response = client.get(request);
-    if (status(response) >= 300) {
+    if (status(response) >= 400 && status(response) <= 403) {
+      throw NonRetriableError(
+          format::Substitute("Server responded with '{{message}}' ({{code}})",
+                             {{"message", status_message(response)},
+                              {"code", format::str(status(response))}}));
+    } else if (status(response) >= 300) {
       throw boost::system::system_error(
           boost::system::errc::make_error_code(boost::system::errc::not_connected),
           format::Substitute("Server responded with '{{message}}' ({{code}})",
@@ -1056,28 +1071,51 @@ void KubernetesReader::UpdateServiceToPodsCache(
   }
 }
 
-bool KubernetesReader::ValidateConfiguration() const {
-  try {
-    (void) QueryMaster(std::string(kKubernetesEndpointPath) + "/nodes?limit=1");
-  } catch (const QueryException& e) {
-    // Already logged.
-    return false;
+bool KubernetesReader::ValidateConfiguration() const
+    throw(MetadataUpdater::ValidationError) {
+  for (int i = 0; true; ++i) {
+    try {
+      (void) QueryMaster(std::string(kKubernetesEndpointPath) + "/nodes?limit=1");
+    } catch (const NonRetriableError& e) {
+      throw MetadataUpdater::ValidationError(
+          "Node query validation failed: " + e.what());
+    } catch (const QueryException& e) {
+      // Already logged.
+      if (i < kKubernetesValidationRetryLimit) {
+        std::this_thread::sleep_for(
+          time::seconds(kKubernetesValidationRetryDelaySeconds));
+      } else {
+        throw MetadataUpdater::ValidationError(
+            "Node query retry limit reached: " + e.what());
+      }
+    }
   }
 
-  try {
-    const std::string pod_label_selector(
-        config_.KubernetesPodLabelSelector().empty()
-        ? "" : "&" + config_.KubernetesPodLabelSelector());
-
-    (void) QueryMaster(std::string(kKubernetesEndpointPath) + "/pods?limit=1" +
-                       pod_label_selector);
-  } catch (const QueryException& e) {
-    // Already logged.
-    return false;
+  for (int i = 0; true; ++i) {
+    try {
+      const std::string pod_label_selector(
+          config_.KubernetesPodLabelSelector().empty()
+          ? "" : "&" + config_.KubernetesPodLabelSelector());
+
+      (void) QueryMaster(std::string(kKubernetesEndpointPath)
+                         + "/pods?limit=1" + pod_label_selector);
+    } catch (const NonRetriableError& e) {
+      throw MetadataUpdater::ValidationError(
+          "Pod query validation failed: " + e.what());
+    } catch (const QueryException& e) {
+      // Already logged.
+      if (i < kKubernetesValidationRetryLimit) {
+        std::this_thread::sleep_for(
+            time::seconds(kKubernetesValidationRetryDelaySeconds));
+      } else {
+        throw MetadataUpdater::ValidationError(
+            "Pod query retry limit reached: " + e.what());
+      }
+    }
   }
 
   if (CurrentNode().empty()) {
-    return false;
+    throw new MetadataUpdater::ValidationError("Current node cannot be empty");
   }
 
   return true;
@@ -1233,8 +1271,9 @@ KubernetesUpdater::KubernetesUpdater(const Configuration& config,
         config.KubernetesUpdaterIntervalSeconds(),
         [=]() { return reader_.MetadataQuery(); }) { }
 
-bool KubernetesUpdater::ValidateConfiguration() const {
-  if (!PollingMetadataUpdater::ValidateConfiguration()) {
+bool KubernetesUpdater::ValidateConfiguration() const throw(ValidationError) {
+  if (!PollingMetadataUpdater::ValidateConfiguration() &&
+      !config().KubernetesUseWatch()) {
     return false;
   }
 
diff --git a/src/kubernetes.h b/src/kubernetes.h
index ea620df3..4bb73749 100644
--- a/src/kubernetes.h
+++ b/src/kubernetes.h
@@ -45,7 +45,7 @@ class KubernetesReader {
 
   // Validates the relevant configuration and returns true if it's correct.
   // Returns a bool that represents if it's configured properly.
-  bool ValidateConfiguration() const;
+  bool ValidateConfiguration() const throw(MetadataUpdater::ValidationError);
 
   // Node watcher.
   void WatchNodes(const std::string& node_name,
@@ -75,6 +75,7 @@ class KubernetesReader {
    private:
     std::string explanation_;
   };
+  class NonRetriableError;
 
   // Issues a Kubernetes master API query at a given path and
   // returns a parsed JSON response. The path has to start with "/".
@@ -226,7 +227,7 @@ class KubernetesUpdater : public PollingMetadataUpdater {
   }
 
  protected:
-  bool ValidateConfiguration() const;
+  bool ValidateConfiguration() const throw(ValidationError);
   void StartUpdater();
 
  private:
diff --git a/src/updater.cc b/src/updater.cc
index 980555df..c90b74f5 100644
--- a/src/updater.cc
+++ b/src/updater.cc
@@ -19,6 +19,7 @@
 #include <chrono>
 
 #include "configuration.h"
+#include "format.h"
 #include "logging.h"
 
 namespace google {
@@ -29,7 +30,7 @@ MetadataUpdater::MetadataUpdater(const Configuration& config,
 
 MetadataUpdater::~MetadataUpdater() {}
 
-void MetadataUpdater::start() {
+void MetadataUpdater::start() throw(ValidationError) {
   if (!ValidateConfiguration()) {
     LOG(ERROR) << "Failed to validate configuration for " << name_;
     return;
@@ -58,8 +59,14 @@ PollingMetadataUpdater::~PollingMetadataUpdater() {
   }
 }
 
-bool PollingMetadataUpdater::ValidateConfiguration() const {
-  return period_ >= time::seconds::zero();
+bool PollingMetadataUpdater::ValidateConfiguration() const
+    throw(ValidationError) {
+  if (period_ < time::seconds::zero()) {
+    throw ValidationError(
+        format::Substitute("Polling period {{period}}s cannot be negative",
+                           {{"period", format::str(period_.count())}}));
+  }
+  return period_ > time::seconds::zero();
 }
 
 void PollingMetadataUpdater::StartUpdater() {
@@ -67,9 +74,7 @@ void PollingMetadataUpdater::StartUpdater() {
   if (config().VerboseLogging()) {
     LOG(INFO) << "Timer locked";
   }
-  if (period_ > time::seconds::zero()) {
-    reporter_thread_ = std::thread([=]() { PollForMetadata(); });
-  }
+  reporter_thread_ = std::thread([=]() { PollForMetadata(); });
 }
 
 void PollingMetadataUpdater::StopUpdater() {
diff --git a/src/updater.h b/src/updater.h
index 8205f912..6b7188cf 100644
--- a/src/updater.h
+++ b/src/updater.h
@@ -55,12 +55,21 @@ class MetadataUpdater {
     MetadataStore::Metadata metadata_;
   };
 
+  // A representation of all validation errors.
+  class ValidationError {
+   public:
+    ValidationError(const std::string& what) : explanation_(what) {}
+    const std::string& what() const { return explanation_; }
+   private:
+    std::string explanation_;
+  };
+
   MetadataUpdater(const Configuration& config, MetadataStore* store,
                   const std::string& name);
   virtual ~MetadataUpdater();
 
   // Starts updating.
-  void start();
+  void start() throw(ValidationError);
 
   // Stops updating.
   void stop();
@@ -71,9 +80,12 @@ class MetadataUpdater {
  protected:
   friend class UpdaterTest;
 
-  // Validates the relevant configuration and returns true if it's correct.
-  // Returns a bool that represents if it's configured properly.
-  virtual bool ValidateConfiguration() const {
+  // Validates the relevant configuration.
+  // If the configuration is invalid, throws a ValidationError, which is
+  // generally expected to pass through and terminate the program.
+  // Returns whether the updater logic should be started based on the current
+  // configuration.
+  virtual bool ValidateConfiguration() const throw(ValidationError) {
     return true;
   }
 
@@ -93,7 +105,7 @@ class MetadataUpdater {
     store_->UpdateMetadata(result.resource_, std::move(result.metadata_));
   }
 
-  const Configuration& config() {
+  const Configuration& config() const {
     return config_;
   }
 
@@ -119,7 +131,7 @@ class PollingMetadataUpdater : public MetadataUpdater {
  protected:
   friend class UpdaterTest;
 
-  bool ValidateConfiguration() const;
+  bool ValidateConfiguration() const throw(ValidationError);
   void StartUpdater();
   void StopUpdater();
 
diff --git a/test/Makefile b/test/Makefile
index b729dfda..153b9877 100644
--- a/test/Makefile
+++ b/test/Makefile
@@ -138,7 +138,7 @@ store_unittest: store_unittest.o $(SRC_DIR)/store.o $(SRC_DIR)/resource.o $(SRC_
 	$(CXX) $(LDFLAGS) $^ $(LDLIBS) -o $@
 time_unittest: time_unittest.o $(SRC_DIR)/time.o
 	$(CXX) $(LDFLAGS) $^ $(LDLIBS) -o $@
-updater_unittest: updater_unittest.o $(SRC_DIR)/updater.o $(SRC_DIR)/resource.o $(SRC_DIR)/store.o $(SRC_DIR)/configuration.o $(SRC_DIR)/logging.o $(SRC_DIR)/time.o $(SRC_DIR)/json.o
+updater_unittest: updater_unittest.o $(SRC_DIR)/updater.o $(SRC_DIR)/resource.o $(SRC_DIR)/store.o $(SRC_DIR)/configuration.o $(SRC_DIR)/logging.o $(SRC_DIR)/time.o $(SRC_DIR)/json.o $(SRC_DIR)/format.o
 	$(CXX) $(LDFLAGS) $^ $(LDLIBS) -o $@
 
 .PHONY: all test clean purge
diff --git a/test/updater_unittest.cc b/test/updater_unittest.cc
index 6dd53404..7410a992 100644
--- a/test/updater_unittest.cc
+++ b/test/updater_unittest.cc
@@ -36,19 +36,20 @@ class UpdaterTest : public ::testing::Test {
 
 namespace {
 
-TEST_F(UpdaterTest, OneMinutePollingIntervalIsValid) {
+TEST_F(UpdaterTest, OneMinutePollingIntervalEnablesUpdate) {
   PollingMetadataUpdater updater(config, &store, "Test", 60, nullptr);
   EXPECT_TRUE(ValidateConfiguration(&updater));
 }
 
-TEST_F(UpdaterTest, ZeroSecondPollingIntervalIsValid) {
+TEST_F(UpdaterTest, ZeroSecondPollingIntervalDisablesUpdate) {
   PollingMetadataUpdater updater(config, &store, "Test", 0, nullptr);
-  EXPECT_TRUE(ValidateConfiguration(&updater));
+  EXPECT_FALSE(ValidateConfiguration(&updater));
 }
 
 TEST_F(UpdaterTest, NegativePollingIntervalIsInvalid) {
   PollingMetadataUpdater updater(config, &store, "BadUpdater", -1, nullptr);
-  EXPECT_FALSE(ValidateConfiguration(&updater));
+  EXPECT_THROW(
+      ValidateConfiguration(&updater), MetadataUpdater::ValidationError);
 }
 
 TEST_F(UpdaterTest, UpdateMetadataCallback) {

From caf1d115e9a756fbda201a654f5146d2b4e4168e Mon Sep 17 00:00:00 2001
From: Igor Peshansky <igorpeshansky@github.com>
Date: Tue, 1 May 2018 10:21:08 -0400
Subject: [PATCH 2/6] Factor out retry functionality.

---
 src/docker.cc         | 48 ++++++++++++++--------------
 src/kubernetes.cc     | 72 ++++++++++++++++++++---------------------
 src/util.h            | 58 +++++++++++++++++++++++++++++++++
 test/Makefile         |  5 ++-
 test/util_unittest.cc | 74 +++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 194 insertions(+), 63 deletions(-)
 create mode 100644 src/util.h
 create mode 100644 test/util_unittest.cc

diff --git a/src/docker.cc b/src/docker.cc
index 2ea1e7a2..89d93ad2 100644
--- a/src/docker.cc
+++ b/src/docker.cc
@@ -29,6 +29,7 @@
 #include "resource.h"
 #include "store.h"
 #include "time.h"
+#include "util.h"
 
 namespace http = boost::network::http;
 
@@ -59,31 +60,28 @@ DockerReader::DockerReader(const Configuration& config)
 
 bool DockerReader::ValidateConfiguration() const
     throw(MetadataUpdater::ValidationError) {
-  for (int i = 0; true; ++i) {
-    try {
-      const std::string container_filter(
-          config_.DockerContainerFilter().empty()
-          ? "" : "&" + config_.DockerContainerFilter());
-
-      // A limit may exist in the container_filter, however, the docker API only
-      // uses the first limit provided in the query params.
-      (void) QueryDocker(std::string(kDockerEndpointPath) +
-                         "/json?all=true&limit=1" + container_filter);
-
-      return true;
-    } catch (const NonRetriableError& e) {
-      throw MetadataUpdater::ValidationError(
-          "Docker query validation failed: " + e.what());
-    } catch (const QueryException& e) {
-      // Already logged.
-      if (i < kDockerValidationRetryLimit) {
-        std::this_thread::sleep_for(
-          time::seconds(kDockerValidationRetryDelaySeconds));
-      } else {
-        throw MetadataUpdater::ValidationError(
-            "Docker query retry limit reached: " + e.what());
-      }
-    }
+  const std::string container_filter(
+      config_.DockerContainerFilter().empty()
+      ? "" : "&" + config_.DockerContainerFilter());
+
+  try {
+    util::Retry<NonRetriableError, QueryException>(
+        kDockerValidationRetryLimit,
+        time::seconds(kDockerValidationRetryDelaySeconds),
+        [this, &container_filter]() {
+          // A limit may exist in the container_filter, however, the docker API only
+          // uses the first limit provided in the query params.
+          (void) QueryDocker(std::string(kDockerEndpointPath) +
+                             "/json?all=true&limit=1" + container_filter);
+        });
+    return true;
+  } catch (const NonRetriableError& e) {
+    throw MetadataUpdater::ValidationError(
+        "Docker query validation failed: " + e.what());
+  } catch (const QueryException& e) {
+    // Already logged.
+    throw MetadataUpdater::ValidationError(
+        "Docker query retry limit reached: " + e.what());
   }
 }
 
diff --git a/src/kubernetes.cc b/src/kubernetes.cc
index 93f480fc..8621f8d7 100644
--- a/src/kubernetes.cc
+++ b/src/kubernetes.cc
@@ -36,6 +36,7 @@
 #include "resource.h"
 #include "store.h"
 #include "time.h"
+#include "util.h"
 
 namespace http = boost::network::http;
 
@@ -1073,45 +1074,42 @@ void KubernetesReader::UpdateServiceToPodsCache(
 
 bool KubernetesReader::ValidateConfiguration() const
     throw(MetadataUpdater::ValidationError) {
-  for (int i = 0; true; ++i) {
-    try {
-      (void) QueryMaster(std::string(kKubernetesEndpointPath) + "/nodes?limit=1");
-    } catch (const NonRetriableError& e) {
-      throw MetadataUpdater::ValidationError(
-          "Node query validation failed: " + e.what());
-    } catch (const QueryException& e) {
-      // Already logged.
-      if (i < kKubernetesValidationRetryLimit) {
-        std::this_thread::sleep_for(
-          time::seconds(kKubernetesValidationRetryDelaySeconds));
-      } else {
-        throw MetadataUpdater::ValidationError(
-            "Node query retry limit reached: " + e.what());
-      }
-    }
+  try {
+    util::Retry<NonRetriableError, QueryException>(
+        kKubernetesValidationRetryLimit,
+        time::seconds(kKubernetesValidationRetryDelaySeconds),
+        [this]() {
+          (void) QueryMaster(std::string(kKubernetesEndpointPath)
+                             + "/nodes?limit=1");
+        });
+  } catch (const NonRetriableError& e) {
+    throw MetadataUpdater::ValidationError(
+        "Node query validation failed: " + e.what());
+  } catch (const QueryException& e) {
+    // Already logged.
+    throw MetadataUpdater::ValidationError(
+        "Node query retry limit reached: " + e.what());
   }
 
-  for (int i = 0; true; ++i) {
-    try {
-      const std::string pod_label_selector(
-          config_.KubernetesPodLabelSelector().empty()
-          ? "" : "&" + config_.KubernetesPodLabelSelector());
-
-      (void) QueryMaster(std::string(kKubernetesEndpointPath)
-                         + "/pods?limit=1" + pod_label_selector);
-    } catch (const NonRetriableError& e) {
-      throw MetadataUpdater::ValidationError(
-          "Pod query validation failed: " + e.what());
-    } catch (const QueryException& e) {
-      // Already logged.
-      if (i < kKubernetesValidationRetryLimit) {
-        std::this_thread::sleep_for(
-            time::seconds(kKubernetesValidationRetryDelaySeconds));
-      } else {
-        throw MetadataUpdater::ValidationError(
-            "Pod query retry limit reached: " + e.what());
-      }
-    }
+  const std::string pod_label_selector(
+      config_.KubernetesPodLabelSelector().empty()
+      ? "" : "&" + config_.KubernetesPodLabelSelector());
+
+  try {
+    util::Retry<NonRetriableError, QueryException>(
+      kKubernetesValidationRetryLimit,
+      time::seconds(kKubernetesValidationRetryDelaySeconds),
+      [this, &pod_label_selector]() {
+        (void) QueryMaster(std::string(kKubernetesEndpointPath)
+                           + "/pods?limit=1" + pod_label_selector);
+      });
+  } catch (const NonRetriableError& e) {
+    throw MetadataUpdater::ValidationError(
+        "Pod query validation failed: " + e.what());
+  } catch (const QueryException& e) {
+    // Already logged.
+    throw MetadataUpdater::ValidationError(
+        "Pod query retry limit reached: " + e.what());
   }
 
   if (CurrentNode().empty()) {
diff --git a/src/util.h b/src/util.h
new file mode 100644
index 00000000..ef661188
--- /dev/null
+++ b/src/util.h
@@ -0,0 +1,58 @@
+/*
+ * Copyright 2018 Google Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ **/
+#ifndef UTIL_H_
+#define UTIL_H_
+
+#include <functional>
+#include <thread>
+
+#include "time.h"
+
+namespace google {
+
+namespace util {
+
+// Executes the work function while it throws exception Fail, up to a given
+// number of times, with a delay between executions. If it throws exception
+// Terminate, pass it through (no retries).
+template<class Terminate, class Fail>
+void Retry(int max_retries, time::seconds delay,
+           std::function<void()> work);
+
+template<class Terminate, class Fail>
+void Retry(int max_retries, time::seconds delay,
+           std::function<void()> work) {
+  for (int i = 0; true; ++i) {
+    try {
+      work();
+      return;
+    } catch (const Terminate& e) {
+      throw e;
+    } catch (const Fail& e) {
+      if (i < max_retries - 1) {
+        std::this_thread::sleep_for(delay);
+      } else {
+        throw e;
+      }
+    }
+  }
+}
+
+}  // util
+
+}  // google
+
+#endif  // UTIL_H_
diff --git a/test/Makefile b/test/Makefile
index 153b9877..645a1754 100644
--- a/test/Makefile
+++ b/test/Makefile
@@ -59,7 +59,8 @@ TESTS=\
       resource_unittest \
       store_unittest \
       time_unittest \
-      updater_unittest
+      updater_unittest \
+      util_unittest
 
 GTEST_LIB=gtest_lib.a
 
@@ -140,5 +141,7 @@ time_unittest: time_unittest.o $(SRC_DIR)/time.o
 	$(CXX) $(LDFLAGS) $^ $(LDLIBS) -o $@
 updater_unittest: updater_unittest.o $(SRC_DIR)/updater.o $(SRC_DIR)/resource.o $(SRC_DIR)/store.o $(SRC_DIR)/configuration.o $(SRC_DIR)/logging.o $(SRC_DIR)/time.o $(SRC_DIR)/json.o $(SRC_DIR)/format.o
 	$(CXX) $(LDFLAGS) $^ $(LDLIBS) -o $@
+util_unittest: util_unittest.o
+	$(CXX) $(LDFLAGS) $^ $(LDLIBS) -o $@
 
 .PHONY: all test clean purge
diff --git a/test/util_unittest.cc b/test/util_unittest.cc
new file mode 100644
index 00000000..b24b5afd
--- /dev/null
+++ b/test/util_unittest.cc
@@ -0,0 +1,74 @@
+#include "../src/util.h"
+#include "gtest/gtest.h"
+
+#include "../src/time.h"
+
+namespace google {
+
+namespace {
+
+class Fail {};
+
+class Terminate {};
+
+class TerminateSubclass : public Fail {};
+
+TEST(RetryTest, NoRetriesOnSuccess) {
+  int invocation_count = 0;
+  EXPECT_NO_THROW((util::Retry<Terminate, Fail>(
+      10, time::seconds(0.01),
+      [&invocation_count]() { ++invocation_count; })));
+  EXPECT_EQ(1, invocation_count);
+}
+
+TEST(RetryTest, RetryOnFail) {
+  int invocation_count = 0;
+  EXPECT_THROW(
+      (util::Retry<Terminate, Fail>(
+           10, time::seconds(0.01),
+           [&invocation_count]() { ++invocation_count; throw Fail(); })),
+      Fail);
+  EXPECT_EQ(10, invocation_count);
+}
+
+TEST(RetryTest, NoRetryOnTerminate) {
+  int invocation_count = 0;
+  EXPECT_THROW(
+      (util::Retry<Terminate, Fail>(
+           10, time::seconds(0.01),
+           [&invocation_count]() { ++invocation_count; throw Terminate(); })),
+      Terminate);
+  EXPECT_EQ(1, invocation_count);
+}
+
+TEST(RetryTest, RetryWhileFail) {
+  int invocation_count = 0;
+  EXPECT_THROW(
+      (util::Retry<Terminate, Fail>(
+           10, time::seconds(0.01),
+           [&invocation_count]() {
+             if (++invocation_count < 3) {
+               throw Fail();
+             } else {
+               throw Terminate();
+             }
+           })),
+      Terminate);
+  EXPECT_EQ(3, invocation_count);
+}
+
+TEST(RetryTest, RetryWithSubclass) {
+  int invocation_count = 0;
+  EXPECT_THROW((util::Retry<TerminateSubclass, Fail>(
+      10, time::seconds(0.01),
+      [&invocation_count]() {
+        if (++invocation_count < 5) {
+          throw Fail();
+        } else {
+          throw TerminateSubclass();
+        }
+      })), TerminateSubclass);
+  EXPECT_EQ(5, invocation_count);
+}
+}  // namespace
+}  // namespace google

From ac1e1f6dccd9a4839603b8503d1889386f572f26 Mon Sep 17 00:00:00 2001
From: Igor Peshansky <igorpeshansky@github.com>
Date: Tue, 1 May 2018 10:30:35 -0400
Subject: [PATCH 3/6] Rename ValidationError to ConfigurationValidationError in
 MetadataUpdater.

---
 src/docker.cc            |  9 +++++----
 src/docker.h             |  5 +++--
 src/kubernetes.cc        | 16 +++++++++-------
 src/kubernetes.h         |  5 +++--
 src/updater.cc           |  6 +++---
 src/updater.h            | 16 +++++++++-------
 test/updater_unittest.cc |  3 ++-
 7 files changed, 34 insertions(+), 26 deletions(-)

diff --git a/src/docker.cc b/src/docker.cc
index 89d93ad2..b014696d 100644
--- a/src/docker.cc
+++ b/src/docker.cc
@@ -59,7 +59,7 @@ DockerReader::DockerReader(const Configuration& config)
     : config_(config), environment_(config) {}
 
 bool DockerReader::ValidateConfiguration() const
-    throw(MetadataUpdater::ValidationError) {
+    throw(MetadataUpdater::ConfigurationValidationError) {
   const std::string container_filter(
       config_.DockerContainerFilter().empty()
       ? "" : "&" + config_.DockerContainerFilter());
@@ -76,11 +76,11 @@ bool DockerReader::ValidateConfiguration() const
         });
     return true;
   } catch (const NonRetriableError& e) {
-    throw MetadataUpdater::ValidationError(
+    throw MetadataUpdater::ConfigurationValidationError(
         "Docker query validation failed: " + e.what());
   } catch (const QueryException& e) {
     // Already logged.
-    throw MetadataUpdater::ValidationError(
+    throw MetadataUpdater::ConfigurationValidationError(
         "Docker query retry limit reached: " + e.what());
   }
 }
@@ -231,7 +231,8 @@ json::value DockerReader::QueryDocker(const std::string& path) const
   }
 }
 
-bool DockerUpdater::ValidateConfiguration() const throw(ValidationError) {
+bool DockerUpdater::ValidateConfiguration() const
+    throw(ConfigurationValidationError) {
   if (!PollingMetadataUpdater::ValidateConfiguration()) {
     return false;
   }
diff --git a/src/docker.h b/src/docker.h
index e72bacd6..2938aaf7 100644
--- a/src/docker.h
+++ b/src/docker.h
@@ -37,7 +37,8 @@ class DockerReader {
 
   // Validates the relevant configuration and returns true if it's correct.
   // Returns a bool that represents if it's configured properly.
-  bool ValidateConfiguration() const throw(MetadataUpdater::ValidationError);
+  bool ValidateConfiguration() const
+      throw(MetadataUpdater::ConfigurationValidationError);
 
  private:
   // A representation of all query-related errors.
@@ -73,7 +74,7 @@ class DockerUpdater : public PollingMetadataUpdater {
           [=]() { return reader_.MetadataQuery(); }) { }
 
  protected:
-  bool ValidateConfiguration() const throw(ValidationError);
+  bool ValidateConfiguration() const throw(ConfigurationValidationError);
 
  private:
   DockerReader reader_;
diff --git a/src/kubernetes.cc b/src/kubernetes.cc
index 8621f8d7..53705a52 100644
--- a/src/kubernetes.cc
+++ b/src/kubernetes.cc
@@ -1073,7 +1073,7 @@ void KubernetesReader::UpdateServiceToPodsCache(
 }
 
 bool KubernetesReader::ValidateConfiguration() const
-    throw(MetadataUpdater::ValidationError) {
+    throw(MetadataUpdater::ConfigurationValidationError) {
   try {
     util::Retry<NonRetriableError, QueryException>(
         kKubernetesValidationRetryLimit,
@@ -1083,11 +1083,11 @@ bool KubernetesReader::ValidateConfiguration() const
                              + "/nodes?limit=1");
         });
   } catch (const NonRetriableError& e) {
-    throw MetadataUpdater::ValidationError(
+    throw MetadataUpdater::ConfigurationValidationError(
         "Node query validation failed: " + e.what());
   } catch (const QueryException& e) {
     // Already logged.
-    throw MetadataUpdater::ValidationError(
+    throw MetadataUpdater::ConfigurationValidationError(
         "Node query retry limit reached: " + e.what());
   }
 
@@ -1104,16 +1104,17 @@ bool KubernetesReader::ValidateConfiguration() const
                            + "/pods?limit=1" + pod_label_selector);
       });
   } catch (const NonRetriableError& e) {
-    throw MetadataUpdater::ValidationError(
+    throw MetadataUpdater::ConfigurationValidationError(
         "Pod query validation failed: " + e.what());
   } catch (const QueryException& e) {
     // Already logged.
-    throw MetadataUpdater::ValidationError(
+    throw MetadataUpdater::ConfigurationValidationError(
         "Pod query retry limit reached: " + e.what());
   }
 
   if (CurrentNode().empty()) {
-    throw new MetadataUpdater::ValidationError("Current node cannot be empty");
+    throw new MetadataUpdater::ConfigurationValidationError(
+        "Current node cannot be empty");
   }
 
   return true;
@@ -1269,7 +1270,8 @@ KubernetesUpdater::KubernetesUpdater(const Configuration& config,
         config.KubernetesUpdaterIntervalSeconds(),
         [=]() { return reader_.MetadataQuery(); }) { }
 
-bool KubernetesUpdater::ValidateConfiguration() const throw(ValidationError) {
+bool KubernetesUpdater::ValidateConfiguration() const
+    throw(ConfigurationValidationError) {
   if (!PollingMetadataUpdater::ValidateConfiguration() &&
       !config().KubernetesUseWatch()) {
     return false;
diff --git a/src/kubernetes.h b/src/kubernetes.h
index 4bb73749..a28a560c 100644
--- a/src/kubernetes.h
+++ b/src/kubernetes.h
@@ -45,7 +45,8 @@ class KubernetesReader {
 
   // Validates the relevant configuration and returns true if it's correct.
   // Returns a bool that represents if it's configured properly.
-  bool ValidateConfiguration() const throw(MetadataUpdater::ValidationError);
+  bool ValidateConfiguration() const
+      throw(MetadataUpdater::ConfigurationValidationError);
 
   // Node watcher.
   void WatchNodes(const std::string& node_name,
@@ -227,7 +228,7 @@ class KubernetesUpdater : public PollingMetadataUpdater {
   }
 
  protected:
-  bool ValidateConfiguration() const throw(ValidationError);
+  bool ValidateConfiguration() const throw(ConfigurationValidationError);
   void StartUpdater();
 
  private:
diff --git a/src/updater.cc b/src/updater.cc
index c90b74f5..91a276c2 100644
--- a/src/updater.cc
+++ b/src/updater.cc
@@ -30,7 +30,7 @@ MetadataUpdater::MetadataUpdater(const Configuration& config,
 
 MetadataUpdater::~MetadataUpdater() {}
 
-void MetadataUpdater::start() throw(ValidationError) {
+void MetadataUpdater::start() throw(ConfigurationValidationError) {
   if (!ValidateConfiguration()) {
     LOG(ERROR) << "Failed to validate configuration for " << name_;
     return;
@@ -60,9 +60,9 @@ PollingMetadataUpdater::~PollingMetadataUpdater() {
 }
 
 bool PollingMetadataUpdater::ValidateConfiguration() const
-    throw(ValidationError) {
+    throw(ConfigurationValidationError) {
   if (period_ < time::seconds::zero()) {
-    throw ValidationError(
+    throw ConfigurationValidationError(
         format::Substitute("Polling period {{period}}s cannot be negative",
                            {{"period", format::str(period_.count())}}));
   }
diff --git a/src/updater.h b/src/updater.h
index 6b7188cf..3210a921 100644
--- a/src/updater.h
+++ b/src/updater.h
@@ -56,9 +56,10 @@ class MetadataUpdater {
   };
 
   // A representation of all validation errors.
-  class ValidationError {
+  class ConfigurationValidationError {
    public:
-    ValidationError(const std::string& what) : explanation_(what) {}
+    ConfigurationValidationError(const std::string& what)
+        : explanation_(what) {}
     const std::string& what() const { return explanation_; }
    private:
     std::string explanation_;
@@ -69,7 +70,7 @@ class MetadataUpdater {
   virtual ~MetadataUpdater();
 
   // Starts updating.
-  void start() throw(ValidationError);
+  void start() throw(ConfigurationValidationError);
 
   // Stops updating.
   void stop();
@@ -81,11 +82,12 @@ class MetadataUpdater {
   friend class UpdaterTest;
 
   // Validates the relevant configuration.
-  // If the configuration is invalid, throws a ValidationError, which is
-  // generally expected to pass through and terminate the program.
+  // If the configuration is invalid, throws a ConfigurationValidationError,
+  // which is generally expected to pass through and terminate the program.
   // Returns whether the updater logic should be started based on the current
   // configuration.
-  virtual bool ValidateConfiguration() const throw(ValidationError) {
+  virtual bool ValidateConfiguration() const
+      throw(ConfigurationValidationError) {
     return true;
   }
 
@@ -131,7 +133,7 @@ class PollingMetadataUpdater : public MetadataUpdater {
  protected:
   friend class UpdaterTest;
 
-  bool ValidateConfiguration() const throw(ValidationError);
+  bool ValidateConfiguration() const throw(ConfigurationValidationError);
   void StartUpdater();
   void StopUpdater();
 
diff --git a/test/updater_unittest.cc b/test/updater_unittest.cc
index 7410a992..39c1f75e 100644
--- a/test/updater_unittest.cc
+++ b/test/updater_unittest.cc
@@ -49,7 +49,8 @@ TEST_F(UpdaterTest, ZeroSecondPollingIntervalDisablesUpdate) {
 TEST_F(UpdaterTest, NegativePollingIntervalIsInvalid) {
   PollingMetadataUpdater updater(config, &store, "BadUpdater", -1, nullptr);
   EXPECT_THROW(
-      ValidateConfiguration(&updater), MetadataUpdater::ValidationError);
+      ValidateConfiguration(&updater),
+      MetadataUpdater::ConfigurationValidationError);
 }
 
 TEST_F(UpdaterTest, UpdateMetadataCallback) {

From 61d88be350daf4f38fd5aa405e260663028f56f9 Mon Sep 17 00:00:00 2001
From: Igor Peshansky <igorpeshansky@github.com>
Date: Tue, 1 May 2018 10:54:50 -0400
Subject: [PATCH 4/6] Split the bool part of ValidateConfiguration into
 ShouldStartUpdater.

---
 src/docker.cc            | 12 ++++--------
 src/docker.h             |  7 +++----
 src/kubernetes.cc        | 18 +++++++++---------
 src/kubernetes.h         |  9 +++++----
 src/updater.cc           | 16 ++++++++++------
 src/updater.h            | 11 +++++++----
 test/updater_unittest.cc | 28 +++++++++++++++++++++-------
 7 files changed, 59 insertions(+), 42 deletions(-)

diff --git a/src/docker.cc b/src/docker.cc
index b014696d..bf941cc5 100644
--- a/src/docker.cc
+++ b/src/docker.cc
@@ -58,7 +58,7 @@ class DockerReader::NonRetriableError : public DockerReader::QueryException {
 DockerReader::DockerReader(const Configuration& config)
     : config_(config), environment_(config) {}
 
-bool DockerReader::ValidateConfiguration() const
+void DockerReader::ValidateConfiguration() const
     throw(MetadataUpdater::ConfigurationValidationError) {
   const std::string container_filter(
       config_.DockerContainerFilter().empty()
@@ -74,7 +74,6 @@ bool DockerReader::ValidateConfiguration() const
           (void) QueryDocker(std::string(kDockerEndpointPath) +
                              "/json?all=true&limit=1" + container_filter);
         });
-    return true;
   } catch (const NonRetriableError& e) {
     throw MetadataUpdater::ConfigurationValidationError(
         "Docker query validation failed: " + e.what());
@@ -231,13 +230,10 @@ json::value DockerReader::QueryDocker(const std::string& path) const
   }
 }
 
-bool DockerUpdater::ValidateConfiguration() const
+void DockerUpdater::ValidateConfiguration() const
     throw(ConfigurationValidationError) {
-  if (!PollingMetadataUpdater::ValidateConfiguration()) {
-    return false;
-  }
-
-  return reader_.ValidateConfiguration();
+  PollingMetadataUpdater::ValidateConfiguration();
+  reader_.ValidateConfiguration();
 }
 
 }
diff --git a/src/docker.h b/src/docker.h
index 2938aaf7..1725118b 100644
--- a/src/docker.h
+++ b/src/docker.h
@@ -35,9 +35,8 @@ class DockerReader {
   // A Docker metadata query function.
   std::vector<MetadataUpdater::ResourceMetadata> MetadataQuery() const;
 
-  // Validates the relevant configuration and returns true if it's correct.
-  // Returns a bool that represents if it's configured properly.
-  bool ValidateConfiguration() const
+  // Validates the relevant configuration and throws if it's incorrect.
+  void ValidateConfiguration() const
       throw(MetadataUpdater::ConfigurationValidationError);
 
  private:
@@ -74,7 +73,7 @@ class DockerUpdater : public PollingMetadataUpdater {
           [=]() { return reader_.MetadataQuery(); }) { }
 
  protected:
-  bool ValidateConfiguration() const throw(ConfigurationValidationError);
+  void ValidateConfiguration() const throw(ConfigurationValidationError);
 
  private:
   DockerReader reader_;
diff --git a/src/kubernetes.cc b/src/kubernetes.cc
index 53705a52..0a525a30 100644
--- a/src/kubernetes.cc
+++ b/src/kubernetes.cc
@@ -1072,7 +1072,7 @@ void KubernetesReader::UpdateServiceToPodsCache(
   }
 }
 
-bool KubernetesReader::ValidateConfiguration() const
+void KubernetesReader::ValidateConfiguration() const
     throw(MetadataUpdater::ConfigurationValidationError) {
   try {
     util::Retry<NonRetriableError, QueryException>(
@@ -1116,8 +1116,6 @@ bool KubernetesReader::ValidateConfiguration() const
     throw new MetadataUpdater::ConfigurationValidationError(
         "Current node cannot be empty");
   }
-
-  return true;
 }
 
 void KubernetesReader::PodCallback(
@@ -1270,14 +1268,16 @@ KubernetesUpdater::KubernetesUpdater(const Configuration& config,
         config.KubernetesUpdaterIntervalSeconds(),
         [=]() { return reader_.MetadataQuery(); }) { }
 
-bool KubernetesUpdater::ValidateConfiguration() const
+void KubernetesUpdater::ValidateConfiguration() const
     throw(ConfigurationValidationError) {
-  if (!PollingMetadataUpdater::ValidateConfiguration() &&
-      !config().KubernetesUseWatch()) {
-    return false;
-  }
+  PollingMetadataUpdater::ValidateConfiguration();
+  reader_.ValidateConfiguration();
+}
 
-  return reader_.ValidateConfiguration();
+bool KubernetesUpdater::ShouldStartUpdater() const {
+  return
+      PollingMetadataUpdater::ShouldStartUpdater() ||
+      config().KubernetesUseWatch();
 }
 
 void KubernetesUpdater::StartUpdater() {
diff --git a/src/kubernetes.h b/src/kubernetes.h
index a28a560c..d5d7d9dc 100644
--- a/src/kubernetes.h
+++ b/src/kubernetes.h
@@ -43,9 +43,8 @@ class KubernetesReader {
   // A Kubernetes metadata query function.
   std::vector<MetadataUpdater::ResourceMetadata> MetadataQuery() const;
 
-  // Validates the relevant configuration and returns true if it's correct.
-  // Returns a bool that represents if it's configured properly.
-  bool ValidateConfiguration() const
+  // Validates the relevant configuration and throws if it's incorrect.
+  void ValidateConfiguration() const
       throw(MetadataUpdater::ConfigurationValidationError);
 
   // Node watcher.
@@ -228,7 +227,9 @@ class KubernetesUpdater : public PollingMetadataUpdater {
   }
 
  protected:
-  bool ValidateConfiguration() const throw(ConfigurationValidationError);
+  void ValidateConfiguration() const throw(ConfigurationValidationError);
+  bool ShouldStartUpdater() const;
+
   void StartUpdater();
 
  private:
diff --git a/src/updater.cc b/src/updater.cc
index 91a276c2..293d4b57 100644
--- a/src/updater.cc
+++ b/src/updater.cc
@@ -31,12 +31,13 @@ MetadataUpdater::MetadataUpdater(const Configuration& config,
 MetadataUpdater::~MetadataUpdater() {}
 
 void MetadataUpdater::start() throw(ConfigurationValidationError) {
-  if (!ValidateConfiguration()) {
-    LOG(ERROR) << "Failed to validate configuration for " << name_;
-    return;
-  }
+  ValidateConfiguration();
 
-  StartUpdater();
+  if (ShouldStartUpdater()) {
+    StartUpdater();
+  } else {
+    LOG(INFO) << "Not starting " << name_;
+  }
 }
 
 void MetadataUpdater::stop() {
@@ -59,13 +60,16 @@ PollingMetadataUpdater::~PollingMetadataUpdater() {
   }
 }
 
-bool PollingMetadataUpdater::ValidateConfiguration() const
+void PollingMetadataUpdater::ValidateConfiguration() const
     throw(ConfigurationValidationError) {
   if (period_ < time::seconds::zero()) {
     throw ConfigurationValidationError(
         format::Substitute("Polling period {{period}}s cannot be negative",
                            {{"period", format::str(period_.count())}}));
   }
+}
+
+bool PollingMetadataUpdater::ShouldStartUpdater() const {
   return period_ > time::seconds::zero();
 }
 
diff --git a/src/updater.h b/src/updater.h
index 3210a921..e26276be 100644
--- a/src/updater.h
+++ b/src/updater.h
@@ -84,10 +84,12 @@ class MetadataUpdater {
   // Validates the relevant configuration.
   // If the configuration is invalid, throws a ConfigurationValidationError,
   // which is generally expected to pass through and terminate the program.
-  // Returns whether the updater logic should be started based on the current
+  virtual void ValidateConfiguration() const
+      throw(ConfigurationValidationError) {}
+
+  // Decides whether the updater logic should be started based on the current
   // configuration.
-  virtual bool ValidateConfiguration() const
-      throw(ConfigurationValidationError) {
+  virtual bool ShouldStartUpdater() const {
     return true;
   }
 
@@ -133,7 +135,8 @@ class PollingMetadataUpdater : public MetadataUpdater {
  protected:
   friend class UpdaterTest;
 
-  bool ValidateConfiguration() const throw(ConfigurationValidationError);
+  void ValidateConfiguration() const throw(ConfigurationValidationError);
+  bool ShouldStartUpdater() const;
   void StartUpdater();
   void StopUpdater();
 
diff --git a/test/updater_unittest.cc b/test/updater_unittest.cc
index 39c1f75e..030e1dca 100644
--- a/test/updater_unittest.cc
+++ b/test/updater_unittest.cc
@@ -14,8 +14,12 @@ class UpdaterTest : public ::testing::Test {
   // query_metadata function not needed to test callbacks.
   UpdaterTest() : config(), store(config) {}
 
-  static bool ValidateConfiguration(MetadataUpdater* updater) {
-    return updater->ValidateConfiguration();
+  static void ValidateConfiguration(MetadataUpdater* updater) {
+    updater->ValidateConfiguration();
+  }
+
+  static bool ShouldStartUpdater(MetadataUpdater* updater) {
+    return updater->ShouldStartUpdater();
   }
 
   static void UpdateMetadataCallback(
@@ -36,23 +40,33 @@ class UpdaterTest : public ::testing::Test {
 
 namespace {
 
-TEST_F(UpdaterTest, OneMinutePollingIntervalEnablesUpdate) {
+TEST_F(UpdaterTest, ValidateConfiguration_OneMinutePollingIntervalIsValid) {
   PollingMetadataUpdater updater(config, &store, "Test", 60, nullptr);
-  EXPECT_TRUE(ValidateConfiguration(&updater));
+  EXPECT_NO_THROW(ValidateConfiguration(&updater));
 }
 
-TEST_F(UpdaterTest, ZeroSecondPollingIntervalDisablesUpdate) {
+TEST_F(UpdaterTest, ValidateConfiguration_ZeroSecondPollingIntervalIsValid) {
   PollingMetadataUpdater updater(config, &store, "Test", 0, nullptr);
-  EXPECT_FALSE(ValidateConfiguration(&updater));
+  EXPECT_NO_THROW(ValidateConfiguration(&updater));
 }
 
-TEST_F(UpdaterTest, NegativePollingIntervalIsInvalid) {
+TEST_F(UpdaterTest, ValidateConfiguration_NegativePollingIntervalIsInvalid) {
   PollingMetadataUpdater updater(config, &store, "BadUpdater", -1, nullptr);
   EXPECT_THROW(
       ValidateConfiguration(&updater),
       MetadataUpdater::ConfigurationValidationError);
 }
 
+TEST_F(UpdaterTest, ShouldStart_OneMinutePollingIntervalEnablesUpdate) {
+  PollingMetadataUpdater updater(config, &store, "Test", 60, nullptr);
+  EXPECT_TRUE(ShouldStartUpdater(&updater));
+}
+
+TEST_F(UpdaterTest, ShouldStart_ZeroSecondPollingIntervalDisablesUpdate) {
+  PollingMetadataUpdater updater(config, &store, "Test", 0, nullptr);
+  EXPECT_FALSE(ShouldStartUpdater(&updater));
+}
+
 TEST_F(UpdaterTest, UpdateMetadataCallback) {
   MetadataStore::Metadata m(
       "test-version",

From dc64b1d340810725890ba38964dd803c5ee7f2dc Mon Sep 17 00:00:00 2001
From: Igor Peshansky <igorpeshansky@github.com>
Date: Tue, 1 May 2018 11:22:24 -0400
Subject: [PATCH 5/6] Clarify comment.

---
 src/util.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/util.h b/src/util.h
index ef661188..44193bf7 100644
--- a/src/util.h
+++ b/src/util.h
@@ -27,7 +27,8 @@ namespace util {
 
 // Executes the work function while it throws exception Fail, up to a given
 // number of times, with a delay between executions. If it throws exception
-// Terminate, pass it through (no retries).
+// Terminate, which may be a subclass of Fail, pass it through (no retries).
+// Any other exception outside of the Fail hierarchy will be passed through.
 template<class Terminate, class Fail>
 void Retry(int max_retries, time::seconds delay,
            std::function<void()> work);
@@ -39,7 +40,7 @@ void Retry(int max_retries, time::seconds delay,
     try {
       work();
       return;
-    } catch (const Terminate& e) {
+    } catch (const Terminate& e) {  // Catch first due to inheritance.
       throw e;
     } catch (const Fail& e) {
       if (i < max_retries - 1) {

From 0e0b8f2bfeccaed86b90dd97d6bcff7a05ac01da Mon Sep 17 00:00:00 2001
From: Igor Peshansky <igorpeshansky@github.com>
Date: Tue, 1 May 2018 12:23:39 -0400
Subject: [PATCH 6/6] Minor message tweak.

---
 src/docker.cc     | 2 +-
 src/kubernetes.cc | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/docker.cc b/src/docker.cc
index bf941cc5..1850e699 100644
--- a/src/docker.cc
+++ b/src/docker.cc
@@ -80,7 +80,7 @@ void DockerReader::ValidateConfiguration() const
   } catch (const QueryException& e) {
     // Already logged.
     throw MetadataUpdater::ConfigurationValidationError(
-        "Docker query retry limit reached: " + e.what());
+        "Docker query validation retry limit reached: " + e.what());
   }
 }
 
diff --git a/src/kubernetes.cc b/src/kubernetes.cc
index 0a525a30..c1f2695a 100644
--- a/src/kubernetes.cc
+++ b/src/kubernetes.cc
@@ -1088,7 +1088,7 @@ void KubernetesReader::ValidateConfiguration() const
   } catch (const QueryException& e) {
     // Already logged.
     throw MetadataUpdater::ConfigurationValidationError(
-        "Node query retry limit reached: " + e.what());
+        "Node query validation retry limit reached: " + e.what());
   }
 
   const std::string pod_label_selector(
@@ -1109,7 +1109,7 @@ void KubernetesReader::ValidateConfiguration() const
   } catch (const QueryException& e) {
     // Already logged.
     throw MetadataUpdater::ConfigurationValidationError(
-        "Pod query retry limit reached: " + e.what());
+        "Pod query validation retry limit reached: " + e.what());
   }
 
   if (CurrentNode().empty()) {