Skip to content
This repository was archived by the owner on Aug 19, 2019. It is now read-only.

Memoize owner objects instead of querying the master all the time. #46

Merged
igorpeshansky merged 2 commits into from
Dec 20, 2017
Merged

Memoize owner objects instead of querying the master all the time. #46

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
94f1019
Memoize owner objects instead of querying the master all the time.
igorpeshansky Nov 17, 2017
7dce675
Store and look up owner references by uid instead of by name.
igorpeshansky Dec 20, 2017
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
39 changes: 34 additions & 5 deletions src/kubernetes.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -527,13 +527,42 @@ json::value KubernetesReader::GetOwner(
const std::string api_version = owner_ref->Get<json::String>("apiVersion");
const std::string kind = owner_ref->Get<json::String>("kind");
const std::string name = owner_ref->Get<json::String>("name");
const auto path_component = KindPath(api_version, kind);
const std::string uid = owner_ref->Get<json::String>("uid");

// Even though we query by name, we should look up the owner by uid,
// to handle the case when an object is deleted and re-constructed.
const std::string encoded_ref = boost::algorithm::join(
std::vector<std::string>{api_version, kind, uid}, "/");

std::lock_guard<std::recursive_mutex> lock(mutex_);
auto found = owners_.emplace(std::piecewise_construct,
std::forward_as_tuple(encoded_ref),
std::forward_as_tuple());
json::value& owner = found.first->second;
if (found.second) { // Not found, inserted new.
const auto path_component = KindPath(api_version, kind);
#ifdef VERBOSE
LOG(DEBUG) << "KindPath returned {" << path_component.first << ", "
<< path_component.second << "}";
LOG(DEBUG) << "KindPath returned {" << path_component.first << ", "
<< path_component.second << "}";
#endif
return QueryMaster(path_component.first + "/namespaces/" + ns + "/" +
path_component.second + "/" + name);
owner = std::move(
QueryMaster(path_component.first + "/namespaces/" + ns + "/" +
Copy link
Contributor

@bmoyles0117 bmoyles0117 Dec 20, 2017

Choose a reason for hiding this comment

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

Is it possible that this throws an exception, without killing the thread, which would end up leaving owner null, which would end up throwing another exception the next time due to owner->Clone() being on a nullptr? If so, can we add a sanity check of if (owner == nullptr || found.second) {?

Copy link
Contributor Author

@igorpeshansky igorpeshansky Dec 20, 2017

Choose a reason for hiding this comment

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

An exception will not allow the rest of this function to continue -- it'll be propagated up the stack, and caught, likely in KubernetesReader::MetadataQuery. So such a check would be superfluous.

path_component.second + "/" + name));
// Sanity check: because we are looking up by name, the object we get
// back might have a different uid.
const json::Object* owner_obj = owner->As<json::Object>();
Copy link
Contributor

@bmoyles0117 bmoyles0117 Dec 20, 2017

Choose a reason for hiding this comment

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

Nit: the naming of the variables seems inconsistent. owner_uid prevents a clash with uid, so perhaps we should have owner_metadata for consistency? Not important to address this.

Copy link
Contributor Author

@igorpeshansky igorpeshansky Dec 20, 2017

Choose a reason for hiding this comment

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

Funny you should mention that -- I initially had it as owner_metadata, but that caused some ugly line breaks, so I went with just metadata. I can change it if you insist.

Copy link
Contributor

@bmoyles0117 bmoyles0117 Dec 20, 2017

Choose a reason for hiding this comment

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

SGTM!

const json::Object* metadata = owner_obj->Get<json::Object>("metadata");
const std::string owner_uid = metadata->Get<json::String>("uid");
if (owner_uid != uid) {
LOG(WARNING) << "Owner " << kind << "'" << name << "' (id " << uid
<< ") disappeared before we could query it. Found id "
<< owner_uid << " instead.";
owner.reset();
throw QueryException("Owner " + kind + " " + name + " (id " + uid +
") disappeared");
}
}
return owner->Clone();
}

json::value KubernetesReader::FindTopLevelOwner(
Expand Down
2 changes: 2 additions & 0 deletions src/kubernetes.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,8 @@ class KubernetesReader {
// A memoized map from version to a map from kind to name.
mutable std::map<std::string, std::map<std::string, std::string>>
version_to_kind_to_name_;
// A memoized map from an encoded owner reference to the owner object.
mutable std::map<std::string, json::value> owners_;

const MetadataAgentConfiguration& config_;
Environment environment_;
Expand Down