From 9886f3e996e3f131b06d65be4e71702b85a96695 Mon Sep 17 00:00:00 2001 From: Bryan Moyles Date: Mon, 26 Feb 2018 17:05:57 -0500 Subject: [PATCH 01/14] Add validation to only initiate pollers if configured properly. --- src/docker.cc | 24 ++++++++++++++++++++++++ src/docker.h | 6 ++++++ src/kubernetes.cc | 21 +++++++++++++++++++++ src/kubernetes.h | 4 ++++ 4 files changed, 55 insertions(+) diff --git a/src/docker.cc b/src/docker.cc index f9b8b7a4..fcb50409 100644 --- a/src/docker.cc +++ b/src/docker.cc @@ -46,6 +46,17 @@ constexpr const char kDockerContainerResourcePrefix[] = "container"; DockerReader::DockerReader(const MetadataAgentConfiguration& config) : config_(config), environment_(config) {} +const bool DockerReader::IsConfigured() const { + try { + QueryDocker(std::string(kDockerEndpointPath) + "/json?all=true"); + + return true; + } catch (const QueryException& e) { + // Already logged. + return false; + } +} + MetadataUpdater::ResourceMetadata DockerReader::GetContainerMetadata( const json::Object* container, Timestamp collected_at) const throw(json::Exception) { @@ -187,4 +198,17 @@ json::value DockerReader::QueryDocker(const std::string& path) const } } +void DockerUpdater::start() { + if (!reader_.IsConfigured()) { + LOG(ERROR) << "Docker reader is not configured properly."; + return; + } + + if (config_.VerboseLogging()) { + LOG(INFO) << "Docker reader is configured properly."; + } + + PollingMetadataUpdater::start(); +} + } diff --git a/src/docker.h b/src/docker.h index da54446b..6339be25 100644 --- a/src/docker.h +++ b/src/docker.h @@ -32,6 +32,10 @@ class DockerReader { // A Docker metadata query function. std::vector MetadataQuery() const; + // Validates that the reader is configured properly. + // Returns a bool that represents if it's configured properly. + const bool IsConfigured() const; + private: // A representation of all query-related errors. class QueryException { @@ -62,6 +66,8 @@ class DockerUpdater : public PollingMetadataUpdater { : reader_(server->config()), PollingMetadataUpdater( server, server->config().DockerUpdaterIntervalSeconds(), std::bind(&google::DockerReader::MetadataQuery, &reader_)) { } + + void start(); private: DockerReader reader_; }; diff --git a/src/kubernetes.cc b/src/kubernetes.cc index 6205d8bc..b621ebf3 100644 --- a/src/kubernetes.cc +++ b/src/kubernetes.cc @@ -961,6 +961,18 @@ json::value KubernetesReader::FindTopLevelOwner( return FindTopLevelOwner(ns, GetOwner(ns, ref->As())); } +const bool KubernetesReader::IsConfigured() const { + try { + json::value raw_node = QueryMaster( + std::string(kKubernetesEndpointPath) + "/nodes"); + + return true; + } catch (const QueryException& e) { + // Already logged. + return false; + } +} + void KubernetesReader::PodCallback( MetadataUpdater::UpdateCallback callback, const json::Object* pod, Timestamp collected_at, bool is_deleted) const @@ -1035,6 +1047,15 @@ void KubernetesReader::WatchNode(MetadataUpdater::UpdateCallback callback) } void KubernetesUpdater::start() { + if (!reader_.IsConfigured()) { + LOG(ERROR) << "Kubernetes reader is not configured properly."; + return; + } + + if (config_.VerboseLogging()) { + LOG(INFO) << "Kubernetes reader is configured properly."; + } + PollingMetadataUpdater::start(); if (config().KubernetesUseWatch()) { // Wrap the bind expression into a function to use as a bind argument. diff --git a/src/kubernetes.h b/src/kubernetes.h index 3ed88cdc..671ec27d 100644 --- a/src/kubernetes.h +++ b/src/kubernetes.h @@ -38,6 +38,10 @@ class KubernetesReader { // A Kubernetes metadata query function. std::vector MetadataQuery() const; + // Validates that the reader is configured properly. + // Returns a bool that represents if it's configured properly. + const bool IsConfigured() const; + // Node watcher. void WatchNode(MetadataUpdater::UpdateCallback callback) const; From ca3b09c834812c2bad4bf91ddd2d828334d7bb49 Mon Sep 17 00:00:00 2001 From: Bryan Moyles Date: Tue, 27 Feb 2018 00:39:11 -0500 Subject: [PATCH 02/14] Move ValidateConfiguration to readers and updaters. Updater::start returns a bool so we can signal an upstream failure. --- src/Makefile | 1 + src/docker.cc | 21 ++++++++------------- src/docker.h | 9 ++++++--- src/instance.h | 3 ++- src/kubernetes.cc | 20 ++++++++++---------- src/kubernetes.h | 11 ++++++++--- src/reader.cc | 25 +++++++++++++++++++++++++ src/reader.h | 29 +++++++++++++++++++++++++++++ src/updater.cc | 12 +++++++++++- src/updater.h | 6 ++++-- 10 files changed, 104 insertions(+), 33 deletions(-) create mode 100644 src/reader.cc create mode 100644 src/reader.h diff --git a/src/Makefile b/src/Makefile index cd510478..8b193f74 100644 --- a/src/Makefile +++ b/src/Makefile @@ -26,6 +26,7 @@ SRCS=\ metadatad.cc \ api_server.cc \ configuration.cc \ + reader.cc \ updater.cc \ instance.cc \ docker.cc \ diff --git a/src/docker.cc b/src/docker.cc index fcb50409..43736af4 100644 --- a/src/docker.cc +++ b/src/docker.cc @@ -46,9 +46,13 @@ constexpr const char kDockerContainerResourcePrefix[] = "container"; DockerReader::DockerReader(const MetadataAgentConfiguration& config) : config_(config), environment_(config) {} -const bool DockerReader::IsConfigured() const { +bool DockerReader::ValidateConfiguration() const { try { - QueryDocker(std::string(kDockerEndpointPath) + "/json?all=true"); + const std::string container_filter( + config_.DockerContainerFilter().empty() + ? "" : "&" + config_.DockerContainerFilter()); + + QueryDocker(std::string(kDockerEndpointPath) + "/json?all=true&limit=1" + container_filter); return true; } catch (const QueryException& e) { @@ -198,17 +202,8 @@ json::value DockerReader::QueryDocker(const std::string& path) const } } -void DockerUpdater::start() { - if (!reader_.IsConfigured()) { - LOG(ERROR) << "Docker reader is not configured properly."; - return; - } - - if (config_.VerboseLogging()) { - LOG(INFO) << "Docker reader is configured properly."; - } - - PollingMetadataUpdater::start(); +bool DockerUpdater::ValidateConfiguration() const { + return reader_.ValidateConfiguration(); } } diff --git a/src/docker.h b/src/docker.h index 6339be25..ae5b495c 100644 --- a/src/docker.h +++ b/src/docker.h @@ -22,11 +22,12 @@ #include "configuration.h" #include "environment.h" +#include "reader.h" #include "updater.h" namespace google { -class DockerReader { +class DockerReader : public MetadataReader { public: DockerReader(const MetadataAgentConfiguration& config); // A Docker metadata query function. @@ -34,7 +35,7 @@ class DockerReader { // Validates that the reader is configured properly. // Returns a bool that represents if it's configured properly. - const bool IsConfigured() const; + bool ValidateConfiguration() const; private: // A representation of all query-related errors. @@ -67,7 +68,9 @@ class DockerUpdater : public PollingMetadataUpdater { server, server->config().DockerUpdaterIntervalSeconds(), std::bind(&google::DockerReader::MetadataQuery, &reader_)) { } - void start(); + // Validates that the reader is configured properly. + // Returns a bool that represents if it's configured properly. + bool ValidateConfiguration() const; private: DockerReader reader_; }; diff --git a/src/instance.h b/src/instance.h index 6fa612f7..2d446521 100644 --- a/src/instance.h +++ b/src/instance.h @@ -22,12 +22,13 @@ #include "configuration.h" #include "environment.h" +#include "reader.h" #include "resource.h" #include "updater.h" namespace google { -class InstanceReader { +class InstanceReader : public MetadataReader { public: InstanceReader(const MetadataAgentConfiguration& config); // A Instance metadata query function. diff --git a/src/kubernetes.cc b/src/kubernetes.cc index b621ebf3..b7ab8b4d 100644 --- a/src/kubernetes.cc +++ b/src/kubernetes.cc @@ -961,10 +961,10 @@ json::value KubernetesReader::FindTopLevelOwner( return FindTopLevelOwner(ns, GetOwner(ns, ref->As())); } -const bool KubernetesReader::IsConfigured() const { +bool KubernetesReader::ValidateConfiguration() const { try { json::value raw_node = QueryMaster( - std::string(kKubernetesEndpointPath) + "/nodes"); + std::string(kKubernetesEndpointPath) + "/nodes/" + CurrentNode()); return true; } catch (const QueryException& e) { @@ -1046,17 +1046,15 @@ void KubernetesReader::WatchNode(MetadataUpdater::UpdateCallback callback) LOG(INFO) << "Watch thread (node) exiting"; } -void KubernetesUpdater::start() { - if (!reader_.IsConfigured()) { - LOG(ERROR) << "Kubernetes reader is not configured properly."; - return; - } +bool KubernetesUpdater::ValidateConfiguration() const { + return reader_.ValidateConfiguration(); +} - if (config_.VerboseLogging()) { - LOG(INFO) << "Kubernetes reader is configured properly."; +bool KubernetesUpdater::start() { + if(!PollingMetadataUpdater::start()) { + return false; } - PollingMetadataUpdater::start(); if (config().KubernetesUseWatch()) { // Wrap the bind expression into a function to use as a bind argument. UpdateCallback cb = std::bind(&KubernetesUpdater::MetadataCallback, this, @@ -1066,6 +1064,8 @@ void KubernetesUpdater::start() { pod_watch_thread_ = std::thread(&KubernetesReader::WatchPods, &reader_, cb); } + + return true; } void KubernetesUpdater::MetadataCallback( diff --git a/src/kubernetes.h b/src/kubernetes.h index 671ec27d..d5a404ad 100644 --- a/src/kubernetes.h +++ b/src/kubernetes.h @@ -28,11 +28,12 @@ #include "configuration.h" #include "environment.h" #include "json.h" +#include "reader.h" #include "updater.h" namespace google { -class KubernetesReader { +class KubernetesReader : public MetadataReader { public: KubernetesReader(const MetadataAgentConfiguration& config); // A Kubernetes metadata query function. @@ -40,7 +41,7 @@ class KubernetesReader { // Validates that the reader is configured properly. // Returns a bool that represents if it's configured properly. - const bool IsConfigured() const; + bool ValidateConfiguration() const; // Node watcher. void WatchNode(MetadataUpdater::UpdateCallback callback) const; @@ -162,7 +163,11 @@ class KubernetesUpdater : public PollingMetadataUpdater { } } - void start(); + // Validates that the reader is configured properly. + // Returns a bool that represents if it's configured properly. + bool ValidateConfiguration() const; + + bool start(); private: // Metadata watcher callback. diff --git a/src/reader.cc b/src/reader.cc new file mode 100644 index 00000000..8c2e814c --- /dev/null +++ b/src/reader.cc @@ -0,0 +1,25 @@ +/* + * 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. + **/ + +#include "reader.h" + +namespace google { + +bool MetadataReader::ValidateConfiguration() const { + return true; +} + +} diff --git a/src/reader.h b/src/reader.h new file mode 100644 index 00000000..760342fb --- /dev/null +++ b/src/reader.h @@ -0,0 +1,29 @@ +/* + * Copyright 2017 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 READER_H_ +#define READER_H_ + +namespace google { + +class MetadataReader { + public: + virtual bool ValidateConfiguration() const; + +}; + +} + +#endif // READER_H_ diff --git a/src/updater.cc b/src/updater.cc index 4e3b7df4..c1eacdb3 100644 --- a/src/updater.cc +++ b/src/updater.cc @@ -27,6 +27,10 @@ MetadataUpdater::MetadataUpdater(MetadataAgent* store) MetadataUpdater::~MetadataUpdater() {} +bool MetadataUpdater::ValidateConfiguration() const { + return true; +} + PollingMetadataUpdater::PollingMetadataUpdater( MetadataAgent* store, double period_s, std::function()> query_metadata) @@ -42,7 +46,11 @@ PollingMetadataUpdater::~PollingMetadataUpdater() { } } -void PollingMetadataUpdater::start() { +bool PollingMetadataUpdater::start() { + if (!ValidateConfiguration()) { + return false; + } + timer_.lock(); if (config().VerboseLogging()) { LOG(INFO) << "Timer locked"; @@ -51,6 +59,8 @@ void PollingMetadataUpdater::start() { reporter_thread_ = std::thread(&PollingMetadataUpdater::PollForMetadata, this); } + + return true; } void PollingMetadataUpdater::stop() { diff --git a/src/updater.h b/src/updater.h index d4878962..6371acc1 100644 --- a/src/updater.h +++ b/src/updater.h @@ -53,8 +53,10 @@ class MetadataUpdater { return store_->config(); } + virtual bool ValidateConfiguration() const; + // Starts updating. - virtual void start() = 0; + virtual bool start() = 0; // Stops updating. virtual void stop() = 0; @@ -86,7 +88,7 @@ class PollingMetadataUpdater : public MetadataUpdater { std::function()> query_metadata); ~PollingMetadataUpdater(); - void start(); + bool start(); void stop(); private: From a445d3fc8e9411a7eb1b5e25b56abd0e92393453 Mon Sep 17 00:00:00 2001 From: Bryan Moyles Date: Tue, 27 Feb 2018 01:18:57 -0500 Subject: [PATCH 03/14] Add name property to updater. --- src/docker.h | 4 +++- src/instance.h | 4 +++- src/kubernetes.h | 4 +++- src/updater.cc | 10 ++++++---- src/updater.h | 6 ++++-- 5 files changed, 19 insertions(+), 9 deletions(-) diff --git a/src/docker.h b/src/docker.h index ae5b495c..2162ea87 100644 --- a/src/docker.h +++ b/src/docker.h @@ -65,7 +65,9 @@ class DockerUpdater : public PollingMetadataUpdater { public: DockerUpdater(MetadataAgent* server) : reader_(server->config()), PollingMetadataUpdater( - server, server->config().DockerUpdaterIntervalSeconds(), + server, + "DockerUpdater", + server->config().DockerUpdaterIntervalSeconds(), std::bind(&google::DockerReader::MetadataQuery, &reader_)) { } // Validates that the reader is configured properly. diff --git a/src/instance.h b/src/instance.h index 2d446521..aa4794d8 100644 --- a/src/instance.h +++ b/src/instance.h @@ -46,7 +46,9 @@ class InstanceUpdater : public PollingMetadataUpdater { public: InstanceUpdater(MetadataAgent* server) : reader_(server->config()), PollingMetadataUpdater( - server, server->config().InstanceUpdaterIntervalSeconds(), + server, + "InstanceUpdater", + server->config().InstanceUpdaterIntervalSeconds(), std::bind(&google::InstanceReader::MetadataQuery, &reader_)) { } private: InstanceReader reader_; diff --git a/src/kubernetes.h b/src/kubernetes.h index d5a404ad..cec790aa 100644 --- a/src/kubernetes.h +++ b/src/kubernetes.h @@ -152,7 +152,9 @@ class KubernetesUpdater : public PollingMetadataUpdater { public: KubernetesUpdater(MetadataAgent* server) : reader_(server->config()), PollingMetadataUpdater( - server, server->config().KubernetesUpdaterIntervalSeconds(), + server, + "KubernetesUpdater", + server->config().KubernetesUpdaterIntervalSeconds(), std::bind(&google::KubernetesReader::MetadataQuery, &reader_)) { } ~KubernetesUpdater() { if (node_watch_thread_.joinable()) { diff --git a/src/updater.cc b/src/updater.cc index c1eacdb3..86bfa155 100644 --- a/src/updater.cc +++ b/src/updater.cc @@ -22,8 +22,9 @@ namespace google { -MetadataUpdater::MetadataUpdater(MetadataAgent* store) - : store_(store) {} +MetadataUpdater::MetadataUpdater(MetadataAgent* store, std::string name) + : store_(store), + name_(name) {} MetadataUpdater::~MetadataUpdater() {} @@ -32,9 +33,9 @@ bool MetadataUpdater::ValidateConfiguration() const { } PollingMetadataUpdater::PollingMetadataUpdater( - MetadataAgent* store, double period_s, + MetadataAgent* store, std::string name, double period_s, std::function()> query_metadata) - : MetadataUpdater(store), + : MetadataUpdater(store, name), period_(period_s), query_metadata_(query_metadata), timer_(), @@ -48,6 +49,7 @@ PollingMetadataUpdater::~PollingMetadataUpdater() { bool PollingMetadataUpdater::start() { if (!ValidateConfiguration()) { + LOG(ERROR) << "Failed to validate configuration for " << name_; return false; } diff --git a/src/updater.h b/src/updater.h index 6371acc1..225217cc 100644 --- a/src/updater.h +++ b/src/updater.h @@ -46,7 +46,7 @@ class MetadataUpdater { MetadataAgent::Metadata metadata; }; - MetadataUpdater(MetadataAgent* store); + MetadataUpdater(MetadataAgent* store, std::string name); virtual ~MetadataUpdater(); const MetadataAgentConfiguration& config() { @@ -74,6 +74,8 @@ class MetadataUpdater { void UpdateMetadataCallback(ResourceMetadata&& result) { store_->UpdateMetadata(result.resource, std::move(result.metadata)); } + + std::string name_; private: // The store for the metadata. @@ -84,7 +86,7 @@ class MetadataUpdater { class PollingMetadataUpdater : public MetadataUpdater { public: PollingMetadataUpdater( - MetadataAgent* store, double period_s, + MetadataAgent* store, std::string name, double period_s, std::function()> query_metadata); ~PollingMetadataUpdater(); From 89d6cf8cefd5e7095d8f64fd2512945585aa432e Mon Sep 17 00:00:00 2001 From: Bryan Moyles Date: Tue, 27 Feb 2018 02:13:47 -0500 Subject: [PATCH 04/14] Make the reader an updater property to validate configuration. --- src/docker.cc | 4 ---- src/docker.h | 4 +--- src/instance.h | 1 + src/kubernetes.cc | 4 ---- src/kubernetes.h | 5 +---- src/updater.cc | 13 +++++-------- src/updater.h | 10 +++++----- 7 files changed, 13 insertions(+), 28 deletions(-) diff --git a/src/docker.cc b/src/docker.cc index 43736af4..4f0497bd 100644 --- a/src/docker.cc +++ b/src/docker.cc @@ -202,8 +202,4 @@ json::value DockerReader::QueryDocker(const std::string& path) const } } -bool DockerUpdater::ValidateConfiguration() const { - return reader_.ValidateConfiguration(); -} - } diff --git a/src/docker.h b/src/docker.h index 2162ea87..91163e51 100644 --- a/src/docker.h +++ b/src/docker.h @@ -66,13 +66,11 @@ class DockerUpdater : public PollingMetadataUpdater { DockerUpdater(MetadataAgent* server) : reader_(server->config()), PollingMetadataUpdater( server, + &reader_, "DockerUpdater", server->config().DockerUpdaterIntervalSeconds(), std::bind(&google::DockerReader::MetadataQuery, &reader_)) { } - // Validates that the reader is configured properly. - // Returns a bool that represents if it's configured properly. - bool ValidateConfiguration() const; private: DockerReader reader_; }; diff --git a/src/instance.h b/src/instance.h index aa4794d8..9c12e57c 100644 --- a/src/instance.h +++ b/src/instance.h @@ -47,6 +47,7 @@ class InstanceUpdater : public PollingMetadataUpdater { InstanceUpdater(MetadataAgent* server) : reader_(server->config()), PollingMetadataUpdater( server, + &reader_, "InstanceUpdater", server->config().InstanceUpdaterIntervalSeconds(), std::bind(&google::InstanceReader::MetadataQuery, &reader_)) { } diff --git a/src/kubernetes.cc b/src/kubernetes.cc index b7ab8b4d..5ef02f14 100644 --- a/src/kubernetes.cc +++ b/src/kubernetes.cc @@ -1046,10 +1046,6 @@ void KubernetesReader::WatchNode(MetadataUpdater::UpdateCallback callback) LOG(INFO) << "Watch thread (node) exiting"; } -bool KubernetesUpdater::ValidateConfiguration() const { - return reader_.ValidateConfiguration(); -} - bool KubernetesUpdater::start() { if(!PollingMetadataUpdater::start()) { return false; diff --git a/src/kubernetes.h b/src/kubernetes.h index cec790aa..45adf9af 100644 --- a/src/kubernetes.h +++ b/src/kubernetes.h @@ -153,6 +153,7 @@ class KubernetesUpdater : public PollingMetadataUpdater { KubernetesUpdater(MetadataAgent* server) : reader_(server->config()), PollingMetadataUpdater( server, + &reader_, "KubernetesUpdater", server->config().KubernetesUpdaterIntervalSeconds(), std::bind(&google::KubernetesReader::MetadataQuery, &reader_)) { } @@ -165,10 +166,6 @@ class KubernetesUpdater : public PollingMetadataUpdater { } } - // Validates that the reader is configured properly. - // Returns a bool that represents if it's configured properly. - bool ValidateConfiguration() const; - bool start(); private: diff --git a/src/updater.cc b/src/updater.cc index 86bfa155..6bb79f32 100644 --- a/src/updater.cc +++ b/src/updater.cc @@ -22,20 +22,17 @@ namespace google { -MetadataUpdater::MetadataUpdater(MetadataAgent* store, std::string name) +MetadataUpdater::MetadataUpdater(MetadataAgent* store, MetadataReader* reader, std::string name) : store_(store), + reader_(reader), name_(name) {} MetadataUpdater::~MetadataUpdater() {} -bool MetadataUpdater::ValidateConfiguration() const { - return true; -} - PollingMetadataUpdater::PollingMetadataUpdater( - MetadataAgent* store, std::string name, double period_s, + MetadataAgent* store, MetadataReader* reader, std::string name, double period_s, std::function()> query_metadata) - : MetadataUpdater(store, name), + : MetadataUpdater(store, reader, name), period_(period_s), query_metadata_(query_metadata), timer_(), @@ -48,7 +45,7 @@ PollingMetadataUpdater::~PollingMetadataUpdater() { } bool PollingMetadataUpdater::start() { - if (!ValidateConfiguration()) { + if (!reader_->ValidateConfiguration()) { LOG(ERROR) << "Failed to validate configuration for " << name_; return false; } diff --git a/src/updater.h b/src/updater.h index 225217cc..16a8b9a2 100644 --- a/src/updater.h +++ b/src/updater.h @@ -26,6 +26,7 @@ #include #include "api_server.h" +#include "reader.h" #include "resource.h" namespace google { @@ -46,15 +47,13 @@ class MetadataUpdater { MetadataAgent::Metadata metadata; }; - MetadataUpdater(MetadataAgent* store, std::string name); + MetadataUpdater(MetadataAgent* store, MetadataReader* reader, std::string name); virtual ~MetadataUpdater(); const MetadataAgentConfiguration& config() { return store_->config(); } - virtual bool ValidateConfiguration() const; - // Starts updating. virtual bool start() = 0; @@ -74,8 +73,9 @@ class MetadataUpdater { void UpdateMetadataCallback(ResourceMetadata&& result) { store_->UpdateMetadata(result.resource, std::move(result.metadata)); } - + std::string name_; + MetadataReader* reader_; private: // The store for the metadata. @@ -86,7 +86,7 @@ class MetadataUpdater { class PollingMetadataUpdater : public MetadataUpdater { public: PollingMetadataUpdater( - MetadataAgent* store, std::string name, double period_s, + MetadataAgent* store, MetadataReader* reader, std::string name, double period_s, std::function()> query_metadata); ~PollingMetadataUpdater(); From 5bd4addd08706f1f376bc140a7fb54222f5581e9 Mon Sep 17 00:00:00 2001 From: Bryan Moyles Date: Tue, 27 Feb 2018 22:25:28 -0500 Subject: [PATCH 05/14] Address feedback. --- src/docker.h | 2 +- src/kubernetes.cc | 9 +++++++-- src/kubernetes.h | 2 +- src/updater.cc | 8 +++----- src/updater.h | 4 ++-- 5 files changed, 14 insertions(+), 11 deletions(-) diff --git a/src/docker.h b/src/docker.h index 91163e51..e3dbf452 100644 --- a/src/docker.h +++ b/src/docker.h @@ -33,7 +33,7 @@ class DockerReader : public MetadataReader { // A Docker metadata query function. std::vector MetadataQuery() const; - // Validates that the reader is configured properly. + // Validates the relevant configuration and returns true if it's correct. // Returns a bool that represents if it's configured properly. bool ValidateConfiguration() const; diff --git a/src/kubernetes.cc b/src/kubernetes.cc index 5ef02f14..8f0a68f2 100644 --- a/src/kubernetes.cc +++ b/src/kubernetes.cc @@ -962,9 +962,14 @@ json::value KubernetesReader::FindTopLevelOwner( } bool KubernetesReader::ValidateConfiguration() const { + const std::string node_name = CurrentNode(); + if (node_name.empty()) { + return false; + } + try { json::value raw_node = QueryMaster( - std::string(kKubernetesEndpointPath) + "/nodes/" + CurrentNode()); + std::string(kKubernetesEndpointPath) + "/nodes/" + node_name); return true; } catch (const QueryException& e) { @@ -1047,7 +1052,7 @@ void KubernetesReader::WatchNode(MetadataUpdater::UpdateCallback callback) } bool KubernetesUpdater::start() { - if(!PollingMetadataUpdater::start()) { + if (!PollingMetadataUpdater::start()) { return false; } diff --git a/src/kubernetes.h b/src/kubernetes.h index 45adf9af..79b93a91 100644 --- a/src/kubernetes.h +++ b/src/kubernetes.h @@ -39,7 +39,7 @@ class KubernetesReader : public MetadataReader { // A Kubernetes metadata query function. std::vector MetadataQuery() const; - // Validates that the reader is configured properly. + // Validates the relevant configuration and returns true if it's correct. // Returns a bool that represents if it's configured properly. bool ValidateConfiguration() const; diff --git a/src/updater.cc b/src/updater.cc index 6bb79f32..e9ec94ae 100644 --- a/src/updater.cc +++ b/src/updater.cc @@ -22,15 +22,13 @@ namespace google { -MetadataUpdater::MetadataUpdater(MetadataAgent* store, MetadataReader* reader, std::string name) - : store_(store), - reader_(reader), - name_(name) {} +MetadataUpdater::MetadataUpdater(MetadataAgent* store, MetadataReader* reader, const std::string& name) + : store_(store), reader_(reader), name_(name) {} MetadataUpdater::~MetadataUpdater() {} PollingMetadataUpdater::PollingMetadataUpdater( - MetadataAgent* store, MetadataReader* reader, std::string name, double period_s, + MetadataAgent* store, MetadataReader* reader, const std::string& name, double period_s, std::function()> query_metadata) : MetadataUpdater(store, reader, name), period_(period_s), diff --git a/src/updater.h b/src/updater.h index 16a8b9a2..82a60e66 100644 --- a/src/updater.h +++ b/src/updater.h @@ -47,7 +47,7 @@ class MetadataUpdater { MetadataAgent::Metadata metadata; }; - MetadataUpdater(MetadataAgent* store, MetadataReader* reader, std::string name); + MetadataUpdater(MetadataAgent* store, MetadataReader* reader, const std::string& name); virtual ~MetadataUpdater(); const MetadataAgentConfiguration& config() { @@ -86,7 +86,7 @@ class MetadataUpdater { class PollingMetadataUpdater : public MetadataUpdater { public: PollingMetadataUpdater( - MetadataAgent* store, MetadataReader* reader, std::string name, double period_s, + MetadataAgent* store, MetadataReader* reader, const std::string& name, double period_s, std::function()> query_metadata); ~PollingMetadataUpdater(); From 2b99257bd04ebf64b6e2db3f66d184bae94cb4f1 Mon Sep 17 00:00:00 2001 From: Bryan Moyles Date: Wed, 28 Feb 2018 11:34:42 -0500 Subject: [PATCH 06/14] Revert "Make the reader an updater property to validate configuration." This reverts commit 89d6cf8cefd5e7095d8f64fd2512945585aa432e. --- src/docker.cc | 4 ++++ src/docker.h | 4 +++- src/instance.h | 1 - src/kubernetes.cc | 4 ++++ src/kubernetes.h | 5 ++++- src/updater.cc | 14 +++++++++----- src/updater.h | 10 +++++----- 7 files changed, 29 insertions(+), 13 deletions(-) diff --git a/src/docker.cc b/src/docker.cc index 4f0497bd..43736af4 100644 --- a/src/docker.cc +++ b/src/docker.cc @@ -202,4 +202,8 @@ json::value DockerReader::QueryDocker(const std::string& path) const } } +bool DockerUpdater::ValidateConfiguration() const { + return reader_.ValidateConfiguration(); +} + } diff --git a/src/docker.h b/src/docker.h index e3dbf452..e5758418 100644 --- a/src/docker.h +++ b/src/docker.h @@ -66,11 +66,13 @@ class DockerUpdater : public PollingMetadataUpdater { DockerUpdater(MetadataAgent* server) : reader_(server->config()), PollingMetadataUpdater( server, - &reader_, "DockerUpdater", server->config().DockerUpdaterIntervalSeconds(), std::bind(&google::DockerReader::MetadataQuery, &reader_)) { } + // Validates that the reader is configured properly. + // Returns a bool that represents if it's configured properly. + bool ValidateConfiguration() const; private: DockerReader reader_; }; diff --git a/src/instance.h b/src/instance.h index 9c12e57c..aa4794d8 100644 --- a/src/instance.h +++ b/src/instance.h @@ -47,7 +47,6 @@ class InstanceUpdater : public PollingMetadataUpdater { InstanceUpdater(MetadataAgent* server) : reader_(server->config()), PollingMetadataUpdater( server, - &reader_, "InstanceUpdater", server->config().InstanceUpdaterIntervalSeconds(), std::bind(&google::InstanceReader::MetadataQuery, &reader_)) { } diff --git a/src/kubernetes.cc b/src/kubernetes.cc index 8f0a68f2..b4e0ff97 100644 --- a/src/kubernetes.cc +++ b/src/kubernetes.cc @@ -1051,6 +1051,10 @@ void KubernetesReader::WatchNode(MetadataUpdater::UpdateCallback callback) LOG(INFO) << "Watch thread (node) exiting"; } +bool KubernetesUpdater::ValidateConfiguration() const { + return reader_.ValidateConfiguration(); +} + bool KubernetesUpdater::start() { if (!PollingMetadataUpdater::start()) { return false; diff --git a/src/kubernetes.h b/src/kubernetes.h index 79b93a91..2012b226 100644 --- a/src/kubernetes.h +++ b/src/kubernetes.h @@ -153,7 +153,6 @@ class KubernetesUpdater : public PollingMetadataUpdater { KubernetesUpdater(MetadataAgent* server) : reader_(server->config()), PollingMetadataUpdater( server, - &reader_, "KubernetesUpdater", server->config().KubernetesUpdaterIntervalSeconds(), std::bind(&google::KubernetesReader::MetadataQuery, &reader_)) { } @@ -166,6 +165,10 @@ class KubernetesUpdater : public PollingMetadataUpdater { } } + // Validates that the reader is configured properly. + // Returns a bool that represents if it's configured properly. + bool ValidateConfiguration() const; + bool start(); private: diff --git a/src/updater.cc b/src/updater.cc index e9ec94ae..24a70cb0 100644 --- a/src/updater.cc +++ b/src/updater.cc @@ -22,15 +22,19 @@ namespace google { -MetadataUpdater::MetadataUpdater(MetadataAgent* store, MetadataReader* reader, const std::string& name) - : store_(store), reader_(reader), name_(name) {} +MetadataUpdater::MetadataUpdater(MetadataAgent* store, std::string name) + : store_(store), name_(name) {} MetadataUpdater::~MetadataUpdater() {} +bool MetadataUpdater::ValidateConfiguration() const { + return true; +} + PollingMetadataUpdater::PollingMetadataUpdater( - MetadataAgent* store, MetadataReader* reader, const std::string& name, double period_s, + MetadataAgent* store, const std::string& name, double period_s, std::function()> query_metadata) - : MetadataUpdater(store, reader, name), + : MetadataUpdater(store, name), period_(period_s), query_metadata_(query_metadata), timer_(), @@ -43,7 +47,7 @@ PollingMetadataUpdater::~PollingMetadataUpdater() { } bool PollingMetadataUpdater::start() { - if (!reader_->ValidateConfiguration()) { + if (!ValidateConfiguration()) { LOG(ERROR) << "Failed to validate configuration for " << name_; return false; } diff --git a/src/updater.h b/src/updater.h index 82a60e66..264ed243 100644 --- a/src/updater.h +++ b/src/updater.h @@ -26,7 +26,6 @@ #include #include "api_server.h" -#include "reader.h" #include "resource.h" namespace google { @@ -47,13 +46,15 @@ class MetadataUpdater { MetadataAgent::Metadata metadata; }; - MetadataUpdater(MetadataAgent* store, MetadataReader* reader, const std::string& name); + MetadataUpdater(MetadataAgent* store, const std::string& name); virtual ~MetadataUpdater(); const MetadataAgentConfiguration& config() { return store_->config(); } + virtual bool ValidateConfiguration() const; + // Starts updating. virtual bool start() = 0; @@ -73,9 +74,8 @@ class MetadataUpdater { void UpdateMetadataCallback(ResourceMetadata&& result) { store_->UpdateMetadata(result.resource, std::move(result.metadata)); } - + std::string name_; - MetadataReader* reader_; private: // The store for the metadata. @@ -86,7 +86,7 @@ class MetadataUpdater { class PollingMetadataUpdater : public MetadataUpdater { public: PollingMetadataUpdater( - MetadataAgent* store, MetadataReader* reader, const std::string& name, double period_s, + MetadataAgent* store, const std::string& name, double period_s, std::function()> query_metadata); ~PollingMetadataUpdater(); From 05fbbfb16adce46bbe25410da9998d1f7b7abc29 Mon Sep 17 00:00:00 2001 From: Bryan Moyles Date: Wed, 28 Feb 2018 12:19:07 -0500 Subject: [PATCH 07/14] Revert Reader base class and add virtual [Start|Stop]Updater methods. --- src/Makefile | 1 - src/docker.cc | 6 +----- src/docker.h | 7 +++---- src/instance.h | 3 +-- src/kubernetes.cc | 10 +++------- src/kubernetes.h | 9 +++------ src/reader.cc | 25 ------------------------- src/reader.h | 29 ----------------------------- src/updater.cc | 27 +++++++++++++++------------ src/updater.h | 23 +++++++++++++++++------ 10 files changed, 43 insertions(+), 97 deletions(-) delete mode 100644 src/reader.cc delete mode 100644 src/reader.h diff --git a/src/Makefile b/src/Makefile index 8b193f74..cd510478 100644 --- a/src/Makefile +++ b/src/Makefile @@ -26,7 +26,6 @@ SRCS=\ metadatad.cc \ api_server.cc \ configuration.cc \ - reader.cc \ updater.cc \ instance.cc \ docker.cc \ diff --git a/src/docker.cc b/src/docker.cc index 43736af4..18b175c4 100644 --- a/src/docker.cc +++ b/src/docker.cc @@ -48,11 +48,7 @@ DockerReader::DockerReader(const MetadataAgentConfiguration& config) bool DockerReader::ValidateConfiguration() const { try { - const std::string container_filter( - config_.DockerContainerFilter().empty() - ? "" : "&" + config_.DockerContainerFilter()); - - QueryDocker(std::string(kDockerEndpointPath) + "/json?all=true&limit=1" + container_filter); + QueryDocker(std::string(kDockerEndpointPath) + "/json?all=true"); return true; } catch (const QueryException& e) { diff --git a/src/docker.h b/src/docker.h index e5758418..b16b31cb 100644 --- a/src/docker.h +++ b/src/docker.h @@ -22,12 +22,11 @@ #include "configuration.h" #include "environment.h" -#include "reader.h" #include "updater.h" namespace google { -class DockerReader : public MetadataReader { +class DockerReader { public: DockerReader(const MetadataAgentConfiguration& config); // A Docker metadata query function. @@ -70,9 +69,9 @@ class DockerUpdater : public PollingMetadataUpdater { server->config().DockerUpdaterIntervalSeconds(), std::bind(&google::DockerReader::MetadataQuery, &reader_)) { } - // Validates that the reader is configured properly. - // Returns a bool that represents if it's configured properly. + protected: bool ValidateConfiguration() const; + private: DockerReader reader_; }; diff --git a/src/instance.h b/src/instance.h index aa4794d8..19365cac 100644 --- a/src/instance.h +++ b/src/instance.h @@ -22,13 +22,12 @@ #include "configuration.h" #include "environment.h" -#include "reader.h" #include "resource.h" #include "updater.h" namespace google { -class InstanceReader : public MetadataReader { +class InstanceReader { public: InstanceReader(const MetadataAgentConfiguration& config); // A Instance metadata query function. diff --git a/src/kubernetes.cc b/src/kubernetes.cc index b4e0ff97..70f0bdc7 100644 --- a/src/kubernetes.cc +++ b/src/kubernetes.cc @@ -1055,11 +1055,7 @@ bool KubernetesUpdater::ValidateConfiguration() const { return reader_.ValidateConfiguration(); } -bool KubernetesUpdater::start() { - if (!PollingMetadataUpdater::start()) { - return false; - } - +void KubernetesUpdater::StartUpdater() { if (config().KubernetesUseWatch()) { // Wrap the bind expression into a function to use as a bind argument. UpdateCallback cb = std::bind(&KubernetesUpdater::MetadataCallback, this, @@ -1068,9 +1064,9 @@ bool KubernetesUpdater::start() { std::thread(&KubernetesReader::WatchNode, &reader_, cb); pod_watch_thread_ = std::thread(&KubernetesReader::WatchPods, &reader_, cb); + } else { + PollingMetadataUpdater::StartUpdater(); } - - return true; } void KubernetesUpdater::MetadataCallback( diff --git a/src/kubernetes.h b/src/kubernetes.h index 2012b226..3f9e1b7d 100644 --- a/src/kubernetes.h +++ b/src/kubernetes.h @@ -28,12 +28,11 @@ #include "configuration.h" #include "environment.h" #include "json.h" -#include "reader.h" #include "updater.h" namespace google { -class KubernetesReader : public MetadataReader { +class KubernetesReader { public: KubernetesReader(const MetadataAgentConfiguration& config); // A Kubernetes metadata query function. @@ -165,11 +164,9 @@ class KubernetesUpdater : public PollingMetadataUpdater { } } - // Validates that the reader is configured properly. - // Returns a bool that represents if it's configured properly. + protected: bool ValidateConfiguration() const; - - bool start(); + void StartUpdater(); private: // Metadata watcher callback. diff --git a/src/reader.cc b/src/reader.cc deleted file mode 100644 index 8c2e814c..00000000 --- a/src/reader.cc +++ /dev/null @@ -1,25 +0,0 @@ -/* - * 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. - **/ - -#include "reader.h" - -namespace google { - -bool MetadataReader::ValidateConfiguration() const { - return true; -} - -} diff --git a/src/reader.h b/src/reader.h deleted file mode 100644 index 760342fb..00000000 --- a/src/reader.h +++ /dev/null @@ -1,29 +0,0 @@ -/* - * Copyright 2017 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 READER_H_ -#define READER_H_ - -namespace google { - -class MetadataReader { - public: - virtual bool ValidateConfiguration() const; - -}; - -} - -#endif // READER_H_ diff --git a/src/updater.cc b/src/updater.cc index 24a70cb0..2d3b6c5a 100644 --- a/src/updater.cc +++ b/src/updater.cc @@ -22,13 +22,22 @@ namespace google { -MetadataUpdater::MetadataUpdater(MetadataAgent* store, std::string name) +MetadataUpdater::MetadataUpdater(MetadataAgent* store, const std::string& name) : store_(store), name_(name) {} MetadataUpdater::~MetadataUpdater() {} -bool MetadataUpdater::ValidateConfiguration() const { - return true; +void MetadataUpdater::start() { + if (!ValidateConfiguration()) { + LOG(ERROR) << "Failed to validate configuration for " << name_; + return; + } + + StartUpdater(); +} + +void MetadataUpdater::stop() { + StopUpdater(); } PollingMetadataUpdater::PollingMetadataUpdater( @@ -46,25 +55,19 @@ PollingMetadataUpdater::~PollingMetadataUpdater() { } } -bool PollingMetadataUpdater::start() { - if (!ValidateConfiguration()) { - LOG(ERROR) << "Failed to validate configuration for " << name_; - return false; - } - +void PollingMetadataUpdater::StartUpdater() { timer_.lock(); if (config().VerboseLogging()) { LOG(INFO) << "Timer locked"; } + if (period_ > seconds::zero()) { reporter_thread_ = std::thread(&PollingMetadataUpdater::PollForMetadata, this); } - - return true; } -void PollingMetadataUpdater::stop() { +void PollingMetadataUpdater::StopUpdater() { timer_.unlock(); if (config().VerboseLogging()) { LOG(INFO) << "Timer unlocked"; diff --git a/src/updater.h b/src/updater.h index 264ed243..03324f0c 100644 --- a/src/updater.h +++ b/src/updater.h @@ -53,18 +53,28 @@ class MetadataUpdater { return store_->config(); } - virtual bool ValidateConfiguration() const; - // Starts updating. - virtual bool start() = 0; + void start(); // Stops updating. - virtual void stop() = 0; + void stop(); using UpdateCallback = std::function&&)>; protected: + // 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 { + return true; + } + + // Internal method for starting the updater's logic. + virtual void StartUpdater() = 0; + + // Internal method for stopping the updater's logic. + virtual void StopUpdater() = 0; + // Updates the resource map in the store. void UpdateResourceCallback(const ResourceMetadata& result) { store_->UpdateResource(result.ids, result.resource); @@ -90,8 +100,9 @@ class PollingMetadataUpdater : public MetadataUpdater { std::function()> query_metadata); ~PollingMetadataUpdater(); - bool start(); - void stop(); + protected: + void StartUpdater(); + void StopUpdater(); private: using seconds = std::chrono::duration; From a333634f9e35699442fb52eb24221cf58e226484 Mon Sep 17 00:00:00 2001 From: Bryan Moyles Date: Wed, 28 Feb 2018 12:34:28 -0500 Subject: [PATCH 08/14] Limit Docker container request to one container, and validate node AND pod connectivity for kubernetes. --- src/docker.cc | 8 +++++++- src/kubernetes.cc | 17 ++++++++++++++++- src/kubernetes.h | 4 ++++ 3 files changed, 27 insertions(+), 2 deletions(-) diff --git a/src/docker.cc b/src/docker.cc index 18b175c4..0fea2b4e 100644 --- a/src/docker.cc +++ b/src/docker.cc @@ -48,7 +48,13 @@ DockerReader::DockerReader(const MetadataAgentConfiguration& config) bool DockerReader::ValidateConfiguration() const { try { - QueryDocker(std::string(kDockerEndpointPath) + "/json?all=true"); + 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. + QueryDocker(std::string(kDockerEndpointPath) + "/json?all=true&limit=1" + container_filter); return true; } catch (const QueryException& e) { diff --git a/src/kubernetes.cc b/src/kubernetes.cc index 70f0bdc7..4c7d95d1 100644 --- a/src/kubernetes.cc +++ b/src/kubernetes.cc @@ -961,7 +961,18 @@ json::value KubernetesReader::FindTopLevelOwner( return FindTopLevelOwner(ns, GetOwner(ns, ref->As())); } -bool KubernetesReader::ValidateConfiguration() const { +bool KubernetesReader::ValidatePodConnectivity() const { + try { + QueryMaster(std::string(kKubernetesEndpointPath) + "/pods?limit=1"); + + return true; + } catch (const QueryException& e) { + // Already logged. + return false; + } +} + +bool KubernetesReader::ValidateNodeConnectivity() const { const std::string node_name = CurrentNode(); if (node_name.empty()) { return false; @@ -978,6 +989,10 @@ bool KubernetesReader::ValidateConfiguration() const { } } +bool KubernetesReader::ValidateConfiguration() const { + return ValidatePodConnectivity() && ValidateNodeConnectivity(); +} + void KubernetesReader::PodCallback( MetadataUpdater::UpdateCallback callback, const json::Object* pod, Timestamp collected_at, bool is_deleted) const diff --git a/src/kubernetes.h b/src/kubernetes.h index 3f9e1b7d..70993fe5 100644 --- a/src/kubernetes.h +++ b/src/kubernetes.h @@ -49,6 +49,10 @@ class KubernetesReader { void WatchPods(MetadataUpdater::UpdateCallback callback) const; private: + bool ValidatePodConnectivity() const; + + bool ValidateNodeConnectivity() const; + // A representation of all query-related errors. class QueryException { public: From 3e56c5c18532ed5e34e479082c04837f83a4e817 Mon Sep 17 00:00:00 2001 From: Bryan Moyles Date: Wed, 28 Feb 2018 14:24:12 -0500 Subject: [PATCH 09/14] Address feedback. --- src/docker.h | 3 +-- src/instance.h | 3 +-- src/kubernetes.cc | 12 ++++++------ src/kubernetes.h | 3 +-- src/updater.h | 5 +++-- 5 files changed, 12 insertions(+), 14 deletions(-) diff --git a/src/docker.h b/src/docker.h index b16b31cb..13344d9c 100644 --- a/src/docker.h +++ b/src/docker.h @@ -64,8 +64,7 @@ class DockerUpdater : public PollingMetadataUpdater { public: DockerUpdater(MetadataAgent* server) : reader_(server->config()), PollingMetadataUpdater( - server, - "DockerUpdater", + server, "DockerUpdater", server->config().DockerUpdaterIntervalSeconds(), std::bind(&google::DockerReader::MetadataQuery, &reader_)) { } diff --git a/src/instance.h b/src/instance.h index 19365cac..7c3cfd30 100644 --- a/src/instance.h +++ b/src/instance.h @@ -45,8 +45,7 @@ class InstanceUpdater : public PollingMetadataUpdater { public: InstanceUpdater(MetadataAgent* server) : reader_(server->config()), PollingMetadataUpdater( - server, - "InstanceUpdater", + server, "InstanceUpdater", server->config().InstanceUpdaterIntervalSeconds(), std::bind(&google::InstanceReader::MetadataQuery, &reader_)) { } private: diff --git a/src/kubernetes.cc b/src/kubernetes.cc index 4c7d95d1..9420a147 100644 --- a/src/kubernetes.cc +++ b/src/kubernetes.cc @@ -973,20 +973,20 @@ bool KubernetesReader::ValidatePodConnectivity() const { } bool KubernetesReader::ValidateNodeConnectivity() const { - const std::string node_name = CurrentNode(); - if (node_name.empty()) { - return false; - } - try { json::value raw_node = QueryMaster( - std::string(kKubernetesEndpointPath) + "/nodes/" + node_name); + std::string(kKubernetesEndpointPath) + "/nodes?limit=1"); return true; } catch (const QueryException& e) { // Already logged. return false; } + + const std::string node_name = CurrentNode(); + if (node_name.empty()) { + return false; + } } bool KubernetesReader::ValidateConfiguration() const { diff --git a/src/kubernetes.h b/src/kubernetes.h index 70993fe5..27620566 100644 --- a/src/kubernetes.h +++ b/src/kubernetes.h @@ -155,8 +155,7 @@ class KubernetesUpdater : public PollingMetadataUpdater { public: KubernetesUpdater(MetadataAgent* server) : reader_(server->config()), PollingMetadataUpdater( - server, - "KubernetesUpdater", + server, "KubernetesUpdater", server->config().KubernetesUpdaterIntervalSeconds(), std::bind(&google::KubernetesReader::MetadataQuery, &reader_)) { } ~KubernetesUpdater() { diff --git a/src/updater.h b/src/updater.h index 03324f0c..7e068534 100644 --- a/src/updater.h +++ b/src/updater.h @@ -84,10 +84,11 @@ class MetadataUpdater { void UpdateMetadataCallback(ResourceMetadata&& result) { store_->UpdateMetadata(result.resource, std::move(result.metadata)); } - - std::string name_; private: + // The name of the updater provided by subclasses. + std::string name_; + // The store for the metadata. MetadataAgent* store_; }; From 475c035703e88ace2bc4bd26788280cbba67b9bc Mon Sep 17 00:00:00 2001 From: Bryan Moyles Date: Wed, 28 Feb 2018 14:41:33 -0500 Subject: [PATCH 10/14] Address feedback. --- src/docker.cc | 3 ++- src/kubernetes.cc | 27 +++++++++++++-------------- src/kubernetes.h | 4 ---- 3 files changed, 15 insertions(+), 19 deletions(-) diff --git a/src/docker.cc b/src/docker.cc index 0fea2b4e..84244826 100644 --- a/src/docker.cc +++ b/src/docker.cc @@ -54,7 +54,8 @@ bool DockerReader::ValidateConfiguration() const { // A limit may exist in the container_filter, however, the docker API only // uses the first limit provided in the query params. - QueryDocker(std::string(kDockerEndpointPath) + "/json?all=true&limit=1" + container_filter); + (void) QueryDocker(std::string(kDockerEndpointPath) + + "/json?all=true&limit=1" + container_filter); return true; } catch (const QueryException& e) { diff --git a/src/kubernetes.cc b/src/kubernetes.cc index 9420a147..d8ace644 100644 --- a/src/kubernetes.cc +++ b/src/kubernetes.cc @@ -961,36 +961,32 @@ json::value KubernetesReader::FindTopLevelOwner( return FindTopLevelOwner(ns, GetOwner(ns, ref->As())); } -bool KubernetesReader::ValidatePodConnectivity() const { +bool KubernetesReader::ValidateConfiguration() const { try { - QueryMaster(std::string(kKubernetesEndpointPath) + "/pods?limit=1"); + (void) QueryMaster( + std::string(kKubernetesEndpointPath) + "/nodes?limit=1"); return true; } catch (const QueryException& e) { // Already logged. return false; } -} -bool KubernetesReader::ValidateNodeConnectivity() const { + const std::string node_name = CurrentNode(); + if (node_name.empty()) { + return false; + } + try { - json::value raw_node = QueryMaster( - std::string(kKubernetesEndpointPath) + "/nodes?limit=1"); + (void) QueryMaster(std::string(kKubernetesEndpointPath) + "/pods?limit=1"); return true; } catch (const QueryException& e) { // Already logged. return false; } - - const std::string node_name = CurrentNode(); - if (node_name.empty()) { - return false; - } -} -bool KubernetesReader::ValidateConfiguration() const { - return ValidatePodConnectivity() && ValidateNodeConnectivity(); + return true; } void KubernetesReader::PodCallback( @@ -1071,6 +1067,9 @@ bool KubernetesUpdater::ValidateConfiguration() const { } void KubernetesUpdater::StartUpdater() { + // The watch and polling implementations are mutually exclusive. Polling can + // only be used if watch is disabled and there is an established polling + // interval. if (config().KubernetesUseWatch()) { // Wrap the bind expression into a function to use as a bind argument. UpdateCallback cb = std::bind(&KubernetesUpdater::MetadataCallback, this, diff --git a/src/kubernetes.h b/src/kubernetes.h index 27620566..c20984ca 100644 --- a/src/kubernetes.h +++ b/src/kubernetes.h @@ -49,10 +49,6 @@ class KubernetesReader { void WatchPods(MetadataUpdater::UpdateCallback callback) const; private: - bool ValidatePodConnectivity() const; - - bool ValidateNodeConnectivity() const; - // A representation of all query-related errors. class QueryException { public: From 812559cacb3ccd556873b3fc70f9fc54927714ca Mon Sep 17 00:00:00 2001 From: Bryan Moyles Date: Wed, 28 Feb 2018 18:07:10 -0500 Subject: [PATCH 11/14] Address feedback. --- src/docker.cc | 2 +- src/kubernetes.cc | 13 +++---------- 2 files changed, 4 insertions(+), 11 deletions(-) diff --git a/src/docker.cc b/src/docker.cc index 84244826..7365129f 100644 --- a/src/docker.cc +++ b/src/docker.cc @@ -55,7 +55,7 @@ bool DockerReader::ValidateConfiguration() const { // 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); + "/json?all=true&limit=1" + container_filter); return true; } catch (const QueryException& e) { diff --git a/src/kubernetes.cc b/src/kubernetes.cc index d8ace644..711eb3b3 100644 --- a/src/kubernetes.cc +++ b/src/kubernetes.cc @@ -965,22 +965,17 @@ bool KubernetesReader::ValidateConfiguration() const { try { (void) QueryMaster( std::string(kKubernetesEndpointPath) + "/nodes?limit=1"); - - return true; } catch (const QueryException& e) { // Already logged. return false; } - const std::string node_name = CurrentNode(); - if (node_name.empty()) { + if (CurrentNode().empty()) { return false; } - + try { (void) QueryMaster(std::string(kKubernetesEndpointPath) + "/pods?limit=1"); - - return true; } catch (const QueryException& e) { // Already logged. return false; @@ -1067,9 +1062,6 @@ bool KubernetesUpdater::ValidateConfiguration() const { } void KubernetesUpdater::StartUpdater() { - // The watch and polling implementations are mutually exclusive. Polling can - // only be used if watch is disabled and there is an established polling - // interval. if (config().KubernetesUseWatch()) { // Wrap the bind expression into a function to use as a bind argument. UpdateCallback cb = std::bind(&KubernetesUpdater::MetadataCallback, this, @@ -1079,6 +1071,7 @@ void KubernetesUpdater::StartUpdater() { pod_watch_thread_ = std::thread(&KubernetesReader::WatchPods, &reader_, cb); } else { + // Only try to poll if watch is disabled. PollingMetadataUpdater::StartUpdater(); } } From 9b375409a22a03e0c45546375310ed16aa7ca408 Mon Sep 17 00:00:00 2001 From: Bryan Moyles Date: Wed, 28 Feb 2018 18:26:44 -0500 Subject: [PATCH 12/14] Address feedback. --- src/kubernetes.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/kubernetes.cc b/src/kubernetes.cc index 711eb3b3..5e1536aa 100644 --- a/src/kubernetes.cc +++ b/src/kubernetes.cc @@ -963,8 +963,7 @@ json::value KubernetesReader::FindTopLevelOwner( bool KubernetesReader::ValidateConfiguration() const { try { - (void) QueryMaster( - std::string(kKubernetesEndpointPath) + "/nodes?limit=1"); + (void) QueryMaster(std::string(kKubernetesEndpointPath) + "/nodes?limit=1"); } catch (const QueryException& e) { // Already logged. return false; From c5fbc290f334129591c059cba728b12dedd8bac8 Mon Sep 17 00:00:00 2001 From: Bryan Moyles Date: Thu, 1 Mar 2018 14:31:24 -0500 Subject: [PATCH 13/14] Address feedback. --- src/kubernetes.cc | 19 ++++++++++++++----- src/updater.cc | 10 ++++++---- src/updater.h | 1 + 3 files changed, 21 insertions(+), 9 deletions(-) diff --git a/src/kubernetes.cc b/src/kubernetes.cc index 5e1536aa..30d5b421 100644 --- a/src/kubernetes.cc +++ b/src/kubernetes.cc @@ -969,17 +969,22 @@ bool KubernetesReader::ValidateConfiguration() const { return false; } - if (CurrentNode().empty()) { - return false; - } - try { - (void) QueryMaster(std::string(kKubernetesEndpointPath) + "/pods?limit=1"); + 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; } + if (CurrentNode().empty()) { + return false; + } + return true; } @@ -1057,6 +1062,10 @@ void KubernetesReader::WatchNode(MetadataUpdater::UpdateCallback callback) } bool KubernetesUpdater::ValidateConfiguration() const { + if (!PollingMetadataUpdater::ValidateConfiguration()) { + return false; + } + return reader_.ValidateConfiguration(); } diff --git a/src/updater.cc b/src/updater.cc index 2d3b6c5a..def46ee2 100644 --- a/src/updater.cc +++ b/src/updater.cc @@ -55,16 +55,18 @@ PollingMetadataUpdater::~PollingMetadataUpdater() { } } +bool PollingMetadataUpdater::ValidateConfiguration() const { + return period_ > seconds::zero(); +} + void PollingMetadataUpdater::StartUpdater() { timer_.lock(); if (config().VerboseLogging()) { LOG(INFO) << "Timer locked"; } - if (period_ > seconds::zero()) { - reporter_thread_ = - std::thread(&PollingMetadataUpdater::PollForMetadata, this); - } + reporter_thread_ = + std::thread(&PollingMetadataUpdater::PollForMetadata, this); } void PollingMetadataUpdater::StopUpdater() { diff --git a/src/updater.h b/src/updater.h index 7e068534..ff013d72 100644 --- a/src/updater.h +++ b/src/updater.h @@ -102,6 +102,7 @@ class PollingMetadataUpdater : public MetadataUpdater { ~PollingMetadataUpdater(); protected: + bool ValidateConfiguration() const; void StartUpdater(); void StopUpdater(); From 275bfbb490d0bc7989d691b288f8ab2c07735c5d Mon Sep 17 00:00:00 2001 From: Bryan Moyles Date: Thu, 1 Mar 2018 16:15:15 -0500 Subject: [PATCH 14/14] Address feedback. --- src/docker.cc | 4 ++++ src/updater.cc | 9 +++++---- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/src/docker.cc b/src/docker.cc index 7365129f..39a4ece2 100644 --- a/src/docker.cc +++ b/src/docker.cc @@ -206,6 +206,10 @@ json::value DockerReader::QueryDocker(const std::string& path) const } bool DockerUpdater::ValidateConfiguration() const { + if (!PollingMetadataUpdater::ValidateConfiguration()) { + return false; + } + return reader_.ValidateConfiguration(); } diff --git a/src/updater.cc b/src/updater.cc index def46ee2..e7593c5e 100644 --- a/src/updater.cc +++ b/src/updater.cc @@ -56,7 +56,7 @@ PollingMetadataUpdater::~PollingMetadataUpdater() { } bool PollingMetadataUpdater::ValidateConfiguration() const { - return period_ > seconds::zero(); + return period_ >= seconds::zero(); } void PollingMetadataUpdater::StartUpdater() { @@ -64,9 +64,10 @@ void PollingMetadataUpdater::StartUpdater() { if (config().VerboseLogging()) { LOG(INFO) << "Timer locked"; } - - reporter_thread_ = - std::thread(&PollingMetadataUpdater::PollForMetadata, this); + if (period_ > seconds::zero()) { + reporter_thread_ = + std::thread(&PollingMetadataUpdater::PollForMetadata, this); + } } void PollingMetadataUpdater::StopUpdater() {