Add validation to only start updaters if they're configured properly. #63

bmoyles0117 · 2018-02-26T22:06:49Z

No description provided.

supriyagarg

Minor comments.

supriyagarg · 2018-02-26T22:14:11Z

src/kubernetes.cc

+const bool KubernetesReader::IsConfigured() const {
+  try {
+    json::value raw_node = QueryMaster(
+        std::string(kKubernetesEndpointPath) + "/nodes");


It would be useful to minimize output by using fieldSelector (or limit=0 if that works)

https://v1-8.docs.kubernetes.io/docs/api-reference/v1.8/#list-474

I've limited it to just the node we're running on.

supriyagarg · 2018-02-26T22:16:46Z

src/docker.cc


+const bool DockerReader::IsConfigured() const {
+  try {
+    QueryDocker(std::string(kDockerEndpointPath) + "/json?all=true");


It would be useful to minimize output by using limit / filter options:
https://docs.docker.com/engine/api/v1.24/#31-containers

There's a DockerContainerFilter config option — you should probably be checking that...

igorpeshansky

Do we need to verify the InstanceUpdater as well?

igorpeshansky · 2018-02-27T00:41:15Z

src/docker.cc


+const bool DockerReader::IsConfigured() const {
+  try {
+    QueryDocker(std::string(kDockerEndpointPath) + "/json?all=true");


There's a DockerContainerFilter config option — you should probably be checking that...

igorpeshansky · 2018-02-27T00:42:44Z

src/docker.cc

 DockerReader::DockerReader(const MetadataAgentConfiguration& config)
    : config_(config), environment_(config) {}

+const bool DockerReader::IsConfigured() const {


const bool is redundant. Just use bool.

I'd call this ValidateConfiguration.

igorpeshansky · 2018-02-27T01:04:47Z

src/docker.cc

 }

+void DockerUpdater::start() {
+  if (!reader_.IsConfigured()) {


Let's define a virtual VerifyConfiguration function in PollingMetadataUpdater, make it return true by default, call it from start in the superclass, and override it here. We might also have to give each updater object a name, so we can identify it in the logs.

igorpeshansky · 2018-02-27T01:05:29Z

src/docker.cc

+    return;
+  }
+
+  if (config_.VerboseLogging()) {


Updater::start returns a bool so we can signal an upstream failure.

igorpeshansky · 2018-02-27T18:30:22Z

src/reader.h

+
+class MetadataReader {
+ public:
+  virtual bool ValidateConfiguration() const;


There's no reason to have a superclass for all readers, actually. I would keep this independently in KubernetesReader and DockerReader. Let's just revert 89d6cf8.

The reader is the thing that's actually interacting with the endpoints. I'll talk with you offline about this to build additional context together with you, but when I tried implementing the validation in the updater it just resulted in transferred responsibility down to the reader.

It's ok for each updater to pass the responsibility down to individual readers — that's why I suggested having a ValidateConfiguration function in each reader that needs it. I'm simply saying that it's not necessary to have the validation in the reader, and we should abstract that away. However, the polling updater needs to perform the validation before starting the polling threads, so let's document that in a virtual function.
Happy to discuss offline.

Discussed and addressed offline.

igorpeshansky · 2018-02-27T18:31:36Z

src/kubernetes.h

  // A Kubernetes metadata query function.
  std::vector<MetadataUpdater::ResourceMetadata> MetadataQuery() const;

+  // Validates that the reader is configured properly.


How about just:

// Validates the relevant configuration and returns true if it's correct.

? Also in other files.

igorpeshansky · 2018-02-27T18:34:38Z

src/docker.cc

+        config_.DockerContainerFilter().empty()
+        ? "" : "&" + config_.DockerContainerFilter());
+
+    QueryDocker(std::string(kDockerEndpointPath) + "/json?all=true&limit=1" + container_filter);


Careful -- config_.DockerContainerFilter() may already include a limit. Should investigate which one takes precedence -- you may have to move your &limit=1 to the end.

Great catch, I did some local testing and it appears that the docker server discards the second value, when two limits are sent in the URL. We'll leave it as-is for now.

Let's add a comment here...

igorpeshansky · 2018-02-27T18:37:49Z

src/kubernetes.cc

+bool KubernetesReader::ValidateConfiguration() const {
+  try {
+    json::value raw_node = QueryMaster(
+        std::string(kKubernetesEndpointPath) + "/nodes/" + CurrentNode());


Should also validate config_.KubernetesPodLabelSelector().

This interaction is just validating that we can communicate with the master, adding a secondary request for pods is redundant, and would be more symptomatic of a failure inside of kubernetes, or a transient failure between requests.

If so, then this is redundant, since CurrentNode issues two QueryMaster calls. So just calling CurrentNode is more than enough. Or you can call QueryMaster with a filter for one (random) node...

I think we should continue to query for something like nodes, as the node name can be overridden by configuration, which would prevent the master interaction at all.

Right, as discussed, it makes sense to issue a QueryMaster on something that doesn't depend on the CurrentNode value, and keep CurrentNode as a secondary check.

igorpeshansky · 2018-02-27T18:38:45Z

src/kubernetes.cc

+bool KubernetesReader::ValidateConfiguration() const {
+  try {
+    json::value raw_node = QueryMaster(
+        std::string(kKubernetesEndpointPath) + "/nodes/" + CurrentNode());


CurrentNode() may be empty if, e.g., the API token is invalid (so you'd get 403s). Should we check that as well?

Great point, done.

igorpeshansky · 2018-02-27T18:42:04Z

src/updater.cc


-void PollingMetadataUpdater::start() {
+bool PollingMetadataUpdater::start() {
+  if (!reader_->ValidateConfiguration()) {


Let's just make ValidateConfiguration a virtual method in PollingMetadataUpdater.

Will address as a part of the conversation for the reader above.

igorpeshansky · 2018-02-27T18:42:32Z

src/updater.h


  // Starts updating.
-  virtual void start() = 0;
+  virtual bool start() = 0;


Why bool? If the configuration is invalid, a start is a no-op, isn't it?

I needed a signal for the KubernetesUpdater to know that the parent updater had started successfully, before firing off the watch threads. The two options I thought of were return a bool or raise an exception in the PollingMetadataUpdater start. Thoughts?

If it weren't for the desire to share a reader object, and the possible need to validate the watch and polling options together, I would have been considering splitting the watch and the poller into separate updaters.
I've already suggested using composition over inheritance here. But we could also have a StartThreads virtual function that actually starts the poller thread and is called from the (non-virtual) start after validation succeeds.
Thoughts?

Discussed and addressed offline.

igorpeshansky · 2018-02-27T18:44:09Z

src/updater.cc

-MetadataUpdater::MetadataUpdater(MetadataAgent* store)
-    : store_(store) {}
+MetadataUpdater::MetadataUpdater(MetadataAgent* store, MetadataReader* reader, std::string name)
+    : store_(store),


You can put all of these on one line.

igorpeshansky · 2018-02-27T18:45:51Z

src/updater.h

  };

-  MetadataUpdater(MetadataAgent* store);
+  MetadataUpdater(MetadataAgent* store, MetadataReader* reader, std::string name);


const std::string& name.

igorpeshansky · 2018-02-27T18:46:06Z

src/updater.h

 public:
  PollingMetadataUpdater(
-      MetadataAgent* store, double period_s,
+      MetadataAgent* store, MetadataReader* reader, std::string name, double period_s,


const std::string& name

bmoyles0117

Thanks for the feedback, PTAL.

bmoyles0117 · 2018-02-28T03:03:47Z

src/kubernetes.cc

-void KubernetesUpdater::start() {
-  PollingMetadataUpdater::start();
+bool KubernetesUpdater::start() {
+  if(!PollingMetadataUpdater::start()) {


bmoyles0117 · 2018-02-28T03:06:04Z

src/kubernetes.cc

+    return false;
+  }
+
  if (config().KubernetesUseWatch()) {


I think it makes sense to choose one or the other, but then I'm not sure if the contract of the KubernetesUpdater gets a little blurred, as it extends a PolliingMetadataUpdater, but only acts on that functionality conditionally. It's almost as though there should be a KubernetesPollingUpdater, and a KubernetesWatchUpdater which is controlled by configuration when the updaters are all started. WDYT?

bmoyles0117 · 2018-02-28T03:07:29Z

src/kubernetes.h

  // A Kubernetes metadata query function.
  std::vector<MetadataUpdater::ResourceMetadata> MetadataQuery() const;

+  // Validates that the reader is configured properly.


bmoyles0117 · 2018-02-28T03:07:42Z

src/updater.cc

-MetadataUpdater::MetadataUpdater(MetadataAgent* store)
-    : store_(store) {}
+MetadataUpdater::MetadataUpdater(MetadataAgent* store, MetadataReader* reader, std::string name)
+    : store_(store),


bmoyles0117 · 2018-02-28T03:08:16Z

src/updater.cc


-void PollingMetadataUpdater::start() {
+bool PollingMetadataUpdater::start() {
+  if (!reader_->ValidateConfiguration()) {


Will address as a part of the conversation for the reader above.

bmoyles0117 · 2018-02-28T03:12:28Z

src/updater.h

  };

-  MetadataUpdater(MetadataAgent* store);
+  MetadataUpdater(MetadataAgent* store, MetadataReader* reader, std::string name);


bmoyles0117 · 2018-02-28T03:12:35Z

src/updater.h

 public:
  PollingMetadataUpdater(
-      MetadataAgent* store, double period_s,
+      MetadataAgent* store, MetadataReader* reader, std::string name, double period_s,


bmoyles0117 · 2018-02-28T03:17:03Z

src/kubernetes.cc

+bool KubernetesReader::ValidateConfiguration() const {
+  try {
+    json::value raw_node = QueryMaster(
+        std::string(kKubernetesEndpointPath) + "/nodes/" + CurrentNode());


Great point, done.

bmoyles0117 · 2018-02-28T03:24:38Z

src/docker.cc

+        config_.DockerContainerFilter().empty()
+        ? "" : "&" + config_.DockerContainerFilter());
+
+    QueryDocker(std::string(kDockerEndpointPath) + "/json?all=true&limit=1" + container_filter);


Great catch, I did some local testing and it appears that the docker server discards the second value, when two limits are sent in the URL. We'll leave it as-is for now.

igorpeshansky

FWIW, I'm happy to do the proposed refactoring in a separate PR if you're willing to deal with merge conflicts.

igorpeshansky · 2018-02-28T03:04:17Z

src/reader.h

+
+class MetadataReader {
+ public:
+  virtual bool ValidateConfiguration() const;


It's ok for each updater to pass the responsibility down to individual readers — that's why I suggested having a ValidateConfiguration function in each reader that needs it. I'm simply saying that it's not necessary to have the validation in the reader, and we should abstract that away. However, the polling updater needs to perform the validation before starting the polling threads, so let's document that in a virtual function.
Happy to discuss offline.

igorpeshansky · 2018-02-28T03:15:19Z

src/updater.h


  // Starts updating.
-  virtual void start() = 0;
+  virtual bool start() = 0;


If it weren't for the desire to share a reader object, and the possible need to validate the watch and polling options together, I would have been considering splitting the watch and the poller into separate updaters.
I've already suggested using composition over inheritance here. But we could also have a StartThreads virtual function that actually starts the poller thread and is called from the (non-virtual) start after validation succeeds.
Thoughts?

igorpeshansky · 2018-02-28T03:32:09Z

src/kubernetes.cc

+bool KubernetesReader::ValidateConfiguration() const {
+  try {
+    json::value raw_node = QueryMaster(
+        std::string(kKubernetesEndpointPath) + "/nodes/" + CurrentNode());


If so, then this is redundant, since CurrentNode issues two QueryMaster calls. So just calling CurrentNode is more than enough. Or you can call QueryMaster with a filter for one (random) node...

igorpeshansky · 2018-02-28T03:36:19Z

src/kubernetes.cc

+    return false;
+  }
+
  if (config().KubernetesUseWatch()) {


I've considered it, but there's an advantage of sharing a reader.
Perhaps the right approach is to factor out a poller that takes a callback, rather than embedding the contract into the hierarchy (i.e., composition over inheritance).

igorpeshansky · 2018-02-28T03:43:25Z

src/docker.cc

+        config_.DockerContainerFilter().empty()
+        ? "" : "&" + config_.DockerContainerFilter());
+
+    QueryDocker(std::string(kDockerEndpointPath) + "/json?all=true&limit=1" + container_filter);


Let's add a comment here...

This reverts commit 89d6cf8.

AND pod connectivity for kubernetes.

igorpeshansky · 2018-02-28T17:28:43Z

src/kubernetes.cc

+bool KubernetesReader::ValidateConfiguration() const {
+  try {
+    json::value raw_node = QueryMaster(
+        std::string(kKubernetesEndpointPath) + "/nodes/" + CurrentNode());


Right, as discussed, it makes sense to issue a QueryMaster on something that doesn't depend on the CurrentNode value, and keep CurrentNode as a secondary check.

igorpeshansky · 2018-02-28T17:28:45Z

src/kubernetes.cc

+    return false;
+  }
+
  if (config().KubernetesUseWatch()) {


igorpeshansky · 2018-02-28T17:35:17Z

src/kubernetes.cc

    pod_watch_thread_ =
        std::thread(&KubernetesReader::WatchPods, &reader_, cb);
+  } else {
+    PollingMetadataUpdater::StartUpdater();


Let's add a comment that polling and watch are mutually exclusive (with watch taking precedence).

[Optional] How about putting it right above this line? Just say something like // Only try to poll if watch is disabled..

igorpeshansky · 2018-02-28T18:43:57Z

src/kubernetes.cc

+}
+
+bool KubernetesReader::ValidateConfiguration() const {
+  return ValidatePodConnectivity() && ValidateNodeConnectivity();


We might also fail if both KubernetesUseWatch and KubernetesUpdaterIntervalSeconds are enabled.

I agree and disagree with this. Being that the other polling updaters are not considered valid / invalid based on the polling interval, I think we should consolidate this to whether or not the pollers are able to communicate with endpoints, not whether or not you have a poll interval > 0.

I think this depends on whether you want to get an error message when poll interval < 0 or just silently avoid polling. Personally, I prefer the former (and reserve poll interval == 0 for "do not poll").

Perhaps we should implement ValidateConfiguration in PollingMetadataUpdater to verify the polling interval for all polling updaters, and invoke PollingMetadataUpdater::ValidateConfiguraiton here before doing updater-specific validation?..

Totally on board with this, updated.

igorpeshansky · 2018-02-28T18:45:00Z

src/kubernetes.cc


+bool KubernetesReader::ValidatePodConnectivity() const {
+  try {
+    QueryMaster(std::string(kKubernetesEndpointPath) + "/pods?limit=1");


This implicitly validates the KubernetesEndpointHost configuration option. We could also make it use KubernetesPodLabelSelector.

I'm concerned about using the pod selector, as we do this same "just check for a single thing" for nodes. I don't think this function should turn into a unit test, strictly validating that we're able to get "any" pods and nodes should suffice as a sanity check that this updater is configured properly.

I was proposing doing the same thing as for Docker, namely adding the pod selector here to have the API check the syntax, but also overriding the limit to 1. Otherwise a user misconfiguration (a bad pod selector) could result in failing queries on every poll, which is exactly what we want to avoid.

Good point, if we're doing this for docker we might as well do it here. It's not completely the same as how we actually invoke this later, as it's combined with the node name later on, but at least we can validate the label selector itself. Done.

igorpeshansky · 2018-02-28T18:48:45Z

src/instance.h

      : reader_(server->config()), PollingMetadataUpdater(
-          server, server->config().InstanceUpdaterIntervalSeconds(),
+          server, 
+          "InstanceUpdater",


[Optional] You can put this on the previous line.

igorpeshansky · 2018-02-28T18:49:41Z

src/kubernetes.h

      : reader_(server->config()), PollingMetadataUpdater(
-          server, server->config().KubernetesUpdaterIntervalSeconds(),
+          server, 
+          "KubernetesUpdater",


[Optional] You can put this on the previous line.

igorpeshansky · 2018-02-28T18:49:49Z

src/kubernetes.h

  void WatchPods(MetadataUpdater::UpdateCallback callback) const;

 private:
+  bool ValidatePodConnectivity() const;


Removed these helpers.

igorpeshansky · 2018-02-28T18:50:44Z

src/kubernetes.cc

  return FindTopLevelOwner(ns, GetOwner(ns, ref->As<json::Object>()));
 }

+bool KubernetesReader::ValidatePodConnectivity() const {


[Optional] I feel like making those into separate functions is overkill - we don't intend to reuse them. Can we not just have blocks within ValidateConfiguration that do this?

Done. I personally prefer smaller easily testable functions, but being that I'm not doing the relevant unit testing that goes with that statement, I'll streamline it.

igorpeshansky · 2018-02-28T18:52:05Z

src/updater.h

    store_->UpdateMetadata(result.resource, std::move(result.metadata));
  }
+
+  std::string name_;


Why make this protected?

Converted to private.

igorpeshansky · 2018-02-28T20:29:15Z

src/docker.cc

+    // 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);


This should be lined up with std::string (after the opening parenthesis).

igorpeshansky · 2018-02-28T20:33:31Z

src/kubernetes.cc

+bool KubernetesReader::ValidateConfiguration() const {
+  try {
+    (void) QueryMaster(
+        std::string(kKubernetesEndpointPath) + "/nodes?limit=1");


Fits on the previous line.

Column count hits 81 on previous line.

I'm confused -- I count 80:

(void) QueryMaster(std::string(kKubernetesEndpointPath) + "/nodes?limit=1");

igorpeshansky · 2018-02-28T20:34:43Z

src/kubernetes.cc

+    (void) QueryMaster(
+        std::string(kKubernetesEndpointPath) + "/nodes?limit=1");
+
+    return true;


This will cause an early return, so the rest of the function is dead code. :-)

Done. Overly ambitious copy / paste :P

igorpeshansky · 2018-02-28T20:39:16Z

src/kubernetes.cc

+  try {
+    (void) QueryMaster(std::string(kKubernetesEndpointPath) + "/pods?limit=1");
+
+    return true;


Ditto here, though this one is less of an issue. But since we return true at the end anyway, let's just get rid of this one (and the blank line) to avoid confusion.

igorpeshansky · 2018-02-28T22:48:19Z

src/kubernetes.cc

+  }
+
+  const std::string node_name = CurrentNode();
+  if (node_name.empty()) {


Can we just call CurrentNode().empty() and avoid the node_name local?

igorpeshansky · 2018-02-28T22:49:33Z

src/kubernetes.cc

+  }
+
+  try {
+    (void) QueryMaster(std::string(kKubernetesEndpointPath) + "/pods?limit=1");


We should try to use KubernetesPodLabelSelector to see if it gets rejected (bad syntax, etc).

I'm really not completely comfortable with this, as technically they could pass in garbage for a node name as well. I strongly feel the intention of this code should not be a unit test, but rather a confirmation that the endpoint is available under normal scenarios. What we have so far is better than nothing, and I would argue we're going too far down a path with too many assumptions by incorporating user-defined values as a part of this logic.

We should validate all user-supplied configuration. That said, I'm ok with this being done in a later PR.

igorpeshansky · 2018-02-28T22:51:26Z

src/kubernetes.cc

    pod_watch_thread_ =
        std::thread(&KubernetesReader::WatchPods, &reader_, cb);
+  } else {
+    PollingMetadataUpdater::StartUpdater();


[Optional] How about putting it right above this line? Just say something like // Only try to poll if watch is disabled..

igorpeshansky · 2018-02-28T23:21:14Z

src/kubernetes.cc

+  }
+
+  try {
+    (void) QueryMaster(std::string(kKubernetesEndpointPath) + "/pods?limit=1");


We should validate all user-supplied configuration. That said, I'm ok with this being done in a later PR.

igorpeshansky · 2018-02-28T23:21:22Z

src/kubernetes.cc

+bool KubernetesReader::ValidateConfiguration() const {
+  try {
+    (void) QueryMaster(
+        std::string(kKubernetesEndpointPath) + "/nodes?limit=1");


I'm confused -- I count 80:

(void) QueryMaster(std::string(kKubernetesEndpointPath) + "/nodes?limit=1");

igorpeshansky

LGTM

igorpeshansky · 2018-02-28T23:57:15Z

src/kubernetes.cc

+  }
+
+  try {
+    (void) QueryMaster(std::string(kKubernetesEndpointPath) + "/pods?limit=1");


For the future: by now this check is probably redundant, as CurrentNode() invokes this endpoint too. But we can fix that later.

On second thought, since you're waiting for a response from @supriyagarg anyway, can you just move this above the CurrentNode() check?

igorpeshansky

I'm only requesting that the CurrentNode change be done now. The other two (pod label selector and poll interval) can be done later, though if they're easy to make, I'd recommend doing them anyway while you're waiting for approval.

igorpeshansky · 2018-03-01T17:11:42Z

src/kubernetes.cc

+  }
+
+  try {
+    (void) QueryMaster(std::string(kKubernetesEndpointPath) + "/pods?limit=1");


On second thought, since you're waiting for a response from @supriyagarg anyway, can you just move this above the CurrentNode() check?

igorpeshansky · 2018-03-01T17:25:24Z

src/kubernetes.cc

+}
+
+bool KubernetesReader::ValidateConfiguration() const {
+  return ValidatePodConnectivity() && ValidateNodeConnectivity();


I think this depends on whether you want to get an error message when poll interval < 0 or just silently avoid polling. Personally, I prefer the former (and reserve poll interval == 0 for "do not poll").

Perhaps we should implement ValidateConfiguration in PollingMetadataUpdater to verify the polling interval for all polling updaters, and invoke PollingMetadataUpdater::ValidateConfiguraiton here before doing updater-specific validation?..

igorpeshansky · 2018-03-01T17:27:42Z

src/kubernetes.cc


+bool KubernetesReader::ValidatePodConnectivity() const {
+  try {
+    QueryMaster(std::string(kKubernetesEndpointPath) + "/pods?limit=1");


I was proposing doing the same thing as for Docker, namely adding the pod selector here to have the API check the syntax, but also overriding the limit to 1. Otherwise a user misconfiguration (a bad pod selector) could result in failing queries on every poll, which is exactly what we want to avoid.

supriyagarg

LGTM!

igorpeshansky

Some issues with the newly added code.

igorpeshansky · 2018-03-01T20:53:23Z

src/kubernetes.cc

+        ? "" : "&" + config_.KubernetesPodLabelSelector());
+
+    (void) QueryMaster(std::string(kKubernetesEndpointPath) + "/pods?limit=1" +
+                       pod_label_selector);


Same question as for Docker -- can we confirm that the first limit=1 will take precedence over whatever is in the selector?

pod_label_selector does not imply passing a limit, it implies passing labels, if a user is passing a limit there, that's a hack, and should be validated elsewhere.

I agree that it's a hack, but a limit=500000 in the (user-specified) selector should not cause us to suddenly grab all the pods in the cluster. Hence I was just asking whether this limit=1 will override it where it is now, or whether it should be at the end.

That said, I've just verified that the first occurrence of limit takes precedence, so we're good.

igorpeshansky · 2018-03-01T20:54:36Z

src/kubernetes.cc

-void KubernetesUpdater::start() {
-  PollingMetadataUpdater::start();
+bool KubernetesUpdater::ValidateConfiguration() const {
+  if (!PollingMetadataUpdater::ValidateConfiguration()) {


Should this not also be in DockerUpdater and InstanceUpdater?

Forgot about DockerUpdater, InstanceUpdater does not override this method.

igorpeshansky · 2018-03-01T20:57:33Z

src/updater.cc


-void PollingMetadataUpdater::start() {
+bool PollingMetadataUpdater::ValidateConfiguration() const {
+  return period_ > seconds::zero();


This should be >= -- 0 is a perfectly valid value (it means "don't poll")

I think in the future when we use composition, this statement isn't true, but for what you're saying makes sense, updated.

Depends on what you mean by "use composition". If it's in the context of "instead of inheritance", then that's orthogonal. If in the context of "composition of options", then you're probably right.
In any case, this looks good now.

igorpeshansky · 2018-03-01T20:58:10Z

src/updater.cc

  if (config().VerboseLogging()) {
    LOG(INFO) << "Timer locked";
  }
-  if (period_ > seconds::zero()) {


Still need this, because we want 0 to turn off polling without an error.

igorpeshansky

LGTM

igorpeshansky · 2018-03-01T21:47:01Z

src/kubernetes.cc

+        ? "" : "&" + config_.KubernetesPodLabelSelector());
+
+    (void) QueryMaster(std::string(kKubernetesEndpointPath) + "/pods?limit=1" +
+                       pod_label_selector);


I agree that it's a hack, but a limit=500000 in the (user-specified) selector should not cause us to suddenly grab all the pods in the cluster. Hence I was just asking whether this limit=1 will override it where it is now, or whether it should be at the end.

That said, I've just verified that the first occurrence of limit takes precedence, so we're good.

igorpeshansky · 2018-03-01T21:48:58Z

src/updater.cc


-void PollingMetadataUpdater::start() {
+bool PollingMetadataUpdater::ValidateConfiguration() const {
+  return period_ > seconds::zero();


Depends on what you mean by "use composition". If it's in the context of "instead of inheritance", then that's orthogonal. If in the context of "composition of options", then you're probably right.
In any case, this looks good now.

Add validation to only initiate pollers if configured properly.

9886f3e

bmoyles0117 requested review from igorpeshansky, qingling128 and supriyagarg February 26, 2018 22:06

supriyagarg suggested changes Feb 26, 2018

View reviewed changes

igorpeshansky suggested changes Feb 27, 2018

View reviewed changes

bmoyles0117 added 3 commits February 27, 2018 00:39

Move ValidateConfiguration to readers and updaters.

ca3b09c

Updater::start returns a bool so we can signal an upstream failure.

Add name property to updater.

a445d3f

Make the reader an updater property to validate configuration.

89d6cf8

igorpeshansky suggested changes Feb 27, 2018

View reviewed changes

igorpeshansky force-pushed the master branch from 70449da to 5aa2078 Compare February 28, 2018 00:00

bmoyles0117 commented Feb 28, 2018

View reviewed changes

Address feedback.

5bd4add

igorpeshansky suggested changes Feb 28, 2018

View reviewed changes

bmoyles0117 added 3 commits February 28, 2018 11:34

Revert "Make the reader an updater property to validate configuration."

2b99257

This reverts commit 89d6cf8.

Revert Reader base class and add virtual [Start|Stop]Updater methods.

05fbbfb

Limit Docker container request to one container, and validate node

a333634

AND pod connectivity for kubernetes.

bmoyles0117 changed the title ~~Add validation to only initiate pollers if configured properly.~~ Add validation to only initiate updaters if they're configured properly. Feb 28, 2018

igorpeshansky suggested changes Feb 28, 2018

View reviewed changes

bmoyles0117 added 2 commits February 28, 2018 14:24

Address feedback.

3e56c5c

Address feedback.

475c035

bmoyles0117 changed the title ~~Add validation to only initiate updaters if they're configured properly.~~ Add validation to only start updaters if they're configured properly. Feb 28, 2018

igorpeshansky suggested changes Feb 28, 2018

View reviewed changes

Address feedback.

812559c

igorpeshansky suggested changes Feb 28, 2018

View reviewed changes

Address feedback.

9b37540

igorpeshansky approved these changes Feb 28, 2018

View reviewed changes

igorpeshansky suggested changes Mar 1, 2018

View reviewed changes

Address feedback.

c5fbc29

supriyagarg approved these changes Mar 1, 2018

View reviewed changes

igorpeshansky suggested changes Mar 1, 2018

View reviewed changes

Address feedback.

275bfbb

igorpeshansky approved these changes Mar 1, 2018

View reviewed changes

bmoyles0117 merged commit 6d7718c into master Mar 1, 2018

igorpeshansky deleted the bmoyles0117-pollers-inspect-env branch March 10, 2018 19:49

Add validation to only start updaters if they're configured properly. #63

Add validation to only start updaters if they're configured properly. #63

Uh oh!

Conversation

bmoyles0117 commented Feb 26, 2018

Uh oh!

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

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!

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!

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!