Skip to content
This repository was archived by the owner on Aug 19, 2019. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ CXXFLAGS=\
LDFLAGS=-L$(CPP_NETLIB_LIBDIR) -L$(NETWORK_URI_LIBDIR) -L$(YAML_CPP_LIBDIR)
LDLIBS=\
-lcppnetlib-client-connections -lcppnetlib-server-parsers -lnetwork-uri \
-lboost_program_options -lboost_system -lboost_thread \
-lboost_program_options -lboost_system -lboost_thread -lboost_filesystem \
-lpthread -lyajl -lssl -lcrypto -lyaml-cpp
SED_EXTRA=-e 's/-Wall/-Wall -Wno-deprecated/'

Expand Down Expand Up @@ -59,6 +59,7 @@ SRCS=\
time.cc \
base64.cc \
format.cc \
health_checker.cc \
environment.cc
OBJS=$(SRCS:%.cc=%.o)

Expand Down
3 changes: 2 additions & 1 deletion src/agent.cc
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,12 @@
#include "configuration.h"
#include "api_server.h"
#include "reporter.h"
#include "health_checker.h"

namespace google {

MetadataAgent::MetadataAgent(const MetadataAgentConfiguration& config)
: config_(config), store_(config_) {}
: config_(config), store_(config_), health_checker_(config) {}

MetadataAgent::~MetadataAgent() {}

Expand Down
6 changes: 6 additions & 0 deletions src/agent.h
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
#include <memory>

#include "store.h"
#include "health_checker.h"

namespace google {

Expand Down Expand Up @@ -52,8 +53,13 @@ class MetadataAgent {
return &store_;
}

HealthChecker* health_checker() {
return &health_checker_;
}

private:
const MetadataAgentConfiguration& config_;
HealthChecker health_checker_;

// The store for the metadata.
MetadataStore store_;
Expand Down
8 changes: 7 additions & 1 deletion src/configuration.cc
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,9 @@ constexpr const bool kKubernetesDefaultUseWatch = true;
constexpr const bool kKubernetesDefaultClusterLevelMetadata = false;
constexpr const char kDefaultInstanceId[] = "";
constexpr const char kDefaultInstanceZone[] = "";
constexpr const char kDefaultHealthCheckFile[] =
"/var/run/metadata-agent/health/unhealthy";

}

MetadataAgentConfiguration::MetadataAgentConfiguration()
Expand Down Expand Up @@ -110,7 +113,8 @@ MetadataAgentConfiguration::MetadataAgentConfiguration()
kubernetes_cluster_level_metadata_(
kKubernetesDefaultClusterLevelMetadata),
instance_id_(kDefaultInstanceId),
instance_zone_(kDefaultInstanceZone) {}
instance_zone_(kDefaultInstanceZone),
health_check_file_(kDefaultHealthCheckFile) {}

int MetadataAgentConfiguration::ParseArguments(int ac, char** av) {
std::string config_file;
Expand Down Expand Up @@ -240,6 +244,8 @@ void MetadataAgentConfiguration::ParseConfiguration(std::istream& input) {
config["InstanceId"].as<std::string>(kDefaultInstanceId);
instance_zone_ =
config["InstanceZone"].as<std::string>(kDefaultInstanceZone);
health_check_file_ =
config["HealthCheckFile"].as<std::string>(kDefaultHealthCheckFile);
}

} // google
Expand Down
7 changes: 7 additions & 0 deletions src/configuration.h
Original file line number Diff line number Diff line change
Expand Up @@ -151,8 +151,14 @@ class MetadataAgentConfiguration {
return instance_zone_;
}

const std::string& HealthCheckFile() const {
std::lock_guard<std::mutex> lock(mutex_);
return health_check_file_;
}

private:
friend class MetadataAgentConfigurationTest;
friend class HealthCheckerUnittest;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this test need to be a friend?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the health check unit tests I set the health check file location. This is set via the configuration object, so I need the test to be a friend so that I can pass it a stream rather than giving an actual file.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thought about this some more... This isn't the only place we will want to parse configuration in a test. We have a good approach in configuration_unittest, namely a static function that parses a config from a string. Would it make sense to factor that out into a config_test_helper and use that whenever we need that functionality? If we go with that approach, we'd want a separate unit test for that helper.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with your suggestion, but I would create a separate PR for this, and track that as an independent bug, and potentially rebase this onto master once the prerequisite PR has been merged. I'll need to do the same with my Kubernetes tests.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I agree, I think its best left for a separate commit.

Copy link
Copy Markdown
Contributor

@igorpeshansky igorpeshansky Mar 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, great. I'm glad we're all on the same page. Let's finish implementing this test, and then refactor to create a helper (in a separate PR) before we invest too much more time into replicating this code in other tests?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep


void ParseConfigFile(const std::string& filename);
void ParseConfiguration(std::istream& input);
Expand Down Expand Up @@ -186,6 +192,7 @@ class MetadataAgentConfiguration {
bool kubernetes_cluster_level_metadata_;
std::string instance_id_;
std::string instance_zone_;
std::string health_check_file_;
};

}
Expand Down
50 changes: 50 additions & 0 deletions src/health_checker.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
/*
* 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 "health_checker.h"

#include <boost/filesystem.hpp>
#include <boost/algorithm/string/join.hpp>
#include <vector>
#include <fstream>

namespace google {

HealthChecker::HealthChecker(const MetadataAgentConfiguration& config)
: config_(config) {
boost::filesystem::create_directories(
boost::filesystem::path(config_.HealthCheckFile()).parent_path());
std::remove(config_.HealthCheckFile().c_str());
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this is just defensive programming in case we are running in space with a non-ephemeral filesystem?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. We don't want a brand new agent process to start out unhealthy.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. We want to make sure the "unhealthy" flag is not set at startup.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM

}

void HealthChecker::SetUnhealthy(const std::string& component) {
std::lock_guard<std::mutex> lock(mutex_);
unhealthy_components_.insert(component);
std::ofstream health_file(config_.HealthCheckFile());
health_file << std::endl;
}

bool HealthChecker::IsHealthy() const {
std::lock_guard<std::mutex> lock(mutex_);
return unhealthy_components_.empty();
}

void HealthChecker::CleanupForTest() {
boost::filesystem::remove_all(boost::filesystem::path(
config_.HealthCheckFile()).parent_path());
}

} // namespace google
46 changes: 46 additions & 0 deletions src/health_checker.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
/*
* 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 HEALTH_CHECKER_H_
#define HEALTH_CHECKER_H_

#include <string>
#include <mutex>
#include <set>

#include "configuration.h"

namespace google {

// Collects and reports health information about the metadata agent.
class HealthChecker {
public:
HealthChecker(const MetadataAgentConfiguration& config);
void SetUnhealthy(const std::string& component);

private:
friend class HealthCheckerUnittest;

bool IsHealthy() const;
void CleanupForTest();

const MetadataAgentConfiguration& config_;
std::set<std::string> unhealthy_components_;
mutable std::mutex mutex_;
};

} // namespace google

#endif // HEALTH_CHECKER_H_
11 changes: 8 additions & 3 deletions src/kubernetes.cc
Original file line number Diff line number Diff line change
Expand Up @@ -89,8 +89,9 @@ bool ReadServiceAccountSecret(

}

KubernetesReader::KubernetesReader(const MetadataAgentConfiguration& config)
: config_(config), environment_(config) {}
KubernetesReader::KubernetesReader(const MetadataAgentConfiguration& config,
HealthChecker* health_checker)
: config_(config), environment_(config), health_checker_(health_checker) {}

MetadataUpdater::ResourceMetadata KubernetesReader::GetNodeMetadata(
const json::Object* node, Timestamp collected_at, bool is_deleted) const
Expand Down Expand Up @@ -1103,6 +1104,7 @@ void KubernetesReader::PodCallback(
void KubernetesReader::WatchPods(
const std::string& node_name,
MetadataUpdater::UpdateCallback callback) const {
HealthChecker health_reporter(config_);
LOG(INFO) << "Watch thread (pods) started for node "
<< (node_name.empty() ? "<unscheduled>" : node_name);

Expand All @@ -1125,6 +1127,7 @@ void KubernetesReader::WatchPods(
} catch (const KubernetesReader::QueryException& e) {
LOG(ERROR) << "No more pod metadata will be collected";
}
health_checker_->SetUnhealthy("kubernetes_pod_thread");
LOG(INFO) << "Watch thread (pods) exiting";
}

Expand Down Expand Up @@ -1157,12 +1160,14 @@ void KubernetesReader::WatchNodes(
} catch (const KubernetesReader::QueryException& e) {
LOG(ERROR) << "No more node metadata will be collected";
}
health_checker_->SetUnhealthy("kubernetes_node_thread");
LOG(INFO) << "Watch thread (node) exiting";
}

KubernetesUpdater::KubernetesUpdater(const MetadataAgentConfiguration& config,
HealthChecker* health_checker,
MetadataStore* store)
: reader_(config), PollingMetadataUpdater(
: reader_(config, health_checker), PollingMetadataUpdater(
config, store, "KubernetesUpdater",
config.KubernetesUpdaterIntervalSeconds(),
[=]() { return reader_.MetadataQuery(); }) { }
Expand Down
7 changes: 6 additions & 1 deletion src/kubernetes.h
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
#include <vector>

#include "environment.h"
#include "health_checker.h"
#include "json.h"
#include "updater.h"

Expand All @@ -37,7 +38,8 @@ class MetadataStore;

class KubernetesReader {
public:
KubernetesReader(const MetadataAgentConfiguration& config);
KubernetesReader(const MetadataAgentConfiguration& config,
HealthChecker* health_checker);
// A Kubernetes metadata query function.
std::vector<MetadataUpdater::ResourceMetadata> MetadataQuery() const;

Expand Down Expand Up @@ -151,12 +153,14 @@ class KubernetesReader {
mutable std::map<std::string, json::value> owners_;

const MetadataAgentConfiguration& config_;
HealthChecker* health_checker_;
Environment environment_;
};

class KubernetesUpdater : public PollingMetadataUpdater {
public:
KubernetesUpdater(const MetadataAgentConfiguration& config,
HealthChecker* health_checker,
MetadataStore* store);
~KubernetesUpdater() {
if (node_watch_thread_.joinable()) {
Expand All @@ -176,6 +180,7 @@ class KubernetesUpdater : public PollingMetadataUpdater {
void MetadataCallback(std::vector<ResourceMetadata>&& result_vector);

KubernetesReader reader_;
HealthChecker* health_checker_;
std::thread node_watch_thread_;
std::thread pod_watch_thread_;
};
Expand Down
2 changes: 1 addition & 1 deletion src/metadatad.cc
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ int main(int ac, char** av) {

google::InstanceUpdater instance_updater(config, server.mutable_store());
google::DockerUpdater docker_updater(config, server.mutable_store());
google::KubernetesUpdater kubernetes_updater(config, server.mutable_store());
google::KubernetesUpdater kubernetes_updater(config, server.health_checker(), server.mutable_store());

instance_updater.start();
docker_updater.start();
Expand Down
3 changes: 3 additions & 0 deletions test/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ TESTS=\
base64_unittest \
configuration_unittest \
format_unittest \
health_checker_unittest \
resource_unittest \
time_unittest

Expand Down Expand Up @@ -84,5 +85,7 @@ resource_unittest: $(GTEST_LIB) resource_unittest.o $(SRC_DIR)/resource.o $(SRC_
$(CXX) $(LDFLAGS) $^ $(LDLIBS) -o $@
time_unittest: $(GTEST_LIB) time_unittest.o $(SRC_DIR)/time.o
$(CXX) $(LDFLAGS) $^ $(LDLIBS) -o $@
health_checker_unittest: $(GTEST_LIB) health_checker_unittest.o $(SRC_DIR)/health_checker.o $(SRC_DIR)/configuration.o
$(CXX) $(LDFLAGS) $^ $(LDLIBS) -lboost_filesystem -lboost_system -o $@

.PHONY: all test clean purge
3 changes: 3 additions & 0 deletions test/configuration_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ class MetadataAgentConfigurationTest : public ::testing::Test {
EXPECT_EQ(true, config.KubernetesUseWatch());
EXPECT_EQ("", config.InstanceId());
EXPECT_EQ("", config.InstanceZone());
EXPECT_EQ("/var/run/metadata-agent/health/unhealthy", config.HealthCheckFile());
}

MetadataAgentConfiguration config;
Expand All @@ -63,11 +64,13 @@ TEST_F(MetadataAgentConfigurationTest, PopulatedConfig) {
"MetadataApiNumThreads: 13\n"
"MetadataReporterPurgeDeleted: true\n"
"MetadataReporterUserAgent: \"foobar/foobaz\"\n"
"HealthCheckFile: /a/b/c\n"
);
EXPECT_EQ("TestProjectId", config.ProjectId());
EXPECT_EQ(13, config.MetadataApiNumThreads());
EXPECT_EQ(true, config.MetadataReporterPurgeDeleted());
EXPECT_EQ("foobar/foobaz", config.MetadataReporterUserAgent());
EXPECT_EQ("/a/b/c", config.HealthCheckFile());
}

TEST_F(MetadataAgentConfigurationTest, CommentSkipped) {
Expand Down
Loading